fix: resolve suspense queries when data gets into the cache by other means#10994
fix: resolve suspense queries when data gets into the cache by other means#10994TkDodo wants to merge 7 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughQuery promise tracking now stays synchronized with query state, and observers/hooks read that shared promise for optimistic and suspense flows. Preact error-boundary handling now uses the current query state, and new tests cover cached data arriving during pending fetches. ChangesStateful Query Promise
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 46bed97
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/query-core/src/thenable.ts`:
- Around line 108-113: The pending branch in thenable handling is reusing an old
promise across query-hash changes, so update the logic in thenable.ts and the
caller in queryObserver.ts to recreate or refresh the thenable whenever the
query identity changes instead of returning the previous pending instance. Use
the query-hash comparison in QueryObserver and the finalizePending path in
finalizeThenableIfPossible/thenable.status handling to ensure a new settled
query does not inherit a stale pending promise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d9cd8e2-21ff-462c-b1e7-4a16aa5cf26d
📒 Files selected for processing (5)
packages/query-core/src/__tests__/query.test.tsxpackages/query-core/src/query.tspackages/query-core/src/queryObserver.tspackages/query-core/src/thenable.tspackages/react-query/src/__tests__/useSuspenseQuery.test.tsx
| switch (thenable.status) { | ||
| case 'pending': | ||
| if (finalizePending) { | ||
| finalizeThenableIfPossible(thenable) | ||
| } | ||
| return thenable |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Recreate the thenable on query-hash switches.
When packages/query-core/src/queryObserver.ts passes false here for a query change, the 'pending' branch reuses the previous query's pending thenable. If the new query is already settled from cache, nextResult.promise stays pending and Suspense can hang indefinitely.
Suggested fix
switch (thenable.status) {
case 'pending':
- if (finalizePending) {
- finalizeThenableIfPossible(thenable)
- }
- return thenable
+ if (!finalizePending) {
+ return recreateThenable()
+ }
+ finalizeThenableIfPossible(thenable)
+ return thenable
case 'fulfilled':
if (isErrorWithoutData || result.data !== thenable.value) {
return recreateThenable()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (thenable.status) { | |
| case 'pending': | |
| if (finalizePending) { | |
| finalizeThenableIfPossible(thenable) | |
| } | |
| return thenable | |
| switch (thenable.status) { | |
| case 'pending': | |
| if (!finalizePending) { | |
| return recreateThenable() | |
| } | |
| finalizeThenableIfPossible(thenable) | |
| return thenable |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/query-core/src/thenable.ts` around lines 108 - 113, The pending
branch in thenable handling is reusing an old promise across query-hash changes,
so update the logic in thenable.ts and the caller in queryObserver.ts to
recreate or refresh the thenable whenever the query identity changes instead of
returning the previous pending instance. Use the query-hash comparison in
QueryObserver and the finalizePending path in
finalizeThenableIfPossible/thenable.status handling to ensure a new settled
query does not inherit a stale pending promise.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/query-core/src/query.ts`:
- Around line 202-203: `Query.promise` can still be undefined, but
`QueryObserver.fetchOptimistic()` assumes it is always a Promise and calls
`.then()` directly. Update `Query.promise` in `Query` to initialize and cache
the promise on first access (or otherwise guarantee a Promise return), and
adjust the getter signature so it no longer exposes `undefined`; verify the flow
where `QueryObserver.fetchOptimistic()` reads `query.promise` remains safe
without extra guards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d646e7ae-bb64-40a4-8fd8-4fd3b3682525
📒 Files selected for processing (1)
packages/query-core/src/query.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/silly-tires-brake.md:
- Line 3: The changeset entry uses the wrong package name, so update the package
identifier in the changeset metadata from the misspelled `@tanstack/peact-query`
to the correct `@tanstack/preact-query`. Make this fix in the changeset file so
the package is properly included in the version bump and release notes
generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d41ac60-a1bc-4992-be31-7239db25860f
📒 Files selected for processing (7)
.changeset/silly-tires-brake.mdpackages/preact-query/src/__tests__/useSuspenseQuery.test.tsxpackages/preact-query/src/errorBoundaryUtils.tspackages/preact-query/src/useBaseQuery.tspackages/preact-query/src/useQueries.tspackages/query-core/src/hydration.tspackages/query-core/src/query.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/query-core/src/query.ts
fixes
Summary by CodeRabbit
setQueryDataoccurs during an in-flight fetch.