Repository navigation
Fix unbound Cmd+Shift forwarding to terminal (#1718) - #3332
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR changes GhosttyNSView's second-pass key-equivalent handling so non-Command key events on the second pass are no longer deferred: the stored timestamp is cleared, the event is redispatched to ChangesKey-equivalent handling fix
Test integration
Sequence Diagram(s)sequenceDiagram
participant AppKit
participant GhosttyNSView
participant TerminalSurface
AppKit->>GhosttyNSView: performKeyEquivalent(event) (first pass)
note right of GhosttyNSView: first pass may store timestamp and return false
AppKit->>GhosttyNSView: performKeyEquivalentAfterMenuMiss(event) (second pass)
alt shouldRetryMainMenu == false (second pass for unbound non-Command)
GhosttyNSView->>GhosttyNSView: clear lastPerformKeyEvent
GhosttyNSView->>GhosttyNSView: keyDown(with: event)
GhosttyNSView->>TerminalSurface: forward key event (GHOSTTY_ACTION_PRESS)
GhosttyNSView-->>AppKit: return true
else shouldRetryMainMenu == true (preserve prior behavior)
GhosttyNSView->>GhosttyNSView: set lastPerformKeyEvent = event.timestamp
GhosttyNSView-->>AppKit: return false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 12 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 a silent key-drop bug where unbound
Confidence Score: 5/5Safe to merge — the change is minimal, confined to one branch of the key-routing function, and covered by a new regression test plus manual tagged-build verification. The production change is a single guard that short-circuits an already-known-to-be-post-menu-miss code path; bound Ghostty keys are handled earlier in the same function and are unaffected. The new test class exercises the exact failure scenario end-to-end, and the debug log from the tagged dev build confirms the key reaches No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit
participant Window as Window routing
participant GNSView as GhosttyNSView
participant Menu as NSApp.mainMenu
participant Ghostty as ghostty_surface_key
AppKit->>Window: performKeyEquivalent(Cmd+Shift+K)
Window->>GNSView: performKeyEquivalent(shouldRetryMainMenu:true)
GNSView->>Ghostty: ghostty_surface_key_is_binding?
Ghostty-->>GNSView: false (unbound)
GNSView->>GNSView: lastPerformKeyEvent = timestamp
GNSView-->>Window: false
Window->>Menu: performKeyEquivalent(Cmd+Shift+K)
Menu-->>Window: false (no menu item)
Window->>GNSView: performKeyEquivalentAfterMenuMiss(shouldRetryMainMenu:false)
GNSView->>Ghostty: ghostty_surface_key_is_binding?
Ghostty-->>GNSView: false (unbound)
Note over GNSView: NEW: !shouldRetryMainMenu branch
GNSView->>GNSView: lastPerformKeyEvent = nil
GNSView->>GNSView: keyDown(with: event)
GNSView-->>Window: true (event handled)
Reviews (2): Last reviewed commit: "Tighten the Cmd+Shift forwarding regress..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 b79c171. Configure here.
Adds a focused AppKit-hosted Ghostty regression test for the after-menu-miss route. The test drives an unbound Cmd+Shift+K NSEvent through performKeyEquivalentAfterMenuMiss and expects Ghostty keyDown observation instead of an AppKit swallow. Constraint: Regression tests must exercise runtime behavior rather than source text. Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Not-tested: Local XCTest run skipped per repository testing policy; CI owns the executable test run.
When the window-level router has already missed the main menu, Ghostty should not wait for AppKit to redispatch an unbound command-modified event. The fallback now forwards the original event to keyDown immediately on the menu-miss path while keeping the existing two-pass redispatch for callers that still need to retry the main menu. Constraint: Bound Ghostty actions must stay on the existing ghostty_surface_key_is_binding path. Rejected: Keep relying on timestamp replay after a menu miss | AppKit can drop unbound Cmd+Shift events without a second pass. 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 and manual app verification pending after tagged reload/launch.
austinywang
force-pushed
the
issue-1718-cmd-shift-swallowed-regression
branch
from
April 30, 2026 01:37
b79c171 to
e3a61db
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/GhosttyCommandShiftForwardingTests.swift`:
- Around line 68-69: The test ignores the Bool result of
window.makeFirstResponder(surfaceView) so focus assignment failures are hidden;
update the setup to capture and assert that makeFirstResponder(surfaceView)
returns true (or call XCTAssertTrue(...) with a clear failure message) before
asserting XCTAssertNotNil(surfaceView.terminalSurface) so a failed handoff fails
the test; reference window.makeFirstResponder and surfaceView.terminalSurface
when making the assertion.
- Around line 71-77: The current observer captures only the last matching
keyEvent via forwardedKeyEvent, which hides duplicate dispatches; add an Int
counter (e.g., forwardedPressCount) alongside forwardedKeyEvent in the
GhosttyNSView.debugGhosttySurfaceKeyEventObserver closure, increment it when the
guard condition (keyEvent.action == GHOSTTY_ACTION_PRESS && keyEvent.keycode ==
40) matches, store the first matching event if forwardedKeyEvent is nil, and at
the end of the test assert forwardedPressCount == 1 (and that forwardedKeyEvent
is non-nil); update both occurrences around
GhosttyNSView.debugGhosttySurfaceKeyEventObserver (the block using
previousKeyEventObserver and forwardedKeyEvent) and the second similar block at
lines 97-101.
🪄 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: 38b95544-8658-4c98-a51b-31f181acf2f3
📒 Files selected for processing (3)
GhosttyTabs.xcodeproj/project.pbxprojSources/GhosttyTerminalView.swiftcmuxTests/GhosttyCommandShiftForwardingTests.swift
Merged the current main branch into the PR branch and resolved the project-file conflict by preserving both new test registrations. The Swift routing fix stayed intact: terminal-focused command events still go to the app menu first, then Ghostty owns delivery after a menu miss. Constraint: PR branch must preserve unbound Cmd+Shift terminal forwarding while accepting main's project structure Confidence: high Scope-risk: narrow Tested: plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; git diff --check; conflict-marker scan on resolved files Not-tested: Local XCTest skipped per repository policy; CI will verify after push Co-authored-by: OmX <omx@oh-my-codex.dev>
The review feedback was correct that capturing only the last Ghostty key event would miss duplicate dispatches. The regression now counts matching press events, preserves the first observed key payload, asserts one delivery, and verifies first-responder setup before exercising the menu-miss path. Constraint: Local XCTest is owned by CI for this repo Rejected: Leave duplicate dispatch uncovered | the bug class is specifically about terminal key delivery guarantees 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 skipped per repository policy Co-authored-by: OmX <omx@oh-my-codex.dev>
This was referenced May 11, 2026
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 #1718
Root cause
PR #1959 fixed the second-pass Cmd+Shift fallback by using
charactersIgnoringModifiers, but a later window-level command routing path now asks the main menu first and calls Ghostty only after a menu miss. For an unbound Cmd+Shift key such as Cmd+Shift+K, Ghostty recorded the timestamp and returnedfalseexpecting AppKit to redispatch the event. When no menu item and no Ghostty binding matched, AppKit could skip that second pass, so the key was silently dropped before it reached the terminal/kitty keyboard protocol.Fix
When
performKeyEquivalentAfterMenuMissis already on the post-menu-miss path and no Ghostty binding matched, forward the original event directly throughkeyDownand returntrue. Bound Ghostty keys still use the existingghostty_surface_key_is_bindingbranch first, so keys like Cmd+Shift+J remain routed through their binding path without double dispatch.Test structure
GhosttyCommandShiftForwardingTests.testUnboundCommandShiftKeyAfterMenuMissForwardsToGhosttyKeyDown, which exercises the menu-miss path with Cmd+Shift+K and expects the focused Ghostty surface to receive a press event.Verification
git diff --checkpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvtests,workflow-guard-tests,ui-regressions,tests-build-and-lag, both compat lanes,release-build,build-ghosttykit, web checks, and reviewer checks.CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-1718-cmd-shift-swallowed-regression --launch..was attempted, but this repo'sreload.shrejects.as an unknown option.cat -vin the 1718 dev terminal window and sent Cmd+Shift+K to that app process./tmp/cmux-debug-issue-1718-cmd-shift-swallowed-regression.logrecordedfocus.keyRepair ... keyCode=40 mods=1179648followed byforceRefresh ... reason=keyDown.textInput, confirming the unbound Cmd+Shift+K reached GhosttykeyDowninstead of being swallowed.Note
Medium Risk
Touches terminal key-equivalent routing, which can subtly affect shortcut handling and event dispatch order, but the change is small and covered by a focused regression test.
Overview
Fixes a regression where unbound
Cmd+Shiftkey presses could be swallowed after the app routes through the main menu first: whenperformKeyEquivalentAfterMenuMissis used and no Ghostty binding matches, the original event is now forwarded directly tokeyDownand consumed.Adds a new unit test
GhosttyCommandShiftForwardingTestsand wires it into the Xcode project to verify that an unboundCmd+Shift+Kreaches the terminal exactly once (including expected modifier flags).Reviewed by Cursor Bugbot for commit 33eec9b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit