Repository navigation
fix(seg): try bulk data fetch for PerFrameFunctionalGroupsSequence before full Part 10 fallback - #151
Closed
igoroctaviano wants to merge 93 commits into
Closed
igoroctaviano wants to merge 93 commits into
igoroctaviano wants to merge 93 commits into
Conversation
…t representations (OHIF#6141) * fix(cornerstone): share one color LUT across a segmentation's viewport representations A labelmap segmentation created by createLabelmapForDisplaySet had no dedicated color LUT, so every viewport that rendered it created its own copy of the default LUT. Editing a segment color only mutated the active viewport's LUT, so the change was lost as soon as a drawing tool was used in another viewport and the segment reverted to the default color. Create a dedicated color LUT when the segmentation is created and record its index, mirroring the SEG and RT paths, so all representations reuse the same LUT and color edits are retained everywhere. * fix(cornerstone): keep the existing color LUT when a segmentation id is reused The caller may pass the id of an existing segmentation, which this method updates rather than replaces. Allocating a new LUT and overwriting the mapping in that case would strand the representations already rendering the segmentation on the previous LUT while later ones used the new index, reintroducing the colour divergence this is meant to fix. Only allocate a LUT when the segmentation has none.
…izations (OHIF#6133) * feat(customization): add typed customization registry via AppTypes.Customizations Seeds a declaration-merged AppTypes.Customizations registry so extensions can declare the value type of each customization id they own, and adds typed overloads to getCustomization, getValue, hasCustomization and setCustomizations. Registered ids get autocomplete and precise types; unregistered and dynamic ids keep the existing loose Customization fallback, so nothing breaks. setCustomizations payloads (including immutability-helper command specs and the custom $filter) are checked against the registry through the new CustomizationEntries type, which CustomizationPhaseInput now reuses for phased app-config blocks. Also drops a redundant string 'mode' scope argument in two modes that was mistyped against the CustomizationScope enum (Mode is the default). CUSTOMIZATION_TYPING_PLAN.md documents the design and the follow-up phases for populating the registry across extensions. * Add some examples to make sure typing actually works --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
…ctable (OHIF#6138) * fix(core): report single-location multi-frame series as non-reconstructable A cine / multi-time series is many frames acquired at a single spatial location: every frame shares one ImagePositionPatient, so the series has no through-plane extent. processMultiframe never checked the spread of the frame positions, so such a series was reported as reconstructable and any volume layout built a degenerate volume with zero slice spacing, which renders black in every plane. Guard both the multi-frame and single-frame paths on a single spatial location. Frame positions are read from the per-frame functional groups, falling back to the shared functional group broadcast to the frame count (a constant position is commonly stored there rather than repeated per frame). The check fails open when fewer than two positions are known, so a genuine volume is never wrongly blocked. * fix(core): prefer per-frame positions only when they can decide an extent getPerFramePositions returned the per-frame positions as soon as a single frame carried a valid one, shadowing the shared functional group. With fewer than two valid positions hasSingleSpatialLocation cannot decide and fails open, so a series whose constant position lives in the shared group was still reported reconstructable. Require the two positions the check needs before preferring the per-frame groups. Also cover the two-instance boundary: the spacing check only runs for more than two instances, so a co-located pair relies entirely on the single-location guard. --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
…ports (OHIF#6139) * fix(cornerstone-dicom-sr): handle content-less imaging measurement reports An Imaging Measurement Report can be stored without its report body: the ConceptNameCodeSequence identifies it as one, but ContentSequence (0040,A730) is absent. Such a report was classified on the concept name alone, so the loader called _getReferencedImagesList and _getMeasurements on the missing content. _getMeasurements calls .find on it and throws, taking the whole viewer down with "Something went wrong" — the study could not be opened at all. Classify a report that has no ContentSequence as a plain SR. Both the loader and the SR viewport already branch on isImagingMeasurementReport, so this keeps them off the measurement path that assumes content exists. The report opens empty in the SR text viewport, which now warns that it has no readable content. Reports that do have a ContentSequence are unaffected. * fix(cornerstone-dicom-sr): report empty SR through display set messages Replace the bespoke warning path with the standard display set message list, so an SR stored without a ContentSequence is reported in the display set tray like any other display set problem. Adds a MISSING_REPORT_CONTENT code rather than a flag read by a single viewport, so further SR warnings can be built on the same mechanism. The text viewport no longer needs the services manager and returns to its previous form. The condition now covers any SR without report content, not only one whose concept name claims to be an Imaging Measurement Report: neither has anything to render. --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
…lients (OHIF#6009) * fix(DicomWebDataSource): set correct cross-service URLs on DICOMweb clients The dicomweb-client library sets qidoURL, wadoURL, and stowURL all to the same base url when no prefixes are provided. This causes requests to be routed to the wrong endpoint when qidoRoot and wadoRoot differ (e.g. /qidors/ vs /wadors/). Extract applyServiceUrls into a shared utility so the production code and tests exercise the same function. After constructing the clients, call applyServiceUrls to assign the correct cross-service URLs. Also adds an optional stowRoot config field for deployments with a separate STOW endpoint. Closes OHIF#5820 Signed-off-by: Agustin Bereciartua <bereciartua.agustin@gmail.com> * fix(DicomWebDataSource): guard against undefined roots and use nullish coalescing Use ?? instead of || for stowRoot fallback to respect explicit empty strings. Wrap assignments in undefined checks so client URLs are not overwritten when a root is not provided. Signed-off-by: Agustin Bereciartua <bereciartua.agustin@gmail.com> * fix(DicomWebDataSource): scope effectiveStowRoot inside each guard to avoid undefined stowURL Move effectiveStowRoot computation inside each conditional block so it cannot leak an undefined value when only one root is configured. In the qidoRoot guard, chain the fallback as stowRoot ?? wadoRoot ?? qidoRoot to handle the case where wadoRoot is undefined. Add test covering the edge case of qidoRoot-only configuration. * fix(DicomWebDataSource): ensure explicit stowRoot reaches both clients unconditionally When stowRoot and wadoRoot are set but qidoRoot is absent, the second guard block does not run and wadoClient.stowURL stays at its constructor default. Since storeInstances() uses wadoDicomWebClient, the explicit stowRoot was silently ignored. Add an unconditional block that applies stowRoot to both clients whenever it is defined. * Use correct sub-paths for DICOMweb operations --------- Signed-off-by: Agustin Bereciartua <bereciartua.agustin@gmail.com> Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
… one on the same slice (OHIF#6198)
…#6214) "Remove from Viewport" clears the overlay from the image but leaves the segmentation loaded, so the only thing that can tell the panel to stop listing it is the representation-removed event -- and `useViewportSegmentations` did not subscribe to it. Nothing else covers that transition. A hydrated segmentation is not a display set in the viewport, so removing it moves no grid state and fires no grid event; the segmentation itself is untouched, so no segmentation event fires either. With two or more segmentations loaded the panel refreshed by accident: `removeSegmentationRepresentations` fires REPRESENTATION_MODIFIED for whichever representation becomes active next. Only removing the last one was a visible no-op, which is why the E2E case covering it was skipped in OHIF#6091. Un-skips that test as regression coverage. Fixes OHIF#6090 Co-authored-by: Thomas Forster <thomas.forster@radicalimaging.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…HIF#6280) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
…#6278) * fix(sr): skip derived display sets when placing SR measurements Loading a structured report while a segmentation is hydrated in another viewport ended the session. Hydrating a segmentation replaces its display set's images with the labelmap's derived images, so getImageIdsForDisplaySet returns derived: ids. When an SR loads, _checkIfCanAddMeasurementsToDisplaySet walks every active display set and destructures metadataProvider.getUIDsFromImageID(imageId); a derived: id is neither WADO-RS nor WADO-URI and is never registered, so that call returns undefined and the destructure throws. An SR measurement references source images, never the images of a derived display set, which is why hydrateStructuredReport already filters on !isDerivedDisplaySet. Apply the same rule to the load, alongside the unsupported check it already skips. It also keeps a SCOORD3D measurement off a segmentation, which shares its source's FrameOfReferenceUID. Adds a unit test for the SOP class handler. * fix(sr): guard every derived display set and unknown image id Review follow ups for the SR measurement placement fix: - Guard the destructure of getUIDsFromImageID. A metadata provider returns undefined for an image id that it does not know, for example an id of a custom SOP class handler or of a data source that marks no derived flag. Skip that image id instead of throwing. - Test isDerived and isOverlayDisplaySet as well as isDerivedDisplaySet. A segmentation that the client makes sets the first two flags only, so the SR pass still walked it and could mark a SCOORD3D measurement loaded on it. - Add the deep import rule to the jest moduleNameMapper of the extension, so that the catch-all rule does not append a second src to an import such as @ohif/core/src/contextProviders/SystemProvider. - Declare isDerivedDisplaySet on the DisplaySet type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The root package.json holds a husky.hooks block that no tool reads. The repository has no husky dependency, so no package installs the git hooks that this block configures. A stale husky 3.1.0 hook in a developer .git/hooks directory looks for node_modules/husky/run.js, does not find that file, and prints "Can't find Husky, skipping <hook> hook". The pre-commit hook never runs lint-staged. This commit removes the husky block only. The lint-staged block stays, because a developer can still run lint-staged directly. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
runtime.txt is a Heroku-style file that hosting platforms, Netlify included, read as the Python version to install before a build. Ours pinned 3.8, added in 2022 alongside the static e2e data and never used by any build step. Netlify's new build image installs Python through mise with attestation checks, and no attestations exist for the end-of-life 3.8, so every deploy preview now fails before pnpm install runs. Nothing in the viewer needs Python; drop the pin and let the image default apply.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* chore(deps): update pnpm to 12.8.1 and i18next to 19.9.2 - Pin packageManager to pnpm@12.8.1 and raise engines.pnpm to >=12 in the root, .netlify and the CLI templates. - Install pnpm 12 in CircleCI and in the Dockerfile. - Update pnpm/action-setup to v6.1.0, the first release that supports pnpm v12. - Raise i18next from 17.3.1 to 19.9.2. react-i18next 12 has an i18next >=19 peer. pnpm 12 otherwise installs i18next 26 for the extensions that do not list i18next. An override holds all packages on 19.9.2. - The lockfile change makes the CircleCI security audit run. Raise the overrides for brace-expansion, fast-uri and joi, and ignore GHSA-hrh2-vp3x-79xf for decompress, which has no fixed version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(deps): move the i18next override next to the React 19 pins Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…lockfile; drop unused node-forge override (OHIF#6330)
…tons when unavailable` (OHIF#6146) * feat: add an optional undo limit to avoid memory pressure when working with large data such as segmentations * fix: remove the default setting of 5 for undox max * doc: document the max undo size option * fix: use numeric comparison rather than null/undefined check * fix: move the undo limit to a configuration item * fix: prevent Infinity from being passed as a maxUndoRedoCacheSize * fix(undo): apply maxUndoRedoCacheSize on mode enter and validate the value The customization was read in preRegistration, before the global phase and the legacy customizationService form are applied, so the limit never took effect for those forms. Read it in onModeEnter instead, and again when a global or mode customization changes, so the mode phase also applies. Accept only an integer from 1 to 10000. The Cornerstone size setter throws a RangeError for a fractional or too-large value, and a size of 0 makes push write to ring[NaN]. An invalid value logs a warning and keeps the default. The setter clears the history, so the size is assigned only when it changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(undo): update the undo/redo buttons after an annotation drag A drag of an existing annotation fires ANNOTATION_MODIFIED only while the pointer moves. The tool pushes the memo on mouse up and fires no event for it, so the buttons kept the state from before the drag. Listen for ANNOTATION_MODIFIED, and re-read the history state after mouseup/touchend. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(undo): document maxUndoRedoCacheSize as a customization; add i18n keys Move cornerstone.maxUndoRedoCacheSize from the list of top-level appConfig keys to the customization table, because no code reads a top-level key. Add the Header:Undo and Header:Redo keys that the button labels use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(undo): apply maxUndoRedoCacheSize once on mode entry Remove the subscriptions to the global and mode customization events. The history size comes from the customization at the time of mode entry only, and onModeEnter applies it once, after it registers the event handlers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(undo): reduce the maxUndoRedoCacheSize tests to a single test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(undo): set the history size directly on mode entry Remove the applyUndoRedoCacheSize helper and its test. onModeEnter sets DefaultHistoryMemo.size to the customization value, or to 50 when the value is unset. An invalid value makes the Cornerstone setter throw. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n pnpm audit (OHIF#6332) Neither advisory has a published fix. http-cache-semantics and braces are reached only through docs and build tooling, never with untrusted input. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
…psSequence is missing Some DICOMweb servers (e.g. IDC's static WADO) omit large sequences like PerFrameFunctionalGroupsSequence from the JSON metadata to save bandwidth. The metadata-based loader (createFromDicomSegImageId) requires this sequence to map frames to segments. When PerFrameFunctionalGroupsSequence is missing from metadata, the SEG loader now falls back to using createFromDICOMSegBuffer which fetches and parses the full DICOM Part 10 file, ensuring SEG loading works regardless of metadata completeness. This regression was introduced in v3.13 when the SEG loading method was changed from buffer-based to metadata-based loading without accounting for incomplete metadata scenarios.
…fore full Part 10 fallback Before falling back to fetching the full DICOM Part 10 file when PerFrameFunctionalGroupsSequence is not inline, first check if the server provides it via BulkDataURI. If available, fetch the bulk data and parse it as JSON, then use the metadata-based loader. Loading strategy (in order of preference): 1. Inline PerFrameFunctionalGroupsSequence (array) → metadata-based loader 2. BulkDataURI for PerFrameFunctionalGroupsSequence → fetch bulk data, then metadata-based loader 3. No bulk data or fetch fails → buffer-based loader (full Part 10 file) This is more efficient than fetching the entire Part 10 file when only the sequence metadata is needed.
igoroctaviano
force-pushed
the
fix/seg-missing-perframe-fallback
branch
from
October 6, 2026 14:22
1ee0028 to
bfbb25a
Compare
Collaborator
Author
|
Closing - branch was based on wrong base. Will recreate with correct base. |
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
14 participants
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.
Summary
Enhancement to the SEG loading fallback to try fetching
PerFrameFunctionalGroupsSequencevia bulk data before falling back to the full Part 10 file.Context
This is a follow-up to PR #148 which added the fallback to buffer-based loader when
PerFrameFunctionalGroupsSequenceis missing from inline metadata.Based on feedback from @fedorov on the upstream OHIF PR (#6333), some servers may provide the sequence via BulkDataURI rather than inline. This is more efficient for large SEGs as it avoids including the entire sequence in the JSON metadata response.
Loading Strategy (in order of preference)
PerFrameFunctionalGroupsSequenceis present as an array → use efficient metadata-based loaderPerFrameFunctionalGroupsSequencehas aBulkDataURI→ fetch bulk data, parse JSON, then use metadata-based loaderChanges
PerFrameFunctionalGroupsSequencehas aBulkDataURIorretrieveBulkDatamethodRelated