fix(router-core): fix handle AbortError logic when rapid navigation #6426
Conversation
Signed-off-by: leesb971204 <leesb971204@gmail.com>
📝 WalkthroughWalkthroughRefines loader abort/error handling to distinguish navigation-caused aborts and avoid spurious errorComponent renders; adds an end-to-end test reproducer that rapidly navigates parameterized routes to validate the fix. Changes
Sequence Diagram(s)mermaid Client->>RouterCore: navigate to /something/1 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
leesb971204
changed the title
|
View your CI Pipeline Execution ↗ for commit b08ca1e
☁️ Nx Cloud last updated this comment at |
![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
🤖 Fix all issues with AI agents
In `@packages/router-core/src/load-matches.ts`:
- Around line 783-792: In loadRouteMatch ensure any pending timeout set during
this load cycle is cleared before the early return: if you created a
pendingTimeout (e.g., pendingTimeout or inner.pendingTimeout) clear it with
clearTimeout(...) and reset the variable (e.g., pendingTimeout = undefined or
inner.pendingTimeout = undefined) immediately before returning when
matchAfterWait.status !== 'pending'; do this near the block that reads
matchAfterWait and calls handleRedirectAndNotFound so pending timers don't
persist and block future pending scheduling.
🧹 Nitpick comments (1)
packages/react-router/tests/loaders.test.tsx (1)
828-936: Tighten final-state assertions to avoid false positives.Right now the test passes as long as some param page renders and the loader completes. Consider asserting the final param value and expected loader completion id to make the repro more specific (Line 933-936).
♻️ Suggested test assertion enhancement
const paramPage = await screen.findByTestId('param-page') expect(paramPage).toBeInTheDocument() - expect(loaderCompleteMock).toHaveBeenCalled() + expect(paramPage).toHaveTextContent('Param Component 1') + expect(loaderCompleteMock).toHaveBeenCalledWith('1')
| const matchAfterWait = inner.router.getMatch(matchId)! | ||
| const error = matchAfterWait._nonReactive.error || matchAfterWait.error | ||
| if (error) { | ||
| handleRedirectAndNotFound(inner, match, error) | ||
| handleRedirectAndNotFound(inner, matchAfterWait, error) | ||
| } | ||
| } else { | ||
|
|
||
| if (matchAfterWait.status !== 'pending') { | ||
| return matchAfterWait | ||
| } | ||
| } |
There was a problem hiding this comment.
Clear pendingTimeout before the early return.
The new early return skips the cleanup block at the end of loadRouteMatch. If a pending timeout was set in this load cycle, it can remain attached and block future pending scheduling. Consider clearing it before returning (Line 789).
🧹 Suggested cleanup before return
if (matchAfterWait.status !== 'pending') {
+ clearTimeout(matchAfterWait._nonReactive.pendingTimeout)
+ matchAfterWait._nonReactive.pendingTimeout = undefined
return matchAfterWait
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const matchAfterWait = inner.router.getMatch(matchId)! | |
| const error = matchAfterWait._nonReactive.error || matchAfterWait.error | |
| if (error) { | |
| handleRedirectAndNotFound(inner, match, error) | |
| handleRedirectAndNotFound(inner, matchAfterWait, error) | |
| } | |
| } else { | |
| if (matchAfterWait.status !== 'pending') { | |
| return matchAfterWait | |
| } | |
| } | |
| const matchAfterWait = inner.router.getMatch(matchId)! | |
| const error = matchAfterWait._nonReactive.error || matchAfterWait.error | |
| if (error) { | |
| handleRedirectAndNotFound(inner, matchAfterWait, error) | |
| } | |
| if (matchAfterWait.status !== 'pending') { | |
| clearTimeout(matchAfterWait._nonReactive.pendingTimeout) | |
| matchAfterWait._nonReactive.pendingTimeout = undefined | |
| return matchAfterWait | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/router-core/src/load-matches.ts` around lines 783 - 792, In
loadRouteMatch ensure any pending timeout set during this load cycle is cleared
before the early return: if you created a pendingTimeout (e.g., pendingTimeout
or inner.pendingTimeout) clear it with clearTimeout(...) and reset the variable
(e.g., pendingTimeout = undefined or inner.pendingTimeout = undefined)
immediately before returning when matchAfterWait.status !== 'pending'; do this
near the block that reads matchAfterWait and calls handleRedirectAndNotFound so
pending timers don't persist and block future pending scheduling.
Signed-off-by: leesb971204 <leesb971204@gmail.com>
1 participant
fixes #6388
Summary
Problem
When rapidly navigating between parameterized routes (e.g., /something/1 → /something/2 → /something/1), the errorComponent was incorrectly triggered with a TypeError.
Root cause: The existing AbortError handler set status: 'success' without setting loaderData. When the component tried to access loaderData, it was undefined, causing a TypeError that was caught by the error boundary.
Solution
Distinguish between two types of AbortError by checking abortController.signal.aborted:
Additionally, after awaiting a previous loaderPromise, check if the match status is still 'pending'. If so, the previous loader was aborted and a fresh loader needs to run.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.