Skip to content

fix(webui): glide the streaming chat tail instead of snapping it per tick - #7378

Open
StephenZ06 wants to merge 1 commit into
nesquena:masterfrom
StephenZ06:fix/streaming-scroll-glide
Open

StephenZ06 wants to merge 1 commit into
nesquena:masterfrom
StephenZ06:fix/streaming-scroll-glide

Conversation

@StephenZ06

Copy link
Copy Markdown

The problem

While an assistant reply typed itself out, the transcript did not follow the text smoothly. It sat still for several render ticks and then jumped a whole line, and the jump repeated for the length of the answer. Most visible on mobile, but present on desktop too.

Root cause

The streaming follow path snapped scrollTop = scrollHeight on every render tick. A tick only adds a couple of words, so the scroll position does not change at all until a line wraps — at which point the whole line height is applied in one painted step. That is the stutter: it is not dropped frames, it is a step function.

The same path also rebuilt the settle ResizeObserver (disconnect + new ResizeObserver + observe + two timers) per tick, i.e. ~30 observer teardown/rebuild cycles a second on a transcript that may be thousands of nodes deep. That is the dominant per-frame cost on mobile.

The fix

While a turn is streaming — or the word fade is still playing text out after the SSE done — scrollIfPinned() hands the tail to a single rAF loop, _tailFollowFrame(), that eases scrollTop toward the bottom: one scrollHeight read and at most one scrollTop write per frame, closing 26% of the remaining gap so a newly wrapped line arrives as a short glide.

Bounded on both ends:

  • 36px lag ceiling — with the word fade off a tick can add several lines at once, and a purely proportional ease would park the newest lines below the fold.
  • 600px snap threshold — a hopeless gap jumps rather than crawling.
  • Lands exactly on scrollHeight - clientHeight for the last pixel, so the near-bottom predicates elsewhere in the file are not left measuring against a fractional residue.
  • Parks itself after ten quiet frames rather than holding a permanent rAF loop.

Ownership is single-writer: _settleMessageScrollToBottom() and _cancelBottomSettle() stop the glide, so the completion and late-layout paths (Prism, KaTeX, Mermaid, images) still re-anchor the bottom exactly as before.

Invariants preserved

