[truongthinh1412]

Description

Tranche 2 of the shared icon-module migration (#208), stacked on #276 (tranche 1, app chrome). This tranche covers the document workspace and arena screens: 27 inline <svg> icon blocks across 13 files, each replaced faithfully (two Octicon arc-encoding variants kept, same rendered glyph) with shared-module components (paths, stroke widths, caps/joins, viewBox, sizes, styles and accessibility preserved). The standalone src/documents/DocumentIcon.tsx folds into the module (its two import sites that relied on its 18px default now pass size={18} explicitly).

Where main already had a same-named icon drawing a different glyph, main's glyph keeps the name and the differing one is renamed (Alt suffix, e.g. StopAltIcon, SyncAltIcon) — no duplicate exports; knip stays green.

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.)

[johannesjo]

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

Thanks for the PR! This moves the document workspace and arena icons into src/components/icons.tsx, and I checked that the swapped glyphs, sizes, and the focus-button pairing match the inline SVGs they replace. The main thing left is that the branch no longer merges cleanly with main, and the DocumentIcon.tsx removal has a catch that conflict resolution alone won't reveal.

Should fix before merge

• src/documents/DocumentIcon.tsx (deleted) – GitHub reports this branch as conflicting with main. One example: src/components/AddProjectMenu.tsx has been removed on main, but this PR still edits it. Also, since this branch was cut, main has added two more importers of ../documents/DocumentIcon that this PR doesn't touch: src/components/CanvasTabStrip.tsx:12 and src/components/TerminalSelectionMenu.tsx:4. Those files won't show conflict markers during a rebase, but once the file is gone they won't typecheck. Fix: rebase onto main (after refactor(ui): migrate app chrome icons to the shared icon module (tranche 1) #276 lands, since this branch includes its commits) and point both imports at ./icons. They already pass explicit sizes (13 and 14), so the change from an 18px default to 16px doesn't affect them.

Nits

• src/components/icons.tsx (ExpandAltIcon / CollapseAltIcon JSDoc) – The JSDoc says ExpandAltIcon means "enter focus mode" and CollapseAltIcon means "exit focus". In practice it's the other way round: DocumentWorkspacePanel.tsx shows CollapseAltIcon when unfocused (your new inline comment there correctly calls it "enter focus") and ExpandAltIcon only while focused, and TaskTitleBar.tsx always shows CollapseAltIcon. Rendering is unchanged, but a later contributor going by the docs would pick the wrong one. Describing them by shape and current use, or renaming them, would fix it.
• src/documents/BlockActions.tsx:13-44 – Nothing renders from BLOCK_ACTIONS[].path any more. It's now a second copy of each icon's path, used only as the oracle in RunComposer.draft.client.test.tsx, so editing e.g. NoteIcon means updating both places. Optional: replace path with an icon component field that ActionIcon renders, and have the test compare against that.

---

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

[truongthinh1412]

@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!

[johannesjo]

Thanks for addressing all three earlier items — the BLOCK_ACTIONS icon field and corrected focus-icon JSDoc look good, and every swapped glyph matches the inline SVG it replaces (paths, strokes, caps, sizes).

Before merge

• main has moved again: the branch conflicts in CanvasTabStrip.tsx, DocumentWorkspacePanel.tsx and icons.tsx (plus the tranche-1 files). On DocumentWorkspacePanel, main now wraps the focus button's onClick in withViewTransition — please keep that when swapping the glyph.
• main added src/components/project-menu.tsx, which imports DocumentIcon from ../documents/DocumentIcon (line 11). This PR deletes that file, so after the rebase it won't typecheck even without a conflict marker. Please point it at ./icons (it already passes size={14}).
• Tranche-1 interaction: main renamed KanbanIcon to PlanIcon and added AppLogo / IdeaIcon / DisclosureChevronIcon, which overlap refactor(ui): migrate app chrome icons to the shared icon module (tranche 1) #276's LogoIcon / LightbulbIcon / ChevronDownAltIcon.

Nits

• ShieldAltIcon draws a box, not a shield — BoxIcon with a matching JSDoc would be clearer.
• The RunComposer close button now uses CloseIcon's path rather than the original; same look, but not byte-identical as the description says.

Landing order: #276 → #305 → #306 → #307, so this goes in right after #276.

[truongthinh1412]

@johannesjo — all four Oct-25 reviews are addressed; the whole stack is rebuilt on current main (3270d01) and rebased again onto the commits that landed an hour later. Per PR:

#276: LogoIcon dropped (main's AppLogo kept, title-bar hunk untouched); the all-exports client test now explicitly exempts EvidenceNoteIcon/AppLogo (render-smoke only, with a comment why); the Thinking button uses main's IdeaIcon and our LightbulbIcon is gone. On the stroke-pattern item: all our stroke additions already use the private StrokeSvgIcon consistently — main's own exports were left untouched.

#305: withViewTransition kept on the focus button; project-menu.tsx (and the newer CanvasTabStrip/TerminalSelectionMenu/file-link-menu/MarkdownViewerDialog importers that appeared with the latest commits) all point at ./icons now; the KanbanIcon→PlanIcon rename turned out moot (upstream removed that footer). ShieldAltIcon renamed BoxIcon with an accurate JSDoc; I'll fix the RunComposer "byte-identical" wording in the description — the close glyph is the same octicon under a different arc encoding.

#306: the SYMBOL bug is fixed as you suggested — it's now Record<status, () => JSX.Element> invoked per row, with a new client test rendering several same-status rows through the real component and asserting every row gets its own icon (documents the <For> node-moving class). The sf()-scaling nit is noted; symbols remain fixed 12px like MergeReadinessPanel's.

#307: dropped our CaretDownIcon/LockIcon/ChevronDownThinIcon/ChevronRightTinyIcon/DocumentLinesIcon in favor of main's (#314's) icons; per-file diffs vs main verified after the rebase (it had silently reverted files last time). This tranche additionally migrates the newer call sites (BoardSidePanel, StatusDot, task-later-menu); one deliberate leave: ShellLayoutMenu's LayoutIcon is a parameterized multi-path composite keyed by app view-mode state — moving it would put app types into the generic module, so I've left it for a follow-up and reworded the description accordingly. FileIcon got its JSDoc.

Every tip passes typecheck (electron+frontend), eslint 0 warnings, prettier, knip, depcruise and the full client suite (187 files / 2318 tests) locally. Note: main's new npm run check wrapper (scripts/check.mjs) fails on Windows with spawn npm ENOENT even on pristine main — worth a look upstream, unrelated to this series. Ready for re-review!