From 06c5ea94d331b9c42f922ea7ea6c40f22a9cfa8c Mon Sep 17 00:00:00 2001 From: ericvannunen <106070823+ericvannunen@users.noreply.github.com> Date: Tue, 23 Sep 2025 23:47:03 +0200 Subject: [PATCH] fix: Race conditions when adding many library items (#10013) * Fix for race condition when adding many library items * Remove unused import * Replace any with LibraryItem type * Fix comments on pr * Fix build errors * Fix hoisted variable * new mime type * duplicate before passing down to be sure * lint * fix tests * Remove unused import --------- Co-authored-by: dwelle <5153846+dwelle@users.noreply.github.com> --- packages/common/src/constants.ts | 3 + packages/excalidraw/components/App.tsx | 48 ++++-- .../components/LibraryMenuItems.tsx | 14 +- packages/excalidraw/data/library.ts | 1 + packages/excalidraw/data/types.ts | 5 + packages/excalidraw/tests/library.test.tsx | 151 +++++++++++------- 6 files changed, 147 insertions(+), 75 deletions(-) diff --git a/packages/common/src/constants.ts b/packages/common/src/constants.ts index b41fb1a37d..a02ccfbb46 100644 --- a/packages/common/src/constants.ts +++ b/packages/common/src/constants.ts @@ -266,7 +266,10 @@ export const STRING_MIME_TYPES = { json: "application/json", // excalidraw data excalidraw: "application/vnd.excalidraw+json", + // LEGACY: fully-qualified library JSON data excalidrawlib: "application/vnd.excalidrawlib+json", + // list of excalidraw library item ids + excalidrawlibIds: "application/vnd.excalidrawlib.ids+json", } as const; export const MIME_TYPES = { diff --git a/packages/excalidraw/components/App.tsx b/packages/excalidraw/components/App.tsx index 0f6fa840b0..2def157b4a 100644 --- a/packages/excalidraw/components/App.tsx +++ b/packages/excalidraw/components/App.tsx @@ -433,6 +433,8 @@ import { findShapeByKey } from "./shapes"; import UnlockPopup from "./UnlockPopup"; +import type { ExcalidrawLibraryIds } from "../data/types"; + import type { RenderInteractiveSceneCallback, ScrollBars, @@ -10545,16 +10547,44 @@ class App extends React.Component { if (imageFiles.length > 0 && this.isToolSupported("image")) { return this.insertImages(imageFiles, sceneX, sceneY); } - - const libraryJSON = dataTransferList.getData(MIME_TYPES.excalidrawlib); - if (libraryJSON && typeof libraryJSON === "string") { + const excalidrawLibrary_ids = dataTransferList.getData( + MIME_TYPES.excalidrawlibIds, + ); + const excalidrawLibrary_data = dataTransferList.getData( + MIME_TYPES.excalidrawlib, + ); + if (excalidrawLibrary_ids || excalidrawLibrary_data) { try { - const libraryItems = parseLibraryJSON(libraryJSON); - this.addElementsFromPasteOrLibrary({ - elements: distributeLibraryItemsOnSquareGrid(libraryItems), - position: event, - files: null, - }); + let libraryItems: LibraryItems | null = null; + if (excalidrawLibrary_ids) { + const { itemIds } = JSON.parse( + excalidrawLibrary_ids, + ) as ExcalidrawLibraryIds; + const allLibraryItems = await this.library.getLatestLibrary(); + libraryItems = allLibraryItems.filter((item) => + itemIds.includes(item.id), + ); + // legacy library dataTransfer format + } else if (excalidrawLibrary_data) { + libraryItems = parseLibraryJSON(excalidrawLibrary_data); + } + if (libraryItems?.length) { + libraryItems = libraryItems.map((item) => ({ + ...item, + // #6465 + elements: duplicateElements({ + type: "everything", + elements: item.elements, + randomizeSeed: true, + }).duplicatedElements, + })); + + this.addElementsFromPasteOrLibrary({ + elements: distributeLibraryItemsOnSquareGrid(libraryItems), + position: event, + files: null, + }); + } } catch (error: any) { this.setState({ errorMessage: error.message }); } diff --git a/packages/excalidraw/components/LibraryMenuItems.tsx b/packages/excalidraw/components/LibraryMenuItems.tsx index 8e06632aad..eb82dde550 100644 --- a/packages/excalidraw/components/LibraryMenuItems.tsx +++ b/packages/excalidraw/components/LibraryMenuItems.tsx @@ -10,7 +10,6 @@ import { MIME_TYPES, arrayToMap } from "@excalidraw/common"; import { duplicateElements } from "@excalidraw/element"; -import { serializeLibraryAsJSON } from "../data/json"; import { useLibraryCache } from "../hooks/useLibraryItemSvg"; import { useScrollPosition } from "../hooks/useScrollPosition"; import { t } from "../i18n"; @@ -27,6 +26,8 @@ import Stack from "./Stack"; import "./LibraryMenuItems.scss"; +import type { ExcalidrawLibraryIds } from "../data/types"; + import type { ExcalidrawProps, LibraryItem, @@ -175,12 +176,17 @@ export default function LibraryMenuItems({ const onItemDrag = useCallback( (id: LibraryItem["id"], event: React.DragEvent) => { + // we want to serialize just the ids so the operation is fast and there's + // no race condition if people drop the library items on canvas too fast + const data: ExcalidrawLibraryIds = { + itemIds: selectedItems.includes(id) ? selectedItems : [id], + }; event.dataTransfer.setData( - MIME_TYPES.excalidrawlib, - serializeLibraryAsJSON(getInsertedElements(id)), + MIME_TYPES.excalidrawlibIds, + JSON.stringify(data), ); }, - [getInsertedElements], + [selectedItems], ); const isItemSelected = useCallback( diff --git a/packages/excalidraw/data/library.ts b/packages/excalidraw/data/library.ts index 4269edbd7f..429ba1046c 100644 --- a/packages/excalidraw/data/library.ts +++ b/packages/excalidraw/data/library.ts @@ -192,6 +192,7 @@ const createLibraryUpdate = ( class Library { /** latest libraryItems */ private currLibraryItems: LibraryItems = []; + /** snapshot of library items since last onLibraryChange call */ private prevLibraryItems = cloneLibraryItems(this.currLibraryItems); diff --git a/packages/excalidraw/data/types.ts b/packages/excalidraw/data/types.ts index 6878b81b18..94947c2b98 100644 --- a/packages/excalidraw/data/types.ts +++ b/packages/excalidraw/data/types.ts @@ -6,6 +6,7 @@ import type { cleanAppStateForExport } from "../appState"; import type { AppState, BinaryFiles, + LibraryItem, LibraryItems, LibraryItems_anyVersion, } from "../types"; @@ -59,3 +60,7 @@ export interface ImportedLibraryData extends Partial { /** @deprecated v1 */ library?: LibraryItems; } + +export type ExcalidrawLibraryIds = { + itemIds: LibraryItem["id"][]; +}; diff --git a/packages/excalidraw/tests/library.test.tsx b/packages/excalidraw/tests/library.test.tsx index f1c0f0a457..55c25188de 100644 --- a/packages/excalidraw/tests/library.test.tsx +++ b/packages/excalidraw/tests/library.test.tsx @@ -15,7 +15,7 @@ import { Excalidraw } from "../index"; import { API } from "./helpers/api"; import { UI } from "./helpers/ui"; -import { fireEvent, getCloneByOrigId, render, waitFor } from "./test-utils"; +import { fireEvent, render, waitFor } from "./test-utils"; import type { LibraryItem, LibraryItems } from "../types"; @@ -46,52 +46,8 @@ vi.mock("../data/filesystem.ts", async (importOriginal) => { }; }); -describe("library", () => { +describe("library items inserting", () => { beforeEach(async () => { - await render(); - await act(() => { - return h.app.library.resetLibrary(); - }); - }); - - it("import library via drag&drop", async () => { - expect(await h.app.library.getLatestLibrary()).toEqual([]); - await API.drop([ - { - kind: "file", - type: MIME_TYPES.excalidrawlib, - file: await API.loadFile("./fixtures/fixture_library.excalidrawlib"), - }, - ]); - await waitFor(async () => { - expect(await h.app.library.getLatestLibrary()).toEqual([ - { - status: "unpublished", - elements: [expect.objectContaining({ id: "A" })], - id: "id0", - created: expect.any(Number), - }, - ]); - }); - }); - - // NOTE: mocked to test logic, not actual drag&drop via UI - it("drop library item onto canvas", async () => { - expect(h.elements).toEqual([]); - const libraryItems = parseLibraryJSON(await libraryJSONPromise); - await API.drop([ - { - kind: "string", - value: serializeLibraryAsJSON(libraryItems), - type: MIME_TYPES.excalidrawlib, - }, - ]); - await waitFor(() => { - expect(h.elements).toEqual([expect.objectContaining({ [ORIG_ID]: "A" })]); - }); - }); - - it("should regenerate ids but retain bindings on library insert", async () => { const rectangle = API.createElement({ id: "rectangle1", type: "rectangle", @@ -117,45 +73,116 @@ describe("library", () => { }, }); + const libraryItems: LibraryItems = [ + { + id: "libraryItem_id0", + status: "unpublished", + elements: [rectangle, text, arrow], + created: 0, + name: "test", + }, + ]; + + await render(); + }); + + afterEach(async () => { + await act(() => { + return h.app.library.resetLibrary(); + }); + }); + + it("should regenerate ids but retain bindings on library insert", async () => { + const libraryItems = await h.app.library.getLatestLibrary(); + + expect(libraryItems.length).toBe(1); + await API.drop([ { kind: "string", - value: serializeLibraryAsJSON([ - { - id: "item1", - status: "published", - elements: [rectangle, text, arrow], - created: 1, - }, - ]), - type: MIME_TYPES.excalidrawlib, + value: JSON.stringify({ + itemIds: [libraryItems[0].id], + }), + type: MIME_TYPES.excalidrawlibIds, }, ]); await waitFor(() => { + const rectangle = h.elements.find((e) => e.type === "rectangle")!; + const text = h.elements.find((e) => e.type === "text")!; + const arrow = h.elements.find((e) => e.type === "arrow")!; expect(h.elements).toEqual( expect.arrayContaining([ expect.objectContaining({ - [ORIG_ID]: "rectangle1", + type: "rectangle", + id: expect.not.stringMatching("rectangle1"), boundElements: expect.arrayContaining([ - { type: "text", id: getCloneByOrigId("text1").id }, - { type: "arrow", id: getCloneByOrigId("arrow1").id }, + { type: "text", id: text.id }, + { type: "arrow", id: arrow.id }, ]), }), expect.objectContaining({ - [ORIG_ID]: "text1", - containerId: getCloneByOrigId("rectangle1").id, + type: "text", + id: expect.not.stringMatching("text1"), + containerId: rectangle.id, }), expect.objectContaining({ - [ORIG_ID]: "arrow1", + type: "arrow", + id: expect.not.stringMatching("arrow1"), endBinding: expect.objectContaining({ - elementId: getCloneByOrigId("rectangle1").id, + elementId: rectangle.id, }), }), ]), ); }); }); +}); + +describe("library", () => { + beforeEach(async () => { + await render(); + await act(() => { + return h.app.library.resetLibrary(); + }); + }); + + it("import library via drag&drop", async () => { + expect(await h.app.library.getLatestLibrary()).toEqual([]); + await API.drop([ + { + kind: "file", + type: MIME_TYPES.excalidrawlib, + file: await API.loadFile("./fixtures/fixture_library.excalidrawlib"), + }, + ]); + await waitFor(async () => { + expect(await h.app.library.getLatestLibrary()).toEqual([ + { + status: "unpublished", + elements: [expect.objectContaining({ id: "A" })], + id: expect.any(String), + created: expect.any(Number), + }, + ]); + }); + }); + + // NOTE: mocked to test logic, not actual drag&drop via UI + it("drop library item onto canvas", async () => { + expect(h.elements).toEqual([]); + const libraryItems = parseLibraryJSON(await libraryJSONPromise); + await API.drop([ + { + kind: "string", + value: serializeLibraryAsJSON(libraryItems), + type: MIME_TYPES.excalidrawlib, + }, + ]); + await waitFor(() => { + expect(h.elements).toEqual([expect.objectContaining({ [ORIG_ID]: "A" })]); + }); + }); it("should fix duplicate ids between items on insert", async () => { // note, we're not testing for duplicate group ids and such because