fix refresh behavior for discarded actions (#64532)

When an action is marked as "discarded", we enqueue a refresh, since the
navigation event will be invoked immediately without waiting for the
action to finish. We refresh because it's possible that the discarded
action triggered some sort of mutation/revalidation, and we want the
router to still be able to respond to that new data.

However there's a bug in this logic -- it'll only enqueue the refresh
action if there were no other actions in the queue, ignoring the case
where something is still in the queue. This makes sure that the refresh
is handled after `runRemainingActions` finishes.

When adding a test for the server component case (which doesn't hit this
refresh branch), I noticed `LayoutRouter` caused React to suspend
indefinitely, because it got stuck in the `use(unresolvedThenable)`
case. We should only suspend indefinitely if we kicked off a the
`SERVER_PATCH` action, as otherwise it's possible nothing will ever
break out of that branch.

Fixes #64517
Closes NEXT-3124

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

@@ -437,10 +437,10 @@ function InnerLayoutRouter({
       // It's important that we mark this as resolved, in case this branch is replayed, we don't want to continously re-apply
       // the patch to the tree.
       childNode.lazyDataResolved = true
-    }
 
-    // Suspend infinitely as `changeByServerResponse` will cause a different part of the tree to be rendered.
-    use(unresolvedThenable) as never
+      // Suspend infinitely as `changeByServerResponse` will cause a different part of the tree to be rendered.
+      use(unresolvedThenable) as never
+    }
   }
 
   // If we get to this point, then we know we have something we can render.

packages/next/src/shared/lib/router/action-queue.ts

@@ -48,6 +48,18 @@ function runRemainingActions(
         action: actionQueue.pending,
         setState,
       })
+    } else {
+      // No more actions are pending, check if a refresh is needed
+      if (actionQueue.needsRefresh) {
+        actionQueue.needsRefresh = false
+        actionQueue.dispatch(
+          {
+            type: ACTION_REFRESH,
+            origin: window.location.origin,
+          },
+          setState
+        )
+      }
     }
   }
 }
@@ -75,17 +87,6 @@ async function runAction({
   function handleResult(nextState: AppRouterState) {
     // if we discarded this action, the state should also be discarded
     if (action.discarded) {
-      // if a refresh is needed, we only want to trigger it once the action queue is empty
-      if (actionQueue.needsRefresh && actionQueue.pending === null) {
-        actionQueue.needsRefresh = false
-        actionQueue.dispatch(
-          {
-            type: ACTION_REFRESH,
-            origin: window.location.origin,
-          },
-          setState
-        )
-      }
       return
     }
 

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

@@ -371,6 +371,31 @@ createNextDescribe(
       }, 'success')
     })
 
+    it('should trigger a refresh for a server action that also dispatches a navigation event', async () => {
+      let browser = await next.browser('/revalidate')
+      let initialJustPutit = await browser.elementById('justputit').text()
+
+      // this triggers a revalidate + redirect in a client component
+      await browser.elementById('redirect-revalidate-client').click()
+      await retry(async () => {
+        const newJustPutIt = await browser.elementById('justputit').text()
+        expect(newJustPutIt).not.toBe(initialJustPutit)
+
+        expect(await browser.url()).toBe(`${next.url}/revalidate?foo=bar`)
+      })
+
+      // this triggers a revalidate + redirect in a server component
+      browser = await next.browser('/revalidate')
+      initialJustPutit = await browser.elementById('justputit').text()
+      await browser.elementById('redirect-revalidate').click()
+      await retry(async () => {
+        const newJustPutIt = await browser.elementById('justputit').text()
+        expect(newJustPutIt).not.toBe(initialJustPutit)
+
+        expect(await browser.url()).toBe(`${next.url}/revalidate?foo=bar`)
+      })
+    })
+
     it('should support next/dynamic with ssr: false', async () => {
       const browser = await next.browser('/dynamic-csr')
 

test/e2e/app-dir/actions/app/revalidate/client.js

@@ -0,0 +1,18 @@
+'use client'
+
+import { useRouter } from 'next/navigation'
+
+export default function RedirectClientComponent({ action }) {
+  const router = useRouter()
+  return (
+    <button
+      id="redirect-revalidate-client"
+      onClick={async () => {
+        await action()
+        router.push('/revalidate?foo=bar')
+      }}
+    >
+      redirect + revalidate (client component)
+    </button>
+  )
+}

test/e2e/app-dir/actions/app/revalidate/page.js

@@ -7,6 +7,7 @@ import { redirect } from 'next/navigation'
 import Link from 'next/link'
 
 import { cookies } from 'next/headers'
+import RedirectClientComponent from './client'
 
 export default async function Page() {
   const data = await fetch(
@@ -133,6 +134,24 @@ export default async function Page() {
           redirect
         </button>
       </form>
+      <form>
+        <button
+          id="redirect-revalidate"
+          formAction={async () => {
+            'use server'
+            revalidateTag('justputit')
+            redirect('/revalidate?foo=bar')
+          }}
+        >
+          redirect + revalidate
+        </button>
+      </form>
+      <RedirectClientComponent
+        action={async () => {
+          'use server'
+          revalidateTag('justputit')
+        }}
+      />
     </>
   )
 }

test/e2e/app-dir/navigation/app/metadata-await-promise/nested/loading.js

@@ -1,3 +1,9 @@
+'use client'
+import { useEffect } from 'react'
+
 export default function Loading() {
+  useEffect(() => {
+    window.shownLoading = true
+  }, [])
   return <div id="loading">Loading</div>
 }

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

@@ -841,22 +841,16 @@ createNextDescribe(
       it('should render the final state of the page with correct metadata', async () => {
         const browser = await next.browser('/metadata-await-promise')
 
-        // dev + PPR doesn't trigger the loading boundary as it's not prefetched
-        if (isNextDev && process.env.__NEXT_EXPERIMENTAL_PPR) {
-          await browser
-            .elementByCss("[href='/metadata-await-promise/nested']")
-            .click()
-        } else {
-          const loadingText = await browser
-            .elementByCss("[href='/metadata-await-promise/nested']")
-            .click()
-            .waitForElementByCss('#loading')
-            .text()
-
-          expect(loadingText).toBe('Loading')
-        }
+        await browser
+          .elementByCss("[href='/metadata-await-promise/nested']")
+          .click()
 
         await retry(async () => {
+          // dev + PPR doesn't trigger the loading boundary as it's not prefetched
+          if (!(isNextDev && process.env.__NEXT_EXPERIMENTAL_PPR)) {
+            expect(await browser.eval(`window.shownLoading`)).toBe(true)
+          }
+
           expect(await browser.elementById('page-content').text()).toBe(
             'Content'
           )