claude-code-chat-browser: Frontend tool registry - link to backend dispatch table via generated manifest#105
Merged
Conversation
Generate static/tool_types.json from KNOWN_TOOL_TYPES and load it at SPA boot to cross-check TOOL_USE_RENDERERS with console.warn on drift. Improve unknown tool/result fallbacks to show raw type names and JSON payloads. Extend test_tool_dispatch_sync.py with manifest freshness and coverage assertions. Await manifest init before first route render; document the flow in CONTRIBUTING.md, Makefile, and docs/architecture.md.
|
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 (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR generates ChangesTool types manifest and frontend sync
Sequence Diagram(s)sequenceDiagram
participant Browser
participant app.js
participant tool_types_manifest.js
participant Server
participant TOOL_USE_RENDERERS
Browser->>app.js: DOMContentLoaded
app.js->>tool_types_manifest.js: initToolTypesManifest()
tool_types_manifest.js->>Server: fetch /static/tool_types.json (5s timeout)
Server-->>tool_types_manifest.js: { tool_types: [...] }
tool_types_manifest.js->>TOOL_USE_RENDERERS: compare backend vs frontend types
alt mismatch found
tool_types_manifest.js-->>Browser: console.warn(drift message)
else fetch fails or times out
tool_types_manifest.js-->>Browser: console.warn(load/timeout error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (2)
Makefile (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the new target to
.PHONY.
gen-tool-types-manifestisn't a file-producing target by name, so omitting it from.PHONYis low-risk today, but it's inconsistent with the existing convention (line 1 lists every other target) and could bite if a same-named file ever appears in the repo root.🔧 Proposed fix
-.PHONY: seed-baselines-local update-baselines check-benchmarks clean-benchmark-artifacts +.PHONY: seed-baselines-local update-baselines check-benchmarks clean-benchmark-artifacts gen-tool-types-manifest🤖 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 `@Makefile` around lines 1 - 16, The Makefile target gen-tool-types-manifest is missing from the .PHONY declaration, which is inconsistent with the other non-file targets. Update the .PHONY list at the top of the Makefile to include gen-tool-types-manifest so make always runs the gen-tool-types-manifest rule even if a same-named file appears.tests/test_tool_dispatch_sync.py (1)
88-97: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNon-string manifest entries are silently coerced rather than rejected.
frozenset(str(t) for t in raw)stringifies every entry instead of validating that all entries are strings. The frontend'stool_types_manifest.jsfilters non-string entries out entirely (types.filter((t) => typeof t === 'string')), so a malformed manifest (e.g. containing a number) would pass this backend test (coerced to a numeric string) while being silently dropped on the frontend — masking a real generator bug.🔧 Proposed fix
def _load_manifest_tool_types(path: Path) -> frozenset[str]: if not path.is_file(): msg = f"Missing manifest: {path} (run python scripts/gen_tool_types_manifest.py)" raise ValueError(msg) data = json.loads(path.read_text(encoding="utf-8")) raw = data.get("tool_types") if not isinstance(raw, list): msg = f"Invalid tool_types in {path}: expected a JSON array" raise ValueError(msg) - return frozenset(str(t) for t in raw) + if not all(isinstance(t, str) for t in raw): + msg = f"Invalid tool_types in {path}: expected all entries to be strings" + raise ValueError(msg) + return frozenset(raw)🤖 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 `@tests/test_tool_dispatch_sync.py` around lines 88 - 97, The manifest parser in _load_manifest_tool_types is coercing every entry with str(t) instead of validating types, so malformed non-string entries can slip through. Update this helper to reject any non-string item in raw by checking each element before building the frozenset, and raise a ValueError that clearly points to the invalid tool_types content in the manifest. Keep the same validation flow used by the existing JSON array check, but make the final return only include entries that were already confirmed as strings.
🤖 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/app.js`:
- Around line 52-56: The DOMContentLoaded bootstrap in app.js is blocking
initial render by awaiting initToolTypesManifest(), even though this diagnostic
load does not gate route handling or UI correctness. Update the bootstrap flow
to call initToolTypesManifest() fire-and-forget instead of awaiting it, keeping
applyTheme() and the footer year update synchronous so the app can continue
booting immediately. Use the existing initToolTypesManifest() symbol as the
non-blocking background task and leave route handling off the critical path.
In `@static/js/render/tool_types_manifest.js`:
- Around line 10-20: The manifest load in initToolTypesManifest can hang
indefinitely because fetch(MANIFEST_URL) has no timeout, which blocks app.js
startup before handleRoute() runs. Add an AbortController-based timeout around
the fetch in initToolTypesManifest so the request is canceled after a bounded
wait, and handle the abort case with a warning similar to the existing HTTP
failure path. Keep the existing manifest parsing and setManifestToolTypes flow
unchanged when the request succeeds.
---
Nitpick comments:
In `@Makefile`:
- Around line 1-16: The Makefile target gen-tool-types-manifest is missing from
the .PHONY declaration, which is inconsistent with the other non-file targets.
Update the .PHONY list at the top of the Makefile to include
gen-tool-types-manifest so make always runs the gen-tool-types-manifest rule
even if a same-named file appears.
In `@tests/test_tool_dispatch_sync.py`:
- Around line 88-97: The manifest parser in _load_manifest_tool_types is
coercing every entry with str(t) instead of validating types, so malformed
non-string entries can slip through. Update this helper to reject any non-string
item in raw by checking each element before building the frozenset, and raise a
ValueError that clearly points to the invalid tool_types content in the
manifest. Keep the same validation flow used by the existing JSON array check,
but make the final return only include entries that were already confirmed as
strings.
🪄 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: 94c23888-412a-4ef5-bcfa-0521c644e25a
📒 Files selected for processing (15)
CONTRIBUTING.mdMakefiledocs/architecture.mdscripts/gen_tool_types_manifest.pystatic/js/app.jsstatic/js/app.test.jsstatic/js/render/registry.jsstatic/js/render/registry.test.jsstatic/js/render/tool_result/fallback.jsstatic/js/render/tool_types_manifest.jsstatic/js/render/tool_types_manifest.test.jsstatic/js/render/tool_types_state.jsstatic/js/render/tool_use/fallback.jsstatic/tool_types.jsontests/test_tool_dispatch_sync.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
static/js/render/tool_types_manifest.test.js (1)
50-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFake timers leak on assertion failure.
vi.useRealTimers()runs only after theexpect()calls. If either assertion throws, fake timers stay active and bleed into subsequent tests in the file, causing hard-to-diagnose flaky failures elsewhere in the suite.♻️ Proposed fix using try/finally
it('warns when fetch times out', async () => { vi.useFakeTimers(); - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); - vi.stubGlobal( - 'fetch', - vi.fn((_url, init) => - new Promise((_resolve, reject) => { - init?.signal?.addEventListener('abort', () => { - reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); - }); - }), - ), - ); - - const promise = initToolTypesManifest(); - await vi.advanceTimersByTimeAsync(5000); - await promise; - - expect(getManifestToolTypes()).toBeNull(); - expect(warn).toHaveBeenCalledWith( - '[tool registry] Could not load /static/tool_types.json: timed out after 5000ms', - ); - vi.useRealTimers(); + try { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.stubGlobal( + 'fetch', + vi.fn((_url, init) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => { + reject(Object.assign(new Error('aborted'), { name: 'AbortError' })); + }); + }), + ), + ); + + const promise = initToolTypesManifest(); + await vi.advanceTimersByTimeAsync(5000); + await promise; + + expect(getManifestToolTypes()).toBeNull(); + expect(warn).toHaveBeenCalledWith( + '[tool registry] Could not load /static/tool_types.json: timed out after 5000ms', + ); + } finally { + vi.useRealTimers(); + } });🤖 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/tool_types_manifest.test.js` around lines 50 - 73, The fake timer setup in initToolTypesManifest's timeout test can leak if an assertion fails because vi.useRealTimers() is only reached at the end of the test. Wrap the timer-enabled test body in a try/finally so the cleanup always runs, and keep the assertions around getManifestToolTypes() and the warn spy inside the try block to ensure subsequent tests are never affected by leftover fake timers.
🤖 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.
Nitpick comments:
In `@static/js/render/tool_types_manifest.test.js`:
- Around line 50-73: The fake timer setup in initToolTypesManifest's timeout
test can leak if an assertion fails because vi.useRealTimers() is only reached
at the end of the test. Wrap the timer-enabled test body in a try/finally so the
cleanup always runs, and keep the assertions around getManifestToolTypes() and
the warn spy inside the try block to ensure subsequent tests are never affected
by leftover fake timers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 511d1910-d41a-4f63-b3f4-6b5858255c67
📒 Files selected for processing (6)
Makefilestatic/js/app.jsstatic/js/app.test.jsstatic/js/render/tool_types_manifest.jsstatic/js/render/tool_types_manifest.test.jstests/test_tool_dispatch_sync.py
💤 Files with no reviewable changes (1)
- static/js/app.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- tests/test_tool_dispatch_sync.py
Wrap the initToolTypesManifest timeout test in try/finally so vi.useRealTimers() runs even when assertions fail. Also ruff-format test_tool_dispatch_sync.py manifest validation helper (CI gate).
timon0305
approved these changes
Jun 30, 2026
wpak-ai
approved these changes
Jul 1, 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 #100
Summary
Links the frontend
TOOL_USE_RENDERERSmap to the backendKNOWN_TOOL_TYPESregistry via a generatedstatic/tool_types.jsonmanifest. On SPA boot, the app fetches the manifest, cross-checks the renderer map, and logsconsole.warnwhen they diverge. Unknown tool fallbacks now show the raw type name and JSON payload instead of failing silently.Sprint item #4 (Tuesday PR 2 of 2). Stacks on PR #1 (#104 —
KNOWN_TOOL_TYPESregistry + sync CI).Problem
The SPA maintained its own tool renderer map with no runtime or build-time linkage to the backend. New backend tool types fell through to a generic fallback without any operator signal.
Changes
scripts/gen_tool_types_manifest.py(new) — writesstatic/tool_types.jsonfromKNOWN_TOOL_TYPESstatic/tool_types.json(new) — committed manifest (11 tool types)static/js/render/tool_types_state.js(new) — module-level manifest Set (avoids circular imports)static/js/render/tool_types_manifest.js(new) — fetch manifest on init; bidirectional drift warningsstatic/js/render/tool_types_manifest.test.js(new) — vitest for init cross-check and fetch failurestatic/js/app.js—await initToolTypesManifest()beforehandleRoute()static/js/render/registry.js— simplified fallback dispatch (drift warnings at init only)static/js/render/tool_use/fallback.js—Unknown tool: {name}+ JSON inputstatic/js/render/tool_result/fallback.js—Unknown tool result: {type}+ JSON payloadtests/test_tool_dispatch_sync.py— manifest vsKNOWN_TOOL_TYPES, freshness check, registry vs manifest in one testCONTRIBUTING.md— regen step:python scripts/gen_tool_types_manifest.pyMakefile—gen-tool-types-manifesttargetdocs/architecture.md— manifest boot flow documentedOut of scope
GET /api/tool-typesendpoint (Option B)Test plan
pytest tests/test_tool_dispatch_sync.py -q(6 passed)pytest -q(419 passed)npm test(84 passed)ruff check .andruff format --check .mypy -p api -p utils -p modelstool_types.jsonwithout aTOOL_USE_RENDERERSentry — confirmconsole.warnon page load, then revertSummary by CodeRabbit
New Features
Bug Fixes
Documentation