Make the node graph's "Merge Selected Nodes" target the right-clicked node, and fix it crashing (#4252)

Author: Keavon

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

@@ -3908,6 +3908,37 @@ mod document_message_handler_tests {
 		editor.handle_message(DocumentMessage::CreateEmptyFolder).await;
 		assert!(true, "Application didn't crash after folder move operation");
 	}
+
+	// Merging nodes whose output isn't wired downstream produces an encapsulating subnetwork with no exports.
+	// Inspecting it via the Data panel (which splices in a monitor node) must not leave a dangling reference that crashes compilation.
+	#[tokio::test]
+	async fn merge_selected_nodes_while_inspecting_does_not_crash() {
+		let mut editor = EditorTestUtils::create();
+		editor.new_document().await;
+		editor.draw_rect(0., 0., 100., 100.).await;
+
+		let node_a = editor.create_node_by_name(DefinitionIdentifier::ProtoNode(graphene_std::transform_nodes::transform::IDENTIFIER)).await;
+		let node_b = editor.create_node_by_name(DefinitionIdentifier::ProtoNode(graphene_std::transform_nodes::transform::IDENTIFIER)).await;
+		editor.handle_message(NodeGraphMessage::SelectedNodesSet { nodes: vec![node_a, node_b] }).await;
+		editor.handle_message(NodeGraphMessage::MergeSelectedNodes).await;
+
+		let merged = editor.active_document().network_interface.selected_nodes_in_nested_network(&[]).unwrap().0.clone();
+		assert_eq!(merged.len(), 1, "merge should leave one encapsulating node selected");
+
+		// Simulate the Data panel inspecting the merged node, which compiles the graph with a monitor node spliced in
+		let portfolio = &mut editor.editor.dispatcher.message_handlers.portfolio_message_handler;
+		let document_id = portfolio.active_document_id.unwrap();
+		let document = portfolio.documents.get_mut(&document_id).unwrap();
+		portfolio
+			.executor
+			.submit_node_graph_evaluation(document, document_id, glam::UVec2::ONE, 1., Default::default(), merged, true, DVec2::ZERO)
+			.unwrap();
+		editor.runtime.run().await;
+
+		let mut messages = VecDeque::new();
+		editor.editor.poll_node_graph_evaluation(&mut messages).unwrap();
+	}
+
 	#[tokio::test]
 	async fn test_moving_folder_with_children() {
 		let mut editor = EditorTestUtils::create();

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

@@ -838,11 +838,17 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 						let currently_is_node = !network_interface.is_layer(&node_id, breadcrumb_network_path);
 						let can_be_layer = network_interface.is_eligible_to_be_layer(&node_id, breadcrumb_network_path);
 
+						let selected_nodes = network_interface.selected_nodes_in_nested_network(selection_network_path);
+						let is_clicked_selected = selected_nodes.as_ref().is_some_and(|selected| selected.selected_nodes().any(|id| *id == node_id));
+
+						// Right-clicking an unselected node selects just it, so selection-based actions like Merge Selected Nodes target it
+						if !is_clicked_selected {
+							responses.add(NodeGraphMessage::SelectedNodesSet { nodes: vec![node_id] });
+						}
+
 						// Determine which layers the Lock/Unlock action would affect:
 						// - If the right-clicked node is in the selection, it affects all selected layers
 						// - If the right-clicked node is not in the selection, it affects just the right-clicked node
-						let selected_nodes = network_interface.selected_nodes_in_nested_network(selection_network_path);
-						let is_clicked_selected = selected_nodes.as_ref().is_some_and(|selected| selected.selected_nodes().any(|id| *id == node_id));
 						let affected_layer_ids = if is_clicked_selected {
 							selected_nodes.map(|selected| selected.selected_nodes().copied().filter(|id| network_interface.is_layer(id, selection_network_path)).collect())
 						} else {

editor/src/node_graph_executor/runtime.rs

@@ -609,11 +609,23 @@ impl PartialEq for InspectResult {
 
 impl InspectState {
 	/// Insert the monitor node alongside the inspect node identified by `inspect_path` (full path from root, last element is the target).
-	/// Returns `None` if the path is empty or doesn't resolve to a node inside a reachable subnetwork.
+	/// Returns `None` if the path is empty, doesn't resolve to a node inside a reachable subnetwork, or the target has no
+	/// flatten-safe primary output to monitor (e.g. an empty merged subnetwork), which would otherwise leave the monitor's
+	/// input dangling once the subnetwork is flattened away.
 	pub fn monitor_inspect_node(network: &mut NodeNetwork, inspect_path: &[NodeId]) -> Option<Self> {
 		let (inspect_node, parent_path) = inspect_path.split_last()?;
 		let inspect_node = *inspect_node;
 		let target_network = navigate_to_network_mut(network, parent_path)?;
+
+		// A subnetwork's primary output only survives flattening if its first export is a node
+		let monitorable = match &target_network.nodes.get(&inspect_node)?.implementation {
+			DocumentNodeImplementation::Network(inner) => matches!(inner.exports.first(), Some(NodeInput::Node { .. })),
+			_ => true,
+		};
+		if !monitorable {
+			return None;
+		}
+
 		let monitor_id = NodeId::new();
 
 		// It is necessary to replace the inputs before inserting the monitor node to avoid changing the input of the new monitor node