refactor(ui): migrate app chrome icons to the shared icon module (tranche 1)

[truongthinh1412]

Description

Tranche 1 of the shared icon-module migration (#208), reworked per review: rebuilt on current main, split into a series of focused PRs (this one + #305, #306, #307), with the review's naming and theming items applied:

• Where main grew same-named icons with different glyphs, main's glyph keeps the name and the differing one is renamed (Alt/Mark suffix: ChevronDownAltIcon, TerminalAltIcon, GitHubMarkIcon, …) — no duplicate exports, knip green.
• WindowTitleBar logo: inline color: '#ffffff' dropped so styles.css themes it per look again; caps/joins kept.
• PullRequestIcon item is moot (that button no longer exists on main); the GitBranchAltIcon path slip is gone (the branch chip uses the byte-identical GitGraphIcon, in refactor(ui): migrate documents and arena icons to the shared module (tranche 2) #305).
• Import-path nits fixed (./icons within src/components, App.tsx import moved after the @fontsource imports).

This tranche: app chrome — 13 files, 31 inline <svg> blocks replaced byte-faithfully; module grows from 48 to 74 exports, all covered by icons.client.test.tsx.

Issues Resolved

Contributes to #208 (resolved by the full tranche series; no other issues)

Check List

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

[truongthinh1412]

Rebased onto current main (78a65db) to resolve the merge conflicts — no review action needed, this just keeps the PR mergeable.

What the rebase involved, in case it's useful context:

• Upstream's document-workspace rewrite (DocumentWorkspaceOverlay → DocumentWorkspacePanel) was merged with the icon migration intact (including the 18px default-size restoration).
• Icons introduced upstream in the meantime were migrated to the shared module as well, so the PR still holds its "no inline icon SVGs" claim: the new focus-mode toggle (added as EnterFocusIcon/ExitFocusIcon), the full-width toggle (FullWidthIcon), the branch chip (GitBranchAltIcon), the "Advanced options" disclosure chevron in NewTaskPanel (ChevronRightThinIcon), and the tour-ready ✓ in ChangeTourButton (CheckIcon). Module now exports 65 icons, all covered by the client test.
• All local gates re-run green after the rebase (npm run check, npm run check:static, full client suite, zero src/ unit failures).

[johannesjo]

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

Thanks for the PR, and for the careful migration! I checked the call sites mechanically. The stroke width, caps, joins and viewBox of each replaced stroke icon match its new component, and apart from one glyph noted below, every replacement draws the path it replaced (the folder and close variants you folded together on purpose excepted). The main issue is that main has moved on: the branch is 212 commits behind it, and a rebase is now more than mechanical.

Since this was opened

• src/components/MergeDialog.tsx was removed on main (replaced by the finish dialog), so that hunk can go.
• 11 files now conflict, among them icons.tsx (10 hunks), Sidebar.tsx, TaskTitleBar.tsx, WindowTitleBar.tsx and DocumentWorkspacePanel.tsx.
• main has added inline icon SVGs that this PR does not cover yet, in PromptHistory.tsx, TaskShellSection.tsx, WorkspaceControls.tsx (its gear matches your GearIcon), chat/Composer.tsx and understanding/TourCard.tsx. Picking those up is optional, but they matter if the PR is to keep claiming no inline icon SVGs remain.

Should fix before merge

• src/components/icons.tsx – main independently added nine icons to the module, and eight of them share a name with an icon this PR adds but draw a different glyph: ChevronLeftIcon, ChevronDownIcon, ChevronRightIcon (stroke chevrons on main, filled Octicons here), CommentIcon, TerminalIcon, ExpandIcon, TrashIcon and ShieldIcon. Keeping both definitions is a duplicate export. Keeping yours silently changes every existing main caller of that name, and keeping main's changes the glyph at your migrated call sites. Please rename the differing ones, or move those call sites onto main's icon where the glyph should match. ExternalLinkIcon is identical on both sides, and your PullRequestIcon is essentially main's GitMergeIcon, so both can reuse what is already there.
• src/components/WindowTitleBar.tsx (logo) – style={{ color: '#ffffff', ... }} faithfully preserves the old stroke="#ffffff". Since then, main has switched that SVG to stroke="currentColor" (4bb51fa) so that styles.css can colour it per look (.window-title-icon uses var(--accent), with overrides for islands-dark and islands-light). If the conflict is resolved with this side, the inline colour beats the stylesheet and the logo stays white under every look. Drop color from the inline style and keep the linecap/linejoin entries.
• src/components/icons.tsx (GitBranchAltIcon) – this changes a glyph. The inline branch icon removed from DocumentWorkspacePanel.tsx is byte-identical to GitGraphIcon and ends .75.75 0 0 0-1.5 0Z. The new GitBranchAltIcon ends .75.75 0 0 0 0-1.5Z instead, so the top-left dot's cut-out no longer closes into a circle. Use GitGraphIcon for the branch chip and drop GitBranchAltIcon; that also keeps the module free of a near-duplicate, which Add a shared icon module (75 inline SVGs across 31 files, close icon pasted 10x) #208 asks for.

Nits

• src/components/NewTaskPanel.tsx:17 – two imports from the same module via different paths ('../components/icons' and './icons'); merge them. PromptInput.tsx, TilingLayout.tsx and ChangeTourButton.tsx also use '../components/icons' from inside src/components, and in App.tsx the new import sits between the @fontsource CSS imports.

---

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

[johannesjo]

Thanks @truongthinh1412! Since main has moved a lot (several overlapping icon names and MergeDialog.tsx is gone), let's take you up on the offer to split this. Could you rebase and send the rest in tranches of roughly 10–15 files, the way #220 went? Where main already has an icon with the same name, please keep main's glyph rather than adding a second one.

[truongthinh1412]

I'm on it, thanks @johannesjo for the review ! First time contributing to any opensource project, glad i choose this one.

[truongthinh1412]

Thanks @johannesjo for the thorough review — and for catching the GitBranchAltIcon path slip, that one was a real bug in my copy. Everything is addressed; the PR is now tranche 1 of 4 (app chrome: 13 files, 31 inline SVGs), rebuilt on current main:

• Collisions: main's 8 (+ a few newer) same-name icons keep main's glyph; ours are renamed with the module's existing Alt suffix (ChevronDownAltIcon, TerminalAltIcon, …) plus GitHubMarkIcon for the GitHub mark. No duplicate exports — knip verifies every export is used.
• WindowTitleBar logo: inline color: '#ffffff' dropped; caps/joins kept, so styles.css colors it per look again.
• PullRequestIcon: moot — that TaskTitleBar button no longer exists on main; nothing to migrate there.
• Import nits fixed (./icons within src/components, App.tsx import moved after the @fontsource imports).
• MergeDialog.tsx hunk is gone with the file.

Tranches 2 (documents + arena), 3 (task components + remote) and 4 (the newer call sites you listed — PromptHistory, TaskShellSection, WorkspaceControls, chat/Composer, understanding/TourCard) follow as stacked PRs, each ≤15 files.

[johannesjo]

🔧 Needs changes – the rework hasn't been pushed yet; still 3 issues at ca0a48b1

@truongthinh1412, thanks for the update. Your latest comment describes a rebuilt "tranche 1 of 4" (13 files on current main), but GitHub still has the old branch. refactor/208-shared-icon-module on your fork is still at ca0a48b1 (committed 2026-09-16, based on 78a65dbc). That is the same commit the previous review covered, so the PR is still the original 40-file diff. It is 287 commits behind main and conflicts in 11+ files, including MergeDialog.tsx, which main has deleted. You probably still need to push your local branch (git push --force-with-lease).

At this head, the earlier findings still hold:

• src/components/WindowTitleBar.tsx:93 – the inline color: '#ffffff' is still there. Once merged with main, it overrides the .window-title-icon var(--accent) colouring, so the logo stays white under every look.
• src/components/icons.tsx:307 / src/documents/DocumentWorkspacePanel.tsx:931 – the branch chip still uses GitBranchAltIcon. Its path ends in 0 0 0 0-1.5Z rather than GitGraphIcon's 0 0 0-1.5 0Z, so the top-left dot doesn't close into a circle.
• src/components/NewTaskPanel.tsx:17-18 – the same module is still imported twice, through '../components/icons' and './icons'.
• The overlapping icon names with main (ChevronLeftIcon, ChevronDownIcon, ChevronRightIcon, CommentIcon, TerminalIcon, ExpandIcon, TrashIcon, ShieldIcon) aren't resolved on this branch either.

Once the tranche-1 branch is pushed, I'll review that diff. If you'd rather open it as a new PR, please link it here and close this one.

---

Generated by Claude Code

[truongthinh1412]

All four tranches are up, rebased onto the latest main (99dd099):

• refactor(ui): migrate app chrome icons to the shared icon module (tranche 1) #276 — tranche 1: app chrome (13 files, 31 SVGs)
• refactor(ui): migrate documents and arena icons to the shared module (tranche 2) #305 — tranche 2: documents + arena (16 files, 27 SVGs, folds DocumentIcon.tsx into the module)
• refactor(ui): migrate task component icons to the shared module (tranche 3) #306 — tranche 3: task components + remote (13 files, incl. the ✓/× text-glyph replacements)
• refactor(ui): migrate remaining upstream call sites to the shared icon module (tranche 4) #307 — tranche 4: the newer call sites from your review (PromptHistory, TaskShellSection, WorkspaceControls, chat/Composer, understanding/TourCard) — with this merged, no inline icon <svg> remains in non-test sources

The latest rebase touched only TaskTitleBar.tsx (the snooze/PR-status rework): your new menu already uses module icons, so the old z-order glyph pair became orphaned and is dropped; the four remaining inline glyphs there (check-circle, minimize bar, canvas panel, focus) are migrated. Every branch passes check, check:static (knip + depcruise), the full client suite, and has zero src/ unit failures locally. Happy to adjust anything!

[truongthinh1412]

@johannesjo — the rework is pushed now, sorry for the timing confusion: my earlier comment went up while the push was still failing on a fork permission (workflow scope needed for the new .github/workflows/android.yml), so GitHub briefly kept showing the old ca0a48b head. That's resolved — current state:

• refactor(ui): migrate app chrome icons to the shared icon module (tranche 1) #276 head is now 6aa3c89: rebuilt on current main, 13 files / 31 inline SVGs, with all review items applied (main's glyphs keep the colliding names; logo color dropped; import nits fixed; GitBranchAltIcon gone in favor of GitGraphIcon in refactor(ui): migrate documents and arena icons to the shared module (tranche 2) #305).
• refactor(ui): migrate documents and arena icons to the shared module (tranche 2) #305 / refactor(ui): migrate task component icons to the shared module (tranche 3) #306 / refactor(ui): migrate remaining upstream call sites to the shared icon module (tranche 4) #307 are the remaining tranches (documents+arena, task components, and the newer call sites from your review), each ≤16 files, stacked and rebased on the same main.

All four are mergeable and pass check + check:static + the client suite locally. Ready for re-review whenever you are — thanks again!