CNV-74280: migrate VMNetwork Details tables to KubevirtTable#3588
Draft
batyana wants to merge 1 commit intokubevirt-ui:mainfrom
Draft
CNV-74280: migrate VMNetwork Details tables to KubevirtTable#3588batyana wants to merge 1 commit intokubevirt-ui:mainfrom
batyana wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
Signed-off-by: batyana <bnahmias@redhat.com>
Contributor
|
Skipping CI for Draft Pull Request. |
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 |
📝 WalkthroughWalkthroughThis PR refactors the KubevirtTable component to support row selection, adds new localization keys for table variants and empty-state messages across six languages, and migrates the Connected Projects and Connected Virtual Machines tabs to use the enhanced table with integrated selection and updated column definitions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
locales/en/plugin__kubevirt-plugin.json (1)
1350-1350: Consider reusing the existing generic filtered-empty key to reduce translation duplication.Line 1350 is close in meaning to the existing generic “No results match the filter criteria.” If VM-specific wording is not required, reusing one key can lower i18n maintenance.
🤖 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 1350, Replace the VM-specific i18n entry "No virtual machines match the filter criteria" with a reuse of the existing generic message key "No results match the filter criteria"; locate the string in locales/en/plugin__kubevirt-plugin.json and either remove the duplicate key or point the UI code that displays the VM filter empty state to use the generic key instead (referenced keys: "No virtual machines match the filter criteria" and "No results match the filter criteria").src/views/vmnetworks/details/tabs/ConnectedProjects/connectedProjectsDefinition.tsx (1)
48-49: Use a stable row id only (avoid index fallback).Line [49] falls back to
unknown-project-${index}. Index-based ids are unstable across sort/filter updates and can cause row identity churn. SinceprojectNameis the stable domain key, this should stay key-only.♻️ Suggested change
-export const getConnectedProjectRowId = (row: ProjectWithVMCount, index: number): string => - row.projectName || `unknown-project-${index}`; +export const getConnectedProjectRowId = (row: ProjectWithVMCount): string => row.projectName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/vmnetworks/details/tabs/ConnectedProjects/connectedProjectsDefinition.tsx` around lines 48 - 49, The row id generator getConnectedProjectRowId currently falls back to an index-based id which is unstable; change it to return only the stable domain key by removing the index fallback so it always returns row.projectName (ensure ProjectWithVMCount.projectName is non-null or handle missing values upstream), i.e., make getConnectedProjectRowId rely solely on projectName instead of `unknown-project-${index}` to avoid identity churn on sort/filter updates.src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachines.tsx (1)
44-73: Consider adding a guard for empty VM list.When
vmListis empty (which can happen for bulk actions with no selection),vmList[0]isundefined. While this is safe in practice because the dropdown is disabled, adding an early return would make the intent clearer and prevent future bugs.🔧 Optional defensive improvement
const createActions = useCallback( (vmList: V1VirtualMachine[]): Action[] => { + if (vmList.length === 0) return []; + const isSingleVM = vmList.length === 1; const vm = vmList[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachines.tsx` around lines 44 - 73, createActions currently assumes vmList[0] exists which breaks when vmList is empty; add an early guard at the start of the createActions callback (check if vmList.length === 0) and return an empty Action[] immediately to avoid accessing vmList[0] or creating access reviews with undefined VM, leaving the rest of the logic (isSingleVM, vm variable, and modal cta creation) unchanged.src/utils/components/KubevirtTable/hooks/useTableSelection.ts (1)
70-76: Consider conventional "select all" behavior.Currently,
handleSelectAllclears selection if any items are selected (allSelected || someSelected). This means clicking "select all" when some rows are selected will deselect everything rather than completing the selection.This is a valid UX pattern (toggle), but many users expect "select all" to always select all, with a separate "clear" action. Consider whether this matches user expectations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/KubevirtTable/hooks/useTableSelection.ts` around lines 70 - 76, The current handleSelectAll toggles selection when any items are selected (using allSelected || someSelected), which causes a click during a partial selection to clear everything; change the logic in handleSelectAll so it only clears selection when allSelected is true, otherwise it should call onSelect(data) to select all rows; update the useCallback dependencies remain [allSelected, onSelect, data] and adjust any UI wording if you add a separate clear action.src/utils/components/KubevirtTable/KubevirtTable.tsx (1)
88-92: Potential render loop ifonSelectreference changes.The
useEffectdepends ononSelect, which could be a new function reference on each parent render if not memoized. While the conditionvalidSelectedItems.length !== selectedItems.lengthprovides a guard, consider addingonSelectto the dependency array warning or documenting that callers should memoizeonSelect.💡 Alternative: Use a ref to avoid onSelect in dependencies
+ const onSelectRef = React.useRef(onSelect); + onSelectRef.current = onSelect; + // Sync valid selection back to parent when orphaned items are detected React.useEffect(() => { if (isSelectable && validSelectedItems.length !== selectedItems.length) { - onSelect?.(validSelectedItems); + onSelectRef.current?.(validSelectedItems); } - }, [isSelectable, validSelectedItems, selectedItems.length, onSelect]); + }, [isSelectable, validSelectedItems, selectedItems.length]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/components/KubevirtTable/KubevirtTable.tsx` around lines 88 - 92, The effect in KubevirtTable (React.useEffect) currently depends on onSelect which may change identity each render and cause unnecessary re-runs; update the implementation to avoid using onSelect directly in the dependency array by storing the latest callback in a ref (e.g., onSelectRef.current = onSelect) and call onSelectRef.current(...) inside the effect, keeping dependencies to isSelectable, validSelectedItems, and selectedItems.length; alternatively, document that callers must memoize onSelect or wrap it with useCallback—reference the useEffect, isSelectable, validSelectedItems, selectedItems, and onSelect symbols to locate the change.
🤖 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`:
- Around line 1299-1300: Update the Spanish locale entries for the keys "No
connected projects found", "No connected virtual machines found", and "No
virtual machines match the filter criteria" to provide proper Spanish
translations (e.g., "No se encontraron proyectos conectados", "No se encontraron
máquinas virtuales conectadas", "Ninguna máquina virtual coincide con los
criterios del filtro") so they match the style of existing translated messages
like "No bootable volumes found" and "No Datapoints found".
In `@locales/fr/plugin__kubevirt-plugin.json`:
- Around line 479-481: Update the French localization values for the listed keys
so they are translated into French: replace "Connected projects table" with
"Tableau des projets connectés", "Connected virtual machines table" with
"Tableau des machines virtuelles connectées", "No connected projects found" with
"Aucun projet connecté trouvé", "No connected virtual machines found" with
"Aucune machine virtuelle connectée trouvée", and "No virtual machines match the
filter criteria" with "Aucune machine virtuelle ne correspond aux critères de
filtre"; ensure you update all occurrences of these keys (e.g., "Connected
projects table", "Connected virtual machines table", "No connected projects
found", "No connected virtual machines found", "No virtual machines match the
filter criteria") in the file so the English placeholder values are replaced
with the provided French strings.
In `@locales/ja/plugin__kubevirt-plugin.json`:
- Around line 464-466: Several localization keys still have English values;
update the JSON entries for the following keys to their Japanese translations:
"Connected projects table", "Connected virtual machines", "Connected virtual
machines table", "No connected projects found", "No connected virtual machines
found", and "No virtual machines match the filter criteria". Locate these keys
in the locales/ja JSON and replace the English strings with correct Japanese
text, preserving the existing key names and JSON formatting/encoding (UTF-8); if
you cannot provide final translations in this PR, mark them explicitly as TODO
or add a comment/placeholder to track translation work.
In `@locales/ko/plugin__kubevirt-plugin.json`:
- Around line 464-466: The new Korean locale entries are still in English;
update the values for the keys "Connected projects table", "Connected virtual
machines", and "Connected virtual machines table" (and the other added keys at
the same locations referenced in the review: the occurrences around lines
1277–1278 and 1344) to proper Korean translations so the UI is fully localized;
locate these keys in the ko locale JSON (entries shown in the diff) and replace
their English strings with the correct Korean text while preserving the JSON key
names and formatting.
In `@locales/zh/plugin__kubevirt-plugin.json`:
- Around line 464-466: Replace the untranslated English strings with Chinese
translations for the listed localization keys: change "Connected projects table"
to a proper zh translation, change "Connected virtual machines" to its zh
equivalent, and change "Connected virtual machines table" to its zh equivalent;
also find the same keys that appear later (the occurrences noted around lines
1277, 1278, and 1344) and update those values to the same Chinese translations
so the zh locale no longer contains English text (locate by the exact key
strings to apply the replacements).
In `@src/utils/components/KubevirtTable/hooks/useTableSelection.ts`:
- Around line 36-39: The validSelectedItems filter is using the selectedItems'
iteration index instead of the corresponding index in the original data, causing
wrong getRowId calls; change the filter to look up each selected item's index in
the data array before calling getRowId (e.g., use data.findIndex(d => d ===
item) to get the correct dataIndex, ensure idx !== -1, then check
dataIds.has(getRowId(item, idx))). Update the useMemo dependency list to include
data as well.
- Around line 26-29: selectedIds currently computes IDs using the position
inside selectedItems (selectedItems.map((item, index) => getRowId(item,
index))), which breaks when getRowId falls back to the index; change the
computation to derive each selected item's original data index before calling
getRowId: for each item in selectedItems find its index in data (e.g.,
data.findIndex(d => d === item or compare unique keys) or compute ID directly
from unique fields) and pass that original index to getRowId so selectedIds
matches the IDs computed from the data array; update references to
selectedIds/selectedItems/getRowId (and any getConnectedVMRowId callers)
accordingly.
In `@src/utils/hooks/useDataViewTableSort/utils.tsx`:
- Around line 30-37: The sanitizedRowId transformation can produce collisions
leading to duplicate DOM ids for the Checkbox; update the code that builds the
Checkbox props (the sanitizedRowId usage in the id and data-test for the
Checkbox component) to append a collision-resistant suffix derived from a unique
per-row value (e.g., the original rowId hashed or an index/UUID passed into the
renderer) so id and data-test become unique even when sanitizedRowId values
match; ensure you still sanitize the final string or encode it safely before
assigning to id/data-test to keep valid DOM id characters.
---
Nitpick comments:
In `@locales/en/plugin__kubevirt-plugin.json`:
- Line 1350: Replace the VM-specific i18n entry "No virtual machines match the
filter criteria" with a reuse of the existing generic message key "No results
match the filter criteria"; locate the string in
locales/en/plugin__kubevirt-plugin.json and either remove the duplicate key or
point the UI code that displays the VM filter empty state to use the generic key
instead (referenced keys: "No virtual machines match the filter criteria" and
"No results match the filter criteria").
In `@src/utils/components/KubevirtTable/hooks/useTableSelection.ts`:
- Around line 70-76: The current handleSelectAll toggles selection when any
items are selected (using allSelected || someSelected), which causes a click
during a partial selection to clear everything; change the logic in
handleSelectAll so it only clears selection when allSelected is true, otherwise
it should call onSelect(data) to select all rows; update the useCallback
dependencies remain [allSelected, onSelect, data] and adjust any UI wording if
you add a separate clear action.
In `@src/utils/components/KubevirtTable/KubevirtTable.tsx`:
- Around line 88-92: The effect in KubevirtTable (React.useEffect) currently
depends on onSelect which may change identity each render and cause unnecessary
re-runs; update the implementation to avoid using onSelect directly in the
dependency array by storing the latest callback in a ref (e.g.,
onSelectRef.current = onSelect) and call onSelectRef.current(...) inside the
effect, keeping dependencies to isSelectable, validSelectedItems, and
selectedItems.length; alternatively, document that callers must memoize onSelect
or wrap it with useCallback—reference the useEffect, isSelectable,
validSelectedItems, selectedItems, and onSelect symbols to locate the change.
In
`@src/views/vmnetworks/details/tabs/ConnectedProjects/connectedProjectsDefinition.tsx`:
- Around line 48-49: The row id generator getConnectedProjectRowId currently
falls back to an index-based id which is unstable; change it to return only the
stable domain key by removing the index fallback so it always returns
row.projectName (ensure ProjectWithVMCount.projectName is non-null or handle
missing values upstream), i.e., make getConnectedProjectRowId rely solely on
projectName instead of `unknown-project-${index}` to avoid identity churn on
sort/filter updates.
In
`@src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachines.tsx`:
- Around line 44-73: createActions currently assumes vmList[0] exists which
breaks when vmList is empty; add an early guard at the start of the
createActions callback (check if vmList.length === 0) and return an empty
Action[] immediately to avoid accessing vmList[0] or creating access reviews
with undefined VM, leaving the rest of the logic (isSingleVM, vm variable, and
modal cta creation) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ffe2f01-49d5-4c30-8847-db055a97b63a
📒 Files selected for processing (19)
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/KubevirtTable/KubevirtTable.tsxsrc/utils/components/KubevirtTable/hooks/useTableSelection.tssrc/utils/components/KubevirtTable/types.tssrc/utils/hooks/useDataViewTableSort/utils.tssrc/utils/hooks/useDataViewTableSort/utils.tsxsrc/views/vmnetworks/details/tabs/ConnectedProjects/ConnectedProjects.tsxsrc/views/vmnetworks/details/tabs/ConnectedProjects/ConnectedProjectsRow.tsxsrc/views/vmnetworks/details/tabs/ConnectedProjects/connectedProjectsDefinition.tsxsrc/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachines.tsxsrc/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachinesRow.tsxsrc/views/vmnetworks/details/tabs/ConnectedVirtualMachines/connectedVirtualMachinesDefinition.tsxsrc/views/vmnetworks/details/tabs/ConnectedVirtualMachines/hooks/useSelectedVMs.tssrc/views/vmnetworks/details/tabs/ConnectedVirtualMachines/hooks/useVirtualMachineColumns.ts
💤 Files with no reviewable changes (5)
- src/utils/hooks/useDataViewTableSort/utils.ts
- src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/hooks/useVirtualMachineColumns.ts
- src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/hooks/useSelectedVMs.ts
- src/views/vmnetworks/details/tabs/ConnectedProjects/ConnectedProjectsRow.tsx
- src/views/vmnetworks/details/tabs/ConnectedVirtualMachines/ConnectedVirtualMachinesRow.tsx
Comment on lines
+1299
to
+1300
| "No connected projects found": "No connected projects found", | ||
| "No connected virtual machines found": "No connected virtual machines found", |
There was a problem hiding this comment.
Missing Spanish translations for empty-state messages.
These new keys have English values in the Spanish locale, but similar "No X found" messages in this file are translated:
- "No bootable volumes found" → "No se encontraron volúmenes de arranque"
- "No Datapoints found" → "No se encontraron puntos de datos"
- "No Results Match the Filter Criteria" → "Ningún resultado coincide con los criterios de filtro"
For consistency, consider translating:
- "No connected projects found" → "No se encontraron proyectos conectados"
- "No connected virtual machines found" → "No se encontraron máquinas virtuales conectadas"
- "No virtual machines match the filter criteria" → "Ninguna máquina virtual coincide con los criterios del filtro"
🌐 Suggested translations
- "No connected projects found": "No connected projects found",
- "No connected virtual machines found": "No connected virtual machines found",
+ "No connected projects found": "No se encontraron proyectos conectados",
+ "No connected virtual machines found": "No se encontraron máquinas virtuales conectadas",- "No virtual machines match the filter criteria": "No virtual machines match the filter criteria",
+ "No virtual machines match the filter criteria": "Ninguna máquina virtual coincide con los criterios del filtro",Also applies to: 1366-1366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/es/plugin__kubevirt-plugin.json` around lines 1299 - 1300, Update the
Spanish locale entries for the keys "No connected projects found", "No connected
virtual machines found", and "No virtual machines match the filter criteria" to
provide proper Spanish translations (e.g., "No se encontraron proyectos
conectados", "No se encontraron máquinas virtuales conectadas", "Ninguna máquina
virtual coincide con los criterios del filtro") so they match the style of
existing translated messages like "No bootable volumes found" and "No Datapoints
found".
Comment on lines
+479
to
+481
| "Connected projects table": "Connected projects table", | ||
| "Connected virtual machines": "Connected virtual machines", | ||
| "Connected virtual machines table": "Connected virtual machines table", |
There was a problem hiding this comment.
Missing French translations for new localization keys.
The new keys have English text as their French translation values. These should be translated to French for proper localization:
| Key | Current Value | Suggested French Translation |
|---|---|---|
Connected projects table |
Connected projects table |
Tableau des projets connectés |
Connected virtual machines table |
Connected virtual machines table |
Tableau des machines virtuelles connectées |
No connected projects found |
No connected projects found |
Aucun projet connecté trouvé |
No connected virtual machines found |
No connected virtual machines found |
Aucune machine virtuelle connectée trouvée |
No virtual machines match the filter criteria |
No virtual machines match the filter criteria |
Aucune machine virtuelle ne correspond aux critères de filtre |
Also applies to: 1299-1300, 1366-1366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/fr/plugin__kubevirt-plugin.json` around lines 479 - 481, Update the
French localization values for the listed keys so they are translated into
French: replace "Connected projects table" with "Tableau des projets connectés",
"Connected virtual machines table" with "Tableau des machines virtuelles
connectées", "No connected projects found" with "Aucun projet connecté trouvé",
"No connected virtual machines found" with "Aucune machine virtuelle connectée
trouvée", and "No virtual machines match the filter criteria" with "Aucune
machine virtuelle ne correspond aux critères de filtre"; ensure you update all
occurrences of these keys (e.g., "Connected projects table", "Connected virtual
machines table", "No connected projects found", "No connected virtual machines
found", "No virtual machines match the filter criteria") in the file so the
English placeholder values are replaced with the provided French strings.
Comment on lines
+464
to
+466
| "Connected projects table": "Connected projects table", | ||
| "Connected virtual machines": "Connected virtual machines", | ||
| "Connected virtual machines table": "Connected virtual machines table", |
There was a problem hiding this comment.
Missing Japanese translations for new localization keys.
The newly added keys have English values instead of Japanese translations:
- Line 464:
"Connected projects table"→ should be Japanese - Line 466:
"Connected virtual machines table"→ should be Japanese - Line 1277:
"No connected projects found"→ should be Japanese - Line 1278:
"No connected virtual machines found"→ should be Japanese - Line 1344:
"No virtual machines match the filter criteria"→ should be Japanese
Other entries in this file follow the pattern of providing Japanese translations (e.g., "Activity": "アクティビティー"). If these are placeholders awaiting translation, consider adding them in this PR or tracking the translation work separately.
Also applies to: 1277-1278, 1344-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/ja/plugin__kubevirt-plugin.json` around lines 464 - 466, Several
localization keys still have English values; update the JSON entries for the
following keys to their Japanese translations: "Connected projects table",
"Connected virtual machines", "Connected virtual machines table", "No connected
projects found", "No connected virtual machines found", and "No virtual machines
match the filter criteria". Locate these keys in the locales/ja JSON and replace
the English strings with correct Japanese text, preserving the existing key
names and JSON formatting/encoding (UTF-8); if you cannot provide final
translations in this PR, mark them explicitly as TODO or add a
comment/placeholder to track translation work.
Comment on lines
+464
to
+466
| "Connected projects table": "Connected projects table", | ||
| "Connected virtual machines": "Connected virtual machines", | ||
| "Connected virtual machines table": "Connected virtual machines table", |
There was a problem hiding this comment.
New ko keys are still English.
Line [464], Line [466], Line [1277], Line [1278], and Line [1344] were added for new table/empty states but remain untranslated, so Korean locale users will see mixed-language UI.
🌐 Suggested ko translations
- "Connected projects table": "Connected projects table",
+ "Connected projects table": "연결된 프로젝트 테이블",
- "Connected virtual machines table": "Connected virtual machines table",
+ "Connected virtual machines table": "연결된 가상 머신 테이블",
- "No connected projects found": "No connected projects found",
+ "No connected projects found": "연결된 프로젝트를 찾을 수 없습니다",
- "No connected virtual machines found": "No connected virtual machines found",
+ "No connected virtual machines found": "연결된 가상 머신을 찾을 수 없습니다",
- "No virtual machines match the filter criteria": "No virtual machines match the filter criteria",
+ "No virtual machines match the filter criteria": "필터 기준과 일치하는 가상 머신이 없습니다",Also applies to: 1277-1278, 1344-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/ko/plugin__kubevirt-plugin.json` around lines 464 - 466, The new
Korean locale entries are still in English; update the values for the keys
"Connected projects table", "Connected virtual machines", and "Connected virtual
machines table" (and the other added keys at the same locations referenced in
the review: the occurrences around lines 1277–1278 and 1344) to proper Korean
translations so the UI is fully localized; locate these keys in the ko locale
JSON (entries shown in the diff) and replace their English strings with the
correct Korean text while preserving the JSON key names and formatting.
Comment on lines
+464
to
+466
| "Connected projects table": "Connected projects table", | ||
| "Connected virtual machines": "Connected virtual machines", | ||
| "Connected virtual machines table": "Connected virtual machines table", |
There was a problem hiding this comment.
Localize new zh keys instead of shipping English values.
Line [464], Line [466], Line [1277], Line [1278], and Line [1344] are newly added but still in English, so the zh UI will render untranslated copy for these new states.
🌐 Suggested zh translations
- "Connected projects table": "Connected projects table",
+ "Connected projects table": "已连接项目表",
- "Connected virtual machines table": "Connected virtual machines table",
+ "Connected virtual machines table": "已连接虚拟机表",
- "No connected projects found": "No connected projects found",
+ "No connected projects found": "未找到已连接项目",
- "No connected virtual machines found": "No connected virtual machines found",
+ "No connected virtual machines found": "未找到已连接虚拟机",
- "No virtual machines match the filter criteria": "No virtual machines match the filter criteria",
+ "No virtual machines match the filter criteria": "没有虚拟机符合筛选条件",Also applies to: 1277-1278, 1344-1344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/zh/plugin__kubevirt-plugin.json` around lines 464 - 466, Replace the
untranslated English strings with Chinese translations for the listed
localization keys: change "Connected projects table" to a proper zh translation,
change "Connected virtual machines" to its zh equivalent, and change "Connected
virtual machines table" to its zh equivalent; also find the same keys that
appear later (the occurrences noted around lines 1277, 1278, and 1344) and
update those values to the same Chinese translations so the zh locale no longer
contains English text (locate by the exact key strings to apply the
replacements).
Comment on lines
+26
to
+29
| const selectedIds = useMemo( | ||
| () => new Set(selectedItems.map((item, index) => getRowId(item, index))), | ||
| [selectedItems, getRowId], | ||
| ); |
There was a problem hiding this comment.
Index mismatch in getRowId calls may cause selection bugs.
When computing selectedIds, the index passed to getRowId is the position within selectedItems, not the original position in data. If getRowId uses the index as a fallback (e.g., getConnectedVMRowId falls back to namespace-name-${index} when UID is missing), selected rows won't match their original IDs.
Example: Select items at data indices 1 and 2. In selectedItems, they become indices 0 and 1, producing different IDs than in data.
🐛 Proposed fix: Track original indices or avoid index-dependent IDs
Option 1: Store selection by ID string instead of by item reference:
-export type UseTableSelectionProps<TData> = {
- data: TData[];
- getRowId: (row: TData, index: number) => string;
- onSelect: (selected: TData[]) => void;
- selectedItems: TData[];
-};
+export type UseTableSelectionProps<TData> = {
+ data: TData[];
+ getRowId: (row: TData, index: number) => string;
+ onSelect: (selectedIds: Set<string>) => void;
+ selectedIds: Set<string>;
+};Option 2: Find the item's original index in data when computing IDs:
const selectedIds = useMemo(
- () => new Set(selectedItems.map((item, index) => getRowId(item, index))),
+ () => new Set(selectedItems.map((item) => {
+ const originalIndex = data.findIndex((d, i) => getRowId(d, i) === getRowId(item, data.indexOf(item)));
+ return getRowId(item, originalIndex !== -1 ? originalIndex : 0);
+ })),
[selectedItems, getRowId],
);Option 3 (simplest): Ensure all getRowId implementations don't rely on index (use UID or unique properties only).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/components/KubevirtTable/hooks/useTableSelection.ts` around lines
26 - 29, selectedIds currently computes IDs using the position inside
selectedItems (selectedItems.map((item, index) => getRowId(item, index))), which
breaks when getRowId falls back to the index; change the computation to derive
each selected item's original data index before calling getRowId: for each item
in selectedItems find its index in data (e.g., data.findIndex(d => d === item or
compare unique keys) or compute ID directly from unique fields) and pass that
original index to getRowId so selectedIds matches the IDs computed from the data
array; update references to selectedIds/selectedItems/getRowId (and any
getConnectedVMRowId callers) accordingly.
Comment on lines
+36
to
+39
| const validSelectedItems = useMemo( | ||
| () => selectedItems.filter((item, index) => dataIds.has(getRowId(item, index))), | ||
| [selectedItems, dataIds, getRowId], | ||
| ); |
There was a problem hiding this comment.
Same index mismatch applies to validSelectedItems computation.
The filter at Line 37 also passes the selectedItems index rather than the data index, compounding the issue noted above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/components/KubevirtTable/hooks/useTableSelection.ts` around lines
36 - 39, The validSelectedItems filter is using the selectedItems' iteration
index instead of the corresponding index in the original data, causing wrong
getRowId calls; change the filter to look up each selected item's index in the
data array before calling getRowId (e.g., use data.findIndex(d => d === item) to
get the correct dataIndex, ensure idx !== -1, then check
dataIds.has(getRowId(item, idx))). Update the useMemo dependency list to include
data as well.
Comment on lines
+30
to
+37
| const sanitizedRowId = rowId.replace(/[^a-zA-Z0-9-_]/g, '-'); | ||
| const selectionCell = { | ||
| cell: ( | ||
| <Checkbox | ||
| aria-label={`Select row ${rowId}`} | ||
| data-test={`select-row-${sanitizedRowId}`} | ||
| id={`select-row-${sanitizedRowId}`} | ||
| isChecked={isSelected} |
There was a problem hiding this comment.
Sanitized checkbox IDs can collide across distinct row IDs.
Line [30] normalizes many characters to -, so different rowId values can generate the same id/data-test on Line [35]-Line [36]. This creates duplicate DOM IDs.
🛠️ Suggested fix
- const sanitizedRowId = rowId.replace(/[^a-zA-Z0-9-_]/g, '-');
+ const sanitizedRowId = `${rowId.replace(/[^a-zA-Z0-9-_]/g, '-')}-${index}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/hooks/useDataViewTableSort/utils.tsx` around lines 30 - 37, The
sanitizedRowId transformation can produce collisions leading to duplicate DOM
ids for the Checkbox; update the code that builds the Checkbox props (the
sanitizedRowId usage in the id and data-test for the Checkbox component) to
append a collision-resistant suffix derived from a unique per-row value (e.g.,
the original rowId hashed or an index/UUID passed into the renderer) so id and
data-test become unique even when sanitizedRowId values match; ensure you still
sanitize the final string or encode it safely before assigning to id/data-test
to keep valid DOM id characters.
Contributor
|
@batyana: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
2 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
useTableSelectionhookChanges
KubevirtTable Enhancements
selectable={true}, TypeScript now requiresgetRowId,onSelect, andselectedItemsprops (discriminated union types)useTableSelectionhook: Manages selection state, handles select all/none, and automatically removes orphaned selections when data changesgenerateRows: Single function handles both selectable and non-selectable tables (backward compatible)Tables Migrated
Files
types.ts,useTableSelection.ts,connectedVirtualMachinesDefinition.tsx,connectedProjectsDefinition.tsxKubevirtTable.tsx,utils.tsx(renamed from.ts)ConnectedVirtualMachinesRow.tsx,ConnectedProjectsRow.tsx,useSelectedVMs.ts,useVirtualMachineColumns.ts🎥 Demo
Summary by CodeRabbit
New Features
Documentation