CNV-69204: make checkups lists multicluster #3165
Conversation
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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: upalatucci The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughAdds multicluster-aware Checkups: navigation and route extensions, cluster-scoped URL helpers, and widespread changes converting watches to multicluster-aware hooks, adding ACM-aware filters/columns, selected-row cluster/project filters, and MulticlusterResourceLink-based rendering across network and storage checkups. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts (1)
102-103: Loading flag flips false too early
useKubevirtWatchResourcemirrorsuseK8sWatchResourceby returning “loaded” booleans. By chaining the negated flags with&&, the hook reportsloading=falseimmediately after the first watch resolves, even if the other resources are still fetching. Please deriveloadingfrom an OR of the negated loaded flags (or equivalently negate the AND of the positives) so the UI waits for all four watches to complete.- loading: - !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding && !loadedClusterRoleBinding, + loading: + !loadingServiceAccounts || + !loadingRoles || + !loadingRolesBinding || + !loadedClusterRoleBinding,src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
87-88: Loading flag becomes false while watches are still pendingFor the network permissions hook we have the same
&&chain of negated loaded flags, soloadingflips tofalseonce the first resource finishes even if the rest are still streaming. Switch to an OR of the negated loaded flags (or negate the AND of the positives) so the consumer only proceeds after every watch completes.- loading: !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding, + loading: + !loadingServiceAccounts || + !loadingRoles || + !loadingRolesBinding,src/views/checkups/network/hooks/useCheckupsNetworkData.ts (1)
64-68: Fix loading aggregation semantics.
useKubevirtWatchResourcereturns the in-flight flag as its second tuple entry. UsingloadingConfigMap && loadingJobsflips the meaning—once either watch resolves,loadingbecomesfalseeven if the other watch is still busy, so consumers stop showing a spinner while data is incomplete. Please keeploadingtrue until both calls finish.- loading: loadingConfigMap && loadingJobs, + loading: loadingConfigMap || loadingJobs,src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (1)
64-68: Keep loading true until both watches settle.
loadingConfigMap && loadingJobsturnsloadingoff as soon as one watch finishes, leaving the UI believing everything is ready while the other fetch still runs. Use OR (or invert a combinedloadedflag) soloadingonly becomesfalseafter both resources are done.- loading: loadingConfigMap && loadingJobs, + loading: loadingConfigMap || loadingJobs,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/multicluster/extensions.ts(2 hunks)src/utils/resources/checkups/urls.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkData.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts(2 hunks)src/views/checkups/network/list/CheckupsNetworkList.tsx(4 hunks)src/views/checkups/network/list/CheckupsNetworkListRow.tsx(3 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts(2 hunks)src/views/checkups/storage/list/CheckupsStorageList.tsx(4 hunks)src/views/checkups/storage/list/CheckupsStorageListRow.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (3)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (4)
columnsSorting(26-37)STATUS_SUCCEEDED(16-16)STATUS_START_TIME_STAMP(14-14)STATUS_COMPILATION_TIME_STAMP(17-17)src/views/checkups/network/utils/utils.ts (7)
CONFIG_PARAM_NAD_NAME(51-51)STATUS_MAX_LATENCY_NANO(41-41)CONFIG_PARAM_SAMPLE_DURATION(49-49)STATUS_SOURCE_NODE(45-45)CONFIG_PARAM_SOURCE_NODE(48-48)STATUS_TARGET_NODE(44-44)CONFIG_PARAM_TARGET_NODE(47-47)
src/views/checkups/network/list/CheckupsNetworkList.tsx (1)
src/utils/utils/utils.ts (1)
isEmpty(32-33)
src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts (1)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts (2)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (5)
columnsSorting(26-37)STATUS_SUCCEEDED(16-16)STATUS_FAILURE_REASON(15-15)STATUS_START_TIME_STAMP(14-14)STATUS_COMPILATION_TIME_STAMP(17-17)
src/views/checkups/network/list/CheckupsNetworkListRow.tsx (6)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/checkups/utils/utils.ts (1)
getJobByName(46-60)src/utils/resources/checkups/urls.ts (1)
getNetworkCheckupURL(1-5)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)src/views/checkups/network/utils/utils.ts (1)
CONFIG_PARAM_NAD_NAME(51-51)src/utils/resources/vm/utils/constants.ts (1)
NO_DATA_DASH(3-3)
src/views/checkups/network/hooks/useCheckupsNetworkData.ts (3)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(12-12)src/views/checkups/network/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL_VALUE(37-37)src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
src/views/checkups/storage/list/CheckupsStorageList.tsx (1)
src/utils/utils/utils.ts (1)
isEmpty(32-33)
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (4)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/checkups/utils/utils.ts (1)
getJobByName(46-60)src/utils/resources/checkups/urls.ts (1)
getStorageCheckupURL(7-11)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)
src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (3)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(12-12)src/views/checkups/storage/utils/utils.ts (1)
KUBEVIRT_STORAGE_LABEL_VALUE(28-28)src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: i18n
- GitHub Check: unit-test
🔇 Additional comments (2)
src/multicluster/extensions.ts (1)
130-144: Checkups nav entry aligns with existing perspective itemsThe new fleet virtualization navigation item matches the surrounding extensions (data attributes, perspective, path), so it plugs cleanly into the ACM perspective menu.
src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts (1)
30-37: Stable filter arrays avoid needless re-rendersMemoizing
filtersWithSelect(and reusing it when composingallFilters) keeps the filter references stable, which helps the list page filter logic behave deterministically. Nice touch.
| export const getNetworkCheckupURL = (name: string, namespace: string, cluster?: string) => { | ||
| return cluster | ||
| ? `/k8s/cluster/${cluster}/ns/${namespace}/checkups/${name}` | ||
| : `/k8s/ns/${namespace}/checkups/network/${name}`; |
There was a problem hiding this comment.
Fix cluster path for network checkup URL
When cluster is set we drop the network segment, so generated links hit /checkups/${name} instead of /checkups/network/${name}, yielding a 404 for multicluster navigation. Please keep the segment aligned with the namespace-scoped branch.
return cluster
- ? `/k8s/cluster/${cluster}/ns/${namespace}/checkups/${name}`
+ ? `/k8s/cluster/${cluster}/ns/${namespace}/checkups/network/${name}`
: `/k8s/ns/${namespace}/checkups/network/${name}`;🤖 Prompt for AI Agents
In src/utils/resources/checkups/urls.ts around lines 1 to 4, the cluster-scoped
branch drops the "network" segment so generated URLs become
/k8s/cluster/<cluster>/ns/<namespace>/checkups/<name> and 404; change the
cluster branch to include the network segment exactly like the namespace-scoped
branch (i.e. return
/k8s/cluster/${cluster}/ns/${namespace}/checkups/network/${name}) so both
branches produce consistent network checkup URLs.
| if ( | ||
| isEmpty(configMaps) && | ||
| loading && | ||
| !loadingPermissions && | ||
| loadedColumns && | ||
| isEmpty(clusters) && | ||
| isEmpty(projects) |
There was a problem hiding this comment.
Fix Set emptiness check so the EmptyState still renders
useSelectedRowFilterClusters() / useSelectedRowFilterProjects() return Set<string> (from useSelectedRowFilters). Our shared isEmpty helper only treats arrays and plain objects as empty, so this condition now always evaluates to false and the CheckupsNetworkListEmptyState never shows up, even when nothing is selected. Please gate on .size for Sets (and fall back to isEmpty for the pre-existing shapes).
- isEmpty(clusters) &&
- isEmpty(projects)
+ (clusters instanceof Set ? clusters.size === 0 : isEmpty(clusters)) &&
+ (projects instanceof Set ? projects.size === 0 : isEmpty(projects))🤖 Prompt for AI Agents
In src/views/checkups/network/list/CheckupsNetworkList.tsx around lines 47 to
53, the condition uses isEmpty(...) for clusters and projects but those values
can be Set<string>, so the check always fails; update the condition to test Set
emptiness via .size (e.g. clusters instanceof Set ? clusters.size === 0 :
isEmpty(clusters)) and do the same for projects (leave existing isEmpty behavior
for arrays/objects as fallback) so the EmptyState renders when no selections
exist.
upalatucci
force-pushed
the
checkups-list
branch
from
9e01243 to
03cb345
Compare
upalatucci
changed the title
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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. |
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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. |
|
/retest |
1 similar comment
|
/retest |
upalatucci
force-pushed
the
checkups-list
branch
from
03cb345 to
222d7ca
Compare
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/checkups/network/hooks/useCheckupsNetworkData.ts (1)
64-68: Fix loading aggregation to avoid premature “loaded” state.Line 67 uses
loadingConfigMap && loadingJobs, so as soon as either watch resolves the combined loading flips tofalse, even if the other watch is still fetching. This causes consumers to render as if everything finished whilejobs(orconfigMaps) are still incomplete. Switch to logical OR so loading only clears once both resources finish.- loading: loadingConfigMap && loadingJobs, + loading: loadingConfigMap || loadingJobs,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/multicluster/extensions.ts(2 hunks)src/utils/resources/checkups/urls.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkData.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts(2 hunks)src/views/checkups/network/list/CheckupsNetworkList.tsx(4 hunks)src/views/checkups/network/list/CheckupsNetworkListRow.tsx(3 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts(2 hunks)src/views/checkups/storage/list/CheckupsStorageList.tsx(4 hunks)src/views/checkups/storage/list/CheckupsStorageListRow.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/utils/resources/checkups/urls.ts
- src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts
- src/views/checkups/storage/list/CheckupsStorageList.tsx
- src/views/checkups/network/list/CheckupsNetworkListRow.tsx
- src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts
- src/views/checkups/network/list/CheckupsNetworkList.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts (1)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (3)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (4)
columnsSorting(26-37)STATUS_SUCCEEDED(16-16)STATUS_START_TIME_STAMP(14-14)STATUS_COMPILATION_TIME_STAMP(17-17)src/views/checkups/network/utils/utils.ts (7)
CONFIG_PARAM_NAD_NAME(51-51)STATUS_MAX_LATENCY_NANO(41-41)CONFIG_PARAM_SAMPLE_DURATION(49-49)STATUS_SOURCE_NODE(45-45)CONFIG_PARAM_SOURCE_NODE(48-48)STATUS_TARGET_NODE(44-44)CONFIG_PARAM_TARGET_NODE(47-47)
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (4)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/checkups/utils/utils.ts (1)
getJobByName(46-60)src/utils/resources/checkups/urls.ts (1)
getStorageCheckupURL(7-11)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)
src/views/checkups/network/hooks/useCheckupsNetworkData.ts (3)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(12-12)src/views/checkups/network/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL_VALUE(37-37)src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (3)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(12-12)src/views/checkups/storage/utils/utils.ts (1)
KUBEVIRT_STORAGE_LABEL_VALUE(28-28)src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)
⏰ 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). (3)
- GitHub Check: i18n
- GitHub Check: build
- GitHub Check: unit-test
🔇 Additional comments (7)
src/multicluster/extensions.ts (1)
131-145: LGTM!The navigation item for Checkups follows the established pattern for Fleet Virtualization perspective navigation items and is properly configured.
src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts (1)
1-47: LGTM!The ACM-aware filtering implementation follows best practices with proper memoization and conditional logic. The combined filters approach and extended return type are well-structured.
src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts (1)
31-33: Verify single-cluster assumption for permissions check.The code uses
selectedClusters?.[0] || hubClusterNameto determine a single cluster for permission checks. IfselectedClusterscan contain multiple clusters (multi-cluster selection), this implementation only checks permissions on the first selected cluster.Confirm whether:
- Permission checks should only run against a single cluster
- The UI prevents multi-cluster selection when viewing permissions
- Or if multi-cluster permission checks require a different approach
If multi-cluster scenarios are supported elsewhere in this PR, consider documenting this single-cluster limitation for permissions.
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (1)
31-71: LGTM!The multicluster-aware row implementation correctly derives cluster, namespace, and name identifiers, uses centralized URL generation via
getStorageCheckupURL, and consistently appliesMulticlusterResourceLinkfor cluster and namespace columns. The job lookup pattern is clear and well-structured.src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (1)
1-72: LGTM!The migration to
useKubevirtWatchResourcewith multicluster filters is well-implemented. The combined filter approach usingcheckupsMulticlusterFiltersproperly merges label-based filtering with multicluster filter requirements, and the memoization is appropriate.src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (2)
1-1: LGTM! Memoization pattern is well-implemented.The use of
useMemoto wrap the column definitions is appropriate and will prevent unnecessary re-renders. The dependency array correctly includesisACMPage,namespace, andt, ensuring columns recompute only when the page type, namespace scope, or translation context changes.Also applies to: 36-128
44-53: Verification successful: The cluster property is correctly handled through multicluster data enrichment.The code pattern is sound. Data flows through
useKubevirtWatchResourcewithcheckupsMulticlusterFilters, which enriches ConfigMap objects with cluster metadata. The sort key'cluster'correctly references the property thatgetCluster()extracts—this exact pattern is used consistently across the codebase for VMs, snapshots, storage resources, and other multicluster resources. The multicluster filters are applied at the data fetching layer, ensuring all returned objects include the cluster property needed for both rendering (viagetCluster()in row data) and sorting.
| { | ||
| properties: { | ||
| component: { | ||
| $codeRef: 'Checkups', | ||
| }, | ||
| path: ['/k8s/all-clusters/all-namespaces/checkups'], | ||
| }, | ||
| type: 'console.page/route', | ||
| } as EncodedExtension<RoutePage>, |
There was a problem hiding this comment.
Missing exposedModules entry for Checkups component.
The route page references a component via $codeRef: 'Checkups', but there is no corresponding Checkups entry in the exposedModules object (lines 14-24). This will cause the component to fail to load when users navigate to /k8s/all-clusters/all-namespaces/checkups.
Add the Checkups component to exposedModules. For example:
export const exposedModules: ConsolePluginBuildMetadata['exposedModules'] = {
acmFlags: './multicluster/flags.ts',
+ Checkups: './views/checkups/Checkups.tsx',
ConsoleStandAlone: './utils/components/Consoles/ConsoleStandAlone.tsx',Verify the correct component path for the Checkups view.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/multicluster/extensions.ts around lines 322 to 330, the route uses
$codeRef: 'Checkups' but the exposedModules object (lines ~14-24) does not
include a Checkups entry; add an entry to exposedModules mapping 'Checkups' to
the correct component module path (verify and use the exact import path for the
Checkups view) so the runtime can resolve the codeRef and load the component
when navigating to /k8s/all-clusters/all-namespaces/checkups.
| ); | ||
|
|
||
| const [clusterRoleBinding, loadedClusterRoleBinding] = useK8sWatchResource< | ||
| const [clusterRoleBinding, loadedClusterRoleBinding] = useKubevirtWatchResource< |
There was a problem hiding this comment.
Verify loading state logic and consider consistent naming.
The variable loadedClusterRoleBinding (line 56) uses past-tense naming suggesting a "loaded" state, while other watch variables use present-progressive loading* naming (lines 36, 47, 66). At line 102, all are negated in the same way: !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding && !loadedClusterRoleBinding.
If loadedClusterRoleBinding represents "loaded=true when done" while loading* represents "loading=true when in progress", this creates a semantic inconsistency that may lead to bugs.
Please verify:
- Does
useKubevirtWatchResourcereturn a loading state (true while loading) or a loaded state (true when done) as the second element? - Rename
loadedClusterRoleBindingtoloadingClusterRoleBindingfor consistency if it follows the loading pattern.
Also applies to: 101-103
🤖 Prompt for AI Agents
In src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts
around lines 56 and 101-103, the second element from useKubevirtWatchResource is
inconsistently named loadedClusterRoleBinding while other watchers use loading*;
first confirm whether useKubevirtWatchResource returns a loading flag (true
while loading) or a loaded flag (true when finished). If it returns a loading
flag, rename loadedClusterRoleBinding to loadingClusterRoleBinding and update
all references and the combined condition at lines 101-103 to use
!loadingClusterRoleBinding consistently; if it actually returns a loaded flag,
rename the other loading* variables (or invert them) so naming semantics are
consistent and update the combined condition accordingly to preserve the
original boolean logic.
|
/retest |
upalatucci
force-pushed
the
checkups-list
branch
from
222d7ca to
36f4063
Compare
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
87-87: Inverted loading state logic.The
loadingproperty returnstruewhen all resources are finished loading (i.e.,!loadingXmeans loaded). This is semantically inverted—loading: trueshould indicate data is still being fetched.If the hook returns
loadedbooleans (true when complete), the correct logic should be:- loading: !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding, + loading: !(loadingServiceAccounts && loadingRoles && loadingRolesBinding),Alternatively, rename the destructured variables to clarify intent (e.g.,
saLoaded,rolesLoaded,roleBindingsLoaded) and use:loading: !saLoaded || !rolesLoaded || !roleBindingsLoaded,
♻️ Duplicate comments (1)
src/views/checkups/storage/list/CheckupsStorageList.tsx (1)
43-43: Empty state check may still mis-handle Set-based filters (recheck clusters/projects types)
isEmptyonly treats Arrays/Objects as empty; ifuseSelectedRowFilterClusters/useSelectedRowFilterProjectsstill returnSet<string>(as in earlier iterations),isEmpty(clusters)/isEmpty(projects)will always befalse, so the storage EmptyState will never render even when there are no configMaps and no active filters.Consider making the condition robust to Sets, e.g.:
- if (isEmpty(configMaps) && isLoaded && isEmpty(clusters) && isEmpty(projects)) { + const isEmptyOrSetEmpty = (value: unknown) => + value instanceof Set ? value.size === 0 : isEmpty(value); + + if ( + isEmpty(configMaps) && + isLoaded && + isEmptyOrSetEmpty(clusters) && + isEmptyOrSetEmpty(projects) + ) {If the hooks were changed to return arrays (or other types), please verify that this condition still behaves as intended for “no resources and no filters” vs “filtered to zero results”.
🧹 Nitpick comments (2)
src/views/checkups/network/list/CheckupsNetworkListRow.tsx (2)
22-26: Import ordering: external/utility imports should precede relative imports.These imports are placed after the relative component imports (lines 9-21). For consistency and readability, move them before the relative imports.
import React, { FC } from 'react'; import { Link } from 'react-router-dom-v5-compat'; import { modelToGroupVersionKind, NamespaceModel } from '@kubevirt-ui/kubevirt-api/console'; import { IoK8sApiBatchV1Job, IoK8sApiCoreV1ConfigMap } from '@kubevirt-ui/kubevirt-api/kubernetes'; +import { getName, getNamespace } from '@kubevirt-utils/resources/shared'; +import { getNetworkCheckupURL } from '@kubevirt-utils/resources/checkups/urls'; import { NO_DATA_DASH } from '@kubevirt-utils/resources/vm/utils/constants'; import { RowProps, TableData, Timestamp } from '@openshift-console/dynamic-plugin-sdk'; +import { getCluster } from '@multicluster/helpers/selectors'; +import MulticlusterResourceLink from '@multicluster/components/MulticlusterResourceLink/MulticlusterResourceLink'; +import { ManagedClusterModel } from '@multicluster/constants'; import CheckupsStatusIcon from '../../CheckupsStatusIcon'; ... -import { getCluster } from '@multicluster/helpers/selectors'; -import { getName, getNamespace } from '@kubevirt-utils/resources/shared'; -import { getNetworkCheckupURL } from '@kubevirt-utils/resources/checkups/urls'; -import MulticlusterResourceLink from '@multicluster/components/MulticlusterResourceLink/MulticlusterResourceLink'; -import { ManagedClusterModel } from '@multicluster/constants';
41-42: Use the extractednamevariable for consistency.
nameis extracted on line 41 but not used on line 42.const name = getName(configMap); - const jobs = getJobByName(configMap?.metadata?.name); + const jobs = getJobByName(name);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/multicluster/extensions.ts(2 hunks)src/utils/resources/checkups/urls.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkData.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts(2 hunks)src/views/checkups/network/list/CheckupsNetworkList.tsx(4 hunks)src/views/checkups/network/list/CheckupsNetworkListRow.tsx(3 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts(2 hunks)src/views/checkups/storage/list/CheckupsStorageList.tsx(4 hunks)src/views/checkups/storage/list/CheckupsStorageListRow.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/views/checkups/network/list/CheckupsNetworkList.tsx
- src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts
- src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts
- src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts
- src/utils/resources/checkups/urls.ts
- src/multicluster/extensions.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T20:28:17.043Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3118
File: src/views/checkups/self-validation/utils/constants.ts:62-68
Timestamp: 2025-11-20T20:28:17.043Z
Learning: For the kubevirt-ui/kubevirt-plugin repository: The test suite labels in TEST_SUITE_OPTIONS (Compute, Network, Storage, SSP, Tier2) in src/views/checkups/self-validation/utils/constants.ts should not be internationalized, even though they are displayed in the UI.
Applied to files:
src/views/checkups/network/list/CheckupsNetworkListRow.tsxsrc/views/checkups/storage/list/CheckupsStorageListRow.tsx
🧬 Code graph analysis (5)
src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts (2)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (5)
columnsSorting(32-43)STATUS_SUCCEEDED(18-18)STATUS_FAILURE_REASON(17-17)STATUS_START_TIME_STAMP(16-16)STATUS_COMPLETION_TIME_STAMP(19-19)
src/views/checkups/network/hooks/useCheckupsNetworkData.ts (2)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(14-14)src/views/checkups/network/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL_VALUE(38-38)
src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (3)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (4)
columnsSorting(32-43)STATUS_SUCCEEDED(18-18)STATUS_START_TIME_STAMP(16-16)STATUS_COMPLETION_TIME_STAMP(19-19)src/views/checkups/network/utils/utils.ts (7)
CONFIG_PARAM_NAD_NAME(51-51)STATUS_MAX_LATENCY_NANO(41-41)CONFIG_PARAM_SAMPLE_DURATION(49-49)STATUS_SOURCE_NODE(45-45)CONFIG_PARAM_SOURCE_NODE(48-48)STATUS_TARGET_NODE(44-44)CONFIG_PARAM_TARGET_NODE(47-47)
src/views/checkups/storage/list/CheckupsStorageList.tsx (1)
src/utils/utils/utils.ts (1)
isEmpty(32-33)
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (4)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/checkups/utils/utils.ts (1)
getJobByName(52-74)src/utils/resources/checkups/urls.ts (1)
getStorageCheckupURL(7-11)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)
⏰ 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). (3)
- GitHub Check: unit-test
- GitHub Check: i18n
- GitHub Check: build
🔇 Additional comments (10)
src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts (1)
1-44: LGTM! Clean implementation of ACM-aware filter composition.The hook correctly conditionally prepends cluster and project filters when on an ACM page, and the memoization dependencies are appropriate since
filtersis a module-level constant.src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
28-30: Verify cluster derivation behavior when no clusters are selected.The logic
selectedClusters?.[0] || hubClusterNamefalls back to the hub cluster when no clusters are selected. Ensure this is the intended behavior—users may expect permissions to be checked against all selected clusters rather than just the first one.src/views/checkups/network/hooks/useCheckupsNetworkData.ts (1)
17-75: LGTM! Well-structured multicluster data fetching.The hook correctly:
- Constructs multicluster filters with the appropriate label selector
- Uses proper memoization for filter and watch config objects
- Aggregates loading and error states appropriately
src/views/checkups/network/list/CheckupsNetworkListRow.tsx (1)
47-61: LGTM! Multicluster-aware links implemented correctly.The cluster column correctly renders
MulticlusterResourceLinkwithout aclusterprop (since it represents the cluster itself), while the namespace column correctly passes theclusterprop for cross-cluster navigation.src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (2)
1-1: LGTM! Good addition for memoization.The
useMemoimport is correctly used to optimize the column definitions and prevent unnecessary re-computations.
34-128: Good memoization pattern and ACM support. Verify the cluster sort field.The memoization of column definitions with proper dependencies (
t,isACMPage,namespace) is well implemented and will prevent unnecessary re-renders. The conditional Cluster column for ACM pages correctly aligns with the multicluster objective.However, the
sort: 'cluster'field on line 48 uses a plain string path unlike other columns which use dot-notation paths (e.g.,'metadata.name','metadata.namespace'). Verify that:
- The
clusterfield exists on the ConfigMap resource at the root level- This field is populated by the multicluster infrastructure
- The field contains sortable string values
src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts (2)
1-2: Memoized column config and ACM/namespace conditional columns look solidThe switch to
useMemoplususeIsACMPageis clean, and the conditional inclusion of Cluster + Namespace columns is consistent with multicluster and all-namespaces behavior. Dependencies[t, isACMPage, namespace]are appropriate so column labels and presence update when context changes. The integration withuseKubevirtUserSettingsTableColumnsremains intact and type-safe.Also applies to: 26-27, 28-91
36-45: Verify that'cluster'is the correct sort field for ACM rowsConfirm the row objects used by this table expose a top-level
clusterproperty, or that the table implementation correctly resolves this sort key. Check the data transformation logic in the storage checkups module to ensure sorting will function as expected.src/views/checkups/storage/list/CheckupsStorageList.tsx (1)
8-9: Multicluster filter wiring looks consistent with filtersWithSelect APIThe new selected-row filter hooks (clusters/projects) and propagation of
filtersWithSelectintoListPageFilterare wired coherently and keep pagination reset behavior intact. AssuminguseCheckupsStorageListFilters’ tuple ordering matches this destructuring andListPageFiltersupportsfiltersWithSelect, this integration looks good.Please double-check the tuple signature of
useCheckupsStorageListFiltersand theListPageFilterprops to ensurefiltersWithSelectis passed in the expected shape.Also applies to: 25-26, 38-38, 80-80
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (1)
7-7: Multicluster-aware links and columns are coherent; just ensure cluster is always well-formedThe row now derives
cluster,namespace, andnamevia shared helpers and usesgetStorageCheckupURLplusMulticlusterResourceLinkfor the cluster/namespace columns, which aligns with the multicluster URL helper and selector patterns in the repo.Two small things to verify:
- That
configMapalways has ametadata.namespaceand a stablemetadata.namesogetStorageCheckupURL(name, namespace, cluster)never seesundefined.- That in all contexts where this row is used,
getCluster(configMap)returns a defined value whenever theclustercolumn is active, so<MulticlusterResourceLink ... name={cluster} />and the namespace link both render valid targets.Also applies to: 16-20, 33-35, 42-49, 52-56
upalatucci
force-pushed
the
checkups-list
branch
from
36f4063 to
ab3550c
Compare
|
@upalatucci: This pull request references CNV-69204 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 epic to target the "4.21.0" version, but no target version was set. In 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
87-87: Loading state logic is inverted — fix requires changing AND to OR.The hook
useKubevirtWatchResourcereturns[data, loaded, error]where the second value is aloadedflag (true when the resource is done loading). However, the variables are namedloadingServiceAccounts,loadingRoles, andloadingRolesBinding— these actually holdloadedflags, not loading flags.At line 87, the logic:
loading: !loadingServiceAccounts && !loadingRoles && !loadingRolesBindingevaluates to
trueonly when all resources have finished loading (loaded=truefor all), which is backwards. It should betruewhen any resource is still loading.Fix:
- loading: !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding, + loading: !loadingServiceAccounts || !loadingRoles || !loadingRolesBinding,Alternatively, rename variables to clarify semantics:
- const [serviceAccounts, loadingServiceAccounts] = useKubevirtWatchResource(...) - const [roles, loadingRoles] = useKubevirtWatchResource(...) - const [rolesBinding, loadingRolesBinding] = useKubevirtWatchResource(...) + const [serviceAccounts, loadedServiceAccounts] = useKubevirtWatchResource(...) + const [roles, loadedRoles] = useKubevirtWatchResource(...) + const [rolesBinding, loadedRolesBinding] = useKubevirtWatchResource(...)Then update line 87:
- loading: !loadingServiceAccounts && !loadingRoles && !loadingRolesBinding, + loading: !loadedServiceAccounts || !loadedRoles || !loadedRolesBinding,
🧹 Nitpick comments (4)
src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts (1)
35-37: Consider addingfiltersto the dependency array for completeness.While
filtersis likely a static module-level import (and thus won't change), ESLint's exhaustive-deps rule would flag this. Since it's constant, there's no runtime impact, but adding it would satisfy linting and make the dependency explicit.const allFilters = useMemo(() => { return [...filtersWithSelect, ...filters]; - }, [filtersWithSelect]); + }, [filtersWithSelect, filters]);src/views/checkups/network/list/CheckupsNetworkListRow.tsx (2)
39-42: Minor consistency/refactor aroundnameusage andclusterundefined caseYou already derive
nameviagetName(configMap)but still callgetJobByName(configMap?.metadata?.name). For consistency and to avoid repeating the access logic, consider reusingname:- const name = getName(configMap); - const jobs = getJobByName(configMap?.metadata?.name); + const name = getName(configMap); + const jobs = getJobByName(name);Also, if
getCluster(configMap)can beundefinedon non‑ACM pages,clusterwill be forwarded toMulticlusterResourceLinkbelow. That likely degrades gracefully, but ifclusteris required in ACM contexts you may want to assert or guard before rendering to surface configuration issues earlier.
48-61: Verify clustered network checkup detail URL and cluster/namespace link behaviorUsing
getNetworkCheckupURL(name, namespace, cluster)in the name column looks correct conceptually, but the helper definition you shared is asymmetric:// clustered `/k8s/cluster/${cluster}/ns/${namespace}/checkups/${name}` // non‑clustered `/k8s/ns/${namespace}/checkups/network/${name}`If the actual route for network checkup details is
/checkups/network/:namein both cases, you probably want a shared suffix and only prepend the optional cluster segment, e.g.:-export const getNetworkCheckupURL = (name: string, namespace: string, cluster?: string) => { - return cluster - ? `/k8s/cluster/${cluster}/ns/${namespace}/checkups/${name}` - : `/k8s/ns/${namespace}/checkups/network/${name}`; -}; +export const getNetworkCheckupURL = (name: string, namespace: string, cluster?: string) => { + const base = cluster ? `/k8s/cluster/${cluster}/ns/${namespace}` : `/k8s/ns/${namespace}`; + return `${base}/checkups/network/${name}`; +};Please double‑check the actual route config to ensure the clustered path matches the non‑clustered one; otherwise ACM navigation from the name link may break.
For the new cluster and namespace columns:
- Cluster:
MulticlusterResourceLinkwithManagedClusterModelandname={cluster}is appropriate for hub‑side ManagedCluster objects.- Namespace: Passing
cluster={cluster}plusNamespaceModelandname={namespace}matches the multicluster pattern.Just ensure that this
clustercolumn is only surfaced on ACM pages (via column definitions /activeColumnIDs), sinceclustermay be undefined on non‑ACM pages.src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts (1)
26-38: ACM-awarefiltersWithSelect/allFilterslogic looks sound, could be simplifiedThe logic to:
- derive
filtersWithSelectas[clusterFilter, projectFilter]only on ACM pages, and- feed
useListPageFilterwithallFilters = [...filtersWithSelect, ...filters]is correct and matches the stated behavior (cluster/project filters only when
isACMPageis true).You could simplify it without nested
useMemos if you want to reduce indirection:const isACMPage = useIsACMPage(); const clusterFilter = useClusterFilter(); const projectFilter = useProjectFilter(); const filtersWithSelect = isACMPage ? [clusterFilter, projectFilter] : []; const allFilters = isACMPage ? [...filtersWithSelect, ...filters] : filters;Given the small arrays, the performance impact is negligible either way; this is purely for readability.
Also applies to: 42-42
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/multicluster/extensions.ts(2 hunks)src/utils/resources/checkups/urls.ts(1 hunks)src/views/checkups/network/hooks/useCheckupsNetworkData.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts(2 hunks)src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts(2 hunks)src/views/checkups/network/list/CheckupsNetworkList.tsx(4 hunks)src/views/checkups/network/list/CheckupsNetworkListRow.tsx(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts(2 hunks)src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts(2 hunks)src/views/checkups/storage/list/CheckupsStorageList.tsx(4 hunks)src/views/checkups/storage/list/CheckupsStorageListRow.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/utils/resources/checkups/urls.ts
- src/views/checkups/network/list/CheckupsNetworkList.tsx
- src/views/checkups/storage/components/hooks/useCheckupsStorageListColumns.ts
- src/views/checkups/storage/components/hooks/useCheckupsStoragePermissions.ts
- src/multicluster/extensions.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T20:28:17.043Z
Learnt from: galkremer1
Repo: kubevirt-ui/kubevirt-plugin PR: 3118
File: src/views/checkups/self-validation/utils/constants.ts:62-68
Timestamp: 2025-11-20T20:28:17.043Z
Learning: For the kubevirt-ui/kubevirt-plugin repository: The test suite labels in TEST_SUITE_OPTIONS (Compute, Network, Storage, SSP, Tier2) in src/views/checkups/self-validation/utils/constants.ts should not be internationalized, even though they are displayed in the UI.
Applied to files:
src/views/checkups/network/list/CheckupsNetworkListRow.tsxsrc/views/checkups/storage/components/hooks/useCheckupsStorageData.tssrc/views/checkups/storage/list/CheckupsStorageListRow.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/checkups/network/list/CheckupsNetworkListRow.tsxsrc/views/checkups/storage/list/CheckupsStorageList.tsxsrc/views/checkups/storage/list/CheckupsStorageListRow.tsx
🧬 Code graph analysis (7)
src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (3)
src/utils/hooks/constants.ts (1)
ALL_NAMESPACES_SESSION_KEY(1-1)src/views/checkups/utils/utils.ts (4)
columnsSorting(32-43)STATUS_SUCCEEDED(18-18)STATUS_START_TIME_STAMP(16-16)STATUS_COMPLETION_TIME_STAMP(19-19)src/views/checkups/network/utils/utils.ts (7)
CONFIG_PARAM_NAD_NAME(51-51)STATUS_MAX_LATENCY_NANO(41-41)CONFIG_PARAM_SAMPLE_DURATION(49-49)STATUS_SOURCE_NODE(45-45)CONFIG_PARAM_SOURCE_NODE(48-48)STATUS_TARGET_NODE(44-44)CONFIG_PARAM_TARGET_NODE(47-47)
src/views/checkups/network/hooks/useCheckupsNetworkData.ts (2)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(14-14)src/views/checkups/network/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL_VALUE(38-38)
src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
src/utils/components/ExportModal/constants.ts (1)
roleBinding(21-38)
src/views/checkups/network/list/CheckupsNetworkListRow.tsx (3)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/utils/resources/checkups/urls.ts (1)
getNetworkCheckupURL(1-5)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)
src/views/checkups/storage/list/CheckupsStorageList.tsx (1)
src/utils/utils/utils.ts (1)
isEmpty(32-33)
src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (2)
src/views/checkups/utils/utils.ts (1)
KUBEVIRT_VM_LATENCY_LABEL(14-14)src/views/checkups/storage/utils/utils.ts (1)
KUBEVIRT_STORAGE_LABEL_VALUE(30-30)
src/views/checkups/storage/list/CheckupsStorageListRow.tsx (4)
src/multicluster/helpers/selectors.ts (1)
getCluster(6-7)src/views/checkups/utils/utils.ts (1)
getJobByName(52-74)src/utils/resources/checkups/urls.ts (1)
getStorageCheckupURL(7-11)src/multicluster/constants.ts (1)
ManagedClusterModel(12-22)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: unit-test
- GitHub Check: i18n
🔇 Additional comments (14)
src/views/checkups/network/hooks/useCheckupsNetworkListColumns.ts (3)
1-7: LGTM on imports and ACM page detection setup.The
useMemoanduseIsACMPageimports are correctly added, and the hook is appropriately called before being used in the memoized columns definition.Also applies to: 34-34
126-128: Memoization dependencies are complete.All reactive values used within the callback (
t,isACMPage,namespace) are correctly included in the dependency array. This ensures the columns array is only recomputed when necessary.
44-53: Verify that ConfigMap objects in the multicluster context have aclusterproperty for the sort functionality to work correctly.The conditional Cluster column implementation follows an idiomatic pattern using the spread operator. However, confirm that the underlying data objects contain a
clusterproperty matching the sort field specified. Check the data transformation pipeline and API responses that populate the checkups network list to ensure this property is consistently available in multicluster environments.src/views/checkups/storage/components/hooks/useCheckupsStorageListFilters.ts (1)
25-44: LGTM!The ACM-aware filter logic is well-structured. The conditional inclusion of cluster/project filters based on
isACMPageand the memoization pattern are appropriate.src/views/checkups/storage/components/hooks/useCheckupsStorageData.ts (1)
17-74: LGTM!The multicluster-aware data fetching is correctly implemented. The label filtering for storage checkups (
KUBEVIRT_STORAGE_LABEL_VALUE) is appropriate, and the hook properly aggregates loading states and errors from both ConfigMaps and Jobs watches.src/views/checkups/storage/list/CheckupsStorageList.tsx (2)
77-81: LGTM!The
filtersWithSelectis correctly propagated toListPageFilter, enabling the ACM-aware cluster and project filter UI.
43-51: Verify hook return types and isEmpty() compatibility.This review comment references a past concern about
isEmpty()not handlingSet<string>correctly. The past review was marked as addressed in commit03cb345. Verify thatuseSelectedRowFilterClustersanduseSelectedRowFilterProjectsreturn types thatisEmpty()correctly handles. If they return Sets, the empty state condition on line 43 may not work as intended.src/views/checkups/network/hooks/useCheckupsNetworkPermissions.ts (1)
28-30: LGTM on cluster selection logic.The fallback from
selectedClusters?.[0]tohubClusterNameis appropriate for determining which cluster to query for permissions.src/views/checkups/storage/list/CheckupsStorageListRow.tsx (2)
33-42: LGTM!The cluster-aware URL generation using
getStorageCheckupURL(name, namespace, cluster)correctly handles both single-cluster and multi-cluster modes. The namespace extraction using utility helpers is clean.
45-50: Consider handling undefined cluster in the cluster column.When
clusterisundefined(single-cluster mode), theMulticlusterResourceLinkreceivesname={undefined}. Verify this renders gracefully (e.g., shows a dash or is hidden) rather than causing errors or showing "undefined".<TableData activeColumnIDs={activeColumnIDs} id="cluster"> - <MulticlusterResourceLink - groupVersionKind={modelToGroupVersionKind(ManagedClusterModel)} - name={cluster} - /> + {cluster ? ( + <MulticlusterResourceLink + groupVersionKind={modelToGroupVersionKind(ManagedClusterModel)} + name={cluster} + /> + ) : ( + NO_DATA_DASH + )} </TableData>src/views/checkups/network/hooks/useCheckupsNetworkData.ts (1)
17-75: LGTM!The multicluster-aware data fetching implementation is correct and consistent with the storage counterpart. The network-specific label (
KUBEVIRT_VM_LATENCY_LABEL_VALUE) is appropriately used for filtering.src/views/checkups/network/list/CheckupsNetworkListRow.tsx (1)
6-12: Imports align with multicluster usageThe new helpers (
getNetworkCheckupURL,getName/getNamespace,MulticlusterResourceLink,ManagedClusterModel,getCluster) are all used correctly and match the intended multicluster behavior. No issues here.src/views/checkups/network/hooks/useCheckupsNetworkFilters.ts (2)
1-6: ACM filter dependencies wired correctlyImporting
useClusterFilter,useProjectFilter, anduseIsACMPagehere is appropriate; they are used only to construct filter metadata and don’t introduce side effects. No issues with these new dependencies.
15-23: Return type expanded to 5‑tuple – ensure all callers updated
UseCheckupsNetworkFiltersnow returns[unfilteredData, dataFilter, onFilterChange, filters, filtersWithSelect]and the implementation matches that shape. This is a good way to expose ACM‑specific filters separately from the basefilters.Please verify that all hook consumers have been updated to destructure the new 5‑tuple in the right order (especially any tests or shared list components that might still expect 4 elements).
Also applies to: 44-44
|
@upalatucci: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -0,0 +1,11 @@ | |||
| export const getNetworkCheckupURL = (name: string, namespace: string, cluster?: string) => { | |||
| return cluster | |||
There was a problem hiding this comment.
@upalatucci You probably need to apply this logic inside getCheckUpTabs.
I suggest pulling latest changes from main and then manually cherry pick and apply all related changes, as the checkups structure went under a very big refactoring.
3 participants
📝 Description
list with multicluster filters
In the following prs i'll address the multicluster creation form and details page
🎥 Demo
Summary by CodeRabbit
New Features
Refactor / Multicluster
✏️ Tip: You can customize this high-level summary in your review settings.