OCPNETUI-38: Virtual Machines tab on NAD/UDN/CUDN detail pages - #442
Open
Parthivk100 wants to merge 12 commits into
Open
OCPNETUI-38: Virtual Machines tab on NAD/UDN/CUDN detail pages#442Parthivk100 wants to merge 12 commits into
Parthivk100 wants to merge 12 commits into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughChangesThe plugin adds NAD and UDN details pages with YAML and virtual-machine tabs, VM association management, resource lookup utilities, configurable table columns, updated confirmation dialogs, manifest wiring, and localization strings. Network details and VM workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Parthivk100
changed the title
|
@Parthivk100: This pull request references OCPNETUI-38 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
lkladnit
approved these changes
Jul 13, 2026
|
/retest |
Contributor
|
Scheduling required tests: |
|
/lgtm |
|
New changes are detected. LGTM label has been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
src/views/nads/details/tabs/virtualMachines/NADVirtualMachines.tsx (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract inline prop type for consistency.
Other new files in this PR (
NADYAML.tsx,NADDescriptionDetails.tsx) define a namedPropstype; this component uses an inline object type 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 `@src/views/nads/details/tabs/virtualMachines/NADVirtualMachines.tsx` at line 46, Extract the inline prop object type from the NADVirtualMachines component into a named Props type, matching the pattern used by NADYAML and NADDescriptionDetails, then update the component declaration to use Props while preserving the optional obj property and its NetworkAttachmentDefinitionKind type.src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx (1)
86-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
getVMStatusLabelColorhelper across two files. Both files define an identical function to map VM status to aLabelcolor; extract to a shared util once.
src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx#L86-L100: remove the local copy and import the shared helper.src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx#L60-L74: remove the local copy and import the shared helper.🤖 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 `@src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx` around lines 86 - 100, Extract the identical getVMStatusLabelColor logic into a shared utility, then remove the local definitions and import the shared helper in src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx (lines 86-100) and src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx (lines 60-74). Preserve the existing status-to-color behavior at both call sites.src/views/udns/details/tabs/details/UDNDetailsTab.tsx (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer named lodash import for tree-shaking.
import * as _ from 'lodash'pulls the entire library. Only_.sizeis used. A named import enables bundler tree-shaking.-import * as _ from 'lodash'; +import { size } from 'lodash';Then replace
_.size(...)withsize(...)on lines 103 and 108.🤖 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 `@src/views/udns/details/tabs/details/UDNDetailsTab.tsx` at line 2, Replace the namespace lodash import in UDNDetailsTab with a named size import, then update both _.size usages in the component to call size directly. Leave all other lodash usage 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.
Inline comments:
In `@package.json`:
- Line 44: Update the `@patternfly/react-data-view` dependency in package.json
from the tilde range to the exact version 6.5.0, matching the file’s existing
exact-version dependency convention.
In `@src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx`:
- Around line 57-84: The MEMORY branch of getVMNetworkTableSortValue currently
returns the display quantity string from getVMMemory, causing lexicographic
ordering; normalize that value to its numeric quantity before returning it for
sorting, while leaving the displayed memory formatting unchanged.
In `@src/utils/resources/vm/utils.ts`:
- Around line 219-223: Update findNADForUDN to resolve and return the matching
watched NAD from _nads instead of calling buildNADReferenceForUDN to fabricate
one. Derive the namespace and candidate NAD names using the existing
CUDN-generated prefix handling, then search _nads and return the matched
resource; preserve undefined when no candidate exists.
- Around line 471-492: Update the single-pair reconnect logic in patchVM to
clone pairToReconnect.network and pairToReconnect.iface rather than constructing
new objects. Override only multus.networkName on the network, and remove only
the interface’s state: 'absent' while preserving all other NIC configuration
fields.
In `@src/views/nads/actions/NADActions.tsx`:
- Around line 8-22: Update NADsRow to pass its isKebabToggle value through the
NADsActions component when rendering it, ensuring the ActionsDropdown receives
the intended kebab-toggle state instead of relying on the optional default.
In
`@src/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsx`:
- Around line 378-390: Update handleAdd to await all addVMToNAD operations to
completion using per-VM outcome collection rather than fail-fast Promise.all.
Keep the modal open when any VM fails, identify and display the failed VM names,
and only clear isSubmitting after every patch has settled; preserve onClose only
when all selected VMs succeed.
In `@src/views/nads/manifest.ts`:
- Around line 80-86: Gate the NetworkAttachmentDefinition details extension
containing NADDetailsPage behind the appropriate KubeVirt feature flag from
src/utils/flags/consts.ts, using the manifest’s required flags mechanism. Ensure
the page cannot mount and start its VirtualMachine watch when KubeVirt is
unavailable.
In `@src/views/udns/details/tabs/details/UDNDetailsTab.tsx`:
- Around line 102-109: Update both annotation translation calls in UDNDetailsTab
to use i18next pluralization with the count value, referencing the pluralized
annotation key rather than the singular key. Add matching annotation_one and
annotation_other entries to every locale file that defines this translation.
In
`@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx`:
- Around line 379-403: Surface user-facing failures for both VM network mutation
flows: in AddUDNVirtualMachinesModal.handleAdd, use Promise.allSettled to detect
rejected addVMToNAD operations and show an appropriate toast or alert while
preserving submission cleanup; in UDNVirtualMachineRowActions, handle
removeVMFromNAD rejection inside the ConfirmModal executeFn flow and show the
failure to the user even though the modal closes immediately. Update both listed
files and retain networkConsole logging as appropriate.
- Around line 194-241: Update getSortParams so sortBy.direction is provided only
when the current column matches the active sort key (sortBy ===
SORT_COLUMN_KEYS[columnIndex]); otherwise leave the direction unset while
preserving each column’s index.
---
Nitpick comments:
In `@src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx`:
- Around line 86-100: Extract the identical getVMStatusLabelColor logic into a
shared utility, then remove the local definitions and import the shared helper
in src/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsx (lines
86-100) and
src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx
(lines 60-74). Preserve the existing status-to-color behavior at both call
sites.
In `@src/views/nads/details/tabs/virtualMachines/NADVirtualMachines.tsx`:
- Line 46: Extract the inline prop object type from the NADVirtualMachines
component into a named Props type, matching the pattern used by NADYAML and
NADDescriptionDetails, then update the component declaration to use Props while
preserving the optional obj property and its NetworkAttachmentDefinitionKind
type.
In `@src/views/udns/details/tabs/details/UDNDetailsTab.tsx`:
- Line 2: Replace the namespace lodash import in UDNDetailsTab with a named size
import, then update both _.size usages in the component to call size directly.
Leave all other lodash usage unchanged.
🪄 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: b9ba4197-28d9-458c-bfa5-efefdd84701b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
locales/en/plugin__networking-console-plugin.jsonlocales/es/plugin__networking-console-plugin.jsonlocales/fr/plugin__networking-console-plugin.jsonlocales/ja/plugin__networking-console-plugin.jsonlocales/ko/plugin__networking-console-plugin.jsonlocales/zh/plugin__networking-console-plugin.jsonpackage.jsonsrc/utils/components/ConfirmModal/ConfirmModal.tsxsrc/utils/components/VMNetworkTable/VMNetworkColumnManagement.tsxsrc/utils/components/VMNetworkTable/constants.tssrc/utils/components/VMNetworkTable/useVMNetworkTableColumns.tssrc/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsxsrc/utils/hooks/useVMResourceLookups.tssrc/utils/resources/vm/selectors.tssrc/utils/resources/vm/utils.tssrc/utils/resources/vm/vmResourceLookups.tssrc/views/nads/actions/NADActions.tsxsrc/views/nads/details/NADDetailsPage.tsxsrc/views/nads/details/components/NADDetailsPageTitle.tsxsrc/views/nads/details/hooks/useNADTabs.tsxsrc/views/nads/details/hooks/useNADVirtualMachines.tssrc/views/nads/details/tabs/details/NADDescriptionDetails.tsxsrc/views/nads/details/tabs/details/NADDetails.tsxsrc/views/nads/details/tabs/details/NADTypeDetails.tsxsrc/views/nads/details/tabs/virtualMachines/NADVirtualMachines.tsxsrc/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsxsrc/views/nads/details/tabs/virtualMachines/components/NADVirtualMachineRowActions.tsxsrc/views/nads/details/tabs/yaml/NADYAML.tsxsrc/views/nads/manifest.tssrc/views/udns/details/UDNDetailsPage.tsxsrc/views/udns/details/components/UDNDetailsPageTitle.tsxsrc/views/udns/details/hooks/useUDNTabs.tsxsrc/views/udns/details/hooks/useUDNVirtualMachines.tssrc/views/udns/details/tabs/details/UDNDetailsTab.tsxsrc/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsxsrc/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsxsrc/views/udns/details/tabs/virtualMachines/components/UDNVirtualMachineRowActions.tsxsrc/views/udns/details/tabs/yaml/UDNYAML.tsxsrc/views/udns/manifest.ts
pcbailey
reviewed
Jul 28, 2026
pcbailey
left a comment
pcbailey
left a comment
Contributor
There was a problem hiding this comment.
Check CodeRabbit comments and squash smaller commits where it makes sense.
| virtualMachines: UDNVirtualMachine[]; | ||
| }; | ||
|
|
||
| const networkUsesUDN = (network: V1Network, udn: UDNResource, vm: V1VirtualMachine): boolean => { |
Contributor
There was a problem hiding this comment.
These functions should be in a utils file for the hook.
| @@ -0,0 +1,136 @@ | |||
| import React, { FC } from 'react'; | |||
| import * as _ from 'lodash'; | |||
Contributor
There was a problem hiding this comment.
Import only needed functions.
|
|
||
| const UDNDetailsTab: FC<UDNDetailsTabProps> = ({ obj: udn }) => { | ||
| const { t } = useNetworkingTranslation(); | ||
| const metadata = udn?.metadata; |
Contributor
There was a problem hiding this comment.
Same as the VM version. Remove this and create variables for fields used multiple times.
| [direction, onSort], | ||
| ); | ||
|
|
||
| const columns = useMemo( |
Contributor
There was a problem hiding this comment.
Look for candidates to become custom hooks.
| obj?: ClusterUserDefinedNetworkKind | UserDefinedNetworkKind; | ||
| }; | ||
|
|
||
| const UDNYAML: FC<UDNYAMLProps> = ({ obj: udn }) => { |
Contributor
There was a problem hiding this comment.
Spell out NAD to save our eyes. 🙏🏼
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkladnit, Parthivk100 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 |
openshift-ci
Bot
added
the
approved
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/utils/utils/units.js`:
- Around line 208-218: Update validate.split to parse the complete value with an
anchored numeric-and-unit pattern, allowing an optional exponent in the numeric
portion so inputs such as 1e3 produce 1000 with no unit while preserving unit
extraction for values that include one.
In
`@src/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsx`:
- Around line 397-398: Update the failed-VM label construction around
getName(selected[index]) to include each VM’s namespace, formatting labels as
namespace/name. Preserve the existing fallback of returning an empty list when
the selected VM lacks a usable name, and use the VM’s existing namespace field.
In
`@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx`:
- Around line 408-429: Update the partial-failure handling around failedNames so
selected contains only the rejected virtual machines before displaying the
error. Derive the retained selection from the rejected result indexes, update
selection through the existing state setter, then preserve the current error
message and return flow.
- Around line 400-401: Update the virtual-machine add failure handling around
the NAD-not-found branch and the related lines to stop logging the full VM
resource or raw API error. Replace those payloads with a generic failure
category while preserving the existing rejection behavior and user-facing error
handling.
🪄 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: Pro Plus
Run ID: a86f3edb-db72-4954-a4a5-c7923d5c1346
📒 Files selected for processing (16)
locales/en/plugin__networking-console-plugin.jsonlocales/es/plugin__networking-console-plugin.jsonlocales/fr/plugin__networking-console-plugin.jsonlocales/ja/plugin__networking-console-plugin.jsonlocales/ko/plugin__networking-console-plugin.jsonlocales/zh/plugin__networking-console-plugin.jsonpackage.jsonsrc/utils/components/VMNetworkTable/vmNetworkDataViewContent.tsxsrc/utils/resources/vm/utils.tssrc/utils/utils/units.jssrc/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsxsrc/views/nads/list/components/NADsRow/NADsRow.tsxsrc/views/nads/manifest.tssrc/views/udns/details/tabs/details/UDNDetailsTab.tsxsrc/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsxsrc/views/udns/details/tabs/virtualMachines/components/UDNVirtualMachineRowActions.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- locales/fr/plugin__networking-console-plugin.json
- locales/es/plugin__networking-console-plugin.json
- locales/ko/plugin__networking-console-plugin.json
- locales/ja/plugin__networking-console-plugin.json
- locales/zh/plugin__networking-console-plugin.json
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/resources/vm/utils.ts (1)
429-440: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGuard detach removals against stale array indexes.
These unconditional removes can delete another NIC/network after a concurrent VM edit shifts an index. Use
removeAtIndex(or equivalenttestoperations) with the objects read above.Proposed fix
- const patches: PatchItem<unknown>[] = - networkIndex >= ifaceIndex - ? [ - { op: 'remove', path: `${NETWORK_PATH}/${networkIndex}`, value: undefined }, - { op: 'remove', path: `${INTERFACE_PATH}/${ifaceIndex}`, value: undefined }, - ] - : [ - { op: 'remove', path: `${INTERFACE_PATH}/${ifaceIndex}`, value: undefined }, - { op: 'remove', path: `${NETWORK_PATH}/${networkIndex}`, value: undefined }, - ]; - - return patchVM(vm, patches); + return patchVM(vm, [ + ...removeAtIndex({ index: networkIndex, path: NETWORK_PATH, value: network }), + ...removeAtIndex({ index: ifaceIndex, path: INTERFACE_PATH, value: iface }), + ]);🤖 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 `@src/utils/resources/vm/utils.ts` around lines 429 - 440, Update the patch construction in the VM detach flow to guard both interface and network removals against stale indexes. Replace the unconditional remove operations with removeAtIndex or equivalent test-and-remove operations using the corresponding objects read earlier, while preserving the existing removal ordering based on networkIndex and ifaceIndex.src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx (1)
100-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPage index is not clamped when the filtered set shrinks. Both pagination slices compute
startfrom the rawpage, so filtering while on a later page yields an empty slice with a non-emptyfilteredData— a blank table instead of theemptystate.
src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx#L100-L103: clamppagetoMath.ceil(sortedData.length / perPage)before slicing (or reset the page param whenfilters.namechanges).src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalData.ts#L60-L63: apply the same clamp/reset in the modal'spaginatedData.🤖 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 `@src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx` around lines 100 - 103, The pagination logic must clamp the requested page to the last valid page after filtering. In src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx lines 100-103, update paginatedData to clamp page using sortedData.length and perPage before slicing; apply the same change to paginatedData in src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalData.ts lines 60-63, preserving valid-page slicing and empty-data behavior.
♻️ Duplicate comments (2)
src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx (2)
127-134: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetry after a partial failure resubmits already-attached VMs.
Successful VMs stay in
selected; the nextAddre-attempts them andaddVMToNADrejects them as already connected. Narrowselectedto the rejected VMs before settingerrorMessage.🤖 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 `@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx` around lines 127 - 134, Update the partial-failure handling in AddUDNVirtualMachinesModal so selected contains only the VMs listed in failedNames before calling setErrorMessage. Preserve the existing failure message and return behavior, allowing retries to resubmit only rejected VMs.
104-104: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStill logging full VM objects and raw API errors.
vmandresult.reasoncan carry customer metadata/spec and internal endpoint details; log a generic failure category plus the VM name instead. As per coding guidelines, "Flag logging that may expose passwords, tokens, API keys, PII ... internal hostnames, or customer data".Also applies to: 117-121
🤖 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 `@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx` at line 104, Update the failure logging in the AddUDNVirtualMachinesModal flow, including the NAD-not-found path and the result.reason handling around the affected lines, to avoid logging full VM objects or raw API errors. Log only a generic failure category and the VM name, using the existing VM name field, while preserving the current failure handling behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
src/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalSelection.ts (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated VM-equality logic.
The
matchOptionpassed touseDataViewSelectionand the localvmMatchescallback implement identical name+namespace equality checks. Extracting a single shared comparator avoids drift if the identity criteria change later.♻️ Proposed refactor
+const vmMatches = (a: V1VirtualMachine, b: V1VirtualMachine) => + getName(a) === getName(b) && getNamespace(a) === getNamespace(b); + const useAddNADVMModalSelection = ({...}) => { const {...} = useDataViewSelection<V1VirtualMachine>({ - matchOption: (item, another) => - getName(item) === getName(another) && getNamespace(item) === getNamespace(another), + matchOption: vmMatches, }); ... - const vmMatches = useCallback( - (a: V1VirtualMachine, b: V1VirtualMachine) => - getName(a) === getName(b) && getNamespace(a) === getNamespace(b), - [], - );Also applies to: 57-61
🤖 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 `@src/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalSelection.ts` around lines 26 - 28, Extract the name-and-namespace comparison used by matchOption and the local vmMatches callback into one shared VM comparator, then reuse it in both useDataViewSelection and the selection logic. Preserve the existing equality behavior while eliminating the duplicated checks.src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalSelection.ts (1)
26-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the VM identity predicate once.
matchOptionandvmMatchesimplement the same name+namespace comparison; a single shared helper avoids the two drifting apart (and it is the same key used forrowIdinuseAddUDNVMModalTable).Also applies to: 57-61
🤖 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 `@src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalSelection.ts` around lines 26 - 28, Extract the shared VM identity comparison of name and namespace into a helper, then reuse it for both matchOption and vmMatches in useAddUDNVMModalSelection and for the rowId key in useAddUDNVMModalTable. Remove the duplicated inline predicates while preserving the existing matching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/views/nads/details/hooks/useNADVirtualMachines/utils/utils.ts`:
- Around line 24-26: Require a matching interface before reporting a VM as
connected: update the interface-connection check in
src/views/nads/details/hooks/useNADVirtualMachines/utils/utils.ts lines 24-26 to
return false when getInterfaces(vm).find finds no interface, and apply the same
paired-interface requirement in
src/views/udns/details/hooks/useUDNVirtualMachines/utils/utils.ts lines 14-21.
In `@src/views/udns/details/tabs/details/UDNDetailsTab.tsx`:
- Around line 106-114: Update the translation resources used by the
UDNDetailsTab annotation calls to define the i18next plural keys annotation_one
and annotation_other for every affected locale, preserving the existing
annotation text and adding missing forms so t('annotation', { count:
annotationCount }) always resolves instead of returning the raw key.
In
`@src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalTable.tsx`:
- Around line 47-58: Update getSortParams and UseAddUDNVMModalTableParams to
accept the current sortBy value from the useAddUDNVMModalData caller, then apply
the sort direction only when sortBy.index matches columnIndex; otherwise provide
the inactive direction expected by the table headers. Preserve the existing
onSort behavior and thread the sortBy dependency through the callback.
---
Outside diff comments:
In `@src/utils/resources/vm/utils.ts`:
- Around line 429-440: Update the patch construction in the VM detach flow to
guard both interface and network removals against stale indexes. Replace the
unconditional remove operations with removeAtIndex or equivalent test-and-remove
operations using the corresponding objects read earlier, while preserving the
existing removal ordering based on networkIndex and ifaceIndex.
In `@src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx`:
- Around line 100-103: The pagination logic must clamp the requested page to the
last valid page after filtering. In
src/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsx lines
100-103, update paginatedData to clamp page using sortedData.length and perPage
before slicing; apply the same change to paginatedData in
src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalData.ts
lines 60-63, preserving valid-page slicing and empty-data behavior.
---
Duplicate comments:
In
`@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx`:
- Around line 127-134: Update the partial-failure handling in
AddUDNVirtualMachinesModal so selected contains only the VMs listed in
failedNames before calling setErrorMessage. Preserve the existing failure
message and return behavior, allowing retries to resubmit only rejected VMs.
- Line 104: Update the failure logging in the AddUDNVirtualMachinesModal flow,
including the NAD-not-found path and the result.reason handling around the
affected lines, to avoid logging full VM objects or raw API errors. Log only a
generic failure category and the VM name, using the existing VM name field,
while preserving the current failure handling behavior.
---
Nitpick comments:
In
`@src/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalSelection.ts`:
- Around line 26-28: Extract the name-and-namespace comparison used by
matchOption and the local vmMatches callback into one shared VM comparator, then
reuse it in both useDataViewSelection and the selection logic. Preserve the
existing equality behavior while eliminating the duplicated checks.
In
`@src/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalSelection.ts`:
- Around line 26-28: Extract the shared VM identity comparison of name and
namespace into a helper, then reuse it for both matchOption and vmMatches in
useAddUDNVMModalSelection and for the rowId key in useAddUDNVMModalTable. Remove
the duplicated inline predicates while preserving the existing matching
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a806962f-fa68-40ed-9a1b-e05e8a4488b5
📒 Files selected for processing (31)
src/utils/resources/nads/types/types.tssrc/utils/resources/udns/constants.tssrc/utils/resources/udns/helper.tssrc/utils/resources/vm/constants.tssrc/utils/resources/vm/utils.tssrc/views/nads/details/hooks/useNADTabs.tsxsrc/views/nads/details/hooks/useNADVirtualMachines/index.tssrc/views/nads/details/hooks/useNADVirtualMachines/useNADVirtualMachines.tssrc/views/nads/details/hooks/useNADVirtualMachines/utils/utils.tssrc/views/nads/details/tabs/details/NADDetails.tsxsrc/views/nads/details/tabs/virtualMachines/NADVirtualMachines.tsxsrc/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsxsrc/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalData.tssrc/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalSelection.tssrc/views/nads/details/tabs/virtualMachines/components/hooks/useAddNADVMModalTable.tsxsrc/views/nads/details/tabs/virtualMachines/constants.tssrc/views/nads/details/tabs/virtualMachines/utils.tssrc/views/nads/details/tabs/yaml/NetworkAttachmentDefinitionYAML.tsxsrc/views/udns/details/hooks/useUDNTabs.tsxsrc/views/udns/details/hooks/useUDNVirtualMachines/index.tssrc/views/udns/details/hooks/useUDNVirtualMachines/useUDNVirtualMachines.tssrc/views/udns/details/hooks/useUDNVirtualMachines/utils/utils.tssrc/views/udns/details/tabs/details/UDNDetailsTab.tsxsrc/views/udns/details/tabs/virtualMachines/UDNVirtualMachines.tsxsrc/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsxsrc/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalData.tssrc/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalSelection.tssrc/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalTable.tsxsrc/views/udns/details/tabs/virtualMachines/constants.tssrc/views/udns/details/tabs/virtualMachines/utils.tssrc/views/udns/details/tabs/yaml/UserDefinedNetworkYAML.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx (1)
133-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse nullable-safe
namespace/namelabels in both failure messages.VM names are namespace-scoped. The UDN flow is ambiguous for duplicate names, while the NAD flow can render
undefined/name.
src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx#L133-L136: includegetNamespace(vm)before the name.src/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsx#L105-L105: prefix with${namespace}/only when a namespace exists.🤖 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 `@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx` around lines 133 - 136, The failure messages need unambiguous, nullable-safe VM labels. In src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx lines 133-136, update the failedNames mapping to include getNamespace(vm) before the name; in src/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsx line 105, prefix the name with namespace and a slash only when the namespace exists, avoiding undefined/name output.
🤖 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.
Duplicate comments:
In
`@src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx`:
- Around line 133-136: The failure messages need unambiguous, nullable-safe VM
labels. In
src/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsx
lines 133-136, update the failedNames mapping to include getNamespace(vm) before
the name; in
src/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsx
line 105, prefix the name with namespace and a slash only when the namespace
exists, avoiding undefined/name output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a6d33a8-dfcb-4574-a50a-597834bdfa66
📒 Files selected for processing (8)
src/utils/utils/constants.tssrc/utils/utils/units.jssrc/views/nads/details/hooks/useNADVirtualMachines/utils/utils.tssrc/views/nads/details/tabs/virtualMachines/components/AddNADVirtualMachinesModal.tsxsrc/views/udns/details/hooks/useUDNVirtualMachines/utils/utils.tssrc/views/udns/details/tabs/virtualMachines/components/AddUDNVirtualMachinesModal.tsxsrc/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalSelection.tssrc/views/udns/details/tabs/virtualMachines/components/hooks/useAddUDNVMModalTable.tsx
pcbailey
reviewed
Jul 30, 2026
| return defaultReturn; | ||
| } | ||
|
|
||
| let units_ = unitArray.slice().reverse(); |
Contributor
There was a problem hiding this comment.
Why the underscore at the end of the name?
|
|
||
| // find which unit we're given | ||
| let truncateStringAt = -1; | ||
| const startingUnitIndex = _.findIndex(units_, function (currentUnitValue) { |
Contributor
There was a problem hiding this comment.
Please remove the general lodash import and import only the functions that are used.
| return { unit, value }; | ||
| }; | ||
|
|
||
| const convertValueWithUnitsToBaseValue = (value, unitArray, divisor) => { |
Contributor
There was a problem hiding this comment.
Where did all of this units-based code come from?
|
@Parthivk100: all tests passed! 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
4 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.
Virtual Machines tab on NAD/UDN/CUDN detail pages
Screenshare.-.2026-07-30.4_35_11.PM.mp4
Summary by CodeRabbit