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)
vercel · Sep 7, 2023 · d330f7b · d330f7b
1 parent 068002b
commit d330f7b
Show file tree

Hide file tree

Showing 8 changed files with 134 additions and 34 deletions.
diff --git a/packages/next/src/client/components/app-router.tsx b/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,11 +409,16 @@ function Router({
   // in <Offscreen>. At least I hope so. (It will run twice in dev strict mode,
   // but that's... fine?)
   if (pushRef.mpaNavigation) {
-    const location = window.location
-    if (pushRef.pendingPush) {
-      location.assign(canonicalUrl)
-    } else {
-      location.replace(canonicalUrl)
+    // 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

diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.test.tsx b/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(() =>

diff --git a/packages/next/src/client/components/router-reducer/reducers/refresh-reducer.test.tsx b/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,
     }
 

diff --git a/packages/next/src/client/components/router-reducer/reducers/server-patch-reducer.test.tsx b/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(() =>

diff --git a/packages/next/src/client/components/router-reducer/router-reducer-types.ts b/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
 }
 

diff --git a/test/e2e/app-dir/navigation/app/mpa-nav-test/page.js b/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>
+    </>
+  )
+}