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
|
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 labels: 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.
| 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.
| 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.
| /** | ||
| * 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.
github-actions
Bot
added
awaiting-author
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.
9d4172a to
bbc9723
Compare
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
added
the
awaiting-review
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