mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-09 13:21:42 +00:00
opentui(v6): kill expand/collapse scroll jitter (suspend stickyScroll across the toggle)
User feedback: tool/thinking rows did a "v small quick lil jump up and
down" when toggled, worst on the bottom rows.
Root cause (verified live with 10ms tmux capture sampling): the
transcript scrollbox's sticky-bottom re-pin and the scroll anchor fought
AFTER paint. On a toggle near the bottom, the content-height change runs
ScrollBox.recalculateBarProps -> applyStickyStart("bottom") (the user is
at the sticky position, so _hasManualScroll is false), which paints a
fully bottom-pinned frame; the anchor's 4x16ms scrollTo re-asserts then
yanked the viewport back up. The capture burst shows the transient
pinned frame between two anchored ones on every expand — the visible
down-up flick.
Fix at the cause instead of correcting after the effect: suspend
stickyScroll (a runtime get/set property on ScrollBoxRenderable) BEFORE
running the toggle and restore it ~100ms later, once the content height
has settled. With sticky off, the toggle's layout pass leaves scrollTop
untouched — the clicked header's document position is unchanged (content
grows/shrinks below it), so nothing moves and there is nothing left to
flicker; a collapse past the new bottom clamps naturally via the
ScrollBar scrollSize setter. Restoring recomputes the manual-scroll
state from the actual position: still at the bottom -> keeps pinning for
new content; mid-content -> manual-scroll semantics until the user
returns (the same end state the old anchor produced). Rapid re-toggles
inside the window keep the ORIGINAL saved value.
The far-from-bottom anchor guarantee is unchanged (scrollTop is simply
never touched), pinned headlessly in scrollAnchor.test.tsx along with
the suspension sequencing, the clamp-then-re-pin collapse path, and the
double-toggle restore. ffiSafe's tall-diff scroll-cut regression now
drives the negative-y condition explicitly via wheel scrolls (the old
anchor exercised it through the very transient sticky-bottom frames this
fix removes).
Verified live (tmux, real gateway): before — toggling the bottom rows
painted a transient bottom-pinned frame (f141 of a 10ms burst); after —
three toggle bursts produce ONLY the clean before/after states (4
distinct frames in 458 samples), headers hold their row, including the
bottom-most rows.
This commit is contained in:
parent
38eb9bb19a
commit
e3cdedbf0f
4 changed files with 248 additions and 21 deletions
|
|
@ -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))
|
||||
|
|
|
|||
|
|
@ -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<void>
|
||||
/** 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<void>
|
||||
/** 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()
|
||||
|
|
|
|||
180
ui-opentui/src/test/scrollAnchor.test.tsx
Normal file
180
ui-opentui/src/test/scrollAnchor.test.tsx
Normal file
|
|
@ -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<ScrollBoxRenderable>()
|
||||
const [expanded, setExpanded] = createSignal(false)
|
||||
const [extra, setExtra] = createSignal(0) // post-toggle "streamed" rows
|
||||
|
||||
function Grab() {
|
||||
anchor = useScrollAnchor()
|
||||
return null
|
||||
}
|
||||
|
||||
const node = () => (
|
||||
<scrollbox
|
||||
ref={(el: ScrollBoxRenderable) => {
|
||||
sb = el
|
||||
setScroll(el)
|
||||
}}
|
||||
style={{ height: 10, width: 40 }}
|
||||
stickyScroll
|
||||
stickyStart="bottom"
|
||||
>
|
||||
<ScrollAnchorProvider scroll={scroll}>
|
||||
<Grab />
|
||||
<For each={Array.from({ length: probeRows }, (_, i) => i)}>{i => <text>{`row-${i}`}</text>}</For>
|
||||
<Show when={expanded()}>
|
||||
<For each={Array.from({ length: 10 }, (_, i) => i)}>{i => <text>{`body-${i}`}</text>}</For>
|
||||
</Show>
|
||||
<For each={Array.from({ length: extra() }, (_, i) => i)}>{i => <text>{`new-${i}`}</text>}</For>
|
||||
</ScrollAnchorProvider>
|
||||
</scrollbox>
|
||||
)
|
||||
|
||||
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()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
@ -2,17 +2,32 @@
|
|||
* Scroll anchoring for collapse/expand toggles (item #4). The transcript
|
||||
* <scrollbox> 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<AnchorFn>()
|
||||
|
||||
/**
|
||||
* 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<ScrollBoxRenderable | undefined>
|
||||
children: JSX.Element
|
||||
}) {
|
||||
let timer: ReturnType<typeof setTimeout> | 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 <Ctx.Provider value={around}>{props.children}</Ctx.Provider>
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue