CNV-75366: reflect filtered VMs in Usage summary #3305
Conversation
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughRefactors VM metrics and mapper flows: adds storage metrics, centralizes metric dispatch, changes hooks to return/use both VMs and VMIs, updates components to accept VMs, and removes the PromQL totals utility module. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
38-38: Inconsistent query access pattern.Line 38 uses dot notation (
queries?.NETWORK_TOTAL_USAGE) while other queries use bracket notation with theVMListQueriesconstant. This should be consistent for maintainability.🔎 Suggested fix
const [networkTotalResponse] = useFleetPrometheusPoll({ ...prometheusPollProps, - query: queries?.NETWORK_TOTAL_USAGE, + query: queries?.[VMListQueries.NETWORK_TOTAL_USAGE], });src/views/virtualmachines/list/metrics.ts (1)
60-69: Consider guarding against missing metric labels.The function assumes
result.metric.nameandresult.metric.namespaceare always present. If a Prometheus result lacks these labels,setMetricValuewill be called withundefinedvalues, creating malformed keys like"cluster-undefined-undefined".🔎 Proposed fix
export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric) => { response?.data?.result?.forEach((result) => { const vmName = result?.metric?.name; const vmNamespace = result?.metric?.namespace; const vmCluster = result?.metric?.cluster; const value = parseFloat(result?.value?.[1]); + if (!vmName || !vmNamespace) return; + setMetricValue(vmName, vmNamespace, vmCluster, metric, value); }); };src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (1)
21-30: Consider consolidating reduce operations for efficiency.Four separate
reduceiterations callgetVMMetrics(vm)repeatedly for each VM. While functionally correct, a single iteration would be more efficient for larger VM lists.🔎 Proposed optimization
return useMemo(() => { - const totalCpuUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.cpuUsage ?? 0), 0); - const totalMemoryUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.memoryUsage ?? 0), 0); - const totalStorageUsage = vms.reduce( - (acc, vm) => acc + (getVMMetrics(vm)?.storageUsage ?? 0), - 0, - ); - const totalStorageCapacity = vms.reduce( - (acc, vm) => acc + (getVMMetrics(vm)?.storageCapacity ?? 0), - 0, - ); + const totals = vms.reduce( + (acc, vm) => { + const metrics = getVMMetrics(vm); + return { + cpuUsage: acc.cpuUsage + (metrics?.cpuUsage ?? 0), + memoryUsage: acc.memoryUsage + (metrics?.memoryUsage ?? 0), + storageUsage: acc.storageUsage + (metrics?.storageUsage ?? 0), + storageCapacity: acc.storageCapacity + (metrics?.storageCapacity ?? 0), + }; + }, + { cpuUsage: 0, memoryUsage: 0, storageUsage: 0, storageCapacity: 0 }, + ); return { cpuRequested: getCpuRequestedText(vmis), - cpuUsage: getCpuUsageText(totalCpuUsage), + cpuUsage: getCpuUsageText(totals.cpuUsage), memoryCapacity: getMemoryCapacityText(vmis), - memoryUsage: getValueWithUnitText(totalMemoryUsage, METRICS.MEMORY), - storageCapacity: getValueWithUnitText(totalStorageCapacity, METRICS.STORAGE), - storageUsage: getValueWithUnitText(totalStorageUsage, METRICS.STORAGE), + memoryUsage: getValueWithUnitText(totals.memoryUsage, METRICS.MEMORY), + storageCapacity: getValueWithUnitText(totals.storageCapacity, METRICS.STORAGE), + storageUsage: getValueWithUnitText(totals.storageUsage, METRICS.STORAGE), }; }, [vms, vmis]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsfile extension for non-component files containing logic or utilities
Hooks should contain only logic and side effects, not return JSX; keep JSX in components while hooks are used for extracting reusable or unit-testable logic
Define constants in utility files with uppercase and underscore-separated naming (e.g., API_URL)
If a type is exported, add it to a utility file
Files:
src/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Keep files under 150 lines whenever possible
Extract as much logic as possible from components into custom hooks or utility files for easier unit testing
Use descriptive names for variables, functions, and components; avoid abbreviations unless widely recognized (e.g., use fetchUserData instead of getData)
Keep functions short and focused on one action; apply Red → Green → Refactor: write a failing solution, implement a working solution, then refactor for readability and performance
Prefer using type instead of interface for defining the shapes of objects or functions in TypeScript
Avoid using any type in TypeScript; use unknown instead and narrow the type as needed to maintain type safety
Always explicitly define return types for functions rather than relying on TypeScript inference to prevent unintentional changes to the return value during bug fixes
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability
Files:
src/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Avoid comments whenever possible; write self-explanatory code; use comments sparingly for unusual values or decisions to explain the rationale
Files:
src/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components with one component per file and no nested components
Use PascalCase for all component names (e.g., HeaderTop.tsx)
Use functional components as the default; only use class components for specific lifecycle methods unavailable in functional components (e.g., componentDidCatch)
Use default exports for all components
Use React memoization tools (React.memo, useMemo, useCallback) to avoid unnecessary re-renders
Lazy load components with React.lazy and Suspense
Always specify dependencies in useEffect to avoid unnecessary re-renders or missed updates; pass an empty array [] if no dependencies are required to run the effect only once
Files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
**/*.{scss,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use project-based class names on components as the anchor for SCSS rules rather than relying on patternfly class names
Files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧬 Code graph analysis (7)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx (1)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)src/views/virtualmachines/list/metrics.ts (1)
setMetricFromResponse(60-69)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (6)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/utils/utils/humanize.js (1)
value(234-234)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/virtualmachines/list/hooks/utils.ts (1)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)
src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)src/views/virtualmachines/utils/mappers.ts (1)
getVMIFromMapper(26-27)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(30-36)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
src/views/virtualmachines/list/metrics.ts (3)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/constants.ts (1)
SINGLE_CLUSTER_KEY(1-1)src/utils/resources/shared.ts (2)
getNamespace(341-342)getName(333-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (12)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx (1)
16-17: LGTM!The destructuring correctly adapts to the updated
useVirtualMachineInstanceMapperhook signature that now returns{ vmiMapper, vmisLoaded }. Since this component only needsvmiMapper, ignoringvmisLoadedis appropriate.src/views/virtualmachines/list/hooks/constants.ts (1)
9-10: LGTM!The new storage metric constants follow the established naming convention and pattern used by other query keys in this object.
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
10-42: LGTM!The hook now correctly exposes both
vmiMapperandvmisLoaded, enabling consumers to access the loading state. The relevant consumers in this PR (NodesField.tsx,VirtualMachinesList.tsx) have been updated accordingly.src/views/virtualmachines/list/hooks/utils.ts (1)
43-44: LGTM!The storage metric queries correctly use
max()aggregation withdisk_namein the grouping clause to handle multiple filesystems per VM. This appropriately differs from other metrics that don't have a disk dimension.src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx (1)
14-17: LGTM!The component correctly accepts both
vmisandvmsprops and forwards them toVirtualMachineUsage. This enables the usage summary to compute metrics across filtered VMs as intended by the PR.Also applies to: 43-43
src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
130-133: LGTM!The derived
filteredVMIscorrectly maps filtered VMs through thevmiMapperto get corresponding VMI objects, with.filter(Boolean)handling VMs that may not have a running VMI. This ensures the usage summary reflects only the filtered subset.
194-194: LGTM!Passing both
filteredVMIsandfilteredVMstoVirtualMachineListSummaryenables the summary to compute totals across only the filtered VMs, achieving the PR's objective.src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
46-74: LGTM on the new storage metric polling.The storage usage and capacity polls follow the established pattern for other metrics. The
useEffecthooks correctly process responses viasetMetricFromResponse, and both'storageUsage'and'storageCapacity'are valid values for theMetrictype.src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx (1)
10-18: LGTM!The component changes are well-structured. The prop type update and hook invocation change align with the new API contract that accepts both
vmisandvmsfor filtered metric calculations.src/views/virtualmachines/list/metrics.ts (1)
38-47: Side-effect in getter is acceptable for lazy initialization.The mutation within
getVMMetricsWithParamsfollows a lazy initialization pattern that ensures the metrics object exists before access. This is a common pattern for signal-based state. The implementation looks correct.src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (2)
27-32: Good defensive handling for NaN.The explicit
isNaNcheck withNO_DATA_DASHfallback is a solid pattern for handling cases where metrics might not be available.
14-19: No argument mismatch exists — thegetHumanizedValuefunction imported from@overview/OverviewTab/metric-charts-card/utils/hooks/utilsis defined as(metric: string, value: number, unit: string), which matches the callgetHumanizedValue(metric, bytes, unit)exactly. The review comment incorrectly referenced a different function from TopConsumersTab.Likely an incorrect or invalid review comment.
| const [vmis, vmisLoaded] = useKubevirtWatchResource<V1VirtualMachineInstance[]>( | ||
| { | ||
| cluster, | ||
| groupVersionKind: VirtualMachineInstanceModelGroupVersionKind, | ||
| isList: true, | ||
| limit: OBJECTS_FETCHING_LIMIT, | ||
| namespace, | ||
| namespaced: true, | ||
| }, | ||
| VMI_FILTER_OPTIONS, | ||
| searchQueries?.vmiQueries, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Can we still use VMI_FILTER_OPTIONS and searchQueries?.vmiQueries in useVirtualMachineInstanceMapper.ts ? This will let us fetch only the vmi that we need when we filter
There was a problem hiding this comment.
@upalatucci Yeah, I know. The issue with this is that we use useVirtualMachineInstanceMapper to create the filters itself. E.g. a Nodes filter will show Nodes in dropdown determined from VMIs. And if we filter the VMIs out we then show only some options / no options in the filter (this was a bug some time ago)
If we would have better way to generate filters (probably some new endpoint on proxy), we can then do this
| }, [cpuUsageResponse]); | ||
|
|
||
| useEffect(() => { | ||
| setMetricFromResponse(storageUsageResponse, 'storageUsage'); |
There was a problem hiding this comment.
We should probably use constants for storageUsage, storageCapacity and etc.
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
adamviktora
force-pushed
the
usage-summary-filtered-vms
branch
from
b7b6b4a to
3091fa6
Compare
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (1)
34-43: Add explicit return type.The function lacks an explicit return type.
🔎 Proposed fix
-export const getMemoryCapacityText = (vmis: V1VirtualMachineInstance[]) => { +export const getMemoryCapacityText = (vmis: V1VirtualMachineInstance[]): string => { const bytes = vmis .map((vmi) => { const memorySize = getMemorySize(getMemory(vmi)); return xbytes.parseSize(`${memorySize?.size} ${memorySize?.unit}B`); }) .reduce((acc, cur) => acc + cur, 0); return getValueWithUnitText(bytes, METRICS.MEMORY); };As per coding guidelines, all exported functions should have explicit return types.
src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
14-14: Add explicit return type for the hook.The hook lacks an explicit return type (should be
voidsince it doesn't return anything).🔎 Proposed fix
-const useVMMetrics = () => { +const useVMMetrics = (): void => { const namespace = useNamespaceParam(); const cluster = useClusterParam(); const allClusters = useIsAllClustersPage();As per coding guidelines, all exported functions should have explicit return types.
src/views/virtualmachines/list/VirtualMachinesList.tsx (1)
137-141: MissingonFilterChangein useEffect dependency array.The
useEffectreferencesonFilterChangein its callback but doesn't include it in the dependency array. While this may work due toonFilterChangebeing stable, ESLint's exhaustive-deps rule would flag this.🔎 Proposed fix
useEffect(() => { Object.values(VirtualMachineRowFilterType).forEach((filterType) => { onFilterChange?.(filterType, filtersFromURL[filterType]); }); - }, [query]); + }, [query, onFilterChange, filtersFromURL]);
🤖 Fix all issues with AI Agents
In @src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts:
- Around line 10-14: Add an explicit return type for the exported hook
useVirtualMachineInstanceMapper by defining a type (e.g.,
UseVirtualMachineInstanceMapperResult) with the properties returned (vmiMapper:
VMIMapper and vmisLoaded: boolean) and annotate the hook signature to return
that type; update the function declaration of useVirtualMachineInstanceMapper to
use UseVirtualMachineInstanceMapperResult and ensure the returned object shape
(vmiMapper and vmisLoaded from the useKubevirtWatchResource call) matches the
type.
In @src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts:
- Around line 20-40: The memoized calculation in useVMTotalsMetrics is stale
because getVMMetrics reads from the vmsMetrics Preact signal but useMemo only
depends on [vms, vmis], so updates to the signal won't retrigger the memo; fix
by removing useMemo so totals are recalculated every render (or alternatively
include the vmsMetrics signal reference in the dependency list), and ensure
getVMMetrics is called directly inside the recalculation (references:
useVMTotalsMetrics, getVMMetrics, vmsMetrics, and the current useMemo with deps
[vms, vmis]).
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts:
- Around line 27-32: The exported function getCpuUsageText has no explicit
return type; update its signature to declare the return type (e.g., string) so
the function reads like: export const getCpuUsageText = (cpuUsage: number):
string => { ... } and keep the existing logic using NO_DATA_DASH and
humanizeCpuCores(cpuUsage).string unchanged.
- Around line 14-19: The exported function getValueWithUnitText currently relies
on TypeScript inference for its return type; update its signature to include an
explicit return type (string) so it reads like: export const
getValueWithUnitText = (bytes: number, metric: string): string => { ... },
ensuring the function explicitly declares the return type as per coding
guidelines.
- Around line 21-25: Add an explicit return type to the getCpuRequestedText
function: change its signature to declare it returns a string (i.e., annotate
getCpuRequestedText(...) : string). Keep the internals as-is — it still maps VMI
objects via getCPU and getVCPUCount and returns
humanizeCpuCores(totalCPU).string — only add the explicit ": string" return type
on the getCpuRequestedText declaration.
🧹 Nitpick comments (1)
src/views/virtualmachines/list/metrics.ts (1)
40-49: Side effect in getter may cause unexpected behavior.
getVMMetricsWithParamsmutatesvmsMetrics.valueas a side effect when the key doesn't exist. This mixing of read and write operations violates the single responsibility principle and could cause subtle bugs or unexpected re-renders.🔎 Consider separating initialization from retrieval
export const getVMMetricsWithParams = ( name: string, namespace: string, cluster = SINGLE_CLUSTER_KEY, ) => { const vmMetrics = vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; - if (isEmpty(vmMetrics)) vmsMetrics.value[`${cluster}-${namespace}-${name}`] = {}; - - return vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; + return vmMetrics ?? {}; };Then ensure
setMetricValuehandles initialization:const setMetricValue = ( name: string, namespace: string, cluster = SINGLE_CLUSTER_KEY, metric: Metric, value: number, ) => { - const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); + const key = `${cluster}-${namespace}-${name}`; + if (!vmsMetrics.value[key]) { + vmsMetrics.value[key] = {}; + } + const vmMetrics = vmsMetrics.value[key]; vmMetrics[metric] = value; };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/views/virtualmachines/list/hooks/constants.ts
- src/views/virtualmachines/list/hooks/utils.ts
- src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧰 Additional context used
📓 Path-based instructions (7)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use
.tsfile extension for non-component files containing logic or utilities
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/metrics.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{tsx,ts}: Extract logic from components into custom hooks or utility files to improve testability and component maintainability
Use React memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. Use an empty array[]if no dependencies are required
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx,js,jsx}: Keep files under 150 lines whenever possible
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized
Keep functions short and focused on one action. Apply Red → Green → Refactor methodology
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Define constants in utility files with uppercase and underscore-separated naming (e.g.,
API_URL)
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript
If a type is exported, add it to a utility file
Avoid usinganytype in TypeScript. Useunknowninstead and narrow the type as needed
Always explicitly define return types for functions rather than relying on TypeScript type inference
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/metrics.ts
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxfile extension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx)
Use functional components by default. Only use class components for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch)
Use default exports for all React components
UseReact.lazyandSuspensefor lazy loading components
Files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
**/*.{tsx,scss}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Use project-based class names on components as anchors for styling rules rather than relying on PatternFly class names
Files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
🧠 Learnings (4)
📚 Learning: 2025-12-24T13:50:10.254Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-24T13:50:10.254Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability
Applied to files:
src/views/virtualmachines/list/hooks/useVMMetrics.ts
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
📚 Learning: 2025-12-24T13:50:10.254Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-24T13:50:10.254Z
Learning: Applies to **/*.{tsx,ts} : Use React memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/VirtualMachinesList.tsx
🧬 Code graph analysis (6)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)src/views/virtualmachines/list/metrics.ts (1)
setMetricFromResponse(62-71)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx (1)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (6)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/utils/utils/humanize.js (1)
value(234-234)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(32-38)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)src/views/virtualmachines/utils/mappers.ts (1)
getVMIFromMapper(26-27)
src/views/virtualmachines/list/metrics.ts (3)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/constants.ts (1)
SINGLE_CLUSTER_KEY(1-1)src/utils/resources/shared.ts (2)
getNamespace(341-342)getName(333-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx (1)
16-16: LGTM!The destructuring correctly adapts to the updated
useVirtualMachineInstanceMapperhook API, which now returns{ vmiMapper, vmisLoaded }instead of just the mapper.src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
42-42: LGTM!Exposing
vmisLoadedalongsidevmiMapperallows consumers to track the loading state, improving UX by enabling proper loading indicators.src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx (1)
43-43: LGTM!The updated prop passing correctly aligns with the new
VirtualMachineUsagecomponent API that requires bothvmisandvmsfor enhanced metrics calculation.src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
46-54: LGTM!The storage metrics queries follow the established pattern and correctly use constants from
VMListQueriesfor the query keys.
68-74: LGTM!The storage metric effects correctly apply the unified
setMetricFromResponsepattern and properly declare their dependencies.src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
130-134: LGTM - Clean derivation of filtered VMIs.The memoization correctly derives
filteredVMIsfrom the filtered VMs using the mapper, with proper dependencies. This aligns well with the PR objective of reflecting filtered VMs in the Usage summary.
113-116: All call sites correctly use the new hook return shape.The
useVirtualMachineInstanceMapperhook returns{ vmiMapper, vmisLoaded }and both usage sites in the codebase destructure this correctly:
VirtualMachinesList.tsxdestructures both properties:{ vmiMapper, vmisLoaded }NodesField.tsxdestructures the required property:{ vmiMapper }No migration issues identified.
src/views/virtualmachines/list/metrics.ts (2)
14-28: Well-structured Metric enum and types.The
Metricenum provides type safety and consistency across the metrics system. TheMetricsObjecttype correctly maps each metric to an optional number.
62-71: LGTM - Clean generic metric setter.
setMetricFromResponseprovides a clean, generic way to update metrics from Prometheus responses. The implementation correctly extracts VM identifiers and values.src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (1)
14-17: LGTM - Clean type definition.The
UseVMTotalsMetricsPropstype clearly defines the hook's inputs. Per coding guidelines, consider exporting this type to a utility file if it will be reused elsewhere.
| cpuUsage: getCpuUsageText(totalCpuUsage), | ||
| memoryCapacity: getMemoryCapacityText(vmis), | ||
| memoryUsage: getValueWithUnitText(totalMemoryUsage, METRICS.MEMORY), | ||
| storageCapacity: getValueWithUnitText(totalStorageCapacity, METRICS.STORAGE), |
There was a problem hiding this comment.
We should be able to use the enums you created here using [storageCapacity] for example.
adamviktora
force-pushed
the
usage-summary-filtered-vms
branch
from
6f3377a to
41a81c2
Compare
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adamviktora, galkremer1 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts:
- Around line 20-40: The memoization is stale because useMemo in
useVMTotalsMetrics reads getVMMetrics (which uses the vmsMetrics Preact signal)
but only depends on [vms, vmis], so updates to vmsMetrics don't trigger
recalculation; fix by removing the useMemo wrapper in useVMTotalsMetrics so
totals are computed on each render (or, alternatively, include the signal/state
that getVMMetrics reads as a dependency), ensuring getVMMetrics, vms, and vmis
are evaluated every render and the returned object (keys: Metric.cpuRequested,
Metric.cpuUsage, Metric.memoryCapacity, Metric.memoryUsage,
Metric.storageCapacity, Metric.storageUsage) uses fresh metric values.
- Around line 19-41: The hook useVMTotalsMetrics is missing an explicit return
type; add a return type annotation to its signature (for example a specific
interface or Record keyed by Metric that maps to string, e.g., Record<Metric,
string> or a named UseVMTotalsMetricsResult) so the exported hook has an
explicit type; adjust imports to include any new type name and ensure the
annotated type matches the object returned (keys like Metric.cpuRequested,
Metric.cpuUsage, etc.).
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts:
- Around line 21-25: The exported function getCpuRequestedText is missing an
explicit return type; update its signature to include the return type string
(e.g., change getCpuRequestedText = (vmis: V1VirtualMachineInstance[]) => { ...
} to declare : string) so the function explicitly returns a string and satisfies
the coding guideline for exported functions.
- Around line 14-19: The exported function getValueWithUnitText lacks an
explicit return type; update its declaration to include a string return type
(e.g., annotate getValueWithUnitText with : string) while keeping the existing
use of findUnit and getHumanizedValue unchanged so the function still computes
unit and value and returns the formatted `${value} ${unit}` string.
- Around line 27-32: The exported function getCpuUsageText lacks an explicit
return type; update its signature to include a return type annotation (e.g.,
change export const getCpuUsageText = (cpuUsage: number) to export const
getCpuUsageText = (cpuUsage: number): string) so the function explicitly returns
a string (covering NO_DATA_DASH and humanizeCpuCores(cpuUsage).string).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
- src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts
- src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
- src/views/virtualmachines/list/hooks/utils.ts
- src/views/virtualmachines/list/hooks/constants.ts
- src/views/virtualmachines/list/metrics.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧠 Learnings (5)
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Applied to files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always explicitly define return types for functions rather than relying on TypeScript type inference.
Applied to files:
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/VirtualMachinesList.tsx
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧬 Code graph analysis (4)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)src/views/virtualmachines/list/metrics.ts (1)
setMetricFromResponse(64-73)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (5)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(34-40)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)src/views/virtualmachines/utils/mappers.ts (1)
getVMIFromMapper(26-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
46-74: LGTM! Clean integration of storage metrics.The storage metrics (STORAGE_USAGE and STORAGE_CAPACITY) are properly integrated using the same pattern as existing metrics. The unified
setMetricFromResponseapproach with theMetricenum provides consistency across all metric types.src/views/virtualmachines/list/VirtualMachinesList.tsx (2)
114-114: LGTM!Correctly destructures the updated return value from
useVirtualMachineInstanceMapper.
130-133: LGTM! Correct VMI derivation from filtered VMs.The
filteredVMIsare properly derived fromfilteredVMsusing the mapper, withfilter(Boolean)correctly removing any null entries. TheuseMemodependencies are accurate.src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx (1)
10-18: LGTM! Props correctly extended for combined VM/VMI metrics.The component now properly accepts and passes both
vmisandvmstouseVMTotalsMetrics, enabling aggregated metrics calculation across filtered VMs.
| const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps) => { | ||
| return useMemo(() => { | ||
| const totalCpuUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.cpuUsage ?? 0), 0); | ||
| const totalMemoryUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.memoryUsage ?? 0), 0); | ||
| const totalStorageUsage = vms.reduce( | ||
| (acc, vm) => acc + (getVMMetrics(vm)?.storageUsage ?? 0), | ||
| 0, | ||
| ); | ||
| const totalStorageCapacity = vms.reduce( | ||
| (acc, vm) => acc + (getVMMetrics(vm)?.storageCapacity ?? 0), | ||
| 0, | ||
| ); | ||
|
|
||
| return { | ||
| [Metric.cpuRequested]: getCpuRequestedText(vmis), | ||
| [Metric.cpuUsage]: getCpuUsageText(totalCpuUsage), | ||
| [Metric.memoryCapacity]: getMemoryCapacityText(vmis), | ||
| [Metric.memoryUsage]: getValueWithUnitText(totalMemoryUsage, METRICS.MEMORY), | ||
| [Metric.storageCapacity]: getValueWithUnitText(totalStorageCapacity, METRICS.STORAGE), | ||
| [Metric.storageUsage]: getValueWithUnitText(totalStorageUsage, METRICS.STORAGE), | ||
| }; | ||
| }, [vms, vmis]); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type to the hook.
The hook function is missing an explicit return type annotation, which violates the coding guideline requiring all functions to define explicit return types.
🔎 Proposed fix
-const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps) => {
+const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps): {
+ cpuRequested: string;
+ cpuUsage: string;
+ memoryCapacity: string;
+ memoryUsage: string;
+ storageCapacity: string;
+ storageUsage: string;
+} => {
return useMemo(() => {
const totalCpuUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.cpuUsage ?? 0), 0);As per coding guidelines, all exported functions should have explicit return types.
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts around lines 19 -
41, The hook useVMTotalsMetrics is missing an explicit return type; add a return
type annotation to its signature (for example a specific interface or Record
keyed by Metric that maps to string, e.g., Record<Metric, string> or a named
UseVMTotalsMetricsResult) so the exported hook has an explicit type; adjust
imports to include any new type name and ensure the annotated type matches the
object returned (keys like Metric.cpuRequested, Metric.cpuUsage, etc.).
|
/retest |
adamviktora
force-pushed
the
usage-summary-filtered-vms
branch
from
41a81c2 to
691b714
Compare
|
New changes are detected. LGTM label has been removed. |
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/list/metrics.ts (1)
42-51: Add explicit return type annotation.This exported function is missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
export const getVMMetricsWithParams = ( name: string, namespace: string, cluster = SINGLE_CLUSTER_KEY, -) => { +): MetricsObject => { const vmMetrics = vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; if (isEmpty(vmMetrics)) vmsMetrics.value[`${cluster}-${namespace}-${name}`] = {}; return vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; };As per coding guidelines, all exported functions should have explicit return types.
🤖 Fix all issues with AI agents
In @src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx:
- Line 16: NodesField currently only destructures vmiMapper from
useVirtualMachineInstanceMapper; extract vmisLoaded as well and use it to
control UI state in the NodesField component (similar to
VirtualMachinesList.tsx). Update the component to combine vmisLoaded with any
other loaded flags into a composed loaded variable, and while vmisLoaded is
false either disable the nodes input or display a loading indicator/spinner
until VMIs finish loading, ensuring you reference
useVirtualMachineInstanceMapper, vmiMapper and vmisLoaded in your changes.
In @src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts:
- Around line 10-14: The hook useVirtualMachineInstanceMapper lacks an explicit
return type; define a return type (e.g. UseVirtualMachineInstanceMapperResult
with properties vmiMapper: VMIMapper and vmisLoaded: boolean) and annotate the
exported hook with that type, then ensure the function returns an object
matching it (including the existing vmiMapper value and vmisLoaded boolean) so
the signature is explicit and type-safe.
In @src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts:
- Line 19: The hook useVMTotalsMetrics is missing an explicit return type; add a
typed return annotation to its declaration (e.g., declare and export a
type/interface like UseVMTotalsMetricsResult that describes the returned
object/values and change the signature to const useVMTotalsMetrics = ({ vmis,
vms }: UseVMTotalsMetricsProps): UseVMTotalsMetricsResult => { ... }); ensure
the new type accurately reflects the keys/types actually returned by
useVMTotalsMetrics.
- Around line 20-40: The memoization in useVMTotalsMetrics (the useMemo wrapping
the totals calculation using getVMMetrics(vm)) is stale because it only lists
[vms, vmis] so updates to the vmsMetrics signal don't retrigger recalculation;
remove the useMemo wrapper in useVMTotalsMetrics and return the computed totals
object directly (keep the same reductions and Metric keys) so signal-driven
re-renders pick up updated getVMMetrics results.
In @src/views/virtualmachines/list/metrics.ts:
- Around line 34-40: getVMMetrics lacks an explicit return type; add one to the
exported function signature. Use the return type of getVMMetricsWithParams to
keep it accurate (e.g. annotate getVMMetrics as (vm: V1VirtualMachine):
ReturnType<typeof getVMMetricsWithParams> => { ... }) so callers and
type-checking are explicit; reference the existing symbols getVMMetrics,
V1VirtualMachine, and getVMMetricsWithParams when updating the signature.
- Around line 64-73: Add an explicit return type to the exported function
setMetricFromResponse (e.g., : void) and guard against invalid numeric parsing
before calling setMetricValue: parse the string, verify the parsed value is a
finite number (e.g., using Number.isFinite or !Number.isNaN) and only call
setMetricValue when valid (or use a sensible default/skip storing when invalid)
to avoid storing NaN metric values.
- Around line 53-62: The function setMetricValue lacks an explicit return type;
add a return type annotation of void to its signature (e.g.,
setMetricValue(...): void) to satisfy the coding guideline, leaving the
implementation unchanged and keeping references to getVMMetricsWithParams,
SINGLE_CLUSTER_KEY and the Metric parameter intact.
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts:
- Around line 14-19: The exported function getValueWithUnitText lacks an
explicit return type; add a return type annotation of string to its signature
(e.g., getValueWithUnitText = (bytes: number, metric: string): string => { ...
}) so the function explicitly returns a string, keeping the existing calls to
findUnit and getHumanizedValue unchanged.
- Around line 27-32: The exported function getCpuUsageText lacks an explicit
return type; add a return type annotation (string) to its signature so it reads
something like getCpuUsageText(...): string, ensuring the two branches (return
NO_DATA_DASH and return humanizeCpuCores(cpuUsage).string) conform to that type;
update the function signature only (referencing getCpuUsageText, NO_DATA_DASH,
and humanizeCpuCores) to satisfy the coding guideline.
- Around line 21-25: Add an explicit return type to the exported function
getCpuRequestedText: declare its return type as string (e.g.,
getCpuRequestedText = (vmis: V1VirtualMachineInstance[]): string => { ... }) so
the function signature explicitly returns a string; update the function
signature near the existing getCpuRequestedText declaration and ensure
imports/types (V1VirtualMachineInstance) remain unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
- src/views/virtualmachines/list/hooks/utils.ts
- src/views/virtualmachines/list/VirtualMachinesList.tsx
- src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.ts
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
🧠 Learnings (5)
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Applied to files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always explicitly define return types for functions rather than relying on TypeScript type inference.
Applied to files:
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
🧬 Code graph analysis (4)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)src/views/virtualmachines/list/metrics.ts (1)
setMetricFromResponse(64-73)
src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx (1)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
useVirtualMachineInstanceMapper(10-43)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(34-40)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (5)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (10)
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts (1)
11-11: LGTM: Hook correctly exposes loading state.The changes correctly destructure
vmisLoadedfromuseKubevirtWatchResourceand expose it in the return value alongsidevmiMapper. This enables consumers to track when VMI data is fully loaded.Also applies to: 42-42
src/views/virtualmachines/list/hooks/constants.ts (1)
9-10: LGTM!The new storage metric constants follow the established naming pattern and integrate cleanly with the existing query keys.
src/views/virtualmachines/list/hooks/useVMMetrics.ts (4)
10-10: LGTM!The import correctly reflects the refactored metrics system using a unified approach.
46-54: LGTM!The storage usage Prometheus poll follows the established pattern for other metrics.
56-66: LGTM!The refactored metric dispatch using the
Metricenum addresses the previous feedback about avoiding hardcoded strings and provides better type safety.
68-74: LGTM!The new storage metrics hooks correctly follow the established pattern and use the
Metricenum for type-safe metric keys.src/views/virtualmachines/list/metrics.ts (2)
14-22: LGTM!The
Metricenum provides excellent type safety and eliminates magic strings throughout the codebase.
24-30: LGTM!The type definitions are well-structured and leverage the
Metricenum for type safety.src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
14-17: LGTM!The type definition clearly specifies the hook's input parameters.
32-39: LGTM!The use of
Metricenum keys with bracket notation provides excellent type safety and addresses previous feedback. This ensures the returned object keys are consistent with the centralized metric definitions.
| const { setValue, value } = useAdvancedSearchField(VirtualMachineRowFilterType.Node); | ||
|
|
||
| const vmiMapper = useVirtualMachineInstanceMapper(); | ||
| const { vmiMapper } = useVirtualMachineInstanceMapper(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check vmisLoaded usage and loading state patterns
# First, find the NodesField.tsx file and examine it
echo "=== Examining NodesField.tsx ==="
find . -name "NodesField.tsx" -type f | head -5
# Search for vmisLoaded usage across the codebase
echo -e "\n=== Searching for vmisLoaded usage ==="
rg -n 'vmisLoaded' --type=tsx --type=ts 2>/dev/null | head -20
# Search for useVirtualMachineInstanceMapper hook definition
echo -e "\n=== Searching for useVirtualMachineInstanceMapper hook ==="
rg -n 'useVirtualMachineInstanceMapper' --type=tsx --type=ts 2>/dev/null | head -20
# Check AdvancedSearchModal for loading state handling
echo -e "\n=== Searching for loading state in AdvancedSearchModal ==="
rg -n 'isLoading|loading|vmisLoaded' "src/views/search/components/AdvancedSearchModal/" --type=tsx --type=ts 2>/dev/nullRepository: kubevirt-ui/kubevirt-plugin
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Examine the actual files
echo "=== NodesField.tsx content ==="
cat -n ./src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
echo -e "\n=== Search for useVirtualMachineInstanceMapper definition ==="
find . -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "useVirtualMachineInstanceMapper" | grep -v node_modules
echo -e "\n=== Search for useVirtualMachineInstanceMapper in hooks ==="
find ./src -type f -name "*hook*" -o -name "*use*" | xargs grep -l "useVirtualMachineInstanceMapper" 2>/dev/null || echo "Not found in hooks directory"Repository: kubevirt-ui/kubevirt-plugin
Length of output: 2244
🏁 Script executed:
#!/bin/bash
# Examine the hook implementation
echo "=== useVirtualMachineInstanceMapper.ts content ==="
cat -n ./src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts
echo -e "\n=== Check useNodesFilter hook ==="
find ./src -type f -name "*useNodesFilter*" | head -5Repository: kubevirt-ui/kubevirt-plugin
Length of output: 2153
🏁 Script executed:
#!/bin/bash
# Search for vmisLoaded usage with better pattern matching
echo "=== Search for vmisLoaded in all files ==="
grep -r "vmisLoaded" ./src --include="*.ts" --include="*.tsx" -n
echo -e "\n=== Examine AdvancedSearchModal.tsx ==="
find ./src -name "AdvancedSearchModal.tsx" -type f | head -1 | xargs cat -n
echo -e "\n=== Check VirtualMachinesList.tsx for loading handling ==="
grep -n "vmisLoaded\|useVirtualMachineInstanceMapper" ./src/views/virtualmachines/list/VirtualMachinesList.tsx | head -20Repository: kubevirt-ui/kubevirt-plugin
Length of output: 7434
Extract and use vmisLoaded to handle loading state in NodesField.
The useVirtualMachineInstanceMapper hook returns vmisLoaded, but NodesField only destructures vmiMapper and ignores the loading flag. This pattern is inconsistent with how VirtualMachinesList.tsx uses the same hook—it extracts vmisLoaded and includes it in a composed loaded flag to control UI state. Without checking vmisLoaded, the component may render an incomplete nodes list while VMIs are still being fetched. Extract vmisLoaded from the hook and either disable the input or show a loading indicator while data is loading.
🤖 Prompt for AI Agents
In @src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx at
line 16, NodesField currently only destructures vmiMapper from
useVirtualMachineInstanceMapper; extract vmisLoaded as well and use it to
control UI state in the NodesField component (similar to
VirtualMachinesList.tsx). Update the component to combine vmisLoaded with any
other loaded flags into a composed loaded variable, and while vmisLoaded is
false either disable the nodes input or display a loading indicator/spinner
until VMIs finish loading, ensuring you reference
useVirtualMachineInstanceMapper, vmiMapper and vmisLoaded in your changes.
| export const useVirtualMachineInstanceMapper = () => { | ||
| const [vmis] = useKubevirtWatchResource<V1VirtualMachineInstance[]>({ | ||
| const [vmis, vmisLoaded] = useKubevirtWatchResource<V1VirtualMachineInstance[]>({ | ||
| groupVersionKind: VirtualMachineInstanceModelGroupVersionKind, | ||
| isList: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type for the hook.
The hook still lacks an explicit return type annotation, which violates the coding guideline requiring all functions to have explicit return types.
♻️ Proposed fix
First, define the return type (if not already present in a types file):
type UseVirtualMachineInstanceMapperResult = {
vmiMapper: VMIMapper;
vmisLoaded: boolean;
};Then apply the return type:
-export const useVirtualMachineInstanceMapper = () => {
+export const useVirtualMachineInstanceMapper = (): UseVirtualMachineInstanceMapperResult => {
const [vmis, vmisLoaded] = useKubevirtWatchResource<V1VirtualMachineInstance[]>({
groupVersionKind: VirtualMachineInstanceModelGroupVersionKind,
isList: true,
});As per coding guidelines, all exported functions should have explicit return types.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts
around lines 10 - 14, The hook useVirtualMachineInstanceMapper lacks an explicit
return type; define a return type (e.g. UseVirtualMachineInstanceMapperResult
with properties vmiMapper: VMIMapper and vmisLoaded: boolean) and annotate the
exported hook with that type, then ensure the function returns an object
matching it (including the existing vmiMapper value and vmisLoaded boolean) so
the signature is explicit and type-safe.
| storageCapacity: getMetricText(storageCapacityResponse, METRICS.STORAGE), | ||
| storageUsage: getMetricText(storageUsageResponse, METRICS.STORAGE), | ||
| }; | ||
| const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This hook is missing an explicit return type, which violates the coding guideline requiring all functions to define explicit return types.
📝 Suggested fix
+type UseVMTotalsMetricsResult = {
+ [Metric.cpuRequested]: string;
+ [Metric.cpuUsage]: string;
+ [Metric.memoryCapacity]: string;
+ [Metric.memoryUsage]: string;
+ [Metric.storageCapacity]: string;
+ [Metric.storageUsage]: string;
+};
+
-const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps) => {
+const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps): UseVMTotalsMetricsResult => {As per coding guidelines, all exported functions should have explicit return types.
📝 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 useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps) => { | |
| type UseVMTotalsMetricsResult = { | |
| [Metric.cpuRequested]: string; | |
| [Metric.cpuUsage]: string; | |
| [Metric.memoryCapacity]: string; | |
| [Metric.memoryUsage]: string; | |
| [Metric.storageCapacity]: string; | |
| [Metric.storageUsage]: string; | |
| }; | |
| const useVMTotalsMetrics = ({ vmis, vms }: UseVMTotalsMetricsProps): UseVMTotalsMetricsResult => { |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts at line 19, The
hook useVMTotalsMetrics is missing an explicit return type; add a typed return
annotation to its declaration (e.g., declare and export a type/interface like
UseVMTotalsMetricsResult that describes the returned object/values and change
the signature to const useVMTotalsMetrics = ({ vmis, vms }:
UseVMTotalsMetricsProps): UseVMTotalsMetricsResult => { ... }); ensure the new
type accurately reflects the keys/types actually returned by useVMTotalsMetrics.
| export const getValueWithUnitText = (bytes: number, metric: string) => { | ||
| const unit = findUnit(metric, bytes); | ||
| const value = getHumanizedValue(metric, bytes, unit).toLocaleString(); | ||
|
|
||
| return `${value} ${unit}`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This function is missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
-export const getValueWithUnitText = (bytes: number, metric: string) => {
+export const getValueWithUnitText = (bytes: number, metric: string): string => {
const unit = findUnit(metric, bytes);
const value = getHumanizedValue(metric, bytes, unit).toLocaleString();
return `${value} ${unit}`;
};As per coding guidelines, all exported functions should have explicit return types.
📝 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.
| export const getValueWithUnitText = (bytes: number, metric: string) => { | |
| const unit = findUnit(metric, bytes); | |
| const value = getHumanizedValue(metric, bytes, unit).toLocaleString(); | |
| return `${value} ${unit}`; | |
| }; | |
| export const getValueWithUnitText = (bytes: number, metric: string): string => { | |
| const unit = findUnit(metric, bytes); | |
| const value = getHumanizedValue(metric, bytes, unit).toLocaleString(); | |
| return `${value} ${unit}`; | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts around lines
14 - 19, The exported function getValueWithUnitText lacks an explicit return
type; add a return type annotation of string to its signature (e.g.,
getValueWithUnitText = (bytes: number, metric: string): string => { ... }) so
the function explicitly returns a string, keeping the existing calls to findUnit
and getHumanizedValue unchanged.
| export const getCpuRequestedText = (vmis: V1VirtualMachineInstance[]) => { | ||
| const totalCPU = vmis.map((vmi) => getCPU(vmi)).reduce((acc, cpu) => acc + getVCPUCount(cpu), 0); | ||
|
|
||
| if (isNaN(value)) { | ||
| return humanizeCpuCores(totalCPU).string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This function is missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
-export const getCpuRequestedText = (vmis: V1VirtualMachineInstance[]) => {
+export const getCpuRequestedText = (vmis: V1VirtualMachineInstance[]): string => {
const totalCPU = vmis.map((vmi) => getCPU(vmi)).reduce((acc, cpu) => acc + getVCPUCount(cpu), 0);
return humanizeCpuCores(totalCPU).string;
};As per coding guidelines, all exported functions should have explicit return types.
📝 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.
| export const getCpuRequestedText = (vmis: V1VirtualMachineInstance[]) => { | |
| const totalCPU = vmis.map((vmi) => getCPU(vmi)).reduce((acc, cpu) => acc + getVCPUCount(cpu), 0); | |
| if (isNaN(value)) { | |
| return humanizeCpuCores(totalCPU).string; | |
| }; | |
| export const getCpuRequestedText = (vmis: V1VirtualMachineInstance[]): string => { | |
| const totalCPU = vmis.map((vmi) => getCPU(vmi)).reduce((acc, cpu) => acc + getVCPUCount(cpu), 0); | |
| return humanizeCpuCores(totalCPU).string; | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts around lines
21 - 25, Add an explicit return type to the exported function
getCpuRequestedText: declare its return type as string (e.g.,
getCpuRequestedText = (vmis: V1VirtualMachineInstance[]): string => { ... }) so
the function signature explicitly returns a string; update the function
signature near the existing getCpuRequestedText declaration and ensure
imports/types (V1VirtualMachineInstance) remain unchanged.
| export const getCpuUsageText = (cpuUsage: number) => { | ||
| if (isNaN(cpuUsage)) { | ||
| return NO_DATA_DASH; | ||
| } | ||
| return humanizeCpuCores(value).string; | ||
| return humanizeCpuCores(cpuUsage).string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This function is missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
-export const getCpuUsageText = (cpuUsage: number) => {
+export const getCpuUsageText = (cpuUsage: number): string => {
if (isNaN(cpuUsage)) {
return NO_DATA_DASH;
}
return humanizeCpuCores(cpuUsage).string;
};As per coding guidelines, all exported functions should have explicit return types.
📝 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.
| export const getCpuUsageText = (cpuUsage: number) => { | |
| if (isNaN(cpuUsage)) { | |
| return NO_DATA_DASH; | |
| } | |
| return humanizeCpuCores(value).string; | |
| return humanizeCpuCores(cpuUsage).string; | |
| }; | |
| export const getCpuUsageText = (cpuUsage: number): string => { | |
| if (isNaN(cpuUsage)) { | |
| return NO_DATA_DASH; | |
| } | |
| return humanizeCpuCores(cpuUsage).string; | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts around lines
27 - 32, The exported function getCpuUsageText lacks an explicit return type;
add a return type annotation (string) to its signature so it reads something
like getCpuUsageText(...): string, ensuring the two branches (return
NO_DATA_DASH and return humanizeCpuCores(cpuUsage).string) conform to that type;
update the function signature only (referencing getCpuUsageText, NO_DATA_DASH,
and humanizeCpuCores) to satisfy the coding guideline.
691b714 to
a8146b4
Compare
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/virtualmachines/list/metrics.ts (1)
42-51: Add explicit return type annotation.This exported function is missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
export const getVMMetricsWithParams = ( name: string, namespace: string, cluster = SINGLE_CLUSTER_KEY, -) => { +): MetricsObject => { const vmMetrics = vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; if (isEmpty(vmMetrics)) vmsMetrics.value[`${cluster}-${namespace}-${name}`] = {}; return vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; };As per coding guidelines.
🤖 Fix all issues with AI agents
In @src/views/virtualmachines/list/metrics.ts:
- Around line 53-62: The function setMetricValue lacks an explicit return type;
update its signature to include an explicit return type (void) to comply with
the coding guideline. Locate the setMetricValue function and add the return type
annotation after the parameter list so the signature becomes
setMetricValue(...): void, leaving the body unchanged.
- Around line 64-73: setMetricFromResponse currently lacks an explicit return
type and doesn't guard against parseFloat producing NaN; update its signature to
include an explicit return type (e.g., void) and validate the parsed value
before calling setMetricValue: after parsing result?.value?.[1] into a number,
check Number.isFinite(value) (or !Number.isNaN(value)) and only call
setMetricValue(vmName, vmNamespace, vmCluster, metric, value) when valid,
otherwise skip or log/handle the invalid case; reference setMetricFromResponse,
PrometheusResponse, Metric, and setMetricValue when locating the change.
- Around line 34-40: The function getVMMetrics currently lacks an explicit
return type; add a return type annotation to its signature that matches what
getVMMetricsWithParams returns (either the concrete type it returns or use
ReturnType<typeof getVMMetricsWithParams>) so the exported function
getVMMetrics(vm: V1VirtualMachine) : <explicit-type> { ... } adheres to the
coding guideline; keep the existing logic using getCluster, getNamespace,
getName, SINGLE_CLUSTER_KEY and returning getVMMetricsWithParams(name,
namespace, cluster).
🧹 Nitpick comments (2)
src/multicluster/hooks/useVMListQueries.ts (1)
17-21: MemoizenonHubClustersto prevent unnecessary query recalculations.The
filtercall on line 17 creates a new array reference on every render, causinguseMemoon lines 19-22 to recalculate even when the underlyingclustersandhubClusterNamehaven't changed.♻️ Suggested fix
+ const nonHubClusters = useMemo( + () => clusters?.filter((c) => c !== hubClusterName), + [clusters, hubClusterName], + ); - const nonHubClusters = clusters?.filter((c) => c !== hubClusterName); const queries = useMemo( () => getVMListQueries(namespaces, nonHubClusters, isAllClustersPage), [namespaces, nonHubClusters, isAllClustersPage], );src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
36-39: Inconsistent query access pattern.Line 38 uses dot notation (
queries?.NETWORK_TOTAL_USAGE) while other queries use bracket notation with theVMListQueriesconstant (e.g.,queries?.[VMListQueries.MEMORY_USAGE]). Use consistent bracket notation for maintainability.♻️ Suggested fix
const [networkTotalResponse] = useFleetPrometheusPoll({ ...prometheusPollProps, - query: queries?.NETWORK_TOTAL_USAGE, + query: queries?.[VMListQueries.NETWORK_TOTAL_USAGE], });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/multicluster/hooks/useVMListQueries.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.ts
- src/views/virtualmachines/list/hooks/utils.ts
- src/views/virtualmachines/list/VirtualMachinesList.tsx
- src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/multicluster/hooks/useVMListQueries.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/multicluster/hooks/useVMListQueries.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧠 Learnings (6)
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Applied to files:
src/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always explicitly define return types for functions rather than relying on TypeScript type inference.
Applied to files:
src/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always specify dependencies in `useEffect` to avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array `[]` to run the effect only once.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/multicluster/hooks/useVMListQueries.ts
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧬 Code graph analysis (5)
src/views/virtualmachines/list/hooks/useVMMetrics.ts (2)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)src/views/virtualmachines/list/metrics.ts (1)
setMetricFromResponse(64-73)
src/views/virtualmachines/list/metrics.ts (3)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/constants.ts (1)
SINGLE_CLUSTER_KEY(1-1)src/utils/resources/shared.ts (2)
getNamespace(341-342)getName(333-334)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(34-40)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
src/multicluster/hooks/useVMListQueries.ts (1)
src/views/virtualmachines/list/hooks/utils.ts (1)
getVMListQueries(26-46)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (6)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/utils/utils/humanize.js (1)
value(234-234)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
src/views/virtualmachines/list/hooks/constants.ts (1)
9-10: LGTM!The new storage query keys follow the existing naming convention and integrate cleanly with the
VMListQueriesobject.src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx (1)
43-43: LGTM!The updated prop passing aligns with the refactored
VirtualMachineUsagecomponent signature and enables the usage summary to reflect filtered VMs.src/views/virtualmachines/list/hooks/useVMMetrics.ts (1)
46-74: LGTM - Storage metrics integration.The new Prometheus polls for storage usage and capacity are correctly wired, and the
useEffecthooks properly dispatch metrics with appropriate dependencies.src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx (1)
10-18: LGTM!The component correctly extends its props to include
vmsand passes bothvmisandvmstouseVMTotalsMetrics, enabling accurate totals computation for filtered VMs.src/views/virtualmachines/list/metrics.ts (4)
11-11: LGTM: Import supports new Prometheus response handling.The PrometheusResponse import is correctly added to support the new
setMetricFromResponsefunction.
14-22: LGTM: Well-structured enum for metric types.The Metric enum provides type-safe keys for all VM metrics, including the newly added storage metrics mentioned in the PR objectives.
24-30: LGTM: Type definitions follow guidelines.The type definitions correctly use
typeinstead ofinterfaceand provide strong typing for the metrics storage structure.
32-32: LGTM: Improved encapsulation.Making
vmsMetricsinternal (not exported) is a good practice that enforces controlled access through the public API functions.
| export const getVMMetrics = (vm: V1VirtualMachine) => { | ||
| const cluster = getCluster(vm) || SINGLE_CLUSTER_KEY; | ||
| const namespace = getNamespace(vm); | ||
| const name = getName(vm); | ||
| const vmMetrics = vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; | ||
| if (isEmpty(vmMetrics)) vmsMetrics.value[`${cluster}-${namespace}-${name}`] = {}; | ||
|
|
||
| return vmsMetrics.value?.[`${cluster}-${namespace}-${name}`]; | ||
| return getVMMetricsWithParams(name, namespace, cluster); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This exported function is still missing a return type, which violates the coding guideline requiring explicit return types for all functions.
📝 Suggested fix
-export const getVMMetrics = (vm: V1VirtualMachine) => {
+export const getVMMetrics = (vm: V1VirtualMachine): MetricsObject => {
const cluster = getCluster(vm) || SINGLE_CLUSTER_KEY;
const namespace = getNamespace(vm);
const name = getName(vm);
return getVMMetricsWithParams(name, namespace, cluster);
};As per coding guidelines.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/metrics.ts around lines 34 - 40, The function
getVMMetrics currently lacks an explicit return type; add a return type
annotation to its signature that matches what getVMMetricsWithParams returns
(either the concrete type it returns or use ReturnType<typeof
getVMMetricsWithParams>) so the exported function getVMMetrics(vm:
V1VirtualMachine) : <explicit-type> { ... } adheres to the coding guideline;
keep the existing logic using getCluster, getNamespace, getName,
SINGLE_CLUSTER_KEY and returning getVMMetricsWithParams(name, namespace,
cluster).
| const setMetricValue = ( | ||
| name: string, | ||
| namespace: string, | ||
| cluster = SINGLE_CLUSTER_KEY, | ||
| memoryUsage: number, | ||
| metric: Metric, | ||
| value: number, | ||
| ) => { | ||
| const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); | ||
|
|
||
| vmMetrics.memoryUsage = memoryUsage; | ||
| vmMetrics[metric] = value; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type annotation.
This function is still missing a return type. While it's not exported, the coding guideline requires explicit return types for all functions.
📝 Suggested fix
const setMetricValue = (
name: string,
namespace: string,
cluster = SINGLE_CLUSTER_KEY,
metric: Metric,
value: number,
-) => {
+): void => {
const vmMetrics = getVMMetricsWithParams(name, namespace, cluster);
vmMetrics[metric] = value;
};As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const setMetricValue = ( | |
| name: string, | |
| namespace: string, | |
| cluster = SINGLE_CLUSTER_KEY, | |
| memoryUsage: number, | |
| metric: Metric, | |
| value: number, | |
| ) => { | |
| const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); | |
| vmMetrics.memoryUsage = memoryUsage; | |
| vmMetrics[metric] = value; | |
| }; | |
| const setMetricValue = ( | |
| name: string, | |
| namespace: string, | |
| cluster = SINGLE_CLUSTER_KEY, | |
| metric: Metric, | |
| value: number, | |
| ): void => { | |
| const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); | |
| vmMetrics[metric] = value; | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/metrics.ts around lines 53 - 62, The function
setMetricValue lacks an explicit return type; update its signature to include an
explicit return type (void) to comply with the coding guideline. Locate the
setMetricValue function and add the return type annotation after the parameter
list so the signature becomes setMetricValue(...): void, leaving the body
unchanged.
| export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric) => { | ||
| response?.data?.result?.forEach((result) => { | ||
| const vmName = result?.metric?.name; | ||
| const vmNamespace = result?.metric?.namespace; | ||
| const vmCluster = result?.metric?.cluster; | ||
| const value = parseFloat(result?.value?.[1]); | ||
|
|
||
| export const setVMCPUUsage = ( | ||
| name: string, | ||
| namespace: string, | ||
| cluster = SINGLE_CLUSTER_KEY, | ||
| cpuUsage: number, | ||
| ) => { | ||
| const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); | ||
| vmMetrics.cpuUsage = cpuUsage; | ||
| setMetricValue(vmName, vmNamespace, vmCluster, metric, value); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return type and consider NaN handling.
This exported function is still missing a return type, which violates the coding guideline. Additionally, parseFloat can return NaN if the value is invalid, which may lead to incorrect metric values being stored and could break percentage calculations.
📝 Suggested fix
-export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric) => {
+export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric): void => {
response?.data?.result?.forEach((result) => {
const vmName = result?.metric?.name;
const vmNamespace = result?.metric?.namespace;
const vmCluster = result?.metric?.cluster;
const value = parseFloat(result?.value?.[1]);
+ if (!Number.isFinite(value)) return;
+
setMetricValue(vmName, vmNamespace, vmCluster, metric, value);
});
};As per coding guidelines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric) => { | |
| response?.data?.result?.forEach((result) => { | |
| const vmName = result?.metric?.name; | |
| const vmNamespace = result?.metric?.namespace; | |
| const vmCluster = result?.metric?.cluster; | |
| const value = parseFloat(result?.value?.[1]); | |
| export const setVMCPUUsage = ( | |
| name: string, | |
| namespace: string, | |
| cluster = SINGLE_CLUSTER_KEY, | |
| cpuUsage: number, | |
| ) => { | |
| const vmMetrics = getVMMetricsWithParams(name, namespace, cluster); | |
| vmMetrics.cpuUsage = cpuUsage; | |
| setMetricValue(vmName, vmNamespace, vmCluster, metric, value); | |
| }); | |
| }; | |
| export const setMetricFromResponse = (response: PrometheusResponse, metric: Metric): void => { | |
| response?.data?.result?.forEach((result) => { | |
| const vmName = result?.metric?.name; | |
| const vmNamespace = result?.metric?.namespace; | |
| const vmCluster = result?.metric?.cluster; | |
| const value = parseFloat(result?.value?.[1]); | |
| if (!Number.isFinite(value)) return; | |
| setMetricValue(vmName, vmNamespace, vmCluster, metric, value); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In @src/views/virtualmachines/list/metrics.ts around lines 64 - 73,
setMetricFromResponse currently lacks an explicit return type and doesn't guard
against parseFloat producing NaN; update its signature to include an explicit
return type (e.g., void) and validate the parsed value before calling
setMetricValue: after parsing result?.value?.[1] into a number, check
Number.isFinite(value) (or !Number.isNaN(value)) and only call
setMetricValue(vmName, vmNamespace, vmCluster, metric, value) when valid,
otherwise skip or log/handle the invalid case; reference setMetricFromResponse,
PrometheusResponse, Metric, and setMetricValue when locating the change.
adamviktora
force-pushed
the
usage-summary-filtered-vms
branch
from
a8146b4 to
5914de6
Compare
|
@adamviktora: This pull request references CNV-75366 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx (1)
15-15: Consider wrapping the component with React.memo.This component receives array props (
vmis,vms) that may trigger unnecessary re-renders when parent components update. Wrapping it withReact.memowould optimize rendering performance by preventing re-renders when props haven't changed.♻️ Proposed refactor
-const VirtualMachineUsage: FC<VirtualMachineUsageProps> = ({ vmis, vms }) => { +const VirtualMachineUsage: FC<VirtualMachineUsageProps> = React.memo(({ vmis, vms }) => { const { t } = useKubevirtTranslation(); const { cpuRequested, cpuUsage, memoryCapacity, memoryUsage, storageCapacity, storageUsage } = useVMTotalsMetrics({ vmis, vms }); return ( <Card> <CardTitle>{t('Usage')}</CardTitle> <CardBody> <Flex justifyContent={{ default: 'justifyContentSpaceBetween' }} spaceItems={{ default: 'spaceItems2xl' }} > <VirtualMachineUsageItem capacityText={t('Requested of {{cpuRequested}}', { cpuRequested })} metricName={t('CPU')} usageText={cpuUsage} /> <VirtualMachineUsageItem capacityText={t('Used of {{value}}', { value: memoryCapacity })} metricName={t('Memory')} usageText={memoryUsage} /> <VirtualMachineUsageItem capacityText={t('Used of {{value}}', { value: storageCapacity })} metricName={t('Storage')} usageText={storageUsage} /> </Flex> </CardBody> </Card> ); -}; +}); + +VirtualMachineUsage.displayName = 'VirtualMachineUsage'; export default VirtualMachineUsage;As per coding guidelines, React.memo should be used to avoid unnecessary re-renders.
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (1)
18-24: Consider consolidating reduce operations to avoid redundantgetVMMetricscalls.
getVMMetrics(vm)is called 4 times per VM (once in each reduce). Since this performs a signal lookup for each call, consolidating into a single iteration would be more efficient.♻️ Proposed refactor
- const totalCpuUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.cpuUsage ?? 0), 0); - const totalMemoryUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.memoryUsage ?? 0), 0); - const totalStorageUsage = vms.reduce((acc, vm) => acc + (getVMMetrics(vm)?.storageUsage ?? 0), 0); - const totalStorageCapacity = vms.reduce( - (acc, vm) => acc + (getVMMetrics(vm)?.storageCapacity ?? 0), - 0, - ); + const { totalCpuUsage, totalMemoryUsage, totalStorageUsage, totalStorageCapacity } = vms.reduce( + (acc, vm) => { + const metrics = getVMMetrics(vm); + return { + totalCpuUsage: acc.totalCpuUsage + (metrics?.cpuUsage ?? 0), + totalMemoryUsage: acc.totalMemoryUsage + (metrics?.memoryUsage ?? 0), + totalStorageUsage: acc.totalStorageUsage + (metrics?.storageUsage ?? 0), + totalStorageCapacity: acc.totalStorageCapacity + (metrics?.storageCapacity ?? 0), + }; + }, + { totalCpuUsage: 0, totalMemoryUsage: 0, totalStorageUsage: 0, totalStorageCapacity: 0 }, + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/multicluster/hooks/useVMListQueries.tssrc/views/search/components/AdvancedSearchModal/formFields/NodesField.tsxsrc/views/virtualmachines/list/VirtualMachinesList.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsxsrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/constants.tssrc/views/virtualmachines/list/hooks/useVMMetrics.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/utils/totalsQueries.ts
💤 Files with no reviewable changes (1)
- src/views/virtualmachines/list/utils/totalsQueries.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/views/virtualmachines/list/VirtualMachinesList.tsx
- src/views/search/components/AdvancedSearchModal/formFields/NodesField.tsx
- src/views/virtualmachines/list/hooks/constants.ts
- src/views/virtualmachines/list/components/VirtualMachineListSummary/VirtualMachineListSummary.tsx
- src/views/virtualmachines/list/hooks/useVMMetrics.ts
- src/multicluster/hooks/useVMListQueries.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.ts: Use.tsextension for non-component files containing logic or utilities.
Hooks should contain only logic and side effects, not return JSX. Keep JSX in components.
Files:
src/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{ts,tsx}: Use React's memoization tools (React.memo,useMemo,useCallback) to avoid unnecessary re-renders.
Always specify dependencies inuseEffectto avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array[]to run the effect only once.
Define constants in utility files with uppercase and underscore-separated naming (e.g.,API_URL).
Use descriptive names for variables, functions, and components. Avoid abbreviations unless widely recognized.
Prefer usingtypeinstead ofinterfacefor defining the shapes of objects or functions in TypeScript.
If a type is exported, add it to a utility file.
Avoidanytype in TypeScript. Useunknowninstead and narrow the type as needed.
Always explicitly define return types for functions rather than relying on TypeScript type inference.
Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Use ESLint with React and TypeScript plugins for consistent linting.
Files:
src/views/virtualmachines/list/hooks/utils.tssrc/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsxsrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
**/*.tsx
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.tsx: Use.tsxextension for all React components. One component per file with no nested components.
Use PascalCase for all React component names (e.g.,HeaderTop.tsx).
Use functional components as the default. Use class components only for specific lifecycle methods unavailable in functional components (e.g.,componentDidCatch).
Use default exports for all React components.
Lazy load components withReact.lazyandSuspense.
Files:
src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
🧠 Learnings (6)
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always explicitly define return types for functions rather than relying on TypeScript type inference.
Applied to files:
src/views/virtualmachines/list/utils/processVMTotalsMetrics.tssrc/views/virtualmachines/list/metrics.tssrc/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Avoid hardcoded values (magic numbers) and define them as constants for easy adjustments and readability.
Applied to files:
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts
📚 Learning: 2025-12-09T17:30:56.131Z
Learnt from: rszwajko
Repo: kubevirt-ui/kubevirt-plugin PR: 3235
File: src/views/checkups/self-validation/details/tabs/details/components/CheckupsSelfValidationDetailsDescriptionList.tsx:26-93
Timestamp: 2025-12-09T17:30:56.131Z
Learning: When using the Timestamp component from openshift-console/dynamic-plugin-sdk, you can pass sentinel values like NO_DATA_DASH directly to the timestamp prop without wrapping in conditional rendering. The component handles invalid/missing values gracefully. This applies to all TSX files that render the Timestamp component; ensure you do not add extra conditional logic for such values and rely on the component's internal handling. Reference: OpenShift Console Timestamp.tsx implementation.
Applied to files:
src/views/virtualmachines/list/components/VirtualMachineListSummary/components/VirtualMachineUsage.tsx
📚 Learning: 2025-12-12T17:26:25.615Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3165
File: src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts:25-27
Timestamp: 2025-12-12T17:26:25.615Z
Learning: In the kubevirt-ui/kubevirt-plugin repository: The `useActiveNamespace` hook has two different implementations with different return types:
1. When imported from `'openshift-console/dynamic-plugin-sdk'`, it returns a tuple and should be destructured: `const [namespace] = useActiveNamespace()`
2. When imported from `'kubevirt-utils/hooks/useActiveNamespace'`, it returns a single value: `const namespace = useActiveNamespace()`
Applied to files:
src/views/virtualmachines/list/hooks/useVirtualMachineInstanceMapper.tssrc/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Use React's memoization tools (`React.memo`, `useMemo`, `useCallback`) to avoid unnecessary re-renders.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
📚 Learning: 2026-01-06T10:11:44.320Z
Learnt from: CR
Repo: kubevirt-ui/kubevirt-plugin PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2026-01-06T10:11:44.320Z
Learning: Applies to **/*.{ts,tsx} : Always specify dependencies in `useEffect` to avoid unnecessary re-renders or missed updates. If no dependencies are required, pass an empty array `[]` to run the effect only once.
Applied to files:
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts
🧬 Code graph analysis (3)
src/views/virtualmachines/list/hooks/utils.ts (1)
src/views/virtualmachines/list/hooks/constants.ts (1)
VMListQueries(4-11)
src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (5)
src/views/clusteroverview/OverviewTab/metric-charts-card/utils/hooks/utils.ts (1)
findUnit(30-31)src/views/clusteroverview/TopConsumersTab/utils/utils.ts (1)
getHumanizedValue(50-53)src/utils/resources/vm/utils/selectors.ts (1)
getCPU(257-257)src/utils/resources/vm/utils/utils.ts (1)
getVCPUCount(34-35)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (2)
src/views/virtualmachines/list/metrics.ts (1)
getVMMetrics(34-40)src/views/virtualmachines/list/utils/processVMTotalsMetrics.ts (4)
getCpuRequestedText(21-25)getCpuUsageText(27-32)getMemoryCapacityText(34-43)getValueWithUnitText(14-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
src/views/virtualmachines/list/hooks/utils.ts (1)
43-44: LGTM! Storage metrics queries are well-formed.The PromQL queries correctly use
max()aggregation with proper grouping by namespace, name, disk_name, and cluster, consistent with the existing metric query patterns.src/views/virtualmachines/list/hooks/useVMTotalsMetrics.ts (3)
1-10: Imports look good.All imports are used and appropriately organized. The switch to
getCpuUsageTextandgetValueWithUnitTextaligns with the refactored utility functions.
12-15: Type definition is appropriate.The props type is well-defined and correctly scoped as internal to this module.
26-36: Return structure and export look good.The return object correctly uses the
Metricenum for computed property keys, and the formatting functions are appropriately applied. Good decision to removeuseMemosincegetVMMetricsreads from a Preact signal, ensuring the component re-renders when metrics update.
|
@galkremer1 because this PR changes the way we count totals in |
5 participants
📝 Description
Reflect filtered VMs in Usage summary
vmsMetricssignal, which stores metrics for individual VMs, it adds Storage metrics to it🎥 Demo
Before:
bug_usage_should_reflect_filters.mp4
After:
Screen.Recording.2025-12-22.at.17.34.26.mp4
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.