Repository navigation
Match Ghostty keyboard input architecture - #8770
Closed
lawrencecchen wants to merge 116 commits into
Closed
lawrencecchen wants to merge 116 commits into
lawrencecchen wants to merge 116 commits into
Conversation
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis change introduces explicit terminal key-input planning, integrates AppKit text interpretation with IME state and physical-key replay, simplifies keyboard translation, and revises Option shortcut routing. It adds extensive planner, Unicode, keyboard-layout, IME, and shortcut regression coverage. ChangesIME and Option Key Routing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AppKit
participant AppDelegate
participant GhosttyNSView
participant TerminalKeyInputPlanner
participant libghostty
AppKit->>AppDelegate: keyDown Option candidate
AppDelegate->>AppDelegate: match configured shortcut
AppDelegate->>GhosttyNSView: dispatch unmatched Option input
GhosttyNSView->>TerminalKeyInputPlanner: plan interpreted snapshot
TerminalKeyInputPlanner->>libghostty: send committed text or physical key
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (21 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 replaces cmux's ad-hoc keyboard-input dispatch with a two-layer architecture borrowed from Ghostty: native key events are first routed through
Confidence Score: 5/5The change replaces ad-hoc IME and input-source branches with a well-bounded planner and two lifecycle trackers; the invariants are explicit and the test coverage is thorough. No correctness defects were found. Every dispatch path through keyDown, performKeyEquivalent, and the local monitor now flows through the lifecycle trackers, eliminating the prior race between multiple AppKit entrypoints. The adoption of NSTextInputContext.handleEvent's native consumed/not-consumed verdict is structurally sound. No ambient global state, blocking primitives, test/debug seams, or actor-isolation mistakes were introduced. Files Needing Attention: No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Mon as Local Key Monitor
participant AppKit as AppKit Responder Chain
participant GNV as GhosttyNSView
participant TIC as NSTextInputContext
participant Plan as TerminalKeyInputPlanner
participant LT as TerminalKeyInputLifecycleTracker
participant ST as ShortcutKeyPressLifecycleTracker
participant Ghost as libghostty
Mon->>ST: prepareKeyDown(keyCode, identity, isRepeat)
alt shortcut claimed
ST-->>Mon: .dispatch / .consume
Mon-->>AppKit: event consumed
else pass through
Mon-->>AppKit: forward event
AppKit->>GNV: keyDown(event)
GNV->>GNV: withGhosttyBindingKeyEvent (probe)
GNV->>LT: physicalIdentityForBindingProbe
GNV->>TIC: handleTextInputEvent(event)
TIC-->>GNV: textInputConsumed (Bool)
Note over GNV: insertText / doCommand callbacks populate committedText
GNV->>Plan: plan(for: snapshot)
Plan-->>GNV: TerminalKeyInputPlan(actions)
GNV->>LT: actions(for: plan, isRepeat, eventIdentity)
LT-->>GNV: filtered [TerminalKeyInputAction]
loop each action
GNV->>Ghost: sendCommittedText / sendGhosttyKey
end
end
AppKit->>GNV: keyUp(event)
GNV->>LT: release(forKeyUp: keyCode)
LT-->>GNV: TerminalKeyInputRelease(forwardsPhysical, identity)
alt forwardsPhysical
GNV->>Ghost: ghostty_surface_key (RELEASE)
end
Reviews (48): Last reviewed commit: "fix: dispatch shortcuts outside lifecycl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 2
🤖 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.
Inline comments:
In
`@Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputPlanner.swift`:
- Around line 62-69: Update shouldSuppressControlText(_ text:composing:) so the
single-scalar control-character check also treats U+007F (DEL) as suppressible
during marked-text composition, while preserving existing behavior for other
text. Add a planner test covering forward-delete during composition and verify
it remains in AppKit’s preedit flow rather than emitting .sendKey.
In `@Sources/GhosttyTextInputSupport.swift`:
- Around line 5-7: Update isControlCharacterScalar so Unicode scalar value 0x7F
is classified as a control character alongside values below 0x20. Preserve the
existing keycode-side encoding in textForKeyEvent and sendGhosttyKey so
Backspace and Forward-Delete do not send literal DELETE text.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98cb55c4-4ee7-4684-9104-cdffd90ca38d
📒 Files selected for processing (20)
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputAction.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputEvent.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputKey.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputPlanner.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Input/TerminalKeyInputSnapshot.swiftPackages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Input/TerminalKeyInputPlannerTests.swiftSources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/GhosttyNSView+IMEComposition.swiftSources/GhosttyTerminalView.swiftSources/GhosttyTextInputSupport.swiftSources/KeyboardLayout.swiftSources/KeyboardShortcutSettings.swiftcmuxTests/AppDelegateOptionDigitShortcutRoutingTests.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/CJKIMEInputTests.swiftcmuxTests/CJKIMEMarkedSelectionTests.swiftcmuxTests/GhosttyOptionAsAltModsTests.swiftcmuxTests/TraditionalChineseIMENumpadRegressionTests.swiftcmuxTests/WindowKeyDownReplayGuardTests.swift
💤 Files with no reviewable changes (1)
- Sources/KeyboardShortcutSettings.swift
Contributor
Author
|
Expanded keyboard coverage in 9175039.
The repository blocks local app-host test execution, so the pushed head leaves the expanded Dictionary: An app-host test loads the test bundle inside a real cmux application process. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Input/TerminalKeyInputPlannerTests.swift`:
- Around line 144-148: Replace the force cast in the input-source iteration with
a safe cast to TISInputSource, and skip or otherwise safely handle elements that
cannot be cast. Preserve processing for valid sources while removing the
SwiftLint force_cast violation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e8e27a4-1dbe-470f-ba84-178b2776b8c3
📒 Files selected for processing (2)
Packages/macOS/CmuxTerminal/Tests/CmuxTerminalTests/Input/TerminalKeyInputPlannerTests.swiftcmuxTests/CJKIMEMarkedSelectionTests.swift
…rchitecture # Conflicts: # cmux.xcodeproj/project.pbxproj
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
This was referenced Jul 31, 2026
lawrencecchen
added
the
stale-revisit
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
2 participants
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
Testing
swift test --package-path Packages/macOS/CmuxTerminal: 174 Swift tests in 23 suites plus 11 XCTest cases passed.git diff --check,scripts/check-pbxproj.sh,plutil, andscripts/lint-pbxproj-test-wiring.shpassed.$autoreview, Greptile, CodeRabbit, and Socket Security found no unresolved actionable findings. GitHub reports zero unresolved review threads.Review Trigger
Review the AppKit/Ghostty ownership boundary, exact release ownership, and the prepare/dispatch/reconcile shortcut lifecycle.
Checklist