Repository navigation
fix(terminal): make viewport ownership transactional across output, replay, and reflow - #8674
Conversation
…ing snapshot restores Returning to a worktree whose agent kept streaming (Codex "Working…") replays a main-buffer snapshot: the buffer is cleared and rewritten, but xterm parses those bytes asynchronously. Scroll-intent capture/enforce that raced the parse — live stream batches, safeFit, visibility resume, and the restore's own synchronous enforcement — read the half-cleared buffer and re-latched the intent as "pinned at line 0", parking the returned pane at the very top of the scrollback (#8625 does not cover this). - Bracket the snapshot replay with a per-terminal rebuild marker (terminal-scroll-intent-rebuild.ts); while it is in flight, intent capture returns null and enforcement no-ops, so no racer can latch transient coordinates. - Re-apply the pre-replay intent once, in a FIFO-ordered empty-write completion after the replay bytes have parsed (bottom-offset restore), with a timeout fallback so a wedged pipeline cannot suppress intent forever. - Defense-in-depth for unbracketed rebuilds: enforcement no longer re-latches a pinned intent from a scrollback shorter than the stored pin's, and capture only normalizes pinned-at-bottom to follow when the scrollback is at least as long as the pin's. - Split the DOM wheel/pointer tracking into terminal-scroll-intent-dom-tracking.ts (max-lines). Deterministic e2e repro: a synchronized-output streaming fixture, pinned six rows above bottom, worktree switch away and back — settled at viewportY=0 (baseY=284) before this fix, restores near the pin after. Co-authored-by: Orca <help@stably.ai>
Delete the per-write-batch scroll-intent capture/enforce from the output scheduler. xterm core already owns follow-vs-pinned for live output — BufferService.isUserScrolling flips only on real viewport movement and is consumed atomically inside the append (scroll() snaps ydisp to ybase when following, holds it when pinned, and walks a pinned viewport down content-stably when scrollback trims). The app-side per-batch enforcement duplicated that decision non-atomically and was the race surface behind the pinned-at-bottom freeze and the streaming top-jump. App-side intent enforcement is now scoped to structural operations xterm cannot know about: snapshot replay (rebuild bracket + post-parse bottom-offset restore), remount/visibility resume, and fit reflow (marker restore). Intent classification (wheel/keyboard settle, phantom-pin normalization) is unchanged — stored intent feeds those structural restores. A pinned-version contract test (@xterm/headless shares BufferService with @xterm/xterm) locks the native semantics the write path now relies on, so an emulator upgrade that changes them fails CI instead of silently breaking follow behavior. Verified: full pane-manager/terminal-pane suites (2508), the three deterministic scroll e2e specs (follow-during-stream, static pinned worktree switch, streaming pinned worktree switch), TUI wheel report/drain and long-table scroll-restore e2e, scheduler throughput bench, typecheck. Co-authored-by: Orca <help@stably.ai>
With per-batch enforcement gone, typing (which xterm scrolls to the bottom via scrollOnUserInput) no longer had a write-path side effect that healed a stored pin — the stale pin would be re-applied by the next workspace-switch restore, yanking the user back to an old reading position. Subscribe to core's onUserInput (real input only, never parser auto-replies) and resync intent from the settled viewport; a pin whose viewport did not move (mouse reports, app-consumed keys) is preserved. Co-authored-by: Orca <help@stably.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes separate DOM-based scroll-intent tracking from intent state management, add rebuild-in-flight coordination, and guard capture and enforcement during snapshot buffer reconstruction. Terminal output scheduling now relies on xterm’s native viewport behavior instead of enforcing intent per write. Snapshot restoration defers intent enforcement until replay parsing completes, with a timeout fallback. Unit and end-to-end tests cover xterm scrolling, rebuild races, and viewport restoration during worktree switching. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/terminal-pinned-viewport-streaming-switch.spec.ts (1)
22-49: 🩺 Stability & Availability | 🔵 TrivialFixture spinner runs ~40s of real background PTY activity beyond what the assertions need.
The 800-tick × 50ms loop keeps the fixture streaming well past the test's actual observation window (pin → switch away 3s → switch back → settle). Cleanup only removes the script file (Line 203); the spawned Node process itself isn't explicitly terminated, so it likely keeps running in the PTY until the app/session teardown reaps it. Worth confirming the harness reliably kills PTY child processes on worktree/app teardown between e2e specs, and consider trimming tick count/duration to reduce the window where a stray process could interfere with subsequent tests in CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c582e23a-51a6-4eed-a02e-232abd146ea3
📒 Files selected for processing (9)
src/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/lib/pane-manager/pane-lifecycle.tssrc/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.tssrc/renderer/src/lib/pane-manager/terminal-scroll-intent-dom-tracking.tssrc/renderer/src/lib/pane-manager/terminal-scroll-intent-rebuild.tssrc/renderer/src/lib/pane-manager/terminal-scroll-intent.test.tssrc/renderer/src/lib/pane-manager/terminal-scroll-intent.tssrc/renderer/src/lib/pane-manager/xterm-user-scrolling-contract.test.tstests/e2e/terminal-pinned-viewport-streaming-switch.spec.ts
Mouse-tracking apps stream mouse reports as user input at pointer frequency, and xterm's scroll-on-user-input jumps a scrolled-up viewport to the bottom on each one (pre-existing native behavior). Resyncing intent from that jump would convert a reading pin into follow-output permanently — a mouse-move over a mouse-tracking normal-buffer app would drop the position a structural restore could otherwise bring back. Subscribe through the public onData instead of the private core user-input signal and skip mouse-report-shaped input (SGR and X10). Parser auto-replies that also flow through onData never move the viewport, so their resync settles as a no-op. Co-authored-by: Orca <help@stably.ai>
…-scroll-ownership # Conflicts: # src/renderer/src/lib/pane-manager/pane-terminal-output-scheduler.ts
…-scroll-ownership
…-scroll-ownership
Co-authored-by: Orca <help@stably.ai>
…-scroll-ownership
…-scroll-ownership # Conflicts: # src/renderer/src/components/terminal-pane/pty-connection.ts
…eplay, and reflow (stablyai#8674) Co-authored-by: Orca <help@stably.ai>
Supersedes #8667 and completes the viewport/replay work that remained incomplete after #8625.
What this fixes
This PR fixes a family of terminal/TUI failures that shared one ownership race:
The deterministic worktree-switch reproduction on
maincould settle atviewportY=0with a much largerbaseY, even though the user had pinned near the bottom.Root cause
Three systems were making overlapping viewport decisions:
BufferService.isUserScrolling;Snapshot bytes are parsed later by xterm. The old code could clear/rewrite the buffer, call
safeFit, forward dimensions/SIGWINCH, and restore scroll intent while xterm still exposed a half-parsed buffer. A live write, fit, or visibility callback could then record that transient state as a durable pin or send the snapshot grid to the live PTY.Ownership model
The resulting boundary is intentionally narrow:
pty-connection. Every delayed resize/replay continuation revalidates the exact PTY and stream generation before touching transport state.This is the mergeable nucleus of a future pane-scoped viewport controller without attempting a risky wholesale terminal rewrite in this PR.
Main changes
1. Native xterm ownership for live output
pane-terminal-output-scheduler.ts.xterm-user-scrolling-contract.test.ts, pinned to the installed xterm version, covering:An xterm upgrade that changes this contract will now fail CI instead of silently changing terminal behavior.
2. Transactional structural replay
Hidden snapshot restore and reattach now run through a pane-scoped structural replay coordinator:
Later replay operations cannot overtake the post-replay fit. A temporarily unmeasurable pane retains a keyed, awaitable fit continuation; any later successful normal fit completes it. Replacement, PTY/stream generation changes, and pane/binding disposal explicitly cancel it.
This prevents both failure modes:
3. Input-driven intent healing
onData-based intent resync so real typing heals a stale pin and resumes native follow behavior.4. Replay-aware fit, reflow, and lifecycle handling
Platform and transport behavior
Regression coverage
New and expanded tests cover:
Verification
Run after merging the latest
origin/maininto this branch:pnpm run typecheck— passed;pnpm exec electron-vite build --mode e2e— passed;git diff --check— passed;The real fullscreen Codex/setup-split diagnostic was also exercised with WebGL enabled, a 3.5-second post-ready wait, human-paced typing, and no click before evidence. xterm, proposed fit, and applied PTY size all measured
65x63; nativeisUserScrollingwasfalse; the remaining 11px pane remainder was below one cell and expected. Before/after-click screenshots were byte-identical, confirming focus was not masking a stale-grid artifact.Review notes
This became larger than the original three-area patch because adversarial review found the same invariant was bypassed by appearance, visibility, WebGL, PTY-size reassertion, remount, and reattach paths. Those paths now use the same replay-aware fit/continuation boundary instead of accumulating independent fixes.
The final architecture deliberately avoids making Orca a second live-output scroll engine. xterm owns the hot path; Orca coordinates only structural state changes.
Made with Orca 🐋