Fix focus on target window#162
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
@chid is attempting to deploy a commit to the Pablo Varela's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis pull request refactors WindowManager.focus(window:) into an explicit Accessibility and app-activation sequence: it derives the owning NSRunningApplication, constructs an AX application element for that PID, raises the target window, sets AX attributes to mark the app/window as main/frontmost/focused, and finally activates the application with activateIgnoringOtherApps. The public signature is unchanged. Sequence Diagram(s)sequenceDiagram
participant WindowManager
participant AXAppElement
participant AXWindow
participant NSRunningApplication
WindowManager->>AXAppElement: init with window PID
WindowManager->>AXWindow: raise(window)
WindowManager->>AXAppElement: set kAXMainWindowAttribute & kAXFocusedWindowAttribute
WindowManager->>AXWindow: set kAXMainAttribute & kAXFocusedAttribute
WindowManager->>AXAppElement: set kAXFrontmostAttribute
WindowManager->>NSRunningApplication: activateIgnoringOtherApps()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
This resolves #160 (tested on 26.5 (25F71)) |
![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 `@Swift` Shift/src/Manager/WindowManager.swift:
- Around line 76-88: WindowManager.focus(window:) currently ignores failures
from app.activate, AXUIElementSetAttributeValue and AXUIElementPerformAction and
always returns Void; change it to return a Bool (or Result) that aggregates the
AXError/OSStatus results (treat any non-success as failure) by checking and
combining the return values of app.activate, AXUIElementSetAttributeValue and
AXUIElementPerformAction inside focus(window:), and propagate failure to the
caller; then update MouseTracker (where trackedWindowIsFocused is set after
calling WindowManager.focus) to only set trackedWindowIsFocused = true when
focus(window:) reports success. Include references to
AXUIElementCreateApplication, kAXFrontmostAttribute, kAXMainWindowAttribute,
kAXFocusedWindowAttribute, kAXMainAttribute and kAXFocusedAttribute when
collecting results so each AX call’s status is checked and affects the returned
success value.
🪄 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: 66cf7092-9da8-4fb5-bd4c-e61c4db8b69a
📒 Files selected for processing (1)
Swift Shift/src/Manager/WindowManager.swift
| static func focus(window: AXUIElement) { | ||
| guard let app = getNSApplication(from: window) else { return } | ||
| let axApp = AXUIElementCreateApplication(app.processIdentifier) | ||
| let trueValue = kCFBooleanTrue! | ||
|
|
||
| app.activate(options: [.activateIgnoringOtherApps]) | ||
| AXUIElementSetAttributeValue(axApp, kAXFrontmostAttribute as CFString, trueValue) | ||
| AXUIElementPerformAction(window, kAXRaiseAction as CFString) | ||
| AXUIElementSetAttributeValue(axApp, kAXMainWindowAttribute as CFString, window) | ||
| AXUIElementSetAttributeValue(axApp, kAXFocusedWindowAttribute as CFString, window) | ||
| AXUIElementSetAttributeValue(window, kAXMainAttribute as CFString, trueValue) | ||
| AXUIElementSetAttributeValue(window, kAXFocusedAttribute as CFString, trueValue) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the current caller contract before changing the return type.
sed -n '76,88p' 'Swift Shift/src/Manager/WindowManager.swift'
sed -n '94,100p' 'Swift Shift/src/Manager/MouseTracker.swift'
rg -n -C2 --type=swift '\bWindowManager\.focus\(window:' .Repository: pablopunk/SwiftShift
Length of output: 2174
Propagate AX focus failures to the caller (don’t mark focused unconditionally).
WindowManager.focus(window:) returns Void and ignores the results of AXUIElementPerformAction / AXUIElementSetAttributeValue (plus app.activate), while MouseTracker sets trackedWindowIsFocused = true immediately after the call (MouseTracker.swift:97). If any AX step fails, that flag blocks future focus retries while the window may still be unfocused.
Make focus(window:) return a success indicator (e.g., Bool / Result) based on the AX return values, and only set trackedWindowIsFocused in MouseTracker when focus reports success.
🤖 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 `@Swift` Shift/src/Manager/WindowManager.swift around lines 76 - 88,
WindowManager.focus(window:) currently ignores failures from app.activate,
AXUIElementSetAttributeValue and AXUIElementPerformAction and always returns
Void; change it to return a Bool (or Result) that aggregates the
AXError/OSStatus results (treat any non-success as failure) by checking and
combining the return values of app.activate, AXUIElementSetAttributeValue and
AXUIElementPerformAction inside focus(window:), and propagate failure to the
caller; then update MouseTracker (where trackedWindowIsFocused is set after
calling WindowManager.focus) to only set trackedWindowIsFocused = true when
focus(window:) reports success. Include references to
AXUIElementCreateApplication, kAXFrontmostAttribute, kAXMainWindowAttribute,
kAXFocusedWindowAttribute, kAXMainAttribute and kAXFocusedAttribute when
collecting results so each AX call’s status is checked and affects the returned
success value.
There was a problem hiding this comment.
there seems to be an issue where other windows that aren't the trackedWindow also get focused, what's the fix for that (we only want to activate the window not the app) - why did the original WindowManager.focus not work?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat "Swift Shift/src/Manager/MouseTracker.swift"Length of output: 17437
@chid — good questions, here's the breakdown:
Why the original focus() didn't work
The original was:
AXUIElementPerformAction(window, kAXRaiseAction as CFString)
getNSApplication(from: window)?.activate()Two problems:
kAXRaiseActiononly adjusts z-order — it brings the window visually in front but does not set it as the app's key/focused window.activate()without.activateIgnoringOtherAppsis "polite" — on modern macOS, it won't reliably steal focus away from the currently active app. If SwiftShift itself is the active app (likely during a drag), the target app may simply be ignored.
Why other windows get focused in the new code
The ordering is the culprit. The new implementation does:
1. app.activate(options: [.activateIgnoringOtherApps]) ← ASYNC, takes effect next run-loop tick
2. AXUIElementSetAttributeValue(axApp, kAXFrontmostAttribute ...)
3. AXUIElementPerformAction(window, kAXRaiseAction ...)
4. AXUIElementSetAttributeValue(axApp, kAXMainWindowAttribute ..., window) ← too late
5. AXUIElementSetAttributeValue(axApp, kAXFocusedWindowAttribute ..., window) ← too late
When app.activate fires, the system activates the app using its own last-known key window (whichever window that app had focused before). The AX attribute writes to nominate the specific trackedWindow as focused come after that activation, so there's a race you can't reliably win.
The fix: set the target window as focused before activating
Swap the order so the app activates into an already-configured state:
static func focus(window: AXUIElement) {
guard let app = getNSApplication(from: window) else { return }
let axApp = AXUIElementCreateApplication(app.processIdentifier)
let trueValue = kCFBooleanTrue!
// Tell the app which window to use as main/focused FIRST
AXUIElementSetAttributeValue(axApp, kAXMainWindowAttribute as CFString, window)
AXUIElementSetAttributeValue(axApp, kAXFocusedWindowAttribute as CFString, window)
AXUIElementSetAttributeValue(window, kAXMainAttribute as CFString, trueValue)
AXUIElementSetAttributeValue(window, kAXFocusedAttribute as CFString, trueValue)
AXUIElementPerformAction(window, kAXRaiseAction as CFString)
// NOW activate — the app will see the target window already marked as main/focused
AXUIElementSetAttributeValue(axApp, kAXFrontmostAttribute as CFString, trueValue)
app.activate(options: [.activateIgnoringOtherApps])
}By writing kAXFocusedWindowAttribute and kAXMainWindowAttribute on the AX app element before calling app.activate, you pre-configure which window the app should treat as key when it comes to the front, avoiding the race that lets the previously-focused window steal activation.
Note: Some apps (especially Electron/sandboxed apps) may ignore AX attribute writes on a background process. For those you'd need an async follow-up write after a short delay, but for standard Cocoa apps the ordering above is reliable.
🧬 Code Graph Analysis Results
Relevant snippet(s)
Swift Shift/src/Manager/WindowManager.swift
- Lines 76-88 —
static func focus(window: AXUIElement)
static func focus(window: AXUIElement) {
guard let app = getNSApplication(from: window) else { return }
let axApp = AXUIElementCreateApplication(app.processIdentifier)
let trueValue = kCFBooleanTrue!
app.activate(options: [.activateIgnoringOtherApps])
AXUIElementSetAttributeValue(axApp, kAXFrontmostAttribute as CFString, trueValue)
AXUIElementPerformAction(window, kAXRaiseAction as CFString)
AXUIElementSetAttributeValue(axApp, kAXMainWindowAttribute as CFString, window)
AXUIElementSetAttributeValue(axApp, kAXFocusedWindowAttribute as CFString, window)
AXUIElementSetAttributeValue(window, kAXMainAttribute as CFString, trueValue)
AXUIElementSetAttributeValue(window, kAXFocusedAttribute as CFString, trueValue)
}- Lines 41-49 —
static func getCurrentWindow() -> AXUIElement?
static func getCurrentWindow() -> AXUIElement? {
guard let ev = CGEvent(source: nil) else { return nil }
let sys = AXUIElementCreateSystemWide(); var el: AXUIElement?
if AXUIElementCopyElementAtPosition(sys, Float(ev.location.x), Float(ev.location.y), &el) == .success, let el = el, let w = getWindow(from: el) {
var pid: pid_t = 0; AXUIElementGetPid(w, &pid)
if pid != NSRunningApplication.current.processIdentifier { return w }
}
return getTopWindowAtCursorUsingCGWindowList(mouseLocation: ev.location)
}- Lines 50-68 —
private static func getTopWindowAtCursorUsingCGWindowList(mouseLocation: NSPoint) -> AXUIElement?
private static func getTopWindowAtCursorUsingCGWindowList(mouseLocation: NSPoint) -> AXUIElement? {
let list = CGWindowListCopyWindowInfo([.excludeDesktopElements, .optionOnScreenOnly], kCGNullWindowID) as? [[String: AnyObject]] ?? []
for e in list.sorted(by: { ($0[kCGWindowLayer as String] as? Int ?? 0) < ($1[kCGWindowLayer as String] as? Int ?? 0) }) {
if let bDict = e[kCGWindowBounds as String] as? [String: CGFloat], let b = CGRect(dictionaryRepresentation: bDict as CFDictionary), b.contains(mouseLocation), let pid = e[kCGWindowOwnerPID as String] as? pid_t {
let app = AXUIElementCreateApplication(pid); var val: AnyObject?
if let nsApp = getNSApplication(from: app), let bid = nsApp.bundleIdentifier, PreferencesManager.isAppIgnored(bid) { continue }
if AXUIElementCopyAttributeValue(app, kAXWindowsAttribute as CFString, &val) == .success, let wList = val as? [AXUIElement] {
for w in wList {
if let pos = getPosition(window: w), let size = getSize(window: w) {
let winRect = CGRect(origin: pos, size: size)
if winRect.contains(mouseLocation) { return w }
}
}
return wList.first
}
}
}
return nil
}- Lines 69-74 —
private static func getWindow(from element: AXUIElement) -> AXUIElement?
private static func getWindow(from element: AXUIElement) -> AXUIElement? {
var r: AnyObject?; AXUIElementCopyAttributeValue(element, kAXRoleAttribute as CFString, &r)
if r as? String == kAXWindowRole { return element }
var p: AnyObject?; AXUIElementCopyAttributeValue(element, kAXParentAttribute as CFString, &p)
if let parent = p { return getWindow(from: parent as! AXUIElement) }
return nil
}Swift Shift/src/Manager/Preferences.swift
- Lines 80-82 —
static func isAppIgnored(_ bundleId: String) -> Bool
static func isAppIgnored(_ bundleId: String) -> Bool {
return getIgnoredApps().contains(bundleId)
}🧠 Learnings used
Learnt from: pablopunk
Repo: pablopunk/SwiftShift PR: 150
File: Swift Shift/src/Manager/WindowManager.swift:0-0
Timestamp: 2026-05-13T17:15:02.309Z
Learning: In SwiftShift's `getVisibleWindowRects(excluding:)` (Swift Shift/src/Manager/WindowManager.swift), the exclusion filter uses exact rect equality (`rect.equalTo(excludedRect)`) to remove only the dragged window itself from snap candidates. Containing or maximized background windows must NOT be excluded — their outer edges are valid snap targets. Actual snapping is gated by perpendicular-overlap and a 10px edge-proximity check in MouseTracker, not by this exclusion filter.
1 participant
Summary
Root Cause
The previous helper only raised the AX window and called app activation. Raising can change z-order without making the target window the app focused/main window, so the Focus on window preference was unreliable.
Validation
Summary by CodeRabbit