fix(query-core): prevent state override when observer remount occurs with signal consumption #9580
Conversation
…with signal consumption
WalkthroughAdds two tests for observer churn and overlapping fetches; tags observer-removal cancellations with isObserverRemoval; changes query fetch-error handling to return cached data when an observer-removal cancellation occurs and cached data exists; adds isObserverRemoval to CancelledError and CancelOptions. No public method signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant O as Observer1
participant Q as Query
participant R as Retryer
participant O2 as Observer2
O->>Q: subscribe() / trigger fetch A
Q->>R: start(fetch A, signalA)
Note over O,Q: Observer1 unmounts (StrictMode churn)
O-->>Q: unsubscribe()
Q->>R: cancel({revert: true, isObserverRemoval: true})
O2->>Q: subscribe() / trigger fetch B
Q->>R: start(fetch B, signalB)
R-->>Q: throw CancelledError{revert:true,isObserverRemoval:true} (for A)
alt isObserverRemoval && observers exist
alt state.data defined
Q-->>O2: keep fetchStatus 'fetching' and return cached data
else
Q-->>O2: rethrow CancelledError
end
else
Q->>Q: revert to #revertState; set fetchStatus 'idle'
end
R-->>Q: success for B -> Q updates state to data from B
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 (
|
|
View your CI Pipeline Execution ↗ for commit a69008b
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9580 +/- ##
===========================================
+ Coverage 45.15% 59.25% +14.10%
===========================================
Files 208 137 -71
Lines 8323 5566 -2757
Branches 1886 1495 -391
===========================================
- Hits 3758 3298 -460
+ Misses 4118 1964 -2154
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/query-core/src/types.ts (1)
1329-1333: Add brief JSDoc for the new CancelOptions.isObserverRemoval flagNice, the new flag threads intent through the cancel pipeline. Since CancelOptions is part of the public types, add a short JSDoc to clarify internal usage so consumers don’t rely on it.
export interface CancelOptions { revert?: boolean silent?: boolean + /** + * Internal: marks cancellation caused by observer removal to disambiguate revert behavior. + * Not intended for external usage. + * @internal + */ isObserverRemoval?: boolean }packages/query-core/src/query.ts (1)
556-573: Avoid reverting to idle when a new active observer exists; refine the guardThe new guard prevents an async revert from overriding an in-flight fetch after a quick re-subscribe. This addresses the StrictMode remount issue.
To avoid edge cases where a disabled observer re-subscribes (enabled: false) and we still skip revert (leaving fetchStatus stuck at 'fetching' without an active fetch), consider checking for an active observer rather than just a non-empty list.
- if (error.isObserverRemoval && this.observers.length > 0) { + if (error.isObserverRemoval && this.isActive()) { if (this.state.data === undefined) { throw error } return this.state.data }Optionally, add a short comment explaining why revert is skipped to aid future maintainers.
packages/query-core/src/__tests__/query.test.tsx (1)
1195-1239: Remove ts-expect-error by typing the mock; clarify intentThe test relies on destructuring signal to mark the abortSignal as consumed. You can avoid the ts-expect-error by typing the mock to QueryFunction and keep the test self-documenting.
- // @ts-expect-error This field has been added for troubleshooting purposes. Disable ts error for testing. - const queryFn = vi.fn(async ({ signal }) => { + const queryFn = vi.fn<QueryFunction<string, ReturnType<typeof queryKey>>>(async ({ signal }) => { await sleep(50) return 'data' })Additionally, add the missing type import at the top of the file:
// augment existing import type block from '..' import type { QueryCache, QueryFunctionContext, QueryFunction, // <- add this QueryKey, QueryObserverResult, } from '..'This keeps the test free of suppression comments and makes the "signal consumption" intent explicit.
If you'd like, I can also contribute a complementary test that covers the edge-case where a disabled observer re-subscribes to ensure we still revert correctly.
📜 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 (4)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)packages/query-core/src/retryer.ts(1 hunks)packages/query-core/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/query-core/src/retryer.ts (1)
packages/query-core/src/types.ts (1)
CancelOptions(1329-1333)
packages/query-core/src/__tests__/query.test.tsx (1)
packages/query-core/src/utils.ts (1)
sleep(355-359)
⏰ 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)
58-67: Propagate observer-removal context via CancelledErrorCapturing isObserverRemoval on CancelledError is a clean way to alter downstream revert behavior without changing existing call sites. Looks good.
packages/query-core/src/query.ts (1)
350-355: Emit isObserverRemoval on cancel when signal was consumedGood call to annotate the cancel originating from observer removal. This enables correct differentiation in the catch path and prevents unintended state reverts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/query-core/src/__tests__/query.test.tsx (2)
1196-1237: Great regression test; consider asserting the cancellation type and avoiding microtask timing flakinessThis test reliably reproduces the StrictMode remount race and validates the intended behavior. Two small improvements:
- Assert the first fetch rejects with CancelledError (parity with the second test).
- Replace the microtask flush with waitFor to avoid timing flakiness tied to async cancel scheduling.
Apply this diff:
- await promise1.catch(() => {}) - - await Promise.resolve() + await expect(promise1).rejects.toBeInstanceOf(CancelledError) + await vi.waitFor(() => + expect(query.state.fetchStatus).toBe('fetching'), + )
1199-1203: Minor: add a clarifying comment about signal destructuringDestructuring
signalintentionally consumes the AbortSignal getter to set#abortSignalConsumed = true. A short comment will make the intent obvious for future readers.- const queryFn = vi.fn(async ({ signal: _signal }) => { + 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' })packages/query-core/src/query.ts (1)
556-573: Correctly avoid revert when cancellation stems from observer removal and observers are activeThe guard
if (error.isObserverRemoval && this.isActive())is the key to preventing the premature flip to 'idle'. Behavior looks correct:
- With active observers, skip revert. If there’s no cached data, rethrow to let the caller handle cancellation. If cached data exists, resolve with it without altering
fetchStatus, preserving the in-progress state for the next fetch.- Otherwise, revert state and set
fetchStatus: 'idle'(previous semantics).Two notes:
- Using
isActive()(instead of justobservers.length > 0) is more precise: it avoids skipping revert when only disabled observers are present and no new fetch will start. Good call.- Consider adding a brief comment explaining why we skip the revert here (to prevent the first fetch’s async cancellation from overriding the state after a new observer triggers a second fetch), for future maintainers.
Suggested inline comment:
- if (error.isObserverRemoval && this.isActive()) { + // If cancellation was caused by observer removal and there are active observers again, + // do not revert to idle: a new fetch may already be in flight, and reverting would + // incorrectly flip isLoading/isFetching to false under StrictMode remounts. + if (error.isObserverRemoval && this.isActive()) {
📜 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 (2)
packages/query-core/src/__tests__/query.test.tsx(1 hunks)packages/query-core/src/query.ts(2 hunks)
⏰ 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: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/query-core/src/__tests__/query.test.tsx (1)
1239-1280: Strong coverage for “no data” path; semantics match the fixThis test exercises the case where the first fetch is cancelled with revert (due to observer removal), no cached data exists, and a new observer immediately triggers a fresh fetch. It correctly asserts:
- The first promise rejects with CancelledError.
- fetchStatus remains 'fetching'.
- New data resolves as expected.
Looks good.
packages/query-core/src/query.ts (2)
347-356: Tag observer-removal cancels to disambiguate revert behaviorPassing
isObserverRemoval: truealongsiderevert: trueis the right lever to differentiate StrictMode cleanup from explicit user cancel. This enables fetch’s catch to avoid reverting state when a new observer is already active.
240-244: All flags and types verified
CancelOptions(packages/query-core/src/types.ts:1332) now declaresisObserverRemoval?: boolean.CancelledError(packages/query-core/src/retryer.ts:61–66) declaresisObserverRemoval?: booleanand assigns it in the constructor.- The internal
retryer.cancel({ revert: true })call inpackages/query-core/src/query.ts(line 352) correctly passesisObserverRemoval: true.No further changes needed.
|
It seems that an error unrelated to this PR is occurring at the CI stage. |
|
Not sure about this fix. I'll have to think a bit about it but I'm on vacation now. |
joseph0926
changed the title
| return this.#retryer.promise | ||
| } else if (error.revert) { | ||
| // If cancellation was caused by observer removal and there are active observers again, | ||
| // do not revert to idle: a new fetch may already be in flight, and reverting would |
There was a problem hiding this comment.
I don’t think this is a good enough check and can easily introduce regressions again. For example, you could mount an observer that won’t trigger a fetch, like a disabled one or one that has refetchOnMount: false.
There was a problem hiding this comment.
Although it's already closed, I've made the fix based on your feedback.
Since isActive() checks “Is there an active observer?”, while observers.length > 0 checks “Does an observer exist?”, this if-check is indeed correct.
if (error.isObserverRemoval && this.observers.length > 0) {
if (this.state.data === undefined) {
throw error
}
return this.state.data
}// query.test.tsx
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()
})
|
I think I have a better fix here: but I kept your test so thank you 🙏 |
2 participants
fixes: #9579
comment: #9579 (comment)
Description
This PR fixes an issue where
isLoadingincorrectly becomesfalseduring an active fetch when using React StrictMode with signal destructuring in the queryFn.Problem
This issue does not appear to be solely related to
StrictMode, but rather occurs when usingsignalwithin thequeryFnunderStrictMode.In fact, if you remove the
signalfrom the provided code as shown below,isLoadingwill correctly becometrue.The problem arises because when the
signaloption is present and the fetch starts, destructuring the signal setsabortSignalConsumed = truequery/packages/query-core/src/query.ts
Line 428 in 7c464e3
StrictMode, during cleanup the following condition is met insideremoveObserverquery/packages/query-core/src/query.ts
Lines 351 to 352 in 7c464e3
This triggers an asynchronous execution of
this.#retryer.cancel({ revert: true }).As a result, the sequence becomes: immediate re-mount → new observer added → second fetch begins → meanwhile, the asynchronous
catchlogic from the first fetch executes, which then hits the following branchquery/packages/query-core/src/query.ts
Lines 555 to 567 in 7c464e3
This causes the state to be overwritten with the initial values, leading to the observed issue.
The reason it did not occur in the previous version is that
onErrorexisted and handled the case, but now the logic is fully asynchronous and falls intocatch, which results in the problem you are seeing.Solution
The fix adds a check to prevent state reversion when
isObserverRemovalflag)this.observers.length > 0)This pattern is typical of React StrictMode's cleanup simulation where components are immediately remounted.
Changes
isObserverRemovalflag toCancelOptionsandCancelledErrorremoveObserverto pass this flag when cancelling with revertfetchmethod to check for this condition before reverting stateTesting
should not override fetching state when revert happens after new observer subscribesSummary by CodeRabbit
Bug Fixes
Types
Tests