Preserve context-separated shortcuts through recorder swaps - #3489
Conversation
The shortcut recorder already resolves conflicts through Action.conflicts, but the secondary validation and swap path could act on a conflictsWithAction value without rechecking the current shortcut context. This keeps recorder presentation and swap persistence behind the same context-aware matcher so browser-panel and non-browser-panel actions cannot be treated as conflicts by stale or invalid attempts. Constraint: Issue 3486 requires Cmd+R to remain assignable to renameTab while browserReload keeps Cmd+R because their contexts do not overlap Rejected: Only add the unbind/reassign regression test | leaves the swap/persistence path able to unbind browserReload if handed a stale conflict Confidence: high Scope-risk: narrow Directive: Shortcut conflict UI and persistence must use Action.conflicts before presenting or swapping conflicts; do not bypass shortcutContext.overlaps() Tested: ./scripts/reload.sh --tag issue-3486-shortcut-context-conflict --launch Not-tested: Local XCTest execution per repo testing policy
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds stricter conflict re-validation to shortcut swapping, gates validation UI presentation so conflict prompts only show when a real conflict exists, tightens swap-eligibility guards, and adds tests covering reassignment/unbinding and non-overlapping context behavior. ChangesShortcut conflict validation & presentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/KeyboardShortcutSettings.swift (1)
708-733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t make a stale swap dialog a no-op.
This revalidation is correct for preventing an incorrect unbind, but it also means an already-open swap prompt can silently discard the user’s shortcut if the conflict disappears before the button is pressed. Because the recorder is stopped before this call, there’s no fallback path to apply
proposedShortcuttocurrentAction.Consider moving the current-action persistence ahead of the conflict recheck, or have the caller handle the “no longer conflicts” case explicitly. This keeps the stale prompt from turning into a lost input path.
Also applies to: 2245-2289, 2348-2362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 708 - 733, The current guard that revalidates the conflict can turn a stale swap dialog into a no-op and silently discard the user's proposedShortcut; to fix this either persist the currentAction immediately or explicitly handle the "no longer conflicts" path: call persistShortcut(resolvedCurrentShortcut, for: currentAction) (and postDidChangeNotification(action: currentAction)) before performing conflictingAction.conflicts(...), or add an explicit else branch after the conflicts check that applies proposedShortcut to currentAction (using storedShortcutForReplacement and persistShortcut) when the conflict no longer exists; update the logic around conflictingAction.conflicts(with:proposedShortcut, proposedAction:currentAction, configuredShortcut:shortcut(for:conflictingAction)), storedShortcutForReplacement(...), persistShortcut(...), and postDidChangeNotification(...) accordingly so a closed/stale swap prompt cannot drop the user input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 708-733: The current guard that revalidates the conflict can turn
a stale swap dialog into a no-op and silently discard the user's
proposedShortcut; to fix this either persist the currentAction immediately or
explicitly handle the "no longer conflicts" path: call
persistShortcut(resolvedCurrentShortcut, for: currentAction) (and
postDidChangeNotification(action: currentAction)) before performing
conflictingAction.conflicts(...), or add an explicit else branch after the
conflicts check that applies proposedShortcut to currentAction (using
storedShortcutForReplacement and persistShortcut) when the conflict no longer
exists; update the logic around
conflictingAction.conflicts(with:proposedShortcut, proposedAction:currentAction,
configuredShortcut:shortcut(for:conflictingAction)),
storedShortcutForReplacement(...), persistShortcut(...), and
postDidChangeNotification(...) accordingly so a closed/stale swap prompt cannot
drop the user input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 005722d5-0f32-4963-b06f-788005959a94
📒 Files selected for processing (2)
Sources/KeyboardShortcutSettings.swiftcmuxTests/KeyboardShortcutContextTests.swift
Greptile SummaryThis PR fixes issue #3486 by threading all three shortcut recorder paths ( Confidence Score: 4/5Safe to merge; all findings are P2 — the core fix is logically sound and well-targeted. The context-aware guard is correctly placed in all three affected sites and the new tests exercise the key regression scenario. Two P2 style observations: Sources/KeyboardShortcutSettings.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Recorder UI
participant VP as ShortcutRecorderValidationPresentation
participant KSS as KeyboardShortcutSettings
participant AC as Action.conflicts()
UI->>VP: init(attempt, action, currentShortcut, shortcutForAction)
VP->>AC: shouldPresent — conflictingAction.conflicts(proposedShortcut, proposedAction, configuredShortcut)
alt contexts do NOT overlap (new guard)
AC-->>VP: false → return nil
VP-->>UI: nil (no conflict UI shown)
else contexts overlap
AC-->>VP: true → continue
VP->>AC: canSwapConflict — conflictingAction.conflicts(...) [defensive re-check]
AC-->>VP: true → evaluate resolvedRecordedShortcutIgnoringConflicts
VP-->>UI: ShortcutRecorderValidationPresentation(canSwap: true/false)
UI->>KSS: swapShortcutConflict(proposedShortcut, currentAction, conflictingAction, previousShortcut)
KSS->>AC: conflictingAction.conflicts(proposedShortcut, proposedAction, shortcut(for: conflictingAction))
alt contexts do NOT overlap (new guard)
AC-->>KSS: false → early return (no unbind)
else contexts overlap
AC-->>KSS: true → persist both shortcuts
end
end
Reviews (1): Last reviewed commit: "Preserve context-separated shortcuts thr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let proposedShortcut = attempt.proposedShortcut else { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
shouldPresent silently suppresses conflict UI when proposedShortcut is nil
When attempt.reason is .conflictsWithAction but attempt.proposedShortcut is nil, the new shouldPresent guard returns false, causing the init to return nil and hide the conflict UI entirely. Before this PR, the presentation would still be created (with canSwap = false) because canSwapConflict was the only nil-guard. If a .conflictsWithAction rejection is ever raised without a proposedShortcut — even for actions with overlapping contexts — the conflict warning would now be silently dropped instead of shown to the user. Consider returning true in the nil-proposedShortcut branch to preserve the pre-existing behavior for the overlapping-context case.
| guard conflictingAction.conflicts( | ||
| with: proposedShortcut, | ||
| proposedAction: action, | ||
| configuredShortcut: shortcutForAction(conflictingAction) | ||
| ) else { | ||
| return false | ||
| } |
There was a problem hiding this comment.
conflicts check already guaranteed by shouldPresent
canSwapConflict is only reachable after shouldPresent has already verified that conflictingAction.conflicts(...) returns true (otherwise the init returns nil). The duplicated guard here is harmless but adds noise. If canSwapConflict is ever intended to be called independently, a comment explaining the defensive duplication would help; otherwise, this guard can safely be removed.
| ), | ||
| action: .renameTab, | ||
| currentShortcut: .unbound, | ||
| shortcutForAction: { $0.defaultShortcut } |
There was a problem hiding this comment.
defaultShortcut closure instead of live shortcut(for:) lookup
ShortcutRecorderValidationPresentation is initialized with shortcutForAction: { $0.defaultShortcut }, but production callers pass KeyboardShortcutSettings.shortcut(for:). In this particular test the two are equivalent (all shortcuts are at defaults), but using the live lookup would make the test a more faithful regression guard. If a future change introduces a non-default initial state for some action, the test could silently pass while the real UI diverges.
| shortcutForAction: { $0.defaultShortcut } | |
| shortcutForAction: KeyboardShortcutSettings.shortcut(for:) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 4fd069a. Configure here.
| configuredShortcut: shortcutForAction(conflictingAction) | ||
| ) else { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Redundant conflict check in canSwapConflict
Low Severity
The conflictingAction.conflicts(with:proposedAction:configuredShortcut:) guard in canSwapConflict is unreachable because shouldPresent already checks the identical condition with the same shortcutForAction closure earlier in init, returning nil before canSwapConflict is ever called. Since canSwapConflict is private static with a single call site, this duplicated check can never fail and risks diverging if only one copy is updated in the future.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4fd069a. Configure here.
The shortcut recorder presentation helper belongs with the settings control that renders it, and moving it out of the large settings model keeps the new context-separation regression from tripping the CI file-length guard without widening the budget. Constraint: workflow-guard-tests enforces .github/swift-file-length-budget.tsv for tracked Swift files Rejected: Increase the file-length budget | the change can be represented as a mechanical relocation Confidence: high Scope-risk: narrow Directive: Keep future recorder UI presentation helpers out of KeyboardShortcutSettings.swift unless they are model state Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: ./scripts/reload.sh --tag fix-pr-ci-shortcut-budget Not-tested: Local XCTest suites per project testing policy
This branch was successfully deployed
1 participant
Summary
Action.conflicts(context-aware) matcher used for primary conflict detection, so a stale or invalidconflictsWithActioncannot unbind a shortcut whose context does not actually overlap.renameTabwhilebrowserReloadkeeps Cmd+R, because their shortcut contexts do not overlap.KeyboardShortcutContextTestscoverage for the swap/persistence path so a stale conflict can no longer silently unbind a context-isolated action.Test plan
KeyboardShortcutContextTests) passrenameTabto Cmd+R and confirmbrowserReloadis not unbound🤖 Generated with Claude Code
Note
Medium Risk
Touches shortcut conflict detection and swap persistence; behavior changes could affect how existing shortcuts are accepted/rejected, but scope is limited and covered by new unit tests.
Overview
Fixes a bug where the shortcut recorder’s swap/secondary validation path could treat a stale
conflictsWithActionas real and incorrectly unbind/reassign shortcuts even when their contexts don’t overlap.swapShortcutConflictand the recorder validation presentation now re-check conflicts viaAction.conflicts(context-aware), and tests add coverage for reassigningCmd+RbetweenrenameTabandbrowserReloadplus ensuring swap logic ignores non-overlapping contexts.Reviewed by Cursor Bugbot for commit c4ee643. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Preserves context-separated shortcuts during recorder validation, presentation, and swaps. Fixes #3486 by allowing Cmd+R to be assigned to
renameTabwithout unbindingbrowserReloadwhen their contexts don’t overlap.Bug Fixes
Action.conflictscheck to ignore non-overlapping contexts. Added tests for Cmd+R reassignment and the swap path across browser vs non-browser panels.Refactors
ShortcutRecorderValidationPresentationtoKeyboardShortcutSettingsControls.swiftand gated presentation withAction.conflictsto avoid showing conflict UI for non-overlapping contexts and meet the Swift file-length budget.Written for commit c4ee643. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests