fix: New growcut (fill) implementation#5954
Open
wayfarer3130 wants to merge 12 commits into
Open
Conversation
wayfarer3130
added
the
ohif-integration
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a cross-platform CI utility script ( ChangesPort-freeing CI Utility and Playwright Integration
RegionSegmentPlus Flood-Fill Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)extensions/cornerstone/src/commandsModule.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modes/segmentation/src/toolbarButtons.ts (1)
766-822:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the flood-fill tool name explicitly when activating this button.
This button still uses
id: 'RegionSegmentPlus', butsetToolActiveToolbarresolves the target fromtoolName || itemId || value(extensions/cornerstone/src/commandsModule.ts, Line 1129). With the updated registration/evaluate path now usingRegionSegmentPlusFloodFill, clicking the button can no-op because it still activates the legacy id instead of the registered tool name.Suggested fix
commands: [ - 'setToolActiveToolbar', + { + commandName: 'setToolActiveToolbar', + commandOptions: { + toolName: 'RegionSegmentPlusFloodFill', + }, + }, 'setRegionSegmentPlusFloodFillConfiguration', { commandName: 'activateSelectedSegmentationOfType',🤖 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 `@modes/segmentation/src/toolbarButtons.ts` around lines 766 - 822, The toolbar button currently uses id 'RegionSegmentPlus' but setToolActiveToolbar resolves targets from toolName || itemId || value so it may activate the wrong legacy id; update the commands for this button (the commands array that currently includes 'setToolActiveToolbar') to pass the flood-fill tool name explicitly (e.g., include a command entry invoking setToolActiveToolbar with toolName: 'RegionSegmentPlusFloodFill' or set the value to 'RegionSegmentPlusFloodFill') so activation targets the registered tool; locate the button definition (id: 'RegionSegmentPlus') in modes/segmentation/src/toolbarButtons.ts and modify the commands element to supply the explicit tool name when calling setToolActiveToolbar.
🧹 Nitpick comments (1)
playwright.config.ts (1)
3-4: 💤 Low valueConsider validating
OHIF_PORTto match the utility script's validation.The port-freeing utility (
free-ohif-e2e-port.mjs) validates thatOHIF_PORTis an integer in range 1-65535 and throws a descriptive error if invalid. This config file usesNumber(process.env.OHIF_PORT || 3335)without validation, which would produceNaNfor non-numeric values like"abc", leading to a confusinghttp://localhost:NaNURL.♻️ Suggested validation
-const E2E_PORT = Number(process.env.OHIF_PORT || 3335); +const E2E_PORT = (() => { + const port = Number(process.env.OHIF_PORT || 3335); + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new Error(`Invalid OHIF_PORT: ${process.env.OHIF_PORT}`); + } + return port; +})();🤖 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 `@playwright.config.ts` around lines 3 - 4, E2E_PORT is set using Number(...) without validation which can produce NaN for bad env values; replicate the utility script's validation: read process.env.OHIF_PORT, if present parse it as an integer (base 10), ensure it's an integer between 1 and 65535, and if invalid throw a descriptive Error stating the invalid OHIF_PORT value and expected range; if not present, use the default 3335. Then set E2E_PORT and E2E_BASE_URL using the validated integer. Reference the constants E2E_PORT and E2E_BASE_URL and mirror the validation behavior from free-ohif-e2e-port.mjs.
🤖 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 `@extensions/cornerstone/src/commandsModule.ts`:
- Around line 202-240: The function _getRegionSegmentPlusLimits uses direct
instanceof checks for StackViewport and VolumeViewport but those classes are not
imported and bypass the legacy GenericViewport adapters; replace the instanceof
checks by using the existing helper predicates isStackViewportType(viewport) and
isVolumeViewportType(viewport) (importing them if missing) to detect viewport
types, keep the same logic for computing maxDeltaK/maxDeltaIJ, and ensure
csUtils.getImageSliceDataForVolumeViewport(viewport) is only called when
isVolumeViewportType returns true; update imports to include the predicate
functions rather than StackViewport/VolumeViewport classes.
---
Outside diff comments:
In `@modes/segmentation/src/toolbarButtons.ts`:
- Around line 766-822: The toolbar button currently uses id 'RegionSegmentPlus'
but setToolActiveToolbar resolves targets from toolName || itemId || value so it
may activate the wrong legacy id; update the commands for this button (the
commands array that currently includes 'setToolActiveToolbar') to pass the
flood-fill tool name explicitly (e.g., include a command entry invoking
setToolActiveToolbar with toolName: 'RegionSegmentPlusFloodFill' or set the
value to 'RegionSegmentPlusFloodFill') so activation targets the registered
tool; locate the button definition (id: 'RegionSegmentPlus') in
modes/segmentation/src/toolbarButtons.ts and modify the commands element to
supply the explicit tool name when calling setToolActiveToolbar.
---
Nitpick comments:
In `@playwright.config.ts`:
- Around line 3-4: E2E_PORT is set using Number(...) without validation which
can produce NaN for bad env values; replicate the utility script's validation:
read process.env.OHIF_PORT, if present parse it as an integer (base 10), ensure
it's an integer between 1 and 65535, and if invalid throw a descriptive Error
stating the invalid OHIF_PORT value and expected range; if not present, use the
default 3335. Then set E2E_PORT and E2E_BASE_URL using the validated integer.
Reference the constants E2E_PORT and E2E_BASE_URL and mirror the validation
behavior from free-ohif-e2e-port.mjs.
🪄 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: 936c80e0-989f-4e34-b998-c88d30ba5898
📒 Files selected for processing (7)
.github/workflows/playwright.yml.scripts/ci/free-ohif-e2e-port.mjsextensions/cornerstone/src/commandsModule.tsextensions/cornerstone/src/initCornerstoneTools.jsmodes/segmentation/src/initToolGroups.tsmodes/segmentation/src/toolbarButtons.tsplaywright.config.ts
…anch Squash of origin/feat/ohifpnpm2 (#6031) which converts the monorepo build to pnpm + Rspack, node >=24, and adds the cs3d:* dev helper scripts. Brought in onto fix/grow-cut-suv-pt ahead of the same squash landing on origin/master. Conflict in playwright.config.ts resolved by keeping the grow-cut branch's CI port-cleanup wrapper (free-ohif-e2e-port.mjs) and parametrized E2E_PORT/E2E_BASE_URL, while switching the underlying webServer command to the pnpm/rspack invocation from the pnpm branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/playwright.yml (1)
113-114:⚠️ Potential issue | 🟠 MajorFix Windows port cleanup to avoid substring-matching other ports.
Free OHIF e2e port (stale self-hosted processes)runs.scripts/ci/free-ohif-e2e-port.mjsbefore Playwright tests. Onwin32, the script usesnetstat -ano | findstr :${port} | findstr LISTENING, which matches substrings (e.g.,:3335can also match:33350) and can kill unrelated listener processes on the runner.🤖 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 @.github/workflows/playwright.yml around lines 113 - 114, The Windows port-cleanup is using netstat + findstr with a substring match (in .scripts/ci/free-ohif-e2e-port.mjs), which can match ports like 33350 when you meant 3335; update the win32 branch to match the exact port: either replace the netstat|findstr pipeline with a PowerShell query (Get-NetTCPConnection -LocalPort <port> | Where-Object State -eq 'Listen' to get exact listeners and their PIDs) or change the findstr invocation to a strict regex (e.g., findstr /R /C:":<port>\\b" or include the trailing space/column delimiter) so only the exact port is matched before killing processes (adjust logic in the function that builds the command in free-ohif-e2e-port.mjs).platform/app/package.json (1)
4-4:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate
productVersionkey in JSON.
productVersionappears at both line 4 and line 103. JSON parsers will use the last occurrence, but this duplication is likely unintentional and confusing. Remove the duplicate at line 103."tailwindcss": "3.2.4" }, - "productVersion": "3.4.0", "proxy": "http://localhost:8042" }Also applies to: 103-103
🤖 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 `@platform/app/package.json` at line 4, Remove the duplicate JSON key by deleting the later occurrence of "productVersion" so only a single "productVersion": "3.4.0" remains; locate the duplicate key named "productVersion" (the second occurrence near the end of the file) and remove that entry, then validate the package.json syntax (e.g., with JSON lint or npm pack) to ensure no trailing commas or structural errors remain.
🧹 Nitpick comments (4)
platform/docs/docs/platform/modes/index.md (1)
454-459: ⚡ Quick winClarify "generating modes with an agent".
The note mentions the CLI "is being phased out in favour of generating modes with an agent" but doesn't specify which agent or link to documentation. Users reading this may not understand what "agent" means in this context (AI agent? build agent? custom tooling?). Consider adding a link or brief explanation.
🤖 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 `@platform/docs/docs/platform/modes/index.md` around lines 454 - 459, The note in the "modes" documentation uses the ambiguous phrase "generating modes with an agent" without specifying which agent or linking to relevant docs; update the note in the index.md "modes" section to clarify what "agent" means (e.g., specify "AI agent", "automation agent", or the specific tool name) and add a short parenthetical explanation plus a link to the appropriate documentation or README for the agent (or to the development/agents page) so readers know which agent to use and where to find usage/installation instructions; keep the existing mention of the CLI but indicate it is being phased out in favor of the named agent.tests/SegmentationPanel.spec.ts (1)
23-29: ⚡ Quick winKeep segment-count assertions on the page-object API.
Please switch these back to
await panel.getSegmentCount()plusexpect(count).toBe(...)rather than asserting onpanel.rowsdirectly. This project already standardized on the page-object count helper for segmentation tests, so bypassing it here makes the suite less consistent and harder to evolve if the underlying locator changes.Based on learnings: "In OHIF/Viewers Playwright tests, prefer asserting segment counts via the relevant page object’s
getSegmentCount()method ... Do not suggest replacing thesegetSegmentCount()+expect(...).toBe(...)assertions with Playwright locator-basedexpect(locator).toHaveCount(n)for this project."Also applies to: 40-41
🤖 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/SegmentationPanel.spec.ts` around lines 23 - 29, Replace direct locator count assertions on panel.rows with the page-object API: call panel.getSegmentCount() and assert using expect(count).toBe(expected). Specifically, change the two places where the test uses await expect(panel.rows).toHaveCount(1) (and the similar assertion at lines 40-41) to use const count = await panel.getSegmentCount(); await expect(count).toBe(1) (or expect(count).toBe(...)), while keeping the segment text check via panel.nthSegment(0).locator; this preserves the standardized page-object abstraction (panel.getSegmentCount, panel.nthSegment).Source: Learnings
extensions/measurement-tracking/package.json (2)
42-42: 💤 Low value
webpack-mergeis declared in bothpeerDependenciesanddevDependencies.This duplication is redundant. If it's needed for build configuration, keeping it only in
devDependenciesis sufficient for development, and it doesn't need to be a peer dependency unless external consumers must provide it."peerDependencies": { ... "react-dom": "18.3.1", - "webpack-merge": "5.10.0" },Also applies to: 50-52
🤖 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 `@extensions/measurement-tracking/package.json` at line 42, The package.json declares "webpack-merge" in both peerDependencies and devDependencies which is redundant; remove the duplicate entry from peerDependencies and keep "webpack-merge" only in devDependencies (or vice-versa if you intentionally require consumers to provide it) and update the peerDependencies block accordingly; also check and remove the duplicate occurrences referenced around the other reported lines (50-52) so "webpack-merge" appears only once in the manifest.
35-35: 💤 Low value
@ohif/uiis declared in bothpeerDependenciesanddependencies.Having the same package in both sections creates ambiguity about resolution. Typically, if it's a peer dependency, it should not also be in direct dependencies. Consider removing it from one section (likely
dependenciesif the consuming app is expected to provide it)."dependencies": { "`@babel/runtime`": "7.29.7", - "`@ohif/ui`": "workspace:*", "`@xstate/react`": "3.2.2", "xstate": "4.38.3" },Also applies to: 44-49
🤖 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 `@extensions/measurement-tracking/package.json` at line 35, The package.json currently lists "`@ohif/ui`" in both dependencies and peerDependencies which causes ambiguity; remove the duplicate by keeping it only in peerDependencies (so the host app provides it) and delete the "`@ohif/ui`" entry from dependencies (also remove any duplicate entries around lines 44-49 mentioned in the review) to ensure a single source of truth for resolution.
🤖 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 @.github/workflows/build-docs.yml:
- Line 81: The gh run download step uses the unvalidated expression `${{
steps.find_pr_run.outputs.run_id }}` which could be abused if its format
changes; fix by validating that `steps.find_pr_run.outputs.run_id` contains only
digits before invoking `gh run download` (e.g., add a prior step that checks
`/^\d+$/` and fails if not) and then use the validated value (or a sanitized
output variable) in the `gh run download` command (also wrap the injected value
in quotes or pass it via a safe flag) so the `run_id` is guaranteed numeric and
cannot be used for command injection.
In `@extensions/cornerstone-dicom-sr/package.json`:
- Around line 23-25: The dev script in modes/segmentation's package.json calls
rspack with .webpack/webpack.dev.js but that file is missing; add a
.webpack/webpack.dev.js for modes/segmentation (or change the dev script) so the
dev build works. Fix by creating modes/segmentation/.webpack/webpack.dev.js that
mirrors the pattern used in extensions/cornerstone-dicom-sr and
modes/usAnnotation: require or import the shared .webpack/webpack.base.js, set
mode to "development" and export the config (ensuring compatibility with
`@rspack/core`), or alternatively change the dev script to point to the existing
.webpack/webpack.prod.js if that is intentional.
In `@modes/tmtv/package.json`:
- Around line 31-38: The peerDependencies block in modes/tmtv/package.json (and
likewise in extensions/cornerstone/package.json) currently uses monorepo-only
"workspace:*" ranges for entries like "`@ohif/core`",
"`@ohif/extension-cornerstone`", "`@ohif/extension-default`", etc.; replace every
"workspace:*" with an appropriate semver range (e.g. ^<version> or a range
matching the published package versions) so the published package exposes real
semver peer ranges; ensure you update each dependency key in the
"peerDependencies" object to the chosen semver (matching the root/monorepo
published versions) and run npm pack / npm publish dry-run to verify no
workspace protocol remains.
In `@platform/cli/src/commands/utils/getYarnInfo.js`:
- Around line 4-5: getYarnInfo currently returns JSON.parse(stdout) directly
from execa('npm', ['info', packageName, '--json']) which can be either an object
or an array; normalize the parsed result to preserve the previous return shape
by detecting if the parsed value is an array and selecting the appropriate
element (e.g., first item or the one matching packageName) or converting an
array into a single object with expected fields (peerDependencies, version,
etc.), and ensure getYarnInfo always returns an object with those fields; update
getYarnInfo to handle null/undefined stdout and throw or return a consistent
empty shape, and add a small unit test that simulates npm info returning both an
object and an array for the same package spec to assert the normalized shape is
returned.
In `@platform/core/package.json`:
- Line 21: The package.json currently sets "sideEffects" as a string ("false")
which can be misinterpreted by bundlers; change the value to the boolean false
(i.e., sideEffects: false) in the package.json so bundlers correctly treat the
package as side-effect-free and enable proper tree-shaking, ensuring the JSON
remains valid.
In `@platform/docs/docs/migration-guide/3p12-to-3p13/node-version.md`:
- Line 59: Update the CI reference line to use the proper product name and
capitalization: replace the lowercase "GitHub" occurrence in the phrase "CI
config (`.github/workflows/*`, `.circleci/config.yml`, " with the correctly
capitalized product name "GitHub Actions" so the line reads something like "CI
config (GitHub Actions `.github/workflows/*`, `.circleci/config.yml`, ..."
ensuring consistent capitalization and explicit mention of "GitHub Actions".
- Line 25: The doc string in node-version.md lists the pnpm engine as "pnpm":
"11.1.1" but templates now declare "pnpm": ">=11" in dependencies.json; update
node-version.md to match the generated manifest (change the documented pnpm
requirement to ">=11" or otherwise mirror the exact engine string used in
platform/cli/templates/mode/dependencies.json) and check the other occurrences
noted (lines 56–58) for the same mismatch so the documentation and template stay
consistent.
In `@platform/docs/docs/migration-guide/3p12-to-3p13/package-manager.md`:
- Around line 156-162: The package.json script examples contain malformed pnpm
invocations: update the script values for "dev:my-extension" and "start" from
"pnpm rundev" to "pnpm run dev", and update "build:package" from "pnpm runbuild"
to "pnpm run build" so the "pnpm run <script>" form is used correctly (look for
the "dev:my-extension", "build:package", and "start" entries to locate and fix
the strings).
In `@platform/ui-next/package.json`:
- Around line 9-17: The package.json currently exposes subpath exports (e.g.,
"./tailwind.config", "./lib/*", "./components/*", ".") that point at source
files (tailwind.config.js and src/**) but the "files" array only includes "dist"
and "README.md", causing published consumers to miss those exported paths; fix
by updating the "exports" entries to reference built outputs under dist (for
example change "./tailwind.config" and the "./lib/*" and "./components/*"
mappings to point at the corresponding files in dist) or alternatively add
"tailwind.config.js" and the relevant "src" paths to the "files" array so those
exported subpaths are included in the package. Ensure changes touch the
package.json keys "exports" and/or "files" so exports and published content
align.
In `@platform/ui/package.json`:
- Line 13: Update the repo documentation to state Node 24 is the new supported
baseline: make sure the package.json engines.node entry (engines.node) and
.node-version align with Node 24, then add a clear note in the README and
release notes/changelog indicating "Node >=24 (Node 24 LTS)" as the minimum
supported version, call out breaking compatibility with Node 18/20/22, and add a
short migration snippet (recommended upgrade steps and an nvm/snippet
suggestion) so consumers know how to upgrade; also mention where CI/Docker were
updated (CI node-version and Docker FROM node:24-slim) so readers understand the
change is repo-wide.
In `@publish-version.mjs`:
- Around line 88-95: The push of the tag (the execa call that currently runs
execa('git', ['push', 'origin', tagName'])) can fail if the remote already has
that tag because you only force the local tag (git tag -f) but not the remote
push; update the tag push to either include --force (execa('git', ['push',
'--force', 'origin', tagName'])) to match the local behavior or wrap the
existing execa call in a try/catch and on failure delete the remote tag (git
push origin :refs/tags/<tagName> or git tag -d <tagName> remote deletion) then
retry pushing tagName; locate the occurrences of tagName and branchName and the
execa('git', ...) calls to implement the change.
In `@tests/pages/RightPanelPageObject.ts`:
- Around line 227-231: The `rows` locator and getSegmentCount use a global
page.getByTestId('data-row'), causing flaky counts; scope them to the
segmentation panel root instead. Define a segmentation-panel-scoped locator
(e.g., segmentationRoot = page.getByTestId('segmentation-panel') or the existing
segmentation panel locator) and replace occurrences so rows =
segmentationRoot.getByTestId('data-row'); update getSegmentCount and any helpers
that reference rows/index/text to reuse that scoped rows locator.
---
Outside diff comments:
In @.github/workflows/playwright.yml:
- Around line 113-114: The Windows port-cleanup is using netstat + findstr with
a substring match (in .scripts/ci/free-ohif-e2e-port.mjs), which can match ports
like 33350 when you meant 3335; update the win32 branch to match the exact port:
either replace the netstat|findstr pipeline with a PowerShell query
(Get-NetTCPConnection -LocalPort <port> | Where-Object State -eq 'Listen' to get
exact listeners and their PIDs) or change the findstr invocation to a strict
regex (e.g., findstr /R /C:":<port>\\b" or include the trailing space/column
delimiter) so only the exact port is matched before killing processes (adjust
logic in the function that builds the command in free-ohif-e2e-port.mjs).
In `@platform/app/package.json`:
- Line 4: Remove the duplicate JSON key by deleting the later occurrence of
"productVersion" so only a single "productVersion": "3.4.0" remains; locate the
duplicate key named "productVersion" (the second occurrence near the end of the
file) and remove that entry, then validate the package.json syntax (e.g., with
JSON lint or npm pack) to ensure no trailing commas or structural errors remain.
---
Nitpick comments:
In `@extensions/measurement-tracking/package.json`:
- Line 42: The package.json declares "webpack-merge" in both peerDependencies
and devDependencies which is redundant; remove the duplicate entry from
peerDependencies and keep "webpack-merge" only in devDependencies (or vice-versa
if you intentionally require consumers to provide it) and update the
peerDependencies block accordingly; also check and remove the duplicate
occurrences referenced around the other reported lines (50-52) so
"webpack-merge" appears only once in the manifest.
- Line 35: The package.json currently lists "`@ohif/ui`" in both dependencies and
peerDependencies which causes ambiguity; remove the duplicate by keeping it only
in peerDependencies (so the host app provides it) and delete the "`@ohif/ui`"
entry from dependencies (also remove any duplicate entries around lines 44-49
mentioned in the review) to ensure a single source of truth for resolution.
In `@platform/docs/docs/platform/modes/index.md`:
- Around line 454-459: The note in the "modes" documentation uses the ambiguous
phrase "generating modes with an agent" without specifying which agent or
linking to relevant docs; update the note in the index.md "modes" section to
clarify what "agent" means (e.g., specify "AI agent", "automation agent", or the
specific tool name) and add a short parenthetical explanation plus a link to the
appropriate documentation or README for the agent (or to the development/agents
page) so readers know which agent to use and where to find usage/installation
instructions; keep the existing mention of the CLI but indicate it is being
phased out in favor of the named agent.
In `@tests/SegmentationPanel.spec.ts`:
- Around line 23-29: Replace direct locator count assertions on panel.rows with
the page-object API: call panel.getSegmentCount() and assert using
expect(count).toBe(expected). Specifically, change the two places where the test
uses await expect(panel.rows).toHaveCount(1) (and the similar assertion at lines
40-41) to use const count = await panel.getSegmentCount(); await
expect(count).toBe(1) (or expect(count).toBe(...)), while keeping the segment
text check via panel.nthSegment(0).locator; this preserves the standardized
page-object abstraction (panel.getSegmentCount, panel.nthSegment).
🪄 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: 24d573f6-1c2d-43d0-aa22-d8df0d4590c6
⛔ Files ignored due to path filters (42)
.webpack/resolveConfig.jsis excluded by!**/.webpack/**.webpack/rules/cssToJavaScript.jsis excluded by!**/.webpack/**.webpack/rules/transpileJavaScript.jsis excluded by!**/.webpack/**.webpack/webpack.base.jsis excluded by!**/.webpack/**bun.lockis excluded by!**/*.lockextensions/cornerstone-dicom-pmap/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-rt/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-seg/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dicom-sr/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone-dynamic-volume/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/cornerstone/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/default/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-microscopy/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-pdf/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/dicom-video/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/measurement-tracking/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/test-extension/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/tmtv/.webpack/webpack.prod.jsis excluded by!**/.webpack/**extensions/usAnnotation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic-dev-mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic-test-mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/basic/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/longitudinal/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/microscopy/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/preclinical-4d/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/segmentation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/tmtv/.webpack/webpack.prod.jsis excluded by!**/.webpack/**modes/usAnnotation/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/app/.webpack/rules/extractStyleChunks.jsis excluded by!**/.webpack/**platform/app/.webpack/webpack.pwa.jsis excluded by!**/.webpack/**platform/app/.webpack/writePluginImportsFile.jsis excluded by!**/.webpack/**platform/cli/templates/extension/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/cli/templates/mode/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/core/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/docs/bun.lockbis excluded by!**/bun.lockbplatform/docs/yarn.lockis excluded by!**/yarn.lock,!**/*.lockplatform/i18n/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui-next/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui/.webpack/webpack.prod.jsis excluded by!**/.webpack/**platform/ui/a34f9d1faa5f3315.woff2is excluded by!**/*.woff2pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (84)
.circleci/config.yml.github/.dependabot.yaml.github/workflows/build-docs.yml.github/workflows/codeql.yml.github/workflows/playwright.yml.gitignore.netlify/build-deploy-preview.sh.netlify/package.json.node-version.npmrc.scripts/dev.shDockerfileaddOns/README.mdaddOns/externals/devDependencies/CHANGELOG.mdaddOns/externals/devDependencies/package.jsonaddOns/externals/dicom-microscopy-viewer/CHANGELOG.mdaddOns/externals/dicom-microscopy-viewer/package.jsonaddOns/package.jsonbunfig.tomlbunfig.update-lockfile.tomlextensions/cornerstone-dicom-pmap/package.jsonextensions/cornerstone-dicom-rt/package.jsonextensions/cornerstone-dicom-seg/package.jsonextensions/cornerstone-dicom-sr/package.jsonextensions/cornerstone-dynamic-volume/package.jsonextensions/cornerstone/package.jsonextensions/default/package.jsonextensions/default/src/ViewerLayout/index.tsxextensions/dicom-microscopy/package.jsonextensions/dicom-pdf/package.jsonextensions/dicom-video/package.jsonextensions/measurement-tracking/package.jsonextensions/test-extension/package.jsonextensions/tmtv/package.jsonextensions/usAnnotation/package.jsonlerna.jsonmodes/basic-dev-mode/package.jsonmodes/basic-test-mode/package.jsonmodes/basic/package.jsonmodes/longitudinal/package.jsonmodes/microscopy/package.jsonmodes/preclinical-4d/package.jsonmodes/segmentation/package.jsonmodes/tmtv/package.jsonmodes/usAnnotation/package.jsonnetlify-lerna-cache.shnetlify-lerna-restore.shnetlify.tomlnx.jsonpackage.jsonplatform/app/cypress.config.tsplatform/app/cypress/integration/measurement-tracking/OHIFSaveMeasurements.spec.jsplatform/app/cypress/support/commands.jsplatform/app/package.jsonplatform/app/tailwind.config.jsplatform/cli/package.jsonplatform/cli/src/commands/createPackage.jsplatform/cli/src/commands/utils/getYarnInfo.jsplatform/cli/src/commands/utils/uninstallNPMPackage.jsplatform/cli/src/index.jsplatform/cli/templates/extension/dependencies.jsonplatform/cli/templates/mode/dependencies.jsonplatform/core/package.jsonplatform/docs/docs/migration-guide/3p12-to-3p13/build-tooling.mdplatform/docs/docs/migration-guide/3p12-to-3p13/index.mdplatform/docs/docs/migration-guide/3p12-to-3p13/node-version.mdplatform/docs/docs/migration-guide/3p12-to-3p13/package-manager.mdplatform/docs/docs/platform/extensions/index.mdplatform/docs/docs/platform/extensions/pluginConfig.mdplatform/docs/docs/platform/modes/index.mdplatform/docs/package.jsonplatform/i18n/package.jsonplatform/ui-next/package.jsonplatform/ui/package.jsonplaywright.config.tspnpm-workspace.yamlpublish-package.mjspublish-version.mjsrsbuild.config.tstests/FreehandROI.spec.tstests/SegmentationPanel.spec.tstests/globalSetup.tstests/pages/RightPanelPageObject.tsversion.mjs
💤 Files with no reviewable changes (11)
- addOns/package.json
- nx.json
- netlify-lerna-cache.sh
- addOns/externals/dicom-microscopy-viewer/package.json
- addOns/README.md
- platform/app/cypress/integration/measurement-tracking/OHIFSaveMeasurements.spec.js
- lerna.json
- bunfig.toml
- netlify-lerna-restore.sh
- addOns/externals/devDependencies/package.json
- bunfig.update-lockfile.toml
✅ Files skipped from review due to trivial changes (6)
- .netlify/package.json
- .npmrc
- platform/docs/docs/migration-guide/3p12-to-3p13/index.md
- platform/docs/docs/platform/extensions/index.md
- platform/docs/docs/platform/extensions/pluginConfig.md
- platform/docs/docs/migration-guide/3p12-to-3p13/build-tooling.md
🚧 Files skipped from review as they are similar to previous changes (1)
- playwright.config.ts
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| mkdir -p ./coverage-artifact | ||
| gh run download ${{ steps.find_pr_run.outputs.run_id }} -n coverage-report-pr --dir ./coverage-artifact |
There was a problem hiding this comment.
Validate run_id format to prevent potential command injection.
The static analyzer flagged a potential template injection risk where ${{ steps.find_pr_run.outputs.run_id }} is used in the shell command. While run_id comes from GitHub's API (via gh run list) and should be a numeric database ID, it's best practice to validate or quote the variable to prevent any theoretical command injection if the output format changes.
🛡️ Recommended fix to add validation or quoting
- name: Download coverage artifact from PR run
if: steps.find_pr_run.outputs.coverage_found == 'true'
env:
GH_TOKEN: ${{ github.token }}
run: |
+ RUN_ID="${{ steps.find_pr_run.outputs.run_id }}"
+ # Validate run_id is numeric
+ if ! [[ "$RUN_ID" =~ ^[0-9]+$ ]]; then
+ echo "Invalid run_id format: $RUN_ID"
+ exit 1
+ fi
mkdir -p ./coverage-artifact
- gh run download ${{ steps.find_pr_run.outputs.run_id }} -n coverage-report-pr --dir ./coverage-artifact
+ gh run download "$RUN_ID" -n coverage-report-pr --dir ./coverage-artifact🧰 Tools
🪛 zizmor (1.25.2)
[info] 81-81: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/build-docs.yml at line 81, The gh run download step uses
the unvalidated expression `${{ steps.find_pr_run.outputs.run_id }}` which could
be abused if its format changes; fix by validating that
`steps.find_pr_run.outputs.run_id` contains only digits before invoking `gh run
download` (e.g., add a prior step that checks `/^\d+$/` and fails if not) and
then use the validated value (or a sanitized output variable) in the `gh run
download` command (also wrap the injected value in quotes or pass it via a safe
flag) so the `run_id` is guaranteed numeric and cannot be used for command
injection.
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
1 participant
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
The old growcut implementation had fundamental issues working with PT type series because of the size/setup of the PT.
This is being replaced by a newer version that uses teh currently displayed image as the base image for determining the cutoff, and uses a flood fill algorithm that is bounded to give reasonable performance for clicking.
Changes & Results
CS3D_REF: fix/grow-cut-suv-pt
Checkout the referenced branch and link/build/run
Mostly changed the growcut version tag to reference the new growcut algorithm.
Testing
If you are using speckled areas (like CT brain matter), check the checkbox to use the full area, otherwise select a bright OR
dark are (high contrast with an edge), and click the circle so there is some inside, some outside regions, and you click on an included point.
Using the full area one click, just click so the circle is entirely within the mottled area.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
Replaces the old GrowCut (
RegionSegmentPlusTool) implementation with a flood-fill variant (RegionSegmentPlusFloodFillTool) that operates on the currently displayed image slice and is bounded by configurable Max Delta K (slice direction) and Max Delta IJ (in-plane) parameters. The CI Playwright setup gains a cross-platform port-cleanup script (free-ohif-e2e-port.mjs) that runs before the dev server starts, and the config is refactored to drive the port from a singleOHIF_PORTenv var.initCornerstoneTools.js/initToolGroups.ts— tool registration and initial configuration wired to the new flood-fill tool.commandsModule.ts— adds_getRegionSegmentPlusLimits,cancelMeasurement(also hooked intorejectPreviewfor ESC cancellation), andsetRegionSegmentPlusFloodFillConfigurationwhich updates both the toolbar sliders and the live tool configuration.playwright.config.ts/free-ohif-e2e-port.mjs— CI improvements: parameterised port, reduced workers (18 → 6), and pre-server port cleanup on self-hosted runners.Confidence Score: 5/5
Safe to merge; the core flood-fill tool swap is clean and the CI helper is well-guarded.
The tool replacement is mechanical (import swap + configuration block), and the new command logic is well-structured. Issues flagged in earlier review threads (imageData fallback, toolbar option mutation, Windows findstr substring match) are the only open concerns; no new blocking problems were found in this pass.
extensions/cornerstone/src/commandsModule.ts — open items from prior threads around the imageData fallback branch and toolbar option mutation remain unresolved.
Important Files Changed
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile