Skip to content

emrg: VS Code-style file tree — icons/selection/compact rows/indent guides/tab merge (rant 2026-08-12T17:28:19) - #707

Merged
argszero merged 2 commits into
masterfrom
feature/file-tree-vscode-style
Aug 12, 2026
Merged

emrg: VS Code-style file tree — icons/selection/compact rows/indent guides/tab merge (rant 2026-08-12T17:28:19)#707
argszero merged 2 commits into
masterfrom
feature/file-tree-vscode-style

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fixes the workspace file-tree display quality gap reported in rant 2026-08-12T17:28:19 ("文件树 UI 对齐 VS Code"). The right-side 文件 tab tree now aligns with VS Code Explorer:

  1. File-type icons (mono, currentColor) — inline SVGs, no icon-library dependency (Electron offline-safe). Folders: closed/expanded folder glyphs; files: ext-mapped icons (js/ts/py/md/json/css/html/yml/images/text) with a generic document fallback.
  2. Depth indent — each row uses padding-left: 8 + depth*16px (root=8px), replacing the loose nested-margin layout.
  3. Selection state — clicking a file row adds .active (bg-soft + 2px accent left bar, single-select); dir rows stay unhighlighted when expanding.
  4. Compact rows — fixed 24px row height, full-row hover.
  5. Ellipsis.ft-name overflow hidden + text-overflow ellipsis + nowrap.
  6. Indent guide lines.ft-kids::before 1px vertical line aligned to the parent's icon column (nested structure forms the line naturally).
  7. Expanded-state persistenceMap<path, expanded> survives re-renders (VS Code behavior), cleared on session switch.
  8. Scrollbar hover display.result-files overlay scrollbar, thumb transparent until the tree area is hovered (existing --scrollbar-thumb token).
  9. File-tab row merge#result-tabbar moved inside .result-tabs so opened file tabs sit on the same row as 文件/产物 (VS Code workspace-tab style); renderTabbar() logic unchanged.

Structural fix: .ft-row now uses flex-wrap: wrap with .ft-kids { flex-basis: 100% } so children render below the parent row while keeping the DOM nesting (previously the kids block laid out inline beside the name — the root cause of the poor display).

Verification: GUI 221 → 225 (4 new renderer smoke tests: icon mapping + selection + depth indent + expanded persistence/tab-merge CSS assertions); pytest 730 unchanged; import + CLI OK. Agent.md test-count breakdown synced.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

CI check present and passing (test run 31585565683, 1m12s — includes actionlint gate + doc-count guard; Agent.md 225 breakdown synced). Locally verified: GUI 225/225, pytest 730, import + CLI OK. Diff: file-tree.js rewrite (mono inline-SVG icons, depth indent 8/24/40, single-select active, expanded-state Map), layout.css (.ft-row flex-wrap + .ft-kids flex-basis:100% fixing the inline-children layout bug; 24px rows, ellipsis, indent guides, hover scrollbar), index.html tabbar merged into .result-tabs row. No regressions in existing FileTree/ResultPanel tests.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Needs fix: expanded-directory children overlap the next sibling row (fixed-height flex-wrap layout)

Reviewed via checkout + headless-Chrome reproduction (pixel-verified, not just jsdom).

What the code does well: icon system (mono inline SVG, currentColor), depth indent (8+depth*16), selection state, ellipsis, indent guides, expanded-state persistence, tabbar merge into .result-tabs — all coherent; GUI tests 225 pass (4 new smoke tests); Agent.md count synced.

The bug: .ft-row is display:flex; flex-wrap:wrap; height:24px with .ft-kids { flex-basis:100% }. The kids flex line is laid out inside the row's fixed 24px box and overflows below it, but the next sibling .ft-row is placed at the parent's 24px bottom edge — i.e. exactly where the first child of the expanded directory starts. The sibling paints over that child; with .active/:hover background (--bg-soft) it fully occludes it.

Pixel-verified with headless Chrome on the PR's exact CSS:

  • parent row y=0-24; kids a.py/b.py/c.py overflow y=24-96
  • next sibling (README.md, .active) laid out at y=24-48 — identical to a.py's band; its blue active background completely covers a.py's row
  • b.py/c.py render below (y=48-96), so the occlusion is exactly the first child of every expanded directory that has a following sibling

Root cause: a definite height on a wrapping flex container makes the second flex line overflow while siblings still position at the container's box edge.

Suggested fix (pick one):

  1. Preferred — restructure the row: .ft-row { display:block } + wrap icon/name in an inner .ft-head { display:flex; align-items:center; height:24px }; .ft-kids then flows in normal block layout below (row auto-height, no overlap). JS change: append a header div instead of appending icon/name directly to the row.
  2. Minimal — drop the fixed height (height:auto; min-height:24px on .ft-row): the row grows to contain both flex lines, so the sibling starts below the kids. Tradeoff: the dir row's hover background then spans its subtree.

Also worth a renderer smoke test that asserts the sibling's offsetTop >= parent's offsetTop + offsetHeight (jsdom offsetHeight of 24 may not reflect the overflow — so this needs a layout-capable assertion or at least a CSS contract test pinning .ft-row to block flow with an inner head wrapper).

@pm25coder

Copy link
Copy Markdown
Contributor

I tested this PR (commit 4c4faf8) end-to-end and reviewed the full diff. Solid work — the VS Code-style alignment is faithful and the new state handling is correct.

Verification (all green)

  • GUI: npm test225/225 pass (renderer smoke 100 → 104 = the 4 new P3.5 tests all pass; rest of suite unchanged). Agent.md's 221 → 225 update matches exactly.
  • pytest: 673 passed / 57 skipped (730) — unchanged, including the doc-count guard.
  • node --check on file-tree.js: OK. Import/CLI OK.

Code review notes (all confirmed correct)

  1. Icon system is safe: iconSvg injects only the static ICON whitelist — no user input reaches innerHTML. Extension mapping lowercases before lookup (.PNG → fileImg ✓); dirs are checked before the extension split, so dir names containing dots are unaffected.
  2. No duplicate rows on cache re-expand: expandDir clears kidsEl.innerHTML unconditionally (line 108, outside the load guard) before re-appending, so expand → collapse → expand renders exactly one copy per entry. The DOM round-trip assertion in the new test covers this.
  3. Selection semantics match VS Code: single-select on files only; directory clicks don't clear the file selection — correct.
  4. Tab merge: #result-tabbar nested inside #result-tabs before #result-toggle, with the old full-width padding/border-bottom removed so it sits inline with the 文件/产物 tabs. The HTML-order assertion is a good regression anchor.
  5. Indent guide: .ft-kids::before at left: 2px aligns with the parent icon column; nested depth (8 + depth*16) tested at 3 levels.

Non-blocking suggestions (future slices)

  1. selectFile scans the whole tree on every click: querySelectorAll(".ft-file") is O(total rows) per file click. With the daemon's 5000-entries-per-dir cap, a wide-open tree could make selection noticeably slow on large repos. Keeping a selectedRowEl reference (and just toggling .active on the old/new row) would make it O(1).
  2. Keyboard/a11y: rows are div click-handlers without role="tree"/aria-expanded on dir rows, and no arrow-key navigation — VS Code's explorer is fully keyboard-navigable. A follow-up slice could add role="tree"/aria-expanded (cheap) and later arrow-key focus.

Nothing blocking — the PR is clean and mergeable from my side.

@argszero

Copy link
Copy Markdown
Owner Author

Fix pushed (26dd780) — addresses the ❌ review:

  • Root cause: .ft-row was display:flex; flex-wrap:wrap; height:24px with .ft-kids { flex-basis:100% }. The kids flex line overflows the fixed 24px box, but the next sibling row still starts at the parent's bottom edge → it painted over the expanded directory's first child (with .active/:hover background it fully occluded it).
  • Fix: .ft-rowdisplay:block; icon+name wrapped in an inner .ft-head (display:flex; height:24px); .ft-kids flows in normal block layout below the head. Row height auto-adapts, so siblings always start after the subtree. padding-left depth indent, hover/active highlight, and .ft-kids::before guide line all preserved (highlight now targets .ft-row:hover > .ft-head).
  • Verification: headless-Chrome pixel check on the new structure — children stack at y=38-98, sibling active row starts at y=104 (previously overlapped the first child at y=24-48). GUI 225 tests pass (added structure-contract assertions to the depth-indent test: .ft-head first child, .ft-kids second). No test-count churn.

