From 63d6901ebd865bfcdcb449381bf6bf3c1e27adcd Mon Sep 17 00:00:00 2001 From: Nathan Beddoe Date: Fri, 11 Sep 2026 00:35:38 +0200 Subject: [PATCH] fix(chat): resume following after a viewport clamp --- docs/bug-lessons.md | 9 +++ src/routes/Conversation.tsx | 15 +++- tests/chat-ui.test.mjs | 14 ++++ tests/fixtures/chat-scroll-clamp-checks.mjs | 80 +++++++++++++++++++++ 4 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 tests/fixtures/chat-scroll-clamp-checks.mjs diff --git a/docs/bug-lessons.md b/docs/bug-lessons.md index fb18f31..b908ee6 100644 --- a/docs/bug-lessons.md +++ b/docs/bug-lessons.md @@ -1,5 +1,14 @@ # Bug lessons +## 2026-09-10 — Downward wheel did not resume a reader clamped to the bottom + +- **Affected area:** `ConversationContent` scroll ownership in `src/routes/Conversation.tsx`. +- **Symptom signature:** While native output streams, a viewport resize clamps a paused reader to the bottom. A downward wheel leaves Jump to latest visible and later output no longer follows. +- **Root cause:** Following resumed only when a scroll event increased `scrollTop` and reached the bottom. After a layout clamp, a downward wheel need not increase `scrollTop`; the Chromium probe emitted only a one-pixel rounding correction in the opposite direction. Later output growth then left the reader behind. +- **Resolution:** Treat an explicit positive wheel at the existing bottom as a request to resume through the existing jump path. Layout changes alone and upward gestures still preserve the paused reader's intent. +- **Regression signal:** The packaged browser viewport-clamp case fails its Jump-hidden assertion before the fix and checks that resizing alone remains paused, then native downward wheel resumes following through the rest of the real stream. The original upward-reading case remains unchanged. This reproduces a concrete product defect; the earlier CI timeout's geometry was not captured, so that exact CI cause remains unconfirmed. +- **Prevention rule:** When scroll policy depends on user intent, account for explicit gestures that cannot move an element already clamped to its boundary. Exercise native gestures after layout changes rather than assuming every gesture emits a positive scroll delta. + ## 2026-09-10 — MCP credential dialog fell behind disabled actions - **Affected area:** `.mcp-portal` in `src/styles.css`, credential replacement after Settings reconnect. diff --git a/src/routes/Conversation.tsx b/src/routes/Conversation.tsx index 4bdace0..2fb9b99 100644 --- a/src/routes/Conversation.tsx +++ b/src/routes/Conversation.tsx @@ -25,6 +25,10 @@ import { type ShellStatus = ReturnType["status"]; +function isAtBottom(element: HTMLElement) { + return element.scrollHeight - element.scrollTop - element.clientHeight <= 1; +} + // Mounted memory only. No transcript or draft is cached across route changes. const positions = new Map< string, @@ -215,6 +219,14 @@ function ConversationContent({ aria-label="Messages" onWheel={(event) => { if (event.deltaY < 0) pauseFollowing(); + // Layout can clamp a paused reader to the bottom. A downward gesture + // then has no positive scroll delta with which to resume following. + else if ( + event.deltaY > 0 && + scroller.current && + isAtBottom(scroller.current) + ) + jump(); }} onTouchStart={(event) => { touchY.current = @@ -242,8 +254,7 @@ function ConversationContent({ const element = scroller.current; if (!element) return; const top = element.scrollTop; - const atBottom = - element.scrollHeight - top - element.clientHeight <= 1; + const atBottom = isAtBottom(element); // Shrinking content can clamp scrollTop to the bottom without user input. if (top < lastScrollTop.current && !atBottom) pauseFollowing(); else if (top > lastScrollTop.current && atBottom) { diff --git a/tests/chat-ui.test.mjs b/tests/chat-ui.test.mjs index 7093468..86319b7 100644 --- a/tests/chat-ui.test.mjs +++ b/tests/chat-ui.test.mjs @@ -1,3 +1,4 @@ +import { checkScrollClamp } from "./fixtures/chat-scroll-clamp-checks.mjs"; import * as Effect from "effect/Effect"; import assert from "node:assert/strict"; import { createHmac } from "node:crypto"; @@ -1998,6 +1999,19 @@ test( await page.setViewportSize({ width: 1280, height: 900 }); }, ); + await run("downward wheel resumes following after a viewport clamp", () => + checkScrollClamp({ + page, + origin, + make, + open, + send, + ready, + until, + sleep, + signal: t.signal, + }), + ); await run( "full Worker restart preserves native identity and honest recovery", async () => { diff --git a/tests/fixtures/chat-scroll-clamp-checks.mjs b/tests/fixtures/chat-scroll-clamp-checks.mjs new file mode 100644 index 0000000..11656b0 --- /dev/null +++ b/tests/fixtures/chat-scroll-clamp-checks.mjs @@ -0,0 +1,80 @@ +import assert from "node:assert/strict"; + +export async function checkScrollClamp({ + page, + origin, + make, + open, + send, + ready, + until, + sleep, + signal, +}) { + const conversation = await make("Viewport clamp while reading"); + const seeded = await fetch(`${origin}/__chat/seed`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ id: conversation.id, count: 50 }), + signal, + }); + assert.equal(seeded.ok, true); + await open(conversation.id); + await send("reading"); + const scroll = page.locator(".chat-scroll"); + const jump = page.getByRole("button", { + name: "Jump to latest", + exact: true, + }); + await until( + () => page.locator("[data-message-id]").last().innerText(), + (text) => text.includes("reading-2"), + "stream before viewport clamp", + ); + await scroll.focus(); + await page.keyboard.press("ArrowUp"); + await jump.waitFor(); + // Let the native key scroll finish before resizing the actual viewport. + await sleep(600); + const distance = await scroll.evaluate( + (element) => + element.scrollHeight - element.scrollTop - element.clientHeight, + ); + await page.setViewportSize({ + width: 1280, + height: 900 + Math.ceil(distance) + 32, + }); + await until( + () => + scroll.evaluate( + (element) => + element.scrollHeight - element.scrollTop - element.clientHeight, + ), + (remaining) => remaining <= 1, + "viewport clamps paused reader to bottom", + ); + await page.evaluate( + () => + new Promise((resolve) => + requestAnimationFrame(() => requestAnimationFrame(resolve)), + ), + ); + assert.equal( + await jump.isVisible(), + true, + "A layout clamp alone must not resume following", + ); + await scroll.hover(); + await page.mouse.wheel(0, 10000); + await jump.waitFor({ state: "hidden" }); + await ready(); + await until( + () => + scroll.evaluate( + (element) => + element.scrollHeight - element.scrollTop - element.clientHeight, + ), + (remaining) => remaining <= 1, + "downward wheel resumes following after layout clamp", + ); +} -- 2.51.2