Repository navigation
Keep viewer content usable while scenes are migrated (#469) - #508
Subomiadekunle wants to merge 4 commits into
Conversation
|
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. |
|
@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
left a comment
There was a problem hiding this comment.
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:
- A confirmed missing-content/404 response, which should show “There are no images for this item yet.”
- 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.
|
Thanks for the review @mudabs! I've addressed the requested change: Distinguish missing content from API failures
Tests (
All tests pass. Ready for re-review. |
mudabs
left a comment
There was a problem hiding this comment.
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.
Closes #469 (part of #463)
Keeps the old bone viewer content usable while content is moved into scenes.
What changed:
How to test:
The plan for retiring the old content will be a separate issue, like the notes on #469 say.