fix(terminal): clear the SGR pen on hidden-output restore and abandon - #14241
fix(terminal): clear the SGR pen on hidden-output restore and abandon#14241brennanb2025 wants to merge 5 commits into
Conversation
The hidden-delivery gate drops renderer-bound PTY bytes while a pane has no visible view. The renderer's xterm is a separate emulator from the daemon model, so when the dropped span contains the sequence closing an attribute run (e.g. the ESC[22m ending a bold run) the renderer's pen stays latched while the daemon model stays correct. Neither recovery path cleared it: - buildMainModelSnapshotReplayWrites reset the pen on the two alt-screen branches but not on the normal-buffer branch, and replayed scrollbackAnsi ahead of the reset it did emit, so replayed content inherited the stale pen. - abandonHiddenOutputRestoreAndDrainPendingForeground declares the dropped bytes unrecoverable (it writes a user-visible warning) and then drained the queued foreground chunks straight into xterm under that same unknown pen. Add RESET_GRAPHIC_RENDITION and emit it ahead of replayed content in every branch, and on both abandon exits. The existing profiles all clear DEC mode bits and none touched SGR.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
A gap can strand more than the pen: a dropped `ESC(B` leaves line-drawing selected and ordinary text renders as box characters. Route both recovery paths through one RESET_AFTER_BYTE_GAP profile covering SGR + charset. Deliberately not a soft reset (DECSTR): xterm's DECSTR wipes kitty flags and stacks (terminal-kitty-keyboard-mode-tracker applySoftReset), which would silence Option chords for a live agent that negotiates them only at startup. Reset what a gap strands and no running TUI re-asserts on its own; leave the rest to its next repaint.
The restore-needed marker is the single point where "renderer-bound bytes were dropped" is known. The handler already resets the transport's cross-chunk parser state there for exactly this reason — a partial escape spanning the gap would corrupt the next chunk. The emulator carries state across chunks in the same way, so reset it in the same place. That makes restore, abandon and overflow all start from a known pen by construction, instead of each recovery path having to remember.
![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)
src/renderer/src/components/terminal-pane/pty-connection.ts (1)
7063-7078: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset before writing the recovery warning.
Line 7068 queues
HIDDEN_OUTPUT_RESTORE_UNAVAILABLE_WARNINGbefore Line 7078 queuesRESET_AFTER_BYTE_GAP. If dropped bytes left SGR or a line-drawing charset active, xterm can render the warning with that stale state. Write the reset before the warning.Proposed fix
+ writePtyOutputToXterm(RESET_AFTER_BYTE_GAP, true) if (!opts.quiet && !rearmedRemoteRestore) { clearHiddenOutputRestoreFloodRepaintTimer() writeRestoreUnavailableWarning() } - writePtyOutputToXterm(RESET_AFTER_BYTE_GAP, true)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 354dfada-ff11-4e5e-8d0b-16da04ea28a7
📒 Files selected for processing (5)
src/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-snapshot-replay-paint.test.tssrc/renderer/src/components/terminal-pane/terminal-snapshot-replay-paint.tssrc/shared/terminal-mode-reset-profiles.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/components/terminal-pane/pty-connection.test.ts
- src/renderer/src/components/terminal-pane/terminal-snapshot-replay-paint.ts
- src/renderer/src/components/terminal-pane/terminal-snapshot-replay-paint.test.ts
Rendered Electron QA @
|
| marker | cells | bold | hyperlink | glyphs |
|---|---|---|---|---|
LATCHED (history) |
7 | 7 | 7 | LATCHED |
PLAINOK (after restore) |
7 | 0 | 0 | PLAINOK |
qPLAINQ (after restore) |
7 | 0 | 0 | qPLAINQ |
Main model snapshot: source headless, seq 9858, contains both LATCHED and PLAINOK. Renderer console: 0 errors (3 unrelated warnings). App log: no restore-path errors.
Gaps. Overflow-abandon and deadline-abandon arms were not forced; this run used the natural hide → drop → reveal path. Alt-screen TUI restore and remote re-arm were not exercised. Hyperlink absence on PLAINOK is from cell attributes, not a hover tooltip.
Verdict: land for the natural hidden-output restore path. Unmarked text after reveal does not inherit the pre-drop bold, hyperlink, or line-drawing latch.
1 participant
ELI5
When you switch away from a terminal, Orca stops sending it output to save work. If it stops mid-way through a piece of styled text — after "start bold" but before "stop bold" — the terminal is left holding the bold pen. When you come back, neither recovery path put the pen down, so everything painted afterwards came out bold. This puts the pen down.
What Changed
Added a
RESET_AFTER_BYTE_GAPprofile (cancel incomplete escape + close OSC 8 + SGR reset + all charset registers back to ASCII) and emitted it on the two paths that recover from dropped bytes:buildMainModelSnapshotReplayWrites— the two alt-screen branches already reset the pen; the normal-buffer branch did not, and the scrollback branch replayedscrollbackAnsiahead of the\x1b[0mit did emit. Both now clear the pen before any replayed content.abandonHiddenOutputRestoreAndDrainPendingForeground— this path declares the dropped bytes unrecoverable (it writes a user-visible "recovery was unavailable" warning) and then drained the queued foreground chunks into xterm under that same unknown pen. Now clears the full byte-gap state before the warning, drained foreground data, and both exits, including pending-overflow; quiet and remote-rearm paths reset without showing a warning.Every existing profile in
terminal-mode-reset-profiles.tsclears DEC mode bits (cursor style, kitty keyboard, mouse reporting, bracketed paste). None touched SGR or charset — neither was ever in scope for a recovery path.The complete profile is included because a gap strands more than the pen: a dropped partial escape, OSC 8 terminator, or charset designation can leave the parser, hyperlink, or character-set state latched. It restores every ISO 2022 register to ASCII and selects G0. It is deliberately not a soft reset (DECSTR) — xterm's DECSTR wipes kitty flags and stacks (see
terminal-kitty-keyboard-mode-trackerapplySoftReset), which would silence Option chords for a live agent that negotiates them only at startup. So: reset what a gap strands and no running TUI re-asserts on its own, and leave the rest to its next repaint.Why
src/main/ipc/pty-hidden-delivery-gate.tsdrops renderer-bound PTY bytes while a pane has no visible view. The renderer's xterm is a separate emulator from the daemon model, which is why a pane can render wrong while the daemon's serialized buffer reads clean — I verified that against a real occurrence (checkpoint.json: 6.1% bold, all runs terminated) and against the raw PTY log (246 bold runs, zero unterminated).If the dropped span contains the sequence closing an attribute run, the renderer's pen stays latched. A recovery path that knows bytes were lost cannot assume the terminal's attribute state is still coherent.
Linked Issue
Related: STA-4042 / STA-4060 (bold and regular text becoming visually identical in terminal panes).
Deliberately not marked as fixing them. The layer is now confirmed against a live occurrence (below), but I have not proven that this specific abandon/replay path is the one users hit.
Visual Proof
N/A for a static screenshot — the defect is a transient render state. Behavioural before/after is the A/B below, measured from the renderer's buffer rather than pixels.
Reliability Contract
terminal-output.scrollback-restore— after a renderer byte gap, restore, abandon, overflow, warning, retry, and remote re-arm paths establish a known parser/hyperlink/SGR/charset state before replayed or foreground text.Testing
Live Electron A/B, isolated profile, 50 000-line scrollback, probing
cell.isBold()on plain text written with no SGR codes at all after a hide → drop → reveal cycle:deep-natural(plain reveal, no forced hold)hold-overflow(overflow abandon after held snapshot)hold-deadlineEach fix closes a different arm: the replay fix closes
deep-natural, the abandon fix closeshold-overflow. Neither alone closes both.Automated: 594 tests pass across
pty-connection.test.tsandterminal-snapshot-replay-paint.test.ts, including abandon/warning/remote-rearm ordering, replay-branch invariants, and headless xterm cell-state coverage. The new abandon test was verified non-vacuous — removing the write makes it fail. Several existing tests pinned the exact replay write arrays and were updated; the reattach/SSH replay assertions were deliberately left alone since that clear comes from a different code path.pnpm typecheck,oxlint,oxfmt --checkall clean.Field capture, live occurrence. A pane was caught mid-symptom in a fresh session. Its raw PTY log — the daemon's record of the full byte stream — was clean: 9 SGR bold-on events, 31 resets, longest bold run 13 chars, zero unterminated, and none of the bolded spans corresponded to the affected text. The affected text was the user's own typed input line, showing bold on sub-word fragments (
shoud+l,spac+ed,al+w+ays) — the signature of per-cell corruption applied as the TUI incrementally repaints. So the byte stream says that text is not bold and the screen rendered it bold: the divergence is renderer-side, on a real occurrence, which is the layer this PR addresses. It then healed on a hide/reveal — the restore path succeeding, which is the same path hardened here.What I did not verify: that this is the cause of the field reports. The repro arms above are normal-buffer shells; the field reports are on alt-screen TUIs (Claude Code), whose branch already emitted a reset — for those, only the abandon path and the scrollback-ordering fix apply. I never observed
abandonWarning=trueon a natural (unforced) snapshot, so the field attribution remains unproven. Treat this as a real correctness fix on the recovery paths, not a confirmed closure of STA-4042.Review
Ensure no issues in: Security, Cross-platform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
\x1b[0mprepended to writes that already cleared the screen in the same chunk. A live TUI re-arms its own attributes on its next write; a shell inherits a defined pen instead of an undefined one.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)