fix(react-router): clear stale route errors on navigation#7136
fix(react-router): clear stale route errors on navigation#7136schiller-manuel merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughA bug fix for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 15d2903
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 9 bumped as dependents. 🟩 Patch bumps
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-router/src/CatchBoundary.tsx`:
- Around line 41-48: getDerivedStateFromProps is using unsafe any types and the
state's resetKey is mis-typed as string; change the method signature to use
proper types (props: { getResetKey: () => string | number } and state: {
resetKey?: string | number; error: Error | null } or the existing
CatchBoundaryState type updated to make resetKey optional and accept
number|string), and update the state type declaration so resetKey is optional
(resetKey?: string | number) to match the async initialization pattern where
initial state may omit resetKey; ensure getDerivedStateFromProps returns {
resetKey } or { resetKey, error: null } with the correctly typed resetKey.
🪄 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: 68b82fc8-adde-418f-ae5e-f79114770e4f
📒 Files selected for processing (3)
.changeset/ten-cougars-jump.mdpackages/react-router/src/CatchBoundary.tsxpackages/react-router/tests/errorComponent.test.tsx
| static getDerivedStateFromProps(props: any, state: any) { | ||
| const resetKey = props.getResetKey() | ||
|
|
||
| if (state.error && state.resetKey !== resetKey) { | ||
| return { resetKey, error: null } | ||
| } | ||
|
|
||
| return { resetKey } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the CatchBoundary.tsx file to see the current state
cd /repo && find . -name "CatchBoundary.tsx" -type f | head -5Repository: TanStack/router
Length of output: 114
🏁 Script executed:
# Check the full CatchBoundary component, especially lines 41-48 and state definition
cat -n packages/react-router/src/CatchBoundary.tsx | head -100Repository: TanStack/router
Length of output: 3439
🏁 Script executed:
# Now check Match.tsx to see how resetKey is derived from loadedAt at lines 74-99
cat -n packages/react-router/src/Match.tsx | sed -n '60,110p'Repository: TanStack/router
Length of output: 1980
🏁 Script executed:
# Search for the type definition of loadedAt in the router store
rg "loadedAt" --type ts --type tsx -A 2 -B 2 | head -50Repository: TanStack/router
Length of output: 86
🏁 Script executed:
# Search for loadedAt type definition with proper TypeScript file type
rg "loadedAt" -A 2 -B 2 --type ts | head -80Repository: TanStack/router
Length of output: 4975
🏁 Script executed:
# Also check the type signature of the store to see what loadedAt should be
rg "loadedAt" --type ts --glob "*.ts" -A 1 -B 1 | grep -E "(loadedAt|:.*number|:.*string)" | head -20Repository: TanStack/router
Length of output: 552
Type the reset-key path instead of erasing it with any.
The getDerivedStateFromProps method on line 41 uses any types, bypassing compiler checks. Additionally, resetKey is declared as string in the state type (line 39), but the prop signature and actual usage show it should be number | string (as returned by getResetKey and sourced from router.stores.loadedAt, which is typed as number). Since the initial state { error: null } does not include resetKey, it should be optional in the type to reflect the async initialization pattern in class components.
♻️ Suggested typing fix
- state = { error: null } as { error: Error | null; resetKey: string }
+ state = { error: null } as {
+ error: Error | null
+ resetKey?: number | string
+ }
- static getDerivedStateFromProps(props: any, state: any) {
+ static getDerivedStateFromProps(
+ props: { getResetKey: () => number | string },
+ state: { error: Error | null; resetKey?: number | string },
+ ) {
const resetKey = props.getResetKey()
if (state.error && state.resetKey !== resetKey) {
return { resetKey, error: null }
}
return { resetKey }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router/src/CatchBoundary.tsx` around lines 41 - 48,
getDerivedStateFromProps is using unsafe any types and the state's resetKey is
mis-typed as string; change the method signature to use proper types (props: {
getResetKey: () => string | number } and state: { resetKey?: string | number;
error: Error | null } or the existing CatchBoundaryState type updated to make
resetKey optional and accept number|string), and update the state type
declaration so resetKey is optional (resetKey?: string | number) to match the
async initialization pattern where initial state may omit resetKey; ensure
getDerivedStateFromProps returns { resetKey } or { resetKey, error: null } with
the correctly typed resetKey.
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
|
In |
Agent-Logs-Url: https://github.com/TanStack/router/sessions/1367dbce-0eb5-4a4b-8fbf-995ce8479c3c Co-authored-by: schiller-manuel <6340397+schiller-manuel@users.noreply.github.com>
fixes #7121
Summary by CodeRabbit
Bug Fixes
Tests