Uh oh!
There was an error while loading. Please reload this page.
fix(query-core): resolve suspense when query data is set programmatically - #11036
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. ClosesTanStack#10924 Co-authored-by: atlarix-agent <agent@atlarix.dev>
Co-authored-by: atlarix-agent <agent@atlarix.dev>
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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
Uh oh!
There was an error while loading. Please reload this page.
…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>
gonzoblasco
commented
Jul 15, 2026
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: letresolveEarly: ((result: QueryObserverResult<TData,TError>)=>void)|undefinedconstcachePromise=newPromise<QueryObserverResult<TData,TError>>((resolve)=>{resolveEarly=resolveunsubscribe=this.#client.getQueryCache().subscribe((event)=>{if(event.type==='updated'&&event.query.queryHash===query.queryHash&&query.state.data!==undefined){unsubscribe()resolve(this.createResult(query,defaultedOptions))}})})returnPromise.race([query.fetch().then(()=>{returnthis.createResult(query,defaultedOptions)}).finally(()=>{unsubscribe()// Resolve the floating promise to avoid it pending foreverif(resolveEarly){// Subscriber never fired — resolve with current result so the promise settles// This value is ignored by Promise.race since the fetch branch already wonresolveEarly(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>
AmariahAK
commented
Jul 16, 2026
@gonzoblasco hey, apologies for seeing this late |
gonzoblasco
commented
Jul 16, 2026
@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. |
AmariahAK
commented
Jul 16, 2026
@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, |
gonzoblasco
commented
Jul 16, 2026
@AmariahAK just re-reviewed the latest diff. Everything looks good:
Your reasoning on the This LGTM. Ready for maintainer review. 🚀 |
Uh oh!
There was an error while loading. Please reload this page.
View your CI Pipeline Execution ↗ for commit b9bcc31
☁️ Nx Cloud last updated this comment at |
PR Description
@tanstack/query-core:test:lib → 546 passed, 0 failures
@tanstack/react-query:test:lib → 567 passed, 0 failures