fix(app-router): preserve page result render ordering - #2760
Conversation
commit: |
|
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — PR #2760: preserve page result render ordering
I reviewed all 9 changed files, traced the render-ordering rework end-to-end, and ran the targeted suites plus the broader app-router/features suites. All green (tests/app-render-dependency.test.ts, tests/app-page-element-builder.test.ts, tests/app-page-route-wiring.test.ts, tests/app-browser-entry.test.ts, tests/app-router.test.ts, tests/features.test.ts) and vp check is clean over the changed source.
Overall
The two-phase page dependency (initialization barrier + resultDependencies) is a clean way to express the intended ordering:
- Layouts/templates gate on the page initialization barrier via
renderAfterAppDependencies(..., [pageRenderDependency, ...])(route-wiring L1130-1133, L1198-1199), released after the page's first async continuation (void Promise.resolve().then(() => renderDependency.release()), element-builder L544). - The returned page subtree gates on layout/template dependencies via
renderAfterAppDependencies(result, renderDependency.resultDependencies)(element-builder L546-561).
Because the flat Flight record serializes page and layouts as siblings reconnected through <Slot>/<Children> (not nested in a single render pass), and each barrier releases when the owning component produces its element (before its children render), the parent-before-child contract holds without introducing a page↔layout deadlock. Consolidating the previous with/without-loading-boundary branches into a single init-release plus result-gating is a genuine simplification, and it correctly stops turning an ancestor Suspense boundary into a blocking render.
Things I verified (no change requested)
resultDependenciesreference safety —setResultDependencies(pageDependencies)is called at route-wiring L1059 after the dependency-collection loop finishes (last mutation at L1056), and it's read lazily via a getter at render time. Safe.isAppRenderSuspensionparity — byte-for-byte equivalent to the removedisReactSuspension, so suspension-vs-error release semantics are unchanged.- memo/lazy/forwardRef unwrapping — recursion handles
memo(clientRef)correctly (client ref path doescreateElement); lazy's thrown thenable lands in the suspension branch and re-invokes on retry.forwardRefundefined-ref argument matches the Flight contract established in 13c3a68 and is asserted by the new subprocess test. - Prefetch snapshot normalization —
prepareConsumedPrefetchResponseForPublicationstripsexpiresAt/preparedElementssosnapshotmatchescreateCachedRscResponseSnapshotoutput shape, and the consumed-snapshot reuse path inapp-browser-entry.tsstill readscompletedDynamicStaleTimeSecondsand re-appliesnavResponseExpiresAtconsistently. Covered by the new unit test.
Minor / non-blocking observations
-
renderAppComponentWithDependencyBarriernow invokes the component directly rather than rendering a pre-built<LayoutComponent>element. ForforwardRef/memo/lazywrappers this bypasses React's own reconciliation of those wrappers (e.g.memono longer memoizes,forwardRefreceivesundefinedref, defaultProps aren't resolved). For App Router layouts/templates these are all deprecated/irrelevant paths, and the subprocess Flight test locks in the ordering, so this is acceptable — but it's worth a one-line note that this deliberately trades wrapper fidelity for ordering control, in case someone later wraps a layout inmemoexpecting memoization. -
isReactOwnedAppComponent(null)returnstrueviatypeof null !== "function". Callers guard with a truthiness check first, so it's never reached withnulltoday; just flagging that the| nullin the cast is effectively dead.
No correctness blockers found. The change is well-scoped, well-tested (including a real react-server subprocess Flight regression), and matches the documented Next.js ordering intent.
|
Review posted to PR #2760. SummaryI reviewed all 9 changed files, traced the render-ordering rework, and ran the targeted suites plus broader Verdict: no correctness blockers. The two-phase page dependency (initialization barrier +
Because the flat Flight record serializes page and layouts as siblings (reconnected via Verified safe:
Minor non-blocking notes I raised:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review — PR #2760: preserve page result render ordering
Re-reviewed on top of the earlier /bigbonk pass, this time drilling into the three areas the first review only summarized: the prefetch peek-vs-consume ownership change, the runtime-prefetch expiry semantics change in navigation.ts, and the seedPrefetchCache flip in app-browser-entry.ts. Ran tests/app-render-dependency.test.ts, tests/app-page-element-builder.test.ts, tests/app-page-route-wiring.test.ts, tests/prefetch-cache.test.ts, tests/app-page-render.test.ts, tests/app-browser-entry.test.ts — 507 passed.
Verdict: no correctness blockers. Confirming the change is safe to merge.
Additional things I verified (no change requested)
-
Peek → consume URL matching (
app-browser-entry.tsL1799,navigation.tsL1547/L577). The settled prefetch is now peeked bytargetPathAndSearchand only consumed later viaconsumePrefetchResponseForNavigation(rscUrl, ...), whererscUrlis derived fromsettledPrefetchedResponse.url.findPrefetchCacheEntryForNavigationdoes exact-key lookup first, then normalized/alias matching (L598-615), so the deferred consume resolves the same entry the peek found. The ownership transfer correctly moves to the reuse planner without risking a "peek hit, consume miss" navigation break. -
Runtime-prefetch expiry semantics (
navigation.tsL486-510). The switch frommin(dynamic, server)to "prefer completed cacheLife, fall back todynamicStaleTimeSecondsonly when no cacheLife claim" is the intended parity fix. Walked the cases: resolved server=240/dynamic=30 → 240s (was 30s — the regression being fixed); pending-server floor and no-cacheLife cases stay identical to before becauseserverStaleTimeSeconds({kind:"pending"})returns the 30s floor andundefinedserver falls through todynamicStaleTimeSeconds.resolveCachedRscResponseTtlMs(visited/BFCache path) still uses the old min-combine, keeping the two lattices independent as the comment claims. -
seedPrefetchCachenow alwaystrue(app-browser-entry.tsL2341). In the non-cacheRestorablebranch this now seeds the prefetch cache even for completed-resolved-dynamic responses. Combined with theconsumedPrefetchSnapshotreuse (L2274-2276) that avoids waiting on the cache tee, this closes the back-nav window where a just-committed destination was absent from both caches.completedDynamicStaleTimeSecondssurvivesprepareConsumedPrefetchResponseForPublication(onlyexpiresAt/preparedElementsare stripped), so thecacheRestorabledecision at L2279-2280 is unchanged for consumed prefetches. -
app-page-render.tscompletion footer (L903-926). Dropping!shouldEmitDynamicStaleTimefrom the guard and adding the inner "emit footer only for a distinct completed cacheLife claim" check (L917-919) means a known-dynamic response now carries BFCache bound in the header and a completion footer when — and only when — cacheLife resolved.tests/app-page-render.test.tslocks in both header + footer together.
Concurring with prior notes
- Two-phase page dependency (init barrier +
resultDependencies) holds parent-before-child with no page↔layout deadlock: layouts gate on page init (released at element-builder L544), page result gates on layout/template deps that release once each layout produces its element. renderAppComponentWithDependencyBarrierbypasses React reconciliation of memo/forwardRef/lazy wrappers — acceptable for deprecated layout-wrapper paths; the react-server subprocess Flight test locks in ordering +forwardRefundefined-ref contract.isReactOwnedAppComponent(null)'s| nullcast branch is dead (callers guard); harmless.
Well-scoped, well-tested, matches documented Next.js ordering + segment-cache intent.
|
Review posted to PR #2760. SummaryI re-reviewed the PR (a Verdict: no correctness blockers. Areas I independently verified this round:
I concurred with the prior review's two non-blocking notes (wrapper-reconciliation tradeoff, dead |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review — PR #2760: preserve page result render ordering
Third /bigbonk pass. Rather than re-summarize the two prior reviews, I independently traced the render-ordering rework, the prefetch peek→consume ownership change, the navigation.ts expiry switch, and the seedPrefetchCache flip — then ran the touched suites and vp check.
Ran: tests/app-render-dependency.test.ts, tests/app-page-element-builder.test.ts, tests/app-page-route-wiring.test.ts (146 passed) + tests/prefetch-cache.test.ts, tests/app-browser-entry.test.ts, tests/app-page-render.test.ts (362 passed). vp check clean over all 7 changed source files.
Verdict: no correctness blockers. Safe to merge.
Independently verified this round
-
Two-phase page dependency has no deadlock across all page kinds. Layouts/templates gate on
[pageRenderDependency, ...before](route-wiring L1130-1133, L1198-1201); the returned page subtree gates onrenderDependency.resultDependencies(element-builder L546-561). The init barrier releases from a microtask before the full page result (element-builder L544), so the page-init→layout→page-result chain is acyclic. Crucially I checked thepageRenderDependency === nullpath (client-ref/class page, element-builder L506-509): layouts then do not wait on the page, and the page gates on the fullpageDependenciesset (route-wiring L1094-1096) — the pre-PR ordering, still acyclic. Both branches preserve parent-before-child. -
setResultDependenciesarray is fully populated.pageDependenciesis mutated in place through L1056 and the setter is called at L1059 after the last push; the getter reads the live array lazily at render time. No stale/partial snapshot. The page result (via getter) and thenull-dependency page (L1096) read the same fully-populated array. -
Prefetch peek→consume ownership is race-free. The settled prefetch is peeked by
targetPathAndSearch(L1802) and consumed by the sametargetPathAndSearchkey (L1979-1983) — not byrscUrl, so there is no key drift between peek and consume. The reuse planner picks either visited-response reuse (whichreturns/continues at L1962-1965 before ever reaching the consume) orconsumePrefetch; no unconditionalawaitsits between peek and consume on the consume path. And a peek-hit/consume-miss degrades gracefully:navResponsestays undefined andfallbackReuseDecisionrecomputes to a fresh fetch (L2011-2024). The peek deliberately leaves the entry available so a visited-response hit can still serve visible Links after back-nav. -
seedPrefetchCachenow alwaystrue(L2350) is safe for dynamic responses. A known-dynamic response seeded into the prefetch cache still gets its expiry fromresolveCachedRscResponseExpiresAt→min(dynamic, server)(navigation.ts L475-483, seed at L1063), sostaleTimes.dynamicstill bounds its reuse — it is not over-reused as a stale prefetch. Combined with theconsumedPrefetchSnapshotreuse (L2284-2286) this closes the back-nav window where a just-committed destination was in neither cache.preserveCommittedPrefetchExpirycorrectly keeps the absolute prefetch deadline only for consumed prefetches (wherenavResponseExpiresAtis set) and no-ops otherwise (prefetch-response.ts L96-101). -
Runtime-prefetch expiry switch (navigation.ts L495-503): prefer completed cacheLife (
serverStaleTimeSeconds), fall back todynamicStaleTimeSecondsonly when there is no cacheLife claim.resolveCachedRscResponseTtlMs/resolveCachedRscResponseExpiresAt(visited/BFCache) keep the oldmin-combine, so the two staleness lattices stay independent as the comment claims. -
Completion-footer guard (app-page-render.ts L917-919): a known-dynamic response emits header + footer only when a distinct completed cacheLife claim resolved; otherwise the footer is dropped and the header alone carries the BFCache bound.
tests/app-page-render.test.tslocks this in.
Concurring with the prior notes
renderAppComponentWithDependencyBarrierinvokes memo/lazy/forwardRef wrappers directly, bypassing React's reconciliation of those wrappers — acceptable for the deprecated layout-wrapper paths, and the react-server subprocess Flight test locks in both the release ordering across plain/memo/lazy/forwardRef and theforwardRefundefined-ref contract.isReactOwnedAppComponent(null)'s| nullcast branch is dead (callers guard); harmless.
Well-scoped, well-tested (including a real react-server subprocess Flight regression), matches the documented Next.js execution-order + segment-cache intent.
|
Review posted to PR #2760. SummaryThird Verdict: no correctness blockers. Safe to merge. Areas I verified myself this round:
I concurred with the two prior non-blocking notes (wrapper-reconciliation tradeoff, dead |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review — PR #2760: preserve page result render ordering
Fourth /bigbonk pass. Rather than restate the three prior reviews, I independently traced each substantive change from the source and drilled into two areas the earlier passes only asserted at a high level: the captured cache-life accessor lifetime and the completion-footer stream framing side effect. Ran the 7 touched suites (535 passed) and vp check clean over all 9 changed source files.
Verdict: no correctness blockers. Safe to merge.
Independently verified this round
-
_captureRequestScopedCacheLifeAccessorscaptures the live request-context object, notfallbackState. The accessor is captured atapp-page-dispatch.ts:1086, beforerenderAppPageLifecycle, which runs inside the unified request scope the RSC entry establishes — sogetCacheState()returnsgetRequestContext()(the real context), and the closure seesrequestScopedCacheLifemutations made during body render. The whole point is that the consume/peek callbacks fire later from stream finalization, outside the ALS scope, where the old_consumeRequestScopedCacheLife()would have resolvedfallbackStateand dropped the claim. The newtests/unified-request-context.test.tscase ("retains captured cache-life access after the request scope exits") locks exactly this.peek()returns a defensive copy;consume()nulls the slot. Correct. -
Completion-footer guard rewrite is a strict superset with a gated addition. Previously a known-dynamic response (
shouldEmitDynamicStaleTime === true) bypassedappendRscCompletionMetadataentirely (served raw). Now everydynamicStaleTimeSeconds !== undefinedresponse is wrapped, but the inner guard (app-page-render.ts:917-919) returnsundefinedfor the known-dynamic/no-cacheLife case — so no footer is emitted there, matching old behavior. A footer is now added only for the previously-missing "known-dynamic + distinct completed cacheLife" case. I checked the stream-framing consequence:appendRscCompletionMetadataescapes every chunk (escapeFlightChunk) even when the footer isundefined, and now setsVINEXT_RSC_COMPLETION_METADATA_HEADERon known-dynamic responses too. This is self-consistent — the header signals the client to runstripRscCompletionMetadata, which handles both the has-footer and escaped-only-no-footer cases.tests/app-page-render.test.tslocks header + footer together. -
Peek→consume key parity confirmed at the source. Both
peekSettledPrefetchResponseForNavigation(prefetch-response.ts:17) and the settled-path consume (consumePrefetchResponse(targetPathAndSearch, ...), browser-entry.ts) key on the sametargetPathAndSearch— resolving the earlier reviews' disagreement in favor of "same key, no drift." A peek-hit/consume-miss degrades to a fresh fetch viafallbackReuseDecision; a visited-response hitreturns before the consume, leaving the peeked entry available. -
storeVisitedResponseSnapshot's newprefetchSnapshotparam is inert when unused. It defaults tosnapshotand is only read underseedPrefetchCache: true(seed + delete). The onefalse-seed call site (initial hydration, browser-entry.ts:1564) never touches it; the twotrue-seed sites passpreserveCommittedPrefetchExpiry(snapshot, navResponseExpiresAt)explicitly. The visited/BFCachesnapshotnever inherits the prefetch's absolutenavResponseExpiresAt; only the seededprefetchSnapshotdoes, and only for consumed prefetches (wherenavResponseExpiresAtis set). Matches the stated intent. -
Two-phase page dependency is acyclic on both page kinds. page-result gates on
resultDependencies(= the fully-populatedpageDependencies, set at route-wiring.tsx:1059 after the last push); each layout gates on[pageRenderDependency(init), ...before]and releases its own dep when it produces its element (barrier); the init barrier releases from a microtask after the page's first continuation (element-builder.tsx:544). ThepageRenderDependency === nullpath (client-ref/class page) drops layouts' wait on the page and gates the page on the full set — the pre-PR ordering, still acyclic. Flat Flight entries are serialized independently and reconnected via<Slot>/<Children>, so no synchronous render nesting can deadlock.
Concurring with prior notes (both still non-blocking)
renderAppComponentWithDependencyBarrierinvokes memo/lazy/forwardRef wrappers directly, bypassing React's reconciliation of those wrappers — acceptable for the deprecated layout-wrapper paths; the react-server subprocess Flight test locks in ordering + theforwardRefundefined-ref contract. Still worth a one-line note in-code that this deliberately trades wrapper fidelity for ordering control.isReactOwnedAppComponent(null)'s| nullcast branch is dead (callers guard); harmless.
Well-scoped, well-tested, matches documented Next.js execution-order + segment-cache intent.
|
Review posted to PR #2760. SummaryFourth Verdict: no correctness blockers. Safe to merge. New verification this round:
Concurred with the two prior non-blocking notes (direct-invoke wrapper-reconciliation tradeoff; dead |
Summary
cacheLifeexpiry independent from visited/BFCachestaleTimes.dynamicexpirycacheLifemetadata until RSC stream completion, even after the request AsyncLocalStorage scope has exitedRoot causes
Page render ordering
PR #2751 used one dependency for both page initialization and full async page completion. Layouts and parallel slots therefore waited too long, blocking ancestor Suspense streaming, while the returned page subtree bypassed the existing layout/template result barriers.
The page dependency now has two phases: an initialization barrier released after the page's first async continuation, and result dependencies applied to the returned subtree after layouts/templates have produced their elements.
Per-page stale window
Navigation consumed and removed the prefetched Flight entry. Returning to the source page exposed no reusable prefetch, so a visible Link fetched a replacement and restarted the stale window. Committed navigations now republish the consumed snapshot while retaining its original absolute expiry.
Runtime-prefetch expiry
Runtime prefetches and visited/BFCache entries were sharing one expiry calculation. Next.js uses the completed
cacheLifeclaim for runtime-prefetch freshness, while visited reuse remains bounded bystaleTimes.dynamic; the implementation now preserves those as independent snapshots and deadlines.The full deploy-suite run also exposed a timing-dependent Linux failure: RSC completion metadata re-resolved request cache state after the request AsyncLocalStorage scope had exited, occasionally losing the completed
cacheLifeclaim and falling back to 30 seconds. Dispatch now captures request-bound cache-life accessors before creating the stream, so footer finalization reads the correct request state.Validation
Final head:
81d08c16bdf0ad763ff6d72261f9c52a67b59e75vp checkover all changed files — format, lint, and types passedfull-prefetch.browser.spec.ts— 2/2 passedsegment-cache-per-page-dynamic-stale-time.test.ts—reuses dynamic data within the per-page stale time windowpassedsegment-cache-stale-time.test.ts—expires runtime prefetches when their stale time has elapsedpassedmetadata-streaming-static-generation.test.ts7/7,app-basepath/index.test.ts13/13, andoptimistic-routing.test.ts9/9; known unrelated baseline failures remain in broader filesv16.2.6,suite-filter=all— 2,615 passed / 189 known parity failures / 631 skipped; both regression assertions above passed on their first invocationAll local Next.js checks used
scripts/run-targeted-nextjs-e2e.shagainst Next.jsv16.2.6with concurrency 1.