fix(link): cancel in-flight <Link> prefetch setup on cache invalidation and scope navigation cancellation to its destination - #2737
Conversation
…on and scope navigation cancellation to its destination A <Link> prefetch whose setup was still awaiting its module imports survived router.refresh(): the invalidation never advanced linkPrefetchNavigationEpoch, so the resumed closure repopulated a navigation-reusable entry from the pre-refresh cache generation. The same global counter also cancelled a pending prefetch for /a when a navigation went to /b, making viewport and intent prefetches timing-dependent for no duplicate-request benefit. Replace the counter with the sticky, destination-scoped pending-setup registry that cloudflare#2709 introduced for router.prefetch(), moved to a dependency-free shims/internal/app-prefetch-setup.ts so link.tsx can register synchronously without pulling navigation.ts onto its startup path. Fixes cloudflare#2718
|
@codex review |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4e8e71ba7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ld the setup token through staged shell work Codex review on cloudflare#2737 found two gaps in the registry wiring: - The token destination was derived from the basePath-prefixed browser href, which toAppPrefetchDestination prefixes again, so under a non-empty basePath no navigation key ever matched and same-route cancellation silently stopped working. Derive it from the app-relative prefetchHref, matching what navigations pass in. - The outer closure unregistered the token while the fire-and-forget loading shell (and the staged shell-first payload) were still awaiting createRscRequestUrl, so an invalidation in that window found no token and the helper repopulated the invalidated cache. Hold the token until staged work settles and re-check cancellation inside the helpers before their cache writes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… boundary Replace the module mock that gated createRscRequestUrl with timing derived from the stubbed fetch itself: a high-priority intent prefetch issues its full-payload fetch synchronously inside the setup closure, so a microtask scheduled from that call lands after the closure returns and before the shell helper's digest-backed createRscRequestUrl resolves. No internal collaborators are mocked.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Overview
<Link>prefetch setup the same cancellation semanticsrouter.prefetch()gained in #2709linkPrefetchNavigationEpochwith the sticky, destination-scoped pending-setup registry, moved to a shared dependency-free moduleshims/internal/app-prefetch-setup.tsowns registration and cancellation;navigation.tsowns the policy of when to cancelrouter.refresh()can no longer be undone by a late<Link>prefetch; navigations no longer drop unrelated pending prefetchesCloses #2718.
Why
A prefetch whose asynchronous setup is still in flight must not commit state from a cache generation that has since been invalidated, and a navigation should only cancel prefetch setup it is about to duplicate. #2709 built both guarantees for
router.prefetch()as a registry of sticky per-destination tokens, but<Link>'sprefetchUrlkept its own global counter with neither property:invalidatePrefetchCache()voids the whole generation, so every pending setup must die with it<Link>setup awaiting its lazy module imports resumed afterrouter.refresh()and repopulated a navigation-reusable entry from the pre-refresh generation/bsilently dropped a pending prefetch for/athat nothing else would ever fetchlink.tsxloadsnavigation.tslazily to keep the navigation runtime off Link's synchronous startup pathshims/internal/module both files import statically, following the layeringapp-route-prefetch-policy.tsestablished in #2709Sticky per-token cancellation is what makes destination scoping sound: a navigation to
/afollowed by one to/bleaves a pending/aprefetch cancelled, where comparing against a "current destination" value would not.What changed
router.refresh()while a<Link>prefetch awaits its module imports<Link>prefetch<Link>prefetch is pendinghistory.pushState(no request issued)<Link>prefetchesnotifyAppNavigationStartrouter.prefetch()(all #2709 scenarios)navigation.tstests/prefetch-cache.test.tsunchanged and greenMaintainer review path
packages/vinext/src/shims/internal/app-prefetch-setup.tsfor the moved registry and the layering constraint its doc comment states (why it must stay dependency-free).packages/vinext/src/shims/navigation.tsfor the mechanical switch to imports; cancellation policy call sites (notifyAppNavigationStart,invalidatePrefetchCache) are unchanged.packages/vinext/src/shims/link.tsxfor the substantive change:prefetchUrlregisters a token synchronously before its firstawait, re-checkscancelledafter the module-loading race and again after the RSC-URL awaits before any cache write, and unregisters infinally. The navigation-runtime hook reverts to plainnotifyLinkNavigationStartsince cancellation no longer rides it.tests/link-navigation.test.tsfor the three regression shapes.Validation
prefetchUrlvia therenderIsolatedLinkharness:vp test rungreen on the three directly affected suites (263 tests) and on the 17 suites importing the navigation or link shims (1,986 tests);vp checkclean on all changed files; pre-commit full check and knip passed.Risk / compatibility
shims/internal/and the removed counter was module-private.url-utils,utils/external-url) were already on Link's synchronous path.<Link>prefetches for other destinations. This trades a possible duplicate request in zero scenarios (nothing else fetches those routes) for prefetch reliability, matching the tradeoff already accepted forrouter.prefetch()in fix(navigation): reuse router.prefetch payloads during navigation #2709.References
router.prefetch(); its doc comment deferred this unificationtests/prefetch-cache.test.ts