Fix Alt-drag duplicate cleanup on right-click abort

[jsjgdh]

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

[Keavon]

/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.

[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.

[cubic-dev-ai]

1 issue found across 3 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs">

<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs:1134">
P2: Alt-drag duplicates are now created inside the current drag transaction (no AddTransaction), but ShakeNode still aborts that transaction mid-drag to reset state. This rollback now deletes the duplicate itself, so Alt-drag followed by Shake loses the duplicated nodes and leaves shake logic operating on the pre-duplication selection. The new duplicated_in_drag flag is never used to guard this path.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[jsjgdh]

@cubic-dev

[cubic-dev-ai]

@cubic-dev

@jsjgdh I have started the AI code review. It will take a few minutes to complete.

[cubic-dev-ai]

No issues found across 3 files

Confidence score: 5/5

• Automated review surfaced no issues in the provided summaries.
• No files require special attention.

_{Re-trigger cubic}

[cubic-dev-ai]

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs">

<violation number="1">
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.</violation>
</file>

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

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>