fix(app-router): preload next/dynamic chunks with CSP nonce#1594
fix(app-router): preload next/dynamic chunks with CSP nonce#1594NathanDrake2406 wants to merge 16 commits into
Conversation
Rendered next/dynamic boundaries produced no SSR preload links for their client chunks. That diverged from Next.js and broke nonce-based CSP tests that inspect preload tags before hydration. The runtime assumed React.lazy alone was enough for dynamic imports. It never carried the dynamic module IDs from the source call into SSR, and client JS chunk URLs did not use Next.js's static/chunks path shape. Add a focused dynamic metadata transform, resolve boundary files from the client build manifest, emit nonce-bearing preload hints during SSR, and keep client JS assets under _next/static/chunks. Ported regression coverage verifies the nonce and the emitted path shape.
commit: |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15122a8a25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const dynamicPreloads = dynamicImportPreloadsWithBase( | ||
| computeDynamicImportPreloads(buildManifest), | ||
| (file) => manifestFileWithBase(file, clientBase), | ||
| ); |
There was a problem hiding this comment.
Build-time dynamic preload metadata is normalized with envConfig.base (manifestFileWithBase(file, clientBase)), but base only tracks basePath and does not carry assetPrefix. For deployments that set assetPrefix (especially absolute CDN prefixes), the injected __VINEXT_DYNAMIC_PRELOADS__ entries stay as /_next/static/..., so next/dynamic preload hints point at the app origin instead of the configured asset host. This diverges from Next.js assetPrefix semantics and causes incorrect preload requests (and CSP host mismatches) whenever assetPrefix is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled in the current code. Both the closeBundle hook (index.ts lines 4476-4482) and installClientBuildManifestGlobals (prod-server.ts lines 301-331) pass assetPrefix through to manifestFileWithAssetPrefix(file, clientBase, nextConfig.assetPrefix). The dynamic preload URLs are correctly prefixed when assetPrefix is set. The deploy test at tests/app-router.test.ts:2434 and tests/deploy.test.ts:2231 cover this explicitly. No code change needed.
| const appAssetBase = appRouterBasePath ? `${appRouterBasePath}/` : "/"; | ||
| installClientBuildManifestGlobals(clientDir, appAssetBase); |
There was a problem hiding this comment.
App Router Node production startup computes appAssetBase from __basePath only and feeds it into installClientBuildManifestGlobals, which populates __VINEXT_DYNAMIC_PRELOADS__/__VINEXT_LAZY_CHUNKS__. When assetPrefix is configured (e.g., CDN origin or a distinct path prefix), these globals are generated without that prefix, so rendered next/dynamic preload URLs resolve to the wrong origin/path. This breaks parity with Next.js and can trigger 404s or CSP source mismatches in App Router prod deployments using assetPrefix.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled. installClientBuildManifestGlobals at prod-server.ts:301 accepts assetPrefix as a parameter and passes it to manifestFileWithAssetPrefix for both lazy chunks and dynamic preloads. The App Router startup at line ~340 calls this function with the resolved assetPrefix. See also the deploy test at tests/deploy.test.ts:2253 which verifies absolute assetPrefix URLs in dynamic preload globals. No code change needed.
| // Load the build manifest to compute lazy chunks and rendered next/dynamic | ||
| // preload files. These globals are read by shims during SSR. | ||
| installClientBuildManifestGlobals(clientDir, assetBase); |
There was a problem hiding this comment.
Pages Router production now also relies on installClientBuildManifestGlobals for __VINEXT_DYNAMIC_PRELOADS__, but this call passes assetBase derived from basePath only. If assetPrefix points to a CDN or a different path prefix, the generated preload/lazy-chunk URLs are missing that prefix, so SSR emits next/dynamic preload hints to the wrong location. This causes incorrect asset fetches (and possible CSP source mismatches) specifically in Pages Router Node production when assetPrefix is set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already handled. The same installClientBuildManifestGlobals function serves both App Router and Pages Router paths. It receives assetPrefix and passes it through to manifestFileWithAssetPrefix for both lazy chunks and dynamic preloads. The Pages Router startup at prod-server.ts calls this function with the resolved assetPrefix. No code change needed.
App Router production only covered the default dynamic preload URL path. That left assetPrefix regressions at the manifest-to-runtime emission boundary unguarded. Add a startProdServer regression that builds app-basic with assetPrefix "/cdn" and asserts nonce-bearing next/dynamic chunk preloads render under /cdn/_next/static/chunks/.
The assetPrefix regression starts a second production server in the same Vitest process. startProdServer and the generated RSC runtime install global manifest and module-loader state, so leaving the temporary build's globals in place breaks later production requests in the shared server. Snapshot and restore the relevant globals around the temporary /cdn server while keeping the integration assertion at the real runtime boundary.
The dynamic preload metadata transform carried a local object guard even though the codebase already has a shared record predicate. That made the AST helper drift from the existing non-array record invariant used elsewhere. Reuse isUnknownRecord from utils/record and keep the plugin-specific AST alias local to the transform.
The next/dynamic metadata transform matched calls by imported identifier text. Shadowed parameters and block bindings with the same name were therefore mutated even when they no longer referred to the next/dynamic import. Walk the parsed module with lexical binding state so shadowed names are excluded, and collect object-form imports only from loader/modules rather than every property on the options object.
The dynamic preload metadata transform still treated imported next/dynamic names as active inside switch case lexical scopes and named class expression bodies. That could inject loadableGenerated metadata into calls that resolve to local bindings rather than the import. The scope walker now removes switch case-block bindings while traversing case tests and consequents, leaves the switch discriminant in the outer scope, and scopes named class declarations and expressions through their children. Regression tests cover switch case shadowing and named class expressions.
There was a problem hiding this comment.
Pull request overview
Aligns vinext’s next/dynamic SSR behavior with Next.js under nonce-based CSP by recording dynamic boundary module IDs at build/transform time, resolving their emitted JS/CSS from Vite’s client manifest, and rendering nonce-bearing preload hints during SSR. Also updates client JS output paths to use Next.js’s canonical /_next/static/chunks/… shape.
Changes:
- Add a Vite transform (
vinext:dynamic-preload-metadata) that injectsloadableGenerated.modulesfornext/dynamic()callsites. - Compute per-module dynamic preload file lists from Vite’s client manifest and expose them via
globalThis.__VINEXT_DYNAMIC_PRELOADS__for SSR/Workers. - Render nonce-bearing dynamic preload hints during SSR and move client JS entry/chunk output under
/_next/static/chunks/(tests updated accordingly).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/pages-router.test.ts | Updates expectations for client JS output under _next/static/chunks/. |
| tests/deploy.test.ts | Extends closeBundle injection tests to include __VINEXT_DYNAMIC_PRELOADS__ and assetPrefix behavior. |
| tests/build-optimization.test.ts | Adds unit tests for dynamic preload computation + metadata transform; asserts chunk output naming. |
| tests/asset-prefix.test.ts | Adds coverage for manifestFileWithAssetPrefix; adjusts JS presence check for nested chunk dirs. |
| tests/app-router.test.ts | Adds CSP nonce regression tests for dynamic preloads; updates static asset path assertions. |
| packages/vinext/src/utils/manifest-paths.ts | Introduces manifestFileWithAssetPrefix() for assetPrefix-aware manifest URL construction. |
| packages/vinext/src/utils/lazy-chunks.ts | Adds computeDynamicImportPreloads() + helper to apply base/prefix mapping. |
| packages/vinext/src/shims/script-nonce-context.tsx | Makes nonce context/hook resilient when React context/hooks aren’t available. |
| packages/vinext/src/shims/dynamic.ts | Adds SSR-time dynamic preload emission using nonce context + __VINEXT_DYNAMIC_PRELOADS__. |
| packages/vinext/src/server/prod-server.ts | Loads client manifest at startup to install lazy + dynamic preload globals (Node prod). |
| packages/vinext/src/plugins/dynamic-preload-metadata.ts | New transform plugin that injects loadableGenerated.modules for next/dynamic. |
| packages/vinext/src/index.ts | Wires new plugin; enables client manifest generation; adjusts client output naming config; injects dynamic preload globals for Workers builds. |
| packages/vinext/src/global.d.ts | Declares globalThis.__VINEXT_DYNAMIC_PRELOADS__. |
| packages/vinext/src/build/client-build-config.ts | Adds _next/static/chunks entry/chunk naming helpers (Rollup/Rolldown). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stylesheets.push( | ||
| React.createElement("link", { | ||
| key: href, | ||
| rel: "stylesheet", | ||
| as: "style", | ||
| href, | ||
| nonce, | ||
| precedence: "dynamic", | ||
| }), |
There was a problem hiding this comment.
Fixing in this PR. While Next.js upstream also emits as="style"" on (seepreload-chunks.tsx:59), the asattribute is semantically arel="preload"" attribute per the HTML spec — it tells the browser the destination of a preload. On rel="stylesheet" it is redundant and ignored by browsers. Dropping it produces cleaner HTML without changing behaviour. This is a deliberate improvement over the upstream output.
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: next/dynamic CSP nonce preload support
This is a well-structured PR that faithfully implements Next.js's next/dynamic SSR preload behavior. The architecture is clean: transform-time metadata injection → build-manifest resolution → SSR-time nonce-bearing preload emission. I verified the implementation matches Next.js's PreloadChunks component behavior across all four key dimensions (JS via ReactDOM.preload(), CSS via <link rel="stylesheet" precedence="dynamic">, as="style" presence, and ssr: false exclusion).
Architecture strengths:
- The scope analysis in
dynamic-preload-metadata.tsis thorough — it correctly handles block scoping, function scoping, switch-case scoping, for-loop bindings, catch clauses, and class expressions. The shadow-tracking approach (narrowingdynamicLocalsas scopes introduce bindings) is the right way to do this. - Dev/prod parity is correct:
DynamicPreloadChunksgracefully no-ops in dev (no build manifest → empty preload map → null render). Both Node.js prod and Cloudflare Workers paths inject the globals. - The
installClientBuildManifestGlobalsextraction inprod-server.tsis a nice deduplication of the Pages Router and App Router startup paths. - The
manifest: truechange from conditional (Cloudflare-only) to unconditional is the right call — prod-server.ts now reads the manifest too.
Regarding the Codex review comments about assetPrefix: These are addressed in this PR. Both installClientBuildManifestGlobals calls pass assetPrefix through to manifestFileWithAssetPrefix, and the closeBundle hook uses the same function. The Codex reviewer appears to have been looking at an earlier commit.
Regarding the Copilot comment about as="style" on <link rel="stylesheet">: This matches Next.js's implementation exactly — confirmed by reading packages/next/src/shared/lib/lazy-dynamic/preload-chunks.tsx in the Next.js source. Not a bug.
I have a few inline comments — one concrete suggestion and a few observations for follow-up. Overall this is ready to land.
Test coverage note: The transform tests cover shadowing well, but there's no test for multiple dynamic() calls in a single file each getting distinct loadableGenerated metadata. This is a common real-world pattern (pages with 2-3 dynamic imports). Consider adding one as a follow-up.
| pending.push( | ||
| resolveManifestModuleIds(specifiers, id, root, resolveDynamicImport).then((moduleIds) => { | ||
| if (moduleIds.length === 0) return; | ||
| if (applyLoadableGenerated(output, code, node, moduleIds)) { |
There was a problem hiding this comment.
Nit: The Promise.all(pending) pattern where each .then callback mutates the shared MagicString is safe because JS is single-threaded (microtask callbacks are serialized), but it's the kind of pattern that makes a reader pause. A brief comment like // Mutations are safe: microtask callbacks run sequentially would help future readers.
Also worth noting: if two dynamic() calls in the same file both hit the insertSecondOptionsArgument path with overwrite, the non-overlapping ranges ensure MagicString won't throw — but only because each call node occupies a distinct source range. This invariant holds as long as visitDynamicCalls never visits the same call node twice, which the scope-narrowing approach guarantees.
| return source === "next/dynamic" || source === "next/dynamic.js"; | ||
| } | ||
|
|
||
| function collectDynamicImportLocals(ast: unknown): Set<string> { |
There was a problem hiding this comment.
This only collects ImportDefaultSpecifier bindings. It correctly misses import * as Dynamic from "next/dynamic" (namespace) and import { default as dynamic } from "next/dynamic" (named default re-import). Both are uncommon patterns for next/dynamic, but Next.js's own Babel plugin also only handles ImportDefaultSpecifier, so this is correct parity. Just flagging for awareness.
| const firstArgEnd = getNumber(firstArg, "end"); | ||
| if (callEnd === null || firstArgEnd === null) return false; | ||
|
|
||
| const closeParen = callEnd - 1; |
There was a problem hiding this comment.
Minor: closeParen = callEnd - 1 assumes the AST end position is exclusive (past the )). This is true for both ESTree/acorn (which Vite's parseAst uses via OXC) and the OXC parser itself, so it's correct. Just worth a brief comment since the assumption is load-bearing — if the parser ever changed to inclusive end positions, this would silently inject into the wrong location.
| const closeParen = callEnd - 1; | |
| const closeParen = callEnd - 1; // AST `end` is exclusive (past the closing paren) |
| continue; | ||
| } | ||
|
|
||
| if (href.endsWith(".js") && typeof ReactDOM.preload === "function") { |
There was a problem hiding this comment.
Nice: using ReactDOM.preload() for JS and returning null exactly matches Next.js's PreloadChunks component. The typeof ReactDOM.preload === "function" guard is a good defensive check for environments where the Float API might not be available.
| import React from "react"; | ||
|
|
||
| export const ScriptNonceContext = React.createContext<string | undefined>(undefined); | ||
| export const ScriptNonceContext = |
There was a problem hiding this comment.
Good defensive coding. React.createContext is not available in the RSC react-server condition (well, it is in React 19.x, but this guards against future React versions stripping it). The createScriptNonceHook factory pattern below also correctly handles React.useContext absence. This ensures DynamicPreloadChunks can be imported in RSC environments without crashing, even though it won't render preloads there.
| * @returns A map keyed by root-relative module ID, with JS/CSS files that | ||
| * should be preloaded when that dynamic boundary is rendered. | ||
| */ | ||
| export function computeDynamicImportPreloads( |
There was a problem hiding this comment.
The algorithm correctly collects transitive static imports from each dynamic entry but does NOT recurse into nested dynamicImports. This matches the intent: each next/dynamic boundary should only preload its own direct dependency tree, not the full transitive closure including nested lazy boundaries.
One edge case to be aware of: if two different parent chunks list the same dynamicKey in their dynamicImports, computeDynamicImportPreloads processes it multiple times (the inner for (const dynamicKey of chunk.dynamicImports) loop). The normalizeManifestKey + merge-into-existing logic (lines 149-157) handles deduplication correctly, but it does redundant work. For large manifests this is fine — just noting for future optimization if needed.
| } | ||
| } | ||
|
|
||
| function installClientBuildManifestGlobals( |
There was a problem hiding this comment.
Clean extraction. Previously the Pages Router had its own inline manifest loading, and App Router didn't compute dynamic preloads at all. Consolidating into a shared function ensures both router types stay in sync — which is exactly what AGENTS.md prescribes for prod-server.ts changes.
The undefined reset at lines 269-270 before loading is a good practice for the Node.js prod server case where the process persists (not Cloudflare Workers where the global is prepended once).
|
Review submitted with approval. Here's a summary of what I found: Verdict: Approved. The PR is well-structured and faithfully implements Next.js's Key findings:
Inline comments posted:
|
…-csp-nonce # Conflicts: # packages/vinext/src/build/client-build-config.ts # packages/vinext/src/index.ts # packages/vinext/src/server/prod-server.ts
- Drop `as="style"` from `<link rel="stylesheet">` — `as` is a
preload attribute per the HTML spec, semantically incorrect on
stylesheet links. Browsers ignore it but it produces bad HTML.
- Remove brittle `import(` string gate from the transform — it could
miss `import ("./x")` (whitespace between import and paren). The
Vite filter already gates on "next/dynamic" and the AST parse is the
real validation.
- Add comment explaining MagicString mutation safety in Promise.all.
- Add comment explaining AST end-exclusive assumption on closeParen.
- Add test for whitespace-separated dynamic import syntax.
Extract the duplicated client-manifest runtime metadata computation (lazy chunks, dynamic preloads, client entry) into a single shared helper in utils/client-runtime-metadata.ts. Both prod-server.ts (Pages Router Node.js production server) and index.ts (Cloudflare closeBundle hook) now call the same helper, eliminating the highest-risk seam in the PR where basePath/ assetPrefix handling produced bugs.
- Add post-type-strip generic call test verifying multi-line dynamic() calls work after TS/JSX stripping by esbuild. - Add object-form existing loadableGenerated preservation test to guard against duplicate injection in the object overload.
simulateCloseBundleAppRouter and simulateCloseBundlePagesRouter no longer hand-roll lazy chunk and dynamic preload computation. They call the shared computeClientRuntimeMetadata helper directly, matching production wiring. Also: - Update helper doc comment to mention Node server startup - Remove unused imports (manifestFileWithAssetPrefix, manifestFileWithBase, computeDynamicImportPreloads, dynamicImportPreloadsWithBase) from deploy.test.ts
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
There was a problem hiding this comment.
Review: next/dynamic CSP nonce preload
Reviewed the implementation end-to-end and ran the targeted suites (build-optimization, client-build-manifest, asset-prefix) plus vp check on the changed source files — all green. This is a solid, well-tested PR that faithfully ports Next.js's next/dynamic SSR preload behavior with CSP nonce support.
Things I verified:
- The Codex assetPrefix concerns are genuinely addressed. Both production startup paths feed
assetPrefixthrough tocomputeClientRuntimeMetadata→manifestFileWithAssetPrefix: App Router atprod-server.ts:1104(installClientBuildManifestGlobals(clientDir, appAssetBase, appRouterAssetPrefix)) and Pages Router atprod-server.ts:1431-1436. ThecloseBundlehook (index.ts:4457-4462) passesnextConfig.assetPrefixtoo. The deploy tests attests/deploy.test.tsnow exercise the real helper rather than hand-rolled manifest logic. - Scope analysis in the transform is thorough. Block / function / switch-case / for-loop / catch / class shadowing are all handled, and
varvs block-scoped bindings are correctly distinguished (collectBlockScopedBindingNamesexcludesvar,collectVarBindingNameshoists it to function scope). - Module-ID keying is consistent.
toManifestModuleId(transform) andnormalizeManifestKey(lazy-chunks) both produce root-relative, forward-slash, query-stripped keys, so the__VINEXT_DYNAMIC_PRELOADS__lookup atdynamic.ts:126lines up. - Dev/prod parity is correct.
DynamicPreloadChunksno-ops in dev (no manifest → empty map →null), and the RSC environment is guarded by the defensiveScriptNonceContext/useContexthandling. - Edge cases hold up. Empty-object first arg, spread-only options object, and trailing-comma
dynamic(fn,)all produce syntactically valid output throughappendObjectProperty/insertSecondOptionsArgument.
No blocking issues. A few minor observations below.
| root: string, | ||
| resolveDynamicImport: ResolveDynamicImport, | ||
| ): Promise<TransformResult | null> { | ||
| if (!code.includes("next/dynamic")) return null; |
There was a problem hiding this comment.
Minor: this early code.includes("next/dynamic") guard is now redundant with the plugin's transform.filter.code: "next/dynamic" (line 592), which Vite enforces before the handler runs. Harmless, and it keeps transformNextDynamicPreloadMetadata usable as a standalone tested function (which the unit tests rely on), so I'd leave it — just flagging that the two checks overlap.
| visitChildren(value, dynamicLocals, visitor); | ||
| } | ||
|
|
||
| function collectImportSpecifiers(node: unknown): string[] { |
There was a problem hiding this comment.
collectImportSpecifiers walks every descendant of the loader node, so it will also pick up unrelated nested import() calls inside the loader body (e.g. a lazy import made conditionally within the loader function). This matches Next.js's react-loadable Babel plugin, which likewise collects all import() callsites in the loader, so it's intentional parity — just noting the behavior since it can over-collect module IDs in unusual loaders. The downstream seen/dedup logic keeps the result correct.
| continue; | ||
| } | ||
|
|
||
| if (href.endsWith(".js") && typeof ReactDOM.preload === "function") { |
There was a problem hiding this comment.
Using ReactDOM.preload() for JS (side-effecting during render) and rendering <link rel="stylesheet" precedence="dynamic"> for CSS matches Next.js's PreloadChunks component, and the typeof ReactDOM.preload === "function" guard is a good defensive check for environments without the Float API. One thing worth confirming in a follow-up: for cross-origin absolute assetPrefix (CDN) URLs, the preload nonce is still emitted — that's correct for CSP script-src, but make sure the deploy test at tests/deploy.test.ts covering absolute assetPrefix also asserts the nonce on those cross-origin hrefs, not just the path-prefix case.
|
Review submitted successfully. Here's a summary of my findings: Verdict: Approved — no blocking issuesThis PR faithfully ports Next.js's What I verified:
Three minor inline observations posted (none blocking):
|
Overview
Preload
next/dynamic()chunks with the request CSP nonce, matching Next.js behavior for apps using nonce-based CSP.Key changes
dynamic-preload-metadata.ts): walks AST fornext/dynamic()calls, records resolved module IDs via Vite's resolver, and injectsloadableGeneratedmetadata (mirrors Next.js's react-loadable Babel plugin).lazy-chunks.ts): reads Vite's client build manifest to resolve each dynamic boundary to its real JS/CSS file paths.dynamic.ts):DynamicPreloadChunksrenders nonce-bearing<link rel="preload">and<link rel="stylesheet">for rendered dynamic boundaries._next/static/chunks/: moves emitted client JS to match Next.js's URL convention (required because preload URLs must match how the browser will request the chunks).computeClientRuntimeMetadatahelper (client-runtime-metadata.ts): single source of truth for lazy chunks, dynamic preloads, and client entry computation — used by both Node prod server startup and Cloudflare closeBundle injection.Refinements from review
<link rel="stylesheet">dropsas="style"(valid only forrel="preload"per HTML spec; deliberate improvement over Next.js).code.includes("import(")string gate — Vite'sfilter.codeon"next/dynamic"is both correct and consistent.computeClientRuntimeMetadatahelper instead of hand-rolling manifest logic.dynamic<Props>()calls (post-type-strip), object-formloadableGeneratedpreservation, whitespace-separatedimport ( ... )syntax.Closes #1516