Code review

Author: Keavon

editor/src/messages/frontend/frontend_message.rs

@@ -2,7 +2,7 @@ use super::utility_types::{DocumentDetails, MouseCursorIcon, OpenDocument};
 use crate::messages::app_window::app_window_message_handler::AppWindowPlatform;
 use crate::messages::layout::utility_types::widget_prelude::*;
 use crate::messages::portfolio::document::node_graph::utility_types::{
-	BoxSelection, ContextMenuInformation, FrontendClickTargets, FrontendGraphInput, FrontendGraphOutput, FrontendNode, FrontendNodeType, NodeGraphError, Transform,
+	BoxSelection, ContextMenuInformation, FrontendClickTargets, FrontendGraphInput, FrontendGraphOutput, FrontendNode, FrontendNodeType, NodeGraphErrorDiagnostic, Transform,
 };
 use crate::messages::portfolio::document::utility_types::nodes::{JsRawBuffer, LayerPanelEntry, RawBuffer};
 use crate::messages::portfolio::document::utility_types::wires::{WirePath, WirePathUpdate};
@@ -289,8 +289,8 @@ pub enum FrontendMessage {
 	UpdateNodeGraphNodes {
 		nodes: Vec<FrontendNode>,
 	},
-	UpdateNodeGraphError {
-		error: Option<NodeGraphError>,
+	UpdateNodeGraphErrorDiagnostic {
+		error: Option<NodeGraphErrorDiagnostic>,
 	},
 	UpdateVisibleNodes {
 		nodes: Vec<NodeId>,

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

@@ -6,7 +6,7 @@ use crate::messages::layout::utility_types::widget_prelude::*;
 use crate::messages::portfolio::document::document_message_handler::navigation_controls;
 use crate::messages::portfolio::document::graph_operation::utility_types::ModifyInputsContext;
 use crate::messages::portfolio::document::node_graph::document_node_definitions::NodePropertiesContext;
-use crate::messages::portfolio::document::node_graph::utility_types::{ContextMenuData, Direction, FrontendGraphDataType, NodeGraphError};
+use crate::messages::portfolio::document::node_graph::utility_types::{ContextMenuData, Direction, FrontendGraphDataType, NodeGraphErrorDiagnostic};
 use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier;
 use crate::messages::portfolio::document::utility_types::misc::GroupFolderType;
 use crate::messages::portfolio::document::utility_types::network_interface::{
@@ -798,9 +798,8 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 						DVec2::new(appear_right_of_mouse, appear_above_mouse) / network_metadata.persistent_metadata.navigation_metadata.node_graph_to_viewport.matrix2.x_axis.x
 					};
 
-					let context_menu_coordinates = node_graph_point + node_graph_shift;
 					self.context_menu = Some(ContextMenuInformation {
-						context_menu_coordinates: context_menu_coordinates.into(),
+						context_menu_coordinates: (node_graph_point + node_graph_shift).into(),
 						context_menu_data,
 					});
 
@@ -1224,10 +1223,9 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 						let node_graph_shift = DVec2::new(appear_right_of_mouse, appear_above_mouse) / network_metadata.persistent_metadata.navigation_metadata.node_graph_to_viewport.matrix2.x_axis.x;
 
 						let compatible_type = network_interface.output_type(&output_connector, selection_network_path).add_node_string();
-						let context_menu_coordinates = point + node_graph_shift;
 
 						self.context_menu = Some(ContextMenuInformation {
-							context_menu_coordinates: context_menu_coordinates.into(),
+							context_menu_coordinates: (point + node_graph_shift).into(),
 							context_menu_data: ContextMenuData::CreateNode { compatible_type },
 						});
 
@@ -1652,7 +1650,7 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
 					responses.add(NodeGraphMessage::UpdateVisibleNodes);
 
 					let error = self.node_graph_error(network_interface, breadcrumb_network_path);
-					responses.add(FrontendMessage::UpdateNodeGraphError { error });
+					responses.add(FrontendMessage::UpdateNodeGraphErrorDiagnostic { error });
 					let (layer_widths, chain_widths, has_left_input_wire) = network_interface.collect_layer_widths(breadcrumb_network_path);
 
 					responses.add(NodeGraphMessage::UpdateImportsExports);
@@ -2596,26 +2594,27 @@ impl NodeGraphMessageHandler {
 		Some(subgraph_names)
 	}
 
-	fn node_graph_error(&self, network_interface: &mut NodeNetworkInterface, breadcrumb_network_path: &[NodeId]) -> Option<NodeGraphError> {
+	fn node_graph_error(&self, network_interface: &mut NodeNetworkInterface, breadcrumb_network_path: &[NodeId]) -> Option<NodeGraphErrorDiagnostic> {
 		let graph_error = network_interface
 			.resolved_types
 			.node_graph_errors
 			.iter()
-			.filter(|error| error.node_path.starts_with(breadcrumb_network_path) && error.node_path.len() > breadcrumb_network_path.len())
-			.next()?;
+			.find(|error| error.node_path.starts_with(breadcrumb_network_path) && error.node_path.len() > breadcrumb_network_path.len())?;
 		let error = if graph_error.node_path.len() == breadcrumb_network_path.len() + 1 {
 			format!("{:?}", graph_error.error)
 		} else {
 			"Node graph type error within this node".to_string()
 		};
 		let error_node = graph_error.node_path[breadcrumb_network_path.len()];
+
 		let mut position = network_interface.position(&error_node, breadcrumb_network_path)?;
 		// Convert to graph space
 		position *= 24;
 		if network_interface.is_layer(&error_node, breadcrumb_network_path) {
 			position += IVec2::new(12, -12)
 		}
-		Some(NodeGraphError { position: position.into(), error })
+
+		Some(NodeGraphErrorDiagnostic { position: position.into(), error })
 	}
 
 	fn update_layer_panel(network_interface: &NodeNetworkInterface, selection_network_path: &[NodeId], collapsed: &CollapsedLayers, layers_panel_open: bool, responses: &mut VecDeque<Message>) {

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

@@ -90,14 +90,14 @@ pub struct FrontendNode {
 	pub primary_output: Option<FrontendGraphOutput>,
 	#[serde(rename = "exposedOutputs")]
 	pub exposed_outputs: Vec<FrontendGraphOutput>,
-	#[serde(rename = "primaryOutputConnectedToLayer")]
-	pub primary_output_connected_to_layer: bool,
 	#[serde(rename = "primaryInputConnectedToLayer")]
 	pub primary_input_connected_to_layer: bool,
+	#[serde(rename = "primaryOutputConnectedToLayer")]
+	pub primary_output_connected_to_layer: bool,
 	pub position: IVec2,
+	pub previewed: bool,
 	pub visible: bool,
 	pub locked: bool,
-	pub previewed: bool,
 }
 
 #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize, specta::Type)]
@@ -156,12 +156,12 @@ pub struct BoxSelection {
 #[serde(tag = "type", content = "data")]
 pub enum ContextMenuData {
 	ModifyNode {
+		#[serde(rename = "nodeId")]
+		node_id: NodeId,
 		#[serde(rename = "canBeLayer")]
 		can_be_layer: bool,
 		#[serde(rename = "currentlyIsNode")]
 		currently_is_node: bool,
-		#[serde(rename = "nodeId")]
-		node_id: NodeId,
 	},
 	CreateNode {
 		#[serde(rename = "compatibleType")]
@@ -179,7 +179,7 @@ pub struct ContextMenuInformation {
 }
 
 #[derive(Clone, Debug, PartialEq, Default, serde::Serialize, serde::Deserialize, specta::Type)]
-pub struct NodeGraphError {
+pub struct NodeGraphErrorDiagnostic {
 	pub position: FrontendXY,
 	pub error: String,
 }
@@ -208,7 +208,7 @@ pub enum Direction {
 	Right,
 }
 
-/// Stores node graph coordinates which are then transformed in svelte based on the node graph transform
+/// Stores node graph coordinates which are then transformed in Svelte based on the node graph transform
 #[derive(Clone, Debug, PartialEq, Default, serde::Serialize, serde::Deserialize, specta::Type)]
 pub struct FrontendXY {
 	pub x: i32,
@@ -223,6 +223,6 @@ impl From<DVec2> for FrontendXY {
 
 impl From<IVec2> for FrontendXY {
 	fn from(v: IVec2) -> Self {
-		FrontendXY { x: v.x as i32, y: v.y as i32 }
+		FrontendXY { x: v.x, y: v.y }
 	}
 }

frontend/src/components/views/Graph.svelte

@@ -212,9 +212,9 @@
 				top: `${$nodeGraph.contextMenuInformation.contextMenuCoordinates.y * $nodeGraph.transform.scale + $nodeGraph.transform.y}px`,
 			}}
 		>
-			{#if $nodeGraph.contextMenuInformation.contextMenuData.type == "CreateNode"}
+			{#if $nodeGraph.contextMenuInformation.contextMenuData.type === "CreateNode"}
 				<NodeCatalog initialSearchTerm={$nodeGraph.contextMenuInformation.contextMenuData.data.compatibleType || ""} on:selectNodeType={(e) => createNode(e.detail)} />
-			{:else if $nodeGraph.contextMenuInformation.contextMenuData.type == "ModifyNode"}
+			{:else if $nodeGraph.contextMenuInformation.contextMenuData.type === "ModifyNode"}
 				<LayoutRow class="toggle-layer-or-node">
 					<TextLabel>Display as</TextLabel>
 					<RadioInput
@@ -223,12 +223,16 @@
 							{
 								value: "node",
 								label: "Node",
-								action: () => editor.handle.setToNodeOrLayer($nodeGraph.contextMenuInformation.contextMenuData.data.nodeId, false),
+								action: () =>
+									$nodeGraph.contextMenuInformation?.contextMenuData.type === "ModifyNode" &&
+									editor.handle.setToNodeOrLayer($nodeGraph.contextMenuInformation.contextMenuData.data.nodeId, false),
 							},
 							{
 								value: "layer",
 								label: "Layer",
-								action: () => editor.handle.setToNodeOrLayer($nodeGraph.contextMenuInformation.contextMenuData.data.nodeId, true),
+								action: () =>
+									$nodeGraph.contextMenuInformation?.contextMenuData.type === "ModifyNode" &&
+									editor.handle.setToNodeOrLayer($nodeGraph.contextMenuInformation.contextMenuData.data.nodeId, true),
 							},
 						]}
 						disabled={!$nodeGraph.contextMenuInformation.contextMenuData.data.canBeLayer}
@@ -244,20 +248,12 @@
 
 	{#if $nodeGraph.error}
 		<div class="node-error-container" style:transform-origin="0 0" style:transform={`translate(${$nodeGraph.transform.x}px, ${$nodeGraph.transform.y}px) scale(${$nodeGraph.transform.scale})`}>
-			<span
-				class="node-error faded"
-				style={`left: ${$nodeGraph.error.position.x}px;
-           			top: ${$nodeGraph.error.position.y}px;`}
-				transition:fade={FADE_TRANSITION}
-				data-node-error>{$nodeGraph.error.error}</span
-			>
-			<span
-				class="node-error hover"
-				style={`left: ${$nodeGraph.error.position.x}px;
-           			top: ${$nodeGraph.error.position.y}px;`}
-				transition:fade={FADE_TRANSITION}
-				data-node-error>{$nodeGraph.error.error}</span
-			>
+			<span class="node-error faded" style:left={`${$nodeGraph.error.position.x}px`} style:top={`${$nodeGraph.error.position.y}px`} transition:fade={FADE_TRANSITION}>
+				{$nodeGraph.error.error}
+			</span>
+			<span class="node-error hover" style:left={`${$nodeGraph.error.position.x}px`} style:top={`${$nodeGraph.error.position.y}px`} transition:fade={FADE_TRANSITION}>
+				{$nodeGraph.error.error}
+			</span>
 		</div>
 	{/if}
 
@@ -337,7 +333,7 @@
 						style:--offset-left={($nodeGraph.updateImportsExports.importPosition.x - 8) / 24}
 						style:--offset-top={($nodeGraph.updateImportsExports.importPosition.y - 8) / 24 + index}
 					>
-						{#if editingNameImportIndex == index}
+						{#if editingNameImportIndex === index}
 							<input
 								class="import-text-input"
 								type="text"
@@ -449,7 +445,7 @@
 				{/if}
 			{/each}
 
-			{#if $nodeGraph.updateImportsExports.addImportExport == true}
+			{#if $nodeGraph.updateImportsExports.addImportExport}
 				<div
 					class="plus"
 					style:--offset-left={($nodeGraph.updateImportsExports.importPosition.x - 12) / 24}
@@ -657,10 +653,6 @@
 				title={`${node.displayName}\n\n${description || ""}`.trim() + (editor.handle.inDevelopmentMode() ? `\n\nNode ID: ${node.id}` : "")}
 				data-node={node.id}
 			>
-				{#if node.errors}
-					<span class="node-error faded" transition:fade={FADE_TRANSITION} title="" data-node-error>{node.errors}</span>
-					<span class="node-error hover" transition:fade={FADE_TRANSITION} title="" data-node-error>{node.errors}</span>
-				{/if}
 				<!-- Primary row -->
 				<div class="primary" class:in-selected-network={$nodeGraph.inSelectedNetwork} class:no-secondary-section={exposedInputsOutputs.length === 0}>
 					<IconLabel icon={nodeIcon(node.reference)} />

frontend/src/messages.ts

@@ -33,10 +33,6 @@ export class UpdateClickTargets extends JsMessage {
 	readonly clickTargets!: FrontendClickTargets | undefined;
 }
 
-export class UpdateContextMenuInformation extends JsMessage {
-	readonly contextMenuInformation!: ContextMenuInformation | undefined;
-}
-
 export class UpdateImportsExports extends JsMessage {
 	readonly imports!: (FrontendGraphOutput | undefined)[];
 
@@ -81,7 +77,7 @@ export class UpdateNodeGraphNodes extends JsMessage {
 	readonly nodes!: FrontendNode[];
 }
 
-export class UpdateNodeGraphError extends JsMessage {
+export class UpdateNodeGraphErrorDiagnostic extends JsMessage {
 	readonly error!: NodeGraphError | undefined;
 }
 
@@ -172,6 +168,10 @@ export type ContextMenuInformation = {
 	contextMenuData: { type: "CreateNode"; data: { compatibleType: string | undefined } } | { type: "ModifyNode"; data: { canBeLayer: boolean; currentlyIsNode: boolean; nodeId: bigint } };
 };
 
+export class UpdateContextMenuInformation extends JsMessage {
+	readonly contextMenuInformation!: ContextMenuInformation | undefined;
+}
+
 export type FrontendGraphDataType = "General" | "Number" | "Artboard" | "Graphic" | "Raster" | "Vector" | "Color" | "Invalid";
 
 export class FrontendGraphInput {
@@ -201,12 +201,12 @@ export class FrontendGraphOutput {
 }
 
 export class FrontendNode {
+	readonly id!: bigint;
+
 	readonly isLayer!: boolean;
 
 	readonly canBeLayer!: boolean;
 
-	readonly id!: bigint;
-
 	readonly reference!: string | undefined;
 
 	readonly displayName!: string;
@@ -232,9 +232,7 @@ export class FrontendNode {
 
 	readonly visible!: boolean;
 
-	readonly unlocked!: boolean;
-
-	readonly errors!: string | undefined;
+	readonly locked!: boolean;
 }
 
 export class FrontendNodeType {
@@ -1696,7 +1694,7 @@ export const messageMakers: Record<string, MessageMaker> = {
 	UpdateMouseCursor,
 	UpdateNodeGraphControlBarLayout,
 	UpdateNodeGraphNodes,
-	UpdateNodeGraphError,
+	UpdateNodeGraphErrorDiagnostic,
 	UpdateNodeGraphSelection,
 	UpdateNodeGraphTransform,
 	UpdateNodeGraphWires,

frontend/src/state-providers/node-graph.ts

@@ -26,7 +26,7 @@ import {
 	UpdateNodeGraphTransform,
 	UpdateNodeThumbnail,
 	UpdateWirePathInProgress,
-	UpdateNodeGraphError,
+	UpdateNodeGraphErrorDiagnostic,
 } from "@graphite/messages";
 
 export function createNodeGraphState(editor: Editor) {
@@ -121,9 +121,9 @@ export function createNodeGraphState(editor: Editor) {
 			return state;
 		});
 	});
-	editor.subscriptions.subscribeJsMessage(UpdateNodeGraphError, (updateNodeGraphError) => {
+	editor.subscriptions.subscribeJsMessage(UpdateNodeGraphErrorDiagnostic, (updateNodeGraphErrorDiagnostic) => {
 		update((state) => {
-			state.error = updateNodeGraphError.error;
+			state.error = updateNodeGraphErrorDiagnostic.error;
 			return state;
 		});
 	});