Repository navigation
Fix AX window polling stalls with app hierarchy caching - #2986
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces a singleton cache that memoizes NSApplication Changes
Sequence Diagram(s)sequenceDiagram
participant AX as AX Client
participant App as NSApplication
participant Cache as CmuxApplicationAccessibilityHierarchyCache
participant WS as WindowServer
AX->>App: accessibilityAttributeValue(.windows)
App->>Cache: resolve(.windows, application)
alt cache hit
Cache-->>App: cached Snapshot
App-->>AX: cached value
else cache miss
Cache->>WS: request window list
WS-->>Cache: windows list
Cache-->>App: built Snapshot
App-->>AX: new value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes AX window polling stalls by swizzling the app-level accessibility attribute method to serve window hierarchy queries from a fingerprinted snapshot instead of rebuilding on every poll. Cache invalidation is implicit via a state equality check, and two unit tests cover the cache hit and invalidation paths. Confidence Score: 5/5Safe to merge; all findings are minor P2 quality suggestions with no blocking correctness issues. The swizzle is installed via the existing pattern, the cache is correctly restricted to the main thread, and both tests pass the invalidation contract. The only flagged issues are a double-read that is benign on the main thread and a test helper that continues iterating after a count failure. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as AX Client
participant App as NSApplication (swizzled)
participant Cache as CmuxApplicationAccessibilityHierarchyCache.shared
participant AppKit as AppKit (original impl)
Client->>App: accessibilityAttributeValue(attr)
App->>App: Thread.isMainThread?
alt main thread and cacheable attr
App->>Cache: resolve(attribute:, application:)
Cache->>Cache: build StateToken (windows + mainWindow + keyWindow)
alt token == cachedStateToken
Cache-->>App: .handled(cachedSnapshot[attr])
else token changed or no cache
Cache->>Cache: build Snapshot (filter visible, capture refs)
Cache->>Cache: store cachedStateToken + cachedSnapshot
Cache-->>App: .handled(snapshot[attr])
end
App-->>Client: value
else background thread or non-cacheable attr
App->>AppKit: cmux_accessibilityAttributeValue(attr) original
AppKit-->>Client: value
end
Reviews (1): Last reviewed commit: "fix: cache app AX window hierarchy" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1453
to
+1461
| private func assertWindowsEqual(_ actual: Any?, _ expected: [NSWindow], file: StaticString = #filePath, line: UInt = #line) { | ||
| guard let actualWindows = actual as? [NSWindow] else { | ||
| XCTFail("Expected NSWindow array", file: file, line: line) | ||
| return | ||
| } | ||
| XCTAssertEqual(actualWindows.count, expected.count, file: file, line: line) | ||
| for (lhs, rhs) in zip(actualWindows, expected) { | ||
| XCTAssertTrue(lhs === rhs, file: file, line: line) | ||
| } |
Contributor
There was a problem hiding this comment.
assertWindowsEqual zip only covers min(actual, expected) pairs
The count assertion on line 1458 does catch a length mismatch, but XCTAssertEqual continues execution on failure — the zip loop then runs over the shorter of the two arrays, silently skipping the extra elements. If the assertion fails and the actual array is longer than expected, the extra windows are never compared. Consider using a guard-early-return before the loop to make failures unambiguous.
Contributor
There was a problem hiding this comment.
No change here. assertWindowsEqual already guard-returns on a count mismatch before the zip loop, so extra elements are not silently skipped.
Comment on lines
+93
to
+110
| func resolve(attribute: NSAccessibility.Attribute, application: NSApplication) -> Resolution { | ||
| guard Self.supportsCaching(attribute) else { return .passthrough } | ||
| let windows = application.windows | ||
| let stateToken = StateToken( | ||
| windows: windows, | ||
| mainWindow: application.mainWindow, | ||
| focusedWindow: application.keyWindow | ||
| ) | ||
| let value = value(for: attribute, stateToken: stateToken) { | ||
| Snapshot( | ||
| windows: windows, | ||
| visibleChildren: windows.filter { $0.isVisible && !$0.isMiniaturized }, | ||
| mainWindow: application.mainWindow, | ||
| focusedWindow: application.keyWindow | ||
| ) | ||
| } | ||
| return .handled(value) | ||
| } |
Contributor
There was a problem hiding this comment.
mainWindow/keyWindow in resolve
application.mainWindow and application.keyWindow are each read twice — once to build the StateToken fingerprint and again inside the builder closure that produces the Snapshot. If those values differ between the two reads (unlikely on the main thread but possible at window-focus transitions), the cached snapshot could be stored under a fingerprint that no longer matches it, causing a spurious cache miss on the next call. Capturing both values into local let bindings before constructing either struct would ensure the token and snapshot always describe the same state.
Contributor
There was a problem hiding this comment.
This no longer applies on the current local patch. The cache is now scoped to .windows only, and resolve(...) no longer fingerprints or snapshots mainWindow / keyWindow; it caches AppKit's original AXWindows payload behind a state token derived from the window graph. I have not pushed that patch yet.
![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 the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 81-91: The cached Snapshot currently holds strong NSWindow
references (Snapshot, cachedSnapshot) which can keep closed windows alive;
change Snapshot to store weak references (e.g., use a WeakWindow wrapper class
or NSPointerArray/NSHashTable for windows and visibleChildren, and optional weak
refs for mainWindow/focusedWindow) so the cache doesn't retain windows, and when
you create/store a snapshot register for NSWindow.willCloseNotification (or
equivalent) to invalidate/clear cachedSnapshot and cachedStateToken if a window
from the snapshot closes (match the closed NSWindow against the snapshot's weak
refs and clear the singleton cache when found).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc767de4-be74-4e19-9a89-9e9dc5f8d953
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/WindowAndDragTests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Respond to PR review: - Cursor Bugbot (high): intercepting .children / .visibleChildren stripped AXMenuBar from NSApplication's accessibility tree, breaking VoiceOver. Cache only .windows (the hot path from the sample(1) stack) and let every other AX attribute fall through to AppKit. - CodeRabbit: cached Snapshot strongly retained NSWindow refs. Subscribe to NSWindow.willCloseNotification and invalidate the cache so closed windows are never kept alive between polls. - Greptile / Cursor Bugbot: remove .mainWindow / .focusedWindow caching so AppKit stays authoritative on focus transitions (addresses the double-read of application.mainWindow / keyWindow concern). - Greptile: assertWindowsEqual now uses guard early-return so the per-element comparison never runs on a count mismatch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Addressed the review feedback in 6dc48ab:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/WindowAndDragTests.swift`:
- Around line 1519-1523: In testNonWindowsAttributesStayPassthrough ensure
AppKit is initialized before using NSApp: call/force-initialize
NSApplication.shared at the start of testNonWindowsAttributesStayPassthrough
(before creating CmuxApplicationAccessibilityHierarchyCache or calling
cache.resolve(attribute:application:)) so NSApp is non-nil; this prevents
flakes/traps when resolve(_:application:) is invoked with NSApp.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d78579a-b8e6-4d11-a755-1837ee0abe1d
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/WindowAndDragTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AppDelegate.swift
Comment on lines
+1519
to
+1523
| func testNonWindowsAttributesStayPassthrough() { | ||
| let cache = CmuxApplicationAccessibilityHierarchyCache() | ||
|
|
||
| for attribute: NSAccessibility.Attribute in [.children, .visibleChildren, .mainWindow, .focusedWindow] { | ||
| switch cache.resolve(attribute: attribute, application: NSApp) { |
There was a problem hiding this comment.
Initialize NSApplication before using NSApp.
XCTest order is not guaranteed; if this test runs before another test initializes AppKit, NSApp can still be nil and the call into resolve can trap or flake.
Proposed fix
func testNonWindowsAttributesStayPassthrough() {
+ let application = NSApplication.shared
let cache = CmuxApplicationAccessibilityHierarchyCache()
for attribute: NSAccessibility.Attribute in [.children, .visibleChildren, .mainWindow, .focusedWindow] {
- switch cache.resolve(attribute: attribute, application: NSApp) {
+ switch cache.resolve(attribute: attribute, application: application) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WindowAndDragTests.swift` around lines 1519 - 1523, In
testNonWindowsAttributesStayPassthrough ensure AppKit is initialized before
using NSApp: call/force-initialize NSApplication.shared at the start of
testNonWindowsAttributesStayPassthrough (before creating
CmuxApplicationAccessibilityHierarchyCache or calling
cache.resolve(attribute:application:)) so NSApp is non-nil; this prevents
flakes/traps when resolve(_:application:) is invoked with NSApp.
Contributor
There was a problem hiding this comment.
Addressed locally. testNonWindowsAttributesStayPassthrough now initializes NSApplication.shared up front, binds it to a local application, and passes that into resolve(...) with the current originalValueProvider parameter. I have not pushed that patch yet.
There was a problem hiding this comment.
@lawrencecchen, thanks for the heads-up! The local fix sounds correct — initializing NSApplication.shared upfront and threading the local reference through to resolve(...) is exactly the right approach. I'll consider this resolved once the patch lands in the PR.
🧠 Learnings used
Learnt from: Horacehxw
Repo: manaflow-ai/cmux PR: 2043
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-04-06T09:32:26.967Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — UI test fallback window pattern: during XCTest launch stabilization, the app may force-create a fallback main window when NSApp.windows is empty. The window is tracked via `private weak var fallbackUITestWindow: NSWindow?` and, upon the first real WindowGroup registration in `registerMainWindow(...)`, if a second main window exists and the registering window differs, the fallback is closed and the reference cleared to avoid two main windows during tests.
Learnt from: Horacehxw
Repo: manaflow-ai/cmux PR: 2043
File: Sources/AppDelegate.swift:2414-2447
Timestamp: 2026-04-06T09:51:56.134Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — UI test launch call-site: In applicationDidFinishLaunching(_:), when running under XCTest, call stabilizeUITestLaunchWindowAndForeground(attempt: 0) instead of a one‑shot asyncAfter block. The stabilizer handles a single fallback window creation, attempt==0‑gated display move, app activation, diagnostics, and retries.
Learnt from: Horacehxw
Repo: manaflow-ai/cmux PR: 1980
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-04-06T09:33:43.688Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — UI test launch stabilization: in stabilizeUITestLaunchWindowAndForeground(attempt:), call moveUITestWindowToTargetDisplayIfNeeded() only on the first pass (attempt == 0) so the helper’s own 20-step retry handles display moves; subsequent attempts only activate the app and write diagnostics. This avoids nested overlapping retries and stage-log churn.
Learnt from: Horacehxw
Repo: manaflow-ai/cmux PR: 1980
File: Sources/AppDelegate.swift:2454-2458
Timestamp: 2026-04-06T09:33:40.789Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — In activateUITestAppIfNeeded() for macOS 14+, prefer NSRunningApplication.current.activate(options: [.activateAllWindows]) without .activateIgnoringOtherApps; the deprecated option is only used on older macOS. A future follow-up may migrate to the modern activation API.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 2505
File: Sources/AppDelegate.swift:4910-4918
Timestamp: 2026-04-06T02:02:45.662Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Keyboard repair pattern: In AppDelegate.repairFocusedTerminalKeyboardRoutingIfNeeded(window:event:), determine whether to repair focus by asking the focused terminal’s hosted view if the current first responder already matches that panel’s preferred keyboard target. Implemented via hostedView.responderMatchesPreferredKeyboardFocus(responder) inside responderNeedsFocusedTerminalKeyRepair(_:in:hostedView:). This covers same-window drift to a different Ghostty surface without comparing workspace/panel IDs. Verified by test cmuxTests/AppDelegateShortcutRoutingTests.swift::testWindowSendEventRepairsVisibleSameWindowResponderDriftForFocusedTerminalTyping.
Learnt from: tayl0r
Repo: manaflow-ai/cmux PR: 1909
File: Sources/AppDelegate.swift:1955-1970
Timestamp: 2026-03-21T07:13:41.796Z
Learning: Repo: manaflow-ai/cmux — AppDelegate.registerMainWindow ownership pattern: the primary window registers from ContentView.onAppear and passes the SwiftUI-owned FileBrowserDrawerState; secondary windows created via AppDelegate.createMainWindow construct and pass their own FileBrowserDrawerState. This mirrors SidebarState and avoids re-registration mismatches.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2124
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-25T08:05:26.034Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Pattern for “New Window” geometry seed:
In AppDelegate.createMainWindow(initialWorkingDirectory:sessionWindowSnapshot:), compute the existingFrame using preferredMainWindowContextForWorkspaceCreation(debugSource: "createMainWindow.initialGeometry") and then resolvedWindow(for:) rather than relying on NSApp.keyWindow or the first registered mainWindowContext. Rationale: ensures the new window inherits size from the intended main-terminal window even when an auxiliary window is key, and keeps behavior consistent with showOpenFolderPanel().
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/AppDelegate.swift:2196-2200
Timestamp: 2026-04-03T03:36:45.112Z
Learning: Repo: manaflow-ai/cmux — In Sources/AppDelegate.swift, when KeyboardShortcutSettings.didChangeNotification fires, AppDelegate must clear configured-chord caches (pendingConfiguredShortcutChord and activeConfiguredShortcutChordPrefixForCurrentEvent) via clearConfiguredShortcutChordState() before refreshing tooltips/UI. Also clear chord state on applicationWillResignActive to avoid cross-activity leakage. Verified by cmuxTests/AppDelegateShortcutRoutingTests.swift::testShortcutChangeClearsPendingConfiguredChord.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-19T00:16:24.631Z
Learning: Applies to **/*.test.swift : For metadata changes, verify the built app bundle or the runtime behavior that depends on that metadata, not the checked-in source file
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2735
File: Sources/AppDelegate.swift:476-483
Timestamp: 2026-04-09T00:29:20.810Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Deprecated API handling pattern: wrap NSWorkspace.shared.fullPath(forApplication:) in a private helper (_legacyFullPath(forApplication:)) to centralize usage and intentionally accept the single deprecation warning at that call site, since there is no non-deprecated name-based replacement. Avoid marking the wrapper itself as deprecated to prevent propagating warnings to every wrapper use.
Learnt from: tayl0r
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-08T03:36:30.160Z
Learning: Repo: manaflow-ai/cmux — AppDelegate.FileBrowserDrawerState threading pattern (PR `#1909`, commit e0e57809): FileBrowserDrawerState must be threaded through AppDelegate.configure() as a weak stored property (matching the sidebarState pattern), passed through both configure() call sites, with registerMainWindow parameter made non-optional. The fallback `?? FileBrowserDrawerState()` must NOT be used as it creates detached instances that are not properly owned by the window context.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 2505
File: Sources/AppDelegate.swift:5636-5642
Timestamp: 2026-04-06T07:18:41.310Z
Learning: Repo: manaflow-ai/cmux — In AppDelegate’s .keyDown focus-repair path, never dereference NSTextView.delegate (unsafe-unretained). Resolve field-editor ownership via cmuxFieldEditorOwnerView(_), and prefer superview/nextResponder traversal or hostedView.responderMatchesPreferredKeyboardFocus(...) for matching, as applied in AppDelegate.swift and GhosttySurfaceScrollView.
Learnt from: lejahmie
Repo: manaflow-ai/cmux PR: 2371
File: cmuxTests/CJKIMEInputTests.swift:1325-1335
Timestamp: 2026-03-31T10:56:43.241Z
Learning: Repo: manaflow-ai/cmux — In `cmuxTests/CJKIMEInputTests.swift`, `GhosttyOptionDeleteRegressionTests.testRightOptionLiteralCharacterSetsAltRightInRawModsFallbackPath` intentionally omits `consumed_mods & GHOSTTY_MODS_ALT_RIGHT` assertions. That test simulates the fallback path (no `NX_DEVICERALTKEYMASK` bit in event flags; right-side state injected via `debugSetRightOptionModifierDownForUITest(true)`), where `consumed_mods` depends on translation-mod flags and is unstable in synthetic test contexts. Only `mods & (ALT | ALT_RIGHT)` is asserted here. The `consumed_mods` coverage is provided by the deterministic `testRightOptionDeleteSetsAltRightModifier` test, which uses the direct `NX_DEVICERALTKEYMASK` modifier flag path.
Learnt from: CR
Repo: manaflow-ai/cmux PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-25T04:48:00.216Z
Learning: Applies to **/*Test*.swift : Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns. Tests must verify observable runtime behavior through executable paths (unit/integration/e2e/CLI), not implementation shape. For metadata changes, verify the built app bundle or runtime behavior, not checked-in source files.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2918-3021
Timestamp: 2026-04-14T20:00:16.490Z
Learning: Repo: manaflow-ai/cmux — Accessibility convention: For toolbar actions in macOS 26+ SwiftUI .toolbar (Sources/ContentView.swift), use localized .accessibilityLabel (and identifiers) for VoiceOver; AppKit NSToolbarItems in Sources/WindowToolbarController.swift set localized label/toolTip/accessibilityDescription. Label(...).labelStyle(.iconOnly) is not required here as VO picks up the localized accessibilityLabel.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-02T21:40:59.098Z
Learning: Repo: manaflow-ai/cmux — In Sources/Workspace.swift session snapshot logic, detected foreground command lines are only recorded if they pass SessionRestoreCommandSettings.isCommandAllowed(...). Commit f887189 moved gating to the SessionForegroundProcessCache so unallowed (potentially sensitive) commands are never written to the session JSON.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2884
File: Sources/AppIconDockTilePlugin.swift:16-19
Timestamp: 2026-04-14T08:50:27.729Z
Learning: Repo: manaflow-ai/cmux — Sources/AppIconDockTilePlugin.swift — Two-layer icon persistence contract (PR `#2884`):
- App process (Sources/cmuxApp.swift): AppIconMode.automatic returns nil for imageName to avoid writing custom icons to the bundle from the app side (runtime-only swaps).
- Dock plugin (Sources/AppIconDockTilePlugin.swift): DockTileAppIconMode.automatic intentionally returns a concrete NSImage.Name (AppIconDark or AppIconLight) based on the current effective appearance, so the plugin persists an appearance-matched icon to the bundle. This ensures the Dock shows the correct icon after force-kill/quit instead of falling back to the static bundle asset.
- Do NOT flag DockTileAppIconMode.automatic returning a concrete image name as a bug; it is the intentional Dock-side persistence path. Only when the user explicitly selects light or dark mode is the plugin's behavior identical to cmuxApp.swift. The .automatic branch must NOT return nil from the plugin.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-04-14T20:01:13.936Z
Learning: Repo: manaflow-ai/cmux — macOS 26 UI: NavigationSplitView with SwiftUI .toolbar owns window chrome; WindowToolbarController is not used on macOS 26 and remains as an AppKit fallback. Notifications popover on macOS 26 is driven via AppDelegate.toggleNotificationsPopover posting cmux.toggleNotificationsPopover, which ContentView observes to present a SwiftUI popover.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:105">
P2: Observer removal uses `NotificationCenter.default` instead of the center used for registration, so custom-center observers are not deregistered.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6dc48ab. Configure here.
3 tasks
Store the injected NotificationCenter on CmuxApplicationAccessibilityHierarchyCache so deinit removes the window-close observer from the same center it was registered on, instead of always falling back to NotificationCenter.default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
…2986) * test: add AX hierarchy cache regression * fix: cache app AX window hierarchy * fix: scope AX cache to .windows only and invalidate on window close Respond to PR review: - Cursor Bugbot (high): intercepting .children / .visibleChildren stripped AXMenuBar from NSApplication's accessibility tree, breaking VoiceOver. Cache only .windows (the hot path from the sample(1) stack) and let every other AX attribute fall through to AppKit. - CodeRabbit: cached Snapshot strongly retained NSWindow refs. Subscribe to NSWindow.willCloseNotification and invalidate the cache so closed windows are never kept alive between polls. - Greptile / Cursor Bugbot: remove .mainWindow / .focusedWindow caching so AppKit stays authoritative on focus transitions (addresses the double-read of application.mainWindow / keyWindow concern). - Greptile: assertWindowsEqual now uses guard early-return so the per-element comparison never runs on a count mismatch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix: remove AX cache observer from the injected notification center Store the injected NotificationCenter on CmuxApplicationAccessibilityHierarchyCache so deinit removes the window-close observer from the same center it was registered on, instead of always falling back to NotificationCenter.default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This branch was successfully deployed
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
2 participants
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.
Closes #2985.
Summary
AXWindowsresponses onNSApplicationso repeated AX polls (Raycast / AltTab / VoiceOver / etc. — ~32/s persample(1)) reuse a single snapshot while the window graph is unchanged.NSApplication.accessibilityAttributeValue(_:)and answer.windowsfrom a cached snapshot fingerprinted by window identity + number + visibility + miniaturized state.NSWindow.willCloseNotificationso closed windows are never retained between polls..windowsonly —.children,.visibleChildren,.mainWindow, and.focusedWindowall pass through to AppKit. This keepsAXMenuBarin the NSApplication AX tree (VoiceOver etc.) and leaves AppKit authoritative on focus transitions.Testing
ApplicationAccessibilityHierarchyCacheTests.windowsqueries with an unchanged state token reuse a single snapshot build.children/.visibleChildren/.mainWindow/.focusedWindowstay passthroughNSWindow.willCloseNotificationinvalidates the cacheSummary by CodeRabbit
New Features
Tests
Note
Medium Risk
Swizzles
NSApplication.accessibilityAttributeValue(_:)and changes how.windowsAX queries are answered, which can affect accessibility clients and window enumeration if the cache/invalidation is wrong.Overview
Reduces repeated accessibility-window polling overhead by caching
NSApplication’s.windowsAX attribute behind a newCmuxApplicationAccessibilityHierarchyCache, keyed by a window-state token (identity/number/visibility/miniaturized) and invalidated onNSWindow.willCloseNotification.Installs a new
NSApplication.accessibilityAttributeValue(_:)swizzle that serves cached results only on the main thread for.windows, while leaving.children,.visibleChildren,.mainWindow, and.focusedWindowas AppKit passthrough. Adds unit tests covering cache reuse, invalidation on state change and window close, and passthrough behavior for non-window attributes.Reviewed by Cursor Bugbot for commit 7d8ad3a. Bugbot is set up for automated code reviews on this repo. Configure here.