OSAC-3604: Storage Tiers list page - #128
Conversation
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…errides Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
MockTransportOverrides declared these fields with the strict generated Message type, which requires $typeName on every literal returned from a test override. No existing test exercised onStorageTierList, onStorageBackendList, or onStorageTierDelete with a literal response until this story's StorageTiersListPage tests, which is why the gap was latent. MessageInitShape matches how the rest of the codebase accepts plain-object message inputs elsewhere (e.g. useCreateStorageTier). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
StorageManagementPage's Tabs mounted both tab bodies simultaneously (PatternFly's default, hiding the inactive one via CSS), so every visit to the Backends tab also mounted StorageTiersListPage and fired its usePrivateStorageTiers/usePrivateStorageBackends fetches for content the user never sees. Harmless while both tabs held inert placeholders, but StorageTiersListPage is now a real, self-fetching page. Add mountOnEnter/unmountOnExit so only the active tab's content mounts. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
|
@ElayAharoni: This pull request references OSAC-3604 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughChangesStorage tiers administration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant StorageTiersListPage
participant StorageTierActionsMenu
participant StorageTierDeleteConfirmModal
participant StorageAPI
Admin->>StorageTiersListPage: open Tiers tab
StorageTiersListPage->>StorageAPI: fetch tiers and referenced backends
StorageAPI-->>StorageTiersListPage: return storage data
StorageTiersListPage-->>Admin: render tier table
Admin->>StorageTierActionsMenu: select Delete
StorageTierActionsMenu->>StorageTierDeleteConfirmModal: open confirmation
StorageTierDeleteConfirmModal->>StorageAPI: delete tier
StorageAPI-->>StorageTierDeleteConfirmModal: return result
StorageTierDeleteConfirmModal-->>StorageTiersListPage: remove deleted row
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Gate the Tiers list page's backend-name lookup with an enabled option on usePrivateStorageBackends (mirroring the existing pattern in networking.ts), avoiding a wasted this.id in [] request on every page load. Surface backend-lookup failures as a distinct inline warning instead of letting them look identical to the per-row deleted-backend fallback. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/ui-components/src/components/Storage/StorageTierStatusLabel.tsx (1)
9-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a
switchfor theStorageTierStatemapping.
resolveStorageTierStatustranslates an enum value to label props. Replace the lookup map and conditional with aswitchthat has anUNSPECIFIED/default branch. This keeps each supported state in one readable control flow.Proposed refactor
-const STORAGE_TIER_STATUS_MAP: Record<StorageTierState, { status: StatusKind; text: string }> = { - [StorageTierState.UNSPECIFIED]: { status: 'unspecified', text: 'Unspecified' }, - [StorageTierState.ACTIVE]: { status: 'ready', text: 'Active' }, -}; - -const resolveStorageTierStatus = ( - state?: StorageTierState, -): { status: StatusKind; text: string } => - state !== undefined && state in STORAGE_TIER_STATUS_MAP - ? STORAGE_TIER_STATUS_MAP[state] - : STORAGE_TIER_STATUS_MAP[StorageTierState.UNSPECIFIED]; +const resolveStorageTierStatus = ( + state?: StorageTierState, +): { status: StatusKind; text: string } => { + switch (state) { + case StorageTierState.ACTIVE: + return { status: 'ready', text: 'Active' }; + case StorageTierState.UNSPECIFIED: + default: + return { status: 'unspecified', text: 'Unspecified' }; + } +};Based on learnings: prefer a
switchstatement when translating a discriminant to corresponding component props.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/components/Storage/StorageTierStatusLabel.tsx` around lines 9 - 19, Replace STORAGE_TIER_STATUS_MAP and the conditional lookup in resolveStorageTierStatus with a switch on state, returning the existing status/text props for ACTIVE and an UNSPECIFIED/default branch for undefined or unsupported values.Source: Learnings
libs/ui-components/src/pages/admin/StorageManagementPage.test.tsx (1)
52-71: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTest unmounting after leaving the Tiers tab.
This test starts on the Backends tab, so it only verifies
mountOnEnter. It does not verifyunmountOnExit. Start on/admin/storage/tiers, wait for the tier request, switch to Backends, and assert that the Tiers content is removed and no further tier request occurs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/pages/admin/StorageManagementPage.test.tsx` around lines 52 - 71, Update the test around StorageManagementPage to start at `/admin/storage/tiers`, wait for the tier request and content, then navigate to the Backends tab. Assert the Tiers content is removed after switching and that `onStorageTierList` is not called again, covering unmount-on-exit rather than only initial mount behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui-components/src/pages/admin/StorageTiersListPage.tsx`:
- Around line 60-70: Render the Create tier action independently of the error
state in StorageTiersListPage, removing the !error guard while preserving its
existing navigation behavior. Add a test covering a usePrivateStorageTiers list
error and verify the Create tier button remains available.
---
Nitpick comments:
In `@libs/ui-components/src/components/Storage/StorageTierStatusLabel.tsx`:
- Around line 9-19: Replace STORAGE_TIER_STATUS_MAP and the conditional lookup
in resolveStorageTierStatus with a switch on state, returning the existing
status/text props for ACTIVE and an UNSPECIFIED/default branch for undefined or
unsupported values.
In `@libs/ui-components/src/pages/admin/StorageManagementPage.test.tsx`:
- Around line 52-71: Update the test around StorageManagementPage to start at
`/admin/storage/tiers`, wait for the tier request and content, then navigate to
the Backends tab. Assert the Tiers content is removed after switching and that
`onStorageTierList` is not called again, covering unmount-on-exit rather than
only initial mount behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6a3320a-7f4d-41e4-90e8-68ccaa3a6263
📒 Files selected for processing (14)
apps/app-frontend/src/shell/StorageRoutes.test.tsxapps/app-frontend/src/shell/StorageRoutes.tsxlibs/i18n/locales/en/translation.jsonlibs/ui-components/src/api/v1/private/storage-backends.tslibs/ui-components/src/components/Storage/StorageTierActionsMenu.tsxlibs/ui-components/src/components/Storage/StorageTierDeleteConfirmModal.test.tsxlibs/ui-components/src/components/Storage/StorageTierDeleteConfirmModal.tsxlibs/ui-components/src/components/Storage/StorageTierStatusLabel.test.tsxlibs/ui-components/src/components/Storage/StorageTierStatusLabel.tsxlibs/ui-components/src/pages/admin/StorageManagementPage.test.tsxlibs/ui-components/src/pages/admin/StorageManagementPage.tsxlibs/ui-components/src/pages/admin/StorageTiersListPage.test.tsxlibs/ui-components/src/pages/admin/StorageTiersListPage.tsxlibs/ui-components/src/test-utils/createMockConnectTransport.ts
|
rename State column to Status and put it after the name column to match most resource tables |
- Keep the Create tier action available when the tier list fails to load — a failed List doesn't mean creation is broken (CodeRabbit). - Add a test covering unmountOnExit for the Tiers tab, not just mountOnEnter (CodeRabbit). - Rename the STATE column to Status and move it right after Name, to match every other resource table in this codebase (batzionb). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
|
@batzionb Done — renamed the column header to Status and moved it right after Name, matching @coderabbitai Also addressed both nitpicks from the review, with one exception:
|
|
Tip For best results, initiate chat on the files or code changes.
Your You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-components/src/pages/admin/StorageTiersListPage.test.tsx (1)
87-93: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the changed column label and order.
This test verifies only the rendered
Activelabels. It does not verify that the column header isStatusor that it appears immediately afterName, which are explicit requirements of this PR.Suggested assertions
await waitFor(() => { expect(screen.getAllByText('Active')).toHaveLength(2); }); + expect(screen.getByRole('columnheader', { name: 'Status' })).toBeInTheDocument(); + expect( + screen.getAllByRole('columnheader') + .slice(0, 4) + .map((header) => header.textContent?.trim()), + ).toEqual(['Name', 'Status', 'Backends', 'Protocol(s)']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/ui-components/src/pages/admin/StorageTiersListPage.test.tsx` around lines 87 - 93, Extend the test “renders the STATUS column via StorageTierStatusLabel” to assert that the table header is labeled “Status” and that it appears immediately after the “Name” header, while preserving the existing checks for the two rendered “Active” labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/ui-components/src/pages/admin/StorageManagementPage.test.tsx`:
- Around line 91-94: Update the test around the mounted storage-tier query to
use an empty storage tier fixture, then move the onStorageTierList call-count
assertion inside the existing waitFor that waits for the Create tier button.
Keep the assertion at one call so it is synchronized with usePrivateStorageTiers
completion and verifies only the mount-triggered query.
In `@libs/ui-components/src/pages/admin/StorageTiersListPage.test.tsx`:
- Around line 186-199: The test around “keeps the Create tier action available
when the tier list fails to load” must first verify that the storage-tier
request was invoked and failed, then assert the “Create tier” button remains
present. Track the onStorageTierList invocation or await the rendered error
state before checking the button, so the assertion cannot pass without
exercising the failure path.
---
Outside diff comments:
In `@libs/ui-components/src/pages/admin/StorageTiersListPage.test.tsx`:
- Around line 87-93: Extend the test “renders the STATUS column via
StorageTierStatusLabel” to assert that the table header is labeled “Status” and
that it appears immediately after the “Name” header, while preserving the
existing checks for the two rendered “Active” labels.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dadcbaaf-9c29-4a61-9994-7302e917dc19
📒 Files selected for processing (4)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/pages/admin/StorageManagementPage.test.tsxlibs/ui-components/src/pages/admin/StorageTiersListPage.test.tsxlibs/ui-components/src/pages/admin/StorageTiersListPage.tsx
💤 Files with no reviewable changes (1)
- libs/i18n/locales/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/pages/admin/StorageTiersListPage.tsx
…rs-list-page # Conflicts: # apps/app-frontend/src/shell/StorageRoutes.test.tsx
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, ElayAharoni The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
OSAC-3604: Storage Tiers list page
Jira: https://redhat.atlassian.net/browse/OSAC-3604
Story type: [UI]
Summary
Replaces the Tiers tab's placeholder (added by OSAC-3598) with a real list page: a table of existing storage tiers with resolved backend names, protocols, and lifecycle state, plus Create/Edit/Delete actions. Backend name resolution is scoped to exactly the backend IDs referenced on the rendered page via the existing
storageBackendIdsFilter, not an unbounded fetch of every registered backend. No backend/API changes — this consumes hooks already merged in OSAC-3597/3603.Changes
New components (
libs/ui-components/src/components/Storage/)StorageTierStatusLabel— wraps the sharedResourceStatusLabelforStorageTierState(ACTIVE → green "Active", UNSPECIFIED → grey "Unspecified"). NamedStorageTierStatusLabel, notStorageTierStateLabelas the story/design doc say — a deliberate choice to match this codebase's existing*StatusLabelconvention (SecurityGroupStatusLabel,TenantStatusLabel, etc.) instead of introducing the first*StateLabel.StorageTierActionsMenu/StorageTierDeleteConfirmModal— row-actions kebab (Edit/Delete) and delete confirmation, structural copies ofIdentityProviderActionsMenu/TenantDeleteConfirmModal.List page (
libs/ui-components/src/pages/admin/)StorageTiersListPage— the new list page, wired intoStorageManagementPage's Tiers tab in place of the old placeholder.Routing (
apps/app-frontend/src/shell/StorageRoutes.tsx)tiers/createandtiers/:id/editroutes (currently pointing atStoragePlaceholder, mirroring the existingbackends/create/backends/:id/editpattern) — the actual create/edit tier forms are separate, later stories.mountOnEnter/unmountOnExittoStorageManagementPage'sTabs: PatternFly mounts both tab bodies by default, so without this, visiting the Backends tab was silently mounting and data-fetching through the new Tiers page too.Test infrastructure (
libs/ui-components/src/test-utils/createMockConnectTransport.ts)onStorageBackendList/onStorageTierDeleteoverride hooks (needed to test exact backend-id filter scoping and delete-driven row removal).MockTransportOverridesfields (onStorageBackendList,onStorageTierList,onStorageTierDelete) from the strict generatedMessagetype toMessageInitShape, fixing a latent type gap no prior test had exercised.Testing
StorageTierStatusLabel,StorageTierDeleteConfirmModal,StorageTiersListPage,StorageManagementPage, andStorageRoutes. Covers: row rendering (single and multi-backend-association tiers), the backend-id-fallback path, exact scoping of the backend lookup filter (proven via exclusion of an unreferenced backend), empty state, Create/Edit navigation, delete success (row removal) and delete failure (FAILED_PRECONDITIONshown verbatim, row left in place), and that the inactive Backends tab no longer mounts/fetches the Tiers page.osac-test-infra, out of scope per the design doc)..artifacts/implement/OSAC-3604/05-validation-report.md.Acceptance Criteria
usePrivateStorageTiers().spec.backends[].backendIdto a name, scoping the lookup to exactly the backend IDs referenced across the rendered page viastorageBackendIdsFilter.backendIdthat fails to resolve falls back to displaying the raw ID rather than breaking the row.ResourceStatusLabel(ACTIVE → green "Active", UNSPECIFIED → grey "Unspecified"). Note: implemented asStorageTierStatusLabel, notStorageTierStateLabelas named in the story — see Changes above for rationale./admin/storage/tiers/createand/admin/storage/tiers/:id/editare added toStorageRoutes.tsx.useDeleteStorageTier(), with aFAILED_PRECONDITIONresponse shown verbatim and the row left in place.Summary by CodeRabbit