Repository navigation
fix(terminal): replay cold restores at recovered grid - #9043
Conversation
2b65ba4 to
877a3c2
Compare
|
Companion Windows restart-regression PR: #9064. #9043 contains the product correction; #9064 makes the PowerShell/replay-readiness harness deterministic and adds the targeted Windows gate. They are independently mergeable, but I recommend they go hand in hand. Merge #9064 first so the workflow exists on main, then synchronize or mark #9043 ready so the gate runs on its matching product paths. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 877a3c28c04b54037f983c8b25029b7fd95e2cbe and 614fafe. 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughCold-restore payloads now include optional terminal dimensions across daemon, provider, preload, IPC, and renderer types. Headless terminal seeding uses separate snapshot and cold-restore sizes while retaining compatibility with dimensionless legacy payloads. Renderer replay suppresses structural resize forwarding, clears the destination viewport using calculated row counts, restores recovered dimensions, and replays scrollback with expanded tests. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
Thanks @slashdevcorpse — reviewed this end to end and it's in good shape. What it does: when the daemon has lost a session and Orca cold-restores a pane from disk-recorded scrollback, that scrollback carries absolute cursor-positioning escapes that only make sense at the grid the session was recorded at. Replaying it at the current pane size garbled restored TUI content. This PR threads the recorded Why it holds up:
Verified locally:
The cold path itself is a Windows-restart scenario I couldn't reproduce deterministically on macOS, but it's covered by the real-xterm regression test and your same-PC Windows runs. Per the description, this should land alongside #9064 (merge that first). |
877a3c2 to
614fafe
Compare
Summary
cols/rowsacross the daemon provider, IPC/preload, renderer, and main headless terminal.CSI 2J+ home), await parsing before any recovered-grid shrink can promote clipped rows into scrollback, continue to forbidCSI 3J, blank enough source/immediate-destination rows, then use the existing structural replay barrier and authoritative fit.This is a cold-restore parity follow-up found while investigating #8751. Runtime evidence indicates the screenshot's immediate warm-reattach race is separately addressed by #8674; this PR does not claim that the cold path alone produced the screenshot or replace #8674.
Screenshots
No visual design change. The failure screenshot and investigation are in #8751.
Testing
pnpm lint— full repository lint not run; touchedoxlintandoxfmt --checkpassedpnpm typecheckpnpm test— full repository suite not run; affected 5-file matrix passed with 904 tests / 15 skippedpnpm build— full production/native build not run; the Electron E2E bundle built successfullyCommands run:
pnpm test -- src/main/daemon/daemon-pty-adapter.test.ts src/main/ipc/pty.test.ts src/renderer/src/components/terminal-pane/pty-connection.test.ts src/renderer/src/lib/pane-manager/terminal-structural-replay-coordinator.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.tspnpm typecheckpnpm exec electron-vite build --mode e2epnpm exec oxfmt --check ...pnpm exec oxlint ...pnpm run check:max-lines-ratchetgit diff --checkThe initial regression was red in three focused assertions: missing adapter dimensions,
undefinedruntime seed dimensions, and no renderer resize to the recovered grid. A strengthened dirty-grid regression then caught a blocker in the first implementation: shrinking from a dirty 10x5 destination to the recovered 20x3 grid before clearing promoted clipped stale rows into scrollback, beyond a later viewport-only clear. Its clear-before-resize ordering assertion failed before the correction and passes after the clear parse barrier. The final five-file matrix passed.Same-PC Windows Electron verification
The exact PR-head app bundle (
877a3c28c04b54037f983c8b25029b7fd95e2cbe) was exercised with disposable user data on the affected Windows PC:On the final amended bundle, both warm checks passed and all 3 repeated cold runs that reached the input assertion passed. One serial run began the delayed keyboard probe during the intentional replay guard: PowerShell received only the suffix
RED_OK, while direct PTY input already worked beforepty:listSessions. Independent trace and code reviews classified this as an E2E replay-readiness race, not a persistent transport or ownership failure. Two further repetitions ended earlier during app setup when navigation destroyed the Playwright execution context; neither reached the cold-input assertion.The committed E2E drivers assume POSIX
Ctrl+U/export PS1, and the cold case is normally skipped on Windows, so only the temporary test driver was adapted; the built application stayed at the exact PR head and all driver edits were restored afterward. The packaged installer/update harness was not run because it correctly refuses while an unrelated Orca app process is active.AI Review Report
Parallel investigation covered the issue/release history, local Windows runtime chronology, restore-path code tracing, test-first implementation, and independent adversarial diff, external-state, and Windows-runtime reviews.
Main risks checked:
CSI 2Jand tests thatCSI 3Jis absent.Cross-platform review explicitly covered macOS, Linux, Windows/ConPTY, SSH relay, and remote-runtime behavior. No shortcut, label, path, shell-command, Git-provider, or local-only assumption was added. SSH relay restore remains on its existing replay branch; optional cold metadata is transport-neutral.
Security Audit
No dependency, command execution, filesystem path, auth, secret, network, or new IPC-channel surface was added. The existing internal cold-restore result gains two numeric metadata fields sourced from recovered terminal history. Renderer use requires finite positive dimensions before resizing; legacy/missing values fall back safely. Replay remains under the existing query-response guard so recorded terminal queries cannot write replies into the fresh shell.
Notes
69c14fadce) merged after v1.4.143 and is absent from that release. Newly published v1.4.144-rc.0 contains fix(terminal): make viewport ownership transactional across output, replay, and reflow #8674, but not this cold-restore follow-up.track-community-prhas run and passed so far.