From 0792105b96e1ad031d9ed879f40367d589e18858 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Thu, 9 Jul 2026 13:05:19 +0200 Subject: [PATCH] feat: prompt on duplicate device instead of silently blocking/merging approve MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single-device approve now guards duplicates per-design the same way bulk approve does, and asks the user instead of failing or silently merging: - create_node and approve_device reject a same-design duplicate (ieee, ip or mac) with 409 + the existing node; a force flag creates it anyway. - Frontend shows a confirm dialog: go to existing node, add duplicate anyway, or cancel. - approve_device no longer rejects a device already on another canvas (status is global, canvas membership is per-design) — it can be placed on a new design, matching bulk approve. - IEEE (Zigbee/Z-Wave) devices now use the same prompt as ip/mac instead of auto-merging into the existing node. - bulk approve reports which devices it skipped as duplicates. Closes #260 ha-relevant: maybe --- backend/app/api/routes/nodes.py | 12 + backend/app/api/routes/scan.py | 106 +++++---- backend/app/schemas/nodes.py | 4 + backend/app/services/node_dedupe.py | 56 ++++- backend/tests/test_nodes.py | 41 ++++ backend/tests/test_scan.py | 222 +++++++++++++++++- frontend/src/api/client.ts | 21 ++ .../components/modals/PendingDevicesModal.tsx | 152 ++++++++++-- .../__tests__/PendingDevicesModal.test.tsx | 100 +++++++- 9 files changed, 629 insertions(+), 85 deletions(-) diff --git a/backend/app/api/routes/nodes.py b/backend/app/api/routes/nodes.py index 30d3f28..be04106 100644 --- a/backend/app/api/routes/nodes.py +++ b/backend/app/api/routes/nodes.py @@ -6,6 +6,7 @@ from app.api.deps import get_current_user from app.db.database import get_db from app.db.models import Design, Node from app.schemas.nodes import NodeCreate, NodeResponse, NodeUpdate +from app.services.node_dedupe import find_duplicate_node router = APIRouter() @@ -19,6 +20,8 @@ async def list_nodes(db: AsyncSession = Depends(get_db), _: str = Depends(get_cu @router.post("", response_model=NodeResponse, status_code=status.HTTP_201_CREATED) async def create_node(body: NodeCreate, db: AsyncSession = Depends(get_db), _: str = Depends(get_current_user)) -> Node: data = body.model_dump() + # `force` bypasses the duplicate guard below; it is not a Node column. + force = data.pop("force", False) # Attach to a design so the node lands on a canvas. Clients that don't send a # design_id (e.g. the MCP write tools) would otherwise create design_id=null # nodes that exist in the DB but never render in the UI until a container @@ -26,6 +29,15 @@ async def create_node(body: NodeCreate, db: AsyncSession = Depends(get_db), _: s if data.get("design_id") is None: first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() data["design_id"] = first_design.id if first_design else None + + # Reject a silent duplicate: a node with the same ip OR mac already on the + # target design. Scripts/MCP clients get a clear 409 (with the existing id) + # instead of a second card for the same host. Pass force=True to override. + if not force: + dup = await find_duplicate_node(db, data["design_id"], data.get("ip"), data.get("mac")) + if dup is not None: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=dup) + node = Node(**data) db.add(node) await db.commit() diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index 49e1db1..8ad1f4b 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -15,7 +15,7 @@ from app.db.database import AsyncSessionLocal, get_db from app.db.models import Design, Edge, Node, PendingDevice, PendingDeviceLink, ScanRun from app.schemas.nodes import NodeCreate from app.schemas.scan import PendingDeviceResponse, ScanRunResponse -from app.services.node_dedupe import dedupe_nodes_by_ieee +from app.services.node_dedupe import dedupe_nodes_by_ieee, find_duplicate_node from app.services.scanner import DeepScanOptions, _valid_port_range, request_cancel, run_scan from app.services.zigbee_service import ( build_zigbee_properties, @@ -335,23 +335,37 @@ async def bulk_approve_devices( devices = result.scalars().all() # What already sits on the target canvas, so we skip devices already placed - # here (by ip or ieee_address) instead of creating duplicate nodes. + # here (by ip or ieee_address) instead of creating duplicate nodes. We map to + # the existing node id so the skip report can point the user at it. A value + # may be a Node still pending flush (in-batch duplicate) — resolved to its id + # after the flush below. existing = ( await db.execute( - select(Node.ip, Node.ieee_address).where(Node.design_id == default_design_id) + select(Node.id, Node.ip, Node.ieee_address).where(Node.design_id == default_design_id) ) ).all() - placed_ips = {ip for ip, _ in existing if ip} - placed_ieee = {ieee for _, ieee in existing if ieee} + placed_ips: dict[str, Any] = {ip: nid for nid, ip, _ in existing if ip} + placed_ieee: dict[str, Any] = {ieee: nid for nid, _, ieee in existing if ieee} created_nodes: list[Node] = [] approved_devices: list[PendingDevice] = [] + skipped_devices: list[dict[str, Any]] = [] for device in devices: - already_here = ( - (device.ip is not None and device.ip in placed_ips) - or (device.ieee_address is not None and device.ieee_address in placed_ieee) - ) - if already_here: + # Record which identifier collided so the caller can explain each skip + # (and, for existing on-canvas nodes, link to the node already there). + if device.ip is not None and device.ip in placed_ips: + skipped_devices.append({ + "device_id": device.id, + "label": device.hostname or device.friendly_name or device.ip or "device", + "match": "ip", "value": device.ip, "_ref": placed_ips[device.ip], + }) + continue + if device.ieee_address is not None and device.ieee_address in placed_ieee: + skipped_devices.append({ + "device_id": device.id, + "label": device.hostname or device.friendly_name or device.ieee_address or "device", + "match": "ieee", "value": device.ieee_address, "_ref": placed_ieee[device.ieee_address], + }) continue device.status = "approved" node_type = device.suggested_type or "generic" @@ -381,16 +395,23 @@ async def bulk_approve_devices( created_nodes.append(node) approved_devices.append(device) # Track within this batch so a duplicate selection (same ip/ieee) is not - # placed twice on the same canvas. + # placed twice on the same canvas. Store the Node so a later in-batch + # skip can resolve to its id after flush. if device.ip: - placed_ips.add(device.ip) + placed_ips[device.ip] = node if device.ieee_address: - placed_ieee.add(device.ieee_address) + placed_ieee[device.ieee_address] = node await db.flush() # populates node.id from Python-side default before reading # node_ids and approved_device_ids stay index-aligned for the client's mapping. node_ids = [n.id for n in created_nodes] approved_device_ids = [d.id for d in approved_devices] + # Resolve each skip's existing-node reference to a concrete id now that any + # in-batch nodes have been flushed, and expose it under a clean key. + for entry in skipped_devices: + ref = entry.pop("_ref") + entry["existing_node_id"] = ref.id if isinstance(ref, Node) else ref + all_edges: list[dict[str, str]] = [] for device in approved_devices: all_edges.extend( @@ -405,6 +426,7 @@ async def bulk_approve_devices( "edges_created": len(all_edges), "edges": all_edges, "skipped": len(payload.device_ids) - len(node_ids), + "skipped_devices": skipped_devices, } @@ -478,41 +500,35 @@ async def approve_device( device = await db.get(PendingDevice, device_id) if not device: raise HTTPException(status_code=404, detail="Device not found") - if device.status != "pending": - raise HTTPException(status_code=409, detail="Device already processed") - device.status = "approved" + # A device's status is GLOBAL — it flips to "approved" the moment it lands on + # ANY canvas — but canvas membership is per-design. Approving onto a NEW + # design must therefore work even when the device already sits on another + # one (mirroring bulk_approve_devices, which deliberately does not filter on + # status == "pending"). Same-design duplicates are caught by the per-design + # IEEE/ip/mac guards below, not by this global flag. Only a user-hidden + # device is off-limits here. + if device.status == "hidden": + raise HTTPException(status_code=409, detail="Device is hidden") wireless = _is_wireless(node_data.type) - # Guard against a true duplicate: same IEEE already placed on THIS design. - # (The same device on a *different* design is valid — one Node per canvas.) - if device.ieee_address is not None: - dup = ( - await db.execute( - select(Node).where( - Node.ieee_address == device.ieee_address, - Node.design_id == node_design_id, - ) - ) - ).scalars().first() - if dup is not None: - dup.properties = merge_zigbee_properties( - dup.properties, - _wireless_properties( - node_data.type, device.ieee_address, - device.vendor, device.model, device.lqi, - ) if wireless else [], - ) - edges = await _resolve_pending_links_for_ieee( - db, device.ieee_address, node_design_id - ) - await db.commit() - return { - "approved": True, - "node_id": dup.id, - "edges_created": len(edges), - "edges": edges, - } + # A device already on THIS design (matched by ieee, ip OR mac) is NOT placed + # again automatically: the user might genuinely want a second card, or might + # be re-approving by mistake. Reject with 409 + the existing node so the UI + # can ask — identical handling for IEEE (Zigbee/Z-Wave) and plain IP/ARP + # hosts. force=True (set after the user confirms) skips this and creates it. + # The same device on a *different* design is valid (one Node per canvas), so + # this is scoped to node_design_id. + if not node_data.force: + conflict = await find_duplicate_node( + db, node_design_id, + node_data.ip or device.ip, + node_data.mac or device.mac, + ieee=device.ieee_address, + ) + if conflict is not None: + raise HTTPException(status_code=409, detail=conflict) + device.status = "approved" # Prefer the MAC discovered during the scan (stored on the pending device); # fall back to whatever the approve payload carried. _mac = device.mac or node_data.mac diff --git a/backend/app/schemas/nodes.py b/backend/app/schemas/nodes.py index 23d3ea6..e01fcc3 100644 --- a/backend/app/schemas/nodes.py +++ b/backend/app/schemas/nodes.py @@ -39,6 +39,10 @@ class NodeBase(BaseModel): class NodeCreate(NodeBase): design_id: str | None = None + # When a node with the same ip/mac already exists on the target design, the + # create/approve endpoints reject with 409 so the UI can ask the user. Set + # force=True to bypass that guard and create the duplicate deliberately. + force: bool = False class NodeUpdate(BaseModel): diff --git a/backend/app/services/node_dedupe.py b/backend/app/services/node_dedupe.py index 76fa1cd..be7b196 100644 --- a/backend/app/services/node_dedupe.py +++ b/backend/app/services/node_dedupe.py @@ -20,7 +20,7 @@ from __future__ import annotations import logging from typing import Any -from sqlalchemy import select +from sqlalchemy import or_, select from sqlalchemy.ext.asyncio import AsyncSession from app.db.models import Edge, Node @@ -28,6 +28,60 @@ from app.services.zigbee_service import merge_zigbee_properties logger = logging.getLogger(__name__) + +async def find_duplicate_node( + db: AsyncSession, + design_id: str | None, + ip: str | None, + mac: str | None, + ieee: str | None = None, +) -> dict[str, Any] | None: + """Return conflict details if an equivalent node (same ieee, ip OR mac) + already sits on ``design_id``, else ``None``. + + Scoped to a single design on purpose: the same device may legitimately + appear on several canvases (one :class:`Node` per design). Only a second + node for the same ieee/ip/mac on the *same* design is a duplicate — which + the create/approve endpoints turn into a 409 so the UI can offer "go to + existing" vs "add duplicate anyway", uniformly for IEEE (Zigbee/Z-Wave) and + plain IP/ARP hosts. + + (:func:`dedupe_nodes_by_ieee` still repairs *pre-existing* same-canvas IEEE + duplicates; this guard prevents new ones unless the user forces them.) + """ + conds = [] + if ieee: + conds.append(Node.ieee_address == ieee) + if ip: + conds.append(Node.ip == ip) + if mac: + conds.append(Node.mac == mac) + if not conds: + return None + existing = ( + await db.execute( + select(Node).where(Node.design_id == design_id, or_(*conds)) + ) + ).scalars().first() + if existing is None: + return None + # Report whichever field actually matched (ieee > ip > mac when several do). + match: str + value: str | None + if ieee and existing.ieee_address == ieee: + match, value = "ieee", existing.ieee_address + elif ip and existing.ip == ip: + match, value = "ip", existing.ip + else: + match, value = "mac", existing.mac + return { + "duplicate": True, + "existing_node_id": existing.id, + "existing_label": existing.label, + "match": match, + "value": value, + } + # Scalar Node fields worth carrying over from a duplicate when the canonical # node has no value. Positions, design, parent and identity fields are left on # the canonical node untouched (that's the row the user actually placed). diff --git a/backend/tests/test_nodes.py b/backend/tests/test_nodes.py index fafe670..3d2d387 100644 --- a/backend/tests/test_nodes.py +++ b/backend/tests/test_nodes.py @@ -99,6 +99,47 @@ async def test_create_node_respects_explicit_design_id(client: AsyncClient, head assert res.json()["design_id"] == second_id +async def test_create_node_rejects_duplicate_ip_on_same_design(client: AsyncClient, headers: dict): + # A second node with the same ip on the same design is a silent duplicate — + # scripts/MCP clients get 409 with the existing node id instead. (#260) + design = await client.post("/api/v1/designs", json={"name": "D"}, headers=headers) + design_id = design.json()["id"] + first = await client.post( + "/api/v1/nodes", + json={"type": "server", "label": "srv", "ip": "192.168.1.5", "design_id": design_id}, + headers=headers, + ) + assert first.status_code == 201 + existing_id = first.json()["id"] + + dup = await client.post( + "/api/v1/nodes", + json={"type": "server", "label": "srv-again", "ip": "192.168.1.5", "design_id": design_id}, + headers=headers, + ) + assert dup.status_code == 409 + detail = dup.json()["detail"] + assert detail["duplicate"] is True + assert detail["existing_node_id"] == existing_id + assert detail["match"] == "ip" + + +async def test_create_node_force_bypasses_duplicate_guard(client: AsyncClient, headers: dict): + design = await client.post("/api/v1/designs", json={"name": "D"}, headers=headers) + design_id = design.json()["id"] + await client.post( + "/api/v1/nodes", + json={"type": "server", "label": "srv", "ip": "192.168.1.5", "design_id": design_id}, + headers=headers, + ) + forced = await client.post( + "/api/v1/nodes", + json={"type": "server", "label": "srv", "ip": "192.168.1.5", "design_id": design_id, "force": True}, + headers=headers, + ) + assert forced.status_code == 201 + + async def test_create_node_without_any_design_stays_null(client: AsyncClient, headers: dict): # No designs exist yet: fallback can't invent one, so design_id stays null # rather than erroring. diff --git a/backend/tests/test_scan.py b/backend/tests/test_scan.py index 4af6a90..19a0713 100644 --- a/backend/tests/test_scan.py +++ b/backend/tests/test_scan.py @@ -312,11 +312,11 @@ async def test_approve_device(client: AsyncClient, headers, pending_device): @pytest.mark.asyncio -async def test_approve_device_no_dupe_same_ieee_same_design( +async def test_approve_device_conflicts_on_existing_ieee_same_design( client: AsyncClient, headers, db_session ): - """Approving a device whose IEEE is already on the target design must reuse - the existing node, not create a duplicate (source of the crash bug).""" + """Approving a device whose IEEE is already on the target design prompts the + user (409) instead of silently merging/replacing — same UX as ip/mac.""" design = Design(name="d1") db_session.add(design) await db_session.flush() @@ -340,13 +340,195 @@ async def test_approve_device_no_dupe_same_ieee_same_design( }, headers=headers, ) - assert res.status_code == 200 - assert res.json()["node_id"] == existing.id # reused, not new + assert res.status_code == 409 + detail = res.json()["detail"] + assert detail["duplicate"] is True + assert detail["existing_node_id"] == existing.id + assert detail["match"] == "ieee" + assert detail["value"] == "0xZZZ" + # No second node created; device stays pending until the user decides. nodes = ( await db_session.execute(select(Node).where(Node.ieee_address == "0xZZZ")) ).scalars().all() - assert len(nodes) == 1 # no duplicate created + assert len(nodes) == 1 + + +@pytest.mark.asyncio +async def test_approve_device_force_creates_duplicate_ieee( + client: AsyncClient, headers, db_session +): + """force=True lets the user place a second card for the same IEEE.""" + design = Design(name="d1") + db_session.add(design) + await db_session.flush() + db_session.add(Node( + label="sensor", type="zigbee_enddevice", ieee_address="0xZZZ", + services=[], design_id=design.id, + )) + device = PendingDevice( + id=str(uuid.uuid4()), ieee_address="0xZZZ", suggested_type="zigbee_enddevice", + status="pending", discovery_source="zigbee", + ) + db_session.add(device) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{device.id}/approve", + json={ + "label": "sensor", "type": "zigbee_enddevice", "status": "online", + "services": [], "design_id": design.id, "force": True, + }, + headers=headers, + ) + assert res.status_code == 200 + nodes = ( + await db_session.execute(select(Node).where(Node.ieee_address == "0xZZZ")) + ).scalars().all() + assert len(nodes) == 2 + + +@pytest.mark.asyncio +async def test_approve_device_conflicts_on_existing_ip( + client: AsyncClient, headers, db_session, pending_device +): + """An ordinary host whose ip already sits on the target design is NOT + silently duplicated: the approve returns 409 with the existing node so the + UI can ask the user.""" + design = await _add_design(db_session, "Home") + existing = _node(design, ip="192.168.1.100") + db_session.add(existing) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "dup", "type": "server", "ip": "192.168.1.100", + "status": "unknown", "services": [], "design_id": design}, + headers=headers, + ) + assert res.status_code == 409 + detail = res.json()["detail"] + assert detail["duplicate"] is True + assert detail["existing_node_id"] == existing.id + assert detail["match"] == "ip" + assert detail["value"] == "192.168.1.100" + + # No node created, device left pending (user hasn't decided yet). + nodes = (await db_session.execute(select(Node).where(Node.design_id == design))).scalars().all() + assert len(nodes) == 1 + await db_session.refresh(pending_device) + assert pending_device.status == "pending" + + +@pytest.mark.asyncio +async def test_approve_device_conflicts_on_existing_mac( + client: AsyncClient, headers, db_session, pending_device +): + """MAC match (device re-IP'd via DHCP) also triggers the duplicate guard.""" + design = await _add_design(db_session, "Home") + existing = Node(id=str(uuid.uuid4()), label="n", type="server", status="online", + ip="10.0.0.9", mac="aa:bb:cc:dd:ee:ff", services=[], design_id=design) + db_session.add(existing) + await db_session.commit() + + # pending_device carries mac aa:bb:cc:dd:ee:ff but a different ip. + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "dup", "type": "server", "ip": "192.168.1.55", + "mac": "aa:bb:cc:dd:ee:ff", "status": "unknown", "services": [], + "design_id": design}, + headers=headers, + ) + assert res.status_code == 409 + assert res.json()["detail"]["match"] == "mac" + + +@pytest.mark.asyncio +async def test_approve_device_force_creates_duplicate( + client: AsyncClient, headers, db_session, pending_device +): + """force=True (user confirmed) bypasses the guard and creates the node.""" + design = await _add_design(db_session, "Home") + db_session.add(_node(design, ip="192.168.1.100")) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "dup", "type": "server", "ip": "192.168.1.100", + "status": "unknown", "services": [], "design_id": design, "force": True}, + headers=headers, + ) + assert res.status_code == 200 + nodes = (await db_session.execute(select(Node).where(Node.design_id == design))).scalars().all() + assert len(nodes) == 2 # duplicate deliberately created + + +@pytest.mark.asyncio +async def test_approve_device_allows_same_ip_on_other_design( + client: AsyncClient, headers, db_session, pending_device +): + """The guard is per-design: the same host on a different canvas is fine.""" + other = await _add_design(db_session, "Lab") + target = await _add_design(db_session, "Home") + db_session.add(_node(other, ip="192.168.1.100")) # exists on a DIFFERENT design + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "ok", "type": "server", "ip": "192.168.1.100", + "status": "unknown", "services": [], "design_id": target}, + headers=headers, + ) + assert res.status_code == 200 + + +@pytest.mark.asyncio +async def test_approve_device_places_already_approved_on_another_design( + client: AsyncClient, headers, db_session +): + """A device already approved on ANOTHER canvas (global status="approved") + must still be placeable on a new design — status is global, canvas + membership is per-design (mirrors bulk_approve).""" + other = await _add_design(db_session, "Other") + target = await _add_design(db_session, "Network Topology") + # Device is on `other` already (its global status is "approved"). + db_session.add(_node(other, ieee="0x00158d0005292b83")) + device = PendingDevice( + id=str(uuid.uuid4()), ieee_address="0x00158d0005292b83", + suggested_type="zigbee_enddevice", status="approved", + discovery_source="zigbee", + ) + db_session.add(device) + await db_session.commit() + + res = await client.post( + f"/api/v1/scan/pending/{device.id}/approve", + json={"label": "sensor", "type": "zigbee_enddevice", "status": "online", + "services": [], "design_id": target}, + headers=headers, + ) + assert res.status_code == 200 + # A node now exists on the target design too (one per canvas). + nodes = ( + await db_session.execute( + select(Node).where(Node.ieee_address == "0x00158d0005292b83") + ) + ).scalars().all() + assert {n.design_id for n in nodes} == {other, target} + + +@pytest.mark.asyncio +async def test_approve_device_rejects_hidden(client: AsyncClient, headers, db_session, pending_device): + """A user-hidden device is not approvable via this endpoint.""" + pending_device.status = "hidden" + db_session.add(pending_device) + await db_session.commit() + res = await client.post( + f"/api/v1/scan/pending/{pending_device.id}/approve", + json={"label": "x", "type": "server", "status": "unknown", "services": []}, + headers=headers, + ) + assert res.status_code == 409 @pytest.mark.asyncio @@ -903,6 +1085,34 @@ async def test_bulk_approve_skips_device_already_on_target_design( assert sorted(n.ip for n in nodes) == ["192.168.1.10", "192.168.1.11"] +@pytest.mark.asyncio +async def test_bulk_approve_reports_skipped_devices( + client: AsyncClient, headers, db_session, two_pending_devices +): + """Bulk can't prompt per-device, so it reports each duplicate it skipped + (with the existing node id) instead of silently dropping it.""" + ids = [d.id for d in two_pending_devices] + design = await _add_design(db_session, "Canvas") + existing = _node(design, ip="192.168.1.10") + db_session.add(existing) + await db_session.commit() + + res = await client.post( + "/api/v1/scan/pending/bulk-approve", + json={"device_ids": ids, "design_id": design}, + headers=headers, + ) + data = res.json() + assert data["approved"] == 1 + skipped = data["skipped_devices"] + assert len(skipped) == 1 + entry = skipped[0] + assert entry["match"] == "ip" + assert entry["value"] == "192.168.1.10" + assert entry["existing_node_id"] == existing.id + assert entry["device_id"] in ids + + @pytest.fixture async def zigbee_pending_device(db_session): device = PendingDevice( diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index bd091c9..2486b85 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -77,6 +77,26 @@ export interface DeepScanConfig { export type ScanConfigData = { ranges: string[] } & DeepScanConfig +// A device the backend refused to place because an equivalent node already +// exists on the target design (same ip/mac/ieee). `existing_node_id` points at +// the node already there so the UI can link to it. +export interface SkippedDevice { + device_id: string + label: string + match: 'ip' | 'mac' | 'ieee' + value: string + existing_node_id: string | null +} + +// 409 body from single approve / create when a same-design duplicate is found. +export interface DuplicateNodeConflict { + duplicate: true + existing_node_id: string + existing_label: string + match: 'ip' | 'mac' | 'ieee' + value: string +} + export const scanApi = { trigger: (deepScan?: Partial) => api.post('/scan/trigger', deepScan ?? {}), pending: () => api.get('/scan/pending'), @@ -100,6 +120,7 @@ export const scanApi = { edges_created: number edges: { id: string; source: string; target: string; type?: string; source_handle?: string | null; target_handle?: string | null }[] skipped: number + skipped_devices: SkippedDevice[] }>('/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 }), restore: (id: string) => api.post<{ restored: boolean; device_id: string }>(`/scan/pending/${id}/restore`), diff --git a/frontend/src/components/modals/PendingDevicesModal.tsx b/frontend/src/components/modals/PendingDevicesModal.tsx index 4ad78ba..1ea9fd2 100644 --- a/frontend/src/components/modals/PendingDevicesModal.tsx +++ b/frontend/src/components/modals/PendingDevicesModal.tsx @@ -4,7 +4,7 @@ import { Search, RefreshCw, X, CheckCircle2, EyeOff, Trash2, Loader2, ServerCog, } from 'lucide-react' import { Dialog, DialogClose, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog' -import { scanApi } from '@/api/client' +import { scanApi, type DuplicateNodeConflict } from '@/api/client' import { useCanvasStore } from '@/stores/canvasStore' import { useDesignStore } from '@/stores/designStore' import { useThemeStore } from '@/stores/themeStore' @@ -93,6 +93,18 @@ function deviceLabel(d: PendingDevice): string { return d.friendly_name ?? d.hostname ?? specialServiceName(d) ?? d.ip ?? d.ieee_address ?? 'device' } +// Pull the duplicate-conflict payload out of a 409 approve response, if that's +// what the error is. Anything else (network, 500, non-duplicate 409) → null. +function extractDuplicateConflict(err: unknown): DuplicateNodeConflict | null { + const detail = (err as { response?: { status?: number; data?: { detail?: unknown } } })?.response + if (detail?.status !== 409) return null + const body = detail.data?.detail + if (body && typeof body === 'object' && (body as DuplicateNodeConflict).duplicate) { + return body as DuplicateNodeConflict + } + return null +} + function injectAutoEdges(edges: AutoEdge[] | undefined) { if (!edges || edges.length === 0) return useCanvasStore.setState((state) => ({ @@ -116,8 +128,12 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus // Optionally restrict to devices that have at least one detected service. const [withServicesOnly, setWithServicesOnly] = useState(false) const { addNode, scanEventTs } = useCanvasStore() + const setSelectedNode = useCanvasStore((s) => s.setSelectedNode) const activeDesignId = useDesignStore((s) => s.activeDesignId) const highlightRef = useRef(null) + // Set when a single approve is refused because the same host is already on + // this design — the user decides: link to the existing node or duplicate. + const [dupPrompt, setDupPrompt] = useState<{ device: PendingDevice; conflict: DuplicateNodeConflict } | null>(null) const load = useCallback(async () => { setLoading(true) @@ -253,30 +269,32 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus } } - const handleApprove = async (device: PendingDevice) => { + // force=true is sent only after the user confirms they want a duplicate node + // on this design (the backend otherwise 409s to let us ask). + const approveDevice = async (device: PendingDevice, force = false) => { + const fallbackLabel = deviceLabel(device) + const type = (device.suggested_type ?? 'generic') as NodeType + const zwave = isZwaveType(type) + const wireless = isZigbeeType(type) || zwave + const properties = zwave + ? buildZwaveProperties(device) + : isZigbeeType(type) + ? buildZigbeeProperties(device) + : [...(device.properties ?? []), ...buildMacProperty(device.mac)] + const nodeData = { + label: fallbackLabel, + type, + ip: device.ip ?? undefined, + mac: device.mac ?? undefined, + hostname: device.hostname ?? undefined, + status: wireless ? 'online' : 'unknown', + services: (device.services ?? []) as ServiceInfo[], + properties, + // Approve onto the design the user is viewing, not the first design. + design_id: activeDesignId ?? undefined, + } try { - const fallbackLabel = deviceLabel(device) - const type = (device.suggested_type ?? 'generic') as NodeType - const zwave = isZwaveType(type) - const wireless = isZigbeeType(type) || zwave - const properties = zwave - ? buildZwaveProperties(device) - : isZigbeeType(type) - ? buildZigbeeProperties(device) - : [...(device.properties ?? []), ...buildMacProperty(device.mac)] - const nodeData = { - label: fallbackLabel, - type, - ip: device.ip ?? undefined, - mac: device.mac ?? undefined, - hostname: device.hostname ?? undefined, - status: wireless ? 'online' : 'unknown', - services: (device.services ?? []) as ServiceInfo[], - properties, - // Approve onto the design the user is viewing, not the first design. - design_id: activeDesignId ?? undefined, - } - const res = await scanApi.approve(device.id, nodeData) + const res = await scanApi.approve(device.id, { ...nodeData, force }) const nodeId = res.data.node_id addNode({ id: nodeId, @@ -290,12 +308,32 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus // Keep the row (now on-canvas, shown with an "In N canvas" badge); reload // for a fresh canvas_count rather than dropping it until reopen. setSelected(null) + setDupPrompt(null) await load() - } catch { + } catch (err) { + const conflict = extractDuplicateConflict(err) + // Only ask on the first (non-forced) attempt; a forced retry that still + // fails is a real error. + if (conflict && !force) { + // Close the device-detail modal first: two Base UI dialogs open at once + // trap focus on the underlying one and the prompt never shows. + setSelected(null) + setDupPrompt({ device, conflict }) + return + } toast.error('Failed to approve device') } } + const handleApprove = (device: PendingDevice) => approveDevice(device, false) + + const goToExistingNode = (nodeId: string) => { + setSelectedNode(nodeId) + setDupPrompt(null) + setSelected(null) + onClose() + } + const handleHide = async (device: PendingDevice) => { try { await scanApi.hide(device.id) @@ -362,6 +400,17 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus await load() const linkExtra = res.data.edges_created > 0 ? ` (+${res.data.edges_created} link${res.data.edges_created !== 1 ? 's' : ''})` : '' toast.success(`Approved ${res.data.approved} device${res.data.approved !== 1 ? 's' : ''}${linkExtra}`) + // Bulk can't prompt per-device, so report the ones already on this canvas + // (skipped as duplicates) instead of silently dropping them. + const dupes = res.data.skipped_devices ?? [] + if (dupes.length > 0) { + const names = dupes.slice(0, 3).map((d) => d.label).join(', ') + const more = dupes.length > 3 ? ` +${dupes.length - 3} more` : '' + toast.info( + `${dupes.length} already on this canvas, skipped: ${names}${more}`, + { description: 'Matched an existing node by IP/MAC/IEEE on this design.' }, + ) + } } catch { toast.error('Failed to bulk approve devices') } @@ -629,6 +678,59 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus )} )} + + {/* Duplicate confirmation: the host is already on this design. Ask + before creating a second card — never silently drop or duplicate. + Rendered INSIDE the inventory DialogContent on purpose: an open + Base UI Dialog marks all outside content inert/aria-hidden, so a + sibling overlay (or nested dialog) would be unclickable. Keeping it + in the dialog's own subtree avoids that. */} + {dupPrompt && ( +
setDupPrompt(null)} + > +
e.stopPropagation()} + > +

