fix(vue-query/useBaseQuery): prevent dual error propagation when 'suspense()' and error watcher both handle the same error#10234
Conversation
…pense()' and error watcher both handle the same error
🦋 Changeset detectedLatest commit: 4ecefd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 22s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-14 01:04:05 UTC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR prevents duplicate error propagation when Vue suspense and an error watcher both handle the same error by tracking in-flight suspense fetches and suppressing redundant throws; tests added for suspense and non-suspense error flows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue-query/src/useBaseQuery.ts (1)
152-176:⚠️ Potential issue | 🟠 MajorUse a ref-count for suspense fetches instead of a shared boolean.
isSuspenseFetchingonly tracks one bit of state for the whole observer. Ifsuspense()is called again before a previous suspense-driven fetch settles, the first completion flips this back tofalsewhile another suspense fetch is still in flight, so the watcher can start throwing again and reintroduce the dual-propagation bug. A counter keeps the guard correct for overlapping calls.Proposed fix
- let isSuspenseFetching = false + let suspenseFetchCount = 0 @@ - isSuspenseFetching = true + suspenseFetchCount += 1 observer.fetchOptimistic(defaultedOptions.value).then( (result) => { - isSuspenseFetching = false + suspenseFetchCount -= 1 resolve(result) }, (error: TError) => { - isSuspenseFetching = false + suspenseFetchCount -= 1 if ( shouldThrowError(defaultedOptions.value.throwOnError, [ error, @@ - if (shouldThrow && !isSuspenseFetching) { + if (shouldThrow && suspenseFetchCount === 0) { throw error }Also applies to: 207-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vue-query/src/useBaseQuery.ts` around lines 152 - 176, Replace the shared boolean isSuspenseFetching in the suspense() function with a ref-count (e.g., suspenseFetchCount) so overlapping suspense-driven fetches are tracked correctly: increment the counter right before calling observer.fetchOptimistic(...) and decrement it in both the success and error handlers (and any early exits), and derive the boolean guard as (suspenseFetchCount > 0) where needed; update the same pattern used later in the file (the other suspense-related block that currently uses isSuspenseFetching) so every start/increment has a matching decrement on all code paths to avoid premature clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/vue-query/src/useBaseQuery.ts`:
- Around line 152-176: Replace the shared boolean isSuspenseFetching in the
suspense() function with a ref-count (e.g., suspenseFetchCount) so overlapping
suspense-driven fetches are tracked correctly: increment the counter right
before calling observer.fetchOptimistic(...) and decrement it in both the
success and error handlers (and any early exits), and derive the boolean guard
as (suspenseFetchCount > 0) where needed; update the same pattern used later in
the file (the other suspense-related block that currently uses
isSuspenseFetching) so every start/increment has a matching decrement on all
code paths to avoid premature clearing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5273af4e-bcdc-447d-a076-e9ed3278acb6
📒 Files selected for processing (4)
.changeset/wide-camels-jog.mdpackages/vue-query/src/__tests__/useInfiniteQuery.test.tspackages/vue-query/src/__tests__/useQuery.test.tspackages/vue-query/src/useBaseQuery.ts
…n with ref-count to handle overlapping suspense fetches
|
@DamianOsipiuk Could you also review this PR? |
| }) | ||
|
|
||
| // Suppress the Unhandled Rejection caused by watcher throw in Vue 3 | ||
| const rejectionHandler = () => {} |
There was a problem hiding this comment.
Should this be a spy with assertion at the end?
There was a problem hiding this comment.
@DamianOsipiuk Good catch — switched the handler to vi.fn() and asserted it was called once with the rejected error in 9e2924c.
| // throwOnError is evaluated in both suspense() and the error watcher | ||
| expect(throwOnErrorFn).toHaveBeenCalledTimes(2) | ||
| // but the error watcher should not throw when suspense is active | ||
| expect(query).toMatchObject({ |
There was a problem hiding this comment.
This actually does not assert, what comment suggests.
Should there be a spy on unhandledRejection with assertion that it was not caled?
There was a problem hiding this comment.
@DamianOsipiuk You're right — the previous assertion did not match the comment's intent. In 9e2924c I added a vi.fn() spy on unhandledRejection and asserted not.toHaveBeenCalled() so the watcher's non-rethrow is directly verified.
| }) | ||
|
|
||
| // Suppress the Unhandled Rejection caused by watcher throw in Vue 3 | ||
| const rejectionHandler = () => {} |
There was a problem hiding this comment.
same for these tests
There was a problem hiding this comment.
@DamianOsipiuk Applied the same spy pattern to both useQuery tests in 9e2924c (and changed test → it for consistency with the rest of the file).
…r-propagation # Conflicts: # packages/vue-query/src/__tests__/useInfiniteQuery.test.ts
…on' spy and use 'it' for consistency
2 participants
🎯 Changes
When using
throwOnError: truewithsuspense(), the same error was propagated through two paths simultaneously:suspense()→fetchOptimisticfailure →reject(error)(Promise rejection)state.errorchange detected →throw error(watcher throw)This caused an Unhandled Rejection because both paths fired concurrently due to Vue's async watcher system.
Added
isSuspenseFetchingflag to coordinate betweensuspense()and the error watcher. Whensuspense()is actively handling a fetch error, the error watcher skips itsthrow, ensuring the error is propagated through only one path (Promise rejection).✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chore