Repository navigation
Fix Windows ConPTY overlay after cold restore - #7310
Conversation
Fresh Windows ConPTY shells paint prompts at viewport coordinates, so restored rows left visible can be overwritten with stale tails. Mark panes that replay persisted scrollback and, only when a fresh shell wins or daemon cold-restore spawns, replay newline scrolling through the guard to move those rows into scrollback without ESC[3J.
📝 WalkthroughWalkthroughThis PR introduces a fresh-shell viewport blanking mechanism for terminal panes during cold restore. A new module provides an ANSI escape-sequence builder and a ref type for tracking panes pending viewport blanking. Scrollback restoration now marks restored panes in that ref. PTY connection logic consumes the marker during fresh spawns to conditionally blank the viewport, replaces a prior unconditional clear, and forces blanking on reattach/reconnect failure paths. Lifecycle wiring creates, passes, and cleans up the tracking ref. Tests cover the helper, restoration flow, and PTY rendering behavior. Sequence Diagram(s)Included in the hidden review stack artifact above. Related PRs: None identified. Suggested labels: terminal, enhancement Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/pty-connection.test.ts (1)
5278-5285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify why the synthetic render is disconnected from
written.This
renderHeadlessTerminalStatecall uses a hand-crafted write sequence ('old TUI row...',blankViewport,'PS >') instead of the actualwrittencaptured fromconnectPanePtyabove — the mockedpane.terminal.writenever produced "old TUI row" content, so this block only demonstrates that the blanking sequence generically preserves prior xterm content, not that this specific scenario preserved real restored rows. A short comment explaining that intent would prevent future readers from assuming this asserts an end-to-end integration with thewrittenoutput from lines above.As per coding guidelines,
**/*.{ts,tsx,js,jsx}changes driven by a non-obvious constraint should carry "a comment explaining why the code behaves the way it does."📝 Suggested clarifying comment
+ // Why: pane.terminal.write is mocked above, so it never renders real + // scrollback content — this synthetic render only proves the blanking + // sequence generically preserves prior rows in scrollback while + // clearing the viewport, independent of the `written` capture above. const rendered = await renderHeadlessTerminalState( ['old TUI row with a long tail\r\nold TUI row two', blankViewport, 'PS >'], 20, 4 )Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2afb0551-16d2-4e3d-9590-2d7cbf9bd488
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/layout-serialization.test.tssrc/renderer/src/components/terminal-pane/layout-serialization.tssrc/renderer/src/components/terminal-pane/pty-connection-types.tssrc/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/terminal-restored-viewport.tssrc/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts
| const consumeRestoredViewportBlankingMarker = (): boolean => { | ||
| return deps.restoredViewportBlankingPanesRef?.current.delete(pane.id) ?? false | ||
| } | ||
|
|
||
| const writeFreshShellViewportBlanking = (): void => { | ||
| writeReplayData(buildFreshShellViewportBlankingSequence(pane.terminal.rows)) | ||
| } | ||
|
|
||
| const prepareFreshShellViewportForSpawn = (options: FreshSpawnOptions): void => { | ||
| const hadRestoredViewport = consumeRestoredViewportBlankingMarker() | ||
| if (!options.forceBlankRestoredViewport && !hadRestoredViewport) { | ||
| return | ||
| } | ||
| // Why: fresh Windows ConPTY output paints at screen coordinates, so | ||
| // restored rows must leave the viewport before the first prompt redraw. | ||
| writeFreshShellViewportBlanking() | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf 'Files:\n'
git ls-files 'src/renderer/src/components/terminal-pane/pty-connection.ts'
printf '\nUsages:\n'
rg -n "consumeRestoredViewportBlankingMarker|restoredViewportBlankingPanesRef|prepareFreshShellViewportForSpawn|handleReattachResult|wakeHibernatedAgentPane|startFreshColdRestoreAgentResume|transport\.attach\(" src/renderer/src/components/terminal-pane/pty-connection.ts
printf '\nOutline around relevant file:\n'
ast-grep outline src/renderer/src/components/terminal-pane/pty-connection.ts --view expandedRepository: stablyai/orca
Length of output: 9250
🏁 Script executed:
set -euo pipefail
sed -n '5152,5295p' src/renderer/src/components/terminal-pane/pty-connection.ts
printf '\n---\n'
sed -n '5750,5855p' src/renderer/src/components/terminal-pane/pty-connection.ts
printf '\n---\n'
sed -n '3836,3858p' src/renderer/src/components/terminal-pane/pty-connection.tsRepository: stablyai/orca
Length of output: 12792
🏁 Script executed:
set -euo pipefail
rg -n "restoredViewportBlankingPanesRef\.current|restoredViewportBlankingPanesRef|viewportBlanking" src/renderer/src/components/terminal-pane/pty-connection.ts
printf '\nSet/Add sites nearby:\n'
rg -n -C 3 "add\(pane\.id\)|delete\(pane\.id\)|restoredViewportBlankingPanesRef" src/renderer/src/components/terminal-pane/pty-connection.tsRepository: stablyai/orca
Length of output: 1190
🏁 Script executed:
set -euo pipefail
rg -n "restoredViewportBlankingPanesRef|viewportBlankingPanesRef" .
printf '\nPotential add sites:\n'
rg -n -C 2 "restoredViewportBlankingPanesRef.*add|viewportBlankingPanesRef.*add|add\(pane\.id\)" .Repository: stablyai/orca
Length of output: 2426
🏁 Script executed:
set -euo pipefail
sed -n '232,270p' src/renderer/src/components/terminal-pane/layout-serialization.ts
printf '\n---\n'
sed -n '1195,1235p' src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts
printf '\n---\n'
sed -n '1390,1435p' src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.tsRepository: stablyai/orca
Length of output: 5877
Clear the restored-viewport marker on live reattach
handleReattachResult only consumes restoredViewportBlankingPanesRef in the cold-restore branch. Snapshot/replay and live transport.attach() success paths leave it set even though the marker is added during layout replay and only cleared on pane close, so a later fresh spawn can blank unrelated on-screen content.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal-pane/terminal-restored-viewport.test.ts (1)
19-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the clamping branch.
The helper under test clamps
rowsviaMath.max(1, Math.floor(Number.isFinite(rows) ? rows : 24)), but only the happy-path (rows: 5) is exercised here. Consider adding a case forNaN/0/negative/fractionalrowsto lock in the clamping behavior, since that logic path currently has no test coverage.✅ Suggested additional test
it('clamps invalid row counts to a safe minimum', () => { expect(buildFreshShellViewportBlankingSequence(0)).toContain('\x1b[1;1H') expect(buildFreshShellViewportBlankingSequence(NaN)).toContain('\x1b[24;1H') })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6754e404-f0ac-43f5-ab85-caa9ca7ab320
📒 Files selected for processing (2)
src/renderer/src/components/terminal-pane/terminal-restored-viewport.test.tssrc/renderer/src/components/terminal-pane/terminal-restored-viewport.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/components/terminal-pane/terminal-restored-viewport.ts
Summary
Verification