From 088968b9f5244e4db9418f851201d3810060c22d Mon Sep 17 00:00:00 2001 From: mattakamatsu Date: Tue, 18 Aug 2026 18:10:46 -0700 Subject: [PATCH 1/2] [ENG-2149] Support dropping multiple images onto the canvas Dropping several images at once only ever created one shape. The canvas overrode tldraw's "files" external-content handler, which is the layer that owns iterating a multi-file drop, and that override read only content.files[0]. It also ignored content.point, so the one image it did create landed at the viewport centre rather than where it was dropped. The only thing the canvas actually needs to customize is where media is stored: Roam's file store rather than base64 inlined into the page's block props. That belongs in the store's asset store, one layer down, so move it there and delete the content-handler override. The Cloudflare sync adapter already had exactly this asset store, so share it, and give the local block-props store one too - it had none, which is why the override existed in the first place. With the override gone, tldraw's own handler takes back over and the canvas matches tldraw.com: every dropped file is uploaded, the shapes are tiled in a row centred on the drop point and left selected, and oversized or unsupported files raise a toast instead of a silent console.error. Accepted image types widen slightly to tldraw's defaults (apng and avif join the existing list). Verified against tldraw 2.4.6 in a harness driving a real three-file drop event: three uploads, three image shapes tiled and centred on the drop point. Re-registering the old files[0] handler in the same harness reproduces the bug - one upload, one shape. Co-Authored-By: Claude Opus 5 --- apps/roam/src/components/canvas/Tldraw.tsx | 70 +---------- .../canvas/TldrawCanvasCloudflareSync.tsx | 22 +--- .../components/canvas/canvasAssetTelemetry.ts | 14 +++ .../src/components/canvas/useRoamStore.ts | 5 + .../__tests__/roamCanvasAssetStore.test.ts | 115 ++++++++++++++++++ apps/roam/src/utils/roamCanvasAssetStore.ts | 37 ++++++ 6 files changed, 179 insertions(+), 84 deletions(-) create mode 100644 apps/roam/src/components/canvas/canvasAssetTelemetry.ts create mode 100644 apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts create mode 100644 apps/roam/src/utils/roamCanvasAssetStore.ts diff --git a/apps/roam/src/components/canvas/Tldraw.tsx b/apps/roam/src/components/canvas/Tldraw.tsx index 505833d02..6c1d8d5bf 100644 --- a/apps/roam/src/components/canvas/Tldraw.tsx +++ b/apps/roam/src/components/canvas/Tldraw.tsx @@ -31,7 +31,6 @@ import { createShapeId, TLPointerEventInfo, TLExternalContent, - MediaHelpers, AssetRecordType, TLAsset, TLAssetId, @@ -117,6 +116,7 @@ import { } from "./useCanvasStoreAdapterArgs"; import { shouldCreateAutoCanvasRelations } from "./autoCanvasRelationsSuppression"; import posthog from "posthog-js"; +import { parseRoamUploadResponse } from "~/utils/roamCanvasAssetStore"; import { getPersonalSetting } from "~/components/settings/utils/accessors"; import { PERSONAL_KEYS } from "~/components/settings/utils/settingKeys"; import { json, normalizeProps } from "~/utils/getBlockProps"; @@ -1541,16 +1541,6 @@ const InsideEditorAndUiContext = ({ ); useEffect(() => { - // https://tldraw.dev/examples/data/assets/hosted-images - const ACCEPTED_IMG_TYPE = [ - "image/jpeg", - "image/png", - "image/gif", - "image/svg+xml", - "image/webp", - ]; - const isImage = (ext: string) => ACCEPTED_IMG_TYPE.includes(ext); - // Register default handlers for images and videos registerDefaultExternalContentHandlers( editor, @@ -1638,62 +1628,6 @@ const InsideEditorAndUiContext = ({ }; editor.registerExternalContentHandler("text", textHandler); - editor.registerExternalContentHandler( - "files", - // eslint-disable-next-line @typescript-eslint/no-misused-promises - async (content: TLExternalContent) => { - if (content.type !== "files") { - console.error("Expected files, received:", content.type); - return; - } - const file = content.files[0]; - - const url = await window.roamAlphaAPI.file.upload({ file }); - const dataUrl = url.replace(/^!\[\]\(/, "").replace(/\)$/, ""); - // TODO add video support - const isImageType = isImage(file.type); - if (!isImageType) { - console.error("Unsupported file type:", file.type); - return; - } - const size = await MediaHelpers.getImageSize(file); - const isAnimated = await MediaHelpers.isAnimated(file); - const assetId: TLAssetId = AssetRecordType.createId( - getHashForString(dataUrl), - ); - const shapeType = isImageType ? "image" : "video"; - const asset: TLAsset = AssetRecordType.create({ - id: assetId, - type: shapeType, - typeName: "asset", - props: { - name: file.name, - src: dataUrl, - w: size.w, - h: size.h, - ...fileSizeProps(getValidFileSize(file)), - mimeType: file.type, - isAnimated, - }, - }); - editor.createAssets([asset]); - - const position = editor.getViewportPageBounds().center; - - editor.createShape({ - type: "image", - x: position.x - size.w / 2, - y: position.y - size.h / 2, - props: { assetId, w: size.w, h: size.h }, - }); - posthog.capture("Canvas: Asset Added", { - source: "file-drop", - mimeType: file.type, - }); - - return asset; - }, - ); //https://github.com/tldraw/tldraw/blob/v2.3.x/packages/tldraw/src/lib/defaultExternalContentHandlers.ts#L183 editor.registerExternalContentHandler( "svg-text", @@ -1732,7 +1666,7 @@ const InsideEditorAndUiContext = ({ }); const url = await window.roamAlphaAPI.file.upload({ file }); - const dataUrl = url.replace(/^!\[\]\(/, "").replace(/\)$/, ""); + const dataUrl = parseRoamUploadResponse(url); const assetId: TLAssetId = AssetRecordType.createId( getHashForString(dataUrl), diff --git a/apps/roam/src/components/canvas/TldrawCanvasCloudflareSync.tsx b/apps/roam/src/components/canvas/TldrawCanvasCloudflareSync.tsx index 3bddf46ae..8069f7857 100644 --- a/apps/roam/src/components/canvas/TldrawCanvasCloudflareSync.tsx +++ b/apps/roam/src/components/canvas/TldrawCanvasCloudflareSync.tsx @@ -2,7 +2,6 @@ import { useSync } from "@tldraw/sync"; import { TLAnyBindingUtilConstructor, TLAnyShapeUtilConstructor, - TLAssetStore, TLStoreWithStatus, defaultBindingUtils, defaultShapeUtils, @@ -10,6 +9,8 @@ import { } from "tldraw"; import { useMemo } from "react"; import { getCurrentRoamTldrawUserInfo } from "~/utils/roamTldrawUserInfo"; +import { createRoamAssetStore } from "~/utils/roamCanvasAssetStore"; +import { captureCanvasAssetUploaded } from "./canvasAssetTelemetry"; /** Base URL for tldraw-sync-cloudflare worker. Use https (not wss) - useSync upgrades to WebSocket. */ export const TLDRAW_CLOUDFLARE_SYNC_WS_BASE_URL = @@ -37,20 +38,6 @@ export const getSyncRoomId = ({ pageUid }: { pageUid: string }): string => { .replace(/=+$/g, ""); }; -const parseRoamUploadResponse = (value: string): string => { - return value.replace(/^!\[\]\(/, "").replace(/\)$/, ""); -}; - -const createRoamAssetStore = (): TLAssetStore => { - return { - upload: async (_asset, file) => { - const response = await window.roamAlphaAPI.file.upload({ file }); - return parseRoamUploadResponse(response); - }, - resolve: (asset) => asset.props.src, - }; -}; - export const useCloudflareSyncStore = ({ pageUid, migrations, @@ -66,7 +53,10 @@ export const useCloudflareSyncStore = ({ customShapeTypes: string[]; customBindingTypes: string[]; }): CloudflareCanvasStoreAdapterResult => { - const assets = useMemo(() => createRoamAssetStore(), []); + const assets = useMemo( + () => createRoamAssetStore({ onUpload: captureCanvasAssetUploaded }), + [], + ); const shapeUtils = useMemo( () => [...defaultShapeUtils, ...customShapeUtils], [customShapeUtils], diff --git a/apps/roam/src/components/canvas/canvasAssetTelemetry.ts b/apps/roam/src/components/canvas/canvasAssetTelemetry.ts new file mode 100644 index 000000000..86fe8fccc --- /dev/null +++ b/apps/roam/src/components/canvas/canvasAssetTelemetry.ts @@ -0,0 +1,14 @@ +import posthog from "posthog-js"; + +/** + * Fired once per file uploaded to the canvas. Both canvas store adapters (local + * block-props and Cloudflare sync) route their uploads through the same asset + * store, so this is the single place assets are counted. The "file-drop" source + * covers drops and pastes alike, matching what this event has always reported. + */ +export const captureCanvasAssetUploaded = ({ file }: { file: File }): void => { + posthog.capture("Canvas: Asset Added", { + source: "file-drop", + mimeType: file.type, + }); +}; diff --git a/apps/roam/src/components/canvas/useRoamStore.ts b/apps/roam/src/components/canvas/useRoamStore.ts index 78989afaa..f00152704 100644 --- a/apps/roam/src/components/canvas/useRoamStore.ts +++ b/apps/roam/src/components/canvas/useRoamStore.ts @@ -26,6 +26,8 @@ import { import { AddPullWatch } from "roamjs-components/types"; import { LEGACY_SCHEMA } from "~/data/legacyTldrawSchema"; import internalError from "~/utils/internalError"; +import { createRoamAssetStore } from "~/utils/roamCanvasAssetStore"; +import { captureCanvasAssetUploaded } from "./canvasAssetTelemetry"; const THROTTLE = 350; @@ -93,6 +95,9 @@ const createCanvasStore = ({ migrations, shapeUtils: [...defaultShapeUtils, ...customShapeUtils], bindingUtils: [...defaultBindingUtils, ...customBindingUtils], + // Without this, tldraw inlines dropped media as base64 into the shape's + // asset, which we then persist into the page's block props. + assets: createRoamAssetStore({ onUpload: captureCanvasAssetUploaded }), }); const getPersistedRoamCanvasState = ({ diff --git a/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts b/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts new file mode 100644 index 000000000..ea33a927d --- /dev/null +++ b/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it, vi } from "vitest"; +import { + createRoamAssetStore, + parseRoamUploadResponse, +} from "~/utils/roamCanvasAssetStore"; + +const setRoamAlphaAPI = (roamAlphaAPI: unknown): void => { + (globalThis as { window: unknown }).window = { roamAlphaAPI }; +}; + +const createUploadSpy = (urls: string[]) => { + let call = 0; + return vi.fn(() => Promise.resolve(urls[call++] ?? urls[urls.length - 1])); +}; + +const fakeFile = (name: string, type = "image/png"): File => + ({ name, type, size: 1024 }) as unknown as File; + +describe("parseRoamUploadResponse", () => { + it("unwraps the markdown image Roam returns from file.upload", () => { + expect( + parseRoamUploadResponse( + "![](https://firebasestorage.googleapis.com/v0/b/x/o/imgs%2Fapp%2Fg%2Fa.png?alt=media)", + ), + ).toBe( + "https://firebasestorage.googleapis.com/v0/b/x/o/imgs%2Fapp%2Fg%2Fa.png?alt=media", + ); + }); + + it("leaves a bare url untouched", () => { + expect(parseRoamUploadResponse("https://example.com/a.png")).toBe( + "https://example.com/a.png", + ); + }); +}); + +describe("createRoamAssetStore", () => { + it("uploads a file to Roam and returns the bare url", async () => { + const upload = createUploadSpy(["![](https://example.com/a.png)"]); + setRoamAlphaAPI({ file: { upload } }); + + const store = createRoamAssetStore(); + const file = fakeFile("a.png"); + + await expect(store.upload({} as never, file)).resolves.toBe( + "https://example.com/a.png", + ); + expect(upload).toHaveBeenCalledWith({ file }); + }); + + // ENG-2149: dropping several images at once must upload every one of them. + // tldraw's default "files" content handler calls the asset store once per + // file, so the store has to stay stateless and per-file. + it("uploads every file of a multi-file drop to its own url", async () => { + const upload = createUploadSpy([ + "![](https://example.com/a.png)", + "![](https://example.com/b.png)", + "![](https://example.com/c.png)", + ]); + setRoamAlphaAPI({ file: { upload } }); + + const store = createRoamAssetStore(); + const files = [fakeFile("a.png"), fakeFile("b.png"), fakeFile("c.png")]; + + const srcs = await Promise.all( + files.map((file) => store.upload({} as never, file)), + ); + + expect(srcs).toEqual([ + "https://example.com/a.png", + "https://example.com/b.png", + "https://example.com/c.png", + ]); + expect(upload).toHaveBeenCalledTimes(3); + }); + + it("reports each upload to the onUpload callback", async () => { + const upload = createUploadSpy(["![](https://example.com/a.png)"]); + setRoamAlphaAPI({ file: { upload } }); + + const onUpload = vi.fn(); + const store = createRoamAssetStore({ onUpload }); + const file = fakeFile("a.gif", "image/gif"); + + await store.upload({} as never, file); + + expect(onUpload).toHaveBeenCalledWith({ file }); + }); + + it("does not fail the upload when the telemetry callback throws", async () => { + const upload = createUploadSpy(["![](https://example.com/a.png)"]); + setRoamAlphaAPI({ file: { upload } }); + + const store = createRoamAssetStore({ + onUpload: () => { + throw new Error("posthog exploded"); + }, + }); + + await expect(store.upload({} as never, fakeFile("a.png"))).resolves.toBe( + "https://example.com/a.png", + ); + }); + + it("resolves an asset to the url stored in its props", () => { + const store = createRoamAssetStore(); + const asset = { + props: { src: "https://example.com/a.png" }, + } as never; + + expect(store.resolve?.(asset, {} as never)).toBe( + "https://example.com/a.png", + ); + }); +}); diff --git a/apps/roam/src/utils/roamCanvasAssetStore.ts b/apps/roam/src/utils/roamCanvasAssetStore.ts new file mode 100644 index 000000000..91af979b7 --- /dev/null +++ b/apps/roam/src/utils/roamCanvasAssetStore.ts @@ -0,0 +1,37 @@ +import type { TLAssetStore } from "tldraw"; + +/** + * `roamAlphaAPI.file.upload` resolves to a markdown image (`![](url)`), but the + * canvas needs the bare url to put in `asset.props.src`. + */ +export const parseRoamUploadResponse = (value: string): string => + value.replace(/^!\[\]\(/, "").replace(/\)$/, ""); + +/** + * The canvas's asset store: uploads canvas media to Roam's file store instead of + * inlining it as base64 (tldraw's default), which would bloat the page's block + * props. + * + * This is deliberately the *only* thing we customize about asset handling. Every + * caller of `editor.uploadAsset` funnels through here — tldraw's own external + * content handlers own the rest: iterating a multi-file drop, enforcing size and + * mime-type limits, and placing the resulting shapes. Overriding a layer above + * this is what made multi-image drops drop all but the first image (ENG-2149). + */ +export const createRoamAssetStore = ({ + onUpload, +}: { + /** Fired after each successful upload. Used for telemetry; never throws. */ + onUpload?: (args: { file: File }) => void; +} = {}): TLAssetStore => ({ + upload: async (_asset, file) => { + const response = await window.roamAlphaAPI.file.upload({ file }); + try { + onUpload?.({ file }); + } catch (error) { + console.error("Canvas asset upload telemetry failed", error); + } + return parseRoamUploadResponse(response); + }, + resolve: (asset) => asset.props.src, +}); From 50f5674e3c4bd9ca5eec1dfdf89911a13ca911e6 Mon Sep 17 00:00:00 2001 From: mattakamatsu Date: Tue, 18 Aug 2026 23:33:19 -0700 Subject: [PATCH 2/2] [ENG-2149] Read the url out of any Roam upload wrapper, not just an image's Dropping an .mp4 crashed the canvas with a schema ValidationError: At asset(type = video).props.src: Expected a valid url, got "{{[[video]]: https://firebasestorage.googleapis.com/...mp4?alt=media}}" roamAlphaAPI.file.upload does not resolve to a bare url. It resolves to the Roam markup that renders the file, and the wrapper depends on the file type: `![](url)` for an image, `{{[[video]]: url}}` for a video, and so on. We were stripping the image wrapper's punctuation specifically, so a video kept "{{[[video]]: " glued to the front of its src. Pull the url out of the wrapper instead of stripping any one wrapper. This was latent in the sync adapter's asset store before the previous commit, which is where that parser came from - video simply never reached it, because the handler it replaced rejected video outright. Also fail early when the response has no url in it at all. A bad src only fails later, inside store.put, which is past the point where tldraw's file handler can catch it, so the whole canvas goes down with an error boundary. Throwing in the asset store turns it into an "Upload failed" toast for that one file and lets the rest of the drop land. Verified in a harness against tldraw 2.4.6 with a real mp4: the video wrapper parses, and a 320x240 video shape and asset are created alongside an image in the same drop. With one file's upload returning prose instead of a url: one toast, no shape for that file, the other two files still imported, no error boundary. Co-Authored-By: Claude Opus 5 --- .../__tests__/roamCanvasAssetStore.test.ts | 56 +++++++++++++++++++ apps/roam/src/utils/roamCanvasAssetStore.ts | 28 ++++++++-- 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts b/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts index ea33a927d..3f229246e 100644 --- a/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts +++ b/apps/roam/src/utils/__tests__/roamCanvasAssetStore.test.ts @@ -27,6 +27,32 @@ describe("parseRoamUploadResponse", () => { ); }); + // Roam picks the wrapper by file type, not one wrapper for everything. + // A video comes back as a {{[[video]]}} render component, and treating that + // as an image left "{{[[video]]: " glued to the front of the url, which the + // tldraw schema rejected and which crashed the whole canvas. + it("unwraps the video render component Roam returns for a video", () => { + expect( + parseRoamUploadResponse( + "{{[[video]]: https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2Fg%2Fo0geRItw_H.mp4?alt=media&token=c254db91-ec06-4519-b43d-1beeef402758}}", + ), + ).toBe( + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2Fg%2Fo0geRItw_H.mp4?alt=media&token=c254db91-ec06-4519-b43d-1beeef402758", + ); + }); + + it("unwraps the other render components Roam uses per file type", () => { + expect(parseRoamUploadResponse("{{[[audio]]: https://x.test/a.mp3}}")).toBe( + "https://x.test/a.mp3", + ); + expect(parseRoamUploadResponse("{{[[pdf]]: https://x.test/a.pdf}}")).toBe( + "https://x.test/a.pdf", + ); + expect(parseRoamUploadResponse("[a.zip](https://x.test/a.zip)")).toBe( + "https://x.test/a.zip", + ); + }); + it("leaves a bare url untouched", () => { expect(parseRoamUploadResponse("https://example.com/a.png")).toBe( "https://example.com/a.png", @@ -34,6 +60,36 @@ describe("parseRoamUploadResponse", () => { }); }); +describe("createRoamAssetStore upload validation", () => { + // A src that isn't a url fails tldraw's schema inside store.put, which is + // outside the file handler's try/catch and takes the canvas down with an + // error boundary. Fail here instead, where it becomes a toast. + it("throws instead of returning a src the canvas schema will reject", async () => { + setRoamAlphaAPI({ + file: { upload: () => Promise.resolve("upload failed: quota exceeded") }, + }); + + await expect( + createRoamAssetStore().upload({} as never, fakeFile("a.png")), + ).rejects.toThrow(/could not find a url/i); + }); + + it("accepts the url out of any wrapper Roam used", async () => { + setRoamAlphaAPI({ + file: { + upload: () => Promise.resolve("{{[[video]]: https://x.test/a.mp4}}"), + }, + }); + + await expect( + createRoamAssetStore().upload( + {} as never, + fakeFile("a.mp4", "video/mp4"), + ), + ).resolves.toBe("https://x.test/a.mp4"); + }); +}); + describe("createRoamAssetStore", () => { it("uploads a file to Roam and returns the bare url", async () => { const upload = createUploadSpy(["![](https://example.com/a.png)"]); diff --git a/apps/roam/src/utils/roamCanvasAssetStore.ts b/apps/roam/src/utils/roamCanvasAssetStore.ts index 91af979b7..b5b55431b 100644 --- a/apps/roam/src/utils/roamCanvasAssetStore.ts +++ b/apps/roam/src/utils/roamCanvasAssetStore.ts @@ -1,11 +1,17 @@ import type { TLAssetStore } from "tldraw"; /** - * `roamAlphaAPI.file.upload` resolves to a markdown image (`![](url)`), but the - * canvas needs the bare url to put in `asset.props.src`. + * `roamAlphaAPI.file.upload` doesn't resolve to a bare url. It resolves to + * whatever Roam markup renders that file, and the markup depends on the file + * type: `![](url)` for an image, `{{[[video]]: url}}` for a video, + * `{{[[audio]]: url}}`, `{{[[pdf]]: url}}`, `[name](url)` for anything else. + * The canvas wants the url on its own, so pull it back out of the wrapper + * rather than stripping any one wrapper's punctuation. */ -export const parseRoamUploadResponse = (value: string): string => - value.replace(/^!\[\]\(/, "").replace(/\)$/, ""); +export const parseRoamUploadResponse = (value: string): string => { + const url = value.match(/https?:\/\/[^\s)}\]]+/)?.[0]; + return url ?? value.trim(); +}; /** * The canvas's asset store: uploads canvas media to Roam's file store instead of @@ -26,12 +32,24 @@ export const createRoamAssetStore = ({ } = {}): TLAssetStore => ({ upload: async (_asset, file) => { const response = await window.roamAlphaAPI.file.upload({ file }); + const src = parseRoamUploadResponse(response); + + // A src that isn't a url fails the tldraw schema later, inside store.put, + // which is past the point where the file handler can catch it — the canvas + // dies with an error boundary. Failing here turns it into a toast for that + // one file, and the rest of a multi-file drop still lands. + if (!/^https?:\/\//.test(src)) { + throw new Error( + `Could not find a url in Roam's upload response for ${file.name}: ${response}`, + ); + } + try { onUpload?.({ file }); } catch (error) { console.error("Canvas asset upload telemetry failed", error); } - return parseRoamUploadResponse(response); + return src; }, resolve: (asset) => asset.props.src, });