MGMT-22941: ABI: add version dropdown#3370
MGMT-22941: ABI: add version dropdown#3370openshift-merge-bot[bot] merged 2 commits intoopenshift-assisted:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
approved
📝 WalkthroughWalkthroughReplaced several OpenShift-version props and modal setters with context-driven props ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant BasicStep as BasicStep(Formik)
participant API as ClustersService
participant OcmSelect as OcmOpenShiftVersionSelect
User->>Router: Open disconnected wizard (maybe with clusterId)
Router->>BasicStep: Render BasicStep (with clusterId)
BasicStep->>API: GET /clusters/{clusterId} (if provided)
API-->>BasicStep: cluster data or error
alt Cluster exists
BasicStep->>BasicStep: Render read-only openshiftVersion (from cluster)
User->>BasicStep: Click Next
BasicStep->>Router: moveNext
else No cluster
BasicStep->>OcmSelect: Render OcmOpenShiftVersionSelect(minVersionAllowed)
User->>OcmSelect: Select or enter custom version
OcmSelect-->>BasicStep: Set form values.openshiftVersion
User->>BasicStep: Submit to create cluster
BasicStep->>API: POST /clusters {openshiftVersion,...}
API-->>BasicStep: Return new cluster
BasicStep->>Router: Navigate to new cluster path and moveNext
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 |
rawagner
force-pushed
the
abi_dropdown
branch
from
66daf41 to
3a58606
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx`:
- Line 67: The local variable disconnectedClusterId is assigned inside
createCluster but React renders before that assignment so ClusterWizardFooter
always receives undefined; change disconnectedClusterId to component state
(e.g., useState) and update it inside createCluster (or remove the prop if the
footer doesn't need it during initial render) so the value is available to
ClusterWizardFooter—specifically replace the let disconnectedClusterId with a
state hook, set the state in createCluster, and pass the state value to the
ClusterWizardFooter prop.
- Around line 38-56: Move the call to useAlerts() so const { addAlert } =
useAlerts(); is declared before the useEffect, then change the useEffect to an
async-aware pattern: define an inner async function (e.g., fetchCluster) inside
the effect, call await ClustersService.get(clusterId) inside a try/catch/finally
within that async function, call setCluster(...) on success, call
handleApiError(...) and addAlert(...) in the catch, and setIsLoading(false) in
the finally; then invoke fetchCluster() (do not rely on a top-level try/catch
around an async IIFE so errors are caught correctly and addAlert is in scope).
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
124-138: MissingaddAlertin dependency array and scope.
PullSecretSyncusesusePullSecret()which internally usesaddAlertfrom its own context call. However, if there's an error fetching the pull secret, the alert is handled insideusePullSecret. The component itself looks correct for its purpose.One consideration: if the user clears the
pullSecretfield after it was auto-filled, the effect will re-fill it becausepullSecret === ''becomes true again whiledefaultPullSecretis still defined. This might be unexpected UX if the user intentionally cleared the field.Consider tracking if auto-fill already occurred
const PullSecretSync = () => { const defaultPullSecret = usePullSecret(); + const hasAutoFilled = React.useRef(false); const { setFieldValue, values: { pullSecret }, } = useFormikContext<OptionalConfigurationsFormValues>(); React.useEffect(() => { - if (defaultPullSecret !== undefined && pullSecret === '') { + if (defaultPullSecret !== undefined && pullSecret === '' && !hasAutoFilled.current) { setFieldValue('pullSecret', defaultPullSecret); + hasAutoFilled.current = true; } }, [defaultPullSecret, pullSecret, setFieldValue]); return null; };libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
141-141: Consider making the minimum version configurable.The hardcoded
"4.21"minimum version may need updates as new versions are released or support policies change. Consider extracting this to a constant or configuration.// In a constants file or at the top of this file const ABI_MIN_OPENSHIFT_VERSION = '4.21'; // Usage <OcmOpenShiftVersionSelect minVersionAllowed={ABI_MIN_OPENSHIFT_VERSION} />
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
from
3a58606 to
ad5a48d
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
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)
148-173: Expose the pull-secret input field when no default value is available.The pull secret is marked as required in the validation schema (line 88), but the field only renders when
editPullSecretis true. Users without a default pull secret have no way to provide the required value unless they first discover and check the "Edit pull secret" checkbox, creating a hidden dependency that blocks form submission.Suggested fix
- {editPullSecret && <PullSecretField isOcm={isInOcm} />} + {(editPullSecret || !values.pullSecret) && <PullSecretField isOcm={isInOcm} />}Also applies to: 296-302
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx`:
- Around line 126-142: The conditional rendering in BasicStep.tsx can show
OcmOpenShiftVersion with an empty string because isLoading can become false
before cluster is populated; update the rendering logic to only render
OcmOpenShiftVersion when the actual cluster object (e.g., cluster) exists and
has openshiftVersion (not just clusterId/isLoading), otherwise show the Spinner
or fall back to OcmOpenShiftVersionSelect; check the useEffect that populates
cluster to ensure it sets loading appropriately and use a definite check like
"cluster" (or cluster?.openshiftVersion) before rendering OcmOpenShiftVersion so
you never pass an empty string to OcmOpenShiftVersion.
- Around line 156-164: Add form validation to require openshiftVersion and
disable advancing when it's empty: add a Yup schema (e.g., const
validationSchema = Yup.object({ openshiftVersion: Yup.string().required('Select
OpenShift version') })) and pass it into the Formik component
(Formik<BasicStepFormikValues> validationSchema={validationSchema} ...). In the
submit/next flow use Formik's props (isValid, values.openshiftVersion) to
disable the "Next" button (or prevent calling createCluster) when the form is
invalid or openshiftVersion is empty; reference the Formik component,
initialValues.openshiftVersion, and the Next button handler to locate where to
apply these changes.
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Around line 124-135: PullSecretSync currently overwrites the pullSecret
whenever it is empty, which snaps user edits back to the default; change the
logic to only seed the default once when the field is untouched. In the
PullSecretSync component (which uses usePullSecret and
useFormikContext<OptionalConfigurationsFormValues>()), either check formik's
touched state (e.g., touched.pullSecret) and only call
setFieldValue('pullSecret', defaultPullSecret) if the field is not touched, or
use an internal seeded ref (e.g., seededRef) to set the default exactly once
when defaultPullSecret becomes available and pullSecret is empty; ensure you
reference setFieldValue, pullSecret, defaultPullSecret, and useFormikContext in
the updated logic.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
141-141: Verify hardcoded minimum version.The
minVersionAllowed="4.21"is hardcoded. Confirm this is the correct minimum version for the disconnected/ABI workflow and consider whether it should be configurable or derived from a constant.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
2 times, most recently
from
301b591 to
9a21d9b
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
9a21d9b to
35dda7a
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
35dda7a to
e11024a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
213-233: Consider guarding against undefinedcluster.openshiftVersion.According to the
Clustertype definition,openshiftVersionis optional (openshiftVersion?: string). While the cluster existence is validated at line 191,cluster.openshiftVersioncould still be undefined, which would override a valid (albeit truncated) version from the API response withundefined.Suggested defensive approach
setDisconnectedInfraEnv({ ...updatedInfraEnv, // infraEnv does not return the whole OCP version - openshiftVersion: cluster.openshiftVersion, + openshiftVersion: cluster.openshiftVersion ?? updatedInfraEnv.openshiftVersion, });Apply the same pattern to the create path at line 232.
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx`:
- Around line 98-108: The footer currently disables Next when
values.openshiftVersion is empty, which blocks resuming clusters; update the
isNextDisabled prop passed to ClusterWizardFooter in BasicStep.tsx to allow Next
when resuming an existing cluster by considering clusterId (and loading state),
e.g. make isNextDisabled = isLoading || (!clusterId && !values.openshiftVersion)
so that existing clusters (clusterId present) are not blocked while preserving
the disabled behavior for new cluster creation; keep existing callbacks
moveNext() and createCluster() unchanged.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardFooter.tsx (1)
91-93: Remove unusedaddAlertfrom dependency array and unnecessaryasynckeyword.After the simplification,
addAlertis no longer used inhandleCancel, and the function performs no async operations.🧹 Suggested cleanup
- const handleCancel = React.useCallback(async () => { + const handleCancel = React.useCallback(() => { navigate('/cluster-list'); - }, [navigate, addAlert]); + }, [navigate]);
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx`:
- Around line 14-15: Replace the localeCompare-based logic in filterMinVersions
with the project's version-helper: compute comparable ints for each option.value
and minVersion using getComparableVersionInt and filter where optionInt >=
minInt; update function filterMinVersions to call getComparableVersionInt
(keeping signature OpenshiftVersionOptionType[] and minVersion: string) so it
matches isMajorMinorVersionEqualOrGreater's comparison behavior and stays
consistent across the codebase.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (2)
154-164: Missing form validation for requiredopenshiftVersion.The Formik form has no
validationSchema, andopenshiftVersionis initialized as an empty string. WhileisNextDisabledpartially mitigates this, adding explicit validation provides better user feedback and follows form best practices.Suggested approach
+import * as Yup from 'yup'; + +const validationSchema = Yup.object({ + openshiftVersion: Yup.string().required('OpenShift version is required'), +}); + const BasicStep = () => { return ( <Formik<BasicStepFormikValues> initialValues={{ openshiftVersion: '', customOpenshiftSelect: null, }} + validationSchema={validationSchema} onSubmit={() => { // nothing to do }} >
141-141: Hardcoded minimum version constraint.The
minVersionAllowed="4.21"is hardcoded here. If this constraint needs to change based on feature requirements or environment, consider extracting it to a constant or configuration.// At the top of the file or in a shared constants file const MIN_DISCONNECTED_VERSION = '4.21'; // Then use: <OcmOpenShiftVersionSelect minVersionAllowed={MIN_DISCONNECTED_VERSION} />
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
from
e11024a to
43744cc
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx`:
- Line 107: Replace the ambiguous expression used for isNextDisabled in
BasicStep.tsx with an explicit boolean expression to avoid operator-precedence
mistakes: use isNextDisabled={isLoading || (!cluster &&
!values.openshiftVersion)} so the Next button is disabled when loading or when
there is no cluster and no openshiftVersion (locate the isNextDisabled prop in
the BasicStep component).
- Line 141: The OcmOpenShiftVersionSelect is being passed a float literal
(minVersionAllowed={4.21}) but filterMinVersions compares against integers
returned by getComparableVersionInt, so replace the float with the comparable
integer (use getComparableVersionInt('4.21') or the equivalent integer 4021)
when setting minVersionAllowed in BasicStep (referencing
OcmOpenShiftVersionSelect, filterMinVersions, and getComparableVersionInt) so
the version filtering works correctly.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
154-167: Consider adding form validation foropenshiftVersion.The Formik form lacks a
validationSchema, allowing users to proceed without selecting a version (when creating a new cluster). While theisNextDisabledprop provides some protection, explicit validation would provide better UX with field-level error messages.♻️ Suggested validation schema
import * as Yup from 'yup'; const validationSchema = Yup.object({ openshiftVersion: Yup.string().required('OpenShift version is required'), }); // Then in Formik: <Formik<BasicStepFormikValues> initialValues={{ openshiftVersion: '', customOpenshiftSelect: null, }} validationSchema={validationSchema} onSubmit={() => {}} >
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
from
43744cc to
6459a06
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
6459a06 to
bd206c8
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
rawagner
force-pushed
the
abi_dropdown
branch
from
a0355fd to
65b86f6
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
65b86f6 to
34d21da
Compare
rawagner
force-pushed
the
abi_dropdown
branch
from
34d21da to
02e8154
Compare
jgyselov
left a comment
jgyselov
left a comment
There was a problem hiding this comment.
This line also needs to change so that the single cluster flow cluster details form shows the actual OpenShift version. https://github.com/openshift-assisted/assisted-installer-ui/blob/master/libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx#L184
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
note that in single cluster flow, only a single cluster version is returned by the versions API. At this point we do not have cluster resources. The version returned from versions API is what the cluster will use. |
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
b899db4 to
1be7622
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx`:
- Around line 53-56: In OpenShiftVersionModal, guard against setting
openshiftVersion to null by checking values.customOpenshiftSelect before calling
setFieldValue in the Select button onClick handler: if it's truthy, call
setFieldValue('openshiftVersion', values.customOpenshiftSelect) and onClose();
otherwise prevent the action (e.g., keep modal open) or disable the Select
button when values.customOpenshiftSelect is null/undefined; update the Select
button's disabled prop or add validation in the onClick to avoid assigning null.
In `@libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx`:
- Around line 182-186: When isSingleClusterFeatureEnabled is true the expression
ocpVersions[0].label can throw if ocpVersions is empty; update the
forceOpenshiftVersion assignment (the prop passed into whatever component here)
to safely access the first item (e.g., check ocpVersions?.length > 0 or use
optional chaining like ocpVersions?.[0]?.label) and provide a sensible fallback
(such as cluster?.openshiftVersion or undefined) so you never dereference
undefined.
In `@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx`:
- Around line 45-46: The footer's Next enabling and the onNext/moveNext flow can
advance when clusterId exists but cluster is undefined after a failed fetch;
update the guard so Next is disabled (or onNext short‑circuits) when clusterId
is set but cluster is null/undefined or a dedicated loadError flag is true.
Specifically, in BasicStep.tsx adjust the footer enable/disabled logic and the
onNext handler (which calls moveNext) to check for the presence of cluster (or a
loadError state) before allowing moveNext to run; add an explicit load-error
boolean where the fetch occurs if needed and use it in the same checks so users
cannot proceed without a successfully loaded cluster.
- Around line 28-68: Initialize isLoading with !!clusterId and clear cluster
when clusterId is falsy to avoid stale state: change the BasicStepForm useState
initializers so isLoading = React.useState(!!clusterId) and cluster =
React.useState<Cluster | undefined>(undefined), and inside the useEffect
no-clusterId branch call setCluster(undefined) (in the same branch where you
setIsLoading(false)) so that when navigating from a resume route to create route
the previous cluster is cleared and the spinner doesn't flash on first render.
🧹 Nitpick comments (4)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (1)
124-138: PullSecretSync pattern relies on the edit checkbox guard.The sync effect still triggers whenever
pullSecret === '', but since the field is now hidden unlesseditPullSecretis true (line 302), this prevents the reset-on-clear issue in practice. The approach works but is implicit—consider adding a brief comment explaining this coupling.libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (2)
61-68: Default selection may re-trigger ifversionsarray reference changes.The effect depends on
versions, so if the array reference changes (e.g., from a new API response), andfield.valuehappens to be empty, this will reset the selection. Consider tracking whether initial selection has occurred.♻️ Suggested fix using a ref
+ const didInitRef = React.useRef(false); + React.useEffect(() => { - if (!field.value) { + if (!didInitRef.current && !field.value) { const defaultVersion = versions.find((item) => item.default) || versions[0]; if (defaultVersion) { setValue(defaultVersion.value); + didInitRef.current = true; } } }, [field.value, versions, setValue]);
70-70: MemoizeparsedVersionsto avoid recomputing on every render.
getParsedVersions(versions)is called on every render. Sinceversionsis a prop, memoize the result to avoid unnecessary computation.♻️ Suggested fix
- const parsedVersions = getParsedVersions(versions); + const parsedVersions = React.useMemo(() => getParsedVersions(versions), [versions]);libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx (1)
8-33: ClarifyminVersionAllowedunits to prevent silent pass‑through.
filterMinVersionscompares againstgetComparableVersionInt(...), so a caller passing4.21would unintentionally let all versions through. Consider normalizing small numeric inputs (or renaming the prop) to make the expected unit explicit and harder to misuse.♻️ Suggested normalization helper
+const normalizeMinVersion = (minVersion: number) => + minVersion < 1000 ? getComparableVersionInt(minVersion.toString()) : minVersion; + const filterMinVersions = (versions: OpenshiftVersionOptionType[], minVersion: number) => versions.filter((v) => getComparableVersionInt(v.value) >= minVersion); const allVersions = minVersionAllowed - ? filterMinVersions(allVersionsCtx, minVersionAllowed) + ? filterMinVersions(allVersionsCtx, normalizeMinVersion(minVersionAllowed)) : allVersionsCtx; const versions = minVersionAllowed - ? filterMinVersions(latestVersions, minVersionAllowed) + ? filterMinVersions(latestVersions, normalizeMinVersion(minVersionAllowed)) : latestVersions;
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
2 times, most recently
from
e5d810f to
0b83eb8
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx`:
- Around line 61-68: In OpenShiftVersionDropdown's React.useEffect, treat a
stale field.value that is no longer present in versions the same as empty: check
whether versions contains an entry matching field.value (e.g., versions.some(v
=> v.value === field.value)); if not found, pick the defaultVersion
(versions.find(item => item.default) || versions[0]) and call
setValue(defaultVersion.value) to reset the form. Ensure the effect depends on
field.value, versions, and setValue so the reset runs when allowed options
change.
0b83eb8 to
3c3cd87
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rawagner
force-pushed
the
abi_dropdown
branch
from
3c3cd87 to
f67bde0
Compare
openshift-ci
bot
added
size/XL
rawagner
force-pushed
the
abi_dropdown
branch
from
f67bde0 to
d74b583
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterDetailsFormFields.tsx`:
- Line 216: The CustomManifestCheckbox is rendered with clusterId={cluster?.id
|| ''} which can pass an empty string into cleanCustomManifests causing
ClustersAPI.getManifests and ClustersService.removeClusterManifests to be called
with an invalid id; add a guard so cleanCustomManifests returns early when
clusterId is falsy (or prevent rendering CustomManifestCheckbox when cluster is
undefined) and ensure any call sites (CustomManifestCheckbox and
cleanCustomManifests) validate clusterId before calling ClustersAPI.getManifests
or ClustersService.removeClusterManifests to avoid malformed API requests.
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterDetailsFormFields.tsx
Show resolved
Hide resolved
rawagner
force-pushed
the
abi_dropdown
branch
from
d74b583 to
edb5fcd
Compare
|
@rawagner: This pull request references MGMT-22941 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 task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, 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-merge-bot
bot
merged commit 0b7af13
into
openshift-assisted:master
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor