`<Navigate>` re-issues its navigation on every render, causing an unbounded loop
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 15.1k
- Forks
- 1.9k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 143
Description
Which project does this relate to?
Router
Describe the bug
<Navigate> re-issues its navigation on every render.
Its only guard is an identity check on the JSX props object:
const previousPropsRef = React.useRef(null)
useLayoutEffect(() => {
if (previousPropsRef.current !== props) {
navigate(props)
previousPropsRef.current = props
}
}, [router, props, navigate])
React allocates a fresh props object on every render, so previousPropsRef.current !== props is always true and the guard has never prevented anything.
That only becomes observable when the component rendering <Navigate> re-renders. The reproducer covers the three realistic ways that happens:
| Case | Renders of the component holding <Navigate> |
Destination beforeLoad runs |
Loop |
|---|---|---|---|
| 1. subscribes to router state | 26 | n/a | yes |
| 2. external store emits during a pending navigation | 32 | 25 | yes |
3. search as an updater function |
26 | n/a | yes |
Case 1 is self-sustaining and needs nothing else. Issuing the navigation changes router state, which re-renders a component subscribed to that state, which re-issues the navigation. No external input, and the destination has no beforeLoad at all. The cost is unbounded render churn: with the reproducer's stop raised to 100000, it reaches 100001 renders in under half a second.
Case 2 is the one that hurts in production. An unrelated subscription re-renders the component while the navigation is pending, and each re-issue supersedes the in-flight navigation, so the destination's beforeLoad is restarted over and over and the navigation never settles.
Case 3 shows that comparing the props by value rather than by identity would not be sufficient either: search and params accept updater functions, which are usually declared inline and so are a fresh value on every render too.
Worth noting: packages/solid-router and packages/vue-router both run the navigation in onMount / onMounted, so they issue it once and never re-issue. React's Navigate is the only adapter that re-issues, and the presence of this guard suggests the once-only semantics were intended here too.
I mention that because #6672 is effectively this same defect in solid-router and was closed with "throw a redirect in beforeLoad instead". I don't think that answer applies here: this isn't a user-authored effect, it's library code carrying an explicit guard against re-issuing that is a no-op.
Complete minimal reproducer
https://github.com/kamalbennani/router/tree/repro/navigate-loop
Steps to Reproduce the Bug
- Clone https://github.com/kamalbennani/router/tree/repro/navigate-loop (branch
repro/navigate-loop) npm installnpm run dev- Open the dev server URL and click 1. redirect component subscribes to router state
The counters on the page report 26 renders of the component holding <Navigate> and loop detected: true. Cases 2 and 3 are linked from the same page.
The reproducer stops rendering <Navigate> after 25 renders so the tab stays usable. Raise MAX_RENDERS in src/main.jsx to see that none of the cases terminate on their own.
Expected behavior
I expected <Navigate> to issue its navigation once, but it re-issues on every render, and when the component re-renders as a consequence of that navigation the result is an unbounded loop.
Screenshots or Videos
No response
Platform
- Router / Start Version: 1.170.27 (also verified on 1.131.7, 1.136.17, 1.136.18)
- OS: macOS
- Browser: Chrome
- Browser Version: 149
- Bundler: vite
- Bundler Version: 5.4
Additional context
This is not a regression. I checked, because #5905 moved this effect from React.useEffect to useLayoutEffect and that looked like a plausible culprit. It isn't:
| Version | Renders (case 1) | Loop |
|---|---|---|
| 1.131.7 | 26 | yes |
| 1.136.17 (last before #5905) | 26 | yes |
| 1.136.18 (first with #5905) | 26 | yes |
| 1.170.27 | 26 | yes |
1.136.17 and 1.136.18 straddle #5905 exactly and behave identically. I also flipped the single line back to React.useEffect on current main, rebuilt, and the loop was unchanged. The defect predates 1.131.7, and any fix should keep useLayoutEffect so #5905's flicker fix stands.
We hit case 2 in production: a redirect component holding an Apollo subscription, pointed at a route with an async beforeLoad. The page sat on a permanent loading state and issued 4511 requests before the tab died. Nothing in our 4500-test suite caught it, since it only manifests in a browser against a real async guard.
I have a fix and an e2e fixture ready if you're open to a PR. It guards on the resolved destination rather than the props object, keeping useLayoutEffect. Happy to adjust the approach if you'd prefer a different direction, for example making React's Navigate fire once on mount to match the Solid and Vue adapters.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the linked reproducer on the repro/navigate-loop branch: run npm install, npm run dev, and inspect src/main.jsx, including MAX_RENDERS. Verify the three reported cases no longer loop, that an async beforeLoad can settle, and that the React adapter retains useLayoutEffect behavior without re-issuing navigation on every render.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react, typescript
- Domain
- frontend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100