From c797efe1c93324ecb3bb0c2b756a39cf1050120f Mon Sep 17 00:00:00 2001 From: flesher Date: Wed, 29 Jul 2026 15:05:50 -0700 Subject: [PATCH 01/12] feat(sites): create buildings inline from the Manage Site modal (#832) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting up a facility required leaving the Manage Site flow to create a building: save an empty site, close the modal, go to the Buildings page, create the building, come back, then associate it. Create the site up front instead of deferring it ------------------------------------------------ "Continue" on the site-details modal now persists the site via CreateSite and opens ManageSiteModal in edit mode against the new row. Inline building-create needs a real site_id to attach to, and the seeded bulk flow in FleetCreateFlowProvider already used exactly this create-then-openManageEdit shape. Because the manage surface is now always backed by a persisted site, the deferred-create machinery is gone: the manageCreate and manageCreateEditingDetails states, SiteSettingsModal's createReturn mode, cancelAll, and the create branch of manageSave. Editing details after Continue reuses the existing UpdateSite path. The seeded CreateSite path (#821) is untouched — the bulk flow still passes its building/rack/device seed to the transactional RPC. Inline building-create folded into the building picker ----------------------------------------------------- ManageBuildingsModal grows a "New building" button beside Continue, mirroring ParentPickerModal's createNewLaunch affordance ("New rack"). It swaps the picker for BuildingSettingsModal with the Site dropdown locked to the current site; on save the building is created against that site via the transactional CreateBuilding RPC (#821). The picker confirms its pending selection on the way out, so staged checkbox changes survive the swap — the delta only edits the caller's in-memory working set, so applying it early is lossless. The created building is injected into the working set and the load-time snapshot rather than triggering a refetch, so buildings staged in the picker aren't dropped. Since CreateBuilding already associated it, the snapshot injection means Save won't redundantly re-assign it while a later Remove still unassigns correctly. Co-Authored-By: Claude Opus 4.8 --- .../ManageBuildingsModal.test.tsx | 94 ++++++++ .../ManageBuildingsModal.tsx | 38 ++- .../ManageSiteModal/ManageSiteModal.test.tsx | 162 ++++++------- .../ManageSiteModal/ManageSiteModal.tsx | 131 +++++++---- .../sites/components/ManageSiteModal/index.ts | 1 - .../components/SiteModals/SiteModals.test.tsx | 18 +- .../components/SiteModals/SiteModals.tsx | 53 ++--- .../SiteSettingsModal.test.tsx | 56 ----- .../SiteSettingsModal/SiteSettingsModal.tsx | 23 +- .../sites/hooks/useSiteModals.test.ts | 154 +++++-------- .../features/sites/hooks/useSiteModals.ts | 217 +++++++++--------- 11 files changed, 490 insertions(+), 457 deletions(-) create mode 100644 client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.test.tsx diff --git a/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.test.tsx b/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.test.tsx new file mode 100644 index 0000000000..0ad1bea787 --- /dev/null +++ b/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.test.tsx @@ -0,0 +1,94 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { create } from "@bufbuild/protobuf"; + +import ManageBuildingsModal from "./ManageBuildingsModal"; +import { BuildingSchema, BuildingWithCountsSchema } from "@/protoFleet/api/generated/buildings/v1/buildings_pb"; +import { SiteSchema, SiteWithCountsSchema } from "@/protoFleet/api/generated/sites/v1/sites_pb"; + +const { listAllBuildingsMock, listSitesMock } = vi.hoisted(() => ({ + listAllBuildingsMock: vi.fn(), + listSitesMock: vi.fn(), +})); + +vi.mock("@/protoFleet/api/buildings", () => ({ + useBuildings: () => ({ listAllBuildings: listAllBuildingsMock }), +})); + +vi.mock("@/protoFleet/api/sites", () => ({ + useSites: () => ({ listSites: listSitesMock }), +})); + +// The picker self-fetches the org-wide building list plus the site catalog for +// its Site column; both mocks resolve synchronously through onSuccess. +const seed = (rows: { id: bigint; name: string; siteId: bigint }[]) => { + listAllBuildingsMock.mockImplementation((args?: { onSuccess?: (rows: unknown[]) => void }) => { + args?.onSuccess?.( + rows.map((r) => + create(BuildingWithCountsSchema, { + building: create(BuildingSchema, { id: r.id, name: r.name, siteId: r.siteId }), + rackCount: 0n, + }), + ), + ); + return Promise.resolve(undefined); + }); + listSitesMock.mockImplementation((args?: { onSuccess?: (rows: unknown[]) => void }) => { + args?.onSuccess?.([create(SiteWithCountsSchema, { site: create(SiteSchema, { id: 7n, name: "East DC" }) })]); + return Promise.resolve(undefined); + }); +}; + +const baseProps = { + open: true as const, + siteId: 7n, + initialSelectedBuildingIds: [] as bigint[], + onDismiss: () => undefined, +}; + +describe("ManageBuildingsModal — New building hand-off", () => { + beforeEach(() => { + listAllBuildingsMock.mockReset(); + listSitesMock.mockReset(); + }); + + it("omits the New building button when no launch handler is supplied", async () => { + seed([{ id: 1n, name: "Building A", siteId: 0n }]); + render(); + + await waitFor(() => expect(screen.getByTestId("manage-buildings-modal-confirm")).toBeInTheDocument()); + expect(screen.queryByTestId("manage-buildings-modal-create-new")).not.toBeInTheDocument(); + }); + + it("confirms the pending selection before launching create, so checkbox changes survive the swap", async () => { + seed([{ id: 1n, name: "Building A", siteId: 0n }]); + const onConfirm = vi.fn(); + const onCreateNewLaunch = vi.fn(); + render(); + + await waitFor(() => expect(screen.getByTestId("manage-buildings-modal-create-new")).toBeEnabled()); + + // Check the unassigned building, then hand off to the create flow without + // pressing Continue. + const checkbox = screen.getAllByRole("checkbox")[0]; + fireEvent.click(checkbox); + fireEvent.click(screen.getByTestId("manage-buildings-modal-create-new")); + + // The staged selection is applied on the way out, then create is launched. + expect(onConfirm).toHaveBeenCalledWith({ + added: [{ buildingId: 1n, label: "Building A" }], + removed: [], + }); + expect(onCreateNewLaunch).toHaveBeenCalled(); + }); + + it("disables the New building button until the building list resolves", () => { + // No onSuccess → items stays undefined, so handleConfirm would no-op and + // leave both modals open. + listAllBuildingsMock.mockReturnValue(Promise.resolve(undefined)); + listSitesMock.mockReturnValue(Promise.resolve(undefined)); + render(); + + expect(screen.getByTestId("manage-buildings-modal-create-new")).toBeDisabled(); + }); +}); diff --git a/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.tsx b/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.tsx index f49fb4469a..28754088b6 100644 --- a/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.tsx +++ b/client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.tsx @@ -4,7 +4,7 @@ import { buildBuildingPickerItem, type BuildingPickerItem } from "./buildingPick import { computeBuildingSelectionDelta } from "./buildingSelectionDelta"; import { useBuildings } from "@/protoFleet/api/buildings"; import { useSites } from "@/protoFleet/api/sites"; -import { ChevronDown } from "@/shared/assets/icons"; +import { ChevronDown, Plus } from "@/shared/assets/icons"; import Button, { sizes, variants } from "@/shared/components/Button"; import List from "@/shared/components/List"; import type { ColConfig, ColTitles } from "@/shared/components/List/types"; @@ -27,6 +27,13 @@ interface ManageBuildingsModalProps { // separate lookup); `removed` is the seeded ids the operator unchecked. // Untouched buildings are in neither list — the caller leaves them as-is. onConfirm: (delta: { added: { buildingId: bigint; label: string }[]; removed: bigint[] }) => void; + // Renders a "New building" button beside Continue that hands off to the + // full building-create flow instead of picking an existing building — + // mirroring ParentPickerModal's createNewLaunch affordance ("New rack"). + // The current selection is confirmed on the way out (see + // handleCreateNewLaunch), so checkbox changes aren't lost in the swap. + // Omitted = no create affordance. + onCreateNewLaunch?: () => void; } const PAGE_SIZE = 25; @@ -60,6 +67,7 @@ const ManageBuildingsModal = ({ initialSelectedBuildingIds, onDismiss, onConfirm, + onCreateNewLaunch, }: ManageBuildingsModalProps) => { const { listAllBuildings } = useBuildings(); const { listSites } = useSites(); @@ -152,6 +160,18 @@ const ManageBuildingsModal = ({ const handleSelectNone = useCallback(() => setSelectedItems([]), []); + // "New building" hand-off. Confirm the current selection first so the swap + // to the create modal doesn't silently drop checkbox changes — the delta + // only edits the caller's in-memory working set (nothing is persisted until + // the Manage Site modal's Save), so applying it early is lossless. The + // caller's onConfirm closes this picker, and onCreateNewLaunch opens the + // create modal in its place. + const handleCreateNewLaunch = useCallback(() => { + if (!onCreateNewLaunch) return; + handleConfirm(); + onCreateNewLaunch(); + }, [handleConfirm, onCreateNewLaunch]); + return ( , + onClick: handleCreateNewLaunch, + disabled: items === undefined, + dismissModalOnClick: false, + testId: "manage-buildings-modal-create-new", + }, + ] + : []), { text: "Continue", variant: "primary", diff --git a/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.test.tsx b/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.test.tsx index 341dfa9750..9526665137 100644 --- a/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.test.tsx +++ b/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.test.tsx @@ -11,7 +11,10 @@ import { emptySiteFormValues, type SiteFormValues } from "@/protoFleet/api/sites // caller's onSuccess synchronously with whatever rows the test queued. const { listBuildingsBySiteMock } = vi.hoisted(() => ({ listBuildingsBySiteMock: vi.fn() })); -vi.mock("@/protoFleet/api/buildings", () => ({ +// Keep the module's real helpers (emptyBuildingFormValues is used by both this +// modal and the BuildingSettingsModal it renders) and override only the hook. +vi.mock("@/protoFleet/api/buildings", async (importActual) => ({ + ...(await importActual()), useBuildings: () => ({ listBuildingsBySite: listBuildingsBySiteMock, listAllBuildings: vi.fn(), @@ -19,6 +22,17 @@ vi.mock("@/protoFleet/api/buildings", () => ({ }), })); +// Stub the building picker: the real one self-fetches the org-wide building +// list and site catalog, which this suite doesn't wire up. All we need from it +// here is the "New building" hand-off that reaches the inline create flow. +vi.mock("../ManageBuildingsModal", () => ({ + default: ({ onCreateNewLaunch }: { onCreateNewLaunch?: () => void }) => ( + + ), +})); + const seedBuildings = (rows: { id: bigint; name: string; siteId: bigint; rackCount: bigint }[]) => { listBuildingsBySiteMock.mockImplementation((args?: { onSuccess?: (rows: unknown[]) => void }) => { args?.onSuccess?.( @@ -39,26 +53,31 @@ const draft = (overrides: Partial = {}): SiteFormValues => ({ ...overrides, }); +const site7 = create(SiteSchema, { id: 7n, name: "East DC" }); + const noop = () => undefined; +// Common props — the site is always persisted by the time the modal opens. +const baseProps = { + open: true as const, + site: site7, + draft: draft({ name: "East DC" }), + onSave: () => Promise.resolve(null), + onCreateBuilding: vi.fn().mockResolvedValue(null), + onEditDetails: noop, + onDeleteRequested: noop, + onDismiss: noop, +}; + describe("ManageSiteModal", () => { beforeEach(() => listBuildingsBySiteMock.mockReset()); it("invokes onSave and closes when the save reports closeOnSuccess", async () => { + seedBuildings([]); const onSave = vi.fn().mockResolvedValue({ closeOnSuccess: true }); const onDismiss = vi.fn(); - render( - , - ); + render(); fireEvent.click(screen.getByTestId("manage-site-modal-save")); @@ -66,86 +85,74 @@ describe("ManageSiteModal", () => { await waitFor(() => expect(onDismiss).toHaveBeenCalled()); }); - it("disables Save in edit mode until the building list has loaded", () => { + it("disables Save until the building list has loaded", () => { // No seed → listBuildingsBySite never calls onSuccess, so the working // set stays in the loading (undefined) state. - const site = create(SiteSchema, { id: 7n, name: "East DC" }); - render( - , - ); + render(); expect(screen.getByTestId("manage-site-modal-save")).toBeDisabled(); }); it("Site settings fires the parent callback", () => { + seedBuildings([]); const onEditDetails = vi.fn(); - render( - Promise.resolve(null)} - onEditDetails={onEditDetails} - onDeleteRequested={noop} - onDismiss={noop} - />, - ); + render(); fireEvent.click(screen.getAllByTestId("manage-site-modal-edit-details")[0]); expect(onEditDetails).toHaveBeenCalled(); }); it("Delete site fires onDeleteRequested", () => { + seedBuildings([]); const onDeleteRequested = vi.fn(); - render( - Promise.resolve(null)} - onEditDetails={noop} - onDeleteRequested={onDeleteRequested} - onDismiss={noop} - />, - ); + render(); fireEvent.click(screen.getAllByTestId("manage-site-modal-delete")[0]); expect(onDeleteRequested).toHaveBeenCalled(); }); - it("create mode lets buildings be staged before the site is saved", () => { - render( - Promise.resolve(null)} - onEditDetails={noop} - onDeleteRequested={noop} - onDismiss={noop} - />, - ); + it("creates a building inline via the picker hand-off and injects it into the working set", async () => { + seedBuildings([]); + const created = create(BuildingSchema, { id: 5n, name: "New Bldg", siteId: 7n }); + const onCreateBuilding = vi.fn().mockResolvedValue(created); + render(); + + // Create is reached through the picker's "New building" hand-off, not a + // dedicated button on this modal. + expect(screen.getByText("No buildings added to this site")).toBeInTheDocument(); + expect(screen.queryByTestId("manage-site-modal-create-building")).not.toBeInTheDocument(); + fireEvent.click(screen.getAllByTestId("manage-site-modal-manage-buildings")[0]); + fireEvent.click(screen.getByTestId("stub-picker-create-new")); + expect(screen.getByTestId("building-settings-modal")).toBeInTheDocument(); + + // Name the building and save. + fireEvent.change(screen.getByTestId("building-settings-name-input"), { target: { value: "New Bldg" } }); + fireEvent.click(screen.getByTestId("building-settings-modal-save")); + + await waitFor(() => expect(onCreateBuilding).toHaveBeenCalled()); + // The created building is injected and the create modal closes. + await waitFor(() => expect(screen.getByTestId("manage-site-modal-building-row-5")).toBeInTheDocument()); + expect(screen.queryByTestId("building-settings-modal")).not.toBeInTheDocument(); + }); + + it("keeps the create modal open and injects nothing when create fails", async () => { + seedBuildings([]); + const onCreateBuilding = vi.fn().mockResolvedValue(null); + render(); - // No "save first" gate — the empty working set renders the same - // add-buildings affordance as edit mode, and Manage buildings is enabled. - expect(screen.queryByText("Save the site first to add buildings.")).not.toBeInTheDocument(); + fireEvent.click(screen.getAllByTestId("manage-site-modal-manage-buildings")[0]); + fireEvent.click(screen.getByTestId("stub-picker-create-new")); + fireEvent.change(screen.getByTestId("building-settings-name-input"), { target: { value: "Nope" } }); + fireEvent.click(screen.getByTestId("building-settings-modal-save")); + + await waitFor(() => expect(onCreateBuilding).toHaveBeenCalled()); + // Modal stays open; no row was added. + expect(screen.getByTestId("building-settings-modal")).toBeInTheDocument(); expect(screen.getByText("No buildings added to this site")).toBeInTheDocument(); - expect(screen.getAllByTestId("manage-site-modal-manage-buildings")[0]).toBeEnabled(); - expect(screen.getAllByTestId("manage-site-modal-empty-state-add")[0]).toBeEnabled(); - // Save is allowed immediately (creates the site with an empty building set). - expect(screen.getByTestId("manage-site-modal-save")).toBeEnabled(); }); it("shows comma-separated meta on each corner of the preview", () => { + seedBuildings([]); const site = create(SiteSchema, { id: 7n, name: "East DC", @@ -155,8 +162,7 @@ describe("ManageSiteModal", () => { }); render( { locationState: "MA", powerCapacityMw: 5, })} - onSave={() => Promise.resolve(null)} - onEditDetails={noop} - onDeleteRequested={noop} - onDismiss={noop} />, ); expect(screen.getByText("East DC, Boston, MA")).toBeInTheDocument(); @@ -176,19 +178,7 @@ describe("ManageSiteModal", () => { it("renders rack count as a subtitle and kebab-removes a building from the working set", () => { seedBuildings([{ id: 1n, name: "Building A", siteId: 7n, rackCount: 3n }]); - const site = create(SiteSchema, { id: 7n, name: "East DC" }); - render( - Promise.resolve(null)} - onEditDetails={noop} - onDeleteRequested={noop} - onDismiss={noop} - />, - ); + render(); // Rack count renders as the row subtitle (not a trailing column). expect(screen.getByTestId("manage-site-modal-building-row-1")).toBeInTheDocument(); diff --git a/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.tsx b/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.tsx index bb2b956f13..b49b267055 100644 --- a/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.tsx +++ b/client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.tsx @@ -1,11 +1,13 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { create } from "@bufbuild/protobuf"; import ManageBuildingsModal from "../ManageBuildingsModal"; -import { useBuildings } from "@/protoFleet/api/buildings"; -import { type BuildingWithCounts } from "@/protoFleet/api/generated/buildings/v1/buildings_pb"; -import { type Site } from "@/protoFleet/api/generated/sites/v1/sites_pb"; +import { type BuildingFormValues, emptyBuildingFormValues, useBuildings } from "@/protoFleet/api/buildings"; +import { type Building, type BuildingWithCounts } from "@/protoFleet/api/generated/buildings/v1/buildings_pb"; +import { type Site, SiteWithCountsSchema } from "@/protoFleet/api/generated/sites/v1/sites_pb"; import { type SiteFormValues } from "@/protoFleet/api/sites"; import FullScreenTwoPaneModal from "@/protoFleet/components/FullScreenTwoPaneModal"; +import BuildingSettingsModal from "@/protoFleet/features/buildings/components/BuildingSettingsModal"; import { formatSiteAddress } from "@/protoFleet/features/sites/formatAddress"; import { Ellipsis } from "@/shared/assets/icons"; import Button, { sizes, variants } from "@/shared/components/Button"; @@ -13,8 +15,6 @@ import Header from "@/shared/components/Header"; import PlaceholderBlock from "@/shared/components/PlaceholderBlock"; import { useEscapeDismiss } from "@/shared/hooks/useEscapeDismiss"; -export type ManageSiteModalMode = "create" | "edit"; - // One building in the modal's working set. Seeded from the server fetch and // mutated locally by the Manage buildings picker; persisted on Save. interface BuildingEntry { @@ -32,17 +32,18 @@ export interface BuildingMembershipDelta { interface ManageSiteModalProps { open: boolean; - mode: ManageSiteModalMode; draft: SiteFormValues; - // In edit mode the parent has a Site row to drive the right-pane preview - // header off; in create mode there is no row yet so the preview uses the - // draft values directly. - site?: Site; - // Persisted at save time. In edit mode the delta is applied via - // AssignBuildingsToSite; in create mode the host first creates the site - // (the delta is empty since building management is gated until the site - // exists). Returns whether the modal should close on success. + // The site is always persisted by the time this modal opens (the create + // flow's Continue creates it up front), so it drives the right-pane preview + // header and the site_id for building writes. + site: Site; + // Persisted at save time: the building-membership delta is applied via + // AssignBuildingsToSite. Returns whether the modal should close on success. onSave: (delta: BuildingMembershipDelta) => Promise<{ closeOnSuccess: boolean } | null>; + // Creates a new building against this site and returns the created row (or + // null on failure). The building is associated to the site atomically, so + // the modal injects the returned row into its working set without a refetch. + onCreateBuilding: (values: BuildingFormValues) => Promise; // Opens SiteSettingsModal stacked on top to edit name / address / etc. onEditDetails: () => void; // Opens the cascade delete dialog (edit) or discards the pending create. @@ -146,10 +147,10 @@ const BuildingRow = ({ const ManageSiteModal = ({ open, - mode, draft, site, onSave, + onCreateBuilding, onEditDetails, onDeleteRequested, onDismiss, @@ -163,18 +164,16 @@ const ManageSiteModal = ({ // via the picker before Save. const [entries, setEntries] = useState(undefined); const [showManageBuildings, setShowManageBuildings] = useState(false); + const [showCreateBuilding, setShowCreateBuilding] = useState(false); // Snapshot of the building ids present at load time so Save can diff the // working set into add / remove buckets for AssignBuildingsToSite. const initialIdsRef = useRef>(new Set()); - // Only fetch when edit mode has a persisted site; create mode renders an - // empty working set until the first Save lands a row. Skipping the effect - // for the no-fetch branches keeps the setState-in-effect lint clean. - const shouldFetchBuildings = open && mode === "edit" && site !== undefined; - const fetchSiteId = shouldFetchBuildings ? site.id : undefined; + const shouldFetchBuildings = open; + const fetchSiteId = site.id; useEffect(() => { - if (!shouldFetchBuildings || fetchSiteId === undefined) return; + if (!shouldFetchBuildings) return; const controller = new AbortController(); void listBuildingsBySite({ siteId: fetchSiteId, @@ -198,25 +197,21 @@ const ManageSiteModal = ({ return () => controller.abort(); }, [shouldFetchBuildings, fetchSiteId, listBuildingsBySite, buildingsRefreshKey]); - // Create mode never fetches (there's no persisted site yet) but still keeps - // a working set: buildings the operator stages via the picker before the - // first Save, which the host then assigns to the freshly-created site. - // `entries` starts undefined there, so fall back to an empty (loaded, not - // loading) list rather than the edit-mode loading skeleton. - const displayEntries: BuildingEntry[] | undefined = useMemo( - () => (shouldFetchBuildings ? entries : (entries ?? [])), - [shouldFetchBuildings, entries], - ); const sortedEntries = useMemo( - () => (displayEntries ? [...displayEntries].sort((a, b) => a.label.localeCompare(b.label)) : undefined), - [displayEntries], + () => (entries ? [...entries].sort((a, b) => a.label.localeCompare(b.label)) : undefined), + [entries], ); - const previewTitle = (site?.name || draft.name || "Untitled site").trim(); + const previewTitle = (site.name || draft.name || "Untitled site").trim(); const previewLocation = useMemo(() => formatSiteAddress(draft) || "—", [draft]); const previewCapacity = draft.powerCapacityMw > 0 ? `${draft.powerCapacityMw} MW` : "—"; const buildingCount = sortedEntries?.length ?? 0; - const currentBuildingIds = useMemo(() => (displayEntries ?? []).map((e) => e.buildingId), [displayEntries]); + const currentBuildingIds = useMemo(() => (entries ?? []).map((e) => e.buildingId), [entries]); + // The inline building-create dropdown is always locked to this site, so a + // one-element list built from `site` is all BuildingSettingsModal needs — and + // it sidesteps the brief window right after create-on-Continue where the + // page's site catalog hasn't refetched the new row yet. + const buildingCreateSites = useMemo(() => [create(SiteWithCountsSchema, { site })], [site]); // Picker confirm — apply the delta against the working set. `added` joins // entries without disturbing existing rows; `removed` drops only those @@ -248,9 +243,27 @@ const ManageSiteModal = ({ setEntries((prev) => (prev ?? []).filter((e) => e.buildingId !== buildingId)); }, []); + // Inline building-create confirm. CreateBuilding already associated the new + // building to this site, so inject it into both the working set (to show it) + // and the load-time snapshot (so Save's diff treats it as a pre-existing + // member — it won't be re-assigned, and a later Remove still unassigns it). + // A failed create returns null (toast shown by the host) and leaves the + // create modal open. + const handleCreateBuildingSave = async (values: BuildingFormValues) => { + const created = await onCreateBuilding(values); + if (!created) return; + setEntries((prev) => { + const next = prev ?? []; + if (next.some((e) => e.buildingId === created.id)) return next; + return [...next, { buildingId: created.id, label: created.name, rackCount: 0n }]; + }); + initialIdsRef.current.add(created.id.toString()); + setShowCreateBuilding(false); + }; + const handleSave = async () => { const initial = initialIdsRef.current; - const current = new Set((displayEntries ?? []).map((e) => e.buildingId.toString())); + const current = new Set((entries ?? []).map((e) => e.buildingId.toString())); const added = [...current].filter((id) => !initial.has(id)).map((id) => BigInt(id)); const removed = [...initial].filter((id) => !current.has(id)).map((id) => BigInt(id)); const result = await onSave({ added, removed }); @@ -286,9 +299,8 @@ const ManageSiteModal = ({ text: "Manage buildings", variant: variants.secondary, onClick: () => setShowManageBuildings(true), - // Enabled in both modes — create stages buildings into the working - // set and assigns them on the first Save. Only blocked while the - // edit-mode list is still loading (sortedEntries undefined). + // Only blocked while the building list is still loading + // (sortedEntries undefined). disabled: saving || sortedEntries === undefined, testId: "manage-site-modal-manage-buildings", }, @@ -296,10 +308,10 @@ const ManageSiteModal = ({ text: saving ? "Saving…" : "Save", variant: variants.primary, onClick: handleSave, - // Block Save until the edit-mode building list has loaded. - // handleSave diffs the working set against initialIdsRef; firing - // it while entries are still undefined would diff against an empty - // (or stale) working set and unassign buildings on save. + // Block Save until the building list has loaded. handleSave diffs + // the working set against initialIdsRef; firing it while entries + // are still undefined would diff against an empty (or stale) + // working set and unassign buildings on save. disabled: buildingsBusy, testId: "manage-site-modal-save", }, @@ -316,6 +328,8 @@ const ManageSiteModal = ({ ) : sortedEntries.length === 0 ? (
No buildings added to this site + {/* Single affordance — the picker itself carries the + "New building" hand-off for creating one from scratch. */}
) : (
- {errorMsg ? } title={errorMsg} /> : null} - Date: Wed, 29 Jul 2026 17:20:09 -0700 Subject: [PATCH 07/12] fix(racks): let a slot entry with no position mean "unplace" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of slot_assignments said "empty list = leave slots alone, non-empty list = authoritative for the whole selector, so a selector device with no entry is cleared". That reads fine until the operator pulls every miner they touched off the grid: the resulting list is empty, empty means "leave alone", and the unplace silently doesn't happen. The one case the rule couldn't express. Make presence per-entry instead, which is what RackPlacement does with its optional aisle_index / position_in_aisle: an entry with a position places, an entry without one clears, and a device the batch never names is untouched. Empty still writes nothing, so pre-existing callers are unaffected. This also decouples the two lists — slot_assignments no longer has to mirror device_selector to avoid clobbering — and drops the walk-the-selector clear in favour of clearing exactly the named devices. Co-Authored-By: Claude --- .../generated/device_set/v1/device_set_pb.ts | 32 ++++---- client/src/protoFleet/api/useDeviceSets.ts | 10 +-- proto/device_set/v1/device_set.proto | 28 +++---- .../grpc/device_set/v1/device_set.pb.go | 28 +++---- server/internal/domain/collection/service.go | 64 +++++++++------- .../domain/collection/service_test.go | 74 +++++++++++++++---- .../handlers/deviceset/handler_test.go | 11 +-- 7 files changed, 150 insertions(+), 97 deletions(-) diff --git a/client/src/protoFleet/api/generated/device_set/v1/device_set_pb.ts b/client/src/protoFleet/api/generated/device_set/v1/device_set_pb.ts index fa293c312a..70a7cdf97a 100644 --- a/client/src/protoFleet/api/generated/device_set/v1/device_set_pb.ts +++ b/client/src/protoFleet/api/generated/device_set/v1/device_set_pb.ts @@ -1705,22 +1705,22 @@ export type AssignDevicesToRackRequest = Message<"device_set.v1.AssignDevicesToR * position_in_aisle: the batch names only the devices it is changing, * and each named device ends up either placed or explicitly unplaced. * - * Empty (the default): slot positions are left untouched. A device - * already in the target rack keeps its slot; a device arriving from - * another rack starts unplaced, because its old slot died with its - * old membership. - * - * Non-empty: authoritative for every device in - * device_selector.device_list. A device with an entry here is placed - * at that position; a device in the selector with NO entry has its - * slot cleared. That is what lets one call mix placed and unplaced - * miners — and what lets a pure relayout swap two occupied slots, - * since the whole batch is cleared before any position is set. - * - * Every entry must name a device present in device_selector, and no - * two entries may share a device or a position. Positions are bounds - * checked against the target rack's rows/columns. Requires - * target_rack_id: an unassign has no rack to place into. + * Per entry: `position` set places the device there, `position` unset + * CLEARS its slot. A device not named here is untouched, so one call + * can mix placed, unplaced and left-alone miners. Every named device + * is cleared before any position is written, which is what lets a + * relayout swap two occupied cells without tripping + * uk_rack_slot_position mid-batch. + * + * Empty (the default): no slot is written at all. A device already in + * the target rack keeps its slot; a device arriving from another rack + * starts unplaced, because its old slot died with its old membership. + * + * Every entry must name a device present in device_selector — a slot + * needs a membership row to hang off — and no two entries may share a + * device or a position. Positions are bounds checked against the + * target rack's rows/columns. Requires target_rack_id: an unassign has + * no rack to place into. * * @generated from field: repeated device_set.v1.RackSlot slot_assignments = 4; */ diff --git a/client/src/protoFleet/api/useDeviceSets.ts b/client/src/protoFleet/api/useDeviceSets.ts index f69aaa00f6..033633cdd2 100644 --- a/client/src/protoFleet/api/useDeviceSets.ts +++ b/client/src/protoFleet/api/useDeviceSets.ts @@ -196,11 +196,11 @@ interface AssignDevicesToRackProps { // miners' site. Default false: the server returns conflicts (surfaced // via onConflicts) and writes nothing. forceClearConflictingSite?: boolean; - // Optional slot placements for the same miners, applied in the same - // transaction. Omit to leave placement alone (a miner already in the - // rack keeps its slot). When supplied it is authoritative for every id - // in deviceIdentifiers: a miner with an entry lands there, a miner - // without one is unplaced. Every entry must name a miner in + // Optional slot placements, applied in the same transaction. One entry + // per miner whose placement changes: `position` set lands it there, + // `position` omitted clears its slot (in the rack, off the grid). A + // miner not named here keeps whatever slot it had, which is what makes + // this safe where saveRack was not. Every entry must name a miner in // deviceIdentifiers; no two may share a miner or a cell. slotAssignments?: RackSlot[]; signal?: AbortSignal; diff --git a/proto/device_set/v1/device_set.proto b/proto/device_set/v1/device_set.proto index ac74c18d28..fb4eeea389 100644 --- a/proto/device_set/v1/device_set.proto +++ b/proto/device_set/v1/device_set.proto @@ -744,22 +744,22 @@ message AssignDevicesToRackRequest { // position_in_aisle: the batch names only the devices it is changing, // and each named device ends up either placed or explicitly unplaced. // - // Empty (the default): slot positions are left untouched. A device - // already in the target rack keeps its slot; a device arriving from - // another rack starts unplaced, because its old slot died with its - // old membership. + // Per entry: `position` set places the device there, `position` unset + // CLEARS its slot. A device not named here is untouched, so one call + // can mix placed, unplaced and left-alone miners. Every named device + // is cleared before any position is written, which is what lets a + // relayout swap two occupied cells without tripping + // uk_rack_slot_position mid-batch. // - // Non-empty: authoritative for every device in - // device_selector.device_list. A device with an entry here is placed - // at that position; a device in the selector with NO entry has its - // slot cleared. That is what lets one call mix placed and unplaced - // miners — and what lets a pure relayout swap two occupied slots, - // since the whole batch is cleared before any position is set. + // Empty (the default): no slot is written at all. A device already in + // the target rack keeps its slot; a device arriving from another rack + // starts unplaced, because its old slot died with its old membership. // - // Every entry must name a device present in device_selector, and no - // two entries may share a device or a position. Positions are bounds - // checked against the target rack's rows/columns. Requires - // target_rack_id: an unassign has no rack to place into. + // Every entry must name a device present in device_selector — a slot + // needs a membership row to hang off — and no two entries may share a + // device or a position. Positions are bounds checked against the + // target rack's rows/columns. Requires target_rack_id: an unassign has + // no rack to place into. repeated RackSlot slot_assignments = 4 [(buf.validate.field).repeated.max_items = 10000]; } diff --git a/server/generated/grpc/device_set/v1/device_set.pb.go b/server/generated/grpc/device_set/v1/device_set.pb.go index 291b2a6041..dad3beb3bb 100644 --- a/server/generated/grpc/device_set/v1/device_set.pb.go +++ b/server/generated/grpc/device_set/v1/device_set.pb.go @@ -3256,22 +3256,22 @@ type AssignDevicesToRackRequest struct { // position_in_aisle: the batch names only the devices it is changing, // and each named device ends up either placed or explicitly unplaced. // - // Empty (the default): slot positions are left untouched. A device - // already in the target rack keeps its slot; a device arriving from - // another rack starts unplaced, because its old slot died with its - // old membership. + // Per entry: `position` set places the device there, `position` unset + // CLEARS its slot. A device not named here is untouched, so one call + // can mix placed, unplaced and left-alone miners. Every named device + // is cleared before any position is written, which is what lets a + // relayout swap two occupied cells without tripping + // uk_rack_slot_position mid-batch. // - // Non-empty: authoritative for every device in - // device_selector.device_list. A device with an entry here is placed - // at that position; a device in the selector with NO entry has its - // slot cleared. That is what lets one call mix placed and unplaced - // miners — and what lets a pure relayout swap two occupied slots, - // since the whole batch is cleared before any position is set. + // Empty (the default): no slot is written at all. A device already in + // the target rack keeps its slot; a device arriving from another rack + // starts unplaced, because its old slot died with its old membership. // - // Every entry must name a device present in device_selector, and no - // two entries may share a device or a position. Positions are bounds - // checked against the target rack's rows/columns. Requires - // target_rack_id: an unassign has no rack to place into. + // Every entry must name a device present in device_selector — a slot + // needs a membership row to hang off — and no two entries may share a + // device or a position. Positions are bounds checked against the + // target rack's rows/columns. Requires target_rack_id: an unassign has + // no rack to place into. SlotAssignments []*RackSlot `protobuf:"bytes,4,rep,name=slot_assignments,json=slotAssignments,proto3" json:"slot_assignments,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/server/internal/domain/collection/service.go b/server/internal/domain/collection/service.go index 6ad9f2349f..09b314e5b4 100644 --- a/server/internal/domain/collection/service.go +++ b/server/internal/domain/collection/service.go @@ -1095,18 +1095,20 @@ type AssignDevicesToRackParams struct { ForceClearConflictingSite bool // SlotAssignments optionally places the assigned devices inside the // target rack's grid, so membership and placement land in the same - // transaction. Empty leaves slots untouched. Non-empty is - // authoritative for every device in DeviceIdentifiers: a device with - // an entry is placed there, a device without one has its slot - // cleared. Requires TargetRackID. See validateAssignRackSlots. + // transaction. One entry per device whose placement changes: Position + // set places it there, Position nil clears its slot. A device not + // named here keeps whatever slot it had. Empty writes no slot at all. + // Requires TargetRackID. See validateAssignRackSlots. SlotAssignments []*pb.RackSlot } // validateAssignRackSlots enforces the SlotAssignments contract that needs -// no DB read: a target rack to place into, a position per entry, entries -// confined to the devices being assigned, and no two entries claiming the -// same device or the same cell. Grid bounds are checked separately, in-tx, -// once the rack's dimensions are known. +// no DB read: a target rack to place into, entries confined to the devices +// being assigned, and no two entries claiming the same device or the same +// cell. A nil Position is legal and means "clear this device's slot", the +// counterpart of buildings.v1.RackPlacement's unset aisle/position. Grid +// bounds are checked separately, in-tx, once the rack's dimensions are +// known. // // The duplicate-position check is the friendly form of // uk_rack_slot_position: two entries on one cell would otherwise surface @@ -1125,8 +1127,8 @@ func validateAssignRackSlots(params AssignDevicesToRackParams) error { seenDevices := make(map[string]struct{}, len(params.SlotAssignments)) seenPositions := make(map[[2]int32]struct{}, len(params.SlotAssignments)) for _, slot := range params.SlotAssignments { - if slot == nil || slot.Position == nil { - return fleeterror.NewInvalidArgumentError("slot assignment must have a position") + if slot == nil { + return fleeterror.NewInvalidArgumentError("slot assignment must not be empty") } if _, ok := assigned[slot.DeviceIdentifier]; !ok { return fleeterror.NewInvalidArgumentErrorf("slot assignment references device %q which is not in the device selector", slot.DeviceIdentifier) @@ -1135,6 +1137,10 @@ func validateAssignRackSlots(params AssignDevicesToRackParams) error { return fleeterror.NewInvalidArgumentErrorf("device %q appears in slot_assignments more than once", slot.DeviceIdentifier) } seenDevices[slot.DeviceIdentifier] = struct{}{} + // Unset position = clear the slot; nothing left to bounds check. + if slot.Position == nil { + continue + } if slot.Position.Row < 0 || slot.Position.Column < 0 { return fleeterror.NewInvalidArgumentError("slot position row and column must not be negative") } @@ -1148,10 +1154,10 @@ func validateAssignRackSlots(params AssignDevicesToRackParams) error { } // applyRackSlotDelta persists the slot half of an AssignDevicesToRack -// batch. It is scoped to deviceIdentifiers — the devices the caller named -// — so a miner nobody mentioned keeps its slot. That scoping is the whole -// point of the delta: SaveRack's replace-all shape cannot express "move -// this one miner" without re-asserting every member. +// batch. Only the devices slotAssignments names are touched, so a miner +// nobody mentioned keeps its slot. That scoping is the whole point of the +// delta: SaveRack's replace-all shape cannot express "move this one miner" +// without re-asserting every member. // // Empty slotAssignments is a no-op, which keeps every pre-existing caller // (the importer, the CLI, the overview page's assign-then-place pair) @@ -1159,23 +1165,21 @@ func validateAssignRackSlots(params AssignDevicesToRackParams) error { // its slot, and an arriving device is unplaced because its old slot died // with its old membership. // -// Otherwise every named device is cleared first, then the requested -// positions are set. The clear-then-set ordering is what lets one call -// swap two occupied cells without tripping uk_rack_slot_position -// mid-batch; a named device with no entry simply stays cleared, which is -// how the caller expresses "unplace this miner". -func (s *Service) applyRackSlotDelta(ctx context.Context, orgID, rackID int64, deviceIdentifiers []string, slotAssignments []*pb.RackSlot) error { - if len(slotAssignments) == 0 { - return nil - } - // Map order is irrelevant: every clear lands before any set, so the - // batch is order-independent by construction. - for deviceIdentifier := range uniqueIdentifiers(deviceIdentifiers) { - if err := s.collectionStore.ClearRackSlotPosition(ctx, rackID, deviceIdentifier, orgID); err != nil { +// Otherwise every named device is cleared first, then the entries carrying +// a position are set. Clearing the whole batch up front is what lets one +// call swap two occupied cells without tripping uk_rack_slot_position +// mid-batch, and it is also how an entry with no position expresses +// "unplace this miner but leave it in the rack". +func (s *Service) applyRackSlotDelta(ctx context.Context, orgID, rackID int64, slotAssignments []*pb.RackSlot) error { + for _, slot := range slotAssignments { + if err := s.collectionStore.ClearRackSlotPosition(ctx, rackID, slot.DeviceIdentifier, orgID); err != nil { return err } } for _, slot := range slotAssignments { + if slot.Position == nil { + continue + } if err := s.collectionStore.SetRackSlotPosition(ctx, rackID, slot.DeviceIdentifier, slot.Position.Row, slot.Position.Column, orgID); err != nil { return err } @@ -1335,6 +1339,10 @@ func (s *Service) AssignDevicesToRack(ctx context.Context, params AssignDevicesT if rackInfo != nil { targetRows, targetColumns = rackInfo.Rows, rackInfo.Columns for _, slot := range params.SlotAssignments { + // Unset position = clear; no cell to bounds check. + if slot.Position == nil { + continue + } if slot.Position.Row >= targetRows { return nil, fleeterror.NewInvalidArgumentErrorf("slot row %d is out of bounds (rack has %d rows)", slot.Position.Row, targetRows) } @@ -1477,7 +1485,7 @@ func (s *Service) AssignDevicesToRack(ctx context.Context, params AssignDevicesT // Placement, last: membership must exist before a slot can // reference it (both slot queries join device_set_membership). - if err := s.applyRackSlotDelta(ctx, params.OrgID, *params.TargetRackID, params.DeviceIdentifiers, params.SlotAssignments); err != nil { + if err := s.applyRackSlotDelta(ctx, params.OrgID, *params.TargetRackID, params.SlotAssignments); err != nil { return nil, err } } diff --git a/server/internal/domain/collection/service_test.go b/server/internal/domain/collection/service_test.go index c3787ecfed..a2924232ef 100644 --- a/server/internal/domain/collection/service_test.go +++ b/server/internal/domain/collection/service_test.go @@ -2935,12 +2935,12 @@ func TestService_AssignDevicesToRack_slotDeltaClearsThenSets(t *testing.T) { assert.ElementsMatch(t, []string{"set:d1@0,1", "set:d2@0,0"}, calls[2:]) } -// TestService_AssignDevicesToRack_slotDeltaUnplacesOmittedDevice pins the +// TestService_AssignDevicesToRack_slotDeltaUnplacesOnNilPosition pins the // mixed placed+unplaced case that makes this a delta rather than a -// replace: a device named in the batch with no slot entry ends up cleared, -// which is how a caller expresses "pull this miner off the grid but leave -// it in the rack". -func TestService_AssignDevicesToRack_slotDeltaUnplacesOmittedDevice(t *testing.T) { +// replace: an entry with no position clears that device's slot — the +// counterpart of RackPlacement's unset aisle/position — which is how a +// caller says "pull this miner off the grid but leave it in the rack". +func TestService_AssignDevicesToRack_slotDeltaUnplacesOnNilPosition(t *testing.T) { svc, mockStore, _ := newTestServiceWithSites(t, nil) ctx := testCtx(t) @@ -2950,14 +2950,66 @@ func TestService_AssignDevicesToRack_slotDeltaUnplacesOmittedDevice(t *testing.T mockStore.EXPECT().ClearRackSlotPosition(gomock.Any(), rackID, "d1", testOrgID).Return(nil) mockStore.EXPECT().ClearRackSlotPosition(gomock.Any(), rackID, "d2", testOrgID).Return(nil) - // Only d1 is placed; d2 stays cleared — no Set call for it. + // Only d1 carries a position; d2's nil-position entry leaves it cleared. mockStore.EXPECT().SetRackSlotPosition(gomock.Any(), rackID, "d1", int32(2), int32(3), testOrgID).Return(nil) _, err := svc.AssignDevicesToRack(ctx, AssignDevicesToRackParams{ OrgID: testOrgID, TargetRackID: &rackID, DeviceIdentifiers: deviceIDs, - SlotAssignments: []*pb.RackSlot{rackSlot("d1", 2, 3)}, + SlotAssignments: []*pb.RackSlot{rackSlot("d1", 2, 3), {DeviceIdentifier: "d2"}}, + }) + require.NoError(t, err) +} + +// TestService_AssignDevicesToRack_slotDeltaPureUnplace covers the case an +// "empty list means clear the selector" rule could not express at all: the +// operator pulls every miner they touched off the grid. Each is named with +// no position, so the batch is non-empty and the clears actually land. +func TestService_AssignDevicesToRack_slotDeltaPureUnplace(t *testing.T) { + svc, mockStore, _ := newTestServiceWithSites(t, nil) + ctx := testCtx(t) + + rackID := int64(42) + deviceIDs := []string{"d1", "d2"} + expectAssignToRackPreamble(mockStore, rackID, deviceIDs, 10, 10) + + mockStore.EXPECT().ClearRackSlotPosition(gomock.Any(), rackID, "d1", testOrgID).Return(nil) + mockStore.EXPECT().ClearRackSlotPosition(gomock.Any(), rackID, "d2", testOrgID).Return(nil) + // No SetRackSlotPosition expectation: the strict mock fails if one fires. + + _, err := svc.AssignDevicesToRack(ctx, AssignDevicesToRackParams{ + OrgID: testOrgID, + TargetRackID: &rackID, + DeviceIdentifiers: deviceIDs, + SlotAssignments: []*pb.RackSlot{{DeviceIdentifier: "d1"}, {DeviceIdentifier: "d2"}}, + }) + require.NoError(t, err) +} + +// TestService_AssignDevicesToRack_slotDeltaLeavesUnnamedDeviceAlone is the +// invariant that makes this safe where SaveRack was not: a miner the batch +// never mentions keeps its slot, even though it is a member of the same +// rack. SaveRack could only express this by re-asserting the entire member +// set from the client's (possibly stale) snapshot. +func TestService_AssignDevicesToRack_slotDeltaLeavesUnnamedDeviceAlone(t *testing.T) { + svc, mockStore, _ := newTestServiceWithSites(t, nil) + ctx := testCtx(t) + + rackID := int64(42) + // d2 is in the selector but absent from slot_assignments. + deviceIDs := []string{"d1", "d2"} + expectAssignToRackPreamble(mockStore, rackID, deviceIDs, 10, 10) + + // Only d1 is cleared and re-placed. A Clear on d2 fails the strict mock. + mockStore.EXPECT().ClearRackSlotPosition(gomock.Any(), rackID, "d1", testOrgID).Return(nil) + mockStore.EXPECT().SetRackSlotPosition(gomock.Any(), rackID, "d1", int32(0), int32(0), testOrgID).Return(nil) + + _, err := svc.AssignDevicesToRack(ctx, AssignDevicesToRackParams{ + OrgID: testOrgID, + TargetRackID: &rackID, + DeviceIdentifiers: deviceIDs, + SlotAssignments: []*pb.RackSlot{rackSlot("d1", 0, 0)}, }) require.NoError(t, err) } @@ -3024,14 +3076,6 @@ func TestService_AssignDevicesToRack_slotDeltaRejectsBadInput(t *testing.T) { SlotAssignments: []*pb.RackSlot{rackSlot("d1", 0, 0), rackSlot("d1", 1, 1)}, }, }, - { - name: "missing position", - params: AssignDevicesToRackParams{ - TargetRackID: &rackID, - DeviceIdentifiers: []string{"d1"}, - SlotAssignments: []*pb.RackSlot{{DeviceIdentifier: "d1"}}, - }, - }, { // An unassign has no rack to place into. name: "slots without a target rack", diff --git a/server/internal/handlers/deviceset/handler_test.go b/server/internal/handlers/deviceset/handler_test.go index 44222049c0..5546a783e3 100644 --- a/server/internal/handlers/deviceset/handler_test.go +++ b/server/internal/handlers/deviceset/handler_test.go @@ -991,9 +991,10 @@ func TestAssignDevicesToRack_CarriesSlotAssignments(t *testing.T) { h.collectionStore.EXPECT(). CascadeAddedDeviceBuildings(gomock.Any(), testOrgID, targetRackID, deviceIDs). Return(int64(0), nil) + // d1 is placed at (1,2). d2 is named with no position, so it is cleared + // and left off the grid — the wire form of "in the rack, unplaced". h.collectionStore.EXPECT().ClearRackSlotPosition(gomock.Any(), targetRackID, "d1", testOrgID).Return(nil) h.collectionStore.EXPECT().ClearRackSlotPosition(gomock.Any(), targetRackID, "d2", testOrgID).Return(nil) - // d1 is placed at (1,2); d2 carries no entry, so it stays unplaced. h.collectionStore.EXPECT(). SetRackSlotPosition(gomock.Any(), targetRackID, "d1", int32(1), int32(2), testOrgID). Return(nil) @@ -1001,10 +1002,10 @@ func TestAssignDevicesToRack_CarriesSlotAssignments(t *testing.T) { resp, err := h.handler.AssignDevicesToRack(testCtx(t), connect.NewRequest(&dspb.AssignDevicesToRackRequest{ TargetRackId: &targetRackID, DeviceSelector: deviceListSelector(deviceIDs...), - SlotAssignments: []*dspb.RackSlot{{ - DeviceIdentifier: "d1", - Position: &dspb.RackSlotPosition{Row: 1, Column: 2}, - }}, + SlotAssignments: []*dspb.RackSlot{ + {DeviceIdentifier: "d1", Position: &dspb.RackSlotPosition{Row: 1, Column: 2}}, + {DeviceIdentifier: "d2"}, + }, })) require.NoError(t, err) assert.Equal(t, int64(2), resp.Msg.AssignedCount) From 8a18425d86034a70f4ab994b93d2519a1831180c Mon Sep 17 00:00:00 2001 From: flesher Date: Wed, 29 Jul 2026 17:31:19 -0700 Subject: [PATCH 08/12] refactor(racks): create the rack on the Rack Settings CTA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rack Settings said "Continue" and wrote nothing; the rack came into existence at the very end, when the operator pressed Save in the manage modal. That put the create behind two modals' worth of work — dismiss before Save and the rack you'd configured never existed — and it forced ManageRackModal to carry a whole second personality: a staged-create mode with seededMinerIds, an undefined existingRackId, a settings step that persisted nothing, and a Save that had to decide between creating and updating. Move the create to the CTA that reads like one. "Create rack" creates it (with the bulk flow's seeded miners in the same atomic SaveRack, so a failed seed can't strand an empty rack), then ManageRackModal opens on a rack that exists. The editing CTA becomes "Save", disabled unless the form actually differs from what it was seeded with — the gate is computed from exactly the fields the payload carries, so it can only read clean when the write would be a no-op. Both entry points now share useCreateRack, so the toast, the double-click guard and the site-strip confirmation can't drift between them. ManageRackModal loses the staged-create branches throughout: one loading path, one settings-save path, one Save. onContinue is renamed onSubmit — it is no longer a step, it is a write. --- .../FleetCreateFlowProvider.tsx | 67 ++++- .../ManageRackModal/ManageRackModal.test.tsx | 12 +- .../ManageRackModal/ManageRackModal.tsx | 275 ++++++++---------- .../components/RackSettingsModal.stories.tsx | 4 +- .../components/RackSettingsModal.tsx | 60 +++- .../fleetManagement/hooks/useCreateRack.ts | 130 +++++++++ .../fleetManagement/pages/RacksPage.tsx | 65 ++++- 7 files changed, 408 insertions(+), 205 deletions(-) create mode 100644 client/src/protoFleet/features/fleetManagement/hooks/useCreateRack.ts diff --git a/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx b/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx index a85a054f4c..0b4960d1b6 100644 --- a/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx @@ -16,7 +16,9 @@ import BuildingModals from "@/protoFleet/features/buildings/components/BuildingM import BuildingSettingsModal from "@/protoFleet/features/buildings/components/BuildingSettingsModal"; import { useBuildingModals } from "@/protoFleet/features/buildings/hooks/useBuildingModals"; import { ManageRackModal, type RackFormData } from "@/protoFleet/features/fleetManagement/components/ManageRackModal"; +import ReparentWarningDialog from "@/protoFleet/features/fleetManagement/components/ManageRackModal/ReparentWarningDialog"; import RackSettingsModal from "@/protoFleet/features/fleetManagement/components/RackSettingsModal"; +import { useCreateRack } from "@/protoFleet/features/fleetManagement/hooks/useCreateRack"; import SiteModals from "@/protoFleet/features/sites/components/SiteModals"; import SiteSettingsModal from "@/protoFleet/features/sites/components/SiteSettingsModal"; import { useSiteModals } from "@/protoFleet/features/sites/hooks/useSiteModals"; @@ -44,9 +46,10 @@ const MAX_DEVICE_BATCH = 10000; // with the operator's current miner selection, which in all-selection mode // resolves to the full fleet (capped only at MAX_DEVICE_BATCH). Without a // gate here, an oversized seed renders one MinersPane row per miner and -// freezes the browser long before the save-time capacity guard fires. Cap -// at the absolute max before ManageRackModal mounts — the exact -// chosen-capacity check still runs at save once rows×columns are picked. +// freezes the browser long before anything else fires. Cap at the absolute +// max before the operator even reaches Rack Settings — the exact +// chosen-capacity check runs server-side on the create, once rows×columns +// are picked. const MAX_RACK_CAPACITY = 12 * 12; // A seed that moves racked miners into a new building/site sends @@ -110,12 +113,14 @@ const FleetCreateFlowProvider = ({ const activeSite = useFleetStore((state) => state.ui.activeSite); const scopedSiteId = useMemo(() => (activeSite.kind === "site" ? BigInt(activeSite.id) : undefined), [activeSite]); - // Rack create flow. rackSettings drives RackSettingsModal; once the - // operator continues, rackFormData opens ManageRackModal seeded with the - // selected miners. rackSeed survives the settings step so the miners reach - // the manage modal. + // Rack create flow. RackSettingsModal's "Create rack" creates the rack with + // the seeded miners already in it — one atomic SaveRack, so a failed seed + // can't leave an empty rack behind — then ManageRackModal opens on the real + // rack for positioning. rackSeed survives the settings step so the miners + // reach the create call. const [rackSettingsOpen, setRackSettingsOpen] = useState(false); const [rackFormData, setRackFormData] = useState(null); + const [rackId, setRackId] = useState(null); const [rackSeed, setRackSeed] = useState(null); // Holds a seed whose miners have a placement the new rack would clear, // until the operator confirms; null when no confirmation is pending. @@ -124,6 +129,7 @@ const FleetCreateFlowProvider = ({ const openRackSettings = useCallback((seed: RackCreateSeed) => { setRackSeed(seed); setRackFormData(null); + setRackId(null); setRackSettingsOpen(true); }, []); @@ -153,13 +159,35 @@ const FleetCreateFlowProvider = ({ const closeRackFlow = useCallback(() => { setRackSettingsOpen(false); setRackFormData(null); + setRackId(null); setRackSeed(null); }, []); - const handleRackSettingsContinue = useCallback((formData: RackFormData) => { - setRackSettingsOpen(false); - setRackFormData(formData); - }, []); + const { + createRack, + creating: creatingRack, + conflict: rackCreateConflict, + confirmConflict: confirmRackCreateConflict, + cancelConflict: cancelRackCreateConflict, + } = useCreateRack({ + onCreated: (createdId, formData) => { + // The rack and its seeded miners are live. Pulse the lists now — the + // operator may well dismiss the manage step without positioning anything, + // and the rack still exists. + bumpEntities(); + setRackSettingsOpen(false); + setRackSeed(null); + setRackFormData(formData); + setRackId(createdId); + }, + }); + + // The seed's miners ride on the create itself, so membership and the rack + // land in one transaction. + const handleRackSettingsSubmit = useCallback( + (formData: RackFormData) => createRack(formData, rackSeed?.minerIds), + [createRack, rackSeed], + ); const handleRackSaved = useCallback(() => { bumpEntities(); @@ -380,20 +408,29 @@ const FleetCreateFlowProvider = ({ existingRacks={[]} defaultSiteId={scopedSiteId} onDismiss={closeRackFlow} - onContinue={handleRackSettingsContinue} + onSubmit={handleRackSettingsSubmit} + saving={creatingRack} /> ) : null} - {rackFormData ? ( + {rackFormData && rackId !== null ? ( ) : null} + {rackCreateConflict ? ( + + ) : null} {rackConflictSeed ? ( void }) => onSuccess([])); +const mockListGroupMembers = vi.fn(({ onSuccess }: { onSuccess: (ids: string[]) => void }) => onSuccess(["miner-1"])); const mockBlinkLED = vi.fn(); const miners: Record = { @@ -33,6 +37,8 @@ vi.mock("@/protoFleet/components/PageHeader/SitePicker", async (importActual) => vi.mock("@/protoFleet/api/useDeviceSets", () => ({ useDeviceSets: () => ({ saveRack: mockSaveRack, + updateRack: mockUpdateRack, + getDeviceSet: mockGetDeviceSet, getRackSlots: mockGetRackSlots, listGroupMembers: mockListGroupMembers, }), @@ -77,8 +83,8 @@ const defaultProps = { orderIndex: RackOrderIndex.BOTTOM_LEFT, coolingType: RackCoolingType.AIR, }, + existingRackId: 7n, existingRacks: [], - seededMinerIds: ["miner-1"], onDismiss: vi.fn(), onSave: vi.fn(), }; diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx index 0573f98d3d..86c91fa6da 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx @@ -54,23 +54,19 @@ function filterAssignmentsByValues(record: Record, keepSet: Set< interface ManageRackModalProps { show: boolean; rackSettings: RackFormData; - existingRackId?: bigint; + // The rack always exists by the time this modal opens: Rack Settings creates + // it on its CTA, so there is no staged-create mode to support. + existingRackId: bigint; existingRacks: DeviceSet[]; - // Pre-seeds the new rack's miner list (e.g. from a bulk "Add to rack → - // New rack" flow) so the selected miners land in the left pane ready - // for slot assignment. Ignored in edit mode (existingRackId set). - seededMinerIds?: string[]; // Page-header site scope (single-site only). Forwarded to the embedded - // RackSettingsModal so a new rack created within a site scope keeps its Site - // field locked to that scope. Ignored for an existing rack (edit). + // RackSettingsModal. scopedSiteId?: bigint; onDismiss: () => void; onSave: () => void; - // Fired after the Rack Settings "Continue" persists an EXISTING rack's - // settings (label/placement/zone/dims) — which happens before the final - // miner Save. Parents should refetch in the background so the rack list / - // overview stays consistent even if the operator dismisses the modal - // without pressing Save. No-op for a new rack (nothing is persisted yet). + // Fired after a write lands that the host's own list/overview reflects — the + // Rack Settings save, and each membership commit. Parents should refetch in + // the background so the rack list stays consistent even if the operator + // dismisses without pressing the final placement Save. onSettingsPersisted?: () => void; onDelete?: () => Promise | void; } @@ -80,7 +76,6 @@ export default function ManageRackModal({ rackSettings: initialRackSettings, existingRackId, existingRacks, - seededMinerIds, scopedSiteId, onDismiss, onSave, @@ -109,12 +104,11 @@ export default function ManageRackModal({ // Target-rack placement for the selection modals' eligibility filter. // rackSettings always reflects the rack's LIVE persisted placement: a - // Site/Building change in Rack Settings is persisted immediately on Continue + // Site/Building change in Rack Settings is persisted immediately on Save // (handleRackSettingsUpdate), which also cascades the rack's members to the // new placement. So by the time this filter runs, the rack and its members // are already at the placement in rackSettings — no current-vs-pending split, - // and a miner already at the new destination reads as assignable. A new rack - // has no persisted placement, so rackSettings is the intended placement. + // and a miner already at the new destination reads as assignable. const eligibility = useMemo( () => ({ rackId: existingRackId, @@ -124,10 +118,10 @@ export default function ManageRackModal({ [existingRackId, rackSettings.siteId, rackSettings.buildingId], ); - // Core assignment state. A new rack (no existingRackId) can be seeded - // with miners from a bulk "Add to rack → New rack" flow; edit mode - // ignores the seed and loads the rack's real membership below. - const [rackMiners, setRackMiners] = useState(() => (existingRackId ? [] : (seededMinerIds ?? []))); + // Core assignment state, loaded from the rack's real membership below. A + // bulk "Add to rack → New rack" flow seeds its miners onto the create call + // itself, so they arrive here as persisted members like any others. + const [rackMiners, setRackMiners] = useState([]); const [slotAssignments, setSlotAssignments] = useState>({}); const [assignmentMode, setAssignmentMode] = useState("manual"); const [manualAssignmentCache, setManualAssignmentCache] = useState>({}); @@ -154,17 +148,13 @@ export default function ManageRackModal({ const [reparentConfirm, setReparentConfirm] = useState<{ count: number; onConfirm: () => void } | null>(null); // Loading / error state - const [isLoading, setIsLoading] = useState(!!existingRackId); + const [isLoading, setIsLoading] = useState(true); const [loadFailed, setLoadFailed] = useState(false); const [isSaving, setIsSaving] = useState(false); const [errorMsg, setErrorMsg] = useState(""); - // No longer need initial state snapshots — saveRack replaces membership atomically. - - // Fetch existing data for edit mode + // Load the rack's members and slots. useEffect(() => { - if (!existingRackId) return; - let cancelled = false; let loadedMembers = false; let loadedSlots = false; @@ -587,86 +577,74 @@ export default function ManageRackModal({ [eligibility, totalSlots, promptReparent], ); - // RackSettingsModal "Continue" handler. For an EXISTING rack, Continue is - // the settings save: it persists label/zone/dimensions AND placement in a - // single atomic UpdateDeviceSet, then cascades the rack's CURRENT server - // members to the new placement — all server-side, in one transaction. This - // is why the eligibility filter above can trust rackSettings as the rack's - // live placement: by the time the operator opens Manage Miners, the rack and - // its members are already there. Membership is untouched here — "Continue - // saves settings, Save saves miners" — so the modal's draft rackMiners can't - // leak into a settings-only change. - // - // A NEW rack doesn't exist yet, so there's nothing to persist; its settings - // (including placement) ride on the create in handleSave. + // Rack Settings "Save" handler: persists label/zone/dimensions AND placement + // in a single atomic UpdateDeviceSet, then cascades the rack's CURRENT server + // members to the new placement — all server-side, in one transaction. This is + // why the eligibility filter above can trust rackSettings as the rack's live + // placement: by the time the operator opens Manage Miners, the rack and its + // members are already there. Membership is untouched here, so the modal's + // draft rackMiners can't leak into a settings-only change. const handleRackSettingsUpdate = useCallback( async (formData: RackFormData) => { - const rackId = existingRackId; - if (rackId !== undefined) { - // Only send placement when the operator actually changed site/building - // this edit (compared to what the form was seeded with). A metadata-only - // edit (label/zone/dims) omits placement, so UpdateDeviceSet preserves - // the rack's CURRENT server placement — a stale cached value can't - // re-parent a rack that another session moved while this modal was open. - // Zone stays authoritative even with placement omitted (the settings - // path treats an empty zone as an explicit clear). - const placementChanged = - canManagePlacement && - (formData.siteId !== rackSettings.siteId || formData.buildingId !== rackSettings.buildingId); - let updated: DeviceSet | undefined; - try { - await new Promise((resolve, reject) => { - updateRack({ - deviceSetId: rackId, - label: formData.label, - zone: formData.zone, - rows: formData.rows, - columns: formData.columns, - orderIndex: formData.orderIndex, - coolingType: formData.coolingType, - // Unset level -> 0n unassign when the operator did change placement. - siteId: placementChanged ? (formData.siteId ?? 0n) : undefined, - buildingId: placementChanged ? (formData.buildingId ?? 0n) : undefined, - onSuccess: (ds) => { - updated = ds; - resolve(); - }, - onError: (msg) => reject(new Error(msg)), - }); - }); - } catch (err) { - pushToast({ - message: getErrorMessage(err, "Failed to update rack settings. Please try again."), - status: STATUSES.error, + // Only send placement when the operator actually changed site/building + // this edit (compared to what the form was seeded with). A metadata-only + // edit (label/zone/dims) omits placement, so UpdateDeviceSet preserves + // the rack's CURRENT server placement — a stale cached value can't + // re-parent a rack that another session moved while this modal was open. + // Zone stays authoritative even with placement omitted (the settings + // path treats an empty zone as an explicit clear). + const placementChanged = + canManagePlacement && + (formData.siteId !== rackSettings.siteId || formData.buildingId !== rackSettings.buildingId); + let updated: DeviceSet | undefined; + try { + await new Promise((resolve, reject) => { + updateRack({ + deviceSetId: existingRackId, + label: formData.label, + zone: formData.zone, + rows: formData.rows, + columns: formData.columns, + orderIndex: formData.orderIndex, + coolingType: formData.coolingType, + // Unset level -> 0n unassign when the operator did change placement. + siteId: placementChanged ? (formData.siteId ?? 0n) : undefined, + buildingId: placementChanged ? (formData.buildingId ?? 0n) : undefined, + onSuccess: (ds) => { + updated = ds; + resolve(); + }, + onError: (msg) => reject(new Error(msg)), }); - // Keep Rack Settings open (don't apply) so the operator can retry. - return; - } - // Adopt the server's AUTHORITATIVE placement from the response, not the - // submitted formData: when placement was omitted (metadata-only edit) - // the server kept whatever the rack's current site/building is — which - // may differ from the stale formData values if another session moved it. - // The eligibility filter reads rackSettings placement, so trusting the - // response keeps the miner list scoped to where the rack really is. - const serverRackInfo = updated?.typeDetails.case === "rackInfo" ? updated.typeDetails.value : undefined; - const applied: RackFormData = serverRackInfo - ? { - ...formData, - siteId: serverRackInfo.siteId, - buildingId: serverRackInfo.buildingId, - zone: serverRackInfo.zone, - } - : formData; - // Settings are now live on the server. Let the parent refetch so its - // rack list/overview reflects the new label/placement even if the - // operator dismisses without pressing the final miner Save. - onSettingsPersisted?.(); - setRackSettings(applied); - setShowRackSettings(false); + }); + } catch (err) { + pushToast({ + message: getErrorMessage(err, "Failed to update rack settings. Please try again."), + status: STATUSES.error, + }); + // Keep Rack Settings open (don't apply) so the operator can retry. return; } - - setRackSettings(formData); + // Adopt the server's AUTHORITATIVE placement from the response, not the + // submitted formData: when placement was omitted (metadata-only edit) + // the server kept whatever the rack's current site/building is — which + // may differ from the stale formData values if another session moved it. + // The eligibility filter reads rackSettings placement, so trusting the + // response keeps the miner list scoped to where the rack really is. + const serverRackInfo = updated?.typeDetails.case === "rackInfo" ? updated.typeDetails.value : undefined; + const applied: RackFormData = serverRackInfo + ? { + ...formData, + siteId: serverRackInfo.siteId, + buildingId: serverRackInfo.buildingId, + zone: serverRackInfo.zone, + } + : formData; + // Settings are now live on the server. Let the parent refetch so its + // rack list/overview reflects the new label/placement even if the + // operator dismisses without pressing the final miner Save. + onSettingsPersisted?.(); + setRackSettings(applied); setShowRackSettings(false); }, [existingRackId, canManagePlacement, rackSettings, updateRack, onSettingsPersisted], @@ -675,9 +653,8 @@ export default function ManageRackModal({ // Save handler — single atomic RPC const handleSave = useCallback(async () => { // Capacity guard. handleManageMinersConfirm enforces this when miners are - // added through the sub-modal, but a seeded new rack (bulk "New rack") - // populates rackMiners directly and bypasses that path — saveRack accepts - // members beyond the slot count, so an over-fill would persist silently. + // added through the sub-modal, but shrinking the layout in Rack Settings + // can leave an already-loaded rack over its new capacity. if (rackMiners.length > totalSlots) { setErrorMsg( `Cannot add ${rackMiners.length} miners with only ${totalSlots} available slots. Deselect some miners or update your rack settings.`, @@ -695,19 +672,12 @@ export default function ManageRackModal({ return { deviceIdentifier: deviceId, row, column: col }; }); - // Placement rides on CREATE only. An existing rack's Site/Building (and - // zone/dimensions) are already persisted on the Rack Settings "Continue" - // — Continue saves settings, Save saves miners — so an edit Save omits - // placement: it preserves the rack's current server placement and can't - // clobber a move made by another session while this modal was open. - const sendPlacement = canManagePlacement && existingRackId === undefined; - - // Existing-rack Save is miners-only, but SaveRack always rewrites - // rack_info, so re-send the rack's CURRENT server metadata rather than the - // modal's cached copy — otherwise a concurrent zone/dimension edit from - // another session would be reverted (and stale dims could mis-validate the - // new slots). Best-effort: fall back to the cached values on a fetch miss - // so a transient error can't block the miner save. + // Save is miners-only, but SaveRack always rewrites rack_info, so re-send + // the rack's CURRENT server metadata rather than the modal's cached copy — + // otherwise a concurrent zone/dimension edit from another session would be + // reverted (and stale dims could mis-validate the new slots). Best-effort: + // fall back to the cached values on a fetch miss so a transient error + // can't block the miner save. let meta = { label: rackSettings.label, zone: rackSettings.zone, @@ -716,35 +686,30 @@ export default function ManageRackModal({ orderIndex: rackSettings.orderIndex, coolingType: rackSettings.coolingType, }; - if (existingRackId !== undefined) { - await new Promise((resolve) => { - getDeviceSet({ - deviceSetId: existingRackId, - onSuccess: (ds) => { - if (ds.typeDetails.case === "rackInfo") { - const ri = ds.typeDetails.value; - meta = { - label: ds.label, - zone: ri.zone, - rows: ri.rows, - columns: ri.columns, - orderIndex: ri.orderIndex, - coolingType: ri.coolingType, - }; - } - resolve(); - }, - onNotFound: () => resolve(), - onError: () => resolve(), - }); + await new Promise((resolve) => { + getDeviceSet({ + deviceSetId: existingRackId, + onSuccess: (ds) => { + if (ds.typeDetails.case === "rackInfo") { + const ri = ds.typeDetails.value; + meta = { + label: ds.label, + zone: ri.zone, + rows: ri.rows, + columns: ri.columns, + orderIndex: ri.orderIndex, + coolingType: ri.coolingType, + }; + } + resolve(); + }, + onNotFound: () => resolve(), + onError: () => resolve(), }); - } + }); const finishSuccess = () => { - pushToast({ - message: existingRackId ? `Rack "${meta.label}" updated` : `Rack "${meta.label}" created`, - status: STATUSES.success, - }); + pushToast({ message: `Rack "${meta.label}" updated`, status: STATUSES.success }); onSave(); }; @@ -765,10 +730,10 @@ export default function ManageRackModal({ coolingType: meta.coolingType, deviceIdentifiers: rackMiners, slotAssignments: slotAssignmentsList, - // Create sends its chosen placement (unset level → NULL), gated on - // site:manage. Edit omits placement (persisted on Continue). - siteId: sendPlacement ? (rackSettings.siteId ?? 0n) : undefined, - buildingId: sendPlacement ? (rackSettings.buildingId ?? 0n) : undefined, + // Placement is omitted: it's already persisted by the Rack Settings + // Save, so leaving it off preserves the rack's current server + // placement and can't clobber a move another session made while this + // modal was open. forceClearConflictingSite: force, onSuccess: () => resolve("ok"), onConflicts: (conflicts) => { @@ -800,17 +765,7 @@ export default function ManageRackModal({ } finally { setIsSaving(false); } - }, [ - existingRackId, - rackSettings, - rackMiners, - totalSlots, - activeAssignments, - canManagePlacement, - getDeviceSet, - saveRack, - onSave, - ]); + }, [existingRackId, rackSettings, rackMiners, totalSlots, activeAssignments, getDeviceSet, saveRack, onSave]); if (!show) return null; @@ -917,10 +872,10 @@ export default function ManageRackModal({ show={showRackSettings} existingRacks={existingRacks} initialFormData={rackSettings} - existingRack={existingRackId !== undefined} + existingRack defaultSiteId={scopedSiteId} onDismiss={() => setShowRackSettings(false)} - onContinue={handleRackSettingsUpdate} + onSubmit={handleRackSettingsUpdate} /> ) : null} diff --git a/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.stories.tsx b/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.stories.tsx index 97fb11ce58..c1e9ea66e7 100644 --- a/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.stories.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.stories.tsx @@ -26,8 +26,8 @@ export const CreateNew = () => { action("onDismiss")(); setShow(false); }} - onContinue={(formData) => { - action("onContinue")(formData); + onSubmit={(formData) => { + action("onSubmit")(formData); setShow(false); }} /> diff --git a/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.tsx b/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.tsx index b7c6e16b28..078fd7b74b 100644 --- a/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.tsx @@ -37,7 +37,12 @@ interface RackSettingsModalProps { // May be async: the caller persists the settings (an UpdateDeviceSet for an // existing rack, a create for a new one), so we await it and keep the button // busy until it resolves — a rejection leaves the modal open for a retry. - onContinue?: (formData: RackFormData) => void | Promise; + onSubmit?: (formData: RackFormData) => void | Promise; + // Caller-driven busy state, OR'd with our own in-flight submit. Needed for + // writes the caller retries on its own — a create that came back with a + // reparent conflict is re-dispatched from the confirmation dialog, long after + // our awaited onSubmit resolved. + saving?: boolean; } // Explicit "Unassigned" entry for the placement dropdowns. The shared Select @@ -72,7 +77,8 @@ const RackSettingsModal = ({ defaultSiteId, existingRack, onDismiss, - onContinue, + onSubmit, + saving, }: RackSettingsModalProps) => { const { listRackZones, listRackTypes } = useDeviceSets(); const { sites } = useSitesContext(); @@ -145,7 +151,8 @@ const RackSettingsModal = ({ initialFormData?.orderIndex ?? RackOrderIndex.BOTTOM_LEFT, ); const [coolingType, setCoolingType] = useState(initialFormData?.coolingType ?? RackCoolingType.AIR); - const [isSubmitting, setIsSubmitting] = useState(false); + const [ownSubmit, setOwnSubmit] = useState(false); + const isSubmitting = ownSubmit || !!saving; const [labelError, setLabelError] = useState(); const [columnsError, setColumnsError] = useState(); const [rowsError, setRowsError] = useState(); @@ -342,6 +349,37 @@ const RackSettingsModal = ({ [rackTypes], ); + // Exactly the fields handleSubmit puts on RackFormData, compared against what + // the form was seeded with — so the gate can only be clean when the write + // would be a no-op. Creating a rack is always a real write, so it never + // gates. Placement is included even when the selects are hidden + // (rack:manage-only): they're then seeded from initialFormData and can't + // diverge, so this reads clean either way. + const isDirty = useMemo(() => { + if (!isExistingRack || !initialFormData) return true; + return ( + label.trim() !== initialFormData.label || + zone.trim() !== initialFormData.zone || + Number(rows) !== initialFormData.rows || + Number(columns) !== initialFormData.columns || + orderIndex !== initialFormData.orderIndex || + coolingType !== initialFormData.coolingType || + (isRealId(siteIdText) ? BigInt(siteIdText) : undefined) !== initialFormData.siteId || + (isRealId(buildingIdText) ? BigInt(buildingIdText) : undefined) !== initialFormData.buildingId + ); + }, [ + isExistingRack, + initialFormData, + label, + zone, + rows, + columns, + orderIndex, + coolingType, + siteIdText, + buildingIdText, + ]); + const handleSubmit = useCallback(async () => { setLabelError(undefined); setColumnsError(undefined); @@ -382,13 +420,13 @@ const RackSettingsModal = ({ // create for a new one. Await it and keep the button busy so a slow save // can't be double-submitted; the caller leaves this modal open on failure // so the operator can retry. - setIsSubmitting(true); + setOwnSubmit(true); try { - await onContinue?.(formData); + await onSubmit?.(formData); } finally { - setIsSubmitting(false); + setOwnSubmit(false); } - }, [label, zone, rows, columns, orderIndex, coolingType, siteIdText, buildingIdText, onContinue]); + }, [label, zone, rows, columns, orderIndex, coolingType, siteIdText, buildingIdText, onSubmit]); if (!show) return null; @@ -404,9 +442,13 @@ const RackSettingsModal = ({ divider={false} buttons={[ { - text: isSubmitting ? "Saving..." : "Continue", + // Named for the write it makes: creating the rack, or saving settings + // onto one that already exists. Disabled with no diff — an existing + // rack's Save would otherwise re-persist identical values and toast + // as though something changed. + text: isExistingRack ? (isSubmitting ? "Saving..." : "Save") : isSubmitting ? "Creating..." : "Create rack", variant: "primary", - disabled: isSubmitting || isInitialLoading, + disabled: isSubmitting || isInitialLoading || !isDirty, loading: isSubmitting, onClick: handleSubmit, dismissModalOnClick: false, diff --git a/client/src/protoFleet/features/fleetManagement/hooks/useCreateRack.ts b/client/src/protoFleet/features/fleetManagement/hooks/useCreateRack.ts new file mode 100644 index 0000000000..6bc998b6b9 --- /dev/null +++ b/client/src/protoFleet/features/fleetManagement/hooks/useCreateRack.ts @@ -0,0 +1,130 @@ +import { useCallback, useRef, useState } from "react"; + +import { type PerDeviceRackConflict } from "@/protoFleet/api/generated/device_set/v1/device_set_pb"; +import { useDeviceSets } from "@/protoFleet/api/useDeviceSets"; +import { type RackFormData } from "@/protoFleet/features/fleetManagement/components/ManageRackModal/types"; + +import { pushToast, STATUSES } from "@/shared/features/toaster"; + +/** + * Creates a rack from the Rack Settings form, optionally seeding it with + * miners, and reports the new rack's id. + * + * SaveRack-with-no-id is the one call that can land dimensions, zone, + * placement and a seeded member set in a single transaction, so a failed seed + * can't strand an empty rack. That makes it right for create — and only for + * create. Every subsequent edit goes through the delta RPCs (UpdateDeviceSet + * for settings, AssignDevicesToRack for members and slots), because SaveRack + * replaces the rack's whole member set and would clobber concurrent changes. + * + * Both rack entry points (RacksPage's "Add rack" and the bulk + * "Add to rack → New rack" flow) share this so the create semantics — the + * toast, the in-flight guard, the site-strip confirmation — can't drift apart. + */ +export interface UseCreateRackResult { + /** + * Creates the rack and resolves with its id, or undefined when the create + * did not happen (an error, or a site-strip conflict awaiting confirmation). + * On a conflict the returned promise resolves undefined and `conflict` + * becomes non-null; call `confirmConflict` to retry with the strip forced. + */ + createRack: (formData: RackFormData, seededMinerIds?: string[]) => Promise; + creating: boolean; + /** + * Non-null while a site-strip confirmation is pending, carrying what the + * warning dialog needs: how many seeded miners would be displaced, and the + * label of the rack they'd move into. + */ + conflict: { count: number; rackLabel: string } | null; + confirmConflict: () => void; + cancelConflict: () => void; +} + +export function useCreateRack({ + // Receives the submitted form data alongside the new id: the caller opens + // ManageRackModal on it, and the forced-retry path fires this from the + // confirmation dialog, where the form data is no longer in scope. + onCreated, +}: { + onCreated: (rackId: bigint, formData: RackFormData) => void; +}): UseCreateRackResult { + const { saveRack } = useDeviceSets(); + const [creating, setCreating] = useState(false); + // The `creating` state lags a render behind the click, so the ref is what + // actually blocks a double-click from dispatching two creates. + const creatingRef = useRef(false); + const [conflict, setConflict] = useState<{ count: number; rackLabel: string } | null>(null); + // Holds the inputs for the forced retry while the confirmation is showing. + const pendingRef = useRef<{ formData: RackFormData; seededMinerIds: string[] } | null>(null); + + const dispatch = useCallback( + (formData: RackFormData, seededMinerIds: string[], force: boolean): Promise => { + if (creatingRef.current) return Promise.resolve(undefined); + creatingRef.current = true; + setCreating(true); + return new Promise((resolve) => { + saveRack({ + label: formData.label, + zone: formData.zone, + rows: formData.rows, + columns: formData.columns, + orderIndex: formData.orderIndex, + coolingType: formData.coolingType, + deviceIdentifiers: seededMinerIds, + // Slots are the operator's next step, in the manage modal. Seeded + // miners land as members without a position. + slotAssignments: [], + // A new rack has no prior placement to preserve, so send the chosen + // one explicitly — including "unassigned", which the form encodes as + // undefined and the wire as 0. + siteId: formData.siteId ?? 0n, + buildingId: formData.buildingId ?? 0n, + forceClearConflictingSite: force, + onSuccess: (deviceSet) => { + pushToast({ message: `Rack "${formData.label}" created`, status: STATUSES.success }); + setConflict(null); + pendingRef.current = null; + onCreated(deviceSet.id, formData); + resolve(deviceSet.id); + }, + // Seeded miners that currently have a site/building the new rack + // lacks would be stripped. The server wrote nothing; hold the inputs + // so the caller's confirmation can retry with force. + onConflicts: (conflicts: PerDeviceRackConflict[]) => { + pendingRef.current = { formData, seededMinerIds }; + setConflict({ count: conflicts.length, rackLabel: formData.label }); + resolve(undefined); + }, + onError: (message) => { + pushToast({ message: message || "Failed to create rack. Please try again.", status: STATUSES.error }); + resolve(undefined); + }, + onFinally: () => { + creatingRef.current = false; + setCreating(false); + }, + }); + }); + }, + [saveRack, onCreated], + ); + + const createRack = useCallback( + (formData: RackFormData, seededMinerIds?: string[]) => dispatch(formData, seededMinerIds ?? [], false), + [dispatch], + ); + + const confirmConflict = useCallback(() => { + const pending = pendingRef.current; + if (!pending) return; + setConflict(null); + void dispatch(pending.formData, pending.seededMinerIds, true); + }, [dispatch]); + + const cancelConflict = useCallback(() => { + pendingRef.current = null; + setConflict(null); + }, []); + + return { createRack, creating, conflict, confirmConflict, cancelConflict }; +} diff --git a/client/src/protoFleet/features/fleetManagement/pages/RacksPage.tsx b/client/src/protoFleet/features/fleetManagement/pages/RacksPage.tsx index 2be4287c45..7efdb1d88d 100644 --- a/client/src/protoFleet/features/fleetManagement/pages/RacksPage.tsx +++ b/client/src/protoFleet/features/fleetManagement/pages/RacksPage.tsx @@ -31,8 +31,10 @@ import FleetGroupActionsMenu from "@/protoFleet/features/fleetManagement/compone import FleetGroupListActionBar from "@/protoFleet/features/fleetManagement/components/FleetGroupActionsMenu/FleetGroupListActionBar"; import { useOptionalFleetOutletContext } from "@/protoFleet/features/fleetManagement/components/FleetLayout"; import { ManageRackModal, type RackFormData } from "@/protoFleet/features/fleetManagement/components/ManageRackModal"; +import ReparentWarningDialog from "@/protoFleet/features/fleetManagement/components/ManageRackModal/ReparentWarningDialog"; import { RackCard } from "@/protoFleet/features/fleetManagement/components/RackCard"; import RackSettingsModal from "@/protoFleet/features/fleetManagement/components/RackSettingsModal"; +import { useCreateRack } from "@/protoFleet/features/fleetManagement/hooks/useCreateRack"; import { BUILDING_URL_PARAM } from "@/protoFleet/features/fleetManagement/utils/buildingFilterUrl"; import { FILTER_URL_PARAM_KEYS, @@ -874,12 +876,6 @@ const RacksPage = () => { return ; }, [hasActiveFilters, isLoading, totalCount, handleClearFilters]); - const handleRackSettingsContinue = useCallback((formData: RackFormData) => { - setShowRackSettingsModal(false); - setManageRackFormData(formData); - setManageRackId(undefined); - }, []); - const handleManageRackDismiss = useCallback(() => { setManageRackFormData(null); setManageRackId(undefined); @@ -891,14 +887,33 @@ const RacksPage = () => { resetAndFetch(); fetchZones(); }, [resetAndFetch, fetchZones]); - // Rack Settings "Continue" persists an existing rack's settings before the - // final miner Save, so refresh the list in the background (without closing - // the modal) to keep it consistent if the operator then dismisses. + // Rack Settings "Save" persists an existing rack's settings before the final + // miner Save, so refresh the list in the background (without closing the + // modal) to keep it consistent if the operator then dismisses. const handleRackSettingsPersisted = useCallback(() => { resetAndFetch(); fetchZones(); }, [resetAndFetch, fetchZones]); + // "Create rack" in Rack Settings creates it for real, then hands off to + // ManageRackModal for miners and placement. The rack exists from here on, so + // the list refreshes immediately rather than waiting for a later Save. + const { + createRack, + creating: creatingRack, + conflict: rackCreateConflict, + confirmConflict, + cancelConflict, + } = useCreateRack({ + onCreated: (rackId, formData) => { + setShowRackSettingsModal(false); + setManageRackFormData(formData); + setManageRackId(rackId); + resetAndFetch(); + fetchZones(); + }, + }); + const handleDeleteRack = useCallback(() => { if (!manageRackId) return Promise.resolve(); return new Promise((resolve, reject) => { @@ -1134,12 +1149,13 @@ const RacksPage = () => { existingRacks={racks} defaultSiteId={scopedSiteId} onDismiss={() => setShowRackSettingsModal(false)} - onContinue={handleRackSettingsContinue} + onSubmit={createRack} + saving={creatingRack} /> ) : null} - {manageRackFormData ? ( + {manageRackFormData && manageRackId !== undefined ? ( { onDismiss={handleManageRackDismiss} onSave={handleManageRackSave} onSettingsPersisted={handleRackSettingsPersisted} - onDelete={manageRackId ? handleDeleteRack : undefined} + onDelete={handleDeleteRack} + /> + ) : null} + {rackCreateConflict ? ( + ) : null} @@ -1461,12 +1485,13 @@ const RacksPage = () => { existingRacks={racks} defaultSiteId={scopedSiteId} onDismiss={() => setShowRackSettingsModal(false)} - onContinue={handleRackSettingsContinue} + onSubmit={createRack} + saving={creatingRack} /> ) : null} - {manageRackFormData ? ( + {manageRackFormData && manageRackId !== undefined ? ( { onSettingsPersisted={handleRackSettingsPersisted} /> ) : null} + {rackCreateConflict ? ( + + ) : null} {reparentTarget ? ( Date: Wed, 29 Jul 2026 17:48:00 -0700 Subject: [PATCH 09/12] refactor(racks): commit rack membership in the pickers, Save owns placement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ManageRackModal funnelled four separate concerns through one SaveRack call: the rack's own fields, its parent pointer, its membership, and the placement of miners within it. Because SaveRack replaces the whole device selector with no version precondition, any Save wrote back the membership snapshot the modal loaded — so a concurrent add elsewhere was silently dropped. Split it the same way sites and buildings were split: - The miner pickers (Manage miners, Select from list, Add by QR scan) now commit their own membership through AssignDevicesToRack, which is a delta, not a replacement. Their CTA is "Save" because it writes. - ManageRackModal's Save owns placement only. It sends the slot delta — the miners whose cell actually changed — with an unset position meaning "unplace" and an unnamed miner meaning "leave alone". - Save is disabled until that delta is non-empty, so a clean Save can no longer report success for a write it never made. The load effect now snapshots placement as it came from the server, and membership commits fold newcomers into it as "unplaced". Without that, adding a miner would leave the Save gate dirty with placement the operator never touched. Membership commits and placement saves share the server's site-strip refusal path, routed through the same ReparentWarningDialog as the client-side check. MinersPane's row controls gained aria-labels; they were previously reachable only through an unnamed ellipsis button. Co-Authored-By: Claude --- .../FleetCreateFlowProvider.tsx | 3 + .../ManageMinersModal.test.tsx | 49 ++- .../ManageRackModal/ManageMinersModal.tsx | 52 ++- .../ManageRackModal/ManageRackModal.test.tsx | 81 +++- .../ManageRackModal/ManageRackModal.tsx | 390 +++++++++++------- .../components/ManageRackModal/MinersPane.tsx | 5 + .../ManageRackModal/ScanMinerQrModal.tsx | 20 +- 7 files changed, 439 insertions(+), 161 deletions(-) diff --git a/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx b/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx index 0b4960d1b6..16993a2dfe 100644 --- a/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx @@ -421,6 +421,9 @@ const FleetCreateFlowProvider = ({ scopedSiteId={scopedSiteId} onDismiss={closeRackFlow} onSave={handleRackSaved} + // Membership commits land while this modal is open; pulse the lists so + // they're right even if the operator dismisses without positioning. + onSettingsPersisted={bumpEntities} /> ) : null} {rackCreateConflict ? ( diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.test.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.test.tsx index 8d08ef7571..969a871a59 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.test.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.test.tsx @@ -20,6 +20,10 @@ vi.mock("@/protoFleet/components/MinerSelectionList", () => ({ useEffect(() => { propsRef.current = props; latestProps.current = props; + // The real list pushes its selection up on mount and on every change, + // which is what feeds the Save dirty gate. + const { selectedItems, allSelected, totalMiners } = mockGetSelection(); + props.onSelectionChange?.({ selectedItems, allSelected, totalMiners }); }); useImperativeHandle(ref, () => ({ getSelection: mockGetSelection, @@ -34,7 +38,7 @@ vi.mock("@/shared/components/Modal", () => ({
{children} {buttons?.map((btn: any, i: number) => ( - ))} @@ -103,7 +107,7 @@ describe("ManageMinersModal", () => { expect(latestProps.current.eligibility).toEqual({ rackId: 5n, siteId: 2n, buildingId: 3n }); }); - it("calls onConfirm with selected IDs on continue", () => { + it("calls onConfirm with selected IDs on save", () => { const onConfirm = vi.fn(); mockGetSelection.mockReturnValue({ selectedItems: ["miner-1", "miner-2"], @@ -114,7 +118,7 @@ describe("ManageMinersModal", () => { }); render(); - fireEvent.click(screen.getByText(/Continue/)); + fireEvent.click(screen.getByText(/Save/)); expect(onConfirm).toHaveBeenCalledWith(["miner-1", "miner-2"], false, undefined, []); }); @@ -129,7 +133,7 @@ describe("ManageMinersModal", () => { }); render(); - fireEvent.click(screen.getByText(/Continue/)); + fireEvent.click(screen.getByText(/Save/)); expect(screen.getByText(/Cannot add 3 miners with only 2 available slots/)).toBeInTheDocument(); }); @@ -145,12 +149,12 @@ describe("ManageMinersModal", () => { }); render(); - fireEvent.click(screen.getByText(/Continue/)); + fireEvent.click(screen.getByText(/Save/)); expect(onConfirm).not.toHaveBeenCalled(); }); - it("blocks Continue and prompts to clear the filter when a placement facet conflicts", () => { + it("blocks Save and prompts to clear the filter when a placement facet conflicts", () => { const onConfirm = vi.fn(); mockGetSelection.mockReturnValue({ selectedItems: ["m1", "m2"], @@ -161,10 +165,41 @@ describe("ManageMinersModal", () => { }); render(); - fireEvent.click(screen.getByText(/Continue/)); + fireEvent.click(screen.getByText(/Save/)); // No save (which would otherwise resolve/commit a hidden selection). expect(onConfirm).not.toHaveBeenCalled(); expect(screen.getByText(/Clear the Building or Rack filter/i)).toBeInTheDocument(); }); + + it("disables Save until the selection differs from the rack's members", () => { + mockGetSelection.mockReturnValue({ + selectedItems: ["miner-1"], + allSelected: false, + totalMiners: 10, + reassignedItems: [], + blockedByFilter: false, + }); + + const { unmount } = render(); + expect(screen.getByText(/Save/)).toBeDisabled(); + unmount(); + + // Same count, different miner — still a real membership change. + render(); + expect(screen.getByText(/Save/)).toBeEnabled(); + }); + + it("treats select-all as a change, since it resolves server-side", () => { + mockGetSelection.mockReturnValue({ + selectedItems: ["miner-1"], + allSelected: true, + totalMiners: 10, + reassignedItems: [], + blockedByFilter: false, + }); + + render(); + expect(screen.getByText(/Save/)).toBeEnabled(); + }); }); diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.tsx index bc9de097dd..d147844b75 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.tsx @@ -1,4 +1,4 @@ -import { useCallback, useRef, useState } from "react"; +import { useCallback, useMemo, useRef, useState } from "react"; import { type MinerListFilter } from "@/protoFleet/api/generated/fleetmanagement/v1/fleetmanagement_pb"; import MinerSelectionList, { @@ -12,6 +12,11 @@ import { Alert } from "@/shared/assets/icons"; import Callout from "@/shared/components/Callout"; import Modal from "@/shared/components/Modal"; +interface SelectionSnapshot { + selectedItems: string[]; + allSelected: boolean; +} + interface ManageMinersModalProps { show: boolean; currentRackMiners: string[]; @@ -38,6 +43,8 @@ interface ManageMinersModalProps { filter: MinerListFilter | undefined, reassignedItems: string[], ) => Promise; + // In-flight signal from the host's write, mirrored into the CTA. + saving?: boolean; } export default function ManageMinersModal({ @@ -49,11 +56,44 @@ export default function ManageMinersModal({ scope, onDismiss, onConfirm, + saving = false, }: ManageMinersModalProps) { const selectionRef = useRef(null); const [overflowError, setOverflowError] = useState(""); + // Mirrors the list's selection so Save can be gated on a real change. The + // list reports its seeded selection on mount, so this is populated before the + // operator touches anything. + const [selection, setSelection] = useState<{ selectedItems: string[]; allSelected: boolean } | null>(null); + + // Keeps the previous object when the selection is equivalent, so React bails + // out of the re-render. Without that, the list re-reports on every render (its + // notify effect depends on the array's identity) and we'd loop forever. + const handleSelectionChange = useCallback(({ selectedItems, allSelected }: SelectionSnapshot) => { + setSelection((prev) => { + if ( + prev && + prev.allSelected === allSelected && + prev.selectedItems.length === selectedItems.length && + prev.selectedItems.every((id, i) => id === selectedItems[i]) + ) { + return prev; + } + return { selectedItems, allSelected }; + }); + }, []); + + // Save writes the membership delta, so it's disabled when the selection still + // matches what's in the rack. "Select all" resolves server-side, so it can't + // be compared here and always counts as a change. + const isDirty = useMemo(() => { + if (!selection) return false; + if (selection.allSelected) return true; + if (selection.selectedItems.length !== currentRackMiners.length) return true; + const current = new Set(currentRackMiners); + return selection.selectedItems.some((id) => !current.has(id)); + }, [selection, currentRackMiners]); - const handleContinue = useCallback(async () => { + const handleSave = useCallback(async () => { const selection = selectionRef.current?.getSelection(); if (!selection) return; @@ -96,9 +136,12 @@ export default function ManageMinersModal({ divider={false} buttons={[ { - text: "Continue", + // Names the write it makes: this picker owns rack membership. + text: saving ? "Saving..." : "Save", variant: "primary", - onClick: handleContinue, + disabled: saving || !isDirty, + loading: saving, + onClick: handleSave, dismissModalOnClick: false, }, ]} @@ -124,6 +167,7 @@ export default function ManageMinersModal({ }} scope={scope} initialSelectedItems={currentRackMiners} + onSelectionChange={handleSelectionChange} eligibility={eligibility} targetRackLabel={targetRackLabel} pairingStatuses={FLEET_VISIBLE_PAIRING_STATUSES} diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.test.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.test.tsx index c69c05de89..96624ef4d2 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.test.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.test.tsx @@ -1,6 +1,6 @@ import { MemoryRouter } from "react-router-dom"; -import { fireEvent, render, screen, within } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import ManageRackModal from "./ManageRackModal"; import { RackCoolingType, RackOrderIndex } from "@/protoFleet/api/generated/device_set/v1/device_set_pb"; @@ -9,6 +9,16 @@ import type { MinerStateSnapshot } from "@/protoFleet/api/generated/fleetmanagem const mockSaveRack = vi.fn(); const mockUpdateRack = vi.fn(); const mockGetDeviceSet = vi.fn(); +// Membership commits and the placement Save both go through this. Succeeds by +// default; individual tests re-stub it to inspect or fail a call. +interface AssignCall { + targetRackId?: bigint; + deviceIdentifiers: string[]; + slotAssignments?: { deviceIdentifier: string; position?: { row: number; column: number } }[]; + onSuccess?: () => void; + onError?: (message: string) => void; +} +const mockAssignDevicesToRack = vi.fn((args: AssignCall) => args.onSuccess?.()); // The rack always exists when this modal opens, so its membership comes from the // server. Resolve both loads immediately with one already-placed miner. const mockGetRackSlots = vi.fn(({ onSuccess }: { onSuccess: (slots: unknown[]) => void }) => onSuccess([])); @@ -37,6 +47,7 @@ vi.mock("@/protoFleet/components/PageHeader/SitePicker", async (importActual) => vi.mock("@/protoFleet/api/useDeviceSets", () => ({ useDeviceSets: () => ({ saveRack: mockSaveRack, + assignDevicesToRack: mockAssignDevicesToRack, updateRack: mockUpdateRack, getDeviceSet: mockGetDeviceSet, getRackSlots: mockGetRackSlots, @@ -64,9 +75,19 @@ vi.mock("@/shared/hooks/useWindowDimensions", () => ({ vi.mock("@/protoFleet/components/FullScreenTwoPaneModal", () => ({ __esModule: true, - default: ({ open, primaryPane, secondaryPane }: any) => + default: ({ open, buttons, primaryPane, secondaryPane }: any) => open ? (
+ {buttons?.map((button: any) => ( + + ))}
{primaryPane}
{secondaryPane}
@@ -97,6 +118,13 @@ const renderManageRackModal = () => ); describe("ManageRackModal", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetRackSlots.mockImplementation(({ onSuccess }) => onSuccess([])); + mockListGroupMembers.mockImplementation(({ onSuccess }) => onSuccess(["miner-1"])); + mockAssignDevicesToRack.mockImplementation((args) => args.onSuccess?.()); + }); + it("clears a selected slot when the slot actions sheet is dismissed", () => { renderManageRackModal(); @@ -118,4 +146,51 @@ describe("ManageRackModal", () => { expect(screen.getByText("Position 01")).toBeInTheDocument(); expect(screen.getByTestId("rack-slot-01")).toHaveAttribute("data-slot-state", "assigned"); }); + + it("disables Save until a miner's slot actually changes", () => { + renderManageRackModal(); + + // The rack loaded with one unplaced miner, so there is no placement delta. + expect(screen.getByRole("button", { name: "Save" })).toBeDisabled(); + + fireEvent.click(screen.getByTestId("rack-slot-01")); + fireEvent.click(within(screen.getByTestId("rack-slot-actions-sheet-content")).getByText("Select from list")); + fireEvent.click(screen.getByText("Miner 1")); + + expect(screen.getByRole("button", { name: "Save" })).toBeEnabled(); + }); + + it("Save sends only the miners whose slot changed, with the new position", async () => { + renderManageRackModal(); + + fireEvent.click(screen.getByTestId("rack-slot-01")); + fireEvent.click(within(screen.getByTestId("rack-slot-actions-sheet-content")).getByText("Select from list")); + fireEvent.click(screen.getByText("Miner 1")); + fireEvent.click(screen.getByRole("button", { name: "Save" })); + + await waitFor(() => expect(mockAssignDevicesToRack).toHaveBeenCalledTimes(1)); + const request = mockAssignDevicesToRack.mock.calls[0][0]; + expect(request.targetRackId).toBe(7n); + expect(request.deviceIdentifiers).toEqual(["miner-1"]); + expect(request.slotAssignments).toEqual([ + expect.objectContaining({ + deviceIdentifier: "miner-1", + position: expect.objectContaining({ row: 0, column: 0 }), + }), + ]); + }); + + it("keeps a miner in the list when its removal fails to persist", async () => { + mockAssignDevicesToRack.mockImplementation((args) => args.onError?.("rack is locked")); + renderManageRackModal(); + + fireEvent.click(screen.getByRole("button", { name: "Actions for Miner 1" })); + fireEvent.click(screen.getByText("Remove miner")); + + // The unassign was attempted (no target rack) and refused, so the row stays. + await waitFor(() => expect(mockAssignDevicesToRack).toHaveBeenCalledTimes(1)); + expect(mockAssignDevicesToRack.mock.calls[0][0].targetRackId).toBeUndefined(); + expect(mockAssignDevicesToRack.mock.calls[0][0].deviceIdentifiers).toEqual(["miner-1"]); + expect(screen.getByText("Miner 1")).toBeInTheDocument(); + }); }); diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx index 86c91fa6da..978248d7af 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.tsx @@ -1,4 +1,5 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { create } from "@bufbuild/protobuf"; import { fetchAllSelectableMinerIds } from "./fetchAllSelectableMinerIds"; import ManageMinersModal from "./ManageMinersModal"; @@ -9,7 +10,12 @@ import ScanMinerQrModal, { type ScanAssignmentResult } from "./ScanMinerQrModal" import SearchMinersModal from "./SearchMinersModal"; import { type AssignmentMode, orderIndexToOrigin, originLabel, type RackFormData, type SelectedSlot } from "./types"; import { useRackMinerScope } from "./useRackMinerScope"; -import { type DeviceSet, type RackSlot } from "@/protoFleet/api/generated/device_set/v1/device_set_pb"; +import { + type DeviceSet, + type RackSlot, + RackSlotPositionSchema, + RackSlotSchema, +} from "@/protoFleet/api/generated/device_set/v1/device_set_pb"; import { type MinerListFilter, type MinerStateSnapshot, @@ -82,7 +88,7 @@ export default function ManageRackModal({ onSettingsPersisted, onDelete, }: ManageRackModalProps) { - const { saveRack, updateRack, getDeviceSet, getRackSlots, listGroupMembers } = useDeviceSets(); + const { assignDevicesToRack, updateRack, getRackSlots, listGroupMembers } = useDeviceSets(); // Rack placement (site/building) is a site:manage action, enforced server- // side on SaveRack and UpdateDeviceSet. A rack:manage-only operator edits // rack contents and metadata (label/zone/dims) without touching placement, @@ -140,17 +146,37 @@ export default function ManageRackModal({ const [showScanQr, setShowScanQr] = useState(false); const [showDeleteConfirm, setShowDeleteConfirm] = useState(false); const [isDeleting, setIsDeleting] = useState(false); - const scanUndoRef = useRef<(() => void) | null>(null); - - // Pending reparent confirmation. Set when a confirm action would pull miners - // out of a rack/building/site they're currently assigned to; `onConfirm` - // runs the deferred action once the operator accepts the warning (#672). - const [reparentConfirm, setReparentConfirm] = useState<{ count: number; onConfirm: () => void } | null>(null); + const scanUndoRef = useRef<(() => Promise) | null>(null); + + // Pending reparent confirmation, from either of two sources: the picker + // reporting miners currently placed elsewhere (#672), and the server refusing + // to strip a miner's site for a site-less rack. `onConfirm` runs the deferred + // action; `onCancel` lets the server-conflict path report the refusal back to + // the write that is awaiting an answer. + const [reparentConfirm, setReparentConfirm] = useState<{ + count: number; + onConfirm: () => void; + onCancel?: () => void; + } | null>(null); + + // Placement as loaded from the server, keyed by miner: a "row-col" cell or + // "unplaced". The Save delta is this compared against the working set, so a + // miner whose cell never moved is never sent. Membership commits keep it in + // step, entering newcomers as "unplaced" so joining a rack doesn't read as + // pending placement dirt on the Save gate. + const [initialPlacement, setInitialPlacement] = useState>(new Map()); // Loading / error state const [isLoading, setIsLoading] = useState(true); const [loadFailed, setLoadFailed] = useState(false); const [isSaving, setIsSaving] = useState(false); + // `isSaving` lags a render behind, so the ref is what actually keeps two + // writes — a Save and a picker's membership commit — from overlapping. + const savingRef = useRef(false); + // How many miners the server refused to strip, handed from the failed attempt + // to the confirmation prompt. A ref, not state: nothing renders from it — the + // dialog reads the count off reparentConfirm. + const pendingConflictCountRef = useRef(0); const [errorMsg, setErrorMsg] = useState(""); // Load the rack's members and slots. @@ -173,6 +199,10 @@ export default function ManageRackModal({ } setSlotAssignments(assignments); setManualAssignmentCache(assignments); + + const placed = new Map(); + for (const [key, deviceId] of Object.entries(assignments)) placed.set(deviceId, key); + setInitialPlacement(new Map(members.map((id) => [id, placed.get(id) ?? "unplaced"]))); setIsLoading(false); }; @@ -390,15 +420,124 @@ export default function ManageRackModal({ setReparentConfirm({ count, onConfirm: proceed }); }, []); - // SearchMinersModal confirm — add miner to rack and assign to selected slot. - // The modal reports the reassignment flag from the row it selected (exact even - // for fleets larger than the display page). + // Same dialog, awaited: the server rejects assigning a placed miner into a + // site-less rack and writes nothing, so the retry-with-force has to wait for + // an answer inside the write it interrupted. + const confirmSiteStrip = useCallback( + (count: number) => + new Promise((resolve) => { + setReparentConfirm({ + count, + onConfirm: () => resolve(true), + onCancel: () => resolve(false), + }); + }), + [], + ); + + // One AssignDevicesToRack. Unset targetRackId unassigns; set assigns, and + // `slots` rides along in the same transaction. Resolves "conflict" when the + // server refused a site strip (nothing was written). + const dispatchAssign = useCallback( + (deviceIdentifiers: string[], targetRackId: bigint | undefined, force: boolean, slots?: RackSlot[]) => + new Promise<"ok" | "conflict">((resolve, reject) => { + void assignDevicesToRack({ + targetRackId, + deviceIdentifiers, + slotAssignments: slots, + forceClearConflictingSite: force, + onSuccess: () => resolve("ok"), + onConflicts: (conflicts) => { + pendingConflictCountRef.current = conflicts.length; + resolve("conflict"); + }, + onError: (msg) => reject(new Error(msg)), + }); + }), + [assignDevicesToRack], + ); + + // Runs a write, and on a site-strip refusal asks the operator and retries with + // the strip forced. Shared by the membership commits and the placement Save so + // the confirmation behaves identically wherever the refusal surfaces. + const dispatchWithSiteStripConfirm = useCallback( + async (deviceIdentifiers: string[], targetRackId: bigint | undefined, slots?: RackSlot[]): Promise => { + if ((await dispatchAssign(deviceIdentifiers, targetRackId, false, slots)) === "ok") return true; + if (!(await confirmSiteStrip(pendingConflictCountRef.current))) return false; + return (await dispatchAssign(deviceIdentifiers, targetRackId, true, slots)) === "ok"; + }, + [dispatchAssign, confirmSiteStrip], + ); + + // Membership commit. The miner pickers own rack membership, so a confirmed + // selection is written straight away and only placement stays staged for Save. + // Newcomers go in without a slot and land in the load-time snapshot as + // "unplaced", so freshly-committed membership doesn't read as pending + // placement dirt on the Save gate. + // + // The caller owns the working-set update — each entry point merges rows + // differently — and should skip it when this returns false. + const commitMembership = useCallback( + async (added: string[], removed: string[]): Promise => { + if (savingRef.current) return false; + const currentIds = new Set(rackMiners); + const newcomers = added.filter((id) => !currentIds.has(id)); + const leavers = removed.filter((id) => currentIds.has(id)); + // Nothing to write. The caller may still have a placement change to stage + // (e.g. re-placing a miner that's already a member). + if (newcomers.length === 0 && leavers.length === 0) return true; + + // Capacity guard — mirrors the server's. Membership is what fills a rack, + // so this is the check that has to happen here rather than on Save. + const nextCount = rackMiners.length + newcomers.length - leavers.length; + if (totalSlots > 0 && nextCount > totalSlots) { + setErrorMsg( + `Cannot hold ${nextCount} miners with only ${totalSlots} available slots. Deselect some miners or update your rack settings.`, + ); + return false; + } + + savingRef.current = true; + setErrorMsg(""); + setIsSaving(true); + try { + // Removals first: they free their slots before any newcomer lands. An + // unassign has no target rack to strip a site for, so its conflict + // branch never fires — it goes through the same helper for uniformity. + if (leavers.length > 0 && !(await dispatchWithSiteStripConfirm(leavers, undefined))) return false; + if (newcomers.length > 0 && !(await dispatchWithSiteStripConfirm(newcomers, existingRackId))) return false; + } catch (err) { + setErrorMsg(getErrorMessage(err, "Failed to update the rack's miners. Please try again.")); + return false; + } finally { + savingRef.current = false; + setIsSaving(false); + } + + setInitialPlacement((prev) => { + const next = new Map(prev); + for (const id of leavers) next.delete(id); + for (const id of newcomers) next.set(id, "unplaced"); + return next; + }); + // Member counts changed on the host's rack list. + onSettingsPersisted?.(); + return true; + }, + [rackMiners, totalSlots, existingRackId, dispatchWithSiteStripConfirm, onSettingsPersisted], + ); + + // SearchMinersModal confirm — commit the miner into the rack if it isn't a + // member yet, then stage it at the selected slot. The modal reports the + // reassignment flag from the row it selected (exact even for fleets larger + // than the display page). const handleSearchMinerConfirm = useCallback( (minerId: string, isReassignment: boolean) => { if (!selectedSlot) return; const slotKey = selectedSlot.key; - promptReparent(isReassignment ? 1 : 0, () => { - // Add miner to rack if not already present + const apply = async () => { + // Failure leaves the picker open, with the error behind it, to retry. + if (!(await commitMembership([minerId], []))) return; setRackMiners((prev) => (prev.includes(minerId) ? prev : [...prev, minerId])); // Remove any existing assignment for this miner, then assign to selected slot setSlotAssignments((prev) => { @@ -408,16 +547,21 @@ export default function ManageRackModal({ }); setSelectedSlot(null); setShowSearchMiners(false); - }); + }; + promptReparent(isReassignment ? 1 : 0, () => void apply()); }, - [selectedSlot, promptReparent], + [selectedSlot, promptReparent, commitMembership], ); const handleScanMinerAssign = useCallback( - (minerId: string): ScanAssignmentResult | null => { + async (minerId: string): Promise => { if (!selectedSlot) return null; - const previousRackMiners = rackMiners; + const wasMember = rackMiners.includes(minerId); + // Membership commits before the modal reports the assignment, so the + // "assigned" screen never claims a miner that isn't actually in the rack. + if (!(await commitMembership([minerId], []))) return null; + const previousSlotAssignments = slotAssignments; const assignedSlot = selectedSlot; const nextSlotAssignments = removeAssignmentByValue(slotAssignments, minerId); @@ -426,8 +570,12 @@ export default function ManageRackModal({ setRackMiners((prev) => (prev.includes(minerId) ? prev : [...prev, minerId])); setSlotAssignments(nextSlotAssignments); - scanUndoRef.current = () => { - setRackMiners(previousRackMiners); + // Undo has to reverse the write too, not just the staged cell — but only + // for a miner this scan actually added. One that was already a member + // keeps its membership and just loses the new cell. + scanUndoRef.current = async () => { + if (!wasMember && !(await commitMembership([], [minerId]))) return; + if (!wasMember) setRackMiners((prev) => prev.filter((id) => id !== minerId)); setSlotAssignments(previousSlotAssignments); setSelectedSlot(assignedSlot); }; @@ -437,7 +585,7 @@ export default function ManageRackModal({ hasNextSlot: !!getNextAssignableSlot(assignedSlot, nextSlotAssignments), }; }, - [getNextAssignableSlot, getSlotLabel, rackMiners, selectedSlot, slotAssignments], + [getNextAssignableSlot, getSlotLabel, rackMiners, selectedSlot, slotAssignments, commitMembership], ); // Scanned miners already assigned elsewhere use the same reparent warning as @@ -447,7 +595,8 @@ export default function ManageRackModal({ (minerId: string, isReassignment: boolean) => { if (!selectedSlot) return; const slotKey = selectedSlot.key; - promptReparent(isReassignment ? 1 : 0, () => { + const apply = async () => { + if (!(await commitMembership([minerId], []))) return; setRackMiners((prev) => (prev.includes(minerId) ? prev : [...prev, minerId])); setSlotAssignments((prev) => { const next = removeAssignmentByValue(prev, minerId); @@ -457,13 +606,14 @@ export default function ManageRackModal({ setSelectedSlot(null); setShowScanQr(false); scanUndoRef.current = null; - }); + }; + promptReparent(isReassignment ? 1 : 0, () => void apply()); }, - [selectedSlot, promptReparent], + [selectedSlot, promptReparent, commitMembership], ); - const handleScanAssignmentUndo = useCallback(() => { - scanUndoRef.current?.(); + const handleScanAssignmentUndo = useCallback(async () => { + await scanUndoRef.current?.(); scanUndoRef.current = null; }, []); @@ -505,15 +655,18 @@ export default function ManageRackModal({ setSelectedMinerId(null); }, []); - // Remove miner from rack + // Row-level "Remove from rack" — an immediate unassign (the miner keeps + // existing; it just leaves the rack). The row drops only once the write lands, + // so a failure leaves the list truthful. const handleRemoveMiner = useCallback( - (deviceId: string) => { + async (deviceId: string) => { + if (!(await commitMembership([], [deviceId]))) return; setRackMiners((prev) => prev.filter((id) => id !== deviceId)); setSlotAssignments((prev) => removeAssignmentByValue(prev, deviceId)); setManualAssignmentCache((prev) => removeAssignmentByValue(prev, deviceId)); if (selectedMinerId === deviceId) setSelectedMinerId(null); }, - [selectedMinerId], + [selectedMinerId, commitMembership], ); // Unassign miner from slot (keep in rack) @@ -563,18 +716,24 @@ export default function ManageRackModal({ return `Cannot add ${finalIds.length} miners with only ${totalSlots} available slots. Deselect some miners or update your rack settings.`; } - promptReparent(reassignedCount, () => { + // The picker owns membership, so the delta is written here rather than + // staged. Accepting the reparent warning is what authorizes that write. + const keepSet = new Set(finalIds); + const removed = rackMiners.filter((id) => !keepSet.has(id)); + const apply = async () => { + // Failure leaves the picker open with the selection intact to retry. + if (!(await commitMembership(finalIds, removed))) return; setRackMiners(finalIds); setShowManageMiners(false); // Remove assignments for miners no longer in rack - const keepSet = new Set(finalIds); setSlotAssignments((prev) => filterAssignmentsByValues(prev, keepSet)); setManualAssignmentCache((prev) => filterAssignmentsByValues(prev, keepSet)); - }); + }; + promptReparent(reassignedCount, () => void apply()); return undefined; }, - [eligibility, totalSlots, promptReparent], + [eligibility, totalSlots, rackMiners, promptReparent, commitMembership], ); // Rack Settings "Save" handler: persists label/zone/dimensions AND placement @@ -650,122 +809,70 @@ export default function ManageRackModal({ [existingRackId, canManagePlacement, rackSettings, updateRack, onSettingsPersisted], ); - // Save handler — single atomic RPC - const handleSave = useCallback(async () => { - // Capacity guard. handleManageMinersConfirm enforces this when miners are - // added through the sub-modal, but shrinking the layout in Rack Settings - // can leave an already-loaded rack over its new capacity. - if (rackMiners.length > totalSlots) { - setErrorMsg( - `Cannot add ${rackMiners.length} miners with only ${totalSlots} available slots. Deselect some miners or update your rack settings.`, + // One RackSlot per miner whose cell differs from what loaded: position set to + // the new cell, position omitted to clear it. Miners the operator never moved + // aren't named at all, so a concurrent placement change elsewhere in the rack + // survives this save — the whole reason Save no longer goes through SaveRack, + // which replaced the rack's entire member set from a possibly-stale snapshot. + const placementDelta = useMemo(() => { + const cellByMiner = new Map(); + for (const [key, deviceId] of Object.entries(activeAssignments)) cellByMiner.set(deviceId, key); + + const delta: RackSlot[] = []; + for (const deviceId of rackMiners) { + const before = initialPlacement.get(deviceId) ?? "unplaced"; + const after = cellByMiner.get(deviceId) ?? "unplaced"; + if (before === after) continue; + if (after === "unplaced") { + delta.push(create(RackSlotSchema, { deviceIdentifier: deviceId })); + continue; + } + const [row, column] = after.split("-").map(Number); + delta.push( + create(RackSlotSchema, { + deviceIdentifier: deviceId, + position: create(RackSlotPositionSchema, { row, column }), + }), ); + } + return delta; + }, [activeAssignments, rackMiners, initialPlacement]); + + // Save owns slot placement only. Membership commits in the pickers, and + // label/zone/dimensions/site/building commit in Rack Settings — so by the time + // the operator gets here the only unwritten thing left is where each miner + // sits in the grid. + const handleSave = useCallback(async () => { + if (savingRef.current) return; + // Defensive: the CTA is dirty-gated, so a clean save shouldn't be reachable. + // Close rather than write nothing and toast as though something changed. + if (placementDelta.length === 0) { + onSave(); return; } + savingRef.current = true; setIsSaving(true); setErrorMsg(""); - try { - // Build slot assignments from the active assignments map - const slotAssignmentsList = Object.entries(activeAssignments).map(([key, deviceId]) => { - const [row, col] = key.split("-").map(Number); - return { deviceIdentifier: deviceId, row, column: col }; - }); - - // Save is miners-only, but SaveRack always rewrites rack_info, so re-send - // the rack's CURRENT server metadata rather than the modal's cached copy — - // otherwise a concurrent zone/dimension edit from another session would be - // reverted (and stale dims could mis-validate the new slots). Best-effort: - // fall back to the cached values on a fetch miss so a transient error - // can't block the miner save. - let meta = { - label: rackSettings.label, - zone: rackSettings.zone, - rows: rackSettings.rows, - columns: rackSettings.columns, - orderIndex: rackSettings.orderIndex, - coolingType: rackSettings.coolingType, - }; - await new Promise((resolve) => { - getDeviceSet({ - deviceSetId: existingRackId, - onSuccess: (ds) => { - if (ds.typeDetails.case === "rackInfo") { - const ri = ds.typeDetails.value; - meta = { - label: ds.label, - zone: ri.zone, - rows: ri.rows, - columns: ri.columns, - orderIndex: ri.orderIndex, - coolingType: ri.coolingType, - }; - } - resolve(); - }, - onNotFound: () => resolve(), - onError: () => resolve(), - }); - }); - - const finishSuccess = () => { - pushToast({ message: `Rack "${meta.label}" updated`, status: STATUSES.success }); - onSave(); - }; - - // Persist the miners. When the rack is site-less, the server rejects a - // member that currently has a site/building (it would be stripped) and - // returns a conflict list without writing — mirroring the reparent RPC. - // We surface the same ReparentWarningDialog and, on confirm, retry with - // forceClearConflictingSite so the strip is explicit, not silent. - const runSaveRack = (force: boolean): Promise<"ok" | "conflict"> => - new Promise((resolve, reject) => { - saveRack({ - deviceSetId: existingRackId, - label: meta.label, - zone: meta.zone, - rows: meta.rows, - columns: meta.columns, - orderIndex: meta.orderIndex, - coolingType: meta.coolingType, - deviceIdentifiers: rackMiners, - slotAssignments: slotAssignmentsList, - // Placement is omitted: it's already persisted by the Rack Settings - // Save, so leaving it off preserves the rack's current server - // placement and can't clobber a move another session made while this - // modal was open. - forceClearConflictingSite: force, - onSuccess: () => resolve("ok"), - onConflicts: (conflicts) => { - setReparentConfirm({ - count: conflicts.length, - onConfirm: () => { - setReparentConfirm(null); - setIsSaving(true); - setErrorMsg(""); - runSaveRack(true) - .then((outcome) => { - if (outcome === "ok") finishSuccess(); - }) - .catch((err) => setErrorMsg(getErrorMessage(err, "Failed to save. Please try again."))) - .finally(() => setIsSaving(false)); - }, - }); - resolve("conflict"); - }, - onError: (msg) => reject(new Error(msg)), - }); - }); - - // conflict → the dialog above drives the confirm/force retry; don't toast - // success or close the modal until that resolves. - if ((await runSaveRack(false)) === "ok") finishSuccess(); + // Every named miner is already a member; re-asserting membership is a + // no-op server-side (and is what lets the slots ride the same + // transaction), so this cannot move a miner between racks. + const ok = await dispatchWithSiteStripConfirm( + placementDelta.map((slot) => slot.deviceIdentifier), + existingRackId, + placementDelta, + ); + if (!ok) return; + pushToast({ message: `Miner positions saved for "${rackSettings.label}"`, status: STATUSES.success }); + onSave(); } catch (err) { setErrorMsg(getErrorMessage(err, "Failed to save. Please try again.")); } finally { + savingRef.current = false; setIsSaving(false); } - }, [existingRackId, rackSettings, rackMiners, totalSlots, activeAssignments, getDeviceSet, saveRack, onSave]); + }, [placementDelta, existingRackId, rackSettings.label, dispatchWithSiteStripConfirm, onSave]); if (!show) return null; @@ -797,9 +904,11 @@ export default function ManageRackModal({ onClick: () => setShowManageMiners(true), }, { + // Placement is all that's left to write, so the gate is the exact + // delta the request carries — no diff, nothing to save. text: isSaving ? "Saving..." : "Save", variant: variants.primary, - disabled: isSaving || isLoading || loadFailed, + disabled: isSaving || isLoading || loadFailed || placementDelta.length === 0, loading: isSaving, onClick: handleSave, }, @@ -887,6 +996,7 @@ export default function ManageRackModal({ targetRackLabel={rackSettings.label} maxSlots={totalSlots} scope={scope} + saving={isSaving} onDismiss={() => setShowManageMiners(false)} onConfirm={handleManageMinersConfirm} /> @@ -928,7 +1038,11 @@ export default function ManageRackModal({ setReparentConfirm(null)} + onCancel={() => { + const abandon = reparentConfirm.onCancel; + setReparentConfirm(null); + abandon?.(); + }} onConfirm={() => { const proceed = reparentConfirm.onConfirm; setReparentConfirm(null); diff --git a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/MinersPane.tsx b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/MinersPane.tsx index 18edd7eeed..7e159c8b7a 100644 --- a/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/MinersPane.tsx +++ b/client/src/protoFleet/features/fleetManagement/components/ManageRackModal/MinersPane.tsx @@ -68,6 +68,9 @@ function MinerRow({ onBlinkLED: (deviceId: string) => void; }) { const name = miner?.name; + // Miners outside the first display page have no cached snapshot, so fall back + // to the identifier rather than labelling a control "for undefined". + const label = name || deviceId; const ipAddress = miner?.ipAddress; const macAddress = miner?.macAddress; const model = miner?.model; @@ -161,6 +164,7 @@ function MinerRow({ {isAssigned ? (