Device already on this canvas

+
+

+ {deviceLabel(dupPrompt.device)}{' '} + matches a node already on this design + {' '}({dupPrompt.conflict.match}{' '} + {dupPrompt.conflict.value}):{' '} + {dupPrompt.conflict.existing_label}. +

+

Add it again anyway, or jump to the existing node?

+
+ + + +
+
+
+
+ )} diff --git a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx index 8623855..d4d527f 100644 --- a/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx +++ b/frontend/src/components/modals/__tests__/PendingDevicesModal.test.tsx @@ -14,6 +14,7 @@ const mockHide = vi.fn() const mockPending = vi.fn() const mockHidden = vi.fn() const mockAddNode = vi.fn() +const mockSetSelectedNode = vi.fn() vi.mock('@/api/client', () => ({ scanApi: { @@ -30,11 +31,15 @@ vi.mock('@/api/client', () => ({ }, })) -vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn() } })) +vi.mock('sonner', () => ({ toast: { success: vi.fn(), error: vi.fn(), info: vi.fn() } })) vi.mock('@/components/modals/PendingDeviceModal', () => ({ - PendingDeviceModal: ({ device }: { device: unknown }) => - device ?
: null, + PendingDeviceModal: ({ device, onApprove }: { device: unknown; onApprove: (d: unknown) => void }) => + device ? ( +
+ +
+ ) : null, })) const DEVICE_IP = { @@ -104,10 +109,12 @@ const DEVICE_PROXMOX = { beforeEach(() => { vi.clearAllMocks() - vi.mocked(useCanvasStore).mockReturnValue({ - addNode: mockAddNode, - scanEventTs: 0, - } as unknown as ReturnType) + // Apply the selector when one is passed (setSelectedNode is read via a + // selector), else return the whole store (destructured in the component). + vi.mocked(useCanvasStore).mockImplementation(((sel?: (s: unknown) => unknown) => { + const store = { addNode: mockAddNode, scanEventTs: 0, setSelectedNode: mockSetSelectedNode } + return sel ? sel(store) : store + }) as unknown as typeof useCanvasStore) // setState is used by injectAutoEdges ;(useCanvasStore as unknown as { setState: (fn: unknown) => void }).setState = vi.fn() mockPending.mockResolvedValue({ data: [DEVICE_IP, DEVICE_ZIGBEE] }) @@ -115,7 +122,7 @@ beforeEach(() => { mockApprove.mockResolvedValue({ data: { node_id: 'n1', edges: [], edges_created: 0 } }) mockHide.mockResolvedValue({ data: {} }) mockBulkApprove.mockResolvedValue({ - data: { approved: 2, node_ids: ['n1', 'n2'], device_ids: ['dev-a', 'dev-b'], edges: [], edges_created: 0 }, + data: { approved: 2, node_ids: ['n1', 'n2'], device_ids: ['dev-a', 'dev-b'], edges: [], edges_created: 0, skipped_devices: [] }, }) mockBulkHide.mockResolvedValue({ data: { hidden: 2, skipped: 0 } }) mockRestore.mockResolvedValue({ data: { restored: true, device_id: 'dev-a' } }) @@ -235,6 +242,60 @@ describe('PendingDevicesModal', () => { expect(screen.getByTestId('approval-modal')).toBeInTheDocument() }) + const DUP_409 = { + response: { + status: 409, + data: { + detail: { + duplicate: true, + existing_node_id: 'n-existing', + existing_label: 'Existing Srv', + match: 'ip', + value: '192.168.1.10', + }, + }, + }, + } + + it('single approve prompts instead of failing when the host is already on the design', async () => { + mockApprove.mockRejectedValueOnce(DUP_409) + render() + await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) + fireEvent.click(screen.getByTestId('pending-card-dev-a')) + fireEvent.click(screen.getByTestId('do-approve')) + // The duplicate dialog appears (not a silent failure). + await waitFor(() => expect(screen.getByText('Device already on this canvas')).toBeInTheDocument()) + expect(screen.getByText('Existing Srv')).toBeInTheDocument() + // Regression: the device-detail modal must close so it doesn't trap focus + // and hide the prompt (two stacked Base UI dialogs). + expect(screen.queryByTestId('approval-modal')).not.toBeInTheDocument() + }) + + it('"Add duplicate anyway" retries the approve with force=true', async () => { + mockApprove.mockRejectedValueOnce(DUP_409) + render() + await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) + fireEvent.click(screen.getByTestId('pending-card-dev-a')) + fireEvent.click(screen.getByTestId('do-approve')) + await waitFor(() => expect(screen.getByText('Device already on this canvas')).toBeInTheDocument()) + fireEvent.click(screen.getByRole('button', { name: /Add duplicate anyway/ })) + await waitFor(() => expect(mockApprove).toHaveBeenCalledTimes(2)) + expect(mockApprove).toHaveBeenLastCalledWith('dev-a', expect.objectContaining({ force: true })) + }) + + it('"Go to existing node" selects the existing node and closes the modal', async () => { + mockApprove.mockRejectedValueOnce(DUP_409) + const onClose = vi.fn() + render() + await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) + fireEvent.click(screen.getByTestId('pending-card-dev-a')) + fireEvent.click(screen.getByTestId('do-approve')) + await waitFor(() => expect(screen.getByText('Device already on this canvas')).toBeInTheDocument()) + fireEvent.click(screen.getByRole('button', { name: /Go to existing node/ })) + expect(mockSetSelectedNode).toHaveBeenCalledWith('n-existing') + expect(onClose).toHaveBeenCalled() + }) + it('toggles selection in select mode instead of opening approval', async () => { render() await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) @@ -263,6 +324,29 @@ describe('PendingDevicesModal', () => { await waitFor(() => expect(mockBulkApprove).toHaveBeenCalledWith(['dev-a', 'dev-b'], null)) }) + it('bulk approve reports devices skipped as duplicates', async () => { + const { toast } = await import('sonner') + mockBulkApprove.mockResolvedValue({ + data: { + approved: 1, node_ids: ['n1'], device_ids: ['dev-b'], edges: [], edges_created: 0, + skipped: 1, + skipped_devices: [{ device_id: 'dev-a', label: 'host-a', match: 'ip', value: '192.168.1.10', existing_node_id: 'n-existing' }], + }, + }) + render() + await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) + fireEvent.click(screen.getByRole('button', { name: 'Select mode' })) + fireEvent.click(screen.getByTestId('pending-card-dev-a')) + fireEvent.click(screen.getByTestId('pending-card-dev-b')) + fireEvent.click(screen.getByRole('button', { name: /Approve \(2\)/ })) + await waitFor(() => + expect(toast.info).toHaveBeenCalledWith( + expect.stringContaining('1 already on this canvas'), + expect.anything(), + ), + ) + }) + it('keeps approved devices listed after bulk approve (reloads, not strips)', async () => { // After approve, pending() still returns the rows (now on-canvas w/ badge). mockPending