Fix editable shortcuts from settings.json - #3462
Conversation
The settings file is a default source, not a lock on shortcut editing. This regression test captures the expected UI override precedence and the absence of a managed subtitle before the production code is changed. Constraint: Issue #3461 requires a failing regression test before the fix. Confidence: high Scope-risk: narrow Tested: Not run; regression-only commit is intended to fail before the fix. Not-tested: Full unit suite and app build deferred until fix commit.
File-configured shortcuts are defaults, not an authority boundary. The shortcut resolver now checks UserDefaults first, then the settings file, then built-in defaults, so Settings edits and conflict swaps use the existing UI persistence path without rewriting settings.json. Constraint: Issue #3461 requires file-set shortcuts to remain editable from Settings. Rejected: Keep file precedence with override tombstones | adds a second exception layer for the same shortcut value. Confidence: high Scope-risk: narrow Directive: Do not use settings-file provenance to disable shortcut UI writes; it is only a lower-priority default source. Tested: git diff --check; jq empty Resources/Localizable.xcstrings Not-tested: Local unit suite per repo testing policy; final tagged reload build still pending.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReorders keyboard-shortcut resolution to prefer persisted UserDefaults over settings-file overrides, removes UI disabling/subtitle for settings-file-managed shortcuts, allows swap/persist even when actions were file-managed, adds tests (including one ensuring file-store parsing doesn't consult live store), and deletes a localization key. ChangesShortcut resolution, UI, and swap behavior
File-store parsing isolation test
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 |
Greptile SummaryThis PR fixes the shortcut priority order so that Settings UI edits (stored in UserDefaults) win over Confidence Score: 4/5Safe to merge; the priority inversion is correctly implemented, tests are isolated via resetAll(), and no production call site is broken. Only P2 findings (dead-code stub settingsFileManagedSubtitle that always returns nil). The core logic change is well-tested with new cases covering override, swap, and reset interactions. Sources/KeyboardShortcutSettings.swift — the permanently-nil settingsFileManagedSubtitle stub (and its mirror in SystemWideHotkeySettings) can be cleaned up. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["shortcut(for: action)"] --> B{UserDefaults\nhas value?}
B -- Yes --> C[Return UserDefaults shortcut]
B -- No --> D{settings.json\nhas override?}
D -- Yes --> E[Return settingsFile shortcut]
D -- No --> F[Return action.defaultShortcut]
G["setShortcut(_:for:)"] --> H[Write to UserDefaults\nalways — no managed guard]
I["resetShortcut(for:)"] --> J[Remove UserDefaults entry]
J --> K[settingsFile default\nnow visible again]
Reviews (1): Last reviewed commit: "Let UI shortcut edits override file defa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func settingsFileManagedSubtitle(for _: Action) -> String? { | ||
| nil | ||
| } |
There was a problem hiding this comment.
settingsFileManagedSubtitle(for:) now unconditionally returns nil, making it dead code. Since it's no longer referenced from cmuxApp.swift or any production call site (only in the stub on SystemWideHotkeySettings at line 818 and in test assertions), both can be deleted. Keeping a permanently-nil function risks future callers incorrectly assuming it carries meaning.
![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)
632-660:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnore semantically invalid persisted overrides before they win precedence.
These paths now return any decodable
UserDefaultspayload ahead ofsettings.jsonand built-in defaults. For.showHideAllWindowsand numbered actions, a stale or hand-edited value can therefore mask a valid file/default shortcut indefinitely; for the global hotkey, that also means registration stays disabled instead of falling back.Suggested fix
fileprivate static func persistedShortcut( for action: Action, defaults: UserDefaults = .standard ) -> StoredShortcut? { guard let data = defaults.data(forKey: action.defaultsKey), - let shortcut = try? JSONDecoder().decode(StoredShortcut.self, from: data) else { + let shortcut = try? JSONDecoder().decode(StoredShortcut.self, from: data) else { return nil } - return shortcut + return storedShortcutForPersistence(shortcut, action: action) }Also applies to: 844-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettings.swift` around lines 632 - 660, persistedShortcut(for:) and shortcut(for:) currently accept any decodable StoredShortcut from UserDefaults and let it take precedence over settingsFileStore.override(for:) and action.defaultShortcut, which allows semantically invalid/stale overrides (e.g., for .showHideAllWindows, numbered actions, or global hotkey) to disable or mask valid shortcuts; update shortcut(for:) (and the analogous logic at the other location mentioned) to validate the decoded StoredShortcut before returning it — call the existing validation logic (or reproduce the same checks you use when loading settingsFileStore overrides) to ensure the persisted shortcut is semantically valid for the given Action, and only return/persist it if valid, otherwise ignore the persisted value and fall back to settingsFileStore.override(for:) or action.defaultShortcut.
🧹 Nitpick comments (2)
cmuxTests/WorkspaceUnitTests.swift (2)
1044-1053: ⚡ Quick winAdd
isManagedBySettingsFileassertion after the live UI override.The test verifies that the UI shortcut wins and the settings file is not rewritten, but it doesn't confirm that
isManagedBySettingsFile(.newWindow)remainstrueaftersetShortcut. The companion testtestPersistedShortcutOverridesSettingsFileDefault(line 1015) checks this invariant for the pre-load scenario; a symmetric check here would catch an implementation regression where a live edit accidentally clears the managed state (which would silently break the reset-reveal behavior tested elsewhere).♻️ Proposed addition
XCTAssertEqual(KeyboardShortcutSettings.shortcut(for: .newWindow), uiShortcut) + XCTAssertTrue(KeyboardShortcutSettings.isManagedBySettingsFile(.newWindow)) XCTAssertEqual( try String(contentsOf: settingsFileURL, encoding: .utf8), originalSettingsFileContents )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 1044 - 1053, After verifying the UI shortcut override, add an assertion that the managed-state is preserved: call KeyboardShortcutSettings.isManagedBySettingsFile(.newWindow) after KeyboardShortcutSettings.setShortcut(uiShortcut, for: .newWindow) and assert it remains true so live UI overrides don't clear the settings-file-managed flag (relate to KeyboardShortcutSettings.shortcut(for:), setShortcut(_:for:), and isManagedBySettingsFile(_:)).
1118-1127: ⚡ Quick winVerify the settings file is not mutated by
swapShortcutConflict.
testUserDefaultsShortcutOverrideWinsOverSettingsFileShortcutcapturesoriginalSettingsFileContentsand asserts file immutability after a singlesetShortcutcall. The same contract applies toswapShortcutConflict(both shortcuts are file-backed; the PR specifically calls out that swap behavior must work consistently for file-backed defaults without rewriting the file), but this test has no equivalent check. A regression whereswapShortcutConflictwrites back to the file would go undetected.♻️ Proposed addition
+ let originalSettingsFileContents = try String(contentsOf: settingsFileURL, encoding: .utf8) + KeyboardShortcutSettings.settingsFileStore = KeyboardShortcutSettingsFileStore( primaryPath: settingsFileURL.path, fallbackPath: nil, startWatching: false ) KeyboardShortcutSettings.swapShortcutConflict( proposedShortcut: proposedNewWindowShortcut, currentAction: .newWindow, conflictingAction: .newTab, previousShortcut: previousNewWindowShortcut ) XCTAssertEqual(KeyboardShortcutSettings.shortcut(for: .newWindow), proposedNewWindowShortcut) XCTAssertEqual(KeyboardShortcutSettings.shortcut(for: .newTab), previousNewWindowShortcut) + XCTAssertEqual( + try String(contentsOf: settingsFileURL, encoding: .utf8), + originalSettingsFileContents + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 1118 - 1127, Add an assertion that the settings file is not mutated by KeyboardShortcutSettings.swapShortcutConflict: before calling swapShortcutConflict in the test, capture the file contents exactly as done in testUserDefaultsShortcutOverrideWinsOverSettingsFileShortcut (e.g. originalSettingsFileContents) and after the swap assert the on-disk contents are equal to originalSettingsFileContents; keep the existing assertions that KeyboardShortcutSettings.shortcut(for: .newWindow) and .newTab reflect the in-memory swap but ensure swapShortcutConflict does not rewrite the file-backed settings.
🤖 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 632-660: persistedShortcut(for:) and shortcut(for:) currently
accept any decodable StoredShortcut from UserDefaults and let it take precedence
over settingsFileStore.override(for:) and action.defaultShortcut, which allows
semantically invalid/stale overrides (e.g., for .showHideAllWindows, numbered
actions, or global hotkey) to disable or mask valid shortcuts; update
shortcut(for:) (and the analogous logic at the other location mentioned) to
validate the decoded StoredShortcut before returning it — call the existing
validation logic (or reproduce the same checks you use when loading
settingsFileStore overrides) to ensure the persisted shortcut is semantically
valid for the given Action, and only return/persist it if valid, otherwise
ignore the persisted value and fall back to settingsFileStore.override(for:) or
action.defaultShortcut.
---
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 1044-1053: After verifying the UI shortcut override, add an
assertion that the managed-state is preserved: call
KeyboardShortcutSettings.isManagedBySettingsFile(.newWindow) after
KeyboardShortcutSettings.setShortcut(uiShortcut, for: .newWindow) and assert it
remains true so live UI overrides don't clear the settings-file-managed flag
(relate to KeyboardShortcutSettings.shortcut(for:), setShortcut(_:for:), and
isManagedBySettingsFile(_:)).
- Around line 1118-1127: Add an assertion that the settings file is not mutated
by KeyboardShortcutSettings.swapShortcutConflict: before calling
swapShortcutConflict in the test, capture the file contents exactly as done in
testUserDefaultsShortcutOverrideWinsOverSettingsFileShortcut (e.g.
originalSettingsFileContents) and after the swap assert the on-disk contents are
equal to originalSettingsFileContents; keep the existing assertions that
KeyboardShortcutSettings.shortcut(for: .newWindow) and .newTab reflect the
in-memory swap but ensure swapShortcutConflict does not rewrite the file-backed
settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7c2085d-1f11-4492-8182-e08f7f620426
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
![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 6d36488. Configure here.
Settings-file shortcut parsing ran through the same conflict resolver used by interactive shortcut recording. During app startup that resolver can query KeyboardShortcutSettings.settingsFileStore while CmuxSettingsFileStore.shared is still initializing, which trips libdispatch's recursive once guard. The parser now applies only action-local normalization for managed settings file values, preserving numbered shortcut canonicalization and invalid managed global-hotkey behavior without consulting the live shortcut store. Constraint: Startup parsing must not depend on the KeyboardShortcutSettings.settingsFileStore singleton it is initializing Rejected: Add an initialization flag around conflict detection | preserves the circular dependency and risks parser behavior changing by call timing Confidence: high Scope-risk: narrow Directive: Keep settings-file parsing independent from live shortcut lookup; conflict validation belongs in interactive recording/persistence paths Tested: ./scripts/reload.sh --tag codex-launch --launch; process stayed alive after startup Not-tested: Local XCTest suite per repo policy
Pulled origin/main after committing the startup parser fix and resolved overlapping settings shortcut conflicts in favor of the newer main structure. The pulled main line moves shortcut recorder/settings-row UI into dedicated files, treats cmux.json shortcuts as managed settings, and adds settings navigation/search surfaces. The parser keeps the non-recursive managed-file normalization path from main and retains the numbered-shortcut regression coverage adapted to cmux.json. Constraint: Pull was requested after the local crash fix commit Rejected: Keep the older inline settings modal shortcut controls | main has already split them into dedicated components and added managed-file UI behavior Confidence: medium Scope-risk: broad Directive: Settings modal appearance differences after this merge are expected from origin/main's SettingsNavigation, SettingsShellLab, and KeyboardShortcutSettingsControls changes Tested: git diff --cached --check Not-tested: Local XCTest/build suite per repo policy
The PR failed CI's Swift file-length budget because the new managed-shortcut parser regression was added to WorkspaceUnitTests.swift, which is already budgeted exactly at its current size. Move the same behavioral coverage into KeyboardShortcutSettingsFileStoreMigrationTests.swift, a smaller settings-file test file already included in the Xcode project, and restore WorkspaceUnitTests.swift to its budgeted length. Constraint: CI enforces .github/swift-file-length-budget.tsv and rejects growth in oversized Swift files Rejected: Refresh the file-length budget | this regression test can live in a smaller settings-file test file without accepting new debt Confidence: high Scope-risk: narrow Directive: Keep new settings-file parser coverage out of WorkspaceUnitTests.swift unless the file is split or the budget is intentionally refreshed Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; git diff --check Not-tested: Local XCTest suite per repo policy
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/KeyboardShortcutSettingsFileStoreMigrationTests.swift (1)
123-127: ⚡ Quick winPass
additionalFallbackPaths: []to both test stores to ensure full isolation.Neither
KeyboardShortcutSettingsFileStoreinitializer call in this test passesadditionalFallbackPaths: [], so both inherit the default — which includesCmuxSettingsFileStore.defaultApplicationSupportFallbackPath. On a developer machine (or a CI agent that has run cmux before), a real settings file at that path could bleed into the test's resolved shortcuts, potentially producing a false pass or false fail on thekey: "1"assertion. This is especially important for a test whose explicit purpose is store isolation.♻️ Proposed fix
- KeyboardShortcutSettings.settingsFileStore = KeyboardShortcutSettingsFileStore( - primaryPath: liveSettingsFileURL.path, - fallbackPath: nil, - startWatching: false - ) + KeyboardShortcutSettings.settingsFileStore = KeyboardShortcutSettingsFileStore( + primaryPath: liveSettingsFileURL.path, + fallbackPath: nil, + additionalFallbackPaths: [], + startWatching: false + )- let store = KeyboardShortcutSettingsFileStore( - primaryPath: settingsFileURL.path, - fallbackPath: nil, - startWatching: false - ) + let store = KeyboardShortcutSettingsFileStore( + primaryPath: settingsFileURL.path, + fallbackPath: nil, + additionalFallbackPaths: [], + startWatching: false + )Also applies to: 141-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/KeyboardShortcutSettingsFileStoreMigrationTests.swift` around lines 123 - 127, The test creates two KeyboardShortcutSettingsFileStore instances but omits additionalFallbackPaths, so they inherit CmuxSettingsFileStore.defaultApplicationSupportFallbackPath and may read real user data; update both initializer calls for KeyboardShortcutSettingsFileStore (the one assigned to KeyboardShortcutSettings.settingsFileStore and the other later in the test) to pass additionalFallbackPaths: [] to ensure full isolation of the stores and deterministic test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/KeyboardShortcutSettingsFileStoreMigrationTests.swift`:
- Around line 123-127: The test creates two KeyboardShortcutSettingsFileStore
instances but omits additionalFallbackPaths, so they inherit
CmuxSettingsFileStore.defaultApplicationSupportFallbackPath and may read real
user data; update both initializer calls for KeyboardShortcutSettingsFileStore
(the one assigned to KeyboardShortcutSettings.settingsFileStore and the other
later in the test) to pass additionalFallbackPaths: [] to ensure full isolation
of the stores and deterministic test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 236308ef-43b6-4a56-bce6-3d38b4fac9ec
📒 Files selected for processing (1)
cmuxTests/KeyboardShortcutSettingsFileStoreMigrationTests.swift
The PR now expects settings-file shortcut bindings to act as defaults rather than locks. These assertions cover UserDefaults precedence, UI edits, reset fallback, and removal of the managed-file subtitle expectation. Constraint: Regression-test policy asks for a failing test commit before the fix commit Confidence: high Scope-risk: narrow Directive: Keep settings-file shortcut tests behavior-level rather than source-shape checks Tested: Not run independently; this commit is intended to fail before the implementation commit Not-tested: Full unit suite for this intermediate failing commit
Settings-file shortcut bindings now provide fallback defaults after UserDefaults, so Settings edits persist without rewriting cmux.json and reset reveals the file-provided value again. The recorder rows no longer show or enforce the old managed-file disabled state, and the unused localized subtitle string is removed. Constraint: Settings UI edits must not rewrite cmux.json while still allowing reset to reveal the file default Rejected: Treat cmux.json shortcuts as managed immutable values | blocked user edits from Settings and conflicted with issue #3461 Confidence: high Scope-risk: narrow Directive: Preserve UserDefaults > cmux.json > built-in default shortcut precedence Tested: git diff --check; jq empty Resources/Localizable.xcstrings; ./scripts/reload.sh --tag issue-3461-shortcut-file-managed-editable Not-tested: Local XCTest suite per repo policy; CI will run PR checks
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/WorkspaceUnitTests.swift (1)
1064-1073: ⚡ Quick winConsider adding an
isManagedBySettingsFileassertion aftersetShortcutto make the invariant explicit.After line 1067, the test has verified that
setShortcutstores the UI-edited value over the file default, but it does not directly assert that the action is still considered file-managed at that point. The reset path at line 1073 provides indirect evidence (it returnsmanagedShortcut, which only happens if the file-managed status is preserved), but an explicit check would make the intent self-documenting.🔍 Suggested addition
XCTAssertEqual(KeyboardShortcutSettings.shortcut(for: .newTab), editedShortcut) + XCTAssertTrue(KeyboardShortcutSettings.isManagedBySettingsFile(.newTab), + "setShortcut must not strip file-managed status") KeyboardShortcutSettings.resetShortcut(for: .newTab)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 1064 - 1073, Add an explicit assertion that the shortcut remains file-managed after calling KeyboardShortcutSettings.setShortcut(editedShortcut, for: .newTab) by checking KeyboardShortcutSettings.isManagedBySettingsFile(for: .newTab) immediately after the set; this makes the invariant explicit before calling resetShortcut(for: .newTab) and complements the existing XCTAssertEqual that verifies the stored value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 1064-1073: Add an explicit assertion that the shortcut remains
file-managed after calling KeyboardShortcutSettings.setShortcut(editedShortcut,
for: .newTab) by checking KeyboardShortcutSettings.isManagedBySettingsFile(for:
.newTab) immediately after the set; this makes the invariant explicit before
calling resetShortcut(for: .newTab) and complements the existing XCTAssertEqual
that verifies the stored value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7abfabc5-0c3e-43d7-b13e-b9effcaa9f93
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsControls.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceUnitTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
This branch was successfully deployed
1 participant
Summary
Fixes #3461
Verification
git diff --checkjq empty Resources/Localizable.xcstrings./scripts/reload.sh --tag issue-3461-shortcut-file-managed-editableCommit structure
d28dc22a: failing regression tests only6d364889: implementation fix3ded8224: parser/startup regression fix7d22d1b5: move parser regression test out of oversized fileea1a48e8: failing regression expectation for editable file-backed shortcuts29422515: implementation/localization cleanupNote
Low Risk
Low risk: test-only change that adds coverage around keyboard shortcut settings parsing behavior without modifying production logic.
Overview
Adds a new regression test in
KeyboardShortcutSettingsFileStoreMigrationTeststo ensure parsing shortcuts from a settings file does not consult the live/globalKeyboardShortcutSettings.settingsFileStore, guarding against recursive initialization and verifying numbered-shortcut canonicalization (e.g.,cmd+2-> workspace1).Reviewed by Cursor Bugbot for commit 7d22d1b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Treat
settings.jsonshortcuts as defaults so Settings edits saved in UserDefaults win without rewriting the file, and remove the managed/disabled UI. Startup parsing is now independent of the live store to avoid recursive init; numbered shortcuts still canonicalize. Fixes #3461.settings.json> built-in defaults (for app shortcuts and the system-wide hotkey).settings.shortcuts.managedByFile.KeyboardShortcutSettingsFileStoreMigrationTests.swiftto satisfy the CI file-length budget.Written for commit 2942251. Summary will update on new commits.
Summary by CodeRabbit
UI Changes
Behavior Changes
Tests