Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions Sources/KeyboardShortcutSettingsFileStore.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1004,6 +1004,17 @@ final class CmuxSettingsFileStore {
allowBareFirstStroke: action.allowsBareFirstStroke
)
}
// Object form written by the CmuxSettings package recorder (the
// in-app Settings UI): { "first": { key, command, ... }, "second": { ... }? }.
// The package serializes StoredShortcut as nested stroke objects, so
// a rebinding made in Settings only reaches this store in that shape.
// Decode it here so every action resolved through this store — most
// visibly the system-wide Carbon hotkeys (globalSearch,
// showHideAllWindows) — honors the rebinding instead of silently
// dropping it and falling back to the built-in default.
if let object = rawValue as? [String: Any] {
return parseShortcutObjectForm(object, action: action)
}
return nil
}()

Expand All @@ -1013,6 +1024,70 @@ final class CmuxSettingsFileStore {
return action.normalizedSettingsFileShortcut(shortcut)
}

/// Decodes the nested-object binding the CmuxSettings package writes
/// (`{ "first": { stroke }, "second": { stroke }? }`) into the app-target
/// ``StoredShortcut``. An empty primary key is the package's explicit
/// "unbound" marker. Returns `nil` when `first` is missing or malformed —
/// and, to stay consistent with the string parser, when a present `second`
/// stroke is malformed (a chord must not silently degrade to a single
/// stroke) or when a bare first stroke is used by an action that requires a
/// modifier.
private func parseShortcutObjectForm(
_ object: [String: Any],
action: KeyboardShortcutSettings.Action
) -> StoredShortcut? {
guard let firstValue = object["first"],
let first = parseShortcutStrokeObject(firstValue) else {
return nil
}
if first.key.isEmpty {
return .unbound
}
// Mirror StoredShortcut.parseConfig(strokes:allowBareFirstStroke:): a
// bare first stroke is only valid for actions that opt into it, or for
// the space key.
guard action.allowsBareFirstStroke || !first.modifierFlags.isEmpty || first.key == "space" else {
return nil
}
let second: ShortcutStroke?
if let secondValue = object["second"], !(secondValue is NSNull) {
// A present-but-malformed second stroke invalidates the whole
// binding rather than silently dropping the chord half.
guard let parsedSecond = parseShortcutStrokeObject(secondValue) else {
return nil
}
second = parsedSecond
} else {
second = nil
}
return StoredShortcut(first: first, second: second)
}

private func parseShortcutStrokeObject(_ rawValue: Any) -> ShortcutStroke? {
if rawValue is NSNull { return nil }
guard let dict = rawValue as? [String: Any],
let key = jsonString(dict["key"]) else {
return nil
}
// An out-of-range keyCode is a corrupt binding, not a key to silently
// wrap into a valid UInt16 (which would re-target a different key).
let keyCode: UInt16?
if let rawKeyCode = jsonInt(dict["keyCode"]) {
guard let value = UInt16(exactly: rawKeyCode) else { return nil }
keyCode = value
} else {
keyCode = nil
}
return ShortcutStroke(
key: key,
command: jsonBool(dict["command"]) ?? false,
shift: jsonBool(dict["shift"]) ?? false,
option: jsonBool(dict["option"]) ?? false,
control: jsonBool(dict["control"]) ?? false,
keyCode: keyCode
)
}

private func parseNullableHex(
_ rawValue: Any?,
path: String,
Expand Down
175 changes: 175 additions & 0 deletions cmuxTests/GlobalSearchShortcutSettingsTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,181 @@ final class GlobalSearchShortcutSettingsTests: XCTestCase {
)
}

func testSettingsFileStoreParsesPackageObjectFormGlobalSearchShortcut() throws {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

// Regression for https://github.com/manaflow-ai/cmux/issues/5137.
// The in-app Settings UI (CmuxSettings package) persists every
// shortcut rebinding to cmux.json under `shortcuts.bindings.<action>`
// as a nested StoredShortcut object ({"first": {key, command, ...}}),
// not the legacy human-editable "cmd+opt+f" string. The file store
// that feeds KeyboardShortcutSettings — and therefore the system-wide
// Carbon hotkeys (globalSearch, showHideAllWindows) — must understand
// that object form. Otherwise SystemWideHotkeyController never sees the
// rebinding and the default ⌥⌘F keeps opening Global Search.
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-global-search-object-settings-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: directoryURL) }

let settingsFileURL = directoryURL.appendingPathComponent("cmux.json", isDirectory: false)
try """
{
"shortcuts": {
"bindings": {
"globalSearch": {
"first": { "key": "j", "command": true, "shift": false, "option": false, "control": true }
}
}
}
}
""".write(to: settingsFileURL, atomically: true, encoding: .utf8)

