From f0f4c42e75235918c0c82bab57414f132a933bc4 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 20 Jul 2026 21:05:03 +0800 Subject: [PATCH 1/4] fix(ui): stop phone scroll locking to the bottom on mode homes (collapse-budget gate) Since #964 docked the compact composer on phone mode homes, one hide-on-scroll event releases ~180-260px of scroll geometry (72-130px header grid collapse + ~110px reserve-pad shrink, both animated). Mode-home content is short, so the release exceeds the remaining scroll runway: scrollTop clamps straight onto the new bottom edge, a 12px upward drag re-reveals and snaps the geometry back under the finger, and the hide/reveal loop reads as "scroll locks to the bottom" with a blank band below the content (reported on /formulation, iOS installed PWA; reproduced and quantified in Chromium: runway 266->84 on hide, 2 chrome flips in a single gesture). Fix: a collapse-budget gate in computeScrollHideUpdate. Hosts report how much layout the chrome would release if it hid right now (readChromeCollapseBudget: collapsible header strip + every dock-clearance pad above the shared 0.75rem hidden size - shell reserve pad, DocumentViewer content pad, scroller-padding fallback), and hiding is refused unless the remaining runway can absorb the release plus the reveal-intent distance and slack. Short pages keep their chrome and scroll plainly; long pages hide mid-scroll exactly as before but can no longer start a hide at the bottom edge. The existing one-sided bottom-clamp guard is untouched. Consumers that report no budget keep today's behavior. Also fixes a latent DocumentViewer bug the new sweep exposed: its one-shot #main-content lookup could permanently hold a detached node when the shell remounts the scroller, silently killing the viewer's hide-on-scroll. The discovery observer now stays alive for the viewer's lifetime (childList-only, deduped setState). Guardrails: - tests/ui-phone-scroll.spec.ts: 18-test sweep over all 10 standalone mode homes, the 3 dashboard modes, and 4 long routes at phone viewports with reducedMotion "no-preference" (the suite-wide "reduce" default disables the very transitions that caused this bug - that is how #964 shipped green) and simulated PWA safe-area insets. Asserts sole-scrollport, no horizontal overflow, true-bottom reachability, and bottom-edge nudge stability (at most one chrome flip). Verified to fail red against the pre-fix behavior. - tests/use-hide-on-scroll.test.ts: oscillation reproduction harness plus runway-refusal, ample-runway, boundary, and budget-omitted back-compat cases. - ui-smoke: bottom-hide scenario updated to the gate contract (hide with runway remaining, then ride the geometry clamp to the bottom while hidden). Verified: vitest focused suites, ui-phone-scroll + ui-formulation + ui-tools + ui-smoke (chromium), webkit advisory sweep, verify:cheap, verify:ui (264 passed), verify:pr-local. Co-Authored-By: Claude Fable 5 --- playwright.config.ts | 4 +- src/components/ClinicalDashboard.tsx | 3 +- src/components/DocumentViewer.tsx | 17 +- .../global-search-shell.tsx | 6 +- .../mobile-composer-reserve.ts | 8 +- .../clinical-dashboard/use-hide-on-scroll.ts | 70 +++++- tests/mobile-composer-reserve.test.ts | 4 + tests/ui-phone-scroll.spec.ts | 224 ++++++++++++++++++ tests/ui-smoke.spec.ts | 11 +- tests/use-hide-on-scroll.test.ts | 127 ++++++++++ 10 files changed, 455 insertions(+), 19 deletions(-) create mode 100644 tests/ui-phone-scroll.spec.ts diff --git a/playwright.config.ts b/playwright.config.ts index 4ba972e55..fc2ab8e7a 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -13,14 +13,14 @@ const chromiumExecutablePath = process.env.PLAYWRIGHT_CHROMIUM_EXECUTABLE_PATH; // they share a spec file. Every required browser project uses the same // production matcher and tag exclusion. const productionSpecPattern = - /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|tools|overlap|universal-search|specifiers|formulation|pwa|route-coverage|visual-artifacts))\.spec\.ts/; + /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|tools|overlap|universal-search|specifiers|formulation|phone-scroll|pwa|route-coverage|visual-artifacts))\.spec\.ts/; const mockupSpecPattern = /.*ui-(tools|tools-collapse|tools-task-directory)\.spec\.ts/; const mockupTag = /@mockup/; export default defineConfig({ testDir: "./tests", testMatch: - /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|tools|tools-collapse|tools-task-directory|overlap|universal-search|specifiers|formulation|pwa|route-coverage|visual-artifacts))\.spec\.ts/, + /.*(?:answer-progress-ui-smoke|ui-(smoke|stress|accessibility|tools|tools-collapse|tools-task-directory|overlap|universal-search|specifiers|formulation|phone-scroll|pwa|route-coverage|visual-artifacts))\.spec\.ts/, timeout: 60_000, retries: 0, // Fail the run if a stray `test.only` is committed: otherwise it silently diff --git a/src/components/ClinicalDashboard.tsx b/src/components/ClinicalDashboard.tsx index cceecf086..c6269c59e 100644 --- a/src/components/ClinicalDashboard.tsx +++ b/src/components/ClinicalDashboard.tsx @@ -97,7 +97,7 @@ import { import { UniversalSearchAlsoMatches } from "@/components/clinical-dashboard/universal-search-also-matches"; import { FavouritesGuestGate } from "@/components/clinical-dashboard/favourites-guest-gate"; import { useDashboardShellActions } from "@/components/clinical-dashboard/use-dashboard-shell-actions"; -import { useScrollHideReporter } from "@/components/clinical-dashboard/use-hide-on-scroll"; +import { readChromeCollapseBudget, useScrollHideReporter } from "@/components/clinical-dashboard/use-hide-on-scroll"; import { SearchCommandProvider } from "@/components/clinical-dashboard/search-command-context"; import { answerReferencesDocument, @@ -2977,6 +2977,7 @@ export function ClinicalDashboard({ reportPhoneScrollHideRef.current({ offset: main.scrollTop, maxOffset: Math.max(0, main.scrollHeight - main.clientHeight), + collapseBudget: readChromeCollapseBudget(main), source: main, }); }); diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index 68b1f847e..f7aec6d04 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -1739,25 +1739,24 @@ export function DocumentViewer({ const [shellScrollContainer, setShellScrollContainer] = useState(null); useEffect(() => { let cancelled = false; - let observer: MutationObserver | null = null; - // #main-content is the app-shell scroll container: it mounts once (usually - // before this effect runs) and persists for the viewer's lifetime. Resolve it - // synchronously, and only fall back to observing the DOM until it appears — - // then disconnect, rather than reacting to every app-wide mutation forever. + // #main-content is the app-shell scroll container, but it does NOT reliably + // mount once: the shell can remount it around chrome-visibility/hydration + // swaps, leaving a one-shot lookup holding a detached node whose scroll + // events never fire (the phone composer then never hides). Keep observing + // for the viewer's lifetime — childList-only mutations are infrequent, and + // the setState below dedups so unrelated inserts cost one no-op callback. const sync = () => { if (cancelled) return; const main = window.document.getElementById("main-content"); if (!main) return; setShellScrollContainer((current) => (current === main ? current : main)); - observer?.disconnect(); - observer = null; }; - observer = new MutationObserver(sync); + const observer = new MutationObserver(sync); observer.observe(window.document.body, { childList: true, subtree: true }); sync(); return () => { cancelled = true; - observer?.disconnect(); + observer.disconnect(); }; }, []); const scrollHidden = useHideOnScroll({ diff --git a/src/components/clinical-dashboard/global-search-shell.tsx b/src/components/clinical-dashboard/global-search-shell.tsx index 49f2101f4..5211d3355 100644 --- a/src/components/clinical-dashboard/global-search-shell.tsx +++ b/src/components/clinical-dashboard/global-search-shell.tsx @@ -33,7 +33,7 @@ import { resolveMobileComposerReserve, resolveShellVisibleMobileComposerReserve, } from "@/components/clinical-dashboard/mobile-composer-reserve"; -import { useScrollHideReporter } from "@/components/clinical-dashboard/use-hide-on-scroll"; +import { readChromeCollapseBudget, useScrollHideReporter } from "@/components/clinical-dashboard/use-hide-on-scroll"; import { ModeHomeRouteLoading } from "@/components/mode-home-page-skeleton"; import { useSidebarCollapsed } from "@/components/clinical-dashboard/use-sidebar-collapsed"; import { useTheme } from "@/components/clinical-dashboard/use-theme"; @@ -539,6 +539,7 @@ function GlobalStandaloneSearchShellClient({ phoneScrollHide.reportScroll({ offset: target.scrollTop, maxOffset: Math.max(0, target.scrollHeight - target.clientHeight), + collapseBudget: readChromeCollapseBudget(target), source: target, }); } @@ -561,6 +562,9 @@ function GlobalStandaloneSearchShellClient({ reportPhoneScrollHideRef.current({ offset: target.scrollTop, maxOffset: Math.max(0, target.scrollHeight - target.clientHeight), + // Collapsing chrome releases layout into nested scrollers too (their + // flex height cap grows with the shell), so the same budget applies. + collapseBudget: readChromeCollapseBudget(main), source: target, }); }; diff --git a/src/components/clinical-dashboard/mobile-composer-reserve.ts b/src/components/clinical-dashboard/mobile-composer-reserve.ts index 40bdc30d2..9b794bcd8 100644 --- a/src/components/clinical-dashboard/mobile-composer-reserve.ts +++ b/src/components/clinical-dashboard/mobile-composer-reserve.ts @@ -9,7 +9,13 @@ * Keep these string values aligned with `--phone-dock-*` tokens in globals.css. */ -/** Content pad after the phone bottom composer has scrolled away. */ +/** + * Content pad after the phone bottom composer has scrolled away. The rem + * number is exported separately so the scroll-hide collapse budget + * (use-hide-on-scroll's readChromeCollapseBudget) measures against the same + * value; tests/mobile-composer-reserve.test.ts pins the pair together. + */ +export const mobileComposerHiddenReserveRem = 0.75; export const mobileComposerHiddenReserve = "0.75rem"; /** Routes with no floating bottom dock (info pages, the answer home hero). */ diff --git a/src/components/clinical-dashboard/use-hide-on-scroll.ts b/src/components/clinical-dashboard/use-hide-on-scroll.ts index d706a8caa..fe0fd5a2c 100644 --- a/src/components/clinical-dashboard/use-hide-on-scroll.ts +++ b/src/components/clinical-dashboard/use-hide-on-scroll.ts @@ -2,6 +2,8 @@ import { useCallback, useEffect, useRef, useState, useSyncExternalStore, type RefObject } from "react"; +import { mobileComposerHiddenReserveRem } from "@/components/clinical-dashboard/mobile-composer-reserve"; + // Matches phoneSearchLayoutMediaQuery in master-search-header.tsx — the repo's // phone/tablet seam. Hide-on-scroll runs below the sm breakpoint unless the // host opts into all breakpoints (the ClinicalDashboard glass-header overlay). @@ -26,11 +28,29 @@ const revealIntentDistance = 12; // offset is this near the maximum, an upward reading is the viewport growing // under a collapsing header rather than a real scroll, so it must not reveal. const bottomClampTolerance = 1; +// Hiding the chrome releases its layout space back to the scroller (header +// grid collapse + dock reserve-pad shrink), shrinking maxOffset by the same +// amount. When the runway left below the current offset is smaller than that +// release, the position clamps straight onto the new bottom edge and any +// upward drag past revealIntentDistance snaps the geometry back under the +// finger — a hide/reveal oscillation that reads as "scroll locks to the +// bottom" on short pages (phone mode homes after #964). The slack keeps a +// margin past the reveal threshold so the post-collapse position cannot sit +// within one deliberate micro-drag of a reveal. +const collapseRunwaySlack = 16; type ScrollDirection = "down" | "up" | null; export interface ScrollMetrics { offset: number; maxOffset?: number; + /** + * Layout px the chrome would release if it hid right now (see + * readChromeCollapseBudget). When provided together with maxOffset, hiding + * is refused unless enough runway remains below the offset to absorb the + * release. Omitted by consumers whose chrome does not change scroll + * geometry when hiding. + */ + collapseBudget?: number; source?: EventTarget; } @@ -39,6 +59,7 @@ export function computeScrollHideUpdate(params: { offset: number; lastOffset: number; maxOffset?: number; + collapseBudget?: number; sourceChanged?: boolean; currentlyHidden: boolean; direction?: ScrollDirection; @@ -53,6 +74,7 @@ export function computeScrollHideUpdate(params: { offset, lastOffset, maxOffset, + collapseBudget, sourceChanged = false, currentlyHidden, direction = null, @@ -106,7 +128,16 @@ export function computeScrollHideUpdate(params: { // Only count travel beyond the activation band. This stops a single flick // from the top hiding the chrome the instant it clears the header height. const travelPastActivation = Math.min(nextDirectionTravel, offset - hideActivationOffset); - hidden = travelPastActivation >= hideIntentDistance; + // Refuse to hide when the geometry the chrome would release exceeds the + // remaining runway (see collapseRunwaySlack above). Short pages then keep + // their chrome and scroll plainly; long pages simply never start a hide + // this close to the bottom edge. + const runwayAfterCollapse = + maxOffset === undefined || collapseBudget === undefined + ? Number.POSITIVE_INFINITY + : maxOffset - offset - collapseBudget; + hidden = + travelPastActivation >= hideIntentDistance && runwayAfterCollapse > revealIntentDistance + collapseRunwaySlack; } else if (currentlyHidden && nextDirection === "up" && nextDirectionTravel >= revealIntentDistance) { hidden = false; } @@ -119,6 +150,35 @@ export function computeScrollHideUpdate(params: { }; } +/** + * Measures how much layout (px) the chrome would release into the given + * scroller if hide-on-scroll fired right now: the in-flow collapsible header + * strip plus every visible dock-clearance pad above its hidden size. Reads the + * documented DOM contracts — `universal-header-collapse` for the header + * (absent under the overlay strategy, which does not affect geometry and so + * contributes 0), `mobile-composer-reserve-pad` for the shell reserve, + * `document-viewer-content` for DocumentViewer's own clearance (its hidden + * `pb-3` equals the shared 0.75rem hidden reserve), falling back to the + * scroller's own padding exactly like tests/playwright-scroll.ts. Call from + * inside a scroll handler, where layout is already flushed. + */ +export function readChromeCollapseBudget(scroller: HTMLElement): number { + const collapse = document.querySelector('[data-testid="universal-header-collapse"]'); + const headerRelease = collapse instanceof HTMLElement ? collapse.getBoundingClientRect().height : 0; + const rootFontSize = Number.parseFloat(window.getComputedStyle(document.documentElement).fontSize) || 16; + const hiddenPadPx = mobileComposerHiddenReserveRem * rootFontSize; + const padRelease = (element: Element | null): number => { + if (!(element instanceof HTMLElement)) return 0; + const paddingBottom = Number.parseFloat(window.getComputedStyle(element).paddingBottom); + return Number.isFinite(paddingBottom) ? Math.max(0, paddingBottom - hiddenPadPx) : 0; + }; + const reservePad = scroller.querySelector('[data-testid="mobile-composer-reserve-pad"]'); + const viewerPad = scroller.querySelector('[data-testid="document-viewer-content"]'); + const reserveRelease = + reservePad || viewerPad ? padRelease(reservePad) + padRelease(viewerPad) : padRelease(scroller); + return headerRelease + reserveRelease; +} + function subscribeToPhoneMedia(onChange: () => void) { const media = window.matchMedia(phoneMediaQuery); media.addEventListener("change", onChange); @@ -156,8 +216,10 @@ export function useScrollHideReporter(disabled = false, allowAllBreakpoints = fa const reportScroll = useCallback( (report: number | ScrollMetrics) => { - const { offset, maxOffset, source } = - typeof report === "number" ? { offset: report, maxOffset: undefined, source: undefined } : report; + const { offset, maxOffset, collapseBudget, source } = + typeof report === "number" + ? { offset: report, maxOffset: undefined, collapseBudget: undefined, source: undefined } + : report; if (!active || offset < 0) return; const lastOffset = lastOffsetRef.current; const delta = offset - lastOffset; @@ -171,6 +233,7 @@ export function useScrollHideReporter(disabled = false, allowAllBreakpoints = fa offset, lastOffset, maxOffset, + collapseBudget, sourceChanged, currentlyHidden: hiddenRef.current, direction: directionRef.current, @@ -267,6 +330,7 @@ export function useHideOnScroll({ return { offset: container.scrollTop, maxOffset: Math.max(0, container.scrollHeight - container.clientHeight), + collapseBudget: readChromeCollapseBudget(container), source: container, }; } diff --git a/tests/mobile-composer-reserve.test.ts b/tests/mobile-composer-reserve.test.ts index 3b0b7f56d..732f27b30 100644 --- a/tests/mobile-composer-reserve.test.ts +++ b/tests/mobile-composer-reserve.test.ts @@ -4,6 +4,7 @@ import { isDocumentViewerOwnedRoute, mobileComposerDifferentialsCompareReserve, mobileComposerHiddenReserve, + mobileComposerHiddenReserveRem, mobileComposerIdleReserve, mobileComposerVisibleReserve, resolveDashboardVisibleMobileComposerReserve, @@ -14,6 +15,9 @@ import { describe("mobile composer reserve contract", () => { it("collapses to the hidden pad without Safari toolbar safe-area", () => { expect(mobileComposerHiddenReserve).toBe("0.75rem"); + // The rem number feeds readChromeCollapseBudget's px math; it must stay + // equal to the CSS string above or the collapse budget silently drifts. + expect(`${mobileComposerHiddenReserveRem}rem`).toBe(mobileComposerHiddenReserve); expect(resolveMobileComposerReserve(true, mobileComposerVisibleReserve.shellAnswer)).toBe( mobileComposerHiddenReserve, ); diff --git a/tests/ui-phone-scroll.spec.ts b/tests/ui-phone-scroll.spec.ts new file mode 100644 index 000000000..036e10138 --- /dev/null +++ b/tests/ui-phone-scroll.spec.ts @@ -0,0 +1,224 @@ +import { expect, test, type Page } from "playwright/test"; + +/** + * Phone scroll-geometry guardrail (the #964 regression class). + * + * Hiding the phone chrome (header grid collapse + dock reserve-pad shrink) + * releases layout back into the #main-content scrollport. When that release + * exceeds the page's remaining scroll runway, the offset clamps onto the new + * bottom edge and any small upward drag snaps the geometry back — a + * hide/reveal oscillation that reads as "scroll locks to the bottom" plus a + * blank band below the content. use-hide-on-scroll's collapse-budget gate + * refuses such hides; this spec sweeps every phone surface and asserts the + * scroll geometry stays stable at the bottom edge. + * + * IMPORTANT: the suite-wide default emulates `reducedMotion: reduce`, which + * disables the very padding/grid transitions that produce this failure mode — + * it is exactly how #964 shipped green. Every test here re-enables motion via + * page.emulateMedia({ reducedMotion: "no-preference" }). + */ + +// Standalone mode homes share GlobalSearchShell's phone scroller and compact +// bottom dock (list mirrors global-search-shell.tsx isStandaloneModeHome). +const modeHomeRoutes = [ + "/formulation", + "/dsm", + "/tools", + "/differentials", + "/specifiers", + "/factsheets", + "/therapy-compass", + "/services", + "/forms", + "/favourites", +]; + +// ClinicalDashboard scroller (answer home keeps the in-flow hero pill; the +// other modes dock the compact composer) plus representative long pages. +const dashboardRoutes = ["/?mode=answer", "/?mode=documents", "/?mode=prescribing"]; +const longRoutes = [ + "/formulation/worry", + "/formulation/builder?mechanism=rumination&template=5Ps", + "/documents/search", + // Demo-corpus document detail: DocumentViewer owns its composer here, and its + // scroll container binding has its own failure mode (stale #main-content). + "/documents/11111111-1111-4111-8111-111111111111?page=1", +]; + +const phoneViewport = { width: 390, height: 844 }; + +async function blockExternalRequests(page: Page) { + await page.route("**/*", async (route) => { + const url = new URL(route.request().url()); + if ( + (url.protocol === "http:" || url.protocol === "https:") && + !["localhost", "127.0.0.1", "::1", "[::1]"].includes(url.hostname) + ) { + await route.abort("blockedbyclient"); + return; + } + await route.fallback(); + }); +} + +async function gotoPhoneSurface(page: Page, path: string) { + await page.goto(path, { waitUntil: "domcontentloaded" }); + await expect(page.locator("#main-content").first()).toBeVisible({ timeout: 15_000 }); + // Simulate installed-PWA safe-area insets (the repo routes env() through + // these vars precisely so Chromium tests can exercise them). + await page.addStyleTag({ + content: ":root{--safe-area-top:59px !important;--safe-area-bottom:34px !important;}", + }); + // Let hydration, fonts, and the composer/portal layout settle. + await page.waitForTimeout(700); +} + +interface ScrollGeometry { + scrollTop: number; + maxOffset: number; + headerHidden: boolean; + docScrollableExcess: number; + horizontalOverflow: number; +} + +function readGeometry(page: Page): Promise { + return page.evaluate(() => { + const main = document.getElementById("main-content"); + const header = document.querySelector('[data-testid="universal-header-collapse"]'); + const doc = document.documentElement; + return { + scrollTop: main?.scrollTop ?? 0, + maxOffset: main ? Math.max(0, main.scrollHeight - main.clientHeight) : 0, + headerHidden: header?.getAttribute("data-scroll-hidden") === "true", + docScrollableExcess: doc.scrollHeight - doc.clientHeight, + horizontalOverflow: Math.max(doc.scrollWidth, document.body?.scrollWidth ?? 0) - window.innerWidth, + }; + }); +} + +/** Counts phone-chrome hide/reveal flips (header collapse + composer dock). */ +async function installFlipCounter(page: Page) { + await page.evaluate(() => { + const counter = { flips: 0 }; + (window as unknown as { __scrollFlipCounter: typeof counter }).__scrollFlipCounter = counter; + const header = document.querySelector('[data-testid="universal-header-collapse"]'); + if (!header) return; + new MutationObserver(() => { + counter.flips += 1; + }).observe(header, { attributes: true, attributeFilter: ["data-scroll-hidden"] }); + }); +} + +function readFlipCount(page: Page): Promise { + return page.evaluate( + () => (window as unknown as { __scrollFlipCounter?: { flips: number } }).__scrollFlipCounter?.flips ?? 0, + ); +} + +/** + * Drags the phone scroller in deliberate steps (one per frame) so the scroll + * state machine sees real directional intent — a single programmatic jump + * models neither a touch drag nor iOS momentum. + */ +async function dragScrollBy(page: Page, totalPx: number, stepPx: number) { + await page.evaluate( + async ({ total, step }) => { + const main = document.getElementById("main-content"); + if (!main) return; + const steps = Math.max(1, Math.ceil(Math.abs(total) / step)); + const direction = total < 0 ? -1 : 1; + for (let i = 0; i < steps; i += 1) { + main.scrollTop += direction * step; + main.dispatchEvent(new Event("scroll", { bubbles: true })); + await new Promise((resolve) => requestAnimationFrame(() => resolve(undefined))); + } + }, + { total: totalPx, step: stepPx }, + ); +} + +test.beforeEach(async ({ page }) => { + await blockExternalRequests(page); +}); + +for (const route of [...modeHomeRoutes, ...dashboardRoutes, ...longRoutes]) { + test(`phone scroll stays smooth and bottom-stable on ${route}`, async ({ page }) => { + await page.emulateMedia({ reducedMotion: "no-preference" }); + await page.setViewportSize(phoneViewport); + await gotoPhoneSurface(page, route); + await installFlipCounter(page); + + const initial = await readGeometry(page); + // The document must never be the phone scroller (#main-content owns it), + // and no route may overflow horizontally. + expect(initial.docScrollableExcess, "document must not scroll on phone").toBeLessThanOrEqual(2); + expect(initial.horizontalOverflow, "no horizontal overflow").toBeLessThanOrEqual(2); + expect(initial.scrollTop).toBe(0); + expect(initial.headerHidden, "header visible at the top").toBe(false); + + // Drag to the bottom in deliberate 24px steps, then let transitions settle. + await dragScrollBy(page, initial.maxOffset + 400, 24); + await page.waitForTimeout(500); + const flipsAfterDescent = await readFlipCount(page); + const atBottom = await readGeometry(page); + + // No dead band: the settled position sits on the real bottom edge of the + // settled geometry (this is what "locks to the bottom" violated — the + // offset was pinned to a phantom bottom short of the content). + expect( + Math.abs(atBottom.scrollTop - atBottom.maxOffset), + "settled scroll sits on the true bottom edge", + ).toBeLessThanOrEqual(2); + // At most one chrome transition on a pure descent (hide, when the page is + // long enough to afford it) — more means hide/reveal oscillation. + expect(flipsAfterDescent, "no chrome oscillation while scrolling down").toBeLessThanOrEqual(1); + + // Oscillation check: geometry must be quiet AFTER settling — no further + // flips while the user holds still at the bottom. + await page.waitForTimeout(400); + expect(await readFlipCount(page), "no chrome flips while resting at the bottom").toBe(flipsAfterDescent); + + // Small deliberate nudges AT the bottom edge are the oscillation trigger: + // pre-fix, a 32px up-drag revealed the chrome (restoring ~180px of + // geometry under the finger), the next down-drag re-hid it, and so on — + // the "locks to the bottom" thrash. The collapse-budget gate permits at + // most the one legitimate reveal here and refuses to re-hide with no + // runway left, so the whole nudge cycle allows a single flip. + for (const nudge of [-32, 48, -32]) { + await dragScrollBy(page, nudge, 8); + await page.waitForTimeout(350); + } + const flipsAfterNudges = await readFlipCount(page); + expect(flipsAfterNudges - flipsAfterDescent, "bottom-edge nudges must not thrash the chrome").toBeLessThanOrEqual( + 1, + ); + await page.waitForTimeout(400); + expect(await readFlipCount(page), "no chrome flips after the nudges settle").toBe(flipsAfterNudges); + + // Top must be reachable with the header visible again. + await dragScrollBy(page, -(atBottom.maxOffset + 800), 48); + await page.waitForTimeout(500); + const backAtTop = await readGeometry(page); + expect(backAtTop.scrollTop, "top reachable after the round trip").toBe(0); + expect(backAtTop.headerHidden, "header visible back at the top").toBe(false); + expect(backAtTop.horizontalOverflow).toBeLessThanOrEqual(2); + }); +} + +// One larger-phone pass over the worst offender to catch viewport-dependent +// regressions (the 390x844 sweep above is the canonical size). +test("phone scroll stays smooth on /formulation at 430x932", async ({ page }) => { + await page.emulateMedia({ reducedMotion: "no-preference" }); + await page.setViewportSize({ width: 430, height: 932 }); + await gotoPhoneSurface(page, "/formulation"); + await installFlipCounter(page); + + const initial = await readGeometry(page); + expect(initial.docScrollableExcess).toBeLessThanOrEqual(2); + + await dragScrollBy(page, initial.maxOffset + 400, 24); + await page.waitForTimeout(500); + const atBottom = await readGeometry(page); + expect(Math.abs(atBottom.scrollTop - atBottom.maxOffset)).toBeLessThanOrEqual(2); + expect(await readFlipCount(page)).toBeLessThanOrEqual(1); +}); diff --git a/tests/ui-smoke.spec.ts b/tests/ui-smoke.spec.ts index 8dd6d723a..aa9412594 100644 --- a/tests/ui-smoke.spec.ts +++ b/tests/ui-smoke.spec.ts @@ -3581,7 +3581,10 @@ test.describe("Clinical KB UI smoke coverage", () => { // At the bottom, collapsing the in-flow header can reflow the scroll // surface and clamp scrollTop. That geometry-driven event is not an upward - // user gesture and must not immediately reveal the header. + // user gesture and must not immediately reveal the header. The collapse + // -budget gate refuses to START a hide at the bottom edge (that is the + // #964 "locks to the bottom" trap), so hide with runway remaining first, + // then ride the clamp to the bottom while hidden. await main.evaluate((node) => { node.scrollTop = 0; }); @@ -3589,7 +3592,11 @@ test.describe("Clinical KB UI smoke coverage", () => { const visibleMaxOffset = await main.evaluate((node) => node.scrollHeight - node.clientHeight); await main.evaluate((node, top) => { node.scrollTop = top; - }, visibleMaxOffset); + }, visibleMaxOffset - 400); + await expect(collapseHost).toHaveAttribute("data-scroll-hidden", "true"); + await main.evaluate((node) => { + node.scrollTop = node.scrollHeight - node.clientHeight; + }); await expect(collapseHost).toHaveAttribute("data-scroll-hidden", "true"); await expect.poll(async () => collapseHost.getAttribute("data-scroll-hidden"), { timeout: 1_000 }).toBe("true"); // The hidden attribute flips before the 240ms grid-row transition has diff --git a/tests/use-hide-on-scroll.test.ts b/tests/use-hide-on-scroll.test.ts index e494253d7..c233bfe85 100644 --- a/tests/use-hide-on-scroll.test.ts +++ b/tests/use-hide-on-scroll.test.ts @@ -223,6 +223,133 @@ describe("computeScrollHideUpdate", () => { }); }); + it("reproduces the short-page hide → clamp → tiny-reveal oscillation without a collapse budget", () => { + // Real numbers from a phone standalone mode home (390x844, post-#964): + // visible-chrome scroll runway maxOffset ≈ 300px; hiding the chrome + // releases ~240px (header grid collapse + dock reserve-pad shrink). The + // hide decision itself must be what prevents the trap; every guard below + // it can only hold state, not restore the lost runway. + let state = { hidden: false, lastOffset: 0, direction: null as "down" | "up" | null, directionTravel: 0 }; + for (const offset of [40, 80, 100]) { + state = computeScrollHideUpdate({ + offset, + lastOffset: state.lastOffset, + maxOffset: 300, + currentlyHidden: state.hidden, + direction: state.direction, + directionTravel: state.directionTravel, + }); + } + // Today the chrome hides at ~100px on a 300px runway… + expect(state.hidden).toBe(true); + + // …then the 240px geometry release clamps the offset to the shrinking + // bottom edge (held correctly by the bottom-clamp guard)… + for (const frame of [90, 75, 60]) { + state = computeScrollHideUpdate({ + offset: frame, + lastOffset: state.lastOffset, + maxOffset: frame, + currentlyHidden: state.hidden, + direction: state.direction, + directionTravel: state.directionTravel, + }); + expect(state.hidden).toBe(true); + } + + // …and a mere 12px upward drag re-reveals, snapping ~240px of geometry + // back under the finger — the "locks to the bottom" oscillation. + const revealed = computeScrollHideUpdate({ + offset: 48, + lastOffset: state.lastOffset, + maxOffset: 60, + currentlyHidden: state.hidden, + direction: state.direction, + directionTravel: state.directionTravel, + }); + expect(revealed.hidden).toBe(false); + }); + + it("refuses to hide when the collapse budget exceeds the remaining runway", () => { + // /formulation at 390x844 (measured pre-fix): maxOffset 266 with chrome + // visible; hiding releases 182px (72px header strip + 110px reserve-pad + // shrink). At offset 100 only 166px of runway remains — hiding would clamp + // the offset straight onto the new bottom edge, so the gate must refuse. + expect( + computeScrollHideUpdate({ + offset: 100, + lastOffset: 80, + maxOffset: 266, + collapseBudget: 182, + currentlyHidden: false, + direction: "down", + directionTravel: 80, + }), + ).toEqual({ + hidden: false, + lastOffset: 100, + direction: "down", + directionTravel: 100, + }); + }); + + it("hides normally when ample runway remains below the collapse release", () => { + expect( + computeScrollHideUpdate({ + offset: 100, + lastOffset: 80, + maxOffset: 2000, + collapseBudget: 240, + currentlyHidden: false, + direction: "down", + directionTravel: 80, + }).hidden, + ).toBe(true); + }); + + it("keeps hiding when the collapse budget is not reported (back-compat)", () => { + // DocumentViewer / settings-dialog consumers report no budget; their + // behavior must be untouched even on a short scroller. + expect( + computeScrollHideUpdate({ + offset: 100, + lastOffset: 80, + maxOffset: 266, + currentlyHidden: false, + direction: "down", + directionTravel: 80, + }).hidden, + ).toBe(true); + }); + + it("blocks a hide that would land within one micro-drag of the reveal threshold", () => { + // runway (500-100) - budget 240 = 160 > 28 → allowed… + expect( + computeScrollHideUpdate({ + offset: 100, + lastOffset: 80, + maxOffset: 500, + collapseBudget: 240, + currentlyHidden: false, + direction: "down", + directionTravel: 80, + }).hidden, + ).toBe(true); + // …but with 28px or less of post-collapse runway the position would sit + // one reveal-intent drag from snapping the geometry back: refused. + expect( + computeScrollHideUpdate({ + offset: 100, + lastOffset: 80, + maxOffset: 368, + collapseBudget: 240, + currentlyHidden: false, + direction: "down", + directionTravel: 80, + }).hidden, + ).toBe(false); + }); + it("ignores an upward clamp caused by the viewport growing at the bottom", () => { const clamped = computeScrollHideUpdate({ offset: 900, From b028cbf0dfd97e817626af83895a0aa25799e2ba Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 20 Jul 2026 21:06:21 +0800 Subject: [PATCH 2/4] docs: record claude/phone-scroll-fix review in branch ledger Co-Authored-By: Claude Fable 5 --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 9c638bf1b..dbcc6fe04 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -646,3 +646,4 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-20 | Credentialed release-gate checkpoint closeout (workflow_dispatch on main `7ec25d9`; user-authorized ≤$10, single dispatch each) | 7ec25d9675dea13635fa4a895af88c93da694a42 | Credentialed half of the release gate: CI dispatch + live eval canary | CI dispatch: 9/10 jobs green (unit coverage, build, Chromium production journeys, migration replay on local Supabase emulator, production-readiness CI-safe, policy self-tests, static/safety/scope). `release-browser-matrix` (WebKit/Firefox) CANCELLED twice by main-churn: ci.yml `concurrency: CI-${ref}, cancel-in-progress: true` kills in-flight dispatch runs on every main push and this repo merges every few minutes — livelock confirmed at the 2-attempt cap; WebKit/iOS verification remains outstanding with three human options (quiet-window dispatch, the weekly scheduled run, or a one-line dedicated concurrency group for the matrix job — operational-risk change, not applied). Eval canary: golden retrieval eval FAILED 4/36 (document_recall@5 0.944, ndcg@10 0.923, force_embedding_failure_count 0, no 429s — vector layer healthy, NOT the documented vector-ptsd transient class); the July 17 dispatch PASSED this step pre-#901, so the regression window implicates #901's deterministic semantic reranking (lithium-therapy-monitoring shows three unrelated documents with byte-identical rerank scores burying the lithium guideline; two other failures add fixture-vs-corpus identity components; answer-quality subset — the July 17 failure — never ran). NO canary re-run per plan (deterministic, not transient); retrieval is clinical-path and deferred to a human decision. Provider spend ≈ $1–2 (one canary run's embeddings); matrix/CI runs $0. | actions_run_trigger dispatches + rerun_failed_jobs (attempt 2); job-level conclusions and log excerpts from runs 29675875530 (both attempts), 29675878737 (July 19 canary), and 29567502452 (July 17 baseline). No local provider calls; secrets never left GitHub Actions. | | 2026-07-20 | claude/clinical-kb-pwa-review-asi3wb (restarted; PR: ci.yml concurrency) | 79aaacf04e64f753c480dd957a81ac9be6acbb43 | CI workflow concurrency: stop main churn cancelling dispatch/schedule runs | One-line group expression: workflow_dispatch/schedule events now get a per-run concurrency group (github.run_id) while push/PR keep the shared ref group with cancel-in-progress — fixes the release-browser-matrix livelock (cancelled twice on 2026-07-19/20 by main merges mid-run; the weekly Sunday 18:00 UTC scheduled run was subject to the same cancellation). release-browser-matrix is not a required branch-protection check; pin/scope checkers do not constrain the concurrency block. Accepted side effect: deliberate runs can overlap push runs. Rollback: plain revert. | check:github-actions PASS; check:ci-scope PASS; format:check PASS (repo files; local-only .claude/settings.local.json warning is gitignored) | | 2026-07-20 | claude/clinical-kb-pwa-review-asi3wb (restarted; PR: #901 rerank regression closeout) | fd5dcd1582e1c7aa8bf92ff097575e0c68aa930d | #901 rerank regression: baseline re-eval + offline guard net + saturated-tie fix | Baseline eval-canary dispatch on remediated main b3ae061 (run 29731533081) PASSED — golden retrieval 36/36 + answer-quality green, confirming #913-#926 closed the live 4/36 regression (July 19 failing run tested the raw #901 state; fixes landed 1-8h after it, never re-evaled until now). Residual defect found by new offline repro and fixed: with byte-identical imputed fast-path primaries, selection's clamped keys tie exactly and the chunk-id fallback decides, which secondStageScore's position adjustment launders into releaseRankScore (buried the CIWA answer doc in repro); fix = contentRankScore carried on RetrievalCandidate as tie-break between clamped rerankScore and chunk id, never added to scores. Amended #901's own never-live-validated saturated-tie pin to content-rank-then-id. Guards: tests/rag-fast-path-ordering.test.ts (4 end-to-end eval-shape repros) + ranking-tuning gates (4 golden-mapped hard negatives at production weights + full-snapshot high-risk). Fixture aliases NOT changed (Step C not triggered — identity cases pass live). | Targeted 4-suite vitest 37/37; npm run test 2981 passed (sole fail = container-only pdf-extraction-budget artifact); verify:cheap green through all 18 pre-test checks incl. lint+typecheck; check:production-readiness PASS on substantive checks (2 FAILs = documented missing-secret class in this container); build + client-bundle scan + check:rag:fixtures PASS | +| 2026-07-20 | claude/phone-scroll-fix (PR #993) | f0f4c42e7 | Phone scroll "locks to bottom" regression from #964 + all-pages phone scroll audit + guardrails | Root cause: #964's phone mode-home dock made one hide-on-scroll event release ~180-260px of scroll geometry (header grid collapse + reserve-pad shrink) — more than a short mode home's remaining runway, so scrollTop clamps onto the new bottom and a 12px up-drag snaps it back (oscillation; measured 266→84px runway, 2 flips/gesture on /formulation @390×844). Invisible to all gates because the suite-wide reducedMotion:"reduce" disables the causal transitions. Fix: collapse-budget gate in computeScrollHideUpdate (hosts report the would-be geometry release via readChromeCollapseBudget; hide refused without runway to absorb it; budget-less consumers unchanged; bottom-clamp guard untouched). Bonus latent bug fixed: DocumentViewer's one-shot #main-content discovery could hold a detached node after shell remounts (viewer hide-on-scroll dead); observer now persists. ui-smoke bottom-hide scenario updated to the gate contract. New 18-test sweep tests/ui-phone-scroll.spec.ts (10 mode homes + 3 dashboard modes + 4 long routes, motion enabled, simulated PWA insets) verified to fail red pre-fix. Residual: real-device iOS momentum clamping is device-only — user PWA confirmation requested post-deploy. | vitest focused 24/24 + mobile-composer-reserve; verify:cheap 3020 green; ui-phone-scroll chromium 18/18 + webkit advisory 18/18; ui-formulation+ui-tools+ui-smoke 180/180; verify:ui 264/264; verify:pr-local green (format/build/bundle-scan/RAG fixtures). No provider calls. | From 4f2946649469d9c4a8da02521235345ffb8a27e8 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 20 Jul 2026 22:39:38 +0800 Subject: [PATCH 3/4] fix(ui): clear shell scroll container on remount; fail flip counter loudly Addresses two CodeRabbit findings on #993: - DocumentViewer kept a detached #main-content node across a two-step shell remount, leaving useHideOnScroll subscribed to a scroller that never fires. Syncing null lets it fall back to window. - installFlipCounter returned early when the header selector missed, pinning the counter at 0 so flip assertions passed vacuously. Co-Authored-By: Claude Opus 4.8 --- src/components/DocumentViewer.tsx | 5 ++++- tests/ui-phone-scroll.spec.ts | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index f7aec6d04..bf99b721e 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -1747,8 +1747,11 @@ export function DocumentViewer({ // the setState below dedups so unrelated inserts cost one no-op callback. const sync = () => { if (cancelled) return; + // Track absence too: a two-step remount would otherwise leave the state + // holding the detached node, keeping useHideOnScroll subscribed to a + // scroller that never fires. null falls back to window until the + // replacement mounts. const main = window.document.getElementById("main-content"); - if (!main) return; setShellScrollContainer((current) => (current === main ? current : main)); }; const observer = new MutationObserver(sync); diff --git a/tests/ui-phone-scroll.spec.ts b/tests/ui-phone-scroll.spec.ts index 036e10138..8ae1a47b4 100644 --- a/tests/ui-phone-scroll.spec.ts +++ b/tests/ui-phone-scroll.spec.ts @@ -102,7 +102,9 @@ async function installFlipCounter(page: Page) { const counter = { flips: 0 }; (window as unknown as { __scrollFlipCounter: typeof counter }).__scrollFlipCounter = counter; const header = document.querySelector('[data-testid="universal-header-collapse"]'); - if (!header) return; + // Fail loudly: returning early would leave the counter pinned at 0, so + // every flip assertion would pass vacuously. + if (!header) throw new Error("installFlipCounter: universal-header-collapse not found"); new MutationObserver(() => { counter.flips += 1; }).observe(header, { attributes: true, attributeFilter: ["data-scroll-hidden"] }); From b2159eecf3985a33d06049049800527ea3053907 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:32:38 +0800 Subject: [PATCH 4/4] fix(ci): overlay-aware flip counter + restore DocumentViewer line budget The answer home hides its glass header via the overlay strategy (header#search carries data-scroll-hidden; no collapse wrapper exists), so the fail-loudly flip counter must observe whichever chrome element the route renders. Condense the DocumentViewer discovery comments back under the 3166-line no-growth maintainability budget while keeping the remount rationale. Co-Authored-By: Claude Fable 5 --- src/components/DocumentViewer.tsx | 16 ++++++---------- tests/ui-phone-scroll.spec.ts | 7 +++++-- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/components/DocumentViewer.tsx b/src/components/DocumentViewer.tsx index bf99b721e..8716249f3 100644 --- a/src/components/DocumentViewer.tsx +++ b/src/components/DocumentViewer.tsx @@ -1739,18 +1739,14 @@ export function DocumentViewer({ const [shellScrollContainer, setShellScrollContainer] = useState(null); useEffect(() => { let cancelled = false; - // #main-content is the app-shell scroll container, but it does NOT reliably - // mount once: the shell can remount it around chrome-visibility/hydration - // swaps, leaving a one-shot lookup holding a detached node whose scroll - // events never fire (the phone composer then never hides). Keep observing - // for the viewer's lifetime — childList-only mutations are infrequent, and - // the setState below dedups so unrelated inserts cost one no-op callback. + // #main-content does NOT reliably mount once: the shell can remount it, + // and a one-shot lookup then holds a detached node whose scroll events + // never fire (the phone composer never hides). Observe for the viewer's + // lifetime — childList mutations are infrequent and the setState dedups. const sync = () => { if (cancelled) return; - // Track absence too: a two-step remount would otherwise leave the state - // holding the detached node, keeping useHideOnScroll subscribed to a - // scroller that never fires. null falls back to window until the - // replacement mounts. + // Track absence too: mid-remount, null falls back to window until the + // replacement mounts (a stale detached node would never fire again). const main = window.document.getElementById("main-content"); setShellScrollContainer((current) => (current === main ? current : main)); }; diff --git a/tests/ui-phone-scroll.spec.ts b/tests/ui-phone-scroll.spec.ts index 8ae1a47b4..c9afad82a 100644 --- a/tests/ui-phone-scroll.spec.ts +++ b/tests/ui-phone-scroll.spec.ts @@ -101,10 +101,13 @@ async function installFlipCounter(page: Page) { await page.evaluate(() => { const counter = { flips: 0 }; (window as unknown as { __scrollFlipCounter: typeof counter }).__scrollFlipCounter = counter; - const header = document.querySelector('[data-testid="universal-header-collapse"]'); + // Collapse hosts flip data-scroll-hidden on the grid wrapper; overlay + // hosts (the answer home's glass bar) flip it on header#search itself. + const header = + document.querySelector('[data-testid="universal-header-collapse"]') ?? document.querySelector("header#search"); // Fail loudly: returning early would leave the counter pinned at 0, so // every flip assertion would pass vacuously. - if (!header) throw new Error("installFlipCounter: universal-header-collapse not found"); + if (!header) throw new Error("installFlipCounter: no scroll-hide chrome element found"); new MutationObserver(() => { counter.flips += 1; }).observe(header, { attributes: true, attributeFilter: ["data-scroll-hidden"] });