fix(app-router): classify intercepted routes by their intercepting tree (parked, outside the stack) - #3473
fix(app-router): classify intercepted routes by their intercepting tree (parked, outside the stack)#3473james-elicx wants to merge 7 commits into
Conversation
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| expect(modalFields).not.toContain("__loadOwnerDefault"); | ||
| const ownerDefaultLoader = modalIntercepts.match(/__loadOwnerDefault: (\w+),/)?.[1]; | ||
| expect(code).toContain( | ||
| `const ${ownerDefaultLoader} = () => import(${JSON.stringify(path.join(appDir, "feed/default.tsx"))});`, |
|
Performance benchmarksCompared 0 improved · 2 regressed · 4 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
6f9ac3b to
4a073a3
Compare
|
/bigbonk review for issues Please review exact head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| ...__resolveRouteStaticGeneration(effectiveRoute, segmentConfigBranches), | ||
| dynamicConfig: segmentConfig.dynamicConfig, | ||
| isDynamicRoute: route.isDynamic, | ||
| isDynamicRoute: route.isDynamic || !!intercept?.interceptedRoute.isDynamic, |
There was a problem hiding this comment.
For an interception-only target, app-rsc-handler promotes the source route before dispatch, so both route and intercept.interceptedRoute here are the static source. A branch such as @modal/(.)[photo] without generateStaticParams is therefore classified static and can receive the infinite ISR default/shared caching even though the intercepting tree is dynamic. Please derive this from the intercept branch segments (or retain the original target dynamism through promotion).
There was a problem hiding this comment.
Fixed in 8ce8e67: static eligibility now also counts the intercepting branch's own dynamic segments, using the same helper as the params gate. So a promoted static source rendering @modal/(.)[photo] without generateStaticParams is classified dynamic.
| }); | ||
|
|
||
| if (interceptState.kind === "source-route") { | ||
| await loadAppPageInterceptModules(interceptState.intercept); |
There was a problem hiding this comment.
This unconditionally evaluates the intercept page/layout/not-found for a source-route match even when the concrete source does not have intercept.slotKey. In that case the tree renders the source unchanged, so a top-level throw in any unused intercept module incorrectly fails the navigation. The current-route and rerender paths already guard with isAppPageInterceptAttachedToRoute; this path needs the same guard before loading.
There was a problem hiding this comment.
Fixed in aec5a1c: the direct intercepted RSC path loads the intercept's page, layouts and not-found only once the concrete source has the intercepted slot, like the current-route and rerender paths.
| await options.ensureRouteLoaded?.(revalidationTarget.route); | ||
| // A stale intercepted entry regenerates the tree its direct render | ||
| // rendered, so it takes that tree's config, not the source route's. | ||
| const revalidationConfigIntercept = revalidationTarget.interceptOpts |
There was a problem hiding this comment.
On stale source-route interception regeneration, resolveAppPageInterceptionRerenderTarget loads the page/layout modules but never ownerDefault, then toInterceptOpts captures the still-null value used here. An owner default exporting dynamic, revalidate, or fetchCache is consequently omitted from regeneration policy, so the refreshed cache entry can get the wrong lifetime/behavior. Load the attached slot owner default before constructing these options/classifying the tree.
There was a problem hiding this comment.
Fixed in d447852: the rerender target (stale regeneration and action rerender) loads an attached slot intercept's owner default before toInterceptOpts builds the options it classifies with.
|
Submitted 3 actionable inline findings. Focused tests passed: 401 tests across 6 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
| // With an intercept, the tree a direct intercepted RSC response renders, from | ||
| // __resolveRouteInterceptSegmentConfig. | ||
| function __resolveRouteDynamicConfig(route, intercept) { | ||
| if (intercept) { |
There was a problem hiding this comment.
[P1] Thread this intercept-aware config into Server Action rerenders too. resolveAppPageActionRerenderTarget preserves interceptOpts, but handleServerActionRscRequest still calls one-argument config callbacks and the generated callbacks at lines 2046-2053 invoke these helpers without the intercept. Thus an action that revalidates while rendering a force-dynamic, revalidated, or fetchCache-configured intercept over a static source rerenders under the source route config, despite loading and rendering the intercept tree. Please pass the active rerender intercept through those callbacks, as dispatch now does.
There was a problem hiding this comment.
Fixed in 95a506b: Server Action rerenders now pass the rerender's intercept (built with the same toRouteConfigIntercept dispatch uses) to the fetchCache, revalidate and dynamic resolvers. The entry's action toInterceptOpts also carries interceptOwnerDefault now, so a slot intercept's tree is the same one dispatch reduces.
| parallelPages: [...collectPages(interceptTree), ...collectPages(renderedTree)], | ||
| parallelSegments: [ | ||
| ...getParallelSegments(interceptTree), | ||
| ...collectSegments(renderedTree).filter((segment) => !interceptSegments.has(segment)), |
There was a problem hiding this comment.
[P1] Do not conflict-check mutually exclusive default and active leaves as one loader tree. interceptTree can contain a sibling slot default module while renderedTree contributes the active page for that same slot; this concatenation sends both through resolveAppPageSegmentConfig. If the default exports force-no-store and the active page exports force-cache, or they use opposing only modes, the navigation throws an incompatible fetchCache values error and returns 500 even though each loader tree is valid independently and only one leaf is rendered by Next.js. Merge the two effective policies without validating mutually exclusive modules as siblings.
There was a problem hiding this comment.
Fixed in 868baee: resolveAppPageInterceptSegmentConfig now reduces the intercepting tree and the rendered tree separately, then merges the two policies. The merge takes the shortest revalidate, force-dynamic or dynamicParams = false from either tree, route-wide fetchCache force/only modes from either (no-store wins a cross-tree conflict), intercept-tree precedence for the rest, and the shortest dynamicStaleTime. The dynamic = "error" → only-cache default is applied once, to the merged mode, so it can't leak from one tree into the other.
| // the intercepting branch's own dynamic segments count too. | ||
| isDynamicRoute: | ||
| route.isDynamic || | ||
| !!intercept?.interceptedRoute.isDynamic || |
There was a problem hiding this comment.
[P2] Avoid using the hard-navigation target match to decide the intercept tree dynamism. For a static branch such as @modal/(.)about, if there is no exact /about page but a broader catch-all route handles hard navigation, interceptedRoute.isDynamic is true and this marks the otherwise-static intercepted tree dynamic, disabling ISR and shared caching. The intercept branch segments already describe whether the loader tree being rendered has dynamic segments; derive this solely from that tree rather than the unrelated fallback route match.
There was a problem hiding this comment.
Fixed in c6af64d + af82e8d: __isRouteTreeStaticEligible no longer reads the matched target's isDynamic. I went one step past the suggested source-plus-branch rule. Next.js's isDynamicRoute rewrites an intercepting app path to its folder-derived intercepted route (https://github.com/vercel/next.js/blob/v16.2.7/packages/next/src/shared/lib/router/utils/is-dynamic.ts), so the check now reads interceptTargetPatternParts. That makes e.g. app/[lang]/feed/@modal/(...)photos static, as in Next.js. The now-unused interceptedRoute field is removed.
|
Submitted 3 actionable inline findings. Focused tests passed: 220 tests across 3 files. Worktree unchanged. |
|
/bigbonk review for issues Please review exact head |
1 similar comment
|
/bigbonk review for issues Please review exact head |
| // The rerender repeats the intercepted render, so it takes that render's | ||
| // config. As above, toInterceptOpts produces the dispatch intercept | ||
| // options, which the generic TInterceptOpts doesn't express. | ||
| const actionRerenderConfigIntercept = actionRerenderTarget.interceptOpts |
There was a problem hiding this comment.
[P1] Hydrate the slot owner default before deriving this config intercept. resolveAppPageActionRerenderTarget is called without routeHasSlot; the no-callback branch in resolveAppPageInterceptState loads the page/layouts and returns before loadAppPageInterceptOwnerDefault. Thus interceptOwnerDefault is still null here and action rerenders omit owner-default dynamic, revalidate, and fetchCache exports. Thread the generated route-slot predicate through the action options and rerender resolver.
There was a problem hiding this comment.
Fixed in 76442ba: the action rerender passes routeHasSlot, so a slot intercept's owner default loads.
| interceptedSlotHasRootLoading, | ||
| ); | ||
| if (intercept && !interceptHasLoadingBoundary) { | ||
| if (intercept && placement && !interceptHasLoadingBoundary) { |
There was a problem hiding this comment.
[P1] Probe the intercept layouts for current-route interception too. The generated probePage loads only the intercept page, and this helper invokes only that page; normal layout probes cover route.layouts, not intercept.interceptLayouts. A static intercept page whose branch layout reads headers() or cookies() can therefore be advertised/cacheable as static. Load and probe branch layouts up to their loading boundary, as the direct source-route probe does.
There was a problem hiding this comment.
Not changed in this PR: main's current-route probePage already loads and probes only the intercept page, never its layouts. This PR only changed where the intercept is placed. Flagged to the maintainer as a pre-existing gap for a follow-up.
| targetTreePosition, | ||
| sourceParams, | ||
| ); | ||
| const slotParams = isIntercepted ? (intercept?.matchedParams ?? sourceParams) : sourceParams; |
There was a problem hiding this comment.
[P1] Use the slot-specific params that the renderer uses here. buildSlotOverrides rematches inherited slots via slotPatternParts/slotParamNames, but this probe always supplies sourceParams to non-intercepted slots. For source [id] plus inherited slot [slug], rendering receives {slug} while probing receives {id}; a conditional dynamic API read can be missed and the payload cached. Pass the route pathname or precomputed slot param overrides into this helper.
There was a problem hiding this comment.
Fixed in 9ebff7b: the entry passes resolveSlotParamOverrides(sourceRoute, cleanPathname) (the same match buildPageElements makes for the intercepted render), and a non-intercepted slot probes with its override, falling back to sourceParams.
| // An interception replaces the slot's normal active branch. Only the slot | ||
| // root is necessarily shared; nested normal-branch loadings belong to a | ||
| // sibling subtree and must not wrap the intercepting page. | ||
| if (override && treePosition !== 0) continue; |
There was a problem hiding this comment.
[P2] Distinguish an interception override from a params-only override. buildSlotOverrides creates { params } entries for inherited slots with differently named params, and any such truthy override removes every non-root loading entry here although the normal branch was not replaced. Loading-shell prefetches can then omit the slot nested loading.tsx; suppress normal-branch loadings only when the override actually supplies an interception page/tree.
There was a problem hiding this comment.
Not changed in this PR: the same createAppPageSlotLoadingEntries(slot, override) call with a params-only override is on main (app-page-route-wiring.tsx prefetchSlotLoadingEntries). This PR only moved it into app-page-prefetch-plan.ts. Flagged to the maintainer as a pre-existing gap for a follow-up.
| routeLayouts.filter(({ treePosition }) => prefetchPlan.includesTreePosition(treePosition)), | ||
| prefetchPlan.isPrefetchLoadingShell ? Infinity : routeLoadingTreePosition, | ||
| ); | ||
| probeLoading(prefetchPlan.prefetchLoadingEntry); |
There was a problem hiding this comment.
[P1] Probe loading fallbacks on navigation responses too. In navigation mode this entry is null, while the actual intercepted tree wraps suspended descendants with these loading.tsx components. If a fallback reads a dynamic API, descendants stop at the boundary and the fallback is never probed, so isInterceptSourceKnownDynamic remains false and cacheable headers can be emitted. Probe the selected route/slot fallback whenever it is in the rendered payload, not only for loading-shell prefetches.
There was a problem hiding this comment.
Fixed in 967dd94: outside a loading-shell prefetch, the source probe now runs the first loading component of the route, of each slot, and of a sibling-page intercept's branch, since each renders as its Suspense fallback.
|
Submitted 5 actionable inline findings. Focused tests passed: 217 tests across 3 files. Worktree unchanged. |
af82e8d to
33b9786
Compare
|
/bigbonk review for issues Please review exact head |
…cepting tree A direct intercepted RSC response, and its regeneration, Server Action rerender and params gate, take their segment config from the intercepting route's own tree, as Next.js does, instead of the source or matched route's. Next.js's isDynamicRoute classifies an intercepting path by the route its folders intercept. Squashed from: - fix(app-router): classify and render an intercepted page by its intercepting tree - fix(app-router): keep a sibling intercept's folders between its source and the marker - fix(app-router): gate an intercepting tree's own dynamic segments under a static route - fix(app-router): gate an intercepting tree's omitted optional catch-all - fix(app-router): classify a promoted interception-only target by its branch's dynamic segments - fix(app-router): skip loading a slot intercept its source has no slot for - fix(app-router): load a slot intercept's owner default for a rerender - fix(app-router): reduce an intercepted response's two config trees separately - fix(app-router): classify an intercepting tree by its own segments, not the matched target - fix(app-router): resolve an intercepted action rerender's config from the intercepting tree - fix(app-router): classify an intercepting route by the route its folders intercept
…h its rematched params
967dd94 to
385a469
Compare
691e188 to
69c9734
Compare
| interceptionContext, | ||
| interceptionId, | ||
| ); | ||
| if (__probeIntercept) { |
There was a problem hiding this comment.
[P1] Resolve attachment before evaluating these lazy modules. For a route-group source variant that does not own the probed intercept slot, the renderer and buildAppPageInterceptSourceProbes correctly ignore the intercept, but this block still imports its page and layouts. Top-level dynamic API reads in that unused branch then mark the unchanged source response uncacheable, and arbitrary module side effects or top-level awaits still run. Apply resolveAppPageProbeIntercept to sourceRoute before loading and pass that filtered value to the probe builder.
There was a problem hiding this comment.
Fixed in 450178b: probeInterceptSource applies resolveAppPageProbeIntercept(sourceRoute, …) before loading the intercept's page or layouts, and passes the filtered value to the probe builder.
| export function hasAppPageInterceptDynamicSegment( | ||
| interceptBranchSegments: readonly string[] | null | undefined, | ||
| ): boolean { | ||
| return (interceptBranchSegments ?? []).some( |
There was a problem hiding this comment.
[P1] Strip the interception marker before checking for a param. The route graph preserves segments such as (.)[photo] and (.)[[...photo]], for which getAppPageSegmentParamName returns null. Consequently a static source rendering a marker-direct dynamic intercept is treated as non-dynamic here, so dynamicParams = false never runs its generated-param gate and unknown targets render. The optional-catch-all extraction in app-page-dispatch.ts needs the same normalization, otherwise an omitted marker-prefixed catch-all is not compared against an explicit empty generated value.
There was a problem hiding this comment.
Fixed in 2ed0ba7: a shared stripAppPageInterceptionMarker (app-page-params.ts) now feeds hasAppPageInterceptDynamicSegment and the dispatch's optional catch-all extraction. It also replaces the copies in isDynamicSegment and the generateStaticParams segment helper. New dispatch tests cover (.)[id] and (.)[[...photo]].
| layouts: tree.route.layouts, | ||
| layoutTreePositions: tree.route.layoutTreePositions, | ||
| page: tree.route.page, | ||
| parallelBranches: tree.branches, |
There was a problem hiding this comment.
[P1] Preserve the parallel branch matching metadata used by generateStaticParams. Unlike the ordinary-route path below, tree.branches originates in resolveRouteSegmentConfigBranches, which drops slotParamNames and slotPatternParts, and this call also omits routePatternParts. On an intercepted tree with an inherited slot whose URL params use different names, its generator receives source-route param names and its results cannot be rematched to the request, causing valid generated paths to 404 or the wrong tuple to pass. Carry that metadata through the intercept tree and pass the effective route pattern here.
There was a problem hiding this comment.
Not changed, because it isn't reachable: __resolveRouteGenerateStaticParams builds the intercepting tree without keepActiveSiblings, so resolveAppPageInterceptTree swaps every non-intercepting sibling for its default (layout: null, configLayouts: []) or drops it when it's owned deeper. The intercepted slot becomes the intercepting branch, whose params are named by its own segments, not by the slot's URL pattern. No inherited slot's generator reaches these sources, so there's no slotParamNames/slotPatternParts rematch to preserve and routePatternParts has nothing to remap. Flagged to the maintainer.
| .flatMap((segment) => getAppPageSegmentParamName(segment) ?? []) | ||
| : undefined, | ||
| params: | ||
| options.staticParamsValidationParams ?? |
There was a problem hiding this comment.
[P1] Do not let the promoted source prerender params replace the intercept params. For an interception-only target, app-rsc-handler sets staticParamsValidationParams from the promoted source match, while attachedCurrentRouteIntercept.matchedParams contains the target branch params. On a prerendered source such as /feed/[slug] rendering (.)[photo], this validates only {slug} against the intercept tree generator and can 404 even a generated photo. Preserve the trusted source values but overlay the attached intercept matched params before validation.
There was a problem hiding this comment.
Fixed in 8d36e01: with an attached current-route intercept, validation uses the intercept's matched params overlaid by the trusted staticParamsValidationParams, so a prerendered /feed/[slug] checks {slug, photo} against the intercepting tree's generators.
|
Submitted 4 actionable inline findings on exact head |
…ource has its slot
…pting branch's params too
|
/bigbonk review for issues Please review exact head |
james-elicx
changed the title
| probeLoading( | ||
| prefetchPlan.isPrefetchLoadingShell | ||
| ? prefetchPlan.prefetchLoadingEntry | ||
| : getFirstLoadingEntry(routeLoadingEntries), |
There was a problem hiding this comment.
[P1] Probe every loading fallback that can render, not only the shallowest one. A route can have ancestor and nested loading boundaries: after the suspended ancestor subtree resumes, React can render the nested fallback too (the loading E2E explicitly observes both). If only that nested fallback reads headers()/cookies(), this direct intercepted response has already emitted cacheable headers because getFirstLoadingEntry probes only the minimum tree position. The sibling-intercept and slot calls at lines 673 and 745 have the same problem; probe all potentially rendered fallbacks (while retaining the single selected entry for loading-shell mode).
| params: attachedCurrentRouteIntercept | ||
| ? { | ||
| ...attachedCurrentRouteIntercept.matchedParams, | ||
| ...options.staticParamsValidationParams, |
There was a problem hiding this comment.
[P1] Reverse this precedence so the intercept target value wins on a same-named param. matchedParams is built by merging source params with target params, and it is what the interception page renders; staticParamsValidationParams contains the promoted/prerendered source value. For /feed/[id] intercepting a target branch that also names [id], this order replaces the target id with the source id before generateStaticParams validation, so a valid target can 404 (or an invalid one can pass). Spread the trusted source values first, then attachedCurrentRouteIntercept.matchedParams.
| isStaticGenerationEdgeRuntime: options.isStaticGenerationEdgeRuntime === true, | ||
| revalidateSeconds: options.revalidateSeconds, | ||
| }); | ||
| const isNextStaticEligible = currentRouteIntercept |
There was a problem hiding this comment.
[P2] Resolve runtime from the same intercepting tree used for static eligibility. The rest of this branch switches to intercept-aware config, but readAppPageCacheResponse and renderAppPageLifecycle still receive options.isEdgeRuntime, generated from the matched route original __segmentConfig.runtime. A sibling-page intercept can replace an edge page with a Node page (or vice versa), leaving fresh/cached responses with the wrong edge marker and making cache-life/speculative-write decisions under the replaced branch runtime. Add an intercept-aware runtime resolver and use its result throughout this dispatch.
|
Submitted 3 actionable inline findings. Focused tests passed: 236 tests across 2 files. Worktree unchanged. |
2 participants
Important
Parked, not part of the ISR query-cache stack. This PR was removed from stack #3482 because it grew to about +7.1k lines (about 2k source and 5k tests), which is too large to review as one PR. None of #3451–#3465 depends on it. It stays open as a record of the approach and of the review findings. If intercepted-route parity is picked up later, it should be split into small, separate PRs (see "If this is picked up again"). It's based on
isr-query/14-workers-cache-query-free(#3465) because it builds on that branch's code.What this PR attempts
It makes intercepted routes follow Next.js's caching rules for the tree that is actually rendered. Today they follow only the matched or source route.
Next.js serves an intercepted request from the intercepting route's loader tree. For example, soft-navigating
/feed→/photos/1rendersapp/feed/@modal/(.)photos/[id]/page.tsx. That tree holds the source route's layouts plus the intercepting branch. Its segment config (dynamic,revalidate,fetchCache,runtime,dynamicParams,generateStaticParams) decides whether the response is static, how long it's cached, and which params it accepts. It's dynamic when the intercepted route is dynamic (isDynamicRoute).vinext only looks at the source route (for a direct intercepted RSC response) or the matched route (for a current-route interception).
Behaviour without this PR (after #3465)
This is the same as
main, plus the source-route handling that #3451 and #3453 add:no-storewhen the source route can't be static, isforce-dynamicorrevalidate = 0, or the request is in draft mode.Cache-Controlheader, as onmain, whatever the intercepting page or its layouts do.@modalintercept).force-dynamicintercepting page that reads no request API can be cached under the base route's revalidate.headers()/cookies()read still makes the render uncacheable at runtime.So dropping this PR regresses nothing. It leaves Next.js parity gaps for intercepted routes, all of them already on
main.What it changes
app-segment-config.ts,app-rsc-entry.ts).defaultin place of replaced children.force-dynamic/dynamicParams = false/ route-widefetchCachefrom either tree.interceptTargetPatternParts), as Next.js'sisDynamicRoutedoes.app-page-dispatch.ts,app-page-probe.ts,app-page-prefetch-plan.ts).no-store.dynamicParamsandgenerateStaticParams, with marker-prefixed segments such as(.)[photo]and omitted optional catch-alls handled. A miss renders through that tree's not-found. The same goes for cache policy, stale time and therevalidate = falsedefault.app-page-dispatch.ts(+483),app-page-probe.ts(+427),app-segment-config.ts(+313),app-rsc-entry.ts(+311),app-page-request.ts(+211),app-page-prefetch-plan.ts(+210, new; mostly moved from route wiring),app-route-graph.ts(+126).Review status
Bonk reviewed it in rounds 16–20. Each round found 3–4 new, narrower edge cases. All in-scope ones were fixed, and each review comment has a reply. The last push, 8d36e01, hasn't had a clean review.
Declined in review:
main, not fixed here:If this is picked up again
It should land as small, separate PRs against
main(or on top of the stack once it has merged), roughly in order of value:no-storeon a direct intercepted response whose intercepting page or layouts areforce-dynamicorrevalidate = 0. This is a config-only check with no probes.dynamic,revalidate,fetchCache) from the intercepting tree.dynamicParams/generateStaticParamsgate, and the ordinary optional catch-all gap onmain.no-store.Tests
tests/app-segment-config.test.ts,tests/app-page-dispatch.test.ts,tests/app-page-probe.test.ts,tests/app-page-request.test.ts,tests/app-router-isr-codegen.test.ts,tests/app-route-graph.test.tsandtests/app-server-action-execution.test.ts.