Fix: cannot load segmentation after export in local mode#5999
Open
nithin-trenser wants to merge 8 commits into
Open
Fix: cannot load segmentation after export in local mode#5999nithin-trenser wants to merge 8 commits into
nithin-trenser wants to merge 8 commits into
Conversation
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@sedghi Could you please take a look at this PR and provide your feedback? |
wayfarer3130
left a comment
wayfarer3130
left a comment
Contributor
There was a problem hiding this comment.
There is a larger PR to fix a number of segmentation store issues, and that one reliably re-loads afterwards. I'd like to get that one merged, and then see if there is still anything required here to fix.
📝 WalkthroughWalkthroughThree files receive defensive improvements to metadata handling: combineFrameInstance protects cached objects from caller mutations by returning isolated inheriting objects; MetadataProvider explicitly assigns imageId to combined multiframe instances; OHIFCornerstoneSEGViewport safely handles missing pixel measures metadata with fallback and warning. ChangesMetadata Instance Mutation Protection and Defensive Fallbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx`:
- Around line 293-300: Guard against a missing SharedFunctionalGroup before
destructuring: in OHIFCornerstoneSEGViewport, check that SharedFunctionalGroup
exists/truthy (and that SharedFunctionalGroupsSequence is present) before doing
"const { PixelMeasuresSequence } = SharedFunctionalGroup"; if it is missing,
fall back to extracting PixelMeasuresSequence from per-frame data (e.g.,
PerFrameFunctionalGroupsSequence or the first frame's FunctionalGroups) and then
compute PixelMeasures (PixelMeasuresSequence -> PixelMeasures) as before; update
the console warning to reflect which source was used (shared vs per-frame) and
ensure PixelMeasures is initialized to {} when neither source provides values to
avoid the crash.
In `@platform/core/src/classes/MetadataProvider.ts`:
- Around line 63-69: The code assigns imageId to combined which can sometimes be
the original shared instance returned by combineFrameInstance; avoid mutating
shared store objects by checking if combined === instance and, if so, create a
shallow copy (or an isolated clone) before assigning imageId; update the logic
around combined/instance in MetadataProvider.ts (referencing
combineFrameInstance, combined, instance, imageId) so you only mutate a newly
created object, then return that copy, otherwise return the original instance
unchanged.
In `@platform/core/src/utils/combineFrameInstance.ts`:
- Around line 166-197: The per-frame template cache is being bypassed because
callers pass a fresh parent object each lookup, so parent[key] never stabilizes;
fix by ensuring the cached-template parent is stable across frame calls—either
make the caller reuse the same sharedInstance object for all frames (so
parent[key] can be reused), or implement a persistent cache keyed by frame
identifier (e.g., a module-level Map) and attach the template to that stable
parent instead of ephemeral objects; adjust code that constructs the
parent/sharedInstance so templateInstance (parent[key]) is created once and
reused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01117ce1-38d1-46c4-aa5a-6b29e68fe87b
📒 Files selected for processing (3)
extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsxplatform/core/src/classes/MetadataProvider.tsplatform/core/src/utils/combineFrameInstance.ts
Comment on lines
293
to
+300
| 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.'); | ||
| } |
There was a problem hiding this comment.
Guard SharedFunctionalGroup before destructuring to avoid a remaining crash path.
On Line 293, const { PixelMeasuresSequence } = SharedFunctionalGroup; can still throw when SharedFunctionalGroupsSequence is missing/empty, so the new fallback on Lines 295–300 is never reached in that case. Please guard SharedFunctionalGroup first (and ideally fall back to per-frame pixel measures).
Proposed fix
function _getReferencedDisplaySetMetadata(referencedDisplaySet, segDisplaySet) {
- const { SharedFunctionalGroupsSequence } = segDisplaySet.instance;
+ const { SharedFunctionalGroupsSequence, PerFrameFunctionalGroupsSequence } = segDisplaySet.instance;
const SharedFunctionalGroup = Array.isArray(SharedFunctionalGroupsSequence)
? SharedFunctionalGroupsSequence[0]
: SharedFunctionalGroupsSequence;
- const { PixelMeasuresSequence } = SharedFunctionalGroup;
+ const PerFrameFunctionalGroup = Array.isArray(PerFrameFunctionalGroupsSequence)
+ ? PerFrameFunctionalGroupsSequence[0]
+ : PerFrameFunctionalGroupsSequence;
+
+ const PixelMeasuresSequence =
+ SharedFunctionalGroup?.PixelMeasuresSequence ?? PerFrameFunctionalGroup?.PixelMeasuresSequence;
const PixelMeasures =
(Array.isArray(PixelMeasuresSequence) ? PixelMeasuresSequence[0] : PixelMeasuresSequence) || {};
if (!PixelMeasuresSequence || Object.keys(PixelMeasures).length === 0) {
console.warn('PixelMeasuresSequence missing from SEG instance metadata.');
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx`
around lines 293 - 300, Guard against a missing SharedFunctionalGroup before
destructuring: in OHIFCornerstoneSEGViewport, check that SharedFunctionalGroup
exists/truthy (and that SharedFunctionalGroupsSequence is present) before doing
"const { PixelMeasuresSequence } = SharedFunctionalGroup"; if it is missing,
fall back to extracting PixelMeasuresSequence from per-frame data (e.g.,
PerFrameFunctionalGroupsSequence or the first frame's FunctionalGroups) and then
compute PixelMeasures (PixelMeasuresSequence -> PixelMeasures) as before; update
the console warning to reflect which source was used (shared vs per-frame) and
ensure PixelMeasures is initialized to {} when neither source provides values to
avoid the crash.
Comment on lines
+63
to
+69
| const combined = frameNumber && combineFrameInstance(frameNumber, instance); | ||
| if (combined) { | ||
| // Add imageId to multiframe result so it matches single-frame instance. | ||
| combined.imageId = imageId; | ||
| return combined; | ||
| } | ||
| return instance; |
There was a problem hiding this comment.
Avoid mutating shared store instances when combineFrameInstance returns the original object.
This block assumes combined is isolated, but some paths can return the original instance; in those cases, assigning imageId here reintroduces shared metadata mutation.
💡 Suggested fix (isolate before assignment when needed)
- const combined = frameNumber && combineFrameInstance(frameNumber, instance);
- if (combined) {
- // Add imageId to multiframe result so it matches single-frame instance.
- combined.imageId = imageId;
- return combined;
- }
- return instance;
+ const combined = frameNumber && combineFrameInstance(frameNumber, instance);
+ if (!combined) {
+ return instance;
+ }
+
+ // Some paths can return the original cached instance.
+ const result = combined === instance ? Object.create(instance) : combined;
+ result.imageId = imageId;
+ return result;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/core/src/classes/MetadataProvider.ts` around lines 63 - 69, The code
assigns imageId to combined which can sometimes be the original shared instance
returned by combineFrameInstance; avoid mutating shared store objects by
checking if combined === instance and, if so, create a shallow copy (or an
isolated clone) before assigning imageId; update the logic around
combined/instance in MetadataProvider.ts (referencing combineFrameInstance,
combined, instance, imageId) so you only mutate a newly created object, then
return that copy, otherwise return the original instance unchanged.
Comment on lines
+166
to
197
| // 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]); | ||
| } |
There was a problem hiding this comment.
Per-frame template caching is effectively bypassed.
createCombinedValue caches by parent[key], but frame-level calls currently pass a fresh sharedInstance parent each lookup, so frame templates are rebuilt repeatedly instead of reused.
💡 Suggested fix (preserve stable frame-cache parent)
@@
const sharedInstance = createCombinedValue(
instance._parentInstance,
SharedFunctionalGroupsSequence?.[0],
'_shared'
);
+ const sharedTemplate = Object.getPrototypeOf(sharedInstance);
const newInstance = createCombinedValue(
- sharedInstance,
+ sharedTemplate,
PerFrameFunctionalGroupsSequence?.[frameNumber - 1],
frameNumber
);
@@
const sharedInstance = createCombinedValue(
instance._parentInstance,
SharedFunctionalGroupsSequence?.[0],
'_shared'
);
+ const sharedTemplate = Object.getPrototypeOf(sharedInstance);
const newInstance = createCombinedValue(
- sharedInstance,
+ sharedTemplate,
PerFrameFunctionalGroupsSequence?.[frameNumber - 1],
frameNumber
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/core/src/utils/combineFrameInstance.ts` around lines 166 - 197, The
per-frame template cache is being bypassed because callers pass a fresh parent
object each lookup, so parent[key] never stabilizes; fix by ensuring the
cached-template parent is stable across frame calls—either make the caller reuse
the same sharedInstance object for all frames (so parent[key] can be reused), or
implement a persistent cache keyed by frame identifier (e.g., a module-level
Map) and attach the template to that stable parent instead of ephemeral objects;
adjust code that constructs the parent/sharedInstance so templateInstance
(parent[key]) is created once and reused.
@wayfarer3130 Could you please share the PR link that addresses the segmentation store issues? We'd like to test it and see if it resolves the current problem. |
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
7 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.
Context
Fix issue : #5540 and related fix in segmentation load for multiframe data : cornerstonejs/cornerstone3D#2727
Segmentation is not loading after export in /local mode.
Fixed jump to segment feature for multiframe data.
Changes & Results
OHIFCornerstoneSEGViewport._getReferencedDisplaySetMetadata.Cannot destructure property 'SpacingBetweenSlices' of 'a' as it is undefined.SharedFunctionalGroupsSequence.PixelMeasuresSequencecan be an empty array ([]). The current code assumes it has an item and does:PixelMeasures = PixelMeasuresSequence[0]-> undefined then destructures{ SpacingBetweenSlices, SliceThickness }from undefined -> throwsFix : Make destructuring resilient to missing/empty
PixelMeasuresSequenceby defaultingPixelMeasuresto {}.Added jump to segment feature for multiframe data -> Bug ticket in CS3D
Before fix
Before-fix-segmentation-load.mp4
After Fix
After-Fix-segmentation-load.mp4
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Greptile Summary
This PR fixes a crash when loading an exported DICOM SEG in local mode, caused by
PixelMeasuresSequencebeing an empty array, and addresses aimageIdmutation issue on shared/cached metadata objects incombineFrameInstance._getReferencedDisplaySetMetadatanow defaultsPixelMeasuresto{}whenPixelMeasuresSequenceis missing or empty, preventing the destructuring crash onSpacingBetweenSlices/SliceThickness.combineFrameInstance: TheNumberOfFrames < 2early-return andcreateCombinedValueboth now returnObject.create(…)fresh proxies, soMetadataProvider._getInstance'scombined.imageId = imageIdassignment no longer stamps the sharedDicomMetadataStorereference.MetadataProvider._getInstance: Explicitly capturescombinedand stampsimageIdonly on the now-safe per-frame proxy.Confidence Score: 4/5
Safe to merge for the crash fix; the combineFrameInstance refactor introduces a per-frame cache regression worth addressing before high-frame-count multiframe workflows regress in practice.
The SEG viewport crash fix and MetadataProvider imageId-stamping changes are straightforward and correct. The createCombinedValue refactor, however, stores the per-frame template on an ephemeral sharedProxy instead of the stable shared template, so the per-frame cache never hits after the first lookup. The bare return instance fall-through at the bottom of combineFrameInstance (reachable when NumberOfFrames is absent) also still returns the shared store object directly, which combined.imageId = imageId in MetadataProvider can mutate.
platform/core/src/utils/combineFrameInstance.ts — per-frame cache parent selection and the bare return instance fall-through.
Important Files Changed
combinedexplicitly and stampsimageIdonto it before returning; safe becausecreateCombinedValuenow returns a fresh proxy instead of the cached reference.Comments Outside Diff (2)
platform/core/src/utils/combineFrameInstance.ts, line 157-161 (link)return instanceat the bottom ofcombineFrameInstancestill returns the shared store reference directly. This path is reachable whenNumberOfFramesisundefined/NaN(skips the< 2early return and the> 1condition), no per-frame/shared functional groups exist, andGridFrameOffsetVectoris absent. In that scenario thecombined.imageId = imageIdassignment inMetadataProvider._getInstancemutates the original object fromDicomMetadataStore. Wrapping this inObject.createis consistent with how theNumberOfFrames < 2path was already fixed.Prompt To Fix With AI
platform/core/src/utils/combineFrameInstance.ts, line 157-161 (link)return instancereturns the sharedDicomMetadataStorereference directly. SinceMetadataProvider._getInstancethen doescombined.imageId = imageIdon the returned value, this permanently mutates the store object for any instance whereNumberOfFramesisundefined(single-frame DICOM that omits the tag) and the imageId URL carries a truthy frame token. ReturningObject.create(instance)here is consistent with the fix already applied to theNumberOfFrames < 2path above.Prompt To Fix With AI
Reviews (6): Last reviewed commit: "Merge branch 'master' into fix-segmentat..." | Re-trigger Greptile