fix(router-core): Catch errors thrown during hydrate #5686
Conversation
WalkthroughA new test route Changes
Sequence DiagramsequenceDiagram
participant Router as Router
participant Head as Route Head Hook
participant NotFound as notFound()
participant Hydration as Route Context Hydration
participant Logger as Console/Error Logger
Router->>Head: run head()
Head->>NotFound: throw notFound()
NotFound-->>Hydration: mark match as isNotFound
Hydration->>Hydration: process match hydration (may reject)
Hydration->>Logger: .catch() → log "Error during route context hydration:" + error
Logger->>Router: log emitted (non-fatal)
Router->>Router: render notFoundComponent for the route
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/router-core/tests/hydrate.test.ts (3)
89-93: Consider more complete mock matches.The mock matches use a minimal structure with only
id,routeId,index, and_nonReactive. While this may be sufficient for the serialization adapter test, consider whether additional properties (e.g.,params,loaderDeps,context,abortController) are needed to accurately represent real matches and avoid potential runtime errors if thehydratefunction accesses these properties.
215-254: LGTM with suggestions for enhanced coverage.The test correctly validates that errors thrown during route context hydration (specifically from the
headhook) are caught and logged toconsole.errorrather than propagating. This aligns with the PR objective.Consider adding assertions to verify that:
- The
hydratefunction completes successfully despite the error (doesn't reject the promise)- Other matches continue to be processed when one match throws an error
13-255: Consider expanding test coverage for additional hydration scenarios.The current tests provide good coverage of core hydration mechanics and error handling. Consider adding tests for:
- CSP nonce meta tag handling
pendingMinMsand_forcePendinglogic for pending matches- SPA mode detection (
lastMatchIdmismatch)- Custom
router.options.hydratecallback invocation- Route chunk loading with
loadRouteChunkThese scenarios are exercised in the
hydrateimplementation but not currently validated by tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
e2e/react-start/basic/src/routeTree.gen.ts(10 hunks)e2e/react-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/react-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/react-start/basic/tests/not-found.spec.ts(3 hunks)e2e/react-start/basic/vite.config.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(10 hunks)e2e/solid-start/basic/src/routes/not-found/index.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/solid-start/basic/tests/not-found.spec.ts(3 hunks)packages/router-core/src/ssr/ssr-client.ts(1 hunks)packages/router-core/tests/hydrate.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxpackages/router-core/src/ssr/ssr-client.tse2e/react-start/basic/vite.config.tse2e/react-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/index.tsxpackages/router-core/tests/hydrate.test.tse2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/react-start/basic/src/routeTree.gen.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxe2e/solid-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routes/not-found/via-head.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/src/routes/not-found/index.tsxe2e/react-start/basic/vite.config.tse2e/react-start/basic/tests/not-found.spec.tse2e/solid-start/basic/src/routes/not-found/index.tsxe2e/solid-start/basic/tests/not-found.spec.tse2e/react-start/basic/src/routes/not-found/via-head.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/src/routes/not-found/via-head.tsxe2e/react-start/basic/src/routeTree.gen.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/ssr/ssr-client.tspackages/router-core/tests/hydrate.test.ts
🧬 Code graph analysis (5)
e2e/react-start/basic/src/routes/not-found/index.tsx (2)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)e2e/react-start/basic/src/routes/not-found/route.tsx (1)
Route(4-8)
e2e/solid-start/basic/src/routes/not-found/index.tsx (3)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
packages/router-core/tests/hydrate.test.ts (1)
packages/router-core/src/ssr/ssr-client.ts (2)
TsrSsrGlobal(17-28)hydrate(59-279)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (2)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
Route(3-15)
e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
🔇 Additional comments (21)
e2e/react-start/basic/vite.config.ts (1)
23-23: LGTM!The addition of
/not-found/via-headto the prerender exclusion list is correct and consistent with other not-found routes. Routes that triggernotFound()should not be prerendered.e2e/solid-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM!The via-head link addition follows the established pattern and uses proper Solid.js syntax (
class, signal accessorpreload()).e2e/react-start/basic/src/routes/not-found/index.tsx (1)
28-37: LGTM!The via-head link addition follows the established pattern and uses proper React syntax.
e2e/solid-start/basic/tests/not-found.spec.ts (2)
12-12: LGTM!The whitelisted error for route context hydration aligns with the PR's objective to improve SSR hydration error handling when
notFound()is thrown.
29-29: LGTM!Expanding the test matrices to include
'head'provides coverage for the new/not-found/via-headroute that triggersnotFound()during the head phase.Also applies to: 60-60
e2e/react-start/basic/tests/not-found.spec.ts (1)
12-12: LGTM!Test updates mirror the Solid implementation and properly extend coverage for the new via-head route with appropriate error whitelisting.
Also applies to: 29-29, 61-61
e2e/react-start/basic/src/routes/not-found/via-head.tsx (2)
3-15: Route correctly implements head-phase notFound() for testing.The route structure properly tests the scenario where
notFound()is thrown during theheadphase. ThenotFoundComponentwill be rendered instead of the regularcomponent.
17-23: RouteComponent exists for completeness.While
RouteComponentwon't render in practice (sincehead()always throwsnotFound()), it's appropriate to define it for test completeness. Thedata-serverattribute helps verify SSR/hydration behavior in tests.e2e/solid-start/basic/src/routes/not-found/via-head.tsx (1)
1-23: LGTM!The Solid implementation correctly mirrors the React version with appropriate framework-specific imports. The route properly tests
notFound()thrown during theheadphase.e2e/solid-start/basic/src/routeTree.gen.ts (1)
35-35: LGTM!The generated route tree correctly integrates the new
NotFoundViaHeadRoutewith proper imports, type definitions, and route registrations throughout the file.Also applies to: 167-171, 261-261, 295-295, 335-335, 375-375, 409-409, 448-448, 655-661, 765-765, 772-772
e2e/react-start/basic/src/routeTree.gen.ts (5)
173-177: Route constant definition is consistent.
idandpathuse '/via-head' and parent isNotFoundRouteRoute, matching sibling routes.
281-282: Types and public route maps updated coherently.Additions for '/not-found/via-head' are present and consistent across FileRoutesByFullPath, FileRoutesByTo, FileRoutesById, and FileRouteTypes unions.
Also applies to: 317-318, 358-359, 401-402, 437-438, 477-478
688-694: Module augmentation entry matches generator conventions.
fullPath,path,preLoaderRoute, andparentRoutealign with the new route; no discrepancies spotted.
819-820: Children wiring under NotFoundRouteRoute is correct.
NotFoundViaHeadRouteadded to both the interface and value object. Mirrors sibling route patterns.Also applies to: 826-827
37-37: Import and route wiring verified successfully.The source file
e2e/react-start/basic/src/routes/not-found/via-head.tsxexists and correctly exportsRoutewith aheadhook that callsnotFound(). The import in routeTree.gen.ts is correctly wired.packages/router-core/tests/hydrate.test.ts (6)
54-58: LGTM!The test correctly validates the error handling when
window.$_TSRis missing.
60-72: LGTM!The test correctly validates the error handling when
window.$_TSR.routeris missing.
118-138: LGTM!The test correctly validates that no transformer map is created when serialization adapters are empty, while initialization still completes.
140-160: LGTM!The test correctly validates that the manifest is propagated from
window.$_TSR.routerintorouter.ssr.
162-213: LGTM!The test correctly validates that matches are hydrated with server-provided loader data and status flags, including the case where some matches lack dehydrated data.
4-10: I need to examine what this test file is actually testing to determine if importing from react-router is correct.The review comment is incorrect and should be disregarded.
The utilities being imported (
createMemoryHistory,createRootRoute,createRoute,createRouter,notFound) are framework-specific APIs that belong in@tanstack/react-router, notpackages/router-core. These are standard React Router imports, not router-core utilities. OnlynotFoundhas a framework-agnostic version in router-core; the others are inherently React-specific and cannot be imported from router-core because they don't exist there.If the concern is about framework coupling in router-core tests, that's a separate architectural question worth discussing, but the specific directive to import from
packages/router-core/srcis not actionable because these utilities do not and should not exist there by design.Likely an incorrect or invalid review comment.
| }) | ||
| ).catch((err) => { | ||
| console.error('Error during route context hydration:', err) | ||
| }) |
There was a problem hiding this comment.
Risk of silent failures with indiscriminate error swallowing.
The catch handler logs all errors but doesn't distinguish between expected errors (e.g., notFound() thrown from head()) and unexpected errors (e.g., programming bugs). This means:
- All errors are swallowed: TypeErrors, ReferenceErrors, and other bugs will be silently ignored, making debugging difficult in production
- No state tracking: The router and matches have no indication that hydration failed, so downstream code may encounter undefined properties (e.g.,
match.meta,match.context) - Inconsistent state risk: Since
Promise.allrejects on the first failure, some matches may have hydrated successfully while others have not, creating an inconsistent application state
Consider these improvements:
- ).catch((err) => {
- console.error('Error during route context hydration:', err)
- })
+ ).catch((err) => {
+ console.error('Error during route context hydration:', err)
+ // Differentiate expected errors from unexpected ones
+ const isExpectedError = err?.isNotFound || err?.routeError
+ if (!isExpectedError) {
+ // For unexpected errors, consider updating router state to indicate hydration failure
+ router.__store.setState((s) => ({
+ ...s,
+ hydrationError: err,
+ }))
+ }
+ // Re-throw unexpected errors in development for easier debugging
+ if (process.env.NODE_ENV !== 'production' && !isExpectedError) {
+ throw err
+ }
+ })Alternatively, document this behavior if swallowing all errors is intentional for hydration resilience.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/ssr-client.ts around lines 219-222, the current
catch handler indiscriminately logs and swallows all errors from the hydration
Promise.all, which hides programming errors and leaves router/matches in
inconsistent state; change to handle expected control-flow errors (e.g.,
notFound()) separately and propagate unexpected errors: replace the single
Promise.all with Promise.allSettled or catch per-match errors so each match
records success/failure (set a flag on match like match.hydrationFailed and
attach error to match.context or match.error), log full error details for
unexpected errors and rethrow them (or surface them to the caller) so they are
not silently swallowed, and ensure the router state gets a hydration status flag
to indicate partial/failed hydration for downstream code to check.
|
|
||
| const routeTree = rootRoute.addChildren([indexRoute.addChildren([otherRoute])]) | ||
|
|
||
| mockRouter = createRouter({ routeTree, history, isServer: true }) |
There was a problem hiding this comment.
Verify the isServer: true flag for client hydration tests.
The hydrate function is designed for client-side hydration, yet the router is created with isServer: true. This contradicts the test scenario and may affect router behavior. Consider using isServer: false or omitting the flag entirely to accurately simulate the client environment.
🤖 Prompt for AI Agents
In packages/router-core/tests/hydrate.test.ts around line 46, the test creates
the router with isServer: true which contradicts the client-side hydration
scenario; update the router instantiation to use isServer: false (or remove the
flag) so the router is created as a client environment, ensuring hydrate runs
against client-mode behavior and any server-only branches are not triggered.
|
View your CI Pipeline Execution ↗ for commit f9ee4a5
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/router-core/src/ssr/ssr-client.ts (1)
220-222: Error handling concern already raised in previous review.The indiscriminate error swallowing in this catch handler has been comprehensively flagged in the previous review, including concerns about silent failures, lack of state tracking, and inconsistent router state.
packages/router-core/tests/hydrate.test.ts (1)
48-48: isServer flag concern already raised in previous review.The use of
isServer: truefor client-side hydration tests has been flagged in the previous review as contradicting the test scenario.
🧹 Nitpick comments (2)
packages/router-core/tests/hydrate.test.ts (2)
1-11: Consider importing from source instead of dist.The import on lines 4-10 uses
../../react-router/dist/esm, which is unusual for unit tests. Typically, tests import from source files to ensure they're testing the actual implementation rather than the built artifacts.Additionally, ESLint flags import ordering issues that could be addressed.
Apply this diff to import from source and fix ordering:
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + notFound, +} from '../../react-router/src' import { hydrate } from '../src/ssr/ssr-client' import type { TsrSsrGlobal } from '../src/ssr/ssr-client' -import { - createMemoryHistory, - createRootRoute, - createRoute, - createRouter, - notFound, -} from '../../react-router/dist/esm' import type { AnyRouteMatch } from '../src'
217-259: Consider expanding error handling test coverage.The test validates that
notFound()errors during hydration are caught and logged. However, it only covers one error type and doesn't verify:
- Router state remains consistent after the error
- Hydration completes successfully despite the error
- Other error types (e.g., TypeError, ReferenceError) are handled similarly
Consider adding test cases for:
it('should handle unexpected errors during route context hydration', async () => { // Test TypeError, ReferenceError, etc. }) it('should continue hydration after error in one match', async () => { // Verify that other matches complete hydration successfully })Additionally, verify that the router state is checked after error handling to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/react-start/basic/src/routes/not-found/via-head.tsx(1 hunks)e2e/solid-start/basic/src/routes/not-found/via-head.tsx(1 hunks)packages/router-core/src/ssr/ssr-client.ts(1 hunks)packages/router-core/tests/hydrate.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-start/basic/src/routes/not-found/via-head.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/tests/hydrate.test.tse2e/react-start/basic/src/routes/not-found/via-head.tsxpackages/router-core/src/ssr/ssr-client.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/hydrate.test.tspackages/router-core/src/ssr/ssr-client.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic/src/routes/not-found/via-head.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic/src/routes/not-found/via-head.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
PR: TanStack/router#5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/hydrate.test.tse2e/react-start/basic/src/routes/not-found/via-head.tsxpackages/router-core/src/ssr/ssr-client.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/tests/hydrate.test.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/tests/hydrate.test.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/basic/src/routes/not-found/via-head.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
PR: TanStack/router#0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Applied to files:
e2e/react-start/basic/src/routes/not-found/via-head.tsx
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
PR: TanStack/router#5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
packages/router-core/src/ssr/ssr-client.ts
🧬 Code graph analysis (2)
packages/router-core/tests/hydrate.test.ts (1)
packages/router-core/src/ssr/ssr-client.ts (2)
TsrSsrGlobal(17-28)hydrate(59-279)
e2e/react-start/basic/src/routes/not-found/via-head.tsx (1)
e2e/react-start/basic/src/routes/not-found/index.tsx (1)
Route(3-41)
🪛 ESLint
packages/router-core/tests/hydrate.test.ts
[error] 1-1: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 4-10: ../../react-router/dist/esm import should occur before type import of ../src/ssr/ssr-client
(import/order)
🔇 Additional comments (4)
packages/router-core/tests/hydrate.test.ts (1)
56-215: Comprehensive test coverage for core hydration scenarios.These tests effectively cover:
- Missing bootstrap data validation
- Serialization adapter initialization and buffer execution
- Manifest propagation to router.ssr
- Match hydration with server-provided data
The test structure is clear and assertions are appropriate.
e2e/react-start/basic/src/routes/not-found/via-head.tsx (3)
1-1: LGTM!The imports are correct and appropriate for creating a file-based route that tests
notFound()behavior.
17-23: LGTM!The
RouteComponentis well-implemented with appropriate test attributes. Thedata-server={typeof window}is a clever way to distinguish between server and client rendering contexts. While this component should not actually render (since theheadhook always throwsnotFound()), it serves as a valid test fixture to ensure thenotFoundComponenttakes precedence.
3-15: Implementation correctly matches all test expectations.The route implementation is correct:
- The
headhook throwsnotFound()immediately, preventingRouteComponentfrom rendering- The
notFoundComponenthasdata-testid="via-head-notFound-component"matching test expectations for visibility- The
RouteComponenthasdata-testid="via-head-route-component"matching test expectations for non-visibility- Both navigation and direct visit scenarios in the test suite will pass
- The hydration error
'Error during route context hydration: {isNotFound: true}'is properly whitelisted in the tests
|
Hi @schiller-manuel, do you need me to do anything more for this PR to merge? 🙏 |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests