fix(server): render progressive action not-found HTML#1254
Conversation
Progressive multipart server actions returned an empty status response when an action called notFound() or another HTTP fallback. That diverged from Next.js' MPA action path, where the action error re-enters app rendering so the not-found boundary produces full HTML. The root cause was treating HTTP fallback action throws as terminal responses in the action execution helper. Pass action failures through the progressive form-state path, preserve redirects as 303 responses, and rethrow action failures during page element construction so existing special-error rendering handles the response body. Regression coverage asserts the action helper handoff and the rendered not-found HTML body.
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Solid work. The design is clean: progressive action failures are lifted from terminal responses into render inputs, so buildAppPageElement's existing special-error/error-boundary machinery handles them. The explicit actionFailed flag correctly handles falsy thrown values (the throw 0 test covers this), the type narrowing in app-rsc-handler.ts via "actionFailed" in progressiveActionResult is correct, and the test coverage targets the right contract boundaries.
Two minor observations below, neither blocking.
| actionFailed: true; | ||
| formState: null; | ||
| kind: "form-state"; | ||
| }; |
There was a problem hiding this comment.
Nit: this type is structurally identical to ProgressiveServerActionResult in app-server-action-execution.ts. They'll drift independently. Consider exporting the type from the execution module and importing it here, or extracting a shared type into a common location.
Not blocking — just a maintenance observation.
| actionError = error; | ||
| actionFailed = true; | ||
| const isControlFlow = | ||
| getActionHttpFallbackStatus(error) !== null || isServerActionNotFoundError(error, null); | ||
| if (!isControlFlow) { | ||
| console.error("[vinext] Server action error:", error); | ||
| options.reportRequestError( | ||
| normalizeError(error), | ||
| { | ||
| path: options.cleanPathname, | ||
| method: options.request.method, | ||
| headers: Object.fromEntries(options.request.headers.entries()), | ||
| }, | ||
| { routerKind: "App Router", routePath: options.cleanPathname, routeType: "action" }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The error reporting for non-control-flow errors is good. Worth noting: this means generic action errors are now reported twice — once here via console.error + reportRequestError, and potentially again downstream when dispatchAppPage re-throws the error through buildAppPageElement and the error boundary / render lifecycle catches it. The createRscOnErrorHandler in the render path would see it again.
This matches the spirit of the change (action execution owns reporting, page render owns rendering), but wanted to flag the double-report possibility in case it matters for observability. The existing Next.js behavior presumably has a similar characteristic since the action handler also logs before handing off.
|
Review submitted with approval. Here's a summary: Approved. The PR correctly fixes progressive server action Key things I verified:
Two minor non-blocking observations left as inline comments: duplicate type definitions between the handler and execution module, and potential double-reporting of generic action errors. |
| if (options.actionFailed) { | ||
| throw options.actionError; | ||
| } |
There was a problem hiding this comment.
wouldn't this be the same as if options.actionError? I don't think we'd have a failure without an associated error?
| ["NEXT_HTTP_ERROR_FALLBACK;403", 403], | ||
| ]) { | ||
| it("passes HTTP fallback errors as actionError to be rendered by error boundaries", async () => { | ||
| for (const digest of ["NEXT_NOT_FOUND", "NEXT_HTTP_ERROR_FALLBACK;403"]) { |
There was a problem hiding this comment.
could use it.each for this
Overview
notFound()or another HTTP fallback.handleProgressiveServerActionRequestnow hands action failures tocreateAppRscHandler, which passes them intodispatchAppPagefor existing special-error rendering.app-server-action-execution.ts,app-rsc-handler.ts,app-page-dispatch.tsuseActionStateform submissions that callnotFound()render the app's not-found HTML instead of a blank 404 body.Why
Progressive form actions are MPA-style submissions. When the action throws an HTTP fallback, the server response must be the page render for that fallback, not only the fallback status code. Next.js models this explicitly: fetch actions package the error into Flight, while MPA actions return a
not-foundaction result andapp-renderrenders the not-found loader tree to HTML.303withLocationand action headers.actionFailedflag instead of truthiness.What Changed
notFound()404response with no body.404response with rendered not-found HTML.303response.303response.Maintainer review path
packages/vinext/src/server/app-server-action-execution.ts: action execution now distinguishes redirects from action failures and returns an action-failed form-state result for non-redirect throws.packages/vinext/src/server/app-rsc-handler.ts: progressive action results now carryactionErrorplus explicitactionFailedinto matched page dispatch.packages/vinext/src/server/app-page-dispatch.ts: action failures are rethrown during element construction so existing HTTP fallback and error boundary machinery owns the response.tests/app-server-action-execution.test.tsandtests/app-page-dispatch.test.ts: helper contract plus rendered HTML regression coverage.Validation
vp test run tests/app-server-action-execution.test.ts tests/app-rsc-handler.test.ts tests/app-page-dispatch.test.tsvp check tests/app-server-action-execution.test.ts tests/app-rsc-handler.test.ts tests/app-page-dispatch.test.tsvp checkRisk / compatibility
Non-goals
useActionStatefallback variant. The added coverage targets the lowest honest server boundaries for this regression.References
HandleActionResultincludes MPAnot-founduseActionStateaction callingnotFound()fixture