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. |
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.