fix: preserve pre-TUI SSH shell output across alternate-screen tab restore (#6106)#6572
fix: preserve pre-TUI SSH shell output across alternate-screen tab restore (#6106)#6572nwparker wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
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 |
EvidenceNode-level reproduction of the exact bug path (before/after)A faithful Electron + SSH-localhost repro was infeasible in this environment (sshd unreachable on This is issue #6106 exactly: the default (pre-fix / mobile) path drops Unit tests (regression coverage)New/updated assertions:
Lint / typecheck
Manual SSH/Electron noteThe verification plan's |
7134f39 to
f479019
Compare
|
Bugbash update pushed as f479019. What changed:
Validation:
|
…store When a desktop SSH terminal tab is hidden and restored while an alternate-screen TUI (Codex) is active, the hidden-restore snapshot discarded all normal-buffer shell output that preceded the TUI. Two behaviors compounded: serializeHeadlessTerminalBuffer forced scrollbackRows=0 in alternate screen, and HeadlessEmulator.getSnapshot sliced off everything before the last ?1049h marker. Both are correct for mobile replay, but wrong for desktop live SSH snapshots. Add a desktop-only opt-in (altScreenPreservesScrollback / preserveNormalBufferOnAltScreen) that keeps pre-TUI scrollback and returns the raw serializer output. Desktop requested snapshots and desktop first-subscribe snapshots for active alternate-screen terminals use this path, so first attach and tab restore both preserve the earlier shell output. Mobile replay remains unchanged, and ordinary desktop subscribe snapshots still use the existing zero-row path unless an alternate-screen TUI is active. Because SerializeAddon emits mouse TRACKING modes but not SGR mouse ENCODING (?1006h/?1016h), the opt-in path also re-appends the dropped encoding so TUI clicks past col/row 223 keep using SGR instead of degrading to legacy X10. Fixes #6106 Co-authored-by: Rod Boev <rod.boev@gmail.com> Co-authored-by: Orca <help@stably.ai>
f479019 to
83a73f2
Compare
|
Follow-up for failed hosted verify pushed as 83a73f2. Root cause from run 28412304316: full-suite RPC tests used partial OrcaRuntimeService stubs that did not define isTerminalAlternateScreen. The production subscribe path now calls that method for desktop first-attach preservation, so those stubs threw before emitting subscribed. I added default non-alternate-screen behavior to the three affected RPC test stub helpers; production scrollback behavior is unchanged. Local validation after the fix:
Watching the new hosted verify now. |
|
Hosted verify is green on 83a73f2 (run 28412924365): lint, typecheck, full test suite, unpacked build, and CLI smoke all passed. The only annotation is the pre-existing unrelated useGitStatusPolling.ts exhaustive-deps warning. |
|
Closing as superseded by #7214: current main headless snapshots preserve normal scrollback separately from alternate-screen state and tests assert preserved shell history is restored before the TUI preamble. Porting this older raw-serializer path would bypass current model/view, mobile, and query-authority contracts. |
🧑🤝🧑 Impact & ELI5
1) What's broken today (ELI5). If you use Orca over SSH (a remote machine), open a terminal, run some commands (like
ls), and then start a full-screen tool like Codex or Claude in that same terminal — switching to another tab and back makes all your earlier command output vanish. The remote shell history you printed before launching the tool is silently thrown away; when you come back, you only see the current Codex/Claude screen and your priorlsresults and prompts are gone. This hits anyone running a full-screen terminal app over SSH and switching tabs, which is a common workflow, so it's a real annoyance (lost context) rather than a crash.2) What this PR does (ELI5). Think of the terminal as a notebook. When a full-screen app takes over, it flips to a fresh clean page (a "scratch page") to draw on, but your earlier notes are still in the notebook. The old code, when saving the terminal to restore it later, would tear out everything except that one scratch page. This PR teaches the desktop restore path to keep the earlier pages too — so when you switch back, your pre-tool command history is still there. It also re-attaches a small "mouse precision" setting that the save process used to drop, so clicking in big full-screen apps keeps working accurately. Oh, so it now keeps your earlier shell output when you restore a tab instead of wiping it the moment a full-screen app was running.
3) Tradeoffs / regressions — honest answer. No regressions for existing desktop or mobile behavior. The fix is a narrowly gated desktop opt-in: the new "keep the history" mode turns on for desktop tab restore, and now also for first desktop attach when the PTY is already inside an alternate-screen TUI. Ordinary desktop subscribe snapshots stay on the existing zero-row path unless a full-screen app is active, and mobile replay is still explicitly opted out because its xterm re-wrap path would duplicate prompts / flatten SGR. The remaining cost is inherent to the fix: desktop snapshots for active TUIs can carry the user's earlier shell history across SSH instead of only the current TUI frame. That transfer is bounded by the existing snapshot byte budget and row fallback, so it is not unbounded.
Summary
Fixes #6106.
When a desktop SSH terminal tab is hidden and then restored while an alternate-screen TUI (Codex/Claude/vim) is active, the restore snapshot dropped all the normal-buffer shell output that preceded the TUI — the user's pre-Codex
lsoutput and prompts vanished and only the current TUI frame survived.Two compounding daemon behaviors caused this on the hidden-restore path (
SnapshotRequest→serializeBudgetedRequestedSnapshot→serializeTerminalBuffer→serializeHeadlessTerminalBuffer→HeadlessEmulator.getSnapshot):serializeHeadlessTerminalBufferforcedscrollbackRows=0whenever the emulator was in alternate screen, so normal-buffer scrollback was never serialized.HeadlessEmulator.getSnapshotrannormalizeSnapshotAnsiForModes, which (for alt-screen) slices off everything before the last\x1b[?1049h, deliberately discarding the normal-buffer content SerializeAddon emits before the alt buffer.Both behaviors are correct for the mobile replay path (mobile xterm re-wraps and would duplicate prompts / flatten SGR), but wrong for the desktop live SSH restore path.
Fix
Add a desktop-only opt-in (
altScreenPreservesScrollbackthreaded through the RPC/runtime,preserveNormalBufferOnAltScreenat the emulator) that keeps the pre-TUI scrollback and returns the raw serializer output for desktop snapshots. The flag is used for desktop restore/requested snapshots, and for first desktop attach when the PTY is already in alternate screen. Mobile remains opted out, and ordinary desktop initial subscribe snapshots stay on the existing zero-row path unless an alternate-screen TUI is active.Because the opt-in path returns the raw
serializer.serialize()output, andSerializeAddon._serializeModesemits mouse tracking modes (?1000h/?1002h/?1003h) and bracketed paste but not SGR mouse encoding (?1006h/?1016h), the opt-in path now also re-appends the dropped encoding mode. Without this, a desktop TUI using SGR mouse encoding (common: Codex/Claude/OpenTUI) would lose?1006hon restore and degrade to legacy X10 mouse reports (breaking clicks past col/row 223). The raw blob already enters alt screen via its embedded?1049h, so only the encoding mode is appended — no duplicate alt-screen entry.This supersedes community PR #6427 by @rodboev (its approach was adopted; credited via
Co-authored-by). On top of that PR it adds: the SGR mouse-encoding restoration + regression test, a clearerserializeBudgetedRequestedSnapshotoption-forwarding (no inverted default), and a WHY comment colocating the desktop-vs-mobile rationale. PR #6427 is left open.Screenshots
No visual screenshot from the Electron app: the bug lives entirely in the daemon-side terminal serialization, and a faithful end-to-end repro needs a running SSH-backed runtime (sshd was not reachable on
localhostin this environment and there was no packaged build). Instead, a Node-level reproduction exercising the exactHeadlessEmulator.getSnapshotpath is attached as a PR comment, showing the before/after, alongside unit tests that assert the regression directly.Testing
pnpm lintpnpm typecheckpnpm test(the three touched files:headless-emulator.test.ts,orca-runtime.test.ts,terminal-multiplex.test.ts)pnpm build(not run — no source other than these modules changed; CI covers build)Tests added:
headless-emulator.test.ts: opt-in preserves pre-alt-screen content; opt-in preserves SGR?1006hencoding when active without duplicating?1049h.orca-runtime.test.ts:serializeTerminalBufferdrops pre-TUI output by default but preserves it withaltScreenPreservesScrollback: true(assertssource: 'headless').terminal-multiplex.test.ts: desktopSnapshotRequestforwardsaltScreenPreservesScrollback: true; mobileSnapshotRequestdoes NOT forward the flag; desktop multiplex subscribe and legacy binary subscribe preserve pre-TUI scrollback on first attach when alternate screen is active.AI Review Report
Reviewed for correctness, edge cases, cross-platform behavior, and elegance.
modes.alternateScreen, so normal-buffer behavior is untouched. Desktop requested snapshots and desktop first-attach snapshots use the opt-in only while an alternate-screen TUI is active; mobile and normal desktop initial subscribe keep the existing normalized/zero-row behavior. Verified no otherserializeTerminalBuffercaller (serializeMainTerminalBuffer,serializeHiddenOutputRecoveryBuffer) sets the new flag, so their behavior is unchanged.buildMouseEncodingSequencereused by bothbuildRehydrateSequencesand the opt-in path (removes duplication, keeps the file under themax-lineslimit without any disable). Replaced PR Preserve SSH pre-Codex scrollback on tab restore #6427's inverted-defaultaltScreenPreservesScrollback === false ? ... : ...with a straightforward spread that forwards the caller's intent.Security Audit
Treated the original community-PR author as potentially malicious and re-audited the adopted code.
eval,child_process, filesystem writes, secret access, or new dependencies.?1006h/?1016h).Notes
normalizeSnapshotAnsiForModesdefault behavior are deliberately unchanged because the mobile replay path re-wraps snapshots and would duplicate prompts / flatten SGR if it used the desktop raw snapshot.Made with Orca 🐋