fix: movable list diff issue

Author: zxch3n

packages/core/src/core/diff.ts

@@ -321,18 +321,22 @@ export function diffMovableList<S extends ArrayLike>(
 
     for (const [newIndex, item] of newState.entries()) {
         const id = idSelector(item);
-        if (id) {
-            newMap.set(id, { index: newIndex, item });
-            if (oldMap.has(id)) {
-                const { index: oldIndex, item: oldItem } = oldMap.get(id)!;
-                commonItems.push({
-                    id,
-                    oldIndex,
-                    newIndex,
-                    oldItem,
-                    newItem: item,
-                });
-            }
+        if (!id) {
+            throw new Error("Item ID cannot be null");
+        }
+        if (newMap.has(id)) {
+            throw new Error("Duplicate item id in new state");
+        }
+        newMap.set(id, { index: newIndex, item });
+        if (oldMap.has(id)) {
+            const { index: oldIndex, item: oldItem } = oldMap.get(id)!;
+            commonItems.push({
+                id,
+                oldIndex,
+                newIndex,
+                oldItem,
+                newItem: item,
+            });
         }
     }
 
@@ -355,27 +359,51 @@ export function diffMovableList<S extends ArrayLike>(
     changes.push(...deletionOps);
 
     // Handle moves
-    const oldIndicesSequence = commonItems.map((info) => info.oldIndex);
-
-    /** LIS of the old indices that are in the common items
-     * This is represented as the core set of indexes which remain the same
-     * betweeen both old and new states.
-     * All move operations should only be performed on items that are not in the LIS.
-     * By excluding items in the LIS, we ensure that we don't perform unnecessary move operations.
-     */
-    const lisIndices = longestIncreasingSubsequence(oldIndicesSequence);
-    const lisSet = new Set<number>(lisIndices);
-    for (const [i, info] of commonItems.entries()) {
-        // If the common item is not in the LIS and its positions differ, mark it for move
-        if (!lisSet.has(i) && info.oldIndex !== info.newIndex) {
-            changes.push({
-                container: containerId,
-                key: info.oldIndex,
-                value: info.newItem,
-                kind: "move",
-                fromIndex: info.oldIndex,
-                toIndex: info.newIndex,
-            });
+    // After deletions are applied, indices shift left for items after a deleted index.
+    // Compute move operations relative to the post-deletion list to avoid index mismatch.
+    // Build the order of common item IDs in old and new states.
+    const oldCommonIds: string[] = [];
+    for (const item of oldState) {
+        const id = idSelector(item);
+        if (id && newMap.has(id)) {
+            oldCommonIds.push(id);
+        }
+    }
+    const newCommonIds: string[] = commonItems.map((info) => info.id);
+
+    // Simulate moves on an array of IDs to compute correct from/to indices
+    const currentOrder = [...oldCommonIds];
+    const currentIndexMap = new Map<string, number>();
+    currentOrder.forEach((id, idx) => currentIndexMap.set(id, idx));
+
+    for (
+        let targetIndex = 0;
+        targetIndex < newCommonIds.length;
+        targetIndex++
+    ) {
+        const id = newCommonIds[targetIndex];
+        const currentIndex = currentIndexMap.get(id);
+        if (currentIndex === undefined) continue; // safety guard
+        if (currentIndex === targetIndex) continue; // already in place
+
+        changes.push({
+            container: containerId,
+            key: currentIndex,
+            value: undefined,
+            kind: "move",
+            fromIndex: currentIndex,
+            toIndex: targetIndex,
+        });
+
+        // Update the simulated order and index map after the move
+        const [moved] = currentOrder.splice(currentIndex, 1);
+        currentOrder.splice(targetIndex, 0, moved);
+
+        // Update indices for the affected range
+        const start = Math.min(currentIndex, targetIndex);
+        const end = Math.max(currentIndex, targetIndex);
+        for (let i = start; i <= end; i++) {
+            currentIndexMap.set(currentOrder[i], i);
         }
     }
 

packages/core/tests/core/mirror-movable-list.test.ts

@@ -12,7 +12,6 @@ const waitForSync = async () => {
 };
 
 describe("MovableList", () => {
-
     async function initTestMirror() {
         const doc = new LoroDoc();
         doc.setPeerId(1);
@@ -349,4 +348,235 @@ describe("MovableList", () => {
 
         expect(mirror.getState()).toEqual(desiredState);
     });
+
+    it("movable list handles basic moves", async () => {
+        const doc = new LoroDoc();
+        doc.setPeerId(1);
+        const schema_ = schema({
+            list: schema.LoroMovableList(
+                schema.LoroMap({
+                    id: schema.String(),
+                    text: schema.LoroText(),
+                }),
+                (item) => item.id,
+            ),
+        });
+
+        const mirror = new Mirror({
+            doc,
+            schema: schema_,
+        });
+
+        mirror.setState({
+            list: [
+                { id: "0", text: "" },
+                { id: "1", text: "" },
+                { id: "2", text: "" },
+                { id: "3", text: "" },
+            ],
+        });
+        expect(doc.frontiers()[0].counter).toBe(11);
+        mirror.setState({
+            list: [
+                { id: "1", text: "" },
+                { id: "0", text: "" },
+                { id: "2", text: "" },
+                { id: "3", text: "" },
+            ],
+        });
+        expect(doc.frontiers()[0].counter).toBe(12);
+        mirror.setState({
+            list: [
+                { id: "0", text: "" },
+                { id: "1", text: "" },
+                { id: "3", text: "" },
+                { id: "2", text: "" },
+            ],
+        });
+        expect(doc.frontiers()[0].counter).toBe(14);
+    });
+
+    it("movable list handles delete + reorder without index errors", async () => {
+        const { mirror, doc } = await initTestMirror();
+
+        // Set to four items first
+        mirror.setState({
+            list: [
+                { id: "A", text: "tA" },
+                { id: "B", text: "tB" },
+                { id: "C", text: "tC" },
+                { id: "D", text: "tD" },
+            ],
+        });
+        await waitForSync();
+
+        const initial = doc.getDeepValueWithID();
+        const idToCid = new Map(
+            initial.list.value.map((x: any) => [x.value.id, x.cid]),
+        );
+
+        const desired = {
+            list: [
+                { id: "D", text: "tD" },
+                { id: "C", text: "tC" },
+                { id: "B", text: "tB" },
+            ],
+        };
+        mirror.setState(desired);
+        await waitForSync();
+
+        const after = doc.getDeepValueWithID();
+        expect(after.list.value.map((x: any) => x.value.id)).toEqual([
+            "D",
+            "C",
+            "B",
+        ]);
+        // Container IDs preserved for remaining items
+        expect(after.list.value[0].cid).toBe(idToCid.get("D"));
+        expect(after.list.value[1].cid).toBe(idToCid.get("C"));
+        expect(after.list.value[2].cid).toBe(idToCid.get("B"));
+
+        // Ensure state mirrors correctly
+        expect(mirror.getState()).toEqual(desired);
+    });
+
+    it("movable list handles insert + delete + reorder mix", async () => {
+        const { mirror, doc } = await initTestMirror();
+
+        mirror.setState({
+            list: [
+                { id: "A", text: "tA" },
+                { id: "B", text: "tB" },
+                { id: "C", text: "tC" },
+            ],
+        });
+        await waitForSync();
+
+        const initial = doc.getDeepValueWithID();
+        const idToCid = new Map(
+            initial.list.value.map((x: any) => [x.value.id, x.cid]),
+        );
+
+        const desired = {
+            list: [
+                { id: "C", text: "tc" },
+                { id: "E", text: "te" },
+                { id: "B", text: "tb" },
+            ],
+        };
+        mirror.setState(desired);
+        await waitForSync();
+
+        const after = doc.getDeepValueWithID();
+        expect(after.list.value.map((x: any) => x.value.id)).toEqual([
+            "C",
+            "E",
+            "B",
+        ]);
+        // C and B preserve container ids; E is new
+        expect(after.list.value[0].cid).toBe(idToCid.get("C"));
+        expect(after.list.value[2].cid).toBe(idToCid.get("B"));
+        expect(after.list.value[1].cid).not.toBe(idToCid.get("A"));
+        expect(after.list.value[1].cid).not.toBe(idToCid.get("B"));
+        expect(after.list.value[1].cid).not.toBe(idToCid.get("C"));
+
+        // Texts updated accordingly
+        expect(after.list.value[0].value.text.value).toBe("tc");
+        expect(after.list.value[2].value.text.value).toBe("tb");
+        expect(after.list.value[1].value.text.value).toBe("te");
+
+        expect(mirror.getState()).toEqual(desired);
+    });
+
+    it("movable list throws on missing item id", async () => {
+        const { mirror } = await initTestMirror();
+        // @ts-expect-error testing runtime validation
+        expect(() =>
+            mirror.setState({
+                list: [
+                    // missing id
+                    { text: "no id" },
+                ],
+            }),
+        ).toThrow();
+    });
+
+    it("movable list throws on duplicate ids in new state", async () => {
+        const { mirror } = await initTestMirror();
+        expect(() =>
+            mirror.setState({
+                list: [
+                    { id: "X", text: "1" },
+                    { id: "X", text: "2" },
+                ],
+            }),
+        ).toThrow();
+    });
+
+    it("movable list fuzz: large shuffles preserve container ids and text", async () => {
+        const { mirror, doc } = await initTestMirror();
+
+        // Deterministic RNG
+        let seed = 0x12345678;
+        const rand = () => {
+            seed = (1664525 * seed + 1013904223) >>> 0;
+            return seed / 0x100000000;
+        };
+        const shuffle = <T>(arr: T[]): T[] => {
+            const a = arr.slice();
+            for (let i = a.length - 1; i > 0; i--) {
+                const j = Math.floor(rand() * (i + 1));
+                [a[i], a[j]] = [a[j], a[i]];
+            }
+            return a;
+        };
+
+        const N = 80;
+        const ROUNDS = 25;
+
+        const makeItem = (i: number) => ({
+            id: String(i + 1),
+            text: `T${i + 1}`,
+        });
+        let current = Array.from({ length: N }, (_, i) => makeItem(i));
+
+        mirror.setState({ list: current });
+        await waitForSync();
+
+        const initial = doc.getDeepValueWithID();
+        const initialCidById = new Map(
+            initial.list.value.map((x: any) => [x.value.id, x.cid]),
+        );
+
+        for (let r = 0; r < ROUNDS; r++) {
+            // Shuffle order
+            let next = shuffle(current);
+
+            // Randomly update a few items' text to exercise nested updates
+            const updates = Math.floor(rand() * 5);
+            for (let k = 0; k < updates; k++) {
+                const idx = Math.floor(rand() * next.length);
+                const id = next[idx].id;
+                next[idx] = { id, text: `T${id}-r${r}` } as any;
+            }
+
+            mirror.setState({ list: next });
+            await waitForSync();
+
+            const after = doc.getDeepValueWithID();
+            // Verify order and IDs
+            const ids = after.list.value.map((x: any) => x.value.id);
+            expect(ids).toEqual(next.map((x) => x.id));
+            // Verify container IDs preserved
+            after.list.value.forEach((x: any, i: number) => {
+                expect(x.cid).toBe(initialCidById.get(next[i].id));
+                expect(x.value.text.value).toBe(next[i].text);
+            });
+
+            // Mirror state reflects next
+            expect(mirror.getState()).toEqual({ list: next });
+
+            current = next;
+        }
+    });
 });