fix(nifti): pass through DICOM metadata for NIfTI-backed display sets#5970
fix(nifti): pass through DICOM metadata for NIfTI-backed display sets#5970nutzlastfan wants to merge 4 commits into
Conversation
✅ Deploy Preview for ohif-dev canceled.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Tracked down the regression from downstream testing: the NIfTI datasource config was being resolved once at module initialization time. In our deployed app that could leave |
📝 WalkthroughWalkthroughThis PR integrates NIfTI volume loader support into the OHIF Viewers by adding a new dependency, implementing URL path helpers, reading NIfTI configuration from the data source, and overriding the image loading pipeline to dynamically import and use the NIfTI loader when a NIfTI URL is available. ChangesNIfTI Volume Loader Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/default/src/getSopClassHandlerModule.js`:
- Around line 180-184: The display set currently forces isReconstructable: true
regardless of the loader result; instead derive it from
createNiftiImageIdsAndCacheMetadata's resolved value (loadedImageIds) by setting
isReconstructable to true only when Array.isArray(loadedImageIds) &&
loadedImageIds.length > 0; update the call to imageSet.setAttributes (and ensure
numImageFrames uses the same safe length logic) so downstream MPR/volume code
sees reconstructability only when there are actual image IDs.
🪄 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: 2a82acd1-d86e-47cf-9ff3-c2b25e65339e
📒 Files selected for processing (2)
extensions/default/package.jsonextensions/default/src/getSopClassHandlerModule.js
| imageSet.setAttributes({ | ||
| imageIds: loadedImageIds, | ||
| numImageFrames: Array.isArray(loadedImageIds) ? loadedImageIds.length : 0, | ||
| isReconstructable: true, | ||
| }); |
There was a problem hiding this comment.
Don't force isReconstructable: true when the loader returns no image IDs.
If createNiftiImageIdsAndCacheMetadata resolves to an empty (or non-array) result, the display set ends up with 0 frames yet still advertises reconstructability, which can mislead downstream MPR/volume paths. Derive the flag from the resolved IDs instead.
🐛 Proposed fix
+ const hasImageIds = Array.isArray(loadedImageIds) && loadedImageIds.length > 0;
+
imageSet.setAttributes({
imageIds: loadedImageIds,
- numImageFrames: Array.isArray(loadedImageIds) ? loadedImageIds.length : 0,
- isReconstructable: true,
+ numImageFrames: hasImageIds ? loadedImageIds.length : 0,
+ isReconstructable: hasImageIds,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| imageSet.setAttributes({ | |
| imageIds: loadedImageIds, | |
| numImageFrames: Array.isArray(loadedImageIds) ? loadedImageIds.length : 0, | |
| isReconstructable: true, | |
| }); | |
| const hasImageIds = Array.isArray(loadedImageIds) && loadedImageIds.length > 0; | |
| imageSet.setAttributes({ | |
| imageIds: loadedImageIds, | |
| numImageFrames: hasImageIds ? loadedImageIds.length : 0, | |
| isReconstructable: hasImageIds, | |
| }); |
🤖 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/default/src/getSopClassHandlerModule.js` around lines 180 - 184,
The display set currently forces isReconstructable: true regardless of the
loader result; instead derive it from createNiftiImageIdsAndCacheMetadata's
resolved value (loadedImageIds) by setting isReconstructable to true only when
Array.isArray(loadedImageIds) && loadedImageIds.length > 0; update the call to
imageSet.setAttributes (and ensure numImageFrames uses the same safe length
logic) so downstream MPR/volume code sees reconstructability only when there are
actual image IDs.
1 participant
Context
This is a small preparatory upstream PR for a downstream NIfTI workflow.
In the downstream integration, a NIfTI-backed display set can carry DICOM-derived metadata that should be preserved for VOI / series registration instead of falling back to generic defaults.
Changes
@cornerstonejs/nifti-volume-loaderto the default extension packageImportant Note
This PR is intentionally opened as a draft because the effect depends on paired loader-side support in Cornerstone3D /
@cornerstonejs/nifti-volume-loader.Today, upstream OHIF does not yet have a full built-in NIfTI path in this area, so this branch is best viewed as a minimal handoff hook for discussion rather than as a complete end-user feature.
Why Open It Anyway
The downstream fix request specifically needs an upstream anchor in
OHIF/Viewers, but I want to be explicit that the real behavior change also needs the corresponding Cornerstone3D-side implementation and review.Verification
Code-level port only in this environment. I could not run the full local test / build toolchain from this checkout.
Summary by CodeRabbit
Greptile Summary
This PR adds a NIfTI-backed display set hook to the default extension: when a data source exposes a
niftiPrivateTagNameconfig key,makeDisplaySetreads the corresponding private tag from the instance, constructs a NIfTI URL (with optional base-URL joining), and attaches aloadImageIdsfunction that dynamically imports@cornerstonejs/nifti-volume-loaderand passes DICOM-derived metadata alongside the load request.niftiPrivateTagName/niftiBaseUrlare resolved fresh from the active data source insidemakeDisplaySet, avoiding stale-config issues on data-source switches.createNiftiImageIdsAndCacheMetadataare logged and re-thrown so callers can detect failures rather than silently receiving a partially-initialised display set.@cornerstonejs/nifti-volume-loader@4.21.7is added as a hard dependency (node_modules needed for the dynamicimport()); the functionaldicomMetadatapass-through depends on paired loader-side support in Cornerstone3D, as noted in the PR description.Confidence Score: 5/5
Safe to merge as a preparatory hook; the NIfTI path is entirely inert unless a data source explicitly configures niftiPrivateTagName.
The NIfTI code path activates only when a data source provides a niftiPrivateTagName config key, leaving all existing DICOM display-set construction untouched. Previous concerns about silent error swallowing and stale config have both been addressed in this iteration. The URL-joining helpers handle the common edge cases. The dicomMetadata parameter is a forward-looking no-op until the paired loader supports it, which is explicitly called out in the PR description.
No files require special attention.
Important Files Changed
Reviews (4): Last reviewed commit: "Merge branch 'master' into fix/nifti-pas..." | Re-trigger Greptile