Resolves: MTV-4551 | Add virtual machines to existing plan by avivtur · Pull Request #2261 · kubev2v/forklift-console-plugin

avivtur · 2026-02-12T17:35:15Z

📝 Links

https://issues.redhat.com/browse/MTV-4551

📝 Description

expose a dialog in the virtual machines tab to add virtual machines to the plan

🎥 Demo

📝 CC://

codecov-commenter · 2026-02-12T17:37:02Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 99 lines in your changes missing coverage. Please review.
✅ Project coverage is 14.84%. Comparing base (13484d0) to head (1b9a69b).
⚠️ Report is 986 commits behind head on main.

Files with missing lines	Patch %	Lines
...ualMachines/components/AddVirtualMachinesTable.tsx	0.00%	48 Missing ⚠️
...nts/AddVirtualMachines/AddVirtualMachinesModal.tsx	0.00%	28 Missing ⚠️
...ts/AddVirtualMachines/AddVirtualMachinesButton.tsx	0.00%	17 Missing and 1 partial ⚠️
...eteVirtualMachines/DeleteVirtualMachinesButton.tsx	0.00%	2 Missing and 1 partial ⚠️
...chinesList/hooks/useSpecVirtualMachinesActions.tsx	0.00%	2 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2261       +/-   ##
===========================================
- Coverage   36.81%   14.84%   -21.97%     
===========================================
  Files         158     1135      +977     
  Lines        2548    20284    +17736     
  Branches      599     3997     +3398     
===========================================
+ Hits          938     3012     +2074     
- Misses       1428    16611    +15183     
- Partials      182      661      +479

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Pedro-S-Abreu · 2026-02-13T17:25:22Z

testing/playwright/e2e/downstream/plans/plan-add-virtual-machines.spec.ts

+
+    // Read the name of a VM that is currently in the plan for exclusion checks
+    const plannedVmName = await planDetailsPage.virtualMachinesTab.getFirstVMName();
+    expect(plannedVmName).toBeTruthy();


Make sure all test logic are within step blocks

Pedro-S-Abreu · 2026-02-13T17:27:20Z

testing/playwright/e2e/downstream/plans/plan-add-virtual-machines.spec.ts