The glide refuses on the same signals the settle path's ResizeObserver callback refuses on, so the sticky-unpin model (#3343 / #4295) is unchanged, plus two additions for this code's neighbours:

  • Mobile: it yields while a finger is actually down on the transcript, so a per-frame write can never cancel an in-progress drag or iOS momentum. Deliberately not the 1200ms _recentMessageTouchScrollIntent() tail — touchstart marks that on any tap (a copy button, expanding a tool card), and pausing follow for 1.2s after a tap would regress against the current settle path, which does not check it at all.
  • Jump-to-question: it yields to an active _messageJumpScrollOwner for the same reason scrollIfPinned() returns early on one (fix(chat): prevent first response jump from snapping back #6621) — a jump deliberately holds the reader at its target across smooth-scroll frames, and two writers would fight each frame.

Siblings found and fixed

Three cadence hitches in the word fade, all the same shape: a setTimeout(33) + rAF pair fires anywhere between 33ms and 50ms depending on where the timer lands relative to the next frame, so the released word wave arrived unevenly even though the pacing math is time-based.

  1. _scheduleRender() now polls rAF against the interval — a steady two-frame beat at 60Hz.
  2. The self-reschedule while the fade is catching up no longer stacks a 33ms timer on top of _scheduleRender()'s own 33ms gate, which had pushed the next playout tick out to as much as 66ms whenever tokens stopped arriving.
  3. The post-done drain gets the same vsync-aligned beat — this is where the hitch was most visible, on an answer's last words.

The drain marks a self-expiring deadline rather than a boolean, so an abandoned drain (session switch, cancel, teardown) can never strand ownership away from the settle.

Tests

tests/test_streaming_tail_follow_glide.py — 12 tests. The behavioural ones drive the real extracted helpers in Node against a fake scroller and a manually pumped frame queue, so they exercise the shipped code, not a replica:

  • a single wrapped line is crossed over several painted frames, not one (this is the stutter itself)
  • the glide never overshoots the bottom, and lands on it exactly
  • lag stays bounded under fast multi-line growth
  • a hopeless gap snaps in one frame
  • it yields to an unpinned reader, and while a finger is down
  • it parks itself when there is nothing to chase
  • the drain deadline keeps ownership after S.activeStreamId clears, and expires on its own

Proof they fail before the fix: stashing the two source files and re-running gives 12 failed. Restoring gives 12 passed.

Verification run on this branch:

  • neighbouring scroll / stream / fade / jitter / anchor suite: 1029 passed
  • full suite: 15082 passed, 119 skipped, 2 failed — the two failures are tests/test_tls_aware_probe.py, which fails on this base before the change and covers scripts/lib/health_probe.sh, not this code.

Verified running

Rebuilt the container image and recreated it, health check green, and confirmed the served /static/ui.js and /static/messages.js carry the new code. Streaming a long prose reply follows the text continuously instead of stepping a line at a time; scrolling up mid-stream still stops follow dead and keeps it stopped.

What I could not verify

  • No before/after images. The change is motion over time — a still frame of a scroll position shows nothing useful, and I had no way to capture video from this environment. The behavioural tests above are the substitute: they assert the intermediate painted positions that a screenshot cannot show.
  • Not tested on real iOS/Android hardware. The mobile-specific reasoning (finger-down yield, iOS momentum, the overflow-anchor interaction that _fixMobileScrollJank() already suppresses during streaming renders) is from reading the surrounding code, not from a device. Worth a check on a real handset before merge.
  • Only exercised against this repo's own streaming path; no other front end consumes these helpers.

🤖 Generated with Claude Code

https://claude.ai/code/session_01S9kbjA9C6YY7f173xr6N6G

…tick

While an assistant reply typed itself out, the transcript did not follow the
text smoothly. It sat still for several render ticks and then jumped a whole
line, and the jump repeated for the length of the answer.

The streaming follow path snapped `scrollTop = scrollHeight` on every render
tick. A tick only adds a couple of words, so the scroll position does not
change at all until a line WRAPS -- at which point the whole line height is
applied in one painted step. The same path also rebuilt the settle
ResizeObserver (disconnect + `new ResizeObserver` + observe + two timers) per
tick, i.e. ~30 observer teardown/rebuild cycles a second on a transcript that
may be thousands of nodes deep, which is the dominant per-frame cost on mobile.

While a turn is streaming -- or the word fade is still playing text out after
the SSE `done` -- `scrollIfPinned()` now hands the tail to a single rAF loop,
`_tailFollowFrame()`, that eases `scrollTop` toward the bottom: one
`scrollHeight` read and at most one `scrollTop` write per frame, closing 26% of
the remaining gap so a newly wrapped line arrives as a short glide. It is
bounded on both ends: a 36px lag ceiling so the newest lines never park below
the fold when the word fade is off and a tick can add several lines at once,
and a 600px snap threshold so a hopeless gap does not crawl. It lands exactly
on `scrollHeight - clientHeight` for the last pixel, and parks itself after ten
quiet frames rather than holding a permanent rAF loop.

Ownership is single-writer: `_settleMessageScrollToBottom()` and
`_cancelBottomSettle()` stop the glide, so the completion and late-layout paths
(Prism, KaTeX, Mermaid, images) still re-anchor the bottom exactly as before.
The glide refuses on the same signals the settle path's ResizeObserver callback
refuses on, so the sticky-unpin model (nesquena#3343/nesquena#4295) is unchanged, plus one
mobile addition: it yields while a finger is actually DOWN on the transcript,
so a per-frame write can never cancel an in-progress drag or iOS momentum.
Deliberately not the 1200ms `_recentMessageTouchScrollIntent()` tail --
touchstart marks that on any tap (a copy button, expanding a tool card), and
pausing follow for 1.2s after a tap would regress against the current settle
path, which does not check it at all.

Three cadence hitches in the word fade are fixed alongside, all the same shape:
a `setTimeout(33) + rAF` pair fires anywhere between 33ms and 50ms depending on
where the timer lands relative to the next frame, so the released word wave
arrived unevenly even though the pacing math is time-based. `_scheduleRender()`
now polls rAF against the interval (a steady two-frame beat at 60Hz); the
self-reschedule while the fade is catching up no longer stacks a 33ms timer on
top of `_scheduleRender()`'s own 33ms gate, which had pushed the next playout
tick out to as much as 66ms whenever tokens stopped arriving; and the post-done
drain gets the same vsync-aligned beat, which is where the hitch was most
visible -- on an answer's last words. The drain marks a self-expiring deadline
rather than a boolean so an abandoned drain (session switch, cancel, teardown)
can never strand ownership away from the settle.

Two integrations with the surrounding scroll code on this base: the glide
yields to an active `_messageJumpScrollOwner` for the same reason
`scrollIfPinned()` returns early on one (nesquena#6621) -- a jump deliberately holds the
reader at its target across smooth-scroll frames, and two writers would fight
each frame -- and the new call in `scrollIfPinned()` carries a `typeof` guard so
the unit harnesses that extract that function body without the tail-follow
helpers stay inert, matching the guard style already used there.

Tests: tests/test_streaming_tail_follow_glide.py drives the real extracted
helpers in Node against a fake scroller and a manually pumped frame queue.
All 12 fail on the pre-change files and pass on these. The neighbouring
scroll/stream/fade/jitter/anchor suite (978 tests) passes, as does the full
suite apart from tests/test_tls_aware_probe.py, which fails on this base
before the change and covers scripts/lib/health_probe.sh, not this code.

Verified on the live instance: rebuilt and recreated the container, health
check green, and the served /static/ui.js and /static/messages.js carry the new
code.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S9kbjA9C6YY7f173xr6N6G
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

I pulled 3829d4510 into a read-only worktree and read the complete three-file diff, both full JavaScript files on the PR head and origin/master, and the full 315-line regression file. The rAF glide itself is coherent: it bounds lag, lands exactly at the bottom, yields to sticky unpin/touch/jump ownership, and prevents the settle observer from competing frame-by-frame. One ownership race blocks this version: the post-done fade lease is a single unscoped window global shared by every stream/session.

Code reference

At static/messages.js:5238-5292, every attached stream closure creates mark/end functions that write the same scalar:

const _markDraining=()=>{
  window._streamFadeDrainingUntil=performance.now()+400;
};
const _endDraining=()=>{
  window._streamFadeDrainingUntil=0;
};

static/ui.js:7246-7253 then treats any future deadline as proof that the current pane still has a live playout. There is no session ID, stream ID, or generation token in that authority.

A post-done drain can continue pumping for the old attachLiveStream() closure after a session switch. Its repeated _markDraining() calls can make a different, non-streaming pane enter glide mode. More importantly, if a newer stream starts its own post-done drain before the old closure finishes, the old _endDraining() clears the newer deadline. Once the newer SSE has already cleared S.activeStreamId, its remaining words fall back to per-tick settle, recreating the final-word snap this PR intends to remove.

Diagnosis / recommendation

Scope the drain lease to its owner and make release compare-and-clear. Capture an immutable token plus activeSid and streamId in _drainStreamFadeBeforeDone(). Publish an object such as {token, sid, streamId, until} only while that session still owns the current pane. _endDraining() must clear only when the stored token still matches. _tailFollowStreamLive() must require the stored session owner to match the current session before consulting the deadline.

This is better than a deadline alone: expiry prevents a permanent stuck flag, but it does not prevent a stale producer from renewing or clearing a newer producer state.

The glide path at static/ui.js:7209-7305 otherwise reads consistently with the existing sticky-unpin model. The cadence changes at static/messages.js:5638-5723 also remove the stacked timer/rAF delay without introducing another writer.

Test plan

The current deadline test at tests/test_streaming_tail_follow_glide.py:251-273 covers only one owner and time expiry. Add two overlapping drain owners: A marks, B replaces it, then A ends. Assert B remains live. Add a session-switch case where A marks after the current pane changes and assert the new pane never enters glide mode. Retain the wrapped-line, lag, snap, unpin, finger-down, quiet-park, and settle-ownership cases. No contributor-authored code was executed.

@nesquena-hermes nesquena-hermes added size:L Large PR (>10 files or >250 LOC) ux User experience / visual polish labels Aug 31, 2026

@Manny7717 Manny7717 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified locally on head 3829d45 (node + repo test harness):

Regression proven. All 12 tests in test_streaming_tail_follow_glide.py FAIL on base e168b67 (helpers don't exist there) and PASS on head — including the ownership-handoff, drain-deadline, and vsync-cadence tests, which exercise the real extracted functions against a fake scroller. Neighboring scroll suites (issue1360, 1731, 3319, 3470, 3479, 2111, 3250): 38/38 pass on head. Streaming/fade suites (2713, 3397, 2965, inflight reuse, cancel owner guard): 91 passed total with the new file. ruff_lint.py --diff origin/master = 0 new violations; node --check clean on both JS files.

Logic review (head):

  • Glide loop: one rAF chain, ease = max(0.6px, 26% of gap, gap−36px lag ceiling), snap at gap>600px or gap−step<1, parks after 10 quiet frames, refuses on the exact settle-path refusal set + finger-down + jump-to-question owner. Single writer at a time: _cancelBottomSettle() and _settleMessageScrollToBottom() both call _tailFollowStop(), so settle and glide can never interleave frames.
  • Drain handoff: _streamFadeDrainingUntil is a self-expiring deadline (400ms, re-marked on every active step), not a boolean — an abandoned drain (session switch/cancel) expires it automatically, so scrollIfPinned() falls back to the full settle (pre-fix behavior) at worst. All exits clear it explicitly (null assistantBody, done, forced-done).
  • The setTimeout(33)+rAF → rAF-poll pacing in _scheduleRender/drain is behavior-preserving for the fade math (time-based, 33/66ms interval with a 4ms tolerance) and strictly smooths the release cadence; the typeof requestAnimationFrame fallback keeps non-browser contexts working.

Two non-blocking nits: (1) pump() in messages.js calls bare requestAnimationFrame without the typeof guard its sibling branch has — browser-only path so harmless, but the guard would make it consistent; (2) the glide reads scrollHeight/clientHeight every frame — intentional and documented (one read, one write), fine for a transcript this size.

Clean, well-tested fix for a real UX stutter. Approve.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L Large PR (>10 files or >250 LOC) ux User experience / visual polish

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants