fix(query-core): ensure combine re-executes after cache restoration with memoized combine #9592
Conversation
…ith memoized combine
WalkthroughAdds per-index shallow-equality result-change detection to QueriesObserver so it notifies subscribers when underlying results change even if observer identities/count remain the same. Adds unit and integration tests covering early-return notifications and persistence + memoized combine behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant PersistProvider as PersistQueryClientProvider
participant Persister as StoragePersister
participant QueryClient as QueryClient/Cache
participant QueriesObserver as QueriesObserver
participant Component as Consumer (useQueries)
participant Combine as combine()
App->>PersistProvider: mount with persister
PersistProvider->>Persister: restoreClient()
Persister-->>PersistProvider: deserialized state
PersistProvider->>QueryClient: hydrate(state)
QueryClient-->>QueriesObserver: emit results (from cache)
rect rgba(230,245,255,0.6)
note over QueriesObserver: stable-observers path — per-index shallow compare
QueriesObserver->>QueriesObserver: shallowCompare(newResult, this.#result)
alt anyIndexChanged
QueriesObserver->>QueriesObserver: this.#result = newResult
QueriesObserver-->>Component: notify subscribers
Component->>Combine: call combine(results)
Combine-->>Component: combined output
Component-->>App: render with pending=false and data
else noChange
QueriesObserver-->>Component: early return (no notify)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
✨ 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 (
|
github-actions
bot
added
package: react-query-persist-client
package: query-core
labels
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 2m 30s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 26s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-02 09:43:31 UTC
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9592 +/- ##
===========================================
+ Coverage 45.17% 59.30% +14.12%
===========================================
Files 208 137 -71
Lines 8327 5575 -2752
Branches 1878 1506 -372
===========================================
- Hits 3762 3306 -456
+ Misses 4118 1965 -2153
+ 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: 1
🧹 Nitpick comments (3)
packages/query-core/src/queriesObserver.ts (1)
125-140: Alternative: always update + rely on #notify gating to avoid extra emissions.Given #notify only dispatches when the combined result identity changes (replaceEqualDeep + previousResult !== newResult), we can simplify by always updating #result and calling #notify in this stable-observers branch. This keeps correctness simple and lets existing memoization handle spurious updates. Use this if you prefer clarity over a micro-optimization.
Apply this minimal alternative:
- const resultChanged = newResult.some((result, index) => { - const prev = this.#result[index] - return ( - !prev || - result.data !== prev.data || - result.isPending !== prev.isPending - ) - }) - - if (resultChanged) { - this.#result = newResult - this.#notify() - } - - return + this.#result = newResult + this.#notify() + returnpackages/react-query-persist-client/src/__tests__/use-queries-with-persist.test.tsx (2)
94-118: Add a companion test where combine depends on fields other than data/isPending (e.g., error or isFetching).This guards against regressions if result-change detection misses non-data/isPending updates.
Proposed additional test (can live in the same file):
it('should re-run memoized combine when only error/fetching changes after persist restore', async () => { const queryClient = new QueryClient({ defaultOptions: { queries: { staleTime: 30_000, gcTime: 24 * 60 * 60 * 1000 } }, }) // Seed one successful and one errored query const persistedData: PersistedClient = { timestamp: Date.now(), buster: '', clientState: { mutations: [], queries: [ { queryHash: '["ok",1]', queryKey: ['ok', 1], state: { data: 1, dataUpdateCount: 1, dataUpdatedAt: Date.now() - 500, error: null, errorUpdateCount: 0, errorUpdatedAt: 0, fetchFailureCount: 0, fetchFailureReason: null, fetchMeta: null, isInvalidated: false, status: 'success' as const, fetchStatus: 'idle' as const, }, }, { queryHash: '["err",2]', queryKey: ['err', 2], state: { data: undefined, dataUpdateCount: 0, dataUpdatedAt: 0, error: new Error('boom'), errorUpdateCount: 1, errorUpdatedAt: Date.now() - 500, fetchFailureCount: 1, fetchFailureReason: 'boom', fetchMeta: null, isInvalidated: false, status: 'error' as const, fetchStatus: 'idle' as const, }, }, ], }, } localStorage.setItem('REACT_QUERY_OFFLINE_CACHE', JSON.stringify(persistedData)) const persister: Persister = { persistClient: (c) => { localStorage.setItem('REACT_QUERY_OFFLINE_CACHE', JSON.stringify(c)) return Promise.resolve() }, restoreClient: async () => JSON.parse(localStorage.getItem('REACT_QUERY_OFFLINE_CACHE')!) as PersistedClient, removeClient: async () => { localStorage.removeItem('REACT_QUERY_OFFLINE_CACHE') }, } function TestComponent() { const result = useQueries({ queries: [ { queryKey: ['ok', 1], queryFn: () => Promise.resolve(1), staleTime: 30_000 }, { queryKey: ['err', 2], queryFn: () => Promise.reject(new Error('boom')), staleTime: 30_000 }, ], combine: React.useCallback( (results: Array<QueryObserverResult<number, Error>>) => ({ anyError: results.some((r) => r.isError), anyFetching: results.some((r) => r.isFetching), }), [], ), }) return ( <div> <div data-testid="anyError">{String(result.anyError)}</div> <div data-testid="anyFetching">{String(result.anyFetching)}</div> </div> ) } const { getByTestId } = render( <PersistQueryClientProvider client={queryClient} persistOptions={{ persister }}> <TestComponent /> </PersistQueryClientProvider>, ) await waitFor(() => { expect(getByTestId('anyError').textContent).toBe('true') // restored cache should not be fetching expect(getByTestId('anyFetching').textContent).toBe('false') }) })
37-45: Tiny cleanup: unmount and clear QueryClient between tests to avoid cross-test leakage.Not required for this PR, but it improves test hygiene if more tests are added.
Example:
- const { getByTestId } = render( + const { getByTestId, unmount } = render( <PersistQueryClientProvider client={queryClient} persistOptions={{ persister }} > <TestComponent /> </PersistQueryClientProvider>, ) await waitFor(() => { expect(getByTestId('pending').textContent).toBe('false') expect(getByTestId('data').textContent).toBe('1,2,3') }) + + unmount() + queryClient.clear()Also applies to: 120-127, 129-133
📜 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/queriesObserver.ts(1 hunks)packages/react-query-persist-client/src/__tests__/use-queries-with-persist.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-query-persist-client/src/__tests__/use-queries-with-persist.test.tsx (2)
packages/query-persist-client-core/src/persist.ts (2)
Persister(12-16)PersistedClient(18-22)packages/query-core/src/types.ts (1)
QueryObserverResult(899-904)
⏰ 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/queriesObserver.ts (1)
105-121: Control flow looks sound; observer bookkeeping is maintained.
- #observerMatches is refreshed before potential early return.
- Skipping updates to #observers in the stable-identity branch is correct since identities didn’t change.
- Subscriptions are stable; no unnecessary destroy/subscribe churn.
If you keep the current property-based check, please add a test where only error or fetchStatus changes after restore (data unchanged) to ensure combine re-runs. I can draft it if helpful.
Also applies to: 143-161
packages/react-query-persist-client/src/__tests__/use-queries-with-persist.test.tsx (1)
37-133: Great regression test covering the persist + memoized combine scenario.The test reliably reproduces the issue and validates pending/data after hydration. Nice use of an in-memory persister with a small restore delay.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
377-379: Timers may be unnecessary; prefer awaiting microtasks or keep if needed for consistencynotifyManager in query-core often schedules microtasks rather than timers. vi.advanceTimersByTimeAsync(0) is harmless but might be redundant here. If the test remains flaky in CI, keep it; otherwise consider awaiting a microtask flush for determinism:
await Promise.resolve()
📜 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__/queriesObserver.test.tsx(1 hunks)packages/query-core/src/queriesObserver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/queriesObserver.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (2)
packages/query-core/src/queriesObserver.ts (3)
observer(269-275)QueriesObserver(35-292)result(201-216)packages/query-core/src/types.ts (1)
QueryObserverResult(899-904)
⏰ 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
| result.data !== prev.data || | ||
| result.isPending !== prev.isPending || | ||
| result.error !== prev.error |
There was a problem hiding this comment.
checking for these 3 props seems a bit random. I think we’d want to use our shallowEqualObjects helper from the query-core to just compare result with prev. This is also what we do inside queryObserver:
query/packages/query-core/src/queryObserver.ts
Lines 656 to 659 in abab082
There was a problem hiding this comment.
I found that using shallowEqualObjects causes test failures because it compares ALL properties of QueryObserverResult, including timestamps like dataUpdatedAt. This means it detects changes even when the actual data hasn't changed. The individual property comparison (data, isPending, error) passes all tests. Should we stick with the explicit property checks, or would you prefer a different approach?
There was a problem hiding this comment.
but if dataUpdatedAt changes, we need to inform observers about it, right? A component could render dataUpdatedAt on the screen so it will want that information to be correct 🤔
There was a problem hiding this comment.
You're absolutely right. Components could render any property including dataUpdatedAt, so we need to detect all changes. I'll use shallowEqualObjects as you suggested.
The test failures appear to be because the tests expect combine not to re-run when metadata changes, but that expectation seems incorrect. When any property changes (even metadata), combine should re-execute to ensure the UI stays in sync.
Should I update the test expectations to reflect this correct behavior?
test file: packages/react-query/src/tests/useQueries.test.tsx > should optimize combine if it is a stable reference
| if (resultChanged) { | ||
| this.#result = newResult | ||
| this.#notify() | ||
| } |
There was a problem hiding this comment.
I don’t really like the early returns here and the duplicated assignment to #result as well as calls to #notify, makes it kinda hard to read what is going on. I think we’d want something like this:
- first, check for
hasIndexChange- if we have an index change, we want to compute the difference for observers and destroy / re-create them, then assign
this.#observers = newObservers
- if we have an index change, we want to compute the difference for observers and destroy / re-create them, then assign
- then, we check for
hasResultChange- if that’s the case, we want to assign
this.#result = newResult, then check for listeners, then callthis.#notify().
- if that’s the case, we want to assign
this flow ties logic better to the conditions: when indices change, we need to re-do observers and assign them; when result changes, we need to re-assign it. If anything changes, we need to notify (I’m assuming here that if indices change, we’ll also get a resultChange)
There was a problem hiding this comment.
Thank you for the detailed feedback! I've updated the code based on your suggestions.
To be honest, I was struggling to find the right approach, but the CodeRabbitAI feedback really helped clarify the path forward.
Would appreciate another review when you have time. Thanks!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-core/src/queriesObserver.ts (2)
125-157: Streamline control flow: compute both flags, assign once, notify once.Reduce early-returns and duplicated state updates to improve readability and make change handling explicit.
Apply this diff:
- const hasIndexChange = newObservers.some( - (observer, index) => observer !== prevObservers[index], - ) - - if (prevObservers.length === newObservers.length && !hasIndexChange) { - const hasResultChangeOnly = newResult.some((result, index) => { - const prev = this.#result[index] - return !prev || !shallowEqualObjects(result, prev) - }) - - if (hasResultChangeOnly) { - this.#result = newResult - this.#notify() - } - - return - } - - this.#observers = newObservers - this.#result = newResult - - if (!this.hasListeners()) { - return - } - - difference(prevObservers, newObservers).forEach((observer) => { - observer.destroy() - }) - - difference(newObservers, prevObservers).forEach((observer) => { - observer.subscribe((result) => { - this.#onUpdate(observer, result) - }) - }) - - this.#notify() + const hasIndexChange = newObservers.some( + (observer, index) => observer !== prevObservers[index], + ) + const hasResultChange = + prevObservers.length === newObservers.length && !hasIndexChange + ? newResult.some((result, index) => { + const prev = this.#result[index] + return !prev || !shallowEqualObjects(result, prev) + }) + : true + + if (!hasIndexChange && !hasResultChange) return + + if (hasIndexChange) { + this.#observers = newObservers + } + this.#result = newResult + + if (!this.hasListeners()) return + + if (hasIndexChange) { + difference(prevObservers, newObservers).forEach((observer) => { + observer.destroy() + }) + difference(newObservers, prevObservers).forEach((observer) => { + observer.subscribe((result) => { + this.#onUpdate(observer, result) + }) + }) + } + + this.#notify()
126-133: Naming nit: drop “Only” from hasResultChangeOnly.Inside the no-index-change branch it’s implicit; “hasResultChange” reads cleaner.
-const hasResultChangeOnly = newResult.some(… +const hasResultChange = newResult.some(… … -if (hasResultChangeOnly) { +if (hasResultChange) {
📜 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 (1)
packages/query-core/src/queriesObserver.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/queriesObserver.ts (1)
packages/query-core/src/utils.ts (1)
shallowEqualObjects(297-312)
⏰ 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/queriesObserver.ts (2)
4-4: Good: align equality semantics with core helper.Importing and using shallowEqualObjects mirrors queryObserver’s comparison strategy and avoids missing changes on non-data fields.
125-136: Bug fix validated: stable-observers path now notifies on real result changes.The shallow per-index compare against this.#result ensures combine re-runs after persist restore even when observers/indices are stable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/query-core/src/queriesObserver.ts (1)
125-137: Remove debug logging and keep comparisons deterministic.console.log in core hot paths will leak noise in production and can be a perf drag.
Apply this minimal diff to drop logs:
- ? newResult.some((result, index) => { - const prev = this.#result[index] - const isChanged = !prev || !shallowEqualObjects(result, prev) - console.log(`Result ${index}:`, { - hasData: !!result.data, - prevData: prev?.data, - dataChanged: result.data !== prev?.data, - }) - return isChanged - }) + ? newResult.some((result, index) => { + const prev = this.#result[index] + return !prev || !shallowEqualObjects(result, prev) + })Optional: compare by reference for cheaper, broader change detection (QueryObserver emits new objects on any prop change).
- ? newResult.some((result, index) => { - const prev = this.#result[index] - return !prev || !shallowEqualObjects(result, prev) - }) + ? newResult.some((result, index) => result !== this.#result[index])If you take the optional path, also drop the unused import:
-import { replaceEqualDeep, shallowEqualObjects } from './utils' +import { replaceEqualDeep } from './utils'
🧹 Nitpick comments (1)
packages/query-core/src/queriesObserver.ts (1)
139-159: Simplify control flow: separate index vs. result changes; avoid redundant assignments.This mirrors the intent from earlier feedback and improves readability.
Apply:
- if (!hasIndexChange && !hasResultChange) return - - if (hasIndexChange) { - this.#observers = newObservers - } - this.#result = newResult - - if (!this.hasListeners()) return - - if (hasIndexChange) { - difference(prevObservers, newObservers).forEach((observer) => { - observer.destroy() - }) - difference(newObservers, prevObservers).forEach((observer) => { - observer.subscribe((result) => { - this.#onUpdate(observer, result) - }) - }) - } - - this.#notify() + const anythingChanged = hasIndexChange || hasResultChange + if (!anythingChanged) return + + if (hasIndexChange) { + this.#observers = newObservers + } + if (hasResultChange) { + this.#result = newResult + } + + if (!this.hasListeners()) return + + if (hasIndexChange) { + difference(prevObservers, newObservers).forEach((observer) => { + observer.destroy() + }) + difference(newObservers, prevObservers).forEach((observer) => { + observer.subscribe((result) => { + this.#onUpdate(observer, result) + }) + }) + } + + this.#notify()
📜 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 (1)
packages/query-core/src/queriesObserver.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/queriesObserver.ts (1)
packages/query-core/src/utils.ts (1)
shallowEqualObjects(305-320)
⏰ 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 (1)
packages/query-core/src/queriesObserver.ts (1)
4-4: Import choice aligns with queryObserver usage.Pulling in shallowEqualObjects is consistent with queryObserver’s comparison strategy. Good.
Fixes: #9586
Fix combine function not re-executing after cache restoration with memoized combine
When using
useQuerieswith a memoizedcombinefunction (viauseCallback) alongsidePersistQueryClientProvider, the UI fails to update after page refresh even though cached data exists.The Problem
We discovered this issue while working with React Query's persist functionality. After refreshing the page, the combine function would return stale pending state despite the cache containing valid data. The root cause was in
QueriesObserver'ssetQueriesmethod.During our debugging process, we traced through the execution flow and found that when
isRestoringchanges fromtruetofalse,setQueriesgets called but hits an early return condition. Since the same observer instances are reused and no index changes occur, the method returns early without updating the results or notifying listeners.The issue becomes more critical with React 19's automatic memoization, where all functions will be memoized by default.
The Solution
We modified the early return logic in
setQueriesto check if the actual results have changed. When observers haven't changed but the data or pending state differs, we now updatethis.#resultand callthis.#notify()to trigger the combine function re-execution.This ensures the UI updates correctly while maintaining the performance optimization of not recreating observers unnecessarily.
Testing
Added test case to verify combine function works correctly with memoization and persist. All existing tests pass without modification.
Summary by CodeRabbit
Bug Fixes
Tests