diff --git a/static/ui.js b/static/ui.js index 352caf47..cb65ae87 100644 --- a/static/ui.js +++ b/static/ui.js @@ -953,6 +953,12 @@ function _captureMessageViewportAnchor(){ key:row&&row.dataset?String(row.dataset.messageAnchorKey||''):'', topOffset:rect.top-containerRect.top, topPadBefore, + // Snapshot the scroll geometry at capture so a later realign can detect that + // content grew ABOVE the viewport between capture and restore — the streaming + // case where the anchor's topOffset is stale and realigning to it would yank a + // still reader backward (issue #5637). + scrollHeightAtCapture:container.scrollHeight, + scrollTopAtCapture:container.scrollTop, }; } } @@ -1029,6 +1035,28 @@ function _restoreMessageViewportAnchor(anchor, rawIdxDelta){ const containerRect=container.getBoundingClientRect(); const rect=row.getBoundingClientRect(); const targetTop=Number(anchor.topOffset)||0; + // Streaming stale-anchor guard (issue #5637). During a live stream, content grows + // ABOVE the viewport between anchor capture and this restore, so the anchor's + // captured topOffset is stale and the realign delta becomes a spurious few-hundred-px + // value that yanks a still reader backward. Detect it by content growth + absence of + // real input intent — NOT by a scrollTop diff, because on an overflow-anchor:auto + // container the browser itself moves scrollTop to compensate the growth (so a still + // reader's scrollTop is not stationary). _recentMessage*ScrollIntent reflects genuine + // touch/wheel/key input, which the browser's anchor layer never writes. If content + // grew since capture AND there is no recent input intent AND the realign would move + // scrollTop non-trivially, refuse it and let the browser overflow-anchor hold. An + // actively scrolling reader (recent intent) keeps the legitimate realign; legacy + // snapshots without the captured geometry keep prior behavior. + const _realignDelta=(rect.top-containerRect.top)-targetTop; + const _shAtCap=Number(anchor.scrollHeightAtCapture); + if(Number.isFinite(_shAtCap)){ + const _grewAbove=(container.scrollHeight-_shAtCap)>4; + const _activeIntent=(typeof _recentMessageScrollIntent==='function' && _recentMessageScrollIntent()) + || (typeof _recentMessageTouchScrollIntent==='function' && _recentMessageTouchScrollIntent()); + if(_grewAbove&&!_activeIntent&&Math.abs(_realignDelta)>8){ + return false; + } + } _programmaticScroll=true;_programmaticScrollSetAt=performance.now(); // Mobile-only jump fix: the resting overflow-anchor on .messages is `auto` on // touch devices (CSS media query keeps it `none` only for hover+fine-pointer @@ -13259,6 +13287,28 @@ function _restoreMessageScrollSnapshotSameFrame(snapshot){ const target=(snapshot.pinned===true&&Number.isFinite(bottom)) ? maxTop-Math.max(0,bottom) : Number(snapshot.top)||0; + // Streaming stale-snapshot guard (issue #5637). The userUnpinned check above is + // defeated when a live stream re-pins the state machine (a scrollHeight-collapse + // scroll event flips userUnpinned back to false even though the reader is up in + // history), so this absolute snapshot.top write still fires and yanks a still + // reader — snapshot.top was captured before the streaming chunk grew above-viewport + // height, so it is stale. Mirror the realign guard: if content grew since the + // snapshot AND there is no recent real input intent AND the write would move + // scrollTop non-trivially, refuse it and let the browser overflow-anchor hold. + // Pinned tail-followers (target is bottom-relative, not snapshot.top) are + // unaffected; an actively scrolling reader has intent and keeps the restore. + const _snapSH=Number(snapshot.scrollHeight); + const _grewSinceSnap=Number.isFinite(_snapSH)&&_snapSH>0&&(el.scrollHeight-_snapSH)>4; + const _fbActiveIntent=(typeof _recentMessageScrollIntent==='function' && _recentMessageScrollIntent()) + || (typeof _recentMessageTouchScrollIntent==='function' && _recentMessageTouchScrollIntent()); + if(snapshot.pinned!==true && _grewSinceSnap && !_fbActiveIntent + && Math.abs((Math.max(0,Math.min(target,maxTop)))-el.scrollTop)>8){ + _lastScrollTop=el.scrollTop;_lastMessageClientHeight=el.clientHeight; + _messageUserUnpinned=true; + _scrollPinned=false; + _nearBottomCount=0; + return; + } _programmaticScroll=true;_programmaticScrollSetAt=performance.now(); el.scrollTop=Math.max(0,Math.min(target,maxTop)); } diff --git a/tests/test_issue5637_stale_anchor_guard.py b/tests/test_issue5637_stale_anchor_guard.py new file mode 100644 index 00000000..403cd661 --- /dev/null +++ b/tests/test_issue5637_stale_anchor_guard.py @@ -0,0 +1,219 @@ +"""Regression tests for the issue #5637 streaming stale-anchor guards. + +Once the reader is up in history during a live stream, the anchor captured for a +same-frame restore goes stale as the streaming chunk grows content ABOVE the viewport: +the anchor's captured topOffset (and the absolute snapshot.top) no longer map to the +same content, so realigning to them yanks a still reader backward by a few hundred px. +The existing `snapshot.userUnpinned===true` fallback skip is defeated because the +scrollHeight-collapse scroll event re-pins the state machine (flips userUnpinned back to +false) mid-stream. + +Two guards close this, both keyed on content-growth-since-capture + absence of real +input intent (NOT a scrollTop diff, which the browser's own overflow-anchor writes on an +overflow-anchor:auto container): + 1. `_restoreMessageViewportAnchor` refuses the realign write. + 2. the absolute `snapshot.top` fallback in `_restoreMessageScrollSnapshotSameFrame` + refuses its write. + +Every behavioral test below is designed to FAIL on the pre-guard code and PASS only with +the guard. Node-harness pattern (extractFunc + mock DOM) shared with the sibling scroll +regression suites. +""" +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" +NODE = shutil.which("node") + +pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH") + + +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 _extract_func_script(js: str) -> str: + prelude = "const src = " + json.dumps(js) + ";\n" + body = r""" +function extractFunc(name) { + const re = new RegExp('function\\s+' + name + '\\s*\\('); + const start = src.search(re); + if (start < 0) throw new Error(name + ' not found'); + let i = src.indexOf('{', start); + let depth = 1; i++; + let str = null, inLine = false, inBlock = false, inRegex = false, prev = ''; + while (depth > 0 && i < src.length) { + const c = src[i], n = src[i + 1]; + if (inLine) { if (c === '\n') inLine = false; i++; continue; } + if (inBlock) { if (c === '*' && n === '/') { inBlock = false; i++; } i++; continue; } + if (str) { if (c === '\\') { i += 2; continue; } if (c === str) str = null; i++; continue; } + if (inRegex) { if (c === '\\') { i += 2; continue; } if (c === '/') inRegex = false; i++; continue; } + if (c === '/' && n === '/') { inLine = true; i += 2; continue; } + if (c === '/' && n === '*') { inBlock = true; i += 2; continue; } + if (c === '"' || c === "'" || c === '`') { str = c; i++; continue; } + if (c === '/' && !'})]0123456789'.includes(prev) && !/[A-Za-z_$]/.test(prev)) { inRegex = true; i++; continue; } + if (c === '{') depth++; else if (c === '}') depth--; + if (c.trim()) prev = c; + i++; + } + return src.slice(start, i); +}""" + return prelude + body + + +# ---- realign guard (_restoreMessageViewportAnchor) ------------------------------- + +def _realign_harness(*, anchor_extra: str, cur_scroll_height: int, rect_top: int, + top_offset: int, active_intent: bool = False) -> str: + js = UI_JS_PATH.read_text(encoding="utf-8") + intent_js = "true" if active_intent else "false" + return _extract_func_script(js) + f""" +let writes = []; +let stTop = 90030; +const row = {{ getBoundingClientRect(){{ return {{ top: {rect_top} }}; }}, + getClientRects(){{ return [{{}}]; }}, dataset: {{ messageAnchorKey: 'k1' }} }}; +const container = {{ + get scrollTop(){{ return stTop; }}, set scrollTop(v){{ writes.push(Math.round(v)); stTop = v; }}, + scrollHeight: {cur_scroll_height}, clientHeight: 427, + getBoundingClientRect(){{ return {{ top: 0, bottom: 427 }}; }}, + querySelectorAll(){{ return [row]; }}, querySelector(){{ return row; }}, style: {{}}, +}}; +function $(id){{ return id === 'messages' ? container : null; }} +function _recentMessageScrollIntent(){{ return {intent_js}; }} +function _recentMessageTouchScrollIntent(){{ return {intent_js}; }} +let _programmaticScroll = false; let _programmaticScrollSetAt = 0; +const performance = {{ now(){{ return 1; }} }}; +function _suppressBrowserOverflowAnchor(){{ return null; }} +function _deferClearProgrammaticScroll(){{}} +function requestAnimationFrame(cb){{ cb(); }} +function setTimeout(cb){{ cb(); return 1; }} +const anchor = Object.assign({{ rawIdx: 53, sessionIdx: 53, key: 'k1', topOffset: {top_offset} }}, {anchor_extra}); +eval(extractFunc('_restoreMessageViewportAnchor')); +const returned = _restoreMessageViewportAnchor(anchor, 0); +console.log(JSON.stringify({{ returned, wrote: writes.length }})); +""" + + +def test_realign_refuses_stale_anchor_when_content_grew_no_intent(): + """Content grew since capture (90000->90453) and no input intent, realign delta -453 + -> REFUSE (return false, no write). Mutation: delete the guard block and it writes.""" + m = json.loads(_run_node(_realign_harness( + anchor_extra="{scrollHeightAtCapture: 90000}", + cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False, + ))) + assert m["returned"] is False and m["wrote"] == 0 + + +def test_realign_allows_fresh_anchor_no_growth(): + """No growth since capture -> realign runs normally even with the same delta.""" + m = json.loads(_run_node(_realign_harness( + anchor_extra="{scrollHeightAtCapture: 90453}", + cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False, + ))) + assert m["returned"] is True and m["wrote"] == 1 + + +def test_realign_allows_with_active_intent(): + """Recent real input intent -> keep the legitimate realign even if content grew.""" + m = json.loads(_run_node(_realign_harness( + anchor_extra="{scrollHeightAtCapture: 90000}", + cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=True, + ))) + assert m["returned"] is True and m["wrote"] == 1 + + +def test_realign_backward_compatible_without_capture_geometry(): + """Legacy anchor without scrollHeightAtCapture -> guard is a no-op, prior behavior.""" + m = json.loads(_run_node(_realign_harness( + anchor_extra="{}", + cur_scroll_height=90453, rect_top=-453, top_offset=0, active_intent=False, + ))) + assert m["returned"] is True and m["wrote"] == 1 + + +# ---- fallback guard (_restoreMessageScrollSnapshotSameFrame) --------------------- + +def _fallback_harness(*, snapshot_scroll_height, cur_scroll_height, snapshot_top, + active_intent: bool = False) -> str: + js = UI_JS_PATH.read_text(encoding="utf-8") + intent_js = "true" if active_intent else "false" + sh = "null" if snapshot_scroll_height is None else str(snapshot_scroll_height) + return _extract_func_script(js) + f""" +let writes = []; +let stTop = 90030; +const el = {{ + get scrollTop(){{ return stTop; }}, set scrollTop(v){{ writes.push(Math.round(v)); stTop = v; }}, + scrollHeight: {cur_scroll_height}, clientHeight: 427, +}}; +function $(id){{ return id === 'messages' ? el : null; }} +function _recentMessageScrollIntent(){{ return {intent_js}; }} +function _recentMessageTouchScrollIntent(){{ return {intent_js}; }} +// realign path fails (no anchor) so execution reaches the absolute fallback +function _restorePinnedMessageScrollSnapshot(){{ return false; }} +function _restoreMessageViewportAnchor(){{ return false; }} +function _remountMessageViewportAnchor(){{ return false; }} +let _messageUserUnpinned = false; let _scrollPinned = true; let _nearBottomCount = 5; +let _lastScrollTop = 0; let _lastMessageClientHeight = 0; +let _programmaticScroll = false; let _programmaticScrollSetAt = 0; +const performance = {{ now(){{ return 1; }} }}; +function _deferClearProgrammaticScroll(){{}} +function requestAnimationFrame(cb){{ cb(); }} +function setTimeout(cb){{ cb(); return 1; }} +const snapshot = {{ anchor: null, top: {snapshot_top}, bottom: 40, + scrollHeight: {sh}, pinned: false, userUnpinned: false }}; +eval(extractFunc('_restoreMessageScrollSnapshotSameFrame')); +_restoreMessageScrollSnapshotSameFrame(snapshot); +console.log(JSON.stringify({{ wrote: writes.length, writes, + messageUserUnpinned: _messageUserUnpinned, scrollPinned: _scrollPinned }})); +""" + + +def test_fallback_refuses_stale_snapshot_top_when_content_grew_no_intent(): + """Content grew since snapshot (90000->90453), reader not pinned, no intent, and the + absolute snapshot.top write would move scrollTop >8px -> REFUSE. This is the -578 + on-device jump the userUnpinned check missed (re-pin flipped userUnpinned false). + Mutation: delete the fallback guard block and scrollHistory gets the stale write.""" + m = json.loads(_run_node(_fallback_harness( + snapshot_scroll_height=90000, cur_scroll_height=90453, snapshot_top=89577, + active_intent=False, + ))) + assert m["writes"] == [] + assert m["messageUserUnpinned"] is True and m["scrollPinned"] is False + + +def test_fallback_allows_snapshot_top_when_no_growth(): + """No growth since snapshot -> the fallback keeps its authoritative absolute restore.""" + m = json.loads(_run_node(_fallback_harness( + snapshot_scroll_height=90453, cur_scroll_height=90453, snapshot_top=89577, + active_intent=False, + ))) + assert m["writes"] == [89577] + + +def test_fallback_backward_compatible_without_capture_scrollheight(): + """Legacy snapshot without scrollHeight -> guard no-op, prior absolute restore runs.""" + m = json.loads(_run_node(_fallback_harness( + snapshot_scroll_height=None, cur_scroll_height=90453, snapshot_top=89577, + active_intent=False, + ))) + assert m["writes"] == [89577]