fix(query-core): fix hydration bugs for already resolved promises#10444
fix(query-core): fix hydration bugs for already resolved promises#10444Ephem merged 9 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
|
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:
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 (1)
📝 WalkthroughWalkthroughHydration behavior was changed so dehydrated queries whose promises already resolved are treated as successful; retryer creation and query.fetch calls are skipped for synchronously-resolved thenables. Tests and a changeset were added to cover streaming/hydration synchronous-thenable scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant DehydratedState
participant Client
participant QueryCache
participant Retryer
Server->>DehydratedState: prefetch/create query (status: pending, promise)
DehydratedState-->>Client: send dehydrated payload
Client->>QueryCache: hydrate(payload)
alt promise resolved synchronously
QueryCache->>QueryCache: set data, set status -> success
Note right of QueryCache: do NOT create Retryer
else promise still pending/asynchronous
QueryCache->>Retryer: create Retryer to manage fetch
Retryer->>QueryCache: update fetchStatus -> fetching
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 652375e
☁️ 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: 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
|
Good regression tests for this hydration edge case. A few observations: On the On preserving On the test setup: The pattern to override |
Yes. The logic whether to revalidate lies elsewhere, if that elsewhere triggers a fetch, that will set up the retryer.
If a fetch is ongoing on the client, it's very likely to fetch fresher data than what was in a dehydrated query, so this is a tradeoff I'm willing to live with. 😄
The whole point is that it needs to be fully synchronous, so no. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-core/src/hydration.ts (1)
213-215:⚠️ Potential issue | 🟠 MajorPreserve freshness when promoting synchronously resolved pending queries.
The sync-data path now skips
query.fetch, so the success dispatch that would normally updatedataUpdatedAtnever runs. Whenstate.status === 'pending', the serialized state can still carrydataUpdatedAt: 0; promoting it tosuccesswith that timestamp makes the hydrated data stale immediately and can regress an existing query even thoughhasNewerSyncDataselected it as newer.🐛 Proposed fix
const syncData = promise ? tryResolveSync(promise) : undefined const rawData = state.data === undefined ? syncData?.data : state.data const data = rawData === undefined ? rawData : deserializeData(rawData) + const shouldPromoteResolvedPendingQuery = + state.status === 'pending' && data !== undefined + const dataUpdatedAt = shouldPromoteResolvedPendingQuery + ? (state.dataUpdatedAt || dehydratedAt || Date.now()) + : state.dataUpdatedAt let query = queryCache.get(queryHash) const existingQueryIsPending = query?.state.status === 'pending' const existingQueryIsFetching = query?.state.fetchStatus === 'fetching' @@ query.setState({ ...serializedState, data, + dataUpdatedAt, // If the query was pending at the moment of dehydration, but resolved to have data // before hydration, we can assume the query should be hydrated as successful. @@ ...state, data, + dataUpdatedAt, fetchStatus: 'idle', // Like above, if the query was pending at the moment of dehydration but has data,Also applies to: 235-250, 266-275, 280-289
🤖 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 213 - 215, When promoting synchronously-resolved pending queries (the code path that uses syncData from tryResolveSync(promise) and assigns rawData/data via deserializeData), preserve freshness by ensuring dataUpdatedAt is set to a current timestamp instead of carrying through a stale serialized value (e.g., 0). Specifically, when state.status === 'pending' and syncData exists (or rawData came from syncData), set dataUpdatedAt = syncData.dataUpdatedAt ?? Date.now() (or at minimum Date.now()) before returning/using the promoted success state; apply the same change to the other sync-resolution blocks referenced (around the other ranges) so promotion never downgrades freshness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/query-core/src/hydration.ts`:
- Around line 213-215: When promoting synchronously-resolved pending queries
(the code path that uses syncData from tryResolveSync(promise) and assigns
rawData/data via deserializeData), preserve freshness by ensuring dataUpdatedAt
is set to a current timestamp instead of carrying through a stale serialized
value (e.g., 0). Specifically, when state.status === 'pending' and syncData
exists (or rawData came from syncData), set dataUpdatedAt =
syncData.dataUpdatedAt ?? Date.now() (or at minimum Date.now()) before
returning/using the promoted success state; apply the same change to the other
sync-resolution blocks referenced (around the other ranges) so promotion never
downgrades freshness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fb7dd8e-b9fd-4a0f-aeac-b4634746b80e
📒 Files selected for processing (1)
packages/query-core/src/hydration.ts
github-actions
Bot
added
documentation
Ephem
removed
documentation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/query-core/src/hydration.ts (2)
213-215:⚠️ Potential issue | 🟠 MajorSet
dataUpdatedAtwhen promoting resolved pending data to success.Line 243 and Line 273 can turn a pending dehydrated query into
success, but both branches keep the pending state’sdataUpdatedAt—often0. In the existing-query path, this can even move the timestamp backwards afterhasNewerSyncDatadecided the payload is newer, causing fresh hydrated data to look stale/refetchable.🐛 Proposed fix
const syncData = promise ? tryResolveSync(promise) : undefined const rawData = state.data === undefined ? syncData?.data : state.data const data = rawData === undefined ? rawData : deserializeData(rawData) + const resolvedPendingQuery = + state.status === 'pending' && data !== undefined + const resolvedPendingDataUpdatedAt = resolvedPendingQuery + ? (dehydratedAt ?? Date.now()) + : state.dataUpdatedAt let query = queryCache.get(queryHash) @@ - ...(state.status === 'pending' && - data !== undefined && { + ...(resolvedPendingQuery && { + dataUpdatedAt: resolvedPendingDataUpdatedAt, status: 'success' as const, // Preserve existing fetchStatus if the existing query is actively fetching. ...(!existingQueryIsFetching && { fetchStatus: 'idle' as const, }), }), @@ { ...state, data, fetchStatus: 'idle', // Like above, if the query was pending at the moment of dehydration but has data, // we can assume it should be hydrated as successful. - status: - state.status === 'pending' && data !== undefined - ? 'success' - : state.status, + dataUpdatedAt: resolvedPendingDataUpdatedAt, + status: resolvedPendingQuery ? 'success' : state.status, },Also applies to: 235-250, 267-275
🤖 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 213 - 215, When promoting a pending dehydrated query to success using syncData from tryResolveSync, ensure you update the state's dataUpdatedAt instead of preserving the old pending timestamp; modify the branches that compute data (where syncData, rawData, data are used and where hasNewerSyncData is consulted) to set dataUpdatedAt = syncData?.updatedAt ?? Date.now() when syncData is applied so hydrated data gets a current timestamp and doesn't appear stale.
217-250:⚠️ Potential issue | 🟠 MajorTreat paused queries as active fetches.
Line 219 only preserves
fetching, butpausedis also a non-idle active fetch state inQuery.fetch(). A paused existing query can be reset toidleat Line 248 and pass the fetch guard at Line 286, losing paused state or creating retryer inconsistencies.🐛 Proposed fix
let query = queryCache.get(queryHash) const existingQueryIsPending = query?.state.status === 'pending' - const existingQueryIsFetching = query?.state.fetchStatus === 'fetching' + const existingQueryHasActiveFetch = + query !== undefined && query.state.fetchStatus !== 'idle' @@ - // Preserve existing fetchStatus if the existing query is actively fetching. - ...(!existingQueryIsFetching && { + // Preserve existing fetchStatus if the existing query has an active fetch. + ...(!existingQueryHasActiveFetch && { fetchStatus: 'idle' as const, }), @@ !syncData && !existingQueryIsPending && - !existingQueryIsFetching && + !existingQueryHasActiveFetch && // Only hydrate if dehydration is newer than any existing data,Also applies to: 280-289
🤖 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 217 - 250, The code only treats fetchStatus === 'fetching' as an active fetch when computing existingQueryIsFetching and later preserving fetchStatus in query.setState; change those checks to treat 'paused' the same as 'fetching' (e.g., existingQueryIsFetching -> existingQueryIsActiveFetch = query?.state.fetchStatus === 'fetching' || query?.state.fetchStatus === 'paused') and use that new boolean wherever the code currently gates preserving fetchStatus (including the similar block later around the other hydration branch), so paused queries are preserved and not reset to 'idle'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/query-core/src/hydration.ts`:
- Around line 213-215: When promoting a pending dehydrated query to success
using syncData from tryResolveSync, ensure you update the state's dataUpdatedAt
instead of preserving the old pending timestamp; modify the branches that
compute data (where syncData, rawData, data are used and where hasNewerSyncData
is consulted) to set dataUpdatedAt = syncData?.updatedAt ?? Date.now() when
syncData is applied so hydrated data gets a current timestamp and doesn't appear
stale.
- Around line 217-250: The code only treats fetchStatus === 'fetching' as an
active fetch when computing existingQueryIsFetching and later preserving
fetchStatus in query.setState; change those checks to treat 'paused' the same as
'fetching' (e.g., existingQueryIsFetching -> existingQueryIsActiveFetch =
query?.state.fetchStatus === 'fetching' || query?.state.fetchStatus ===
'paused') and use that new boolean wherever the code currently gates preserving
fetchStatus (including the similar block later around the other hydration
branch), so paused queries are preserved and not reset to 'idle'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e452fb36-302a-4618-91ba-a816173070fb
📒 Files selected for processing (1)
packages/query-core/src/hydration.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/hydration.test.tsx (1)
1394-1448: Nit:asyncwithoutawait.This test is declared
asyncbut never awaits anything — everything it asserts happens synchronously duringhydrate(...). The function can simply be non-async (and the trailingresolvePrefetch?.('server data')at line 1419 is also effectively a no-op since no consumer ofprefetchPromiseexists before the dehydrate call and the overriddenthenat 1421 short-circuits any await on the dehydrated promise). Keeping it truly sync makes the "must hydrate synchronously" intent clearer, matching the companion test at 1350.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/hydration.test.tsx` around lines 1394 - 1448, Remove the unnecessary async marker from the test "should set status to success when rehydrating an existing pending query with a synchronously resolved promise" and any unused async-related artifacts: make the test function non-async, remove/comment out any unused await usage (there are none), and keep the synchronous resolution of prefetchPromise via resolvePrefetch and the overridden dehydrated.queries[0].promise.then behavior as-is; ensure the test still calls serverQueryClient.prefetchQuery, dehydrate(...), hydrate(clientQueryClient, dehydrated), and asserts clientQueryClient.getQueryData(key) === 'server data' and query.state.status === 'success'.
🤖 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/__tests__/hydration.test.tsx`:
- Around line 1394-1505: Three new tests use test(...) instead of the project's
preferred it(...), causing ESLint vitest/consistent-test-it failures; rename the
three occurrences of test('should set status to success when rehydrating an
existing pending query with a synchronously resolved promise', ...),
test('should not transition to a fetching/pending state when hydrating an
already resolved promise into a new query', ...), and test('should not
transition to a fetching/pending state when hydrating an already resolved
promise into an existing query', ...) to use it(...) with the exact same
descriptions and bodies so the tests remain identical but conform to the
consistent-it rule.
---
Nitpick comments:
In `@packages/query-core/src/__tests__/hydration.test.tsx`:
- Around line 1394-1448: Remove the unnecessary async marker from the test
"should set status to success when rehydrating an existing pending query with a
synchronously resolved promise" and any unused async-related artifacts: make the
test function non-async, remove/comment out any unused await usage (there are
none), and keep the synchronous resolution of prefetchPromise via
resolvePrefetch and the overridden dehydrated.queries[0].promise.then behavior
as-is; ensure the test still calls serverQueryClient.prefetchQuery,
dehydrate(...), hydrate(clientQueryClient, dehydrated), and asserts
clientQueryClient.getQueryData(key) === 'server data' and query.state.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: 03783648-8519-4e2d-a4e2-dc7cc1142db1
📒 Files selected for processing (1)
packages/query-core/src/__tests__/hydration.test.tsx
3 participants
🎯 Changes
Fixes #9642
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
Bug Fixes
Tests
Chores