test(router, start): query navigation transitions #5802
Conversation
WalkthroughQuery results changed from a primitive to an object Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as Page Component
participant Loader as Route Loader
participant QC as QueryClient
participant QFn as Query Function
User->>Page: Click "increase"
Page->>Loader: navigate with new search (n+1)
Loader->>QC: ensureQueryData(doubleQueryOptions(n+1))
QC->>QFn: invoke queryFn(n+1)
QFn-->>QC: return { n: n+1, double: (n+1)*2 } (after delay)
QC-->>Loader: resolved data
Loader-->>Page: route renders (old data remains visible until resolved)
Page->>Page: render data?.n and data?.double
Note right of Page: Tests poll body text to ensure "Loading..." never appears
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 3e9bda4
☁️ 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
🧹 Nitpick comments (2)
e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts (2)
13-19: Consider using try-finally for interval cleanup.The polling interval is cleared at line 84, but if an assertion fails earlier, the interval will continue running. This could cause resource leaks or interfere with other tests.
Consider wrapping the test logic in a try-finally block:
const pollInterval = setInterval(async () => { const text = await page .locator('body') .textContent() .catch(() => '') if (text) bodyTexts.push(text) }, 50) +try { + // 1 click page.getByTestId('increase-button').click() // ... rest of test ... +} finally { + clearInterval(pollInterval) +} -clearInterval(pollInterval)
25-39: Minor inconsistency: timeout format varies.Timeouts use both
2_000(with underscore) and2000(without). While both are valid TypeScript numeric literals, consistency improves readability.Consider using consistent formatting throughout:
-await expect(page.getByTestId('n-value')).toContainText('n: 1', { - timeout: 2_000, -}) -await expect(page.getByTestId('double-value')).toContainText('double: 2', { - timeout: 2_000, -}) +await expect(page.getByTestId('n-value')).toContainText('n: 1', { + timeout: 2000, +}) +await expect(page.getByTestId('double-value')).toContainText('double: 2', { + timeout: 2000, +})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx(3 hunks)e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts(1 hunks)e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx(2 hunks)e2e/solid-router/basic-solid-query-file-based/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-router/basic-solid-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/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.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-router/basic-solid-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/tests/transition.spec.tse2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx
🧠 Learnings (3)
📓 Common learnings
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: 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/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
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.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/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (6)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (3)
2-2: LGTM: Unused import removed.The removal of
useMemofrom the import is correct since it's not used anywhere in the file.
10-18: LGTM: Data shape refactored for better transition UX.The query now returns an object
{ n, double }instead of a primitive, which works well withplaceholderDatato keep both values visible during navigation transitions.
47-50: LGTM: Rendering updated to match new data shape.The UI correctly accesses the new object properties with optional chaining, and the test IDs align with the test expectations.
e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts (3)
3-5: LGTM: Test name is clear and descriptive.The updated test name clearly communicates the test's purpose: validating that old values remain visible during navigation transitions.
41-61: LGTM: Batch navigation test validates transition coalescing.The two rapid clicks correctly test that transitions batch appropriately, with
njumping from 2 to 4 (skipping 3), demonstrating proper navigation coalescing behavior.
62-83: LGTM: Three-click scenario validates additional batching.The test correctly validates that three rapid clicks batch properly, with
nadvancing from 4 to 7, further confirming the transition coalescing behavior.
| if (hasLoadingText) { | ||
| throw new Error( | ||
| 'FAILED: "Loading..." appeared during navigation. ' + | ||
| 'Solid Router should use transitions to keep old values visible.', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Critical: Error message references wrong framework.
The error message mentions "Solid Router" but this is a React Router test. This appears to be a copy-paste error from the solid-router equivalent test.
Apply this diff:
if (hasLoadingText) {
throw new Error(
'FAILED: "Loading..." appeared during navigation. ' +
- 'Solid Router should use transitions to keep old values visible.',
+ 'React Router should use transitions to keep old values visible.',
)
}📝 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.
| if (hasLoadingText) { | |
| throw new Error( | |
| 'FAILED: "Loading..." appeared during navigation. ' + | |
| 'Solid Router should use transitions to keep old values visible.', | |
| ) | |
| } | |
| if (hasLoadingText) { | |
| throw new Error( | |
| 'FAILED: "Loading..." appeared during navigation. ' + | |
| 'React Router should use transitions to keep old values visible.', | |
| ) | |
| } |
🤖 Prompt for AI Agents
In e2e/react-router/basic-react-query-file-based/tests/transition.spec.ts around
lines 89 to 94, the thrown error message incorrectly references "Solid Router";
update the string to reference "React Router" instead. Replace the message so it
reads something like: 'FAILED: "Loading..." appeared during navigation. React
Router should use transitions to keep old values visible.' and keep the rest of
the throw logic unchanged.
| page.getByTestId('increase-button').click() | ||
|
|
||
| await expect(page.getByTestId('n-value')).toContainText('n: 1', { | ||
| timeout: 2_000, | ||
| }) | ||
| await expect(page.getByTestId('double-value')).toContainText('double: 2', { | ||
| timeout: 2_000, | ||
| }) | ||
|
|
||
| await page.waitForTimeout(200) | ||
|
|
||
| clearInterval(interval) | ||
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | ||
| timeout: 2000, | ||
| }) | ||
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | ||
| timeout: 2000, | ||
| }) | ||
|
|
||
| // 2 clicks | ||
|
|
||
| page.getByTestId('increase-button').click() | ||
| page.getByTestId('increase-button').click() | ||
|
|
||
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | ||
| timeout: 2_000, | ||
| timeout: 2000, | ||
| }) | ||
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | ||
| timeout: 2_000, | ||
| timeout: 2000, | ||
| }) | ||
|
|
||
| await page.waitForTimeout(200) | ||
|
|
||
| await expect(page.getByTestId('n-value')).toContainText('n: 4', { | ||
| timeout: 2000, | ||
| }) | ||
| await expect(page.getByTestId('double-value')).toContainText('double: 8', { | ||
| timeout: 2000, | ||
| }) | ||
|
|
||
| // 3 clicks | ||
|
|
||
| page.getByTestId('increase-button').click() | ||
| page.getByTestId('increase-button').click() | ||
| page.getByTestId('increase-button').click() |
There was a problem hiding this comment.
Await each Playwright click call
Every page.getByTestId('increase-button').click() returns a Promise; without await, the test continues while the click is still in flight, so the subsequent assertions can race the navigation and become flaky. Playwright’s docs note these click helpers resolve only after the action completes, which is why they must be awaited.(playwright.bootcss.com)
Add await (and keep the existing awaits for the expectations) so each step waits for the click to finish before proceeding:
- page.getByTestId('increase-button').click()
+ await 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()
...
- 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()📝 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.
| page.getByTestId('increase-button').click() | |
| await expect(page.getByTestId('n-value')).toContainText('n: 1', { | |
| timeout: 2_000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 2', { | |
| timeout: 2_000, | |
| }) | |
| await page.waitForTimeout(200) | |
| clearInterval(interval) | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2000, | |
| }) | |
| // 2 clicks | |
| page.getByTestId('increase-button').click() | |
| page.getByTestId('increase-button').click() | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2_000, | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2_000, | |
| timeout: 2000, | |
| }) | |
| await page.waitForTimeout(200) | |
| await expect(page.getByTestId('n-value')).toContainText('n: 4', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 8', { | |
| timeout: 2000, | |
| }) | |
| // 3 clicks | |
| page.getByTestId('increase-button').click() | |
| page.getByTestId('increase-button').click() | |
| page.getByTestId('increase-button').click() | |
| await page.getByTestId('increase-button').click() | |
| await expect(page.getByTestId('n-value')).toContainText('n: 1', { | |
| timeout: 2_000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 2', { | |
| timeout: 2_000, | |
| }) | |
| await page.waitForTimeout(200) | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2000, | |
| }) | |
| // 2 clicks | |
| await page.getByTestId('increase-button').click() | |
| await page.getByTestId('increase-button').click() | |
| await expect(page.getByTestId('n-value')).toContainText('n: 2', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 4', { | |
| timeout: 2000, | |
| }) | |
| await page.waitForTimeout(200) | |
| await expect(page.getByTestId('n-value')).toContainText('n: 4', { | |
| timeout: 2000, | |
| }) | |
| await expect(page.getByTestId('double-value')).toContainText('double: 8', { | |
| timeout: 2000, | |
| }) | |
| // 3 clicks | |
| await page.getByTestId('increase-button').click() | |
| await page.getByTestId('increase-button').click() | |
| await page.getByTestId('increase-button').click() |
🤖 Prompt for AI Agents
In e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts around
lines 23 to 66, several page.getByTestId('increase-button').click() calls are
missing awaits; update each click call to be awaited (add await before each
page.getByTestId(...).click()) so each click completes before continuing,
leaving all existing await expect(...) calls intact to preserve sequencing.
birkskyum
changed the title
birkskyum
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
e2e/react-start/basic-react-query/package.json(1 hunks)e2e/react-start/basic-react-query/src/routeTree.gen.ts(11 hunks)e2e/react-start/basic-react-query/src/routes/transition/count/query.tsx(1 hunks)e2e/react-start/basic-react-query/tests/transition.spec.ts(1 hunks)e2e/solid-start/basic-solid-query/package.json(1 hunks)e2e/solid-start/basic-solid-query/src/routeTree.gen.ts(11 hunks)e2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsx(1 hunks)e2e/solid-start/basic-solid-query/tests/transition.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
e2e/react-start/basic-react-query/package.jsone2e/solid-start/basic-solid-query/package.json
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic-react-query/package.jsone2e/react-start/basic-react-query/src/routes/transition/count/query.tsxe2e/solid-start/basic-solid-query/package.jsone2e/solid-start/basic-solid-query/tests/transition.spec.tse2e/solid-start/basic-solid-query/src/routeTree.gen.tse2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsxe2e/react-start/basic-react-query/src/routeTree.gen.tse2e/react-start/basic-react-query/tests/transition.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/basic-react-query/src/routes/transition/count/query.tsxe2e/solid-start/basic-solid-query/tests/transition.spec.tse2e/solid-start/basic-solid-query/src/routeTree.gen.tse2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsxe2e/react-start/basic-react-query/src/routeTree.gen.tse2e/react-start/basic-react-query/tests/transition.spec.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/basic-react-query/src/routes/transition/count/query.tsxe2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsx
🧠 Learnings (5)
📓 Common learnings
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: 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-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-start/basic-solid-query/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-start/basic-solid-query/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-start/basic-solid-query/src/routeTree.gen.tse2e/react-start/basic-react-query/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-solid-query/src/routeTree.gen.tse2e/react-start/basic-react-query/src/routeTree.gen.ts
🧬 Code graph analysis (3)
e2e/react-start/basic-react-query/src/routes/transition/count/query.tsx (1)
e2e/react-router/basic-react-query-file-based/src/routes/transition/count/query.tsx (1)
TransitionPage(29-53)
e2e/solid-start/basic-solid-query/tests/transition.spec.ts (2)
e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts (1)
page(3-37)e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
page(3-95)
e2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsx (1)
e2e/solid-router/basic-solid-query-file-based/src/routes/transition/count/query.tsx (1)
TransitionPage(29-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (10)
e2e/react-start/basic-react-query/package.json (1)
24-25: LGTM!The zod dependency addition is appropriate for the new route's search schema validation using
z.objectandz.number().default(1).e2e/react-start/basic-react-query/tests/transition.spec.ts (1)
1-95: Test logic looks solid.The test effectively validates that transitions keep old values visible during navigation by:
- Polling body content at 50ms intervals during navigation
- Asserting proper state transitions after incremental clicks
- Verifying "Loading..." never appears in the captured text stream
The timeout values (2000ms) align well with the route's 1s artificial delay.
e2e/react-start/basic-react-query/src/routes/transition/count/query.tsx (3)
6-18: Well-designed query options for transition testing.The search schema and query options are properly configured:
- Zod 4 syntax is correct with
z.number().default(1)placeholderData: (oldData) => oldDataensures old values persist during navigation, which is key for the transition test- The 1s artificial delay provides a realistic async scenario
20-27: Loader correctly preloads query data.Using
queryClient.ensureQueryData()in the loader ensures data is available before navigation completes, working in tandem withplaceholderDatato provide seamless transitions.
29-55: Component properly handles async query state.The component correctly:
- Accesses search params via
Route.useSearch()- Uses optional chaining (
data?.n,data?.double) to safely access query results- Wraps content in Suspense (though the fallback should rarely show due to preloading + placeholderData)
e2e/react-start/basic-react-query/src/routeTree.gen.ts (1)
25-25: Generated route tree correctly integrates new route.The auto-generated route tree properly registers
/transition/count/query:
- Import added at line 25
- Route constant defined at lines 94-98
- All type maps updated (FileRoutesByFullPath, FileRoutesByTo, FileRoutesById)
- Union types extended with the new path
- Module augmentation includes proper route metadata
- Root route children correctly includes the new route
Also applies to: 94-98, 136-136, 152-152, 173-173, 193-193, 209-209, 229-229, 242-242, 338-344, 447-447
e2e/solid-start/basic-solid-query/package.json (1)
23-24: LGTM!The addition of Zod 4 as a dependency is appropriate for the new route's search parameter validation. The version specification is correct and aligns with the latest Zod release.
e2e/solid-start/basic-solid-query/tests/transition.spec.ts (1)
1-95: Excellent transition test implementation!This test comprehensively validates that Solid Router properly maintains visibility of old values during navigation transitions. The polling mechanism effectively captures any transitional states, and the multi-click scenarios (1, 2, and 3 clicks) thoroughly exercise the transition behavior.
e2e/solid-start/basic-solid-query/src/routes/transition/count/query.tsx (2)
6-18: Well-structured query configuration for transitions.The search schema with a numeric default and the doubleQueryOptions with
placeholderData: (oldData) => oldDatacorrectly enable smooth transitions by keeping old data visible while new data loads. The 1-second delay provides a realistic testing scenario.
47-50: The implementations are already consistent; no changes needed.The script output confirms that all four framework implementations (Solid Start, Solid Router, React Start, and React Router) render
nidentically usingdoubleQuery.data?.n. The original review comment incorrectly claimed Solid Router usedsearch().ndirectly, but the actual code matches Solid Start's implementation.Likely an incorrect or invalid review comment.
| if (hasLoadingText) { | ||
| throw new Error( | ||
| 'FAILED: "Loading..." appeared during navigation. ' + | ||
| 'Solid Router should use transitions to keep old values visible.', | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix incorrect framework name in error message.
The error message references "Solid Router" but this test is for React Router/React Start. This is a copy-paste error that would confuse developers if the test fails.
Apply this diff:
if (hasLoadingText) {
throw new Error(
'FAILED: "Loading..." appeared during navigation. ' +
- 'Solid Router should use transitions to keep old values visible.',
+ 'React Router should use transitions to keep old values visible.',
)
}📝 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.
| if (hasLoadingText) { | |
| throw new Error( | |
| 'FAILED: "Loading..." appeared during navigation. ' + | |
| 'Solid Router should use transitions to keep old values visible.', | |
| ) | |
| } | |
| if (hasLoadingText) { | |
| throw new Error( | |
| 'FAILED: "Loading..." appeared during navigation. ' + | |
| 'React Router should use transitions to keep old values visible.', | |
| ) | |
| } |
🤖 Prompt for AI Agents
In e2e/react-start/basic-react-query/tests/transition.spec.ts around lines 89 to
94, the error message incorrectly mentions "Solid Router"; update the thrown
Error string to reference "React Router" (or "React Start" if that aligns with
project naming) so it reads something like 'FAILED: "Loading..." appeared during
navigation. React Router should use transitions to keep old values visible.';
ensure punctuation and concatenation remain correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
e2e/react-start/basic-react-query/tests/transition.spec.ts (1)
89-93: Keep the framework name consistent in the failure message.The thrown error still references “Solid Router,” but this suite covers the React Start example. Please update the string so the failure message names the correct framework.
[scratchpad_end] -->
🧹 Nitpick comments (1)
e2e/solid-start/basic-solid-query/tests/transition.spec.ts (1)
26-38: Minor: Use consistent timeout formatting.For consistency, consider using the same numeric literal format for all timeouts (either
2_000or2000).- await expect(page.getByTestId('n-value')).toContainText('n: 2', { - timeout: 2000, - }) - await expect(page.getByTestId('double-value')).toContainText('double: 4', { - timeout: 2000, - }) + await expect(page.getByTestId('n-value')).toContainText('n: 2', { + timeout: 2_000, + }) + await expect(page.getByTestId('double-value')).toContainText('double: 4', { + timeout: 2_000, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/react-start/basic-react-query/tests/transition.spec.ts(1 hunks)e2e/solid-start/basic-solid-query/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/react-start/basic-react-query/tests/transition.spec.tse2e/solid-start/basic-solid-query/tests/transition.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/basic-react-query/tests/transition.spec.tse2e/solid-start/basic-solid-query/tests/transition.spec.ts
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/react-start/basic-react-query/tests/transition.spec.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/react-start/basic-react-query/tests/transition.spec.ts
🧬 Code graph analysis (1)
e2e/solid-start/basic-solid-query/tests/transition.spec.ts (2)
e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts (1)
page(3-37)e2e/solid-router/basic-file-based/tests/transition.spec.ts (1)
page(3-95)
⏰ 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 (2)
e2e/solid-start/basic-solid-query/tests/transition.spec.ts (2)
1-10: LGTM! Test setup is clean and well-structured.The imports, test declaration, initial navigation, and assertions are correct.
84-95: LGTM! Cleanup and final assertions are well-implemented.The polling interval cleanup and the assertion that "Loading..." never appeared during transitions are correctly implemented with a clear, descriptive error message.
|
|
||
| // 1 click | ||
|
|
||
| page.getByTestId('increase-button').click() |
There was a problem hiding this comment.
Await the click action to avoid race conditions.
page.getByTestId('increase-button').click() returns a promise. Without await, the test can assert before the click completes, causing flakiness or false positives. Please await the click so Playwright finishes dispatching the event before moving on.
Apply this diff:
- page.getByTestId('increase-button').click()
+ await page.getByTestId('increase-button').click()📝 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.
| page.getByTestId('increase-button').click() | |
| await page.getByTestId('increase-button').click() |
🤖 Prompt for AI Agents
In e2e/react-start/basic-react-query/tests/transition.spec.ts around line 23,
the test calls page.getByTestId('increase-button').click() without awaiting it;
change this to await the click call so the test waits for the click promise to
resolve before proceeding, preventing race conditions and flakiness.
| // 2 clicks | ||
|
|
||
| // page.getByTestId('increase-button').click() | ||
| // page.getByTestId('increase-button').click() | ||
|
|
||
| // await expect(page.getByTestId('n-value')).toContainText('n: 2', { | ||
| // timeout: 2000, | ||
| // }) | ||
| // await expect(page.getByTestId('double-value')).toContainText('double: 4', { | ||
| // timeout: 2000, | ||
| // }) | ||
|
|
||
| // await page.waitForTimeout(200) | ||
|
|
||
| // await expect(page.getByTestId('n-value')).toContainText('n: 4', { | ||
| // timeout: 2000, | ||
| // }) | ||
| // await expect(page.getByTestId('double-value')).toContainText('double: 8', { | ||
| // timeout: 2000, | ||
| // }) | ||
|
|
||
| // // 3 clicks | ||
|
|
||
| // page.getByTestId('increase-button').click() | ||
| // page.getByTestId('increase-button').click() | ||
| // page.getByTestId('increase-button').click() | ||
|
|
||
| // await expect(page.getByTestId('n-value')).toContainText('n: 4', { | ||
| // timeout: 2000, | ||
| // }) | ||
| // await expect(page.getByTestId('double-value')).toContainText('double: 8', { | ||
| // timeout: 2000, | ||
| // }) | ||
|
|
||
| // await page.waitForTimeout(200) | ||
|
|
||
| // await expect(page.getByTestId('n-value')).toContainText('n: 7', { | ||
| // timeout: 2000, | ||
| // }) | ||
| // await expect(page.getByTestId('double-value')).toContainText('double: 14', { | ||
| // timeout: 2000, | ||
| // }) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Remove or uncomment the dead code.
A large block of commented-out code (~40 lines) for 2-click and 3-click test scenarios suggests this test may be incomplete. Please clarify the intent:
- If only testing the 1-click scenario is sufficient (like
basic-solid-query-file-based/tests/transition.spec.ts), remove the commented code. - If comprehensive multi-click testing is needed (like
basic-file-based/tests/transition.spec.ts), uncomment and enable these test cases.
Dead code reduces maintainability and creates confusion about the test's intended scope.
🏁 Script executed:
#!/bin/bash
# Locate and examine the target test file
find . -type f -name "transition.spec.ts" | grep -E "e2e/(solid-start|solid-router)" | head -20Length of output: 243
🏁 Script executed:
#!/bin/bash
# Display the content of the target file to verify the commented code
cat "e2e/solid-start/basic-solid-query/tests/transition.spec.ts"Length of output: 2654
🏁 Script executed:
#!/bin/bash
# Look for reference test files mentioned in the review
find . -type f -path "*/e2e/*/tests/transition.spec.ts" | sortLength of output: 377
🏁 Script executed:
#!/bin/bash
# Compare the basic-file-based test (comprehensive multi-click)
cat "e2e/solid-router/basic-file-based/tests/transition.spec.ts"Length of output: 2578
🏁 Script executed:
#!/bin/bash
# Compare the basic-solid-query-file-based test (potentially simpler)
cat "e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts"Length of output: 2558
🏁 Script executed:
#!/bin/bash
# Also check solid-start/basic for comparison
cat "e2e/solid-start/basic/tests/transition.spec.ts"Length of output: 2578
Uncomment the disabled test scenarios to complete the multi-click test coverage.
The commented 2-click and 3-click sections (lines 41-82) represent genuine dead code. All peer test files—e2e/solid-router/basic-file-based/tests/transition.spec.ts, e2e/solid-router/basic-solid-query-file-based/tests/transition.spec.ts, and e2e/solid-start/basic/tests/transition.spec.ts—include these same scenarios in active (uncommented) form. The target file's incomplete state is inconsistent with the established pattern across the e2e test suite. Either uncomment these sections to match the comprehensive multi-click coverage pattern used elsewhere, or remove them if simplification is intentional (though the evidence strongly suggests they should be active).
🤖 Prompt for AI Agents
In e2e/solid-start/basic-solid-query/tests/transition.spec.ts around lines 41 to
83, the 2-click and 3-click test scenarios are commented out causing missing
multi-click coverage; uncomment those blocks so the test performs the two-click
and three-click sequences (invoke page.getByTestId('increase-button').click()
the correct number of times, keep the await expect(...) assertions and the
intermediate await page.waitForTimeout(200)) to match the other e2e test files,
run the test to confirm timing/assertions pass, and remove the comment markers
only (do not alter the assertions or waits).
Summary by CodeRabbit
New Features
Tests
Refactor
Chores