Minor followup for feedback from incremental cache implementation - #13360
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 2f45effadfa94f5305cf925d ✅ AI Style Review — No Changes DetectedNo MDX files were changed in this pull request. Review Log: View detailed log
|
|
commit: |
📝 WalkthroughWalkthroughApollo Client updates incremental diff contracts, derives incremental behavior from option presence in ChangesIncremental diff handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant QueryInfo
participant ApolloCache
participant InMemoryCache
participant StoreReader
QueryInfo->>ApolloCache: Pass incrementalInfo to diff
ApolloCache->>InMemoryCache: Execute incremental diff overload
InMemoryCache->>StoreReader: Read with incremental option
StoreReader-->>InMemoryCache: Return diff and dataState
InMemoryCache-->>ApolloCache: Return diff result
ApolloCache-->>QueryInfo: Return diff or data-state result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cache/inmemory/__tests__/cache.diff/incremental.test.ts (1)
54-9423: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a presence-versus-value regression case.
These replacements are correct only because StoreReader now uses own-property presence. Add paired coverage where the symbol is omitted versus present with
undefined; otherwise a future truthiness check or symbol loss during object spreading could make the existing tests pass while non-incremental reads regress.🤖 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/cache/inmemory/__tests__/cache.diff/incremental.test.ts` around lines 54 - 9423, Add paired regression tests around the incremental-read options used by cache.diff: verify behavior when handleIncrementalSymbol is omitted entirely versus explicitly present with an undefined value. Use the existing defer-related cache.diff tests and assertions as the model, ensuring both cases cover presence semantics and would fail if implementation reverted to truthiness checks or lost the symbol during object spreading.
🤖 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/cache/inmemory/__tests__/cache.diff/incremental.test.ts`:
- Around line 54-9423: Add paired regression tests around the incremental-read
options used by cache.diff: verify behavior when handleIncrementalSymbol is
omitted entirely versus explicitly present with an undefined value. Use the
existing defer-related cache.diff tests and assertions as the model, ensuring
both cases cover presence semantics and would fail if implementation reverted to
truthiness checks or lost the symbol during object spreading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f17b909c-89f5-4558-b61f-140baf2e9bab
📒 Files selected for processing (8)
.api-reports/api-report-cache.api.md.api-reports/api-report.api.mdsrc/cache/core/cache.tssrc/cache/inmemory/__tests__/cache.diff/incremental.test.tssrc/cache/inmemory/inMemoryCache.tssrc/cache/inmemory/readFromStore.tssrc/cache/inmemory/types.tssrc/core/QueryInfo.ts
💤 Files with no reviewable changes (1)
- src/cache/inmemory/types.ts
Followup to #13347
Discussed with @phryneas in person. Instead of conditionally passing in the symbol by having caches opt-in, we pass it unconditionally. Caches that want to take advantage of behavior using that symbol can come talk to us on how to implement against the incremental behavior.
Summary by CodeRabbit