-
Notifications
You must be signed in to change notification settings - Fork 666
OCPBUGS-69387: update operator details when changing channels #15853
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
base: main
Are you sure you want to change the base?
Conversation
|
@rhamilto: This pull request references Jira Issue OCPBUGS-69387, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds seven new operator-hub React components, a new useCurrentChannel hook, updates useCurrentCSVDescription to accept selectedChannel and use useSearchParams, and moves infrastructure-feature computation into OperatorDescription and OperatorHubItemDetails internals. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
TheRealJon
left a comment
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.
@rhamilto Other than de-duping some of the channel selection logic that we talked about, looks good! Let me know when you update and I can give it the final lgtm tag.
ac20932 to
08657c3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx(2 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx
🧬 Code graph analysis (8)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (2)
frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)frontend/public/components/utils/tile-view-page.jsx (1)
searchParams(466-466)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx (3)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
CapabilityLevel(58-86)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx (7)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx (1)
OperatorCapability(6-11)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (1)
OperatorInfrastructureFeatures(8-30)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (1)
OperatorValidSubscriptions(8-24)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (1)
OperatorRepository(6-11)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (1)
OperatorContainerImage(5-12)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (1)
OperatorCreatedAt(6-15)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx (1)
OperatorSupport(8-22)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (4)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/index.ts (1)
CSVAnnotations(139-166)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (1)
getValidSubscription(205-208)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (1)
getInfrastructureFeatures(216-275)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
🔇 Additional comments (12)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx (1)
1-20: LGTM! Well-structured refactor with proper fallback logic.The hook correctly prioritizes channel selection (explicit parameter → URL query → default → first available) and handles edge cases with appropriate optional chaining.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (1)
1-15: LGTM! Clean component with proper null handling.The component correctly uses optional chaining to handle undefined values and follows the established pattern for operator metadata components.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx (1)
1-26: LGTM! Proper prioritization and null handling.The component correctly prioritizes the support workflow URL over plain support text and handles all undefined cases appropriately.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (1)
1-28: LGTM! Proper error handling and null safety.The component correctly handles parsing errors with informative warnings and safely renders subscriptions or a fallback dash.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (1)
1-16: LGTM! Simple and effective.The component correctly extracts and displays the container image with appropriate fallback handling.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (1)
1-37: LGTM! Consistent error handling and proper cluster flag integration.The component follows the established pattern and correctly passes cluster capabilities for infrastructure feature computation.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx (1)
1-15: LGTM! Clean delegation to CapabilityLevel component.The component correctly extracts capabilities and delegates rendering to the existing CapabilityLevel component with proper fallback handling.
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
1-22: Clean implementation with proper edge case handling.The hook correctly implements the channel selection logic with a well-defined fallback chain (URL param → defaultChannel → first channel). Optional chaining is used consistently to handle cases where
packageManifestor its nested properties might be unavailable.frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx (2)
361-396: Good modular refactoring for dynamic channel-based updates.The new components correctly receive
packageManifestand leverageuseCurrentChannelinternally to read the selected channel from URL query parameters. This ensures the displayed metadata updates dynamically when users switch channels.The cluster flags (
clusterIsAWSSTS,clusterIsAzureWIF,clusterIsGCPWIF) are appropriately passed toOperatorInfrastructureFeatureswhere they're needed for computing infrastructure features.
167-181: Verify filter attribute consistency with displayed values.
getCurrentCSVDescription(pkg)at line 167 returns the default channel's CSV description regardless of the selected channel. Theattributesobject (used for filtering) will containinfrastructureFeaturesandvalidSubscriptionbased on the default channel, while the rendered details now show the URL-selected channel's values.If users filter by infrastructure features or valid subscriptions, the filter behavior may not match what's displayed when a non-default channel is selected. This may be acceptable if filters are intended to use default channel data, but worth verifying against expected UX.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (2)
37-42: LGTM!The imported utilities support the new infrastructure features computation logic.
276-281: LGTM!The token authentication feature detection correctly uses the locally computed
infraFeaturesand properly memoizes the result.
...tend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx
Outdated
Show resolved
Hide resolved
...ackages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx
Show resolved
Hide resolved
08657c3 to
bc0b4ef
Compare
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)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
368-371: Extractrepository,support, andvalidSubscriptionfrom the selected channel's CSV annotations instead of the static item prop.These fields should update when switching channels, consistent with how
containerImage,createdAt, andcapabilityare handled. The dedicated components (OperatorRepository,OperatorSupport,OperatorValidSubscriptions) already follow this pattern by extracting these values from the current channel's CSV annotations. Extract them fromcurrentCSVDescription?.annotationssimilar to the other annotation-based fields to ensure all operator details reflect the selected channel.
🧹 Nitpick comments (1)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
260-274: Consider extracting duplicated infraFeatures computation into a custom hook.The
infraFeaturesuseMemo logic is nearly identical in bothOperatorDescriptionandOperatorHubItemDetails. This could be consolidated into a reusable hook:// e.g., useInfrastructureFeatures.ts export const useInfrastructureFeatures = ( currentCSVDescription: CSVDescription, clusterIsAWSSTS: boolean, clusterIsAzureWIF: boolean, clusterIsGCPWIF: boolean, packageName?: string, ) => { return React.useMemo(() => { const annotations = currentCSVDescription?.annotations ?? {}; return getInfrastructureFeatures(annotations, { clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, onError: (error) => { console.warn( `Error parsing infrastructure features from PackageManifest "${packageName}":`, error, ); }, }); }, [currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, packageName]); };Also applies to: 393-407
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx(3 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx
- frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx
🧬 Code graph analysis (6)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (2)
frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)frontend/public/components/utils/tile-view-page.jsx (1)
searchParams(466-466)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (4)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/index.ts (1)
CSVAnnotations(139-166)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (1)
getInfrastructureFeatures(216-275)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (3)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/console-shared/src/components/links/ExternalLink.tsx (1)
ExternalLink(29-61)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (4)
getInfrastructureFeatures(216-275)isAWSSTSCluster(43-53)isAzureWIFCluster(55-65)isGCPWIFCluster(67-77)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
🔇 Additional comments (11)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (1)
6-11: LGTM!The component correctly uses
useCurrentChannelto derive the current CSV description and safely extracts the repository annotation. The conditional rendering appropriately handles both present and absent repository values.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (1)
6-15: LGTM!The component correctly validates dates using
!Number.isNaN(Date.parse(createdAt)), which properly handles epoch time (Unix time 0) that would be falsy but valid. The fallback logic appropriately renders either the rawcreatedAtvalue or a dash when the date is invalid or missing.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (1)
8-30: LGTM!The component correctly derives infrastructure features from the current channel's CSV annotations using
getInfrastructureFeatures. The error handling appropriately logs warnings with the package name for debugging, and the rendering logic properly handles both present and absent feature lists.frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx (1)
4-15: LGTM!The hook successfully migrates from the deprecated
getQueryArgumenttouseSearchParamsfrom react-router-dom-v5-compat. The channel resolution logic correctly prioritizes the providedselectedChannelparameter, then the URL query parameter, and finally the package manifest's default channel, maintaining backward compatibility while adding new functionality.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (1)
5-12: LGTM!The component correctly uses
useCurrentChannelto derive the current CSV description and safely extracts the container image annotation. The rendering logic appropriately handles both present and absent values.frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
8-22: LGTM!The hook correctly implements channel resolution with a sensible fallback chain: URL query parameter → default channel → first available channel. The use of
useSearchParamsfrom react-router-dom-v5-compat is appropriate, and the implementation includes clear documentation describing its behavior.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (1)
8-24: LGTM!The component correctly derives valid subscriptions from the current channel's CSV annotations using
getValidSubscription. The error handling appropriately logs warnings with the package name, and the rendering logic properly handles both present and absent subscription lists.frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx (3)
11-34: LGTM!The imports correctly include all the new modular operator-hub components (
OperatorCapability,OperatorContainerImage,OperatorCreatedAt,OperatorInfrastructureFeatures,OperatorRepository,OperatorSupport,OperatorValidSubscriptions) that are used later in the file to replace inline rendering logic.
362-396: LGTM!The details properties have been successfully refactored to use modular operator-hub components. Each component receives
packageManifest={pkg}and will dynamically update when the channel changes, achieving the PR's objective. TheOperatorInfrastructureFeaturescomponent correctly receives the additional cluster capability flags (clusterIsAWSSTS,clusterIsAzureWIF,clusterIsGCPWIF) required for feature determination.
401-413: LGTM!The
OperatorDescriptioncomponent call correctly removes theinfraFeaturesprop, aligning with the PR's objective to internalize infrastructure features computation. The component now receivespackageManifestand cluster capability flags, allowing it to derive infrastructure features internally based on the current channel's CSV annotations.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
491-503: LGTM on the OperatorDescription integration.The component now correctly receives cluster capability flags (
clusterIsAWSSTS,clusterIsAzureWIF,clusterIsGCPWIF) and computes infrastructure features internally. The prop interface is cleaner with theinfraFeaturesprop removed.
|
/retest |
|
/cherrypick release-4.21 |
|
@rhamilto: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
Done. And I think Claude, CodeRabbit, and I are all in agreement now. ;-) |
|
/label acknowledge-critical-fixes-only |
|
/jira refresh |
|
@rhamilto: This pull request references Jira Issue OCPBUGS-69387, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
TheRealJon
left a comment
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.
/lgtm
|
@XiyunZhao, can you please verify? |
bc0b4ef to
0effcdc
Compare
TheRealJon
left a comment
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.
/lgtm
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
🧹 Nitpick comments (2)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (2)
258-274: Consider removingpackageManifestfrom the dependency array.The
packageManifestis included in theuseMemodependencies but is only used in the error handler (line 269). This could cause unnecessary recomputations whenpackageManifestchanges. Consider usingpackageManifest?.metadata?.namein the dependency array or accepting that it's only used for logging.However, this is a minor optimization as
packageManifestchanges are likely infrequent.🔎 Optional optimization
- }, [currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, packageManifest]); + }, [currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF]);
258-274: Consider extracting the duplicatedinfraFeaturescomputation into a shared hook.The
infraFeaturescomputation logic (lines 258-274 and 392-407) is duplicated betweenOperatorDescriptionandOperatorHubItemDetails. This violates the DRY principle and increases maintenance burden when the logic needs to change.Consider creating a custom hook like
useInfrastructureFeaturesthat accepts the current CSV description and cluster flags, returning the computed infrastructure features.🔎 Example refactor
Create a new hook in a shared location:
export const useInfrastructureFeatures = ( currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, packageManifestName, ) => { return React.useMemo(() => { const currentCSVAnnotations = currentCSVDescription?.annotations ?? {}; return getInfrastructureFeatures(currentCSVAnnotations, { clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, onError: (error) => { console.warn( `Error parsing infrastructure features from PackageManifest "${packageManifestName}":`, error, ); }, }); }, [currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF]); };Then use it in both components:
const infraFeatures = useInfrastructureFeatures( currentCSVDescription, clusterIsAWSSTS, clusterIsAzureWIF, clusterIsGCPWIF, packageManifest?.metadata?.name, );Also applies to: 392-407
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (11)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx(3 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-capability.tsx
- frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentCSVDescription.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsxfrontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx
🧬 Code graph analysis (7)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (4)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/index.ts (1)
CSVAnnotations(141-168)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (1)
getValidSubscription(205-208)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (4)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/index.ts (1)
CSVAnnotations(141-168)frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (1)
getInfrastructureFeatures(216-275)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (3)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/console-shared/src/components/links/ExternalLink.tsx (1)
ExternalLink(29-61)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (2)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
useCurrentChannel(8-22)frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (2)
frontend/packages/operator-lifecycle-manager/src/types.ts (1)
PackageManifestKind(256-283)frontend/public/components/utils/tile-view-page.jsx (1)
searchParams(466-466)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (1)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-utils.ts (4)
getInfrastructureFeatures(216-275)isAWSSTSCluster(43-53)isAzureWIFCluster(55-65)isGCPWIFCluster(67-77)
🔇 Additional comments (12)
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-repository.tsx (1)
1-15: LGTM!Clean, focused component that follows the established pattern for modular operator-hub components. The implementation correctly uses
useCurrentChannelto derive channel-specific data.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-infrastructure-features.tsx (1)
1-37: LGTM!The component correctly computes infrastructure features from channel annotations with appropriate error handling. The error callback includes the package name which aids debugging.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-created-at.tsx (1)
1-19: LGTM!The date validation correctly handles edge cases including Unix epoch. The graceful fallback to display the raw string or dash provides good UX for invalid dates.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-support.tsx (1)
1-26: LGTM!The component correctly extracts support workflow annotations and renders the appropriate link with proper internationalization.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-container-image.tsx (1)
1-16: LGTM!Simple, focused component following the established pattern.
frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-valid-subscriptions.tsx (1)
1-28: LGTM!Consistent implementation with proper error handling that follows the established pattern of the other modular components.
frontend/packages/operator-lifecycle-manager/src/hooks/useCurrentChannel.tsx (1)
1-22: LGTM!Well-designed centralized hook with a clear fallback chain (URL param → default channel → first channel). The JSDoc comment clearly documents the behavior. Good use of optional chaining for null safety.
frontend/packages/operator-lifecycle-manager/src/hooks/useOperatorCatalogItems.tsx (1)
352-412: No action needed. The channel selection properly syncs to URL parameters.The
OperatorChannelSelectcomponent includes auseEffecthook that callssetQueryArgument('channel', selectedChannel)whenever the selected channel changes (line 57-65 of operator-channel-version-select.tsx). This ensures the channel query parameter is always updated in the URL when users change the selection. The modular components usinguseCurrentChannelcorrectly read from these updated URL parameters viasearchParams.get('channel'), so they will reflect channel changes as expected.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-item-details.tsx (4)
37-42: LGTM!The new imports support the infrastructure feature computation logic and align with the centralized utility functions in
operator-hub-utils.ts.
387-390: LGTM!The cluster type detection logic correctly uses the imported utility functions to determine the cluster configuration for infrastructure feature computation.
409-409: LGTM! Previous issue resolved.The infrastructure features are now correctly derived from the dynamically computed
infraFeatures(based on the current channel's CSV annotations) rather than the staticitem.infraFeatures. This ensures the UI updates properly when users switch channels.Based on past review comments, this addresses the concern about static infrastructure feature display.
Also applies to: 458-460
383-383: LGTM!The date validation using
Date.parsebefore rendering theTimestampcomponent is a good practice that prevents errors from invalid date strings, aligning with the PR's improved date validation objective.
|
/retest |
0effcdc to
120b7fb
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, sg00dwin, TheRealJon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test okd-scos-images |
2 similar comments
|
/test okd-scos-images |
|
/test okd-scos-images |
|
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary
This PR refactors the operator details display to dynamically update when changing channels in the OperatorHub. Previously, operator details would remain static even when switching channels. Now, most operator metadata fields (see noted exclusions below) respond to channel selection changes.
Changes
New Hook Created
New Components Created
The following components were extracted to dynamically read from the selected channel (all use
useCurrentChannelhook):Number.isNaN(Date.parse(...))Hook Updates
useSearchParamsinstead of deprecatedgetQueryArgument, and now accepts optionalselectedChannelparameterRefactoring in useOperatorCatalogItems
PlainList,Timestamp,ExternalLink,CapabilityLevel,getSupportWorkflowUrl)Infrastructure Features Calculation
useMemobased on current channel's CSV annotations in both:OperatorDescriptioncomponent - for description sectionOperatorHubItemDetailscomponent - for details displayinfraFeaturesprop fromOperatorDescriptioncomponentTesting
Note
source,provider,displayName, andkeywordsoperator metadata fields were intentionally excluded from this fix since they are less likely to change when the channel changes.🤖 Generated with Claude Code