fix: warm server action modules in dev#995
fix: warm server action modules in dev#995NathanDrake2406 wants to merge 2 commits intocloudflare:mainfrom
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/server-action-dev-warmup
branch
from
04f0de8 to
00e3297
Compare
![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: 04f0de812c
ℹ️ 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 extensions = normalizePageExtensions(options.pageExtensions); | ||
| const pattern = `**/*.${buildExtensionGlob(extensions)}`; |
There was a problem hiding this comment.
The warmup collector currently limits its scan to nextConfig.pageExtensions, so projects that set pageExtensions to route-only values like ['tsx', 'jsx'] will skip valid server action modules such as app/actions.ts. Those modules are still legal imports in App Router code, so they can still be referenced by forms, but they won’t be pre-warmed and the original cold-POST failure this patch fixes can still occur for those apps.
Useful? React with 👍 / 👎.
NathanDrake2406
force-pushed
the
nathan/server-action-dev-warmup
branch
from
00e3297 to
66363b0
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean implementation — the directive parser is careful, the extension union handles the pageExtensions concern correctly (addresses the Codex P2; SERVER_ACTION_SOURCE_EXTENSIONS always provides the full base set), and the test coverage is thorough. Two observations worth considering:
| const actionWarmupEntries = await collectServerActionWarmupEntries({ | ||
| root, | ||
| appDir, | ||
| pageExtensions: nextConfig?.pageExtensions, | ||
| }); |
There was a problem hiding this comment.
This scan reads every file under app/ to check for the directive prologue, which is correct for dev but unnecessary during production builds — dev.warmup is a no-op when command === "build". For large apps this is wasted I/O at build time.
Consider guarding with the config env:
| const actionWarmupEntries = await collectServerActionWarmupEntries({ | |
| root, | |
| appDir, | |
| pageExtensions: nextConfig?.pageExtensions, | |
| }); | |
| const actionWarmupEntries = | |
| env?.command !== "build" | |
| ? await collectServerActionWarmupEntries({ | |
| root, | |
| appDir, | |
| pageExtensions: nextConfig?.pageExtensions, | |
| }) | |
| : []; |
| if (char === "\\") { | ||
| const escaped = source[index + 1]; | ||
| if (escaped === undefined) { | ||
| return null; | ||
| } | ||
| value += escaped; |
There was a problem hiding this comment.
Nit: The escape handling here treats \s → s, \n → n (literal), etc. That's fine for the warmup scanner's purposes (nobody writes "use \x73erver" in practice), but it does mean the parsed value diverges from what a JS engine would produce for escape sequences like \n, \t, \u0073, or \x73.
Not a correctness issue for the "use server" check today — just noting it so we don't accidentally reuse this parser for general string literal extraction later.
|
Review posted. Here's a summary of my findings: Overall: This is a well-structured PR. The directive parser correctly implements the ECMAScript directive prologue semantics (whitespace/comments between directives, BOM handling, semicolons optional), the extension handling correctly takes the union of base extensions and Two comments posted:
|
NathanDrake2406
force-pushed
the
nathan/server-action-dev-warmup
branch
from
66363b0 to
9655358
Compare
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The directive parser correctly implements ECMAScript directive prologue semantics, the extension handling is right (the base SERVER_ACTION_SOURCE_EXTENSIONS always covers .ts/.mts/etc. regardless of pageExtensions), the build-time skip is already in place (env?.command === "build" guard at index.ts:1306), and the test suite covers unit, integration, and the build-skip path.
One minor observation and one small suggestion below — neither is blocking.
| } | ||
|
|
||
| export function hasModuleUseServerDirective(source: string): boolean { | ||
| let index = source.charCodeAt(0) === 0xfeff ? 1 : 0; |
There was a problem hiding this comment.
Nit: The BOM skip is good, but the parser doesn't handle hashbangs (#!/usr/bin/env node). If a file starts with #!, readDirectiveLiteral sees # (not a quote) and returns null, so the function returns false — the file would not be warmed up.
In practice nobody writes #!/usr/bin/env node followed by "use server", so this is purely theoretical. Just flagging it for completeness since the Next.js reference parser (getDirectiveFromByteCode) also doesn't handle hashbangs.
| const pattern = `**/*.${buildExtensionGlob(extensions)}`; | ||
| const entries: string[] = []; | ||
|
|
||
| for await (const relativeFile of glob(pattern, { cwd: options.appDir })) { |
There was a problem hiding this comment.
The for await loop reads each file sequentially — await readFile inside the loop body means each file is read only after the previous one finishes. For apps with many files under app/, this serializes all the I/O.
Not blocking (the scan only runs once at dev startup and the files are small), but if it ever shows up in profiling, batching the reads with Promise.all over chunks would be a straightforward improvement:
// Collect paths first, then read in parallel
const candidates: string[] = [];
for await (const relativeFile of glob(pattern, { cwd: options.appDir })) {
candidates.push(relativeFile);
}
await Promise.all(candidates.map(async (relativeFile) => {
const filePath = path.join(options.appDir, relativeFile);
const source = await readFile(filePath, "utf8");
if (hasModuleUseServerDirective(source)) {
entries.push(toViteEntry(options.root, filePath));
}
}));
|
Review posted. Here's a summary: Verdict: Approved This is a well-structured fix. The implementation correctly addresses the cold server action POST failure in dev by warming up module-level What I verified:
Two minor, non-blocking comments:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9655358d0f
ℹ️ 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 pattern = `**/*.${buildExtensionGlob(extensions)}`; | ||
| const entries: string[] = []; | ||
|
|
||
| for await (const relativeFile of glob(pattern, { cwd: options.appDir })) { |
There was a problem hiding this comment.
The warmup collector only globs under options.appDir, so module-level "use server" files placed elsewhere (for example src/lib/actions.ts and imported into App Router components) are never pre-transformed. In those setups, first action POSTs can still hit the same cold reference path this patch is trying to prevent, because only actions physically inside app/ get added to dev.warmup.
Useful? React with 👍 / 👎.
NathanDrake2406
force-pushed
the
nathan/server-action-dev-warmup
branch
from
9655358 to
3b7addc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7addc0c1
ℹ️ 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".
|
|
||
| for await (const relativeFile of glob(pattern, { | ||
| cwd: options.root, | ||
| exclude: SERVER_ACTION_SCAN_EXCLUDES, |
There was a problem hiding this comment.
collectServerActionWarmupEntries passes a string-array exclude to fs.promises.glob, but this option shape is only supported starting Node 22.14. Because vinext advertises "node": ">=22", users on 22.0–22.13 will hit a runtime error during config evaluation (when hasAppDir is true), preventing dev startup. The repo already documents this compatibility constraint in routing/file-matcher.ts and uses function-form excludes there, so this new call regresses supported Node versions.
Useful? React with 👍 / 👎.
NathanDrake2406
force-pushed
the
nathan/server-action-dev-warmup
branch
from
3b7addc to
a05c187
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Good fix for a real dev-experience pain point. The directive parser is carefully implemented, the build-time skip is in place, the function-form exclude is Node < 22.14 compatible (the Codex P1 is a false positive), and the test suite covers unit, integration, and build-skip paths.
Two issues worth addressing — one correctness concern about scan scope, one minor about the PR description accuracy.
| const entries: string[] = []; | ||
|
|
||
| for await (const relativeFile of glob(pattern, { | ||
| cwd: options.root, |
There was a problem hiding this comment.
The scan runs from options.root (the project root), not from appDir. This means it scans the entire project tree — src/, lib/, scripts/, tools/, etc. — reading every matching source file to check for the directive prologue.
The test at line 121 confirms this is intentional (it expects src/lib/actions.ts to be collected), and it makes sense given the issue's workaround example ('./lib/actions/**/*.ts'). But the PR summary says "discover module-level "use server" files under app/" which is misleading — it actually discovers them under the entire project root.
Two things to consider:
-
Accuracy: Update the PR description to say "under the project root" instead of "under
app/". -
Performance: For monorepo layouts where
rootcontains many non-app directories (e.g.,packages/,docs/,scripts/), this reads every source file in the entire tree. The excluded-roots set covers common build outputs, but not arbitrary sibling directories. In practice this is probably fine (the files are small, the directive check is a fast prefix scan, and it only runs once at dev startup), but it's worth being aware of. If it ever needs scoping, theappDir+ any user-configured action directories would be a tighter default.
Not blocking — the broader scan is arguably more correct since server actions can live anywhere — but the description should match the implementation.
| function shouldExcludeServerActionScanPath(name: string): boolean { | ||
| const segments = name.split(/[\\/]+/).filter(Boolean); | ||
| return ( | ||
| SERVER_ACTION_SCAN_EXCLUDED_ROOTS.has(segments[0] ?? "") || segments.includes("node_modules") |
There was a problem hiding this comment.
The segment-based exclusion works, but note that segments[0] checks the first path component relative to cwd (which is root). This means directories like .git, dist, build, coverage, out are excluded only when they're direct children of root.
A nested build/ or dist/ directory (e.g., packages/foo/dist/actions.ts in a monorepo) would NOT be excluded. The node_modules check covers the most important case since it checks segments.includes("node_modules"), but the other excluded names only match at the root level.
This is probably fine for the initial implementation — most projects have these directories at root level. Just flagging the asymmetry between the node_modules check (any depth) and the other exclusions (root only).
| return null; | ||
| } | ||
|
|
||
| export function hasModuleUseServerDirective(source: string): boolean { |
There was a problem hiding this comment.
Minor: this function has a subtle correctness assumption worth documenting. The directive prologue loop correctly handles:
- BOM prefix
- Whitespace and comments between directives
- Multiple directives before
"use server"(e.g.,"use strict"; "use server") - Semicolons between directives (optional)
- Both single and double quote styles
It correctly rejects:
- Inline
"use server"inside function bodies (theexportkeyword hits the non-quote branch) importstatements before the directive (theiis not a quote character)- Bare semicolons before the directive (the
;is not a quote character, so it returnsfalse— this matches the test at line 56)
The bare-semicolon case is interesting: ; "use server" is rejected because the semicolon at position 0 is not preceded by a directive literal. In the loop, after skipWhitespaceAndComments, we try readDirectiveLiteral which sees ; (not a quote) and returns null, terminating the function with false. This matches ECMAScript directive prologue semantics where a bare semicolon is an empty statement, not a directive — good.
No change needed, just confirming the behavior is correct.
| import fs from "node:fs/promises"; | ||
| import os from "node:os"; | ||
| import path from "node:path"; | ||
| import { createServer, type InlineConfig, type Plugin, type ViteDevServer } from "vite"; |
There was a problem hiding this comment.
Nit: createServer and ViteDevServer are imported from "vite" but other test utilities come from "vite-plus/test". This works because vite resolves to the bundled vite-plus core, but for consistency with the rest of the repo's conventions, is there a reason not to import from "vite-plus" here? Not blocking.
|
Review posted. Here's a summary: Overall: Clean, well-scoped fix. The directive parser correctly implements ECMAScript directive prologue semantics, the build-time skip is in place, the function-form Four comments posted:
|
james-elicx
left a comment
james-elicx
left a comment
There was a problem hiding this comment.
scanning every file on the disk would be somewhat inefficient - we'd probably want to do this by walking through a module graph and only scanning modules depended on by app router page for directives. however, that could still be somewhat taxing in large projects... this probably needs some more thought... maybe some collaboration in the vite plugin as well - there may be something that could be improved there for this
i think this might not work during hmr, as in, new server actions wouldn't be caught as the scan is a one-time thing.
Summary
Fixes #994.
In dev,
@vitejs/plugin-rscvalidates incoming server action references against its in-memoryserverReferenceMetaMap. That map is populated when action modules are transformed, so a cold action POST can fail before the action file has been loaded. This PR has vinext discover module-level"use server"files underapp/and add them to the RSC environmentdev.warmup, preserving any user-specified warmup entries.The behavior is intentionally implemented in a normal config helper instead of expanding generated entries: codegen continues to describe the app shape, while runtime/config behavior lives in importable modules.
Next.js reference behavior
Next.js does not rely on first-request lazy module transforms to recognize server actions. The webpack client module loader identifies server action IDs from RSC module metadata and proxies them in production:
Next also builds action assets and emits the server action manifest from discovered action entries, rather than waiting for the first action request to discover the module:
For directive semantics, the warmup scanner follows Next.js's module directive model: only directive-prologue string literals count, so inline
"use server"actions are not treated as module-level server action files. Reference:Tests
vp test run tests/server-action-warmup.test.tsvp check packages/vinext/src/config/server-action-warmup.ts packages/vinext/src/index.ts tests/server-action-warmup.test.tsvp test run tests/app-server-action-execution.test.tsvp test run tests/app-router.test.ts -t "RSC plugin auto-registration"vp run vinext#buildvp run vinext#buildstill reports the existing virtual-module external warnings forprivate-next-instrumentation-client,virtual:vinext-rsc-entry, andvirtual:vite-rsc/client-references, then exits successfully.