diff --git a/boneset-api/scenes.js b/boneset-api/scenes.js index b425179a..eb1fe3b9 100644 --- a/boneset-api/scenes.js +++ b/boneset-api/scenes.js @@ -9,8 +9,35 @@ 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; +// 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); @@ -30,6 +57,106 @@ 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 }, + }; +} + +const UPDATABLE_IMAGE_FIELDS = [ + "x", "y", "width", "height", "rotation", "flipX", "flipY", "cropX", "cropY", "cropWidth", "cropHeight", +]; +const CROP_FIELDS = ["cropX", "cropY", "cropWidth", "cropHeight"]; + +/** + * Validates and applies a partial update to an already-saved image (Issues + * #413-417: position, resize, rotate, flip, crop). Only the keys present in + * `fields` are touched - everything else on `existingImage` (including `id` + * and `src`, which this never accepts) carries over unchanged. Validation + * runs against the *merged* result, not the raw patch in isolation, because + * some constraints (the crop rect fitting within width/height) depend on the + * final value of a field that might be changing in the very same request. + * @param {object} existingImage + * @param {object} fields + * @returns {{ error: string } | { image: object }} + */ +function applyImagePatch(existingImage, fields) { + if (!fields || typeof fields !== "object") { + return { error: "fields must be an object" }; + } + const unknownField = Object.keys(fields).find((key) => !UPDATABLE_IMAGE_FIELDS.includes(key)); + if (unknownField) { + return { error: `Unknown image field: ${unknownField}` }; + } + + const merged = { ...existingImage, ...fields }; + + if (![merged.x, merged.y, merged.width, merged.height].every(isFiniteNumber)) { + return { error: "x, y, width, and height must be numbers" }; + } + if (merged.width <= 0 || merged.height <= 0) { + return { error: "width and height must be greater than 0" }; + } + if (merged.rotation !== undefined && !isFiniteNumber(merged.rotation)) { + return { error: "rotation must be a number" }; + } + if (merged.flipX !== undefined && typeof merged.flipX !== "boolean") { + return { error: "flipX must be a boolean" }; + } + if (merged.flipY !== undefined && typeof merged.flipY !== "boolean") { + return { error: "flipY must be a boolean" }; + } + + const hasCropField = CROP_FIELDS.some((key) => merged[key] !== undefined); + if (hasCropField) { + if (!CROP_FIELDS.every((key) => isFiniteNumber(merged[key]))) { + return { error: "cropX, cropY, cropWidth, and cropHeight must all be numbers when cropping" }; + } + if (merged.cropWidth <= 0 || merged.cropHeight <= 0) { + return { error: "cropWidth and cropHeight must be greater than 0" }; + } + if ( + merged.cropX < 0 || merged.cropY < 0 || + merged.cropX + merged.cropWidth > merged.width || + merged.cropY + merged.cropHeight > merged.height + ) { + return { error: "The crop area must fit within the image" }; + } + } + + return { image: merged }; +} + function toSummary(scene) { return { id: scene.id, @@ -43,15 +170,34 @@ 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`); + // 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; @@ -73,8 +219,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); @@ -82,8 +234,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; @@ -91,14 +250,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; @@ -123,14 +290,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) { @@ -170,6 +365,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 }); } @@ -246,7 +444,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" }); } @@ -257,35 +460,130 @@ 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), adding one newly imported image (validated and upserted + * by id so a retried request can't create a duplicate - Issue #412), updating + * an existing image's position/size/rotation/flip/crop (`updateImage` - Issues + * #413-417), removing one image by id (`removeImageId` - a no-op if already + * gone, so a retry is always safe - Issue #418), and reordering images + * (`reorderImageIds` - must be a permutation of the scene's current image ids + * - Issue #419). At least one of these fields must be present in the body. + * The whole read-modify-write + * runs under a per-scene lock (PR #493 review) so concurrent updates (e.g. a + * rename racing an image save) 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) { - return res.status(400).json({ error: "name is required" }); + if (!isValidSceneId(sceneId)) { + return res.status(400).json({ error: "Invalid sceneId" }); } - const result = normalizeSceneName(req.body.name); - if (result.error) { - return res.status(400).json({ error: result.error }); + const hasKnownField = req.body && ( + req.body.name !== undefined || + req.body.image !== undefined || + req.body.updateImage !== undefined || + req.body.removeImageId !== undefined || + req.body.reorderImageIds !== undefined + ); + if (!hasKnownField) { + return res.status(400).json({ + error: "name, image, updateImage, removeImageId, or reorderImageIds is required", + }); } - const { sceneId } = req.params; - const scene = await store.get(sceneId); - if (!scene) { - return res.status(404).json({ error: "Scene not found" }); - } + 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` }); - } + 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; + } - scene.name = result.name; - 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 (req.body.updateImage !== undefined) { + const { id, ...fields } = req.body.updateImage || {}; + const target = scene.images.find((img) => img.id === id); + if (!target) { + res.status(404).json({ error: "Image not found" }); + return; + } + const result = applyImagePatch(target, fields); + if (result.error) { + res.status(400).json({ error: result.error }); + return; + } + Object.assign(target, result.image); + changed = true; + } + + if (req.body.removeImageId !== undefined) { + const before = scene.images.length; + scene.images = scene.images.filter((img) => img.id !== req.body.removeImageId); + if (scene.images.length !== before) { + changed = true; + } + } + + if (req.body.reorderImageIds !== undefined) { + const ids = req.body.reorderImageIds; + const currentIds = scene.images.map((img) => img.id); + const isSamePermutation = + Array.isArray(ids) && + ids.length === currentIds.length && + [...ids].sort().join() === [...currentIds].sort().join(); + if (!isSamePermutation) { + res.status(400).json({ error: "reorderImageIds must match the scene's current images" }); + return; + } + scene.images = ids.map((id) => scene.images.find((img) => img.id === id)); + 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"); } }); @@ -294,7 +592,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" }); } @@ -314,5 +617,9 @@ module.exports = { resolveSceneStore, isValidSceneId, normalizeSceneName, + normalizeImage, + applyImagePatch, 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 4b74bcc7..2ed674f1 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,8 +9,38 @@ const { createFileSceneStore, createRedisSceneStore, resolveSceneStore, + 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() { @@ -22,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"; }, @@ -45,7 +77,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 +241,402 @@ 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", () => { + 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]); + }); + + // 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]); + }); + }); + + // Issues #413-417: Position, Resize, Rotate, Flip, Crop an Image + describe("PATCH /api/scenes/:sceneId with updateImage - Issues 413-417", () => { + async function createSceneWithImage(overrides = {}) { + const created = await createScene(); + const image = validImage(overrides); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image }); + return { sceneId: created.body.id, image }; + } + + it("updates position and size (Issues 413, 414)", async () => { + const { sceneId, image } = await createSceneWithImage(); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, x: 40, y: 60, width: 200, height: 100 } }); + + expect(response.statusCode).toBe(200); + expect(response.body.images[0]).toMatchObject({ x: 40, y: 60, width: 200, height: 100 }); + }); + + it("updates rotation (Issue 415)", async () => { + const { sceneId, image } = await createSceneWithImage(); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, rotation: 90 } }); + + expect(response.statusCode).toBe(200); + expect(response.body.images[0].rotation).toBe(90); + }); + + it("updates flipX/flipY (Issue 416)", async () => { + const { sceneId, image } = await createSceneWithImage(); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, flipX: true, flipY: true } }); + + expect(response.statusCode).toBe(200); + expect(response.body.images[0]).toMatchObject({ flipX: true, flipY: true }); + }); + + it("sets a crop rect that fits within the image (Issue 417)", async () => { + const { sceneId, image } = await createSceneWithImage({ width: 100, height: 50 }); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, cropX: 10, cropY: 5, cropWidth: 50, cropHeight: 25 } }); + + expect(response.statusCode).toBe(200); + expect(response.body.images[0]).toMatchObject({ cropX: 10, cropY: 5, cropWidth: 50, cropHeight: 25 }); + }); + + it("rejects a crop rect that doesn't fit within the image", async () => { + const { sceneId, image } = await createSceneWithImage({ width: 100, height: 50 }); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, cropX: 60, cropY: 0, cropWidth: 50, cropHeight: 25 } }); + + expect(response.statusCode).toBe(400); + expect(response.body.error).toMatch(/fit within the image/i); + }); + + it("rejects an incomplete crop rect (all four fields required together)", async () => { + const { sceneId, image } = await createSceneWithImage(); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, cropX: 10, cropY: 5, cropWidth: 20 } }); + + expect(response.statusCode).toBe(400); + }); + + it("validates crop against a width/height changing in the same request", async () => { + const { sceneId, image } = await createSceneWithImage({ width: 100, height: 50 }); + + const response = await request(app).patch(`/api/scenes/${sceneId}`).send({ + updateImage: { id: image.id, width: 40, cropX: 0, cropY: 0, cropWidth: 50, cropHeight: 25 }, + }); + + expect(response.statusCode).toBe(400); + }); + + it("rejects an unknown field", async () => { + const { sceneId, image } = await createSceneWithImage(); + + const response = await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, src: "data:image/png;base64,EVIL" } }); + + expect(response.statusCode).toBe(400); + }); + + it("never lets updateImage change id or src", async () => { + const { sceneId, image } = await createSceneWithImage(); + + await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, x: 5 } }); + + const reloaded = await request(app).get(`/api/scenes/${sceneId}`); + expect(reloaded.body.images[0].id).toBe(image.id); + expect(reloaded.body.images[0].src).toBe(image.src); + }); + + it("keeps updates after the scene is reloaded", async () => { + const { sceneId, image } = await createSceneWithImage(); + await request(app) + .patch(`/api/scenes/${sceneId}`) + .send({ updateImage: { id: image.id, rotation: 45 } }); + + const reloaded = await request(app).get(`/api/scenes/${sceneId}`); + expect(reloaded.body.images[0].rotation).toBe(45); + }); + + it("returns 404 when updating an image that doesn't exist", async () => { + const created = await createScene(); + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ updateImage: { id: crypto.randomUUID(), rotation: 10 } }); + + expect(response.statusCode).toBe(404); + }); + }); + + // Issue #418: Remove an Image from a Scene + describe("PATCH /api/scenes/:sceneId with removeImageId - Issue 418", () => { + it("removes the requested image and leaves the others intact", async () => { + const created = await createScene(); + const keep = validImage(); + const remove = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: keep }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: remove }); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ removeImageId: remove.id }); + + expect(response.statusCode).toBe(200); + expect(response.body.images).toEqual([keep]); + }); + + it("keeps the removal after the scene is reloaded", async () => { + const created = await createScene(); + const image = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ removeImageId: image.id }); + + const reloaded = await request(app).get(`/api/scenes/${created.body.id}`); + expect(reloaded.body.images).toEqual([]); + }); + + it("is a no-op success when the image is already gone (safe to retry)", async () => { + const created = await createScene(); + const image = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ removeImageId: image.id }); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ removeImageId: image.id }); + + expect(response.statusCode).toBe(200); + expect(response.body.images).toEqual([]); + }); + + it("returns 404 when removing from an unknown scene", async () => { + const response = await request(app) + .patch(`/api/scenes/${UNKNOWN_ID}`) + .send({ removeImageId: crypto.randomUUID() }); + + expect(response.statusCode).toBe(404); + }); + }); + + // Issue #419: Reorder Image Layers + describe("PATCH /api/scenes/:sceneId with reorderImageIds - Issue 419", () => { + it("reorders images to match the given id order", async () => { + const created = await createScene(); + const first = validImage(); + const second = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: first }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: second }); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ reorderImageIds: [second.id, first.id] }); + + expect(response.statusCode).toBe(200); + expect(response.body.images).toEqual([second, first]); + }); + + it("keeps the new order after the scene is reloaded", async () => { + const created = await createScene(); + const first = validImage(); + const second = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: first }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: second }); + await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ reorderImageIds: [second.id, first.id] }); + + const reloaded = await request(app).get(`/api/scenes/${created.body.id}`); + expect(reloaded.body.images.map((img) => img.id)).toEqual([second.id, first.id]); + }); + + it("rejects an order that drops an existing image", async () => { + const created = await createScene(); + const first = validImage(); + const second = validImage(); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: first }); + await request(app).patch(`/api/scenes/${created.body.id}`).send({ image: second }); + + const response = await request(app) + .patch(`/api/scenes/${created.body.id}`) + .send({ reorderImageIds: [first.id] }); + + expect(response.statusCode).toBe(400); + + const reloaded = await request(app).get(`/api/scenes/${created.body.id}`); + expect(reloaded.body.images).toEqual([first, second]); + }); + + it("rejects an order that includes an id not on the scene", async () => { + const created = await createScene(); + const first = 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({ reorderImageIds: [first.id, crypto.randomUUID()] }); + + expect(response.statusCode).toBe(400); + }); + }); + // Issue #424: Delete a Scene describe("DELETE /api/scenes/:sceneId - Issue 424", () => { it("deletes only the requested scene", async () => { @@ -227,6 +657,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(() => {}); diff --git a/boneset-api/server.js b/boneset-api/server.js index e995e4df..55bac23a 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 bc496a17..d62ae011 100644 --- a/templates/boneset.html +++ b/templates/boneset.html @@ -218,16 +218,36 @@

    + +
    + + +

    This scene is blank.

    Images and annotations added to this scene will appear here.

    +
    diff --git a/templates/js/navigation.js b/templates/js/navigation.js index 8d5210db..a895885f 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/js/sceneCanvas.js b/templates/js/sceneCanvas.js index a2ffdfa3..eb39c40d 100644 --- a/templates/js/sceneCanvas.js +++ b/templates/js/sceneCanvas.js @@ -16,7 +16,14 @@ 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; +// Order matches renderImage's own `corners` array: [TL, TR, BR, BL]. +const RESIZE_HANDLE_CORNERS = ["nw", "ne", "se", "sw"]; +const RESIZE_HANDLE_SIZE = 10; let markerCount = 0; @@ -55,7 +62,28 @@ function el(name, attrs = {}) { return node; } -function renderImage(image) { +/** + * The visible sub-rectangle of an image's own local box (0,0 to width,height) + * - Issue #417. Defaults to the full box when no crop fields are set, so an + * uncropped image behaves exactly as before. + */ +function cropRectFor(image) { + const hasCrop = [image.cropX, image.cropY, image.cropWidth, image.cropHeight].every(isNumber); + return hasCrop + ? { x: image.cropX, y: image.cropY, width: image.cropWidth, height: image.cropHeight } + : { x: 0, y: 0, width: image.width, height: image.height }; +} + +/** + * @param {object} image + * @param {number} index + * @param {SVGDefsElement} defs + * @param {{ skipClip?: boolean }} [options] - skipClip shows the full, + * unclipped image regardless of any saved crop - used only while actively + * editing the crop (Issue #417), so the user can see the whole source to + * choose a region from. + */ +function renderImage(image, index, defs, { skipClip = false } = {}) { 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; @@ -72,7 +100,29 @@ function renderImage(image) { transforms.push(`translate(${cx} ${cy}) scale(${sx} ${sy}) translate(${-cx} ${-cy})`); } - const node = el("image", { + const crop = cropRectFor(image); + const isCropped = + !skipClip && (crop.x !== 0 || crop.y !== 0 || crop.width !== image.width || crop.height !== image.height); + + let clipId; + if (isCropped) { + markerCount += 1; + clipId = `scene-image-clip-${markerCount}`; + const clipPath = el("clipPath", { id: clipId }); + // In the SAME pre-rotation, absolute coordinates as the image's own + // x/y/width/height below - both live inside the same transformed + // when rotated/flipped, so the crop window rotates/flips WITH the + // image as one rigid unit, not independently of it. + clipPath.appendChild(el("rect", { + x: image.x + crop.x, + y: image.y + crop.y, + width: crop.width, + height: crop.height, + })); + defs.appendChild(clipPath); + } + + const imageNode = el("image", { href: image.src, x: image.x, y: image.y, @@ -80,15 +130,49 @@ function renderImage(image) { height: image.height, preserveAspectRatio: "none", opacity: isNumber(image.opacity) ? image.opacity : undefined, - transform: transforms.length ? transforms.join(" ") : undefined, + "clip-path": clipId ? `url(#${clipId})` : undefined, + class: "scene-image-object", + "data-scene-image-index": index, }); - const corners = [ + + // The transform only moves to a wrapping when a clip-path is also in + // play (so the clip rotates/flips with the image, see above) - otherwise + // it stays directly on the , unchanged from before #417. + let node = imageNode; + if (transforms.length) { + if (clipId) { + const group = el("g", { transform: transforms.join(" ") }); + group.appendChild(imageNode); + node = group; + } else { + imageNode.setAttribute("transform", transforms.join(" ")); + } + } + + const fullCorners = [ [image.x, image.y], [image.x + image.width, image.y], [image.x + image.width, image.y + image.height], [image.x, image.y + image.height], ]; - return { node, bounds: isNumber(image.rotation) ? rotatePoints(corners, image.rotation, cx, cy) : corners }; + const cropCorners = [ + [image.x + crop.x, image.y + crop.y], + [image.x + crop.x + crop.width, image.y + crop.y], + [image.x + crop.x + crop.width, image.y + crop.y + crop.height], + [image.x + crop.x, image.y + crop.y + crop.height], + ]; + const visibleCorners = isCropped ? cropCorners : fullCorners; + const rotation = image.rotation; + + return { + node, + // The visible (cropped, if applicable) region - used for the normal + // selection outline and the canvas's auto-sizing. + bounds: isNumber(rotation) ? rotatePoints(visibleCorners, rotation, cx, cy) : visibleCorners, + // The current crop rect regardless of skipClip - used only while + // actively editing the crop, to draw its own handles/outline. + cropBounds: isNumber(rotation) ? rotatePoints(cropCorners, rotation, cx, cy) : cropCorners, + }; } function rotatePoints(points, degrees, cx, cy) { @@ -178,16 +262,24 @@ 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, cropMode?: boolean }} [options] - cropMode + * shows the selected image uncropped with handles on its *draft* crop rect + * instead of the normal selection outline/resize handles (Issue #417). * @returns {{ svg: SVGSVGElement, rendered: number, unsupported: number, width: number, height: number }} */ -export function renderScene(scene) { +export function renderScene(scene, { selectedIndex, cropMode = false } = {}) { 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 +291,7 @@ export function renderScene(scene) { const place = (result, layer) => { if (!result) { unsupported += 1; - return; + return null; } layer.appendChild(result.node); rendered += 1; @@ -209,9 +301,37 @@ 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 isEditingCrop = cropMode && index === selectedIndex; + const result = place(renderImage(image, index, defs, { skipClip: isEditingCrop }), imageLayer); + if (result && index === selectedIndex) { + // While actively editing the crop, the outline/handles track the + // *draft* crop rect instead of the image's visible bounds, so + // they can be dragged inward from the full (now unclipped) image. + const handleBounds = isEditingCrop ? result.cropBounds : result.bounds; + selectionLayer.appendChild(el("polygon", { + class: isEditingCrop ? "scene-crop-outline" : "scene-selection-outline", + points: handleBounds.map((p) => p.join(",")).join(" "), + "pointer-events": "none", + })); + // One square handle per corner (Issues #413/#414/#417), at the + // same already-rotated bounds the outline above uses - + // axis-aligned regardless of the image's own rotation. + handleBounds.forEach(([x, y], cornerIndex) => { + selectionLayer.appendChild(el("rect", { + class: "scene-resize-handle", + x: x - RESIZE_HANDLE_SIZE / 2, + y: y - RESIZE_HANDLE_SIZE / 2, + width: RESIZE_HANDLE_SIZE, + height: RESIZE_HANDLE_SIZE, + "data-corner": RESIZE_HANDLE_CORNERS[cornerIndex], + })); + }); + } + }); 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 498f36c0..0f84c81c 100644 --- a/templates/js/scenes.js +++ b/templates/js/scenes.js @@ -53,6 +53,55 @@ 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 }) }); +} + +/** + * Partially updates an existing image's fields - position, size, rotation, + * flip, or crop (Issues #413-417). Only the given fields are touched. + * @param {string} sceneId + * @param {string} imageId + * @param {object} fields + * @returns {Promise} The updated scene. + */ +export function updateSceneImage(sceneId, imageId, fields) { + return request(scenePath(sceneId), { + method: "PATCH", + body: JSON.stringify({ updateImage: { id: imageId, ...fields } }), + }); +} + +/** + * Removes one image from a scene by id (Issue #418). A retry after a prior + * success is always safe - the server treats "already gone" as success too. + * @param {string} sceneId + * @param {string} imageId + * @returns {Promise} The updated scene. + */ +export function removeSceneImage(sceneId, imageId) { + return request(scenePath(sceneId), { method: "PATCH", body: JSON.stringify({ removeImageId: imageId }) }); +} + +/** + * Reorders a scene's images to match the given id order (Issue #419). The + * server rejects any order that isn't an exact permutation of the scene's + * current image ids. + * @param {string} sceneId + * @param {string[]} orderedIds + * @returns {Promise} The updated scene. + */ +export function reorderSceneImages(sceneId, orderedIds) { + return request(scenePath(sceneId), { method: "PATCH", body: JSON.stringify({ reorderImageIds: orderedIds }) }); +} + export function deleteScene(sceneId) { return request(scenePath(sceneId), { method: "DELETE" }); } @@ -74,7 +123,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: @@ -90,9 +141,63 @@ const state = { openRequest: 0, fitToWidth: true, busy: false, + selectedImageIndex: null, + cropMode: false, }; +function deselectImage() { + state.selectedImageIndex = null; + state.cropMode = false; +} + let dom = null; +// Set by startImageDrag's mouseup once a real drag (not just a click) just +// finished, so the canvas's own click handler (select/deselect toggle) knows +// to skip itself for that click - browsers fire `click` after mouseup on the +// same element regardless of how far the mouse moved in between, so without +// this a move/resize drag would immediately deselect the image it just moved. +let suppressNextClick = false; + +// 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; +// 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) => { + 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 = ""; @@ -170,9 +275,30 @@ function renderLibrary() { } } +/** + * Shows the selected-image toolbar (reorder/rotate/flip/crop/remove) only + * while an image is actually selected: disables the reorder buttons at + * whichever end of the stack the selection is already at, and swaps the + * "Crop…" button for "Done"/"Reset crop" while actively editing a crop + * (Issue #417). + */ +function updateImageToolbar() { + const scene = state.active; + const index = state.selectedImageIndex; + const hasSelection = Boolean(scene) && index !== null && index >= 0 && index < scene.images.length; + dom.imageToolbar.hidden = !hasSelection; + if (!hasSelection) return; + dom.imageBackward.disabled = index === 0; + dom.imageForward.disabled = index === scene.images.length - 1; + dom.imageCropStart.hidden = state.cropMode; + dom.imageCropDone.hidden = !state.cropMode; + dom.imageCropReset.hidden = !state.cropMode; +} + function renderCanvas() { const scene = state.active; dom.canvas.replaceChildren(); + updateImageToolbar(); const isEmpty = scene.images.length === 0 && scene.annotations.length === 0; dom.canvasEmpty.hidden = !isEmpty; dom.canvas.hidden = isEmpty; @@ -180,7 +306,10 @@ 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, + cropMode: state.cropMode, + }); svg.setAttribute("aria-label", `Scene canvas for ${scene.name}`); if (state.fitToWidth) { svg.style.width = "100%"; @@ -214,6 +343,7 @@ function renderWorkspace() { function clearActiveScene() { state.active = null; + deselectImage(); state.openRequest += 1; dom.workspace.removeAttribute("aria-busy"); dom.workspaceLoading.hidden = true; @@ -254,6 +384,7 @@ async function openScene(sceneId) { const scene = await getScene(sceneId); if (requestId !== state.openRequest) return; state.active = scene; + deselectImage(); renderWorkspace(); renderLibrary(); announce(`Opened ${scene.name}.`); @@ -284,6 +415,7 @@ async function handleCreate() { const scene = await createScene(); state.openRequest += 1; state.active = scene; + deselectImage(); await loadLibrary(); setBusy(false); renderWorkspace(); @@ -398,6 +530,523 @@ 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 = []; + 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; + } + showWorkspaceError( + `${plural(stillFailing.length, "image")} could not be saved and will be lost if you reload.`, + { retry: () => retryImageSaves(sceneId, stillFailing) } + ); +} + +/** + * Removes one image from a scene, optimistically and persisted (Issue #418). + * Re-entrant by design so its own retry can just call it again: it always + * re-finds the image's current position before acting, which is what makes a + * restore-then-retry sequence work correctly. + * @param {string} sceneId + * @param {{id: string}} image + */ +async function performImageRemoval(sceneId, image) { + const isActiveScene = Boolean(state.active) && state.active.id === sceneId; + const index = isActiveScene ? state.active.images.findIndex((img) => img.id === image.id) : -1; + + if (index !== -1) { + state.active.images.splice(index, 1); + if (state.selectedImageIndex === index) deselectImage(); + renderWorkspace(); + } + const summary = state.scenes.find((s) => s.id === sceneId); + if (summary) { + summary.imageCount = Math.max(0, summary.imageCount - 1); + renderLibrary(); + } + + try { + await removeSceneImage(sceneId, image.id); + hideWorkspaceError(); + } catch { + if (state.active && state.active.id === sceneId && !state.active.images.some((img) => img.id === image.id)) { + state.active.images.splice(index === -1 ? state.active.images.length : index, 0, image); + renderWorkspace(); + } + if (summary) { + summary.imageCount += 1; + renderLibrary(); + } + showWorkspaceError("Could not remove the image. It has been restored - try again.", { + retry: () => performImageRemoval(sceneId, image), + }); + } +} + +function handleImageRemove() { + if (!state.active || state.selectedImageIndex === null) return; + const image = state.active.images[state.selectedImageIndex]; + if (!image) return; + performImageRemoval(state.active.id, image); +} + +/** + * Persists a field-level change already applied to `image` (the caller has + * already set the new values and rendered them - this just saves and handles + * failure). On failure, reverts exactly the touched fields back to `previous` + * and offers a retry that re-applies `fields` and tries again - used by both + * the button-driven updates below and by drag-release in initializeSceneEditor + * (Issues #413-417: position, resize, rotate, flip, crop all share this path). + * @param {string} sceneId + * @param {number} index - The image's index at the time of the change, used + * to confirm it's still the same image object before reverting/re-rendering. + * @param {object} image + * @param {object} previous - The pre-change values for exactly the touched fields. + * @param {object} fields - The new values that were set, e.g. `{ rotation: 90 }`. + */ +async function persistImageFields(sceneId, index, image, previous, fields) { + try { + await updateSceneImage(sceneId, image.id, fields); + hideWorkspaceError(); + } catch { + if (state.active && state.active.id === sceneId && state.active.images[index] === image) { + Object.assign(image, previous); + renderWorkspace(); + } + showWorkspaceError("Could not save the change. Try again.", { + retry: () => { + if (state.active && state.active.id === sceneId && state.active.images[index] === image) { + Object.assign(image, fields); + renderWorkspace(); + } + persistImageFields(sceneId, index, image, previous, fields); + }, + }); + } +} + +/** + * Applies a field-level change to the selected image - optimistic locally, + * then persisted via persistImageFields. For button-driven changes (rotate + * stepper, flip) where nothing has been applied yet, unlike a drag which + * applies its own changes live frame-by-frame before calling + * persistImageFields directly at release. + * @param {object} fields - The new values to set, e.g. `{ rotation: 90 }`. + */ +function applySelectedImageUpdate(fields) { + if (!state.active || state.selectedImageIndex === null) return; + const scene = state.active; + const sceneId = scene.id; + const index = state.selectedImageIndex; + const image = scene.images[index]; + if (!image) return; + + const previous = {}; + for (const key of Object.keys(fields)) previous[key] = image[key]; + Object.assign(image, fields); + renderWorkspace(); + + persistImageFields(sceneId, index, image, previous, fields); +} + +/** + * Rotates the selected image by a relative amount, normalized into [0, 360) + * (Issue #415). Stepper buttons only (±15°/±90°), not a free-drag handle. + * @param {number} deltaDegrees + */ +function rotateSelectedImage(deltaDegrees) { + if (!state.active || state.selectedImageIndex === null) return; + const image = state.active.images[state.selectedImageIndex]; + if (!image) return; + const current = typeof image.rotation === "number" ? image.rotation : 0; + const next = ((current + deltaDegrees) % 360 + 360) % 360; + applySelectedImageUpdate({ rotation: next }); +} + +/** + * Toggles the selected image's flip state on one axis (Issue #416). + * @param {"horizontal" | "vertical"} axis + */ +function flipSelectedImage(axis) { + if (!state.active || state.selectedImageIndex === null) return; + const image = state.active.images[state.selectedImageIndex]; + if (!image) return; + const field = axis === "horizontal" ? "flipX" : "flipY"; + applySelectedImageUpdate({ [field]: !image[field] }); +} + +/** + * Swaps the selected image with its neighbor toward the front (direction=1) + * or back (direction=-1) of the paint order (Issue #419). No-op at either end + * of the stack - the toolbar also disables the button there, this is just the + * same guard for any other caller. + * @param {1 | -1} direction + */ +async function moveSelectedImage(direction) { + if (!state.active || state.selectedImageIndex === null) return; + const scene = state.active; + const sceneId = scene.id; + const index = state.selectedImageIndex; + const targetIndex = index + direction; + if (targetIndex < 0 || targetIndex >= scene.images.length) return; + + const images = scene.images; + [images[index], images[targetIndex]] = [images[targetIndex], images[index]]; + state.selectedImageIndex = targetIndex; + renderWorkspace(); + + try { + await reorderSceneImages(sceneId, images.map((img) => img.id)); + hideWorkspaceError(); + } catch { + if (state.active && state.active.id === sceneId) { + [images[index], images[targetIndex]] = [images[targetIndex], images[index]]; + state.selectedImageIndex = index; + renderWorkspace(); + } + showWorkspaceError("Could not reorder images. Try again.", { + retry: () => moveSelectedImage(direction), + }); + } +} + +// Issues #413/#414: dragging a selected image to move it, or dragging one of +// its corner handles to resize it. Both read the SVG's current on-screen size +// vs. its viewBox to convert mouse-pixel deltas into scene units - deltas +// only, never absolute positions, so there's no dependency on the SVG's +// screen offset, only its scale. This is what makes it testable without a +// real browser: a test can set `svg.getBoundingClientRect` to a fixed value, +// the same way existing tests mock `FileReader`/`Image` for things jsdom +// can't do for real. +const MIN_IMAGE_SIZE = 10; +const DRAG_THRESHOLD = 2; +const CORNER_GROWTH_SIGN = { + nw: { x: -1, y: -1 }, + ne: { x: 1, y: -1 }, + se: { x: 1, y: 1 }, + sw: { x: -1, y: 1 }, +}; + +function getSceneScale(svg) { + const rect = svg.getBoundingClientRect(); + // Parsed from the attribute directly, not `.viewBox.baseVal` - jsdom's + // SVG support leaves that an empty stub, and parsing the attribute string + // works identically in a real browser too since renderScene always keeps + // it in sync via setAttribute. + const [, , vbWidth, vbHeight] = (svg.getAttribute("viewBox") || "0 0 0 0").split(" ").map(Number); + return { + x: rect.width ? vbWidth / rect.width : 1, + y: rect.height ? vbHeight / rect.height : 1, + }; +} + +/** + * Rotates a delta *vector* (not a point - no center needed) by -degrees, to + * convert a mouse-drag delta measured in global scene axes into the image's + * own local (unrotated) axes. Uses the same rotation convention as + * sceneCanvas.js's rotatePoints, just inverted and center-free. + */ +function unrotateVector(dx, dy, degrees) { + const radians = (degrees * Math.PI) / 180; + const cos = Math.cos(radians); + const sin = Math.sin(radians); + return { x: dx * cos + dy * sin, y: -dx * sin + dy * cos }; +} + +/** + * Starts a move-or-resize drag on the currently selected image. Moving + * updates x/y directly (translation is rotation-invariant). Resizing scales + * from the image's own center (which stays fixed regardless of rotation, + * matching how sceneCanvas.js already rotates images around their center) - + * the mouse delta is un-rotated into the image's local axes first so dragging + * a corner grows/shrinks the image along its own edges even when rotated, + * and width/height are scaled by the same factor so it's never distorted. + * @param {MouseEvent} event + * @param {SVGSVGElement} svg + * @param {string|null} corner - "nw"|"ne"|"se"|"sw" to resize, or null to move. + */ +function startImageDrag(event, svg, corner) { + const scene = state.active; + const index = state.selectedImageIndex; + const image = scene.images[index]; + const sceneId = scene.id; + const scale = getSceneScale(svg); + const startClientX = event.clientX; + const startClientY = event.clientY; + const startX = image.x; + const startY = image.y; + const startWidth = image.width; + const startHeight = image.height; + const rotation = typeof image.rotation === "number" ? image.rotation : 0; + let didDrag = false; + + function onMouseMove(moveEvent) { + const dxClient = moveEvent.clientX - startClientX; + const dyClient = moveEvent.clientY - startClientY; + if (Math.abs(dxClient) > DRAG_THRESHOLD || Math.abs(dyClient) > DRAG_THRESHOLD) didDrag = true; + const globalDx = dxClient * scale.x; + const globalDy = dyClient * scale.y; + + if (corner) { + const sign = CORNER_GROWTH_SIGN[corner]; + const local = unrotateVector(globalDx, globalDy, rotation); + const minScale = MIN_IMAGE_SIZE / Math.min(startWidth, startHeight); + const scaleFactor = Math.max(minScale, (startWidth + sign.x * local.x) / startWidth); + const newWidth = startWidth * scaleFactor; + const newHeight = startHeight * scaleFactor; + const cx = startX + startWidth / 2; + const cy = startY + startHeight / 2; + image.width = newWidth; + image.height = newHeight; + image.x = cx - newWidth / 2; + image.y = cy - newHeight / 2; + } else { + image.x = startX + globalDx; + image.y = startY + globalDy; + } + renderWorkspace(); + } + + function onMouseUp() { + document.removeEventListener("mousemove", onMouseMove); + document.removeEventListener("mouseup", onMouseUp); + suppressNextClick = didDrag; + if (!didDrag) return; + + const fields = corner + ? { x: image.x, y: image.y, width: image.width, height: image.height } + : { x: image.x, y: image.y }; + const previous = corner + ? { x: startX, y: startY, width: startWidth, height: startHeight } + : { x: startX, y: startY }; + persistImageFields(sceneId, index, image, previous, fields); + } + + document.addEventListener("mousemove", onMouseMove); + document.addEventListener("mouseup", onMouseUp); +} + +// Issue #417: non-destructive cropping. The crop rect lives in the image's +// own local box space (0..width, 0..height); absent fields mean "fully +// visible" everywhere this is read, matching sceneCanvas.js's cropRectFor. +const MIN_CROP_SIZE = 10; + +function currentCropRect(image) { + const hasCrop = [image.cropX, image.cropY, image.cropWidth, image.cropHeight] + .every((value) => typeof value === "number" && Number.isFinite(value)); + return hasCrop + ? { x: image.cropX, y: image.cropY, width: image.cropWidth, height: image.cropHeight } + : { x: 0, y: 0, width: image.width, height: image.height }; +} + +/** + * Computes a new crop rect from a corner drag, anchored at the OPPOSITE + * corner of the crop rect itself - unlike resize's center anchor, this is + * the standard crop-tool behavior, since there's no reason a crop needs to + * stay centered. Clamped to stay within the image's own local box and never + * shrink below MIN_CROP_SIZE. + * @param {"nw"|"ne"|"se"|"sw"} corner + * @param {{x: number, y: number, width: number, height: number}} start - crop rect at drag-start. + * @param {{x: number, y: number}} localDelta - mouse delta, already un-rotated. + * @param {number} imageWidth + * @param {number} imageHeight + */ +function computeCropRect(corner, start, localDelta, imageWidth, imageHeight) { + const clamp = (value, min, max) => Math.min(Math.max(value, min), max); + let { x, y, width, height } = start; + + if (corner === "se" || corner === "ne") { + width = clamp(start.width + localDelta.x, MIN_CROP_SIZE, imageWidth - start.x); + } else { + const rightEdge = start.x + start.width; + x = clamp(start.x + localDelta.x, 0, rightEdge - MIN_CROP_SIZE); + width = rightEdge - x; + } + + if (corner === "se" || corner === "sw") { + height = clamp(start.height + localDelta.y, MIN_CROP_SIZE, imageHeight - start.y); + } else { + const bottomEdge = start.y + start.height; + y = clamp(start.y + localDelta.y, 0, bottomEdge - MIN_CROP_SIZE); + height = bottomEdge - y; + } + + return { x, y, width, height }; +} + +/** + * Starts a crop-rect drag on the selected image's currently-grabbed corner + * handle (Issue #417) - the same scale/un-rotate approach as startImageDrag. + * @param {MouseEvent} event + * @param {SVGSVGElement} svg + * @param {"nw"|"ne"|"se"|"sw"} corner + */ +function startCropDrag(event, svg, corner) { + const scene = state.active; + const index = state.selectedImageIndex; + const image = scene.images[index]; + const sceneId = scene.id; + const scale = getSceneScale(svg); + const startClientX = event.clientX; + const startClientY = event.clientY; + const startCrop = currentCropRect(image); + const rotation = typeof image.rotation === "number" ? image.rotation : 0; + let didDrag = false; + + function onMouseMove(moveEvent) { + const dxClient = moveEvent.clientX - startClientX; + const dyClient = moveEvent.clientY - startClientY; + if (Math.abs(dxClient) > DRAG_THRESHOLD || Math.abs(dyClient) > DRAG_THRESHOLD) didDrag = true; + const local = unrotateVector(dxClient * scale.x, dyClient * scale.y, rotation); + + const next = computeCropRect(corner, startCrop, local, image.width, image.height); + image.cropX = next.x; + image.cropY = next.y; + image.cropWidth = next.width; + image.cropHeight = next.height; + renderWorkspace(); + } + + function onMouseUp() { + document.removeEventListener("mousemove", onMouseMove); + document.removeEventListener("mouseup", onMouseUp); + suppressNextClick = didDrag; + if (!didDrag) return; + + const fields = { cropX: image.cropX, cropY: image.cropY, cropWidth: image.cropWidth, cropHeight: image.cropHeight }; + const previous = { cropX: startCrop.x, cropY: startCrop.y, cropWidth: startCrop.width, cropHeight: startCrop.height }; + persistImageFields(sceneId, index, image, previous, fields); + } + + document.addEventListener("mousemove", onMouseMove); + document.addEventListener("mouseup", onMouseUp); +} + +/** Enters crop-editing mode for the selected image (Issue #417). */ +function enterCropMode() { + if (!state.active || state.selectedImageIndex === null) return; + state.cropMode = true; + renderCanvas(); +} + +/** Exits crop-editing mode - a pure view toggle, since any actual crop + * adjustment was already persisted at the end of its own drag gesture. */ +function exitCropMode() { + state.cropMode = false; + renderCanvas(); +} + +/** Clears the selected image's crop back to fully visible, persisted immediately. */ +function resetSelectedImageCrop() { + if (!state.active || state.selectedImageIndex === null) return; + const image = state.active.images[state.selectedImageIndex]; + if (!image) return; + applySelectedImageUpdate({ cropX: 0, cropY: 0, cropWidth: image.width, cropHeight: image.height }); +} + +/** + * 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; + // 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 = ""; + + 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); + 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 * (baseImageCount + added.length + 1); + const image = { id: crypto.randomUUID(), src: dataUrl, x: offset, y: offset, width, height }; + 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)`); + } + } + + 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) { + 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); + } +} + export function getActiveScene() { return state.active; } @@ -460,6 +1109,23 @@ 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"), + imageToolbar: $("scene-image-toolbar"), + imageForward: $("scene-image-forward"), + imageBackward: $("scene-image-backward"), + imageRemove: $("scene-image-remove"), + imageRotateLeft90: $("scene-image-rotate-left-90"), + imageRotateLeft15: $("scene-image-rotate-left-15"), + imageRotateRight15: $("scene-image-rotate-right-15"), + imageRotateRight90: $("scene-image-rotate-right-90"), + imageFlipHorizontal: $("scene-image-flip-horizontal"), + imageFlipVertical: $("scene-image-flip-vertical"), + imageCropStart: $("scene-image-crop-start"), + imageCropDone: $("scene-image-crop-done"), + imageCropReset: $("scene-image-crop-reset"), }; if (!dom.editorView || !dom.enterButton) return; @@ -487,6 +1153,70 @@ 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) => { + if (suppressNextClick) { + suppressNextClick = false; + return; + } + const target = event.target.closest("[data-scene-image-index]"); + const index = target ? Number(target.dataset.sceneImageIndex) : null; + if (state.selectedImageIndex === index) { + deselectImage(); + } else { + state.selectedImageIndex = index; + } + renderCanvas(); + }); + dom.canvas.addEventListener("keydown", (event) => { + if (event.key === "Escape" && state.selectedImageIndex !== null) { + deselectImage(); + renderCanvas(); + } + }); + dom.canvas.addEventListener("mousedown", (event) => { + if (!state.active || state.selectedImageIndex === null) return; + const image = state.active.images[state.selectedImageIndex]; + if (!image) return; + + const handle = event.target.closest("[data-corner]"); + const svg = dom.canvas.querySelector("svg"); + if (!svg) return; + + if (state.cropMode) { + if (!handle) return; + event.preventDefault(); + startCropDrag(event, svg, handle.dataset.corner); + return; + } + + const imageNode = event.target.closest("[data-scene-image-index]"); + const isSelectedImageNode = + imageNode && Number(imageNode.dataset.sceneImageIndex) === state.selectedImageIndex; + if (!handle && !isSelectedImageNode) return; + + event.preventDefault(); + startImageDrag(event, svg, handle ? handle.dataset.corner : null); + }); + + dom.imageForward.addEventListener("click", () => moveSelectedImage(1)); + dom.imageBackward.addEventListener("click", () => moveSelectedImage(-1)); + dom.imageRemove.addEventListener("click", handleImageRemove); + dom.imageCropStart.addEventListener("click", enterCropMode); + dom.imageCropDone.addEventListener("click", exitCropMode); + dom.imageCropReset.addEventListener("click", resetSelectedImageCrop); + dom.imageRotateLeft90.addEventListener("click", () => rotateSelectedImage(-90)); + dom.imageRotateLeft15.addEventListener("click", () => rotateSelectedImage(-15)); + dom.imageRotateRight15.addEventListener("click", () => rotateSelectedImage(15)); + dom.imageRotateRight90.addEventListener("click", () => rotateSelectedImage(90)); + dom.imageFlipHorizontal.addEventListener("click", () => flipSelectedImage("horizontal")); + dom.imageFlipVertical.addEventListener("click", () => flipSelectedImage("vertical")); + renderWorkspace(); } diff --git a/templates/style.css b/templates/style.css index c59cfd7e..e8da007a 100644 --- a/templates/style.css +++ b/templates/style.css @@ -2818,6 +2818,21 @@ button:disabled { gap: var(--space-2); } +.scene-image-toolbar { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: var(--space-2); + margin-top: var(--space-2); + padding-top: var(--space-2); + border-top: 1px solid var(--border); +} + +.scene-image-toolbar-label { + font-weight: 600; + color: var(--text-muted); +} + .scene-canvas-frame { border: 2px solid var(--border); border-radius: var(--radius); @@ -2851,6 +2866,31 @@ button:disabled { color: #92400e; } +.scene-image-object { + cursor: pointer; +} + +.scene-selection-outline { + fill: none; + stroke: var(--accent); + stroke-width: 2; + stroke-dasharray: 6 4; +} + +.scene-resize-handle { + fill: #ffffff; + stroke: var(--accent); + stroke-width: 1.5; + cursor: nwse-resize; +} + +.scene-crop-outline { + fill: rgba(0, 0, 0, 0.15); + stroke: #ffffff; + stroke-width: 2; + stroke-dasharray: 4 3; +} + @media (max-width: 900px) { .scene-editor-layout { grid-template-columns: 1fr; diff --git a/templates/tests/dropdowns.test.js b/templates/tests/dropdowns.test.js index 7f67fe12..46572df2 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 f00cf73f..c9bf9e76 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); diff --git a/templates/tests/sceneCanvas.test.js b/templates/tests/sceneCanvas.test.js index 1a88e724..adf702b5 100644 --- a/templates/tests/sceneCanvas.test.js +++ b/templates/tests/sceneCanvas.test.js @@ -33,3 +33,143 @@ 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); + }); +}); + +// Issues #413/#414: a selected image gets one resize handle per corner. +describe("renderScene resize handles", () => { + it("draws exactly 4 corner handles for the selected image, tagged nw/ne/se/sw", () => { + const { svg } = renderScene({ images: [image()], annotations: [] }, { selectedIndex: 0 }); + const handles = svg.querySelectorAll(".scene-resize-handle"); + + expect(handles).toHaveLength(4); + expect([...handles].map((h) => h.dataset.corner)).toEqual(["nw", "ne", "se", "sw"]); + }); + + it("places handles at the image's corners", () => { + const { svg } = renderScene( + { images: [image({ x: 10, y: 20, width: 200, height: 100 })], annotations: [] }, + { selectedIndex: 0 } + ); + const handles = svg.querySelectorAll(".scene-resize-handle"); + const center = (handle) => ({ + x: Number(handle.getAttribute("x")) + Number(handle.getAttribute("width")) / 2, + y: Number(handle.getAttribute("y")) + Number(handle.getAttribute("height")) / 2, + }); + + expect(center(handles[0])).toEqual({ x: 10, y: 20 }); // nw + expect(center(handles[2])).toEqual({ x: 210, y: 120 }); // se + }); + + it("draws no resize handles when no image is selected", () => { + const { svg } = renderScene({ images: [image()], annotations: [] }); + expect(svg.querySelectorAll(".scene-resize-handle")).toHaveLength(0); + }); +}); + +// Issue #417: non-destructive cropping via an SVG clip-path. +describe("renderScene cropping", () => { + it("clips the image to the crop rect without changing its own width/height", () => { + const { svg } = renderScene({ + images: [image({ x: 0, y: 0, width: 200, height: 100, cropX: 20, cropY: 10, cropWidth: 100, cropHeight: 50 })], + annotations: [], + }); + const img = svg.querySelector("image"); + expect(img.getAttribute("width")).toBe("200"); + expect(img.getAttribute("height")).toBe("100"); + expect(img.getAttribute("clip-path")).toMatch(/^url\(#/); + + const clipId = img.getAttribute("clip-path").match(/url\(#(.+)\)/)[1]; + const clipRect = svg.querySelector(`#${clipId} rect`); + expect(clipRect.getAttribute("x")).toBe("20"); + expect(clipRect.getAttribute("y")).toBe("10"); + expect(clipRect.getAttribute("width")).toBe("100"); + expect(clipRect.getAttribute("height")).toBe("50"); + }); + + it("applies no clip-path when no crop fields are set", () => { + const { svg } = renderScene({ images: [image()], annotations: [] }); + expect(svg.querySelector("image").hasAttribute("clip-path")).toBe(false); + }); + + it("uses the crop rect's corners for the selection outline and canvas sizing, not the full image", () => { + const { svg } = renderScene( + { + images: [image({ x: 0, y: 0, width: 200, height: 100, cropX: 0, cropY: 0, cropWidth: 50, cropHeight: 50 })], + annotations: [], + }, + { selectedIndex: 0 } + ); + const outline = svg.querySelector(".scene-selection-outline"); + expect(outline.getAttribute("points")).toBe("0,0 50,0 50,50 0,50"); + }); + + it("rotates the crop window together with the image rather than independently of it", () => { + const { svg } = renderScene({ + images: [image({ x: 0, y: 0, width: 200, height: 200, rotation: 90, cropX: 0, cropY: 0, cropWidth: 100, cropHeight: 100 })], + annotations: [], + }); + const img = svg.querySelector("image"); + const group = img.parentElement; + expect(group.tagName.toLowerCase()).toBe("g"); + expect(group.getAttribute("transform")).toMatch(/rotate\(90/); + }); + + it("shows the full uncropped image and a draft crop outline while cropMode is active", () => { + const { svg } = renderScene( + { + images: [image({ x: 0, y: 0, width: 200, height: 100, cropX: 20, cropY: 10, cropWidth: 100, cropHeight: 50 })], + annotations: [], + }, + { selectedIndex: 0, cropMode: true } + ); + expect(svg.querySelector("image").hasAttribute("clip-path")).toBe(false); + expect(svg.querySelectorAll(".scene-crop-outline")).toHaveLength(1); + expect(svg.querySelectorAll(".scene-selection-outline")).toHaveLength(0); + expect(svg.querySelector(".scene-crop-outline").getAttribute("points")).toBe("20,10 120,10 120,60 20,60"); + }); + + it("defaults the draft crop rect to the full image when no crop is saved yet", () => { + const { svg } = renderScene( + { images: [image({ x: 0, y: 0, width: 200, height: 100 })], annotations: [] }, + { selectedIndex: 0, cropMode: true } + ); + expect(svg.querySelector(".scene-crop-outline").getAttribute("points")).toBe("0,0 200,0 200,100 0,100"); + }); +}); diff --git a/templates/tests/scenes.test.js b/templates/tests/scenes.test.js index d063893f..13a852f1 100644 --- a/templates/tests/scenes.test.js +++ b/templates/tests/scenes.test.js @@ -57,10 +57,49 @@ 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; + const hasKnownField = body && ( + body.name !== undefined || + body.image !== undefined || + body.updateImage !== undefined || + body.removeImageId !== undefined || + body.reorderImageIds !== undefined + ); + if (!hasKnownField) { + return respond(400, { + error: "name, image, updateImage, removeImageId, or reorderImageIds 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); + } + if (body.updateImage !== undefined) { + const { id: imageId, ...fields } = body.updateImage; + const target = scene.images.find((img) => img.id === imageId); + if (!target) return respond(404, { error: "Image not found" }); + Object.assign(target, fields); + } + if (body.removeImageId !== undefined) { + scene.images = scene.images.filter((img) => img.id !== body.removeImageId); + } + if (body.reorderImageIds !== undefined) { + const ids = body.reorderImageIds; + const currentIds = scene.images.map((img) => img.id); + const isSamePermutation = + Array.isArray(ids) && + ids.length === currentIds.length && + [...ids].sort().join() === [...currentIds].sort().join(); + if (!isSamePermutation) { + return respond(400, { error: "reorderImageIds must match the scene's current images" }); + } + scene.images = ids.map((imgId) => scene.images.find((img) => img.id === imgId)); + } scene.updatedAt = now; return respond(200, scene); } @@ -350,3 +389,620 @@ 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); + }); + + // 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)); + }); +}); + +function selectImage(index) { + document + .querySelector(`[data-scene-image-index="${index}"]`) + .dispatchEvent(new MouseEvent("click", { bubbles: true })); +} + +// Issues #418/#419: removing an image and reordering the stack. +describe("Scene editor: remove and reorder images", () => { + async function openSceneWithTwoImages() { + backend.seed({ + name: "Pair", + images: [ + { id: "img-a", src: "/images/a.png", x: 0, y: 0, width: 100, height: 100 }, + { id: "img-b", src: "/images/b.png", x: 50, y: 50, width: 100, height: 100 }, + ], + }); + await enterEditor(); + await openByName("Pair"); + } + + it("shows the image toolbar only while an image is selected", async () => { + await openSceneWithTwoImages(); + expect($("scene-image-toolbar").hidden).toBe(true); + + selectImage(0); + expect($("scene-image-toolbar").hidden).toBe(false); + + selectImage(0); + expect($("scene-image-toolbar").hidden).toBe(true); + }); + + it("disables bring-forward at the top and send-backward at the bottom of the stack", async () => { + await openSceneWithTwoImages(); + + selectImage(0); + expect($("scene-image-backward").disabled).toBe(true); + expect($("scene-image-forward").disabled).toBe(false); + + selectImage(0); // deselect + selectImage(1); + expect($("scene-image-forward").disabled).toBe(true); + expect($("scene-image-backward").disabled).toBe(false); + }); + + it("removes the selected image, persists it, and updates the scene list count", async () => { + await openSceneWithTwoImages(); + selectImage(0); + + click("scene-image-remove"); + await waitFor(() => expect(document.querySelectorAll(".scene-image-object")).toHaveLength(1)); + + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-b"]); + expect($("scene-image-toolbar").hidden).toBe(true); + expect(listButton("Pair").textContent).toMatch(/1 image/); + + const patchCall = backend.calls.find((c) => c.method === "PATCH" && c.body && c.body.removeImageId); + expect(patchCall.body.removeImageId).toBe("img-a"); + expect([...backend.scenes.values()][0].images.map((img) => img.id)).toEqual(["img-b"]); + }); + + it("restores the image and offers a retry if removal fails", async () => { + await openSceneWithTwoImages(); + selectImage(0); + backend.fail("PATCH", 500); + + click("scene-image-remove"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + + expect(document.querySelectorAll(".scene-image-object")).toHaveLength(2); + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-a", "img-b"]); + + click("scene-workspace-retry"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(true)); + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-b"]); + }); + + it("brings an image forward and sends it backward, persisting the new order", async () => { + await openSceneWithTwoImages(); + selectImage(0); + + click("scene-image-forward"); + await waitFor(() => + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-b", "img-a"]) + ); + let reorderCall = backend.calls.filter((c) => c.method === "PATCH" && c.body && c.body.reorderImageIds).pop(); + expect(reorderCall.body.reorderImageIds).toEqual(["img-b", "img-a"]); + + click("scene-image-backward"); + await waitFor(() => + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-a", "img-b"]) + ); + reorderCall = backend.calls.filter((c) => c.method === "PATCH" && c.body && c.body.reorderImageIds).pop(); + expect(reorderCall.body.reorderImageIds).toEqual(["img-a", "img-b"]); + }); + + it("reverts the order and offers a retry if reordering fails", async () => { + await openSceneWithTwoImages(); + selectImage(0); + backend.fail("PATCH", 500); + + click("scene-image-forward"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-a", "img-b"]); + + click("scene-workspace-retry"); + await waitFor(() => + expect(scenesModule.getActiveScene().images.map((img) => img.id)).toEqual(["img-b", "img-a"]) + ); + }); +}); + +// Issues #415/#416: rotating and flipping an image. +describe("Scene editor: rotate and flip images", () => { + async function openSceneWithOneImage() { + backend.seed({ + name: "Solo", + images: [{ id: "img-a", src: "/images/a.png", x: 0, y: 0, width: 100, height: 50 }], + }); + await enterEditor(); + await openByName("Solo"); + selectImage(0); + } + + it("rotates by the stepper amount and wraps into [0, 360)", async () => { + await openSceneWithOneImage(); + + click("scene-image-rotate-right-90"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].rotation).toBe(90)); + + click("scene-image-rotate-left-15"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].rotation).toBe(75)); + + click("scene-image-rotate-left-90"); + click("scene-image-rotate-left-90"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].rotation).toBe(255)); + + const lastPatch = backend.calls.filter((c) => c.method === "PATCH" && c.body.updateImage).pop(); + expect(lastPatch.body.updateImage).toMatchObject({ id: "img-a", rotation: 255 }); + }); + + it("toggles flipX and flipY independently and persists each", async () => { + await openSceneWithOneImage(); + + click("scene-image-flip-horizontal"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].flipX).toBe(true)); + expect(scenesModule.getActiveScene().images[0].flipY).toBeFalsy(); + + click("scene-image-flip-vertical"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].flipY).toBe(true)); + + click("scene-image-flip-horizontal"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].flipX).toBe(false)); + + const patchCalls = backend.calls.filter((c) => c.method === "PATCH" && c.body.updateImage); + expect(patchCalls.map((c) => c.body.updateImage)).toEqual([ + { id: "img-a", flipX: true }, + { id: "img-a", flipY: true }, + { id: "img-a", flipX: false }, + ]); + }); + + it("reverts a failed rotation and offers a retry", async () => { + await openSceneWithOneImage(); + backend.fail("PATCH", 500); + + click("scene-image-rotate-right-90"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + expect(scenesModule.getActiveScene().images[0].rotation).toBeFalsy(); + + click("scene-workspace-retry"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].rotation).toBe(90)); + }); + + it("reverts a failed flip and offers a retry", async () => { + await openSceneWithOneImage(); + backend.fail("PATCH", 500); + + click("scene-image-flip-horizontal"); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + expect(scenesModule.getActiveScene().images[0].flipX).toBeFalsy(); + + click("scene-workspace-retry"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].flipX).toBe(true)); + }); +}); + +// Issues #413/#414: dragging a selected image to move or resize it. jsdom +// doesn't implement real SVG layout, so `getBoundingClientRect` is mocked to +// a fixed size matching the scene's default 960x600 viewBox (a single small +// image never grows it), giving an exact 1:1 scene-unit-per-pixel scale that +// makes the expected numbers easy to check. +describe("Scene editor: move and resize images", () => { + async function openSceneWithOneImage() { + backend.seed({ + name: "Solo", + images: [{ id: "img-a", src: "/images/a.png", x: 100, y: 100, width: 200, height: 100 }], + }); + await enterEditor(); + await openByName("Solo"); + selectImage(0); + } + + function mockSvgScale() { + const svg = document.querySelector("#scene-canvas svg"); + svg.getBoundingClientRect = () => ({ width: 960, height: 600 }); + return svg; + } + + function drag(target, from, to) { + target.dispatchEvent(new MouseEvent("mousedown", { bubbles: true, clientX: from.x, clientY: from.y })); + document.dispatchEvent(new MouseEvent("mousemove", { clientX: to.x, clientY: to.y })); + document.dispatchEvent(new MouseEvent("mouseup", { clientX: to.x, clientY: to.y })); + } + + it("moves the selected image by the drag delta and persists the new position", async () => { + await openSceneWithOneImage(); + mockSvgScale(); + const imageNode = document.querySelector("[data-scene-image-index=\"0\"]"); + + drag(imageNode, { x: 0, y: 0 }, { x: 30, y: 20 }); + + await waitFor(() => expect(scenesModule.getActiveScene().images[0]).toMatchObject({ x: 130, y: 120 })); + const patchCall = backend.calls.filter((c) => c.method === "PATCH" && c.body.updateImage).pop(); + expect(patchCall.body.updateImage).toMatchObject({ id: "img-a", x: 130, y: 120 }); + }); + + it("does not move the image or fire a save for a mousedown/mouseup with no real movement", async () => { + await openSceneWithOneImage(); + mockSvgScale(); + const imageNode = document.querySelector("[data-scene-image-index=\"0\"]"); + + drag(imageNode, { x: 0, y: 0 }, { x: 0, y: 0 }); + + expect(scenesModule.getActiveScene().images[0]).toMatchObject({ x: 100, y: 100 }); + expect(backend.calls.some((c) => c.method === "PATCH" && c.body.updateImage)).toBe(false); + }); + + it("keeps the image selected after a move drag instead of the resulting click deselecting it", async () => { + await openSceneWithOneImage(); + mockSvgScale(); + const imageNode = document.querySelector("[data-scene-image-index=\"0\"]"); + + drag(imageNode, { x: 0, y: 0 }, { x: 30, y: 20 }); + + // The drag's own re-renders replace the DOM node, so the + // original reference is now detached and can't bubble anywhere; the + // simulated click (which real browsers fire after mouseup on the + // same element regardless of movement in between, something jsdom + // doesn't synthesize from dispatched mousedown/mouseup alone) has to + // target the current node instead. + document + .querySelector("[data-scene-image-index=\"0\"]") + .dispatchEvent(new MouseEvent("click", { bubbles: true })); + + expect(document.querySelectorAll(".scene-selection-outline")).toHaveLength(1); + }); + + it("resizes from a corner handle, preserving aspect ratio, anchored at the center", async () => { + await openSceneWithOneImage(); // 200x100 at (100,100) -> center (200,150) + mockSvgScale(); + const handle = document.querySelector("[data-corner=\"se\"]"); + + drag(handle, { x: 0, y: 0 }, { x: 40, y: 0 }); + + await waitFor(() => { + const image = scenesModule.getActiveScene().images[0]; + expect(image.width).toBeCloseTo(240); + expect(image.height).toBeCloseTo(120); + expect(image.x).toBeCloseTo(80); + expect(image.y).toBeCloseTo(90); + }); + }); + + it("reverts a failed move and offers a retry", async () => { + await openSceneWithOneImage(); + mockSvgScale(); + backend.fail("PATCH", 500); + const imageNode = document.querySelector("[data-scene-image-index=\"0\"]"); + + drag(imageNode, { x: 0, y: 0 }, { x: 30, y: 20 }); + await waitFor(() => expect($("scene-workspace-error").hidden).toBe(false)); + expect(scenesModule.getActiveScene().images[0]).toMatchObject({ x: 100, y: 100 }); + + click("scene-workspace-retry"); + await waitFor(() => expect(scenesModule.getActiveScene().images[0]).toMatchObject({ x: 130, y: 120 })); + }); +}); + +// Issue #417: non-destructive cropping. +describe("Scene editor: crop an image", () => { + async function openSceneWithOneImage() { + backend.seed({ + name: "Solo", + images: [{ id: "img-a", src: "/images/a.png", x: 100, y: 100, width: 200, height: 100 }], + }); + await enterEditor(); + await openByName("Solo"); + selectImage(0); + } + + function mockSvgScale() { + const svg = document.querySelector("#scene-canvas svg"); + svg.getBoundingClientRect = () => ({ width: 960, height: 600 }); + return svg; + } + + function drag(target, from, to) { + target.dispatchEvent(new MouseEvent("mousedown", { bubbles: true, clientX: from.x, clientY: from.y })); + document.dispatchEvent(new MouseEvent("mousemove", { clientX: to.x, clientY: to.y })); + document.dispatchEvent(new MouseEvent("mouseup", { clientX: to.x, clientY: to.y })); + } + + it("toggles the toolbar between Crop and Done/Reset", async () => { + await openSceneWithOneImage(); + expect($("scene-image-crop-start").hidden).toBe(false); + expect($("scene-image-crop-done").hidden).toBe(true); + + click("scene-image-crop-start"); + expect($("scene-image-crop-start").hidden).toBe(true); + expect($("scene-image-crop-done").hidden).toBe(false); + expect($("scene-image-crop-reset").hidden).toBe(false); + + click("scene-image-crop-done"); + expect($("scene-image-crop-start").hidden).toBe(false); + expect($("scene-image-crop-done").hidden).toBe(true); + }); + + it("shows the full image uncropped with a draft crop outline while in crop mode", async () => { + await openSceneWithOneImage(); + click("scene-image-crop-start"); + + expect(document.querySelector("image").hasAttribute("clip-path")).toBe(false); + expect(document.querySelectorAll(".scene-crop-outline")).toHaveLength(1); + }); + + it("drags the se handle to shrink the crop rect and persists it on release", async () => { + await openSceneWithOneImage(); // 200x100 + mockSvgScale(); + click("scene-image-crop-start"); + const handle = document.querySelector("[data-corner=\"se\"]"); + + drag(handle, { x: 0, y: 0 }, { x: -50, y: -20 }); + + await waitFor(() => { + const image = scenesModule.getActiveScene().images[0]; + expect(image).toMatchObject({ cropX: 0, cropY: 0, cropWidth: 150, cropHeight: 80 }); + }); + const patchCall = backend.calls.filter((c) => c.method === "PATCH" && c.body.updateImage).pop(); + expect(patchCall.body.updateImage).toMatchObject({ id: "img-a", cropWidth: 150, cropHeight: 80 }); + }); + + it("drags the nw handle, anchoring at the opposite corner of the crop rect", async () => { + await openSceneWithOneImage(); // 200x100 + mockSvgScale(); + click("scene-image-crop-start"); + const handle = document.querySelector("[data-corner=\"nw\"]"); + + drag(handle, { x: 0, y: 0 }, { x: 40, y: 20 }); + + await waitFor(() => { + const image = scenesModule.getActiveScene().images[0]; + expect(image).toMatchObject({ cropX: 40, cropY: 20, cropWidth: 160, cropHeight: 80 }); + }); + }); + + it("resets the crop back to fully visible and persists it", async () => { + await openSceneWithOneImage(); + mockSvgScale(); + click("scene-image-crop-start"); + drag(document.querySelector("[data-corner=\"se\"]"), { x: 0, y: 0 }, { x: -50, y: -20 }); + await waitFor(() => expect(scenesModule.getActiveScene().images[0].cropWidth).toBe(150)); + + click("scene-image-crop-reset"); + + await waitFor(() => { + const image = scenesModule.getActiveScene().images[0]; + expect(image).toMatchObject({ cropX: 0, cropY: 0, cropWidth: 200, cropHeight: 100 }); + }); + }); + + it("exits crop mode without an extra save when nothing was dragged", async () => { + await openSceneWithOneImage(); + click("scene-image-crop-start"); + const callsBefore = backend.calls.length; + + click("scene-image-crop-done"); + + expect(backend.calls.length).toBe(callsBefore); + expect($("scene-image-toolbar").hidden).toBe(false); + expect(document.querySelectorAll(".scene-crop-outline")).toHaveLength(0); + }); +});