Skip to content

Commit 26f42d9

Browse files
khaliqgantclaude
andauthored
fix(terminal): keep the viewport pinned across resize/redraw reflows (#403) (#412)
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 <noreply@anthropic.com>
1 parent a52d3a2 commit 26f42d9

4 files changed

Lines changed: 190 additions & 3 deletions

File tree

src/renderer/src/lib/echo-router.test.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -562,3 +562,72 @@ describe('echo-router — reseed capture timeout', () => {
562562
expect(written).toEqual(['held'])
563563
})
564564
})
565+
566+
// pear#403: on the direct route the viewport re-pin must fire from the write
567+
// COMPLETION callback (after xterm parses the chunk), not synchronously before
568+
// it. term.write is async: a chunk can scroll the viewport off the bottom
569+
// during its own parse (a TUI SIGWINCH full-screen redraw rewrites scrollback
570+
// and resets ydisp). A synchronous scrollToBottom issued before that parse is
571+
// undone by it; the viewport is then stranded off-bottom, isViewportPinned()
572+
// reads false, and every later chunk stops re-pinning — the grid freezes in
573+
// scrollback with byte-correct content (codex resize-mid-stream, viewport
574+
// [0,baseY] at quiet). These lock the post-parse re-pin, gated on the
575+
// pre-write follow intent so a deliberate scrollback read is never yanked down.
576+
describe('echo-router — #403 direct-route viewport re-pin after parse', () => {
577+
function makeRepinHarness() {
578+
const writes: string[] = []
579+
let pendingCb: (() => void) | null = null
580+
let pinned = true
581+
const scrollToBottom = vi.fn()
582+
const router = createEchoRouter({
583+
// Defer the completion callback so the test controls the parse boundary,
584+
// exactly like xterm's async WriteBuffer.
585+
write: (data, callback) => {
586+
writes.push(data)
587+
pendingCb = callback ?? null
588+
},
589+
getEngine: () => null,
590+
buildModelSeed: () => '\x1bc',
591+
getInputSrtt: () => null,
592+
isViewportPinned: () => pinned,
593+
scrollToBottom
594+
})
595+
return {
596+
router,
597+
writes,
598+
scrollToBottom,
599+
setPinned: (value: boolean) => {
600+
pinned = value
601+
},
602+
completeParse: () => {
603+
const cb = pendingCb
604+
pendingCb = null
605+
cb?.()
606+
}
607+
}
608+
}
609+
610+
it('re-pins only AFTER the chunk is parsed, never synchronously before it', async () => {
611+
const h = makeRepinHarness()
612+
// A full-screen redraw: the kind of chunk whose parse moves the viewport.
613+
h.router.onServerOutput('\x1b[2J\x1b[Hfull redraw at new size')
614+
// Write issued, parse not yet complete → a synchronous (pre-parse) re-pin
615+
// would already have fired here. It must not.
616+
expect(h.writes).toHaveLength(1)
617+
expect(h.scrollToBottom).not.toHaveBeenCalled()
618+
// Parse completes (viewport may now be off-bottom); the completion callback
619+
// re-pins, so the stranding cascade never starts.
620+
h.completeParse()
621+
expect(h.scrollToBottom).toHaveBeenCalledTimes(1)
622+
await h.router.dispose()
623+
})
624+
625+
it('does not re-pin when the user has scrolled into scrollback', async () => {
626+
const h = makeRepinHarness()
627+
h.setPinned(false)
628+
h.router.onServerOutput('another streamed row\r\n')
629+
h.completeParse()
630+
expect(h.scrollToBottom).not.toHaveBeenCalled()
631+
await h.router.dispose()
632+
})
633+
})

src/renderer/src/lib/echo-router.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,32 @@ export function createEchoRouter(deps: EchoRouterDeps): EchoRouter {
139139
if (wasPinned) deps.scrollToBottom()
140140
}
141141

