fix: even deltas with version & version nonce are valid (#9897)

packages/element/src/delta.ts

@@ -1111,16 +1111,16 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
     inserted,
   }: Delta<ElementPartial>) =>
     !!(
-      deleted.version &&
-      inserted.version &&
       // versions are required integers
+      (
         Number.isInteger(deleted.version) &&
         Number.isInteger(inserted.version) &&
         // versions should be positive, zero included
-      deleted.version >= 0 &&
+        deleted.version! >= 0 &&
-      inserted.version >= 0 &&
+        inserted.version! >= 0 &&
         // versions should never be the same
         deleted.version !== inserted.version
+      )
     );
 
   private static satisfiesUniqueInvariants = (
@@ -1191,9 +1191,10 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           ElementsDelta.stripIrrelevantProps,
         );
 
-        // ignore updates which would "delete" already deleted element
         if (!prevElement.isDeleted) {
           removed[prevElement.id] = delta;
+        } else {
+          updated[prevElement.id] = delta;
         }
       }
     }
@@ -1221,6 +1222,8 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
         // ignore updates which would "delete" already deleted element
         if (!nextElement.isDeleted) {
           added[nextElement.id] = delta;
+        } else {
+          updated[nextElement.id] = delta;
         }
 
         continue;
@@ -1250,17 +1253,9 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           continue;
         }
 
-        const strippedDeleted = ElementsDelta.stripVersionProps(delta.deleted);
-        const strippedInserted = ElementsDelta.stripVersionProps(
-          delta.inserted,
-        );
-
-        // making sure there are at least some changes and only changed version & versionNonce does not count!
-        if (Delta.isInnerDifferent(strippedDeleted, strippedInserted, true)) {
         updated[nextElement.id] = delta;
       }
     }
-    }
 
     return ElementsDelta.create(added, removed, updated);
   }
@@ -1372,15 +1367,8 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
           latestDelta = delta;
         }
 
-        const strippedDeleted = ElementsDelta.stripVersionProps(
-          latestDelta.deleted,
-        );
-        const strippedInserted = ElementsDelta.stripVersionProps(
-          latestDelta.inserted,
-        );
-
         // it might happen that after applying latest changes the delta itself does not contain any changes
-        if (Delta.isInnerDifferent(strippedDeleted, strippedInserted)) {
+        if (Delta.isInnerDifferent(latestDelta.deleted, latestDelta.inserted)) {
           modifiedDeltas[id] = latestDelta;
         }
       }
@@ -2075,12 +2063,4 @@ export class ElementsDelta implements DeltaContainer<SceneElementsMap> {
 
     return strippedPartial;
   }
-
-  private static stripVersionProps(
-    partial: Partial<OrderedExcalidrawElement>,
-  ): ElementPartial {
-    const { version, versionNonce, ...strippedPartial } = partial;
-
-    return strippedPartial;
-  }
 }

packages/element/tests/delta.test.tsx

