Repository navigation
perf(browser): dispatch coordinate pointer input in process instead of one subprocess per event - #20593
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 72cbef945899d825874652e7c2e53e62a0b241f0 and f9f3daf. 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between e84a272 and 72cbef945899d825874652e7c2e53e62a0b241f0. 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe bridge now sends coordinate-based mouse movement, button, and wheel events through CDP. It tracks pointer state per tab, supports back and forward buttons, preserves chorded-button state, counts multi-clicks, validates finite values, and rolls back failed dispatches. The mouse command bridge uses the shared pointer implementation. Unit and end-to-end tests cover dispatch payloads, errors, latency, and gesture behavior. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Repeated direct clicks cannot produce the tracked multi-click behavior, and mixing direct clicks with held-button gestures can leave gesture state inconsistent. Resolve this before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
src/main/browser/agent-browser-bridge-mouse-commands.ts (1)
49-66: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute
mouseClickthrough the shared pointer state.
mouseClickbypassesCdpPointerState. Two consecutivemouseClickcalls always sendclickCount: 1, so they cannot generate the new double-click behavior. A mixed sequence such asmouseDown('left'),mouseClick(..., 'right'), andmouseUp()also sends CDP button masks that omit the held left button while the tracked state still records it. CDP definesbuttonsas the buttons pressed when the event occurs. (chromedevtools.github.io)Use the shared pointer transition and dispatch path for this click sequence. Preserve the mobile touch-point handling, but update button masks and click counts from the same state as
mouseDownandmouseUp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: d9affb12-ff52-4712-8833-43f8c284b290
📥 Commits
Reviewing files that changed from the base of the PR and between 539d4d1 and c77ecd4f737d5ecf9ff96393faf720621157f470.
📒 Files selected for processing (7)
src/main/browser/agent-browser-bridge-command-transport.test.tssrc/main/browser/agent-browser-bridge-mouse-commands.tssrc/main/browser/agent-browser-bridge-mouse.tssrc/main/browser/agent-browser-bridge-pointer-commands.tssrc/main/browser/agent-browser-bridge-pointer-input.test.tssrc/main/browser/cdp-pointer-input.tstests/e2e/browser-pointer-input-dispatch.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review.
e84a272 to
72cbef9
Compare
|
Checked this carefully. It's a real gap, but it's pre-existing and not one this PR widens — and closing it here would trade a measured low-risk change for an unmeasured mobile behavior change. Declining, with reasoning: This PR does not alter - const buttons = cdpMouseButtonMask(cdpButton)
+ const buttons = cdpPointerButtonMask(cdpButton)
On the mixed-sequence claim. Tracing
The inconsistency is identical on both sides. I verified the analogous case empirically against the shipped agent-browser 0.27 binary, because I had the same worry. After On Why not fix it here. Routing Happy to open a follow-up issue for it if you'd like. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues. The dispatch/state machine checks out — one test-reliability suggestion inline.
Reviewed changes
- Coordinate pointer input now dispatched in process.
mouseMove/mouseDown/mouseUp/mouseWheelmove offexecAgentBrowserontowc.debugger.sendCommand('Input.dispatchMouseEvent', …), via a newAgentBrowserBridgePointerCommandsclass slotted betweenInputCommandsandMouseCommands; element-ref commands stay on the helper. - Per-tab pointer state added in
cdp-pointer-input.ts: aWeakMap<WebContents, CdpPointerState>holding position, held-button mask, active button, and click cadence, with a one-line rollback when a dispatch rejects. - Button-mask helpers generalized (
cdpMouseButtonMask→cdpPointerButtonMask, plusnormalizeCdpPointerButton/cdpPointerButtonFromMask) so back/forward pass through as X1/X2 on the coordinate path while the element-click path keeps its left coercion. - New tests:
agent-browser-bridge-pointer-input.test.ts(23 unit tests over payloads, chorded releases, click cadence, failure rollback, debugger lifecycle) andtests/e2e/browser-pointer-input-dispatch.spec.ts(latency vs abrowser.evalcontrol plus click/dblclick/right-click/drag-select/wheel fidelity);command-transport.test.tsswaps itsmouseMovehelper-spawn assertion forscroll.
I verified the deleted cdpMouseButtonMask has no remaining references, that MouseButton is OptionalPlainString (so back/forward reach the new passthrough) while MouseXY/MouseWheel.dy use z.custom<number> (so the new finiteness guard is load-bearing), and ran the three affected unit files (47 passing). A focused audit of the payload/state machine found no divergence from the CDP CdpMouse convention.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…f one subprocess per event
72cbef9 to
f9f3daf
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes (delta since the prior pullfrog review at c77ecd4)
- E2E gesture sequences now run in one renderer round-trip. New
driveRpcSequence/clickStepsreplace per-RPCclickAt, so harness IPC no longer sits between the two presses;pressGapMsis read from recorded event timestamps and asserted< 500before thedblclickcheck, turning a slow host into an explicit precondition failure rather than a misleading missing-dblclick. - Latency measurement recalibrated. Thresholds move to
3x/3x/10x(measured in-process1.5x/1.5x/4xvs helper5.7x/5.5x/20.5x),REPEATS60→12 to amortize the renderer's ~16.6ms clock, the unusedmouseClicksample is dropped, and the test timeout is raised to 240s.
Production files are unchanged in this commit; the pointer dispatch and state machine are identical to what I reviewed at c77ecd4. The prior e2e timing thread is addressed and resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes (delta since the 72cbef9 review)
- Double-click verification is now conditional rather than failing on a slow host. When
pressGapMs >= DOUBLE_CLICK_INTERVAL_MS, the spec records apointer-dblclick-unverifiedannotation and skips thedblclick/detail: 2assertions instead of failing; theclickcount is still asserted, and the click-cadence machine itself remains covered deterministically by the fake-timer unit tests. - Latency/cadence numbers are surfaced in CI.
latencyReportandcadenceReportride ontest.info().annotationsand asexpectmessages, so passing runs no longer depend on stdout that the CI job log drops.
Test-only change; no production files touched since the last review. The new DOUBLE_CLICK_INTERVAL_MS = 500 constant intentionally mirrors MULTI_CLICK_INTERVAL_MS, which is fine to keep local to the spec.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
Moving the mouse in the remote browser pane launches a whole new process on the host for every wiggle, click and scroll tick. A click is four of those. This makes the runtime deliver mouse events the same way it already delivers taps from the mobile app and keystrokes since #15310: directly, in process. A click drops from ~670ms to ~133ms, and double-click works through the pane for the first time.
Supersedes #15309, which was opened against a 2000-commit-old
mainand could no longer apply —agent-browser-bridge.tshas since been split into a class chain. This is the same idea rebuilt on currentmain, following the conventions #15310 established for keyboard input.What Changed
mouseMove/mouseDown/mouseUp/mouseWheelno longer go throughexecAgentBrowser(anexecFileof the agent-browser helper per event). They dispatchInput.dispatchMouseEventover the electron debugger — the transportmouseClickhas used since it was added for mobile, and thatkeypressmoved to in #15310. Error normalization, the{ ensureSession: false }queue option and the lease handling are copied from that PR so both input paths behave identically.Two new modules:
cdp-pointer-input.ts— per-tab pointer state (position, held-buttons mask, click cadence) in aWeakMapkeyed by the WebContents, so state dies with the tab.agent-browser-bridge-pointer-commands.ts— the four commands. They live in their own class because folding them intoagent-browser-bridge-mouse-commands.tspushed it to 387 lines, past the 300-line budget. Nomax-linesdisable.Element-ref commands (
click --element,snapshot, …) are untouched; they genuinely need the helper's accessibility snapshot.clickCountis now tracked (1 → 2 → 3 → 1 for repeated presses within 500ms / 2px) where the helper hardcodedclickCount: 1. Double-click through the remote pane — select a word, open a file from a list — was previously impossible to express at all.Why
Measured on the same host, same dev build,
mainvs this branch, through the exact RPCsBrowserPanesends.browser.evalon the same socket and queue is the control: it isolates dispatch cost from the fixed per-RPC floor, so these numbers don't depend on how fast the harness is.browser.eval(control)browser.mouseClick(already in process)browser.mouseMovebrowser.mouseWheelA pointer event now costs exactly what an eval costs — the dispatch itself adds ~0ms above the floor. Both ends serialize input (
enqueueRemoteInputon the client, the per-session command queue on the server), so the old per-event cost multiplied into every gesture. That was most of "the remote pane feels laggy".The test asserts this relatively (
mouseMove < evalControl × 2), not against this machine's milliseconds.On awaiting the dispatch
#15309 argued the CDP ack is frame-bound (~16ms on a watched tab, ~1s on an idle one) and therefore must not be awaited, and paid for that with deferred-failure machinery and an
ORCA_BROWSER_INPUT_AWAIT_ACKescape hatch. That premise is wrong: measured,mouseMoveawaited costs the same asbrowser.evalawaited. All of that machinery is gone. This awaits, exactly like #15310.Zero-Regression Evidence
tests/e2e/browser-pointer-input-dispatch.spec.tsrun againstmainsources and then branch sources, same host, same page:clickevents fireddblclickdetail: 2contextmenuon right-click"alpha bravo""alpha bravo"(identical)#scroller.scrollTopafter 6 notcheswindow.scrollYEverything
maindid, this does identically. Two thingsmaincould not do now work.Result shapes were checked by running the pinned agent-browser 0.27 binary directly —
{moved:true},{pressed:true},{released:true},{scrolled:true,deltaX,deltaY}, byte-identical, includingdeltaX: 0when the CLI passes nodx. No wire, schema or RPC surface change, so mixed-version pairs just get a faster server. Checked againstdocs/reference/remote-wire-compatibility.md: nothing there applies.Callouts
Deltas from the helper path, all deliberate:
dblclickwithdetail: 2, as a physical mouse would. The old path physically could not emit a sub-500ms pair (4 × ~170ms per click), so nothing existing can have depended on fast pairs stayingdetail: 1. Newly-fast CLI scripting that double-invokes a control in under 500ms will now look like a double-click to the page, because it is one.mainsix notches moved the scroller 0px and the page 0px — nothing scrolled at all. Both panes sendmouseMovebefore every wheel, so interactive scrolling gets strictly better.mouseUpwith no button releases the held button. The helper always releasedleft, which left a right-button press stuck forever. A chorded release keeps the still-held button addressable. Both panes always pass the button, so only CLI/RPC callers see this.browser_errornaming the bad argument. Verified against the 0.27 binary:mouse move NaN 20errored there too, butmouse wheel NaNsucceeded and silently scrolled 100px. That silent coercion is the one behavior deliberately not preserved.button: 'back'/'forward'pass through as real X1/X2 presses with the right mask bits, as the helper dispatched them. The element-click path's existing left-coercion is untouched.wc.focus()on mouseDown mirrors whatmouseClickalready does. Relative to the old down/up path this widens the focus surface the same way every mobile tap already does — flagging for coordination with fix(browser): restore host focus after synthetic mouse presses #10875/fix(browser): stop agent browser input from stealing the user's focus #11732, which this neither fixes nor worsens beyond the established precedent.build()mutates state before the event is on the wire; a rejected dispatch changed nothing in the page, so keeping the mutation would leave a phantom held button on every later event. Mutation-tested: deleting the one-line rollback fails exactly three tests.mouseMovebefore any down/up/wheel in the same serialized queue, which re-establishes position.Testing
pnpm test src/main/browser/plus the RPC, pane and CLI consumers of these methods: 3063 passed, run 5× to rule out flakes.agent-browser-bridge-pointer-input.test.ts, 23 tests: CDP payloads for move/down/up/wheel, position tracking through a drag, per-tab state isolation, clickCount escalation and all three reset conditions, back/forward passthrough, chorded release, non-finite rejection, dispatch-failure rollback (position, held button, click cadence),browser_tab_not_foundon a page that dies mid-dispatch, debugger attach/detach symmetry, queue cleanup. Three click-cadence tests are pinned to fake timers after one flaked under concurrent load.pnpm test: 81100 passed. The 9 failures are real-Claude-CLI, node-pty and fish/zsh shell-integration tests; verified pre-existing by running those same files againstmainsources and getting the identical failure.pnpm tc,oxlint,pnpm run check:code-quality:changed,pnpm run check:max-lines-ratchet— all clean, no new casting or type-assertion findings.mouseClickuses on all three platforms andkeypressuses since fix(browser): press keys through a US-layout CDP key table instead of a subprocess per keystroke #15310; no platform-conditional code added.AI Disclosure
Built with Claude Code. All numbers come from scripted probes against real instances including the
main-branch baseline on the same host and build; claims about the helper's current behavior were re-checked by running the pinned agent-browser 0.27 binary directly. Two claims inherited from #15309 did not survive that re-check and are corrected above: the frame-bound-ack rationale for not awaiting, and the assertion that the helper rejects non-finite wheel deltas.Notes
Security: no new parsed inputs beyond a finiteness check; RPC surface and schemas unchanged. Cross-platform: no platform-conditional code. Remote/SSH: no wire change, server-side speedup only. Performance: the hot path loses a subprocess spawn per event and gains a small per-tab state object.
Agent skill upstream boundary
Checklist
pnpm lint,pnpm typecheck,pnpm testpass locally