Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/olive-jokes-hunt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@tanstack/react-router': patch
---

Stop `Navigate` from re-issuing its navigation on every render. The guard compared the props object by identity, which is fresh on every render, so a component that re-rendered while the destination was still pending superseded its own in-flight navigation and never committed it. The guard now compares the resolved destination.
12 changes: 12 additions & 0 deletions e2e/react-router/navigate-component/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Navigate component test</title>
</head>
<body>
<div id="app"></div>
<script type="module" src="/src/main.tsx"></script>
</body>
</html>
26 changes: 26 additions & 0 deletions e2e/react-router/navigate-component/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"name": "tanstack-router-e2e-navigate-component",
"private": true,
"type": "module",
"scripts": {
"dev": "vite --port 3000",
"dev:e2e": "vite",
"build": "vite build && tsc --noEmit",
"preview": "vite preview",
"start": "vite",
"test:e2e": "rm -rf port*.txt; playwright test --project=chromium"
},
"dependencies": {
"@tanstack/react-router": "workspace:^",
"react": "^19.0.0",
"react-dom": "^19.0.0"
},
"devDependencies": {
"@playwright/test": "^1.61.0",
"@tanstack/router-e2e-utils": "workspace:^",
"@types/react": "^19.0.8",
"@types/react-dom": "^19.0.3",
"@vitejs/plugin-react": "^6.0.1",
"vite": "^8.0.14"
}
}
25 changes: 25 additions & 0 deletions e2e/react-router/navigate-component/playwright.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { defineConfig, devices } from '@playwright/test'
import { getTestServerPort } from '@tanstack/router-e2e-utils'
import packageJson from './package.json' with { type: 'json' }

const PORT = await getTestServerPort(packageJson.name)
const baseURL = `http://localhost:${PORT}`

export default defineConfig({
testDir: './tests',
workers: 1,
reporter: [['line']],
use: { baseURL },
webServer: {
command: `VITE_NODE_ENV="test" VITE_SERVER_PORT=${PORT} pnpm dev:e2e --port ${PORT}`,
url: baseURL,
reuseExistingServer: !process.env.CI,
stdout: 'pipe',
},
projects: [
{
name: 'chromium',
use: { ...devices['Desktop Chrome'] },
},
],
})
186 changes: 186 additions & 0 deletions e2e/react-router/navigate-component/src/main.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import ReactDOM from 'react-dom/client'
import { useSyncExternalStore } from 'react'
import {
Navigate,
Outlet,
RouterProvider,
createRootRoute,
createRoute,
createRouter,
useRouterState,
} from '@tanstack/react-router'

/**
* `<Navigate>` re-issues its navigation from an effect guarded by an identity
* check on the JSX props object. React allocates a fresh props object on every
* render, so an identity check never holds and the navigation is re-issued on
* every render.
*
* That is only observable when the component rendering `<Navigate>` re-renders,
* which the three routes below are the realistic ways of causing.
*
* Subscribing to router state is enough on its own: issuing the navigation
* changes router state, which re-renders the component, which re-issues the
* navigation. No external input and no async destination are needed.
*
* An async destination makes it worse rather than being a precondition. It
* stays pending across the re-renders, so each re-issue supersedes the previous
* one and restarts its `beforeLoad`, turning the render loop into unbounded
* requests.
*
* `MAX_RENDERS` bounds the loop so a regressed build fails the assertions
* instead of exhausting the browser tab.
*/

const MAX_RENDERS = 25

const stats = {
/** Renders of the component that returns `<Navigate>`. */
redirectRenders: 0,
/** Invocations of the destination route's async `beforeLoad`. */
targetBeforeLoads: 0,
/** Sticky: set once the render guard trips, survives the redirect unmount. */
loopDetected: false,
}

declare global {
interface Window {
__navigateStats: typeof stats
}
}

window.__navigateStats = stats

/** Returns true once the redirect component has rendered suspiciously often. */
function trackRender() {
stats.redirectRenders++
if (stats.redirectRenders > MAX_RENDERS) {
stats.loopDetected = true
return true
}
return false
}

/**
* An external store that keeps ticking while a navigation is pending, standing
* in for the data-fetching subscriptions apps commonly hold in redirect
* components.
*/
let tick = 0
const listeners = new Set<() => void>()
setInterval(() => {
tick++
listeners.forEach((listener) => listener())
}, 20)

function subscribe(listener: () => void) {
listeners.add(listener)
return () => listeners.delete(listener)
}

function RootComponent() {
const pathname = useRouterState({ select: (s) => s.location.pathname })

return (
<div>
<h1>Navigate component</h1>
<div data-testid="pathname">{pathname}</div>
<hr />
<Outlet />
</div>
)
}

const rootRoute = createRootRoute({ component: RootComponent })

/** Re-renders because it subscribes to router state, with no external input. */
function RedirectViaRouterState() {
useRouterState()

if (trackRender()) {
return <div data-testid="loop-detected">loop detected</div>
}

return <Navigate to="/target" replace />
}

/** Re-renders because an unrelated external store keeps emitting. */
function RedirectViaExternalStore() {
useSyncExternalStore(subscribe, () => tick)

if (trackRender()) {
return <div data-testid="loop-detected">loop detected</div>
}

return <Navigate to="/async-target" replace />
}

