fix(preact-query): hydrate unobserved queries before observers subscribe to avoid a redundant refetch - #11157
Conversation
Sorry, something went wrong.
|
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 reviews.auto_review.auto_pause_after_reviewed_commits setting. 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 Plus Run ID: a39d2aee-001d-4541-a406-e8c55e4c221c 📥 CommitsReviewing files that changed from the base of the PR and between e6def1f and eebc2c1. 📒 Files selected for processing (2)
📝 Walkthrough WalkthroughHydrationBoundary now hydrates unobserved queries during commit with useLayoutEffect. Observed queries remain deferred to the passive effect. Tests cover suspended renders, remounts, observed queries, restoring mode, and duplicate deserialization. ChangesPreact HydrationBoundary hydration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HydrationBoundary
participant QueryCache
participant useQuery
HydrationBoundary->>QueryCache: hydrate unobserved queries during commit
HydrationBoundary->>QueryCache: hydrate deferred observed queries after commit
QueryCache-->>useQuery: provide hydrated cached data
Possibly related PRs
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
MILLERMARRU
left a comment
There was a problem hiding this comment.
This is the preact-query counterpart of #11137, and it takes a meaningfully different approach worth flagging rather than just mirroring it.
#11137 moves the whole hydrationQueue into a layout effect, existing queries included, regardless of whether anything is currently observing them. Looking at that PR's own test diff, the assertion for an on-screen existing query changed from "should not have updated at this point" (render phase) to expecting the text to already show the hydrated value right after render, since layout effects flush synchronously before the test's next assertion. That's a behavior change for observed queries too, not just the unobserved ones the bug report was actually about.
This PR keeps that distinction instead: only queries with zero observers move to useLayoutEffect (the unobserved half of HydrationQueue), existing queries that are actually on screen still wait for the passive useEffect, same as before. The new "should not hydrate a query that is on screen while a sibling suspends" test locks that in, an observed query stays untouched while a sibling boundary is still suspended. That matches the original comment in this file about not wanting to surprise users with updates to state that's currently rendered, which #11137's version seems to relax for react-query.
Not sure which one is intentional. If preact-query is supposed to track react-query's behavior 1:1, this is worth reconciling one way or the other, either #11137 needs the same observed/unobserved split, or this PR is doing more than the reported bug strictly needs. Either way, the mechanism in this PR is correct for what it does, getObserversCount() === 0 is the right check and the layout effect ordering argument in the comments holds up against how passive effects flush in child-before-parent order.
Sorry, something went wrong.
|
Good catch, and you're right about what #11137 does to observed queries. The two differ because React and Preact don't agree on what a suspended render is. React never commits one, so in #11137 the layout effect simply doesn't run for a render that gets thrown away, React keeps that guarantee for us. Preact commits the tree, runs the layout effects and only then swaps in the fallback, so moving the whole queue there lets a route that suspends and never arrives overwrite data a sibling is still showing on screen. That's what the sidebar test locks in. So I'd reconcile it the other way round and put the same split in #11137. I tried that on the branch: applied the observed/unobserved split and reverted the test file back to what's on main, keeping only the new inactive-query test. The whole file passes as is, 13 tests, nothing else touched. The existing assertions stay, including "Existing observer should not have updated at this point" and the aborted transition one, and the timing change for observed queries you pointed at goes away, they keep updating after paint like before. Pushing that to #11137 now, then both packages have the same shape and the bug fix is the only behavior that changes. |
Sorry, something went wrong.
|
Pushed to #11137, both are the same shape now: the observer check moved into the layout effect in this one too, so the memo stays untouched and the diff is just the two effects. In react-query the split needed no test changes at all, the file is additions only. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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/preact-query/src/HydrationBoundary.tsx`: - Around line 120-122: Prevent duplicate hydration retryers by deduplicating query hashes between the layout and passive hydration passes in HydrationBoundary, or equivalently within hydrate. Ensure an unobserved existing query with an already-rejected initialPromise is hydrated through only one retry path, and add a regression test covering this scenario.
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 Plus
Run ID: 295aec3e-8c85-435f-a108-6d472284e6ad
📥 CommitsReviewing files that changed from the base of the PR and between f17d363 and c975625.
📒 Files selected for processing (1)- packages/preact-query/src/HydrationBoundary.tsx
Sorry, something went wrong.
|
Follow up on the reconciliation: I had it backwards, the two packages can't share the same shape. I put the split in #11137 and it breaks the fix there. React cleans up a removed subtree's subscriptions in the passive phase, after the boundary's layout effect, so when a page unmounts and remounts in the same commit the outgoing observer is still on the query and the split skips it. Preact tears the old tree down while it diffs, so by the time the boundary commits the query really has no subscribers and the split works. Added a test for that case here, it passes. So #11137 goes back to hydrating everything in the layout effect, which is safe there because React never commits a render it throws away, and this one keeps the split because Preact does commit a suspended tree. Two different behaviours out of two different commit models, not a choice. Also caught while reviewing: nothing subscribes while isRestoring is true, so a query on screen looks unsubscribed. That path now waits for the passive effect, with a test. |
Sorry, something went wrong.
…ibe to avoid a redundant refetch
🎯 Changes
Fixes #11155.
HydrationBoundary waits until after render to hydrate queries that are already
cached, so a render that never commits can't change what's on screen. But
useSyncExternalStore subscribes from a passive effect too, and child effects
run before the parent's. So on a revisit — query cached, page unmounted — the
observer gets there first, reads the stale entry, and refetches the exact data
the dehydrated state is already carrying.
Same race as #11137 on the react-query side, but that fix doesn't port over.
React never commits a render that suspends; preact does. It diffs, commits, runs
layout effects, then swaps in the fallback. So moving the whole queue to a layout
effect would let a route that suspends and never arrives overwrite data a sibling
is still showing.
Hence only part of it moves: queries nobody is observing when the boundary
renders hydrate in a layout effect, the rest keep waiting. Nothing can be
rendering a query with no observers, so hydrating one early can't change what's
on screen — the whole reason existing queries wait. And it lands before a
remounting observer subscribes, which is what kills the refetch.
No isServer branch on the layout effect: preact-render-to-string sets
options._skipEffects, which skips useLayoutEffect and useEffect alike. The
useSyncExternalStore shim here already calls useLayoutEffect unconditionally
for the same reason.
Tests
is fresh. Fails on main.
main, fails if you move the whole queue instead.
and the observer is gone before the boundary renders. The query is now
indistinguishable from one nobody watches, so it asserts the data does land —
the sibling test above covers what it was protecting.
✅ Checklist
🚀 Release Impact
🤖 Generated with Claude Code
Summary by CodeRabbit