fix: ensure mpa navigation render side effects are only fired once (#55032)

This is to fix an issue where these redirect side effects can be fired multiple times when the router reducer state changes. This block is still run when the router state updates, which can lead to superfluous attempts to redirect to a page.

With these changes, we keep track of the page that is being redirected to. If a re-render occurs while that request is in flight, we don't trigger the side effects. 

[Slack x-ref](https://vercel.slack.com/archives/C04DUD7EB1B/p1694049914264839)

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

@@ -33,12 +33,12 @@ import {
   ACTION_RESTORE,
   ACTION_SERVER_ACTION,
   ACTION_SERVER_PATCH,
+  Mutable,
   PrefetchKind,
   ReducerActions,
   RouterChangeByServerResponse,
   RouterNavigate,
   ServerActionDispatcher,
-  ServerActionMutable,
 } from './router-reducer/router-reducer-types'
 import { createHrefFromUrl } from './router-reducer/create-href-from-url'
 import {
@@ -73,7 +73,7 @@ export function getServerActionDispatcher() {
   return globalServerActionDispatcher
 }
 
-let globalServerActionMutable: ServerActionMutable['globalMutable'] = {
+let globalMutable: Mutable['globalMutable'] = {
   refresh: () => {}, // noop until the router is initialized
 }
 
@@ -145,7 +145,7 @@ function useServerActionDispatcher(dispatch: React.Dispatch<ReducerActions>) {
         dispatch({
           ...actionPayload,
           type: ACTION_SERVER_ACTION,
-          mutable: { globalMutable: globalServerActionMutable },
+          mutable: { globalMutable },
           cache: createEmptyCacheNode(),
         })
       })
@@ -174,7 +174,7 @@ function useChangeByServerResponse(
           previousTree,
           overrideCanonicalUrl,
           cache: createEmptyCacheNode(),
-          mutable: {},
+          mutable: { globalMutable },
         })
       })
     },
@@ -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)
-      globalServerActionMutable.pendingNavigatePath = href
+      globalMutable.pendingNavigatePath = href
 
       return dispatch({
         type: ACTION_NAVIGATE,
@@ -197,7 +197,7 @@ function useNavigate(dispatch: React.Dispatch<ReducerActions>): RouterNavigate {
         shouldScroll: shouldScroll ?? true,
         navigateType,
         cache: createEmptyCacheNode(),
-        mutable: {},
+        mutable: { globalMutable },
       })
     },
     [dispatch]
@@ -322,7 +322,7 @@ function Router({
           dispatch({
             type: ACTION_REFRESH,
             cache: createEmptyCacheNode(),
-            mutable: {},
+            mutable: { globalMutable },
             origin: window.location.origin,
           })
         })
@@ -338,7 +338,7 @@ function Router({
             dispatch({
               type: ACTION_FAST_REFRESH,
               cache: createEmptyCacheNode(),
-              mutable: {},
+              mutable: { globalMutable },
               origin: window.location.origin,
             })
           })
