feat(webview): add batchNearby utility for smarter tool ask batching - #1005
feat(webview): add batchNearby utility for smarter tool ask batching#1005easonLiangWorldedtech wants to merge 3 commits into
Conversation
|
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 Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe chat view now batches same-type tool asks across ignorable streaming rows while preserving semantic boundaries. A generic ChangesNearby tool-ask batching
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ChatView
participant batchNearby
participant ToolBatchSynthesizer
ChatView->>batchNearby: classify streamed tool-ask rows
batchNearby->>batchNearby: skip ignorable rows and stop at boundaries
batchNearby->>ToolBatchSynthesizer: synthesize grouped tool asks
ToolBatchSynthesizer-->>batchNearby: return batched tool-ask row
batchNearby-->>ChatView: return processed chat rows
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@webview-ui/src/utils/batchNearby.ts`:
- Around line 42-67: Update batchNearby in webview-ui/src/utils/batchNearby.ts
(lines 42-67) to track skipped isIgnorableBetweenTargets items in
pendingIgnorable, clear them only when a subsequent target is found, and restore
remaining items to result after finalizing the batch. Make no code change in
webview-ui/src/components/chat/ChatView.tsx (lines 1311-1328). Update the
boundary-message test in webview-ui/src/utils/__tests__/batchNearby.spec.ts
(lines 135-152) to expect four items, including the restored api_req_started
item between match-1 and visible text.
🪄 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 Plus
Run ID: 421939d2-6c08-49b5-989a-af52340d01f0
📒 Files selected for processing (3)
webview-ui/src/components/chat/ChatView.tsxwebview-ui/src/utils/__tests__/batchNearby.spec.tswebview-ui/src/utils/batchNearby.ts
github-actions
Bot
added
the
awaiting-review
| expect(result[1].text).toBe("done") | ||
| }) | ||
|
|
||
| test("non-ignorable non-target message stops batching", () => { |
There was a problem hiding this comment.
This test has no ignorable message before command_output, so pendingIgnorable is empty when the else break fires — the restore path at batchNearby.ts:68–71 is never actually exercised here. What happens with input like [match-1, api_req_started, command_output, match-2]? The api_req_started should be restored to the output but no test currently covers that shape.
There was a problem hiding this comment.
This is already covered now. The regression test starts on batchNearby.spec.ts and uses exactly the shape you described:
match-1atbatchNearby.spec.ts- pending ignorable
api_req_startedatbatchNearby.spec.ts - non-ignorable
command_outputatbatchNearby.spec.ts match-2atbatchNearby.spec.ts
The assertions verify the restore path by checking that api_req_started is preserved in the output before command_output: batchNearby.spec.ts through batchNearby.spec.ts. This exercises the restore branch in batchNearby.ts, where pending ignorable messages are pushed back when the bridge fails.
| if (msg.type !== "say") return false | ||
| return ( | ||
| msg.say === "api_req_started" || | ||
| msg.say === "api_req_finished" || |
There was a problem hiding this comment.
Is api_req_finished reachable here? combineApiRequests (applied when building modifiedMessages) absorbs every api_req_finished into its paired api_req_started or drops it as an orphan, and visibleMessages adds a second filter at lines 1047–1050. If that's right, this arm can never fire and could be removed.
There was a problem hiding this comment.
Already addressed on this branch — the isIgnorableBetweenTargets arm in ChatView no longer treats api_req_finished as ignorable. It now only allows api_req_started, empty text, and reasoning rows: ChatView.tsx.
That means an api_req_finished row cannot be used as a bridge for batching tool asks. In practice it should still be unreachable there because modifiedMessages is built through combineApiRequests(...), and visibleMessages also filters api_req_finished out before rendering: ChatView.tsx, ChatView.tsx.
| m.say === "checkpoint_saved" || | ||
| m.say === "error" || | ||
| m.say === "condense_context" | ||
| ) |
There was a problem hiding this comment.
The production isBoundary in ChatView.tsx includes codebase_search_result but this fixture doesn't. Worth keeping them in sync — and a test with [readFileAsk, codebase_search_result_msg, readFileAsk] would confirm it stops the batch.
There was a problem hiding this comment.
Already covered. The test fixture isBoundary now includes codebase_search_result on batchNearby.spec.ts, matching the production boundary predicate in ChatView.tsx.
There is also a regression test for the exact boundary shape starting on batchNearby.spec.ts: it uses [match-1, codebase_search_result, match-2] on lines 272-274, then asserts the messages remain separate on lines 282-285. So codebase_search_result stops batching rather than being bridged.
| /** | ||
| * Batch tool asks that are near each other, allowing ignorable messages in between. | ||
| * | ||
| * Unlike `batchConsecutive` which only merges truly adjacent items, this function |
There was a problem hiding this comment.
Since ChatView.tsx was the only caller of batchConsecutive, that file and its spec are now dead code. Can you delete them in this PR to avoid confusing future readers. Dead code will start being enforced via knip soon.
There was a problem hiding this comment.
removed
github-actions
Bot
added
awaiting-author
9d4172a to
bbc9723
Compare
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
added
the
awaiting-review
Replace batchConsecutive with batchNearby in ChatView.tsx to allow merging same-type tool asks even when separated by ignorable messages (api_req_started/finished, empty text rows, reasoning). Semantic boundaries (user_feedback, visible text, completion_result, checkpoint_saved, error) stop the merge, preserving correct ordering. This fixes UX where models like qwen insert API request rows between tool calls, causing UI to show multiple 'Zoo wants to read this file' instead of a single batched prompt.
When a single target is followed by ignorable messages and then a boundary (no second target found), the original code silently dropped those ignorable items — contradicting the documented contract that all items are preserved in-order. Fix: track skipped ignorable items in pendingIgnorable, flush them after the batch result so they're never lost. When bridge succeeds (second target found), pending items are consumed into the synthesized batch as expected.
4ef7a8c to
26b3351
Compare
| if (m.type !== "say") return false | ||
| return ( | ||
| m.say === "api_req_started" || | ||
| m.say === "api_req_finished" || |
There was a problem hiding this comment.
The latest commit dropped api_req_finished from production isIgnorableBetweenTargets (ChatView.tsx:1289), but the fixture here still lists it — and the tests below (~lines 350, 367, 386) rely on it being ignorable. Since it can't reach batchNearby in production anyway (combineApiRequests merges/drops it and the visibleMessages filter strips it again), should we drop it from the fixture too so the tests model the real predicate?
| // - reasoning rows (hidden from user by default) | ||
| const isIgnorableBetweenTargets = (msg: ClineMessage): boolean => { | ||
| if (msg.type !== "say") return false | ||
| return msg.say === "api_req_started" || (msg.say === "text" && !msg.text?.trim()) || msg.say === "reasoning" |
There was a problem hiding this comment.
These predicates are inline closures, and the spec's copies are hand-synced — which has now drifted twice (the codebase_search_result addition, and the api_req_finished comment on the spec). Two suggestions:
- Export
isIgnorableBetweenTargets/isBoundaryfrom a small shared module so the spec imports the real predicates instead of copies. - Add a
ChatViewJSDOM test that feeds[readFile ask, api_req_started, readFile ask]and asserts one batched approval row renders instead of two — pins the UI tool ask batching fails when models insert API rows between tool calls #1004 regression where it lives (behavior-only per webview-ui/AGENTS.md, so Vitest rather than visual or e2e).
github-actions
Bot
added
awaiting-author
4 participants
UI tool ask batching fails when models insert API rows between tool calls
Problem
When using models like qwen, the UI shows multiple separate prompts for the same type of tool call:
Instead of a single batched prompt:
Root Cause
The existing
batchConsecutive()utility only merges tool asks that are truly adjacent in the message array. However, models like qwen insert low-information messages between tool calls during streaming:say: "api_req_started"— API request metadata rowsay: "api_req_finished"— API request completion rowsay: "text"with empty content — partial streaming rowssay: "reasoning"— hidden reasoning textThese messages break the consecutive chain, so
batchConsecutivestops merging.Current Code (ChatView.tsx ~line 1272)
Proposed Solution:
batchNearby()utilityA new utility that merges same-type tool asks even when separated by ignorable messages. It uses three predicates:
isTarget— matches the target tool type (readFile, listFiles, editFile)isIgnorableBetweenTargets— skips over low-info messages without breaking the batchisBoundary— stops merging when hitting a semantic boundaryIgnorable Messages (skipped during batching)
api_req_started/api_req_finishedsay: "text"with no content)say: "reasoning")Semantic Boundaries (stop merging)
user_feedback,user_feedback_diff)textwith content)completion_result)checkpoint_saved)error)condense_context)codebase_search_result)New Usage in ChatView.tsx
Files Changed
webview-ui/src/utils/batchNearby.ts— the batching utility functionwebview-ui/src/utils/__tests__/batchNearby.spec.ts— 23 comprehensive testswebview-ui/src/components/chat/ChatView.tsx— replacebatchConsecutivewithbatchNearbyTesting
All 23 unit tests pass, covering:
Long-term Improvement (separate issue)
Add
toolCallGroupIdmetadata to backend messages so UI can use explicit grouping instead of heuristic-based merging. This would make batching work consistently across all model providers regardless of streaming format differences.Fixes #1004
Screen Cap
Summary by CodeRabbit
Summary by CodeRabbit