Fix macOS modifier desync causing idle terminal input corruption - #2855
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdded a helper to map macOS modifier keycodes and modifier-flag state to Ghostty PRESS/RELEASE (or nil). Updated flagsChanged handling to skip when IME marked text exists and to use the helper before sending synthesized Ghostty modifier events. Added tests for side-specific modifier behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant NSView as GhosttyNSView
participant ModAction as cmuxGhosttyModifierActionForFlagsChanged
participant Ghostty as Ghostty Input
User->>NSView: flagsChanged(with: NSEvent)
NSView->>NSView: hasMarkedText()?
alt IME marked text exists
NSView-->>Ghostty: skip modifier synthesis
else No marked text
NSView->>ModAction: keyCode + modifierFlagsRawValue
ModAction->>ModAction: map keyCode to modifier type\ndetermine PRESS or RELEASE (or nil)
ModAction-->>NSView: ghostty_input_action_e?
alt action != nil
NSView->>Ghostty: send key event (PRESS/RELEASE, codepoint=0)
else action == nil
NSView-->>Ghostty: no modifier event sent
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Greptile SummaryThis PR fixes terminal input corruption after idle (issue #2853) by extracting Confidence Score: 4/5Safe to merge for the primary fix; a symmetric left-side modifier gap remains that could cause the same phantom-modifier issue in rare dual-modifier scenarios. Both findings are P2: the left-side device-mask gap requires simultaneously holding both left and right of the same modifier then releasing the left one — an unusual input pattern — and the missing test is a companion to that gap. The core fix (right-side detection, correct PRESS/RELEASE edges) is correct and well-tested. Scoring 4 rather than 5 because the left-side logic issue is the exact same class of phantom-modifier bug the PR is intended to close, and it is straightforward to fix symmetrically. Sources/GhosttyTerminalView.swift — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[flagsChanged event arrives\nkeyCode + modifierFlagsRawValue] --> B{keyCode is a\nknown modifier?}
B -- No --> C[return nil\nskip Ghostty event]
B -- Yes --> D{Device-independent\nmodifier flag active?}
D -- No --> E[return GHOSTTY_ACTION_RELEASE]
D -- Yes --> F{keyCode is a\nright-side key?\n0x3C 0x3E 0x3D 0x36}
F -- No\nleft-side / caps lock --> G[sidePressed = true\ndefault branch]
F -- Yes --> H{Device-specific\nNX_DEVICER*KEYMASK set?}
H -- Yes --> I[sidePressed = true]
H -- No --> J[sidePressed = false]
G --> K{sidePressed?}
I --> K
J --> K
K -- Yes --> L[return GHOSTTY_ACTION_PRESS]
K -- No --> M[return GHOSTTY_ACTION_RELEASE]
style G fill:#ffdddd,stroke:#ff8888
style L fill:#ddffdd,stroke:#88cc88
style M fill:#ddeeff,stroke:#88aaff
Reviews (1): Last reviewed commit: "Fix modifier release handling after idle" | 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: 1
🧹 Nitpick comments (1)
cmuxTests/TerminalAndGhosttyTests.swift (1)
3901-3977: Consider adding right Option / right Control cases to round out side-specific coverage.Given this helper is now the single decision point, adding explicit assertions for
NX_DEVICERALTKEYMASKandNX_DEVICERCTLKEYMASKwould make the suite more future-proof.Based on learnings Repo: manaflow-ai/cmux — In
cmuxTests/CJKIMEInputTests.swift, deterministic right-option behavior is validated via the directNX_DEVICERALTKEYMASKpath.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 3901 - 3977, Add two additional test methods in GhosttyModifierFlagsChangedActionTests to cover right-Option and right-Control side-specific behavior: create tests mirroring testRightShiftRequiresRightSideDeviceMaskForPress and testRightShiftWithoutRightSideDeviceMaskReturnsRelease but use keyCode values for Option (right Option keycode) and Control (right Control keycode) and assert that cmuxGhosttyModifierActionForFlagsChanged returns GHOSTTY_ACTION_PRESS when modifierFlagsRawValue includes NSEvent.ModifierFlags.option.rawValue (or .control.rawValue) combined with UInt(NX_DEVICERALTKEYMASK) / UInt(NX_DEVICERCTLKEYMASK) respectively, and GHOSTTY_ACTION_RELEASE when the device mask is omitted; use the same helper function name cmuxGhosttyModifierActionForFlagsChanged to locate where behavior is decided.
🤖 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 56-94: The function cmuxGhosttyModifierActionForFlagsChanged
currently treats left-side modifier keyCodes (0x38,0x3B,0x3A,0x37) as
sidePressed = true in the default branch, causing left-side releases to be
mis-detected when the opposite side remains held; update the second switch in
cmuxGhosttyModifierActionForFlagsChanged to explicitly check the left-side
device masks (use NX_DEVICELSHIFTKEYMASK for 0x38, NX_DEVICELCTLKEYMASK for
0x3B, NX_DEVICELALTKEYMASK for 0x3A, NX_DEVICELCMDKEYMASK for 0x37) the same way
right-side keys check NX_DEVICER*KEYMASK values, and add a regression test that
verifies releasing a left modifier while the right modifier remains held (and
vice versa) yields GHOSTTY_ACTION_RELEASE for the release event and
GHOSTTY_ACTION_PRESS for the held side.
---
Nitpick comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 3901-3977: Add two additional test methods in
GhosttyModifierFlagsChangedActionTests to cover right-Option and right-Control
side-specific behavior: create tests mirroring
testRightShiftRequiresRightSideDeviceMaskForPress and
testRightShiftWithoutRightSideDeviceMaskReturnsRelease but use keyCode values
for Option (right Option keycode) and Control (right Control keycode) and assert
that cmuxGhosttyModifierActionForFlagsChanged returns GHOSTTY_ACTION_PRESS when
modifierFlagsRawValue includes NSEvent.ModifierFlags.option.rawValue (or
.control.rawValue) combined with UInt(NX_DEVICERALTKEYMASK) /
UInt(NX_DEVICERCTLKEYMASK) respectively, and GHOSTTY_ACTION_RELEASE when the
device mask is omitted; use the same helper function name
cmuxGhosttyModifierActionForFlagsChanged to locate where behavior is decided.
🪄 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: 9513c489-d1c9-4af0-aefa-723a3997f103
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
![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 2 potential issues.
Bugbot Autofix is kicking off a free cloud agent to fix these issues. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fda8daa. Configure here.
![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 2 files
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:90">
P1: Left-side modifier releases are misclassified as presses when the opposite-side modifier is still held.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
austinywang
changed the title
|
Addressed the review feedback on this branch:
Latest branch tip: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/TerminalAndGhosttyTests.swift (1)
4002-4010: Add a right-Command release parity case.Line [4002] verifies right-Command press, but there’s no symmetric release case when left Command is still held (aggregate
.commandremains set). Adding that protects against side-desync regressions on Command, similar to Shift coverage.Proposed test addition
func testRightCommandRequiresRightSideDeviceMaskForPress() { XCTAssertEqual( cmuxGhosttyModifierActionForFlagsChanged( keyCode: 0x36, modifierFlagsRawValue: NSEvent.ModifierFlags.command.rawValue | UInt(NX_DEVICERCMDKEYMASK) ), GHOSTTY_ACTION_PRESS ) } + + func testRightCommandWithoutRightSideDeviceMaskReturnsReleaseWhenLeftCommandHeld() { + XCTAssertEqual( + cmuxGhosttyModifierActionForFlagsChanged( + keyCode: 0x36, + modifierFlagsRawValue: NSEvent.ModifierFlags.command.rawValue | UInt(NX_DEVICELCMDKEYMASK) + ), + GHOSTTY_ACTION_RELEASE + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 4002 - 4010, Add a symmetric release test for right-Command: create a new test (e.g., testRightCommandRequiresRightSideDeviceMaskForRelease) that calls cmuxGhosttyModifierActionForFlagsChanged(keyCode: 0x36, modifierFlagsRawValue: NSEvent.ModifierFlags.command.rawValue) — i.e., aggregate .command remains set but the NX_DEVICERCMDKEYMASK is not present — and assert the result equals GHOSTTY_ACTION_RELEASE to mirror the existing press case (cmuxGhosttyModifierActionForFlagsChanged and GHOSTTY_ACTION_PRESS) and guard against side-desync regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 4002-4010: Add a symmetric release test for right-Command: create
a new test (e.g., testRightCommandRequiresRightSideDeviceMaskForRelease) that
calls cmuxGhosttyModifierActionForFlagsChanged(keyCode: 0x36,
modifierFlagsRawValue: NSEvent.ModifierFlags.command.rawValue) — i.e., aggregate
.command remains set but the NX_DEVICERCMDKEYMASK is not present — and assert
the result equals GHOSTTY_ACTION_RELEASE to mirror the existing press case
(cmuxGhosttyModifierActionForFlagsChanged and GHOSTTY_ACTION_PRESS) and guard
against side-desync regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8b8b928-c487-4a31-9e1a-f4b44b22a214
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
… composition Closes manaflow-ai#2949 ## Root Cause manaflow-ai#2855 added the modifier press/release dispatch but wrapped it in 'if !hasMarkedText()'. macOS 'flagsChanged' is the only event that fires for modifier transitions ('keyUp' does not fire for modifier- only events), so when the user releases Shift / Control / Option / Command during an IME composition the release never reaches Ghostty. After 'unmarkText' commits the composition, Ghostty still believes the suppressed modifier is in its previous state and the next keystroke is mis-routed - the same class of corruption manaflow-ai#2855 was meant to fix. ## Fix Flip the seam introduced in the previous commit so the modifier action is always forwarded; the IME composition state rides along on 'CmuxFlagsChangedDecision.composing' (mapped onto 'ghostty_input_key_s.composing') so Ghostty can suppress side effects on its side without cmux dropping the edge. The text-bearing path elsewhere in the file already short-circuits on 'hasMarkedText', so removing the guard here does not regress text input. ## Tests The matching XCTest cases land in the previous commit (test-first) so CI shows the regression coverage red->green progression.
…nal-input-corruption-idle Fix macOS modifier desync causing idle terminal input corruption
This branch was successfully deployed
1 participant
Summary
flagsChangedhandling so modifier-only events emit correct press/release edgesflagsChangedevents off AppKit character APIs to avoid crashes on non-text eventsTesting
Fixes #2853
Summary by CodeRabbit
Bug Fixes
Tests