Add /import?src=<url>: a hand-off point for scanning apps - #720
Add /import?src=<url>: a hand-off point for scanning apps#720alxbouchard wants to merge 8 commits into
Conversation
|
Follow-up commit: while testing the import end to end I found that |
|
Addressed the Bugbot review (it ran against the first commit, 53b688c):
|
|
Third Bugbot point addressed: |
|
Double-tap point addressed: a synchronous |
|
Fifth point addressed: a failed create no longer unmounts the review — the validated graph stays on screen with the error shown inline and the button relabelled "Try again", so a short-lived |
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for this, and genuinely thanks for how you've handled the review rounds — the escalation from "import drops materials" to "validateBuildJson drops materials on the Load Build path too" is a real bug you found for us, and it's the strongest part of the PR. I confirmed it at main: handleConfirmImport passes only installedPlugins to setScene.
A few things before this can land.
One blocker: apps/editor/lib/import-src.test.ts imports from vitest, but vitest isn't a dependency anywhere in the repo, and every other test under apps/editor/lib/ uses bun:test (apps/editor's test script is bun test lib). That file can't resolve its imports, so the "41 pass" in the description can't have run. Please switch it to bun:test and re-run.
Then:
- The size cap uses
text.length, which is UTF-16 code units, not bytes — a graph with non-ASCII names can pass review and still 413 at the store, which is the thing the 10 MB alignment commit was for.new Blob([text]).sizecovers it. validateBuildJsonnow storesSceneMaterial.safeParse().data, so it injects defaults and drops unknown keys.apps/editor/lib/graph-schema.tsdeliberately does the opposite for exactly that reason (there's a comment). I'm fine with normalizing on a client-side import, but let's make it an explicit choice.- In the settings panel, the param is widened to
Record<string, unknown>and then cast back.ParsedBuildJsonis the right type now — please use it directly. - Please drop the
bun.lockchanges; the addedsha512hashes on thegithub:deps are a bun regeneration artifact, not part of this change.
On scope: I'd like to take the materials fix on its own, because it fixes a live bug on Load Build and shouldn't wait on the rest. Would you split it into a separate PR? I'll merge that quickly.
On the import page itself, one thing to sort out first. editor.pascal.app is a separate hosted app from apps/editor, so this page would only ship on the standalone editor, not the hosted one. And we just landed @pascal-app/capture-protocol (#713), which is the versioned, extensible hand-off format for exactly this use case — manifests, locators, capture sources. I don't think these are the same thing (yours is an already-converted build graph becoming a new scene; #713 is a capture session rendered as scan layers), and I can see wanting both. But I'd rather we agree on where the seam sits before adding a second entry point. Have a look at wiki/architecture/capture-runtime.md and tell me whether your tool would be better served by emitting a capture-protocol manifest, or whether the build-JSON path is genuinely the one you need — happy to talk it through.
|
Verified the new head — the code asks are done. Three things left here, only one of them substantive:
Two non-blocking notes while you're in there. Bugbot's stale- I checked the re-entry guard and retry path myself: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4473b86. Configure here.
|
Read through They're two different payloads, and this tool ultimately wants both — but for this hand-off, build JSON is the right seam. What A3 Atlas sends through The capture protocol is the right seam for the evidence: the RoomPlan mesh, the camera track (device motion), the scan video, a point cloud. Atlas already produces those artifacts, and the natural integration is exactly the one the doc describes — the exported build JSON later carries a So my read: |
|
That settles it — and it's the right answer. The distinction you drew is the one the doc was written to protect: the manifest carries evidence, never a second scene graph, and what Atlas hands off here is the interpreted scene — the same object the file picker already imports. So the gate is lifted. What's left is mechanical:
Then this lands. |
|
Status on the three: the seam answer is posted above; the biome hunks are collapsed and repushed (4473b86); the rebase is queued for right after #729 merges — this branch still carries the pre-#729 hunks and they'll drop then. Both non-blocking notes taken: On the hosted-vs-standalone point: for Atlas's real scenario the recipient clicks a link and lands in a browser, so we ultimately need this on editor.pascal.app — nobody installs a local editor to open a shared scan. I read this PR as the reference implementation on the standalone app; whether and when the hosted app adopts the same entry is your call once the seam is settled, and I'm happy to keep Atlas pointing at build-JSON files that work on both. |
* Carry scene materials through Load Build validateBuildJson dropped the top-level materials table, so every scene:<id> slot ref in an imported file pointed at a material that no longer existed — custom finishes silently reverted to defaults on Load Build. ParsedBuildJson now carries materials, each entry validated individually (a bad material never takes the import down, it is skipped with a warning), and handleConfirmImport hands them to setScene, whose extra.materials support already existed. Normalization here is DELIBERATE and documented in-line: safeParse().data injects defaults and drops unknown keys — the opposite of apiGraphSchema's preserve-unknowns stance — because import feeds the live scene store, which only understands schema-shaped materials. Split out of #720 at the maintainer's request. * Save Build exports the materials table it now imports Review follow-up (#729): paint a finish, Save Build, Load Build that file — the finish reverted to default because handleSaveBuild still exported only { nodes, rootNodeIds, installedPlugins }. Materials ride along now, closing the round-trip this PR opened on the import side. Also names the skipped ids in the invalid_materials warning: the audience is hand-edited files, and a bare count leaves nothing to repair by. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
#729 is merged (with your Save Build follow-up — verified and squashed). This branch now conflicts with main on the duplicated |
|
To make the use case concrete — the end state Atlas wants is one button: "Open in Pascal Editor" on a completed scan. It opens this import page with |
A scanning app (or any external tool) can now open editor.pascal.app/import?src=<https-url> to hand a build JSON to the editor. The fetch happens client-side in the visitor's browser (same trust model as dropping a file on Load Build; the host must allow CORS), the file runs through the same validateBuildJson pre-flight, the visitor reviews the contents, and only an explicit click creates the scene through the regular POST /api/scenes route — so auth, origin checks and apiGraphSchema validation all apply unchanged. src accepts https only (http for localhost during development), no embedded credentials, 25 MB cap. Unit tests for the URL validation.
Review feedback (Bugbot): a superseded or aborted fetch could overwrite a newer state — including surfacing the cleanup abort as a CORS error — and a src change left the previous review (and its Import button) live against the old file. The effect now resets to 'fetching' on every src change and every state update from a cancelled run is ignored.
Review feedback (Bugbot): MAX_IMPORT_BYTES was 25 MB while the sqlite scene store rejects graphs over DEFAULT_MAX_SCENE_BYTES (10 MB) — a file could pass review then fail POST /api/scenes with a 413 shown as a generic error. The cap now matches the store's limit, and a 413 gets its own explanation.
Review feedback (Bugbot): a second tap on Import could fire before React re-rendered into 'creating', creating two scenes and racing the redirect. A synchronous useRef guard now blocks re-entry; it is released in a finally so a failed create can be retried.
Review feedback (Bugbot): a failed POST switched to the error phase, unmounting the review and the validated graph — nothing left to retry, and refreshing re-fetches a src URL that may be short-lived. A create failure now stays in the review phase with the error shown inline and the button relabelled 'Try again'.
- import-src.test.ts now imports from bun:test like every other test under apps/editor/lib (vitest is not a repo dependency — the bun runner shimmed the import, which is why the suite did run, but the file was wrong and the description should have said bun test). - The size cap measures real bytes via Blob, not UTF-16 code units — non-ASCII names could otherwise pass review and still 413. - bun.lock restored to main (the sha512 additions were a bun regeneration artifact, not part of this change).
Two hunks collapsed per biome (createError ternary, createError JSX). Bugbot's stale-sceneName note taken: the page keys <ImportClient> by src, so a new file is a new mount and no state leaks between files — the manual phase reset inside the effect is gone with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot: the shared validateBuildJson error says "see details below", but the page listed only errors and warnings — a blocked import had no per-node path or message. Same data Load Build already shows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
988acec to
ce2917a
Compare

What
A new
/import?src=<https-url>[&name=<scene name>]page: an external tool — in our case an iOS LiDAR scanning app — hosts a build JSON at a URL and opens this page; the visitor reviews what the file contains and imports it as a new scene with one click.Until now the only way to get a generated scene into the editor was dragging a file onto Load Build, which does not exist on mobile. With this page, any scan app can end its export flow with "Open in Pascal Editor".
How it works
validateBuildJsonpre-flight as Load Build, and the page shows the node counts, floor area, warnings and errors before anything happens.POST /api/scenesroute — so auth, origin checks andapiGraphSchemavalidation (including the AssetUrl allowlist) all apply unchanged.srcaccepts https only (http for localhost during development), rejects embedded credentials, and caps the document at 25 MB. URL validation lives inlib/import-src.tswith unit tests.Tested
bun test lib: 41 pass (6 new)bun run check-types,biome check: cleanWhy we built it
We build A3 Atlas Scanner, an iOS field tool that captures homes with RoomPlan and already exports your
{nodes, rootNodeIds, materials}graph (catalog items scaled to measured dimensions, measured colors as scene materials, IFC alongside). This page is the missing link that turns every scan into a one-tap Pascal scene. Happy to adjust anything to fit the project's conventions.🤖 Generated with Claude Code
Note
Medium Risk
New entry point for creating scenes from arbitrary HTTPS URLs, but creation still goes through existing authenticated
/api/scenesvalidation; primary risk is user-supplied remote JSON in the browser, not server SSRF.Overview
Adds
/import?src=<url>[&name=…]so scanning apps and other tools can open the editor with a hosted build JSON instead of relying on desktop Load Build drag-and-drop (not available on mobile).The browser fetches and parses the file client-side (CORS required; no server-side fetch), validates it with
validateBuildJsonlike Load Build, shows stats/errors/warnings and an editable scene name, and only on confirm POSTs to/api/scenesthen navigates to the new scene. Failed creates stay on the review screen with Try again; fetch/create paths enforce 10 MB (byte-accurate viaBlob), abort on unmount, and a ref guard against double-submit.parseImportSrcinlib/import-src.tsrestrictssrcto absolute https ( http only on localhost), blocks credentials and dangerous schemes, with unit tests.Reviewed by Cursor Bugbot for commit ce2917a. Bugbot is set up for automated code reviews on this repo. Configure here.