From d7648518aa59362ec6fa00556d2e40d16543e099 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Fri, 17 Jul 2026 19:23:33 +0200 Subject: [PATCH] fix(terminal): keep the viewport pinned across resize/redraw reflows (#403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The residual intermittent divergence on relay 10.6.3 (codex resize-mid-stream) is NOT byte loss and NOT predictive echo: the rendered cells match the broker oracle exactly (differingCells=0; replaying the delivered bytes through a clean emulator equals the broker). The failure is the viewport being stranded in scrollback — at quiet it sits at [viewportY=0, baseY=100] with byte-correct content, so the fidelity quiet gate (viewportAtBottom) never closes. (The "[39,2] vs [40,3]" cursor signature is a 0-indexed xterm vs 1-indexed broker reporting artifact — the stream's last CUP is literally ESC[40;3H — not corruption.) Root cause: "am I following the tail?" was inferred from the INSTANTANEOUS viewportY === baseY. Two paths transiently scroll the viewport off the bottom: a TUI's SIGWINCH full-screen redraw (a server write, off-bottom DURING xterm's async parse) and a width/height reflow (fitAddon.fit -> term.resize). Once the viewport is off by even one line, the instantaneous check reads false and every later write AND fit stops re-pinning — the grid freezes in scrollback. Fix: track a sticky `followBottom` intent, flipped ONLY by a real user wheel scroll. Re-pin against that intent (a) in the echo-router's direct-route write COMPLETION callback, so a chunk that scrolls during its own parse is corrected after the parse, and (b) centrally in tryFit, so every reflow site (ResizeObserver, reconciler onPersistentDimsMismatch, init, refreshOnShow) keeps a following viewport at the tail. A deliberate wheel-up into scrollback clears the intent and is left alone (scrollback workload safe). No bytes are dropped and the reconciler confirm-twice / rate-limit / dims gates are untouched. Verified: 12/12 clean codex resize-mid-stream matrix runs (from a ~1-in-2 baseline); 71 focused renderer tests green including new regression tests that lock the post-parse write re-pin and the reflow/wheel intent behavior. Co-Authored-By: Claude Opus 4.8 --- src/renderer/src/lib/echo-router.test.ts | 69 +++++++++++++++++++ src/renderer/src/lib/echo-router.ts | 30 +++++++- .../lib/terminal-runtime-registry.dom.test.ts | 56 +++++++++++++++ .../src/lib/terminal-runtime-registry.ts | 38 +++++++++- 4 files changed, 190 insertions(+), 3 deletions(-) diff --git a/src/renderer/src/lib/echo-router.test.ts b/src/renderer/src/lib/echo-router.test.ts index 2f77a69f..d5152871 100644 --- a/src/renderer/src/lib/echo-router.test.ts +++ b/src/renderer/src/lib/echo-router.test.ts @@ -562,3 +562,72 @@ describe('echo-router — reseed capture timeout', () => { expect(written).toEqual(['held']) }) }) + +// pear#403: on the direct route the viewport re-pin must fire from the write +// COMPLETION callback (after xterm parses the chunk), not synchronously before +// it. term.write is async: a chunk can scroll the viewport off the bottom +// during its own parse (a TUI SIGWINCH full-screen redraw rewrites scrollback +// and resets ydisp). A synchronous scrollToBottom issued before that parse is +// undone by it; the viewport is then stranded off-bottom, isViewportPinned() +// reads false, and every later chunk stops re-pinning — the grid freezes in +// scrollback with byte-correct content (codex resize-mid-stream, viewport +// [0,baseY] at quiet). These lock the post-parse re-pin, gated on the +// pre-write follow intent so a deliberate scrollback read is never yanked down. +describe('echo-router — #403 direct-route viewport re-pin after parse', () => { + function makeRepinHarness() { + const writes: string[] = [] + let pendingCb: (() => void) | null = null + let pinned = true + const scrollToBottom = vi.fn() + const router = createEchoRouter({ + // Defer the completion callback so the test controls the parse boundary, + // exactly like xterm's async WriteBuffer. + write: (data, callback) => { + writes.push(data) + pendingCb = callback ?? null + }, + getEngine: () => null, + buildModelSeed: () => '\x1bc', + getInputSrtt: () => null, + isViewportPinned: () => pinned, + scrollToBottom + }) + return { + router, + writes, + scrollToBottom, + setPinned: (value: boolean) => { + pinned = value + }, + completeParse: () => { + const cb = pendingCb + pendingCb = null + cb?.() + } + } + } + + it('re-pins only AFTER the chunk is parsed, never synchronously before it', async () => { + const h = makeRepinHarness() + // A full-screen redraw: the kind of chunk whose parse moves the viewport. + h.router.onServerOutput('\x1b[2J\x1b[Hfull redraw at new size') + // Write issued, parse not yet complete → a synchronous (pre-parse) re-pin + // would already have fired here. It must not. + expect(h.writes).toHaveLength(1) + expect(h.scrollToBottom).not.toHaveBeenCalled() + // Parse completes (viewport may now be off-bottom); the completion callback + // re-pins, so the stranding cascade never starts. + h.completeParse() + expect(h.scrollToBottom).toHaveBeenCalledTimes(1) + await h.router.dispose() + }) + + it('does not re-pin when the user has scrolled into scrollback', async () => { + const h = makeRepinHarness() + h.setPinned(false) + h.router.onServerOutput('another streamed row\r\n') + h.completeParse() + expect(h.scrollToBottom).not.toHaveBeenCalled() + await h.router.dispose() + }) +}) diff --git a/src/renderer/src/lib/echo-router.ts b/src/renderer/src/lib/echo-router.ts index 0d92e1f9..264e2bb4 100644 --- a/src/renderer/src/lib/echo-router.ts +++ b/src/renderer/src/lib/echo-router.ts @@ -139,6 +139,32 @@ export function createEchoRouter(deps: EchoRouterDeps): EchoRouter { if (wasPinned) deps.scrollToBottom() } + // Re-pin to the bottom AFTER xterm has parsed a direct-route chunk, not + // before it. `term.write` is asynchronous: a chunk can move the viewport off + // the bottom DURING its own parse — a TUI's SIGWINCH full-screen redraw + // rewrites scrollback and resets ydisp to the top — so a scrollToBottom + // issued synchronously (writePinnedAware, before the parse) is immediately + // undone by the parse. Once the viewport is stranded off-bottom, + // isViewportPinned() reads false and every subsequent chunk stops re-pinning: + // the grid freezes in scrollback showing byte-correct content forever + // (pear#403 — codex resize-mid-stream, viewport [0,baseY] at quiet). Firing + // the re-pin from the write-completion callback closes that race. Follow + // intent is still captured BEFORE the write, so a user who has deliberately + // scrolled up (viewportY < baseY) is never yanked back down. + const writeDirectRepinned = (data: string, callback?: () => void): void => { + if (!deps.isViewportPinned()) { + // Viewport is not at the bottom: the user has deliberately scrolled into + // scrollback. Leave it there — never yank a reader down — and don't + // manufacture a completion callback the caller didn't ask for. + deps.write(data, callback) + return + } + deps.write(data, () => { + deps.scrollToBottom() + callback?.() + }) + } + // Engine route: always enqueued (the engine's tail is asynchronous). const writeViaEngine = (engine: PredictiveEchoWithStatus, data: string): void => { enqueueOp(() => { @@ -159,11 +185,11 @@ export function createEchoRouter(deps: EchoRouterDeps): EchoRouter { // zero overhead on local sessions), ordered behind it otherwise. const writeDirectOrdered = (data: string, callback?: () => void): void => { if (queuedOps === 0) { - writePinnedAware(data, (chunk) => deps.write(chunk, callback)) + writeDirectRepinned(data, callback) return } enqueueOp(() => { - writePinnedAware(data, (chunk) => deps.write(chunk, callback)) + writeDirectRepinned(data, callback) }) } diff --git a/src/renderer/src/lib/terminal-runtime-registry.dom.test.ts b/src/renderer/src/lib/terminal-runtime-registry.dom.test.ts index 9b1596a0..9cd2b220 100644 --- a/src/renderer/src/lib/terminal-runtime-registry.dom.test.ts +++ b/src/renderer/src/lib/terminal-runtime-registry.dom.test.ts @@ -432,3 +432,59 @@ describe('terminal-runtime-registry — dispose cancels pending init rAF', () => } }) }) + +// pear#403: a reflow (fitAddon.fit → term.resize) can scroll the viewport off +// the bottom — a narrower grid rewraps lines into scrollback, bumping baseY +// past viewportY. tryFit must re-pin a FOLLOWING viewport to the tail on every +// reflow, so a resize storm never strands the grid in scrollback with +// byte-correct content. The follow intent is sticky (flipped only by a real +// user wheel scroll), NOT the instantaneous viewportY===baseY, because that is +// exactly what a transient reflow poisons. +describe('terminal-runtime-registry — #403 follow-bottom re-pin across reflow', () => { + async function nextFrame(): Promise { + await new Promise((resolve) => requestAnimationFrame(() => resolve())) + } + + it('re-pins on fit while following, suspends after a wheel-scroll into scrollback, resumes at the bottom', async () => { + const runtime = registry.acquireTerminalRuntime({ + projectId: 'p', + agentName: 'a', + terminalMode: 'drive', + theme: 'dark', + getInputSrtt: () => null + }) + const term = createdTerminals[0] + // Give the runtime's own host layout so tryFit runs its fit + re-pin. + Object.defineProperty(runtime.host, 'clientWidth', { configurable: true, value: 800 }) + Object.defineProperty(runtime.host, 'clientHeight', { configurable: true, value: 600 }) + runtime.mount(makeLayoutContainer()) + await flushAsync() + + const scrollSpy = vi.spyOn(term, 'scrollToBottom') + + // Following by default: a reflow re-pins to the bottom. + scrollSpy.mockClear() + runtime.fitAndSync() + expect(scrollSpy).toHaveBeenCalled() + + // User wheels up into scrollback (viewport off the bottom): following is + // suspended, so a subsequent reflow must NOT yank them down. + term.buffer.active.viewportY = 40 + term.buffer.active.baseY = 100 + runtime.host.dispatchEvent(new Event('wheel')) + await nextFrame() + scrollSpy.mockClear() + runtime.fitAndSync() + expect(scrollSpy).not.toHaveBeenCalled() + + // User returns to the bottom: following resumes and reflow re-pins again. + term.buffer.active.viewportY = 100 + runtime.host.dispatchEvent(new Event('wheel')) + await nextFrame() + scrollSpy.mockClear() + runtime.fitAndSync() + expect(scrollSpy).toHaveBeenCalled() + + registry.disposeTerminalRuntime(runtime.key) + }) +}) diff --git a/src/renderer/src/lib/terminal-runtime-registry.ts b/src/renderer/src/lib/terminal-runtime-registry.ts index 11adfdbb..81cac86d 100644 --- a/src/renderer/src/lib/terminal-runtime-registry.ts +++ b/src/renderer/src/lib/terminal-runtime-registry.ts @@ -294,6 +294,29 @@ function createRuntime( term.loadAddon(fitAddon) term.loadAddon(new WebLinksAddon()) + // #403: sticky "user is following the bottom" intent. The instantaneous + // `viewportY === baseY` is NOT a reliable follow signal: a width/height + // reflow (resize) or a TUI's SIGWINCH full-screen redraw can transiently + // scroll the viewport off the bottom, and once it is off, that instantaneous + // check reads false, so every subsequent write/fit stops re-pinning and the + // grid freezes in scrollback with byte-correct content (codex + // resize-mid-stream: viewport ends at [0, baseY] though every cell matches + // the broker). `followBottom` is flipped ONLY by a real user wheel scroll, so + // reflow/redraw unpins are always corrected while a deliberate scrollback + // read is respected. The echo-router and tryFit re-pin against this intent. + let followBottom = true + host.addEventListener( + 'wheel', + () => { + // Read after xterm has applied the scroll, so a wheel-to-bottom re-arms + // following and a wheel-up (into scrollback) suspends it. + requestAnimationFrame(() => { + if (term) followBottom = isViewportPinnedToBottom(term) + }) + }, + { passive: true } + ) + let onDataHandler: ((data: string) => void) | null = null term.onData((data) => { onDataHandler?.(data) @@ -350,7 +373,12 @@ function createRuntime( getEngine: () => predictiveEcho, buildModelSeed: () => (term ? buildModelSeedFromTerminal(term) : '\x1bc'), getInputSrtt: () => currentSrttGetter(), - isViewportPinned: () => (term ? isViewportPinnedToBottom(term) : false), + // Re-pin against the sticky follow intent, not the instantaneous viewport + // position: a chunk that scrolls the viewport off the bottom during its + // own async parse must still be re-pinned by the completion callback + // (pear#403). A user who scrolled into scrollback (wheel-up) clears the + // intent and is left alone. + isViewportPinned: () => followBottom, scrollToBottom: () => term?.scrollToBottom() }) // Quiet-time convergence to the broker's authoritative screen. Catches the @@ -440,6 +468,14 @@ function createRuntime( } catch { return null } + // #403: a width/height reflow can scroll the viewport off the bottom (a + // narrower grid rewraps lines into scrollback, bumping baseY past + // viewportY). Centralize the re-pin here so EVERY reflow site — the + // ResizeObserver fit, the reconciler's onPersistentDimsMismatch resync, + // init, and refreshOnShow — keeps a following viewport at the tail. Gated + // on the sticky follow intent, so a resize while the user is reading + // scrollback does not yank them down. + if (followBottom) term.scrollToBottom() const { rows, cols } = term if (rows > 0 && cols > 0) { return { rows, cols }