feat: Add display set split store logic to metadata#2738
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
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:
📝 WalkthroughWalkthroughAdds a framework-agnostic display-set subsystem to ChangesDisplay set metadata, viewport APIs, and example migration
CI / Example Runner
Sequence Diagram(s)sequenceDiagram
participant App
participant splitImageIdsBySplitRules
participant createDisplaySetFromGroup
participant registerDisplaySetMetadata
participant Viewport
participant genericViewportDisplaySetMetadataProvider
App->>splitImageIdsBySplitRules: imageIds + SplitContext
splitImageIdsBySplitRules->>splitImageIdsBySplitRules: resolveInstances → groupInstancesBySplitRules
splitImageIdsBySplitRules-->>App: InstanceGroup[]
App->>createDisplaySetFromGroup: group + options
createDisplaySetFromGroup-->>App: IDisplaySet
App->>registerDisplaySetMetadata: imageIds + displaySet
registerDisplaySetMetadata->>genericViewportDisplaySetMetadataProvider: addTyped per imageId
App->>genericViewportDisplaySetMetadataProvider: add(displaySetId, {imageIds, kind})
App->>Viewport: setDisplaySets({displaySetId})
Viewport->>genericViewportDisplaySetMetadataProvider: getGenericViewportXxxDisplaySet(displaySetId)
genericViewportDisplaySetMetadataProvider-->>Viewport: imageIds / webClient / sourceDataId
Viewport->>Viewport: clearDisplaySets → raw setter → mountDisplaySets
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
wayfarer3130
force-pushed
the
fix/dwi-wrong-wl-split
branch
from
0c48d0c to
5cfd79a
Compare
wayfarer3130
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
packages/core/examples/displaySets/index.ts (1)
225-231: ⚡ Quick winValidate viewport type before casting.
Lines 227 and 230 cast
viewportto specific types without runtime validation. IfenableElementsilently failed or returned an unexpected viewport type, the casts would succeed but method calls could throw at runtime.♻️ Suggested defensive check
if (row.hint === 'volume3d') { // 3D needs a preset to be visible; setDisplaySets already loaded the volume. - (viewport as Types.IVolumeViewport).setProperties({ preset: 'CT-Bone' }); + if ('setProperties' in viewport) { + (viewport as Types.IVolumeViewport).setProperties({ preset: 'CT-Bone' }); + } } if (row.hint === 'video') { - (viewport as Types.IVideoViewport).play(); + if ('play' in viewport) { + (viewport as Types.IVideoViewport).play(); + } }🤖 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 `@packages/core/examples/displaySets/index.ts` around lines 225 - 231, The code casts viewport to Types.IVolumeViewport and Types.IVideoViewport and calls setProperties/play without runtime checks, which can throw if enableElement returned a different viewport; update the branches handling row.hint === 'volume3d' and 'video' to first validate the viewport type (e.g., check for presence of expected methods/properties such as setProperties for volume and play for video or use a type guard) before casting and calling setProperties or play on the viewport instance to safely bail or log an error if the viewport is not the expected type.packages/metadata/src/displayset/ImageStackDisplaySet.ts (1)
13-37: 💤 Low valueConsider refactoring to eliminate code duplication.
The loops in
collectUnderlyingImageIds(lines 15-19) andcollectImageIds(lines 28-32) perform identical operations: iterating instances and collectinginstance.imageIdvalues.collectImageIdscould delegate tocollectUnderlyingImageIdsand only handle the fallback logic.♻️ Proposed refactor
function collectImageIds( instances: NaturalizedInstance[], underlyingImageIds: string[] ): string[] { - const imageIds: string[] = []; - for (const instance of instances) { - if (instance.imageId) { - imageIds.push(instance.imageId); - } - } + const imageIds = collectUnderlyingImageIds(instances); if (imageIds.length === 0) { return [...underlyingImageIds]; } return imageIds; }🤖 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 `@packages/metadata/src/displayset/ImageStackDisplaySet.ts` around lines 13 - 37, Duplicate loops collectUnderlyingImageIds and collectImageIds both iterate instances to gather instance.imageId; refactor by making collectImageIds call collectUnderlyingImageIds(instances) to get imageIds, then if that result is empty return [...underlyingImageIds] else return the collected ids; update/remove the duplicated loop in collectImageIds and keep collectUnderlyingImageIds as the single source of truth (refer to functions collectUnderlyingImageIds and collectImageIds).packages/metadata/src/displayset/displaySetProvider.ts (1)
5-11: ⚡ Quick winAdd type annotations to provider function parameters.
The
displaySetAddProviderfunction has untyped parameters (next,_data,options). Adding type annotations would improve type safety and IDE autocomplete support.🔧 Suggested typing improvement
-function displaySetAddProvider(next, query: string, _data, options) { +function displaySetAddProvider( + next: (...args: unknown[]) => unknown, + query: string, + _data: unknown, + options?: { displaySet?: unknown } +) { const displaySet = options?.displaySet; if (displaySet) { return displaySet; } return next(query, _data, options); }🤖 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 `@packages/metadata/src/displayset/displaySetProvider.ts` around lines 5 - 11, The provider function displaySetAddProvider should have explicit TypeScript types added for its parameters: annotate next as a function type (e.g. (query: string, data?: unknown, options?: DisplayOptions) => DisplaySetType) so callers and returns are typed, type _data as unknown or a more specific payload type used by callers, and type options as an interface with an optional displaySet?: DisplaySetType (or use the existing DisplaySet/DisplayOptions types in the codebase). Update the function signature of displaySetAddProvider to use these types and import or declare DisplaySetType/DisplayOptions as needed so tooling and autocomplete pick up the shapes of next, _data, and options.packages/metadata/package.json (1)
1-1: Pipeline failures are unrelated CI configuration issues.The reported pipeline failures concern a pnpm version conflict between
packageManager(pnpm@11.5.2) and GitHub Action configuration (11.4.0). This is a CI/workflow configuration issue outside the scope of the code changes in this file. ThepackageManagerfield is not modified in this diff and likely resides in the repository rootpackage.jsonor workflow files.🤖 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 `@packages/metadata/package.json` at line 1, CI failures are caused by a pnpm version mismatch between the repository/packageManager field and the GitHub Actions workflow; do not change packages/metadata/package.json in this PR—either update the workflow action that installs pnpm to use pnpm@11.5.2 (matching the packageManager field) or update the root package.json packageManager field to the version used by the workflow so they align; look for the packageManager field in the root package.json and the pnpm install step in your GitHub Actions workflow to make the change.packages/metadata/src/displayset/buildSeriesInfo.ts (1)
16-19: ⚡ Quick winValidate
NumberOfFramesbefore numeric conversion.If
NumberOfFramescontains a malformed string,Number()returnsNaN, silently corrupting thenumberOfFramestotal. Consider usingparseInt()with validation or explicitly checking for valid numeric values.🛡️ Proposed validation
if (instance.NumberOfFrames) { - numberOfFrames += Number(instance.NumberOfFrames); + const frames = parseInt(String(instance.NumberOfFrames), 10); + if (!isNaN(frames)) { + numberOfFrames += frames; + } } else if (instance.Rows) {🤖 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 `@packages/metadata/src/displayset/buildSeriesInfo.ts` around lines 16 - 19, The code adds instance.NumberOfFrames to numberOfFrames without validating it, so malformed strings yield NaN; change the branch in buildSeriesInfo (the logic that reads instance.NumberOfFrames) to validate and parse the value first (e.g., use parseInt or Number and then test with Number.isFinite / Number.isInteger or isNaN) and only add the parsed numeric value to numberOfFrames when valid, otherwise treat it as zero or log/skip the invalid value; reference the variables instance.NumberOfFrames and numberOfFrames and the function/build block in buildSeriesInfo.ts when making this change.
🤖 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 `@package.json`:
- Line 183: package.json declares "packageManager": "pnpm@11.5.2" but CI
workflows pin pnpm to 11.4.0 causing ERR_PNPM_BAD_PM_VERSION; update the pnpm
version pin in the referenced GitHub Actions workflows
(.github/workflows/validate-codemod-registry.yml, test.yml, playwright.yml,
format-check.yml, docusaurus-build.yml, build-docs.yml) and the PNPM_VERSION env
in ohif-downstream.yml to 11.5.2 (or remove explicit pnpm version pins so the
runner respects packageManager), or alternatively change the packageManager
value back to "pnpm@11.4.0" to keep CI consistent. Ensure you update all
occurrences of pnpm@11.4.0 and PNPM_VERSION to 11.5.2 (or remove pins) so pnpm
install --frozen-lockfile no longer errors.
In `@packages/core/examples/displaySets/index.ts`:
- Around line 180-193: The loop that builds the details rows uses line.innerHTML
= `<strong>${label}:</strong> ${value}` which injects untrusted DICOM metadata
into HTML; change this to create elements and set textContent to avoid HTML
parsing: for each (label, value) create a container div, create a <strong>
element and set its textContent to `${label}:`, append it to the container, then
append a text node or set the container.textContent (or append a text node for
value) using the string `value` (not innerHTML); also replace
statusLine.innerText with statusLine.textContent for consistency. Update the
code paths that reference row.detailsElement, the loop over lines, and the
statusLine creation to use textContent-safe methods.
In `@packages/core/src/RenderingEngine/BaseVolumeViewport.ts`:
- Around line 1511-1513: The code casts dataSet.volumeId to string without
checking type before calling resolveViewportVolumeId, which can hide non-string
values; update the logic in BaseVolumeViewport (around the
resolveViewportVolumeId call) to validate dataSet.volumeId is a string (e.g.,
typeof dataSet.volumeId === 'string' && dataSet.volumeId.length > 0) before
using it, otherwise fall back to entry.displaySetId; ensure the computed
volumeId passed into resolveViewportVolumeId is guaranteed to be a string or
null/undefined so downstream code can rely on its type.
- Around line 1515-1520: The volume is being created with
createAndCacheVolume(...) but volume.load() is not awaited, causing potential
race conditions when the viewport mounts; change the flow to await the load
(e.g., await volume.load()) before proceeding to any mounting or calling
setVolumes, and wrap the await in a try/catch to handle and log load errors so
failures don’t leave the cache in an inconsistent state (refer to
cache.getVolume, createAndCacheVolume, and volume.load).
In `@packages/core/src/RenderingEngine/StackViewport.ts`:
- Around line 1998-2004: The code uses dataSet.initialImageIdIndex directly and
can pass invalid values (NaN, negative, out-of-range, non-integer) into
setStack; before calling setStack, validate and clamp initialImageIdIndex: read
dataSet.initialImageIdIndex, coerce to a finite integer (e.g., via
Number.isFinite and Math.floor), default to 0 on invalid input, then clamp it to
the range 0 .. Math.max(0, dataSet.imageIds.length - 1); pass that sanitized
value to setStack instead of the raw value so setStack always receives a valid
index.
In `@packages/metadata/src/displayset/displayset.test.ts`:
- Around line 56-57: The test assertions are out of sync with the `volume3d`
rule in defaultDisplaySetSplitRules.ts which sets viewportTypes: ['volume',
'volume3d', 'stack'] (so the first element is 'volume'); update the test in
displayset.test.ts to assert expect(displaySet.viewportTypes[0]).toBe('volume')
and adjust expect(displaySet.preferredViewportType) to match the actual
preferredViewportType produced by the rule (or, if the intent was to prefer
'volume3d', change the rule in defaultDisplaySetSplitRules.ts where the
'volume3d' rule is defined so its viewportTypes order and preferredViewportType
reflect that intent).
In `@packages/metadata/src/displayset/ImageStackDisplaySet.ts`:
- Around line 85-102: ImageStackDisplaySet.fromImageIds currently drops
unresolved imageIds silently; modify it to surface missing-ID handling by either
routing through the existing resolveInstances utility or by adding
options.skipMissing?: boolean and options.onMissing?: (imageId: string) => void
to the fromImageIds signature, then use those options to call onMissing for each
undefined instance and throw or skip based on skipMissing before calling
ImageStackDisplaySet.fromInstances; reference the method name
ImageStackDisplaySet.fromImageIds, the helper resolveInstances, and the
downstream ImageStackDisplaySet.fromInstances so reviewers can locate the
change.
In `@packages/metadata/src/displayset/resolveInstances.ts`:
- Line 4: Update the doc comment for the skipMissing parameter in
resolveInstances (or the top-level comment in resolveInstances.ts) to accurately
state that when skipMissing is true missing ids are skipped silently, and when
skipMissing is false the function throws an error (with optional warning
behavior removed), rather than "omitted with optional warn"; reference the
skipMissing parameter name and the resolveInstances function so readers can find
the behavior.
In `@packages/tools/examples/videoNavigation/index.ts`:
- Around line 259-261: Ensure you check that displaySet.instances exists and has
at least one element before using displaySet.instances[0].imageId; update the
code around the call to viewport.setDisplaySets to validate (e.g., if
(!displaySet || !Array.isArray(displaySet.instances) ||
displaySet.instances.length === 0) then handle the empty case or skip calling
viewport.setDisplaySets) so you never access instances[0] on an empty array and
include a clear fallback or error handling path.
In `@packages/tools/examples/videoSegmentation/index.ts`:
- Around line 193-204: The code assumes displaySet.instances has elements when
deriving videoId; update the block after createDisplaySets to validate that
displaySet.instances is an array with length > 0 (and that instances[0].imageId
exists) before accessing instances[0].imageId; if the check fails, throw a clear
Error (e.g., "No instances found in display set") or handle the empty case, and
only assign videoId once the guard passes—apply this change around the
displaySets/displaySet/videoId usage to prevent runtime crashes.
In `@packages/tools/examples/videoSplineROITools/index.ts`:
- Around line 286-290: The code assumes displaySet.instances has at least one
element before accessing instances[0]; update the logic around the displaySet
extraction to validate that displaySet.instances is an array with length > 0 (or
at least one element) before reading displaySet.instances[0].imageId and
assigning videoId; if empty, throw a clear Error (e.g., "No instances found in
display set") or handle the empty-case appropriately so videoId is not accessed
from an empty array. Ensure references to displaySet.instances and the videoId
assignment are updated accordingly.
In `@packages/tools/examples/videoTools/index.ts`:
- Around line 179-183: The code assumes displaySet.instances has elements before
accessing instances[0]; update the block that sets videoId to first verify
displaySet.instances is an array with length > 0 (e.g., check
Array.isArray(displaySet.instances) && displaySet.instances.length > 0) and
throw a clear Error if empty, then safely read displaySet.instances[0].imageId
to set videoId; reference the existing variables displaySets, displaySet, and
videoId when making this change.
---
Nitpick comments:
In `@packages/core/examples/displaySets/index.ts`:
- Around line 225-231: The code casts viewport to Types.IVolumeViewport and
Types.IVideoViewport and calls setProperties/play without runtime checks, which
can throw if enableElement returned a different viewport; update the branches
handling row.hint === 'volume3d' and 'video' to first validate the viewport type
(e.g., check for presence of expected methods/properties such as setProperties
for volume and play for video or use a type guard) before casting and calling
setProperties or play on the viewport instance to safely bail or log an error if
the viewport is not the expected type.
In `@packages/metadata/package.json`:
- Line 1: CI failures are caused by a pnpm version mismatch between the
repository/packageManager field and the GitHub Actions workflow; do not change
packages/metadata/package.json in this PR—either update the workflow action that
installs pnpm to use pnpm@11.5.2 (matching the packageManager field) or update
the root package.json packageManager field to the version used by the workflow
so they align; look for the packageManager field in the root package.json and
the pnpm install step in your GitHub Actions workflow to make the change.
In `@packages/metadata/src/displayset/buildSeriesInfo.ts`:
- Around line 16-19: The code adds instance.NumberOfFrames to numberOfFrames
without validating it, so malformed strings yield NaN; change the branch in
buildSeriesInfo (the logic that reads instance.NumberOfFrames) to validate and
parse the value first (e.g., use parseInt or Number and then test with
Number.isFinite / Number.isInteger or isNaN) and only add the parsed numeric
value to numberOfFrames when valid, otherwise treat it as zero or log/skip the
invalid value; reference the variables instance.NumberOfFrames and
numberOfFrames and the function/build block in buildSeriesInfo.ts when making
this change.
In `@packages/metadata/src/displayset/displaySetProvider.ts`:
- Around line 5-11: The provider function displaySetAddProvider should have
explicit TypeScript types added for its parameters: annotate next as a function
type (e.g. (query: string, data?: unknown, options?: DisplayOptions) =>
DisplaySetType) so callers and returns are typed, type _data as unknown or a
more specific payload type used by callers, and type options as an interface
with an optional displaySet?: DisplaySetType (or use the existing
DisplaySet/DisplayOptions types in the codebase). Update the function signature
of displaySetAddProvider to use these types and import or declare
DisplaySetType/DisplayOptions as needed so tooling and autocomplete pick up the
shapes of next, _data, and options.
In `@packages/metadata/src/displayset/ImageStackDisplaySet.ts`:
- Around line 13-37: Duplicate loops collectUnderlyingImageIds and
collectImageIds both iterate instances to gather instance.imageId; refactor by
making collectImageIds call collectUnderlyingImageIds(instances) to get
imageIds, then if that result is empty return [...underlyingImageIds] else
return the collected ids; update/remove the duplicated loop in collectImageIds
and keep collectUnderlyingImageIds as the single source of truth (refer to
functions collectUnderlyingImageIds and collectImageIds).
🪄 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: d98b3480-8b3c-4da7-a312-0acc2575fc1f
📒 Files selected for processing (52)
package.jsonpackages/core/examples/displaySets/index.tspackages/core/examples/dynamicVolume/index.tspackages/core/examples/video/index.tspackages/core/src/RenderingEngine/BaseVolumeViewport.tspackages/core/src/RenderingEngine/ECGViewport.tspackages/core/src/RenderingEngine/StackViewport.tspackages/core/src/RenderingEngine/VideoViewport.tspackages/core/src/RenderingEngine/Viewport.tspackages/core/src/RenderingEngine/WSIViewport.tspackages/docs/docs/concepts/cornerstone-metadata/index.mdpackages/metadata/jest.config.jspackages/metadata/package.jsonpackages/metadata/src/displayset/BaseDisplaySet.tspackages/metadata/src/displayset/IDisplaySet.tspackages/metadata/src/displayset/ImageStackDisplaySet.tspackages/metadata/src/displayset/buildSeriesInfo.tspackages/metadata/src/displayset/createDisplaySetFromGroup.tspackages/metadata/src/displayset/defaultDisplaySetSplitRules.tspackages/metadata/src/displayset/displaySetProvider.tspackages/metadata/src/displayset/displayset.test.tspackages/metadata/src/displayset/groupInstancesBySplitRules.tspackages/metadata/src/displayset/index.tspackages/metadata/src/displayset/isEcgInstance.tspackages/metadata/src/displayset/isImageSopClass.tspackages/metadata/src/displayset/isVideoInstance.tspackages/metadata/src/displayset/isWsiInstance.tspackages/metadata/src/displayset/registerDisplaySetMetadata.tspackages/metadata/src/displayset/resolveInstances.tspackages/metadata/src/displayset/splitSeriesInstanceGroupsFromImageIds.tspackages/metadata/src/displayset/types.tspackages/metadata/src/displayset/viewportTypes.tspackages/metadata/src/enums/MetadataModules.tspackages/metadata/src/index.tspackages/metadata/src/registerDefaultProviders.tspackages/metadata/src/types/MetadataModuleTypes.tspackages/metadata/src/types/index.tspackages/metadata/src/utilities/index.tspackages/tools/examples/dynamicCINETool/index.tspackages/tools/examples/generateImageFromTimeData/index.tspackages/tools/examples/localAdvanced/index.tspackages/tools/examples/videoColor/index.tspackages/tools/examples/videoContourSegmentation/index.tspackages/tools/examples/videoGroup/index.tspackages/tools/examples/videoNavigation/index.tspackages/tools/examples/videoRange/index.tspackages/tools/examples/videoSegmentation/index.tspackages/tools/examples/videoSplineROITools/index.tspackages/tools/examples/videoTools/index.tsutils/ExampleRunner/example-runner-cli.jsutils/demo/helpers/index.jsutils/demo/helpers/splitDisplaySetsFromImageIds.ts
…ack and volume viewports (#2717) * fix: closed freehand stat value mismatch * fix: review comments * chore: renamed function * docs: fix MDX rendering issue in snapIndexBounds JSDoc * Fix view issue on stack so that the issue is testable --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
This was referenced Jun 27, 2026
Closed
- isVideoInstance: reuse shared videoUIDs (no MPEG2 misclassification) - buildSeriesInfo + default split rules: guard empty instance lists - numImageFrames: coerce NumberOfFrames with Number() - MetadataModuleType.displaySetModule: type as full IDisplaySet - createDisplaySetFromGroup: denylist reserved data keys - Viewport: extract shared mountDisplaySets template for the 5 overrides - re-export isWsiInstance; add JSDoc to classifiers/viewport-type helpers - demo toBaseImageId: normalize ?frame=N as well as &frame=N - tests for the above
Add the split -> create -> consume pipeline, driving a viewport via setDisplaySets, caching display sets in the metadata layer, the split-rule model (with the DWI mixedDimensionalityBValue worked example), and the reusable instance classifiers.
sedghi
changed the title
- GenericViewport: add getDisplaySets() to ViewportController + base impl (derived from live bindings), giving parity with the legacy viewport. - createDisplaySetFromGroup: fold splitNumber into the default displaySetInstanceUID so a split series (DWI) yields unique ids; the viewport displaySetId == the display set instance UID. - demo splitter passes splitNumber; displaySets example keys the registry on displaySetInstanceUID to exercise the convention. - legacy mountDisplaySets records only the mounted source (honest getDisplaySets); documents overlays as a non-breaking future addition. - rename genericViewportDataSet* -> genericViewportDisplaySet* across the registry/access layer, types, and call sites (beta, no alias).
Collapse the displaySetId / displaySetInstanceUID duality onto a single name, displaySetId: - metadata: rename IDisplaySet.displaySetInstanceUID -> displaySetId (and CreateDisplaySetFromGroup option, reserved keys, tests, docs). - generic viewport: rename the DataId type -> DisplaySetId; align the registry/access param names (dataId -> displaySetId). - keep dataId in the public, persisted ViewReference / ViewportDataReference (a generic data-ref pointer) and in loadECGWaveform (an imageId), per the identifier-surface-only scope.
- group buckets are now namespaced by rule and JSON-encoded, so different rules can't collide on a shared key and separator/undefined aliasing is gone (issues 1, 6). - groups are returned in deterministic (split-key sorted) order, making split identity / displaySetId suffixes stable across input ordering (3). - groupInstancesBySplitRules/splitImageIdsBySplitRules accept onUnmatched(d) so instances matching no rule are observable, not silently dropped (5). - updateSeriesInfo typed () => void (return was always ignored) (4). - default rules: singleImageModality split key is now series-scoped (7); comments document the instances[0] homogeneity assumption (2), the multiFrame SliceLocation guard (8); removed the redundant viewportTypes-only customAttributes now that rule-level viewportTypes is applied (9). - tests for rule namespacing, deterministic order, and onUnmatched.
The metadata page had grown to be mostly display-set content. Move it to a dedicated concepts/cornerstone-metadata/display-sets page (pipeline, driving a viewport, caching, split-rule model + DWI example, IDisplaySet model), leave a short pointer on the metadata index, and add it to the sidebar. Also folds in the split-engine guarantees (rule-namespaced keys, deterministic order, onUnmatched, updateSeriesInfo mutation).
Complete the in-progress rename of the SplitRule fields so the engine and the rules line up again: - ruleSelector -> matches (per-instance predicate) - splitKey (rule field) -> groupBy (bucket recipe) - keep updateSeriesInfo; revert the stray augmentSeriesInfo so the series-level hook actually fires The rename had been applied to types.ts and groupInstancesBySplitRules.ts but not to defaultDisplaySetSplitRules.ts/tests, so every rule fell back to a catch-all grouped by SeriesInstanceUID and the series flags were never set - silently disabling the DWI mixed-b-value split. Updates the rules, tests, and docs to the new names.
…ies hook
Series-level facts used to be contributed by a rule's `updateSeriesInfo`
hook, which mutated a shared `seriesInfo` object that `matches`/`groupBy`
then read. Replace that with a pure, rule-scoped `series` hook:
- `series?: ({ instances }) => SeriesFacts` runs once per rule per split and
*returns* that rule's derived facts (no shared mutation).
- `matches`/`groupBy` receive a `RuleContext` ({ series }) carrying only that
rule's own facts.
- `buildSeriesInfo` is now a standalone series-statistics helper, decoupled
from rules; `SeriesInfo` no longer carries rule flags.
Most rules need only matches + groupBy; series is the opt-in for whole-series
facts (multi-frame, volumetric, mixed-b-value DWI). Adds tests covering the
DWI mixed/non-mixed split and updates the display-sets docs.
…tadataProvider Three core jest suites still imported/mocked the pre-rename `genericViewportDataSetMetadataProvider` module and its `VIEWPORT_V2_DATA_SET` constant, so they failed to load after the DataSet->DisplaySet rename. Update the module path and constant to the current `genericViewportDisplaySetMetadataProvider` / `VIEWPORT_V2_DISPLAY_SET` names.
Library: - defaultDisplaySetSplitRules: mixed-b-value DWI rule now prefers volume (MPR) over stack, matching normal MR series; it runs before volume3d so stack-first was regressing the defined-b-value subgroup. - isVideoInstance: check all advertised transfer syntaxes, not just the first array entry, so a video syntax in a later slot is not missed. - BaseDisplaySet: clone viewportTypes and instances so a caller mutating them after construction cannot drift the display set / preferredViewportType. - ImageStackDisplaySet.fromImageIds: preserve caller frame-level imageIds instead of collapsing them to underlying-instance ids. - BaseVolumeViewport: attach a rejection handler to the fire-and-forget streaming volume.load(). Examples/docs: - video examples: select the video display set by preferredViewportType instead of assuming displaySets[0], which is fragile once a series splits. - displaySets example: build metadata rows with DOM nodes/textContent instead of innerHTML (avoids injecting DICOM-derived markup). - docs: import metaData in the getNaturalizedInstance snippet; align the mixed-b-value rule example with the volume-first viewport order.
The CodeRabbit nitpick assumed volume.load() returns a promise, but it returns void, so .catch was a type error that broke the ESM build (TS2339). Restore the original fire-and-forget volume.load() call.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/core/src/RenderingEngine/GenericViewport/index.ts (1)
43-64: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider keeping a deprecated
DataIdexport for one release.Dropping the public alias here turns a naming cleanup into a source-breaking change for downstream TypeScript consumers that import
DataId. If this PR is not intentionally shipping behind a major-version migration, re-exportingtype DataId = DisplaySetIdas a shim would make the rename much safer.🤖 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 `@packages/core/src/RenderingEngine/GenericViewport/index.ts` around lines 43 - 64, Keep the public DataId type available as a temporary compatibility shim in GenericViewport/index.ts; the current export list removes a downstream-facing alias and can break TypeScript consumers. Re-export DataId alongside DisplaySetId (for example by aliasing DataId to DisplaySetId) and mark it deprecated for one release so existing imports continue to compile while the rename is rolled out.
🤖 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 `@packages/core/src/RenderingEngine/GenericViewport/GenericViewport.ts`:
- Around line 140-153: The source promotion logic in GenericViewport is updating
roles without updating ordering, so the newly added source can stay behind
earlier bindings. Adjust the addDisplaySet / source-selection path so when a
later display set becomes role 'source' it is moved to the front of the bindings
order before getDisplaySets() and getCurrentBinding() read from the Map. Keep
the existing role flips for other entries, but ensure the promoted source is
inserted/reinserted as the first binding so implicit updates target the correct
display set.
In `@packages/core/src/RenderingEngine/WSIViewport.ts`:
- Around line 643-655: `WSIViewport.setDisplaySets` is indirectly re-entering
`WSIViewportLegacyAdapter` through `this.setWSI(...)`, causing an infinite
recursion path between `setWSI`, `setDataIds`, and `setDisplaySets`. Update the
WSI load path in `setDisplaySets` to call the non-virtual internal
implementation directly (or otherwise bypass the legacy adapter override) so the
legacy adapter cannot dispatch back into `setDisplaySets`. Keep the existing
validation in `setDisplaySets`, but ensure the final load uses the base
WSI-loading method rather than the polymorphic `setWSI` entrypoint.
In `@packages/core/src/utilities/genericViewportDisplaySetMetadataProvider.ts`:
- Around line 5-27: The cache in genericViewportDisplaySetMetadataProvider uses
a plain object state, which can mishandle special keys like __proto__ and
constructor. Replace state with a Map<string, unknown> (or an
Object.create(null) store) and update add, get, remove, and clear to use the Map
API while keeping the VIEWPORT_V2_DISPLAY_SET check in get.
In `@packages/metadata/src/displayset/BaseDisplaySet.ts`:
- Around line 33-34: The BaseDisplaySet constructor is deduplicating both image
id arrays, which changes the caller-supplied cardinality and ordering that
IDisplaySet relies on. Update BaseDisplaySet to preserve options.imageIds and
options.underlyingImageIds exactly as provided instead of wrapping them in Set,
so repeated ids and frame-to-instance mapping remain intact.
In `@packages/metadata/src/displayset/defaultDisplaySetSplitRules.ts`:
- Around line 64-72: The series-wide split rules are matching non-image SOP
instances because the matches callbacks only check the cached series fact from
series. Update the relevant matches logic in defaultDisplaySetSplitRules so
that, in addition to checking series.isMultiFrame, it also verifies the current
instance is an image before claiming it, while leaving the series-level gate in
place. Apply this same adjustment to the other affected rule blocks in the same
file so non-image instances continue to fall through to onUnmatchedInstance.
In `@packages/metadata/src/displayset/groupInstancesBySplitRules.ts`:
- Around line 76-80: The bucket key in groupInstancesBySplitRules is too broad
because buildSplitKey is using SplitRule.id alone when it exists, which can
collide across different rules and merge instances into the wrong InstanceGroup.
Update the discriminator logic in groupInstancesBySplitRules so it always
includes the rule index together with splitRule.id (or otherwise namespaces by
both id and ruleIndex), and ensure the matchedRule recorded for each bucket
still comes from the correct split rule.
---
Nitpick comments:
In `@packages/core/src/RenderingEngine/GenericViewport/index.ts`:
- Around line 43-64: Keep the public DataId type available as a temporary
compatibility shim in GenericViewport/index.ts; the current export list removes
a downstream-facing alias and can break TypeScript consumers. Re-export DataId
alongside DisplaySetId (for example by aliasing DataId to DisplaySetId) and mark
it deprecated for one release so existing imports continue to compile while the
rename is rolled out.
🪄 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: 9bb21406-0f85-46e1-9d13-cee2b4f8980e
📒 Files selected for processing (86)
packages/core/examples/displaySets/index.tspackages/core/examples/genericEcg/index.tspackages/core/examples/genericMultiVolumeAPI/index.tspackages/core/examples/genericStackAPI/index.tspackages/core/examples/genericStackPosition/index.tspackages/core/examples/genericVideo/index.tspackages/core/examples/genericViewportScale/index.tspackages/core/examples/genericWsi/index.tspackages/core/examples/viewportProjection/index.tspackages/core/examples/wsi/index.tspackages/core/src/RenderingEngine/BaseVolumeViewport.tspackages/core/src/RenderingEngine/ECGViewport.tspackages/core/src/RenderingEngine/GenericViewport/ECG/DefaultECGDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/GenericViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarLegacyCompatibilityController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Video/DefaultVideoDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Video/VideoViewport.tspackages/core/src/RenderingEngine/GenericViewport/ViewportArchitectureTypes.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/DefaultVolume3DDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/VolumeViewport3DLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3D.tspackages/core/src/RenderingEngine/GenericViewport/WSI/DefaultWSIDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/WSI/WSIViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/genericViewportDisplaySetAccess.tspackages/core/src/RenderingEngine/GenericViewport/index.tspackages/core/src/RenderingEngine/StackViewport.tspackages/core/src/RenderingEngine/VideoViewport.tspackages/core/src/RenderingEngine/Viewport.tspackages/core/src/RenderingEngine/WSIViewport.tspackages/core/src/utilities/genericViewportDataSetMetadataProvider.tspackages/core/src/utilities/genericViewportDisplaySetMetadataProvider.tspackages/core/src/utilities/index.tspackages/core/test/PlanarLegacyCompatibilityController.jest.jspackages/core/test/planarOrchestration.jest.jspackages/core/test/planarViewportState.jest.jspackages/docs/docs/concepts/cornerstone-core/generic-viewport/api.mdpackages/docs/docs/concepts/cornerstone-core/generic-viewport/data-bindings-and-loading.mdpackages/docs/docs/concepts/cornerstone-core/generic-viewport/migration.mdpackages/docs/docs/concepts/cornerstone-metadata/display-sets.mdpackages/docs/docs/concepts/cornerstone-metadata/index.mdpackages/docs/docs/migration-guides/5x/2-generic-viewport.mdpackages/docs/sidebars.jspackages/metadata/src/displayset/BaseDisplaySet.tspackages/metadata/src/displayset/IDisplaySet.tspackages/metadata/src/displayset/ImageStackDisplaySet.tspackages/metadata/src/displayset/buildSeriesInfo.tspackages/metadata/src/displayset/createDisplaySetFromGroup.tspackages/metadata/src/displayset/defaultDisplaySetSplitRules.tspackages/metadata/src/displayset/displayset.test.tspackages/metadata/src/displayset/groupInstancesBySplitRules.tspackages/metadata/src/displayset/index.tspackages/metadata/src/displayset/isEcgInstance.tspackages/metadata/src/displayset/isImageInstance.tspackages/metadata/src/displayset/isVideoInstance.tspackages/metadata/src/displayset/isWsiInstance.tspackages/metadata/src/displayset/splitImageIdsBySplitRules.tspackages/metadata/src/displayset/types.tspackages/metadata/src/displayset/viewportTypes.tspackages/metadata/src/index.tspackages/metadata/src/types/MetadataModuleTypes.tspackages/tools/examples/genericLabelmapOverlapPlayground/index.tspackages/tools/examples/genericLabelmapRendering/index.tspackages/tools/examples/genericLabelmapSegmentationTools/index.tspackages/tools/examples/genericLabelmapSliceRendering/index.tspackages/tools/examples/genericLabelmapSliceRenderingTools/index.tspackages/tools/examples/genericStackLabelmapSegmentation/index.tspackages/tools/examples/genericStackManipulationTools/index.tspackages/tools/examples/genericVolumeAnnotationTools/index.tspackages/tools/examples/videoColor/index.tspackages/tools/examples/videoContourSegmentation/index.tspackages/tools/examples/videoGroup/index.tspackages/tools/examples/videoNavigation/index.tspackages/tools/examples/videoRange/index.tspackages/tools/examples/videoSegmentation/index.tspackages/tools/examples/videoSplineROITools/index.tspackages/tools/examples/videoTools/index.tspackages/tools/examples/viewportProjectionSynchronizer/index.tspackages/tools/examples/wsiAnnotationTools/index.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan.spec.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.tspackages/tools/src/tools/displayTools/Labelmap/removeLabelmapRepresentationData.tstests/vitest-browser/genericStackApi.browser.test.tsutils/demo/helpers/splitDisplaySetsFromImageIds.ts
💤 Files with no reviewable changes (1)
- packages/core/src/utilities/genericViewportDataSetMetadataProvider.ts
✅ Files skipped from review due to trivial changes (5)
- packages/docs/docs/concepts/cornerstone-core/generic-viewport/data-bindings-and-loading.md
- packages/metadata/src/displayset/isImageInstance.ts
- packages/core/src/RenderingEngine/GenericViewport/Video/DefaultVideoDataProvider.ts
- packages/docs/docs/concepts/cornerstone-metadata/index.md
- packages/docs/docs/concepts/cornerstone-core/generic-viewport/migration.md
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/metadata/src/displayset/isEcgInstance.ts
- packages/metadata/src/displayset/isWsiInstance.ts
- packages/metadata/src/displayset/viewportTypes.ts
- packages/metadata/src/displayset/isVideoInstance.ts
- packages/core/src/RenderingEngine/StackViewport.ts
- packages/metadata/src/displayset/IDisplaySet.ts
- packages/tools/examples/videoColor/index.ts
- packages/tools/examples/videoRange/index.ts
- packages/tools/examples/videoContourSegmentation/index.ts
- packages/core/src/RenderingEngine/BaseVolumeViewport.ts
- packages/tools/examples/videoGroup/index.ts
- packages/core/src/RenderingEngine/ECGViewport.ts
- packages/tools/examples/videoNavigation/index.ts
- packages/tools/examples/videoSegmentation/index.ts
- packages/tools/examples/videoTools/index.ts
- packages/tools/examples/videoSplineROITools/index.ts
- packages/core/examples/displaySets/index.ts
- utils/demo/helpers/splitDisplaySetsFromImageIds.ts
Comment on lines
+140
to
+153
| /** | ||
| * Returns the display sets currently mounted on the viewport, in mount order | ||
| * (source binding first, then overlays). Derived from the live bindings, so | ||
| * it always reflects what is actually rendered - including overlays and any | ||
| * `removeData` calls. The per-entry `options` carry the binding `role`. | ||
| */ | ||
| getDisplaySets(): Array<{ displaySetId: DisplaySetId; options?: unknown }> { | ||
| return Array.from(this.bindings.entries()).map( | ||
| ([displaySetId, binding]) => ({ | ||
| displaySetId, | ||
| options: { role: binding.role }, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Promoting a later display set to source does not update the active/source ordering.
When addDisplaySet(..., { role: 'source' }) runs after other bindings exist, this only flips their roles to overlay; it does not move the new source to the front. Because getDisplaySets() and getCurrentBinding() both depend on Map iteration order, the old first binding can remain “current”, which breaks the source-first contract and can send implicit presentation/view-reference updates to the wrong display set.
Also applies to: 537-545
🤖 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 `@packages/core/src/RenderingEngine/GenericViewport/GenericViewport.ts` around
lines 140 - 153, The source promotion logic in GenericViewport is updating roles
without updating ordering, so the newly added source can stay behind earlier
bindings. Adjust the addDisplaySet / source-selection path so when a later
display set becomes role 'source' it is moved to the front of the bindings order
before getDisplaySets() and getCurrentBinding() read from the Map. Keep the
existing role flips for other entries, but ensure the promoted source is
inserted/reinserted as the first binding so implicit updates target the correct
display set.
- WSIViewport.setDisplaySets: call WSIViewport.prototype.setWSI non-virtually. The legacy adapter overrides setWSI -> setDataIds -> setDisplaySets, so the polymorphic call recursed infinitely on WSIViewportLegacyAdapter (critical). - genericViewportDisplaySetMetadataProvider: back the cache with a Map so reserved keys (__proto__/constructor) are stored as ordinary keys. - defaultDisplaySetSplitRules: the series-gated rules (multiFrame, mixedBValue, volume3d) now also require the instance to be an image, so non-image SOPs in a qualifying series fall through to onUnmatchedInstance instead of being pulled into an image display set. - groupInstancesBySplitRules: always fold ruleIndex into the bucket key so two rules reusing the same id can't collide into one InstanceGroup. - BaseDisplaySet: stop deduping imageIds/underlyingImageIds; preserve caller cardinality/order (underlyingImageIds is one entry per instance).
sedghi
changed the title
…osted runner
The legacy Playwright job intermittently recorded a near-zero drag (a
~0.5mm length instead of ~138mm) because a single batched mousemove - even
with Playwright's built-in { steps } - can be coalesced/dropped on the
self-hosted runner, and moving in the same burst as the mousedown can be
dropped before the tool attaches its active-drag handler.
For stepped gestures (length / window-level), settle briefly after mousedown
and deliver the move as discrete, individually-flushed mousemoves paced with a
short wait. The end point is unchanged so screenshot baselines still match.
Path-integrating tools (planar-rotate) keep the single-move path.
Confirmed locally: stackAnnotation + stackAnnotationTiled length tests pass.
sedghi
approved these changes
Jun 30, 2026
…aySets Addresses review findings on the setDisplaySets migration: - VideoViewport.setDisplaySets now threads an optional initial frame (frameNumber, or viewReference.sliceIndex matching setDataIds) through to setVideo. Restores the frame-25 start in the four migrated video examples (videoColor, videoGroup, videoSplineROITools, videoNavigation) that had regressed to frame 1. - WSIViewport.setDisplaySets injects addWSIMiniNavigationOverlayCss (honoring miniNavigationOverlay !== false), mirroring setDataIds, so the overview/position-control overlay is styled on the display-set path.
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
4 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 extracts OHIF-style display set splitting into a new, framework-agnostic
@cornerstonejs/metadata/displaysetmodule and wires the legacy viewports anddemo helpers to drive content from display sets.
The motivating bug is diffusion MR (DWI): a series that mixes 4D b-value
frames with trailing frames that have no b-value. The undefined-b-value frames
are not part of the 4D data set, so rendering them together applies the wrong
window/level. The new
mixedDimensionalityBValuesplit rule separates them intotheir own display set.
This is purely additive — it does not change any existing API. The new
setDisplaySets/getDisplaySets/clearDisplaySetson the legacyViewporthierarchy do not collide with the
GenericViewport/ViewportControllermembers (separate hierarchy), and existing
setStack/setVolumes/setVideo/setWSI/setEcgcallers are unaffected.Changes & Results
New metadata module —
packages/metadata/src/displayset/splitSeriesInstanceGroupsFromImageIds→createDisplaySetFromGroup,with
defaultDisplaySetSplitRules,buildSeriesInfo, andgroupInstancesBySplitRules.IDisplaySet(plain data-shaped object, OHIF parity) withBaseDisplaySet/ImageStackDisplaySet, plus underlying-vs-frame imageId semantics.isImageSopClass,isVideoInstance,isEcgInstance,isWsiInstance.DISPLAY_SETmetadata module + provider (registerDisplaySetProviders/registerDisplaySetMetadata) so a display set can be resolved from any image id.Viewports —
packages/core/src/RenderingEngine/ViewportgainssetDisplaySets/getDisplaySets/clearDisplaySetsand ashared
mountDisplaySets()template that owns the clear-then-record ordering.setDisplaySets, eachdelegating to
mountDisplaySets()(resolve → native setter → record).Demos — helpers and examples are rewired to drive viewports from display
sets, including the new
packages/core/examples/displaySetswalkthrough.Review round (applied in this PR) — addressed code-review feedback:
isVideoInstancenow reuses the sharedvideoUIDslist (fixes MPEG2 misclassification).buildSeriesInfoand every rule'smakeSeriesInfoare guarded against empty instance lists.numImageFramesis coerced withNumber(...)(naturalized DICOM yields strings).DISPLAY_SETreturn type is the fullIDisplaySet(no shape drift).customAttributescannot clobber the reserved data fields (imageIds,underlyingImageIds,instances,displaySetInstanceUID).mountDisplaySets().isWsiInstance; added JSDoc to the classifiers and viewport-type helpers.toBaseImageIdnormalizes?frame=Nas well as&frame=N.Testing
Unit:
Covers the DWI mixed-b-value split, video/ECG/WSI/volume routing,
customAttributesspreading, empty-instance-list safety, MPEG2 video classification, string-
NumberOfFramescoercion, and reserved-key protection.
Manual / visual: run the
displaySetsexample (one viewport per display setacross stack, volume, video, ECG, and whole-slide). Loading the DWI series shows
the undefined-b-value frames split into their own display set rather than
distorting the 4D volume's window/level.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Documentation
Tests