CNV-74280: migrate Template and Wizard disk/network tables to KubevirtTable#3586
Open
batyana wants to merge 1 commit intokubevirt-ui:mainfrom
Open
CNV-74280: migrate Template and Wizard disk/network tables to KubevirtTable#3586batyana wants to merge 1 commit intokubevirt-ui:mainfrom
batyana wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
Collaborator
|
@batyana: This pull request references CNV-74280 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. |
Contributor
|
[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 |
📝 WalkthroughWalkthroughAdded locale keys; introduced POD_NETWORK_SORT_KEY and VM network selector helpers; replaced row-based disk/network tables with column-driven KubevirtTable definitions for wizard and template pages; removed multiple Row components and useDiskColumns hooks; added reusable cell components and exported table-definition helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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)
📝 Coding Plan
Comment |
Collaborator
|
@batyana: This pull request references CNV-74280 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: 5
🧹 Nitpick comments (1)
src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx (1)
82-88: State column:getValuemay not match display logic for sorting.The
getValuefunction returnsrow.iface?.state(Line 83), butStateCellusesgetConfigInterfaceStateFromVM(callbacks.vm, row.network?.name)to determine the displayed state (Line 42). If these values differ, sorting by state won't match what users see.Consider aligning the sorting logic with the display logic for consistent UX.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx` around lines 82 - 88, The State column's getValue currently returns row.iface?.state but the cell display uses getConfigInterfaceStateFromVM(callbacks.vm, row.network?.name) inside StateCell, causing inconsistent sorting; update the column's getValue to compute and return the same value used for display by calling getConfigInterfaceStateFromVM with the same inputs (callbacks.vm and row.network?.name) or by delegating to the same helper StateCell logic so sorting reflects displayed state (keep getValue, key 'state', label 'State', and sortable: true).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/resources/vm/utils/network/constants.ts`:
- Line 50: POD_NETWORK_SORT_KEY is currently a user-facing English label; change
it to a locale-neutral token (e.g., 'podNetwork' or 'pod_network') and update
any consumers to use that token for sorting, then derive the visible label via
translation (useKubevirtTranslation hook and t('Pod networking')) when rendering
table headers/cells; ensure the constant name POD_NETWORK_SORT_KEY stays but its
value becomes the stable token and update references to map token->t('Pod
networking') in the table layer.
In `@src/utils/resources/vm/utils/network/selectors.ts`:
- Around line 81-84: The function isSRIOVNetworkByVM uses
getNetworkInterface(vm, nicName) which can return undefined but then calls
getNetworkInterfaceType(iface) violating its expected V1Interface parameter; fix
by adding an explicit null/undefined check on the local variable (iface)
returned from getNetworkInterface inside isSRIOVNetworkByVM and return false
early if it's missing, otherwise call getNetworkInterfaceType(iface) and compare
via interfaceTypesProxy to interfaceTypesProxy.sriov.
In `@src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx`:
- Around line 85-90: The Size column's getValue currently returns the raw
display string causing lexicographic sorting; update getValue in
wizardDisksTableDefinition to parse the size string (e.g., values like "10Gi",
"500Mi", "1Ti") into a normalized numeric value in a single base unit (bytes)
and return that number (or null/0 when absent) so KubevirtTable sorts
numerically; keep renderCell using readableSizeUnit and NO_DATA_DASH for
display, but ensure getValue returns the parsed numeric value rather than the
formatted string.
In `@src/views/templates/details/tabs/disks/utils.ts`:
- Around line 7-17: getTemplateVMWithNamespace currently assumes
getTemplateVirtualMachineObject returned a full VM and writes
draftVM.metadata.namespace, which can throw if the returned object is a truthy
shell without metadata; update the guard to verify the VM shape (e.g., ensure vm
&& vm.metadata is an object) and in the produce callback initialize metadata
before writing: set draftVM.metadata = draftVM.metadata || {} (or return
undefined if the object isn't a real VM). Reference getTemplateVMWithNamespace,
getTemplateVirtualMachineObject, produce, and draftVM.metadata.namespace when
making the change.
In
`@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx`:
- Around line 89-94: getValue currently returns row.iface?.state which differs
from the StateCell render path that uses getConfigInterfaceStateFromVM(...);
update getValue to compute and return the same effective state as
getConfigInterfaceStateFromVM(...) (i.e., call getConfigInterfaceStateFromVM
with the same inputs used by StateCell) so sorting reflects the rendered state;
ensure you keep the fallback to an empty string if the computed state is
undefined.
---
Nitpick comments:
In
`@src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx`:
- Around line 82-88: The State column's getValue currently returns
row.iface?.state but the cell display uses
getConfigInterfaceStateFromVM(callbacks.vm, row.network?.name) inside StateCell,
causing inconsistent sorting; update the column's getValue to compute and return
the same value used for display by calling getConfigInterfaceStateFromVM with
the same inputs (callbacks.vm and row.network?.name) or by delegating to the
same helper StateCell logic so sorting reflects displayed state (keep getValue,
key 'state', label 'State', and sortable: true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1584693-01e6-4efa-a4cd-2eeda043babd
📒 Files selected for processing (26)
locales/en/plugin__kubevirt-plugin.jsonlocales/es/plugin__kubevirt-plugin.jsonlocales/fr/plugin__kubevirt-plugin.jsonlocales/ja/plugin__kubevirt-plugin.jsonlocales/ko/plugin__kubevirt-plugin.jsonlocales/zh/plugin__kubevirt-plugin.jsonsrc/utils/resources/vm/utils/network/constants.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/disks/WizardDisksTab.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskRow.tsxsrc/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.tssrc/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsxsrc/views/templates/details/tabs/disks/TemplateDisksPage.tsxsrc/views/templates/details/tabs/disks/components/DiskRow.tsxsrc/views/templates/details/tabs/disks/hooks/useDiskColumns.tssrc/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.tssrc/views/templates/details/tabs/disks/templateDisksTableDefinition.tsxsrc/views/templates/details/tabs/disks/utils.tssrc/views/templates/details/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsxsrc/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts
💤 Files with no reviewable changes (6)
- src/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.ts
- src/views/templates/details/tabs/disks/components/DiskRow.tsx
- src/views/templates/details/tabs/disks/hooks/useDiskColumns.ts
- src/views/catalog/wizard/tabs/disks/components/DiskRow.tsx
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsx
Comment on lines
+85
to
+90
| { | ||
| getValue: (row) => row?.size ?? '', | ||
| key: 'size', | ||
| label: t('Size'), | ||
| renderCell: (row) => <>{readableSizeUnit(row?.size) ?? NO_DATA_DASH}</>, | ||
| sortable: true, |
There was a problem hiding this comment.
Sort the Size column by a normalized numeric value.
Line 86 currently returns the raw size string, so "10Gi" will sort before "2Gi", and mixed units like "500Mi" vs "1Gi" will also order incorrectly. Since this column is now sortable in KubevirtTable, getValue should convert the size to a single numeric unit instead of using the display string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx` around
lines 85 - 90, The Size column's getValue currently returns the raw display
string causing lexicographic sorting; update getValue in
wizardDisksTableDefinition to parse the size string (e.g., values like "10Gi",
"500Mi", "1Ti") into a normalized numeric value in a single base unit (bytes)
and return that number (or null/0 when absent) so KubevirtTable sorts
numerically; keep renderCell using readableSizeUnit and NO_DATA_DASH for
display, but ensure getValue returns the parsed numeric value rather than the
formatted string.
Comment on lines
+89
to
+94
| { | ||
| getValue: (row) => row.iface?.state ?? '', | ||
| key: 'state', | ||
| label: t('State'), | ||
| renderCell: (row, callbacks) => <StateCell callbacks={callbacks} row={row} />, | ||
| sortable: true, |
There was a problem hiding this comment.
Keep state sorting aligned with the rendered state.
Line 90 sorts on row.iface?.state, but Line 93 renders the effective state from getConfigInterfaceStateFromVM(...). For interfaces with no explicit state, the table will display the default configured state while sorting them as an empty value. Please normalize getValue to the same effective state so the State column sorts what it actually shows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx`
around lines 89 - 94, getValue currently returns row.iface?.state which differs
from the StateCell render path that uses getConfigInterfaceStateFromVM(...);
update getValue to compute and return the same effective state as
getConfigInterfaceStateFromVM(...) (i.e., call getConfigInterfaceStateFromVM
with the same inputs used by StateCell) so sorting reflects the rendered state;
ensure you keep the fallback to an empty string if the computed state is
undefined.
upalatucci
reviewed
Mar 11, 2026
src/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts
Outdated
Show resolved
Hide resolved
galkremer1
reviewed
Mar 11, 2026
src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx
Show resolved
Hide resolved
src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx
Outdated
Show resolved
Hide resolved
src/views/templates/details/tabs/disks/templateDisksTableDefinition.tsx
Outdated
Show resolved
Hide resolved
| row: NetworkPresentation; | ||
| }; | ||
|
|
||
| const NetworkCell: FC<NetworkCellProps> = ({ row }) => { |
Member
There was a problem hiding this comment.
@upalatucci - WDYT, should we keep these small components within this file?
Member
Author
There was a problem hiding this comment.
Moved to a separate file - NetworkCell and StateCell are now in WizardNetworkCells.tsx. Same pattern applied for templates with TemplateNetworkCells.tsx.
batyana
force-pushed
the
CNV-74280-pr8-template-wizard-tables
branch
from
d93e71c to
1ab8826
Compare
Collaborator
|
@batyana: An error was encountered searching for bug CNV-74280 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/CNV-74280": GET https://issues.redhat.com/rest/api/2/issue/CNV-74280 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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.
♻️ Duplicate comments (1)
src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx (1)
89-94:⚠️ Potential issue | 🟡 MinorSorting and rendering mismatch for State column persists.
The
getValuefunction returnsrow.iface?.state ?? ''(line 90), whileStateCellrenders the state computed viagetConfigInterfaceStateFromVM(templateVM, row.network?.name)(line 35). These may produce different values when the interface has no explicitstateproperty but defaults are computed. The table will sort on one value while displaying another.To fix, align
getValuewith the computed state:♻️ Proposed fix
{ - getValue: (row) => row.iface?.state ?? '', + getValue: (row, callbacks) => { + const templateVM = getTemplateVirtualMachineObject(callbacks.template); + return getConfigInterfaceStateFromVM(templateVM, row.network?.name) ?? ''; + }, key: 'state', label: t('State'), renderCell: (row, callbacks) => <StateCell callbacks={callbacks} row={row} />, sortable: true, },Note: This requires updating the
ColumnConfigtype'sgetValuesignature to accept callbacks, or computing the effective state inline. IfgetValuedoesn't support callbacks, consider pre-computing the state in the data layer.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx` around lines 89 - 94, The State column sorts on row.iface?.state but renders a computed value via StateCell which uses getConfigInterfaceStateFromVM(templateVM, row.network?.name), causing mismatched sort/display; fix by aligning getValue with the computed state: either change the ColumnConfig.getValue implementation for the 'state' column to compute the effective state the same way StateCell does (call getConfigInterfaceStateFromVM(templateVM, row.network?.name) or an equivalent helper) or, if getValue cannot access callbacks/templateVM, pre-compute and attach an effectiveState property to each row in the data layer and have both getValue and StateCell read that property; update usages of getValue/StateCell to reference the single source (effective state) so sorting matches rendering.
🧹 Nitpick comments (5)
locales/zh/plugin__kubevirt-plugin.json (1)
1275-1275: Consider localizing the newly added zh entries instead of keeping English fallbacks.These keys are valid, but they will render English text in the Chinese locale and create mixed-language UX.
🌐 Suggested zh translations
- "No disks found": "No disks found", + "No disks found": "未找到磁盘", - "Template disks table": "Template disks table", + "Template disks table": "模板磁盘表", - "Template network interfaces table": "Template network interfaces table", + "Template network interfaces table": "模板网络接口表", - "Wizard disks table": "Wizard disks table", + "Wizard disks table": "向导磁盘表", - "Wizard network interfaces table": "Wizard network interfaces table", + "Wizard network interfaces table": "向导网络接口表",Also applies to: 1877-1882, 2279-2280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@locales/zh/plugin__kubevirt-plugin.json` at line 1275, Replace the English fallback strings with proper Chinese translations for the localization keys shown (e.g., update the "No disks found" entry) and the other keys mentioned at lines 1877-1882 and 2279-2280 so the zh locale contains native text; locate the keys by their string identifiers (e.g., "No disks found") in the locales/zh/plugin__kubevirt-plugin.json content, substitute the English values with correct Simplified Chinese translations, ensure JSON quoting/escaping remains valid and run a linter/JSON parse to confirm no syntax errors after your edits.src/utils/components/DiskNameCell/DiskNameCell.tsx (1)
32-32: Minor: Redundant fragment wrapper.The fragment
<>...</>aroundwrapper(content)is unnecessary sincewrapperalready returnsReactNode.♻️ Suggested simplification
- return wrapper ? <>{wrapper(content)}</> : content; + return wrapper ? wrapper(content) : content;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/DiskNameCell/DiskNameCell.tsx` at line 32, The return uses an unnecessary fragment around wrapper(content); update the DiskNameCell component's return expression to return wrapper(content) directly when wrapper is truthy (instead of <>wrapper(content)</>)—refer to the wrapper and content variables in the DiskNameCell component and remove the redundant fragment so the expression becomes wrapper ? wrapper(content) : content.src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts (1)
7-18: Good type safety improvement.Changing from
anytoError | nullfor the error tuple element improves type safety.Consider updating the JSDoc on lines 10-12 to mention the error return value for completeness:
/** * A Hook for getting disks data for a VM template * `@param` template - VM template to get disks from - * `@returns` disks data and loading state + * `@returns` disks data, loading state, and error (if any) */,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts` around lines 7 - 18, Update the JSDoc for useTemplateDisksTableData (and/or the type alias UseDisksTableDisks) to document the error element returned in the tuple: state that the hook returns [DiskRowDataLayout[], boolean, Error | null] where the third item is an Error when loading fails or null when there is no error; reference the function name useTemplateDisksTableData and the type UseDisksTableDisks so the documentation clearly matches the signature and error semantics.src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsx (1)
101-119: Minor inconsistency in disabled state descriptions.The "Set link down" action (line 112) shows a description when disabled for SR-IOV interfaces, but the "Set link up" action (lines 102-108) does not include a similar description. For consistency and better UX, consider adding the same description to the "Set link up" action:
♻️ Suggested fix for consistency
{interfaceState === NetworkInterfaceState.DOWN && ( <DropdownItem + description={isSRIOVIface && t('Not available for SR-IOV interfaces')} isDisabled={isSRIOVIface} key="network-interface-state-up" onClick={() => onUpdate(setInterfaceLinkState(vm, nicName, NetworkInterfaceState.UP))} > {t('Set link up')} </DropdownItem> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsx` around lines 101 - 119, The "Set link up" DropdownItem is missing a disabled description for SR-IOV interfaces; update the DropdownItem rendered when interfaceState === NetworkInterfaceState.DOWN to include description={isSRIOVIface && t('Not available for SR-IOV interfaces')} (same as the "Set link down" item) while keeping isDisabled={isSRIOVIface}, key="network-interface-state-up", and the existing onClick that calls onUpdate(setInterfaceLinkState(vm, nicName, NetworkInterfaceState.UP)); this ensures consistent UX for isSRIOVIface-disabled actions.src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx (1)
43-51: Consider consistency with TemplateValue usage in NetworkCell.For pod networking (line 47), the component returns a plain fragment with translated text, but for multus networks (line 50), it uses
TemplateValue. This inconsistency may affect visual presentation. IfTemplateValueapplies specific styling or behavior, consider using it consistently or clarify if the distinction is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx` around lines 43 - 51, NetworkCell returns plain translated text for pod networking but uses TemplateValue for multus networks, causing inconsistent presentation; update NetworkCell so both branches render via TemplateValue for consistent styling/behavior (use TemplateValue with value={t('Pod networking')} in the isPodNetwork(row.network) branch) while keeping the existing multus branch that uses TemplateValue with value={row.network?.multus?.networkName ?? NO_DATA_DASH}; ensure imports and symbols (NetworkCell, isPodNetwork, TemplateValue, NO_DATA_DASH, useKubevirtTranslation) are used accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx`:
- Around line 89-94: The State column sorts on row.iface?.state but renders a
computed value via StateCell which uses
getConfigInterfaceStateFromVM(templateVM, row.network?.name), causing mismatched
sort/display; fix by aligning getValue with the computed state: either change
the ColumnConfig.getValue implementation for the 'state' column to compute the
effective state the same way StateCell does (call
getConfigInterfaceStateFromVM(templateVM, row.network?.name) or an equivalent
helper) or, if getValue cannot access callbacks/templateVM, pre-compute and
attach an effectiveState property to each row in the data layer and have both
getValue and StateCell read that property; update usages of getValue/StateCell
to reference the single source (effective state) so sorting matches rendering.
---
Nitpick comments:
In `@locales/zh/plugin__kubevirt-plugin.json`:
- Line 1275: Replace the English fallback strings with proper Chinese
translations for the localization keys shown (e.g., update the "No disks found"
entry) and the other keys mentioned at lines 1877-1882 and 2279-2280 so the zh
locale contains native text; locate the keys by their string identifiers (e.g.,
"No disks found") in the locales/zh/plugin__kubevirt-plugin.json content,
substitute the English values with correct Simplified Chinese translations,
ensure JSON quoting/escaping remains valid and run a linter/JSON parse to
confirm no syntax errors after your edits.
In `@src/utils/components/DiskNameCell/DiskNameCell.tsx`:
- Line 32: The return uses an unnecessary fragment around wrapper(content);
update the DiskNameCell component's return expression to return wrapper(content)
directly when wrapper is truthy (instead of <>wrapper(content)</>)—refer to the
wrapper and content variables in the DiskNameCell component and remove the
redundant fragment so the expression becomes wrapper ? wrapper(content) :
content.
In
`@src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsx`:
- Around line 101-119: The "Set link up" DropdownItem is missing a disabled
description for SR-IOV interfaces; update the DropdownItem rendered when
interfaceState === NetworkInterfaceState.DOWN to include
description={isSRIOVIface && t('Not available for SR-IOV interfaces')} (same as
the "Set link down" item) while keeping isDisabled={isSRIOVIface},
key="network-interface-state-up", and the existing onClick that calls
onUpdate(setInterfaceLinkState(vm, nicName, NetworkInterfaceState.UP)); this
ensures consistent UX for isSRIOVIface-disabled actions.
In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts`:
- Around line 7-18: Update the JSDoc for useTemplateDisksTableData (and/or the
type alias UseDisksTableDisks) to document the error element returned in the
tuple: state that the hook returns [DiskRowDataLayout[], boolean, Error | null]
where the third item is an Error when loading fails or null when there is no
error; reference the function name useTemplateDisksTableData and the type
UseDisksTableDisks so the documentation clearly matches the signature and error
semantics.
In
`@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx`:
- Around line 43-51: NetworkCell returns plain translated text for pod
networking but uses TemplateValue for multus networks, causing inconsistent
presentation; update NetworkCell so both branches render via TemplateValue for
consistent styling/behavior (use TemplateValue with value={t('Pod networking')}
in the isPodNetwork(row.network) branch) while keeping the existing multus
branch that uses TemplateValue with value={row.network?.multus?.networkName ??
NO_DATA_DASH}; ensure imports and symbols (NetworkCell, isPodNetwork,
TemplateValue, NO_DATA_DASH, useKubevirtTranslation) are used accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23364aea-7e06-48e5-839f-781abb6fe90a
📒 Files selected for processing (27)
locales/en/plugin__kubevirt-plugin.jsonlocales/es/plugin__kubevirt-plugin.jsonlocales/fr/plugin__kubevirt-plugin.jsonlocales/ja/plugin__kubevirt-plugin.jsonlocales/ko/plugin__kubevirt-plugin.jsonlocales/zh/plugin__kubevirt-plugin.jsonsrc/utils/components/DiskNameCell/DiskNameCell.tsxsrc/utils/resources/vm/utils/network/constants.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/disks/WizardDisksTab.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskRow.tsxsrc/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.tssrc/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsxsrc/views/templates/details/tabs/disks/TemplateDisksPage.tsxsrc/views/templates/details/tabs/disks/components/DiskRow.tsxsrc/views/templates/details/tabs/disks/hooks/useDiskColumns.tssrc/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.tssrc/views/templates/details/tabs/disks/templateDisksTableDefinition.tsxsrc/views/templates/details/tabs/disks/utils.tssrc/views/templates/details/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsxsrc/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts
💤 Files with no reviewable changes (6)
- src/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/catalog/wizard/tabs/disks/components/DiskRow.tsx
- src/views/templates/details/tabs/disks/components/DiskRow.tsx
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.ts
- src/views/templates/details/tabs/disks/hooks/useDiskColumns.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx
- locales/es/plugin__kubevirt-plugin.json
- locales/en/plugin__kubevirt-plugin.json
- locales/fr/plugin__kubevirt-plugin.json
- locales/ko/plugin__kubevirt-plugin.json
- src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx
- locales/ja/plugin__kubevirt-plugin.json
batyana
force-pushed
the
CNV-74280-pr8-template-wizard-tables
branch
from
1ab8826 to
3cec1e5
Compare
Collaborator
|
@batyana: An error was encountered searching for bug CNV-74280 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/CNV-74280": GET https://issues.redhat.com/rest/api/2/issue/CNV-74280 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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. |
…tTable Signed-off-by: batyana <bnahmias@redhat.com>
batyana
force-pushed
the
CNV-74280-pr8-template-wizard-tables
branch
from
3cec1e5 to
49dcf3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsx (1)
19-27: Consider usinggetNetworkNameLabelfor consistency withWizardNetworkCells.NetworkCell.
WizardNetworkCells.NetworkCellusesgetNetworkNameLabel(t, { network: row.network })which handles the Pod networking case internally. This component manually implements the same logic. While functionally equivalent, using the shared utility would improve consistency and reduce duplication.♻️ Optional: Use shared utility
+import { getNetworkNameLabel } from '@kubevirt-utils/resources/vm/utils/network/network-columns'; -import { - getConfigInterfaceStateFromVM, - isPodNetwork, -} from '@kubevirt-utils/resources/vm/utils/network/selectors'; +import { getConfigInterfaceStateFromVM } from '@kubevirt-utils/resources/vm/utils/network/selectors'; export const TemplateNetworkCell: FC<TemplateNetworkCellProps> = ({ row }) => { const { t } = useKubevirtTranslation(); - - if (isPodNetwork(row.network)) { - return <>{t('Pod networking')}</>; - } - - return <TemplateValue value={row.network?.multus?.networkName ?? NO_DATA_DASH} />; + const label = getNetworkNameLabel(t, { network: row.network }); + return <TemplateValue value={label ?? NO_DATA_DASH} />; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsx` around lines 19 - 27, TemplateNetworkCell duplicates network-label logic; replace its manual Pod-network check with the shared helper to improve consistency. In TemplateNetworkCell (function TemplateNetworkCell) import and call getNetworkNameLabel(t, { network: row.network }) instead of the current if (isPodNetwork(...)) branch and the TemplateValue usage; pass the result into TemplateValue (or render directly) and keep NO_DATA_DASH fallback behavior consistent with WizardNetworkCells.NetworkCell.src/views/templates/details/tabs/disks/TemplateDisksPage.tsx (1)
50-53: Memoizevmderivation to keepcallbacksstable.
vmis recomputed each render (Line 38), socallbacks(Lines 50-53) can still churn despiteuseMemo. Memoizingvmfromtemplatewill reduce avoidable table/cell rerenders.Proposed diff
- const vm = getTemplateVMWithNamespace(template); + const vm = useMemo(() => getTemplateVMWithNamespace(template), [template]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/TemplateDisksPage.tsx` around lines 50 - 53, The derived vm value used inside TemplateDisksPage is being recomputed each render causing the callbacks memo to churn; wrap the vm computation in a useMemo that depends only on template (e.g., memoize the expression that produces vm from template) and then update the callbacks useMemo dependency array to use that memoized vm instead of the recomputed value so TemplateDiskCallbacks (callbacks) remains stable; adjust references to vm, TemplateDiskCallbacks, callbacks, isTemplateEditable, and onUpdate accordingly.src/views/templates/details/tabs/disks/templateDisksTableDefinition.tsx (1)
96-97: Prefer a non-index fallback for row IDs when possible.Using
disk-${index}as fallback can remount rows after sorting/filtering if a name is empty. Consider using another stable field (for examplemetadata.name/source) before falling back to index.Proposed diff
export const getTemplateDiskRowId = (row: DiskRowDataLayout, index: number): string => - row?.name ?? `disk-${index}`; + row?.name || row?.metadata?.name || row?.source || `disk-${index}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/templateDisksTableDefinition.tsx` around lines 96 - 97, getTemplateDiskRowId currently falls back to an unstable index-based id (disk-${index}) when row.name is missing; change it to prefer another stable field on DiskRowDataLayout such as row.metadata?.name or row.source (in that order) before finally using the index. Update the getTemplateDiskRowId function to return row.name ?? row.metadata?.name ?? row.source ?? `disk-${index}` so rows keep consistent ids across sorts/filters.src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts (1)
7-13: Update the return JSDoc to include the error element.Line 12 still says “disks data and loading state”, but this hook now also returns
loadError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts` around lines 7 - 13, The JSDoc for the UseDisksTableDisks hook is out of date: update the return description in the comment above UseDisksTableDisks (the hook signature taking template: V1Template and returning [DiskRowDataLayout[], boolean, Error | null]) to explicitly state that it returns disks data, a loading state, and a loadError (Error | null) instead of only "disks data and loading state"; keep the existing `@param` and overall comment but replace the `@returns` text to mention all three return elements (disks data, loading boolean, and loadError).src/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsx (1)
14-23: Adopt explicit PVC detection for consistency and robustness.While the current namespace-based detection is functionally safe—namespace is set only when a matching PVC object exists in the row data—using an implicit invariant is fragile. The VM disk tables use a more explicit and maintainable
isPVCSource()helper that checks source values directly. For consistency and to prevent future bugs if row-shaping invariants change, replacerow?.namespace != nullwith explicit PVC logic. Consider either:
- Importing and using the existing
isPVCSource(row)helper from disk table utils- Checking if source is neither
CONTAINER_EPHERMALnorOTHER- Adding a dedicated
isPVC: booleanfield toDiskRowDataLayoutApply the same improvement to
DiskSourceCell.tsxin the wizard for consistency across both template and wizard UIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsx` around lines 14 - 23, The current PVC detection in TemplateDiskSourceCell.tsx uses the fragile check row?.namespace != null; replace this with explicit PVC logic by importing and calling the existing isPVCSource(row) helper (or, if unavailable, check source values excluding CONTAINER_EPHEMERAL/OTHER or add an isPVC boolean to DiskRowDataLayout) and use that result to decide returning the ResourceLink (keep the existing ResourceLink with modelToGroupVersionKind(PersistentVolumeClaimModel), name=row?.source, namespace=row?.namespace); also apply the same change in DiskSourceCell.tsx so both template and wizard UIs use the explicit isPVC detection for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locales/es/plugin__kubevirt-plugin.json`:
- Line 1297: Update the Spanish locale entries that currently use English
placeholders by replacing their values with proper Spanish translations: change
the value for the key "No disks found" to "No se encontraron discos", "Template
disks table" to "Tabla de discos de la plantilla", "Template network interfaces
table" to "Tabla de interfaces de red de la plantilla", "Wizard disks table" to
"Tabla de discos del asistente", and "Wizard network interfaces table" to "Tabla
de interfaces de red del asistente"; ensure you edit the corresponding JSON keys
(e.g., "No disks found", "Template disks table", "Template network interfaces
table", "Wizard disks table", "Wizard network interfaces table") in
locales/es/plugin__kubevirt-plugin.json and keep JSON formatting and quoting
intact.
---
Nitpick comments:
In
`@src/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsx`:
- Around line 14-23: The current PVC detection in TemplateDiskSourceCell.tsx
uses the fragile check row?.namespace != null; replace this with explicit PVC
logic by importing and calling the existing isPVCSource(row) helper (or, if
unavailable, check source values excluding CONTAINER_EPHEMERAL/OTHER or add an
isPVC boolean to DiskRowDataLayout) and use that result to decide returning the
ResourceLink (keep the existing ResourceLink with
modelToGroupVersionKind(PersistentVolumeClaimModel), name=row?.source,
namespace=row?.namespace); also apply the same change in DiskSourceCell.tsx so
both template and wizard UIs use the explicit isPVC detection for consistency.
In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts`:
- Around line 7-13: The JSDoc for the UseDisksTableDisks hook is out of date:
update the return description in the comment above UseDisksTableDisks (the hook
signature taking template: V1Template and returning [DiskRowDataLayout[],
boolean, Error | null]) to explicitly state that it returns disks data, a
loading state, and a loadError (Error | null) instead of only "disks data and
loading state"; keep the existing `@param` and overall comment but replace the
`@returns` text to mention all three return elements (disks data, loading boolean,
and loadError).
In `@src/views/templates/details/tabs/disks/TemplateDisksPage.tsx`:
- Around line 50-53: The derived vm value used inside TemplateDisksPage is being
recomputed each render causing the callbacks memo to churn; wrap the vm
computation in a useMemo that depends only on template (e.g., memoize the
expression that produces vm from template) and then update the callbacks useMemo
dependency array to use that memoized vm instead of the recomputed value so
TemplateDiskCallbacks (callbacks) remains stable; adjust references to vm,
TemplateDiskCallbacks, callbacks, isTemplateEditable, and onUpdate accordingly.
In `@src/views/templates/details/tabs/disks/templateDisksTableDefinition.tsx`:
- Around line 96-97: getTemplateDiskRowId currently falls back to an unstable
index-based id (disk-${index}) when row.name is missing; change it to prefer
another stable field on DiskRowDataLayout such as row.metadata?.name or
row.source (in that order) before finally using the index. Update the
getTemplateDiskRowId function to return row.name ?? row.metadata?.name ??
row.source ?? `disk-${index}` so rows keep consistent ids across sorts/filters.
In
`@src/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsx`:
- Around line 19-27: TemplateNetworkCell duplicates network-label logic; replace
its manual Pod-network check with the shared helper to improve consistency. In
TemplateNetworkCell (function TemplateNetworkCell) import and call
getNetworkNameLabel(t, { network: row.network }) instead of the current if
(isPodNetwork(...)) branch and the TemplateValue usage; pass the result into
TemplateValue (or render directly) and keep NO_DATA_DASH fallback behavior
consistent with WizardNetworkCells.NetworkCell.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec09c998-7c96-443a-a51e-e531eb9183c9
📒 Files selected for processing (31)
locales/en/plugin__kubevirt-plugin.jsonlocales/es/plugin__kubevirt-plugin.jsonlocales/fr/plugin__kubevirt-plugin.jsonlocales/ja/plugin__kubevirt-plugin.jsonlocales/ko/plugin__kubevirt-plugin.jsonlocales/zh/plugin__kubevirt-plugin.jsonsrc/utils/components/DiskNameCell/DiskNameCell.tsxsrc/utils/resources/vm/utils/network/constants.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/disks/WizardDisksTab.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskRow.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskSourceCell.tsxsrc/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.tssrc/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/catalog/wizard/tabs/network/components/list/WizardNetworkCells.tsxsrc/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsxsrc/views/templates/details/tabs/disks/TemplateDisksPage.tsxsrc/views/templates/details/tabs/disks/components/DiskRow.tsxsrc/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsxsrc/views/templates/details/tabs/disks/hooks/useDiskColumns.tssrc/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.tssrc/views/templates/details/tabs/disks/templateDisksTableDefinition.tsxsrc/views/templates/details/tabs/disks/utils.tssrc/views/templates/details/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsxsrc/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsxsrc/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts
💤 Files with no reviewable changes (6)
- src/views/templates/details/tabs/disks/hooks/useDiskColumns.ts
- src/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.ts
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/catalog/wizard/tabs/disks/components/DiskRow.tsx
- src/views/templates/details/tabs/disks/components/DiskRow.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- src/utils/resources/vm/utils/network/constants.ts
- locales/zh/plugin__kubevirt-plugin.json
- src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsx
- locales/fr/plugin__kubevirt-plugin.json
- src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx
- src/views/catalog/wizard/tabs/disks/WizardDisksTab.tsx
- locales/ko/plugin__kubevirt-plugin.json
- locales/en/plugin__kubevirt-plugin.json
- locales/ja/plugin__kubevirt-plugin.json
- src/utils/resources/vm/utils/network/selectors.ts
- src/utils/components/DiskNameCell/DiskNameCell.tsx
| "No Datapoints found": "No se encontraron puntos de datos", | ||
| "No dedicated resources applied": "No se aplicaron recursos dedicados", | ||
| "No default value": "Sin valor predeterminado", | ||
| "No disks found": "No disks found", |
There was a problem hiding this comment.
Missing Spanish translations for new locale keys.
The new keys added to the Spanish locale file have English text as placeholder values instead of actual Spanish translations. Spanish-speaking users will see English text for these UI elements.
Keys requiring translation:
- Line 1297:
"No disks found"→ e.g.,"No se encontraron discos" - Line 1901:
"Template disks table"→ e.g.,"Tabla de discos de la plantilla" - Line 1906:
"Template network interfaces table"→ e.g.,"Tabla de interfaces de red de la plantilla" - Line 2307:
"Wizard disks table"→ e.g.,"Tabla de discos del asistente" - Line 2308:
"Wizard network interfaces table"→ e.g.,"Tabla de interfaces de red del asistente"
Also applies to: 1901-1901, 1906-1906, 2307-2308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/es/plugin__kubevirt-plugin.json` at line 1297, Update the Spanish
locale entries that currently use English placeholders by replacing their values
with proper Spanish translations: change the value for the key "No disks found"
to "No se encontraron discos", "Template disks table" to "Tabla de discos de la
plantilla", "Template network interfaces table" to "Tabla de interfaces de red
de la plantilla", "Wizard disks table" to "Tabla de discos del asistente", and
"Wizard network interfaces table" to "Tabla de interfaces de red del asistente";
ensure you edit the corresponding JSON keys (e.g., "No disks found", "Template
disks table", "Template network interfaces table", "Wizard disks table", "Wizard
network interfaces table") in locales/es/plugin__kubevirt-plugin.json and keep
JSON formatting and quoting intact.
Collaborator
|
@batyana: An error was encountered searching for bug CNV-74280 on the Jira server at https://issues.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
No response returned: Get "https://issues.redhat.com/rest/api/2/issue/CNV-74280": GET https://issues.redhat.com/rest/api/2/issue/CNV-74280 giving up after 5 attempt(s)
Please contact an administrator to resolve this issue, then request a bug refresh with 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: 4
♻️ Duplicate comments (1)
src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx (1)
61-65:⚠️ Potential issue | 🟡 MinorState sort key is inconsistent with rendered state.
Line 61 sorts by raw
row.iface?.state, while Line 64 renders effective state viaTemplateStateCell(getConfigInterfaceStateFromVM(...)). Rows can display one state but sort as another/empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx` around lines 61 - 65, The column is sorting on raw iface state (getValue: row.iface?.state) while rendering uses TemplateStateCell which computes the effective state via getConfigInterfaceStateFromVM; change the column's getValue/key so it returns the same effective state used by TemplateStateCell (call the same helper used there, passing the row and callbacks.template or extract that logic into a shared function) so rendered value and sort key match.
🧹 Nitpick comments (1)
src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts (1)
7-13: Update the hook docs to reflect the new tuple shape.Line 12 still says only “data and loading state”, but the hook now also returns a load error (
Error | null).📝 Suggested doc update
/** * A Hook for getting disks data for a VM template * `@param` template - VM template to get disks from - * `@returns` disks data and loading state + * `@returns` disks data, loading state, and load error */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts` around lines 7 - 13, The comment notes the hook's JSDoc is out of date: update the docblock for UseDisksTableDisks / the hook that returns [DiskRowDataLayout[], boolean, Error | null] to describe all three tuple elements (disk data array, loading boolean, and load error Error | null) and update the `@returns` line to explicitly list "disks data, loading state, and load error (Error | null)"; ensure the function/type name UseDisksTableDisks and any reference to the hook reflect the new tuple shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@locales/fr/plugin__kubevirt-plugin.json`:
- Line 1303: The French locale contains untranslated English values (e.g., the
key "No disks found" and a few other English-valued keys in the same FR locale
JSON), so replace those English values with proper French translations (for
example "Aucun disque trouvé" for "No disks found") by editing the corresponding
JSON properties in the French locale file, ensure string punctuation and
escaping match the surrounding entries, and run the locale/lint checks to
confirm no syntax or missing-key regressions.
In `@locales/zh/plugin__kubevirt-plugin.json`:
- Line 1281: Several string values in the Chinese locale are left in English
(e.g., the value for the key "No disks found" and the other English placeholder
entries referenced in the review); replace those English placeholder values with
their proper Chinese translations while leaving the JSON keys unchanged, ensure
proper UTF-8 encoding and valid JSON string escaping, and run a quick scan to
confirm no other English values remain in this locale file.
In
`@src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx`:
- Around line 62-65: getValue currently returns raw row.iface?.state while
renderCell shows the user the derived state from StateCell (which uses
getConfigInterfaceStateFromVM), causing sort/display mismatch; change getValue
to compute and return the same effective state by calling
getConfigInterfaceStateFromVM with the same inputs used by StateCell (e.g.,
getValue: (row, callbacks) => getConfigInterfaceStateFromVM(callbacks.vm,
row.iface) ?? ''), ensuring the sort key ('state') matches the rendered value
from StateCell and using the same VM/context.
In `@src/views/templates/details/tabs/disks/TemplateDisksPage.tsx`:
- Around line 50-53: The callbacks object sets actionsDisabled based only on
isTemplateEditable so actions remain enabled when vm is undefined; change the
actionsDisabled expression in the useMemo that creates TemplateDiskCallbacks to
also check vm (e.g., actionsDisabled: !isTemplateEditable || !vm) and keep vm in
the dependency array so actions are disabled whenever vm is missing or the
template is not editable.
---
Duplicate comments:
In
`@src/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsx`:
- Around line 61-65: The column is sorting on raw iface state (getValue:
row.iface?.state) while rendering uses TemplateStateCell which computes the
effective state via getConfigInterfaceStateFromVM; change the column's
getValue/key so it returns the same effective state used by TemplateStateCell
(call the same helper used there, passing the row and callbacks.template or
extract that logic into a shared function) so rendered value and sort key match.
---
Nitpick comments:
In `@src/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.ts`:
- Around line 7-13: The comment notes the hook's JSDoc is out of date: update
the docblock for UseDisksTableDisks / the hook that returns
[DiskRowDataLayout[], boolean, Error | null] to describe all three tuple
elements (disk data array, loading boolean, and load error Error | null) and
update the `@returns` line to explicitly list "disks data, loading state, and load
error (Error | null)"; ensure the function/type name UseDisksTableDisks and any
reference to the hook reflect the new tuple shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b890b9e7-8adc-41ef-9d74-903c88f500b7
📒 Files selected for processing (31)
locales/en/plugin__kubevirt-plugin.jsonlocales/es/plugin__kubevirt-plugin.jsonlocales/fr/plugin__kubevirt-plugin.jsonlocales/ja/plugin__kubevirt-plugin.jsonlocales/ko/plugin__kubevirt-plugin.jsonlocales/zh/plugin__kubevirt-plugin.jsonsrc/utils/components/DiskNameCell/DiskNameCell.tsxsrc/utils/resources/vm/utils/network/constants.tssrc/utils/resources/vm/utils/network/selectors.tssrc/views/catalog/wizard/tabs/disks/WizardDisksTab.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskRow.tsxsrc/views/catalog/wizard/tabs/disks/components/DiskSourceCell.tsxsrc/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.tssrc/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/catalog/wizard/tabs/network/components/list/WizardNetworkCells.tsxsrc/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsxsrc/views/templates/details/tabs/disks/TemplateDisksPage.tsxsrc/views/templates/details/tabs/disks/components/DiskRow.tsxsrc/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsxsrc/views/templates/details/tabs/disks/hooks/useDiskColumns.tssrc/views/templates/details/tabs/disks/hooks/useTemplateDisksTableData.tssrc/views/templates/details/tabs/disks/templateDisksTableDefinition.tsxsrc/views/templates/details/tabs/disks/utils.tssrc/views/templates/details/tabs/network/components/list/NetworkInterfaceList.tsxsrc/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsxsrc/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsxsrc/views/templates/details/tabs/network/components/list/templateNetworkInterfaceDefinition.tsxsrc/views/virtualmachines/details/tabs/configuration/network/utils/utils.ts
💤 Files with no reviewable changes (6)
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceRow.tsx
- src/views/templates/details/tabs/disks/components/DiskRow.tsx
- src/views/catalog/wizard/tabs/disks/hooks/useDiskColumns.ts
- src/views/templates/details/tabs/disks/hooks/useDiskColumns.ts
- src/views/catalog/wizard/tabs/disks/components/DiskRow.tsx
- src/views/templates/details/tabs/network/components/list/NetworkInterfaceRow.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- src/utils/resources/vm/utils/network/constants.ts
- src/views/templates/details/tabs/network/components/list/TemplateNetworkCells.tsx
- src/views/catalog/wizard/tabs/disks/wizardDisksTableDefinition.tsx
- src/utils/resources/vm/utils/network/selectors.ts
- src/views/catalog/wizard/tabs/network/components/list/NetworkInterfaceActions.tsx
- src/views/templates/details/tabs/disks/templateDisksTableDefinition.tsx
- src/views/catalog/wizard/tabs/disks/components/DiskSourceCell.tsx
- locales/es/plugin__kubevirt-plugin.json
- locales/ko/plugin__kubevirt-plugin.json
- src/views/catalog/wizard/tabs/network/components/list/WizardNetworkCells.tsx
- src/views/templates/details/tabs/disks/components/TemplateDiskSourceCell.tsx
- src/utils/components/DiskNameCell/DiskNameCell.tsx
- locales/ja/plugin__kubevirt-plugin.json
| "No Datapoints found": "Aucun point de données trouvé.", | ||
| "No dedicated resources applied": "Aucune ressource dédiée appliquée", | ||
| "No default value": "Valeur par défaut", | ||
| "No disks found": "No disks found", |
There was a problem hiding this comment.
Translate the newly added French locale strings.
Line 1303 and Lines 1908, 1913, 2315-2316 are currently English values in the French locale file, so FR users will see mixed-language UI in these new table labels/empty states.
Also applies to: 1908-1908, 1913-1913, 2315-2316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/fr/plugin__kubevirt-plugin.json` at line 1303, The French locale
contains untranslated English values (e.g., the key "No disks found" and a few
other English-valued keys in the same FR locale JSON), so replace those English
values with proper French translations (for example "Aucun disque trouvé" for
"No disks found") by editing the corresponding JSON properties in the French
locale file, ensure string punctuation and escaping match the surrounding
entries, and run the locale/lint checks to confirm no syntax or missing-key
regressions.
| "No Datapoints found": "未找到数据点", | ||
| "No dedicated resources applied": "没有应用专用资源", | ||
| "No default value": "没有默认值", | ||
| "No disks found": "No disks found", |
There was a problem hiding this comment.
Localize newly added zh strings instead of keeping English placeholders.
Line 1281, Line 1884, Line 1889, Line 2287, and Line 2288 are still English in the Chinese locale file, which will surface mixed-language labels in the UI.
🌐 Proposed translation update
- "No disks found": "No disks found",
+ "No disks found": "未找到磁盘",
...
- "Template disks table": "Template disks table",
+ "Template disks table": "模板磁盘表",
...
- "Template network interfaces table": "Template network interfaces table",
+ "Template network interfaces table": "模板网络接口表",
...
- "Wizard disks table": "Wizard disks table",
+ "Wizard disks table": "向导磁盘表",
- "Wizard network interfaces table": "Wizard network interfaces table",
+ "Wizard network interfaces table": "向导网络接口表",Also applies to: 1884-1884, 1889-1889, 2287-2288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/zh/plugin__kubevirt-plugin.json` at line 1281, Several string values
in the Chinese locale are left in English (e.g., the value for the key "No disks
found" and the other English placeholder entries referenced in the review);
replace those English placeholder values with their proper Chinese translations
while leaving the JSON keys unchanged, ensure proper UTF-8 encoding and valid
JSON string escaping, and run a quick scan to confirm no other English values
remain in this locale file.
Comment on lines
+62
to
+65
| getValue: (row) => row.iface?.state ?? '', | ||
| key: 'state', | ||
| label: t('State'), | ||
| renderCell: (row, callbacks) => <StateCell row={row} vm={callbacks.vm} />, |
There was a problem hiding this comment.
State sorting can drift from what the cell displays.
Line 62 sorts using raw row.iface?.state, but Line 65 renders derived state from StateCell (getConfigInterfaceStateFromVM(...)). Please align the sort value with the rendered effective state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/views/catalog/wizard/tabs/network/components/list/wizardNetworkInterfaceDefinition.tsx`
around lines 62 - 65, getValue currently returns raw row.iface?.state while
renderCell shows the user the derived state from StateCell (which uses
getConfigInterfaceStateFromVM), causing sort/display mismatch; change getValue
to compute and return the same effective state by calling
getConfigInterfaceStateFromVM with the same inputs used by StateCell (e.g.,
getValue: (row, callbacks) => getConfigInterfaceStateFromVM(callbacks.vm,
row.iface) ?? ''), ensuring the sort key ('state') matches the rendered value
from StateCell and using the same VM/context.
Comment on lines
+50
to
+53
| const callbacks: TemplateDiskCallbacks = useMemo( | ||
| () => ({ actionsDisabled: !isTemplateEditable, onUpdate, vm }), | ||
| [isTemplateEditable, onUpdate, vm], | ||
| ); |
There was a problem hiding this comment.
Disable disk actions when vm is unavailable.
callbacks.actionsDisabled only checks editability. If vm is missing, actions can still be enabled and may fail downstream.
✅ Suggested safeguard
const callbacks: TemplateDiskCallbacks = useMemo(
- () => ({ actionsDisabled: !isTemplateEditable, onUpdate, vm }),
+ () => ({ actionsDisabled: !isTemplateEditable || !vm, onUpdate, vm }),
[isTemplateEditable, onUpdate, vm],
);📝 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.
Suggested change
| const callbacks: TemplateDiskCallbacks = useMemo( | |
| () => ({ actionsDisabled: !isTemplateEditable, onUpdate, vm }), | |
| [isTemplateEditable, onUpdate, vm], | |
| ); | |
| const callbacks: TemplateDiskCallbacks = useMemo( | |
| () => ({ actionsDisabled: !isTemplateEditable || !vm, onUpdate, vm }), | |
| [isTemplateEditable, onUpdate, vm], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/templates/details/tabs/disks/TemplateDisksPage.tsx` around lines 50
- 53, The callbacks object sets actionsDisabled based only on isTemplateEditable
so actions remain enabled when vm is undefined; change the actionsDisabled
expression in the useMemo that creates TemplateDiskCallbacks to also check vm
(e.g., actionsDisabled: !isTemplateEditable || !vm) and keep vm in the
dependency array so actions are disabled whenever vm is missing or the template
is not editable.
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
4 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.
📝 Description
Summary
🎥 Demo
Screen.Recording.2026-03-10.at.19.50.07.mov
Summary by CodeRabbit
New Features
Improvements