Repository navigation
Make shortcut routing focus tests deterministic - #6419
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces ChangesShortcut Routing Window Centralization and Testing
CI Infrastructure and Project Configuration
Sequence Diagram(s)sequenceDiagram
participant Test as XCTest<br/>(AppDelegateShortcutRoutingTests)
participant Swizzle as NSWindow<br/>cmux_makeKeyAndOrderFront
participant Override as DebugShortcutRoutingFocusedWindowOverrideForTesting
participant Router as AppDelegate<br/>shortcutRoutingKeyWindow
participant Handler as handleCustomShortcut
participant Repair as repairFocusedTerminalKeyboardRoutingIfNeeded
Test->>Test: beginFocusedWindowCapture()
Test->>Test: attachTestResponder() injects wrong responder
Test->>Handler: Send keyboard event (typing in terminal)
Handler->>Router: Resolve shortcut routing window
Router->>Override: Check DEBUG focused-window override
Override-->>Router: Return override if in capture session
Router-->>Handler: Return key window for routing
Handler->>Repair: Detect first-responder mismatch
Repair->>Repair: Use optional firstResponderOverride in DEBUG
Repair->>Test: Invoke debugFocusedTerminalKeyRepairObserverForTesting callback
Test->>Test: Probe captures repairCount and responder
Test->>Test: waitUntil() confirms first responder restored
Test->>Test: Assert repairCount == 1 and responder matches simulation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (20 passed)
✨ 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 |
Greptile SummaryThis PR introduces a DEBUG-only focused-window override seam so shortcut-routing tests can pin the target window deterministically without relying on the OS window server to honor
Confidence Score: 4/5Safe to merge with awareness that two production shortcut-routing behavior changes bundled with the test-determinism work need a second look before shipping. The DEBUG infrastructure itself is clean and well-contained — all override paths are #if DEBUG-guarded and have no effect on release builds. The mechanical substitution of NSApp.keyWindow with shortcutRoutingKeyWindow across 16 call sites is consistent and correct. Two production changes bundled in the same commit warrant extra attention: the reordering of findShortcutTarget before focusForInWindowCommand silently drops the focus side-effect when no find target is available, and the makeKeyAndOrderFront swizzle that captures any NSWindow subclass during test scope can overwrite the intended override if an auxiliary window becomes key mid-test. Both were noted in prior review threads and neither has been addressed in this revision. Sources/AppDelegate.swift — the find-shortcut reordering and Sources/AppDelegate+ShortcutRoutingTesting.swift — the broad NSWindow swizzle. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Test as XCTest body
participant Setup as setUpWithError
participant Delegate as AppDelegate
participant Override as DebugOverride singleton
participant Swizzle as cmux_makeKeyAndOrderFront
Setup->>Override: focusedWindowCaptureDepth plus 1
Setup->>Delegate: debugResetShortcutRoutingStateForTesting
Test->>Swizzle: window.makeKeyAndOrderFront fires
Swizzle->>Override: shouldCaptureFocusedWindow true
Swizzle->>Delegate: debugSetShortcutRoutingFocusedWindowForTesting
Test->>Delegate: shortcutRoutingKeyWindow read
Delegate->>Override: capture mode active, return override window
Delegate-->>Test: testWindow
Test->>Delegate: window.sendEvent keyDown
Delegate->>Delegate: repairFocusedTerminalKeyboardRoutingIfNeeded 2-arg
Delegate->>Override: read keyRepairFirstResponder override
Delegate->>Delegate: repairFocusedTerminalKeyboardRoutingIfNeeded 3-arg
Test->>Setup: tearDown calls debugEndShortcutRoutingFocusedWindowCaptureForTesting
Setup->>Override: depth clamped to 0
Setup->>Delegate: debugResetShortcutRoutingStateForTesting clears all state
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Test as XCTest body
participant Setup as setUpWithError
participant Delegate as AppDelegate
participant Override as DebugOverride singleton
participant Swizzle as cmux_makeKeyAndOrderFront
Setup->>Override: focusedWindowCaptureDepth plus 1
Setup->>Delegate: debugResetShortcutRoutingStateForTesting
Test->>Swizzle: window.makeKeyAndOrderFront fires
Swizzle->>Override: shouldCaptureFocusedWindow true
Swizzle->>Delegate: debugSetShortcutRoutingFocusedWindowForTesting
Test->>Delegate: shortcutRoutingKeyWindow read
Delegate->>Override: capture mode active, return override window
Delegate-->>Test: testWindow
Test->>Delegate: window.sendEvent keyDown
Delegate->>Delegate: repairFocusedTerminalKeyboardRoutingIfNeeded 2-arg
Delegate->>Override: read keyRepairFirstResponder override
Delegate->>Delegate: repairFocusedTerminalKeyboardRoutingIfNeeded 3-arg
Test->>Setup: tearDown calls debugEndShortcutRoutingFocusedWindowCaptureForTesting
Setup->>Override: depth clamped to 0
Setup->>Delegate: debugResetShortcutRoutingStateForTesting clears all state
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "fr=\(beforeResponder)" | ||
| ) | ||
| #endif | ||
| let target = context.keyboardFocusCoordinator.findShortcutTarget( | ||
| currentResponder: window?.firstResponder | ||
| ) | ||
| guard target != .none else { | ||
| #if DEBUG | ||
| dlog( | ||
| "find.shortcut.app.end target=\(target) result=0 " + | ||
| "targetWin={\(debugWindowToken(window))} fr=\(beforeResponder)" | ||
| ) | ||
| #endif | ||
| return false | ||
| } | ||
|
|
||
| if let window { | ||
| mainWindowVisibilityController.focusForInWindowCommand(window, reason: .findShortcut) | ||
| } | ||
|
|
||
| let target = context.keyboardFocusCoordinator.findShortcutTarget( | ||
| currentResponder: window?.firstResponder | ||
| ) | ||
| let result: Bool | ||
| switch target { | ||
| case .rightSidebarFileSearch: | ||
| result = context.keyboardFocusCoordinator.focusFileSearch() | ||
| case .mainPanelFind: | ||
| result = context.tabManager.startSearch() | ||
| case .none: | ||
| result = false | ||
| return false | ||
| } | ||
| #if DEBUG |
There was a problem hiding this comment.
focusForInWindowCommand silently dropped when find target is .none
The reordering moves findShortcutTarget before focusForInWindowCommand, then guards early on .none. This means pressing the Find shortcut in a window state where no target is available no longer calls focusForInWindowCommand(window, reason: .findShortcut). Previously the window was always brought forward regardless of whether find had a target. Whether that was intentional or accidental, this is an unannounced production behavior change bundled into a test-determinism PR — if any caller relied on the focus side-effect even when find returns false, those paths silently regress here.
| } | ||
|
|
||
| private extension NSWindow { | ||
| #if DEBUG | ||
| @objc func cmux_makeKeyAndOrderFront(_ sender: Any?) { | ||
| cmux_makeKeyAndOrderFront(sender) | ||
| AppDelegate.shared?.debugSetShortcutRoutingFocusedWindowForTesting(self) |
There was a problem hiding this comment.
makeKeyAndOrderFront call
cmux_makeKeyAndOrderFront fires for ALL NSWindow subclasses in DEBUG builds. If any auxiliary window (settings panel, alert sheet, a secondary test window's makeKeyAndOrderFront from setup helpers, etc.) calls makeKeyAndOrderFront after a test pins its target window, the override is silently replaced with that auxiliary window. The validation in shortcutRoutingKeyWindow then clears the stale non-cmux override, but the test's intended window has already been erased. Tests that open two windows or share setup helpers that call makeKeyAndOrderFront after the assertion window will see .none routing rather than the pinned override, producing exactly the kind of non-deterministic failures this PR is meant to fix.
d2742fe to
c1d1885
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/AppDelegate.swift (1)
12827-12838:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the shortcut window before inspecting first responder state.
These checks run before
synchronizeShortcutRoutingContext(event:), but they readshortcutRoutingKeyWindow?.firstResponder. For key events that identify the target bywindowNumberwhileevent.windowis nil, this can inspect another focused window, clearingbrowserAddressBarFocusedPanelIdor bypassing IME handling for the wrong shortcut context. Use the event-resolved window once and reuse it for these first-responder checks.🐛 Proposed fix
+ let shortcutWindowForFirstResponderChecks = resolvedShortcutEventWindow(event) ?? shortcutRoutingActiveWindow if browserAddressBarFocusedPanelId != nil, - cmuxOwningGhosttyView(for: shortcutRoutingKeyWindow?.firstResponder) != nil { + cmuxOwningGhosttyView(for: shortcutWindowForFirstResponderChecks?.firstResponder) != nil { `#if` DEBUG let stalePanelToken = browserAddressBarFocusedPanelId.map { String($0.uuidString.prefix(5)) } ?? "nil" - let firstResponderType = shortcutRoutingKeyWindow?.firstResponder.map { String(describing: type(of: $0)) } ?? "nil" + let firstResponderType = shortcutWindowForFirstResponderChecks?.firstResponder.map { String(describing: type(of: $0)) } ?? "nil" cmuxDebugLog( "browser.focus.addressBar.staleClear panel=\(stalePanelToken) " + "reason=terminal_first_responder fr=\(firstResponderType)" @@ if !normalizedFlags.contains(.command), - let ghosttyView = cmuxOwningGhosttyView(for: shortcutRoutingKeyWindow?.firstResponder), + let ghosttyView = cmuxOwningGhosttyView(for: shortcutWindowForFirstResponderChecks?.firstResponder), ghosttyView.hasMarkedText() { return false } - let shortcutWindowForMarkedText = resolvedShortcutEventWindow(event) ?? event.window ?? shortcutRoutingActiveWindow + let shortcutWindowForMarkedText = shortcutWindowForFirstResponderChecksAlso applies to: 12903-12909
🤖 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 `@Sources/AppDelegate.swift` around lines 12827 - 12838, The code inspects shortcutRoutingKeyWindow?.firstResponder before the window context has been properly resolved, which can cause it to check the wrong window when event.window is nil and the target is identified by windowNumber. This leads to incorrectly clearing browserAddressBarFocusedPanelId or bypassing IME handling for the wrong context. Resolve the shortcut window from the event first (before these checks run), then reuse that resolved window reference when inspecting firstResponder state instead of directly accessing shortcutRoutingKeyWindow?.firstResponder. Apply this fix to both occurrences mentioned in the comment (the one in the diff and the one at lines 12903-12909).cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
3991-4003:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRestore the
contentViewbinding before using it.Line 4003 references
contentView, but the guard no longer binds it, so this file will not compile. Addlet contentView = window.contentViewback to the guard.🐛 Proposed fix
- guard let window = window(withId: windowId), + guard let window = window(withId: windowId), + let contentView = window.contentView, let manager = appDelegate.tabManagerFor(windowId: windowId), let workspace = manager.selectedWorkspace, let browserPanelId = manager.openBrowser(inWorkspace: workspace.id) else {🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 3991 - 4003, The contentView property is being accessed when calling addSubview on line 4003, but it is not bound in the guard statement. Add contentView as an additional binding in the guard statement by including let contentView = window.contentView alongside the existing bindings for window, manager, workspace, and browserPanelId. This will ensure contentView is properly initialized before it is used.
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 13520-13523: The performSplitShortcut call is using a fallback to
shortcutRoutingActiveWindow when event.window is nil, which can cause the wrong
workspace to be mutated if the routing context had a specific window resolved.
Replace the preferredWindow parameter value from the pattern event.window ??
shortcutRoutingActiveWindow with resolvedShortcutEventWindow(event) to properly
preserve the resolved window through the split action. This same fix should be
applied to all other similar shortcut call sites mentioned in the comment (lines
13534-13537, 14450, 14919-14924, 15096-15112) and within configured-action
execution, use resolvedWindow(for: context) instead.
---
Outside diff comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 3991-4003: The contentView property is being accessed when calling
addSubview on line 4003, but it is not bound in the guard statement. Add
contentView as an additional binding in the guard statement by including let
contentView = window.contentView alongside the existing bindings for window,
manager, workspace, and browserPanelId. This will ensure contentView is properly
initialized before it is used.
In `@Sources/AppDelegate.swift`:
- Around line 12827-12838: The code inspects
shortcutRoutingKeyWindow?.firstResponder before the window context has been
properly resolved, which can cause it to check the wrong window when
event.window is nil and the target is identified by windowNumber. This leads to
incorrectly clearing browserAddressBarFocusedPanelId or bypassing IME handling
for the wrong context. Resolve the shortcut window from the event first (before
these checks run), then reuse that resolved window reference when inspecting
firstResponder state instead of directly accessing
shortcutRoutingKeyWindow?.firstResponder. Apply this fix to both occurrences
mentioned in the comment (the one in the diff and the one at lines 12903-12909).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 045207ae-7925-4306-8f3b-be3219f26d9c
📒 Files selected for processing (3)
.github/workflows/ci.ymlSources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
…-focus # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
6862-6925: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake the simulated drift responder own focus before dispatch.
Both same-window drift tests inject
strayViewinto the DEBUG repair seam, but the actualwindow.firstResponderis still the terminal/search responder whenwindow.sendEventruns. That means the final focus/string assertions can pass even if the repair path stops restoring AppKit focus. SetstrayViewas the real first responder before sending the key event.Proposed test tightening
XCTAssertFalse( terminalPanel.hostedView.responderMatchesPreferredKeyboardFocus(strayView), "Expected the simulated responder to disagree with the focused terminal" ) + XCTAssertTrue(window.makeFirstResponder(strayView), "Expected simulated responder drift before typing") + XCTAssertTrue(window.firstResponder === strayView)XCTAssertFalse( terminalPanel.hostedView.responderMatchesPreferredKeyboardFocus(strayView), "Expected the simulated responder to disagree with terminal search focus" ) + XCTAssertTrue(window.makeFirstResponder(strayView), "Expected simulated responder drift before typing") + XCTAssertTrue(window.firstResponder === strayView)As per coding guidelines, tests must assert on causality rather than only observing that a path was reached.
Also applies to: 11037-11088
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 6862 - 6925, The test injects strayView into the DEBUG repair seam via debugSetShortcutRoutingKeyRepairFirstResponderForTesting but does not actually set it as the window's real first responder before calling window.sendEvent. This means the test can pass even if the repair mechanism fails to restore focus, since the actual first responder is still the terminal. Before dispatching the key event with window.sendEvent(keyDown), explicitly set strayView as the window's first responder by calling window.makeFirstResponder(strayView) so the test validates that the repair path correctly restores focus from the wrong responder back to the terminal.Source: Coding guidelines
🤖 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 `@Sources/AppDelegate`+ShortcutRoutingWindow.swift:
- Around line 33-50: The method activeTabManagerForCommands directly iterates
over mainWindowContexts.values while calling resolvedWindow(for:), which can
cause mutation-during-enumeration issues since resolvedWindow(for:) may mutate
mainWindowContexts during iteration. To fix this, snapshot the values before
iterating by wrapping mainWindowContexts.values with Array() in the same way it
is correctly done at line 70, ensuring the dictionary values are captured as an
immutable array before the first call to resolvedWindow(for:).
---
Outside diff comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 6862-6925: The test injects strayView into the DEBUG repair seam
via debugSetShortcutRoutingKeyRepairFirstResponderForTesting but does not
actually set it as the window's real first responder before calling
window.sendEvent. This means the test can pass even if the repair mechanism
fails to restore focus, since the actual first responder is still the terminal.
Before dispatching the key event with window.sendEvent(keyDown), explicitly set
strayView as the window's first responder by calling
window.makeFirstResponder(strayView) so the test validates that the repair path
correctly restores focus from the wrong responder back to the terminal.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 627e7ee9-b8cb-4636-a204-8eb0260586e7
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
.github/workflows/ci.ymlSources/AppDelegate+ShortcutRoutingTesting.swiftSources/AppDelegate+ShortcutRoutingWindow.swiftSources/AppDelegate.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateShortcutRoutingTests.swift
| func activeTabManagerForCommands(preferredWindow: NSWindow? = nil) -> TabManager? { | ||
| if let context = contextForMainWindow(preferredWindow) { | ||
| return context.tabManager | ||
| } | ||
| if let context = contextForMainWindow(shortcutRoutingKeyWindow) { | ||
| return context.tabManager | ||
| } | ||
| if let context = contextForMainWindow(NSApp.mainWindow) { | ||
| return context.tabManager | ||
| } | ||
| if let activeManager = tabManager, | ||
| let activeContext = liveMainWindowContext(for: activeManager) { | ||
| return activeContext.tabManager | ||
| } | ||
| return mainWindowContexts.values.first { context in | ||
| resolvedWindow(for: context) != nil | ||
| }?.tabManager | ||
| } |
There was a problem hiding this comment.
Snapshot mainWindowContexts.values before iterating with resolvedWindow(for:).
Line 70 correctly uses Array(mainWindowContexts.values) before calling resolvedWindow(for:), but the fallback at lines 47-49 iterates the dictionary directly. Since resolvedWindow(for:) may reindex/mutate mainWindowContexts, this can cause mutation-during-enumeration issues.
🔧 Suggested fix
- return mainWindowContexts.values.first { context in
+ return Array(mainWindowContexts.values).first { context in
resolvedWindow(for: context) != nil
}?.tabManagerBased on learnings: "avoid iterating mainWindowContexts.values directly while calling resolvedWindow(for:). Since resolvedWindow(for:) may reindex/mutate mainWindowContexts, this can cause mutation-during-enumeration issues. Instead, snapshot first with Array(mainWindowContexts.values)".
🤖 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 `@Sources/AppDelegate`+ShortcutRoutingWindow.swift around lines 33 - 50, The
method activeTabManagerForCommands directly iterates over
mainWindowContexts.values while calling resolvedWindow(for:), which can cause
mutation-during-enumeration issues since resolvedWindow(for:) may mutate
mainWindowContexts during iteration. To fix this, snapshot the values before
iterating by wrapping mainWindowContexts.values with Array() in the same way it
is correctly done at line 70, ensuring the dictionary values are captured as an
immutable array before the first call to resolvedWindow(for:).
Source: Learnings
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift`:
- Around line 43-46: In the restore closure where
appDelegate.debugFocusedTerminalKeyRepairObserverForTesting and
GhosttyNSView.debugGhosttySurfaceKeyEventObserver are being reset, add a call to
addTeardownBlock with the same cleanup logic to automatically register the
cleanup with XCTest's teardown mechanism. This ensures cleanup happens even if a
caller forgets to explicitly call the returned restore closure in a defer
statement, preventing observer leaks between tests.
- Around line 12-37: The function installFocusedTerminalRepairProbeForTesting
accesses the actor-isolated property
GhosttyNSView.debugGhosttySurfaceKeyEventObserver without proper actor
isolation, which violates Swift concurrency rules. Add the `@MainActor` attribute
to the function signature of installFocusedTerminalRepairProbeForTesting to
match the MainActor isolation of the debugGhosttySurfaceKeyEventObserver
property.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96c98740-c75d-48aa-af85-cc58fb08ad7d
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Sources/AppDelegate+ShortcutRoutingTesting.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AppDelegateShortcutRoutingRepairProbe.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| func installFocusedTerminalRepairProbeForTesting( | ||
| appDelegate: AppDelegate, | ||
| keyCode: UInt32 | ||
| ) -> ( | ||
| repairCount: () -> Int, | ||
| repairResponder: () -> NSResponder?, | ||
| forwardedKeyDownCount: () -> Int, | ||
| restore: () -> Void | ||
| ) { | ||
| var repairCount = 0 | ||
| var repairResponder: NSResponder? | ||
| let previousRepairObserver = appDelegate.debugFocusedTerminalKeyRepairObserverForTesting | ||
| appDelegate.debugFocusedTerminalKeyRepairObserverForTesting = { window, event, responder in | ||
| previousRepairObserver?(window, event, responder) | ||
| guard UInt32(event.keyCode) == keyCode else { return } | ||
| repairCount += 1 | ||
| repairResponder = responder | ||
| } | ||
|
|
||
| var forwardedKeyDownCount = 0 | ||
| let previousKeyEventObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver | ||
| GhosttyNSView.debugGhosttySurfaceKeyEventObserver = { keyEvent in | ||
| previousKeyEventObserver?(keyEvent) | ||
| guard keyEvent.action == GHOSTTY_ACTION_PRESS, keyEvent.keycode == keyCode else { return } | ||
| forwardedKeyDownCount += 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the test file and examine its context
fd -t f "AppDelegateShortcutRoutingTests.swift" --type fRepository: manaflow-ai/cmux
Length of output: 108
🏁 Script executed:
# Find GhosttyNSView to see the declaration of debugGhosttySurfaceKeyEventObserver
fd -type f "GhosttyNSView.swift" -o "GhosttyNSView.mm" -o "*Ghostty*View*"Repository: manaflow-ai/cmux
Length of output: 231
🏁 Script executed:
# Search for the debugGhosttySurfaceKeyEventObserver declaration
rg "debugGhosttySurfaceKeyEventObserver" -A 2 -B 2 --type swiftRepository: manaflow-ai/cmux
Length of output: 20911
🏁 Script executed:
# Find and examine AppDelegateShortcutRoutingTests class
rg -t swift "class AppDelegateShortcutRoutingTests" -A 20Repository: manaflow-ai/cmux
Length of output: 2169
🏁 Script executed:
# Check if the test file exists and examine the helper function
cat -n cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift 2>/dev/null | head -50Repository: manaflow-ai/cmux
Length of output: 2176
🏁 Script executed:
# Search for GhosttyNSView.swift in the Sources directory
find . -name "GhosttyNSView.swift" -o -name "*GhosttyNSView*" 2>/dev/null | grep -v ".build" | head -10Repository: manaflow-ai/cmux
Length of output: 157
🏁 Script executed:
# Broader search for debugGhosttySurfaceKeyEventObserver
rg "debugGhosttySurfaceKeyEventObserver" -B 5 -A 3Repository: manaflow-ai/cmux
Length of output: 32306
🏁 Script executed:
# Let's search for all files mentioning this observer
rg -l "debugGhosttySurfaceKeyEventObserver" --type swiftRepository: manaflow-ai/cmux
Length of output: 374
🏁 Script executed:
# Check for files in the Packages directory since GhosttyKit might be there
find . -path "*Packages*" -name "*.swift" -type f | xargs grep -l "debugGhosttySurfaceKeyEventObserver" 2>/dev/null | head -5Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Look for the GhosttyTerminalView or related Swift files that might contain GhosttyNSView
fd "GhosttyTerminalView" -t f --type swiftRepository: manaflow-ai/cmux
Length of output: 233
🏁 Script executed:
# Search the entire codebase for `@MainActor` declarations near this observer
rg "`@MainActor`" -B 2 -A 2 | grep -A 5 -B 5 "debugGhosttySurfaceKeyEventObserver"Repository: manaflow-ai/cmux
Length of output: 824
🏁 Script executed:
# Let's check what files are in cmuxTests
ls -la cmuxTests/*.swift | head -20Repository: manaflow-ai/cmux
Length of output: 2026
🏁 Script executed:
# Check if AppDelegateShortcutRoutingRepairProbe.swift exists
cat -n cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift 2>&1Repository: manaflow-ai/cmux
Length of output: 2176
🏁 Script executed:
# Search for AppDelegateShortcutRoutingRepairProbe in all Swift files
rg "AppDelegateShortcutRoutingRepairProbe" -t swiftRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if the file exists at all
find . -name "*AppDelegateShortcutRouting*" -type f 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 166
🏁 Script executed:
# Search for installFocusedTerminalRepairProbeForTesting
rg "installFocusedTerminalRepairProbeForTesting" -B 5 -A 30Repository: manaflow-ai/cmux
Length of output: 9984
🏁 Script executed:
# Search for GhosttyNSView anywhere in the codebase
rg "GhosttyNSView" -t swift | head -20Repository: manaflow-ai/cmux
Length of output: 2096
🏁 Script executed:
# Let's search for the pattern more broadly
rg "debugFocusedTerminalKeyRepairObserverForTesting" -B 5 -A 10Repository: manaflow-ai/cmux
Length of output: 9204
🏁 Script executed:
# Check what test files exist
find cmuxTests -name "*.swift" -type f | sortRepository: manaflow-ai/cmux
Length of output: 11427
🏁 Script executed:
# Check if there are recent files or this is a new PR file
git ls-files cmuxTests/ | grep -i "repair\|probe"Repository: manaflow-ai/cmux
Length of output: 114
🏁 Script executed:
# Check git status for new files
git status --short | head -20Repository: manaflow-ai/cmux
Length of output: 89
🏁 Script executed:
# Search for mentions of GhosttyNSView with `@MainActor`
rg "`@MainActor`.*GhosttyNSView|class GhosttyNSView" -A 10Repository: manaflow-ai/cmux
Length of output: 1815
🏁 Script executed:
# Look for any observer pattern definitions
rg "debugGhosttySurfaceKeyEventObserver\s*=" -B 10 -A 5Repository: manaflow-ai/cmux
Length of output: 47542
🏁 Script executed:
# Check if there are any untracked or staged files matching the pattern
git status --short | grep -i "appdel.*shortcut.*repair"Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Look for any file with similar naming that might exist
fd -type f ".*Shortcut.*" -path "*/cmuxTests/*"Repository: manaflow-ai/cmux
Length of output: 231
🏁 Script executed:
# Check if this is in a staged or unstaged change
git diff --name-only 2>/dev/null | grep -i repairRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Look for AppDelegate related test files
fd -type f "AppDelegate.*Test" -path "*/cmuxTests/*"Repository: manaflow-ai/cmux
Length of output: 231
🏁 Script executed:
# Check if the code snippet references are pointing to an existing test class
rg "AppDelegateShortcutRoutingTests" -t swift -A 5Repository: manaflow-ai/cmux
Length of output: 1340
🏁 Script executed:
# Look for other test helper patterns to understand the established pattern
rg "extension.*Tests.*{" -A 10 --type swift | grep -A 10 "func.*Probe\|func.*Observer\|func.*Testing" | head -40Repository: manaflow-ai/cmux
Length of output: 162
🏁 Script executed:
# Search for existing `@MainActor` uses in test files
rg "`@MainActor`" --type swift -B 2 -A 2 | grep -B 5 -A 5 "cmuxTests"Repository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
# Look for manual observer cleanup patterns in existing tests
rg "previousObserver\|previousKeyEvent" -A 5 -B 5 --type swiftRepository: manaflow-ai/cmux
Length of output: 42
Mark this helper @MainActor to match actor-isolated observer APIs.
This function reads and writes GhosttyNSView.debugGhosttySurfaceKeyEventObserver (declared @MainActor static var in GhosttyTerminalView.swift) from a nonisolated context. Swift concurrency rules require an explicit @MainActor annotation on the function to match the isolation of the property it accesses.
Suggested fix
extension AppDelegateShortcutRoutingTests {
+ `@MainActor`
func installFocusedTerminalRepairProbeForTesting(
appDelegate: AppDelegate,
keyCode: UInt32
) -> (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func installFocusedTerminalRepairProbeForTesting( | |
| appDelegate: AppDelegate, | |
| keyCode: UInt32 | |
| ) -> ( | |
| repairCount: () -> Int, | |
| repairResponder: () -> NSResponder?, | |
| forwardedKeyDownCount: () -> Int, | |
| restore: () -> Void | |
| ) { | |
| var repairCount = 0 | |
| var repairResponder: NSResponder? | |
| let previousRepairObserver = appDelegate.debugFocusedTerminalKeyRepairObserverForTesting | |
| appDelegate.debugFocusedTerminalKeyRepairObserverForTesting = { window, event, responder in | |
| previousRepairObserver?(window, event, responder) | |
| guard UInt32(event.keyCode) == keyCode else { return } | |
| repairCount += 1 | |
| repairResponder = responder | |
| } | |
| var forwardedKeyDownCount = 0 | |
| let previousKeyEventObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver | |
| GhosttyNSView.debugGhosttySurfaceKeyEventObserver = { keyEvent in | |
| previousKeyEventObserver?(keyEvent) | |
| guard keyEvent.action == GHOSTTY_ACTION_PRESS, keyEvent.keycode == keyCode else { return } | |
| forwardedKeyDownCount += 1 | |
| } | |
| `@MainActor` | |
| func installFocusedTerminalRepairProbeForTesting( | |
| appDelegate: AppDelegate, | |
| keyCode: UInt32 | |
| ) -> ( | |
| repairCount: () -> Int, | |
| repairResponder: () -> NSResponder?, | |
| forwardedKeyDownCount: () -> Int, | |
| restore: () -> Void | |
| ) { | |
| var repairCount = 0 | |
| var repairResponder: NSResponder? | |
| let previousRepairObserver = appDelegate.debugFocusedTerminalKeyRepairObserverForTesting | |
| appDelegate.debugFocusedTerminalKeyRepairObserverForTesting = { window, event, responder in | |
| previousRepairObserver?(window, event, responder) | |
| guard UInt32(event.keyCode) == keyCode else { return } | |
| repairCount += 1 | |
| repairResponder = responder | |
| } | |
| var forwardedKeyDownCount = 0 | |
| let previousKeyEventObserver = GhosttyNSView.debugGhosttySurfaceKeyEventObserver | |
| GhosttyNSView.debugGhosttySurfaceKeyEventObserver = { keyEvent in | |
| previousKeyEventObserver?(keyEvent) | |
| guard keyEvent.action == GHOSTTY_ACTION_PRESS, keyEvent.keycode == keyCode else { return } | |
| forwardedKeyDownCount += 1 | |
| } |
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift` around lines 12 - 37,
The function installFocusedTerminalRepairProbeForTesting accesses the
actor-isolated property GhosttyNSView.debugGhosttySurfaceKeyEventObserver
without proper actor isolation, which violates Swift concurrency rules. Add the
`@MainActor` attribute to the function signature of
installFocusedTerminalRepairProbeForTesting to match the MainActor isolation of
the debugGhosttySurfaceKeyEventObserver property.
| restore: { | ||
| appDelegate.debugFocusedTerminalKeyRepairObserverForTesting = previousRepairObserver | ||
| GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousKeyEventObserver | ||
| } |
There was a problem hiding this comment.
Avoid cleanup leaks by auto-registering restore in teardown.
Relying only on a returned restore closure is brittle; one missed defer path can leak observers into later tests and reintroduce flakiness. Register cleanup via addTeardownBlock inside this helper as a safety net.
Suggested hardening
return (
repairCount: { repairCount },
repairResponder: { repairResponder },
forwardedKeyDownCount: { forwardedKeyDownCount },
restore: {
appDelegate.debugFocusedTerminalKeyRepairObserverForTesting = previousRepairObserver
GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousKeyEventObserver
}
)
+ // Alternative pattern:
+ // addTeardownBlock { [weak appDelegate] in
+ // appDelegate?.debugFocusedTerminalKeyRepairObserverForTesting = previousRepairObserver
+ // GhosttyNSView.debugGhosttySurfaceKeyEventObserver = previousKeyEventObserver
+ // }🤖 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 `@cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift` around lines 43 - 46,
In the restore closure where
appDelegate.debugFocusedTerminalKeyRepairObserverForTesting and
GhosttyNSView.debugGhosttySurfaceKeyEventObserver are being reset, add a call to
addTeardownBlock with the same cleanup logic to automatically register the
cleanup with XCTest's teardown mechanism. This ensures cleanup happens even if a
caller forgets to explicitly call the returned restore closure in a defer
statement, preventing observer leaks between tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
4072-4077:⚠️ Potential issue | 🔴 CriticalFix the undeclared
contentViewreference.Line 4076 calls
contentView.addSubview(field), butcontentViewis not declared in the test. Replace with the existingattachTestResponderhelper already used elsewhere in this file.Proposed fix
field.identifier = browserOmnibarTextFieldIdentifier field.panelId = browserPanelId field.stringValue = "ㄉㄚˋ" - contentView.addSubview(field) + attachTestResponder(field, to: window) BrowserOmnibarNativeFieldRegistry.shared.register(field, panelId: browserPanelId)🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 4072 - 4077, The test code contains an undeclared reference to contentView on the line calling contentView.addSubview(field) in the OmnibarNativeTextField setup block. Replace the contentView.addSubview(field) call with the attachTestResponder helper function that is already being used elsewhere in this test file, passing the field object as the appropriate parameter to properly attach the test responder instead of relying on the undefined contentView variable.
♻️ Duplicate comments (1)
Sources/AppDelegate+ShortcutRoutingWindow.swift (1)
50-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnapshot
mainWindowContexts.valuesbefore iterating withresolvedWindow(for:).This fallback iterates the dictionary directly while calling
resolvedWindow(for:), which may mutatemainWindowContextsand cause mutation-during-enumeration issues. Line 73 inliveMainWindowContextcorrectly usesArray(mainWindowContexts.values).🔧 Suggested fix
- return mainWindowContexts.values.first { context in + return Array(mainWindowContexts.values).first { context in resolvedWindow(for: context) != nil }?.tabManager🤖 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 `@Sources/AppDelegate`+ShortcutRoutingWindow.swift around lines 50 - 52, The code iterates directly over mainWindowContexts.values while calling resolvedWindow(for:), which may mutate the mainWindowContexts dictionary during enumeration and cause a runtime crash. Fix this by creating a snapshot of the dictionary values before iterating, similar to the pattern used in liveMainWindowContext on line 73. Wrap mainWindowContexts.values with Array() to create an immutable copy before calling first on it, ensuring the dictionary cannot be mutated during the enumeration with resolvedWindow(for:).Source: Learnings
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift`:
- Line 21: The current implementation uses RunLoop.main.run(until:
Date(timeIntervalSinceNow: 0.05)) which is a fixed wall-clock sleep pattern that
violates test guidelines and can cause flaky tests. Replace this fixed-duration
wait with a predicate-based waiting mechanism that checks for an actual
readiness condition or completion signal. Instead of sleeping for a fixed 0.05
seconds, implement a wait that polls or observes for the actual completion of
the async operation you are testing (such as a delegate callback, published
property, or completion flag), allowing the test to proceed immediately once the
expected state is reached rather than always waiting the full duration.
---
Outside diff comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 4072-4077: The test code contains an undeclared reference to
contentView on the line calling contentView.addSubview(field) in the
OmnibarNativeTextField setup block. Replace the contentView.addSubview(field)
call with the attachTestResponder helper function that is already being used
elsewhere in this test file, passing the field object as the appropriate
parameter to properly attach the test responder instead of relying on the
undefined contentView variable.
---
Duplicate comments:
In `@Sources/AppDelegate`+ShortcutRoutingWindow.swift:
- Around line 50-52: The code iterates directly over mainWindowContexts.values
while calling resolvedWindow(for:), which may mutate the mainWindowContexts
dictionary during enumeration and cause a runtime crash. Fix this by creating a
snapshot of the dictionary values before iterating, similar to the pattern used
in liveMainWindowContext on line 73. Wrap mainWindowContexts.values with Array()
to create an immutable copy before calling first on it, ensuring the dictionary
cannot be mutated during the enumeration with resolvedWindow(for:).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3b4f191-11f9-44c2-9475-1931065e9e83
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
Sources/AppDelegate+ShortcutRoutingTesting.swiftSources/AppDelegate+ShortcutRoutingWindow.swiftSources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingRepairProbe.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| hostedView.setVisibleInUI(true) | ||
| hostedView.setActive(true) | ||
| hostedView.moveFocus() | ||
| RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05)) |
There was a problem hiding this comment.
Replace fixed run-loop sleep with predicate-based waiting.
Line 21 uses a fixed wall-clock wait, which is exactly the flaky sleep-then-assert pattern the test guidelines prohibit. Wait on a real readiness predicate instead.
Suggested change
- RunLoop.main.run(until: Date(timeIntervalSinceNow: 0.05))
- XCTAssertTrue(
- hostedView.isSurfaceViewFirstResponder(),
- "Expected terminal surface to own first responder before repair test"
- )
+ XCTAssertTrue(
+ waitUntil(timeout: 1.0) {
+ hostedView.isSurfaceViewFirstResponder()
+ },
+ "Expected terminal surface to own first responder before repair test"
+ )As per coding guidelines, tests must not use fixed sleep-style waits for async readiness and should assert causality via real completion/predicate signals.
🤖 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 `@cmuxTests/AppDelegateShortcutRoutingRepairProbe.swift` at line 21, The
current implementation uses RunLoop.main.run(until: Date(timeIntervalSinceNow:
0.05)) which is a fixed wall-clock sleep pattern that violates test guidelines
and can cause flaky tests. Replace this fixed-duration wait with a
predicate-based waiting mechanism that checks for an actual readiness condition
or completion signal. Instead of sleeping for a fixed 0.05 seconds, implement a
wait that polls or observes for the actual completion of the async operation you
are testing (such as a delegate callback, published property, or completion
flag), allowing the test to proceed immediately once the expected state is
reached rather than always waiting the full duration.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
4139-4155:⚠️ Potential issue | 🔴 CriticalReplace out-of-scope
contentViewreference that causes compilation error.Line 4154 uses
contentView.addSubview(field), but this function does not definecontentViewlocally. All adjacent omnibar tests in this file correctly use theattachTestResponder(_:to:)helper. This inconsistency will cause a compilation failure.Proposed fix
- contentView.addSubview(field) + attachTestResponder(field, to: window)🤖 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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 4139 - 4155, The line referencing contentView.addSubview(field) uses an undefined variable contentView that is not in scope, causing a compilation error. Replace this line with a call to the attachTestResponder(_:to:) helper function, passing field and the appropriate target (the browserPanel), which is the consistent pattern used by other omnibar tests in this file.
🤖 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.
Outside diff comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 4139-4155: The line referencing contentView.addSubview(field) uses
an undefined variable contentView that is not in scope, causing a compilation
error. Replace this line with a call to the attachTestResponder(_:to:) helper
function, passing field and the appropriate target (the browserPanel), which is
the consistent pattern used by other omnibar tests in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66a000f4-ad27-48c6-9f03-02ad2c94b507
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift
…-focus # Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
This branch was successfully deployed
1 participant
Summary
Testing
Related
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stabilizes shortcut routing with DEBUG focus/responder overrides and routes all shortcuts through new
shortcutRouting*helpers. Restores deterministic focused-terminal key-repair coverage and fixes test responder attachments; tests stay on the hosted GUI runner.Refactors
shortcutRoutingKeyWindow,shortcutRoutingActiveWindow, andshortcutRoutingFirstResponderand route shortcut commands through them.NSWindow.makeKeyAndOrderFrontswizzle seam.debugFocusedTerminalKeyRepairObserverForTesting; expose seams for routing checks.Tests
TextBoxInputTextView.flushPendingSessionDraftAttachmentCopies.Written for commit 1159deb. Summary will update on new commits.
Summary by CodeRabbit