fix(DICOMMetadata): prevent crash when PixelMeasuresSequence is empty#6016
Open
vishal112222 wants to merge 1 commit into
Open
fix(DICOMMetadata): prevent crash when PixelMeasuresSequence is empty#6016vishal112222 wants to merge 1 commit into
vishal112222 wants to merge 1 commit into
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Comment on lines
295
to
+297
| const PixelMeasures = Array.isArray(PixelMeasuresSequence) | ||
| ? PixelMeasuresSequence[0] | ||
| : PixelMeasuresSequence; | ||
| : PixelMeasuresSequence || {}; |
Contributor
There was a problem hiding this comment.
The
|| {} fallback is applied to the non-array branch, but the crash described in the PR occurs when PixelMeasuresSequence is an array — specifically an empty one. When PixelMeasuresSequence = [], Array.isArray([]) is true, so the true branch is taken: PixelMeasuresSequence[0] evaluates to undefined, and then const { SpacingBetweenSlices, SliceThickness } = undefined still throws a TypeError. The fix must guard the true (array) branch as well.
Suggested change
| const PixelMeasures = Array.isArray(PixelMeasuresSequence) | |
| ? PixelMeasuresSequence[0] | |
| : PixelMeasuresSequence; | |
| : PixelMeasuresSequence || {}; | |
| const PixelMeasures = Array.isArray(PixelMeasuresSequence) | |
| ? PixelMeasuresSequence[0] || {} | |
| : PixelMeasuresSequence || {}; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx
Line: 295-297
Comment:
The `|| {}` fallback is applied to the **non-array** branch, but the crash described in the PR occurs when `PixelMeasuresSequence` **is** an array — specifically an empty one. When `PixelMeasuresSequence = []`, `Array.isArray([])` is `true`, so the true branch is taken: `PixelMeasuresSequence[0]` evaluates to `undefined`, and then `const { SpacingBetweenSlices, SliceThickness } = undefined` still throws a `TypeError`. The fix must guard the true (array) branch as well.
```suggestion
const PixelMeasures = Array.isArray(PixelMeasuresSequence)
? PixelMeasuresSequence[0] || {}
: PixelMeasuresSequence || {};
```
How can I resolve this? If you propose a fix, please make it concise.
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
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
Fixes a runtime error that occurs when dragging and dropping segmentation data onto the viewport for studies containing incomplete or empty
PixelMeasuresSequencemetadata.The previous implementation assumed that:
always returned a valid object when
PixelMeasuresSequencewas an array.However, some DICOM studies contain:
which caused the following runtime exception during segmentation loading:
This resulted in the viewer crashing when a segmentation was dragged and dropped onto the viewport.
Changes & Results
PixelMeasuresSequencearraysundefinedBefore
Dragging and dropping segmentation data onto the viewport could crash the viewer when
PixelMeasuresSequenceexisted but was empty.After
The viewer now safely handles empty or missing
PixelMeasuresSequencevalues and continues loading segmentation data without crashing.Testing
Load a study containing segmentation data
Drag and drop segmentation onto the viewport
Verify:
Additional cases tested:
PixelMeasuresSequence = []PixelMeasuresSequence = undefinedChecklist
PR
Suggested PR title:
Code
Before
const PixelMeasures = Array.isArray(PixelMeasuresSequence)
? PixelMeasuresSequence[0]
: PixelMeasuresSequence;
After
const PixelMeasures = Array.isArray(PixelMeasuresSequence)
? PixelMeasuresSequence[0]
: PixelMeasuresSequence || {};
Public Documentation Updates
Tested Environment
Greptile Summary
This PR attempts to prevent a
TypeErrorcrash when destructuringPixelMeasuresfrom an empty or missingPixelMeasuresSequence. However, the|| {}fallback is placed only on the non-array branch of the ternary, leaving the reported crash path — an empty array[]— still unguarded.PixelMeasuresSequence = []:Array.isArray([])istrue, soPixelMeasuresSequence[0]returnsundefined, and the destructure on line 299 still throws. The|| {}must also be applied to the array branch:PixelMeasuresSequence[0] || {}.SharedFunctionalGroupsSequenceon line 289\u2013291 has the same empty-array vulnerability but is not addressed by this PR.Confidence Score: 3/5
The fix does not cover the crash scenario it was written to address; merging as-is leaves the empty-array path broken.
The
|| {}guard is applied to the non-array branch only. The exact case described in the PR —PixelMeasuresSequence = []— still takes the array branch, returnsundefinedfrom index 0, and crashes on destructuring.extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx — the ternary on lines 295–297 still leaves the empty-array case unguarded.
Important Files Changed
[]) goes through the array branch where the guard is still missing.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "crash when PixelMeasuresSequence item is..." | Re-trigger Greptile