fix(segmentation): restrict overlay segmentation menu to same frame of reference as viewport background display set by GhadeerAlbattarni · Pull Request #5900 · OHIF/Viewers

GhadeerAlbattarni · 2026-03-16T14:01:47Z

Context

This PR fixes a bug where the segmentation overlay menu allowed users to select segmentations that did not share the same Frame of Reference (FOR) as the underlying viewport display set.

Also, in segmentation mode, the hydrate segmentation synchronizer added segmentations to all viewports regardless of their Frame of Reference, causing segmentations to appear immediately in viewports with a different FOR.

Root Cause

SEG and RTSTRUCT display sets had no FrameOfReferenceUID so the FOR filter in getEnhancedDisplaySets silently passed every SEG/RT and mark them as isOverlayable: true since the the check never gets true, the check always (undefined && ...) in extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.ts
The hydrate segmentation synchronizer did not check whether the target viewport matched the source viewport's FOR. So the segmentations were added to all viewports with different FOR.

Changes & Results

Added FrameOfReferenceUID to the DisplaySet and ensured SEG and RTSTRUCT Sop class handlers set it (from instance and/or referenced display set).
In getEnhancedDisplaySets, overlay display sets (SEG/RTSTRUCT) whose FrameOfReferenceUID does not match the viewport background’s are marked non-overlayable, so they no longer appear as selectable in the overlay segmentation menu.
The overlay menu’s optimistic state (optimisticOverlayDisplaySets) resets when the user changes the background display set, so the list stays in sync with the current viewport.
Added FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align.

After

FOR-fix.mov

Testing

Open a study with multiple frames of reference in a segmentation mode
(e.g., StudyInstanceUIDs=1.3.6.1.4.1.12201.1091.126683095609223531686845324113579088978)
Open the overlay menu on a viewport
Click on add segmentation
Verify only segmentations matching that viewport’s background FOR should be listed.
Change the viewport background display set
Verify the segmentation display set will reset
Select a segmentation
Open another Viewport overlay menu
Verify that the selected segmentation (from previous viewport) was add only to the matching viewport display set FOR

Checklist

PR

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

Code

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

Public Documentation Updates

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

Tested Environment

OS: macOS 10.15.4
Node version: v22.13.0
Browser: Chrome 83.0.4103.116

Greptile Summary

This PR fixes a bug where the segmentation overlay menu allowed selecting segmentations with a different Frame of Reference (FOR) than the viewport's background display set, and where the hydrate segmentation synchronizer propagated segmentations to viewports with mismatched FORs.

SEG/RTSTRUCT FrameOfReferenceUID propagation: Both the SEG and RTSTRUCT SOP class handlers now populate FrameOfReferenceUID on their display sets — from the instance metadata directly and from the referenced display set as a fallback. This enables the existing FOR check in getEnhancedDisplaySets (utils.ts) to properly filter non-matching overlays.
Optimistic state reset on background change: A useEffect in ViewportDataOverlayMenu resets the optimistic overlay display set list when the background display set changes, keeping the UI in sync. However, overlayDisplaySets is missing from the effect's dependency array.
Hydration synchronizer FOR guard: The createHydrateSegmentationSynchronizer now compares source and target viewport FORs before propagating segmentation representations, preventing cross-FOR hydration.
Type update: FrameOfReferenceUID added as an optional property to the core DisplaySet type.

Confidence Score: 4/5

This PR is safe to merge with one minor issue to address in the useEffect dependency array.
The core fix is well-reasoned and addresses a real bug across multiple layers (SOP class handlers, overlay menu, synchronizer). The changes are consistent and minimal. The one issue found is a missing React hook dependency that could lead to stale state in an edge case.
Pay close attention to extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/ViewportDataOverlayMenu.tsx — the useEffect dependency array is incomplete.

Important Files Changed

