fix: make Ctrl+P command palette navigation remappable (#1713) - #3335
Merged
Merged
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 configurable command-palette navigation (next/previous) with unbind support and settings-file managed overrides, routes those shortcuts into palette selection (or forwards unhandled keys to the focused terminal), updates persistence/parsing/UI/tests/docs, and refactors Zig selection + helper build/reload scripts. ChangesCommand Palette Shortcut Customization
Build Script & Reload Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsUI as Settings UI
participant KSS as KeyboardShortcutSettings
participant FileStore as SettingsFileStore
participant App as AppDelegate/Runtime
participant CP as Command Palette
participant Terminal
User->>SettingsUI: Remap or unbind Ctrl+P
SettingsUI->>KSS: setShortcut / unbindShortcut(for:.commandPalettePrevious)
KSS->>FileStore: persist override or mark unbound / managed
User->>App: Press Ctrl+P in window
App->>App: commandPaletteSelectionDeltaForKeyboardNavigation(event)
alt Matches bound previous shortcut
App->>CP: post .commandPaletteMoveSelection (delta: -1)
else No match (unbound/cleared)
App->>Terminal: forward key event to focused terminal surface
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 2 inconclusive)
✅ Passed checks (8 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 |
Contributor
Greptile SummaryThis PR promotes command palette navigation (next/previous) to first-class Confidence Score: 5/5Safe to merge; all three previously flagged issues are addressed and no new routing or isolation mistakes were found. The three prior review findings — actor isolation on Coordinator, managed-shortcut value leaking into UserDefaults via onChange, and missing @mainactor on commandPaletteSelectionDeltaForFieldEditorCommand — are all fixed in this PR. The new routing logic (chord arming, field-editor passthrough, unbound detection) follows existing patterns correctly. SystemWideHotkeySettings.shortcut() now returns .unbound for a managed-unbound binding, but the hotkey registration path already gates on a non-nil carbonHotKeyRegistration and calls unregisterHotKey() when that is nil, so the behavior is correct. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant Coordinator as Coordinator (Field Editor)
participant CommandPaletteRouting
participant KSSLookup as KeyboardShortcutSettings
participant Terminal
User->>AppDelegate: keyDown event (global monitor)
AppDelegate->>KSSLookup: shortcutIfBound(.commandPaletteNext / .commandPalettePrevious)
KSSLookup-->>AppDelegate: StoredShortcut? (managed or UserDefaults or default)
AppDelegate->>CommandPaletteRouting: commandPaletteSelectionDeltaForKeyboardNavigation(flags, chars, keyCode, nextShortcut, previousShortcut)
alt Shortcut matches (non-chord)
CommandPaletteRouting-->>AppDelegate: delta (±1)
AppDelegate->>AppDelegate: post .commandPaletteMoveSelection
else Chord shortcut
AppDelegate->>AppDelegate: armConfiguredShortcutChordIfNeeded / matchConfiguredShortcut
AppDelegate->>AppDelegate: post .commandPaletteMoveSelection
else No match
AppDelegate->>AppDelegate: continue normal key routing
end
User->>Coordinator: NSTextFieldDelegate doCommandBy
Coordinator->>CommandPaletteRouting: commandPaletteSelectionDeltaForFieldEditorCommand(selector, event)
CommandPaletteRouting->>KSSLookup: shortcutIfBound(.commandPaletteNext / .commandPalettePrevious)
KSSLookup-->>CommandPaletteRouting: StoredShortcut?
alt Shortcut matches event
CommandPaletteRouting-->>Coordinator: delta (±1)
Coordinator->>Coordinator: onMoveSelection(delta)
else Unbound / no match (moveDown or moveUp selector)
CommandPaletteRouting-->>Coordinator: nil
Coordinator->>Terminal: forwardKeyDownToSurface(event)
end
Reviews (15): Last reviewed commit: "Clarify nullable shortcut schema binding" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Sources/KeyboardShortcutSettingsFileStore.swift (1)
838-844: ⚡ Quick winUse the trimmed string for shortcut parsing to avoid whitespace-sensitive failures.
You already compute
trimmed; parsing the originalstrokecan make" ctrl+p "behavior depend on parser internals.Suggested change
- shortcut = StoredShortcut.parseConfig(stroke) + shortcut = StoredShortcut.parseConfig(trimmed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettingsFileStore.swift` around lines 838 - 844, The code computes a trimmed string but then calls StoredShortcut.parseConfig with the original stroke, which can cause whitespace-sensitive failures; update the parsing call to use the trimmed variable instead (i.e., replace StoredShortcut.parseConfig(stroke) with StoredShortcut.parseConfig(trimmed)) so that parsing and the empty/"none"/"unbound"/"disabled" check use the same normalized input for assigning shortcut.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/messages/ja.json`:
- Line 390: Update the "chordsCallout" localization string to document all
supported unbind sentinels by adding "unbound" and "disabled" to the existing
list (which currently mentions null, empty string, and "none"); locate the
"chordsCallout" entry in web/messages/ja.json and modify its text so the callout
lists: null, empty string, "none", "unbound", and "disabled" as valid values for
unassigning actions.
---
Nitpick comments:
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Around line 838-844: The code computes a trimmed string but then calls
StoredShortcut.parseConfig with the original stroke, which can cause
whitespace-sensitive failures; update the parsing call to use the trimmed
variable instead (i.e., replace StoredShortcut.parseConfig(stroke) with
StoredShortcut.parseConfig(trimmed)) so that parsing and the
empty/"none"/"unbound"/"disabled" check use the same normalized input for
assigning shortcut.
🪄 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: ebf28471-57fb-4828-9e4b-118512904339
📒 Files selected for processing (13)
README.mdResources/Localizable.xcstringsSources/App/ShortcutRoutingSupport.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftweb/app/[locale]/docs/configuration/page.tsxweb/app/[locale]/docs/keyboard-shortcuts/page.tsxweb/data/cmux-settings.schema.jsonweb/data/cmux-shortcuts.tsweb/messages/en.jsonweb/messages/ja.json
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
austinywang
force-pushed
the
issue-1713-ctrl-p-remap-unbind
branch
from
April 30, 2026 01:58
a63050f to
ad84fba
Compare
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 `@scripts/build-ghostty-cli-helper.sh`:
- Around line 36-83: The universal-build logic currently decides the "native"
Zig arch using command -v zig which can differ from the Zig chosen by
build_helper; update the universal branch to call select_zig_for_target "" to
pick the actual Zig binary (respecting CMUX_ZIG and fallbacks) and then call
zig_binary_arch on that result to get ZIG_ARCH, replacing the direct command -v
zig / zig_binary_arch usage so the native leg matches the Zig used by
build_helper.
In `@scripts/reload.sh`:
- Around line 611-619: The script treats any executable at GHOSTTY_HELPER_DEST
as valid (using -x) even when it's a skip stub created by
scripts/build-ghostty-cli-helper.sh when CMUX_SKIP_ZIG_BUILD=1; update reload.sh
around the GHOSTTY_HELPER_DEST check so that if CMUX_SKIP_ZIG_BUILD is not set
to 1 you either (a) detect the stub marker written by
build-ghostty-cli-helper.sh inside the file and force a rebuild, or (b) always
overwrite Contents/Resources/bin/ghostty by invoking
scripts/build-ghostty-cli-helper.sh --output "$GHOSTTY_HELPER_DEST" when Zig
builds are enabled; reference the CMUX_SKIP_ZIG_BUILD flag, GHOSTTY_HELPER_DEST
variable and the build-ghostty-cli-helper.sh script to locate where to add the
stub-marker check or unconditional overwrite.
🪄 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: 144c4b5b-3beb-4325-8e85-fc7c88ae9144
📒 Files selected for processing (2)
scripts/build-ghostty-cli-helper.shscripts/reload.sh
austinywang
force-pushed
the
issue-1713-ctrl-p-remap-unbind
branch
from
May 4, 2026 06:30
ad84fba to
2843ca7
Compare
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/KeyboardShortcutSettingsControls.swift (1)
5-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isDisabledandsubtitlecan go stale when the settings file manages a shortcut at the same value already showing in the UI.
isDisabledandsubtitleare both computed directly insidebodyfromKeyboardShortcutSettings.isManagedBySettingsFile(action)/settingsFileManagedSubtitle(for:). SwiftUI only re-evaluatesbodywhen a@Stateor@Bindingchanges. The.onReceivehandler only mutatesshortcutwhenlatest != shortcut. So ifcmux.jsonnewly manages an action but sets it to the same value already showing (e.g., locks ctrl+n to its default),shortcutdoesn't change,bodyis never re-invoked, andisDisabledremainsfalse— the row appears editable even though all writes to it are silently no-oped.The same staleness applies in reverse when the file is removed.
Fix: track the managed state as
@Stateand update it unconditionally in.onReceive:🛡️ Proposed fix
struct ShortcutSettingRow: View { let action: KeyboardShortcutSettings.Action `@State` private var shortcut: StoredShortcut + `@State` private var isManagedByFile: Bool init(action: KeyboardShortcutSettings.Action) { self.action = action _shortcut = State(initialValue: KeyboardShortcutSettings.shortcut(for: action)) + _isManagedByFile = State(initialValue: KeyboardShortcutSettings.isManagedBySettingsFile(action)) } var body: some View { ShortcutRecorderSettingsControl( action: action, shortcut: $shortcut, - subtitle: KeyboardShortcutSettings.settingsFileManagedSubtitle(for: action), + subtitle: isManagedByFile ? KeyboardShortcutSettings.settingsFileManagedSubtitle(for: action) : nil, displayString: { action.displayedShortcutString(for: $0) }, - isDisabled: KeyboardShortcutSettings.isManagedBySettingsFile(action) + isDisabled: isManagedByFile ) .onChange(of: shortcut) { _, newValue in KeyboardShortcutSettings.setShortcut(newValue, for: action) } .onReceive(NotificationCenter.default.publisher(for: KeyboardShortcutSettings.didChangeNotification)) { _ in let latest = KeyboardShortcutSettings.shortcut(for: action) if latest != shortcut { shortcut = latest } + let managed = KeyboardShortcutSettings.isManagedBySettingsFile(action) + if managed != isManagedByFile { + isManagedByFile = managed + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettingsControls.swift` around lines 5 - 28, The body’s computed isDisabled and subtitle can become stale because they’re not tied to any `@State/`@Binding; add `@State` properties (e.g., `@State` private var isManaged and `@State` private var managedSubtitle) and initialize them from KeyboardShortcutSettings.isManagedBySettingsFile(action) and settingsFileManagedSubtitle(for: action), use those state vars in the ShortcutRecorderSettingsControl isDisabled and subtitle parameters, and in the .onReceive(NotificationCenter.default.publisher(for: KeyboardShortcutSettings.didChangeNotification)) handler always update both isManaged and managedSubtitle (in addition to updating shortcut when needed) so the view re-renders whenever the settings file changes even if the shortcut value itself is unchanged.
🤖 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/KeyboardShortcutSettingsControls.swift`:
- Around line 5-28: The body’s computed isDisabled and subtitle can become stale
because they’re not tied to any `@State/`@Binding; add `@State` properties (e.g.,
`@State` private var isManaged and `@State` private var managedSubtitle) and
initialize them from KeyboardShortcutSettings.isManagedBySettingsFile(action)
and settingsFileManagedSubtitle(for: action), use those state vars in the
ShortcutRecorderSettingsControl isDisabled and subtitle parameters, and in the
.onReceive(NotificationCenter.default.publisher(for:
KeyboardShortcutSettings.didChangeNotification)) handler always update both
isManaged and managedSubtitle (in addition to updating shortcut when needed) so
the view re-renders whenever the settings file changes even if the shortcut
value itself is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b47e1375-d51e-4233-afeb-c616f5ab04f1
📒 Files selected for processing (10)
README.mdResources/Localizable.xcstringsSources/App/ShortcutRoutingSupport.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsControls.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftscripts/build-ghostty-cli-helper.shscripts/reload.sh
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build-ghostty-cli-helper.sh
Ctrl+P command palette navigation is currently hardcoded outside KeyboardShortcutSettings, so clearing or remapping shortcut settings cannot let terminal apps receive ^P. The tests codify the desired settings-backed behavior before changing the router. Constraint: Preserve the two-commit regression-test-then-fix workflow requested for issue #1713 Rejected: Only source-grep the hardcoded path | would not prove remap/unbind behavior through the app shortcut router Confidence: medium Scope-risk: narrow Tested: Not run locally; red commit intentionally references the missing settings API/action for the fix commit to satisfy Not-tested: Local unit test execution and manual cat -v reproduction, because the tagged reload build failed in the Ghostty helper before launch
Command palette result navigation now uses first-class shortcut actions instead of hardcoded Ctrl+N/Ctrl+P checks. Shortcut bindings can persist an explicit unbound state, the Settings UI exposes a Clear control, and settings.json can unbind with null, an empty string, none, unbound, or disabled. Constraint: Ctrl+P must pass through to terminal panes when the command palette previous shortcut is cleared Constraint: Keep default behavior as Ctrl+N/Ctrl+P for users without custom settings Rejected: Copy PR #1736 directly | it did not represent unbound state and risked falling back to defaults Rejected: Remove only the Ctrl+P keyCode branch | would make remapping impossible Confidence: medium Scope-risk: moderate Tested: jq empty on updated JSON resources and schema; git diff --check Not-tested: Local unit tests and manual app verification; tagged reload currently fails in Ghostty helper before launch
The dev reload was failing on macOS 26 because Xcode's script phase found the x86_64 Homebrew Zig first and cross-linked the arm64 Ghostty CLI helper. Zig 0.15.x then failed to resolve libc, CoreFoundation, CoreText, and Objective-C symbols before the app could launch. The helper builder now selects a Zig binary that matches the requested target architecture when one is installed, while reload.sh reuses the helper produced by the app build instead of running a second raw zig build afterward. LaunchServices can also return -600 for the tagged app immediately after a rebuild. reload.sh now retries the launch with open -n -g so the required --launch command exits successfully once the app is built. Constraint: Dev builds must run through scripts/reload.sh rather than direct xcodebuild. Rejected: Require CMUX_SKIP_ZIG_BUILD=1 for this branch | the required launch command cannot pass that environment override and it would replace the real helper with a stub. Confidence: high Scope-risk: narrow Tested: bash -n scripts/build-ghostty-cli-helper.sh scripts/reload.sh Tested: PATH with /usr/local/bin first still built an arm64 Ghostty helper via /opt/homebrew/bin/zig Tested: ./scripts/reload.sh --tag issue-1713-ctrl-p-remap-unbind --launch succeeded and launched the tagged app
Rebasing the Ctrl+P shortcut work onto current main overlapped with main's existing StoredShortcut.unbound declaration. Keeping one declaration preserves the existing storage shape and avoids a duplicate type member. Constraint: origin/main already defines StoredShortcut.unbound Confidence: high Scope-risk: narrow Tested: jq empty Resources/Localizable.xcstrings web/messages/en.json web/messages/ja.json web/data/cmux.schema.json web/data/cmux-settings.schema.json; bash -n scripts/build-ghostty-cli-helper.sh scripts/reload.sh; git diff --check Not-tested: local unit tests per repository policy
The Ctrl+P remap/unbind fix added runtime coverage and helper logic to files that are already tracked by the Swift file-length guard. Move the new coverage and lookup/routing helpers into small focused files so CI can enforce the existing budget without accepting new long-file debt. Constraint: workflow-guard-tests rejects growth in tracked Swift files over the budget Rejected: Refresh the Swift file-length budget | would accept new long-file debt for a localized shortcut fix Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: jq empty Resources/Localizable.xcstrings web/messages/en.json web/messages/ja.json web/data/cmux.schema.json web/data/cmux-settings.schema.json Tested: bash -n scripts/build-ghostty-cli-helper.sh scripts/reload.sh Tested: plutil -lint GhosttyTabs.xcodeproj/project.pbxproj Not-tested: XCTest execution; repository policy keeps tests in CI
The shortcut customization paths need to respect cmux.json as the source of truth across AppKit field-editor routing, chord dispatch, Settings conflict handling, and the system-wide hotkey registrar. This keeps remapped or unbound command-palette navigation from being resurrected by AppKit move commands, prevents Settings from pretending it can swap immutable managed shortcuts, and ensures the registered global hotkey matches the managed value shown in Settings. Constraint: AppKit field editors translate Ctrl+P/Ctrl+N into moveUp:/moveDown: before normal key routing can pass them to the terminal. Constraint: cmux.json-managed shortcuts must remain authoritative over UserDefaults-backed Settings UI values. Rejected: Only change AppDelegate shortcut matching | the focused command palette search field still handles moveUp:/moveDown: locally. Confidence: high Scope-risk: moderate Directive: Keep command-palette navigation routing shared between the AppDelegate monitor and field-editor delegate when adding future shortcut customization. Tested: git diff --check; ./scripts/reload.sh --tag review-shortcuts; ./scripts/reload.sh --tag review-shortcuts --launch Not-tested: Full XCTest suite per repo policy; added regression tests for CI coverage.
austinywang
force-pushed
the
issue-1713-ctrl-p-remap-unbind
branch
from
May 4, 2026 09:01
b8c1d31 to
dd6ba36
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 3713-3763: The test is missing an XCTestExpectation/wait for the
asynchronous .commandPaletteMoveSelection notification; update the
NotificationCenter handler to create an XCTestExpectation (e.g.,
moveExpectation), call moveExpectation.fulfill() when you append the delta, and
then call wait(for: [moveExpectation], timeout: 1.0) before asserting
observedDeltas and observedWindow; reference the existing handler/variables
(observedDeltas, observedWindow, moveToken) and the expectation name
(moveExpectation) so the assertions at the end run only after the notification
is received.
In `@cmuxTests/CommandPaletteShortcutCustomizationTests.swift`:
- Around line 15-27: The test suite must snapshot and clear the two UserDefaults
keys for command-palette shortcuts to isolate persisted state: in
setUpWithError() read and store the current values for
"shortcut.commandPaletteNext" and "shortcut.commandPalettePrevious" (e.g. into
properties like savedCommandPaletteNext / savedCommandPalettePrevious), then
remove those keys from UserDefaults so default-argument lookups in
commandPaletteSelectionDeltaForFieldEditorCommand(...) and
debugHandleCustomShortcut(...) are deterministic; in tearDown() restore the
saved values (or remove if nil) and then restore
KeyboardShortcutSettings.settingsFileStore as before. Ensure you use
UserDefaults.standard for the get/remove/set operations and handle optional
values when restoring.
In `@Resources/Localizable.xcstrings`:
- Around line 5-7: Add the missing locale entries for the three new keys
("shortcut.commandPaletteNext.label", "shortcut.commandPalettePrevious.label",
"settings.shortcuts.managedByFile") by adding 17 locale objects: ar, bs, da, de,
es, fr, it, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant using the same
"localizations" -> "<locale>": { "stringUnit": { "state": "translated", "value":
"<English text>" } } structure; for ar, bs, da, pl, th, tr use English text and
state "translated" (lower-confidence locales per note), and for the remaining
locales also include the English value and "translated" state so the catalog
supports all 19 locales for each of the three keys.
In `@Sources/AppDelegate.swift`:
- Around line 10675-10694: The new direct handling of
.commandPaletteNext/.commandPalettePrevious bypasses the existing routing guard
and allows shortcuts to be consumed even when inline text mode should prevent
them; update the early return block that checks
commandPaletteInteractiveInTargetWindow and commandPaletteShortcutWindow to
first call shouldRouteCommandPaletteSelectionNavigation(...) (the same check
used elsewhere) and only perform matchConfiguredShortcut(..., action:
.commandPaletteNext/.commandPalettePrevious) and post
.commandPaletteMoveSelection notifications when that guard returns true,
ensuring the single source of truth for routing is respected.
In `@web/app/`[locale]/docs/configuration/page.tsx:
- Around line 82-84: Update the explanatory guidance text for shortcuts.bindings
to document the null/empty-string unbinding form and its aliases: explicitly
state that setting a binding to null (or an empty string) removes/unbinds the
shortcut (same effect as the shown "commandPalettePrevious": null example), and
mention that both null and "" are accepted aliases for unbinding. Edit the
explanation block in web/app/[locale]/docs/configuration/page.tsx where
shortcuts.bindings forms are described so it describes three cases (single
string, two-item array, and null/empty-string unbind) and includes a brief
example note referencing shortcuts.bindings.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a300ff83-4026-44c2-aab3-0610c023544c
📒 Files selected for processing (25)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/App/CommandPaletteShortcutRouting.swiftSources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsControls.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/KeyboardShortcutSettingsLookup.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/CommandPaletteShortcutCustomizationTests.swiftcmuxTests/ShortcutUnbindingTests.swiftcmuxTests/WorkspaceUnitTests.swiftscripts/build-ghostty-cli-helper.shscripts/reload.shweb/app/[locale]/docs/configuration/page.tsxweb/app/[locale]/docs/keyboard-shortcuts/page.tsxweb/data/cmux-shortcuts.tsweb/data/cmux.schema.jsonweb/messages/en.jsonweb/messages/ja.json
💤 Files with no reviewable changes (1)
- Sources/App/ShortcutRoutingSupport.swift
The shortcut remap/unbind work added behavior-level coverage and small routing changes, but the checked-in per-file Swift length budget still held pre-change counts for those files. This refreshes the budget to the measured current counts and also tightens entries where files shrank, so the guard remains exact rather than loosely raised. Constraint: CI enforces per-file Swift length budgets from .github/swift-file-length-budget.tsv Rejected: Split the new shortcut regression tests | the added coverage exercises distinct entrypoints and splitting would add churn without reducing behavior risk Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Full GitHub Actions matrix not run locally
GitHub validates pull requests against the synthetic merge ref, not the branch tip alone. origin/main advanced after the local budget refresh, and the merge ref keeps CLI/cmux.swift at the previous 20530-line budget even though the branch tip alone is shorter. Restoring that budget keeps the guard aligned with the ref CI actually checks. Constraint: Pull request CI checks refs/pull/<id>/merge, which includes current origin/main Rejected: Keep the branch-tip-only CLI/cmux.swift reduction | it fails the synthetic merge ref while main still carries the larger file Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Full GitHub Actions matrix not run locally
The PR was functionally green but still had requested changes from review bots. This keeps command-palette next/previous routing behind the same inline-text guard as arrow navigation, prevents chord prefixes from being treated as complete palette moves, mirrors settings-file managed state into SwiftUI row state, and tightens the affected tests/docs/localization coverage. Constraint: CodeRabbit marked the PR changes-requested on routing guards, test isolation, localization coverage, SwiftUI managed-state redraws, and docs Rejected: Leave direct next/previous handling as a separate path | it bypassed the shared inline-text routing guard Rejected: Treat chord prefixes as keyboard-navigation matches | prefixes should arm chords without moving palette selection Confidence: high Scope-risk: moderate Tested: jq empty Resources/Localizable.xcstrings Tested: jq localization count for the three new keys is 19 each Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check Tested: ./scripts/reload.sh --tag fix-ci-review Not-tested: Local web typecheck is blocked by this checkout's TypeScript toolchain rejecting existing moduleResolution=bundler before checking changed TSX; GitHub web-typecheck will verify after push
The PR branch needed the latest main branch before CI iteration. The merge kept main's relocated shortcut validation presentation while preserving this branch's managed-shortcut swap protections. Constraint: origin/main already moved ShortcutRecorderValidationPresentation into KeyboardShortcutSettingsControls.swift Rejected: Keep duplicate definitions in both files | Swift module emission fails with an invalid redeclaration Confidence: medium Scope-risk: moderate Directive: Keep context-aware conflict presentation and settings-file managed swap blocking together when touching shortcut recorder validation Tested: git diff --cached --check Not-tested: tagged reload build is still queued behind the shared xcodebuild lock
The branch needed the latest main after origin/main advanced during PR iteration. The resolution keeps both command-palette shortcut customization and main's bare-space shortcut routing/schema work. Constraint: main split shortcut schema validation into unbound, first-stroke, and stroke definitions while this branch added disabled as an unbind alias Rejected: Pick either project-file side | each side added distinct source/test files that must remain in the Xcode target Confidence: medium Scope-risk: moderate Directive: Keep disabled/null unbind support aligned between Swift parsing, schema, docs, and localized copy Tested: git diff --cached --check; python3 -m json.tool web/data/cmux.schema.json; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj Not-tested: tagged reload build is still queued behind the shared xcodebuild lock
| let onMoveSelection: (Int) -> Void | ||
| let onUnhandledNavigationKey: (NSEvent) -> Bool | ||
|
|
||
| final class Coordinator: NSObject, NSTextFieldDelegate { |
Contributor
There was a problem hiding this comment.
@MainActor-isolated callees invoked from non-isolated Coordinator
Coordinator (line 4866) carries no actor annotation, so it is nonisolated under Swift 6 strict concurrency. Lines 4905 and 4911 both access NSApp.currentEvent, which is a @MainActor-isolated property, and line 4905 also calls commandPaletteSelectionDeltaForFieldEditorCommand, which is annotated @MainActor. Calling @MainActor declarations from a nonisolated synchronous context is a data-race error under Swift 6 strict concurrency checking — the compiler will reject both sites. The NSTextFieldDelegate callback is guaranteed by AppKit to run on the main thread at runtime, so adding @MainActor to Coordinator is the correct fix and will not change behavior.
Suggested change
| final class Coordinator: NSObject, NSTextFieldDelegate { | |
| @MainActor final class Coordinator: NSObject, NSTextFieldDelegate { |
File Used: .github/review-bot-rules/swift-actor-isolation.md (source)
The command palette search coordinator runs on AppKit delegate callbacks and calls main-actor shortcut routing helpers. Marking the coordinator itself as main-actor isolated keeps the Swift concurrency contract explicit after merging current main. Constraint: AppKit delegate callbacks for this NSTextField are delivered on the main thread Rejected: Wrap individual calls in Task or DispatchQueue | would make synchronous key handling asynchronous Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Tagged reload build is still queued behind the shared Xcode build lock; CI is pending
Main-actor isolation on the command palette coordinator exposed the NotificationCenter teardown path as nonisolated. The observer token is explicitly nonisolated for deinit cleanup, while notification callbacks hop through MainActor.assumeIsolated because they are delivered on the main queue. Constraint: Deinit cannot call the coordinator's main-actor-isolated detach method synchronously Rejected: Drop observer cleanup | would leave a block observer registered after coordinator teardown Confidence: medium Scope-risk: narrow Tested: git diff --check Not-tested: Tagged reload build has not completed after this follow-up yet; CI is pending
The main-actor coordinator cleanup can stay behaviorally identical without increasing ContentView's tracked file length. Compacting the deinit and notification callback keeps the workflow guard within its existing budget. Constraint: workflow-guard-tests enforces .github/swift-file-length-budget.tsv for ContentView.swift Rejected: Refresh the file-length budget | unnecessary for a compact follow-up Confidence: high Scope-risk: narrow Tested: git diff --check; wc -l Sources/ContentView.swift is below the checked-in budget Not-tested: CI rerun still pending
Greptile flagged the pure keyboard-navigation helper because its default arguments read effective shortcut settings. The helper now defaults only to static built-in shortcuts, while AppDelegate and the main-actor field editor coordinator pass effective settings-file-aware shortcuts explicitly. Constraint: Nonisolated default argument expressions must not touch main-actor settings state Rejected: Mark every caller and test path @mainactor | broader than needed for a pure matching helper Confidence: high Scope-risk: narrow Tested: git diff --check; checked tracked Swift file lengths remain within .github/swift-file-length-budget.tsv Not-tested: Tagged reload and CI rerun are pending after this commit
origin/main advanced while the PR iteration was green, so the branch was updated again before rerunning verification. The merge was clean and preserved the explicit palette shortcut lookup fix while taking the latest localization and sidebar/session updates from main. Constraint: User requested repeated git pull from origin main before completing the PR iteration Rejected: Leave the default auto-merge message | it does not satisfy the repo's Lore commit protocol Confidence: high Scope-risk: moderate Tested: git diff --check HEAD Not-tested: Tagged reload and CI rerun are pending after this merge
austinywang
force-pushed
the
issue-1713-ctrl-p-remap-unbind
branch
from
May 5, 2026 02:11
88df7ab to
4830588
Compare
AppKit can invoke the command-palette field editor delegate with only moveUp: and no current key event. That fallback was still interpreted as command-palette previous after Ctrl+P was unbound. The fallback now only maps moveUp:/moveDown: when the matching default shortcut is still active, and tests cover the nil-event unbound and remapped cases. Constraint: A selector-only AppKit callback cannot distinguish Ctrl+P from an arrow key without the original key event Rejected: Always consume moveUp:/moveDown: without an event | breaks explicit unbind/remap semantics Confidence: high Scope-risk: narrow Tested: git diff --check; ./scripts/reload.sh --tag issue-1713-ctrl-p-remap-unbind --launch Not-tested: CI rerun is pending after this commit
origin/main advanced while fixing the command-palette Ctrl+P unbind fallback. The clean merge keeps the nil-event fallback fix on top of the latest main changes before rerunning build and PR checks. Constraint: User requested the PR branch stay pulled from origin main while iterating Rejected: Leave the default auto-merge message | it does not satisfy the repo's Lore commit protocol Confidence: high Scope-risk: moderate Tested: git diff --check HEAD Not-tested: Tagged reload and CI rerun are pending after this merge Co-authored-by: OmX <omx@oh-my-codex.dev>
austinywang
force-pushed
the
issue-1713-ctrl-p-remap-unbind
branch
from
May 5, 2026 02:53
4264542 to
16490e4
Compare
Cursor found that the no-argument keyboard-navigation helper still defaulted to static Ctrl+N/Ctrl+P bindings. The pure matcher now requires explicit shortcuts, while the convenience overload resolves the effective settings-aware bindings and tests cover unbound and remapped default lookup paths. Constraint: Command-palette shortcut routing must respect user unbinds and remaps from every helper entrypoint Rejected: Keep hardcoded default parameters on the pure matcher | future callers could consume unbound Ctrl+P again Confidence: high Scope-risk: narrow Tested: git diff --check; python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Local XCTest execution per repo policy; tagged reload and CI rerun are pending after this commit Co-authored-by: OmX <omx@oh-my-codex.dev>
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 e1b2e9a. Configure here.
Cursor identified that shortcut binding null support could be represented as an overlapping nullable oneOf. The schema now keeps shortcutBinding non-null and introduces shortcutBindingNullable as the only wrapper that adds null, so shortcut bindings can accept null without ambiguous oneOf matching. Constraint: shortcuts.bindings must accept null for unbinding while remaining valid JSON Schema Rejected: Leave null directly in shortcutBinding | makes nullable wrapper composition ambiguous for future schema edits Confidence: high Scope-risk: narrow Tested: python3 -m json.tool web/data/cmux.schema.json; focused Python assertion for shortcutBindingNullable shape; git diff --check Not-tested: Full web/schema test suite not run; user asked to address Cursorbot comments only Co-authored-by: OmX <omx@oh-my-codex.dev>
This was referenced May 6, 2026
2 of 6 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1713
Root cause
Ctrl+P command palette navigation lived in
commandPaletteSelectionDeltaForKeyboardNavigationas a hardcodedkeyCode == 35/ Ctrl+P branch, outsideKeyboardShortcutSettings. Remapping or clearing keybinding settings could not stop cmux from consuming Ctrl+P while the palette was interactive.Fix
commandPaletteNextandcommandPalettePreviousas first-classKeyboardShortcutSettingsactions, defaulting to Ctrl+N and Ctrl+P.null, an empty string,none,unbound, ordisabled.open -n -gwhen plainopen -greturns -600.Test plan
98b29a7dbefore the fix commit.jq empty Resources/Localizable.xcstrings web/messages/en.json web/messages/ja.json web/data/cmux-settings.schema.json.git diff --check.bash -n scripts/build-ghostty-cli-helper.sh scripts/reload.sh.scripts/build-ghostty-cli-helper.sh --target aarch64-macosselects/opt/homebrew/bin/zigand produces an arm64 helper even when/usr/local/binis first in PATH../scripts/reload.sh --tag issue-1713-ctrl-p-remap-unbind --launch; the tagged dev app built and launched successfully.Manual verification
The tagged app now launches locally. Accessibility automation is blocked on this machine by Apple event error -1743, so UI-driven Settings verification is still not automated from this shell.
Note
Medium Risk
Changes global shortcut lookup/routing for the command palette (including chord handling and terminal pass-through), which can affect key handling across the app. Also updates dev build/reload scripts for Zig selection and app launching, which may impact local developer workflows.
Overview
Command palette navigation is now fully configurable. Adds new shortcut actions
commandPaletteNext/commandPalettePrevious(defaults⌃N/⌃P), moves navigation detection intoCommandPaletteShortcutRouting, and updatesAppDelegate/command palette field editor handling so remapped or unbound keys are no longer hardcoded and can pass through to the focused terminal.Shortcut settings now treat “unbound” as a first-class state. Introduces
KeyboardShortcutSettingsLookupwithshortcutIfBound, prevents writes/swaps when a shortcut is managed bycmux.json, updates UI to show a “Managed in cmux.json” subtitle and disable recording, and expands config/schema/docs to allow unbinding vianull, empty string, and additional aliases (includingdisabled).Tests and tooling updates. Adds regression tests for remapping/unbinding/managed-file behavior, updates localized strings/docs/site data, and improves scripts to select an arch-appropriate
zig, preserve an Xcode-built Ghostty helper when present, and retry LaunchServices withopen -n -gon failure.Reviewed by Cursor Bugbot for commit c3c00e8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation
null, empty, "none", "clear", "unbound", "disabled").Localization
Tests