fix: incorrectly duplicating items on paste/library insert (#6467

* fix: incorrectly duplicating items on paste/library insert

* fix: deduplicate element ids on restore

* tests

src/components/App.tsx

@@ -127,7 +127,11 @@ import {
 } from "../element/binding";
 import { LinearElementEditor } from "../element/linearElementEditor";
 import { mutateElement, newElementWith } from "../element/mutateElement";
-import { deepCopyElement, newFreeDrawElement } from "../element/newElement";
+import {
+  deepCopyElement,
+  duplicateElements,
+  newFreeDrawElement,
+} from "../element/newElement";
 import {
   hasBoundTextElement,
   isArrowElement,
@@ -1625,35 +1629,22 @@ class App extends React.Component<AppProps, AppState> {
 
     const dx = x - elementsCenterX;
     const dy = y - elementsCenterY;
-    const groupIdMap = new Map();
 
     const [gridX, gridY] = getGridPoint(dx, dy, this.state.gridSize);
 
-    const oldIdToDuplicatedId = new Map();
-    const newElements = elements.map((element) => {
-      const newElement = duplicateElement(
-        this.state.editingGroupId,
-        groupIdMap,
-        element,
-        {
+    const newElements = duplicateElements(
+      elements.map((element) => {
+        return newElementWith(element, {
           x: element.x + gridX - minX,
           y: element.y + gridY - minY,
-        },
-      );
-      oldIdToDuplicatedId.set(element.id, newElement.id);
-      return newElement;
         });
+      }),
+    );
 
-    bindTextToShapeAfterDuplication(newElements, elements, oldIdToDuplicatedId);
     const nextElements = [
       ...this.scene.getElementsIncludingDeleted(),
       ...newElements,
     ];
-    fixBindingsAfterDuplication(nextElements, elements, oldIdToDuplicatedId);
-
-    if (opts.files) {
-      this.files = { ...this.files, ...opts.files };
-    }
 
     this.scene.replaceAllElements(nextElements);
 
@@ -1664,6 +1655,10 @@ class App extends React.Component<AppProps, AppState> {
       }
     });
 
+    if (opts.files) {
+      this.files = { ...this.files, ...opts.files };
+    }
+
     this.history.resumeRecording();
 
     this.setState(

src/data/restore.ts

@@ -369,6 +369,9 @@ export const restoreElements = (
   localElements: readonly ExcalidrawElement[] | null | undefined,
   opts?: { refreshDimensions?: boolean; repairBindings?: boolean } | undefined,
 ): ExcalidrawElement[] => {
+  // used to detect duplicate top-level element ids
+  const existingIds = new Set<string>();
+
   const localElementsMap = localElements ? arrayToMap(localElements) : null;
   const restoredElements = (elements || []).reduce((elements, element) => {
     // filtering out selection, which is legacy, no longer kept in elements,
@@ -383,6 +386,10 @@ export const restoreElements = (
         if (localElement && localElement.version > migratedElement.version) {
           migratedElement = bumpVersion(migratedElement, localElement.version);
         }
+        if (existingIds.has(migratedElement.id)) {
+          migratedElement = { ...migratedElement, id: randomId() };
+        }
+        existingIds.add(migratedElement.id);
         elements.push(migratedElement);
       }
     }

src/element/newElement.ts

@@ -439,6 +439,29 @@ export const deepCopyElement = <T extends ExcalidrawElement>(
   return _deepCopyElement(val);
 };
 
+/**
+ * utility wrapper to generate new id. In test env it reuses the old + postfix
+ * for test assertions.
+ */
+const regenerateId = (
+  /** supply null if no previous id exists */
+  previousId: string | null,
+) => {
+  if (isTestEnv() && previousId) {
+    let nextId = `${previousId}_copy`;
+    // `window.h` may not be defined in some unit tests
+    if (
+      window.h?.app
+        ?.getSceneElementsIncludingDeleted()
+        .find((el) => el.id === nextId)
+    ) {
+      nextId += "_copy";
+    }
+    return nextId;
+  }
+  return randomId();
+};
+
 /**
  * Duplicate an element, often used in the alt-drag operation.
  * Note that this method has gotten a bit complicated since the
@@ -461,19 +484,7 @@ export const duplicateElement = <TElement extends ExcalidrawElement>(
 ): Readonly<TElement> => {
   let copy = deepCopyElement(element);
 
-  if (isTestEnv()) {
-    copy.id = `${copy.id}_copy`;
-    // `window.h` may not be defined in some unit tests
-    if (
-      window.h?.app
-        ?.getSceneElementsIncludingDeleted()
-        .find((el) => el.id === copy.id)
-    ) {
-      copy.id += "_copy";
-    }
-  } else {
-    copy.id = randomId();
-  }
+  copy.id = regenerateId(copy.id);
   copy.boundElements = null;
   copy.updated = getUpdatedTimestamp();
   copy.seed = randomInteger();
@@ -482,7 +493,7 @@ export const duplicateElement = <TElement extends ExcalidrawElement>(
     editingGroupId,
     (groupId) => {
       if (!groupIdMapForOperation.has(groupId)) {
-        groupIdMapForOperation.set(groupId, randomId());
+        groupIdMapForOperation.set(groupId, regenerateId(groupId));
       }
       return groupIdMapForOperation.get(groupId)!;
     },
@@ -520,7 +531,7 @@ export const duplicateElements = (elements: readonly ExcalidrawElement[]) => {
     // if we haven't migrated the element id, but an old element with the same
     // id exists, generate a new id for it and return it
     if (origElementsMap.has(id)) {
-      const newId = randomId();
+      const newId = regenerateId(id);
       elementNewIdsMap.set(id, newId);
       return newId;
     }
@@ -538,7 +549,7 @@ export const duplicateElements = (elements: readonly ExcalidrawElement[]) => {
     if (clonedElement.groupIds) {
       clonedElement.groupIds = clonedElement.groupIds.map((groupId) => {
         if (!groupNewIdsMap.has(groupId)) {
-          groupNewIdsMap.set(groupId, randomId());
+          groupNewIdsMap.set(groupId, regenerateId(groupId));
         }
         return groupNewIdsMap.get(groupId)!;
       });

src/tests/__snapshots__/regressionTests.test.tsx.snap

@@ -13431,7 +13431,7 @@ Object {
   "boundElements": null,
   "fillStyle": "hachure",
   "groupIds": Array [
-    "id6",
+    "id4_copy",
   ],
   "height": 10,
   "id": "id0_copy",
@@ -13464,7 +13464,7 @@ Object {
   "boundElements": null,
   "fillStyle": "hachure",
   "groupIds": Array [
-    "id6",
+    "id4_copy",
   ],
   "height": 10,
   "id": "id1_copy",
@@ -13497,7 +13497,7 @@ Object {
   "boundElements": null,
   "fillStyle": "hachure",
   "groupIds": Array [
-    "id6",
+    "id4_copy",
   ],
   "height": 10,
   "id": "id2_copy",
@@ -13981,7 +13981,7 @@ Object {
           "boundElements": null,
           "fillStyle": "hachure",
           "groupIds": Array [
-            "id6",
+            "id4_copy",
           ],
           "height": 10,
           "id": "id0_copy",
@@ -14011,7 +14011,7 @@ Object {
           "boundElements": null,
           "fillStyle": "hachure",
           "groupIds": Array [
-            "id6",
+            "id4_copy",
           ],
           "height": 10,
           "id": "id1_copy",
@@ -14041,7 +14041,7 @@ Object {
           "boundElements": null,
           "fillStyle": "hachure",
           "groupIds": Array [
-            "id6",
+            "id4_copy",
           ],
           "height": 10,
           "id": "id2_copy",

src/tests/helpers/api.ts

@@ -211,7 +211,10 @@ export class API {
           type,
           startArrowhead: null,
           endArrowhead: null,
-          points: rest.points ?? [],
+          points: rest.points ?? [
+            [0, 0],
+            [100, 100],
+          ],
         });
         break;
       case "image":

src/tests/library.test.tsx

@@ -72,6 +72,100 @@ describe("library", () => {
     });
   });
 
+  it("should regenerate ids but retain bindings on library insert", async () => {
+    const rectangle = API.createElement({
+      id: "rectangle1",
+      type: "rectangle",
+      boundElements: [
+        { type: "text", id: "text1" },
+        { type: "arrow", id: "arrow1" },
+      ],
+    });
+    const text = API.createElement({
+      id: "text1",
+      type: "text",
+      text: "ola",
+      containerId: "rectangle1",
+    });
+    const arrow = API.createElement({
+      id: "arrow1",
+      type: "arrow",
+      endBinding: { elementId: "rectangle1", focus: -1, gap: 0 },
+    });
+
+    await API.drop(
+      new Blob(
+        [
+          serializeLibraryAsJSON([
+            {
+              id: "item1",
+              status: "published",
+              elements: [rectangle, text, arrow],
+              created: 1,
+            },
+          ]),
+        ],
+        {
+          type: MIME_TYPES.excalidrawlib,
+        },
+      ),
+    );
+
+    await waitFor(() => {
+      expect(h.elements).toEqual([
+        expect.objectContaining({
+          id: "rectangle1_copy",
+          boundElements: expect.arrayContaining([
+            { type: "text", id: "text1_copy" },
+            { type: "arrow", id: "arrow1_copy" },
+          ]),
+        }),
+        expect.objectContaining({
+          id: "text1_copy",
+          containerId: "rectangle1_copy",
+        }),
+        expect.objectContaining({
+          id: "arrow1_copy",
+          endBinding: expect.objectContaining({ elementId: "rectangle1_copy" }),
+        }),
+      ]);
+    });
+  });
+
+  it("should fix duplicate ids between items on insert", async () => {
+    // note, we're not testing for duplicate group ids and such because
+    // deduplication of that happens upstream in the library component
+    // which would be very hard to orchestrate in this test
+
+    const elem1 = API.createElement({
+      id: "elem1",
+      type: "rectangle",
+    });
+    const item1: LibraryItem = {
+      id: "item1",
+      status: "published",
+      elements: [elem1],
+      created: 1,
+    };
+
+    await API.drop(
+      new Blob([serializeLibraryAsJSON([item1, item1])], {
+        type: MIME_TYPES.excalidrawlib,
+      }),
+    );
+
+    await waitFor(() => {
+      expect(h.elements).toEqual([
+        expect.objectContaining({
+          id: "elem1_copy",
+        }),
+        expect.objectContaining({
+          id: expect.not.stringMatching(/^(elem1_copy|elem1)$/),
+        }),
+      ]);
+    });
+  });
+
   it("inserting library item should revert to selection tool", async () => {
     UI.clickTool("rectangle");
     expect(h.elements).toEqual([]);