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 subscribers are notified when underlying per-query results change even if observer identities/count remain the same. Adds tests for early-return notifications and for useQueries with persistence + a memoized combine. 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 cached results
rect rgba(230,245,255,0.6)
note over QueriesObserver: per-index shallow compare
QueriesObserver->>QueriesObserver: shallowCompare(newResult, this.#result)
alt anyIndexOrResultChanged
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
✨ 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
|
View your CI Pipeline Execution ↗ for commit 44bfedb
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9592 +/- ##
===========================================
+ Coverage 45.23% 59.35% +14.12%
===========================================
Files 208 137 -71
Lines 8335 5578 -2757
Branches 1882 1510 -372
===========================================
- Hits 3770 3311 -459
+ Misses 4118 1963 -2155
+ 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 || |
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
There was a problem hiding this comment.
I've updated the test expectations to account for the shallowEqualObjects comparison. The tests now expect combine to re-run when metadata changes occur.
However, I'm not entirely confident whether having combine re-run on metadata changes is the intended behavior from this PR... Perhaps there's a performance consideration I'm missing, or my implementation itself might be incorrect.
The tests are passing with the updated expectations, but I believe a different approach might be more appropriate. I defer to your judgment on whether this is the desired behavior.
There was a problem hiding this comment.
yeah that test seems slightly off, and actually shows the bug: we get new metadata (dataUpdatedAt), which is part of results passed to combine, but we don’t run it :/
I think I was basing this test off of select on useQuery, but the difference is that select only gets data passed while combine gets passed everything.
Should I update the test expectations to reflect this correct behavior?
yes please, and we need to test combine with a stable reference differently, likely by triggering a re-render differently, e.g. just having an increment that triggers an unrelated re-render, because in those cases, a stable combine shouldn’t re-run.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/react-query/src/__tests__/useQueries.test.tsx (3)
1213-1213: Reduce test flakiness by avoiding hard-coded acceptable counts.Accepting [4,5,6] is brittle across timing/impl changes. Prefer explicit bounds.
- expect([4, 5, 6]).toContain(results.length) + expect(results.length).toBeGreaterThanOrEqual(4) + expect(results.length).toBeLessThanOrEqual(6)
1382-1384: Fix misleading inline comment to match the new expectation.Comment says “no increase” but we now expect one extra call.
- // no increase because just a re-render - expect(spy).toHaveBeenCalledTimes(4) + // one extra call due to recomputing the combined result on rerender + expect(spy).toHaveBeenCalledTimes(4)
1394-1396: Adjust comment to reflect why the count increases to 7.Clarify that the refetch sequence drives three combine calls (pending → partial → settled).
- // two value changes = two re-renders + // refetch with new values triggers: both pending -> one pending -> both resolved expect(spy).toHaveBeenCalledTimes(7)packages/query-core/src/queriesObserver.ts (2)
125-132: Micro-optimization: short-circuit on reference equality before shallow compare.Most QueryObserver results are stable by reference when unchanged. Add a ref check to avoid iterating keys.
- ? newResult.some((result, index) => { - const prev = this.#result[index] - return !prev || !shallowEqualObjects(result, prev) - }) + ? newResult.some((result, index) => { + const prev = this.#result[index] + return !prev || (result !== prev && !shallowEqualObjects(result, prev)) + })
141-152: Avoid retaining obsolete observers when there are no listeners.If indices change with no listeners, we return before destroying removed observers. It’s minor, but proactively cleaning them up prevents transient retention during lifecycles where setQueries is called pre-subscription.
- 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) - }) - }) - } + if (hasIndexChange) { + // Always clean up obsolete observers to avoid leaks even without listeners + difference(prevObservers, newObservers).forEach((observer) => { + observer.destroy() + }) + if (this.hasListeners()) { + difference(newObservers, prevObservers).forEach((observer) => { + observer.subscribe((result) => { + this.#onUpdate(observer, result) + }) + }) + } + } + + if (!this.hasListeners()) return
📜 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(2 hunks)packages/react-query/src/__tests__/useQueries.test.tsx(3 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: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/query-core/src/queriesObserver.ts (2)
4-4: Aligns with queryObserver: using shallowEqualObjects for result comparisons.Good call to match core semantics and catch metadata changes (e.g., dataUpdatedAt, fetchStatus).
133-141: Clearer control flow.Early-return + separating index/result changes improves readability and mirrors intended semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/react-query/src/__tests__/useQueries.test.tsx (3)
1213-1213: Relax render-count assertion to reduce flakiness.Render counts can vary slightly across environments/schedulers. Prefer a bounded range over enumerating discrete values.
- expect([4, 5, 6]).toContain(results.length) + expect(results.length).toBeGreaterThanOrEqual(4) + expect(results.length).toBeLessThanOrEqual(6)
1382-1384: Confirm intent: extra combine call on rerender with stable callback.This weakens the “optimize combine for stable reference” story by recomputing on parent rerender even when inputs don’t change. If intentional, consider asserting the last payload to clarify that it’s a recomputation without value change.
// one extra call due to recomputing the combined result on rerender expect(spy).toHaveBeenCalledTimes(4) + expect(spy).toHaveBeenLastCalledWith({ + combined: true, + res: 'first result:0,second result:0', + })
1394-1396: Avoid brittle absolute spy call counts; assert relative +3 after refetch.Make the test resilient to incidental extra emissions while still validating the sequence (pending → partial → resolved).
- // refetch with new values triggers: both pending -> one pending -> both resolved - expect(spy).toHaveBeenCalledTimes(7) + // refetch with new values triggers: both pending -> one pending -> both resolved (+3) + expect(spy).toHaveBeenCalledTimes(baseCalls + 3)Add this just before
client.refetchQueries()to capture the baseline:const baseCalls = spy.mock.calls.length
📜 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/react-query/src/__tests__/useQueries.test.tsx(3 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: Test
- GitHub Check: Preview
There was a problem hiding this comment.
please add another test that verifies that combine isn’t called when an unrelated re-render happens
There was a problem hiding this comment.
please add another test that verifies that
combineisn’t called when an unrelated re-render happens
I have added the test.
Comments have been appropriately added by referring to existing other tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-query/src/__tests__/useQueries.test.tsx (1)
1662-1743: Great addition: guards against unnecessary combine executions on unrelated re-rendersThis directly addresses the earlier reviewer request to add such a test and protects the optimization going forward.
🧹 Nitpick comments (1)
packages/react-query/src/__tests__/useQueries.test.tsx (1)
1213-1213: Prefer explicit range assertions over discrete setUsing a range communicates intent and is less brittle than enumerating acceptable lengths.
- expect([4, 5, 6]).toContain(results.length) + expect(results.length).toBeGreaterThanOrEqual(4) + expect(results.length).toBeLessThanOrEqual(6)
📜 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/react-query/src/__tests__/useQueries.test.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-query/src/__tests__/useQueries.test.tsx (4)
packages/query-core/src/queriesObserver.ts (2)
queries(234-260)result(194-209)packages/react-query/src/useQueries.ts (1)
useQueries(208-332)packages/query-core/src/utils.ts (1)
sleep(362-366)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/react-query/src/__tests__/useQueries.test.tsx (2)
1382-1383: Unrelated re-render should not recompute stable combine — adjust expectationThis contradicts the new test added below (“should not re-run stable combine on unrelated re-render”), which asserts no extra combine call on an unrelated re-render with a stable reference. Align this test to the same behavior.
- // one extra call due to recomputing the combined result on rerender - expect(spy).toHaveBeenCalledTimes(4) + // stable combine should not recompute on unrelated rerender + expect(spy).toHaveBeenCalledTimes(3)
1394-1396: Call count after refetch should reflect only the refetch transitionsIf the rerender did not recompute combine, the subsequent refetch with new values should add three calls (both pending -> one pending -> both resolved), bringing the total from 3 to 6.
- // refetch with new values triggers: both pending -> one pending -> both resolved - expect(spy).toHaveBeenCalledTimes(7) + // refetch with new values triggers: both pending -> one pending -> both resolved + expect(spy).toHaveBeenCalledTimes(6)
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