From 3c62a6993c494ecf584e9f4ce253a261b8bde8c0 Mon Sep 17 00:00:00 2001
From: chihlasm <michael@chihlas.com>
Date: Sat, 4 Apr 2026 14:33:00 +0000
Subject: [PATCH] fix: address code review findings for React Flow UI
 integration

- Use screenToFlowPosition() for drop coordinates (fixes zoom/pan bug)
- Remove duplicate selection border from DeviceNode (BaseNode handles it)
- Add w-full to GroupNode for proper container sizing
- Remove unused 'selected' destructuring from DeviceNode

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 frontend/src/components/network/nodes/DeviceNode.tsx |  4 ++--
 .../src/components/network/ui/labeled-group-node.tsx |  2 +-
 frontend/src/pages/NetworkDiagrams/DiagramEditor.tsx | 12 +++---------
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/frontend/src/components/network/nodes/DeviceNode.tsx b/frontend/src/components/network/nodes/DeviceNode.tsx
index 4335b38d..8d45317d 100644
--- a/frontend/src/components/network/nodes/DeviceNode.tsx
+++ b/frontend/src/components/network/nodes/DeviceNode.tsx
@@ -25,7 +25,7 @@ function TooltipRow({ label, value }: { label: string; value: string | null | un
   )
 }
 
-function DeviceNodeComponent({ data, selected }: NodeProps) {
+function DeviceNodeComponent({ data }: NodeProps) {
   const nodeData = data as unknown as DeviceNodeData
   const { icon: Icon, color } = getDeviceRenderConfig(nodeData.deviceType, nodeData.category)
   const status = (nodeData.properties?.status || 'unknown') as NodeStatus
@@ -38,7 +38,7 @@ function DeviceNodeComponent({ data, selected }: NodeProps) {
     <NodeStatusIndicator status={status}>
       <NodeTooltip>
         <NodeTooltipTrigger>
-          <BaseNode className={`min-w-[120px] group ${selected ? 'border-accent' : ''}`}>
+          <BaseNode className="min-w-[120px] group">
             <BaseNodeHeader className="flex-col gap-1 items-center py-3 px-4">
               <Icon size={28} style={{ color }} />
               <BaseNodeHeaderTitle className="text-center text-xs">
diff --git a/frontend/src/components/network/ui/labeled-group-node.tsx b/frontend/src/components/network/ui/labeled-group-node.tsx
index 060335c0..18127dc0 100644
--- a/frontend/src/components/network/ui/labeled-group-node.tsx
+++ b/frontend/src/components/network/ui/labeled-group-node.tsx
@@ -45,7 +45,7 @@ export function GroupNode({ data, selected }: NodeProps) {
   return (
     <BaseNode
       className={cn(
-        'h-full min-h-[200px] min-w-[300px] overflow-hidden rounded-lg bg-elevated/30 border-default/50',
+        'h-full w-full min-h-[200px] min-w-[300px] overflow-hidden rounded-lg bg-elevated/30 border-default/50',
         selected && 'border-accent',
       )}
     >
diff --git a/frontend/src/pages/NetworkDiagrams/DiagramEditor.tsx b/frontend/src/pages/NetworkDiagrams/DiagramEditor.tsx
index 8c0b916a..d8b82740 100644
--- a/frontend/src/pages/NetworkDiagrams/DiagramEditor.tsx
+++ b/frontend/src/pages/NetworkDiagrams/DiagramEditor.tsx
@@ -25,7 +25,7 @@ import type { DeviceNodeData } from '@/components/network/nodes/DeviceNode'
 function DiagramEditorInner() {
   const { id } = useParams<{ id: string }>()
   const navigate = useNavigate()
-  const { getNodes, fitView } = useReactFlow()
+  const { getNodes, fitView, screenToFlowPosition } = useReactFlow()
 
   const [diagramId, setDiagramId] = useState<string | null>(id || null)
   const [name, setName] = useState('Untitled Diagram')
@@ -201,13 +201,7 @@ function DiagramEditorInner() {
   const onDrop = useCallback((event: React.DragEvent) => {
     event.preventDefault()
 
-    const reactFlowBounds = (event.target as HTMLElement).closest('.react-flow')?.getBoundingClientRect()
-    if (!reactFlowBounds) return
-
-    const position = {
-      x: event.clientX - reactFlowBounds.left,
-      y: event.clientY - reactFlowBounds.top,
-    }
+    const position = screenToFlowPosition({ x: event.clientX, y: event.clientY })
 
     // Handle device drops
     const deviceRaw = event.dataTransfer.getData('application/reactflow-device')
@@ -256,7 +250,7 @@ function DiagramEditorInner() {
       setNodes(nds => [...nds, newNode])
       setIsDirty(true)
     }
-  }, [setNodes])
+  }, [setNodes, screenToFlowPosition])
 
   const handleNodeUpdate = useCallback((nodeId: string, updates: Partial<DeviceNodeData>) => {
     setNodes(nds => nds.map(n => {