diff --git a/ui-opentui/src/test/ffiSafe.test.tsx b/ui-opentui/src/test/ffiSafe.test.tsx index 7904d9a3737..8a6bb73622d 100644 --- a/ui-opentui/src/test/ffiSafe.test.tsx +++ b/ui-opentui/src/test/ffiSafe.test.tsx @@ -102,15 +102,27 @@ describe('node-ffi coordinate safety (boundary/ffiSafe.ts)', () => { const onErr = (e: unknown) => errors.push(e) process.on('uncaughtException', onErr) try { - // Expanding renders transient sticky-bottom frames where the diff top sits - // ABOVE the viewport (negative y) — the exact live-crash condition. await clickHeader(probe, 'patch') - // let tree-sitter + the scrollAnchor's 4x16ms re-asserts land + // let tree-sitter + the scrollAnchor's sticky-suspension window land await new Promise(r => setTimeout(r, 200)) // added rows only paint when the diff body is actually expanded (the // scrollAnchor holds the viewport at the diff TOP, so assert early rows) const expanded = await probe.waitForFrame(f => f.includes('fn_0')) expect(expanded).toContain('+ def fn_0(): pass') + // Scroll INTO the tall diff so its top rows sit ABOVE the viewport + // (negative screen y) — the exact live-crash condition. (The old anchor + // produced this via transient sticky-bottom frames; the sticky + // suspension removed those, so drive the scroll-cut explicitly.) + let downTicks = 0 + while (probe.frame().includes('fn_0(') && downTicks < 30) { + await probe.scroll(40, 15, 'down') + downTicks++ + } + const cut = probe.frame() + expect(cut).not.toContain('fn_0(') // the diff top is cut above the viewport… + expect(cut).toContain('fn_') // …while mid-diff rows still paint (negative-y path) + // bring the header back on screen for the toggle churn + for (let i = 0; i < downTicks + 5; i++) await probe.scroll(40, 15, 'up') // toggle a few times + resize churn await clickHeader(probe, 'patch') await new Promise(r => setTimeout(r, 100)) diff --git a/ui-opentui/src/test/lib/render.ts b/ui-opentui/src/test/lib/render.ts index de7c4e442c9..b0b614f9309 100644 --- a/ui-opentui/src/test/lib/render.ts +++ b/ui-opentui/src/test/lib/render.ts @@ -55,6 +55,8 @@ export interface RenderProbe { readonly resize: (width: number, height: number) => void /** Left-click at screen cell (x, y) via the mock mouse, then settle a pass. */ readonly click: (x: number, y: number) => Promise + /** Mouse-wheel at screen cell (x, y) via the mock mouse, then settle a pass. */ + readonly scroll: (x: number, y: number, direction: 'up' | 'down') => Promise /** The mock keyboard (typeText / pressArrow / pressEnter / …) — pair with `settle()`. */ readonly keys: TestRendererSetup['mockInput'] /** Run a render pass + flush so simulated input lands in the next `frame()`. */ @@ -96,6 +98,11 @@ export async function renderProbe( await setup.renderOnce() await setup.flush() }, + scroll: async (x, y, direction) => { + await setup.mockMouse.scroll(x, y, direction) + await setup.renderOnce() + await setup.flush() + }, keys: setup.mockInput, settle: async () => { await setup.renderOnce() diff --git a/ui-opentui/src/test/scrollAnchor.test.tsx b/ui-opentui/src/test/scrollAnchor.test.tsx new file mode 100644 index 00000000000..266c30f85db --- /dev/null +++ b/ui-opentui/src/test/scrollAnchor.test.tsx @@ -0,0 +1,180 @@ +/** + * Scroll-anchor sequencing (view/scrollAnchor.tsx). The jitter itself is a + * paint-timing artifact (verified live), but the MECHANISM is pinnable + * headlessly: `around(toggle)` must suspend the scrollbox's stickyScroll + * BEFORE the toggle's layout pass (so the sticky re-pin never engages — no + * transient bottom-pinned frame to flicker through), hold scrollTop across + * the toggle, and restore stickyScroll afterwards with the pin-on-new-content + * behavior intact when the view is still at the bottom. + */ +import type { ScrollBoxRenderable } from '@opentui/core' +import { createSignal, For, Show } from 'solid-js' +import { describe, expect, test } from 'vitest' + +import { ScrollAnchorProvider, useScrollAnchor } from '../view/scrollAnchor.tsx' +import { renderProbe, type RenderProbe } from './lib/render.ts' + +/** Sticky-bottom scrollbox + anchored toggleable block, instruments exposed. */ +function mountHarness(probeRows = 30) { + let sb: ScrollBoxRenderable | undefined + let anchor: ((toggle: () => void) => void) | undefined + const [scroll, setScroll] = createSignal() + const [expanded, setExpanded] = createSignal(false) + const [extra, setExtra] = createSignal(0) // post-toggle "streamed" rows + + function Grab() { + anchor = useScrollAnchor() + return null + } + + const node = () => ( + { + sb = el + setScroll(el) + }} + style={{ height: 10, width: 40 }} + stickyScroll + stickyStart="bottom" + > + + + i)}>{i => {`row-${i}`}} + + i)}>{i => {`body-${i}`}} + + i)}>{i => {`new-${i}`}} + + + ) + + return { + node, + sb: () => sb!, + anchor: () => anchor!, + setExpanded, + setExtra + } +} + +async function settle(probe: RenderProbe, passes = 3) { + for (let i = 0; i < passes; i++) await probe.settle() +} + +const sleep = (ms: number) => new Promise(r => setTimeout(r, ms)) + +describe('scroll anchor — stickyScroll suspension (expand/collapse jitter fix)', () => { + test('expanding while pinned at the bottom: sticky suspended, scrollTop never re-pins, restored after', async () => { + const h = mountHarness() + const probe = await renderProbe(h.node, { width: 40, height: 12 }) + try { + await settle(probe) + const sb = h.sb() + expect(sb.stickyScroll).toBe(true) + const pinned = sb.scrollTop + expect(pinned).toBeGreaterThan(0) // 30 rows in a 10-high box → pinned at bottom + + h.anchor()(() => h.setExpanded(true)) + // suspended SYNCHRONOUSLY, before any layout pass could sticky-pin + expect(sb.stickyScroll).toBe(false) + + // across the settle window the viewport must NEVER jump below the + // anchored offset (the old code painted a bottom-pinned frame here) + for (let i = 0; i < 5; i++) { + await probe.settle() + expect(sb.scrollTop).toBe(pinned) + } + + await sleep(150) // > RESTORE_MS + await settle(probe) + expect(sb.stickyScroll).toBe(true) // restored + expect(sb.scrollTop).toBe(pinned) // viewport still held + // content DID grow below (the expansion is real) + expect(sb.scrollHeight).toBeGreaterThan(30) + + // mid-content now → new content must NOT yank the view (manual-scroll + // semantics, same end state as the old anchor) + h.setExtra(3) + await settle(probe) + expect(sb.scrollTop).toBe(pinned) + } finally { + probe.destroy() + } + }) + + test('collapsing at the bottom clamps to the new bottom and sticky pinning resumes for new content', async () => { + const h = mountHarness() + const probe = await renderProbe(h.node, { width: 40, height: 12 }) + try { + await settle(probe) + const sb = h.sb() + // expand first (anchored), then scroll back to the bottom to re-pin + h.anchor()(() => h.setExpanded(true)) + await sleep(150) + await settle(probe) + sb.scrollTo(sb.scrollHeight) // user returns to the bottom + await settle(probe) + const bottom = sb.scrollTop + + h.anchor()(() => h.setExpanded(false)) + expect(sb.stickyScroll).toBe(false) + await settle(probe) + // content shrank: the scrollbar clamps to the NEW max (no sticky needed) + expect(sb.scrollTop).toBeLessThan(bottom) + const clamped = sb.scrollTop + + await sleep(150) + await settle(probe) + expect(sb.stickyScroll).toBe(true) + // still at the (new) bottom → sticky re-engages: appended rows pin + h.setExtra(4) + await settle(probe) + expect(sb.scrollTop).toBeGreaterThan(clamped) + } finally { + probe.destroy() + } + }) + + test('rapid double-toggle restores the ORIGINAL stickyScroll once (not our own false)', async () => { + const h = mountHarness() + const probe = await renderProbe(h.node, { width: 40, height: 12 }) + try { + await settle(probe) + const sb = h.sb() + h.anchor()(() => h.setExpanded(true)) + expect(sb.stickyScroll).toBe(false) + // second toggle lands INSIDE the suspension window + h.anchor()(() => h.setExpanded(false)) + expect(sb.stickyScroll).toBe(false) + await sleep(150) + await settle(probe) + expect(sb.stickyScroll).toBe(true) // the original value, not the suspended false + } finally { + probe.destroy() + } + }) + + test('far from the bottom (scrolled up): toggle holds the viewport (the original anchor guarantee)', async () => { + const h = mountHarness() + const probe = await renderProbe(h.node, { width: 40, height: 12 }) + try { + await settle(probe) + const sb = h.sb() + sb.scrollTo(5) // scroll away from the bottom + await settle(probe) + expect(sb.scrollTop).toBe(5) + + h.anchor()(() => h.setExpanded(true)) + for (let i = 0; i < 5; i++) { + await probe.settle() + expect(sb.scrollTop).toBe(5) + } + await sleep(150) + await settle(probe) + expect(sb.stickyScroll).toBe(true) + expect(sb.scrollTop).toBe(5) + } finally { + probe.destroy() + } + }) +}) diff --git a/ui-opentui/src/view/scrollAnchor.tsx b/ui-opentui/src/view/scrollAnchor.tsx index bd3e31d853c..5f13961aafa 100644 --- a/ui-opentui/src/view/scrollAnchor.tsx +++ b/ui-opentui/src/view/scrollAnchor.tsx @@ -2,17 +2,32 @@ * Scroll anchoring for collapse/expand toggles (item #4). The transcript * has stickyScroll+stickyStart="bottom": on a content-height change * it re-pins to the bottom whenever the user hasn't manually scrolled away - * (@opentui/core ScrollBox: `if (stickyStart && !_hasManualScroll) applyStickyStart`). - * So expanding a tool/thinking block while at the bottom yanks the viewport to the - * NEW bottom — scrolling the header you just clicked up off-screen. + * (@opentui/core ScrollBox `recalculateBarProps`: `if (stickyStart && + * !_hasManualScroll) applyStickyStart`). So expanding a tool/thinking block + * while at the bottom yanks the viewport to the NEW bottom — scrolling the + * header you just clicked up off-screen. * - * Fix: keep scrollTop constant across the toggle. The clicked element's document - * position is unchanged (content grows BELOW it), so holding scrollTop keeps that - * header at the same screen row and simply reveals the expansion beneath it. We - * re-assert the saved offset over a few frames because the content height (and the - * sticky re-pin) only settle on the next render pass. + * Fix: SUSPEND stickyScroll for the duration of the toggle (it's a runtime + * get/set property on ScrollBoxRenderable, recomputing its state on set). With + * sticky off, the toggle's layout passes leave scrollTop untouched: the clicked + * element's document position is unchanged (content grows/shrinks BELOW it), so + * the header stays on the same screen row and the expansion is simply revealed + * beneath it; a collapse past the new bottom clamps naturally (ScrollBar's + * `scrollSize` setter re-clamps `scrollPosition`). stickyScroll is restored a + * few frames later, once the content height has settled; the setter then + * recomputes the manual-scroll state from the ACTUAL position — still at the + * bottom → keeps pinning for new content; mid-content → behaves like a manual + * scroll-away until the user returns to the bottom (same end state as before). + * + * Why not hold scrollTop after the fact (the previous approach): re-asserting + * the saved offset over 4×16ms timers FIGHTS the sticky re-pin frame-by-frame — + * the pin paints (viewport jumps down), the re-assert paints (jumps back up) — + * a visible jitter on rows near the bottom, where the pin actually engages + * (reproduced live: a transient fully-bottom-pinned frame between two anchored + * ones on every expand). Suppressing the pin BEFORE the layout pass means no + * correcting after paint, so there is nothing left to flicker. */ -import { type Accessor, createContext, type JSX, useContext } from 'solid-js' +import { type Accessor, createContext, type JSX, onCleanup, useContext } from 'solid-js' import type { ScrollBoxRenderable } from '@opentui/core' @@ -20,31 +35,44 @@ type AnchorFn = (toggle: () => void) => void const Ctx = createContext() +/** + * How long stickyScroll stays suspended after a toggle. The content-height + * change (and any text-rewrap settling) lands over the next render pass or + * two; ~3 frames at the ~30fps render loop covers it (matches the old hold + * window). Restoring "too late" is harmless: the setter recomputes state from + * the actual position, and a mid-stream toggle leaves the view un-pinned + * either way (you're no longer at the bottom after the content grew). + */ +const RESTORE_MS = 100 + export function ScrollAnchorProvider(props: { scroll: Accessor children: JSX.Element }) { + let timer: ReturnType | undefined + let saved = true // the scrollbox's own stickyScroll while suspended const around: AnchorFn = toggle => { const sb = props.scroll() if (!sb) { toggle() return } - const prev = sb.scrollTop + // Rapid re-toggle while still suspended: keep the ORIGINAL saved value + // (reading sb.stickyScroll now would capture our own `false`). + if (timer === undefined) saved = sb.stickyScroll + else clearTimeout(timer) + sb.stickyScroll = false toggle() - // Re-assert across the next few frames: the layout + sticky re-pin land on - // subsequent render passes, so a single sync restore wouldn't hold. - let n = 0 - const hold = () => { + timer = setTimeout(() => { + timer = undefined try { - sb.scrollTo(prev) + sb.stickyScroll = saved } catch { /* renderable torn down */ } - if (++n < 4) setTimeout(hold, 16) - } - setTimeout(hold, 0) + }, RESTORE_MS) } + onCleanup(() => clearTimeout(timer)) return {props.children} }