fix(chat): stop autoscrolling while a reply streams (owner direction)
TODO.md L5: "Get rid of the chat reply autoscroll, it's breaking things
like making it impossible for the user to scroll while a reply
generates." Owner direction 2026-08-27 (roadmap A1) revises the
phase-18 follow-the-bottom choice: the page NEVER auto-scrolls while a
turn streams. Kept (owner decision): the submit reveal (the user's own
message) and the one-shot phase-14 restore landing.
- frontend/assets/app.js: delete NEAR_BOTTOM_PX + isNearBottom;
scrollReveal becomes the one unconditional scrollIntoView (still
smooth, still "auto" under prefers-reduced-motion via SCROLL);
addMessage(who, html, scroll = false) carries an explicit scroll
intent — only the submit (", true") and the two restore landings
scroll. The thinking/tool/delta handlers and the typing indicator
drop their page-scroll calls; the thinking block's INTERNAL
bottom-pin (textEl.scrollTop, phase 17 — reworked separately in
phase 43) and the turn-end focus({ preventScroll: true }) survive.
- tests/unit/test_frontend_scroll.py: rewritten pin for the new
contract — phase-18 gate absent, helper unconditional, explicit
intent at submit/restore, no page-scroll call in the streaming
handlers, typing bubble scroll-free, SCROLL reduced-motion intact.
- tests/unit/test_chat_persistence.py: restore-landing pin updated to
the new signature (the old forced "auto" is gone; the landing
rides the default SCROLL — noted at the call site).
- tests/e2e/test_no_reply_autoscroll.py (new, replaces the deleted
test_follow_bottom_scroll.py): no autoscroll across >=10 samples
(1px tolerance) during a long answer and during the thinking stream;
submit-from-the-top still reveals the user message; the restore
landing lands one-shot on the latest message and stays; long answer
+ sources and the collapsed thinking block persist and restore.
E2E (isolation): test_no_reply_autoscroll.py 5/5; regressions
test_chat_rag 3/3, test_thinking_display 5/5,
test_chat_persistence 4/4, test_long_answers 2/2, test_smoke 3/3;
unit+integration 723 passed, app/ coverage 99%; ruff + pyright clean.
This commit is contained in:
@@ -85,10 +85,13 @@ def test_raw_text_only_stored_and_re_rendered_on_restore() -> None:
|
||||
on restore) — no HTML is ever stored. Restore re-applies the full
|
||||
brain-message chrome: is-deflected styling, maybe-try chips, sources."""
|
||||
js = _js()
|
||||
# Phase 18: restore landings are forced ("auto" + force) one-shot
|
||||
# scrollReveal calls — the only forced scrolls in the app.
|
||||
assert 'addMessage("user", renderMarkdown(m.text), "auto", true)' in js
|
||||
assert 'addMessage("brain", renderMarkdown(m.text), "auto", true)' in js
|
||||
# Phase 42 (owner direction 2026-08-27): the reply autoscroll is gone;
|
||||
# restore landings keep their one-shot load-time scroll via the
|
||||
# explicit intent (scroll=true) — the new addMessage signature has no
|
||||
# per-call behavior override (default SCROLL instead of forced
|
||||
# "auto" — documented at the call site).
|
||||
assert 'addMessage("user", renderMarkdown(m.text), true)' in js
|
||||
assert 'addMessage("brain", renderMarkdown(m.text), true)' in js
|
||||
assert "wrap.classList.add(\"is-deflected\")" in js
|
||||
assert "appendMaybeTry(wrap, m.suggestions)" in js
|
||||
assert "appendSources(wrap, m.sources)" in js
|
||||
|
||||
@@ -1,11 +1,21 @@
|
||||
"""Unit: the follow-the-bottom scroll contract in the static frontend
|
||||
(phase 18, owner choice 2026-08-23).
|
||||
"""Unit: the no-reply-autoscroll contract in the static frontend
|
||||
(phase 42, owner direction 2026-08-27, TODO.md L5).
|
||||
|
||||
The JS behavior itself is E2E-covered (tests/e2e/test_follow_bottom_scroll.py);
|
||||
here we pin the exported band constant and the single-gate markers that the
|
||||
story depends on — scrollIntoView appears exactly once in app.js, inside
|
||||
scrollReveal — so a silent regression back to unconditional per-delta /
|
||||
per-chunk scrolls is caught without a browser.
|
||||
The owner removed the phase-18 follow-the-bottom auto-follow: the page
|
||||
NEVER auto-scrolls while a turn streams (thinking / tool / delta frames
|
||||
all leave the viewport alone), so a user reading earlier content is no
|
||||
longer yanked down mid-answer. Scrolls happen only on explicit user
|
||||
intent: the submit (the user's own message is revealed) and the phase-14
|
||||
restore landing (one-shot, load-time).
|
||||
|
||||
The JS behavior itself is E2E-covered
|
||||
(tests/e2e/test_no_reply_autoscroll.py); here we pin the source markers
|
||||
of the new contract — the phase-18 gate is gone (no NEAR_BOTTOM_PX /
|
||||
isNearBottom), scrollReveal scrolls unconditionally and is the single
|
||||
scrollIntoView in app.js, addMessage takes an explicit `scroll` intent,
|
||||
and the streaming handlers contain no page-scroll call at all — so a
|
||||
silent regression back to per-frame autoscroll is caught without a
|
||||
browser.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
@@ -28,109 +38,140 @@ def _fn_body(js: str, name: str) -> str:
|
||||
return js[fn : js.find("\n}\n", fn)]
|
||||
|
||||
|
||||
def test_near_bottom_constant_exported_at_200px() -> None:
|
||||
"""The "pinned to the bottom" band (the composer zone) must be an
|
||||
*exported* constant — unit-pinned, same pattern as TURN_TIMEOUT_MS."""
|
||||
def test_phase18_gate_is_gone() -> None:
|
||||
"""The follow-the-bottom machinery (phase 18) is removed by owner
|
||||
direction 2026-08-27: no band constant, no gate function, and no
|
||||
document-scroller measurement left anywhere in app.js."""
|
||||
js = _js()
|
||||
assert re.search(r"export\s+const\s+NEAR_BOTTOM_PX\s*=\s*200\s*;", js), (
|
||||
"app.js must export `const NEAR_BOTTOM_PX = 200`"
|
||||
assert "NEAR_BOTTOM_PX" not in js, "the 200px band constant must be gone"
|
||||
assert "isNearBottom" not in js, "the pinned-to-bottom gate must be gone"
|
||||
assert "window.scrollY" not in js, (
|
||||
"nothing in app.js measures the page scroll offset anymore"
|
||||
)
|
||||
|
||||
|
||||
def test_is_near_bottom_uses_document_scroller() -> None:
|
||||
"""isNearBottom measures the DOCUMENT scroller (there is no inner
|
||||
scroll container — the page scrolls on the window): distance from the
|
||||
bottom of the document <= NEAR_BOTTOM_PX."""
|
||||
js = _js()
|
||||
body = _fn_body(js, "isNearBottom")
|
||||
for ref in (
|
||||
"documentElement.scrollHeight",
|
||||
"window.scrollY",
|
||||
"window.innerHeight",
|
||||
"NEAR_BOTTOM_PX",
|
||||
):
|
||||
assert ref in body, f"isNearBottom must reference {ref!r}"
|
||||
assert "<=" in body, "the pinned band is an upper bound, not exact equality"
|
||||
|
||||
|
||||
def test_single_scroll_gate() -> None:
|
||||
"""scrollReveal is the ONE scroll call site in app.js: it fires only
|
||||
when forced or when the user is pinned to the bottom, keeps
|
||||
`block: "end"`, and both addMessage (behavior + force passthrough) and
|
||||
addTyping (defaults) delegate to it."""
|
||||
def test_scroll_helper_is_unconditional() -> None:
|
||||
"""scrollReveal is still the ONE scrollIntoView in app.js, but it now
|
||||
scrolls unconditionally — no force-or-near-bottom condition in its
|
||||
body, and the phase-18 `force` parameter is gone. A page scroll can
|
||||
only ever happen where scrollReveal is CALLED (submit + restore)."""
|
||||
js = _js()
|
||||
body = _fn_body(js, "scrollReveal")
|
||||
assert "force || isNearBottom()" in body, "gate: force OR pinned to the bottom"
|
||||
assert "scrollIntoView" in body
|
||||
assert 'block: "end"' in body
|
||||
# The regression pin: exactly one scrollIntoView in the whole file, and
|
||||
# it lives inside scrollReveal.
|
||||
assert js.count("scrollIntoView") == 1, (
|
||||
assert "if (" not in body, "the helper must have no gate — it scrolls when called"
|
||||
assert "force" not in body, "the phase-18 force parameter must be gone"
|
||||
# Still smooth / reduced-motion-aware through the default behavior.
|
||||
assert "behavior = SCROLL" in body
|
||||
# The regression pin: exactly one actual scrollIntoView CALL in the
|
||||
# whole file, and it lives inside scrollReveal (the word may appear
|
||||
# in comments; the call must not).
|
||||
assert js.count(".scrollIntoView(") == 1, (
|
||||
"app.js must call scrollIntoView exactly once (inside scrollReveal)"
|
||||
)
|
||||
assert js.find("scrollIntoView") > js.find("function scrollReveal")
|
||||
# addMessage passes its behavior/force through; addTyping uses defaults.
|
||||
add_body = _fn_body(js, "addMessage")
|
||||
assert "scrollReveal(wrap, scrollBehavior, force)" in add_body
|
||||
assert "force = false" in add_body
|
||||
typing_body = _fn_body(js, "addTyping")
|
||||
assert "scrollReveal(wrap)" in typing_body
|
||||
assert js.find(".scrollIntoView(") > js.find("function scrollReveal")
|
||||
|
||||
|
||||
def test_submit_reveal_is_gated() -> None:
|
||||
"""Submit keeps the plain default call — no force: the gate decides,
|
||||
and it does in real use because submitting from the composer means the
|
||||
user is pinned (inside the 200px band); a submit with the viewport away
|
||||
from the bottom does not yank it."""
|
||||
def test_add_message_takes_explicit_scroll_intent() -> None:
|
||||
"""addMessage(who, html, scroll = false): the phase-18
|
||||
scrollBehavior/force parameters are gone; the bubble scrolls only
|
||||
when the caller explicitly asks (submit reveal, restore landing)."""
|
||||
js = _js()
|
||||
body = _fn_body(js, "addMessage")
|
||||
assert "function addMessage(who, html, scroll = false)" in body
|
||||
assert "if (scroll) scrollReveal(wrap)" in body
|
||||
assert "force" not in body
|
||||
assert "scrollBehavior" not in body
|
||||
|
||||
|
||||
def test_submit_reveals_user_message() -> None:
|
||||
"""User intent kept by the owner: submitting scrolls the viewport down
|
||||
so the user's own message is visible — the submit addMessage passes
|
||||
the scroll intent; the streaming brain-bubble creations in the same
|
||||
function never do."""
|
||||
js = _js()
|
||||
send = js.find("async function handleSend")
|
||||
assert send != -1, "handleSend must exist"
|
||||
call = 'addMessage("user", renderMarkdown(text));'
|
||||
idx = js.find(call, send)
|
||||
assert idx != -1, "handleSend must reveal the user message via the plain default"
|
||||
assert 'addMessage("user", renderMarkdown(text),' not in js, (
|
||||
"the submit call must not pass a third/fourth argument (no force)"
|
||||
body = js[send : js.find("\n}\n", send)]
|
||||
assert 'addMessage("user", renderMarkdown(text), true)' in body, (
|
||||
"the submit must reveal the user message (scroll intent true)"
|
||||
)
|
||||
for call in re.findall(r'addMessage\("brain"([^)]*)\)', body):
|
||||
assert "true" not in call, (
|
||||
f"streaming brain bubbles must not scroll the page: {call!r}"
|
||||
)
|
||||
|
||||
|
||||
def test_restore_force_landing() -> None:
|
||||
"""Both restore call sites are the only `force`d scrolls: one-shot,
|
||||
non-smooth ("auto") landing on the last restored message (phase-14
|
||||
behavior preserved)."""
|
||||
def test_streaming_handlers_never_scroll_the_page() -> None:
|
||||
"""The heart of the phase-42 contract: the thinking / tool / delta
|
||||
branches contain NO page-scroll call (no scrollReveal, no raw
|
||||
scrollIntoView). The thinking branch keeps the block-INTERNAL pin
|
||||
(textEl.scrollTop — phase 17, reworked in phase 43): that scrolls
|
||||
the block's own clip, not the page."""
|
||||
js = _js()
|
||||
think = js.find('ev.type === "thinking"')
|
||||
tool = js.find('ev.type === "tool"')
|
||||
delta = js.find('ev.type === "delta"')
|
||||
done = js.find('ev.type === "done"')
|
||||
assert -1 < think < tool < delta < done, "the turn handler must branch in order"
|
||||
for name, branch in (
|
||||
("thinking", js[think:tool]),
|
||||
("tool", js[tool:delta]),
|
||||
("delta", js[delta:done]),
|
||||
):
|
||||
assert "scrollReveal" not in branch, f"the {name} branch must not scroll the page"
|
||||
assert ".scrollIntoView(" not in branch, (
|
||||
f"the {name} branch must not scroll the page"
|
||||
)
|
||||
# The thinking window pin survives (phase 17 — untouched by this phase).
|
||||
thinking_branch = js[think:delta]
|
||||
assert "textEl.scrollTop = textEl.scrollHeight" in thinking_branch
|
||||
assert js.count("textEl.scrollTop = textEl.scrollHeight") == 1
|
||||
|
||||
|
||||
def test_restore_landing_is_one_shot() -> None:
|
||||
"""The phase-14 restore landing keeps its one-shot scroll
|
||||
(owner-kept): both restore call sites pass the explicit scroll
|
||||
intent and they are the only two restore scrolls; with the submit's
|
||||
single reveal, exactly three `true` intents exist in the whole file.
|
||||
The old forced "auto" landing is gone, and the one-shot, load-time
|
||||
contract is documented at the call site."""
|
||||
js = _js()
|
||||
body = _fn_body(js, "renderStoredMessage")
|
||||
assert 'addMessage("user", renderMarkdown(m.text), "auto", true)' in body
|
||||
assert 'addMessage("brain", renderMarkdown(m.text), "auto", true)' in body
|
||||
# Forced restores are restore-only: exactly two ("auto", true) sites.
|
||||
assert js.count('"auto", true') == 2, "only the two restore calls may force"
|
||||
assert 'addMessage("user", renderMarkdown(m.text), true)' in body
|
||||
assert 'addMessage("brain", renderMarkdown(m.text), true)' in body
|
||||
assert js.count('"auto", true') == 0, "the old forced 'auto' landing must be gone"
|
||||
# Submit reveal + the two restore landings — nothing else scrolls.
|
||||
assert js.count(", true)") == 3, "only submit + the two restore calls may scroll"
|
||||
# The marker comment documents the one-shot, load-time contract.
|
||||
assert "restore landing" in body
|
||||
assert "one-shot" in body
|
||||
|
||||
|
||||
def test_streaming_scrolls_only_through_gate() -> None:
|
||||
"""The per-chunk scrolls that used to yank the viewport (the phase-17
|
||||
thinking branch and the streaming delta branch) now go through
|
||||
scrollReveal with no raw scrollIntoView at either call site; the
|
||||
block's internal bottom-pinning (its own overflow, not the page) stays."""
|
||||
def test_typing_bubble_does_not_scroll() -> None:
|
||||
"""A typing indicator appearing must not yank the page — the phase-18
|
||||
scrollReveal call in addTyping is removed with the gate."""
|
||||
js = _js()
|
||||
thinking_idx = js.find('ev.type === "thinking"')
|
||||
delta_idx = js.find('ev.type === "delta"')
|
||||
done_idx = js.find('ev.type === "done"')
|
||||
assert -1 < thinking_idx < delta_idx < done_idx
|
||||
thinking_branch = js[thinking_idx:delta_idx]
|
||||
delta_branch = js[delta_idx:done_idx]
|
||||
assert "scrollReveal(wrap)" in thinking_branch
|
||||
assert "scrollReveal(wrap)" in delta_branch
|
||||
assert "scrollIntoView" not in thinking_branch
|
||||
assert "scrollIntoView" not in delta_branch
|
||||
assert "textEl.scrollTop = textEl.scrollHeight" in thinking_branch
|
||||
body = _fn_body(js, "addTyping")
|
||||
assert "scrollReveal" not in body
|
||||
assert ".scrollIntoView(" not in body
|
||||
|
||||
|
||||
def test_scroll_constant_reduced_motion_intact() -> None:
|
||||
"""The SCROLL constant is untouched (calm, don't remove): smooth by
|
||||
default, "auto" under prefers-reduced-motion — the two kept scroll
|
||||
call sites ride it as the default behavior."""
|
||||
js = _js()
|
||||
assert 'const SCROLL = reducedMotion ? "auto" : "smooth";' in js
|
||||
assert 'matchMedia("(prefers-reduced-motion: reduce)")' in js
|
||||
assert "Calm, don't remove" in js
|
||||
|
||||
|
||||
def test_turn_end_focus_does_not_scroll() -> None:
|
||||
"""The turn-end focus-back (phase 06's "always focus back") must not
|
||||
move the viewport: focusing the composer while the user is scrolled up
|
||||
would yank them to the bottom at the moment the turn ends — the exact
|
||||
defect phase 18 removes. preventScroll keeps the keyboard flow.
|
||||
startNewChat keeps plain focus (the list is cleared, nothing to yank
|
||||
past)."""
|
||||
move the viewport: focusing the composer while the user is scrolled
|
||||
up would yank them to the bottom at the moment the turn ends.
|
||||
preventScroll keeps the keyboard flow without the scroll."""
|
||||
js = _js()
|
||||
finally_idx = js.find("// done | error → idle: always settle, always focus back")
|
||||
assert finally_idx != -1, "the turn's finally block must exist"
|
||||
|
||||
Reference in New Issue
Block a user