Skip to content

OSAC-2932: [UI] Catalog management list page with resource type tabs - #100

Merged
openshift-merge-bot[bot] merged 15 commits into
osac-project:mainfrom
ElayAharoni:OSAC-2932-catalog-management-list-page
Jul 28, 2026
Merged

OSAC-2932: [UI] Catalog management list page with resource type tabs#100
openshift-merge-bot[bot] merged 15 commits into
osac-project:mainfrom
ElayAharoni:OSAC-2932-catalog-management-list-page

Conversation

@ElayAharoni

@ElayAharoni ElayAharoni commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

OSAC-2932: [UI] Catalog management list page with resource type tabs

Jira: https://redhat.atlassian.net/browse/OSAC-2932
Story type: [UI]

Summary

  • Adds CatalogManagementListPage, the admin catalog-management page with three PatternFly Tabs (Clusters, Virtual Machines, Bare Metal), wired into the existing /admin/catalog route from OSAC-2931
  • Each tab shows a searchable, publication-filterable Gallery of CatalogItemCards with a scope badge (General/Organization/Project), publication status, and a publish/unpublish toggle
  • CSP Admin sees all items (published or not) via the private API; Tenant Admin sees their tenant's items via the public API
  • Extends shared components (CatalogItemCard, CatalogItemListSection) with optional admin-only slots — fully backward-compatible with the existing tenant-facing CatalogPage

Changes

API hooks (libs/ui-components/src/api/v1/)

  • New useAdminClusterCatalogItems, useAdminComputeInstanceCatalogItems, useAdminBareMetalInstanceCatalogItems — select the public or private API client based on useSession().role
  • New useAdminSet*CatalogItemPublished mutation hooks per kind, sending updateMask: { paths: ['published'] }
  • 3 new ApiRoute entries to isolate the admin private-client query cache from the tenant-facing public-client cache

Shared types/components (libs/ui-components/src/components/catalog/)

  • CatalogItem widened to include osac.private.v1.* types alongside the existing public ones
  • New catalogItemScope() + SHARED_TENANT constant to derive scope (General/Organization/Project) for both roles — resolves the design doc's own unresolved "scope visibility in public API responses" open question by using metadata.tenant
  • CatalogItemIcon widened to render the correct icon for private-v1 kinds
  • CatalogItemCard gained optional scopeBadge/statusLabel/publishToggle slots; CatalogItemListSection gained an optional renderCardAddons prop

New admin components (libs/ui-components/src/components/catalogManagement/)

  • CatalogItemScopeBadge, CatalogItemStatusLabel, CatalogItemPublishToggle

Page (libs/ui-components/src/pages/admin/)

  • CatalogManagementListPage + CatalogManagementTabPanel (per-tab toolbar, gallery, empty state)
  • apps/app-frontend/src/shell/AdminCatalogRoutes.tsx: wired the new page into the previously-placeholder index route

Known limitation (not fixable in this repo)

The public catalog-items List RPC in fulfillment-service unconditionally filters to published = true for every caller — verified directly against the backend source, no role check exists. This means a Tenant Admin's "Unpublished"/"All" filter won't show unpublished items, and unpublishing their own item via the new toggle makes it disappear from this page entirely. The osac-ui implementation here is correct against the documented design; the backend doesn't yet honor it. Filed as OSAC-3121.

Testing

  • Unit tests: 55 new test cases across 11 files — role-based hook client selection, catalogItemScope()'s full decision table, icon rendering for all 6 kinds, new component behavior (including a click-isolation/CSS-stacking-order regression test for the publish toggle), and full end-to-end page behavior (tab switching with independent per-tab data, search, publication filtering, navigation, empty state)
  • Integration tests: N/A — frontend-only, tested against mocked Connect transports (this repo's established pattern; E2E coverage tracked separately in OSAC-2935)
  • Coverage: Comprehensive at the behavioral level (no numeric coverage tooling configured in this repo)

Test plan

  • pnpm run typecheck passes
  • pnpm run lint passes (ESLint + Prettier + i18n sync)
  • pnpm test passes (390/391 — 1 pre-existing, unrelated flaky test in ClusterCreatePage.test.tsx, confirmed to pass in isolation and untouched by this PR)
  • Manually verified locally against kind-dev as CSP Admin

Acceptance Criteria

  • AC-1: The list page displays three PatternFly Tabs: Clusters, Virtual Machines, Bare Metal
  • AC-2: Each tab shows a PatternFly Gallery of CatalogItemCard cards (same layout as the tenant CatalogPage)
  • AC-3: Each card shows resource type icon, title, description, resource spec labels, scope badge, publication status, and a publish/unpublish Switch toggle
  • AC-4: Each tab has a "Create" button that navigates to the kind-specific create route
  • AC-5: Search by name uses client-side filtering via filterCatalogItemsBySearch()
  • AC-6: Publication status filter (All / Published / Unpublished) is available as a ToggleGroup (UI complete; see known limitation above for Tenant Admin runtime behavior)
  • AC-7: CSP Admin uses the private API and sees "General"/"Organization: {tenant name}" scope badges
  • AC-8: Tenant Admin uses the public API and sees "General"/"Organization"/"Project: {project name}" scope badges (badge logic complete; see known limitation above for unpublished-item visibility)
  • AC-9: Clicking a card navigates to the detail page
  • AC-10: All user-visible strings are defined as i18next translation keys

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

  • New Features
    • Added a dedicated Catalog Management page with tabs for Clusters, Virtual Machines, and Bare Metal.
    • Enabled catalog search and publication-status filtering.
    • Added scope badges (General, Organization, Project) and publication status labels (Published/Unpublished).
    • Improved role-aware admin views with creation and detail navigation, plus clearer empty states.
  • Bug Fixes
    • Fixed Bare Metal catalog listing behavior to use proper query parameters and avoid unnecessary requests when disabled.
  • Chores
    • Expanded UI tests and English translations for the new catalog management labels and messages.
image

the main difference here is in the option to publish or unpublish global catalog items.
the tenant admin can publish/unpublish only tenant scope catalog items

…views

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>
…ggle)

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…ItemPublishToggle components

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
… CatalogItemCard

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>
@openshift-ci-robot

openshift-ci-robot commented Jul 26, 2026

Copy link
Copy Markdown

@ElayAharoni: This pull request references OSAC-2932 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.

Details

In response to this:

OSAC-2932: [UI] Catalog management list page with resource type tabs

Jira: https://redhat.atlassian.net/browse/OSAC-2932
Story type: [UI]

Summary

  • Adds CatalogManagementListPage, the admin catalog-management page with three PatternFly Tabs (Clusters, Virtual Machines, Bare Metal), wired into the existing /admin/catalog route from OSAC-2931
  • Each tab shows a searchable, publication-filterable Gallery of CatalogItemCards with a scope badge (General/Organization/Project), publication status, and a publish/unpublish toggle
  • CSP Admin sees all items (published or not) via the private API; Tenant Admin sees their tenant's items via the public API
  • Extends shared components (CatalogItemCard, CatalogItemListSection) with optional admin-only slots — fully backward-compatible with the existing tenant-facing CatalogPage

Changes

API hooks (libs/ui-components/src/api/v1/)

  • New useAdminClusterCatalogItems, useAdminComputeInstanceCatalogItems, useAdminBareMetalInstanceCatalogItems — select the public or private API client based on useSession().role
  • New useAdminSet*CatalogItemPublished mutation hooks per kind, sending updateMask: { paths: ['published'] }
  • 3 new ApiRoute entries to isolate the admin private-client query cache from the tenant-facing public-client cache

Shared types/components (libs/ui-components/src/components/catalog/)

  • CatalogItem widened to include osac.private.v1.* types alongside the existing public ones
  • New catalogItemScope() + SHARED_TENANT constant to derive scope (General/Organization/Project) for both roles — resolves the design doc's own unresolved "scope visibility in public API responses" open question by using metadata.tenant
  • CatalogItemIcon widened to render the correct icon for private-v1 kinds
  • CatalogItemCard gained optional scopeBadge/statusLabel/publishToggle slots; CatalogItemListSection gained an optional renderCardAddons prop

New admin components (libs/ui-components/src/components/catalogManagement/)

  • CatalogItemScopeBadge, CatalogItemStatusLabel, CatalogItemPublishToggle

Page (libs/ui-components/src/pages/admin/)

  • CatalogManagementListPage + CatalogManagementTabPanel (per-tab toolbar, gallery, empty state)
  • apps/app-frontend/src/shell/AdminCatalogRoutes.tsx: wired the new page into the previously-placeholder index route

Known limitation (not fixable in this repo)

The public catalog-items List RPC in fulfillment-service unconditionally filters to published = true for every caller — verified directly against the backend source, no role check exists. This means a Tenant Admin's "Unpublished"/"All" filter won't show unpublished items, and unpublishing their own item via the new toggle makes it disappear from this page entirely. The osac-ui implementation here is correct against the documented design; the backend doesn't yet honor it. Filed as OSAC-3121.

