fix(ppr): gate fallback shells until request-time resume is supported

[NathanDrake2406]

Overview

This PR builds the fallback-shell generation and serving prerequisites needed for Next.js cacheComponents parity, but keeps fallback-shell artifact generation disabled by default until vinext implements request-time resume.

Partial HTML/RSC fallback artifacts cannot safely be served as complete documents. Without resume, React hydration falls into the global error boundary. The internal __VINEXT_EXPERIMENTAL_PPR_FALLBACK_SHELLS=1 opt-in exists only for focused generation tests and development of the follow-up resume implementation.

Area	Current behavior
Default production behavior	Does not generate partial fallback-shell artifacts; unknown params use a normal full render.
Internal opt-in	__VINEXT_EXPERIMENTAL_PPR_FALLBACK_SHELLS=1 enables artifact generation for focused testing only.
Request-time resume	Not implemented in this PR.
Cloudflare Worker cache population	Not implemented in this PR.
Runtime safety	Existing fallback-shell lookup remains narrow, but default builds contain no generated shells to serve.

Why

Next.js PPR fallback shells require two coordinated halves:

1. Build a partial HTML/RSC shell using React's static prerender APIs.
2. Resume that shell with request-specific dynamic content before completing the response.

This PR establishes and tests the build/render prerequisites, including the RSC-to-SSR signal handoff, abort lifecycle, static Flight prerendering, and cache-task coordination. Since vinext does not yet have step 2, generation is gated off by default rather than exposing a broken hydration path.

What Changed

• Adds the fallback-shell warmup/final-render lifecycle and static Flight prerender plumbing.
• Preserves authenticated prerender route parameters across internal header filtering.
• Explicitly passes PPR state from the RSC environment to SSR.
• Keeps exact cache lookup and static-param validation ahead of fallback-shell probing.
• Disables fallback-shell artifact generation by default behind an internal environment gate.
• Adds unit coverage proving default-off and explicit opt-in generation.
• Adds a Cloudflare-configured real application fixture and Playwright E2E proving an unknown route fully renders without global hydration-error UI.

Safe Default

With ordinary configuration, cacheComponents: true does not queue a bracket-param fallback artifact. For example, /en/blog/known may be prerendered, but /en/blog/[slug] is not emitted. A request for /en/blog/new-post therefore performs a normal concrete render and hydrates successfully.

The internal opt-in is intentionally not a supported public feature flag. It allows artifact generation to remain testable while the request-time resume follow-up is developed separately.

Validation

• vp run knip
• vp test run tests/ppr-fallback-shell.test.ts tests/app-page-stream.test.ts tests/app-page-dispatch.test.ts tests/prerender.test.ts
• vp test run tests/prerender.test.ts -t \"cacheComponents PPR fallback-shell artifacts\"
• PLAYWRIGHT_PROJECT=ppr-impact-demo vp exec playwright test
• Focused vp check on all touched PPR runtime, test, example, and Playwright files
• Manual browser verification of /en/blog/new-post: concrete blog/comments content rendered, no global error UI

Follow-up Required

A separate PR should implement and validate request-time resume before enabling fallback-shell generation by default. Worker prerender-cache population should be designed separately rather than added implicitly here.

Non-goals

• Enabling partial fallback shells for users today.
• Populating the Worker cache with fallback artifacts.
• Implementing request-time resume.
• Changing ordinary RSC, action, non-GET, or normal full-render behavior.

References

• Next.js root-param PPR fallback test
• Next.js cacheComponents render flow

Refs #1359

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1716

npm i https://pkg.pr.new/vinext@1716

commit: 7a7ee0a

[NathanDrake2406]

Replacing with stacked version

[NathanDrake2406]

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 (Vite 8.0.16)

vinext deploy-only behavior.

[NathanDrake2406]

Addressed the two small non-blocking clarity items in 77379e0: addPregeneratedConcretePath() now normalizes internally, and rewriteAppPprFallbackShellHtmlNavigation has a regression covering cached placeholder metadata followed by request-specific metadata. #1861 remains the follow-up for the deploy-time registry coupling.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

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).

[ask-bonk]

Review submitted successfully. Summary of what I found:

Review of PR #1716 — feat(ppr): safely serve cacheComponents fallback shells

Overall: Solid, well-tested PR. The core safe-serving invariant is implemented correctly and behavior aligns with Next.js. I verified all relevant tests pass (378 tests across 7 files) and vp check is clean on the touched server files.

What's correct:

• Exact cache lookup + static-param validation run before fallback-shell probing (app-page-dispatch.ts:863-886), preserving the invariant that a known route is never silently replaced by placeholder HTML.
• The renderedConcreteUrlPaths guard (tryServePprFallbackShell, line 529) correctly treats a missing exact cache for a known pregenerated route as a transient gap rather than an "unknown" signal.
• Search-param / RSC / non-GET guards keep shell reuse narrowly scoped.
• Stale shells serve STALE and schedule regeneration against the shell key, avoiding concrete-path cache poisoning.
• Entry stays thin and delegates to typed helpers per repo layering guidance.
• I confirmed the registry key (manifest route = route.pattern, colon notation) matches the runtime lookup key in production — no real key mismatch.

Non-blocking observations posted as inline comments:

1. isFallbackShellArtifactPath substring-scans for [/], which could false-positive on a concrete URL whose slug contains a literal bracket, excluding it from the registry and re-opening the degradation path. Detecting via fallbackParamNames would be more robust.
2. isReactDevelopmentRuntime() keys off Function.prototype.toString().includes("getOwner") — brittle; suggested gating on NODE_ENV or documenting the choice.
3. Test fidelity: seed-cache/deploy fixtures use bracket-notation route patterns while production uses colon notation; tests pass via round-trip consistency but don't reflect real input format.

None of these block merge.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

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. The renderedConcreteUrlPaths guard (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). cleanPathname normalization (normalizePath ∘ normalizePathnameForRouteMatchStrict) agrees with the registry's normalizePregeneratedPathname for 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, returning null otherwise so the ordinary path is unchanged. The _decorateSuspendingRequestApiPromise proxy correctly distinguishes await headers() (then/catch/finally) from synchronous member access (throws).
• server-only guard 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 rewriteAppPprFallbackShellHtmlNavigation appends a second metadata script; since createNavigationRuntimeRscMetadataScript emits Object.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):

• isFallbackShellArtifactPath now checks route.fallback === true first; the manifest reliably carries the flag (prerender.ts:1394).
• isReactDevelopmentRuntime now gates on NODE_ENV before 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.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 (feat(ppr): safely serve cacheComponents fallback shells) at commit 2f57bf6 and posted a batch review with two inline comments.

What's correct (verified):

• Core safe-serving ordering: exact cache read → static-param validation → fallback-shell probing. The renderedConcreteUrlPaths guard correctly treats a missing exact cache for a known pregenerated route as a transient gap, not an "unknown" signal.
• Scope guards (RSC / non-GET / search-params / draft-mode / cache-disabled) keep shell reuse narrow.
• Key parity holds: manifest writer stores route.route (= route.pattern, colon notation, set in build/prerender.ts) and runtime looks up via getRenderedConcreteUrlPathsForRoute(route.pattern).
• Prior Bonk concerns all resolved (bracket heuristic gated on route.fallback, isReactDevelopmentRuntime gated on NODE_ENV, colon-notation fixtures, deploy injection idempotency well-tested).
• 63 targeted tests pass; vp check clean on touched files.

Blocking issue found:

• renderFreshPprFallbackShellForCache (and its private helpers/types in app-ppr-fallback-shell-render.ts) is exported but never imported anywhere — only warmPprFallbackShellCaches is consumed. vp run knip (run by CI in ci.yml:28) fails on this. The PR's Validation section claims knip passed, but it does not on this commit. I reproduced the failure locally.

Behavioral gap surfaced by the dead code:

