Repository navigation
fix(input): don't swallow unbound Cmd+Shift combinations - #1731
BillionClaw wants to merge 1 commit into
Conversation
Unbound Cmd+Shift+<key> combinations were being silently swallowed by the two-pass timestamp mechanism in performKeyEquivalent. When no menu item matches and no Ghostty binding exists, AppKit never redispatches the event, causing it to be lost. Now directly send unbound Cmd+Shift combinations through to keyDown so they reach terminal applications via the kitty keyboard protocol. Fixes manaflow-ai#1718
📝 WalkthroughWalkthroughThe change modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
📝 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 |
|
@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.
![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-4896: The current block sets lastPerformKeyEvent, calls
keyDown(with: event) and returns true, which bypasses AppKit menu handling;
modify it so that for Cmd+Shift combinations you first call
NSApp.mainMenu?.performKeyEquivalent(event) and if that returns true let the
menu handle it (do not call keyDown or return true), otherwise fall back to
setting lastPerformKeyEvent, calling keyDown(with: event) and returning true;
update the logic around lastPerformKeyEvent and the call site where
keyDown(with:) is invoked so menu routing is attempted before the fallback
dispatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5991e7af-377e-4afe-8287-8ac35794467b
📒 Files selected for processing (1)
Sources/GhosttyTerminalView.swift
| // First pass: instead of returning false and hoping for a redispatch | ||
| // (which AppKit may not do if no menu item matches), directly send | ||
| // unbound Cmd+Shift combinations through to keyDown so they reach the | ||
| // terminal application via the kitty keyboard protocol. | ||
| // This fixes issue #1718 where unbound Cmd+Shift+<key> was silently swallowed. | ||
| lastPerformKeyEvent = event.timestamp | ||
| return false | ||
| keyDown(with: event) | ||
| return true |
There was a problem hiding this comment.
Don’t consume unbound Cmd+Shift before menu routing.
Line 4895 now forwards unbound Command shortcuts straight to keyDown(with:) and returns true, which bypasses AppKit menu-equivalent handling. This can block app shortcuts like Cmd+Shift+P / Cmd+Shift+, / Cmd+Shift+T / Cmd+Shift+G when terminal focus is active.
Proposed fix
- // First pass: instead of returning false and hoping for a redispatch
- // (which AppKit may not do if no menu item matches), directly send
- // unbound Cmd+Shift combinations through to keyDown so they reach the
- // terminal application via the kitty keyboard protocol.
- // This fixes issue `#1718` where unbound Cmd+Shift+<key> was silently swallowed.
- lastPerformKeyEvent = event.timestamp
- keyDown(with: event)
- return true
+ let normalized = event.modifierFlags
+ .intersection(.deviceIndependentFlagsMask)
+ .subtracting([.capsLock, .numericPad, .function])
+ let isCmdShiftOnly = normalized == [.command, .shift]
+
+ if isCmdShiftOnly {
+ // Preserve app menu shortcuts first, then pass through unbound Cmd+Shift.
+ if let menu = NSApp.mainMenu, menu.performKeyEquivalent(with: event) {
+ return true
+ }
+ keyDown(with: event)
+ return true
+ }
+
+ // Keep existing non-Cmd+Shift behavior.
+ lastPerformKeyEvent = event.timestamp
+ return falseBased on learnings: command-key routing should attempt NSApp.mainMenu.performKeyEquivalent before fallback handling to avoid swallowed app shortcuts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 4889 - 4896, The current
block sets lastPerformKeyEvent, calls keyDown(with: event) and returns true,
which bypasses AppKit menu handling; modify it so that for Cmd+Shift
combinations you first call NSApp.mainMenu?.performKeyEquivalent(event) and if
that returns true let the menu handle it (do not call keyDown or return true),
otherwise fall back to setting lastPerformKeyEvent, calling keyDown(with: event)
and returning true; update the logic around lastPerformKeyEvent and the call
site where keyDown(with:) is invoked so menu routing is attempted before the
fallback dispatch.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:4895">
P1: Unbound Command key equivalents are now always consumed, which can block normal AppKit menu shortcut routing for non-Shift Cmd shortcuts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // This fixes issue #1718 where unbound Cmd+Shift+<key> was silently swallowed. | ||
| lastPerformKeyEvent = event.timestamp | ||
| return false | ||
| keyDown(with: event) |
There was a problem hiding this comment.
P1: Unbound Command key equivalents are now always consumed, which can block normal AppKit menu shortcut routing for non-Shift Cmd shortcuts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyTerminalView.swift, line 4895:
<comment>Unbound Command key equivalents are now always consumed, which can block normal AppKit menu shortcut routing for non-Shift Cmd shortcuts.</comment>
<file context>
@@ -4886,8 +4886,14 @@ class GhosttyNSView: NSView, NSUserInterfaceValidations {
+ // This fixes issue #1718 where unbound Cmd+Shift+<key> was silently swallowed.
lastPerformKeyEvent = event.timestamp
- return false
+ keyDown(with: event)
+ return true
}
</file context>
|
I'll get the CLA signed — will follow up once it's done. |
1 similar comment
|
I'll get the CLA signed — will follow up once it's done. |
|
Closing per repository blocklist: maintainer threatened to ban. All submissions to this repo have been suspended. |
2 participants
Fixes #1718
Unbound Cmd+Shift+ combinations were being silently swallowed.
Now properly passed through to terminal applications via the kitty keyboard protocol.
Summary by CodeRabbit
Bug Fixes
Summary by cubic
Ensure unbound Cmd+Shift+ shortcuts pass through to terminal apps instead of being swallowed by AppKit. We now route them directly to keyDown in performKeyEquivalent so they are sent via the kitty keyboard protocol (fixes #1718).
Written for commit 65f4c86. Summary will update on new commits.