fix(solid-router): make navigation transitions work #5691
Conversation
WalkthroughAdds a new file-based Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Action
participant App as Route Component
participant RouterCore as Router Core
participant Transitioner as Solid Transitioner
participant Solid as Solid Runtime
participant Suspense as Manual Suspense
User->>App: click -> updateSearchParams()
App->>RouterCore: navigate/update
RouterCore->>Transitioner: call startTransition(fn)
Transitioner->>Solid: Solid.startTransition(fn)
rect rgb(210,230,250)
RouterCore->>RouterCore: compute exiting/entering/staying matches
RouterCore->>RouterCore: update cachedMatches / clearExpiredCache
RouterCore->>RouterCore: invoke onLeave/onEnter/onStay
end
Suspense->>Suspense: hold old UI (no fallback) during transition
Suspense->>App: when resources resolve -> render new UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 | 8m 7s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 24s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-02 13:30:06 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: |
birkskyum
changed the title
![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 (5)
e2e/solid-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/solid-router/basic-file-based/src/routes/transition/index.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/transition.spec.ts(1 hunks)packages/router-core/src/router.ts(2 hunks)packages/solid-router/src/Transitioner.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-router/basic-file-based/tests/transition.spec.tspackages/router-core/src/router.tspackages/solid-router/src/Transitioner.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/transition/index.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/transition/index.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.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/src/Transitioner.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-router/basic-file-based/src/routes/transition/index.tsx
🧠 Learnings (8)
📓 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-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:
e2e/solid-router/basic-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/transition/index.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/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/src/router.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/{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/Transitioner.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/transition/index.tsx
📚 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/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-file-based/src/routes/transition/index.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:
e2e/solid-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
PR: TanStack/router#5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/solid-router/basic-file-based/src/routes/transition/index.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 **/src/routes/** : Place file-based routes under src/routes/ directories
Applied to files:
e2e/solid-router/basic-file-based/src/routes/transition/index.tsx
🧬 Code graph analysis (2)
packages/router-core/src/router.ts (1)
packages/router-core/src/Matches.ts (1)
AnyRouteMatch(240-240)
packages/solid-router/src/Transitioner.tsx (1)
packages/solid-router/src/link.tsx (1)
isTransitioning(581-583)
⏰ 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
🔇 Additional comments (4)
packages/router-core/src/router.ts (2)
941-946: LGTM: Clean framework-agnostic extension point.The
wrapBatchhook provides a well-designed extension point for framework-specific transition handling. The default no-op implementation ensures backward compatibility while allowing frameworks like Solid to opt into custom batching behavior.Based on coding guidelines
2105-2141: LGTM: Correct integration of wrapBatch for transition-aware batching.The refactoring correctly hoists the match array declarations outside
wrapBatchso they can be accessed by the lifecycle hooks (lines 2143-2154). The batch operation and cache cleanup are properly wrapped, enabling framework-specific transition handling while preserving the existing state update logic.packages/solid-router/src/Transitioner.tsx (2)
64-71: LGTM: Proper async lifecycle management.The try/finally block correctly ensures that
isTransitioningis always reset, even if the navigation fails. The separation of concerns between tracking the transition state here and wrapping batch updates inwrapBatchis well-designed and clearly documented.Based on coding guidelines
38-60: No actionable concerns found.The jsdom detection in
wrapBatch(line 48) is part of a multi-layered guard strategy that works correctly. The approach has three safety checks:isRealBrowser && !router.isServer && isTransitioning(). The test suite validates this behavior in both jsdom (standard tests) and server environments (server mode tests), confirming the detection works as intended. The fallback behavior—directly executing the callback—is safe in all scenarios wherestartTransitionis skipped.
| const pollInterval = setInterval(async () => { | ||
| const text = await page.locator('body').textContent().catch(() => '') | ||
| if (text) bodyTexts.push(text) | ||
| }, 50) | ||
|
|
||
| // Click the increase button to trigger navigation with new search params | ||
| await page.getByTestId('increase-button').click() | ||
|
|
||
| // Wait a bit to capture text during the transition | ||
| await page.waitForTimeout(200) | ||
|
|
||
| clearInterval(pollInterval) | ||
|
|
||
| // Eventually, new values should appear | ||
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | ||
| timeout: 2000, | ||
| }) | ||
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | ||
| timeout: 2000, | ||
| }) |
There was a problem hiding this comment.
Guard against leaking the polling interval on failures.
If any assertion between starting the interval and calling clearInterval throws (for example, the click hangs), the timer keeps running against a disposed page. That leaves an active handle in the worker and can make later tests flaky or hang. Please ensure the interval is always cleared via try/finally.
Apply this diff:
- const pollInterval = setInterval(async () => {
- const text = await page.locator('body').textContent().catch(() => '')
- if (text) bodyTexts.push(text)
- }, 50)
-
- // Click the increase button to trigger navigation with new search params
- await page.getByTestId('increase-button').click()
-
- // Wait a bit to capture text during the transition
- await page.waitForTimeout(200)
-
- clearInterval(pollInterval)
+ const pollInterval = setInterval(async () => {
+ const text = await page.locator('body').textContent().catch(() => '')
+ if (text) bodyTexts.push(text)
+ }, 50)
+
+ try {
+ // Click the increase button to trigger navigation with new search params
+ await page.getByTestId('increase-button').click()
+
+ // Wait a bit to capture text during the transition
+ await page.waitForTimeout(200)
+ } finally {
+ clearInterval(pollInterval)
+ }📝 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 pollInterval = setInterval(async () => { | |
| const text = await page.locator('body').textContent().catch(() => '') | |
| if (text) bodyTexts.push(text) | |
| }, 50) | |
| // Click the increase button to trigger navigation with new search params | |
| await page.getByTestId('increase-button').click() | |
| // Wait a bit to capture text during the transition | |
| await page.waitForTimeout(200) | |
| clearInterval(pollInterval) | |
| // Eventually, new values should appear | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2000, | |
| }) | |
| const pollInterval = setInterval(async () => { | |
| const text = await page.locator('body').textContent().catch(() => '') | |
| if (text) bodyTexts.push(text) | |
| }, 50) | |
| try { | |
| // Click the increase button to trigger navigation with new search params | |
| await page.getByTestId('increase-button').click() | |
| // Wait a bit to capture text during the transition | |
| await page.waitForTimeout(200) | |
| } finally { | |
| clearInterval(pollInterval) | |
| } | |
| // Eventually, new values should appear | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2000, | |
| }) |
🤖 Prompt for AI Agents
In e2e/solid-router/basic-file-based/tests/transition.spec.ts around lines 17 to
36, the polling interval started before the click is not guaranteed to be
cleared if an assertion or click throws; wrap the actions that depend on the
interval (the click, waitForTimeout, and the subsequent assertions) inside a try
block and call clearInterval(pollInterval) in a finally block so the timer is
always cleared even on failures; ensure the finally runs before making further
cleanup or test teardown.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/solid-router/src/Transitioner.tsx (1)
38-52: Consider caching theisRealBrowsercheck for performance.The
isRealBrowsercomputation runs on everywrapBatchinvocation, which could be frequent during navigation. Since the browser environment doesn't change during the component lifecycle, consider computing this once during mount and storing it in a variable.Apply this diff to optimize:
const [isTransitioning, setIsTransitioning] = Solid.createSignal(false) + + const isRealBrowser = + typeof window !== 'undefined' && + typeof window.navigator !== 'undefined' && + typeof window.navigator.userAgent === 'string' && + !window.navigator.userAgent.includes('jsdom') router.wrapBatch = (fn: () => void) => { - const isRealBrowser = - typeof window !== 'undefined' && - typeof window.navigator !== 'undefined' && - typeof window.navigator.userAgent === 'string' && - !window.navigator.userAgent.includes('jsdom') - if (isRealBrowser && isTransitioning()) { Solid.startTransition(() => { fn() }) } else { fn() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/router.ts(2 hunks)packages/solid-router/src/Transitioner.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/src/Transitioner.tsxpackages/router-core/src/router.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/src/Transitioner.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/router.ts
🧠 Learnings (3)
📓 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/
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.
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/
📚 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/Transitioner.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/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/src/router.ts
🧬 Code graph analysis (2)
packages/solid-router/src/Transitioner.tsx (1)
packages/solid-router/src/link.tsx (1)
isTransitioning(581-583)
packages/router-core/src/router.ts (1)
packages/router-core/src/Matches.ts (1)
AnyRouteMatch(240-240)
⏰ 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 (4)
packages/solid-router/src/Transitioner.tsx (2)
23-23: Minor formatting change.The extra blank line improves readability.
54-61: Excellent error handling improvement.The try/finally block ensures
isTransitioningis always reset, even if the transition function throws. This prevents the router from getting stuck in a transitioning state.packages/router-core/src/router.ts (2)
949-954: Well-designed framework extension point.The
wrapBatchmethod provides a clean hook for framework-specific transition handling while keeping the core router framework-agnostic. The default no-op implementation and clear documentation make this easy for framework adapters to override.
2105-2141: Refactoring correctly enables framework-specific transition wrapping.The refactored logic wraps the batch operation in
wrapBatch, allowing framework adapters like Solid to inject transition handling around state updates. The match computation moved inside the wrapped context is necessary for Solid's reactivity tracking to work correctly.The closure correctly captures
exitingMatches,enteringMatches, andstayingMatchesfor use in lifecycle hooks after the batch completes.
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 (2)
packages/solid-router/src/Transitioner.tsx(2 hunks)packages/solid-start/src/useServerFn.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/solid-router/src/Transitioner.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-start/src/useServerFn.ts
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/solid-start/src/useServerFn.ts
🧠 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/
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.
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/
📚 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/solid-start/src/useServerFn.ts
⏰ 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
🔇 Additional comments (1)
packages/solid-start/src/useServerFn.ts (1)
21-26: Race condition concern is technically valid but requires architectural evaluation.Concurrent server function calls do occur in practice (confirmed via
Promise.allpatterns in e2e tests), and the shared_isServerFnRedirectflag can indeed be cleared prematurely by the first redirect'sfinally()block while other navigations are in progress.However, this pattern appears intentional for Solid's architecture—the comment states the flag exists to "skip Solid transitions" during server function redirects. React-start omits this flag entirely, suggesting different architectural needs between frameworks.
The suggested fix (passing flag as navigation options) requires verifying whether the router's navigation API supports this parameter. Alternatively, consider whether the race condition's actual impact (skipping the "skip transitions" behavior for concurrent redirects) warrants mitigation given the Solid-specific requirements.
| err.options._fromLocation = router.state.location | ||
| return router.navigate(router.resolveRedirect(err).options) | ||
| // Mark this as a server function redirect to skip Solid transitions | ||
| ;(router as any)._isServerFnRedirect = true |
There was a problem hiding this comment.
Type safety violation: avoid any type assertion in strict TypeScript.
Using (router as any) bypasses TypeScript's type checking and defeats the purpose of strict mode. This creates a maintenance risk as the router's API evolves.
Consider one of these approaches:
Option 1: Add the flag to navigation options (preferred)
- // Mark this as a server function redirect to skip Solid transitions
- ;(router as any)._isServerFnRedirect = true
- const result = router.navigate(router.resolveRedirect(err).options)
- // Clear the flag after navigation starts
- result.finally(() => {
- ;(router as any)._isServerFnRedirect = false
- })
- return result
+ // Mark this as a server function redirect to skip Solid transitions
+ const result = router.navigate({
+ ...router.resolveRedirect(err).options,
+ _isServerFnRedirect: true,
+ })
+ return resultOption 2: Properly type the router extension
interface RouterWithServerFnFlag {
_isServerFnRedirect?: boolean
}
// Then use:
(router as RouterWithServerFnFlag)._isServerFnRedirect = trueOption 1 is cleaner as it avoids shared mutable state and eliminates the need for the finally block.
🤖 Prompt for AI Agents
In packages/solid-start/src/useServerFn.ts around line 21 you assign a flag
using an any cast which breaks strict TypeScript; instead add a typed navigation
option (e.g., extend the existing navigate/redirect options type to include an
optional _isServerFnRedirect boolean) and pass that flag through the navigation
call so you can remove the (router as any) cast and the shared mutable
state/finally block; if you must keep the router mutation, declare a small
interface for the router extension with _isServerFnRedirect?: boolean and cast
to that interface rather than any.
6fc2d8b to
05672a2
Compare
birkskyum
force-pushed
the
solid-navigation-transition
branch
from
6b8192a to
29ffc5c
Compare
birkskyum
force-pushed
the
solid-navigation-transition
branch
2 times, most recently
from
e1ef229 to
39dc575
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/solid-router/tests/router.test.tsx (1)
1681-1685: Consider aligning this test with the similar loader test or clarifying the intent.The root route now includes a
notFoundComponent, but the similar test above (lines 1640-1676, "loader throws a notFound()") creates the root route without this option. Additionally, the specific route at lines 1700-1702 already defines its ownnotFoundComponentwith identical content.If this change is intentional to test fallback behavior or component precedence, consider adding a comment explaining the difference. Otherwise, for consistency and clarity, align both tests to use the same pattern.
</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/solid-router/tests/router.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/solid-router/tests/router.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/solid-router/tests/router.test.tsx
🧠 Learnings (5)
📓 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/
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.
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/
📚 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/solid-router/tests/router.test.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:
packages/solid-router/tests/router.test.tsx
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
PR: TanStack/router#5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/solid-router/tests/router.test.tsx
📚 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/solid-router/tests/router.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
birkskyum
force-pushed
the
solid-navigation-transition
branch
2 times, most recently
from
8087ee3 to
5c9998d
Compare
birkskyum
force-pushed
the
solid-navigation-transition
branch
from
5c9998d to
6b0d3e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
17-32: Always clear the polling interval.If the click or wait throws, the interval keeps running against a torn-down page, leading to flaky tests. Wrap the interaction in
try/finallyand clear the interval in thefinallyblock so it always tears down.- const pollInterval = setInterval(async () => { - const text = await page - .locator('body') - .textContent() - .catch(() => '') - if (text) bodyTexts.push(text) - }, 50) - - // Click the increase button to trigger navigation with new search params - await page.getByTestId('increase-button').click() - - // Wait a bit to capture text during the transition - await page.waitForTimeout(200) - - clearInterval(pollInterval) + const pollInterval = setInterval(async () => { + const text = await page + .locator('body') + .textContent() + .catch(() => '') + if (text) bodyTexts.push(text) + }, 50) + + try { + // Click the increase button to trigger navigation with new search params + await page.getByTestId('increase-button').click() + + // Wait a bit to capture text during the transition + await page.waitForTimeout(200) + } finally { + clearInterval(pollInterval) + }
🧹 Nitpick comments (2)
packages/solid-router/tests/link.test.tsx (2)
785-793: Good synchronization for navigation side-effects.The
waitForwrappers properly ensure that both the URL search parameters and DOM content have updated before assertions. This aligns with the PR's objective of handling navigation transitions correctly.Consider consolidating the pathname assertion at line 788 into the first
waitForblock for consistency:await waitFor(() => { expect(window.location.search).toBe('?page=2&filter=inactive') + expect(window.location.pathname).toBe('/posts') }) - expect(window.location.pathname).toBe('/posts')
900-910: Navigation synchronization is correctly implemented.The
waitForwrappers ensure navigation transitions complete before assertions, consistent with the fix for manual Suspense boundaries during navigation.For consistency, consider consolidating the pathname assertion at line 903 into the first
waitForblock:await waitFor(() => { expect(window.location.search).toBe('?page=2&filter=inactive') + expect(window.location.pathname).toBe('/Dashboard/posts') }) - expect(window.location.pathname).toBe('/Dashboard/posts')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/solid-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/solid-router/basic-file-based/src/routes/transition/index.tsx(1 hunks)e2e/solid-router/basic-file-based/tests/transition.spec.ts(1 hunks)packages/react-router/tests/store-updates-during-navigation.test.tsx(1 hunks)packages/router-core/src/router.ts(1 hunks)packages/solid-router/src/Match.tsx(1 hunks)packages/solid-router/src/Transitioner.tsx(3 hunks)packages/solid-router/tests/link.test.tsx(4 hunks)packages/solid-router/tests/optional-path-params.test.tsx(1 hunks)packages/solid-router/tests/router.test.tsx(1 hunks)packages/solid-router/tests/store-updates-during-navigation.test.tsx(8 hunks)packages/solid-router/tests/useNavigate.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/router-core/src/router.ts
- e2e/solid-router/basic-file-based/src/routes/transition/index.tsx
- packages/solid-router/tests/optional-path-params.test.tsx
- packages/solid-router/tests/useNavigate.test.tsx
- packages/react-router/tests/store-updates-during-navigation.test.tsx
- packages/solid-router/src/Match.tsx
🧰 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:
e2e/solid-router/basic-file-based/tests/transition.spec.tspackages/solid-router/tests/store-updates-during-navigation.test.tsxpackages/solid-router/src/Transitioner.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tspackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/router.test.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routeTree.gen.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/store-updates-during-navigation.test.tsxpackages/solid-router/src/Transitioner.tsxpackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/router.test.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: TanStack/router PR: 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/
Learnt from: nlynzaad
Repo: TanStack/router PR: 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.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 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:
e2e/solid-router/basic-file-based/tests/transition.spec.tspackages/solid-router/tests/store-updates-during-navigation.test.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tspackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/router.test.tsx
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/solid-router/basic-file-based/tests/transition.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/solid-router/basic-file-based/tests/transition.spec.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 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/tests/store-updates-during-navigation.test.tsxpackages/solid-router/src/Transitioner.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tspackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/router.test.tsx
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 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/solid-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 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:
e2e/solid-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 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/solid-router/tests/router.test.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 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/solid-router/tests/router.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/solid-router/tests/link.test.tsx (1)
396-399: LGTM! Proper synchronization for navigation transition.The addition of
waitForensures the navigation has completed and the link has acquired the 'active' class before proceeding with subsequent assertions. This properly handles the asynchronous nature of navigation transitions.
| router.startTransition = (fn: () => void | Promise<void>) => { | ||
| setIsTransitioning(true) | ||
| await fn() | ||
| setIsTransitioning(false) | ||
| Solid.startTransition(async () => { | ||
| try { | ||
| await fn() | ||
| } finally { | ||
| setIsTransitioning(false) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Don’t hand Solid.startTransition an async callback.
Solid’s startTransition expects a synchronous callback. Giving it an async function means the awaited part runs after Solid has already ended the transition, so updates escape the transition and errors become unhandled rejections. Run fn synchronously (capture its promise separately) and only await it outside the transition to keep semantics intact.
🤖 Prompt for AI Agents
In packages/solid-router/src/Transitioner.tsx around lines 38 to 47, the code
passes an async callback to Solid.startTransition which breaks transition
semantics; instead call Solid.startTransition with a synchronous function that
invokes fn() but does not await it, capture the returned promise in a local
variable, then await that promise outside the transition in a finally block that
resets setIsTransitioning(false) so the awaited work runs after Solid ends the
transition and any errors can be handled properly.
Maintain startTransition’s async contract.
router.startTransition used to resolve once the wrapped work finished. After this refactor it always returns void, so any callers still doing await router.startTransition(...) now resume immediately, which can break sequencing and let isTransitioning drop before the work finishes. Please restore the async return (e.g. by returning the Promise from Solid.startTransition).
🤖 Prompt for AI Agents
In packages/solid-router/src/Transitioner.tsx around lines 38 to 47,
router.startTransition currently returns void causing callers that await it to
resume immediately; restore the async contract by returning a Promise that
resolves when the wrapped work completes. Change the implementation to return
the Promise from Solid.startTransition (or wrap the async work in a new Promise)
so callers can await completion, ensure setIsTransitioning(true) is set before
starting and setIsTransitioning(false) in finally after the awaited work, and
propagate any errors to the returned Promise.
| }) | ||
|
|
||
| test('Router.preload="intent", pendingComponent renders during unresolved route loader', async () => { | ||
| test.skip('Router.preload="intent", pendingComponent renders during unresolved route loader', async () => { |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify why this critical test is skipped.
This test validates that pendingComponent renders during an unresolved route loader, which is directly related to the PR's objective of fixing navigation transitions with Suspense boundaries. Skipping it without explanation is concerning.
Please explain why this test is being skipped:
- Does the new navigation transition behavior intentionally change how
pendingComponentworks? - Is this a temporary skip while the implementation is in progress?
- Does the test need to be rewritten to match the new transition behavior?
If the behavior has changed intentionally, the test should be updated rather than skipped. If there's a regression, it should be fixed before merging.
🏁 Script executed:
#!/bin/bash
# Retrieve the test file content around line 4358
cat packages/solid-router/tests/link.test.tsx | sed -n '4340,4380p'Length of output: 1096
🏁 Script executed:
#!/bin/bash
# Search for any context around the skipped test - look for comments explaining why
sed -n '4350,4370p' packages/solid-router/tests/link.test.tsx | cat -ALength of output: 689
🏁 Script executed:
#!/bin/bash
# Search for TODO, FIXME, or skip-related comments in the test file
rg -n 'skip|TODO|FIXME' packages/solid-router/tests/link.test.tsx -A2 -B2 | head -50Length of output: 217
🏁 Script executed:
#!/bin/bash
# Check if there are any recent changes or comments mentioning pendingComponent
rg -n 'pendingComponent' packages/solid-router/tests/link.test.tsx -B3 -A3Length of output: 308
🏁 Script executed:
#!/bin/bash
# Get the full implementation of the skipped test
sed -n '4358,4420p' packages/solid-router/tests/link.test.tsxLength of output: 1768
🏁 Script executed:
#!/bin/bash
# Search for other pendingComponent references in tests
rg -n 'pendingComponent' packages/solid-router/tests/ -B2 -A2Length of output: 2016
🏁 Script executed:
#!/bin/bash
# Check source code for pendingComponent implementation
rg -n 'pendingComponent' packages/solid-router/src/ -B2 -A2Length of output: 1594
🏁 Script executed:
#!/bin/bash
# Look for any related changes or comments about navigation transitions
rg -n 'preload.*intent' packages/solid-router/tests/link.test.tsx -B3 -A3Length of output: 524
Remove test.skip() or add explanation for why this critical test is disabled.
This test validates that pendingComponent renders during navigation with preload="intent", which is core to the PR's objective. The test is complete and properly implemented, yet skipped without any comment explaining why.
If the behavior has intentionally changed, update the test expectations. If there's a known issue, document it with a TODO comment. If this is temporary, the test must be re-enabled before merging.
🤖 Prompt for AI Agents
In packages/solid-router/tests/link.test.tsx around line 4358, the test is
marked with test.skip which disables a critical test; either re-enable the test
by removing .skip, or if skipping is intentional, add a clear inline
comment/TODO explaining why (linking an issue or describing expected behavioral
change) and include a plan/date to restore it; if the behavior changed, update
the test expectations to match the new behavior instead of skipping.
closes #5636
Summary by CodeRabbit
New Features
Behavioral Changes
Tests