Explain shortcut recorder rejections inline (fixes #2988) - #3035
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:
📝 WalkthroughWalkthroughAdds explicit shortcut-recording result types and rejection reasons, surfaces validation and swap/reassign UI flows, records/matches Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Recorder as ShortcutRecorder
participant Stroke as ShortcutStroke
participant Settings as KeyboardShortcutSettings
participant UI as ShortcutRecorderSettingsControl
User->>Recorder: press key / media event
Recorder->>Stroke: recordingResult(from:event, requireModifier)
Stroke-->>Recorder: .accepted(proposed) / .rejected(reason) / .unsupported
alt Stroke rejected
Recorder-->>UI: onRecorderFeedbackChanged(rejection)
UI->>User: show rejection message
else Stroke accepted
Recorder->>Settings: normalizedRecordedShortcutResult(proposed)
Settings-->>Recorder: .accepted(StoredShortcut) / .rejected(reason + metadata)
alt Settings rejected
Recorder-->>UI: onRecorderFeedbackChanged(rejection)
UI->>User: show message + Reassign/Swap (if available)
opt User chooses Swap
UI->>Settings: swapShortcutConflict(proposed,currentAction,conflictingAction,previousShortcut)
Settings->>Settings: persist replacements & emit notifications
Settings-->>UI: confirmation
end
else Settings accepted
Recorder->>Settings: persist accepted shortcut
UI->>UI: update binding and clear state
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
1426-1674: Add coverage for the remaining rejection reasons.The new suite covers bare-key, conflict, and reserved-system paths, but not
numberedShortcutRequiresDigitorsystemWideHotkeyRequiresModifier. Adding one normalization/presentation assertion for each would lock down all recorder rejection messages introduced by this PR.🧪 Example coverage to add
+ func testShortcutRecorderResolutionRejectsNumberedShortcutWithoutDigit() { + KeyboardShortcutSettings.resetAll() + defer { KeyboardShortcutSettings.resetAll() } + + let shortcut = StoredShortcut(key: "x", command: true, shift: false, option: false, control: false) + + XCTAssertEqual( + KeyboardShortcutSettings.Action.selectWorkspaceByNumber.normalizedRecordedShortcutResult(shortcut), + .rejected(.numberedShortcutRequiresDigit) + ) + } + + func testSystemWideHotkeyNormalizationRequiresPrimaryModifier() { + let shortcut = StoredShortcut(key: "f1", command: false, shift: true, option: false, control: false, keyCode: 122) + + XCTAssertEqual( + KeyboardShortcutSettings.Action.showHideAllWindows.normalizedRecordedShortcutResult(shortcut), + .rejected(.systemWideHotkeyRequiresModifier) + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 1426 - 1674, Add two unit tests that cover the remaining rejection reasons: create a test that constructs a ShortcutRecorderRejectedAttempt(reason: .numberedShortcutRequiresDigit, proposedShortcut: nil) and passes it into ShortcutRecorderValidationPresentation (use an action such as .selectWorkspaceByNumber and currentShortcut from KeyboardShortcutSettings.Action.selectWorkspaceByNumber.defaultShortcut) then assert the presentation.message equals the expected "Shortcuts to switch to numbered workspaces must use a number key" (or the actual localized string used), reassignButtonTitle is nil, and canReassign is false; and create a second test that constructs ShortcutRecorderRejectedAttempt(reason: .systemWideHotkeyRequiresModifier, proposedShortcut: nil) and passes it into ShortcutRecorderValidationPresentation for a system-wide action (e.g., .showHideAllWindows) asserting the presentation.message equals the expected "System-wide hotkeys must include a modifier", reassignButtonTitle is nil, and canReassign is false. Use the same test patterns and helpers as existing tests (ShortcutRecorderRejectedAttempt, ShortcutRecorderValidationPresentation, KeyboardShortcutSettings.Action.*.defaultShortcut) to match style and placement.
🤖 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 10268-10272: The current NSEvent.addLocalMonitorForEvents
(assigned to shortcutMonitor and the debug hook at the other location) routes
all .systemDefined events into the shortcut pipeline; narrow the predicate so
only systemDefined events with subtype == 8 (media-key events) are passed to
handleCustomShortcut by checking event.subtype.rawValue == 8 before calling
handleCustomShortcut; update both occurrences where
NSEvent.addLocalMonitorForEvents is used and referenced (shortcutMonitor and the
debugShortcutMonitor hook) and keep using the existing mediaKey(from:) gating
inside handleCustomShortcut for keyState semantics.
In `@Sources/cmuxApp.swift`:
- Around line 7276-7290: The handler reassignConflictingShortcut unconditionally
updates the local binding (shortcut) and clears rejectedAttempt even though
KeyboardShortcutSettings.reassignShortcutConflict(...) can silently no-op;
change reassignConflictingShortcut to call the settings method and only update
shortcut and clear rejectedAttempt if the settings call reports success (or
reloads the persisted shortcut) — i.e., have
KeyboardShortcutSettings.reassignShortcutConflict return a Bool (or throw) and
use that result to decide whether to set shortcut = proposedShortcut and
rejectedAttempt = nil, otherwise re-read the persisted shortcut state before
returning. Ensure you reference reassignConflictingShortcut,
KeyboardShortcutSettings.reassignShortcutConflict, rejectedAttempt and shortcut
in the fix.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 1990-2011: The conflict message currently always asks "Reassign?"
even when reassignment is disabled; update the API so message(for:) accepts the
reassignment availability and choose the localized string accordingly: change
private static func message(for reason:
KeyboardShortcutSettings.ShortcutRecordingRejection) -> String to message(for:
KeyboardShortcutSettings.ShortcutRecordingRejection, canReassign: Bool), update
all call sites (e.g., where message(for: attempt.reason) is invoked) to pass the
canReassign value, and in the .conflictsWithAction(conflictingAction) branch
return a question-format localized string only when canReassign is true and a
non-question localized string when false (use String(localized: "…",
defaultValue: "…") for both). Ensure you update the other occurrence range noted
(around lines 2030–2049) to use the new signature as well.
- Around line 1334-1340: The matches(keyCode:...) path needs to reject media
shortcuts to avoid keyCode collisions (e.g., media.volumeUp has keyCode 0);
inside the block where shortcutKey is computed and
Self.usesDirectKeyCodeMatching(shortcutKey) is true, add a guard that returns
false if the shortcutKey represents a media/system shortcut (for example if it
starts with "media." or by using any existing helper like Self.isMediaShortcut
if available), before computing expectedKeyCode and comparing keyCode ==
expectedKeyCode.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 1426-1674: Add two unit tests that cover the remaining rejection
reasons: create a test that constructs a ShortcutRecorderRejectedAttempt(reason:
.numberedShortcutRequiresDigit, proposedShortcut: nil) and passes it into
ShortcutRecorderValidationPresentation (use an action such as
.selectWorkspaceByNumber and currentShortcut from
KeyboardShortcutSettings.Action.selectWorkspaceByNumber.defaultShortcut) then
assert the presentation.message equals the expected "Shortcuts to switch to
numbered workspaces must use a number key" (or the actual localized string
used), reassignButtonTitle is nil, and canReassign is false; and create a second
test that constructs ShortcutRecorderRejectedAttempt(reason:
.systemWideHotkeyRequiresModifier, proposedShortcut: nil) and passes it into
ShortcutRecorderValidationPresentation for a system-wide action (e.g.,
.showHideAllWindows) asserting the presentation.message equals the expected
"System-wide hotkeys must include a modifier", reassignButtonTitle is nil, and
canReassign is false. Use the same test patterns and helpers as existing tests
(ShortcutRecorderRejectedAttempt, ShortcutRecorderValidationPresentation,
KeyboardShortcutSettings.Action.*.defaultShortcut) to match style and placement.
🪄 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: c8b3572d-3240-4726-afa1-2454b4b8b5ee
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
| shortcutMonitor = NSEvent.addLocalMonitorForEvents( | ||
| matching: [.keyDown, .keyUp, .flagsChanged, .systemDefined] | ||
| ) { [weak self] event in | ||
| guard let self else { return event } | ||
| if event.type == .keyDown { | ||
| if event.type == .keyDown || event.type == .systemDefined { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect media-key parsing and tests for subtype/key-state filtering.
# Expectation: mediaKey(from:) filters subtype 8 and media-key press state, and tests cover non-media systemDefined pass-through.
rg -n -A50 -B5 'private static func mediaKey\(from event: NSEvent\)'
rg -n -C4 'systemDefined|subtype\.rawValue|data1|makeMediaKeyEvent|debugHandleShortcutMonitorEvent' Sources cmuxTestsRepository: manaflow-ai/cmux
Length of output: 20960
Narrow .systemDefined routing to media-key events (subtype 8) only.
Line 10272 routes every .systemDefined event through handleCustomShortcut, but only subtype 8 with keyState 0x0A represents valid media-key shortcuts. Non-media system events unnecessarily enter broad shortcut handling before being filtered out. The mediaKey(from:) function already gates release events via the keyState == 0x0A check, so your secondary concern is resolved—no additional key-state filtering is needed in the predicate.
Narrow both the production monitor and debug hook to skip non-media systemDefined events:
Proposed fix
+ private func isShortcutMonitorShortcutCandidate(_ event: NSEvent) -> Bool {
+ if event.type == .keyDown {
+ return true
+ }
+ return event.type == .systemDefined && event.subtype.rawValue == Int16(8)
+ }
+
private func installShortcutMonitor() {
// Local monitor only receives events when app is active (not global)
shortcutMonitor = NSEvent.addLocalMonitorForEvents(
matching: [.keyDown, .keyUp, .flagsChanged, .systemDefined]
) { [weak self] event in
guard let self else { return event }
- if event.type == .keyDown || event.type == .systemDefined {
+ if self.isShortcutMonitorShortcutCandidate(event) { // Debug/test hook: mirrors local monitor routing (keyDown + keyUp lifecycle).
func debugHandleShortcutMonitorEvent(event: NSEvent) -> Bool {
- if event.type == .keyDown || event.type == .systemDefined {
+ if isShortcutMonitorShortcutCandidate(event) {
return handleCustomShortcut(event: event)
}Also applies to: 12266-12269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 10268 - 10272, The current
NSEvent.addLocalMonitorForEvents (assigned to shortcutMonitor and the debug hook
at the other location) routes all .systemDefined events into the shortcut
pipeline; narrow the predicate so only systemDefined events with subtype == 8
(media-key events) are passed to handleCustomShortcut by checking
event.subtype.rawValue == 8 before calling handleCustomShortcut; update both
occurrences where NSEvent.addLocalMonitorForEvents is used and referenced
(shortcutMonitor and the debugShortcutMonitor hook) and keep using the existing
mediaKey(from:) gating inside handleCustomShortcut for keyState semantics.
| private func reassignConflictingShortcut() { | ||
| guard case let .conflictsWithAction(conflictingAction)? = rejectedAttempt?.reason, | ||
| let proposedShortcut = rejectedAttempt?.proposedShortcut else { | ||
| return | ||
| } | ||
|
|
||
| let previousShortcut = shortcut | ||
| KeyboardShortcutSettings.reassignShortcutConflict( | ||
| proposedShortcut: proposedShortcut, | ||
| currentAction: action, | ||
| conflictingAction: conflictingAction, | ||
| previousShortcut: previousShortcut | ||
| ) | ||
| shortcut = proposedShortcut | ||
| rejectedAttempt = nil |
There was a problem hiding this comment.
Avoid showing a reassignment that the settings layer rejected.
KeyboardShortcutSettings.reassignShortcutConflict(...) can silently no-op, but this handler still sets the local binding to proposedShortcut and clears the rejection. Since the parent .onChange persists that binding, a stale “Reassign” click can show false success or recreate a conflict. Re-read persisted state or make the settings method return success before updating local UI.
🐛 Proposed guard against rejected reassignment
KeyboardShortcutSettings.reassignShortcutConflict(
proposedShortcut: proposedShortcut,
currentAction: action,
conflictingAction: conflictingAction,
previousShortcut: previousShortcut
)
- shortcut = proposedShortcut
- rejectedAttempt = nil
+ let latestShortcut = KeyboardShortcutSettings.shortcut(for: action)
+ shortcut = latestShortcut
+ if latestShortcut == proposedShortcut {
+ rejectedAttempt = nil
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/cmuxApp.swift` around lines 7276 - 7290, The handler
reassignConflictingShortcut unconditionally updates the local binding (shortcut)
and clears rejectedAttempt even though
KeyboardShortcutSettings.reassignShortcutConflict(...) can silently no-op;
change reassignConflictingShortcut to call the settings method and only update
shortcut and clear rejectedAttempt if the settings call reports success (or
reloads the persisted shortcut) — i.e., have
KeyboardShortcutSettings.reassignShortcutConflict return a Bool (or throw) and
use that result to decide whether to set shortcut = proposedShortcut and
rejectedAttempt = nil, otherwise re-read the persisted shortcut state before
returning. Ensure you reference reassignConflictingShortcut,
KeyboardShortcutSettings.reassignShortcutConflict, rejectedAttempt and shortcut
in the fix.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 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/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:7289">
P2: After calling `reassignShortcutConflict(...)`, the binding is unconditionally updated to `proposedShortcut` and the rejection is cleared. However, `reassignShortcutConflict` can silently no-op via its guard clause (e.g., if the resolved shortcut for the conflicting action fails validation). Read back the persisted shortcut to confirm the reassignment succeeded before updating the local UI state.</violation>
</file>
<file name="Sources/KeyboardShortcutSettings.swift">
<violation number="1" location="Sources/KeyboardShortcutSettings.swift:1335">
P2: `media.volumeUp` has keyCode `0`, which collides with ANSI key `A` (also keyCode `0`). The `matches(event:)` overload correctly gates media shortcuts via `.systemDefined` event type, but this `matches(keyCode:...)` overload uses `usesDirectKeyCodeMatching` which returns `true` for media keys, causing a false positive match. Add an early return for `media.*` keys.</violation>
<violation number="2" location="Sources/KeyboardShortcutSettings.swift:1990">
P3: The conflict message always says "Reassign?" even when `canReassign` is `false` and the button is hidden. Pass `canReassign` into `message(for:)` and use a separate localization key without the question for the non-reassignable case (e.g. "This shortcut is already used by %@.").</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| shortcut = proposedShortcut | ||
| rejectedAttempt = nil |
There was a problem hiding this comment.
P2: After calling reassignShortcutConflict(...), the binding is unconditionally updated to proposedShortcut and the rejection is cleared. However, reassignShortcutConflict can silently no-op via its guard clause (e.g., if the resolved shortcut for the conflicting action fails validation). Read back the persisted shortcut to confirm the reassignment succeeded before updating the local UI state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/cmuxApp.swift, line 7289:
<comment>After calling `reassignShortcutConflict(...)`, the binding is unconditionally updated to `proposedShortcut` and the rejection is cleared. However, `reassignShortcutConflict` can silently no-op via its guard clause (e.g., if the resolved shortcut for the conflicting action fails validation). Read back the persisted shortcut to confirm the reassignment succeeded before updating the local UI state.</comment>
<file context>
@@ -7237,6 +7236,61 @@ private struct ShortcutSettingRow: View {
+ conflictingAction: conflictingAction,
+ previousShortcut: previousShortcut
+ )
+ shortcut = proposedShortcut
+ rejectedAttempt = nil
+ }
</file context>
| shortcut = proposedShortcut | |
| rejectedAttempt = nil | |
| let latestShortcut = KeyboardShortcutSettings.shortcut(for: action) | |
| shortcut = latestShortcut | |
| if latestShortcut == proposedShortcut { | |
| rejectedAttempt = nil | |
| } |
| guard flags == self.modifierFlags else { return false } | ||
|
|
||
| let shortcutKey = key.lowercased() | ||
| if Self.usesDirectKeyCodeMatching(shortcutKey) { |
There was a problem hiding this comment.
P2: media.volumeUp has keyCode 0, which collides with ANSI key A (also keyCode 0). The matches(event:) overload correctly gates media shortcuts via .systemDefined event type, but this matches(keyCode:...) overload uses usesDirectKeyCodeMatching which returns true for media keys, causing a false positive match. Add an early return for media.* keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/KeyboardShortcutSettings.swift, line 1335:
<comment>`media.volumeUp` has keyCode `0`, which collides with ANSI key `A` (also keyCode `0`). The `matches(event:)` overload correctly gates media shortcuts via `.systemDefined` event type, but this `matches(keyCode:...)` overload uses `usesDirectKeyCodeMatching` which returns `true` for media keys, causing a false positive match. Add an early return for `media.*` keys.</comment>
<file context>
@@ -1036,6 +1332,13 @@ struct ShortcutStroke: Equatable {
guard flags == self.modifierFlags else { return false }
let shortcutKey = key.lowercased()
+ if Self.usesDirectKeyCodeMatching(shortcutKey) {
+ guard let expectedKeyCode = self.keyCode ?? Self.keyCodeForShortcutKey(shortcutKey) else {
+ return false
</file context>
| if Self.usesDirectKeyCodeMatching(shortcutKey) { | |
| if shortcutKey.hasPrefix("media.") { | |
| return false | |
| } | |
| if Self.usesDirectKeyCodeMatching(shortcutKey) { |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/KeyboardShortcutSettings.swift`:
- Around line 631-654: reassignShortcutConflict currently writes
previousShortcut onto conflictingAction without re-checking whether that
previousShortcut now conflicts with any other action or violates other
action-specific constraints; before calling
persistShortcut(resolvedConflictingShortcut, for: conflictingAction) call
conflictingAction(for: previousShortcut, excluding: conflictingAction) (and also
validate against the same constraints used by
storedShortcutForReplacement/resolvedRecordedShortcutIgnoringConflicts) and if
it returns a third action abort the swap or surface a follow-up prompt to the
user instead of persisting; keep existing guards (isManagedBySettingsFile,
storedShortcutForReplacement) and only proceed to persistShortcut(...) and
postDidChangeNotification(...) when no further conflicts/constraint violations
are detected.
- Around line 366-378: The chord rejection currently returned by
normalizedSystemWideHotkeyShortcutResult maps shortcut.hasChord to
.reservedBySystem which yields a misleading "reserved by macOS" message; add a
new rejection case (e.g. .systemWideHotkeyDoesNotSupportChords) to the
RecordedShortcutResolution rejection enum and return that from
normalizedSystemWideHotkeyShortcutResult when shortcut.hasChord is true, then
update
ShortcutRecorderValidationPresentation.message(for:canReassign:shortcutForAction:)
to handle the new case with a localized user-facing string using
String(localized: "key.name", defaultValue: "Single-stroke system-wide hotkeys
only") and add the key and translations to Resources/Localizable.xcstrings;
finally extend relevant regression tests to assert the new rejection reason and
localized message appears for chords.
🪄 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: 0bc759d1-47eb-4bdc-b0c0-d832a8641cbb
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftcmuxTests/WorkspaceUnitTests.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/WorkspaceUnitTests.swift
| private static func normalizedSystemWideHotkeyShortcutResult(_ shortcut: StoredShortcut) -> RecordedShortcutResolution { | ||
| guard !shortcut.hasChord else { | ||
| return .rejected(.reservedBySystem) | ||
| } | ||
| guard shortcut.hasPrimaryModifier else { | ||
| return .rejected(.systemWideHotkeyRequiresModifier) | ||
| } | ||
| guard shortcut.carbonHotKeyRegistration != nil, | ||
| !systemWideHotkeyConflicts(with: shortcut) else { | ||
| return .rejected(.reservedBySystem) | ||
| } | ||
| return .accepted(shortcut) | ||
| } |
There was a problem hiding this comment.
Chord rejection for system-wide hotkey surfaces a misleading "reserved by macOS" message.
normalizedSystemWideHotkeyShortcutResult maps shortcut.hasChord == true onto .reservedBySystem, which ShortcutRecorderValidationPresentation.message(for:…) renders as "This keystroke is reserved by macOS." A user recording a valid chord like ⌘K, X for Show/Hide All Windows will see a reason that is factually wrong (the keystroke isn't system-reserved — chords simply aren't supported for a Carbon global hotkey). Consider a dedicated rejection reason (e.g. .systemWideHotkeyDoesNotSupportChords) with a clearer localized string so the user knows to press a single stroke rather than hunting for a "non-reserved" alternative.
🩹 Suggested direction
enum ShortcutRecordingRejection: Equatable {
case bareKeyNotAllowed
case conflictsWithAction(Action)
case reservedBySystem
case numberedShortcutRequiresDigit
case systemWideHotkeyRequiresModifier
+ case systemWideHotkeyDoesNotSupportChords
}
...
private static func normalizedSystemWideHotkeyShortcutResult(_ shortcut: StoredShortcut) -> RecordedShortcutResolution {
guard !shortcut.hasChord else {
- return .rejected(.reservedBySystem)
+ return .rejected(.systemWideHotkeyDoesNotSupportChords)
}Then add a localized message for the new case in ShortcutRecorderValidationPresentation.message(for:canReassign:shortcutForAction:) and extend the regression tests accordingly.
As per coding guidelines, all user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") and added to Resources/Localizable.xcstrings with translations for all supported languages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/KeyboardShortcutSettings.swift` around lines 366 - 378, The chord
rejection currently returned by normalizedSystemWideHotkeyShortcutResult maps
shortcut.hasChord to .reservedBySystem which yields a misleading "reserved by
macOS" message; add a new rejection case (e.g.
.systemWideHotkeyDoesNotSupportChords) to the RecordedShortcutResolution
rejection enum and return that from normalizedSystemWideHotkeyShortcutResult
when shortcut.hasChord is true, then update
ShortcutRecorderValidationPresentation.message(for:canReassign:shortcutForAction:)
to handle the new case with a localized user-facing string using
String(localized: "key.name", defaultValue: "Single-stroke system-wide hotkeys
only") and add the key and translations to Resources/Localizable.xcstrings;
finally extend relevant regression tests to assert the new rejection reason and
localized message appears for chords.
| static func reassignShortcutConflict( | ||
| proposedShortcut: StoredShortcut, | ||
| currentAction: Action, | ||
| conflictingAction: Action, | ||
| previousShortcut: StoredShortcut | ||
| ) { | ||
| guard !isManagedBySettingsFile(currentAction), | ||
| !isManagedBySettingsFile(conflictingAction), | ||
| let resolvedCurrentShortcut = storedShortcutForReplacement( | ||
| proposedShortcut, | ||
| action: currentAction | ||
| ), | ||
| let resolvedConflictingShortcut = storedShortcutForReplacement( | ||
| previousShortcut, | ||
| action: conflictingAction | ||
| ) else { | ||
| return | ||
| } | ||
|
|
||
| persistShortcut(resolvedCurrentShortcut, for: currentAction) | ||
| persistShortcut(resolvedConflictingShortcut, for: conflictingAction) | ||
| postDidChangeNotification(action: currentAction) | ||
| postDidChangeNotification(action: conflictingAction) | ||
| } |
There was a problem hiding this comment.
Reassign does not re-validate against other actions.
reassignShortcutConflict persists previousShortcut onto conflictingAction without checking whether previousShortcut now conflicts with a third action (or violates that action's constraints beyond resolvedRecordedShortcutIgnoringConflicts, which by design ignores conflicts). In practice the user can end up with two actions sharing a keystroke after pressing "Reassign". Consider running conflictingAction(for: previousShortcut, excluding: conflictingAction) and aborting (or surfacing a follow-up prompt) when the swap would cascade into another conflict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/KeyboardShortcutSettings.swift` around lines 631 - 654,
reassignShortcutConflict currently writes previousShortcut onto
conflictingAction without re-checking whether that previousShortcut now
conflicts with any other action or violates other action-specific constraints;
before calling persistShortcut(resolvedConflictingShortcut, for:
conflictingAction) call conflictingAction(for: previousShortcut, excluding:
conflictingAction) (and also validate against the same constraints used by
storedShortcutForReplacement/resolvedRecordedShortcutIgnoringConflicts) and if
it returns a third action abort the swap or surface a follow-up prompt to the
user instead of persisting; keep existing guards (isManagedBySettingsFile,
storedShortcutForReplacement) and only proceed to persistShortcut(...) and
postDidChangeNotification(...) when no further conflicts/constraint violations
are detected.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR surfaces inline validation messages in the shortcut recorder (bare key disallowed, macOS-reserved, and conflict with named action + Reassign button), adds function-key (F1–F20) and media-key recording/matching via
Confidence Score: 4/5Safe to merge after fixing the "Reassign?" message shown without a button when the conflicting action is settings-file-managed. One P1 UX defect: the conflict message always says "Reassign?" even when the Reassign button is not rendered (conflicting action managed by settings.json), leaving users with an un-actionable instruction. Two P2 findings (Release-mode test failure, brief unnormalized binding value) do not affect correctness at runtime. Sources/KeyboardShortcutSettings.swift (ShortcutRecorderValidationPresentation.message), Sources/cmuxApp.swift (reassignConflictingShortcut) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Recorder as ShortcutRecorderNSButton
participant Control as ShortcutRecorderSettingsControl
participant KSS as KeyboardShortcutSettings
User->>Recorder: press key (first stroke)
Recorder->>Recorder: ShortcutStroke.recordingResult(requireModifier:true)
alt bareKeyNotAllowed
Recorder-->>Control: onRecorderFeedbackChanged(RejectedAttempt(bareKey, nil))
Control-->>User: show "Shortcuts must include ⌘ ⌥ ⌃ or ⇧"
else accepted
Recorder->>Recorder: store pendingChordStart
end
User->>Recorder: press second key / click button
Recorder->>KSS: transformRecordedShortcut(shortcut)
KSS->>KSS: conflictingAction(for:excluding:)
alt conflict found
KSS-->>Recorder: .rejected(.conflictsWithAction(X))
Recorder-->>Control: onRecorderFeedbackChanged(RejectedAttempt(conflict, shortcut))
Control->>Control: compute canReassign via ShortcutRecorderValidationPresentation
alt canReassign == true
Control-->>User: message + Reassign button
User->>Control: tap Reassign
Control->>KSS: reassignShortcutConflict(proposed, current, conflicting, previous)
KSS->>KSS: persistShortcut x2 + postNotification x2
else canReassign == false
Control-->>User: message only (no button)
end
else accepted
KSS-->>Recorder: .accepted(normalizedShortcut)
Recorder-->>Control: onShortcutRecorded(shortcut)
end
Reviews (1): Last reviewed commit: "Show shortcut conflicts immediately in r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) -> String { | ||
| switch reason { | ||
| case .bareKeyNotAllowed: | ||
| return String( | ||
| localized: "shortcut.recorder.error.bareKeyNotAllowed", | ||
| defaultValue: "Shortcuts must include ⌘ ⌥ ⌃ or ⇧" |
There was a problem hiding this comment.
message always appends "Reassign?" for a conflict, but when canReassign = false (e.g., the conflicting action is managed by settings.json) reassignButtonTitle is nil and no button is rendered. The user sees an instruction they cannot act on.
This is reachable: the current action's recorder is enabled (it is not managed), so the conflict message is displayed, but the conflicting action is managed (so canReassign = false).
| ) -> String { | |
| switch reason { | |
| case .bareKeyNotAllowed: | |
| return String( | |
| localized: "shortcut.recorder.error.bareKeyNotAllowed", | |
| defaultValue: "Shortcuts must include ⌘ ⌥ ⌃ or ⇧" | |
| case let .conflictsWithAction(conflictingAction): | |
| let format = String( | |
| localized: "shortcut.recorder.error.conflictsWithAction", | |
| defaultValue: "This shortcut is already used by %@." | |
| ) | |
| return String.localizedStringWithFormat(format, conflictingAction.label) |
The "Reassign" affordance already lives on the separate button; the message doesn't need to repeat the prompt.
| @@ -319,6 +319,59 @@ final class WorkspaceRenameShortcutDefaultsTests: XCTestCase { | |||
| XCTFail("Shortcut recorder debug hooks are only available in DEBUG") | |||
| #endif | |||
| } | |||
There was a problem hiding this comment.
The #else branch calls XCTFail("Shortcut recorder debug hooks are only available in DEBUG"), so if the test suite is ever run in a non-DEBUG (Release) configuration this test will always fail. Consider skipping instead of failing:
| func testShortcutRecorderStopsRecordingWhenFirstStrokeConfirmationIsRejected() { | |
| #if DEBUG | |
| let button = ShortcutRecorderNSButton(frame: .zero) | |
| button.transformRecordedShortcut = { _ in .rejected(.reservedBySystem) } | |
| button.debugSetPendingChordStart( | |
| ShortcutStroke( | |
| key: "x", | |
| command: true, | |
| shift: false, | |
| option: false, | |
| control: false | |
| ) | |
| ) | |
| button.performClick(nil) | |
| XCTAssertFalse(button.debugIsRecording) | |
| #else | |
| throw XCTSkip("Shortcut recorder debug hooks are only available in DEBUG builds") | |
| #endif | |
| } |
| private func reassignConflictingShortcut() { | ||
| guard case let .conflictsWithAction(conflictingAction)? = rejectedAttempt?.reason, | ||
| let proposedShortcut = rejectedAttempt?.proposedShortcut else { | ||
| return | ||
| } | ||
|
|
||
| let previousShortcut = shortcut | ||
| KeyboardShortcutSettings.reassignShortcutConflict( | ||
| proposedShortcut: proposedShortcut, | ||
| currentAction: action, | ||
| conflictingAction: conflictingAction, | ||
| previousShortcut: previousShortcut | ||
| ) | ||
| shortcut = proposedShortcut | ||
| rejectedAttempt = nil | ||
| } |
There was a problem hiding this comment.
shortcut binding set to un-normalized value before notification corrects it
reassignShortcutConflict persists storedShortcutForReplacement(proposedShortcut, currentAction), which normalizes the shortcut (e.g., for selectWorkspaceByNumber the key is normalized to "1"). But immediately after, shortcut = proposedShortcut writes the un-normalized value (e.g., "3") to the binding. The onChange handler in ShortcutSettingRow then calls setShortcut with the un-normalized value and posts another change notification.
The notification from reassignShortcutConflict will eventually correct the displayed shortcut, but for numbered-digit actions this creates a double-write and a brief display inconsistency. Consider reading the canonical stored value instead:
private func reassignConflictingShortcut() {
guard case let .conflictsWithAction(conflictingAction)? = rejectedAttempt?.reason,
let proposedShortcut = rejectedAttempt?.proposedShortcut else {
return
}
let previousShortcut = shortcut
KeyboardShortcutSettings.reassignShortcutConflict(
proposedShortcut: proposedShortcut,
currentAction: action,
conflictingAction: conflictingAction,
previousShortcut: previousShortcut
)
// Use the stored (normalized) shortcut so numbered actions show "1…9" immediately.
shortcut = KeyboardShortcutSettings.shortcut(for: action)
rejectedAttempt = nil
}
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/KeyboardShortcutSettings.swift">
<violation number="1" location="Sources/KeyboardShortcutSettings.swift:2288">
P1: First accepted key stroke is committed immediately, so `pendingChordStart` is never set and chord shortcuts can no longer be recorded.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
7287-7297:⚠️ Potential issue | 🟠 MajorConfirm the swap before clearing the rejection.
swapShortcutConflict(...)is called without an observable success result, but Line 7296 immediately shows the proposed shortcut and Line 7297 clears the inline error. If the swap no-ops because the conflict is stale or blocked by policy, the UI can report a successful reassignment while persisted settings remain unchanged.🐛 Proposed guard against an optimistic swap result
KeyboardShortcutSettings.swapShortcutConflict( proposedShortcut: proposedShortcut, currentAction: action, conflictingAction: conflictingAction, previousShortcut: previousShortcut ) - shortcut = proposedShortcut - rejectedAttempt = nil + let latestShortcut = KeyboardShortcutSettings.shortcut(for: action) + shortcut = latestShortcut + if latestShortcut == proposedShortcut { + rejectedAttempt = nil + }Verify whether
swapShortcutConflictcan fail/no-op and whethercanSwapfully covers those cases:#!/bin/bash # Description: Inspect swap conflict persistence and validation gating. # Expected: either swapShortcutConflict returns/throws success that callers check, # or canSwap proves every call is guaranteed to persist. Otherwise keep the guard above. rg -n -C 6 --type=swift '\bswapShortcutConflict\s*\(|\bfunc\s+swapShortcutConflict\b|\bvar\s+canSwap\b|\bcanSwap\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 7287 - 7297, The call to KeyboardShortcutSettings.swapShortcutConflict(...) is used optimistically: update shortcut and clear rejectedAttempt immediately without verifying the swap actually persisted; update the caller in cmuxApp.swift (around where KeyboardShortcutRecorderActivity.stopAllRecording(), previousShortcut, swapShortcutConflict, shortcut = proposedShortcut, rejectedAttempt = nil are) to only set shortcut and clear rejectedAttempt after swapShortcutConflict indicates success—either by checking a Bool return, handling a thrown error, or by re-querying persisted settings—and if swapShortcutConflict has no result, modify KeyboardShortcutSettings.swapShortcutConflict or expose canSwap to provide an explicit success signal so the UI only updates when the persistence/validation actually succeeded.
🤖 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 7287-7297: The call to
KeyboardShortcutSettings.swapShortcutConflict(...) is used optimistically:
update shortcut and clear rejectedAttempt immediately without verifying the swap
actually persisted; update the caller in cmuxApp.swift (around where
KeyboardShortcutRecorderActivity.stopAllRecording(), previousShortcut,
swapShortcutConflict, shortcut = proposedShortcut, rejectedAttempt = nil are) to
only set shortcut and clear rejectedAttempt after swapShortcutConflict indicates
success—either by checking a Bool return, handling a thrown error, or by
re-querying persisted settings—and if swapShortcutConflict has no result, modify
KeyboardShortcutSettings.swapShortcutConflict or expose canSwap to provide an
explicit success signal so the UI only updates when the persistence/validation
actually succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d607ae37-e60d-4db8-ba17-c5a0b1bc9a0d
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Resources/Localizable.xcstrings
- cmuxTests/WorkspaceUnitTests.swift
- Sources/KeyboardShortcutSettings.swift
…-2988-shortcut-conflict-msg
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 699d76a. Configure here.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…anaflow-ai#3035) * Add shortcut recorder rejection regression tests * Refine shortcut conflict detection for chords and media keys * Add shortcut recorder rejection presentation tests * Explain shortcut recorder rejections inline * Show shortcut conflicts immediately in recorder * Commit recorded shortcut on first valid stroke * Clarify shortcut conflict swap flow * Keep shortcut recorder active for key equivalents * Show press-shortcut prompt after rejection Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Offer Undo link for rejected shortcut recordings Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Add regression test for non-media systemDefined recording Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Guard key-only NSEvent properties against systemDefined events Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary
Verification
Note
Medium Risk
Changes shortcut recording/validation and event monitoring (including
.systemDefinedmedia keys) plus persistence/swap behavior, which could affect input handling and global hotkey registration. Risk is mitigated by expanded unit test coverage but still touches core interaction paths.Overview
Improves the shortcut recorder to surface inline, localized validation instead of failing silently: bare-key rejection, macOS-reserved/system-wide hotkey constraints, numbered shortcut digit requirements, and shortcut conflicts that name the owning action and optionally offer
Swap+Undo.Adds conflict-aware shortcut resolution and swapping in
KeyboardShortcutSettings(including chord vs single-stroke and 1–9 “digit family” matching), updates the settings UI to use a new wrapper that manages rejection state, and prevents app shortcut routing while any recorder is active.Extends recording/matching to support F1–F20 and media keys by handling
.systemDefinedevents and keycode-based matching, and addsen/jastrings plus regression tests for the new behaviors.Reviewed by Cursor Bugbot for commit 8abeae3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds inline, localized validation to the shortcut recorder with instant commit on the first valid stroke, clear conflict messaging with optional “Swap” and “Undo,” and support for F1–F20 and media keys. Fixes #2988; blocks app shortcuts while recording and safely ignores non‑media
.systemDefinedevents.New Features
.systemDefined, match by keyCode, and show proper labels.Bug Fixes
en/jalocalizations.NSEventaccess for.systemDefinedand ignore non‑media system‑defined events; added regression test.Written for commit 8abeae3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation