From 8fa54a913c51719bc09651e219c6e5d3a95e5ce4 Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:35:53 -0500 Subject: [PATCH 1/6] Fix Next-button skipping first sub-bone; strengthen image test coverage --- templates/js/navigation.js | 9 +++++--- templates/tests/dropdowns.test.js | 33 ++++++++++++++++++++++++++---- templates/tests/navigation.test.js | 30 +++++++++++++++++++-------- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/templates/js/navigation.js b/templates/js/navigation.js index 8d5210d..a895885 100644 --- a/templates/js/navigation.js +++ b/templates/js/navigation.js @@ -65,8 +65,11 @@ function resetToInitialState() { } /** - * Sets the currently active bone and its associated subbones for navigation, - * resetting the index to the first subbone. + * Sets the currently active bone and its associated subbones for navigation. + * The index resets to -1 (nothing shown yet) rather than 0, since the subbone + * dropdown itself still shows its placeholder at this point - starting at 0 + * would make the first "Next" click skip subbones[0] and jump straight to + * subbones[1], because nextSubbone() would treat index 0 as already displayed. * @param {string} bone - The ID of the currently selected bone. * @param {string[]} boneSubbones - Array of subbone IDs belonging to that bone. * @returns {void} @@ -74,7 +77,7 @@ function resetToInitialState() { export function setBoneAndSubbones(bone, boneSubbones) { currentBone = bone; subbones = boneSubbones || []; - currentSubboneIndex = subbones.length > 0 ? 0 : -1; + currentSubboneIndex = -1; } /** diff --git a/templates/tests/dropdowns.test.js b/templates/tests/dropdowns.test.js index 7f67fe1..46572df 100644 --- a/templates/tests/dropdowns.test.js +++ b/templates/tests/dropdowns.test.js @@ -9,11 +9,14 @@ jest.mock("../js/annotationOverlay.js", () => ({ clearAnnotations: jest.fn(), })); jest.mock("../js/api.js", () => ({ - fetchBoneData: jest.fn(() => Promise.resolve({ images: [{ url: "test.jpg" }] })), + // Images are keyed by boneId so tests can confirm the correct parent's + // images (not just any images) are displayed after a deselect. + fetchBoneData: jest.fn((boneId) => Promise.resolve({ images: [{ url: `${boneId}.jpg` }] })), })); const { loadDescription } = require("../js/description.js"); -const { showPlaceholder } = require("../js/imageDisplay.js"); +const { showPlaceholder, displayBoneImages } = require("../js/imageDisplay.js"); +const { fetchBoneData } = require("../js/api.js"); const { setupDropdownListeners, populateBonesetDropdown } = require("../js/dropdowns.js"); const combinedData = { @@ -53,31 +56,53 @@ describe("Deselecting a bone/sub-bone reverts to parent info - Issue 248", () => setupDropdownListeners(combinedData); }); - test("deselecting a bone falls back to the boneset info instead of the placeholder", async () => { + test("deselecting a bone falls back to the boneset's description and images instead of the placeholder", async () => { selectValue(bonesetSelect, "bony_pelvis"); selectValue(boneSelect, "ilium"); + await new Promise(process.nextTick); loadDescription.mockClear(); showPlaceholder.mockClear(); + fetchBoneData.mockClear(); + displayBoneImages.mockClear(); selectValue(boneSelect, ""); await new Promise(process.nextTick); expect(loadDescription).toHaveBeenCalledWith("bony_pelvis"); expect(showPlaceholder).not.toHaveBeenCalled(); + + // Confirm the boneset's own images were fetched and rendered, not the + // bone's images left over from before the deselect (nor a placeholder). + expect(fetchBoneData).toHaveBeenCalledWith("bony_pelvis"); + expect(displayBoneImages).toHaveBeenCalledWith( + [{ url: "bony_pelvis.jpg" }], + expect.objectContaining({ boneId: "bony_pelvis" }) + ); }); - test("deselecting a sub-bone falls back to the parent bone info instead of the placeholder", async () => { + test("deselecting a sub-bone falls back to the parent bone's description and images instead of the placeholder", async () => { selectValue(bonesetSelect, "bony_pelvis"); selectValue(boneSelect, "ilium"); selectValue(subboneSelect, "iliac_crest"); + await new Promise(process.nextTick); loadDescription.mockClear(); showPlaceholder.mockClear(); + fetchBoneData.mockClear(); + displayBoneImages.mockClear(); selectValue(subboneSelect, ""); await new Promise(process.nextTick); expect(loadDescription).toHaveBeenCalledWith("ilium"); expect(showPlaceholder).not.toHaveBeenCalled(); + + // Confirm the parent bone's own images were fetched and rendered, not the + // sub-bone's images left over from before the deselect (nor a placeholder). + expect(fetchBoneData).toHaveBeenCalledWith("ilium"); + expect(displayBoneImages).toHaveBeenCalledWith( + [{ url: "ilium.jpg" }], + expect.objectContaining({ boneId: "ilium" }) + ); }); test("deselecting a bone with no boneset selected still shows the placeholder", async () => { diff --git a/templates/tests/navigation.test.js b/templates/tests/navigation.test.js index f00cf73..c9bf9e7 100644 --- a/templates/tests/navigation.test.js +++ b/templates/tests/navigation.test.js @@ -37,34 +37,38 @@ describe("Prev/Next navigation keeps the dropdown and its listeners in sync - Is subboneDropdown.addEventListener("change", changeSpy); }); - test("clicking Next dispatches a change event and advances the dropdown selection", () => { + test("clicking Next for the first time after selecting a bone shows the first subbone, not the second", () => { + // Regression test: the dropdown still shows its placeholder right after a + // bone is selected, so the first Next click must reveal subbones[0]. It must + // not skip straight to subbones[1] as if subbones[0] had already been shown. nextButton.click(); - expect(subboneDropdown.value).toBe("subbone_b"); + expect(subboneDropdown.value).toBe("subbone_a"); expect(changeSpy).toHaveBeenCalledTimes(1); }); - test("clicking Next repeatedly stops at the last subbone", () => { + test("clicking Next repeatedly walks through every subbone in order and stops at the last one", () => { + nextButton.click(); // (none shown yet) -> subbone_a nextButton.click(); // subbone_a -> subbone_b nextButton.click(); // subbone_b -> subbone_c nextButton.click(); // already last subbone: no-op, no extra change event expect(subboneDropdown.value).toBe("subbone_c"); - expect(changeSpy).toHaveBeenCalledTimes(2); + expect(changeSpy).toHaveBeenCalledTimes(3); }); test("clicking Previous dispatches a change event and moves back a subbone", () => { - nextButton.click(); - nextButton.click(); + nextButton.click(); // -> subbone_a + nextButton.click(); // -> subbone_b changeSpy.mockClear(); prevButton.click(); - expect(subboneDropdown.value).toBe("subbone_b"); + expect(subboneDropdown.value).toBe("subbone_a"); expect(changeSpy).toHaveBeenCalledTimes(1); }); - test("clicking Previous at the first subbone does nothing", () => { + test("clicking Previous before any Next click does nothing", () => { const valueBeforeClick = subboneDropdown.value; prevButton.click(); @@ -73,6 +77,16 @@ describe("Prev/Next navigation keeps the dropdown and its listeners in sync - Is expect(changeSpy).not.toHaveBeenCalled(); }); + test("clicking Previous at the first subbone does nothing", () => { + nextButton.click(); // -> subbone_a + changeSpy.mockClear(); + + prevButton.click(); + + expect(subboneDropdown.value).toBe("subbone_a"); + expect(changeSpy).not.toHaveBeenCalled(); + }); + test("buttons are disabled when the current bone has no subbones", () => { setBoneAndSubbones("ischium", []); disableButtons(prevButton, nextButton); From 5ff40f8a5c038cb8847b49868a8552d61de85b51 Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:22:59 -0500 Subject: [PATCH 2/6] worked on issues 411 and 412, which dealt with setting up being able to create a scene and upload images, with working tests --- boneset-api/scenes.js | 92 +++++++++++-- boneset-api/scenes.test.js | 136 ++++++++++++++++++- boneset-api/server.js | 26 +++- boneset-api/server.test.js | 87 +++++++++++- templates/boneset.html | 4 + templates/js/sceneCanvas.js | 36 ++++- templates/js/scenes.js | 161 ++++++++++++++++++++++- templates/style.css | 11 ++ templates/tests/sceneCanvas.test.js | 38 ++++++ templates/tests/scenes.test.js | 196 +++++++++++++++++++++++++++- 10 files changed, 756 insertions(+), 31 deletions(-) diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index b425179..3111ce1 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -9,6 +9,10 @@ const path = require("path"); const DEFAULT_SCENE_NAME = "Untitled Scene"; const MAX_SCENE_NAME_LENGTH = 100; const SCENE_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +// 2MB raw file * ~4/3 base64 overhead, rounded down slightly for headroom under +// the express.json() body limit once the surrounding JSON is added (Issue #412). +const MAX_IMAGE_SRC_LENGTH = 2_800_000; +const SAFE_IMAGE_SRC_PREFIX = /^data:image\//i; class SceneStorageUnavailableError extends Error {} @@ -30,6 +34,42 @@ function normalizeSceneName(name) { return { name: trimmed }; } +function isFiniteNumber(value) { + return typeof value === "number" && Number.isFinite(value); +} + +/** + * Validates an image object submitted for persistence via PATCH (Issue #412). + * The `id` reuses the same UUID shape as scene ids (isValidSceneId) since both + * are just crypto.randomUUID() values identifying different kinds of records. + * @param {object} image + * @returns {{ error: string } | { image: object }} + */ +function normalizeImage(image) { + if (!image || typeof image !== "object") { + return { error: "image must be an object" }; + } + if (!isValidSceneId(image.id)) { + return { error: "image.id must be a valid id" }; + } + if (typeof image.src !== "string" || !SAFE_IMAGE_SRC_PREFIX.test(image.src)) { + return { error: "image.src must be a data:image/ URL" }; + } + if (image.src.length > MAX_IMAGE_SRC_LENGTH) { + return { error: "Image is too large (max 2MB)" }; + } + if (![image.x, image.y, image.width, image.height].every(isFiniteNumber)) { + return { error: "image.x, image.y, image.width, and image.height must be numbers" }; + } + if (image.width <= 0 || image.height <= 0) { + return { error: "image.width and image.height must be greater than 0" }; + } + + return { + image: { id: image.id, src: image.src, x: image.x, y: image.y, width: image.width, height: image.height }, + }; +} + function toSummary(scene) { return { id: scene.id, @@ -257,16 +297,15 @@ function createScenesRouter(store = resolveSceneStore()) { }); /** - * Renames a scene. Empty names are rejected; duplicate names return 409. Issue #423. + * Updates a scene. Supports renaming (empty names rejected, duplicates return + * 409 - Issue #423) and/or adding one newly imported image (validated and + * upserted by id so a retried request can't create a duplicate - Issue #412). + * At least one of `name`/`image` must be present in the body. */ router.patch("/:sceneId", async (req, res) => { try { - if (!req.body || req.body.name === undefined) { - return res.status(400).json({ error: "name is required" }); - } - const result = normalizeSceneName(req.body.name); - if (result.error) { - return res.status(400).json({ error: result.error }); + if (!req.body || (req.body.name === undefined && req.body.image === undefined)) { + return res.status(400).json({ error: "name or image is required" }); } const { sceneId } = req.params; @@ -275,17 +314,40 @@ function createScenesRouter(store = resolveSceneStore()) { return res.status(404).json({ error: "Scene not found" }); } - const scenes = await store.list(); - if (isNameTaken(scenes, result.name, sceneId)) { - return res.status(409).json({ error: `A scene named "${result.name}" already exists` }); + let changed = false; + + if (req.body.name !== undefined) { + const result = normalizeSceneName(req.body.name); + if (result.error) { + return res.status(400).json({ error: result.error }); + } + const scenes = await store.list(); + if (isNameTaken(scenes, result.name, sceneId)) { + return res.status(409).json({ error: `A scene named "${result.name}" already exists` }); + } + scene.name = result.name; + changed = true; } - scene.name = result.name; - scene.updatedAt = new Date().toISOString(); - await store.save(scene); + if (req.body.image !== undefined) { + const result = normalizeImage(req.body.image); + if (result.error) { + return res.status(400).json({ error: result.error }); + } + const alreadyPresent = scene.images.some((img) => img.id === result.image.id); + if (!alreadyPresent) { + scene.images.push(result.image); + changed = true; + } + } + + if (changed) { + scene.updatedAt = new Date().toISOString(); + await store.save(scene); + } res.json(scene); } catch (error) { - sendStoreError(res, error, "Failed to rename scene"); + sendStoreError(res, error, "Failed to update scene"); } }); @@ -314,5 +376,7 @@ module.exports = { resolveSceneStore, isValidSceneId, normalizeSceneName, + normalizeImage, DEFAULT_SCENE_NAME, + MAX_IMAGE_SRC_LENGTH, }; diff --git a/boneset-api/scenes.test.js b/boneset-api/scenes.test.js index 4b74bcc..35ebd46 100644 --- a/boneset-api/scenes.test.js +++ b/boneset-api/scenes.test.js @@ -1,6 +1,7 @@ const fs = require("fs"); const os = require("os"); const path = require("path"); +const crypto = require("crypto"); const express = require("express"); const request = require("supertest"); const { @@ -8,6 +9,7 @@ const { createFileSceneStore, createRedisSceneStore, resolveSceneStore, + MAX_IMAGE_SRC_LENGTH, } = require("./scenes"); // In-memory stand-in for the subset of the @upstash/redis client the store uses. @@ -45,7 +47,9 @@ function createFakeRedis() { function buildApp(store) { const app = express(); - app.use(express.json({ limit: "2mb" })); + // Matches server.js's real limit (Issue #412) so tests near the image size + // cap are rejected by the route's own validation, not Express's raw limit. + app.use(express.json({ limit: "6mb" })); app.use("/api/scenes", createScenesRouter(store)); return app; } @@ -207,6 +211,136 @@ describe.each(backends)("Scenes API ($name)", (backend) => { }); }); + // Issue #412: Upload and Store an Imported Image + describe("PATCH /api/scenes/:sceneId with an image - Issue 412", () => { + function validImage(overrides = {}) { + return { + id: crypto.randomUUID(), + src: "data:image/png;base64,AAAA", + x: 10, + y: 20, + width: 100, + height: 50, + ...overrides, + }; + } + + it("adds a newly imported image to the scene", async () => { + const created = await createScene(); + const image = validImage(); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image }); + + expect(response.statusCode).toBe(200); + expect(response.body.images).toEqual([image]); + }); + + it("keeps the image after the scene is reloaded", async () => { + const created = await createScene(); + const image = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image }); + + const reloaded = await request(app).get(`/api/scenes/${created.body.id}`); + + expect(reloaded.statusCode).toBe(200); + expect(reloaded.body.images).toEqual([image]); + }); + + it("does not create a duplicate record when the same image id is saved twice", async () => { + const created = await createScene(); + const image = validImage(); + + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image }); + const second = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image }); + + expect(second.statusCode).toBe(200); + expect(second.body.images).toHaveLength(1); + }); + + it("keeps existing images when a different image is added", async () => { + const created = await createScene(); + const first = validImage(); + const second = validImage(); + + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: first }); + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image: second }); + + expect(response.body.images).toEqual([first, second]); + }); + + it("rejects an image whose src is not a data:image/ URL", async () => { + const created = await createScene(); + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image: validImage({ src: "https://example.com/x.png" }) }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toMatch(/data:image/); + }); + + it("rejects an image whose src exceeds the size cap", async () => { + const created = await createScene(); + const oversizedSrc = `data:image/png;base64,${"A".repeat(MAX_IMAGE_SRC_LENGTH + 1)}`; + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image: validImage({ src: oversizedSrc }) }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toMatch(/too large/i); + }); + + it("rejects an image with non-numeric position or size fields", async () => { + const created = await createScene(); + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image: validImage({ width: "big" }) }); + + expect(response.statusCode).toBe(400); + }); + + it("rejects an image with a width of 0", async () => { + const created = await createScene(); + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ image: validImage({ width: 0 }) }); + + expect(response.statusCode).toBe(400); + }); + + it("returns 404 when adding an image to an unknown scene", async () => { + const response = await request(app) + .patch(`/api/scenes/${UNKNOWN_ID}`) + .send({ image: validImage() }); + + expect(response.statusCode).toBe(404); + }); + + it("returns 400 when the body has neither name nor image", async () => { + const created = await createScene(); + const response = await request(app).patch(`/api/scenes/${created.body.id}`).send({}); + expect(response.statusCode).toBe(400); + }); + + it("can rename and add an image in the same request", async () => { + const created = await createScene(); + const image = validImage(); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ name: "Renamed", image }); + + expect(response.statusCode).toBe(200); + expect(response.body.name).toBe("Renamed"); + expect(response.body.images).toEqual([image]); + }); + }); + // Issue #424: Delete a Scene describe("DELETE /api/scenes/:sceneId - Issue 424", () => { it("deletes only the requested scene", async () => { diff --git a/boneset-api/server.js b/boneset-api/server.js index e995e4d..55bac23 100644 --- a/boneset-api/server.js +++ b/boneset-api/server.js @@ -11,7 +11,9 @@ const app = express(); const PORT = process.env.PORT || 8000; app.use(cors()); -app.use(express.json()); +// Default 100kb is too small for a base64-encoded imported scene image (Issue #412); +// the scenes route independently caps image size well under this ceiling too. +app.use(express.json({ limit: "6mb" })); const LOCAL_DATA_DIR = path.join(__dirname, "data"); const BONESET_DIR = path.join(LOCAL_DATA_DIR, "boneset"); @@ -50,6 +52,22 @@ function escapeHtml(str = "") { })[c]); } +/** + * Renders lightweight inline formatting in description text: `**bold**` becomes + * , and `*italic*` becomes . The input is HTML-escaped first, so the + * `*`/`**` markers are the only formatting ever produced - any literal HTML in the + * source text (e.g. a stray `")).toBe( + "<script>alert(1)</script>" + ); + }); + + it("escapes HTML even when it appears alongside formatting markers", () => { + expect(formatDescriptionText("****")).toBe( + "<img src=x onerror=alert(1)>" + ); + }); +}); + +// Integration test for Issue 381: GET /api/description renders the formatting end-to-end +describe("GET /api/description - Issue 381", () => { + const testBoneId = "__format_test_bone_381"; + const descriptionsDir = path.join(__dirname, "data", "descriptions"); + const fixturePath = path.join(descriptionsDir, `${testBoneId}_description.json`); + + beforeAll(async () => { + await fs.writeFile( + fixturePath, + JSON.stringify({ + name: "Format Test Bone", + id: testBoneId, + description: [ + "Plain sentence with no markers.", + "A **bold** point and an *italic* point.", + ], + images: [], + }) + ); + }); + + afterAll(async () => { + await fs.unlink(fixturePath); + }); + + it("renders / tags for a description that uses formatting markers", async () => { + const response = await request(app).get( + `/api/description/?boneId=${testBoneId}` + ); + + expect(response.statusCode).toBe(200); + expect(response.text).toContain("
  • Plain sentence with no markers.
    • "); + expect(response.text).toContain( + "
  • A bold point and an italic point.
    • " + ); + }); +}); diff --git a/templates/boneset.html b/templates/boneset.html index bc496a1..21e3d79 100644 --- a/templates/boneset.html +++ b/templates/boneset.html @@ -218,16 +218,20 @@

    + +
    +

    This scene is blank.

    Images and annotations added to this scene will appear here.

    +
    diff --git a/templates/js/sceneCanvas.js b/templates/js/sceneCanvas.js index a2ffdfa..62f3720 100644 --- a/templates/js/sceneCanvas.js +++ b/templates/js/sceneCanvas.js @@ -16,7 +16,11 @@ const MIN_WIDTH = 960; const MIN_HEIGHT = 600; const PADDING = 40; const DEFAULT_STROKE = "#003366"; -const SAFE_IMAGE_SRC = /^(\/|\.\/|https?:\/\/|data:image\/)/i; +// `blob:` is included alongside the existing schemes because Issue #411's image +// import flow previews a freshly chosen file via `URL.createObjectURL(file)` - +// the browser mints that URL itself from a real File/Blob, so it's exactly as +// safe as the already-allowed `data:image/` scheme, never attacker-controllable. +const SAFE_IMAGE_SRC = /^(\/|\.\/|https?:\/\/|data:image\/|blob:)/i; let markerCount = 0; @@ -55,7 +59,7 @@ function el(name, attrs = {}) { return node; } -function renderImage(image) { +function renderImage(image, index) { if (typeof image.src !== "string" || !SAFE_IMAGE_SRC.test(image.src)) return null; if (![image.x, image.y, image.width, image.height].every(isNumber)) return null; if (image.width <= 0 || image.height <= 0) return null; @@ -81,6 +85,8 @@ function renderImage(image) { preserveAspectRatio: "none", opacity: isNumber(image.opacity) ? image.opacity : undefined, transform: transforms.length ? transforms.join(" ") : undefined, + class: "scene-image-object", + "data-scene-image-index": index, }); const corners = [ [image.x, image.y], @@ -178,16 +184,22 @@ function renderAnnotation(annotation, defs) { } /** - * Renders a scene's images and annotations into an SVG element. + * Renders a scene's images and annotations into an SVG element. Optionally draws a + * non-interactive selection outline around one image, identified by its index in + * `scene.images` - the same index each rendered `` carries as its + * `data-scene-image-index` attribute, letting a caller wire up click-to-select + * without this renderer owning any selection state itself. * @param {{ images: object[], annotations: object[] }} scene + * @param {{ selectedIndex?: number }} [options] * @returns {{ svg: SVGSVGElement, rendered: number, unsupported: number, width: number, height: number }} */ -export function renderScene(scene) { +export function renderScene(scene, { selectedIndex } = {}) { const svg = el("svg", { class: "scene-svg", role: "img" }); const defs = el("defs"); const imageLayer = el("g", { class: "scene-layer-images" }); const annotationLayer = el("g", { class: "scene-layer-annotations" }); - svg.append(defs, imageLayer, annotationLayer); + const selectionLayer = el("g", { class: "scene-layer-selection" }); + svg.append(defs, imageLayer, annotationLayer, selectionLayer); let minX = 0; let minY = 0; @@ -199,7 +211,7 @@ export function renderScene(scene) { const place = (result, layer) => { if (!result) { unsupported += 1; - return; + return null; } layer.appendChild(result.node); rendered += 1; @@ -209,9 +221,19 @@ export function renderScene(scene) { maxX = Math.max(maxX, x); maxY = Math.max(maxY, y); } + return result; }; - for (const image of scene.images || []) place(renderImage(image), imageLayer); + (scene.images || []).forEach((image, index) => { + const result = place(renderImage(image, index), imageLayer); + if (result && index === selectedIndex) { + selectionLayer.appendChild(el("polygon", { + class: "scene-selection-outline", + points: result.bounds.map((p) => p.join(",")).join(" "), + "pointer-events": "none", + })); + } + }); for (const annotation of scene.annotations || []) place(renderAnnotation(annotation, defs), annotationLayer); const x = minX < 0 ? minX - PADDING : 0; diff --git a/templates/js/scenes.js b/templates/js/scenes.js index 498f36c..5071b9f 100644 --- a/templates/js/scenes.js +++ b/templates/js/scenes.js @@ -53,6 +53,17 @@ export function renameScene(sceneId, name) { return request(scenePath(sceneId), { method: "PATCH", body: JSON.stringify({ name }) }); } +/** + * Persists one newly imported image on a scene (Issue #412). The server upserts + * by `image.id`, so retrying this call for the same image is always safe. + * @param {string} sceneId + * @param {{id: string, src: string, x: number, y: number, width: number, height: number}} image + * @returns {Promise} The updated scene. + */ +export function saveImageToScene(sceneId, image) { + return request(scenePath(sceneId), { method: "PATCH", body: JSON.stringify({ image }) }); +} + export function deleteScene(sceneId) { return request(scenePath(sceneId), { method: "DELETE" }); } @@ -90,10 +101,47 @@ const state = { openRequest: 0, fitToWidth: true, busy: false, + selectedImageIndex: null, }; let dom = null; +// Issues #411/#412: importing an image into a scene and persisting it. The +// image is read as a base64 data: URL - that same string is both the immediate +// preview `src` and what gets saved to the scene document, so there's only one +// representation to reason about (see saveImageToScene in the API section above). +const SUPPORTED_IMAGE_TYPES = ["image/png", "image/jpeg", "image/gif", "image/webp", "image/svg+xml"]; +const MAX_IMPORT_DIMENSION = 320; +const IMPORT_OFFSET_STEP = 24; +// Kept comfortably under the backend's MAX_IMAGE_SRC_LENGTH cap (boneset-api/scenes.js) +// once base64's ~4/3 overhead is applied - checked client-side for immediate feedback, +// but the server independently re-checks its own cap too (never trust the client alone). +const MAX_IMPORT_FILE_SIZE = 2 * 1024 * 1024; + +function readFileAsDataUrl(file) { + return new Promise((resolve, reject) => { + const reader = new FileReader(); + reader.onload = () => resolve(reader.result); + reader.onerror = () => reject(new Error("Could not read file")); + reader.readAsDataURL(file); + }); +} + +function loadImageDimensions(src) { + return new Promise((resolve, reject) => { + const img = new Image(); + img.onload = () => resolve({ width: img.naturalWidth, height: img.naturalHeight }); + img.onerror = () => reject(new Error("Could not read image dimensions")); + img.src = src; + }); +} + +function scaleToFit(width, height, max) { + if (width <= max && height <= max) return { width, height }; + const scale = Math.min(max / width, max / height); + return { width: width * scale, height: height * scale }; +} + function announce(message) { dom.status.textContent = ""; // Clearing first makes screen readers repeat identical consecutive messages. @@ -180,7 +228,7 @@ function renderCanvas() { dom.canvasNote.hidden = true; if (isEmpty) return; - const { svg, unsupported, width, height } = renderScene(scene); + const { svg, unsupported, width, height } = renderScene(scene, { selectedIndex: state.selectedImageIndex }); svg.setAttribute("aria-label", `Scene canvas for ${scene.name}`); if (state.fitToWidth) { svg.style.width = "100%"; @@ -214,6 +262,7 @@ function renderWorkspace() { function clearActiveScene() { state.active = null; + state.selectedImageIndex = null; state.openRequest += 1; dom.workspace.removeAttribute("aria-busy"); dom.workspaceLoading.hidden = true; @@ -254,6 +303,7 @@ async function openScene(sceneId) { const scene = await getScene(sceneId); if (requestId !== state.openRequest) return; state.active = scene; + state.selectedImageIndex = null; renderWorkspace(); renderLibrary(); announce(`Opened ${scene.name}.`); @@ -284,6 +334,7 @@ async function handleCreate() { const scene = await createScene(); state.openRequest += 1; state.active = scene; + state.selectedImageIndex = null; await loadLibrary(); setBusy(false); renderWorkspace(); @@ -398,6 +449,91 @@ async function handleDelete() { } } +/** + * Retries persisting images that previously failed to save (Issue #412). Safe + * to call repeatedly - the server upserts by image id, so a partially-succeeded + * previous attempt is never double-saved. + * @param {string} sceneId - The scene these images belong to, captured at + * import time so a later retry targets the right scene even if the user has + * since opened a different one. + * @param {object[]} images + * @returns {Promise} + */ +async function retryImageSaves(sceneId, images) { + const stillFailing = []; + for (const image of images) { + try { + await saveImageToScene(sceneId, image); + } catch { + stillFailing.push(image); + } + } + + if (stillFailing.length === 0) { + hideWorkspaceError(); + return; + } + showWorkspaceError( + `${plural(stillFailing.length, "image")} could not be saved and will be lost if you reload.`, + { retry: () => retryImageSaves(sceneId, stillFailing) } + ); +} + +/** + * Imports one or more image files into the currently open scene as new, + * unselected image objects, and persists each one (Issues #411/#412). + * Unsupported or oversized files are skipped with a combined error message + * rather than aborting the whole import. + * @param {FileList|File[]} fileList - Files chosen via the import input. + * @returns {Promise} + */ +async function handleImportFiles(fileList) { + if (!state.active || !fileList || fileList.length === 0) return; + const sceneId = state.active.id; + + dom.importError.hidden = true; + dom.importError.textContent = ""; + + const skipped = []; + const added = []; + + for (const file of fileList) { + if (!SUPPORTED_IMAGE_TYPES.includes(file.type)) { + skipped.push(`${file.name} (unsupported type)`); + continue; + } + if (file.size > MAX_IMPORT_FILE_SIZE) { + skipped.push(`${file.name} (too large, max 2MB)`); + continue; + } + + try { + const dataUrl = await readFileAsDataUrl(file); + const natural = await loadImageDimensions(dataUrl); + const { width, height } = scaleToFit(natural.width, natural.height, MAX_IMPORT_DIMENSION); + const offset = IMPORT_OFFSET_STEP * (state.active.images.length + 1); + const image = { id: crypto.randomUUID(), src: dataUrl, x: offset, y: offset, width, height }; + state.active.images.push(image); + added.push(image); + } catch { + skipped.push(`${file.name} (could not be read)`); + } + } + + if (skipped.length > 0) { + const addedPart = added.length > 0 ? `${plural(added.length, "image")} added. ` : ""; + dom.importError.hidden = false; + dom.importError.textContent = + `${addedPart}${plural(skipped.length, "file")} skipped: ${skipped.join(", ")}.`; + } + + if (added.length > 0) { + renderWorkspace(); + announce(`${plural(added.length, "image")} added to the scene.`); + await retryImageSaves(sceneId, added); + } +} + export function getActiveScene() { return state.active; } @@ -460,6 +596,10 @@ export function initializeSceneEditor(root = document) { canvas: $("scene-canvas"), canvasEmpty: $("scene-canvas-empty"), canvasNote: $("scene-canvas-note"), + importButton: $("scene-import-image"), + importButtonEmpty: $("scene-canvas-empty-import"), + importInput: $("scene-import-input"), + importError: $("scene-import-error"), }; if (!dom.editorView || !dom.enterButton) return; @@ -487,6 +627,25 @@ export function initializeSceneEditor(root = document) { dom.deleteButton.addEventListener("click", handleDelete); dom.fitToggle.addEventListener("click", () => setFitToWidth(!state.fitToWidth)); + dom.importButton.addEventListener("click", () => dom.importInput.click()); + dom.importButtonEmpty.addEventListener("click", () => dom.importInput.click()); + dom.importInput.addEventListener("change", (event) => { + handleImportFiles(event.target.files); + event.target.value = ""; + }); + dom.canvas.addEventListener("click", (event) => { + const target = event.target.closest("[data-scene-image-index]"); + const index = target ? Number(target.dataset.sceneImageIndex) : null; + state.selectedImageIndex = state.selectedImageIndex === index ? null : index; + renderCanvas(); + }); + dom.canvas.addEventListener("keydown", (event) => { + if (event.key === "Escape" && state.selectedImageIndex !== null) { + state.selectedImageIndex = null; + renderCanvas(); + } + }); + renderWorkspace(); } diff --git a/templates/style.css b/templates/style.css index c59cfd7..6630db6 100644 --- a/templates/style.css +++ b/templates/style.css @@ -2851,6 +2851,17 @@ button:disabled { color: #92400e; } +.scene-image-object { + cursor: pointer; +} + +.scene-selection-outline { + fill: none; + stroke: var(--accent); + stroke-width: 2; + stroke-dasharray: 6 4; +} + @media (max-width: 900px) { .scene-editor-layout { grid-template-columns: 1fr; diff --git a/templates/tests/sceneCanvas.test.js b/templates/tests/sceneCanvas.test.js index 1a88e72..081743d 100644 --- a/templates/tests/sceneCanvas.test.js +++ b/templates/tests/sceneCanvas.test.js @@ -33,3 +33,41 @@ describe("renderScene bounds", () => { expect(box.y + box.height).toBeGreaterThanOrEqual(600); }); }); + +// Issue #411: images are tagged with their index and can be shown as selected. +describe("renderScene selection support", () => { + it("tags each rendered image with its index in scene.images", () => { + const { svg } = renderScene({ + images: [image({ x: 0 }), image({ x: 300 })], + annotations: [], + }); + const nodes = svg.querySelectorAll(".scene-image-object"); + + expect(nodes).toHaveLength(2); + expect(nodes[0].getAttribute("data-scene-image-index")).toBe("0"); + expect(nodes[1].getAttribute("data-scene-image-index")).toBe("1"); + }); + + it("draws a selection outline for the image at selectedIndex", () => { + const { svg } = renderScene( + { images: [image(), image({ x: 300 })], annotations: [] }, + { selectedIndex: 1 } + ); + + const outlines = svg.querySelectorAll(".scene-selection-outline"); + expect(outlines).toHaveLength(1); + }); + + it("draws no selection outline when selectedIndex is omitted", () => { + const { svg } = renderScene({ images: [image()], annotations: [] }); + expect(svg.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + }); + + it("draws no selection outline for an out-of-range selectedIndex", () => { + const { svg } = renderScene( + { images: [image()], annotations: [] }, + { selectedIndex: 5 } + ); + expect(svg.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + }); +}); diff --git a/templates/tests/scenes.test.js b/templates/tests/scenes.test.js index d063893..b9dc7f7 100644 --- a/templates/tests/scenes.test.js +++ b/templates/tests/scenes.test.js @@ -57,10 +57,19 @@ function createFakeBackend() { if (!scene) return respond(404, { error: "Scene not found" }); if (method === "GET") return respond(200, scene); if (method === "PATCH") { - const name = (body.name || "").trim(); - if (!name) return respond(400, { error: "Scene name cannot be empty" }); - if (nameTaken(name, id)) return respond(409, { error: "Name taken" }); - scene.name = name; + if (!body || (body.name === undefined && body.image === undefined)) { + return respond(400, { error: "name or image is required" }); + } + if (body.name !== undefined) { + const name = (body.name || "").trim(); + if (!name) return respond(400, { error: "Scene name cannot be empty" }); + if (nameTaken(name, id)) return respond(409, { error: "Name taken" }); + scene.name = name; + } + if (body.image !== undefined) { + const alreadyPresent = scene.images.some((img) => img.id === body.image.id); + if (!alreadyPresent) scene.images.push(body.image); + } scene.updatedAt = now; return respond(200, scene); } @@ -350,3 +359,182 @@ describe("describeError", () => { expect(describeError(new SceneApiError(500, "x"), "delete")).toMatch(/Try again/); }); }); + +// Issues #411/#412: importing an image into a scene, selecting it once rendered, +// and persisting it. +describe("Scene editor: importing images - Issue 411", () => { + let originalImage; + let originalFileReader; + + beforeEach(() => { + // jsdom doesn't implement real file reading or image loading, so both are + // stubbed: FileReader "reads" a fixed data URL and Image "loads" fixed + // dimensions, both asynchronously via a real setTimeout (fake timers are + // not enabled in this suite, so this resolves naturally). + originalFileReader = window.FileReader; + window.FileReader = class { + readAsDataURL(_file) { + setTimeout(() => { + this.result = "data:image/png;base64,AAAA"; + if (this.onload) this.onload(); + }, 0); + } + }; + + originalImage = window.Image; + window.Image = class { + constructor() { + this.naturalWidth = 800; + this.naturalHeight = 400; + } + set src(_value) { + setTimeout(() => this.onload && this.onload(), 0); + } + }; + }); + + afterEach(() => { + window.Image = originalImage; + window.FileReader = originalFileReader; + }); + + function setFiles(files) { + const input = $("scene-import-input"); + Object.defineProperty(input, "files", { value: files, configurable: true }); + input.dispatchEvent(new Event("change", { bubbles: true })); + } + + async function openNewScene() { + await enterEditor(); + click("scene-new"); + await waitFor(() => expect($("scene-workspace-scene").hidden).toBe(false)); + } + + it("adds a supported image to the scene and renders it as a selectable object", async () => { + await openNewScene(); + + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect($("scene-meta").textContent).toContain("1 image")); + + const nodes = document.querySelectorAll(".scene-image-object"); + expect(nodes).toHaveLength(1); + expect(nodes[0].getAttribute("data-scene-image-index")).toBe("0"); + expect($("scene-canvas").hidden).toBe(false); + expect($("scene-canvas-empty").hidden).toBe(true); + }); + + it("shows a clear message and adds nothing for an unsupported file type", async () => { + await openNewScene(); + + setFiles([{ name: "notes.pdf", type: "application/pdf" }]); + await waitFor(() => expect($("scene-import-error").hidden).toBe(false)); + + expect($("scene-import-error").textContent).toMatch(/notes\.pdf/); + expect($("scene-import-error").textContent).toMatch(/unsupported/i); + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(0); + expect($("scene-meta").textContent).toContain("0 images"); + }); + + it("still adds the supported files when a multi-file import includes an unsupported one", async () => { + await openNewScene(); + + setFiles([ + { name: "ilium.png", type: "image/png" }, + { name: "notes.pdf", type: "application/pdf" }, + ]); + await waitFor(() => expect($("scene-import-error").hidden).toBe(false)); + + expect($("scene-meta").textContent).toContain("1 image"); + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1); + expect($("scene-import-error").textContent).toMatch(/notes\.pdf/); + }); + + it("selects an image on click and deselects it on a second click", async () => { + await openNewScene(); + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1)); + + document.querySelector(".scene-image-object").dispatchEvent(new MouseEvent("click", { bubbles: true })); + await waitFor(() => expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(1)); + + document.querySelector(".scene-image-object").dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + }); + + it("clears the selection when clicking empty canvas background", async () => { + await openNewScene(); + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1)); + + document.querySelector(".scene-image-object").dispatchEvent(new MouseEvent("click", { bubbles: true })); + await waitFor(() => expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(1)); + + $("scene-canvas").dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + }); + + it("resets the selection when a different scene is opened", async () => { + // Seeded with its own pre-existing image so the assertion below proves the + // selection was actually reset, not just that the other scene is empty. + backend.seed({ + name: "Other", + images: [{ src: "/images/other.png", x: 0, y: 0, width: 100, height: 100 }], + }); + await openNewScene(); + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1)); + document.querySelector(".scene-image-object").dispatchEvent(new MouseEvent("click", { bubbles: true })); + await waitFor(() => expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(1)); + + await openByName("Other"); + + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1); + expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + }); + + // Issue #412: persisting an imported image so it survives a reload. + it("persists an imported image by PATCHing the scene", async () => { + await openNewScene(); + + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => + expect(backend.calls.some((c) => c.method === "PATCH" && c.body && c.body.image)).toBe(true) + ); + + const patchCall = backend.calls.find((c) => c.method === "PATCH" && c.body && c.body.image); + expect(patchCall.body.image).toMatchObject({ src: "data:image/png;base64,AAAA" }); + expect(typeof patchCall.body.image.id).toBe("string"); + + const storedScene = [...backend.scenes.values()][0]; + expect(storedScene.images).toHaveLength(1); + }); + + it("rejects a file over the size cap client-side without contacting the server", async () => { + await openNewScene(); + + setFiles([{ name: "huge.png", type: "image/png", size: 3 * 1024 * 1024 }]); + await waitFor(() => expect($("scene-import-error").hidden).toBe(false)); + + expect($("scene-import-error").textContent).toMatch(/huge\.png/); + expect($("scene-import-error").textContent).toMatch(/too large/i); + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(0); + expect(backend.calls.some((c) => c.method === "PATCH")).toBe(false); + }); + + it("keeps a save-failed image visible and lets the user retry", async () => { + await openNewScene(); + backend.fail("PATCH", 500); + + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + + // Still visible locally even though persistence failed. + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1); + expect($("scene-workspace-error-text").textContent).toMatch(/could not be saved/i); + + click("scene-workspace-retry"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(true)); + + expect(backend.calls.filter((c) => c.method === "PATCH" && c.body && c.body.image)).toHaveLength(2); + }); +}); From cd8622ce2fef349194e307ba52fc394c5849de13 Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:05:02 -0500 Subject: [PATCH 3/6] fixed the issues with my original PR that closes 412 and 413 --- boneset-api/scenes.js | 154 +++++++++++++++++++++++++-------- boneset-api/scenes.test.js | 85 +++++++++++++++--- templates/js/scenes.js | 47 ++++++++-- templates/tests/scenes.test.js | 45 ++++++++++ 4 files changed, 276 insertions(+), 55 deletions(-) diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index 3111ce1..2221ffd 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -12,9 +12,32 @@ const SCENE_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a // 2MB raw file * ~4/3 base64 overhead, rounded down slightly for headroom under // the express.json() body limit once the surrounding JSON is added (Issue #412). const MAX_IMAGE_SRC_LENGTH = 2_800_000; +// Vercel caps request/response bodies at 4.5MB. A single image can pass the +// per-image cap above yet still combine with others to blow past that (PR #493 +// review: two 2MB images ~ 5.6MB of base64). This bounds the sum of every +// image's `src` on one scene, leaving headroom for the rest of the JSON +// (ids/positions/annotations/timestamps are tiny next to base64 image data). +const MAX_SCENE_IMAGES_TOTAL_LENGTH = 3_800_000; const SAFE_IMAGE_SRC_PREFIX = /^data:image\//i; class SceneStorageUnavailableError extends Error {} +class SceneLockTimeoutError extends Error {} + +/** + * Serializes concurrent operations that share the same key. Used to fix a + * lost-update race (PR #493 review): PATCH does read-the-whole-scene then + * write-the-whole-scene-back, so a rename and an image save racing each other + * would otherwise silently clobber one another depending on which wrote last. + */ +function createKeyedMutex() { + const tails = new Map(); + return function withLock(key, fn) { + const tail = tails.get(key) || Promise.resolve(); + const run = tail.then(fn, fn); + tails.set(key, run.catch(() => {})); + return run; + }; +} function isValidSceneId(sceneId) { return typeof sceneId === "string" && SCENE_ID_PATTERN.test(sceneId); @@ -131,14 +154,22 @@ function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes") } } - return { list, get, save, remove }; + return { list, get, save, remove, withLock: createKeyedMutex() }; } +// How long a PATCH will wait for another PATCH on the same scene before giving +// up (returns 409 "busy" rather than hanging indefinitely), and how long a +// held lock survives if its holder crashes before releasing it. +const SCENE_LOCK_WAIT_MS = 3000; +const SCENE_LOCK_POLL_MS = 50; +const SCENE_LOCK_TTL_MS = 5000; + // Durable shared storage for deployments. Each scene is stored under its own key, // and a set tracks every scene id so the list route doesn't need to scan keys. function createRedisSceneStore(redis, prefix = "bonebox") { const indexKey = `${prefix}:scenes`; const sceneKey = (sceneId) => `${prefix}:scene:${sceneId}`; + const lockKey = (sceneId) => `${prefix}:lock:${sceneId}`; async function get(sceneId) { return (await redis.get(sceneKey(sceneId))) || null; @@ -163,14 +194,42 @@ function createRedisSceneStore(redis, prefix = "bonebox") { return deleted > 0; } - return { list, get, save, remove }; + /** + * Distributed per-scene lock (PR #493 review fix) so a rename and an image + * save racing each other across serverless invocations can't clobber one + * another the way two unserialized read-modify-writes otherwise would. The + * TTL is a safety net if a holder crashes; the token check on release + * avoids deleting a lock we no longer own after it already expired. + */ + async function withLock(sceneId, fn) { + const key = lockKey(sceneId); + const token = crypto.randomUUID(); + const deadline = Date.now() + SCENE_LOCK_WAIT_MS; + for (;;) { + const acquired = await redis.set(key, token, { nx: true, px: SCENE_LOCK_TTL_MS }); + if (acquired) { + try { + return await fn(); + } finally { + const current = await redis.get(key); + if (current === token) await redis.del(key); + } + } + if (Date.now() >= deadline) { + throw new SceneLockTimeoutError("Scene is busy, try again"); + } + await new Promise((resolve) => setTimeout(resolve, SCENE_LOCK_POLL_MS)); + } + } + + return { list, get, save, remove, withLock }; } function createUnavailableSceneStore(reason) { const fail = async () => { throw new SceneStorageUnavailableError(reason); }; - return { list: fail, get: fail, save: fail, remove: fail }; + return { list: fail, get: fail, save: fail, remove: fail, withLock: fail }; } function hasRedisConfig(env) { @@ -210,6 +269,9 @@ function sendStoreError(res, error, message) { if (error instanceof SceneStorageUnavailableError) { return res.status(503).json({ error: error.message }); } + if (error instanceof SceneLockTimeoutError) { + return res.status(409).json({ error: error.message }); + } console.error(`${message}:`, error.message); return res.status(500).json({ error: message }); } @@ -300,52 +362,69 @@ function createScenesRouter(store = resolveSceneStore()) { * Updates a scene. Supports renaming (empty names rejected, duplicates return * 409 - Issue #423) and/or adding one newly imported image (validated and * upserted by id so a retried request can't create a duplicate - Issue #412). - * At least one of `name`/`image` must be present in the body. + * At least one of `name`/`image` must be present in the body. The whole + * read-modify-write runs under a per-scene lock (PR #493 review) so a + * rename and an image save racing each other can't silently clobber one + * another - without it, both would read the same snapshot and whichever + * wrote back last would discard the other's change entirely. */ router.patch("/:sceneId", async (req, res) => { + const { sceneId } = req.params; try { if (!req.body || (req.body.name === undefined && req.body.image === undefined)) { return res.status(400).json({ error: "name or image is required" }); } - const { sceneId } = req.params; - const scene = await store.get(sceneId); - if (!scene) { - return res.status(404).json({ error: "Scene not found" }); - } - - let changed = false; - - if (req.body.name !== undefined) { - const result = normalizeSceneName(req.body.name); - if (result.error) { - return res.status(400).json({ error: result.error }); + await store.withLock(sceneId, async () => { + const scene = await store.get(sceneId); + if (!scene) { + res.status(404).json({ error: "Scene not found" }); + return; } - const scenes = await store.list(); - if (isNameTaken(scenes, result.name, sceneId)) { - return res.status(409).json({ error: `A scene named "${result.name}" already exists` }); - } - scene.name = result.name; - changed = true; - } - if (req.body.image !== undefined) { - const result = normalizeImage(req.body.image); - if (result.error) { - return res.status(400).json({ error: result.error }); - } - const alreadyPresent = scene.images.some((img) => img.id === result.image.id); - if (!alreadyPresent) { - scene.images.push(result.image); + let changed = false; + + if (req.body.name !== undefined) { + const result = normalizeSceneName(req.body.name); + if (result.error) { + res.status(400).json({ error: result.error }); + return; + } + const scenes = await store.list(); + if (isNameTaken(scenes, result.name, sceneId)) { + res.status(409).json({ error: `A scene named "${result.name}" already exists` }); + return; + } + scene.name = result.name; changed = true; } - } - if (changed) { - scene.updatedAt = new Date().toISOString(); - await store.save(scene); - } - res.json(scene); + if (req.body.image !== undefined) { + const result = normalizeImage(req.body.image); + if (result.error) { + res.status(400).json({ error: result.error }); + return; + } + const alreadyPresent = scene.images.some((img) => img.id === result.image.id); + if (!alreadyPresent) { + const existingTotal = scene.images.reduce((sum, img) => sum + img.src.length, 0); + if (existingTotal + result.image.src.length > MAX_SCENE_IMAGES_TOTAL_LENGTH) { + res.status(400).json({ + error: "This scene is near its total image size limit; remove an image before adding another", + }); + return; + } + scene.images.push(result.image); + changed = true; + } + } + + if (changed) { + scene.updatedAt = new Date().toISOString(); + await store.save(scene); + } + res.json(scene); + }); } catch (error) { sendStoreError(res, error, "Failed to update scene"); } @@ -379,4 +458,5 @@ module.exports = { normalizeImage, DEFAULT_SCENE_NAME, MAX_IMAGE_SRC_LENGTH, + MAX_SCENE_IMAGES_TOTAL_LENGTH, }; diff --git a/boneset-api/scenes.test.js b/boneset-api/scenes.test.js index 35ebd46..e2892f3 100644 --- a/boneset-api/scenes.test.js +++ b/boneset-api/scenes.test.js @@ -12,6 +12,35 @@ const { MAX_IMAGE_SRC_LENGTH, } = require("./scenes"); +function validImage(overrides = {}) { + return { + id: crypto.randomUUID(), + src: "data:image/png;base64,AAAA", + x: 10, + y: 20, + width: 100, + height: 50, + ...overrides, + }; +} + +// Widens the read-save race window deterministically so a concurrency test +// doesn't depend on machine speed or backend internals (PR #493 review). +function withArtificialDelay(store, ms) { + return { + ...store, + async get(sceneId) { + const result = await store.get(sceneId); + await new Promise((resolve) => setTimeout(resolve, ms)); + return result; + }, + async save(scene) { + await new Promise((resolve) => setTimeout(resolve, ms)); + return store.save(scene); + }, + }; +} + // In-memory stand-in for the subset of the @upstash/redis client the store uses. // Values round-trip through JSON like the real client's automatic serialization. function createFakeRedis() { @@ -24,7 +53,8 @@ function createFakeRedis() { async mget(...keys) { return keys.map((key) => (strings.has(key) ? JSON.parse(strings.get(key)) : null)); }, - async set(key, value) { + async set(key, value, options = {}) { + if (options.nx && strings.has(key)) return null; strings.set(key, JSON.stringify(value)); return "OK"; }, @@ -213,18 +243,6 @@ describe.each(backends)("Scenes API ($name)", (backend) => { // Issue #412: Upload and Store an Imported Image describe("PATCH /api/scenes/:sceneId with an image - Issue 412", () => { - function validImage(overrides = {}) { - return { - id: crypto.randomUUID(), - src: "data:image/png;base64,AAAA", - x: 10, - y: 20, - width: 100, - height: 50, - ...overrides, - }; - } - it("adds a newly imported image to the scene", async () => { const created = await createScene(); const image = validImage(); @@ -339,6 +357,47 @@ describe.each(backends)("Scenes API ($name)", (backend) => { expect(response.body.name).toBe("Renamed"); expect(response.body.images).toEqual([image]); }); + + // PR #493 review: a scene can have multiple images that each pass the + // per-image cap yet together exceed Vercel's 4.5MB request/response cap. + it("rejects an image that would push the scene over its total image size budget", async () => { + const created = await createScene(); + const first = validImage({ src: `data:image/png;base64,${"A".repeat(MAX_IMAGE_SRC_LENGTH - 100)}` }); + const firstResponse = await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: first }); + expect(firstResponse.statusCode).toBe(200); + + const second = validImage({ src: `data:image/png;base64,${"A".repeat(MAX_IMAGE_SRC_LENGTH - 100)}` }); + const response = await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: second }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toMatch(/size limit/i); + + const reloaded = await request(app).get(`/api/scenes/${created.body.id}`); + expect(reloaded.body.images).toEqual([first]); + }); + }); + + // PR #493 review: a rename and an image save racing each other must not + // clobber one another. An artificial delay widens the read-save window so + // this reproduces deterministically instead of depending on real timing. + describe("PATCH /api/scenes/:sceneId concurrency - PR #493 review", () => { + it("does not lose a concurrent rename or image save", async () => { + const slowApp = buildApp(withArtificialDelay(env.newStore(), 30)); + const created = await request(slowApp).post("/api/scenes").send({}); + const image = validImage(); + + const [renameResponse, imageResponse] = await Promise.all([ + request(slowApp).patch(`/api/scenes/${created.body.id}`).send({ name: "Renamed" }), + request(slowApp).patch(`/api/scenes/${created.body.id}`).send({ image }), + ]); + + expect(renameResponse.statusCode).toBe(200); + expect(imageResponse.statusCode).toBe(200); + + const final = await request(slowApp).get(`/api/scenes/${created.body.id}`); + expect(final.body.name).toBe("Renamed"); + expect(final.body.images).toEqual([image]); + }); }); // Issue #424: Delete a Scene diff --git a/templates/js/scenes.js b/templates/js/scenes.js index 5071b9f..1d78929 100644 --- a/templates/js/scenes.js +++ b/templates/js/scenes.js @@ -85,7 +85,9 @@ export function describeError(error, context) { case 404: return "This scene is no longer available. It may have been deleted."; case 409: - return "Another scene already uses that name. Choose a different name."; + return /busy/i.test(error.message) + ? "The scene is busy right now (another save is in progress). Try again in a moment." + : "Another scene already uses that name. Choose a different name."; case 429: return "Too many requests. Wait a moment and try again."; case 503: @@ -117,6 +119,11 @@ const IMPORT_OFFSET_STEP = 24; // once base64's ~4/3 overhead is applied - checked client-side for immediate feedback, // but the server independently re-checks its own cap too (never trust the client alone). const MAX_IMPORT_FILE_SIZE = 2 * 1024 * 1024; +// Mirrors the backend's MAX_SCENE_IMAGES_TOTAL_LENGTH (PR #493 review: two +// individually-valid images can still combine to blow past Vercel's 4.5MB +// request/response cap) - same "check and skip clearly before uploading" +// philosophy as the per-file checks above; the server is the real enforcement. +const MAX_SCENE_IMAGES_TOTAL_LENGTH = 3_800_000; function readFileAsDataUrl(file) { return new Promise((resolve, reject) => { @@ -461,14 +468,28 @@ async function handleDelete() { */ async function retryImageSaves(sceneId, images) { const stillFailing = []; + let savedCount = 0; for (const image of images) { try { await saveImageToScene(sceneId, image); + savedCount += 1; } catch { stillFailing.push(image); } } + // Reflects the scene list's count from what's actually confirmed saved + // (not the optimistic local count), regardless of whether this scene is + // still the one open - it was going stale until the next full reload + // otherwise (PR #493 review). + if (savedCount > 0) { + const summary = state.scenes.find((s) => s.id === sceneId); + if (summary) { + summary.imageCount += savedCount; + renderLibrary(); + } + } + if (stillFailing.length === 0) { hideWorkspaceError(); return; @@ -490,6 +511,11 @@ async function retryImageSaves(sceneId, images) { async function handleImportFiles(fileList) { if (!state.active || !fileList || fileList.length === 0) return; const sceneId = state.active.id; + // Captured once up front so later iterations don't depend on state.active, + // which can change mid-loop if the user opens a different scene while an + // earlier file is still being read (PR #493 review). + const baseImageCount = state.active.images.length; + let totalSrcLength = state.active.images.reduce((sum, img) => sum + img.src.length, 0); dom.importError.hidden = true; dom.importError.textContent = ""; @@ -509,12 +535,19 @@ async function handleImportFiles(fileList) { try { const dataUrl = await readFileAsDataUrl(file); + if (totalSrcLength + dataUrl.length > MAX_SCENE_IMAGES_TOTAL_LENGTH) { + skipped.push(`${file.name} (scene is near its image size limit)`); + continue; + } const natural = await loadImageDimensions(dataUrl); const { width, height } = scaleToFit(natural.width, natural.height, MAX_IMPORT_DIMENSION); - const offset = IMPORT_OFFSET_STEP * (state.active.images.length + 1); + const offset = IMPORT_OFFSET_STEP * (baseImageCount + added.length + 1); const image = { id: crypto.randomUUID(), src: dataUrl, x: offset, y: offset, width, height }; - state.active.images.push(image); added.push(image); + totalSrcLength += dataUrl.length; + if (state.active && state.active.id === sceneId) { + state.active.images.push(image); + } } catch { skipped.push(`${file.name} (could not be read)`); } @@ -528,8 +561,12 @@ async function handleImportFiles(fileList) { } if (added.length > 0) { - renderWorkspace(); - announce(`${plural(added.length, "image")} added to the scene.`); + if (state.active && state.active.id === sceneId) { + renderWorkspace(); + announce(`${plural(added.length, "image")} added to the scene.`); + } + // Always persists to the scene the import actually started on, even + // if the user has since switched to viewing a different one. await retryImageSaves(sceneId, added); } } diff --git a/templates/tests/scenes.test.js b/templates/tests/scenes.test.js index b9dc7f7..7809405 100644 --- a/templates/tests/scenes.test.js +++ b/templates/tests/scenes.test.js @@ -537,4 +537,49 @@ describe("Scene editor: importing images - Issue 411", () => { expect(backend.calls.filter((c) => c.method === "PATCH" && c.body && c.body.image)).toHaveLength(2); }); + + // PR #493 review: the scene list's image count was only ever refreshed by + // a full re-list, so it kept showing "0 images" after an import until the + // page was reloaded. + it("updates the scene list's image count after an import without waiting for a reload", async () => { + await openNewScene(); + const title = $("scene-title").textContent; + + setFiles([{ name: "ilium.png", type: "image/png" }]); + await waitFor(() => expect($("scene-meta").textContent).toContain("1 image")); + + expect(listButton(title).textContent).toMatch(/1 image/); + }); + + // PR #493 review: switching scenes while an earlier file in the same + // import was still being read could land the image on whichever scene + // happened to be open when the read finished, not the one import started on. + it("does not add an imported image to the wrong scene when the user switches scenes mid-import", async () => { + backend.seed({ name: "Other" }); + await openNewScene(); + const originalTitle = $("scene-title").textContent; + + // Slowed down so switching scenes reliably finishes first. + window.FileReader = class { + readAsDataURL(_file) { + setTimeout(() => { + this.result = "data:image/png;base64,AAAA"; + if (this.onload) this.onload(); + }, 30); + } + }; + + setFiles([{ name: "ilium.png", type: "image/png" }]); + await openByName("Other"); + + // Give the slow read time to finish while "Other" is the open scene, + // then check state directly - a DOM check alone wouldn't catch a push + // onto the wrong in-memory scene unless something happens to re-render + // it afterward, which isn't guaranteed. + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(scenesModule.getActiveScene().images).toHaveLength(0); + + await openByName(originalTitle); + await waitFor(() => expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1)); + }); }); From 44ab31f118842c0ffcbb6599a01e7f382d0d75ab Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Fri, 2 Oct 2026 10:27:42 -0500 Subject: [PATCH 4/6] fixed failing CodeQL test --- boneset-api/scenes.js | 11 ++++++++++- boneset-api/scenes.test.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index 2221ffd..18e7e12 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -106,7 +106,16 @@ function toSummary(scene) { // Local development only: one JSON file per scene. function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes")) { - const scenePath = (sceneId) => path.join(scenesDir, `${sceneId}.json`); + // Re-validated here, not just trusted from the route's own check (CodeQL: + // "uncontrolled data used in path expression") - this function takes a bare + // sceneId and builds a filesystem path from it, so the guard has to live + // at the point the path is built, not in a caller it can't see. + const scenePath = (sceneId) => { + if (!isValidSceneId(sceneId)) { + throw new Error("Invalid sceneId"); + } + return path.join(scenesDir, `${sceneId}.json`); + }; async function ensureDir() { await fs.mkdir(scenesDir, { recursive: true }); diff --git a/boneset-api/scenes.test.js b/boneset-api/scenes.test.js index e2892f3..f26620a 100644 --- a/boneset-api/scenes.test.js +++ b/boneset-api/scenes.test.js @@ -420,6 +420,34 @@ describe.each(backends)("Scenes API ($name)", (backend) => { }); }); +// CodeQL flagged the file store's path construction as taking uncontrolled +// data: this proves it now re-validates the id itself rather than trusting a +// caller, even though the route-level `router.param` check already blocks +// malformed ids from ever reaching the store in normal operation. +describe("createFileSceneStore path safety", () => { + let dir; + let store; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), "bonebox-scenes-pathsafety-")); + store = createFileSceneStore(dir); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it("rejects a traversal attempt instead of reading outside the scenes directory", async () => { + await expect(store.get("../../etc/passwd")).rejects.toThrow(/invalid sceneid/i); + await expect(store.remove("../../etc/passwd")).rejects.toThrow(/invalid sceneid/i); + }); + + it("rejects saving a scene with a malformed id", async () => { + await expect(store.save({ id: "../../evil", name: "x", images: [], annotations: [] })) + .rejects.toThrow(/invalid sceneid/i); + }); +}); + describe("resolveSceneStore", () => { it("refuses to store scenes on Vercel when Redis is not configured", async () => { const consoleError = jest.spyOn(console, "error").mockImplementation(() => {}); From 5165c37e11d2b5c10ad414d77376e985ca04dcca Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:26:19 -0500 Subject: [PATCH 5/6] fixing codeQL issue --- boneset-api/scenes.js | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index 18e7e12..cf3404a 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -106,15 +106,26 @@ function toSummary(scene) { // Local development only: one JSON file per scene. function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes")) { - // Re-validated here, not just trusted from the route's own check (CodeQL: - // "uncontrolled data used in path expression") - this function takes a bare - // sceneId and builds a filesystem path from it, so the guard has to live - // at the point the path is built, not in a caller it can't see. + // Re-validated here, not just trusted from the route's own check (CodeQL + // js/path-injection: "uncontrolled data used in path expression") - this + // function takes a bare sceneId and builds a filesystem path from it, so + // the guard has to live at the point the path is built, not in a caller + // it can't see. The allowlist regex alone isn't a pattern CodeQL's + // path-injection query recognizes as a barrier, so this also resolves the + // path and proves it stays inside scenesDir - the same resolve+relative + // containment check server.js's readJSON already uses for bone/boneset + // lookups, which CodeQL does recognize. const scenePath = (sceneId) => { if (!isValidSceneId(sceneId)) { throw new Error("Invalid sceneId"); } - return path.join(scenesDir, `${sceneId}.json`); + const resolved = path.resolve(scenesDir, `${sceneId}.json`); + const relative = path.relative(scenesDir, resolved); + const staysInsideScenesDir = relative && !relative.startsWith("..") && !path.isAbsolute(relative); + if (!staysInsideScenesDir) { + throw new Error("Invalid sceneId"); + } + return resolved; }; async function ensureDir() { @@ -357,7 +368,12 @@ function createScenesRouter(store = resolveSceneStore()) { */ router.get("/:sceneId", async (req, res) => { try { - const scene = await store.get(req.params.sceneId); + const { sceneId } = req.params; + if (!isValidSceneId(sceneId)) { + return res.status(400).json({ error: "Invalid sceneId" }); + } + + const scene = await store.get(sceneId); if (!scene) { return res.status(404).json({ error: "Scene not found" }); } @@ -380,6 +396,9 @@ function createScenesRouter(store = resolveSceneStore()) { router.patch("/:sceneId", async (req, res) => { const { sceneId } = req.params; try { + if (!isValidSceneId(sceneId)) { + return res.status(400).json({ error: "Invalid sceneId" }); + } if (!req.body || (req.body.name === undefined && req.body.image === undefined)) { return res.status(400).json({ error: "name or image is required" }); } @@ -444,7 +463,12 @@ function createScenesRouter(store = resolveSceneStore()) { */ router.delete("/:sceneId", async (req, res) => { try { - const deleted = await store.remove(req.params.sceneId); + const { sceneId } = req.params; + if (!isValidSceneId(sceneId)) { + return res.status(400).json({ error: "Invalid sceneId" }); + } + + const deleted = await store.remove(sceneId); if (!deleted) { return res.status(404).json({ error: "Scene not found" }); } From 4d145d939483d53b7745d9b666c06578c84fe310 Mon Sep 17 00:00:00 2001 From: mayokunl <153923029+mayokunl@users.noreply.github.com> Date: Wed, 7 Oct 2026 13:36:13 -0500 Subject: [PATCH 6/6] trying again to fix the vulnerabilites --- boneset-api/scenes.js | 60 ++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index cf3404a..01d940b 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -106,35 +106,34 @@ function toSummary(scene) { // Local development only: one JSON file per scene. function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes")) { - // Re-validated here, not just trusted from the route's own check (CodeQL - // js/path-injection: "uncontrolled data used in path expression") - this - // function takes a bare sceneId and builds a filesystem path from it, so - // the guard has to live at the point the path is built, not in a caller - // it can't see. The allowlist regex alone isn't a pattern CodeQL's - // path-injection query recognizes as a barrier, so this also resolves the - // path and proves it stays inside scenesDir - the same resolve+relative - // containment check server.js's readJSON already uses for bone/boneset - // lookups, which CodeQL does recognize. - const scenePath = (sceneId) => { - if (!isValidSceneId(sceneId)) { - throw new Error("Invalid sceneId"); - } - const resolved = path.resolve(scenesDir, `${sceneId}.json`); - const relative = path.relative(scenesDir, resolved); - const staysInsideScenesDir = relative && !relative.startsWith("..") && !path.isAbsolute(relative); - if (!staysInsideScenesDir) { - throw new Error("Invalid sceneId"); - } - return resolved; - }; + // Not tainted by user input - just the configured base directory, resolved + // once so every function below checks containment against the same value. + const resolvedScenesDir = path.resolve(scenesDir); async function ensureDir() { await fs.mkdir(scenesDir, { recursive: true }); } + // CodeQL (js/path-injection) flagged fs calls fed by a sceneId-derived path + // even though the route layer already validates it: a sanitizer defined in + // a separate helper function wasn't recognized as covering a different + // function's fs call. So this check is duplicated inline in get/save/remove + // below - right next to the fs call it guards, with nothing delegated - and + // combines two independent proofs of safety on the exact value passed to + // fs: (1) sceneId can only be a bare UUID (no "/", "\", or ".." possible), + // and (2) the resolved path is explicitly re-verified to still be inside + // resolvedScenesDir before it's used, so even a hypothetical regex bypass + // could not escape this directory. async function get(sceneId) { + if (!isValidSceneId(sceneId)) { + throw new Error("Invalid sceneId"); + } + const target = path.resolve(resolvedScenesDir, `${sceneId}.json`); + if (!target.startsWith(resolvedScenesDir + path.sep)) { + throw new Error("Invalid sceneId"); + } try { - const raw = await fs.readFile(scenePath(sceneId), "utf8"); + const raw = await fs.readFile(target, "utf8"); return JSON.parse(raw); } catch (error) { if (error.code === "ENOENT") return null; @@ -156,8 +155,14 @@ function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes") } async function save(scene) { + if (!isValidSceneId(scene.id)) { + throw new Error("Invalid sceneId"); + } + const target = path.resolve(resolvedScenesDir, `${scene.id}.json`); + if (!target.startsWith(resolvedScenesDir + path.sep)) { + throw new Error("Invalid sceneId"); + } await ensureDir(); - const target = scenePath(scene.id); const tmp = `${target}.${crypto.randomUUID()}.tmp`; await fs.writeFile(tmp, JSON.stringify(scene, null, 2), "utf8"); await fs.rename(tmp, target); @@ -165,8 +170,15 @@ function createFileSceneStore(scenesDir = path.join(__dirname, "data", "scenes") } async function remove(sceneId) { + if (!isValidSceneId(sceneId)) { + throw new Error("Invalid sceneId"); + } + const target = path.resolve(resolvedScenesDir, `${sceneId}.json`); + if (!target.startsWith(resolvedScenesDir + path.sep)) { + throw new Error("Invalid sceneId"); + } try { - await fs.unlink(scenePath(sceneId)); + await fs.unlink(target); return true; } catch (error) { if (error.code === "ENOENT") return false;