fix: redirect should always return updated router state

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')
+  })
 })