fix(query-core): fix hydration bugs for already resolved promises#10444
fix(query-core): fix hydration bugs for already resolved promises#10444Ephem wants to merge 5 commits intoTanStack:mainfrom
Conversation
Implements failing test for incorrect pending status when hydrating already resolved promises and query already exists in the cache.
…n already resolved promises
📝 WalkthroughWalkthroughAdded a changeset documenting a patch release for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 3485052
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/query-core/src/hydration.ts`:
- Around line 239-247: The current hydration always forces status: 'success'
whenever data !== undefined, which wipes error states; restrict that override to
only when the existing query was a resolved pending promise by adding a guard
(e.g., compute an existingQueryIsPending boolean by checking the existing
query's status === 'pending') and apply the status: 'success' branch only when
data !== undefined && existingQueryIsPending (keeping the existing fetchStatus
preservation via existingQueryIsFetching). Locate the block in hydration.ts
where data and existingQueryIsFetching are used and add the pending-status check
before setting status: 'success'.
🪄 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: 816634ee-3ab7-4323-b7ac-8b69ccf55c9d
📒 Files selected for processing (3)
.changeset/shy-wings-buy.mdpackages/query-core/src/__tests__/hydration.test.tsxpackages/query-core/src/hydration.ts
| // if data was resolved synchronously, transition to success | ||
| // (mirrors the new-query branch below), but preserve fetchStatus | ||
| // if the query is already actively fetching | ||
| ...(data !== undefined && { | ||
| status: 'success' as const, | ||
| ...(!existingQueryIsFetching && { | ||
| fetchStatus: 'idle' as const, | ||
| }), | ||
| }), |
There was a problem hiding this comment.
Scope the success override to resolved pending queries only.
This now rewrites any hydrated existing query with data !== undefined to status: 'success'. Queries can legitimately be status: 'error' while still holding prior data after a background refetch failure, and callers can opt into dehydrating those. In that case hydration will silently clear the error state. Limit the override to the pending-promise case that this PR is fixing.
Suggested fix
- ...(data !== undefined && {
+ ...(state.status === 'pending' && data !== undefined && {
status: 'success' as const,
...(!existingQueryIsFetching && {
fetchStatus: 'idle' as const,
}),
}),📝 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.
| // if data was resolved synchronously, transition to success | |
| // (mirrors the new-query branch below), but preserve fetchStatus | |
| // if the query is already actively fetching | |
| ...(data !== undefined && { | |
| status: 'success' as const, | |
| ...(!existingQueryIsFetching && { | |
| fetchStatus: 'idle' as const, | |
| }), | |
| }), | |
| // if data was resolved synchronously, transition to success | |
| // (mirrors the new-query branch below), but preserve fetchStatus | |
| // if the query is already actively fetching | |
| ...(state.status === 'pending' && data !== undefined && { | |
| status: 'success' as const, | |
| ...(!existingQueryIsFetching && { | |
| fetchStatus: 'idle' as const, | |
| }), | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/query-core/src/hydration.ts` around lines 239 - 247, The current
hydration always forces status: 'success' whenever data !== undefined, which
wipes error states; restrict that override to only when the existing query was a
resolved pending promise by adding a guard (e.g., compute an
existingQueryIsPending boolean by checking the existing query's status ===
'pending') and apply the status: 'success' branch only when data !== undefined
&& existingQueryIsPending (keeping the existing fetchStatus preservation via
existingQueryIsFetching). Locate the block in hydration.ts where data and
existingQueryIsFetching are used and add the pending-status check before setting
status: 'success'.
🎯 Changes
Fixes #9642
Warning
While I think this is complete, I still need some time to test it properly before feeling confident to merge it.
This PR fixes and adds tests for two different bugs that would both cause queries to incorrectly jump into the fetching/pending state on hydration.
This would happen for queries that were prefetched without awaiting, like so:
void queryClient.prefetchQuery(...)and then dehydrated, but where the promise would resolve before the hydration would happen. The query state in the hydration would bestatus: 'pending', since that's the status on the server at the moment of hydration, but when hydrating we should put them into the'successful'state since we already have data.First bug is that we did this for new queries, but not when queries already existed in the cache.
The second bug is that for both cases, we also still called
query.fetchwith aninitialPromise, which would briefly emit a fetching state. We did this to set up a retryer and I even wrote a comment in the code that this was necessary even for queries that already had data, but I've since reconsidered. The retryer is optional, so if the query is already in a successful state, I don't think we need to do that for either new or existing queries and removing it shows no failing tests. This is a controversial part of the PR though and I'm happy for pushback.If we need to keep the data in sync with the promise in the retryer, we already have a problem with that when people call
query.setData, since we don't do anything with the retryer in those situations either.Note that the promise we return from
useQueryetc is different, that comes from the observer, not directly from the query.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit