fix: redirect should always return updated router state (#85533)

When a server action triggers a redirect, it will return the RSC payload
for the destination state in a single roundtrip. This lets us update the
UI without needing to waterfall when getting the target page's data. We
reject the server action with a special error signaling that the form
state should be reset, but otherwise the redirect itself is unnecessary,
since we already are handling it in the server action reducer.

However, when we introduced Activity for bfCache, this meant navigating
back to the previous page would replay the redirect error that was
stashed in the `RedirectBoundary`. I incorrectly patched this by just
returning the current router state, rather than the redirect
destination's state, but this lost the ability to return the updated
state in the same roundtrip as the redirect. Instead, this marks the
error that gets stashed in the `RedirectBoundary` as being "handled".
Re-activating the hidden subtree will now still correctly reset the
state (because the boundary handles it), but won't attempt to perform
the redirect again.

Closes NAR-470

Author: ztanner

packages/next/src/client/components/redirect-boundary.tsx

@@ -48,6 +48,13 @@ export class RedirectErrorBoundary extends React.Component<
     if (isRedirectError(error)) {
       const url = getURLFromRedirectError(error)
       const redirectType = getRedirectTypeFromError(error)
+      if ('handled' in error) {
+        // The redirect was already handled. We'll still catch the redirect error
+        // so that we can remount the subtree, but we don't actually need to trigger the
+        // router.push.
+        return { redirect: null, redirectType: null }
+      }
+
       return { redirect: url, redirectType }
     }
     // Re-throw if error is not for redirect

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

@@ -415,19 +415,19 @@ export function serverActionReducer(
         // 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(redirectHref)
-              ? removeBasePath(redirectHref)
-              : redirectHref,
-            redirectType || RedirectType.push
-          )
+        const redirectError = getRedirectError(
+          hasBasePath(redirectHref)
+            ? removeBasePath(redirectHref)
+            : redirectHref,
+          redirectType || RedirectType.push
         )
-
-        // TODO: Investigate why this is needed with Activity.
-        if (process.env.__NEXT_CACHE_COMPONENTS) {
-          return state
-        }
+        // We mark the error as handled because we don't want the redirect to be tried later by
+        // the RedirectBoundary, in case the user goes back and `Activity` triggers the redirect
+        // again, as it's run within an effect.
+        // We don't actually need the RedirectBoundary to do a router.push because we already
+        // have all the necessary RSC data to render the new page within a single roundtrip.
+        ;(redirectError as any).handled = true
+        reject(redirectError)
       } else {
         resolve(actionResult)
       }

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

@@ -0,0 +1,31 @@
+'use server'
+
+import { cookies } from 'next/headers'
+import { refresh } from 'next/cache'
+import { redirect } from 'next/navigation'
+
+export async function addTodoEntry(entry: string) {
+  // simulate some latency
+  await new Promise((resolve) => setTimeout(resolve, 500))
+
+  const cookieStore = await cookies()
+  const existing = cookieStore.get('todo-entries')?.value || '[]'
+  const entries = JSON.parse(existing)
+  entries.push(entry)
+  cookieStore.set('todo-entries', JSON.stringify(entries))
+}
+
+export async function getTodoEntries() {
+  const cookieStore = await cookies()
+  const existing = cookieStore.get('todo-entries')?.value || '[]'
+  return JSON.parse(existing) as string[]
+}
+
+export async function addEntryAndRefresh(formData: FormData) {
+  const entry = formData.get('entry') as string
+
+  await addTodoEntry(entry)
+
+  refresh()
+  redirect('/redirect-and-refresh/foo')
+}

test/e2e/app-dir/refresh/app/redirect-and-refresh/foo/page.tsx

@@ -0,0 +1,3 @@
+export default function Foo() {
+  return <div id="foo-page">Hello from Foo</div>
+}

test/e2e/app-dir/refresh/app/redirect-and-refresh/layout.tsx

@@ -0,0 +1,19 @@
+import { getTodoEntries } from './actions'
+
+export default async function Layout({ children }) {
+  const entries = await getTodoEntries()
+
+  console.log({ entries })
+  return (
+    <div>
+      <div id="todo-entries">
+        {entries.length > 0 ? (
+          entries.map((phrase, index) => <div key={index}>{phrase}</div>)
+        ) : (
+          <div id="no-entries">No entries</div>
+        )}
+      </div>
+      <div>{children}</div>
+    </div>
+  )
+}

test/e2e/app-dir/refresh/app/redirect-and-refresh/page.tsx

@@ -0,0 +1,20 @@
+import { addEntryAndRefresh } from './actions'
+
+export default function Page() {
+  return (
+    <>
+      <form action={addEntryAndRefresh}>
+        <label htmlFor="todo-input">Entry</label>
+        <input
+          id="todo-input"
+          type="text"
+          name="entry"
+          placeholder="Enter a new entry"
+        />
+        <button id="add-button" type="submit">
+          Add New Todo Entry
+        </button>
+      </form>
+    </>
+  )
+}

test/e2e/app-dir/refresh/next.config.js

@@ -0,0 +1,3 @@
+module.exports = {
+  cacheComponents: true,
+}

test/e2e/app-dir/refresh/refresh.test.ts

@@ -80,4 +80,24 @@ describe('app-dir refresh', () => {
       })
     }
   })
+
+  it('should let you read your write after a redirect and refresh', async () => {
+    const browser = await next.browser('/redirect-and-refresh')
+
+    const todoEntries = await browser.elementById('todo-entries').text()
+    expect(todoEntries).toBe('No entries')
+
+    const todoInput = await browser.elementById('todo-input')
+    await todoInput.fill('foo')
+
+    await browser.elementById('add-button').click()
+
+    await retry(async () => {
+      const newTodoEntries = await browser.elementById('todo-entries').text()
+      expect(newTodoEntries).toContain('foo')
+    })
+
+    expect(await browser.hasElementByCssSelector('#foo-page')).toBe(true)
+    expect(await browser.url()).toContain('/redirect-and-refresh/foo')
+  })
 })