From cd6402a6800c8a9d02da1457721b88d5bc87dc62 Mon Sep 17 00:00:00 2001 From: Pouzor Date: Sat, 8 Aug 2026 12:42:40 +0200 Subject: [PATCH] fix(rack): keep a rack height edit from orphaning its mounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The number input's min/max are hints the browser does not enforce on a typed value, and updateRack wrote the patch straight through. Two ways out of a usable canvas: - Shrinking below a mounted device left the plate drawn above the chassis, outside the React Flow node, with nothing in the UI able to drag it back. freeUnits only counts 1..uHeight, so the modal's "used of" label under-counted it in silence. - A height over 100 made RackSave reject every save with a 422, so both the explicit Save and each autosave tick reported "Save failed" with no cause. updateRack now clamps to [MIN_RACK_U, MAX_RACK_U] and relocates the mounts a shrink pushes past the top rail, one at a time so two never land on the same slot. It returns false — changing nothing — when one has nowhere to go, and the modal says so. Two server-side guards that would have contained it: RackSaveRequest cross-checks every mount against the rack it names, and RackDeviceSave checks col_start + col_span against the grid, which each field passing its own bounds never caught. Also normalizes the MAC on POST /scan/pending. Every other write path canonicalizes it and dedup compares by equality, so a hand-typed AA-BB-CC-11-22-33 never matched the scanned aa:bb:cc:11:22:33 and approve built a duplicate node. ha-relevant: maybe --- backend/app/api/routes/scan.py | 6 +- backend/app/schemas/racks.py | 39 +++++++++- backend/tests/scan/test_scan_routes.py | 29 +++++++ backend/tests/test_racks.py | 46 +++++++++++ frontend/src/rack/README.md | 1 + frontend/src/rack/__tests__/store.test.ts | 78 +++++++++++++++++++ .../src/rack/components/RackSettingsModal.tsx | 14 +++- frontend/src/rack/rackDefaults.ts | 8 ++ frontend/src/rack/store.ts | 61 +++++++++++++-- 9 files changed, 272 insertions(+), 10 deletions(-) diff --git a/backend/app/api/routes/scan.py b/backend/app/api/routes/scan.py index faadefb..56a91a5 100644 --- a/backend/app/api/routes/scan.py +++ b/backend/app/api/routes/scan.py @@ -15,6 +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 PendingDeviceCreate, PendingDeviceResponse, ScanRunResponse +from app.services.mac_utils import normalize_mac 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 ( @@ -335,7 +336,10 @@ async def create_pending( device = PendingDevice( hostname=body.hostname, ip=body.ip, - mac=body.mac, + # Canonical form, like every other write path: dedup compares MACs by + # equality, so a hand-typed "AA-BB-CC-11-22-33" would never match the + # scanned "aa:bb:cc:11:22:33" and approve would build a duplicate node. + mac=normalize_mac(body.mac), suggested_type=body.suggested_type, model=body.model, vendor=body.vendor, diff --git a/backend/app/schemas/racks.py b/backend/app/schemas/racks.py index ff1de24..ca6bb40 100644 --- a/backend/app/schemas/racks.py +++ b/backend/app/schemas/racks.py @@ -1,6 +1,6 @@ from typing import Any -from pydantic import BaseModel, field_validator +from pydantic import BaseModel, field_validator, model_validator # Kept in sync with the frontend rack model (frontend/src/types). WIDTH_STANDARDS = {"19", "10"} @@ -79,6 +79,20 @@ class RackDeviceSave(BaseModel): raise ValueError(f"col_span must be between 1 and {RACK_COLUMNS}") return v + @model_validator(mode="after") + def _fits_the_column_grid(self) -> "RackDeviceSave": + """`col_start` and `col_span` are legal apart and still illegal together. + + 11 + 12 spans to column 23 of a 12-column grid — each field passes its + own check, and the frontend serializer clamps them independently, so + nothing caught it before this. + """ + if self.col_start + self.col_span > RACK_COLUMNS: + raise ValueError( + f"col_start + col_span must not exceed the {RACK_COLUMNS}-column grid" + ) + return self + class RackCableSave(BaseModel): id: str @@ -123,6 +137,29 @@ class RackSaveRequest(BaseModel): # Pan/zoom, stored on the design's shared CanvasState row like the logical canvas. viewport: dict[str, Any] = {} + @model_validator(mode="after") + def _devices_fit_their_rack(self) -> "RackSaveRequest": + """A mount must fit inside the rack it names. + + `RackDeviceSave` validates each field in isolation and never sees the + rack, so `u_start: 40` in a 12U rack passed. A mount above the top rail + draws outside the chassis and nothing in the UI can drag it back, so the + server refuses to store it. Rack membership itself is checked in the + route, which knows what is already persisted. + """ + heights = {r.id: r.u_height for r in self.racks} + for device in self.devices: + u_height = heights.get(device.rack_id) + if u_height is None: + continue # unknown rack — the route reports that with a 400 + if device.u_start + device.u_height - 1 > u_height: + raise ValueError( + f"Device {device.id} does not fit in rack {device.rack_id}: " + f"U {device.u_start}–{device.u_start + device.u_height - 1} " + f"of {u_height}U" + ) + return self + class RackResponse(BaseModel): id: str diff --git a/backend/tests/scan/test_scan_routes.py b/backend/tests/scan/test_scan_routes.py index 401dcfb..012b1b6 100644 --- a/backend/tests/scan/test_scan_routes.py +++ b/backend/tests/scan/test_scan_routes.py @@ -260,3 +260,32 @@ async def test_update_scan_config_persists_deep_scan(client: AsyncClient, header ) assert res.status_code == 200 assert saved == {"http_ranges": ["9000-9100"], "probe": True} + + +@pytest.mark.asyncio +async def test_create_pending_normalizes_the_mac(client: AsyncClient, headers): + # Dedup compares MACs by equality, so a hand-typed entry in any other + # notation would never match the scanned row and approve would build a + # duplicate node. + res = await client.post( + "/api/v1/scan/pending", + json={ + "hostname": "printer", + "mac": "AA-BB-CC-11-22-33", + "discovery_source": "manual", + }, + headers=headers, + ) + assert res.status_code == 201, res.text + assert res.json()["mac"] == "aa:bb:cc:11:22:33" + + +@pytest.mark.asyncio +async def test_create_pending_keeps_a_missing_mac_null(client: AsyncClient, headers): + res = await client.post( + "/api/v1/scan/pending", + json={"hostname": "no-mac", "discovery_source": "manual"}, + headers=headers, + ) + assert res.status_code == 201, res.text + assert res.json()["mac"] is None diff --git a/backend/tests/test_racks.py b/backend/tests/test_racks.py index 8fe57ed..1b0e1e0 100644 --- a/backend/tests/test_racks.py +++ b/backend/tests/test_racks.py @@ -226,6 +226,52 @@ class TestSaveAndLoad: res = await client.post("/api/v1/racks/save", json=payload, headers=headers) assert res.status_code == 422 + async def test_rejects_a_device_taller_than_its_rack(self, client: AsyncClient, headers): + # Each field is legal on its own; only the rack says otherwise. A mount + # above the top rail draws outside the chassis with no way to drag it back. + design_id = await _design(client, headers) + payload = _state(design_id) + payload["devices"][0]["u_start"] = 40 # rack is 12U + res = await client.post("/api/v1/racks/save", json=payload, headers=headers) + assert res.status_code == 422 + + async def test_rejects_a_device_whose_height_overruns_the_rack( + self, client: AsyncClient, headers + ): + design_id = await _design(client, headers) + payload = _state(design_id) + payload["devices"][0].update({"u_start": 11, "u_height": 4}) # 11..14 of 12U + res = await client.post("/api/v1/racks/save", json=payload, headers=headers) + assert res.status_code == 422 + + async def test_accepts_a_device_that_ends_on_the_top_rail( + self, client: AsyncClient, headers + ): + design_id = await _design(client, headers) + payload = _state(design_id) + payload["devices"][0].update({"u_start": 11, "u_height": 2}) # 11..12 of 12U + res = await client.post("/api/v1/racks/save", json=payload, headers=headers) + assert res.status_code == 200, res.text + + async def test_rejects_a_span_that_overruns_the_column_grid( + self, client: AsyncClient, headers + ): + design_id = await _design(client, headers) + payload = _state(design_id) + payload["devices"][0].update({"col_start": 11, "col_span": 12}) + res = await client.post("/api/v1/racks/save", json=payload, headers=headers) + assert res.status_code == 422 + + async def test_accepts_a_half_width_pair_sharing_one_u( + self, client: AsyncClient, headers + ): + design_id = await _design(client, headers) + payload = _state(design_id) + payload["devices"][0].update({"col_start": 0, "col_span": 6}) + payload["devices"][1].update({"col_start": 6, "col_span": 6}) + res = await client.post("/api/v1/racks/save", json=payload, headers=headers) + assert res.status_code == 200, res.text + async def test_rejects_an_unknown_cable_type(self, client: AsyncClient, headers): design_id = await _design(client, headers) payload = _state(design_id) diff --git a/frontend/src/rack/README.md b/frontend/src/rack/README.md index 890411e..bfbeb4d 100644 --- a/frontend/src/rack/README.md +++ b/frontend/src/rack/README.md @@ -55,6 +55,7 @@ types in `@/types/rack`, narrowing every enum on the way in. - Double-click a plate → the same modal in edit mode: label, faceplate, U/height/column/width, status, colour, port list, Unmount. Single click only selects. - Double-click empty rack chrome → `RackSettingsModal` (name, location, U height, 19"/10", numbering direction, frame/rail/interior colours, U numbers, enclosed, delete). - Growing a device — by hand or by picking a taller plate — relocates it to the nearest slot that takes the new size. Only a rack with no such slot rejects the edit, and says so. +- Shrinking a **rack** follows the same rule: `updateRack` clamps `uHeight` to `[MIN_RACK_U, MAX_RACK_U]` (1–48; the backend tolerates 100) and relocates every mount the new height would push above the top rail, one at a time so two never land on the same slot. It returns false and changes nothing when one has nowhere to go. The number input's `min`/`max` are hints the browser does not enforce on typed input — a raw 999 used to make every save fail the backend's `1..100` check with no cause shown, and a shrink left plates drawn outside the chassis that nothing could drag back. - **Patch mode**: drag from port A to port B to cable them — a dashed rubber band follows the pointer, exactly like dragging an edge on the logical canvas. Clicking A then B still works; Escape drops a half-drawn patch. Rack dragging is disabled while in patch mode. - **Click a cable** — in patch mode or out of it — to select it: it gets an accent halo and opens `RackCablePanel` on the right (endpoints, type, colour, label, properties, Unplug). Delete/Backspace unplugs the selection; Escape or a pane click deselects. Selecting a cable drops any mount/rack selection and vice versa — one rail, one occupant. - **Import links**: reads the physical edges (ethernet/fibre/vlan/cluster) of every non-rack design and matches them on `nodeId`. Idempotent — a device pair already cabled is skipped, whichever ports carry it, so a re-run after racking more gear adds only what is missing. There is no "done" flag: one lived in memory only, and a reload re-armed the import onto the next free ports. diff --git a/frontend/src/rack/__tests__/store.test.ts b/frontend/src/rack/__tests__/store.test.ts index 1742b48..d04d4b3 100644 --- a/frontend/src/rack/__tests__/store.test.ts +++ b/frontend/src/rack/__tests__/store.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach } from 'vitest' import { useRackStore } from '../store' import { getFaceplate } from '../faceplates' import { RACK_COLUMNS } from '@/types' +import { MAX_RACK_U, MIN_RACK_U } from '../rackDefaults' import { demoNetworkLinks } from '../demoData' const store = () => useRackStore.getState() @@ -36,6 +37,83 @@ describe('racks', () => { expect(style.showNumbers).toBe(false) expect(style.frame).toBeTruthy() }) + + /** An empty rack, so a clamp test is not also a relocation test. */ + const emptyRack = () => { + const id = store().addRack() + return { id, height: () => store().racks.find((r) => r.id === id)!.uHeight } + } + + it('clamps the height to the supported range', () => { + // The number input's min/max do not survive a typed value, and the backend + // rejects anything over 100 U with a 422 the user cannot read. + const rack = emptyRack() + + expect(store().updateRack(rack.id, { uHeight: 999 })).toBe(true) + expect(rack.height()).toBe(MAX_RACK_U) + + store().updateRack(rack.id, { uHeight: 0 }) + expect(rack.height()).toBe(MIN_RACK_U) + + store().updateRack(rack.id, { uHeight: -5 }) + expect(rack.height()).toBe(MIN_RACK_U) + }) + + it('rounds a fractional height', () => { + const rack = emptyRack() + store().updateRack(rack.id, { uHeight: 12.6 }) + expect(rack.height()).toBe(13) + }) + + it('relocates the mounts a shrink would push above the top rail', () => { + const rack = store().racks[0] + const tallest = store().devices + .filter((d) => d.rackId === rack.id) + .reduce((max, d) => (d.uStart + d.uHeight - 1 > max.uStart + max.uHeight - 1 ? d : max)) + const next = tallest.uStart - 1 + const mountedBefore = store().devices.filter((d) => d.rackId === rack.id).length + + expect(store().updateRack(rack.id, { uHeight: next })).toBe(true) + expect(store().racks[0].uHeight).toBe(next) + // Every mount is inside the chassis, and none was dropped on the way. + const after = store().devices.filter((d) => d.rackId === rack.id) + expect(after).toHaveLength(mountedBefore) + for (const d of after) expect(d.uStart + d.uHeight - 1).toBeLessThanOrEqual(next) + }) + + it('never stacks two relocated mounts on the same slot', () => { + const rack = store().racks[0] + store().updateRack(rack.id, { uHeight: 12 }) + const mounted = store().devices.filter((d) => d.rackId === rack.id) + for (const a of mounted) { + for (const b of mounted) { + if (a.id === b.id) continue + const uOverlap = a.uStart < b.uStart + b.uHeight && b.uStart < a.uStart + a.uHeight + const colOverlap = + a.colStart < b.colStart + b.colSpan && b.colStart < a.colStart + a.colSpan + expect(uOverlap && colOverlap).toBe(false) + } + } + }) + + it('refuses a shrink that leaves a mount nowhere to go, changing nothing', () => { + const rack = store().racks[0] + const before = store().devices.map((d) => ({ ...d })) + + expect(store().updateRack(rack.id, { uHeight: 1 })).toBe(false) + expect(store().racks[0].uHeight).toBe(rack.uHeight) + expect(store().devices).toEqual(before) + }) + + it('leaves the mounts alone when the rack grows', () => { + const before = store().devices.map((d) => ({ ...d })) + expect(store().updateRack('rack-main', { uHeight: MAX_RACK_U })).toBe(true) + expect(store().devices).toEqual(before) + }) + + it('returns false for a rack that does not exist', () => { + expect(store().updateRack('nope', { name: 'Ghost' })).toBe(false) + }) }) describe('mounting', () => { diff --git a/frontend/src/rack/components/RackSettingsModal.tsx b/frontend/src/rack/components/RackSettingsModal.tsx index a6fbf94..33685b0 100644 --- a/frontend/src/rack/components/RackSettingsModal.tsx +++ b/frontend/src/rack/components/RackSettingsModal.tsx @@ -4,11 +4,13 @@ * Same reasoning as `RackDeviceModal`: the rack canvas has no right rail, so the * settings live in a dialog. Opened by double-clicking a rack's chassis. */ +import { toast } from 'sonner' import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/dialog' import { Button } from '@/components/ui/button' import { Label } from '@/components/ui/label' import { useRackStore } from '../store' import { freeUnits } from '../layout' +import { MAX_RACK_U, MIN_RACK_U } from '../rackDefaults' import type { RackNumbering, RackWidthStandard } from '@/types' const inputClass = @@ -62,12 +64,18 @@ export function RackSettingsModal() { updateRack(rack.id, { uHeight: Number(e.target.value) || 1 })} + onChange={(e) => { + // The store clamps and relocates; it only refuses when a mount + // the shrink pushes out has nowhere left to go. + if (!updateRack(rack.id, { uHeight: Number(e.target.value) || MIN_RACK_U })) { + toast.error('Not enough room to shrink the rack — unmount something first') + } + }} /> diff --git a/frontend/src/rack/rackDefaults.ts b/frontend/src/rack/rackDefaults.ts index 371bdf3..642eb83 100644 --- a/frontend/src/rack/rackDefaults.ts +++ b/frontend/src/rack/rackDefaults.ts @@ -9,6 +9,14 @@ export const DEFAULT_RACK_STYLE: RackStyle = { enclosed: false, } +/** + * Capacity a rack may be set to. The backend accepts up to 100 U; the UI stops + * at 48 — taller than any cabinet a homelab owns, and the number input's + * `min`/`max` are only hints, so the store clamps to these for real. + */ +export const MIN_RACK_U = 1 +export const MAX_RACK_U = 48 + export const CABLE_COLORS: Record = { ethernet: '#39d353', fiber: '#f0a500', diff --git a/frontend/src/rack/store.ts b/frontend/src/rack/store.ts index 2abec0f..f5501a4 100644 --- a/frontend/src/rack/store.ts +++ b/frontend/src/rack/store.ts @@ -21,7 +21,7 @@ import { toRackDevice, } from '@/utils/rackSerializer' import { getFaceplate, suggestFaceplate } from './faceplates' -import { canPlace, findSlot, type Placement } from './layout' +import { canPlace, clamp, findSlot, type Placement } from './layout' import { RACK_COLUMNS, type Cable, @@ -34,7 +34,13 @@ import { type RackStyle, } from '@/types' import { demoCables, demoDevices, demoInventory, demoRacks } from './demoData' -import { CABLE_COLORS, DEFAULT_RACK_STYLE, PORT_CABLE_TYPE } from './rackDefaults' +import { + CABLE_COLORS, + DEFAULT_RACK_STYLE, + MAX_RACK_U, + MIN_RACK_U, + PORT_CABLE_TYPE, +} from './rackDefaults' export { CABLE_COLORS, DEFAULT_RACK_STYLE } @@ -130,7 +136,12 @@ interface RackState { // Racks addRack: (partial?: Partial) => string - updateRack: (id: string, patch: Partial>) => void + /** + * Edit a rack. `uHeight` is clamped to `[MIN_RACK_U, MAX_RACK_U]`, and a + * shrink relocates the mounts it would push above the top rail. Returns false + * — leaving the rack untouched — when one of them has nowhere left to go. + */ + updateRack: (id: string, patch: Partial>) => boolean updateRackStyle: (id: string, patch: Partial) => void moveRack: (id: string, position: { x: number; y: number }) => void /** Removes the rack and every device mounted in it (inventory untouched). */ @@ -388,8 +399,48 @@ export const useRackStore = create((set, get) => { return id }, - updateRack: (id, patch) => - edit((s) => ({ racks: s.racks.map((r) => (r.id === id ? { ...r, ...patch } : r)) })), + updateRack: (id, patch) => { + const { racks, devices } = get() + const rack = racks.find((r) => r.id === id) + if (!rack) return false + + const next: Rack = { ...rack, ...patch } + if (patch.uHeight !== undefined) { + // The modal's min/max are hints the browser does not enforce on typed + // input, and the height reached the store raw: 999 made every save fail + // the backend's 1..100 check with no cause shown, and a shrink left + // mounts drawn above the chassis with no way to drag them back. + next.uHeight = clamp(Math.round(patch.uHeight) || MIN_RACK_U, MIN_RACK_U, MAX_RACK_U) + } + + let relocated = devices + if (next.uHeight < rack.uHeight) { + const mounted = devices.filter((d) => d.rackId === id) + const pushedOut = mounted.filter((d) => d.uStart + d.uHeight - 1 > next.uHeight) + if (pushedOut.length > 0) { + const moved = new Map() + // Place them one at a time against the layout decided so far, so two + // relocated mounts cannot be handed the same slot. + let settled = mounted.filter((d) => !pushedOut.includes(d)) + for (const device of pushedOut) { + const slot = findSlot(next, settled, { ...device, uStart: next.uHeight }, device.id) + // Nowhere left to put it: refuse the whole edit rather than drop a + // mount off the rack, the same way a device resize refuses. + if (!slot) return false + const placed = { ...device, ...slot } + settled = [...settled, placed] + moved.set(device.id, placed) + } + relocated = devices.map((d) => moved.get(d.id) ?? d) + } + } + + edit(() => ({ + racks: racks.map((r) => (r.id === id ? next : r)), + devices: relocated, + })) + return true + }, updateRackStyle: (id, patch) => edit((s) => ({