Skip to content

refactor(ui): migrate remaining upstream call sites to the shared icon module (tranche 4) - #307

Open
truongthinh1412 wants to merge 5 commits into
johannesjo:mainfrom
truongthinh1412:refactor/208-tranche-4-upstream-call-sites
Open

truongthinh1412 wants to merge 5 commits into
johannesjo:mainfrom
truongthinh1412:refactor/208-tranche-4-upstream-call-sites

Conversation

@truongthinh1412

@truongthinh1412 truongthinh1412 commented Oct 10, 2026 •

Copy link
Copy Markdown

Description

Tranche 4 (final) of the shared icon-module migration (#208), stacked on tranches 1–3 (#276, #305, #306). Rebased onto current main (717229e1), now covering every remaining inline icon SVG in non-test .tsx sources — including the call sites listed in the review: PromptHistory, TaskShellSection, chat/Composer, understanding/TourCard, TaskProjectFilter (dropdown caret), graph/GraphCanvas (lock + related glyphs), graph/KindMark, understanding/TourOutline, and documents/FilesPopover.

Remaining <svg>s after this series, each deliberate:

  • src/components/icons.tsx — the module itself.
  • src/lib/mermaid-lightbox.ts — string-built HTML in .ts (left as-is per review).
  • Three non-icon SVGs: the GraphCanvas mind-map surface (interactive pan/zoom diagram), the chat ContextMeter ring (data-driven progress meter), and MergeCommitList's accent dot (decorative bullet colored by inline theme.accent, not a currentColor glyph).
  • Test fixtures.

Issues Resolved

Fixes #208 (together with the earlier tranches; scope as described above)

Check List

  • New functionality includes testing.
  • New functionality has been documented in the README if applicable. (Not applicable — refactor only.)

@johannesjo

Copy link
Copy Markdown
Owner

🔧 Needs changes – labelled needs work; ping me once the items below are addressed and I'll re-review.

Thanks for the PR, and for splitting the series as asked! I checked this tranche's own commit (045417e9, the six files on top of #306) mechanically. Every replacement matches what it replaced: HistoryAltIcon, TerminalSmallIcon and ArrowUpAltIcon keep their stroke width, round caps/joins and paths; StopSmallIcon keeps the filled rect; WorkspaceControls now uses the existing GearIcon and ChevronLeftAltIcon with identical paths, and the reactive rotate style still reaches the <svg>. FileIcon keeps the 9x11 box via its extra height prop, and the client test picks up all five new exports automatically. Two things stand in the way.

Should fix before merge

  • Rebase needed – GitHub reports the branch as conflicting with main (84 commits behind). In this tranche's own files, main has since changed PromptHistory.tsx, TaskShellSection.tsx, chat/Composer.tsx, understanding/TourCard.tsx and icons.tsx (which gained SaveIcon, TargetIcon and CircleCheckIcon). Lower in the stack, main has deleted AddProjectMenu.tsx. None of those files picked up new inline SVGs, so the rebase should be textual.
  • PR description (Fixes #208, "no inline icon <svg> remains in non-test sources") – at this PR's base, inline icon SVGs remain in files the four tranches never touch: src/components/TaskProjectFilter.tsx:64 (dropdown caret), src/graph/GraphCanvas.tsx:831 (lock glyph), src/graph/KindMark.tsx:11 (document glyph) and src/lib/mermaid-lightbox.ts:11 / :14 (close and enlarge glyphs as HTML strings; the close one is the same M3.72 3.72… path that Add a shared icon module (75 inline SVGs across 31 files, close icon pasted 10x) #208 counts as pasted 10x). Current main also adds src/components/understanding/TourOutline.tsx:6 (chevron) and src/documents/FilesPopover.tsx:111. As written, merging would auto-close Add a shared icon module (75 inline SVGs across 31 files, close icon pasted 10x) #208 while those remain. Either migrate those sites (the string-built lightbox glyphs may need to stay as they are), or change Fixes #208 to Refs #208 and narrow the claim to the call sites this series actually covers.

Automated review pass (Claude Code). Anything unclear or wrong – say so and I'll take a look.

@johannesjo johannesjo added the needs work Review requested changes; waiting on the author label Oct 15, 2026
@truongthinh1412

Copy link
Copy Markdown
Author

@johannesjo — all three reviews are addressed; the whole series is rebuilt on current main (717229e1) and all four PRs are mergeable again.

#305 items: rebased; the two newer DocumentIcon importers (CanvasTabStrip, TerminalSelectionMenu) now import from ./icons with their explicit sizes kept; ExpandAltIcon/CollapseAltIcon JSDoc corrected (shape + actual use); and BLOCK_ACTIONS now carries an icon component field that ActionIcon renders directly — the RunComposer.draft test asserts component identity and path equality against the module, so there's no second copy of the paths to maintain.

#306 items: rebased (the TaskBranchInfoBar import line now unions your ToolsIcon); and the remote phone UI is now actually covered — remote/TaskActionsDialog's pass/blocked ✓/× symbols are migrated to CheckIcon/CloseIcon like MergeReadinessPanel, so the description stands as written.

#307 items: rebased; and rather than weakening the claim, this tranche now migrates every remaining inline icon SVG in non-test .tsx sources (the review's list plus FilesPopover). What deliberately remains is listed in the updated PR description: the module itself, the string-built mermaid-lightbox.ts glyphs (left as-is per your note), three genuinely non-icon SVGs (mind-map canvas surface, data-driven context-meter ring, the accent-colored decorative dot), and test fixtures.

One heads-up: a plain rebase had silently reverted most of the migrated files (the main rewrites swallowed the patches — e.g. BlockActions came back with the inline-SVG ActionIcon), so the branches were rebuilt file-by-file on the new base and re-audited (path/size/conditional-pair fidelity, knip, full client suite) rather than trusting the rebase. Thanks again for the detailed reviews — they've made this series much better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs work Review requested changes; waiting on the author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a shared icon module (75 inline SVGs across 31 files, close icon pasted 10x)

2 participants