perf(react-router): bail out of Link re-renders when href and active state are unchanged - #7952
perf(react-router): bail out of Link re-renders when href and active state are unchanged#7952matclayton wants to merge 9 commits into
Conversation
…state are unchanged useLinkProps subscribes to the location store with an identity selector and an href comparator, then derives href and isActive from the published location in downstream memos. The comparator can only ask "is this a different URL?", never "does this link care?", so every Link on the page re-renders on every navigation. Move the location-derived values into the selector and compare them, so a link whose resolved href and active state are unaffected by a navigation bails out. buildLocation still runs once per link per location change; what goes away is the React render and the host reconciliation under it. doPreload no longer pre-supplies _builtLocation, because the built location is no longer kept in render state. preloadRoute already falls back to building it, which is what handleClick has always relied on for router.navigate. The isActive and externalLink bodies move to module-level helpers unchanged so the selector stays readable; activeOptions is spread into its four primitive fields in the dependency list because callers routinely pass an inline object literal.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesLink state subscription
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocationStore
participant LinkProps
participant Router
participant Anchor
LocationStore->>LinkProps: publish location
LinkProps->>Router: build destination location
Router-->>LinkProps: return selected link state
LinkProps-->>Anchor: provide href and active state
Anchor->>Anchor: render when selected state changes
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-router/tests/link.test.tsx (1)
7615-7632: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the published link state, not only the render counts.
The test proves that the unaffected link does not re-render. It does not prove that the selector still publishes correct values. A selector that returned a constant
LinkStatewould also pass. Add assertions on the active status ofbecomesActiveand on thehrefofunaffectedafter navigation.💚 Proposed additional assertions
// `/posts` gains its active state, so it has to re-render. expect(renderCounts.becomesActive).toBeGreaterThan(before.becomesActive) + expect(screen.getByTestId('becomesActive')).toHaveAttribute( + 'data-status', + 'active', + ) // `/elsewhere` is neither the origin nor the destination: its href and // active state are identical before and after, so the subscription must // bail out rather than publish an equal value. expect(renderCounts.unaffected).toBe(before.unaffected) + expect(screen.getByTestId('unaffected')).toHaveAttribute( + 'href', + '/elsewhere', + ) + expect(screen.getByTestId('unaffected')).not.toHaveAttribute('data-status') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/tests/link.test.tsx` around lines 7615 - 7632, Extend the test around the existing becomesActive and unaffected link references to assert published state after navigation: verify becomesActive is active and verify unaffected retains the expected href. Keep the render-count assertions, ensuring the test validates both selector values and the unaffected link’s bailout behavior.packages/react-router/src/link.tsx (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd braces to the single-line
ifbodies and drop the redundant cast.Lines 76, 77, and 101 use one-line
ifbodies. The coding guidelines require curly braces for allifstatements. At line 79,tois already narrowed tostringby the guard at line 77, soas anyremoves type information without need.♻️ Proposed style fix
- if (isSafeInternal(to)) return undefined - if (typeof to !== 'string' || to.indexOf(':') === -1) return undefined + if (isSafeInternal(to)) { + return undefined + } + if (typeof to !== 'string' || to.indexOf(':') === -1) { + return undefined + } try { - new URL(to as any) + new URL(to)Apply the same change at line 101:
- if (isExternal) return false + if (isExternal) { + return false + }As per coding guidelines: "Always use curly braces for
if,else, loops, and similar control statements."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/link.tsx` around lines 76 - 79, Update the relevant conditionals in the link handling flow, including the guards around isSafeInternal and the string check and the conditional at the later indicated location, to use curly-braced bodies. In the new URL validation, remove the unnecessary “as any” cast because the preceding typeof guard narrows to to as a string.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react-router/src/link.tsx`:
- Around line 76-79: Update the relevant conditionals in the link handling flow,
including the guards around isSafeInternal and the string check and the
conditional at the later indicated location, to use curly-braced bodies. In the
new URL validation, remove the unnecessary “as any” cast because the preceding
typeof guard narrows to to as a string.
In `@packages/react-router/tests/link.test.tsx`:
- Around line 7615-7632: Extend the test around the existing becomesActive and
unaffected link references to assert published state after navigation: verify
becomesActive is active and verify unaffected retains the expected href. Keep
the render-count assertions, ensuring the test validates both selector values
and the unaffected link’s bailout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f721a164-9e52-4f8c-9625-fc7274c2309c
📒 Files selected for processing (2)
packages/react-router/src/link.tsxpackages/react-router/tests/link.test.tsx
The comments explained the bail-out rationale twice — once on the LinkState type and again above the selector — and two helper docblocks restated their function names. The rationale now appears once, where a reader meets the selector; the detail belongs in the PR description rather than the source. Also braces the three single-line if bodies, per the AGENTS.md rule that if/else bodies always use curly braces.
|
View your CI Pipeline Execution ↗ for commit 48dc687
☁️ Nx Cloud last updated this comment at |
Merging this PR will regress 5 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | client-control-flow navigation loop (react) |
66.1 ms | 72.3 ms | -8.66% |
| ❌ | Simulation | client-head navigation loop (react) |
76.2 ms | 82.3 ms | -7.51% |
| ❌ | Memory | mem server request-churn (react) |
491.6 KB | 528.6 KB | -6.99% |
| ❌ | Memory | mem server error-paths redirect (react) |
197 KB | 208.7 KB | -5.61% |
| ❌ | Simulation | ssr assets linked-css control (vue) |
186.1 ms | 193.9 ms | -4.01% |
| ⚡ | Memory | mem server error-paths unmatched (vue) |
2,142.1 KB | 478.2 KB | ×4.5 |
| ⚡ | Simulation | client-links navigation loop (react) |
204.8 ms | 98.3 ms | ×2.1 |
| ⚡ | Memory | mem server peak-large-page (react) |
1,737.5 KB | 952.9 KB | +82.34% |
| ⚡ | Memory | mem server error-paths redirect (solid) |
388 KB | 273.7 KB | +41.76% |
| ⚡ | Simulation | client-route-tree-scale navigation loop (react) |
72.3 ms | 55.3 ms | +30.92% |
| ⚡ | Simulation | client-side navigation loop (react) |
47.8 ms | 41.2 ms | +16.13% |
| ⚡ | Simulation | client-preload interaction loop (react) |
53.4 ms | 49.1 ms | +8.72% |
| ⚡ | Memory | mem server error-paths not-found (react) |
277.7 KB | 255.7 KB | +8.58% |
| ⚡ | Simulation | client-search-params navigation loop (react) |
80.9 ms | 75.8 ms | +6.67% |
| ⚡ | Simulation | client-nested-params navigation loop (react) |
76.7 ms | 73.4 ms | +4.52% |
| ⚡ | Memory | mem client navigation-churn (vue) |
1.3 MB | 1.2 MB | +3.83% |
| ⚡ | Simulation | ssr control-flow unmatched 404 (react) |
58.1 ms | 56.4 ms | +3.06% |
| 👁 | Memory | mem server error-paths unmatched (react) |
317.5 KB | 268.9 KB | +18.08% |
| 👁 | Memory | mem server error-paths redirect (vue) |
338.6 KB | 300 KB | +12.86% |
| 👁 | Memory | mem server server-fn-churn (vue) |
4,147.3 KB | 264.3 KB | ×16 |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing matclayton:link-rerender-bailout (48dc687) with main (697ebb6)
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as environment_state because the failing task (tanstack-solid-start-e2e-basic:test:e2e--vite-preview) belongs to the solid-start E2E suite, while this PR exclusively modifies @tanstack/react-router. The test asserting window.SCRIPT_1 === true after client-side navigation is a solid-start script-injection concern with no connection to the Link re-render bail-out changes introduced here.
No code changes were suggested for this issue.
You can trigger a rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
| // state. Matches `handleClick`, which lets `router.navigate` build its own. | ||
| router.preloadRoute({ ..._options } as any).catch((err) => { |
There was a problem hiding this comment.
why not return the built location from selectLinkState and use it here? It's a cheap way to bypass a bunch of work (thouh admittedly "preload" work happens less frequently than "location update" re-renders)
(maybe it's not possible without causing a re-render on every location change, just asking in case it is. This might also be the only place in the repo where we use _builtLocation as a param for preloadRoute, so if we really don't use it anymore, we might be able to clean that code path up)
There was a problem hiding this comment.
I don't think it can work, for the reason you suspected. Two ways to try it:
- Include
nextin the compared state. Its identity changes on every location change, so the comparator is always false and every link re-renders on every navigation. That removes the point of the PR. - Return it but leave it out of the comparator. When the comparator reports equal,
useSyncExternalStoreWithSelectorhands back the previous selection, sonextis stale. Harmless for an absoluteto, wrong for relativetoand inherited params: you'd preload a location built from the old one, silently.
On cleaning up the code path, not yet: _builtLocation is still passed to preloadRoute by packages/solid-router/src/link.tsx:254 and packages/vue-router/src/link.tsx:255. It'd only become removable once those get the same treatment, which I've deliberately left alone here.
There was a problem hiding this comment.
once we've confirmed this change is acceptable for React, I think it should also be applied to Solid and Vue to limit drift. And then if what you are saying is correct, we will be able to remove _builtLocation which saves some bytes again
There was a problem hiding this comment.
Happy to have a go at those but it'll be much more AI driven and less me steering, as honestly I'm not a Vue/solid person, would you like them on this PR?
There was a problem hiding this comment.
I looked at both. Two findings, and they both point away from the plan.
The change does not port, because the problem is React specific. What this PR removes is a React component re-render: useLinkProps re-runs and the whole <a> subtree reconciles on every navigation. Solid and Vue have no such thing. In Solid, useLinkProps runs once per link and next / hrefOption / isActive are createMemos (packages/solid-router/src/link.tsx:136-203), so a location change recomputes those memos and updates only the bound attributes. Vue is the same shape with computed and a computedProps ref built in setup() (packages/vue-router/src/link.tsx:226-244, 427-449). Neither re-runs the component body, so there is no re-render to bail out of.
Both also already gate on the href the same way this PR's base did, via the location store's equality option: solid-router/src/link.tsx:128-131 and vue-router/src/link.tsx:222-224. Converting either to a tuple selector would do the same work at coarser granularity, and would lose the per-attribute update that fine-grained reactivity gives them. I would leave both alone.
_builtLocation cannot be removed. It is core redirect plumbing rather than a link optimisation, so it survives regardless of what the adapters do:
router-core/src/router.ts:2437throwsredirect({ href, _builtLocation: nextLocation })from core itself.resolveRedirectreads it atrouter.ts:2804-2806to skip rebuilding the location.router.ts:2825gates the dangerous-protocol check on its absence, so a redirect carrying an internally built location deliberately skips validation that an externally suppliedhrefreceives. Removing the field would change that security behaviour.
The only piece that becomes dead if all three adapters stop passing it is the opts._builtLocation ?? fallback in preloadRoute (router.ts:2891), one line. The type, the redirect plumbing and the protocol gate all stay.
Worth adding that Solid and Vue pass an already memoized next() into preloadRoute, so for them the parameter is a genuine saving. Dropping it there would be a pessimisation, which is the opposite of the byte win. React is the odd one out here only because the selector cannot safely publish next (my earlier comment in this thread).
| const { | ||
| exact: activeExact, | ||
| explicitUndefined: activeExplicitUndefined, | ||
| includeHash: activeIncludeHash, | ||
| includeSearch: activeIncludeSearch, | ||
| } = activeOptions ?? {} |
There was a problem hiding this comment.
I think (needs to be checked) that we would save a few bytes by not destructuring here, and just
- using property access in the dependencies array
activeOptions.exact, activeOptions.explicitUndefined, activeOptions.includeHash, activeOptions.includeSearch,
- and not re-building the object when calling
resolveIsActiveresolveIsActive( //..., activeOptions, //..., )
There was a problem hiding this comment.
Tried this: lint fails. Passing activeOptions straight through to resolveIsActive makes the body reference the whole object, so react-hooks/exhaustive-deps then wants it in the dependency array:
542:5 error React Hook React.useCallback has a missing dependency: 'activeOptions' react-hooks/exhaustive-deps
The destructure keeps the deps as primitives while the body still gets an object. Also worth noting the snippet needs activeOptions?.exact rather than activeOptions.exact, since activeOptions is optional and most links don't pass it, so the unguarded access throws on the common path.
Happy to switch if listing activeOptions itself is preferable. The cost is that callers passing an inline activeOptions={{ exact: true }} literal rebuild the selector on every render. That doesn't cause extra re-renders, since the comparator still returns the previous selection; it just re-runs buildLocation on renders that were happening anyway. Either trade works for me, just say which you'd rather have.
There was a problem hiding this comment.
it's ok to disable eslint locally if we can save some bytes (and we know that it is safe to do so)
There was a problem hiding this comment.
Can re-check this in a bit, just vendoring this into our internal test suite to check if it finds any regressions.
There was a problem hiding this comment.
Done in 07081d4. activeOptions now goes straight to resolveIsActive, with the dependency array listing the four fields and an exhaustive-deps disable above it.
Saves 269 bytes on each of dist/esm/link.js and dist/cjs/link.cjs, unminified.
Kept the ?. on the dependency entries, since activeOptions is undefined on most links and the unguarded access throws on that path.
The type is erased either way, but the object literal's property names survive minification and a tuple's positions don't — so this drops three property names from the selector's return plus the three property reads in compareLinkState. Measured on the unminified build: -54 bytes in dist/esm/link.js and the same in dist/cjs/link.cjs.
The render-count assertions proved the bail-out but not that the selector still publishes correct values, so a selector returning a constant could have passed. The test now also asserts that the link gaining active state carries `data-status="active"` afterwards (and does not beforehand), and that the unaffected link keeps its href and stays inactive. Checked by sabotaging the selector: returning a constant tuple with a wrong href but a correct active state now fails, where previously it passed. Also drops `as any` from `new URL(to)` in resolveExternalLink — the guard above already narrows `to` to string.
|
Pushed Both nitpicks are addressed: braces on the single-line On CodSpeed: most regressions are Solid and Vue benchmarks, and this PR only touches The four React |
yes memory benchmarks should be ignored, i haven't managed to stabilize them yet.
Some CPU benchmarks are more stable than others, so to take with a grain of salt. All the benchmarks can be run locally though, where you have more control over stability. That includes bundle size benchmarks and all the codspeed benchmarks (that just run on vitest locally). There is also |
Depends on the four fields rather than the object, with an exhaustive-deps disable: callers routinely pass an inline literal, which would otherwise rebuild the selector every render. resolveIsActive reads only those four fields, so the disable is not hiding a live dependency. -269 bytes on each of dist/esm/link.js and dist/cjs/link.cjs (unminified).
The `useMemo` chain this replaced keyed `getHrefOption` and the external-link resolution on the href string, so a navigation that left a link's href alone skipped both. Deriving everything in the selector ran them on every location notification instead, which showed up as a ~10% regression on the client-nav rewrites benchmark, where rewrite handling makes `getHrefOption` expensive. Cache both on the built href inside the selector closure. Measured on a five-link root layout, per navigation: getHrefOption drops from 5 calls back to 0, matching the pre-change profile, with buildLocation and the active-state derivation unchanged at 5.
Links commonly pass inline `params` / `search` object literals. Those change identity on every parent render, which rebuilt `_options`, which changed the store selector's identity, which discarded useSyncExternalStoreWithSelector's memoized selection. buildLocation then ran twice per navigation: once in the notification check and once in the render-phase selection. Measured on a replica of the client-nav rewrites scenario (six links, root subscribed to the pathname via useLocation), buildLocation per navigation: base 7, before this commit 12, after 7.
Reverts 409371b. It did cut getHrefOption from 5 calls per navigation to 0, matching the pre-change profile, but that is not where the time went: on the rewrites scenario it moved the number by 0.05% (medians 245.33 vs 245.21 hz over four interleaved rounds). Not worth ~15 lines of mutable closure state. The rewrites regression is fixed by the _options stabilisation instead.
Replaces a leftover scratch note.
|
Ran Two of the four React regressions ( The rewrites regression was real, and is fixedLinks commonly pass inline Measured on a replica of the
The arithmetic pins it: the one link with no inline props builds once, the five with inline props build twice. Fixed by keeping Current numbersMedians of three interleaved runs, hz, higher is better:
One regression left, and I have not explained it
The CodSpeed report above is staleThe workflow runs for the latest commits are sitting in Bundle sizeAgainst base, from Also dropped a commit along the way. I had added an href memoization that cut |
2 participants
Fixes #7951.
Problem
useLinkPropssubscribes every client-sideLinkto the location store with an identity selector:hrefandisActiveare then derived in downstream memos that listcurrentLocationin their dependencies. So the comparator can only ask "is this a different URL?", never "does this link care?" — and everyLinkon the page re-renders on every navigation, including the ones whose rendered output is identical before and after.On one page in our app (Mixcloud), 313 links persisted across a single navigation and exactly 1 changed its rendered output — the tab gaining
data-status="active". The other 312 re-rendered for nothing. Full measurements are in #7951.This is complementary to #2359 / #2516, which reduced the cost of each
Linkrender via theroutesByPathfast path. This reduces the number of renders. After #2516 each render is cheaper, but every link still renders on every navigation — the remainingbuildLocationtime measured at 15.8ms across 1563 calls, with the residue being the React render pass itself.Change
The location-derived values move into the selector, and
compareLinkStatecompares the three resulting primitives, so a link whosehrefand active state are unaffected by a navigation bails out.buildLocationstill runs once per link per location change. What goes away is the React render and the host reconciliation beneath it.Supporting details:
isActiveandexternalLinkbodies move to module-level helpers unchanged, so the selector stays readable.activeOptionsis spread into its four primitive fields in the selector's dependency list rather than depended on directly, because callers routinely pass an inline object literal — this matches what the previousisActivememo already did.doPreloadno longer pre-supplies_builtLocation, because the built location is no longer held in render state.preloadRoutealready falls back toopts._builtLocation ?? this.buildLocation(opts), andhandleClickhas always letrouter.navigatebuild its own. Net cost is one extrabuildLocationper hover.The subscribed-location pinning is preserved deliberately.
Linkpasses_fromLocation: currentLocation, and inbuildLocationthat is the head ofdest._fromLocation || this.pendingBuiltLocation || this.latestLocation— pinning resolution to the subscribed snapshot rather than one that can differ mid-transition. The selector formulation keeps that, because it derives from the value being published. A naive "subscribe to a boolean" would not, sincenextstill needs the location for relativetoresolution and param inheritance.Tests
tests/link.test.tsx→link re-render bail-out. It renders two memoized components that calluseLinkProps, navigates, and asserts that the link the navigation cannot affect does not re-render. The components are memoized so a re-render of the owning route component cannot be mistaken for the subscription firing, and the link options are module-stable for the same reason.Verified in both directions by reverting the source change and keeping the test:
AssertionError: expected 4 to be 2(one re-render, doubled by StrictMode)Existing suite, against the unmodified baseline on the same commit:
test:unittest:typestest:eslinttest:buildScope
Only
packages/react-router.packages/solid-routerandpackages/vue-routerhave the same shape and would want the same treatment — I have not touched them, so please don't assume they're covered. Happy to follow up on those if you'd like it done the same way.tests/link.bench.tsxexists and I did not run it; it looks like the natural home for a perf guard here if you want one.Summary by CodeRabbit