fix(editor): elements duplicated when moving frame children (#11485)

* fix(editor): elements duplicated when moving frame children * fix(editor): accumulate reorders across frames in bring-to-front/back * add invalid-order tests * fix: make sure moved indices are within range * add length/duplicate elements guards
2026-06-13 20:32:45 +02:00
parent a83ac48853
commit b324a85ab1
2 changed files with 255 additions and 3 deletions
@@ -1,5 +1,7 @@
 import { arrayToMap, findIndex, findLastIndex } from "@excalidraw/common";

+import { isFiniteNumber } from "@excalidraw/math";
+
 import type { AppState } from "@excalidraw/excalidraw/types";
 import type { GlobalPoint } from "@excalidraw/math";

@@ -313,12 +315,46 @@ const getTargetElementsMap = <T extends ExcalidrawElement>(
  }, new Map<string, ExcalidrawElement>());
 };

+const hasSameElementIds = (
+  prevElements: readonly ExcalidrawElement[],
+  nextElements: readonly ExcalidrawElement[],
+) => {
+  if (prevElements.length !== nextElements.length) {
+    console.error(
+      "z-index reordering failed: resulting array have different lengths",
+    );
+    return false;
+  }
+
+  const prevElementIdCounts = new Map<ExcalidrawElement["id"], number>();
+  for (const element of prevElements) {
+    prevElementIdCounts.set(
+      element.id,
+      (prevElementIdCounts.get(element.id) || 0) + 1,
+    );
+  }
+
+  for (const element of nextElements) {
+    const count = prevElementIdCounts.get(element.id);
+    if (!count) {
+      console.error(
+        "z-index reordering failed: element id mismatch / duplicate ids",
+      );
+      return false;
+    }
+    prevElementIdCounts.set(element.id, count - 1);
+  }
+
+  return true;
+};
+
 const shiftElementsByOne = (
  elements: readonly ExcalidrawElement[],
  appState: AppState,
  direction: "left" | "right",
  scene: Scene,
 ) => {
+  const originalElements = elements;
  const indicesToMove = getIndicesToMove(elements, appState);
  const targetElementsMap = getTargetElementsMap(elements, indicesToMove);

@@ -389,6 +425,10 @@ const shiftElementsByOne = (
          ];
  });

+  if (!hasSameElementIds(originalElements, elements)) {
+    return originalElements;
+  }
+
  syncMovedIndices(elements, targetElementsMap);

  return elements;
