Skip to content

Commit ade3726

Browse files
maxjayclaude
andauthored
fix: restore() on Remove op uses insertAt not add() (sparse array bug) (#37)
* fix: restore() on Remove op in keyed array uses insertAt not add() When restoring a removed keyed array element, the base-time positional path may point to an index beyond the current draft array length. Calling add() fell through to upsertAt() -> setAt(), assigning directly to the index and creating a sparse array with undefined holes that crashed diff(). Fix: call insertAt() directly (splice semantics), matching the impl spec. Undo is removeAt() at the same segments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: restore() on Remove op uses insertAt not add() (v0.18.0) When restoring a removed keyed array element, the base-time positional path may point to an index beyond the current draft array length. Calling add() fell through to upsertAt() -> setAt(), assigning directly to the index and creating a sparse array with undefined holes that crashed diff(). Fix: call insertAt() directly (splice semantics), matching the impl spec. Undo is removeAt() at the same segments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d499706 commit ade3726

4 files changed

Lines changed: 77 additions & 3 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,3 +3,4 @@ dist/
33
.future/
44
.env
55
site/.vitepress/cache
6+
demo/

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@maxjay/patchwork",
3-
"version": "0.17.2",
3+
"version": "0.18.0",
44
"description": "A JSON editing engine with base/draft, diff, undo, and scoped lenses.",
55
"type": "module",
66
"main": "dist/index.js",

src/engine.restore.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { Engine, OpType } from './engine';
3+
4+
const INITIAL = {
5+
regions: [
6+
{
7+
code: 'EU',
8+
name: 'Europe',
9+
districts: [
10+
{ id: 'eu-north', label: 'Northern Europe', population: 105 },
11+
{ id: 'eu-west', label: 'Western Europe', population: 198 },
12+
{ id: 'eu-east', label: 'Eastern Europe', population: 292 },
13+
{ id: 'eu-south', label: 'Southern Europe', population: 151 },
14+
{ id: 'eu-cent', label: 'Central Europe', population: 134 },
15+
],
16+
},
17+
],
18+
};
19+
20+
const SCHEMA = {
21+
properties: {
22+
regions: {
23+
'x-key': 'code',
24+
items: {
25+
properties: {
26+
districts: {
27+
'x-key': 'id',
28+
items: { properties: {} },
29+
},
30+
},
31+
},
32+
},
33+
},
34+
};
35+
36+
describe('restore removed item in keyed array after multiple deletes', () => {
37+
it('restores Southern Europe after Western, Eastern, Southern, Central all deleted', () => {
38+
const e = new Engine(INITIAL, { schema: SCHEMA });
39+
40+
// delete Western, Eastern, Southern, Central (always delete index 1 as items shift down)
41+
e.delete("$['regions'][0]['districts'][1]");
42+
e.delete("$['regions'][0]['districts'][1]");
43+
e.delete("$['regions'][0]['districts'][1]");
44+
e.delete("$['regions'][0]['districts'][1]");
45+
46+
// draft is now [Northern Europe] only
47+
expect((e.draft as any).regions[0].districts).toHaveLength(1);
48+
49+
// find the Remove op for Southern Europe — nested in the Replace op's changes for EU
50+
const ops = e.diff();
51+
const euOp = ops.find(op => op.op === OpType.Replace && (op as any).identity === 'EU');
52+
const southernOp = (euOp as any)?.changes?.find((op: any) => op.op === OpType.Remove && op.identity === 'eu-south');
53+
expect(southernOp).toBeDefined();
54+
55+
// restore it — should splice in, not setAt index 3 on a 1-item array
56+
e.restore(southernOp!);
57+
58+
const districts = (e.draft as any).regions[0].districts;
59+
60+
// no sparse holes — Southern Europe appended after Northern Europe
61+
expect(districts).toHaveLength(2);
62+
expect(districts[0].id).toBe('eu-north');
63+
expect(districts[1].id).toBe('eu-south');
64+
65+
// diff must not throw
66+
expect(() => e.diff()).not.toThrow();
67+
});
68+
});

src/engine.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -595,9 +595,14 @@ export class Engine<T extends JsonValue = JsonValue> {
595595
case OpType.Add:
596596
this.delete(op.path);
597597
break;
598-
case OpType.Remove:
599-
this.add(op.path, op.value!);
598+
case OpType.Remove: {
599+
const segments = this.segmentsFrom(op.path);
600+
const doRestore = () => this.insertAt(segments, structuredClone(op.value!));
601+
const undoRestore = () => this.removeAt(segments);
602+
doRestore();
603+
this.pushOperation({ undo: undoRestore, redo: doRestore });
600604
break;
605+
}
601606
case OpType.Replace:
602607
this.replace(op.path, op.oldValue!);
603608
break;

0 commit comments

Comments
 (0)