fix(solid-start): navigation transitions #5792
Conversation
|
Warning Rate limit exceeded@birkskyum has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughWraps route-match updates in a transition boundary and surfaces redirect/not-found errors non-reactively on matches; updates loader context merging and error propagation in the load pipeline. Adds an e2e Playwright test for transition behavior and adjusts multiple test expectations for update counts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page
participant RouterCore as Router Core
participant Transition as startTransition
participant Loaders as Loader Pipeline
participant UI
User->>Page: trigger navigation / click
Page->>RouterCore: updateMatch(request)
RouterCore->>Transition: wrap mutation in startTransition
Note right of Transition: isPending = true (hold old UI)
Transition->>RouterCore: perform match mutation (deferred commit)
RouterCore->>Loaders: loadRouteMatch / getLoaderContext
Loaders->>RouterCore: set match._nonReactive.error if redirect/notFound
RouterCore->>UI: commit updates when ready
Note right of UI: old values remain visible until transition completes
Transition->>UI: isPending = false (show new values)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
View your CI Pipeline Execution ↗ for commit 705b952
☁️ 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-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: |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts(8 hunks)e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx(2 hunks)e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsx(2 hunks)e2e/solid-router/basic-file-based/tests/transition.spec.ts(4 hunks)e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts(8 hunks)e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx(3 hunks)e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(11 hunks)e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx(1 hunks)e2e/solid-start/basic/tests/transition.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-start/basic/tests/transition.spec.tse2e/solid-router/basic-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsxe2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/react-router/basic-react-query-file-based/tests/transition.spec.tse2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/tests/transition.spec.tse2e/solid-router/basic-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsxe2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/react-router/basic-react-query-file-based/tests/transition.spec.tse2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts
**/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/count/create-resource.tsxe2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx
🧠 Learnings (6)
📓 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.
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/
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-cli,router-generator,router-plugin,virtual-file-routes}/** : Keep CLI, generators, bundler plugins, and virtual file routing utilities in their dedicated tooling package directories
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-devtools,*-router-devtools}/** : Keep router devtools packages in packages/router-devtools/ and packages/*-router-devtools/
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to e2e/** : Store end-to-end tests under the e2e/ directory
Learnt from: FatahChan
Repo: TanStack/router PR: 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.
📚 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-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/react-router/basic-react-query-file-based/tests/transition.spec.tse2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.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:
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsxe2e/solid-start/basic/src/routeTree.gen.tse2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts
📚 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.tse2e/solid-start/basic/src/routeTree.gen.tse2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-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-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 **/src/routes/** : Place file-based routes under src/routes/ directories
Applied to files:
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts
🧬 Code graph analysis (5)
e2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsx (1)
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx (1)
Route(5-10)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (1)
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx (1)
Route(20-27)
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx (1)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (1)
Route(20-27)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts (1)
e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts (3)
FileRoutesByTo(75-82)FileRoutesById(83-94)RootRouteChildren(126-131)
e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts (1)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts (3)
FileRoutesByTo(75-82)FileRoutesById(83-94)RootRouteChildren(126-131)
⏰ 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 (14)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (1)
20-20: LGTM! Route path restructuring is consistent.The route path and Link
fromprop are correctly updated to the nested structure/transition/count/query. The React Query integration withplaceholderDataproperly maintains previous data during transitions, which the test verifies.Also applies to: 40-40
e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts (1)
6-6: LGTM! Test navigation updated correctly.The navigation target properly reflects the new nested route structure. The test logic appropriately verifies that
placeholderDataprevents loading states during transitions.e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts (1)
1-9: Autogenerated file - no review required.This file is automatically generated by TanStack Router and should not be manually modified. The route tree updates correctly reflect the new
/transition/count/querystructure.e2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsx (4)
5-10: LGTM! Route definition correctly updated.The route path has been properly updated to the nested structure '/transition/count/create-resource', and the route configuration with search validation is well-formed.
12-27: LGTM! Link navigation updated consistently.The Link component's
fromprop correctly references the new route path '/transition/count/create-resource', ensuring type-safe navigation.
3-6: LGTM! Test updated for new route path.The test name and navigation target are correctly updated to reflect the new nested route structure.
23-23: Verify: Fire-and-forget clicks intentional for transition testing?The
awaitkeyword has been removed from multiple click actions. This creates rapid consecutive clicks without waiting for each to complete, which appears intentional for testing that transitions keep old values visible during rapid navigation.Please confirm this is the intended behavior for testing transition handling under rapid user interactions.
Also applies to: 43-44, 64-66
e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts (1)
6-6: LGTM! Test navigation updated for query variant.The test correctly navigates to '/transition/count/query', which aligns with this being the Solid Query-based variant of the transition test.
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx (3)
2-2: Good addition! Suspense import now explicit.The
Suspenseimport from 'solid-js' has been properly added. Since the component uses<Suspense>at line 35, this import is necessary for the code to function correctly.
20-27: LGTM! Route definition correctly updated.The route path has been updated to the nested structure '/transition/count/query', and the route configuration properly integrates with Solid Query for data loading.
40-40: LGTM! Link navigation updated consistently.The Link component's
fromprop correctly references the new route path '/transition/count/query'.e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx (2)
5-10: LGTM! Solid Start route correctly defined.The route definition for '/transition/count/create-resource' is properly configured with search validation and component assignment for the Solid Start environment.
29-48: LGTM! Async resource with Suspense boundary.The Result component correctly uses
createResourcefor async data fetching with a Suspense boundary to handle loading states. This aligns with the test's goal of verifying transition behavior.e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
3-6: LGTM! Test updated to reflect new route structure.The test name and navigation target correctly reflect the restructured route hierarchy from
/transitionto/transition/count/create-resource.
| // 1 click | ||
|
|
||
| await page.getByTestId('increase-button').click() | ||
| page.getByTestId('increase-button').click() |
There was a problem hiding this comment.
Await all Playwright click actions to prevent race conditions.
Removing await from click operations can cause race conditions where the test proceeds before clicks are fully processed, leading to flaky tests. This is particularly problematic in this test because:
- The test relies on precise timing to verify transition behavior
- Multiple rapid clicks need to complete before subsequent expectations run
- The polling mechanism may not capture the correct state transitions
Playwright best practice is to always await actions, even when firing multiple clicks in rapid succession.
Apply this diff to restore the await keywords:
- page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
await expect(page.getByTestId('n-value')).toContainText('n: 1', {
timeout: 2_000,
})- page.getByTestId('increase-button').click()
- page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
await expect(page.getByTestId('n-value')).toContainText('n: 2', {
timeout: 2000,
})- page.getByTestId('increase-button').click()
- page.getByTestId('increase-button').click()
- page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()
await expect(page.getByTestId('n-value')).toContainText('n: 4', {
timeout: 2000,
})Also applies to: 43-44, 64-66
🤖 Prompt for AI Agents
In e2e/solid-router/basic-file-based/tests/transition.spec.ts around lines 23
(and similarly at 43-44 and 64-66), several Playwright click() calls are missing
await which can cause race conditions and flaky tests; update each
page.getByTestId(...).click() invocation to be awaited (prepend with await) so
the test waits for each click to complete before proceeding, ensuring multiple
rapid clicks and subsequent expectations observe the correct state transitions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/solid-start/basic/tests/transition.spec.ts (1)
41-84: Commented-out multi-click scenarios need resolution.The TODO and commented sections for 2-click and 3-click scenarios indicate incomplete test coverage for rapid consecutive transitions. This was already flagged in previous reviews.
🧹 Nitpick comments (1)
e2e/solid-start/basic/tests/transition.spec.ts (1)
13-19: Consider awaiting pending polls before validation.The async callback in
setIntervalcan have multiple overlapping operations in flight, and clearing the interval (line 86) doesn't wait for pending async operations to complete. While unlikely to affect the test outcome given the timing, you could improve robustness by:
Adding a brief wait after clearing the interval:
clearInterval(pollInterval) +await page.waitForTimeout(100) // Allow pending polls to completeOr using a more controlled polling approach with recursive setTimeout that waits for each poll to complete before scheduling the next.
Also applies to: 86-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/solid-start/basic/tests/transition.spec.ts(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:
e2e/solid-start/basic/tests/transition.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/tests/transition.spec.ts
🧠 Learnings (2)
📓 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: 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/
📚 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-start/basic/tests/transition.spec.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
e2e/solid-start/basic/tests/transition.spec.ts (1)
1-40: Well-structured transition test for single-click scenario.The test correctly validates that old values remain visible during navigation transitions:
- Initial state is verified
- Click is intentionally not awaited to enable immediate assertion of old values
- Old values are asserted to persist during transition
- New values are confirmed after a brief wait
- Validation ensures no "Loading..." state interrupts the user experience
The implementation aligns well with the PR objectives for testing resource-based transitions.
Also applies to: 86-97
birkskyum
force-pushed
the
test(solid-start)--add-transition-testing-with-create-resource
branch
from
ad50018 to
b76c0df
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
23-23: Await all Playwright click actions to prevent race conditions.The missing
awaiton click operations was previously flagged and remains unresolved. Even when testing rapid transitions, Playwright actions should be awaited to ensure clicks are fully processed before subsequent expectations run. This prevents flaky test behavior.Apply this diff:
- page.getByTestId('increase-button').click() + await page.getByTestId('increase-button').click()Also applies to: 43-44, 64-66
🧹 Nitpick comments (1)
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx (1)
15-22: Consider removing redundantfromprop.The
fromprop on Line 18 is unnecessary since the Link is rendered within the route's component context. The Link can automatically infer the current route.Apply this diff:
<Link data-testid="increase-button" class="border bg-gray-50 px-3 py-1" - from="/transition/count/create-resource" search={(s) => ({ n: s.n + 1 })} > Increase </Link>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts(8 hunks)e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx(2 hunks)e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts(1 hunks)e2e/solid-router/basic-file-based/src/routeTree.gen.ts(11 hunks)e2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsx(2 hunks)e2e/solid-router/basic-file-based/tests/transition.spec.ts(4 hunks)e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts(8 hunks)e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx(3 hunks)e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(11 hunks)e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx(1 hunks)e2e/solid-start/basic/tests/transition.spec.ts(1 hunks)packages/router-core/src/router.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx
- e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts
- e2e/solid-router/basic-file-based/src/routes/transition/count/create-resource.tsx
- packages/router-core/src/router.ts
- e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts
🧰 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-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/tests/transition.spec.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.tse2e/solid-start/basic/tests/transition.spec.ts
🧠 Learnings (7)
📓 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: 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/
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.
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to docs/{router,start}/** : Place router docs under docs/router/ and start framework docs under docs/start/
Learnt from: FatahChan
Repo: TanStack/router PR: 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.
📚 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:
e2e/solid-start/basic/src/routes/transition/count/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
📚 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-solid-query-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/tests/transition.spec.tse2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
📚 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.tse2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.tse2e/solid-start/basic/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-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 **/src/routes/** : Place file-based routes under src/routes/ directories
Applied to files:
e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts
📚 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-start/basic/tests/transition.spec.ts
🧬 Code graph analysis (2)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (1)
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx (1)
Route(20-27)
e2e/solid-router/basic-solid-query-file-based/src/routeTree.gen.ts (1)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts (3)
FileRoutesByTo(75-82)FileRoutesById(83-94)RootRouteChildren(126-131)
⏰ 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 (6)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (2)
20-27: LGTM! Route path correctly matches file location.The route path update to
/transition/count/queryproperly aligns with the file-based routing convention, where the file path must match the route path. This refactoring to a more specific nested route structure improves test organization.
37-44: LGTM! Link reference correctly updated.The
fromprop is properly updated to match the new route path, maintaining type-safe routing behavior. The search parameter increment logic remains correct.e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
3-6: LGTM - Test updated for new route path.The test name and navigation correctly reflect the new nested route structure at
/transition/count/create-resource.e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts (1)
6-36: LGTM - Test properly awaits actions.The test correctly navigates to the new
/transition/count/queryroute and properly awaits all Playwright actions, following best practices.e2e/solid-start/basic/src/routes/transition/count/create-resource.tsx (2)
5-10: LGTM - Route configuration is correct.The route setup properly validates search parameters with a sensible default value for
n.
29-48: LGTM - Resource implementation correctly tests transitions.The
createResourceimplementation with a 1-second delay properly simulates async data fetching for testing transition behavior. The Suspense fallback ensures loading states are handled correctly.
|
|
||
| // 1 click | ||
|
|
||
| page.getByTestId('increase-button').click() |
There was a problem hiding this comment.
Await Playwright click actions to prevent race conditions.
The click operation is missing await, which can cause race conditions where the test proceeds before the click is fully processed. This is particularly problematic when testing transition behavior with precise timing requirements.
Apply this diff:
- page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()Also applies to: 45-46, 66-68
🤖 Prompt for AI Agents
In e2e/solid-start/basic/tests/transition.spec.ts around lines 23 (and also at
45-46 and 66-68), the Playwright click calls are not awaited which can cause
race conditions; update each occurrence to await the click promise (i.e., prefix
each page.getByTestId(...).click() with await) so the test waits for the click
to complete before proceeding, keeping the existing selectors and test flow
intact.
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 (1)
packages/solid-router/src/Transitioner.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/src/Transitioner.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/Transitioner.tsx
🧠 Learnings (2)
📓 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/
📚 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/src/Transitioner.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
| router.startTransition = (fn: () => void | Promise<void>) => { | ||
| setIsTransitioning(true) | ||
| Solid.startTransition(async () => { | ||
| try { | ||
| await fn() | ||
| } finally { | ||
| setIsTransitioning(false) | ||
| } | ||
| // Execute fn synchronously to check if it returns a Promise | ||
| let result: void | Promise<void> | ||
| Solid.startTransition(() => { | ||
| result = fn() | ||
| }) | ||
|
|
||
| // If fn returned a Promise, track the transition state | ||
| if (result! instanceof Promise) { | ||
| setIsTransitioning(true) | ||
| result.finally(() => { | ||
| setIsTransitioning(false) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Remove unnecessary non-null assertion on line 46.
The result! syntax is unnecessary and misleading. Since result is typed as void | Promise<void>, it can be undefined when fn() returns void. The instanceof Promise check works correctly without the non-null assertion:
undefined instanceof Promise→false(skips the block)promise instanceof Promise→true(enters the block)
Apply this diff:
- if (result! instanceof Promise) {
+ if (result instanceof Promise) {Additionally, verify error handling behavior:
Consider whether synchronous errors thrown by fn() or unhandled Promise rejections need explicit handling in this implementation. The finally handler will reset transition state even on rejection, but there's no explicit error catching.
I'll verify the review comment by examining the actual code in the repository.
🏁 Script executed:
#!/bin/bash
# Locate and inspect the Transitioner.tsx file
find . -type f -name "Transitioner.tsx" 2>/dev/null | head -20Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for the startTransition function in Transitioner.tsx
fd "Transitioner.tsx" --exec cat {} \;Length of output: 7491
🏁 Script executed:
#!/bin/bash
# Check for TypeScript configuration to understand type checking strictness
fd "tsconfig.json" -path "*/solid-router/*" --exec cat {} \;Length of output: 168
🏁 Script executed:
#!/bin/bash
# Look for similar Promise handling patterns in the codebase
rg -n "instanceof Promise" packages/ -A2 -B2Length of output: 1556
🏁 Script executed:
#!/bin/bash
# Search for error handling patterns with promises in the solid-router package
rg -n "\.finally\(" packages/solid-router/ -A2 -B2 | head -40Length of output: 370
🏁 Script executed:
#!/bin/bash
# Check if there's error handling for startTransition elsewhere
rg -n "startTransition" packages/solid-router/ -A5 -B2 | head -60Length of output: 2402
Remove unnecessary non-null assertion on line 46.
The result! syntax is unnecessary and inconsistent with similar instanceof Promise checks elsewhere in the codebase (e.g., packages/start-client-core/src/createServerFn.ts:677 and packages/router-core/src/router.ts:2604). Since result is typed as void | Promise<void>, it can legitimately be undefined, and TypeScript's instanceof check works correctly without the assertion:
- if (result! instanceof Promise) {
+ if (result instanceof Promise) {Note: The finally handler resets transition state on both successful resolution and rejection, which is the current intended behavior. If explicit error handling for synchronous exceptions or Promise rejections is desired, add a catch handler accordingly.
📝 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.
| router.startTransition = (fn: () => void | Promise<void>) => { | |
| setIsTransitioning(true) | |
| Solid.startTransition(async () => { | |
| try { | |
| await fn() | |
| } finally { | |
| setIsTransitioning(false) | |
| } | |
| // Execute fn synchronously to check if it returns a Promise | |
| let result: void | Promise<void> | |
| Solid.startTransition(() => { | |
| result = fn() | |
| }) | |
| // If fn returned a Promise, track the transition state | |
| if (result! instanceof Promise) { | |
| setIsTransitioning(true) | |
| result.finally(() => { | |
| setIsTransitioning(false) | |
| }) | |
| } | |
| } | |
| router.startTransition = (fn: () => void | Promise<void>) => { | |
| // Execute fn synchronously to check if it returns a Promise | |
| let result: void | Promise<void> | |
| Solid.startTransition(() => { | |
| result = fn() | |
| }) | |
| // If fn returned a Promise, track the transition state | |
| if (result instanceof Promise) { | |
| setIsTransitioning(true) | |
| result.finally(() => { | |
| setIsTransitioning(false) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
packages/solid-router/src/Transitioner.tsx around lines 38 to 52: the non-null
assertion `result!` is unnecessary and should be removed; change the conditional
to use the normal instanceof check (`if (result instanceof Promise)`) so the
code correctly handles `void | Promise<void>` (allowing undefined) and leave the
finally handler as-is to reset state on both resolve and reject.
24112d9 to
b76c0df
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/router-core/src/Matches.ts(1 hunks)packages/router-core/src/load-matches.ts(5 hunks)packages/router-core/src/router.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/Matches.ts
🧰 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/router-core/src/load-matches.tspackages/router-core/src/router.ts
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.tspackages/router-core/src/router.ts
🧠 Learnings (5)
📓 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: 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/
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.
Learnt from: FatahChan
Repo: TanStack/router PR: 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.
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.
📚 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/router-core/src/load-matches.tspackages/router-core/src/router.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/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/src/load-matches.tspackages/router-core/src/router.ts
📚 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:
packages/router-core/src/load-matches.tspackages/router-core/src/router.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/router-core/src/router.ts
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (1)
packages/router-core/src/route.ts (1)
id(1547-1549)
⏰ 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 (8)
packages/router-core/src/load-matches.ts (5)
114-115: LGTM: Non-reactive error storage for transitions.The dual storage approach (both
_nonReactive.errorand reactiveerrorin updateMatch) correctly handles frameworks that batch reactive updates during transitions.
454-458: LGTM: Non-reactive context storage.Correctly stores
beforeLoadContextnon-reactively for immediate access during transitions, complementing the reactive context update at lines 459-465.
570-587: Context accumulation logic updated to walk entire ancestor chain.The new approach correctly builds context by merging
__routeContextand_nonReactive.__beforeLoadContextfrom all ancestor matches. This ensures proper context inheritance and bypasses reactive batching during transitions.
767-772: LGTM: Comprehensive error checking before loader.Correctly checks both non-reactive and reactive error storage to catch redirect/notFound errors from preloaded matches, ensuring errors aren't missed during transitions.
784-788: LGTM: Post-loader error handling.Correctly retrieves fresh match after awaiting the loader promise and checks both error storage locations for comprehensive error handling.
packages/router-core/src/router.ts (3)
1492-1492: LGTM: Context composition includes non-reactive beforeLoadContext.Correctly merges non-reactive
__beforeLoadContextinto the final context, ensuring proper precedence order.
1532-1532: LGTM: Consistent context composition.Same correct pattern as line 1492, ensuring
_nonReactive.__beforeLoadContextis included in the final context.
2284-2304: LGTM: Framework-agnostic transition wrapper.Wrapping
updateMatchlogic instartTransitionis correct. SincestartTransitionis a configurable function (line 939, defaults to direct execution), this maintains the framework-agnostic design of router-core while enabling framework-specific transition behavior when needed.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/load-matches.ts(5 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/router-core/src/load-matches.ts
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
🧠 Learnings (4)
📓 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: 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/
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.
Learnt from: FatahChan
Repo: TanStack/router PR: 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.
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.
📚 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/router-core/src/load-matches.ts
📚 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:
packages/router-core/src/load-matches.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/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/src/load-matches.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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/load-matches.ts (2)
114-115: LGTM!Storing errors in non-reactive storage for immediate access during transitions is a sound pattern that maintains framework-agnostic code while supporting reactive frameworks.
454-458: LGTM!The non-reactive storage of
__beforeLoadContextwith proper null-safety checks is well-implemented. This pattern allows immediate access to context data during concurrent updates.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/typing/create-resource.tsx(1 hunks)e2e/solid-start/basic/src/routeTree.gen.ts(11 hunks)e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsx(1 hunks)e2e/solid-start/basic/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/solid-start/basic/vite.config.ts
🧰 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-start/basic/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
🧠 Learnings (5)
📓 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: 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/
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
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.
📚 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:
e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
📚 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/src/routes/transition/typing/create-resource.tsxe2e/solid-router/basic-file-based/src/routeTree.gen.tse2e/solid-start/basic/src/routeTree.gen.ts
📚 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.tse2e/solid-start/basic/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
🧬 Code graph analysis (1)
e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsx (1)
e2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsx (1)
Route(4-10)
⏰ 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
| <input | ||
| class="border bg-gray-50 text-black px-3 py-1" | ||
| value={''} | ||
| placeholder="Type to search..." | ||
| onInput={(e) => { | ||
| navigate({ search: { query: e.currentTarget.value } }) | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Fix the input value binding.
The input value is hardcoded to an empty string, which means it will never display the current search query even though the onInput handler correctly updates it. Users won't see what they've typed, creating a broken user experience.
Apply this diff to bind the input to the search query:
<input
class="border bg-gray-50 text-black px-3 py-1"
- value={''}
+ value={searchQuery().query}
placeholder="Type to search..."
onInput={(e) => {
navigate({ search: { query: e.currentTarget.value } })
}}
/>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
e2e/solid-router/basic-file-based/src/routes/transition/typing/create-resource.tsx
around lines 27-34, the input's value is hardcoded to an empty string so user
typing isn't displayed; change the value to the current search query (the source
driving navigate({ search: { query } })) so the input is a controlled component
— e.g., read the query from the route/search params or component state and set
value to that query (falling back to an empty string when undefined) while
keeping the onInput handler that updates navigate.
| <input | ||
| class="border bg-gray-50 text-black px-3 py-1" | ||
| value={''} | ||
| placeholder="Type to search..." | ||
| onInput={(e) => { | ||
| navigate({ search: { query: e.currentTarget.value } }) | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Fix the input value binding.
The input value is hardcoded to an empty string, which means it will never display the current search query even though the onInput handler correctly updates it. Users won't see what they've typed, creating a broken user experience.
Apply this diff to bind the input to the search query:
<input
class="border bg-gray-50 text-black px-3 py-1"
- value={''}
+ value={searchQuery().query}
placeholder="Type to search..."
onInput={(e) => {
navigate({ search: { query: e.currentTarget.value } })
}}
/>📝 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.
| <input | |
| class="border bg-gray-50 text-black px-3 py-1" | |
| value={''} | |
| placeholder="Type to search..." | |
| onInput={(e) => { | |
| navigate({ search: { query: e.currentTarget.value } }) | |
| }} | |
| /> | |
| <input | |
| class="border bg-gray-50 text-black px-3 py-1" | |
| value={searchQuery().query} | |
| placeholder="Type to search..." | |
| onInput={(e) => { | |
| navigate({ search: { query: e.currentTarget.value } }) | |
| }} | |
| /> |
🤖 Prompt for AI Agents
In e2e/solid-start/basic/src/routes/transition/typing/create-resource.tsx around
lines 27-34, the input's value is hardcoded to an empty string so user typing is
never shown; change the value binding to read the current search query from the
route search state (e.g. use the search object's query property or signal)
instead of '' so the input displays the current query, leaving the existing
placeholder and onInput navigate handler intact.
1356791 to
de8a3b5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
birkskyum
force-pushed
the
test(solid-start)--add-transition-testing-with-create-resource
branch
2 times, most recently
from
ed42f6f to
9af59c7
Compare
birkskyum
force-pushed
the
test(solid-start)--add-transition-testing-with-create-resource
branch
from
9af59c7 to
d74d533
Compare
birkskyum
deleted the
test(solid-start)--add-transition-testing-with-create-resource
branch
Follow-up to
Closes #5636
Summary by CodeRabbit
New Features
Bug Fixes
Tests