Repository navigation
Honor Settings rebinding of Global Search (parse package object-form cmux.json bindings) - #5143
Conversation
… dropped
The in-app Settings UI (CmuxSettings package) persists shortcut rebindings
to cmux.json under shortcuts.bindings.<action> as nested StoredShortcut
objects ({"first": {key, command, ...}}), but the legacy
KeyboardShortcutSettingsFileStore parser — which feeds KeyboardShortcutSettings
and thus the system-wide Carbon hotkeys (globalSearch, showHideAllWindows) —
only understands the human-editable "cmd+opt+f" string form. The object form is
silently dropped, so SystemWideHotkeyController never sees a Global Search
rebinding and the default ⌥⌘F keeps opening Global Search.
This commit adds the regression coverage only (red); the parser fix follows.
Issue: #5137
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eader
KeyboardShortcutSettingsFileStore is the parser that feeds
KeyboardShortcutSettings.shortcut(for:) and therefore SystemWideHotkeyController.
It understood only the human-editable string ("cmd+opt+f") and string-array
chord forms of a cmux.json shortcut binding. But the in-app Settings UI lives in
the CmuxSettings package, which serializes each binding as a nested StoredShortcut
object ({"first": {key, command, ...}, "second": {...}?}) under
shortcuts.bindings.<action>. Those objects were silently dropped, so a rebinding
made in Settings never reached the store: the global-search Carbon hotkey kept
its built-in ⌥⌘F default and Global Search kept opening on ⌥⌘F after a rebind
(and showHideAllWindows had the same latent gap).
Teach the reader to decode the package object form (including the empty-first-key
"unbound" marker and two-stroke chords) so every action resolved through this
store honors a Settings rebinding. This converges the two cmux.json readers on a
single on-disk contract instead of patching one shortcut.
Fixes #5137
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughKeyboardShortcutSettingsFileStore now decodes shortcut bindings encoded as dictionaries with nested ChangesObject-form shortcut binding support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (15 passed)
✨ 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 SummaryTeaches
Confidence Score: 5/5Safe to merge — the change is a pure additive parsing branch in the settings file store with no effect on existing string or array shortcut forms, and the previous-round issues (chord degradation and UInt16 wrapping) are confirmed fixed in this commit. The fix is a self-contained parser addition inside parseShortcutBindingValue. It adds a new dictionary branch after the existing string and string-array paths, so pre-existing JSON forms are completely unaffected. All edge cases in the new path — missing first, malformed strokes, empty-key unbound marker, bare-key rejection, chord invalidation on a bad second stroke, and out-of-range keyCode — are explicitly handled and covered by the five new regression tests. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmux.json shortcuts.bindings.action] --> B{parseShortcutBindingValue}
B -->|NSNull| C[.unbound]
B -->|String e.g. cmd+opt+f| D[StoredShortcut.parseConfig string]
B -->|String array e.g. ctrl+b n| E[StoredShortcut.parseConfig strokes]
B -->|Dictionary object form| F[parseShortcutObjectForm]
F --> G{first key exists?}
G -->|no| H[nil — log ignored]
G -->|yes| I{parseShortcutStrokeObject first}
I -->|nil / malformed| H
I -->|key empty| C
I -->|valid| J{bare key allowed?}
J -->|no: action requires modifier| H
J -->|yes| K{second key in object?}
K -->|absent or null| L[StoredShortcut first only]
K -->|present| M{parseShortcutStrokeObject second}
M -->|nil / malformed| H
M -->|valid| N[StoredShortcut first + second chord]
L --> O[normalizedSettingsFileShortcut]
N --> O
O --> P[SystemWideHotkeyController registers Carbon hotkey]
Reviews (2): Last reviewed commit: "Harden object-form shortcut parsing (rev..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let second = object["second"].flatMap(parseShortcutStrokeObject) | ||
| return StoredShortcut(first: first, second: second) |
There was a problem hiding this comment.
If object["second"] exists but parseShortcutStrokeObject returns nil (e.g., the dict is missing its "key" field), second silently becomes nil and parseShortcutObjectForm returns a single-stroke StoredShortcut. The outer caller in parseShortcutBindingValue sees a non-nil result and skips the "ignoring invalid shortcut binding" log, so an intended chord binding quietly degrades to a single key with no diagnostic. Consider logging a warning when object["second"] != nil but the stroke couldn't be decoded.
There was a problem hiding this comment.
Fixed in 00ca077 — a present-but-malformed second stroke now invalidates the whole binding instead of degrading the chord to a single stroke.
— Claude Code
| shift: jsonBool(dict["shift"]) ?? false, | ||
| option: jsonBool(dict["option"]) ?? false, | ||
| control: jsonBool(dict["control"]) ?? false, | ||
| keyCode: jsonInt(dict["keyCode"]).map { UInt16(truncatingIfNeeded: $0) } |
There was a problem hiding this comment.
UInt16(truncatingIfNeeded:) silently bit-pattern wraps any Int outside 0…65535. If cmux.json ever carries a negative or oversized keyCode (e.g., hand-edited or written by a future schema change), the result is a quietly wrong hardware key code that gets registered with Carbon — potentially stealing an unrelated hotkey. flatMap { UInt16(exactly: $0) } keeps the same nil-for-missing behaviour while rejecting out-of-range values cleanly.
| keyCode: jsonInt(dict["keyCode"]).map { UInt16(truncatingIfNeeded: $0) } | |
| keyCode: jsonInt(dict["keyCode"]).flatMap { UInt16(exactly: $0) } |
There was a problem hiding this comment.
Fixed in 00ca077 — switched to UInt16(exactly:); an out-of-range keyCode now rejects the binding instead of bit-wrapping to a different key.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 2 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="cmuxTests/GlobalSearchShortcutSettingsTests.swift">
<violation number="1" location="cmuxTests/GlobalSearchShortcutSettingsTests.swift:97">
P3: New unit tests use XCTest. Repo rule says new non-UI tests should use Swift Testing. Migrate these cases to `@Test` style.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ) | ||
| } | ||
|
|
||
| func testSettingsFileStoreParsesPackageObjectFormGlobalSearchShortcut() throws { |
There was a problem hiding this comment.
P3: New unit tests use XCTest. Repo rule says new non-UI tests should use Swift Testing. Migrate these cases to @Test style.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GlobalSearchShortcutSettingsTests.swift, line 97:
<comment>New unit tests use XCTest. Repo rule says new non-UI tests should use Swift Testing. Migrate these cases to `@Test` style.</comment>
<file context>
@@ -94,6 +94,121 @@ final class GlobalSearchShortcutSettingsTests: XCTestCase {
)
}
+ func testSettingsFileStoreParsesPackageObjectFormGlobalSearchShortcut() throws {
+ // Regression for https://github.com/manaflow-ai/cmux/issues/5137.
+ // The in-app Settings UI (CmuxSettings package) persists every
</file context>
There was a problem hiding this comment.
Intentional: this extends the existing XCTest suite GlobalSearchShortcutSettingsTests (an XCTestCase). The repo rule migrates files to Swift Testing incrementally when edited and says not to bulk-rewrite untouched tests; mixing @test into an existing XCTestCase isn't possible, so new methods match the file's framework.
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Address greptile/cubic review findings on the new cmux.json object-form parser: - Reject a present-but-malformed `second` stroke instead of silently degrading a chord to a single-stroke binding. - Reject an out-of-range `keyCode` (UInt16(exactly:)) instead of bit-pattern wrapping it into a different physical key. - Apply the same bare-first-stroke rule the string parser uses (StoredShortcut.parseConfig): an action that requires a modifier rejects a bare key in the object form too. Adds regression coverage for the malformed-chord and bare-key rejections. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 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="cmuxTests/GlobalSearchShortcutSettingsTests.swift">
<violation number="1" location="cmuxTests/GlobalSearchShortcutSettingsTests.swift:212">
P2: New tests use XCTest instead of Swift Testing. Project policy requires Swift Testing for new non-CLI unit tests.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| XCTAssertEqual(store.override(for: .globalSearch), .unbound) | ||
| } | ||
|
|
||
| func testSettingsFileStoreRejectsObjectFormChordWithMalformedSecondStroke() throws { |
There was a problem hiding this comment.
P2: New tests use XCTest instead of Swift Testing. Project policy requires Swift Testing for new non-CLI unit tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GlobalSearchShortcutSettingsTests.swift, line 212:
<comment>New tests use XCTest instead of Swift Testing. Project policy requires Swift Testing for new non-CLI unit tests.</comment>
<file context>
@@ -209,6 +209,66 @@ final class GlobalSearchShortcutSettingsTests: XCTestCase {
XCTAssertEqual(store.override(for: .globalSearch), .unbound)
}
+ func testSettingsFileStoreRejectsObjectFormChordWithMalformedSecondStroke() throws {
+ // A present-but-malformed `second` stroke must invalidate the whole
+ // binding rather than silently degrading the chord to a single stroke
</file context>
This branch was successfully deployed
1 participant
Summary
Fixes #5137 — pressing ⌥⌘F kept opening Global Search even after rebinding the Global Search shortcut in Settings → Keyboard Shortcuts, so the binding was effectively hardcoded.
Root cause (dual source of truth, divergent on-disk schema)
The keyboard-shortcut system is mid-migration into the
CmuxSettings/CmuxSettingsUIpackages, and the in-app Settings UI now lives in the package. When you rebind a shortcut, the package persists it to~/.config/cmux/cmux.jsonundershortcuts.bindings.<action>as a nestedStoredShortcutobject:{ "shortcuts": { "bindings": { "globalSearch": { "first": { "key": "j", "command": true, "control": true } } } } }But
SystemWideHotkeyController(the CarbonRegisterEventHotKeyowner for the system-wide hotkeys) resolves its shortcut through the legacyKeyboardShortcutSettings→KeyboardShortcutSettingsFileStore, whoseparseShortcutBindingValueonly understood the human-editable string form ("cmd+opt+f") and string-array chords. It returnednilfor the object form and logged "ignoring invalid shortcut binding", so the rebinding never reached the store — the controller kept the built-in ⌥⌘F default registered.This is a class of bug, not a one-off: every action resolved through the legacy file store (the two system-wide Carbon hotkeys
globalSearchandshowHideAllWindows) silently ignored any rebinding made in the package Settings UI.globalSearchis the visible victim because it is always enabled;showHideAllWindowsis off by default so the same gap went unnoticed.Reproduced + verified locally (running Debug app)
Driving the real Carbon hotkey via synthesized
CGEvents and reading the in-appglobalHotkey.*debug log:shortcuts.bindings.globalSearchin cmux.json{"first":{"key":"j","command":true,"control":true}}(what the Settings UI writes) — before fix"ctrl+cmd+j"(legacy string form)Fix
Teach
KeyboardShortcutSettingsFileStoreto decode the package's nested object form ({ "first": { stroke }, "second": { stroke }? }), including the empty-first-key "unbound" marker and two-stroke chords. The legacy reader now understands every form that can legitimately appear in cmux.json, converging both cmux.json readers on one on-disk contract instead of patching a single shortcut.Tests
Two-commit structure (failing test → fix). Added behavior-level regression coverage to
cmuxTests/GlobalSearchShortcutSettingsTests.swiftthat exercisesKeyboardShortcutSettingsFileStore.override(for:)through cmux.json:testSettingsFileStoreParsesPackageObjectFormGlobalSearchShortcut— fails before the fix (returns the default instead of the rebinding), passes after.testSettingsFileStoreParsesPackageObjectFormChordShortcut— a non-system-wide action, proving the fix is general.testSettingsFileStoreParsesPackageObjectFormUnboundShortcut— the explicit "no shortcut" marker.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized settings-file parsing with strict validation and broad test coverage; no auth or data-path changes.
Overview
The legacy
KeyboardShortcutSettingsFileStorepath only accepted string / string-array shortcut bindings incmux.json, so rebinding in the CmuxSettings Settings UI (nested{"first": {...}, "second": {...}?}objects undershortcuts.bindings) was dropped and system-wide shortcuts like Global Search kept their built-in defaults.This PR adds object-form decoding in
parseShortcutBindingValue, including unbound (emptyfirst.key), two-stroke chords, and the same validation as the string parser (bare keys for modifier-required actions, reject malformedsecondinstead of degrading to a single stroke, strictkeyCodehandling). Regression tests inGlobalSearchShortcutSettingsTestscover global search, chords, unbound, and rejection cases.Reviewed by Cursor Bugbot for commit 00ca077. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #5137 by making Global Search honor the shortcut set in Settings. The legacy reader now parses the object-form bindings the
CmuxSettingsUI writes tocmux.json, so ⌥⌘F no longer triggers after a rebind.KeyboardShortcutSettingsFileStore, including the empty "unbound" marker, two-stroke chords, and validation (reject malformedsecond, out-of-rangekeyCode, and bare first-stroke when a modifier is required).Written for commit 00ca077. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests