Skip to content

fix(rtstruct): prevent viewport from becoming blank on second load#5996

Open
Belbin-GK wants to merge 6 commits into
OHIF:masterfrom
Belbin-GK:rtstruct-viewport-blank-on-second-load
Open

fix(rtstruct): prevent viewport from becoming blank on second load#5996
Belbin-GK wants to merge 6 commits into
OHIF:masterfrom
Belbin-GK:rtstruct-viewport-blank-on-second-load

Conversation

@Belbin-GK
Copy link
Copy Markdown
Contributor

@Belbin-GK Belbin-GK commented May 6, 2026

Context

Hydration was being triggered before the viewport was fully initialized, which could lead to rendering issues such as a blank viewport, especially when loading RTSTRUCT multiple times.

Changes & Results

  • Removed the use of setTimeout for delaying hydration
  • Introduced a mechanism to wait for actual viewport readiness
5996-rtstruct-viewport-blank-on-second-load.mp4

Testing

  • Load a study with RT Struct.
  • Drag and drop the RT Struct.
  • Close the RT struct from the segmentation panel.
  • Drag and drop the RT Struct again.
  • Observe that RT struct is not loaded in the viewport and the segmentation 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: Windows 11
  • Node version: 22.20
  • Browser: Chrome 147.0.7727.138

Greptile Summary

This PR fixes a blank-viewport regression when RTSTRUCT is loaded a second time by replacing the old setTimeout(0) deferral with an event-driven wait: if getCornerstoneViewport returns null at hydration time, the code now subscribes to VIEWPORT_DATA_CHANGED and waits up to 5 seconds before proceeding, rejecting the outer promise on timeout.

  • The timeout and error-propagation logic inside waitForViewportDataChange are correctly wired — the inner promise's reject is used for the timer, and the outer catch block calls the outer reject before returning.
  • The new waitForViewportDataChange branch is entirely absent from tests because mockServicesManager never provides cornerstoneViewportService; the happy path (viewport already ready), the wait path, and the timeout/reject path all go unexercised.
  • Minor housekeeping: a stale window.setTimeout spy and a stale test description ("with setTimeout") remain in the test file from the old implementation.

Confidence Score: 5/5

The core fix is logically sound with correct timeout and rejection propagation; only test coverage is lacking.

The event-driven wait correctly handles both the success path (viewport becomes ready) and the failure path (5-second timeout rejects the outer promise). No new runtime error paths are introduced for production callers. The uncovered test paths are in the new branch only; all existing tested paths are unaffected.

extensions/cornerstone/src/utils/promptHydrationDialog.test.ts — the new waitForViewportDataChange branch is never entered by any test.

Important Files Changed

Filename Overview
extensions/cornerstone/src/utils/promptHydrationDialog.ts Replaces setTimeout-based hydration deferral with an event-driven wait on VIEWPORT_DATA_CHANGED; timeout and error propagation look correct, but the existing-viewport synchronous path for SEG (which previously always deferred) is a known behavioral change from prior threads.
extensions/cornerstone/src/utils/promptHydrationDialog.test.ts Removes the stale setTimeout assertion, but the new waitForViewportDataChange code path is entirely untested — mockServicesManager lacks cornerstoneViewportService, so the new branch is never entered in any test.
Prompt To Fix All With AI
Fix the following 4 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 4
extensions/cornerstone/src/utils/promptHydrationDialog.test.ts:20-26
**Missing tests for the new `waitForViewportDataChange` code path**

The `mockServicesManager` never includes `cornerstoneViewportService`, so `cornerstoneViewportService?.getCornerstoneViewport` evaluates to `undefined` in every test and the new `if (!viewport)` branch is never entered. This means the entire new mechanism — subscribing to `VIEWPORT_DATA_CHANGED`, the 5-second timeout, and the `reject` + `return` path — has zero test coverage. If a regression breaks the subscription or error-propagation logic, no test will catch it.

### Issue 2 of 4
extensions/cornerstone/src/utils/promptHydrationDialog.test.ts:165
Stale test description references `setTimeout`, which was the mechanism this PR explicitly removes. The test no longer exercises `setTimeout` at all (the `window.setTimeout` spy is never triggered for this path since `cornerstoneViewportService` is absent from the mock). Renaming avoids confusing future readers who expect a timeout-based deferral here.

```suggestion
  it('should handle SEG type hydration', async () => {
```

### Issue 3 of 4
extensions/cornerstone/src/utils/promptHydrationDialog.test.ts:49-52
The `window.setTimeout` spy was set up to intercept the old `window.setTimeout(async () => {...}, 0)` calls that this PR removes. The new `waitForViewportDataChange` uses bare `setTimeout` (not `window.setTimeout`), and none of the existing tests reach that branch anyway. Leaving a spy that no longer intercepts anything can mislead future authors into thinking timeouts are being controlled in these tests.

```suggestion
    jest.useFakeTimers();
```

### Issue 4 of 4
extensions/cornerstone/src/utils/promptHydrationDialog.ts:110-116
**Unnecessary `async` on the event callback inside `waitForViewportDataChange`**

The callback passed to `cornerstoneViewportService.subscribe` is marked `async` but contains no `await` expression. The `async` keyword causes the callback to return a Promise that the subscribe infra likely discards. While harmless today, it creates a subtle expectation mismatch: if an exception were ever introduced into the callback body, it would be swallowed as an unhandled promise rejection rather than propagating synchronously. Removing `async` makes the intent explicit.

Reviews (9): Last reviewed commit: "est: remove setTimeout expectation from ..." | Re-trigger Greptile

@netlify
Copy link
Copy Markdown

netlify Bot commented May 6, 2026

Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit 3f6ae2a
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a15525b6016220009ad1efe

@Belbin-GK Belbin-GK marked this pull request as ready for review May 6, 2026 09:21
Comment thread extensions/cornerstone/src/utils/promptHydrationDialog.ts Outdated
Comment thread extensions/cornerstone/src/utils/promptHydrationDialog.ts Outdated
Comment thread extensions/cornerstone/src/utils/promptHydrationDialog.ts
@Belbin-GK
Copy link
Copy Markdown
Contributor Author

@wayfarer3130 Could you please review the changes?

Comment thread extensions/cornerstone/src/utils/promptHydrationDialog.ts Outdated
@Belbin-GK Belbin-GK force-pushed the rtstruct-viewport-blank-on-second-load branch from e1f10d2 to 2ab1602 Compare May 26, 2026 05:32
@Belbin-GK Belbin-GK force-pushed the rtstruct-viewport-blank-on-second-load branch from c9aa6a8 to 3f6ae2a Compare May 26, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants