NO-ISSUE: Update to PatternFly 6.6, add theme/contrast prefs, and improve catalog and VM list pages - #139
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. WalkthroughThis PR adds routed catalog detail pages, typed multi-service catalog browsing, contrast-aware theme preferences, translated shell updates, and URL-backed filtering for networking and VM list pages. ChangesTyped catalog browsing and detail pages
UI preferences and shell updates
URL-backed list filters
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The PR introduces the described UI, theming, routing, filtering, and dependency updates; no actionable merge-blocking risk remains based on the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant User
participant CatalogPage
participant Router
participant CatalogItemDetailPage
participant CatalogAPI
User->>CatalogPage: Open catalog
CatalogPage->>CatalogAPI: Load VM, cluster, and bare-metal catalogs
CatalogAPI-->>CatalogPage: Return typed items
User->>CatalogPage: Select a catalog item
CatalogPage->>Router: Navigate to /catalog/{kind}/{id}
Router->>CatalogItemDetailPage: Render detail route
CatalogItemDetailPage->>CatalogAPI: Fetch item by kind and id
CatalogAPI-->>CatalogItemDetailPage: Return item details
User->>CatalogItemDetailPage: Start create action
CatalogItemDetailPage->>Router: Navigate to resource create route
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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.
Actionable comments posted: 8
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/components/catalog/CatalogItemListSection.tsx (1)
45-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe spinner label reads "Loading undefined" without a title.
titleis now optional, andCatalogPagerendersCatalogItemListSectionwithout it. The template literal then produces the aria-labelLoading undefined. Screen readers announce that text. Use a fallback label.🐛 Proposed fix
- <Spinner aria-label={`Loading ${title}`} /> + <Spinner aria-label={title ? `Loading ${title}` : 'Loading'} />🤖 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/catalog/CatalogItemListSection.tsx` around lines 45 - 51, Update the loading Spinner in CatalogItemListSection to use a meaningful fallback when the optional title is absent, ensuring its aria-label never renders “Loading undefined” while preserving the existing title-specific label when provided.
🧹 Nitpick comments (5)
libs/ui-components/src/api/v1/baremetal-instance.ts (1)
45-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid sharing the list query key when the ID is blank.
If
trimmedIdis empty,apiQueryKey('v1/baremetal_instance_catalog_items', undefined)produces['v1/baremetal_instance_catalog_items']. That is the exact key used byuseBareMetalInstanceCatalogItems. The detail query is disabled in that case, so no request is sent, but the disabled observer still reads the list cache entry and runsselect: (data) => data.objecton a list response. Keep the detail cache namespace separate.♻️ Proposed key separation
- queryKey: apiQueryKey( - 'v1/baremetal_instance_catalog_items', - trimmedId ? [trimmedId] : undefined, - ), + queryKey: apiQueryKey('v1/baremetal_instance_catalog_items', [trimmedId || 'detail']),🤖 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/api/v1/baremetal-instance.ts` around lines 45 - 57, Update useBareMetalInstanceCatalogItem so its queryKey always uses a distinct detail-query namespace, including when trimmedId is empty, rather than passing undefined to apiQueryKey and colliding with useBareMetalInstanceCatalogItems. Preserve the existing ID-specific key behavior and disabled state for blank IDs.libs/ui-components/src/pages/tenant/CatalogPage.tsx (2)
179-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the cast by making the search filter generic.
filterCatalogItemsBySearchis typed(items: CatalogItem[]) => CatalogItem[], so the result needsas CatalogItemWithType[]. A generic signature keeps thetypefield in the return type.♻️ Proposed generic signature
-export const filterCatalogItemsBySearch = (items: CatalogItem[], search: string): CatalogItem[] => { +export const filterCatalogItemsBySearch = <T extends CatalogItem>( + items: T[], + search: string, +): T[] => {Then the call site needs no cast:
- const filteredItems = useMemo(() => - filterCatalogItemsBySearch(filterCatalogItemsByTypes(data, typeFilters), search) as CatalogItemWithType[], - [search, data, typeFilters]); + const filteredItems = useMemo( + () => filterCatalogItemsBySearch(filterCatalogItemsByTypes(data, typeFilters), search), + [search, data, typeFilters], + );🤖 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/tenant/CatalogPage.tsx` around lines 179 - 181, Update filterCatalogItemsBySearch to be generic over the item subtype so it accepts and returns the same specific item type, preserving CatalogItemWithType.type through filtering. Then remove the as CatalogItemWithType[] cast from the filteredItems useMemo call while keeping its existing filtering behavior.
148-162: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe search input drops whitespace-only input.
setSearchdeletes thesearchparameter when the trimmed value is empty. TheSearchInputvalue comes from the URL. If the user types only spaces, the characters disappear from the field. Store the raw value and trim only for filtering.♻️ Proposed change
- const trimmed = value.trim(); - if (!trimmed) { + if (!value) { next.delete(SEARCH_PARAM); } else { next.set(SEARCH_PARAM, value); }Also applies to: 220-227
🤖 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/tenant/CatalogPage.tsx` around lines 148 - 162, Update setSearch in CatalogPage so whitespace-only input is preserved in the URL by checking the raw value for emptiness and storing the raw value unchanged; leave trimming to the search filtering logic, including the corresponding search-parameter update at the other referenced location.libs/ui-components/src/components/catalog/details/CatalogItemDetailContent.test.tsx (1)
31-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the
spec.prefixed field paths, and rename the test.All fixture paths are un-prefixed, so the test never exercises
normalizeCatalogFieldPath. TheCatalogPage.test.tsxfixture usesspec.image.source_ref, which means both shapes occur. Add a fixture field such asspec.coresand assert the resource chip renders. Also rename "shows drawer-era details" because the drawer no longer exists.Also applies to: 79-81
🤖 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/catalog/details/CatalogItemDetailContent.test.tsx` around lines 31 - 64, Update the fixture in the relevant CatalogItemDetailContent test to include a spec.-prefixed field path such as spec.cores, and assert that its resource chip renders so normalizeCatalogFieldPath is exercised for both path shapes. Rename the test currently describing “drawer-era details” to reflect the current detail UI rather than the removed drawer.libs/ui-components/src/components/catalogProvision/catalogFieldDefinition.ts (1)
155-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNormalization is correct, but the type predicate now narrows wrongly.
isCatalogItemResourceFieldPath('spec.cores')returnstrue, and TypeScript then narrowspathto'cores'. The raw value is still'spec.cores'.formatCatalogResourcePartalready works around this with a cast of the normalized path. Returnbooleanand expose a normalizing lookup instead, so callers cannot index by the un-normalized path.♻️ Optional signature change
-export const isCatalogItemResourceFieldPath = ( - path: string, -): path is CatalogItemResourceFieldPath => { - return catalogItemResourceFieldPathSet.has(normalizeCatalogFieldPath(path)); -}; +export const catalogItemResourceFieldPath = ( + path: string, +): CatalogItemResourceFieldPath | undefined => { + const normalized = normalizeCatalogFieldPath(path); + return catalogItemResourceFieldPathSet.has(normalized) + ? (normalized as CatalogItemResourceFieldPath) + : undefined; +}; + +export const isCatalogItemResourceFieldPath = (path: string): boolean => + catalogItemResourceFieldPath(path) !== undefined;🤖 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/catalogProvision/catalogFieldDefinition.ts` around lines 155 - 170, Change isCatalogItemResourceFieldPath to return a plain boolean instead of a type predicate, since it normalizes the input without changing the raw path value. Add a separate normalizing lookup helper that returns the normalized CatalogItemResourceFieldPath for callers needing typed indexing, and update formatCatalogResourcePart to use that helper rather than casting the normalized path.
🤖 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 `@apps/app-frontend/index.html`:
- Around line 15-26: Normalize the persisted theme and contrast values in the
inline initialization script before evaluating system preferences or toggling
classes. Update the logic around the theme and contrast storage reads to match
use-theme.ts: treat unsupported values as “system,” then apply the existing
dark, high-contrast, and glass class behavior consistently.
- Around line 8-28: Update the theme/contrast boot script in index.html to
satisfy a strict script-src CSP by moving it into an external JavaScript asset
or adding its exact content hash/nonce to the production CSP. Ensure the chosen
approach works with the production Go server, and do not use unsafe-inline or
unsafe-eval.
In `@libs/ui-components/src/components/catalog/CatalogItemCard.tsx`:
- Around line 34-45: Replace the local catalogItemCreatePath if-chain in
libs/ui-components/src/components/catalog/CatalogItemCard.tsx:34-45 with an
import of the shared switch-based catalogItemCreatePath helper. In
libs/ui-components/src/components/catalog/details/CatalogItemDetails.tsx:15-33,
retain the label mapping in getCatalogCreateAction but obtain the route through
that shared helper; update both sites to use the centralized mapping for all
CatalogItemKind values.
In `@libs/ui-components/src/components/catalog/details/CatalogItemDetailPage.tsx`:
- Around line 15-18: Update useCatalogItemByKind so the cluster ID is trimmed
before being passed to useClusterCatalogItem, preventing whitespace-only values
from enabling the query while preserving the existing kind-based selection
behavior.
In `@libs/ui-components/src/pages/networking/VirtualNetworksListPage.tsx`:
- Around line 60-73: Update the search state and handlers in
VirtualNetworksListPage’s SearchInput to use useSearchParams instead of local
component state, matching the parameter-preserving setter pattern used by
SecurityGroupsListPage. Read the search value from the URL and preserve existing
query parameters while updating or clearing the virtual-network search
parameter.
In `@libs/ui-components/src/pages/tenant/CatalogPage.tsx`:
- Around line 232-249: Update the empty-state branching in CatalogPage so the
service-selection prompt is shown only when typeFilters is empty and data
contains items; when the catalog data is empty, render the “No catalog items
found” state with the “No published catalog items are available yet.” body. In
CatalogPage.test.tsx, update the no-items transport case to assert that heading
and body text.
In `@libs/ui-components/src/pages/tenant/VmListPage.tsx`:
- Around line 90-105: Update the status-filter synchronization logic around
STATUS_FILTER_PARAM and serializeStatusFilters so clearing all selections
preserves an explicit empty status query parameter instead of deleting it.
Ensure the initialization guard recognizes that empty parameter as intentionally
initialized, preventing a reload or remount from restoring default statuses.
- Around line 171-172: Update the showEmptyState condition in VmListPage so “No
virtual machines yet” is shown only when the total VM inventory is empty, using
the unfiltered inventory count rather than filteredVms.length. Apply the same
condition to the corresponding empty-state rendering branch around the secondary
occurrence, preserving the filter-empty message when VMs exist but none match
the selected status.
---
Outside diff comments:
In `@libs/ui-components/src/components/catalog/CatalogItemListSection.tsx`:
- Around line 45-51: Update the loading Spinner in CatalogItemListSection to use
a meaningful fallback when the optional title is absent, ensuring its aria-label
never renders “Loading undefined” while preserving the existing title-specific
label when provided.
---
Nitpick comments:
In `@libs/ui-components/src/api/v1/baremetal-instance.ts`:
- Around line 45-57: Update useBareMetalInstanceCatalogItem so its queryKey
always uses a distinct detail-query namespace, including when trimmedId is
empty, rather than passing undefined to apiQueryKey and colliding with
useBareMetalInstanceCatalogItems. Preserve the existing ID-specific key behavior
and disabled state for blank IDs.
In
`@libs/ui-components/src/components/catalog/details/CatalogItemDetailContent.test.tsx`:
- Around line 31-64: Update the fixture in the relevant CatalogItemDetailContent
test to include a spec.-prefixed field path such as spec.cores, and assert that
its resource chip renders so normalizeCatalogFieldPath is exercised for both
path shapes. Rename the test currently describing “drawer-era details” to
reflect the current detail UI rather than the removed drawer.
In
`@libs/ui-components/src/components/catalogProvision/catalogFieldDefinition.ts`:
- Around line 155-170: Change isCatalogItemResourceFieldPath to return a plain
boolean instead of a type predicate, since it normalizes the input without
changing the raw path value. Add a separate normalizing lookup helper that
returns the normalized CatalogItemResourceFieldPath for callers needing typed
indexing, and update formatCatalogResourcePart to use that helper rather than
casting the normalized path.
In `@libs/ui-components/src/pages/tenant/CatalogPage.tsx`:
- Around line 179-181: Update filterCatalogItemsBySearch to be generic over the
item subtype so it accepts and returns the same specific item type, preserving
CatalogItemWithType.type through filtering. Then remove the as
CatalogItemWithType[] cast from the filteredItems useMemo call while keeping its
existing filtering behavior.
- Around line 148-162: Update setSearch in CatalogPage so whitespace-only input
is preserved in the URL by checking the raw value for emptiness and storing the
raw value unchanged; leave trimming to the search filtering logic, including the
corresponding search-parameter update at the other referenced location.
🪄 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: d601fbd9-4b8f-4820-bb68-ee26eadb50a4
⛔ Files ignored due to path filters (2)
libs/ui-components/src/assets/RH-OSAC.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
apps/app-frontend/index.htmlapps/app-frontend/package.jsonapps/app-frontend/src/shell/AppShell.tsxapps/app-frontend/src/shell/ShellMasthead.tsxapps/app-frontend/src/shell/ShellSidebar.tsxlibs/i18n/locales/en/translation.jsonlibs/ui-components/src/api/v1/baremetal-instance.tslibs/ui-components/src/components/ErrorBoundary/ErrorBoundary.tsxlibs/ui-components/src/components/UserPreferences/UserPreferencesModal.tsxlibs/ui-components/src/components/catalog/CatalogItemCard.tsxlibs/ui-components/src/components/catalog/CatalogItemDetailContent.tsxlibs/ui-components/src/components/catalog/CatalogItemDetailDrawer.csslibs/ui-components/src/components/catalog/CatalogItemDetailDrawer.tsxlibs/ui-components/src/components/catalog/CatalogItemListSection.tsxlibs/ui-components/src/components/catalog/catalogItemDisplay.tslibs/ui-components/src/components/catalog/details/CatalogFieldEditabilityLabel.tsxlibs/ui-components/src/components/catalog/details/CatalogItemDetailContent.test.tsxlibs/ui-components/src/components/catalog/details/CatalogItemDetailContent.tsxlibs/ui-components/src/components/catalog/details/CatalogItemDetailPage.tsxlibs/ui-components/src/components/catalog/details/CatalogItemDetails.tsxlibs/ui-components/src/components/catalogProvision/catalogFieldDefinition.tslibs/ui-components/src/hooks/use-session.tsxlibs/ui-components/src/hooks/use-theme.tslibs/ui-components/src/pages/networking/SecurityGroupsListPage.tsxlibs/ui-components/src/pages/networking/VirtualNetworksListPage.tsxlibs/ui-components/src/pages/tenant/CatalogPage.test.tsxlibs/ui-components/src/pages/tenant/CatalogPage.tsxlibs/ui-components/src/pages/tenant/VmListPage.csslibs/ui-components/src/pages/tenant/VmListPage.tsx
💤 Files with no reviewable changes (4)
- libs/ui-components/src/pages/tenant/VmListPage.css
- libs/ui-components/src/components/catalog/CatalogItemDetailDrawer.tsx
- libs/ui-components/src/components/catalog/CatalogItemDetailContent.tsx
- libs/ui-components/src/components/catalog/CatalogItemDetailDrawer.css
7e2e985 to
cb841cf
Compare
d4e87eb to
f237bc9
Compare
f237bc9 to
78a1355
Compare
78a1355 to
c743714
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, 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 |
rawagner
changed the title
|
@jeff-phillips-18: This pull request references PFRFE-37 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 feature request to target the "5.1.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. |
jeff-phillips-18
changed the title
|
@jeff-phillips-18: This pull request explicitly references no jira issue. 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. |
rawagner
changed the title
|
@jeff-phillips-18: This pull request references PFRFE-37 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 feature request to target the "5.1.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. |
rawagner
changed the title
|
@jeff-phillips-18: This pull request explicitly references no jira issue. 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. |
3 participants
Summary
index.htmlthat applies the saved theme before React boots/catalog/:kind/:id, supporting VM, cluster, and bare metal catalog items with a create actionreact-svgDemo
Kapture.2026-08-12.at.14.00.11.mp4
Test plan
Summary by CodeRabbit
New Features
Improvements