Skip to content

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

Open
Subomiadekunle wants to merge 4 commits into
oss-slu:mainfrom
Subomiadekunle:support-legacy-viewer-content-469
Open

Subomiadekunle wants to merge 4 commits into
oss-slu:mainfrom
Subomiadekunle:support-legacy-viewer-content-469

Conversation

@Subomiadekunle

Copy link
Copy Markdown
Collaborator

Closes #469 (part of #463)

Keeps the old bone viewer content usable while content is moved into scenes.

What changed:

  • The viewer shows a button to open any scene made from the bone you're looking at. In the Scene Editor, "Made from: ..." links back to those bones in the viewer.
  • Bones that haven't been moved into a scene look the same as before.
  • Items with no images (like the Skull views) now say there are no images yet, instead of "Please select a bone".
  • Removed the leftover axis_body_(dens) item.
  • Bone ids can have hyphens now, so 2nd-5th metatarsals finally opens. Ids with slashes or dots are still blocked.
  • If you click between bones quickly, an old slow response can't replace the bone you're on.
  • Gave the [Small Rock] Validate Migrated Scenes #468 validation tests a 30 second timeout, since they go through every item.

How to test:

  • npm test (new tests: boneset-api/viewerTransition.test.js and templates/tests/viewerTransition.test.js)
  • Open Ilium, import it into a scene, go back to Ilium in the viewer and click the scene button, then click "Ilium" under "Made from". Then open Ischium (not migrated) and the Skull boneset (no images).

The plan for retiring the old content will be a separate issue, like the notes on #469 say.

@mudabs

mudabs commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Thank you. I will hold off on performing a code review. However, I would suggest performing a sync on the branch as I have merged newer changes and this might resolve the 4 conflicts.

@Subomiadekunle

Copy link
Copy Markdown
Collaborator Author

@mudabs I updated this branch with the latest main and fixed the merge conflicts by keeping both sets of changes.

I also updated one test helper. After #507, importing a bone no longer saves it to the scene right away; the scene only saves when you click Save. The old test assumed importing also saved, so I changed it to import first and then save. All tests pass now.

@mudabs mudabs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes

Thank Subomi you for the thoughtful work on this migration transition. The viewer-to-scene links, legacy source tracking, missing-content handling, hyphenated bone IDs, and stale-response protection are all useful improvements. The added tests also cover the main migration paths well.

I’m requesting one behavior fix before approval, details below:

[P2] Distinguish missing content from API failures

Files:

  • templates/js/dropdowns.js — loadBoneImages()
  • templates/js/api.js — fetchBoneData()
  • templates/tests/viewerTransition.test.js

fetchBoneData() currently returns null for every failure, including a legitimate 404, a network failure, and a 5xx server response. loadBoneImages() then treats all of those cases as “There are no images for this item yet.”

As a result, a temporary backend outage or failed request can make valid viewer content appear as though it has not been migrated. The user also receives no useful retry or error indication.

Please preserve enough response information to distinguish:

  1. A confirmed missing-content/404 response, which should show “There are no images for this item yet.”
  2. A network, 500, or other server failure, which should show an error message and preferably offer a retry path.

Please add a regression test in templates/tests/viewerTransition.test.js for a failed bone-data request and verify that it does not display the “no images yet” message. Keep the existing test for genuinely missing content.

The database and production image-storage work can remain out of scope for this PR and be handled separately.

Thank you again for the effort on this feature.

fetchBoneDataResult() returns ok, missing (404), or error (network/5xx). The viewer shows 'no images yet' only for missing content; failures show an error with a Try again button. Added regression tests and updated the dropdowns test mock.
@Subomiadekunle

Subomiadekunle commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks for the review @mudabs! I've addressed the requested change:

Distinguish missing content from API failures

  • Added fetchBoneDataResult() in api.js, which returns ok, missing (404), or error (network failure, 5xx, or unreadable response).
  • loadBoneImages() in dropdowns.js now shows "There are no images for this item yet" only for confirmed-missing content. Failed requests show an error message with a Try again button that reloads the item.
  • fetchBoneData() keeps its previous behavior (returns null on failure), so the existing callers in quiz.js and main.js are unaffected.

Tests (templates/tests/viewerTransition.test.js)

  • Regression tests for 500/503 and network failures confirming they do not show the "no images yet" message
  • Retry after a failure loads the images
  • Genuinely missing content still shows "no images yet" (existing test kept)
  • Unit tests for fetchBoneDataResult() response classification
  • Updated the api.js mock in dropdowns.test.js for the new function

All tests pass. Ready for re-review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@mudabs mudabs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Subomi for the thoughtful work on keeping the legacy viewer usable during the scene migration. The viewer-to-scene links, missing-content messaging, retry behavior, stale-request protection, hyphenated bone IDs, and migration coverage are all valuable improvements.

There is one blocking integration issue before approval: this branch now conflicts with the current main branch in templates/js/scenes.js. Main contains the annotation-editor changes from PR #509, while this PR adds the scene-source links and viewer/editor navigation in the same file.

Please rebase this branch onto the current main or merge main into it, then resolve the conflict while preserving both sets of behavior:

  • Keep the annotation-editor imports, state, rendering, and event handlers already present in main.
  • Reapply this PR’s sceneSources rendering in the Scene Editor.
  • Preserve the SHOW_IN_VIEWER_EVENT and OPEN_SCENE_EVENT navigation behavior.
  • Keep the scene-sources markup in templates/boneset.html.
  • Confirm that the viewer scene links and annotation-editor controls can coexist without duplicate or lost event listeners.
    Please also remove the extra blank line at the end of templates/tests/viewerTransition.test.js so git diff --check is clean.

After resolving the integration conflict, please rerun the full test suite and lint against the rebased branch. The current PR-specific tests are passing; this request is primarily to ensure the feature is integrated safely with the latest main.

Thank you again for the substantial effort and good regression coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Small Rock] Support Legacy Viewer Content

3 participants