Keep viewer content usable while scenes are migrated (#469)

boneset-api/data/bones/axis.json

@@ -3,7 +3,6 @@
     "id": "axis",
     "subBones": [
         "axis_body",
-        "axis_body_(dens)",
         "axis_transverse_process",
         "axis_transverse_foramen",
         "axis_pedicle",

boneset-api/images.js

@@ -13,7 +13,7 @@ const path = require("path");
 
 const IMAGE_EXTENSIONS = new Set([".jpg", ".jpeg", ".png"]);
 const DESCRIPTION_SUFFIX = /_description\.json$/i;
-const BONE_ID_PATTERN = /^[a-z0-9_]+$/i;
+const BONE_ID_PATTERN = /^[a-z0-9_-]+$/i; // hyphens allowed for 2nd-5th_metatarsals (Issue #469)
 const HEADER_BYTES = 256 * 1024;
 
 function imageUrl(filename) {

boneset-api/legacyImport.js

@@ -10,7 +10,7 @@ const fs = require("fs").promises;
 const path = require("path");
 const { getImageDimensions, imageUrl } = require("./images");
 
-const BONE_ID_PATTERN = /^[a-z0-9_]+$/i;
+const BONE_ID_PATTERN = /^[a-z0-9_-]+$/i; // hyphens allowed for 2nd-5th_metatarsals (Issue #469)
 const MAX_IMAGE_SIZE = 480; // longest side of an imported image, in scene units
 const IMAGE_GAP = 40; // space between imported images, and between import rows
 

boneset-api/migrationValidation.test.js

@@ -9,6 +9,7 @@ const { buildLegacyAnnotations } = require("./legacyAnnotations");
 const { listItems, migrate } = require("./scripts/migration-report");
 
 const DATA_DIR = path.join(__dirname, "data");
+const LONG_TEST = 30000; // going through every item can take longer than Jest's default 5 seconds
 const items = listItems();
 
 function readJson(file) {
@@ -37,7 +38,7 @@ describe("Migrated scene validation - Issue 468", () => {
             const row = await migrate(item);
             if (row.status !== "imported") expect(row.note.length).toBeGreaterThan(0);
         }
-    });
+    }, LONG_TEST);
 
     describe("every imported item", () => {
         let imported;
@@ -47,7 +48,7 @@ describe("Migrated scene validation - Issue 468", () => {
                 const result = await importItem(item.id).catch(() => null);
                 if (result) imported.push({ id: item.id, ...result });
             }
-        });
+        }, LONG_TEST);
 
         it("keeps the viewer's image order", () => {
             for (const { id, images } of imported) {
@@ -120,4 +121,4 @@ describe("Migrated scene validation - Issue 468", () => {
             }
         });
     });
-});
+});

boneset-api/scenes.js

@@ -167,6 +167,16 @@ function applyImagePatch(existingImage, fields) {
     return { image: merged };
 }
 
+// The bone viewer items a scene's images were imported from (Issue #469), so the
+// viewer can link an item to the scenes made from it.
+function legacySources(scene) {
+    const boneIds = (scene.images || [])
+        .filter((image) => image && image.source && image.source.type === "legacy-image")
+        .map((image) => image.source.boneId)
+        .filter((boneId) => typeof boneId === "string");
+    return [...new Set(boneIds)];
+}
+
 function isPlainObject(value) {
     return value !== null && typeof value === "object" && !Array.isArray(value);
 }
@@ -243,6 +253,7 @@ function toSummary(scene) {
         name: scene.name,
         imageCount: (scene.images || []).length,
         annotationCount: (scene.annotations || []).length,
+        sources: legacySources(scene),
         createdAt: scene.createdAt,
         updatedAt: scene.updatedAt,
     };

boneset-api/server.js

@@ -60,9 +60,10 @@ function isValidBoneId(boneId) {
         return false;
     }
 
-    // Only allow alphanumeric characters and underscores
+    // Only allow letters, numbers, underscores, and hyphens.
     // This prevents path traversal and URL injection attacks
-    const validBoneIdPattern = /^[a-z0-9_]+$/i;
+    // (hyphens are needed for ids like 2nd-5th_metatarsals, Issue #469).
+    const validBoneIdPattern = /^[a-z0-9_-]+$/i;
     return validBoneIdPattern.test(boneId) && boneId.length > 0 && boneId.length <= 100;
 }
 
@@ -304,7 +305,7 @@ app.get("/api/bone-data/", async (req, res) => {
     }
 
     if (!isValidBoneId(boneId)) {
-        return res.status(400).json({ error: "Invalid boneId format. Only alphanumeric characters and underscores are allowed." });
+        return res.status(400).json({ error: "Invalid boneId format. Only letters, numbers, underscores, and hyphens are allowed." });
     }
 
     const descriptionPath = path.join(DESCRIPTIONS_DIR, `${boneId}_description.json`);

boneset-api/viewerTransition.test.js

@@ -0,0 +1,176 @@
+// Tests for keeping the bone viewer usable while content is moved into scenes.
+// Issue #469 (parent #463).
+const fs = require("fs");
+const os = require("os");
+const path = require("path");
+const request = require("supertest");
+
+// Keep scenes made by these tests out of boneset-api/data/scenes.
+// This has to be set before server.js is loaded.
+const scenesDir = fs.mkdtempSync(path.join(os.tmpdir(), "bonebox-transition-test-"));
+process.env.SCENES_DIR = scenesDir;
+
+const { app, serverReady } = require("./server");
+
+const IMAGES_DIR = path.join(__dirname, "data", "images");
+const LONG_TEST = 30000; // tests that go through every item need more than Jest's default 5 seconds
+
+// Every boneset, bone, and bone part in the viewer's dropdowns.
+let items = [];
+
+beforeAll(async () => {
+    await serverReady;
+    const response = await request(app).get("/combined-data");
+    items = [
+        ...response.body.bonesets.map((set) => set.id),
+        ...response.body.bones.map((bone) => bone.id),
+        ...response.body.subbones.map((subbone) => subbone.id),
+    ];
+});
+
+afterAll(() => {
+    fs.rmSync(scenesDir, { recursive: true, force: true });
+});
+
+const createScene = async (name) => (await request(app).post("/api/scenes").send({ name })).body;
+   // Import only builds the content now (#466); save it like the editor's Save does (#451).
+   async function importInto(sceneId, boneId) {
+       const imported = await request(app)
+           .post(`/api/scenes/${sceneId}/import-legacy`)
+           .send({ boneId, annotations: { labels: true, regions: true } });
+       if (imported.statusCode !== 200) return imported;
+
+       const scene = (await request(app).get(`/api/scenes/${sceneId}`)).body;
+       return request(app).put(`/api/scenes/${sceneId}`).send({
+           images: [...scene.images, ...imported.body.images],
+           annotations: [...scene.annotations, ...imported.body.annotations],
+           importLog: [...(scene.importLog || []), imported.body.importLogEntry],
+           baseUpdatedAt: scene.updatedAt,
+       });
+   }
+// Everything the viewer loads for one item, so we can check it doesn't change.
+async function viewerContent(boneId) {
+    const boneData = await request(app).get(`/api/bone-data/?boneId=${boneId}`);
+    const description = await request(app).get(`/api/description/?boneId=${boneId}`);
+    const regions = await request(app).get(`/api/colored-regions?boneId=${boneId}`);
+    return {
+        boneData: [boneData.statusCode, boneData.body],
+        description: [description.statusCode, description.text],
+        regions: [regions.statusCode, regions.body],
+    };
+}
+
+describe("Non-migrated content stays viewable - Issue 469", () => {
+    it("checks all the items in the dropdowns", () => {
+        expect(items.length).toBeGreaterThan(150);
+    });
+
+    it("gives every item an id the API accepts", async () => {
+        const rejected = [];
+        for (const id of items) {
+            const response = await request(app).get(`/api/bone-data/?boneId=${encodeURIComponent(id)}`);
+            if (response.statusCode === 400) rejected.push(id);
+        }
+        expect(rejected).toEqual([]);
+    }, LONG_TEST);
+
+    it("never gives a server error for any item", async () => {
+        for (const id of items) {
+            const boneData = await request(app).get(`/api/bone-data/?boneId=${encodeURIComponent(id)}`);
+            // 200 = has content, 404 = no content yet (the viewer shows a message for that)
+            expect([200, 404]).toContain(boneData.statusCode);
+
+            const description = await request(app).get(`/api/description/?boneId=${encodeURIComponent(id)}`);
+            expect(description.statusCode).toBe(200);
+            expect(description.text).not.toContain("Invalid bone ID");
+        }
+    }, LONG_TEST);
+
+    it("has a file for every image the viewer lists", async () => {
+        const missing = [];
+        for (const id of items) {
+            const response = await request(app).get(`/api/bone-data/?boneId=${encodeURIComponent(id)}`);
+            if (response.statusCode !== 200) continue;
+            for (const image of response.body.images) {
+                if (!fs.existsSync(path.join(IMAGES_DIR, image.filename))) missing.push(`${id}: ${image.filename}`);
+            }
+        }
+        expect(missing).toEqual([]);
+    }, LONG_TEST);
+
+    it("no longer lists the leftover axis_body_(dens) item", () => {
+        expect(items).not.toContain("axis_body_(dens)");
+        expect(items).toContain("axis_body");
+    });
+
+    it("can open 2nd-5th metatarsals now that ids can have hyphens", async () => {
+        const boneData = await request(app).get("/api/bone-data/?boneId=2nd-5th_metatarsals");
+        expect(boneData.statusCode).toBe(200);
+        expect(boneData.body.images.length).toBeGreaterThan(0);
+
+        const regions = await request(app).get("/api/colored-regions?boneId=2nd-5th_metatarsals");
+        expect(regions.statusCode).toBe(200);
+    });
+
+    it("still rejects ids that could reach other folders", async () => {
+        expect((await request(app).get("/api/bone-data/?boneId=../server")).statusCode).toBe(400);
+        expect((await request(app).get("/api/bone-data/?boneId=a/b")).statusCode).toBe(400);
+        expect((await request(app).get("/api/colored-regions?boneId=..%2Fserver")).statusCode).toBe(400);
+    });
+});
+
+describe("Migrated and non-migrated content side by side - Issue 469", () => {
+    it("lists which viewer items each scene was made from", async () => {
+        const scene = await createScene("Pelvis and Femur");
+        expect((await importInto(scene.id, "ilium")).statusCode).toBe(200);
+        expect((await importInto(scene.id, "femur")).statusCode).toBe(200);
+
+        const list = await request(app).get("/api/scenes");
+        const summary = list.body.scenes.find((s) => s.id === scene.id);
+        expect(summary.sources).toEqual(["ilium", "femur"]);
+    });
+
+    it("lists no sources for a scene without imported bone images", async () => {
+        const scene = await createScene("Empty Scene");
+        const list = await request(app).get("/api/scenes");
+        expect(list.body.scenes.find((s) => s.id === scene.id).sources).toEqual([]);
+    });
+
+    it("doesn't change what the viewer shows for an item after it is migrated", async () => {
+        const before = await viewerContent("iliac_crest");
+
+        const scene = await createScene("Iliac Crest Scene");
+        expect((await importInto(scene.id, "iliac_crest")).statusCode).toBe(200);
+
+        expect(await viewerContent("iliac_crest")).toEqual(before);
+    });
+
+    it("works when moving between migrated and non-migrated items in one session", async () => {
+        const scene = await createScene("Atlas Scene");
+        expect((await importInto(scene.id, "atlas")).statusCode).toBe(200);
+        const atlasBefore = await viewerContent("atlas");
+
+        // 1. A migrated item in the viewer
+        expect(atlasBefore.boneData[0]).toBe(200);
+
+        // 2. The scene made from it, with every image loading
+        const opened = await request(app).get(`/api/scenes/${scene.id}`);
+        expect(opened.statusCode).toBe(200);
+        expect(opened.body.images.length).toBeGreaterThan(0);
+        for (const image of opened.body.images) {
+            expect(image.source.boneId).toBe("atlas");
+            const file = await request(app).get(image.src);
+            expect(file.statusCode).toBe(200);
+        }
+
+        // 3. An item nobody has migrated
+        const scenes = (await request(app).get("/api/scenes")).body.scenes;
+        expect(scenes.some((s) => s.sources.includes("humerus_head"))).toBe(false);
+        const humerusHead = await viewerContent("humerus_head");
+        expect(humerusHead.boneData[0]).toBe(200);
+        expect(humerusHead.boneData[1].images.length).toBeGreaterThan(0);
+
+        // 4. Back to the migrated item, which looks the same as before
+        expect(await viewerContent("atlas")).toEqual(atlasBefore);
+    });
+});

templates/boneset.html

@@ -140,6 +140,7 @@ <h2>Visual Reference</h2>
                         <div id="bone-image-container" role="region" aria-live="polite"></div>
 
                     </div>
+                    <div id="viewer-scene-links" class="viewer-scene-links" hidden></div>
                 </section>
             </div>
 
@@ -208,6 +209,7 @@ <h2 id="scene-workspace-heading" class="scene-visually-hidden">Workspace</h2>
                             <div id="scene-title-row" class="scene-title-row">
                                 <h3 id="scene-title" tabindex="-1"></h3>
                                 <p id="scene-meta" class="scene-meta"></p>
+                                <p id="scene-sources" class="scene-sources" hidden></p>
                                 <p id="scene-save-status" class="scene-save-status"></p>
                             </div>
 

templates/js/description.js

@@ -8,14 +8,21 @@ import { fetchDescription } from "./api.js";
  *   used to construct the filename `{id}_description.json`.
  * @returns {Promise<void>}
  */
+// Counts requests so a slow answer for an item the user already left can't
+// replace the description they're reading now (Issue #469).
+let latestRequest = 0;
+
 export async function loadDescription(id) {
     const container = document.getElementById("description-Container");
     container.innerHTML = "";
+    const requestId = ++latestRequest;
 
     try {
         const html = await fetchDescription(id);
+        if (requestId !== latestRequest) return;
         container.innerHTML = html;
     } catch (error) {
+        if (requestId !== latestRequest) return;
         container.innerHTML = "<li>Error loading description.</li>";
         console.error("Failed to fetch description:", error);
     }