Skip to content

refactor(web): introduce tool renderer registry #758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
heat0206 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(web): introduce tool renderer registry #758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions packages/web/src/components/tool-call-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ interface ToolCallItemProps {
showTime?: boolean;
}

//Registry
const TOOL_RENDERERS: Record<string, React.ComponentType<ToolCallItemProps>> = {

@ColeMurray ColeMurray Jun 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated Review] This Record<string, React.ComponentType<ToolCallItemProps>> typing is the crux of the scalability question. Because the value type is the whole item component, every renderer has to re-implement the collapsible shell (the py-0.5 wrapper, toggle button, chevron rotation, time span, and expanded container) — exactly what SlackNotifyEvent does today, duplicating ~25 lines that are character-identical to ToolCallItem's. With N renderers you get N copies of that shell.

Consider narrowing the contract so a renderer contributes only what varies, and let ToolCallItem own the shell once:

interface ToolRenderer {
  icon?: (e: ToolCallEvent) => ReactNode;     // defaults to formatToolCall
  summary?: (e: ToolCallEvent) => ReactNode;  // defaults to formatToolCall
  Body: (props: { event: ToolCallEvent }) => ReactNode;
}

This also mirrors the existing eventRenderers registry in session-timeline.tsx (render-fn + shared prop bag + Partial<Record<...>>), so the two dispatchers stay consistent. See the main review comment for why apply_patch specifically needs this.

Minor: //Registry -> // Registry (space after //), or drop the comment.

"slack-notify": SlackNotifyEvent,
};

function ToolIcon({ name }: { name: string | null }) {
if (!name) return null;

Expand Down Expand Up @@ -51,14 +56,11 @@ function ToolIcon({ name }: { name: string | null }) {
}

export function ToolCallItem({ event, isExpanded, onToggle, showTime = true }: ToolCallItemProps) {
if (event.tool === "slack-notify") {
const Renderer = TOOL_RENDERERS[event.tool];

@ColeMurray ColeMurray Jun 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Automated Review] Two small things on the lookup key:

  1. Case-sensitivity divergence. This uses the raw event.tool, but the canonical dispatch in tool-formatters.ts normalizes with tool?.toLowerCase(). So a renderer registered here only matches an exact-case tool name, while formatToolCall matches case-insensitively. Not a regression (the old === "slack-notify" was also exact), but it bakes an inconsistency into the new seam — whoever adds get-task-status has to remember the registry is case-sensitive. Suggest keying on event.tool?.toLowerCase() to match the rest of the file.
  2. This const Renderer = registry[key]; if (Renderer) { ... } shape is the same dispatch EventItem already does with eventRenderers in session-timeline.tsx. Aligning the two (same contract shape + fallback) keeps the rendering layer coherent.


if (Renderer) {
return (
<SlackNotifyEvent
event={event}
isExpanded={isExpanded}
onToggle={onToggle}
showTime={showTime}
/>
<Renderer event={event} isExpanded={isExpanded} onToggle={onToggle} showTime={showTime} />
);
}

Expand Down
Loading