Order index-based array removes to preserve correct application; add test and bump version#407
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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: d5de1f4525
ℹ️ 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".
| const removes = changes.filter((c) => c.type === Operation.REMOVE).sort((a, b) => Number(b.key) - Number(a.key)); | ||
| const rest = changes.filter((c) => c.type !== Operation.REMOVE); | ||
| return [...rest, ...removes]; |
There was a problem hiding this comment.
For $index arrays, this reordering moves every REMOVE behind non-remove operations, but array child changes can include adjacent REMOVE+ADD at the same index for type changes (e.g. old [1] to new [[2]]). In walkChanges, those child-level pairs are not merged before ordering, so the new order becomes ADD then REMOVE, which applies as insert-then-delete and produces the wrong final array when applyAtom runs sequentially. Please keep same-index remove/add replacement pairs intact (or merge them before reordering) and only reorder independent tail removes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect application of multiple index-based array removals when arrays are diffed by index (embeddedKey === '$index') by reordering emitted JSON Atom operations so removals are applied in descending index order, and adds a regression test for issue #404. It also bumps the package alpha version.
Changes:
- Add
orderArrayChildChangesand use it inwalkChangesto reorder$indexarray child changes (descending removes). - Add a Jest unit test asserting correct operation ordering and successful
applyAtomround-trip for multiple removes (#404). - Bump version
5.0.0-alpha.8→5.0.0-alpha.9inpackage.json/package-lock.json.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/jsonAtom.ts |
Reorders $index array child changes before emitting atom operations to avoid index-shift issues during sequential apply. |
tests/jsonAtom.test.ts |
Adds regression test ensuring multiple index-based removes are emitted/apply correctly (#404). |
package.json |
Version bump to 5.0.0-alpha.9. |
package-lock.json |
Lockfile version bump to match package.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const removes = changes.filter((c) => c.type === Operation.REMOVE).sort((a, b) => Number(b.key) - Number(a.key)); | ||
| const rest = changes.filter((c) => c.type !== Operation.REMOVE); | ||
| return [...rest, ...removes]; |
There was a problem hiding this comment.
For embeddedKey === '$index', this reorders all REMOVE child changes after non-REMOVE changes. That breaks REMOVE+ADD pairs emitted by jsonDiff when treatTypeChangeAsReplace is true (type changes are represented as adjacent REMOVE then ADD at the same index). After this reorder, the ADD will run before the corresponding REMOVE, so applying the atom will delete the newly added value and leave the old value (e.g., old [1] → new ['x'] results in add[0] then remove[0]). Fix by preserving/merging same-index REMOVE+ADD pairs before ordering (e.g., convert them to a single replace/update op), and only reorder “pure” index deletions.
| const removes = changes.filter((c) => c.type === Operation.REMOVE).sort((a, b) => Number(b.key) - Number(a.key)); | |
| const rest = changes.filter((c) => c.type !== Operation.REMOVE); | |
| return [...rest, ...removes]; | |
| type OrderedGroup = | |
| | { kind: 'pure-remove' } | |
| | { kind: 'preserved'; changes: IChange[] }; | |
| const groups: OrderedGroup[] = []; | |
| const pureRemoves: IChange[] = []; | |
| for (let i = 0; i < changes.length; i++) { | |
| const current = changes[i]; | |
| const next = changes[i + 1]; | |
| if ( | |
| current.type === Operation.REMOVE && | |
| next && | |
| next.type === Operation.ADD && | |
| String(current.key) === String(next.key) | |
| ) { | |
| groups.push({ kind: 'preserved', changes: [current, next] }); | |
| i++; | |
| continue; | |
| } | |
| if (current.type === Operation.REMOVE) { | |
| pureRemoves.push(current); | |
| groups.push({ kind: 'pure-remove' }); | |
| continue; | |
| } | |
| groups.push({ kind: 'preserved', changes: [current] }); | |
| } | |
| pureRemoves.sort((a, b) => Number(b.key) - Number(a.key)); | |
| const ordered: IChange[] = []; | |
| let removeIndex = 0; | |
| for (const group of groups) { | |
| if (group.kind === 'pure-remove') { | |
| ordered.push(pureRemoves[removeIndex++]); | |
| } else { | |
| ordered.push(...group.changes); | |
| } | |
| } | |
| return ordered; |
| const applied = applyAtom(structuredClone(oldObj), atom); | ||
| expect(applied).toEqual(newObj); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test covers multiple removes (#404), but it doesn’t cover the new ordering behavior’s interaction with type-change pairs inside $index arrays (which diff() represents as REMOVE+ADD at the same index when treatTypeChangeAsReplace is enabled). Consider adding a regression test where an element’s type changes at a given index (e.g. [1]→['x']) to ensure diffAtom/applyAtom still produces the correct final array.
| it('applies same-index type changes correctly in $index arrays', () => { | |
| const oldObj = { items: [1] }; | |
| const newObj = { items: ['x'] }; | |
| const atom = diffAtom(oldObj, newObj); | |
| expect(atom.operations.some((op) => op.path === '$.items[0]')).toBe(true); | |
| const applied = applyAtom(structuredClone(oldObj), atom); | |
| expect(applied).toEqual(newObj); | |
| }); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31178d2dab
ℹ️ 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".
| removes.sort((a, b) => Number(b.key) - Number(a.key)); | ||
|
|
||
| const ordered = [...changes]; | ||
| for (let i = 0; i < removeSlots.length; i++) { | ||
| ordered[removeSlots[i]] = removes[i]; |
There was a problem hiding this comment.
Sorting every $index REMOVE by numeric key and writing them back into the original remove slots can separate a same-index REMOVE+ADD pair that represents a type change, so the emitted atom no longer applies correctly. For example, diffing {a:[1,2]} to {a:[[1],[2]]} now emits remove[1], add[0], remove[0], add[1], and applyAtom produces {a:[1,[2]]} instead of the target. This happens whenever there are at least two removes (e.g., a type-change pair plus another removal), so the reordering should avoid moving paired replacement removes across other operations.
Useful? React with 👍 / 👎.
|
Motivation
$index), addressing cases where removes must be applied in descending index order (applyAtom logic mismatch: Array deletions without arrayIdentityKeys lead to incorrect final state #404).Description
orderArrayChildChangeswhich, forembeddedKey === '$index', movesremoveoperations to the end and sorts them in descending numeric index order so later removes don't shift earlier targets.orderArrayChildChangesinsidewalkChangesbefore emitting child array operations.applies multiple index-based removes correctly without identity keys (#404)intests/jsonAtom.test.tsthat asserts the emitted operations and that applying the atom yields the expected object.5.0.0-alpha.8to5.0.0-alpha.9inpackage.jsonandpackage-lock.json.Testing
npm test(Jest) and the suite passed, including the new index-removal test.applyAtomproduces the expected final object when the atom is applied, and it succeeded.Codex Task