Conversation
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 <noreply@anthropic.com>
…an't lie Save previously fired unconditionally. With no pending placement change every dispatch short-circuited on its empty bucket, so zero RPCs went out — but the handler still pushed `Building "X" saved`, called onSaved, and closed. The operator saw a successful save that never happened. Extract the snapshot diff into `buildPlacementDelta` in assignmentMath so the CTA's dirty gate and the dispatch read the same buckets; a gate computed separately could drift and either block a real change or let a no-op through. The load-time snapshot moves from a ref to state because the gate derives from it — a ref wouldn't re-derive when the load resolves. Save is now disabled when the delta is empty, with `isLoading` kept in the gate so a not-yet-loaded baseline can't read clean for the wrong reason. handleSave also bails on an empty delta defensively rather than falling through to the toast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…they write Site-level modals split so each one commits what it owns, and each CTA says whether a write happens: - Create site: "Continue" → "Create site". The site is persisted by this click (CreateSite), so the label names the write rather than implying a step. - Manage buildings picker: "Continue" → "Save", and it now applies the membership delta via AssignBuildingsToSite instead of staging it into ManageSiteModal. Leaving via "New building" therefore abandons the pending selection — which is the point: without a Save, nothing was written. - Row-level "Remove building" unassigns immediately instead of queueing behind a Save, and the row only disappears once the write lands. - ManageSiteModal's Save now owns nothing but building placement, which lands in #263. Left inert and deliberately toast-free until then. Diff-gating on the writing CTAs, each derived from the same value the write sends so the gate can't drift from it: - Picker Save is gated on computeBuildingSelectionDelta being non-empty. Note that isn't a plain set-difference — seeded ids the response omitted, and rows that became ineligible, are excluded on purpose — so a raw selection comparison would read dirty with nothing to send. - Site settings Save (edit) is gated on a normalized comparison against initialValues, so trailing whitespace or a capacity retyped as "8.0" doesn't count. Create has no baseline and stays validation-gated. Both gates stay closed while their baseline is still loading, so nothing reads clean for the wrong reason. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…writes Mirrors the site-level split: each modal commits what it owns, and each CTA says whether a write happens. - Manage racks picker: "Continue" → "Save", and it now writes the membership delta via AssignRacksToBuilding instead of staging it. Accepting the reparent warning is what authorizes that write, so a reparent commits on confirm rather than riding a later Save. - Row-level "Remove rack" unassigns immediately; the row drops only once the write lands, so a failure leaves the list truthful. - Search racks "Assign" commits membership for a rack that isn't a member yet, then stages the chosen cell. Already a write-verb, already single-select gated, so the label and gate stand. - ManageBuildingModal's Save now owns rack placement only. Membership commits fire onSaved themselves, so a dismiss has nothing left to reconcile, and the success toast says "Rack positions saved" rather than implying the whole building was written. - Building settings: create CTA → "Create building"; edit Save is dirty-gated on a normalized comparison against initialValues, so "5" → "5.0" isn't an edit. Create has no baseline and stays validation-gated. The chunked AssignRacksToBuilding dispatcher is now shared between the membership commits and the placement Save. Membership commits carry the same capacity guard the Save had, and they update the load-time snapshot so freshly-committed racks read as "unplaced" rather than as pending placement dirt on the Save gate. The picker's Save gate is derived from computeRackSelectionDelta — the same value the write sends. That matters because it isn't a plain set-difference: seeded ids the response omitted are excluded on purpose, so comparing selections directly would report dirty with nothing to send. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AssignDevicesToRack was AssignRacksToBuilding minus the placement fields — membership only, no way to say where in the grid a miner lands. That left SaveRack as the only batch placement write, and SaveRack replaces the rack's entire member set, so any placement edit had to re-assert membership from the client's cached snapshot. ManageRackModal already pays for that twice by hand: it omits placement on an edit Save so it "can't clobber a move made by another session", and it refetches rack_info before saving so "a concurrent zone/dimension edit from another session" isn't reverted. Membership was the one concern with no such guard — a stale list silently drops miners another session added. Add slot_assignments to AssignDevicesToRackRequest, mirroring buildings.v1.RackPlacement one level down: the batch names only the children it changes, placement optional per child. Empty leaves slots untouched, so the importer, the CLI and the overview page's assign-then-place pair are unaffected. Non-empty is authoritative for the named devices — an entry places, no entry unplaces — and every named device is cleared before any position is set, which is what lets one call swap two occupied cells without tripping uk_rack_slot_position. Also add the rack capacity guard SaveRack already enforces. A rack has no floating members, so a delta must not push membership past rows×columns; prior count + newly-inserted count is exact, so re-asserting existing members can't read as an overflow. Co-Authored-By: Claude <noreply@anthropic.com>
No caller has ever passed `rack=`, so `isEditMode` was permanently false and everything behind it was dead: the internal UpdateDeviceSet call, the "Save" label branch, the `onSuccess` callback, and the `rack?.label` / `rackInfo?.*` seed fallbacks. The error Callout went with it — the only `setErrorMsg` that could fire lived in that block, so it could never render. The two live entry points both drive this modal through `onContinue` (RacksPage / FleetCreateFlowProvider for create, ManageRackModal for a settings edit), which means the caller already owns the write. Making that the only shape is what lets the next commits move the create up front without threading a second write path through here. Behavior is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
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 <noreply@anthropic.com>
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.
…cement 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 <noreply@anthropic.com>
The rack flow no longer defers its writes to one final Save, so the page object's CTA helpers were naming steps that don't exist: - clickContinueFromRackSettings -> clickCreateRackFromSettings / clickSaveRackSettings. The Rack settings CTA creates the rack (or updates an existing one), so the `Rack "X" created` toast is now asserted there rather than after the miner save. - clickContinueInMinerSelector -> clickSaveInMinerSelector. The picker commits membership itself. - clickSaveRack -> clickSaveMinerPositions, asserted with the new `Miner positions saved for "X"` toast. Two setups pressed the final Save with nothing placed. That button is now disabled, so the rack-overview-search spec asserts the disabled state instead, and the RBAC setup just dismisses the modal — the rack it needs already exists by then. The overview-actions spec read the rack's members off a SaveRack request. That call is gone; membership arrives on the first AssignDevicesToRack instead. Also gives the rack settings Save a `Rack "X" saved` toast. It wrote silently while the building settings Save has toasted `Building "X" saved` since the building split, and the order-index spec had no signal that its edit persisted. Co-Authored-By: Claude <noreply@anthropic.com>
…tinue" Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
Bot
added
javascript
github-actions
Bot
added
the
review-policy: needs-review
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89bbe02af9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+507
to
+508
| if (leavers.length > 0 && !(await dispatchWithSiteStripConfirm(leavers, undefined))) return false; | ||
| if (newcomers.length > 0 && !(await dispatchWithSiteStripConfirm(newcomers, existingRackId))) return false; |
There was a problem hiding this comment.
When the Manage Miners picker changes the rack membership by both removing existing miners and adding newcomers, this unassign call commits first and the add call runs afterward. If the second call fails or the user cancels the site-strip confirmation, the function returns false and leaves the picker open as though no change landed, but the removed miners have already been cleared from the rack; this regresses the prior single SaveRack transaction for replacement edits. Keep the replacement atomic, or reload/restore state after a partial commit so users do not unknowingly lose rack memberships.
Useful? React with 👍 / 👎.
Comment on lines
+417
to
+421
| await dispatchAssign( | ||
| removed.map((rackId) => ({ rackId })), | ||
| undefined, | ||
| ); | ||
| await dispatchAssign( |
There was a problem hiding this comment.
When the Manage Racks picker simultaneously removes racks from this building and adds other racks, this removal dispatch can commit before the newcomer dispatch starts. If the add batch fails (for example because the target building changed capacity or a later chunk errors), the modal leaves the picker open for retry without updating its local entries, but the removed racks have already been unassigned server-side; keep the membership replacement atomic or refresh/reflect the partial commit before letting the user retry.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the fleet “hierarchy” write paths so CTAs more explicitly reflect the server-side writes they trigger, and adds a delta-style slot_assignments capability to AssignDevicesToRack so rack membership + placement can be persisted atomically without the replace-all semantics of SaveRack.
Changes:
- Server: add
slot_assignmentssupport toAssignDevicesToRack(wire → handler → domain), including validation, capacity guarding, and slot write ordering (clear-then-set). - Client: refactor Site/Rack/Building management modals so membership writes commit in the picker modals (Save), while outer modals primarily stage/commit placement; update CTA labels accordingly.
- Tests: add/adjust unit + E2E coverage for the new commit points, dirty gates, and request shapes.
Reviewed changes
Copilot reviewed 45 out of 48 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/internal/handlers/deviceset/translate.go | Maps slot_assignments from RPC request into domain params. |
| server/internal/handlers/deviceset/handler_test.go | Adds handler-level coverage ensuring slot assignments are forwarded and validated. |
| server/internal/domain/collection/service.go | Implements slot assignment validation + in-tx bounds/capacity checks + slot delta persistence. |
| server/internal/domain/collection/service_test.go | Adds domain-level tests for slot delta semantics, bounds/capacity guards, and ordering. |
| server/generated/grpc/device_set/v1/device_setv1connect/device_set.connect.go | Generated client/handler comments updated for the new RPC semantics (generated — skip). |
| proto/device_set/v1/device_set.proto | Documents slot_assignments semantics and constraints on the RPC. |
| client/src/protoFleet/features/sites/hooks/useSiteModals.ts | Moves site create earlier (“Continue” commits CreateSite) and adds commit-style building operations. |
| client/src/protoFleet/features/sites/hooks/useSiteModals.test.ts | Updates hook tests for create/edit flow changes and new building operations. |
| client/src/protoFleet/features/sites/components/SiteSettingsModal/SiteSettingsModal.tsx | Renames create CTA (“Create site”) and adds edit-mode dirty gating. |
| client/src/protoFleet/features/sites/components/SiteSettingsModal/SiteSettingsModal.test.tsx | Adds tests for dirty gating and updated behaviors/labels. |
| client/src/protoFleet/features/sites/components/SiteModals/SiteModals.tsx | Simplifies modal stacking now that manage modal always targets a persisted site. |
| client/src/protoFleet/features/sites/components/SiteModals/SiteModals.test.tsx | Updates expectations for delete behavior and building hook mocking. |
| client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.tsx | Changes building membership to commit immediately; adds inline building-create handoff. |
| client/src/protoFleet/features/sites/components/ManageSiteModal/ManageSiteModal.test.tsx | Adds tests for inline building create and immediate unassign behavior. |
| client/src/protoFleet/features/sites/components/ManageSiteModal/index.ts | Removes exported create/edit mode type that no longer applies. |
| client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.tsx | Adds “New building” handoff + Save dirty gate + saving-state behavior. |
| client/src/protoFleet/features/sites/components/ManageBuildingsModal/ManageBuildingsModal.test.tsx | New unit tests for “New building” handoff and Save gating. |
| client/src/protoFleet/features/fleetManagement/pages/RacksPage.tsx | Uses useCreateRack; rack settings now creates the rack before opening ManageRackModal. |
| client/src/protoFleet/features/fleetManagement/pages/RackOverviewPage.tsx | Updates copy to reflect per-step persistence and background refresh semantics. |
| client/src/protoFleet/features/fleetManagement/hooks/useCreateRack.ts | New shared hook to create racks via SaveRack with conflict confirmation handling. |
| client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.tsx | Renames submit handler, adds dirty gating, delegates persistence to caller, adjusts labels. |
| client/src/protoFleet/features/fleetManagement/components/RackSettingsModal.stories.tsx | Updates story to use new onSubmit prop. |
| client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ScanMinerQrModal.tsx | Makes assign/undo async to ensure UI reflects committed membership actions. |
| client/src/protoFleet/features/fleetManagement/components/ManageRackModal/MinersPane.tsx | Adds aria-labels for miner actions with a safe fallback label. |
| client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageRackModal.test.tsx | Updates tests for assign/placement save semantics and dirty gating. |
| client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.tsx | Renames CTA to Save, adds selection dirty gate and saving prop. |
| client/src/protoFleet/features/fleetManagement/components/ManageRackModal/ManageMinersModal.test.tsx | Updates tests for Save labeling and new dirty gating behavior. |
| client/src/protoFleet/features/fleetManagement/components/FleetCreateFlow/FleetCreateFlowProvider.tsx | Adopts create-first rack flow with conflict dialog; opens manage modal with real rack id. |
| client/src/protoFleet/features/buildings/components/ManageRacksModal/ManageRacksModal.tsx | Renames CTA to Save, adds dirty gating, and supports external saving state. |
| client/src/protoFleet/features/buildings/components/ManageRacksModal/ManageRacksModal.test.tsx | Updates tests for Save gating and revised delta behaviors. |
| client/src/protoFleet/features/buildings/components/ManageBuildingModal/ManageBuildingModal.tsx | Commits membership in pickers; Save now only commits placement with a derived dirty gate. |
| client/src/protoFleet/features/buildings/components/ManageBuildingModal/ManageBuildingModal.test.tsx | Updates reparent tests to commit-on-confirm; adds placement Save dirty gate tests. |
| client/src/protoFleet/features/buildings/components/ManageBuildingModal/assignmentMath.ts | Extracts placement delta computation and emptiness checks. |
| client/src/protoFleet/features/buildings/components/ManageBuildingModal/assignmentMath.test.ts | Adds unit tests for placement delta computation. |
| client/src/protoFleet/features/buildings/components/BuildingSettingsModal/BuildingSettingsModal.tsx | Renames create CTA (“Create building”) and adds edit-mode dirty gating. |
| client/src/protoFleet/features/buildings/components/BuildingSettingsModal/BuildingSettingsModal.test.tsx | Adds tests for edit-mode dirty gating. |
| client/src/protoFleet/api/useDeviceSets.ts | Plumbs slotAssignments through assignDevicesToRack and documents delta semantics. |
| client/e2eTests/protoFleet/spec/racksOverviewActions.spec.ts | Updates E2E flow to observe AssignDevicesToRack and new toasts/buttons. |
| client/e2eTests/protoFleet/spec/racksManualAssignment.spec.ts | Updates E2E flow for create + membership save + placement save split. |
| client/e2eTests/protoFleet/spec/racksManagement.spec.ts | Updates E2E flows for create-first semantics and renamed actions/toasts. |
| client/e2eTests/protoFleet/spec/racksCreation.spec.ts | Updates E2E flows for create-first and updated settings/membership/placement saves. |
| client/e2eTests/protoFleet/pages/racks.ts | Updates page object helpers for renamed CTAs/toasts and new disabled-state asserts. |
| client/e2eTests/protoFleet/helpers/rbacTestSetup.ts | Updates rack creation helper to dismiss manage modal post-create. |
| client/e2eTests/protoFleet/helpers/racksHelpers.ts | Updates miner selector helper to use “Save” instead of “Continue”. |
| client/e2eTests/protoFleet/helpers/buildingsTestSetup.ts | Updates rack creation helper to save placement via the new CTA/toast. |
Comment on lines
+1339
to
+1353
| 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) | ||
| } | ||
| if slot.Position.Column >= targetColumns { | ||
| return nil, fleeterror.NewInvalidArgumentErrorf("slot column %d is out of bounds (rack has %d columns)", slot.Position.Column, targetColumns) | ||
| } | ||
| } | ||
| } |
🔐 Codex Security Review
Review SummaryOverall Risk: HIGH Findings[HIGH] Rack miner replacement can partially unassign miners before a failed add
[HIGH] Placement-only save can silently reparent miners moved by another session
[MEDIUM] Rack-only users now send explicit placement and hit site-manage authorization
NotesI did not find evidence of cryptostealing, pool hijacking, SQL injection, command injection, or protobuf wire-format breakage in the reviewed hunks. The main risks are around the new immediate-commit workflow and reusing reparent RPCs for placement-only saves. Generated by Codex Security Review | |
#263 is closed and its scope was the real BuildingCard/metrics work, not building placement within a site. Placement has no backend and no tracking issue yet, so say that instead of pointing at an unrelated number. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d775e2b9a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+80
to
+81
| siteId: formData.siteId ?? 0n, | ||
| buildingId: formData.buildingId ?? 0n, |
There was a problem hiding this comment.
When a user has rack:manage but not site:manage, RackSettingsModal hides the placement fields and submits formData.siteId/buildingId as undefined; this hook converts that to explicit 0n values. The SaveRack handler treats any non-nil placement field, including 0 for unassign, as placement intent before requiring site:manage, so creating an unplaced rack now fails for rack-only operators. Leave both fields undefined when no placement was chosen/allowed, or pass the caller's placement permission through.
Useful? React with 👍 / 👎.
| // 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; |
There was a problem hiding this comment.
When a QR scan auto-adds a miner that was not already in the rack, the undo callback closes over the pre-add commitMembership and its rackMiners snapshot. On undo, commitMembership([], [minerId]) computes no leaver because that snapshot does not include the miner, returns without sending an unassign RPC, and then the UI removes the row locally, leaving the miner assigned server-side after the modal rescans. Use a live membership ref/state or call the unassign RPC directly for this undo path.
Useful? React with 👍 / 👎.
| // Refresh the site catalog (building counts) but deliberately NOT | ||
| // the modal's building list — the modal injects the new row | ||
| // locally so unsaved picker selections survive. | ||
| refetchSites(); |
There was a problem hiding this comment.
When ManageSiteModal is opened from SiteDetailPage, the visible building cards are refreshed through refetchBuildings/buildingsRefreshKey, but the inline "New building" success path only calls refetchSites(). The modal injects the created row into its own local list, yet after closing the modal the page-level building section remains stale until navigation or manual refresh. Trigger refetchBuildings?.() here as well, separate from the modal-local injection.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewable diff: +1722/-960 across 27 files (excludes generated, test, and story files).
Summary
Every CTA in the Site → Building → Rack hierarchy now means one thing: Continue moves to the next step, Save (or Create X) makes an API call. Previously each level funnelled several unrelated writes through one final Save at the end of a multi-step modal stack, so an operator could stage membership in a picker, walk away, and lose it — or press a Save that wrote nothing and still got a success toast.
The fix is structural, not cosmetic: each modal now commits the one concern it owns, at the moment its CTA is pressed, and every write-CTA is disabled until its own payload actually differs from what loaded. This also closes a lost-update bug at the rack level, where
SaveRackreplaced a rack's entire member set from a possibly-stale client snapshot.Out of scope: the
ManageSiteModalSave is deliberately left inert (it closes without writing, and shows no toast). It owns building placement within a site, which has no backend yet and is not currently tracked by an issue. Rack-placement-within-a-building already ships here.How it works
Each level of the hierarchy has four separable concerns: an entity's own fields, its parent pointer, its child membership, and its children's placement within it. The old flow collected all four across a modal stack and flushed them on one button. The new flow gives each its own commit point.
Site level. The create modal's Continue now calls
CreateSiteimmediately and transitions to the manage surface against the real row — which is what gives inline building-create a realsite_idto attach to. The buildings picker's Save applies its membership delta viaAssignBuildingsToSiteon the spot. The row kebab's "Remove building" unassigns on click. Inline building-create uses the transactionalCreateBuilding(#821), so the new building is associated to the site atomically and the modal injects the returned row into its local list rather than refetching — which preserves any selections still open in the picker.Building level. Same split. The racks picker commits membership; the manage modal's Save owns only rack placement, computed as a delta against a load-time snapshot. Placement dispatches in two passes — every changed rack's old cell is vacated before any new cell is claimed — because a straight swap of two occupied cells would otherwise trip the
uk_device_set_rack_building_positionunique index mid-batch.Rack level. The Rack Settings CTA reads Create rack and creates the rack up front (previously the rack existed only in client state until the final Save). The miner pickers — Manage miners, Select from list, Add by QR scan — each commit membership through
AssignDevicesToRack. The manage modal's Save owns placement only.That last part needed a server change.
SaveRackis a coarse PUT: it replacesdevice_selectorwholesale with no version precondition, so any Save wrote back the membership the modal loaded, silently dropping a miner added concurrently elsewhere.AssignDevicesToRackwas already the delta for membership but had no way to express placement, so the client had no delta-shaped option. This PR gives itslot_assignments, making it the rack-level counterpart toAssignRacksToBuilding:slot_assignmentsentrypositionsetpositionunsetServer-side, every named device is cleared before any position is written, so a relayout can swap two occupied cells in one call without tripping
uk_rack_slot_position. Membership and placement land in the same transaction.The client's Save then sends only the miners whose cell actually changed. A concurrent placement change to a miner the operator never touched survives.
flowchart TD A["Create site modal<br/>CTA: Continue"] -->|"CreateSite"| B["Manage Site modal"] B --> C["Manage buildings picker<br/>CTA: Save"] C -->|"AssignBuildingsToSite (delta)"| B B --> D["Inline building create<br/>CTA: Create building"] D -->|"CreateBuilding (transactional, #821)"| B B -->|"Save: inert, placement lands in #263"| Z["closes"] E["Rack settings modal<br/>CTA: Create rack / Save"] -->|"CreateRack / UpdateDeviceSet"| F["Manage Rack modal"] F --> G["Miner pickers<br/>CTA: Save"] G -->|"AssignDevicesToRack (membership delta)"| F F -->|"Save: AssignDevicesToRack (slot delta only)"| Y["Miner positions saved"]sequenceDiagram participant Op as Operator participant M as ManageRackModal participant S as Server Op->>M: open rack M->>S: GetRackSlots + ListGroupMembers S-->>M: members + placement Note over M: snapshot placement per miner<br/>(cell or "unplaced") Op->>M: Manage miners -> Save M->>S: AssignDevicesToRack (members delta, no slots) S-->>M: ok Note over M: newcomers folded into<br/>snapshot as "unplaced" Op->>M: drag miner to a cell Op->>M: Save Note over M: delta = only miners whose cell moved M->>S: AssignDevicesToRack (targetRackId + slotAssignments) S-->>M: ok (membership + placement, one txn)Areas of the code involved
proto/device_set/v1/device_set.protoslot_assignmentsonAssignDevicesToRackRequest(max 10000)position= unplace) are the cruxserver/internal/domain/collection/service.goserver/internal/handlers/deviceset/translate.goserver/generated/**,client/src/protoFleet/api/generated/**client/src/protoFleet/api/useDeviceSets.tsslotAssignmentsplumbed through theassignDevicesToRackhook.../sites/hooks/useSiteModals.tsmanageSavebecomesmanageAssignBuildings/manageRemoveBuilding/manageCreateBuilding.../sites/components/ManageSiteModal/ManageSiteModal.tsx.../sites/components/{ManageBuildingsModal,SiteSettingsModal,SiteModals}.../buildings/.../ManageBuildingModal.tsxassignmentMath.ts.../buildings/.../assignmentMath.ts(new logic)buildPlacementDelta+isPlacementDeltaEmpty, extracted for unit testing.../fleetManagement/hooks/useCreateRack.ts(new).../fleetManagement/components/RackSettingsModal.tsxisDirty; caller-drivensavingsavingprop exists because the conflict retry re-dispatches afteronSubmitresolved.../ManageRackModal/ManageRackModal.tsxSaveRackgone;commitMembership+placementDelta; server site-strip refusal routed through the existing confirm dialog.../ManageRackModal/{ManageMinersModal,ScanMinerQrModal,MinersPane}.tsxMinersPanelabels were previously only reachable via an unnamed ellipsisclient/e2eTests/protoFleet/**Key technical decisions & trade-offs
AssignDevicesToRackrather than addingSetRackSlots. A separate placement RPC would have made membership+placement two transactions, reintroducing a partial-failure window. Chosen over both that and continuing to useSaveRack.SaveRackis left in place, not deleted. It still has callers; the proto comment now steers new code toAssignDevicesToRack. Deleting it is a larger, separable change.positionmeans unplace, rather than a separateclearflag. MatchesAssignRacksToBuilding's optionalaisle_index. The cost: "unplace" and "don't mention" are distinguished only by presence in the list, which is why the proto comment is explicit.manageCreate*states) and gives child writes a real parent id — at the cost of leaving a persisted row behind if the operator abandons the flow mid-way."unplaced". Without it, adding a miner would leave the placement Save dirty with placement the operator never touched.Testing & validation
src/protoFleet). New coverage for each dirty gate and each commit boundary — including that Save sends only the changed miner with the new position, and that a miner stays in the list when its removal fails to persist.internal/domain/collectionandinternal/handlers/devicesetpass uncached, covering the clear-then-place ordering, bounds checks, and duplicate device/position rejection.tsc --noEmit(includese2eTests), ESLint, andjust formatall clean.protofleet-e2e-tests. This is the main thing to watch on this PR.