CNV-74280: Migrate checkups tables to KubevirtTable#3587
CNV-74280: Migrate checkups tables to KubevirtTable#3587batyana wants to merge 1 commit intokubevirt-ui:mainfrom
Conversation
|
@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. |
|
Skipping CI for Draft Pull Request. |
|
[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 |
📝 WalkthroughWalkthroughRefactors checkups tables from row-based VirtualizedTable to a column/cell-driven KubevirtTable architecture, adds dynamic column management, per-column sort handlers, pagination-with-filters, new cell/definition modules, and adds localization keys for new table labels and messages. 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 |
5394f17 to
0c26327
Compare
|
@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. |
|
@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: 8
🧹 Nitpick comments (1)
src/views/checkups/checkupsDetailsPageHistoryDefinition.tsx (1)
62-68: Consider using numeric values for status sorting.The
getValuereturns'1'or'0'as strings. While this works for basic sorting, returning numeric values (1or0) would be more semantically appropriate and consistent with typical sorting patterns.{ - getValue: (row) => (row?.status?.succeeded ? '1' : '0'), + getValue: (row) => (row?.status?.succeeded ? 1 : 0), key: 'status', label: t('Status'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/checkups/checkupsDetailsPageHistoryDefinition.tsx` around lines 62 - 68, The getValue for the status column currently returns string '1' or '0'; change it to return numeric values 1 or 0 so sorting is numeric and semantically correct—update the getValue implementation on the column with key 'status' (the same entry that uses renderCell: (row) => <StatusCell row={row} />) to yield 1 when row?.status?.succeeded is truthy and 0 otherwise.
🤖 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 386: Several new Spanish locale entries were added but left with English
values (e.g., the key with value "Checkups history table" plus four other new
keys); update those values to proper Spanish translations without changing the
locale keys. Open the locales/es/plugin__kubevirt-plugin.json file, locate the
newly added keys (the entries added around the recent diff) and replace the
English strings with idiomatic Spanish equivalents (preserve quotes, commas and
key names exactly), verify accents/grammar, and run any i18n lint/validation you
have to ensure JSON and locale consistency.
In `@locales/fr/plugin__kubevirt-plugin.json`:
- Line 386: Replace the untranslated English strings with French translations
for the specified keys: update "Checkups history table" → "Tableau de
l'historique des vérifications", "No checkup runs yet" → "Aucune vérification
effectuée pour le moment", "No results match the current filters" → "Aucun
résultat ne correspond aux filtres actuels", "Self validation checkups table" →
"Tableau des vérifications d'auto-validation", and "Storage checkups table" →
"Tableau des vérifications du stockage"; ensure you edit the corresponding JSON
entries (the exact keys shown) so the French strings replace the English ones.
In `@locales/ja/plugin__kubevirt-plugin.json`:
- Line 371: Translate the untranslated English literals in the ja locale JSON by
replacing the English values (e.g. "Checkups history table") with their proper
Japanese translations; locate the entries by searching for the exact English
strings (like "Checkups history table") and the other untranslated
checkups-related keys referenced in the review (the entries at the other
occurrences) and update their values with accurate Japanese text so the ja
locale no longer contains English placeholders.
In `@locales/ko/plugin__kubevirt-plugin.json`:
- Line 371: The Korean locale contains untranslated English strings—e.g., the
key "Checkups history table" in the ko plugin JSON—so update the value to a
proper Korean translation and do the same for the other four newly added English
values in the same locales/ko/plugin__kubevirt-plugin.json file (the entries
around the other added keys). Keep the key names unchanged, replace only the
English value strings with Korean translations, preserve JSON syntax (quotes,
commas, encoding), and run a quick lint/validate to ensure the JSON remains
valid after edits.
In `@locales/zh/plugin__kubevirt-plugin.json`:
- Line 371: Several new localization keys in
locales/zh/plugin__kubevirt-plugin.json remain in English (e.g., "Checkups
history table" and the other entries referenced at the review), breaking Chinese
translations; update each of these keys in the zh JSON to their proper Chinese
translations (replace the English values with Chinese strings) while preserving
the exact key names, ensuring entries like "Checkups history table" and the ones
at the other mentioned locations are translated consistently and validated as
valid JSON.
In `@src/views/checkups/self-validation/list/CheckupsSelfValidationList.tsx`:
- Around line 102-112: The onPaginationChange function returned by the
usePagination hook should be memoized to avoid triggering effects that list it
as a dependency; update the usePagination implementation (the onPaginationChange
returned from the hook) to wrap the setter call in useCallback (e.g., define
const onPaginationChange = useCallback((newPagination: PaginationState) =>
setPagination(curr => ({...curr, ...newPagination})), [])) so the function
identity is stable across renders and continue returning it from the hook.
In
`@src/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsx`:
- Around line 81-91: The code uses Array.prototype.toSorted() on variable data
which is an ES2023 feature not supported by the project's tsconfig target;
replace the toSorted(...) call with a non‑stage feature by creating a shallow
copy and using sort, e.g. copy data (e.g. [...data] or Array.from(data]) and
call .sort(...) with the same comparator that references jobsByConfigMapName,
getJobStatus, getConfigMapStatus, statusOrder and sortDirection (including
SortByDirection.asc), or alternatively update tsconfig target to es2023 or add a
runtime polyfill—pick the approach (prefer replacing toSorted with a copy +
.sort) and apply it to the code around the data.toSorted(...) expression.
In `@src/views/checkups/storage/list/CheckupsStorageList.tsx`:
- Around line 145-170: Pagination controls update local pagination state
(page/perPage) but the table still renders filteredData unpaged, so page changes
don't affect visible rows; because sorting now happens inside KubevirtTable, you
must apply pagination after sorting. Fix by either (A) adding pagination props
to KubevirtTable (e.g., page and perPage) and have KubevirtTable slice the
sorted data internally before rendering, or (B) add a callback from
KubevirtTable (e.g., onSortedData) that returns the post-sort array and then
compute a pagedData = sorted.slice((page-1)*perPage, page*perPage) and pass
pagedData as the data prop; update the current usage (where Pagination,
filteredData, pagination, handlePerPageSelect, handleSetPage, and KubevirtTable
are referenced) to pass the pagination state or consume the onSortedData
callback accordingly.
---
Nitpick comments:
In `@src/views/checkups/checkupsDetailsPageHistoryDefinition.tsx`:
- Around line 62-68: The getValue for the status column currently returns string
'1' or '0'; change it to return numeric values 1 or 0 so sorting is numeric and
semantically correct—update the getValue implementation on the column with key
'status' (the same entry that uses renderCell: (row) => <StatusCell row={row}
/>) to yield 1 when row?.status?.succeeded is truthy and 0 otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6fd6301-c037-4c05-88d5-e6732dbfdc5d
📒 Files selected for processing (29)
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/hooks/useDataViewTableSort/types.tssrc/utils/hooks/useDataViewTableSort/useDataViewTableSort.tssrc/utils/hooks/useKubevirtUserSettings/useKubevirtTableColumns.tssrc/views/checkups/CheckupsDetailsPageHistory.tsxsrc/views/checkups/CheckupsDetailsPageHistoryRow.tsxsrc/views/checkups/CheckupsStatusIcon.tsxsrc/views/checkups/checkupsDetailsPageHistoryDefinition.tsxsrc/views/checkups/components/cells/CheckupsSharedCells.tsxsrc/views/checkups/hooks/useCheckupsDetailsPageHistoryColumns.tssrc/views/checkups/self-validation/components/hooks/useCheckupsSelfValidationListColumns.tssrc/views/checkups/self-validation/details/tabs/details/CheckupsSelfValidationDetailsTab.tsxsrc/views/checkups/self-validation/list/CheckupsSelfValidationList.tsxsrc/views/checkups/self-validation/list/CheckupsSelfValidationListRow.tsxsrc/views/checkups/self-validation/list/checkupsSelfValidationCells.tsxsrc/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsxsrc/views/checkups/storage/components/hooks/useCheckupsStorageData.tssrc/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.tssrc/views/checkups/storage/details/tabs/details/CheckupsStorageDetailsTab.tsxsrc/views/checkups/storage/list/CheckupsStorageList.tsxsrc/views/checkups/storage/list/CheckupsStorageListRow.tsxsrc/views/checkups/storage/list/checkupsStorageCells.tsxsrc/views/checkups/storage/list/checkupsStorageListDefinition.tsx
💤 Files with no reviewable changes (6)
- src/views/checkups/hooks/useCheckupsDetailsPageHistoryColumns.ts
- src/views/checkups/self-validation/list/CheckupsSelfValidationListRow.tsx
- src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts
- src/views/checkups/self-validation/components/hooks/useCheckupsSelfValidationListColumns.ts
- src/views/checkups/storage/list/CheckupsStorageListRow.tsx
- src/views/checkups/CheckupsDetailsPageHistoryRow.tsx
| "Checkup not found": "No se encontró el control", | ||
| "Checkup progress": "Progreso del control", | ||
| "Checkups": "Controles", | ||
| "Checkups history table": "Checkups history table", |
There was a problem hiding this comment.
Translate new es locale entries instead of shipping English fallback text.
Lines 386, 1295, 1345, 1734, and 1845 add keys in the Spanish bundle but keep English values, creating inconsistent localized UX.
🌐 Suggested translation patch
- "Checkups history table": "Checkups history table",
+ "Checkups history table": "Tabla de historial de controles",
- "No checkup runs yet": "No checkup runs yet",
+ "No checkup runs yet": "Todavía no hay ejecuciones de controles",
- "No results match the current filters": "No results match the current filters",
+ "No results match the current filters": "No hay resultados que coincidan con los filtros actuales",
- "Self validation checkups table": "Self validation checkups table",
+ "Self validation checkups table": "Tabla de controles de autovalidación",
- "Storage checkups table": "Storage checkups table",
+ "Storage checkups table": "Tabla de controles de almacenamiento",Also applies to: 1295-1295, 1345-1345, 1734-1734, 1845-1845
🤖 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 386, Several new Spanish
locale entries were added but left with English values (e.g., the key with value
"Checkups history table" plus four other new keys); update those values to
proper Spanish translations without changing the locale keys. Open the
locales/es/plugin__kubevirt-plugin.json file, locate the newly added keys (the
entries added around the recent diff) and replace the English strings with
idiomatic Spanish equivalents (preserve quotes, commas and key names exactly),
verify accents/grammar, and run any i18n lint/validation you have to ensure JSON
and locale consistency.
| "Checkup not found": "Vérification introuvable", | ||
| "Checkup progress": "État d'avancement de la vérification", | ||
| "Checkups": "Vérifications", | ||
| "Checkups history table": "Checkups history table", |
There was a problem hiding this comment.
French translations missing for new keys.
The following new keys are not translated to French—they just contain the English strings:
- Line 386:
"Checkups history table" - Line 1295:
"No checkup runs yet" - Line 1345:
"No results match the current filters" - Line 1734:
"Self validation checkups table" - Line 1845:
"Storage checkups table"
These should be translated to proper French text (e.g., "Tableau de l'historique des vérifications", "Aucune vérification exécutée", etc.) to provide a consistent localized experience for French-speaking users.
Also applies to: 1295-1295, 1345-1345, 1734-1734, 1845-1845
🤖 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 386, Replace the
untranslated English strings with French translations for the specified keys:
update "Checkups history table" → "Tableau de l'historique des vérifications",
"No checkup runs yet" → "Aucune vérification effectuée pour le moment", "No
results match the current filters" → "Aucun résultat ne correspond aux filtres
actuels", "Self validation checkups table" → "Tableau des vérifications
d'auto-validation", and "Storage checkups table" → "Tableau des vérifications du
stockage"; ensure you edit the corresponding JSON entries (the exact keys shown)
so the French strings replace the English ones.
| "Checkup not found": "チェックアップが見つかりません", | ||
| "Checkup progress": "チェックアップの進捗", | ||
| "Checkups": "チェックアップ", | ||
| "Checkups history table": "Checkups history table", |
There was a problem hiding this comment.
Translate the new Japanese strings before merging.
These entries are still English literals, so the ja locale will show untranslated copy for the new checkups table labels and empty states.
Also applies to: 1273-1273, 1323-1323, 1710-1710, 1821-1821
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/ja/plugin__kubevirt-plugin.json` at line 371, Translate the
untranslated English literals in the ja locale JSON by replacing the English
values (e.g. "Checkups history table") with their proper Japanese translations;
locate the entries by searching for the exact English strings (like "Checkups
history table") and the other untranslated checkups-related keys referenced in
the review (the entries at the other occurrences) and update their values with
accurate Japanese text so the ja locale no longer contains English placeholders.
| "Checkup not found": "점검을 찾을 수 없음", | ||
| "Checkup progress": "검사 진행 상황", | ||
| "Checkups": "검사", | ||
| "Checkups history table": "Checkups history table", |
There was a problem hiding this comment.
New Korean locale entries are still in English.
These five added keys in locales/ko are untranslated, so Korean users will see mixed-language table empty states/labels.
🌐 Suggested localization update
- "Checkups history table": "Checkups history table",
+ "Checkups history table": "점검 기록 테이블",
- "No checkup runs yet": "No checkup runs yet",
+ "No checkup runs yet": "아직 점검 실행 내역이 없습니다",
- "No results match the current filters": "No results match the current filters",
+ "No results match the current filters": "현재 필터와 일치하는 결과가 없습니다",
- "Self validation checkups table": "Self validation checkups table",
+ "Self validation checkups table": "자체 검증 점검 테이블",
- "Storage checkups table": "Storage checkups table",
+ "Storage checkups table": "스토리지 점검 테이블",Also applies to: 1273-1273, 1323-1323, 1710-1710, 1821-1821
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@locales/ko/plugin__kubevirt-plugin.json` at line 371, The Korean locale
contains untranslated English strings—e.g., the key "Checkups history table" in
the ko plugin JSON—so update the value to a proper Korean translation and do the
same for the other four newly added English values in the same
locales/ko/plugin__kubevirt-plugin.json file (the entries around the other added
keys). Keep the key names unchanged, replace only the English value strings with
Korean translations, preserve JSON syntax (quotes, commas, encoding), and run a
quick lint/validate to ensure the JSON remains valid after edits.
| "Checkup not found": "未找到检查", | ||
| "Checkup progress": "检查进行中", | ||
| "Checkups": "检查", | ||
| "Checkups history table": "Checkups history table", |
There was a problem hiding this comment.
New Chinese locale entries are untranslated.
These new keys in locales/zh currently keep English values, which breaks localization consistency in the checkups tables/empty states.
🌐 Suggested localization update
- "Checkups history table": "Checkups history table",
+ "Checkups history table": "检查历史表",
- "No checkup runs yet": "No checkup runs yet",
+ "No checkup runs yet": "尚无检查运行记录",
- "No results match the current filters": "No results match the current filters",
+ "No results match the current filters": "没有与当前筛选条件匹配的结果",
- "Self validation checkups table": "Self validation checkups table",
+ "Self validation checkups table": "自检检查表",
- "Storage checkups table": "Storage checkups table",
+ "Storage checkups table": "存储检查表",Also applies to: 1273-1273, 1323-1323, 1710-1710, 1821-1821
🤖 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 371, Several new
localization keys in locales/zh/plugin__kubevirt-plugin.json remain in English
(e.g., "Checkups history table" and the other entries referenced at the review),
breaking Chinese translations; update each of these keys in the zh JSON to their
proper Chinese translations (replace the English values with Chinese strings)
while preserving the exact key names, ensuring entries like "Checkups history
table" and the ones at the other mentioned locations are translated consistently
and validated as valid JSON.
src/views/checkups/self-validation/list/CheckupsSelfValidationList.tsx
Outdated
Show resolved
Hide resolved
| return data.toSorted((a, b) => { | ||
| const jobA = jobsByConfigMapName.get(a?.metadata?.name)?.[0]; | ||
| const jobB = jobsByConfigMapName.get(b?.metadata?.name)?.[0]; | ||
| const statusA = getConfigMapStatus(a, getJobStatus(jobA)); | ||
| const statusB = getConfigMapStatus(b, getJobStatus(jobB)); | ||
|
|
||
| const orderA = statusOrder[statusA] || 999; | ||
| const orderB = statusOrder[statusB] || 999; | ||
|
|
||
| return sortDirection === SortByDirection.asc ? orderA - orderB : orderB - orderA; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What browsers support Array.prototype.toSorted() ES2023?
💡 Result:
Array.prototype.toSorted() (ES2023) is supported in these browsers (minimum versions):
- Chrome: 110+ [1]
- Edge: 110+ [1]
- Firefox: 115+ [1]
- Safari (macOS): 16.0+ [1]
- Safari (iOS): 16.0+ [1]
- Opera: 96+ [1]
- Internet Explorer: not supported [1]
MDN also notes it’s been available across major browsers since July 2023. [2]
Sources: [1] Can I use: “Array: toSorted” https://caniuse.com/mdn-javascript_builtins_array_tosorted, [2] MDN: Array.prototype.toSorted() https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/toSorted
🏁 Script executed:
# Check for TypeScript configuration
if [ -f "tsconfig.json" ]; then
echo "=== tsconfig.json ===" && cat tsconfig.json
fi
# Check for browser targets
if [ -f ".browserslistrc" ]; then
echo "=== .browserslistrc ===" && cat .browserslistrc
fi
# Check package.json for Node/browser constraints
if [ -f "package.json" ]; then
echo "=== package.json (engines, browserslist) ===" && jq '.engines, .browserslist' package.json 2>/dev/null || cat package.json | grep -A 5 -E '"engines"|"browserslist"'
fi
# Check for babel configuration
if [ -f ".babelrc" ] || [ -f "babel.config.js" ] || [ -f "babel.config.json" ]; then
echo "=== Babel config ===" && cat .babelrc babel.config.js babel.config.json 2>/dev/null
fiRepository: kubevirt-ui/kubevirt-plugin
Length of output: 1340
Array.prototype.toSorted() target mismatch with TypeScript configuration.
Your tsconfig.json targets es2021, but this code uses toSorted() from ES2023. This feature will not be transpiled by your build system and will cause runtime errors in environments that don't support it (e.g., older browsers, older Node versions despite your >=22.0.0 requirement for the dev environment). Either update the TypeScript target to es2023, add a polyfill, or replace toSorted() with sort() on a copy of the array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsx`
around lines 81 - 91, The code uses Array.prototype.toSorted() on variable data
which is an ES2023 feature not supported by the project's tsconfig target;
replace the toSorted(...) call with a non‑stage feature by creating a shallow
copy and using sort, e.g. copy data (e.g. [...data] or Array.from(data]) and
call .sort(...) with the same comparator that references jobsByConfigMapName,
getJobStatus, getConfigMapStatus, statusOrder and sortDirection (including
SortByDirection.asc), or alternatively update tsconfig target to es2023 or add a
runtime polyfill—pick the approach (prefer replacing toSorted with a copy +
.sort) and apply it to the code around the data.toSorted(...) expression.
Signed-off-by: batyana <bnahmias@redhat.com>
0c26327 to
752712d
Compare
|
@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)
locales/fr/plugin__kubevirt-plugin.json (1)
386-386:⚠️ Potential issue | 🟡 MinorTranslate the new checkups strings to French.
These entries are still English in the French locale (Line 386, Line 1295, Line 1733, Line 1844), which causes mixed-language UI text.
💬 Suggested translations
- "Checkups history table": "Checkups history table", + "Checkups history table": "Tableau de l'historique des vérifications", - "No checkup runs yet": "No checkup runs yet", + "No checkup runs yet": "Aucune vérification effectuée pour le moment", - "Self validation checkups table": "Self validation checkups table", + "Self validation checkups table": "Tableau des vérifications d'auto-validation", - "Storage checkups table": "Storage checkups table", + "Storage checkups table": "Tableau des vérifications du stockage",Also applies to: 1295-1295, 1733-1733, 1844-1844
🤖 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 386, The French locale contains untranslated "checkups" strings — replace the English value for the key "Checkups history table" with a French translation and do the same for the three other untranslated checkups entries referenced in the comment (lines 1295, 1733, 1844); specifically, update "Checkups history table" → "Tableau de l'historique des contrôles" and similarly translate the other checkups values (suggested: "Checkups" → "Contrôles", "Checkup details" → "Détails du contrôle", "Run checkup" → "Exécuter un contrôle") so the UI is fully in French.
🧹 Nitpick comments (2)
src/utils/hooks/usePagination/usePaginationWithFilters.ts (1)
34-45: Extract duplicated “reset to first page” logic.The same pagination reset object is repeated in two places. A small helper will reduce drift risk.
♻️ Proposed refactor
+ const resetToFirstPage = useCallback( + (perPage: number) => + onPaginationChange({ + endIndex: perPage, + page: 1, + perPage, + startIndex: 0, + }), + [onPaginationChange], + ); + const handleFilterChange = useCallback( (...args: Parameters<OnFilterChange>) => { onFilterChange(...args); - onPaginationChange({ - endIndex: pagination?.perPage, - page: 1, - perPage: pagination?.perPage, - startIndex: 0, - }); + resetToFirstPage(pagination.perPage); }, - [onFilterChange, onPaginationChange, pagination?.perPage], + [onFilterChange, resetToFirstPage, pagination.perPage], ); @@ useEffect(() => { if (filteredDataLength > 0 && pagination?.startIndex >= filteredDataLength) { - onPaginationChange({ - endIndex: pagination?.perPage, - page: 1, - perPage: pagination?.perPage, - startIndex: 0, - }); + resetToFirstPage(pagination.perPage); } - }, [filteredDataLength, pagination?.startIndex, pagination?.perPage, onPaginationChange]); + }, [filteredDataLength, pagination.startIndex, pagination.perPage, resetToFirstPage]);Also applies to: 59-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/hooks/usePagination/usePaginationWithFilters.ts` around lines 34 - 45, The pagination-reset object used in handleFilterChange (and the similar block at lines 59-68) is duplicated; extract a small helper function (e.g., getResetPagination or makeResetPagination) inside usePaginationWithFilters.ts that returns { endIndex: pagination?.perPage, page: 1, perPage: pagination?.perPage, startIndex: 0 }, and replace the inline objects in handleFilterChange and the other handler(s) to call that helper and pass its result into onPaginationChange; ensure the helper references the same pagination variable used by the hooks so semantics remain identical.src/views/checkups/self-validation/list/CheckupsSelfValidationList.tsx (1)
57-63: BackgetJobByNamewith a precomputed lookup.
getJobByName(jobs, ...)re-filters and re-sorts the full jobs array on every call, and the current cells hit this callback multiple times per row. Building a map once here would keep those render-time lookups O(1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/checkups/self-validation/list/CheckupsSelfValidationList.tsx` around lines 57 - 63, The callback currently calls getJobByName(jobs, ...) on every invocation which re-filters/sorts the full jobs array; instead, precompute a lookup map once (inside a useMemo keyed on jobs) that maps job name (or configMapName) to the pre-sorted array of IoK8sApiBatchV1Job entries, then make callbacks.getJobByName use that map for O(1) lookups; update the useMemo that defines callbacks to depend on the precomputed map (not the raw jobs) and keep the CheckupsSelfValidationCallbacks signature the same so callers are unaffected.
🤖 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/components/KubevirtTable/KubevirtTable.tsx`:
- Around line 73-78: The activeColumns useMemo can end up empty when persisted
activeColumnKeys no longer match any column; update the logic in the useMemo
that computes activeColumns to fall back to the default set of non-additional
columns when the filtered result is empty. Concretely, inside the useMemo that
references activeColumnKeys, columns, and col.additional, compute the filtered
list (keeping cols where activeColumnKeys.includes(col.key) or col.key ===
'actions') and if that filtered array has length 0 return columns.filter(col =>
!col.additional) instead.
In
`@src/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsx`:
- Around line 95-100: The sorter for the "startTime" column currently uses
STATUS_START_TIME_STAMP (status.startTimestamp) but TimeCell renders using
latestJob.status.startTime with fallbacks; update the sort function (the lambda
passed to sort for key 'startTime') to mirror TimeCell's value resolution:
prefer latestJob.status.startTime, then fall back to status.startTimestamp (or
the same fallbacks TimeCell uses), and pass that resolved value into
columnsSorting so sorting matches the rendered cell; locate TimeCell and the
existing sort entry (using columnsSorting and STATUS_START_TIME_STAMP) to copy
the same fallback logic.
In `@src/views/checkups/storage/list/checkupsStorageListDefinition.tsx`:
- Around line 47-55: The cluster column's getValue currently returns
getCluster(row) ?? '' which causes ACM rows to sort as blank even though
ClusterCell renders a fallback (hubClusterName); update the column's getValue to
mirror the display logic used by ClusterCell by returning getCluster(row) ??
<the same hubClusterName fallback the ClusterCell uses> ?? '' (i.e., thread the
same fallback into getValue so sorting matches rendered cell text), keeping the
column key 'cluster' and isACMPage branch unchanged.
- Around line 106-113: The fallback row id for getCheckupsStorageRowId (and
similarly update getCheckupsSelfValidationRowId) must include the cluster to
avoid collisions across clusters; modify the fallback to build the id from
configMap.metadata.clusterName (or another cluster-identifying field on
metadata) plus namespace and name (e.g. `${cluster}-${namespace}-${name}`) and
fall back to a safe unique token like `unknown-cluster-${index}` when
clusterName is missing so the id remains stable and multicluster-safe.
---
Duplicate comments:
In `@locales/fr/plugin__kubevirt-plugin.json`:
- Line 386: The French locale contains untranslated "checkups" strings — replace
the English value for the key "Checkups history table" with a French translation
and do the same for the three other untranslated checkups entries referenced in
the comment (lines 1295, 1733, 1844); specifically, update "Checkups history
table" → "Tableau de l'historique des contrôles" and similarly translate the
other checkups values (suggested: "Checkups" → "Contrôles", "Checkup details" →
"Détails du contrôle", "Run checkup" → "Exécuter un contrôle") so the UI is
fully in French.
---
Nitpick comments:
In `@src/utils/hooks/usePagination/usePaginationWithFilters.ts`:
- Around line 34-45: The pagination-reset object used in handleFilterChange (and
the similar block at lines 59-68) is duplicated; extract a small helper function
(e.g., getResetPagination or makeResetPagination) inside
usePaginationWithFilters.ts that returns { endIndex: pagination?.perPage, page:
1, perPage: pagination?.perPage, startIndex: 0 }, and replace the inline objects
in handleFilterChange and the other handler(s) to call that helper and pass its
result into onPaginationChange; ensure the helper references the same pagination
variable used by the hooks so semantics remain identical.
In `@src/views/checkups/self-validation/list/CheckupsSelfValidationList.tsx`:
- Around line 57-63: The callback currently calls getJobByName(jobs, ...) on
every invocation which re-filters/sorts the full jobs array; instead, precompute
a lookup map once (inside a useMemo keyed on jobs) that maps job name (or
configMapName) to the pre-sorted array of IoK8sApiBatchV1Job entries, then make
callbacks.getJobByName use that map for O(1) lookups; update the useMemo that
defines callbacks to depend on the precomputed map (not the raw jobs) and keep
the CheckupsSelfValidationCallbacks signature the same so callers are
unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57c1fd9d-d7f1-47c2-84c9-4479b465fd08
📒 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/KubevirtTable/KubevirtTable.tsxsrc/utils/hooks/useDataViewTableSort/types.tssrc/utils/hooks/useDataViewTableSort/useDataViewTableSort.tssrc/utils/hooks/useKubevirtUserSettings/useKubevirtTableColumns.tssrc/utils/hooks/usePagination/usePagination.tssrc/utils/hooks/usePagination/usePaginationWithFilters.tssrc/views/checkups/CheckupsDetailsPageHistory.tsxsrc/views/checkups/CheckupsDetailsPageHistoryRow.tsxsrc/views/checkups/CheckupsStatusIcon.tsxsrc/views/checkups/checkupsDetailsPageHistoryDefinition.tsxsrc/views/checkups/components/cells/CheckupsSharedCells.tsxsrc/views/checkups/hooks/useCheckupsDetailsPageHistoryColumns.tssrc/views/checkups/self-validation/components/hooks/useCheckupsSelfValidationListColumns.tssrc/views/checkups/self-validation/details/tabs/details/CheckupsSelfValidationDetailsTab.tsxsrc/views/checkups/self-validation/list/CheckupsSelfValidationList.tsxsrc/views/checkups/self-validation/list/CheckupsSelfValidationListRow.tsxsrc/views/checkups/self-validation/list/checkupsSelfValidationCells.tsxsrc/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsxsrc/views/checkups/storage/components/hooks/useCheckupsStorageData.tssrc/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.tssrc/views/checkups/storage/details/tabs/details/CheckupsStorageDetailsTab.tsxsrc/views/checkups/storage/list/CheckupsStorageList.tsxsrc/views/checkups/storage/list/CheckupsStorageListRow.tsxsrc/views/checkups/storage/list/checkupsStorageCells.tsxsrc/views/checkups/storage/list/checkupsStorageListDefinition.tsx
💤 Files with no reviewable changes (6)
- src/views/checkups/hooks/useCheckupsDetailsPageHistoryColumns.ts
- src/views/checkups/storage/list/CheckupsStorageListRow.tsx
- src/views/checkups/self-validation/components/hooks/useCheckupsSelfValidationListColumns.ts
- src/views/checkups/self-validation/list/CheckupsSelfValidationListRow.tsx
- src/views/checkups/CheckupsDetailsPageHistoryRow.tsx
- src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- locales/en/plugin__kubevirt-plugin.json
- src/utils/hooks/useDataViewTableSort/types.ts
- locales/es/plugin__kubevirt-plugin.json
- src/views/checkups/checkupsDetailsPageHistoryDefinition.tsx
- locales/ko/plugin__kubevirt-plugin.json
- locales/ja/plugin__kubevirt-plugin.json
- locales/zh/plugin__kubevirt-plugin.json
- src/utils/hooks/useDataViewTableSort/useDataViewTableSort.ts
- src/utils/hooks/useKubevirtUserSettings/useKubevirtTableColumns.ts
- src/views/checkups/storage/list/checkupsStorageCells.tsx
| const activeColumns = useMemo(() => { | ||
| if (!activeColumnKeys) { | ||
| return columns.filter((col) => !col.additional); | ||
| } | ||
| return columns.filter((col) => activeColumnKeys.includes(col.key) || col.key === 'actions'); | ||
| }, [columns, activeColumnKeys]); |
There was a problem hiding this comment.
Add a fallback when persisted column keys resolve to no columns.
If activeColumnKeys is stale (e.g., key renamed/removed), activeColumns can become empty and the table can render without usable columns. Please fall back to default non-additional columns in that case.
🛠️ Proposed fix
const activeColumns = useMemo(() => {
+ const defaultColumns = columns.filter((col) => !col.additional);
if (!activeColumnKeys) {
- return columns.filter((col) => !col.additional);
+ return defaultColumns;
}
- return columns.filter((col) => activeColumnKeys.includes(col.key) || col.key === 'actions');
+ const selected = columns.filter(
+ (col) => activeColumnKeys.includes(col.key) || col.key === 'actions',
+ );
+ return selected.length > 0 ? selected : defaultColumns;
}, [columns, activeColumnKeys]);📝 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.
| const activeColumns = useMemo(() => { | |
| if (!activeColumnKeys) { | |
| return columns.filter((col) => !col.additional); | |
| } | |
| return columns.filter((col) => activeColumnKeys.includes(col.key) || col.key === 'actions'); | |
| }, [columns, activeColumnKeys]); | |
| const activeColumns = useMemo(() => { | |
| const defaultColumns = columns.filter((col) => !col.additional); | |
| if (!activeColumnKeys) { | |
| return defaultColumns; | |
| } | |
| const selected = columns.filter( | |
| (col) => activeColumnKeys.includes(col.key) || col.key === 'actions', | |
| ); | |
| return selected.length > 0 ? selected : defaultColumns; | |
| }, [columns, activeColumnKeys]); |
🤖 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 73 - 78,
The activeColumns useMemo can end up empty when persisted activeColumnKeys no
longer match any column; update the logic in the useMemo that computes
activeColumns to fall back to the default set of non-additional columns when the
filtered result is empty. Concretely, inside the useMemo that references
activeColumnKeys, columns, and col.additional, compute the filtered list
(keeping cols where activeColumnKeys.includes(col.key) or col.key === 'actions')
and if that filtered array has length 0 return columns.filter(col =>
!col.additional) instead.
| { | ||
| key: 'startTime', | ||
| label: t('Start time'), | ||
| renderCell: (row, callbacks) => <TimeCell callbacks={callbacks} row={row} type="start" />, | ||
| sort: (data, direction) => columnsSorting(data, direction, STATUS_START_TIME_STAMP), | ||
| sortable: true, |
There was a problem hiding this comment.
Sort “Start time” with the same source that TimeCell renders.
TimeCell prefers latestJob.status.startTime, but this sorter only reads status.startTimestamp from the ConfigMap. After a rerun, users can see one start time and get a different sort order. Mirror the cell fallback here.
🕒 Suggested fix
{
key: 'startTime',
label: t('Start time'),
renderCell: (row, callbacks) => <TimeCell callbacks={callbacks} row={row} type="start" />,
- sort: (data, direction) => columnsSorting(data, direction, STATUS_START_TIME_STAMP),
+ sort: (data, sortDirection) => {
+ return [...data].sort((a, b) => {
+ const jobA = jobsByConfigMapName.get(a?.metadata?.name)?.[0];
+ const jobB = jobsByConfigMapName.get(b?.metadata?.name)?.[0];
+ const startTimeA = jobA?.status?.startTime || a?.data?.[STATUS_START_TIME_STAMP];
+ const startTimeB = jobB?.status?.startTime || b?.data?.[STATUS_START_TIME_STAMP];
+
+ if (!startTimeA && !startTimeB) return 0;
+ if (!startTimeA) return sortDirection === SortByDirection.asc ? 1 : -1;
+ if (!startTimeB) return sortDirection === SortByDirection.asc ? -1 : 1;
+
+ const timeA = new Date(startTimeA).getTime();
+ const timeB = new Date(startTimeB).getTime();
+
+ return sortDirection === SortByDirection.asc ? timeA - timeB : timeB - timeA;
+ });
+ },
sortable: true,
},📝 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.
| { | |
| key: 'startTime', | |
| label: t('Start time'), | |
| renderCell: (row, callbacks) => <TimeCell callbacks={callbacks} row={row} type="start" />, | |
| sort: (data, direction) => columnsSorting(data, direction, STATUS_START_TIME_STAMP), | |
| sortable: true, | |
| { | |
| key: 'startTime', | |
| label: t('Start time'), | |
| renderCell: (row, callbacks) => <TimeCell callbacks={callbacks} row={row} type="start" />, | |
| sort: (data, sortDirection) => { | |
| return [...data].sort((a, b) => { | |
| const jobA = jobsByConfigMapName.get(a?.metadata?.name)?.[0]; | |
| const jobB = jobsByConfigMapName.get(b?.metadata?.name)?.[0]; | |
| const startTimeA = jobA?.status?.startTime || a?.data?.[STATUS_START_TIME_STAMP]; | |
| const startTimeB = jobB?.status?.startTime || b?.data?.[STATUS_START_TIME_STAMP]; | |
| if (!startTimeA && !startTimeB) return 0; | |
| if (!startTimeA) return sortDirection === SortByDirection.asc ? 1 : -1; | |
| if (!startTimeB) return sortDirection === SortByDirection.asc ? -1 : 1; | |
| const timeA = new Date(startTimeA).getTime(); | |
| const timeB = new Date(startTimeB).getTime(); | |
| return sortDirection === SortByDirection.asc ? timeA - timeB : timeB - timeA; | |
| }); | |
| }, | |
| sortable: true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/views/checkups/self-validation/list/checkupsSelfValidationListDefinition.tsx`
around lines 95 - 100, The sorter for the "startTime" column currently uses
STATUS_START_TIME_STAMP (status.startTimestamp) but TimeCell renders using
latestJob.status.startTime with fallbacks; update the sort function (the lambda
passed to sort for key 'startTime') to mirror TimeCell's value resolution:
prefer latestJob.status.startTime, then fall back to status.startTimestamp (or
the same fallbacks TimeCell uses), and pass that resolved value into
columnsSorting so sorting matches the rendered cell; locate TimeCell and the
existing sort entry (using columnsSorting and STATUS_START_TIME_STAMP) to copy
the same fallback logic.
| ...(isACMPage | ||
| ? [ | ||
| { | ||
| getValue: (row: IoK8sApiCoreV1ConfigMap) => getCluster(row) ?? '', | ||
| key: 'cluster', | ||
| label: t('Cluster'), | ||
| renderCell: (row: IoK8sApiCoreV1ConfigMap) => <ClusterCell row={row} />, | ||
| sortable: true, | ||
| }, |
There was a problem hiding this comment.
Sort the cluster column by the value users actually see.
ClusterCell falls back to hubClusterName when row.cluster is missing, but this getValue returns ''. On ACM pages those rows sort as blank even though the table renders a cluster name. Thread the same fallback into the column definition so sorting matches the cell text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/checkups/storage/list/checkupsStorageListDefinition.tsx` around
lines 47 - 55, The cluster column's getValue currently returns getCluster(row)
?? '' which causes ACM rows to sort as blank even though ClusterCell renders a
fallback (hubClusterName); update the column's getValue to mirror the display
logic used by ClusterCell by returning getCluster(row) ?? <the same
hubClusterName fallback the ClusterCell uses> ?? '' (i.e., thread the same
fallback into getValue so sorting matches rendered cell text), keeping the
column key 'cluster' and isACMPage branch unchanged.
| export const getCheckupsStorageRowId = ( | ||
| configMap: IoK8sApiCoreV1ConfigMap, | ||
| index: number, | ||
| ): string => | ||
| configMap?.metadata?.uid || | ||
| (configMap?.metadata?.namespace && configMap?.metadata?.name | ||
| ? `${configMap.metadata.namespace}-${configMap.metadata.name}` | ||
| : `configmap-${index}`); |
There was a problem hiding this comment.
Make the fallback row id multicluster-safe.
namespace-name is not unique on ACM views. If a row ever lacks metadata.uid, two ConfigMaps from different clusters can collide and React will reuse the wrong row state. Include the cluster in this fallback here, and in getCheckupsSelfValidationRowId as well.
🪪 Suggested fix
export const getCheckupsStorageRowId = (
configMap: IoK8sApiCoreV1ConfigMap,
index: number,
-): string =>
- configMap?.metadata?.uid ||
- (configMap?.metadata?.namespace && configMap?.metadata?.name
- ? `${configMap.metadata.namespace}-${configMap.metadata.name}`
- : `configmap-${index}`);
+): string => {
+ const namespace = configMap?.metadata?.namespace;
+ const name = configMap?.metadata?.name;
+
+ return (
+ configMap?.metadata?.uid ||
+ (namespace && name
+ ? [getCluster(configMap), namespace, name].filter(Boolean).join('-')
+ : `configmap-${index}`)
+ );
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/checkups/storage/list/checkupsStorageListDefinition.tsx` around
lines 106 - 113, The fallback row id for getCheckupsStorageRowId (and similarly
update getCheckupsSelfValidationRowId) must include the cluster to avoid
collisions across clusters; modify the fallback to build the id from
configMap.metadata.clusterName (or another cluster-identifying field on
metadata) plus namespace and name (e.g. `${cluster}-${namespace}-${name}`) and
fall back to a safe unique token like `unknown-cluster-${index}` when
clusterName is missing so the id remains stable and multicluster-safe.
| }; | ||
|
|
||
| const useKubevirtTableColumns: UseKubevirtTableColumnsType = ({ columnManagementID, columns }) => { | ||
| const [userColumns, , loadedColumns, error] = useKubevirtUserSettings('columns'); |
There was a problem hiding this comment.
'columns' and 'actions' can be constants. 'actions' contant can also be used in column definition
| export const ClusterCell: FC<{ row: IoK8sApiCoreV1ConfigMap }> = ({ row }) => { | ||
| const [hubClusterName] = useHubClusterName(); | ||
| const cluster = getCluster(row) || hubClusterName; | ||
|
|
||
| return ( | ||
| <MulticlusterResourceLink | ||
| groupVersionKind={modelToGroupVersionKind(ManagedClusterModel)} | ||
| name={cluster} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
all the managedcluster resources are in the hubcluster .
To see the 'spoke' managed cluster resource u should fetch the managedcluster resource from the hub cluster. The spoke cluster do not know to be a managed cluster.
so u should not use the MulticlusterResourceLink but only the ResourceLink.
| const [hubClusterName] = useHubClusterName(); | ||
| const cluster = getCluster(row) || hubClusterName; | ||
|
|
||
| return ( | ||
| <MulticlusterResourceLink | ||
| cluster={cluster} | ||
| groupVersionKind={modelToGroupVersionKind(NamespaceModel)} | ||
| name={row?.metadata?.namespace} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
If we always provide a cluster resource to the MulticlusterResourceLink, even tho its the hubcluster resource, we'll generate an ACM link in any case. In this case u'll get a link to the ACM search page for the namespace resource. But if we are not in ACM, we should just pass undefined cluster to this component so it will generate a regular k8s console namespace link (non ACM)
📝 Description
Summary
activeColumnKeyspropuseKubevirtTableColumnshook to integrate with user settings for column persistenceadditionalproperty to ColumnConfig for columns hidden by default🎥 Demo
Screen.Recording.2026-03-15.at.1.23.25.mov
Summary by CodeRabbit
New Features
Refactor