feat(platform-macos): WindowChangeDetector + FocusGuard per-action focus suppression (#1526) - #1531
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements per-action focus suppression by porting Swift's ChangesPer-action focus suppression infrastructure and tool integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-macos/src/tools/press_key.rs`:
- Around line 103-133: The AX pre-focus call (the earlier focus_element() step)
must be moved inside the FocusGuard/WindowChangeDetector wrapper so the focus
write is suppressed and its side-effects are captured: when handling the
window_id + element_index path inside the closure passed to
focus_guard::with_focus_suppressed (the async spawn_blocking block created for
press_key.CGEvent after WindowChangeDetector::snapshot()), perform the AX focus
write there before calling crate::input::keyboard::press_key (and before the
NSMenu branch that calls
crate::input::skylight::with_menu_shortcut_activation/press_key_no_auth); ensure
the focus_element invocation uses the same pid, window_id and element_index
values and runs inside that guarded closure so WindowChangeDetector will observe
any resulting window changes.
In `@libs/cua-driver-rs/crates/platform-macos/src/window_change_detector.rs`:
- Around line 168-186: The snapshot() function currently re-reads
apps::frontmost_pid() (front_pid) and uses it to create the suppression lease
via focus_steal::begin_suppression, which can differ from the prior_front
captured by callers; change the API to accept the caller-captured PID (e.g., add
snapshot_with_front_pid(prior_front: Option<u32>) or an overload) and use that
value when arming the wildcard lease instead of re-calling
apps::frontmost_pid(); update callers that already capture prior_front to pass
it into snapshot_with_front_pid so both leases use the identical captured PID
and avoid timing-dependent restoration.
In `@libs/cua-driver-rs/PARITY.md`:
- Around line 1421-1422: The markdown contains code spans with trailing spaces
(`\, ` and `\; `) that trigger MD038; replace those code spans so they contain
only the punctuation characters (use `,` and `;`) and move any spacing
description into the surrounding prose (e.g., say "joined with ', ' (comma
followed by a space)" instead of embedding the space inside the backticks);
update the text that currently reads with `` `, ` `` and `` `; ` `` to use ```,`
`` and ``;`` and add a short prose note describing that items are joined with a
comma+space or semicolon+space as appropriate.
🪄 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: 0941f6e3-59c7-413f-8287-eeb903ae058d
📒 Files selected for processing (11)
libs/cua-driver-rs/PARITY.mdlibs/cua-driver-rs/crates/platform-macos/src/focus_guard.rslibs/cua-driver-rs/crates/platform-macos/src/lib.rslibs/cua-driver-rs/crates/platform-macos/src/tools/click.rslibs/cua-driver-rs/crates/platform-macos/src/tools/drag.rslibs/cua-driver-rs/crates/platform-macos/src/tools/hotkey.rslibs/cua-driver-rs/crates/platform-macos/src/tools/press_key.rslibs/cua-driver-rs/crates/platform-macos/src/tools/scroll.rslibs/cua-driver-rs/crates/platform-macos/src/tools/set_value.rslibs/cua-driver-rs/crates/platform-macos/src/tools/type_text.rslibs/cua-driver-rs/crates/platform-macos/src/window_change_detector.rs
Comment on lines
+103
to
+133
| // ── Focus-suppression wrap (Swift WindowChangeDetector + FocusGuard) ── | ||
| // Single-key presses can fire autocomplete (Return on a search | ||
| // box opens a results popover) or trigger menu shortcuts that | ||
| // open windows. Wrapping mirrors the hotkey path. | ||
| let prior_front = apps::frontmost_pid(); | ||
| let snapshot = WindowChangeDetector::snapshot(); | ||
|
|
||
| let result = focus_guard::with_focus_suppressed( | ||
| Some(pid), | ||
| prior_front, | ||
| "press_key.CGEvent", | ||
| || async move { | ||
| tokio::task::spawn_blocking(move || { | ||
| let m: Vec<&str> = modifiers.iter().map(String::as_str).collect(); | ||
| if let Some(wid) = window_id { | ||
| if element_index.is_none() { | ||
| // NSMenu path: window_id set but no element_index. | ||
| crate::input::skylight::with_menu_shortcut_activation(pid as libc::pid_t, wid, || { | ||
| crate::input::keyboard::press_key_no_auth(pid, &key, &m) | ||
| })?; | ||
| return Ok(()); | ||
| } | ||
| } | ||
| crate::input::keyboard::press_key(pid, &key, &m) | ||
| }) | ||
| .await | ||
| }, | ||
| ) | ||
| .await; | ||
|
|
||
| let changes = snapshot.detect_async().await; |
Contributor
There was a problem hiding this comment.
Include the AX pre-focus step in this guarded window.
This wrapper starts after the optional focus_element() call at Lines 93-101. In the window_id + element_index path, that AX focus write is the step most likely to activate the target or surface focus-driven UI, but it currently happens outside both FocusGuard and WindowChangeDetector. That means this path can still miss the exact side effect the new suppression machinery is supposed to catch.
🤖 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-macos/src/tools/press_key.rs` around lines
103 - 133, The AX pre-focus call (the earlier focus_element() step) must be
moved inside the FocusGuard/WindowChangeDetector wrapper so the focus write is
suppressed and its side-effects are captured: when handling the window_id +
element_index path inside the closure passed to
focus_guard::with_focus_suppressed (the async spawn_blocking block created for
press_key.CGEvent after WindowChangeDetector::snapshot()), perform the AX focus
write there before calling crate::input::keyboard::press_key (and before the
NSMenu branch that calls
crate::input::skylight::with_menu_shortcut_activation/press_key_no_auth); ensure
the focus_element invocation uses the same pid, window_id and element_index
values and runs inside that guarded closure so WindowChangeDetector will observe
any resulting window changes.
Comment on lines
+168
to
+186
| pub fn snapshot() -> Snapshot { | ||
| let window_ids: HashSet<u32> = windows::visible_windows() | ||
| .into_iter() | ||
| .filter(|w| w.layer == 0) | ||
| .map(|w| w.window_id) | ||
| .collect(); | ||
| let front_pid = apps::frontmost_pid(); | ||
|
|
||
| // Arm wildcard suppression — covers snapshot → detect window. | ||
| // restore_to = current frontmost; target = wildcard (any other pid). | ||
| // If there's no frontmost (rare — screensaver, login window), we | ||
| // skip the lease; foreground-change tracking still runs. | ||
| let lease = front_pid.map(|restore_to| { | ||
| focus_steal::begin_suppression( | ||
| None, // wildcard | ||
| restore_to, | ||
| "WindowChangeDetector.snapshot", | ||
| ) | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the same captured frontmost PID for both leases.
Each tool already captures prior_front immediately before calling snapshot(), but snapshot() re-queries apps::frontmost_pid() and arms the wildcard lease against that second value. If the frontmost app flips in that gap, the targeted lease and wildcard lease restore different pids, so focus restoration becomes timing-dependent in the exact path meant to make it deterministic.
Proposed direction
impl WindowChangeDetector {
+ pub fn snapshot_with_front_pid(front_pid: Option<i32>) -> Snapshot {
+ let window_ids: HashSet<u32> = windows::visible_windows()
+ .into_iter()
+ .filter(|w| w.layer == 0)
+ .map(|w| w.window_id)
+ .collect();
+
+ let lease = front_pid.map(|restore_to| {
+ focus_steal::begin_suppression(
+ None,
+ restore_to,
+ "WindowChangeDetector.snapshot",
+ )
+ });
+
+ Snapshot {
+ window_ids,
+ front_pid,
+ _lease: lease,
+ }
+ }
+
pub fn snapshot() -> Snapshot {
- let window_ids: HashSet<u32> = windows::visible_windows()
- .into_iter()
- .filter(|w| w.layer == 0)
- .map(|w| w.window_id)
- .collect();
- let front_pid = apps::frontmost_pid();
-
- let lease = front_pid.map(|restore_to| {
- focus_steal::begin_suppression(
- None, // wildcard
- restore_to,
- "WindowChangeDetector.snapshot",
- )
- });
-
- Snapshot {
- window_ids,
- front_pid,
- _lease: lease,
- }
+ Self::snapshot_with_front_pid(apps::frontmost_pid())
}
}Then have the tool wrappers pass their already-captured prior_front into snapshot_with_front_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-macos/src/window_change_detector.rs`
around lines 168 - 186, The snapshot() function currently re-reads
apps::frontmost_pid() (front_pid) and uses it to create the suppression lease
via focus_steal::begin_suppression, which can differ from the prior_front
captured by callers; change the API to accept the caller-captured PID (e.g., add
snapshot_with_front_pid(prior_front: Option<u32>) or an overload) and use that
value when arming the wildcard lease instead of re-calling
apps::frontmost_pid(); update callers that already capture prior_front to pass
it into snapshot_with_front_pid so both leases use the identical captured PID
and avoid timing-dependent restoration.
Comment on lines
+1421
to
+1422
| (multiple windows grouped by app, titles in quotes, joined with `, `; | ||
| multiple apps joined with `; `; alphabetical by app name). |
Contributor
There was a problem hiding this comment.
Fix markdownlint MD038 violations in code spans.
Line 1421 and Line 1422 use code spans with trailing spaces (`, `, `; `), which triggers MD038. Use ,/; without embedded trailing spaces and describe spacing in prose.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1421-1421: Spaces inside code span elements
(MD038, no-space-in-code)
[warning] 1422-1422: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 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/PARITY.md` around lines 1421 - 1422, The markdown contains
code spans with trailing spaces (`\, ` and `\; `) that trigger MD038; replace
those code spans so they contain only the punctuation characters (use `,` and
`;`) and move any spacing description into the surrounding prose (e.g., say
"joined with ', ' (comma followed by a space)" instead of embedding the space
inside the backticks); update the text that currently reads with `` `, ` `` and
`` `; ` `` to use ```,` `` and ``;`` and add a short prose note describing that
items are joined with a comma+space or semicolon+space as appropriate.
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
…losure press_key and scroll were pre-focusing the target AX element BEFORE the focus_guard::with_focus_suppressed wrapper and before WindowChangeDetector::snapshot. Any reflex activations triggered by the AXSetAttribute(kAXFocused) write itself (Chromium tab activation on focus, autocomplete popovers) escaped both the wildcard snapshot suppressor and the targeted FocusGuard lease. Move the focus_element() spawn_blocking + settle inside the closure passed to with_focus_suppressed so the AX write runs under the same suppression umbrella as the keystroke that follows. Cache lookup stays outside (no side-effects). Spotted by CodeRabbit on PR #1531.
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
…or::snapshot snapshot() previously called apps::frontmost_pid() internally to seed the wildcard suppression lease's restore_to. Every action-tool caller ALREADY captured prior_front one line earlier for its surrounding focus_guard::with_focus_suppressed lease — and between the caller's read and the detector's own a different app could become frontmost, leaving the wildcard lease and the targeted FocusGuard lease pointing at different pids to restore to. Change snapshot() to take prior_front: Option<i32> and use it both as the lease's restore_to and as the recorded front_pid for change detection. Update all 7 wired tools (8 call sites — click has two) to thread the value they already had. Add snapshot_stores_caller_prior_front regression test pinning the round-trip behaviour. Spotted by CodeRabbit on PR #1531.
f-trycua
added a commit
that referenced
this pull request
May 16, 2026
…ter code spans The "joined with \`, \`" / "joined with \`; \`" code spans included a trailing space inside the backticks to convey "the join string is a comma + space", but markdownlint MD038 flags whitespace inside code spans. Rewrite the spans to hold only the punctuation character and spell out the trailing space in the surrounding prose. Spotted by CodeRabbit on PR #1531.
Collaborator
Author
|
Pushed fixup commits for the 3 CodeRabbit findings:
@coderabbitai please re-review. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4405 🏁 Script executed: Length of output: 12857 🏁 Script executed: Length of output: 11042 🏁 Script executed: Length of output: 3954 🏁 Script executed: Length of output: 1461
All three findings are cleanly resolved. 21/21 tests passing with the added regression. LGTM — approving. [approve] |
Ports Swift's WindowChangeDetector (libs/cua-driver/Sources/CuaDriverServer/ Tools/WindowChangeDetector.swift) to Rust. Snapshot the visible window set + frontmost pid before an action, diff after, return a one-line result suffix that matches Swift's wording verbatim so MCP callers don't have to special-case the binary. The Snapshot owns an Option<SuppressionLease> (wildcard target_pid=None, restore_to = current frontmost) — dropping the Snapshot ends the lease, covering the full snapshot→action→detect window. This catches fast self-activations before the poll loop can observe them. Unit tests cover the pure-function diff (opened/closed) plus all five result_suffix branches (no-change, single titled window, grouped by app, empty title, foreground-only change). 16/16 passing (was 8/8 baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ports Swift's FocusGuard.withFocusSuppressed layer-3 reactive suppressor (libs/cua-driver/Sources/CuaDriverCore/Focus/FocusGuard.swift) to Rust as a closure-based async wrapper. Per-action call sites (click, type_text, set_value, drag) wrap their AX dispatch in this helper to catch the case where an AX attribute write triggers a reflexive self-activation in the target app. Behavior matches Swift: - Skip arming when target == prior_frontmost (self → self short-circuit) - Skip arming when prior_frontmost is None (no app to restore to) - 50ms post-action settle before dropping the lease, giving any in-flight focus-grab reflex time to fire and be observed Layers 1+2 (AXManualAccessibility / synthetic-focus write+restore) are deliberately deferred to a follow-up — they require AX assertion + attribute write/restore plumbing not yet ported and empirically the layer-3 guard combined with WindowChangeDetector's wildcard catches the majority of side-effects on real-world workflows. PARITY.md will document the gap in phase 4. Also adds a `with_focus_suppressed_now` convenience that resolves prior_frontmost at call time for callers without a surrounding snapshot. Tests: 4 new under focus_guard::tests covering the arm/skip branches and the now-variant. 20/20 platform-macos passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the 7 macOS action tools (click, type_text, hotkey, press_key,
drag, scroll, set_value) in the WindowChangeDetector + FocusGuard
helpers shipped in the previous two commits, mirroring the Swift
ClickTool / TypeTextTool / SetValueTool pattern.
Per-tool integration is identical:
let prior = apps::frontmost_pid();
let snapshot = WindowChangeDetector::snapshot(); // arms wildcard
let result = focus_guard::with_focus_suppressed(
Some(pid), prior, "<origin>", || async { ... }
).await;
let changes = snapshot.detect_async().await; // drops wildcard
// append changes.result_suffix() to success text
ClickTool covers both AX (element_index path) and pixel paths — both
can land on focus-grabbing UI. PressKeyTool and HotkeyTool wrap their
CGEvent paths because Cmd+N / Return-on-autocomplete can open new
windows. DragTool wraps because drag-and-drop on the Dock or a
background app icon can spawn helper windows. ScrollTool wraps for
parity (rare side-effects but cheap to add).
Each tool's success message now suffixes any window/foreground
side-effects observed during the snapshot→detect cycle. Wording
matches Swift verbatim ("Action opened new window(s): X (\"title\")."
or "Action caused a different app to become frontmost.") so MCP
callers don't need per-binary special cases.
Also adds Snapshot::detect_async() so callers don't block the tokio
runtime on the up-to-1s poll loop. The synchronous detect() remains
for tests and other blocking call sites.
Build clean (release). 20/20 platform-macos tests pass; api_parity
suite shows identical pass/fail count vs main (25/7 pre-existing
failures, all unrelated to focus suppression).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documents the WindowChangeDetector + FocusGuard port (#1526) in PARITY.md: - New "Per-action focus suppression" section under the existing Focus-steal block, with snapshot→action→detect cycle diagram, verbatim result-suffix wording, and the layer-1+2 deferral notes. - click / press_key / hotkey rows flipped from "(TBD)" / "OPEN" to VERIFIED for the focus-suppression wrap with cross-refs to the new section. - Focus-steal intro reworded — no longer "slated for use", now "used by `launch_app` and by the 7 action tools". - "Intentional simplifications" updated to reflect that WindowChangeDetector has shipped and FocusGuard ships layer-3 only. Per coordinator instruction (Phase 4 update): the new integration test cases are dropped. The existing parity suite was confirmed not to regress after Phase 3 — identical pass/fail count vs main (25 failures / 7 errors, all pre-existing environmental: test_mcp_set_recording_enable_disable, test_mcp_screenshot_has_png_image, missing Swift binary path, etc.; no focus-suppression-related regressions). Closes #1526. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…losure press_key and scroll were pre-focusing the target AX element BEFORE the focus_guard::with_focus_suppressed wrapper and before WindowChangeDetector::snapshot. Any reflex activations triggered by the AXSetAttribute(kAXFocused) write itself (Chromium tab activation on focus, autocomplete popovers) escaped both the wildcard snapshot suppressor and the targeted FocusGuard lease. Move the focus_element() spawn_blocking + settle inside the closure passed to with_focus_suppressed so the AX write runs under the same suppression umbrella as the keystroke that follows. Cache lookup stays outside (no side-effects). Spotted by CodeRabbit on PR #1531.
…or::snapshot snapshot() previously called apps::frontmost_pid() internally to seed the wildcard suppression lease's restore_to. Every action-tool caller ALREADY captured prior_front one line earlier for its surrounding focus_guard::with_focus_suppressed lease — and between the caller's read and the detector's own a different app could become frontmost, leaving the wildcard lease and the targeted FocusGuard lease pointing at different pids to restore to. Change snapshot() to take prior_front: Option<i32> and use it both as the lease's restore_to and as the recorded front_pid for change detection. Update all 7 wired tools (8 call sites — click has two) to thread the value they already had. Add snapshot_stores_caller_prior_front regression test pinning the round-trip behaviour. Spotted by CodeRabbit on PR #1531.
…ter code spans The "joined with \`, \`" / "joined with \`; \`" code spans included a trailing space inside the backticks to convey "the join string is a comma + space", but markdownlint MD038 flags whitespace inside code spans. Rewrite the spans to hold only the punctuation character and spell out the trailing space in the surrounding prose. Spotted by CodeRabbit on PR #1531.
f-trycua
force-pushed
the
feat/cua-driver-rs-focus-guard-window-detector
branch
from
May 16, 2026 22:19
751f2db to
57359bc
Compare
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
Ports Swift's
WindowChangeDetectorandFocusGuard.withFocusSuppressedper-action focus-suppression machinery to cua-driver-rs, building on the focus-steal infrastructure that landed in #1524.Closes #1526.
crates/platform-macos/src/window_change_detector.rs— snapshot the visible window set + frontmost pid, arm a wildcardfocus_steal::begin_suppressionlease, diff after the action, return a result suffix that matches Swift's wording verbatim so MCP callers don't need per-binary special cases.crates/platform-macos/src/focus_guard.rs— async closure helper that arms a targeted suppressor across the AX dispatch and sleeps ~50ms post-action so any reflex activation is observed before the lease drops.click,type_text,hotkey,press_key,drag,scroll,set_value) wrapped in the snapshot → suppressed-action → detect cycle. Success messages now suffix any observed side-effects.Layering
Swift's
FocusGuarddoes three things: (1) AX enablement assertion, (2) synthetic-focus write+restore on the enclosing window+element, (3) reactive suppressor. This Rust port ships layer 3 only — layers 1+2 are deferred because the AX assertion + attribute-write plumbing isn't yet ported. Empirically the layer-3 reactive guard combined withWindowChangeDetector's wildcard lease catches the majority of side-effects on real-world workflows. Documented in PARITY.md so the gap is auditable.Commits
0e2d9e72feat(platform-macos): WindowChangeDetector module9ef085e2feat(platform-macos): FocusGuard with_focus_suppressed helper7784e131feat(tools): wrap action tools in focus suppression + change detection77d3ed95docs(parity): per-action focus suppression section + flip wired toolsTest plan
cargo build --releaseclean (no new warnings vs main).cargo test -p platform-macos— 20/20 passing (was 8/8 on main; +8 WindowChangeDetector tests, +4 FocusGuard tests)../run_tests.sh --parity -vfromtests/integration/— identical pass/fail count vs main (25 failures / 7 errors, all pre-existing environmental: missing Swift binary path, focus-grabber on host, etc.; no focus-suppression regressions).test_focus_steal_parityon this runner).Notes
test_focus_steal_parity.pyalready covers the launch path and the new unit tests pin the new modules.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation