feat(apps): render tool.icons on ToolListItem and ToolDetailPanel (#1264)

[cliffhall]

Summary

• Renders tool.icons on the Tools sidebar (ToolListItem, 20×20) so it matches the Apps sidebar.
• Renders tool.icons on the tool detail header (ToolDetailPanel, 24×24) so it matches the Apps screen header.
• Both use Image with fit: "contain" and alt="" (decorative — visible label sits next to it). No icon row is rendered when tool.icons is absent.
• Adds WithIcon Storybook stories for both groups, plus tests covering icon-present and icon-absent states.

Closes #1264.

Implementation note

Going with Option B from the issue (keep AppListItem separate). AppListItem is already shipped (#1261) with chevron + description, so the cleaner path is to fold icon support into the existing Tool* components without disturbing the Apps row.

Sizes match the existing precedent set by Apps (#1261, #1263): list items use 20×20, detail/screen headers use 24×24.

Test plan

• npm run validate — all 666 tests pass; format, lint, build clean
• ToolListItem.tsx and ToolDetailPanel.tsx per-file coverage: 100% lines/branches/functions
• Storybook visual check: Groups/ToolListItem/WithIcon, Groups/ToolDetailPanel/WithIcon
• Visual check in app: a tool with icons shows the icon in the sidebar list and in the detail panel header

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 3s —— View job

---

Claude PR Review

• Read changed files
• Compare against AppListItem pattern
• Check tests and stories
• Post review

---

Summary

Clean, focused change. The implementation mirrors AppListItem's icon pattern exactly (20×20 Image, fit: "contain", alt="", conditional render on iconSrc), uses withProps() subcomponents per the repo conventions, and has good test coverage for icon-present and icon-absent states. Option B (keep list-item components separate) is reasonable given the semantic divergence (tools don't push deeper and don't show description).

Nits / minor

1. Storybook icon data URL is double-encoded — ToolListItem.stories.tsx:28-32

const ICON_DATA_URL =
  "data:image/svg+xml;utf8," +
  encodeURIComponent(
    '<svg ... fill="%23228be6"><circle .../></svg>',
  );

The source string already has %23 in place of #. Running encodeURIComponent over it re-encodes the % → %25, so the decoded SVG the browser parses ends up with fill="%23228be6", which isn't a valid color — the circle renders black instead of Mantine blue. Either write fill="#228be6" in the source and let encodeURIComponent handle the encoding, or drop encodeURIComponent entirely and pre-encode. Cosmetic only (Storybook), but the story is meant to show an icon visual.

Fix this →

2. getByRole("presentation") is implicit on Mantine's behavior — ToolListItem.test.tsx:57

The test relies on Mantine's Image emitting role="presentation" for alt="". This works today but is brittle if Mantine changes its role handling. A more durable alternative is screen.getByAltText("") or querying by img tag — either survives ARIA-role changes. Not blocking.

Nice-to-haves (not blocking)

• Row, ItemBody, ItemLabel, ItemSubLabel, and the 20×20 fit: "contain" icon are now duplicated verbatim between AppListItem.tsx and ToolListItem.tsx. If a third icon-row variant appears, this is the natural moment to extract a small primitive. For now, two copies is fine and matches the "three similar lines is better than a premature abstraction" guidance.
• align: "flex-start" in Row is correct when two lines of body text are present, but for the common single-line case (no title) the icon sits slightly above the text baseline. AppListItem does the same, so this is consistent — just noting in case you want center-align when body is one line.

Approvals

• ✅ Follows repo conventions: withProps() subcomponents on Mantine primitives, no inline styles, no CSS classes, proper use of miw: 0 to enable truncation inside the flex row.
• ✅ alt="" on a decorative icon alongside a visible label is the correct accessibility call.
• ✅ Tests cover both states; PR description confirms 100% per-file coverage and validate passing.

Nothing blocking here — the data-URL nit is the only tangible issue.
• Branch: feat/1264-tool-list-item-icons

[cliffhall]

Thanks for the review! Responses below:

1. Storybook icon data URL is double-encoded — fixed in 624b1aa. Changed the source to fill="#228be6" and let encodeURIComponent produce the right escape.

2. getByRole("presentation") is implicit on Mantine's behavior — leaving as-is for now. The pattern matches AppListItem.test.tsx:61 exactly; if we want to migrate to getByAltText("") for durability, it should be done across both files in a follow-up.

Nice-to-haves:

• Duplication of Row/ItemBody/icon between AppListItem and ToolListItem — agreed. Two copies is fine per the "three similar lines is better than a premature abstraction" guidance; if a third icon-row variant lands we can extract a shared primitive.
• align: "flex-start" for single-line bodies — keeping it consistent with AppListItem. Worth revisiting both at once if we want centered alignment for one-line rows.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 56s —— View job

---

Claude PR Review (follow-up)

• Read current state of changed files
• Verify fix for data URL double-encoding (624b1aa)
• Re-check against review criteria
• Post review

---

Summary

All prior review feedback is resolved or reasonably dispositioned. Data-URL fix landed correctly; the remaining items (test role selector, cross-file duplication, alignment for single-line rows) are fine as-is with your reasoning. LGTM.

Verification of 624b1aa (data URL fix)

ToolListItem.stories.tsx:28-32 — the source string now uses fill="#228be6":

const ICON_DATA_URL =
  "data:image/svg+xml;utf8," +
  encodeURIComponent(
    '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" fill="#228be6"><circle cx="12" cy="12" r="10"/></svg>',
  );

encodeURIComponent now escapes # → %23 exactly once, so the decoded SVG the browser parses has fill="#228be6" and the circle should render in Mantine blue. Correct fix.

Re-checked against repo conventions

• ✅ .withProps() subcomponents on Mantine primitives (Row, ItemBody, ItemLabel, ItemSubLabel, ToolIcon) — no Box used as a subcomponent constant.
• ✅ No inline styles, no CSS classes, no raw hex values in component props.
• ✅ miw: 0 on ItemBody to enable truncation inside the flex row.
• ✅ alt="" on decorative icon with a visible label — correct a11y.
• ✅ Mirrors AppListItem.tsx:1-68 structurally (icon treatment, Row align, body stack), diverging only where semantics differ (no chevron, no description → ItemSubLabel for the name when title is present).
• ✅ Tests cover both icon-present and icon-absent branches; PR description reports 100% per-file coverage and validate green.

Responses to prior nits

• Role selector in tests — agreed, fine to leave. Migrating getByRole("presentation") → getByAltText("") should land as a single follow-up across AppListItem.test.tsx:61 and ToolListItem.test.tsx:57 so they stay in lockstep.
• Row/ItemBody/icon duplication — agreed with your call. Two copies beats a premature abstraction; extract on the third occurrence.
• align: "flex-start" on single-line rows — agreed, consistency with AppListItem wins here. Revisit both together if/when you want centered alignment for one-line rows.

Nothing blocking. Ready to merge once the manual visual checks in the PR description are ticked off.
• Branch: feat/1264-tool-list-item-icons