@@ -357,7 +357,7 @@ function Router({
   }, [appRouter])
 
   useEffect(() => {
-    globalServerActionMutable.refresh = appRouter.refresh
+    globalMutable.refresh = appRouter.refresh
   }, [appRouter.refresh])
 
   if (process.env.NODE_ENV !== 'production') {
@@ -409,12 +409,17 @@ function Router({
   // in <Offscreen>. At least I hope so. (It will run twice in dev strict mode,
   // but that's... fine?)
   if (pushRef.mpaNavigation) {
+    // if there's a re-render, we don't want to trigger another redirect if one is already in flight to the same URL
+    if (globalMutable.pendingMpaPath !== canonicalUrl) {
       const location = window.location
       if (pushRef.pendingPush) {
         location.assign(canonicalUrl)
       } else {
         location.replace(canonicalUrl)
       }
+
+      globalMutable.pendingMpaPath = canonicalUrl
+    }
     // TODO-APP: Should we listen to navigateerror here to catch failed
     // navigations somehow? And should we call window.stop() if a SPA navigation
     // should interrupt an MPA one?

packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx

@@ -106,6 +106,10 @@ const getInitialRouterStateTree = (): FlightRouterState => [
   true,
 ]
 
+const globalMutable = {
+  refresh: () => {},
+}
+
 async function runPromiseThrowChain(fn: any): Promise<any> {
   try {
     return await fn()
@@ -194,7 +198,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     const newState = await runPromiseThrowChain(() =>
@@ -438,7 +442,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -633,7 +637,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -792,7 +796,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -948,7 +952,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -1147,7 +1151,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -1317,7 +1321,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -1630,7 +1634,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => navigateReducer(state, action))
@@ -1841,6 +1845,7 @@ describe('navigateReducer', () => {
         hashFragment: '#hash',
         pendingPush: true,
         shouldScroll: true,
+        globalMutable,
       },
     }
 
@@ -1983,7 +1988,7 @@ describe('navigateReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     const newState = await runPromiseThrowChain(() =>

packages/next/src/client/components/router-reducer/reducers/refresh-reducer.test.tsx

@@ -66,6 +66,10 @@ const getInitialRouterStateTree = (): FlightRouterState => [
   true,
 ]
 
+const globalMutable = {
+  refresh: () => {},
+}
+
 async function runPromiseThrowChain(fn: any): Promise<any> {
   try {
     return await fn()
@@ -139,7 +143,7 @@ describe('refreshReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
       origin: new URL('/linking', 'https://localhost').origin,
     }
 
@@ -300,7 +304,7 @@ describe('refreshReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
       origin: new URL('/linking', 'https://localhost').origin,
     }
 
@@ -487,7 +491,7 @@ describe('refreshReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
       origin: new URL('/linking', 'https://localhost').origin,
     }
 
@@ -723,7 +727,7 @@ describe('refreshReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
       origin: new URL('/linking', 'https://localhost').origin,
     }
 

packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx

@@ -7,6 +7,10 @@ import type {
 
 const buildId = 'development'
 
+const globalMutable = {
+  refresh: () => {},
+}
+
 jest.mock('../fetch-server-response', () => {
   const flightData: FlightData = [
     [
@@ -184,7 +188,7 @@ describe('serverPatchReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     const newState = await runPromiseThrowChain(() =>
@@ -375,7 +379,7 @@ describe('serverPatchReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     await runPromiseThrowChain(() => serverPatchReducer(state, action))
@@ -514,7 +518,7 @@ describe('serverPatchReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     const state = createInitialRouterState({
@@ -556,7 +560,7 @@ describe('serverPatchReducer', () => {
         subTreeData: null,
         parallelRoutes: new Map(),
       },
-      mutable: {},
+      mutable: { globalMutable },
     }
 
     const newState = await runPromiseThrowChain(() =>

packages/next/src/client/components/router-reducer/router-reducer-types.ts

@@ -38,11 +38,15 @@ export interface Mutable {
   prefetchCache?: AppRouterState['prefetchCache']
   hashFragment?: string
   shouldScroll?: boolean
+  globalMutable: {
+    pendingNavigatePath?: string
+    pendingMpaPath?: string
+    refresh: () => void
+  }
 }
 
 export interface ServerActionMutable extends Mutable {
   inFlightServerAction?: Promise<any> | null
-  globalMutable: { pendingNavigatePath?: string; refresh: () => void }
   actionResultResolved?: boolean
 }
 

test/e2e/app-dir/navigation/app/mpa-nav-test/page.js

@@ -0,0 +1,38 @@
+'use client'
+import Link from 'next/link'
+import { useEffect, useRef } from 'react'
+
+export default function Page() {
+  const prefetchRef = useRef()
+  const slowPageRef = useRef()
+
+  useEffect(() => {
+    function triggerPrefetch() {
+      const event = new MouseEvent('mouseover', {
+        view: window,
+        bubbles: true,
+        cancelable: true,
+      })
+
+      prefetchRef.current.dispatchEvent(event)
+      console.log('dispatched')
+    }
+
+    slowPageRef.current.click()
+
+    setInterval(() => {
+      triggerPrefetch()
+    }, 1000)
+  }, [])
+
+  return (
+    <>
+      <Link id="link-to-slow-page" href="/slow-page" ref={slowPageRef}>
+        To /slow-page
+      </Link>
+      <Link id="prefetch-link" href="/hash" ref={prefetchRef}>
+        Prefetch link
+      </Link>
+    </>
+  )
+}

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

@@ -1,5 +1,5 @@
 import { createNextDescribe } from 'e2e-utils'
-import { check } from 'next-test-utils'
+import { check, waitFor } from 'next-test-utils'
 import type { Request } from 'playwright-chromium'
 
 createNextDescribe(
@@ -497,6 +497,33 @@ createNextDescribe(
           .waitForElementByCss('#link-to-app')
         expect(await browser.url()).toBe(next.url + '/some')
       })
+
+      if (!isNextDev) {
+        // this test is pretty hard to test in playwright, so most of the heavy lifting is in the page component itself
+        // it triggers a hover on a link to initiate a prefetch request every second, and so we check that
+        // it doesn't repeatedly initiate the mpa navigation request
+        it('should not continously initiate a mpa navigation to the same URL when router state changes', async () => {
+          let requestCount = 0
+          const browser = await next.browser('/mpa-nav-test', {
+            beforePageLoad(page) {
+              page.on('request', (request) => {
+                const url = new URL(request.url())
+                // skip rsc prefetches
+                if (url.pathname === '/slow-page' && !url.search) {
+                  requestCount++
+                }
+              })
+            },
+          })
+
+          await browser.waitForElementByCss('#link-to-slow-page')
+
+          // wait a few seconds since prefetches are triggered in 1s intervals in the page component
+          await waitFor(5000)
+
+          expect(requestCount).toBe(1)
+        })
+      }
     })
 
     describe('nested navigation', () => {
@@ -562,7 +589,7 @@ createNextDescribe(
         )
       })
 
-      it('should emit refresh meta tag (peramnent) for redirect page when streaming', async () => {
+      it('should emit refresh meta tag (permanent) for redirect page when streaming', async () => {
         const html = await next.render('/redirect/suspense-2')
         expect(html).toContain(
           '<meta http-equiv="refresh" content="0;url=/redirect/result"/>'

test/e2e/app-dir/navigation/pages/slow-page.js

@@ -0,0 +1,13 @@
+export default function Page() {
+  return 'Hello from slow page'
+}
+
+export async function getServerSideProps({ resolvedUrl }) {
+  if (!resolvedUrl.includes('?_rsc')) {
+    // only stall on the navigation, not prefetch
+    await new Promise((resolve) => setTimeout(resolve, 100000))
+  }
+  return {
+    props: {},
+  }
+}