Add tests for incremental payloads with custom scalars - #13319
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: c95e9f8f66e97c603022292c ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
📝 WalkthroughWalkthroughChangesCustom scalar behavior
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
commit: |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/core/__tests__/client.query/customScalars.test.ts (1)
568-639: 🎯 Functional Correctness | 🔵 TrivialUnresolved TODO baked into assertion.
The test asserts
startDateis parsed insideCombinedGraphQLErrors.datawith a// TODO: Determine if this is correctcomment. This pattern (serialized innone/ignorepolicies vs. parsed here) repeats identically acrossclient.mutate,client.subscribe, andcreateQueryPreloadertests. Worth resolving whether error.data should stay serialized (matching thenonepolicy test) or intentionally diverge forall.Want me to open a tracking issue for this?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/__tests__/client.query/customScalars.test.ts` around lines 568 - 639, The test in customScalars.test.ts contains an unresolved TODO inside the CombinedGraphQLErrors.data assertion for client.query with errorPolicy "all". Remove the TODO and make the expectation consistent with the intended behavior for scalar handling in error data: either assert the serialized value to match the "none"/"ignore" cases, or keep the parsed Date only if that is the deliberate contract for errorPolicy "all". Apply the same decision consistently across the related client.mutate, client.subscribe, and createQueryPreloader tests so the behavior is uniform.src/core/QueryInfo.ts (1)
329-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect fix, matches diff.result nullability contract.
The broadened condition correctly writes back the in-flight incremental diff so parsed scalars propagate to intermediate
@deferchunks. Sincediff.resultis typed asDeepPartial<T> | null(neverundefined) when!diff.complete, the!== nullcheck is safe.One minor nit:
this.incremental?.hasNextduplicates the existinghasNextgetter (Line 182-184) which already encapsulates this exact check. Usingthis.hasNexthere would be slightly more consistent with usage elsewhere in the file (e.g., Lines 405, 513, 526).♻️ Optional consistency tweak
if ( diff.complete || // Use the diff result if the query is still an in-flight // incremental query so that parsed custom scalar values // in diff.result are returned. - (this.incremental?.hasNext && diff.result !== null) + (this.hasNext && diff.result !== null) ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/QueryInfo.ts` around lines 329 - 337, The incremental diff write-back in QueryInfo should use the existing hasNext getter instead of duplicating its logic with this.incremental?.hasNext. Update the condition in the result merge block so it checks this.hasNext together with diff.result !== null, keeping the current nullability-safe behavior while matching the rest of QueryInfo’s usage and reducing duplicated state checks.src/core/__tests__/client.watchQuery/customScalars.test.ts (1)
1105-1193: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnresolved TODO questions correctness of an assertion.
// TODO: Determine if this is correct(line 1173-1174) flags uncertainty about whether the parsedstartDateshould appear insideCombinedGraphQLErrors.dataforerrorPolicy: "all". This same TODO is duplicated across several other new test files in this PR (useQuery, useLazyQuery, useMutation, useBackgroundQuery, useSuspenseQuery, useLoadableQuery). Worth resolving before merge since it signals the author isn't fully confident in the asserted behavior.Want me to help investigate the expected error-data shape for
errorPolicy: "all"and propose a fix/clarifying comment?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/__tests__/client.watchQuery/customScalars.test.ts` around lines 1105 - 1193, The test assertion in ObservableStream for `client.watchQuery` has an unresolved TODO about the shape of `CombinedGraphQLErrors.data` under `errorPolicy: "all"`. Confirm the expected error payload for parsed scalars in the `watchQuery` path and then either keep the parsed `startDate` in the asserted error data or adjust the assertion to match the actual behavior, removing the TODO; also apply the same fix pattern to the matching new tests in the other query/mutation hooks if they share this uncertainty.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/__tests__/client.query/customScalars.test.ts`:
- Around line 568-639: The test in customScalars.test.ts contains an unresolved
TODO inside the CombinedGraphQLErrors.data assertion for client.query with
errorPolicy "all". Remove the TODO and make the expectation consistent with the
intended behavior for scalar handling in error data: either assert the
serialized value to match the "none"/"ignore" cases, or keep the parsed Date
only if that is the deliberate contract for errorPolicy "all". Apply the same
decision consistently across the related client.mutate, client.subscribe, and
createQueryPreloader tests so the behavior is uniform.
In `@src/core/__tests__/client.watchQuery/customScalars.test.ts`:
- Around line 1105-1193: The test assertion in ObservableStream for
`client.watchQuery` has an unresolved TODO about the shape of
`CombinedGraphQLErrors.data` under `errorPolicy: "all"`. Confirm the expected
error payload for parsed scalars in the `watchQuery` path and then either keep
the parsed `startDate` in the asserted error data or adjust the assertion to
match the actual behavior, removing the TODO; also apply the same fix pattern to
the matching new tests in the other query/mutation hooks if they share this
uncertainty.
In `@src/core/QueryInfo.ts`:
- Around line 329-337: The incremental diff write-back in QueryInfo should use
the existing hasNext getter instead of duplicating its logic with
this.incremental?.hasNext. Update the condition in the result merge block so it
checks this.hasNext together with diff.result !== null, keeping the current
nullability-safe behavior while matching the rest of QueryInfo’s usage and
reducing duplicated state checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 17cce7bb-f976-4338-9293-6d15981fdbdd
📒 Files selected for processing (15)
.changeset/nasty-cheetahs-confess.mdsrc/core/QueryInfo.tssrc/core/__tests__/client.mutate/customScalars.test.tssrc/core/__tests__/client.query/customScalars.test.tssrc/core/__tests__/client.readQuery/customScalars.test.tssrc/core/__tests__/client.subscribe/customScalars.test.tssrc/core/__tests__/client.watchQuery/customScalars.test.tssrc/react/hooks/__tests__/useBackgroundQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useLazyQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useMutation/customScalars.test.tsxsrc/react/hooks/__tests__/useQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useQueryRefHandlers/customScalars.test.tsxsrc/react/hooks/__tests__/useSuspenseQuery/customScalars.test.tsxsrc/react/query-preloader/__tests__/createQueryPreloader/customScalars.test.tsx
There was a problem hiding this comment.
This file has a larger diff because I created a render helper and updated the existing tests to use it. Sorry for the extra noise!
| event: { | ||
| __typename: "Event", | ||
| id: "1", | ||
| startDate: "2026-01-01", |
There was a problem hiding this comment.
We'll need to flush out what we want this value to be. In this test, the value is the raw network value because the value was never written to the cache (because of the error policy), so the value is never parsed into a Date object. In the errorPolicy: "all" test below, the startDate is the parsed Date object in this error object because markQueryResult in QueryInfo writes the value to the cache and re-read with the parsed value before the errorPolicy is handled in QueryManager.
The benefit to doing it that way is that CombinedGraphQLErrors sets data as the combined result for incremental payloads rather than just the current chunk, however it does mean the inconsistency in the data value. Is this ok?
|
Moving to draft because I need some further discussion on the failing tests. |
|
Much of this should be fixed with #13324. Once that is merged, I'll put this back up for review which should be mostly tests at that point. |
32599de to
0ecf5e1
Compare
@defer payloads|
@coderabbitai rereview |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx (1)
541-558: 🩺 Stability & Availability | 🔵 TrivialResolve the
TODOon error-data scalar serialization before un-drafting.Under
errorPolicy: "all"theCombinedGraphQLErrorsdata expectsstartDateas a parsedDate, whereas theerrorPolicy: "none"test (Line 443) expects the raw string"2026-01-01"for the same field. This asymmetry is exactly what the// TODO: Determine if this is correctcomment questions and should be pinned down so the test encodes intended behavior rather than incidental behavior.Want me to open an issue to track deciding whether error-embedded scalars should be parsed or left serialized?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx` around lines 541 - 558, Resolve the TODO in the error-policy test by establishing the intended scalar representation for error-embedded data and making the `errorPolicy: "all"` and `errorPolicy: "none"` assertions consistent. Update the test data and expectations around `CombinedGraphQLErrors` and the corresponding `startDate` case so the chosen parsed-`Date` or raw-string behavior is explicitly pinned down.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsx`:
- Around line 541-558: Resolve the TODO in the error-policy test by establishing
the intended scalar representation for error-embedded data and making the
`errorPolicy: "all"` and `errorPolicy: "none"` assertions consistent. Update the
test data and expectations around `CombinedGraphQLErrors` and the corresponding
`startDate` case so the chosen parsed-`Date` or raw-string behavior is
explicitly pinned down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: da00ade5-2c3c-4cda-9969-6cecea52b939
📒 Files selected for processing (17)
config/jest.config.tssrc/core/__tests__/client.mutate/customScalars.test.tssrc/core/__tests__/client.query/customScalars.test.tssrc/core/__tests__/client.readQuery/customScalars.test.tssrc/core/__tests__/client.subscribe/customScalars.test.tssrc/core/__tests__/client.watchQuery/customScalars.test.tssrc/react/hooks/__tests__/useBackgroundQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useBackgroundQuery/testUtils.tsxsrc/react/hooks/__tests__/useLazyQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useLoadableQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useLoadableQuery/testUtils.tsxsrc/react/hooks/__tests__/useMutation/customScalars.test.tsxsrc/react/hooks/__tests__/useQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useQueryRefHandlers/customScalars.test.tsxsrc/react/hooks/__tests__/useSuspenseQuery/customScalars.test.tsxsrc/react/hooks/__tests__/useSuspenseQuery/testUtils.tsxsrc/react/query-preloader/__tests__/createQueryPreloader/customScalars.test.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- src/core/tests/client.subscribe/customScalars.test.ts
- src/react/query-preloader/tests/createQueryPreloader/customScalars.test.tsx
- src/react/hooks/tests/useLazyQuery/customScalars.test.tsx
- src/react/hooks/tests/useQuery/customScalars.test.tsx
- src/react/hooks/tests/useBackgroundQuery/customScalars.test.tsx
- src/core/tests/client.query/customScalars.test.ts
- src/react/hooks/tests/useSuspenseQuery/customScalars.test.tsx
- src/core/tests/client.mutate/customScalars.test.ts
- src/react/hooks/tests/useMutation/customScalars.test.tsx
- src/react/hooks/tests/useQueryRefHandlers/customScalars.test.tsx
- src/core/tests/client.watchQuery/customScalars.test.ts
|
As @coderabbitai notes, we haven't resolved the |
|
Tip For best results, initiate chat on the files or code changes.
(・ω・)ノ |
Adds tests to ensure scalar values are applied to incremental payloads (both
@deferand@stream).Summary by CodeRabbit
Bug Fixes
Tests