CNV-79276:Virtual machine networks page - row overlapping#3566
CNV-79276:Virtual machine networks page - row overlapping#3566aviavissar wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
|
@aviavissar: This pull request references CNV-79276 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aviavissar 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 |
📝 WalkthroughWalkthroughThis PR adds localization strings for VM network features across six language packs, introduces a new hook for configuring VM network table columns, refactors the VM network list component to use KubevirtTable, updates the useVMNetworkColumns hook to return a loading state flag, and enhances ExpandableProjectList with resize event handling for virtualized table measurement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
@aviavissar: This pull request references CNV-79276 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx (1)
44-52: Extract the repeated48pxheight into a named constant (and preferably shared styling).The same literal height is duplicated in two inline style objects. Please centralize it to reduce drift and match constants/style conventions.
♻️ Proposed refactor
+const CELL_MIN_HEIGHT_PX = 48; + ... - <div style={{ alignItems: 'center', display: 'flex', minHeight: '48px' }}> + <div style={{ alignItems: 'center', display: 'flex', minHeight: `${CELL_MIN_HEIGHT_PX}px` }}> <Skeleton /> </div> ... - <div style={{ minHeight: '48px' }}> + <div style={{ minHeight: `${CELL_MIN_HEIGHT_PX}px` }}>As per coding guidelines: "Extract styles into separate files; don't embed styles in components" and "Define magic numbers as named constants in constants.ts with units in name."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx` around lines 44 - 52, The component uses the magic value '48px' in two inline style objects (the Skeleton wrapper and the cellContent wrapper) — extract this into a named constant (e.g., ROW_MIN_HEIGHT_PX or ROW_MIN_HEIGHT_48_PX) in your shared constants (constants.ts) and replace the inline literals with that constant; also move the style objects out of the JSX into a shared style/module or a local styles object (e.g., const styles = { rowMinHeight: { minHeight: ROW_MIN_HEIGHT_PX } }) and update the references in ExpandableProjectList (the div around Skeleton and the cellContent div) so no inline '48px' literals remain and styling follows the project's styles/constants conventions.
🤖 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/en/plugin__kubevirt-plugin.json`:
- Line 1290: The localization key/value "No virtualmachine networks found" uses
incorrect casing; update it to "No VirtualMachine networks found" to match the
project's PascalCase convention for the resource name (match other entries like
"No VirtualMachine templates found"); ensure both the JSON key and value (if
identical) are changed consistently so lookup and display remain correct.
In `@src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx`:
- Around line 28-40: The cleanup only cancels the outer requestAnimationFrame
handle; capture the nested rAF id as well and cancel it to avoid the inner
callback dispatching a stale 'resize' after rapid toggles/unmount. Update the
effect in ExpandableProjectList: declare a second variable (e.g., innerId) that
stores the inner requestAnimationFrame returned inside the outer callback, and
call cancelAnimationFrame on both id and innerId in the returned cleanup; keep
the isFirstRun, expand dependency and window.dispatchEvent usage unchanged and,
if this is a workaround, add a comment referencing the tracked issue.
In `@src/views/vmnetworks/list/hooks/useVMNetworkTableColumns.tsx`:
- Around line 43-47: The VLAN column is being sorted lexicographically because
getValue currently returns a string; change getValue in the vlanID column to
return a numeric value instead (e.g. Number(getVLANID(row)) when present, or
null/undefined when absent) so sorting is numeric; keep renderCell as-is
(getVLANID(row) ?? NO_DATA_DASH) and reference the column key 'vlanID',
getValue, getVLANID, and renderCell when making the change.
In `@src/views/vmnetworks/list/VMNetworkList.tsx`:
- Around line 38-45: Normalize the VM network UI copy to a single consistent
phrase and i18n key: replace the inconsistent "VirtualMachine
networks"/"virtualmachine networks" strings passed to t(...) in VMNetworkList
(props like ariaLabel and noFilteredDataEmptyMsg) with a standardized phrase
such as "Virtual machine networks" and "No virtual machine networks found";
update the corresponding translation keys/entries to match the new strings and
ensure getRowId/getName usage remains unchanged.
---
Nitpick comments:
In `@src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx`:
- Around line 44-52: The component uses the magic value '48px' in two inline
style objects (the Skeleton wrapper and the cellContent wrapper) — extract this
into a named constant (e.g., ROW_MIN_HEIGHT_PX or ROW_MIN_HEIGHT_48_PX) in your
shared constants (constants.ts) and replace the inline literals with that
constant; also move the style objects out of the JSX into a shared style/module
or a local styles object (e.g., const styles = { rowMinHeight: { minHeight:
ROW_MIN_HEIGHT_PX } }) and update the references in ExpandableProjectList (the
div around Skeleton and the cellContent div) so no inline '48px' literals remain
and styling follows the project's styles/constants conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 258ca582-fa03-4225-8e2b-0ca2a5cd1c1c
📒 Files selected for processing (10)
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/ExpandableProjectList/ExpandableProjectList.tsxsrc/views/vmnetworks/list/VMNetworkList.tsxsrc/views/vmnetworks/list/hooks/useVMNetworkColumns.tsxsrc/views/vmnetworks/list/hooks/useVMNetworkTableColumns.tsx
| "No Upload URL found {{configError}}": "No Upload URL found {{configError}}", | ||
| "No username set, see operating system documentation for the default username.": "No username set, see operating system documentation for the default username.", | ||
| "No valid results found in ConfigMap": "No valid results found in ConfigMap", | ||
| "No virtualmachine networks found": "No virtualmachine networks found", |
There was a problem hiding this comment.
Inconsistent casing: virtualmachine should be VirtualMachine.
This entry uses virtualmachine (lowercase) while the rest of the file consistently uses VirtualMachine (PascalCase) for the resource name—see lines 1291–1293 for similar "No ... found" patterns.
🔧 Suggested fix
- "No virtualmachine networks found": "No virtualmachine networks found",
+ "No VirtualMachine networks found": "No VirtualMachine networks found",📝 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.
| "No virtualmachine networks found": "No virtualmachine networks found", | |
| "No VirtualMachine networks found": "No VirtualMachine networks found", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/en/plugin__kubevirt-plugin.json` at line 1290, The localization
key/value "No virtualmachine networks found" uses incorrect casing; update it to
"No VirtualMachine networks found" to match the project's PascalCase convention
for the resource name (match other entries like "No VirtualMachine templates
found"); ensure both the JSON key and value (if identical) are changed
consistently so lookup and display remain correct.
| // Notify virtualized table to re-measure after expand/collapse so row overlap is avoided | ||
| useEffect(() => { | ||
| if (isFirstRun.current) { | ||
| isFirstRun.current = false; | ||
| return; | ||
| } | ||
| const id = requestAnimationFrame(() => { | ||
| requestAnimationFrame(() => { | ||
| window.dispatchEvent(new Event('resize')); | ||
| }); | ||
| }); | ||
| return () => cancelAnimationFrame(id); | ||
| }, [expand]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx | head -50Repository: kubevirt-ui/kubevirt-plugin
Length of output: 1911
🏁 Script executed:
rg "CNV-79276" --type ts --type tsxRepository: kubevirt-ui/kubevirt-plugin
Length of output: 98
🏁 Script executed:
rg "CNV-79276"Repository: kubevirt-ui/kubevirt-plugin
Length of output: 53
🏁 Script executed:
# Check if there are any comments or references to issues in the ExpandableProjectList file
cat src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx | grep -i "cny\|cnv\|issue\|jira\|bug"Repository: kubevirt-ui/kubevirt-plugin
Length of output: 53
🏁 Script executed:
# Check if there's a constants file for this component
ls -la src/utils/components/ExpandableProjectList/Repository: kubevirt-ui/kubevirt-plugin
Length of output: 326
🏁 Script executed:
cat src/utils/components/ExpandableProjectList/constants.tsRepository: kubevirt-ui/kubevirt-plugin
Length of output: 103
🏁 Script executed:
# Check if there are any other references to this pattern or related issues
rg "requestAnimationFrame" src/utils/components/ExpandableProjectList/ -A 5 -B 2Repository: kubevirt-ui/kubevirt-plugin
Length of output: 920
Cancel both animation-frame callbacks in the resize workaround.
Line 35 schedules a second requestAnimationFrame, but line 39 cancels only the first handle. The inner callback can still fire after rapid toggle/unmount and dispatch a stale global resize event.
Add a variable to capture the nested animation frame ID and cancel it in the cleanup function:
🔧 Proposed fix
// Notify virtualized table to re-measure after expand/collapse so row overlap is avoided
useEffect(() => {
if (isFirstRun.current) {
isFirstRun.current = false;
return;
}
+ let nestedId: number | undefined;
const id = requestAnimationFrame(() => {
- requestAnimationFrame(() => {
+ nestedId = requestAnimationFrame(() => {
window.dispatchEvent(new Event('resize'));
});
});
- return () => cancelAnimationFrame(id);
+ return () => {
+ cancelAnimationFrame(id);
+ if (nestedId !== undefined) cancelAnimationFrame(nestedId);
+ };
}, [expand]);If this is a workaround for a tracked issue, update the comment to reference it per coding guidelines.
📝 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.
| // Notify virtualized table to re-measure after expand/collapse so row overlap is avoided | |
| useEffect(() => { | |
| if (isFirstRun.current) { | |
| isFirstRun.current = false; | |
| return; | |
| } | |
| const id = requestAnimationFrame(() => { | |
| requestAnimationFrame(() => { | |
| window.dispatchEvent(new Event('resize')); | |
| }); | |
| }); | |
| return () => cancelAnimationFrame(id); | |
| }, [expand]); | |
| // Notify virtualized table to re-measure after expand/collapse so row overlap is avoided | |
| useEffect(() => { | |
| if (isFirstRun.current) { | |
| isFirstRun.current = false; | |
| return; | |
| } | |
| let nestedId: number | undefined; | |
| const id = requestAnimationFrame(() => { | |
| nestedId = requestAnimationFrame(() => { | |
| window.dispatchEvent(new Event('resize')); | |
| }); | |
| }); | |
| return () => { | |
| cancelAnimationFrame(id); | |
| if (nestedId !== undefined) cancelAnimationFrame(nestedId); | |
| }; | |
| }, [expand]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/components/ExpandableProjectList/ExpandableProjectList.tsx` around
lines 28 - 40, The cleanup only cancels the outer requestAnimationFrame handle;
capture the nested rAF id as well and cancel it to avoid the inner callback
dispatching a stale 'resize' after rapid toggles/unmount. Update the effect in
ExpandableProjectList: declare a second variable (e.g., innerId) that stores the
inner requestAnimationFrame returned inside the outer callback, and call
cancelAnimationFrame on both id and innerId in the returned cleanup; keep the
isFirstRun, expand dependency and window.dispatchEvent usage unchanged and, if
this is a workaround, add a comment referencing the tracked issue.
| getValue: (row) => String(getVLANID(row) ?? ''), | ||
| key: 'vlanID', | ||
| label: t('VLAN ID'), | ||
| renderCell: (row) => getVLANID(row) ?? NO_DATA_DASH, | ||
| sortable: true, |
There was a problem hiding this comment.
Fix VLAN sorting to be numeric, not lexicographic.
Line 43 converts VLAN ID to string, which can misorder sortable results.
🔧 Suggested fix
- getValue: (row) => String(getVLANID(row) ?? ''),
+ getValue: (row) => getVLANID(row) ?? -1,📝 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.
| getValue: (row) => String(getVLANID(row) ?? ''), | |
| key: 'vlanID', | |
| label: t('VLAN ID'), | |
| renderCell: (row) => getVLANID(row) ?? NO_DATA_DASH, | |
| sortable: true, | |
| getValue: (row) => getVLANID(row) ?? -1, | |
| key: 'vlanID', | |
| label: t('VLAN ID'), | |
| renderCell: (row) => getVLANID(row) ?? NO_DATA_DASH, | |
| sortable: true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/vmnetworks/list/hooks/useVMNetworkTableColumns.tsx` around lines 43
- 47, The VLAN column is being sorted lexicographically because getValue
currently returns a string; change getValue in the vlanID column to return a
numeric value instead (e.g. Number(getVLANID(row)) when present, or
null/undefined when absent) so sorting is numeric; keep renderCell as-is
(getVLANID(row) ?? NO_DATA_DASH) and reference the column key 'vlanID',
getValue, getVLANID, and renderCell when making the change.
| ariaLabel={t('VirtualMachine networks')} | ||
| columns={columns} | ||
| data={filteredData} | ||
| getRowId={(row) => getName(row) ?? ''} | ||
| loaded={loaded} | ||
| loadError={error} | ||
| noFilteredDataEmptyMsg={t('No virtualmachine networks found')} | ||
| unfilteredData={data} |
There was a problem hiding this comment.
Normalize VM network labels to avoid typo and i18n key drift.
Line 44 uses "No virtualmachine networks found" (missing space), and Line 38/44 introduce inconsistent wording variants. This will surface awkward UI copy and duplicate translation keys.
💡 Suggested fix
- ariaLabel={t('VirtualMachine networks')}
+ ariaLabel={t('Virtual machine networks')}
...
- noFilteredDataEmptyMsg={t('No virtualmachine networks found')}
+ noFilteredDataEmptyMsg={t('No virtual machine networks found')}📝 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.
| ariaLabel={t('VirtualMachine networks')} | |
| columns={columns} | |
| data={filteredData} | |
| getRowId={(row) => getName(row) ?? ''} | |
| loaded={loaded} | |
| loadError={error} | |
| noFilteredDataEmptyMsg={t('No virtualmachine networks found')} | |
| unfilteredData={data} | |
| ariaLabel={t('Virtual machine networks')} | |
| columns={columns} | |
| data={filteredData} | |
| getRowId={(row) => getName(row) ?? ''} | |
| loaded={loaded} | |
| loadError={error} | |
| noFilteredDataEmptyMsg={t('No virtual machine networks found')} | |
| unfilteredData={data} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/vmnetworks/list/VMNetworkList.tsx` around lines 38 - 45, Normalize
the VM network UI copy to a single consistent phrase and i18n key: replace the
inconsistent "VirtualMachine networks"/"virtualmachine networks" strings passed
to t(...) in VMNetworkList (props like ariaLabel and noFilteredDataEmptyMsg)
with a standardized phrase such as "Virtual machine networks" and "No virtual
machine networks found"; update the corresponding translation keys/entries to
match the new strings and ensure getRowId/getName usage remains unchanged.
| @@ -0,0 +1,70 @@ | |||
| import React, { useMemo } from 'react'; | |||
| import { Link } from 'react-router-dom-v5-compat'; | |||
There was a problem hiding this comment.
Simpler fix available
The root cause diagnosis is correct, but the solution can be simplified.
Issues with this PR
The ExpandableProjectList change is unnecessary and should be reverted.
KubevirtTable renders all rows in the DOM — there is no virtualizer to notify.
The window.dispatchEvent(new Event('resize')) workaround was needed for
VirtualizedTable, which this PR already removes. It also has a bug: the inner
requestAnimationFrame is not cancelled on cleanup.
useVMNetworkColumns.tsx becomes dead code but is not deleted.
Cleaner approach
Same number of files (VMNetworkList, new hook, deleted hook, 6 locales), but no changes to ExpandableProjectList.tsx:
- Replace
useVMNetworkColumns.tsxwithuseVMNetworkTableColumns.tsx—
column config inColumnConfig[]format forKubevirtTable, with explicit
pf-m-width-*props to preserve column widths. VLAN ID sort fixed to numeric. - Update
VMNetworkList.tsx— swapVirtualizedTable→KubevirtTable. - Delete
useVMNetworkColumns.tsx. - Add
"No virtual machine networks found"to all 6 locale files (en, es, fr, ja, ko, zh). - Zero changes to
ExpandableProjectList.tsx.
AI prompt to generate this fix
In
kubevirt-ui/kubevirt-plugin, fix the row-overlap bug on the VM Networks
page (VMNetworkList.tsx).
- Create
src/views/vmnetworks/list/hooks/useVMNetworkTableColumns.tsx
returningColumnConfig<ClusterUserDefinedNetworkKind>[]forKubevirtTable.
Columns: Name (link), Connected projects (<MatchedProjects>), Physical
network name, VLAN ID (numeric sort), MTU (numeric sort), Actions (kebab).
Addpf-m-width-*props to pin column widths (20 / 25 / 20 / 15 / 15 for
the five data columns;pf-v6-c-table__actionfor actions).- Update
VMNetworkList.tsx— replaceVirtualizedTablewithKubevirtTable,
use the new hook, addariaLabel,getRowId, andnoFilteredDataEmptyMsg.- Delete
useVMNetworkColumns.tsx(dead code).- Add
"No virtual machine networks found"to
locales/en/plugin__kubevirt-plugin.json.- Make no changes to
ExpandableProjectList.tsx.
There was a problem hiding this comment.
Also - please note that while it is not part of the Jira - I think it's work adding a loading state to the table.
There was a problem hiding this comment.
3 participants
📝 Description
🎥 Demo
Video_2026-03-05_17-04-08.mp4
Summary by CodeRabbit
New Features
Improvements