fix: server action redirects between multiple root layouts (#73063)

When handling a redirect through a server action, we pass in the current href rather than the destination href to `handleExternalUrl`, which is incorrect since that won't reflect the updated URL from a server action's redirect response. While looking into this, I noticed a handful of server action branches that weren't properly rejecting or resolving the action promises. This would cause the transitions to stall until they were fulfilled, so I updated all of the existing branches where we early return to also resolve the promise. Fixes #72842
vercel · Nov 21, 2024 · b42f73a · b42f73a
1 parent eb86d82
commit b42f73a
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 14 deletions.
diff --git a/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/server-action-reducer.ts
@@ -206,7 +206,6 @@ export function serverActionReducer(
 ): ReducerState {
   const { resolve, reject } = action
   const mutable: ServerActionMutable = {}
-  const href = state.canonicalUrl
 
   let currentTree = state.tree
 
@@ -230,6 +229,8 @@ export function serverActionReducer(
       isPrerender,
       revalidatedParts,
     }) => {
+      let redirectHref: string | undefined
+
       // honor the redirect type instead of defaulting to push in case of server actions.
       if (redirectLocation) {
         if (redirectType === RedirectType.replace) {
@@ -239,6 +240,9 @@ export function serverActionReducer(
           state.pushRef.pendingPush = true
           mutable.pendingPush = true
         }
+
+        redirectHref = createHrefFromUrl(redirectLocation, false)
+        mutable.canonicalUrl = redirectHref
       }
 
       if (!flightData) {
@@ -258,6 +262,8 @@ export function serverActionReducer(
 
       if (typeof flightData === 'string') {
         // Handle case when navigating to page in `pages` from `app`
+        resolve(actionResult)
+
         return handleExternalUrl(
           state,
           mutable,
@@ -282,6 +288,8 @@ export function serverActionReducer(
         if (!isRootRender) {
           // TODO-APP: handle this case better
           console.log('SERVER ACTION APPLY FAILED')
+          resolve(actionResult)
+
           return state
         }
 
@@ -291,20 +299,22 @@ export function serverActionReducer(
           [''],
           currentTree,
           treePatch,
-          redirectLocation
-            ? createHrefFromUrl(redirectLocation)
-            : state.canonicalUrl
+          redirectHref ? redirectHref : state.canonicalUrl
         )
 
         if (newTree === null) {
+          resolve(actionResult)
+
           return handleSegmentMismatch(state, action, treePatch)
         }
 
         if (isNavigatingToNewRootLayout(currentTree, newTree)) {
+          resolve(actionResult)
+
           return handleExternalUrl(
             state,
             mutable,
-            href,
+            redirectHref || state.canonicalUrl,
             state.pushRef.pendingPush
           )
         }
@@ -343,10 +353,7 @@ export function serverActionReducer(
         currentTree = newTree
       }
 
-      if (redirectLocation) {
-        const newHref = createHrefFromUrl(redirectLocation, false)
-        mutable.canonicalUrl = newHref
-
+      if (redirectLocation && redirectHref) {
         // 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.
@@ -381,7 +388,9 @@ export function serverActionReducer(
         // a response with the correct status code.
         reject(
           getRedirectError(
-            hasBasePath(newHref) ? removeBasePath(newHref) : newHref,
+            hasBasePath(redirectHref)
+              ? removeBasePath(redirectHref)
+              : redirectHref,
             redirectType || RedirectType.push
           )
         )

diff --git a/test/e2e/app-dir/root-layout-redirect/app/actions.ts b/test/e2e/app-dir/root-layout-redirect/app/actions.ts
@@ -0,0 +1,7 @@
+'use server'
+
+import { redirect } from 'next/navigation'
+
+export async function redirectAction() {
+  redirect('/result')
+}
diff --git a/test/e2e/app-dir/root-layout/app/(multiple-root)/(root-a)/root-layout-a/page.js b/test/e2e/app-dir/root-layout/app/(multiple-root)/(root-a)/root-layout-a/page.js
@@ -1,10 +1,21 @@
 import React from 'react'
 import Link from 'next/link'
+import { redirect } from 'next/navigation'
+
+export default async function Page() {
+  async function redirectAction() {
+    'use server'
+    redirect('/root-layout-b')
+  }
 
-export default function Page() {
   return (
-    <Link href="../root-layout-b" id="link-to-b">
-      To b
-    </Link>
+    <>
+      <Link href="../root-layout-b" id="link-to-b">
+        To b
+      </Link>
+      <form action={redirectAction}>
+        <button id="action-redirect-to-b">Redirect to B</button>
+      </form>
+    </>
   )
 }
diff --git a/test/e2e/app-dir/root-layout/root-layout.test.ts b/test/e2e/app-dir/root-layout/root-layout.test.ts
@@ -224,4 +224,16 @@ describe('app-dir root layout', () => {
       .waitForElementByCss('#root-b')
     expect(await browser.hasElementByCssSelector('#root-a')).toBeFalse()
   })
+
+  it('should correctly handle navigation between multiple root layouts when redirecting in a server action', async () => {
+    const browser = await next.browser('/root-layout-a')
+
+    await browser.waitForElementByCss('#action-redirect-to-b')
+    expect(await browser.hasElementByCssSelector('#root-b')).toBeFalse()
+    await browser
+      .elementById('action-redirect-to-b')
+      .click()
+      .waitForElementByCss('#root-b')
+    expect(await browser.hasElementByCssSelector('#root-a')).toBeFalse()
+  })
 })