Skip to content

Fix: cannot load segmentation after export in local mode #5999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nithin-trenser wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix: cannot load segmentation after export in local mode #5999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d0c7aee
Fix: cannot load segmentation after export in local mode
nithin-trenser May 7, 2026
e3da11c
fix: prevent shared metadata mutation in combineFrameInstance
trenser-belbin May 21, 2026
cf98514
Merge branch 'master' into fix-segmentation-load-error
Belbin-GK May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,12 @@ function _getReferencedDisplaySetMetadata(referencedDisplaySet, segDisplaySet) {

const { PixelMeasuresSequence } = SharedFunctionalGroup;

const PixelMeasures = Array.isArray(PixelMeasuresSequence)
? PixelMeasuresSequence[0]
: PixelMeasuresSequence;
const PixelMeasures =
(Array.isArray(PixelMeasuresSequence) ? PixelMeasuresSequence[0] : PixelMeasuresSequence) || {};

if (!PixelMeasuresSequence || Object.keys(PixelMeasures).length === 0) {
console.warn('PixelMeasuresSequence missing from SEG instance metadata.');
}
const { SpacingBetweenSlices, SliceThickness } = PixelMeasures;

const image0 = referencedDisplaySet.images[0];
Expand Down
8 changes: 7 additions & 1 deletion platform/core/src/classes/MetadataProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ class MetadataProvider {
return;
}

return (frameNumber && combineFrameInstance(frameNumber, instance)) || instance;
const combined = frameNumber && combineFrameInstance(frameNumber, instance);
if (combined) {
// Add imageId to multiframe result so it matches single-frame instance.
combined.imageId = imageId;
return combined;
}
Comment on lines +63 to +68
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 7, 2026

Choose a reason for hiding this comment

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

P2 Mutation of a cached combined instance

createCombinedValue inside combineFrameInstance caches the per-frame object at instance._parentInstance[frameNumber] and returns the same reference on every subsequent call. Setting combined.imageId = imageId permanently stamps that imageId onto the cached object. If the same SOP-instance frame is ever resolved through two different imageIds (e.g., the same DICOM served from two WADO-RS endpoints), the second caller receives a combined instance whose imageId belongs to the first caller.

Additionally, when NumberOfFrames < 2 combineFrameInstance returns the original instance reference unchanged. If frameNumber is somehow truthy for such an instance, combined === instance, and combined.imageId = imageId mutates the shared object stored in DicomMetadataStore directly — not just its per-frame projection.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: platform/core/src/classes/MetadataProvider.ts
Line: 63-68

Comment:
**Mutation of a cached combined instance**

`createCombinedValue` inside `combineFrameInstance` caches the per-frame object at `instance._parentInstance[frameNumber]` and returns the same reference on every subsequent call. Setting `combined.imageId = imageId` permanently stamps that `imageId` onto the cached object. If the same SOP-instance frame is ever resolved through two different imageIds (e.g., the same DICOM served from two WADO-RS endpoints), the second caller receives a combined instance whose `imageId` belongs to the first caller.

Additionally, when `NumberOfFrames < 2` `combineFrameInstance` returns the original `instance` reference unchanged. If `frameNumber` is somehow truthy for such an instance, `combined === instance`, and `combined.imageId = imageId` mutates the shared object stored in `DicomMetadataStore` directly — not just its per-frame projection.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor

@Belbin-GK Belbin-GK May 21, 2026

Choose a reason for hiding this comment

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

Fixed by avoiding mutation of cached/shared metadata objects in combineFrameInstance.

Previously, combined.imageId = imageId modified shared references returned from createCombinedValue, which could leak imageId values across callers. For single-frame instances, it could also mutate the original metadata object stored in DicomMetadataStore.

Updated the implementation to return a cloned object before assigning imageId.

return instance;
}

get(query, imageId, options = { fallback: false }) {
Expand Down
63 changes: 33 additions & 30 deletions platform/core/src/utils/combineFrameInstance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ const combineFrameInstance = (frame, instance) => {
} = instance;

if (NumberOfFrames < 2) {
return instance;
// Return a fresh proxy so callers can set properties (eg. imageId)
// without mutating the shared `instance` stored in DicomMetadataStore.
return Object.create(instance);
}

instance.ImageType = dicomSplit(ImageType);
Expand All @@ -32,7 +34,8 @@ const combineFrameInstance = (frame, instance) => {

if (
(PerFrameFunctionalGroupsSequence && SharedFunctionalGroupsSequence) ||
hasDetectorButMissingSpatialInfo || NumberOfFrames > 1
hasDetectorButMissingSpatialInfo ||
NumberOfFrames > 1
) {
// this is to fix NM multiframe datasets with position and orientation
// information inside DetectorInformationSequence
Expand Down Expand Up @@ -160,37 +163,37 @@ const combineFrameInstance = (frame, instance) => {
* The storage key in the parent is in key
*/
function createCombinedValue(parent, functionalGroups, key) {
if (parent[key]) {
return parent[key];
}
// Exclude any proxying values
const newInstance = Object.create(parent);
Object.defineProperty(parent, key, {
value: newInstance,
writable: false,
enumerable: false,
});
if (!functionalGroups) {
return newInstance;
}
const shared = functionalGroups
? Object.values(functionalGroups)
// Ensure there's a cached template on the parent (a non-writable, non-enumerable object)
if (!parent[key]) {
const templateInstance = Object.create(parent);
Object.defineProperty(parent, key, {
value: templateInstance,
writable: false,
enumerable: false,
});

if (functionalGroups) {
const shared = Object.values(functionalGroups)
.filter(Boolean)
.map(it => it[0])
.filter(it => typeof it === 'object')
: [];

// merge the shared first then the per frame to override
[...shared].forEach(item => {
if (item.SOPInstanceUID) {
// This sub-item is a previous value information item, so don't merge it
return;
.filter(it => typeof it === 'object');

// merge the shared/per-frame attributes onto the template
[...shared].forEach(item => {
if (item.SOPInstanceUID) {
// This sub-item is a previous value information item, so don't merge it
return;
}
Object.entries(item).forEach(([k, value]) => {
templateInstance[k] = value;
});
});
}
Object.entries(item).forEach(([key, value]) => {
newInstance[key] = value;
});
});
return newInstance;
}

// Return a fresh object that inherits from the cached template so
// mutations by the caller do not affect the cached template.
return Object.create(parent[key]);
}

export default combineFrameInstance;
Loading