Filename	Overview
extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/ViewportDataOverlayMenu.tsx	Adds useEffect to reset optimistic overlay state on background change, but the `overlayDisplaySets` dependency is missing from the effect's dependency array, which could cause stale state.
extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts	Correctly adds FrameOfReferenceUID from the instance to SEG display sets, with proper fallback to the referenced display set's FOR when available.
extensions/cornerstone-dicom-rt/src/getSopClassHandlerModule.ts	Adds FrameOfReferenceUID to RTSTRUCT display sets from ReferencedFrameOfReferenceSequence and referenced display set, consistent with the SEG handler pattern.
extensions/cornerstone/src/services/SyncGroupService/createHydrateSegmentationSynchronizer.ts	Adds a FOR matching guard to prevent hydration synchronizer from propagating segmentations across viewports with different frames of reference. The logic is correct and uses safe optional chaining for the source viewport.
platform/core/src/types/DisplaySet.ts	Adds the optional FrameOfReferenceUID property to the DisplaySet type definition, enabling type-safe FOR filtering across the codebase.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[SEG/RTSTRUCT Instance Loaded] --> B[SOP Class Handler]
    B --> C[Set FrameOfReferenceUID on DisplaySet]
    C --> D{Referenced DisplaySet Available?}
    D -->|Yes| E[Copy FOR from Referenced DisplaySet]
    D -->|No| F[Subscribe for DISPLAY_SETS_ADDED event]
    F --> G[On event: Copy FOR from added DisplaySet]
    
    E --> H[getEnhancedDisplaySets]
    G --> H
    
    H --> I{displaySet.FrameOfReferenceUID matches background?}
    I -->|Yes or undefined| J[isOverlayable: true]
    I -->|No| K[isOverlayable: false]
    
    J --> L[Shown in Overlay Menu]
    K --> M[Hidden from Overlay Menu]
    
    N[Segmentation Hydration Sync] --> O{Shared DisplaySet exists?}
    O -->|Yes| P[Add segmentation to target viewport]
    O -->|No| Q{Source FOR == Target FOR?}
    Q -->|Yes| P
    Q -->|No| R[Skip - different Frame of Reference]

_{Last reviewed commit: 9e5ae01}

Greptile also left 1 inline comment on this PR.

…f reference as viewport - Add FrameOfReferenceUID to SEG and RTSTRUCT displaySet in SOP Class Handlers so the FOR is available for filtering - Sync optimisticOverlayDisplaySets when background display set changes so the overlay menu reflects the correct state after a background switch - Add FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align.

…segmentation-menu-FOR

netlify · 2026-03-16T14:02:05Z

✅ Deploy Preview for ohif-dev canceled.

Name	Link
🔨 Latest commit	`c89dc7b`
🔍 Latest deploy log	https://app.netlify.com/projects/ohif-dev/deploys/69c1e4da60d10c0008ad7dda

greptile-apps · 2026-03-16T14:04:45Z

...nsions/cornerstone/src/components/ViewportDataOverlaySettingMenu/ViewportDataOverlayMenu.tsx

+  useEffect(() => {
+    setOptimisticOverlayDisplaySets(overlayDisplaySets);
+  }, [backgroundDisplaySet?.displaySetInstanceUID]);


Missing overlayDisplaySets dependency

overlayDisplaySets is referenced inside the useEffect callback but is not listed in the dependency array. This means the effect captures a stale closure value if overlayDisplaySets changes between renders without backgroundDisplaySet?.displaySetInstanceUID also changing. For instance, if a segmentation representation is added or removed while the background stays the same, and then the user switches the background, the effect would reset to a stale overlayDisplaySets snapshot from the render when the background last changed — not the current one.

Since React re-runs the effect by comparing dependency values, adding overlayDisplaySets ensures the optimistic state stays synchronized. If the intent is to only reset on background changes, consider using a ref to hold the latest overlayDisplaySets value instead.

Suggested change

useEffect(() => {

setOptimisticOverlayDisplaySets(overlayDisplaySets);

}, [backgroundDisplaySet?.displaySetInstanceUID]);

useEffect(() => {

setOptimisticOverlayDisplaySets(overlayDisplaySets);

}, [backgroundDisplaySet?.displaySetInstanceUID, overlayDisplaySets]);

@greptileai is correct here. Please add the missing dependency.

Suggested change

useEffect(() => {

setOptimisticOverlayDisplaySets(overlayDisplaySets);

}, [backgroundDisplaySet?.displaySetInstanceUID]);

useEffect(() => {

setOptimisticOverlayDisplaySets(overlayDisplaySets);

}, [backgroundDisplaySet?.displaySetInstanceUID, overlayDisplaySets]);

…segmentation-menu-FOR

jbocce · 2026-03-23T19:25:58Z

tests/pages/DataOverlayPageObject.ts

+    return labels;
+  }
+
+  getActiveSegmentationLocator(label: string) {


I am not crazy about the word Locator in this method name. Maybe ask AI for a better name?

I changed the name here :)

jbocce · 2026-03-24T12:49:38Z

tests/DataOverlayMenuFrameOfReference.spec.ts

+  page,
+  viewportPageObject,
+}) => {
+  const viewportId = await viewportPageObject.getIdOfNth(0);


I am not crazy about this getIdOfNth method. Could we not simply use await viewportPageObject.getNth(0) and then get the overlay object for it?

jbocce · 2026-03-24T12:51:58Z

tests/pages/ViewportPageObject.ts

    return this.viewportPageObjectFactory(viewport);
  }

+  async getIdOfNth(index: number): Promise<string> {


See my comment in the spec about possibly removing this metody. In general Playwright strongly suggests to NOT assert and/or get attributes, text or data of any kind... https://playwright.dev/docs/api/class-locator#locator-get-attribute

jbocce · 2026-03-24T12:56:22Z

tests/DataOverlayMenuFrameOfReference.spec.ts

+  await page.waitForTimeout(2000);
+
+  const segmentationLabels = await dataOverlay.getDropdownOptionLabels();
+  expect(segmentationLabels.sort()).toEqual([...segmentationLabelsFOR1].sort());


Are we sorting these because at present there is a bug whereby the labels might be in different order?

jbocce · 2026-03-24T13:01:08Z

tests/pages/DataOverlayPageObject.ts

+    await this.page.getByText('SELECT A SEGMENTATION').click();
+  }
+
+  async getDropdownOptionLabels(): Promise<string[]> {


There are two concerns I have about this method:

It goes against Playwright's principle to not get data and assert it. So instead this should be replaced by a utility that asserts the labels

It assumes that the drop down menu is open. Instead whether we keep this method or replace it with a utility, this function/method itself should open and close the menu to get and/or assert its data.

jbocce · 2026-03-24T13:07:32Z

tests/DataOverlayMenuFrameOfReference.spec.ts

+
+  await dataOverlay.toggle(viewportId);
+  await dataOverlay.openAddSegmentationDropdown(viewportId);
+  await page.waitForTimeout(2000);


No need to wait, especially if we replace getDropdownOptionLabels to be a utility that does an expect which will do the waiting for us.

jbocce · 2026-03-24T13:07:49Z

tests/DataOverlayMenuFrameOfReference.spec.ts

+  await dataOverlay.toggle(viewportId);
+  await dataOverlay.changeBackgroundDisplaySet('CT Std (FoR 2)-CT', viewportId);
+  await dataOverlay.openAddSegmentationDropdown(viewportId);
+  await page.waitForTimeout(2000);


See comment above about waiting.

jbocce · 2026-03-24T13:08:04Z

tests/DataOverlayMenuFrameOfReference.spec.ts

+  await dataOverlay.addSegmentation(segmentationFOR1Label, FOR1ViewportIdA);
+  await expect(dataOverlay.getOverlaySegmentationRow(segmentationFOR1Label)).toBeVisible();
+  await dataOverlay.closeMenu(FOR1ViewportIdA);
+  await page.waitForTimeout(2000);


jbocce · 2026-03-24T13:09:19Z

tests/pages/DataOverlayPageObject.ts

  }

+  async closeMenu(viewportId: string = 'default') {
+    await this.toggle(viewportId);


I think it is also best to wait for the menu to no longer be visible before returning. Again these are the principles of GUI testing.

jbocce

Thank you for this. See my comments.

GhadeerAlbattarni added 2 commits March 16, 2026 08:42

Merge remote-tracking branch 'upstream/master' into fix/5890-overlay-…

9e5ae01

…segmentation-menu-FOR

greptile-apps bot reviewed Mar 16, 2026

View reviewed changes

GhadeerAlbattarni added 2 commits March 17, 2026 09:10

test: add E2E tests for segmentation overlay menu Frame of Reference

937de0b

Merge remote-tracking branch 'upstream/master' into fix/5890-overlay-…

4051a6f

…segmentation-menu-FOR

GhadeerAlbattarni requested a review from jbocce March 23, 2026 00:53

jbocce reviewed Mar 23, 2026

View reviewed changes

address PR feedback

c89dc7b

GhadeerAlbattarni requested a review from jbocce March 24, 2026 03:21

jbocce reviewed Mar 24, 2026

View reviewed changes

jbocce requested changes Mar 24, 2026

View reviewed changes

Uh oh!

Conversation

GhadeerAlbattarni commented Mar 16, 2026 • edited by greptile-apps bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Root Cause

Changes & Results

Testing

Checklist

PR

Code

Public Documentation Updates

Tested Environment

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

netlify bot commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for ohif-dev canceled.

Uh oh!

greptile-apps bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbocce left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

GhadeerAlbattarni commented Mar 16, 2026 •

edited by greptile-apps bot

Loading

netlify bot commented Mar 16, 2026 •

edited

Loading