feat(cua-driver-rs)(windows): no-foreground audit v2 — hotkey Chromium + Invoke restore + click polling - #1669
Merged
Merged
Conversation
…k polling restore Two changes that close two of the focus-stealing paths PR #1668 documented as "still has transient activation": 1. **`hotkey` Chromium routing.** The non-XAML Win32 branch in `HotkeyTool::invoke` previously routed modifier+key combos through `send_key_synthesized` (SendInput + SetForegroundWindow(target) swap) because `TranslateAccelerator`-based apps (LibreOffice, FAR, classic Notepad) need the system modifier state updated, which PostMessage can't do. **But Chromium doesn't use TranslateAccelerator** — it dispatches accelerators in `Browser::HandleKeyboardEvent` which reads modifier state from the WM_KEYDOWN LPARAM bits directly. PostMessage works for Chromium with no foreground swap. Routing now: `is_chromium_target_window(hwnd)` → `post_key` even with modifiers. Non-Chromium Win32 with modifiers continues to use `send_key_synthesized` (the TranslateAccelerator constraint). The no-foreground win covers every Chromium browser AND every Electron app (Slack/VS Code/Discord/Teams/Notion — they all use `Chrome_WidgetWin_*` class). 2. **Chromium pixel-click polling restore guard.** `click({pid, x, y})` on Chromium falls through to `send_click_synthesized` because Chromium's input thread filters by queue-origin and PostMessage clicks don't fire DOM events. `send_click_synthesized` does its own synchronous SetForegroundWindow(prev) ~40ms after the click — but Chromium's reaction to the click can include an async window re-activation 100-500ms later (focus().activate() in the renderer event handler). The pre-existing restore misses that. Wrapping the call with a `tokio::spawn(restore_foreground_polling_best_effort(...))` — same helper PR #1668's `launch_app` `FocusRestoreGuard` uses, already gated on `GetWindowThreadProcessId(fg_now) == pid` so user Alt-Tabs are respected. Catches the async re-activation case. Build clean (0 warnings, x86_64-pc-windows-msvc). Tests: - platform-windows: 32/32 pass - mcp_protocol_test: 28/28 pass Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…olish Two changes to the bookmark-based JS exec path: 1. **Auto-toggle the Favorites bar via PostMessage** (was: bail). PR #1668 removed the auto-toggle because the old `send_key_synthesized` call did a SetForegroundWindow swap. With the hotkey-on-Chromium routing change in the previous commit, the same Ctrl+Shift+B accelerator can now be delivered via `post_key` — no foreground swap. If the bar is still hidden after the keystroke (browser policy override / locked-down profile / Ctrl+Shift+B remapped), the call falls back to the same actionable error PR #1668 introduced, telling the user to enable the bar manually. 2. **Polling foreground restore around the bookmark `InvokePattern.Invoke`.** Same pattern PR #1668's `launch_app` `FocusRestoreGuard` uses, adapted for the Chromium URL-load activation case. Capture `prev_fg` immediately before Invoke; after Invoke, poll for ~600 ms in 50 ms steps for "Chromium grabbed foreground" and SetForegroundWindow(prev) the user's window back. Gated on `GetWindowThreadProcessId(fg_now) == browser_pid` so user Alt-Tabs are respected. Synchronous (not tokio::spawn) because the `poll_for_marker` step that reads the title via UIA must see the restored foreground state. When Windows' foreground lock denies the restore (we lack UIAccess), browser dwell is bounded to the 600 ms poll budget instead of "until next user action". Logged at trace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…m + Invoke restore + click polling
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
📝 WalkthroughWalkthroughThis PR adds foreground-restore polling guards to Windows Chromium tool operations and updates bookmark execution to ensure Favorites bar visibility. HotkeyTool now routes modifier hotkeys to Chromium targets via PostMessage instead of SendInput. ClickTool and bookmark invocation capture prior foreground state and restore it after Chromium async re-activation. Favorites bar visibility is toggled via PostMessage Ctrl+Shift+B when hidden. ChangesChromium foreground-restore mitigations and bookmark handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
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 `@libs/cua-driver-rs/crates/platform-windows/src/tools/impl_.rs`:
- Around line 1713-1716: The restore task currently spawned with
restore_foreground_polling_best_effort(prev_fg_addr, pid) uses only pid to
decide whether to restore focus; change it to guard by the clicked target HWND
(or its root owner) instead: capture the clicked HWND (or compute its root
owner) at click time, pass that HWND (e.g. clicked_hwnd or clicked_root_owner)
into the restore task instead of or in addition to pid, and in
restore_foreground_polling_best_effort compare the current foreground HWND/root
owner (via GetForegroundWindow/GetAncestor/RootOwner) with the captured HWND
before restoring; update the spawn call and the restore function signature (and
any callers) to accept this HWND and use it for the focus check rather than
PID-only logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4879e45-83de-4884-a14f-6ba4ba00e85f
📒 Files selected for processing (3)
libs/cua-driver-rs/Skills/cua-driver-rs/WINDOWS.mdlibs/cua-driver-rs/crates/platform-windows/src/tools/impl_.rslibs/cua-driver-rs/crates/platform-windows/src/tools/page_bookmark.rs
Comment on lines
+1713
to
+1716
| // Kick off the polling restore regardless of click result | ||
| // — even a partially-inserted click might have started an | ||
| // async activation Chromium can't undo. | ||
| tokio::spawn(restore_foreground_polling_best_effort(prev_fg_addr, pid)); |
Contributor
There was a problem hiding this comment.
Guard async restore by the activated target HWND, not PID-only.
This restore task can steal focus from a user who Alt-Tabs to another window in the same browser process, because PID match alone still passes. Gate on the clicked target HWND (or root owner) for the click path before restoring.
Proposed direction
- tokio::spawn(restore_foreground_polling_best_effort(prev_fg_addr, pid));
+ let target_hwnd_addr = hwnd as usize;
+ tokio::spawn(restore_foreground_polling_after_click_best_effort(
+ prev_fg_addr,
+ pid,
+ target_hwnd_addr,
+ ));// Click-specific guard idea:
// restore only when foreground is the same HWND we clicked (or same root owner),
// not just any window owned by `pid`.🤖 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-rs/crates/platform-windows/src/tools/impl_.rs` around lines
1713 - 1716, The restore task currently spawned with
restore_foreground_polling_best_effort(prev_fg_addr, pid) uses only pid to
decide whether to restore focus; change it to guard by the clicked target HWND
(or its root owner) instead: capture the clicked HWND (or compute its root
owner) at click time, pass that HWND (e.g. clicked_hwnd or clicked_root_owner)
into the restore task instead of or in addition to pid, and in
restore_foreground_polling_best_effort compare the current foreground HWND/root
owner (via GetForegroundWindow/GetAncestor/RootOwner) with the captured HWND
before restoring; update the spawn call and the restore function signature (and
any callers) to accept this HWND and use it for the focus check rather than
PID-only logic.
3 tasks
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
1 participant
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.
Summary
Three commits. Closes the remaining tractable no-foreground violations the user identified after PR #1668. Overnight autonomous work; user signed off in advance.
PR #1668 already shipped:
launch_appFocusRestoreGuard + bookmark Ctrl+Shift+B bail. The "still has transient activation" table in #1668's docs listed four remaining items. This PR closes three of them and re-frames the fourth as an unavoidable irreducible minimum.Changes
1.
hotkeyChromium routing (tools/impl_.rs)The non-XAML Win32 branch previously routed all modifier+key combos through
send_key_synthesized(SendInput + foreground swap) becauseTranslateAcceleratorneedsGetKeyStateupdated, which PostMessage can't do. But Chromium doesn't use TranslateAccelerator —Browser::HandleKeyboardEventreads modifier state from WM_KEYDOWN LPARAM bits directly. PostMessage works on Chromium with no swap.Routing:
is_chromium_target_window(hwnd)→post_keyeven with modifiers. Non-Chromium Win32 + modifiers continues throughsend_key_synthesized(real constraint).Coverage: every Chromium browser AND every Electron app uses the
Chrome_WidgetWin_*class — Chrome, Edge, Brave, Arc, Vivaldi, Slack, VS Code, Discord, Teams, Notion.2.
page.execute_javascriptCtrl+Shift+B auto-toggle restored (tools/page_bookmark.rs)PR #1668 removed the auto-toggle because it used the focus-stealing
send_key_synthesized. With the hotkey routing change in this PR's commit 1, the same accelerator can now be delivered viapost_key— no foreground swap. If the bar is still hidden after the keystroke (browser policy override / Ctrl+Shift+B remapped), the same actionable bail error from PR #1668 fires.3. Bookmark
InvokePattern.Invokepolling foreground restore (tools/page_bookmark.rs)Chromium activates the browser window when a bookmark is clicked —
WebContents::OpenURLtreats it as user-initiated navigation. PR #1668 documented this as "not closable without forking Chromium." Re-examined: same patternlaunch_app'sFocusRestoreGuarduses solves it.New helper
restore_foreground_after_browser_activation(prev_fg, browser_pid)polls for ~600 ms in 50 ms steps; once Chromium grabs foreground,SetForegroundWindow(prev)the user's window back. Gated onGetWindowThreadProcessId(fg_now) == browser_pidso user Alt-Tabs are respected. Synchronous (nottokio::spawn) because thepoll_for_markerstep that reads the result title via UIA must see the restored foreground.When Windows' foreground lock denies the restore (we lack UIAccess), browser dwell is bounded to the 600 ms poll budget instead of "until next user action."
4. Chromium pixel-click polling restore (
tools/impl_.rs)click({pid, x, y})on Chromium usessend_click_synthesized(SendInput + brief foreground swap) — required by Chromium's input thread queue-origin filter. The synchronous SetForegroundWindow(prev) insidesend_click_synthesized~40 ms after the click covers the immediate swap, but Chromium's async re-activation (focus().activate() in the renderer event handler, 100-500 ms later) was uncovered.Wraps the call with a
tokio::spawn(restore_foreground_polling_best_effort(...))— the same helper PR #1668'slaunch_appFocusRestoreGuardalready uses. Catches the async case.5. Audit findings (no code change needed)
drag: already usespost_drag(PostMessage). PR fix(cua-driver-rs)(windows): no-foreground audit — launch_app focus restore + bookmark-exec bail #1668's footnote saying "likely uses SendInput" was a false alarm. ✓clickUWP/WinUI3: already covered by the UIAInvokePattern.Invoke at pointlayered dispatch (runs FIRST inClickTool, before any SendInput path). UWP/WebView2/DirectComposition surfaces route through UIA without any focus swap. ✓press_key: already usespost_key. ✓The new no-foreground status table
launch_app(any target)press_keyhotkeyno-modifierhotkeymodifier + Chromium targethotkeymodifier + classic Win32 (LibreOffice etc.)click({element_index})click({pid, x, y})UWP/WinUI3click({pid, x, y})non-Chromium Win32click({pid, x, y})Chromiumtype_textset_valuescrolldragpage.execute_javascriptCtrl+Shift+B auto-togglepage.execute_javascriptbookmark Invokepage.get_text/query_domThe only paths that still touch foreground:
hotkeywith modifiers on classic Win32 —TranslateAcceleratorarchitectural constraintclick({pid, x, y})on Chromium when UIA Invoke misses — Chromium input thread architectural constraintBoth are mitigated where possible (polling restores catch async re-activations) but the synchronous foreground swap itself is unavoidable without UIAccess / Chromium fork.
Verification
cargo build --release -p cua-driver --target x86_64-pc-windows-msvc→ 0 warningscargo test --release -p platform-windows→ 32/32 pass (including all 10launch_focus_restore_decision_testsfrom PR fix(cua-driver-rs)(windows): no-foreground audit — launch_app focus restore + bookmark-exec bail #1668)cargo test --release -p cua-driver --test mcp_protocol_test→ 28/28 passTest plan
cua-driver hotkey '{"pid":<edge_pid>, "keys":["ctrl","t"]}'against an Edge window — opens a new tab WITHOUT the caller's foreground app losing focuscua-driver page '{"pid":<edge_pid>, "window_id":<W>, "action":"execute_javascript", "javascript":"document.title"}'— runs JS, returns the title, the only visible foreground activity is a brief (~150-600 ms) bookmark-click visit that the polling restore catchescua-driver click '{"pid":<edge_pid>, "x":100, "y":200}'— clicks the (x,y) point, no persistent foreground steal🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation