From 74423812ec597e0e33cbe5032011fead2312e7b4 Mon Sep 17 00:00:00 2001 From: David Luzar <5153846+dwelle@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:45:39 +0200 Subject: [PATCH] perf(editor): avoid rebuilding id sets on every drag (#12183) perf(element): reuse one moved-element ID set when dragging dragSelectedElements passed a fresh Array.from(elementsToUpdate) to every element's updateBoundElements call. Each call missed the per-array ID-set cache and rebuilt a selection-sized set, so a drag step was quadratic in the number of dragged elements. Build the array once per drag step, pass it to every call, and reuse its cached set (getSimultaneouslyUpdatedElementIds, now exported) for the unbinding checks too. Document that callers must pass one array instance per operation and must not change its membership afterwards. Dragging 1K / 2K / 4K selected rectangles in the app went from 45 / 160 / 643 ms to 18.5 / 31 / 54 ms per frame (same tab, same window, dev build). Add coverage for the shared array, grouped labels across z-order, frames with their children, bound arrows, bindings, unbinding, and unchanged coordinates. Claude Opus 5.5 (1M context), reasoning effort 40 --- packages/element/src/binding.ts | 9 +- packages/element/src/dragElements.ts | 15 +- packages/element/tests/dragElements.test.ts | 228 ++++++++++++++++++++ 3 files changed, 247 insertions(+), 5 deletions(-) create mode 100644 packages/element/tests/dragElements.test.ts diff --git a/packages/element/src/binding.ts b/packages/element/src/binding.ts index 36e68dc29e..b0c6a56054 100644 --- a/packages/element/src/binding.ts +++ b/packages/element/src/binding.ts @@ -1322,6 +1322,7 @@ export const updateBoundElements = ( changedElement: NonDeletedExcalidrawElement, scene: Scene, options?: { + /** see `getSimultaneouslyUpdatedElementIds` for how to pass it */ simultaneouslyUpdated?: readonly NonDeletedExcalidrawElement[]; changedElements?: Map; }, @@ -1550,7 +1551,13 @@ const simultaneouslyUpdatedElementIdsCache = new WeakMap< ReadonlySet >(); -const getSimultaneouslyUpdatedElementIds = ( +/** + * The id set of `simultaneouslyUpdated`, shared by every `updateBoundElements()` + * call given the same array. Pass one array instance for all elements of an + * operation, and don't change its membership after the first lookup, or the + * cached ids go stale. + */ +export const getSimultaneouslyUpdatedElementIds = ( simultaneouslyUpdated: readonly ExcalidrawElement[] | undefined, ): ReadonlySet => { if (!simultaneouslyUpdated) { diff --git a/packages/element/src/dragElements.ts b/packages/element/src/dragElements.ts index a63f17624c..3f8c427b34 100644 --- a/packages/element/src/dragElements.ts +++ b/packages/element/src/dragElements.ts @@ -15,7 +15,11 @@ import type { import type { NonDeletedExcalidrawElement } from "@excalidraw/element/types"; -import { unbindBindingElement, updateBoundElements } from "./binding"; +import { + getSimultaneouslyUpdatedElementIds, + unbindBindingElement, + updateBoundElements, +} from "./binding"; import { getCommonBounds } from "./bounds"; import { getPerfectElementSize } from "./sizeHelpers"; import { getBoundTextElement } from "./textElement"; @@ -103,8 +107,11 @@ export const dragSelectedElements = ( gridSize, ); - const elementsToUpdateIds = new Set( - Array.from(elementsToUpdate, (el) => el.id), + // one array for the whole update, so that every element's bound-arrow + // update and the unbinding checks share its cached id set + const elementsToUpdateArray = Array.from(elementsToUpdate); + const elementsToUpdateIds = getSimultaneouslyUpdatedElementIds( + elementsToUpdateArray, ); elementsToUpdate.forEach((element) => { @@ -125,7 +132,7 @@ export const dragSelectedElements = ( ); } updateBoundElements(element, scene, { - simultaneouslyUpdated: Array.from(elementsToUpdate), + simultaneouslyUpdated: elementsToUpdateArray, }); } else if ( // NOTE: Add a little initial drag to the arrow dragging when the arrow diff --git a/packages/element/tests/dragElements.test.ts b/packages/element/tests/dragElements.test.ts new file mode 100644 index 0000000000..55d3fe0477 --- /dev/null +++ b/packages/element/tests/dragElements.test.ts @@ -0,0 +1,228 @@ +import { API } from "@excalidraw/excalidraw/tests/helpers/api"; +import { pointFrom } from "@excalidraw/math"; + +import type { PointerDownState } from "@excalidraw/excalidraw/types"; +import type { LocalPoint } from "@excalidraw/math"; + +import { Scene } from "../src/Scene"; +import { updateBoundElements } from "../src/binding"; +import { dragSelectedElements } from "../src/dragElements"; +import { syncInvalidIndices } from "../src/fractionalIndex"; +import { LinearElementEditor } from "../src/linearElementEditor"; + +import type { NonDeletedExcalidrawElement } from "../src/types"; + +vi.mock("../src/binding", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + updateBoundElements: vi.fn(actual.updateBoundElements), + }; +}); + +const pointerDownStateFor = (scene: Scene): PointerDownState => { + const state: Pick = { + originalElements: new Map( + scene + .getNonDeletedElements() + .map((element) => [element.id, { ...element }]), + ), + }; + return state as PointerDownState; +}; + +const drag = ( + scene: Scene, + selectedElements: NonDeletedExcalidrawElement[], + offset = { x: 20, y: 30 }, + pointerDownState = pointerDownStateFor(scene), +) => { + dragSelectedElements( + pointerDownState, + selectedElements, + offset, + scene, + { x: 0, y: 0 }, + null, + ); +}; + +const createBoundArrow = () => { + const rectangle = API.createElement({ + id: "rectangle", + boundElements: [{ id: "arrow", type: "arrow" }], + }); + const arrow = API.createElement({ + type: "arrow", + id: "arrow", + x: 25, + y: 25, + width: 200, + height: 0, + points: [pointFrom(0, 0), pointFrom(200, 0)], + startBinding: { + elementId: rectangle.id, + fixedPoint: [0.25, 0.25], + mode: "inside", + }, + }); + const scene = new Scene(syncInvalidIndices([rectangle, arrow])); + return { scene, rectangle, arrow }; +}; + +describe("dragSelectedElements", () => { + afterEach(() => { + vi.mocked(updateBoundElements).mockClear(); + }); + + it("passes every bound-arrow update the same moved-element array", () => { + const selected = [0, 1, 2].map((i) => + API.createElement({ x: i * 200, y: 0 }), + ); + const scene = new Scene(syncInvalidIndices(selected)); + + drag(scene, selected); + + const calls = vi.mocked(updateBoundElements).mock.calls; + expect(calls).toHaveLength(3); + // a fresh array per call misses the per-array id-set cache and rebuilds + // a selection-sized set for every element + const [[, , firstOptions]] = calls; + expect(firstOptions?.simultaneouslyUpdated).toEqual(selected); + for (const [, , options] of calls) { + expect(options?.simultaneouslyUpdated).toBe( + firstOptions?.simultaneouslyUpdated, + ); + } + }); + + it("moves grouped elements and their bound label across z-order", () => { + const rectangle = API.createElement({ + id: "rectangle", + groupIds: ["group"], + boundElements: [{ id: "label", type: "text" }], + }); + const label = API.createElement({ + type: "text", + id: "label", + containerId: rectangle.id, + x: 10, + y: 15, + width: 80, + height: 20, + groupIds: ["group"], + }); + const unrelated = API.createElement({ x: 500, y: 600 }); + const second = API.createElement({ + x: 200, + y: 300, + groupIds: ["group"], + }); + const scene = new Scene( + syncInvalidIndices([rectangle, label, unrelated, second]), + ); + const order = scene.getNonDeletedElements().map((element) => element.id); + + drag(scene, [rectangle, second]); + + expect(scene.getNonDeletedElements().map(({ x, y }) => ({ x, y }))).toEqual( + [ + { x: 20, y: 30 }, + { x: 30, y: 45 }, + { x: 500, y: 600 }, + { x: 220, y: 330 }, + ], + ); + expect(scene.getNonDeletedElements().map((element) => element.id)).toEqual( + order, + ); + }); + + it("moves a frame and its children once even when a child is also selected", () => { + const frame = API.createElement({ type: "frame", width: 400, height: 400 }); + const selectedChild = API.createElement({ x: 10, frameId: frame.id }); + const otherChild = API.createElement({ x: 200, frameId: frame.id }); + const elements = [selectedChild, otherChild, frame]; + const scene = new Scene(syncInvalidIndices(elements)); + const versions = elements.map((element) => element.version); + + drag(scene, [frame, selectedChild]); + + expect(elements.map(({ x, y }) => ({ x, y }))).toEqual([ + { x: 30, y: 40 }, + { x: 220, y: 230 }, + { x: 20, y: 30 }, + ]); + expect(elements.map((element) => element.version)).toEqual( + versions.map((version) => version + 1), + ); + }); + + it("updates an unselected bound arrow", () => { + const { scene, rectangle, arrow } = createBoundArrow(); + + drag(scene, [rectangle]); + + expect(rectangle).toMatchObject({ x: 20, y: 30 }); + expect( + LinearElementEditor.getPointAtIndexGlobalCoordinates( + arrow, + 0, + scene.getNonDeletedElementsMap(), + ), + ).toEqual([45, 55]); + expect( + LinearElementEditor.getPointAtIndexGlobalCoordinates( + arrow, + -1, + scene.getNonDeletedElementsMap(), + ), + ).toEqual([225, 25]); + expect(arrow.startBinding?.elementId).toBe(rectangle.id); + }); + + it("preserves bindings when translating an arrow with its attached element", () => { + const { scene, rectangle, arrow } = createBoundArrow(); + const arrowVersion = arrow.version; + const originalPoints = arrow.points; + + drag(scene, [rectangle, arrow]); + + expect(arrow).toMatchObject({ + x: 45, + y: 55, + startBinding: { elementId: rectangle.id }, + version: arrowVersion + 1, + }); + expect(arrow.points).toBe(originalPoints); + expect(rectangle.boundElements).toEqual([{ id: arrow.id, type: "arrow" }]); + }); + + it("unbinds a translated arrow from an unselected element", () => { + const { scene, rectangle, arrow } = createBoundArrow(); + + drag(scene, [arrow]); + + expect(arrow).toMatchObject({ x: 45, y: 55, startBinding: null }); + expect(rectangle).toMatchObject({ x: 0, y: 0, boundElements: [] }); + }); + + it("does not mutate for unchanged coordinates", () => { + const rectangle = API.createElement({}); + const second = API.createElement({ x: 200 }); + const scene = new Scene(syncInvalidIndices([rectangle, second])); + const originalState = pointerDownStateFor(scene); + const versions = () => [rectangle.version, second.version]; + const initial = versions(); + + drag(scene, [rectangle, second], { x: 0, y: 0 }, originalState); + expect(versions()).toEqual(initial); + + drag(scene, [rectangle, second], { x: 20, y: 30 }, originalState); + const moved = versions(); + expect(moved).toEqual(initial.map((version) => version + 1)); + + drag(scene, [rectangle, second], { x: 20, y: 30 }, originalState); + expect(versions()).toEqual(moved); + }); +});