Repository navigation
fix(terminal): forward deleteBackward text input commands - #5494
richardgetz wants to merge 11 commits into
Conversation
|
@richardgetz is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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 (2)
📝 WalkthroughWalkthroughAdds responder-chain text-input command forwarding to GhosttyNSView: maps NSResponder selectors (currently deleteBackward) to Ghostty keycodes/text, accumulates commands during interpretKeyEvents, and synthesizes matching Ghostty press/release events. ChangesText-Input Command Forwarding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 SummaryFixes Backspace input being mis-forwarded in cmux terminal sessions when remote-controlling via Astropad Workbench. Synthetic events from Astropad arrive through
Confidence Score: 5/5Safe to merge. The change is well-scoped: two new dispatch branches that only fire on events that would otherwise fall through to an incorrect or silent path, with both paths gated by the same modifier disjoint check. The previous reviewer concerns about a stale textInputCommandReleaseKeyCodes dictionary and a parallel release side-channel have both been fully resolved — the synthesised key-up is gone entirely, and the regression test confirms the later physical key-up is forwarded with its own original keycode. The two new production branches are mutually exclusive with the IME text path and the normal key path, modifier gating is consistent, and the withCString lifetime is safe. No actor isolation changes, no new blocking primitives, no sensitive data in logs. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[keyDown NSEvent] --> B[Build textInputEvent / translationEvent]
B --> C[Set up keyTextAccumulator / keyCommandAccumulator]
C --> D[interpretKeyEvents]
D --> E{IME changed keyboard layout?}
E -- yes --> F[imeConsumedKeyUps.insert / return early]
E -- no --> G[syncPreedit]
G --> H[Compute accumulatedText / accumulatedCommands / forwardableCommands]
H --> I{shouldSuppressGhosttyKeyForwarding?}
I -- yes --> J[imeConsumedKeyUps.insert / return]
I -- no --> K{accumulatedText non-empty?}
K -- yes --> L[IME text path / forward each text]
K -- no --> M{forwardableCommands non-empty?}
M -- yes --> N[deleteBackward: path / keycode=kVK_Delete / text=0x7F]
M -- no --> O{emptyKeyCodeZeroFallback?}
O -- yes --> P[Fallback Backspace / keyCode==0 + empty chars + no terminal mods]
O -- no --> Q[Normal key path / textForKeyEvent / sendGhosttyKey]
style N fill:#d4edda,stroke:#28a745
style P fill:#d4edda,stroke:#28a745
style F fill:#f8d7da,stroke:#dc3545
style J fill:#f8d7da,stroke:#dc3545
Reviews (4): Last reviewed commit: "fix(terminal): trust deleteBackward over..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| keyEvent.consumed_mods = GHOSTTY_MODS_NONE | ||
| keyEvent.composing = false | ||
| keyEvent.unshifted_codepoint = command.unshiftedCodepoint | ||
| textInputCommandReleaseKeyCodes[event.keyCode] = command.keycode |
There was a problem hiding this comment.
event.keyCode is unreliable
The PR description explicitly calls out that Astropad delivers events with "empty or unreliable raw NSEvent fields" — including keyCode. If Astropad's keyDown arrives with keyCode = 0 (the physical 'a' key) and no matching keyUp follows (or the keyUp arrives with a different keyCode), textInputCommandReleaseKeyCodes[0] = kVK_Delete stays in the dictionary. The next physical 'a' key-up then hits removeValue(forKey: 0), fires a spurious kVK_Delete RELEASE to Ghostty, and returns early — silently dropping the real 'a' key-up. Because the fix is specifically motivated by keyCodes being unreliable, using event.keyCode as the release-tracking key creates a concrete correctness gap in the exact scenario being fixed.
There was a problem hiding this comment.
Fixed in fc6dcfc0 and covered by 449c32e1. I removed textInputCommandReleaseKeyCodes entirely, so the synthetic deleteBackward: command no longer stores any release identity under the unreliable event.keyCode. The updated regression now sends a later physical a key-up with keyCode == 0 and expects that release to stay keyCode 0, not kVK_Delete.
| } else if !accumulatedCommands.isEmpty { | ||
| for command in accumulatedCommands { | ||
| keyEvent.keycode = command.keycode | ||
| keyEvent.consumed_mods = GHOSTTY_MODS_NONE | ||
| keyEvent.composing = false | ||
| keyEvent.unshifted_codepoint = command.unshiftedCodepoint | ||
| textInputCommandReleaseKeyCodes[event.keyCode] = command.keycode | ||
|
|
||
| #if DEBUG | ||
| let ghosttySendStart = ProcessInfo.processInfo.systemUptime | ||
| _ = sendTextInputCommand( | ||
| command, | ||
| surface: surface, | ||
| keyEvent: keyEvent, | ||
| path: "terminal.keyDown.commandGhosttySend", | ||
| event: event | ||
| ) | ||
| ghosttySendMs += (ProcessInfo.processInfo.systemUptime - ghosttySendStart) * 1000.0 | ||
| #else | ||
| _ = sendTextInputCommand(command, surface: surface, keyEvent: keyEvent) | ||
| #endif | ||
| } |
There was a problem hiding this comment.
textInputCommandReleaseKeyCodes is a third parallel dictionary/set tracking key-release identity, alongside imeConsumedKeyUps and keyboardCopyModeConsumedKeyUps. The root issue is that doCommand(by:) carries no reliable key identity from the synthetic NSEvent, and the fix routes around that by accumulating commands and wiring release tracking through the original unreliable event.keyCode. That leaves the invariant "a keyUp event for keyCode X always closes out exactly the state opened by keyDown for keyCode X" unenforceable for synthetic input. It may be worth considering whether the release event should be synthesised immediately at the end of keyDown (avoiding the cross-event state altogether) or whether the existing imeConsumedKeyUps pattern can be reused to suppress the normal release path without registering a replacement.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Fixed by removing the parallel release side channel rather than adding another release-suppression path. The text-input command path now behaves like committed text input: it forwards the immediate canonical Backspace press/text, but it does not claim ownership of a later keyUp whose physical identity is unavailable from doCommand(by:). Any later key-up is translated from its own AppKit event data.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 10228-10234: The current loop in GhosttyTerminalView that rewrites
accumulatedCommands into kVK_Delete + 0x7f and clears consumed_mods
indiscriminately breaks AppKit's Ctrl-H (deleteBackward:) handling; change the
logic so you only perform this synthetic translation for the explicit
"empty-text backspace" case (i.e., the synthetic deleteBackward path), and leave
all other accumulatedCommands (including real physical keys like Ctrl-H)
untouched so their original keycode and consumed_mods propagate. Concretely,
inside the loop over accumulatedCommands (the block that sets keyEvent.keycode,
keyEvent.consumed_mods, keyEvent.composing, keyEvent.unshifted_codepoint and
writes textInputCommandReleaseKeyCodes), gate the rewrite behind a check that
the command is the synthetic empty-text backspace (for example by testing the
command kind/name == deleteBackward: or an indicator such as
command.unshiftedCodepoint == 0 / synthetic flag), and otherwise skip modifying
consumed_mods/keycode so Ctrl-H retains its control scalar path (keeping
compatibility with GhosttyTextInputSupport).
🪄 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
Run ID: 66b28ab5-c3b8-4f11-b505-1df5275524b8
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
|
@coderabbitai Follow-up on the pre-merge table:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✓ ✏️ Learnings added
🧠 Learnings used |
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 `@Sources/GhosttyTerminalView.swift`:
- Around line 10567-10568: The check that computes hasTerminalModifier using
flags.intersection([.control, .option, .command]).isEmpty should be rewritten to
use isDisjoint(with:) for clarity: replace the current expression that uses
intersection(...).isEmpty with the idiomatic !flags.isDisjoint(with: [.control,
.option, .command]) (using the same flags variable derived from
event.modifierFlags.intersection(.deviceIndependentFlagsMask)) so the intent is
clearer in the hasTerminalModifier computation.
🪄 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
Run ID: 141e7388-41e8-40c8-9419-06c7a931b73f
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/TerminalAndGhosttyTests.swift
|
Follow-up from local Astropad validation: the original I added a second red/green pair:
This keeps the earlier review constraints intact: physical |
|
Another local Astropad validation pass showed the previous review-driven empty-text gate was still too narrow: this path can report Added another red/green pair:
The earlier safeguards remain: Ctrl/Option/Command modified keys are still excluded, physical |
1 participant
Summary
deleteBackward:text-input commands into canonical Ghostty Backspace events.Problem
When remote-controlling my Mac from an iPhone with Astropad Workbench, tapping the on-screen Backspace key inside a cmux terminal did not delete text. In my Codex/tmux session, it appeared as bad input and could type
ainstead.The same Backspace key works correctly outside this path:
0x7f.0x7f.0x08.That narrows the issue to cmux's embedded Ghostty/AppKit input forwarding path, not tmux, stty, or Ghostty configuration.
Fix
Synthetic input can arrive as
deleteBackward:throughinterpretKeyEventswith empty or unreliable rawNSEventfields. cmux previously discardeddoCommand(by:)commands to suppress system beeps.This change captures
deleteBackward:only while processinginterpretKeyEventsand forwards it to Ghostty as canonical Backspace/DEL: text0x7fwithkVK_Delete.Testing
git diff --check origin/main..HEADNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes Backspace not working in cmux terminals with Astropad Workbench by forwarding AppKit
deleteBackward:and keyCode-0 “empty” events as canonical Backspace (DEL), even when raw fields look like theAkey. Also prevents stale key-up mapping and keeps modified key combos on their original path.Bug Fixes
deleteBackward:duringinterpretKeyEventsas Backspace/DEL (0x7F,kVK_Delete), ignoring inconsistent raw fields (e.g.,characters: "a",keyCode: kVK_ANSI_A); DEBUG tests validate canonical press text.keyCode == 0) that don’t emitdeleteBackward:by sending DEL; gated to plain keys only (no Control/Option/Command), with tests confirming the fallback and that modified commands are untouched.kVK_DeletekeyUp; the later physical keyUp forwards with its original keycode, with DEBUG tests covering both cases.Refactors
Written for commit ce31f5b. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests