From 3a404eb1619eaab62b1d0d253302aad2488c06e9 Mon Sep 17 00:00:00 2001 From: ducoterra Date: Fri, 28 Aug 2026 17:10:02 -0400 Subject: [PATCH] fix(chat): stop the submit up-hop and keep the thinking pin alive across paragraph breaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - scrollReveal lands at the document bottom (window.scrollTo) instead of scrollIntoView({ block: 'end' }): the old alignment sat above the in-flow composer, so every Enter hopped the page up by the composer+footer height and pushed the composer below the fold. - The thinking window's pin state is now captured BEFORE the re-render (const pinned = block.open && isThinkingNearBottom(textEl)): the post-render distance read the new chunk's rendered height, not the user's position, so any chunk taller than the 32px band (real-model deltas, '\n\n' paragraph breaks) killed the follow at the first 2-newline gap. - Mock LLM: new 'think in paragraphs' trigger (scratchpad with real blank-line breaks, 60-char frames) — the 12-char mock frames never rendered past the band, which is why the bug survived the E2E gates. - E2E (both verified red against the old code): test_submit_does_not_hop_up, test_thinking_window_follows_across_paragraph_breaks. - Unit source-marker tests updated to the new contracts. --- frontend/assets/app.js | 45 +++++++++---- tests/e2e/mock_llm.py | 89 ++++++++++++++++++++++++-- tests/e2e/test_no_reply_autoscroll.py | 79 +++++++++++++++++++++++ tests/e2e/test_thinking_scroll.py | 91 +++++++++++++++++++++++++++ tests/unit/test_frontend_scroll.py | 51 +++++++++------ tests/unit/test_thinking_scroll.py | 65 +++++++++++++------ 6 files changed, 365 insertions(+), 55 deletions(-) diff --git a/frontend/assets/app.js b/frontend/assets/app.js index 922aed5..a832f1b 100644 --- a/frontend/assets/app.js +++ b/frontend/assets/app.js @@ -76,8 +76,10 @@ * moves the viewport, so scrolling up to read earlier content holds for * the rest of the turn. The only scroll call sites are user intent: the * submit (your own message is revealed) and the phase-14 restore landing - * (one-shot, load-time). scrollReveal(wrap) is the one scrollIntoView in - * this file; addMessage(who, html, scroll) carries the intent. The + * (one-shot, load-time). scrollReveal(wrap) is the one page scroll in + * this file — a document-BOTTOM landing, not a message-bottom alignment + * (its comment explains why block:"end" hopped the page up on submit); + * addMessage(who, html, scroll) carries the intent. The * thinking block's internal bottom-pin (textEl.scrollTop, phase 17 — * reworked separately in phase 43) pins the block's own clip, not the * page, and is untouched here. @@ -167,8 +169,10 @@ const SCROLL = reducedMotion ? "auto" : "smooth"; * tail only while the user is pinned near the window's bottom — * the 32px band is the "window bottom in view" threshold. Scrolling * up pauses the follow; returning to the bottom resumes it (the - * check runs on every chunk). Exported so the band is unit-pinned - * (same pattern as TURN_TIMEOUT_MS). */ + * check runs on every chunk — against the PRE-render geometry: a + * post-render reading measures the new chunk's height, not the user's + * position, and the follow died at the first \"\n\n\" paragraph break). + * Exported so the band is unit-pinned (same pattern as TURN_TIMEOUT_MS). */ export const THINKING_NEAR_BOTTOM_PX = 32; function isThinkingNearBottom(textEl) { @@ -184,11 +188,20 @@ function isThinkingNearBottom(textEl) { * the user submit (reveal my message) and the phase-14 restore * landing (one-shot, load-time). */ -/* The ONE scrollIntoView in this file — unconditional (unit-pinned): +/* The ONE page scroll in this file — unconditional (unit-pinned): * scrollReveal scrolls whenever it is called, so a page scroll can only - * ever happen from those two user-intent call sites. */ + * ever happen from those two user-intent call sites. It lands at the + * DOCUMENT BOTTOM, not on the message's own bottom edge: the composer + * and footer sit below the message in flow, so the old + * scrollIntoView({ block: "end" }) aligned the message's bottom to the + * viewport bottom — ABOVE the document bottom — and hopped the page UP + * by the composer+footer height on every submit (pushing the composer + * below the fold). At the document bottom the revealed message sits in + * view with the composer right under it. `wrap` is the revealed + * element, kept in the signature so the call sites read as intent. */ function scrollReveal(wrap, behavior = SCROLL) { - wrap.scrollIntoView({ behavior, block: "end" }); + void wrap; + window.scrollTo({ top: document.documentElement.scrollHeight, behavior }); } /* ---------- document viewer link (phase 10; phase 13 adds `back`) ---------- @@ -970,11 +983,21 @@ async function handleSend(e) { removeTyping(); // the live block replaces the dots as feedback const block = ensureThinkingBlock(wrap); const textEl = block.querySelector(".thinking-text"); + // Pin state is measured BEFORE the re-render: a chunk taller + // than the 32px band — any paragraph break ("\n\n") or a few + // lines of text, which is exactly what the real model streams — + // grows the window's content below the old bottom, so measuring + // the distance AFTER the update reads the chunk's height, not + // the user's position, and the follow died at the first + // 2-newline gap. Pre-update, the distance is where the user + // actually is. + const pinned = block.open && isThinkingNearBottom(textEl); textEl.innerHTML = renderMarkdown(thinkingAcc); // escape-first, XSS-safe - if (block.open && isThinkingNearBottom(textEl)) { - // Follow the live tail only while the user is pinned to the window - // bottom (owner direction 2026-08-27); a scrolled-up reader is - // never re-pinned — returning to the bottom re-arms the pin. + if (pinned) { + // Follow the live tail only while the user was pinned to the + // window bottom before this chunk (owner direction + // 2026-08-27); a scrolled-up reader is never re-pinned — + // returning to the bottom re-arms the pin on the next chunk. textEl.scrollTop = textEl.scrollHeight; } } else if (ev.type === "tool") { diff --git a/tests/e2e/mock_llm.py b/tests/e2e/mock_llm.py index 3c04d20..1417c39 100644 --- a/tests/e2e/mock_llm.py +++ b/tests/e2e/mock_llm.py @@ -30,6 +30,17 @@ Implements just enough of the aipi surface: ``think out loud`` stream, then a 4s pause before the first content frame (the sources-midstream story, phase 20 — a deterministic "leave during pure thinking" navigation window). + - user message containing ``think in paragraphs`` (``THINK_PARAS_TRIGGER``) + -> the ``think out loud`` scratchpad WITH REAL paragraph breaks + ("\n\n"), streamed at 60-char frames (vs the mock's 12-char default). + One frame renders several lines — a real-model-sized delta, the + condition under which a POST-render pin-state reading (the old + app.js) measured the chunk's height instead of the user's position + and the think-window follow died at the first 2-newline gap. The + regression pin for the pre-render capture in app.js (2026-08-29, + owner report). Checked BEFORE ``think out loud`` (it is the more + specific phrase); existing E2E questions carry neither, so every + other suite is unaffected. - system prompt containing ```` (phase 15, steering notes) -> the composed answer ends with `` (tuning: )`` — makes prompt injection observable in the UI deterministically. @@ -167,6 +178,22 @@ LONG_ANSWER_END = "LONG-ANSWER-END" #: contain the substring, so every other suite is unaffected. THINKING_TRIGGER = "think out loud" +#: Regression pin (2026-08-29, owner report): a user message containing +#: this substring (case-insensitive) gets the phase-17 scratchpad WITH +#: REAL paragraph breaks ("\n\n"), streamed at ``THINK_PARAS_CHUNK`` +#: chars/frame — a single frame renders several lines (a real-model-sized +#: delta), which is the condition under which the old POST-render pin +#: reading in app.js died at the first 2-newline gap. Existing E2E +#: questions do not contain the phrase, so every other suite is +#: unaffected (checked before ``THINKING_TRIGGER`` — the more specific +#: phrase wins). +THINK_PARAS_TRIGGER = "think in paragraphs" +#: 60-char thinking frames for the paragraph trigger (the mock default is +#: 12 — a 12-char frame renders at most one line ≈ 22px, always inside +#: the 32px think-window band, which is why the old code passed the +#: 12-char suites while the real model's larger deltas killed the pin). +THINK_PARAS_CHUNK = 60 + #: Phase 20 (sources-midstream bug): a user message containing this #: substring (case-insensitive) gets the phase-17 thinking stream followed #: by a multi-second pause before the FIRST content frame — the @@ -526,6 +553,13 @@ def compose_thinking(body: dict[str, Any]) -> str: THINKING_TAIL) are what the E2E assertions key off — both are preserved. """ q = _user(body).strip()[:60] + return _thinking_template(q) + + +def _thinking_template(q: str) -> str: + """The fixed Step/Scratch scratchpad (``compose_thinking`` and its + paragraph variant share the exact same text — only the line + separators differ).""" return ( f"Step 1: Read the question carefully — “{q}” — and figure out what kind of " "answer it wants (a how-to, a lookup, or a design decision) before touching " @@ -572,6 +606,22 @@ def compose_thinking(body: dict[str, Any]) -> str: ) +def compose_thinking_paragraphs(body: dict[str, Any]) -> str: + """The phase-17 scratchpad with REAL paragraph breaks (\n\n, 2026-08-29 + regression pin): the same deterministic text as ``compose_thinking``, + with a blank line inserted after scratchpad lines 2 and 6 (0-based) — + two genuine \"2-newline gaps\" in the rendered scratchpad. Unique per + question, byte-stable across runs (same length contract + 2 chars).""" + base = compose_thinking(body) + lines = base.split("\n") + out: list[str] = [] + for i, line in enumerate(lines): + out.append(line) + if i in (2, 6): + out.append("") # blank line -> a real \"\n\n\" gap + return "\n".join(out) + + @app.post("/__shutdown__") def shutdown() -> dict[str, Any]: """Test hook (loading-feedback story): terminate this mock process to @@ -617,11 +667,21 @@ def embeddings(body: dict[str, Any]) -> dict[str, Any]: def _sse_stream( - answer: str, delay: float, thinking: str = "", pre_content_delay: float = 0.0 + answer: str, + delay: float, + thinking: str = "", + pre_content_delay: float = 0.0, + chunk: int = 12, ) -> Any: """SSE frames for one chat completion (phase 17: + reasoning). - When ``thinking`` is non-empty its 12-char slices go out FIRST as + ``chunk`` (default 12) is the slice size for BOTH the thinking and + the content frames — the ``think in paragraphs`` trigger raises it + to ``THINK_PARAS_CHUNK`` (60) so a single frame renders past the + 32px think-window band (see ``THINK_PARAS_TRIGGER``). At 12 the + output is byte-identical to the original. + + When ``thinking`` is non-empty its ``chunk``-sized slices go out FIRST as ``delta.reasoning_content`` frames — same 0.02s cadence and envelope as the content frames, the aipi wire convention (reasoning before content). Without ``thinking`` the output is byte-identical to the @@ -636,7 +696,7 @@ def _sse_stream( chunk_id = f"chatcmpl-{uuid.uuid4()}" if delay: time.sleep(delay) - for piece in re.findall(r".{1,12}", thinking, re.S): + for piece in re.findall(rf".{{1,{chunk}}}", thinking, re.S): payload = { "id": chunk_id, "object": "chat.completion.chunk", @@ -650,7 +710,7 @@ def _sse_stream( time.sleep(0.02) if pre_content_delay: time.sleep(pre_content_delay) - for piece in re.findall(r".{1,12}", answer, re.S): + for piece in re.findall(rf".{{1,{chunk}}}", answer, re.S): payload = { "id": chunk_id, "object": "chat.completion.chunk", @@ -791,7 +851,18 @@ def chat_completions(body: dict[str, Any]) -> Any: answer = _apply_max_tokens(compose_answer(body), body.get("max_tokens")) delay = 3.0 if "pretend to think slowly" in _user(body) else 0.0 - thinking = compose_thinking(body) if THINKING_TRIGGER in user_lower else "" + # ``think in paragraphs`` wins over ``think out loud`` (more specific): + # the same scratchpad WITH real "\n\n" paragraph breaks, at 60-char + # frames (real-model-sized deltas — the 32px-band regression pin). + if THINK_PARAS_TRIGGER in user_lower: + thinking = compose_thinking_paragraphs(body) + chunk = THINK_PARAS_CHUNK + elif THINKING_TRIGGER in user_lower: + thinking = compose_thinking(body) + chunk = 12 + else: + thinking = "" + chunk = 12 pre_content = ( PRE_CONTENT_PAUSE_S if SLOW_PRETOKEN_TRIGGER in user_lower else 0.0 ) @@ -814,7 +885,13 @@ def chat_completions(body: dict[str, Any]) -> Any: } return StreamingResponse( - _sse_stream(answer, delay, thinking=thinking, pre_content_delay=pre_content), + _sse_stream( + answer, + delay, + thinking=thinking, + pre_content_delay=pre_content, + chunk=chunk, + ), media_type="text/event-stream", headers={"Cache-Control": "no-cache", "X-Accel-Buffering": "no"}, ) diff --git a/tests/e2e/test_no_reply_autoscroll.py b/tests/e2e/test_no_reply_autoscroll.py index 17bba65..7593b56 100644 --- a/tests/e2e/test_no_reply_autoscroll.py +++ b/tests/e2e/test_no_reply_autoscroll.py @@ -38,6 +38,13 @@ Test → story mapping (Playwright Mapping Rule): 3. ``test_submit_reveals_user_message`` 4. ``test_restore_landing_one_shot`` 5. ``test_answer_content_intact`` +6. ``test_submit_does_not_hop_up`` — regression (2026-08-29, owner + report): submitting from the document bottom must NOT pull the page + up. The old ``scrollIntoView({ block: "end" })`` reveal aligned the + message's bottom to the viewport bottom — which sits above the + in-flow composer — so every Enter hopped the page up by the + composer+footer height and pushed the composer below the fold. The + reveal now lands at the document bottom (composer stays in view). """ from __future__ import annotations @@ -494,3 +501,75 @@ def test_answer_content_intact(page: Page, app_url: str, seeded_kb: None) -> Non expect(restored).not_to_have_attribute("open") expect(restored.locator(".thinking-text")).to_contain_text(THINKING_FRAGMENT) wait_settled(page) + + +# --------------------------------------------------------------------------- +# 6. Regression (2026-08-29, owner report): pressing Enter to send must +# not scroll the page UP. The user is at the document bottom (having +# read the answer), sends a new question; the reveal may only move +# the viewport DOWN (the new message grows the page) — the old +# block:"end" alignment hopped it up by the composer+footer height +# and pushed the composer below the fold on every submit. +# --------------------------------------------------------------------------- + + +def test_submit_does_not_hop_up( + page: Page, app_url: str, seeded_kb: None +) -> None: + page.set_default_timeout(30_000) + page.goto(app_url) + + # A settled long answer (the document overflows the 800px viewport), + # and the user has scrolled to the very bottom to read it — the + # real-world position for the next Enter. + submit(page, LONG_QUESTION) + wait_settled(page) + page.evaluate("() => window.scrollTo(0, document.documentElement.scrollHeight)") + page.wait_for_timeout(150) # let the (user) scroll land + state = scroll_state(page) + assert state["sh"] > state["ch"], "a long answer must make the document scrollable" + assert state["y"] + state["ch"] >= state["sh"] - STABLE_PX, "at the bottom" + y0 = state["y"] + + # Send the next question from the bottom. Sample the viewport from + # the moment the user bubble lands through the smooth reveal: the + # page must never move ABOVE where the user left it (no up-hop), and + # the reveal may only settle at or BELOW the starting position + # (the new message grows the page — a downward reveal). The old + # block:"end" alignment settled ~170px (composer+footer) ABOVE it. + page.fill("#message-input", SHORT_QUESTION) + page.click("#send-btn") + expect(page.locator(".msg.user .bubble").last).to_contain_text(SHORT_QUESTION) + + samples: list[float] = [] + prev: float | None = None + deadline = time.monotonic() + 10 + while True: + y = scroll_state(page)["y"] + samples.append(y) + if prev is not None and abs(y - prev) <= STABLE_PX: + break # the reveal has settled (the only scroll in flight) + prev = y + if time.monotonic() >= deadline: + raise AssertionError("the submit reveal did not settle within timeout") + time.sleep(0.1) + + assert min(samples) >= y0 - STABLE_PX, ( + f"the submit hopped the page UP (y0={y0:.0f}, min={min(samples):.0f}) — " + "the reveal must land at the document bottom, never above the user" + ) + assert samples[-1] >= y0 - STABLE_PX, ( + f"the reveal settled ABOVE where the user was (y0={y0:.0f}, " + f"settled={samples[-1]:.0f}) — it must land at or below the start" + ) + # The user's own message is revealed in view. (The composer sits in + # view at the settled position too — the reveal lands at the document + # bottom — but it is not asserted here: phase 42 (no page autoscroll) + # lets the in-flight reply push it down afterwards, so its exact box + # is a timing race, not a contract.) + assert user_message_in_view(page), "the submit must reveal the user's message" + + # And the turn completes normally (nothing about the reveal changed + # the never-stale contract). + wait_settled(page) + expect(page.locator(".msg.brain .bubble").last).to_contain_text(MOCK_ANSWER_MARKER) diff --git a/tests/e2e/test_thinking_scroll.py b/tests/e2e/test_thinking_scroll.py index 5e89434..2693921 100644 --- a/tests/e2e/test_thinking_scroll.py +++ b/tests/e2e/test_thinking_scroll.py @@ -67,6 +67,14 @@ Test → story mapping (Playwright Mapping Rule): long answer, the page scrolls, the bubble's overflow is untouched. 7. ``test_restored_collapsed_thinking_unaffected`` — regression (phase 17): a settled thinking turn reloads collapsed with full text. +8. ``test_thinking_window_follows_across_paragraph_breaks`` — + regression (2026-08-29, owner report): a 2-newline gap (a real + "\n\n" paragraph break) must not stop the follow — the pin state is + measured PRE-render in app.js, so a chunk taller than the 32px band + (the mock streams this question at 60-char frames — a single frame + renders several lines, a real-model-sized delta) cannot kill the pin. + The 12-char suites above cannot catch this: a 12-char frame renders + at most one line (≈22px), always inside the band. """ from __future__ import annotations @@ -104,6 +112,10 @@ MOCK_ANSWER_MARKER = "Deterministic mock answer for E2E" #: Line fragment the mock's deterministic scratchpad must carry (phase 17 #: convention, mock_llm.compose_thinking). THINKING_FRAGMENT = "Step 2: Check my notes" +#: 2026-08-29 regression trigger (mock_llm.THINK_PARAS_TRIGGER): the +#: scratchpad WITH real "\n\n" paragraph breaks, streamed at 60-char +#: frames (mock_llm.THINK_PARAS_CHUNK — real-model-sized deltas). +PARAS_QUESTION = "think in paragraphs — how is my kubernetes cluster set up?" STORAGE_KEY = "bor.chat.v1" SELECTOR = ".msg.brain details.thinking .thinking-text" @@ -701,3 +713,82 @@ def test_restored_collapsed_thinking_unaffected( "messages" ][1]["thinking"] assert re.sub(r"\s+", "", raw) == re.sub(r"\s+", "", captured) + + +# --------------------------------------------------------------------------- +# 8. Regression (2026-08-29, owner report): a 2-newline gap (a real +# "\n\n" paragraph break) must not stop the follow. The old code +# measured the window's bottom distance AFTER the re-render — where it +# reads the new chunk's rendered height, not the user's position — so +# any frame taller than the 32px band (a real model's sentence, a +# paragraph break) killed the pin permanently. The fix measures the +# pin state BEFORE the re-render; this suite's 60-char mock frames +# guarantee multiple over-band frames land before the stream ends. +# --------------------------------------------------------------------------- + + +def test_thinking_window_follows_across_paragraph_breaks( + page: Page, app_url: str, seeded_kb: None +) -> None: + page.set_default_timeout(30_000) + page.goto(app_url) + submit(page, PARAS_QUESTION) + expect(page.locator(".msg.user .bubble").last).to_contain_text(PARAS_QUESTION) + + details = page.locator(".msg.brain").last.locator("details.thinking") + details.wait_for(state="attached", timeout=10_000) + + # Two preconditions, in one poll: the window is REAL (content past + # the 320px clip — a "window" only exists once it clips) AND the + # first real paragraph break has rendered (>=2

children — the + # mock's paragraph scratchpad breaks after scratchpad lines 2 and 6; + # break 1 lands well before the clip fills, so both hold together). + # By here, several over-band 60-char frames have landed — the old + # post-render reading is dead long ago. + page.wait_for_function( + f"() => {{ const el = document.querySelector('{SELECTOR}');" + " return !!el && el.scrollHeight > el.clientHeight &&" + " el.querySelectorAll('p').length >= 2; }", + timeout=10_000, + ) + len_overflow = page.evaluate( + f"() => document.querySelector('{SELECTOR}').innerText.length" + ) + # One more full frame past the clip edge, then measure — the follow + # must still be pinned to the live tail. + page.wait_for_function( + f"(minLen) => {{ const el = document.querySelector('{SELECTOR}');" + " return !!el && el.innerText.length >= minLen; }", + arg=len_overflow + 60, + timeout=15_000, + ) + + # Atomic sample (one evaluate — no frame can land between the + # preconditions and the measurement): still inside the pure-thinking + # window (block open, no answer token yet), the window overflows, a + # paragraph break is present, and the pin survived it. + sample = page.evaluate( + f"""() => {{ const el = document.querySelector('{SELECTOR}'); + const block = document.querySelector('details.thinking'); + const wrap = block ? block.closest('.msg.brain') : null; + const bubble = wrap ? wrap.querySelector('.bubble') : null; + return {{ top: el.scrollTop, height: el.scrollHeight, + client: el.clientHeight, + p: el.querySelectorAll('p').length, + open: !!(block && block.open), + bubble: bubble ? bubble.innerText.length : 0 }}; }}""" + ) + assert sample["p"] >= 2, "a real paragraph break must be in the scratchpad" + assert sample["open"], "the sample must land while the block is open" + assert sample["bubble"] == 0, "no answer token may have landed yet" + assert sample["height"] > sample["client"], "the window must overflow" + assert _at_tail(sample), ( + f"the pin must survive the paragraph break: {sample}" + ) + + # The turn settles; the scratchpad text past the break is intact and + # the answer landed. + expect(page.locator("#send-btn")).to_be_enabled(timeout=30_000) + expect(page.locator("#send-label")).to_have_text("Send") + expect(details.locator(".thinking-text")).to_contain_text("Step 3") + expect(page.locator(".msg.brain .bubble").last).to_contain_text(MOCK_ANSWER_MARKER) diff --git a/tests/unit/test_frontend_scroll.py b/tests/unit/test_frontend_scroll.py index 2c7f070..1fb403f 100644 --- a/tests/unit/test_frontend_scroll.py +++ b/tests/unit/test_frontend_scroll.py @@ -6,16 +6,22 @@ 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). +restore landing (one-shot, load-time). Both user-intent scrolls land at +the DOCUMENT BOTTOM (scrollReveal's `window.scrollTo`): the old +`scrollIntoView({ block: "end" })` aligned the message's bottom to the +viewport bottom — which sits above the in-flow composer — so every +submit hopped the page UP by the composer+footer height and pushed the +composer below the fold. 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 / +of the 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. +page scroll in app.js (a document-bottom `window.scrollTo` — no +`scrollIntoView` call remains), 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 (or to the +upward-hopping block:"end" reveal) is caught without a browser. """ from __future__ import annotations @@ -56,25 +62,34 @@ def test_phase18_gate_is_gone() -> None: 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).""" + """scrollReveal is still the ONE page scroll in app.js, and it + scrolls unconditionally — no gate in its body, and the phase-18 + `force` parameter is gone. A page scroll can only ever happen where + scrollReveal is CALLED (submit + restore). It lands at the document + BOTTOM via `window.scrollTo`: the old `scrollIntoView({ block: + "end" })` aligned the message's bottom to the viewport bottom, which + sits above the in-flow composer, so every submit hopped the page UP + by the composer+footer height (the "Enter scrolls the page up" + bug) — no `scrollIntoView` call may remain.""" js = _js() body = _fn_body(js, "scrollReveal") - assert "scrollIntoView" in body - assert 'block: "end"' in body + assert "window.scrollTo(" in body + assert "document.documentElement.scrollHeight" in body 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" + assert 'block: "end"' not in body, ( + "the upward-hopping block:'end' alignment 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)" + # The regression pins: no scrollIntoView call anywhere in the file, + # and the one window.scrollTo call lives inside scrollReveal. + assert js.count(".scrollIntoView(") == 0, ( + "app.js must not call scrollIntoView — the document-bottom " + "scrollTo replaces the block:'end' reveal" ) - assert js.find(".scrollIntoView(") > js.find("function scrollReveal") + assert js.count("window.scrollTo(") == 1 + assert js.find("window.scrollTo(") > js.find("function scrollReveal") def test_add_message_takes_explicit_scroll_intent() -> None: diff --git a/tests/unit/test_thinking_scroll.py b/tests/unit/test_thinking_scroll.py index 1e0dc99..724320e 100644 --- a/tests/unit/test_thinking_scroll.py +++ b/tests/unit/test_thinking_scroll.py @@ -12,12 +12,22 @@ live tail only while the user is pinned near the window's bottom (the follow; returning to the bottom re-arms it (the check runs on every chunk, by construction). +The gate is measured against the PRE-render geometry: the chunk's +re-render grows the window's content below the old bottom, so a +post-render reading measures the new chunk's height, not the user's +position — any chunk taller than the 32px band (a real model's +sentence, or a "\n\n" paragraph break) killed the follow at the first +2-newline gap. The pin state is captured into ``pinned`` BEFORE +``textEl.innerHTML = …`` and the pin line runs inside ``if (pinned)`` +after it. + The browser behavior itself is E2E-covered (tests/e2e/test_thinking_scroll.py, task 03); here we pin the CSS value + the owner-direction comment, the exported band, the gate -function's math, and the gated pin call — so a silent regression -(``overflow-y`` back to ``hidden``, band removed, pin ungated) is -catched without a browser. +function's math, and the pre-render capture + gated pin call — so a +silent regression (``overflow-y`` back to ``hidden``, band removed, +pin ungated, or the capture moved back after the re-render) is caught +without a browser. """ from __future__ import annotations @@ -94,35 +104,50 @@ def test_is_thinking_near_bottom_band_math() -> None: def test_thinking_pin_is_gated_on_window_bottom() -> None: - """The phase-17 pin is now GATED: the pin line sits inside - ``if (block.open && isThinkingNearBottom(textEl))`` in the - streaming thinking branch — the window follows the live tail only - while the user is pinned near its bottom, and ``block.open`` stays - in the gate so a closed block (e.g. restored collapsed, phase 17) - is never pinned. No unconditional ``if (block.open) { … pin … }`` - remains anywhere in app.js (the exact old gate string is gone).""" + """The phase-17 pin is GATED on the user's pin state captured BEFORE + the re-render: ``const pinned = block.open && + isThinkingNearBottom(textEl)`` sits above ``textEl.innerHTML = + …`` in the streaming thinking branch, and the pin line runs inside + ``if (pinned)`` below it. Measuring after the update would read the + new chunk's rendered height instead of the user's position — the + "2-newline gap" regression. ``block.open`` stays in the capture so a + closed block (e.g. restored collapsed, phase 17) is never pinned, + and no direct ``if (block.open)`` gate remains anywhere in app.js.""" js = _js() pin = "textEl.scrollTop = textEl.scrollHeight" assert js.count(pin) == 1, "the bottom-pin must exist exactly once" - # Surviving task-01 assertion: the pin lives in the streaming - # thinking branch (before the delta branch). + # The pin lives in the streaming thinking branch (before the delta + # branch). thinking_idx = js.find('ev.type === "thinking"') delta_idx = js.find('ev.type === "delta"') assert -1 < thinking_idx < delta_idx thinking_branch = js[thinking_idx:delta_idx] - gate = "if (block.open && isThinkingNearBottom(textEl))" - assert gate in thinking_branch, "the pin must be behind the combined gate" - # The pin line follows the gate (inside it) — the only pin in the - # branch is the gated one. - gate_idx = thinking_branch.find(gate) + capture = "block.open && isThinkingNearBottom(textEl)" + render = "textEl.innerHTML = renderMarkdown(thinkingAcc)" + gate = "if (pinned)" + assert capture in thinking_branch, ( + "the combined gate must be captured (block.open + the window band)" + ) + assert gate in thinking_branch, "the pin must sit inside the captured gate" + # Pre-capture → re-render → gated pin, in exactly that order: the + # pin state is the user's PRE-render position, not the chunk's + # rendered height. + cap_idx = thinking_branch.find(capture) + render_idx = thinking_branch.find(render, cap_idx) + assert render_idx != -1 and render_idx > cap_idx, ( + "the pin state must be measured BEFORE the re-render" + ) + gate_idx = thinking_branch.find(gate, render_idx) + assert gate_idx != -1 and gate_idx > render_idx, ( + "the pin must run AFTER the re-render, inside the captured gate" + ) pin_idx = thinking_branch.find(pin, gate_idx) assert pin_idx != -1 assert thinking_branch.count(pin) == 1 - # The old unconditional gate is gone from the whole file: no - # `if (block.open)` (closed paren) — the combined condition is the + # No direct `if (block.open)` gate may remain — the capture is the # only gate left. assert "if (block.open)" not in js, ( - "no unconditional `if (block.open)` pin may remain" + "no direct `if (block.open)` gate may remain — the capture is the gate" )