let store = KeyboardShortcutSettingsFileStore(
primaryPath: settingsFileURL.path,
fallbackPath: nil,
startWatching: false
)

XCTAssertEqual(
store.override(for: .globalSearch),
StoredShortcut(key: "j", command: true, shift: false, option: false, control: true)
)
}

func testSettingsFileStoreParsesPackageObjectFormChordShortcut() throws {
// The package object form also encodes two-stroke chords as
// {"first": {...}, "second": {...}}. A non-system-wide action exercises
// the general path so the fix is not narrowed to global search.
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-chord-object-settings-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: directoryURL) }

let settingsFileURL = directoryURL.appendingPathComponent("cmux.json", isDirectory: false)
try """
{
"shortcuts": {
"bindings": {
"newTab": {
"first": { "key": "b", "command": false, "shift": false, "option": false, "control": true },
"second": { "key": "n", "command": false, "shift": false, "option": false, "control": false }
}
}
}
}
""".write(to: settingsFileURL, atomically: true, encoding: .utf8)

let store = KeyboardShortcutSettingsFileStore(
primaryPath: settingsFileURL.path,
fallbackPath: nil,
startWatching: false
)

XCTAssertEqual(
store.override(for: .newTab),
StoredShortcut(
key: "b",
command: false,
shift: false,
option: false,
control: true,
chordKey: "n",
chordCommand: false,
chordShift: false,
chordOption: false,
chordControl: false
)
)
}

func testSettingsFileStoreParsesPackageObjectFormUnboundShortcut() throws {
// The package marks an explicit "no shortcut" override with an empty
// primary key ({"first": {"key": ""}}). The legacy reader must treat
// that as unbound, not as an invalid binding to be dropped.
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-unbound-object-settings-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: directoryURL) }

let settingsFileURL = directoryURL.appendingPathComponent("cmux.json", isDirectory: false)
try """
{
"shortcuts": {
"bindings": {
"globalSearch": { "first": { "key": "", "command": false, "shift": false, "option": false, "control": false } }
}
}
}
""".write(to: settingsFileURL, atomically: true, encoding: .utf8)

let store = KeyboardShortcutSettingsFileStore(
primaryPath: settingsFileURL.path,
fallbackPath: nil,
startWatching: false
)

XCTAssertEqual(store.override(for: .globalSearch), .unbound)
}

func testSettingsFileStoreRejectsObjectFormChordWithMalformedSecondStroke() throws {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

// A present-but-malformed `second` stroke must invalidate the whole
// binding rather than silently degrading the chord to a single stroke
// (which could create an unintended single-key shortcut).
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-bad-chord-object-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: directoryURL) }

let settingsFileURL = directoryURL.appendingPathComponent("cmux.json", isDirectory: false)
try """
{
"shortcuts": {
"bindings": {
"newTab": {
"first": { "key": "b", "command": false, "shift": false, "option": false, "control": true },
"second": { "command": false }
}
}
}
}
""".write(to: settingsFileURL, atomically: true, encoding: .utf8)

let store = KeyboardShortcutSettingsFileStore(
primaryPath: settingsFileURL.path,
fallbackPath: nil,
startWatching: false
)

XCTAssertNil(store.override(for: .newTab))
}

func testSettingsFileStoreRejectsObjectFormBareKeyForModifierRequiringAction() throws {
// Object-form parsing must apply the same bare-first-stroke rule as the
// string parser: an action that requires a modifier rejects a bare key.
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-bare-object-\(UUID().uuidString)", isDirectory: true)
try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true)
defer { try? FileManager.default.removeItem(at: directoryURL) }

let settingsFileURL = directoryURL.appendingPathComponent("cmux.json", isDirectory: false)
try """
{
"shortcuts": {
"bindings": {
"newTab": { "first": { "key": "j", "command": false, "shift": false, "option": false, "control": false } }
}
}
}
""".write(to: settingsFileURL, atomically: true, encoding: .utf8)

let store = KeyboardShortcutSettingsFileStore(
primaryPath: settingsFileURL.path,
fallbackPath: nil,
startWatching: false
)

XCTAssertNil(store.override(for: .newTab))
}

func testSettingsFileStoreRejectsGlobalSearchChordBinding() throws {
let directoryURL = FileManager.default.temporaryDirectory
.appendingPathComponent("cmux-global-search-invalid-settings-\(UUID().uuidString)", isDirectory: true)
Expand Down
Loading