Review 4228
#4228 feat(TreeList): customizable expand/collapse indicator icon by AKnassa (bucket: external contributor)
7eb427eaba46200cfcff95cb3a155adce207e3c2
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full
WHY: R1e re-review after the prior CHANGES_REQUESTED; manual visual evidence was required for the indicator-column fix.
WHY 1: TreeList's built-in chevron cannot express a file tree's folder-open/folder-closed/file icon language.
WHY 2: Builders have to put file/folder icons in startContent, where they no longer replace the expand affordance and leaf rows stop lining up with expandable rows.
WHY 3: A tree view exists to make hierarchy scan quickly; a mismatched affordance makes the hierarchy harder to read.
USER-FACING PROBLEM: a person scanning a file tree cannot use the expected folder/file glyphs in the expand column, so parent and leaf rows read less like one hierarchy. PROBLEM SEVERITY: missing capability — #4131 asks for this capability and names the file-tree case.
VERDICT: clear
The tree now asks the caller for an indicator for each row. Parent rows keep the existing toggle button and accessibility wiring; leaf rows can fill the same indicator column without becoming toggles. The reply commit puts that column and the recommended Icon size="sm" on the same 1rem scale, so the custom glyphs track user font size instead of overflowing a fixed px column.
SOLUTION (1 reply decision · ~100 runtime lines)
- Size the shared indicator column and default/custom glyph contract at
1rem, and document/usesize="sm".
BURDEN: low — one internal style constant plus docs/story/tests; no new effect, listener, observer, or state. BURDEN MATCH: proportionate — the previous blocker was exactly a sizing contract mismatch.
VERDICT: clear
OWNER: TreeListItem owns the indicator slot and toggle; caller owns the glyph returned by renderExpandIcon.
TIER 1: existing Icon sizing contract reused.
TIER 2: semantic icons/indicator column only; no new owner.
SEAMS: default chevron, custom parent icon, custom leaf icon, null leaf, keyboard toggle, LTR/RTL.
BEHAVIOR UNIT: inline — no new state machine; the change is size/layout only.
| seam | driven result |
|---|---|
| default chevron | still renders and toggles; basic story matches main at 16px and scales to 20px at large root font |
| custom parent icon | expanded and collapsed folder icons are inside the indicator box at 16px and 20px |
| custom leaf icon | file icons use the same 16px/20px indicator box and keep the 4px label gap |
| keyboard toggle/focus | focusing public and pressing ArrowRight expands it, keeps focus on public, and swaps to the expanded icon |
| RTL | indicator column mirrors to the inline-start side; gap remains 4px |
VERDICT: clear
The prior blocker is gone: at the current head, size="sm" custom icons and the default chevron fill the same indicator column at both default and large root font sizes. Existing default TreeList output is unchanged at 16px; at a larger root font, the default chevron now scales with the icon/text system instead of staying 16px.
VERDICT: clear
renderExpandIcon?: (state: TreeListExpandIconState) => ReactNode is unchanged from the prior reviewed API. #4131 was filed by the maintainer and proposed this render-prop shape; the re-review did not reopen that scope.
VERDICT: clear
No new theme target in the reply commit. The changed runtime surface is the indicator sizing rule in TreeListItem.tsx: INDICATOR_SIZE = '1rem' applied to width, height, and font-size on the shared indicator column. No existing theme target is removed.
VERDICT: clear
BEHAVIOR: no behavior break found; the focused Vitest suite passes and Chromium keyboard toggle keeps the same focus/expansion path. API: no reply-commit API change. VISUAL: intended large-root change only — the default chevron grows from 16px on main to 20px at a 20px root, matching the icon scale; label gap remains 4px and no glyph pixels escape the slot. THEME: no existing target/token removed.
VERDICT: clear
EFFECTS: zero added or changed.
RENDER: no new state or render path.
LISTENERS/OBSERVERS: none.
LAYOUT: static 1rem sizing; no DOM reads or forced layout.
BUNDLE: no dependency change.
VERDICT: clear
VISUAL CHECK: manual frames required WHY: the prior blocker was visual geometry at the indicator slot, including large root font size.
| case | receipt | result |
|---|---|---|
| main basic LTR 16px / 20px | build 9aaa6f40016a, core-treelist--basic, neutral light, target count 1 |
baseline chevron stays 16px at 20px root |
| head basic LTR/RTL 16px | build 7eb427eaba46, core-treelist--basic, neutral light, target count 1 |
default chevron 16×16 inside 16×16 column, 4px label gap |
| head basic LTR/RTL 20px | same | default chevron 20×20 inside 20×20 column, 4px label gap |
| head custom LTR/RTL 16px | build 7eb427eaba46, core-treelist--custom-expand-icons, neutral light, target count 1 |
custom parent and leaf icons 16×16 inside 16×16 column, 4px label gap |
| head custom LTR/RTL 20px | same | custom parent and leaf icons 20×20 inside 20×20 column, 4px label gap |
| keyboard toggle/focus | same story, LTR 16px | ArrowRight on focused public expands it; focus stays on public; icon remains 16×16 in the slot |
Pixel scan: every head case had zero dark icon pixels in the 2px ring outside the indicator box; no overflow reproduced. Sensor receipts matched build, story, theme, color mode, direction, viewport, media, target count, semantic state, and no page/Storybook errors.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
The reply commit does not change labels, translations, roles, or keyboard routing. Focused Vitest covers custom-icon toggle wiring, Enter/ArrowLeft paths, and disabled parent state; Chromium verifies keyboard focus/ArrowRight expansion with custom icons.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: met — the prior overflow finding does not reproduce at 16px or 20px roots in Chromium.
DISPOSITION: prior blocker fixed; prior inline about the code example fixed by size="sm"; no new blocking finding in the re-review scope.
ADVICE: omitted — no author action needed from code review.
AUTHOR CAN PROCEED: yes — code-review draft is approval; remaining blockers are repository gates.
WORST OUTCOME: none found → approve.
JUDGEMENT NEEDED: none for this re-review scope; the repo's design/visual-acceptance gates still stand independently.
approve
Thanks, this addresses my earlier blocker: the 1rem column and size="sm" docs keep default, parent, and leaf icons aligned at 16px/20px.
[Reviewed by Robohands]
None.
- Current main advanced past the PR, but no commits since the PR's merge-base touch the PR's files and
git merge-treeis clean, so I did not add another merge. - CI's substantive GitHub jobs are green; Vercel failed,
visual-acceptanceis pending, andreview-requiredis pending. - The PR remains labeled
needs:design-review; this code re-review does not clear that label.
TIME total 18m rules 3m required review kit and rubric setup 5m untrusted guard, worktree, fast install, warm main, one build for Storybook config reading 3m prior review, reply diff, current source checks 2m focused TreeList Vitest: 108 passed measuring 4m 11 Chromium frames with sensor receipts writing 1m draft, critic check, wiki record waste 1m first keyboard probe stopped on the wrong row; corrected and reran
- I did not inspect Vercel logs; the failing Vercel deployment is outside the focused code re-review.
Draft only; nothing was posted to GitHub in this run.