-
Notifications
You must be signed in to change notification settings - Fork 53
CNV-74670: installed features in High availability are checked #3320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CNV-74670: installed features in High availability are checked #3320
Conversation
|
@adamviktora: This pull request references CNV-74670 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. |
📝 WalkthroughWalkthroughA single checkbox state logic modification in the HighAvailabilityFeatureItem component. The Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
@adamviktora: This pull request references CNV-74670 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/clusteroverview/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx (1)
33-34: Add defensive handling for potentially undefined operatorDetailsMap entry.The optional chaining on line 33 may result in
undefinedvalues for bothinstallStateandoperatorHubURLifoperatorDetailsMapis undefined or the operator key doesn't exist. WhileisInstalledreturnsfalsewhen passedundefined(preventing runtime errors), this is a type safety issue—the function signature expectsInstallState, notInstallState | undefined.Consider adding explicit defaults:
Suggested approach
- const { installState, operatorHubURL } = operatorDetailsMap?.[operatorName]; + const { installState = InstallState.UNKNOWN, operatorHubURL = '' } = operatorDetailsMap?.[operatorName] ?? {}; const installed = isInstalled(installState);This ensures
isInstalledreceives a validInstallStateenum value rather thanundefined, making the type handling explicit and correct.
🧹 Nitpick comments (1)
src/views/clusteroverview/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx (1)
23-75: Consider wrapping the component in React.memo for performance.While the component is relatively simple, the coding guidelines recommend using React memoization tools to avoid unnecessary re-renders. Wrapping this component in
React.memocould prevent re-renders when parent components update but props remain the same.🔎 Example implementation
-const HighAvailabilityFeatureItem: FC<HighAvailabilityFeatureItemProps> = ({ +const HighAvailabilityFeatureItem: FC<HighAvailabilityFeatureItemProps> = React.memo(({ alternativeChecked, checkboxLabel, description, operatorName, setAlternativeChecked, -}) => { +}) => { const { operatorDetailsMap, operatorsToInstall, updateInstallRequests } = useVirtualizationFeaturesContext(); // ... rest of component return ( // ... JSX ); -}; +}); + +HighAvailabilityFeatureItem.displayName = 'HighAvailabilityFeatureItem'; export default HighAvailabilityFeatureItem;As per coding guidelines, memoization helps optimize render performance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/views/clusteroverview/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx
**/*.{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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx
🧠 Learnings (1)
📚 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/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.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 (1)
src/views/clusteroverview/SettingsTab/ClusterTab/components/VirtualizationFeaturesSection/VirtualizationFeaturesWizard/components/ConfigurationStep/components/HighAvailabilityConfigurationSection/components/HighAvailabilityContent/HighAvailabilityFeatureItem/HighAvailabilityFeatureItem.tsx (1)
49-49: LGTM! Correctly implements the PR objective.The change makes the checkbox checked when the operator is installed, which aligns with the PR goal of showing installed features as checked. Combined with the
isDisabledprop on line 50, installed operators are displayed as checked and disabled, correctly preventing uninstallation.
|
[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 |

📝 Description
Installed features in Settings -> Configure features modal -> High availability are now checked.
(We can't make the checkboxes enabled, because the modal can't uninstall the features.)
🎥 Demo
After:

Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.