diff --git a/CHANGELOG.md b/CHANGELOG.md index c9b49141..2ce75a01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,10 @@ ## [Unreleased] +### Fixed + +- **No more mid-stream scroll jitter while following a streaming reply.** When you were pinned to the bottom of a live-streaming answer, every mid-stream re-render (tool completion, activity-scene refresh, clarify echo) caused a brief ~1-row up-and-back bounce, so a long streaming reply visibly shuddered. The tail is now re-anchored to the settled bottom in a microtask after the re-render (after layout flushes, before paint), so the short intermediate never reaches the screen. Only a reader already following the tail is affected — an unpinned reader parked up in history is never moved. Thanks @allenliang2022. (#5685) + ### Documentation - **Documented the reverse-proxy basic-auth caveat for installed PWAs.** README, onboarding, and troubleshooting now explain that proxy basic auth can block the same-origin service-worker/shell update fetches an installed PWA needs (leaving it on a blank screen after an update), and give recovery steps — prefer WebUI's built-in password, or scope proxy auth so `sw.js`/manifest/shell requests complete. Thanks @rodboev. (#5673, #2781) diff --git a/static/ui.js b/static/ui.js index 352caf47..d99fc4c7 100644 --- a/static/ui.js +++ b/static/ui.js @@ -13402,6 +13402,36 @@ function _assistantTurnAnchorSettledFinalAnswer(message, content, context){ return null; } } +// Re-anchor a pinned/tail-following reader to the settled bottom after a full +// renderMessages() rebuild, eliminating the one-frame mid-stream jitter. MUST be scheduled +// in a MICROTASK from the end of renderMessages (see the queueMicrotask call site), NOT run +// synchronously. Why: the mid-stream re-render bug is that renderMessages wipes #msgInner +// then rebuilds, and the pinned tail-follow path (scrollIfPinned → scrollToBottom) writes +// scrollTop while still INSIDE the render sync stack, where the browser reports a TRANSIENT +// scrollHeight a few px short of the settled value (layout is batched — every geometry read +// inside the sync stack returns the mid-settle height). So scrollToBottom lands scrollTop a +// little HIGH (short of the true tail); that intermediate is painted this frame and the +// settle rAF corrects it the next frame → a fast ~1-row back-and-forth bounce (~82px). A +// microtask runs AFTER the sync stack unwinds (layout has flushed, so scrollHeight/clientHeight +// are the settled values) but BEFORE the browser paints — so writing the now-correct settled +// max here lands the tail exactly and the short intermediate never reaches the screen. Only +// fires for a pre-wipe tail-follower left short of the settled max, so an unpinned reader +// parked in history is never moved (orthogonal to the unpinned jump-back class). The +// _programmaticScroll latch (armed at the wipe) keeps the scroll listener from misreading +// this write as a manual unpin. Idempotent: a no-op once scrollTop already equals the max. +function _reanchorPinnedTailAfterRender(wasNearTail){ + if(!wasNearTail) return; + const el=$('messages'); + if(!el) return; + const settledMax=Math.max(0, el.scrollHeight-el.clientHeight); + if(el.scrollTop < settledMax-1){ + _programmaticScroll=true;_programmaticScrollSetAt=performance.now(); + el.scrollTop=settledMax; + _lastScrollTop=el.scrollTop;_lastMessageClientHeight=el.clientHeight; + _nearBottomCount=2; + _scrollPinned=true; + } +} function _scrollAfterMessageRender(preserveScroll, scrollSnapshot){ // Terminal stream renders can happen after S.activeStreamId is cleared. // In that case, preserveScroll asks the normal pin-state helper to decide: @@ -13595,6 +13625,20 @@ function renderMessages(options){ // Mobile scroll-jank fix: temporarily disable overflow-anchor so Chromium // cannot re-anchor to the topmost row during the DOM wipe-and-rebuild gap. if(window._fixMobileScrollJank) window._fixMobileScrollJank(); + // Capture whether the reader was at/near the tail BEFORE the wipe. A tail-follower + // hit by a mid-stream re-render gets a one-frame jitter: the wipe+rebuild lands the + // sync scrollTop write against a transient layout whose above-viewport height is a + // few px short of the settled value, so the browser clamps scrollTop a little high; + // the settle rAF corrects it the next frame, producing a fast ~1-row back-and-forth + // bounce. We remember the pre-wipe near-tail state here (geometry, not closure pin + // flags — the wipe's clamp scroll event can transiently perturb those) so the tail + // of renderMessages can re-anchor to the settled bottom before the intermediate is + // painted. See _reanchorPinnedTailAfterRender + its queueMicrotask call site. + const _preWipeNearTail=(()=>{ + const _m=$('messages'); + if(!_m) return false; + return (_m.scrollHeight-_m.scrollTop-_m.clientHeight)<=8; + })(); // The DOM wipe can briefly collapse #msgInner to zero height, causing the // browser to clamp #messages.scrollTop to 0 and emit a scroll event. That // event is a render artifact, not user intent; if the scroll listener sees it @@ -14929,6 +14973,20 @@ function renderMessages(options){ } } _updateMessageVirtualMeasurements(renderVisWithIdx, renderVisibleIdxs, virtualWindow); + // Kill the pinned/tail-follower mid-stream jitter. Schedule the re-anchor in a MICROTASK, + // not synchronously: inside this render sync stack the browser still reports a transient + // scrollHeight (layout is batched), so a synchronous re-anchor would read the SAME short + // value scrollToBottom already clamped against and be a no-op. The microtask runs after the + // stack unwinds (scrollHeight has flushed to the settled value) but before the browser + // paints this frame, so writing the settled max lands the tail exactly and the ~1-row high + // intermediate never reaches the screen. Only re-anchors a pre-wipe tail-follower left short + // of the settled max — an unpinned reader parked in history is never moved (orthogonal to + // the unpinned jump-back class). See _reanchorPinnedTailAfterRender for the full rationale. + // (typeof guards mirror the _deferClearProgrammaticScroll call below so standalone + // renderMessages() test harnesses that don't define these helpers still run.) + if(typeof queueMicrotask==='function' && typeof _reanchorPinnedTailAfterRender==='function'){ + queueMicrotask(()=>_reanchorPinnedTailAfterRender(_preWipeNearTail)); + } _recycleStash.clear(); if(typeof _deferClearProgrammaticScroll==='function') _deferClearProgrammaticScroll(160); } diff --git a/tests/test_pinned_tail_midstream_jitter.py b/tests/test_pinned_tail_midstream_jitter.py new file mode 100644 index 00000000..99904a84 --- /dev/null +++ b/tests/test_pinned_tail_midstream_jitter.py @@ -0,0 +1,232 @@ +"""Regression tests for the pinned/tail-follower mid-stream scroll JITTER (a fast +~1-row back-and-forth bounce), distinct from the unpinned jump-back class. + +Root cause (reproduced on desktop Chromium with a per-frame scrollTop flight +recorder + an isolated CSSOM A/B): when a mid-stream re-render fires (tool +completion, activity-scene refresh, clarify echo), renderMessages() wipes +#msgInner (`inner.innerHTML=''`) then rebuilds and, inside the SAME synchronous +stack, the pinned/tail-follow path (scrollIfPinned -> scrollToBottom -> +_setMessageScrollToBottom) writes scrollTop against a TRANSIENT layout whose +above-viewport height is a few px short of the settled value. The browser clamps +scrollTop a little HIGH (short of the true max); that intermediate is PAINTED this +frame, and the settle rAF corrects it the next frame -> a visible fast bounce of +~1 row (measured ~82px on a real session). scrollHeight/max are back to their +settled values by the end of the renderMessages sync stack, so re-anchoring to the +settled max THERE lands exactly, before the intermediate is painted. + +Fix: capture the near-tail state BEFORE the wipe (geometry, not closure pin flags, +which the wipe's clamp scroll event can transiently perturb); at the very end of +renderMessages call `_reanchorPinnedTailAfterRender(preWipeNearTail)`, which -- only +for a pre-wipe tail-follower still sitting short of the settled max -- snaps scrollTop +to the pre-computed settled max synchronously (writing the max, NOT scrollHeight, +which would reflow to the same transient short value and re-clamp). An unpinned reader +parked in history is never moved (orthogonal to the unpinned jump-back class). + +Each behavioral test FAILS on the known-buggy version (guard disabled) and PASSES +only on the fixed version. A pure-string structural test locks the call site + the +"write settled max, not scrollHeight" invariant. +""" +import json +import pathlib +import shutil +import subprocess +import tempfile + +import pytest + +ROOT = pathlib.Path(__file__).parent.parent +UI_JS_PATH = ROOT / "static" / "ui.js" +UI_JS = UI_JS_PATH.read_text(encoding="utf-8") +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +def _function_body(src: str, name: str) -> str: + start = src.index(f"function {name}") + brace = src.index("{", start) + depth = 0 + for i in range(brace, len(src)): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + if depth == 0: + return src[start : i + 1] + raise AssertionError(f"function {name} body not found") + + +def _run_node(source: str) -> str: + with tempfile.NamedTemporaryFile( + "w", suffix=".cjs", encoding="utf-8", dir=ROOT, delete=False + ) as script: + script.write(source) + script_path = pathlib.Path(script.name) + try: + result = subprocess.run( + [NODE, str(script_path)], + cwd=str(ROOT), + capture_output=True, + text=True, + timeout=30, + ) + finally: + script_path.unlink(missing_ok=True) + if result.returncode != 0: + raise RuntimeError(result.stderr) + return result.stdout.strip() + + +def _harness(helper_body: str, scenario_js: str, mutate=None) -> str: + """Run the REAL extracted _reanchorPinnedTailAfterRender body against a fake + #messages scroller. The scenario sets the pre-wipe near-tail flag + the current + (clamped) scrollTop and settled geometry, then prints the resulting scrollTop and + the pin-state bookkeeping the helper writes. + """ + body = helper_body + if mutate: + old, new = mutate + assert old in body, "mutation target not found in extracted helper" + body = body.replace(old, new) + return ( + r""" +'use strict'; +// ---- fake #messages scroller: settled layout (scrollHeight/clientHeight are the +// POST-render settled values, matching the end of the renderMessages sync stack). ---- +const el = { scrollTop: 0, scrollHeight: 9235, clientHeight: 626 }; +function $(id){ return id === 'messages' ? el : null; } +// ---- module-level state the helper closes over ---- +let _programmaticScroll = false; +let _programmaticScrollSetAt = 0; +let _lastScrollTop = 0; +let _lastMessageClientHeight = 0; +let _nearBottomCount = 0; +let _scrollPinned = false; +const performance = { now: () => 0 }; +""" + + body + + "\n" + + scenario_js + ) + + +HELPER = _function_body(UI_JS, "_reanchorPinnedTailAfterRender") + + +def test_pinned_tailfollower_clamped_short_is_reanchored_to_settled_max(): + """The exact jitter event: a pre-wipe tail-follower whose scrollTop the browser + left clamped SHORT of the settled max (8527 vs settled max 9235-626=8609). The + fixed helper must snap scrollTop to the settled max (8609) synchronously, so the + short intermediate is never painted. On the buggy version scrollTop stays 8527. + """ + scenario = r""" +el.scrollTop = 8527; // browser clamped a bit high during the transient-layout write +_reanchorPinnedTailAfterRender(true); // reader WAS at/near tail before the wipe +console.log(JSON.stringify({ top: el.scrollTop, prog: _programmaticScroll, pinned: _scrollPinned })); +""" + m = json.loads(_run_node(_harness(HELPER, scenario))) + assert m["top"] == 8609, ( + "a pre-wipe tail-follower left clamped short (8527) must be re-anchored to the " + f"settled max (8609); got {m['top']}. On the buggy version it stays at 8527, " + "which is the painted intermediate that produces the ~1-row bounce." + ) + assert m["prog"] is True, ( + "the re-anchor write must arm _programmaticScroll so the scroll listener does " + "not misread it as a manual unpin" + ) + assert m["pinned"] is True, "re-anchoring a tail-follower must keep it pinned" + + +def test_unpinned_reader_not_near_tail_is_never_moved(): + """An unpinned reader parked in history (wasNearTail=false) must NOT be moved -- + this is the orthogonality guarantee vs the unpinned jump-back class. Even though scrollTop is far below + the settled max, the helper must leave it exactly where it is. + """ + scenario = r""" +el.scrollTop = 3232; // reader parked mid-history +_reanchorPinnedTailAfterRender(false); // was NOT near tail before the wipe +console.log(JSON.stringify({ top: el.scrollTop, prog: _programmaticScroll })); +""" + m = json.loads(_run_node(_harness(HELPER, scenario))) + assert m["top"] == 3232, ( + "a reader who was not a tail-follower before the wipe must never be snapped " + f"to the bottom; got {m['top']}" + ) + assert m["prog"] is False, "no scroll write should happen for a non-tail reader" + + +def test_already_at_settled_bottom_is_idempotent_noop(): + """When scrollTop already equals the settled max, the helper must be a no-op (no + redundant write / no _programmaticScroll churn) -- guards against re-arming the + latch every render on a steady tail-follower. + """ + scenario = r""" +el.scrollTop = 8609; // already at settled max (9235 - 626) +_reanchorPinnedTailAfterRender(true); +console.log(JSON.stringify({ top: el.scrollTop, prog: _programmaticScroll })); +""" + m = json.loads(_run_node(_harness(HELPER, scenario))) + assert m["top"] == 8609 + assert m["prog"] is False, ( + "already at the settled max: the helper must not re-write scrollTop or arm the " + "programmatic latch (idempotent no-op)" + ) + + +def test_mutation_disabling_the_guard_fails_the_reanchor(): + """MUTATION CHECK: reverting the fix (make the short-clamp branch dead) must break + the re-anchor. Proves test_pinned_...reanchored actually exercises the fix and is + not vacuously green. + """ + scenario = r""" +el.scrollTop = 8527; +_reanchorPinnedTailAfterRender(true); +console.log(JSON.stringify({ top: el.scrollTop })); +""" + # Disable the guard body: the branch that performs the re-anchor never runs. + mutated = _harness( + HELPER, scenario, mutate=("if(el.scrollTop < settledMax-1){", "if(false){") + ) + m = json.loads(_run_node(mutated)) + assert m["top"] == 8527, ( + "with the guard disabled the reader must stay at the clamped-short 8527 -- if " + "this still reported 8609 the behavioral test would be vacuous" + ) + + +def test_source_locks_call_site_and_settled_max_write(): + """Structural locks so a future refactor can't silently drop the fix or regress it + to (a) a SYNCHRONOUS call (which reads the transient short height inside the render + stack and no-ops) or (b) writing scrollHeight (which reflows to the transient short + value and re-clamps). + """ + # The pre-wipe near-tail capture exists and is taken before the wipe. + assert "const _preWipeNearTail=(()=>{" in UI_JS + # renderMessages schedules the re-anchor in a MICROTASK (NOT synchronously): the + # microtask runs after the render sync stack flushes layout to the settled height but + # before paint. A bare synchronous call reads the mid-settle height and is a no-op. + assert "queueMicrotask(()=>_reanchorPinnedTailAfterRender(_preWipeNearTail));" in UI_JS + # The call is typeof-guarded so a standalone renderMessages() node harness (which does + # not define queueMicrotask / _reanchorPinnedTailAfterRender) does not throw a + # ReferenceError when it evals the extracted renderMessages body. + assert ( + "if(typeof queueMicrotask==='function' && " + "typeof _reanchorPinnedTailAfterRender==='function'){" in UI_JS + ), ( + "the queueMicrotask re-anchor must be typeof-guarded (mirroring the " + "_deferClearProgrammaticScroll guard) so harnesses that eval renderMessages " + "without these helpers defined don't ReferenceError" + ) + # Guard against regressing to a synchronous call. + assert "\n _reanchorPinnedTailAfterRender(_preWipeNearTail);" not in UI_JS, ( + "the re-anchor must be scheduled via queueMicrotask, not called synchronously " + "at the end of renderMessages (a sync call reads the transient short height)" + ) + # The helper writes the settled max (read fresh in the microtask), NOT scrollHeight. + helper = _function_body(UI_JS, "_reanchorPinnedTailAfterRender") + assert "const settledMax=Math.max(0, el.scrollHeight-el.clientHeight);" in helper + assert "el.scrollTop=settledMax;" in helper + assert "el.scrollTop=el.scrollHeight" not in helper, ( + "must write the pre-computed settledMax, not scrollHeight (re-reading " + "scrollHeight reflows to the transient short value and re-clamps)" + )