fix(useQuery): prevent hydration mismatch when ssr: false and skip: true are combined#13128
Conversation
|
@pavelivanov: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
🦋 Changeset detectedLatest commit: ca88f33 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@phryneas hi, please can you help me understand how to assign a reviewer? I don't have such action in the UI. |
phryneas
left a comment
phryneas
left a comment
There was a problem hiding this comment.
@pavelivanov
Thank you for the PR, sorry for the late response - I kinda missed this one.
I've taken a look, and I agree with the problem, but I think we should try to fix as much as possible from this over in useSSRQuery, as any addition to useQuery will make it into the client bundle.
That means that the two blocks
if (
options === skipToken ||
options.skip ||
options.fetchPolicy === "standby"
) {
return withoutObservableAccess({
...baseResult,
...skipStandbyResult,
});
}and
if (options.ssr === false || options.fetchPolicy === "no-cache") {
return withoutObservableAccess({
...baseResult,
...useQuery.ssrDisabledResult,
});
}in there would need to switch places.
The only change in useQuery in the end should be
- () => (ssr === false ? useQuery.ssrDisabledResult : resultData.current)
+ () => (ssr === false || observable.options.fetchPolicy === "no-cache" ? useQuery.ssrDisabledResult : resultData.current)Could you please implement these changes?
|
|
||
| When both options were combined, the server would return `loading: false` (because `useSSRQuery` checks `skip` first), but the client's `getServerSnapshot` was returning `ssrDisabledResult` with `loading: true`, causing a hydration mismatch. This fix updates `getServerSnapshot` to check `isSkipped` before the `ssr` option to match server behavior. |
There was a problem hiding this comment.
| When both options were combined, the server would return `loading: false` (because `useSSRQuery` checks `skip` first), but the client's `getServerSnapshot` was returning `ssrDisabledResult` with `loading: true`, causing a hydration mismatch. This fix updates `getServerSnapshot` to check `isSkipped` before the `ssr` option to match server behavior. | |
| When both options were combined, the server would return `loading: false` (because `useSSRQuery` checks `skip` first), but the client's `getServerSnapshot` was returning `ssrDisabledResult` with `loading: true`, causing a hydration mismatch. |
- prioritize ssr: false over skip in useSSRQuery so the server always returns ssrDisabledResult (loading: true), matching the client’s getServerSnapshot and preventing hydration mismatches. - handle fetchPolicy: "no-cache" in getServerSnapshot to avoid similar inconsistencies.
|
I leave this for history: SSR test ( Hooks test ( Why no hydration mismatch:
During hydration, React calls So during hydration with After hydration completes, React switches to The hydration test (https://github.com/apollographql/apollo-client/pull/13128/changes#diff-d2c5924a921e0cb39c6d9910863c40eaab64a7901aa9414ab702a6eda79f7e82R1204) validates this exact flow end-to-end: |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes a hydration mismatch in useQuery when ssr: false and skip: true by changing SSR data provisioning to consider fetchPolicy, adds an end-to-end hydration test, updates Jest ignores, and adds a changeset documenting the patch. Changes
Sequence Diagram(s)mermaid ServerRenderer->>ApolloClient_SSR: render component (useQuery ssr:false, skip:true) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/fix-usequery-ssr-skip-hydration.mdsrc/react/hooks/__tests__/useQuery.test.tsxsrc/react/hooks/useQuery.tssrc/react/ssr/__tests__/useQuery.test.tsxsrc/react/ssr/useSSRQuery.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/react/hooks/useQuery.ts (2)
src/react/index.ts (1)
useQuery(8-8)src/react/index.react-server.ts (1)
useQuery(42-42)
src/react/hooks/__tests__/useQuery.test.tsx (6)
src/core/ApolloClient.ts (2)
query(982-1036)ApolloClient(700-1685)src/core/ObservableQuery.ts (1)
query(312-314)src/core/index.ts (3)
gql(183-183)NetworkStatus(30-30)ApolloClient(17-17)src/react/hooks/useQuery.ts (1)
useQuery(380-399)src/testing/react/MockedProvider.tsx (1)
MockedProvider(33-84)src/testing/core/mocking/mockLink.ts (1)
MockLink(90-261)
src/react/ssr/useSSRQuery.ts (2)
src/react/hooks/useQuery.ts (1)
useQuery(380-399)src/react/index.ts (2)
useQuery(8-8)skipToken(18-18)
src/react/ssr/__tests__/useQuery.test.tsx (3)
src/react/hooks/useQuery.ts (1)
useQuery(380-399)src/react/index.ts (1)
useQuery(8-8)src/react/index.react-server.ts (1)
useQuery(42-42)
⏰ 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). (1)
- GitHub Check: Build and Test
🔇 Additional comments (7)
.changeset/fix-usequery-ssr-skip-hydration.md (1)
1-7: LGTM!The changeset correctly documents the hydration mismatch fix as a patch release. The description accurately captures the root cause (ordering mismatch between
useSSRQueryandgetServerSnapshot) and the behavioral change.src/react/ssr/__tests__/useQuery.test.tsx (1)
131-156: LGTM!The test correctly validates the fixed SSR behavior:
- Updated description accurately reflects the new expectation (
loading: trueduring SSR).- The inline comment on lines 137-139 clearly explains the priority semantics (
ssr: falseoverskip: true) and how this prevents hydration mismatches.- Assertions align with the
ssrDisabledResultbehavior described in the PR objectives.src/react/hooks/__tests__/useQuery.test.tsx (2)
1123-1158: Covers the ssr:false + skip:true client result as intended.
Nice focused regression check for the loading state.
1203-1272: Hydration mismatch regression test looks solid.
Good coverage of server render → hydrateRoot → no recoverable errors.src/react/hooks/useQuery.ts (1)
594-598: LGTM: getServerSnapshot treatsno-cacheas SSR-disabled.Line 595 now mirrors the SSR gating logic by returning
ssrDisabledResultforfetchPolicy: "no-cache", which should prevent hydration mismatches.src/react/ssr/useSSRQuery.ts (2)
49-56: LGTM: SSR-disabled branch now coversssr: falseandno-cache.Line 50-56 ensures SSR returns the disabled placeholder for both cases, aligning SSR output with client hydration logic.
59-67: LGTM: skip/standby branch preserved after SSR-disabled check.Line 60-67 keeps the skip/standby result intact once SSR-disabled scenarios are ruled out, maintaining consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/react/hooks/__tests__/useQuery.test.tsx`:
- Around line 1160-1201: The test name claims it uses "ssr: false in
defaultOptions" but the ApolloClient is created without defaultOptions; update
the ApolloClient instantiation in this test (the new ApolloClient({...}) call)
to include defaultOptions: { watchQuery: { ssr: false } } so the test actually
exercises that behavior, or alternatively rename the it(...) description to
reflect that only ssrMode: false is set (e.g., "should return loading: false on
the client when skipToken is used with ssrMode: false"); pick one approach and
make the corresponding change to keep the test name accurate.
| it("should return loading: false on the client when skipToken is used with ssr: false in defaultOptions", async () => { | ||
| const query = gql` | ||
| { | ||
| hello | ||
| } | ||
| `; | ||
| const mocks = [ | ||
| { | ||
| request: { query }, | ||
| result: { data: { hello: "world" } }, | ||
| }, | ||
| ]; | ||
|
|
||
| const client = new ApolloClient({ | ||
| cache: new InMemoryCache(), | ||
| link: new MockLink(mocks), | ||
| ssrMode: false, | ||
| }); | ||
|
|
||
| using _disabledAct = disableActEnvironment(); | ||
| const { takeSnapshot } = await renderHookToSnapshotStream( | ||
| () => useQuery(query, skipToken), | ||
| { | ||
| wrapper: ({ children }) => ( | ||
| <ApolloProvider client={client}>{children}</ApolloProvider> | ||
| ), | ||
| } | ||
| ); | ||
|
|
||
| { | ||
| const result = await takeSnapshot(); | ||
|
|
||
| expect(result).toStrictEqualTyped({ | ||
| data: undefined, | ||
| dataState: "empty", | ||
| loading: false, | ||
| networkStatus: NetworkStatus.ready, | ||
| previousData: undefined, | ||
| variables: {}, | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test name doesn’t match the setup (no defaultOptions configured).
The client here doesn’t set defaultOptions, so the test doesn’t actually exercise “ssr: false in defaultOptions.” Either add the defaultOptions configuration or rename the test to reflect the ssrMode: false setup.
✏️ Rename to match current setup
-it("should return loading: false on the client when skipToken is used with ssr: false in defaultOptions", async () => {
+it("should return loading: false on the client when skipToken is used with ssrMode: false", async () => {📝 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.
| it("should return loading: false on the client when skipToken is used with ssr: false in defaultOptions", async () => { | |
| const query = gql` | |
| { | |
| hello | |
| } | |
| `; | |
| const mocks = [ | |
| { | |
| request: { query }, | |
| result: { data: { hello: "world" } }, | |
| }, | |
| ]; | |
| const client = new ApolloClient({ | |
| cache: new InMemoryCache(), | |
| link: new MockLink(mocks), | |
| ssrMode: false, | |
| }); | |
| using _disabledAct = disableActEnvironment(); | |
| const { takeSnapshot } = await renderHookToSnapshotStream( | |
| () => useQuery(query, skipToken), | |
| { | |
| wrapper: ({ children }) => ( | |
| <ApolloProvider client={client}>{children}</ApolloProvider> | |
| ), | |
| } | |
| ); | |
| { | |
| const result = await takeSnapshot(); | |
| expect(result).toStrictEqualTyped({ | |
| data: undefined, | |
| dataState: "empty", | |
| loading: false, | |
| networkStatus: NetworkStatus.ready, | |
| previousData: undefined, | |
| variables: {}, | |
| }); | |
| } | |
| }); | |
| it("should return loading: false on the client when skipToken is used with ssrMode: false", async () => { | |
| const query = gql` | |
| { | |
| hello | |
| } | |
| `; | |
| const mocks = [ | |
| { | |
| request: { query }, | |
| result: { data: { hello: "world" } }, | |
| }, | |
| ]; | |
| const client = new ApolloClient({ | |
| cache: new InMemoryCache(), | |
| link: new MockLink(mocks), | |
| ssrMode: false, | |
| }); | |
| using _disabledAct = disableActEnvironment(); | |
| const { takeSnapshot } = await renderHookToSnapshotStream( | |
| () => useQuery(query, skipToken), | |
| { | |
| wrapper: ({ children }) => ( | |
| <ApolloProvider client={client}>{children}</ApolloProvider> | |
| ), | |
| } | |
| ); | |
| { | |
| const result = await takeSnapshot(); | |
| expect(result).toStrictEqualTyped({ | |
| data: undefined, | |
| dataState: "empty", | |
| loading: false, | |
| networkStatus: NetworkStatus.ready, | |
| previousData: undefined, | |
| variables: {}, | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/react/hooks/__tests__/useQuery.test.tsx` around lines 1160 - 1201, The
test name claims it uses "ssr: false in defaultOptions" but the ApolloClient is
created without defaultOptions; update the ApolloClient instantiation in this
test (the new ApolloClient({...}) call) to include defaultOptions: { watchQuery:
{ ssr: false } } so the test actually exercises that behavior, or alternatively
rename the it(...) description to reflect that only ssrMode: false is set (e.g.,
"should return loading: false on the client when skipToken is used with ssrMode:
false"); pick one approach and make the corresponding change to keep the test
name accurate.
✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
Okay I'm sorry I actually liked the behavior of your initial solution better 😅 I'm going to restore your original code and tweak it a little bit. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/ssr/__tests__/useQueryEndToEnd.test.tsx (1)
24-29: Consider resettingrenderedarray before client hydration for clearer assertions.The
renderedarray accumulates entries from both server (prerenderStatic) and client (hydrateRoot) renders. While the final assertion at lines 103-107 validates all entries are consistent, resetting the array after server render would make it easier to reason about client-specific render behavior separately.That said, the current approach does validate the important invariant that all renders (server and client) produce identical state.
Optional: Reset array between phases for clarity
expect(serverClient.extract()).toEqual({}); + rendered.length = 0; // Reset for client-side tracking const container = document.createElement("div");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/react/ssr/__tests__/useQueryEndToEnd.test.tsx` around lines 24 - 29, The test collects server and client render entries into the same rendered array, which makes it harder to reason about client-only renders; before calling hydrateRoot (after prerenderStatic completes), clear or reinitialize the rendered array (the variable named rendered) so client hydration pushes start from an empty array, then assert on client-specific entries separately while still keeping the existing overall invariant checks if desired; locate the array in the test and the two phases around prerenderStatic and hydrateRoot to place the reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/react/ssr/__tests__/useQueryEndToEnd.test.tsx`:
- Around line 24-29: The test collects server and client render entries into the
same rendered array, which makes it harder to reason about client-only renders;
before calling hydrateRoot (after prerenderStatic completes), clear or
reinitialize the rendered array (the variable named rendered) so client
hydration pushes start from an empty array, then assert on client-specific
entries separately while still keeping the existing overall invariant checks if
desired; locate the array in the test and the two phases around prerenderStatic
and hydrateRoot to place the reset.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/fix-usequery-ssr-skip-hydration.mdsrc/react/hooks/useQuery.tssrc/react/ssr/__tests__/useQueryEndToEnd.test.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use@apollo/client/*/internalpaths for accessing internal APIs
Use RxJS observables for reactive programming patterns
Follow existing code style enforced by ESLint and Prettier
Files:
src/react/hooks/useQuery.tssrc/react/ssr/__tests__/useQueryEndToEnd.test.tsx
🧠 Learnings (1)
📚 Learning: 2026-02-19T17:01:37.124Z
Learnt from: CR
Repo: apollographql/apollo-client PR: 0
File: .github/instructions/apollo-client.instructions.md:0-0
Timestamp: 2026-02-19T17:01:37.124Z
Learning: Use the apollo-client skill when answering questions about Apollo Client setup, configuration, usage, or troubleshooting
Applied to files:
.changeset/fix-usequery-ssr-skip-hydration.md
🔇 Additional comments (3)
src/react/hooks/useQuery.ts (1)
547-547: LGTM! The SSR-disabled logic correctly handles the skip + ssr: false combination.The implementation uses
fetchPolicy !== "standby"to detect skipped queries, sinceuseOptionssetsfetchPolicy: "standby"whenskip: true(lines 518-522). This ensures:
ssr: false+skip: false→ returnsssrDisabledResult(loading: true)ssr: false+skip: true→ returnsresultData.current(loading: false), matching the serverfetchPolicy: "no-cache"→ returnsssrDisabledResult(no caching for SSR)This aligns the
getServerSnapshotbehavior withuseSSRQuery, fixing the hydration mismatch.Also applies to: 595-601
.changeset/fix-usequery-ssr-skip-hydration.md (1)
1-7: LGTM!The changeset correctly documents the patch fix with a clear explanation of the root cause: the server returning
loading: falsefor skipped queries while the client'sgetServerSnapshotwas returningloading: true.src/react/ssr/__tests__/useQueryEndToEnd.test.tsx (1)
11-111: Well-structured end-to-end hydration test!The test comprehensively validates the fix by:
- Rendering server-side with
prerenderStaticand assertingloading: false- Hydrating client-side and capturing any hydration errors
- Verifying zero hydration mismatches via
hydrationErrors.toHaveLength(0)- Confirming state consistency across all render phases
This directly exercises the code path that was causing the hydration mismatch.
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 `@config/jest.config.ts`:
- Around line 97-101: The react17TestFileIgnoreList is missing the new
React-18-only test so running against React 17 will fail; update the
react17TestFileIgnoreList (the variable used alongside
testPathIgnorePatterns/standardReact18Config) to include
"src/react/ssr/__tests__/useQueryEndToEnd.test.tsx" just like
"src/react/ssr/__tests__/prerenderStatic.test.tsx" is excluded, ensuring the new
test is ignored when running the React 17 test suite.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/jest.config.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: Build and Test
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use@apollo/client/*/internalpaths for accessing internal APIs
Use RxJS observables for reactive programming patterns
Follow existing code style enforced by ESLint and Prettier
Files:
config/jest.config.ts
🧠 Learnings (2)
📚 Learning: 2026-02-19T17:01:54.243Z
Learnt from: CR
Repo: apollographql/apollo-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:01:54.243Z
Learning: Use Jest with ts-jest for testing TypeScript code
Applied to files:
config/jest.config.ts
📚 Learning: 2026-02-19T17:01:54.243Z
Learnt from: CR
Repo: apollographql/apollo-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-19T17:01:54.243Z
Learning: Testing utilities should be located in `src/testing/` with core testing utilities in `core/` subdirectory and React testing utilities (MockedProvider) in `react/` subdirectory
Applied to files:
config/jest.config.ts
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/[email protected] ### Patch Changes - [#13128](#13128) [`6c0b8e4`](6c0b8e4) Thanks [@pavelivanov](https://github.com/pavelivanov)! - Fix `useQuery` hydration mismatch when `ssr: false` and `skip: true` are used together When both options were combined, the server would return `loading: false` (because `useSSRQuery` checks `skip` first), but the client's `getServerSnapshot` was returning `ssrDisabledResult` with `loading: true`, causing a hydration mismatch. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
4 participants
Summary
useQuerywith bothskip: trueandssr: falseoptionsgetServerSnapshotbehavior withuseSSRQueryby checkingskipbeforessrProblem
When using
useQuerywith bothskip: trueandssr: false, a hydration mismatch occurs because:useSSRQuery): Checksskipfirst → returnsloading: falsegetServerSnapshot): Only checksssr: false→ returnsloading: trueThis causes React to throw a hydration error since server rendered
loading: falsebut client initially showsloading: true.Code flow before fix:
useSSRQuerychecksskipFIRSTloading: falsegetServerSnapshotchecksssr === falseonlyloading: truegetSnapshotreturns observable stateloading: falseSolution
Modified
getServerSnapshotinuseQuery.tsto checkisSkippedbefore checkingssr === false, matching the condition ordering inuseSSRQuery: