OSAC-3780: Add provider-admin instance type routing, nav, and list page - #129
Conversation
Wire the provider-admin instance type pages into the admin shell, add the private instance type hook and lifecycle labels, and rebalance the list table for the reviewed browser layout. Assisted-by: Cursor <cursoragent@cursor.com> Signed-off-by: batzionb <brotman@redhat.com>
Align the Infrastructure nav "Instance types" label casing with the list page and create-page shell so they share one i18n key instead of two near-duplicate entries. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Wire a "Create instance type" action into the list page so the create-page shell (AC-4) is reachable from the UI, matching the pattern used by TenantListPage. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Extract the instance type table markup into AdminInstanceTypeTable so the list page stays focused on data wiring, add a richer empty state with an icon and guidance text, and fall back to the raw lifecycle state (or an em dash) instead of a generic 'Unspecified' label. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
Add a dedicated ResourceLifecycleLabel primitive instead of duplicating PatternFly Label rendering in InstanceTypeLifecycleLabel, default-export the component to match its siblings, show the raw state text for any lifecycle value outside the three designed states, add coming-soon messaging to the create-page shell, align the Lifecycle state column header casing, and drop an unrelated .gitignore change. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
|
@batzionb: This pull request references OSAC-3780 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: 42 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 (5)
WalkthroughThe PR adds private instance-type API access, admin list and create pages, lifecycle labels, translated UI text, and Infrastructure navigation. It also moves storage routes under ChangesInstance Type Administration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant AppShell
participant InstanceTypeRoutes
participant AdminInstanceTypeListPage
participant useAdminInstanceTypes
Admin->>AppShell: open Infrastructure instance types
AppShell->>InstanceTypeRoutes: match instance-types route
InstanceTypeRoutes->>AdminInstanceTypeListPage: render list page
AdminInstanceTypeListPage->>useAdminInstanceTypes: load instance types
useAdminInstanceTypes-->>AdminInstanceTypeListPage: return items and query state
AdminInstanceTypeListPage-->>Admin: render table or empty state
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/ui-components/src/components/Resource/ResourceLifecycleLabel.tsx (1)
7-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a
switchfor the lifecycle color mapping.Replace
LIFECYCLE_COLOR[lifecycle]with aswitchfunction. This keeps every lifecycle case in one readable control flow.Based on learnings: prefer a
switchstatement when translating a discriminant to props or a component.Proposed refactor
-const LIFECYCLE_COLOR: Record<LifecycleKind, LifecycleLabelColor> = { - active: 'green', - deprecated: 'orange', - obsolete: 'grey', - unspecified: 'grey', +const lifecycleColor = (lifecycle: LifecycleKind): LifecycleLabelColor => { + switch (lifecycle) { + case 'active': + return 'green'; + case 'deprecated': + return 'orange'; + case 'obsolete': + case 'unspecified': + return 'grey'; + } }; @@ - return <Label color={LIFECYCLE_COLOR[lifecycle]}>{text}</Label>; + return <Label color={lifecycleColor(lifecycle)}>{text}</Label>;🤖 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/Resource/ResourceLifecycleLabel.tsx` around lines 7 - 20, Replace the LIFECYCLE_COLOR lookup used by ResourceLifecycleLabel with a switch-based helper that maps each LifecycleKind case to its corresponding LifecycleLabelColor. Update the component to use this helper while preserving the existing colors for active, deprecated, obsolete, and unspecified lifecycles.Source: Learnings
🤖 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.
Nitpick comments:
In `@libs/ui-components/src/components/Resource/ResourceLifecycleLabel.tsx`:
- Around line 7-20: Replace the LIFECYCLE_COLOR lookup used by
ResourceLifecycleLabel with a switch-based helper that maps each LifecycleKind
case to its corresponding LifecycleLabelColor. Update the component to use this
helper while preserving the existing colors for active, deprecated, obsolete,
and unspecified lifecycles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: daa847c7-e4db-4002-a3b0-5dabc01bd1dd
📒 Files selected for processing (22)
apps/app-frontend/src/shell/AppShell.test.tsxapps/app-frontend/src/shell/AppShell.tsxapps/app-frontend/src/shell/InstanceTypeRoutes.test.tsxapps/app-frontend/src/shell/InstanceTypeRoutes.tsxapps/app-frontend/src/shell/StorageRoutes.test.tsxapps/app-frontend/src/shell/shellNav.test.tsapps/app-frontend/src/shell/shellNav.tslibs/i18n/locales/en/translation.jsonlibs/ui-components/src/api/types.tslibs/ui-components/src/api/v1/private/instance-type.test.tslibs/ui-components/src/api/v1/private/instance-type.tslibs/ui-components/src/components/InstanceType/AdminInstanceTypeCreatePage.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeListPage.test.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeListPage.tsxlibs/ui-components/src/components/InstanceType/AdminInstanceTypeTable.tsxlibs/ui-components/src/components/InstanceType/InstanceTypeLifecycleLabel.test.tsxlibs/ui-components/src/components/InstanceType/InstanceTypeLifecycleLabel.tsxlibs/ui-components/src/components/Resource/ResourceLifecycleLabel.test.tsxlibs/ui-components/src/components/Resource/ResourceLifecycleLabel.tsxlibs/ui-components/src/pages/admin/StorageManagementPage.test.tsxlibs/ui-components/src/pages/admin/StorageManagementPage.tsxlibs/ui-components/src/test-utils/createMockConnectTransport.ts
| @@ -0,0 +1,11 @@ | |||
| import { Route, Routes } from 'react-router-dom'; | |||
There was a problem hiding this comment.
should we have this file in ui-components/src/components/InstanceType ?
There was a problem hiding this comment.
InstanceTypeRoutes.tsx mirrors the existing TenantRoutes.tsx exactly — a thin wrapper composing page components, living alongside StorageRoutes.tsx and TenantRoutes.tsx in apps/app-frontend/src/shell/. All routing files live there because they depend on react-router-dom, an app-level dependency not otherwise used inside the framework-agnostic ui-components package. Moving it would break with the established convention and pull a routing dependency into a shared package.
Proposed response: "This follows the same pattern as TenantRoutes.tsx/StorageRoutes.tsx — all our routing wrappers live in apps/app-frontend/src/shell since they depend on react-router-dom, which ui-components doesn't otherwise use. Keeping it here for consistency.
There was a problem hiding this comment.
There is no consistency now. Some routes are defined in app, others in ui-components (for example <ClusterRoutes />).
I think it is better to place these route definitions into ui-components. The components there expect a specific route structure (as they use navigate with specific strings).
| <Truncate | ||
| content={(instanceType.metadata?.name || instanceType.id) | ||
| .replace(/\s+/g, ' ') | ||
| .trim()} | ||
| maxCharsDisplayed={INSTANCE_TYPE_NAME_PREVIEW_LENGTH} | ||
| omissionContent="..." | ||
| /> |
There was a problem hiding this comment.
would be nice to make this a shared component
There was a problem hiding this comment.
Done — extracted a generic TruncatedText primitive under libs/ui-components/src/components/Primitives/ and switched both the Name and Description columns to use it. See c1a49dc.
| const props = (): ResourceLifecycleLabelProps => { | ||
| switch (state) { | ||
| case InstanceTypeState.ACTIVE: | ||
| return { lifecycle: 'active', text: t('Active') }; | ||
| case InstanceTypeState.DEPRECATED: | ||
| return { lifecycle: 'deprecated', text: t('Deprecated') }; | ||
| case InstanceTypeState.OBSOLETE: | ||
| return { lifecycle: 'obsolete', text: t('Obsolete') }; | ||
| case InstanceTypeState.UNSPECIFIED: | ||
| case undefined: | ||
| return { lifecycle: 'unspecified', text: '—' }; | ||
| default: | ||
| return { lifecycle: 'unspecified', text: String(state) }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
consider changing this to a Record like https://github.com/osac-project/osac-ui/blob/main/libs/ui-components/src/components/IdentityProvider/IdentityProviderStatusLabel.tsx#L15-L34
which ensures a TS build check - anytime a new state is added in backend, the TS ensures this state is defined in our state map.
There was a problem hiding this comment.
Good call — switched to a Record<InstanceTypeState, ResourceLifecycleLabelProps>, matching IdentityProviderStatusLabel. Also updated the unknown-state fallback: rather than surfacing the raw backend value (e.g. 99), it now renders the same unspecified/em-dash treatment as a missing state, so we never show a state text that doesn't correspond to a real UI-designed lifecycle. Test updated accordingly in InstanceTypeLifecycleLabel.test.tsx. See 2bfb4cd.
There was a problem hiding this comment.
But seems like coderabbit wants the opposite?
#132 (comment)
There was a problem hiding this comment.
I would say we want the combination. We want to have the Record<*State, ResourceLifecycleLabelProps>to ensure our code is up-to-date with the generated types but also have a fallback in case our generated types are not up-to-date with backend. That fallback is currently missing in my code.
There was a problem hiding this comment.
WDYT the fallback should be?
There was a problem hiding this comment.
Do you think having Unspecified as the fallback is good enough?
…ecycle mapping Switch InstanceTypeLifecycleLabel to a Record<InstanceTypeState, ...> map, following the IdentityProviderStatusLabel pattern, so an unrecognized backend state falls back to the unspecified treatment instead of surfacing the raw backend value in the UI. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
… text Use the shared 'Unspecified' translation for the lifecycle label's fallback text instead of an em dash, matching IdentityProviderStatusLabel. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
…mitive Extract the whitespace-normalize-then-truncate pattern used by the Name and Description columns in AdminInstanceTypeTable into a shared TruncatedText primitive, so other tables can reuse it. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: batzionb <brotman@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, ElayAharoni, rawagner 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 |
4 participants
OSAC-3780: Add provider-admin instance type routing, nav, and list page
Jira: OSAC-3780
Story type: [UI]
Summary
Adds a provider-admin surface for browsing instance types: a new
Infrastructureadmin nav section with anInstance Typesentry, list and create routes, a dedicated private-API list hook, and a list page rendering Name, Lifecycle State, CPU cores, Memory, Description, and Created columns. As part of this change, the existing adminStorageentry was relocated under the sameInfrastructuresection so both resources share one grouping.Changes
Infrastructureadmin nav section withInstance Types(and relocatedStorage) entries; register/admin/infrastructure/instance-typesand/admin/infrastructure/instance-types/createroutes, scoped to provider-admin users only.useAdminInstanceTypes(privatev1/private/instance_typeshook), isolated from the tenant-facinguseInstanceTypesquery key.AdminInstanceTypeListPage(loading/empty/error states viaListPageBody),AdminInstanceTypeTable(extracted table with truncation and a richer empty state),AdminInstanceTypeCreatePage(breadcrumb + coming-soon shell, no form yet), and a new sharedResourceLifecycleLabelprimitive (green/orange/grey) used byInstanceTypeLifecycleLabelto render ACTIVE/DEPRECATED/OBSOLETE, falling back to the raw state value for any unrecognized state.Testing
Createdcolumn, and lifecycle label color mapping including the unknown-state fallback.Acceptance Criteria
/admin/infrastructure/instance-typesrenders the provider-admin list page/admin/infrastructure/instance-types/createrenders the create-page shellScreenshots
Empty state
Instance types with different lifecycles
Error state
Summary by CodeRabbit
New Features
Updates