refactor(web): introduce tool renderer registry#758
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTool Renderer Registry
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ColeMurray
left a comment
ColeMurray
left a comment
There was a problem hiding this comment.
[Automated Review]
Thanks @heat0206 — the direction is right and matches what was agreed in #724 (registry first, slack-notify as the first case), and the refactor is behavior-preserving. Before this becomes the foundation that get-task-status and other renderers build on, I want to flag one structural issue with the contract the registry is built on, since it determines whether the pattern actually scales. Line-specific notes are inline; the summary and the apply_patch analysis are here.
The pattern already exists in this file — mirror it
session-timeline.tsx already has this exact registry, one level up, in a cleaner shape:
// session-timeline.tsx — dispatch on event type, shared prop bag, single fallback
const eventRenderers: Partial<Record<SandboxEvent["type"], (props: EventRendererProps) => ReactNode>> = {
user_message: UserMessageEvent,
artifact: ArtifactEvent, // <- this is the screenshot renderer
// ...
};
const render = eventRenderers[event.type];
if (!render) return null;
return render({ event, /* ... */ });Two takeaways:
- Screenshots are already a registered renderer — on the event axis, not the tool axis. So the new
TOOL_RENDERERSis a sibling registry one level down (dispatch on tool name within atool_call). Worth being explicit that these are two different axes so future "render types" land in the right one: rich/inline media -> anartifactevent (existing path); custom framing of a tool's own result -> a tool renderer. - The new registry should match the established one's conventions (
Partial<Record<...>>, render-fn + shared prop bag, single fallback) rather than introduce a parallelRecord<string, ComponentType<FullProps>>shape.
apply_patch is the real test — and it doesn't fit the current contract
The genuinely in-disguise tool renderer today is apply_patch, special-cased inside the generic ToolCallItem body (tool-call-item.tsx:66, plus the Patch: block further down). Moving it into the registry is the litmus test for whether the abstraction earns its keep — slack-notify doesn't prove much, because it was already a standalone component.
The catch: apply_patch does not fit the current ComponentType<ToolCallItemProps> contract without duplication. The tell is what each renderer overrides:
| renderer | icon | collapsed summary | expanded body |
|---|---|---|---|
slack-notify |
custom | custom | custom |
apply_patch |
generic | generic | generic + one extra Patch section |
slack-notify wants total control, so "render the whole item" fits it by accident. apply_patch wants the generic collapsed line and most of the generic body, overriding only part of the body — and the full-component contract has no way to express partial override. Migrating it as-is forces you to copy the entire shell and re-implement the generic args/output body (and export the private ToolIcon), adding ~50 lines of duplication to delete ~4 lines of branching. Net negative — so in practice it wouldn't get migrated, and the registry would stay a one-entry slack-notify holder.
Under a slot contract it's a one-liner instead:
interface ToolRenderer {
icon?: (e: ToolCallEvent) => ReactNode; // optional - defaults to formatToolCall
summary?: (e: ToolCallEvent) => ReactNode; // optional - defaults to formatToolCall
Body: (props: { event: ToolCallEvent }) => ReactNode;
}
const TOOL_RENDERERS: Partial<Record<string, ToolRenderer>> = {
"slack-notify": { icon: slackIcon, summary: slackSummary, Body: SlackNotifyBody },
"apply_patch": { Body: ApplyPatchBody }, // icon/summary fall back to generic
};ToolCallItem owns the shell and renders the generic body as the default; apply_patch provides only its Body (reusing a shared ToolDetails with an extra patch prop), and the special-casing leaves ToolCallItem entirely.
Recommendation
- Preferred: adopt the slot contract
{ icon?, summary?, Body }+ extract a sharedToolCallShell/ToolDetails, and migrateapply_patchin this PR as the proof. Adding the next tool is then genuinely one registry line. - Acceptable minimum: keep
Record<string, ComponentType<...>>but extract a sharedToolCallShellevery renderer wraps, soapply_patchfits without shell duplication. (Still re-derives icon/summary per renderer; can't register a body-only tool.) - Not recommended: ship the contract as-is —
apply_patchcan't move in cleanly, so the registry never proves itself and theToolCallItemspecial-casing stays.
Happy to provide a concrete diff for the preferred version if that's useful.
| } | ||
|
|
||
| //Registry | ||
| const TOOL_RENDERERS: Record<string, React.ComponentType<ToolCallItemProps>> = { |
There was a problem hiding this comment.
[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.
|
|
||
| export function ToolCallItem({ event, isExpanded, onToggle, showTime = true }: ToolCallItemProps) { | ||
| if (event.tool === "slack-notify") { | ||
| const Renderer = TOOL_RENDERERS[event.tool]; |
There was a problem hiding this comment.
[Automated Review] Two small things on the lookup key:
- Case-sensitivity divergence. This uses the raw
event.tool, but the canonical dispatch intool-formatters.tsnormalizes withtool?.toLowerCase(). So a renderer registered here only matches an exact-case tool name, whileformatToolCallmatches case-insensitively. Not a regression (the old=== "slack-notify"was also exact), but it bakes an inconsistency into the new seam — whoever addsget-task-statushas to remember the registry is case-sensitive. Suggest keying onevent.tool?.toLowerCase()to match the rest of the file. - This
const Renderer = registry[key]; if (Renderer) { ... }shape is the same dispatchEventItemalready does witheventRenderersinsession-timeline.tsx. Aligning the two (same contract shape + fallback) keeps the rendering layer coherent.
|
Thanks for the detailed review. I spent some time digging into
I'm exploring a slot-based approach where |
2 participants
Summary
Introduce a simple tool renderer registry and migrate the existing
slack-notifycustom renderer to use it.Changes
TOOL_RENDERERSregistry mapping tool names to custom renderer components.slack-notifyconditional inToolCallItemwith a registry lookup.React.ComponentType<ToolCallItemProps>to support future custom renderers.Motivation
This is a first step toward supporting additional custom tool renderers without adding more tool-specific conditionals inside
ToolCallItem.With this structure in place, future renderers (such as
get-task-status) can be added by registering a component rather than modifying the core rendering logic.Summary by CodeRabbit