fix(cua-driver/windows): glide session cursor for scroll#2103
fix(cua-driver/windows): glide session cursor for scroll#2103outdog-hwh wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@outdog-hwh is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a ChangesWindows Scroll Cursor Visualization
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant ScrollTool
participant windowscreencenter as window_screen_center
participant Overlay
Agent->>ScrollTool: invoke(scroll params)
ScrollTool->>ScrollTool: resolve cursor_key
alt windowless desktop scroll (x,y)
ScrollTool->>Overlay: glide cursor to (sx, sy)
ScrollTool->>Overlay: emit ClickPulse at (sx, sy)
else window-scoped scroll (pid/window_id)
ScrollTool->>windowscreencenter: compute center for hwnd
windowscreencenter-->>ScrollTool: Some((x, y)) or None
ScrollTool->>Overlay: pin to hwnd, glide to center
ScrollTool->>Overlay: emit ClickPulse at center
end
ScrollTool->>ScrollTool: synthesize wheel event
Possibly related PRs
Poem
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs (1)
4147-4169: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant window enumeration for center resolution.
When
hwnd_optisNone,list_windows(Some(pid))is already fetched at Line 4150 to pick the first window. Then Line 4158-4160 unconditionally spawns a second blocking task that re-enumerates windows (EnumWindows+ UIA perwin32::windows::list_windows) just to compute the center — doubling window-enumeration cost on that path.Worse, this second enumeration (and its
spawn_blockingoverhead) runs unconditionally for every scroll call — even for the defaultPostMessage(WM_VSCROLL/HSCROLL) delivery path wherecenteris never read, and even when the overlay/session cursor is disabled (cursor_key == NO_CURSOR).spawn_blockingoccupies a thread from the runtime's dedicated blocking pool for the call duration, so doing this unconditionally on a plausibly hot input path (scroll) adds avoidable overhead.Consider computing the window list once, reusing it both for hwnd auto-resolution and center lookup, and deferring/skipping the center computation when it isn't needed (overlay disabled and delivery != Foreground).
♻️ Sketch: reuse a single window list fetch
- let hwnd = match hwnd_opt { - Some(h) => h, - None => { - let windows = tokio::task::spawn_blocking(move || crate::win32::list_windows(Some(pid))).await.unwrap_or_default(); - match windows.first() { - Some(w) => w.hwnd, - None => return ToolResult::error(format!("No windows found for pid {pid}. Provide window_id.")), - } - } - }; - - let center = tokio::task::spawn_blocking(move || { - window_screen_center(&crate::win32::list_windows(Some(pid)), hwnd) - }).await.ok().flatten(); + let (hwnd, windows) = match hwnd_opt { + Some(h) => (h, tokio::task::spawn_blocking(move || crate::win32::list_windows(Some(pid))).await.unwrap_or_default()), + None => { + let windows = tokio::task::spawn_blocking(move || crate::win32::list_windows(Some(pid))).await.unwrap_or_default(); + match windows.first() { + Some(w) => { let h = w.hwnd; (h, windows) } + None => return ToolResult::error(format!("No windows found for pid {pid}. Provide window_id.")), + } + } + }; + let center = window_screen_center(&windows, hwnd);Also applies to: 4199-4206
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs` around lines 4147 - 4169, The scroll path is redundantly enumerating windows and spawning blocking work even when the center is unused. In the logic that resolves hwnd and computes center, reuse the same window list fetched via crate::win32::list_windows(Some(pid)) for both hwnd auto-selection and window_screen_center instead of calling it again inside the second tokio::task::spawn_blocking. Also gate the center/overlay work so it only runs when needed (for example when cursor_key is not NO_CURSOR and the delivery path actually uses the overlay), keeping pin_overlay_above and overlay_glide_to off the hot default PostMessage path.
🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs`:
- Around line 4147-4169: The scroll path is redundantly enumerating windows and
spawning blocking work even when the center is unused. In the logic that
resolves hwnd and computes center, reuse the same window list fetched via
crate::win32::list_windows(Some(pid)) for both hwnd auto-selection and
window_screen_center instead of calling it again inside the second
tokio::task::spawn_blocking. Also gate the center/overlay work so it only runs
when needed (for example when cursor_key is not NO_CURSOR and the delivery path
actually uses the overlay), keeping pin_overlay_above and overlay_glide_to off
the hot default PostMessage path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af69f71e-cd46-41a6-9923-ccd9494631ae
📒 Files selected for processing (1)
libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
1 participant
Summary
scrollthrough the session cursor overlay for both desktopx,ywheel injection and pid/window-targeted scrollswindow_screen_centerhelper so overlay targeting and foreground wheel targeting resolve the same center pointTesting
Closes #2080.
Summary by CodeRabbit