From b5ad7ae4e3fe2b201ee639a11345e4e31e2974e6 Mon Sep 17 00:00:00 2001 From: Marcel Mraz Date: Thu, 21 Aug 2025 16:09:19 +0200 Subject: [PATCH] fix: even deltas with version & version nonce are valid (#9897) --- packages/element/src/delta.ts | 50 +-- packages/element/tests/delta.test.tsx | 37 +- .../tests/__snapshots__/history.test.tsx.snap | 372 ++++++++++++++++-- .../regressionTests.test.tsx.snap | 38 +- packages/excalidraw/tests/history.test.tsx | 5 +- 5 files changed, 415 insertions(+), 87 deletions(-) diff --git a/packages/element/src/delta.ts b/packages/element/src/delta.ts index d7b242d60f..97b9403bcc 100644 --- a/packages/element/src/delta.ts +++ b/packages/element/src/delta.ts @@ -1111,16 +1111,16 @@ export class ElementsDelta implements DeltaContainer { inserted, }: Delta) => !!( - 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 && - inserted.version >= 0 && - // versions should never be the same - deleted.version !== inserted.version + ( + Number.isInteger(deleted.version) && + Number.isInteger(inserted.version) && + // versions should be positive, zero included + deleted.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 { 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 { // ignore updates which would "delete" already deleted element if (!nextElement.isDeleted) { added[nextElement.id] = delta; + } else { + updated[nextElement.id] = delta; } continue; @@ -1250,15 +1253,7 @@ export class ElementsDelta implements DeltaContainer { 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; - } + updated[nextElement.id] = delta; } } @@ -1372,15 +1367,8 @@ export class ElementsDelta implements DeltaContainer { 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 { return strippedPartial; } - - private static stripVersionProps( - partial: Partial, - ): ElementPartial { - const { version, versionNonce, ...strippedPartial } = partial; - - return strippedPartial; - } } diff --git a/packages/element/tests/delta.test.tsx b/packages/element/tests/delta.test.tsx index e9a19d850a..2b39b32df7 100644 --- a/packages/element/tests/delta.test.tsx +++ b/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(delta.isEmpty()).toBeTruthy(); + expect(() => + ElementsDelta.calculate(prevElements, nextElements), + ).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(delta.isEmpty()).toBeTruthy(); + expect(() => + ElementsDelta.calculate(prevElements, nextElements), + ).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, + }, + ), + }, + ), + ); }); }); diff --git a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap index 60d7e5ed6b..2f9e04d562 100644 --- a/packages/excalidraw/tests/__snapshots__/history.test.tsx.snap +++ b/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, }, }, - }, - "removed": {}, - "updated": { - "id5": { + "id1": { "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, - "version": 3, - "x": 205, - "y": 205, + "angle": 0, + "version": 7, + "x": 15, + "y": 15, }, }, }, @@ -5632,12 +5671,12 @@ exports[`history > multiplayer undo/redo > conflicts in frames and their childre "updated": { "id1": { "deleted": { - "frameId": "id0", - "version": 5, + "frameId": null, + "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", diff --git a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap b/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap index c16cd9884f..a895eb6366 100644 --- a/packages/excalidraw/tests/__snapshots__/regressionTests.test.tsx.snap +++ b/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", }, diff --git a/packages/excalidraw/tests/history.test.tsx b/packages/excalidraw/tests/history.test.tsx index 09510e5ebb..9ef8198569 100644 --- a/packages/excalidraw/tests/history.test.tsx +++ b/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: null, + containerId: container.id, isDeleted: false, }), ]);