bugfix: actions that redirect should reject the action promise (verce…

…l#69945)

When calling `redirect` in a server action, the action handler will
invoke a worker to fetch the RSC payload from the URL being redirected
to, and will return it in the action request. This allows the redirect
to be handled in a single request rather than requiring a second request
to fetch the RSC payload on the URL being redirected to. As part of this
special redirection handling, no action result is returned, so the
action promise is resolved with `undefined`.

As a result of this behavior, if your server action redirects to a page
that has the same form that was just submitted, and you're leveraging
`useActionState`, the updated state value will be undefined rather than
being reset to the initial state. **More generally, redirect errors are
handled by the client currently by simply resolving the promise with
undefined, which isn't the correct behavior.**

This PR will reject the server action promise with a redirect error, so
that it'll be caught by our `RedirectBoundary`. Since React will remount
the tree as part of the error boundary recovery, this effectively resets
the form to it's initial state.

If the action is rejected in a global `startTransition`, then it'll be
handled by a global `error` handler, which will perform the redirect.
And if it occurs outside of a transition, the redirect will be handled
by a `unhandledrejection` handler.

Because we're not able to apply the flight data as-is in the server
action reducer (and instead defer to the navigation action), we seed the
prefetch cache for the target page with the flight data from the action
result so that we do not need to make another server roundtrip. We apply
the same static/dynamic cache staleTime heuristics for these cache
entries.

Fixes vercel#68549
Closes NDX-229

packages/next/src/client/components/app-router.tsx

@@ -58,6 +58,12 @@ import type { FlightRouterState } from '../../server/app-render/types'
 import { useNavFailureHandler } from './nav-failure-handler'
 import { useServerActionDispatcher } from '../app-call-server'
 import type { AppRouterActionQueue } from '../../shared/lib/router/action-queue'
+import {
+  getRedirectTypeFromError,
+  getURLFromRedirectError,
+  isRedirectError,
+  RedirectType,
+} from './redirect'
 
 const globalMutable: {
   pendingMpaPath?: string
@@ -369,6 +375,33 @@ function Router({
     }
   }, [dispatch])
 
+  useEffect(() => {
+    // Ensure that any redirect errors that bubble up outside of the RedirectBoundary
+    // are caught and handled by the router.
+    function handleUnhandledRedirect(
+      event: ErrorEvent | PromiseRejectionEvent
+    ) {
+      const error = 'reason' in event ? event.reason : event.error
+      if (isRedirectError(error)) {
+        event.preventDefault()
+        const url = getURLFromRedirectError(error)
+        const redirectType = getRedirectTypeFromError(error)
+        if (redirectType === RedirectType.push) {
+          appRouter.push(url, {})
+        } else {
+          appRouter.replace(url, {})
+        }
+      }
+    }
+    window.addEventListener('error', handleUnhandledRedirect)
+    window.addEventListener('unhandledrejection', handleUnhandledRedirect)
+
+    return () => {
+      window.removeEventListener('error', handleUnhandledRedirect)
+      window.removeEventListener('unhandledrejection', handleUnhandledRedirect)
+    }
+  }, [appRouter])
+
   // When mpaNavigation flag is set do a hard navigation to the new url.
   // Infinitely suspend because we don't actually want to rerender any child
   // components with the new URL and any entangled state updates shouldn't

packages/next/src/client/components/react-dev-overlay/internal/helpers/use-error-handler.ts

@@ -58,6 +58,11 @@ if (typeof window !== 'undefined') {
     'unhandledrejection',
     (ev: WindowEventMap['unhandledrejection']): void => {
       const reason = ev?.reason
+      if (isNextRouterError(reason)) {
+        ev.preventDefault()
+        return
+      }
+
       if (
         !reason ||
         !(reason instanceof Error) ||

packages/next/src/client/components/router-reducer/create-initial-router-state.ts

@@ -4,8 +4,8 @@ import type { FlightDataPath } from '../../../server/app-render/types'
 import { createHrefFromUrl } from './create-href-from-url'
 import { fillLazyItemsTillLeafWithHead } from './fill-lazy-items-till-leaf-with-head'
 import { extractPathFromFlightRouterState } from './compute-changed-path'
-import { createPrefetchCacheEntryForInitialLoad } from './prefetch-cache-utils'
-import type { PrefetchCacheEntry } from './router-reducer-types'
+import { createSeededPrefetchCacheEntry } from './prefetch-cache-utils'
+import { PrefetchKind, type PrefetchCacheEntry } from './router-reducer-types'
 import { addRefreshMarkerToActiveParallelSegments } from './refetch-inactive-parallel-segments'
 import { getFlightDataPartsFromPath } from '../../flight-data-helpers'
 
@@ -112,7 +112,7 @@ export function createInitialRouterState({
       location.origin
     )
 
-    createPrefetchCacheEntryForInitialLoad({
+    createSeededPrefetchCacheEntry({
       url,
       data: {
         flightData: [normalizedFlightData],
@@ -125,6 +125,7 @@ export function createInitialRouterState({
       tree: initialState.tree,
       prefetchCache: initialState.prefetchCache,
       nextUrl: initialState.nextUrl,
+      kind: PrefetchKind.AUTO,
     })
   }
 

packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts

@@ -249,19 +249,20 @@ function prefixExistingPrefetchCacheEntry({
 /**
  * Use to seed the prefetch cache with data that has already been fetched.
  */
-export function createPrefetchCacheEntryForInitialLoad({
+export function createSeededPrefetchCacheEntry({
   nextUrl,
   tree,
   prefetchCache,
   url,
   data,
+  kind,
 }: Pick<ReadonlyReducerState, 'nextUrl' | 'tree' | 'prefetchCache'> & {
   url: URL
   data: FetchServerResponseResult
+  kind: PrefetchKind
 }) {
   // The initial cache entry technically includes full data, but it isn't explicitly prefetched -- we just seed the
   // prefetch cache so that we can skip an extra prefetch request later, since we already have the data.
-  const kind = PrefetchKind.AUTO
   // if the prefetch corresponds with an interception route, we use the nextUrl to prefix the cache key
   const prefetchCacheKey = data.couldBeIntercepted
     ? createPrefetchCacheKey(url, kind, nextUrl)

packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts

@@ -5,6 +5,7 @@ import type {
 import { callServer } from '../../../app-call-server'
 import {
   ACTION_HEADER,
+  NEXT_IS_PRERENDER_HEADER,
   NEXT_ROUTER_STATE_TREE_HEADER,
   NEXT_URL,
   RSC_CONTENT_TYPE_HEADER,
@@ -21,11 +22,12 @@ const { createFromFetch, encodeReply } = (
       require('react-server-dom-webpack/client')
 ) as typeof import('react-server-dom-webpack/client')
 
-import type {
-  ReadonlyReducerState,
-  ReducerState,
-  ServerActionAction,
-  ServerActionMutable,
+import {
+  PrefetchKind,
+  type ReadonlyReducerState,
+  type ReducerState,
+  type ServerActionAction,
+  type ServerActionMutable,
 } from '../router-reducer-types'
 import { addBasePath } from '../../../add-base-path'
 import { createHrefFromUrl } from '../create-href-from-url'
@@ -43,11 +45,16 @@ import {
   normalizeFlightData,
   type NormalizedFlightData,
 } from '../../../flight-data-helpers'
+import { getRedirectError, RedirectType } from '../../redirect'
+import { createSeededPrefetchCacheEntry } from '../prefetch-cache-utils'
+import { removeBasePath } from '../../../remove-base-path'
+import { hasBasePath } from '../../../has-base-path'
 
 type FetchServerActionResult = {
   redirectLocation: URL | undefined
   actionResult?: ActionResult
   actionFlightData?: NormalizedFlightData[] | string
+  isPrerender: boolean
   revalidatedParts: {
     tag: boolean
     cookie: boolean
@@ -85,6 +92,7 @@ async function fetchServerAction(
   })
 
   const location = res.headers.get('x-action-redirect')
+  const isPrerender = !!res.headers.get(NEXT_IS_PRERENDER_HEADER)
   let revalidatedParts: FetchServerActionResult['revalidatedParts']
   try {
     const revalidatedHeader = JSON.parse(
@@ -127,6 +135,7 @@ async function fetchServerAction(
         actionFlightData: normalizeFlightData(response.f),
         redirectLocation,
         revalidatedParts,
+        isPrerender,
       }
     }
 
@@ -135,6 +144,7 @@ async function fetchServerAction(
       actionFlightData: normalizeFlightData(response.f),
       redirectLocation,
       revalidatedParts,
+      isPrerender,
     }
   }
 
@@ -153,6 +163,7 @@ async function fetchServerAction(
   return {
     redirectLocation,
     revalidatedParts,
+    isPrerender,
   }
 }
 
@@ -186,6 +197,7 @@ export function serverActionReducer(
       actionResult,
       actionFlightData: flightData,
       redirectLocation,
+      isPrerender,
     }) => {
       // Make sure the redirection is a push instead of a replace.
       // Issue: https://github.com/vercel/next.js/issues/53911
@@ -221,7 +233,41 @@ export function serverActionReducer(
 
       if (redirectLocation) {
         const newHref = createHrefFromUrl(redirectLocation, false)
-        mutable.canonicalUrl = newHref
+
+        // Because the RedirectBoundary will trigger a navigation, we need to seed the prefetch cache
+        // with the FlightData that we got from the server action for the target page, so that it's
+        // available when the page is navigated to and doesn't need to be re-fetched.
+        createSeededPrefetchCacheEntry({
+          url: redirectLocation,
+          data: {
+            flightData,
+            canonicalUrl: undefined,
+            couldBeIntercepted: false,
+            isPrerender: false,
+            postponed: false,
+          },
+          tree: state.tree,
+          prefetchCache: state.prefetchCache,
+          nextUrl: state.nextUrl,
+          kind: isPrerender ? PrefetchKind.FULL : PrefetchKind.AUTO,
+        })
+
+        mutable.prefetchCache = state.prefetchCache
+        // If the action triggered a redirect, we reject the action promise with a redirect
+        // so that it's handled by RedirectBoundary as we won't have a valid
+        // action result to resolve the promise with. This will effectively reset the state of
+        // the component that called the action as the error boundary will remount the tree.
+        // The status code doesn't matter here as the action handler will have already sent
+        // a response with the correct status code.
+
+        reject(
+          getRedirectError(
+            hasBasePath(newHref) ? removeBasePath(newHref) : newHref,
+            RedirectType.push
+          )
+        )
+
+        return handleMutable(state, mutable)
       }
 
       for (const normalizedFlightData of flightData) {

packages/next/src/server/base-server.ts

@@ -3068,25 +3068,26 @@ export default abstract class Server<
 
     if (
       isSSG &&
-      !this.minimalMode &&
       // We don't want to send a cache header for requests that contain dynamic
       // data. If this is a Dynamic RSC request or wasn't a Prefetch RSC
       // request, then we should set the cache header.
       !isDynamicRSCRequest &&
       (!didPostpone || isPrefetchRSCRequest)
     ) {
-      // set x-nextjs-cache header to match the header
-      // we set for the image-optimizer
-      res.setHeader(
-        'x-nextjs-cache',
-        isOnDemandRevalidate
-          ? 'REVALIDATED'
-          : cacheEntry.isMiss
-            ? 'MISS'
-            : cacheEntry.isStale
-              ? 'STALE'
-              : 'HIT'
-      )
+      if (!this.minimalMode) {
+        // set x-nextjs-cache header to match the header
+        // we set for the image-optimizer
+        res.setHeader(
+          'x-nextjs-cache',
+          isOnDemandRevalidate
+            ? 'REVALIDATED'
+            : cacheEntry.isMiss
+              ? 'MISS'
+              : cacheEntry.isStale
+                ? 'STALE'
+                : 'HIT'
+        )
+      }
       // Set a header used by the client router to signal the response is static
       // and should respect the `static` cache staleTime value.
       res.setHeader(NEXT_IS_PRERENDER_HEADER, '1')

test/e2e/app-dir/actions/app-action.test.ts

@@ -564,6 +564,64 @@ describe('app-dir action handling', () => {
     await check(() => browser.elementByCss('h1').text(), 'Transition is: idle')
   })
 
+  it('should reset the form state when the action redirects to a page that contains the same form', async () => {
+    const browser = await next.browser('/redirect')
+    const input = await browser.elementByCss('input[name="name"]')
+    const submit = await browser.elementByCss('button')
+
+    expect(await browser.hasElementByCssSelector('#error')).toBe(false)
+
+    await input.fill('foo')
+    await submit.click()
+
+    // The server action will fail validation and will return error state
+    // verify that the error state is displayed
+    await retry(async () => {
+      expect(await browser.hasElementByCssSelector('#error')).toBe(true)
+      expect(await browser.elementByCss('#error').text()).toBe(
+        "Only 'justputit' is accepted."
+      )
+    })
+
+    // The server action won't return an error state, it will just call redirect to itself
+    // Validate that the form state is reset
+    await input.fill('justputit')
+    await submit.click()
+
+    await retry(async () => {
+      expect(await browser.hasElementByCssSelector('#error')).toBe(false)
+    })
+  })
+
+  it('should reset the form state when the action redirects to itself', async () => {
+    const browser = await next.browser('/self-redirect')
+    const input = await browser.elementByCss('input[name="name"]')
+    const submit = await browser.elementByCss('button')
+
+    expect(await browser.hasElementByCssSelector('#error')).toBe(false)
+
+    await input.fill('foo')
+    await submit.click()
+
+    // The server action will fail validation and will return error state
+    // verify that the error state is displayed
+    await retry(async () => {
+      expect(await browser.hasElementByCssSelector('#error')).toBe(true)
+      expect(await browser.elementByCss('#error').text()).toBe(
+        "Only 'justputit' is accepted."
+      )
+    })
+
+    // The server action won't return an error state, it will just call redirect to itself
+    // Validate that the form state is reset
+    await input.fill('justputit')
+    await submit.click()
+
+    await retry(async () => {
+      expect(await browser.hasElementByCssSelector('#error')).toBe(false)
+    })
+  })
+
   // This is disabled when deployed because the 404 page will be served as a static route
   // which will not support POST requests, and will return a 405 instead.
   if (!isNextDeploy) {
@@ -899,9 +957,13 @@ describe('app-dir action handling', () => {
           'redirected'
         )
 
-        // no other requests should be made
-        expect(requests).toHaveLength(1)
-        expect(responses).toHaveLength(1)
+        // This verifies the redirect & server response happens in a single roundtrip,
+        // if the redirect resource was static. In development, these responses are always
+        // dynamically generated, so we only expect a single request for build/deploy.
+        if (!isNextDev) {
+          expect(requests).toHaveLength(1)
+          expect(responses).toHaveLength(1)
+        }
 
         const request = requests[0]
         const response = responses[0]
@@ -998,9 +1060,13 @@ describe('app-dir action handling', () => {
         await browser.elementById(`redirect-${redirectType}`).click()
         await check(() => browser.url(), `${next.url}${destinationPagePath}`)
 
-        // no other requests should be made
-        expect(requests).toHaveLength(1)
-        expect(responses).toHaveLength(1)
+        // This verifies the redirect & server response happens in a single roundtrip,
+        // if the redirect resource was static. In development, these responses are always
+        // dynamically generated, so we only expect a single request for build/deploy.
+        if (!isNextDev) {
+          expect(requests).toHaveLength(1)
+          expect(responses).toHaveLength(1)
+        }
 
         const request = requests[0]
         const response = responses[0]

test/e2e/app-dir/actions/app/redirect/actions.ts

@@ -0,0 +1,17 @@
+'use server'
+
+import { redirect } from 'next/navigation'
+
+type State = {
+  errors: Record<string, string>
+}
+
+export async function action(previousState: State, formData: FormData) {
+  const name = formData.get('name')
+
+  if (name !== 'justputit') {
+    return { errors: { name: "Only 'justputit' is accepted." } }
+  }
+
+  redirect('/redirect/other')
+}