fix(router-core): handle AbortError in router execution flow #4570
Conversation
Signed-off-by: leesb971204 <[email protected]>
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 4m 12s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 22s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-24 02:28:15 UTC
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
packages/router-core/src/router.ts
Outdated
| if ( | ||
| e instanceof DOMException && | ||
| e.name === 'AbortError' | ||
| ) { | ||
| const head = await executeHead() | ||
| updateMatch(matchId, (prev) => ({ | ||
| ...prev, | ||
| status: | ||
| prev.status === 'pending' | ||
| ? 'success' | ||
| : prev.status, | ||
| ...head, | ||
| })) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
According to the documentation, the abortController.abort() method throws an Error object of type DOMException with the name AbortError.
When this error is thrown during route loading, it is treated as a normal control flow and the route is updated to a valid state accordingly.
There was a problem hiding this comment.
I noticed you’ve explored similar concerns before(#4531), so I think it would be great to discuss this approach together.
…asepath Signed-off-by: leesb971204 <[email protected]>
|
Would there be a chance for us to have a conversation about this? |
There was a problem hiding this comment.
Sorry about that!
@schiller-manuel there were recent changes this flow if I'm not mistaken with the selective SSR stuff. Would this be an issue?
@leesb971204 provided Manuel's fine with these changes, I don't see a reason not to get this pushed in.
Edit: Also, please make sure that your test are reflected in @tanstack/solid-router as well.
…asepath Signed-off-by: leesb971204 <[email protected]>
Thanks for the comment! |
Are there any other tasks that need to be completed before this work can be approved? |
Other than the current merge conflict, as mentioned here, I'd like @schiller-manuel to take a look at how this is being handled. |
|
Is there any particular reason this work hasn’t been reviewed yet? |
|
no, we are just busy with a lot of stuff, sorry. will get back to you as soon as time permits |
WalkthroughAdds AbortError handling in loader error flow to prevent error propagation on initial load with basepath. Executes head, preserves/updates match status, and returns early. Adds React and Solid tests reproducing AbortError during basepath mount and asserting no error UI and correct path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant Router
participant RouteLoader as Loader
participant Head as executeHead
participant UI as ErrorBoundary
User->>App: Open "/"
App->>Router: Initialize with basepath "/app"
Router->>Loader: Run loader (initial load)
note right of Loader: Abort due to basepath redirect
Loader-->>Router: Throw DOMException(name="AbortError")
alt AbortError
Router->>Head: executeHead()
Head-->>Router: headResult
note over Router: Merge head, preserve/resolve match status
Router-->>App: Render route content (no error)
App-->>User: UI shows index under "/app"
else Other errors
Router->>UI: Propagate to error boundary
UI-->>User: Error UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/tests/loaders.test.tsx(2 hunks)packages/router-core/src/load-matches.ts(1 hunks)packages/solid-router/tests/loaders.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/load-matches.tspackages/solid-router/tests/loaders.test.tsxpackages/react-router/tests/loaders.test.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/load-matches.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/tests/loaders.test.tsxpackages/react-router/tests/loaders.test.tsx
🧬 Code graph analysis (1)
packages/react-router/tests/loaders.test.tsx (1)
packages/react-router/src/router.ts (1)
createRouter(80-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/react-router/tests/loaders.test.tsx (2)
433-433: Non-functional className reorder — OKNo behavior change; safe.
733-757: Good coverage for AbortError + basepathTest correctly asserts no error UI and basepath prefix.
Optionally also assert
router.state.location.href.startsWith('/app')for stronger verification.packages/solid-router/tests/loaders.test.tsx (1)
321-345: Solid test parity — looks goodMirrors React test; assertions are appropriate.
Optionally assert
router.state.location.href.startsWith('/app')for symmetry with the React suite.
| if (error instanceof DOMException && error.name === 'AbortError') { | ||
| const head = await executeHead(inner, matchId, route) | ||
| inner.updateMatch(matchId, (prev) => ({ | ||
| ...prev, | ||
| status: prev.status === 'pending' ? 'success' : prev.status, | ||
| ...head, | ||
| })) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
DOMException check can crash in SSR; also ensure pending-min duration and reset isFetching on async path
error instanceof DOMExceptionwill throw ifDOMExceptionis undefined (some SSR/envs).- Early return skips awaiting
minPendingPromise, potentially violating pendingMinMs guarantees. - In background reloads (
loaderShouldRunAsync),isFetchingcan remain'loader'after abort since onlyrunLoadercan flip it tofalse.
Fix by detecting abort via name === 'AbortError', awaiting min-pending, and explicitly setting isFetching: false.
- if (error instanceof DOMException && error.name === 'AbortError') {
- const head = await executeHead(inner, matchId, route)
- inner.updateMatch(matchId, (prev) => ({
- ...prev,
- status: prev.status === 'pending' ? 'success' : prev.status,
- ...head,
- }))
- return
- }
+ // Treat aborted loaders as cancellations, not errors
+ if ((error as any)?.name === 'AbortError') {
+ const head = await executeHead(inner, matchId, route)
+ const pendingPromise = match._nonReactive.minPendingPromise
+ if (pendingPromise) await pendingPromise
+ inner.updateMatch(matchId, (prev) => ({
+ ...prev,
+ status: prev.status === 'pending' ? 'success' : prev.status,
+ isFetching: false,
+ ...head,
+ }))
+ return
+ }📝 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.
| if (error instanceof DOMException && error.name === 'AbortError') { | |
| const head = await executeHead(inner, matchId, route) | |
| inner.updateMatch(matchId, (prev) => ({ | |
| ...prev, | |
| status: prev.status === 'pending' ? 'success' : prev.status, | |
| ...head, | |
| })) | |
| return | |
| } | |
| // Treat aborted loaders as cancellations, not errors | |
| if ((error as any)?.name === 'AbortError') { | |
| const head = await executeHead(inner, matchId, route) | |
| const pendingPromise = match._nonReactive.minPendingPromise | |
| if (pendingPromise) await pendingPromise | |
| inner.updateMatch(matchId, (prev) => ({ | |
| ...prev, | |
| status: prev.status === 'pending' ? 'success' : prev.status, | |
| isFetching: false, | |
| ...head, | |
| })) | |
| return | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/load-matches.ts around lines 673–682, replace the
fragile `error instanceof DOMException && error.name === 'AbortError'` check
with a safe abort detection using `error?.name === 'AbortError'` (or equivalent
null-safe check) so SSR environments without DOMException don't throw; before
returning, await the existing min-pending promise used elsewhere (e.g., `await
minPendingPromise`) to honor pendingMinMs guarantees; and call
`inner.updateMatch(matchId, ...)` to explicitly set `isFetching: false` (and
keep the current status transition logic: set status to 'success' only if
previously 'pending') so background loader aborts don't leave `isFetching` stuck
as 'loader'.
|
The tests can only pass after #5202 has been merged. |
3 participants
fixes #4517
Summary by CodeRabbit
Bug Fixes
Tests