test(solid-start): add transition testing with create resource #5792
Conversation
WalkthroughRoutes across multiple e2e suites were changed from the generic Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Application
participant RouterCore as Router Core
participant Matches as RouteMatches (_nonReactive)
participant Runtime as JS Runtime (React/Solid)
participant Store as Store / UI
User->>App: trigger navigation(to)
App->>RouterCore: handleNavigate(to)
RouterCore->>Matches: compute new matches & run beforeLoad
note right of Matches `#f7f3e8`: beforeLoad stores\ncontext into match._nonReactive.__beforeLoadContext
Matches-->>RouterCore: beforeLoad contexts set
RouterCore->>Runtime: startTransition(() => updateMatch/updater())
Runtime->>Store: perform state updates (low-priority)
Store-->>Runtime: render/update
Runtime-->>App: commit rendered view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
packages/{react-router,solid-router}/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-08T08:11:47.088ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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 39s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-08 22:38:12 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: |
![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.
| timeout: 2000, | ||
| }) | ||
|
|
||
| // TODO - the below tests should be possible to uncomment |
There was a problem hiding this comment.
🧩 Analysis chain
Remove misleading TODO comment.
The TODO comment states "the below tests should be possible to uncomment," but the tests on lines 43-84 are already uncommented and active. This comment should either be removed or clarified if there's a known issue with these test scenarios.
Could you clarify: Are these multi-click test scenarios working correctly, or is there a known issue that the TODO is referring to?
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the transition.spec.ts file to verify the TODO comment and test state
fd -n "transition.spec.ts" -t f | head -1 | xargs -I {} sh -c 'echo "=== File: {} ===" && sed -n "35,90p" {}'
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Find and examine transition.spec.ts to verify TODO comment and test state
find . -name "transition.spec.ts" -type f | head -1 | xargs -I {} sh -c 'echo "=== File: {} ===" && sed -n "35,90p" {}'Length of output: 123
🏁 Script executed:
#!/bin/bash
# Read the specific transition.spec.ts file from solid-start directory
cat "e2e/solid-start/basic/tests/transition.spec.ts" | sed -n "35,90p"Length of output: 1494
Add await keywords to click operations in multi-click test scenarios.
The TODO comment is not misleading—it correctly identifies that these tests cannot function as written. While lines 43-84 are uncommented and active, the click() calls at lines 47-48 and 61-63 are missing await keywords, which breaks the test logic. Playwright requires these async operations to be awaited for proper sequencing. Add await before each page.getByTestId('increase-button').click() call to make these test scenarios functional.
🤖 Prompt for AI Agents
In e2e/solid-start/basic/tests/transition.spec.ts around lines 43–84, the
multi-click test calls page.getByTestId('increase-button').click() multiple
times without awaiting the returned promises; add await before each
page.getByTestId('increase-button').click() invocation (notably at the
occurrences around lines 47–48 and 61–63) so Playwright sequences the clicks
correctly and the test logic executes as intended.
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests