CONSOLE-4447: Address code review feedback for OLM modals

rhamilto · claude · rhamilto · commit 36ea34c711b4 · 2026-03-09T13:14:31.000-04:00
- Use unique form IDs with useId() in ResourceRequirementsModal to prevent
  ID collisions when multiple modal instances mount
- Prevent modal dismissal during async operations in UninstallOperatorModal
  by disabling onClose when operations are in progress
- Remove redundant onClick handlers from submit buttons in
  disable-default-source-modal and edit-default-sources-modal
- Simplify Radio onChange handlers in installplan-approval-modal to use
  closures with known constant values instead of reading e.target.value
- Remove duplicate InfoCircleIcon from operator-hub-community-provider-modal
  since titleIconVariant="info" already displays the icon

Co-Authored-By: Claude Sonnet 4.5 &lt;noreply@anthropic.com&gt;
diff --git a/frontend/packages/operator-lifecycle-manager/locales/en/olm.json b/frontend/packages/operator-lifecycle-manager/locales/en/olm.json
@@ -257,7 +257,7 @@
   "Manual update approval is required when not installing the latest version for the selected channel.": "Manual update approval is required when not installing the latest version for the selected channel.",
   "Show community Operator": "Show community Operator",
   "Community Operators are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.": "Community Operators are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.",
-  "Learn more about Red Hat’s third party software support policy": "Learn more about Red Hat’s third party software support policy",
+  "Learn more about Red Hat's third party software support policy": "Learn more about Red Hat's third party software support policy",
   "Do not show this warning again": "Do not show this warning again",
   "Continue": "Continue",
   "OperatorHub details": "OperatorHub details",
@@ -278,6 +278,7 @@
   "This Operator is being installed on the cluster.": "This Operator is being installed on the cluster.",
   "Community Operator": "Community Operator",
   "This is a community provided Operator. These are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.": "This is a community provided Operator. These are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.",
+  "Learn more about Red Hat’s third party software support policy": "Learn more about Red Hat’s third party software support policy",
   "Cluster in STS Mode": "Cluster in STS Mode",
   "This cluster is using AWS Security Token Service to reach the cloud API. In order for this operator to take the actions it requires directly with the cloud API, you must provide a role ARN (with an attached policy) during installation. Please see the operator description for more details.": "This cluster is using AWS Security Token Service to reach the cloud API. In order for this operator to take the actions it requires directly with the cloud API, you must provide a role ARN (with an attached policy) during installation. Please see the operator description for more details.",
   "Cluster in Azure Workload Identity / Federated Identity Mode": "Cluster in Azure Workload Identity / Federated Identity Mode",
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx b/frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx
@@ -1,5 +1,5 @@
 import type { FC } from 'react';
-import { useState } from 'react';
+import { useState, useId } from 'react';
 import {
   Button,
   Form,
@@ -97,6 +97,7 @@ export const ResourceRequirementsModal = (props: ResourceRequirementsModalProps)
   const [storage, setStorage] = useState<string>(
     _.get(obj.spec, `${path}.${type}.ephemeral-storage`, ''),
   );
+  const formId = useId();
 
   const submit = (e) => {
     e.preventDefault();
@@ -116,7 +117,7 @@ export const ResourceRequirementsModal = (props: ResourceRequirementsModalProps)
     <>
       <ModalHeader title={props.title} data-test-id="modal-title" />
       <ModalBody>
-        <Form id="resource-requirements-form" onSubmit={(e) => submit(e)}>
+        <Form id={formId} onSubmit={(e) => submit(e)}>
           <Grid hasGutter>
             <GridItem>{props.description}</GridItem>
             <ResourceRequirements
@@ -135,7 +136,7 @@ export const ResourceRequirementsModal = (props: ResourceRequirementsModalProps)
         <Button
           type="submit"
           variant="primary"
-          form="resource-requirements-form"
+          form={formId}
           isLoading={inProgress}
           isDisabled={inProgress}
           data-test="confirm-action"
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx b/frontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx
@@ -64,7 +64,6 @@ const DisableDefaultSourceModal: FC<DisableDefaultSourceModalProps> = ({
         <Button
           type="submit"
           variant="danger"
-          onClick={submit}
           form="disable-default-source-form"
           isLoading={inProgress}
           isDisabled={inProgress}
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx b/frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx
@@ -107,7 +107,6 @@ const EditDefaultSourcesModal: FC<EditDefaultSourcesModalProps> = ({
         <Button
           type="submit"
           variant="primary"
-          onClick={submit}
           form="edit-default-sources-form"
           isLoading={inProgress}
           isDisabled={inProgress}
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx b/frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx
@@ -70,11 +70,7 @@ export const InstallPlanApprovalModal: FC<InstallPlanApprovalModalProps> = ({
               value={InstallPlanApproval.Automatic}
               label={`${t(`olm~Automatic`)} (${t('public~default')})`}
               description={t('olm~New updates will be installed as soon as they become available.')}
-              onChange={(e) =>
-                setSelectedApprovalStrategy(
-                  (e.target as HTMLInputElement).value as InstallPlanApproval,
-                )
-              }
+              onChange={() => setSelectedApprovalStrategy(InstallPlanApproval.Automatic)}
               isChecked={selectedApprovalStrategy === InstallPlanApproval.Automatic}
               data-checked-state={selectedApprovalStrategy === InstallPlanApproval.Automatic}
             />
@@ -86,11 +82,7 @@ export const InstallPlanApprovalModal: FC<InstallPlanApprovalModalProps> = ({
               description={t(
                 'olm~New updates need to be manually approved before installation begins.',
               )}
-              onChange={(e) =>
-                setSelectedApprovalStrategy(
-                  (e.target as HTMLInputElement).value as InstallPlanApproval,
-                )
-              }
+              onChange={() => setSelectedApprovalStrategy(InstallPlanApproval.Manual)}
               isChecked={selectedApprovalStrategy === InstallPlanApproval.Manual}
               data-checked-state={selectedApprovalStrategy === InstallPlanApproval.Manual}
             />
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx b/frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx
@@ -383,7 +383,7 @@ export const UninstallOperatorModal: FC<UninstallOperatorModalProps> = ({
   );
 
   return (
-    <>
+    <Modal variant={ModalVariant.small} isOpen onClose={isSubmitInProgress ? undefined : close}>
       <ModalHeader
         title={t('olm~Uninstall Operator?')}
         titleIconVariant="warning"
@@ -445,7 +445,7 @@ export const UninstallOperatorModal: FC<UninstallOperatorModalProps> = ({
           {t('public~Cancel')}
         </Button>
       </ModalFooterWithAlerts>
-    </>
+    </Modal>
   );
 };
 
@@ -664,9 +664,7 @@ export const UninstallOperatorModalOverlay: OverlayComponent<UninstallOperatorMo
   props,
 ) => {
   return (
-    <Modal variant={ModalVariant.small} isOpen onClose={props.closeOverlay}>
-      <UninstallOperatorModal {...props} close={props.closeOverlay} cancel={props.closeOverlay} />
-    </Modal>
+    <UninstallOperatorModal {...props} close={props.closeOverlay} cancel={props.closeOverlay} />
   );
 };
 
diff --git a/frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx b/frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx
@@ -6,16 +6,12 @@ import {
   Content,
   ContentVariants,
   Form,
-  Icon,
   Modal,
   ModalBody,
   ModalFooter,
   ModalHeader,
   ModalVariant,
-  Split,
-  SplitItem,
 } from '@patternfly/react-core';
-import { InfoCircleIcon } from '@patternfly/react-icons/dist/esm/icons/info-circle-icon';
 import { useTranslation } from 'react-i18next';
 import type { OverlayComponent } from '@console/dynamic-plugin-sdk/src/app/modal-support/OverlayProvider';
 import type { ModalComponentProps } from '@console/internal/components/factory/modal';
@@ -47,36 +43,27 @@ export const OperatorHubCommunityProviderModal: FC<OperatorHubCommunityProviderM
       />
       <ModalBody>
         <Form id="community-provider-form" onSubmit={submit}>
-          <Split hasGutter>
-            <SplitItem>
-              <Icon size="xl" status="info">
-                <InfoCircleIcon />
-              </Icon>
-            </SplitItem>
-            <SplitItem>
-              <Content component={ContentVariants.p}>
-                {t(
-                  'olm~Community Operators are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.',
-                )}
-              </Content>
-              {RH_OPERATOR_SUPPORT_POLICY_LINK && (
-                <Content component={ContentVariants.p}>
-                  <ExternalLink
-                    href={RH_OPERATOR_SUPPORT_POLICY_LINK}
-                    text={t("olm~Learn more about Red Hat's third party software support policy")}
-                  />
-                </Content>
-              )}
-              <Checkbox
-                className="co-modal-ignore-warning__checkbox"
-                onChange={(_event, value) => setIgnoreWarnings(value)}
-                isChecked={ignoreWarnings}
-                data-checked-state={ignoreWarnings}
-                id="do-not-show-warning"
-                label={t('olm~Do not show this warning again')}
+          <Content component={ContentVariants.p}>
+            {t(
+              'olm~Community Operators are Operators which have not been vetted or verified by Red Hat. Community Operators should be used with caution because their stability is unknown. Red Hat provides no support for community Operators.',
+            )}
+          </Content>
+          {RH_OPERATOR_SUPPORT_POLICY_LINK && (
+            <Content component={ContentVariants.p}>
+              <ExternalLink
+                href={RH_OPERATOR_SUPPORT_POLICY_LINK}
+                text={t("olm~Learn more about Red Hat's third party software support policy")}
               />
-            </SplitItem>
-          </Split>
+            </Content>
+          )}
+          <Checkbox
+            className="co-modal-ignore-warning__checkbox"
+            onChange={(_event, value) => setIgnoreWarnings(value)}
+            isChecked={ignoreWarnings}
+            data-checked-state={ignoreWarnings}
+            id="do-not-show-warning"
+            label={t('olm~Do not show this warning again')}
+          />
         </Form>
       </ModalBody>
       <ModalFooter>
diff --git a/frontend/public/locales/en/public.json b/frontend/public/locales/en/public.json
@@ -1802,7 +1802,6 @@
   "Disable": "Disable",
   "Disabled": "Disabled",
   "Enabled": "Enabled",
-  "Select service account": "Select service account",
   "Failed to load kubecontrollermanager": "Failed to load kubecontrollermanager",
   "Failed to parse cloud provider config {{cm}}": "Failed to parse cloud provider config {{cm}}",
   "The following content was expected to be defined in the configMap: {{ expectedValues }}": "The following content was expected to be defined in the configMap: {{ expectedValues }}",
@@ -1811,5 +1810,6 @@
   "Failed to patch cloud provider config": "Failed to patch cloud provider config",
   "Failed to add taint to nodes": "Failed to add taint to nodes",
   "Failed to patch infrastructure spec": "Failed to patch infrastructure spec",
-  "Unexpected error": "Unexpected error"
+  "Unexpected error": "Unexpected error",
+  "Select service account": "Select service account"
 }
