Repository navigation
Fix focus history shortcut rebinding - #8853
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughShortcut routing now checks configured shortcuts for printable Option events before text-input dispatch. Marked-text inputs defer shortcut handling, matching requires keydown events, and regression tests cover rebinding, modifier variants, window targeting, and editor behavior. ChangesShortcut routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant NSEvent
participant AppDelegate
participant ShortcutMatcher
participant FocusedTextTarget
NSEvent->>AppDelegate: keyDown printable Option event
AppDelegate->>ShortcutMatcher: match configured shortcut
ShortcutMatcher-->>AppDelegate: handled or unmatched result
AppDelegate->>FocusedTextTarget: dispatch unmatched input when marked text is active
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (23 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 fixes focus-history shortcut rebinding (#8849) by routing configured shortcuts through the live dispatcher before Option-generated text input, and by making
Confidence Score: 5/5The changes are well-scoped to the shortcut routing layer, covered by intentional regression tests, and do not touch persistence schemas or user data. The routing overhaul is thoroughly tested — six new tests across AppDelegate, FilePreviewTextEditor, TextBoxInput, and the recorder cover Option, Shift+Option, Cmd+Shift, Ctrl, and Shift+Opt+Ctrl rebinds as well as marked-text precedence. The key-code fast path is a pure simplification with no new state. The only observation is a style point in the new canonicalization module. ShortcutKeyCanonicalization.swift — the two public top-level functions are a minor style point; no correctness concerns in any file. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant Win as NSWindow.cmux_performKeyEquivalent
participant Del as AppDelegate.handleCustomShortcut
participant Resp as Text Responder (Ghostty/WebKit/NSTextView)
AppKit->>Win: keyDown (Option+Y)
Win->>Win: shortcutRoutingShouldBypassForPrintableOptionText?
alt not printable Option (e.g. Cmd+Y)
Win->>Del: standard shortcut routing path
Del-->>Win: handled / not handled
else printable Option event
Win->>Win: browserWebKitKeyDownReentry? → bail if true
Win->>Win: firstResponderHasMarkedText?
alt marked text active
Win->>Resp: cmuxForceDispatchKeyDownOnce("unmatched Option input")
else no marked text
Win->>Del: handleConfiguredShortcutKeyEquivalent
Del->>Del: browserResponderHasMarkedText? → clearChordState + return false if true
Del->>Del: ShortcutStroke.matches(keyCode:…)
note over Del: Fast path: if self.keyCode present,<br/>return keyCode == recordedKeyCode
alt configured shortcut matched
Del-->>Win: true (consumed)
else no match
Win->>Resp: cmuxForceDispatchKeyDownOnce("unmatched Option input")
end
end
end
Reviews (11): Last reviewed commit: "Match recorded shortcuts by physical key" | Re-trigger Greptile |
![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/AppDelegate.swift (1)
17057-17076: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard the configurable Option shortcut handler during IME composition.
firstResponderHasMarkedTextis available before this branch, and the existing Ghostty composition guard a few lines below relies onghosttyView.hasMarkedText(). This new bypass callshandleConfiguredShortcutKeyEquivalent(event)first for any printable-Option event, so a configured shortcut can consume an in-progress Option diacritic/composition before that guard runs. Gate this shortcut check on!firstResponderHasMarkedText(orghosttyView.hasMarkedText()when Ghostty is the responder), matching the existing IME guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AppDelegate.swift` around lines 17057 - 17076, Guard the handleConfiguredShortcutKeyEquivalent call within the shortcutRoutingShouldBypassForPrintableOptionText branch so it runs only when no IME composition is active, using firstResponderHasMarkedText or the responder-specific Ghostty hasMarkedText check already used nearby. Preserve the existing text-input dispatch and re-entry behavior while preventing configured shortcuts from consuming in-progress composition input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 17057-17076: Guard the handleConfiguredShortcutKeyEquivalent call
within the shortcutRoutingShouldBypassForPrintableOptionText branch so it runs
only when no IME composition is active, using firstResponderHasMarkedText or the
responder-specific Ghostty hasMarkedText check already used nearby. Preserve the
existing text-input dispatch and re-entry behavior while preventing configured
shortcuts from consuming in-progress composition input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7a581a8b-01fa-4e05-9daa-3ed122b82afb
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/KeyboardShortcutSettings.swiftcmuxTests/AppDelegateOptionDigitShortcutRoutingTests.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
💤 Files with no reviewable changes (1)
- Sources/KeyboardShortcutSettings.swift
added 6 commits
July 24, 2026 03:02
added 10 commits
July 24, 2026 04:00
…ory-rebind # Conflicts: # Sources/ControlSurfaceResumeTarget.swift # Sources/DockSplitStore+SessionSnapshot.swift
This was referenced Jul 25, 2026
This was referenced Jul 31, 2026
This was referenced Jul 31, 2026
2 tasks done
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
Fixes #8849
Root cause
The recorder, JSON store, settings reload, menu updates, titlebar actions, and KeyboardShortcutSettings.shortcut(for:) lookup all reflected the rebound shortcut correctly. The event died in the shared runtime routing path before the focus-history action block:
That made settings accept bindings such as Option+Y while runtime matching could never own them. The old defaults are not cached; after a rebind they cease matching as expected.
Fix
ShortcutStroke matching is now a pure comparison against the live configured shortcut. The shared AppDelegate dispatcher no longer rejects printable Option input before checking actions. At the NSWindow fallback chokepoint, configured shortcuts get first chance; only unmatched Option input is forwarded as text. This applies to all app-owned actions in the dispatcher rather than special-casing focus history.
Regression proof
The commits intentionally preserve the required red/green structure:
The regression exercises both directions through AppDelegate, verifies new live bindings navigate history, and verifies Cmd+[ / Cmd+] no longer match or dispatch after rebinding.
Local validation
git diff --checkscripts/lint-pbxproj-test-wiring.sh(577 test files)scripts/check-workspace-package-groups.py --checkscripts/check-package-resolved-policy.pyThe requested
scripts/swift_file_length_budget.pyis absent on currentmain; no budget TSVs were changed or generated. A diagnostic run of the unchanged current-main TextBox IME layout suite remains runner-failing outside this patch, so its auxiliary responder test was removed and the suite file restored exactly toorigin/main. Per task instructions, no localxcodebuildor app build was run. No user-facing strings were introduced; the localization audit found no catalog changes required.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes Focus Back/Forward rebinds by giving configured shortcuts first pass over printable Option input while preserving IME. Normalizes recorded/stored keys to physical base keys and matches by keyCode so shifted symbols and non‑US layouts record and fire correctly; Cmd+[ / Cmd+] no longer trigger after rebinding (fixes #8849).
Written for commit 02ff2ed. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests