[ENG-2149] Support dropping multiple images onto the canvas - #1312
Draft
mattakamatsu wants to merge 2 commits into
Draft
[ENG-2149] Support dropping multiple images onto the canvas#1312mattakamatsu wants to merge 2 commits into
mattakamatsu wants to merge 2 commits into
Conversation
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 <noreply@anthropic.com>
…mage'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: `` 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 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes ENG-2149.
What was wrong
tldraw splits a file drop across two layers. A content handler for
"files"owns iterating the dropped files, enforcing size and type limits, and placing the shapes. An asset store owns turning one file into one stored asset, meaning where the bytes go.The canvas overrode the content handler, and that override read
content.files[0]and nothing else. Every file after the first was discarded. It also ignoredcontent.point, so the one image it did create landed at the centre of the viewport instead of where you dropped it.The override existed for a real reason: canvas media has to go to Roam's file store, not be inlined as base64 into the page's block props. But that is an asset-store concern implemented one layer too high, and it took multi-file handling down with it.
What changed
The Roam upload moves down to the asset store, and the content-handler override is deleted.
apps/roam/src/utils/roamCanvasAssetStore.tsis the shared asset store.With the override gone, tldraw's own handler takes back over and the canvas matches tldraw.com.
Where images live: unchanged
Before and after, media is uploaded through
window.roamAlphaAPI.file.uploadinto Roam's Firebase-backed file store atimgs/app/<graph>/<id>.<ext>. The canvas stores only the URL, in the tldraw asset record'sprops.src. Image bytes never enter block props. Same API call, same destination, same field. Only the layer that makes the call changed.Existing canvases are unaffected and no migration is involved. Only new uploads take the new code path.
Reviewer notes
Asset IDs now hash file content rather than the upload URL. This is tldraw's default and the one behavioural change worth a second opinion. Identical images now collapse to one asset record. Measured: dropping the same image three times gives 3 uploads, 1 asset record, 3 correctly-rendering shapes, and therefore 2 orphaned files in Roam. Context: the plugin never calls
file.deleteanywhere, so deleting an image shape already orphaned its upload. Reclaiming unreferenced uploads is separate work and deliberately not in here.file.uploadreturns Roam markup, not a URL, and the wrapper depends on file type.for images,{{[[video]]: url}}for video,{{[[audio]]: …}},{{[[pdf]]: …}},[name](url)otherwise. The parser inherited from the sync adapter stripped the image wrapper specifically, so the first real video dropped crashed the canvas with a schema error. It now extracts the URL from whatever wrapper came back. This was already latent in the sync adapter before this PR; it was simply unreachable, because the deleted handler rejected video before the asset store ever saw it.A bad
srcused to take down the whole canvas. It fails insidestore.put, which is past thetry/catchin tldraw's file handler, so the error boundary caught it and offered "Reset data" for the entire canvas. The asset store now validates before returning, which turns it into an "Upload failed" toast for that one file while the rest of the drop still lands.Smaller consequences of using tldraw's defaults: accepted image types widen to include apng and avif; failures raise a toast instead of a silent
console.error; the toolbar's Media insert button was broken the same way and is fixed by the same change, since its picker already setsmultiple = true; andCanvas: Asset Addednow fires once per file instead of once per drop, with the same event name andsourcevalue.Test plan
Automated:
tsc --noEmitclean, eslint 0 errors, extension builds.Against real tldraw 2.4.6 in a harness driving genuine
dropevents:files[0]handler re-registered: 1 upload, 1 shape. Confirms the deleted code was the cause.In a live Roam graph:
plugin-testing-akamatsulab2. The mp4 crash above was found this way and is fixed.Still worth covering before merge: persistence across a page reload, and the same drop on a sync-mode canvas.
Scope check
$scope-checkagainst ENG-2149 and the final diff.Done When: The ticket has noDone Whensection, so the boundary was read from its one-line description, "drag multiple images into the canvas, as tldraw.com does". Beyond that: (1) video files now import as video shapes, where they were previously rejected with a silentconsole.error; (2) accepted image types widen to tldraw's defaults, adding apng and avif; (3) upload failures now surface as toasts."files"handler, which is the fix itself rather than an addition to it. Keeping the previous behaviour would mean re-implementing the handler's iteration and placement logic in order to narrow it again, which reintroduces the class of bug this PR removes. Video also forced the{{[[video]]: url}}parse fix and the crash-containment work, both of which are needed regardless of whether video stays.acceptedVideoMimeTypesfrom the handler config, and dropped videos get a clean "type not allowed" toast.🤖 Generated with Claude Code