Fix Alt-drag duplicate cleanup on right-click abort by jsjgdh · Pull Request #3782 · GraphiteEditor/Graphite

jsjgdh · 2026-02-17T22:01:45Z

discussion : https://discord.com/channels/@me/1461994742291234997/1473407462097948765 from DM with @Keavon

Keavon · 2026-02-17T23:47:17Z

/gemini review

Copilot

Pull request overview

This PR aims to ensure Alt-drag node duplication is cleaned up correctly when the drag is aborted (notably via right-click), by tracking whether duplication occurred during the drag and adjusting transaction handling.

Changes:

Introduces a duplicated_in_drag flag to track Alt-drag duplication state across drag lifecycle.
Splits duplication into a regular path vs. an Alt-drag path (DuplicateSelectedNodesForDrag) via a shared helper implementation.
Adjusts abort/commit behavior on right-click/Escape/PointerUp to try to undo or finalize Alt-drag duplication.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs	Adds `duplicated_in_drag`, introduces `DuplicateSelectedNodesForDrag`, and changes duplication + drag abort/commit behavior
editor/src/messages/portfolio/document/node_graph/node_graph_message.rs	Adds the `DuplicateSelectedNodesForDrag` message variant
editor/src/messages/portfolio/document/document_message_handler.rs	Changes Escape handling to attempt undoing Alt-drag duplication on abort

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

Copilot · 2026-02-17T23:52:25Z

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

+		if add_transaction {
+			responses.add(DocumentMessage::AddTransaction);
+		} else {
+			responses.add(DocumentMessage::StartTransaction);
+		}
+		responses.add(NodeGraphMessage::AddNodes { nodes, new_ids: new_ids.clone() });
+		responses.add(NodeGraphMessage::SelectedNodesSet {
+			nodes: new_ids.values().cloned().collect(),
+		});
+		self.duplicated_in_drag = !add_transaction;


DuplicateSelectedNodesForDrag calls DocumentMessage::StartTransaction inside duplicate_selected_nodes_impl, but a drag already starts a transaction in PointerDown (see the StartTransaction queued at the end of the node click handling). Starting a second transaction mid-drag overwrites transaction_status and pushes an extra undo snapshot, which makes abort/undo semantics incorrect (it can require multiple undos and may preserve earlier drag movement). For the drag path, avoid starting/committing a separate transaction—just duplicate nodes within the existing drag transaction, and let the PointerUp EndTransaction commit/cancel the whole drag+duplicate as one unit.

Suggested change

if add_transaction {

responses.add(DocumentMessage::AddTransaction);

} else {

responses.add(DocumentMessage::StartTransaction);

}

responses.add(NodeGraphMessage::AddNodes { nodes, new_ids: new_ids.clone() });

responses.add(NodeGraphMessage::SelectedNodesSet {

nodes: new_ids.values().cloned().collect(),

});

self.duplicated_in_drag = !add_transaction;

// Only start or add a transaction when not already in a drag transaction.

if self.drag_start.is_none() {

if add_transaction {

responses.add(DocumentMessage::AddTransaction);

} else {

responses.add(DocumentMessage::StartTransaction);

}

}

responses.add(NodeGraphMessage::AddNodes { nodes, new_ids: new_ids.clone() });

responses.add(NodeGraphMessage::SelectedNodesSet {

nodes: new_ids.values().cloned().collect(),

});

// Track that duplication occurred during a drag so the drag transaction can handle undo/abort.

self.duplicated_in_drag = self.drag_start.is_some();

Same here, you can judge whether or not this makes any sense.

Copilot · 2026-02-17T23:52:25Z

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

 					responses.add(NodeGraphMessage::SendGraph);

