Fix segmentation save dialog#6116
Open
aayandeb wants to merge 4 commits into
Open
Conversation
Replace the database cylinder with a file save icon so the panel action reads as saving a segmentation rather than selecting storage. Register the icon in ui-next for reuse. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a new ChangesSegmentation Save Button & Report Dialog Rework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
🤖 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/panels/PanelSegmentation.tsx`:
- Around line 236-239: The save modality is being derived from
segmentationRepresentationTypes[0] instead of the currently selected
segmentation, which can make the Save flow use the wrong modality for mixed
representations. Update PanelSegmentation so saveModality is computed from
selectedSegmentationIdForType and its matching representation entry, and ensure
the Save handler in the same component passes that selected modality through the
downstream store flow that builds the save dialog path and default filename.
In `@extensions/default/src/customizations/reportDialogCustomization.tsx`:
- Around line 148-239: The new save-flow text in reportDialogCustomization.tsx
is hard-coded instead of using the existing i18n setup, so localize the added
copy with react-i18next. Update the labels, placeholder, and helper text in the
save-mode UI around the Select, SelectTrigger/SelectValue, InputDialog, and the
“Series name cannot be changed here” message to use translation keys via the
same translation hook already used in this component.
- Around line 49-80: The existing series state in reportDialogCustomization is
derived from a one-time getDisplaySetCache snapshot, so existingSeriesOptions,
saveMode, and selectedSeries can become stale when display sets change after
mount. Update the logic around existingSeriesOptions, hasExistingSeries, and the
selected-series defaults to subscribe to displaySetService updates instead of
relying on useMemo over the cache snapshot. Recompute and reconcile the derived
selection whenever the service publishes changes, and ensure the subscription is
cleaned up on unmount so the “default to existing when available” behavior stays
in sync.
- Around line 213-236: The new-series input in reportDialogCustomization’s save
mode flow still only switches via the wrapper click, so keyboard interaction
cannot activate it. Update the saveMode handling around the
InputDialog/InputDialog.Input block so entering or focusing the “Series name”
field when saveMode is existing calls switchToNew, and keep the existing click
behavior as well. Use the existing symbols switchToNew, saveMode, and
InputDialog.Input to locate the fix.
🪄 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: f1673b88-0770-442b-9032-9303a111c16d
📒 Files selected for processing (4)
extensions/cornerstone/src/panels/PanelSegmentation.tsxextensions/default/src/customizations/reportDialogCustomization.tsxplatform/ui-next/src/components/Icons/Icons.tsxplatform/ui-next/src/components/Icons/Sources/Save.tsx
Comment on lines
+236
to
+239
| const saveModality = | ||
| segmentationRepresentationTypes?.[0] === SegmentationRepresentations.Contour | ||
| ? 'RTSTRUCT' | ||
| : 'SEG'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive saveModality from the selected representation, not the first configured type.
Line 236 always keys off segmentationRepresentationTypes?.[0], but the active selectedSegmentationIdForType can come from any entry in that array. In a mixed labelmap/contour panel, the Save click at Line 303 can therefore send the wrong modality for the selected segmentation, and the downstream store flow uses that field to build the save dialog path and default filename.
Suggested fix
- const saveModality =
- segmentationRepresentationTypes?.[0] === SegmentationRepresentations.Contour
+ const selectedRepresentationType = segmentationRepresentationTypes?.find(
+ type => selectedSegmentationsForViewportMap?.get(type) === selectedSegmentationIdForType
+ );
+
+ const saveModality =
+ selectedRepresentationType === SegmentationRepresentations.Contour
? 'RTSTRUCT'
: 'SEG';Also applies to: 303-309
🤖 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/cornerstone/src/panels/PanelSegmentation.tsx` around lines 236 -
239, The save modality is being derived from segmentationRepresentationTypes[0]
instead of the currently selected segmentation, which can make the Save flow use
the wrong modality for mixed representations. Update PanelSegmentation so
saveModality is computed from selectedSegmentationIdForType and its matching
representation entry, and ensure the Save handler in the same component passes
that selected modality through the downstream store flow that builds the save
dialog path and default filename.
Comment on lines
+49
to
+80
| const existingSeriesOptions = useMemo(() => { | ||
| const displaySetsMap = displaySetService.getDisplaySetCache(); | ||
| const displaySets = Array.from(displaySetsMap.values()); | ||
| const options = displaySets | ||
| return displaySets | ||
| .filter(ds => ds.Modality === modality) | ||
| .map(ds => ({ | ||
| value: ds.predecessorImageId || ds.SeriesInstanceUID, | ||
| seriesNumber: isFinite(ds.SeriesNumber) ? ds.SeriesNumber : minSeriesNumber, | ||
| description: ds.SeriesDescription, | ||
| description: ds.SeriesDescription ?? '', | ||
| label: `${ds.SeriesDescription} ${ds.SeriesDate}/${ds.SeriesTime} ${ds.SeriesNumber}`, | ||
| })); | ||
| }, [displaySetService, modality, minSeriesNumber]); | ||
|
|
||
| return [ | ||
| { | ||
| value: null, | ||
| description: null, | ||
| seriesNumber: minSeriesNumber, | ||
| label: 'Create new series', | ||
| }, | ||
| ...options, | ||
| ]; | ||
| }, [displaySetService, modality]); | ||
| const hasExistingSeries = existingSeriesOptions.length > 0; | ||
|
|
||
| useEffect(() => { | ||
| const seriesOption = seriesOptions.find(s => s.value === selectedSeries); | ||
| const newReportName = | ||
| selectedSeries && seriesOption?.description ? seriesOption.description : ''; | ||
| setReportName(newReportName); | ||
| }, [selectedSeries, seriesOptions]); | ||
| const [saveMode, setSaveMode] = useState<SaveMode>(hasExistingSeries ? 'existing' : 'new'); | ||
| const [selectedSeries, setSelectedSeries] = useState<string | null>( | ||
| predecessorImageId | ||
| ? (existingSeriesOptions.find(s => s.value === predecessorImageId)?.value ?? | ||
| existingSeriesOptions[0]?.value ?? | ||
| null) | ||
| : (existingSeriesOptions[0]?.value ?? null) | ||
| ); | ||
| const [newSeriesName, setNewSeriesName] = useState(''); | ||
|
|
||
| const priorSeriesNumber = useMemo( | ||
| () => Math.max(minSeriesNumber, ...existingSeriesOptions.map(s => s.seriesNumber)), | ||
| [existingSeriesOptions, minSeriesNumber] | ||
| ); | ||
|
|
||
| const existingSeriesDesc = useMemo( | ||
| () => existingSeriesOptions.find(s => s.value === selectedSeries)?.description ?? '', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Subscribe to displaySetService instead of memoizing a cache snapshot.
getDisplaySetCache() is only read during render, and this memo depends on the service object rather than cache updates. That leaves existingSeriesOptions, the default saveMode, and selectedSeries stale if display sets arrive/change after mount, which breaks the “default to existing when available” flow. Drive this from a service subscription and reconcile the derived selection when the service publishes changes. As per coding guidelines, "Prioritize pub-sub by calling service subscribe over useEffects for more reliable event handling, unsubscribing in cleanup functions".
🤖 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/default/src/customizations/reportDialogCustomization.tsx` around
lines 49 - 80, The existing series state in reportDialogCustomization is derived
from a one-time getDisplaySetCache snapshot, so existingSeriesOptions, saveMode,
and selectedSeries can become stale when display sets change after mount. Update
the logic around existingSeriesOptions, hasExistingSeries, and the
selected-series defaults to subscribe to displaySetService updates instead of
relying on useMemo over the cache snapshot. Recompute and reconcile the derived
selection whenever the service publishes changes, and ensure the subscription is
cleaned up on unmount so the “default to existing when available” behavior stays
in sync.
Source: Coding guidelines
Comment on lines
+148
to
+239
| {showDataSourceSelect && ( | ||
| <div> | ||
| <div className="mb-1 pl-1 text-base">Data source</div> | ||
| <Select | ||
| value={selectedDataSource} | ||
| onValueChange={setSelectedDataSource} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select a data source" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {dataSources.map(source => ( | ||
| <SelectItem | ||
| key={source.value} | ||
| value={source.value} | ||
| > | ||
| {source.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Save to existing series */} | ||
| <div> | ||
| <label className="mb-2 flex cursor-pointer items-center gap-2"> | ||
| <input | ||
| type="radio" | ||
| name="save-mode" | ||
| checked={saveMode === 'existing'} | ||
| onChange={() => setSaveMode('existing')} | ||
| disabled={!hasExistingSeries} | ||
| className="cursor-pointer" | ||
| /> | ||
| <span className={!hasExistingSeries ? 'text-muted-foreground' : ''}> | ||
| Save to existing series | ||
| </span> | ||
| </label> | ||
| <div className="pl-6"> | ||
| <Select | ||
| value={selectedSeries} | ||
| onValueChange={setSelectedSeries} | ||
| disabled={saveMode !== 'existing' || !hasExistingSeries} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue | ||
| placeholder={hasExistingSeries ? 'Select a series' : 'No existing series'} | ||
| /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {existingSeriesOptions.map(series => ( | ||
| <SelectItem | ||
| key={series.value} | ||
| value={series.value} | ||
| > | ||
| {series.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| </div> | ||
| <div className="flex items-end gap-4"> | ||
| {!showDataSourceSelect && ( | ||
| <div className="w-1/3"> | ||
| <div className="mb-1 pl-1 text-base">Series</div> | ||
| <Select | ||
| value={selectedSeries} | ||
| onValueChange={setSelectedSeries} | ||
| > | ||
| <SelectTrigger> | ||
| <SelectValue placeholder="Select a series" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {seriesOptions.map(series => ( | ||
| <SelectItem | ||
| key={series.value} | ||
| value={series.value} | ||
| > | ||
| {series.label} | ||
| </SelectItem> | ||
| ))} | ||
| </SelectContent> | ||
| </Select> | ||
| </div> | ||
| )} | ||
| <InputDialog | ||
| value={reportName} | ||
| onChange={setReportName} | ||
| submitOnEnter | ||
| className="flex-1" | ||
| > | ||
| <InputDialog.Field className="mb-0"> | ||
| <InputDialog.Input | ||
| placeholder="Report name" | ||
| disabled={!!selectedSeries} | ||
| /> | ||
| </InputDialog.Field> | ||
| </InputDialog> | ||
|
|
||
| {/* Create new series — entire section clickable to switch mode */} | ||
| <div onClick={saveMode === 'existing' ? switchToNew : undefined}> | ||
| <label className="mb-2 flex cursor-pointer items-center gap-2"> | ||
| <input | ||
| type="radio" | ||
| name="save-mode" | ||
| checked={saveMode === 'new'} | ||
| onChange={switchToNew} | ||
| className="cursor-pointer" | ||
| /> | ||
| <span>Create new series</span> | ||
| </label> | ||
| <div className="pl-6"> | ||
| <InputDialog | ||
| value={saveMode === 'existing' ? existingSeriesDesc : newSeriesName} | ||
| onChange={saveMode === 'new' ? setNewSeriesName : undefined} | ||
| submitOnEnter | ||
| > | ||
| <InputDialog.Field className="mb-0"> | ||
| <InputDialog.Input | ||
| placeholder="Series name" | ||
| disabled={saveMode === 'existing'} | ||
| /> | ||
| </InputDialog.Field> | ||
| </InputDialog> | ||
| {saveMode === 'existing' && ( | ||
| <p className="text-muted-foreground mt-1 pl-1 text-xs"> | ||
| Series name cannot be changed here |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Localize the new dialog copy.
The new labels, placeholders, and helper text here are hard-coded even though this component already uses react-i18next. That makes the new save flow English-only while the rest of the dialog is localized.
🤖 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/default/src/customizations/reportDialogCustomization.tsx` around
lines 148 - 239, The new save-flow text in reportDialogCustomization.tsx is
hard-coded instead of using the existing i18n setup, so localize the added copy
with react-i18next. Update the labels, placeholder, and helper text in the
save-mode UI around the Select, SelectTrigger/SelectValue, InputDialog, and the
“Series name cannot be changed here” message to use translation keys via the
same translation hook already used in this component.
Comment on lines
+213
to
+236
| <div onClick={saveMode === 'existing' ? switchToNew : undefined}> | ||
| <label className="mb-2 flex cursor-pointer items-center gap-2"> | ||
| <input | ||
| type="radio" | ||
| name="save-mode" | ||
| checked={saveMode === 'new'} | ||
| onChange={switchToNew} | ||
| className="cursor-pointer" | ||
| /> | ||
| <span>Create new series</span> | ||
| </label> | ||
| <div className="pl-6"> | ||
| <InputDialog | ||
| value={saveMode === 'existing' ? existingSeriesDesc : newSeriesName} | ||
| onChange={saveMode === 'new' ? setNewSeriesName : undefined} | ||
| submitOnEnter | ||
| > | ||
| <InputDialog.Field className="mb-0"> | ||
| <InputDialog.Input | ||
| placeholder="Series name" | ||
| disabled={saveMode === 'existing'} | ||
| /> | ||
| </InputDialog.Field> | ||
| </InputDialog> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Typing into the new-series section still cannot switch modes.
Only the wrapper click calls switchToNew. While saveMode === 'existing', the field is disabled, so keyboard users cannot focus it and start typing to auto-select “Create new series,” which leaves the linked requirement unmet. Trigger the mode switch on first field interaction, not only on pointer clicks.
🤖 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/default/src/customizations/reportDialogCustomization.tsx` around
lines 213 - 236, The new-series input in reportDialogCustomization’s save mode
flow still only switches via the wrapper click, so keyboard interaction cannot
activate it. Update the saveMode handling around the
InputDialog/InputDialog.Input block so entering or focusing the “Series name”
field when saveMode is existing calls switchToNew, and keep the existing click
behavior as well. Use the existing symbols switchToNew, saveMode, and
InputDialog.Input to locate the fix.
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
Fixes #6106
This change improves the segmentation save workflow described in #6106. The previous dialog mixed saving to an existing segmentation series with creating a new segmentation series, which made the save destination unclear.
This change improves the segmentation save workflow so users can more clearly understand where a saved segmentation will go.
Previously, the save flow did not clearly distinguish between saving into an existing series and creating a new series. The segmentation panel save action also used a database cylinder icon, which could read more like storage/data-source selection than a direct save action.
Changes & Results
Saveicon toui-next.Result: the segmentation save action is easier to recognize, and the destination dialog makes the save behavior clearer before the user confirms.
Testing
Not run locally.
Suggested manual test:
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
New Features
Bug Fixes