fix(tools): only reject a preview the tool actually has - #2891
fix(tools): only reject a preview the tool actually has#2891TFRadicalImaging wants to merge 1 commit into
Conversation
`LabelmapBaseTool.previewData` is a single static object shared by every labelmap tool instance, so once any brush has painted, `previewData.element` is set for all of them. `rejectPreview` took that element as its only condition and ran the tool's RejectPreview strategy regardless of whether that tool had a preview. For the slice-wise tools this is wasted work: the reject handler undoes a memo only when it carries preview voxels. For the 3D variants it throws. `ensureSegmentationVolumeFor3DManipulation` raises "Volume is not reconstructable for sphere manipulation" whenever the viewport cannot form a volume, and `onSetToolPassive` rejects the preview — so simply deactivating a sphere brush on a single-frame series (a DX/CR image, a cine, an unevenly spaced stack) throws, as long as some brush has painted before. The throw escapes `ToolGroup.setToolPassive` from inside `setToolActive`, so the tool being activated in its place never is and the tool group is left with no active primary tool: the toolbar shows no tool armed and a brush cannot be selected again. Run the strategy only when there is a preview to reject. The edit memo is still closed and the shared preview state still reset on every call.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
ChangesPreview rejection handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change prevents brush deactivation from throwing when no preview exists, so users can switch brush tools normally while preserving rejection of real previews. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description clearly explains the shared preview-state defect, the code changes, expected behavior, test coverage, manual verification, and tested environment. All required template sections are present and substantially complete. Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3 files. ✨ 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 |
1 participant
Context
LabelmapBaseTool.previewDatais a single static object shared by every labelmap tool instance:So as soon as any brush has painted,
previewData.elementis set for all of them — the circular brush, the sphere brush, the erasers, the threshold brushes.rejectPreviewtook that element as its only condition and ran the tool'sRejectPreviewstrategy regardless of whether that tool had a preview (previewData.previewis null in this state).For the slice-wise tools that is wasted work: the reject handler undoes a history memo only when the memo carries preview voxels.
For the 3D variants it throws.
ensureSegmentationVolumeFor3DManipulationraisesVolume is not reconstructable for sphere manipulationwheneverisValidVolume(viewport.getImageIds())is false, andBrushTool.onSetToolPassive→disableCursor()→rejectPreview(). So deactivating a sphere brush on a series that cannot form a volume (a single-frame DX/CR image, a cine, an unevenly spaced stack) throws — as long as some brush painted first.The throw escapes
ToolGroup.setToolPassive(), which is called fromsetToolActive()before the new tool is armed. The result on a real viewer: the tool group ends with no active primary tool, the toolbar shows nothing selected, the brush's options disappear and the brush cannot be selected again — clicking it re-arms the sphere, which throws on the next deactivation, and so on.Measured live (OHIF-based viewer,
@cornerstonejs/tools5.6.8, CR chest, one image per series,isValidVolume: false), instrumentingToolGroup.setToolActive/setToolPassiveandapplyActiveStrategyCallback:with
previewData.preview === nullandpreviewData.elementset by the earlier circular-brush stroke.Changes & Results
Run the
RejectPreviewstrategy only when the tool has a preview to reject, in bothBrushTool.rejectPreview(which overrides it) andLabelmapBaseTool.rejectPreview. The edit memo is still closed and the shared preview state still fully reset on every call, so undo/redo grouping is unchanged.Before: deactivating a sphere brush after any brush stroke, on a viewport that cannot form a volume, throws and leaves the tool group with no active tool.
After: the deactivation is silent and the replacement tool is armed as usual.
The sibling
acceptPreviewhas the same unconditional shape, but it is only reached from explicit accept paths (_endCallbackfor a non-drag interaction, the Enter action), so it is left alone here — happy to include it if you would rather have both symmetric.Testing
New
packages/tools/src/tools/segmentation/__tests__/rejectPreview.spec.ts, run against both classes:elementand there is no preview (the crash path), whiledoneEditMemostill runs;pnpm jest --selectProjects tools: 50 suites / 746 tests pass (2 pre-existing skips). Negative check: restoring the unconditional call fails exactly the two "leaves the strategy alone" cases.Manual: on a single-image CR/DX series, create a labelmap segmentation, arm the brush, paint one stroke, then switch the brush shape between Circle and Sphere. Before the change the switch throws and the brush ends up unarmed with its Shape/Radius options gone; after it, the brush stays armed.
Checklist
PR
Code
Public Documentation Updates
Tested Environment
@wayfarer3130 assigning this one to you, as with #2880 — it is the upstream half of a sphere-brush defect we are closing downstream.
Summary by CodeRabbit