Fix wrong-input disconnects, ghost wire caches, and partial import removal in the network interface (#4381)

* Remove commented-out code and fix stale names and doc comments

* Validate that the pinned node order only references existing nodes

* Abort remove_import before any mutation when the import's wires cannot be resolved

* Purge the deleted nodes' cached wire paths in delete_nodes

* Disconnect a deleted node's inputs by position so hidden inputs cannot mask a later exposed wire

* Gather the delete_nodes layer sweep only when deleting children needs it

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

@@ -17,9 +17,9 @@ use super::utility_types::network_interface::storage_metadata::{StorageMetadataV
 #[derivative(Clone, Debug, Default)]
 pub struct DocumentHistory {
 	/// Stack of document network snapshots for previous history states.
-	lagacy_undo_stack: VecDeque<NodeNetworkInterface>,
+	legacy_undo_stack: VecDeque<NodeNetworkInterface>,
 	/// Stack of document network snapshots for future history states.
-	lagacy_redo_stack: VecDeque<NodeNetworkInterface>,
+	legacy_redo_stack: VecDeque<NodeNetworkInterface>,
 	/// The `Gdd` working copy: owns the CRDT `Session` and mirrors edits to disk. `None` until the mount
 	/// future built by `load_document` resolves.
 	#[derivative(Debug = "ignore")]
@@ -31,38 +31,38 @@ impl DocumentHistory {
 
 	/// Push a snapshot onto the undo stack, evicting the oldest entry past the history cap.
 	pub fn push_undo(&mut self, snapshot: NodeNetworkInterface) {
-		Self::push_capped(&mut self.lagacy_undo_stack, snapshot);
+		Self::push_capped(&mut self.legacy_undo_stack, snapshot);
 	}
 
 	/// Push a snapshot onto the redo stack, evicting the oldest entry past the history cap.
 	pub fn push_redo(&mut self, snapshot: NodeNetworkInterface) {
-		Self::push_capped(&mut self.lagacy_redo_stack, snapshot);
+		Self::push_capped(&mut self.legacy_redo_stack, snapshot);
 	}
 
 	/// Pop the most recent undo snapshot, or `None` when the stack is empty.
 	pub fn pop_undo(&mut self) -> Option<NodeNetworkInterface> {
-		self.lagacy_undo_stack.pop_back()
+		self.legacy_undo_stack.pop_back()
 	}
 
 	/// Pop the most recent redo snapshot, or `None` when the stack is empty.
 	pub fn pop_redo(&mut self) -> Option<NodeNetworkInterface> {
-		self.lagacy_redo_stack.pop_back()
+		self.legacy_redo_stack.pop_back()
 	}
 
 	/// Drop the most recently pushed undo snapshot (used to cancel a transaction that ended up unmodified).
 	pub fn discard_last_undo(&mut self) {
-		self.lagacy_undo_stack.pop_back();
+		self.legacy_undo_stack.pop_back();
 	}
 
 	/// Clear the redo stack, called when a fresh edit invalidates the redo future.
 	pub fn clear_redo(&mut self) {
-		self.lagacy_redo_stack.clear();
+		self.legacy_redo_stack.clear();
 	}
 
 	/// Add the resources referenced by every snapshot in both history stacks into `resources`, so
 	/// history-only resources stay alive for legacy undo/redo.
 	pub fn collect_used_resources(&self, resources: &mut HashSet<ResourceId>) {
-		for interface in self.lagacy_undo_stack.iter().chain(&self.lagacy_redo_stack) {
+		for interface in self.legacy_undo_stack.iter().chain(&self.legacy_redo_stack) {
 			interface.collect_used_resources(resources);
 		}
 	}

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

@@ -103,7 +103,7 @@ pub struct DocumentNodeDefinition {
 	/// Used to create the [`DefinitionIdentifier::Network`] identifier.
 	pub identifier: &'static str,
 
-	/// All data required to construct a [`DocumentNode`] and [`DocumentNodeMetadata`]
+	/// The default [`NodeTemplate`] this definition instantiates.
 	pub node_template: NodeTemplate,
 
 	/// Definition specific data. In order for the editor to access this data, the reference will be used.

editor/src/messages/portfolio/document/utility_types/network_interface/layout.rs

@@ -77,44 +77,12 @@ impl NodeNetworkInterface {
 			log::error!("Could not set chain position for layer node {node_id}");
 		}
 		// let previous_upstream_node = self.upstream_output_connector(&InputConnector::node(*node_id, 0), network_path).and_then(|output| output.node_id());
-		// let Some(previous_upstream_node_position) = previous_upstream_node.and_then(|upstream| self.position_from_downstream_node(&upstream, network_path)) else {
-		// 	log::error!("Could not get previous_upstream_node_position");
-		// 	return;
-		// };
 		self.unload_upstream_node_click_targets(vec![*node_id], network_path);
 		// Reload click target of the layer which encapsulate the chain
 		if let Some(downstream_layer) = self.downstream_layer_for_chain_node(node_id, network_path) {
 			self.unload_node_click_targets(&downstream_layer, network_path);
 		}
 		self.unload_all_nodes_bounding_box(network_path);
-
-		// let Some(new_upstream_node_position) = previous_upstream_node.and_then(|upstream| self.position_from_downstream_node(&upstream, network_path)) else {
-		// 	log::error!("Could not get new_upstream_node_position");
-		// 	return;
-		// };
-		// if let Some(previous_upstream_node) =   {
-		// 	let x_delta = new_upstream_node_position.x - previous_upstream_node_position.x;
-		// 	// Upstream node got shifted to left, so shift all upstream absolute sole dependents
-		// 	if x_delta != 0 {
-		// 		let upstream_absolute_nodes = SelectedNodes(
-		// 			self.upstream_flow_back_from_nodes(vec![previous_upstream_node], network_path, FlowType::UpstreamFlow)
-		// 				.into_iter()
-		// 				.filter(|node_id| self.is_absolute(node_id, network_path))
-		// 				.collect::<Vec<_>>(),
-		// 		);
-		// 		let old_selected_nodes = std::mem::replace(self.selected_nodes_mut(network_path).unwrap(), upstream_absolute_nodes);
-		// 		if x_delta < 0 {
-		// 			for _ in 0..x_delta.abs() {
-		// 				self.shift_selected_nodes(Direction::Left, false, network_path);
-		// 			}
-		// 		} else {
-		// 			for _ in 0..x_delta.abs() {
-		// 				self.shift_selected_nodes(Direction::Right, false, network_path);
-		// 			}
-		// 		}
-		// 		let _ = std::mem::replace(self.selected_nodes_mut(network_path).unwrap(), old_selected_nodes);
-		// 	}
-		// }
 	}
 
 	pub(crate) fn valid_upstream_chain_nodes(&self, input_connector: &InputConnector, network_path: &[NodeId]) -> Vec<NodeId> {

editor/src/messages/portfolio/document/utility_types/network_interface/mutations.rs

@@ -226,34 +226,26 @@ impl NodeNetworkInterface {
 		self.unload_modify_import_export(network_path);
 	}
 
-	/// Disconnects every wire fed by the given import within the network.
+	/// Disconnects every wire fed by the given import within the network. Returns false without mutating if the import's wires cannot be resolved.
-	pub(crate) fn disconnect_import_wires(&mut self, import_index: usize, network_path: &[NodeId]) {
+	pub(crate) fn disconnect_import_wires(&mut self, import_index: usize, network_path: &[NodeId]) -> bool {
-		let Some(wires_for_import) = self.with_outward_wires(network_path, |outward_wires| outward_wires.get(&OutputConnector::Import(import_index)).cloned()) else {
+		self.disconnect_output_wires(&OutputConnector::Import(import_index), network_path)
-			log::error!("Could not get outward wires in disconnect_import_wires");
-			return;
-		};
-		let Some(wires_for_import) = wires_for_import else {
-			log::error!("Could not get outward wires for import in disconnect_import_wires");
-			return;
-		};
-		for downstream_connection in wires_for_import {
-			self.disconnect_input(&downstream_connection, network_path);
-		}
 	}
 
-	/// Disconnects every wire fed by the given output within the network.
+	/// Disconnects every wire fed by the given output within the network. Returns false without mutating if the output's wires cannot be resolved.
-	pub(crate) fn disconnect_output_wires(&mut self, output_connector: &OutputConnector, network_path: &[NodeId]) {
+	pub(crate) fn disconnect_output_wires(&mut self, output_connector: &OutputConnector, network_path: &[NodeId]) -> bool {
 		let Some(downstream_connections) = self.with_outward_wires(network_path, |outward_wires| outward_wires.get(output_connector).cloned()) else {
 			log::error!("Could not get outward wires in disconnect_output_wires");
-			return;
+			return false;
 		};
 		let Some(downstream_connections) = downstream_connections else {
-			log::error!("Could not get downstream connections in disconnect_output_wires");
+			log::error!("Could not get downstream connections for {output_connector:?} in disconnect_output_wires");
-			return;
+			return false;
 		};
 		for downstream_connection in downstream_connections {
 			self.disconnect_input(&downstream_connection, network_path);
 		}
+
+		true
 	}
 
 	/// Refreshes the metadata invalidated when the encapsulating node's signature changes, demoting it from a layer if it is no longer eligible.
@@ -346,9 +338,11 @@ impl NodeNetworkInterface {
 			}
 		}
 
-		// Disconnect all upstream connections
+		// Disconnect all upstream connections, aborting before any mutation if the import's wires cannot be resolved
-		self.disconnect_import_wires(import_index, network_path);
+		if !self.disconnect_import_wires(import_index, network_path) {
-		// Shift inputs connected to to imports at a higher index down one
+			return;
+		}
+		// Shift inputs connected to imports at a higher index down one
 		for (output_connector, input_wire) in new_import_mapping {
 			self.create_wire(&output_connector, &input_wire, network_path);
 		}
@@ -1105,13 +1099,16 @@ impl NodeNetworkInterface {
 		}
 
 		// Layer membership is fixed during the expansion phase, so gather it once for the sole-dependent closure
-		let layer_nodes = self
+		let layer_nodes = if delete_children {
-			.nested_network(network_path)
+			self.nested_network(network_path)
-			.map(|network| network.nodes.keys().copied().collect::<Vec<_>>())
+				.map(|network| network.nodes.keys().copied().collect::<Vec<_>>())
-			.unwrap_or_default()
+				.unwrap_or_default()
-			.into_iter()
+				.into_iter()
-			.filter(|candidate| self.is_layer(candidate, network_path))
+				.filter(|candidate| self.is_layer(candidate, network_path))
-			.collect::<HashSet<_>>();
+				.collect::<HashSet<_>>()
+		} else {
+			HashSet::new()
+		};
 
 		let mut delete_nodes = HashSet::new();
 		for node_id in &nodes_to_delete {
@@ -1163,7 +1160,8 @@ impl NodeNetworkInterface {
 				continue;
 			}
 
-			for input_index in 0..self.number_of_displayed_inputs(delete_node_id, network_path) {
+			// Disconnect every input by position, since hidden inputs make the displayed count undershoot the index of a later exposed wire
+			for input_index in 0..self.number_of_inputs(delete_node_id, network_path) {
 				self.disconnect_input(&InputConnector::node(*delete_node_id, input_index), network_path);
 			}
 
@@ -1193,6 +1191,15 @@ impl NodeNetworkInterface {
 			network_metadata.persistent_metadata.pinned_node_order.retain(|node_id| surviving_nodes.contains(node_id));
 		}
 
+		// Purge the deleted nodes' cached wire paths, since the per-node unload can no longer reach them once the nodes are gone
+		if let Some(network_metadata) = self.network_metadata(network_path) {
+			network_metadata
+				.transient_metadata
+				.wires
+				.borrow_mut()
+				.retain(|connector, _| connector.node_id().is_none_or(|node_id| !delete_nodes.contains(&node_id)));
+		}
+
 		self.unload_all_nodes_bounding_box(network_path);
 		// Instead of unloaded all node click targets, just unload the nodes upstream from the deleted nodes. unload_upstream_node_click_targets will not work since the nodes have been deleted.
 		self.unload_all_nodes_click_targets(network_path);
@@ -1301,22 +1308,6 @@ impl NodeNetworkInterface {
 		network_metadata.persistent_metadata.previewing = Previewing::No;
 	}
 
-	// /// Sets the root node only if a node is being previewed
-	// pub fn update_root_node(&mut self, node_id: NodeId, output_index: usize) {
-	// 	if let Previewing::Yes { root_node_to_restore } = self.previewing {
-	// 		// Only continue previewing if the new root node is not the same as the primary export. If it is the same, end the preview
-	// 		if let Some(root_node_to_restore) = root_node_to_restore {
-	// 			if root_node_to_restore.id != node_id {
-	// 				self.start_previewing(node_id, output_index);
-	// 			} else {
-	// 				self.stop_preview();
-	// 			}
-	// 		} else {
-	// 			self.stop_preview();
-	// 		}
-	// 	}
-	// }
-
 	pub fn set_display_name(&mut self, node_id: &NodeId, display_name: String, network_path: &[NodeId]) {
 		let Some(node_metadata) = self.node_metadata_mut(node_id, network_path) else {
 			log::error!("Could not get node {node_id} in set_visibility");

editor/src/messages/portfolio/document/utility_types/network_interface/queries.rs

@@ -1210,21 +1210,6 @@ impl NodeNetworkInterface {
 		Some(parent_metadata)
 	}
 
-	// /// Mutably get the node which encapsulates the currently viewed network. Will always be None in the document network.
-	// fn encapsulating_node_mut(&mut self, network_path: &[NodeId]) -> Option<&mut DocumentNode> {
-	// 	let mut encapsulating_path = network_path.to_vec();
-	// 	let encapsulating_node_id = encapsulating_path.pop()?;
-	// 	let Some(parent_network) = self.network_mut(&encapsulating_path) else {
-	// 		log::error!("Could not get parent network in encapsulating_node_mut");
-	// 		return None;
-	// 	};
-	// 	let Some(encapsulating_node) = parent_network.nodes.mut(&encapsulating_node_id) else {
-	// 		log::error!("Could not get encapsulating node in encapsulating_node_mut");
-	// 		return None;
-	// 	};
-	// 	Some(encapsulating_node)
-	// }
-
 	/// Get the node metadata for the node which encapsulates the currently viewed network. Will always be None in the document network.
 	pub(crate) fn encapsulating_node_metadata_mut(&mut self, network_path: &[NodeId]) -> Option<&mut DocumentNodeMetadata> {
 		let mut encapsulating_path = network_path.to_vec();

editor/src/messages/portfolio/document/utility_types/network_interface/types.rs

@@ -426,8 +426,6 @@ pub struct NodeNetworkTransientMetadata {
 	pub(crate) stack_dependents: TransientCache<HashMap<NodeId, LayerOwner>>,
 	/// Cache for the bounding box around all nodes in node graph space.
 	pub(crate) all_nodes_bounding_box: TransientCache<[DVec2; 2]>,
-	// /// Cache bounding box for all "groups of nodes", which will be used to prevent overlapping nodes
-	// node_group_bounding_box: Vec<(Subpath<ManipulatorGroupId>, Vec<Nodes>)>,
 	/// Cache for all outward wire connections
 	pub(crate) outward_wires: TransientCache<HashMap<OutputConnector, Vec<InputConnector>>>,
 	/// All export connector click targets
@@ -704,8 +702,6 @@ impl NodeTypePersistentMetadata {
 /// All fields in LayerMetadata should automatically be updated by using the network interface API
 #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
 pub struct LayerPersistentMetadata {
-	// TODO: Store click target for the preview button, which will appear when the node is a selected/(hovered?) layer node
-	// preview_click_target: Option<ClickTarget>,
 	/// Stores the position of a layer node, which can either be Absolute or Stack
 	pub position: LayerPosition,
 }
@@ -790,8 +786,6 @@ pub struct LayerClickTargets {
 	/// to skip the drill-into-subgraph behavior when the click lands on the name itself.
 	/// `None` for layers whose display name is empty.
 	pub name_click_target: Option<ClickTarget>,
-	// TODO: Store click target for the preview button, which will appear when the node is a selected/(hovered?) layer node
-	// preview_click_target: ClickTarget,
 }
 
 pub enum LayerClickTargetTypes {

editor/src/messages/portfolio/document/utility_types/network_interface/validation.rs

@@ -28,6 +28,13 @@ fn validate_network(network: &NodeNetwork, network_metadata: &NodeNetworkMetadat
 		}
 	}
 
+	// The pinned display order may only reference existing nodes
+	for node_id in &network_metadata.persistent_metadata.pinned_node_order {
+		if !network.nodes.contains_key(node_id) {
+			violations.push(format!("Pinned node order in network {path:?} references nonexistent node {node_id}"));
+		}
+	}
+
 	// Every wire must reference an existing endpoint within this network
 	let validate_input = |input: &NodeInput, location: &str, violations: &mut Vec<String>| match input {
 		NodeInput::Node { node_id, output_index, .. } => match network.nodes.get(node_id) {