Resolves: MTV-6247 | Add resume conversion action and resumable status indicator for failed migration plans - #2611
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds resumable-conversion eligibility checks, Resume actions in plan interfaces, a confirmation modal that creates a resume Migration for VMs with copied disks, tests, and locale keys. ChangesResume conversion workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlanStatus
participant PlanActionsDropdownItems
participant PlanResumeConversionModal
participant k8sCreate
PlanStatus->>PlanActionsDropdownItems: expose resume conversion action
PlanActionsDropdownItems->>PlanResumeConversionModal: open for selected plan
PlanResumeConversionModal->>k8sCreate: create Migration with resumeConversion
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Hazanel
changed the title
Hazanel
changed the title
![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
🧹 Nitpick comments (4)
src/plans/actions/PlanActionsDropdownItems.tsx (1)
124-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
testIdfor the action selector.As per coding guidelines, interactive elements must use the
testIdprop rather than alternate selector attributes.🤖 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/plans/actions/PlanActionsDropdownItems.tsx` at line 124, Update the interactive menu item in the plan actions dropdown to use the `testId` prop instead of the `data-testid` attribute, preserving the existing selector value `plan-actions-resume-conversion-menuitem`.Source: Coding guidelines
src/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx (1)
103-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared label-status constant.
Replace
color="blue"with the applicablePF_LABEL_STATUSvalue for this informational badge. As per coding guidelines, “Use PF_LABEL_STATUS constants instead of string literals for PatternFly status props.”🤖 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/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx` around lines 103 - 109, Update the informational Label in the canPlanResumeConversion(plan) rendering block to use the applicable PF_LABEL_STATUS constant instead of the literal color="blue" value, preserving the existing label text, icon, and layout.Source: Coding guidelines
src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep resource hooks before local state.
Move
usePlanMigration(plan)aboveuseState()to preserve the required component hook order: data fetching hooks before local state.🤖 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/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx` at line 42, Move the usePlanMigration(plan) call in the MigrationStatusVirtualMachinesList component so it appears before all useState() calls, keeping resource/data hooks ahead of local state while preserving the existing activeMigration usage.Source: Coding guidelines
src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx (1)
61-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse PatternFly enums for both informational alerts.
src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx#L61-L61: replace"info"with the Alert variant enum.src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx#L77-L77: replace"info"with the Alert variant enum.🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` at line 61, Replace the string "info" with the appropriate PatternFly Alert variant enum in the informational Alert within PlanResumeConversionModal in src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx:61-61 and the corresponding Alert in MigrationStatusVirtualMachinesList in src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx:77-77, adding or reusing the required enum imports.Source: Coding guidelines
🤖 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/plans/actions/PlanActionsDropdownItems.tsx`:
- Around line 118-122: Update the resume-conversion DropdownItem’s isDisabled
condition to also require migrationLoaded, keeping the action disabled while
usePlanMigration() is still loading. Preserve the existing canResume and
activeMigration checks once migration state has loaded.
---
Nitpick comments:
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Line 61: Replace the string "info" with the appropriate PatternFly Alert
variant enum in the informational Alert within PlanResumeConversionModal in
src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx:61-61
and the corresponding Alert in MigrationStatusVirtualMachinesList in
src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx:77-77,
adding or reusing the required enum imports.
In `@src/plans/actions/PlanActionsDropdownItems.tsx`:
- Line 124: Update the interactive menu item in the plan actions dropdown to use
the `testId` prop instead of the `data-testid` attribute, preserving the
existing selector value `plan-actions-resume-conversion-menuitem`.
In
`@src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx`:
- Line 42: Move the usePlanMigration(plan) call in the
MigrationStatusVirtualMachinesList component so it appears before all useState()
calls, keeping resource/data hooks ahead of local state while preserving the
existing activeMigration usage.
In `@src/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx`:
- Around line 103-109: Update the informational Label in the
canPlanResumeConversion(plan) rendering block to use the applicable
PF_LABEL_STATUS constant instead of the literal color="blue" value, preserving
the existing label text, icon, and layout.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82d1d2b9-5321-41f5-9b47-e81bf5b1cf62
📒 Files selected for processing (11)
locales/en/plugin__forklift-console-plugin.jsonlocales/es/plugin__forklift-console-plugin.jsonlocales/fr/plugin__forklift-console-plugin.jsonlocales/ja/plugin__forklift-console-plugin.jsonlocales/ko/plugin__forklift-console-plugin.jsonlocales/zh/plugin__forklift-console-plugin.jsonsrc/plans/actions/PlanActionsDropdownItems.tsxsrc/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsxsrc/plans/details/components/PlanStatus/utils/utils.tssrc/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsxsrc/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx
Hazanel
force-pushed
the
resume-conversion-ui
branch
4 times, most recently
from
July 27, 2026 07:41
6dc10cd to
e1f2b51
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2611 +/- ##
===========================================
- Coverage 36.81% 20.30% -16.51%
===========================================
Files 158 1298 +1140
Lines 2548 24555 +22007
Branches 599 5079 +4480
===========================================
+ Hits 938 4987 +4049
- Misses 1428 19555 +18127
+ Partials 182 13 -169 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
avivtur
changed the title
avivtur
reviewed
Jul 27, 2026
avivtur
left a comment
avivtur
left a comment
Member
There was a problem hiding this comment.
Hi @Hazanel, thanks for this PR!
MTV-6204 | Add resume conversion action and resumable status indicator for failed migration plans
Clean feature addition — the modal, status badge, and info alert all follow established project patterns.
- [Developer] The i18n linter CI check is failing because the locale files don't match what
npm run i18nproduces. Runnpm run i18nand commit the updated locale files to fix it. The<ForkliftTrans>warnings in the CI logs are pre-existing across the codebase. - [Architect] Two new fields used via
ascasts —V1beta1MigrationSpec.resumeConversionandV1beta1PlanStatusMigrationVms.disksCopied— are not yet in@forklift-ui/types. Worth filing a follow-up types-update issue so the casts can be removed once the upstream CRD schema includes them.
No blocking issues — feel free to address these in this PR or in a follow-up.
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 09:37
e1f2b51 to
7b21a87
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAccess migration VMs through a Plan selector.
This component directly traverses
plan.status. Move this into an existing or dedicated Plan selector, then consume that selector here.🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` around lines 25 - 28, Move the migration VM filtering logic from PlanResumeConversionModal into an existing or dedicated Plan selector that derives resumable VMs from the plan state, preserving the disksCopied handling and empty-array fallback. Update the component to consume that selector instead of directly traversing plan.status.migration.vms.Source: Coding guidelines
🤖 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 `@locales/en/plugin__forklift-console-plugin.json`:
- Line 1162: Fix the plural confirmation strings: in
locales/en/plugin__forklift-console-plugin.json at lines 1162-1162, change the
_other variant’s “{{count}} VM” to “{{count}} VMs”; in
locales/es/plugin__forklift-console-plugin.json at lines 1172-1172, replace the
literal _many key-like value with the proper localized sentence or intended
fallback value.
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Around line 32-46: Add tests for the ResumeConversionModal confirmation flow,
covering both comparison and execution behavior. Verify that confirming creates
a Migration with the expected plan reference, owner reference, and
resumeConversion set to true, using the existing k8sCreate interaction and modal
test patterns.
---
Nitpick comments:
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Around line 25-28: Move the migration VM filtering logic from
PlanResumeConversionModal into an existing or dedicated Plan selector that
derives resumable VMs from the plan state, preserving the disksCopied handling
and empty-array fallback. Update the component to consume that selector instead
of directly traversing plan.status.migration.vms.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 47a76c7d-63a2-422e-b5ea-65ed6a42bcfb
📒 Files selected for processing (8)
locales/en/plugin__forklift-console-plugin.jsonlocales/es/plugin__forklift-console-plugin.jsonlocales/fr/plugin__forklift-console-plugin.jsonlocales/ja/plugin__forklift-console-plugin.jsonlocales/ko/plugin__forklift-console-plugin.jsonlocales/zh/plugin__forklift-console-plugin.jsonsrc/plans/actions/PlanActionsDropdownItems.tsxsrc/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- locales/fr/plugin__forklift-console-plugin.json
- locales/ja/plugin__forklift-console-plugin.json
- src/plans/actions/PlanActionsDropdownItems.tsx
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 10:20
7b21a87 to
65e6db5
Compare
avivtur
reviewed
Jul 27, 2026
Comment on lines
+58
to
+62
| <Alert variant={AlertVariant.info} isInline title={t('Disk copy will be skipped')}> | ||
| {t( | ||
| 'This will re-run only the guest conversion step, reusing the disks that were already copied. The source VM must not have been powered on since the original migration.', | ||
| )} | ||
| </Alert> |
Member
There was a problem hiding this comment.
this alert is not matching the pattern we have for info alerts inside modal dialogs, please look at this:
avivtur
reviewed
Jul 27, 2026
| []; | ||
|
|
||
| const onConfirm = useCallback(async () => { | ||
| const { name: ownerName, namespace: _ns, uid: ownerUid, ...ownerRest } = getObjectRef(plan); |
Member
There was a problem hiding this comment.
this could be done instead of lines 21-23, you extract the same data twice and there's no reason for it.
avivtur
reviewed
Jul 27, 2026
Comment on lines
+124
to
+130
| {canPlanResumeConversion(plan) && (hasActiveMigration || !loaded) && ( | ||
| <FlexItem> | ||
| <Label status={PF_LABEL_STATUS.INFO} icon={<InfoCircleIcon />} isCompact> | ||
| {t('Migrated disks available')} | ||
| </Label> | ||
| </FlexItem> | ||
| )} |
avivtur
reviewed
Jul 27, 2026
| )} | ||
| </Alert> | ||
| </StackItem> | ||
| )} |
Member
There was a problem hiding this comment.
this warning adds only visual noise. the action's description is enough, please remove this alert and remove the stack layout
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 11:29
65e6db5 to
2522bf8
Compare
avivtur
reviewed
Jul 27, 2026
| {canPlanResumeConversion(plan) && loaded && !hasActiveMigration && ( | ||
| <FlexItem> | ||
| <Button | ||
| variant={ButtonVariant.secondary} |
Member
There was a problem hiding this comment.
try changing this variant to link instead, the secondary variant looks a bit weird next to the label
Hazanel
force-pushed
the
resume-conversion-ui
branch
2 times, most recently
from
July 27, 2026 11:39
8b43537 to
8d2fc6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx (1)
109-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the resume-status branches.
Codecov reports this changed plan-list status path as uncovered. Add cases for eligible/loaded plans opening Resume, active or loading plans showing the badge, and ineligible plans showing neither.
🤖 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/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx` around lines 109 - 130, Add tests covering the PlanStatus resume-status branches: verify an eligible loaded plan renders the Resume button and opens PlanResumeConversionModal on click, an eligible plan with an active migration or unfinished loading renders the “Migrated disks available” badge, and an ineligible plan renders neither control.
🤖 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/plans/actions/components/ResumeConversionModal/__tests__/PlanResumeConversionModal.test.tsx`:
- Around line 11-19: Replace the local mockT implementations in
PlanResumeConversionModal tests with the utilities from `@test-utils/mockI18n`,
ensuring interpolation parameters are applied. Update the one- and two-VM
assertions to verify the rendered resume messages include “test-plan” and the
actual VM counts, rather than translation keys.
- Line 8: Update the mock callback definitions in PlanResumeConversionModal
tests so the k8sCreate rest parameter and the related obj callback parameter are
explicitly typed, reusing the imported k8sCreate/getObjectRef overload
signatures where appropriate; preserve the existing mock delegation behavior.
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Around line 22-25: Update the resumableVMs calculation in
PlanResumeConversionModal to use getPlanVirtualMachinesMigrationStatus(plan)
instead of traversing plan.status.migration.vms directly, and filter only VMs
whose disksCopied value is exactly true. Remove the obsolete type-cast
workaround if the selector provides the appropriate VM type.
- Around line 60-62: Update the confirmation message in
PlanResumeConversionModal to explicitly state that the source VM must not have
been powered on since the original migration, while retaining the existing
requirement that it remain powered off. Ensure the wording clearly disqualifies
reuse of copied disks if the source VM was powered on afterward.
---
Nitpick comments:
In `@src/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx`:
- Around line 109-130: Add tests covering the PlanStatus resume-status branches:
verify an eligible loaded plan renders the Resume button and opens
PlanResumeConversionModal on click, an eligible plan with an active migration or
unfinished loading renders the “Migrated disks available” badge, and an
ineligible plan renders neither control.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 81852412-66f0-493a-ab6f-b9dd21484d21
📒 Files selected for processing (13)
locales/en/plugin__forklift-console-plugin.jsonlocales/es/plugin__forklift-console-plugin.jsonlocales/fr/plugin__forklift-console-plugin.jsonlocales/ja/plugin__forklift-console-plugin.jsonlocales/ko/plugin__forklift-console-plugin.jsonlocales/zh/plugin__forklift-console-plugin.jsonsrc/plans/actions/PlanActionsDropdownItems.tsxsrc/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsxsrc/plans/actions/components/ResumeConversionModal/__tests__/PlanResumeConversionModal.test.tsxsrc/plans/details/components/PlanStatus/utils/constants.tssrc/plans/details/components/PlanStatus/utils/utils.tssrc/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsxsrc/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/plans/details/components/PlanStatus/utils/utils.ts
- locales/ja/plugin__forklift-console-plugin.json
- src/plans/actions/PlanActionsDropdownItems.tsx
- locales/en/plugin__forklift-console-plugin.json
- locales/zh/plugin__forklift-console-plugin.json
- src/plans/details/tabs/VirtualMachines/components/MigrationStatusVirtualMachineList/MigrationStatusVirtualMachinesList.tsx
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 11:47
8d2fc6b to
27e58c4
Compare
Contributor
Author
|
avivtur
reviewed
Jul 27, 2026
Member
|
non english translation keys are missing, please run npm run i18n to fix the translations |
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 14:27
27e58c4 to
3b780c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx (2)
36-38: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse the shared Migration GVK constant.
The resource currently hardcodes
apiVersionandkind. Build these fields from the repository’s Migration GVK constant/helper so the created resource remains aligned withMigrationModel.🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` around lines 36 - 38, Update the migration object in PlanResumeConversionModal to derive apiVersion and kind from the repository’s shared Migration GVK constant or helper, matching MigrationModel, instead of hardcoding either field; preserve the existing resource construction and behavior.Source: Coding guidelines
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve or track the temporary type-gap workarounds.
The
disksCopiedassertion bypasses runtime validation, and theresumeConversionTODO has no corresponding cast. Update the upstream types/selectors when available; otherwise use a type guard and link these TODOs to a tracked issue. SonarCloud currently flags both TODOs.Also applies to: 46-47
🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` around lines 30 - 32, Address the temporary type-gap workaround in PlanResumeConversionModal by replacing the inline disksCopied cast with a type guard that safely validates the property at runtime, and update the related resumeConversion TODO to reference a tracked issue. When upstream `@forklift-ui/types` and selectors expose disksCopied, remove the workaround and TODOs and use the typed field directly.Sources: Coding guidelines, Linters/SAST tools
🤖 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/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx`:
- Around line 102-109: Update the Resume button in PlanStatus to use the
repository-standard testId prop instead of data-testid, preserving the existing
selector value and button behavior.
- Around line 100-114: Add tests covering the Resume action in PlanStatus:
verify the button renders only when canPlanResumeConversion(plan), loaded, and
!hasActiveMigration are all true, remains hidden when any gate fails, and
invokes launcher with PlanResumeConversionModal and the current plan when
clicked.
---
Nitpick comments:
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Around line 36-38: Update the migration object in PlanResumeConversionModal to
derive apiVersion and kind from the repository’s shared Migration GVK constant
or helper, matching MigrationModel, instead of hardcoding either field; preserve
the existing resource construction and behavior.
- Around line 30-32: Address the temporary type-gap workaround in
PlanResumeConversionModal by replacing the inline disksCopied cast with a type
guard that safely validates the property at runtime, and update the related
resumeConversion TODO to reference a tracked issue. When upstream
`@forklift-ui/types` and selectors expose disksCopied, remove the workaround and
TODOs and use the typed field directly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 222402d8-2f50-4beb-b619-9509c97f7cb8
📒 Files selected for processing (12)
locales/en/plugin__forklift-console-plugin.jsonlocales/es/plugin__forklift-console-plugin.jsonlocales/fr/plugin__forklift-console-plugin.jsonlocales/ja/plugin__forklift-console-plugin.jsonlocales/ko/plugin__forklift-console-plugin.jsonlocales/zh/plugin__forklift-console-plugin.jsonsrc/plans/actions/PlanActionsDropdownItems.tsxsrc/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsxsrc/plans/actions/components/ResumeConversionModal/__tests__/PlanResumeConversionModal.test.tsxsrc/plans/details/components/PlanStatus/utils/constants.tssrc/plans/details/components/PlanStatus/utils/utils.tssrc/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plans/details/components/PlanStatus/utils/utils.ts
- src/plans/details/components/PlanStatus/utils/constants.ts
- locales/en/plugin__forklift-console-plugin.json
- src/plans/actions/components/ResumeConversionModal/tests/PlanResumeConversionModal.test.tsx
- src/plans/actions/PlanActionsDropdownItems.tsx
Hazanel
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 14:35
3b780c1 to
30cc567
Compare
avivtur
force-pushed
the
resume-conversion-ui
branch
from
July 27, 2026 15:31
30cc567 to
2b25ad6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
locales/en/plugin__forklift-console-plugin.json (1)
1161-1161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the plural noun in the
_othertranslation.Line [1161] renders
2 VMinstead of2 VMs. Update the generated English source/locale entry and rerunnpm run i18n; keep the singular form only in_one. This duplicates the unresolved issue from the previous review.🤖 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 `@locales/en/plugin__forklift-console-plugin.json` at line 1161, Update the `_other` English translation for the resume-conversion message to use the plural noun “VMs,” keeping “VM” only in the corresponding `_one` entry, then rerun `npm run i18n` to regenerate the locale output.Source: Path instructions
🧹 Nitpick comments (4)
src/plans/list/components/PlanRowFields/PlanStatus/__tests__/PlanStatusResumeButton.test.tsx (2)
76-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the real resumability predicate.
Every case mocks
canPlanResumeConversion, so theConversionResumablecontract is untested. Add focused utility tests for true, false, and missing conditions; Codecov identifiesPlanStatus/utils/utils.tsas uncovered in this PR.🤖 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/plans/list/components/PlanRowFields/PlanStatus/__tests__/PlanStatusResumeButton.test.tsx` around lines 76 - 123, Add focused tests for the ConversionResumable predicate in PlanStatus/utils/utils.ts, covering true, false, and missing-condition inputs without mocking canPlanResumeConversion. Preserve the existing PlanStatus component tests, and assert the utility’s behavior directly for each relevant resumability condition.Sources: Coding guidelines, Path instructions
120-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the specific modal component.
expect.any(Function)passes if the click launches any modal, including the wrong action. MockPlanResumeConversionModalas a sentinel and assert that exact component was passed tomockLauncher.🤖 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/plans/list/components/PlanRowFields/PlanStatus/__tests__/PlanStatusResumeButton.test.tsx` around lines 120 - 121, Update the PlanStatusResumeButton test to mock PlanResumeConversionModal with a sentinel component and assert mockLauncher was called with that exact sentinel and { plan }, replacing the broad expect.any(Function) assertion while preserving the existing call-count check.src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx (2)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the stale TODO.
This TODO says to remove a cast, but
resumeConversionis not cast here. It creates a Sonar warning and misdirects future maintenance.🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` at line 46, Remove the stale TODO comment near the ResumeConversionModal implementation; it no longer describes the surrounding code and should be deleted without changing the resumeConversion behavior.Source: Linters/SAST tools
1-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder external imports before path aliases.
src/utils/i18nand@components/...imports precede external packages. Move all package imports first, then aliases. As per coding guidelines, “Sort imports in this order: Node built-ins, React and external packages, path aliases...”.🤖 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/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx` around lines 1 - 17, Reorder imports in PlanResumeConversionModal so React and all external package imports come before path-alias imports such as useForkliftTranslation and ModalForm, while preserving the existing imported symbols and grouping.Source: Coding guidelines
🤖 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 `@locales/en/plugin__forklift-console-plugin.json`:
- Line 1161: Update the `_other` English translation for the resume-conversion
message to use the plural noun “VMs,” keeping “VM” only in the corresponding
`_one` entry, then rerun `npm run i18n` to regenerate the locale output.
---
Nitpick comments:
In
`@src/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsx`:
- Line 46: Remove the stale TODO comment near the ResumeConversionModal
implementation; it no longer describes the surrounding code and should be
deleted without changing the resumeConversion behavior.
- Around line 1-17: Reorder imports in PlanResumeConversionModal so React and
all external package imports come before path-alias imports such as
useForkliftTranslation and ModalForm, while preserving the existing imported
symbols and grouping.
In
`@src/plans/list/components/PlanRowFields/PlanStatus/__tests__/PlanStatusResumeButton.test.tsx`:
- Around line 76-123: Add focused tests for the ConversionResumable predicate in
PlanStatus/utils/utils.ts, covering true, false, and missing-condition inputs
without mocking canPlanResumeConversion. Preserve the existing PlanStatus
component tests, and assert the utility’s behavior directly for each relevant
resumability condition.
- Around line 120-121: Update the PlanStatusResumeButton test to mock
PlanResumeConversionModal with a sentinel component and assert mockLauncher was
called with that exact sentinel and { plan }, replacing the broad
expect.any(Function) assertion while preserving the existing call-count check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07d6a786-680d-45cc-92b2-181f7c355bda
📒 Files selected for processing (13)
locales/en/plugin__forklift-console-plugin.jsonlocales/es/plugin__forklift-console-plugin.jsonlocales/fr/plugin__forklift-console-plugin.jsonlocales/ja/plugin__forklift-console-plugin.jsonlocales/ko/plugin__forklift-console-plugin.jsonlocales/zh/plugin__forklift-console-plugin.jsonsrc/plans/actions/PlanActionsDropdownItems.tsxsrc/plans/actions/components/ResumeConversionModal/PlanResumeConversionModal.tsxsrc/plans/actions/components/ResumeConversionModal/__tests__/PlanResumeConversionModal.test.tsxsrc/plans/details/components/PlanStatus/utils/constants.tssrc/plans/details/components/PlanStatus/utils/utils.tssrc/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsxsrc/plans/list/components/PlanRowFields/PlanStatus/__tests__/PlanStatusResumeButton.test.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- src/plans/list/components/PlanRowFields/PlanStatus/PlanStatus.tsx
- src/plans/details/components/PlanStatus/utils/utils.ts
- locales/ja/plugin__forklift-console-plugin.json
- src/plans/details/components/PlanStatus/utils/constants.ts
- locales/fr/plugin__forklift-console-plugin.json
- locales/ko/plugin__forklift-console-plugin.json
- locales/es/plugin__forklift-console-plugin.json
- src/plans/actions/PlanActionsDropdownItems.tsx
- src/plans/actions/components/ResumeConversionModal/tests/PlanResumeConversionModal.test.tsx
- locales/zh/plugin__forklift-console-plugin.json
… disks When a migration fails after disk copy completes but before guest conversion succeeds, users can now resume just the conversion step without re-copying disks. - Add "Resume conversion" dropdown action on plans with the ConversionResumable condition - Show "Migrated disks available" badge on failed resumable plans - Display info alert during resume-conversion execution - Create Migration CR with resumeConversion: true on confirm Resolves: MTV-5812 Signed-off-by: Elad Haan <ehazan@redhat.com>
avivtur
force-pushed
the
resume-conversion-ui
branch
from
July 29, 2026 11:19
2b25ad6 to
394918c
Compare
|
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
3 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.
Summary
ConversionResumablecondition (set when disk copy completed but virt-v2v conversion failed)MigrationCR withspec.resumeConversion: trueon confirm, which instructs the controller to skip disk copy and run only the guest conversion stepDepends On
Screenshots
Test Plan
ConversionResumableconditionresumeConversion: trueSummary by CodeRabbit