From 46d01fca652b02f103d926906bc1265fdbf9a8ea Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Thu, 30 Jul 2026 22:58:57 +0100 Subject: [PATCH] refactor(site/src/pages/AgentsPage/components/ChatElements/tools): replace label/icon switches with tables and registry invariants (#27706) Stacked on #27697. Do not merge before it; this diff is against that branch, not main. Replaces the `ToolLabel` and `ToolIcon` string switches with lookup tables, and makes the registry/table agreement a CI failure instead of a manual audit. No behaviour change; every label and icon renders identically. ## Why Three enumerations of the same tool-name set (`toolRenderers`, the label switch, the icon switch) were kept in agreement by convention alone, and drifted repeatedly (#27684, #27687, #27697 each deleted arms a registered renderer had silently shadowed). Switches are unenumerable, so the drift was invisible to both tsc and tests. ## What changed - `genericToolLabels` (ToolLabel.tsx): the four generic-rendered labels (`process_signal`, `process_list`, `attach_file`, `advisor`) as a `Partial>`. `ToolLabel` is now a table lookup plus the MCP/raw-name fallback. - `toolIcons` (ToolIcon.tsx): all 19 built-in icon names as a `Partial>`, same pattern. Unknown/MCP names still fall to `WrenchIcon`. - `Tool.tsx`: exports `toolRenderers` (it is the single source of truth for dispatch; the tests read it directly). - `toolLabelVisibility.test.ts`: fails if a registered renderer that does not delegate to `GenericToolRenderer` shadows a `genericToolLabels` entry, naming the arm. `process_signal` (known delegator) and `advisor` (consumed directly by `AdvisorTool`) are allowlisted in the test. This is the tripwire requested in review on #27697, as a CI failure rather than a comment. - `toolIconsCoverage.test.ts`: fails if a registered renderer has no dedicated icon, with `read_skill_file` allowlisted for its `read_skill` icon alias. ## Design notes - The invariant checks live in tests, not in the type system. TypeScript cannot assert a runtime object's key set against an independent intent without either re-listing the names (the `satisfies Record` approach, rejected as ugly repetition) or abusing conditional types. The tables are plain objects; the tests own the invariant. A generated union from the Go `chattool` constants is the proper long-term fix and is deliberately out of scope. - No `as const` / union key types: they added annotation without buying safety the tests don't already provide, and nothing consumes `keyof typeof` here. ## Validation - `tsc --noEmit`, `biome check --error-on-warnings`, knip: clean. - Unit (`--project=unit src/pages/AgentsPage`): 1502 passed, 2 skipped (base: 1500/2; +2 are the new invariant tests). - Storybook (`--project=storybook src/pages/AgentsPage`): 949 passed, 2 failed, identical to base: `AgentChatPageView.stories.tsx > Scroll To Bottom Button Works With Inverse Scroll` and `Tool.stories.tsx > MCP Tool Completed`. Both reproduce on unmodified main, so pre-existing and unrelated. One additional flake (`AgentChatPage.stories.tsx > Slash Compact Yields To Personal Skill`) failed once under parallel load and passed in isolation on the final code. - Line delta vs #27697: +148 / -98 across 5 files. Generated by Coder Agents. --- .../components/ChatElements/tools/Tool.tsx | 2 +- .../ChatElements/tools/ToolIcon.tsx | 60 ++++---- .../ChatElements/tools/ToolLabel.tsx | 137 ++++++++++-------- .../tools/toolIconsCoverage.test.ts | 18 +++ .../tools/toolLabelVisibility.test.ts | 26 ++++ 5 files changed, 144 insertions(+), 99 deletions(-) create mode 100644 site/src/pages/AgentsPage/components/ChatElements/tools/toolIconsCoverage.test.ts create mode 100644 site/src/pages/AgentsPage/components/ChatElements/tools/toolLabelVisibility.test.ts diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx index 5934b2524f..4e8d6aa237 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx @@ -1016,7 +1016,7 @@ const StartWorkspaceRenderer: FC = ({ // Renderer lookup map for tool names and specialized renderers. // --------------------------------------------------------------------------- -const toolRenderers: Record> = { +export const toolRenderers: Record> = { execute: ExecuteRenderer, process_output: ProcessOutputRenderer, process_signal: ProcessSignalRenderer, diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx index 913b8d0bc8..b74f26d148 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ToolIcon.tsx @@ -5,6 +5,7 @@ import { FilePenLineIcon, FileTextIcon, LightbulbIcon, + type LucideIcon, MonitorIcon, PowerIcon, RouteIcon, @@ -22,6 +23,28 @@ import { } from "#/components/Tooltip/Tooltip"; import { cn } from "#/utils/cn"; +export const toolIcons: Partial> = { + execute: TerminalIcon, + process_output: TerminalIcon, + process_list: TerminalIcon, + process_signal: TerminalIcon, + read_file: FileTextIcon, + read_skill: FileTextIcon, + write_file: FilePenLineIcon, + edit_files: FilePenLineIcon, + list_templates: ServerIcon, + read_template: ServerIcon, + create_workspace: ServerIcon, + start_workspace: PowerIcon, + chat_summarized: BotIcon, + list_agents: BotIcon, + thinking: LightbulbIcon, + propose_plan: RouteIcon, + ask_user_question: BadgeQuestionMarkIcon, + advisor: CompassIcon, + computer: MonitorIcon, +}; + export const ToolIcon: React.FC<{ name: string; iconUrl?: string; @@ -73,39 +96,6 @@ export const ToolIcon: React.FC<{ return img; } - switch (name) { - case "execute": - case "process_output": - case "process_list": - case "process_signal": - return ; - case "read_file": - case "read_skill": - return ; - case "write_file": - case "edit_files": - return ; - case "list_templates": - case "read_template": - case "create_workspace": - return ; - case "start_workspace": - return ; - case "chat_summarized": - case "list_agents": - return ; - case "thinking": - return ; - case "propose_plan": - return ; - case "ask_user_question": - return ; - case "advisor": - return ; - case "computer": - return ; - - default: - return ; - } + const Icon = toolIcons[name] ?? WrenchIcon; + return ; }; diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx b/site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx index 4b465e7b03..bbe9b59daf 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/ToolLabel.tsx @@ -2,75 +2,86 @@ import type React from "react"; import { getPathBasename } from "../../../utils/path"; import { asRecord, asString, humanizeMCPToolName, parseArgs } from "./utils"; -export const ToolLabel: React.FC<{ +type ToolLabelProps = { name: string; args: unknown; result: unknown; mcpSlug?: string; -}> = ({ name, args, result, mcpSlug }) => { +}; + +const ProcessSignalLabel: React.FC = ({ args, result }) => { const parsed = parseArgs(args); const parsedResult = asRecord(result); + const signal = parsed ? asString(parsed.signal) : ""; + const processId = parsed ? asString(parsed.process_id) : ""; + const shortId = processId ? processId.slice(0, 8) : ""; + const suffix = shortId ? ` ${shortId}` : ""; + const isKill = signal === "kill"; + const isTerminate = signal === "terminate"; - switch (name) { - case "process_signal": { - const signal = parsed ? asString(parsed.signal) : ""; - const processId = parsed ? asString(parsed.process_id) : ""; - const shortId = processId ? processId.slice(0, 8) : ""; - const hasResult = result !== undefined && result !== null; - const success = parsedResult ? Boolean(parsedResult.success) : false; - if (hasResult && success) { - const verb = signal === "kill" ? "Killed" : "Terminated"; - return ( - - {verb} process{shortId ? ` ${shortId}` : ""} - - ); - } - if (hasResult && !success) { - const verb = - signal === "kill" - ? "kill" - : signal === "terminate" - ? "terminate" - : "signal"; - return ( - - Failed to {verb} process{shortId ? ` ${shortId}` : ""} - - ); - } - return ( - - {signal === "kill" - ? "Killing process…" - : signal === "terminate" - ? "Terminating process…" - : "Sending signal…"} - - ); - } - case "process_list": - return Listing processes; - case "attach_file": { - const attachedName = - (parsedResult ? asString(parsedResult.name) : "") || - (parsed ? asString(parsed.name) : "") || - (parsed ? getPathBasename(asString(parsed.path)) : "") || - "file"; - return ( - {`Attached ${attachedName}`} - ); - } - case "advisor": - return ( - - Advisor - - ); - - default: { - const displayName = mcpSlug ? humanizeMCPToolName(mcpSlug, name) : name; - return {displayName}; - } + const hasResult = result !== undefined && result !== null; + if (!hasResult) { + const inFlightVerb = isKill + ? "Killing process…" + : isTerminate + ? "Terminating process…" + : "Sending signal…"; + return {inFlightVerb}; } + + const success = parsedResult ? Boolean(parsedResult.success) : false; + if (success) { + const verb = isKill ? "Killed" : "Terminated"; + return ( + + {verb} process{suffix} + + ); + } + + const failedVerb = isKill ? "kill" : isTerminate ? "terminate" : "signal"; + return ( + + Failed to {failedVerb} process{suffix} + + ); +}; + +const AttachFileLabel: React.FC = ({ args, result }) => { + const parsed = parseArgs(args); + const parsedResult = asRecord(result); + const resultName = parsedResult ? asString(parsedResult.name) : ""; + const argName = parsed ? asString(parsed.name) : ""; + const argPath = parsed ? asString(parsed.path) : ""; + const attachedName = + resultName || argName || getPathBasename(argPath) || "file"; + return ( + {`Attached ${attachedName}`} + ); +}; + +export const genericToolLabels: Partial< + Record> +> = { + process_signal: ProcessSignalLabel, + process_list: () => ( + Listing processes + ), + attach_file: AttachFileLabel, + advisor: () => ( + + Advisor + + ), +}; + +export const ToolLabel: React.FC = (props) => { + const Label = genericToolLabels[props.name]; + if (Label) { + return