fix(react-router,solid-router): top-level Matches should use pendingComponent on root route
#5495
Conversation
…defined on the __root__ route fixes #2317
WalkthroughResolve the root route's Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Router
participant Root as __root__
participant Child
rect rgba(135,206,250,0.12)
Note over Router,Root: Resolve PendingComponent
Router->>Root: lookup router.routesById[rootRouteId]
Root-->>Router: root.options.pendingComponent (or undefined)
Router->>Router: PendingComponent = root.pendingComponent || defaultPendingComponent
end
App->>Router: mount / navigate
Router->>Root: run loader (async)
alt PendingComponent exists
Router->>App: render <PendingComponent/> as Suspense fallback
else
Router->>App: render null fallback
end
Root-->>Router: loader resolves
Router->>Child: render child/root content
Router->>App: update UI with resolved content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 6fc6e2b
☁️ Nx Cloud last updated this comment at |
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: |
![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
🧹 Nitpick comments (1)
packages/solid-router/src/Matches.tsx (1)
49-52: Consider simplifying rootRoute to a direct assignment.The function wrapper
() => router.routesById['__root__']provides no reactivity benefit here since it's only called once when computingPendingComponentat the top level. Direct assignment would be clearer and consistent with the React implementation.Apply this diff to simplify:
- const rootRoute: () => AnyRoute = () => router.routesById['__root__'] + const rootRoute: AnyRoute = router.routesById['__root__'] const PendingComponent = - rootRoute().options.pendingComponent ?? + rootRoute.options.pendingComponent ?? router.options.defaultPendingComponent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/src/Matches.tsx(2 hunks)packages/solid-router/src/Matches.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/src/Matches.tsxpackages/react-router/src/Matches.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/src/Matches.tsxpackages/react-router/src/Matches.tsx
🧠 Learnings (2)
📓 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/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
📚 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:
packages/solid-router/src/Matches.tsx
🧬 Code graph analysis (2)
packages/solid-router/src/Matches.tsx (1)
packages/router-core/src/route.ts (1)
AnyRoute(778-797)
packages/react-router/src/Matches.tsx (1)
packages/router-core/src/route.ts (1)
AnyRoute(778-797)
🔇 Additional comments (3)
packages/react-router/src/Matches.tsx (2)
15-15: LGTM: Type import added correctly.The
AnyRoutetype import is necessary for typing the root route.
45-50: LGTM: Root route pending component resolution implemented correctly.The implementation correctly resolves the pending component from the root route's options before falling back to the router's default pending component. This fixes the issue where the pending component defined on
__root__was not being used.The nullish coalescing operator ensures proper fallback behavior, and the
pendingElementcorrectly renders the component or null.packages/solid-router/src/Matches.tsx (1)
59-59: LGTM: Pending component resolution implemented correctly.The fallback correctly uses the resolved
PendingComponent, which now properly prioritizes the root route's pending component before falling back to the router's default. This fixes the issue where the pending component defined on__root__was not being displayed.
|
|
||
| export function Matches() { | ||
| const router = useRouter() | ||
| const rootRoute: AnyRoute = router.routesById['__root__'] |
There was a problem hiding this comment.
do we need to be defensive here and type this as AnyRoute | undefined. I think a __root__ route must always exist ...
There was a problem hiding this comment.
please use rootRouteId from @tanstack/router-core instead of __root__
There was a problem hiding this comment.
and to answer your question, the answer is probably not, since a root route must always be present, but if it helps silence typescript then a ! assertion or a throw Error should be fine.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/solid-router/tests/Matches.test.tsx (1)
225-247: LGTM! Test correctly verifies root route's pendingComponent is displayed.The test successfully validates that the root route's
pendingComponentis shown during the loader execution and that the index route renders after loading completes.Optional enhancement: Consider adding an assertion to verify the pending component is no longer present after loading completes, and that the
defaultPendingComponentwas never used:expect(await rendered.findByText('root pending...')).toBeInTheDocument() expect(await rendered.findByText('index route')).toBeInTheDocument() + expect(rendered.queryByText('root pending...')).not.toBeInTheDocument() + expect(rendered.queryByText('default pending...')).not.toBeInTheDocument()This would make the test more explicit about the expected behavior and provide better coverage of the pending component lifecycle.
packages/react-router/tests/Matches.test.tsx (1)
125-147: LGTM! Test correctly verifies root route's pendingComponent is displayed.The test successfully validates that the root route's
pendingComponentis shown during the loader execution and that the index route renders after loading completes.Optional enhancement: Consider adding an assertion to verify the pending component is no longer present after loading completes, and that the
defaultPendingComponentwas never used:expect(await rendered.findByText('root pending...')).toBeInTheDocument() expect(await rendered.findByText('index route')).toBeInTheDocument() + expect(rendered.queryByText('root pending...')).not.toBeInTheDocument() + expect(rendered.queryByText('default pending...')).not.toBeInTheDocument()This would make the test more explicit about the expected behavior and provide better coverage of the pending component lifecycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/Matches.test.tsx(1 hunks)packages/solid-router/tests/Matches.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/Matches.test.tsxpackages/solid-router/tests/Matches.test.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/react-router/tests/Matches.test.tsxpackages/solid-router/tests/Matches.test.tsx
🧬 Code graph analysis (1)
packages/solid-router/tests/Matches.test.tsx (1)
packages/solid-router/src/RouterProvider.tsx (1)
RouterProvider(44-53)
⏰ 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). (1)
- GitHub Check: Test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/tests/Matches.test.tsx (1)
125-143: Consider adding an assertion to verifydefaultPendingComponentis not rendered.The test correctly verifies that the root route's
pendingComponentis shown, but it would be more comprehensive to also assert that the router'sdefaultPendingComponent("default pending...") is NOT shown. This would explicitly validate the precedence behavior being fixed.Add this assertion after rendering but before the first
findByText:const rendered = render(<RouterProvider router={router} />) + expect(rendered.queryByText('default pending...')).not.toBeInTheDocument() expect(await rendered.findByText('root pending...')).toBeInTheDocument() expect(await rendered.findByText('root content')).toBeInTheDocument()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/Matches.test.tsx(1 hunks)packages/solid-router/tests/Matches.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/tests/Matches.test.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/Matches.test.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/react-router/tests/Matches.test.tsx
⏰ 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). (1)
- GitHub Check: Test
|
|
||
| test('should show pendingComponent of root route', async () => { | ||
| const root = createRootRoute({ | ||
| pendingComponent: () => <div>root pending...</div>, |
There was a problem hiding this comment.
please use data-testid and getByTestId or similar methods instead of looking for textual matches
SeanCassiere
changed the title
Matches should use pendingComponent on root route
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/solid-router/src/Matches.tsx (1)
49-52: Consider simplifyingrootRouteto a direct reference.The
rootRouteis defined as a function but only called once to derivePendingComponent. SincePendingComponentis not wrapped in a reactive primitive (likecreateMemo), it won't update reactively anyway. Additionally, consider adding optional chaining for defensive coding in case the root route is unexpectedly missing.Apply this diff to simplify:
- const rootRoute: () => AnyRoute = () => router.routesById[rootRouteId] - const PendingComponent = - rootRoute().options.pendingComponent ?? - router.options.defaultPendingComponent + const rootRoute = router.routesById[rootRouteId] + const PendingComponent = + rootRoute?.options.pendingComponent ?? + router.options.defaultPendingComponent
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/react-router/src/Matches.tsx(3 hunks)packages/react-router/tests/Matches.test.tsx(1 hunks)packages/solid-router/src/Matches.tsx(3 hunks)packages/solid-router/tests/Matches.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-router/src/Matches.tsx
- packages/react-router/tests/Matches.test.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/tests/Matches.test.tsxpackages/solid-router/src/Matches.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/Matches.test.tsxpackages/solid-router/src/Matches.tsx
🧠 Learnings (1)
📚 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:
packages/solid-router/src/Matches.tsx
🧬 Code graph analysis (2)
packages/solid-router/tests/Matches.test.tsx (1)
packages/solid-router/src/RouterProvider.tsx (1)
RouterProvider(44-53)
packages/solid-router/src/Matches.tsx (1)
packages/router-core/src/route.ts (1)
AnyRoute(778-797)
⏰ 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/solid-router/src/Matches.tsx (2)
3-3: LGTM! Imports support the root route pending component resolution.The new imports of
rootRouteIdandAnyRouteare necessary for accessing and typing the root route to resolve itspendingComponent.Also applies to: 12-12
49-59: Excellent fix! Root route'spendingComponentnow renders correctly.The implementation correctly prioritizes the root route's
pendingComponentover the router'sdefaultPendingComponentand uses it as the Suspense fallback. This resolves the issue where the root route's pending component was unmounting prematurely.packages/solid-router/tests/Matches.test.tsx (1)
225-244: LGTM! Test validates root routependingComponentbehavior.The test correctly verifies that:
- A
pendingComponentcan be specified on the root route- The pending component renders while the root loader executes
- The root content appears after the loader completes
The test structure properly exercises the fix for issue #2317.
3 participants
fixes #2317
Summary by CodeRabbit
New Features
Bug Fixes
Tests