From 7c49ad0baa363045c69f1db64a3c174be648f929 Mon Sep 17 00:00:00 2001 From: Jan-Henrik Bruhn Date: Tue, 21 Oct 2025 11:28:48 +0200 Subject: [PATCH] feat: render edges between minimized groups with floating edges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements proper edge rendering when both source and target actors are in minimized groups. Edges are now correctly rerouted to connect the minimized group nodes with dynamic floating edge calculations. Key changes: - Add edge rerouting logic to redirect edges from hidden actors to their parent minimized groups - Implement edge deduplication to prevent multiple edges between same groups when multiple actors have connections - Fix floating edge calculations with fallback dimensions to prevent NaN - Use canonical parentId property instead of actorIds array for accuracy - Create constants.ts for MINIMIZED_GROUP_WIDTH/HEIGHT (220x80) - Replace magic numbers throughout codebase with named constants - Remove sourceHandle/targetHandle properties when routing to groups to avoid React Flow validation errors Technical details: - GraphEditor: Build actor-to-group mapping using parentId, deduplicate edges with Map keyed by source_target, strip handle properties - CustomEdge: Use floating edge params for minimized groups on both ends - edgeUtils: Add fallback dimensions and division-by-zero guards in getNodeIntersection and getEdgePosition - graphStore: Use constants for minimized group dimensions Edges between minimized groups now render with proper Bezier curves and dynamic connection points that adapt to group positions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/components/Edges/CustomEdge.tsx | 11 +- src/components/Editor/GraphEditor.tsx | 139 +++++++++++++++++--------- src/constants.ts | 15 +++ src/stores/graphStore.ts | 5 +- src/utils/edgeUtils.ts | 21 ++-- 5 files changed, 131 insertions(+), 60 deletions(-) create mode 100644 src/constants.ts diff --git a/src/components/Edges/CustomEdge.tsx b/src/components/Edges/CustomEdge.tsx index 191c694..ddd884f 100644 --- a/src/components/Edges/CustomEdge.tsx +++ b/src/components/Edges/CustomEdge.tsx @@ -53,7 +53,8 @@ const CustomEdge = ({ const targetIsMinimizedGroup = targetNode?.type === 'group' && (targetNode.data as Group['data']).minimized; // Calculate floating edge parameters if needed - // Only float the side(s) that connect to minimized groups + // When connecting to groups (especially minimized ones), we need to use floating edges + // because groups don't have specific handles let finalSourceX = sourceX; let finalSourceY = sourceY; let finalTargetX = targetX; @@ -61,10 +62,14 @@ const CustomEdge = ({ let finalSourcePosition = sourcePosition; let finalTargetPosition = targetPosition; - if ((sourceIsMinimizedGroup || targetIsMinimizedGroup) && sourceNode && targetNode) { + // Check if we need to use floating edge calculations + const needsFloatingEdge = (sourceIsMinimizedGroup || targetIsMinimizedGroup) && sourceNode && targetNode; + + if (needsFloatingEdge) { const floatingParams = getFloatingEdgeParams(sourceNode, targetNode); - // Only use floating position for the minimized group side(s) + // When either endpoint is a minimized group, use floating positions for that side + // IMPORTANT: When BOTH are groups, we must use floating for BOTH sides if (sourceIsMinimizedGroup) { finalSourceX = floatingParams.sx; finalSourceY = floatingParams.sy; diff --git a/src/components/Editor/GraphEditor.tsx b/src/components/Editor/GraphEditor.tsx index b5d7acc..1f2753f 100644 --- a/src/components/Editor/GraphEditor.tsx +++ b/src/components/Editor/GraphEditor.tsx @@ -170,58 +170,85 @@ const GraphEditor = ({ onNodeSelect, onEdgeSelect, onGroupSelect, onMultiSelect, // Reroute edges to minimized groups and filter internal edges const visibleEdges = useMemo(() => { - // Build a map of actor -> group for actors in minimized groups + // Build a map of actor -> group by examining each actor's parentId + // This is the canonical source of truth in React Flow const actorToMinimizedGroup = new Map(); - storeGroups.forEach((group) => { - if (group.data.minimized) { - group.data.actorIds.forEach((actorId) => { - actorToMinimizedGroup.set(actorId, group.id); - }); + + // Get set of minimized group IDs + const minimizedGroupIds = new Set( + storeGroups.filter((group) => group.data.minimized).map((group) => group.id) + ); + + // Map each actor to its parent group (if the group is minimized) + storeNodes.forEach((node) => { + const nodeWithParent = node as Actor & { parentId?: string }; + if (nodeWithParent.parentId && minimizedGroupIds.has(nodeWithParent.parentId)) { + actorToMinimizedGroup.set(node.id, nodeWithParent.parentId); } }); + // Map to deduplicate edges between groups: "source_target" -> edge + const edgeMap = new Map(); + // Reroute edges: if source or target is in a minimized group, redirect to the group // Filter out edges that are internal to a minimized group (both source and target in same group) - return (storeEdges as Edge[]) - .map((edge) => { - const newSource = actorToMinimizedGroup.get(edge.source) || edge.source; - const newTarget = actorToMinimizedGroup.get(edge.target) || edge.target; + (storeEdges as Edge[]).forEach((edge) => { + const newSource = actorToMinimizedGroup.get(edge.source) || edge.source; + const newTarget = actorToMinimizedGroup.get(edge.target) || edge.target; - const sourceChanged = newSource !== edge.source; - const targetChanged = newTarget !== edge.target; + const sourceChanged = newSource !== edge.source; + const targetChanged = newTarget !== edge.target; - // Filter: if both source and target are rerouted to the SAME group, hide this edge - // (it's an internal edge within a minimized group) - if (sourceChanged && targetChanged && newSource === newTarget) { - return null; // Mark for filtering + // Filter: if both source and target are rerouted to the SAME group, hide this edge + // (it's an internal edge within a minimized group) + if (sourceChanged && targetChanged && newSource === newTarget) { + return; // Skip this edge + } + + // Create edge key for deduplication + const edgeKey = `${newSource}_${newTarget}`; + + // Only update if source or target changed + if (sourceChanged || targetChanged) { + // Destructure to separate handle properties from the rest + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { sourceHandle, targetHandle, ...edgeWithoutHandles } = edge; + + // Create new edge object with updated source/target + const newEdge: Edge = { + ...edgeWithoutHandles, + source: newSource, + target: newTarget, + }; + + // Explicitly delete handle properties to ensure they don't exist + // This is critical - React Flow will error if handles are present but invalid + delete (newEdge as Record).sourceHandle; + delete (newEdge as Record).targetHandle; + + // Only re-add handle IDs if the endpoint was NOT rerouted to a group + if (!sourceChanged && sourceHandle !== undefined && sourceHandle !== null) { + newEdge.sourceHandle = sourceHandle; + } + if (!targetChanged && targetHandle !== undefined && targetHandle !== null) { + newEdge.targetHandle = targetHandle; } - // Only update if source or target changed - if (sourceChanged || targetChanged) { - // Destructure to separate handle properties from the rest - const { sourceHandle, targetHandle, ...edgeWithoutHandles } = edge; - - // Create new edge object, omitting handle properties when rerouting to groups - const newEdge: Edge = { - ...edgeWithoutHandles, - source: newSource, - target: newTarget, - }; - - // Only include handle IDs if not rerouted to a group - if (!sourceChanged && sourceHandle) { - newEdge.sourceHandle = sourceHandle; - } - if (!targetChanged && targetHandle) { - newEdge.targetHandle = targetHandle; - } - - return newEdge; + // If we already have an edge between these nodes, keep the first one + // (you could also merge labels or aggregate data here if needed) + if (!edgeMap.has(edgeKey)) { + edgeMap.set(edgeKey, newEdge); } - return edge; - }) - .filter((edge): edge is Edge => edge !== null); // Remove null entries - }, [storeEdges, storeGroups]); + } else { + // No rerouting needed, just add the edge + if (!edgeMap.has(edgeKey)) { + edgeMap.set(edgeKey, edge); + } + } + }); + + return Array.from(edgeMap.values()); + }, [storeEdges, storeGroups, storeNodes]); const [edges, setEdgesState, onEdgesChange] = useEdgesState( visibleEdges, @@ -284,10 +311,17 @@ const GraphEditor = ({ onNodeSelect, onEdgeSelect, onGroupSelect, onMultiSelect, if (hasPendingSelection) { const pendingEdgeId = pendingType === 'edge' ? pendingId : null; - const newEdges = visibleEdges.map((edge) => ({ - ...edge, - selected: edge.id === pendingEdgeId, - })); + const newEdges = visibleEdges.map((edge) => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { sourceHandle, targetHandle, ...edgeWithoutHandles } = edge; + return { + ...edgeWithoutHandles, + selected: edge.id === pendingEdgeId, + // Only include handles if they exist and are not null + ...(sourceHandle !== undefined && sourceHandle !== null ? { sourceHandle } : {}), + ...(targetHandle !== undefined && targetHandle !== null ? { targetHandle } : {}), + }; + }); // Clear pending selection after applying it to both nodes and edges pendingSelectionRef.current = null; @@ -300,10 +334,17 @@ const GraphEditor = ({ onNodeSelect, onEdgeSelect, onGroupSelect, onMultiSelect, currentEdges.map((edge) => [edge.id, edge.selected]) ); - return visibleEdges.map((edge) => ({ - ...edge, - selected: selectionMap.get(edge.id) || false, - })); + return visibleEdges.map((edge) => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + const { sourceHandle, targetHandle, ...edgeWithoutHandles } = edge; + return { + ...edgeWithoutHandles, + selected: selectionMap.get(edge.id) || false, + // Only include handles if they exist and are not null + ...(sourceHandle !== undefined && sourceHandle !== null ? { sourceHandle } : {}), + ...(targetHandle !== undefined && targetHandle !== null ? { targetHandle } : {}), + }; + }); }); }, [allNodes, visibleEdges, setNodesState, setEdgesState]); diff --git a/src/constants.ts b/src/constants.ts new file mode 100644 index 0000000..a2b186b --- /dev/null +++ b/src/constants.ts @@ -0,0 +1,15 @@ +/** + * Application-wide constants + */ + +/** + * Minimized group dimensions + */ +export const MINIMIZED_GROUP_WIDTH = 220; +export const MINIMIZED_GROUP_HEIGHT = 80; + +/** + * Default actor/node dimensions (approximate) + */ +export const DEFAULT_ACTOR_WIDTH = 150; +export const DEFAULT_ACTOR_HEIGHT = 80; diff --git a/src/stores/graphStore.ts b/src/stores/graphStore.ts index 9e7653f..e028bdd 100644 --- a/src/stores/graphStore.ts +++ b/src/stores/graphStore.ts @@ -11,6 +11,7 @@ import type { GroupData, GraphActions } from '../types'; +import { MINIMIZED_GROUP_WIDTH, MINIMIZED_GROUP_HEIGHT } from '../constants'; /** * ⚠️ IMPORTANT: DO NOT USE THIS STORE DIRECTLY IN COMPONENTS ⚠️ @@ -443,8 +444,8 @@ export const useGraphStore = create((set) => ({ }, style: { ...g.style, - width: 220, - height: 80, + width: MINIMIZED_GROUP_WIDTH, + height: MINIMIZED_GROUP_HEIGHT, }, }; } else { diff --git a/src/utils/edgeUtils.ts b/src/utils/edgeUtils.ts index c3fedba..1b19ca9 100644 --- a/src/utils/edgeUtils.ts +++ b/src/utils/edgeUtils.ts @@ -1,6 +1,7 @@ import type { Relation, RelationData } from '../types'; import type { Node } from '@xyflow/react'; import { Position } from '@xyflow/react'; +import { MINIMIZED_GROUP_WIDTH, MINIMIZED_GROUP_HEIGHT } from '../constants'; /** * Generates a unique ID for edges @@ -21,13 +22,20 @@ function getNodeIntersection(intersectionNode: Node, targetNode: Node) { } = intersectionNode; const targetPosition = targetNode.position; - const w = (intersectionNodeWidth ?? 0) / 2; - const h = (intersectionNodeHeight ?? 0) / 2; + // Use fallback dimensions if width/height are not set (e.g., for groups without measured dimensions) + const w = (intersectionNodeWidth ?? MINIMIZED_GROUP_WIDTH) / 2; + const h = (intersectionNodeHeight ?? MINIMIZED_GROUP_HEIGHT) / 2; const x2 = intersectionNodePosition.x + w; const y2 = intersectionNodePosition.y + h; - const x1 = targetPosition.x + (targetNode.width ?? 0) / 2; - const y1 = targetPosition.y + (targetNode.height ?? 0) / 2; + const x1 = targetPosition.x + (targetNode.width ?? MINIMIZED_GROUP_WIDTH) / 2; + const y1 = targetPosition.y + (targetNode.height ?? MINIMIZED_GROUP_HEIGHT) / 2; + + // Guard against division by zero + if (w === 0 || h === 0) { + // If node has no dimensions, return center point + return { x: x2, y: y2 }; + } const xx1 = (x1 - x2) / (2 * w) - (y1 - y2) / (2 * h); const yy1 = (x1 - x2) / (2 * w) + (y1 - y2) / (2 * h); @@ -50,8 +58,9 @@ function getEdgePosition(node: Node, intersectionPoint: { x: number; y: number } const px = Math.round(intersectionPoint.x); const py = Math.round(intersectionPoint.y); - const width = node.width ?? 0; - const height = node.height ?? 0; + // Use fallback dimensions if not set (same as getNodeIntersection) + const width = node.width ?? MINIMIZED_GROUP_WIDTH; + const height = node.height ?? MINIMIZED_GROUP_HEIGHT; if (px <= nx + 1) { return Position.Left;