Skip to content

fix(HistoryMemo): Segmentation delete wasn't remembered#5775

Merged
wayfarer3130 merged 4 commits intomasterfrom
fix/OHIF-2461-OHIF-2460-segmentation-history-fix
Feb 5, 2026
Merged

fix(HistoryMemo): Segmentation delete wasn't remembered#5775
wayfarer3130 merged 4 commits intomasterfrom
fix/OHIF-2461-OHIF-2460-segmentation-history-fix

Conversation

@wayfarer3130
Copy link
Contributor

Context

The delete of segment index wasn't being remembered for the history.
This requires cornerstonejs/cornerstone3D#2593 to be linked to work.

Note: Undo of a delete restores the segment index, and redo of a delete removes it.

Changes & Results

Adds a history memo for delete/restore of the segment index.

Does not add support for remembering other changes on segment index like rename.

Testing

Test for segment index for labelmap and contour indexes

Create segmentation (not undo create segmentation is NOT supported)
Draw a segment
Create a new segment index
Draw a segment
Create a new segment index
Draw a segment

Delete segment #1, then #2 then #3

Undo 6 times to remove all segment indices on screen
Segment indexes should appear as the 3 deletes are undone, AND the segment data should reappear.
Segments drawn should disappear as remaining 3 deletes are done.
NOTE: Segment index add is not recorded, so undoing it does nothing

Redo 6 times
Segments drawn should reappear in first 3 redo
Segments drawn should disappear as redo of next 3 deletes. This should remove the segment index from the table.

Test the whole kittencaboodle with a contours after doing labelmaps.
Maybe test undo/redo of multi frames of segmentations - this should delete all the single segment index on all frames at once.

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

@netlify
Copy link

netlify bot commented Feb 3, 2026

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 2bac9fd
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6983be97a126160008f8752f
😎 Deploy Preview https://deploy-preview-5775--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@cypress
Copy link

cypress bot commented Feb 4, 2026

Viewers    Run #5908

Run Properties:  status check passed Passed #5908  •  git commit 2bac9fdb03: fix: Use newer dcmjs version and delete segmentation with history cs3d
Project Viewers
Branch Review fix/OHIF-2461-OHIF-2460-segmentation-history-fix
Run status status check passed Passed #5908
Run duration 02m 17s
Commit git commit 2bac9fdb03: fix: Use newer dcmjs version and delete segmentation with history cs3d
Committer Bill Wallace
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 37
View all changes introduced in this branch ↗︎

@jbocce
Copy link
Collaborator

jbocce commented Feb 4, 2026

Testing Notes:

  • I was not able to undo the creation of a spline contour; that is once created clicking undo does not (eventually) remove the spline contour. Furthermore after several clicks of undo, several clicks of undo adds somewhat of an identical spline, sort of duplicating what is there
ScreenHunter.Feb.04.11.37.mp4
  • undo of Duplicate Contour does not work or is not implemented yet

@wayfarer3130 wayfarer3130 requested a review from jbocce February 4, 2026 22:22
@@ -1,5 +1,6 @@
{
"lockfileVersion": 1,
"configVersion": 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What version of bun are you running? Mine does not add this in to the lock file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.3.8 is what I run. I don't think it is a big deal to have this added unless it causes problems in earlier versions. The bun.lock is not hte official lockfile, but is for faster dev testing.

Object.entries(stats).forEach(([segmentIndex, segmentStats]) => {
const index = parseInt(segmentIndex);

if (!updatedSegmentation.segments[index]) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps safest to do if (!updatedSegmentation?.segments?.[index]) {?

*/
public removeSegment(segmentationId: string, segmentIndex: number): void {
cstSegmentation.removeSegment(segmentationId, segmentIndex);
public removeSegment(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why doesn't SegmentationService.addSegment memo/record its actions? Is it worth mentioning somewhere in the code comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I only implemented the delete operation as I was trying to minimize how many changes we have at this point. If we want, we can go ahead and fix all the remaining issues we've seen, but I specifically created a secondary issue to track things like the addSegment not adding the memo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Then maybe add a ToDo or something?

@jbocce jbocce requested a review from aimee-ferreira February 5, 2026 02:34
Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments. See my testing notes #5775 (comment). I added @aimee-ferreira as a reviewer for testing purposes.

@wayfarer3130
Copy link
Contributor Author

Added cornerstonejs/cornerstone3D#2599 to describe Joe's issue.

Copy link
Collaborator

@aimee-ferreira aimee-ferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verification has passed.

  • Tested undo/redo with both label map and contours + mixture - all deleted segments are added back to both the image and panel successfully
  • Tested when segment is hidden, delete and undo returns the segment in the hidden state, the segment can be re-displayed successfully

Copy link
Collaborator

@jbocce jbocce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving even though there are pending comments.

@wayfarer3130 wayfarer3130 merged commit 48f2f6f into master Feb 5, 2026
10 of 11 checks passed
@wayfarer3130 wayfarer3130 deleted the fix/OHIF-2461-OHIF-2460-segmentation-history-fix branch February 5, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants