Skip to content

Fix Alt-drag duplicate cleanup on right-click abort #3782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jsjgdh wants to merge 10 commits into
base: master
Choose a base branch
from
+17 −7
Open

Fix Alt-drag duplicate cleanup on right-click abort #3782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a91f572
Final Fix
jsjgdh Feb 17, 2026
c21ab6d
Fix-3
jsjgdh Feb 17, 2026
189a389
Fix-4
jsjgdh Feb 17, 2026
3afd417
separate version
jsjgdh Feb 17, 2026
cb1ebb1
Fix -5
jsjgdh Feb 17, 2026
6e5e3d2
Revert
jsjgdh Feb 17, 2026
87ff27f
refactor
jsjgdh Feb 18, 2026
9eae26b
Fix-5
jsjgdh Feb 18, 2026
f51a066
Logic improvment
jsjgdh Mar 13, 2026
8da1463
Claude Review
jsjgdh Jul 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions editor/src/messages/portfolio/document/document_message_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,19 +540,19 @@
responses.add(NodeGraphMessage::UpdateNodeGraphWidth);
}
DocumentMessage::Escape => {
// Abort dragging nodes
if self.node_graph_handler.drag_start.is_some() {
responses.add(DocumentMessage::AbortTransaction);
self.node_graph_handler.drag_start = None;
self.node_graph_handler.select_if_not_dragged = None;
}
// Abort dragging nodes
if self.node_graph_handler.drag_start.is_some() {
responses.add(DocumentMessage::AbortTransaction);
self.node_graph_handler.drag_start = None;
self.node_graph_handler.select_if_not_dragged = None;
}
// Abort box selection
else if self.node_graph_handler.box_selection_start.is_some() {
self.node_graph_handler.box_selection_start = None;
responses.add(NodeGraphMessage::SelectedNodesSet {
nodes: self.node_graph_handler.selection_before_pointer_down.clone(),
});
responses.add(FrontendMessage::UpdateBox { box_selection: None });

Check warning on line 555 in editor/src/messages/portfolio/document/document_message_handler.rs

View workflow job for this annotation

GitHub Actions / rust-fmt 

Diff in /home/runner/work/Graphite/Graphite/editor/src/messages/portfolio/document/document_message_handler.rs
}
// Abort wire in progress of being connected
else if self.node_graph_handler.wire_in_progress_from_connector.is_some() {
Expand Down
12 changes: 11 additions & 1 deletion editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,7 +1172,17 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
if self.begin_dragging {

@cubic-dev-ai cubic-dev-ai Bot Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: The core node-copy logic (computing all_selected_nodes, copy_ids, copy_nodes, new_ids, then sending AddNodes + SelectedNodesSet) is now duplicated in two places: the DuplicateSelectedNodes message handler and the Alt-drag path inside PointerDown. Previously this was centralized in duplicate_selected_nodes_impl. While the transaction setup differs (AddTransaction in one path, none in the other because it runs inside an existing transaction), the four computation lines and the two response messages are identical. Extracting just the shared computation (mapping, copy, ID generation) into a private helper would prevent these two paths from drifting apart.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs, line 398:

<comment>The core node-copy logic (computing `all_selected_nodes`, `copy_ids`, `copy_nodes`, `new_ids`, then sending `AddNodes` + `SelectedNodesSet`) is now duplicated in two places: the `DuplicateSelectedNodes` message handler and the Alt-drag path inside `PointerDown`. Previously this was centralized in `duplicate_selected_nodes_impl`. While the transaction setup differs (`AddTransaction` in one path, none in the other because it runs inside an existing transaction), the four computation lines and the two response messages are identical. Extracting just the shared computation (mapping, copy, ID generation) into a private helper would prevent these two paths from drifting apart.</comment>

<file context>
@@ -397,13 +395,19 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
-				self.duplicate_selected_nodes_impl(network_interface, selection_network_path, responses, false);
-				responses.add(DocumentMessage::EndTransaction);
-				responses.add(DocumentMessage::StartTransaction);
+				let all_selected_nodes = network_interface.upstream_chain_nodes(selection_network_path);
+
+				let copy_ids = all_selected_nodes.iter().enumerate().map(|(new, id)| (*id, NodeId(new as u64))).collect::<HashMap<NodeId, NodeId>>();
</file context>

self.begin_dragging = false;
if ipp.keyboard.get(Key::Alt as usize) {
responses.add(NodeGraphMessage::DuplicateSelectedNodes);
// Duplicate nodes inline within the existing PointerDown transaction.
// This means a completed Alt-drag is one undo step, and aborting (right-click / Escape)
// rolls back both the duplication and the move with a single plain AbortTransaction.
let all_selected_nodes = network_interface.upstream_chain_nodes(selection_network_path);
let copy_ids = all_selected_nodes.iter().enumerate().map(|(new, id)| (*id, NodeId(new as u64))).collect::<HashMap<NodeId, NodeId>>();
let nodes = network_interface.copy_nodes(&copy_ids, selection_network_path).collect::<Vec<_>>();
let new_ids = nodes.iter().map(|(id, _)| (*id, NodeId::new())).collect::<HashMap<_, _>>();
responses.add(NodeGraphMessage::AddNodes { nodes, new_ids: new_ids.clone() });
responses.add(NodeGraphMessage::SelectedNodesSet {
nodes: new_ids.values().cloned().collect(),
});
// Duplicating sets a 2x2 offset, so shift the nodes back to the original position
responses.add(NodeGraphMessage::ShiftSelectedNodesByAmount {
graph_delta: IVec2::new(-2, -2),
Expand Down
Loading