CNV-77263: redesign the cd-rom modal #3455
Conversation
|
@batyana: This pull request references CNV-77263 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 NOT APPROVED This pull-request has been approved by: batyana The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughReworks CD‑ROM modal to a radio-based source selector (Mount ISO / Upload / Empty), extracts submission/upload logic into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
@batyana: This pull request references CNV-77263 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/details/tabs/configuration/storage/components/modal/hooks/useMountCDROMForm.ts (1)
74-74:⚠️ Potential issue | 🟠 Major
isFormValiddoesn't account for the empty drive mode — "Add" button will be disabled.When the user selects "Leave empty drive",
selectedISOis''anduploadFileisnull, soisFormValidevaluates tofalse. The submit button will remain disabled for the empty drive option.🐛 Proposed fix
- const isFormValid = Boolean(selectedISO || uploadFile?.filename); + const isFormValid = Boolean(selectedISO || uploadFile?.filename || uploadMode === UPLOAD_MODE_EMPTY);
🤖 Fix all issues with AI agents
In `@src/utils/components/DiskModal/utils/helpers.ts`:
- Around line 330-339: createMutableUploadData currently always fabricates a
dataVolumeTemplate when data.dataVolumeTemplate is undefined; change it to guard
for existence of data.dataVolumeTemplate and only shallow-copy/modify
spec.source when the template exists—i.e., in createMutableUploadData, detect if
data.dataVolumeTemplate is truthy and then return a new object with
dataVolumeTemplate: { ...data.dataVolumeTemplate, spec: {
...data.dataVolumeTemplate.spec, source: {
...data.dataVolumeTemplate.spec?.source } } }, otherwise return the original
data unchanged (or copy other fields without adding dataVolumeTemplate);
reference createMutableUploadData and the dataVolumeTemplate/spec/source
properties to locate the change.
In `@src/utils/components/DiskModal/utils/submit.ts`:
- Around line 239-256: The hot-plug upload path creates the DataVolume with a
generated name inside uploadDataVolume (which uses generateUploadDiskName) but
then reads dvName from data.dataVolumeTemplate?.metadata?.name (which may
instead have been set by createDataVolumeName), causing a mismatch; fix by using
the DataVolume returned by uploadDataVolume to update
data.dataVolumeTemplate.metadata.name and data.volume.dataVolume.name (or
replace data.dataVolumeTemplate and data.volume.dataVolume with the returned
object) inside the uploadPromise.then handler (and keep calling
onUploadedDataVolume/uploadStarted as before) so the VM volume references the
actual uploaded DataVolume name generated by uploadDataVolume.
In
`@src/views/virtualmachines/details/tabs/configuration/storage/components/hooks/useUploadAlert.ts`:
- Around line 8-23: The hook useUploadAlert should declare explicit return types
and its internal handlers should be typed: add a return type for useUploadAlert
(including uploadStatus, uploadError, onUploadStarted, dismissAlert), annotate
onUploadStarted as (uploadPromise: Promise<unknown>) => void and dismissAlert as
() => void, and change the catch handler parameter to err: unknown and narrow it
(e.g., if (err instanceof Error) setUploadError(err.message) else
setUploadError(String(err))) before calling setUploadError; keep using
setUploadStatus('error'/'success'/'uploading') as before.
🧹 Nitpick comments (4)
src/views/virtualmachines/details/tabs/configuration/storage/utils/constants.ts (1)
5-24: RenameuploadAlertConfigto uppercase constant style.This is a utility constant and should follow the repo’s uppercase underscore naming convention. Update call sites accordingly.
Proposed fix
-export const uploadAlertConfig: Record< +export const UPLOAD_ALERT_CONFIG: Record< UploadAlertStatus, { body: string; title: string; variant: AlertVariant } > = {As per coding guidelines, "Define constants in utility files with uppercase and underscore-separated naming (e.g.,
API_URL)."src/utils/components/DiskModal/utils/submit.ts (2)
217-274: Missing explicit return type onsubmitCDROM.Per coding guidelines, functions should always have explicit return types rather than relying on inference.
Proposed fix
export const submitCDROM = async ( data: V1DiskFormState, { isHotPluggable, onSubmit, onUploadedDataVolume, onUploadStarted, selectedISO, uploadData: uploadDataFn, uploadEnabled, vm, }: SubmitCDROMInput, -) => { +): Promise<V1VirtualMachine | void> => {As per coding guidelines, "Always explicitly define return types for functions rather than relying on TypeScript type inference."
230-269: Direct mutation ofdataparameter may cause issues.
submitCDROMmutates itsdataparameter in-place (delete data.dataVolumeTemplate,data.volume = ..., etc.). If the caller or any.then()handler referencesdataafter this call, it will see the mutated state. Consider working on a local copy instead of mutating the input.src/utils/components/DiskModal/AddCDROMModal.tsx (1)
94-104: WraphandleModalSubmitinuseCallbackand add an explicit return type.
handleModalSubmitis recreated every render and passed asonSubmittoTabModal. This can trigger unnecessary child re-renders. Per the project's coding guidelines, memoization tools should be used, and functions should have explicit return types.♻️ Suggested refactor
- const handleModalSubmit = () => - submitCDROM(getValues(), { + const handleModalSubmit = useCallback((): Promise<void> => + submitCDROM(getValues(), { isHotPluggable, onSubmit, onUploadedDataVolume, onUploadStarted, selectedISO, uploadData, uploadEnabled, vm, - }); + }), [getValues, isHotPluggable, onSubmit, onUploadedDataVolume, onUploadStarted, selectedISO, uploadData, uploadEnabled, vm]);You'll also need to add
useCallbackto the React import on line 1:import React, { FC, useCallback, useEffect } from 'react';As per coding guidelines: "Use React's memoization tools (
React.memo,useMemo,useCallback) to avoid unnecessary re-renders." and "Always explicitly define return types for functions rather than relying on TypeScript type inference."
| export const useUploadAlert = () => { | ||
| const [uploadStatus, setUploadStatus] = useState<null | UploadAlertStatus>(null); | ||
| const [uploadError, setUploadError] = useState(''); | ||
|
|
||
| const onUploadStarted = (uploadPromise: Promise<unknown>) => { | ||
| setUploadStatus('uploading'); | ||
| setUploadError(''); | ||
| uploadPromise | ||
| .then(() => setUploadStatus('success')) | ||
| .catch((err) => { | ||
| setUploadStatus('error'); | ||
| setUploadError(err?.message || ''); | ||
| }); | ||
| }; | ||
|
|
||
| const dismissAlert = () => setUploadStatus(null); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "src/views/virtualmachines/details/tabs/configuration/storage/components/hooks/useUploadAlert.ts"Repository: kubevirt-ui/kubevirt-plugin
Length of output: 1117
🏁 Script executed:
cat -n "src/views/virtualmachines/details/tabs/configuration/storage/utils/constants.ts" | head -50Repository: kubevirt-ui/kubevirt-plugin
Length of output: 951
Add explicit return types and type the catch handler error as unknown.
Guidelines require explicit function return types for all functions in TypeScript files. Additionally, the caught error must be typed as unknown and narrowed to avoid implicit any.
Suggested fix
+type UploadAlertConfig = (typeof uploadAlertConfig)[UploadAlertStatus];
+
+type UseUploadAlertResult = {
+ alertConfig: UploadAlertConfig | null;
+ dismissAlert: () => void;
+ onUploadStarted: (uploadPromise: Promise<unknown>) => void;
+ uploadError: string;
+};
+
-export const useUploadAlert = () => {
+export const useUploadAlert = (): UseUploadAlertResult => {
const [uploadStatus, setUploadStatus] = useState<null | UploadAlertStatus>(null);
const [uploadError, setUploadError] = useState('');
- const onUploadStarted = (uploadPromise: Promise<unknown>) => {
+ const onUploadStarted = (uploadPromise: Promise<unknown>): void => {
setUploadStatus('uploading');
setUploadError('');
uploadPromise
.then(() => setUploadStatus('success'))
- .catch((err) => {
+ .catch((err: unknown) => {
setUploadStatus('error');
- setUploadError(err?.message || '');
+ setUploadError(err instanceof Error ? err.message : '');
});
};
- const dismissAlert = () => setUploadStatus(null);
+ const dismissAlert = (): void => setUploadStatus(null);🤖 Prompt for AI Agents
In
`@src/views/virtualmachines/details/tabs/configuration/storage/components/hooks/useUploadAlert.ts`
around lines 8 - 23, The hook useUploadAlert should declare explicit return
types and its internal handlers should be typed: add a return type for
useUploadAlert (including uploadStatus, uploadError, onUploadStarted,
dismissAlert), annotate onUploadStarted as (uploadPromise: Promise<unknown>) =>
void and dismissAlert as () => void, and change the catch handler parameter to
err: unknown and narrow it (e.g., if (err instanceof Error)
setUploadError(err.message) else setUploadError(String(err))) before calling
setUploadError; keep using setUploadStatus('error'/'success'/'uploading') as
before.
batyana
force-pushed
the
CNV-77263-redesign-add-cd-rom-modal
branch
from
ab0acd4 to
ee93633
Compare
|
@batyana: This pull request references CNV-77263 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. |
|
@yfrimanm - do you think we should use |
|
@ronensdeor please review @galkremer1 question. Thanks. |
@yfrimanm is good with that change - @batyana please update the description message, thank you. |
Signed-off-by: batyana <[email protected]>
batyana
force-pushed
the
CNV-77263-redesign-add-cd-rom-modal
branch
from
ee93633 to
01e4d66
Compare
|
@batyana: This pull request references CNV-77263 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/details/tabs/configuration/storage/components/modal/hooks/useMountCDROMForm.ts (1)
74-74:⚠️ Potential issue | 🟡 MinorInclude
uploadMode === UPLOAD_MODE_EMPTYin theisFormValidcalculation.The hook exports
handleEmptyDriveSelection, which setsuploadModetoUPLOAD_MODE_EMPTY, but theisFormValidcheck on line 74 only validatesselectedISOoruploadFile?.filename. When a user selects "Leave empty drive", both values are empty, makingisFormValidreturnfalsedespite being a valid selection.While
AddCDROMModalcompensates by computing its own validity check, the hook's exportedisFormValidis incomplete and misleading for any other consumer.Proposed fix
- const isFormValid = Boolean(selectedISO || uploadFile?.filename); + const isFormValid = Boolean(selectedISO || uploadFile?.filename || uploadMode === UPLOAD_MODE_EMPTY);
🤖 Fix all issues with AI agents
In `@src/utils/components/DiskModal/utils/submit.ts`:
- Around line 292-294: The current forked chain on uploadPromise
(.then(...).catch(...)) logs errors but swallows them, and onUploadStarted is
still given the original rejecting uploadPromise; also any error thrown inside
onUploadedDataVolume will be caught and suppressed by that catch. Replace the
fork with a chained promise that preserves rejection: create a new promise like
uploadPromise.then((u) => onUploadedDataVolume?.(u)).catch((err) => {
kubevirtConsole.error('CD-ROM upload error:', err); throw err; }) and pass that
chained promise to onUploadStarted (or return it) so errors are logged but
rethrown and propagate to consumers (references: uploadPromise,
onUploadedDataVolume, kubevirtConsole.error, onUploadStarted, useUploadAlert).
🧹 Nitpick comments (2)
src/utils/components/DiskModal/utils/submit.ts (1)
206-215:SubmitCDROMInputcould be exported for testability.The type is private, which is fine for current usage, but exporting it would make it easier to unit-test
submitCDROMin isolation (constructing typed mock inputs). This is a minor suggestion.src/utils/components/DiskModal/AddCDROMModal.tsx (1)
94-104:handleModalSubmitis missing an explicit return type.Per coding guidelines: "Always explicitly define return types for functions."
Proposed fix
- const handleModalSubmit = () => + const handleModalSubmit = (): Promise<V1VirtualMachine | void> => submitCDROM(getValues(), {As per coding guidelines, "Always explicitly define return types for functions rather than relying on TypeScript type inference."
| uploadPromise | ||
| .then((uploaded) => onUploadedDataVolume?.(uploaded)) | ||
| .catch((err) => kubevirtConsole.error('CD-ROM upload error:', err)); |
There was a problem hiding this comment.
Swallowed upload errors — the .catch only logs, but onUploadStarted still receives the rejecting promise.
The .catch on line 294 logs the error but also means the promise chain itself resolves (the catch handler returns undefined). Meanwhile, onUploadStarted on line 295 receives the original uploadPromise (before .then/.catch), which will reject. This is fine for the useUploadAlert hook (which attaches its own .catch), but be aware that .then/.catch on line 292–294 creates a separate fork that won't propagate to onUploadStarted's consumer. If onUploadedDataVolume throws, that error is silently swallowed by the .catch on line 294.
🤖 Prompt for AI Agents
In `@src/utils/components/DiskModal/utils/submit.ts` around lines 292 - 294, The
current forked chain on uploadPromise (.then(...).catch(...)) logs errors but
swallows them, and onUploadStarted is still given the original rejecting
uploadPromise; also any error thrown inside onUploadedDataVolume will be caught
and suppressed by that catch. Replace the fork with a chained promise that
preserves rejection: create a new promise like uploadPromise.then((u) =>
onUploadedDataVolume?.(u)).catch((err) => { kubevirtConsole.error('CD-ROM upload
error:', err); throw err; }) and pass that chained promise to onUploadStarted
(or return it) so errors are logged but rethrown and propagate to consumers
(references: uploadPromise, onUploadedDataVolume, kubevirtConsole.error,
onUploadStarted, useUploadAlert).
There was a problem hiding this comment.
@batyana this is a really good point. Why did u decide not to await this promise?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| draft.volume = { | ||
| name: draft.volume.name, | ||
| persistentVolumeClaim: { | ||
| claimName: selectedISO, | ||
| ...(isHotPluggable && { hotpluggable: true }), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
With produce u should be able to do that. draft is already a copy of the original object so i don't want to do
name: draft.volume.name
| draft.volume = { | |
| name: draft.volume.name, | |
| persistentVolumeClaim: { | |
| claimName: selectedISO, | |
| ...(isHotPluggable && { hotpluggable: true }), | |
| }, | |
| }; | |
| draft.volume.persistentVolumeClaim = { | |
| claimName: selectedISO, | |
| ...(isHotPluggable && { hotpluggable: true }), | |
| }; |
| draft.volume = { | ||
| name: draft.volume.name, | ||
| persistentVolumeClaim: { claimName }, | ||
| }; | ||
| delete draft.dataVolumeTemplate; |
5 participants
📝 Description
🎥 Demo
Screen.Recording.2026-02-11.at.14.26.21.mov
Summary by CodeRabbit
New Features
Localization