feat: prompt on duplicate device instead of silently blocking/merging approve

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
This commit is contained in:
Pouzor
2026-07-09 13:05:19 +02:00
parent 8ebc716de8
commit 0792105b96
9 changed files with 629 additions and 85 deletions
+12
View File
@@ -6,6 +6,7 @@ from app.api.deps import get_current_user
from app.db.database import get_db from app.db.database import get_db
from app.db.models import Design, Node from app.db.models import Design, Node
from app.schemas.nodes import NodeCreate, NodeResponse, NodeUpdate from app.schemas.nodes import NodeCreate, NodeResponse, NodeUpdate
from app.services.node_dedupe import find_duplicate_node
router = APIRouter() 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) @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: async def create_node(body: NodeCreate, db: AsyncSession = Depends(get_db), _: str = Depends(get_current_user)) -> Node:
data = body.model_dump() 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 # 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 # 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 # 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: if data.get("design_id") is None:
first_design = (await db.execute(select(Design).order_by(Design.created_at).limit(1))).scalar() 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 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) node = Node(**data)
db.add(node) db.add(node)
await db.commit() await db.commit()
+61 -45
View File
@@ -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.db.models import Design, Edge, Node, PendingDevice, PendingDeviceLink, ScanRun
from app.schemas.nodes import NodeCreate from app.schemas.nodes import NodeCreate
from app.schemas.scan import PendingDeviceResponse, ScanRunResponse 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.scanner import DeepScanOptions, _valid_port_range, request_cancel, run_scan
from app.services.zigbee_service import ( from app.services.zigbee_service import (
build_zigbee_properties, build_zigbee_properties,
@@ -335,23 +335,37 @@ async def bulk_approve_devices(
devices = result.scalars().all() devices = result.scalars().all()
# What already sits on the target canvas, so we skip devices already placed # 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 = ( existing = (
await db.execute( 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() ).all()
placed_ips = {ip for ip, _ in existing if ip} placed_ips: dict[str, Any] = {ip: nid for nid, ip, _ in existing if ip}
placed_ieee = {ieee for _, ieee in existing if ieee} placed_ieee: dict[str, Any] = {ieee: nid for nid, _, ieee in existing if ieee}
created_nodes: list[Node] = [] created_nodes: list[Node] = []
approved_devices: list[PendingDevice] = [] approved_devices: list[PendingDevice] = []
skipped_devices: list[dict[str, Any]] = []
for device in devices: for device in devices:
already_here = ( # Record which identifier collided so the caller can explain each skip
(device.ip is not None and device.ip in placed_ips) # (and, for existing on-canvas nodes, link to the node already there).
or (device.ieee_address is not None and device.ieee_address in placed_ieee) if device.ip is not None and device.ip in placed_ips:
) skipped_devices.append({
if already_here: "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 continue
device.status = "approved" device.status = "approved"
node_type = device.suggested_type or "generic" node_type = device.suggested_type or "generic"
@@ -381,16 +395,23 @@ async def bulk_approve_devices(
created_nodes.append(node) created_nodes.append(node)
approved_devices.append(device) approved_devices.append(device)
# Track within this batch so a duplicate selection (same ip/ieee) is not # 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: if device.ip:
placed_ips.add(device.ip) placed_ips[device.ip] = node
if device.ieee_address: 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 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 and approved_device_ids stay index-aligned for the client's mapping.
node_ids = [n.id for n in created_nodes] node_ids = [n.id for n in created_nodes]
approved_device_ids = [d.id for d in approved_devices] 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]] = [] all_edges: list[dict[str, str]] = []
for device in approved_devices: for device in approved_devices:
all_edges.extend( all_edges.extend(
@@ -405,6 +426,7 @@ async def bulk_approve_devices(
"edges_created": len(all_edges), "edges_created": len(all_edges),
"edges": all_edges, "edges": all_edges,
"skipped": len(payload.device_ids) - len(node_ids), "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) device = await db.get(PendingDevice, device_id)
if not device: if not device:
raise HTTPException(status_code=404, detail="Device not found") raise HTTPException(status_code=404, detail="Device not found")
if device.status != "pending": # A device's status is GLOBAL — it flips to "approved" the moment it lands on
raise HTTPException(status_code=409, detail="Device already processed") # ANY canvas — but canvas membership is per-design. Approving onto a NEW
device.status = "approved" # 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) wireless = _is_wireless(node_data.type)
# Guard against a true duplicate: same IEEE already placed on THIS design. # A device already on THIS design (matched by ieee, ip OR mac) is NOT placed
# (The same device on a *different* design is valid — one Node per canvas.) # again automatically: the user might genuinely want a second card, or might
if device.ieee_address is not None: # be re-approving by mistake. Reject with 409 + the existing node so the UI
dup = ( # can ask — identical handling for IEEE (Zigbee/Z-Wave) and plain IP/ARP
await db.execute( # hosts. force=True (set after the user confirms) skips this and creates it.
select(Node).where( # The same device on a *different* design is valid (one Node per canvas), so
Node.ieee_address == device.ieee_address, # this is scoped to node_design_id.
Node.design_id == node_design_id, if not node_data.force:
) conflict = await find_duplicate_node(
) db, node_design_id,
).scalars().first() node_data.ip or device.ip,
if dup is not None: node_data.mac or device.mac,
dup.properties = merge_zigbee_properties( ieee=device.ieee_address,
dup.properties, )
_wireless_properties( if conflict is not None:
node_data.type, device.ieee_address, raise HTTPException(status_code=409, detail=conflict)
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,
}
device.status = "approved"
# Prefer the MAC discovered during the scan (stored on the pending device); # Prefer the MAC discovered during the scan (stored on the pending device);
# fall back to whatever the approve payload carried. # fall back to whatever the approve payload carried.
_mac = device.mac or node_data.mac _mac = device.mac or node_data.mac
+4
View File
@@ -39,6 +39,10 @@ class NodeBase(BaseModel):
class NodeCreate(NodeBase): class NodeCreate(NodeBase):
design_id: str | None = None 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): class NodeUpdate(BaseModel):
+55 -1
View File
@@ -20,7 +20,7 @@ from __future__ import annotations
import logging import logging
from typing import Any from typing import Any
from sqlalchemy import select from sqlalchemy import or_, select
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from app.db.models import Edge, Node from app.db.models import Edge, Node
@@ -28,6 +28,60 @@ from app.services.zigbee_service import merge_zigbee_properties
logger = logging.getLogger(__name__) 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 # 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 # 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). # the canonical node untouched (that's the row the user actually placed).
+41
View File
@@ -99,6 +99,47 @@ async def test_create_node_respects_explicit_design_id(client: AsyncClient, head
assert res.json()["design_id"] == second_id 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): 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 # No designs exist yet: fallback can't invent one, so design_id stays null
# rather than erroring. # rather than erroring.
+216 -6
View File
@@ -312,11 +312,11 @@ async def test_approve_device(client: AsyncClient, headers, pending_device):
@pytest.mark.asyncio @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 client: AsyncClient, headers, db_session
): ):
"""Approving a device whose IEEE is already on the target design must reuse """Approving a device whose IEEE is already on the target design prompts the
the existing node, not create a duplicate (source of the crash bug).""" user (409) instead of silently merging/replacing — same UX as ip/mac."""
design = Design(name="d1") design = Design(name="d1")
db_session.add(design) db_session.add(design)
await db_session.flush() await db_session.flush()
@@ -340,13 +340,195 @@ async def test_approve_device_no_dupe_same_ieee_same_design(
}, },
headers=headers, headers=headers,
) )
assert res.status_code == 200 assert res.status_code == 409
assert res.json()["node_id"] == existing.id # reused, not new 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 = ( nodes = (
await db_session.execute(select(Node).where(Node.ieee_address == "0xZZZ")) await db_session.execute(select(Node).where(Node.ieee_address == "0xZZZ"))
).scalars().all() ).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 @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"] 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 @pytest.fixture
async def zigbee_pending_device(db_session): async def zigbee_pending_device(db_session):
device = PendingDevice( device = PendingDevice(
+21
View File
@@ -77,6 +77,26 @@ export interface DeepScanConfig {
export type ScanConfigData = { ranges: string[] } & 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 = { export const scanApi = {
trigger: (deepScan?: Partial<DeepScanConfig>) => api.post('/scan/trigger', deepScan ?? {}), trigger: (deepScan?: Partial<DeepScanConfig>) => api.post('/scan/trigger', deepScan ?? {}),
pending: () => api.get('/scan/pending'), pending: () => api.get('/scan/pending'),
@@ -100,6 +120,7 @@ export const scanApi = {
edges_created: number edges_created: number
edges: { id: string; source: string; target: string; type?: string; source_handle?: string | null; target_handle?: string | null }[] edges: { id: string; source: string; target: string; type?: string; source_handle?: string | null; target_handle?: string | null }[]
skipped: number skipped: number
skipped_devices: SkippedDevice[]
}>('/scan/pending/bulk-approve', { device_ids: ids, design_id: designId ?? undefined }), }>('/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 }), 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`), restore: (id: string) => api.post<{ restored: boolean; device_id: string }>(`/scan/pending/${id}/restore`),
@@ -4,7 +4,7 @@ import {
Search, RefreshCw, X, CheckCircle2, EyeOff, Trash2, Loader2, ServerCog, Search, RefreshCw, X, CheckCircle2, EyeOff, Trash2, Loader2, ServerCog,
} from 'lucide-react' } from 'lucide-react'
import { Dialog, DialogClose, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog' 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 { useCanvasStore } from '@/stores/canvasStore'
import { useDesignStore } from '@/stores/designStore' import { useDesignStore } from '@/stores/designStore'
import { useThemeStore } from '@/stores/themeStore' 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' 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) { function injectAutoEdges(edges: AutoEdge[] | undefined) {
if (!edges || edges.length === 0) return if (!edges || edges.length === 0) return
useCanvasStore.setState((state) => ({ 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. // Optionally restrict to devices that have at least one detected service.
const [withServicesOnly, setWithServicesOnly] = useState(false) const [withServicesOnly, setWithServicesOnly] = useState(false)
const { addNode, scanEventTs } = useCanvasStore() const { addNode, scanEventTs } = useCanvasStore()
const setSelectedNode = useCanvasStore((s) => s.setSelectedNode)
const activeDesignId = useDesignStore((s) => s.activeDesignId) const activeDesignId = useDesignStore((s) => s.activeDesignId)
const highlightRef = useRef<HTMLButtonElement>(null) const highlightRef = useRef<HTMLButtonElement>(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 () => { const load = useCallback(async () => {
setLoading(true) 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 { try {
const fallbackLabel = deviceLabel(device) const res = await scanApi.approve(device.id, { ...nodeData, force })
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 nodeId = res.data.node_id const nodeId = res.data.node_id
addNode({ addNode({
id: nodeId, 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 // 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. // for a fresh canvas_count rather than dropping it until reopen.
setSelected(null) setSelected(null)
setDupPrompt(null)
await load() 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') 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) => { const handleHide = async (device: PendingDevice) => {
try { try {
await scanApi.hide(device.id) await scanApi.hide(device.id)
@@ -362,6 +400,17 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus
await load() await load()
const linkExtra = res.data.edges_created > 0 ? ` (+${res.data.edges_created} link${res.data.edges_created !== 1 ? 's' : ''})` : '' 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}`) 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 { } catch {
toast.error('Failed to bulk approve devices') toast.error('Failed to bulk approve devices')
} }
@@ -629,6 +678,59 @@ export function PendingDevicesModal({ open, onClose, highlightId, initialStatus
)} )}
</div> </div>
)} )}
{/* 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 && (
<div
role="dialog"
aria-modal="true"
aria-label="Device already on this canvas"
className="fixed inset-0 z-50 flex items-center justify-center bg-black/60 p-4"
onClick={() => setDupPrompt(null)}
>
<div
className="w-full max-w-md rounded-lg border border-border bg-[#161b22] p-5 shadow-xl"
onClick={(e) => e.stopPropagation()}
>
<h2 className="text-base font-semibold text-foreground mb-3">Device already on this canvas</h2>
<div className="text-sm text-muted-foreground space-y-3">
<p>
<span className="text-foreground font-medium break-all">{deviceLabel(dupPrompt.device)}</span>{' '}
matches a node already on this design
{' '}(<span className="uppercase text-[11px] font-mono">{dupPrompt.conflict.match}</span>{' '}
<span className="font-mono text-foreground">{dupPrompt.conflict.value}</span>):{' '}
<span className="text-foreground font-medium break-all">{dupPrompt.conflict.existing_label}</span>.
</p>
<p>Add it again anyway, or jump to the existing node?</p>
<div className="flex flex-wrap justify-end gap-2 pt-1">
<button
onClick={() => setDupPrompt(null)}
className="text-xs px-3 py-1.5 rounded border border-border text-muted-foreground hover:text-foreground transition-colors"
>
Cancel
</button>
<button
onClick={() => goToExistingNode(dupPrompt.conflict.existing_node_id)}
className="text-xs px-3 py-1.5 rounded bg-[#00d4ff]/20 text-[#00d4ff] hover:bg-[#00d4ff]/30 font-medium transition-colors"
>
Go to existing node
</button>
<button
onClick={() => approveDevice(dupPrompt.device, true)}
className="text-xs px-3 py-1.5 rounded bg-[#e3b341]/20 text-[#e3b341] hover:bg-[#e3b341]/30 font-medium transition-colors"
>
Add duplicate anyway
</button>
</div>
</div>
</div>
</div>
)}
</DialogContent> </DialogContent>
</Dialog> </Dialog>
@@ -14,6 +14,7 @@ const mockHide = vi.fn()
const mockPending = vi.fn() const mockPending = vi.fn()
const mockHidden = vi.fn() const mockHidden = vi.fn()
const mockAddNode = vi.fn() const mockAddNode = vi.fn()
const mockSetSelectedNode = vi.fn()
vi.mock('@/api/client', () => ({ vi.mock('@/api/client', () => ({
scanApi: { 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', () => ({ vi.mock('@/components/modals/PendingDeviceModal', () => ({
PendingDeviceModal: ({ device }: { device: unknown }) => PendingDeviceModal: ({ device, onApprove }: { device: unknown; onApprove: (d: unknown) => void }) =>
device ? <div data-testid="approval-modal" /> : null, device ? (
<div data-testid="approval-modal">
<button data-testid="do-approve" onClick={() => onApprove(device)}>approve</button>
</div>
) : null,
})) }))
const DEVICE_IP = { const DEVICE_IP = {
@@ -104,10 +109,12 @@ const DEVICE_PROXMOX = {
beforeEach(() => { beforeEach(() => {
vi.clearAllMocks() vi.clearAllMocks()
vi.mocked(useCanvasStore).mockReturnValue({ // Apply the selector when one is passed (setSelectedNode is read via a
addNode: mockAddNode, // selector), else return the whole store (destructured in the component).
scanEventTs: 0, vi.mocked(useCanvasStore).mockImplementation(((sel?: (s: unknown) => unknown) => {
} as unknown as ReturnType<typeof useCanvasStore>) const store = { addNode: mockAddNode, scanEventTs: 0, setSelectedNode: mockSetSelectedNode }
return sel ? sel(store) : store
}) as unknown as typeof useCanvasStore)
// setState is used by injectAutoEdges // setState is used by injectAutoEdges
;(useCanvasStore as unknown as { setState: (fn: unknown) => void }).setState = vi.fn() ;(useCanvasStore as unknown as { setState: (fn: unknown) => void }).setState = vi.fn()
mockPending.mockResolvedValue({ data: [DEVICE_IP, DEVICE_ZIGBEE] }) mockPending.mockResolvedValue({ data: [DEVICE_IP, DEVICE_ZIGBEE] })
@@ -115,7 +122,7 @@ beforeEach(() => {
mockApprove.mockResolvedValue({ data: { node_id: 'n1', edges: [], edges_created: 0 } }) mockApprove.mockResolvedValue({ data: { node_id: 'n1', edges: [], edges_created: 0 } })
mockHide.mockResolvedValue({ data: {} }) mockHide.mockResolvedValue({ data: {} })
mockBulkApprove.mockResolvedValue({ 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 } }) mockBulkHide.mockResolvedValue({ data: { hidden: 2, skipped: 0 } })
mockRestore.mockResolvedValue({ data: { restored: true, device_id: 'dev-a' } }) mockRestore.mockResolvedValue({ data: { restored: true, device_id: 'dev-a' } })
@@ -235,6 +242,60 @@ describe('PendingDevicesModal', () => {
expect(screen.getByTestId('approval-modal')).toBeInTheDocument() 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(<PendingDevicesModal {...baseProps} />)
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(<PendingDevicesModal {...baseProps} />)
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(<PendingDevicesModal {...baseProps} onClose={onClose} />)
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 () => { it('toggles selection in select mode instead of opening approval', async () => {
render(<PendingDevicesModal {...baseProps} />) render(<PendingDevicesModal {...baseProps} />)
await waitFor(() => expect(screen.getByTestId('pending-card-dev-a')).toBeInTheDocument()) 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)) 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(<PendingDevicesModal {...baseProps} />)
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 () => { it('keeps approved devices listed after bulk approve (reloads, not strips)', async () => {
// After approve, pending() still returns the rows (now on-canvas w/ badge). // After approve, pending() still returns the rows (now on-canvas w/ badge).
mockPending mockPending