@@ -402,11 +442,20 @@ const shiftElementsToEnd = (
  elementsToBeMoved?: readonly ExcalidrawElement[],
 ) => {
  const indicesToMove = getIndicesToMove(elements, appState, elementsToBeMoved);
+
+  // Nothing to move (e.g. `elementsToBeMoved` is empty because all selected
+  // elements were frame children handled in a prior pass). Bail out early —
+  // otherwise `leadingIndex`/`trailingIndex` below resolve to `undefined` and
+  // the resulting `slice()` calls overlap, duplicating elements.
+  if (indicesToMove.length === 0) {
+    return elements;
+  }
+
  const targetElementsMap = getTargetElementsMap(elements, indicesToMove);
  const displacedElements: ExcalidrawElement[] = [];

-  let leadingIndex: number;
-  let trailingIndex: number;
+  let leadingIndex: number | undefined;
+  let trailingIndex: number | undefined;
  if (direction === "left") {
    if (containingFrame) {
      leadingIndex = findIndex(elements, (el) =>
@@ -451,6 +500,19 @@ const shiftElementsToEnd = (
    leadingIndex = 0;
  }

+  const isValidIndex = (index: number | undefined): index is number => {
+    return isFiniteNumber(index) && index >= 0;
+  };
+
+  if (
+    !isValidIndex(leadingIndex) ||
+    !isValidIndex(trailingIndex) ||
+    leadingIndex > trailingIndex ||
+    indicesToMove.some((index) => index < leadingIndex || index > trailingIndex)
+  ) {
+    return elements;
+  }
+
  for (let index = leadingIndex; index < trailingIndex + 1; index++) {
    if (!indicesToMove.includes(index)) {
      displacedElements.push(elements[index]);
@@ -475,6 +537,10 @@ const shiftElementsToEnd = (
          ...trailingElements,
        ];

+  if (!hasSameElementIds(elements, nextElements)) {
+    return elements;
+  }
+
  syncMovedIndices(nextElements, targetElementsMap);

  return nextElements;
@@ -543,7 +609,7 @@ function shiftElementsAccountingForFrames(

  for (const [frameId, children] of frameChildrenSets) {
    nextElements = shiftFunction(
-      allElements,
+      nextElements,
      appState,
      direction,
      frameId,
@@ -1509,4 +1509,190 @@ describe("z-indexing with frames", () => {
      ],
    });
  });
+
+  it("bringing to front / sending to back children of MULTIPLE frames at once moves all of them", () => {
+    assertZindex({
+      elements: [
+        { id: "F1_1", frameId: "F1", isSelected: true },
+        { id: "F1_2", frameId: "F1" },
+        { id: "F1", type: "frame" },
+        { id: "F2_1", frameId: "F2", isSelected: true },
+        { id: "F2_2", frameId: "F2" },
+        { id: "F2", type: "frame" },
+      ],
+      operations: [
+        // +∞: each selected child moves to the front of its own frame
+        [actionBringToFront, ["F1_2", "F1", "F1_1", "F2_2", "F2", "F2_1"]],
+        // -∞: each selected child moves to the back of its own frame
+        [actionSendToBack, ["F1_1", "F1_2", "F1", "F2_1", "F2_2", "F2"]],
+      ],
+    });
+  });
+
+  it("send to back / bring to front of a grouped frame child (in group-editing mode) must not duplicate elements", () => {
+    assertZindex({
+      elements: [
+        { id: "F1_1", frameId: "F1", groupIds: ["g1"] },
+        { id: "F1_2", frameId: "F1", groupIds: ["g1"], isSelected: true },
+        { id: "F1", type: "frame" },
+        { id: "F2_1", frameId: "F2", groupIds: ["g2"] },
+        { id: "F2_2", frameId: "F2", groupIds: ["g2"] },
+        { id: "F2", type: "frame" },
+      ],
+      appState: { editingGroupId: "g1" },
+      operations: [
+        // -∞ (send to back, within the frame)
+        [actionSendToBack, ["F1_2", "F1_1", "F1", "F2_1", "F2_2", "F2"]],
+        // +∞ (bring to front, within the frame)
+        [actionBringToFront, ["F1_1", "F1", "F1_2", "F2_1", "F2_2", "F2"]],
+      ],
+    });
+  });
+});
+
+/**
+ * The inputs in this block intentionally VIOLATE the (soft) invariant that a
+ * frame's children — and a group's members — are contiguous in the elements
+ * array. Such states shouldn't occur in normal use, but they CAN arise from
+ * bugs or broken input, because nothing re-defragments element order during
+ * a reorder (`normalizeElementOrder` only runs on duplication). We keep these
+ * tests so the reordering ops stay exercised against malformed order.
+ *
+ * HARD CONTRACT (a failure here is a real bug): a reorder must never throw,
+ * duplicate, or drop elements. `assertReorderPreservesElements` checks this.
+ *
+ * SOFT SNAPSHOT (read before "fixing"): the exact resulting ORDER is NOT a
+ * contract for invalid input — it's whatever the slice math happens to
+ * produce. If a future change alters an `expected` order below, that is NOT
+ * necessarily a functional regression. First confirm from the diff that the
+ * hard contract still holds (nothing duplicated/lost), then update the
+ * expected order to match, provided it's deemed an improvement over the
+ * previous order, or it's an acceptable change given the underlying logic
+ * change.
+ */
+describe("z-index reordering with broken contiguity (invariant-violating input)", () => {
+  beforeEach(async () => {
+    await render(<Excalidraw />);
+  });
+
+  const assertReorderPreservesElements = (
+    elements: Parameters<typeof populateElements>[0],
+    appState: Parameters<typeof populateElements>[1],
+    // each op is applied to a freshly-populated (broken) state
+    cases: [Actions, string[]][],
+  ) => {
+    for (const [action, expected] of cases) {
+      populateElements(elements, appState);
+      const before = h.elements.map((el) => el.id);
+
+      expect(() => API.executeAction(action)).not.toThrow();
+
+      const after = h.elements.map((el) => el.id);
+      // hard contract:
+      expect(after.length).toBe(before.length); // no loss
+      expect(new Set(after).size).toBe(after.length); // no duplication
+      // soft snapshot (see block comment before changing):
+      expect(after).toEqual(expected);
+    }
+  };
+
+  it("discontiguous frame children (foreign frame's child interleaved in span)", () => {
+    // F2_1 (a child of frame F2) sits INSIDE frame F1's z-span. Reordering F1's
+    // child sweeps F2_1 along (span-based frame handling) — wrong ordering, but
+    // never a duplication/loss, and the op does not throw.
+    const elements: Parameters<typeof populateElements>[0] = [
+      { id: "F1_1", frameId: "F1", isSelected: true },
+      { id: "F2_1", frameId: "F2" },
+      { id: "F1_2", frameId: "F1" },
+      { id: "F1", type: "frame" },
+      { id: "F2", type: "frame" },
+    ];
+    assertReorderPreservesElements(elements, undefined, [
+      [actionBringForward, ["F2_1", "F1_2", "F1_1", "F1", "F2"]],
+      [actionSendBackward, ["F1_1", "F2_1", "F1_2", "F1", "F2"]],
+      [actionBringToFront, ["F2_1", "F1_2", "F1", "F1_1", "F2"]],
+      [actionSendToBack, ["F1_1", "F2_1", "F1_2", "F1", "F2"]],
+    ]);
+  });
+
+  it("discontiguous group, whole group selected", () => {
+    // g1 = {A, C}, scattered by the loose elements B and D.
+    const elements: Parameters<typeof populateElements>[0] = [
+      { id: "A", groupIds: ["g1"], isSelected: true },
+      { id: "B" },
+      { id: "C", groupIds: ["g1"], isSelected: true },
+      { id: "D" },
+    ];
+    assertReorderPreservesElements(elements, undefined, [
+      // move-by-one leaves the group scattered (each run moves independently)
+      [actionBringForward, ["B", "A", "D", "C"]],
+      [actionSendBackward, ["A", "C", "B", "D"]],
+      // to-front / to-back gather the scattered members back into one block
+      [actionBringToFront, ["B", "D", "A", "C"]],
+      [actionSendToBack, ["A", "C", "B", "D"]],
+    ]);
+  });
+
+  it("discontiguous group, single member selected in group-editing mode", () => {
+    const elements: Parameters<typeof populateElements>[0] = [
+      { id: "A", groupIds: ["g1"] },
+      { id: "B" },
+      { id: "C", groupIds: ["g1"], isSelected: true },
+      { id: "D" },
+    ];
+    assertReorderPreservesElements(elements, { editingGroupId: "g1" }, [
+      [actionBringForward, ["A", "B", "C", "D"]],
+      [actionSendBackward, ["C", "A", "B", "D"]],
+      [actionBringToFront, ["A", "B", "C", "D"]],
+      [actionSendToBack, ["C", "A", "B", "D"]],
+    ]);
+  });
+
+  it("two interleaved groups, both fully selected", () => {
+    const elements: Parameters<typeof populateElements>[0] = [
+      { id: "A", groupIds: ["g1"], isSelected: true },
+      { id: "X", groupIds: ["g2"], isSelected: true },
+      { id: "C", groupIds: ["g1"], isSelected: true },
+      { id: "Y", groupIds: ["g2"], isSelected: true },
+      { id: "Z" },
+    ];
+    assertReorderPreservesElements(elements, undefined, [
+      [actionBringForward, ["Z", "A", "X", "C", "Y"]],
+      [actionSendBackward, ["A", "X", "C", "Y", "Z"]],
+      [actionBringToFront, ["Z", "A", "X", "C", "Y"]],
+      [actionSendToBack, ["A", "X", "C", "Y", "Z"]],
+    ]);
+  });
+});
+
+describe("z-index reordering with inconsistent group-editing state", () => {
+  beforeEach(async () => {
+    await render(<Excalidraw />);
+  });
+
+  it("does not duplicate or drop elements when selected elements fall outside the edited group scope", () => {
+    assertZindex({
+      elements: [
+        { id: "A", groupIds: ["g1"], isSelected: true },
+        { id: "C", groupIds: ["g1"] },
+        { id: "X", groupIds: ["g2"] },
+        { id: "Y", groupIds: ["g2"] },
+        { id: "R" },
+      ],
+      appState: { editingGroupId: "g2" },
+      operations: [[actionSendToBack, ["A", "C", "X", "Y", "R"]]],
+    });
+
+    assertZindex({
+      elements: [
+        { id: "A", groupIds: ["g1"] },
+        { id: "C", groupIds: ["g1"] },
+        { id: "X", groupIds: ["g2"], isSelected: true },
+        { id: "Y", groupIds: ["g2"] },
+        { id: "R" },
+      ],
+      appState: { editingGroupId: "g1" },
+      operations: [[actionBringToFront, ["A", "C", "X", "Y", "R"]]],
+    });
+  });
 });