CNV-83773: VirtualMachineTemplates filter fixes#3819
CNV-83773: VirtualMachineTemplates filter fixes#3819adamviktora wants to merge 2 commits intokubevirt-ui:mainfrom
Conversation
|
@adamviktora: This pull request references CNV-83773 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 task to target the "5.0.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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Cypress command to toggle the VM templates preview feature and updates gating tests; extends row-filter types to allow optional item content; introduces template-specific architecture helpers and unique-architecture utilities; separates templates from requests in aggregation and gates the Type filter by the VM templates feature flag. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/views/templates/utils/utils.ts (1)
46-47: Return type may includeundefinedvalues.
getTemplateArchitecturecan returnundefined, so the mapped array type is(string | undefined)[]. TheSetwill preserveundefinedas a distinct value, andArray.fromwill include it. The return typestring[]is technically incorrect.The consumer (
useArchitectureFilter) applies.filter(Boolean)to handle this, but the type annotation here could mislead other consumers.♻️ Suggested type correction
-export const getUniqueTemplateArchitectures = (templates: Template[]): string[] => - Array.from(new Set(templates.map((template) => getTemplateArchitecture(template)))); +export const getUniqueTemplateArchitectures = (templates: Template[]): (string | undefined)[] => + Array.from(new Set(templates.map((template) => getTemplateArchitecture(template))));Or filter out undefined values inside the function:
-export const getUniqueTemplateArchitectures = (templates: Template[]): string[] => - Array.from(new Set(templates.map((template) => getTemplateArchitecture(template)))); +export const getUniqueTemplateArchitectures = (templates: Template[]): string[] => + Array.from(new Set(templates.map((template) => getTemplateArchitecture(template)).filter(Boolean))) as string[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/utils/utils.ts` around lines 46 - 47, getUniqueTemplateArchitectures currently maps templates via getTemplateArchitecture which can return undefined, so the Set/Array.from may include undefined while the function is typed as string[]; update the function to exclude undefined values before creating the Set (e.g., map then filter(Boolean) or use reduce) so its actual return type is string[] and callers like useArchitectureFilter don't need to coerce; ensure you reference getTemplateArchitecture and preserve uniqueness via Set after filtering.src/views/templates/list/filters/useTypeFilter.tsx (1)
28-52: MovetypeFilterItemsinsideuseMemoor memoize separately.
typeFilterItemsis recreated on every render since it's defined outsideuseMemo. This causes theuseMemoat line 54 to recompute on each render despitefeatureEnabledbeing unchanged, negating the memoization benefit.♻️ Suggested refactor
const useTypeFilter = (featureEnabled: boolean): null | RowFilter<TemplateOrRequest> => { const { t } = useKubevirtTranslation(); - const openShiftTemplateTitle = t('OpenShift templates'); - const virtualMachineTemplateTitle = t('VirtualMachine templates'); - - const typeFilterItems: ExtendedRowFilterItem[] = [ - { - content: ( - <> - <ResourceIcon groupVersionKind={TemplateModelGroupVersionKind} /> - {openShiftTemplateTitle} - </> - ), - id: TEMPLATE_TYPE.OPENSHIFT, - title: openShiftTemplateTitle, - }, - { - content: ( - <> - <ResourceIcon groupVersionKind={VirtualMachineTemplateGroupVersionKind} /> - {virtualMachineTemplateTitle} - </> - ), - id: TEMPLATE_TYPE.VM, - title: virtualMachineTemplateTitle, - }, - ]; - return useMemo(() => { if (!featureEnabled) return null; + const openShiftTemplateTitle = t('OpenShift templates'); + const virtualMachineTemplateTitle = t('VirtualMachine templates'); + + const typeFilterItems: ExtendedRowFilterItem[] = [ + { + content: ( + <> + <ResourceIcon groupVersionKind={TemplateModelGroupVersionKind} /> + {openShiftTemplateTitle} + </> + ), + id: TEMPLATE_TYPE.OPENSHIFT, + title: openShiftTemplateTitle, + }, + { + content: ( + <> + <ResourceIcon groupVersionKind={VirtualMachineTemplateGroupVersionKind} /> + {virtualMachineTemplateTitle} + </> + ), + id: TEMPLATE_TYPE.VM, + title: virtualMachineTemplateTitle, + }, + ]; + return { filter: (types, obj) => includeFilter(types, typeFilterItems, getTemplateType(obj)), filterGroupName: t('Type'), items: typeFilterItems, reducer: (obj) => getTemplateType(obj), type: 'type', }; - }, [featureEnabled, typeFilterItems, t]); + }, [featureEnabled, t]); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/list/filters/useTypeFilter.tsx` around lines 28 - 52, typeFilterItems is recreated every render outside the useMemo, causing the memo that depends on featureEnabled to always recompute; move or memoize typeFilterItems so it is stable. Wrap the typeFilterItems array creation in useMemo (or include it inside the existing useMemo that returns the filters) and ensure its dependencies include TemplateModelGroupVersionKind, VirtualMachineTemplateGroupVersionKind, TEMPLATE_TYPE, and any i18n titles (openShiftTemplateTitle, virtualMachineTemplateTitle) so the ExtendedRowFilterItem objects are only recreated when those values change, preserving the intended memoization around featureEnabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/support/commands.ts`:
- Around line 118-127: The toggleVMTemplatesFeature command currently toggles
the vmTemplates switch but does not assert the resulting state, which can cause
flaky tests; after the conditional click inside the
Cypress.Commands.add('toggleVMTemplatesFeature', ...) block, re-query the
[data-test-id="vmTemplates"] element and assert that its aria-checked equals the
desired enable value (use
cy.get('[data-test-id="vmTemplates"]').should('have.attr', 'aria-checked',
String(enable))) so the command only resolves when the switch is in the expected
state.
In `@src/views/templates/list/filters/useArchitectureFilter.ts`:
- Around line 31-35: The architecture filter currently omits requests because
getTemplateArchitecture(obj) can be undefined for VirtualMachineTemplateRequest,
getItemNameWithOther(..., architectureItems) maps that to OTHER but
architectureItems (built via getUniqueTemplateArchitectures and
.filter(Boolean)) never contains OTHER; update the filter generation so
architectureItems always includes an "Other/Unknown" entry (or explicitly append
an OTHER item) so includeFilter(availableArchitectures, architectureItems,
getTemplateArchitecture(obj)) and reducer: getItemNameWithOther(...) will match
requests, or alternatively change the filter predicate (the filter property
using includeFilter) to treat undefined architectures as always allowed when
compareData.selected has selections; adjust the code around architectureItems,
getUniqueTemplateArchitectures, getTemplateArchitecture and getItemNameWithOther
accordingly.
---
Nitpick comments:
In `@src/views/templates/list/filters/useTypeFilter.tsx`:
- Around line 28-52: typeFilterItems is recreated every render outside the
useMemo, causing the memo that depends on featureEnabled to always recompute;
move or memoize typeFilterItems so it is stable. Wrap the typeFilterItems array
creation in useMemo (or include it inside the existing useMemo that returns the
filters) and ensure its dependencies include TemplateModelGroupVersionKind,
VirtualMachineTemplateGroupVersionKind, TEMPLATE_TYPE, and any i18n titles
(openShiftTemplateTitle, virtualMachineTemplateTitle) so the
ExtendedRowFilterItem objects are only recreated when those values change,
preserving the intended memoization around featureEnabled.
In `@src/views/templates/utils/utils.ts`:
- Around line 46-47: getUniqueTemplateArchitectures currently maps templates via
getTemplateArchitecture which can return undefined, so the Set/Array.from may
include undefined while the function is typed as string[]; update the function
to exclude undefined values before creating the Set (e.g., map then
filter(Boolean) or use reduce) so its actual return type is string[] and callers
like useArchitectureFilter don't need to coerce; ensure you reference
getTemplateArchitecture and preserve uniqueness via Set after filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3aeeebeb-5b94-4f30-98d1-1b4602b92347
📒 Files selected for processing (15)
cypress/support/commands.tscypress/tests/gating/template-list.cy.tssrc/utils/components/ListPageFilter/ListPageFilter.tsxsrc/utils/components/ListPageFilter/components/RowFilters.tsxsrc/utils/components/ListPageFilter/types.tssrc/utils/components/ListPageFilter/utils.tssrc/utils/utils/architecture.tssrc/views/templates/list/VirtualMachineTemplatesList.tsxsrc/views/templates/list/filters/useArchitectureFilter.tssrc/views/templates/list/filters/useTypeFilter.tssrc/views/templates/list/filters/useTypeFilter.tsxsrc/views/templates/list/filters/useVirtualMachineTemplatesFilters.tssrc/views/templates/list/hooks/useAllTemplateResources.tssrc/views/templates/list/virtualMachineTemplatesDefinition.tsxsrc/views/templates/utils/utils.ts
💤 Files with no reviewable changes (1)
- src/views/templates/list/filters/useTypeFilter.ts
adamviktora
force-pushed
the
vmtemplates-fixes
branch
from
300be53 to
eb8433e
Compare
|
/retest |
1 similar comment
|
/retest |
|
I will need to fix the e2e tests. |
adamviktora
force-pushed
the
vmtemplates-fixes
branch
from
eb8433e to
e0989f5
Compare
- display Architecture cell for VM templates, fix Architecture filter - show resource badge for Type filter (T and VMT) - reenable and adjust cypress tests for template filters
adamviktora
force-pushed
the
vmtemplates-fixes
branch
from
e0989f5 to
0347275
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/views/settings/SettingsTab.tsx (1)
30-36: Optional: replacemap(... ? ... : null)with direct active-tab selection.Current logic works, but selecting the active tab once is simpler to read and avoids iterating with mostly
nullreturns.♻️ Proposed refactor
- {tabs.map(({ Component, name }) => - activeTab === name ? ( - <div className="settings-tab__content" key={name}> - <Component /> - </div> - ) : null, - )} + {(() => { + const active = tabs.find(({ name }) => name === activeTab); + if (!active) return null; + const { Component } = active; + return ( + <div className="settings-tab__content"> + <Component /> + </div> + ); + })()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/settings/SettingsTab.tsx` around lines 30 - 36, The current rendering iterates tabs with tabs.map(({ Component, name }) => activeTab === name ? <div className="settings-tab__content" key={name}><Component /></div> : null), which is noisy; replace it by selecting the active tab once (e.g., const active = tabs.find(t => t.name === activeTab)) and then render a single conditional block like active ? <div className="settings-tab__content"><active.Component /></div> to avoid mapping to many nulls and simplify logic; reference the tabs array, activeTab variable, and the Component/name properties when making this change.src/views/templates/list/filters/useArchitectureFilter.ts (1)
16-21: Optional cleanup: remove unreachable nullish fallback in architecture item mapping.Because the list is already filtered with
.filter(Boolean),arch ?? OTHERandarch ?? t(OTHER)are not hit in practice. You can simplify this for readability.♻️ Suggested simplification
- getUniqueTemplateArchitectures(templates) - .filter(Boolean) - .map((arch) => ({ - id: arch ?? OTHER, - title: arch ?? t(OTHER), - })), + getUniqueTemplateArchitectures(templates) + .filter((arch): arch is string => Boolean(arch)) + .map((arch) => ({ + id: arch, + title: arch, + })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/templates/list/filters/useArchitectureFilter.ts` around lines 16 - 21, The mapping over architectures in getUniqueTemplateArchitectures(templates).filter(Boolean).map(...) contains unreachable nullish fallbacks (arch ?? OTHER and arch ?? t(OTHER)); remove the ?? fallbacks and map using arch directly so each item becomes { id: arch, title: arch && t ? t(arch) : arch } or simply { id: arch, title: t ? t(arch) : arch } depending on where translation is expected; update the map in useArchitectureFilter.ts to use arch directly and drop the OTHER fallback to improve readability and avoid redundant checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/views/settings/SettingsTab.tsx`:
- Around line 30-36: The current rendering iterates tabs with tabs.map(({
Component, name }) => activeTab === name ? <div
className="settings-tab__content" key={name}><Component /></div> : null), which
is noisy; replace it by selecting the active tab once (e.g., const active =
tabs.find(t => t.name === activeTab)) and then render a single conditional block
like active ? <div className="settings-tab__content"><active.Component /></div>
to avoid mapping to many nulls and simplify logic; reference the tabs array,
activeTab variable, and the Component/name properties when making this change.
In `@src/views/templates/list/filters/useArchitectureFilter.ts`:
- Around line 16-21: The mapping over architectures in
getUniqueTemplateArchitectures(templates).filter(Boolean).map(...) contains
unreachable nullish fallbacks (arch ?? OTHER and arch ?? t(OTHER)); remove the
?? fallbacks and map using arch directly so each item becomes { id: arch, title:
arch && t ? t(arch) : arch } or simply { id: arch, title: t ? t(arch) : arch }
depending on where translation is expected; update the map in
useArchitectureFilter.ts to use arch directly and drop the OTHER fallback to
improve readability and avoid redundant checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ba411ac-1225-4e9f-837e-3f7a7be85595
📒 Files selected for processing (16)
cypress/support/commands.tscypress/tests/gating/template-list.cy.tssrc/utils/components/ListPageFilter/ListPageFilter.tsxsrc/utils/components/ListPageFilter/components/RowFilters.tsxsrc/utils/components/ListPageFilter/types.tssrc/utils/components/ListPageFilter/utils.tssrc/utils/utils/architecture.tssrc/views/settings/SettingsTab.tsxsrc/views/templates/list/VirtualMachineTemplatesList.tsxsrc/views/templates/list/filters/useArchitectureFilter.tssrc/views/templates/list/filters/useTypeFilter.tssrc/views/templates/list/filters/useTypeFilter.tsxsrc/views/templates/list/filters/useVirtualMachineTemplatesFilters.tssrc/views/templates/list/hooks/useAllTemplateResources.tssrc/views/templates/list/virtualMachineTemplatesDefinition.tsxsrc/views/templates/utils/utils.ts
💤 Files with no reviewable changes (1)
- src/views/templates/list/filters/useTypeFilter.ts
✅ Files skipped from review due to trivial changes (2)
- src/utils/utils/architecture.ts
- src/views/templates/list/VirtualMachineTemplatesList.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src/utils/components/ListPageFilter/utils.ts
- cypress/tests/gating/template-list.cy.ts
- src/utils/components/ListPageFilter/ListPageFilter.tsx
- src/views/templates/utils/utils.ts
- src/views/templates/list/filters/useTypeFilter.tsx
- src/utils/components/ListPageFilter/types.ts
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adamviktora, galkremer1, upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
New changes are detected. LGTM label has been removed. |
galkremer1
force-pushed
the
vmtemplates-fixes
branch
from
d760d97 to
460a83a
Compare
|
/retest |
1 similar comment
|
/retest |
|
@adamviktora: 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. |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17086
❌ Failure 1 —
|
| # | Test | Root Cause | Related to PR? |
|---|---|---|---|
| 1 | filter templates by Provider | Filter click interaction not working | Yes (test changes) |
| 2 | Type filter with VM templates feature | Feature toggle / filter logic issue | Yes (new test + new code) |
| 3 | verify live migration limits in HCO | Env state / pre-existing flake | Unlikely |
4 participants
📝 Description
Fixes for VirtualMachineTemplates:
🎥 Demo
Before:
Screen.Recording.2026-04-20.at.15.17.31.mov
After:
Screen.Recording.2026-04-20.at.15.16.41.mov
Summary by CodeRabbit
New Features
Tests