fix(cache): keep encoded dynamic prefetches learning-only - #2866
Conversation
|
/bigbonk review for issues |
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 |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(cache): keep encoded dynamic prefetches learning-only
Reviewed the policy change, the new hasFreshLearningOnlyPrefetchCacheEntry helper, the <Link> dedupe rewrite, and the new test. The <Link> path and its test are sound (75/75 link-navigation, 127/127 link, checks all green). One real issue.
Issue: router.prefetch() learning-only dedupe was not updated — parity gap with <Link>
packages/vinext/src/shims/navigation.ts:2789
The PR's core behavioral change is switching <Link>'s learning-only dedupe from permanent prefetched.has(cacheKey) membership to the freshness-aware hasFreshLearningOnlyPrefetchCacheEntry (which evicts + allows a refetch once the entry's window expires). But router.prefetch() still uses the old permanent-membership gate for its learning-only (!reusable) branch:
} else if (prefetched.has(cacheKey)) {
attachPrefetchInvalidationCallback(cacheKey, options?.onInvalidate);
return;
}Both <Link> and router.prefetch() resolve policy via the same resolveAutoAppRoutePrefetch and write into the same prefetch cache, so the new policy change (isCacheComponentsDynamicRoute → cacheForNavigation: false) applies to router.prefetch() too. That means:
router.prefetch("/products/foo%2Fbar")under Cache Components produces a learning-only entry (reusable === false).- A settled learning-only entry is never swept on expiry — nothing calls
deletePrefetchCacheEntryfor it until byte-budget/LRU eviction or a navigation gate (prefetched.deleteonly fires indeletePrefetchCacheEntryat navigation.ts:960 and LRU eviction at :811). - So
prefetched.has(cacheKey)staystrueforever, and repeatedrouter.prefetch()calls return early at :2789 and never refetch after expiry — the exact behavior this PR fixes for<Link>.
Per AGENTS.md ("request handling logic exists in multiple places that must stay in sync … do not leave known bugs as follow-ups — fix them in the same PR"), router.prefetch()'s learning-only branch should route through hasFreshLearningOnlyPrefetchCacheEntry (with additionalRscUrls handling as needed) so its expiry/refetch semantics match <Link>. Add a router.prefetch() unit mirroring the new Link test (pending dedupe, settled-fresh suppression, expired refetch).
If this divergence is intentional (e.g., router.prefetch() is considered explicit-intent and should stay sticky), please document why in a comment at :2789 and note it in the PR body, since it reads as an accidental miss right now.
Minor / non-blocking
- Env define shape —
String(process.env.__NEXT_CACHE_COMPONENTS) === "true"(app-route-prefetch-policy.ts:103) is correct: the plugin defines this as a boolean literal (index.ts:2361,JSON.stringify(cacheComponents ?? false)), andString(true) === "true". Matches the existingslot.tsx:68pattern. No change needed — flagging only to confirm it was checked. - Test
Date.nowspy — the new test'svi.spyOn(Date, "now")(link-navigation.test.ts:2096) isn't restored in its ownfinally, but the suiteafterEachrunsvi.restoreAllMocks()(:448), so it doesn't leak. Fine as-is. - Helper exact-key lookup —
hasFreshLearningOnlyPrefetchCacheEntrylooks up only the exactencodeCacheKey(rscUrl, ctx). That's correct for learning-only entries becausecacheForNavigation === falseentries never get rendered-path aliases (addRenderedPathAndSearchPrefetchAliasearly-returns at navigation.ts:1447), so there's no alias key to miss.
Overall the encoded-slash learning-only fix for <Link> is well-targeted and tested; the blocking concern is the un-mirrored router.prefetch() path.
|
Review posted to PR #2866. Summary of my findings: Review SummaryI examined the policy change, the new One blocking issue
Non-blocking (verified, no change needed)
|
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): keep encoded dynamic prefetches learning-only
Reviewed both commits — the original learning-only policy change (fb136ef) and the follow-up that closed the router.prefetch() parity gap (7c5c680). The core <Link> + router.prefetch() fix is well-targeted, correctly freshness-aware, and tested (148/148 across link-navigation + prefetch-cache, vp check green on all changed source files).
Blocking issue from the earlier review is resolved
The prior review flagged that router.prefetch()'s learning-only branch still used the permanent prefetched.has(cacheKey) gate. Commit 7c5c680 routes it through hasFreshLearningOnlyPrefetchCacheEntry (navigation.ts:2789) and adds a mirroring unit in prefetch-cache.test.ts covering pending-dedupe, settled-fresh suppression, and expired refetch. <Link> and router.prefetch() are now in sync. Verified:
- The
cacheForNavigation === truefall-through inlink.tsxis behavior-preserving vs. the old code (old innerif (!cacheForNavigation)never returned in that branch either). - The helper's exact-key lookup is correct — learning-only entries (
cacheForNavigation === false) never get rendered-path aliases, so there's no alias key to miss. deletePrefetchCacheEntry(..., notify=true)on expiry-sweep matches every other expiry-based deletion in the file (e.g.evictPrefetchCacheIfNeededat :787,hasPrefetchCacheEntryForNavigationdefault at :674).resolvePrefetchCacheEntryExpiresAtfalls back totimestamp + PREFETCH_CACHE_TTLfor learning-only entries lacking an explicitexpiresAt, so the freshness gate is well-defined.
Non-blocking: same learning-only dedupe gap still exists in next/form
packages/vinext/src/shims/form.tsx:343
<Form>'s viewport prefetch writes a learning-only entry (cacheForNavigation: false, form.tsx:365) and dedupes with the exact permanent-membership pattern this PR just replaced in link.tsx / navigation.ts:
const prefetched = getPrefetchedUrls();
if (prefetched.has(cacheKey)) return;
if (hasPrefetchCacheEntryForNavigation(rscUrl, interceptionContext, mountedSlotsHeader)) return;Because a settled learning-only entry is never swept on expiry (nothing calls deletePrefetchCacheEntry for it until LRU/byte-budget eviction or a navigation gate), prefetched.has(cacheKey) stays true forever and <Form> never refetches after expiry — the exact behavior this PR fixes for <Link> and router.prefetch().
This is a pre-existing gap, not a regression from this PR: form.tsx always hardcodes cacheForNavigation: false and does not consult resolveAutoAppRoutePrefetch, so the new isCacheComponentsDynamicRoute policy branch doesn't change its behavior. It's out of scope for the stated encoded-slash fix, so not blocking. But since AGENTS.md calls out keeping these parallel prefetch-dedupe paths in sync, it's worth either folding form.tsx into hasFreshLearningOnlyPrefetchCacheEntry here or filing a follow-up so the three learning-only callers don't drift.
Minor (verified, no change needed)
- Env define shape —
String(process.env.__NEXT_CACHE_COMPONENTS) === "true"(app-route-prefetch-policy.ts:100) matches the boolean define and the existingslot.tsxpattern. Correct. - Test
Date.nowspies — neither new test restores itsvi.spyOn(Date, "now")in afinally, but suite-levelafterEachvi.restoreAllMocks()covers it. Fine.
Overall LGTM on the encoded-slash learning-only fix. The one thing I'd want a decision on before merge is whether form.tsx's identical learning-only gate should be folded in now or tracked as a follow-up.
|
Review posted to PR #2866. SummaryI reviewed both commits on the PR — the original learning-only policy change ( The previously-flagged blocking issue is resolved. The follow-up commit routes One non-blocking finding I surfaced independently: Overall the encoded-slash fix is well-targeted and tested; the only open question for merge is the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(cache): keep encoded dynamic prefetches learning-only
Reviewed all three commits, including the third (804e16f "preserve static prefetch paths") that landed after the previous ask-bonk reviews and had not been analyzed yet. Ran link.test.ts + link-navigation.test.ts + prefetch-cache.test.ts (276/276 green) and vp check on all three changed source files (format/lint/types clean). Confirmed the narrowing against the upstream fixture.
The un-reviewed commit 804e16f is the substantive change — and it's correct
The earlier reviews only saw fb136ef + 7c5c680, where the policy was "any Cache Components dynamic route → learning-only". Commit 804e16f narrows that to:
const isFullyDynamicRootRoute =
route.patternParts.length === 1 && route.patternParts[0]?.startsWith(":");
const hasCacheComponentsLearningOnlyDynamicPath =
route.isDynamic &&
String(process.env.__NEXT_CACHE_COMPONENTS) === "true" &&
(isFullyDynamicRootRoute || ENCODED_PATH_DELIMITER_RE.test(routeUrl.pathname));This is the right call and matches the upstream fixture exactly. The Next.js test (encoded-slash-params.test.ts, v16.2.6) explicitly documents "The fixture is intentionally fully dynamic (no generateStaticParams)" and is a single-segment root route (/[slug]), exercising both /foo (unencoded root) and /foo%2Fbar (encoded). Both must stay learning-only. The prior blanket rule also demoted prefixed dynamic routes like /products/[id] to learning-only, killing navigation-reuse where the encoded-slash cache-key mismatch doesn't apply — hence the narrowing "preserves static prefetch paths." Verified:
URL.pathnamepreserves%2F/%5C(uppercase + lowercase) — the regex/%(?:2f|5c)/imatches all four; the delimiter check works.route.patternPartsis a required (non-optional) field onVinextLinkPrefetchRouteand is populated by the manifest generator (index.ts:3873), so.lengthis always safe; the?.guards the empty-array (/) case.- The encoded-delimiter branch is correctly gated behind
route.isDynamic, so a static route containing%2Fis not demoted. requiresDynamicNavigationRequest/canPrefetchLoadingShellstill cover the general "dynamic nav needs fresh data" cases independently — this gate is scoped to the encoded-slash + root-dynamic identity problem only.
Note: catch-all root routes (/[...slug] → patternParts: [":slug+"], /[[...slug]] → [":slug*"]) also satisfy isFullyDynamicRootRoute and stay learning-only. That's consistent with the fully-dynamic-root rationale, but it's a behavior the upstream fixture doesn't directly cover — worth a one-line confirmation from the author that it's intended (not blocking).
<Link> dedupe rewrite is behavior-preserving
The old gate returned early only inside the !cacheForNavigation branch; the new gate !autoPrefetch.cacheForNavigation && hasFreshLearningOnlyPrefetchCacheEntry(...) returns in exactly that same case, and never early-returns for cacheForNavigation === true (matching the old fall-through). prefetched and cacheKey remain in use downstream (link.tsx:634–646), so no dead variables. The helper's exact-key lookup is correct — addRenderedPathAndSearchPrefetchAlias early-returns for cacheForNavigation === false (navigation.ts:1447), so learning-only entries never get an alias key to miss.
Agreement with the prior reviews (no new blocking issues)
- The
router.prefetch()parity gap flagged in the first review is genuinely closed by7c5c680(navigation.ts:2789 routes through the freshness helper, with a mirroringprefetch-cache.test.tsunit). - The
next/formviewport-prefetch learning-only gap (form.tsx:343) is real but pre-existing and out of scope —form.tsxhardcodescacheForNavigation: falseand never consultsresolveAutoAppRoutePrefetch, so this PR doesn't change its behavior. Recommend tracking it as a follow-up so the three learning-only callers (link.tsx,navigation.ts,form.tsx) don't drift, per AGENTS.md "keep parallel paths in sync."
Minor (verified, no change needed)
- Env define shape —
String(process.env.__NEXT_CACHE_COMPONENTS) === "true"matches the boolean define (JSON.stringify(cacheComponents ?? false)) and the existingslot.tsxpattern. - Test
Date.nowspies — not restored in-test, but suiteafterEachvi.restoreAllMocks()covers it.
Verdict: LGTM. No blocking issues. The only open items are the author's confirmation that catch-all-root learning-only demotion is intended, and a decision to fold form.tsx in now vs. a tracked follow-up.
|
Review posted to PR #2866. SummaryI focused on the third commit No new blocking issues. The encoded-slash learning-only fix is correct and well-targeted. Key findings:
Two open non-blocking items for the author: confirm that catch-all-root routes ( Verdict: LGTM. |
|
Final exact-head deploy-suite audit for
Audit verdict: no regression attributable to #2866. |
1 participant
Summary
Original deploy-suite rows
Next.js v16.2.6 suite: test/e2e/app-dir/segment-cache/encoded-slash-params/encoded-slash-params.test.ts
Verification
Draft pending CI, Big Bonk, and an exact-head deploy-suite audit.