fix(app): restore terminal keyboard input to PTY - #732
Conversation
Ghostty's `customKeyEventHandler` uses `true` to BLOCK default handling — opposite of xterm.js. terminal.tsx returned `true` for every non-toggle keystroke, so Ghostty silently dropped the key before invoking `onData`, preventing any input from reaching the PTY (#696). Cursor and focus ring still rendered, which is exactly what the reporter described. Introduce a typed wrapper `@/utils/terminal-key-handler` that exposes a `"block" | "passthrough"` enum, so the boolean contract is impossible to invert. The wrapper compresses the cross-library quirk into one ten-line file: future authors get a compile error if they try `return true`, and missing returns are rejected by the union type. The same change rewrites the handler with intercept-first / passthrough-default ordering and fixes the toggle branch (which previously returned `false`, letting Ghostty leak a Ctrl+\` escape sequence to the PTY while the toggle command fired). The bug was present since the initial OpenCode import (`bde4c3927`), inherited from upstream PR #5081 where the boolean semantics were likely copy-pasted from xterm.js docs. Add an E2E regression spec that types via `runTerminal` and asserts the echoed token reaches the rendered buffer. Confirmed locally that reverting the wrapper return to `"block"` for the default branch makes the spec time out at the exact assertion the user-reported symptom maps to.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ 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 |
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/components/terminal.tsx, packages/app/src/utils/terminal-key-handler.ts)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
There was a problem hiding this comment.
Code Review
This pull request introduces a typed wrapper, attachKeyHandler, for Ghostty's custom key event handler to prevent boolean inversion bugs and adds an E2E regression test for terminal input. Feedback includes improving E2E test stability by avoiding .first() locators and optimizing performance by caching parsed keybind configurations instead of re-parsing them on every keystroke.
Perf delta summaryComparator: pass
|
642a9fb to
9583e8e
Compare
Summary
Rewires terminal.tsx's Ghostty key handler with the correct boolean contract so keystrokes reach the PTY again, and adds a typed wrapper that makes the inversion impossible to write.
Why
Ghostty's
customKeyEventHandlerreturnstrueto BLOCK default handling, opposite of xterm.js.packages/app/src/components/terminal.tsx:408returnedtruefor every non-toggle keystroke, so Ghostty silently dropped the key beforeonDatafired and nothing reached the PTY. Cursor and focus ring kept rendering, matching the reporter's "looks focused but no input" description. The bug was inherited from the initial OpenCode import (bde4c3927, upstream PR #5081) where the boolean semantics were almost certainly copy-pasted from xterm.js docs.The fix introduces
packages/app/src/utils/terminal-key-handler.ts, a ten-line wrapper that exposes a"block" | "passthrough"enum. Writingreturn trueis rejected by TypeScript, and missing returns are rejected by the union — the upstream-class bug becomes structurally impossible. The handler is rewritten with intercept-first / passthrough-default ordering, and the toggle branch is corrected to return block (previously returned the wrong value, letting Ghostty leak a Ctrl+` escape sequence to the PTY while the toggle command fired in parallel).Related Issue
Closes #696.
Human Review Status
Pending.
Review Focus
The wrapper is the structural defense; please scrutinise the contract in
terminal-key-handler.tsand confirm the rewritten handler interminal.tsxcovers the same three branches (Ctrl+Shift+C, toggle keybind, default passthrough) with the right enum value at each return point.Risk Notes
Renderer-only change; no IPC, preload, native menu, updater, packaging, or signing surfaces are touched. Behaviour parity between dev:web and Electron renderer holds, so manual
dev:desktopwas skipped intentionally in favour of E2E plus visual screenshots. The third branch fix (toggle returning block) removes a previously-leaked escape sequence to the PTY but this was never user-visible.How To Verify
Screenshots or Recordings
Checklist
bug,enhancement,task, ordocumentation), at least one primary routing label (app,ui,platform,harness, orci), and exactly one priority label (P0toP3), or I requested maintainer labelingdev, and my PR title and commit messages use Conventional Commits in English