@@ -8,7 +8,7 @@ import { AppStateDelta, Delta, ElementsDelta } from "../src/delta";
 
 describe("ElementsDelta", () => {
   describe("elements delta calculation", () => {
-    it("should not create removed delta when element gets removed but was already deleted", () => {
+    it("should not throw when element gets removed but was already deleted", () => {
       const element = API.createElement({
         type: "rectangle",
         x: 100,
@@ -19,12 +19,12 @@ describe("ElementsDelta", () => {
       const prevElements = new Map([[element.id, element]]);
       const nextElements = new Map();
 
-      const delta = ElementsDelta.calculate(prevElements, nextElements);
+      expect(() =>
-
+        ElementsDelta.calculate(prevElements, nextElements),
-      expect(delta.isEmpty()).toBeTruthy();
+      ).not.toThrow();
     });
 
-    it("should not create added delta when adding element as already deleted", () => {
+    it("should not throw when adding element as already deleted", () => {
       const element = API.createElement({
         type: "rectangle",
         x: 100,
@@ -35,12 +35,12 @@ describe("ElementsDelta", () => {
       const prevElements = new Map();
       const nextElements = new Map([[element.id, element]]);
 
-      const delta = ElementsDelta.calculate(prevElements, nextElements);
+      expect(() =>
-
+        ElementsDelta.calculate(prevElements, nextElements),
-      expect(delta.isEmpty()).toBeTruthy();
+      ).not.toThrow();
     });
 
-    it("should not create updated delta when there is only version and versionNonce change", () => {
+    it("should create updated delta even when there is only version and versionNonce change", () => {
       const baseElement = API.createElement({
         type: "rectangle",
         x: 100,
@@ -65,7 +65,24 @@ describe("ElementsDelta", () => {
         nextElements as SceneElementsMap,
       );
 
-      expect(delta.isEmpty()).toBeTruthy();
+      expect(delta).toEqual(
+        ElementsDelta.create(
+          {},
+          {},
+          {
+            [baseElement.id]: Delta.create(
+              {
+                version: baseElement.version,
+                versionNonce: baseElement.versionNonce,
+              },
+              {
+                version: baseElement.version + 1,
+                versionNonce: baseElement.versionNonce + 1,
+              },
+            ),
+          },
+        ),
+      );
     });
   });
 

packages/excalidraw/tests/__snapshots__/history.test.tsx.snap

@@ -282,6 +282,14 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
       "added": {},
       "removed": {},
       "updated": {
+        "id0": {
+          "deleted": {
+            "version": 12,
+          },
+          "inserted": {
+            "version": 11,
+          },
+        },
         "id1": {
           "deleted": {
             "boundElements": [],
@@ -396,6 +404,14 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
             "version": 12,
           },
         },
+        "id15": {
+          "deleted": {
+            "version": 10,
+          },
+          "inserted": {
+            "version": 9,
+          },
+        },
         "id4": {
           "deleted": {
             "height": "99.19972",
@@ -837,6 +853,14 @@ exports[`history > multiplayer undo/redo > conflicts in arrows and their bindabl
       "added": {},
       "removed": {},
       "updated": {
+        "id0": {
+          "deleted": {
+            "version": 13,
+          },
+          "inserted": {
+            "version": 12,
+          },
+        },
         "id1": {
           "deleted": {
             "boundElements": [],
@@ -2632,7 +2656,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "height": 100,
   "id": "id0",
   "index": "a0",
-  "isDeleted": true,
+  "isDeleted": false,
   "link": null,
   "locked": false,
   "opacity": 100,
@@ -2681,7 +2705,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "textAlign": "left",
   "type": "text",
   "updated": 1,
-  "version": 6,
+  "version": 8,
   "verticalAlign": "top",
   "width": 100,
   "x": 15,
@@ -2695,7 +2719,7 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
   "autoResize": true,
   "backgroundColor": "transparent",
   "boundElements": null,
-  "containerId": null,
+  "containerId": "id0",
   "customData": undefined,
   "fillStyle": "solid",
   "fontFamily": 5,
@@ -2742,10 +2766,12 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
       },
     },
     "elements": {
-      "added": {
+      "added": {},
+      "removed": {},
+      "updated": {
         "id0": {
           "deleted": {
-            "isDeleted": true,
+            "isDeleted": false,
             "version": 9,
           },
           "inserted": {
@@ -2774,16 +2800,21 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
             "y": 10,
           },
         },
-      },
+        "id1": {
-      "removed": {},
-      "updated": {
-        "id5": {
           "deleted": {
+            "containerId": null,
+            "version": 8,
+          },
+          "inserted": {
             "containerId": null,
             "version": 7,
           },
+        },
+        "id5": {
+          "deleted": {
+            "version": 7,
+          },
           "inserted": {
-            "containerId": "id0",
             "version": 6,
           },
         },
@@ -3096,6 +3127,14 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
             "version": 8,
           },
         },
+        "id5": {
+          "deleted": {
+            "version": 7,
+          },
+          "inserted": {
+            "version": 6,
+          },
+        },
       },
     },
     "id": "id9",
@@ -4645,15 +4684,15 @@ exports[`history > multiplayer undo/redo > conflicts in bound text elements and
         "id1": {
           "deleted": {
             "angle": 0,
-            "version": 4,
+            "version": 8,
             "x": 15,
             "y": 15,
           },
           "inserted": {
-            "angle": 90,
+            "angle": 0,
-            "version": 3,
+            "version": 7,
-            "x": 205,
+            "x": 15,
-            "y": 205,
+            "y": 15,
           },
         },
       },
@@ -5632,12 +5671,12 @@ exports[`history > multiplayer undo/redo > conflicts in frames and their childre
       "updated": {
         "id1": {
           "deleted": {
-            "frameId": "id0",
+            "frameId": null,
-            "version": 5,
+            "version": 9,
           },
           "inserted": {
             "frameId": null,
-            "version": 6,
+            "version": 8,
           },
         },
       },
@@ -5784,7 +5823,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 5,
+  "version": 6,
   "width": 100,
   "x": 0,
   "y": 0,
@@ -5816,7 +5855,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 4,
+  "version": 5,
   "width": 100,
   "x": 100,
   "y": 100,
@@ -5852,7 +5891,74 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [
+              "A",
+            ],
+            "height": 100,
+            "index": "a0",
+            "isDeleted": true,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 5,
+            "width": 100,
+            "x": 0,
+            "y": 0,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 4,
+          },
+        },
+        "id1": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [
+              "A",
+            ],
+            "height": 100,
+            "index": "a1",
+            "isDeleted": true,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 5,
+            "width": 100,
+            "x": 100,
+            "y": 100,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 4,
+          },
+        },
+      },
     },
     "id": "id13",
   },
@@ -6072,7 +6178,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 8,
+  "version": 9,
   "width": 10,
   "x": 20,
   "y": 0,
@@ -6102,7 +6208,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 8,
+  "version": 9,
   "width": 10,
   "x": 50,
   "y": 50,
@@ -6187,7 +6293,39 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id3": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [],
+            "height": 10,
+            "index": "a1",
+            "isDeleted": true,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 8,
+            "width": 10,
+            "x": 20,
+            "y": 0,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 7,
+          },
+        },
+      },
     },
     "id": "id18",
   },
@@ -6205,11 +6343,11 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
         "id3": {
           "deleted": {
             "backgroundColor": "#ffc9c9",
-            "version": 8,
+            "version": 9,
           },
           "inserted": {
             "backgroundColor": "transparent",
-            "version": 7,
+            "version": 8,
           },
         },
       },
@@ -6234,7 +6372,39 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id8": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "#ffc9c9",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [],
+            "height": 10,
+            "index": "a2",
+            "isDeleted": true,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 8,
+            "width": 10,
+            "x": 30,
+            "y": 30,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 7,
+          },
+        },
+      },
     },
     "id": "id20",
   },
@@ -6251,12 +6421,12 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
       "updated": {
         "id8": {
           "deleted": {
-            "version": 8,
+            "version": 9,
             "x": 50,
             "y": 50,
           },
           "inserted": {
-            "version": 7,
+            "version": 8,
             "x": 30,
             "y": 30,
           },
@@ -7104,7 +7274,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "arrow",
   "updated": 1,
-  "version": 8,
+  "version": 9,
   "width": 10,
   "x": 0,
   "y": 0,
@@ -7135,7 +7305,60 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "elbowed": false,
+            "endArrowhead": "arrow",
+            "endBinding": null,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [],
+            "height": 10,
+            "index": "a0",
+            "isDeleted": true,
+            "lastCommittedPoint": [
+              10,
+              10,
+            ],
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "points": [
+              [
+                0,
+                0,
+              ],
+              [
+                10,
+                10,
+              ],
+            ],
+            "roughness": 1,
+            "roundness": {
+              "type": 2,
+            },
+            "startArrowhead": null,
+            "startBinding": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "arrow",
+            "version": 9,
+            "width": 10,
+            "x": 0,
+            "y": 0,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 8,
+          },
+        },
+      },
     },
     "id": "id13",
   },
@@ -7344,7 +7567,7 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 8,
+  "version": 9,
   "width": 10,
   "x": 10,
   "y": 0,
@@ -7375,7 +7598,39 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "angle": 0,
+            "backgroundColor": "transparent",
+            "boundElements": null,
+            "customData": undefined,
+            "fillStyle": "solid",
+            "frameId": null,
+            "groupIds": [],
+            "height": 10,
+            "index": "a0",
+            "isDeleted": true,
+            "link": null,
+            "locked": false,
+            "opacity": 100,
+            "roughness": 1,
+            "roundness": null,
+            "strokeColor": "#1e1e1e",
+            "strokeStyle": "solid",
+            "strokeWidth": 2,
+            "type": "rectangle",
+            "version": 8,
+            "width": 10,
+            "x": 10,
+            "y": 0,
+          },
+          "inserted": {
+            "isDeleted": true,
+            "version": 7,
+          },
+        },
+      },
     },
     "id": "id7",
   },
@@ -7393,11 +7648,11 @@ exports[`history > multiplayer undo/redo > should iterate through the history wh
         "id0": {
           "deleted": {
             "backgroundColor": "#ffec99",
-            "version": 8,
+            "version": 9,
           },
           "inserted": {
             "backgroundColor": "transparent",
-            "version": 7,
+            "version": 8,
           },
         },
       },
@@ -10326,7 +10581,7 @@ exports[`history > multiplayer undo/redo > should redistribute deltas when eleme
   "strokeWidth": 2,
   "type": "rectangle",
   "updated": 1,
-  "version": 8,
+  "version": 9,
   "width": 10,
   "x": 10,
   "y": 0,
@@ -10409,7 +10664,18 @@ exports[`history > multiplayer undo/redo > should redistribute deltas when eleme
     "elements": {
       "added": {},
       "removed": {},
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "isDeleted": false,
+            "version": 9,
+          },
+          "inserted": {
+            "isDeleted": false,
+            "version": 8,
+          },
+        },
+      },
     },
     "id": "id8",
   },
@@ -15775,6 +16041,14 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 5,
           },
         },
+        "id1": {
+          "deleted": {
+            "version": 5,
+          },
+          "inserted": {
+            "version": 4,
+          },
+        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -16736,6 +17010,14 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 5,
           },
         },
+        "id1": {
+          "deleted": {
+            "version": 6,
+          },
+          "inserted": {
+            "version": 5,
+          },
+        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -17361,6 +17643,14 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 9,
           },
         },
+        "id1": {
+          "deleted": {
+            "version": 10,
+          },
+          "inserted": {
+            "version": 9,
+          },
+        },
         "id2": {
           "deleted": {
             "boundElements": [
@@ -17722,6 +18012,14 @@ exports[`history > singleplayer undo/redo > should support bidirectional binding
             "version": 7,
           },
         },
+        "id2": {
+          "deleted": {
+            "version": 4,
+          },
+          "inserted": {
+            "version": 3,
+          },
+        },
       },
     },
     "id": "id21",

packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap

@@ -2216,7 +2216,16 @@ exports[`regression tests > alt-drag duplicates an element > [end of test] undo
           },
         },
       },
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "version": 5,
+          },
+          "inserted": {
+            "version": 3,
+          },
+        },
+      },
     },
     "id": "id6",
   },
@@ -10892,7 +10901,32 @@ exports[`regression tests > make a group and duplicate it > [end of test] undo s
           },
         },
       },
-      "updated": {},
+      "updated": {
+        "id0": {
+          "deleted": {
+            "version": 6,
+          },
+          "inserted": {
+            "version": 4,
+          },
+        },
+        "id3": {
+          "deleted": {
+            "version": 6,
+          },
+          "inserted": {
+            "version": 4,
+          },
+        },
+        "id6": {
+          "deleted": {
+            "version": 6,
+          },
+          "inserted": {
+            "version": 4,
+          },
+        },
+      },
     },
     "id": "id21",
   },

packages/excalidraw/tests/history.test.tsx

@@ -4055,7 +4055,7 @@ describe("history", () => {
             expect.objectContaining({
               id: container.id,
               boundElements: [{ id: remoteText.id, type: "text" }],
-              isDeleted: true,
+              isDeleted: false,
             }),
             expect.objectContaining({
               id: text.id,
@@ -4064,8 +4064,7 @@ describe("history", () => {
             }),
             expect.objectContaining({
               id: remoteText.id,
-              // unbound
+              containerId: container.id,
-              containerId: null,
               isDeleted: false,
             }),
           ]);