diff --git a/excalidraw-app/App.tsx b/excalidraw-app/App.tsx index 8ba4f5fe..56779e0a 100644 --- a/excalidraw-app/App.tsx +++ b/excalidraw-app/App.tsx @@ -545,7 +545,7 @@ const ExcalidrawWrapper = () => { const saveToPds = useCallback(() => { if (atprotoAuth && activeScene && excalidrawAPI) { scheduleAutosave( - excalidrawAPI.getSceneElements(), + excalidrawAPI.getSceneElementsIncludingDeleted(), excalidrawAPI.getAppState(), excalidrawAPI.getFiles(), ); diff --git a/excalidraw-app/data/atproto/activeScene.ts b/excalidraw-app/data/atproto/activeScene.ts index 572f9ff2..c99c9e90 100644 --- a/excalidraw-app/data/atproto/activeScene.ts +++ b/excalidraw-app/data/atproto/activeScene.ts @@ -108,6 +108,7 @@ export const isAtprotoLink = (link: string) => { }; type SaveArgs = { + /** Must include soft-deleted elements — see the `elements` doc in saveScene's options (scenes.ts). */ elements: readonly OrderedExcalidrawElement[]; appState: Partial; files: BinaryFiles; diff --git a/excalidraw-app/data/atproto/scenes.ts b/excalidraw-app/data/atproto/scenes.ts index 47c26a68..06b131d3 100644 --- a/excalidraw-app/data/atproto/scenes.ts +++ b/excalidraw-app/data/atproto/scenes.ts @@ -161,6 +161,15 @@ export const saveScene = async ( lastBlobCid, swapCid, }: { + /** + * Must include soft-deleted (tombstone) elements — i.e. the caller should + * pass `getSceneElementsIncludingDeleted()`, not `getSceneElements()`. The + * blob-CID dedupe above compares `serializeAsJSON` output across call + * sites (autosave vs. manual save vs. viewing snapshots), so passing a + * differently-filtered element set here produces a different CID and + * defeats the skip check with a redundant uploadBlob. Tombstones are also + * required for `reconcileElements` in the CAS merge path below. + */ elements: readonly OrderedExcalidrawElement[]; appState: Partial; files: BinaryFiles; diff --git a/excalidraw-app/data/atproto/viewingActions.ts b/excalidraw-app/data/atproto/viewingActions.ts index 38bc0309..c8534f7f 100644 --- a/excalidraw-app/data/atproto/viewingActions.ts +++ b/excalidraw-app/data/atproto/viewingActions.ts @@ -32,7 +32,7 @@ export const enterViewing = async ( // sketches if (!appJotaiStore.get(preViewSnapshotAtom)) { appJotaiStore.set(preViewSnapshotAtom, { - elements: excalidrawAPI.getSceneElements(), + elements: excalidrawAPI.getSceneElementsIncludingDeleted(), appState: excalidrawAPI.getAppState(), files: excalidrawAPI.getFiles(), activeScene: appJotaiStore.get(activeSceneAtom), @@ -117,7 +117,7 @@ export const saveACopy = async (excalidrawAPI: ExcalidrawImperativeAPI) => { } const saved = await saveScene(auth.agent, { - elements: excalidrawAPI.getSceneElements(), + elements: excalidrawAPI.getSceneElementsIncludingDeleted(), appState: excalidrawAPI.getAppState(), files: excalidrawAPI.getFiles(), name: `Copy of ${viewing.name}`, diff --git a/excalidraw-app/share/ShareDialog.tsx b/excalidraw-app/share/ShareDialog.tsx index c919ba93..2a2a4948 100644 --- a/excalidraw-app/share/ShareDialog.tsx +++ b/excalidraw-app/share/ShareDialog.tsx @@ -102,7 +102,7 @@ const SaveToPdsContent = ({ handleClose }: { handleClose: () => void }) => { try { trackEvent("share", "save to pds"); const saved = await saveScene(auth.agent, { - elements: excalidrawAPI.getSceneElements(), + elements: excalidrawAPI.getSceneElementsIncludingDeleted(), appState: excalidrawAPI.getAppState(), files: excalidrawAPI.getFiles(), name, diff --git a/excalidraw-app/tests/atprotoScenes.test.ts b/excalidraw-app/tests/atprotoScenes.test.ts index 8e8843c7..cbd249cc 100644 --- a/excalidraw-app/tests/atprotoScenes.test.ts +++ b/excalidraw-app/tests/atprotoScenes.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it, vi } from "vitest"; +import { serializeAsJSON } from "@excalidraw/excalidraw/data/json"; import { API } from "@excalidraw/excalidraw/tests/helpers/api"; +import { computeSceneBlobCid } from "../data/atproto/blobCid"; import { compressSceneJson } from "../data/atproto/compression"; import { saveScene } from "../data/atproto/scenes"; @@ -288,3 +290,93 @@ describe("saveScene compare-and-swap", () => { expect(saved?.cid).toBe("createcid"); }); }); + +describe("saveScene dedupe with tombstones", () => { + // Regression test: callers must pass elements INCLUDING deleted ones. The + // blob-CID dedupe check (scenes.ts) compares serializeAsJSON output across + // call sites — if one call site passes non-deleted-only elements while + // another (e.g. a later autosave) passes elements including deleted, the + // computed CIDs differ and the skip check never fires, causing a redundant + // uploadBlob. Tombstones are also required by reconcileElements on the CAS + // merge path. + it("skips the second uploadBlob when the same elements (incl. a tombstone) are saved twice", async () => { + const { agent, uploadBlob, putRecord, createRecord } = createFakeAgent(); + + const elementsIncludingDeleted = [ + API.createElement({ id: "visible1", index: "a1" as any }), + API.createElement({ + id: "deleted1", + index: "a2" as any, + isDeleted: true, + }), + ]; + + const first = await saveScene(agent, { + elements: elementsIncludingDeleted as any, + appState: {}, + files: {}, + name: "Test drawing", + }); + expect(first).not.toBeNull(); + expect(first?.blobCid).toMatch(/^bafkrei[a-z2-7]{52}$/); + + uploadBlob.mockClear(); + putRecord.mockClear(); + createRecord.mockClear(); + + // same elements (still including the tombstone), same rkey + lastBlobCid + // seeded from the first save's result — must be a no-op + const second = await saveScene(agent, { + elements: elementsIncludingDeleted as any, + appState: {}, + files: {}, + name: "Test drawing", + rkey: first!.rkey, + lastBlobCid: first!.blobCid, + }); + + expect(second).toBeNull(); + expect(uploadBlob).not.toHaveBeenCalled(); + expect(putRecord).not.toHaveBeenCalled(); + expect(createRecord).not.toHaveBeenCalled(); + }); + + it("computes a different blob CID when the tombstone is dropped", async () => { + const elementsIncludingDeleted = [ + API.createElement({ id: "visible1", index: "a1" as any }), + API.createElement({ + id: "deleted1", + index: "a2" as any, + isDeleted: true, + }), + ]; + const elementsExcludingDeleted = elementsIncludingDeleted.filter( + (el) => !el.isDeleted, + ); + + const jsonWithTombstone = serializeAsJSON( + elementsIncludingDeleted as any, + {} as any, + {}, + "database", + ); + const jsonWithoutTombstone = serializeAsJSON( + elementsExcludingDeleted as any, + {} as any, + {}, + "database", + ); + + const cidWithTombstone = await computeSceneBlobCid(jsonWithTombstone); + const cidWithoutTombstone = await computeSceneBlobCid( + jsonWithoutTombstone, + ); + + // proves why every saveScene call site must pass elements including + // deleted: a caller that filters tombstones out produces a different CID, + // which defeats the dedupe check against a call site that doesn't filter + expect(cidWithTombstone).not.toBeNull(); + expect(cidWithoutTombstone).not.toBeNull(); + expect(cidWithTombstone).not.toBe(cidWithoutTombstone); + }); +}); diff --git a/lexicons/app/lexidraw/scene.json b/lexicons/app/lexidraw/scene.json index f4d0ae57..74c5cc22 100644 --- a/lexicons/app/lexidraw/scene.json +++ b/lexicons/app/lexidraw/scene.json @@ -4,7 +4,7 @@ "defs": { "main": { "type": "record", - "description": "An Excalidraw drawing. The full scene (elements, appState and embedded image files as dataURLs) is stored as a JSON blob", + "description": "An Excalidraw drawing. The full scene (elements, appState and embedded image files as dataURLs) is stored as a gzipped JSON blob", "key": "tid", "record": { "type": "object",