From c65ae46983215f8234ce6d9585e475c064b62c3d Mon Sep 17 00:00:00 2001 From: ai-ag2026 <261867348+ai-ag2026@users.noreply.github.com> Date: Fri, 8 May 2026 20:02:05 +0200 Subject: [PATCH] fix: prevent chat scroll resets after final render Keep explicit bottom pins stable across late layout growth and make clicking the already-active sidebar session a no-op before loadSession mutates state. Update scroll regression tests for the delayed settle path. --- static/sessions.js | 6 +- static/ui.js | 47 ++++++++++- tests/test_issue1731_upward_scroll_unpins.py | 10 ++- tests/test_streaming_sidebar_scroll.py | 8 +- tests/test_tars_scroll_reset_regressions.py | 86 ++++++++++++++++++++ 5 files changed, 147 insertions(+), 10 deletions(-) create mode 100644 tests/test_tars_scroll_reset_regressions.py diff --git a/static/sessions.js b/static/sessions.js index e0b29990..8c4d2ce5 100644 --- a/static/sessions.js +++ b/static/sessions.js @@ -330,6 +330,11 @@ async function newSession(flash){ } async function loadSession(sid){ + const currentSid = S.session ? S.session.session_id : null; + // Clicking the already-open session in the sidebar is a no-op. Reloading it + // tears down active pane state and can reset the long-session scroll window + // to the top even though the user did not navigate anywhere. + if(currentSid===sid) return; // Mark this session as the in-flight load. Subsequent loadSession() calls // will overwrite this; stale awaits use the mismatch to bail out (#1060). _loadingSessionId = sid; @@ -339,7 +344,6 @@ async function loadSession(sid){ if(typeof hideClarifyCard==='function') hideClarifyCard(); // Show loading indicator immediately for responsiveness. // Cleared by renderMessages() once full session data arrives. - const currentSid = S.session ? S.session.session_id : null; // Persist the current composer draft before switching away so it can be // restored when the user switches back (#1060). if (currentSid && currentSid !== sid) { diff --git a/static/ui.js b/static/ui.js index 6ba7346b..3e95e1b0 100644 --- a/static/ui.js +++ b/static/ui.js @@ -1510,8 +1510,10 @@ let _lastScrollTop=null; let _lastNonMessageScrollIntentMs=0; let _lastMessageUpwardIntentMs=0; let _messageUserUnpinned=false; +let _bottomSettleToken=0; const NON_MESSAGE_SCROLL_INTENT_SUPPRESS_MS=350; const MESSAGE_UPWARD_INTENT_MS=450; +function _cancelBottomSettle(){ _bottomSettleToken++; } function _recordNonMessageScrollIntent(e){ const el=document.getElementById('messages'); const target=e&&e.target; @@ -1527,6 +1529,9 @@ function _recordNonMessageScrollIntent(e){ // not masquerade as user intent and strand live streaming away from bottom. _lastMessageUpwardIntentMs=performance.now(); _messageUserUnpinned=true; + // User is intentionally moving in the transcript. Cancel any delayed + // scrollToBottom settling that was scheduled by session-load/layout growth. + _cancelBottomSettle(); _nearBottomCount=0; _scrollPinned=false; } @@ -1559,8 +1564,14 @@ if (typeof window !== 'undefined') window._resetScrollDirectionTracker = _resetS // scrollToBottomBtn visibility is updated below after pin state settles. const movedUp=_lastScrollTop!==null && top<_lastScrollTop-2 && _recentMessageUpwardIntent(); _lastScrollTop=top; - if(movedUp){ _nearBottomCount=0; _scrollPinned=false; _messageUserUnpinned=true; } // #1731 - else { _nearBottomCount=nearBottom?_nearBottomCount+1:0; _scrollPinned=_nearBottomCount>=2; if(_scrollPinned) _messageUserUnpinned=false; } // #1360 + if(movedUp){ _cancelBottomSettle(); _nearBottomCount=0; _scrollPinned=false; _messageUserUnpinned=true; } // #1731 + else { + if(nearBottom){ + _nearBottomCount=_nearBottomCount+1; + if(_nearBottomCount>=2) _scrollPinned=true; + } else { _nearBottomCount=0; _scrollPinned=false; } + if(_scrollPinned) _messageUserUnpinned=false; + } // #1360 const btn=$('scrollToBottomBtn'); if(btn) btn.style.display=_scrollPinned?'none':'flex'; // Load older messages when scrolled near the top @@ -1852,6 +1863,8 @@ function _setMessageScrollToBottom(){ _programmaticScroll=true; el.scrollTop=el.scrollHeight; _lastScrollTop=el.scrollTop; + _nearBottomCount=2; + _scrollPinned=true; requestAnimationFrame(()=>{ setTimeout(()=>{_programmaticScroll=false;},0); }); } function _isMessagePaneNearBottom(threshold=250){ @@ -1862,16 +1875,42 @@ function _isMessagePaneNearBottom(threshold=250){ function _shouldFollowMessagesOnDomReplace(){ return !_messageUserUnpinned && (_scrollPinned || _isMessagePaneNearBottom(1200)); } +function _settleMessageScrollToBottom(force){ + // Markdown post-processing (Prism, tables, Mermaid/KaTeX/PDF placeholders) + // can grow the transcript after the first scroll write. Re-apply the bottom + // position across a few frames while pinned so late layout does not leave the + // viewport a few lines above the real end. User scroll increments + // _bottomSettleToken and cancels the delayed passes. + const token=++_bottomSettleToken; + const passes=[0,16,80,180]; + passes.forEach(delay=>setTimeout(()=>{ + if(token!==_bottomSettleToken) return; + if(!force && (!_scrollPinned||_recentNonMessageScrollIntent())) return; + _setMessageScrollToBottom(); + },delay)); + requestAnimationFrame(()=>{ + if(token!==_bottomSettleToken) return; + if(force || (_scrollPinned&&!_recentNonMessageScrollIntent())) _setMessageScrollToBottom(); + requestAnimationFrame(()=>{ + if(token!==_bottomSettleToken) return; + if(force || (_scrollPinned&&!_recentNonMessageScrollIntent())) _setMessageScrollToBottom(); + }); + }); +} function scrollIfPinned(){ if(!_scrollPinned) return; if(_recentNonMessageScrollIntent()) return; - const el=$('messages'); - if(el){_programmaticScroll=true;el.scrollTop=el.scrollHeight;_lastScrollTop=el.scrollTop;requestAnimationFrame(()=>{ setTimeout(()=>{_programmaticScroll=false;},0); });} + _settleMessageScrollToBottom(false); } function scrollToBottom(){ _scrollPinned=true; _messageUserUnpinned=false; + // Write the first bottom position synchronously. A final renderMessages() + // rebuild can queue a native scroll event from the temporary scrollTop=0 + // layout state; if we only schedule delayed settles, that event can cancel + // them before the viewport ever reaches the bottom. _setMessageScrollToBottom(); + _settleMessageScrollToBottom(true); const btn=$('scrollToBottomBtn'); if(btn) btn.style.display='none'; } diff --git a/tests/test_issue1731_upward_scroll_unpins.py b/tests/test_issue1731_upward_scroll_unpins.py index e387e5c2..c8bfc862 100644 --- a/tests/test_issue1731_upward_scroll_unpins.py +++ b/tests/test_issue1731_upward_scroll_unpins.py @@ -142,13 +142,17 @@ def test_downward_path_preserves_macos_momentum_hysteresis(): end_idx = block.index("const btn=", else_idx) downward_branch = block[else_idx:end_idx] - assert "_nearBottomCount=nearBottom?_nearBottomCount+1:0" in downward_branch, ( + assert "if(nearBottom)" in downward_branch, ( + "Downward path must branch on near-bottom state so the macOS momentum " + "re-pin guard still applies (#1360)." + ) + assert "_nearBottomCount=_nearBottomCount+1" in downward_branch, ( "Downward path must keep incrementing the near-bottom counter so " "the macOS momentum re-pin guard still applies (#1360)." ) - assert "_scrollPinned=_nearBottomCount>=2" in downward_branch, ( + assert "if(_nearBottomCount>=2) _scrollPinned=true" in downward_branch, ( "Downward path must keep the >=2 hysteresis re-pin requirement " - "(#1360)." + "without downgrading an explicit bottom pin on the first near-bottom event (#1360)." ) diff --git a/tests/test_streaming_sidebar_scroll.py b/tests/test_streaming_sidebar_scroll.py index 60aa9339..f4dcecec 100644 --- a/tests/test_streaming_sidebar_scroll.py +++ b/tests/test_streaming_sidebar_scroll.py @@ -38,8 +38,12 @@ def test_scroll_if_pinned_skips_during_recent_non_message_scroll(): fn = _extract_fn(UI_JS, "scrollIfPinned") assert "_recentNonMessageScrollIntent()" in fn guard_index = fn.find("_recentNonMessageScrollIntent()") - write_index = fn.find("scrollTop=el.scrollHeight") - assert guard_index >= 0 and write_index >= 0 and guard_index < write_index + settle_index = fn.find("_settleMessageScrollToBottom(false)") + assert guard_index >= 0 and settle_index >= 0 and guard_index < settle_index + + settle = _extract_fn(UI_JS, "_settleMessageScrollToBottom") + assert "_setMessageScrollToBottom();" in settle + assert "_recentNonMessageScrollIntent()" in settle def test_session_list_has_its_own_scroll_boundary(): diff --git a/tests/test_tars_scroll_reset_regressions.py b/tests/test_tars_scroll_reset_regressions.py new file mode 100644 index 00000000..b05fa4e8 --- /dev/null +++ b/tests/test_tars_scroll_reset_regressions.py @@ -0,0 +1,86 @@ +from pathlib import Path + +REPO = Path(__file__).resolve().parents[1] +UI_JS = (REPO / "static" / "ui.js").read_text(encoding="utf-8") +SESSIONS_JS = (REPO / "static" / "sessions.js").read_text(encoding="utf-8") + + +def _function_body(src: str, signature: str) -> str: + start = src.index(signature) + 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 body not found: {signature}") + + +def _scroll_listener_block() -> str: + start = UI_JS.index("el.addEventListener('scroll'") + return UI_JS[start : UI_JS.index("})();", start)] + + +def test_clicking_current_session_is_noop_before_load_session_side_effects(): + load_session = _function_body(SESSIONS_JS, "async function loadSession") + + current_idx = load_session.index("const currentSid = S.session ? S.session.session_id : null") + noop_idx = load_session.index("if(currentSid===sid) return") + loading_idx = load_session.index("_loadingSessionId = sid") + stop_idx = load_session.index("stopApprovalPolling") + + assert current_idx < noop_idx < loading_idx < stop_idx, ( + "clicking the already-open sidebar row must be a no-op before loadSession() " + "mutates loading/runtime state or scroll-affecting UI" + ) + + +def test_scroll_to_bottom_settles_across_late_markdown_layout_growth(): + settle = _function_body(UI_JS, "function _settleMessageScrollToBottom") + scroll = _function_body(UI_JS, "function scrollToBottom") + pinned = _function_body(UI_JS, "function scrollIfPinned") + + assert "requestAnimationFrame" in settle + assert "setTimeout" in settle + assert "const passes=[0,16,80,180]" in settle + assert "_settleMessageScrollToBottom(true)" in scroll + assert "_settleMessageScrollToBottom(false)" in pinned + assert "!_scrollPinned" in settle + assert "const token=++_bottomSettleToken" in settle + assert "token!==_bottomSettleToken" in settle + + +def test_scroll_to_bottom_writes_scroll_position_immediately_before_delayed_settle(): + scroll = _function_body(UI_JS, "function scrollToBottom") + + immediate_idx = scroll.index("_setMessageScrollToBottom();") + settle_idx = scroll.index("_settleMessageScrollToBottom(true)") + + assert immediate_idx < settle_idx, ( + "scrollToBottom() must write scrollTop synchronously before scheduling delayed settles; " + "otherwise a DOM-rebuild scroll event can cancel the delayed passes and strand the viewport at the top" + ) + + +def test_message_scroll_listener_does_not_downgrade_explicit_bottom_pin_on_first_near_bottom_event(): + listener_block = _scroll_listener_block() + set_bottom = _function_body(UI_JS, "function _setMessageScrollToBottom") + + assert "_nearBottomCount=2" in set_bottom + assert "_scrollPinned=_nearBottomCount>=2" not in listener_block + assert "if(_nearBottomCount>=2) _scrollPinned=true" in listener_block + assert "else { _nearBottomCount=0; _scrollPinned=false; }" in listener_block + + +def test_user_scroll_cancels_delayed_bottom_settling(): + listener_block = _scroll_listener_block() + record = _function_body(UI_JS, "function _recordNonMessageScrollIntent") + + assert "function _cancelBottomSettle" in UI_JS + assert "_cancelBottomSettle();" in listener_block + assert "e.deltaY<0" in record + assert "_cancelBottomSettle();" in record + assert "_scrollPinned=false" in record