Testing

  • Unit tests: 55 new test cases across 11 files — role-based hook client selection, catalogItemScope()'s full decision table, icon rendering for all 6 kinds, new component behavior (including a click-isolation/CSS-stacking-order regression test for the publish toggle), and full end-to-end page behavior (tab switching with independent per-tab data, search, publication filtering, navigation, empty state)
  • Integration tests: N/A — frontend-only, tested against mocked Connect transports (this repo's established pattern; E2E coverage tracked separately in OSAC-2935)
  • Coverage: Comprehensive at the behavioral level (no numeric coverage tooling configured in this repo)

Test plan

  • pnpm run typecheck passes
  • pnpm run lint passes (ESLint + Prettier + i18n sync)
  • pnpm test passes (390/391 — 1 pre-existing, unrelated flaky test in ClusterCreatePage.test.tsx, confirmed to pass in isolation and untouched by this PR)
  • Manually verified locally against kind-dev as CSP Admin

Acceptance Criteria

  • AC-1: The list page displays three PatternFly Tabs: Clusters, Virtual Machines, Bare Metal
  • AC-2: Each tab shows a PatternFly Gallery of CatalogItemCard cards (same layout as the tenant CatalogPage)
  • AC-3: Each card shows resource type icon, title, description, resource spec labels, scope badge, publication status, and a publish/unpublish Switch toggle
  • AC-4: Each tab has a "Create" button that navigates to the kind-specific create route
  • AC-5: Search by name uses client-side filtering via filterCatalogItemsBySearch()
  • AC-6: Publication status filter (All / Published / Unpublished) is available as a ToggleGroup (UI complete; see known limitation above for Tenant Admin runtime behavior)
  • AC-7: CSP Admin uses the private API and sees "General"/"Organization: {tenant name}" scope badges
  • AC-8: Tenant Admin uses the public API and sees "General"/"Organization"/"Project: {project name}" scope badges (badge logic complete; see known limitation above for unpublished-item visibility)
  • AC-9: Clicking a card navigates to the detail page
  • AC-10: All user-visible strings are defined as i18next translation keys

Assisted-by: Claude Code noreply@anthropic.com

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.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds a role-aware admin catalog management page with resource tabs, search and publication filters, private/public catalog hooks, scope and status labels, card metadata, empty states, and create/detail navigation.

Changes

Catalog management

Layer / File(s) Summary
Role-aware catalog APIs
libs/ui-components/src/api/types.ts, libs/ui-components/src/api/v1/baremetal-instance.ts, libs/ui-components/src/api/v1/baremetal-instance.test.ts, libs/ui-components/src/api/v1/cluster-catalog-item.ts, libs/ui-components/src/api/v1/cluster-catalog-item.test.ts, libs/ui-components/src/api/v1/compute-instance-catalog-item.ts, libs/ui-components/src/api/v1/compute-instance-catalog-item.test.ts, libs/ui-components/src/api/v1/private/baremetal-instance-catalog-item.ts, libs/ui-components/src/api/v1/private/baremetal-instance-catalog-item.test.ts, libs/ui-components/src/api/v1/private/cluster-catalog-item.ts, libs/ui-components/src/api/v1/private/cluster-catalog-item.test.ts, libs/ui-components/src/api/v1/private/compute-instance-catalog-item.ts, libs/ui-components/src/api/v1/private/compute-instance-catalog-item.test.ts, libs/ui-components/src/pages/tenant/CatalogPage.tsx, libs/ui-components/src/test-utils/TestProviders.tsx, libs/ui-components/src/test-utils/catalogHookTestHelpers.ts
Extends ApiRoute with private catalog base routes; adds public and private catalog list hooks with parameterized ListParams and enabled support; updates bare-metal hook signature; introduces renderHookWithProviders test helper and createCatalogHookTests factory for catalog hook validation.
Catalog item presentation and controls
libs/ui-components/src/components/catalog/catalogItemDisplay.ts, libs/ui-components/src/components/catalog/catalogItemDisplay.test.ts, libs/ui-components/src/components/catalog/CatalogItemCard.tsx, libs/ui-components/src/components/catalog/CatalogItemCard.test.tsx, libs/ui-components/src/components/catalogManagement/CatalogItemScopeBadge.tsx, libs/ui-components/src/components/catalogManagement/CatalogItemScopeBadge.test.tsx, libs/ui-components/src/components/catalogManagement/CatalogItemStatusLabel.tsx, libs/ui-components/src/components/catalogManagement/CatalogItemStatusLabel.test.tsx, libs/ui-components/src/icons.tsx, libs/i18n/locales/en/translation.json
Extends CatalogItem union to include private catalog item variants; adds PublicationFilter type and matchesPublicationFilter helper; introduces catalogItemScope, SHARED_TENANT, and CatalogItemScope union for role-aware scope derivation; adds CatalogItemScopeBadge and CatalogItemStatusLabel components; extends CatalogItemCard props to render scope and status metadata; updates icon mappings for private catalog kinds; adds translations for scope labels, status labels, and filter controls.
Catalog management page flow
apps/app-frontend/src/shell/AdminCatalogRoutes.tsx, libs/ui-components/src/pages/admin/CatalogManagementListPage.tsx, libs/ui-components/src/pages/admin/CatalogManagementListPage.test.tsx, libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx, libs/ui-components/src/pages/admin/ComputeInstanceCatalogManagementPanel.tsx, libs/ui-components/src/pages/admin/BareMetalInstanceCatalogManagementPanel.tsx
Replaces generic ListPage in admin catalog route with CatalogManagementListPage; introduces tabbed page component managing active tab, search query, and publication filter state; builds three resource-specific panels (cluster, compute-instance, bare-metal) that fetch role-specific catalog items, filter by search and publication status, render empty states, and provide create/detail navigation; includes comprehensive integration tests covering tab switching, search filtering, publication filtering, empty states, and navigation flows.

Estimated code review effort: 4 (Complex) | ~50 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Admin as AdminCatalogRoutes
  participant Page as CatalogManagementListPage
  participant Panel as CatalogManagementPanel
  participant API as CatalogListHook
  participant Card as CatalogItemCard

  Admin->>Page: Render catalog index
  Page->>Panel: Select resource tab and pass filters
  Panel->>API: Fetch role-specific catalog items
  API-->>Panel: Return catalog items
  Panel->>Card: Render filtered items with scope and status labels
Loading

Possibly related PRs

Suggested labels: lgtm

Suggested reviewers: batzionb

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed No hardcoded secrets found: scans showed no credential-shaped assignments, embedded auth URLs, or long base64/hex literals; password/pull-secret hits were only UI labels.
No-Weak-Crypto ✅ Passed No weak crypto, custom crypto, or non-constant-time secret/token comparisons appear in the PR-touched files.
No-Injection-Vectors ✅ Passed Scanned the modified catalog/admin files and found no dangerous sinks: no dangerouslySetInnerHTML, eval/exec, yaml.load, os.system, pickle.loads, or SQL string concatenation.
Container-Privileges ✅ Passed No privileged pod settings or allowPrivilegeEscalation found in charts/deployment, and the runtime Containerfile drops to USER 1001; root is only used in build stages.
No-Sensitive-Data-In-Logs ✅ Passed Searched all touched files; found no console/logger/trace calls or other logging that could expose sensitive data.
Ai-Attribution ✅ Passed AI use is attributed with Assisted-by: Claude Code trailers in the PR commits, and no AI-related Co-Authored-By: trailers were found.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: a UI catalog management list page with resource type tabs.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ElayAharoni
ElayAharoni marked this pull request as ready for review July 26, 2026 14:24
@openshift-ci
openshift-ci Bot requested review from eranco74 and rawagner July 26, 2026 14:24
@ElayAharoni
ElayAharoni requested review from batzionb and rawagner and removed request for eranco74 and rawagner July 26, 2026 14:24

@coderabbitai coderabbitai 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.

Actionable comments posted: 5

🤖 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/api/v1/baremetal-instance.test.ts`:
- Around line 40-89: Deduplicate the identical
createTestTransport/renderWithSession harnesses in baremetal-instance.test.ts
(lines 40-89), cluster-catalog-item.test.ts (lines 40-89), and
compute-instance-catalog-item.test.ts (lines 40-89) by moving the shared
provider wiring into TestProviders or adding a parameterized
renderHookWithProviders helper; retain per-test service and route configuration
through parameters, then replace all three local wrappers with the shared
harness.

In `@libs/ui-components/src/api/v1/baremetal-instance.ts`:
- Around line 36-44: Update useBareMetalInstanceCatalogItems to accept
parameters in the sibling-hook order, using (params: ListParams = {}, enabled =
true), while preserving its existing queryKey, queryFn, select, and enabled
behavior.

In `@libs/ui-components/src/api/v1/cluster-catalog-item.ts`:
- Around line 31-35: Update the doc comment for the admin list hook to state
that Tenant Admins access their tenant’s items through the public API, which
returns only published catalog items; remove the claim that unpublished items
are visible. Keep the CSP Admin/private API behavior unchanged.
- Around line 36-69: Extract the duplicated role-aware logic from
useAdminClusterCatalogItems and useAdminSetClusterCatalogItemPublished into
shared createAdminCatalogListHook and createAdminPublishMutationHook factories.
Parameterize the factories with the base query key and public/private clients,
preserving providerAdmin selection, enabled handling, private query-key suffix,
published update mask, and matching query invalidation behavior; update the
cluster hooks to use them.

In
`@libs/ui-components/src/components/catalogManagement/CatalogItemStatusLabel.test.tsx`:
- Around line 14-17: Update the test case for CatalogItemStatusLabel with
published={false} to assert that the closest pf-v6-c-label element has the
pf-m-grey class, replacing the current negative assertion against pf-m-green.
🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: 0d5c6dc9-adae-4eb1-abd0-a99d3b1aedaf

📥 Commits

Reviewing files that changed from the base of the PR and between 51dccc5 and 9402235.

📒 Files selected for processing (27)
  • apps/app-frontend/src/shell/AdminCatalogRoutes.tsx
  • libs/i18n/locales/en/translation.json
  • libs/ui-components/src/api/types.ts
  • libs/ui-components/src/api/v1/baremetal-instance.test.ts
  • libs/ui-components/src/api/v1/baremetal-instance.ts
  • libs/ui-components/src/api/v1/cluster-catalog-item.test.ts
  • libs/ui-components/src/api/v1/cluster-catalog-item.ts
  • libs/ui-components/src/api/v1/compute-instance-catalog-item.test.ts
  • libs/ui-components/src/api/v1/compute-instance-catalog-item.ts
  • libs/ui-components/src/components/catalog/CatalogItemCard.css
  • libs/ui-components/src/components/catalog/CatalogItemCard.test.tsx
  • libs/ui-components/src/components/catalog/CatalogItemCard.tsx
  • libs/ui-components/src/components/catalog/CatalogItemListSection.test.tsx
  • libs/ui-components/src/components/catalog/CatalogItemListSection.tsx
  • libs/ui-components/src/components/catalog/catalogItemDisplay.test.ts
  • libs/ui-components/src/components/catalog/catalogItemDisplay.ts
  • libs/ui-components/src/components/catalogManagement/CatalogItemPublishToggle.test.tsx
  • libs/ui-components/src/components/catalogManagement/CatalogItemPublishToggle.tsx
  • libs/ui-components/src/components/catalogManagement/CatalogItemScopeBadge.test.tsx
  • libs/ui-components/src/components/catalogManagement/CatalogItemScopeBadge.tsx
  • libs/ui-components/src/components/catalogManagement/CatalogItemStatusLabel.test.tsx
  • libs/ui-components/src/components/catalogManagement/CatalogItemStatusLabel.tsx
  • libs/ui-components/src/icons.test.tsx
  • libs/ui-components/src/icons.tsx
  • libs/ui-components/src/pages/admin/CatalogManagementListPage.test.tsx
  • libs/ui-components/src/pages/admin/CatalogManagementListPage.tsx
  • libs/ui-components/src/pages/admin/CatalogManagementTabPanel.tsx

Comment thread libs/ui-components/src/api/v1/baremetal-instance.test.ts Outdated
Comment thread libs/ui-components/src/api/v1/baremetal-instance.ts Outdated
Comment thread libs/ui-components/src/api/v1/cluster-catalog-item.ts Outdated
Comment thread libs/ui-components/src/api/v1/cluster-catalog-item.ts Outdated
…, doc accuracy

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Comment thread libs/ui-components/src/icons.test.tsx Outdated
@@ -0,0 +1,25 @@
import CloudIcon from '@patternfly/react-icons/dist/esm/icons/cloud-icon';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does this test bring any value ? We dont need test for each file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — this only asserted a trivial 1:1 kind→icon lookup via exact SVG path comparison, low value. Removed the test file in 390f0cc.

* the Tenant Admin's own tenant are not visible through this hook (tracked as a backend limitation in
* OSAC-3121).
*/
export const useAdminBareMetalInstanceCatalogItems = (params: ListParams = {}, enabled = true) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets not have private and public hooks in one file.

Lets update the structure to

  • api/v1 - what we already have, for public APIs
  • api/v1/private - for private APIs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed and done — split into a plain public hook in api/v1/ and a plain private hook in the new api/v1/private/ for each of the three catalog item kinds (390f0cc).

Comment on lines +55 to +57
const isProviderAdmin = role === 'providerAdmin';
const publicResult = useBareMetalInstanceCatalogItems(params, enabled && !isProviderAdmin);
const privateClient = useApiFetch(PrivateBareMetalInstanceCatalogItems);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this hooks should always work only with PrivateBareMetalInstanceCatalogItems

lets not check role here. The caller should be resposible for calling the right hook to get the expected resources back.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — deleted useAdminBareMetalInstanceCatalogItems (and the equivalent for cluster/compute-instance). The new admin panel components call the plain public or private hook directly based on role instead (390f0cc).

Comment on lines +68 to +71
const { role } = useSession();
const isProviderAdmin = role === 'providerAdmin';
const publicClient = useApiFetch(BareMetalInstanceCatalogItems);
const privateClient = useApiFetch(PrivateBareMetalInstanceCatalogItems);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here - lets not mix role + private/public

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

applies for all other catalog item hooks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same fix applied here — see the reply on the sibling comment (390f0cc).

Comment thread libs/ui-components/src/api/types.ts Outdated
Comment on lines +28 to +30
| 'v1/compute_instance_catalog_items_private'
| 'v1/cluster_catalog_items_private'
| 'v1/baremetal_instance_catalog_items_private';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe

Suggested change
| 'v1/compute_instance_catalog_items_private'
| 'v1/cluster_catalog_items_private'
| 'v1/baremetal_instance_catalog_items_private';
| 'v1/private/compute_instance_catalog_items
| 'v1/private/cluster_catalog_items'
| 'v1/private/baremetal_instance_catalog_items';

to mirror our proposed api/v1 hooks structure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed and applied (fixed the missing closing quote from the suggested diff) — 390f0cc.

Comment on lines 25 to +31
export type CatalogItem =
| ClusterCatalogItem
| BareMetalInstanceCatalogItem
| ComputeInstanceCatalogItem;
| ComputeInstanceCatalogItem
| PrivateClusterCatalogItem
| PrivateBareMetalInstanceCatalogItem
| PrivateComputeInstanceCatalogItem;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is getting a bit out of hand.

Lets split the components for specific ones - each type here is a separate component
each component loads the data, handles loading/error states (ListPageBody..) and renders the items.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Replaced the single generic CatalogManagementTabPanel with three concrete components (Cluster/ComputeInstance/BareMetalInstance), each calling its own hook and owning its own loading/error handling via ListPageBody. Kept the shared CatalogItem union and generic display helpers (search, scope, subtitle) since those are genuinely reused across all three panels and the tenant page (390f0cc).

| { level: 'organization'; name?: string }
| { level: 'project'; name: string };

const isPrivateCatalogItem = (item: CatalogItem): item is PrivateCatalogItem => 'tenant' in item;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is quite fragile check. Each object has $typeName which you can use to get the exact type - much better & safer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed — now checks item.$typeName.startsWith('osac.private.') (390f0cc).

return isProviderAdmin ? privateResult : publicResult;
};

export const useAdminSetBareMetalInstanceCatalogItemPublished = () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we should have a single hook for updating BM. Not for every operation - ie usePatchBareMetalInstance that is below. The caller then defines what gets updated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This hook is gone now — see the publish-toggle removal below, there's no longer a BM catalog-item mutation to consolidate.

Comment on lines +24 to +27
default: {
const exhaustiveCheck: never = scope;
return exhaustiveCheck;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and how would this exhaustiveCheck get rendered ? It would fail

a good default could be Label gray, text Unknown

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed — kept the compile-time exhaustiveness check (still fails the build if a scope level is added without updating this switch) but the runtime fallback now renders a grey 'Unknown' Label instead of the never value (390f0cc).

Comment on lines +42 to +44
const setClusterPublished = useAdminSetClusterCatalogItemPublished();
const setComputeInstancePublished = useAdminSetComputeInstanceCatalogItemPublished();
const setBareMetalPublished = useAdminSetBareMetalInstanceCatalogItemPublished();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these hooks are not used in this component, so why not call them in the child that actually uses them ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed and fixed — CatalogManagementListPage no longer calls any data hooks, it only owns the tab/search/filter UI state. Each of the three new panel components calls its own hooks (390f0cc).

onChange: (published: boolean) => void;
}

const CatalogItemPublishToggle = ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think this toggle may be breaking accessibility. How about after user clicks on the catalogitem - we show the drawer and in the drawer actions they can publish/unpublish.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Took this suggestion — removed the publish/unpublish toggle from this story's scope entirely (component, prop, CSS z-index workaround, and the mutation hooks all deleted). It moves to a detail-page action alongside Edit/Delete in OSAC-2933, which avoids the nested-interactive-control-in-a-clickable-card problem you're flagging. Updated both OSAC-2932 and OSAC-2933 Jira tickets to reflect the scope move (390f0cc).

…public/private hooks

Removes the card-level publish/unpublish toggle from this story's scope
per reviewer suggestion; it moves to a detail-page action in OSAC-2933
instead, avoiding a nested interactive control inside an already-clickable
card. Splits each admin catalog-item hook into a plain public hook
(api/v1/) and a plain private hook (api/v1/private/), with role-based
selection now living in the consuming panel component rather than inside
the hooks themselves. Replaces the single generic CatalogManagementTabPanel
with three concrete per-kind panels that own their own data fetching and
compose ListPageBody + Gallery + CatalogItemCard directly. Also fixes the
fragile 'tenant' in item type guard to use $typeName, gives the scope
badge's exhaustiveness fallback a safe rendered default instead of a
type-only never-check, and drops the low-value per-icon-kind test file.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…agement-list-page

# Conflicts:
#	libs/ui-components/src/api/v1/baremetal-instance.test.ts
#	libs/ui-components/src/api/v1/baremetal-instance.ts
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>

@coderabbitai coderabbitai 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.

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)
libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx (1)

1-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Three near-identical panel components — extract shared data/layout logic.

ClusterCatalogManagementPanel, ComputeInstanceCatalogManagementPanel, and BareMetalInstanceCatalogManagementPanel are ~95% copy-pasted (state derivation, search/publication filtering, control bar, empty state, gallery rendering) and differ only in the hook pair, title text, and route segment. This already shows early drift — the isSuccess-vs-isLoading empty-state rationale is documented only in the Cluster panel's comment, not the other two. A shared internal hook (e.g. useCatalogManagementPanelData(publicHook, privateHook, role, isActive, search, publicationFilter)) plus a shared presentational component parameterized by title, routeBase, and emptyMessages would keep the three files as thin, per-type wrappers (preserving the earlier requested split) while removing ~300 lines of duplicated logic and preventing future divergence.

  • libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx#L1-L160: extract the shared data-fetch/filter/render logic into a common hook/helper and keep this file as a thin per-type wrapper.
  • libs/ui-components/src/pages/admin/ComputeInstanceCatalogManagementPanel.tsx#L1-L163: adopt the same shared helper instead of re-duplicating the Cluster panel's logic.
  • libs/ui-components/src/pages/admin/BareMetalInstanceCatalogManagementPanel.tsx#L1-L163: adopt the same shared helper instead of re-duplicating the Cluster panel's logic.
🤖 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/ClusterCatalogManagementPanel.tsx` around
lines 1 - 160, Three catalog management panels duplicate the same data,
filtering, controls, empty state, and gallery rendering logic. In
libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx:1-160,
extract shared hook/presentational logic and retain only the cluster-specific
wrapper configuration. Update
libs/ui-components/src/pages/admin/ComputeInstanceCatalogManagementPanel.tsx:1-163
and
libs/ui-components/src/pages/admin/BareMetalInstanceCatalogManagementPanel.tsx:1-163
to use the shared implementation, parameterized by their hooks, title, route
base, and empty-state messages, while preserving the isSuccess-based empty-state
behavior.
libs/ui-components/src/pages/admin/CatalogManagementListPage.tsx (1)

15-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Use isLoading when gating admin catalog access by role.

useSession() is only safe inside SessionProvider, which is already mounted after the initial auth loading/error handling. The remaining race is updates to role from useOIDCLogin() while still loading; in that state this page falls back to the non-provider path. Add an isLoading boundary around rendering AdminCatalogRoutes/CatalogManagementListPage so /admin/catalog only renders when the selected role is final.

🤖 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/CatalogManagementListPage.tsx` around
lines 15 - 22, The admin catalog route currently renders before the role from
useOIDCLogin() is finalized. Update the admin catalog access gate surrounding
AdminCatalogRoutes and CatalogManagementListPage to include the session/auth
isLoading state, rendering those components only after loading completes while
preserving the existing role-based authorization 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/api/v1/baremetal-instance.test.ts`:
- Around line 21-55: Extract the repeated useBareMetalInstanceCatalogItems
fixture, fetch-success test, and disabled-fetch test into a shared parameterized
createCatalogHookTests factory. In
libs/ui-components/src/api/v1/baremetal-instance.test.ts:21-55, define and use
the factory; update
libs/ui-components/src/api/v1/cluster-catalog-item.test.ts:21-55 to pass
useClusterCatalogItems, ClusterCatalogItems, and tenantAdmin; update
libs/ui-components/src/api/v1/private/baremetal-instance-catalog-item.test.ts:22-56,
libs/ui-components/src/api/v1/private/cluster-catalog-item.test.ts:22-56, and
libs/ui-components/src/api/v1/private/compute-instance-catalog-item.test.ts:22-56
to invoke it with their respective private hook/service symbols and
providerAdmin. Preserve each test’s item fixture and existing assertions.

---

Outside diff comments:
In `@libs/ui-components/src/pages/admin/CatalogManagementListPage.tsx`:
- Around line 15-22: The admin catalog route currently renders before the role
from useOIDCLogin() is finalized. Update the admin catalog access gate
surrounding AdminCatalogRoutes and CatalogManagementListPage to include the
session/auth isLoading state, rendering those components only after loading
completes while preserving the existing role-based authorization behavior.

In `@libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx`:
- Around line 1-160: Three catalog management panels duplicate the same data,
filtering, controls, empty state, and gallery rendering logic. In
libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx:1-160,
extract shared hook/presentational logic and retain only the cluster-specific
wrapper configuration. Update
libs/ui-components/src/pages/admin/ComputeInstanceCatalogManagementPanel.tsx:1-163
and
libs/ui-components/src/pages/admin/BareMetalInstanceCatalogManagementPanel.tsx:1-163
to use the shared implementation, parameterized by their hooks, title, route
base, and empty-state messages, while preserving the isSuccess-based empty-state
behavior.
🪄 Autofix (Beta)

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: ASSERTIVE

Plan: Pro Plus

Run ID: 38fdf6e6-bbba-4726-b3ac-805c52363e84

📥 Commits

Reviewing files that changed from the base of the PR and between 9402235 and 390f0cc.

📒 Files selected for processing (22)
  • libs/ui-components/src/api/types.ts
  • libs/ui-components/src/api/v1/baremetal-instance.test.ts
  • libs/ui-components/src/api/v1/baremetal-instance.ts
  • libs/ui-components/src/api/v1/cluster-catalog-item.test.ts
  • libs/ui-components/src/api/v1/compute-instance-catalog-item.test.ts
  • libs/ui-components/src/api/v1/private/baremetal-instance-catalog-item.test.ts
  • libs/ui-components/src/api/v1/private/baremetal-instance-catalog-item.ts
  • libs/ui-components/src/api/v1/private/cluster-catalog-item.test.ts
  • libs/ui-components/src/api/v1/private/cluster-catalog-item.ts
  • libs/ui-components/src/api/v1/private/compute-instance-catalog-item.test.ts
  • libs/ui-components/src/api/v1/private/compute-instance-catalog-item.ts
  • libs/ui-components/src/components/catalog/CatalogItemCard.test.tsx
  • libs/ui-components/src/components/catalog/CatalogItemCard.tsx
  • libs/ui-components/src/components/catalog/catalogItemDisplay.ts
  • libs/ui-components/src/components/catalogManagement/CatalogItemScopeBadge.tsx
  • libs/ui-components/src/pages/admin/BareMetalInstanceCatalogManagementPanel.tsx
  • libs/ui-components/src/pages/admin/CatalogManagementListPage.test.tsx
  • libs/ui-components/src/pages/admin/CatalogManagementListPage.tsx
  • libs/ui-components/src/pages/admin/ClusterCatalogManagementPanel.tsx
  • libs/ui-components/src/pages/admin/ComputeInstanceCatalogManagementPanel.tsx
  • libs/ui-components/src/pages/tenant/CatalogPage.tsx
  • libs/ui-components/src/test-utils/TestProviders.tsx

Comment thread libs/ui-components/src/api/v1/baremetal-instance.test.ts
CodeRabbit flagged that the "fetches items" / "does not fetch when
disabled" test pair was copy-pasted across all five catalog-item list
hook test files (public and private, all three kinds), differing only in
hook/service/role. Extracts createCatalogHookTests, which owns the two
test bodies while each call site supplies its own concretely-typed
registerList callback, so the mock service registration stays fully
type-checked against the real Connect service descriptor.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…agement-list-page

# Conflicts:
#	libs/ui-components/src/api/types.ts
@openshift-ci

openshift-ci Bot commented Jul 28, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:
  • OWNERS [ElayAharoni,batzionb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ElayAharoni
ElayAharoni requested a review from rawagner July 28, 2026 07:31
@ElayAharoni
ElayAharoni removed the request for review from rawagner July 28, 2026 07:32
@openshift-merge-bot
openshift-merge-bot Bot merged commit 53f774c into osac-project:main Jul 28, 2026
7 checks passed
ElayAharoni added a commit to ElayAharoni/osac-ui that referenced this pull request Jul 30, 2026
After reverting OSAC-2932 (osac-project#100), the "Catalog management" nav item
still pointed at an empty placeholder page (the OSAC-2931 scaffold
that osac-project#100 built on top of). Remove the nav item, the now-empty
Administration nav section, the /admin/catalog route, and the
AdminCatalogRoutes placeholder so nothing links to dead UI.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
openshift-merge-bot Bot pushed a commit that referenced this pull request Jul 30, 2026
…pe tabs" (#100)

This reverts commit 53f774c, reversing
changes made to afbc45d391851d2cea2417e29fbc27977c8ff8d8.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants