-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(core): make sure queries revert synchronously #9601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
+64
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
the switch from the callback approach to async/await with catch was necessary to be able to transform errors into revert-state data for imperative query calls; however, this was a change in behavior because catch in async/await doesn't happen immediately - it runs in the next microtask. That opened up an opportunity for a slight race condition if we re-start a fetch in-between. And who does that? Of course, React Strict Mode. This PR moves the actual state reverting back to a callback (so it happens synchronously), while still keeping the try/catch refactoring merely to transform the promise and the usual error handling
WalkthroughIntroduces a new cancellation flow by replacing Retryer abort with onCancel, adjusts revert-on-cancel handling in query logic, and adds a test validating cancellation when resubscribing during an in-flight fetch. No other production files are modified. Changes
Sequence Diagram(s)sequenceDiagram
participant Obs1 as Observer #1
participant Query as Query
participant Retryer as Retryer
participant AC as AbortController
participant QFn as queryFn
Obs1->>Query: subscribe()
activate Query
Query->>AC: new AbortController()
Query->>Retryer: createRetryer({ onCancel })
Retryer->>QFn: invoke(queryFn, { signal: AC.signal })
activate QFn
Obs1-->>Query: unsubscribe() (during in-flight)
Retryer->>Retryer: cancel()
Retryer-->>Query: onCancel(CancelledError)
Note right of Query: If error.revert === true<br/>revert transient state
Query->>AC: abort()
deactivate QFn
Retryer-->>Obs1: reject(CancelledError)
participant Obs2 as Observer #2
Obs2->>Query: subscribe()
Query->>AC: new AbortController()
Query->>Retryer: createRetryer({ onCancel })
Retryer->>QFn: invoke(queryFn, { signal: AC.signal })
QFn-->>Retryer: resolve(data1)
Retryer-->>Query: success(data1)
Query-->>Obs2: notify(status: success, fetchStatus: idle, data1)
deactivate Query
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Closed
3 tasks
|
View your CI Pipeline Execution ↗ for commit 23d4346
☁️ Nx Cloud last updated this comment at |
More templates
- @tanstack/query-example-angular-auto-refetching
- @tanstack/query-example-angular-basic
- @tanstack/query-example-angular-basic-persister
- @tanstack/query-example-angular-devtools-panel
- @tanstack/query-example-angular-infinite-query-with-max-pages
- @tanstack/query-example-angular-optimistic-updates
- @tanstack/query-example-angular-pagination
- @tanstack/query-example-angular-query-options-from-a-service
- @tanstack/query-example-angular-router
- @tanstack/query-example-angular-rxjs
- @tanstack/query-example-angular-simple
- @tanstack/query-example-solid-astro
- @tanstack/query-example-solid-basic
- @tanstack/query-example-solid-basic-graphql-request
- @tanstack/query-example-solid-default-query-function
- @tanstack/query-example-solid-simple
- @tanstack/query-example-solid-start-streaming
- @tanstack/query-example-react-algolia
- @tanstack/query-example-react-auto-refetching
- @tanstack/query-example-react-basic
- @tanstack/query-example-react-basic-graphql-request
- @tanstack/query-example-chat
- @tanstack/query-example-react-default-query-function
- @tanstack/query-example-react-devtools-panel
- @tanstack/query-example-eslint-legacy
- @tanstack/query-example-react-infinite-query-with-max-pages
- @tanstack/query-example-react-load-more-infinite-scroll
- @tanstack/query-example-react-nextjs
- @tanstack/query-example-react-nextjs-app-prefetching
- @tanstack/query-example-nextjs-suspense-streaming
- @tanstack/query-example-react-offline
- @tanstack/query-example-react-optimistic-updates-cache
- @tanstack/query-example-react-optimistic-updates-ui
- @tanstack/query-example-react-pagination
- @tanstack/query-example-react-playground
- @tanstack/query-example-react-prefetching
- @tanstack/query-example-react-react-native
- @tanstack/query-example-react-router
- @tanstack/query-example-react-rick-morty
- @tanstack/query-example-react-shadow-dom
- @tanstack/query-example-react-simple
- @tanstack/query-example-react-star-wars
- @tanstack/query-example-react-suspense
- @tanstack/query-example-svelte-auto-refetching
- @tanstack/query-example-svelte-basic
- @tanstack/query-example-svelte-load-more-infinite-scroll
- @tanstack/query-example-svelte-optimistic-updates
- @tanstack/query-example-svelte-playground
- @tanstack/query-example-svelte-simple
- @tanstack/query-example-svelte-ssr
- @tanstack/query-example-svelte-star-wars
- @tanstack/query-example-vue-2.6-basic
- @tanstack/query-example-vue-2.7-basic
- @tanstack/query-example-vue-basic
- @tanstack/query-example-vue-dependent-queries
- @tanstack/query-example-vue-nuxt3
- @tanstack/query-example-vue-persister
- @tanstack/query-example-vue-simple
@tanstack/angular-query-devtools-experimental
npm i https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@9601
@tanstack/angular-query-experimental
npm i https://pkg.pr.new/@tanstack/angular-query-experimental@9601
@tanstack/eslint-plugin-query
npm i https://pkg.pr.new/@tanstack/eslint-plugin-query@9601
@tanstack/query-async-storage-persister
npm i https://pkg.pr.new/@tanstack/query-async-storage-persister@9601
@tanstack/query-broadcast-client-experimental
npm i https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@9601
@tanstack/query-core
npm i https://pkg.pr.new/@tanstack/query-core@9601
@tanstack/query-devtools
npm i https://pkg.pr.new/@tanstack/query-devtools@9601
@tanstack/query-persist-client-core
npm i https://pkg.pr.new/@tanstack/query-persist-client-core@9601
@tanstack/query-sync-storage-persister
npm i https://pkg.pr.new/@tanstack/query-sync-storage-persister@9601
@tanstack/react-query
npm i https://pkg.pr.new/@tanstack/react-query@9601
@tanstack/react-query-devtools
npm i https://pkg.pr.new/@tanstack/react-query-devtools@9601
@tanstack/react-query-next-experimental
npm i https://pkg.pr.new/@tanstack/react-query-next-experimental@9601
@tanstack/react-query-persist-client
npm i https://pkg.pr.new/@tanstack/react-query-persist-client@9601
@tanstack/solid-query
npm i https://pkg.pr.new/@tanstack/solid-query@9601
@tanstack/solid-query-devtools
npm i https://pkg.pr.new/@tanstack/solid-query-devtools@9601
@tanstack/solid-query-persist-client
npm i https://pkg.pr.new/@tanstack/solid-query-persist-client@9601
@tanstack/svelte-query
npm i https://pkg.pr.new/@tanstack/svelte-query@9601
@tanstack/svelte-query-devtools
npm i https://pkg.pr.new/@tanstack/svelte-query-devtools@9601
@tanstack/svelte-query-persist-client
npm i https://pkg.pr.new/@tanstack/svelte-query-persist-client@9601
@tanstack/vue-query
npm i https://pkg.pr.new/@tanstack/vue-query@9601
@tanstack/vue-query-devtools
npm i https://pkg.pr.new/@tanstack/vue-query-devtools@9601
commit: 23d4346
|
Sizes for commit 23d4346:
|
@coderabbitai
coderabbitai
bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/query-core/src/retryer.ts (1)
13-13: Type-safety and ordering: make onCancel explicitly handle CancelledError and run before reject.
- onCancel always receives a CancelledError; tightening the type clarifies intent and avoids downstream casts.
- Running onCancel before reject ensures synchronous side-effects (like revert) occur prior to any queued catch handlers, making the fix more robust across environments.
Apply this diff:
@@ interface RetryerConfig<TData = unknown, TError = DefaultError> { - onCancel?: (error: TError) => void + onCancel?: (error: CancelledError & TError) => void @@ - const cancel = (cancelOptions?: CancelOptions): void => { - if (!isResolved()) { - const error = new CancelledError(cancelOptions) as TError - reject(error) - - config.onCancel?.(error) - } - } + const cancel = (cancelOptions?: CancelOptions): void => { + if (!isResolved()) { + const error = new CancelledError(cancelOptions) as CancelledError & TError + // Run synchronous cancel side-effects (e.g., revert) before any queued catch handlers. + config.onCancel?.(error) + reject(error) + } + }Also applies to: 87-93
packages/query-core/src/query.ts (1)
510-518: Defensive revert: guard against undefined revertState and narrow error type.Today #revertState is set before creating the retryer, but a small guard makes this future-proof and avoids spreading undefined by accident.
Apply this diff:
- onCancel: (error) => { - if (error instanceof CancelledError && error.revert) { - this.setState({ - ...this.#revertState, - fetchStatus: 'idle' as const, - }) - } - abortController.abort() - }, + onCancel: (error) => { + if (error instanceof CancelledError) { + if (error.revert && this.#revertState) { + this.setState({ ...this.#revertState, fetchStatus: 'idle' as const }) + } + abortController.abort() + } + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)packages/query-core/src/retryer.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/query-core/src/__tests__/query.test.tsx (3)
packages/query-core/src/utils.ts (2)
sleep(355-359)hashQueryKeyByOptions(203-209)packages/query-core/src/query.ts (1)
Query(159-678)packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
packages/query-core/src/query.ts (1)
packages/query-core/src/retryer.ts (1)
CancelledError(58-66)
⏰ 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)
packages/query-core/src/retryer.ts (1)
10-21: LGTM: onCancel hook restores synchronous revert without touching retry semantics.Replacing abort with onCancel cleanly separates transport aborting from cancellation-side effects, aligning with the PR goal of synchronous revert. No behavioral regressions spotted in retry loop or pause/continue paths.
packages/query-core/src/query.ts (1)
561-563: Comment clarity is good; matches the new behavior.The "piggyback onto that promise" note reads well and aligns with the silent cancel path. No change needed.
Comment on lines
+1196
to
+1244
test('should not override fetching state when revert happens after new observer subscribes', async () => {
const key = queryKey()
let count = 0
const queryFn = vi.fn(async ({ signal: _signal }) => {
// Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path
await sleep(50)
return 'data' + count++
})
const query = new Query({
client: queryClient,
queryKey: key,
queryHash: hashQueryKeyByOptions(key),
options: { queryFn },
})
const observer1 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
})
query.addObserver(observer1)
const promise1 = query.fetch()
await vi.advanceTimersByTimeAsync(10)
query.removeObserver(observer1)
const observer2 = new QueryObserver(queryClient, {
queryKey: key,
queryFn,
})
query.addObserver(observer2)
query.fetch()
await expect(promise1).rejects.toBeInstanceOf(CancelledError)
await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle'))
expect(queryFn).toHaveBeenCalledTimes(2)
expect(query.state).toMatchObject({
fetchStatus: 'idle',
status: 'success',
data: 'data1',
})
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix flakiness and expectation mismatch in new revert-sync test.
- With fake timers active, waitFor won’t progress the 50ms sleep; advance timers instead to let the second fetch resolve.
- Assert that after a microtask tick the state remains "fetching" (proves revert no longer overrides a new fetch).
- The counter logic returns "data0" for the first completed call; make it pre-increment so the second fetch yields "data1" as asserted.
Apply this diff:
@@ - test('should not override fetching state when revert happens after new observer subscribes', async () => { + test('should not override fetching state when revert happens after new observer subscribes', async () => { @@ - let count = 0 + let count = 0 @@ - const queryFn = vi.fn(async ({ signal: _signal }) => { - // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path - await sleep(50) - return 'data' + count++ - }) + const queryFn = vi.fn(async ({ signal: _signal }) => { + // Destructure `signal` to intentionally consume it so observer-removal uses revert-cancel path + await sleep(50) + return 'data' + ++count + }) @@ - query.fetch() + query.fetch() + // Let any queued catch handlers run; fetching state must remain + await Promise.resolve() + expect(query.state.fetchStatus).toBe('fetching') @@ - await expect(promise1).rejects.toBeInstanceOf(CancelledError) - await vi.waitFor(() => expect(query.state.fetchStatus).toBe('idle')) + await expect(promise1).rejects.toBeInstanceOf(CancelledError) + await vi.advanceTimersByTimeAsync(50) + expect(query.state.fetchStatus).toBe('idle') @@ - expect(query.state).toMatchObject({ + expect(query.state).toMatchObject({ fetchStatus: 'idle', status: 'success', - data: 'data1', + data: 'data1', })
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/query-core/src/__tests__/query.test.tsx around lines 1196 to 1244,
the test is flaky because fake timers prevent the 50ms sleep from advancing, the
test doesn’t assert the intermediate fetching state after the revert/new fetch
overlap, and the counter returns the wrong data; replace the vi.waitFor that
expects the 50ms sleep to complete with vi.advanceTimersByTimeAsync(50) so the
second fetch can resolve, add a microtask tick (e.g., await Promise.resolve() or
equivalent) and assert query.state.fetchStatus === 'fetching' immediately after
to prove revert didn’t override the new fetch, and change the counter increment
to pre-increment so the second resolved call yields "data1" to match the final
expectation.
Hi @TkDodo , I was thinking about the synchronous revert approach and wondered if it might face a similar challenge to what you mentioned in my PR?
Your comment: #9580 (comment)
When onCancel executes synchronously during removeObserver, wouldn't the revert happen immediately before any new observer can be added? Here's a test case that might help illustrate this scenario
test('should prevent revert when disabled observer is added after removal', async () => { const key = queryKey() const queryFn = vi.fn(async ({ signal: _signal }) => { await sleep(50) return 'data' }) const query = new Query({ client: queryClient, queryKey: key, queryHash: hashQueryKeyByOptions(key), options: { queryFn }, }) const observer1 = new QueryObserver(queryClient, { queryKey: key, queryFn, }) query.addObserver(observer1) const promise1 = query.fetch() await vi.advanceTimersByTimeAsync(10) query.removeObserver(observer1) const observer2 = new QueryObserver(queryClient, { queryKey: key, queryFn, enabled: false, }) query.addObserver(observer2) await expect(promise1).rejects.toBeInstanceOf(CancelledError) expect(query.state.fetchStatus).toBe('fetching') expect(query.state.data).toBeUndefined() })
It seems like the synchronous execution might revert the state before the disabled observer gets added, which could lose the fetch state even though no new fetch was initiated.
Perhaps checking this.observers.length > 0 instead of this.isActive() might address both concerns? Since a disabled observer still indicates the component is using the query (just not actively fetching), wouldn't it make sense to preserve the state in that case? I'm curious what you think about this approach - it might handle both the original StrictMode issue and the disabled observer scenario.
When onCancel executes synchronously during removeObserver, wouldn't the revert happen immediately before any new observer can be added?
yes, it would, but that is what we want. The flow is this:
- observer gets added, we fetch
- observer gets removed, we cancel and revert the query back into
fetchStatus: 'idle' - new observer gets mounted
now, the new observer can start from a "clean state" and will get us into fetchStatus: 'fetching' if the observer triggers a fetch, or it will keep us in idle state if its disabled.
before, we had this flow:
- observer gets added, we fetch
- observer gets removed, revert gets "scheduled" but isn’t done.
- new observer gets mounted, initiates fetch
- revert happens and sets us back to
fetchStatus: 'idle', which is not what we want.
the test case you showed is wrong because query.addObserver alone doesn’t trigger anything. To initiate a fetch, we need observer.subscribe() or query.fetch. So you can’t expect the query to be in fetchStatus: 'fetching' by just calling query.addObserver.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #9601 +/- ## =========================================== + Coverage 45.15% 59.26% +14.11% =========================================== Files 208 137 -71 Lines 8323 5565 -2758 Branches 1886 1494 -392 =========================================== - Hits 3758 3298 -460 + Misses 4118 1963 -2155 + Partials 447 304 -143 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
the switch from the callback approach to async/await with catch was necessary to be able to transform errors into revert-state data for imperative query calls; however, this was a change in behavior because catch in async/await doesn't happen immediately - it runs in the next microtask. That opened up an opportunity for a slight race condition if we re-start a fetch in-between. And who does that? Of course, React Strict Mode.
This PR moves the actual state reverting back to a callback (so it happens synchronously), while still keeping the try/catch refactoring merely to transform the promise and the usual error handling
fixes #9579
Summary by CodeRabbit