Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit de2ad93
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
size-limit report 📦
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/svelte-query/tests/createQueries/createQueries.svelte.test.ts`:
- Around line 19-20: The shared teardown afterEach currently only calls
queryClient.clear(), so when tests switch to fake timers (e.g. the test at line
939) a failing test can leave Vitest in fake-timer mode; update the afterEach
block to also call vi.useRealTimers() to ensure timers are reset on every
teardown — modify the afterEach that references queryClient.clear() to invoke
vi.useRealTimers() alongside clearing the query client.
- Around line 23-25: The test helper withEffectRoot in
packages/svelte-query/tests/utils.svelte.ts currently calls cleanup() only after
await promise succeeds; change it so cleanup() runs regardless of promise
resolution by wrapping the awaited call in a try/finally (or use
promise.finally) that always calls cleanup(), ensuring the effect root is
unsubscribed even when the wrapped async function throws or rejects; target the
withEffectRoot wrapper and the existing cleanup() call to implement this
unconditional cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 398c3a20-9817-449f-8c28-7ef99ff5f129
📒 Files selected for processing (3)
packages/svelte-query/tests/createQueries.svelte.test.tspackages/svelte-query/tests/createQueries/createQueries.svelte.test.tspackages/svelte-query/tests/createQueries/createQueries.test-d.ts
💤 Files with no reviewable changes (1)
- packages/svelte-query/tests/createQueries.svelte.test.ts
| afterEach(() => { | ||
| vi.useRealTimers() | ||
| queryClient.clear() |
There was a problem hiding this comment.
Reset Vitest timers in the shared teardown.
Line 939 switches this suite to fake timers, but the reset currently happens only at the tail of that one test. If any earlier assertion throws, later cases inherit fake timers and fail for unrelated reasons. Add vi.useRealTimers() to afterEach so cleanup is failure-safe.
🧹 Proposed fix
afterEach(() => {
queryClient.clear()
+ vi.useRealTimers()
})📝 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.
| afterEach(() => { | |
| vi.useRealTimers() | |
| queryClient.clear() | |
| afterEach(() => { | |
| queryClient.clear() | |
| vi.useRealTimers() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/svelte-query/tests/createQueries/createQueries.svelte.test.ts`
around lines 19 - 20, The shared teardown afterEach currently only calls
queryClient.clear(), so when tests switch to fake timers (e.g. the test at line
939) a failing test can leave Vitest in fake-timer mode; update the afterEach
block to also call vi.useRealTimers() to ensure timers are reset on every
teardown — modify the afterEach that references queryClient.clear() to invoke
vi.useRealTimers() alongside clearing the query client.
| it( | ||
| 'should return the correct states', | ||
| withEffectRoot(async () => { |
There was a problem hiding this comment.
withEffectRoot needs failure-safe cleanup before broad reuse.
packages/svelte-query/tests/utils.svelte.ts:24-32 only calls cleanup() after await promise succeeds. Any rejected promise or failed expectation in these new withEffectRoot(...) cases skips cleanup and leaves the effect root subscribed for later tests.
🔧 Proposed fix in packages/svelte-query/tests/utils.svelte.ts
export function withEffectRoot(fn: () => void | Promise<void>) {
return async () => {
let promise: void | Promise<void> = Promise.resolve()
const cleanup = $effect.root(() => {
promise = fn()
})
- await promise
- cleanup()
+ try {
+ await promise
+ } finally {
+ cleanup()
+ }
}
}📝 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.
| it( | |
| 'should return the correct states', | |
| withEffectRoot(async () => { | |
| export function withEffectRoot(fn: () => void | Promise<void>) { | |
| return async () => { | |
| let promise: void | Promise<void> = Promise.resolve() | |
| const cleanup = $effect.root(() => { | |
| promise = fn() | |
| }) | |
| try { | |
| await promise | |
| } finally { | |
| cleanup() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/svelte-query/tests/createQueries/createQueries.svelte.test.ts`
around lines 23 - 25, The test helper withEffectRoot in
packages/svelte-query/tests/utils.svelte.ts currently calls cleanup() only after
await promise succeeds; change it so cleanup() runs regardless of promise
resolution by wrapping the awaited call in a try/finally (or use
promise.finally) that always calls cleanup(), ensuring the effect root is
unsubscribed even when the wrapped async function throws or rejects; target the
withEffectRoot wrapper and the existing cleanup() call to implement this
unconditional cleanup.
1 participant
🎯 Changes
createQueries.svelte.test.tsandcreateQueries.test-d.tsintocreateQueries/directoryIsRestoringExample.sveltetest component from test(svelte-query/createQueries): add test for not fetching when 'isRestoring' is true #10382../src/to../../src/and./utilsto../utils✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes