[WIP] CNV-77372: uncheck shared disks on delete vm modal #3434
Conversation
|
@upalatucci: This pull request references CNV-77372 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: upalatucci 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 |
📝 WalkthroughWalkthroughThis PR simplifies disk deletion translation labels across multiple locales and introduces shared volume support to VM deletion workflows. It adds new selectors for detecting shared disks/volumes and updates the DeleteVMModal component tree to identify and preserve shared resources during deletion. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@upalatucci: This pull request references CNV-77372 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/views/virtualmachines/actions/components/DeleteVMModal/components/DeleteResourceCheckbox.tsx`:
- Around line 38-44: The label JSX in DeleteResourceCheckbox.tsx currently
renders the literal text "Shared" without translation and too close to
resourceName; change it to use the translation function (e.g., t('Shared')) and
add spacing/separator so it doesn't run into resourceName. Locate the label
where ResourceIcon, modelToGroupVersionKind(getResourceModel(resource)),
resourceName and isShared are used and replace the {isShared ?
<Label>Shared</Label> : undefined} with a localized Label (e.g.,
<Label>{t('Shared')}</Label>) and ensure there is appropriate spacing or margin
between resourceName and the Label so the UI displays correctly.
In
`@src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useSavePVCs.ts`:
- Around line 14-17: The useEffect in useSavePVCs is re-adding the entire pvcs
array on every pvcs/filteredPvcs change; change the effect to only append PVCs
that are not already present in PVCsToSave (compare by unique key like name/uid)
so user deselections and duplicates are preserved, and annotate the hook
signature with an explicit return type of void; update references to
setPVCsToSave and pvcs/filteredPvcs in the effect to perform a deduplicating
merge rather than spreading the whole array.
🧹 Nitpick comments (4)
src/utils/resources/vm/utils/selectors.ts (1)
59-82: Make shared selectors return typed, non-optional arrays.This avoids nullable handling downstream and aligns with the explicit return-type guideline.
Proposed change
-export const getSharedDisks = (vm: V1VirtualMachine) => - getDisks(vm)?.filter((disk) => disk.shareable); +export const getSharedDisks = (vm: V1VirtualMachine): V1Disk[] => + (getDisks(vm) ?? []).filter((disk) => Boolean(disk.shareable)); -export const getSharedVolumes = (vm: V1VirtualMachine) => { - const sharedDisks = getSharedDisks(vm)?.map((disk) => disk.name); - return getVolumes(vm)?.filter((volume) => sharedDisks?.includes(volume.name)); -}; +export const getSharedVolumes = (vm: V1VirtualMachine): V1Volume[] => { + const sharedDisks = getSharedDisks(vm).map((disk) => disk.name); + return (getVolumes(vm) ?? []).filter((volume) => sharedDisks.includes(volume.name)); +};As per coding guidelines, Always explicitly define return types for functions rather than relying on TypeScript inference.
src/views/virtualmachines/actions/components/DeleteVMModal/utils/helpers.ts (1)
109-135: Add explicit return types (and guard nullish inputs) for shared helpers.Proposed change
export const getSharedDataVolumes = ( sharedVolumes: V1Volume[], dataVolumes: V1beta1DataVolume[], -) => { - const sharedDataVolumeNames = sharedVolumes - ?.map((volume) => volume?.dataVolume?.name) - ?.filter(Boolean); - return dataVolumes?.filter((dataVolume) => sharedDataVolumeNames?.includes(getName(dataVolume))); -}; +): V1beta1DataVolume[] => { + const sharedDataVolumeNames = (sharedVolumes ?? []) + .map((volume) => volume?.dataVolume?.name) + .filter(Boolean); + return (dataVolumes ?? []).filter((dataVolume) => + sharedDataVolumeNames.includes(getName(dataVolume)), + ); +}; -export const isResourceShared = (sharedVolumes: V1Volume[], resource: K8sResourceCommon) => { - return sharedVolumes.some( - (volume) => - volume?.dataVolume?.name === getName(resource) || - volume?.persistentVolumeClaim?.claimName === getName(resource), - ); -}; +export const isResourceShared = ( + sharedVolumes: V1Volume[], + resource: K8sResourceCommon, +): boolean => + (sharedVolumes ?? []).some( + (volume) => + volume?.dataVolume?.name === getName(resource) || + volume?.persistentVolumeClaim?.claimName === getName(resource), + ); -export const getResourceModel = (resource: K8sResourceCommon) => { +export const getResourceModel = ( + resource: K8sResourceCommon, +): + | typeof PersistentVolumeClaimModel + | typeof VirtualMachineSnapshotModel + | typeof DataVolumeModel => { if (resource.kind === PersistentVolumeClaimModel.kind) { return PersistentVolumeClaimModel; } if (resource.kind === VirtualMachineSnapshotModel.kind) { return VirtualMachineSnapshotModel; } return DataVolumeModel; };As per coding guidelines, Always explicitly define return types for functions rather than relying on TypeScript inference.
src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useSaveSharedDisks.ts (1)
15-31: Avoid duplicate saved volumes and add an explicit return type.Proposed change
const useSaveSharedDisks = ( sharedVolumes: V1Volume[], setVolumesToSave: Dispatch< SetStateAction<(IoK8sApiCoreV1PersistentVolumeClaim | V1beta1DataVolume)[]> >, dataVolumes: V1beta1DataVolume[], -) => { +): void => { const sharedDataVolumes = useMemo( () => getSharedDataVolumes(sharedVolumes, dataVolumes), [sharedVolumes, dataVolumes], ); useEffect(() => { - if (sharedDataVolumes) { - setVolumesToSave((prevVolumes) => [...prevVolumes, ...sharedDataVolumes]); - } + if (!sharedDataVolumes?.length) return; + setVolumesToSave((prevVolumes) => { + const next = [...prevVolumes]; + sharedDataVolumes.forEach((dv) => { + if (!next.some((v) => v?.metadata?.uid === dv?.metadata?.uid)) { + next.push(dv); + } + }); + return next; + }); }, [setVolumesToSave, sharedDataVolumes]); };As per coding guidelines, Always explicitly define return types for functions rather than relying on TypeScript inference.
src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useSavePVCs.ts (1)
10-13: Add an explicit return type touseSavePVCs.This hook currently relies on inference. As per coding guidelines, always explicitly define return types for functions rather than relying on TypeScript inference.
✍️ Suggested change
-const useSavePVCs = ( +const useSavePVCs = ( setPVCsToSave: Dispatch<SetStateAction<IoK8sApiCoreV1PersistentVolumeClaim[]>>, pvcs: IoK8sApiCoreV1PersistentVolumeClaim[], -) => { +): void => {
| label={ | ||
| <> | ||
| {t('Delete disk')}{' '} | ||
| <ResourceIcon groupVersionKind={modelToGroupVersionKind(getResourceModel(resource))} />{' '} | ||
| {resourceName} | ||
| {isShared ? <Label>Shared</Label> : undefined} | ||
| </> |
There was a problem hiding this comment.
Localize the “Shared” label and add spacing.
“Shared” is user‑visible text and should be translated; also add a separator so it doesn’t run into the resource name.
🌐 Suggested fix
- {resourceName}
- {isShared ? <Label>Shared</Label> : undefined}
+ {resourceName}
+ {isShared ? (
+ <>
+ {' '}
+ <Label>{t('Shared')}</Label>
+ </>
+ ) : undefined}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label={ | |
| <> | |
| {t('Delete disk')}{' '} | |
| <ResourceIcon groupVersionKind={modelToGroupVersionKind(getResourceModel(resource))} />{' '} | |
| {resourceName} | |
| {isShared ? <Label>Shared</Label> : undefined} | |
| </> | |
| label={ | |
| <> | |
| {t('Delete disk')}{' '} | |
| <ResourceIcon groupVersionKind={modelToGroupVersionKind(getResourceModel(resource))} />{' '} | |
| {resourceName} | |
| {isShared ? ( | |
| <> | |
| {' '} | |
| <Label>{t('Shared')}</Label> | |
| </> | |
| ) : undefined} | |
| </> |
🤖 Prompt for AI Agents
In
`@src/views/virtualmachines/actions/components/DeleteVMModal/components/DeleteResourceCheckbox.tsx`
around lines 38 - 44, The label JSX in DeleteResourceCheckbox.tsx currently
renders the literal text "Shared" without translation and too close to
resourceName; change it to use the translation function (e.g., t('Shared')) and
add spacing/separator so it doesn't run into resourceName. Locate the label
where ResourceIcon, modelToGroupVersionKind(getResourceModel(resource)),
resourceName and isShared are used and replace the {isShared ?
<Label>Shared</Label> : undefined} with a localized Label (e.g.,
<Label>{t('Shared')}</Label>) and ensure there is appropriate spacing or margin
between resourceName and the Label so the UI displays correctly.
| useEffect(() => { | ||
| if (pvcs.length > 0) { | ||
| setPVCsToSave((prevPVCs) => [...prevPVCs, ...pvcs]); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "useSavePVCs" -C 5 --type=ts --type=tsxRepository: kubevirt-ui/kubevirt-plugin
Length of output: 98
🏁 Script executed:
rg -n "useSavePVCs" -C 5Repository: kubevirt-ui/kubevirt-plugin
Length of output: 4705
🏁 Script executed:
find . -name "useDeleteVMResources*" -type fRepository: kubevirt-ui/kubevirt-plugin
Length of output: 161
🏁 Script executed:
cat -n ./src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useDeleteVMResources.tsRepository: kubevirt-ui/kubevirt-plugin
Length of output: 4590
🏁 Script executed:
cat -n ./src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useSavePVCs.tsRepository: kubevirt-ui/kubevirt-plugin
Length of output: 931
Prevent re‑adding PVCs on every pvcs change; add explicit return type.
The effect spreads the full PVC list each time the pvcs reference changes. Since filteredPvcs in useDeleteVMResources is memoized but creates a new reference whenever pvcs, dvVolumesNames, isDataVolumeGarbageCollector, or pvcVolumesNames changes, this effect re-runs frequently and re-adds all PVCs to the saved list, undoing user deselections and creating duplicates.
Additionally, the hook lacks an explicit return type annotation; it should be void.
🛠️ Suggested fix
-import { Dispatch, SetStateAction, useEffect } from 'react';
+import { Dispatch, SetStateAction, useEffect, useRef } from 'react';
+
+import { getName } from '@kubevirt-utils/resources/shared';
import { IoK8sApiCoreV1PersistentVolumeClaim } from '@kubevirt-ui-ext/kubevirt-api/kubernetes';
-const useSavePVCs = (
+const useSavePVCs = (
setPVCsToSave: Dispatch<SetStateAction<IoK8sApiCoreV1PersistentVolumeClaim[]>>,
pvcs: IoK8sApiCoreV1PersistentVolumeClaim[],
-) => {
+): void => {
+ const autoAddedRef = useRef<Set<string>>(new Set());
+
useEffect(() => {
- if (pvcs.length > 0) {
- setPVCsToSave((prevPVCs) => [...prevPVCs, ...pvcs]);
- }
+ if (pvcs.length === 0) return;
+
+ setPVCsToSave((prevPVCs) => {
+ const nextPVCs = [...prevPVCs];
+
+ pvcs.forEach((pvc) => {
+ const key = pvc?.metadata?.uid || getName(pvc);
+ if (!autoAddedRef.current.has(key)) {
+ autoAddedRef.current.add(key);
+ nextPVCs.push(pvc);
+ }
+ });
+
+ return nextPVCs;
+ });
}, [setPVCsToSave, pvcs]);
};🤖 Prompt for AI Agents
In
`@src/views/virtualmachines/actions/components/DeleteVMModal/hooks/useSavePVCs.ts`
around lines 14 - 17, The useEffect in useSavePVCs is re-adding the entire pvcs
array on every pvcs/filteredPvcs change; change the effect to only append PVCs
that are not already present in PVCsToSave (compare by unique key like name/uid)
so user deselections and duplicates are preserved, and annotate the hook
signature with an explicit return type of void; update references to
setPVCsToSave and pvcs/filteredPvcs in the effect to perform a deduplicating
merge rather than spreading the whole array.
|
@upalatucci: 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. |
2 participants
📝 Description
Uncheck sharable disks and non-cloned PVCs.
Non-cloned PVCs (
volume.persistentVolumeClaim.claimName) can be also shared within other vms as they are not cloned during vm creation🎥 Demo
Summary by CodeRabbit
New Features
Chores