fix(app-router): honor per-response dynamic stale times on the client - #1712
Conversation
commit: |
36fd2e3 to
28880a8
Compare
App Router navigation reused and evicted cached RSC responses using one global prefetch TTL, so pages with different unstable_dynamicStaleTime values could not stay fresh independently. Traversal also needed a route instance boundary so browser history restores hidden state instead of reusing a forward-navigation component instance. The client cache now snapshots dynamic stale-time response metadata, computes expiry per entry, preserves staleAt across cache replays, and derives mounted-slot cache variants from the decoded payload. Browser traversal keeps route instances under React Activity while normal navigations allocate fresh route instances, matching Next's BFCache/stale-time contract.
28880a8 to
05d59e4
Compare
…ches The old prefetched.has(cacheKey) gate in prefetchUrl returned early for any exact key match, including stale entries. This meant an expired prefetch with the same cache key would never reach hasPrefetchCacheEntryForNavigation, so it would never be deleted and never refreshed. The new helper is built to check resolvePrefetchCacheEntryExpiresAt, delete stale entries, and return false. But the Link path bypassed it when the old prefetched Set contained the exact key. Fix: gate the exact-key check on the helper's freshness decision. When the helper reports the exact entry is stale and deletes it, also remove the key from the prefetched Set so the fall-through path can issue a fresh fetch. Keep the second helper call for equivalent _rsc variants. Add an integration test proving visible Links re-prefetch when their exact cache entry goes stale, not just that the helper reports stale entries as unavailable.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: per-response dynamic stale times on the client
Reviewed all 10 changed files and ran the targeted suites locally:
vp test run tests/app-visited-response-cache.test.ts tests/app-page-render.test.ts→ 44 passedvp test run tests/prefetch-cache.test.ts tests/link-navigation.test.ts→ 70 passedvp test run tests/app-router.test.ts tests/features.test.ts→ 650 passedvp checkon all 5 changed source files → format/lint/types clean
The change is well-scoped and the implementation is solid. Observations below are all non-blocking — mostly questions and small cleanups.
Verified correct
parsePrefetchCacheKeycorrectly invertsencodeCacheKey/createAppPayloadCacheKey— both use\0(APP_INTERCEPTION_SEPARATOR) as the separator (app-elements-wire.ts:19,294).deletePrefetchCacheEntryremoves from both the cache map and theprefetchedset, so the re-scan inconsumePrefetchResponseafter deleting an incompatible exact entry is safe.- The dynamic-stale-time header is emitted from a single centralized path (
app-page-response.ts:applyDynamicStaleTimeHeader), App Router only, consumed client-side — no Pages Router / prod-server parity gap introduced. app-page-render.tsis shared between App Router dev and prod, so the new gating is uniform across dev/prod.
Header gating: confirm the narrowed-but-not-closed over-gating is intended
The new shouldEmitDynamicStaleTime reads dynamicUsedDuringBuild (the consumeDynamicUsage() result captured at header-build time, before the RSC stream is consumed). consumeDynamicUsage() resets the flag (shims/headers.ts:316-321), and the late streaming-phase dynamic detection happens later in scheduleAppPageRscCacheWrite (app-page-cache.ts:644). So a production default-config route that turns dynamic only via a late cookies()/headers() call during the stream still has dynamicUsedDuringBuild === false and shouldCaptureRscForCacheMetadata === true → header still omitted. This narrows the original over-gating (phase-1 / searchParams-driven dynamic now emits) but does not fully close it. That matches the PR's stated non-goals, and the new omits ... on static production default-config test covers the phase-2 case — just confirming this residual gap is deliberate, not an oversight.
pingVisibleLinks restoration (not in this diff)
The PR description says the removed pingVisibleLinks() ping was "restored in BrowserRoot's treeState.elements effect." That effect (app-browser-entry.ts:1132, useLayoutEffect([treeState.elements])) does call pingVisibleLinks, but it is not part of this PR's diff — it appears to be pre-existing. The only change here is the removal of the ping from commitClientNavigationState (navigation.ts:1553). Behavioral question: the old ping fired on every notified navigation commit, whereas the surviving effect fires on treeState.elements reference changes. Is there any committed navigation that yields a referentially-equal treeState.elements (e.g. a same-URL replace, or a navigation reusing the element tree)? If so, visible links that previously re-pinged would no longer do so. If every commit produces a fresh elements reference, this is fine — worth confirming the same-tree case is covered (the new link-navigation re-prefetch test drives the ping directly via pingVisibleLinksFromRuntime, so it doesn't exercise the commit→ping wiring).
| } | ||
|
|
||
| if (hasPrefetchCacheEntryForNavigation(rscUrl, interceptionContext, mountedSlotsHeader)) { | ||
| return; |
There was a problem hiding this comment.
Minor: when this path is reached via the prefetched.has(cacheKey) branch above, hasPrefetchCacheEntryForNavigation has already been called once (and deleted the stale exact entry from prefetched). This second call re-runs the same lookup. It's still correct — and necessary to catch the case where the URL was never in prefetched but an equivalent _rsc variant exists in the cache — but for entries that came through the first branch it's a redundant scan. Not blocking; flagging in case the duplicate gate can be collapsed.
|
|
||
| if (cached.expiresAt > Date.now()) { | ||
| if ( | ||
| isVisitedResponseCacheEntryFresh(cached, { |
There was a problem hiding this comment.
isVisitedResponseCacheEntryFresh already returns false for navigationKind === "refresh" (app-visited-response-cache.ts:33-35), so the pre-existing early return null for refresh just above this block is now redundant. Harmless, but you could drop the early refresh return and keep the refresh-bypass logic solely in the freshness helper.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
@ask-bonk Thanks for the thorough review. Header gating residual gap — deliberate. The pingVisibleLinks removal from commitClientNavigationState — safe and more precise. The old ping fired on every
So every App Router visible commit yields a fresh The Bonk workflow failure appears to be a GitHub Actions runner auth issue ( |
Address review feedback on the client dynamic-stale-time changes: - link.tsx: the prefetch dedupe path called hasPrefetchCacheEntryForNavigation twice when an exact entry already existed. Collapse to a single freshness-aware gate that covers both the exact entry and equivalent _rsc variants. - app-browser-entry.ts: drop the redundant early refresh return in getVisitedResponse; isVisitedResponseCacheEntryFresh already bypasses the cache for refresh navigations (and the miss path evicts the entry).
|
/bigbonk review |
There was a problem hiding this comment.
Review: per-response dynamic stale times on the client
Reviewed all 10 files and ran the targeted suites locally — all green:
vp test run tests/app-visited-response-cache.test.ts tests/prefetch-cache.test.ts→ 30 passedvp test run tests/link-navigation.test.ts tests/app-page-render.test.ts→ 84 passedvp check packages/vinext/src/server/app-visited-response-cache.ts→ clean
The change is well-scoped and the implementation is solid. The per-entry expiresAt derivation, freshness-aware dedupe, and header gating all look correct, and the test coverage (including the ported Next.js staleness cases) is good. I confirmed:
parsePrefetchCacheKeycorrectly invertscreateAppPayloadCacheKey— both use\0(APP_INTERCEPTION_SEPARATOR,app-elements-wire.ts:19,297).deleteVisitedResponse(rscUrl, requestInterceptionContext)uses the same interception context thatgetVisitedResponselooked the entry up with, so the no-commit eviction targets the right key.- The narrowed header-gating (
shouldEmitDynamicStaleTime) is uniform across App Router dev/prod sinceapp-page-render.tsis shared, and the residual late-stream gap is documented + tested. - Storing
createdAtexplicitly on the visited entry is a correct fix: the oldexpiresAt - VISITED_RESPONSE_CACHE_TTLcreatedAt derivation breaks onceexpiresAtis per-response.
One behavioral-change question and a couple of non-blocking cleanups below.
Behavior change: refresh navigations now evict the visited-cache entry
The biggest semantic shift is in getVisitedResponse. On main, a refresh navigation did return null without deleting the entry. Now isVisitedResponseCacheEntryFresh returns false for refresh, so control falls through to the bottom visitedResponseCache.delete(cacheKey); return null. A refresh therefore evicts the entry instead of leaving it intact.
In practice the impact is limited because refresh requests are keyed under a different cache-busting URL (APP_RSC_RENDER_MODE_REFRESH_PRESERVE_UI, see the varies preserve-current-UI refresh payloads test), so the deleted key is usually the refresh-keyed entry rather than the normal-navigation one, and a missed refresh re-stores via storeVisitedResponseSnapshot. But it is a deliberate divergence from the prior return null-without-delete behavior, and there is no test asserting the new eviction. Worth confirming this is intended; if so, a short comment near the fall-through delete would prevent a future reader from "fixing" it back. See inline comments.
Non-blocking
The link.tsx double-gate and the redundant early refresh return null were also noted in the prior automated review; I agree with both and have re-flagged them inline.
| // `isVisitedResponseCacheEntryFresh` already bypasses the cache for | ||
| // `navigationKind === "refresh"`, so the refresh case falls through to the | ||
| // miss path below (which also evicts the stale entry). | ||
| if ( |
There was a problem hiding this comment.
Behavior change worth confirming + documenting. On main, navigationKind === "refresh" did return null without deleting the entry. Now that refresh routes through isVisitedResponseCacheEntryFresh (which returns false for refresh), control falls through to the bottom visitedResponseCache.delete(cacheKey) — so a refresh now evicts the visited entry instead of leaving it intact.
Impact is limited (refresh requests are keyed under a different cache-busting URL via APP_RSC_RENDER_MODE_REFRESH_PRESERVE_UI, and a miss re-stores), but it is a deliberate divergence from prior behavior with no test asserting it. Please confirm it's intended, and consider a comment at the fall-through delete noting that refresh now intentionally evicts.
There was a problem hiding this comment.
Confirmed intentional. Documented at the fall-through delete in 4012a46: a refresh deliberately drops any prior snapshot here, since the navigation re-fetches and re-stores a fresh one — leaving the old entry would only risk a later non-refresh navigation reusing a snapshot the user explicitly refreshed. The refresh-bypass itself stays the single source of truth in isVisitedResponseCacheEntryFresh, which is unit-tested (app-visited-response-cache.test.ts "never reuses visited responses for refresh navigations").
| export function isVisitedResponseCacheEntryFresh( | ||
| entry: VisitedResponseCacheEntry, | ||
| options: { | ||
| navigationKind: VisitedResponseCacheNavigationKind; |
There was a problem hiding this comment.
Since this helper now authoritatively short-circuits refresh, the pre-existing early if (navigationKind === "refresh") return null; that used to live in getVisitedResponse is gone — good. Just make sure the refresh-bypass logic stays solely here so there's a single source of truth for refresh handling (rather than reintroducing an early return upstream). Non-blocking.
There was a problem hiding this comment.
Agreed — kept as the single source of truth. No early refresh return was reintroduced upstream in getVisitedResponse; the only refresh handling lives here in the helper.
| // entry, so a stale `prefetched` member is harmlessly re-added below. | ||
| if ( | ||
| autoPrefetch.cacheForNavigation && | ||
| hasPrefetchCacheEntryForNavigation(rscUrl, interceptionContext, mountedSlotsHeader) |
There was a problem hiding this comment.
Minor redundancy (re-flagging from the earlier automated review): when this path is reached via the prefetched.has(cacheKey) branch above, the exact entry has already been looked up. This hasPrefetchCacheEntryForNavigation call then re-runs the same scan. It's correct and necessary to also catch the case where the URL was never in prefetched but an equivalent _rsc variant exists — just noting the duplicate lookup for entries that came through the first branch in case the gate can be collapsed. Non-blocking.
There was a problem hiding this comment.
This was already collapsed in ca6c7ee (before this re-flag landed): there's now a single hasPrefetchCacheEntryForNavigation gate outside the prefetched.has(cacheKey) branch, so the duplicate scan is gone. The prefetched.has branch now only handles the early !cacheForNavigation return and the cacheForNavigation upgrade.
|
fatal: could not read Username for 'https://github.com': No such device or address |
Per review feedback: getVisitedResponse now routes refresh navigations through isVisitedResponseCacheEntryFresh (the single source of truth), which returns false for refresh and therefore falls through to the read eviction. Document at the fall-through delete that a refresh intentionally drops any prior snapshot, since the navigation re-fetches and re-stores a fresh one.
|
/bigbonk review |
There was a problem hiding this comment.
Review: per-response dynamic stale times on the client
Reviewed all 10 changed files and ran the targeted suites locally — all green:
vp test run tests/app-visited-response-cache.test.ts tests/prefetch-cache.test.ts→ 30 passedvp test run tests/app-page-render.test.ts tests/link-navigation.test.ts→ 84 passed
The change is well-scoped and the implementation is correct. The per-entry expiresAt derivation, freshness-aware dedupe, and narrowed header gating all look right, and the ported Next.js staleness cases give good coverage.
Verified
parsePrefetchCacheKeycorrectly invertsencodeCacheKey. Both use\0(APP_INTERCEPTION_SEPARATOR,app-elements-wire.ts:19,297) and both find the first separator viaindexOf, so round-tripping is exact. URLs can't contain null bytes, so the split is unambiguous.pingVisibleLinksremoval fromcommitClientNavigationStateis safe. The surviving[treeState.elements]layout effect (app-browser-entry.ts:1132-1136) fires on every App Router visible commit becausemergeElementsalways returns a fresh{ ...next }object (slot.tsx:154,201) fornavigate/traverse, andreplaceinstalls a newaction.elementspayload (app-browser-visible-commit.ts:199). SotreeState.elementsis always a new reference on commit — the author's reasoning holds. The removed ping previously also fired on hash-only / externalpushState/ Pages popstate, which don't produce new App Router elements, so removing it is more precise, not a regression.consumePrefetchResponseexpiry propagation is correct:expiresAtis only injected onto the returned snapshot when the entry or snapshot carries one, otherwise the raw snapshot is returned (matches thepreserves the original expiryandmatches equivalent RSC cache variantstests).- Header gating uniformity:
app-page-render.tsis shared between App Router dev and prod, soshouldEmitDynamicStaleTimebehaves identically across dev/prod.
Header gating residual gap — confirmed deliberate
shouldEmitDynamicStaleTime reads dynamicUsedDuringBuild (the consumeDynamicUsage() result captured before the RSC stream is consumed). A production default-config route that turns dynamic only via a late cookies()/headers() call during streaming still has dynamicUsedDuringBuild === false and shouldCaptureRscForCacheMetadata === true, so the header is omitted. This narrows the prior over-gating (phase-1 / searchParams-driven dynamic now emits) but doesn't fully close it, matching the PR's stated non-goals and the new omits ... on static production default-config test. Noting it here as a documented residual gap, not an oversight.
No blocking issues. Two minor cleanups below.
| }; | ||
| } | ||
|
|
||
| function isPrefetchCacheEntryCompatibleWithMountedSlots( |
There was a problem hiding this comment.
isPrefetchCacheEntryCompatibleWithMountedSlots checks resolvePrefetchCacheEntryMountedSlotsHeader(entry) first, then falls back to entry.snapshot?.mountedSlotsHeader. Since resolvePrefetchCacheEntryMountedSlotsHeader already falls back to entry.snapshot?.mountedSlotsHeader when entry.mountedSlotsHeader === undefined, the second clause only ever differs when entry.mountedSlotsHeader is defined but disagrees with the snapshot's header (e.g. the prefetchRscResponse case where the request-time slot context is pinned on the entry while the response carried a different mountedSlotsHeader). That's the intended "accept either the request-time or server-declared slot context" semantics — just flagging that the dual check is load-bearing and not redundant, so a future reader doesn't collapse it. A one-line comment would help. Non-blocking.
There was a problem hiding this comment.
Added a comment in 0f2a509 documenting that the two clauses are load-bearing: the first accepts the entry's pinned request-time slot context, the second the server-declared snapshot header, and they only diverge in the prefetchRscResponse case. Thanks for catching the readability gap.
| if (resolvePrefetchCacheEntryExpiresAt(entry) <= Date.now()) { | ||
| return null; | ||
| } | ||
| if (entry.expiresAt !== undefined || entry.snapshot.expiresAt !== undefined) { |
There was a problem hiding this comment.
Minor: this guard re-checks entry.expiresAt !== undefined || entry.snapshot.expiresAt !== undefined only to decide whether to spread a fresh expiresAt onto the returned snapshot. Since resolveCachedRscResponseExpiresAt already returns snapshot.expiresAt when present, you could always return { ...entry.snapshot, expiresAt: resolvePrefetchCacheEntryExpiresAt(entry) } — but that would change toEqual(snapshot) expectations in matches equivalent RSC cache variants (which asserts the raw snapshot with no expiresAt). So the conditional is preserving an observable contract (don't synthesize an expiresAt field on entries that never had one). Worth a short comment to that effect so the branch isn't "simplified" away later. Non-blocking.
There was a problem hiding this comment.
Added a comment in 0f2a509 noting the conditional preserves an observable contract — entries that never carried an explicit expiresAt round-trip unchanged so callers/tests can assert the raw snapshot (e.g. "matches equivalent RSC cache variants"). Flagged not to collapse into an unconditional spread.
|
Review posted successfully. SummaryI reviewed PR #1712 (per-response dynamic stale times on the client) across all 10 changed files and ran the targeted test suites locally — all 114 tests pass. Verdict: Approve with minor non-blocking nits. The implementation is correct and well-scoped. Key things I verified:
I posted two non-blocking inline comments suggesting clarifying comments for load-bearing-but-non-obvious dual checks (the mounted-slots compatibility fallback and the I did not re-flag the two earlier automated-review nits (link.tsx double-gate, redundant refresh early-return) since the author already addressed them in later commits. |
Per review feedback, document two checks that look superficially redundant but preserve observable contracts: - isPrefetchCacheEntryCompatibleWithMountedSlots: the two clauses accept either the entry's pinned request-time slot context or the server-declared snapshot header; they diverge in the prefetchRscResponse case. - consumePrefetchResponse expiresAt guard: only synthesize expiresAt onto the returned snapshot when the entry already carried one, so entries without an explicit expiry round-trip unchanged for callers/tests.
2 participants
Overview
dynamicStaleTimeSeconds; prefetch and visited caches computeexpiresAtper entry; duplicate-prefetch suppression is freshness-aware.packages/vinext/src/shims/navigation.ts,packages/vinext/src/shims/link.tsx,packages/vinext/src/entries/app-browser-entry.tstests/prefetch-cache.test.ts,tests/link-navigation.test.ts,tests/app-visited-response-cache.test.ts,tests/app-page-render.test.tsWhy
Next.js treats stale time as response metadata, not a global client TTL. The upstream segment cache computes
staleAtper entry, and back/forward traversal uses a separate freshness window from normal navigation. Vinext previously used one globalPREFETCH_CACHE_TTL, so pages withunstable_dynamicStaleTime = 60andunstable_dynamicStaleTime = 10could not expire independently.What changed
PREFETCH_CACHE_TTLfor all entriesexpiresAtderived from the response'sdynamicStaleTimeSecondsor the global fallbackcreatedAtand computes per-entry expiry; refresh never reuses visited responses; traversal restore uses traversal windowprefetched.has(cacheKey)returned early for any exact match, including stale entries_rscvariants are still checkedunstable_dynamicStaleTimeand the response is known to be dynamic/non-cache-captured; static/default-config responses omit itBrowserRoot'streeState.elementseffect so persistent visible links re-prefetch when mounted-slot context changesReviewer path
packages/vinext/src/shims/navigation.ts—resolvePrefetchCacheEntryExpiresAt,hasPrefetchCacheEntryForNavigation,consumePrefetchResponse,snapshotRscResponse/restoreRscResponsepackages/vinext/src/shims/link.tsx—prefetchUrlfreshness-aware dedupe gatepackages/vinext/src/entries/app-browser-entry.ts— visited-cache snapshot/restore and traversal window handlingTests
tests/prefetch-cache.test.ts— per-response stale window consumption, stale entry deletion byhasPrefetchCacheEntryForNavigation, expiry preservation on consumed prefetchestests/link-navigation.test.ts— visible Link re-prefetch after exact cache entry goes staletests/app-visited-response-cache.test.ts— visited-cache replay, refresh bypass, traversal windowtests/app-page-render.test.ts— dynamic stale-time header gating for static vs dynamic responsestests/e2e/app-router/nextjs-compat/use-router-bfcache-id.spec.tsRisk / compatibility
unstable_dynamicStaleTimegain per-entry expiryNon-goals
Activityroute boundaryReferences