142+
// Re-pin to the bottom AFTER xterm has parsed a direct-route chunk, not
143+
// before it. `term.write` is asynchronous: a chunk can move the viewport off
144+
// the bottom DURING its own parse — a TUI's SIGWINCH full-screen redraw
145+
// rewrites scrollback and resets ydisp to the top — so a scrollToBottom
146+
// issued synchronously (writePinnedAware, before the parse) is immediately
147+
// undone by the parse. Once the viewport is stranded off-bottom,
148+
// isViewportPinned() reads false and every subsequent chunk stops re-pinning:
149+
// the grid freezes in scrollback showing byte-correct content forever
150+
// (pear#403 — codex resize-mid-stream, viewport [0,baseY] at quiet). Firing
151+
// the re-pin from the write-completion callback closes that race. Follow
152+
// intent is still captured BEFORE the write, so a user who has deliberately
153+
// scrolled up (viewportY < baseY) is never yanked back down.
154+
const writeDirectRepinned = (data: string, callback?: () => void): void => {
155+
if (!deps.isViewportPinned()) {
156+
// Viewport is not at the bottom: the user has deliberately scrolled into
157+
// scrollback. Leave it there — never yank a reader down — and don't
158+
// manufacture a completion callback the caller didn't ask for.
159+
deps.write(data, callback)
160+
return
161+
}
162+
deps.write(data, () => {
163+
deps.scrollToBottom()
164+
callback?.()
165+
})
166+
}
167+
142168
// Engine route: always enqueued (the engine's tail is asynchronous).
143169
const writeViaEngine = (engine: PredictiveEchoWithStatus, data: string): void => {
144170
enqueueOp(() => {
@@ -159,11 +185,11 @@ export function createEchoRouter(deps: EchoRouterDeps): EchoRouter {
159185
// zero overhead on local sessions), ordered behind it otherwise.
160186
const writeDirectOrdered = (data: string, callback?: () => void): void => {
161187
if (queuedOps === 0) {
162-
writePinnedAware(data, (chunk) => deps.write(chunk, callback))
188+
writeDirectRepinned(data, callback)
163189
return
164190
}
165191
enqueueOp(() => {
166-
writePinnedAware(data, (chunk) => deps.write(chunk, callback))
192+
writeDirectRepinned(data, callback)
167193
})
168194
}
169195

src/renderer/src/lib/terminal-runtime-registry.dom.test.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,3 +432,59 @@ describe('terminal-runtime-registry — dispose cancels pending init rAF', () =>
432432
}
433433
})
434434
})
435+
436+
// pear#403: a reflow (fitAddon.fit → term.resize) can scroll the viewport off
437+
// the bottom — a narrower grid rewraps lines into scrollback, bumping baseY
438+
// past viewportY. tryFit must re-pin a FOLLOWING viewport to the tail on every
439+
// reflow, so a resize storm never strands the grid in scrollback with
440+
// byte-correct content. The follow intent is sticky (flipped only by a real
441+
// user wheel scroll), NOT the instantaneous viewportY===baseY, because that is
442+
// exactly what a transient reflow poisons.
443+
describe('terminal-runtime-registry — #403 follow-bottom re-pin across reflow', () => {
444+
async function nextFrame(): Promise<void> {
445+
await new Promise<void>((resolve) => requestAnimationFrame(() => resolve()))
446+
}
447+
448+
it('re-pins on fit while following, suspends after a wheel-scroll into scrollback, resumes at the bottom', async () => {
449+
const runtime = registry.acquireTerminalRuntime({
450+
projectId: 'p',
451+
agentName: 'a',
452+
terminalMode: 'drive',
453+
theme: 'dark',
454+
getInputSrtt: () => null
455+
})
456+
const term = createdTerminals[0]
457+
// Give the runtime's own host layout so tryFit runs its fit + re-pin.
458+
Object.defineProperty(runtime.host, 'clientWidth', { configurable: true, value: 800 })
459+
Object.defineProperty(runtime.host, 'clientHeight', { configurable: true, value: 600 })
460+
runtime.mount(makeLayoutContainer())
461+
await flushAsync()
462+
463+
const scrollSpy = vi.spyOn(term, 'scrollToBottom')
464+
465+
// Following by default: a reflow re-pins to the bottom.
466+
scrollSpy.mockClear()
467+
runtime.fitAndSync()
468+
expect(scrollSpy).toHaveBeenCalled()
469+
470+
// User wheels up into scrollback (viewport off the bottom): following is
471+
// suspended, so a subsequent reflow must NOT yank them down.
472+
term.buffer.active.viewportY = 40
473+
term.buffer.active.baseY = 100
474+
runtime.host.dispatchEvent(new Event('wheel'))
475+
await nextFrame()
476+
scrollSpy.mockClear()
477+
runtime.fitAndSync()
478+
expect(scrollSpy).not.toHaveBeenCalled()
479+
480+
// User returns to the bottom: following resumes and reflow re-pins again.
481+
term.buffer.active.viewportY = 100
482+
runtime.host.dispatchEvent(new Event('wheel'))
483+
await nextFrame()
484+
scrollSpy.mockClear()
485+
runtime.fitAndSync()
486+
expect(scrollSpy).toHaveBeenCalled()
487+
488+
registry.disposeTerminalRuntime(runtime.key)
489+
})
490+
})

