fix(query-core): fix combine function cache invalidation with stable reference #9618
Conversation
WalkthroughAdds input-length tracking to QueriesObserver.#combineResult so recomputation occurs when the input array size changes (e.g., optimistic updates). Stores the last input and updates it on recompute. Adds tests exercising useQueries combine behavior across stable/inline combines and dynamic query arrays. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component
participant RQ as useQueries Hook
participant QO as QueriesObserver
participant QC as QueryCache
C->>RQ: render({ queries[], combine })
RQ->>QC: read query results
QC-->>RQ: QueryObserverResults[] (input)
RQ->>QO: #combineResult(input)
rect rgba(200,230,255,0.25)
note right of QO: New: track lastInput length
QO->>QO: if !#lastCombine or !#lastResult
QO->>QO: or input.length != #lastInput?.length
QO->>QO: or input !== #lastResult
alt recompute
QO->>QO: #combinedResult = combine(input)
QO->>QO: #lastInput = input
QO->>QO: update #lastCombine / #lastResult
else
QO->>QO: return cached #combinedResult
end
end
QO-->>RQ: combined result
RQ-->>C: hook result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 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 02ddb7e
☁️ Nx Cloud last updated this comment at |
![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/queriesObserver.ts (1)
46-47: Add a brief doc comment for#lastInputA one-liner clarifying that
#lastInputtracks the last optimistic input used for combine to detect shape (length) changes would aid future maintainers.Apply near-field doc:
- #lastInput?: Array<QueryObserverResult> + // Last optimistic input passed to #combineResult; used to detect shape (length) changes before #result updates. + #lastInput?: Array<QueryObserverResult>packages/react-query/src/__tests__/useQueries-combine.test.tsx (2)
231-272: Same-length key changes assert eventual data, not immediate combine resultThese assertions wait for data and don’t validate whether the combined result updates in the same render when keys change but length stays constant. If the intended behavior allows one interim render with the previous combined cache for same-length changes, consider adding a short inline comment to the test noting this expectation to prevent future “tightening” attempts.
1-22: Reduce duplication: factor a test helper to create client/wrapperEach test repeats
new QueryClientandwrapper. Consider a small factory to DRY this up and ensure consistent defaults (and optional teardown).Example helper:
+function createTestWrapper() { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + const wrapper = ({ children }: { children: React.ReactNode }) => ( + <QueryClientProvider client={queryClient}>{children}</QueryClientProvider> + ) + return { queryClient, wrapper } +}Then in tests:
-const queryClient = new QueryClient({ ... }) -const wrapper = ({ children }: { children: React.ReactNode }) => ( ... ) +const { queryClient, wrapper } = createTestWrapper()Optionally call
queryClient.clear()at test end to avoid lingering GC timers.Also applies to: 54-65, 96-107, 151-153, 197-199, 241-242, 284-285
📜 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-combine.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-query/src/__tests__/useQueries-combine.test.tsx (2)
packages/query-core/src/queriesObserver.ts (2)
queries(238-264)result(195-210)packages/react-query/src/__tests__/useQueries.test.tsx (1)
key1(1091-1190)
packages/query-core/src/queriesObserver.ts (1)
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 (7)
packages/query-core/src/queriesObserver.ts (3)
46-46: Track last input to break stale combine cache — good additionAdding
#lastInputenables detecting optimistic input shape changes independent of#resultupdates. This unblocks the stale-one-render bug for length changes.
220-227: Invalidate combine cache on input length change — fix is correct and minimalThe extra
input.length !== this.#lastInput?.lengthguard precisely targets the stale case introduced by optimistic inputs while preserving perf. Updating#lastInputonly on recompute keeps cache hits stable.
220-221: Confirm intended behavior for same-length input changesThis fix handles length deltas immediately. For same-length but reordered/replaced queries, we still rely on
setQueriesdriving#result !== #lastResultto trigger recompute, which may allow one interim render using the prior combined cache. Please confirm this is acceptable per #9207’s trade-off, or call out the behavior in docs.packages/react-query/src/__tests__/useQueries-combine.test.tsx (4)
10-52: Stable combine: immediate length updates — solid regression testCovers the exact bug surface (0→1→2) and asserts sync length changes without waiting for data. Nice.
94-140: Dynamic add/remove sequences — good breadthValidates increases and decreases; keeps assertions focused on length, which is what the fix targets.
142-186: Transforming combine results — verifies non-array shapesEnsures memoization works when
combinereturns objects with derived fields. Looks good.
188-229: Switching stable/inline combine — interoperability check passesCovers toggling function identity without regressions. Good safety net.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9618 +/- ##
===========================================
+ Coverage 45.50% 59.90% +14.40%
===========================================
Files 209 138 -71
Lines 8377 5617 -2760
Branches 1897 1512 -385
===========================================
- Hits 3812 3365 -447
+ Misses 4118 1949 -2169
+ Partials 447 303 -144 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-core/src/__tests__/queriesObserver.test.tsx (2)
419-432: Strengthen assertions to prove we combined the new, longer inputRight now we only assert that the call count increased. Also assert that at least one combine call received an input of length 2, and that we observed a new emission post-update.
Apply:
@@ - const initialCallCount = combine.mock.calls.length + const initialCallCount = combine.mock.calls.length + const baselineResults = results.length @@ - expect(combine.mock.calls.length).toBeGreaterThan(initialCallCount) - expect(results[results.length - 1]).toHaveLength(2) + expect(combine.mock.calls.length).toBeGreaterThan(initialCallCount) + expect( + combine.mock.calls.some((call) => Array.isArray(call[0]) && call[0].length === 2), + ).toBe(true) + expect(results.length).toBeGreaterThan(baselineResults) + expect(results[results.length - 1]).toHaveLength(2)
412-416: Ensure unsubscribe always runsWrap with try/finally so a failed assertion doesn’t leak the subscription in CI.
Apply:
- const unsubscribe = observer.subscribe((result) => { - results.push(result) - }) + const unsubscribe = observer.subscribe((result) => { + results.push(result) + }) + try { @@ - expect(results[results.length - 1]).toHaveLength(2) - - unsubscribe() + expect(results[results.length - 1]).toHaveLength(2) + } finally { + unsubscribe() + }Also applies to: 434-435
📜 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/__tests__/queriesObserver.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
packages/query-core/src/queriesObserver.ts (2)
observer(267-273)result(196-211)
⏰ 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 (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
398-411: Good targeted regression test for stable combine invalidationCovers the 1→2 query-count change with a stable combine reference and ensures cache invalidation kicks in. Nice alignment with the bug’s root cause.
| // Compare input.length to handle optimistic updates where input differs from this.#result | ||
| input.length !== this.#lastInput?.length || |
There was a problem hiding this comment.
I don’t think a length check is what we want here. What about situations where the length is the same, but data inside it changed?
I’m honestly not sure why input would change, but the check for this.#result !== this.#lastResult would still see things as being equal. That’s what we need to find out imo
There was a problem hiding this comment.
I’m honestly not sure why
inputwould change, but the check forthis.#result !== this.#lastResultwould still see things as being equal. That’s what we need to find out imo
When getOptimisticResult is called, a new input is created—but this happens at render time.
query/packages/react-query/src/useQueries.ts
Lines 261 to 266 in 92d6b7b
On the other hand, observer.setQueries is executed inside useEffect, which means it runs at commit time.
query/packages/react-query/src/useQueries.ts
Lines 281 to 286 in 92d6b7b
In other words, at the moment getOptimisticResult is called, the input already reflects the new value, but this.#result still holds the previous value.
Because of this timing difference, the following sequence occurs
First render
this.#result = [],this.#lastResult = undefined→getOptimisticResultis called →input = []is created#combineResultis called → sincethis.#lastResult = this.#result, the result becomesthis.#result = [],this.#lastResult = []useEffect() { setQueries ... }runs
Second render
4. getOptimisticResult is called → input = [query1], while this.#result = [], this.#lastResult = []
5. #combineResult is called → still this.#result === this.#lastResult ([] === [])
6. useEffect() { setQueries ... } runs → this.#result = [query1]
For this reason, a discrepancy arises between the result and the input
From a semantic perspective, it would indeed be more accurate to add a comparison such as,
input !== this.#result && input !== this.#lastInput
However, while this approach handles optimistic updates well, I believe it could also cause excessive cache misses in regular updates. That is why I chose the alternative solution of comparing lengths instead
Regarding your concern about edge cases—such as when the arrays have the same length but different order—I confirmed through the additional tests I wrote ("should handle same length but different queries" and "should handle query order changes with same length") that no issues arise there. Based on that, I decided to adopt this solution as the final choice.
That said, as you pointed out, this method is ultimately more of a stopgap—it works well for optimistic updates while avoiding excessive cache misses in regular updates, but it remains a temporary measure.
Aside from this temporary workaround, I think a better solution would be to explicitly distinguish the case using a flag.
For example, something like this
#combineResult(
input: Array<QueryObserverResult>,
combine: CombineFn<TCombinedResult> | undefined,
isOptimisticUpdate = false
): TCombinedResult {
if (combine) {
if (
!this.#combinedResult ||
this.#result !== this.#lastResult ||
(isOptimisticUpdate && input !== this.#lastInput) ||
combine !== this.#lastCombine
) {
// ...
}
}
}Perhaps an approach along these lines?
But come to think of it, this method would still cause additional cache misses,,
For example, the expected values for the three tests below seem likely to increase from 3 to 5.
useQueries > should optimize combine if it is a stable reference
useQueries > should re-run combine if the functional reference changes
useQueries > should not re-run stable combine on unrelated re-render
So my final thought is that comparing lengths seems like the best choice when not considering overall mechanism modifications. What do you think?
Fixes #8781
Closes #9207 : He had taken ownership of the fix, and his proposed direction was similar to mine. However, since there has been no response for several months and no tests seem to be included, I am opening an additional PR to help resolve the issue. If there is a rule that prevents someone else from submitting a PR once an issue has already been assigned, I will be happy to close this one.
Description
combinefunction with stable reference not updating correctly when queries changeOn the other hand,
combineimplemented as an inline function works correctly because the reference changes each time.Problem
When using
useQuerieswith a stable reference for thecombinefunction, the combined result was not updated immediately when the queries array changed. This resulted in stale data being returned for one render cycle.Root Cause
The caching logic in
#combineResultwas comparingthis.#resultto determine cache invalidation, but the actual data being processed was theinputparameter. WhengetOptimisticResultgenerated a new result array,this.#resulthadn't been updated yet (happens later inuseEffect), causing incorrect cache hits.Solution
Added
input.length !== this.#lastInput?.lengthcheck to the cache invalidation logic. This ensures the cache is properly invalidated when the number of queries changes, which is the most common scenario.Why length comparison is sufficient
While comparing only the length might seem incomplete, it works because,
setQueriesis called viauseEffectwhich updatesthis.#resultfor the next renderTesting
All existing tests continue to pass, ensuring backward compatibility.
No breaking changes – this is a targeted optimization
Alternative approaches considered
input.length !== this.#lastInput: This has been confirmed to significantly impact existing caching to the point of breaking existing caching tests.#combineResultand redesigning it, we determined that the overall impact would be too significant.#combineCache = new WeakMap<Array<QueryObserverResult>, TCombinedResult>()It might be possible, but I judged it to be overhead.
Summary by CodeRabbit
Bug Fixes
Tests