fix: skip restored state for datasets that failed to load, plus test cleanups - #960
Merged
Merged
Conversation
…exity The duplication ratchet found four clones inside the DICOM chunk image spec, one shared by the two DICOM store specs, and one shared by the two new DICOM end-to-end specs. The complexity ratchet found onRegularChunkHasData at 18. The chunk image spec builds its held-open decodes with one deferredDecoder helper, shares the re-sorted chunk setup the three staleness tests need, and asserts slice contents through expectSliceValues. The two store specs build their instance tags from a shared fixture. The end-to-end specs make their scratch directory with a makeTempDir helper in the existing test utils. No assertion changed. onRegularChunkHasData delegates its three rejections to assertSingleFramePerFile, assertChunkFitsSlot and assertSamplesRepresentable, which also removes the repeated "File X (chunk N)" prefix. Behaviour and error messages are unchanged.
Restoring a state file mapped each saved crop entry's image id through the restore dataIDMap without checking that the image had actually loaded. A dataset that could not be reached is absent from that map, so its saved planes were seated under the key `undefined`, clamped against an empty extent to a degenerate zero-width crop. No image with that id is ever deleted, so the store's onImageDeleted cascade could never drop the entry: it survived in `tools.crop` and was written back out on every later save, leaving the saved manifest with a crop keyed by an image no dataset in the file describes, which the dev-only save backstop reports as a dangling reference. Deserialization now skips a saved crop whose image did not resolve, so the crops of resolved images survive a save unchanged.
Restoring a state file mapped each saved layer relationship's parent and source through the restore dataIDMap without checking that either image had actually loaded. A dataset that could not be reached is absent from that map, so addLayer was handed a missing id and only failed once the build was under way. A missing parent was the worse of the two. addLayer's failure path assigns `parentToLayers[parent]` the result of filtering a list that is not there, which seats the key `undefined` with the value `undefined`. Serialize maps over every entry, so the next save threw a TypeError before it could write anything, and the user saw only a failed save. A missing source instead left a layer whose selection was undefined hanging off a real parent, and the save wrote that parent's whole relationship with an undefined source key, which the manifest schema rejects: the parent's valid layers were dropped from the file with it. Deserialization now skips a saved relationship whose parent did not resolve and each source that did not resolve, so the layers of resolved images are built and saved unchanged.
Polling, cancel and result loading all classify their errors and hand a 401 to markSessionExpired. The job-history load did not: it logged and recorded a per-provider string, so the session stayed live in the store, the persistent reload notice never appeared, and the panel offered a Retry that could only 401 again. Its catch now goes through expireSessionIf first, and both the history request and the page loop stop once the session is expired, so nothing re-issues a request that cannot succeed. The two catches build their detail string in a local, which keeps the file inside the line ratchet. A case in the store spec drives a 401 listing through adoption and asserts the notice, the empty history error, and that a retry sends nothing.
✅ Deploy Preview for volview-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Small fixes and test cleanups that stand on their own. They were written alongside the segmentation work in #950 but do not depend on it, so they are split out here to keep that review smaller.
Restoring a session whose dataset failed to load
A dataset that cannot be reached is absent from the restore id map. Several restore paths mapped saved ids through it without checking:
undefinedand written back out on every later save.parentToLayers[undefined] = undefined, and the next save threw before writing anything. A missing source left a layer the manifest schema rejects, which dropped the parent's valid layers from the file.Processing
The job history load now expires the session on a 401, as polling, cancel and result loading already do. Before, the session stayed live and the panel offered a Retry that could only 401 again.
State file
Drops the paint
labelmapOpacityschema field, which nothing reads or writes.Tests
onRegularChunkHasDatadelegates its three rejections to named assertions. No assertion, behaviour or error message changed. This clears the duplication and complexity that a follow-up change adding quality ratchets to CI would otherwise flag.