+    });
+
+    await test.step('4. Select a VM and verify confirm button becomes enabled', async () => {
+      const modal = await planDetailsPage.virtualMachinesTab.clickAddVirtualMachines();


Test could possible be optimized by reducing number of times opening and closing of modal.

Pedro-S-Abreu · 2026-02-13T17:28:56Z

testing/playwright/e2e/downstream/plans/plan-add-virtual-machines.spec.ts

+      await planDetailsPage.virtualMachinesTab.navigateToVirtualMachinesTab();
+
+      // Wait for the status to reflect the archived state
+      await page.waitForTimeout(2000);


Is waitForTimeout avoidable here?

Pedro-S-Abreu · 2026-02-13T17:39:29Z

testing/playwright/page-objects/PlanDetailsPage/modals/AddVirtualMachinesModal.ts

Please consider reusing common/VirtualMachinesTable.ts or common/Table.ts page objects instead to avoid redefining common actions. getRowCount method could be moved to common/Table.ts since it's generic enough.

jpuzz0 · 2026-02-18T20:40:08Z

...ls/tabs/VirtualMachines/components/AddVirtualMachines/components/AddVirtualMachinesTable.tsx

+
+  const tableProps: ProviderVirtualMachinesListProps = useMemo(
+    () => ({
+      hasCriticalConcernFilter: true,


selectedIds is in the useMemo deps for tableProps, so every selection rebuilds the object and re-renders the entire VM list. I think initialSelectedIds should be set once (maybe via a ref) rather than updating on every selection change.

A ref cannot replace state here. The useEffect in usePageSelection (line 33-35) is the only mechanism that re-synchronizes internalSelectedIds when the component re-renders from external causes (inventory polling, K8s watch events). It fires only when the selectedIds prop reference changes. A ref never triggers that change, so selection silently resets on the next poll-driven re-render. The create-plan wizard's VirtualMachinesTable has the same pattern -- it uses react-hook-form's value (which is state) as initialSelectedIds and includes it in useMemo deps (line 83). One re-render per user click is the expected cost and matches the existing working pattern.

jpuzz0 · 2026-02-18T20:40:08Z

...plans/details/tabs/VirtualMachines/components/AddVirtualMachines/AddVirtualMachinesModal.tsx

+  const selectedVmsRef = useRef<VmData[]>([]);
+  const [hasSelection, setHasSelection] = useState(false);
+
+  const handleSelect = useCallback((vms: VmData[]): void => {


usePlanSourceProvider(plan) is called here and again in AddVirtualMachinesTable. That's two K8s watches for the same provider. Could the modal fetch it once and pass it down?

jpuzz0 · 2026-02-18T20:40:08Z

...lans/details/tabs/VirtualMachines/components/AddVirtualMachines/AddVirtualMachinesButton.tsx

+    launcher<AddVirtualMachineProps>(AddVirtualMachinesModal, { plan });
+  };
+
+  const reason = useMemo((): string | null => {


This uses isPlanEditable while the existing DeleteVirtualMachinesButton uses isPlanArchived. Add is more restrictive (disables during execution, succeeded, etc.) but delete only disables for archived. Is that inconsistency intentional, or should delete be updated too?

changed the delete button to also disabled if plan is not editable

jpuzz0 · 2026-02-18T20:40:08Z

...ls/tabs/VirtualMachines/components/AddVirtualMachines/components/AddVirtualMachinesTable.tsx

+    case PROVIDER_TYPES.ova:
+      return <OvaVirtualMachinesList {...tableProps} />;
+    case PROVIDER_TYPES.hyperv:
+      return <HypervVirtualMachinesList {...tableProps} />;


The default case returns <></> — if the provider type is still loading, the user sees an empty modal with no feedback. I think a loading indicator would be better here.

jpuzz0 · 2026-02-18T20:40:08Z

...ls/tabs/VirtualMachines/components/AddVirtualMachines/components/AddVirtualMachinesTable.tsx

+  const existingVmIds = useMemo((): Set<string> => {
+    const planVms = getPlanVirtualMachines(plan);
+    return new Set((planVms ?? []).map((vm) => vm.id).filter(Boolean) as string[]);
+  }, [plan]);


The filtering uses vm.id to exclude existing VMs. For OpenShift providers, is id guaranteed unique across namespaces from the inventory?

id is based on uid which should be globally unique

jpuzz0 · 2026-02-18T20:40:08Z

...plans/details/tabs/VirtualMachines/components/AddVirtualMachines/AddVirtualMachinesModal.tsx

+    const op = currentVms ? REPLACE : ADD;
+
+    const newVmEntries: V1beta1PlanSpecVms[] = selectedVmsRef.current.map((vmData) => ({
+      id: vmData.vm.id,


handleSave reads getPlanVirtualMachines(plan) from the plan prop. If someone else modifies the plan concurrently, this could add duplicate VMs. Is the plan object kept fresh via the K8s watch, or is this a stale closure risk?

k8s resources have resourceVersion in their metadata to avoid this, so there should be no issue on this

jpuzz0 · 2026-02-18T20:40:08Z

testing/playwright/e2e/downstream/plans/plan-add-virtual-machines.spec.ts

+      await page.waitForLoadState('networkidle');
+      await planDetailsPage.virtualMachinesTab.verifyTableLoaded();
+
+      const afterAddRowCount = await planDetailsPage.virtualMachinesTab.getRowCount();


networkidle can be flaky with K8s watches polling in the background. Would waiting for a specific element or API response be more reliable?

jpuzz0

1 remaining comment. Otherwise looks good!

jpuzz0 · 2026-02-19T19:48:54Z

...plans/details/tabs/VirtualMachines/components/AddVirtualMachines/AddVirtualMachinesModal.tsx

+
+  const handleSave = useCallback(async () => {
+    const currentVms = getPlanVirtualMachines(plan);
+    const op = currentVms ? REPLACE : ADD;


getPlanVirtualMachines(plan) returns plan?.spec?.vms ?? [], which is always truthy (empty array). So op will always be REPLACE and the ADD branch is dead code. Should this just use REPLACE directly, or should the check be !isEmpty(currentVms) if the ADD path is actually needed?

there's always at least one vm in the plan so it should always be REPLACE, changed

Signed-off-by: Aviv Turgeman <aturgema@redhat.com>

sonarqubecloud · 2026-02-19T20:13:01Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
4.0% Duplication on New Code

See analysis details on SonarQube Cloud

Pedro-S-Abreu reviewed Feb 13, 2026

View reviewed changes

avivtur force-pushed the feature/MTV-4551-allow-add-vms-to-plan branch 3 times, most recently from 65c685d to b342c5c Compare February 18, 2026 15:21

jpuzz0 reviewed Feb 18, 2026

View reviewed changes

avivtur force-pushed the feature/MTV-4551-allow-add-vms-to-plan branch 5 times, most recently from 9b3f1b6 to 99194ad Compare February 19, 2026 17:42

jpuzz0 approved these changes Feb 19, 2026

View reviewed changes

avivtur added 2 commits February 19, 2026 22:08

Resolves: MTV-4551 | Add virtual machines to existing plan

dca4c6d

Signed-off-by: Aviv Turgeman <aturgema@redhat.com>

Resolves: MTV-4551 | fix jpuzz0 comments

1b9a69b

Signed-off-by: Aviv Turgeman <aturgema@redhat.com>

avivtur force-pushed the feature/MTV-4551-allow-add-vms-to-plan branch from 99194ad to 1b9a69b Compare February 19, 2026 20:12

Comments

Conversation

avivtur commented Feb 12, 2026

📝 Links

📝 Description

🎥 Demo

📝 CC://

Uh oh!

codecov-commenter commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pedro-S-Abreu Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 19, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Feb 12, 2026 •

edited

Loading

Pedro-S-Abreu Feb 13, 2026 •

edited

Loading