Seanaye/feat/optimistic links - #5502
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe GraphQL cache adds typed 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
56e123f to
719fcd4
Compare
e9a1f37 to
318d475
Compare
719fcd4 to
d913c1d
Compare
318d475 to
e9725b5
Compare
d913c1d to
7286798
Compare
7286798 to
54e30a8
Compare
![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
🧹 Nitpick comments (5)
apps/web/src/lib/graphql-cache/exchange/optimistic.ts (2)
42-47: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider excluding managed fields from
ScalarInsertFields.
ScalarInsertFields<T>permits every scalar key ofTItem, including the selector field and the count field. The Rust engine rejects such payloads at runtime withConflictingInsertField(seecrates/client/cache-core/src/link_patch.rslines 273-277). You can move that check to compile time by excluding the managed keys.The selector and count keys are generic parameters of
upsertEmbeddedLink, soScalarInsertFieldscan accept them:type ScalarInsertFields<T, TExclude extends keyof T = never> = Partial<{ [K in Exclude<ScalarKey<T>, TExclude>]: Exclude<T[K], undefined>; }>;Then use
ScalarInsertFields<TItem, TSelectorField | TCountField | TLinkField>in theupsertEmbeddedLinkargs.🤖 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 `@apps/web/src/lib/graphql-cache/exchange/optimistic.ts` around lines 42 - 47, Update ScalarInsertFields to accept an exclusion-key generic and omit those keys from its mapped scalar fields. In upsertEmbeddedLink, instantiate it with TSelectorField, TCountField, and TLinkField so managed fields cannot be supplied in insert payloads while other scalar fields remain supported.
275-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
as unknown as OptimisticUpdatein the two new builders.The sibling
updatebuilder at lines 242-251 uses a singleas OptimisticUpdatecast. The new builders use a double cast, which removes all structural checking againstOptimisticLinkPatchWire. A field name typo or a shape drift inprotocol.tswill then compile without error.Bind the payload to
OptimisticLinkPatchWirefirst, so the compiler validates the wire shape, and cast only the branded phantom property.♻️ Proposed change for `removeEmbeddedLink` (apply the same pattern to `upsertEmbeddedLink`)
- return { + const wire: OptimisticLinkPatchWire = { query: stringifyDocument(selection.document), operationName: documentOperationName(selection.document), variablesJson: JSON.stringify(selection.variables ?? {}), path: [...selection.path], operation: { kind: 'removeEmbeddedLink', listItem: args.listItem, linkField: args.linkField, countField: args.countField, entityKey: normalizedEntityKey(args.entity), }, - } as unknown as OptimisticUpdate; + }; + return wire as OptimisticUpdate;Note:
args.listItem.equalshas typePresent<TItem[TSelectorField]>, andinsertFieldshas typeScalarInsertFields<TItem>. If either does not satisfy the wire types, the assignment will fail. That failure is the signal that the wire type must widen, not that the cast must stay.🤖 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 `@apps/web/src/lib/graphql-cache/exchange/optimistic.ts` around lines 275 - 287, Update the new removeEmbeddedLink and upsertEmbeddedLink builders to construct their operation payload as an OptimisticLinkPatchWire value before returning it, so the compiler validates all wire fields and types. Replace each as unknown as OptimisticUpdate double cast with only the narrow cast needed for the branded phantom property, following the existing update builder pattern and preserving the current payload fields.crates/client/cache-core/src/link_patch.rs (2)
1010-1010: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTests belong in a separate
test.rsfile.The new embedded-link tests were added to the inline
mod testsblock in this file. The repository convention places tests in a separatetest.rsfile inside the same module directory, and the implementation file declares#[cfg(test)] mod test;.The inline block is pre-existing, so this is a migration rather than a fix for the new tests alone. Treat it as optional in this PR.
As per coding guidelines: "Place tests in a separate
test.rsfile within the same module directory; implementation files should declare the test submodule with#[cfg(test)] mod test;".🤖 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 `@crates/client/cache-core/src/link_patch.rs` at line 1010, Optionally migrate the existing inline `mod tests` in the link-patch implementation into a sibling `test.rs` module, moving all test code there and declaring it with `#[cfg(test)] mod test;` in the implementation file. Keep test behavior unchanged and include the new embedded-link tests in the external module.Source: Coding guidelines
886-895: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDetect duplicate storage keys when resolving insert fields.
resolved_insert_fieldscollects into aHashMapkeyed by the resolved storage key. Two distinct response keys can resolve to the same storage key when the query aliases the same field.collect::<Result<HashMap, _>>keeps the last entry and drops the earlier one silently.The surrounding code validates every other field conflict explicitly. Add the same rigor here.
♻️ Proposed change to reject duplicate resolved keys
- let resolved_insert_fields = insert_fields - .into_iter() - .flat_map(|fields| fields.iter()) - .map(|(field, value)| { - Ok(( - selected_storage_key(selected_field(selections, concrete, field)?, variables)?, - value.clone(), - )) - }) - .collect::<Result<HashMap<_, _>, LinkPatchError>>()?; + let mut resolved_insert_fields: HashMap<FieldKey, Json> = HashMap::new(); + for (field, value) in insert_fields.into_iter().flat_map(|fields| fields.iter()) { + let key = selected_storage_key(selected_field(selections, concrete, field)?, variables)?; + if resolved_insert_fields.insert(key.clone(), value.clone()).is_some() { + return Err(LinkPatchError::ConflictingInsertField(key)); + } + }🤖 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 `@crates/client/cache-core/src/link_patch.rs` around lines 886 - 895, Update the resolved insert-field collection in the surrounding link-patch function to detect duplicate storage keys instead of silently overwriting earlier values. When two response fields resolve to the same key, return the existing LinkPatchError conflict used by nearby field-validation logic; retain successful collection for unique keys.apps/web/src/lib/graphql-cache/exchange/optimistic.test.ts (1)
119-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative type assertion for
linkField.The block asserts only that a non-numeric field is rejected as
countField.NormalizedEntityListKeyis a new constraint and has no coverage. Add a case that passes a non-entity-list field, such as'key', aslinkField. That case locks the new constraint against regressions.🤖 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 `@apps/web/src/lib/graphql-cache/exchange/optimistic.test.ts` around lines 119 - 126, Add a separate negative type assertion in the optimistic test around upsertEmbeddedLink, passing the non-list field 'key' as linkField while keeping the existing invalid countField assertion. Ensure the added case uses the expected TypeScript error annotation to verify NormalizedEntityListKey rejects non-entity-list fields.
🤖 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.
Inline comments:
In `@crates/client/cache-core/src/link_patch.rs`:
- Around line 295-300: Add a distinct managed-field conflict error variant, such
as ConflictingManagedField { first: String, second: String }, and update
validate_embedded_link_fields at
crates/client/cache-core/src/link_patch.rs#L295-L300 to return it with the
actual colliding fields: where_field/link_field for the first check and the
matching pair for the second. Also update
crates/client/cache-core/src/link_patch.rs#L902-L907 to return the new variant
using the two resolved storage keys that collide instead of
resolved_count_field.
---
Nitpick comments:
In `@apps/web/src/lib/graphql-cache/exchange/optimistic.test.ts`:
- Around line 119-126: Add a separate negative type assertion in the optimistic
test around upsertEmbeddedLink, passing the non-list field 'key' as linkField
while keeping the existing invalid countField assertion. Ensure the added case
uses the expected TypeScript error annotation to verify NormalizedEntityListKey
rejects non-entity-list fields.
In `@apps/web/src/lib/graphql-cache/exchange/optimistic.ts`:
- Around line 42-47: Update ScalarInsertFields to accept an exclusion-key
generic and omit those keys from its mapped scalar fields. In
upsertEmbeddedLink, instantiate it with TSelectorField, TCountField, and
TLinkField so managed fields cannot be supplied in insert payloads while other
scalar fields remain supported.
- Around line 275-287: Update the new removeEmbeddedLink and upsertEmbeddedLink
builders to construct their operation payload as an OptimisticLinkPatchWire
value before returning it, so the compiler validates all wire fields and types.
Replace each as unknown as OptimisticUpdate double cast with only the narrow
cast needed for the branded phantom property, following the existing update
builder pattern and preserving the current payload fields.
In `@crates/client/cache-core/src/link_patch.rs`:
- Line 1010: Optionally migrate the existing inline `mod tests` in the
link-patch implementation into a sibling `test.rs` module, moving all test code
there and declaring it with `#[cfg(test)] mod test;` in the implementation file.
Keep test behavior unchanged and include the new embedded-link tests in the
external module.
- Around line 886-895: Update the resolved insert-field collection in the
surrounding link-patch function to detect duplicate storage keys instead of
silently overwriting earlier values. When two response fields resolve to the
same key, return the existing LinkPatchError conflict used by nearby
field-validation logic; retain successful collection for unique keys.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 617881b5-0816-4515-ab3c-ce8169ef04ea
📒 Files selected for processing (8)
apps/web/src/lib/graphql-cache/exchange/optimistic.test.tsapps/web/src/lib/graphql-cache/exchange/optimistic.tsapps/web/src/lib/graphql-cache/index.tsapps/web/src/lib/graphql-cache/protocol.tsapps/web/src/lib/queries/soup/grouped/graphql-optimistic.test.tsapps/web/src/lib/queries/soup/grouped/graphql-optimistic.tscrates/client/cache-core/src/link_patch.rscrates/client/cache-core/tests/grouped_optimistic.rs
| if list_item.where_field == *link_field { | ||
| return Err(LinkPatchError::ConflictingInsertField(link_field.clone())); | ||
| } | ||
| if list_item.where_field == *count_field || link_field == count_field { | ||
| return Err(LinkPatchError::ConflictingInsertField(count_field.clone())); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ConflictingInsertField reports the wrong conflict class and the wrong field. Both sites reuse one error variant for conflicts that do not involve an insert field. A caller that sets link_field equal to the selector receives "embedded insert field items conflicts with a managed field", which points at the wrong input. The second site also always names the count field, so a selector-versus-link conflict is reported as a count-field conflict. Add a distinct variant, such as ConflictingManagedField { first: String, second: String }, and name the two fields that actually collide.
crates/client/cache-core/src/link_patch.rs#L295-L300: return the new variant fromvalidate_embedded_link_fields, namingwhere_fieldandlink_fieldfor the first check and the colliding pair for the second check.crates/client/cache-core/src/link_patch.rs#L902-L907: return the new variant and name the two resolved storage keys that match, instead of always returningresolved_count_field.
📍 Affects 1 file
crates/client/cache-core/src/link_patch.rs#L295-L300(this comment)crates/client/cache-core/src/link_patch.rs#L902-L907
🤖 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 `@crates/client/cache-core/src/link_patch.rs` around lines 295 - 300, Add a
distinct managed-field conflict error variant, such as ConflictingManagedField {
first: String, second: String }, and update validate_embedded_link_fields at
crates/client/cache-core/src/link_patch.rs#L295-L300 to return it with the
actual colliding fields: where_field/link_field for the first check and the
matching pair for the second. Also update
crates/client/cache-core/src/link_patch.rs#L902-L907 to return the new variant
using the two resolved storage keys that collide instead of
resolved_count_field.
b500f20 to
ee57f1d
Compare
ee57f1d to
56a1740
Compare
1 participant
This PR adds a new cache core message which allows upserting links (edges) between entities for optimitic update purposes. This allows callers to correct optimitically add new links, changing the relationship between nodes before the server response returns.
Initially this is used in group soup to re-create bins which might no longer exist, then place objects into those bins