Conversation
…d View racks (#274) Wires the `?building=<id>` URL contract through the rack list and re-enables the "View racks" link from `/buildings/:id`. Backend filter (building_ids / include_no_building on ListDeviceSetsRequest) already shipped in #229; this PR is the FE side. - RacksPage: parses `?building=<id>` (repeated + legacy comma-joined), loads ListAllBuildings, registers a Building chip in FilterChipsBar with the building name, treats the URL as the source of truth so back/forward + chip removal stay in sync. - useDeviceSets / useDeviceSetListState: thread buildingIds through listRacks → ListDeviceSets. - BuildingPageHeader: re-enables the disabled "View racks" button, linking to /racks?building=<id> (mirrors the existing "View miners" contract). - MinerList.handleServerFilter: also translates dropdownFilters.building → MinerListFilter.buildingIds (was silently dropped, stripping ?building= from the URL on initial mount). FILTER_AND_SORT_KEYS now includes "building" so the saved-views machinery canonicalizes building-scoped URLs as filtered state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
🔐 Codex Security Review
Review SummaryOverall Risk: MEDIUM Findings[MEDIUM] Miner building filter is applied without a visible filter control
NotesNo auth, SQL, command execution, plugin, infrastructure, protobuf wire-format, or mining pool/payout hijack issues were introduced in the reviewed diff. I did not run tests; this was a scoped diff review of Generated by Codex Security Review | |
There was a problem hiding this comment.
Pull request overview
This PR wires the existing ?building=<id> URL filter contract into the racks list, re-enables the “View racks” navigation from /buildings/:id, and ensures the miner list preserves building-scoped deep links by translating the building dropdown filter into MinerListFilter.buildingIds.
Changes:
- Add building-aware rack listing by parsing
?building=on/racks, loading building names for the filter chip, and passingbuildingIdsthrough toListDeviceSets. - Re-enable
/buildings/:id→/racks?building=…navigation via a “View racks” link. - Fix miner list deep-link canonicalization by mapping
dropdownFilters.building→MinerListFilter.buildingIdsand whitelistingbuildingin saved-view URL canonicalization.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/src/protoFleet/hooks/useDeviceSetListState.ts | Threads optional buildingIds into paginated rack listing requests. |
| client/src/protoFleet/features/rackManagement/pages/RacksPage.tsx | Parses ?building= from URL, loads building options, and binds a Building filter chip to URL state. |
| client/src/protoFleet/features/fleetManagement/views/savedViews.ts | Treats building as a canonical filter/sort key so deep links persist as filtered state. |
| client/src/protoFleet/features/fleetManagement/components/MinerList/MinerList.tsx | Translates building dropdown selections into minerFilter.buildingIds. |
| client/src/protoFleet/features/buildings/components/BuildingPageHeader.tsx | Re-enables “View racks” navigation using the building URL param. |
| client/src/protoFleet/api/useDeviceSets.ts | Passes buildingIds through in listRacks requests to the backend. |
Comments suppressed due to low confidence (1)
client/src/protoFleet/features/rackManagement/pages/RacksPage.tsx:242
handleClearFilterscallssetBuildingFilter([])and then immediatelyresetAndFetch(). Because the building filter is URL-driven andselectedBuildingIdsRefis updated in an effect, this first fetch can still run with the old buildingIds (and then the separate URL-change effect triggers another fetch). This can briefly show stale, still-building-filtered results and does an extra round-trip.
Consider making the clear-all path update the buildingIds ref synchronously (or set it to [] before fetching), or remove the direct resetAndFetch() here and instead have a single refetch effect that responds to all filter changes (building/zone/issues) so the fetch always uses the latest filter state once per change.
const handleClearFilters = useCallback(() => {
setSelectedZones([]);
selectedZonesRef.current = [];
setSelectedIssues([]);
selectedIssuesRef.current = [];
setBuildingFilter([]);
resetAndFetch();
}, [resetAndFetch, selectedIssuesRef, selectedZonesRef, setBuildingFilter]);
#225 added direct imports of otel/metric, otlpmetricgrpc, sdk/metric, and proto/otlp under server/internal/infrastructure/metrics/ but committed go.mod with those modules still in the // indirect block. `go mod tidy` promotes them to direct and bumps docker/go-connections 0.6.0 → 0.7.0 as a transitive resolution. Unrelated to #274; folded in here to unblock the tree. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10cdd3ca8
ℹ️ 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".
Addresses three findings from PR #285 review: 1. RacksPage filter-empty state (Codex P2). `hasEverLoaded` only flips when `total > 0`, so deep-linking `/racks?building=<id>` where the building has zero racks (but the org has racks elsewhere) fell through to the "You haven't set up any racks" null state instead of the filtered-empty state. Including `hasActiveFilters` in the `hasRacks` check short-circuits the null state whenever a filter is active. 2. `handleClearFilters` double-fetch (Copilot suppressed). When the building filter was active, clearing all triggered one synchronous resetAndFetch with stale ref values plus a second via the URL-change effect. Snapshot the building-active state up front; skip the explicit resetAndFetch when the URL-change effect will handle the refetch. 3. No FE tests for the URL parse (me + Copilot). Extracted `parseBuildingIdsFromParams` + `BUILDING_URL_PARAM` to a new `rackManagement/utils/buildingFilterUrl.ts` module and added a vitest covering repeated keys, legacy comma-joined values, invalid input, ordering, and large bigint ids. Follow-up issue #291 tracks the pre-existing `<Link><Button/></Link>` nesting on BuildingPageHeader (PR review finding #2) — fixing both View racks and View miners together rather than mixing scope into this PR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Review polish — 1425dabAddressed three of the four review findings; filed #291 for the fourth.
Deferred chip-name resolution for the miner-list still tracked on #202. Local verification: |
negarn
left a comment
There was a problem hiding this comment.
looks good! just wondering if you meant to include the server/ changes in this PR
|
ya i think that dependency tidying was missed in a previous PR so i just included it. |
Summary
Wires the
?building=<id>URL contract through the rack list and re-enables the View racks affordance on/buildings/:id(was disabled in #269 pending this work). Backendbuilding_ids/include_no_buildingfilter onListDeviceSetsRequestalready shipped in #229 / #198; this PR is the FE side.Closes #274.
Changes
RacksPage.tsx— parses?building=<id>from URL (accepts repeated-key + legacy comma-joined forms, matchesfilterUrlParams.ts), loads all buildings vialistAllBuildings, registers a Building filter inFilterChipsBarwith the building name. URL is the source of truth — back/forward navigation and chip removal stay in sync.useDeviceSets.ts/useDeviceSetListState.ts— threadbuildingIdsthroughlistRacks→ListDeviceSets.BuildingPageHeader.tsx— replaces the disabledView racksbutton + tooltip with<Link to="/racks?building=…">. Mirrors the existingView minersURL contract.MinerList.handleServerFilter(drive-by, fixes regression surfaced by this PR's deep-link) — also translatesdropdownFilters.building→MinerListFilter.buildingIds. Without this, the building filter was silently dropped on mount and the URL was rewritten without?building=.savedViews.FILTER_AND_SORT_KEYSnow includes"building"so building-scoped URLs canonicalize as filtered state (instead of "All miners").Deferred
?building=<id>end-to-end, but the FilterChipsBar dropdown sources don't include aBuildingentry, so the chip renders the raw id (10) rather than the building name (Bldg-A-23924) and there's no UI affordance to clear it. Punted to Multi-site — Phase 1b: miner/rack list active-site consumption #202 (Phase 1b miner/rack list active-site consumption) since the fix needslistAllBuildingsplumbed intoFleet.tsx+ a newDropdownFilterIteminMinerList.tsx— naturally overlaps with the site-picker consumption work tracked there. Tracking comment: Multi-site — Phase 1b: miner/rack list active-site consumption #202 (comment)Test plan
/buildings/:id, click View racks — lands on/racks?building=<id>./racksrenders the building name, removable like other filters; clearing the chip strips?building=from the URL./buildings/:id, click View miners — lands on/miners?building=<id>and the URL stays intact (no on-mount strip).just lint,tsc --noEmit,vitest run(full client suite), Go tests forinternal/handlers/deviceset+internal/domain/collectionall pass locally.🤖 Generated with Claude Code