feat: support lazy error component on beforeLoad failures #5568
Conversation
WalkthroughMultiple test files are restructured to support lazy error component testing through a new parameterization flag. The error handling in load-matches.ts is modified to ensure route chunks load before error processing. A formatting line is added to useNavigate.tsx. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant executeBeforeLoad
participant Chunk as loadRouteChunk
participant Error as handleSerialError
App->>executeBeforeLoad: Begin before load
executeBeforeLoad->>executeBeforeLoad: Execute route logic
rect rgb(200, 220, 255)
Note over executeBeforeLoad,Error: On Error (New Flow)
executeBeforeLoad->>Chunk: Await load route chunk
Chunk-->>executeBeforeLoad: Chunk loaded
executeBeforeLoad->>Error: Handle error with chunk ready
end
Error-->>App: Error handled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes mix formatting updates, substantial test restructuring with new lazy-loading patterns across two test files, and focused error-handling logic modifications. The heterogeneity of changes and test complexity warrant moderate review effort despite homogeneous patterns within test parameterizations. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 5m 36s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 24s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-23 10:52:37 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-router-ssr-query
@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: |
| RegisteredRouter, | ||
| UseNavigateResult, | ||
| } from '@tanstack/router-core' | ||
|
|
There was a problem hiding this comment.
This was causing the lint to fail, is that expected that it failed on main too ?
![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
♻️ Duplicate comments (1)
packages/react-router/src/useNavigate.tsx (1)
10-10: Formatting-only change; OK.No behavioral or type-safety impact. Resolves lint grouping between type/value imports.
🧹 Nitpick comments (4)
packages/react-router/tests/errorComponent.test.tsx (2)
111-121: Reduce timeout brittleness.The 1500ms/750ms timeouts are close to the 500ms async throw + lazy load overhead. Consider a slightly larger margin or
waitForwith a generous timeout to minimize flakes on CI.Also applies to: 158-167
82-90: Optional: mirror real-world lazy imports.Instead of
Promise.resolve(createLazyRoute(...)), using dynamic import better reflects production lazy behavior:- aboutRoute.lazy(() => - Promise.resolve( - createLazyRoute('/about')({ - errorComponent: MyErrorComponent, - }), - ), - ) + aboutRoute.lazy(async () => + createLazyRoute('/about')({ + errorComponent: MyErrorComponent, + }), + )(Same for indexRoute)
Also applies to: 139-146
packages/solid-router/tests/errorComponent.test.tsx (2)
101-110: Same timeout flake risk note as React tests.Consider slightly larger timeouts or
waitForto reduce CI flakiness.Also applies to: 147-156
74-81: Optional: mirror production lazy imports.Align lazy stubs with dynamic import style as in app code for closer fidelity.
- aboutRoute.lazy(() => - Promise.resolve( - createLazyRoute('/about')({ - errorComponent: MyErrorComponent, - }), - ), - ) + aboutRoute.lazy(async () => + createLazyRoute('/about')({ + errorComponent: MyErrorComponent, + }), + )(Same for indexRoute)
Also applies to: 128-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-router/src/useNavigate.tsx(1 hunks)packages/react-router/tests/errorComponent.test.tsx(2 hunks)packages/router-core/src/load-matches.ts(1 hunks)packages/solid-router/tests/errorComponent.test.tsx(2 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/solid-router/tests/errorComponent.test.tsxpackages/router-core/src/load-matches.tspackages/react-router/tests/errorComponent.test.tsxpackages/react-router/src/useNavigate.tsx
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/errorComponent.test.tsxpackages/react-router/tests/errorComponent.test.tsxpackages/react-router/src/useNavigate.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
🔇 Additional comments (2)
packages/react-router/tests/errorComponent.test.tsx (1)
42-91: Good coverage for lazy vs non‑lazy errorComponent.The parametric suite cleanly verifies both
.lazy()and staticerrorComponentpaths. Route setup mirrors real usage and asserts lazy error rendering correctly.packages/solid-router/tests/errorComponent.test.tsx (1)
33-81: Parity looks good; scenarios mirror React tests.Covers lazy/static errorComponent and navigate/first-load paths. Assertions are appropriate.
Yes that seems good |
…/github.com/beaussan/router into support-lazy-error-component-on-before-load
Great, thanks! Then I guess the pr is good to go from my point of view |
I do think we should at least make sure not to load the components if it's throwing a redirect. |
2 participants
Current behaviour: if you .lazy() an error component on a beforeLoad, then the error component won't be use and the default one will be instead
Reproduction of current error: https://stackblitz.com/edit/tanstack-router-wiqzauc7?file=src%2Fmain.tsx
Open questions:
I have the following test failing (most likely due to the loading of the lazy component):
Is that ok to bump to 8 ?
Summary by CodeRabbit
Release Notes