OCPBUGS-76280: Next button is disabled on optional configurations page#3402
Conversation
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
openshift-ci-robot
added
jira/valid-reference
|
@ElayAharoni: This pull request references Jira Issue OCPBUGS-76280, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: bmanzari. Note that only openshift-assisted members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
openshift-ci
bot
added
the
size/M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPull-secret handling in the disconnected cluster wizard was refactored: pullSecret is conditionally validated, removed from wizard context state, initialized from InfraEnv or user input depending on environment/feature flags, and included in infraEnv payloads only when applicable; Formik-driven submission/onNext flow updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Step as OptionalConfigurationsStep
participant Pull as PullSecretComponent
participant Context as ClusterWizardContext
participant API as InfraEnv API
User->>Step: open step / fill form
Step->>Pull: render (non‑OCM only)
User->>Pull: enter pullSecret (if needed)
User->>Step: click Next (submit)
Step->>Step: validate (getValidationSchema(t, requirePullSecret))
Step->>Context: update wizard state (no disconnected pull-secret)
Step->>API: create/update infraEnv (include pullSecretToUse when applicable)
API-->>Step: respond
Step-->>User: navigate next
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
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Line 224: The submit path in OptionalConfigurationsStep.tsx is allowing
defaultPullSecret (from usePullSecret) to be undefined and falling back to ''
which can submit an empty pull secret to InfraEnvsService.create; update the
component to guard against an unresolved pull secret by checking the hook state
(usePullSecret / defaultPullSecret) before enabling Next or running the submit
handler (e.g., handleSubmit/onNext), disabling the Next button (or returning
early from the submit) until defaultPullSecret is defined (or show a loading
state/error) so InfraEnvsService.create never receives an empty string.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
d7b3819 to
1cd137a
Compare
openshift-ci
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx`:
- Line 49: The current line conditionally calls the hook usePullSecret inside a
ternary which violates React's Rules of Hooks; always call usePullSecret()
unconditionally (e.g., const pulled = usePullSecret()) and then set pullSecret
based on isSingleClusterFeatureEnabled and infraEnv (e.g., if
isSingleClusterFeatureEnabled use infraEnv?.pullSecret || '' else use the value
returned by usePullSecret); update the symbol pullSecret assignment in
ClusterDetails.tsx to use the unconditionally-called hook variable and keep the
existing logic using isSingleClusterFeatureEnabled and infraEnv.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
76-91: Pull secret is not restored when navigating back.When the user navigates back to this step,
infraEnvToFormValuesalways setspullSecret: ''(Line 90). In non-OCM flows (!isInOcm), this means the user must re-enter their pull secret even though they already provided it. Consider usinginfraEnv.pullSecret ?? ''if the server returns it, or note that this is intentional (e.g., for security reasons).Possible improvement
- pullSecret: '', + pullSecret: infraEnv.pullSecret ?? '',
238-238: Redundant nullish coalescing.
pullSecretToUseis already guaranteed to be astring(both branches on Line 211 use?? ''), so?? ''here is a no-op.Cleanup
- pullSecret: pullSecretToUse ?? '', + pullSecret: pullSecretToUse,
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx
Outdated
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
1cd137a to
2c383e6
Compare
There was a problem hiding this comment.
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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
86-90:⚠️ Potential issue | 🟡 MinorUse
infraEnv.pullSecretto preserve the pull secret on back-navigation.The
pullSecretfield was added to theInfraEnvtype but is not being used ininfraEnvToFormValues. Currently, line 90 hardcodespullSecret: '', which causes non-OCM users to lose their pull secret when navigating back to this step. This inconsistently deviates from how all other optional fields (likesshPublicKey, proxy settings, andrendezvousIp) are rehydrated using the null coalescing pattern.- pullSecret: '', + pullSecret: infraEnv.pullSecret ?? '',
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
238-238: Redundant nullish coalescing onpullSecretToUse.
pullSecretToUseis already guaranteed to be astring(via?? ''on line 211), so the second?? ''on line 238 is unnecessary.- pullSecret: pullSecretToUse ?? '', + pullSecret: pullSecretToUse,
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
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
Show resolved
Hide resolved
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
2c383e6 to
4efc098
Compare
There was a problem hiding this comment.
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/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
95-117:⚠️ Potential issue | 🟠 MajorMissing
hostsNetworkConfigurationTypein API parameters.
buildInfraEnvParamsconditionally addsstaticNetworkConfigbased on the selected network mode, but never includes thehostsNetworkConfigurationTypefield itself. SinceInfraEnvUpdateParamsdefineshostsNetworkConfigurationType?: 'dhcp' | 'static'as an optional field, it should be explicitly passed to the API instead of relying on implicit inference from the presence ofstaticNetworkConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx` around lines 95 - 117, buildInfraEnvParams currently omits the hostsNetworkConfigurationType property from the returned object (it only conditionally adds staticNetworkConfig), so update buildInfraEnvParams to always include hostsNetworkConfigurationType: values.hostsNetworkConfigurationType (or undefined) alongside the other returned fields; reference the function buildInfraEnvParams, the enum HostsNetworkConfigurationType and the InfraEnvUpdateParams shape to ensure the returned payload explicitly contains hostsNetworkConfigurationType rather than relying on the presence/absence of staticNetworkConfig.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 95-117: buildInfraEnvParams currently omits the
hostsNetworkConfigurationType property from the returned object (it only
conditionally adds staticNetworkConfig), so update buildInfraEnvParams to always
include hostsNetworkConfigurationType: values.hostsNetworkConfigurationType (or
undefined) alongside the other returned fields; reference the function
buildInfraEnvParams, the enum HostsNetworkConfigurationType and the
InfraEnvUpdateParams shape to ensure the returned payload explicitly contains
hostsNetworkConfigurationType rather than relying on the presence/absence of
staticNetworkConfig.
---
Duplicate comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 207-208: OptionalConfigurationsStep currently allows progressing
when isInOcm is true but defaultPullSecret is still undefined; update the Next
button disable logic to prevent advancing until the async defaultPullSecret is
resolved: add a guard (e.g., incorporate (isInOcm && !defaultPullSecret) into
the existing disabled condition) so the Next action and any create/submit paths
that use pullSecretToUse (computed from isInOcm ? defaultPullSecret :
values.pullSecret) cannot run while defaultPullSecret is unresolved; ensure the
same guard is applied wherever pullSecretToUse is used (including the
create/submit handler paths referenced near the use of pullSecretToUse and lines
around 235 and 273).
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
Next-button-is-disabled-on-Optional-configurations-page
branch
from
4efc098 to
d3a7b3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
27-27: Merge the two separate imports from../../../../common/api.Both lines import from the same module path and should be combined into a single import statement.
♻️ Proposed fix
-import { handleApiError, getApiErrorMessage } from '../../../../common/api'; +import { handleApiError, getApiErrorMessage, isInOcm } from '../../../../common/api'; ... -import { isInOcm } from '../../../../common/api';Also applies to: 46-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx` at line 27, The imports for handleApiError and getApiErrorMessage are split but come from the same module; update the import in OptionalConfigurationsStep.tsx to combine them into a single import from '../../../../common/api' (replace the separate import lines that reference handleApiError and getApiErrorMessage with one consolidated import statement that includes both symbols).
191-194: Memoize the validation schema to avoid rebuilding it on every render.
getValidationSchema(t, !isInOcm)creates a newYup.lazywrapper on each render, triggering unnecessary Formik re-validation cycles.♻️ Proposed fix
+ const validationSchema = React.useMemo( + () => getValidationSchema(t, !isInOcm), + [t], + ); + return ( <Formik initialValues={initialValues} validateOnMount - validationSchema={getValidationSchema(t, !isInOcm)} + validationSchema={validationSchema}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx` around lines 191 - 194, The Formik validationSchema is being rebuilt each render because getValidationSchema(t, !isInOcm) returns a new Yup.lazy every time; wrap that call in useMemo so the schema is memoized and only recreated when its dependencies change (e.g., useMemo(() => getValidationSchema(t, !isInOcm), [t, isInOcm])), then pass the memoized value to the Formik validationSchema prop; ensure useMemo is imported and the dependency array includes the translation function/obj (t) and isInOcm flag so Formik re-validates only when those actually change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/types/assisted-installer-service.d.ts`:
- Around line 1841-1844: The InfraEnv type currently exposes a write-only field
pullSecret but the backend appears to only set pull_secret_set/pullSecretSet on
responses; either make the backend return pullSecret on GET or update the
frontend types and code to stop reading a non-existent value: remove pullSecret
from the InfraEnv type (leave InfraEnvCreateParams requiring it for creates) and
change all usages (e.g., ClusterDetails.tsx reading infraEnv?.pullSecret) to
rely on infraEnv?.pullSecretSet (or map pull_secret_set) for presence, and
update any fixtures/tests to reflect the new shape.
In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx`:
- Around line 49-50: The hook call is fine but you must avoid returning an empty
string for the single-cluster path and avoid the wasted fetch: change the
pullSecret assignment to yield undefined when infraEnv.pullSecret is missing
(e.g. pullSecret = isSingleClusterFeatureEnabled ? infraEnv?.pullSecret ??
undefined : defaultPullSecret) and modify usePullSecret to accept an option
(e.g. { skipFetch: boolean }) so ClusterDetails can call usePullSecret({
skipFetch: isSingleClusterFeatureEnabled }) to prevent
AccessTokenAPI.fetchPullSecret() when single-cluster is enabled; keep the
loading gate that checks pullSecret === undefined so the spinner still appears
when the secret is truly not available and pass the resulting pullSecret
(possibly undefined) to ClusterDetailsForm.
---
Duplicate comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 208-217: The OCM flow can submit an empty string for
InfraEnvCreateParams.pullSecret because usePullSecret() (defaultPullSecret)
starts as undefined; update the submit and validation to prevent sending '': in
the submit path where pullSecretToUse is computed (uses isInOcm and
defaultPullSecret), do not fallback to ''—either return/throw early if
defaultPullSecret is still undefined or omit setting pullSecret until it
resolves; also update the Next guard (isNextDisabled) to include a check that
when isInOcm is true defaultPullSecret !== undefined so the button is disabled
until usePullSecret() resolves. Reference symbols: usePullSecret /
defaultPullSecret, pullSecretToUse, isInOcm, isNextDisabled, and
InfraEnvCreateParams.pullSecret.
---
Nitpick comments:
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Line 27: The imports for handleApiError and getApiErrorMessage are split but
come from the same module; update the import in OptionalConfigurationsStep.tsx
to combine them into a single import from '../../../../common/api' (replace the
separate import lines that reference handleApiError and getApiErrorMessage with
one consolidated import statement that includes both symbols).
- Around line 191-194: The Formik validationSchema is being rebuilt each render
because getValidationSchema(t, !isInOcm) returns a new Yup.lazy every time; wrap
that call in useMemo so the schema is memoized and only recreated when its
dependencies change (e.g., useMemo(() => getValidationSchema(t, !isInOcm), [t,
isInOcm])), then pass the memoized value to the Formik validationSchema prop;
ensure useMemo is imported and the dependency array includes the translation
function/obj (t) and isInOcm flag so Formik re-validates only when those
actually change.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, rawagner 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-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit a5428af
into
openshift-assisted:master
|
@ElayAharoni: Jira Issue OCPBUGS-76280: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-76280 has been moved to the MODIFIED state. 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. |
|
Fix included in accepted release 4.22.0-0.nightly-2026-02-26-092444 |
4 participants
https://issues.redhat.com/browse/OCPBUGS-76280
removed the pull secret fields from the form as ti is not necessary.
and for the infraEnv creation api we are using the user's pull secret with the usePullSecret hook.
Summary by CodeRabbit