Occupancy Page Cross Linking and UI Polish#677
Merged
fernandol-nvidia merged 7 commits intomainfrom Mar 11, 2026
Merged
Conversation
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGroup-by and search chip state were threaded through occupancy UI: toolbar, column generation, and data table now accept and propagate Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OccupancyPage
participant OccupancyToolbar
participant OccupancyDataTable
participant ColumnDefs
participant WorkflowsApp
User->>OccupancyPage: Open occupancy page (URL with chips)
OccupancyPage->>OccupancyToolbar: provide `groupBy`, `searchChips`, handlers
OccupancyToolbar->>OccupancyPage: emit groupBy/searchChips events
OccupancyPage->>OccupancyDataTable: pass `searchChips`, `groupBy`, data props
OccupancyDataTable->>ColumnDefs: createOccupancyColumns(groupBy, searchChips)
ColumnDefs->>OccupancyDataTable: return columns (include workflow/pool links)
User->>OccupancyDataTable: click "View Workflows"
OccupancyDataTable->>WorkflowsApp: navigate to built workflows URL (includes chips, groupBy)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/ui/src/features/pools/components/panel/panel-content.tsx (1)
191-202: Avoid hand-rolling another filter URL here.This adds a third inline copy of the
f=query serialization in the quick-links card. Please consider pushing these hrefs through a small shared route/filter builder so occupancy, workflows, and resources stay aligned if the chip/query format changes again.As per coding guidelines, "Single source of truth for each concept - avoid multiple representations of the same data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/pools/components/panel/panel-content.tsx` around lines 191 - 202, Replace the hand-rolled occupancy href that constructs `?f=pool:${encodeURIComponent(pool.name)}&groupBy=pool` with a call to the shared route/filter builder (e.g., use or add a helper like buildFilterUrl or buildOccupancyLink) so the Link uses that single source of truth; update the Link in panel-content.tsx to call the helper with the filter spec (pool: pool.name) and groupBy="pool" instead of embedding `f=` manually, ensuring URL encoding is handled by the shared builder.src/ui/src/components/filter-bar/hooks/use-default-filter.ts (1)
72-77: Make the “explicit chip wins” rule synchronous.This only cleans up the URL after commit. On the render where both
optOutand an explicit chip are present, the hook still returnsoptOut=true, so callers can observe contradictory state for one render. Returning aneffectiveOptOut = optOut && !hasDefaultInUrlwould make the hook’s output match the precedence rule immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts` around lines 72 - 77, The hook useDefaultFilter should synchronously expose the precedence rule so callers don’t see a transient contradictory state: compute an effectiveOptOut = optOut && !hasDefaultInUrl and return that instead of raw optOut, while keeping the existing useEffect to clear the URL (call setOptOut(false)) as a side-effect when optOut && hasDefaultInUrl; update any return/exports from useDefaultFilter to use effectiveOptOut so render-time consumers observe the explicit-chip-wins rule immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/src/components/filter-bar/hooks/use-url-chips.ts`:
- Around line 35-37: The current parseAsChipStrings (created via
createMultiParser) is lossy because parse splits on "," and serialize joins with
","; update parse and serialize to avoid using an unescaped secondary separator.
Specifically, in parseAsChipStrings' parse function stop splitting each entry on
commas and instead treat each incoming string as one chip (e.g., return
values.filter(Boolean) or flatMap only for true multi-entry encodings), and
change serialize to return one array element per chip (e.g., return
Array.from(values) / values.slice()) so each chip is preserved as its own
parameter rather than conflating them with a comma join. Ensure you only
reference parseAsChipStrings, createMultiParser, parse, and serialize when
making the change.
In `@src/ui/src/features/occupancy/components/occupancy-column-defs.tsx`:
- Around line 149-158: The row actions button is invisible to keyboard focus
because it only becomes opaque on hover/open; update the className passed to the
button (the cn(...) call in occupancy-column-defs.tsx for the MoreHorizontal
trigger) to also reveal the control on keyboard focus and show a visible focus
ring—add a focus-visible:opacity-100 and appropriate focus-visible:ring and
focus-visible:outline-none (or your design-system ring classes) alongside the
existing hover/open classes, preserving the original.isExpanded check so
expanded rows remain visible.
- Around line 144-181: Wrap the Radix DropdownMenu in a mounted guard using the
existing useMounted() hook so it only renders on the client; specifically, check
useMounted() before rendering the DropdownMenu (the block that contains
DropdownMenu, DropdownMenuTrigger, DropdownMenuContent and DropdownMenuItem) and
return a fallback/omit the menu when mounted is false following the same pattern
used in theme-toggle.tsx to avoid hydration ID mismatches. Ensure the guard
surrounds the whole DropdownMenu subtree (including the MoreHorizontal trigger
button and Link items) and use the mounted boolean to conditionally render that
subtree.
In `@src/ui/src/lib/workflows/workflow-constants.ts`:
- Around line 92-101: WORKFLOW_FILTER_FIELDS currently includes "status", which
causes invalid deep-links because occupancy chips use TaskGroupStatus values
that don't map to WorkflowStatus; remove "status" from WORKFLOW_FILTER_FIELDS to
stop forwarding raw occupancy status values into workflow URLs, and instead add
a translation step wherever occupancy filters are converted into workflow query
params (map TaskGroupStatus -> WorkflowStatus, e.g., in the function that
constructs workflow URLs or the occupancy-to-workflow forwarding logic). Ensure
the translation explicitly handles unmappable values (drop or normalize them)
and reference TaskGroupStatus and WorkflowStatus in the translator to make the
mapping intent clear.
---
Nitpick comments:
In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts`:
- Around line 72-77: The hook useDefaultFilter should synchronously expose the
precedence rule so callers don’t see a transient contradictory state: compute an
effectiveOptOut = optOut && !hasDefaultInUrl and return that instead of raw
optOut, while keeping the existing useEffect to clear the URL (call
setOptOut(false)) as a side-effect when optOut && hasDefaultInUrl; update any
return/exports from useDefaultFilter to use effectiveOptOut so render-time
consumers observe the explicit-chip-wins rule immediately.
In `@src/ui/src/features/pools/components/panel/panel-content.tsx`:
- Around line 191-202: Replace the hand-rolled occupancy href that constructs
`?f=pool:${encodeURIComponent(pool.name)}&groupBy=pool` with a call to the
shared route/filter builder (e.g., use or add a helper like buildFilterUrl or
buildOccupancyLink) so the Link uses that single source of truth; update the
Link in panel-content.tsx to call the helper with the filter spec (pool:
pool.name) and groupBy="pool" instead of embedding `f=` manually, ensuring URL
encoding is handled by the shared builder.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 380dcf71-54cc-4532-8ce9-5a151213cce2
📒 Files selected for processing (12)
src/ui/src/app/(dashboard)/occupancy/occupancy-page-content.tsxsrc/ui/src/components/filter-bar/hooks/use-default-filter.tssrc/ui/src/components/filter-bar/hooks/use-url-chips.tssrc/ui/src/features/occupancy/components/occupancy-column-defs.tsxsrc/ui/src/features/occupancy/components/occupancy-data-table.tsxsrc/ui/src/features/occupancy/components/occupancy-toolbar.tsxsrc/ui/src/features/occupancy/lib/occupancy-search-fields.tssrc/ui/src/features/pools/components/panel/panel-content.tsxsrc/ui/src/features/workflows/list/components/workflows-toolbar.tsxsrc/ui/src/features/workflows/list/lib/workflow-search-fields.test.tssrc/ui/src/features/workflows/list/lib/workflow-search-fields.tssrc/ui/src/lib/workflows/workflow-constants.ts
💤 Files with no reviewable changes (1)
- src/ui/src/features/workflows/list/lib/workflow-search-fields.ts
src/ui/src/features/occupancy/components/occupancy-column-defs.tsx
Outdated
Show resolved
Hide resolved
src/ui/src/features/occupancy/components/occupancy-column-defs.tsx
Outdated
Show resolved
Hide resolved
fernandol-nvidia
force-pushed
the
fernandol/occupancy-followups
branch
from
eaec07e to
5fa577f
Compare
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/ui/src/components/filter-bar/hooks/use-default-filter.ts (1)
105-105: Memoize the hook result.Line 105 allocates a new object every render, so any consumer that depends on the whole return value will re-run even when
effectiveChips,handleChipsChange, andoptOutare stable.♻️ Suggested fix
- return { effectiveChips, handleChipsChange, optOut: effectiveOptOut }; + return useMemo( + () => ({ effectiveChips, handleChipsChange, optOut: effectiveOptOut }), + [effectiveChips, handleChipsChange, effectiveOptOut], + );As per coding guidelines, "Memoize returned objects in hooks to prevent cascading re-renders when used as React Query keys (use useMemo for objects)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts` at line 105, The hook currently returns a newly allocated object each render which causes consumers to re-run; wrap the returned object in useMemo so it only changes when its dependencies change (memoize the object containing effectiveChips, handleChipsChange, and optOut derived from effectiveOptOut) — replace the plain return of { effectiveChips, handleChipsChange, optOut: effectiveOptOut } with a useMemo that lists [effectiveChips, handleChipsChange, effectiveOptOut] as dependencies to ensure a stable reference from use-default-filter.ts.src/ui/src/features/datasets/list/components/toolbar/datasets-toolbar.tsx (1)
54-63: Avoid hard-codingDATASET_STATIC_FIELDSby index.This works today, but the toolbar is now coupled to the array’s export order. Adding or reordering a static dataset field will silently change which fields show up here; a named field map or explicit destructuring would make this safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/datasets/list/components/toolbar/datasets-toolbar.tsx` around lines 54 - 63, The toolbar currently builds searchFields by hard-indexing DATASET_STATIC_FIELDS, coupling UI to the array order; update the useMemo in datasets-toolbar.tsx (searchFields) to select fields explicitly instead of by index — either destructure named exports or filter DATASET_STATIC_FIELDS by an explicit list of field keys/names (e.g., ["title","description","..."]) and include userField, so reordering the export won't break the toolbar; ensure the dependency array still includes userField.src/ui/src/features/workflows/list/lib/workflow-search-fields.ts (1)
24-24: KeepWORKFLOW_FIELDkey-safe.Line 24 widens this object to
Record<string, ...>, so typos likeWORKFLOW_FIELD.priortystill type-check and only fail later asundefined. Use explicit keys orsatisfieshere so the keyed-map refactor actually preserves compile-time safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/workflows/list/lib/workflow-search-fields.ts` at line 24, The WORKFLOW_FIELD object is typed as Record<string, SearchField<WorkflowListEntry>> which permits typos like WORKFLOW_FIELD.priorty; change its declaration to enforce exact keys by replacing the broad Record<string,...> with a concrete key type or use TypeScript's satisfies operator so the object literal is checked against an explicit key union (e.g. a WorkflowFieldKey union) or a typed mapped type; update the declaration of WORKFLOW_FIELD to use that exact-key type (and keep Object.freeze) so misspelled property access fails at compile time rather than runtime, referencing WORKFLOW_FIELD, SearchField and WorkflowListEntry in the fix.src/ui/src/features/occupancy/lib/occupancy-search-fields.ts (1)
1-15: Copyright header format inconsistency.Same issue as noted in
occupancy-page-content.tsx- missing space after//in SPDX lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/occupancy/lib/occupancy-search-fields.ts` around lines 1 - 15, Update the SPDX license header comments in occupancy-search-fields.ts to match the project's comment style by ensuring there is a single space after the comment markers (i.e., change lines starting with "//SPDX" to "// SPDX" and similarly add a space after "//" on all SPDX-related lines) so the header formatting matches occupancy-page-content.tsx and other files.src/ui/src/features/occupancy/components/occupancy-toolbar.tsx (1)
1-15: Copyright header format inconsistency.Same issue as noted in other occupancy files - consider normalizing the format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/features/occupancy/components/occupancy-toolbar.tsx` around lines 1 - 15, Replace the inconsistent copyright header in occupancy-toolbar.tsx with the project-standard SPDX header used across other occupancy files; ensure it matches the exact formatting (including SPDX-License-Identifier line and license text sequence) used in files like other occupancy components so the header is normalized across the codebase.src/ui/src/app/(dashboard)/occupancy/occupancy-page-content.tsx (1)
1-15: Copyright header format inconsistency.The copyright header uses
//SPDX-FileCopyrightText:without a space after//and uses "NVIDIA CORPORATION." without "& AFFILIATES". Compare with other files in this PR (e.g.,task-search-fields.tsx) which use the standard format:// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION. All rights reserved.This appears to be a pre-existing issue in this file. Consider normalizing to match the standard format used elsewhere. As per coding guidelines: "Copyright headers must keep 'All rights reserved.' on the same line as 'NVIDIA CORPORATION & AFFILIATES'".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/app/`(dashboard)/occupancy/occupancy-page-content.tsx around lines 1 - 15, Replace the malformed SPDX header lines "//SPDX-FileCopyrightText:" and "//SPDX-License-Identifier:" with the standard formatted header: add a space after the comment slashes and update the copyright line to include "NVIDIA CORPORATION & AFFILIATES. All rights reserved." on the same line (e.g., "// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved."); keep the SPDX-License-Identifier line similarly formatted ("// SPDX-License-Identifier: Apache-2.0").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/src/components/filter-bar/hooks/use-url-chips.ts`:
- Around line 35-38: The current eq function in use-url-chips.ts only checks set
membership and thus treats arrays with different duplicate counts as equal;
change eq to compare multisets by counting occurrences: if lengths differ return
false, build frequency maps (or sort) for the two arrays and ensure every
element has the same count in both; update the function named eq to use these
count comparisons so duplicates are honored and URL/UI state stays in sync.
In `@src/ui/src/components/filter-bar/lib/preset-pill.ts`:
- Around line 1-15: Replace the current top-of-file comment block with the
standard NVIDIA Apache-2.0 SPDX header used for src/ui/src TypeScript files:
include the copyright line "Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES.
All rights reserved." on a single line, the Apache License 2.0 notice, the URL
http://www.apache.org/licenses/LICENSE-2.0, and the SPDX-License-Identifier:
Apache-2.0 token; ensure the entire header mirrors the project’s canonical
header format for ui TS/TSX files so the SPDX/license block and "All rights
reserved." placement match policy.
In `@src/ui/src/features/occupancy/components/occupancy-column-defs.tsx`:
- Around line 33-35: The CROSS_LINKABLE_FIELDS constant currently hard-codes
only ["pool","user","priority"], dropping other active occupancy chips (e.g.,
"status") when building the workflows link; replace this ad-hoc allow-list with
a single-source mapping or reuse the centralized translation function/mapping
used for occupancy→workflow filters (do not hard-code here), ensure all
applicable occupancy filter keys are forwarded or translated (including status)
when building the workflows URL, and remove the duplicate logic at the other
occurrence (lines referenced 84-87) so both places call the same shared
mapping/translator (refer to CROSS_LINKABLE_FIELDS and the code that constructs
the workflows link).
- Around line 104-109: The placeholder button and the mounted menu trigger both
use a generic aria-label "Row actions"; update their accessible names to include
the row identifier by interpolating original.key into the aria-label (e.g.
aria-label={`Row actions ${original.key}`}) so screen readers get row-specific
context; ensure you change the disabled placeholder button element and the
mounted trigger component that renders the real menu trigger (both referencing
original.key) to use the new aria-label format.
In `@src/ui/src/lib/task-group-status-presets.tsx`:
- Around line 1-15: Replace the current top-of-file comment block in
task-group-status-presets.tsx with the standard NVIDIA Apache-2.0 SPDX header: a
copyright line that reads "Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES.
All rights reserved." on the same line, followed by the full Apache License,
Version 2.0 notice and the SPDX-License-Identifier: Apache-2.0 line; ensure the
block replaces the existing lines 1–15 so the file conforms to
src/ui/src/**/*.{ts,tsx} header requirements.
---
Nitpick comments:
In `@src/ui/src/app/`(dashboard)/occupancy/occupancy-page-content.tsx:
- Around line 1-15: Replace the malformed SPDX header lines
"//SPDX-FileCopyrightText:" and "//SPDX-License-Identifier:" with the standard
formatted header: add a space after the comment slashes and update the copyright
line to include "NVIDIA CORPORATION & AFFILIATES. All rights reserved." on the
same line (e.g., "// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA
CORPORATION & AFFILIATES. All rights reserved."); keep the
SPDX-License-Identifier line similarly formatted ("// SPDX-License-Identifier:
Apache-2.0").
In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts`:
- Line 105: The hook currently returns a newly allocated object each render
which causes consumers to re-run; wrap the returned object in useMemo so it only
changes when its dependencies change (memoize the object containing
effectiveChips, handleChipsChange, and optOut derived from effectiveOptOut) —
replace the plain return of { effectiveChips, handleChipsChange, optOut:
effectiveOptOut } with a useMemo that lists [effectiveChips, handleChipsChange,
effectiveOptOut] as dependencies to ensure a stable reference from
use-default-filter.ts.
In `@src/ui/src/features/datasets/list/components/toolbar/datasets-toolbar.tsx`:
- Around line 54-63: The toolbar currently builds searchFields by hard-indexing
DATASET_STATIC_FIELDS, coupling UI to the array order; update the useMemo in
datasets-toolbar.tsx (searchFields) to select fields explicitly instead of by
index — either destructure named exports or filter DATASET_STATIC_FIELDS by an
explicit list of field keys/names (e.g., ["title","description","..."]) and
include userField, so reordering the export won't break the toolbar; ensure the
dependency array still includes userField.
In `@src/ui/src/features/occupancy/components/occupancy-toolbar.tsx`:
- Around line 1-15: Replace the inconsistent copyright header in
occupancy-toolbar.tsx with the project-standard SPDX header used across other
occupancy files; ensure it matches the exact formatting (including
SPDX-License-Identifier line and license text sequence) used in files like other
occupancy components so the header is normalized across the codebase.
In `@src/ui/src/features/occupancy/lib/occupancy-search-fields.ts`:
- Around line 1-15: Update the SPDX license header comments in
occupancy-search-fields.ts to match the project's comment style by ensuring
there is a single space after the comment markers (i.e., change lines starting
with "//SPDX" to "// SPDX" and similarly add a space after "//" on all
SPDX-related lines) so the header formatting matches occupancy-page-content.tsx
and other files.
In `@src/ui/src/features/workflows/list/lib/workflow-search-fields.ts`:
- Line 24: The WORKFLOW_FIELD object is typed as Record<string,
SearchField<WorkflowListEntry>> which permits typos like WORKFLOW_FIELD.priorty;
change its declaration to enforce exact keys by replacing the broad
Record<string,...> with a concrete key type or use TypeScript's satisfies
operator so the object literal is checked against an explicit key union (e.g. a
WorkflowFieldKey union) or a typed mapped type; update the declaration of
WORKFLOW_FIELD to use that exact-key type (and keep Object.freeze) so misspelled
property access fails at compile time rather than runtime, referencing
WORKFLOW_FIELD, SearchField and WorkflowListEntry in the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfc58f61-317d-4052-b5a5-bbc3cce84ea3
📒 Files selected for processing (18)
src/ui/src/app/(dashboard)/occupancy/occupancy-page-content.tsxsrc/ui/src/components/filter-bar/hooks/use-default-filter.tssrc/ui/src/components/filter-bar/hooks/use-url-chips.tssrc/ui/src/components/filter-bar/lib/preset-pill.tssrc/ui/src/features/datasets/list/components/toolbar/datasets-toolbar.tsxsrc/ui/src/features/occupancy/components/occupancy-column-defs.tsxsrc/ui/src/features/occupancy/components/occupancy-data-table.tsxsrc/ui/src/features/occupancy/components/occupancy-toolbar.tsxsrc/ui/src/features/occupancy/lib/occupancy-search-fields.tssrc/ui/src/features/pools/components/pools-toolbar.tsxsrc/ui/src/features/workflows/detail/components/panel/core/lib/task-search-fields.tsxsrc/ui/src/features/workflows/detail/components/panel/ui/group/group-tasks-tab.tsxsrc/ui/src/features/workflows/detail/components/panel/ui/table/workflow-tasks-table.tsxsrc/ui/src/features/workflows/detail/components/panel/ui/table/workflow-tasks-toolbar.tsxsrc/ui/src/features/workflows/list/components/workflows-toolbar.tsxsrc/ui/src/features/workflows/list/lib/workflow-search-fields.test.tssrc/ui/src/features/workflows/list/lib/workflow-search-fields.tssrc/ui/src/lib/task-group-status-presets.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/src/features/workflows/list/lib/workflow-search-fields.test.ts
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
fernandol-nvidia
changed the title
cypres
previously approved these changes
Mar 11, 2026
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ui/src/lib/api/adapter/occupancy-shim.ts (1)
155-160: Only sendall_users/all_poolswhen they are enabled (true).The backend treats omission and explicit
falseidentically via FastAPI's default parameter handling. Conditional omission is cleaner and better documents intent. Match the pattern in workflows-shim.ts:110 and align with "only when necessary."Proposed change
const response = await listTaskApiTaskGet({ summary: true, limit: MAX_SUMMARY_ROWS, ...(hasUserFilter ? { users: params.users } : {}), ...(hasPoolFilter ? { pools: params.pools } : {}), ...(params.priorities && params.priorities.length > 0 ? { priority: params.priorities } : {}), ...(params.statuses && params.statuses.length > 0 ? { statuses: params.statuses } : {}), - all_pools: !hasPoolFilter, - all_users: !hasUserFilter, + ...(!hasPoolFilter ? { all_pools: true } : {}), + ...(!hasUserFilter ? { all_users: true } : {}), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/lib/api/adapter/occupancy-shim.ts` around lines 155 - 160, The object currently always includes all_pools and all_users (set to !hasPoolFilter / !hasUserFilter); change this so the properties are only added when true (i.e., when !hasPoolFilter is true add { all_pools: true }, and when !hasUserFilter is true add { all_users: true }) following the same conditional-spread pattern used above (use hasPoolFilter, hasUserFilter and params.users/params.pools as reference points) so omission and explicit false are not sent to the backend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ui/src/lib/api/adapter/occupancy-shim.ts`:
- Around line 155-160: The object currently always includes all_pools and
all_users (set to !hasPoolFilter / !hasUserFilter); change this so the
properties are only added when true (i.e., when !hasPoolFilter is true add {
all_pools: true }, and when !hasUserFilter is true add { all_users: true })
following the same conditional-spread pattern used above (use hasPoolFilter,
hasUserFilter and params.users/params.pools as reference points) so omission and
explicit false are not sent to the backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d7cc0eb-a40c-439c-b6e0-284c79b0955d
📒 Files selected for processing (1)
src/ui/src/lib/api/adapter/occupancy-shim.ts
cypres
previously approved these changes
Mar 11, 2026
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
fernandol-nvidia
temporarily deployed
to
pr-preview
GitHub Actions
Inactive
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/677/index.html |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/src/app/(dashboard)/occupancy/occupancy-page-content.tsx (2)
149-157:⚠️ Potential issue | 🟡 MinorInclude the new table inputs in the boundary reset keys.
OccupancyDataTablenow depends ongroupByandsearchChips, but the boundary still only resets ongroups.length. If a render error happens for one filter state and the next state has the same row count, the fallback will stay stuck.As per coding guidelines, "Use component-level error boundaries for independent data sources. Wrap with InlineErrorBoundary with descriptive titles, onReset, and resetKeys."Possible fix
<InlineErrorBoundary title="Unable to display occupancy table" - resetKeys={[groups.length]} + resetKeys={[groupBy, groups.length, JSON.stringify(searchChips)]} onReset={refetch} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/app/`(dashboard)/occupancy/occupancy-page-content.tsx around lines 149 - 157, The InlineErrorBoundary wrapping OccupancyDataTable currently only uses resetKeys={[groups.length]}, so errors won't reset when table inputs change; update the resetKeys to include the new inputs that affect rendering (e.g., groupBy and searchChips) so the boundary will remount when those change — locate the InlineErrorBoundary around OccupancyDataTable and change resetKeys to include groups.length, groupBy, and searchChips (or stable identifiers derived from them) while keeping onReset={refetch} and the same title.
73-81:⚠️ Potential issue | 🟡 MinorPrune expanded keys when the visible group set changes.
This only resets on
groupBy, so a status/search change can leave stale keys from now-hidden groups inexpandedKeys. That makesallExpandeddrift out of sync with the rows currently on screen.As per coding guidelines, "Single source of truth for each concept - avoid multiple representations of the same data."Possible fix
+ const visibleGroupKeys = useMemo(() => new Set(groups.map((group) => group.key)), [groups]); + const expandedKeys = useMemo( - () => (expandedState.groupBy === groupBy ? expandedState.keys : new Set<string>()), - [expandedState, groupBy], + () => { + if (expandedState.groupBy !== groupBy) { + return new Set<string>(); + } + + return new Set([...expandedState.keys].filter((key) => visibleGroupKeys.has(key))); + }, + [expandedState, groupBy, visibleGroupKeys], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/app/`(dashboard)/occupancy/occupancy-page-content.tsx around lines 73 - 81, expandedKeys can contain stale keys because you only reset when groupBy changes; update the logic that derives expandedKeys to prune any keys not present in the current visible group set (e.g. from the current rows/status/search) so expandedState.keys stays in sync with what's actually rendered. Concretely, in the code surrounding expandedState / setExpandedState and the computed expandedKeys, filter expandedState.keys against the currentVisibleGroupKeys (compute from the current rows or groupRows selector) whenever those visible groups change (add that dependency), or update setExpandedState when visibility changes to remove keys that are no longer present.
🧹 Nitpick comments (1)
src/ui/src/components/filter-bar/hooks/use-default-filter.ts (1)
78-103: Code duplication:defaultsarray is constructed identically in two places.The same mapping logic to create the
defaultsarray appears in botheffectiveChips(lines 80-85) and the prepopulate effect (lines 97-102). Extract this to a single memoized value to keep logic consistent and reduce duplication.♻️ Proposed refactor to extract defaults computation
+ const defaultChips = useMemo( + (): SearchChip[] => + normalizedDefaults.map((v) => ({ + field, + value: v, + label: label ?? `${field}: ${v}`, + })), + [field, normalizedDefaults, label], + ); + const effectiveChips = useMemo((): SearchChip[] => { if (!shouldPrePopulate) return searchChips; - const defaults: SearchChip[] = normalizedDefaults.map((v) => ({ - field, - value: v, - label: label ?? `${field}: ${v}`, - })); - return [...searchChips, ...defaults]; - }, [searchChips, shouldPrePopulate, field, normalizedDefaults, label]); + return [...searchChips, ...defaultChips]; + }, [searchChips, shouldPrePopulate, defaultChips]); // ... opt-out effect unchanged ... useEffect(() => { if (!shouldPrePopulate) return; - const defaults: SearchChip[] = normalizedDefaults.map((v) => ({ - field, - value: v, - label: label ?? `${field}: ${v}`, - })); - void setSearchChips([...searchChips, ...defaults], { history: "replace" }); - }, [shouldPrePopulate, searchChips, field, normalizedDefaults, label, setSearchChips]); + void setSearchChips([...searchChips, ...defaultChips], { history: "replace" }); + }, [shouldPrePopulate, searchChips, defaultChips, setSearchChips]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts` around lines 78 - 103, Extract the duplicated defaults mapping into a single memoized value and use it in both places: create a const like computedDefaults = useMemo(() => normalizedDefaults.map(v => ({ field, value: v, label: label ?? `${field}: ${v}` })), [normalizedDefaults, field, label]) and replace the inline `defaults` in effectiveChips and the prepopulate useEffect with computedDefaults; ensure the dependencies arrays for effectiveChips and the prepopulate effect reference computedDefaults instead of normalizedDefaults/field/label to avoid missing deps and keep behavior identical (references: effectiveChips, setSearchChips, normalizedDefaults, setOptOut).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts`:
- Around line 80-84: The current mapping in use-default-filter.ts assigns the
same custom label to every chip when defaultValue is an array; change the label
construction so the value is always included when there are multiple defaults:
inside the mapping that builds defaults from normalizedDefaults, use the
provided label (or field when label is undefined) as a prefix and append `:
${v}` for each chip when normalizedDefaults.length > 1 (but preserve the
original single-chip behavior when there's only one default), referencing the
normalizedDefaults array, the defaults mapping, field, label, and the SearchChip
shape.
---
Outside diff comments:
In `@src/ui/src/app/`(dashboard)/occupancy/occupancy-page-content.tsx:
- Around line 149-157: The InlineErrorBoundary wrapping OccupancyDataTable
currently only uses resetKeys={[groups.length]}, so errors won't reset when
table inputs change; update the resetKeys to include the new inputs that affect
rendering (e.g., groupBy and searchChips) so the boundary will remount when
those change — locate the InlineErrorBoundary around OccupancyDataTable and
change resetKeys to include groups.length, groupBy, and searchChips (or stable
identifiers derived from them) while keeping onReset={refetch} and the same
title.
- Around line 73-81: expandedKeys can contain stale keys because you only reset
when groupBy changes; update the logic that derives expandedKeys to prune any
keys not present in the current visible group set (e.g. from the current
rows/status/search) so expandedState.keys stays in sync with what's actually
rendered. Concretely, in the code surrounding expandedState / setExpandedState
and the computed expandedKeys, filter expandedState.keys against the
currentVisibleGroupKeys (compute from the current rows or groupRows selector)
whenever those visible groups change (add that dependency), or update
setExpandedState when visibility changes to remove keys that are no longer
present.
---
Nitpick comments:
In `@src/ui/src/components/filter-bar/hooks/use-default-filter.ts`:
- Around line 78-103: Extract the duplicated defaults mapping into a single
memoized value and use it in both places: create a const like computedDefaults =
useMemo(() => normalizedDefaults.map(v => ({ field, value: v, label: label ??
`${field}: ${v}` })), [normalizedDefaults, field, label]) and replace the inline
`defaults` in effectiveChips and the prepopulate useEffect with
computedDefaults; ensure the dependencies arrays for effectiveChips and the
prepopulate effect reference computedDefaults instead of
normalizedDefaults/field/label to avoid missing deps and keep behavior identical
(references: effectiveChips, setSearchChips, normalizedDefaults, setOptOut).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8022e46-cc76-4741-9bac-423443b0fdd3
📒 Files selected for processing (2)
src/ui/src/app/(dashboard)/occupancy/occupancy-page-content.tsxsrc/ui/src/components/filter-bar/hooks/use-default-filter.ts
fernandol-nvidia
temporarily deployed
to
internal-ci
GitHub Actions
Inactive
RyaliNvidia
approved these changes
Mar 11, 2026
cypres
approved these changes
Mar 11, 2026
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
3 participants
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.
Description
Issue #682
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements