DYN-9599 : Uniform undo behavior restores nodes + pinned notes. by CustomBuildingConfigurators-Balaji · Pull Request #16920 · DynamoDS/Dynamo

CustomBuildingConfigurators-Balaji · 2026-02-25T01:52:55Z

Purpose

DYN-9599 : Uniform undo behavior restores nodes + pinned notes.

Currently the Undo behavior for the deletion of a node with an attached pinned varies depending on whether the node is deleted using Delte Key or context menu. Make Undo (Ctrl+Z) uniformly restore a deleted node and re-establish pinned note association in one step, preserving each note’s content, pin state, and relative position, regardless of whether deletion was via Delete key or context menu. (DYN -9599 )

When undoing deletion of a node with pinned note(s), restore the node and note(s) in a single undo step as below:
Re-pin note(s) to the node, preserving:
1. note content
2. pin state (pinned=true)
3. relative position offset to the node
4. group membership if applicable
Redo restores the original deleted state consistently for both entry points.
Multi-selection delete: handle multiple nodes, each with 0..n pinned notes.

DYN-9599.mp4

Declarations

Check these if you believe they are true

Is documented according to the standards
The level of testing this PR includes is appropriate
Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Release Notes

Uniform undo behavior to restores nodes and pinned notes

Reviewers

@zeusongit

FYIs

@stevecbc

github-actions

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9599

1. Placed the functions next to each other 2. Used Any() instead of Count()

zeusongit · 2026-02-25T19:29:45Z

            SetNodeCountOptimizationEnabled(zoomAnimationThresholdFeatureFlagVal);
        }
-
+        


can be cleaned up

Copilot

Pull request overview

This PR targets Dynamo’s undo/redo consistency in the WPF workspace layer by ensuring that undoing deletion of a node also restores any pinned notes and re-establishes their pin association uniformly (independent of delete entry point).

Changes:

Adds logic in WorkspaceViewModel to resolve and re-establish pinned note ↔ node references during undo when models are recreated out-of-order.
Invokes the note pin command when either the note is created before its node (or vice-versa) during undo replay.

Copilot · 2026-02-25T19:38:17Z

+                    DynamoSelection.Instance.Selection.Add(pinnedNode.First().NodeModel);
+                    viewModel.PinToNodeCommand.Execute(null);
+                }


Calling PinToNodeCommand.Execute(null) during undo-repair will run NoteViewModel.PinToNode, which records a new undo action group (WorkspaceModel.RecordModelForModification) and sets HasUnsavedChanges. That can pollute/corrupt the undo/redo stacks and break the requirement that restoring a deleted node + pinned notes happens in a single undo step. Consider re-establishing the pin relationship without recording undo (e.g., a dedicated non-undo-recording helper for undo replay).

makes sense

Added a internal property SuppressUndoRecording which is used to skip the WorkspaceModel.RecordModelForModification call that stores information for Undo/Redo.
We set this property to False by passing an argument to TryToSubscribeUndoNote function.

Replace PinToNodeCommand invocation with TryToSubscribeUndoNote function call.

Copilot · 2026-02-25T19:38:17Z

+                    DynamoSelection.Instance.Selection.Clear();
+                    DynamoSelection.Instance.Selection.Add(node);
+                    note.PinToNodeCommand.Execute(null);
+                }


Same issue as above: DynamoSelection.Instance.Selection.Clear() can leave stale IsSelected flags because it bypasses DynamoSelection.ClearSelection() (which explicitly deselects items and respects ClearSelectionDisabled). Use DynamoSelection.Instance.ClearSelection() instead.

I would agree: Use DynamoSelection.Instance.ClearSelection() instead.

Removed the usage DynamoSelection.Instance.Selection property because it will be handled in note_PinUnpinToNode function.

Copilot · 2026-02-25T19:38:17Z

+                    DynamoSelection.Instance.Selection.Clear();
+                    DynamoSelection.Instance.Selection.Add(node);
+                    note.PinToNodeCommand.Execute(null);


PinToNodeCommand.Execute(null) here will record a new undo action group via NoteViewModel.PinToNode and mark the workspace dirty, even though this is intended to be part of undo replay. This risks adding extra undo steps / clearing redo history. Reconnect pinned notes to the restored node without generating new undo records.

Suggested change

DynamoSelection.Instance.Selection.Clear();

DynamoSelection.Instance.Selection.Add(node);

note.PinToNodeCommand.Execute(null);

// Reconnect the note to the restored node directly on the model to avoid

// generating new undo records or marking the workspace dirty during undo.

note.Model.PinnedNode = node;

makes sense

Added a internal property SuppressUndoRecording which is used to skip the WorkspaceModel.RecordModelForModification call that stores information for Undo/Redo.
We set this property to False by passing an argument to TryToSubscribeUndoNote function.

