ohifohifupdateplaywightimgaes#6043
Open
sedghi wants to merge 16 commits into
Open
Conversation
…ered in multiple specs for improved rendering stability
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Viewers
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
ohifohifupdateplaywightimgaes
|
| Run status |
|
| Run duration | 02m 21s |
| Commit |
|
| Committer | Alireza |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
2
|
 |
0
|
 |
0
|
 |
0
|
 |
26
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
| Test | Artifacts | |
|---|---|---|
| OHIF HP > Should navigate next/previous stage |
Test Replay
Screenshots
Video
|
|
| Test | Artifacts | |
|---|---|---|
| OHIF Double Click > Should double click each viewport to one up and back |
Test Replay
Screenshots
Video
|
|
Contributor
There was a problem hiding this comment.
Can you switch this to viewports only compare?
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpgrades Jest and Babel test config, modernizes Jest matchers, adds paint-settle test utilities and integrates them into screenshot tests, applies small viewport/DOM fixes, loosens some visual-test assertions, and adds a local screenshot-review server + Alpine UI. ChangesTesting Infrastructure & Compatibility
Screenshot Reviewer Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 (1)
addOns/externals/devDependencies/package.json (1)
55-58: Confirm Jest/jest-junit upgrade versions are published and have no known GitHub-advisory vulnerabilities.
jest@30.2.0,jest-environment-jsdom@30.2.0, andjest-junit@16.0.0are present on npm.- GitHub security advisory lookup returned no known vulnerabilities for
jestorjest-junit.Jest is a major bump (29.x → 30.x) and may still require reviewing the Jest 30 upgrade notes and running the test suite.
🤖 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 `@addOns/externals/devDependencies/package.json` around lines 55 - 58, Confirm the new Jest-related versions are safe and compatible: verify that "jest@30.2.0", "jest-environment-jsdom@30.2.0", and "jest-junit@16.0.0" are published to npm, run a GitHub Advisory and npm audit check for any vulnerabilities, update the lockfile (package-lock.json or yarn.lock) by running npm ci/install to pin these versions, run the full test suite and address any Jest 30 compatibility issues (refer to jest config and test files), and if failures occur, revert to a compatible minor version or apply the necessary Jest 30 migration changes in test setup/configuration.
🤖 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 `@scripts/screenshot-reviewer.html`:
- Around line 379-508: The x-text binding in the notes footer uses selected.name
which throws when selected is null; update the binding to use optional chaining
(selected?.name) so the expression is safe when the element is hidden via
x-show; locate the notes footer block (the element with <span class="count"
x-show="selected" x-text="selected.name">) and change the x-text to use
selected?.name to match other bindings like selected?.path.
In `@scripts/screenshot-reviewer.mjs`:
- Around line 138-159: handleImage currently pipes
createReadStream(absolutePath) to response without handling stream errors;
attach an 'error' listener to the stream returned by createReadStream in the
worktree branch (after resolveScreenshotPath and the version checks) to catch
file-not-found or read errors, log or include the error via your existing
logger, send an appropriate response (e.g., response.writeHead(404 or 500) and
response.end()) and ensure you remove/cleanup listeners as needed so the
response cannot hang or throw an unhandled exception when the file is
unreadable.
---
Nitpick comments:
In `@addOns/externals/devDependencies/package.json`:
- Around line 55-58: Confirm the new Jest-related versions are safe and
compatible: verify that "jest@30.2.0", "jest-environment-jsdom@30.2.0", and
"jest-junit@16.0.0" are published to npm, run a GitHub Advisory and npm audit
check for any vulnerabilities, update the lockfile (package-lock.json or
yarn.lock) by running npm ci/install to pin these versions, run the full test
suite and address any Jest 30 compatibility issues (refer to jest config and
test files), and if failures occur, revert to a compatible minor version or
apply the necessary Jest 30 migration changes in test setup/configuration.
🪄 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: c8047956-ce75-44fd-aed7-6e1aae24829c
⛔ Files ignored due to path filters (111)
bun.lockis excluded by!**/*.locktests/screenshots/chromium/3DFourUp.spec.ts/threeDFourUpDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/3DMain.spec.ts/threeDMainDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/3DOnly.spec.ts/threeDOnlyDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/3DPrimary.spec.ts/threeDPrimaryDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Angle.spec.ts/angleDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/ArrowAnnotate.spec.ts/arrowAnnotateDisplayedCorrectly0.pngis excluded by!**/*.pngtests/screenshots/chromium/ArrowAnnotate.spec.ts/arrowAnnotateDisplayedCorrectly1.pngis excluded by!**/*.pngtests/screenshots/chromium/ArrowAnnotate.spec.ts/arrowAnnotateDisplayedCorrectly2.pngis excluded by!**/*.pngtests/screenshots/chromium/AxialPrimary.spec.ts/axialPrimaryDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Bidirectional.spec.ts/bidirectionalDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Circle.spec.ts/circleDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/CobbAngle.spec.ts/cobbangleDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/ContextMenu.spec.ts/contextMenuNearBottomEdgeNotClipped.pngis excluded by!**/*.pngtests/screenshots/chromium/ContextMenu.spec.ts/preContextMenuNearBottomEdge.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/noOverlay.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/overlay2d-tta-nnU-Net-Segmentation.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/overlayMenuWith2d-tta-nnU-Net-SegmentationSelected.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/overlayMenuWithSegmentationOverlaysRemoved.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/overlayMenuWithSegmentationSelected.pngis excluded by!**/*.pngtests/screenshots/chromium/DataOverlayMenu.spec.ts/overlaySegmentation.pngis excluded by!**/*.pngtests/screenshots/chromium/DicomTagBrowser.spec.ts/dicomTagBrowserDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/DicomTagBrowser.spec.ts/scrollBarRenderedProperly.pngis excluded by!**/*.pngtests/screenshots/chromium/Ellipse.spec.ts/ellipseDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/FlipHorizontal.spec.ts/flipHorizontalDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Invert.spec.ts/invertDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-changeSeriesInMPR.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-initialDraw.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-jumpInMPR.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-jumpToMeasurementAfterSeriesChange.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-jumpToMeasurementStack.pngis excluded by!**/*.pngtests/screenshots/chromium/JumpToMeasurementMPR.spec.ts/jumpToMeasurementMPR-scrollAway.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegLocking.spec.ts/lockedSegPostEdit.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegLocking.spec.ts/lockedSegPreEdit.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegLocking.spec.ts/unlockedSegPostEdit.pngis excluded by!**/*.pngtests/screenshots/chromium/LabelMapSegLocking.spec.ts/unlockedSegPreEdit.pngis excluded by!**/*.pngtests/screenshots/chromium/Length.spec.ts/lengthDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Livewire.spec.ts/livewireDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/MPR.spec.ts/mprDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/MPRThenRTOverlayNoHydration.spec.ts/mprPostRTOverlayNoHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/MPRThenRTOverlayNoHydration.spec.ts/mprPreRTOverlayNoHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/MPRThenSEGOverlayNoHydration.spec.ts/mprPostSEGOverlayNoHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/MPRThenSEGOverlayNoHydration.spec.ts/mprPreSEGOverlayNoHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/MultipleSegmentationDataOverlays.spec.ts/overlaySEGsAndRTDisplayed.pngis excluded by!**/*.pngtests/screenshots/chromium/MultipleSegmentationDataOverlays.spec.ts/overlaysDisplayed.pngis excluded by!**/*.pngtests/screenshots/chromium/MultipleSegmentationDataOverlays.spec.ts/threeSegOverlaysInOverlayMenu.pngis excluded by!**/*.pngtests/screenshots/chromium/Probe.spec.ts/probeDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/RTDataOverlayForUnreferencedDisplaySetNoHydration.spec.ts/overlayFirstImage.pngis excluded by!**/*.pngtests/screenshots/chromium/RTDataOverlayForUnreferencedDisplaySetNoHydration.spec.ts/overlayMiddleImage.pngis excluded by!**/*.pngtests/screenshots/chromium/RTDataOverlayNoHydrationThenMPR.spec.ts/rtDataOverlayNoHydrationPostMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/RTDataOverlayNoHydrationThenMPR.spec.ts/rtDataOverlayNoHydrationPreMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydration.spec.ts/rtJumpToStructure.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydration.spec.ts/rtPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydration.spec.ts/rtPreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydration2.spec.ts/rtPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydration2.spec.ts/rtPreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationDisableConfirmation.spec.ts/firstLoadPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationDisableConfirmation.spec.ts/secondLoadPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationDisableConfirmation.spec.ts/viewportAfterFirstDelete.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationDisableConfirmation.spec.ts/viewportAfterSecondDelete.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationFromMPR.spec.ts/mprAfterRT.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationFromMPR.spec.ts/mprAfterRTHydrated.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationFromMPR.spec.ts/mprAfterRTHydratedAfterLayoutChange.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationFromMPR.spec.ts/mprBeforeRT.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationThenMPR.spec.ts/rtPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/RTHydrationThenMPR.spec.ts/rtPostHydrationMPRAxialPrimary.pngis excluded by!**/*.pngtests/screenshots/chromium/RTNoHydrationThenMPR.spec.ts/rtNoHydrationPostMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/RTNoHydrationThenMPR.spec.ts/rtNoHydrationPreMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/Rectangle.spec.ts/rectangleDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Reset.spec.ts/resetDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/RotateRight.spec.ts/rotateRightDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDataOverlayForUnreferencedDisplaySetNoHydration.spec.ts/overlayFirstImage.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDataOverlayForUnreferencedDisplaySetNoHydration.spec.ts/overlayMiddleImage.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDataOverlayNoHydrationThenMPR.spec.ts/segDataOverlayNoHydrationPostMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDataOverlayNoHydrationThenMPR.spec.ts/segDataOverlayNoHydrationPreMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDrawingToolsResizing.spec.ts/brushTool.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGDrawingToolsResizing.spec.ts/eraserTool.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydration.spec.ts/segPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydration.spec.ts/segPreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationDeleteAndReload.spec.ts/viewportAfterSecondDelete.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationDeleteAndReload.spec.ts/viewportAfterSecondHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFrom3DFourUp.spec.ts/afterSEGHydrated.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFrom3DFourUp.spec.ts/backTo3DFourUp.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFrom3DFourUp.spec.ts/threeDFourUpAfterSEG.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFrom3DFourUp.spec.ts/threeDFourUpAfterSegHydrated.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFrom3DFourUp.spec.ts/threeDFourUpBeforeSEG.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFromMPR.spec.ts/mprAfterSEG.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFromMPR.spec.ts/mprAfterSegHydrated.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFromMPR.spec.ts/mprAfterSegHydratedAfterLayoutChange.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationFromMPR.spec.ts/mprBeforeSEG.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationThenMPR.spec.ts/segPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGHydrationThenMPR.spec.ts/segPostHydrationMPRAxialPrimary.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGNoHydrationThenMPR.spec.ts/segNoHydrationPostMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/SEGNoHydrationThenMPR.spec.ts/segNoHydrationPreMpr.pngis excluded by!**/*.pngtests/screenshots/chromium/SRHydration.spec.ts/srJumpToMeasurement.pngis excluded by!**/*.pngtests/screenshots/chromium/SRHydration.spec.ts/srPostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/SRHydration.spec.ts/srPreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/Scoord3dProbe.spec.ts/scoord3dProbeDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Scoord3dProbe.spec.ts/scoord3dProbeJumpToMeasurement.pngis excluded by!**/*.pngtests/screenshots/chromium/Scoord3dProbe.spec.ts/scoord3dProbePostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/Scoord3dProbe.spec.ts/scoord3dProbePreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/ScoordRectangle.spec.ts/scoordRectangleDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/ScoordRectangle.spec.ts/scoordRectangleJumpToMeasurement.pngis excluded by!**/*.pngtests/screenshots/chromium/ScoordRectangle.spec.ts/scoordRectanglePostHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/ScoordRectangle.spec.ts/scoordRectanglePreHydration.pngis excluded by!**/*.pngtests/screenshots/chromium/Spline.spec.ts/splineDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/Worklist.spec.ts/scrollBarRenderedProperly.pngis excluded by!**/*.pngtests/screenshots/chromium/ZoomIn.spec.ts/magnifyViewportDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/mpr2.spec.ts/mprDisplayedCorrectly.pngis excluded by!**/*.pngtests/screenshots/chromium/mpr2.spec.ts/mprDisplayedCorrectlyZoomed.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
addOns/externals/devDependencies/package.jsonbabel.config.jsextensions/cornerstone-dicom-sr/src/tools/DICOMSRDisplayTool.tsextensions/cornerstone/src/utils/generateSegmentationCSVReport.test.tspackage.jsonplatform/core/src/utils/Queue.test.jsplatform/core/src/utils/absoluteUrl.test.jsplatform/core/src/utils/addServer.test.jsplatform/core/src/utils/hierarchicalListUtils.test.jsplatform/core/src/utils/progressTrackingUtils.test.jsscripts/screenshot-reviewer.htmlscripts/screenshot-reviewer.mjstestdatatests/JumpToMeasurementMPR.spec.tstests/SEGHydrationFrom3DFourUp.spec.tstests/SEGHydrationFromMPR.spec.tstests/SEGNoHydrationThenMPR.spec.tstests/SRHydration.spec.tstests/Scoord3dProbe.spec.tstests/ScoordRectangle.spec.tstests/utils/index.tstests/utils/waitForViewportsRendered.ts
Comment on lines
+379
to
+508
| <body> | ||
| <div class="app" x-data="reviewer()" x-init="init()"> | ||
| <aside class="sidebar"> | ||
| <div class="sidebar-header"> | ||
| <div class="title-row"> | ||
| <h1>Screenshot Review</h1> | ||
| <button class="ghost" @click="refresh()">Refresh</button> | ||
| </div> | ||
| <input class="search" type="search" placeholder="Filter" x-model="query" /> | ||
| <label class="check-row"> | ||
| <input type="checkbox" x-model="hideStaged" /> | ||
| <span>Hide staged</span> | ||
| </label> | ||
| <div class="count" x-text="`${filteredEntries.length} changed screenshots`"></div> | ||
| </div> | ||
| <div class="list"> | ||
| <div class="section-title" x-show="unstagedEntries.length">Unstaged</div> | ||
| <template x-for="entry in unstagedEntries" :key="entry.path"> | ||
| <button | ||
| class="item" | ||
| :class="{ active: selected?.path === entry.path }" | ||
| @click="select(entry)" | ||
| > | ||
| <div class="meta-row"> | ||
| <div class="item-name" x-text="entry.name"></div> | ||
| <span class="badge warn" x-show="entry.partiallyStaged">partial</span> | ||
| </div> | ||
| <div class="item-path" x-text="entry.spec"></div> | ||
| </button> | ||
| </template> | ||
|
|
||
| <div class="section-title" x-show="stagedEntries.length">Staged</div> | ||
| <template x-for="entry in stagedEntries" :key="entry.path"> | ||
| <button | ||
| class="item" | ||
| :class="{ active: selected?.path === entry.path }" | ||
| @click="select(entry)" | ||
| > | ||
| <div class="item-name" x-text="entry.name"></div> | ||
| <div class="item-path" x-text="entry.spec"></div> | ||
| </button> | ||
| </template> | ||
| </div> | ||
| </aside> | ||
|
|
||
| <main class="main"> | ||
| <header class="topbar"> | ||
| <div class="toolbar"> | ||
| <div class="segmented"> | ||
| <button :class="{ active: mode === 'side' }" @click="mode = 'side'">Side by side</button> | ||
| <button :class="{ active: mode === 'slider' }" @click="mode = 'slider'">Slider</button> | ||
| <button :class="{ active: mode === 'blink' }" @click="mode = 'blink'">Blink</button> | ||
| </div> | ||
| <template x-if="mode === 'blink'"> | ||
| <label class="check-row"> | ||
| <span>Seconds</span> | ||
| <input class="blink-range" type="range" min="0.15" max="3" step="0.05" x-model.number="blinkSeconds" /> | ||
| <span x-text="blinkSeconds.toFixed(2)"></span> | ||
| </label> | ||
| </template> | ||
| <div class="spacer"></div> | ||
| <button class="primary" :disabled="!selected || selected.staged" @click="stageSelected()">git add</button> | ||
| <button @click="copyCurrentNote()" :disabled="!selected">Copy note</button> | ||
| <button @click="copyAllNotes()">Copy all notes</button> | ||
| </div> | ||
| <div class="path" x-text="selected?.path || 'No screenshot selected'"></div> | ||
| <div class="status" :class="{ error: statusIsError }" x-text="status"></div> | ||
| </header> | ||
|
|
||
| <section class="viewer" x-show="selected"> | ||
| <template x-if="mode === 'side'"> | ||
| <div class="side-grid"> | ||
| <div class="frame"> | ||
| <div class="frame-label">Before</div> | ||
| <div class="image-wrap"> | ||
| <img :src="beforeUrl()" alt="Before screenshot" /> | ||
| </div> | ||
| </div> | ||
| <div class="frame"> | ||
| <div class="frame-label">After</div> | ||
| <div class="image-wrap"> | ||
| <img :src="afterUrl()" alt="After screenshot" /> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </template> | ||
|
|
||
| <template x-if="mode === 'slider'"> | ||
| <div class="compare"> | ||
| <div class="slider-stage" x-ref="sliderStage" :style="`--image-width: ${sliderWidth}px`"> | ||
| <img :src="beforeUrl()" alt="Before screenshot" @load="syncSliderWidth()" /> | ||
| <div class="after" :style="`width: ${slider}%`"> | ||
| <img :src="afterUrl()" alt="After screenshot" /> | ||
| </div> | ||
| </div> | ||
| <input class="slider-range" type="range" min="0" max="100" x-model.number="slider" /> | ||
| </div> | ||
| </template> | ||
|
|
||
| <template x-if="mode === 'blink'"> | ||
| <div class="compare"> | ||
| <div class="blink-stage"> | ||
| <img :src="blinkShowAfter ? afterUrl() : beforeUrl()" alt="Blink screenshot" /> | ||
| </div> | ||
| <div class="meta-row"> | ||
| <span class="badge" x-text="blinkShowAfter ? 'after' : 'before'"></span> | ||
| <button @click="blinkPaused = !blinkPaused" x-text="blinkPaused ? 'Resume' : 'Pause'"></button> | ||
| </div> | ||
| </div> | ||
| </template> | ||
| </section> | ||
|
|
||
| <section class="empty" x-show="!selected"> | ||
| <div>No changed screenshots found.</div> | ||
| </section> | ||
|
|
||
| <footer class="notes"> | ||
| <div class="meta-row"> | ||
| <strong>Notes</strong> | ||
| <span class="count" x-show="selected" x-text="selected.name"></span> | ||
| </div> | ||
| <textarea | ||
| :disabled="!selected" | ||
| x-model="currentNote" | ||
| @input="saveNote()" | ||
| placeholder="Review note" | ||
| ></textarea> | ||
| </footer> | ||
| </main> | ||
| </div> |
There was a problem hiding this comment.
Missing optional chaining on selected.name.
On line 498, x-text="selected.name" will throw a TypeError when selected is null. Unlike x-if, x-show only toggles visibility—the element remains in DOM and the expression is still evaluated. Other bindings in this file correctly use optional chaining (e.g., line 399: selected?.path).
🐛 Proposed fix
- <span class="count" x-show="selected" x-text="selected.name"></span>
+ <span class="count" x-show="selected" x-text="selected?.name"></span>🤖 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 `@scripts/screenshot-reviewer.html` around lines 379 - 508, The x-text binding
in the notes footer uses selected.name which throws when selected is null;
update the binding to use optional chaining (selected?.name) so the expression
is safe when the element is hidden via x-show; locate the notes footer block
(the element with <span class="count" x-show="selected" x-text="selected.name">)
and change the x-text to use selected?.name to match other bindings like
selected?.path.
Comment on lines
+138
to
+159
| async function handleImage(requestUrl, response) { | ||
| const path = requestUrl.searchParams.get('path'); | ||
| const version = requestUrl.searchParams.get('version'); | ||
| const { absolutePath, relativePath } = resolveScreenshotPath(path); | ||
|
|
||
| response.writeHead(200, { | ||
| 'content-type': 'image/png', | ||
| 'cache-control': 'no-store', | ||
| }); | ||
|
|
||
| if (version === 'head') { | ||
| const buffer = await runGit(['show', `HEAD:${relativePath}`]); | ||
| response.end(buffer); | ||
| return; | ||
| } | ||
|
|
||
| if (version !== 'worktree') { | ||
| throw new Error('Invalid image version'); | ||
| } | ||
|
|
||
| createReadStream(absolutePath).pipe(response); | ||
| } |
There was a problem hiding this comment.
Missing error handler on read stream.
If the worktree file doesn't exist or is unreadable, createReadStream emits an 'error' event that's not caught. This bypasses the try/catch wrapper (stream errors are async) and could cause an unhandled exception or leave the response hanging.
🛡️ Proposed fix to handle stream errors
- createReadStream(absolutePath).pipe(response);
+ const stream = createReadStream(absolutePath);
+ stream.on('error', (err) => {
+ if (!response.headersSent) {
+ sendJson(response, 500, { error: err.message });
+ } else {
+ response.destroy(err);
+ }
+ });
+ stream.pipe(response);🤖 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 `@scripts/screenshot-reviewer.mjs` around lines 138 - 159, handleImage
currently pipes createReadStream(absolutePath) to response without handling
stream errors; attach an 'error' listener to the stream returned by
createReadStream in the worktree branch (after resolveScreenshotPath and the
version checks) to catch file-not-found or read errors, log or include the error
via your existing logger, send an appropriate response (e.g.,
response.writeHead(404 or 500) and response.end()) and ensure you remove/cleanup
listeners as needed so the response cannot hang or throw an unhandled exception
when the file is unreadable.
|
Actionable comments posted: 0 |
wayfarer3130
approved these changes
May 28, 2026
Contributor
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
Lets wait till we can include an update to cs3d build versions before merging, but the changes look good.
|
Actionable comments posted: 0 |
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores