Fix Dvorak Cmd+C colliding with notifications shortcut#762
Merged
lawrencecchen merged 20 commits intomainfrom Mar 6, 2026
Merged
Fix Dvorak Cmd+C colliding with notifications shortcut#762lawrencecchen merged 20 commits intomainfrom
lawrencecchen merged 20 commits intomainfrom
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:
📝 WalkthroughWalkthroughMakes main shortcut handling layout‑aware (new normalization and ANSI fallbacks), renames command‑focused shortcut hint monitors/policies to modifier‑focused variants across UI and titlebar, adds a persisted shortcut‑hint visibility setting, and expands tests for layout‑specific shortcut behavior (including Dvorak). Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Key Event)
participant App as AppDelegate.matchShortcut
participant Layout as KeyboardLayout
participant Action as Feature Handler
User->>App: key event + modifier flags
App->>Layout: layoutCharacterProvider(keyCode)
Layout-->>App: layout-aware character or nil
App->>App: normalizedShortcutEventCharacter(chars, keyCode)
alt character-based match required
App->>Action: dispatch action (character match)
else ANSI fallback allowed
App->>Action: dispatch action (ANSI keyCode match)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
328-330: Consider extracting temporary-shortcut setup into a test helper.The same set/restore
KeyboardShortcutSettingspattern appears in multiple tests; a small helper (for example,withTemporaryShortcut) would reduce duplication and future maintenance risk.Also applies to: 371-373, 412-417, 456-461
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 328 - 330, Extract the repeated save/set/restore pattern for KeyboardShortcutSettings into a test helper (e.g., withTemporaryShortcut) that takes the action (KeyboardShortcutSettings.Action), a temporary shortcut (or uses action.defaultShortcut), and a closure to run; inside the helper capture the original via KeyboardShortcutSettings.shortcut(for:), set the temporary via KeyboardShortcutSettings.setShortcut(..., for:), run the closure, and restore the original in a defer block. Replace the inline patterns (calls to KeyboardShortcutSettings.shortcut(for:), KeyboardShortcutSettings.setShortcut(..., for:), and the defer restore) in the tests that reference Action.showNotifications (and the other occurrences at the noted ranges) with calls to withTemporaryShortcut(action: .showNotifications) { ... } so tests remain behaviorally identical but share the helper.
🤖 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/AppDelegateShortcutRoutingTests.swift`:
- Around line 328-330: Extract the repeated save/set/restore pattern for
KeyboardShortcutSettings into a test helper (e.g., withTemporaryShortcut) that
takes the action (KeyboardShortcutSettings.Action), a temporary shortcut (or
uses action.defaultShortcut), and a closure to run; inside the helper capture
the original via KeyboardShortcutSettings.shortcut(for:), set the temporary via
KeyboardShortcutSettings.setShortcut(..., for:), run the closure, and restore
the original in a defer block. Replace the inline patterns (calls to
KeyboardShortcutSettings.shortcut(for:),
KeyboardShortcutSettings.setShortcut(..., for:), and the defer restore) in the
tests that reference Action.showNotifications (and the other occurrences at the
noted ranges) with calls to withTemporaryShortcut(action: .showNotifications) {
... } so tests remain behaviorally identical but share the helper.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69e47a9a8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Greptile SummaryFixed Dvorak keyboard layout issue where Key changes:
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 69e47a9 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmuxTests/CmuxWebViewKeyEquivalentTests.swift (1)
2577-2579: Tighten the hold-delay assertion to catch overly long regressions.Line 2578 only enforces a minimum, so an accidentally large delay would still pass.
♻️ Proposed test hardening
func testShortcutHintUsesIntentionalHoldDelay() { XCTAssertGreaterThanOrEqual(ShortcutHintModifierPolicy.intentionalHoldDelay, 0.25) + XCTAssertLessThanOrEqual(ShortcutHintModifierPolicy.intentionalHoldDelay, 1.0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 2577 - 2579, The test testShortcutHintUsesIntentionalHoldDelay currently only asserts a minimum; tighten it to also assert an upper bound by checking ShortcutHintModifierPolicy.intentionalHoldDelay equals the expected value (e.g., 0.25) within a small tolerance (use XCTAssertEqual with an accuracy or assert both >= 0.25 and <= 0.25 + ε) so overly large delays fail; update the assertion in testShortcutHintUsesIntentionalHoldDelay accordingly referencing ShortcutHintModifierPolicy.intentionalHoldDelay.Sources/AppDelegate.swift (1)
6532-6539: Precompute layout character once per key event to avoid repeated translation calls.
matchShortcut(event:shortcut:)can be called many times per keyDown, and each miss can triggerKeyboardLayout.character(forKeyCode:). Consider passing a per-event precomputed layout character to reduce hot-path overhead.♻️ Suggested optimization
- private func matchShortcut(event: NSEvent, shortcut: StoredShortcut) -> Bool { + private func matchShortcut( + event: NSEvent, + shortcut: StoredShortcut, + precomputedLayoutCharacter: String? = nil + ) -> Bool { @@ - if shortcutCharacterMatches( - eventCharacter: KeyboardLayout.character(forKeyCode: event.keyCode), + if shortcutCharacterMatches( + eventCharacter: precomputedLayoutCharacter ?? KeyboardLayout.character(forKeyCode: event.keyCode), shortcutKey: shortcutKey, applyShiftSymbolNormalization: false ) { return true }// In handleCustomShortcut(event:) let eventLayoutCharacter = KeyboardLayout.character(forKeyCode: event.keyCode) // then reuse via a local helper/wrapper for all shortcut probes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6532 - 6539, Precompute the layout character once per key event and pass it into the shortcut-matching path instead of calling KeyboardLayout.character(forKeyCode:) repeatedly: in handleCustomShortcut(event:) call KeyboardLayout.character(forKeyCode: event.keyCode) once (e.g. eventLayoutCharacter) and update the matchShortcut(event:shortcut:) calls (or create a small wrapper that accepts eventLayoutCharacter) so matchShortcut uses that precomputed character when invoking shortcutCharacterMatches(... eventCharacter: ..., shortcutKey: ..., applyShiftSymbolNormalization: false).
🤖 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/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 2577-2579: The test testShortcutHintUsesIntentionalHoldDelay
currently only asserts a minimum; tighten it to also assert an upper bound by
checking ShortcutHintModifierPolicy.intentionalHoldDelay equals the expected
value (e.g., 0.25) within a small tolerance (use XCTAssertEqual with an accuracy
or assert both >= 0.25 and <= 0.25 + ε) so overly large delays fail; update the
assertion in testShortcutHintUsesIntentionalHoldDelay accordingly referencing
ShortcutHintModifierPolicy.intentionalHoldDelay.
In `@Sources/AppDelegate.swift`:
- Around line 6532-6539: Precompute the layout character once per key event and
pass it into the shortcut-matching path instead of calling
KeyboardLayout.character(forKeyCode:) repeatedly: in
handleCustomShortcut(event:) call KeyboardLayout.character(forKeyCode:
event.keyCode) once (e.g. eventLayoutCharacter) and update the
matchShortcut(event:shortcut:) calls (or create a small wrapper that accepts
eventLayoutCharacter) so matchShortcut uses that precomputed character when
invoking shortcutCharacterMatches(... eventCharacter: ..., shortcutKey: ...,
applyShiftSymbolNormalization: false).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/ContentView.swiftSources/Update/UpdateTitlebarAccessory.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/AppDelegateShortcutRoutingTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9985d576a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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 `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 355-540: Wrap each affected test's setup+ invocation in a
deterministic shortcut context using withTemporaryShortcut(...) so the tests do
not depend on persisted UserDefaults remaps: for
testCmdPhysicalPWithDvorakCharactersDoesNotTriggerCommandPaletteSwitcher(),
testCmdShiftPhysicalPWithDvorakCharactersDoesNotTriggerCommandPalette(),
testCmdOptionPhysicalTWithDvorakCharactersDoesNotTriggerCloseOtherTabsShortcut(),
and testCmdPhysicalWWithDvorakCharactersDoesNotTriggerClosePanelShortcut() call
withTemporaryShortcut for the relevant actions (the command-palette-switcher
action, command-palette action, close-other-tabs action, and close-panel action
respectively) around the notification expectation/ASSERT and the call to
AppDelegate.debugHandleCustomShortcut(event:), so the inverted
NotificationCenter expectations and XCTAssertFalse/XCTAssertEqual checks run
with pinned shortcuts rather than whatever is in UserDefaults.
In `@Sources/AppDelegate.swift`:
- Around line 1017-1024: Store the result of
KeyboardLayout.character(forKeyCode: keyCode) in a local (e.g. let char =
KeyboardLayout.character(forKeyCode: keyCode)), then only run the ANSI fallback
(return keyCode == 3) when that char is empty / translation-unavailable;
otherwise preserve the existing special-case check for "f" and return false for
non-empty non-"f" characters. This ensures KeyboardLayout.character(forKeyCode:)
is consulted once and the ANSI fallback is gated to the “layout translation
unavailable” case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bb7615c1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
Addressed remaining review nits in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b58e463502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This was referenced Mar 7, 2026
This was referenced Mar 15, 2026
This was referenced Mar 22, 2026
This was referenced Apr 1, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Fix layout-safe command shortcut matching * Fix unshifted symbol shortcut coercion * Fix layout handling for hardcoded Cmd shortcuts * Support Cmd/Ctrl modifier hold shortcut hints * Address PR shortcut review feedback * Handle Caps Lock in command shortcut matching * Address remaining PR shortcut review comments * Handle Cmd+Ctrl+F control-character fallback * Tighten shortcut hint hold-delay test * Expose shortcut hint visibility in settings * Restore Cmd+digit fallback on symbol-first layouts * Stabilize shortcut regression coverage * Align shortcut hint toggle with Cmd/Ctrl behavior * Restore Claude workflow file * Match Claude workflow file to main * Keep shortcut hint hold behavior command-only * Restore command shortcut fallback when layout data is unavailable * Preserve command-aware shortcut translation
9 tasks
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.
Summary
Cmd+Cunder Dvorak no longer collides with theCmd+Inotifications shortcutCmd+Shift+]) still workCmd+Cbehavior, normalCmd+I, shifted symbol shortcuts, and non-US bracket fallback behaviorTesting
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -only-testing:cmuxTests/AppDelegateShortcutRoutingTests test(pass)codex --dangerously-bypass-approvals-and-sandbox --model gpt-5.3-codex -c model_reasoning_effort="medium" review --uncommitted(No findings)Issues
Summary by CodeRabbit
Bug Fixes
UX
Refactor
Tests