Copilot · 2026-02-25T19:38:17Z

        private void Model_NoteAdded(NoteModel note)
        {
            var viewModel = new NoteViewModel(this, note);
+            ResolvePinnedNodeReference(note, viewModel);
            Notes.Add(viewModel);


This change introduces new undo/redo behavior (restoring node deletion should also restore pinned notes uniformly and in a single undo step, including multi-select and redo). There are existing WPF tests around note pinning/undo, but no coverage found for deleting node(s) with pinned note(s) and undo/redo from both delete entry points. Add UI/unit coverage for: delete via Delete key vs context menu, multi-node deletion, undo restores pin+position+group membership, and redo re-deletes consistently.

Added a internal property SuppressUndoRecording which is used to skip the WorkspaceModel.RecordModelForModification call that stores information for Undo/Redo.
We set this property to False by passing an argument to TryToSubscribeUndoNote function.

Copilot · 2026-02-25T19:38:18Z

+                    DynamoSelection.Instance.Selection.Clear();
+                    DynamoSelection.Instance.Selection.Add(pinnedNode.First().NodeModel);
+                    viewModel.PinToNodeCommand.Execute(null);
+                }


Using DynamoSelection.Instance.Selection.Clear() here bypasses DynamoSelection.ClearSelection() (which explicitly deselects items and honors ClearSelectionDisabled). Because SmartObservableCollection.Clear() raises a reset without old items, this can leave previous items' IsSelected state out of sync. Use DynamoSelection.Instance.ClearSelection() (and ideally restore prior selection after repinning).

I would agree: Use DynamoSelection.Instance.ClearSelection() instead.

Removed the usage DynamoSelection.Instance.Selection property because it will be handled in note_PinUnpinToNode function.

zeusongit · 2026-02-25T20:08:42Z

@CustomBuildingConfigurators-Balaji a few comments by me and copilot, PTAL

zeusongit · 2026-02-25T20:39:50Z

Code review

Found 1 issue:

DynamoSelection.Instance.Selection.Clear() should be DynamoSelection.Instance.ClearSelection(). Both ResolvePinnedNodeReference overloads call Selection.Clear() directly, which bypasses the Deselect() call on each item and ignores the ClearSelectionDisabled flag. This leaves IsSelected = true on removed items, causing stale selection state. Every other call site in this file (lines 1165, 1791, 1816, 1838, 1856) and the analogous note_PinUnpinToNode handler in NoteViewModel.cs use ClearSelection() instead.

Dynamo/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

Lines 947 to 951 in ff878e2

    
           { 
        
               DynamoSelection.Instance.Selection.Clear(); 
        
               DynamoSelection.Instance.Selection.Add(pinnedNode.First().NodeModel); 
        
               viewModel.PinToNodeCommand.Execute(null); 
        
           }

Dynamo/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

Lines 969 to 973 in ff878e2

    
           { 
        
               DynamoSelection.Instance.Selection.Clear(); 
        
               DynamoSelection.Instance.Selection.Add(node); 
        
               note.PinToNodeCommand.Execute(null); 
        
           }

For reference, ClearSelection() calls Deselect() on every item and respects ClearSelectionDisabled:

Dynamo/src/DynamoCore/Core/DynamoSelection.cs

Lines 95 to 101 in ff878e2

    
           public void ClearSelection() 
        
           { 
        
               if (ClearSelectionDisabled) return; 
        
               Instance.Selection.ToList().ForEach(x => x.Deselect()); 
        
               Instance.Selection.Clear(); 
        
           }

Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

zeusongit · 2026-02-25T21:06:54Z

+                {
+                    DynamoSelection.Instance.Selection.Clear();
+                    DynamoSelection.Instance.Selection.Add(pinnedNode.First().NodeModel);
+                    viewModel.PinToNodeCommand.Execute(null);


PinToNodeCommand.Execute() relies on DelegateCommand bypassing CanExecute this is diverging from existing pattern note_PinUnpinToNode which calls PinToNode() directly

Replaced PinToNodeCommand invocation with TryToSubscribeUndoNote function in NoteModel.cs which in turn calls note_PinUnpinToNode function.

jasonstratton · 2026-03-04T00:03:38Z

@zeusongit , looks like you reviewed this last week, but it's not approved. Is there more work to do? Should I review it?

zeusongit · 2026-03-04T00:55:21Z

@zeusongit , looks like you reviewed this last week, but it's not approved. Is there more work to do? Should I review it?

Yes, I have requested some changes, feel free to review more though.

Removed direct invocation of PinToNodeCommand and used TryToSubscribeUndoNote function in NoteModel.cs Removed the code added for DynamoSelection.Instance.Selection since we do not need it and it will be handled by note_PinUnpinToNode function in NoteViewModel.cs

sonarqubecloud · 2026-03-04T13:38:23Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Copilot · 2026-03-05T00:57:28Z

+                var pinnedNode = Nodes.Where(x => x.Id.Equals(note.PinnedNodeGuid));
+                if (pinnedNode.Any())
+                {
+                    note.TryToSubscribeUndoNote(recordForUndo: false);                    
+                }


ResolvePinnedNodeReference(NoteModel) enumerates Nodes without taking the same lock (Nodes) that is used elsewhere in this class for adding/removing/clearing nodes. If nodes can be modified concurrently (which the existing locking suggests), this LINQ enumeration can throw "Collection was modified" during undo replay. Consider wrapping the Nodes lookup in lock (Nodes) (or using a thread-safe snapshot like Nodes.ToList() under the lock) before calling TryToSubscribeUndoNote.

Suggested change

var pinnedNode = Nodes.Where(x => x.Id.Equals(note.PinnedNodeGuid));

if (pinnedNode.Any())

{

note.TryToSubscribeUndoNote(recordForUndo: false);

}

bool hasPinnedNode;

lock (Nodes)

{

hasPinnedNode = Nodes.Any(x => x.Id.Equals(note.PinnedNodeGuid));

}

if (hasPinnedNode)

{

note.TryToSubscribeUndoNote(recordForUndo: false);

}

@zeusongit, Practically this is not an issue because this is function is executed during undo operation and there is no chance of collection getting modified while executing the above specified lines.
Let me know if you would like to implement this.

zeusongit · 2026-03-05T14:35:50Z

passed here: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/19378/

DYN-9599 (DynamoDS#16920) landed on master concurrently and added deferred pin restoration when a deleted node+note pair is restored via undo. It introduced SuppressUndoRecording on NoteModel and ResolvePinnedNodeReference on WorkspaceViewModel, both of which rely on TryToSubscribeUndoNote and the UndoRequest event that DYN-10130 had removed. Resolution: - Restore UndoRequest event, note_PinUnpinToNode handler, and TryToSubscribeUndoNote(recordForUndo) from DYN-9599 so that ResolvePinnedNodeReference continues to work for the undo-of-deletion and file-load deferred resolution paths. - Keep SuppressUndoRecording from DYN-9599 and guard PinToNode's RecordModelsForModification (note + group) behind it so that deferred restoration does not pollute the undo stack. - Keep DYN-10130's Workspace property and DeserializeCore direct resolution, which handles the undo-of-modification path and also sets PinnedNodeGuid so ResolvePinnedNodeReference detects pending pins in the Workspace-null paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

CustomBuildingConfigurators-Balaji added 2 commits February 25, 2026 06:27

Added functions to resolve pinned Note with Nodes during undo operation.

1153432

Merge branch 'master' into DYN-9599

d7685fd

CustomBuildingConfigurators-Balaji requested a review from a team February 25, 2026 01:52

CustomBuildingConfigurators-Balaji self-assigned this Feb 25, 2026

github-actions Bot reviewed Feb 25, 2026

View reviewed changes

Resolved SonarQube comments

ff878e2

1. Placed the functions next to each other 2. Used Any() instead of Count()

zeusongit reviewed Feb 25, 2026

View reviewed changes

zeusongit requested review from a team and Copilot February 25, 2026 19:29

Copilot started reviewing on behalf of zeusongit February 25, 2026 19:30 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes

zeusongit reviewed Feb 25, 2026

View reviewed changes

Merge branch 'master' into DYN-9599

c850f1e

CustomBuildingConfigurators-Balaji added 5 commits March 4, 2026 18:26

Merge branch 'master' into DYN-9599

6f9812a

Remove unnecessary whitespace in WorkspaceViewModel.cs

ad27ac9

Fix indentation in NoteViewModel.cs

d848ec7

Removed the unused parameter in ResolvePinnedNodeReference function

90fb86c

CustomBuildingConfigurators-Balaji requested a review from zeusongit March 4, 2026 19:15

zeusongit requested a review from Copilot March 5, 2026 00:44

Copilot started reviewing on behalf of zeusongit March 5, 2026 00:44 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

zeusongit approved these changes Mar 5, 2026

View reviewed changes

zeusongit merged commit 5132638 into DynamoDS:master Mar 5, 2026
30 of 33 checks passed

CustomBuildingConfigurators-Balaji deleted the DYN-9599 branch March 6, 2026 05:36

jasonstratton mentioned this pull request Mar 10, 2026

DYN-10130 Pinning a Dynamo Note can not be undone #16942

Merged

		SetNodeCountOptimizationEnabled(zoomAnimationThresholdFeatureFlagVal);
		}

Conversation

CustomBuildingConfigurators-Balaji commented Feb 25, 2026 • edited by zeusongit Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusongit commented Feb 25, 2026

Uh oh!

zeusongit commented Feb 25, 2026

Code review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonstratton commented Mar 4, 2026

Uh oh!

zeusongit commented Mar 4, 2026

Uh oh!

sonarqubecloud Bot commented Mar 4, 2026

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

CustomBuildingConfigurators-Balaji Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CustomBuildingConfigurators-Balaji commented Feb 25, 2026 •

edited by zeusongit

Loading

CustomBuildingConfigurators-Balaji Mar 5, 2026 •

edited

Loading