Fix shortcut recorder capture for Cmd-D remaps - #3398
Conversation
The shortcut settings recorder needs to receive Cmd+D even when the same keystroke is currently installed as a menu key equivalent for split actions. This adds the AppKit event-boundary regression before changing routing code so CI can show the failure without the fix. Constraint: Local tests are intentionally not run in this task per operator instruction Confidence: high Scope-risk: narrow Tested: Not run locally Not-tested: CI execution of the new regression
Shortcut recording must own the next key event even when that keystroke is currently registered as a SwiftUI or AppKit menu equivalent. The recorder now keeps a weak active-capture target, App-level and Window-level event routing dispatch capture candidates to it before menu fallback, and menu shortcut builders expose no key equivalents while recording is active. Constraint: Cmd+D and Cmd+Shift+D are split menu defaults and may also exist as stale menu equivalents after user remapping Rejected: Only suppress stale default shortcuts | current menu bindings also block recording and conflict feedback Confidence: high Scope-risk: moderate Tested: git diff --check Not-tested: Local XCTest/build run per instruction; CI should run the regression
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughKeyboard shortcut handling was changed so active shortcut recorders intercept key events early; menu-displayed shortcuts are suppressed during recording via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as NSApplication
participant Router as ShortcutRecorderEventRouter
participant Recorder as ShortcutRecorderNSButton
participant Menu as Menu Handler
User->>App: Press Cmd+D
App->>Router: dispatchActiveRecordingEvent(event, preferredWindow)
alt Router finds active recorder
Router->>Recorder: handleRecordingEvent(event)
Recorder->>Recorder: record/consume shortcut
Recorder-->>Router: true (consumed)
Router-->>App: true
App->>App: stop propagation (do not route to menu)
else No active recorder found
Router-->>App: false
App->>Menu: route event to menu equivalent
Menu->>Menu: perform action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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
🧹 Nitpick comments (1)
Sources/AppDelegate.swift (1)
10017-10022: ⚡ Quick winLog recorder-intercepted key events before consuming them.
This early return skips the existing monitor/AppKit key logging, so recorder-owned events disappear from the unified debug event log. Please add a
#if DEBUGcmuxDebugLog(...)here and mirror it in the matching early-return branches at Line 13518 and Line 13777.As per coding guidelines "All debug events (keys, mouse, focus, splits, tabs) must be logged to the unified debug event log in DEBUG builds."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 10017 - 10022, The early return inside the ShortcutRecorderEventRouter.dispatchActiveRecordingEvent(...) branch prevents debug logging of recorder-owned key events; add a DEBUG-only log call before the return: wrap a cmuxDebugLog(...) call in `#if` DEBUG / `#endif` and log the event (same format used by existing key-event logs) immediately before the `return nil` in the dispatchActiveRecordingEvent branch, and apply identical DEBUG-only cmuxDebugLog(...) additions to the two other early-return branches that short-circuit recorder events (the matching branches referenced in the review) so recorder-intercepted events still appear in the unified debug event log.
🤖 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 10017-10022: The DEBUG shortcut-monitor handler
debugHandleShortcutMonitorEvent(event:) bypasses the new pre-route and calls
handleCustomShortcut directly; update debugHandleShortcutMonitorEvent(event:) to
first call ShortcutRecorderEventRouter.dispatchActiveRecordingEvent(event,
preferredWindow: event.window ?? NSApp.keyWindow ?? NSApp.mainWindow) and return
early if it returns true, mirroring the production pre-route, before falling
back to handleCustomShortcut so socket/driven simulations exercise the
active-recorder path.
---
Nitpick comments:
In `@Sources/AppDelegate.swift`:
- Around line 10017-10022: The early return inside the
ShortcutRecorderEventRouter.dispatchActiveRecordingEvent(...) branch prevents
debug logging of recorder-owned key events; add a DEBUG-only log call before the
return: wrap a cmuxDebugLog(...) call in `#if` DEBUG / `#endif` and log the event
(same format used by existing key-event logs) immediately before the `return
nil` in the dispatchActiveRecordingEvent branch, and apply identical DEBUG-only
cmuxDebugLog(...) additions to the two other early-return branches that
short-circuit recorder events (the matching branches referenced in the review)
so recorder-intercepted events still appear in the unified debug event log.
🪄 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: f712ca77-4719-4f49-9b16-1943d9b6fbc2
📒 Files selected for processing (8)
Sources/App/MenuBarExtraController.swiftSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutRecorder.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
| if ShortcutRecorderEventRouter.dispatchActiveRecordingEvent( | ||
| event, | ||
| preferredWindow: event.window ?? NSApp.keyWindow ?? NSApp.mainWindow | ||
| ) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Keep the DEBUG shortcut-monitor hook aligned with this new pre-route.
debugHandleShortcutMonitorEvent(event:) at Line 12101 still jumps straight to handleCustomShortcut, so DEBUG/socket-driven simulations no longer exercise the active-recorder path that production now takes.
🔧 Suggested follow-up
func debugHandleShortcutMonitorEvent(event: NSEvent) -> Bool {
if event.type == .systemDefined {
return false
}
+ if ShortcutRecorderEventRouter.dispatchActiveRecordingEvent(
+ event,
+ preferredWindow: event.window ?? NSApp.keyWindow ?? NSApp.mainWindow
+ ) {
+ return true
+ }
if event.type == .keyDown {
return handleCustomShortcut(event: event)
}
handleBrowserOmnibarSelectionRepeatLifecycleEvent(event)
return clearEscapeSuppressionForKeyUp(event: event, consumeIfSuppressed: true)📝 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.
| if ShortcutRecorderEventRouter.dispatchActiveRecordingEvent( | |
| event, | |
| preferredWindow: event.window ?? NSApp.keyWindow ?? NSApp.mainWindow | |
| ) { | |
| return nil | |
| } | |
| func debugHandleShortcutMonitorEvent(event: NSEvent) -> Bool { | |
| if event.type == .systemDefined { | |
| return false | |
| } | |
| if ShortcutRecorderEventRouter.dispatchActiveRecordingEvent( | |
| event, | |
| preferredWindow: event.window ?? NSApp.keyWindow ?? NSApp.mainWindow | |
| ) { | |
| return true | |
| } | |
| if event.type == .keyDown { | |
| return handleCustomShortcut(event: event) | |
| } | |
| handleBrowserOmnibarSelectionRepeatLifecycleEvent(event) | |
| return clearEscapeSuppressionForKeyUp(event: event, consumeIfSuppressed: true) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 10017 - 10022, The DEBUG
shortcut-monitor handler debugHandleShortcutMonitorEvent(event:) bypasses the
new pre-route and calls handleCustomShortcut directly; update
debugHandleShortcutMonitorEvent(event:) to first call
ShortcutRecorderEventRouter.dispatchActiveRecordingEvent(event, preferredWindow:
event.window ?? NSApp.keyWindow ?? NSApp.mainWindow) and return early if it
returns true, mirroring the production pre-route, before falling back to
handleCustomShortcut so socket/driven simulations exercise the active-recorder
path.
Greptile SummaryThis PR fixes a bug where pressing Cmd+D (or any key mapped to both a user-configured shortcut and a menu key equivalent) while a shortcut recorder is active would trigger the menu action instead of being captured by the recorder. The fix has two complementary layers: Confidence Score: 4/5Safe to merge; only P2 style findings with no correctness impact on the core fix. The event-routing logic is sound: the three-layer Sources/KeyboardShortcutRecorder.swift (consumeRecordingEvent invariant) and cmuxTests/AppDelegateShortcutRoutingTests.swift (#else XCTFail) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NSApp as NSApplication.sendEvent (override)
participant AppMon as AppDelegate event monitor
participant WinSend as NSWindow.sendEvent (override)
participant Menu as NSMenu (key equivalents)
participant Recorder as ShortcutRecorderNSButton
Note over Recorder: User clicks recorder → startRecording()<br/>activeRecorder = self<br/>isAnyRecorderActive = true<br/>menuShortcut → .unbound (menus rebuilt)
User->>NSApp: keyDown Cmd+D
NSApp->>NSApp: dispatchActiveRecordingEvent?
alt recorder active
NSApp->>Recorder: consumeRecordingEvent(event)
Recorder->>Recorder: handleRecordingEvent → accept shortcut
Recorder->>Recorder: stopRecording() → activeRecorder = nil, isAnyRecorderActive = false
Recorder-->>NSApp: true (consumed)
NSApp-->>User: return (super never called, menu never fires)
else recorder not active
NSApp->>NSApp: super.sendEvent
NSApp->>AppMon: local monitor fires
AppMon->>AppMon: dispatchActiveRecordingEvent → false
AppMon-->>NSApp: pass event through
NSApp->>WinSend: route to window
WinSend->>WinSend: dispatchActiveRecordingEvent → false
WinSend->>Menu: performKeyEquivalent
Menu-->>User: menu action fires
end
Reviews (1): Last reviewed commit: "Let shortcut recording preempt menu equi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| guard let event = makeKeyDownEvent( | ||
| key: "d", |
There was a problem hiding this comment.
#else branch unconditionally fails the test
The #else arm calls XCTFail(...), which means running this test in a non-DEBUG configuration (e.g., a Release test scheme) will always report a hard failure instead of skipping. Using throw XCTSkip would be safer:
| guard let event = makeKeyDownEvent( | |
| key: "d", | |
| #else | |
| throw XCTSkip("Shortcut recorder debug hooks are only available in DEBUG") | |
| #endif |
| private func consumeRecordingEvent(_ event: NSEvent) -> Bool { | ||
| guard isRecording else { return false } | ||
| _ = handleRecordingEvent(event) | ||
| return true | ||
| } |
There was a problem hiding this comment.
consumeRecordingEvent silently discards handleRecordingEvent's return value
handleRecordingEvent currently always returns nil (it consumes every event unconditionally), so returning true here is correct. However the method contract is not enforced: if handleRecordingEvent ever returned a non-nil event in a future branch, consumeRecordingEvent would still return true, incorrectly telling callers the event was consumed when it wasn't. A short comment or assertion would make the invariant explicit:
| private func consumeRecordingEvent(_ event: NSEvent) -> Bool { | |
| guard isRecording else { return false } | |
| _ = handleRecordingEvent(event) | |
| return true | |
| } | |
| private func consumeRecordingEvent(_ event: NSEvent) -> Bool { | |
| guard isRecording else { return false } | |
| let unconsumed = handleRecordingEvent(event) | |
| // handleRecordingEvent always returns nil (consumes every event). | |
| assert(unconsumed == nil, "Expected handleRecordingEvent to consume the event") | |
| return true | |
| } |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 8 files
You’re at about 99% of the daily review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Absorb growth from AppDelegate event routing changes and the AppDelegateShortcutRoutingTests regression covering Cmd+D recording while a matching menu equivalent is installed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Resolve Swift file length budget by regenerating from current actuals; both branches grew separate files so the union must reflect both. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Closes #3397
Summary
Testing
./scripts/reload.sh --tag issue-3397): Cmd+D is captured by the shortcut recorder while the matching menu equivalent is installed, and the menu action does not fire during recording.git diff --checkNote
Medium Risk
Changes keyboard event routing and menu key-equivalent installation while shortcut recording is active, which can affect command handling across the app; scope is contained to shortcut recording/menu integration.
Overview
Fixes a regression where the shortcut recorder could fail to capture keystrokes (notably
Cmd+D) when a matching menu key equivalent was installed.This routes key events to an active
ShortcutRecorderNSButtonearly inAppDelegate(local monitor,NSApplication.sendEvent, and window key-equivalent handling), and suppresses menu key equivalents during recording via a newKeyboardShortcutSettings.menuShortcut(for:)API.Adds a DEBUG-only regression test ensuring
Cmd+Dis recorded and does not trigger the menu item action while recording.Reviewed by Cursor Bugbot for commit 241a03b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests