fix(app-router): bail out static search params rendering - #2882
Conversation
|
/bigbonk review for issues |
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): bail out static search params rendering
I traced the full data flow and ran the affected tests. No blocking issues found. The change is correct, well-scoped, and matches Next.js useDynamicSearchParams semantics.
What I verified
1. Cross-environment PPR state sharing is sound. The bailout guard is:
ctx?.isStaticGeneration === true && ctx.isForceStatic !== true && getPprFallbackShellState() === nullThe concern was that useSearchParams() runs in a "use client" component during the SSR environment (a separate Vite module graph from RSC), so a module-local getPprFallbackShellState() could return null during PPR fallback generation and incorrectly bail out. This is not a problem: ppr-fallback-shell.ts obtains its ALS via getOrCreateAls(...), which is a globalThis[Symbol.for(...)] cross-module-instance singleton (shims/internal/als-registry.ts). Combined with dispatchAppPage wrapping the entire dispatch — including the awaited handleSsr — in pprRuntime.run() (app-page-dispatch.ts:618), the SSR-env call correctly sees the non-null shell state and suppresses the bailout. The PPR exclusion described in the PR body holds.
2. State threading is complete for both static paths.
- Lifecycle render:
app-page-render.ts:1065setsisStaticGeneration: options.isPrerender === true— no bailout for ordinary runtime SSR (confirmed by the new dev-server test). - Cache regeneration:
app-page-cache-render.ts:98setsisStaticGeneration: trueunconditionally withisForceStaticforwarded fromrevalidationDynamicConfig === "force-static"(app-page-dispatch.ts:824), consistent with the siblingobserve*SearchParamsAccessguards. - The single
renderAppPageHtmlStreamcall site (viarenderAppPageHtmlStreamWithRecovery) carries the flags throughapp-page-stream.ts→handleSsr.
3. No client-payload leak. buildHeadInjectionHtml (app-ssr-entry.ts:323) serializes only pathname/searchParams/params into the bootstrap script; isStaticGeneration/isForceStatic are not serialized, matching the navigation-context-state.ts doc comment ("Client navigation payloads omit it").
4. handleSsr shallow-copy is safe. The new spread at app-ssr-entry.ts:407 creates a fresh nav context; both setNavigationContext and buildHeadInjectionHtml (line 703) read the copy, so no identity/mutation regression.
5. force-static path. ctx.isForceStatic !== true short-circuits the bailout and falls through to the pre-existing empty-search-params snapshot behavior (verified by the new shims test).
Tests run locally (all pass)
tests/shims.test.ts— new bailout / fallback-shell / force-static casestests/prerender.test.ts— Suspense fallback emitted into static HTMLtests/app-router-dev-server.test.ts— runtime SSR stays server-renderedtests/app-page-cache-render.test.ts— regeneration marks static statevp checkon all 5 changed source files — format/lint/types clean
Minor observations (non-blocking)
- The
shims.test.tsunit tests exercisegetPprFallbackShellState()synchronously (same call stack), which does not cover the RSC→SSR cross-environment ALS propagation. Theprerender.test.tsintegration test does cover the real path, so coverage is adequate — just noting the unit test alone would not catch an ALS-scoping regression. - Consider a future integration test asserting a PPR route with
useSearchParamsdoes not bail out (the current PPR unit test only checkshasDynamicBoundary, not the end-to-end HTML), to lock in the exclusion at the integration level.
LGTM.
|
Review posted to PR #2882. Summary of my findings: No blocking issues foundI traced the full data flow and ran the affected tests. The change correctly threads static-generation state through the App Router SSR boundary and throws the canonical Key correctness checks that passed:
Tests I ran locally — all pass: the new shims, prerender, dev-server, and cache-render cases, plus I noted two non-blocking suggestions: the unit tests don't cover the RSC→SSR cross-environment ALS path (the integration prerender test does), and a future integration test asserting a PPR route does not bail out would lock in the exclusion end-to-end. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): bail out static search params rendering
I independently traced the full data flow and ran the affected tests. No blocking issues found. The change is correct, well-scoped, and matches Next.js useDynamicSearchParams semantics.
What I verified
1. State threading is complete and asymmetric in the right way.
- Lifecycle render sets
isStaticGeneration: options.isPrerender === true(app-page-render.ts:1065), so ordinary runtime SSR never bails (confirmed by the newapp-router-dev-server.test.tscase, which asserts?value=runtimestill server-rendersruntime). - Cache regeneration sets
isStaticGeneration: trueunconditionally withisForceStaticforwarded fromrevalidationDynamicConfig === "force-static"(app-page-dispatch.ts:824). isForceStaticis a real required field on the lifecycle options type (app-page-render.ts:139), not a silentundefinedtypo — plumbing is genuine.
2. Cache-regeneration path can't wrongly bail dynamic routes. My main correctness concern was that renderAppPageCacheArtifacts sets isStaticGeneration: true unconditionally. But regeneration is only entered for cacheable artifacts: force-dynamic / revalidateSeconds === 0 are excluded from caching (app-page-method.ts:23, app-page-cache.ts:461), and the regenerated element is built with an empty new URLSearchParams() (app-page-dispatch.ts:789). So a dynamic route never reaches this path, and search params are correctly meaningless in the static artifact.
3. Cross-environment PPR exclusion holds. useSearchParams() runs in the SSR Vite environment (separate module graph from RSC). The getPprFallbackShellState() === null guard works across environments because the PPR ALS is a globalThis[Symbol.for(...)] singleton (shims/internal/als-registry.ts) and dispatchAppPage wraps the awaited handleSsr in pprRuntime.run(). When the shell state is non-null the code skips the throw and falls through to markPprFallbackShellDynamicBoundary(), preserving hasDynamicBoundary (verified by the fallback-shell shims test). In the non-PPR static path there is no shell, so throwing before markPprFallbackShellDynamicBoundary() (a no-op there) is harmless.
4. Bailout is a recognized control-flow signal. BailoutToCSRError's BAILOUT_TO_CLIENT_SIDE_RENDERING digest is matched by getDigestForWellKnownError (app-rsc-errors.ts:62), so it is not reported as a real server error — React renders the nearest Suspense fallback instead of crashing the render.
5. No client-payload leak. buildHeadInjectionHtml serializes only pathname/searchParams/params (app-ssr-entry.ts:323); the new isStaticGeneration/isForceStatic fields are never sent to the client, matching the navigation-context-state.ts doc comment. The handleSsr shallow spread (app-ssr-entry.ts:407) creates a fresh nav context read by both setNavigationContext and buildHeadInjectionHtml, so no identity/mutation regression.
6. Symmetry with sibling guards. isForceStatic is applied consistently with the existing observeMetadataSearchParamsAccess/observePageSearchParamsAccess !== "force-static" guards on the RSC side. Those observe server-component searchParams props; the new bailout covers SSR of client-component useSearchParams() — distinct mechanisms, kept consistent.
Tests run locally (all pass)
tests/shims.test.ts -t useSearchParams— 7 passed (bailout, fallback-shell preservation, force-static empty snapshot)tests/prerender.test.ts— the integration case asserts the Suspense fallback IS emitted into static HTML andid="search-params-value"is NOT, exercising the real RSC→SSR cross-environment path end-to-endtests/app-page-cache-render.test.ts,tests/app-page-stream.test.ts,tests/app-router-dev-server.test.ts— all passvp checkon all 7 changed source files — format/lint/types clean
Minor observations (non-blocking)
- The
shims.test.tsunit tests exercisegetPprFallbackShellState()on the same synchronous call stack, so they don't cover the RSC→SSR cross-environment ALS propagation on their own. Theprerender.test.tsintegration test does cover the real path, so coverage is adequate. - A future integration test asserting a PPR route with
useSearchParamsdoes not bail out (current PPR unit coverage only checkshasDynamicBoundary) would lock the exclusion in at the integration level.
LGTM.
|
Review posted to PR #2882. SummaryI independently reviewed PR #2882 ( Key correctness checks that passed
The |
|
Independent exact-head review of I reviewed the full fourteen-file diff independently against Next.js's current The cache-render touch is route-artifact regeneration plumbing, not Fresh exact-head validation:
I found no actionable bailout, PPR, force-static, ordinary-SSR, or scope-isolation issue. |
1 participant
Summary
Fix the non-cache App Router parity failure from Actions run 31439707085, job 93624401572:
test/e2e/app-dir/app-static/app-static.test.tsuseSearchParams server response should bailout to client rendering with suspense boundaryDuring legacy static generation, vinext previously supplied an empty server navigation search-params value to client components. That rendered
N/Ainto static HTML instead of throwing the canonical CSR bailout that lets React emit the nearest Suspense fallback.This change threads static-generation and
force-staticstate through the shared App Router SSR boundary, throwsBailoutToCSRError("useSearchParams()")only for non-force-staticlegacy static artifacts, and applies the same state during runtime cache regeneration. Ordinary dev/runtime SSR remains server-rendered.Active PPR fallback-shell generation is deliberately excluded from the legacy bailout so
useSearchParams()continues to markhasDynamicBoundaryand preserve the existing PPR abort/cache-task coordination.Existing james-elicx PR #2243 was not adopted because it conflicts with current
main.Next.js reference
Verified against Next.js v16.2.6 at
ee6e79b1792a4d401ddf2480f40a83549fe8e722, including itsuseDynamicSearchParamsbehavior and the exact upstream app-static fixture/assertions.Validation
REPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-dir/app-static/app-static.test.tsupdateTag/revalidateTag should successfully update tag when called from server action; this is cache-function behavior explicitly outside this backlog item and unrelated to the changed static navigation/PPR pathsshims, app-page stream/cache-render/render/dispatch): 1,440/1,440 passedvp run vinext#build: passedexamples/app-router-cloudflareproduction build: all 5/5 RSC/client/SSR environments passedcdffbc54b6866d47770f8c66b8e411ede7263bdb: no blocking issuesThe only post-implementation follow-up narrows the dev regression to assert the resolved runtime query value. A cold streaming render may legitimately include a transient Suspense shell before the resolved value in the same HTML response; presence of the resolved value is what distinguishes runtime SSR from the static CSR bailout.
This PR is intentionally limited to the non-cache
useSearchParamsSuspense failure. It does not changeuse cache, Cache Components, or cache-function semantics.