fix(query-core): resolve suspense when query data is set programmatically#11036
fix(query-core): resolve suspense when query data is set programmatically#11036AmariahAK wants to merge 6 commits into
Conversation
…ally When useSuspenseQuery is used and setQueryData populates the cache while a fetch is in-flight, the suspense boundary stays stuck on the original fetch promise. This prevents streamedQuery and manual cache updates from releasing suspense. Modify fetchOptimistic in queryObserver to use Promise.race between the fetch promise and a cache subscriber that listens for 'updated' events. When data appears in the cache via setQueryData or a streamed chunk, the race resolves immediately without aborting the fetch. This allows the suspense boundary to release and show the available data while the fetch continues in the background. Closes TanStack#10924 Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: atlarix-agent <agent@atlarix.dev>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSuspense Resolution Fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant QueryObserver
participant QueryFn
participant QueryCache
QueryObserver->>QueryFn: query.fetch()
QueryObserver->>QueryCache: subscribe(updated)
alt cache data arrives first
QueryCache-->>QueryObserver: updated event with defined data
QueryObserver->>QueryCache: unsubscribe()
QueryObserver-->>QueryObserver: createResult()
else fetch completes first
QueryFn-->>QueryObserver: fetch resolved
QueryObserver->>QueryCache: unsubscribe()
QueryObserver-->>QueryObserver: createResult()
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f9c9f6e-0fdd-44e8-b976-3179c1f4098f
📒 Files selected for processing (3)
.changeset/resolve-suspense-setquerydata.mdpackages/query-core/src/queryObserver.tspackages/react-query/src/__tests__/useSuspenseQuery.test.tsx
…h rejection unsubscribe() was only called in the .then() success path. If query.fetch() rejected before the cache subscriber resolved the race, the subscription leaked — staying registered on QueryCache indefinitely and checking a stale queryHash on every 'updated' event. Use .finally() to ensure unsubscribe() runs on both success and rejection, preserving the original promise type through the chain. Co-authored-by: atlarix-agent <agent@atlarix.dev>
|
Thanks for this PR — the 1. Floating promise when fetch wins the race When the fetch promise resolves first, A cleaner approach would be to make the subscriber promise self-resolving on cleanup: let resolveEarly: ((result: QueryObserverResult<TData, TError>) => void) | undefined
const cachePromise = new Promise<QueryObserverResult<TData, TError>>((resolve) => {
resolveEarly = resolve
unsubscribe = this.#client.getQueryCache().subscribe((event) => {
if (
event.type === 'updated' &&
event.query.queryHash === query.queryHash &&
query.state.data !== undefined
) {
unsubscribe()
resolve(this.createResult(query, defaultedOptions))
}
})
})
return Promise.race([
query.fetch().then(() => {
return this.createResult(query, defaultedOptions)
}).finally(() => {
unsubscribe()
// Resolve the floating promise to avoid it pending forever
if (resolveEarly) {
// Subscriber never fired — resolve with current result so the promise settles
// This value is ignored by Promise.race since the fetch branch already won
resolveEarly(this.createResult(query, defaultedOptions))
}
}),
cachePromise,
])This ensures both promises always settle, regardless of which wins. 2. No cleanup if the component unmounts mid-fetch If the consuming component unmounts while Consider adding an AbortSignal-aware cleanup, or at minimum documenting that 3. Multiple observers on the same query hash The cache subscriber listens to 4. Test coverage consideration The three tests are solid. One edge case that might be worth adding: what happens when Overall, this is the right approach — much less invasive than #10994 and uses the existing notification system well. The main actionable item is settling the floating promise in point 1. |
…ndefined test - Resolve the subscriber promise when fetch succeeds so both branches of Promise.race always settle, avoiding a permanently pending promise - Move resolution into .then() rather than .finally() so fetch failures still propagate through Promise.race rejection - Add test: setQueryData with undefined must NOT release suspense, documenting the query.state.data !== undefined guard Co-authored-by: atlarix-agent <agent@atlarix.dev>
|
@gonzoblasco hey, apologies for seeing this late |
|
@AmariahAK thanks for the quick turnaround. I took a look at the latest commit. The floating promise fix looks good — the One thing still open: the unmount cleanup concern. If the component unmounts while A simple guard would be to check if the observer is still mounted before resolving: unsubscribe = this.#client.getQueryCache().subscribe((event) => {
if (
event.type === 'updated' &&
event.query.queryHash === query.queryHash &&
query.state.data !== undefined
) {
unsubscribe()
resolve(this.createResult(query, defaultedOptions))
}
})Could add a Re: #10994 — Dominik's approach is more structural but has a larger blast radius. I think both PRs solve the core issue, and whichever the maintainers prefer is fine. Yours is the safer incremental fix. |
|
@gonzoblasco thanks for the re-review. On the unmount cleanup concern — I looked into adding a liveness guard ( I agree with your assessment that this is acceptable for a patch release , the subscriber entry lives only until All other points are resolved — floating promise settled, |
|
@AmariahAK just re-reviewed the latest diff. Everything looks good:
Your reasoning on the This LGTM. Ready for maintainer review. 🚀 |
3 participants
PR Description
@tanstack/query-core:test:lib → 546 passed, 0 failures
@tanstack/react-query:test:lib → 567 passed, 0 failures