refactor(frontend): decompose sessions.js tool rendering into dispatc…#62
Merged
Merged
Conversation
…h registry Extract tool use and tool result rendering from sessions.js into static/js/render/ with TOOL_USE_RENDERERS and TOOL_RESULT_RENDERERS, one module per tool type, mirroring utils/tool_dispatch.py. sessions.js retains workspace and message orchestration only. Add Vitest coverage for registry wiring, HTML escaping, and getToolSummary. Document the frontend registry in docs/architecture.md. No intended visual or behavior change for existing sessions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughRefactors frontend tool rendering into a centralized registry (static/js/render/registry.js), adds per-tool use/result renderer modules, updates sessions.js to call the registry, and adds Vitest coverage plus docs noting the registry and its test. ChangesTool Rendering Registry Decomposition
sequenceDiagram
participant Sessions as static/js/sessions.js
participant Registry as static/js/render/registry.js
participant ToolUse as static/js/render/tool_use/*
participant ToolResult as static/js/render/tool_result/*
Sessions->>Registry: renderToolUse(tool)
Registry->>ToolUse: dispatch to registered renderer or fallback
Sessions->>Registry: renderToolResult(parsed)
Registry->>ToolResult: dispatch to registered renderer or fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/js/render/registry.js`:
- Around line 30-40: TOOL_USE_RENDERERS currently omits the known web tool names
so WebSearch and WebFetch calls fall back to the generic renderer; update the
registry constant TOOL_USE_RENDERERS to include entries for WebSearch and
WebFetch mapping to the proper renderers (renderWebSearchUse and
renderWebFetchUse) and ensure those renderer functions are imported or
implemented in this module (or referenced correctly if they live elsewhere) so
web tool uses are rendered with the intended UI rather than the fallback.
In `@static/js/render/registry.test.js`:
- Around line 82-90: The test currently uses the real tool name "WebFetch" which
hides missing-registry bugs; update the test that calls renderToolUse(...) to
use a clearly fictitious tool name (e.g., "UnknownToolXYZ") so the JSON fallback
path is exercised, and add or update tests that explicitly register/cover the
real web tools (the registry/registration helper used by your codebase) so that
real web tools are asserted separately; locate references to renderToolUse and
the test block "renderToolUse fallback" and modify the tool name and add a
sibling test that calls the actual registry registration function for web tools
to ensure they are covered.
In `@static/js/render/tool_result/bash.js`:
- Around line 5-7: The status string can end up as "exit undefined" when
parsed.exit_code is missing; update the logic around exitCode/status/summary to
provide a fallback (e.g., use a safeExit variable like typeof parsed.exit_code
=== 'number' ? parsed.exit_code : 'unknown' or 'N/A') and use that safeExit in
the final branch that builds status and summary so the summary never contains
"undefined".
In `@static/js/render/tool_result/utils.js`:
- Around line 1-7: toolResultHasBody currently skips several result types that
render only a summary; update the function (toolResultHasBody) to treat
'file_write', 'grep', 'plan', 'web_fetch', and 'web_search' as having a body
when their renderers will show a summary by checking parsed.summary (or
parsed.summary_html) or parsed.title — e.g., add explicit conditions for those
result_type values that return true when parsed.summary || parsed.summary_html
|| parsed.title is truthy so renderUser() will emit the standalone tool-result
bubble for summary-only results.
In `@static/js/render/tool_use/edit.js`:
- Around line 9-10: The current checks drop sections when old_string or
new_string are empty strings; change the truthiness guards to explicit
undefined/null checks so empty strings still render: replace the conditions
around inp.old_string and inp.new_string with checks like "if (inp.old_string
!== undefined && inp.old_string !== null)" (and same for inp.new_string) so the
Edit tool will render empty Old/New sections rather than omitting them; keep the
existing esc(truncate(...)) rendering logic and styling unchanged.
In `@static/js/render/tool_use/summary.js`:
- Around line 4-11: The Bash, WebFetch, and WebSearch branches return raw
user-controlled strings (inp.command, inp.url, inp.query) which must be
HTML-escaped; update the summary generation so these branches call the existing
esc() helper (and still apply truncate() where used) instead of returning raw
values—i.e., for the Bash branch use esc(inp.command || '') wrapped/applied with
truncate as needed, and for WebFetch/WebSearch call esc(inp.url || '') and
esc(inp.query || '') (respectively) when building their returned strings so all
user text flows through esc().
In `@static/js/render/tool_use/write.js`:
- Around line 8-9: The content section is omitted when inp.content is an empty
string because the code uses a truthiness check; change the condition to detect
the presence of the content property instead (e.g., check "'content' in inp" or
"inp.content !== undefined") so that an explicit empty-string write still
renders the Content section; update the block that builds the body (referencing
variables body, inp.content, esc, and truncate) to use that presence check and
keep the existing esc/truncate rendering logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ff48947-b9d5-413a-9a4b-98e8da1c6f5d
📒 Files selected for processing (31)
docs/architecture.mdstatic/js/render/registry.jsstatic/js/render/registry.test.jsstatic/js/render/tool_result/bash.jsstatic/js/render/tool_result/common.jsstatic/js/render/tool_result/fallback.jsstatic/js/render/tool_result/file_edit.jsstatic/js/render/tool_result/file_read.jsstatic/js/render/tool_result/file_write.jsstatic/js/render/tool_result/glob.jsstatic/js/render/tool_result/grep.jsstatic/js/render/tool_result/plan.jsstatic/js/render/tool_result/task.jsstatic/js/render/tool_result/todo_write.jsstatic/js/render/tool_result/user_input.jsstatic/js/render/tool_result/utils.jsstatic/js/render/tool_result/web_fetch.jsstatic/js/render/tool_result/web_search.jsstatic/js/render/tool_use/ask_user_question.jsstatic/js/render/tool_use/bash.jsstatic/js/render/tool_use/common.jsstatic/js/render/tool_use/edit.jsstatic/js/render/tool_use/fallback.jsstatic/js/render/tool_use/glob.jsstatic/js/render/tool_use/grep.jsstatic/js/render/tool_use/read.jsstatic/js/render/tool_use/summary.jsstatic/js/render/tool_use/task.jsstatic/js/render/tool_use/todo_write.jsstatic/js/render/tool_use/write.jsstatic/js/sessions.js
…h registry Extract tool use and tool result rendering from sessions.js into static/js/render/ with TOOL_USE_RENDERERS and TOOL_RESULT_RENDERERS, one module per tool type, mirroring utils/tool_dispatch.py. Register WebFetch and WebSearch explicitly (JSON body via fallback helper). Fix bash result summary when exit_code is missing (use "unknown" not "exit undefined"). Tighten Vitest: fictitious tool for fallback path, separate WebFetch/WebSearch registry tests. Add Vitest coverage for registry wiring, HTML escaping, and getToolSummary. Document the frontend registry in docs/architecture.md. No intended visual or behavior change for existing sessions.
Collaborator
Author
|
Please review this PR fully again |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
static/js/render/registry.test.js (1)
99-107: 💤 Low valueAssertions don't actually distinguish the WebFetch renderer from the JSON fallback.
The test name promises "not unknown-tool path", but
tool-call/example.com/<pre><code>would also appear in the generic JSON fallback. The real guarantee comes from the registration check at Lines 28-32. Consider asserting something renderer-specific (or it's fine to rely on the registration test and rename this case).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/render/registry.test.js` around lines 99 - 107, The test "renders WebFetch via registry (JSON body, not unknown-tool path)" doesn't prove the WebFetch renderer was used because the assertions (tool-call, example.com, <pre><code>) also match the JSON fallback; update the test to either assert a WebFetch-specific output (e.g. a unique CSS class/HTML fragment produced by the WebFetch renderer) by calling renderToolUse({name: 'WebFetch', ...}) and checking for that marker, or rename the test to indicate it only verifies rendering output generically and rely on the existing registration test that confirms the renderer is registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@static/js/render/registry.js`:
- Around line 61-70: The registry lookups in renderToolUse and renderToolResult
rely on direct property access (TOOL_USE_RENDERERS[name] ?? fallback) which can
hit prototype properties like "__proto__" or "constructor"; change both
dispatches to use an own-property check (e.g.,
Object.prototype.hasOwnProperty.call(TOOL_USE_RENDERERS, name) ?
TOOL_USE_RENDERERS[name] : renderToolUseFallback and similarly for
TOOL_RESULT_RENDERERS with parsed.result_type/rt) so that only own keys are used
and the fallback is reliably chosen when no renderer is defined.
In `@static/js/render/tool_result/todo_write.js`:
- Around line 5-13: The summary count can mismatch because count is set from
parsed.todo_count independently of parsed.todos; change the summary computation
to derive count from the same source you iterate (e.g., set count = typeof
parsed.todo_count === 'number' ? parsed.todo_count : (parsed.todos ?
parsed.todos.length : 0)) and then build summary = `Todos updated (${count}
items)` so that count, parsed.todo_count and parsed.todos stay consistent with
the rendered body; update the code surrounding count, summary and the for-loop
that iterates parsed.todos accordingly.
---
Nitpick comments:
In `@static/js/render/registry.test.js`:
- Around line 99-107: The test "renders WebFetch via registry (JSON body, not
unknown-tool path)" doesn't prove the WebFetch renderer was used because the
assertions (tool-call, example.com, <pre><code>) also match the JSON fallback;
update the test to either assert a WebFetch-specific output (e.g. a unique CSS
class/HTML fragment produced by the WebFetch renderer) by calling
renderToolUse({name: 'WebFetch', ...}) and checking for that marker, or rename
the test to indicate it only verifies rendering output generically and rely on
the existing registration test that confirms the renderer is registered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a608499-9abb-4bd1-960a-ccdd21707366
📒 Files selected for processing (33)
docs/architecture.mdstatic/js/render/registry.jsstatic/js/render/registry.test.jsstatic/js/render/tool_result/bash.jsstatic/js/render/tool_result/common.jsstatic/js/render/tool_result/fallback.jsstatic/js/render/tool_result/file_edit.jsstatic/js/render/tool_result/file_read.jsstatic/js/render/tool_result/file_write.jsstatic/js/render/tool_result/glob.jsstatic/js/render/tool_result/grep.jsstatic/js/render/tool_result/plan.jsstatic/js/render/tool_result/task.jsstatic/js/render/tool_result/todo_write.jsstatic/js/render/tool_result/user_input.jsstatic/js/render/tool_result/utils.jsstatic/js/render/tool_result/web_fetch.jsstatic/js/render/tool_result/web_search.jsstatic/js/render/tool_use/ask_user_question.jsstatic/js/render/tool_use/bash.jsstatic/js/render/tool_use/common.jsstatic/js/render/tool_use/edit.jsstatic/js/render/tool_use/fallback.jsstatic/js/render/tool_use/glob.jsstatic/js/render/tool_use/grep.jsstatic/js/render/tool_use/read.jsstatic/js/render/tool_use/summary.jsstatic/js/render/tool_use/task.jsstatic/js/render/tool_use/todo_write.jsstatic/js/render/tool_use/web_fetch.jsstatic/js/render/tool_use/web_search.jsstatic/js/render/tool_use/write.jsstatic/js/sessions.js
…h registry Extract tool use and tool result rendering into static/js/render/ with TOOL_USE_RENDERERS and TOOL_RESULT_RENDERERS (one module per type), mirroring utils/tool_dispatch.py. sessions.js keeps workspace orchestration. Harden registry dispatch with hasOwnProperty lookups; register WebFetch and WebSearch explicitly (JSON body via fallback helper). Fix bash result summary when exit_code is missing; align todo_write summary count with parsed.todos. Add Vitest for registry wiring, escaping, fallback vs registered tools, and todo_write count consistency. Document render/registry.js in architecture.md.
Collaborator
|
The string 'unknown' is used as both "the value when tool.name is empty" and (implicitly, via the hasOwnProperty miss) the fallback-routing key. Three places agree on this convention. If anyone ever adds TOOL_USE_RENDERERS.unknown = …, behavior silently changes. Either add a short comment naming it a sentinel, or use a Symbol-based sentinel that can't collide with a real tool name. Minor — current code works. |
timon0305
approved these changes
Jun 2, 2026
…Body Introduce UNKNOWN_DISPATCH_KEY sentinel with documented non-registration rule. Guard renderToolUse/renderToolResult against null input; guard toolResultHasBody similarly. Add Vitest coverage for sentinel, null args, and toolResultHasBody contract used by sessions.js.
Collaborator
Author
|
@CodeRabbit, do you think my last commit addressed 3 comments of @timon0305 ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all PR review comments by timon0305
gh pr view 62 --repo cppalliance/claude-code-chat-browser --json reviews,comments 2>/dev/null | jq '.reviews[] | select(.author.login == "timon0305")' 2>/dev/null || true
echo "--- PR review comments ---"
gh api repos/cppalliance/claude-code-chat-browser/pulls/62/comments 2>/dev/null | jq '.[] | select(.user.login == "timon0305") | {id: .id, body: .body, path: .path, line: .line}' 2>/dev/null || true
echo "--- PR issue comments ---"
gh api repos/cppalliance/claude-code-chat-browser/issues/62/comments 2>/dev/null | jq '.[] | select(.user.login == "timon0305") | {id: .id, body: .body}' 2>/dev/null || trueLength of output: 2391 🏁 Script executed: #!/bin/bash
# Read the new constants.js sentinel file
cat static/js/render/constants.js
echo "--- registry.js ---"
cat static/js/render/registry.js
echo "--- fallback.js (tool_use) ---"
cat static/js/render/tool_use/fallback.js
echo "--- fallback.js (tool_result) ---"
cat static/js/render/tool_result/fallback.jsLength of output: 4780
The only design choice worth noting: the sentinel remains a string constant rather than a (´• ω •`) 🐇 |
wpak-ai
approved these changes
Jun 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #59
Summary
Decompose
sessions.jstool rendering intostatic/js/render/dispatch registry, mirroring backendutils/tool_dispatch.py.TOOL_USE_RENDERERSandTOOL_RESULT_RENDERERSinstatic/js/render/registry.jsrender/tool_use/) and perresult_type(render/tool_result/), with fallbacks for unknown toolssessions.jsto workspace/session orchestration and message bubbles; tool cards delegate to the registrystatic/js/render/registry.test.js(registry keys, HTML escaping,getToolSummary, fallbacks)docs/architecture.mdCloses Chen W1 sprint item #1 (8 pt).
Motivation
static/js/sessions.jshad longif/elsechains forrenderToolUseandrenderToolResult. Every new tool type required editing that file. The backend already uses a dispatch table inutils/tool_dispatch.py; this PR brings the same pattern to the frontend with direct key lookup plus fallback renderers.Test plan
npm test— includes newregistry.test.js(25 tests total)pytest -q— 324 passedpython app.py --host 127.0.0.1 --port 5000smoke on a real sessionAcceptance criteria
sessions.jsno longer contains multi-page toolif/elsechainsregistry.jsnpm testandpytest -qgreen locallyOut of scope
tool_dispatch.pychangesexport_count(Thursday #4)Files changed
static/js/render/registry.jsstatic/js/render/tool_use/*.jsstatic/js/render/tool_result/*.jsstatic/js/sessions.jsstatic/js/render/registry.test.jsdocs/architecture.mdSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Documentation