• The orphaned function is the only code path that would regenerate a stale fallback shell. Because it's never wired in, the stale path uses scheduleRegeneration() {} (a no-op). This contradicts the PR description's claim that stale shells "schedule regeneration for the fallback-shell key" — the code and test actually agree on no regeneration. Needs reconciling: either wire the function in, or remove it and update the description.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-reviewed at 433cf14 (two commits past the 2f57bf6 the last Bonk review saw). The two blocking concerns from that review are now resolved:

• knip / dead code: renderFreshPprFallbackShellForCache and its private helpers/types were removed in 91def17 + 433cf14. I reproduced knip locally and it now exits clean (no unused exports). The CI Check job should pass.
• Stale-shell regeneration: removing the orphaned regen path makes the code, the test (app-page-cache.test.ts:689 — "serves stale static fallback shells without regenerating the shared shell key"), and the no-op scheduleRegeneration() {} (app-page-cache.ts:618) all internally consistent. No-regeneration is now the deliberate, tested design.

Verified correct (independent trace):

• Ordering in dispatchAppPageInner: exact cache lookup (ends app-page-dispatch.ts:830) → static-param validation (832–843) → fallback-shell probing (845–855). A known route is never silently replaced by a placeholder shell.
• classifyPprFallbackShellEligibility (app-page-dispatch.ts:510) puts the renderedConcreteUrlPaths known-route guard as the first branch; a missing exact cache for a pregenerated path is treated as a transient gap, not an "unknown" signal.
• Scope guards (RSC / non-GET / search-params / draft-mode / cache-disabled) keep shell reuse narrow.
• isFallbackShellArtifactPath (prerender-manifest.ts:52) treats route.fallback as authoritative and scopes the bracket heuristic to legacy manifests (fallback === undefined) with a dev warning — prior review's suggestion adopted.
• isReactDevelopmentRuntime (app-ssr-entry.ts:87) gates on NODE_ENV before the function-source fallback.

Ran locally with vp (the harness CI uses): tests/app-page-dispatch.test.ts, app-page-cache.test.ts, app-ppr-fallback-shell.test.ts (72), plus deploy/seed-cache/pregenerated-concrete-paths/prerender-route-params (334) — all green. vp check clean on touched server files.

One non-blocking documentation nit below. Nothing blocks merge.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 (feat(ppr): safely serve cacheComponents fallback shells) at the current HEAD 433cf14 — two commits past the 2f57bf6 that the previous Bonk review examined.

The two blocking concerns from the last review are resolved:

• knip / dead code: renderFreshPprFallbackShellForCache and its private helpers/types were removed in 91def17 + 433cf14. I reproduced knip locally — it now exits clean. The CI Check job should pass.
• Stale regeneration: removing the orphaned regen path makes the code, the test, and the no-op scheduleRegeneration() {} internally consistent. No-regeneration is now the deliberate, tested design.

Independently verified correct:

• Ordering: exact cache lookup → static-param validation → fallback-shell probing (app-page-dispatch.ts:830/832/845). A known route is never silently replaced by a placeholder shell.
• classifyPprFallbackShellEligibility puts the renderedConcreteUrlPaths known-route guard first, treating a missing exact cache as a transient gap.
• Scope guards (RSC/non-GET/search-params/draft-mode/cache-disabled) keep reuse narrow.
• Prior review fixes confirmed: route.fallback-authoritative bracket heuristic (legacy-only fallback), NODE_ENV-gated isReactDevelopmentRuntime.
• 72 + 334 targeted tests pass via vp; vp check clean on touched files.

One non-blocking nit: the PR description's "What Changed" table still claims stale shells "schedule regeneration," which now contradicts the intentional no-op. Posted an inline comment asking for the description to be reconciled. Nothing blocks merge.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Reviewed at HEAD 2fe663d (one commit past the 433cf14 the previous Bonk review examined). That last commit, fix(ppr): safely gate fallback shell prerendering, is a meaningful behavioral change that the earlier reviews did not see, so I focused there.

