fix(desktop): unblock v1 terminal user input during shell init (#3478)#3550
Conversation
The v1 terminal host buffered all stdin writes — user keystrokes included — until the shell emitted OSC 133;A. When a user's `.zshrc` hook (e.g. fnm's `use-on-cd`) opened an interactive prompt during init, the marker never fired and typed y/N answers sat in the queue for the full 15s timeout, making the workspace look frozen. Pass writes straight through instead, keeping only the escape-sequence drop for stale DA/DSR replies from the renderer's xterm. Mirrors the v2 host-service behavior, which has always written user input directly.
📝 WalkthroughWalkthroughThe pull request modifies the terminal shell's write handling semantics, changing from buffering pre-ready writes until shell readiness to immediately passing non-escape-sequence writes through to the subprocess. Test assertions were updated to reflect this pass-through behavior, and the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/terminal-host/session.ts (1)
77-82:⚠️ Potential issue | 🟡 MinorStale doc: there are no "buffered writes" to flush on timeout anymore.
With this PR,
Session.write()no longer queues stdin duringpending— non-escape data passes through immediately. The only thing the timeout now does is flipshellReadyStateso that subsequent escape-sequence writes (e.g. arrow keys from the renderer) stop being dropped. Please update this comment so it doesn't mislead future readers into thinking there's still a pre-ready queue.📝 Proposed doc update
/** * How long to wait for the shell-ready marker before unblocking writes. - * 15s covers heavy setups like Nix-based devenv via direnv. On timeout, - * buffered writes flush immediately (same behavior as before this feature). + * 15s covers heavy setups like Nix-based devenv via direnv. On timeout, + * the session transitions to `timed_out` and escape-sequence writes + * (arrow keys, etc.) stop being filtered — non-escape writes already + * pass through immediately regardless of state. */ const SHELL_READY_TIMEOUT_MS = 15_000;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 77 - 82, Update the stale comment for SHELL_READY_TIMEOUT_MS to remove mention of flushing "buffered writes": explain that Session.write() now passes non-escape stdin through immediately during pending, and the timeout only flips shellReadyState so that subsequent escape-sequence writes (e.g., arrow keys) are no longer dropped; reference the symbols SHELL_READY_TIMEOUT_MS, Session.write(), and shellReadyState in the comment so future readers understand the current behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 77-82: Update the stale comment for SHELL_READY_TIMEOUT_MS to
remove mention of flushing "buffered writes": explain that Session.write() now
passes non-escape stdin through immediately during pending, and the timeout only
flips shellReadyState so that subsequent escape-sequence writes (e.g., arrow
keys) are no longer dropped; reference the symbols SHELL_READY_TIMEOUT_MS,
Session.write(), and shellReadyState in the comment so future readers understand
the current behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eda2d9aa-16a3-487c-a2c9-d44b659ad148
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/session-shell-ready.test.tsapps/desktop/src/main/terminal-host/session.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a terminal freeze in the v1 desktop terminal host ( Changes:
Confidence Score: 5/5Safe to merge — targeted fix with comprehensive regression tests and no regressions to existing behavior. The change is minimal (the write() method body is a few lines), directly matches the v2 host-service design, is backed by 37 passing tests including a named regression test for #3478, and the PR description clearly traces the root cause. The only note is that the ESC-prefix filter is slightly broader than just DA/DSR replies (it also silently drops arrow-key input during the ≤15 s init window), which is documented in comments and is an acceptable tradeoff. No files require special attention.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/terminal-host/session.ts | Core fix: write() now passes all non-escape-sequence data through during the pending shell-ready window instead of buffering everything; only \x1b-prefixed bytes are dropped to prevent DA/DSR replies from appearing as literal text. The change is minimal, well-commented, and consistent with the v2 terminal host design. |
| apps/desktop/src/main/terminal-host/session-shell-ready.test.ts | New regression/unit test file covering all key cases: write pass-through during pending state (the #3478 fix), escape-sequence filtering, marker detection (including split-frame and combined-marker cases), kill/exit before readiness, and per-shell coverage for supported and unsupported shells. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Session.write(data) called"] --> B{subprocess ready?}
B -- No --> C["throw Error: PTY not spawned"]
B -- Yes --> D{shellReadyState == pending?}
D -- No --> G["sendWriteToSubprocess(data)"]
D -- Yes --> E{data.startsWith ESC?}
E -- Yes --> F["DROP - stale DA/DSR reply from renderer xterm"]
E -- No --> G
G --> H["PTY receives data immediately"]
style F fill:#f88,color:#000
style C fill:#f88,color:#000
style H fill:#8f8,color:#000
Comments Outside Diff (1)
-
apps/desktop/src/main/terminal-host/session.ts, line 860-868 (link)Escape filter is broader than DA/DSR replies
The JSDoc states the dropped sequences are "stale DA/DSR replies from the renderer's xterm", but
data.startsWith("\x1b")silently discards all escape-prefixed bytes during thependingwindow — including legitimate user keystrokes such as arrow keys (\x1b[A), function keys (\x1bOP), and Home/End (\x1b[H).In practice this is unlikely to matter (the window is ≤ 15 s and users rarely press navigation keys during shell init), but it is worth noting that the v2 terminal host (
packages/host-service/src/terminal/terminal.ts:539) passes all input — including escape sequences — directly to the PTY without any filter, meaning the two implementations are not fully symmetric.Consider updating the JSDoc to be explicit that the filter is intentionally broader than just DA/DSR and covers all
\x1b-prefixed input during init:Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/terminal-host/session.ts Line: 860-868 Comment: **Escape filter is broader than DA/DSR replies** The JSDoc states the dropped sequences are *"stale DA/DSR replies from the renderer's xterm"*, but `data.startsWith("\x1b")` silently discards **all** escape-prefixed bytes during the `pending` window — including legitimate user keystrokes such as arrow keys (`\x1b[A`), function keys (`\x1bOP`), and Home/End (`\x1b[H`). In practice this is unlikely to matter (the window is ≤ 15 s and users rarely press navigation keys during shell init), but it is worth noting that the v2 terminal host (`packages/host-service/src/terminal/terminal.ts:539`) passes all input — including escape sequences — directly to the PTY without any filter, meaning the two implementations are not fully symmetric. Consider updating the JSDoc to be explicit that the filter is intentionally broader than just DA/DSR and covers all `\x1b`-prefixed input during init: How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/terminal-host/session.ts
Line: 860-868
Comment:
**Escape filter is broader than DA/DSR replies**
The JSDoc states the dropped sequences are *"stale DA/DSR replies from the renderer's xterm"*, but `data.startsWith("\x1b")` silently discards **all** escape-prefixed bytes during the `pending` window — including legitimate user keystrokes such as arrow keys (`\x1b[A`), function keys (`\x1bOP`), and Home/End (`\x1b[H`).
In practice this is unlikely to matter (the window is ≤ 15 s and users rarely press navigation keys during shell init), but it is worth noting that the v2 terminal host (`packages/host-service/src/terminal/terminal.ts:539`) passes all input — including escape sequences — directly to the PTY without any filter, meaning the two implementations are not fully symmetric.
Consider updating the JSDoc to be explicit that the filter is intentionally broader than just DA/DSR and covers all `\x1b`-prefixed input during init:
```suggestion
/**
* Write data to the PTY's stdin.
*
* Escape-sequence-prefixed data (`\x1b`-prefixed) is dropped while the
* shell is still initializing. In practice these bytes are stale DA/DSR
* replies from the renderer's xterm to terminal queries sent by the shell
* during startup; if forwarded they appear as typed text like
* `?62;4;9;22c` at the shell prompt. The headless emulator answers those
* queries directly (see constructor), so dropping the renderer's duplicate
* is safe. As a side-effect, escape-prefixed user keystrokes (arrow keys,
* function keys) are also dropped during this window — acceptable given
* the ≤ 15 s duration.
*
* All other data — user keystrokes and preset commands alike — passes
* through immediately. Buffering here previously froze workspaces when
* shell init commands (e.g. fnm's `use-on-cd` hook) opened an interactive
* prompt before the OSC 133;A marker fired. See #3478.
*/
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): unblock v1 terminal user i..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Summary
.nvmrc-pinned Node version was missing and the shell-initfnm use-on-cdhook asked"Do you want to install it? answer [y/N]:".Session.write()in the v1 terminal host queued all stdin writes (user keystrokes included) until OSC 133;A. The prompt blocks init, so the marker never fires and typedy/Nsits buffered for the 15s timeout.packages/host-service/src/terminal/terminal.ts), which has always written user input directly.Test plan
bun test apps/desktop/src/main/terminal-host/— 37 pass, including a new#3478regression test asserting writes reach the PTY whileshellReadyState === "pending".bun run typecheckclean.bun run lintclean.y/Nimmediately instead of freezing for 15s.Summary by cubic
Fixes #3478 by letting the v1 desktop terminal send user input to the PTY during shell init, so prompts like fnm’s “install missing Node version?” accept answers immediately. We now drop only stale escape-sequence replies and otherwise pass input through, matching v2 behavior.
shellReadyState === 'pending'; removed the pre-ready stdin queue and replay on marker/exit/timeout.\x1b-prefixed DA/DSR responses from the renderer during pending; after ready, forward all input (e.g., arrow keys).Written for commit 05e2d7b. Summary will update on new commits.
Summary by CodeRabbit