Skip to content

refactor(ui): migrate documents and arena icons to the shared module (tranche 2) - #305

Open
truongthinh1412 wants to merge 2 commits into
johannesjo:mainfrom
truongthinh1412:refactor/208-tranche-2-documents-arena
Open

truongthinh1412 wants to merge 2 commits into
johannesjo:mainfrom
truongthinh1412:refactor/208-tranche-2-documents-arena

Conversation

@truongthinh1412

Copy link
Copy Markdown

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 byte-faithfully 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

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

@johannesjo johannesjo added the needs work Review requested changes; waiting on the author label Oct 15, 2026
@truongthinh1412
truongthinh1412 force-pushed the refactor/208-tranche-2-documents-arena branch from a9b7d11 to 5550474 Compare October 17, 2026 16:15
@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.

2 participants