From d62ed7060773b98eac215f2f2fa4ebedf7bc482e Mon Sep 17 00:00:00 2001
From: dwelle <5153846+dwelle@users.noreply.github.com>
Date: Thu, 17 Apr 2025 14:51:14 +0200
Subject: [PATCH] fix: duplicating within group outside frame should remove
 from group

---
 packages/element/src/duplicate.ts         |   8 +-
 packages/element/tests/duplicate.test.tsx | 132 ++++++++++++++++++++--
 packages/excalidraw/components/App.tsx    |   9 +-
 3 files changed, 138 insertions(+), 11 deletions(-)

diff --git a/packages/element/src/duplicate.ts b/packages/element/src/duplicate.ts
index 59beae43c..c2cee4c08 100644
--- a/packages/element/src/duplicate.ts
+++ b/packages/element/src/duplicate.ts
@@ -95,6 +95,7 @@ export const duplicateElements = (
     elements: readonly ExcalidrawElement[];
     randomizeSeed?: boolean;
     overrides?: (data: {
+      duplicateElement: ExcalidrawElement;
       origElement: ExcalidrawElement;
       origIdToDuplicateId: Map<
         ExcalidrawElement["id"],
@@ -377,12 +378,13 @@ export const duplicateElements = (
   );
 
   if (opts.overrides) {
-    for (const copy of duplicatedElements) {
-      const origElement = duplicateIdToOrigElement.get(copy.id);
+    for (const duplicateElement of duplicatedElements) {
+      const origElement = duplicateIdToOrigElement.get(duplicateElement.id);
       if (origElement) {
         Object.assign(
-          copy,
+          duplicateElement,
           opts.overrides({
+            duplicateElement,
             origElement,
             origIdToDuplicateId,
           }),
diff --git a/packages/element/tests/duplicate.test.tsx b/packages/element/tests/duplicate.test.tsx
index 24042b347..409200b6e 100644
--- a/packages/element/tests/duplicate.test.tsx
+++ b/packages/element/tests/duplicate.test.tsx
@@ -408,6 +408,117 @@ describe("duplicating multiple elements", () => {
   });
 });
 
+describe("group-related duplication", () => {
+  beforeEach(async () => {
+    await render(<Excalidraw />);
+  });
+
+  it("action-duplicating within group", async () => {
+    const rectangle1 = API.createElement({
+      type: "rectangle",
+      x: 0,
+      y: 0,
+      groupIds: ["group1"],
+    });
+    const rectangle2 = API.createElement({
+      type: "rectangle",
+      x: 10,
+      y: 10,
+      groupIds: ["group1"],
+    });
+
+    API.setElements([rectangle1, rectangle2]);
+    API.setSelectedElements([rectangle2], "group1");
+
+    act(() => {
+      h.app.actionManager.executeAction(actionDuplicateSelection);
+    });
+
+    assertElements(h.elements, [
+      { id: rectangle1.id },
+      { id: rectangle2.id },
+      { [ORIG_ID]: rectangle2.id, selected: true, groupIds: ["group1"] },
+    ]);
+    expect(h.state.editingGroupId).toBe("group1");
+  });
+
+  it("alt-duplicating within group", async () => {
+    const rectangle1 = API.createElement({
+      type: "rectangle",
+      x: 0,
+      y: 0,
+      groupIds: ["group1"],
+    });
+    const rectangle2 = API.createElement({
+      type: "rectangle",
+      x: 10,
+      y: 10,
+      groupIds: ["group1"],
+    });
+
+    API.setElements([rectangle1, rectangle2]);
+    API.setSelectedElements([rectangle2], "group1");
+
+    Keyboard.withModifierKeys({ alt: true }, () => {
+      mouse.down(rectangle2.x + 5, rectangle2.y + 5);
+      mouse.up(rectangle2.x + 50, rectangle2.y + 50);
+    });
+
+    assertElements(h.elements, [
+      { id: rectangle1.id },
+      { id: rectangle2.id },
+      { [ORIG_ID]: rectangle2.id, selected: true, groupIds: ["group1"] },
+    ]);
+    expect(h.state.editingGroupId).toBe("group1");
+  });
+
+  it.skip("alt-duplicating within group away outside frame", () => {
+    const frame = API.createElement({
+      type: "frame",
+      x: 0,
+      y: 0,
+      width: 100,
+      height: 100,
+    });
+    const rectangle1 = API.createElement({
+      type: "rectangle",
+      x: 0,
+      y: 0,
+      width: 50,
+      height: 50,
+      groupIds: ["group1"],
+      frameId: frame.id,
+    });
+    const rectangle2 = API.createElement({
+      type: "rectangle",
+      x: 10,
+      y: 10,
+      width: 50,
+      height: 50,
+      groupIds: ["group1"],
+      frameId: frame.id,
+    });
+
+    API.setElements([frame, rectangle1, rectangle2]);
+    API.setSelectedElements([rectangle2], "group1");
+
+    Keyboard.withModifierKeys({ alt: true }, () => {
+      mouse.down(rectangle2.x + 5, rectangle2.y + 5);
+      mouse.up(frame.x + frame.width + 50, frame.y + frame.height + 50);
+    });
+
+    // console.log(h.elements);
+
+    assertElements(h.elements, [
+      { id: frame.id },
+      { id: rectangle1.id, frameId: frame.id },
+      { id: rectangle2.id, frameId: frame.id },
+      { [ORIG_ID]: rectangle2.id, selected: true, groupIds: [], frameId: null },
+    ]);
+    expect(h.state.editingGroupId).toBe(null);
+  });
+});
+
 describe("duplication z-order", () => {
   beforeEach(async () => {
     await render(<Excalidraw />);
@@ -703,7 +814,7 @@ describe("duplication z-order", () => {
     ]);
   });
 
-  it("alt-duplicating bindable element with bound arrow should keep the arrow on the duplicate", () => {
+  it("alt-duplicating bindable element with bound arrow should keep the arrow on the duplicate", async () => {
     const rect = UI.createElement("rectangle", {
       x: 0,
       y: 0,
@@ -725,11 +836,18 @@ describe("duplication z-order", () => {
       mouse.up(15, 15);
     });
 
-    expect(window.h.elements).toHaveLength(3);
-
-    const newRect = window.h.elements[0];
-
-    expect(arrow.endBinding?.elementId).toBe(newRect.id);
-    expect(newRect.boundElements?.[0]?.id).toBe(arrow.id);
+    assertElements(h.elements, [
+      {
+        id: rect.id,
+        boundElements: expect.arrayContaining([
+          expect.objectContaining({ id: arrow.id }),
+        ]),
+      },
+      { [ORIG_ID]: rect.id, boundElements: [], selected: true },
+      {
+        id: arrow.id,
+        endBinding: expect.objectContaining({ elementId: rect.id }),
+      },
+    ]);
   });
 });
diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx
index 2b75249ba..6a7306422 100644
--- a/packages/excalidraw/components/App.tsx
+++ b/packages/excalidraw/components/App.tsx
@@ -8445,8 +8445,15 @@ class App extends React.Component<AppProps, AppState> {
               appState: this.state,
               randomizeSeed: true,
               idsOfElementsToDuplicate,
-              overrides: () => {
+              overrides: ({ duplicateElement, origElement }) => {
                 return {
+                  // reset to the original element's frameId (unless we've
+                  // duplicated alongside a frame in which case we need to
+                  // keep the duplicate frame's id) so that the element
+                  // frame membership is refreshed on pointerup
+                  // NOTE this is a hacky solution and should be done
+                  // differently
+                  frameId: duplicateElement.frameId ?? origElement.frameId,
                   seed: randomInteger(),
                 };
               },