Skip to content

test(contour): add test for RT contour position after viewport maximize/restore - #6036

Merged
jbocce merged 2 commits into
masterfrom
fix/6025-contour-offset-double-click
May 22, 2026
Merged

test(contour): add test for RT contour position after viewport maximize/restore#6036
jbocce merged 2 commits into
masterfrom
fix/6025-contour-offset-double-click

Conversation

@GhadeerAlbattarni

@GhadeerAlbattarni GhadeerAlbattarni commented May 22, 2026

Copy link
Copy Markdown
Collaborator

Context

Fixes #6025

  • Adds a playwright test to verify that contour stays in the correct position after the user hydrates RT STRUCT series, clicks double clicks and double clicks again to go back to the original layout.

  • Updated Cornerstone3D to v4.22.10

Changes & Results

Add RTContourViewportLayoutChange.spec.ts

Testing

  • Open RT study in segmentation mode (e.g., StudyInstanceUIDs=1.2.840.113619.2.290.3.3767434740.226.1600859119.501)
  • Hydrate RTSTRUCT into a viewport
  • Double-click the viewport to maximize (one-up)
  • Double-click again
  • Verify contours remain correctly positioned

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 adds a Playwright regression test covering the bug where RT contour SVG paths shift position after maximizing a viewport and restoring the original multi-viewport layout. It also bumps Cornerstone3D from v4.22.8 to v4.22.10 across all extension and platform packages.

  • New test (RTContourViewportLayoutChange.spec.ts): hydrates an RTSTRUCT series in segmentation mode, captures the initial SVG path d attribute, performs two double-clicks to maximize then restore the layout, and asserts the path is identical after the cycle.
  • Dependency update: all @cornerstonejs/* packages (core, tools, adapters, ai, dicom-image-loader, labelmap-interpolation, polymorphic-segmentation) bumped uniformly to 4.22.10, with lock files updated accordingly.

Confidence Score: 4/5

Safe to merge; changes are additive (a new test) plus a routine patch-version dependency bump with no production logic changes.

The test is logically correct and follows the established waitForViewportRenderCycle pattern used elsewhere in the repo. The missing post-loadSeriesByModality wait and absent descriptive expect messages could cause occasional intermittent CI failures but do not affect production behavior.

tests/RTContourViewportLayoutChange.spec.ts deserves a second look for timing robustness before landing.

Important Files Changed

Filename Overview
tests/RTContourViewportLayoutChange.spec.ts New Playwright test verifying RT contour position is preserved after viewport maximize/restore; uses waitForViewportRenderCycle pattern but is missing a post-loadSeriesByModality wait and descriptive expect messages present in peer tests.
extensions/cornerstone/package.json Cornerstone3D packages bumped from 4.22.8 → 4.22.10 (patch version bump, consistent across all extension/platform packages).
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
tests/RTContourViewportLayoutChange.spec.ts:14-18
**Missing wait after `loadSeriesByModality`**

Other RT hydration tests in this repo (e.g. `ContourSegNavigation.spec.ts`) call `await page.waitForTimeout(5000)` between `loadSeriesByModality` and clicking the hydration button to let the RTSTRUCT series finish loading and the hydration prompt appear. Without this, the `waitForViewportRenderCycle` polling could catch an unrelated render cycle that completes before the hydration is triggered — or `getSvgAttribute` could run before the contours are actually rendered. This is a potential source of intermittent test failures.

### Issue 2 of 2
tests/RTContourViewportLayoutChange.spec.ts:22
**`expect` assertions missing descriptive messages**

Every `expect` call in this test omits the descriptive message string that the rest of the contour test files use (e.g. `expect(seg0, 'Segment at index 0: expected a non-null SVG path').not.toBeNull()`). Without these messages, test failures in CI will just report `expect(received).not.toBeNull()` with no indication of what was being verified or at which step the test broke. The same applies to the `not.toBeNull()` on line 42 and both assertions on lines 44–45.

Reviews (1): Last reviewed commit: "chore: update Cornerstone3D to v4.22.10" | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@netlify

netlify Bot commented May 22, 2026

Copy link
Copy Markdown

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit c4ea5e2
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a10922fe5d11a17bfdc4ebf
😎 Deploy Preview https://deploy-preview-6036--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment on lines +14 to +18
await leftPanelPageObject.loadSeriesByModality('RTSTRUCT');

let viewportRenderCycle = waitForViewportRenderCycle(page);
await DOMOverlayPageObject.viewport.segmentationHydration.yes.click();
await viewportRenderCycle;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing wait after loadSeriesByModality

Other RT hydration tests in this repo (e.g. ContourSegNavigation.spec.ts) call await page.waitForTimeout(5000) between loadSeriesByModality and clicking the hydration button to let the RTSTRUCT series finish loading and the hydration prompt appear. Without this, the waitForViewportRenderCycle polling could catch an unrelated render cycle that completes before the hydration is triggered — or getSvgAttribute could run before the contours are actually rendered. This is a potential source of intermittent test failures.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/RTContourViewportLayoutChange.spec.ts
Line: 14-18

Comment:
**Missing wait after `loadSeriesByModality`**

Other RT hydration tests in this repo (e.g. `ContourSegNavigation.spec.ts`) call `await page.waitForTimeout(5000)` between `loadSeriesByModality` and clicking the hydration button to let the RTSTRUCT series finish loading and the hydration prompt appear. Without this, the `waitForViewportRenderCycle` polling could catch an unrelated render cycle that completes before the hydration is triggered — or `getSvgAttribute` could run before the contours are actually rendered. This is a potential source of intermittent test failures.

How can I resolve this? If you propose a fix, please make it concise.


const initialSvgPath = await getSvgAttribute({
viewportPageObject,
svgInnerElement: 'path',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 expect assertions missing descriptive messages

Every expect call in this test omits the descriptive message string that the rest of the contour test files use (e.g. expect(seg0, 'Segment at index 0: expected a non-null SVG path').not.toBeNull()). Without these messages, test failures in CI will just report expect(received).not.toBeNull() with no indication of what was being verified or at which step the test broke. The same applies to the not.toBeNull() on line 42 and both assertions on lines 44–45.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/RTContourViewportLayoutChange.spec.ts
Line: 22

Comment:
**`expect` assertions missing descriptive messages**

Every `expect` call in this test omits the descriptive message string that the rest of the contour test files use (e.g. `expect(seg0, 'Segment at index 0: expected a non-null SVG path').not.toBeNull()`). Without these messages, test failures in CI will just report `expect(received).not.toBeNull()` with no indication of what was being verified or at which step the test broke. The same applies to the `not.toBeNull()` on line 42 and both assertions on lines 44–45.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@GhadeerAlbattarni
GhadeerAlbattarni requested a review from jbocce May 22, 2026 17:26
@jbocce
jbocce merged commit 5762095 into master May 22, 2026
12 checks passed
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.

[Bug] Double click to one-up and back with contour displaces the contour

2 participants