Tests/labelmap color change tests#6054
Conversation
Moves the labelmap segmentation color-change E2E test and its screenshots onto a dedicated branch.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR adds comprehensive test infrastructure for the labelmap segmentation color-picker dialog. UI components receive ChangesLabelmap Segmentation Color Change Test Suite
Sequence Diagram(s)sequenceDiagram
participant Test
participant RightPanel
participant Dialog
participant DOMOverlay
Test->>RightPanel: openChangeColor()
RightPanel->>Dialog: click "Change Color" action
Test->>DOMOverlay: colorPicker.fillHex(hex)
DOMOverlay->>Dialog: fill hex input + press Enter
Test->>DOMOverlay: colorPicker.save()
DOMOverlay->>Dialog: click save button
Test->>RightPanel: rowDataColorHex
RightPanel->>Test: return color indicator CSS value
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| get colorPicker() { | ||
| const locator = page.getByTestId('color-picker-dialog'); | ||
| const hexInput = locator.getByLabel('hex'); |
There was a problem hiding this comment.
getByLabel('hex') may not resolve to the ChromePicker input
react-color's ChromePicker uses a <span> as a visual label below the input — it is not a <label> element and carries no for/aria-labelledby association. Playwright's getByLabel relies on the accessibility tree; without a proper label association, this locator will either return no element or throw at runtime. A more reliable selector would be something like locator.locator('input[id*="hex"], input[aria-label="hex"]') or querying by the label-companion pattern that ChromePicker actually renders.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/pages/DOMOverlayPageObject.ts
Line: 65
Comment:
**`getByLabel('hex')` may not resolve to the ChromePicker input**
`react-color`'s `ChromePicker` uses a `<span>` as a visual label below the input — it is not a `<label>` element and carries no `for`/`aria-labelledby` association. Playwright's `getByLabel` relies on the accessibility tree; without a proper label association, this locator will either return no element or throw at runtime. A more reliable selector would be something like `locator.locator('input[id*="hex"], input[aria-label="hex"]')` or querying by the label-companion pattern that ChromePicker actually renders.
How can I resolve this? If you propose a fix, please make it concise.| await expect(DOMOverlayPageObject.dialog.colorPicker.hexInput).toHaveValue( | ||
| LABELMAP_SEGMENT_0_DEFAULT_HEX | ||
| ); |
There was a problem hiding this comment.
# prefix mismatch
ChromePicker typically stores and displays the hex value as the 6-character string without the # (e.g. 9D6CA2), while the # glyph is rendered as a static prefix outside the <input>. If that's the case here, toHaveValue('#9D6CA2') will always fail. The same concern applies to hexInput.fill('#FF00FF') in fillHexAndSave / fillHexAndCancel — passing the # prefix into an input that doesn't expect it may silently produce an unexpected color state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/LabelMapSegmentationColorChange.spec.ts
Line: 44-46
Comment:
**Hex value assertion may fail due to `#` prefix mismatch**
`ChromePicker` typically stores and displays the hex value as the 6-character string without the `#` (e.g. `9D6CA2`), while the `#` glyph is rendered as a static prefix outside the `<input>`. If that's the case here, `toHaveValue('#9D6CA2')` will always fail. The same concern applies to `hexInput.fill('#FF00FF')` in `fillHexAndSave` / `fillHexAndCancel` — passing the `#` prefix into an input that doesn't expect it may silently produce an unexpected color state.
How can I resolve this? If you propose a fix, please make it concise.| const colorChangeCycle = waitForViewportRenderCycle(page); | ||
| await segment.actions.changeColor(NEW_LABELMAP_SEGMENT_HEX); | ||
| await colorChangeCycle; |
There was a problem hiding this comment.
waitForViewportRenderCycle(page) is called before segment.actions.changeColor(...), which itself performs four sequential interactions (open menu → click "Change Color" → fill hex → click Save). If the viewport renders for any intermediate reason during those steps — such as the color picker opening or focus events — the promise resolves early and await colorChangeCycle becomes a no-op. The screenshot that follows may then be captured before the actual color-change render completes. Capturing the promise after changeColor resolves would remove this race.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/LabelMapSegmentationColorChange.spec.ts
Line: 72-74
Comment:
**Render-cycle promise captured before the action that triggers the render**
`waitForViewportRenderCycle(page)` is called before `segment.actions.changeColor(...)`, which itself performs four sequential interactions (open menu → click "Change Color" → fill hex → click Save). If the viewport renders for any intermediate reason during those steps — such as the color picker opening or focus events — the promise resolves early and `await colorChangeCycle` becomes a no-op. The screenshot that follows may then be captured before the actual color-change render completes. Capturing the promise _after_ `changeColor` resolves would remove this race.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/pages/DOMOverlayPageObject.ts`:
- Around line 64-65: The hex input selection and assertions are brittle: replace
locator.getByLabel('hex') with a scoped query for the actual input element
inside the color picker (e.g., use the locator from
getByTestId('color-picker-dialog') and then find the input element via
locator.locator('input') or getByRole('textbox') scoped to that dialog) so you
reliably target the ChromePicker EditableInput, and when checking its value
normalize case and/or perform a case-insensitive match and ensure the string
contains the leading '#' (e.g., compare lowercased value or use a regex like
/^`#/i`) instead of asserting a specific uppercase hex string.
🪄 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: c3454003-0f70-4bf6-a421-d5920f8a6aef
⛔ Files ignored due to path filters (4)
tests/screenshots/chromium/LabelMapSegmentationColorChange.spec.ts/labelMapSegmentationColor-afterCancel.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegmentationColorChange.spec.ts/labelMapSegmentationColor-afterChange.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegmentationColorChange.spec.ts/labelMapSegmentationColor-beforeCancel.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegmentationColorChange.spec.ts/labelMapSegmentationColor-beforeChange.pngis excluded by!**/*.png
📒 Files selected for processing (6)
extensions/default/src/utils/colorPickerDialog.tsxplatform/ui-next/src/components/DataRow/DataRow.tsxtests/LabelMapSegmentationColorChange.spec.tstests/pages/DOMOverlayPageObject.tstests/pages/RightPanelPageObject.tstests/utils/screenShotPaths.ts
| const locator = page.getByTestId('color-picker-dialog'); | ||
| const hexInput = locator.getByLabel('hex'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In react-color 2.19.3, does the ChromePicker "hex" EditableInput render a <label> associated with its <input> (so a by-label query matches it), and is the displayed value uppercased and prefixed with "#"?
💡 Result:
Answer: No. In react-color 2.19.3, the ChromePicker “hex” field uses EditableInput that renders an without an associated (so a by-label query for the label text should not match the input). The displayed hex value is rendered as a plain hex string coming from tinycolor’s toHexString, which includes the “#” prefix but does not guarantee uppercase (it reflects tinycolor’s output/casing for the given color). Evidence: 1) The EditableInput implementation in react-color renders a wrapper
Citations:
Fix hexInput locator/value expectations for react-color ChromePicker
locator.getByLabel('hex') is unreliable with react-color 2.19.3: the ChromePicker “hex” EditableInput renders an <input> without a label/htmlFor association, so getByLabel('hex') may not resolve. The displayed value comes from tinycolor(...).toHexString (includes #), but casing isn’t guaranteed—avoid asserting a specific uppercase hex string (use a case-insensitive match or normalize case).
🤖 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/pages/DOMOverlayPageObject.ts` around lines 64 - 65, The hex input
selection and assertions are brittle: replace locator.getByLabel('hex') with a
scoped query for the actual input element inside the color picker (e.g., use the
locator from getByTestId('color-picker-dialog') and then find the input element
via locator.locator('input') or getByRole('textbox') scoped to that dialog) so
you reliably target the ChromePicker EditableInput, and when checking its value
normalize case and/or perform a case-insensitive match and ensure the string
contains the leading '#' (e.g., compare lowercased value or use a regex like
/^`#/i`) instead of asserting a specific uppercase hex string.
1 participant
Context
Add coverage for changing the color of labelmaps
Changes & Results
tests/utils/screenShotPaths.ts— registered newlabelMapSegmentationColorChangebaseline keys.tests/LabelMapSegmentationColorChange.spec.ts— three testscovering: opening the dialog with the current hex prefilled, saving
a new color, and cancelling the dialog. Verification uses
checkForScreenshotscoped to the default viewport for thecanvas-side check; four baseline screenshots are added under
tests/screenshots/chromium/LabelMapSegmentationColorChange.spec.ts/.Testing
From the repo root, run both new specs:
yarn playwright test tests/LabelMapSegmentationColorChange.spec.tsTo regenerate the labelmap baselines after an intentional rendering
change:
yarn playwright test tests/LabelMapSegmentationColorChange.spec.ts --update-snapshotsChecklist
PR
semantic-release format and guidelines.
Code
Public Documentation Updates
Tested Environment
Summary by CodeRabbit
Greptile Summary
This PR adds Playwright end-to-end test coverage for the labelmap segmentation color-change workflow, wiring up three tests (dialog opens with prefilled hex, save changes the color, cancel does not). Supporting changes add
data-cyattributes toColorPickerDialogand theDataRowcolor swatch, and extend theRightPanelPageObject/DOMOverlayPageObjectwith color-picker helpers.data-cyattributes are added to the dialog wrapper and the Save/Cancel buttons incolorPickerDialog.tsx, and to the color swatch span inDataRow.tsx.Confidence Score: 3/5
The production code changes (data-cy additions) are safe, but the new test infrastructure has three areas that may make the tests unreliable or always-failing on CI without further verification.
The hexInput locator uses getByLabel('hex') against ChromePicker, which does not expose a proper accessibility label — the locator may silently return no element. The hex value assertions and fill calls pass a '#'-prefixed string that ChromePicker's raw input may not accept or return. Finally, waitForViewportRenderCycle is captured before the multi-step changeColor action, so an intermediate render during the action sequence could let the screenshot race ahead of the actual color-change render.
tests/pages/DOMOverlayPageObject.ts (hexInput locator) and tests/LabelMapSegmentationColorChange.spec.ts (hex value format, render-cycle timing)
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Merge branch 'master' into feat/labelmap..." | Re-trigger Greptile