From 05ef746f227c05910fe621bb0c483d6bb30c1e15 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Mon, 6 Jul 2026 11:13:32 +0200 Subject: [PATCH] fix: cluster edges render on left/right handles from approve flow Cluster edges created via the pending -> approve path rendered on the top handle instead of left/right, because the edge and its endpoints lost their handle information on the way to the canvas. - Approve resolver (scan.py) now returns each edge's type + source/target handle. Handle IDs are the bare slot-0 side names ('right'/'left'), the canonical stored form React Flow resolves to the correct side; a '-t' target id fails to resolve and falls back to the top handle. - Frontend injectAutoEdges no longer hardcodes iot/bottom/top-t. It injects each edge with its real type + handles and bumps the referenced nodes' left/right handle counts (which default to 0) so the cluster endpoints exist. Logic extracted to a pure, tested util (applyAutoEdges). - clusterEdges direct-import path uses the bare 'left' target to match. Tests: new autoEdges unit tests; updated backend handle assertions. ha-relevant: maybe --- backend/app/api/routes/scan.py | 23 ++++-- backend/tests/test_proxmox_router.py | 4 +- backend/tests/test_scan.py | 3 +- frontend/src/api/client.ts | 4 +- .../components/modals/PendingDevicesModal.tsx | 16 +--- .../src/components/proxmox/clusterEdges.ts | 7 +- .../src/utils/__tests__/autoEdges.test.ts | 65 ++++++++++++++++ frontend/src/utils/autoEdges.ts | 78 +++++++++++++++++++ 8 files changed, 176 insertions(+), 24 deletions(-) create mode 100644 frontend/src/utils/__tests__/autoEdges.test.ts create mode 100644 frontend/src/utils/autoEdges.ts diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 219a26b..8198c3c 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -642,16 +642,20 @@ async def _resolve_pending_links_for_ieee( if edge_design_id is None: first = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() edge_design_id = first.id if first else None - # Edge shape by link source: + # Edge shape by link source. Handle IDs are the *bare* slot-0 side names + # (the canonical stored form — the save path normalizes '-t' → the + # bare source id, and React Flow resolves the bare id to that side). A + # '-t' target id does not resolve here and RF falls back to the top + # handle, so never emit one. # proxmox → 'virtual' host→guest, vertical (bottom → top) # proxmox_cluster → 'cluster' host↔host, horizontal (right → left) # anything else → 'iot' mesh link, vertical if link.discovery_source == "proxmox": - edge_type, src_handle, tgt_handle = "virtual", "bottom", "top-t" + edge_type, src_handle, tgt_handle = "virtual", "bottom", "top" elif link.discovery_source == "proxmox_cluster": - edge_type, src_handle, tgt_handle = "cluster", "right", "left-t" + edge_type, src_handle, tgt_handle = "cluster", "right", "left" else: - edge_type, src_handle, tgt_handle = "iot", "bottom", "top-t" + edge_type, src_handle, tgt_handle = "iot", "bottom", "top" edge = Edge( source=src_id, target=tgt_id, @@ -663,7 +667,16 @@ async def _resolve_pending_links_for_ieee( db.add(edge) await db.flush() existing_pairs.add((src_id, tgt_id)) - created.append({"id": edge.id, "source": src_id, "target": tgt_id}) + # Return the edge's type + handles so the client injects it faithfully + # (a cluster edge must keep its right→left handles, not the iot default). + created.append({ + "id": edge.id, + "source": src_id, + "target": tgt_id, + "type": edge_type, + "source_handle": src_handle, + "target_handle": tgt_handle, + }) await db.delete(link) return created diff --git a/backend/tests/test_proxmox_router.py b/backend/tests/test_proxmox_router.py index 7d6c135..a7a9bb7 100644 --- a/backend/tests/test_proxmox_router.py +++ b/backend/tests/test_proxmox_router.py @@ -265,7 +265,9 @@ async def test_cluster_link_resolves_to_cluster_edge(db_session) -> None: edge = (await db_session.execute(select(Edge))).scalars().one() assert edge.type == "cluster" assert edge.source_handle == "right" - assert edge.target_handle == "left-t" + # Bare side name (canonical) so React Flow resolves it to the left side; + # a '-t' target would fall back to the top handle. + assert edge.target_handle == "left" @pytest.mark.asyncio diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 8087e20..8e7834d 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -1281,7 +1281,8 @@ async def test_approve_zigbee_creates_edge_when_other_endpoint_is_node( assert edges[0].source == coord.id assert edges[0].target == data["node_id"] assert edges[0].source_handle == "bottom" - assert edges[0].target_handle == "top-t" + # Bare side name (canonical stored form); renders at the top like before. + assert edges[0].target_handle == "top" assert edges[0].type == "iot" diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 3c0aecb..9d64483 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -88,7 +88,7 @@ export const scanApi = { approved: boolean node_id: string edges_created: number - edges: { id: string; source: string; target: string }[] + edges: { id: string; source: string; target: string; type?: string; source_handle?: string | null; target_handle?: string | null }[] }>(`/scan/pending/${id}/approve`, nodeData), hide: (id: string) => api.post(`/scan/pending/${id}/hide`), ignore: (id: string) => api.post(`/scan/pending/${id}/ignore`), @@ -98,7 +98,7 @@ export const scanApi = { node_ids: string[] device_ids: string[] edges_created: number - edges: { id: string; source: string; target: string }[] + edges: { id: string; source: string; target: string; type?: string; source_handle?: string | null; target_handle?: string | null }[] skipped: number }>('/scan/pending/bulk-approve', { device_ids: ids, design_id: designId ?? undefined }), bulkHide: (ids: string[]) => api.post<{ hidden: number; skipped: number }>('/scan/pending/bulk-hide', { device_ids: ids }), diff --git a/frontend/src/components/modals/PendingDevicesModal.tsx b/frontend/src/components/modals/PendingDevicesModal.tsx index 8868074..338384e 100644 --- a/frontend/src/components/modals/PendingDevicesModal.tsx +++ b/frontend/src/components/modals/PendingDevicesModal.tsx @@ -12,6 +12,7 @@ import { resolveNodeColors } from '@/utils/nodeColors' import { toast } from 'sonner' import { PendingDeviceModal, type PendingDevice } from '@/components/modals/PendingDeviceModal' import type { NodeType, ServiceInfo } from '@/types' +import { applyAutoEdges, type AutoEdge } from '@/utils/autoEdges' import { buildZigbeeProperties, isZigbeeType } from '@/utils/zigbeeProperties' import { buildZwaveProperties, isZwaveType } from '@/utils/zwaveProperties' import { buildMacProperty } from '@/utils/macProperty' @@ -100,21 +101,10 @@ function deviceLabel(d: PendingDevice): string { return d.friendly_name ?? d.hostname ?? specialServiceName(d) ?? d.ip ?? d.ieee_address ?? 'device' } -function injectAutoEdges(edges: { id: string; source: string; target: string }[] | undefined) { +function injectAutoEdges(edges: AutoEdge[] | undefined) { if (!edges || edges.length === 0) return useCanvasStore.setState((state) => ({ - edges: [ - ...state.edges, - ...edges.map((e) => ({ - id: e.id, - source: e.source, - target: e.target, - sourceHandle: 'bottom', - targetHandle: 'top-t', - type: 'iot', - data: { type: 'iot' as const }, - })), - ], + ...applyAutoEdges(state.nodes, state.edges, edges), hasUnsavedChanges: true, })) } diff --git a/frontend/src/components/proxmox/clusterEdges.ts b/frontend/src/components/proxmox/clusterEdges.ts index 5a6337a..98dbbc4 100644 --- a/frontend/src/components/proxmox/clusterEdges.ts +++ b/frontend/src/components/proxmox/clusterEdges.ts @@ -9,9 +9,12 @@ */ import type { ProxmoxNode } from './types' -/** Source/target handle IDs for a cluster link (see handleUtils.handleId). */ +/** Source/target handle IDs for a cluster link (see handleUtils.handleId). + * Both are the bare slot-0 side names — the canonical stored form. React Flow + * resolves a bare id to that side; a '-t' target id fails to resolve and falls + * back to the top handle (which is why the target must be 'left', not 'left-t'). */ export const CLUSTER_SOURCE_HANDLE = 'right' -export const CLUSTER_TARGET_HANDLE = 'left-t' +export const CLUSTER_TARGET_HANDLE = 'left' export interface ClusterEdgeSpec { source: string diff --git a/frontend/src/utils/__tests__/autoEdges.test.ts b/frontend/src/utils/__tests__/autoEdges.test.ts new file mode 100644 index 0000000..4f4c8f6 --- /dev/null +++ b/frontend/src/utils/__tests__/autoEdges.test.ts @@ -0,0 +1,65 @@ +import { describe, it, expect } from 'vitest' +import { applyAutoEdges, handleSide, type AutoEdge } from '../autoEdges' +import type { Edge, Node } from '@xyflow/react' +import type { EdgeData, NodeData } from '@/types' + +function node(id: string, data: Partial = {}): Node { + return { + id, + position: { x: 0, y: 0 }, + data: { label: id, type: 'proxmox', status: 'online', services: [], ...data }, + } +} + +describe('handleSide', () => { + it('maps handle ids (with -t / slot suffixes) to their side', () => { + expect(handleSide('left-t')).toBe('left') + expect(handleSide('right')).toBe('right') + expect(handleSide('bottom-2')).toBe('bottom') + expect(handleSide('top')).toBe('top') + expect(handleSide(null)).toBeNull() + }) +}) + +describe('applyAutoEdges', () => { + it('injects a cluster edge with its handles and grants left/right handles', () => { + const nodes = [node('a'), node('b')] + const auto: AutoEdge[] = [ + { id: 'e1', source: 'a', target: 'b', type: 'cluster', source_handle: 'right', target_handle: 'left' }, + ] + const res = applyAutoEdges(nodes, [], auto) + + // Edge keeps its cluster type + right→left handles (not the iot default). + expect(res.edges).toHaveLength(1) + expect(res.edges[0]).toMatchObject({ type: 'cluster', sourceHandle: 'right', targetHandle: 'left' }) + // Source node gained a right handle; target node gained a left handle. + expect(res.nodes.find((n) => n.id === 'a')!.data.right_handles).toBe(1) + expect(res.nodes.find((n) => n.id === 'b')!.data.left_handles).toBe(1) + }) + + it('does not lower an existing higher handle count', () => { + const nodes = [node('a', { right_handles: 3 }), node('b')] + const auto: AutoEdge[] = [ + { id: 'e1', source: 'a', target: 'b', type: 'cluster', source_handle: 'right', target_handle: 'left' }, + ] + const res = applyAutoEdges(nodes, [], auto) + expect(res.nodes.find((n) => n.id === 'a')!.data.right_handles).toBe(3) + }) + + it('defaults to an iot bottom→top edge and bumps no handles', () => { + const nodes = [node('a'), node('b')] + const auto: AutoEdge[] = [{ id: 'e1', source: 'a', target: 'b' }] + const res = applyAutoEdges(nodes, [], auto) + expect(res.edges[0]).toMatchObject({ type: 'iot', sourceHandle: 'bottom', targetHandle: 'top' }) + // top/bottom always exist — nodes are returned untouched. + expect(res.nodes).toBe(nodes) + }) + + it('appends to existing edges rather than replacing them', () => { + const existing = [{ id: 'x', source: 'a', target: 'b' }] as Edge[] + const res = applyAutoEdges([node('a'), node('b')], existing, [ + { id: 'e1', source: 'a', target: 'b', type: 'iot' }, + ]) + expect(res.edges.map((e) => e.id)).toEqual(['x', 'e1']) + }) +}) diff --git a/frontend/src/utils/autoEdges.ts b/frontend/src/utils/autoEdges.ts new file mode 100644 index 0000000..bd9c941 --- /dev/null +++ b/frontend/src/utils/autoEdges.ts @@ -0,0 +1,78 @@ +/** Apply server-created "auto" edges (from scan/import approve) to the canvas. + * + * The approve endpoints create edges server-side and return them with their + * type + handle IDs. We must inject them faithfully: a Proxmox cluster edge + * keeps its right→left handles, mesh links stay iot bottom→top. Left/right + * handles default to 0, so a node referenced on its left/right side must be + * granted that side's connection point or the edge endpoint won't exist and + * React Flow falls back to the top handle. + */ +import type { Edge, Node } from '@xyflow/react' +import type { EdgeData, EdgeType, NodeData } from '@/types' +import { normalizeHandle, handleCountField, type Side } from '@/utils/handleUtils' + +export interface AutoEdge { + id: string + source: string + target: string + type?: string + source_handle?: string | null + target_handle?: string | null +} + +/** Side a handle id sits on ('left-t' → 'left', 'bottom-2' → 'bottom'). */ +export function handleSide(h: string | null | undefined): Side | null { + const bare = normalizeHandle(h) + const m = bare?.match(/^(top|bottom|left|right)/) + return m ? (m[1] as Side) : null +} + +/** + * Pure transform: given current nodes + edges and the server auto-edges, + * return the next nodes (with left/right handle counts bumped where an edge + * needs them) and edges (with the injected edges appended). + */ +export function applyAutoEdges( + nodes: Node[], + edges: Edge[], + autoEdges: AutoEdge[], +): { nodes: Node[]; edges: Edge[] } { + // node id → left/right sides that need at least one handle. + const bumps = new Map>() + const mark = (id: string, side: Side | null) => { + if (!side || side === 'top' || side === 'bottom') return // always exist + const set = bumps.get(id) ?? new Set() + set.add(side) + bumps.set(id, set) + } + + const injected: Edge[] = autoEdges.map((e) => { + const type = (e.type ?? 'iot') as EdgeType + const sourceHandle = e.source_handle ?? 'bottom' + const targetHandle = e.target_handle ?? 'top' + mark(e.source, handleSide(sourceHandle)) + mark(e.target, handleSide(targetHandle)) + return { + id: e.id, + source: e.source, + target: e.target, + sourceHandle, + targetHandle, + type, + data: { type } as EdgeData, + } + }) + + const nextNodes = bumps.size === 0 ? nodes : nodes.map((n) => { + const sides = bumps.get(n.id) + if (!sides) return n + const data = { ...n.data } + for (const side of sides) { + const field = handleCountField(side) + data[field] = Math.max((data[field] as number | undefined) ?? 0, 1) + } + return { ...n, data } + }) + + return { nodes: nextNodes, edges: [...edges, ...injected] } +}