feat: improve details view with tree hierarchy (go4dot-3re)#123
feat: improve details view with tree hierarchy (go4dot-3re)#123nvandessel merged 1 commit intomainfrom
Conversation
- Replace flat indentation with proper tree connectors (├─, └─, │) - Add PATHS section showing source and destination directories - Inline linked/total stats in FILESYSTEM MAPPINGS header - Use tree connectors for external repositories section - Better visual consolidation throughout the details panel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe details panel rendering in the dashboard has been refactored to improve visual hierarchy and information organization. Tree rendering now uses structural connectors (├─, └─, │) for file mappings, a PATHS section displays source and destination directories, external dependencies use a two-column tree layout, and the filesystem stats section now includes dynamic status indicators. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR improves the details panel by replacing the flat file list with a proper tree hierarchy using box-drawing connectors ( Key observations:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| internal/ui/dashboard/details_panel.go | Adds a PATHS section to the details panel, upgrades the file tree to use proper box-drawing connectors (├─, └─, │), and moves the linked/total stats inline with the FILESYSTEM MAPPINGS header. Contains a logical bug where 0/0 files renders as a green success badge, a hardcoded $HOME destination, and a Go slice-aliasing pattern worth cleaning up. VHS visual tests are not included despite being mandatory for TUI changes per AGENTS.md. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
RC[renderConfigDetails] --> PATHS["PATHS section\n(Source + Dest via os.Getenv('HOME'))"]
RC --> FM["FILESYSTEM MAPPINGS header\n+ statsTag [linked/total]"]
FM --> BFT[buildFileTree\nfiles → fileTreeNode tree]
BFT --> RFT["renderFileTree\nprefix='', isRoot=true"]
RFT --> SORT["Sort: dirs first,\nthen files alphabetically"]
SORT --> ITER["For each child:\nisLast? → connector ├─ or └─"]
ITER -->|isDir| DIR["Render: linePrefix + connector + name/\nRecurse with childPrefix, isRoot=false"]
ITER -->|isFile| FILE["Determine icon:\n✓ okStyle\n⚠ warnStyle (conflict/exists/elsewhere)\n✗ errStyle"]
FILE --> ISSUE["If !isLinked && issue != ''\nRender: childPrefix + → + issue"]
RC --> DEPS[MODULE DEPENDENCIES]
RC --> EXTDEPS["EXTERNAL REPOSITORIES\ntree connectors ├─/└─"]
Last reviewed commit: 5f4decc
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if linkStatus != nil { | ||
| lines = append(lines, headerStyle.Render("FILESYSTEM MAPPINGS")) | ||
| linked := linkStatus.LinkedCount | ||
| total := linkStatus.TotalCount | ||
| var statsTag string | ||
| if linked == total { | ||
| statsTag = okStyle.Render(fmt.Sprintf(" [%d/%d ✓]", linked, total)) | ||
| } else { | ||
| statsTag = warnStyle.Render(fmt.Sprintf(" [%d/%d]", linked, total)) | ||
| } |
There was a problem hiding this comment.
False success badge when TotalCount is 0
When linked == total == 0 (i.e., a config with no files has been loaded into linkStatus), this branch renders a green [0/0 ✓] badge, incorrectly indicating a fully-linked state. The existing IsFullyLinked() helper in stow/status.go already guards against this with && s.TotalCount > 0, but this inline check omits that guard.
| if linkStatus != nil { | |
| lines = append(lines, headerStyle.Render("FILESYSTEM MAPPINGS")) | |
| linked := linkStatus.LinkedCount | |
| total := linkStatus.TotalCount | |
| var statsTag string | |
| if linked == total { | |
| statsTag = okStyle.Render(fmt.Sprintf(" [%d/%d ✓]", linked, total)) | |
| } else { | |
| statsTag = warnStyle.Render(fmt.Sprintf(" [%d/%d]", linked, total)) | |
| } | |
| if linkStatus != nil { | |
| linked := linkStatus.LinkedCount | |
| total := linkStatus.TotalCount | |
| var statsTag string | |
| if total > 0 && linked == total { | |
| statsTag = okStyle.Render(fmt.Sprintf(" [%d/%d ✓]", linked, total)) | |
| } else { | |
| statsTag = warnStyle.Render(fmt.Sprintf(" [%d/%d]", linked, total)) | |
| } |
| home := os.Getenv("HOME") | ||
| if home != "" { | ||
| lines = append(lines, fmt.Sprintf("%s %s", | ||
| subtleStyle.Render("Dest: "), | ||
| pathStyle.Render(home))) | ||
| } |
There was a problem hiding this comment.
Destination hardcoded to $HOME regardless of stow target
os.Getenv("HOME") is always used as the destination, but the actual GNU stow target directory is not necessarily $HOME for all configs and setups. If the stow target is configurable (e.g., via a flag or the state), this will always display the wrong path. The p.state already carries DotfilesPath — does it also carry the stow target? If so, that should be used here instead to accurately reflect where symlinks land.
Additionally, even when $HOME is correct as a base, showing only the root directory as "Dest" is a bit coarse — all the individual file destinations can be inferred by the tree below, so consider whether this label adds enough clarity or may confuse users into thinking files are symlinked at $HOME rather than inside it.
| // Combine into ordered list for proper connector rendering | ||
| allNames := append(dirs, files...) | ||
| totalChildren := len(allNames) |
There was a problem hiding this comment.
Potential slice aliasing with append(dirs, files...)
In Go, append(dirs, files...) will reuse dirs's underlying array if it has sufficient capacity, making allNames alias dirs. While dirs is not modified after this point and there's no runtime bug here today, this is a classic Go footgun. If future code ever modifies dirs after this line (e.g., for a secondary sort pass or debug output), it will silently corrupt allNames. The idiomatic safe pattern is to allocate explicitly:
| // Combine into ordered list for proper connector rendering | |
| allNames := append(dirs, files...) | |
| totalChildren := len(allNames) | |
| allNames := make([]string, 0, len(dirs)+len(files)) | |
| allNames = append(allNames, dirs...) | |
| allNames = append(allNames, files...) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 49.30% 49.37% +0.06%
==========================================
Files 110 110
Lines 12946 12977 +31
==========================================
+ Hits 6383 6407 +24
- Misses 6563 6570 +7
🚀 New features to boost your workflow:
|
1 participant
Summary
Source
Test plan
go test ./...passed (1 pre-existing failure tracked in go-56z)Summary by CodeRabbit
New Features
Improvements