MGMT-22505: Stop using deprecated Modal components#3343
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgyselov: This pull request references MGMT-22505 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 sub-task 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. |
openshift-ci
bot
added
the
approved
📝 WalkthroughWalkthroughReplaces deprecated PatternFly modal APIs with composed ModalHeader/ModalBody/ModalFooter across many UI components, removes Changes
Sequence Diagram(s)(Skipped — changes are UI-wrapper/component migrations without new multi-component sequential flows that require diagramming.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
size/XL
|
/cherry-pick releases/v2.17-cim |
|
@jgyselov: 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. |
|
@jgyselov: This pull request references MGMT-22505 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 sub-task 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: 2
🧹 Nitpick comments (4)
libs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsx (1)
110-120: Migration to modal composition pattern is correct.The modal structure properly implements the PatternFly v6 composition pattern with ModalHeader, ModalBody, and ModalFooter. The functionality and accessibility are preserved.
Optional: Remove unnecessary key prop
The
key="close"prop on line 116 is unnecessary since the button is not part of a list. In React, keys are only needed when rendering arrays of elements.- <Button key="close" variant={ButtonVariant.primary} onClick={onClose}> + <Button variant={ButtonVariant.primary} onClick={onClose}>apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (1)
65-73: Consider simplifying Content nesting.The outer
<Content>wrapper (line 65) without acomponentprop may be unnecessary since each paragraph is already wrapped in<Content component="p">. PatternFly's Content component is primarily for semantic HTML elements, so the extra wrapper doesn't add semantic value.🔎 Suggested simplification
<StackItem> - <Content> - <Content component="p"> - {t( - 'ai:This will remove all current configurations and will revert to the defaults.', - )} - </Content> - - <Content component="p">{t('ai:Are you sure you want to reset the cluster?')}</Content> - </Content> + <Content component="p"> + {t( + 'ai:This will remove all current configurations and will revert to the defaults.', + )} + </Content> + </StackItem> + <StackItem> + <Content component="p">{t('ai:Are you sure you want to reset the cluster?')}</Content> </StackItem>Note: If the outer
<Content>serves a specific styling purpose in your design system, this suggestion can be disregarded.libs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsx (1)
84-86: Consider usinggetRandomStringfor better uniqueness.The current approach with
Math.floor(Math.random() * 100)has limited entropy (only 100 possible values), which could lead to name collisions. TheHostsForm.tsxin this codebase usesgetRandomString(5)for the same purpose, providing much better uniqueness.🔎 Proposed fix
+import { getRandomString } from '../../../../common/utils'; ... initialValues={{ - nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${Math.floor( - Math.random() * 100, - )}`, + nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${getRandomString(5)}`, agentLabels: [],libs/ui-lib/lib/cim/components/Hypershift/modals/ManageHostsModal.tsx (1)
132-132: Note:isInitialValidis deprecated in Formik.While not introduced by this PR,
isInitialValid={false}is deprecated. Consider usingvalidateOnMountcombined with appropriate initial values validation in a future cleanup.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsxlibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/cim/components/Agent/BMCForm.tsxlibs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsxlibs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsxlibs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/ManageHostsModal.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/NodePoolForm.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/RemoveNodePoolModal.tsxlibs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostModal.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostYamlForm.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostYamlModal.tsxlibs/ui-lib/lib/cim/components/modals/AddHostModal.tsxlibs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsxlibs/ui-lib/lib/cim/components/modals/EditBMHModal.tsxlibs/ui-lib/lib/cim/components/modals/EditNtpSourcesModal.tsxlibs/ui-lib/lib/cim/components/modals/EditProxyModal.tsxlibs/ui-lib/lib/cim/components/modals/EditPullSecretModal.tsxlibs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsxlibs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsxlibs/ui-lib/lib/cim/components/modals/ScaleUpModal.tsx
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
📚 Learning: 2025-12-17T09:08:18.279Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/cim/components/helpers/toAssisted.ts:199-201
Timestamp: 2025-12-17T09:08:18.279Z
Learning: In libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, the platformType field on agentClusterInstall.spec is guaranteed to always contain a valid PlatformType value (when lowercased), making the type assertion safe without additional runtime validation.
Applied to files:
libs/ui-lib/lib/cim/components/Agent/BMCForm.tsxlibs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsx
📚 Learning: 2025-12-17T09:16:41.439Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/common/components/hosts/utils.ts:208-210
Timestamp: 2025-12-17T09:16:41.439Z
Learning: In libs/ui-lib/lib/common/components/hosts/utils.ts, the nodeLabels field on Host objects is guaranteed to be valid JSON because it's always constructed using JSON.stringify in libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, making direct JSON.parse safe without additional error handling.
Applied to files:
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsx
🧬 Code graph analysis (5)
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx (3)
libs/ui-lib-tests/cypress/views/reusableComponents/Alert.ts (1)
Alert(1-20)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigDisconnectedAlert.tsx (1)
CimConfigDisconnectedAlert(8-27)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx (1)
CimConfigurationFormFields(26-259)
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (2)
libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (1)
EnvironmentErrors(16-53)libs/ui-lib/lib/common/components/clusterConfiguration/DiscoveryImageConfigForm.tsx (1)
DiscoveryImageConfigForm(83-165)
libs/ui-lib/lib/cim/components/modals/AddBmcHostYamlModal.tsx (1)
libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (1)
EnvironmentErrors(16-53)
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (1)
libs/ui-lib-tests/cypress/views/reusableComponents/Alert.ts (1)
Alert(1-20)
libs/ui-lib/lib/cim/components/modals/AddBmcHostModal.tsx (1)
libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (1)
EnvironmentErrors(16-53)
⏰ 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). (6)
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (31)
libs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsx (1)
2-10: LGTM! Imports correctly updated for new modal composition pattern.The added imports (ModalBody, ModalFooter, ModalHeader) properly support the migration from deprecated Modal API to the PatternFly v6 composition pattern.
libs/ui-lib/lib/cim/components/modals/EditPullSecretModal.tsx (3)
4-15: LGTM! Imports correctly updated for modern PatternFly modal composition.The new imports (ModalBody, ModalFooter, ModalHeader) replace the deprecated modal components and are correctly sourced from @patternfly/react-core.
80-101: LGTM! Modal body and footer correctly migrated.The migration from ModalBoxBody → ModalBody and ModalBoxFooter → ModalFooter follows the modern PatternFly composition pattern while preserving all existing functionality and logic.
132-132: LGTM! ModalHeader correctly added.The ModalHeader is properly positioned as a direct child of Modal with the title prop, completing the migration to the modern PatternFly modal composition pattern.
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (4)
2-14: LGTM! Imports correctly updated for PatternFly v6 modal composition.The import additions (
Content,ModalHeader,ModalBody,ModalFooter) are all necessary for the new modal composition pattern and properly support the migration from deprecated Modal APIs.
60-61: LGTM! Modal structure correctly migrated to PatternFly v6 composition pattern.The Modal props are properly scoped to structural concerns (
isOpen,variant,onClose), while the title and icon are correctly moved toModalHeader. ThetitleIconVariant="warning"is appropriate for this destructive action confirmation.
75-81: LGTM! Error Alert correctly implemented.The conditional error Alert is properly configured with
isInline,variant="danger", and dynamic title/message. The StackItem wrapper ensures proper spacing within the modal body.
84-102: LGTM! ModalFooter and buttons correctly migrated.The action buttons are properly moved from the deprecated
actionsprop toModalFooter. Button variants, states, and event handlers are all correctly configured:
- Reset button uses
dangervariant with proper loading/disabled states- Cancel button correctly uses
linkvariant and is disabled during loading- The
voidoperator on the async handler is appropriatelibs/ui-lib/lib/cim/components/Hypershift/modals/NodePoolForm.tsx (1)
97-97: LGTM! Appropriate use of horizontal term width modifier.The addition of
horizontalTermWidthModifier={{ default: '17ch' }}improves the visual alignment of the description list by setting a consistent term width for the "OpenShift version" label.libs/ui-lib/lib/cim/components/modals/EditBMHModal.tsx (1)
2-2: LGTM! Proper migration to current PatternFly Modal API.The migration correctly:
- Updates imports from the deprecated path to
@patternfly/react-core- Adds
ModalHeaderwith the title instead of using the deprecatedtitleprop onModal- Preserves all existing functionality and accessibility attributes
Also applies to: 60-60
libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx (1)
4-14: LGTM! Clean migration to PatternFly composition pattern.The modal structure has been correctly updated to use:
ModalHeaderfor the title (line 59)ModalBodywrapper for the form content (lines 78-92)ModalFooterwrapper for action buttons (lines 93-101)All existing form validation, submission logic, and content structure are properly preserved.
Also applies to: 59-59, 78-78, 92-101
libs/ui-lib/lib/cim/components/Agent/BMCForm.tsx (2)
16-17: LGTM! Proper migration to ModalBody and ModalFooter.The component correctly wraps:
- Form content in
ModalBody(lines 278-346)- Action buttons in
ModalFooter(lines 347-355)This aligns with the current PatternFly modal composition pattern.
Also applies to: 278-278, 346-347, 355-355
304-306: LGTM! Correct prop migration from description to helperText.The change from
descriptiontohelperTextaligns with PatternFly's current API for form fields. Both InputField and CodeField now usehelperTextinstead of the deprecateddescriptionprop.Also applies to: 328-330
libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx (2)
4-15: LGTM! Proper migration to current PatternFly Modal API.The imports have been updated and
ModalHeaderis correctly used to render the modal title (line 86).Also applies to: 86-86
49-60: LGTM! Correct modal composition structure.The modal content is properly structured with:
ModalFooterwrapper in the Footer component (lines 49-60)ModalBodywrapper for the form content (lines 106-122)All form validation, submission logic, and error handling are preserved.
Also applies to: 106-106, 122-122
libs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsx (1)
2-13: LGTM - Modal migration follows PF6 composition pattern correctly.The import updates and modal structure with
ModalHeader,ModalBody, andModalFooterare correctly implemented according to PatternFly 6 patterns.Also applies to: 80-81
libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx (1)
140-179: LGTM - Clean migration to PF6 Modal composition.The modal structure correctly separates header, body, and footer. The existing logic for mass approval with progress tracking and error handling is preserved.
libs/ui-lib/lib/cim/components/modals/ScaleUpModal.tsx (1)
128-184: LGTM - Modal migration correctly preserves Formik integration.The
ModalHeaderis appropriately placed outside the Formik render prop (since the title is static), whileModalBodyandModalFooterremain inside to access form state (isSubmitting,isValid,submitForm). The nestedEditAgentModalpattern is preserved.libs/ui-lib/lib/cim/components/Hypershift/modals/RemoveNodePoolModal.tsx (1)
29-78: LGTM - Proper use of warning modal pattern.The migration correctly uses
titleIconVariant="warning"onModalHeaderfor the destructive action modal, and the danger button variant is appropriate. The async removal logic with loading spinner and error handling is preserved.libs/ui-lib/lib/cim/components/Hypershift/modals/ManageHostsModal.tsx (1)
116-170: LGTM - Modal composition correctly integrated with Formik.The migration properly separates header, body, and footer while maintaining access to Formik's form state for button disabling and submission.
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx (1)
127-220: LGTM - Complex modal correctly migrated with conditional footer.The migration handles the conditional footer logic well - showing Configure/Cancel buttons when
isConfigureis true, otherwise a single Close button. The body content is well-organized withStackandStackItemcomponents, and the Formik integration is preserved.libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsx (1)
204-204: LGTM - Text capitalization normalized.The change from "Add Nodepool" to "Add nodepool" aligns with sentence case convention and is consistent with similar updates elsewhere in the PR (e.g., NodePoolsTable.tsx).
libs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsx (1)
231-231: LGTM! Capitalization update aligned with translation changes.The button label change from "Add Nodepool" to "Add nodepool" is consistent with the updated translation key in translation.json.
libs/ui-lib/lib/cim/components/modals/AddBmcHostYamlForm.tsx (1)
11-12: LGTM! Correct migration to PatternFly v6 modal composition.The changes properly replace deprecated
ModalBoxBodyandModalBoxFooterwith the currentModalBodyandModalFootercomponents. The imports and usage are correctly updated, and the component structure is preserved.Also applies to: 130-130, 215-216, 228-228
libs/locales/lib/en/translation.json (1)
66-66: LGTM! Translation key properly aligned with UI changes.The translation key update from "ai:Add Nodepool" to "ai:Add nodepool" maintains consistency with the button label changes in the UI components.
libs/ui-lib/lib/cim/components/modals/AddBmcHostYamlModal.tsx (1)
2-2: LGTM! Consistent modal migration pattern.The modal migration properly follows the PatternFly v6 composition pattern with
ModalHeaderandModalBody. This implementation is consistent with the changes inAddBmcHostModal.tsx.Note: The
inModalprop passed toEnvironmentErrors(line 27) should be verified as mentioned in the review comment forAddBmcHostModal.tsx.Also applies to: 25-31
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (2)
3-11: LGTM: Imports updated correctly for PatternFly modal composition.The imports now include
ModalBodyandModalHeaderto support the new modal structure, following the PF6 composition pattern.
69-69: LGTM: Modal header correctly extracted.The title is now handled by the dedicated
ModalHeadercomponent, and the pluralization from "Add host" to "Add hosts" aligns with the modal's purpose.libs/ui-lib/lib/cim/components/modals/EditNtpSourcesModal.tsx (1)
2-14: LGTM: Clean migration to PatternFly modal composition.The modal has been correctly updated to use
ModalHeader,ModalBody, andModalFootercomponents, replacing the deprecated PatternFly modal structure. The Formik form logic and validation flow are preserved without changes.Also applies to: 48-48, 72-114, 115-123
libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (2)
16-52: LGTM: Component correctly simplified for inline rendering.The removal of the
inModalconditional wrapper logic aligns with the PatternFly modal composition pattern, where parent modals now provide theModalBodywrapper. This makes the component simpler and more reusable.
9-14: The type definition is correct as-is. While theEnvironmentErrorscomponent doesn't destructure theinModalprop in its implementation, all three callers (AddHostModal.tsx, AddBmcHostModal.tsx, and AddBmcHostYamlModal.tsx) still pass this prop. Removing it from the type definition would break these existing usages. This is valid React code—components are not required to use all props they receive.
jgyselov
force-pushed
the
pf6_cim_modal_component
branch
from
456c375 to
ad9898c
Compare
|
@jgyselov: This pull request references MGMT-22505 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 sub-task 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. |
|
/retest |
jgyselov
force-pushed
the
pf6_cim_modal_component
branch
from
ad9898c to
383ddfa
Compare
|
@jgyselov: This pull request references MGMT-22505 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx:
- Around line 69-110: The AddHostModal currently wraps children including
DiscoveryImageConfigForm (which itself renders ModalBody/ModalFooter) with an
outer ModalBody, creating nested ModalBody components; remove the outer
ModalBody wrapper in AddHostModal (keep EnvironmentErrors but render its
children directly) and instead conditionally wrap only the download branches
(GeneratingIsoDownload / GeneratingIPXEDownload) in a ModalBody when those
components do not render their own ModalBody, or refactor
DiscoveryImageConfigForm to not output ModalBody/ModalFooter and keep a single
ModalBody in AddHostModal so ModalBody is rendered exactly once per modal.
🧹 Nitpick comments (1)
libs/ui-lib/lib/cim/components/modals/EditNtpSourcesModal.tsx (1)
107-111: Consider addingisInlineto the error Alert for consistency.Other modals in this PR (e.g.,
EditProxyModal,MassApproveAgentModal) useisInlineon their Alert components. Consider adding it here for visual consistency.Suggested change
{error && ( <StackItem> - <Alert variant="danger" title={error} /> + <Alert variant="danger" title={error} isInline /> </StackItem> )}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsxlibs/locales/lib/en/translation.jsonlibs/ui-lib/lib/cim/components/Agent/BMCForm.tsxlibs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsxlibs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsxlibs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/ManageHostsModal.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/NodePoolForm.tsxlibs/ui-lib/lib/cim/components/Hypershift/modals/RemoveNodePoolModal.tsxlibs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostModal.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostYamlForm.tsxlibs/ui-lib/lib/cim/components/modals/AddBmcHostYamlModal.tsxlibs/ui-lib/lib/cim/components/modals/AddHostModal.tsxlibs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsxlibs/ui-lib/lib/cim/components/modals/EditBMHModal.tsxlibs/ui-lib/lib/cim/components/modals/EditNtpSourcesModal.tsxlibs/ui-lib/lib/cim/components/modals/EditProxyModal.tsxlibs/ui-lib/lib/cim/components/modals/EditPullSecretModal.tsxlibs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsxlibs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsxlibs/ui-lib/lib/cim/components/modals/ScaleUpModal.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- libs/ui-lib/lib/cim/components/modals/EditBMHModal.tsx
- libs/ui-lib/lib/cim/components/Hypershift/modals/ManageHostsModal.tsx
- libs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsx
- libs/ui-lib/lib/cim/components/modals/AddBmcHostYamlModal.tsx
- libs/ui-lib/lib/cim/components/Agent/BMCForm.tsx
- libs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsx
- libs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsx
- libs/ui-lib/lib/cim/components/modals/ScaleUpModal.tsx
- libs/ui-lib/lib/cim/components/modals/AddBmcHostYamlForm.tsx
- libs/ui-lib/lib/cim/components/Hypershift/modals/RemoveNodePoolModal.tsx
- libs/ui-lib/lib/cim/components/modals/EditPullSecretModal.tsx
- libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/cim/components/Hypershift/modals/NodePoolForm.tsx
- libs/ui-lib/lib/cim/components/modals/AddBmcHostModal.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
📚 Learning: 2025-12-17T09:16:41.439Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/common/components/hosts/utils.ts:208-210
Timestamp: 2025-12-17T09:16:41.439Z
Learning: In libs/ui-lib/lib/common/components/hosts/utils.ts, the nodeLabels field on Host objects is guaranteed to be valid JSON because it's always constructed using JSON.stringify in libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, making direct JSON.parse safe without additional error handling.
Applied to files:
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsx
🧬 Code graph analysis (1)
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (2)
libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (1)
EnvironmentErrors(15-52)libs/ui-lib/lib/common/components/clusterConfiguration/DiscoveryImageConfigForm.tsx (1)
DiscoveryImageConfigForm(83-165)
🔇 Additional comments (11)
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/HostsStep/HostsForm.tsx (1)
204-204: LGTM!The capitalization standardization from "Add Nodepool" to "Add nodepool" is consistent with the parallel changes in
NodePoolsTable.tsxand the translation file. This improves UI text consistency across the application.libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx (4)
11-15: LGTM!The imports correctly use the non-deprecated PatternFly modal components (
Modal,ModalHeader,ModalBody,ModalFooter). This aligns with the PR objective of stopping usage of deprecated Modal components.
134-141: LGTM!The modal structure correctly follows the PatternFly 6 composed pattern with
ModalHeadercontaining the title, whilearia-labelremains on theModalfor accessibility. The migration from deprecated props to composed children is properly implemented.
191-217: LGTM!The footer correctly implements conditional rendering based on
isConfigurestate, showing Configure/Cancel buttons for the configuration flow and a Close button otherwise. Thevoid handleSubmit()pattern on line 204 properly handles the async form submission.
142-189: LGTM!The modal body content is well-structured using
StackandStackItemfor consistent spacing. The error alert (lines 152-162) is properly guarded byisError, and the storage size warning (lines 180-188) is appropriately shown only during configuration mode. All child component props are correctly preserved.apps/assisted-disconnected-ui/src/components/ResetSingleClusterModal.tsx (1)
59-103: LGTM! Clean migration to composed Modal structure.The modal correctly uses the modern PatternFly composition pattern with
ModalHeader,ModalBody, andModalFooter. The warning icon variant is appropriate for this destructive reset action, and button states are properly managed.libs/ui-lib/lib/cim/components/modals/EditNtpSourcesModal.tsx (1)
40-128: LGTM! Modal structure correctly migrated.The migration properly places
ModalHeaderbefore the Formik component and wraps the form content inModalBodywith actions inModalFooter.libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx (2)
45-62: Nice extraction of the Footer component.The
Footercomponent cleanly encapsulates the form submission logic usinguseFormikContext, making the code more modular while maintaining proper access to Formik's state.
79-127: LGTM! Modal structure correctly migrated.The modal properly uses the composed structure with
ModalHeaderoutside Formik and theFootercomponent correctly positioned to access Formik context.libs/ui-lib/lib/cim/components/InfraEnv/EnvironmentErrors.tsx (1)
23-51: Verify: Errors now block children rendering.When
infraEnv.statusis falsy or there are failing alerts, the component returns only the error UI without renderingchildren. This appears intentional as a blocking error state, but confirm this matches the expected UX—users won't see the form/content when there are infrastructure environment errors.libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx (1)
140-180: LGTM! Modal structure correctly migrated.The migration properly separates header, body, and footer. The progress-based button disabling logic is preserved correctly.
jgyselov
force-pushed
the
pf6_cim_modal_component
branch
2 times, most recently
from
ea0b867 to
345bfbb
Compare
openshift-merge-robot
added
the
needs-rebase
jgyselov
force-pushed
the
pf6_cim_modal_component
branch
from
345bfbb to
8b6ec92
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@jgyselov: This pull request references MGMT-22505 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsx`:
- Line 231: Update the inconsistent translation key casing so both buttons
match: change the usage of t('ai:Remove Nodepool') to t('ai:Remove nodepool')
(matching the existing t('ai:Add nodepool') casing). Locate the occurrences of
the translation keys in NodePoolsTable.tsx (references to t('ai:Add nodepool')
and t('ai:Remove Nodepool')), update the latter to use the lowercase "nodepool"
key, and run a quick build or i18n check to ensure the translation lookup still
resolves.
🧹 Nitpick comments (2)
libs/ui-lib/lib/cim/components/modals/EditPullSecretModal.tsx (1)
80-101: Keep the submit error inside the Stack for consistent gutter spacing.
Line 84-Line 88 renders aStackItemoutside aStack, so it won’t receive the stack’s spacing rules. Consider nesting it inside the existingStackto keep layout consistent.♻️ Proposed tweak
<ModalBody> <Stack hasGutter> <StackItem>{body}</StackItem> - </Stack> - {submitError && ( - <StackItem> - <Alert title={submitError} variant="danger" isInline /> - </StackItem> - )} + {submitError && ( + <StackItem> + <Alert title={submitError} variant="danger" isInline /> + </StackItem> + )} + </Stack> </ModalBody>libs/ui-lib/lib/cim/components/Hypershift/modals/AddNodePoolModal.tsx (1)
84-86: Consider improving nodepool name uniqueness.Using
Math.floor(Math.random() * 100)for the nodepool name suffix provides only 100 possible values, which could lead to naming collisions in environments with many nodepools. Consider using a more robust approach such as a UUID fragment or timestamp.♻️ Optional: Use more unique suffix
initialValues={{ - nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${Math.floor( - Math.random() * 100, - )}`, + nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${Date.now().toString(36).slice(-4)}`, agentLabels: [],Or use a larger random range:
- nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${Math.floor( - Math.random() * 100, - )}`, + nodePoolName: `nodepool-${hostedCluster.metadata?.name || ''}-${Math.random().toString(36).slice(2, 7)}`,
libs/ui-lib/lib/cim/components/Hypershift/DetailsPage/NodePoolsTable.tsx
Show resolved
Hide resolved
jgyselov
force-pushed
the
pf6_cim_modal_component
branch
from
8b6ec92 to
d3b9b0e
Compare
|
@jgyselov: This pull request references MGMT-22505 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 sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (1)
69-108: Alerts rendered withoutModalBodywrapper when errors occur.When
EnvironmentErrorsdetects errors (e.g.,!infraEnv.status), it renders alerts directly and does not renderchildren. Since there's noModalBodywrapper aroundEnvironmentErrorshere, the error alerts will render directly inside theModalwithout proper body styling.This differs from
AddBmcHostModal.tsxandAddBmcHostYamlModal.tsxwhereEnvironmentErrorsis wrapped inModalBody.Suggested fix - Wrap EnvironmentErrors in ModalBody for error states
One option is to have
EnvironmentErrorswrap its error output inModalBodywhen used in this context, or add conditional wrapping here. However, sinceDiscoveryImageConfigFormalready provides its ownModalBody, you may need to refactorEnvironmentErrorsto accept a wrapper prop or handle this at the call site.A simpler approach if acceptable:
<ModalHeader title={t('ai:Add hosts')} /> + <ModalBody> - <EnvironmentErrors infraEnv={infraEnv} docVersion={docVersion}> + {(!infraEnv.status || getFailingResourceConditions(infraEnv, ['ImageCreated']).length > 0) ? ( + <EnvironmentErrors infraEnv={infraEnv} docVersion={docVersion} /> + ) : ( {dialogType === 'config' && ( - <DiscoveryImageConfigForm + // Note: DiscoveryImageConfigForm has internal ModalBody - may need refactoringConsider verifying that the current error state layout is acceptable or aligning the structure with other modals.
🧹 Nitpick comments (2)
libs/ui-lib/lib/cim/components/Agent/MinimalHWRequirements.tsx (1)
110-119: Confirm close icon behavior in composed ModalHeader.
Please verify that PatternFly 6’s composed modal still renders the standard header close control withonCloseonModal, or add the appropriate prop toModalHeaderif required. This keeps expected UX/accessibility even with the footer Close button.libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx (1)
192-212: LGTM!Footer correctly implements the conditional action pattern:
- Configure/Cancel buttons when
isConfigureis true- Close button when in view-only mode
- Form submission properly wired through
handleSubmitMinor: The double negation on line 198 is unnecessary since the expression already evaluates to a boolean:
♻️ Optional simplification
- isDisabled={!!(isSubmitting || !isValid)} + isDisabled={isSubmitting || !isValid}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, LiorSoffer 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 |
openshift-merge-bot
bot
merged commit 81ef26a
into
openshift-assisted:master
|
@jgyselov: #3343 failed to apply on top of branch "releases/v2.17-cim": 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. |
5 participants
https://issues.redhat.com/browse/MGMT-22505
Summary by CodeRabbit
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.