feat: add results for different viewports on the fusion view for rectangle/circle ROI#2557
Merged
Conversation
wayfarer3130
commented
Jan 15, 2026
wayfarer3130
commented
Jan 15, 2026
wayfarer3130
commented
Jan 15, 2026
wayfarer3130
commented
Jan 15, 2026
sedghi
reviewed
Jan 21, 2026
| "name": "PET-CT Fusion + MIPLayout", | ||
| "description": "PT-CT fusion layout with Crosshairs, and synchronized cameras, CT W/L and PET threshold" | ||
| }, | ||
| "tmtv": { |
Member
There was a problem hiding this comment.
don't we already have two ptct one? maybe do it in those examples? and we don't add one here?
sedghi
reviewed
Jan 21, 2026
sedghi
reviewed
Jan 21, 2026
sedghi
reviewed
Jan 21, 2026
…ults # Conflicts: # packages/tools/src/tools/annotation/CircleROITool.ts # packages/tools/src/tools/annotation/RectangleROITool.ts # packages/tools/src/utilities/index.ts
|
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:
📝 WalkthroughWalkthroughThe PR adds multi-target ROI statistics and filtering, updates volume-aware viewport references and image-data lookup, refactors the TMTV demo around dynamic layouts, and adds dynamic-volume and multi-volume measurement examples with related documentation and tests. ChangesMulti-target measurement statistics
Viewport reference and image-data behavior
Dynamic TMTV layouts
Additional examples and repository support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LayoutDropdown
participant TMTV
participant RenderingEngine
participant SynchronizerManager
User->>LayoutDropdown: choose default, fusion, or oblique
LayoutDropdown->>TMTV: call applyLayout(layoutKey)
TMTV->>RenderingEngine: rebuild viewport grid and bindings
TMTV->>SynchronizerManager: destroy and recreate layout synchronizers
TMTV->>RenderingEngine: set layout volumes and render
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tools/src/tools/segmentation/CircleROIStartEndThresholdTool.ts (1)
191-230: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDrop
nullforlabelmapUID.VolumeStats.statisticsisunknown, sostatistics: []is fine; the remaining mismatch islabelmapUID: nullvslabelmapUID?: string, which should beundefinedor broadened in the type.🤖 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 `@packages/tools/src/tools/segmentation/CircleROIStartEndThresholdTool.ts` around lines 191 - 230, In CircleROIStartEndThresholdTool, the annotation object’s data.labelmapUID is using null even though the type is optional string, so update the CircleROIStartEndThresholdAnnotation construction to omit labelmapUID or set it to undefined instead. Check the annotation literal in the create flow alongside cachedStats and statistics so the shape matches the defined data type.packages/tools/src/tools/annotation/RectangleROITool.ts (1)
618-659: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSeed cached stats for all measurement targets
getMeasurementTargets()only feeds text-line rendering here; cache initialization and recomputation still start from a singletargetId. When it returns multiple IDs, the extra entries never get seeded in this path, so multi-target annotations stay incomplete until another viewport populatescachedStats.🤖 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 `@packages/tools/src/tools/annotation/RectangleROITool.ts` around lines 618 - 659, Seed and recompute cached stats for every entry returned by getMeasurementTargets() in RectangleROITool, not just the single targetId. Update the cache-initialization path and the invalidated branch so each measurement target gets its own cachedStats entry and calls into _calculateCachedStats or _throttledCalculateCachedStats as needed. Keep the existing getTargetId/getMeasurementTargets split, but make the stats setup loop over all targetIds so multi-target annotations are fully populated in this rendering path.packages/tools/src/tools/annotation/CircleROITool.ts (1)
697-733: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSeed all measurement targets before building text lines.
getMeasurementTargets()only reuses IDs already present indata.cachedStats, but this path seeds justtargetId. If this viewport is the first one rendering the annotation, secondary volumes never get added to the cache, so_calculateCachedStats()never computes them and their text lines stay hidden until another viewport fillsdata.cachedStats. packages/tools/src/tools/annotation/CircleROITool.ts:697-733, 877-880🤖 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 `@packages/tools/src/tools/annotation/CircleROITool.ts` around lines 697 - 733, Seed cached stats for every measurement target before text generation, not just the current targetId. In CircleROITool’s rendering path, make sure all IDs returned by getMeasurementTargets()/derived from getTargetId are initialized in data.cachedStats before _calculateCachedStats() and the measurement text lines are built, so secondary volumes are not skipped when this viewport renders first.
🧹 Nitpick comments (5)
packages/tools/src/types/AnnotationTypes.ts (1)
104-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
Omitfor a cleaner intersection type.
AnnotationDataStatsByTargetId = AnnotationData & { cachedStats: Record<string, CachedStats> }produces an intersection wherecachedStatsbecomes(Record<string, CachedStats> | VolumeStats) & Record<string, CachedStats>, which includes aVolumeStats & Record<string, CachedStats>arm that is never intended. UsingOmitavoids this:-export type AnnotationDataStatsByTargetId = AnnotationData & { - cachedStats: Record<string, CachedStats>; -}; +export type AnnotationDataStatsByTargetId = Omit<AnnotationData, 'cachedStats'> & { + cachedStats: Record<string, CachedStats>; +};🤖 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 `@packages/tools/src/types/AnnotationTypes.ts` around lines 104 - 107, The AnnotationDataStatsByTargetId type currently uses an intersection with AnnotationData, which leaves an unintended cachedStats union arm in the resulting type. Update the type alias in AnnotationTypes.ts to use Omit on AnnotationData for cachedStats before adding cachedStats: Record<string, CachedStats>, so the resulting shape is clean and avoids the extra VolumeStats intersection.packages/tools/src/tools/base/BaseTool.ts (1)
327-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
if (ref)check and missing return type annotation.The
if (ref)on line 361 is always true sinceif (!ref) { continue; }on line 358 already handles the falsy case. Also,getMeasurementTargetslacks an explicit return type annotation — the inferred type is(string | undefined)[]becausegetTargetIdreturnsstring | undefined, but callers use the result asstring[].♻️ Proposed cleanup
- protected getMeasurementTargets( + protected getMeasurementTargets( viewport: Types.IViewport, data?: AnnotationData - ) { + ): string[] { const { showAllTargets } = this.configurationTyped; const actors = viewport.getActors(); if (showAllTargets === false || !actors?.length) { - return [this.getTargetId(viewport, data)]; + return [this.getTargetId(viewport, data) ?? '']; } const references = []; const keys = data?.cachedStats ? Object.keys(data.cachedStats) : []; for (const actor of actors) { const volumeId = actor.referencedId; if (!volumeId) { continue; } const volumeIdQuery = volumeId + '?'; const ref = keys.find((key) => key.indexOf(volumeIdQuery) !== -1); - if (!ref) { - continue; - } - if (ref) { - references.push(ref); - } + if (ref) { + references.push(ref); + } }🤖 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 `@packages/tools/src/tools/base/BaseTool.ts` around lines 327 - 370, Remove the redundant `if (ref)` check in `BaseTool.getMeasurementTargets` since the falsy case is already skipped by the preceding `continue`, and add an explicit return type for `getMeasurementTargets` so it matches the expected string array shape used by callers. Update the method signature in `BaseTool` to make the return type explicit, and ensure the fallback from `getTargetId` is handled consistently with the rest of the method’s returned references.packages/tools/src/utilities/defaultGetTextLines.ts (1)
30-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFilter hardcodes
meancheck, makingcreateGetTextLinesnon-generic.The filter
it?.mean !== undefined && it?.mean !== nullassumes all metric sets includemean. IfcreateGetTextLinesis used with metrics that don't computemean(e.g., a custom metrics array with onlyareaandperimeter), valid entries would be excluded and no text lines would be generated.Consider filtering based on whether any of the configured metrics' attributes are present, or at minimum document that
meanis required for all stat entries.♻️ Proposed generic filter
const cachedVolumeStats = targetIds .map((id) => data.cachedStats[id]) - .filter((it) => it?.mean !== undefined && it?.mean !== null); + .filter((it) => + metrics.some((m) => it?.[m.attribute] !== undefined && it?.[m.attribute] !== null) + );🤖 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 `@packages/tools/src/utilities/defaultGetTextLines.ts` around lines 30 - 32, The cached stats filter in createGetTextLines/defaultGetTextLines is hardcoded to require mean, which breaks use with metric sets that don’t define that field. Update the filter on cachedVolumeStats to check for the presence of the configured metric attributes instead of assuming mean, using the existing targetIds/data.cachedStats flow. If mean is intentionally required, make that contract explicit in createGetTextLines documentation and type expectations so callers know the constraint.packages/tools/src/tools/annotation/CircleROITool.ts (2)
877-880: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getMeasurementTargetscalled on every annotation, every render frame.This resolves targets fresh per annotation per animation frame instead of once per
renderAnnotationinvocation (viewport doesn't change across the loop). If the resolver iterates viewport actors, this repeats that workannotations.lengthtimes per frame.🤖 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 `@packages/tools/src/tools/annotation/CircleROITool.ts` around lines 877 - 880, `getMeasurementTargets` is being recomputed for each annotation inside the `CircleROITool` render loop, which repeats the same viewport lookup work every frame. Move the `this.getMeasurementTargets(viewport, data)` call out of the per-annotation path in `renderAnnotation` so it is resolved once per invocation and then reused when calling `this.configuration.getTextLines`, keeping the behavior the same while avoiding repeated actor iteration.
1157-1165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared modality metrics.
CIRCLE_ROI_METRICSduplicates the Mean/Max/Min/Std Dev entries fromAREA_METRICS; split those four lines into a shared constant and composeAREA_METRICS/CIRCLE_ROI_METRICSfrom it.AREA_METRICSitself can’t be reused directly because it also includesArea.🤖 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 `@packages/tools/src/tools/annotation/CircleROITool.ts` around lines 1157 - 1165, The CircleROITool metric definitions duplicate the shared Mean/Max/Min/Std Dev entries already present in AREA_METRICS. Extract those four common metric objects into a shared constant in CircleROITool, then build both AREA_METRICS and CIRCLE_ROI_METRICS from that shared list while keeping AREA_METRICS’ Area entry separate. Use the existing AREA_METRICS and CIRCLE_ROI_METRICS symbols to locate the change and preserve createGetTextLines usage.
🤖 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 `@packages/core/examples/tmtv/index.ts`:
- Line 173: The toolbar dropdown options in the example setup include
CircleROITool.toolName twice, creating a duplicate entry. Update the
optionsValues list in the tmtv example to remove the repeated
CircleROITool.toolName reference and keep only the single existing occurrence so
the dropdown shows each tool once.
In `@packages/tools/src/tools/base/BaseTool.ts`:
- Line 350: Guard the `Object.keys(data?.cachedStats)` call in `BaseTool` so it
never runs on `undefined`; the current `cachedStats` access can throw for
annotations without computed stats. Update the logic that builds the keys list
to safely fall back to an empty object/array when `data`, `data.cachedStats`, or
the `cachedStats` field on `AnnotationData`/`CircleROIAnnotation` is missing,
and keep the behavior in the surrounding `showAllTargets` path unchanged.
---
Outside diff comments:
In `@packages/tools/src/tools/annotation/CircleROITool.ts`:
- Around line 697-733: Seed cached stats for every measurement target before
text generation, not just the current targetId. In CircleROITool’s rendering
path, make sure all IDs returned by getMeasurementTargets()/derived from
getTargetId are initialized in data.cachedStats before _calculateCachedStats()
and the measurement text lines are built, so secondary volumes are not skipped
when this viewport renders first.
In `@packages/tools/src/tools/annotation/RectangleROITool.ts`:
- Around line 618-659: Seed and recompute cached stats for every entry returned
by getMeasurementTargets() in RectangleROITool, not just the single targetId.
Update the cache-initialization path and the invalidated branch so each
measurement target gets its own cachedStats entry and calls into
_calculateCachedStats or _throttledCalculateCachedStats as needed. Keep the
existing getTargetId/getMeasurementTargets split, but make the stats setup loop
over all targetIds so multi-target annotations are fully populated in this
rendering path.
In `@packages/tools/src/tools/segmentation/CircleROIStartEndThresholdTool.ts`:
- Around line 191-230: In CircleROIStartEndThresholdTool, the annotation
object’s data.labelmapUID is using null even though the type is optional string,
so update the CircleROIStartEndThresholdAnnotation construction to omit
labelmapUID or set it to undefined instead. Check the annotation literal in the
create flow alongside cachedStats and statistics so the shape matches the
defined data type.
---
Nitpick comments:
In `@packages/tools/src/tools/annotation/CircleROITool.ts`:
- Around line 877-880: `getMeasurementTargets` is being recomputed for each
annotation inside the `CircleROITool` render loop, which repeats the same
viewport lookup work every frame. Move the `this.getMeasurementTargets(viewport,
data)` call out of the per-annotation path in `renderAnnotation` so it is
resolved once per invocation and then reused when calling
`this.configuration.getTextLines`, keeping the behavior the same while avoiding
repeated actor iteration.
- Around line 1157-1165: The CircleROITool metric definitions duplicate the
shared Mean/Max/Min/Std Dev entries already present in AREA_METRICS. Extract
those four common metric objects into a shared constant in CircleROITool, then
build both AREA_METRICS and CIRCLE_ROI_METRICS from that shared list while
keeping AREA_METRICS’ Area entry separate. Use the existing AREA_METRICS and
CIRCLE_ROI_METRICS symbols to locate the change and preserve createGetTextLines
usage.
In `@packages/tools/src/tools/base/BaseTool.ts`:
- Around line 327-370: Remove the redundant `if (ref)` check in
`BaseTool.getMeasurementTargets` since the falsy case is already skipped by the
preceding `continue`, and add an explicit return type for
`getMeasurementTargets` so it matches the expected string array shape used by
callers. Update the method signature in `BaseTool` to make the return type
explicit, and ensure the fallback from `getTargetId` is handled consistently
with the rest of the method’s returned references.
In `@packages/tools/src/types/AnnotationTypes.ts`:
- Around line 104-107: The AnnotationDataStatsByTargetId type currently uses an
intersection with AnnotationData, which leaves an unintended cachedStats union
arm in the resulting type. Update the type alias in AnnotationTypes.ts to use
Omit on AnnotationData for cachedStats before adding cachedStats: Record<string,
CachedStats>, so the resulting shape is clean and avoids the extra VolumeStats
intersection.
In `@packages/tools/src/utilities/defaultGetTextLines.ts`:
- Around line 30-32: The cached stats filter in
createGetTextLines/defaultGetTextLines is hardcoded to require mean, which
breaks use with metric sets that don’t define that field. Update the filter on
cachedVolumeStats to check for the presence of the configured metric attributes
instead of assuming mean, using the existing targetIds/data.cachedStats flow. If
mean is intentionally required, make that contract explicit in
createGetTextLines documentation and type expectations so callers know the
constraint.
🪄 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: 1747fd9a-6487-44d3-b334-ef4e7ce1526c
📒 Files selected for processing (17)
packages/core/examples/tmtv/index.tspackages/core/src/RenderingEngine/BaseVolumeViewport.tspackages/core/src/RenderingEngine/VolumeViewport.tspackages/core/src/RenderingEngine/WebGLContextPool.tspackages/tools/src/tools/annotation/BidirectionalTool.tspackages/tools/src/tools/annotation/CircleROITool.tspackages/tools/src/tools/annotation/DragProbeTool.tspackages/tools/src/tools/annotation/RectangleROITool.tspackages/tools/src/tools/base/AnnotationTool.tspackages/tools/src/tools/base/BaseTool.tspackages/tools/src/tools/segmentation/CircleROIStartEndThresholdTool.tspackages/tools/src/types/AnnotationTypes.tspackages/tools/src/types/ToolSpecificAnnotationTypes.tspackages/tools/src/utilities/defaultGetTextLines.tspackages/tools/src/utilities/index.tspackages/tools/src/utilities/planar/filterAnnotationsWithinSlice.tsutils/ExampleRunner/example-info.json
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts`:
- Around line 1224-1240: Update GenericViewport.getImageData so an explicit
volumeId request never falls back to the active binding: resolve the volume
through findDataIdByVolumeId and getBinding, return that binding’s image data
when available, and otherwise return undefined. Preserve the current-binding
fallback only when volumeId is omitted.
In
`@packages/docs/docs/concepts/cornerstone-tools/annotation/measurementTargets.md`:
- Around line 252-255: Update the note in measurementTargets.md to state that
multi-target selection is limited to acquisition-oriented volume viewports, and
that secondary or non-acquisition viewports may not display measurements,
alongside the existing on-screen and stack-fusion limitations.
- Around line 214-223: Clarify the documentation so the cachedStats reuse rule
explicitly applies only to volume candidates. State that volume target IDs use
the `volumeId:<volumeId>?sliceIndex=...&viewPlaneNormal=...` format, and
separately document that stack candidates use imageId-derived target IDs as
described earlier.
In `@packages/tools/src/tools/base/BaseTool.ts`:
- Around line 556-590: Set MeasurementTargetCandidate.index to the matching
display-set position rather than the actor-loop index. In the actor processing
logic, derive the index from the displaySets entry associated with referencedId
(or otherwise preserve its array position), and use that value when constructing
candidates; do not assign the loop variable index from viewport.getActors().
In `@packages/tools/test/CircleROI_test.js`:
- Around line 415-429: Add a rejection handler to the Promise chain in the
test’s volume-loading block by chaining .catch(done.fail) after the existing
.then callback. Keep the synchronous try/catch, and ensure asynchronous failures
from either createAndCacheVolume promise are passed to done.fail.
🪄 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: eaa5f78e-57d6-411e-916d-feb10ad49295
📒 Files selected for processing (11)
packages/core/examples/tmtv/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/docs/docs/concepts/cornerstone-tools/annotation/measurementTargets.mdpackages/docs/sidebars.jspackages/tools/examples/petCt/index.tspackages/tools/src/tools/annotation/CircleROITool.tspackages/tools/src/tools/annotation/RectangleROITool.tspackages/tools/src/tools/base/BaseTool.tspackages/tools/src/types/IBaseTool.tspackages/tools/src/types/index.tspackages/tools/test/CircleROI_test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tools/src/tools/annotation/CircleROITool.ts
- packages/tools/src/tools/annotation/RectangleROITool.ts
Closed
6 tasks
Member
|
The target-selection API has been reworked per the review feedback - see #2802 for the alternative implementation (targets this branch, so its diff is just the architecture change). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
…ults # Conflicts: # packages/tools/src/utilities/index.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tools/src/tools/base/BaseTool.ts (1)
615-630: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against undefined
cachedStatsinensureCachedStatsTargets.
data.cachedStatsis optional onAnnotationData, but line 621 casts it directly and line 623 indexes into it without a null check. If an annotation is created or hydrated withoutcachedStats(e.g., from external persistence), this throws aTypeError. The similarObject.keys(data?.cachedStats)issue was previously fixed ingetMeasurementTargets; this method needs the same guard.🛡️ Proposed fix
protected ensureCachedStatsTargets( data: AnnotationData, targetIds: string[], needsUpdate?: (stats) => boolean ): boolean { let missing = false; + if (!data.cachedStats) { + data.cachedStats = {}; + } const cachedStats = data.cachedStats as Record<string, unknown>;🤖 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 `@packages/tools/src/tools/base/BaseTool.ts` around lines 615 - 630, Update ensureCachedStatsTargets in BaseTool to safely handle missing or undefined data.cachedStats before indexing target IDs. Initialize or otherwise provide an empty mutable stats record when cachedStats is absent, then preserve the existing needsUpdate checks and missing-target behavior.
🤖 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 @.claude/skills/cornerstone3d-dev/SKILL.md:
- Around line 67-71: Update the cross-session project memory guidance in
SKILL.md to use a generic, configured memory location rather than the
user-specific path. Remove the embedded personal project key and all
transcript/session IDs from the Evidence section, while preserving the
instruction to read and persist durable project memory.
- Around line 23-31: Update the type-refresh and typecheck commands in the build
workflow so they are cwd-independent and execute from the repository root
context, avoiding a persistent `cd packages/core`. Use subshells or equivalent
`pnpm --dir` invocations for `tsc` and the tools fallback, while preserving the
existing projects and declaration-only behavior.
In `@packages/core/examples/genericDynamicVolume/index.ts`:
- Around line 100-131: Guard both toolbar callbacks against incomplete
initialization in run: have the orientation handler verify getViewport() returns
a viewport before calling setOrientation or render, and have the Play/Stop
handler verify volume is initialized before starting or advancing the cine
interval. Preserve existing behavior once the viewport and volume are ready.
In
`@packages/docs/docs/concepts/cornerstone-tools/annotation/measurementTargets.md`:
- Around line 103-105: Update the measurement-target documentation to reference
NON_PIXEL_DATA_MODALITIES as its standalone export from
measurementTargetFilters.ts, not as a property of measurementTargetFilters.
Since it is not publicly re-exported by `@cornerstonejs/tools`, avoid presenting
it as a public API property and describe the non-pixel modalities directly or
use the supported public exports.
---
Outside diff comments:
In `@packages/tools/src/tools/base/BaseTool.ts`:
- Around line 615-630: Update ensureCachedStatsTargets in BaseTool to safely
handle missing or undefined data.cachedStats before indexing target IDs.
Initialize or otherwise provide an empty mutable stats record when cachedStats
is absent, then preserve the existing needsUpdate checks and missing-target
behavior.
🪄 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: 792b6fa9-3722-4d93-8b52-bbe6dd3c6b2c
📒 Files selected for processing (17)
.claude/skills/cornerstone3d-dev/SKILL.mdpackages/core/examples/genericDynamicVolume/index.tspackages/core/examples/tmtv/index.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/test/planarViewportState.jest.jspackages/docs/docs/concepts/cornerstone-tools/annotation/measurementTargets.mdpackages/tools/examples/petCt/index.tspackages/tools/src/index.tspackages/tools/src/tools/annotation/CircleROITool.tspackages/tools/src/tools/annotation/RectangleROITool.tspackages/tools/src/tools/base/AnnotationTool.tspackages/tools/src/tools/base/BaseTool.tspackages/tools/src/tools/base/index.tspackages/tools/src/tools/base/measurementTargetFilters.tspackages/tools/src/tools/index.tspackages/tools/src/types/IBaseTool.tspackages/tools/test/measurementTargets.jest.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tools/examples/petCt/index.ts
- packages/tools/src/tools/annotation/CircleROITool.ts
- packages/tools/src/types/IBaseTool.ts
- packages/core/examples/tmtv/index.ts
targetsFilter now receives the viewport's candidate display sets and returns the subset to measure, instead of being called per candidate with a true/false/'useAndStop'/'stop' return. Drops the MeasurementTargetsFilterResult union and the per-candidate 'previous' field; the built-in filters become plain array transforms.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
sedghi
reviewed
Jul 15, 2026
sedghi
approved these changes
Jul 15, 2026
sedghi
left a comment
sedghi
left a comment
Member
There was a problem hiding this comment.
i think docs should be updated though
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
This PR adds display of measurements from multiple volumes onto a fused display.
Note this currently only works for acquisition orientation on volume viewports because the restrictions for display of the annotations are such that they don't work on the secondary viewports for non-acquisition. That is being fixed as a separate PR/issue.
Changes & Results
Add a generic method for get text lines
Add support for displaying stats from different volumes or multiple volumes
Add a getMeasurementTargets to allow getting a list of volumes to use for this.
Testing
Run tmtv mode before/after the change
Create a rectangle roi on each of the three orientations.
In the old version, only one measurement from both volumes would appear, whereas now both of them appear.
Note: Area measurement is unchanged - it is incorrect on both old/new.
Try again with circular measurement.
For annotations drawn on the center row, you may need to navigate the top row some as the distance between annotations and thus the distance that a single annotation might be shown changes depending on the differing slice thickness.
Annotations must have been displayed in both top and second row at some point in order to allow displaying measurements in the bottom row.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit