test(vitest-browser): state-based integration suites for GenericViewport and tools#2792
Conversation
…iewport and tools Adds 12 state-based (non-screenshot) integration test suites in Vitest Browser Mode, plus a shared harness (deterministic fake image stack, viewport factory, state-snapshot capture, event recorder, tools input synthesis). All assertions target public API state, events, and closed-form expected values on synthetic data. Core viewport suites: render-path state parity, geometric round-trip invariants, event contracts, lifecycle/teardown/races, serialization and session restore. Tools suites (black-box, public API only): annotation tool measurement matrix, tool group state machine + manipulation tools, annotation state management and persistence, labelmap segmentation state, undo/redo history, cross-viewport synchronizers. Runs green in ~90s: 100 passed, 12 expected-fail (each test.fails pins a real engine finding documented in-file), no pixel comparisons. Claude-Session: https://claude.ai/code/session_01NJNAWVWWtguh3aA8w1R9mp
📝 WalkthroughWalkthroughAdds a large Vitest browser test harness and contract suite for Cornerstone viewport, annotation, segmentation, synchronizer, tool-group, and serialization behavior. Also adds a Playwright helper for retrying remote fixture requests and wires it into DICOM loader specs, plus minor browser-test config and ignore updates. ChangesVitest Browser Test Coverage
Remote fixture retry utility
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/vitest-browser/planarInvariants.browser.test.ts (1)
92-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing the normalization helper across test files.
deepRoundhere andnormalizeDeepinviewStateSerialization.browser.test.ts(lines 86–123) implement the same rounding/typed-array/undefined-key normalization logic. Both comments note this duplication is intentional per the plan's "add local helpers rather than edit harness files" instruction. If the harness's internal normalizer were ever exported in the future, both copies could be replaced with a single import, reducing the maintenance surface.This is purely optional given the explicit plan constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/planarInvariants.browser.test.ts` around lines 92 - 129, The deepRound helper duplicates the same normalization logic already implemented as normalizeDeep in viewStateSerialization.browser.test.ts. Keep the current local helper for now, but if the shared harness normalization becomes available later, replace both copies with a single imported utility so the rounding, typed-array conversion, and undefined-key filtering stay consistent across test files.tests/vitest-browser/synchronizers.browser.test.ts (1)
673-708: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
wait(500)vsNO_PROPAGATION_WAIT_MS(400) inconsistency.Line 692 uses a hardcoded
500for the quiescence window whileNO_PROPAGATION_WAIT_MS = 400(line 266) was defined for the same negative-result pattern. The comment explains the 500ms is per the plan's wording, but a future reader may wonder why two different constants exist for the same purpose. Consider either reusingNO_PROPAGATION_WAIT_MSor defining a separateQUIESCENCE_WAIT_MSconstant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/synchronizers.browser.test.ts` around lines 673 - 708, The no-sync-storms test is using a hardcoded quiescence delay that duplicates an existing timeout constant. Update the test around createZoomPanSynchronizer and the wait-based negative-result check to either reuse NO_PROPAGATION_WAIT_MS or introduce a clearly named QUIESCENCE_WAIT_MS constant for this bounded wait, so the intent stays consistent and the timeout value is defined in one place.tests/vitest-browser/eventContracts.browser.test.ts (1)
198-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
secondElementbypasses the safety-net teardown.Unlike every other resource in this suite,
secondElementis only removed at Lines 230-232. If any assertion between Lines 213-228 fails, the test throws and the element stays attached todocument.body(per test 2,destroy()leaves DOM children in place), leaking into subsequent tests. Registering it with thetrack()net keeps it consistent with the file's stated leak-prevention intent.♻️ Track secondElement removal in the safety net
document.body.appendChild(secondElement); + track(() => secondElement.parentNode?.removeChild(secondElement)); ctx.renderingEngine.enableElement({Then the explicit removal at the end (Lines 230-232) becomes redundant and can be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/eventContracts.browser.test.ts` around lines 198 - 232, The `secondElement` in the `eventContracts.browser.test.ts` teardown is not covered by the suite’s `track()` safety net, so a failure before the final cleanup can leak a DOM node into later tests. Update the test setup around `secondElement` to register it with `track()` alongside the other resources, using the existing teardown helpers in this file. Then remove the manual `parentNode.removeChild` cleanup at the end since the tracked teardown should handle it consistently.tests/vitest-browser/toolGroupAndManipulation.browser.test.ts (1)
184-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against a leaked first viewport if the second
createPlanarViewport()throws.
ais created, thenb = await createPlanarViewport(). Ifb's creation rejects, thecleanupclosure is never returned andactiveCleanupis never assigned in the calling test, soa'sRenderingEngine, DOM element, fake image-stack registration, and the flippeduseGenericViewportconfig all leak into subsequent tests. In CI a flaky WebGL context on the second viewport would then cascade failures across the file instead of failing this one test cleanly.♻️ Clean up
aifbfails- const a = await createPlanarViewport(); - const b = await createPlanarViewport(); + const a = await createPlanarViewport(); + let b: PlanarViewportContext; + try { + b = await createPlanarViewport(); + } catch (error) { + a.cleanup(); + cornerstoneTools.destroy(); + throw error; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/toolGroupAndManipulation.browser.test.ts` around lines 184 - 197, Guard the first viewport setup in this test so it cannot leak if the second createPlanarViewport call fails. Update the test flow around createPlanarViewport, activeCleanup, and the cleanup handling so that if b creation rejects after a is created, a.cleanup() is still invoked before the error propagates. Make the fix in the browser test’s two-viewport setup path so RenderingEngine, DOM, and config state are always released even when the second viewport creation throws.tests/vitest-browser/undoRedoHistory.browser.test.ts (1)
118-133: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWait for
SEGMENTATION_RENDEREDbefore the first paint
renderAndWaitonly waits forIMAGE_RENDERED. This setup attaches the labelmap and starts painting immediately afterward, but the sibling segmentation suite waits for the firstSEGMENTATION_RENDEREDbecause the labelmap-to-viewport resolver can still be unresolved until that pass. Switching this setup to the segmentation render event would avoid a latent no-op stroke andSEGMENTATION_DATA_MODIFIEDhang.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/undoRedoHistory.browser.test.ts` around lines 118 - 133, The setup in undoRedoHistory.browser.test.ts starts painting after renderAndWait, but that helper only waits for IMAGE_RENDERED and can leave the segmentation layer unresolved. Update the first paint path to wait for the first SEGMENTATION_RENDERED event instead, using the same segmentation render listener approach already used in the sibling segmentation suite, so the labelmap-to-viewport resolver is ready before strokes begin.tests/vitest-browser/annotationToolsMatrix.browser.test.ts (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider resetting selection/locking/visibility in
afterEachfor consistency withannotationStateManagement.The
afterEachinannotationStateManagement.browser.test.ts(lines 85–122) explicitly resetsannotation.selection,annotation.locking, andannotation.visibilitybecause these are module-level UID-keyed Sets not cleared bydestroy()/init()orremoveAllAnnotations(). Drawing annotations auto-selects them, leaving stale UIDs that could leak into subsequent tests. While no current test in this file asserts on selection/locking/visibility state, adding the same resets here would ensure consistency and prevent subtle issues if future tests are added.♻️ Suggested addition
afterEach(() => { if (!active) { return; } const ctx = active; active = null; ctx.cleanup(); + + // Reset module-level UID sets not cleared by cleanup(), matching + // annotationStateManagement.browser.test.ts afterEach. + try { annotation.selection.deselectAnnotation(); } catch {} + try { annotation.locking.unlockAllAnnotations(); } catch {} + try { annotation.visibility.showAllAnnotations(); } catch {} });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/annotationToolsMatrix.browser.test.ts` around lines 49 - 57, The afterEach cleanup in annotationToolsMatrix.browser.test.ts only calls ctx.cleanup(), but it should also reset the shared annotation.selection, annotation.locking, and annotation.visibility state for consistency with annotationStateManagement.browser.test.ts. Update the afterEach block near active/ctx.cleanup to clear those module-level UID-keyed Sets after cleanup so auto-selected annotations do not leak stale state into later tests.tests/vitest-browser/toolMeasurements.browser.test.ts (1)
34-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated utilities and setup with the shared harness.
This file imports
createPlanarViewportandrenderAndWaitfrom./harnessbut duplicates several utilities that the harness already provides:
round2(lines 34–36) — duplicated inannotationToolsMatrix.browser.test.tsdistance3(lines 38–43) — equivalent toworldDistancefrom the harnessclientPointFromCanvasPoint,dispatchMouseDown,dispatchMouseMove,dispatchMouseUp(lines 45–105) — equivalent to the harness's internal dispatch helpers intools.tsdrawAnnotationByDrag(lines 119–142) — equivalent tomouseDrag+waitForAnnotationRenderedfrom the harness, but dispatches a singlemousemoveinstead of interpolated stepssetupToolTest(lines 160–195) — equivalent tosetupToolsfrom the harness
annotationToolsMatrix.browser.test.tssuccessfully usessetupTools,mouseDrag,waitForAnnotationRendered, andworldDistancefrom the harness for the same tool types. Migrating this file to those same imports would eliminate ~100 lines of duplicated code and ensure consistent test infrastructure. The single-mousemovedifference indrawAnnotationByDragworks for Length/RectangleROI but could silently break if path-sensitive tools are added later.♻️ Suggested migration sketch
import { createPlanarViewport, renderAndWait, + setupTools, + mouseDrag, + waitForAnnotationRendered, + worldDistance, type PlanarViewportContext, + type ToolsContext, } from './harness'; -const { LengthTool, RectangleROITool, ToolGroupManager, annotation } = - cornerstoneTools; -const { Events: ToolsEvents, MouseBindings } = cornerstoneTools.Enums; -const { filterAnnotationsForDisplay } = cornerstoneTools.utilities.planar; - -function round2(point: Types.Point2): [number, number] { - return [Math.round(point[0]), Math.round(point[1])]; -} - -function distance3(a: Types.Point3, b: Types.Point3): number { - ... -} - -// ... dispatchMouseDown, dispatchMouseMove, dispatchMouseUp, -// ... drawAnnotationByDrag, setupToolTest all removed - -async function drawAnnotationByDrag(element, p1, p2) { ... } - -async function setupToolTest(toolClass, createOpts) { ... } +const { LengthTool, RectangleROITool, annotation } = cornerstoneTools; +const { Events: ToolsEvents } = cornerstoneTools.Enums; +const { filterAnnotationsForDisplay } = cornerstoneTools.utilities.planar; + +// Use setupTools from harness, then mouseDrag + waitForAnnotationRendered +// for drawing, and worldDistance for length calculations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/vitest-browser/toolMeasurements.browser.test.ts` around lines 34 - 195, This test file reimplements harness helpers and setup logic that already exist in the shared browser test harness. Replace the local `round2`, `distance3`, mouse dispatch helpers, `drawAnnotationByDrag`, and `setupToolTest` usage with the shared helpers used by `annotationToolsMatrix.browser.test.ts` (`setupTools`, `mouseDrag`, `waitForAnnotationRendered`, and `worldDistance`). Keep the test behavior aligned with the harness by using the same drag/wait flow and remove the duplicated utility code so the test suite stays consistent with `harness` and `tools.ts`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/vitest-browser/harness/createPlanarViewport.ts`:
- Around line 120-153: The setup in createPlanarViewport leaves
renderingConfig.useGenericViewport set to true before cleanup is available, so
any throw from RenderingEngine, enableElement, or nearby setup can leak state
into later tests. Move the restoration logic into a try/finally (or equivalent
guarded setup) around the initialization path so useGenericViewport is always
reset, and make sure partially created resources like the rendering engine, DOM
element, and fake stack are cleaned up even when setup fails. Keep the fix
localized to createPlanarViewport and its cleanup handling.
In `@tests/vitest-browser/harness/fakeImageStack.ts`:
- Around line 181-196: The imagePlaneModule shape in fakeImageStack is using the
wrong geometry mapping: swap width/height to match the IImage object, and fix
pixelSpacing to follow DICOM row/column ordering. Update the imagePlaneModule
return in fakeImageStack so width uses columns, height uses rows, and
pixelSpacing matches rowPixelSpacing/columnPixelSpacing semantics (with
rowPixelSpacing and columnPixelSpacing left consistent with the existing
values).
---
Nitpick comments:
In `@tests/vitest-browser/annotationToolsMatrix.browser.test.ts`:
- Around line 49-57: The afterEach cleanup in
annotationToolsMatrix.browser.test.ts only calls ctx.cleanup(), but it should
also reset the shared annotation.selection, annotation.locking, and
annotation.visibility state for consistency with
annotationStateManagement.browser.test.ts. Update the afterEach block near
active/ctx.cleanup to clear those module-level UID-keyed Sets after cleanup so
auto-selected annotations do not leak stale state into later tests.
In `@tests/vitest-browser/eventContracts.browser.test.ts`:
- Around line 198-232: The `secondElement` in the
`eventContracts.browser.test.ts` teardown is not covered by the suite’s
`track()` safety net, so a failure before the final cleanup can leak a DOM node
into later tests. Update the test setup around `secondElement` to register it
with `track()` alongside the other resources, using the existing teardown
helpers in this file. Then remove the manual `parentNode.removeChild` cleanup at
the end since the tracked teardown should handle it consistently.
In `@tests/vitest-browser/planarInvariants.browser.test.ts`:
- Around line 92-129: The deepRound helper duplicates the same normalization
logic already implemented as normalizeDeep in
viewStateSerialization.browser.test.ts. Keep the current local helper for now,
but if the shared harness normalization becomes available later, replace both
copies with a single imported utility so the rounding, typed-array conversion,
and undefined-key filtering stay consistent across test files.
In `@tests/vitest-browser/synchronizers.browser.test.ts`:
- Around line 673-708: The no-sync-storms test is using a hardcoded quiescence
delay that duplicates an existing timeout constant. Update the test around
createZoomPanSynchronizer and the wait-based negative-result check to either
reuse NO_PROPAGATION_WAIT_MS or introduce a clearly named QUIESCENCE_WAIT_MS
constant for this bounded wait, so the intent stays consistent and the timeout
value is defined in one place.
In `@tests/vitest-browser/toolGroupAndManipulation.browser.test.ts`:
- Around line 184-197: Guard the first viewport setup in this test so it cannot
leak if the second createPlanarViewport call fails. Update the test flow around
createPlanarViewport, activeCleanup, and the cleanup handling so that if b
creation rejects after a is created, a.cleanup() is still invoked before the
error propagates. Make the fix in the browser test’s two-viewport setup path so
RenderingEngine, DOM, and config state are always released even when the second
viewport creation throws.
In `@tests/vitest-browser/toolMeasurements.browser.test.ts`:
- Around line 34-195: This test file reimplements harness helpers and setup
logic that already exist in the shared browser test harness. Replace the local
`round2`, `distance3`, mouse dispatch helpers, `drawAnnotationByDrag`, and
`setupToolTest` usage with the shared helpers used by
`annotationToolsMatrix.browser.test.ts` (`setupTools`, `mouseDrag`,
`waitForAnnotationRendered`, and `worldDistance`). Keep the test behavior
aligned with the harness by using the same drag/wait flow and remove the
duplicated utility code so the test suite stays consistent with `harness` and
`tools.ts`.
In `@tests/vitest-browser/undoRedoHistory.browser.test.ts`:
- Around line 118-133: The setup in undoRedoHistory.browser.test.ts starts
painting after renderAndWait, but that helper only waits for IMAGE_RENDERED and
can leave the segmentation layer unresolved. Update the first paint path to wait
for the first SEGMENTATION_RENDERED event instead, using the same segmentation
render listener approach already used in the sibling segmentation suite, so the
labelmap-to-viewport resolver is ready before strokes begin.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19f53b78-3fcf-4c44-9148-523435e84e23
📒 Files selected for processing (20)
.gitignoretests/vitest-browser/annotationStateManagement.browser.test.tstests/vitest-browser/annotationToolsMatrix.browser.test.tstests/vitest-browser/eventContracts.browser.test.tstests/vitest-browser/harness/captureViewportState.tstests/vitest-browser/harness/createPlanarViewport.tstests/vitest-browser/harness/fakeImageStack.tstests/vitest-browser/harness/index.tstests/vitest-browser/harness/recordEvents.tstests/vitest-browser/harness/tools.tstests/vitest-browser/lifecycleAndRaces.browser.test.tstests/vitest-browser/planarInvariants.browser.test.tstests/vitest-browser/renderPathParity.browser.test.tstests/vitest-browser/segmentationState.browser.test.tstests/vitest-browser/synchronizers.browser.test.tstests/vitest-browser/toolGroupAndManipulation.browser.test.tstests/vitest-browser/toolMeasurements.browser.test.tstests/vitest-browser/undoRedoHistory.browser.test.tstests/vitest-browser/viewStateSerialization.browser.test.tsvitest.browser.config.ts
| init(); | ||
|
|
||
| const renderingConfig = getConfiguration().rendering; | ||
| const previousUseGenericViewport = renderingConfig.useGenericViewport; | ||
| renderingConfig.useGenericViewport = true; | ||
|
|
||
| const isVolumeBacked = | ||
| renderMode === 'vtkVolumeSlice' || renderMode === 'cpuVolume'; | ||
| const isCpu = renderMode === 'cpuImage' || renderMode === 'cpuVolume'; | ||
|
|
||
| if (isVolumeBacked) { | ||
| ensureFakeVolumeLoaderRegistered(); | ||
| } | ||
|
|
||
| const fakeStack = registerFakeImageStack(opts.stack); | ||
| const { imageIds } = fakeStack; | ||
|
|
||
| const renderingEngine = new RenderingEngine(renderingEngineId); | ||
| const element = document.createElement('div'); | ||
| element.dataset.testid = 'planar-harness-viewport'; | ||
| element.style.width = `${width}px`; | ||
| element.style.height = `${height}px`; | ||
| document.body.appendChild(element); | ||
|
|
||
| renderingEngine.enableElement({ | ||
| viewportId, | ||
| type: ViewportType.PLANAR_NEXT, | ||
| element, | ||
| defaultOptions: { | ||
| orientation, | ||
| }, | ||
| }); | ||
|
|
||
| const viewport = renderingEngine.getViewport<PlanarViewport>(viewportId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
useGenericViewport config leaks if setup fails before cleanup is defined.
renderingConfig.useGenericViewport is set to true at line 124, but the cleanup function (which restores it) isn't defined until line 156. If new RenderingEngine(), enableElement(), or any intervening call throws, the config is never restored — all subsequent tests in the run would use the wrong viewport type, producing cascading failures that are hard to attribute.
Wrap the setup section in a try/catch that restores the config and cleans up partially created resources (rendering engine, DOM element, fake stack).
🔧 Proposed fix
init();
const renderingConfig = getConfiguration().rendering;
const previousUseGenericViewport = renderingConfig.useGenericViewport;
renderingConfig.useGenericViewport = true;
+ let earlyFakeStack: ReturnType<typeof registerFakeImageStack> | undefined;
+ let earlyEngine: RenderingEngine | undefined;
+ let earlyElement: HTMLDivElement | undefined;
+
+ try {
const isVolumeBacked =
renderMode === 'vtkVolumeSlice' || renderMode === 'cpuVolume';
const isCpu = renderMode === 'cpuImage' || renderMode === 'cpuVolume';
if (isVolumeBacked) {
ensureFakeVolumeLoaderRegistered();
}
- const fakeStack = registerFakeImageStack(opts.stack);
+ earlyFakeStack = registerFakeImageStack(opts.stack);
+ const fakeStack = earlyFakeStack;
const { imageIds } = fakeStack;
- const renderingEngine = new RenderingEngine(renderingEngineId);
- const element = document.createElement('div');
+ earlyEngine = new RenderingEngine(renderingEngineId);
+ earlyElement = document.createElement('div');
+ const renderingEngine = earlyEngine;
+ const element = earlyElement;
element.dataset.testid = 'planar-harness-viewport';
element.style.width = `${width}px`;
element.style.height = `${height}px`;
document.body.appendChild(element);
renderingEngine.enableElement({
viewportId,
type: ViewportType.PLANAR_NEXT,
element,
defaultOptions: {
orientation,
},
});
const viewport = renderingEngine.getViewport<PlanarViewport>(viewportId);
+ } catch (error) {
+ if (previousUseGenericViewport !== undefined) {
+ getConfiguration().rendering.useGenericViewport =
+ previousUseGenericViewport;
+ }
+ earlyFakeStack?.unregister();
+ earlyEngine?.destroy();
+ if (earlyElement?.parentNode) {
+ earlyElement.parentNode.removeChild(earlyElement);
+ }
+ throw error;
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/vitest-browser/harness/createPlanarViewport.ts` around lines 120 - 153,
The setup in createPlanarViewport leaves renderingConfig.useGenericViewport set
to true before cleanup is available, so any throw from RenderingEngine,
enableElement, or nearby setup can leak state into later tests. Move the
restoration logic into a try/finally (or equivalent guarded setup) around the
initialization path so useGenericViewport is always reset, and make sure
partially created resources like the rendering engine, DOM element, and fake
stack are cleaned up even when setup fails. Keep the fix localized to
createPlanarViewport and its cleanup handling.
| if (type === 'imagePlaneModule') { | ||
| return { | ||
| rows, | ||
| columns, | ||
| width: rows, | ||
| height: columns, | ||
| imageOrientationPatient: [1, 0, 0, 0, 1, 0], | ||
| rowCosines: [1, 0, 0], | ||
| columnCosines: [0, 1, 0], | ||
| imagePositionPatient: [0, 0, sliceIndex], | ||
| pixelSpacing: [xSpacing, ySpacing], | ||
| rowPixelSpacing: ySpacing, | ||
| columnPixelSpacing: xSpacing, | ||
| frameOfReferenceUID, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
imagePlaneModule has swapped width/height and reversed pixelSpacing order.
width: rows and height: columns are swapped — should be width: columns and height: rows, matching the IImage object at lines 124–125. Additionally, pixelSpacing: [xSpacing, ySpacing] is reversed from the DICOM convention [rowSpacing, columnSpacing] = [ySpacing, xSpacing] (the individual rowPixelSpacing/columnPixelSpacing at lines 193–194 are correct).
Both are currently masked by the 64×64 default and 1×1 uniform spacing, but would produce incorrect geometry for any test using non-square images or non-uniform spacing.
🔧 Proposed fix
if (type === 'imagePlaneModule') {
return {
rows,
columns,
- width: rows,
- height: columns,
+ width: columns,
+ height: rows,
imageOrientationPatient: [1, 0, 0, 0, 1, 0],
rowCosines: [1, 0, 0],
columnCosines: [0, 1, 0],
imagePositionPatient: [0, 0, sliceIndex],
- pixelSpacing: [xSpacing, ySpacing],
+ pixelSpacing: [ySpacing, xSpacing],
rowPixelSpacing: ySpacing,
columnPixelSpacing: xSpacing,
frameOfReferenceUID,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (type === 'imagePlaneModule') { | |
| return { | |
| rows, | |
| columns, | |
| width: rows, | |
| height: columns, | |
| imageOrientationPatient: [1, 0, 0, 0, 1, 0], | |
| rowCosines: [1, 0, 0], | |
| columnCosines: [0, 1, 0], | |
| imagePositionPatient: [0, 0, sliceIndex], | |
| pixelSpacing: [xSpacing, ySpacing], | |
| rowPixelSpacing: ySpacing, | |
| columnPixelSpacing: xSpacing, | |
| frameOfReferenceUID, | |
| }; | |
| } | |
| if (type === 'imagePlaneModule') { | |
| return { | |
| rows, | |
| columns, | |
| width: columns, | |
| height: rows, | |
| imageOrientationPatient: [1, 0, 0, 0, 1, 0], | |
| rowCosines: [1, 0, 0], | |
| columnCosines: [0, 1, 0], | |
| imagePositionPatient: [0, 0, sliceIndex], | |
| pixelSpacing: [ySpacing, xSpacing], | |
| rowPixelSpacing: ySpacing, | |
| columnPixelSpacing: xSpacing, | |
| frameOfReferenceUID, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/vitest-browser/harness/fakeImageStack.ts` around lines 181 - 196, The
imagePlaneModule shape in fakeImageStack is using the wrong geometry mapping:
swap width/height to match the IImage object, and fix pixelSpacing to follow
DICOM row/column ordering. Update the imagePlaneModule return in fakeImageStack
so width uses columns, height uses rows, and pixelSpacing matches
rowPixelSpacing/columnPixelSpacing semantics (with rowPixelSpacing and
columnPixelSpacing left consistent with the existing values).
The WADOURI examples fetch every image over HTTP from raw.githubusercontent.com (most from the cornerstone3D repo, the TG18 set from the external OHIF/viewer-testdata repo). Under the parallel Playwright workers on the self-hosted runner, GitHub raw intermittently rate-limits (429) or drops these requests. The DICOM loader issues a single XHR per image with no retry, so one bad response fails the whole load and surfaces as a flaky waitForImageRendered timeout (reproduced locally on TG18-LN-1k-01). Add a test-side retryRemoteFixtures helper that intercepts those requests and retries them with exponential backoff from the Node side via route.fetch, then replays the successful response to the browser. Retries stay off the browser's single-shot XHR, product code is untouched, and the example's public URLs are unchanged so the deployed docs demo is unaffected. Also give the remote-load render gate a 60s budget to absorb the retry backoff on the large 1k/2k images. Verified: TG18-LN-1k-01 (legacy + generic), previously a hard failure, now passes 3/3 repeats locally. Claude-Session: https://claude.ai/code/session_01NJNAWVWWtguh3aA8w1R9mp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/utils/retryRemoteFixtures.ts (1)
68-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd jitter to exponential backoff to avoid thundering herd.
Parallel Playwright workers hitting the same rate-limited endpoint will retry in lockstep with deterministic backoff (500, 1000, 2000ms), amplifying the rate-limiting the helper is trying to mitigate. Adding random jitter spreads retries across time.
♻️ Proposed fix: add ±25% jitter
if (attempt < attempts - 1) { - const backoffMs = 500 * 2 ** attempt; + const baseMs = 500 * 2 ** attempt; + const jitter = baseMs * 0.25 * (Math.random() * 2 - 1); + const backoffMs = Math.round(baseMs + jitter); await new Promise((resolve) => setTimeout(resolve, backoffMs)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils/retryRemoteFixtures.ts` around lines 68 - 71, The retryRemoteFixtures backoff in the retry loop is fully deterministic, so parallel Playwright workers will retry in sync and keep hammering the same endpoint. Update the backoff calculation in the retry helper (the attempt/backoffMs logic inside the retry loop) to add random ±25% jitter around the exponential delay before the setTimeout call, while preserving the existing retry structure and attempt-based scaling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/utils/retryRemoteFixtures.ts`:
- Around line 38-42: The retry fixture defaults in retryRemoteFixtures are
allowing a worst-case duration that exceeds the 60s render gate, so adjust the
defaults in the options destructuring and related helper usage to keep the route
handler under the render timeout. Update the perAttemptTimeoutMs default in
retryRemoteFixtures (and any linked call sites/specs that rely on it) so the
total retry window stays below waitForImageRendered, or add a clear
comment/docstring in retryRemoteFixtures explaining the required relationship
between attempts, perAttemptTimeoutMs, and the 60s timeout.
---
Nitpick comments:
In `@tests/utils/retryRemoteFixtures.ts`:
- Around line 68-71: The retryRemoteFixtures backoff in the retry loop is fully
deterministic, so parallel Playwright workers will retry in sync and keep
hammering the same endpoint. Update the backoff calculation in the retry helper
(the attempt/backoffMs logic inside the retry loop) to add random ±25% jitter
around the exponential delay before the setTimeout call, while preserving the
existing retry structure and attempt-based scaling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8da020c-2fd3-47b8-9f4f-be7ec3362bb0
📒 Files selected for processing (4)
tests/dicomImageLoaderWADOURI.spec.tstests/genericViewport/genericDicomImageLoaderWADOURI.spec.tstests/utils/index.tstests/utils/retryRemoteFixtures.ts
✅ Files skipped from review due to trivial changes (1)
- tests/utils/index.ts
| const { | ||
| attempts = 4, | ||
| hosts = DEFAULT_HOSTS, | ||
| perAttemptTimeoutMs = 20000, | ||
| } = options; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Worst-case retry duration (83.5s) exceeds the 60s render timeout in both spec files.
With defaults (4 attempts × 20s timeout + 500+1000+2000ms backoff = 83.5s), if every attempt times out, the route handler alone runs 23.5s past the waitForImageRendered 60s gate. The browser's XHR is blocked inside the route handler until fulfill/continue, so the render timeout fires while retries are still in-flight, producing a confusing timeout failure rather than a clear network error.
Consider reducing perAttemptTimeoutMs to ~12s (worst case 51.5s < 60s) or documenting the relationship between these two values.
💡 Suggested fix: reduce default per-attempt timeout
/** Per-attempt fetch timeout in ms. Default 20000. */
- perAttemptTimeoutMs?: number;
+ perAttemptTimeoutMs?: number; const {
attempts = 4,
hosts = DEFAULT_HOSTS,
- perAttemptTimeoutMs = 20000,
+ perAttemptTimeoutMs = 12000,
} = options;Also applies to: 68-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/utils/retryRemoteFixtures.ts` around lines 38 - 42, The retry fixture
defaults in retryRemoteFixtures are allowing a worst-case duration that exceeds
the 60s render gate, so adjust the defaults in the options destructuring and
related helper usage to keep the route handler under the render timeout. Update
the perAttemptTimeoutMs default in retryRemoteFixtures (and any linked call
sites/specs that rely on it) so the total retry window stays below
waitForImageRendered, or add a clear comment/docstring in retryRemoteFixtures
explaining the required relationship between attempts, perAttemptTimeoutMs, and
the 60s timeout.
1 participant
What
Adds 12 state-based (non-screenshot) integration test suites running in Vitest Browser Mode (real headless Chromium + WebGL,
pnpm test:vitest:browser), plus a shared harness undertests/vitest-browser/harness/:10 + sliceIndex, bar255at columns 20-25, 1 mm spacing) so every expected value is closed-formAll assertions target public API state, events, and exact expected values. No pixel comparisons.
Core viewport suites
renderPathParityplanarInvariantseventContractslifecycleAndRacesviewStateSerializationtoolMeasurementsTools suites (black-box: public exports, synthesized input, observable state only)
annotationToolsMatrixtoolGroupAndManipulationannotationStateManagementsegmentationStateundoRedoHistorysynchronizersResult
100 passed, 12 expected-fail, in ~90 s. Each
test.failspins a real engine finding with an in-file comment (observed vs expected), so fixing the underlying bug flips the test to green and fails the build until the marker is removed.Findings pinned by
test.failsinitialImageIdIndex: 0, whilegetSliceIndex()reports 0 (createInitialVolumeSliceState)PlanarViewporthas nogetCamera(), which breaksCircleROIToolrendering (uncaught RAF error) and silently disables the ImageSlice synchronizer (async throw swallowed byPromise.allSettled)invertStateChangedis never set); CameraPosition synchronizer drops rotationsetDisplaySetPresentation/setViewStateemitVOI_MODIFIED/CAMERA_MODIFIEDwithout no-op detectiongetViewReference()numbers are not byte-stable through JSON (float32 promotion + trig round-off)resetDisplaySetPresentationresets to{}rather than pre-mutation defaultsSEGMENTATION_REPRESENTATION_ADDEDis declared and listened for but never firedgetUniqueSegmentIndicesis metadata-driven and reports stale indices after undo/eraseOther contracts documented in-file
Annotation JSON persistence silently loses selection/locking/visibility (separate stores + preprocessing hook); activating a second tool on a binding does not demote the first and dispatch resolves by first-inserted tool;
cancelActiveManipulationscompletes rather than deletes the in-progress annotation; labelmap-on-generic-stack setup requires awaiting the firstSEGMENTATION_RENDEREDbefore painting.Notes
vitest.browser.config.ts: adds a@cornerstonejs/toolssource alias (tools now testable in vitest browser mode)..gitignore: vitest browser-mode failure artifacts (.vitest-attachments/,tests/vitest-browser/__screenshots__/).genericStackApi.browser.test.tsscreenshot test is environment-sensitive (software-rasterizer baselines); it is untouched here and unrelated to these suites.https://claude.ai/code/session_01NJNAWVWWtguh3aA8w1R9mp
Summary by CodeRabbit
New Features
Bug Fixes
Chores