feat: device-adaptive link prefetch, viewport-safe on mobile - #594
Conversation
|
Design rationale: why adaptive default, dwell-gate, and where the coverage lives A few decisions worth recording while they are fresh. Why the default is adaptive rather than just flipping to viewport. Why the viewport path needed a dwell + cancel, not just the existing observer. The old viewport mode fired immediately on intersection with no cancel. On a long, fast-scrolled mobile list that warms every link the scroll passes, which is exactly the network-tab bloat we do not want. I read how Astro, Next, Nuxt, Remix, TanStack, and Turbo all handle this: every one of them either delays the start (dwell) or cancels on exit, most do both. We now do both (250ms dwell, cancelled on scroll-out). The guiding rule is snappy without bloating the network tab, and when those conflict the gate under-fetches. Why touchstart still warms the tapped link even in viewport mode. On touch the default is now viewport, but the link the user actually taps is about to be navigated anyway, so warming it on Where the coverage lives, and a regression note. The adaptive resolution is unit-tested with a stubbed Bun matrix is N/A. This is browser-only client code in |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went over the dwell/cancel bookkeeping hard, since over-fetch is the whole point of this change. Two things on the teardown paths, one real.
-
The
webjs:navigatere-scan disconnects the observer but never cancels pending dwell timers (refreshPrefetchObservers, router-client.js:1413). If an anchor had a pending timer and the soft-nav swap removes it from the DOM, the re-observe()of the fresh set never delivers a callback for the removed node, so the cancel branch never runs and the orphaned timer fires a prefetch for a stale URL. Bounded (swallowed + cache-deduped) but it still breaks the cancel-on-exit guarantee across a soft nav, which is exactly what we promised. -
prefetch()(router-client.js:1191) has noenabledguard. TodaydisableClientRoutercancels the view timers before returning so it is not reachable through that path, but the queue drain and a leftover hover timer are the same shape, and finding 1 widens it. A one-line guard closes all of them.
Everything else holds: the /2g$/ regex has no false positives, the matchMedia-absent fallback keeps the historical intent default, the touchstart branch returns early for none/render so it does not over-fetch, and the unit/browser tests are counterfactual-sound.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass, zoomed in on the round-1 fix and cross-file consistency. The re-scan now clears the dwell timers and re-observes, so on-screen anchors re-arm (a real IntersectionObserver redelivers the initial intersection on observe) while a swapped-out anchor's timer is dropped. The enabled guard in prefetch() doesn't touch any normal warm path (every caller already runs only while enabled, and the unit suite auto-enables on import). Test exports line up, the browser tests don't leak state across cases (distinct URLs, teardown disables + nulls the observer), and the new comments are clean on the punctuation rules. Nothing left to fix.
|
Context: a real interaction CI caught (adaptive default vs headless runners + streamed pages) The first CI run went red on E2E and it was a genuine find, not flake, so worth writing down. Headless Puppeteer reports
I verified both by forcing Worth flagging for later (out of scope here): viewport-prefetching a deliberately-streamed (slow) page buffers the whole thing speculatively, which both defeats its streaming on the eventual click and spends server work on a slow boundary the user may never reach. For request-stable pages that is a fine tradeoff; for a genuinely slow streamed page it leans against the no-over-fetch rule. A future option is to not prefetch-buffer a streaming response (detect the stream and skip caching it). Not doing it in this PR. |
The client router's default prefetch was always 'intent' (hover/focus/
touch-start). On touch there is no hover and touchstart fires at tap
time, so the prefetch raced the navigation and the fast-by-default
promise did not hold on mobile.
Make the default device-adaptive: 'intent' on a hover-capable fine
pointer, 'viewport' on touch (matchMedia('(hover: hover) and (pointer:
fine)'), not a UA sniff). A per-link data-prefetch always overrides.
Guard the viewport path against over-fetch the way Astro/Next/Nuxt/
Remix/TanStack/Turbo all do: a 250ms dwell before a viewport link warms,
cancelled on scroll-out, so a fast scroll through a long list spends no
requests. Keep touchstart as an extra warm for the tapped link. Broaden
the connection gate to skip 2g effectiveType, not just Save-Data.
Drive a stubbed IntersectionObserver to prove the over-fetch gate in a real browser: a viewport link warms only after a dwell, a fast scroll-through (enter then exit before the dwell) spends no request, and a cancelled anchor can still warm on a later settled re-entry.
Update agent-docs/advanced.md, the docs-site client-router page, and the root AGENTS.md client-router summary: the default is now intent on a hover pointer and viewport on touch, the viewport path is dwell-gated and cancels on scroll-out, and the connection gate also skips 2g.
Prove the viewport path end-to-end against the real server wire (the browser test stubs fetch): a data-prefetch=viewport link below the fold issues no prefetch on load, and warms exactly once after it scrolls in and dwells.
Self-review found two over-fetch holes in the dwell-timer bookkeeping. The webjs:navigate re-scan disconnected the observer but left pending dwell timers armed, so a timer for an anchor the soft-nav swap removed fired a prefetch for a stale URL (no exit callback ever comes for a gone node). Clear the timers on the re-scan too. Also guard prefetch() with the enabled flag so any leftover hover/queue/dwell timer that fires after disableClientRouter cannot issue a fetch.
…fault Headless Puppeteer in CI reports hover:none, so the adaptive default resolves to viewport there. That broke two classes of existing e2e: the hover-prefetch tests (a bare link no longer warms on hover) and the progressive-streaming tests (the trigger link got viewport-prefetched and buffered, so the soft-nav consumed a resolved page and the live fallback never showed). Pin the hover tests to data-prefetch=intent, opt the streaming triggers out with data-no-prefetch, and add a touch-emulated test that a bare link defaults to viewport prefetch (#530).
The previous attempt used per-test data-prefetch pins and emulateMediaFeatures, but CI Chromium does not support the hover media feature (it threw 'Unsupported media feature: hover'), and pinning each link did not address the real cause: on a no-hover runner the adaptive default makes every link viewport, so a page gets passively warmed into the prefetch cache before a hover test can observe a fresh fetch. Install a matchMedia shim via evaluateOnNewDocument that models a desktop pointer by default (restoring main's behaviour for the existing hover and streaming tests with no per-test hacks) and reads window.__webjsForceTouch so the touch test opts into no-hover. The shim is plain JS, so it works on every Chromium and controls the modality deterministically.
037d988 to
0e60413
Compare
1 participant
Closes #530
The client router's default link prefetch was always
intent(hover/focus/touch-start). On touch there is no hover andtouchstartfires at tap time, so the prefetch raced the navigation and the fast-by-default promise did not hold on mobile.What changed
intenton a hover-capable fine pointer andviewporton touch, detected withmatchMedia('(hover: hover) and (pointer: fine)')(not a UA sniff). A per-linkdata-prefetchalways overrides.touchstartstill warms the tapped link (a single request for a link about to be navigated), even when its mode isviewport.effectiveType, not just Save-Data / prefers-reduced-data.prefetch()is also guarded by theenabledflag so no leftover timer fetches after teardown.Design north star (per the issue + maintainer): snappy, but never at the cost of bloating the client network tab. When snappy and extra bytes conflict, under-fetch wins.
Test plan
packages/core/test/routing/router-client.test.js). Full node suite 2615 pass.packages/core/test/routing/browser/prefetch-viewport.test.js). Full routing browser suite 61 pass. Re-scan-cancel counterfactual verified (reverting the fix reds exactly that test).data-prefetch="viewport"link below the fold warms only after it scrolls in and dwells, against the real server wire; the existing hover prefetch e2e tests still pass (no desktop regression).agent-docs/advanced.md, the docs-site client-router page, and the root AGENTS.md client-router summary.router-client.js, nonode:*/ server / serializer / listener surface).Self-review ran 2 rounds; round 1 found two over-fetch holes in the dwell-timer teardown (fixed in eea7afa), round 2 clean.
Update: CI-caught regression (fixed)
Headless Puppeteer reports
hover: none, so the adaptive default resolved toviewporton CI and broke the existing hover-prefetch e2e tests and the progressive-streaming e2e tests (the streamed-page trigger got viewport-prefetched and buffered). Pinned the hover tests todata-prefetch="intent", opted the streaming triggers out withdata-no-prefetch, and added a touch-emulated test that a bare link defaults to viewport prefetch. Verified locally by forcingemulateMediaFeaturesno-hover. See the context comment for the full writeup, including the noted out-of-scope follow-up (not prefetch-buffering a streaming response).