/** Re-renders like the above, but passes `search` as an updater function. */
function RedirectWithFunctionSearch() {
useRouterState()

if (trackRender()) {
return <div data-testid="loop-detected">loop detected</div>
}

return <Navigate to="/target" search={(prev) => ({ ...prev })} replace />
}

const functionSearchRedirectRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/redirect-function-search',
component: RedirectWithFunctionSearch,
})

const routerStateRedirectRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/redirect-router-state',
component: RedirectViaRouterState,
})

const externalStoreRedirectRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/redirect-external-store',
component: RedirectViaExternalStore,
})

const targetRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/target',
component: () => <div data-testid="target-content">Target</div>,
})

// A destination that does not resolve synchronously stays pending across the
// re-renders, so each re-issue supersedes the previous one and restarts this
// guard. That is what turns the loop into unbounded requests.
const asyncTargetRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/async-target',
beforeLoad: async () => {
stats.targetBeforeLoads++
await new Promise((resolve) => setTimeout(resolve, 150))
},
component: () => <div data-testid="target-content">Async target</div>,
})

const routeTree = rootRoute.addChildren([
routerStateRedirectRoute,
externalStoreRedirectRoute,
functionSearchRedirectRoute,
targetRoute,
asyncTargetRoute,
])

const router = createRouter({ routeTree })

declare module '@tanstack/react-router' {
interface Register {
router: typeof router
}
}

const rootElement = document.getElementById('app')!

if (!rootElement.innerHTML) {
ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />)
}
64 changes: 64 additions & 0 deletions e2e/react-router/navigate-component/tests/navigate.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { expect, test } from '@playwright/test'

interface NavigateStats {
redirectRenders: number
targetBeforeLoads: number
loopDetected: boolean
}

declare global {
interface Window {
__navigateStats: NavigateStats
}
}

/**
* `<Navigate>` must issue its navigation once, however often the component
* rendering it re-renders.
*/

test('<Navigate> issues once when the redirect component subscribes to router state', async ({
page,
}) => {
await page.goto('/redirect-router-state')

await expect(page.getByTestId('target-content')).toBeVisible()
await expect(page.getByTestId('pathname')).toHaveText('/target')

const stats = await page.evaluate(() => window.__navigateStats)

// Self-sustaining: the navigation itself is what re-renders the component,
// so this loops with no external input and a synchronous destination.
expect(stats.loopDetected).toBe(false)
})

test('<Navigate> issues once when an external store re-renders the redirect component', async ({
page,
}) => {
await page.goto('/redirect-external-store')

await expect(page.getByTestId('target-content')).toBeVisible()
await expect(page.getByTestId('pathname')).toHaveText('/async-target')

const stats = await page.evaluate(() => window.__navigateStats)

expect(stats.loopDetected).toBe(false)
// Each re-issue supersedes the pending navigation and restarts the
// destination guard, which is what makes the loop cost requests.
expect(stats.targetBeforeLoads).toBe(1)
})

test('<Navigate> issues once when search is passed as an updater function', async ({
page,
}) => {
await page.goto('/redirect-function-search')

await expect(page.getByTestId('target-content')).toBeVisible()
await expect(page.getByTestId('pathname')).toHaveText('/target')

const stats = await page.evaluate(() => window.__navigateStats)

// Inline updater functions are fresh on every render, so guarding on the
// props alone - by identity or by value - does not hold here.
expect(stats.loopDetected).toBe(false)
})
15 changes: 15 additions & 0 deletions e2e/react-router/navigate-component/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"compilerOptions": {
"strict": true,
"esModuleInterop": true,
"jsx": "react-jsx",
"target": "ESNext",
"moduleResolution": "Bundler",
"module": "ESNext",
"resolveJsonModule": true,
"allowJs": true,
"skipLibCheck": true,
"types": ["vite/client"]
},
"exclude": ["node_modules", "dist"]
}
6 changes: 6 additions & 0 deletions e2e/react-router/navigate-component/vite.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'

export default defineConfig({
plugins: [react()],
})
35 changes: 25 additions & 10 deletions packages/react-router/src/useNavigate.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,33 @@ export function Navigate<
const router = useRouter()
const navigate = useNavigate()

const previousPropsRef = React.useRef<NavigateOptions<
TRouter,
TFrom,
TTo,
TMaskFrom,
TMaskTo
> | null>(null)
// Guard on the resolved destination rather than on the props object.
//
// React allocates a fresh props object on every render, so an identity check
// never holds and the navigation is re-issued on every render. That is only
// observable when this component re-renders while the navigation is still
// pending - each re-issue supersedes the in-flight navigation before it can
// settle, so it never commits and the app is stuck on a loading state.
//
// A value comparison of `props` is not enough either: `search` and `params`
// accept updater functions, which are usually declared inline and so are also
// fresh on every render. Resolving the location collapses those to a concrete
// href. `Link` already builds the location on every render, so this is a cost
// the router is used to paying.
const href = router.buildLocation(props as any).href

const previousHrefRef = React.useRef<string | null>(null)

useLayoutEffect(() => {
if (previousPropsRef.current !== props) {
if (previousHrefRef.current !== href) {
previousHrefRef.current = href
navigate(props)
previousPropsRef.current = props
}
}, [router, props, navigate])
// `props` is intentionally omitted: it is a fresh object on every render,
// and `href` is what determines whether the destination actually changed.
// Closing over `props` from the committed render keeps the options that are
// not part of the href, such as `replace`, consistent with that href.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [href, navigate])
return null
}
Loading