What 2fe663d does (and it's the right call): fallback-shell generation is now gated behind an internal __VINEXT_EXPERIMENTAL_PPR_FALLBACK_SHELLS env flag, defaulting off. The comment explains why — these artifacts are partial HTML/RSC shells that need a request-time resume lifecycle vinext doesn't yet implement, and serving one as complete HTML drops hydration into the global error boundary. The new ppr-impact-demo e2e (tests/e2e/ppr-impact-demo/ppr-fallback-shell.spec.ts) runs with the flag off and asserts an unknown root-param route renders safely without a partial artifact, and it's wired into CI (ci.yml). The render-side prerender/abort wiring (prerenderToReadableStream, reactAbortController, beginPprFallbackShellFinalRender, skipProbes) is coherent and the new exports are all consumed.

Verified:

• Build-time generation is gated; runtime serving (runtimeFallbackShells -> probePprFallbackShellCache) is not gated, but that's safe: with generation off no shells are prerendered, so the runtime probe finds nothing and returns null (clean fallthrough). The warmup/prerender path (app-page-dispatch.ts:981) only fires for isPrerenderFallbackShell requests, which only occur during fallback-shell prerendering — gated off.
• tests/ppr-fallback-shell.test.ts, tests/prerender.test.ts, tests/app-page-stream.test.ts pass locally (117 tests). vp check clean on the touched server files.
• The default-off prerender.test.ts now asserts /en/blog/[slug] is not queued, and the opt-in variant asserts it is — good coverage of both paths.

One blocking issue (CI): see inline. vp run knip (ci.yml:28) fails on this branch.

Non-blocking: the PR description's "What Changed" table still says stale shells "schedule regeneration," which contradicts the deliberate scheduleRegeneration() {} no-op (app-page-cache.ts:618) — already noted in a prior review. The description's overall framing ("safely serve cacheComponents fallback shells") also no longer matches the shipped default behavior, which is to not serve them; worth a description update so reviewers/readers aren't misled.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 at HEAD 2fe663d — one commit past the 433cf14 that the prior Bonk reviews examined. That latest commit (fix(ppr): safely gate fallback shell prerendering) is a substantive behavioral change the earlier reviews never saw, so I focused there.

Key finding — the latest commit reverses the PR's posture: fallback-shell generation is now gated behind an internal __VINEXT_EXPERIMENTAL_PPR_FALLBACK_SHELLS env flag, defaulting off. Partial HTML/RSC shells need a request-time resume lifecycle vinext doesn't implement yet; serving one as complete HTML drops hydration into the global error boundary. This is the safe, correct call. A new gated-off e2e (ppr-impact-demo) wired into CI verifies unknown root-param routes render safely without a partial artifact.

Verified correct:

• Build-time generation gated; runtime serving path is not gated but is safe (no shells exist to probe with generation off → clean null fallthrough).
• 117 targeted tests pass locally; vp check clean on touched files.
• Both default-off and opt-in prerender paths have coverage.

One blocking issue (CI): isExperimentalPprFallbackShellGenerationEnabled is exported but only used within its own module — vp run knip (which CI runs at ci.yml:28) flags it as an unused export, failing the Check job. I reproduced it locally and posted an inline suggestion to drop the export.

Non-blocking: the PR description's "What Changed" table (stale shells "schedule regeneration") and the overall "safely serve cacheComponents fallback shells" framing no longer match the shipped default (which is to not serve them) — worth a description update.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Reviewed at HEAD e9159b9 (fix(ppr): keep fallback shell gate internal) — one commit past the 2fe663d the previous Bonk review examined.

The prior blocking issue is resolved. e9159b9 drops the export from isExperimentalPprFallbackShellGenerationEnabled (its only caller is in prerender.ts:1198). I reproduced locally: npx knip --no-progress now exits 0, so the CI Check job should pass.

Verified correct (independent trace):

• Build-time fallback-shell generation is gated behind __VINEXT_EXPERIMENTAL_PPR_FALLBACK_SHELLS and cacheComponents === true, defaulting off (prerender.ts:1196-1216). prerender.test.ts covers all three paths: default-off does not queue /en/blog/[slug], opt-in does, and cacheComponents-disabled does not.
• Runtime serving path is not gated but is safe: with generation off no shells are prerendered, probePprFallbackShellCache finds nothing and returns null (clean fallthrough). classifyPprFallbackShellEligibility puts the renderedConcreteUrlPaths known-route guard first, treating a missing exact cache for a pregenerated route as a transient gap.
• Scope guards (RSC / non-GET / search-params / draft-mode / cache-disabled) keep reuse narrow.
• skipProbes is correctly scoped to pprFallbackShellSignal !== undefined (fallback prerender only) — app-page-render.ts:583.
• The new shim plumbing (markPprFallbackShellDynamicBoundary, suspending request-API proxy in headers()/cookies()) early-returns when no fallback-shell state is present (fallbackParamNames.size === 0), so the ordinary path is unchanged.
• ppr-impact-demo e2e is wired into CI (ci.yml, playwright.config.ts) and asserts an unknown root-param route renders without the global error UI.

Ran locally with vp: tests/ppr-fallback-shell.test.ts, tests/prerender.test.ts, tests/app-page-dispatch.test.ts, tests/app-page-cache.test.ts (158), plus tests/shims.test.ts, tests/app-page-stream.test.ts, tests/app-ppr-fallback-shell.test.ts (1116) — all green. vp check clean on touched server files.

One non-blocking observation (latent safety gap): see inline. The serve-time defense-in-depth guard relies on a marker that production code never applies. Nothing blocks merge given generation is off by default.

Doc nit: the PR title/body now correctly reflect the gated-off posture, but the "What Changed" line about stale shells scheduling regeneration still contradicts the deliberate scheduleRegeneration() {} no-op (app-page-cache.ts:618) — already raised in prior reviews; worth reconciling.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 at HEAD e9159b9 (fix(ppr): keep fallback shell gate internal) — one commit past the 2fe663d the previous Bonk review examined — and posted a batch review with one inline comment.

Prior blocking issue resolved: e9159b9 drops the export on isExperimentalPprFallbackShellGenerationEnabled, fixing the knip failure the last review flagged. I reproduced npx knip --no-progress locally — it now exits 0.

Verified correct (independent trace):

• Build-time generation gated behind the experimental env flag + cacheComponents, defaulting off. prerender.test.ts covers default-off, opt-in, and cacheComponents-disabled.
• Runtime serving path is safe via empty-shells fallthrough; classifyPprFallbackShellEligibility puts the known-route guard first.
• skipProbes correctly scoped to fallback prerender; shim plumbing is a no-op when no fallback state exists.
• E2E wired into CI; all targeted suites pass (1274 tests); vp check clean.

One new non-blocking finding (latent safety gap): the serve-time defense-in-depth guard at app-page-cache.ts:600 (isAppPprDynamicFallbackShellHtml) depends on the markAppPprDynamicFallbackShellHtml marker, but that marker is only ever applied in tests — never in the production render path. So if the experimental flag is enabled, generated partial shells wouldn't carry the marker and the guard wouldn't reject them. Latent today since generation is off by default, but the runtime guard is effectively inert in production until the marker is wired in (likely with the request-time resume follow-up).

Doc nit: the "What Changed" stale-shell-regeneration line still contradicts the deliberate scheduleRegeneration() {} no-op.

Nothing blocks merge.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Reviewed at HEAD 9a8353c (fix(ppr): mark generated dynamic fallback shells) — one commit past the e9159b9 the previous Bonk review examined.

The prior latent safety gap is resolved. The previous review flagged that the serve-time guard at app-page-cache.ts:600 (isAppPprDynamicFallbackShellHtml) depended on the markAppPprDynamicFallbackShellHtml marker, but that marker was only ever applied in tests — never in the production render path. 9a8353c wires it into the actual prerender path (build/prerender.ts:1341-1343), marking the HTML when isFallback === true. With the experimental flag enabled, generated partial shells now carry the marker, so the serve-time guard genuinely rejects them. The guard is no longer inert. prerender.test.ts now asserts the opt-in artifact contains the marker.

Verified correct (independent trace):

• Marker application is correctly scoped: isFallback is only set inside the experimental-flag + cacheComponents-gated block (prerender.ts:1199-1217), so default builds are unchanged.
• The marker is appended to the end of the HTML, after the RSC bootstrap <script> chunks. extractRscPayloadFromPrerenderedHtml scans <script> tags, so the trailing comment does not interfere with RSC payload extraction.
• The serve-time guard reads the raw cached HTML and rejects before serveAppPageCachedHtml/rewriteHtml runs, so the rewrite never sees a marked shell.
• headers()/cookies() shim integration is a no-op in the ordinary path (createPprFallbackShellSuspensePromise returns null when no fallback-shell state exists).
• server-only guard refactor (index.ts) preserves behavior — the "use server" exemption now reuses getLeadingReactDirective; tests/middleware-server-only.test.ts passes (incl. the App Router Server Actions import 'server-only' case).
• knip exits clean; vp check clean on touched files.

Ran locally with vp: prerender.test.ts (fallback-shell suite), app-page-cache.test.ts, app-ppr-fallback-shell.test.ts, app-page-dispatch.test.ts, ppr-fallback-shell.test.ts, app-page-stream.test.ts, shims.test.ts, seed-cache.test.ts, pregenerated-concrete-paths.test.ts, prerender-route-params.test.ts, and the PPR-specific deploy.test.ts cases — all green.

Note (unrelated pre-existing failure): tests/deploy.test.ts > resolveWranglerBin > returns a clear fallback path when Wrangler is missing fails in this environment, but the test is unmodified by this PR and present unchanged on main — it resolves wrangler from the monorepo's hoisted node_modules, so the "missing wrangler" simulation doesn't hold. Not a regression from this PR.

Two non-blocking observations inline. Nothing blocks merge.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 at HEAD 9a8353c (fix(ppr): mark generated dynamic fallback shells) — one commit past the e9159b9 the previous Bonk review examined — and posted a batch review with two inline comments.

The prior latent safety gap is resolved. The last review flagged that the serve-time guard (isAppPprDynamicFallbackShellHtml at app-page-cache.ts:600) relied on a marker that production code never applied — it was test-only. 9a8353c wires markAppPprDynamicFallbackShellHtml into the actual prerender path (build/prerender.ts:1341-1343) for isFallback artifacts, so generated partial shells now carry the marker and the serve-time guard genuinely rejects them. The guard is no longer inert.

Verified correct (independent trace):

• Marker scoping is correct (isFallback only set inside the experimental-flag + cacheComponents gate), so default builds are unchanged.
• The trailing comment doesn't break RSC payload extraction (which scans <script> tags).
• The serve-time guard rejects before rewriteHtml runs.
• headers()/cookies() shim integration is a no-op in the ordinary path.
• The server-only guard refactor preserves behavior (verified via tests/middleware-server-only.test.ts).
• knip clean; vp check clean.
• All targeted PPR/dispatch/cache/shim/deploy/seed-cache suites pass locally.

Two non-blocking observations posted inline:

1. useSearchParams()'s server branch doesn't call markPprFallbackShellDynamicBoundary() while usePathname/useParams/useSelectedLayoutSegments do — a possible dynamic-boundary inconsistency in the experimental generation path (latent today since generation is off by default).
2. examples/ppr-impact-demo is effectively a CI-only e2e fixture placed under examples/ (against AGENTS.md guidance) and isn't in the deploy matrix or smoke test.

Note: one failing test (tests/deploy.test.ts > resolveWranglerBin) is unrelated and pre-existing on main — an environment quirk, not a regression from this PR.

Nothing blocks merge.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Reviewed at HEAD 7a7ee0a (test(ppr): align fallback fixture and search params) — one commit past the 9a8353c the previous Bonk review examined. This commit addresses the two non-blocking observations from that review, and I found nothing blocking.

Prior observations both resolved:

1. useSearchParams dynamic-boundary inconsistency — the server branch now calls markPprFallbackShellDynamicBoundary() (navigation.ts:1507), matching usePathname/useParams/useSelectedLayoutSegments. A new shims.test.ts read-hook entry exercises useSearchParams() alongside the others. The marker is a no-op in the ordinary path — markPprFallbackShellDynamicBoundary() early-returns when state === null || fallbackParamNames.size === 0 (ppr-fallback-shell.ts:236), so normal server renders are unaffected and only the gated-off fallback-shell generation path observes the boundary.

2. examples/ppr-impact-demo placed under examples/ — moved to tests/fixtures/ppr-impact-demo per AGENTS.md guidance. I confirmed:

   • examples/ppr-impact-demo is fully removed (clean rename, no stale dir).
   • playwright.config.ts cwd/testDir and the README's relative CLI paths all updated to the new depth (../../../packages/...).
   • pnpm-lock.yaml workspace link updated; tests/fixtures/* is in pnpm-workspace.yaml, so the fixture is registered.
   • It is correctly absent from the deploy matrix and scripts/smoke-test.sh (it's a CI-only fixture), and remains wired into the e2e matrix in ci.yml:337 and the ppr-impact-demo Playwright project.

Independently verified (still correct after the move):

• npx knip --no-progress exits 0 — no unused exports; the CI Check job should pass.
• The serve-time guard (app-page-cache.ts:600) is backed by a real production marker: markAppPprDynamicFallbackShellHtml is applied in the prerender path (build/prerender.ts:1341-1343) only when isFallback === true, which is scoped inside the experimental-flag + cacheComponents gate. Default builds are unmarked and unchanged.
• Dispatch ordering invariant holds: classifyPprFallbackShellEligibility puts the renderedConcreteUrlPaths known-pregenerated-route guard as the first branch (app-page-dispatch.ts:529-533), treating a missing exact cache for a known route as a transient gap rather than an unknown signal.
• Targeted suites pass locally via vp: tests/shims.test.ts, tests/ppr-fallback-shell.test.ts, tests/app-ppr-fallback-shell.test.ts (1109), plus tests/app-page-dispatch.test.ts, tests/app-page-cache.test.ts, tests/prerender.test.ts (143).

One non-blocking doc nit (carried over, still unaddressed): the PR description's "What Changed"/posture framing has been updated to the gated-off stance, but earlier reviews noted the stale-shell-regeneration line contradicting the deliberate scheduleRegeneration() {} no-op (app-page-cache.ts:618). The current body no longer contains that table row, so this appears resolved as well.

Nothing blocks merge.

[ask-bonk]

Review submitted successfully.

Summary

I reviewed PR #1716 at HEAD 7a7ee0a (test(ppr): align fallback fixture and search params) — one commit past the 9a8353c the previous Bonk review examined. The latest commit cleanly addresses both non-blocking observations from the prior review, and I found nothing blocking.

Both prior observations resolved:

1. useSearchParams dynamic-boundary inconsistency — its server branch now calls markPprFallbackShellDynamicBoundary() (navigation.ts:1507), matching the other navigation hooks. A new shims.test.ts read-hook entry covers it. I confirmed the marker is a no-op in the ordinary path (ppr-fallback-shell.ts:236 early-returns when no fallback-shell state is active).
2. Test fixture placement — examples/ppr-impact-demo moved to tests/fixtures/ppr-impact-demo per AGENTS.md. I verified the rename is clean, the relative CLI paths and Playwright cwd/testDir are updated to the new depth, the lockfile workspace link is fixed, and the fixture is correctly absent from the deploy matrix/smoke tests while remaining wired into the e2e matrix.

Independently verified still-correct after the move: knip exits 0 (CI Check should pass), the serve-time guard is backed by the production marker in the gated prerender path, the dispatch ordering invariant holds (known-pregenerated-route guard first), and targeted suites pass locally (1109 + 143 tests).

The carried-over doc nit about stale-shell regeneration also appears resolved — that table row is no longer in the description.

Nothing blocks merge.

github run