fix bugs pertaining to server actions + navigation (#55853)

This fixes some scenarios where executing a server action after navigation can cause the action to behave incorrectly (double submitting, not resolving). There are two separate issues: - `canonicalUrl` and `pendingNavigatePath` were not constructed using the same function (`createHrefFromUrl`) so in certain situations they'd be comparing different values - a fulfilled inFlightServerAction should not be invoked again Closes NEXT-1655 Closes NEXT-1654 Fixes #55845 Fixes #55814 Fixes #55805
vercel · Sep 23, 2023 · c2e0213 · c2e0213
1 parent d5db4d3
commit c2e0213
Show file tree

Hide file tree

Showing 17 changed files with 193 additions and 8 deletions.
diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx
@@ -186,7 +186,7 @@ function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
   return useCallback(
     (href, navigateType, forceOptimisticNavigation, shouldScroll) => {
       const url = new URL(addBasePath(href), location.href)
-      globalMutable.pendingNavigatePath = href
+      globalMutable.pendingNavigatePath = createHrefFromUrl(url)
 
       return dispatch({
         type: ACTION_NAVIGATE,

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
@@ -163,6 +163,7 @@ export function serverActionReducer(
     // unblock if a navigation event comes through
     // while we've suspended on an action
     if (
+      mutable.inFlightServerAction.status !== 'fulfilled' &&
       mutable.globalMutable.pendingNavigatePath &&
       mutable.globalMutable.pendingNavigatePath !== href
     ) {

diff --git a/test/e2e/app-dir/actions-navigation/app/action-after-redirect/actions.js b/test/e2e/app-dir/actions-navigation/app/action-after-redirect/actions.js
@@ -0,0 +1,9 @@
+'use server'
+
+export async function expensiveCalculation() {
+  console.log('server action invoked')
+  // sleep for 1 second
+  await new Promise((resolve) => setTimeout(resolve, 1000))
+
+  return Math.random()
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/action-after-redirect/form.js b/test/e2e/app-dir/actions-navigation/app/action-after-redirect/form.js
@@ -0,0 +1,35 @@
+'use client'
+
+import React from 'react'
+import { expensiveCalculation } from './actions'
+
+export function Form({ randomNum }) {
+  const [isPending, setIsPending] = React.useState(false)
+  const [result, setResult] = React.useState(null)
+
+  async function handleSubmit(event) {
+    event.preventDefault()
+
+    setIsPending(true)
+    const result = await expensiveCalculation()
+    setIsPending(false)
+    setResult(result)
+  }
+
+  return (
+    <form
+      style={{ display: 'flex', gap: '2rem', flexDirection: 'column' }}
+      id="form"
+      onSubmit={handleSubmit}
+    >
+      <section>
+        <button style={{ width: 'max-content' }} type="submit" id="submit">
+          Submit
+        </button>
+        {isPending && 'Loading...'}
+      </section>
+      <div>Server side rendered number: {randomNum}</div>
+      {result && <div id="result">RESULT FROM SERVER ACTION: {result}</div>}
+    </form>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/action-after-redirect/page.js b/test/e2e/app-dir/actions-navigation/app/action-after-redirect/page.js
@@ -0,0 +1,11 @@
+import { Form } from './form'
+
+export default async function Page() {
+  const randomNum = Math.random()
+
+  return (
+    <div>
+      <Form randomNum={randomNum} />
+    </div>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/layout.js b/test/e2e/app-dir/actions-navigation/app/layout.js
@@ -0,0 +1,8 @@
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <head />
+      <body>{children}</body>
+    </html>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/middleware-redirect/page.js b/test/e2e/app-dir/actions-navigation/app/middleware-redirect/page.js
@@ -0,0 +1,3 @@
+export default function Page() {
+  return <div>Hello World</div>
+}
diff --git a/...app-dir/actions-navigation/app/nested-folder/(foo)/product-category/[...slugs]/actions.js b/...app-dir/actions-navigation/app/nested-folder/(foo)/product-category/[...slugs]/actions.js
@@ -0,0 +1,5 @@
+'use server'
+
+export async function addToCart() {
+  console.log('addToCart')
+}
diff --git a/...2e/app-dir/actions-navigation/app/nested-folder/(foo)/product-category/[...slugs]/page.js b/...2e/app-dir/actions-navigation/app/nested-folder/(foo)/product-category/[...slugs]/page.js
@@ -0,0 +1,24 @@
+'use client'
+import { experimental_useFormStatus as useFormStatus } from 'react-dom'
+import { addToCart } from './actions'
+
+function SubmitButton() {
+  const { pending } = useFormStatus()
+
+  return (
+    <button type="submit" aria-disabled={pending}>
+      Add to cart
+    </button>
+  )
+}
+
+export default async function Page() {
+  return (
+    <>
+      <h1>Add to cart</h1>
+      <form action={addToCart}>
+        <SubmitButton />
+      </form>
+    </>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/nested-folder/[slug]/page.js b/test/e2e/app-dir/actions-navigation/app/nested-folder/[slug]/page.js
@@ -0,0 +1,5 @@
+import Link from 'next/link'
+
+export default async function Page() {
+  return <Link href="../nested-folder/product-category/machine">Machines</Link>
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/nested-folder/page.js b/test/e2e/app-dir/actions-navigation/app/nested-folder/page.js
@@ -0,0 +1,7 @@
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <Link href="../action-after-redirect">Go to ../action-after-redirect</Link>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/app/page.js b/test/e2e/app-dir/actions-navigation/app/page.js
@@ -0,0 +1,9 @@
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <Link href="/middleware-redirect" id="middleware-redirect">
+      Go to /middleware-redirect
+    </Link>
+  )
+}
diff --git a/test/e2e/app-dir/actions-navigation/index.test.ts b/test/e2e/app-dir/actions-navigation/index.test.ts
@@ -0,0 +1,49 @@
+import { createNextDescribe } from 'e2e-utils'
+import { check, waitFor } from 'next-test-utils'
+
+createNextDescribe(
+  'app-dir action handling',
+  {
+    files: __dirname,
+  },
+  ({ next }) => {
+    it('should handle actions correctly after navigation / redirection events', async () => {
+      const browser = await next.browser('/')
+
+      await browser.elementByCss('#middleware-redirect').click()
+
+      expect(await browser.elementByCss('#form').text()).not.toContain(
+        'Loading...'
+      )
+
+      await browser.elementByCss('#submit').click()
+
+      await check(() => {
+        return browser.elementByCss('#form').text()
+      }, /Loading.../)
+
+      // wait for 2 seconds, since the action takes a second to resolve
+      await waitFor(2000)
+
+      expect(await browser.elementByCss('#form').text()).not.toContain(
+        'Loading...'
+      )
+
+      expect(await browser.elementByCss('#result').text()).toContain(
+        'RESULT FROM SERVER ACTION'
+      )
+    })
+
+    it('should handle actions correctly after following a relative link', async () => {
+      const browser = await next.browser('/nested-folder/products')
+
+      await browser.elementByCss('a').click()
+
+      await browser.elementByCss('button').click()
+
+      await check(() => {
+        return (next.cliOutput.match(/addToCart/g) || []).length
+      }, 1)
+    })
+  }
+)
diff --git a/test/e2e/app-dir/actions-navigation/middleware.js b/test/e2e/app-dir/actions-navigation/middleware.js
@@ -0,0 +1,9 @@
+import { NextResponse } from 'next/server'
+
+export function middleware(request) {
+  if (request.nextUrl.pathname.startsWith('/middleware-redirect')) {
+    return NextResponse.redirect(new URL('/action-after-redirect', request.url))
+  }
+}
+
+export const matcher = ['/middleware-redirect']
diff --git a/test/e2e/app-dir/actions-navigation/next.config.js b/test/e2e/app-dir/actions-navigation/next.config.js
@@ -0,0 +1,5 @@
+module.exports = {
+  experimental: {
+    serverActions: true,
+  },
+}
diff --git a/test/e2e/app-dir/actions/app-action.test.ts b/test/e2e/app-dir/actions/app-action.test.ts
@@ -1,6 +1,6 @@
 /* eslint-disable jest/no-standalone-expect */
 import { createNextDescribe } from 'e2e-utils'
-import { check } from 'next-test-utils'
+import { check, waitFor } from 'next-test-utils'
 import { Request } from 'playwright-chromium'
 import fs from 'fs-extra'
 import { join } from 'path'
@@ -436,9 +436,7 @@ createNextDescribe(
       it('should handle redirect to a relative URL in a single pass', async () => {
         const browser = await next.browser('/client/edge')
 
-        await new Promise((resolve) => {
-          setTimeout(resolve, 3000)
-        })
+        await waitFor(3000)
 
         let requests = []
 
@@ -505,9 +503,7 @@ createNextDescribe(
       it('should handle redirect to a relative URL in a single pass', async () => {
         const browser = await next.browser('/client')
 
-        await new Promise((resolve) => {
-          setTimeout(resolve, 3000)
-        })
+        await waitFor(3000)
 
         let requests = []
 

diff --git a/test/e2e/app-dir/actions/app/test/page.js b/test/e2e/app-dir/actions/app/test/page.js
@@ -0,0 +1,9 @@
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <div>
+      <Link href="/middleware-redirect">Go to /middleware-redirect</Link>
+    </div>
+  )
+}