From 03fc2954c2f67360b96162393497eae568f06d21 Mon Sep 17 00:00:00 2001 From: Archmonger Date: Sun, 20 Sep 2026 11:31:36 +0000 Subject: [PATCH] fix: make scroll restoration robust against slow/loaded content Two issues made test_scroll_restoration_preserves_scroll flaky under CI load: 1. ScrollRestoration's restore loop used a fixed 10-animation-frame budget. After a client-side navigation the destination content is streamed in by the server, so the document can still be too short to reach the saved position, and frame cadence also collapses under CPU load. When the budget expired early, scrollTo clamped against a short/short-lived document and the position was silently dropped (observed: scroll stuck near 0). Replace the frame budget with a wall-clock window and stop as soon as the target is reached (within a small tolerance), so we keep re-asserting the position while late content grows, and never fight the user's own scroll. 2. The test navigated back with a synthetic element.click() and asserted a loose lower bound. A synthetic click can outrun the Link preventDefault handler, triggering a real anchor navigation (full page reload) that wipes the client-side scroll store; and the bottom-anchored forward link made Playwright scroll it into view, saving the page-bottom position so the lower-bound check passed without verifying real restoration. Use a native page.click (waits for actionability, so the handler is attached), place the forward link in-viewport at the target via a spacer so scroll-into-view is a no-op, return via the browser back action, and assert a near-exact restored position. --- src/js/src/components.ts | 54 ++++++++++++++++++++++++++---------- tests/test_router.py | 59 +++++++++++++++++++++++++--------------- 2 files changed, 77 insertions(+), 36 deletions(-) diff --git a/src/js/src/components.ts b/src/js/src/components.ts index 918cc67..d09906f 100644 --- a/src/js/src/components.ts +++ b/src/js/src/components.ts @@ -129,6 +129,19 @@ export function Navigate({ // unmount/remount during route transitions. const _scrollPositions: Record = {}; +// How long (wall-clock) to keep re-asserting a restored scroll position while the +// destination route's content is still streaming in from the server. A fixed +// *frame* count is unreliable because frame cadence varies with CPU load: under a +// loaded CI runner the document can still be short (content not yet rendered) when +// a small frame budget expires, so `scrollTo` clamps against the not-yet-tall page +// and the position is silently lost. A time window is robust to both fast and slow +// frame cadences. The loop stops as soon as the target is reached, so a generous +// window only buys time for slow content — it never fights the user's own scroll. +const _scrollRestoreWindowMs = 1000; + +// Sub-pixel tolerance for considering a scroll position "reached". +const _scrollTolerancePx = 2; + /** * ScrollRestoration component that saves and restores scroll positions across * client-side navigation. @@ -186,21 +199,34 @@ export function ScrollRestoration({}: ScrollRestorationProps): null { React.useEffect(() => { const key = window.location.pathname; const pos = _scrollPositions[key]; - if (pos) { - // Retry across animation frames — Preact may perform multiple - // render commits that reset scroll. - let remaining = 10; - const tryRestore = () => { - window.scrollTo(pos.x, pos.y); - if ( - (window.scrollY !== pos.y || window.scrollX !== pos.x) && - --remaining > 0 - ) { - requestAnimationFrame(tryRestore); - } - }; - requestAnimationFrame(tryRestore); + if (!pos) { + return; } + + const reached = () => + Math.abs(window.scrollY - pos.y) <= _scrollTolerancePx && + Math.abs(window.scrollX - pos.x) <= _scrollTolerancePx; + + // Retry across animation frames until the target is reached, bounded by a + // wall-clock window instead of a frame count. After a client-side navigation + // the destination content is streamed in by the server, so the document can + // still be too short to reach `pos` and scrollTo clamps against it; a small + // frame budget can also expire far too early when frame cadence drops under + // CPU load. Both cases would otherwise silently drop the restored position. + const deadline = performance.now() + _scrollRestoreWindowMs; + let frame = requestAnimationFrame(function tryRestore() { + // Stop the moment the target is in place so we never fight scrolling the + // user performs after the position has been restored. + if (reached() || performance.now() >= deadline) { + return; + } + window.scrollTo(pos.x, pos.y); + frame = requestAnimationFrame(tryRestore); + }); + + // Effect cleanup runs before the next render's effect, so a re-render (or + // unmount) cancels this attempt and no competing loops are left running. + return () => cancelAnimationFrame(frame); }); return null; diff --git a/tests/test_router.py b/tests/test_router.py index 3e5af19..01464b2 100644 --- a/tests/test_router.py +++ b/tests/test_router.py @@ -500,23 +500,27 @@ def sample(): async def test_scroll_restoration_preserves_scroll(display: DisplayFixture): - """Verify scroll position is preserved when navigating back.""" + """Verify the exact scroll position is restored when navigating back.""" @component def scroll_page(): - tall_content = [html.div({"style": {"height": "1500px"}}, f"Section {i}") for i in range(10)] - link_list = link({"to": "/other", "id": "to-other"}, "Go to other", key="to-other") + tall_content = [html.div({"style": {"height": "1500px"}}, f"Section {i}") for i in range(8)] return scroll_restoration( html.h1({"id": "scroll-page"}, "Scroll Page"), + # A short spacer places the forward link roughly 550px down, so it is + # inside the viewport when the page is scrolled to ``target_y``. That + # keeps the navigation click from scrolling the page and changing the + # position captured at navigation time. + html.div({"style": {"height": "550px"}}, "Spacer"), + link({"to": "/other", "id": "to-other"}, "Go to other", key="to-other"), *tall_content, - link_list, ) @component def other_page(): return scroll_restoration( html.h1({"id": "other-page"}, "Other Page"), - link({"to": "/", "id": "back-to-scroll"}, "Back to scroll page", key="back-to-scroll"), + html.div({"style": {"height": "2000px"}}, "Other content"), ) @component @@ -528,26 +532,37 @@ def sample(): await display.show(sample) - # Wait for the scroll page to render + # Wait for the scroll page and its tall content to be laid out so the target + # scroll position is actually reachable. await display.page.wait_for_selector("#scroll-page") - - # Scroll down 500px - await display.page.evaluate("window.scrollTo(0, 500)") - scroll_y = await display.page.evaluate("window.scrollY") - assert scroll_y >= 500, f"Expected scrollY >= 500, got {scroll_y}" - - # Navigate to /other via link + await display.page.wait_for_function("document.documentElement.scrollHeight > 10000", timeout=10000) + + # Scroll to a known position that is neither the top nor the bottom, so a + # wrong-but-plausible outcome (staying at 0, or clamping to the page bottom) + # cannot pass for the wrong reason. + target_y = 500 + await display.page.evaluate(f"window.scrollTo(0, {target_y})") + await display.page.wait_for_function(f"Math.abs(window.scrollY - {target_y}) <= 1", timeout=5000) + + # The forward link sits within the viewport at ``target_y`` (see the spacer in + # ``scroll_page``), so Playwright's scroll-into-view before the click is a + # no-op and the position we scrolled to is exactly what gets saved. We use a + # native ``page.click`` (not a synthetic ``element.click()``) because it waits + # for the element to be actionable, which gives ReactPy's ``preventDefault`` + # handler time to attach -- a synthetic click can beat the handler and trigger a + # real anchor navigation (a full reload), wiping the client-side scroll store. await display.page.click("#to-other") await display.page.wait_for_selector("#other-page") - # Navigate back to / via link - await display.page.click("#back-to-scroll") + # Return via the browser's own back action (a genuine ``popstate``). This + # exercises the real restore path without a second click that could race the + # router's event wiring. + await display.page.go_back() await display.page.wait_for_selector("#scroll-page") - # Poll for scroll restoration to apply (it runs in useLayoutEffect which - # fires synchronously after DOM commit, but the browser needs at least one - # frame to paint when scrollTo is called during the same commit). - await display.page.wait_for_function( - "window.scrollY >= 450", - timeout=5000, - ) + # The restore loop keeps re-asserting the saved position until the destination + # content is tall enough to reach it (bounded by a time window), so we expect + # a near-exact match rather than the previous loose lower bound. + await display.page.wait_for_function(f"Math.abs(window.scrollY - {target_y}) <= 5", timeout=5000) + final_y = await display.page.evaluate("window.scrollY") + assert abs(final_y - target_y) <= 5, f"Expected scrollY ~{target_y}, got {final_y}"