🤖🤖🤖 fix(masking): preserve referential equality of masked data on refetch#13184
🤖🤖🤖 fix(masking): preserve referential equality of masked data on refetch#13184audrius-savickas wants to merge 1 commit into
Conversation
|
@audrius-savickas: 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/ |
✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
🦋 Changeset detectedLatest commit: d4a0cf8 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 |
|
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:
📝 WalkthroughWalkthroughObservableQuery now caches the last masked query, input, and result and reuses the cached masked object when appropriate to preserve referential equality on identical refetch results; includes a regression test and a patch changeset documenting the fix. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/hooks/__tests__/useQuery.test.tsx (1)
11970-11977: Assert referential equality on the settled refetch result, not the first emitted frame.Line 11973 currently consumes only one render after
refetch(). In this suite, refetch often emits an intermediateNetworkStatus.refetchframe first, so this can pass without validating the final post-network masked result.Proposed fix
- // Trigger refetch with identical result - await initialSnapshot.refetch(); - - const { snapshot: refetchSnapshot } = await renderStream.takeRender(); - - // The masked data should be the same reference since the underlying - // data hasn't changed - expect(refetchSnapshot.data).toBe(initialSnapshot.data); + // Trigger refetch with identical result + await initialSnapshot.refetch(); + + let refetchSnapshot: useQuery.Result<Query, Record<string, never>>; + do { + ({ snapshot: refetchSnapshot } = await renderStream.takeRender()); + } while (refetchSnapshot.networkStatus !== NetworkStatus.ready); + + // The masked data should be the same reference since the underlying + // data hasn't changed + expect(refetchSnapshot.data).toBe(initialSnapshot.data); + await expect(renderStream).not.toRerender();🤖 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 11970 - 11977, The test currently only awaits one render after calling initialSnapshot.refetch() and may capture an intermediate NetworkStatus.refetch frame; modify the assertion to consume frames from renderStream until the settled post-refetch frame is emitted (i.e., skip frames where network status is NetworkStatus.refetch or where the snapshot is not the final masked result) and then assert that refetchSnapshot.data is the same reference as initialSnapshot.data; specifically, replace the single await renderStream.takeRender() with logic that repeatedly calls renderStream.takeRender() (or an equivalent helper) and checks the snapshot's network status or settled flag before performing expect(refetchSnapshot.data).toBe(initialSnapshot.data).
🤖 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/hooks/__tests__/useQuery.test.tsx`:
- Around line 11970-11977: The test currently only awaits one render after
calling initialSnapshot.refetch() and may capture an intermediate
NetworkStatus.refetch frame; modify the assertion to consume frames from
renderStream until the settled post-refetch frame is emitted (i.e., skip frames
where network status is NetworkStatus.refetch or where the snapshot is not the
final masked result) and then assert that refetchSnapshot.data is the same
reference as initialSnapshot.data; specifically, replace the single await
renderStream.takeRender() with logic that repeatedly calls
renderStream.takeRender() (or an equivalent helper) and checks the snapshot's
network status or settled flag before performing
expect(refetchSnapshot.data).toBe(initialSnapshot.data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b6896e7-ff5e-4e91-b87a-4ea7f0ebf2ef
📒 Files selected for processing (3)
.changeset/fix-masked-data-referential-equality.mdsrc/core/ObservableQuery.tssrc/react/hooks/__tests__/useQuery.test.tsx
📜 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:
src/core/ObservableQuery.tssrc/react/hooks/__tests__/useQuery.test.tsx
🧠 Learnings (1)
📚 Learning: 2026-03-09T13:28:57.460Z
Learnt from: phryneas
Repo: apollographql/apollo-client PR: 13132
File: src/core/defaultOptions.ts:5-7
Timestamp: 2026-03-09T13:28:57.460Z
Learning: In the apollographql/apollo-client repository, do not flag lint issues related to Biome for TypeScript files. There is no biome.json/biome.jsonc and no Biome dependency in package.json. Do not suggest biome-ignore comments or Biome-specific lint fixes for any TS file.
Applied to files:
src/core/ObservableQuery.ts
🔇 Additional comments (2)
src/core/ObservableQuery.ts (1)
1726-1754: Well-designed memoization for preserving masked data referential equality.The implementation correctly addresses the issue where
maskOperationalways produces new object references. The dual-check approach (reference equality first, then deep equality) is efficient and handles both the cache-hit scenario (same input reference) and the network-refetch scenario (deeply equal masked output).A few observations:
- The short-circuit on line 1738 correctly handles cases where masking is disabled or data is null/undefined
- The reference check (
result.data === this.lastMaskedInput) is tried first, avoiding unnecessary deep comparisons- The deep equality fallback ensures stability even when the cache returns a new reference for identical data
One consideration:
lastMaskedInputandlastMaskedResultpersist acrossreset()andstop()calls. This is likely fine since the memoization remains valid if the query/options don't change, but worth noting if unexpected behavior occurs after resets..changeset/fix-masked-data-referential-equality.md (1)
1-5: LGTM! Well-formatted changeset for this bug fix.The changeset correctly specifies a patch version bump for
@apollo/client, uses conventional commit format, and clearly describes the fix—preserving referential equality of masked data on refetch. The description is concise and user-facing, which is appropriate for release notes.
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
2 times, most recently
from
fcea3af to
a70e5ab
Compare
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 `@src/core/ObservableQuery.ts`:
- Around line 1751-1757: The memoization currently reuses this.lastMaskedResult
purely on raw result.data identity, which can be invalid if this.query has
changed; modify the masking cache to also record the query identity when the
mask is produced (e.g., set this.lastMaskedQuery = this.query or a stable
query-version token at the time you compute/assign this.lastMaskedResult) and
guard the reuse by checking that stored identity matches the current this.query
(add condition like this.lastMaskedQuery === this.query alongside the existing
checks before returning the cached masked result); also ensure you update
this.lastMaskedQuery whenever you update
this.lastMaskedResult/this.lastMaskedInput.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c17a3ec-b5a3-4f97-8e50-4072dec3153f
📒 Files selected for processing (3)
.changeset/fix-masked-data-referential-equality.mdsrc/core/ObservableQuery.tssrc/react/hooks/__tests__/useQuery.test.tsx
✅ Files skipped from review due to trivial changes (2)
- .changeset/fix-masked-data-referential-equality.md
- src/react/hooks/tests/useQuery.test.tsx
📜 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:
src/core/ObservableQuery.ts
🧠 Learnings (1)
📚 Learning: 2026-03-09T13:28:57.460Z
Learnt from: phryneas
Repo: apollographql/apollo-client PR: 13132
File: src/core/defaultOptions.ts:5-7
Timestamp: 2026-03-09T13:28:57.460Z
Learning: In the apollographql/apollo-client repository, do not flag lint issues related to Biome for TypeScript files. There is no biome.json/biome.jsonc and no Biome dependency in package.json. Do not suggest biome-ignore comments or Biome-specific lint fixes for any TS file.
Applied to files:
src/core/ObservableQuery.ts
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
from
a70e5ab to
6feb45d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/hooks/__tests__/useQuery.test.tsx (1)
11876-11985: Consider adding the array-field companion case from issue scope.Lines 11876-11985 validate single-object masking. A sibling test for masked array fields would fully cover the
#13181behavior matrix and guard list-specific regressions.If you want, I can draft that companion test in the same render-stream pattern.
🤖 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 11876 - 11985, Add a new test mirroring "preserves referential equality of masked data on refetch with identical results" but using an array field to cover list-specific behavior: create a query (e.g., MaskedArrayQuery) that returns currentUsers: [User] and a fragment UserFields on User (age) so masking applies to array elements; set up mocks with two identical responses for that query, create an ApolloClient with dataMasking and MockLink, render via createRenderStream and an App that calls useQuery(query) and pushes snapshots to renderStream, then perform the refetch on initialSnapshot and loop taking renders until refetchSnapshot.networkStatus === NetworkStatus.ready; finally assert that refetchSnapshot.data.currentUsers (the masked array) is referentially equal to initialSnapshot.data.currentUsers (use .toBe) and that renderStream does not rerender. Ensure test uses the same helpers (useQuery, createRenderStream, disableActEnvironment, NetworkStatus) and naming consistent with the existing test.
🤖 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/hooks/__tests__/useQuery.test.tsx`:
- Around line 11876-11985: Add a new test mirroring "preserves referential
equality of masked data on refetch with identical results" but using an array
field to cover list-specific behavior: create a query (e.g., MaskedArrayQuery)
that returns currentUsers: [User] and a fragment UserFields on User (age) so
masking applies to array elements; set up mocks with two identical responses for
that query, create an ApolloClient with dataMasking and MockLink, render via
createRenderStream and an App that calls useQuery(query) and pushes snapshots to
renderStream, then perform the refetch on initialSnapshot and loop taking
renders until refetchSnapshot.networkStatus === NetworkStatus.ready; finally
assert that refetchSnapshot.data.currentUsers (the masked array) is
referentially equal to initialSnapshot.data.currentUsers (use .toBe) and that
renderStream does not rerender. Ensure test uses the same helpers (useQuery,
createRenderStream, disableActEnvironment, NetworkStatus) and naming consistent
with the existing test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3d6a96d-06c4-47c3-be40-9639a0864937
📒 Files selected for processing (3)
.changeset/fix-masked-data-referential-equality.mdsrc/core/ObservableQuery.tssrc/react/hooks/__tests__/useQuery.test.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-masked-data-referential-equality.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/ObservableQuery.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:
src/react/hooks/__tests__/useQuery.test.tsx
🔇 Additional comments (1)
src/react/hooks/__tests__/useQuery.test.tsx (1)
11876-11985: Solid regression coverage for masked refetch stability.This test directly targets the refetch referential-equality regression and verifies the masked-data identity behavior with a clear
toBeassertion.
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
from
6feb45d to
0300ce8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/ObservableQuery.ts (1)
1726-1771: LGTM! Memoization correctly preserves referential equality with query identity guard.The implementation properly addresses the referential equality issue for masked data on refetch. The query identity check (
this.lastMaskedQuery === query) correctly invalidates the cache when the query changes via reobserve, and the dual condition (input reference OR deep equality) efficiently handles both cache-hit and data-equality scenarios.Minor optimization opportunity: Consider updating
lastMaskedInputin the reuse branch to avoid redundant deep equality checks on subsequent calls when the input reference has changed but the masked output is equal.🔧 Optional optimization
if ( this.lastMaskedResult !== undefined && this.lastMaskedQuery === query && (result.data === this.lastMaskedInput || equal(masked, this.lastMaskedResult)) ) { + this.lastMaskedInput = result.data; return { ...result, data: this.lastMaskedResult }; }This ensures that if the input reference changes but the masked output is equal, subsequent calls with the same new input reference will hit the fast path (
result.data === this.lastMaskedInput) instead of performing a deep equality check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ObservableQuery.ts` around lines 1726 - 1771, The reuse branch in maskResult currently returns the previous lastMaskedResult without updating lastMaskedInput, so when result.data has changed but masked equals lastMaskedResult we still pay deep-equality cost on the next call; modify maskResult (method name: maskResult) so that inside the reuse branch (the if that checks this.lastMaskedResult !== undefined && this.lastMaskedQuery === query && (result.data === this.lastMaskedInput || equal(masked, this.lastMaskedResult))) you also set this.lastMaskedInput = result.data (and keep lastMaskedQuery/lastMaskedResult as-is) before returning, ensuring subsequent calls short-circuit on the reference equality fast path.
🤖 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/core/ObservableQuery.ts`:
- Around line 1726-1771: The reuse branch in maskResult currently returns the
previous lastMaskedResult without updating lastMaskedInput, so when result.data
has changed but masked equals lastMaskedResult we still pay deep-equality cost
on the next call; modify maskResult (method name: maskResult) so that inside the
reuse branch (the if that checks this.lastMaskedResult !== undefined &&
this.lastMaskedQuery === query && (result.data === this.lastMaskedInput ||
equal(masked, this.lastMaskedResult))) you also set this.lastMaskedInput =
result.data (and keep lastMaskedQuery/lastMaskedResult as-is) before returning,
ensuring subsequent calls short-circuit on the reference equality fast path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d02bca4a-501c-4938-81d0-a36dcf6989c3
📒 Files selected for processing (3)
.changeset/fix-masked-data-referential-equality.mdsrc/core/ObservableQuery.tssrc/react/hooks/__tests__/useQuery.test.tsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-masked-data-referential-equality.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/hooks/tests/useQuery.test.tsx
📜 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:
src/core/ObservableQuery.ts
🧠 Learnings (1)
📚 Learning: 2026-03-09T13:28:57.460Z
Learnt from: phryneas
Repo: apollographql/apollo-client PR: 13132
File: src/core/defaultOptions.ts:5-7
Timestamp: 2026-03-09T13:28:57.460Z
Learning: In the apollographql/apollo-client repository, do not flag lint issues related to Biome for TypeScript files. There is no biome.json/biome.jsonc and no Biome dependency in package.json. Do not suggest biome-ignore comments or Biome-specific lint fixes for any TS file.
Applied to files:
src/core/ObservableQuery.ts
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
from
0300ce8 to
3f0068e
Compare
| * The query document used for the last `maskResult` call, used to | ||
| * invalidate the cache when the query changes via reobserve. | ||
| */ | ||
| private lastMaskedQuery: DocumentNode | undefined = undefined; |
There was a problem hiding this comment.
Not all codepaths going through maskResult actually are stored as a property on this, so storing these results as a property would probably continue flip-flopping object identities in some edge cases (e.g. if you were to fetchMore and await the result, that could destroy object identity).
It feels to me like the re-calculation can't easily be avoided and the logic for ensuring a stable object identity should probably live a the end of the operator filterMap, comparing the calculation result with previous.result, and keeping the previous value if they are identical.
There was a problem hiding this comment.
Thanks for the feedback! I've reworked the approach as suggested — removed the instance-level caching from maskResult entirely and moved the referential equality preservation into the operator's filterMap stage, comparing the newly masked result against previous.result.data.
The fix is now just:
if (
previous.result.data !== undefined &&
result.data !== previous.result.data &&
equal(result.data, previous.result.data)
) {
result = { ...result, data: previous.result.data };
}This avoids the flip-flopping issue since all emissions flow through the operator pipeline, and we always compare against the actual previous emitted value rather than a separately cached one.
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
from
3f0068e to
ca90a5c
Compare
When `dataMasking: true` is configured, `maskOperation` always creates new object references for masked data (since fragment fields are stripped). This breaks referential equality on refetch with identical results, causing unnecessary useEffect callbacks and re-renders. Memoize the masked output in `ObservableQuery.maskResult` so that if the input data reference is unchanged or the masked result is deeply equal to the previous one, the previous masked reference is reused. Fixes apollographql#13181 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
audrius-savickas
force-pushed
the
fix/masked-data-referential-equality
branch
from
ca90a5c to
d4a0cf8
Compare
|
@phryneas can you re-review please? |
|
@phryneas ping, can we get a re-review? |
jerelmiller
left a comment
jerelmiller
left a comment
There was a problem hiding this comment.
Thanks so much for being patient! I've got a few requested changes around test structure and such. Could you take a look?
We also just released 4.2 so can you make sure to rebase this branch against the latest changes on main? I'd love to get this out in a patch.
| const mocks = [ | ||
| { | ||
| request: { query }, | ||
| result: { | ||
| data: { | ||
| currentUser: { | ||
| __typename: "User", | ||
| id: 1, | ||
| name: "Test User", | ||
| age: 30, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| request: { query }, | ||
| result: { | ||
| data: { | ||
| currentUser: { | ||
| __typename: "User", | ||
| id: 1, | ||
| name: "Test User", | ||
| age: 30, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Rather than repeating the mock, use maxUsageCount instead:
| const mocks = [ | |
| { | |
| request: { query }, | |
| result: { | |
| data: { | |
| currentUser: { | |
| __typename: "User", | |
| id: 1, | |
| name: "Test User", | |
| age: 30, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| request: { query }, | |
| result: { | |
| data: { | |
| currentUser: { | |
| __typename: "User", | |
| id: 1, | |
| name: "Test User", | |
| age: 30, | |
| }, | |
| }, | |
| }, | |
| }, | |
| ]; | |
| const mocks = [ | |
| { | |
| request: { query }, | |
| result: { | |
| data: { | |
| currentUser: { | |
| __typename: "User", | |
| id: 1, | |
| name: "Test User", | |
| age: 30, | |
| }, | |
| }, | |
| }, | |
| maxUsageCount: 2, | |
| }, | |
| ]; |
| await renderStream.render(<App />, { | ||
| wrapper: ({ children }) => ( | ||
| <ApolloProvider client={client}>{children}</ApolloProvider> | ||
| ), | ||
| }); |
There was a problem hiding this comment.
Since this test doesn't test anything but the hook result, prefer using renderHookToSnapshotStream instead which returns the hook result directly instead of a snapshot key:
const { takeSnapshot } = await renderHookToSnapshotStream(
() => useQuery(query),
{ wrapper: createClientWrapper(client) }
); Then you can just call takeSnapshot to get the result:
const snapshot = await takeSnapshot();
expect(snapshot) // ....Probably our fault on other tests since they use renderStream.render 🤣. Some of these tests pre-date the renderHookToSnapshotStream API and we haven't gone back to update them yet.
| expect(initialSnapshot.data).toStrictEqual({ | ||
| currentUser: { | ||
| __typename: "User", | ||
| id: 1, | ||
| name: "Test User", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
A couple suggestions here:
- We prefer checking against the full result type everywhere, not just
data, that way we can catch regressions if we have too many or too few rerenders. - Prefer using
toStrictEqualTypedwhich catches regressions in the types via TypeScript and ensures we can mass update tests if/when we need to make changes.
| expect(initialSnapshot.data).toStrictEqual({ | |
| currentUser: { | |
| __typename: "User", | |
| id: 1, | |
| name: "Test User", | |
| }, | |
| }); | |
| expect(initialSnapshot).toStrictEqualTyped({ | |
| data: { | |
| currentUser: { | |
| __typename: "User", | |
| id: 1, | |
| name: "Test User", | |
| }, | |
| }, | |
| dataState: "complete", | |
| loading: false, | |
| networkStatus: NetworkStatus.ready, | |
| previousData: undefined, | |
| variables: {}, | |
| }); |
| }, | ||
| }); | ||
|
|
||
| // Trigger refetch with identical result |
There was a problem hiding this comment.
| // Trigger refetch with identical result |
No need to add these kinds of comments (looking at you Claude 🤣). The test description and mocks makes it clear what we are testing.
| let refetchSnapshot: useQuery.Result<Query, Record<string, never>>; | ||
| do { | ||
| ({ snapshot: refetchSnapshot } = await renderStream.takeRender()); | ||
| } while (refetchSnapshot.networkStatus !== NetworkStatus.ready); |
There was a problem hiding this comment.
While it may seem repetitive, we'd prefer checking snapshot by snapshot. We want to maintain an exact number of renders. This is especially important with the change for referential equality to make sure we didn't add or remove a render by accident!
Instead check each intermediate snapshot.
Note: We have a toRerenderWithSimilarSnapshot which reduces some of the boilerplate. If you use it, you'll need to const renderStream = await renderHookToSnapshotStream instead since it takes the full renderStream object:
await expect(renderStream).toRerenderWithSimilarSnapshot({
expected: (previous) => ({
...previous,
loading: true,
networkStatus: NetworkStatus.refetch,
})
});
await expect(renderStream).toRerenderWithSimilarSnapshot({
expected: (previous) => ({
...previous,
loading: false,
networkStatus: NetworkStatus.ready,
})
});Then you can check the current snapshot using the renderStream.getCurrentSnapshot function:
expect(getCurrentSnapshot().data).toBe(initialSnapshot.data);The render-by-render snapshot approach just ensures that we didn't introduce a regression for this case by adding a new render by accident.
| }); | ||
|
|
||
| // https://github.com/apollographql/apollo-client/issues/13181 | ||
| it("preserves referential equality of masked array data on refetch with identical results", async () => { |
There was a problem hiding this comment.
Take a look at this test and apply the same changes here as well.
| "@apollo/client": patch | ||
| --- | ||
|
|
||
| fix(masking): preserve referential equality of masked data on refetch with identical results |
There was a problem hiding this comment.
| fix(masking): preserve referential equality of masked data on refetch with identical results | |
| Preserve referential equality of masked data on refetch when the result is deeply equal to the previous result. |
No need to add the conventional commit style in this message. This will show up as a "patch change" in the release notes.
|
|
||
| await expect(renderStream).not.toRerender(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We should add an additional test that checks the behavior of a refetch when a masked field changes.
If we use the the first test as the example, this would mean refetch should return an updated age. i.e.
const mocks = [
{
request: { query },
result: {
data: {
currentUser: {
__typename: "User",
id: 1,
name: "Test User",
age: 30,
},
},
},
},
{
request: { query },
result: {
data: {
currentUser: {
__typename: "User",
id: 1,
name: "Test User",
age: 31,
},
},
},
},
];Since age is a masked field, I would expect that the data property should be referentially equal to the previous result as well.
| result.data !== previous.result.data && | ||
| equal(result.data, previous.result.data) | ||
| ) { | ||
| (result as { data: unknown }).data = previous.result.data; |
There was a problem hiding this comment.
| (result as { data: unknown }).data = previous.result.data; | |
| result.data = previous.result.data; |
No need for the type cast here. result and previous.result are the same type.
| // result is deeply equal to the previous one. This prevents | ||
| // unnecessary re-renders when refetching returns identical data. |
There was a problem hiding this comment.
| // result is deeply equal to the previous one. This prevents | |
| // unnecessary re-renders when refetching returns identical data. | |
| // result is deeply equal to the previous one. This prevents | |
| // React hooks like `useMemo` or `useEffect` from firing | |
| // unnecessarily. |
This fix isn't so much about preventing re-renders (you have the same number of renders as before), but rather its ensuring that we maintain referential equality so that hooks that rely on referential equality don't fire unnecessarily.
I'd also avoid the "identical data" label here since this should be true even when the full result is not identical (e.g. the age example in the tests that I mentioned)
5 participants
Summary
dataMasking: trueis configured,maskOperationalways creates new object references for masked data (fragment fields are stripped, causing key count mismatch). This breaks referential equality on refetch with identical results, triggering unnecessaryuseEffectcallbacks and re-renders.ObservableQuery.maskResultso that if the input data reference is unchanged or the masked result is deeply equal to the previous one, the previous masked reference is reused — matching how the cache itself preserves referential stability viaoptimism.Fixes #13181
Test plan
maskOperationtests pass (114 tests)ObservableQuerytests pass (272 tests)useQuerytests pass (586 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores