fix(CS-11713): fully replace array attributes in store.patch#5328
Open
lucaslyl wants to merge 1 commit into
Open
fix(CS-11713): fully replace array attributes in store.patch#5328lucaslyl wants to merge 1 commit into
lucaslyl wants to merge 1 commit into
Conversation
Motivation: a card app lets the AI assistant plan and mutate a
containsMany of itinerary stops. Asking it to reduce the stops got stuck —
the assistant replies that the stops have been reduced (e.g. "Day 2 trimmed
to a relaxed 4-stop flow"), but the containsMany field is never actually
patched (or only the first day shrank while later days kept stale stops).
After switching the merge logic to mergeWith, the same prompts now mutate
the stops correctly.
Root cause: StoreService.patch merged patch.attributes with lodash `merge`,
which merges arrays by index and never shortens the destination array. A
containsMany (or any array attribute) could grow or change in place via a
patch but never shrink — a shorter array left the old trailing items
behind. Because stops is one flat array (each carries a `day`), reducing
every day reliably trimmed the low indices (Day 1) but left the high-index
tail (Day 2+) untouched.
This only bit patch-based writes: the AI patchCardInstance tool and any
PatchCardInstanceCommand caller. A component-side `model.field = [...]`
assignment was never affected, because it replaces the whole field value.
Fix: merge attributes with mergeWith and a customizer that returns the
source array, so a patched array fully replaces the existing one —
matching PatchCardInstanceCommand's documented contract ("attributes
specified will be fully replaced; display the full array in the patch
code"). Non-array values still deep-merge (nested patches like
`cardInfo: { name }` keep their siblings); the relationships and meta
merge paths are unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Author
|
@codex comment |
Summary
Testing
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a758cc4b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+929
to
+933
| doc.data.attributes = mergeWith( | ||
| doc.data.attributes, | ||
| patch.attributes, | ||
| (_dest, src) => (Array.isArray(src) ? src : undefined), | ||
| ); |
There was a problem hiding this comment.
When the patched attribute is a shorter array for a field with per-index metadata (for example a polymorphic containsMany with data.meta.fields['items.1']), this replaces only data.attributes; the serialized doc still carries the old per-item metadata from serializeCard. updateFromSerialized can keep those stale field overrides on the live instance, so expanding the array again before a reload can write the removed element's type metadata back onto the new entry. Please clear or replace the matching doc.data.meta.fields entries when an array attribute is fully replaced.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
linear: https://github.com/cardstack/boxel/pulls
Motivation: I have created a travel planner app tjat lets the AI assistant plan and able tomutate a "containsMany" of itinerary stops. Asking it to reduce the stops got stuck — the assistant replies that the stops have been reduced (e.g. "Day 2 trimmed to a relaxed 4-stop flow"), but the containsMany field is never actually patched (or only the first day shrank while later days kept stale stops). After switching the merge logic to mergeWith, the same prompts now mutate the stops correctly.
Root cause: StoreService.patch merged patch.attributes with lodash
merge, which merges arrays by index and never shortens the destination array. A containsMany (or any array attribute) could grow or change in place via a patch but never shrink — a shorter array left the old trailing items behind. Because stops is one flat array (each carries aday), reducing every day reliably trimmed the low indices (Day 1) but left the high-index tail (Day 2+) untouched.This only bit patch-based writes: the AI patchCardInstance tool and any PatchCardInstanceCommand caller. A component-side
model.field = [...]assignment was never affected, because it replaces the whole field value.Fix: merge attributes with mergeWith and a customizer that returns the source array, so a patched array fully replaces the existing one — matching PatchCardInstanceCommand's documented contract ("attributes specified will be fully replaced; display the full array in the patch code"). Non-array values still deep-merge (nested patches like
cardInfo: { name }keep their siblings); the relationships and meta merge paths are unchanged.Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Before:
After:
