perf(react-router): add match selector compares#7596
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds equality comparators for Match/Outlet useStore subscriptions to avoid unnecessary rerenders and refactors OnRendered to derive and track a renderedLocationKey from resolvedLocation.state.__TSR_key for effect re-invocation. ChangesMatch subscription and OnRendered optimization
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 620e967
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview2 package(s) bumped directly, 7 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/react-router/src/Match.tsx`:
- Around line 34-35: The equality comparator matchViewFieldsEqual currently only
compares routeId and _displayPending, so changes to match.ssr are ignored;
update matchViewFieldsEqual (the function named matchViewFieldsEqual that takes
AnyRouteMatch a and b) to also compare a.ssr === b.ssr so it matches the useMemo
dependencies that include match.ssr and prevents stale matchState when ssr
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b3cf076-503f-439e-a25d-639ea300b66b
📒 Files selected for processing (1)
packages/react-router/src/Match.tsx
Merging this PR will improve performance by 6.28%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | client-side navigation loop (react) |
58.7 ms | 55.3 ms | +6.28% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/react-router-match-selector-compares (9926ac4) with main (52db703)
This reverts commit 620e967.
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We fixed the ESLint error introduced by this PR by moving the // eslint-disable-next-line react-hooks/rules-of-hooks directive to immediately precede the React.useRef call in OnRendered. Previously, the directive was placed before a block of multi-line comments, causing it to suppress the @ts-expect-error line instead of the hook call, which left React.useRef unguarded after the early return. This resolves the single blocking error in @tanstack/react-router:test:eslint.
Tip
✅ We verified this fix by re-running @tanstack/react-router:test:eslint.
diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx
index acecde90..3e37579c 100644
--- a/packages/react-router/src/Match.tsx
+++ b/packages/react-router/src/Match.tsx
@@ -247,13 +247,13 @@ function OnRendered() {
return null
}
- // eslint-disable-next-line react-hooks/rules-of-hooks
// @ts-expect-error -- init to `undefined` but don't write `undefined` to shave bytes
// Track the resolvedLocation as of the last render so that onRendered can
// report the correct fromLocation. By the time this effect fires,
// resolvedLocation has already been updated to the new location by
// Transitioner, so we cannot use router.stores.resolvedLocation.get()
// directly as the fromLocation.
+ // eslint-disable-next-line react-hooks/rules-of-hooks
const prevResolvedLocationRef = React.useRef<
ParsedLocation<any> | undefined
>()
Or Apply changes locally with:
npx nx-cloud apply-locally AsVF-7DDa
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Summary
Tests
Summary by CodeRabbit