CONSOLE-4946: Add Configuration tab to Node view#16124
Open
jeff-phillips-18 wants to merge 1 commit into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 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 "4.22.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. |
openshift-ci
Bot
added
component/core
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 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 "4.22.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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a FEATURE-flagged "Configuration" tab with extensible node sub-tabs, storage (local disks, mounted PVs) and machine (BMC, MachineConfigPool, Machine) pages, a multi-namespace watch hook (useAccessibleResources), refactors watch utilities to use it, updates imports, tests, and adds SDK extension types/docs for node sub-nav tabs. ChangesNode Management Feature Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.ts (1)
299-353:⚠️ Potential issue | 🟡 MinorCover the new non-admin watch path too.
These cases drive
useFlagwith onemockReturnValue, soKUBEVIRT_DYNAMICandFLAGS.CAN_LIST_NSalways resolve to the same boolean. That means the newuseK8sWatchResourcesaggregation branch inuseAccessibleResourcesnever runs here, and the permissions-aware behavior can regress without this suite noticing. Please add a case that returns different values per flag and asserts the per-namespace watch path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.ts` around lines 299 - 353, Add a test for useWatchVirtualMachineInstances that covers the non-admin/per-namespace watch path by making useFlagMock return different values for the two flags instead of a single mockReturnValue; specifically, use mockImplementation or mockReturnValueOnce so KUBEVIRT_DYNAMIC is true but FLAGS.CAN_LIST_NS is false (or vice versa) to trigger the useK8sWatchResources branch in useAccessibleResources, then assert that useK8sWatchResourcesMock was called with the per-namespace resources aggregation and that the hook returns the expected filtered VMIs for the node; locate the logic around useWatchVirtualMachineInstances, useFlagMock, useK8sWatchResourcesMock, and useAccessibleResources to add this case.frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.ts (1)
194-205:⚠️ Potential issue | 🟡 MinorAssert the machine watch as well.
This case only proves that the BareMetalHost request is issued. If the new Machine watch disappears, the test still passes even though
useWatchBareMetalHostcan no longer resolve the host for a node. Please add an expectation for the MachinegroupVersionKindrequest too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.ts` around lines 194 - 205, Test currently only asserts the BareMetalHost watch; update the test for useWatchBareMetalHost to also assert that useK8sWatchResourceMock was called for the Machine resource by adding an expectation that a call was made with isList: true and groupVersionKind: MachineGroupVersionKind (or the exact Machine GVK constant used in the code) and namespaced: true. Locate the test for useWatchBareMetalHost, keep the existing BareMetalHost assertion (BareMetalHostGroupVersionKind) and add a second expect(...) to verify the Machine watch via useK8sWatchResourceMock so the test fails if the Machine watch is removed.
🧹 Nitpick comments (12)
frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsx (1)
1-42: LGTM! Solid unit tests for the composition component.The tests correctly verify that
NodeStoragepasses the node object to its child components. The mock setup is clean andbeforeEachproperly clears mocks between tests.Consider adding a test that verifies both child components render together in a single assertion, which would catch any conditional rendering logic:
it('should render both LocalDisks and PersistentVolumes components', () => { render(<NodeStorage obj={mockNode} />); expect(screen.getByText('LocalDisks for test-node')).toBeInTheDocument(); expect(screen.getByText('PersistentVolumes for test-node')).toBeInTheDocument(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsx` around lines 1 - 42, Add a new test to NodeStorage.spec.tsx that renders the NodeStorage component and asserts both child mocks are present in a single test; specifically, create a test (e.g., "should render both LocalDisks and PersistentVolumes components") that calls render(<NodeStorage obj={mockNode} />) and then verifies screen.getByText('LocalDisks for test-node') and screen.getByText('PersistentVolumes for test-node') are in the document to catch any conditional rendering issues.frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx (2)
80-90: Consider adding a unique key fallback.Using
disk.nameas the React key is reasonable, but if two disks could theoretically share a name (e.g., multi-path devices), consider using an index fallback or combining multiple fields:💡 Optional: More robust key
- disks.map((disk) => <LocalDiskRow key={disk.name} obj={disk} />) + disks.map((disk, idx) => ( + <LocalDiskRow key={disk.serialNumber || disk.name || idx} obj={disk} /> + ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx` around lines 80 - 90, The keys for the mapped rows use disk.name which may not be unique; update the mapping in LocalDisks.tsx where disks.map(...) renders <LocalDiskRow key={disk.name} obj={disk} /> to provide a stable unique fallback (e.g., combine disk.name with another unique field like disk.path or disk.uuid, or use the map index as a last-resort fallback) so each LocalDiskRow has a truly unique key and avoids React key collisions.
67-92: Accessibility: Consider adding scope to table headers.For better screen reader support, adding
scope="col"to the<th>elements helps assistive technologies understand the table structure.♿ Accessibility improvement
- <th className="pf-v6-c-table__th">{t('console-app~Name')}</th> + <th className="pf-v6-c-table__th" scope="col">{t('console-app~Name')}</th>Apply similarly to all
<th>elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx` around lines 67 - 92, The table headers in the LocalDisks component lack column scope attributes; update each <th> in the header row inside the LocalDisks JSX (the header rendering that currently lists Name, Size, Type, Model, Serial number, Vendor, HCTL) to include scope="col" so screen readers can correctly interpret the table columns; no other logic changes needed.frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsx (2)
107-119: Good coverage, but consider adding a test for undefinedsizeBytes.The
LocalDiskscomponent falls back toDASHwhensizeBytesis undefined. Adding a test case where one disk hassizeBytes: undefinedwould verify this edge case renders correctly.💡 Suggested additional test case
it('should display dash when disk size is undefined', () => { const hostWithUndefinedSize = { ...mockBareMetalHost, status: { hardware: { storage: [ { name: '/dev/sdc', rotational: false, model: 'Unknown', }, ], }, }, }; useWatchBareMetalHostMock.mockReturnValue([hostWithUndefinedSize, true, undefined]); render(<LocalDisks node={mockNode} />); const rows = screen.getAllByRole('row'); expect(rows[1]).toHaveTextContent('-'); // DASH for missing size });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsx` around lines 107 - 119, Add a test that verifies LocalDisks renders a DASH when a disk's sizeBytes is undefined: create a host variant (e.g., hostWithUndefinedSize) by cloning mockBareMetalHost and setting status.hardware.storage to include an entry for '/dev/sdc' without sizeBytes, call useWatchBareMetalHostMock.mockReturnValue([hostWithUndefinedSize, true, undefined]), render <LocalDisks node={mockNode} />, and assert the rendered table row for that disk contains the DASH (e.g., via screen.getAllByRole('row') or screen.getByText('-')) to confirm the fallback behavior in LocalDisks.
75-81: Consider using a data-test attribute instead of class selector.Querying by
.loading-skeleton--tableclass couples the test to CSS implementation. Adata-testordata-testidattribute would be more resilient to styling changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsx` around lines 75 - 81, The test couples to CSS by querying '.loading-skeleton--table'; update the component and test: add a stable data attribute (e.g. data-testid="loading-skeleton-table") to the loading skeleton element in the LocalDisks component, then change the test in LocalDisks.spec.tsx to render(<LocalDisks node={mockNode} />) and assert using screen.getByTestId('loading-skeleton-table') (or container.querySelector('[data-testid="loading-skeleton-table"]')) after mocking useWatchBareMetalHostMock; reference LocalDisks and useWatchBareMetalHostMock when making these changes.frontend/packages/console-app/locales/en/console-app.json (1)
365-365: Verify "None" key usage scope.The generic key
"None": "None"could potentially be reused across different contexts. Ensure this is intentional and that it doesn't conflict with any existing or future usage that might require different translations in other locales (e.g., "None" for a missing value vs. "None" for a selection option).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/locales/en/console-app.json` at line 365, The "None" translation key is too generic and may conflict across contexts; either confirm its intended global reuse or replace it with context-specific keys (e.g., "no_value" or "selection_none") in frontend/packages/console-app/locales/en/console-app.json and update all code references that use the "None" key (search for string/lookup usages that reference "None") to the new key names; ensure corresponding entries are added to other locale files if needed and run a quick translation lookup test to validate no breaks.frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx (4)
315-325: Table accessibility: Consider addingscope="col"to header cells.For screen readers, explicit
scopeattributes improve table navigation:♻️ Proposed improvement
<thead className="pf-v6-c-table__thead"> <tr className="pf-v6-c-table__tr"> - <th className="pf-v6-c-table__th">{t('console-app~Name')}</th> - <th className="pf-v6-c-table__th">{t('console-app~PVC')}</th> + <th className="pf-v6-c-table__th" scope="col">{t('console-app~Name')}</th> + <th className="pf-v6-c-table__th" scope="col">{t('console-app~PVC')}</th> ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx` around lines 315 - 325, The table header in the PersistentVolumes component renders plain <th> elements without scope attributes which hurts screen reader navigation; update the header row in PersistentVolumes.tsx (the table within the PersistentVolumes component) to add scope="col" to each <th> (Name, PVC, Namespace, Pod, StorageClass, Capacity) so each header cell explicitly indicates it is a column header for accessibility.
267-280: PVs with hostname label but no bound PVC are silently dropped.In
nodePVCs, if a PV has thekubernetes.io/hostnamelabel but no matching PVC (orspec.claimRefis unset), it's filtered out. This could hide unbound local PVs from the user.Consider whether showing unbound PVs is valuable—if so, push them to
accwithpersistentVolumeClaimasundefined.♻️ Proposed fix to include unbound PVs
return ( nodePVs?.reduce<NodePersistentVolumeData[]>((acc, persistentVolume) => { const persistentVolumeClaim = pvcs.find( (pvc) => pvc.metadata.name === persistentVolume.spec?.claimRef?.name && pvc.metadata.namespace === persistentVolume.spec?.claimRef?.namespace, ); - if (persistentVolumeClaim) { - acc.push({ - persistentVolume, - persistentVolumeClaim, - }); - } + acc.push({ + persistentVolume, + persistentVolumeClaim, // may be undefined for unbound PVs + }); return acc; }, []) ?? [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx` around lines 267 - 280, The reduce that builds nodePVCs currently drops PVs that have the kubernetes.io/hostname label but lack a matching PVC or a spec.claimRef; update the logic in the nodePVs?.reduce callback so that when a persistentVolume has the hostname label (check persistentVolume.metadata?.labels?.['kubernetes.io/hostname']) but no matching pvc is found (or spec?.claimRef is unset), you still acc.push an entry of type NodePersistentVolumeData with persistentVolumeClaim set to undefined; keep existing behavior of pushing paired PV+PVC when PVC exists (the persistentVolume, pvcs.find(...) branch) and ensure you use optional chaining on spec.claimRef to avoid exceptions.
133-176: Performance consideration: Five simultaneous watch/list operations.This component establishes watches for PVs, PVCs, DataVolumes, VMIs, and Pods. For clusters with many resources, this could cause:
- Significant memory pressure from caching lists
- Network overhead from concurrent watch connections
Consider whether PVs and PVCs could be fetched with label/field selectors to reduce payload, or if lazy loading would help.
That said, for the Node Configuration tab use case where users explicitly navigate here, the current approach is reasonable. Just flag it for monitoring if performance issues arise in large clusters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx` around lines 133 - 176, PersistentVolumes currently opens five concurrent watches (useK8sWatchResource for persistentVolumes/pvcs, useAccessibleResources for dataVolumes/pods, and useWatchVirtualMachineInstances for vms) which can be heavy; modify PersistentVolumes to reduce watch scope by adding fieldSelector/labelSelector where possible (e.g., filter PVs/PVCs by node-related labels or PVC namespace) or convert some watches to on-demand fetches (useK8sGet or fetch in effect) and/or only start watches when the Node Configuration tab/component is visible; update usages of useK8sWatchResource, useAccessibleResources, and useWatchVirtualMachineInstances accordingly so heavy lists are either selector-filtered or lazily loaded to lower memory/network overhead.
178-183: Error state conflates multiple distinct failures.
loadErroris a union of five possible errors. When displayed (line 312), users see a generic message without knowing which resource failed. For debugging, consider logging the specific error or showing which resource couldn't load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx` around lines 178 - 183, The current loadError combines multiple distinct errors (persistentVolumesLoadError, pvcsLoadError, dataVolumesLoadError, vmsLoadError, podsLoadError) into a single value which hides which resource failed; update the logic that computes loadError (in the PersistentVolumes component) to produce an error object or tuple that includes the failing resource name and its error (e.g., { resource: 'PVCs', error: pvcsLoadError }) or a map of resource->error, and then use that structured value where the error is displayed to show the resource name and the specific error message and also log the full error (console.error or app logger) for debugging; replace references to loadError with the new loadErrorInfo (or similar) and update the UI render path that currently shows the generic message to include loadErrorInfo.resource and loadErrorInfo.error.message.frontend/packages/console-app/src/components/nodes/configuration/__tests__/NodeConfiguration.spec.tsx (1)
68-83: Test usesrenderbut extension tests userenderWithProviders—ensure consistency.Lines 72, 89, 101, 113, 123 use bare
render(), while extension tests userenderWithProviders. If the component tree requires Redux/plugin store context for any path, inconsistent wrapper usage could mask integration issues. Consider standardizing onrenderWithProvidersfor all tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/__tests__/NodeConfiguration.spec.tsx` around lines 68 - 83, Tests in NodeConfiguration.spec.tsx use the bare render() helper instead of the standardized renderWithProviders, which may miss required Redux/plugin context; replace each render(<NodeConfiguration obj={mockNode} />) call with renderWithProviders(<NodeConfiguration obj={mockNode} />) (including in the "should render Storage tab by default" test and the other tests that currently call render), and ensure renderWithProviders is imported into the test file; keep the rest of the assertions unchanged.frontend/packages/console-app/src/components/nodes/configuration/NodeMachine.tsx (1)
86-96: Consider using Alert components for error states to improve accessibility and UX consistency.The plain
<div>elements for error messages (lines 87, 92, 98, 117) lack semantic meaning and visual prominence. PatternFlyAlertwithvariant="danger"provides consistent styling, proper ARIA roles, and screen reader support.♻️ Proposed refactor using Alert
+import { Alert } from '@patternfly/react-core'; ... {machineLoadError ? ( - <div>{t('console-app~Error loading machine')}</div> + <Alert variant="danger" isInline title={t('console-app~Error loading machine')} /> ) : machineLoaded ? ( machine ? ( <MachineDetails obj={machine} hideConditions /> ) : ( - <div>{t('console-app~Machine not found')}</div> + <Alert variant="info" isInline title={t('console-app~Machine not found')} /> ) ) : (Also applies to: 97-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/nodes/configuration/NodeMachine.tsx` around lines 86 - 96, Replace the plain <div> messages in the NodeMachine render with PatternFly Alert components to improve accessibility and UX: import Alert from '@patternfly/react-core' and replace the machineLoadError branch (currently rendering <div>{t('...Error loading machine')}</div>) with <Alert variant="danger" isInline title={t('...Error loading machine')}> (or similar inline Alert with the translated string); replace the "Machine not found" <div> with an Alert (variant="warning" or "info") using t('...Machine not found') as the title/content; apply the same Alert substitution for the other plain-message <div>s in this component (lines 97-121) while keeping the existing MachineDetails and SkeletonDetails usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsx`:
- Around line 242-258: Rename the test in PersistentVolumes.spec.tsx so its
description matches the assertion: update the it(...) title from 'should display
"No claim" when PVC is not found' to something like 'should display "No
persistent volumes found" when PV has no claimRef' so it clearly reflects the
mocked pvNoClaim setup and the expectation in the PersistentVolumes test block.
In `@frontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsx`:
- Around line 57-58: activePage can be null when pages is empty, causing a
TypeError on reading activePage.component; update the code that sets Component
to safely handle null (e.g., use optional chaining or a fallback) and ensure the
render logic handles a null Component. Specifically, change the assignment
around activePage/component (referencing pages, activeTabKey, activePage,
Component) so Component = activePage?.component ?? null (or return early if no
activePage) and adjust downstream rendering to skip or render a safe placeholder
when Component is null.
In
`@frontend/packages/console-app/src/components/nodes/utils/useAccessibleResources.ts`:
- Around line 61-76: The hook currently collapses to an empty result when any
namespace has a loadError; update useAccessibleResources to treat per-namespace
failures as partial results by: 1) computing allResources by flat-mapping
namespacedResources[key].data only for namespaces that have data (so successful
watches are merged even if others errored), 2) computing loaded as
projectsLoaded && Object.keys(namespacedResources).every(key =>
namespacedResources[key].loaded || namespacedResources[key].loadError) (so
“loaded” reflects that all watches settled, either success or error), and 3)
keeping loadError as projectsLoadError ||
Object.values(namespacedResources).find(res => res.loadError)?.loadError but do
NOT return [] when loadError is present—return [allResources, loaded, loadError]
instead; update references to loaded, loadError, namespacedResources and the
return path in useAccessibleResources accordingly.
In `@frontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.md`:
- Line 61: The markdown link fragment uses mixed case and won't match the
generated lowercase anchor; update the link target for the
console.tab/nodeSubNavTab entry so the fragment is all lowercase (e.g., change
the current "(`#consoletabnodeSubNavTab`)" to "(`#consoletabnodesubnavtab`)")
ensuring the link fragment matches the header-generated anchor for
console.tab/nodeSubNavTab.
---
Outside diff comments:
In
`@frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.ts`:
- Around line 194-205: Test currently only asserts the BareMetalHost watch;
update the test for useWatchBareMetalHost to also assert that
useK8sWatchResourceMock was called for the Machine resource by adding an
expectation that a call was made with isList: true and groupVersionKind:
MachineGroupVersionKind (or the exact Machine GVK constant used in the code) and
namespaced: true. Locate the test for useWatchBareMetalHost, keep the existing
BareMetalHost assertion (BareMetalHostGroupVersionKind) and add a second
expect(...) to verify the Machine watch via useK8sWatchResourceMock so the test
fails if the Machine watch is removed.
In
`@frontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.ts`:
- Around line 299-353: Add a test for useWatchVirtualMachineInstances that
covers the non-admin/per-namespace watch path by making useFlagMock return
different values for the two flags instead of a single mockReturnValue;
specifically, use mockImplementation or mockReturnValueOnce so KUBEVIRT_DYNAMIC
is true but FLAGS.CAN_LIST_NS is false (or vice versa) to trigger the
useK8sWatchResources branch in useAccessibleResources, then assert that
useK8sWatchResourcesMock was called with the per-namespace resources aggregation
and that the hook returns the expected filtered VMIs for the node; locate the
logic around useWatchVirtualMachineInstances, useFlagMock,
useK8sWatchResourcesMock, and useAccessibleResources to add this case.
---
Nitpick comments:
In `@frontend/packages/console-app/locales/en/console-app.json`:
- Line 365: The "None" translation key is too generic and may conflict across
contexts; either confirm its intended global reuse or replace it with
context-specific keys (e.g., "no_value" or "selection_none") in
frontend/packages/console-app/locales/en/console-app.json and update all code
references that use the "None" key (search for string/lookup usages that
reference "None") to the new key names; ensure corresponding entries are added
to other locale files if needed and run a quick translation lookup test to
validate no breaks.
In
`@frontend/packages/console-app/src/components/nodes/configuration/__tests__/NodeConfiguration.spec.tsx`:
- Around line 68-83: Tests in NodeConfiguration.spec.tsx use the bare render()
helper instead of the standardized renderWithProviders, which may miss required
Redux/plugin context; replace each render(<NodeConfiguration obj={mockNode} />)
call with renderWithProviders(<NodeConfiguration obj={mockNode} />) (including
in the "should render Storage tab by default" test and the other tests that
currently call render), and ensure renderWithProviders is imported into the test
file; keep the rest of the assertions unchanged.
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsx`:
- Around line 107-119: Add a test that verifies LocalDisks renders a DASH when a
disk's sizeBytes is undefined: create a host variant (e.g.,
hostWithUndefinedSize) by cloning mockBareMetalHost and setting
status.hardware.storage to include an entry for '/dev/sdc' without sizeBytes,
call useWatchBareMetalHostMock.mockReturnValue([hostWithUndefinedSize, true,
undefined]), render <LocalDisks node={mockNode} />, and assert the rendered
table row for that disk contains the DASH (e.g., via screen.getAllByRole('row')
or screen.getByText('-')) to confirm the fallback behavior in LocalDisks.
- Around line 75-81: The test couples to CSS by querying
'.loading-skeleton--table'; update the component and test: add a stable data
attribute (e.g. data-testid="loading-skeleton-table") to the loading skeleton
element in the LocalDisks component, then change the test in LocalDisks.spec.tsx
to render(<LocalDisks node={mockNode} />) and assert using
screen.getByTestId('loading-skeleton-table') (or
container.querySelector('[data-testid="loading-skeleton-table"]')) after mocking
useWatchBareMetalHostMock; reference LocalDisks and useWatchBareMetalHostMock
when making these changes.
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsx`:
- Around line 1-42: Add a new test to NodeStorage.spec.tsx that renders the
NodeStorage component and asserts both child mocks are present in a single test;
specifically, create a test (e.g., "should render both LocalDisks and
PersistentVolumes components") that calls render(<NodeStorage obj={mockNode} />)
and then verifies screen.getByText('LocalDisks for test-node') and
screen.getByText('PersistentVolumes for test-node') are in the document to catch
any conditional rendering issues.
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx`:
- Around line 80-90: The keys for the mapped rows use disk.name which may not be
unique; update the mapping in LocalDisks.tsx where disks.map(...) renders
<LocalDiskRow key={disk.name} obj={disk} /> to provide a stable unique fallback
(e.g., combine disk.name with another unique field like disk.path or disk.uuid,
or use the map index as a last-resort fallback) so each LocalDiskRow has a truly
unique key and avoids React key collisions.
- Around line 67-92: The table headers in the LocalDisks component lack column
scope attributes; update each <th> in the header row inside the LocalDisks JSX
(the header rendering that currently lists Name, Size, Type, Model, Serial
number, Vendor, HCTL) to include scope="col" so screen readers can correctly
interpret the table columns; no other logic changes needed.
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx`:
- Around line 315-325: The table header in the PersistentVolumes component
renders plain <th> elements without scope attributes which hurts screen reader
navigation; update the header row in PersistentVolumes.tsx (the table within the
PersistentVolumes component) to add scope="col" to each <th> (Name, PVC,
Namespace, Pod, StorageClass, Capacity) so each header cell explicitly indicates
it is a column header for accessibility.
- Around line 267-280: The reduce that builds nodePVCs currently drops PVs that
have the kubernetes.io/hostname label but lack a matching PVC or a
spec.claimRef; update the logic in the nodePVs?.reduce callback so that when a
persistentVolume has the hostname label (check
persistentVolume.metadata?.labels?.['kubernetes.io/hostname']) but no matching
pvc is found (or spec?.claimRef is unset), you still acc.push an entry of type
NodePersistentVolumeData with persistentVolumeClaim set to undefined; keep
existing behavior of pushing paired PV+PVC when PVC exists (the
persistentVolume, pvcs.find(...) branch) and ensure you use optional chaining on
spec.claimRef to avoid exceptions.
- Around line 133-176: PersistentVolumes currently opens five concurrent watches
(useK8sWatchResource for persistentVolumes/pvcs, useAccessibleResources for
dataVolumes/pods, and useWatchVirtualMachineInstances for vms) which can be
heavy; modify PersistentVolumes to reduce watch scope by adding
fieldSelector/labelSelector where possible (e.g., filter PVs/PVCs by
node-related labels or PVC namespace) or convert some watches to on-demand
fetches (useK8sGet or fetch in effect) and/or only start watches when the Node
Configuration tab/component is visible; update usages of useK8sWatchResource,
useAccessibleResources, and useWatchVirtualMachineInstances accordingly so heavy
lists are either selector-filtered or lazily loaded to lower memory/network
overhead.
- Around line 178-183: The current loadError combines multiple distinct errors
(persistentVolumesLoadError, pvcsLoadError, dataVolumesLoadError, vmsLoadError,
podsLoadError) into a single value which hides which resource failed; update the
logic that computes loadError (in the PersistentVolumes component) to produce an
error object or tuple that includes the failing resource name and its error
(e.g., { resource: 'PVCs', error: pvcsLoadError }) or a map of resource->error,
and then use that structured value where the error is displayed to show the
resource name and the specific error message and also log the full error
(console.error or app logger) for debugging; replace references to loadError
with the new loadErrorInfo (or similar) and update the UI render path that
currently shows the generic message to include loadErrorInfo.resource and
loadErrorInfo.error.message.
In
`@frontend/packages/console-app/src/components/nodes/configuration/NodeMachine.tsx`:
- Around line 86-96: Replace the plain <div> messages in the NodeMachine render
with PatternFly Alert components to improve accessibility and UX: import Alert
from '@patternfly/react-core' and replace the machineLoadError branch (currently
rendering <div>{t('...Error loading machine')}</div>) with <Alert
variant="danger" isInline title={t('...Error loading machine')}> (or similar
inline Alert with the translated string); replace the "Machine not found" <div>
with an Alert (variant="warning" or "info") using t('...Machine not found') as
the title/content; apply the same Alert substitution for the other plain-message
<div>s in this component (lines 97-121) while keeping the existing
MachineDetails and SkeletonDetails usage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 633fcc13-3a0a-4fcf-b5f7-0bd1f115d660
📒 Files selected for processing (28)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsxfrontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeMachine.tsxfrontend/packages/console-app/src/components/nodes/configuration/__tests__/NodeConfiguration.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/__tests__/NodeMachine.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/useAccessibleResources.tsfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/index.tsfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node-subnav-tabs.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.tsfrontend/public/components/machine-config-pool.tsxfrontend/public/components/machine.tsx
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 which is a valid 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. |
|
Looks good Jeff, send it. |
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 which is a valid 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. |
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
7989685 to
bc86e8f
Compare
jeff-phillips-18
added
the
do-not-merge/hold
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
bc86e8f to
73656f5
Compare
jeff-phillips-18
removed
the
do-not-merge/hold
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-5.0" instead. 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
force-pushed
the
node-configuration
branch
from
73656f5 to
3ab0460
Compare
Member
Author
|
/test e2e-gcp-console |
jeff-phillips-18
force-pushed
the
node-configuration
branch
2 times, most recently
from
e229c05 to
9c4d0ca
Compare
Member
Author
|
/retest |
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
9c4d0ca to
2ef4822
Compare
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-4946 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 either version "4.22." or "openshift-4.22.", but it targets "openshift-4.23" instead. 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
force-pushed
the
node-configuration
branch
2 times, most recently
from
fe3ec05 to
6325bc3
Compare
Member
Author
|
/retest |
jhadvig
requested changes
May 19, 2026
Member
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Review Summary
Substantial, well-structured PR with good test coverage and solid extensibility design. The console.node/sub-nav-tab extension follows existing patterns well, and the useAccessibleResources hook is a useful addition for permission-aware resource fetching. Several issues need attention before merge:
Critical
- SDK exports:
SubPageComponentPropsandNodeSubNavTabmay not be accessible to external plugin consumers — thesrc/api/re-export files aren't updated (see inline comment)
Major
- Barrel imports: Multiple new files import from
@console/dynamic-plugin-sdk/srcbarrel instead of specific paths - Error handling:
useAccessibleResourcesreturns partial data +loadErrortogether, butPersistentVolumes.tsxhides all data whenloadErroris truthy - Missing tests: No unit tests for
NodeSubNavPage(the core extension orchestrator) oruseAccessibleResources(the new permission-aware hook). Both are non-trivial components with multiple edge cases
Minor (not all posted as inline comments)
- CHANGELOG placeholder
#XXXXX SubPageComponentPropsgeneric defaults toK8sResourceCommoninstead ofNodeKind- Documented
tabIddeduplication is not implemented setActiveTabKeynot memoizedSkeletonDetailsduplicated across 3 files — consider extracting a shared component- Raw HTML tables in
LocalDisks.tsxandPersistentVolumes.tsxinstead of PatternFly Table — bypasses built-in a11y features - PR description says Machine priority is 40, code and docs say 50
Comment on lines
+70
to
+78
| const loadError = | ||
| projectsLoadError || namespacedResults.find((results) => results.loadError)?.loadError; | ||
|
|
||
| const allResources = namespacedResults | ||
| .filter((results) => !results.loadError) | ||
| .map((results) => results.data ?? []) | ||
| .flat(); | ||
|
|
||
| return [allResources, true, loadError]; |
Member
There was a problem hiding this comment.
This returns [allResources, true, loadError] where allResources contains partial success data and loadError is from a failed namespace. The problem is downstream — PersistentVolumes.tsx checks loadError first and shows an error alert that hides all data:
const loadError = persistentVolumesLoadError || pvcsLoadError || podsLoadError;
// ...
} : loadError ? (
<Alert isInline variant="danger" ...>So a single forbidden namespace for Pods will hide all PV/PVC data that loaded successfully. Either suppress loadError when partial data is available, or update consumers to show partial data alongside the error.
Member
Author
There was a problem hiding this comment.
A comment was added to the function explaining that any failure will return a value as the loadError (third value returned) and that only the first error found is returned.
jeff-phillips-18
force-pushed
the
node-configuration
branch
2 times, most recently
from
f4e3261 to
ca420e2
Compare
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
ca420e2 to
b3ff1f2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx (1)
61-65: ⚡ Quick winNICs formatting may produce empty comma-separated values.
The current implementation includes NICs without IP addresses as empty strings, resulting in output like
"10.0.0.1, , 10.0.0.3". Consider filtering out blank entries for cleaner display.♻️ Proposed fix
<ConfigPropertyListItem title={t('console-app~NICs')} isLoading={isLoading}> - { - // Intentionally include blank nics - bareMetalHost?.status?.hardware?.nics?.map((nic) => nic.ip).join(', ') || DASH - } + {bareMetalHost?.status?.hardware?.nics + ?.map((nic) => nic.ip) + .filter(Boolean) + .join(', ') || DASH} </ConfigPropertyListItem>🤖 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 `@frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx` around lines 61 - 65, The NICs display currently maps bareMetalHost?.status?.hardware?.nics to nic.ip and joins them, producing empty entries when ip is missing; update the render in BMCConfiguration (the ConfigPropertyListItem that uses bareMetalHost?.status?.hardware?.nics?.map((nic) => nic.ip).join(', ') || DASH) to first filter out falsy/empty ip values (e.g., filter(nic => nic?.ip) or filter(Boolean) on the mapped ips) and then join with ', ' so only non-empty IPs are shown and the fallback DASH remains unchanged.
🤖 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
`@frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx`:
- Around line 61-65: The NICs display currently maps
bareMetalHost?.status?.hardware?.nics to nic.ip and joins them, producing empty
entries when ip is missing; update the render in BMCConfiguration (the
ConfigPropertyListItem that uses
bareMetalHost?.status?.hardware?.nics?.map((nic) => nic.ip).join(', ') || DASH)
to first filter out falsy/empty ip values (e.g., filter(nic => nic?.ip) or
filter(Boolean) on the mapped ips) and then join with ', ' so only non-empty IPs
are shown and the fallback DASH remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5b8958c8-f41f-4a62-a7d9-fdb4dd695ba9
📒 Files selected for processing (32)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsxfrontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/BMCConfiguration.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/NodeMachine.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/GroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/NodeGroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeGroupUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.ts
💤 Files with no reviewable changes (24)
- frontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx
- frontend/packages/console-app/src/components/nodes/modals/tests/NodeGroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.ts
- frontend/packages/console-app/src/components/nodes/modals/tests/GroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeGroupUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/NodeMachine.spec.tsx
- frontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/BMCConfiguration.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/NodeStorage.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeVmUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.ts
- frontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.ts
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeBareMetalUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/LocalDisks.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/PersistentVolumes.spec.tsx
- frontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/packages/console-app/src/components/nodes/NodesPage.tsx
Member
Author
|
/retest |
openshift-ci
Bot
added
lgtm
Member
QA Verification Evidence
Verification Steps
Animated overview (click to expand)
Step 1: Nodes list page (pass)
Step 2: Node overview (tab bar) (pass)
Step 3: Configuration - Storage sub-tab (pass)
Step 4: Configuration - Machine sub-tab (pass)
Step 5: Node details (regression check) (pass)
Note Setup: Candidate was run with Warning This verification was performed by an AI agent. Results may contain false positives or miss Automated QA verification by Claude Code |
Member
Bug: Node details page crashes with
|
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
b3ff1f2 to
3506045
Compare
Contributor
|
New changes are detected. LGTM label has been removed. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, jhadvig 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 |
Member
Author
|
/retest analyze |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/packages/console-app/src/hooks/useAccessibleResources.ts (1)
52-52: ⚡ Quick winReplace
WatchK8sResources<any>with a concrete typed shape.
anyhere drops type guarantees for per-namespace watch entries and makes regressions easier to miss in this exported hook.🔧 Proposed fix
- const initResources: WatchK8sResources<any> = useMemo(() => { + const initResources: WatchK8sResources<R[]> = useMemo(() => {As per coding guidelines "Flag use of
anytype and suggest proper type definitions instead".🤖 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 `@frontend/packages/console-app/src/hooks/useAccessibleResources.ts` at line 52, The initResources variable in useAccessibleResources uses WatchK8sResources<any>, which erases type safety; define a concrete interface for the per-resource shape (e.g., AccessibleResourceSpec with properties you actually use such as kind: string, isList?: boolean, namespaced?: boolean, namespace?: string, name?: string) then replace the any with that type (e.g., type AccessibleWatchResources = WatchK8sResources<AccessibleResourceSpec> and change const initResources: WatchK8sResources<any> to const initResources: AccessibleWatchResources) and update any related usages in the useAccessibleResources hook to match the new interface.
🤖 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 `@frontend/packages/console-app/src/hooks/useAccessibleResources.ts`:
- Around line 47-55: projectsNames can be undefined and causes a crash when
forEach is called; ensure you treat it as an array (e.g., coalesce or guard with
Array.isArray) before iterating so the block building initResources won't throw,
and replace the weak WatchK8sResources<any> typing with a concrete generic
derived from initResource (or the actual resource type) so initResources has a
proper typed shape; update the code that consumes projectsData/getName,
initResource and useK8sWatchResource to use the safe projectsNames array and the
stronger WatchK8sResources<...> generic.
---
Nitpick comments:
In `@frontend/packages/console-app/src/hooks/useAccessibleResources.ts`:
- Line 52: The initResources variable in useAccessibleResources uses
WatchK8sResources<any>, which erases type safety; define a concrete interface
for the per-resource shape (e.g., AccessibleResourceSpec with properties you
actually use such as kind: string, isList?: boolean, namespaced?: boolean,
namespace?: string, name?: string) then replace the any with that type (e.g.,
type AccessibleWatchResources = WatchK8sResources<AccessibleResourceSpec> and
change const initResources: WatchK8sResources<any> to const initResources:
AccessibleWatchResources) and update any related usages in the
useAccessibleResources hook to match the new interface.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a3ed87e-4b19-4b82-b33e-4890f1971b99
📒 Files selected for processing (37)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsxfrontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/BMCConfiguration.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/NodeMachine.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/GroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/NodeGroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeGroupUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.tsfrontend/packages/console-app/src/hooks/useAccessibleResources.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
✅ Files skipped from review due to trivial changes (6)
- frontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.ts
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeGroupUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/modals/tests/NodeGroupsEditorModal.spec.tsx
- frontend/packages/console-app/locales/en/console-app.json
🚧 Files skipped from review as they are similar to previous changes (24)
- frontend/packages/console-app/src/components/nodes/modals/tests/GroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/NodeStorage.spec.tsx
- frontend/packages/console-app/src/components/nodes/NodesPage.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/BMCConfiguration.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/LocalDisks.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsx
- frontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeVmUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/PersistentVolumes.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/NodeMachine.spec.tsx
- frontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.ts
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeBareMetalUtils.spec.ts
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
3506045 to
107bb15
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx`:
- Around line 61-66: The NIC IP rendering currently maps
bareMetalHost?.status?.hardware?.nics to nic.ip and may produce the literal
"undefined"; update the expression in the ConfigPropertyListItem
(title={t('console-app~NICs')}) to first filter out undefined/empty values
(e.g., get nics via bareMetalHost?.status?.hardware?.nics?.map(n =>
n.ip).filter(ip => ip) or filter(n => n.ip) then map) and then join(', '),
falling back to DASH when the resulting array is empty; ensure you still respect
the isLoading prop.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 35e74ee6-19ea-4c52-b83c-4124d95fc13e
⛔ Files ignored due to path filters (2)
node_modules/.yarn-integrityis excluded by!**/node_modules/**,!node_modules/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!yarn.lock
📒 Files selected for processing (39)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsxfrontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/BMCConfiguration.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/NodeMachine.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/GroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/NodeGroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/VirtualMachinesInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeGroupUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.tsfrontend/packages/console-app/src/hooks/useAccessibleResources.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
✅ Files skipped from review due to trivial changes (6)
- frontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx
- frontend/packages/console-app/src/components/nodes/modals/tests/GroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/modals/tests/NodeGroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeGroupUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsx
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md
🚧 Files skipped from review as they are similar to previous changes (27)
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.ts
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/NodeStorage.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeVmUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/NodeMachine.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeBareMetalUtils.spec.ts
- frontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
- frontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsx
- frontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/extensions/node.ts
- frontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsx
- frontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.md
- frontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.ts
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/LocalDisks.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/BMCConfiguration.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/PersistentVolumes.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.ts
- frontend/packages/console-app/locales/en/console-app.json
- frontend/packages/console-app/src/hooks/useAccessibleResources.ts
- frontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsx
Member
Author
|
@jseseCCS Could you please review for me? Thanks. |
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
107bb15 to
5e55462
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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
`@frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsx`:
- Around line 45-54: The current rendering uses configuration.source directly
which is truthy for an empty array and results in blank output; update the
MachineConfigPoolCharacteristics rendering to check configuration.source.length
(e.g., use configuration.source && configuration.source.length) before mapping
so that when the array is empty it falls back to '-' instead of rendering
nothing; locate the JSX where configuration.source is mapped and adjust the
conditional around the map (affecting the ResourceLink rendering) to explicitly
handle the empty array case.
In
`@frontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsx`:
- Around line 187-190: The vmPVCs useMemo currently returns [] when loadError ||
vmDataLoadError || isLoading which hides VM-backed volume failures when
node-local PV rows exist; change the logic in the vmPVCs computation (and the
similar block at the other occurrence around lines 297-300) to not discard
partial results — instead keep whatever PVC data was successfully loaded, set a
separate flag (e.g., vmPVCsLoadWarning or use the existing vmDataLoadError
boolean) and surface a non-blocking warning in the UI when vmDataLoadError or
vmsLoadError is true; update the rendering path that displays the table to check
that warning flag and render the warning even if rows are present so VM-backed
volume failures are visible.
- Around line 150-158: The PVC watch currently uses useK8sWatchResource which
can fail for users without cluster-wide access; replace the useK8sWatchResource
call for PersistentVolumeClaimModel with the permissions-aware hook
useAccessibleResources (same approach used for Pod and DataVolume watches),
requesting the PersistentVolumeClaimModel as a namespaced list and map its
returned values into pvcs, pvcsLoaded and pvcsLoadError so the component uses
the accessible-resources result instead of the all-namespaces watcher.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94159603-c3d7-4d0e-bb23-ed3b328e2cc8
⛔ Files ignored due to path filters (2)
node_modules/.yarn-integrityis excluded by!**/node_modules/**,!node_modules/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!yarn.lock
📒 Files selected for processing (39)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsxfrontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsxfrontend/packages/console-app/src/components/nodes/NodesPage.tsxfrontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolCharacteristics.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/BMCConfiguration.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/machine/__tests__/NodeMachine.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/NodeStorage.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/PersistentVolumes.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/LocalDisks.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/NodeStorage.spec.tsxfrontend/packages/console-app/src/components/nodes/configuration/node-storage/__tests__/PersistentVolumes.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/GroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/NodeGroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/VirtualMachinesInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeBareMetalUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeGroupUtils.spec.tsfrontend/packages/console-app/src/components/nodes/utils/__tests__/NodeVmUtils.spec.tsfrontend/packages/console-app/src/hooks/useAccessibleResources.tsfrontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.mdfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
✅ Files skipped from review due to trivial changes (5)
- frontend/packages/console-app/src/components/nodes/NodesPage.tsx
- frontend/packages/console-dynamic-plugin-sdk/CHANGELOG-core.md
- frontend/packages/console-app/src/components/nodes/modals/tests/GroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/DetailsCard.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/VirtualMachinesInventoryItems.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (29)
- frontend/packages/console-app/src/components/nodes/modals/tests/NodeGroupsEditorModal.spec.tsx
- frontend/packages/console-app/src/components/nodes/modals/NodeGroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeGroupUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/utils/NodeGroupUtils.ts
- frontend/packages/console-app/src/components/nodes/NodeDetailsPage.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/LocalDisks.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/NodeMachine.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/NodeStorage.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeBareMetalUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/NodeConfiguration.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/tests/BMCConfiguration.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/modals/GroupsEditorModal.tsx
- frontend/packages/console-app/src/components/nodes/utils/tests/NodeVmUtils.spec.ts
- frontend/packages/console-app/src/components/nodes/configuration/machine/NodeMachine.tsx
- frontend/packages/console-app/src/components/nodes/NodeSubNavPage.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineDetails.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/BMCConfiguration.tsx
- frontend/packages/console-app/src/components/nodes/configuration/machine/MachineConfigPoolSummary.tsx
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/PersistentVolumes.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/extensions/node.ts
- frontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.ts
- frontend/packages/console-app/src/hooks/useAccessibleResources.ts
- frontend/packages/console-app/src/components/nodes/configuration/node-storage/tests/LocalDisks.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.ts
- frontend/packages/console-app/locales/en/console-app.json
jeff-phillips-18
force-pushed
the
node-configuration
branch
from
5e55462 to
26fe28a
Compare
Member
Author
|
/test frontend |
Contributor
|
@jeff-phillips-18: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes CONSOLE-4946
Closes CONSOLE-4948
Closes CONSOLE-4949
Note
This feature is available in Tech Preview only.
Description
Add Configuration tab to Node details page with extensible sub-navigation
Summary
This PR adds a new "Configuration" tab to the Node details page that displays storage and machine information using an extensible sub-navigation pattern. It also introduces a new plugin extension type (console.node/sub-nav-tab) that allows dynamic plugins to contribute additional sub-tabs to Node detail pages.
Key changes:
Extension API
The new console.tab/nodeSubNavTab extension allows plugins to contribute sub-tabs:
Tabs are displayed in priority order (highest to lowest). Current priorities:
Test plan
Screenshots
Node Configuration - Storage
Node Configuration - Machine
🤖 Generated with https://claude.com/claude-code
Summary by CodeRabbit
New Features
Localization
Documentation
/cc @kybaker