MGMT-22512: Add additional new fields to the above the sea UI#3337
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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. |
📝 WalkthroughWalkthroughAdds disconnected-image support and static-IP flows: new InfraEnv fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as OptionalConfigurationsStep (Formik)
participant Service as ClustersService / InfraEnvsAPI
participant API as Assisted-Installer API
participant Wizard as ClusterWizardContextProvider
Note over User,UI: User fills optional configs (proxy, NTP, hosts network, rendezvous IP)
User ->> UI: submit(values)
UI ->> UI: buildInfraEnvParams(values)
UI ->> Service: createInfraEnv(params) / updateInfraEnv(params)
Service ->> API: POST/PUT /infra-env (includes rendezvousIp, hostsNetworkConfigurationType, proxy, pullSecret, etc.)
API -->> Service: 200/201 InfraEnv
Service -->> Wizard: updated infraEnv (disconnected flow)
Wizard ->> UI: update step list (adds Static IP sub-steps)
UI -->> User: proceed to Static IP / Review (rendezvous IP shown)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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/L
openshift-merge-robot
added
the
needs-rebase
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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 (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
329-337: Consider translating the placeholder text.The placeholder
"e.g., 192.168.1.10"is hardcoded. For consistency with the internationalization approach used elsewhere in the component, consider moving this to a translation key.Suggested change
<InputField label={t('ai:Rendezvous IP')} name="rendezvousIp" helperText={t( 'ai:The IP address that hosts will use to communicate with the bootstrap node during installation.', )} - placeholder="e.g., 192.168.1.10" + placeholder={t('ai:e.g., 192.168.1.10')} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/locales/lib/en/translation.json(8 hunks)libs/types/assisted-installer-service.d.ts(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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:07.992Z
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:07.992Z
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/types/assisted-installer-service.d.ts
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/locales/lib/en/translation.json
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
libs/ui-lib/lib/ocm/components/hosts/ModalDialogsContext.tsx (1)
ModalDialogsContextProvider(146-146)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (4)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/api/utils.ts (1)
getApiErrorMessage(53-58)libs/types/assisted-installer-service.d.ts (2)
InfraEnvUpdateParams(1888-1925)InfraEnvCreateParams(1834-1886)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostsNetworkConfigurationControlGroup.tsx (1)
HostsNetworkConfigurationControlGroup(18-66)
🔇 Additional comments (15)
libs/types/assisted-installer-service.d.ts (3)
1737-1737: LGTM: New image type for disconnected environments.The addition of
'disconnected-iso'to theImageTypeunion correctly extends the type to support the new disconnected cluster installation flow.
1878-1885: LGTM: InfraEnvCreateParams extended with new fields.The new optional fields
rendezvousIpandhostsNetworkConfigurationTypeare well-documented with clear JSDoc comments and appropriate types. The literal union'dhcp' | 'static'provides type safety for the network configuration option.
1917-1924: LGTM: InfraEnvUpdateParams mirrors create params.The update params consistently include the same new fields as the create params, maintaining API symmetry.
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
9-9: LGTM: Context provider wrapping for modal dialogs.Wrapping the wizard in
ModalDialogsContextProvideris the correct pattern for enabling modal dialog context to descendant components. This enables theOptionalConfigurationsStepand other wizard steps to access modal-related functionality.Also applies to: 27-31
libs/locales/lib/en/translation.json (1)
183-183: LGTM: Translation strings for new UI features.The new translation keys are well-organized, follow the existing
ai:namespace convention, and provide clear user-facing messages for the new infrastructure environment configuration features.Also applies to: 213-213, 346-346, 350-350, 360-360, 566-566, 675-675, 744-744, 846-846
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (10)
45-46: LGTM: Well-defined constants with type narrowing.Using
as constassertions ensures these values are typed as literal types rather than generic strings, which provides better type safety when used with the API types.
60-84: LGTM: Clean param construction with conditional spreading.The
buildInfraEnvParamsfunction correctly constructs the API params object, only including fields that have values. The proxy object is appropriately constructed only when at least one proxy field is provided.
86-108: LGTM: Dynamic validation schema with optional IP validation.The validation schema correctly handles the optional
rendezvousIpfield—applying IP validation only when a value is provided. This allows the field to be left empty while ensuring any provided value is a valid IP address.
134-153: LGTM: Cluster fetching with proper error handling.The effect correctly fetches the cluster data and handles errors with translated messages. The
void fetchCluster()pattern properly handles the async function in useEffect.
131-166: LGTM: Sensible initial values and state management.The
editPullSecretstate correctly defaults to expanded when no pull secret is available. Initial form values provide appropriate defaults, withhostsNetworkConfigurationTypedefaulting to DHCP as the most common use case.
171-188: LGTM: Proper custom validation with dynamic schema.The validate function correctly builds a dynamic schema based on current values and extracts all validation errors (using
abortEarly: false) into a Formik-compatible error object.
283-296: LGTM: Proxy settings with conditional display.The proxy configuration section correctly uses the
CheckboxFieldcomponent with a conditionalbodythat rendersProxyInputFieldsonly when the checkbox is enabled.
298-321: LGTM: NTP configuration with conditional display.The NTP sources section follows the same pattern as proxy settings, with the
AdditionalNTPSourcesFieldrendered conditionally based on theenableNtpSourcescheckbox state.
323-327: LGTM: Network configuration control group.The
HostsNetworkConfigurationControlGroupis appropriately included to allow users to select between DHCP and static IP configuration.
202-211: The update params includeimageType: DISCONNECTED_IMAGE_TYPE. The assisted-service backend explicitly supports updatingimageTypeon existing InfraEnv objects throughInfraEnvUpdateParams, and the code correctly follows this pattern.
openshift-merge-robot
removed
the
needs-rebase
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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. |
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
3db2116 to
aa6525b
Compare
openshift-merge-robot
added
needs-rebase
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/locales/lib/en/translation.json(8 hunks)libs/types/assisted-installer-service.d.ts(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T09:08:07.992Z
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:07.992Z
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/types/assisted-installer-service.d.ts
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
libs/ui-lib/lib/ocm/components/hosts/ModalDialogsContext.tsx (1)
ModalDialogsContextProvider(146-146)
⏰ 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: lint
- GitHub Check: circular-deps
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: tests
🔇 Additional comments (4)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
9-9: LGTM! Clean context provider integration.The
ModalDialogsContextProviderwrapper correctly provides modal dialog context to the wizard subtree, enabling child components (like OptionalConfigurationsStep) to access modal functionality during the cluster creation workflow.Also applies to: 27-31
libs/types/assisted-installer-service.d.ts (3)
1737-1737: LGTM! ImageType extended correctly.The new
'disconnected-iso'variant is properly added to theImageTypeunion and aligns with the PR's objective to support disconnected ISO provisioning.
1917-1924: Same verification needed for update parameters.The
InfraEnvUpdateParamsfields mirror the create parameters correctly. Apply the same verification and JSDoc comment improvements suggested forInfraEnvCreateParams.
1878-1885: No further action needed.The new
rendezvousIpandhostsNetworkConfigurationTypefields in the InfraEnvCreateParams interface are well-typed as optional properties with clear JSDoc comments explaining their purpose ('dhcp' or 'static' for network configuration). The field names align with API conventions and the implementation is correct.
libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
670af24 to
5fba11f
Compare
openshift-merge-robot
added
needs-rebase
|
/hold while we confirm releasing these changes. |
openshift-ci
bot
added
the
do-not-merge/hold
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
60816ad to
acfd45f
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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: 3
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
107-107: Inconsistent validation logic for optional IP field.The current approach
values.rendezvousIp ? ipValidationSchema : Yup.string()creates the schema based on whether the field currently has a value. This can lead to issues during validation since the schema is rebuilt each time.A cleaner approach is to make the IP validation optional within the schema itself.
🔎 Proposed fix
- rendezvousIp: values.rendezvousIp ? ipValidationSchema : Yup.string(), + rendezvousIp: Yup.string().test( + 'valid-ip', + 'Must be a valid IP address', + (value) => !value || ipValidationSchema.isValidSync(value) + ),Or if
ipValidationSchemasupports chaining:rendezvousIp: ipValidationSchema.optional(),
260-260: Error boundary title is not translated.For consistency with other translated strings in this component, the error boundary title should also use the translation function.
🔎 Proposed fix
- <WithErrorBoundary title="Failed to load Optional Configurations step"> + <WithErrorBoundary title={t('ai:Failed to load Optional Configurations step')}>Note: You'll need to add the corresponding key to the translation file.
329-337: Consider translating the placeholder text.For full i18n support, the placeholder text
"e.g., 192.168.1.10"could be wrapped in a translation function, though IP address examples are often kept as-is for clarity.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/locales/lib/en/translation.json(8 hunks)libs/types/assisted-installer-service.d.ts(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx
- libs/types/assisted-installer-service.d.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 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-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-12-17T09:08:07.992Z
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:07.992Z
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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/locales/lib/en/translation.json
⏰ 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: unit-tests
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: translation-files
- GitHub Check: tests
- GitHub Check: circular-deps
🔇 Additional comments (4)
libs/locales/lib/en/translation.json (1)
186-186: New translation keys are well-structured.The new translation keys for infrastructure environment configuration, proxy settings, and rendezvous IP follow the established
ai:prefix convention and are properly formatted.Also applies to: 218-218, 351-351, 355-355, 574-574, 683-683, 753-753, 855-855
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
134-153: LGTM - Cluster fetch with proper error handling.The async fetch with error handling and translated error messages follows good practices.
171-188: LGTM - Custom validation handler.The validation function correctly uses
validateSyncwithabortEarly: falseto collect all errors and maps them to Formik's error format.
189-238: LGTM - Well-structured submit handler.The submit logic properly handles both create and update flows with appropriate error handling and translated messages.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
acfd45f to
c426664
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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. |
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
c426664 to
57fe300
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
80-82: Static network configuration uses placeholder data.This is a known issue from past reviews. When
hostsNetworkConfigurationTypeisSTATIC, the code usesgetDummyInfraEnvField()which returns placeholder data rather than actual user-provided configuration. Consider either:
- Adding form fields to collect actual static network configuration values
- Hiding/disabling the STATIC option until the feature is complete
- Adding a TODO comment explaining the incomplete state
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
104-108: Conditional IP validation may cause unexpected behavior.The conditional
values.rendezvousIp ? ipValidationSchema : Yup.string()means validation only applies when the field has a value. However, this approach recalculates the schema on every keystroke, which could cause validation flicker. Consider using a consistent schema that allows empty strings:🔎 Proposed fix
- rendezvousIp: values.rendezvousIp ? ipValidationSchema : Yup.string(), + rendezvousIp: Yup.string().test( + 'ip-validation', + 'Not a valid IP address', + (value) => !value || Address4.isValid(value) || Address6.isValid(value), + ),Note: You'll need to import
Address4andAddress6fromip-addresspackage, or create a helper that wrapsipValidationSchemato allow empty values.
45-46: Consider exporting constants for reuse.
DEFAULT_CPU_ARCHITECTUREandDISCONNECTED_IMAGE_TYPEcould be useful in other parts of the codebase. Consider exporting them or moving to a shared constants file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/locales/lib/en/translation.json(8 hunks)libs/types/assisted-installer-service.d.ts(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 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-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-12-17T09:08:07.992Z
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:07.992Z
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/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/types/assisted-installer-service.d.tslibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/locales/lib/en/translation.json
📚 Learning: 2025-10-21T04:53:07.362Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx:71-91
Timestamp: 2025-10-21T04:53:07.362Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx, dual-stack mode requires both IPv4 and IPv6 networks (machineNetworks must have at least 2 entries: one IPv4 and one IPv6). Dual-stack with only IPv6 is not supported—dual-stack must have both IP families present.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx (1)
RenderableDiscoveryImageType(19-19)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (4)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/api/utils.ts (2)
getApiErrorMessage(53-58)handleApiError(19-51)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts (1)
ProxyInputFields(2-2)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostsNetworkConfigurationControlGroup.tsx (1)
HostsNetworkConfigurationControlGroup(18-66)
⏰ 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: lint
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: tests
🔇 Additional comments (5)
libs/types/assisted-installer-service.d.ts (1)
1737-1737: LGTM!The type definitions are well-structured and consistent. The new
'disconnected-iso'image type and therendezvousIp/hostsNetworkConfigurationTypefields in bothInfraEnvCreateParamsandInfraEnvUpdateParamsalign with the PR objectives for disconnected installation support. The JSDoc comments clearly describe each field's purpose.Also applies to: 1878-1885, 1917-1924
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx (1)
37-40: LGTM!The migration from
ImageTypetoRenderableDiscoveryImageTypefor the props is appropriate. This type excludes'disconnected-iso'since that image type is handled through a separate flow (the newOptionalConfigurationsStep), while the internalOcmImageCreateParamsinterface retains the broaderDiscoveryImageTypefor API compatibility.Also applies to: 75-75, 109-109
libs/locales/lib/en/translation.json (1)
186-186: LGTM!The new translation keys are well-organized, follow the existing
"ai:"prefix convention, and provide clear user-facing messages for the infrastructure environment configuration flow. The merge conflict markers from the previous review have been resolved.Also applies to: 218-218, 351-351, 355-355, 365-366, 571-571, 680-680, 750-750, 852-852
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
189-237: Well-structured submission logic with proper error handling.The form submission logic properly:
- Validates cluster existence before proceeding
- Distinguishes between create and update flows
- Uses translated error messages
- Handles API errors consistently with
handleApiErrorandgetApiErrorMessage
283-337: UI components are well-organized with appropriate conditional rendering.The new form sections for proxy settings, NTP configuration, network configuration, and rendezvous IP follow the existing patterns in the codebase. The conditional rendering based on checkbox states (
values.enableProxy,values.enableNtpSources) prevents unnecessary field clutter.
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
f4df023 to
7a764d1
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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
♻️ Duplicate comments (4)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
46-46: Verify OpenShift version consistency across wizard steps.The version is hardcoded to
'4.22'here and at line 101. A past review flagged inconsistency with ReviewStep showing'4.21'. Please confirm ReviewStep has been updated to match, or extract this version into a shared constant to prevent drift.#!/bin/bash # Check OpenShift version references across disconnected wizard files rg -n "openshiftVersion|4\.2[0-9]" libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ --type ts --type tsx 2>/dev/null || rg -n "openshiftVersion|4\.2[0-9]" libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
77-81: Static network configuration uses placeholder data.The
getDummyInfraEnvField()returns hardcoded placeholder data when users select static network configuration. The comment indicates this is intentional for StaticIpPage rendering, but consider adding a more explicit TODO or feature flag if this is incomplete functionality.Is this the intended behavior for the current milestone? If the static IP form in subsequent steps collects and persists actual configuration, this dummy initialization may be acceptable as a bootstrap value.
60-83: AddhostsNetworkConfigurationTypeto the API payload.The
hostsNetworkConfigurationTypefield is collected from the form but never included in the params sent to the API. According to the type definitions, this should be a top-level field inInfraEnvCreateParams/InfraEnvUpdateParams.🔎 Proposed fix
return { pullSecret: values.pullSecret, ...(values.sshPublicKey && { sshAuthorizedKey: values.sshPublicKey }), ...(hasProxy && { proxy }), ...(values.additionalNtpSources && { additionalNtpSources: values.additionalNtpSources, }), ...(values.rendezvousIp && { rendezvousIp: values.rendezvousIp }), + hostsNetworkConfigurationType: values.hostsNetworkConfigurationType, // Initialize with dummy static network config when static IP is selected // This is required for the StaticIpPage to render properly ...(values.hostsNetworkConfigurationType === HostsNetworkConfigurationType.STATIC && { staticNetworkConfig: getDummyInfraEnvField(), }), };libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx (1)
23-23: New option defined but not rendered in dropdown.The
'disconnected-iso'entry is added todiscoveryImageTypesbut is not included in thedropdownItemsarray (lines 47-84). Users cannot select this option from the UI.🔎 Proposed fix - add the missing dropdown item
{discoveryImageTypes['discovery-image-ipxe']} </DropdownItem>, + <Divider component="li" key="separator3" />, + <DropdownItem + key="disconnected-iso" + id="disconnected-iso" + description={'Use when provisioning from a disconnected environment'} + value={discoveryImageTypes['disconnected-iso']} + > + {discoveryImageTypes['disconnected-iso']} + </DropdownItem>, ];If the disconnected-iso option should only appear in specific contexts (e.g., disconnected wizard only), consider conditionally including it based on a prop or context flag.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
170-187: Consider using Yup'svalidateSyncwithcontextfor cleaner validation.The custom
validatefunction works but rebuilds the schema on every validation call. SincegetValidationSchemadepends onvalues, this is unavoidable with the current approach. However, this pattern is acceptable for forms with conditional validation requirements.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (1)
35-42: Improve error handling for missing infraEnv.The
updateInfraEnvfunction throws a genericErrorifdisconnectedInfraEnv?.idis missing. Since this is an async function passed toStaticIpPage, the error may surface as an unhandled promise rejection depending on howStaticIpPagehandles it.🔎 Consider handling this more gracefully
const updateInfraEnv = async (params: InfraEnvUpdateParams) => { if (!disconnectedInfraEnv?.id) { - throw new Error('No disconnected infraEnv available'); + // This shouldn't happen as component returns null when disconnectedInfraEnv is missing + console.error('updateInfraEnv called without disconnectedInfraEnv'); + throw new Error('No disconnected infraEnv available'); } const { data: updatedInfraEnv } = await InfraEnvsAPI.update(disconnectedInfraEnv.id, params); setDisconnectedInfraEnv(updatedInfraEnv); return updatedInfraEnv; };Alternatively, ensure
StaticIpPagewraps calls toupdateInfraEnvin try-catch and surfaces errors via the form state or alerts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/locales/lib/en/translation.jsonlibs/types/assisted-installer-service.d.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/types/assisted-installer-service.d.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx
- libs/locales/lib/en/translation.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
📚 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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-10-21T04:53:07.362Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx:71-91
Timestamp: 2025-10-21T04:53:07.362Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx, dual-stack mode requires both IPv4 and IPv6 networks (machineNetworks must have at least 2 entries: one IPv4 and one IPv6). Dual-stack with only IPv6 is not supported—dual-stack must have both IP families present.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
libs/ui-lib/lib/ocm/components/hosts/ModalDialogsContext.tsx (1)
ModalDialogsContextProvider(146-146)
⏰ 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: translation-files
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: tests
- GitHub Check: unit-tests
🔇 Additional comments (6)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (2)
20-23: LGTM!The routing correctly maps all three static IP step IDs to the
DisconnectedStaticIpcomponent. TheStaticIpPagecomponent rendered inside handles the internal step transitions based on the current step ID from context.
32-36: LGTM!Wrapping with
ModalDialogsContextProvideris appropriate here to enable modal dialog functionality (e.g., for static IP configuration modals) within the wizard without prop drilling.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (2)
79-104: LGTM!The
getDisconnectedWizardStepIdshelper correctly mirrors the connected flow logic while inserting static IP steps after'disconnected-optional-configurations'instead of'cluster-details'. The additional cleanup on line 100 (removing yaml-view when dhcp-selected) ensures no orphaned steps remain.
279-313: LGTM!The
onUpdateHostNetworkConfigTypehandler correctly branches between connected and disconnected flows, applying the appropriate step ID updaters. This maintains consistency with the dual-path architecture introduced in this PR.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (2)
44-46: LGTM!The
isNextDisabledlogic correctly prevents navigation when auto-save is running, form is invalid, alerts are present, or submission is in progress. This provides a good UX by preventing premature navigation.
61-75: LGTM!The early return when
disconnectedInfraEnvis missing prevents rendering the step without required data. TheStaticIpPageintegration withinfraEnv,updateInfraEnv, andonFormStateChangefollows the expected pattern for this component.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
7a764d1 to
b57c1b7
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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
♻️ Duplicate comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx (1)
19-24: New dropdown option is defined but not rendered in the UI.The
'disconnected-iso'entry is added todiscoveryImageTypesat line 23, but no correspondingDropdownItemis added to thedropdownItemsarray (lines 47-84). Users cannot select this option from the dropdown.Additionally, the
onSelecthandler (line 91) only checks for'discovery-image-ipxe'in theonChangecallback - if'disconnected-iso'requires similar special handling, this also needs updating.🔎 Proposed fix to add the missing dropdown item
{discoveryImageTypes['discovery-image-ipxe']} </DropdownItem>, + <Divider component="li" key="separator3" />, + <DropdownItem + key="disconnected-iso" + id="disconnected-iso" + description={'Use when provisioning from a disconnected environment'} + value={discoveryImageTypes['disconnected-iso']} + > + {discoveryImageTypes['disconnected-iso']} + </DropdownItem>, ];libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
77-81: Static network configuration uses placeholder data as initialization.The comment explains this is intentional - the dummy config initializes the infraEnv so that
StaticIpPagein the subsequent wizard step (DisconnectedStaticIp) can render and allow users to configure the actual static IP values. This is a valid multi-step wizard pattern.However, ensure the
DisconnectedStaticIpcomponent properly updates thestaticNetworkConfigwith real values before the wizard completes.#!/bin/bash # Verify that StaticIpPage updates staticNetworkConfig with actual values rg -n "staticNetworkConfig|updateInfraEnv" libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx -A 3 -B 1
60-83:hostsNetworkConfigurationTypefield is not included in the API params.The
buildInfraEnvParamsfunction validateshostsNetworkConfigurationType(line 103-105) but never includes it in the returned params object. According to the type definitions,hostsNetworkConfigurationTypeshould be a top-level field inInfraEnvCreateParamsandInfraEnvUpdateParams.🔎 Proposed fix
const buildInfraEnvParams = (values: OptionalConfigurationsFormValues) => { // Build proxy object - only include fields that have values const proxy = { ...(values.httpProxy && { httpProxy: values.httpProxy }), ...(values.httpsProxy && { httpsProxy: values.httpsProxy }), ...(values.noProxy && { noProxy: values.noProxy }), }; const hasProxy = Object.keys(proxy).length > 0; return { pullSecret: values.pullSecret, ...(values.sshPublicKey && { sshAuthorizedKey: values.sshPublicKey }), ...(hasProxy && { proxy }), ...(values.additionalNtpSources && { additionalNtpSources: values.additionalNtpSources, }), ...(values.rendezvousIp && { rendezvousIp: values.rendezvousIp }), + hostsNetworkConfigurationType: values.hostsNetworkConfigurationType, // Initialize with dummy static network config when static IP is selected // This is required for the StaticIpPage to render properly ...(values.hostsNetworkConfigurationType === HostsNetworkConfigurationType.STATIC && { staticNetworkConfig: getDummyInfraEnvField(), }), }; };
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (1)
61-63: Consider showing a loading state instead of returning null.When
disconnectedInfraEnvis undefined, the component returnsnull, which may cause a brief blank screen. Consider rendering a loading spinner or skeleton to improve user experience.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
279-313: Consider reducing duplication inonUpdateHostNetworkConfigType.The STATIC and DHCP branches have identical structure in both connected and disconnected flows. This could be simplified by extracting the common pattern.
🔎 Proposed refactor
onUpdateHostNetworkConfigType(type: HostsNetworkConfigurationType): void { - if (installDisconnected) { - // For disconnected wizard, update wizard steps (same pattern as connected) - if (type === HostsNetworkConfigurationType.STATIC) { - setDisconnectedWizardStepIds( - getDisconnectedWizardStepIds(disconnectedWizardStepIds, StaticIpView.FORM), - ); - } else { - setDisconnectedWizardStepIds( - getDisconnectedWizardStepIds(disconnectedWizardStepIds, 'dhcp-selected'), - ); - } + const staticIpView = type === HostsNetworkConfigurationType.STATIC + ? StaticIpView.FORM + : 'dhcp-selected'; + + if (installDisconnected) { + setDisconnectedWizardStepIds( + getDisconnectedWizardStepIds(disconnectedWizardStepIds, staticIpView), + ); } else { - // For connected wizard, update wizard steps directly - if (type === HostsNetworkConfigurationType.STATIC) { - setWizardStepIds( - getWizardStepIds( - wizardStepIds, - StaticIpView.FORM, - customManifestsStep, - isSingleClusterFeatureEnabled, - ), - ); - } else { - setWizardStepIds( - getWizardStepIds( - wizardStepIds, - 'dhcp-selected', - customManifestsStep, - isSingleClusterFeatureEnabled, - ), - ); - } + setWizardStepIds( + getWizardStepIds( + wizardStepIds, + staticIpView, + customManifestsStep, + isSingleClusterFeatureEnabled, + ), + ); } },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/locales/lib/en/translation.jsonlibs/types/assisted-installer-service.d.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
- libs/types/assisted-installer-service.d.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 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-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-10-21T04:53:07.362Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx:71-91
Timestamp: 2025-10-21T04:53:07.362Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx, dual-stack mode requires both IPv4 and IPv6 networks (machineNetworks must have at least 2 entries: one IPv4 and one IPv6). Dual-stack with only IPv6 is not supported—dual-stack must have both IP families present.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (3)
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (2)
ClusterWizardStepsType(21-35)disconnectedSteps(51-55)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts (1)
staticIpFormViewSubSteps(29-32)libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/data/fromInfraEnv.ts (1)
getStaticIpInfo(54-60)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (6)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/data/dummyData.ts (1)
getDummyInfraEnvField(82-87)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/api/utils.ts (2)
getApiErrorMessage(53-58)handleApiError(19-51)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
InfraEnvsAPI(7-7)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts (1)
ProxyInputFields(2-2)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostsNetworkConfigurationControlGroup.tsx (1)
HostsNetworkConfigurationControlGroup(18-66)
⏰ 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: tests
- GitHub Check: format
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: translation-files
🔇 Additional comments (4)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
79-104: Well-structured parallel implementation for disconnected flow.The
getDisconnectedWizardStepIdsfunction correctly mirrors thegetWizardStepIdslogic while adapting the insertion point to'disconnected-optional-configurations'. The dhcp-selected branch properly cleans up both static IP step types.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
106-110: Good fix for rendezvousIp validation.The
.test()approach correctly handles the optional IP field - it allows empty values while validating non-empty inputs againstipValidationSchema. This addresses the concern from the previous review about schema evaluation timing.
125-169: Initial values and form setup look correct.The component correctly:
- Initializes
editPullSecretbased on pull secret availability (line 135)- Sets reasonable defaults for all form fields (lines 158-169)
- Uses
HostsNetworkConfigurationType.DHCPas the default network config type
337-340: No action needed—HostsNetworkConfigurationControlGroupcorrectly wires the form field internally.The component uses Formik's
useFieldhook with the hardcoded field name'hostsNetworkConfigurationType'(line 24), eliminating the need for an explicitnameprop from the parent. The form field is automatically bound to the correct schema field through this mechanism.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
b57c1b7 to
f263c5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
77-81: Acknowledge the dummy data usage is intentional for workflow initialization.The comment explains that
getDummyInfraEnvField()is required for the StaticIpPage to render properly. This appears to be intentional scaffolding that gets replaced with actual configuration in subsequent wizard steps (static-ip-yaml-view,static-ip-network-wide-configurations,static-ip-host-configurations).Please confirm that the actual static network configuration is captured and persisted in the
DisconnectedStaticIpcomponent that handles those subsequent steps.
60-83: Critical:hostsNetworkConfigurationTypeis not included in the API params.The
buildInfraEnvParamsfunction builds configuration but omits thehostsNetworkConfigurationTypefield. According to the type definitions inassisted-installer-service.d.ts(lines 1889-1890 and 1930-1931), this should be a top-level field in bothInfraEnvCreateParamsandInfraEnvUpdateParams.🔎 Proposed fix
const buildInfraEnvParams = (values: OptionalConfigurationsFormValues) => { // Build proxy object - only include fields that have values const proxy = { ...(values.httpProxy && { httpProxy: values.httpProxy }), ...(values.httpsProxy && { httpsProxy: values.httpsProxy }), ...(values.noProxy && { noProxy: values.noProxy }), }; const hasProxy = Object.keys(proxy).length > 0; return { pullSecret: values.pullSecret, ...(values.sshPublicKey && { sshAuthorizedKey: values.sshPublicKey }), ...(hasProxy && { proxy }), ...(values.additionalNtpSources && { additionalNtpSources: values.additionalNtpSources, }), ...(values.rendezvousIp && { rendezvousIp: values.rendezvousIp }), + hostsNetworkConfigurationType: values.hostsNetworkConfigurationType, // Initialize with dummy static network config when static IP is selected // This is required for the StaticIpPage to render properly ...(values.hostsNetworkConfigurationType === HostsNetworkConfigurationType.STATIC && { staticNetworkConfig: getDummyInfraEnvField(), }), }; };
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
279-313: Consider extracting the step update logic to reduce duplication.The connected and disconnected branches share similar patterns. This could be simplified, though it's not critical.
🔎 Optional refactor for reduced duplication
onUpdateHostNetworkConfigType(type: HostsNetworkConfigurationType): void { + const view = type === HostsNetworkConfigurationType.STATIC + ? StaticIpView.FORM + : 'dhcp-selected'; + if (installDisconnected) { - if (type === HostsNetworkConfigurationType.STATIC) { - setDisconnectedWizardStepIds( - getDisconnectedWizardStepIds(disconnectedWizardStepIds, StaticIpView.FORM), - ); - } else { - setDisconnectedWizardStepIds( - getDisconnectedWizardStepIds(disconnectedWizardStepIds, 'dhcp-selected'), - ); - } + setDisconnectedWizardStepIds( + getDisconnectedWizardStepIds(disconnectedWizardStepIds, view), + ); } else { - if (type === HostsNetworkConfigurationType.STATIC) { - setWizardStepIds( - getWizardStepIds( - wizardStepIds, - StaticIpView.FORM, - customManifestsStep, - isSingleClusterFeatureEnabled, - ), - ); - } else { - setWizardStepIds( - getWizardStepIds( - wizardStepIds, - 'dhcp-selected', - customManifestsStep, - isSingleClusterFeatureEnabled, - ), - ); - } + setWizardStepIds( + getWizardStepIds( + wizardStepIds, + view, + customManifestsStep, + isSingleClusterFeatureEnabled, + ), + ); } },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/locales/lib/en/translation.jsonlibs/types/assisted-installer-service.d.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx
- libs/locales/lib/en/translation.json
- libs/types/assisted-installer-service.d.ts
🧰 Additional context used
🧠 Learnings (5)
📓 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-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
📚 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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-10-21T04:53:07.362Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx:71-91
Timestamp: 2025-10-21T04:53:07.362Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx, dual-stack mode requires both IPv4 and IPv6 networks (machineNetworks must have at least 2 entries: one IPv4 and one IPv6). Dual-stack with only IPv6 is not supported—dual-stack must have both IP families present.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (3)
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (2)
ClusterWizardStepsType(21-35)disconnectedSteps(51-55)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts (1)
staticIpFormViewSubSteps(29-32)libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/data/fromInfraEnv.ts (1)
getStaticIpInfo(54-60)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (6)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/data/dummyData.ts (1)
getDummyInfraEnvField(82-87)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/api/utils.ts (2)
getApiErrorMessage(53-58)handleApiError(19-51)libs/ui-lib/lib/common/api/assisted-service/EventsAPI.ts (1)
createParams(21-48)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts (1)
ProxyInputFields(2-2)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostsNetworkConfigurationControlGroup.tsx (1)
HostsNetworkConfigurationControlGroup(18-66)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
libs/ui-lib/lib/ocm/components/hosts/ModalDialogsContext.tsx (1)
ModalDialogsContextProvider(146-146)
⏰ 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: tests
- GitHub Check: format
- GitHub Check: circular-deps
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: lint
🔇 Additional comments (8)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (2)
20-23: Clean fall-through pattern for static IP steps.The shared rendering of
DisconnectedStaticIpfor all three static-ip steps is appropriate since the component likely handles internal navigation between these sub-steps.
32-36: LGTM - ModalDialogsContextProvider integration.Wrapping the wizard content with
ModalDialogsContextProviderappropriately enables modal dialog support for the disconnected static IP workflow.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (2)
79-104: LGTM - Disconnected wizard step management.The
getDisconnectedWizardStepIdsfunction correctly mirrors the connected flow logic, inserting static IP steps afterdisconnected-optional-configurationsinstead ofcluster-details. Thedhcp-selectedcase properly removes all static IP steps.
195-219: The guard is already implemented at line 198.The code already contains the defensive check you're suggesting. The ternary operator
currentInfraEnv ? getStaticIpInfo(currentInfraEnv) : undefinedon line 198 prevents callinggetStaticIpInfo()whencurrentInfraEnvis undefined. IfcurrentInfraEnvis undefined,staticIpInfois set toundefined, which then triggers the clear error message on lines 200-201: "Wizard step is currently ${currentStepId}, but no static ip info is defined". No additional guard is needed.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (4)
106-112: Rendezvous IP validation is correctly implemented.The validation now uses a
.test()pattern that properly handles the optional nature of the field - validating IP format only when a value is present, and allowing empty values.
173-185: Consider whether showing all validation errors on mount is the intended UX.Setting all fields as touched initially (
initialTouched) combined withvalidateOnMountwill show validation errors immediately when the form loads. This is unconventional and may be jarring for optional fields. Typically, validation errors are shown after user interaction.Is this the intended design? If not, consider removing
initialTouchedandvalidateOnMountto follow standard form validation patterns where errors appear after blur/submit.
220-246: Verify the spread order in createParams.The
commonParamsis spread after the explicit fields increateParams. This is correct, but ensure thatcommonParamsdoesn't accidentally overrideclusterId,openshiftVersion, etc. if those fields were ever added tobuildInfraEnvParams.
296-305: LGTM - Rendezvous IP field implementation.The field is properly configured with appropriate label, helper text, placeholder, and max length matching the validation schema (45 characters for IPv6).
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
Add-additional-new-fields-to-the-above-the-sea-UI
branch
from
f263c5a to
f879e66
Compare
|
@ElayAharoni: This pull request references MGMT-22512 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 story 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: 2
🤖 Fix all issues with AI agents
In @libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx:
- Around line 149-156: The label string is inconsistent: change the
DescriptionListTerm text from "Controller Ip" to "Controller IP" in the
ReviewStep component so it matches other labels (e.g., "API IP"); locate the
block that renders disconnectedInfraEnv?.rendezvousIp and update the
DescriptionListTerm content to "Controller IP" (the JSX using
DescriptionListGroup/DescriptionListTerm/DescriptionListDescription).
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (1)
35-42: Consider adding error handling to updateInfraEnv.While the StaticIpPage component likely handles errors from this callback, adding explicit error handling here would improve robustness and provide better error context.
♻️ Proposed enhancement
const updateInfraEnv = async (params: InfraEnvUpdateParams) => { if (!disconnectedInfraEnv?.id) { throw new Error('No disconnected infraEnv available'); } - const { data: updatedInfraEnv } = await InfraEnvsAPI.update(disconnectedInfraEnv.id, params); - setDisconnectedInfraEnv(updatedInfraEnv); - return updatedInfraEnv; + try { + const { data: updatedInfraEnv } = await InfraEnvsAPI.update(disconnectedInfraEnv.id, params); + setDisconnectedInfraEnv(updatedInfraEnv); + return updatedInfraEnv; + } catch (error) { + handleApiError(error, () => { + addAlert({ + title: 'Failed to update infrastructure environment', + message: getApiErrorMessage(error), + variant: AlertVariant.danger, + }); + }); + throw error; // Re-throw to let caller handle if needed + } };Note: You'll need to import
handleApiErrorandgetApiErrorMessagefrom../../../../common/apiif this approach is taken.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
45-45: Consider making CPU architecture configurable.The CPU architecture is hardcoded to
'x86_64', which prevents users from creating disconnected infraEnvs for ARM or other architectures. If multi-architecture support is planned, consider making this configurable through the UI or deriving it from cluster settings.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/locales/lib/en/translation.jsonlibs/types/assisted-installer-service.d.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageTypeDropdown.tsx
- libs/types/assisted-installer-service.d.ts
🧰 Additional context used
🧠 Learnings (8)
📓 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-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 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/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/locales/lib/en/translation.json
📚 Learning: 2025-10-21T04:53:07.362Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx:71-91
Timestamp: 2025-10-21T04:53:07.362Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/StackTypeControl.tsx, dual-stack mode requires both IPv4 and IPv6 networks (machineNetworks must have at least 2 entries: one IPv4 and one IPv6). Dual-stack with only IPv6 is not supported—dual-stack must have both IP families present.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-05-22T07:31:16.062Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2826
File: libs/ui-lib/lib/cim/components/YamlEditor/YamlEditor.tsx:50-61
Timestamp: 2025-05-22T07:31:16.062Z
Learning: The `submitYamls` function in the YamlEditor component (libs/ui-lib/lib/cim/components/YamlEditor/utils.ts) already handles YAML validation with yaml.loadAll() and provides user-friendly error messages. Additional validation in the submit function would be redundant.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
📚 Learning: 2025-05-22T07:31:16.062Z
Learnt from: rawagner
Repo: openshift-assisted/assisted-installer-ui PR: 2826
File: libs/ui-lib/lib/cim/components/YamlEditor/YamlEditor.tsx:50-61
Timestamp: 2025-05-22T07:31:16.062Z
Learning: The YamlEditor component in libs/ui-lib/lib/cim/components/YamlEditor/YamlEditor.tsx already handles YAML validation through the submitYamls function in utils.ts, which validates YAML syntax and throws user-friendly error messages if parsing fails.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
🧬 Code graph analysis (4)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (1)
libs/ui-lib/lib/ocm/components/hosts/ModalDialogsContext.tsx (1)
ModalDialogsContextProvider(146-146)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx (1)
libs/ui-lib/lib/common/types/clusters.ts (1)
DiscoveryImageType(75-75)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (6)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/propTypes.ts (1)
StaticIpFormState(9-16)libs/ui-lib/lib/ocm/store/slices/infra-envs/thunks.ts (1)
updateInfraEnv(40-47)libs/types/assisted-installer-service.d.ts (2)
InfraEnvUpdateParams(1896-1933)Error(1003-1024)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
InfraEnvsAPI(7-7)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFormikErrorFields(34-37)libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/StaticIpPage.tsx (1)
StaticIpPage(30-107)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (3)
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (2)
ClusterWizardStepsType(21-35)disconnectedSteps(51-55)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts (1)
staticIpFormViewSubSteps(29-32)libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/data/fromInfraEnv.ts (1)
getStaticIpInfo(54-60)
🔇 Additional comments (21)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmDiscoveryImageConfigForm.tsx (1)
72-72: LGTM: Type alignment with extended image type ecosystem.The change from
ImageTypetoDiscoveryImageTypealigns with the broader PR changes introducing 'disconnected-iso' support. The direct assignment in initialValues is appropriate since the prop is optional and the form can handle undefined values.Also applies to: 106-106
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (2)
32-36: LGTM: ModalDialogsContextProvider correctly wraps wizard content.The ModalDialogsContextProvider wrapper is properly placed to enable modal dialog functionality for the static IP configuration steps. This aligns with the documented usage pattern.
20-23: LGTM: Clean route mappings for static IP workflow.The three new route cases ('static-ip-yaml-view', 'static-ip-network-wide-configurations', 'static-ip-host-configurations') correctly map to the DisconnectedStaticIp component using a fall-through pattern. All route IDs are properly defined in
ClusterWizardStepsTypeand included in thewizardStepsOrderarray, confirming proper integration into the wizard flow.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (1)
159-159: LGTM: OpenShift version update aligns with PR scope.The version update from 4.20 to 4.22 is consistent with the PR's introduction of disconnected flow support for OpenShift 4.22.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/DisconnectedStaticIp.tsx (3)
12-21: LGTM: Clean initial state factory.The
getInitialFormStatePropshelper provides appropriate defaults for the form state, with safe initial values for all required properties.
44-46: LGTM: Comprehensive navigation state management.The
isNextDisabledlogic correctly considers all relevant form states (auto-save, validation, alerts, submission), anderrorFieldsproperly extracts touched errors for display.
61-75: LGTM: Proper null safety and error boundary usage.The early return for missing
disconnectedInfraEnvprevents runtime errors, and the error boundary aroundStaticIpPageprovides graceful degradation if the component fails.libs/locales/lib/en/translation.json (1)
186-186: LGTM: Translation keys support new disconnected flow features.All new translation keys are well-written with clear, concise messaging that aligns with existing translation patterns. They appropriately support the new infrastructure environment configuration, proxy settings, rendezvous IP, and error handling for the disconnected installation flow.
Note: Per repository practices, non-English translations are handled separately and will be updated through a different workflow.
Also applies to: 218-218, 351-351, 355-355, 365-365, 570-570, 679-679, 749-749, 851-851
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (5)
79-104: LGTM! Disconnected step management mirrors connected flow correctly.The
getDisconnectedWizardStepIdsfunction correctly implements step management for the disconnected wizard flow, appropriately inserting static IP steps after 'disconnected-optional-configurations' instead of 'cluster-details', and omitting custom manifests and single-cluster feature handling which are not applicable to the disconnected flow.
119-120: LGTM! State management is properly initialized.The
disconnectedWizardStepIdsstate is correctly initialized withdisconnectedSteps, and line 138 properly switches between connected and disconnected step arrays based on theinstallDisconnectedflag.Also applies to: 138-138
195-220: LGTM! Static IP step transition handling is correct for both flows.The
handleMoveFromStaticIpfunction correctly determines the current infraEnv based on the installation mode and updates the appropriate wizard steps array using the corresponding getter function. The separation between disconnected and connected flows is clean and maintainable.
257-278: LGTM! Static IP view switching handles both flows appropriately.The
onUpdateStaticIpViewhandler correctly updates the current step ID and wizard steps for both disconnected and connected flows. The disconnected flow appropriately omits custom manifests and single-cluster parameters, which are not applicable to that flow.
279-313: LGTM! Network configuration type handler correctly manages dynamic step injection.The
onUpdateHostNetworkConfigTypehandler properly updates wizard steps when users switch between DHCP and static IP configuration modes, maintaining consistency across both disconnected and connected installation flows.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (8)
90-99: Verify proxy validation allows both HTTP and HTTPS proxy to be empty.The validation schema allows both
httpProxyandhttpsProxyto be empty (allowEmpty: true) even whenenableProxyis true. This means users can enable proxy configuration without providing any proxy URLs, which may not be the intended behavior. Typically, enabling proxy requires at least one of httpProxy or httpsProxy to be specified.Consider whether this is the desired UX or if at least one proxy field should be required when
enableProxyis true.
115-122: LGTM! Validation function follows standard Formik pattern.The
validatefunction correctly integrates the Yup schema with Formik usingvalidateSyncandyupToFormErrorsfor proper error formatting.
140-140: LGTM! Translation and pull secret editing state properly initialized.The i18n integration and pull secret editing logic correctly defaults to showing the pull secret field when no pull secret is available.
Also applies to: 146-146
148-167: LGTM! Cluster fetch effect properly handles errors with i18n.The cluster fetch logic correctly handles errors and displays translated error messages via the alerts system.
169-236: LGTM! Form submission logic correctly handles both create and update flows.The form initialization and submission logic properly:
- Initializes all fields with appropriate defaults
- Validates on mount to catch errors early
- Builds API parameters using the helper function
- Differentiates between creating new and updating existing infraEnvs
- Provides translated error messages
294-330: Form handles conditional fields correctly via buildInfraEnvParams.The conditional rendering of proxy and NTP fields is properly handled. Although toggling the checkboxes off leaves values in the form state, the
buildInfraEnvParamsfunction correctly filters out empty or unused fields before API submission, ensuring clean payloads.
333-336: LGTM! Network configuration control integrated correctly.The
HostsNetworkConfigurationControlGroupcomponent is properly wired to manage thehostsNetworkConfigurationTypeform field, enabling users to switch between DHCP and static IP configuration. This triggers the wizard step updates viaonUpdateHostNetworkConfigTypein the context provider.
79-81: No action needed. The dummystaticNetworkConfigis a legitimate design pattern used consistently across the codebase. The field is optional in the API type definitions, properly structured with required fields (networkYaml,macInterfaceMap), and intentionally replaces user-provided values when the StaticIpPage form is completed. The API accepts this initialization approach.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
Show resolved
Hide resolved
|
Going back in form fails (we wont load the fields). We will address it in a follow up |
|
/lgtm |
openshift-merge-bot
bot
merged commit 2283bcd
into
openshift-assisted:master
…assisted#3337)" This reverts commit 2283bcd.
* Revert "UI freezes when clicking Select button without version (#3401)" This reverts commit 52c39a9. * Revert "OCPBUGS-75072 | OVE UI: Above-the-sea UI QE test (#3383)" This reverts commit f61e1b1. * Revert "OCPBUGS-74421 | OVE UI: Fix rendezvousIP field name in error message (#3376)" This reverts commit 142fdc5. * Revert "Add additional new fields to the above the sea UI (#3337)" This reverts commit 2283bcd. * Revert "add new fields to above sea level ABI (#3274)" This reverts commit 41a3210. * Revert "Assisted Installer OVE UI shows transient blank page with Technology Preview spinner (#3341)" This reverts commit d47ab5f.
6 participants
fixes:
https://issues.redhat.com/browse/MGMT-22512
added proxy, NTP, network config, and rendezvous IP to the Above-the-sea UI.
with validations and sen the right API to create the infraEnv
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.