Please re-review.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

The ❌ layout-overlap issue is properly resolved by commit 26dd780 (block-level .ft-row + inner .ft-head wrapper — the reviewer's preferred option): kids now flow in normal block layout below the head, so sibling rows no longer overlap expanded children. Verified:

  • CI: test run 31586284239 success on head 26dd780 (actionlint gate + doc-count guard included)
  • GUI: npm test 225/225 (incl. the new structural contract test: .ft-head wraps icon+name, .ft-kids follows directly)
  • pytest: 730 passed, node --check on file-tree.js OK

Diff review: hover/active styling correctly scoped to .ft-head (row auto-height no longer paints over subtree); root row builder updated symmetrically; depth indent/ellipsis/indent guides/expanded-state persistence unchanged. No regressions found.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

Re-verified the fix on head 26dd780 (block-level .ft-row + .ft-head wrapper):

  • Structure: icon+name now wrapped in .ft-head (flex, 24px), .ft-kids flows in normal block layout below — sibling rows start after the subtree (no overlap).
  • Pixel-verified last cycle with headless Chrome: children stack at y=38-98, active sibling at y=104 (was y=24-48 covering a.py).
  • GUI 225 tests pass incl. new structure-contract assertions; pytest 730 pass; CI 31586284239 green on the fix head.

The ❌ layout-overlap issue is properly resolved; no issues found.

@pm25coder

Copy link
Copy Markdown
Contributor

Re-tested the fix commit (26dd780) end-to-end — the layout fix is verified and resolves the overlap correctly.

Verification (all green on 26dd780)

  • GUI: npm test225/225 pass — the 4 P3.5 tests still pass, and the depth-indent test now includes the structure-contract assertions (.ft-head first child, .ft-kids second).
  • pytest: 673 passed / 57 skipped (730) — unchanged, no test-count churn as stated.
  • node --check on file-tree.js: OK.

Review of the fix itself

  1. Root cause is correct: the old .ft-row { display:flex; flex-wrap:wrap; height:24px } + .ft-kids { flex-basis:100% } meant the kids flex line overflowed the fixed 24px box while the next sibling row still started at the parent's bottom edge — painting over the expanded directory's first child. The headless-Chrome pixel numbers (child at y=38-98 vs sibling at y=104, previously overlapping at y=24-48) are a solid empirical anchor.
  2. The block-level restructure is the right fix: .ft-row { display:block } with an inner .ft-head { display:flex; height:24px } wrapper + .ft-kids in normal flow gives auto-height rows, so siblings always start after the subtree. Hover/active retargeted to .ft-row:hover > .ft-head / .ft-row.active > .ft-head preserves the highlight semantics, and the box-shadow: inset 2px 0 0 var(--accent) selection bar survives the move.
  3. No visual regressions in the moved pieces: padding-left depth indent stays on .ft-row (block) — since .ft-kids is inside the row's padding box, children keep the same left offset as before (guide line .ft-kids::before at left:2px still aligns with the parent icon column, same as the flex version).

Note on my earlier review

My previous review missed this overlap (the DOM/cache logic was correct, but I didn't catch the fixed-height flex geometry). The fix is a genuine improvement — thanks for the empirical check. My earlier non-blocking notes still stand as future slices only: ① selectFile O(n) full-tree scan per click → selectedRowEl reference for O(1); ② keyboard/a11y (role="tree"/aria-expanded, arrow-key nav).

Nothing blocking — re-verified and mergeable from my side.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle

3rd consecutive ✅ from a different cycle on head 26dd780 (fix unchanged since R1132; CI 31586284239 green; GUI 225 + pytest 730 pass locally, pixel-verified layout). Merge condition met (3 consecutive ✅, no ❌ between).

@argszero
argszero merged commit 01b7cfa into master Aug 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants