Add a system-wide hotkey to show and hide cmux windows - #2389
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to 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:
📝 WalkthroughWalkthroughAdds a configurable system-wide hotkey: persisted settings, Carbon-based global hotkey registration and handling, layout-aware shortcut matching and recording state propagation, Settings UI for enable/configure, AppDelegate/controller wiring, and unit tests for matching. Changes
Sequence DiagramssequenceDiagram
participant User
participant Carbon as Carbon/kEventHotKeyPressed
participant System as SystemWideHotkeyController
participant UD as UserDefaults
participant App
User->>Carbon: Press configured global hotkey
Carbon->>System: HotKey event delivered
System->>UD: Read enabled state & shortcut
System->>System: Match event vs stored shortcut (layout-aware)
alt App visible
System->>App: Request hide all windows
App->>App: Hide/deactivate windows
else App hidden
System->>App: Unhide/reveal windows, activate app
App->>App: Focus preferred window
end
sequenceDiagram
participant User
participant Settings as GlobalHotkeySection
participant UD as UserDefaults
participant System as SystemWideHotkeyController
participant A11y as Accessibility (AXIsProcessTrusted)
User->>Settings: Toggle enable or record shortcut
Settings->>UD: Persist enabled state / shortcut
Settings->>System: Notify enabled/shortcut/recording-active
System->>A11y: Check Accessibility permission (if applicable)
alt Not trusted
A11y-->>Settings: Not trusted (show grant-access state)
else Trusted
System->>System: Register or unregister Carbon hotkey
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. Comment |
![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 4 files
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/KeyboardShortcutSettings.swift">
<violation number="1" location="Sources/KeyboardShortcutSettings.swift:538">
P1: Global hotkey matching now short-circuits on a single hardcoded keycode, which breaks logical-key matching (for example keypad Enter and layout variants).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR adds a system-wide hotkey feature to cmux that mirrors iTerm2's app-hotkey behavior: a configurable Key observations:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant EventTap as CGEvent Tap (Main RunLoop)
participant Controller as SystemWideHotkeyController
participant NSApp
participant Settings as GlobalHotkeySection (SwiftUI)
participant Defaults as UserDefaults
User->>Settings: Toggle "Enable System-Wide Hotkey"
Settings->>Defaults: @AppStorage write (enabledKey)
Settings->>Defaults: setEnabled() [redundant write]
Defaults-->>Controller: didChangeNotification → refreshRegistration()
Controller->>Controller: installEventTapIfNeeded()
Note over Controller: CGEvent.tapCreate (headInsert, defaultTap)
User->>EventTap: Key press (any app)
EventTap->>Controller: handleEventTap(type, event)
Controller->>Controller: matchesShortcut(event)?
alt Matches hotkey & not recording
Controller->>NSApp: DispatchQueue.main.async → toggleApplicationVisibility()
EventTap-->>User: return nil (event consumed)
alt NSApp.isActive
NSApp-->>User: NSApp.hide(nil)
else NSApp inactive
NSApp-->>User: unhide + activate + orderFront
end
else No match
EventTap-->>User: return event (pass through)
end
User->>Settings: Click "Grant Access…"
Settings->>Controller: requestAccessibilityAccess()
Controller->>Controller: AXIsProcessTrustedWithOptions(prompt:true)
Controller->>Controller: refreshRegistration()
Reviews (1): Last reviewed commit: "Add system-wide app hotkey" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Binding( | ||
| get: { isEnabled }, | ||
| set: { newValue in | ||
| isEnabled = newValue | ||
| SystemWideHotkeySettings.setEnabled(newValue) | ||
| guard newValue else { return } | ||
| let trusted = SystemWideHotkeyController.shared.requestAccessibilityAccess() | ||
| accessibilityTrusted = trusted | ||
| if !trusted { | ||
| SystemWideHotkeySettings.openAccessibilitySettings() |
There was a problem hiding this comment.
enabledBinding
isEnabled = newValue already writes to UserDefaults.standard via @AppStorage, which also fires UserDefaults.didChangeNotification. The subsequent SystemWideHotkeySettings.setEnabled(newValue) call writes the same key a second time, triggering a second notification and causing syncFromDefaults() (in this view) and refreshRegistration (in the controller) to both run twice unnecessarily.
The setEnabled call can be removed — @AppStorage is sufficient:
| Binding( | |
| get: { isEnabled }, | |
| set: { newValue in | |
| isEnabled = newValue | |
| SystemWideHotkeySettings.setEnabled(newValue) | |
| guard newValue else { return } | |
| let trusted = SystemWideHotkeyController.shared.requestAccessibilityAccess() | |
| accessibilityTrusted = trusted | |
| if !trusted { | |
| SystemWideHotkeySettings.openAccessibilitySettings() | |
| set: { newValue in | |
| isEnabled = newValue | |
| guard newValue else { return } | |
| let trusted = SystemWideHotkeyController.shared.requestAccessibilityAccess() | |
| accessibilityTrusted = trusted | |
| if !trusted { | |
| SystemWideHotkeySettings.openAccessibilitySettings() | |
| } | |
| } |
| private func showAllApplicationWindows() { | ||
| NSApp.unhide(nil) | ||
|
|
||
| let windowsToReveal = NSApp.windows.filter { $0.isVisible || $0.isMiniaturized } | ||
| for window in windowsToReveal where window.isMiniaturized { | ||
| window.deminiaturize(nil) | ||
| } | ||
|
|
||
| NSRunningApplication.current.activate(options: [.activateAllWindows, .activateIgnoringOtherApps]) | ||
|
|
||
| let focusWindow = preferredFocusWindow(from: windowsToReveal) | ||
| focusWindow?.orderFrontRegardless() | ||
| focusWindow?.makeKeyAndOrderFront(nil) | ||
|
|
||
| for window in windowsToReveal where window !== focusWindow { | ||
| window.orderFrontRegardless() | ||
| } |
There was a problem hiding this comment.
windowsToReveal may be empty when the app was hidden
NSApp.unhide(nil) restores windows from an app-level hide. On the main thread this is synchronous, so isVisible should return true immediately after — however, the filter $0.isVisible || $0.isMiniaturized only captures windows in those two states. If every window was hidden by the OS (app hidden via NSApp.hide) AND unhide hasn't yet updated isVisible in the same synchronous call, windowsToReveal comes back empty, focusWindow is nil, and the makeKeyAndOrderFront/orderFrontRegardless loop is skipped entirely. The activate call above would still bring the app forward, but no specific window would receive key focus.
A safer pattern is to snapshot NSApp.windows before unhide (while all windows are known app-members, even if hidden) and operate on that full list:
private func showAllApplicationWindows() {
let allWindows = NSApp.windows // snapshot before any state changes
NSApp.unhide(nil)
let windowsToReveal = allWindows.filter { $0.isVisible || $0.isMiniaturized }
for window in windowsToReveal where window.isMiniaturized {
window.deminiaturize(nil)
}
// … rest unchanged
}This also avoids re-querying NSApp.windows (which may include new windows created during the unhide call path).
| private func installEventTapIfNeeded() { | ||
| guard eventTap == nil else { return } | ||
|
|
||
| let eventMask = CGEventMask(1) << CGEventType.keyDown.rawValue |
There was a problem hiding this comment.
CGEventMask shift operand type mismatch (style)
CGEventMask(1) << CGEventType.keyDown.rawValue mixes a UInt64 lhs with a UInt32 rhs. Swift allows this, but the canonical Apple pattern keeps the bit-shift inside the cast to make the intent explicit and avoid potential issues if rawValue is ever widened:
| let eventMask = CGEventMask(1) << CGEventType.keyDown.rawValue | |
| let eventMask = CGEventMask(1 << CGEventType.keyDown.rawValue) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| let focusWindow = preferredFocusWindow(from: windowsToReveal) | ||
| focusWindow?.orderFrontRegardless() |
There was a problem hiding this comment.
orderFrontRegardless before makeKeyAndOrderFront
makeKeyAndOrderFront(_:) already brings the window to front and makes it the key window, so the preceding orderFrontRegardless() on focusWindow is a no-op.
| let focusWindow = preferredFocusWindow(from: windowsToReveal) | |
| focusWindow?.orderFrontRegardless() | |
| focusWindow?.makeKeyAndOrderFront(nil) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/cmuxApp.swift`:
- Around line 6632-6642: The enableSubtitle computed property currently returns
the "works from any app" text whenever isEnabled is true even if
accessibilityTrusted is false; change enableSubtitle (used for the global hotkey
row) to check both isEnabled and accessibilityTrusted and only return the
"subtitleOn" localized string when accessibilityTrusted is true, otherwise
return the "subtitleOff" or a new intermediate key indicating that Accessibility
permission is required; update Resources/Localizable.xcstrings to add the new
localization key referenced by enableSubtitle so the string resolves correctly.
- Around line 6623-6627: The app updates accessibilityTrusted on app activation
but doesn’t notify SystemWideHotkeyController to re-arm its event tap; in
didBecomeActive (handler around didBecomeActive) call
SystemWideHotkeyController.shared.refreshRegistration() (or
SystemWideHotkeyController.shared.requestAccessibilityAccess() and then
refreshRegistration() if needed) when accessibilityTrusted becomes true so the
controller re-installs the event tap and syncs its state with the new
accessibility permission; update the didBecomeActive handler to detect the
granted state and invoke refreshRegistration() on
SystemWideHotkeyController.shared.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 530-542: The matching logic reconstructs a US/ANSI keycode from
the CGEvent and compares it to shortcut.expectedKeyCode, which breaks non‑US
layouts; instead always construct an NSEvent from the CGEvent and call
shortcut.matches(event:) so recorded symbols are matched the same way they were
recorded. Update matchesShortcut(_:) to stop deriving/using keyCode for
comparisons (remove the expectedKeyCode branch) and use NSEvent(cgEvent:) +
shortcut.matches(event:) after normalizing modifiers
(StoredShortcut.normalizedModifierFlags and shortcut.modifierFlags) to perform
the match.
- Around line 545-552: The current toggleApplicationVisibility uses
NSApp.isActive to decide hide vs show which fails when the app is frontmost but
all windows are miniaturized/hidden; update toggleApplicationVisibility to
inspect actual window visibility instead: query NSApp.windows (or relevant
window collection) and determine if any window is currently
visible/non-miniaturized, and only call NSApp.hide(nil) when the app has at
least one visible window; otherwise call showAllApplicationWindows() — change
the conditional that references NSApp.isActive to check window visibility (e.g.,
any { $0.isVisible && !$0.isMiniaturized }) and keep showAllApplicationWindows
and NSApp.hide(nil) as the two branches.
- Around line 361-375: isValid currently only ensures a primary modifier and
allows bindings that collide with existing cmux shortcuts; update isValid(_:) to
reject shortcuts that match any existing KeyboardShortcutSettings shortcuts and
known hard-coded handlers (e.g., the Cmd+Option+T used by
AppDelegate.handleCustomShortcut(event:)), so normalizedRecordedShortcut(_:)
will return nil for collisions and setShortcut(_:, defaults:) will not store
them; specifically, add a collision check in isValid that compares the candidate
StoredShortcut against the app's current KeyboardShortcutSettings (all defined
actions) and against a small list of hard-coded forbidden shortcuts (including
Cmd+Option+T), and ensure KeyboardShortcutSettings.Action.toggleTextBoxInput
keeps its default Cmd+Option+B to avoid that collision.
🪄 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: 2bb1a932-0ff2-452c-a606-f74197345043
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swift
| let trusted = SystemWideHotkeyController.shared.requestAccessibilityAccess() | ||
| accessibilityTrusted = trusted | ||
| if !trusted { | ||
| SystemWideHotkeySettings.openAccessibilitySettings() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'SystemWideHotkey.*\.swift' Sources -x sed -n '1,220p' {}
rg -n -C3 'AXIsProcessTrusted|requestAccessibilityAccess|didBecomeActive|CGEvent\.tapCreate|eventTap|start\(|didChangeNotification|setShortcutRecordingActive' SourcesRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
rg -n -A 30 'func refreshRegistration' Sources/KeyboardShortcutSettings.swiftRepository: manaflow-ai/cmux
Length of output: 1193
Notify SystemWideHotkeyController when accessibility access is granted after app regains focus.
When a user grants Accessibility access via System Settings and returns to the app, the didBecomeActive handler (line 6718–6720) only refreshes the local accessibilityTrusted flag. It does not re-arm the event tap in SystemWideHotkeyController. If the user disabled and re-enabled the toggle at lines 6623–6627, or if they granted access outside the app, the controller's refreshRegistration() must be called to reinstall the event tap; otherwise the hotkey remains inactive until restart or another settings change.
Call SystemWideHotkeyController.shared.requestAccessibilityAccess() or refreshRegistration() on didBecomeActive to re-sync controller state with the current accessibility trust status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 6623 - 6627, The app updates
accessibilityTrusted on app activation but doesn’t notify
SystemWideHotkeyController to re-arm its event tap; in didBecomeActive (handler
around didBecomeActive) call
SystemWideHotkeyController.shared.refreshRegistration() (or
SystemWideHotkeyController.shared.requestAccessibilityAccess() and then
refreshRegistration() if needed) when accessibilityTrusted becomes true so the
controller re-installs the event tap and syncs its state with the new
accessibility permission; update the didBecomeActive handler to detect the
granted state and invoke refreshRegistration() on
SystemWideHotkeyController.shared.
| private var enableSubtitle: String { | ||
| if isEnabled { | ||
| return String( | ||
| localized: "settings.globalHotkey.enable.subtitleOn", | ||
| defaultValue: "Press the shortcut from any app to show or hide all cmux windows." | ||
| ) | ||
| } | ||
| return String( | ||
| localized: "settings.globalHotkey.enable.subtitleOff", | ||
| defaultValue: "Turn this on to show or hide all cmux windows from any app." | ||
| ) |
There was a problem hiding this comment.
Don’t show the “works from any app” subtitle until permission is actually granted.
With isEnabled == true and accessibilityTrusted == false, this row says the shortcut works globally even though the next row still says Accessibility access is required. That makes the section’s primary status misleading.
Suggested tweak
private var enableSubtitle: String {
- if isEnabled {
+ if isEnabled && accessibilityTrusted {
return String(
localized: "settings.globalHotkey.enable.subtitleOn",
defaultValue: "Press the shortcut from any app to show or hide all cmux windows."
)
}
+ if isEnabled {
+ return String(
+ localized: "settings.globalHotkey.enable.subtitlePendingAccess",
+ defaultValue: "Grant Accessibility access to use this shortcut from other apps."
+ )
+ }
return String(
localized: "settings.globalHotkey.enable.subtitleOff",
defaultValue: "Turn this on to show or hide all cmux windows from any app."
)
}This also needs a matching key in Resources/Localizable.xcstrings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 6632 - 6642, The enableSubtitle computed
property currently returns the "works from any app" text whenever isEnabled is
true even if accessibilityTrusted is false; change enableSubtitle (used for the
global hotkey row) to check both isEnabled and accessibilityTrusted and only
return the "subtitleOn" localized string when accessibilityTrusted is true,
otherwise return the "subtitleOff" or a new intermediate key indicating that
Accessibility permission is required; update Resources/Localizable.xcstrings to
add the new localization key referenced by enableSubtitle so the string resolves
correctly.
The leftover working-tree changes are a real follow-up to the app hotkey feature, not stale WIP. They move shortcut matching into StoredShortcut so the in-app shortcut handler and the CGEvent-based system-wide hotkey path use the same caps-lock normalization, keypad enter handling, remapped command-letter behavior, and non-Latin layout fallback. The added unit coverage documents those cases.
…yboard settings conflicts Preserve main's chord-capable shortcut settings model, managed-in-settings-file messaging, and newer recorder behavior while keeping the system-wide hotkey controller, settings UI strings, and the shared single-stroke matching path needed by the global hotkey and its tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (2)
6722-6733:⚠️ Potential issue | 🟡 MinorEnabled subtitle is misleading before Accessibility is granted.
On Line 6723,
enableSubtitleshows the “works from any app” copy wheneverisEnabledis true, even whenaccessibilityTrustedis false.Suggested fix
private var enableSubtitle: String { - if isEnabled { + if isEnabled && accessibilityTrusted { return String( localized: "settings.globalHotkey.enable.subtitleOn", defaultValue: "Press the shortcut from any app to show or hide all cmux windows." ) } + if isEnabled { + return String( + localized: "settings.globalHotkey.enable.subtitlePendingAccess", + defaultValue: "Grant Accessibility access to use this shortcut from other apps." + ) + } return String( localized: "settings.globalHotkey.enable.subtitleOff", defaultValue: "Turn this on to show or hide all cmux windows from any app." ) }Also add
settings.globalHotkey.enable.subtitlePendingAccesstoResources/Localizable.xcstrings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6722 - 6733, The enableSubtitle computed property currently returns the "works from any app" copy whenever isEnabled is true even if accessibility isn't granted; update enableSubtitle to check accessibilityTrusted and return a new "pending access" localized string (settings.globalHotkey.enable.subtitlePendingAccess) when isEnabled && !accessibilityTrusted, keep the existing "subtitleOn" for isEnabled && accessibilityTrusted, and the existing "subtitleOff" when !isEnabled; also add the new key settings.globalHotkey.enable.subtitlePendingAccess to Resources/Localizable.xcstrings with the appropriate copy.
6808-6810:⚠️ Potential issue | 🟠 MajorRe-activate should re-sync controller registration, not only local trust state.
Line 6808 updates
accessibilityTrustedbut does not explicitly re-sync the controller registration path after trust changes, so the global hotkey can stay inactive until another setting mutation.Suggested fix
.onReceive(NotificationCenter.default.publisher(for: NSApplication.didBecomeActiveNotification)) { _ in - accessibilityTrusted = SystemWideHotkeySettings.isAccessibilityTrusted() + accessibilityTrusted = SystemWideHotkeySettings.isAccessibilityTrusted() + if isEnabled { + SystemWideHotkeyController.shared.refreshRegistration() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6808 - 6810, The onReceive handler only updates accessibilityTrusted using SystemWideHotkeySettings.isAccessibilityTrusted() but does not re-sync or re-register the global hotkey controller; modify the handler that reacts to NSApplication.didBecomeActiveNotification (the closure updating accessibilityTrusted) to also call the controller re-registration routine (e.g., invoke the same method used elsewhere to register/unregister the SystemWideHotkey controller or a public method like syncSystemWideHotkeyRegistration() / registerSystemWideHotkeyController()) so that when trust changes the app explicitly re-checks and (un)registers the global hotkey immediately rather than waiting for another setting mutation. Ensure you reference and call the existing registration/unregistration functions used for SystemWideHotkeySettings so logic is reused rather than duplicated.
🤖 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`:
- Line 2594: The code starts SystemWideHotkeyController directly
(SystemWideHotkeyController.shared.start()), which creates a separate
persistence path; instead model the global hotkey as a
KeyboardShortcutSettings.Action and have the hotkey controller consume that
value. Change the flow so the global shortcut is declared/registered in
KeyboardShortcutSettings (add a new Action enum case for the cmux-owned global
hotkey), ensure KeyboardShortcutSettings.setShortcut(...) treats file-managed
shortcuts as no-ops as before, and update SystemWideHotkeyController to read the
shortcut from KeyboardShortcutSettings (e.g., observe the
KeyboardShortcutSettings.Action value) rather than using
SystemWideHotkeySettings; remove direct calls to
SystemWideHotkeyController.shared.start() and wire controller lifecycle to the
KeyboardShortcutSettings-backed value.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 6722-6733: The enableSubtitle computed property currently returns
the "works from any app" copy whenever isEnabled is true even if accessibility
isn't granted; update enableSubtitle to check accessibilityTrusted and return a
new "pending access" localized string
(settings.globalHotkey.enable.subtitlePendingAccess) when isEnabled &&
!accessibilityTrusted, keep the existing "subtitleOn" for isEnabled &&
accessibilityTrusted, and the existing "subtitleOff" when !isEnabled; also add
the new key settings.globalHotkey.enable.subtitlePendingAccess to
Resources/Localizable.xcstrings with the appropriate copy.
- Around line 6808-6810: The onReceive handler only updates accessibilityTrusted
using SystemWideHotkeySettings.isAccessibilityTrusted() but does not re-sync or
re-register the global hotkey controller; modify the handler that reacts to
NSApplication.didBecomeActiveNotification (the closure updating
accessibilityTrusted) to also call the controller re-registration routine (e.g.,
invoke the same method used elsewhere to register/unregister the
SystemWideHotkey controller or a public method like
syncSystemWideHotkeyRegistration() / registerSystemWideHotkeyController()) so
that when trust changes the app explicitly re-checks and (un)registers the
global hotkey immediately rather than waiting for another setting mutation.
Ensure you reference and call the existing registration/unregistration functions
used for SystemWideHotkeySettings so logic is reused rather than duplicated.
🪄 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: 13cb0353-437a-4b7a-9dcc-043d04c9fe3b
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Resources/Localizable.xcstrings
- Sources/KeyboardShortcutSettings.swift
| installBrowserAddressBarFocusObservers() | ||
| installShortcutMonitor() | ||
| installShortcutDefaultsObserver() | ||
| SystemWideHotkeyController.shared.start() |
There was a problem hiding this comment.
Keep the global hotkey inside KeyboardShortcutSettings.
Starting a separate SystemWideHotkeyController/SystemWideHotkeySettings path gives this cmux-owned shortcut its own persistence source of truth instead of KeyboardShortcutSettings, so settings.json management and the existing file-managed no-op semantics won’t apply. Please model the global hotkey as a KeyboardShortcutSettings.Action and have the controller consume that value.
Based on learnings: Every new cmux-owned keyboard shortcut must be added to KeyboardShortcutSettings and supported in ~/.config/cmux/settings.json; file-managed shortcuts must remain a no-op in KeyboardShortcutSettings.setShortcut(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` at line 2594, The code starts
SystemWideHotkeyController directly (SystemWideHotkeyController.shared.start()),
which creates a separate persistence path; instead model the global hotkey as a
KeyboardShortcutSettings.Action and have the hotkey controller consume that
value. Change the flow so the global shortcut is declared/registered in
KeyboardShortcutSettings (add a new Action enum case for the cmux-owned global
hotkey), ensure KeyboardShortcutSettings.setShortcut(...) treats file-managed
shortcuts as no-ops as before, and update SystemWideHotkeyController to read the
shortcut from KeyboardShortcutSettings (e.g., observe the
KeyboardShortcutSettings.Action value) rather than using
SystemWideHotkeySettings; remove direct calls to
SystemWideHotkeyController.shared.start() and wire controller lifecycle to the
KeyboardShortcutSettings-backed value.
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
Sources/KeyboardShortcutSettings.swift (3)
620-627:⚠️ Potential issue | 🟠 MajorUse visible-window state to choose hide vs show.
If cmux is frontmost but every window is miniaturized or otherwise hidden,
NSApp.isActiveis still true and this path hides again instead of restoring the app. A quick repro is: miniaturize the last cmux window while cmux stays active, then press the hotkey.Minimal fix
private func toggleApplicationVisibility() { - if NSApp.isActive { + let hasVisibleWindow = NSApp.windows.contains { $0.isVisible && !$0.isMiniaturized } + if NSApp.isActive && hasVisibleWindow { NSApp.hide(nil) return } showAllApplicationWindows() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 620 - 627, toggleApplicationVisibility currently uses NSApp.isActive to decide hide vs show which fails when the app is frontmost but all windows are miniaturized/hidden; change the decision to inspect the app's windows (e.g. NSApp.windows or NSApp.orderedWindows) and check for any window that is visible and not miniaturized (use properties like isVisible and isMiniaturized) — if any such window exists, call NSApp.hide(nil), otherwise call showAllApplicationWindows(); keep the function name toggleApplicationVisibility and the existing showAllApplicationWindows call.
449-450:⚠️ Potential issue | 🟠 MajorReject bindings that collide with existing cmux shortcuts.
isValidcurrently accepts globals like⌘W,⌘⌥T, or⌘4. Once the system-wide hotkey owns one of those, the original cmux shortcut becomes unreachable. Please validate against the effectiveKeyboardShortcutSettings.Actionbindings, including numbered-digit actions, plus the hard-coded handlers inAppDelegate.handleCustomShortcut(event:).Based on learnings,
KeyboardShortcutSettings.Action.toggleTextBoxInputdefaults to Cmd+Option+B to avoid conflict with the existing hard-coded Cmd+Option+T binding inAppDelegate.handleCustomShortcut(event:).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 449 - 450, isValid currently only checks modifiers/chords; update it to reject any StoredShortcut that collides with existing cmux shortcuts by comparing against the effective bindings from KeyboardShortcutSettings.Action (including the numbered-digit actions) and the hard-coded handlers in AppDelegate.handleCustomShortcut(event:). Modify isValid( _ shortcut: StoredShortcut) to build a set of currently reserved shortcuts by iterating KeyboardShortcutSettings.Action.allCases (and mapping any digit-based actions to their digit keys) and by including the specific hard-coded shortcuts checked in AppDelegate.handleCustomShortcut(event:), then return false if shortcut matches any reserved entry (for example conflicts with KeyboardShortcutSettings.Action.toggleTextBoxInput default Cmd+Option+B or any Cmd+W/Cmd+Option+T/Cmd+digit entries); otherwise preserve the existing hasChord/hasPrimaryModifier checks and return true.
437-450:⚠️ Potential issue | 🟠 MajorPersist the physical key code for system-wide bindings.
StoredShortcutonly saveskey, thencarbonHotKeyRegistrationrebuilds an ANSI key code from that glyph. That loses real key identity: keypad digits get re-registered as number-row digits,"\r"is recordable but never maps back to any Carbon key code, and non-Latin letters can be saved without any registrable Carbon key. Please store the recordedkeyCodefor this feature, or reject anything that cannot round-trip into registration losslessly.Also applies to: 904-939, 997-1084
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 437 - 450, StoredShortcut currently persists only the printable key glyph which loses physical key identity; update persistence and validation so the recorded physical keyCode is saved and only round-tripable shortcuts are accepted. Modify setShortcut to encode and store the StoredShortcut.keyCode (not just key) and ensure StoredShortcut contains the recorded keyCode; update normalizedRecordedShortcut to reject shortcuts that cannot be losslessly mapped to a Carbon/registration key (use the same round-trip check used by carbonHotKeyRegistration or a new helper that converts recorded keyCode → Carbon key → back and verifies equality). Update isValid (and any similar validation sites referenced around 904-939 and 997-1084) to require !hasChord, hasPrimaryModifier, and that the shortcut is round-tripable into registration; apply the same keyCode persistence and validation wherever shortcuts are saved or normalized.
🤖 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/cmuxApp.swift`:
- Around line 6728-6768: The UI lacks an Accessibility permission status/action
row and doesn't update when macOS TCC changes; add a small status/action row in
the SettingsCard (near KeyboardShortcutRecorder) that reads
SystemWideHotkeyController.shared.accessibilityAuthorized (or calls
AXIsProcessTrusted()) and shows a button to open System Settings or to call
AXIsProcessTrustedWithOptions prompt if needed; update the SettingsCardNote text
to not claim “No extra macOS permission is required” when accessibility is
required; and instead of relying solely on UserDefaults.didChangeNotification,
subscribe to an accessibility-authorization change source (e.g., poll
AXIsProcessTrusted periodically or subscribe to a
DistributedNotificationCenter/AX notification if available) and call
syncFromDefaults() / update the enabledBinding/shortcut UI when authorization
changes so KeyboardShortcutRecorder and the toggle reflect the current TCC
state.
- Around line 6701-6757: The GlobalHotkeySection currently stores the shortcut
in a local State and uses SystemWideHotkeySettings, which bypasses the app-wide
KeyboardShortcutSettings source of truth; change this so the shortcut is read
from and written to KeyboardShortcutSettings (e.g., add a
KeyboardShortcutSettings.Action for the global-show/hide action), bind
KeyboardShortcutRecorder's shortcut to that KeyboardShortcutSettings value
instead of the local shortcut State, and keep SystemWideHotkeySettings only for
the separate enabled flag (update calls to
SystemWideHotkeySettings.setShortcut/setEnabled to use KeyboardShortcutSettings
APIs for the shortcut and preserve SystemWideHotkeySettings.setEnabled if still
needed). Ensure the new action is exposed in Settings and persists to the normal
settings.json/storage used by KeyboardShortcutSettings.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 414-443: You split the shortcut storage into a new
SystemWideHotkeySettings path which bypasses
settingsFileStore/isManagedBySettingsFile and creates a second persistence
source; revert this by modeling the shortcut as a
KeyboardShortcutSettings.Action and delegating persistence to
KeyboardShortcutSettings.setShortcut(...) (keeping the separate enabled key
allowed), remove use of SystemWideHotkeySettings.shortcut and the
"systemWideHotkey.shortcut" UserDefaults key, ensure any reads call
KeyboardShortcutSettings.action(for:) or KeyboardShortcutSettings.shortcut
accessor and any writes call KeyboardShortcutSettings.setShortcut(_:for:) so
settings.json remains the source of truth and the no-op behavior when
isManagedBySettingsFile is in effect is preserved.
---
Duplicate comments:
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 620-627: toggleApplicationVisibility currently uses NSApp.isActive
to decide hide vs show which fails when the app is frontmost but all windows are
miniaturized/hidden; change the decision to inspect the app's windows (e.g.
NSApp.windows or NSApp.orderedWindows) and check for any window that is visible
and not miniaturized (use properties like isVisible and isMiniaturized) — if any
such window exists, call NSApp.hide(nil), otherwise call
showAllApplicationWindows(); keep the function name toggleApplicationVisibility
and the existing showAllApplicationWindows call.
- Around line 449-450: isValid currently only checks modifiers/chords; update it
to reject any StoredShortcut that collides with existing cmux shortcuts by
comparing against the effective bindings from KeyboardShortcutSettings.Action
(including the numbered-digit actions) and the hard-coded handlers in
AppDelegate.handleCustomShortcut(event:). Modify isValid( _ shortcut:
StoredShortcut) to build a set of currently reserved shortcuts by iterating
KeyboardShortcutSettings.Action.allCases (and mapping any digit-based actions to
their digit keys) and by including the specific hard-coded shortcuts checked in
AppDelegate.handleCustomShortcut(event:), then return false if shortcut matches
any reserved entry (for example conflicts with
KeyboardShortcutSettings.Action.toggleTextBoxInput default Cmd+Option+B or any
Cmd+W/Cmd+Option+T/Cmd+digit entries); otherwise preserve the existing
hasChord/hasPrimaryModifier checks and return true.
- Around line 437-450: StoredShortcut currently persists only the printable key
glyph which loses physical key identity; update persistence and validation so
the recorded physical keyCode is saved and only round-tripable shortcuts are
accepted. Modify setShortcut to encode and store the StoredShortcut.keyCode (not
just key) and ensure StoredShortcut contains the recorded keyCode; update
normalizedRecordedShortcut to reject shortcuts that cannot be losslessly mapped
to a Carbon/registration key (use the same round-trip check used by
carbonHotKeyRegistration or a new helper that converts recorded keyCode → Carbon
key → back and verifies equality). Update isValid (and any similar validation
sites referenced around 904-939 and 997-1084) to require !hasChord,
hasPrimaryModifier, and that the shortcut is round-tripable into registration;
apply the same keyCode persistence and validation wherever shortcuts are saved
or normalized.
🪄 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: 9ff13d30-245b-4d6c-a139-39ad9c99142b
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
| var body: some View { | ||
| SettingsSectionHeader(title: String(localized: "settings.section.globalHotkey", defaultValue: "Global Hotkey")) | ||
| .accessibilityIdentifier("SettingsGlobalHotkeySection") | ||
|
|
||
| SettingsCard { | ||
| SettingsCardRow( | ||
| String(localized: "settings.globalHotkey.enable", defaultValue: "Enable System-Wide Hotkey"), | ||
| subtitle: enableSubtitle | ||
| ) { | ||
| Toggle("", isOn: enabledBinding) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| .accessibilityIdentifier("SettingsGlobalHotkeyToggle") | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| KeyboardShortcutRecorder( | ||
| label: String(localized: "settings.globalHotkey.shortcut", defaultValue: "Show/Hide All Windows"), | ||
| shortcut: $shortcut, | ||
| transformRecordedShortcut: { SystemWideHotkeySettings.normalizedRecordedShortcut($0) }, | ||
| onRecordingChanged: { SystemWideHotkeyController.shared.setShortcutRecordingActive($0) } | ||
| ) | ||
| .padding(.horizontal, 14) | ||
| .padding(.vertical, 9) | ||
| .accessibilityIdentifier("SettingsGlobalHotkeyRecorder") | ||
| } | ||
| .onChange(of: shortcut) { newValue in | ||
| SystemWideHotkeySettings.setShortcut(newValue) | ||
| } | ||
| .onReceive(NotificationCenter.default.publisher(for: UserDefaults.didChangeNotification)) { _ in | ||
| syncFromDefaults() | ||
| } | ||
|
|
||
| SettingsCardNote( | ||
| String( | ||
| localized: "settings.globalHotkey.note", | ||
| defaultValue: "Use Command, Option, or Control with another key. No extra macOS permission is required." | ||
| ) | ||
| ) | ||
| .accessibilityIdentifier("SettingsGlobalHotkeyNote") |
There was a problem hiding this comment.
Add the Accessibility access workflow to this section.
The system-wide hotkey depends on Accessibility trust, but this UI only exposes the toggle/recorder and even tells users “No extra macOS permission is required.” There’s no status/action row here, and the current UserDefaults.didChangeNotification observer won’t react when the user grants TCC permission in System Settings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 6728 - 6768, The UI lacks an
Accessibility permission status/action row and doesn't update when macOS TCC
changes; add a small status/action row in the SettingsCard (near
KeyboardShortcutRecorder) that reads
SystemWideHotkeyController.shared.accessibilityAuthorized (or calls
AXIsProcessTrusted()) and shows a button to open System Settings or to call
AXIsProcessTrustedWithOptions prompt if needed; update the SettingsCardNote text
to not claim “No extra macOS permission is required” when accessibility is
required; and instead of relying solely on UserDefaults.didChangeNotification,
subscribe to an accessibility-authorization change source (e.g., poll
AXIsProcessTrusted periodically or subscribe to a
DistributedNotificationCenter/AX notification if available) and call
syncFromDefaults() / update the enabledBinding/shortcut UI when authorization
changes so KeyboardShortcutRecorder and the toggle reflect the current TCC
state.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
6818-6819:⚠️ Potential issue | 🟠 MajorKeep the shortcut itself in
KeyboardShortcutSettings.Line 6850 opts this row out with
.settingsOnly, and Lines 6818-6819 / 6872-6873 read and write the shortcut throughSystemWideHotkeySettings. That creates a parallel shortcut source of truth, so this cmux-owned shortcut won't participate insettings.jsonsupport or the shared shortcut flow. Keep only the enabled flag here if needed; the shortcut should be backed by a dedicatedKeyboardShortcutSettings.Action.Based on learnings: Every new cmux-owned keyboard shortcut must be added to
KeyboardShortcutSettings, visible/editable in Settings, supported in~/.config/cmux/settings.json, and documented in the keyboard shortcut and configuration docs.Also applies to: 6850-6850, 6872-6873
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6818 - 6819, Remove the duplicate local shortcut state and instead back the shortcut from the centralized KeyboardShortcutSettings Action for this cmux shortcut: keep only the enabled AppStorage flag (SystemWideHotkeySettings.enabledKey) in this file, delete the `@State` private var shortcut, and update any read/write usage to use the dedicated KeyboardShortcutSettings Action binding (e.g., obtain the shortcut via KeyboardShortcutSettings for the specific Action name and read/update through that API) so the shortcut participates in Settings UI, ~/.config/cmux/settings.json, and shared shortcut flow; leave the row's .settingsOnly opt-out as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 6818-6819: Remove the duplicate local shortcut state and instead
back the shortcut from the centralized KeyboardShortcutSettings Action for this
cmux shortcut: keep only the enabled AppStorage flag
(SystemWideHotkeySettings.enabledKey) in this file, delete the `@State` private
var shortcut, and update any read/write usage to use the dedicated
KeyboardShortcutSettings Action binding (e.g., obtain the shortcut via
KeyboardShortcutSettings for the specific Action name and read/update through
that API) so the shortcut participates in Settings UI,
~/.config/cmux/settings.json, and shared shortcut flow; leave the row's
.settingsOnly opt-out as-is.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Sources/KeyboardShortcutSettings.swift (3)
673-680:⚠️ Potential issue | 🟠 MajorUse actual window visibility to decide hide vs show.
NSApp.isActivestays true even when every cmux window is miniaturized or otherwise not visible. In that state this takes the hide branch again, so the hotkey cannot restore the app's windows.🔧 Suggested fix
private func toggleApplicationVisibility() { - if NSApp.isActive { + let hasVisibleWindow = NSApp.windows.contains { $0.isVisible && !$0.isMiniaturized } + if NSApp.isActive && hasVisibleWindow { NSApp.hide(nil) return } showAllApplicationWindows() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 673 - 680, Replace the NSApp.isActive check in toggleApplicationVisibility with a real check for visible/non-miniaturized app windows: inspect NSApp.windows (or NSApp.orderedWindows) and if any window satisfies (isVisible && !isMiniaturized) then call NSApp.hide(nil), otherwise call showAllApplicationWindows(); update the function toggleApplicationVisibility and reference showAllApplicationWindows so the hotkey restores windows when they are merely miniaturized or hidden rather than relying on NSApp.isActive.
477-489:⚠️ Potential issue | 🟠 MajorDon't backfill
UserDefaultswhensettings.jsonowns this binding.This direct
defaults.set(...)bypassesKeyboardShortcutSettings.setShortcut(_:for:)and itsisManagedBySettingsFileguard. If a legacysystemWideHotkey.shortcutexists while.showHideAllWindowsis file-managed, you silently stash a hidden fallback inshortcut.showHideAllWindowsthat resurfaces later when the file entry is removed.🔧 Suggested fix
private static func migrateLegacyShortcutIfNeeded(defaults: UserDefaults = .standard) { guard defaults.object(forKey: legacyShortcutKey) != nil else { return } defer { defaults.removeObject(forKey: legacyShortcutKey) } - guard defaults.object(forKey: action.defaultsKey) == nil, + guard !KeyboardShortcutSettings.isManagedBySettingsFile(action), + defaults.object(forKey: action.defaultsKey) == nil, let data = defaults.data(forKey: legacyShortcutKey), let shortcut = try? JSONDecoder().decode(StoredShortcut.self, from: data) else { return }At minimum this migration needs the file-managed guard; routing the final write through
KeyboardShortcutSettings.setShortcut(_:for:)would also keep the notification path centralized.Based on learnings,
KeyboardShortcutSettings.setShortcut(...)is intentionally a no-op whenKeyboardShortcutSettings.isManagedBySettingsFile(action)is true sosettings.jsonremains the source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 477 - 489, The migration currently writes directly to UserDefaults (defaults.set(...)) inside migrateLegacyShortcutIfNeeded, bypassing KeyboardShortcutSettings.setShortcut(_:for:) and its isManagedBySettingsFile(action) guard; update migrateLegacyShortcutIfNeeded to first check KeyboardShortcutSettings.isManagedBySettingsFile(action) and if false, call KeyboardShortcutSettings.setShortcut(migratedShortcut, for: action) instead of defaults.set, preserving the existing defer that removes legacyShortcutKey so the legacy key is cleaned up regardless.
309-315:⚠️ Potential issue | 🟠 MajorReject global hotkeys that collide with existing cmux bindings.
This helper still accepts any non-chord shortcut with a primary modifier, so
Cmd+W,Cmd+Option+T, or another configured cmux binding can still be recorded here. Once the global hotkey claims that chord, the original app action stops receiving it while the feature is enabled.🔧 Suggested direction
private static func normalizedSystemWideHotkeyShortcut(_ shortcut: StoredShortcut) -> StoredShortcut? { guard !shortcut.hasChord, shortcut.hasPrimaryModifier, - shortcut.carbonHotKeyRegistration != nil else { + shortcut.carbonHotKeyRegistration != nil, + !conflictsWithExistingShortcut(shortcut) else { return nil } return shortcut }The collision helper should compare logical shortcut semantics, not full
StoredShortcutequality, because recorded global shortcuts now carrykeyCodewhile many existing defaults do not.Based on learnings,
KeyboardShortcutSettings.Action.toggleTextBoxInputdefaults to Cmd+Option+B specifically to avoid the existing hard-coded Cmd+Option+T binding inAppDelegate.handleCustomShortcut(event:).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 309 - 315, The helper normalizedSystemWideHotkeyShortcut(_:) must also reject shortcuts that logically collide with existing cmux bindings (not just exact StoredShortcut equality). Update normalizedSystemWideHotkeyShortcut to, after the existing guards, compare the candidate shortcut against the app's configured bindings (e.g., the defaults used by KeyboardShortcutSettings.Action and the hard-coded bindings checked in AppDelegate.handleCustomShortcut(event:)) by matching modifier sets and the logical key rather than raw StoredShortcut equality: consider two shortcuts a collision if they have the same primary modifier flags and the same logical key (treat keyCode equality as definitive, and when keyCode is absent compare keyEquivalent characters case‑insensitively), and return nil when a collision is detected. Ensure you reference StoredShortcut properties like modifierFlags, keyCode and keyEquivalent in the comparison.
🤖 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/cmuxApp.swift`:
- Around line 6864-6888: The UI note and the shortcut validation disagree with
Apple RegisterEventHotKey limits: update the SettingsCardNote copy and the
shortcut-normalization/validation to match real constraints and reject
unsupported combos; change the note string used in SettingsCardNote (localized
"settings.globalHotkey.note") to explicitly state that Command must be one of
the modifiers (e.g., "Use Command plus another key; combinations using
Option-only or Shift-only are not supported"), and in
SystemWideHotkeySettings.normalizedRecordedShortcut (and any validator named
normalizedSystemWideHotkeyShortcut) add pre-recording validation that
rejects/normalizes shortcuts that lack the Command modifier (unless Command is
present with other modifiers) and that disallow Option-only or Shift-only
modifier sets—return a validation error or prevent recording so
KeyboardShortcutRecorder shows an invalid state and
SystemWideHotkeyController.shared.setShortcutRecordingActive remains consistent.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 1070-1127: keyCodeForShortcutKey(_:) is missing a mapping for the
Return key so StoredShortcut(key: "\r", keyCode: nil) never becomes a
carbonHotKeyRegistration; add a case for "\r" that returns the Carbon keyCode
for Return (36) inside keyCodeForShortcutKey(_:) so shortcuts like
.showHideAllWindows using Return will register correctly.
---
Duplicate comments:
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 673-680: Replace the NSApp.isActive check in
toggleApplicationVisibility with a real check for visible/non-miniaturized app
windows: inspect NSApp.windows (or NSApp.orderedWindows) and if any window
satisfies (isVisible && !isMiniaturized) then call NSApp.hide(nil), otherwise
call showAllApplicationWindows(); update the function
toggleApplicationVisibility and reference showAllApplicationWindows so the
hotkey restores windows when they are merely miniaturized or hidden rather than
relying on NSApp.isActive.
- Around line 477-489: The migration currently writes directly to UserDefaults
(defaults.set(...)) inside migrateLegacyShortcutIfNeeded, bypassing
KeyboardShortcutSettings.setShortcut(_:for:) and its
isManagedBySettingsFile(action) guard; update migrateLegacyShortcutIfNeeded to
first check KeyboardShortcutSettings.isManagedBySettingsFile(action) and if
false, call KeyboardShortcutSettings.setShortcut(migratedShortcut, for: action)
instead of defaults.set, preserving the existing defer that removes
legacyShortcutKey so the legacy key is cleaned up regardless.
- Around line 309-315: The helper normalizedSystemWideHotkeyShortcut(_:) must
also reject shortcuts that logically collide with existing cmux bindings (not
just exact StoredShortcut equality). Update normalizedSystemWideHotkeyShortcut
to, after the existing guards, compare the candidate shortcut against the app's
configured bindings (e.g., the defaults used by KeyboardShortcutSettings.Action
and the hard-coded bindings checked in AppDelegate.handleCustomShortcut(event:))
by matching modifier sets and the logical key rather than raw StoredShortcut
equality: consider two shortcuts a collision if they have the same primary
modifier flags and the same logical key (treat keyCode equality as definitive,
and when keyCode is absent compare keyEquivalent characters case‑insensitively),
and return nil when a collision is detected. Ensure you reference StoredShortcut
properties like modifierFlags, keyCode and keyEquivalent in the comparison.
🪄 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: e872d420-7d2a-430d-a3e6-eb34e1e5651d
📒 Files selected for processing (5)
Sources/KeyboardShortcutSettings.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swiftweb/data/cmux-settings.schema.jsonweb/data/cmux-shortcuts.ts
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/WorkspaceUnitTests.swift
…-2362-system-wide-hotkey
|
This does not work for me. I downloaded and installed the nightly today 4/22, and tried a few different shortcuts to make sure it's not just a conflict. Pressing the hotkey does not do anything. |
|
Hi @austinywang, thank you for implementing it! Do you know by any chance how can I register |
This branch was successfully deployed
3 participants
Summary
KeyboardShortcutSettings/settings.jsonsupport while keeping the enabled flag inUserDefaultsRegisterEventHotKey, so no Accessibility permission is requiredTesting
./scripts/reload.sh --tag issue-2362-system-wide-hotkeyDemo Video
Known Notes
KeyboardShortcutSettings), includingsettings.jsonoverridessettings.json, but cmux will not register them until they are changed to a valid non-conflicting shortcutReview Trigger (Copy/Paste as PR comment)
Checklist
Note
Medium Risk
Touches keyboard shortcut persistence/matching and introduces Carbon-level global hotkey registration, which can affect input handling across layouts and app/window visibility behavior if edge cases are missed.
Overview
Adds a new system-wide “Show/Hide All Windows” hotkey backed by Carbon
RegisterEventHotKey, with a Settings > Global Hotkey section to enable/disable it and record the shortcut.Extends
KeyboardShortcutSettingswith ashowHideAllWindowsaction and introducesSystemWideHotkeyController/SystemWideHotkeySettingsto manage enablement, legacy migration, and registration; invalid/chord/conflicting shortcuts are preserved for display but not registered, and chord-prefix handling explicitly excludes this action.Refactors shortcut matching/recording to carry and use recorded
keyCode(plus sharedmatches(...)helpers) to improve layout/input-source behavior and to validate system-wide conflicts; adds recorder activity tracking to temporarily unregister the global hotkey while recording.Includes supporting updates: new localizations, settings schema/docs (
web/data), new unit tests around matching and migration, and a small zsh integration change to delay restoringTERMuntil the first prompt.Reviewed by Cursor Bugbot for commit d03a82a. Bugbot is set up for automated code reviews on this repo. Configure here.