feat(testing): OHIF Test Agent Skills (#5993)

2026-06-20 11:30:27 -04:00 · 2026-06-20 11:30:27 -04:00 · b9cde4981a
commit b9cde4981a
parent fc994fe525
9 changed files with 896 additions and 2 deletions
--- a/.agents/skills/ohif-test-agent/SKILL.md
+++ b/.agents/skills/ohif-test-agent/SKILL.md
@ -0,0 +1,346 @@
+---
+name: ohif-test-agent
+description: Generate runnable Playwright E2E tests for the OHIF Viewer using its custom fixture system, page objects, and normalized WebGL viewport coordinates. Use this skill whenever the user asks to write, add, modify, or debug tests in an OHIF/Viewers context — including vague asks like "write a test for X" when working in the OHIF repo, tests touching platform/app/tests/, or anything involving Cornerstone viewports, DICOM studies, measurements, segmentations, or OHIF modes/extensions. Prefer this skill over generic test-writing even if the user doesn't say "Playwright" or "E2E" explicitly.
+---
+
+# OHIF Test Agent
+
+This skill teaches you to generate correct, runnable Playwright end-to-end tests for the OHIF Viewer. Follow the workflow below.
+
+## Environment model
+
+This package follows the agentskills.io SKILL.md convention. `SKILL.md` is the entire behavior contract — there is no separate runtime entrypoint.
+
+## Workflow: how to write a new OHIF test
+
+1. **Classify the feature.** What area does the test belong to — a measurement tool, segmentation hydration, contour panel interaction, MPR layout, crosshairs, tag browser, etc.? The area determines the mode, the StudyInstanceUID, and the seed spec you'll read.
+2. **Read the seed spec.** Consult [references/patterns-by-feature.md](references/patterns-by-feature.md) to find the canonical existing spec for that area. Read it end-to-end before writing. This is the single most important step — OHIF specs follow consistent idioms that are easier to mimic than to reconstruct from first principles. (This mirrors Playwright's own agent guidance: use seed tests as the example for generated tests.)
+3. **Scaffold from the template.** Start from [assets/spec-template.ts](assets/spec-template.ts) — or copy the seed spec and adapt.
+4. **Look up specifics in the source, not from memory.** The reference files [page-objects.md](references/page-objects.md) and [utilities.md](references/utilities.md) capture the **stable rules** — fixture keys, import conventions, access idioms, the reasons certain things trip people up. They deliberately do not enumerate methods. For the current method surface or a utility's exact signature, open the relevant file under `tests/pages/` or `tests/utils/` — the source evolves, and the source is always right. The seed spec you picked in step 2 is usually the fastest second source, because it co-evolves with the API.
+5. **Run the test when execution is available.** `yarn test:e2e:ci` runs the whole suite, but for iteration use `yarn playwright test tests/YourNew.spec.ts` (or via the Playwright VS Code extension).
+6. **If runtime execution is unavailable, do static validation.** Validate import source, fixture keys, normalized viewport usage, UID/mode pairing, and hydration/tracking prompt handling. Then report clearly that execution was not performed.
+7. **If it fails, triage before debugging.** Use [references/failure-triage.md](references/failure-triage.md) — most OHIF test failures are timing / hydration, not real regressions.
+
+## Architecture
+
+OHIF uses Playwright with a custom fixture system. Tests are **not** vanilla Playwright — they import `test`, `expect`, and utilities from `./utils`, which re-exports an extended test runner that injects page objects.
+
+```text
+playwright.config.ts            → Chromium-only, port 3335, data-cy as testId
+  └─ tests/utils/fixture.ts     → Extends playwright-test-coverage, injects page objects
+       └─ tests/*.spec.ts       → Each imports { test, expect, ... } from './utils'
+            ├─ tests/pages/     → Page objects (ViewportPageObject, MainToolbarPageObject, …)
+            └─ tests/utils/     → Utilities (visitStudy, checkForScreenshot, screenShotPaths, …)
+```
+
+Why the custom fixture matters: the page objects are created for each test and bound to the right Playwright `page`. If you `new ViewportPageObject(page)` manually, you skip the fixture wiring and some sub-objects won't resolve correctly.
+
+### Import rule
+
+```ts
+// Correct
+import { test, expect, visitStudy, checkForScreenshot, screenShotPaths } from './utils';
+
+// Wrong — will compile but fixtures won't be injected
+import { test, expect } from '@playwright/test';
+```
+
+A few utilities (`press`, `downloadAsString`, the `assert*` helpers) are NOT re-exported from `./utils`. See [references/utilities.md](references/utilities.md) for the correct import path per utility.
+
+## The viewport is WebGL
+
+OHIF renders medical images onto a WebGL canvas. You cannot query *canvas* pixels by CSS selector. Use **normalized coordinates** (0–1 range, top-left is `{x:0, y:0}`) for clicks and drags, and **visual regression** (screenshot comparison) for canvas assertions. (Not everything in the viewport is canvas — some overlays render as SVG you *can* query via DOM, e.g. a vector overlay's color through `getSvgAttribute`. The canvas rule is about raster output painted onto the WebGL surface.)
+
+```ts
+const activeViewport = await viewportPageObject.active;
+
+await activeViewport.normalizedClickAt([{ x: 0.5, y: 0.5 }]);                 // click center
+await activeViewport.normalizedClickAt([{ x: 0.3, y: 0.3 }, { x: 0.7, y: 0.7 }]); // draw two points
+await activeViewport.normalizedClickAt([{ x: 0.5, y: 0.5 }], 'right');         // right-click
+await activeViewport.normalizedDragAt({
+  start: { x: 0.3, y: 0.3 },
+  end:   { x: 0.7, y: 0.7 },
+});
+```
+
+Pixel coordinates (`clickAt`, `doubleClickAt`) exist but prefer normalized for portability across viewport sizes.
+
+For DOM-rendered state (panel counts, dialog text, overlay text values, button enabled states), assert directly:
+
+```ts
+await expect(activeViewport.overlayText.bottomRight.instanceNumber).toContainText('17/');
+const count = await rightPanelPageObject.measurementsPanel.panel.getMeasurementCount();
+expect(count).toBe(1);
+```
+
+## Study loading lifecycle
+
+Every test follows this sequence. Skipping steps causes flakiness:
+
+```ts
+test.beforeEach(async ({ page }) => {
+  const studyInstanceUID = '1.3.6.1.4.1.25403.345050719074.3824.20170125095438.5';
+  const mode = 'viewer';
+  await visitStudy(page, studyInstanceUID, mode, 2000); // 2s delay is the community norm
+});
+```
+
+`visitStudy` navigates to `/{mode}/ohif?StudyInstanceUIDs={uid}`, waits for `domcontentloaded`, then `networkidle`, then the explicit delay. Default delay is `0`, but most specs pass `2000` to let the first render settle.
+
+Delay by scene type (observed across the current suite, not a rule to apply blindly):
+
+| Scene | Delay |
+|-------|-------|
+| Default (viewer mode, 2D/MPR/3D layouts, crosshairs) | `2000` |
+| `mode: 'tmtv'` | `10000` — PET fusion + SUV calculation takes noticeably longer |
+
+Start at the convention for your scene; ramp only if the test flakes on initial render. 3D layouts in `viewer` mode already stay at `2000` — the stabilization problem there is solved with `attemptAction(() => reduce3DViewportSize(page), 10, 100)`, not with a longer `visitStudy` delay.
+
+If the study has DICOM SEG, RT, or SR data, OHIF asks whether to hydrate. Handle it:
+
+```ts
+await leftPanelPageObject.loadSeriesByModality('SEG'); // or 'RTSTRUCT', 'SR'
+await page.waitForTimeout(3000);                        // allow the prompt to appear
+await expect(DOMOverlayPageObject.viewport.segmentationHydration.locator).toBeVisible();
+await DOMOverlayPageObject.viewport.segmentationHydration.yes.click();
+```
+
+The first measurement you create triggers a "start tracking?" prompt:
+
+```ts
+await DOMOverlayPageObject.viewport.measurementTracking.confirm.click();
+```
+
+For 3D / MPR scenes, wrap stabilization in `attemptAction(() => reduce3DViewportSize(page), 10, 100)` or insert a `page.waitForTimeout(...)` after the layout change before asserting.
+
+## Wait for renders, don't sleep
+
+`page.waitForTimeout(...)` after an action that re-renders the viewport is a smell. The viewports tell us when they're done — use that signal. `tests/utils/waitForViewportsRendered.ts` exposes three helpers, all barrel-exported from `./utils`:
+
+- `waitForViewportRenderCycle(page)` — wait for the next full cycle: a viewport enters `needsRender`, then **all** viewports report `rendered` (and volumes are loaded, by default).
+- `waitForViewportsRendered(page)` — only the second half: wait until all viewports are `rendered`. Use this when the action has already requested a render before you started waiting (e.g. a layout change or `loadSeriesByDescription`).
+- `waitForAnyViewportNeedsRender(page)` — only the first half. Rarely needed directly.
+
+The canonical idiom — **start the watcher before the action, await it after**:
+
+```ts
+// start watching for the next render cycle
+const viewportRenderCycle = waitForViewportRenderCycle(page);
+
+await action(); // e.g. segmentationHydration.yes.click(), layoutSelection.MPR.click(), addSegmentation, etc.
+
+// wait for the render to finish
+await viewportRenderCycle;
+
+await check(); // e.g. checkForScreenshot, count assertion, overlay text
+```
+
+Why "start before"? `waitForViewportRenderCycle` first waits for a viewport to enter `needsRender`. If you start it **after** the action, that transition may already be over and you'll hang until the timeout. Starting it first captures the cycle the action is about to trigger.
+
+When to use which:
+
+| Situation | Helper |
+|-----------|--------|
+| Click that triggers a re-render and you want to assert after | `waitForViewportRenderCycle(page)` started before the click |
+| Layout switch / series load — render already in flight | `await waitForViewportsRendered(page)` after the call |
+| Compose with another await (e.g. screenshot the same time as load) | Save the promise, `await` it later |
+
+Replace patterns like this:
+
+```ts
+// ❌ Sleep-and-pray
+await action();
+await page.waitForTimeout(5000);
+await checkForScreenshot(...);
+
+// ✅ Wait on the actual signal
+const cycle = waitForViewportRenderCycle(page);
+await action();
+await cycle;
+await checkForScreenshot(...);
+```
+
+This shaves real wall-clock time off the suite and removes a class of flake (sleep too short → flake; sleep too long → slow). `tests/SEGHydrationFromMPR.spec.ts` is the canonical seed for this pattern.
+
+Caveats:
+- These helpers wait on Cornerstone viewport state. They won't help for purely DOM-side state (panel rows appearing, dialogs opening) — for those, prefer `expect(locator).toHaveCount(n)` / `toBeVisible()` which auto-retry, or `expect.toPass({ timeout })`.
+- For some actions (hanging-protocol changes are the documented example) the viewport doesn't transition through `needsRender` synchronously — those still need a short `waitForTimeout`. The source comment in `waitForViewportsRendered.ts` calls this out.
+
+## Fixture-injected page objects
+
+Destructure these from the test function argument. **Never `new` them manually.**
+
+```ts
+test('my test', async ({
+  page,
+  viewportPageObject,
+  mainToolbarPageObject,
+  leftPanelPageObject,
+  rightPanelPageObject,
+  DOMOverlayPageObject,          // note the capital D — this matches the fixture key
+  notFoundStudyPageObject,
+}) => { ... });
+```
+
+Two page objects are **not** fixture-injected:
+- `DataOverlayPageObject` — reach via `viewportPageObject.getById(id).overlayMenu.dataOverlay`.
+- `DicomTagBrowserPageObject` — reach via `DOMOverlayPageObject.dialog.dicomTagBrowser`.
+
+See [references/page-objects.md](references/page-objects.md) for fixture rules and a map of which file covers which concern; read the `.ts` file under `tests/pages/` for the current method surface.
+
+## When the control you need has no page object yet
+
+A spec must not reach for `page.getByTestId(...)` / `getByRole(...)` directly for
+application controls. If the button, menu, dialog, or field you need isn't already
+exposed by a page object, **add it to one — or create a new page object — instead of
+inlining a raw selector.** Raw selectors in a spec are the clearest sign a test was
+written without reading the existing suite: they duplicate locators, bypass the
+fixture wiring, and rot silently when the DOM changes.
+
+Where new coverage goes:
+
+| What you need | Where it belongs |
+|---|---|
+| A toolbar button or tool (Zoom, Pan) | a getter on `MainToolbarPageObject`, next to `crosshairs` / `measurementTools` |
+| A menu, prompt, context menu, or small dialog | `DOMOverlayPageObject` |
+| A substantial dialog with its own fields (User Preferences) | its **own** page object class, reached through `DOMOverlayPageObject` — follow `DicomTagBrowserPageObject` (`DOMOverlayPageObject.dialog.dicomTagBrowser`) |
+| Each field/row inside that dialog | a method or sub-object on the dialog's page object — not a raw selector in the spec |
+| A side-panel control | `LeftPanelPageObject` / `RightPanelPageObject` |
+
+If the control has no `data-cy`, **add `data-cy` to the source component** and target
+it — don't fall back to `getByRole`/text selectors, which are brittle and
+locale-sensitive (`testIdAttribute` is `data-cy`, so `getByTestId('Zoom')` resolves
+`[data-cy="Zoom"]`). Call out any `data-cy` you add so it ships in the same PR.
+
+**Worked example — "set the Zoom hotkey in User Preferences":** the options menu, the
+preferences dialog, each preference field, and the Zoom toolbar button should all be
+page-object surface — e.g. `mainToolbarPageObject.zoom`,
+`DOMOverlayPageObject.optionsMenu.settings.click()`,
+`DOMOverlayPageObject.dialog.userPreferences.hotkey('Zoom').set('q')`. The spec then
+reads as intent, not as a pile of `getByTestId` calls.
+
+## Assert the effect, not just the attribute
+
+Prefer asserting the actual rendered result over a proxy attribute. Activating Zoom and
+checking `data-active="true"` confirms the *button* toggled — not that zoom works. Drag
+on the viewport and assert the image actually zoomed (a viewport-scoped screenshot, or a
+measurable state change). Attribute checks are fine as a secondary signal, not the whole
+test.
+
+## Visual regression
+
+**Direction:** the suite is moving off *full-app* screenshots — not off screenshots
+altogether. "Avoid screenshots" means: don't screenshot the whole page, and don't
+screenshot something that has a faithful DOM/state signal. It does **not** mean avoid
+screenshots for output that is genuinely canvas-only — for that output a screenshot is
+the correct and required tool, and you should use it without apology. Older specs that
+screenshot the whole page are the legacy pattern being phased out; viewport-scoped
+screenshots are not.
+
+### Screenshot vs. DOM assertion — how to choose
+
+Reach for the cheapest *faithful* signal, in this order:
+
+1. **A faithful DOM/SVG/state signal exists → assert on it.** Panel counts, dialog and
+   overlay text, enabled/disabled state, and any overlay that renders as SVG (a vector
+   overlay's color is readable via `getSvgAttribute`) all have a DOM representation — assert
+   on it directly, no screenshot.
+2. **The thing under test is painted onto the WebGL canvas with no DOM representation → a
+   screenshot is correct and required.** Raster output on the canvas exposes no attribute to
+   read for a painted pixel. Scope a `checkForScreenshot` to the viewport (pane or grid) and
+   assert it — this is the right tool, not a last resort, whenever what you're verifying is
+   the rendered canvas itself.
+3. **Never substitute a service/state read for a render assertion.** Reading a service's
+   state (any `window.services...`) asserts the *data model*, not the pixels the user sees —
+   it passes even when rendering is broken. `page.evaluate(() => window.services...)` is an
+   escape hatch for *setup*, not for *appearance* assertions.
+
+For anything drawn onto the WebGL canvas with no DOM signal, compare a screenshot scoped to a specific viewport or the viewport grid:
+
+```ts
+await checkForScreenshot({
+  page,
+  locator: viewportPageObject.grid, // scope to the viewport grid — not the whole page
+  screenshotPath: screenShotPaths.length.lengthDisplayedCorrectly,
+});
+```
+
+`checkForScreenshot` retries up to 10 times at 500 ms intervals. Use `screenShotPaths.<category>.<name>` rather than a hand-typed string — the tree of valid keys lives in `tests/utils/screenShotPaths.ts`.
+
+Rules (apply to all new screenshot assertions):
+
+- **Use the object form.** The positional form is legacy; don't introduce it in new code, and don't treat existing positional-form usage as a pattern to copy.
+- **Never screenshot the full app.** Full-page screenshots include panels, toolbars, and dialogs that drift independently of what's under test and make baselines fragile. Scope by passing a `locator` — `viewportPageObject.grid` for the grid, or a specific viewport pane. A bare `normalizedClip: { x: 0, y: 0, width: 1, height: 1 }` with no `locator` is **not** scoping — it clips to the full page. Use `normalizedClip` only to target a sub-region *of a locator* (e.g. a scrollbar strip). If you reach for `fullPage: true`, stop and pick a locator.
+- **Do not tune `maxDiffPixelRatio` or `threshold`** to make a screenshot pass. If a baseline mismatches, regenerate it after a human review of the diff, or fix the underlying flake.
+
+## Playwright config facts worth remembering
+
+| Setting | Value | Why |
+|---------|-------|-----|
+| `baseURL` | `http://localhost:3335` | OHIF e2e uses 3335, not 3000 |
+| `testIdAttribute` | `data-cy` | `getByTestId(...)` maps to `[data-cy="..."]` |
+| `browser` | Chromium only | Firefox/WebKit disabled (SharedArrayBuffer + stability) |
+| `retries` | 3 in CI, 0 locally | Flaky rendering needs CI retries |
+| `workers` | 6 in CI, undefined locally | Parallel execution |
+| `globalTimeout` / `timeout` | 800_000 ms | Medical image loads are slow |
+| `actionTimeout` | 10_000 ms | Per-action cap |
+| `webServer.command` | `cross-env APP_CONFIG=config/e2e.js COVERAGE=true OHIF_PORT=3335 nyc yarn start` | e2e config + coverage |
+
+## Test data: which study for which test
+
+| StudyInstanceUID | Mode | Used for |
+|------------------|------|----------|
+| `1.3.6.1.4.1.25403.345050719074.3824.20170125095438.5` | `viewer` | General measurements, annotations, context menu |
+| `1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785` | `viewer` | 3D, MPR, crosshairs |
+| `1.3.6.1.4.1.14519.5.2.1.256467663913010332776401703474716742458` | `viewer` or `segmentation` | Labelmap SEG |
+| `1.2.840.113619.2.290.3.3767434740.226.1600859119.501` | `viewer` / `segmentation` / `tmtv` | RTSTRUCT/contour and TMTV |
+| `1.3.6.1.4.1.14519.5.2.1.7695.4007.324475281161490036195179843543` | `viewer` | SR hydration |
+
+Full mapping in [references/patterns-by-feature.md](references/patterns-by-feature.md). **Do not invent UIDs** — they must exist on the e2e data server.
+
+## Rules (short, so they're actually read)
+
+1. Import `test`, `expect`, and utilities from `./utils`, not `@playwright/test`.
+2. Destructure fixture-injected page objects; don't `new` them.
+3. Use normalized coordinates (0–1) for viewport interactions.
+4. Use `visitStudy` with a real UID, correct mode, and a non-zero delay (2000 is conventional).
+5. Handle hydration and measurement-tracking prompts where applicable.
+6. Choose the faithful signal: DOM/SVG assertions where the rendered result has one (panels, dialogs, overlay text, SVG/vector overlays), a viewport-scoped screenshot when what you're verifying is canvas-only raster output, and never a `window.services` state read standing in for a render check. Screenshots use the object form, scoped via a `locator` — never the full app.
+7. Use `data-cy` selectors (already wired via `testIdAttribute`).
+8. When an assertion needs retry tolerance, wrap it in `expect.toPass({ timeout })`.
+9. Test in the correct mode — segmentation tools aren't available in `viewer` mode.
+10. If a utility isn't exported from `./utils`, import from the deeper path (see [references/utilities.md](references/utilities.md)).
+11. After an action that re-renders the viewport, prefer `waitForViewportRenderCycle(page)` (started before the action) over `page.waitForTimeout(...)`. See the "Wait for renders, don't sleep" section.
+12. Don't inline raw `page.getByTestId(...)` / `getByRole(...)` for app controls. If a control has no page object, create or augment one (see "When the control you need has no page object yet"), adding a `data-cy` to the source if needed.
+13. Assert the actual effect (e.g. the image zoomed), not just a proxy attribute like `data-active`.
+
+## Pre-output self-check (mandatory)
+
+Before returning a generated OHIF test, confirm all items:
+
+1. Imports `test`/`expect` from `./utils` (not `@playwright/test`).
+2. Uses fixture-injected keys and exact casing (especially `DOMOverlayPageObject`).
+3. Uses normalized viewport interactions (`normalizedClickAt` / `normalizedDragAt`) unless there is a strong reason otherwise.
+4. Uses a valid canonical StudyInstanceUID and compatible mode.
+5. Handles hydration or measurement tracking prompts when the workflow requires them.
+6. Uses the faithful signal for each assertion — DOM/SVG where the result has a DOM representation, a viewport-scoped screenshot when what's verified is canvas-only raster output, and never a `window.services` state read in place of a render check. Any `checkForScreenshot` call uses the object form, scoped via a `locator` (viewport pane or grid) — no full-app screenshots.
+7. Replaces `page.waitForTimeout(...)` after viewport-rendering actions with `waitForViewportRenderCycle(page)` (started before the action) — keeps `waitForTimeout` only for non-render waits like the hydration prompt in `beforeEach`.
+8. If execution was skipped, states that explicitly and provides concrete run commands.
+9. Every application control is reached through a page object — no raw `getByTestId`/`getByRole` in the spec for buttons, menus, dialogs, or fields. Any control not already covered was added to the right page object (or a new one), with a source `data-cy` if it lacked one.
+10. Assertions verify the real effect where feasible (e.g. the image visibly zoomed), not only an attribute toggle.
+
+## Output contract (for non-executing agents)
+
+When execution cannot be performed in the current environment, the response should include:
+
+1. The test code.
+2. Assumptions made (if any).
+3. Static checks that were verified.
+4. What still must be run locally and exact commands to run.
+
+## When to consult each reference
+
+- **Before writing** → [references/patterns-by-feature.md](references/patterns-by-feature.md). Pick the seed spec for the feature area and read it. The seed spec is the closest thing to a live API example because it co-evolves with the code.
+- **For a stable rule or idiom** (fixture keys, import paths, panel-access order, capital-D quirk, object-param convention) → [references/page-objects.md](references/page-objects.md), [references/utilities.md](references/utilities.md).
+- **For a method name, property, or signature** → read the source under `tests/pages/` or `tests/utils/`. Do not rely on a static table for these; they drift as the code is refactored.
+- **When a test fails** → [references/failure-triage.md](references/failure-triage.md).
--- a/.agents/skills/ohif-test-agent/assets/spec-template.ts
+++ b/.agents/skills/ohif-test-agent/assets/spec-template.ts
@ -0,0 +1,54 @@
+import {
+  checkForScreenshot,
+  expect,
+  screenShotPaths,
+  test,
+  visitStudy,
+  waitForViewportRenderCycle,
+} from './utils';
+
+test.beforeEach(async ({ page }) => {
+  // Pick the right UID + mode for your feature (see references/patterns-by-feature.md)
+  const studyInstanceUID = '1.3.6.1.4.1.25403.345050719074.3824.20170125095438.5';
+  const mode = 'viewer';
+  await visitStudy(page, studyInstanceUID, mode, 2000);
+});
+
+test.describe('FEATURE NAME', () => {
+  test('describes the behaviour in one sentence', async ({
+    page,
+    viewportPageObject,
+    mainToolbarPageObject,
+    rightPanelPageObject,
+    DOMOverlayPageObject, // capital D on purpose
+  }) => {
+    // 1. Arrange — select a tool, open a panel, etc.
+
+    // 2. Act — interact with the viewport.
+    //    Prefer normalized (0–1) coordinates:
+    //    const activeViewport = await viewportPageObject.active;
+    //    await activeViewport.normalizedClickAt([{ x: 0.3, y: 0.3 }, { x: 0.7, y: 0.7 }]);
+
+    //    For actions that re-render the viewport, gate on the render cycle
+    //    instead of sleeping — start the watcher BEFORE the action:
+    //    const cycle = waitForViewportRenderCycle(page);
+    //    await action();
+    //    await cycle;
+
+    // 3. Handle prompts (first measurement prompts for tracking; SEG/RT/SR prompts for hydration)
+    //    await DOMOverlayPageObject.viewport.measurementTracking.confirm.click();
+
+    // 4. Assert canvas output via visual regression — object form, scoped to the
+    //    viewport via a locator (not the full page). normalizedClip is only for
+    //    clipping to a sub-region of that locator.
+    //    await checkForScreenshot({
+    //      page,
+    //      locator: viewportPageObject.grid,
+    //      screenshotPath: screenShotPaths.YOUR_CATEGORY.YOUR_KEY,
+    //    });
+
+    // 5. Assert DOM state directly
+    //    const count = await rightPanelPageObject.measurementsPanel.panel.getMeasurementCount();
+    //    expect(count).toBe(1);
+  });
+});
--- a/.agents/skills/ohif-test-agent/references/failure-triage.md
+++ b/.agents/skills/ohif-test-agent/references/failure-triage.md
@ -0,0 +1,70 @@
+# Failure triage
+
+Before debugging, classify. Most OHIF test failures are timing or hydration — not real regressions.
+
+| Category | Symptom | Fix |
+|----------|---------|-----|
+| Timing | Element not visible, action timeout | Add / increase the `delay` param of `visitStudy`; for actions that re-render viewports, use `waitForViewportRenderCycle(page)` (started before the action) instead of `waitForTimeout`; wrap the assertion in `expect.toPass({ timeout })` |
+| Selector | Element not found | Verify `data-cy` on the target; confirm the panel is open (`toggle()` / `select()` before interacting); check for capital `D` in `DOMOverlayPageObject` when destructuring |
+| Hydration | Segmentation/RT not interactive | Ensure the `segmentationHydration.yes.click()` fired; add `waitForTimeout(3000)` after `loadSeriesByModality('SEG'\|'RTSTRUCT'\|'SR')` |
+| Data | Study not found, empty viewport | Confirm the UID is in the canonical list (see [patterns-by-feature.md](patterns-by-feature.md)); confirm the mode supports the feature (segmentation tools aren't in `viewer` mode) |
+| Visual drift | Screenshot mismatch but feature works | Have a human review the diff, then regenerate the baseline with `yarn playwright test --update-snapshots`. Do not adjust `maxDiffPixelRatio` or `threshold` to make a failing screenshot pass. |
+| Real regression | Feature is actually broken | Report as a bug — this is the test doing its job |
+
+## Prefer render-cycle waits over sleeps
+
+If you're tempted to add `await page.waitForTimeout(2000)` after an action, ask whether the action re-rendered the viewport. If it did, use:
+
+```ts
+const cycle = waitForViewportRenderCycle(page);
+await action();
+await cycle;
+await check();
+```
+
+The watcher must be created **before** the action — it waits for `needsRender` first, and that transition is gone by the time the action returns. See the "Wait for renders, don't sleep" section in [SKILL.md](../SKILL.md) and `tests/SEGHydrationFromMPR.spec.ts`.
+
+### When the cycle helper times out at `waitForAnyViewportNeedsRender`
+
+Symptom: the test fails inside `waitForAnyViewportNeedsRender` after 5s, with the action having actually completed in the UI. The action just doesn't transition the viewport through `needsRender` synchronously. Known cases:
+
+- Hanging-protocol changes.
+- **RTSTRUCT / contour segmentation hydration confirm.** SEG (labelmap) hydration does fire `needsRender`; contour does not. The fix is to gate on the actual end-state — for hydration in `beforeEach`, `await expect(page.getByTestId('data-row')).toHaveCount(N)` is the right wait.
+
+Don't react by raising the cycle's timeout — the transition isn't coming. Replace the cycle wrapper with an auto-retrying DOM/SVG assertion, or `expect.toPass({ timeout })` around the assertion block.
+
+## The `toPass` pattern
+
+When an assertion needs to wait for async render / propagation:
+
+```ts
+await expect(async () => {
+  await rightPanelPageObject.labelMapSegmentationPanel.panel.segmentByText('Spleen').click();
+  await expect(activeViewport.overlayText.bottomRight.instanceNumber).toContainText('17/');
+}).toPass({ timeout: 10_000 });
+```
+
+`toPass` reruns the assertion block until it succeeds or the timeout expires — cleaner than a hand-rolled retry loop and surfaces the last failure reason if it times out.
+
+## Common `DOMOverlayPageObject` mistake
+
+The fixture key is capital-D `DOMOverlayPageObject`, not lowercase. If your destructure is silently `undefined`, check the casing.
+
+## `press` import mistake
+
+`press` is NOT re-exported from `./utils`. `import { press } from './utils'` resolves to `undefined` and fails at runtime. Use:
+
+```ts
+import { press } from './utils/keyboardUtils';
+await press({ page, key: 'ArrowDown', nTimes: 50 }); // object param, not (page, key)
+```
+
+## Visual regression iteration
+
+Screenshots live under `tests/screenshots/chromium/<testFilePath>/`. To accept new output as the baseline:
+
+```sh
+yarn playwright test tests/YourSpec.spec.ts --update-snapshots
+```
+
+Review the resulting PNGs carefully — an agent-accepted baseline that's subtly wrong is worse than a failing test.
--- a/.agents/skills/ohif-test-agent/references/page-objects.md
+++ b/.agents/skills/ohif-test-agent/references/page-objects.md
@ -0,0 +1,120 @@
+# OHIF Page Object guide
+
+> This file documents the **stable structural rules** of the page object system. For the current list of methods and properties on any class, **read the source under `tests/pages/`** — it is always authoritative, and it evolves as the product does. A static method table in a reference file goes stale the moment someone refactors; the source does not.
+
+## How to discover the API of a page object
+
+1. Find the relevant class in `tests/pages/`. File names match class names.
+2. Read it end-to-end once — most are under a few hundred lines.
+3. Some classes compose sub-objects (e.g. `RightPanelPageObject` holds a measurementsPanel, contourSegmentationPanel, labelMapSegmentationPanel, tmtvPanel, etc.). Those sub-objects usually live in the same file or a sibling under `tests/pages/`.
+4. To see how a method is actually used, grep `tests/` or open the seed spec listed in [patterns-by-feature.md](patterns-by-feature.md). Real usage beats a synthesized signature every time.
+
+Do not try to memorize a method surface from this file — it intentionally does not list one. It lists only the rules you cannot derive from the source by reading a single file.
+
+---
+
+## Stable rules
+
+### Fixture keys (case-sensitive)
+
+These are injected via `tests/utils/fixture.ts`. Destructure them from the test function's first argument — do not `new` them, because the fixture wires sub-objects to the correct `page` and hand-constructed instances skip that wiring.
+
+- `viewportPageObject`
+- `mainToolbarPageObject`
+- `leftPanelPageObject`
+- `rightPanelPageObject`
+- `DOMOverlayPageObject` — **capital D**. A silent `undefined` destructure is almost always a casing typo here.
+- `notFoundStudyPageObject`
+
+If the fixture file is updated and new keys are added, they will show up there first — check it if something feels missing.
+
+### Non-fixture page objects
+
+Some page object classes are not fixture-injected. They are reached through an injected fixture:
+
+- `DicomTagBrowserPageObject` → via `DOMOverlayPageObject.dialog.dicomTagBrowser`
+- `DataOverlayPageObject` → via `viewportPageObject.getById(viewportId).overlayMenu.dataOverlay`
+
+Both can be constructed manually (`new DataOverlayPageObject(page)`) if a test really needs a fresh instance, but the accessor path is the idiomatic one.
+
+### Viewport wrapper vs. viewport instance
+
+`viewportPageObject` is a **wrapper**. You almost always want a specific viewport out of it first:
+
+- `await viewportPageObject.active` — the currently focused viewport
+- `viewportPageObject.getAll()` — every viewport in the grid
+- `viewportPageObject.getNth(i)` — zero-indexed
+- `viewportPageObject.getById(cornerstoneViewportId)` — e.g. `'default'`, `'ctAXIAL'`
+
+The object these return is the one with `normalizedClickAt`, `normalizedDragAt`, `overlayText`, `nthAnnotation`, etc. Reach for the viewport instance first, then call methods on it.
+
+### Panel access order
+
+Every `rightPanelPageObject` sub-panel follows the same three-step idiom: **open the side panel, `.select()` the sub-panel tab, then interact with `.panel.*`**. Skipping either of the first two is the most common cause of "element not found".
+
+```ts
+await rightPanelPageObject.toggle();
+await rightPanelPageObject.measurementsPanel.select();
+const count = await rightPanelPageObject.measurementsPanel.panel.getMeasurementCount();
+```
+
+The exact row/action methods vary by panel — check the source file for the one you need.
+
+### Layout identifiers are camelCase JS properties
+
+`mainToolbarPageObject.layoutSelection.<layout>.click()` — access layouts by camelCase property name (e.g. `threeDFourUp`, `axialPrimary`), not with bracket-escaped DICOM-ish strings like `['3DFourUp']`. This is a convention enforced by how the class exposes its tools.
+
+### Sub-tools auto-open their dropdown
+
+Tools nested inside a toolbar dropdown (measurement tools, more tools, layouts) each expose a `.click()` that opens the parent menu for you. You almost never need to open the menu first. `await mainToolbarPageObject.measurementTools.length.click()` does both the expand and the select.
+
+### When the control you need isn't covered yet — create or augment
+
+The page objects here cover what the suite currently exercises. When your test needs a
+control that isn't exposed, **extend the page object system rather than dropping a raw
+`page.getByTestId(...)` into the spec.** A raw selector in a spec is the clearest tell
+that the author didn't read the existing tests — it duplicates a locator that should
+live in one place and bypasses the fixture wiring.
+
+| What you need | Where it goes | Precedent to copy |
+|---|---|---|
+| A toolbar button/tool (Zoom, Pan) | a getter on `MainToolbarPageObject` | `crosshairs`, `measurementTools` |
+| A menu / prompt / context menu / small dialog | `DOMOverlayPageObject` | `viewport.measurementTracking`, `dialog.input` |
+| A large dialog with its own fields (User Preferences) | a **new** page object class reached via `DOMOverlayPageObject` | `DicomTagBrowserPageObject` (`DOMOverlayPageObject.dialog.dicomTagBrowser`) |
+| Individual fields/rows in that dialog | methods/sub-objects on the dialog's page object | `DicomTagBrowserPageObject.seriesSelect` |
+| Side-panel controls | `LeftPanelPageObject` / `RightPanelPageObject` | existing sub-panels |
+
+Two more expectations:
+
+- **Missing `data-cy`?** Add it to the source component and target it; don't fall back
+  to `getByRole`/text. `testIdAttribute` is `data-cy`, so `getByTestId('Zoom')` resolves
+  `[data-cy="Zoom"]`. Mention any attribute you added so it ships in the same PR.
+- **Mirror the existing shape.** A new getter should look like its neighbors (return a
+  locator, or a small object with `.click()` etc.) so the new surface is
+  indistinguishable from what was already there.
+
+Example — a "set the Zoom hotkey" test should make `mainToolbarPageObject.zoom`, an
+options-menu accessor on `DOMOverlayPageObject`, and a `userPreferences` dialog page
+object (with a per-hotkey field accessor) exist — then the spec reads as steps, not
+selectors.
+
+---
+
+## Page object map — what each class is *for*
+
+This table exists to help you pick the right file to open, not to enumerate methods. Source of truth for any specific method remains the `.ts` file.
+
+| Class | File | Covers |
+|-------|------|--------|
+| ViewportPageObject | `tests/pages/ViewportPageObject.ts` | Cornerstone viewports — clicks, drags, overlays, annotations, crosshairs |
+| MainToolbarPageObject | `tests/pages/MainToolbarPageObject.ts` | Top toolbar — measurement tools, more tools, layouts, crosshairs, pan |
+| LeftPanelPageObject | `tests/pages/LeftPanelPageObject.ts` | Study browser — thumbnails, load by modality or description |
+| RightPanelPageObject | `tests/pages/RightPanelPageObject.ts` | Side panels — measurements, contour seg, labelmap seg, TMTV, microscopy |
+| DOMOverlayPageObject | `tests/pages/DOMOverlayPageObject.ts` | DOM overlays — dialogs, hydration/tracking prompts, context menus, tag-browser accessor |
+| NotFoundStudyPageObject | `tests/pages/NotFoundStudyPageObject.ts` | Study-not-found error page |
+| DicomTagBrowserPageObject | `tests/pages/DicomTagBrowserPageObject.ts` | Tag-browser dialog (non-fixture; reach via `DOMOverlayPageObject.dialog`) |
+| DataOverlayPageObject | `tests/pages/DataOverlayPageObject.ts` | Data-overlay menu (non-fixture; reach via `viewport.overlayMenu`) |
+
+If the directory adds or renames a file, that diff is your first clue and this table is your second — trust the directory.
+
+For live usage, the seed spec in [patterns-by-feature.md](patterns-by-feature.md) shows how a class is actually called; the source file tells you everything else that's on it.
--- a/.agents/skills/ohif-test-agent/references/patterns-by-feature.md
+++ b/.agents/skills/ohif-test-agent/references/patterns-by-feature.md
@ -0,0 +1,148 @@
+# Seed specs by feature area
+
+> When writing a new OHIF test, find the closest feature area below and read the listed spec in full before writing. Playwright's own guidance says the seed test "serves as an example of all the generated tests" — that applies here.
+>
+> **If a spec listed below has moved or been renamed**, grep `tests/` for a remaining example (e.g. `grep -rn "loadSeriesByModality('RTSTRUCT')" tests/`). The pattern matters more than the exact filename — specs get renamed, the feature area persists.
+>
+> **No close match below?** This list only covers areas with a seed worth copying; don't add a stub for every untested area. See [Feature area not listed above?](#feature-area-not-listed-above-no-existing-seed).
+
+## Canonical study UIDs
+
+| UID | Mode(s) | What it has |
+|-----|---------|-------------|
+| `1.3.6.1.4.1.25403.345050719074.3824.20170125095438.5` | `viewer` | CT — default for measurement/annotation tests |
+| `1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785` | `viewer` | CT volume for 3D/MPR/crosshairs |
+| `1.3.6.1.4.1.14519.5.2.1.256467663913010332776401703474716742458` | `viewer` or `segmentation` | CT + SEG for labelmap tests |
+| `1.2.840.113619.2.290.3.3767434740.226.1600859119.501` | `viewer` / `segmentation` / `tmtv` | CT + RTSTRUCT + PET, used for contour and TMTV |
+| `1.3.6.1.4.1.14519.5.2.1.7695.4007.324475281161490036195179843543` | `viewer` | SR structured report |
+
+Do not invent UIDs — they must exist on the e2e data server.
+
+---
+
+## 1. Simple measurement tools (length, angle, bidirectional, rectangle, ellipse, circle)
+
+**Seed:** `tests/Length.spec.ts`, `tests/Angle.spec.ts`
+
+Pattern: select tool via `mainToolbarPageObject.measurementTools.<tool>.click()`, place N points via `activeViewport.normalizedClickAt([...])`, confirm the tracking prompt, screenshot via `screenShotPaths.<name>.<name>DisplayedCorrectly`.
+
+## 2. Freehand / spline / livewire ROIs
+
+**Seed:** `tests/FreehandROI.spec.ts`, `tests/Livewire.spec.ts`, `tests/Spline.spec.ts`
+
+Pattern: `normalizedDragAt({ start, end, config: { steps: 20, delay: 30 } })` for smooth strokes; `subscribeToMeasurementAdded` to assert the event fires; `activeViewport.nthAnnotation(0)` to reference what was drawn.
+
+## 3. Annotations (arrow, probe)
+
+**Seed:** `tests/ArrowAnnotate.spec.ts`, `tests/Probe.spec.ts`
+
+Arrow annotate opens `DOMOverlayPageObject.dialog.input` for the label. Use `fillAndSave(label)`.
+
+## 4. Measurement panel interactions
+
+**Seed:** `tests/MeasurementPanel.spec.ts`
+
+Panel access: `rightPanelPageObject.toggle()` → `.measurementsPanel.select()` → `.panel.nthMeasurement(i)` → `.actions.rename|delete|toggleLock|duplicate|...`. Also demonstrates `addLengthMeasurement(page)` and panel-row `click()` for jump-to.
+
+## 5. Context menu (right-click on annotation)
+
+**Seed:** `tests/ContextMenu.spec.ts`
+
+Two ways to open: `activeViewport.normalizedClickAt([{...}], 'right')` on an empty area, or `activeViewport.nthAnnotation(0).contextMenu.open()` on a drawn annotation. Then `DOMOverlayPageObject.viewport.annotationContextMenu.addLabel|delete.click()`.
+
+## 6. Labelmap segmentation (SEG) hydration
+
+**Seed:** `tests/SEGHydration.spec.ts`, `tests/SEGHydrationThenMPR.spec.ts`
+
+Flow: `leftPanelPageObject.loadSeriesByModality('SEG')` → `waitForTimeout(3000)` → `DOMOverlayPageObject.viewport.segmentationHydration.yes.click()`. Often pokes Cornerstone state directly via `page.evaluate(() => window.cornerstone...)` for zoom/render.
+
+## 7. Contour segmentation (RTSTRUCT) + interactions
+
+**Seeds:**
+- Hydration: `tests/RTHydration.spec.ts`
+- Rename: `tests/ContourSegmentRename.spec.ts`
+- Navigation between segments: `tests/ContourSegNavigation.spec.ts`
+- Duplicate: `tests/ContourSegmentDuplicate.spec.ts`
+- Visibility: `tests/ContourSegmentToggleVisibility.spec.ts`
+- Locking: `tests/ContourSegLocking.spec.ts`
+
+Pattern: load RTSTRUCT series → hydrate → use `rightPanelPageObject.contourSegmentationPanel.panel.nthSegment(i)` / `.segmentByText('Small Sphere')` and their `.actions.rename|delete`, or the segment's `.click()` to jump.
+
+## 8. Labelmap segmentation editing (brush / eraser / threshold)
+
+**Seed:** `tests/LabelMapSegLocking.spec.ts`, `tests/SEGDrawingToolsResizing.spec.ts`
+
+Pattern: `rightPanelPageObject.labelMapSegmentationPanel.tools.brush.setRadius(n)` / `.click()`, then `activeViewport.normalizedDragAt(...)` to paint.
+
+## 9. TMTV / PET
+
+**Seeds:** `tests/TMTVCSVReport.spec.ts`, `tests/TMTVSUV.spec.ts`, `tests/TMTVAlignment.spec.ts`, `tests/TMTVRendering.spec.ts`
+
+Visit `mode: 'tmtv'` with a longer delay (`10000`). `rightPanelPageObject.tmtvPanel` for the side panel. Exporting a report uses `page.waitForEvent('download')` + `downloadAsString(download)`.
+
+## 10. MPR and 3D layouts
+
+**Seeds:** `tests/MPR.spec.ts`, `tests/3DOnly.spec.ts`, `tests/3DFourUp.spec.ts`, `tests/3DMain.spec.ts`, `tests/AxialPrimary.spec.ts`
+
+Pattern: `mainToolbarPageObject.layoutSelection.<layoutName>.click()`. For 3D, wrap stabilization with `attemptAction(() => reduce3DViewportSize(page), 10, 100)` to settle the render before asserting.
+
+## 11. Crosshairs
+
+**Seed:** `tests/Crosshairs.spec.ts`
+
+Pattern: `initializeMousePositionTracker(page)` in `beforeEach`, `mainToolbarPageObject.crosshairs.click()`, then `viewportPageObject.crosshairs.axial.rotate()` / `.increase()`.
+
+## 12. Overlays — data overlay menu, window/level, orientation
+
+**Seeds:** `tests/DataOverlayMenu.spec.ts`, `tests/WindowLevelOverlayText.spec.ts`, `tests/MultipleSegmentationDataOverlays.spec.ts`
+
+Pattern: `viewportPageObject.getById('default').overlayMenu.dataOverlay.toggle()`, then `.addSegmentation(name)` / `.changeSegmentation(from, to)` / `.remove(name)`. For keyboard navigation, remember `press({ page, key, nTimes })` imports from `./utils/keyboardUtils`.
+
+## 13. DICOM Tag Browser
+
+**Seed:** `tests/DicomTagBrowser.spec.ts`
+
+Open with `mainToolbarPageObject.moreTools.tagBrowser.click()`, then interact via `DOMOverlayPageObject.dialog.dicomTagBrowser.waitVisible()` / `.seriesSelect.selectOption(i)` / `.seriesSelect.getOptionText(i)`.
+
+## 14. Study validation / not-found / worklist
+
+**Seeds:** `tests/StudyValidation.spec.ts`, `tests/Worklist.spec.ts`
+
+These are the rare specs that use `page.goto(...)` directly instead of `visitStudy()` — because they test error states or non-study pages. `notFoundStudyPageObject` gives you `errorMessage`, `returnMessage`, `studyListLink`.
+
+## 15. SR (Structured Report) hydration
+
+**Seeds:** `tests/SRHydration.spec.ts`, `tests/SRHydrationThenReload.spec.ts`
+
+Same shape as SEG hydration but load via `loadSeriesByModality('SR')`.
+
+## Feature area not listed above? (no existing seed)
+
+Don't add a stub section here for an untested area. When your target isn't listed:
+
+1. **Look for an indirect seed.** Grep `tests/` for the controls or flow you need
+   (`grep -rn "options-menu" tests/`, a related panel/dialog). An adjacent area's seed
+   usually still shows how the harness is driven.
+2. **If genuinely uncovered, you're *creating* page objects.** Read
+   [page-objects.md](page-objects.md) → "When the control you need isn't covered yet", then:
+   - One page object per new control. Dialogs follow `DicomTagBrowserPageObject` (reached
+     through `DOMOverlayPageObject`); toolbar buttons get a getter on `MainToolbarPageObject`.
+   - Add a `data-cy` to any control that lacks one.
+   - **Verify the real effect, not a proxy** — drag and screenshot that the image moved via
+     `locator: viewportPageObject.grid`, not just that a button gained `data-active`.
+
+**Example — user preferences / hotkeys (no seed today):** open the options menu and User
+Preferences dialog (options-menu accessor on `DOMOverlayPageObject` + a `userPreferences`
+dialog page object reached through it), set the hotkey, save, then trigger it and confirm the
+viewport effect (activate Zoom, drag, screenshot the zoom). Add the Zoom button as a getter on
+`MainToolbarPageObject`.
+
+---
+
+## Advanced patterns worth knowing
+
+- **`subscribeToMeasurementAdded`** (`tests/FreehandROI.spec.ts`) — for async "was a measurement actually added?" assertions. Always `try { ... } finally { await measurementAdded.unsubscribe() }`.
+- **`attemptAction`** (`tests/3DOnly.spec.ts`) — retry flaky setup (3D render, heavy layout change) without silencing real failures.
+- **`addOHIFConfiguration`** (`tests/RTHydrationDisableConfirmation.spec.ts`) — pre-load config overrides before `visitStudy`.
+- **`page.evaluate(() => window.services...)`** — used in several SEG/SR specs to set customizations or poke viewport state. Treat as an escape hatch, not a default.
+- **`expect.toPass({ timeout })`** — wrap flaky assertions (common for jump-to-measurement tests where rendering settles asynchronously).
--- a/.agents/skills/ohif-test-agent/references/utilities.md
+++ b/.agents/skills/ohif-test-agent/references/utilities.md
@ -0,0 +1,100 @@
+# OHIF Test Utility guide
+
+> This file documents the **stable import rules and conventions** around `tests/utils/`. For the current list of exported helpers and their exact signatures, **read `tests/utils/index.ts` and the files it re-exports** — the barrel is always current; a static table here is not. Utilities get added, renamed, and refactored; the rules below change much more slowly.
+
+## How to discover what's available
+
+1. Open `tests/utils/index.ts`. Every symbol exported from the barrel is importable as `import { foo } from './utils'`.
+2. If what you need isn't in the barrel, look in the rest of `tests/utils/` — there are a handful of specialized files (`keyboardUtils.ts`, `assertions.ts`, `download.ts`, …). These need the **deeper import path**; see the rule below.
+3. For the actual signature, read the utility's `.ts` file. It's one short function per file in most cases.
+4. To see a utility in context, grep `tests/` (`grep -rn visitStudy tests/`) — or open the seed spec for the relevant feature area ([patterns-by-feature.md](patterns-by-feature.md)). Existing specs are the most reliable signature reference because they co-evolve with the API.
+
+Do not guess parameter shapes from memory, and do not treat this file as an API catalog — it intentionally isn't one.
+
+---
+
+## Stable rules
+
+### Barrel vs. deep imports
+
+Two import styles exist. They are not interchangeable.
+
+```ts
+// Barrel — anything re-exported from tests/utils/index.ts
+import { test, expect, visitStudy, checkForScreenshot, screenShotPaths } from './utils';
+
+// Deep — for files NOT re-exported by the barrel
+import { press } from './utils/keyboardUtils';
+import { assertNumberOfModalityLoadBadges } from './utils/assertions';
+import { downloadAsString } from './utils/download';
+```
+
+If a symbol isn't in the barrel, `import { x } from './utils'` **compiles**, resolves `x` to `undefined`, and blows up at the first call site. Confirm by opening `tests/utils/index.ts` for your working revision. At the time of this writing, `press`, `downloadAsString`, and the `assert*` helpers live outside the barrel — but maintainers can move things in or out, so treat `index.ts` as the ground truth rather than this note.
+
+### Never import `test` / `expect` from `@playwright/test`
+
+```ts
+// ✅ Correct — gets the fixture-extended runner
+import { test, expect } from './utils';
+
+// ❌ Wrong — compiles, but every page-object fixture is silently undefined
+import { test, expect } from '@playwright/test';
+```
+
+If your test function's destructured arguments (like `viewportPageObject`) are `undefined`, this import is almost always why.
+
+### `visitStudy` — 2000 ms is a convention, not a default
+
+```ts
+await visitStudy(page, studyInstanceUID, mode, 2000);
+```
+
+The function's own default delay is `0`. Nearly every spec passes `2000` to let the first render settle. The one consistent exception is `mode: 'tmtv'`, where the suite uniformly uses `10000` because PET fusion and SUV calculation add real wall-clock cost before the UI is interactive.
+
+Notably, 3D layouts in `viewer` mode (3DOnly, 3DFourUp, 3DMain, 3DPrimary) and MPR also use `2000` — they're not "heavy" in the `visitStudy` sense. Their stabilization problem is solved at the interaction layer with `attemptAction(() => reduce3DViewportSize(page), 10, 100)`, not by a longer visit delay.
+
+So: pick `10000` when the mode is `tmtv`, `2000` otherwise, and only ramp up if a specific test flakes on first-render assertions. The delay is a good first lever for "not visible" flakes, but it's not a universal upgrade.
+
+### `checkForScreenshot` — use the object form, never screenshot the full app
+
+**This is the direction going forward** The suite is being migrated off *full-app* screenshots — not off screenshots altogether. Screenshots are the correct and required tool whenever what you're verifying is canvas-only raster output with no DOM signal; don't avoid them there. Avoid them only where a faithful DOM/SVG signal exists (e.g. a vector overlay's color via `getSvgAttribute`) or where you'd be capturing the whole app. See SKILL.md → "Screenshot vs. DOM assertion — how to choose". Any new spec must follow the rules below, and any modification to an older spec should bring it in line when reasonable.
+
+- **Object form** (required for all new specs): `checkForScreenshot({ page, screenshotPath, normalizedClip?, ... })`
+- **Positional form**: legacy. It still appears in older specs because they haven't been migrated yet. **Do not treat existing positional-form usage as a pattern to copy** — those specs are the thing being moved away from. Do not introduce the positional form in new code.
+
+**Hard rules for new screenshots:**
+
+1. Use the object form.
+2. Scope by passing a `locator` — `viewportPageObject.grid` for the whole grid, or a specific viewport pane locator. **Never screenshot the full app.** `normalizedClip` is computed *relative to the locator* (and defaults to the full page when no locator is given), so `{ x: 0, y: 0, width: 1, height: 1 }` alone does not scope anything — reserve `normalizedClip` for clipping to a sub-region of a locator. If you find yourself reaching for `fullPage: true`, stop and pass a locator instead.
+
+Do not tune `maxDiffPixelRatio` or `threshold` to make a screenshot pass — those are intentionally rarely touched and not the right knob for flakes. If a baseline mismatches, regenerate it (`--update-snapshots`) after a human review of the diff, or fix the underlying instability. Check the current signature in `tests/utils/checkForScreenshot.ts` if something looks off.
+
+### `screenShotPaths` — use keys, not raw strings
+
+```ts
+await checkForScreenshot({
+  page,
+  locator: viewportPageObject.grid,
+  screenshotPath: screenShotPaths.length.lengthDisplayedCorrectly,
+});
+```
+
+The full tree of categories lives in `tests/utils/screenShotPaths.ts`. When you need a new baseline, **add the key there and reference it by name** rather than typing a raw path. A typo in a key becomes a compile error instead of a silent mismatch, and other tests become discoverable through the object.
+
+### Object-param convention
+
+Many OHIF test utilities take a single **object argument** rather than positional arguments — notably `press({ page, key, nTimes? })`, the `simulate*` helpers, and the `assert*` helpers. If a call looks like it should work but throws "cannot read properties of undefined," check whether you're passing positional args to something that expects `{ page, ... }`.
+
+Read the one-line signature at the top of the utility's source file before calling it — it's faster than guessing, and it's always right.
+
+---
+
+## Utility shapes worth flagging
+
+Most utilities are obvious once you read the source; these earn a mention:
+
+- **`waitForViewportRenderCycle(page, options?)`** — preferred replacement for `page.waitForTimeout(...)` after any viewport-mutating action (hydration confirm, layout change, segmentation add, series load, etc.). Start it **before** the action, await it after — it captures the `needsRender → rendered` transition the action triggers. Use `waitForViewportsRendered(page)` (the second-half-only variant) when the render is already in flight before you can attach a watcher. Source: `tests/utils/waitForViewportsRendered.ts`. Seed: `tests/SEGHydrationFromMPR.spec.ts`. The "Wait for renders, don't sleep" section in [SKILL.md](../SKILL.md) covers the idiom in full.
+- **`subscribeToMeasurementAdded(page)`** — returns `{ waitFired(timeout?), unsubscribe() }`. Use in freehand/livewire/spline specs to assert the event fired. Always wrap in `try { ... } finally { await sub.unsubscribe() }` so a failing assertion doesn't leak the listener across tests.
+- **`attemptAction(action, attempts?, delay?)`** — retries a flaky async action without masking real failures. Mainly used to stabilize 3D scenes (`attemptAction(() => reduce3DViewportSize(page), 10, 100)`).
+
+For everything else, the pattern is: find a spec that uses it (see [patterns-by-feature.md](patterns-by-feature.md)), copy the shape, adapt.
--- a/.gitignore
+++ b/.gitignore
@ -61,8 +61,6 @@ tests/playwright-report/
 /blob-report/
 /playwright/.cache/

-**/.claude/settings.local.json
-
 # cornerstone3D local linking
 libs/

--- a/AGENTS.md
+++ b/AGENTS.md
@ -176,6 +176,10 @@ Always prioritrize pub sub, by calling a services subscribe over useEffects as i
 ### Never modify core architecture
 Do not modify the core and always find a way to implement the solution via the extensions and modes, only modify core as a last resort if all other fail or there's an architectural constraint.

+## Skills
+
+The `ohif-test-agent` skill (Playwright E2E test guidance) lives at `.agents/skills/ohif-test-agent/`.
+
 ## Configuration

 ### Plugin Configuration
--- a/preinstall.js
+++ b/preinstall.js
@ -32,3 +32,57 @@ try {
 } catch (err) {
  console.log(`Skipped CLAUDE.md symlink: ${err.message}`);
 }
+
+// Mirror every skill under .agents/skills/ into .claude/skills/
+try {
+  const agentsSkillsDir = path.join(__dirname, '.agents', 'skills');
+  const claudeSkillsDir = path.join(__dirname, '.claude', 'skills');
+
+  if (!fs.existsSync(agentsSkillsDir)) {
+    console.log('No .agents/skills directory found, skipping skill symlinks');
+  } else {
+    fs.mkdirSync(claudeSkillsDir, { recursive: true });
+
+    const agentsDirectoryEntries = fs.readdirSync(agentsSkillsDir, { withFileTypes: true });
+    for (const agentFolderEntry of agentsDirectoryEntries) {
+      if (!agentFolderEntry.isDirectory()) {
+        continue;
+      }
+
+      const claudeSkillLinkPath = path.join(claudeSkillsDir, agentFolderEntry.name);
+      const expectedSymlinkTarget = path.join('..', '..', '.agents', 'skills', agentFolderEntry.name);
+
+      try {
+        const existingLinkStats = fs.lstatSync(claudeSkillLinkPath, { throwIfNoEntry: false });
+
+        if (existingLinkStats) {
+          if (!existingLinkStats.isSymbolicLink()) {
+            console.log(
+              `Skipped skill symlink ${agentFolderEntry.name}: ${claudeSkillLinkPath} exists and is not a symlink`
+            );
+            continue;
+          }
+
+          const existingSymlinkTarget = fs.readlinkSync(claudeSkillLinkPath);
+          if (existingSymlinkTarget === expectedSymlinkTarget) {
+            continue;
+          }
+
+          // A symlink pointing somewhere else is treated as a developer
+          // override (.claude/ is gitignored local config). Leave it alone.
+          console.log(
+            `Skipped skill symlink ${agentFolderEntry.name}: ${claudeSkillLinkPath} points to ${existingSymlinkTarget}, not ${expectedSymlinkTarget}`
+          );
+          continue;
+        }
+
+        fs.symlinkSync(expectedSymlinkTarget, claudeSkillLinkPath, 'dir');
+        console.log(`Linked .claude/skills/${agentFolderEntry.name} -> ${expectedSymlinkTarget}`);
+      } catch (err) {
+        console.log(`Skipped skill symlink ${agentFolderEntry.name}: ${err.message}`);
+      }
+    }
+  }
+} catch (err) {
+  console.log(`Skipped skill symlink setup: ${err.message}`);
+}