Repository navigation
fix(keyboard): forward unbound Cmd+Shift combinations to terminal - #1755
BillionClaw wants to merge 2 commits into
Conversation
Unbound Cmd+Shift+<key> combinations were being silently swallowed by the AppKit key-routing layer and never reached the terminal via the kitty keyboard protocol. The root cause was in performKeyEquivalent's two-pass mechanism: 1. First pass returned false, expecting AppKit to redispatch 2. When no menu item matched, AppKit never redispatched the event The fix checks on the first pass if any menu item would handle the event. If not, the key is processed directly instead of relying on AppKit redispatch. Fixes manaflow-ai#1718
|
@BillionClaw is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a pre-check and conditional menu-dispatch gate in GhosttyNSView.performKeyEquivalent(_:): it may clear the stored timestamp and bypass AppKit routing, or set the timestamp and call NSApp.mainMenu?.performKeyEquivalent(with:), consuming the event if the menu handled it, otherwise falling back to existing handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GhosttyNSView
participant AppMenu as NSApp.mainMenu
participant TerminalBackend
User->>GhosttyNSView: key event (Cmd+...)
GhosttyNSView->>GhosttyNSView: shouldRouteCommandEquivalentDirectlyToMainMenu(event)?
alt shouldRoute == false
GhosttyNSView-->>User: return false (clear lastPerformKeyEvent)
Note right of GhosttyNSView: immediate menu routing bypassed
else shouldRoute == true
GhosttyNSView->>GhosttyNSView: lastPerformKeyEvent = event.timestamp
GhosttyNSView->>AppMenu: performKeyEquivalent(with: event)
alt AppMenu handled
AppMenu-->>GhosttyNSView: true
GhosttyNSView-->>User: event consumed
else not handled
AppMenu-->>GhosttyNSView: false
GhosttyNSView->>TerminalBackend: fallback: process event characters/equivalent
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4889-4897: The current first-pass unconditionally calls
NSApp.mainMenu?.performKeyEquivalent(with: event), which bypasses the intended
exclusion for certain Command-key combos (e.g., Cmd+`); update the logic in the
key handling block (the code around lastPerformKeyEvent and the
performKeyEquivalent call) to consult
shouldRouteCommandEquivalentDirectlyToMainMenu(event) before routing to
NSApp.mainMenu, and only call menu.performKeyEquivalent(with: event) when that
helper permits routing (preserving the existing fallback to terminal handling
when it does not); ensure you reference the existing symbols
lastPerformKeyEvent, shouldRouteCommandEquivalentDirectlyToMainMenu(_:),
NSApp.mainMenu, and performKeyEquivalent(with:) so Cmd+` (keyCode 50) and other
intentionally excluded shortcuts are not routed through the main menu.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97b52d57-65d3-4f9d-a11b-34b5f9eb6d34
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
…efore menu routing - Only route to NSApp.mainMenu if shouldRouteCommandEquivalentDirectlyToMainMenu permits it; otherwise fall through to terminal handling - Preserves intended exclusion for Cmd+` (keyCode 50) and non-menu command paths - Addresses reviewer feedback that unconditional main-menu routing bypasses command-routing filtering and can regress intentionally excluded shortcuts
|
Closing older duplicate PR; keeping the newer open PR for review. |
|
Closing older duplicate PR to keep the newest active submission for this fix. |
|
Thanks for the review! Glad the updated performKeyEquivalent path works as expected. I'll keep an eye out for any further feedback. |
|
Thanks for the review! Glad the performKeyEquivalent path is working well. Let me know if you'd like any further adjustments before merging — happy to make changes. |
|
Glad the fix is working well! Happy to make any further adjustments if needed before merge. |
|
This is BillionClaw. Happy to discuss the approach or make adjustments to the fix. |
|
Thank you for the approval! Happy to address any final comments. |
|
Closing older duplicate PR; keeping the newest open. |
2 participants
Unbound Cmd+Shift+ combinations were being silently swallowed by the AppKit key-routing layer and never reached the terminal via the kitty keyboard protocol.
The root cause was in performKeyEquivalent's two-pass mechanism:
The fix checks on the first pass if any menu item would handle the event. If not, the key is processed directly via keyDown instead of relying on AppKit redispatch.
Fixes #1718
Summary by cubic
Forward unbound Cmd+Shift+ events to the terminal instead of letting AppKit swallow them, fixing #1718. On the first pass we gate main menu routing with shouldRouteCommandEquivalentDirectlyToMainMenu and, if allowed but no item matches, handle via keyDown so the event reaches the kitty keyboard protocol; excluded shortcuts like Cmd+` remain unchanged.
Written for commit e0802e9. Summary will update on new commits.
Summary by CodeRabbit