+					if self.duplicated_in_drag {
+						responses.add(DocumentMessage::CommitTransaction);


This CommitTransaction on PointerUp (guarded by duplicated_in_drag) finishes the transaction before the unconditional EndTransaction at the end of PointerUp. That can prevent later drag mutations from being captured (since transaction_modified won’t transition from Finished) and makes the drag transaction lifecycle inconsistent. If duplication is meant to be part of the drag transaction, remove this explicit commit and rely on the existing EndTransaction at the end of PointerUp.

Suggested change

responses.add(DocumentMessage::CommitTransaction);

Does this sound sensible or dumb? I'm not familiar enough with the code to tell so I'll leave that to you.

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

Copilot · 2026-02-17T23:52:26Z

editor/src/messages/portfolio/document/document_message_handler.rs

-					responses.add(DocumentMessage::AbortTransaction);
+					if self.node_graph_handler.duplicated_in_drag {
+						responses.add(DocumentMessage::AbortTransaction);
+						responses.add(DocumentMessage::Undo);


Escape handling aborts the transaction and then calls Undo when duplicated_in_drag is set. AbortTransaction already performs an undo when the transaction is modified (see DocumentMessage::AbortTransaction -> RepeatedAbortTransaction), so the extra Undo will roll back one additional history step and can undo unrelated user actions. Instead, ensure Alt-drag duplication occurs within the existing drag transaction so a single AbortTransaction reverts both move and duplication, and remove the extra Undo here.

Suggested change

responses.add(DocumentMessage::Undo);

Same here, please check if this makes sense or not.

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

Copilot · 2026-02-17T23:52:26Z

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

 				Cut,
 				DeleteSelectedNodes,
 				DuplicateSelectedNodes,
+				DuplicateSelectedNodesForDrag,


DuplicateSelectedNodesForDrag appears to be an internal helper message triggered by the Alt-drag logic, but it’s added to actions_additional_if_node_graph_is_open(). Since this action list is surfaced for input/action availability, exposing an internal-only message can allow it to be triggered in unsupported contexts (e.g., via keybinding) and complicate transaction semantics. Consider removing it from the public action list and keeping the duplication-for-drag behavior internal to the drag handler.

Suggested change

DuplicateSelectedNodesForDrag,

gemini-code-assist

Code Review

This pull request addresses an issue with cleaning up duplicated nodes when an Alt-drag operation is aborted. The changes introduce a duplicated_in_drag flag to track when a duplication has occurred during a drag, and uses this flag to ensure proper cleanup via transaction abortion and undos. The logic has been updated for both Escape key and right-click aborts. A good refactoring was done to extract the node duplication logic into a shared duplicate_selected_nodes_impl function.

I have one suggestion to improve code clarity by removing a redundant if/else block. Overall, the changes look good and correctly solve the issue.

gemini-code-assist · 2026-02-17T23:52:50Z

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

+						if self.duplicated_in_drag {
+							responses.add(DocumentMessage::AbortTransaction);
+							self.duplicated_in_drag = false;
+						} else {
+							responses.add(DocumentMessage::AbortTransaction);
+						}


This if/else block is redundant as responses.add(DocumentMessage::AbortTransaction) is called in both branches. This can be simplified by moving the common call out and unconditionally setting self.duplicated_in_drag to false, as it's a harmless no-op if it's already false.

responses.add(DocumentMessage::AbortTransaction); self.duplicated_in_drag = false;

Please make this change.

Keavon · 2026-02-18T00:03:43Z

editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs

 				DuplicateSelectedNodes,
+				DuplicateSelectedNodesForDrag,


Neither of these are listed in input_mappings.rs so adding them here is wrong, unless the intention actually was to have them in input_mappings.rs.

Final Fix

1f505c6

jsjgdh force-pushed the Alt-Fix branch from 39076ad to 1f505c6 Compare February 17, 2026 22:02

jsjgdh added 2 commits February 17, 2026 22:23

Fix-3

f0b82bd

Fix-4

8c7322f

jsjgdh marked this pull request as draft February 17, 2026 23:16

jsjgdh added 3 commits February 17, 2026 23:19

separate version

6cda432

Fix -5

041e33c

Revert

1e70008

Keavon requested a review from Copilot February 17, 2026 23:48

Copilot started reviewing on behalf of Keavon February 17, 2026 23:49 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

gemini-code-assist bot reviewed Feb 17, 2026

View reviewed changes

Keavon reviewed Feb 18, 2026

View reviewed changes

Uh oh!

Conversation

jsjgdh commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keavon commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Keavon Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Keavon Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Keavon Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Keavon Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Keavon Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jsjgdh commented Feb 17, 2026 •

edited

Loading