Skip to content

fix(Cornerstone): guard volume validation when metadata is unavailable#6093

Open
WillianVarela wants to merge 3 commits into
OHIF:masterfrom
cfaz-net:fix/multiframe-metadata-guard
Open

fix(Cornerstone): guard volume validation when metadata is unavailable#6093
WillianVarela wants to merge 3 commits into
OHIF:masterfrom
cfaz-net:fix/multiframe-metadata-guard

Conversation

@WillianVarela

@WillianVarela WillianVarela commented Jun 18, 2026

Copy link
Copy Markdown

Context

When opening some multi-frame DICOM studies from JSON metadata, the viewport overlay/rendering controls can crash while evaluating overlay display sets.

The crash happens in getEnhancedDisplaySets when csUtils.isValidVolume(imageIds) is called before Cornerstone metadata is available for one or more imageIds.

Example error:

TypeError: Cannot destructure property 'modality' of 'metaData.get(...)' as it is undefined.
    at Module.isValidVolume
    at getEnhancedDisplaySets
    at useViewportDisplaySets
    at useViewportRendering
    at AdvancedRenderingControls

This can happen during viewport initialization, where the display set already has imageIds but the metadata provider is not ready for all of them yet.

Fixes #5680
Fixes #5909

Changes & Results

  • Add a defensive helper around csUtils.isValidVolume.
  • Return false when there are no imageIds or when volume validation throws because metadata is unavailable.
  • Reuse the same helper for background and foreground display set volume checks.
  • Fix a typo in CornerstoneCacheService where stack image IDs were assigned to imagesIds instead of imageIds.

Before:

  • The viewport UI could crash when isValidVolume tried to read missing metadata.

After:

  • The display set is treated as not volume-compatible until metadata is available, and the viewport UI does not crash.

Testing

Tested locally with a multi-frame DICOM study loaded from JSON metadata.

  • Started the OHIF viewer locally.
  • Opened a study containing a multi-frame DICOM instance.
  • Verified that viewport overlay/rendering controls no longer crash with metaData.get(...) is undefined.

Also ran:

./node_modules/.bin/prettier --write extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.ts
NX_SKIP_NX_CACHE=true npx lerna run build --scope @ohif/extension-cornerstone --stream

The extension build completed successfully.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

No public API additions or removals were made.

Tested Environment

  • OS: Windows 11 (WSL)
  • Node version: 24.2.0
  • Browser: Google Chrome 149.0.7827.116

Summary by CodeRabbit

  • Bug Fixes
    • Corrected how image identifiers are populated in cached viewport data, ensuring consistent downstream access to the right image IDs.
  • Improvements
    • Enhanced background/volume overlay eligibility by more reliably extracting image IDs across varied display-set formats.
    • When image IDs are missing or validation fails, the overlay is now treated as invalid for more consistent behavior.

Greptile Summary

This PR guards against a crash in getEnhancedDisplaySets when Cornerstone metadata is not yet available for all imageIds during viewport initialization, and fixes a typo in CornerstoneCacheService that silently wrote stack image IDs to the wrong property name.

  • Introduces isValidVolumeDisplaySet, a try/catch wrapper around csUtils.isValidVolume that returns false (with a console.warn) instead of crashing when metadata is absent; both background and foreground volume checks now use it.
  • Adds getImageIdsFromDisplaySet to normalize imageId extraction across the three display set shapes (imageIds, images[].imageId, instances[].imageId), always returning at least [].
  • Fixes the imagesIdsimageIds typo in CornerstoneCacheService._getOtherViewportData so that displaySet.imageIds is actually populated and readable by downstream consumers.

Confidence Score: 5/5

Safe to merge — both changes are narrowly scoped defensive fixes with no behavioural regressions on the happy path.

The crash guard in isValidVolumeDisplaySet only activates when csUtils.isValidVolume throws or receives an empty array, leaving normal execution unchanged. The typo fix in CornerstoneCacheService corrects a silent no-op assignment so that displaySet.imageIds is correctly populated; the return value shape of the function is unaffected. No new logic branches, no API surface changes, and the previous review concern about silent swallowing was already addressed with a console.warn.

No files require special attention.

Important Files Changed

Filename Overview
extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.ts Adds getImageIdsFromDisplaySet and isValidVolumeDisplaySet helpers to guard against crashes when metadata is unavailable; replaces direct csUtils.isValidVolume calls with the safe wrapper throughout getEnhancedDisplaySets.
extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts One-line typo fix: imagesIdsimageIds so the populated property is actually readable by downstream consumers via displaySet.imageIds.

Reviews (2): Last reviewed commit: "Update extensions/cornerstone/src/compon..." | Re-trigger Greptile

@netlify

netlify Bot commented Jun 18, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 81cf2e7
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a343a71e518b200087b53cc
😎 Deploy Preview https://deploy-preview-6093--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Fixes a typo in CornerstoneCacheService._getOtherViewportData (imagesIdsimageIds) and adds two new defensive helpers in the overlay utils (getImageIdsFromDisplaySet, isValidVolumeDisplaySet). getEnhancedDisplaySets is updated to use these helpers for background and non-derived modality volume eligibility checks.

Changes

Image ID Handling and Volume Validation Fixes

Layer / File(s) Summary
CornerstoneCacheService imageIds typo fix
extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts
Corrects the property assignment from displaySet.imagesIds to displaySet.imageIds in _getOtherViewportData, ensuring the populated value is actually accessible via displaySet.imageIds downstream.
Defensive image ID and volume validation helpers
extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.ts
Adds getImageIdsFromDisplaySet (handles imageIds, images, and instances shapes, filters falsy values) and isValidVolumeDisplaySet (wraps csUtils.isValidVolume with null guard and try/catch). Updates getEnhancedDisplaySets background and non-derived modality checks to use these helpers instead of direct csUtils.isValidVolume calls.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A typo once hid in the IDs array,
imagesIds spelled the wrong way!
With a catch and a guard,
No more crashes, not hard,
The frames load correctly today! 🖼️

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically describes the main change: guarding volume validation when metadata is unavailable, which is the core issue addressed.
Linked Issues check ✅ Passed The PR directly addresses both linked issues (#5680, #5909) by guarding against metadata access before it's available, preventing the 'Cannot destructure property modality' error during multi-frame DICOM loading from dicom-json sources.
Out of Scope Changes check ✅ Passed All changes are in scope: defensive helpers for volume validation (utils.ts) and a typo fix for property assignment (CornerstoneCacheService.ts) both directly support the objective of handling metadata unavailability gracefully.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The pull request description comprehensively covers all required sections: Context with issue links, detailed Changes & Results, Testing methodology, and all checklist items properly marked.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/src/components/ViewportDataOverlaySettingMenu/utils.ts`:
- Around line 8-12: In the getImageIdsFromDisplaySet function, add optional
chaining when accessing the imageId property on array elements to prevent
potential errors if null or undefined elements exist in the arrays.
Specifically, update the map operations for both the images array and instances
array to use optional chaining (image?.imageId and instance?.imageId) instead of
direct property access (image.imageId and instance.imageId). This ensures the
function remains defensive and won't throw if array elements are null or
undefined.
🪄 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: 14233cb8-db8d-481d-bb7b-3e4e5510c1d2

📥 Commits

Reviewing files that changed from the base of the PR and between ffd7884 and 269b9d6.

📒 Files selected for processing (2)
  • extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.ts
  • extensions/cornerstone/src/services/CornerstoneCacheService/CornerstoneCacheService.ts

…ngMenu/utils.ts

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ngMenu/utils.ts

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant