fix(app-router): handle Vite ref errors that elide #exportName (#1340) - #1668
Conversation
#1340) When a server action id resolved from request headers carries the `#<exportName>` suffix (e.g. `/app/foo.ts#bar`), `loadServerAction()` splits the id at `#` and calls `requireModule(file)` with the module path only. `@vitejs/plugin-rsc`'s reference-validation virtual module therefore emits `[vite-rsc] invalid server reference '/app/foo.ts'`, omitting the `#<exportName>` portion. The previous match in `isServerActionNotFoundError` compared the full actionId against the Vite error string, so missing/stale references surfaced as a generic 500 instead of the Next.js-compatible 404 with the `x-nextjs-action-not-found` header. Also accept the pre-`#` module-path form for both the dev-time `[vite-rsc] invalid server reference '<id>'` and prod-time `server reference not found '<id>'` error shapes. Adds a focused unit test reproducing the dev-mode case and integration tests asserting that throwing `notFound()` from a server action returns 404 (both fetch and progressive form paths), porting fixture from Next.js: test/e2e/app-dir/actions/app/server/form.js.
|
/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, well-scoped fix. The root cause analysis is solid — loadServerAction(id) splits at # before requireModule, so the Vite reference-validator only sees the module path, not the full module#export action id. The existing isServerActionNotFoundError was comparing against the full action id and never matched, causing the error to fall through to the generic 500 path instead of the 404 + x-nextjs-action-not-found response.
Implementation review:
The moduleId extraction via actionId.split("#")[0] with the moduleId !== actionId guard is the right approach — when the action id has no # (e.g. "stale-action-id"), moduleId === actionId so the second branch is skipped, preserving the existing behavior for those cases. The duplication across the dev/prod patterns is a reasonable trade-off for readability given there are only two patterns.
Test coverage:
- Unit test directly exercises the elided-
#exportNameerror shape — good regression anchor. - The revalidating-fetch-action 404 status test (
"preserves the 404 status when notFound() is thrown by a revalidating fetch action") covers theactionStatuspropagation through the page-rerender branch (line 818 ofapp-server-action-execution.ts), which was previously only tested for the skip-rerender shortcut. This is a separate but related concern and the test is well-structured. - Integration tests for both fetch-action and progressive-form-action paths against the live dev server are thorough. The pre-visit to
/actionsto ensure the"use server"module is registered is a necessary detail.
One minor nit (non-blocking): the notFoundAction fixture function is also used by the integration tests, which is correct. Just noting it's clean that both the unit and integration tests exercise different layers of the same fix.
Looks good to merge once CI is green.
|
Approved PR #1668. The fix correctly handles the mismatch between the full action id ( |
commit: |
…#1340) (#1853) * fix(actions): return 404 for MPA action on app with no server actions (#1340) A multipart form POST to an App Router page is always a server-action attempt. When the form body decodes to no action at all (e.g. the build has no server actions, so decodeAction returns null rather than throwing), vinext fell through to a 200 page render instead of Next.js' 404 + x-nextjs-action-not-found. The fetch-action variant (via the Next-Action header) already worked; only the MPA/no-JS form-POST variant was wrong. Gate the new 404 on the posted-to route being a page: route handlers run after action dispatch and legitimately accept raw multipart POSTs, so they still fall through. The page-vs-route distinction uses the static __loadPage / __loadRouteHandler markers, available before lazy module hydration. Finishes the no-server-actions MPA case of #1340 (error-boundary, content-type, and unrecognized-action handling landed in #1386/#1668). * test(actions): add MPA no-action integration test; skip redundant route match (#1340) Address ask-bonk review on #1853: - Add a dev-server integration test that POSTs multipart/form-data to a page (/about) and asserts 404 + x-nextjs-action-not-found, locking in the entry-side matchRoute + __loadPage/__loadRouteHandler classification end-to-end (previously only unit-tested by passing hasPageRoute directly). - Gate the progressive-action route match on isProgressiveServerActionRequest so matchRoute no longer runs on every App Router request — only for actual multipart/no-actionId POST candidates.
1 participant
Summary
[vite-rsc] invalid server reference '<moduleId>'without the#<exportName>suffix, becauseloadServerAction(id)splits the id at#before callingrequireModule(file).isServerActionNotFoundErroronly compared the full action id against the Vite error, so stale/missing references surfaced as a generic 500 instead of Next.js' 404 +x-nextjs-action-not-foundresponse.#module-path forms for the dev ([vite-rsc] invalid server reference '<id>') and prod (server reference not found '<id>') error shapes.Scoped fix for issue #1340 — the
notFound()-from-server-action 404 status path. Other parts of #1340 (error-boundary propagation fortext/plainaction responses, unrecognized-action client behaviour) remain in scope for follow-up PRs.Test plan
pnpm test tests/app-server-action-execution.test.ts— new unit test fails onmain, passes after fixpnpm test tests/nextjs-compat/not-found.test.ts— integration coverage thatnotFound()from a server action returns 404 for both fetch and progressive form action pathspnpm run check(format / lint / type) clean