diff --git a/ui-tui/src/__tests__/transcriptScrollbarInline.test.tsx b/ui-tui/src/__tests__/transcriptScrollbarInline.test.tsx new file mode 100644 index 000000000..99ed7ce63 --- /dev/null +++ b/ui-tui/src/__tests__/transcriptScrollbarInline.test.tsx @@ -0,0 +1,167 @@ +import { PassThrough } from 'node:stream' + +import { Box, renderSync, type ScrollBoxHandle, Text } from '@clawcodex/ink' +import React from 'react' +import { describe, expect, it } from 'vitest' + +import { TranscriptScrollbar } from '../components/appChrome.js' +import { INLINE_MODE } from '../config/env.js' +import { stripAnsi } from '../lib/text.js' +import { DEFAULT_THEME } from '../theme.js' + +/** + * Regression: resizing the terminal blanked the whole transcript (reported in + * the VS Code integrated terminal). + * + * Inline mode lays the tree out with `calculateLayout(columns)` and NO height + * constraint, so the transcript ScrollBox sizes to its content. This scrollbar + * is a ROW SIBLING of that ScrollBox and renders exactly `viewportHeight` rows + * tall — and that value is LAST frame's `scrollViewportHeight`, written by the + * renderer at paint time. A flex row stretches its children to the tallest one, + * so a stale-tall bar stretches the ScrollBox; its inner wrapper (flexGrow:1) + * fills the extra with blank rows; and the stretched height becomes the next + * frame's `scrollViewportHeight`, re-rendering the bar at that same height. A + * stable fixed point, not a transient. + * + * Widening the terminal re-wraps every row shorter, so the gap opens exactly + * when the content shrinks: measured 161 → 102 rows at 60 → 110 columns on a + * real PTY, with the ScrollBox pinned at 161. Those ~59 blank rows pushed the + * transcript above the terminal viewport, where the resize repaint's + * ERASE_SCROLLBACK (log-update `fullReset` → `clearTerminal`) wiped it from + * scrollback — so the transcript was gone, not merely scrolled off. + * + * ROWS, not glyphs, is the thing to assert. Inline mode always satisfies + * `total <= vp`, so the bar takes its !scrollable branch and paints a column of + * SPACES: invisible, but 40 rows tall, and height is the whole defect. Pinning + * "draws no bar characters" would pass against the bug. + * + * The end-to-end proof is a PTY + pyte run against the real binary: blank rows + * after a 60→110 resize went 20-22/30 → 7-8/30. Only WIDENING exhibits it — + * narrowing grows the content past the stale-short bar, so there is no gap to + * open — and that run needs a live agent-server, so it can't happen here. + * + * What this file pins is the guard, not the loop: reproducing the loop needs a + * real TTY resize, which renderSync over a PassThrough does not deliver. + */ + +const SENTINEL = 'BAR_PROBE' + +/** Reports a stale-tall viewport, exactly as it would one frame after a widen. */ +function staleTallHandle(viewportHeight: number, scrollHeight: number): ScrollBoxHandle { + return { + getFreshScrollHeight: () => scrollHeight, + getLastManualScrollAt: () => 0, + getPendingDelta: () => 0, + getScrollHeight: () => scrollHeight, + getScrollTop: () => 0, + getViewportHeight: () => viewportHeight, + getViewportTop: () => 0, + isSticky: () => true, + scrollBy: () => {}, + scrollTo: () => {}, + scrollToBottom: () => {}, + scrollToElement: () => {}, + setClampBounds: () => {}, + subscribe: () => () => {} + } as unknown as ScrollBoxHandle +} + +/** + * Rows the bar occupies, measured from a frame we KNOW was painted. + * + * A fixed settle would be load-dependent, and here it would fail open: with the + * fix the bar paints nothing, so "no rows yet" is indistinguishable from "not + * rendered yet" and a slow CI box would turn this green against the bug. The + * sentinel is a positive signal — once it appears a frame has flushed, and the + * bar's contribution to that frame's height is exactly what we measure. + */ +async function paintedRows(bar: null | { scrollHeight: number; viewportHeight: number }) { + const stdout = new PassThrough() + const stdin = new PassThrough() + const stderr = new PassThrough() + let out = '' + + stdout.on('data', (c: Buffer) => { + out += c.toString() + }) + Object.assign(stdout, { columns: 80, isTTY: true, rows: 24 }) + Object.assign(stdin, { isTTY: true, ref: () => {}, setRawMode: () => {}, unref: () => {} }) + + const app = renderSync( + React.createElement( + Box, + { flexDirection: 'row' }, + React.createElement(Text, null, SENTINEL), + bar + ? React.createElement(TranscriptScrollbar, { + scrollRef: { current: staleTallHandle(bar.viewportHeight, bar.scrollHeight) }, + t: DEFAULT_THEME + }) + : null + ), + { + exitOnCtrlC: false, + patchConsole: false, + stderr: stderr as unknown as NodeJS.WriteStream, + stdin: stdin as unknown as NodeJS.ReadStream, + stdout: stdout as unknown as NodeJS.WriteStream + } + ) + + // Two conditions, both required. The sentinel proves a frame was flushed — + // without it, "no rows yet" and "not rendered yet" are indistinguishable and + // the assertion would fail OPEN against the bug. The quiet period then proves + // the frame is COMPLETE: ink writes incrementally, so measuring on first + // paint can catch the sentinel in one chunk and the bar's rows in a later + // one. Both renders settle the same way, which is what makes their row counts + // comparable. + const deadline = Date.now() + 15_000 + let quiet = 0 + let lastSize = -1 + + while (Date.now() < deadline) { + if (out.length === lastSize && stripAnsi(out).includes(SENTINEL)) { + if (++quiet >= 5) { + break + } + } else { + quiet = 0 + lastSize = out.length + } + + await new Promise(resolve => setTimeout(resolve, 20)) + } + + app.unmount() + + const painted = stripAnsi(out) + + if (!painted.includes(SENTINEL) || quiet < 5) { + throw new Error('renderer never settled a frame') + } + + return (painted.match(/\n/g) ?? []).length +} + +describe('TranscriptScrollbar in inline mode', () => { + it('is the default mode, so the guard below is the one that ships', () => { + expect(INLINE_MODE).toBe(true) + }) + + it('adds no rows when the content fits — the real inline case', async () => { + // total === vp is what inline mode always produces: the ScrollBox sizes to + // its content, so it never scrolls. Pre-fix this painted 40 rows of spaces + // and stretched the transcript by the same 40 rows. + const baseline = await paintedRows(null) + + expect(await paintedRows({ scrollHeight: 40, viewportHeight: 40 })).toBe(baseline) + }) + + it('adds no rows even if it somehow believes it is scrollable', async () => { + // Defence in depth: a stale snapshot could report total > vp mid-resize, + // which pre-fix drew 40 rows of ┃/│ glyphs. + const baseline = await paintedRows(null) + + expect(await paintedRows({ scrollHeight: 120, viewportHeight: 40 })).toBe(baseline) + }) +}) diff --git a/ui-tui/src/components/agentsOverlay.tsx b/ui-tui/src/components/agentsOverlay.tsx index c4a6e8e3c..3996baf1d 100644 --- a/ui-tui/src/components/agentsOverlay.tsx +++ b/ui-tui/src/components/agentsOverlay.tsx @@ -11,6 +11,7 @@ import { import { patchOverlayState } from '../app/overlayStore.js' import { $spawnDiff, $spawnHistory, clearDiffPair, type SpawnSnapshot } from '../app/spawnHistoryStore.js' import { useTurnSelector } from '../app/turnStore.js' +import { INLINE_MODE } from '../config/env.js' import type { GatewayClient } from '../gatewayClient.js' import type { DelegationPauseResponse, DelegationStatusResponse, SubagentInterruptResponse } from '../gatewayTypes.js' import { asRpcResult } from '../lib/rpc.js' @@ -156,7 +157,11 @@ function OverlayScrollbar({ const s = scrollRef.current const vp = Math.max(0, s?.getViewportHeight() ?? 0) - if (!vp) { + // Inline mode: same stale-tall-sibling trap as TranscriptScrollbar — see the + // long comment there. This bar is `vp` rows tall and is a row sibling of the + // overlay's ScrollBox, so in an unconstrained-height tree it stretches that + // ScrollBox and pads the difference with blank rows. + if (!vp || INLINE_MODE) { return } diff --git a/ui-tui/src/components/appChrome.tsx b/ui-tui/src/components/appChrome.tsx index 251db7f49..ff242bbe7 100644 --- a/ui-tui/src/components/appChrome.tsx +++ b/ui-tui/src/components/appChrome.tsx @@ -6,7 +6,7 @@ import unicodeSpinners from 'unicode-animations' import { $delegationState } from '../app/delegationStore.js' import type { IndicatorStyle, Notice } from '../app/interfaces.js' import { useTurnSelector } from '../app/turnStore.js' -import { DEV_CREDITS_MODE } from '../config/env.js' +import { DEV_CREDITS_MODE, INLINE_MODE } from '../config/env.js' import { FACES } from '../content/faces.js' import { VERBS } from '../content/verbs.js' import { fmtDuration } from '../domain/messages.js' @@ -723,7 +723,28 @@ export function TranscriptScrollbar({ scrollRef, t }: TranscriptScrollbarProps) const grabRef = useRef(null) const { scrollHeight: total, top: pos, viewportHeight: vp } = useScrollbarSnapshot(scrollRef) - if (!vp) { + // Inline mode renders no scrollbar, and MUST NOT render a tall one. + // + // `vp` is last frame's scrollViewportHeight (render-node-to-output writes it + // at paint time), and the bar below is exactly `vp` rows tall. It is a row + // sibling of the transcript ScrollBox, and that row stretches its children to + // the tallest one — so a stale-tall bar stretches the ScrollBox, whose inner + // wrapper (flexGrow:1) then fills the extra with blank rows. The stretched + // height becomes the next frame's scrollViewportHeight, which re-renders the + // bar at that same height: a stable fixed point, not a transient. + // + // That is what blanked the transcript on resize. Widening the terminal + // re-wraps every row shorter (measured 161 → 102 rows at 60 → 110 columns), + // but the bar held the ScrollBox at 161, and the ~59 blank rows pushed the + // transcript above the terminal viewport — where the resize repaint's + // ERASE_SCROLLBACK then wiped it from scrollback. + // + // Inline mode has no constrained-height root, so the ScrollBox always sizes + // to its content and `total <= vp` holds: the bar would take the !scrollable + // branch and paint a column of spaces. It is invisible there and contributes + // nothing but height. Keep the 1-column gutter so transcriptPanelWidth's + // reservation still matches what we draw. + if (!vp || INLINE_MODE) { return } diff --git a/ui-tui/src/hooks/useVirtualHistory.ts b/ui-tui/src/hooks/useVirtualHistory.ts index 2c3cea511..e38587861 100644 --- a/ui-tui/src/hooks/useVirtualHistory.ts +++ b/ui-tui/src/hooks/useVirtualHistory.ts @@ -189,6 +189,11 @@ export function useVirtualHistory( // key → React.Object.is short-circuits the commit entirely. The key includes // sticky state, target scroll position, and viewport height so resize-only // changes still recompute the mounted transcript window. + // Stable deps are only safe because the transcript ScrollBox never remounts: + // useImperativeHandle(..., []) builds a NEW listenersRef Set per mount, and + // this subscribe identity never changes, so React would never resubscribe — + // the listeners would stay on the dead handle. Anything that makes a + // ScrollBox handle identity change must add a handle version to these deps. const subscribe = useCallback( (cb: () => void) => (hasScrollRef ? scrollRef.current?.subscribe(cb) : null) ?? NOOP, [hasScrollRef, scrollRef] diff --git a/ui-tui/src/lib/viewportStore.ts b/ui-tui/src/lib/viewportStore.ts index e379cf44e..63bec1337 100644 --- a/ui-tui/src/lib/viewportStore.ts +++ b/ui-tui/src/lib/viewportStore.ts @@ -85,6 +85,11 @@ export function scrollbarSnapshotKey(v: ScrollbarSnapshot) { } export function useViewportSnapshot(scrollRef: RefObject): ViewportSnapshot { + // Stable deps are only safe because the transcript ScrollBox never remounts: + // useImperativeHandle(..., []) builds a NEW listenersRef Set per mount, and + // this subscribe identity never changes, so React would never resubscribe — + // the listeners would stay on the dead handle. Anything that makes a + // ScrollBox handle identity change must add a handle version to these deps. const key = useSyncExternalStore( useCallback((cb: () => void) => scrollRef.current?.subscribe(cb) ?? (() => {}), [scrollRef]), () => viewportSnapshotKey(getViewportSnapshot(scrollRef.current)), @@ -106,6 +111,11 @@ export function useViewportSnapshot(scrollRef: RefObject } export function useScrollbarSnapshot(scrollRef: RefObject): ScrollbarSnapshot { + // Stable deps are only safe because the transcript ScrollBox never remounts: + // useImperativeHandle(..., []) builds a NEW listenersRef Set per mount, and + // this subscribe identity never changes, so React would never resubscribe — + // the listeners would stay on the dead handle. Anything that makes a + // ScrollBox handle identity change must add a handle version to these deps. const key = useSyncExternalStore( useCallback((cb: () => void) => scrollRef.current?.subscribe(cb) ?? (() => {}), [scrollRef]), () => scrollbarSnapshotKey(getScrollbarSnapshot(scrollRef.current)),