feat: Add support for labelmap seg images in any supported tsuid (also for compressed bitmap)#5806
Open
wayfarer3130 wants to merge 28 commits into
Open
feat: Add support for labelmap seg images in any supported tsuid (also for compressed bitmap)#5806wayfarer3130 wants to merge 28 commits into
wayfarer3130 wants to merge 28 commits into
Conversation
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Viewers
 |
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
feat/load-seg-images
|
| Run status |
|
| Run duration | 01m 50s |
| Commit |
|
| Committer | Bill Wallace |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
28
|
| View all changes introduced in this branch ↗︎ | |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
wayfarer3130
changed the title
wayfarer3130
changed the title
Collaborator
|
@wayfarer3130 please add details for testing this. Thank you. |
Collaborator
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
fedorov
reviewed
May 22, 2026
fedorov
left a comment
fedorov
left a comment
Member
There was a problem hiding this comment.
AI-Assisted Code Review
How this review was generated: This review was produced using Claude Code (model:
claude-sonnet-4-6, Anthropic, May 2026). The review process had two passes:
- Diff review — the full PR diff (2,621 lines, 39 files) was analyzed for correctness, style, and known anti-patterns.
- Codebase analysis — the branch was checked out locally at
/Users/af61/github/Viewersand ~160 targeted file reads and greps were performed across the full monorepo to assess cross-cutting impact on unchanged code.All findings were verified against the actual source. Sections marked "Cleared" below were flagged in the diff pass but found safe after full-codebase inspection.
Overview
The PR replaces the ArrayBuffer-based SEG loading path with a per-frame imageId approach that delegates pixel data loading to the Cornerstone image loader. Key changes:
- New imageId-based API for
adaptersSEG.Cornerstone3D.Segmentation.createFromDICOMSegBuffer - Support for the labelmap SEG SOP class (
1.2.840.10008.5.1.4.1.1.66.7) in addition to bitmap - Configurable store encoding (labelmap vs bitmap, transfer syntax) via
customizationService - OHIF runtime properties (
imageId,wadoRoot,frameNumber, etc.) made non-enumerable to prevent dcmjs from writing them into stored DICOM - Centralized DICOM serialization in a new
dicomWriter.tsutility dcmjsbumped from 0.49.4 → 0.51.1 across the monorepo
The architectural direction is sound and several pre-existing bugs are fixed. However there is one critical sequencing issue that would likely prevent the primary feature from working.
Critical Issues
1. SEGMENTATION_LOADING_COMPLETE fires before segmentation is in state — null-guard silently drops overlay
This is the most significant issue, found via full codebase analysis. In SegmentationService.ts, SEGMENTATION_LOADING_COMPLETE is broadcast (line ~587) before addOrUpdateSegmentation is called (line ~611), which is what actually puts the segmentation into state.
The new subscriber in CornerstoneViewportService.ts fires applyRepresentation() immediately on the event. That calls addSegmentationRepresentation → getSegmentation(segmentationId). Because the segmentation isn't in state yet at that point, the null-guard introduced by this same PR at line 302 triggers — logs a warning and silently returns without adding the overlay representation. The SEG labelmap is parsed and loaded, but never applied to the viewport.
Fix: Move the SEGMENTATION_LOADING_COMPLETE broadcast to after addOrUpdateSegmentation completes, or restructure the subscriber to wait until the segmentation is in state before calling addSegmentationRepresentation.
2. Promise in addOverlayRepresentationForDisplaySet never resolves on SEG load failure
If _loadSegments throws (no imageId, adapter error, missing referenced display set), SEGMENTATION_LOADING_COMPLETE is never fired. The Promise returned by addOverlayRepresentationForDisplaySet never resolves or rejects, so _addOverlayRepresentations stalls indefinitely. In practice this blocks the VIEWPORT_DATA_CHANGED broadcast but not viewport rendering (since viewport.setStack completes before overlay setup is awaited). Still, any downstream update dependent on that broadcast is silently dropped with no way to recover short of a page reload. No error event subscription, timeout, or rejection path is present.
Issues Flagged in Diff Pass, Cleared by Codebase Analysis
frameIndex → frameNumber rename: Not a breaking change. MetadataProvider.getUIDsFromImageID always overrides the stored field with the URL-parsed frame value; the stored field name was always ignored.
baseImageURIForMetadata coverage: Correct and consistent. The new function strips both ?frame= and &frame=, fixing an inconsistency in the old code that only stripped &frame=. Keys remain consistent within a session.
getImageIdsForInstance behavior change: Correct fix. The old code always used &frame= even for URLs with no existing query params; appendFrameQueryToImageId correctly chooses ? vs &. No callers depended on the old behavior.
displaySets={[referencedDisplaySet, segDisplaySet]}: Correct fix. OHIFCornerstoneViewport uses only viewportData.data[0].imageIds for the stack; SEG is picked up by _processExtraDisplaySetsForViewport as an overlay. No double rendering.
Non-enumerable frameNumber: Safe. All access patterns use direct property access (destructuring, dot notation) — no code uses Object.keys, JSON.stringify, or spread on combined frame instances expecting frameNumber.
Medium Issues
3. tmtv RT export not updated
tmtv/src/utils/dicomRTAnnotationExport/RTStructureSet/dicomRTAnnotationExport.js still calls dcmjs.data.datasetToBlob directly without makeExistingPropertiesNonEnumerable. Any OHIF runtime properties that remain enumerable on RT export instances will be serialized as DICOM tags, potentially corrupting RT output. This is pre-existing, but the PR introduces the fix pattern everywhere else and misses this file.
4. Blob URL memory leak in registerNaturalizedDatasetForLocalWadouri
dicomImageLoader.wadouri.fileManager.add(blob) registers a blob URL that is never revoked. Each report store/download/clipboard operation accumulates a blob in memory for the session lifetime. No fileManager.remove() call exists anywhere in the codebase. For long sessions with many reports this could be meaningful.
5. Duplicate getDatasetTransferSyntaxUID function
Defined privately in both dicomWriter.ts and DicomWebDataSource/index.ts with slightly different fallback logic (the latter falls back to EXPLICIT_VR_LITTLE_ENDIAN; the former returns undefined). The DicomWebDataSource version should import from dicomWriter.ts to avoid future divergence.
6. Dual transferSyntaxUID / transferSyntaxUid keys in segmentationConfig.ts
options.transferSyntaxUID = transferSyntaxUID;
options.transferSyntaxUid = transferSyntaxUID; // camelCase inconsistencyThis is clearly a workaround for an API inconsistency in @cornerstonejs/adapters. Without a comment referencing the upstream issue or PR, future readers will remove one key as an obvious duplicate and silently break one code path.
7. LABELMAP_SEG_SOP_CLASS_UID defined twice
Defined at the top of getSopClassHandlerModule.ts and again in segmentationConfig.ts. The local definition in getSopClassHandlerModule.ts is dead code since getSegmentationParserType is imported from segmentationConfig.ts.
8. registerNaturalizedDatasetForLocalWadouri.js formatting
Every line in this new file is separated by an extra blank line — the entire file is double-spaced. Likely an editor/diff artifact. Makes the file hard to read and should be cleaned up before merge.
Minor / Nits
- Verbose
console.infoin production paths —_logSegImageIdsdumps full imageId arrays on every SEG load;registerNaturalizedDatasetForLocalWadourilogs blob byte size and all frame imageIds. Useful for debugging, noisy in production. - PR checklist not filled out — all checkboxes use
[]; tested OS/Node/Browser fields are blank. @cornerstonejs/adapters 4.22.8still depends on dcmjs 0.49.4 — perbun.lock, adapters may bundle their own copy alongside OHIF's 0.51.1. Worth verifying the adapters' behavior is compatible with the API surface used indicomWriter.ts.
What's Done Well
- Event listener cleanup —
try/finallyaroundSEGMENTATION_LOAD_PROGRESScorrectly fixes a pre-existing leak where N SEG loads accumulated N persistent listeners. - Non-enumerable runtime properties — clean, surgical fix preventing dcmjs from writing OHIF bookkeeping fields into stored DICOM output.
dicomWriter.tscentralization — single place for dcmjs serialization with consistent options and transfer syntax preservation; a clear improvement over scattered ad-hocdatasetToBlobcalls._getSegDataSourcefallback logic — correctly prefersdicomlocalfor local-scheme URLs, falls back to the active data source.appendFrameQueryToImageId— fixes a real bug where&frame=was always used regardless of whether the base URL already had query params.- Null-guards in
addOrUpdateSegmentationanddetermineViewportAndSegmentationType— reasonable defensive programming; the warn log in each case makes skips visible rather than truly silent. - SEG viewport stack fix — passing
[referencedDisplaySet, segDisplaySet]instead of just[segDisplaySet]is a meaningful and correct fix; the old code left the viewport with only derived labelmap imageIds as the primary stack, which are not displayable as grayscale images.
Summary
The critical issue is the event ordering in SegmentationService.ts: SEGMENTATION_LOADING_COMPLETE is broadcast before the segmentation is in state, so the new CornerstoneViewportService subscriber calls addSegmentationRepresentation too early, hits the null-guard introduced by this same PR, and silently exits — meaning the labelmap overlay is never applied. This would be caught only by end-to-end testing; the null-guard makes it invisible as a code-level error.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors SEG multiframe loading to use imageId-based parsing via ChangesSEG Loading, DICOM Writing, and Workspace Infrastructure
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over OHIFCornerstoneSEGViewport,segmentationService: SEG Load and Deferred Overlay Application
end
participant OHIFCornerstoneSEGViewport
participant getSopClassHandlerModule
participant MetadataProvider
participant adaptersSEG
participant segmentationService
participant CornerstoneViewportService
OHIFCornerstoneSEGViewport->>getSopClassHandlerModule: loadSEG(segDisplaySet)
getSopClassHandlerModule->>getSopClassHandlerModule: _getSegDataSource → segImageIdStr
getSopClassHandlerModule->>getSopClassHandlerModule: _resolveFrameImageIds → frameImageIds
getSopClassHandlerModule->>MetadataProvider: addImageIdToUIDs per frame
getSopClassHandlerModule->>segmentationService: subscribe SEGMENTATION_LOAD_PROGRESS
getSopClassHandlerModule->>adaptersSEG: createFromDicomSegImageId(segImageIdForMetadata, frameImageIds, parserType)
adaptersSEG-->>getSopClassHandlerModule: labelMaps
getSopClassHandlerModule->>segmentationService: unsubscribe + emit SEGMENTATION_LOADING_COMPLETE
CornerstoneViewportService->>segmentationService: getSegmentation(segmentationId)
alt segmentation already present
CornerstoneViewportService->>segmentationService: addSegmentationRepresentation(immediate)
else segmentation not yet loaded
CornerstoneViewportService->>segmentationService: subscribe SEGMENTATION_LOADING_COMPLETE
segmentationService-->>CornerstoneViewportService: event(segDisplaySetInstanceUID match)
CornerstoneViewportService->>segmentationService: addSegmentationRepresentation(deferred)
CornerstoneViewportService->>segmentationService: unsubscribe
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
extensions/default/src/customizations/reportDialogCustomization.tsx (1)
25-49:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
minSeriesNumberis destructured but missing fromReportDialogProps(TS compile break).At Line 44,
minSeriesNumberis read from props, butReportDialogProps(Line 25-38) does not declare it. This creates a type error and breaks type-safe compilation.Suggested fix
type ReportDialogProps = { dataSources: DataSource[]; modality?: string; predecessorImageId?: string; + minSeriesNumber?: number; hide: () => void; onSave: (data: { reportName: string; dataSource: string | null; series: string | null; priorSeriesNumber: number; }) => void; onCancel: () => void; enableDownload?: boolean; };🤖 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/customizations/reportDialogCustomization.tsx` around lines 25 - 49, ReportDialog destructures minSeriesNumber but ReportDialogProps doesn't declare it; add an optional numeric property minSeriesNumber?: number to the ReportDialogProps type (or change to required number if intended), and keep the existing default in the ReportDialog function signature; update any callers if needed to satisfy the new prop contract so the TypeScript compile error is resolved (refer to ReportDialogProps and the ReportDialog function).extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx (1)
83-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle unresolved referenced display set before rendering overlay stack.
displaySetService.getDisplaySetByUID(...)can returnundefinedeven whenreferencedDisplaySetInstanceUIDexists. On that path, Line 104 passesundefinedintodisplaySets, and Line 260 dereferencesreferencedDisplaySet.displaySetInstanceUID, which can crash the viewport. Add an explicit guard right after Line 83 and short-circuit (or invoke the same missing-reference handler) whenreferencedDisplaySetis absent.Suggested patch
const referencedDisplaySet = displaySetService.getDisplaySetByUID( referencedDisplaySetInstanceUID ); + if (!referencedDisplaySet) { + const missingReferenceDisplaySetHandler = customizationService.getCustomization( + 'missingReferenceDisplaySetHandler' + ); + if (typeof missingReferenceDisplaySetHandler === 'function') { + missingReferenceDisplaySetHandler(); + } + return null; + } const referencedDisplaySetMetadata = _getReferencedDisplaySetMetadata( referencedDisplaySet, segDisplaySet );Also applies to: 104-104, 260-261
🤖 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 83 - 90, After calling displaySetService.getDisplaySetByUID(referencedDisplaySetInstanceUID), add an explicit guard for referencedDisplaySet === undefined and short-circuit rendering (or call the component's existing missing-reference handler) instead of continuing; this prevents passing undefined into displaySets and avoids dereferencing referencedDisplaySet.displaySetInstanceUID later. Update the code around the getDisplaySetByUID call and the _getReferencedDisplaySetMetadata(referencedDisplaySet, segDisplaySet) usage so that when referencedDisplaySet is missing you return early (or invoke the same missing-reference handler used elsewhere) and do not call _getReferencedDisplaySetMetadata or access referencedDisplaySet.displaySetInstanceUID.extensions/cornerstone/src/commandsModule.ts (1)
1995-2002:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
backgroundin the disabled tool config too.When
enabledByDefaultis false, the registeredSegmentLabeltool dropssegmentLabelConfig.background, so the same customization renders differently depending on the initial enabled state. Includebackgroundin the disabled branch as well.Suggested fix
disabledTools.push({ toolName: toolNames.SegmentLabel, configuration: { hoverTimeout: segmentLabelConfig?.hoverTimeout ?? 1, color: segmentLabelConfig?.labelColor, + background: segmentLabelConfig?.background, }, });🤖 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/src/commandsModule.ts` around lines 1995 - 2002, The disabled-tools branch drops segmentLabelConfig.background causing inconsistent rendering; update the object pushed into disabledTools (the entry with toolName: toolNames.SegmentLabel) to include background: segmentLabelConfig?.background alongside hoverTimeout and labelColor so the disabled configuration mirrors the enabled registration (ensure the key is added to the configuration object referenced by disabledTools, using the same nullable access pattern as hoverTimeout/labelColor).extensions/default/src/DicomLocalDataSource/index.js (1)
147-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing per-frame metadata registration for multiframe instances.
DicomLocalDataSourceregisters metadata once per instance (line 164-169), but multiframe instances require per-frame registration likeDicomWebDataSource(lines 565-580) andregisterNaturalizedDatasetForLocalWadouri(lines 91-97). Without per-frame metadata mappings, imageIds with frame queries (e.g.,wadouri:...?frame=2) won't resolve correctly.Additionally, line 168 incorrectly uses the instance index within the series (
index + 1) as the frame number, rather than iterating over the frames within each instance.🔧 Proposed fix to register per-frame metadata
aSeries.instances.forEach((instance, index) => { const { url: imageId, StudyInstanceUID, SeriesInstanceUID, SOPInstanceUID, } = instance; setNonEnumerableInstanceProperty(instance, 'imageId', imageId); makeExistingPropertiesNonEnumerable(instance); + const numberOfFrames = instance.NumberOfFrames || 1; + - // Add imageId specific mapping to this data as the URL isn't necessarily WADO-URI. - metadataProvider.addImageIdToUIDs(imageId, { - StudyInstanceUID, - SeriesInstanceUID, - SOPInstanceUID, - frameNumber: isMultiframe ? index + 1 : 1, - }); + // Register metadata for each frame + for (let frame = 1; frame <= numberOfFrames; frame++) { + const frameImageId = numberOfFrames > 1 + ? appendFrameQueryToImageId(imageId, frame) + : imageId; + + metadataProvider.addImageIdToUIDs(frameImageId, { + StudyInstanceUID, + SeriesInstanceUID, + SOPInstanceUID, + frameNumber: numberOfFrames > 1 ? frame : 1, + }); + } });🤖 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/DicomLocalDataSource/index.js` around lines 147 - 169, The current loop in DicomLocalDataSource registers one mapping per instance using index+1 as frameNumber, which fails for multiframe instances; update the aSeries.instances.forEach block so that for multiframe instances you register an imageId mapping for each actual frame (1..NumberOfFrames) by calling metadataProvider.addImageIdToUIDs with frameNumber equal to the frame index + 1 for each frame, not the instance index, mirroring the per-frame registration logic used in DicomWebDataSource and registerNaturalizedDatasetForLocalWadouri; ensure you still call setNonEnumerableInstanceProperty and makeExistingPropertiesNonEnumerable for the instance, but add the additional per-frame registrations when aSeries.instances[0].NumberOfFrames > 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-dicom-seg/src/utils/segLocalImageIds.ts`:
- Around line 5-13: stripFrameFromImageId currently chops everything after the
first occurrence of "?frame=" or "&frame=", which discards other query params;
update stripFrameFromImageId and appendFrameToImageId to only remove or replace
the "frame" query parameter while preserving all other query parameters and the
original separator/ordering. Specifically, in stripFrameFromImageId(imageId:
string) parse the query portion (text after the first '?') into key/value pairs,
drop only the "frame" key, rebuild the query string (empty if no params left)
and return basePath + (rebuiltQuery ? '?' + rebuiltQuery : ''); then change
appendFrameToImageId(baseImageId: string, frame: number) to call the updated
stripFrameFromImageId, then append or set the "frame" param into the existing
query string (using '&' when a query exists) so other loader options or
transfer-syntax hints remain intact.
In
`@extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts`:
- Around line 1310-1323: The wait for SEGMENTATION_LOADING_COMPLETE in
_addOverlayRepresentations() can hang forever; change the Promise returned by
the block that subscribes via segmentationService.subscribe to include a bounded
timeout and an explicit failure cleanup: start a timer (e.g., 10s) when
subscribing, and if the timeout fires or a segmentation-load-failure event
occurs, unsubscribe using the captured unsubscribe(), reject or resolve the
Promise (resolve to a no-op fallback) and ensure applyRepresentation() is not
called; keep using segmentationService.EVENTS.SEGMENTATION_LOADING_COMPLETE and
the segmentationId check, but add handling for a
SEGMENTATION_LOADING_FAILED/timeout path and ensure
setViewportData()/updateViewport() can proceed by resolving instead of leaving
the Promise pending.
In `@extensions/default/src/DicomLocalDataSource/index.js`:
- Around line 231-233: getImageIdsForInstance currently returns undefined when
baseImageId is falsy, which causes callers that push its result directly into
the imageIds array to inject undefined; change getImageIdsForInstance to return
an empty array ([]) instead of undefined when baseImageId is missing so callers
(the code that pushes/concat results into imageIds) will safely add no entries,
and verify any use of getImageIdsForInstance (the call sites that push/concat
its return) expects an array rather than possibly undefined.
---
Outside diff comments:
In
`@extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx`:
- Around line 83-90: After calling
displaySetService.getDisplaySetByUID(referencedDisplaySetInstanceUID), add an
explicit guard for referencedDisplaySet === undefined and short-circuit
rendering (or call the component's existing missing-reference handler) instead
of continuing; this prevents passing undefined into displaySets and avoids
dereferencing referencedDisplaySet.displaySetInstanceUID later. Update the code
around the getDisplaySetByUID call and the
_getReferencedDisplaySetMetadata(referencedDisplaySet, segDisplaySet) usage so
that when referencedDisplaySet is missing you return early (or invoke the same
missing-reference handler used elsewhere) and do not call
_getReferencedDisplaySetMetadata or access
referencedDisplaySet.displaySetInstanceUID.
In `@extensions/cornerstone/src/commandsModule.ts`:
- Around line 1995-2002: The disabled-tools branch drops
segmentLabelConfig.background causing inconsistent rendering; update the object
pushed into disabledTools (the entry with toolName: toolNames.SegmentLabel) to
include background: segmentLabelConfig?.background alongside hoverTimeout and
labelColor so the disabled configuration mirrors the enabled registration
(ensure the key is added to the configuration object referenced by
disabledTools, using the same nullable access pattern as
hoverTimeout/labelColor).
In `@extensions/default/src/customizations/reportDialogCustomization.tsx`:
- Around line 25-49: ReportDialog destructures minSeriesNumber but
ReportDialogProps doesn't declare it; add an optional numeric property
minSeriesNumber?: number to the ReportDialogProps type (or change to required
number if intended), and keep the existing default in the ReportDialog function
signature; update any callers if needed to satisfy the new prop contract so the
TypeScript compile error is resolved (refer to ReportDialogProps and the
ReportDialog function).
In `@extensions/default/src/DicomLocalDataSource/index.js`:
- Around line 147-169: The current loop in DicomLocalDataSource registers one
mapping per instance using index+1 as frameNumber, which fails for multiframe
instances; update the aSeries.instances.forEach block so that for multiframe
instances you register an imageId mapping for each actual frame
(1..NumberOfFrames) by calling metadataProvider.addImageIdToUIDs with
frameNumber equal to the frame index + 1 for each frame, not the instance index,
mirroring the per-frame registration logic used in DicomWebDataSource and
registerNaturalizedDatasetForLocalWadouri; ensure you still call
setNonEnumerableInstanceProperty and makeExistingPropertiesNonEnumerable for the
instance, but add the additional per-frame registrations when
aSeries.instances[0].NumberOfFrames > 1.
🪄 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: 79ed8425-84cc-4f91-ac64-296a1ea79516
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (37)
addOns/package.jsonextensions/cornerstone-dicom-seg/package.jsonextensions/cornerstone-dicom-seg/src/commandsModule.tsextensions/cornerstone-dicom-seg/src/customizations/segmentationCustomization.tsextensions/cornerstone-dicom-seg/src/getCustomizationModule.tsextensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.tsextensions/cornerstone-dicom-seg/src/index.tsxextensions/cornerstone-dicom-seg/src/utils/segLocalImageIds.tsextensions/cornerstone-dicom-seg/src/utils/segmentationConfig.tsextensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsxextensions/cornerstone-dicom-sr/package.jsonextensions/cornerstone-dynamic-volume/package.jsonextensions/cornerstone/package.jsonextensions/cornerstone/src/commandsModule.tsextensions/cornerstone/src/services/SegmentationService/SegmentationService.tsextensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.tsextensions/cornerstone/src/utils/segmentationHandlers.tsextensions/default/package.jsonextensions/default/src/DicomLocalDataSource/index.jsextensions/default/src/DicomWebDataSource/index.tsextensions/default/src/commandsModule.tsextensions/default/src/customizations/reportDialogCustomization.tsxextensions/default/src/utils/appendFrameQueryToImageId.jsextensions/default/src/utils/dicomWriter.tsextensions/default/src/utils/registerNaturalizedDatasetForLocalWadouri.jsextensions/dicom-pdf/package.jsonextensions/dicom-video/package.jsonextensions/measurement-tracking/package.jsonextensions/test-extension/package.jsonextensions/tmtv/package.jsonpackage.jsonplatform/app/package.jsonplatform/app/public/config/default.jsplatform/core/package.jsonplatform/core/src/classes/MetadataProvider.tsplatform/core/src/utils/combineFrameInstance.tsplatform/core/src/utils/imageIdToURI.js
wayfarer3130
force-pushed
the
feat/load-seg-images
branch
from
599f2b4 to
1324bc5
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/codeql.yml (1)
14-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the PR branch filter to
master.CodeQL is still scoped to
pull_request.branches: ["main"], but this PR targetsmasterand the Playwright workflow is already keyed offmaster. In the current state, CodeQL won't run for normal PRs into the active default branch.🤖 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 @.github/workflows/codeql.yml around lines 14 - 16, Update the CodeQL workflow trigger so pull_request is run against the repository's active branch by changing the pull_request.branches setting from ["main"] to ["master"]; edit the on: pull_request block in .github/workflows/codeql.yml (the pull_request.branches array) to include "master" instead of "main" so CodeQL runs for PRs targeting master.
🧹 Nitpick comments (4)
extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts (1)
420-439: 💤 Low valueConsider logging when falling back to alternative imageId sources.
The fallback cascade silently proceeds through three different sources for
imageIds. If the primary source (referencedDisplaySet.imageIds) is unexpectedly empty due to a configuration issue, this silent fallback could mask the root cause and make debugging harder.Also, backfilling
referencedDisplaySet.imageIdson line 438 mutates the display set object, which could cause unexpected side effects if other code expects that property to remain empty.🤖 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/getSopClassHandlerModule.ts` around lines 420 - 439, Detect and log which fallback source provided imageIds instead of silently proceeding: when falling back from referencedDisplaySet.imageIds to dataSource.getImageIdsForDisplaySet(referencedDisplaySet) and then to (referencedDisplaySet as {images?})...map(...), emit a warning (include referencedDisplaySet.id or other identifying fields and the selected source) so callers can trace unexpected empty primary data; assign the chosen IDs to segDisplaySet.referencedImageIds as you already do but do not mutate referencedDisplaySet.imageIds in place—either remove that backfill or only apply it to a shallow clone/copy if truly required, and ensure logs reflect that no mutation was performed.tests/SegmentationPanel.spec.ts (1)
23-29: ⚡ Quick winKeep segment-count assertions behind
getSegmentCount().These retrying checks fixed the timing issue, but this project prefers count assertions through the page object API. A retrying
expect.poll(async () => panel.getSegmentCount()).toBe(1)/toBe(0)keeps the stabilization and avoids coupling the spec topanel.rows.Based on learnings: In OHIF/Viewers Playwright tests, prefer asserting segment counts via the relevant page object’s
getSegmentCount()method and do not replace those assertions with locator-basedtoHaveCount.Also applies to: 40-41
🤖 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 `@tests/SegmentationPanel.spec.ts` around lines 23 - 29, Replace the locator-based count assertions that use panel.rows and toHaveCount with retrying assertions that call the page object API: use expect.poll(async () => panel.getSegmentCount()).toBe(1) for the expectation that one segment exists (and similarly toBe(0) where checking for zero), and keep the text assertion using panel.nthSegment(0).locator unchanged; specifically, change the toHaveCount on panel.rows to a poll on panel.getSegmentCount(), and apply the same replacement for the assertions referenced at lines 40-41.Source: Learnings
extensions/default/src/utils/appendFrameQueryToImageId.js (1)
4-9: ⚡ Quick winAdd JSDoc and input validation.
The function lacks documentation of its contract and doesn't validate the
frameparameter. Consider adding JSDoc to clarify expected input format and return value, plus basic validation.📝 Proposed JSDoc and validation
+/** + * Appends or updates the frame query parameter in a wadouri imageId. + * Removes any existing frame parameter and appends the new frame number. + * + * `@param` {string} baseImageId - The base imageId (e.g., 'wadouri:blob:...') + * `@param` {number} frame - The frame number (1-based) + * `@returns` {string} The imageId with frame parameter appended + * `@example` + * appendFrameQueryToImageId('wadouri:file.dcm', 2) + * // Returns: 'wadouri:file.dcm?frame=2' + */ export function appendFrameQueryToImageId(baseImageId, frame) { + if (typeof baseImageId !== 'string' || !baseImageId) { + throw new Error('baseImageId must be a non-empty string'); + } + if (typeof frame !== 'number' || frame < 1 || !Number.isInteger(frame)) { + throw new Error('frame must be a positive integer'); + } + const withoutFrame = baseImageId.split('&frame=')[0].split('?frame=')[0]; const separator = withoutFrame.includes('?') ? '&' : '?'; return `${withoutFrame}${separator}frame=${frame}`; }🤖 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/utils/appendFrameQueryToImageId.js` around lines 4 - 9, Add JSDoc for appendFrameQueryToImageId that documents parameters (baseImageId: string, frame: number|string expected to be a non-negative integer or numeric string) and the returned string (image id with appended or replaced frame query). Add input validation at the start of appendFrameQueryToImageId: ensure baseImageId is a non-empty string and frame is a finite non-negative integer (or numeric string coerced to integer); if invalid, throw a TypeError with a clear message. Keep the existing logic using withoutFrame and separator unchanged but coerce/validate frame before constructing the return value.Dockerfile (1)
31-31: ⚡ Quick winConsider pinning pnpm to the same version as packageManager.
The Dockerfile installs
pnpm@11(major version only) whilepackage.jsonpinspnpm@11.5.2. This inconsistency could lead to different behavior between local development and Docker builds if pnpm 11.x introduces breaking changes in minor releases.♻️ Proposed fix
-RUN npm install -g pnpm@11 +RUN npm install -g pnpm@11.5.2🤖 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 `@Dockerfile` at line 31, The Dockerfile currently installs pnpm with a floating major version via the RUN npm install -g pnpm@11 instruction; update that to install the exact version declared in package.json's "packageManager" field (e.g., pnpm@11.5.2) so Docker builds match local dev; ensure the RUN line is pinned to the same version string used in package.json and keep them in sync going forward.
🤖 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 @.github/workflows/build-docs.yml:
- Around line 82-84: The current validation only checks for any .html file in
./coverage-artifact which allows partial artifacts; update the conditional that
validates the downloaded artifact (the shell if that references
./coverage-artifact and the echo with coverage-report-pr and
steps.find_pr_run.outputs.run_id) to assert the presence of the full expected
asset set (e.g., ./coverage-artifact/*.html plus ./coverage-artifact/base.css,
./coverage-artifact/prettify.js, ./coverage-artifact/sorter.js and any other
required coverage assets) before proceeding to the copy step; implement this by
extending the test to verify each required file exists (or a stricter pattern
match) and fail with the same error message if any are missing.
In `@extensions/default/package.json`:
- Around line 20-27: The package.json in the extension is missing the
devDependency declaration for "cross-env" while its scripts (e.g., the "dev" and
"build" npm scripts) reference it; add "cross-env": "7.0.3" to the
"devDependencies" object in extensions/default/package.json (and likewise in
extensions/measurement-tracking/package.json) so the scripts are self-contained
and consistent with other extensions; ensure you update the correct
package.json's "devDependencies" section and run a quick install to verify
scripts still work.
In `@extensions/default/src/utils/appendFrameQueryToImageId.js`:
- Line 5: The current one-liner in appendFrameQueryToImageId.js that builds
withoutFrame via baseImageId.split(...).split(...) can drop other query params;
replace that logic in the appendFrameQueryToImageId function by parsing
baseImageId's query string and removing only the "frame" parameter (e.g., use
URL/URLSearchParams or a precise regex) so other parameters remain untouched,
then reconstruct the URL preserving pathname and remaining search params before
appending the desired &frame= value; target the withoutFrame computation and
baseImageId handling to implement this change.
In `@modes/basic-dev-mode/package.json`:
- Around line 22-27: The scripts in the mode packages (scripts like "dev",
"build", "start", "dev:cornerstone" in the package.json for
modes/basic-dev-mode, modes/basic-test-mode, modes/basic, and
modes/longitudinal) call the cross-env binary but do not declare it locally; add
"cross-env" to each package's devDependencies (e.g., add "cross-env": "^X.Y.Z"
under devDependencies in each package.json), run install so the package has a
local binary, or alternatively change the scripts to invoke the
workspace-installed binary explicitly (e.g., via pnpm -w or a relative path),
ensuring the mode package scripts reliably find cross-env.
In `@platform/app/package.json`:
- Around line 103-104: Remove the duplicate top-level JSON key "productVersion"
found near the end of platform/app/package.json; keep the original declaration
(the one at the top of the file) and delete the later `"productVersion":
"3.4.0",` entry so there is only one productVersion key and the remaining
`"proxy": "http://localhost:8042"` entry stays intact.
In `@platform/cli/src/commands/utils/uninstallNPMPackage.js`:
- Around line 4-6: The execa('pnpm', ['remove', packageName]) call in
uninstallNPMPackage currently catches and logs errors which swallows failures;
change this so uninstall failures propagate: either remove the .catch handler
entirely or rethrow the caught error (throw err) inside the catch, and ensure
callers like removeMode.js can await and handle the rejection. Update the
uninstallNPMPackage function to let the promise reject on failure (or explicitly
throw) so downstream cleanup halts when pnpm remove fails.
In `@platform/core/package.json`:
- Line 21: The package.json entry "sideEffects": "false" uses a string instead
of a boolean; change the sideEffects value in platform/core/package.json from
the string "false" to the boolean false (i.e., sideEffects: false) so bundlers
recognize the package as side-effect-free and enable proper tree-shaking for
`@ohif/core`.
In `@platform/docs/docs/migration-guide/3p12-to-3p13/build-tooling.md`:
- Around line 25-27: The sentence "There is no Webpack fallback. Webpack and all
of its plugins have been removed from the dependency tree; the only supported
bundler is Rspack…" overstates removal; update it to scope the claim to the
app/extension build path (e.g., "There is no Webpack fallback for the
app/extension build. Webpack and its plugins have been removed from the
app/extension build dependency tree; other packages in the repo may still
declare webpack. The only supported bundler for that build path is Rspack…") so
it no longer claims repo-wide removal.
- Around line 402-403: Remove the stray closing MDX tags `</content>` and
`</invoke>` found at the end of the MDX document; open the file and delete those
two tags (and any other non-MDX orphaned tags) so the document ends with valid
MDX content, then run the MDX build/lint to confirm parsing succeeds.
In `@platform/docs/docs/migration-guide/3p12-to-3p13/node-version.md`:
- Around line 10-12: The sentence in node-version.md overstates changes by
claiming "every workspace package's `engines.node` field have been updated";
instead update the wording to be accurate: mention that the repository root
`.node-version` was bumped to 24 and that only specific workspace packages had
their `engines.node` fields updated (e.g., list or say "relevant runtime
packages" or enumerate the updated workspaces), and avoid asserting that
`platform/docs/package.json` or all workspaces were changed.
In `@platform/docs/docs/platform/extensions/index.md`:
- Around line 187-192: Replace the stale generated filename `pluginImport.js`
with the correct `pluginImports.js` in the paragraph that describes the
non-tracked internal import file; update the sentence to read that the final
registration and import of extensions happen inside a non-tracked file
`pluginImports.js` (internal use only) so it matches the surrounding migration
docs and the actual generated filename.
In `@platform/ui-next/package.json`:
- Around line 9-17: The package.json exposes "./tailwind.config" via the
"exports" map but the "files" array does not include tailwind.config.js, so the
file won't be published; update package.json to include "tailwind.config.js" (or
the path you used for the export) in the "files" array so that the exported
entry "./tailwind.config" is present in the published tarball (refer to the
"exports" entry "./tailwind.config" and the "files" array when making the
change).
In `@platform/ui/package.json`:
- Around line 23-24: The package.json scripts "build" and "build:package" use
cross-env but the package does not declare cross-env; add cross-env to this
package's devDependencies (e.g., in package.json add a devDependency entry for
"cross-env" with an appropriate version), so the script "build" has an explicit
local contract rather than relying on a hoisted/transitive binary; after adding,
run the package manager (pnpm) to update the lockfile and verify pnpm --filter
`@ohif/ui` run build works in isolation.
In `@tests/pages/RightPanelPageObject.ts`:
- Around line 227-231: RightPanelPageObject's rows and getSegmentCount use
page.getByTestId('data-row') which matches rows across the whole page; scope
them to the panel root instead. Replace page.getByTestId('data-row') in the rows
property and in getSegmentCount() with a panel-scoped locator (e.g., use an
existing root locator like this.panel or this.root.getByTestId('data-row'); if
none exists, add a panelRoot =
page.getByTestId('segmentation-panel')/page.getByTestId('<panel-testid>') and
use panelRoot.getByTestId('data-row')). Update both the rows field and the
getSegmentCount method to use that panel-scoped locator.
---
Outside diff comments:
In @.github/workflows/codeql.yml:
- Around line 14-16: Update the CodeQL workflow trigger so pull_request is run
against the repository's active branch by changing the pull_request.branches
setting from ["main"] to ["master"]; edit the on: pull_request block in
.github/workflows/codeql.yml (the pull_request.branches array) to include
"master" instead of "main" so CodeQL runs for PRs targeting master.
---
Nitpick comments:
In `@Dockerfile`:
- Line 31: The Dockerfile currently installs pnpm with a floating major version
via the RUN npm install -g pnpm@11 instruction; update that to install the exact
version declared in package.json's "packageManager" field (e.g., pnpm@11.5.2) so
Docker builds match local dev; ensure the RUN line is pinned to the same version
string used in package.json and keep them in sync going forward.
In `@extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts`:
- Around line 420-439: Detect and log which fallback source provided imageIds
instead of silently proceeding: when falling back from
referencedDisplaySet.imageIds to
dataSource.getImageIdsForDisplaySet(referencedDisplaySet) and then to
(referencedDisplaySet as {images?})...map(...), emit a warning (include
referencedDisplaySet.id or other identifying fields and the selected source) so
callers can trace unexpected empty primary data; assign the chosen IDs to
segDisplaySet.referencedImageIds as you already do but do not mutate
referencedDisplaySet.imageIds in place—either remove that backfill or only apply
it to a shallow clone/copy if truly required, and ensure logs reflect that no
mutation was performed.
In `@extensions/default/src/utils/appendFrameQueryToImageId.js`:
- Around line 4-9: Add JSDoc for appendFrameQueryToImageId that documents
parameters (baseImageId: string, frame: number|string expected to be a
non-negative integer or numeric string) and the returned string (image id with
appended or replaced frame query). Add input validation at the start of
appendFrameQueryToImageId: ensure baseImageId is a non-empty string and frame is
a finite non-negative integer (or numeric string coerced to integer); if
invalid, throw a TypeError with a clear message. Keep the existing logic using
withoutFrame and separator unchanged but coerce/validate frame before
constructing the return value.
In `@tests/SegmentationPanel.spec.ts`:
- Around line 23-29: Replace the locator-based count assertions that use
panel.rows and toHaveCount with retrying assertions that call the page object
API: use expect.poll(async () => panel.getSegmentCount()).toBe(1) for the
expectation that one segment exists (and similarly toBe(0) where checking for
zero), and keep the text assertion using panel.nthSegment(0).locator unchanged;
specifically, change the toHaveCount on panel.rows to a poll on
panel.getSegmentCount(), and apply the same replacement for the assertions
referenced at lines 40-41.
🪄 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: 53948896-9daa-42c0-9adc-4f931570491d
⛔ Files ignored due to path filters (42)
.webpack/resolveConfig.jsis excluded by!**/.webpack/**.webpack/rules/cssToJavaScript.jsis excluded by!**/.webpack/**.webpack/rules/transpileJavaScript.jsis excluded by!**/.webpack/**.webpack/webpack.base.jsis excluded by!**/.webpack/**bun.lockis excluded by!**/*.lockextensions/cornerstone-dicom-pmap/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-rt/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-seg/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-sr/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dynamic-volume/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/default/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-microscopy/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-pdf/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-video/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/measurement-tracking/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/test-extension/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/tmtv/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/usAnnotation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic-dev-mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic-test-mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/longitudinal/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/microscopy/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/preclinical-4d/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/segmentation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/tmtv/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/usAnnotation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/app/.webpack/rules/extractStyleChunks.jsis excluded by!**/.webpack/**platform/app/.webpack/webpack.pwa.jsis excluded by!**/.webpack/**platform/app/.webpack/writePluginImportsFile.jsis excluded by!**/.webpack/**platform/cli/templates/extension/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/cli/templates/mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/core/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/docs/bun.lockbis excluded by!**/bun.lockbplatform/docs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockplatform/i18n/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui-next/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui/a34f9d1faa5f3315.woff2is excluded by!**/*.woff2pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (107)
.circleci/config.yml.github/.dependabot.yaml.github/workflows/build-docs.yml.github/workflows/codeql.yml.github/workflows/playwright.yml.gitignore.netlify/build-deploy-preview.sh.netlify/package.json.node-version.npmrc.scripts/dev.shDockerfileaddOns/README.mdaddOns/externals/devDependencies/CHANGELOG.mdaddOns/externals/devDependencies/package.jsonaddOns/externals/dicom-microscopy-viewer/CHANGELOG.mdaddOns/externals/dicom-microscopy-viewer/package.jsonaddOns/package.jsonbunfig.tomlbunfig.update-lockfile.tomlextensions/cornerstone-dicom-pmap/package.jsonextensions/cornerstone-dicom-rt/package.jsonextensions/cornerstone-dicom-seg/package.jsonextensions/cornerstone-dicom-seg/src/commandsModule.tsextensions/cornerstone-dicom-seg/src/customizations/segmentationCustomization.tsextensions/cornerstone-dicom-seg/src/getCustomizationModule.tsextensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.tsextensions/cornerstone-dicom-seg/src/index.tsxextensions/cornerstone-dicom-seg/src/utils/segLocalImageIds.tsextensions/cornerstone-dicom-seg/src/utils/segmentationConfig.tsextensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsxextensions/cornerstone-dicom-sr/package.jsonextensions/cornerstone-dynamic-volume/package.jsonextensions/cornerstone/package.jsonextensions/cornerstone/src/commandsModule.tsextensions/cornerstone/src/services/SegmentationService/SegmentationService.tsextensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.tsextensions/cornerstone/src/utils/segmentationHandlers.tsextensions/default/package.jsonextensions/default/src/DicomLocalDataSource/index.jsextensions/default/src/DicomWebDataSource/index.tsextensions/default/src/ViewerLayout/index.tsxextensions/default/src/commandsModule.tsextensions/default/src/customizations/reportDialogCustomization.tsxextensions/default/src/utils/appendFrameQueryToImageId.jsextensions/default/src/utils/dicomWriter.tsextensions/default/src/utils/registerNaturalizedDatasetForLocalWadouri.jsextensions/dicom-microscopy/package.jsonextensions/dicom-pdf/package.jsonextensions/dicom-video/package.jsonextensions/measurement-tracking/package.jsonextensions/test-extension/package.jsonextensions/tmtv/package.jsonextensions/usAnnotation/package.jsonlerna.jsonmodes/basic-dev-mode/package.jsonmodes/basic-test-mode/package.jsonmodes/basic/package.jsonmodes/longitudinal/package.jsonmodes/microscopy/package.jsonmodes/preclinical-4d/package.jsonmodes/segmentation/package.jsonmodes/tmtv/package.jsonmodes/usAnnotation/package.jsonnetlify-lerna-cache.shnetlify-lerna-restore.shnetlify.tomlnx.jsonpackage.jsonplatform/app/cypress.config.tsplatform/app/cypress/integration/measurement-tracking/OHIFSaveMeasurements.spec.jsplatform/app/cypress/support/commands.jsplatform/app/package.jsonplatform/app/public/config/default.jsplatform/app/tailwind.config.jsplatform/cli/package.jsonplatform/cli/src/commands/createPackage.jsplatform/cli/src/commands/utils/getYarnInfo.jsplatform/cli/src/commands/utils/uninstallNPMPackage.jsplatform/cli/src/index.jsplatform/cli/templates/extension/dependencies.jsonplatform/cli/templates/mode/dependencies.jsonplatform/core/package.jsonplatform/core/src/classes/MetadataProvider.tsplatform/core/src/utils/combineFrameInstance.tsplatform/core/src/utils/imageIdToURI.jsplatform/docs/docs/migration-guide/3p12-to-3p13/build-tooling.mdplatform/docs/docs/migration-guide/3p12-to-3p13/index.mdplatform/docs/docs/migration-guide/3p12-to-3p13/node-version.mdplatform/docs/docs/migration-guide/3p12-to-3p13/package-manager.mdplatform/docs/docs/platform/extensions/index.mdplatform/docs/docs/platform/extensions/pluginConfig.mdplatform/docs/docs/platform/modes/index.mdplatform/docs/package.jsonplatform/i18n/package.jsonplatform/ui-next/package.jsonplatform/ui/package.jsonplaywright.config.tspnpm-workspace.yamlpublish-package.mjspublish-version.mjsrsbuild.config.tstests/FreehandROI.spec.tstests/SegmentationPanel.spec.tstests/globalSetup.tstests/pages/RightPanelPageObject.tsversion.mjs
💤 Files with no reviewable changes (11)
- addOns/README.md
- bunfig.update-lockfile.toml
- addOns/externals/devDependencies/package.json
- platform/app/cypress/integration/measurement-tracking/OHIFSaveMeasurements.spec.js
- nx.json
- netlify-lerna-restore.sh
- netlify-lerna-cache.sh
- addOns/externals/dicom-microscopy-viewer/package.json
- bunfig.toml
- addOns/package.json
- lerna.json
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- .npmrc
- .netlify/package.json
- .node-version
- platform/docs/docs/migration-guide/3p12-to-3p13/index.md
- platform/docs/docs/platform/modes/index.md
- platform/docs/docs/platform/extensions/pluginConfig.md
🚧 Files skipped from review as they are similar to previous changes (21)
- extensions/cornerstone-dicom-seg/src/commandsModule.ts
- extensions/cornerstone-dicom-seg/src/getCustomizationModule.ts
- extensions/cornerstone/src/commandsModule.ts
- extensions/cornerstone-dicom-seg/src/utils/segmentationConfig.ts
- platform/app/public/config/default.js
- extensions/cornerstone-dicom-seg/src/index.tsx
- platform/core/src/utils/imageIdToURI.js
- extensions/cornerstone-dicom-seg/src/customizations/segmentationCustomization.ts
- platform/core/src/utils/combineFrameInstance.ts
- extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx
- extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
- extensions/default/src/commandsModule.ts
- platform/core/src/classes/MetadataProvider.ts
- extensions/cornerstone/src/utils/segmentationHandlers.ts
- extensions/default/src/utils/dicomWriter.ts
- extensions/cornerstone-dicom-seg/src/utils/segLocalImageIds.ts
- extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts
- extensions/default/src/customizations/reportDialogCustomization.tsx
- extensions/default/src/DicomWebDataSource/index.ts
- extensions/default/src/utils/registerNaturalizedDatasetForLocalWadouri.js
- extensions/default/src/DicomLocalDataSource/index.js
Comment on lines
+10
to
+12
| OHIF 3.13 bumps the minimum supported Node.js runtime from 18 to **24**. | ||
| The `.node-version` file at the repository root and every workspace | ||
| package's `engines.node` field have been updated. |
There was a problem hiding this comment.
Avoid saying every workspace package now declares engines.node.
That overstates the migration relative to the supplied manifests: platform/docs/package.json still has no engines block. Wording this as “the relevant runtime packages” or enumerating the updated workspaces would keep the guide accurate.
🤖 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/docs/docs/migration-guide/3p12-to-3p13/node-version.md` around lines
10 - 12, The sentence in node-version.md overstates changes by claiming "every
workspace package's `engines.node` field have been updated"; instead update the
wording to be accurate: mention that the repository root `.node-version` was
bumped to 24 and that only specific workspace packages had their `engines.node`
fields updated (e.g., list or say "relevant runtime packages" or enumerate the
updated workspaces), and avoid asserting that `platform/docs/package.json` or
all workspaces were changed.
Comment on lines
+187
to
192
| For the full set of fields (`packageName`, `version`, `default`, `directory`, | ||
| the `public` section) and the three ways a plugin can be located (in-tree, | ||
| out-of-tree `directory`, or installed dependency), see the | ||
| [pluginConfig.json reference](./pluginConfig.md). | ||
|
|
||
| The final registration and import of the extensions happen inside a non-tracked file `pluginImport.js` (this file is also for internal use only). |
There was a problem hiding this comment.
Update the generated import filename in this section.
This paragraph now points readers at the current pluginConfig.json flow, but the next sentence still refers to pluginImport.js. The surrounding migration docs describe pluginImports.js, so this filename looks stale and will send readers to the wrong generated file.
🧰 Tools
🪛 LanguageTool
[style] ~192-~192: The adverb ‘also’ is commonly used to connect clauses and isn’t usually used at the end of a phrase or before a conjunction. Consider replacing it with a more formal alternative.
Context: ...ed file pluginImport.js (this file is also for internal use only). After an exten...
(ALSO_AS_WELL)
🤖 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/docs/docs/platform/extensions/index.md` around lines 187 - 192,
Replace the stale generated filename `pluginImport.js` with the correct
`pluginImports.js` in the paragraph that describes the non-tracked internal
import file; update the sentence to read that the final registration and import
of extensions happen inside a non-tracked file `pluginImports.js` (internal use
only) so it matches the surrounding migration docs and the actual generated
filename.
Comment on lines
+9
to
17
| "exports": { | ||
| "./tailwind.config": "./tailwind.config.js", | ||
| "./lib/*": "./src/lib/*.ts", | ||
| "./components/*": "./src/components/*.tsx", | ||
| ".": "./src/index.ts" | ||
| }, | ||
| "files": [ | ||
| "dist", | ||
| "README.md" |
There was a problem hiding this comment.
Publish the new ./tailwind.config export.
./tailwind.config now points at ./tailwind.config.js, but files still only whitelists dist and README.md. After publish, consumers importing @ohif/ui-next/tailwind.config will resolve an export that is missing from the tarball.
📦 Suggested fix
"files": [
"dist",
- "README.md"
+ "README.md",
+ "tailwind.config.js"
],📝 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.
Suggested change
| "exports": { | |
| "./tailwind.config": "./tailwind.config.js", | |
| "./lib/*": "./src/lib/*.ts", | |
| "./components/*": "./src/components/*.tsx", | |
| ".": "./src/index.ts" | |
| }, | |
| "files": [ | |
| "dist", | |
| "README.md" | |
| "exports": { | |
| "./tailwind.config": "./tailwind.config.js", | |
| "./lib/*": "./src/lib/*.ts", | |
| "./components/*": "./src/components/*.tsx", | |
| ".": "./src/index.ts" | |
| }, | |
| "files": [ | |
| "dist", | |
| "README.md", | |
| "tailwind.config.js" | |
| ], |
🤖 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/ui-next/package.json` around lines 9 - 17, The package.json exposes
"./tailwind.config" via the "exports" map but the "files" array does not include
tailwind.config.js, so the file won't be published; update package.json to
include "tailwind.config.js" (or the path you used for the export) in the
"files" array so that the exported entry "./tailwind.config" is present in the
published tarball (refer to the "exports" entry "./tailwind.config" and the
"files" array when making the change).
Comment on lines
+227
to
231
| // Retrying-friendly locator for `expect(...).toHaveCount(n)` — prefer this | ||
| // over the one-shot getSegmentCount() when asserting row counts. | ||
| rows: page.getByTestId('data-row'), | ||
| getSegmentCount: async () => { | ||
| return await page.getByTestId('data-row').count(); |
There was a problem hiding this comment.
Scope segmentation rows to the panel instead of the whole page.
page.getByTestId('data-row') is the shared DataRow marker, and this page object also uses it for measurements. With tests/SegmentationPanel.spec.ts now asserting exact counts through panel.rows, any unrelated data-row elsewhere on the page can make the segmentation checks fail even when the panel is correct. Build rows and getSegmentCount() from a panel-scoped root locator instead.
🤖 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 `@tests/pages/RightPanelPageObject.ts` around lines 227 - 231,
RightPanelPageObject's rows and getSegmentCount use page.getByTestId('data-row')
which matches rows across the whole page; scope them to the panel root instead.
Replace page.getByTestId('data-row') in the rows property and in
getSegmentCount() with a panel-scoped locator (e.g., use an existing root
locator like this.panel or this.root.getByTestId('data-row'); if none exists,
add a panelRoot =
page.getByTestId('segmentation-panel')/page.getByTestId('<panel-testid>') and
use panelRoot.getByTestId('data-row')). Update both the rows field and the
getSegmentCount method to use that panel-scoped locator.
Resolved version-bump conflicts: kept workspace:* for internal @ohif/* deps and dcmjs 0.52.0 (feature branch convention), took master's beta.90 version bumps, root version, and the playwright self-hosted-runner pnpm fix, and dropped a duplicate productVersion in platform/app. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # extensions/cornerstone-dicom-pmap/package.json # extensions/cornerstone-dicom-rt/package.json # extensions/cornerstone-dicom-seg/package.json # extensions/cornerstone-dicom-sr/package.json # extensions/cornerstone-dynamic-volume/package.json # extensions/cornerstone/package.json # extensions/default/package.json # extensions/dicom-microscopy/package.json # extensions/dicom-pdf/package.json # extensions/dicom-video/package.json # extensions/measurement-tracking/package.json # extensions/test-extension/package.json # extensions/tmtv/package.json # extensions/usAnnotation/package.json # modes/basic-dev-mode/package.json # modes/basic-test-mode/package.json # modes/basic/package.json # modes/longitudinal/package.json # modes/microscopy/package.json # modes/preclinical-4d/package.json # modes/segmentation/package.json # modes/tmtv/package.json # modes/usAnnotation/package.json # platform/app/package.json # platform/core/package.json
…into feat/load-seg-images
# Conflicts: # extensions/cornerstone-dicom-pmap/package.json # extensions/cornerstone-dicom-rt/package.json # extensions/cornerstone-dicom-seg/package.json # extensions/cornerstone-dicom-sr/package.json # extensions/cornerstone-dynamic-volume/package.json # extensions/cornerstone/package.json # extensions/default/package.json # extensions/dicom-microscopy/package.json # extensions/dicom-pdf/package.json # extensions/dicom-video/package.json # extensions/measurement-tracking/package.json # extensions/test-extension/package.json # extensions/tmtv/package.json # extensions/usAnnotation/package.json # modes/basic-dev-mode/package.json # modes/basic-test-mode/package.json # modes/basic/package.json # modes/longitudinal/package.json # modes/microscopy/package.json # modes/preclinical-4d/package.json # modes/segmentation/package.json # modes/tmtv/package.json # modes/usAnnotation/package.json # platform/app/package.json # platform/core/package.json
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 `@platform/app/package.json`:
- Line 3: The postcss configuration is broken due to inconsistent package
removals. In the root package.json devDependencies, add the missing
`autoprefixer` package that is referenced in rsbuild.config.ts postcss options,
and then either remove the obsolete postcss.config.js file entirely (if the
inline rsbuild configuration is sufficient) or update postcss.config.js to
remove all references to the missing packages cssnano, postcss-import, and
postcss-preset-env. Ensure that the postcss configuration sources are aligned -
either rely solely on the rsbuild.config.ts inline configuration or restore the
removed packages to support postcss.config.js.
🪄 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: 422723ea-f225-4e6b-b991-add3a8950acc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
extensions/cornerstone-dicom-sr/package.jsonextensions/cornerstone-dynamic-volume/package.jsonextensions/cornerstone/package.jsonextensions/default/package.jsonextensions/dicom-pdf/package.jsonextensions/dicom-video/package.jsonextensions/measurement-tracking/package.jsonextensions/test-extension/package.jsonextensions/tmtv/package.jsonplatform/app/package.jsonplatform/core/package.jsonpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (2)
- extensions/test-extension/package.json
- extensions/dicom-pdf/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions/default/package.json
- extensions/tmtv/package.json
- extensions/dicom-video/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@platform/app/package.json`:
- Line 3: The postcss configuration is broken due to inconsistent package
removals. In the root package.json devDependencies, add the missing
`autoprefixer` package that is referenced in rsbuild.config.ts postcss options,
and then either remove the obsolete postcss.config.js file entirely (if the
inline rsbuild configuration is sufficient) or update postcss.config.js to
remove all references to the missing packages cssnano, postcss-import, and
postcss-preset-env. Ensure that the postcss configuration sources are aligned -
either rely solely on the rsbuild.config.ts inline configuration or restore the
removed packages to support postcss.config.js.
🪄 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: 422723ea-f225-4e6b-b991-add3a8950acc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
extensions/cornerstone-dicom-sr/package.jsonextensions/cornerstone-dynamic-volume/package.jsonextensions/cornerstone/package.jsonextensions/default/package.jsonextensions/dicom-pdf/package.jsonextensions/dicom-video/package.jsonextensions/measurement-tracking/package.jsonextensions/test-extension/package.jsonextensions/tmtv/package.jsonplatform/app/package.jsonplatform/core/package.jsonpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (2)
- extensions/test-extension/package.json
- extensions/dicom-pdf/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- extensions/default/package.json
- extensions/tmtv/package.json
- extensions/dicom-video/package.json
🛑 Comments failed to post (1)
platform/app/package.json (1)
3-3:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check for Rspack CSS configuration and any remaining postcss usage echo "=== Rspack configuration files ===" fd -e js -e ts -e json 'rspack|webpack' platform/app echo -e "\n=== Remaining postcss references ===" rg -n --type-add 'config:*.{js,json,ts}' --type=config 'postcss' platform/app echo -e "\n=== CSS imports in source ===" rg -n --type=typescript --type=javascript "\.css|\.scss|\.sass" platform/app/src | head -20Repository: OHIF/Viewers
Length of output: 299
🏁 Script executed:
# Check postcss.config.js files and their content echo "=== platform/app/postcss.config.js ===" cat platform/app/postcss.config.js echo -e "\n=== Root postcss.config.js ===" cat postcss.config.js echo -e "\n=== platform/app/package.json (checking devDependencies) ===" rg -A 50 '"devDependencies"' platform/app/package.json echo -e "\n=== Look for Rspack or build config in root ===" ls -la | grep -E 'rspack|webpack|build' echo -e "\n=== Check root level config files ===" fd -e js -e json -e ts 'rspack|webpack|build' . --max-depth=2 | grep -v node_modules | head -20Repository: OHIF/Viewers
Length of output: 1225
🏁 Script executed:
# Check root package.json for postcss dependencies echo "=== Root package.json devDependencies (postcss related) ===" rg -A 150 '"devDependencies"' package.json | grep -E 'postcss|cssnano' echo -e "\n=== rsbuild.config.ts content ===" head -100 rsbuild.config.ts echo -e "\n=== Check for other build configs ===" ls -la platform/app/*.config.* 2>/dev/null | head -20 echo -e "\n=== Look for webpack/rspack config in platform/app ===" fd 'config' platform/app --max-depth 2Repository: OHIF/Viewers
Length of output: 4907
🏁 Script executed:
# Verify postcss-loader presence and exact setup echo "=== platform/app/package.json - check postcss-loader ===" grep -i postcss platform/app/package.json echo -e "\n=== Root package.json - postcss related ===" grep -E 'postcss|cssnano' package.json echo -e "\n=== Check if postcss.config.js is actually used ===" rg -n 'postcss\.config' . --type-add 'config:*.{js,json,ts}' | grep -v node_modules echo -e "\n=== Verify what postcss-loader version and which package ===" cat package.json | grep -A 2 -B 2 postcss-loaderRepository: OHIF/Viewers
Length of output: 555
🏁 Script executed:
# Final verification: confirm postcss packages are actually removed echo "=== Root package.json - all devDependencies to check for postcss packages ===" rg -B 5 -A 100 '"devDependencies"' package.json | grep -E '^\s+"(postcss|cssnano|autoprefixer)' || echo "None found in root devDeps" echo -e "\n=== Verify the exact packages removed from platform/app ===" echo "The AI summary claimed removed: cssnano, postcss-import, postcss-preset-env" echo "Let's check if they're in root package.json:" rg '(cssnano|postcss-import|postcss-preset-env)' package.json echo -e "\n=== Check if autoprefixer is available (used in rsbuild.config.ts) ===" rg 'autoprefixer' package.jsonRepository: OHIF/Viewers
Length of output: 419
Critical: CSS processing broken after postcss package removal.
The removal of
cssnano,postcss-import, andpostcss-preset-envhas created a broken state:
- postcss.config.js still exists but references missing packages (cssnano, postcss-import, postcss-preset-env) that are no longer in dependencies.
- Missing autoprefixer dependency: rsbuild.config.ts specifies
autoprefixer: {}in postcss options, but theautoprefixerpackage is not in rootpackage.jsondevDependencies.- Unclear configuration:
postcss-loaderis present in root devDependencies, but it's unclear whether postcss.config.js is being used or if the inline rsbuild config is sufficient.Either remove the obsolete postcss.config.js and add the missing
autoprefixerpackage, or restore the removed postcss packages if they're needed by the build.🤖 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/app/package.json` at line 3, The postcss configuration is broken due to inconsistent package removals. In the root package.json devDependencies, add the missing `autoprefixer` package that is referenced in rsbuild.config.ts postcss options, and then either remove the obsolete postcss.config.js file entirely (if the inline rsbuild configuration is sufficient) or update postcss.config.js to remove all references to the missing packages cssnano, postcss-import, and postcss-preset-env. Ensure that the postcss configuration sources are aligned - either rely solely on the rsbuild.config.ts inline configuration or restore the removed packages to support postcss.config.js.
Comment on lines
+1333
to
+1342
| settle(); | ||
|
|
||
| try { | ||
| await applyRepresentation(); | ||
| } catch (error) { | ||
| console.warn( | ||
| `Failed to apply segmentation representation for "${segmentationId}":`, | ||
| error | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
settle() resolves the outer promise before applyRepresentation() finishes
settle() calls resolve() on line 1333, which immediately unblocks the await in _addOverlayRepresentations. applyRepresentation() (line 1336) is then awaited concurrently in the subscriber callback — the outer caller has already moved on. This creates an asymmetry: the synchronous fast path (getSegmentation already truthy at line 1306) returns applyRepresentation() directly and the caller properly awaits it, while this async path resolves before addSegmentationRepresentation has run. Any work _addOverlayRepresentations defers until after its loop relies on the representation being attached, but for the first load it won't be.
Move settle() to after the await applyRepresentation() call, or use a try/finally to guarantee resolution regardless of outcome.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
Line: 1333-1342
Comment:
**`settle()` resolves the outer promise before `applyRepresentation()` finishes**
`settle()` calls `resolve()` on line 1333, which immediately unblocks the `await` in `_addOverlayRepresentations`. `applyRepresentation()` (line 1336) is then awaited concurrently in the subscriber callback — the outer caller has already moved on. This creates an asymmetry: the synchronous fast path (`getSegmentation` already truthy at line 1306) returns `applyRepresentation()` directly and the caller properly awaits it, while this async path resolves before `addSegmentationRepresentation` has run. Any work `_addOverlayRepresentations` defers until after its loop relies on the representation being attached, but for the first load it won't be.
Move `settle()` to after the `await applyRepresentation()` call, or use a try/finally to guarantee resolution regardless of outcome.
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
5 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
This PR adds integration with the cs3d adapter changes that provide an image loader instead of providing a full binary segmentation object. That results in significantly faster loading and saving of segmentations.
cs3d_ref: fix/use-imageLoader-for-seg
Changes & Results
Testing
[] Save → restore: Create/edit a segmentation, save it, reload, and confirm it restores correctly.
[] Download: Download a saved segmentation and confirm the output is an RLE-compressed labelmap.
[] Backward compat: Loading existing/older segmentations still works.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Improvements
Updates
?frame=lookups.dcmjs0.52.0.Greptile Summary
This PR refactors DICOM SEG loading to use a per-frame image-loader approach (via
createFromDicomSegImageId) instead of downloading the full binary buffer, enabling support for any transfer syntax including RLE-compressed labelmaps. It also fixes the ordering ofaddOrUpdateSegmentationvsSEGMENTATION_LOADING_COMPLETEbroadcast, adds a wait-and-timeout mechanism in the viewport service so the representation is applied only after the segmentation is in Cornerstone state, and upgradesdcmjsto 0.52.0._loadSegmentsnow resolves frame-level imageIds through the datasource and passes them to the new adapter API, removing the dependency on fetching the full DICOM buffer upfront.addOrUpdateSegmentationis now called before_broadcastEvent(SEGMENTATION_LOADING_COMPLETE), ensuring Cornerstone state is populated before any subscriber attempts to attach a representation.dicomWriter.tscentralises DICOM Part-10 writing withfragmentMultiframe: false(one fragment per frame), runtime property non-enumeration helpers, and agetDatasetTransferSyntaxUIDlookup; covered by new unit tests.Confidence Score: 4/5
The critical race condition that previously silently prevented representations from being attached on initial SEG load is now fixed by reordering addOrUpdateSegmentation before the broadcast; an async ordering subtlety in the viewport service subscriber (settle() resolving the outer promise before applyRepresentation() completes) flagged in a prior round remains.
The core ordering bug is fixed and the indefinitely-hanging promise path is addressed with a timeout. The remaining async sequencing issue means _addOverlayRepresentations can return while the first SEG's representation is still being attached, which could affect downstream viewport setup code that assumes overlays are fully applied.
extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts — async subscriber callback and settle() ordering; extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts — duplicate constant and dead headers argument.
Important Files Changed
Comments Outside Diff (1)
extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts, line 216-221 (link)The
SEGMENTATION_LOAD_PROGRESSlistener is registered every time_loadSegmentsruns but never cleaned up. Loading N SEG display sets accumulates N listeners, so each subsequent progress tick fires N broadcasts ofSEGMENT_LOADING_COMPLETE, causing incorrect progress UI and a growing memory leak.Then add
eventTarget.removeEventListener(Enums.Events.SEGMENTATION_LOAD_PROGRESS, onProgress);in afinallyblock (or right after theawait).Prompt To Fix With AI
extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts, line 591-615 (link)SEGMENTATION_LOADING_COMPLETEfired before segmentation enters Cornerstone state_broadcastEvent(EVENTS.SEGMENTATION_LOADING_COMPLETE, ...)is called at line 591, butaddOrUpdateSegmentation(seg)— which callscstSegmentation.addSegmentations— isn't called until line 615. The new subscriber inCornerstoneViewportServicefiresapplyRepresentation()synchronously inside the event callback, which immediately callsaddSegmentationRepresentation. That function's new guard (getSegmentation(segmentationId)→cstSegmentation.state.getSegmentation(...)) finds nothing and returns early with a warning. Once the broadcast returns, step 615 finally adds the segmentation to Cornerstone state — but the representation callback has already run and bailed out silently. No other mechanism re-triggersaddSegmentationRepresentationafteraddOrUpdateSegmentation, so the SEG overlay never appears in the viewport on fresh load.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (22): Last reviewed commit: "Merge branch 'master' into feat/load-seg..." | Re-trigger Greptile