fix(ui): guard all bare _autoScrollFollow reads (#6614)
This commit is contained in:
1 parent
f4f9157f8d
commit
6dd1fed8d3
3 files changed
+125
-7
No files matched your search
+13
-5
@@ -5664,6 +5664,14 @@ window.addEventListener('resize',function(){
|
||||
// Any manual scroll up sets a sticky unpinned flag until the user scrolls back
|
||||
// to the bottom (near-bottom hysteresis on downward motion) or clicks ↓.
|
||||
// Programmatic scrolls are ignored via _programmaticScroll. Fixes #1469 / #1360 / #1731.
|
||||
// #6606 ownership: ui.js/messages.js load before boot.js, and boot.js only
|
||||
// assigns window._autoScrollFollow after its awaited settings request. Every
|
||||
// consumer in this file (scroll listener, settle paths, scrollIfPinned,
|
||||
// DOM-replace gate) can run before that assignment — a bare read would throw
|
||||
// ReferenceError. Establish the default synchronously here (single owner);
|
||||
// boot.js overwrites it with the saved setting after hydration. The typeof
|
||||
// guard preserves an explicit saved `false` if boot.js already ran.
|
||||
if(typeof window._autoScrollFollow==='undefined'){ window._autoScrollFollow=true; }
|
||||
let _scrollPinned=true;
|
||||
let _programmaticScroll=false;
|
||||
let _programmaticScrollSetAt=0;
|
||||
@@ -6104,7 +6112,7 @@ if(typeof window!=='undefined'){
|
||||
if(nearBottom){
|
||||
_nearBottomCount=_nearBottomCount+1;
|
||||
if(_nearBottomCount>=2){_scrollPinned=true;_nearBottomCount=0;}
|
||||
}else if(!movedUp && (typeof _autoScrollFollow==='undefined'||_autoScrollFollow) && _scrollPinned){
|
||||
}else if(!movedUp && window._autoScrollFollow && _scrollPinned){
|
||||
// Content-grew-beneath-a-pinned-viewport case (NOT a user scroll-away).
|
||||
// During streaming on a tall transcript (esp. mobile, where chunks land
|
||||
// fast), new content increases scrollHeight under a stationary viewport,
|
||||
@@ -6779,7 +6787,7 @@ function _shouldFollowMessagesOnDomReplace(){
|
||||
// following only for users who are still pinned or effectively at the tail.
|
||||
// A broad near-bottom window causes long answers/mobile readers who scroll up
|
||||
// a little to read mid-stream to get snapped back to the bottom on completion.
|
||||
return (typeof _autoScrollFollow==='undefined'||_autoScrollFollow) && !_messageUserUnpinned && (_scrollPinned || _isMessagePaneNearBottom(120));
|
||||
return window._autoScrollFollow && !_messageUserUnpinned && (_scrollPinned || _isMessagePaneNearBottom(120));
|
||||
}
|
||||
function _followMessagesAfterDomReplace(){
|
||||
if(_shouldFollowMessagesOnDomReplace()){
|
||||
@@ -6829,7 +6837,7 @@ function _settleMessageScrollToBottom(force, explicit){
|
||||
// active one that may now be in the global _settleRO. (Codex review #3.)
|
||||
const ro=new ResizeObserver(()=>{
|
||||
if(token!==_bottomSettleToken){ ro.disconnect(); if(_settleRO===ro) _settleRO=null; return; }
|
||||
if((!_autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()){
|
||||
if((!window._autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()){
|
||||
ro.disconnect(); if(_settleRO===ro) _settleRO=null;
|
||||
_programmaticScroll=false;
|
||||
return;
|
||||
@@ -6868,7 +6876,7 @@ function _settleMessageScrollToBottom(force, explicit){
|
||||
_settleFinalTimer=setTimeout(()=>{
|
||||
if(token!==_bottomSettleToken) return;
|
||||
ro.disconnect(); if(_settleRO===ro) _settleRO=null;
|
||||
if((!_autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()){ _programmaticScroll=false; return; }
|
||||
if((!window._autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()){ _programmaticScroll=false; return; }
|
||||
_settleFinalScroll(token);
|
||||
},2000);
|
||||
}
|
||||
@@ -6889,7 +6897,7 @@ function _settleFinalScroll(token){
|
||||
_deferClearProgrammaticScroll();
|
||||
}
|
||||
function scrollIfPinned(){
|
||||
if(!_autoScrollFollow) return;
|
||||
if(!window._autoScrollFollow) return;
|
||||
if(_messageUserUnpinned){
|
||||
// Only scrollToBottom() cleared this flag, so one scroll-up permanently
|
||||
// killed auto-follow. Re-pin ONLY when the reader has genuinely returned to
|
||||
|
||||
@@ -7,8 +7,14 @@ future edit can't silently flip it or, worse, default it ON in config.py while
|
||||
hydrating it OFF in the browser (the classic default-mismatch bug, where an
|
||||
existing user with no saved value sees the feature as disabled).
|
||||
"""
|
||||
import json
|
||||
import pathlib
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
import textwrap
|
||||
|
||||
import pytest
|
||||
|
||||
ROOT = pathlib.Path(__file__).resolve().parent.parent
|
||||
|
||||
@@ -72,3 +78,107 @@ def test_follow_gate_references_auto_scroll_follow_and_unpin():
|
||||
assert "_autoScrollFollow" in src and "_messageUserUnpinned" in src, (
|
||||
"the follow gate must reference both _autoScrollFollow and _messageUserUnpinned"
|
||||
)
|
||||
|
||||
|
||||
def test_ui_sync_init_defaults_auto_scroll_follow_before_consumers():
|
||||
"""ui.js must establish window._autoScrollFollow synchronously at the
|
||||
top-level scroll-state section — before boot.js hydrates it from the awaited
|
||||
settings request. ui.js/messages.js load before boot.js (index.html), so any
|
||||
scroll listener / settle path / scrollIfPinned / DOM-replace gate running in
|
||||
that window reads an undeclared binding and throws ReferenceError (#6606).
|
||||
|
||||
The typeof guard makes the default `true` WITHOUT clobbering an explicit
|
||||
saved `false` if boot.js already hydrated it."""
|
||||
src = _read("static/ui.js")
|
||||
assert "if(typeof window._autoScrollFollow==='undefined'){ window._autoScrollFollow=true; }" in src, (
|
||||
"ui.js must synchronously default window._autoScrollFollow to true before "
|
||||
"boot.js hydration (bare reads otherwise throw ReferenceError pre-boot)"
|
||||
)
|
||||
# The sync init must sit BEFORE every consumer it protects in the file.
|
||||
init_idx = src.index("window._autoScrollFollow=true;")
|
||||
scroll_listener_idx = src.index("!movedUp && window._autoScrollFollow && _scrollPinned")
|
||||
gate_idx = src.index("window._autoScrollFollow && !_messageUserUnpinned")
|
||||
settle_idx = src.index("!window._autoScrollFollow&&!explicit")
|
||||
pinned_idx = src.index("function scrollIfPinned")
|
||||
for label, idx in (
|
||||
("scroll listener", scroll_listener_idx),
|
||||
("DOM-replace gate", gate_idx),
|
||||
("settle guard", settle_idx),
|
||||
("scrollIfPinned", pinned_idx),
|
||||
):
|
||||
assert init_idx < idx, f"sync init must precede the {label} consumer"
|
||||
|
||||
|
||||
def test_ui_readers_use_explicit_window_owner():
|
||||
"""Every _autoScrollFollow reader in ui.js must go through the explicit
|
||||
window._autoScrollFollow owner — no bare undeclared reads that can throw
|
||||
ReferenceError before boot.js assigns the global (#6606)."""
|
||||
src = _read("static/ui.js")
|
||||
# Any bare `_autoScrollFollow` token not preceded by `window.` is a bug.
|
||||
for m in re.finditer(r"(?<![\w.])_autoScrollFollow", src):
|
||||
before = src[max(0, m.start() - 7):m.start()]
|
||||
assert before == "window.", (
|
||||
f"bare _autoScrollFollow read at ui.js offset {m.start()}: "
|
||||
f"...{src[max(0, m.start()-40):m.start()+20]!r}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.skipif(shutil.which("node") is None, reason="node required for behavioral test")
|
||||
def test_auto_scroll_follow_undefined_true_false_behavior():
|
||||
"""Behavioral: extract the real follow expressions from ui.js and evaluate
|
||||
them in Node across undefined / true / false:
|
||||
|
||||
- undefined must NOT throw (property read, not a bare undeclared binding)
|
||||
and must not block an explicit bottom settlement;
|
||||
- false must suppress automatic following (DOM-replace gate, settle guard,
|
||||
scrollIfPinned) WITHOUT suppressing explicit bottom settlement;
|
||||
- true keeps following enabled."""
|
||||
src = _read("static/ui.js")
|
||||
# Extract the exact live expressions (not hand-copied replicas).
|
||||
dom_replace_expr = "window._autoScrollFollow && !_messageUserUnpinned && (_scrollPinned || _isMessagePaneNearBottom(120))"
|
||||
assert dom_replace_expr in src, "DOM-replace follow gate expression changed"
|
||||
settle_expr = "(!window._autoScrollFollow&&!explicit)||!_scrollPinned||_messageUserUnpinned||_recentNonMessageScrollIntent()"
|
||||
assert settle_expr in src, "settle guard expression changed"
|
||||
pinned_guard_expr = "!window._autoScrollFollow"
|
||||
assert f"if({pinned_guard_expr}) return;" in src, "scrollIfPinned guard changed"
|
||||
|
||||
def run(expr, follow, explicit=False, pinned=True, unpinned=False, near=True):
|
||||
follow_js = "undefined" if follow is None else ("true" if follow else "false")
|
||||
harness = textwrap.dedent(f"""
|
||||
const window = {{ _autoScrollFollow: {follow_js} }};
|
||||
let _scrollPinned = {str(pinned).lower()};
|
||||
let _messageUserUnpinned = {str(unpinned).lower()};
|
||||
let explicit = {str(explicit).lower()};
|
||||
function _isMessagePaneNearBottom(px){{ return {str(near).lower()}; }}
|
||||
function _recentNonMessageScrollIntent(){{ return false; }}
|
||||
let _out;
|
||||
try {{
|
||||
_out = ({expr});
|
||||
}} catch (e) {{
|
||||
console.error('THREW: ' + e);
|
||||
process.exit(2);
|
||||
}}
|
||||
console.log(JSON.stringify(_out === undefined ? '__UNDEF__' : _out));
|
||||
""")
|
||||
res = subprocess.run(["node", "-e", harness], capture_output=True, text=True, timeout=30)
|
||||
assert res.returncode == 0, res.stderr
|
||||
val = json.loads(res.stdout.strip())
|
||||
return None if val == "__UNDEF__" else val
|
||||
|
||||
# undefined: property read → falsy, never throws, explicit settle still allowed.
|
||||
assert not run(dom_replace_expr, None) # undefined && … → undefined (falsy)
|
||||
assert run(settle_expr, None) is True # suppress implicit settle
|
||||
assert run(settle_expr, None, explicit=True) is False # explicit settle proceeds
|
||||
assert run(pinned_guard_expr, None) is True # scrollIfPinned returns early
|
||||
# true: follow enabled everywhere.
|
||||
assert run(dom_replace_expr, True) is True
|
||||
assert run(settle_expr, True) is False # implicit settle proceeds
|
||||
assert run(pinned_guard_expr, True) is False # scrollIfPinned does NOT early-return
|
||||
# false: automatic follow suppressed…
|
||||
assert run(dom_replace_expr, False) is False
|
||||
assert run(settle_expr, False) is True
|
||||
assert run(pinned_guard_expr, False) is True
|
||||
# …but an EXPLICIT bottom settlement still runs (false must not block ↓/open).
|
||||
assert run(settle_expr, False, explicit=True) is False
|
||||
# Unpinned reader is never re-followed, regardless of the setting.
|
||||
assert run(dom_replace_expr, True, unpinned=True) is False
|
||||
@@ -50,11 +50,11 @@ def test_content_grew_keeps_pin_and_resnaps_not_unpin():
|
||||
# did NOT move up and auto-follow is on, keep the pin and re-snap to bottom
|
||||
# rather than unpinning. Assert the new guard + re-snap call are present.
|
||||
c = _compact(UI_JS)
|
||||
assert _compact("}else if(!movedUp && _autoScrollFollow && _scrollPinned){") in c, (
|
||||
assert _compact("}else if(!movedUp && window._autoScrollFollow && _scrollPinned){") in c, (
|
||||
"content-grew-beneath-pinned guard missing from the scroll listener"
|
||||
)
|
||||
# The guard body must re-snap to bottom, not set _scrollPinned=false.
|
||||
idx = c.find(_compact("else if(!movedUp && _autoScrollFollow && _scrollPinned){"))
|
||||
idx = c.find(_compact("else if(!movedUp && window._autoScrollFollow && _scrollPinned){"))
|
||||
assert idx != -1
|
||||
# The re-snap call must be the FIRST scroll action after the guard opens,
|
||||
# before the chain reaches any `else{ _scrollPinned=false }` fallthrough.
|
||||
|
||||
Reference in new issue
Block a user