fix: prevent black viewport when navigating series with client-created segmentation

[GhadeerAlbattarni]

Context

Fixes #5888

When a user opens a study with multiple series, adds a segmentation, and then keeps using Page Down to navigate between series, the viewport goes black once reaches the added segmentation displayset.

Root cause
When a segmentation is added via "Add Segmentation", a client-created display set is pushed into activeDisplaySets.

The Page Down hotkey triggers updateViewportDisplaySet, which iterates through activeDisplaySets to determine the next display set to render.

Since these client-created segmentation display sets are not filtered out, the command attempts to render them as standalone viewport display sets, causing the viewport to go black.

Changes & Results

Skip display sets marked with madeInClient when selecting the next display set to show in updateViewportDisplaySet during Page Down/Up navigation

Before
After adding a segmentation, repeated Page Down navigation could leave the viewport blank

After
Page Down now skips client-created segmentation display sets and keeps the viewport on a valid series.

Testing

1. open a study with multiple series in a segmentation mode (e.g., StudyInstanceUIDs=1.2.840.113619.2.290.3.3767434740.226.1600859119.501 )
2. click Add segmentation
3. Double click on the default viewport
4. Press Page Down repeatedly to navigate across all display sets
5. Verify the viewport continues to show valid series instead of turning black once it reaches the last display set in the left panel.

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 black viewport issue that occurred when pressing Page Down to navigate series after adding a client-created segmentation in OHIF. The root cause was that updateViewportDisplaySet iterated over activeDisplaySets without filtering out display sets marked madeInClient = true, so the viewport tried to render a segmentation-only display set as a standalone image, resulting in a blank screen.

Key changes:

• extensions/default/src/commandsModule.ts: Inside the updateViewportDisplaySet navigation loop, a continue guard is added before the existing modality filter. If nextDisplaySet.madeInClient is true, the loop immediately moves to the next candidate instead of breaking on it. This is consistent with how madeInClient is guarded elsewhere (e.g., setUpSegmentationEventHandlers.ts line 84).
• tests/SegmentationSeriesNavigation.spec.ts: New Playwright E2E test that adds a segmentation to a known study, double-clicks into the viewport, presses PageDown 6 times, and asserts the instance-number overlay remains visible after each press — verifying the viewport never goes black.

Confidence Score: 4/5

• Safe to merge; the logic change is minimal, targeted, and consistent with existing madeInClient guards across the codebase.
• The one-line continue guard is a small, well-scoped change with no regression risk for normal series navigation. The only non-blocking concern is the hardcoded totalPageDownPresses = 6 in the test, which may not definitively exercise the boundary condition depending on the study's series count.
• No files require special attention.

Important Files Changed

Filename	Overview
extensions/default/src/commandsModule.ts	Adds a madeInClient guard in the updateViewportDisplaySet loop to skip client-created segmentation display sets during Page Down/Up navigation, preventing black viewports.
tests/SegmentationSeriesNavigation.spec.ts	New E2E test that adds a segmentation and verifies the viewport overlay remains visible across 6 PageDown presses; the hardcoded count of 6 may not reliably trigger the boundary condition on all study configurations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User presses Page Down] --> B[updateViewportDisplaySet called]
    B --> C[Get activeDisplaySets from DisplaySetService]
    C --> D[Find activeDisplaySetIndex of current viewport]
    D --> E[Loop: next index = activeDisplaySetIndex + direction]
    E --> F{nextDisplaySet.madeInClient?}
    F -- Yes --> G[continue — skip this display set]
    G --> E
    F -- No --> H{excludeNonImageModalities AND modality is non-image?}
    H -- Yes: skip --> E
    H -- No: use it --> I[break — found valid display set]
    E --> J{Index out of bounds?}
    J -- Yes --> K[return — no navigation]
    I --> L[getViewportsRequireUpdate]
    L --> M[setDisplaySetsForViewports]
    M --> N[Viewport renders valid display set]

Loading

_{Reviews (1): Last reviewed commit: "test(e2e): add a test for Page Down seri..." | Re-trigger Greptile}

Greptile also left 1 inline comment on this PR.

[netlify]

✅ Deploy Preview for ohif-dev canceled.

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

[greptile-apps]

Hardcoded navigation count may not exercise the bug

totalPageDownPresses = 6 is a magic number with no inline comment explaining why it was chosen. If the study referenced (1.2.840.113619.2.290.3.3767434740.226.1600859119.501) has more than 5 real (non-madeInClient) series before the segmentation display set is inserted, pressing PageDown only 6 times may never actually reach the madeInClient entry, meaning the test would pass even without the fix.

Consider either:

• Adding a comment documenting how many series this study has and where the madeInClient display set lands, so future readers know the number is sufficient.
• Or increasing the count to a higher bound (e.g., 10) to ensure the segmentation boundary is crossed.

[jbocce]

This is a good point. One possibility might be to add an expect the number of thumbnails in the side panel to be at least 6. To stick with the Playwright principle of using waits for assertions, we should do something like this...

await expect.poll(async () => {
    return await thumbnailsLocator.count();
  }).toBeGreaterThanOrEqual(6);

https://playwright.dev/docs/test-assertions#expectpoll

[GhadeerAlbattarni]

Hello @jbocce. For the test I didn't use the screenshots approach as I noticed in the test environments, the series in the left panel render in a different order. So when navigating through them, we might not always be on the same first display set which makes the test fails.

I went with asserting the viewport image/instance number.

I also removed the fix, and the run the test. The test failed as expected when reached the black screen.

[jbocce]

I hate to say it, but I think we need a wait timeout here. 🤣

[jbocce]

Actually, no, I am wrong. With what you have the expect does the wait! Good stuff!

[jbocce]

Great effort on this. Please see my comments.

[hongxiao-gif]

Test passed.
The viewport is smoothly navigate between series using the Page Down and Page Up hotkey, even after a new segmentation is created.
Testing notes: https://linear.app/ohif/issue/TEST-49

System:
OS: Windows 11 10.0.26200
CPU: (8) x64 Intel(R) Core(TM) Ultra 5 226V
Memory: 4.82 GB / 15.67 GB
Browsers:
Chrome: 146.0.7680.165
Edge: Chromium (146.0.3856.59)

[jbocce]

To keep the number of series tied to this consider something like this...

const minimumSeriesExpected = 5;
const totalPageDownPresses = minimumSeriesExpected + 3;

And then on line 27 you can use the minimumSeriesExpected variable too.

[GhadeerAlbattarni]

Thanks for pointing this :)

[jbocce]

One minor comment left...

[jbocce]

Looks good.