[truongthinh1412]

Description

Tranche 3 of the shared icon-module migration (#208), stacked on tranches 1 (#276) and 2. This tranche covers task components and the remote agent view: 14 replacements — inline <svg> blocks plus the remaining text-glyph icon substitutes (✓/× status symbols in MergeReadinessPanel, standalone × close glyphs in AskCodeCard and TaskBranchAdoptionBanner). The ❯ characters in PromptInput are PTY-parsing logic, not icons, and stay as-is. Dynamic sizing (size={iconSize()}) and reactive styles are preserved.

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! I reviewed this tranche's own commit (859ef52a, the 13 files on top of #305). Every replaced inline SVG draws the same path, viewBox, size and stroke attributes through its new component, the CommitNavBar chevrons keep their dynamic iconSize(), and the new icons are picked up by the Object.values(icons) render test. The code itself looks good; the item below is about merge state.

Should fix before merge

• Merge conflicts with main – GitHub reports this branch as conflicting (main has moved 84 commits since 99dd099). In this tranche's own changes, src/components/TaskBranchInfoBar.tsx:20 edits the same ./icons import line that main changed to add ToolsIcon for the new Fix CI button. The other overlapping files (Sidebar.tsx, TaskTitleBar.tsx, DocumentWorkspacePanel.tsx, icons.tsx and so on) come from the tranches below, so rebasing the whole stack onto current main should clear it.

Nits

• src/remote/TaskActionsDialog.tsx:30 – the description says this tranche covers "task components and the remote agent view", but nothing under src/remote/ is touched. The phone UI's merge-readiness list still uses the text ✓ / × symbols that MergeReadinessPanel.tsx replaces here. Either migrate its pass/blocked symbols the same way (src/remote already imports from ../components), or drop "and the remote agent view" from the description.

---

_{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 the update — migrating TaskActionsDialog closes the earlier nit, and the remaining swaps match the originals' paths, sizes and strokes.

Before merge

• src/remote/TaskActionsDialog.tsx:32: SYMBOL stores JSX elements at module level. Solid builds each as a single DOM node, so inside <For> the same <CheckIcon> / <CloseIcon> node moves between rows and only the last pass row and last blocked row keep an icon (a three-row render gives svg counts [0, 0, 1]). Please make it a function, like statusSymbol in MergeReadinessPanel, or map statuses to icon components, and add a small client test with several rows of the same status.
• Conflicts with main again: this tranche conflicts in EditProjectDialog.tsx, TaskBranchInfoBar.tsx, TaskNotesBody.tsx and icons.tsx; the rest come from refactor(ui): migrate app chrome icons to the shared icon module (tranche 1) #276 and refactor(ui): migrate documents and arena icons to the shared module (tranche 2) #305. Since the last plain rebase silently reverted some migrations, please re-check the files afterwards.

Nit

• The fixed 12px icons replacing the ✓ and × glyphs no longer follow sf() font scaling and sit on the baseline — worth a quick visual check next to the remaining text ! and … symbols.

Landing order: #276 → #305 → #306 → #307.

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