fix(react-router): stop Navigate re-issuing its navigation on every render - #8066
Open
kamalbennani wants to merge 3 commits into
Open
fix(react-router): stop Navigate re-issuing its navigation on every render#8066kamalbennani wants to merge 3 commits into
kamalbennani wants to merge 3 commits into
Conversation
`Navigate` guarded its navigation on an identity check against the props object. React allocates a fresh props object on every render, so the guard never held and the navigation was re-issued on every render. That is only observable when the component rendering `Navigate` re-renders while the navigation is still pending, which happens whenever it subscribes to router state or to any external store. Each re-issue supersedes the in-flight navigation before its `beforeLoad` can settle, so the navigation never commits: the app stays on a loading state while requests pile up. Guard on the resolved destination instead. A value comparison of the props is not enough, because `search` and `params` accept updater functions that are usually declared inline and so are fresh on every render too. Adds an e2e fixture covering the three ways the redirect component gets re-rendered during a pending navigation. All three fail before this change.
Close over the committed render's props instead of mirroring them into a ref during render: a render React discards can still write the ref, so the effect could navigate with options from an abandoned render. `Navigate` now resolves its destination during render, which also runs on the server where the navigation effect does not. Adds a server-render test for both the plain and the updater-function form.
The async destination is not a precondition. 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. The async case now sits on its own route, where it shows what it actually contributes - each re-issue supersedes the pending navigation and restarts its beforeLoad.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthrough
ChangesNavigate loop prevention
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The PR fixes repeated navigation and adds focused coverage; the only remaining concern is a minor consistency issue in the new e2e package's internal dependency ranges, which is mergeable with owner awareness or a small follow-up. Sequence Diagram(s)sequenceDiagram
participant Browser
participant Navigate
participant Router
participant AsyncTarget
Browser->>Navigate: Render redirect component
Navigate->>Router: Resolve destination and start navigation
Router->>AsyncTarget: Run beforeLoad
AsyncTarget-->>Router: Complete destination loading
Router-->>Browser: Render target route
Browser->>Navigate: Trigger repeated re-render
Navigate->>Navigate: Reuse resolved href guard
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8060.
Problem
Navigateguards its navigation on an identity check against the props object:React allocates a fresh props object on every render, so this never holds and the navigation is re-issued on every render.
That guard came from #3465 ("only navigate once in StrictMode", fixing #3455). It works for StrictMode because both invocations of the double-invoked effect close over the same props object. Across a genuine re-render it cannot hold, so the general case was never covered.
The consequence is an unbounded loop. Subscribing to router state is sufficient 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 required - with the fixture's stop raised to 100000 it reaches 100001 renders in under half a second.
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. That is the shape that produces unbounded requests. We hit it in production - a redirect component holding a data-fetching subscription, 4511 requests before the tab died.This is not a regression. I checked #5905 (which moved this to
useLayoutEffect) because it looked like the culprit. It isn't - 1.136.17 and 1.136.18 straddle it and behave identically, and flipping the line back toReact.useEffectonmainchanges nothing. The defect predates 1.131.7, and this PR keepsuseLayoutEffectso #5905's flicker fix stands.Fix
Guard on the resolved destination instead of the props object.
A value comparison of the props is not sufficient:
searchandparamsaccept updater functions, which are usually declared inline and so are a fresh value on every render too. Resolving the location collapses those to a concrete href.Linkalready builds the location on every render, so this is a cost the router is used to paying.This does not stop
Navigatefrom navigating on updates. ANavigatewhose destination genuinely changes still re-navigates; only re-issuing the identical resolved destination is suppressed.The effect closes over the committed render's props rather than reading a ref written during render, so a render React discards cannot influence which options get used.
Tests
New e2e fixture
e2e/react-router/navigate-component, covering the three ways the redirect component gets re-rendered:searchas an updater functionAll three fail on
mainand pass with this change.Navigatenow resolves its destination during render, which also runs on the server where the navigation effect does not, sopackages/react-router/tests/navigate-component.test.tsxadds a server-render check for the plain and updater-function forms.Verification: 1012 react-router unit tests pass (1010 before, +2 added), 0 lint errors, no type errors, and the
basicandbasic-file-basede2e suites pass.Note on the lockfile
The
pnpm-lock.yamldiff contains peer-resolution churn beyond the new fixture's own entry. That is pre-existing: I confirmed an empty workspace package produces a 2-line diff, but any package with dependencies triggers a full peer re-resolution, because the committed lockfile is stale relative to pnpm 11.9.0. Happy to drop the fixture into an existing e2e app instead if you'd rather avoid it.Note on the other adapters
solid-routerandvue-routerrun this inonMount/onMountedand issue the navigation exactly once. React'sNavigateis the only adapter that re-issues. If you'd prefer React match them by firing once on mount, I'm glad to redo it that way - it's a larger behavior change, so I went with the smaller one that makes the existing guard work.Summary by CodeRabbit
Bug Fixes
<Navigate>repeatedly triggering navigation when components re-render.Tests