fix(query-core): fix combine function cache invalidation with stable reference #9635
Conversation
WalkthroughAdds query-core logic to recompute combined results when the number of observed queries changes, and introduces tests in query-core and react-query to validate correct behavior for stable and inline combine functions as query arrays grow, shrink, reorder, or switch combine references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Component/useQueries
participant O as QueriesObserver
participant Q as QueryCache
C->>O: observe(queries, { combine })
O->>Q: subscribe to query results
Q-->>O: results update (array)
rect rgba(230,245,255,0.6)
note over O: Combine memoization
O->>O: #combineResult(input)
alt New combine fn OR results changed OR input.length changed
O->>O: Recompute combined result<br/>Update #lastInput, #lastCombine, #lastResult
else No relevant change
O->>O: Return cached #lastResult
end
end
O-->>C: emit combined result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
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 (5)
packages/query-core/src/queriesObserver.ts (2)
46-47: Store only the input length (or compare to current result length) to reduce state and couplingYou only need the length. Two lightweight options:
- Option A (keep a field): store a number
- Option B (no new field): compare against
this.#result.lengthApply Option A:
- // Tracks the last input passed to #combineResult to detect query count changes in optimistic updates - #lastInput?: Array<QueryObserverResult> + // Tracks last input length to detect query-count changes during optimistic updates + #lastInputLen?: numberAnd below:
- input.length !== this.#lastInput?.length || + input.length !== this.#lastInputLen ||- this.#lastInput = input + this.#lastInputLen = input.lengthOr Option B (remove field entirely):
- input.length !== this.#lastInput?.length || + input.length !== this.#result.length ||…and drop the
#lastInputfield and its assignment.
172-194: Add shrink-path coverage at core levelWe have a growth test for stable combine; add a matching shrink test (length 2 → 1) exercising the same optimistic path to guard against regressions.
I can draft the core test mirroring the growth case if you want it included in this PR.
Also applies to: 279-287
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
398-446: Consider adding the symmetric shrink caseAdd a second test that starts with 2 queries and then removes one with the same stable
combine, asserting a call wherecall[0].length === 1and final result length is 1. This closes the gap for length decreases.Example to append at the end of this file:
test('combine with stable reference should invalidate cache when query count shrinks', async () => { const key1 = queryKey() const key2 = queryKey() const queryFn1 = vi.fn().mockReturnValue(1) const queryFn2 = vi.fn().mockReturnValue(2) const combine = vi.fn((results) => results) const observer = new QueriesObserver( queryClient, [ { queryKey: key1, queryFn: queryFn1 }, { queryKey: key2, queryFn: queryFn2 }, ], { combine }, ) const results: Array<Array<QueryObserverResult>> = [] const unsubscribe = observer.subscribe((r) => results.push(r)) try { await vi.advanceTimersByTimeAsync(0) const initialCalls = combine.mock.calls.length const baseline = results.length observer.setQueries([{ queryKey: key1, queryFn: queryFn1 }], { combine }) await vi.advanceTimersByTimeAsync(0) expect(combine.mock.calls.length).toBeGreaterThan(initialCalls) expect( combine.mock.calls.some((c) => Array.isArray(c[0]) && c[0].length === 1), ).toBe(true) expect(results.length).toBeGreaterThan(baseline) expect(results[results.length - 1]).toHaveLength(1) } finally { unsubscribe() } })packages/react-query/src/__tests__/useQueries-combine.test.tsx (2)
231-274: Optional: assert combine invocation to guard perfWrap
stableCombinein a spy and assert minimal recomputes across same-length changes to catch accidental cache busts.Example:
- const stableCombine = (results: any) => results + const stableCombine = vi.fn((results: any) => results) ... - rerender({ keys: ['c', 'd'] }) + const callsBefore = stableCombine.mock.calls.length + rerender({ keys: ['c', 'd'] }) + expect(stableCombine.mock.calls.length).toBeGreaterThanOrEqual(callsBefore)
1-92: Nit: avoidanyin tests where simple generics sufficeTighten types for
resultsto improve editor help without noise.For example:
- const stableCombine = (results: any) => results + const stableCombine = <T extends unknown[]>(results: T) => results
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/query-core/src/__tests__/queriesObserver.test.tsx(1 hunks)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 (3)
packages/react-query/src/__tests__/useQueries-combine.test.tsx (2)
packages/query-core/src/queriesObserver.ts (2)
queries(239-265)result(196-211)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)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
packages/query-core/src/queriesObserver.ts (3)
observer(267-273)QueriesObserver(35-290)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 (6)
packages/query-core/src/queriesObserver.ts (1)
221-223: Targeted cache-bust on input length change looks correctThis condition plugs the optimistic-result gap for stable combine refs without regressing existing memoization. Updating
#lastInputinside the recompute branch is also correct.Also applies to: 227-232
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
398-446: Good regression test for stable-combine growthThe assertions prove the cache is invalidated when the query count increases and that combine receives a 2-length array.
packages/react-query/src/__tests__/useQueries-combine.test.tsx (4)
10-51: Stable combine immediate updates: nice, focused signal checkSynchronous length checks on rerender validate the intended behavior for stable refs.
94-140: Dynamic add/remove coverage is solidCovers grow/shrink without relying on data resolution. Good.
142-186: Transforming combine path coveredVerifies memoization still works when combine returns a different shape.
188-229: Switching stable ↔ inline combineEnsures no regressions when toggling references. All good.
|
Length comparison is not a fundamental solution |
2 participants
Fixes #8781
Related PR: #9618
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.
Notes
This is regarding the feedback from the previous PR.
#9618 (comment)
inputaren't detected inthis.#result !== this.#lastResult.My response to this
#9618 (comment)
So my conclusion was that the length comparison is a reasonable compromise—it resolves the issue and detects input changes while minimizing impact on existing functionality (the cache). I ended the discussion by asking if my reasoning was correct.
Summary by CodeRabbit
Bug Fixes
Tests