feat(ppr): safely serve cacheComponents fallback shells#1716
feat(ppr): safely serve cacheComponents fallback shells#1716NathanDrake2406 wants to merge 10 commits into
Conversation
commit: |
|
Replacing with stacked version |
NathanDrake2406
force-pushed
the
nathan/pr-4-safe-serving
branch
3 times, most recently
from
a389392 to
f0ede3a
Compare
- normalizePregeneratedPathname: document the deliberate non-strict vs strict normalization choice so it isn't "fixed" into a build-time throw - addPregeneratedConcretePath: normalize the pathname internally as the single source of truth, removing the caller pre-normalize footgun - export PrerenderRouteParamsRouteMatch for cloudflare#1716 consumers - cover direct un-normalized recording with a test
The type is exported for cloudflare#1716's serving consumers but has no in-repo reference yet, so knip's no-unused-exports gate flags it. Knip excludes exports tagged @public from that report; this keeps the reviewer-requested export without failing CI.
…d tests (#1714) * feat(ppr): add fallback shell payload identity * refactor(ppr): address review feedback on payload identity helpers - normalizePregeneratedPathname: document the deliberate non-strict vs strict normalization choice so it isn't "fixed" into a build-time throw - addPregeneratedConcretePath: normalize the pathname internally as the single source of truth, removing the caller pre-normalize footgun - export PrerenderRouteParamsRouteMatch for #1716 consumers - cover direct un-normalized recording with a test * fix(ppr): mark PrerenderRouteParamsRouteMatch @public for knip The type is exported for #1716's serving consumers but has no in-repo reference yet, so knip's no-unused-exports gate flags it. Knip excludes exports tagged @public from that report; this keeps the reviewer-requested export without failing CI. * docs(ppr): document live-Set and dedup contracts on concrete-path registry Address bonk review notes: - getRenderedConcreteUrlPathsForRoute returns the live backing Set for allocation-free hot-path lookups; document that callers must not retain the reference across a re-seed (clear empties the map, stranding it). - parsePregeneratedConcretePaths intentionally does not dedupe repeated route patterns; document that they merge additively and value-dedup in addPregeneratedConcretePath makes the result equivalent. --------- Co-authored-by: James <james@eli.cx>
NathanDrake2406
force-pushed
the
nathan/pr-4-safe-serving
branch
2 times, most recently
from
bcef56b to
bdc87a4
Compare
NathanDrake2406
changed the title
|
Filed follow-up #1861 for the remaining deploy-time concrete-path registry coupling. Treating that as non-blocking for this PR: #1716 now covers the runtime guard and module-init hydration contract, while #1861 tracks whether registry population should move into build/prerender finalization or be documented as vinext deploy-only behavior. |
|
Addressed the two small non-blocking clarity items in 77379e0: |
NathanDrake2406
force-pushed
the
nathan/pr-4-safe-serving
branch
from
77379e0 to
276f2dc
Compare
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the full stack. The core safe-serving invariant is implemented correctly: exact cache lookup and static-param validation run ahead of fallback-shell probing, the known-pregenerated-route guard (renderedConcreteUrlPaths) prevents silently degrading a known route whose cache has a transient gap, and the search-param / RSC / non-GET guards keep shell reuse narrowly scoped. Behavior matches the documented Next.js execution order and the referenced ppr-root-param-fallback tests. The entry stays thin and delegates to typed helpers (app-page-cache.ts, app-ppr-fallback-shell-render.ts), consistent with the repo's layering guidance, and test coverage is thorough (exact HIT, unknown-child STALE regen, query-bearing skip, encoded/worker-registry known routes). Ran vp test run on the dispatch/cache/seed/deploy/prerender-route-params/ppr-fallback-shell suites (378 tests) and vp check on the touched server files — all green.
A few non-blocking observations below. None block merge.
Production key consistency confirmed: the prerender manifest route field is set to route.pattern (colon notation, e.g. /:locale/blog/:slug) at write time (build/prerender.ts), and the runtime guard looks up by route.pattern, so the registry key and lookup key match in production. The only mismatch is in test fixtures (see comment on prerender-manifest.ts).
| * rather than a concrete rendered URL. | ||
| */ | ||
| export function isFallbackShellArtifactPath(pathname: string): boolean { | ||
| return pathname.includes("[") || pathname.includes("]"); |
There was a problem hiding this comment.
isFallbackShellArtifactPath treats any path containing a literal [ or ] as a fallback-shell placeholder. A legitimate concrete URL whose dynamic param value happens to contain a bracket (e.g. a slug like my-[draft]-post, which is valid in a URL path segment) would be excluded from the concrete-path registry. That would make the safe-serving guard fail to recognize it as a known pregenerated route, re-opening the exact path this PR is designed to close (serving a fallback shell in place of a known route after a transient cache gap).
This is a narrow edge case, but detecting fallback shells via the encoded fallbackParamNames on the manifest entry (or a dedicated marker) rather than substring-scanning the rendered URL would be more robust. Worth at least a follow-up note if not fixed here.
|
|
||
| type StaticPrerender = typeof import("react-dom/static.edge").prerender; | ||
|
|
||
| function isReactDevelopmentRuntime(): boolean { |
There was a problem hiding this comment.
isReactDevelopmentRuntime() keys off Function.prototype.toString.call(createReactElement).includes("getOwner"). This is brittle against minified/renamed dev builds and future React internals refactors — if the heuristic silently flips, you'd load the wrong renderer (prod vs dev) only on the fallback-shell prerender path, which is easy to miss. Consider gating on process.env.NODE_ENV (the canonical signal React itself uses to pick dev vs prod) instead of inspecting function source, or add a comment documenting why source inspection is preferred here so the fragility is intentional and tracked.
| function groupRoutesByPattern(routes: PrerenderManifestRoute[]): Map<string, string[]> { | ||
| const byPattern = new Map<string, string[]>(); | ||
| for (const r of routes) { | ||
| const pathname = r.path ?? r.route; |
There was a problem hiding this comment.
Test-fidelity note (not a production bug): the new seed-cache.test.ts / deploy.test.ts fixtures use bracket-notation route patterns (/blog/[slug], /en/blog/[slug]) for the manifest route field, but the real prerender manifest writes route.pattern in colon notation (/:slug). The tests still pass because the registry stores and reads by the same raw string, but they don't exercise the actual production key format. Using colon-notation patterns in at least one fixture would make the round-trip reflect real input and guard against an accidental format divergence between the writer (build/prerender.ts) and the runtime lookup (app-rsc-handler.ts).
|
Review submitted successfully. Summary of what I found: Review of PR #1716 —
|
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review of #1716 — feat(ppr): safely serve cacheComponents fallback shells
Reviewed the full diff and traced the runtime path end-to-end. The core safe-serving invariant is implemented correctly and the prior Bonk review's three observations have all been addressed in commit b7c3803.
Verified correct:
- Ordering (
app-page-dispatch.ts:681-886): exact cache read → static-param validation → fallback-shell probing. A known route is never silently replaced by a placeholder shell. TherenderedConcreteUrlPathsguard (tryServePprFallbackShell, line 529-530) treats a missing exact cache for a known pregenerated route as a transient gap. - Scope guards (line 531-549): RSC, non-GET, search-param-bearing (unless force-static), draft-mode, and non-production requests all correctly skip shell reuse.
- Key parity: registry stores/looks up by
route.pattern(colon notation) in both the manifest writer (build/prerender.ts) and runtime (app-rsc-handler.ts:807).cleanPathnamenormalization (normalizePath ∘ normalizePathnameForRouteMatchStrict) agrees with the registry'snormalizePregeneratedPathnamefor all well-formed input (malformed encoding is rejected with 400 before dispatch). Encoded-known-route test confirms this. - Stale regen targets the shell key, not the concrete unknown URL (
app-page-cache.ts:555-578) — no concrete-path cache poisoning. - Suspense integration:
headers()/cookies()return a hanging suspense promise only when a fallback-shell state is active, returningnullotherwise so the ordinary path is unchanged. The_decorateSuspendingRequestApiPromiseproxy correctly distinguishesawait headers()(then/catch/finally) from synchronous member access (throws). server-onlyguard refactor (index.ts): the"use server"exemption now uses the lightweight directive-prologue scanner instead of a full AST parse; behavior preserved.- Idempotent HTML rewrite: re-running
rewriteAppPprFallbackShellHtmlNavigationappends a second metadata script; sincecreateNavigationRuntimeRscMetadataScriptemitsObject.assign(...,{params,nav})and scripts run in document order, last-wins correctly overrides the cached placeholder. Covered by the new test.
Resolved prior observations (commit b7c3803):
isFallbackShellArtifactPathnow checksroute.fallback === truefirst; the manifest reliably carries the flag (prerender.ts:1394).isReactDevelopmentRuntimenow gates onNODE_ENVbefore the function-source heuristic.- Test fixtures converted to colon notation, matching production manifest format.
Ran targeted suites locally — all green:
tests/app-page-dispatch.test.ts,tests/app-page-cache.test.ts,tests/app-ppr-fallback-shell.test.ts(64 tests)tests/deploy.test.ts,tests/seed-cache.test.ts,tests/pregenerated-concrete-paths.test.ts,tests/prerender-route-params.test.ts,tests/ppr-fallback-shell.test.ts(322 tests)
One minor, non-blocking follow-up note inline. Nothing blocks merge.
| if (route?.fallback === true) { | ||
| return true; | ||
| } | ||
| return pathname.includes("[") || pathname.includes("]"); |
There was a problem hiding this comment.
Now that route.fallback === true is reliably written into the manifest (build/prerender.ts:1394), the route?.fallback === true check above is the authoritative signal. The substring fallback (pathname.includes("[") || pathname.includes("]")) is now only reachable for legacy manifests that predate the fallback flag — but it remains a live code path that would mis-classify a legitimate concrete URL whose slug contains a literal bracket (e.g. /blog/my-[draft]-post, valid in a URL segment) as a fallback shell, excluding it from the concrete-path registry and re-opening the exact degradation this PR closes.
Since fallback is authoritative for any manifest this build produces, consider scoping the substring scan to only fire when route?.fallback === undefined (backward-compat only), so current builds never hit it:
| return pathname.includes("[") || pathname.includes("]"); | |
| if (route?.fallback === true) { | |
| return true; | |
| } | |
| // Backward-compat only: manifests predating the `fallback` flag. Current | |
| // builds always set `fallback`, so a concrete URL containing a literal | |
| // bracket is never misclassified here. | |
| if (route?.fallback === undefined) { | |
| return pathname.includes("[") || pathname.includes("]"); | |
| } | |
| return false; |
|
Review submitted successfully. SummaryI completed a thorough review of PR #1716 ( Findings: The PR is well-implemented and well-tested. I traced the runtime path end-to-end and verified:
Prior Bonk review concerns are all resolved in commit
One non-blocking inline note: the substring fallback ( Nothing blocks merge. |
NathanDrake2406
force-pushed
the
nathan/pr-4-safe-serving
branch
from
915cc3b to
1e5cf37
Compare
|
/bigbonk review for issues |
PR 1 of 4: core model + build integration. Model: app-ppr-fallback-shell, pregenerated-concrete-paths, prerender-manifest Build: prerender/run-prerender fallback shell artifact generation Tests: createAppPprFallbackShells, pregenerated-concrete-paths core
The safe-serving branch failed CI because a client-side server-only guard rejected valid 'use server' action modules, a fallback-shell dependency cast no longer matched its helper type, apps/web lost the Cloudflare cache adapter source path during typecheck, and Knip could not resolve the documented Cloudflare cache adapter subpaths. The server-only guard now reads the directive prologue and skips top-level 'use server' modules, fallback-shell regeneration passes an explicit typed dependency object, apps/web gets the workspace Cloudflare cache path, the Cloudflare package exports explicit adapter subpaths, Knip uses a relative source import in the dispatch unit test, and the readiness test awaits the actual cache-ready promise.
NathanDrake2406
force-pushed
the
nathan/pr-4-safe-serving
branch
from
1e5cf37 to
1536ab7
Compare
|
/bigbonk review for issues |
2 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
2 participants
Overview
GETdocument HTML only; RSC, actions, non-GET, and ordinary streaming paths stay out of shell reuse.app-page-dispatch.ts,app-page-cache.ts,app-ppr-fallback-shell-render.ts,seed-cache.ts,deploy.tsserver-onlyhandling, typed fallback-shell deps, Cloudflare adapter resolution, and a timer-free readiness assertion.Why
Fallback-shell serving must preserve the invariant that an exact pregenerated route is never silently replaced by placeholder shell HTML. This PR keeps exact cache entries and static-param validation ahead of fallback-shell probing, then serves only eligible unknown child-param document requests from the reusable shell artifact.
What Changed
STALEand schedules regeneration for the fallback-shell key.'use server'modules importingserver-onlywere rejected from client-referenced action graphs.Maintainer review path
packages/vinext/src/server/app-page-dispatch.ts- request eligibility, exact-cache ordering, static-param validation, fallback-shell probing.packages/vinext/src/server/app-page-cache.ts- shell cache read/write and stale regeneration behavior.packages/vinext/src/server/app-ppr-fallback-shell-render.ts- warmup/final render pipeline for shell regeneration.packages/vinext/src/server/seed-cache.tsandpackages/vinext/src/deploy.ts- pregenerated concrete-path registry seeding/injection.tests/app-page-dispatch.test.ts,tests/app-page-cache.test.ts,tests/deploy.test.ts,tests/seed-cache.test.ts- behavioral contract coverage.Validation
Ran locally after the latest review-response commit:
vp run checkvp test run tests/app-page-dispatch.test.ts tests/deploy.test.tsRan locally after the CI fix commit:
vp run checkPATH="$PATH:/Users/nathan/Projects/vinext/.git/hooks/../../node_modules/.bin" knip --no-progressvp test run tests/app-page-dispatch.test.ts tests/ppr-fallback-shell.test.ts tests/middleware-server-only.test.tsPLAYWRIGHT_PROJECT=standalone-output vp exec playwright test --shard=1/1PLAYWRIGHT_PROJECT=static-export vp exec playwright test --shard=1/1PLAYWRIGHT_PROJECT=pages-router-prod vp exec playwright test --shard=1/1Earlier focused stack validation:
vp test run tests/app-ppr-fallback-shell.test.ts tests/prerender.test.ts tests/prerender-route-params.test.ts tests/ppr-fallback-shell.test.ts tests/thenable-params.test.ts tests/app-page-dispatch.test.ts tests/app-page-cache.test.ts tests/pregenerated-concrete-paths.test.ts tests/deploy.test.ts tests/seed-cache.test.ts tests/app-router.test.tsRisk / compatibility
@vinext/cloudflare/cache/*adapter exports already used by docs/config examples.'use server'may importserver-onlywithout tripping the client graph guard.Non-goals
allReadypolicy.References
Refs #1359