Repository navigation
Conversation
…to create a scene and upload images, with working tests
Remex9
left a comment
There was a problem hiding this comment.
Nice work on the import flow: validation on both sides, retry-safe IDs, and good test coverage. Requesting changes for a few issues:
Major issues
- Large scenes break on Vercel. Vercel caps requests and responses at 4.5 MB. A scene with two 2 MB images (about 5.6 MB as base64) can no longer be opened, and the second import saves but reports "could not be saved". The
6mbJSON limit doesn't help on Vercel. Store image bytes separately (e.g., Vercel Blob) or cap each scene's total image size well under 4.5 MB. - Renaming while an image is saving deletes the image. Each PATCH writes the whole scene back, so the last write wins. I reproduced it: a rename sent alongside an image save lost the image 20 out of 20 times. Save images atomically or under separate keys, or disable Rename while saves are pending.
Should fix
|
Thanks for the thorough review. I agree with the requested changes. The import flow meets the basic requirements for issues #411 and #412, but the scene-size limitation and concurrent-save issue could cause failed saves or data loss in production. Please address those two issues before I can merge. Please also fix the stale image count and ensure images are only added to the scene that was active when the import started. The unrelated navigation and description-formatting changes should either be moved into separate PRs or clearly justified here. Once these updates are pushed, I can retest the import, persistence, retry, and scene-switching flows. |
|
I've updated the fix ! |
mudabs
left a comment
There was a problem hiding this comment.
The Jest and lint checks are now passing, and the earlier image-size, concurrent-save, scene-count, and scene-switching concerns appear to have been addressed.
However, the overall CodeQL check is still failing with one high-severity alert:
Uncontrolled data used in path expression at boneset-api/scenes.js:126.
Although sceneId is validated as a UUID, CodeQL still detects user-controlled data flowing into the filesystem path used by the local scene store. Please update the path construction so the input is explicitly sanitized or otherwise proven to remain inside the scene storage directory. Do not suppress the alert unless the safety of the path handling is clearly demonstrated.
After the CodeQL check passes, please rerun the full test suite and request another review.
|
Hello Munashe, all tests are passing now, im requesting to merge this again |
Pull Request Summary
Closes #411 and #412
Adds the ability to import an image into a Scene Editor scene and have it persist across reloads.
What: Users can now click "Import image…" (toolbar button or the one shown in the empty-scene placeholder) to add a PNG/JPEG/GIF/WEBP/SVG file to the current scene. The image renders on the canvas immediately, can be clicked to select/deselect (dashed outline), and is now saved to the scene so it's still there after a page reload.
Why: #411 covers letting a user get an image onto the canvas at all; #412 covers making that import actually durable — before this, an imported image only lived in browser memory (
URL.createObjectURL) and vanished on reload, which failed the issue's own persistence requirement.How:
boneset.html(toolbar button + empty-state button + hidden file input + inline error message area), styled instyle.css.templates/js/scenes.jsreads the selected file(s) as a base64 data URL (FileReader.readAsDataURL), scales it to fit a max display dimension, and adds it to the in-memory scene for an immediate render.crypto.randomUUID()id) is then persisted via a newPATCH /api/scenes/:sceneIdpayload shape,{ image: {...} }, independent of the existing{ name }rename support. The server (boneset-api/scenes.js) validates and appends it, upserting byidso a retried/duplicate save can never create a duplicate image record.data:image/...strings directly in the scene JSON document (no new file/blob storage infrastructure needed) — validated server-side to be well-formed, correctly typed, and under a 2MB (pre-encoding) size cap, enforced both client-side (immediate feedback) and server-side (defense in depth).express.json()'s body limit was raised from the 100kb default to 6mb to accommodate this.sceneCanvas.js's renderer already trusteddata:image/sources; its safe-source allowlist was extended to also acceptblob:for the local-preview path.Testing: Added/extended unit and integration tests in
boneset-api/scenes.test.js,boneset-api/server.test.js,templates/tests/scenes.test.js, andtemplates/tests/sceneCanvas.test.jscovering: import success, unsupported-type/oversized-file rejection, selection toggling, idempotent persistence (no duplicate on repeated save), scene reload showing the persisted image, and save-failure/retry behavior. All suites pass (npm test). Also manually verified end-to-end against the running dev server (import → reload → re-open scene → image still present) and via direct API calls confirming the idempotency guarantee.Screenshots
PR Checklist
Detailed Description
Design choices worth noting for reviewers: