From 906aebefa9bdd03d9dc5bc42cc097cf52fea15fd Mon Sep 17 00:00:00 2001 From: Kevin Meinon <3797846+KevinMeinon@users.noreply.github.com> Date: Wed, 16 Sep 2026 16:03:36 +0200 Subject: [PATCH] fix(scroll): do not unpin a pinned tail reader on an above-tail collapse clamp (#7605) --- static/ui.js | 13 +- tests/test_issue1731_upward_scroll_unpins.py | 5 +- ...est_issue4856_android_scroll_regression.py | 2 +- ...test_scroll_collapse_clamp_keeps_follow.py | 240 ++++++++++++++++++ 4 files changed, 257 insertions(+), 3 deletions(-) create mode 100644 tests/test_scroll_collapse_clamp_keeps_follow.py diff --git a/static/ui.js b/static/ui.js index a174c632..4a631482 100644 --- a/static/ui.js +++ b/static/ui.js @@ -6457,7 +6457,18 @@ if(typeof window!=='undefined'){ return; } _lastScrollTop=top; - if(movedUp){ + if(movedUp&&bottomDistance>1){ + // Only a real scroll-away unpins. A collapse ABOVE the tail (worklog + // "Done" fold, thinking/tool card collapse, interim-note collapse) shrinks + // scrollHeight while the reader is still flush at the tail, so the browser + // clamps scrollTop DOWN by the collapsed height and fires a scroll event: + // movedUp is true while bottomDistance stays ~0. Reading that as user + // intent killed live-follow mid-stream on a reader who never scrolled. + // The render-artifact suppression below cannot cover it: it needs a + // renderMessages() within the last 1400ms, and the collapse paths above run + // from the streaming handlers, which update the DOM incrementally and never + // stamp _lastMessageRenderAt. A genuine upward scroll always leaves the true + // bottom first, so it still has bottomDistance>1 here. _cancelBottomSettle(); _nearBottomCount=0; _scrollPinned=false; diff --git a/tests/test_issue1731_upward_scroll_unpins.py b/tests/test_issue1731_upward_scroll_unpins.py index ffd62f83..8138a261 100644 --- a/tests/test_issue1731_upward_scroll_unpins.py +++ b/tests/test_issue1731_upward_scroll_unpins.py @@ -71,7 +71,10 @@ def test_scroll_listener_detects_upward_motion(): def test_upward_scroll_unpins_immediately_without_hysteresis(): """Upward motion sets _scrollPinned=false and resets the counter, no count needed.""" block = _scroll_listener_block() - if_idx = block.index("if(movedUp)") + # The upward branch is the clamped one: it only fires once the reader has left + # the true bottom (bottomDistance>1), so an above-tail collapse clamp cannot + # unpin a reader who is still flush at the tail. + if_idx = block.index("if(movedUp&&bottomDistance>1){") else_idx = block.find("else", if_idx) assert else_idx > if_idx, "upward / downward branches not found (#1731)" upward_branch = block[if_idx:else_idx] diff --git a/tests/test_issue4856_android_scroll_regression.py b/tests/test_issue4856_android_scroll_regression.py index 52a46a9e..3aa24d82 100644 --- a/tests/test_issue4856_android_scroll_regression.py +++ b/tests/test_issue4856_android_scroll_regression.py @@ -133,7 +133,7 @@ def test_recent_render_scroll_artifact_window_suppresses_upward_unpin(): "low-delta message-pane wheel intent so a gentle trackpad scroll-up is " "not swallowed." ) - assert listener.find("return;") < listener.find("if(movedUp){"), ( + assert listener.find("return;") < listener.find("if(movedUp&&bottomDistance>1){"), ( "recent render artifact scrolls must return before the movedUp branch " "can mark the reader unpinned." ) diff --git a/tests/test_scroll_collapse_clamp_keeps_follow.py b/tests/test_scroll_collapse_clamp_keeps_follow.py new file mode 100644 index 00000000..68b258d6 --- /dev/null +++ b/tests/test_scroll_collapse_clamp_keeps_follow.py @@ -0,0 +1,240 @@ +"""A collapse above the tail must not unpin a reader who is flush at the tail. + +Geometry: the reader is pinned, flush at the tail (bottomDistance ~= 0). A block +ABOVE the tail collapses - the settled worklog fold, a thinking/tool card +collapse, or the interim-note collapse. scrollHeight shrinks by the collapsed +height, the browser clamps scrollTop DOWN by the same amount, and a scroll event +fires with movedUp=true while bottomDistance is still ~0. + +The post-render artifact suppression does not cover this: it requires a +renderMessages() within the last 1400ms, and those collapse paths run from the +streaming handlers (closeCurrentLiveActivityGroup / interim collapse), which +patch the DOM incrementally and never stamp _lastMessageRenderAt. So the +upward-unpin branch itself has to require that the reader actually left the +bottom before it treats an upward delta as scroll-away intent. + +These tests drive the real rAF callback body extracted from static/ui.js in node +with controllable geometry, the same way the #4970 harness does. +""" + +import json # noqa: E402 +import shutil # noqa: E402 +import subprocess # noqa: E402 +from pathlib import Path # noqa: E402 + +import pytest # noqa: E402 + +REPO = Path(__file__).resolve().parents[1] +UI_JS = (REPO / "static" / "ui.js").read_text(encoding="utf-8") +NODE = shutil.which("node") + + +def _balanced_block(src: str, brace_start: int) -> str: + depth = 0 + for i in range(brace_start, len(src)): + ch = src[i] + if ch == "{": + depth += 1 + elif ch == "}": + depth -= 1 + if depth == 0: + return src[brace_start + 1 : i] + raise AssertionError("balanced block not found") + + +def _scroll_listener_raf_body() -> str: + listener_start = UI_JS.index("el.addEventListener('scroll'") + raf_start = UI_JS.index("requestAnimationFrame(()=>", listener_start) + brace_start = UI_JS.index("{", raf_start) + return _balanced_block(UI_JS, brace_start) + + +def _run_listener( + samples, + *, + render_artifact=False, + wheel_intent=False, + touch_intent=False, + key_scroll=False, + scrollbar_drag=False, + start_scroll_top=5703, + start_client_height=474, + auto_follow=True, +): + """Run the extracted scroll-listener rAF body over geometry samples.""" + payload = { + "body": _scroll_listener_raf_body(), + "samples": samples, + "renderArtifact": bool(render_artifact), + "wheelIntent": bool(wheel_intent), + "touchIntent": bool(touch_intent), + "keyScroll": bool(key_scroll), + "scrollbarDrag": bool(scrollbar_drag), + "startTop": start_scroll_top, + "startClientHeight": start_client_height, + "autoFollow": bool(auto_follow), + } + script = ( + "const payload = " + json.dumps(payload) + ";\n" + + r""" +global.window = { _autoScrollFollow: payload.autoFollow, _showThinking: true }; +const step = new Function( + 'el', + '_lastScrollTop', + '_lastMessageClientHeight', + '_nearBottomCount', + '_scrollPinned', + '_messageUserUnpinned', + '_newMessageCueVisible', + '_cancelBottomSettle', + '_setMessageScrollToBottom', + '_clearNewMessageScrollCue', + '_syncScrollToBottomCue', + '_updateSessionStartJumpButton', + '_isSessionEndlessScrollEnabled', + '_messagesTruncated', + '_loadOlderMessages', + '_recentMessageRenderArtifactWindow', + '_recentMessageTouchScrollIntent', + '_recentNonMessageScrollIntent', + '_recentMessageWheelIntent', + '_recentMessageKeyScrollIntent', + '_recentMessageScrollIntent', + '_scheduleDeferredOlderMessagesLoad', + '_scrollbarDragActive', + '(()=>{' + payload.body + `})(); +return { + _lastScrollTop, + _lastMessageClientHeight, + _nearBottomCount, + _scrollPinned, + _messageUserUnpinned, +}; +` +); + +let state = { + _lastScrollTop: payload.startTop, + _lastMessageClientHeight: payload.startClientHeight, + _nearBottomCount: 0, + _scrollPinned: true, + _messageUserUnpinned: false, +}; + +const noop = () => {}; +const yes = () => true; +const no = () => false; + +for (const sample of payload.samples) { + const el = { + scrollTop: sample.scrollTop, + scrollHeight: sample.scrollHeight, + clientHeight: sample.clientHeight, + }; + state = step( + el, + state._lastScrollTop, + state._lastMessageClientHeight, + state._nearBottomCount, + state._scrollPinned, + state._messageUserUnpinned, + false, + noop, + noop, + noop, + noop, + noop, + no, + false, + noop, + () => payload.renderArtifact, + () => payload.touchIntent, + no, + () => payload.wheelIntent, + () => payload.keyScroll, + no, + noop, + payload.scrollbarDrag + ); +} + +console.log(JSON.stringify(state)); +""" + ) + result = subprocess.run( + [NODE, "-e", script], + check=True, + capture_output=True, + text=True, + timeout=30, + ) + return json.loads(result.stdout.strip()) + + +# The clamp sample, verbatim from a headless-Chrome run against the real app +# (kanban t_f4eb293b): a 519px block above the tail collapses while the reader is +# flush at the bottom. scrollTop 5703 -> 5184, scrollHeight 6177 -> 5658, +# bottomDistance stays 0 in both samples. +_CLAMP_AFTER = {"scrollTop": 5184, "scrollHeight": 5658, "clientHeight": 474} +_CLAMP_BEFORE_TOP = 5703 +# A genuine scroll-away from the same start: the reader moves up and is no longer +# flush at the tail. +_SCROLL_AWAY = {"scrollTop": 5000, "scrollHeight": 6177, "clientHeight": 474} + + +@pytest.mark.skipif(NODE is None, reason="node not on PATH") +class TestCollapseClampKeepsFollow: + def test_above_tail_collapse_clamp_does_not_unpin(self): + state = _run_listener([_CLAMP_AFTER], start_scroll_top=_CLAMP_BEFORE_TOP) + assert state["_messageUserUnpinned"] is False, ( + "An above-tail collapse clamps scrollTop down while the reader is still " + "flush at the tail (bottomDistance 0). That is a layout clamp, not " + "scroll-away intent: it must not set the sticky manual-unpin flag, or " + "live-follow dies mid-stream for a reader who never scrolled." + ) + assert state["_scrollPinned"] is True + + def test_real_upward_scroll_still_unpins(self): + state = _run_listener( + [_SCROLL_AWAY], start_scroll_top=_CLAMP_BEFORE_TOP + ) + assert state["_messageUserUnpinned"] is True, ( + "A real upward scroll leaves the true bottom (bottomDistance>1) and must " + "still unpin immediately - #1731 behaviour is unchanged." + ) + assert state["_scrollPinned"] is False + + def test_clamp_during_open_artifact_window_stays_pinned(self): + # Same clamp geometry with a renderMessages() in the last 1400ms: that path + # was already covered before this fix and must keep behaving the same way. + state = _run_listener( + [_CLAMP_AFTER], + start_scroll_top=_CLAMP_BEFORE_TOP, + render_artifact=True, + ) + assert state["_messageUserUnpinned"] is False + assert state["_scrollPinned"] is True + + def test_upward_delta_just_above_the_bottom_still_unpins(self): + # Boundary: a few px off the true bottom (bottomDistance=4) is a real, if + # tiny, scroll-away and is still honoured; only the 0/1px clamp case is + # treated as a layout artifact. + state = _run_listener( + [{"scrollTop": 5699, "scrollHeight": 6177, "clientHeight": 474}], + start_scroll_top=_CLAMP_BEFORE_TOP, + ) + assert state["_messageUserUnpinned"] is True + assert state["_scrollPinned"] is False + + +def test_upward_unpin_branch_requires_leaving_the_bottom(): + # Source pin: the upward branch must not fire on a pure clamp. Keeps a future + # refactor from dropping the bottom-distance check while the behavioural tests + # above still exercise the real callback body. + listener_start = UI_JS.index("el.addEventListener('scroll'") + listener = UI_JS[listener_start : listener_start + 4000] + assert "if(movedUp&&bottomDistance>1){" in listener, ( + "The movedUp branch must require the reader to have left the true bottom " + "(bottomDistance>1) before it unpins; an above-tail collapse clamp fires " + "movedUp with bottomDistance~=0." + )