fix(react-query): prevent double-fetching on subsequent visits with HydrationBoundary #9617
Conversation
WalkthroughImplements a temporary per-query runtime hydration flag set during render to suppress refetch-on-mount for existing hydrated queries (unless resolved refetchOnMount === "always"); QueryObserver reads the flag on first subscribe to decide whether to fetch; HydrationBoundary clears the flag in an effect after hydrate(). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HB as HydrationBoundary
participant QC as QueryCache
participant Q as Query
participant Comp as Component
participant QO as QueryObserver
rect rgba(230,240,255,0.5)
note over HB: Render-phase marking
HB->>QC: lookup existing queries from dehydrated state
QC-->>HB: return live queries
HB->>Q: set (query as any)._pendingHydration = true
end
rect rgba(240,255,240,0.5)
note over Comp: Component mounts / observer subscribes
Comp->>QO: subscribe (first listener)
QO->>Q: read _pendingHydration
alt _pendingHydration && resolvedRefetchOnMount != "always"
QO->>Comp: updateResult() (skip fetch)
else
QO->>Q: shouldFetchOnMount(...)
alt should fetch
QO->>Q: execute fetch
else
QO->>Comp: updateResult() (no fetch)
end
end
end
rect rgba(255,245,230,0.5)
note over HB: Effect-phase cleanup
HB->>QC: iterate hydrationQueue
QC->>Q: delete (query as any)._pendingHydration
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit a8e760f
☁️ 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: 2
🧹 Nitpick comments (7)
packages/query-core/src/queryObserver.ts (2)
98-105: Clarify cleanup strategy for the hydration flagComment says it’s “immediately removed in QueryObserver.onSubscribe”, but QueryObserver explicitly doesn’t delete it; HydrationBoundary’s effect removes it. Update the comment to avoid confusion and prevent future regressions.
- // Check hydration flag but DON'T delete it - // The flag is only meaningful during the current render cycle + // Check hydration flag but DON'T delete it here. + // The flag is cleared in HydrationBoundary's effect after hydrate().
468-478: Optional: avoid transient “fetching” state during pending hydrationgetOptimisticResult may set fetching state while we plan to skip fetch due to pending hydration, causing UI spinners without network traffic. Consider honoring _pendingHydration in the optimistic “fetchOnMount” path.
Would you like me to draft a minimal change that checks (query as any)._pendingHydration before applying fetchState in the optimistic branch?
Also applies to: 484-486
packages/react-query/src/HydrationBoundary.tsx (2)
98-101: Comment contradicts implementationThe flag is cleared in the effect below, not in QueryObserver.onSubscribe. Please align the comment.
- // Temporary flag to prevent double-fetching during hydration - // Will be immediately removed in QueryObserver.onSubscribe + // Temporary flag to prevent double-fetching during hydration. + // Cleared in the effect below after hydrate() completes.
95-102: Optional: use a shared Symbol for the internal flag to avoid key collisionsA unique Symbol avoids accidental user collision with _pendingHydration. We can export an internal symbol from query-core and use it here.
I can wire this up with a non-breaking internal export if you want.
packages/react-query/src/__tests__/HydrationBoundary.test.tsx (3)
493-553: Nice coverage for revisit without refetchConsider clearing the client at the end to avoid cross-test leakage.
expect(screen.getByText('fresh-from-server')).toBeInTheDocument() vi.useRealTimers() + queryClient.clear()
555-619: Covers 'always' string; add a test for function form tooGood test for the 'always' literal. Please also add a case where refetchOnMount is a function returning 'always' to guard against regressions.
Example:
test('should refetch when refetchOnMount is a function returning "always"', async () => { vi.useFakeTimers() const queryFn = vi.fn().mockResolvedValueOnce('initial').mockResolvedValueOnce('refetched') const qc = new QueryClient() function Page() { const { data } = useQuery({ queryKey: ['fn-always'], queryFn, refetchOnMount: () => 'always', }) return <div>{data}</div> } const { rerender } = render(<QueryClientProvider client={qc}><Page /></QueryClientProvider>) await vi.waitFor(() => expect(queryFn).toHaveBeenCalledTimes(1)) const serverQC = new QueryClient() await serverQC.prefetchQuery({ queryKey: ['fn-always'], queryFn: () => Promise.resolve('from-server') }) const state = dehydrate(serverQC) queryFn.mockClear() rerender(<QueryClientProvider client={qc}><HydrationBoundary state={state}><Page /></HydrationBoundary></QueryClientProvider>) await vi.runOnlyPendingTimersAsync() expect(queryFn).toHaveBeenCalledTimes(1) vi.useRealTimers() qc.clear() })
620-690: Good multi-observer scenarioLooks correct and guards the intended behavior. Same small cleanup suggestion as above.
expect(screen.getByText('B: fresh-from-server')).toBeInTheDocument() vi.useRealTimers() + queryClient.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/query-core/src/queryObserver.ts(1 hunks)packages/react-query/src/HydrationBoundary.tsx(2 hunks)packages/react-query/src/__tests__/HydrationBoundary.test.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/react-query/src/HydrationBoundary.tsx (1)
packages/query-core/src/queryObserver.ts (1)
query(711-728)
packages/react-query/src/__tests__/HydrationBoundary.test.tsx (2)
packages/query-core/src/hydration.ts (1)
dehydrate(122-163)packages/react-query/src/HydrationBoundary.tsx (1)
HydrationBoundary(25-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
| existingQueries.forEach((q) => { | ||
| const query = queryCache.get(q.queryHash) | ||
| if (query) { | ||
| // Temporary flag to prevent double-fetching during hydration | ||
| // Will be immediately removed in QueryObserver.onSubscribe | ||
| ;(query as any)._pendingHydration = true | ||
| } | ||
| }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Safer flag lifecycle: avoid mutating queries during render; set/cleanup in layout effect
Setting the flag inside useMemo mutates cache objects during render. If a transition is aborted pre-commit, cleanup never runs and the flag can stick. Move flagging into a parent layout effect (runs before children subscribe) and add a fallback cleanup.
Targeted changes:
- Remove render-phase flagging.
- Add a React.useLayoutEffect that sets the flag for hydrationQueue and cleans it up on unmount/change.
- Keep the existing effect for hydrate().
@@
- if (existingQueries.length > 0) {
- existingQueries.forEach((q) => {
- const query = queryCache.get(q.queryHash)
- if (query) {
- // Temporary flag to prevent double-fetching during hydration
- // Will be immediately removed in QueryObserver.onSubscribe
- ;(query as any)._pendingHydration = true
- }
- })
- return existingQueries
- }
+ if (existingQueries.length > 0) {
+ return existingQueries
+ }
@@
React.useEffect(() => {
if (hydrationQueue) {
hydrate(client, { queries: hydrationQueue }, optionsRef.current)
- hydrationQueue.forEach((q) => {
- const query = client.getQueryCache().get(q.queryHash)
- if (query) {
- delete (query as any)._pendingHydration
- }
- })
}
}, [client, hydrationQueue])
+
+ // Ensure flags are set before children subscribe; also clean up on unmount/change.
+ React.useLayoutEffect(() => {
+ if (!hydrationQueue?.length) return
+ const cache = client.getQueryCache()
+ const flagged: string[] = []
+ hydrationQueue.forEach((q) => {
+ const query = cache.get(q.queryHash)
+ if (query) {
+ ;(query as any)._pendingHydration = true
+ flagged.push(q.queryHash)
+ }
+ })
+ return () => {
+ flagged.forEach((hash) => {
+ const q = cache.get(hash)
+ if (q) delete (q as any)._pendingHydration
+ })
+ }
+ }, [client, hydrationQueue])
+
+ // Clear flags after hydrate() completes
+ React.useEffect(() => {
+ if (!hydrationQueue?.length) return
+ const cache = client.getQueryCache()
+ hydrationQueue.forEach((q) => {
+ const query = cache.get(q.queryHash)
+ if (query) delete (query as any)._pendingHydration
+ })
+ }, [client, hydrationQueue])If you prefer minimal scope, we can keep render-phase logic and add a test to prove flags don’t persist after an aborted transition.
Also applies to: 112-117
🏁 Script executed:
#!/bin/bash
file=$(find . -type f -name 'HydrationBoundary.tsx')
if [ -z "$file" ]; then
echo "File not found"
else
sed -n '1,200p' "$file"
fiLength of output: 4661
Move hydration flagging into a useLayoutEffect
Remove the _pendingHydration assignment from the render‐phase useMemo and instead set and clean up the flag inside a React.useLayoutEffect before children subscribe and on unmount/change.
🤖 Prompt for AI Agents
In packages/react-query/src/HydrationBoundary.tsx around lines 95 to 102, remove
the render-phase assignment of (query as any)._pendingHydration inside the
useMemo and instead set these flags inside a React.useLayoutEffect: in the
effect iterate existingQueries, set _pendingHydration = true on each query, and
return a cleanup that deletes the _pendingHydration flag for each query (also
ensure cleanup runs when dependencies change); finally delete the original
assignment from the memo so flagging only happens in the layout effect.
There was a problem hiding this comment.
Since useLayoutEffect runs after the DOM update, isn't it already too late at the time of component rendering?
For example, the following timing issues may exist
- HydrationBoundary renders
- Child component renders (useQuery call) // No flag
- useLayoutEffect executes // May be delayed
There was a problem hiding this comment.
@joseph0926 You're absolutely right! That's an excellent catch on the React lifecycle timing.
The timing you described is exactly correct:
- HydrationBoundary renders (useMemo runs)
- Child components render (useQuery hooks execute, QueryObserver.onSubscribe runs)
- useLayoutEffect executes (too late - the unwanted refetch already happened)
The _pendingHydration flag must be set during the render phase (in useMemo) because:
QueryObserver.onSubscriberuns when child components render and subscribe to queries- This happens before any layout effects execute
- If the flag isn't set yet, the unwanted refetch will be triggered before
useLayoutEffecthas a chance to set the flag
Your original implementation is correct - the flag needs to be set synchronously during the render phase to be available when children subscribe. Moving it to useLayoutEffect would completely break the double-fetching prevention.
The render-phase mutation, while not ideal from a purity standpoint, is necessary here because the flag needs to exist at the exact moment child components subscribe to prevent the unwanted refetch behavior.
🐰💭
Sometimes the render phase is exactly where we need to be...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/query-core/src/queryObserver.ts (2)
103-114: Avoid invoking refetchOnMount function twice on mount.Call the function once, pass its resolved value to shouldFetchOnMount to prevent duplicate user callbacks/side effects.
- const resolvedRefetchOnMount = + const resolvedRefetchOnMount = typeof this.options.refetchOnMount === 'function' ? this.options.refetchOnMount(this.#currentQuery) : this.options.refetchOnMount - const shouldSkipFetch = - hasPendingHydration && resolvedRefetchOnMount !== 'always' + const localOptions = { + ...this.options, + // ensure shouldFetchOnMount doesn't call the function again + refetchOnMount: resolvedRefetchOnMount, + } as typeof this.options + + const shouldSkipFetch = + hasPendingHydration && resolvedRefetchOnMount !== 'always' - if ( - shouldFetchOnMount(this.#currentQuery, this.options) && - !shouldSkipFetch - ) { + const shouldFetch = + shouldFetchOnMount(this.#currentQuery, localOptions) && !shouldSkipFetch + + if (shouldFetch) { this.#executeFetch() } else { this.updateResult() }
98-102: Guard against lingering _pendingHydration suppressing needed fetches.If HydrationBoundary unmounts before its effect runs, the flag might remain and skip future mount fetches. Consider a fallback (e.g., clear-on-unsubscribe/update, or only skip once per observer tick).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/query-core/src/queryObserver.ts(1 hunks)packages/react-query/src/HydrationBoundary.tsx(2 hunks)packages/react-query/src/__tests__/HydrationBoundary.test.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-query/src/HydrationBoundary.tsx
- packages/react-query/src/tests/HydrationBoundary.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/query-core/src/queryObserver.ts (1)
98-114: Hydration-aware mount logic looks correct; honors function-valued refetchOnMount('always).Reading _pendingHydration and resolving refetchOnMount before skipping prevents the double-fetch and preserves 'always'. LGTM.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9617 +/- ##
===========================================
+ Coverage 45.50% 59.74% +14.23%
===========================================
Files 209 138 -71
Lines 8377 5631 -2746
Branches 1897 1529 -368
===========================================
- Hits 3812 3364 -448
+ Misses 4118 1963 -2155
+ Partials 447 304 -143 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/query-core/src/__tests__/queryObserver.test.tsx (5)
1505-1511: Assert the query was found before mutating internal flagIf
findever returns undefined, subsequent expectations would be misleading. Add an explicit guard in each test.Apply this diff in each test right after obtaining
query:- const query = queryClient.getQueryCache().find({ queryKey: key }) + const query = queryClient.getQueryCache().find({ queryKey: key }) + expect(query).toBeDefined() if (query) { ;(query as any)._pendingHydration = true }Also applies to: 1532-1535, 1559-1562
1516-1523: Strengthen the first test by asserting the observer result uses hydrated dataThis proves we didn’t fetch and we’re reading the cached value under hydration gating.
Apply:
const unsubscribe = observer.subscribe(() => undefined) await vi.advanceTimersByTimeAsync(0) expect(queryFn).toHaveBeenCalledTimes(0) + expect(observer.getCurrentResult()).toMatchObject({ + status: 'success', + fetchStatus: 'idle', + data: 'initial-data', + }) expect((query as any)?._pendingHydration).toBe(true)
1571-1579: Also assert evaluation order: refetchOnMount callback runs before the fetchLocks in the intended onSubscribe resolution sequence.
Apply:
expect(refetchOnMountFn).toHaveBeenCalledTimes(1) expect(refetchOnMountFn).toHaveBeenCalledWith(query) expect(queryFn).toHaveBeenCalledTimes(1) + expect( + refetchOnMountFn.mock.invocationCallOrder[0] < + queryFn.mock.invocationCallOrder[0], + ).toBe(true)
1497-1581: DRY up test setup for pending hydrationReduce duplication and make intent explicit by extracting a small helper.
Add this helper near the top of the file (outside the changed block):
function flagPendingHydrationForKey( qc: QueryClient, key: ReadonlyArray<unknown>, data: unknown = 'initial-data', ) { qc.setQueryData(key, data) const q = qc.getQueryCache().find({ queryKey: key }) expect(q).toBeDefined() ;(q as any)!._pendingHydration = true return q! }Then replace the repeated setup in each test:
- queryClient.setQueryData(key, 'initial-data') - - const query = queryClient.getQueryCache().find({ queryKey: key }) - if (query) { - ;(query as any)._pendingHydration = true - } + const query = flagPendingHydrationForKey(queryClient, key, 'initial-data')
1497-1581: Optional: add a multi-subscriber case to mirror real-world mountingValidates that multiple observers mounting during pending hydration don’t trigger a fetch.
You can append:
test('should not fetch with multiple observers during pending hydration', async () => { const key = queryKey() const queryFn = vi.fn().mockResolvedValue('data') const query = flagPendingHydrationForKey(queryClient, key, 'initial-data') const observer1 = new QueryObserver(queryClient, { queryKey: key, queryFn }) const observer2 = new QueryObserver(queryClient, { queryKey: key, queryFn }) const u1 = observer1.subscribe(() => undefined) const u2 = observer2.subscribe(() => undefined) await vi.advanceTimersByTimeAsync(0) expect(queryFn).toHaveBeenCalledTimes(0) expect((query as any)._pendingHydration).toBe(true) u1(); u2() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/__tests__/queryObserver.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/__tests__/queryObserver.test.tsx (1)
packages/query-core/src/queryObserver.ts (2)
query(721-738)QueryObserver(39-764)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/query-core/src/__tests__/queryObserver.test.tsx (1)
1498-1581: Hydration gating cases well coveredGood additions: skip when
_pendingHydrationis set, override withrefetchOnMount: 'always', and functional form coverage including param passing. This aligns with queryObserver’s new onSubscribe logic.
Fixes #9610
Description
This PR fixes the double-fetching issue that happens when using
HydrationBoundarywith server-side data fetching patterns such as React Router’s loader.Problem
When revisiting a page that already has fresh server-side data
useMemouseEffectuseQueryruns and sees stale datarefetchOnMount: true, this triggers an unnecessary refetchuseEffectruns and hydrates with the fresh server dataExample with React Router
Investigation
I added logging to better understand the execution order
HydrationBoundary’suseMemoruns first and defers hydration of existing queriesQueryObserver.onSubscriberuns beforeuseEffect, and since the query appears stale, it triggers a refetchThe core issue is that deferring hydration avoids updates during transitions, but this also creates a short gap where queries look stale and trigger refetching.
Solution
I introduced an internal
_pendingHydrationflag to mark queries that are waiting to be hydratedHydrationBoundarysets this flag on existing queries before deferring hydrationQueryObserverchecks the flag duringonSubscribeand skips the refetch if it is setHydrationBoundaryremoves the flagrefetchOnMount: 'always'is explicitly set, the refetch still occurs as intendedFor implementation, I used
as anywhen setting the flag. I think this is acceptable because the flag is purely internal, temporary, and cleared immediately after use. Adding it to the publicQuerytype would unnecessarily expose details that don’t belong in the public API.Testing
refetchOnMount: 'always'is still respected.No breaking changes – this is a targeted optimization
Summary by CodeRabbit
Bug Fixes
Performance
Tests