CNV-75833: useClusterObservabilityDisabled should only be used in ACM #3321
Conversation
|
@galkremer1: This pull request references CNV-75833 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. |
📝 WalkthroughWalkthroughThe changes introduce observability loading state requirements across alert handling, data computation, and component rendering. ACM pages now require observability to be loaded alongside existing resource loading states, while non-ACM pages maintain existing behavior. This affects hooks for alerts, cluster observability status, migration card data, and the VM statuses card component. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
|
@galkremer1: This pull request references CNV-75833 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. |
|
@galkremer1: This pull request references CNV-75833 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: 0
🧹 Nitpick comments (3)
src/views/clusteroverview/MigrationsTab/hooks/useMigrationCardData.ts (1)
64-69: Verify searchQueries behavior with empty enabledClusters.For non-ACM pages,
useClusterObservabilityDisabledreturnsenabledClusters: []. Ifcluster === ALL_CLUSTERS_KEYin a non-ACM context, this would create a search filter with an empty values array:[{ property: 'cluster', values: [] }]. This might cause unexpected filtering behavior.Consider adding a length check to ensure the search query is only created when there are actually enabled clusters to filter by.
🔎 Suggested enhancement
const searchQueries = useMemo<AdvancedSearchFilter | undefined>(() => { - if (cluster === ALL_CLUSTERS_KEY && observabilityLoaded) { + if (cluster === ALL_CLUSTERS_KEY && observabilityLoaded && enabledClusters.length > 0) { return [{ property: 'cluster', values: enabledClusters }]; } return undefined; }, [cluster, enabledClusters, observabilityLoaded]);src/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx (1)
46-51: Verify searchQueries behavior with empty enabledClusters.Similar to the migration card data hook, this could create a search filter with an empty values array for non-ACM pages where
cluster === ALL_CLUSTERS_KEY. Consider adding a length check as demonstrated inuseAlerts.ts(line 42).🔎 Suggested enhancement
const searchQueries = useMemo<AdvancedSearchFilter | undefined>(() => { - if (cluster === ALL_CLUSTERS_KEY && observabilityLoaded) { + if (cluster === ALL_CLUSTERS_KEY && observabilityLoaded && enabledClusters.length > 0) { return [{ property: 'cluster', values: enabledClusters }]; } return undefined; }, [cluster, enabledClusters, observabilityLoaded]);src/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.ts (1)
93-101: Early return unblocks non-ACM pages effectively.The early return for non-ACM pages with
loaded: trueand empty cluster arrays achieves the PR objective of making the hook safe to call in non-ACM environments. This prevents consumers from waiting indefinitely for cluster observability data that isn't relevant outside ACM.Consider enhancing the comment to clarify that the hooks above still execute but their results are discarded.
🔎 Optional comment enhancement
- // For non-ACM pages, return loaded=true immediately to avoid blocking consumers + // For non-ACM pages, return loaded=true immediately to avoid blocking consumers. + // Note: The hooks above still execute, but their results are safely discarded. if (!isACMPage) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.tssrc/views/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.tssrc/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx
**/*.{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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.tssrc/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx
**/*.{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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.tssrc/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx
**/*.{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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.tssrc/utils/hooks/useAlerts/useAlerts.tssrc/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.tssrc/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx
**/*.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/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.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/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.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/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.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/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx
⏰ 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/clusteroverview/MigrationsTab/hooks/useMigrationCardData.ts (1)
122-122: Loading state correctly incorporates observability readiness.The loaded condition now requires
observabilityLoadedin addition to the existing resource loading states. This ensures the component waits for observability data before rendering. SinceuseClusterObservabilityDisabledreturnsloaded: trueimmediately for non-ACM pages, this doesn't block non-ACM environments.src/utils/hooks/useAlerts/useAlerts.ts (2)
41-46: Good defensive check for enabled clusters.The
selectedClusterscomputation correctly verifiesenabledClusters.length > 0before using the array. This prevents issues when the observability hook returns an empty array for non-ACM pages or when no clusters have observability enabled.
106-106: Loading state correctly differentiated by page context.The loading condition properly gates the
observabilityLoadedrequirement onisACMPage, ensuring ACM pages wait for both metric and observability data while non-ACM pages maintain their existing behavior. This aligns well with the PR objective.src/views/clusteroverview/OverviewTab/vm-statuses-card/VMStatusesCard.tsx (2)
71-71: Loading state correctly incorporates observability readiness.The
isLoadedcomputation ensures both VM data and observability data are ready before rendering. SinceuseClusterObservabilityDisabledreturnsloaded: trueimmediately for non-ACM pages, this doesn't block non-ACM environments.
83-88: Conditional rendering properly uses combined loading state.The loading and content rendering logic correctly uses
isLoadedto determine when all required data is available. The separation between loading, error, and loaded states follows best practices.src/utils/hooks/useAlerts/utils/useClusterObservabilityDisabled.ts (3)
7-7: ACM page detection properly integrated.The addition of
useIsACMPageenables the hook to differentiate between ACM and non-ACM contexts, which is the core fix for this PR. This allows the hook to gate its cluster management logic appropriately.Also applies to: 37-37
50-53: Cluster computation correctly gated on ACM context.The
useMemologic properly short-circuits cluster processing for non-ACM pages, and the dependency array correctly includesisACMPageto ensure the computation updates if the page context changes.Also applies to: 84-84
40-48: The code already properly handles non-ACM environments with the implementation shown at lines 94–100. The error state is explicitly cleared by returningerror: undefinedin the early return for non-ACM pages, and data processing is guarded by the!isACMPagecheck in theuseMemoat line 51. The hooks are correctly called unconditionally (as required by React), and the underlying watch hooks gracefully handle null/empty resource options by early-returning without triggering API errors. No changes are needed.Likely an incorrect or invalid review comment.
|
[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 |
3 participants
📝 Description
useClusterObservabilityDisabledshould only used in ACM - if used in non ACM envs it returns an error.Jira ticket: CNV-75833
🎥 Demo
Before:
Before.mov
After:
After.mov
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.