Skip to content

Add breadcrumb to rack detail#589

Open
jmarrxyz wants to merge 2 commits into
jmarr/breadcrumb-componentfrom
jmarr/rack-detail-breadcrumb
Open

Add breadcrumb to rack detail#589
jmarrxyz wants to merge 2 commits into
jmarr/breadcrumb-componentfrom
jmarr/rack-detail-breadcrumb

Conversation

@jmarrxyz

Copy link
Copy Markdown
Contributor

Summary

Stack: this PR is based on #587 (jmarr/breadcrumb-component) and only contains the rack-detail integration. It replaces the rack detail header's chevron back button with breadcrumb navigation and preserves the existing rack title, zone subtitle, and action buttons.

How it works

RackOverviewPage loads site and building labels in parallel with the existing rack resolution. Assigned racks render Sites / {site} / {building} / {rack} when building context exists, or Sites / {site} / {rack} for direct-to-site racks; unassigned racks fall back to Racks / {rack}.

Diagrams

flowchart LR
  rack["GetDeviceSet rack"] --> placement["site_id / building_id"]
  sites["ListSites"] --> labels["Ancestor labels"]
  buildings["ListBuildings"] --> labels
  placement --> breadcrumb["Breadcrumb trail"]
  labels --> breadcrumb
Loading

Areas of the code involved

Area / file What changed Why it matters for review
client/src/protoFleet/features/fleetManagement/pages/RackOverviewPage.tsx Adds Breadcrumb and ancestor-label lookups, removes back icon Gives rack detail the same hierarchy as site/building detail
client/src/protoFleet/features/fleetManagement/pages/RackOverviewPage.test.tsx Updates mocks for the new lookup hooks and asserts no header back button Keeps the existing rack subtitle coverage while validating the navigation swap

Key technical decisions & trade-offs

Decision Trade-off
Resolve labels client-side from existing list APIs Avoids backend/API changes, with graceful "Site"/"Building" fallback labels while context loads
Keep rack title outside the breadcrumb Preserves the existing detail-page scan pattern and action layout

Testing & validation

  • Storybook checked locally through Add shared breadcrumb component #587's breadcrumb variants.
  • vitest run src/protoFleet/features/fleetManagement/pages/RackOverviewPage.test.tsx passed.
  • Targeted ESLint passed for the touched rack files.
  • Full client-typecheck is currently blocked by unrelated infra/maintenance TypeScript errors already present on the parent branch.

@jmarrxyz jmarrxyz requested a review from a team as a code owner June 25, 2026 19:35
@github-actions github-actions Bot added javascript Pull requests that update javascript code client labels Jun 25, 2026
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

  • Reviewed pull request diff only (12d782e995a542fa5d91fd6a628e9eee571392c6...5ff787b0949ec8450a8f2cd74dc63aa71430790d, exact PR three-dot diff)
  • Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.


Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] Breadcrumb rack-sibling lookup repeats an org-wide rack scan on every poll

  • Category: Reliability
  • Location: client/src/protoFleet/features/fleetManagement/pages/RackOverviewPage.tsx:230
  • Description: The new breadcrumb sibling effect calls listRacks() without pageSize, so the client helper pages through every rack in the org. Because the effect depends on the whole rack / rackInfo object, the existing 15-second silent rack poll replaces rack with a fresh response object and retriggers this full rack scan continuously while the page is open.
  • Impact: A rack detail page now adds recurring org-wide ListDeviceSets pagination, count queries, response decoding, and client memory churn just to populate breadcrumb siblings. On large fleets or with multiple operators viewing rack pages, this can create unnecessary database and API load and degrade UI responsiveness.
  • Recommendation: Fetch siblings from a stable placement key instead of the full rack object, and scope the request. For building-backed racks, call listRacks({ buildingIds: [buildingId] }); for broader sibling menus, cache/fetch once per rack/placement rather than every poll. Consider adding AbortSignal support to listRacks, and add a fake-timer test that advances POLL_INTERVAL_MS and asserts the sibling lookup is not repeated.

Notes

No auth, SQL injection, command injection, cryptostealing/pool hijack, protobuf wire-format, or XSS issues were evident in the scoped diff. The changed files are limited to the ProtoFleet rack overview page and its tests.


Generated by Codex Security Review |
Triggered by: @jmarrxyz |
Review workflow run

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 702199502d

ℹ️ 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 +311 to +314
if (rackSiteId) {
rackBreadcrumbSegments.push({ label: "Sites", to: "/fleet/sites" });
rackBreadcrumbSegments.push({ label: siteNameById.get(rackSiteId) ?? "Site", to: `/sites/${rackSiteId}` });
if (rackBuildingId) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle racks in buildings without sites

When a rack is assigned to a building whose building.siteId is unset (the API supports unassigned buildings), rackSiteId is undefined, so this top-level if (rackSiteId) sends it through the unassigned-rack fallback and never adds the building segment/link. That makes racks in unassigned buildings show Racks / rack instead of preserving their building context; branch on rackBuildingId separately so those pages still link to /buildings/:id.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

client javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant