CNV-69048: Update logic for boot order modal / summary #3219
Conversation
|
@galkremer1: This pull request references CNV-69048 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 bug to target the "4.21.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: galkremer1 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 |
📝 WalkthroughWalkthroughBoot order handling was changed: BootOrderModal now sets bootOrder for devices based on their index; BootOrderModalBody stops mutating bootOrder on reorders and passes the reordered list through; BootOrderSummary filters to only show devices with a defined bootOrder. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.tsx📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-25T19:31:46.140ZApplied to files:
📚 Learning: 2025-12-09T17:30:56.131ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
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 |
|
|
||
| const onDrop = (source, dest) => { | ||
| if (dest) { | ||
| const newBootableDevices = reorder(devices, source.index, dest.index).map( |
There was a problem hiding this comment.
bootOrder is added when submitting the changes using the boot order modal.
| title={t('No resource selected')} | ||
| /> | ||
| ) : ( | ||
| <> |
There was a problem hiding this comment.
removed unnecessary Fragment wrapper.
|
@galkremer1: This pull request references CNV-69048 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 bug to target the "4.21.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/components/BootOrder/BootOrderSummary.tsx (1)
13-21: Guard againstgetSortedBootableDevicesreturningundefinedbefore calling.filter.
getSortedBootableDevicescan returnundefined, so calling.filteron it risks a runtime error when there are no bootable devices. Consider defaulting to an empty array before filtering:- const transformedDevices = useMemo( - () => - getSortedBootableDevices({ instanceTypeVM, vm }).filter( - (device) => device.value.bootOrder !== undefined, - ), - [vm, instanceTypeVM], - ); + const transformedDevices = useMemo( + () => + (getSortedBootableDevices({ instanceTypeVM, vm }) ?? []).filter( + (device) => device.value.bootOrder !== undefined, + ), + [vm, instanceTypeVM], + );
♻️ Duplicate comments (1)
src/utils/components/BootOrderModal/BootOrderModalBody.tsx (1)
46-65:onDeleteno longer actually “removes” boot order once persisted.
onDeletestill clearsbootOrderon the selected device and moves it to the end of the list, butBootOrderModalnow unconditionally assignsbootOrder = index + 1for every device on save. As a result, the clearedbootOrderis immediately reintroduced, and this handler no longer has its intended effect.Once you adjust
BootOrderModal(see corresponding comment there) to respectbootOrder: undefined, this function will regain its original meaning. Otherwise, if the new design is “all devices always have bootOrder,” you should update this handler and the comment to reflect the new behavior (e.g., treat it as “move to end” rather than “remove boot order”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/components/BootOrder/BootOrderSummary.tsx(1 hunks)src/utils/components/BootOrderModal/BootOrderModal.tsx(1 hunks)src/utils/components/BootOrderModal/BootOrderModalBody.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/utils/components/BootOrder/BootOrderSummary.tsx (1)
src/utils/resources/vm/utils/boot-order/bootOrder.ts (1)
getSortedBootableDevices(72-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit-test
- GitHub Check: i18n
- GitHub Check: build
🔇 Additional comments (1)
src/utils/components/BootOrderModal/BootOrderModalBody.tsx (1)
85-132: Drag-and-drop reordering implementation looks correct.The new
DragDrop/Droppable/Draggableblock correctly mapsdevicesin order, usesonDropto reorder by indices, and wires up the drag handle and delete button per item. No obvious logic or accessibility regressions introduced here.
| const updatedVirtualMachine = produce<V1VirtualMachine>(vm, (draftVM) => { | ||
| ensurePath(draftVM, ['spec.template.spec.domain.devices']); | ||
| const updatedDevices = devices.map((device, index) => ({ | ||
| ...device, | ||
| value: { | ||
| ...device.value, | ||
| bootOrder: index + 1, | ||
| }, | ||
| })); | ||
|
|
||
| draftVM.spec.template.spec.domain.devices.disks = devices | ||
| draftVM.spec.template.spec.domain.devices.disks = updatedDevices | ||
| .filter((source) => source.type === DeviceType.DISK) | ||
| .map((source) => source.value); | ||
|
|
||
| draftVM.spec.template.spec.domain.devices.interfaces = devices | ||
| draftVM.spec.template.spec.domain.devices.interfaces = updatedDevices | ||
| .filter((source) => source.type === DeviceType.NIC) | ||
| .map((source) => source.value); | ||
| }); |
There was a problem hiding this comment.
updatedDevices overwrites cleared bootOrder values, breaking “remove from boot order” semantics.
updatedDevices assigns bootOrder: index + 1 for every device, so any device whose bootOrder was cleared in BootOrderModalBody.onDelete is still persisted with a new bootOrder. That:
- Removes the ability to keep a device without explicit boot order.
- Makes the minus button effectively just move the device to the end.
- Conflicts with
BootOrderSummary, which now shows only devices withbootOrder !== undefined.
If the intent is to keep “unset boot order” as a supported state, you should only renumber devices that currently have bootOrder defined, preserving undefined:
- const updatedDevices = devices.map((device, index) => ({
- ...device,
- value: {
- ...device.value,
- bootOrder: index + 1,
- },
- }));
+ let nextOrder = 1;
+ const updatedDevices = devices.map((device) => {
+ const hasBootOrder = device.value.bootOrder !== undefined;
+
+ return {
+ ...device,
+ value: {
+ ...device.value,
+ bootOrder: hasBootOrder ? nextOrder++ : undefined,
+ },
+ };
+ });If instead the product decision is that all devices must always have a bootOrder after saving, then onDelete in BootOrderModalBody and the summary filtering logic should be aligned to that behavior (e.g., by removing the “clear boot order” path and/or changing the summary filter).
There was a problem hiding this comment.
For now I left the onDelete for reference. Basically it is just moving the device all the way down the list.
Per product decision since the bootOrder defaults to device order if it is not stated - we can just leave the boot order explicitly.
There was a problem hiding this comment.
@galkremer1, thank you for the clarification! That makes sense—if the product decision is to always maintain explicit bootOrder values for all devices (since KubeVirt defaults to device order anyway), then the current implementation correctly assigns bootOrder: index + 1 to every device on save.
Given this, the filtering in BootOrderSummary will show all devices after the modal is saved at least once, which aligns with the expected behavior.
One minor suggestion: since onDelete no longer actually removes a device from boot order but instead moves it to the end of the list, consider renaming it (e.g., onMoveToEnd or onMoveDown) or adding a comment to clarify its new purpose, to avoid confusion for future maintainers.
✏️ Learnings added
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3219
File: src/utils/components/BootOrderModal/BootOrderModal.tsx:36-53
Timestamp: 2025-11-25T19:31:46.118Z
Learning: In the KubeVirt plugin boot order modal, the product decision is to always set explicit `bootOrder` values for all devices when saving. Since KubeVirt defaults to device order if `bootOrder` is not specified, maintaining explicit boot order values for all devices is the intended behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
/retest |
| getSortedBootableDevices({ instanceTypeVM, vm }).filter( | ||
| (device) => device.value.bootOrder !== undefined, | ||
| ), |
There was a problem hiding this comment.
From what i understood, the bootOrder is required only for NIC
There was a problem hiding this comment.
As far as I understand - bootOrder is optional for both interfaces and disks.
If the order is not applied explicitly using the bootOrder property, it's applied implicitly by the order in the array (disks first, then interfaces).
There was a problem hiding this comment.
yes its optional. But it works differently for disks and interfaces.
For disks if no bootoder is provided, then we can use the index. but for interfaces if no bootoder is provided, the vm is not bootable using that interface
There was a problem hiding this comment.
I asked this question on the ticket and the backend team responded
There was a problem hiding this comment.
Thanks for the clarification Ugo. The question is if we still want to show the boot order in the summary even though it was not provided explicitly (for disks).
|
/retest |
galkremer1
force-pushed
the
CNV-69048-update-bootorder
branch
from
2d5c6ca to
e863a65
Compare
|
@galkremer1: This pull request references CNV-69048 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 bug 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. |
4 participants
📝 Description
Jira ticket: CNV-69048
bootOrderproperty.bootOrderproperty to all devices.🎥 Demo
Before:
Before.mov
After:
after.mov
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.