From f7dd4f41ba55a89f17e242466dbf13e4b1604fc9 Mon Sep 17 00:00:00 2001 From: allenliang2022 Date: Mon, 6 Jul 2026 20:32:52 +0800 Subject: [PATCH 1/3] fix(webui): restore unpinned reader after mid-stream wipe-and-rebuild scroll clamp renderMessages() rebuilds the transcript via inner.innerHTML='' then re-appends rows. That wipe collapses #msgInner scrollHeight toward the empty-table height, so the browser is forced to clamp #messages.scrollTop down to the new near-zero max -- a browser primitive (no JS writes scrollTop; overflow-anchor does not govern it). When this rebuild happens mid-stream while a reader has scrolled up into history (_messageUserUnpinned), _scrollAfterMessageRender took the S.activeStreamId branch and called scrollIfPinned(), which is a no-op for an unpinned reader -- so the clamped-to-0 scrollTop was never restored and the reader was stranded at the top. Fix: in that branch, when the reader is unpinned and a pre-wipe scroll snapshot exists (renderMessages already captures one whenever _messageUserUnpinned), restore the snapshot instead of the no-op. Pinned / tail-following readers keep scrollIfPinned() so live-follow is unchanged. Reuses the existing _restoreMessageScrollSnapshot helper (same one the preserveScroll and idle-unpin branches use), so no new core-timing risk. Tests: test_wipe_rows0_streaming_unpinned_restore.py executes the real extracted _scrollAfterMessageRender against a fake #messages + instrumented restore stubs and asserts the unpinned+activeStreamId+snapshot case restores (scrollTop returns to the pre-clamp value) while pinned / no-snapshot cases fall back to scrollIfPinned(); mutation-checked (reverting to the bare scrollIfPinned() fails the behavioral asserts). Updates the test_issue1690 source-lock for the new branch structure. --- static/ui.js | 15 ++ tests/test_issue1690_scroll_completion.py | 8 +- ...t_wipe_rows0_streaming_unpinned_restore.py | 211 ++++++++++++++++++ 3 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 tests/test_wipe_rows0_streaming_unpinned_restore.py diff --git a/static/ui.js b/static/ui.js index 352caf47..a84673a2 100644 --- a/static/ui.js +++ b/static/ui.js @@ -13427,6 +13427,21 @@ function _scrollAfterMessageRender(preserveScroll, scrollSnapshot){ return; } if(S.activeStreamId){ + // Mid-stream re-render (tool completion, activity-scene refresh, clarify echo). + // renderMessages() wipes #msgInner (inner.innerHTML='') then rebuilds; that wipe + // collapses scrollHeight toward the empty-table height, and the browser is FORCED + // to clamp #messages.scrollTop down to the new (near-zero) max. For a reader who + // scrolled UP into history (unpinned), scrollIfPinned() is a no-op — so it does NOT + // undo that clamp, and the reader is stranded at the top (the scroll jump-back). The + // wipe-to-empty clamp is a browser primitive (device-agnostic; JS never writes the + // scrollTop), so the passive no-op cannot preserve position here. renderMessages() + // captured a pre-wipe snapshot for exactly this case (its scrollSnapshot init fires + // when _messageUserUnpinned), so restore the unpinned reader's viewport instead of + // the no-op. Pinned/tail-following readers keep scrollIfPinned() (correct live-follow). + if(_messageUserUnpinned && scrollSnapshot){ + _restoreMessageScrollSnapshot(scrollSnapshot); + return; + } scrollIfPinned(); return; } diff --git a/tests/test_issue1690_scroll_completion.py b/tests/test_issue1690_scroll_completion.py index ffacaa83..44073847 100644 --- a/tests/test_issue1690_scroll_completion.py +++ b/tests/test_issue1690_scroll_completion.py @@ -66,7 +66,13 @@ def test_render_messages_preserve_scroll_option_uses_user_pin_state_not_stream_l assert "if(!readerAwayFromBottom && !_messageUserUnpinned && _followMessagesAfterDomReplace()) return;\n _restoreMessageScrollSnapshot(scrollSnapshot);\n _maybeShowNewMessageScrollCue(scrollSnapshot);\n return;\n }" in scroll_helper assert "_shouldFollowMessagesOnDomReplace()" in follow_helper assert "scrollToBottom();" in follow_helper - assert "if(S.activeStreamId){\n scrollIfPinned();\n return;\n }" in scroll_helper + # Mid-stream re-render branch (issue: wipe-rows0 scrollHeight-collapse jump-back). + # An unpinned reader (scrolled up into history) must have their pre-wipe viewport + # RESTORED — scrollIfPinned() is a no-op for the unpinned case and cannot undo the + # browser's scrollTop clamp from the inner.innerHTML='' wipe, so it stranded the + # reader at the top. Pinned/tail-following readers still take scrollIfPinned(). + assert "if(S.activeStreamId){" in scroll_helper + assert "if(_messageUserUnpinned && scrollSnapshot){\n _restoreMessageScrollSnapshot(scrollSnapshot);\n return;\n }\n scrollIfPinned();\n return;\n }" in scroll_helper def test_cached_render_path_uses_same_scroll_policy_as_fresh_render(): diff --git a/tests/test_wipe_rows0_streaming_unpinned_restore.py b/tests/test_wipe_rows0_streaming_unpinned_restore.py new file mode 100644 index 00000000..cb4e9a2a --- /dev/null +++ b/tests/test_wipe_rows0_streaming_unpinned_restore.py @@ -0,0 +1,211 @@ +"""Regression tests for the mobile/desktop scroll jump-back caused by the +renderMessages wipe-and-rebuild collapsing #msgInner to zero rows mid-stream +(the "wipe-rows0" scrollHeight-collapse -> browser scrollTop CLAMP class). + +Root cause (reproduced on desktop Chromium AND matching real-device telemetry): +renderMessages() rebuilds the transcript by `inner.innerHTML=''` then re-appending +rows. That wipe collapses #msgInner to the empty-table height, so the browser is +FORCED to clamp #messages.scrollTop down to the new (near-zero) maximum -- a +browser primitive, no JS writes the scrollTop (telemetry tags it JS=none, and it +is unaffected by overflow-anchor). When this rebuild happens while a reader has +scrolled UP into history (`_messageUserUnpinned`) during an active stream, the +post-render helper `_scrollAfterMessageRender` took the `S.activeStreamId` branch +and called `scrollIfPinned()`, which is a NO-OP for an unpinned reader -- so the +clamped scrollTop was never restored and the reader was stranded at the top. + +Fix: in the `S.activeStreamId` branch, when the reader is unpinned and a pre-wipe +scroll snapshot exists (renderMessages captures one whenever _messageUserUnpinned), +restore the snapshot instead of the no-op. Pinned / tail-following readers keep +scrollIfPinned() so live-follow is unchanged. + +Each behavioral test is written to FAIL on the known-buggy version (bare +`scrollIfPinned()` with no unpinned-restore) and PASS only on the fixed version. +The pure-string structural test locks the branch order so a future refactor can't +silently drop the restore ahead of the fallback. +""" +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(scroll_helper_body: str, scenario_js: str) -> str: + """Run the REAL extracted _scrollAfterMessageRender body against a fake DOM + + instrumented stubs, so we observe WHICH restore path fires. The stubs record + their calls; the scenario sets the state (unpinned / activeStreamId / snapshot) + and prints the recorded calls + resulting scrollTop. + """ + return ( + r""" +'use strict'; +// ---- recorded side effects ---- +const calls = []; +// ---- fake #messages scroller ---- +const el = { scrollTop: 0, scrollHeight: 600, clientHeight: 400 }; +function $(id){ return id === 'messages' ? el : null; } +// ---- module-level state the helper closes over ---- +let _messageUserUnpinned = false; +let _scrollPinned = true; +let _programmaticScroll = false; +let _programmaticScrollSetAt = 0; +let _lastScrollTop = 0; +let _lastMessageClientHeight = 0; +let _nearBottomCount = 0; +const S = { activeStreamId: null }; +function performanceNow(){ return 0; } +const performance = { now: performanceNow }; +// ---- instrumented stubs for the two restore paths ---- +function scrollIfPinned(){ calls.push('scrollIfPinned'); } +function scrollToBottom(){ calls.push('scrollToBottom'); } +function _followMessagesAfterDomReplace(){ calls.push('follow'); return true; } +function _maybeShowNewMessageScrollCue(){ } +// _restoreMessageScrollSnapshot is what the FIX must call for an unpinned reader. +// Model its real effect: it moves scrollTop back toward the captured snapshot.top. +function _restoreMessageScrollSnapshot(snap){ + calls.push('restoreSnapshot'); + if(snap && typeof snap.top === 'number'){ el.scrollTop = snap.top; } +} +""" + + scroll_helper_body + + "\n" + + scenario_js + ) + + +SCROLL_HELPER = _function_body(UI_JS, "_scrollAfterMessageRender") + + +def test_streaming_unpinned_reader_is_restored_not_left_to_scrollifpinned_noop(): + """The exact bug event: mid-stream re-render (S.activeStreamId set) while the + reader is unpinned, with a pre-wipe snapshot captured. The browser has already + clamped scrollTop to 0 (modelled). The fixed helper must RESTORE the snapshot + (moving scrollTop back to 3000) and must NOT fall through to the scrollIfPinned + no-op. On the buggy version only 'scrollIfPinned' is recorded and scrollTop + stays clamped at 0. + """ + scenario = r""" +S.activeStreamId = 'stream-1'; +_messageUserUnpinned = true; +_scrollPinned = false; +// Browser already clamped the wipe: reader was at 3000, now stranded at 0. +el.scrollTop = 0; +const snapshot = { top: 3000, userUnpinned: true, pinned: false, bottom: 5200 }; +_scrollAfterMessageRender(false, snapshot); +console.log(JSON.stringify({ calls, finalTop: el.scrollTop })); +""" + m = json.loads(_run_node(_harness(SCROLL_HELPER, scenario))) + assert "restoreSnapshot" in m["calls"], ( + "an unpinned reader during an active-stream re-render must have their " + f"pre-wipe viewport restored; recorded calls={m['calls']!r}. On the buggy " + "version this is 'scrollIfPinned' only (a no-op), leaving the reader " + "stranded at the clamped top." + ) + assert m["finalTop"] == 3000, ( + "after restore, scrollTop must return to the reader's pre-wipe position " + f"(3000), not stay at the browser-clamped 0; got {m['finalTop']}" + ) + + +def test_streaming_pinned_reader_still_follows_via_scrollifpinned(): + """A pinned / tail-following reader (NOT unpinned) during an active stream must + STILL take scrollIfPinned() -- the fix must not divert the live-follow path. + """ + scenario = r""" +S.activeStreamId = 'stream-1'; +_messageUserUnpinned = false; // pinned / following the tail +_scrollPinned = true; +el.scrollTop = 200; +const snapshot = { top: 200, userUnpinned: false, pinned: true, bottom: 0 }; +_scrollAfterMessageRender(false, snapshot); +console.log(JSON.stringify({ calls, finalTop: el.scrollTop })); +""" + m = json.loads(_run_node(_harness(SCROLL_HELPER, scenario))) + assert m["calls"] == ["scrollIfPinned"], ( + "a pinned reader during an active stream must follow via scrollIfPinned() " + f"only; recorded calls={m['calls']!r}" + ) + assert "restoreSnapshot" not in m["calls"], ( + "the fix must NOT restore a stale snapshot for a pinned/tail reader" + ) + + +def test_streaming_unpinned_without_snapshot_falls_back_to_scrollifpinned(): + """Defensive: if no snapshot was captured (e.g. a caller that did not preserve + scroll), the unpinned-restore is skipped and the branch falls back to the + existing scrollIfPinned() behavior rather than throwing. + """ + scenario = r""" +S.activeStreamId = 'stream-1'; +_messageUserUnpinned = true; +_scrollPinned = false; +el.scrollTop = 0; +_scrollAfterMessageRender(false, null); +console.log(JSON.stringify({ calls, finalTop: el.scrollTop })); +""" + m = json.loads(_run_node(_harness(SCROLL_HELPER, scenario))) + assert m["calls"] == ["scrollIfPinned"], ( + "with no snapshot the branch must fall back to scrollIfPinned(); " + f"recorded calls={m['calls']!r}" + ) + + +def test_source_locks_unpinned_restore_precedes_scrollifpinned_fallback(): + """Structural lock: inside the S.activeStreamId branch the unpinned-restore + guard must appear BEFORE the scrollIfPinned() fallback, or the fallback would + shadow it and re-introduce the no-op strand. + """ + helper = SCROLL_HELPER + stream_idx = helper.index("if(S.activeStreamId){") + branch = helper[stream_idx:] + restore_idx = branch.index("_restoreMessageScrollSnapshot(scrollSnapshot);") + fallback_idx = branch.index("scrollIfPinned();") + guard_idx = branch.index("if(_messageUserUnpinned && scrollSnapshot){") + assert guard_idx < restore_idx < fallback_idx, ( + "the unpinned-restore guard + restore call must precede the scrollIfPinned() " + "fallback inside the S.activeStreamId branch" + ) From d31a345b174c1ec65359e89290675e6ac73b0dcd Mon Sep 17 00:00:00 2001 From: allenliang2022 Date: Mon, 6 Jul 2026 20:42:58 +0800 Subject: [PATCH 2/3] fix(webui): show new-message cue after unpinned mid-stream scroll restore Address greptile review: the new S.activeStreamId restore branch omitted the _maybeShowNewMessageScrollCue call that the preserveScroll and idle-unpin restore branches both make. An unpinned reader whose position is restored mid-stream should still see the new-message cue, so add it for consistency. Update the behavioral test to assert the cue fires and the test_issue1690 source-lock for the new branch body. --- static/ui.js | 1 + tests/test_issue1690_scroll_completion.py | 2 +- tests/test_wipe_rows0_streaming_unpinned_restore.py | 7 ++++++- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/static/ui.js b/static/ui.js index a84673a2..1801ce02 100644 --- a/static/ui.js +++ b/static/ui.js @@ -13440,6 +13440,7 @@ function _scrollAfterMessageRender(preserveScroll, scrollSnapshot){ // the no-op. Pinned/tail-following readers keep scrollIfPinned() (correct live-follow). if(_messageUserUnpinned && scrollSnapshot){ _restoreMessageScrollSnapshot(scrollSnapshot); + _maybeShowNewMessageScrollCue(scrollSnapshot); return; } scrollIfPinned(); diff --git a/tests/test_issue1690_scroll_completion.py b/tests/test_issue1690_scroll_completion.py index 44073847..f7940e4a 100644 --- a/tests/test_issue1690_scroll_completion.py +++ b/tests/test_issue1690_scroll_completion.py @@ -72,7 +72,7 @@ def test_render_messages_preserve_scroll_option_uses_user_pin_state_not_stream_l # browser's scrollTop clamp from the inner.innerHTML='' wipe, so it stranded the # reader at the top. Pinned/tail-following readers still take scrollIfPinned(). assert "if(S.activeStreamId){" in scroll_helper - assert "if(_messageUserUnpinned && scrollSnapshot){\n _restoreMessageScrollSnapshot(scrollSnapshot);\n return;\n }\n scrollIfPinned();\n return;\n }" in scroll_helper + assert "if(_messageUserUnpinned && scrollSnapshot){\n _restoreMessageScrollSnapshot(scrollSnapshot);\n _maybeShowNewMessageScrollCue(scrollSnapshot);\n return;\n }\n scrollIfPinned();\n return;\n }" in scroll_helper def test_cached_render_path_uses_same_scroll_policy_as_fresh_render(): diff --git a/tests/test_wipe_rows0_streaming_unpinned_restore.py b/tests/test_wipe_rows0_streaming_unpinned_restore.py index cb4e9a2a..35aea51e 100644 --- a/tests/test_wipe_rows0_streaming_unpinned_restore.py +++ b/tests/test_wipe_rows0_streaming_unpinned_restore.py @@ -103,7 +103,7 @@ const performance = { now: performanceNow }; function scrollIfPinned(){ calls.push('scrollIfPinned'); } function scrollToBottom(){ calls.push('scrollToBottom'); } function _followMessagesAfterDomReplace(){ calls.push('follow'); return true; } -function _maybeShowNewMessageScrollCue(){ } +function _maybeShowNewMessageScrollCue(){ calls.push('newMessageCue'); } // _restoreMessageScrollSnapshot is what the FIX must call for an unpinned reader. // Model its real effect: it moves scrollTop back toward the captured snapshot.top. function _restoreMessageScrollSnapshot(snap){ @@ -149,6 +149,11 @@ console.log(JSON.stringify({ calls, finalTop: el.scrollTop })); "after restore, scrollTop must return to the reader's pre-wipe position " f"(3000), not stay at the browser-clamped 0; got {m['finalTop']}" ) + assert "newMessageCue" in m["calls"], ( + "after restoring an unpinned reader mid-stream, the new-message cue must be " + "shown (consistent with the preserveScroll and idle-unpin restore branches); " + f"recorded calls={m['calls']!r}" + ) def test_streaming_pinned_reader_still_follows_via_scrollifpinned(): From db231464c8ee3b6a5e748ed7b4feed8f52cf121f Mon Sep 17 00:00:00 2001 From: t Date: Mon, 6 Jul 2026 17:37:15 +0000 Subject: [PATCH 3/3] docs(changelog): stamp #5681 unpinned-reader scroll-jump fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ce75a01..05046214 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed +- **Scrolling up into history during a live stream no longer throws you to the top.** While a reply was streaming, a mid-stream re-render (tool completion, activity-scene refresh, clarify echo) wipes and rebuilds the transcript, momentarily collapsing its height so the browser clamps the scroll position to the top. For a reader who had scrolled up into history, that stranded them at the top (a jump of thousands of pixels). The pre-wipe viewport is now restored for an unpinned reader instead of being left clamped; readers following the tail are unaffected. Thanks @allenliang2022. (#5681) - **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