fix(client-router): reuse the response downloaded by prefetch on navigation - #17550
Open
Mohith26 wants to merge 3 commits into
Open
fix(client-router): reuse the response downloaded by prefetch on navigation#17550Mohith26 wants to merge 3 commits into
Mohith26 wants to merge 3 commits into
Conversation
…gation When ClientRouter navigates to a URL that its own prefetch downloaded moments earlier, fetchHTML() used the default cache mode. On-demand rendered pages carry no cache headers, so the browser downloaded the page a second time. The prefetch script now records when each URL was prefetched in a small shared registry, and the router fetches with cache: 'force-cache' when the URL was prefetched within the last five minutes, matching Chromium's own reuse window for prefetched responses (kPrefetchReuseMins). Form submissions are excluded so they always reach the server. The registry also normalizes URLs consistently (resolved against location.href, hash stripped), fixing the mismatch where prefetch() stored relative URLs while lookups used absolute ones. Fixes withastro#17549
Adds unit tests for the registry shared between the prefetch script and the view transitions router: URL normalization (relative vs absolute, hash stripping, query preservation), the five-minute reuse window, and the distinction between deduplication (hasBeenPrefetched) and reuse eligibility (wasPrefetchedRecently).
🦋 Changeset detectedLatest commit: 2310fb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 399 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
the
pkg: astro
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
|
Thanks for picking this up, and for reading the issue closely. The mechanism, the 5 minute constant, the I applied your patch locally and measured it against the reproduction. The fix works. It also does one thing you probably did not intend, and that part I would fix before this lands. Setup for everything below. The repro from the issue ( The reuse entry is never consumed, so this is a five minute stale HTML windowNothing removes a URL from Control, stock 7.1.6, hover
The double download is the bug from the issue. The second visit renders fresh, which is correct. Same sequence with your branch:
The first click is the fix working, and it is a clean win. After that The second hover appends no new The justification in my issue ("the prefetch had already fetched exactly that copy and the user is one click behind it") only covers the first navigation. Past that this is a silent client side HTML cache on on-demand rendered pages, which is the one thing a cart, an inbox or a dashboard cannot have. A user who submits a form and navigates back to a page they hovered four minutes ago sees pre-submit HTML. Consuming the entry fixes it and keeps the whole winThe key can keep meaning "already prefetched" while the value means "still reusable": // registry.ts, replacing wasPrefetchedRecently()
export function consumePrefetchReuse(url: string): boolean {
const key = normalizePrefetchUrl(url);
const prefetchedAt = prefetchedUrls.get(key);
if (prefetchedAt === undefined || Date.now() - prefetchedAt >= PREFETCH_REUSE_WINDOW) return false;
prefetchedUrls.set(key, 0); // still deduped by hasBeenPrefetched(), no longer reusable
return true;
}
// router.ts
if (!init?.method && consumePrefetchReuse(href)) fetchInit.cache = 'force-cache';Measured with that applied:
First click still served from cache, second visit back to normal. One known trade-off worth a comment: a navigation aborted mid-fetch and retried loses the reuse. That is a missed optimization, not a correctness problem. Whether The PR does more than it claims, and
|
clientPrerender: true |
initiatorType | transferSize | deliveryType |
|---|---|---|---|
| stock 7.1.6, click | fetch |
7027 | "" |
| your branch, click | fetch |
0 | cache |
So today clientPrerender users pay for a speculation rules prefetch, a prerender, and then a full download on click, and this PR fixes that too. Worth saying in the description, it is a stronger result than the one you are claiming. The staleness above applies there as well, since it is the same registry entry.
Test coverage
The ten new tests cover the Map, and you are upfront that the wiring itself is not covered. I think that gap is worth closing in this PR rather than after it, because the Playwright suite already has everything needed:
e2e/prefetch.test.ts:9-18already records every request throughpage.on('request').build()pluspreview()against an SSR fixture is an established pattern, seee2e/server-islands.test.tsande2e/csp-server-islands.test.ts.playwright.config.jsruns Chrome Stable only, so theperformance.getEntriesByType('resource')assertions above transfer directly.
An output: 'server' fixture with <ClientRouter /> and a page rendering a timestamp covers it: hover, wait past the 80 ms debounce, click, assert deliveryType === 'cache' and that the rendered timestamp is the prefetch's. Two more asserts are cheap and would have caught the issue above: a second visit gets a fresh render, and a form POST still reaches the server.
One thing you do not need to worry about: I checked whether the two entry points really share one instance of the registry, since separate Maps would make this a silent no-op with green unit tests. In a production build everything lands in a single client chunk with a single Map, so the sharing holds.
Smaller things
- Name the
no-cacheinteraction in the description. The issue's "honest downside" coversmax-age=0, must-revalidate. The sharper case isCache-Control: no-cachewith anETag, the canonical "SSR HTML, always revalidate" setup, which works well today: prefetch stores it, the click revalidates, the server answers 304, no body crosses the wire. This PR turns that into a skipped revalidation. Bounded to one navigation seconds after its own prefetch it is defensible and I would still argue for no opt-out. Left as a five minute window it is the kind of thing a maintainer will wantprefetch.reusePrefetchedResponsefor. Either way it belongs in the PR body rather than only in the issue. - One normalization instead of two.
prefetch()still doesurl = url.replace(/#.*/, '')atprefetch/index.ts:222and the registry normalizes again. Usingurl = normalizePrefetchUrl(url)there makes the string handed to<link href>and to the speculation rules identical to the registry key, drops a regex, and removes a latent<base href>discrepancy, sincelink[href]resolves againstdocument.baseURIwhile the registry resolves againstlocation.href. The existing e2e assertions usehref$=suffix matching so absolute hrefs still pass. - The new call sits inside
fetchHTML's catch-all. A throw anywhere in thattryreturns null, and null makes the router fall back to a full page load.wasPrefetchedRecently()callsnew URL(). It cannot throw forpreparationEvent.to.href, but hoisting the call above thetrycosts nothing and keeps a hard reload off that path for good. prefetch: falsestill pays for this.router.tsimports the registry unconditionally, so with prefetch off every navigation runs anew URL()and a map lookup that can never be true.__PREFETCH_DISABLED__is only substituted intoClientRouter.astro, so gating it means widening that transform torouter.ts. Probably not worth it, just flagging that I looked.- Bundle rationale. The comment in
registry.tssays importingprefetch/index.tsfrom the router would pull the prefetch script into bundles that do not use it.ClientRouter.astro:151-153already importsinitfrom it, so that is only true for a bareastro:transitions/clientimport that just usesnavigate(). Still a good reason to keep the module separate, just a narrower one than the comment suggests. - Docs. The prefetch guide describes navigation behaviour, so this probably wants a companion
withastro/docsPR, or a line saying it does not.
Happy to help verify anything here, and the repro is set up for it if you want the exact sequence.
iseraph-dev
reviewed
Jul 29, 2026
iseraph-dev
left a comment
iseraph-dev
left a comment
Contributor
There was a problem hiding this comment.
Suggestions for the one blocking item from my comment above, plus the rename it pulls through the tests. They are written to be committed as a batch (Add suggestion to batch on each, then Commit suggestions), which keeps them in a single commit and leaves CI green: dropping wasPrefetchedRecently without the test edits would fail the unit suite.
Everything else in my comment is a judgement call for you, so I have not put it in a suggestion. Take, edit or ignore any of these freely.
Comment on lines
+41
to
+48
| /** | ||
| * Whether a URL was prefetched recently enough that the response the prefetch downloaded | ||
| * can be reused without revalidation, mirroring how browsers treat their own prefetches. | ||
| */ | ||
| export function wasPrefetchedRecently(url: string): boolean { | ||
| const prefetchedAt = prefetchedUrls.get(normalizePrefetchUrl(url)); | ||
| return prefetchedAt !== undefined && Date.now() - prefetchedAt < PREFETCH_REUSE_WINDOW; | ||
| } |
Contributor
There was a problem hiding this comment.
This is the one change I would not merge without. Measured on your branch against the reproduction: after one hover, /about served the same T=0 body on three consecutive visits (transferSize: 0, deliveryType: "cache" each time) while / kept rendering fresh. Stock 7.1.6 renders fresh on the second visit, so this is new behaviour rather than something the PR inherits. Details and tables in my comment above.
Consuming the entry keeps the whole win: with this applied the first click was still 0 B / cache and the second visit went back to the network. Known trade-off worth knowing about: a navigation aborted mid-fetch and retried loses the reuse, which is a missed optimization rather than a correctness problem.
Suggested change
| /** | |
| * Whether a URL was prefetched recently enough that the response the prefetch downloaded | |
| * can be reused without revalidation, mirroring how browsers treat their own prefetches. | |
| */ | |
| export function wasPrefetchedRecently(url: string): boolean { | |
| const prefetchedAt = prefetchedUrls.get(normalizePrefetchUrl(url)); | |
| return prefetchedAt !== undefined && Date.now() - prefetchedAt < PREFETCH_REUSE_WINDOW; | |
| } | |
| /** | |
| * Whether a URL was prefetched recently enough that the response the prefetch downloaded | |
| * can be reused without revalidation, mirroring how browsers treat their own prefetches. | |
| * | |
| * The reuse is one-shot. `canPrefetchUrl()` refuses to prefetch a URL it already tracks, and | |
| * `force-cache` does not refresh a cache hit, so leaving the timestamp in place would replay | |
| * the same prefetched body for every navigation to that URL until the window expires. | |
| */ | |
| export function consumePrefetchReuse(url: string): boolean { | |
| const key = normalizePrefetchUrl(url); | |
| const prefetchedAt = prefetchedUrls.get(key); | |
| if (prefetchedAt === undefined || Date.now() - prefetchedAt >= PREFETCH_REUSE_WINDOW) { | |
| return false; | |
| } | |
| // Keep the key so `hasBeenPrefetched()` still deduplicates, but drop the reusable timestamp. | |
| prefetchedUrls.set(key, 0); | |
| return true; | |
| } |
| @@ -1,4 +1,5 @@ | |||
| import { internalFetchHeaders } from 'virtual:astro:adapter-config/client'; | |||
| import { wasPrefetchedRecently } from '../prefetch/registry.js'; | |||
Contributor
There was a problem hiding this comment.
Suggested change
| import { wasPrefetchedRecently } from '../prefetch/registry.js'; | |
| import { consumePrefetchReuse } from '../prefetch/registry.js'; |
| // downloaded. On-demand rendered pages typically carry no cache headers, so with the | ||
| // default cache mode the browser would download the page a second time (#17549). | ||
| // Form submissions must always reach the server, so they never reuse the cache. | ||
| if (!init?.method && wasPrefetchedRecently(href)) { |
Contributor
There was a problem hiding this comment.
Suggested change
| if (!init?.method && wasPrefetchedRecently(href)) { | |
| if (!init?.method && consumePrefetchReuse(href)) { |
Comment on lines
+3
to
+8
| import { | ||
| hasBeenPrefetched, | ||
| normalizePrefetchUrl, | ||
| recordPrefetch, | ||
| wasPrefetchedRecently, | ||
| } from '../../../dist/prefetch/registry.js'; |
Contributor
There was a problem hiding this comment.
Import list re-sorted so biome's organize-imports stays happy.
Suggested change
| import { | |
| hasBeenPrefetched, | |
| normalizePrefetchUrl, | |
| recordPrefetch, | |
| wasPrefetchedRecently, | |
| } from '../../../dist/prefetch/registry.js'; | |
| import { | |
| consumePrefetchReuse, | |
| hasBeenPrefetched, | |
| normalizePrefetchUrl, | |
| recordPrefetch, | |
| } from '../../../dist/prefetch/registry.js'; |
Comment on lines
+50
to
+83
| describe('wasPrefetchedRecently()', () => { | ||
| it('matches an absolute URL with a hash after prefetching a relative URL', () => { | ||
| // `prefetch()` can be handed a relative URL, while the router looks up | ||
| // `preparationEvent.to.href`, which is absolute and may carry a hash. | ||
| recordPrefetch('/relative-vs-absolute'); | ||
| assert.equal(wasPrefetchedRecently('https://example.com/relative-vs-absolute#section'), true); | ||
| }); | ||
|
|
||
| it('matches a relative URL after prefetching an absolute URL', () => { | ||
| recordPrefetch('https://example.com/absolute-vs-relative'); | ||
| assert.equal(wasPrefetchedRecently('/absolute-vs-relative'), true); | ||
| }); | ||
|
|
||
| it('does not match URLs that were never prefetched', () => { | ||
| assert.equal(wasPrefetchedRecently('/never-prefetched'), false); | ||
| }); | ||
|
|
||
| it('does not match a URL with a different query string', () => { | ||
| recordPrefetch('/list?page=1'); | ||
| assert.equal(wasPrefetchedRecently('/list?page=2'), false); | ||
| }); | ||
|
|
||
| it('stops matching once the reuse window has passed', () => { | ||
| const start = 1_000_000; | ||
| mock.timers.enable({ apis: ['Date'], now: start }); | ||
| recordPrefetch('/expires'); | ||
|
|
||
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW - 1); | ||
| assert.equal(wasPrefetchedRecently('/expires'), true); | ||
|
|
||
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW); | ||
| assert.equal(wasPrefetchedRecently('/expires'), false); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Renames the block and adds the case the change exists for. Note that the original stops matching once the reuse window has passed test has to be split in two: it asserted true just inside the window and false at the boundary on the same URL, and once the first call consumes the entry the second assertion would pass for the wrong reason. Two tests keep both edges honest.
Suggested change
| describe('wasPrefetchedRecently()', () => { | |
| it('matches an absolute URL with a hash after prefetching a relative URL', () => { | |
| // `prefetch()` can be handed a relative URL, while the router looks up | |
| // `preparationEvent.to.href`, which is absolute and may carry a hash. | |
| recordPrefetch('/relative-vs-absolute'); | |
| assert.equal(wasPrefetchedRecently('https://example.com/relative-vs-absolute#section'), true); | |
| }); | |
| it('matches a relative URL after prefetching an absolute URL', () => { | |
| recordPrefetch('https://example.com/absolute-vs-relative'); | |
| assert.equal(wasPrefetchedRecently('/absolute-vs-relative'), true); | |
| }); | |
| it('does not match URLs that were never prefetched', () => { | |
| assert.equal(wasPrefetchedRecently('/never-prefetched'), false); | |
| }); | |
| it('does not match a URL with a different query string', () => { | |
| recordPrefetch('/list?page=1'); | |
| assert.equal(wasPrefetchedRecently('/list?page=2'), false); | |
| }); | |
| it('stops matching once the reuse window has passed', () => { | |
| const start = 1_000_000; | |
| mock.timers.enable({ apis: ['Date'], now: start }); | |
| recordPrefetch('/expires'); | |
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW - 1); | |
| assert.equal(wasPrefetchedRecently('/expires'), true); | |
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW); | |
| assert.equal(wasPrefetchedRecently('/expires'), false); | |
| }); | |
| }); | |
| describe('consumePrefetchReuse()', () => { | |
| it('matches an absolute URL with a hash after prefetching a relative URL', () => { | |
| // `prefetch()` can be handed a relative URL, while the router looks up | |
| // `preparationEvent.to.href`, which is absolute and may carry a hash. | |
| recordPrefetch('/relative-vs-absolute'); | |
| assert.equal(consumePrefetchReuse('https://example.com/relative-vs-absolute#section'), true); | |
| }); | |
| it('matches a relative URL after prefetching an absolute URL', () => { | |
| recordPrefetch('https://example.com/absolute-vs-relative'); | |
| assert.equal(consumePrefetchReuse('/absolute-vs-relative'), true); | |
| }); | |
| it('does not match URLs that were never prefetched', () => { | |
| assert.equal(consumePrefetchReuse('/never-prefetched'), false); | |
| }); | |
| it('does not match a URL with a different query string', () => { | |
| recordPrefetch('/list?page=1'); | |
| assert.equal(consumePrefetchReuse('/list?page=2'), false); | |
| }); | |
| it('reuses a prefetch only once', () => { | |
| // Nothing ever records a fresher copy, so a second navigation has to go back to the | |
| // network instead of replaying the body the first one already used. | |
| recordPrefetch('/reuse-once'); | |
| assert.equal(consumePrefetchReuse('/reuse-once'), true); | |
| assert.equal(consumePrefetchReuse('/reuse-once'), false); | |
| }); | |
| it('still reuses just inside the window', () => { | |
| const start = 1_000_000; | |
| mock.timers.enable({ apis: ['Date'], now: start }); | |
| recordPrefetch('/just-inside'); | |
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW - 1); | |
| assert.equal(consumePrefetchReuse('/just-inside'), true); | |
| }); | |
| it('stops matching once the reuse window has passed', () => { | |
| const start = 1_000_000; | |
| mock.timers.enable({ apis: ['Date'], now: start }); | |
| recordPrefetch('/expires'); | |
| mock.timers.setTime(start + PREFETCH_REUSE_WINDOW); | |
| assert.equal(consumePrefetchReuse('/expires'), false); | |
| }); | |
| }); |
| // Still deduplicated by the prefetch script... | ||
| assert.equal(hasBeenPrefetched('/old-prefetch'), true); | ||
| // ...but too old for the router to reuse without revalidation. | ||
| assert.equal(wasPrefetchedRecently('/old-prefetch'), false); |
Contributor
There was a problem hiding this comment.
Suggested change
| assert.equal(wasPrefetchedRecently('/old-prefetch'), false); | |
| assert.equal(consumePrefetchReuse('/old-prefetch'), false); |
Mohith26
added a commit
to Mohith26/astro
that referenced
this pull request
Jul 29, 2026
Review follow-up from withastro#17550: reusing the prefetched response for the whole five-minute window froze on-demand rendered pages on their first prefetched body (measured by the reporter: three visits, one render). consumePrefetchReuse() zeroes the entry on first use so exactly one navigation reuses the prefetch, while hasBeenPrefetched() keeps deduplicating. Also hoists the registry call above fetchHTML's catch-all so a registry error can never cause a full page load, and uses the registry normalization in prefetch() itself so the link href, speculation rules, and registry key are byte-identical (resolving against location.href rather than a divergent <base href>).
Review follow-up from withastro#17550: reusing the prefetched response for the whole five-minute window froze on-demand rendered pages on their first prefetched body (measured by the reporter: three visits, one render). consumePrefetchReuse() zeroes the entry on first use so exactly one navigation reuses the prefetch, while hasBeenPrefetched() keeps deduplicating. Also hoists the registry call above fetchHTML's catch-all so a registry error can never cause a full page load, and uses the registry normalization in prefetch() itself so the link href, speculation rules, and registry key are byte-identical (resolving against location.href rather than a divergent <base href>).
Mohith26
force-pushed
the
fix/client-router-prefetch-reuse
branch
from
July 29, 2026 19:11
caa99f1 to
2310fb7
Compare
Author
|
This is a phenomenal review, thank you for measuring everything - you were right on all counts, and the staleness finding especially. All three fixes are in as of 2310fb7:
Three new unit tests pin the one-shot behavior (consume once, still deduped, re-prefetch restores reuse); full unit suite is green (3221). The PR body now names the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #17549, reported by @iseraph-dev, following the issue's "no new API" design preference.
The ClientRouter re-downloaded pages its own prefetch had just fetched:
fetchHTML()used the default cache mode, and SSR pages carry no cache headers, so navigation always refetched.The fix adds a tiny dependency-free
prefetch/registry.tsshared by both bundles that records when each URL was prefetched, under one normalization (now used byprefetch()itself too, so the link href, speculation rules, and registry key are byte-identical and resolve againstlocation.hrefrather than a divergent<base href>).fetchHTML()usescache: 'force-cache'when the URL was prefetched within Chromium's 5-minute prefetch-reuse window, gated on!init?.methodso form POSTs always hit the server.Per @iseraph-dev's measured review: the reuse entry is consumed on first navigation (
consumePrefetchReuse()zeroes the timestamp,hasBeenPrefetched()keeps deduplicating), so this is "reuse the prefetch once", not a five-minute client-side HTML cache; later visits revalidate normally. The registry call is hoisted abovefetchHTML's catch-all so a registry error can never fall back to a full page load. WhetherhasBeenPrefetched()should itself expire (allowing re-prefetch) was considered and left out deliberately.Two scope notes worth naming: this also fixes the same double-download for
experimental.clientPrerenderusers (speculation-rules prefetches populate the HTTP cache the same way, measured in the review); and forCache-Control: no-cache+ ETag setups the single reused navigation skips one revalidation, bounded to seconds after the prefetch, which we think is defensible without an opt-out.Thirteen unit tests lock the registry semantics including one-shot consumption; the unit suite passes with zero new failures, biome/eslint gates clean. The
force-cachewiring itself is only verifiable in the Playwright e2e suite; happy to add the SSR fixture e2e from the review's pointers if you want it in this PR.