src/renderer/src/lib/terminal-runtime-registry.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,29 @@ function createRuntime(
295295
term.loadAddon(fitAddon)
296296
term.loadAddon(new WebLinksAddon())
297297

298+
// #403: sticky "user is following the bottom" intent. The instantaneous
299+
// `viewportY === baseY` is NOT a reliable follow signal: a width/height
300+
// reflow (resize) or a TUI's SIGWINCH full-screen redraw can transiently
301+
// scroll the viewport off the bottom, and once it is off, that instantaneous
302+
// check reads false, so every subsequent write/fit stops re-pinning and the
303+
// grid freezes in scrollback with byte-correct content (codex
304+
// resize-mid-stream: viewport ends at [0, baseY] though every cell matches
305+
// the broker). `followBottom` is flipped ONLY by a real user wheel scroll, so
306+
// reflow/redraw unpins are always corrected while a deliberate scrollback
307+
// read is respected. The echo-router and tryFit re-pin against this intent.
308+
let followBottom = true
309+
host.addEventListener(
310+
'wheel',
311+
() => {
312+
// Read after xterm has applied the scroll, so a wheel-to-bottom re-arms
313+
// following and a wheel-up (into scrollback) suspends it.
314+
requestAnimationFrame(() => {
315+
if (term) followBottom = isViewportPinnedToBottom(term)
316+
})
317+
},
318+
{ passive: true }
319+
)
320+
298321
let onDataHandler: ((data: string) => void) | null = null
299322
term.onData((data) => {
300323
onDataHandler?.(data)
@@ -351,7 +374,12 @@ function createRuntime(
351374
getEngine: () => predictiveEcho,
352375
buildModelSeed: () => (term ? buildModelSeedFromTerminal(term) : '\x1bc'),
353376
getInputSrtt: () => currentSrttGetter(),
354-
isViewportPinned: () => (term ? isViewportPinnedToBottom(term) : false),
377+
// Re-pin against the sticky follow intent, not the instantaneous viewport
378+
// position: a chunk that scrolls the viewport off the bottom during its
379+
// own async parse must still be re-pinned by the completion callback
380+
// (pear#403). A user who scrolled into scrollback (wheel-up) clears the
381+
// intent and is left alone.
382+
isViewportPinned: () => followBottom,
355383
scrollToBottom: () => term?.scrollToBottom()
356384
})
357385
// Quiet-time convergence to the broker's authoritative screen. Catches the
@@ -462,6 +490,14 @@ function createRuntime(
462490
} catch {
463491
return null
464492
}
493+
// #403: a width/height reflow can scroll the viewport off the bottom (a
494+
// narrower grid rewraps lines into scrollback, bumping baseY past
495+
// viewportY). Centralize the re-pin here so EVERY reflow site — the
496+
// ResizeObserver fit, the reconciler's onPersistentDimsMismatch resync,
497+
// init, and refreshOnShow — keeps a following viewport at the tail. Gated
498+
// on the sticky follow intent, so a resize while the user is reading
499+
// scrollback does not yank them down.
500+
if (followBottom) term.scrollToBottom()
465501
const { rows, cols } = term
466502
if (rows > 0 && cols > 0) {
467503
return { rows, cols }

0 commit comments

Comments
 (0)