CONSOLE-4447: Migrate operator-lifecycle-manager modals to PatternFly v6#16122
CONSOLE-4447: Migrate operator-lifecycle-manager modals to PatternFly v6#16122rhamilto wants to merge 4 commits intoopenshift:mainfrom
Conversation
Migrate all OLM modals from deprecated PatternFly modal components (ModalTitle, ModalSubmitFooter, ModalWrapper) to PatternFly v6 Modal components (Modal, ModalHeader, ModalBody, ModalFooter/ModalFooterWithAlerts). Changes: - Use PatternFly Form component with external submit button pattern - Replace deprecated Checkbox with PatternFly Checkbox - Add proper data-test-id attributes for Cypress integration tests - Update test files to work with new modal structure - Use ModalFooterWithAlerts for error handling where applicable Migrated modals: - disable-default-source-modal.tsx - edit-default-sources-modal.tsx - installplan-approval-modal.tsx - installplan-preview-modal.tsx - subscription-channel-modal.tsx - uninstall-operator-modal.tsx - update-strategy-modal.tsx - operator-hub-community-provider-modal.tsx - resource-requirements.tsx Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-4447 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto 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
|
@rhamilto: This pull request references CONSOLE-4447 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. |
|
@rhamilto: This pull request references CONSOLE-4447 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. |
📝 WalkthroughWalkthroughThis pull request refactors modal UI across multiple operator lifecycle manager components to standardize on PatternFly Modal composition patterns. Legacy ModalWrapper, ModalTitle, and ModalSubmitFooter components are replaced with PatternFly ModalHeader, ModalBody, and ModalFooterWithAlerts. Forms are now explicitly structured with id attributes and linked via form attributes on submit buttons. Checkbox and Radio components are updated to use PatternFly prop signatures. Test files are updated to mock ModalFooterWithAlerts and adjust submission from form.submit() to button clicks. One new public type, OperatorHubCommunityProviderModalProps, is introduced. Overall logic and navigation flows remain unchanged. 🚥 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)
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: 3
🧹 Nitpick comments (5)
frontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx (1)
64-75: RedundantonClickhandler alongside external submit pattern.The PR uses the external submit button pattern (
type="submit"+form="disable-default-source-form"), which links this button to the form'sonSubmit. However, theonClick={submit}handler intercepts the click and callspreventDefault(), effectively bypassing the form's submission mechanism entirely.This is functional but inconsistent—pick one approach:
- Option A (preferred): Remove
onClickand rely on the form'sonSubmit={submit}. This is cleaner, keeps keyboard submission working via Enter, and aligns with the stated pattern.- Option B: Remove
type="submit"andform="...", keep onlyonClick, and removeonSubmitfrom the Form.♻️ Suggested refactor (Option A)
<Button type="submit" variant="danger" - onClick={submit} form="disable-default-source-form" isLoading={inProgress} isDisabled={inProgress} data-test="confirm-action" id="confirm-action" > {t('public~Disable')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx` around lines 64 - 75, The Button uses both type="submit" with form="disable-default-source-form" and an onClick={submit} handler that calls preventDefault(), which bypasses the form's onSubmit; remove the redundant onClick from the Button and rely on the form's onSubmit={submit} instead so keyboard Enter works and the external submit pattern is consistent—update the Button component (id="confirm-action", data-test="confirm-action") to remove onClick and keep type="submit" and form="disable-default-source-form".frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx (2)
75-79: Verify the checkbox group is actually labelled.
fieldId="enabled-default-sources"does not match any rendered control in this block—the checkboxes each getid={source.name}instead. IfFormGroupstill ties its label tofieldId, assistive tech loses the “Enabled default sources” group context. I’d wire this as a real fieldset/legend or add explicit group labelling witharia-labelledby.Also applies to: 85-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx` around lines 75 - 79, The FormGroup label "Enabled default sources" is not tied to the checkbox controls because fieldId="enabled-default-sources" doesn't match the checkboxes' ids (they use id={source.name}); update the markup so the group label is programmatically associated with the controls by either (a) turning the FormGroup into a proper fieldset/legend (wrap checkboxes in a <fieldset> with a <legend> that contains the label) or (b) keep FormGroup and give the label an id (e.g., "enabled-default-sources-label") and add aria-labelledby="enabled-default-sources-label" on the checkbox container (or add aria-describedby/aria-labelledby on each checkbox to reference that label), adjusting the ids used in the map (where source.name is used for id) accordingly so assistive tech receives the group context for the checkboxes rendered by the component that maps source.name to checkbox ids.
107-116: Let the form own submission.With
type="submit"andform="edit-default-sources-form"already set,onClick={submit}creates a second submit path and callspreventDefault()on the click event rather than the form submit event. DroppingonClickkeeps native submit semantics intact and avoids drifting this handler into a button-click shape later.♻️ Suggested cleanup
<Button type="submit" variant="primary" - onClick={submit} form="edit-default-sources-form" isLoading={inProgress} isDisabled={inProgress}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx` around lines 107 - 116, The Button currently has both type="submit" and form="edit-default-sources-form" but also onClick={submit}, which creates a secondary submit path and calls preventDefault on the click event; remove the onClick prop from the Button (leave type, form, id, isLoading/isDisabled and data-test intact) and ensure the form element with id "edit-default-sources-form" uses the submit handler (submit) via its onSubmit so the form owns submission semantics; verify the submit function is wired to the form's onSubmit and that any event.preventDefault() is applied to the form submit event handler, not a button click.frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx (1)
43-47: Good use oftitleIconVariantfor informational modal.The
titleIconVariant="info"prop onModalHeaderis the correct PF v6 approach for indicating the modal's intent. However, I notice there's also an explicitInfoCircleIconrendered in the body (Lines 52-54). This creates visual redundancy - you have an info icon in both the header and the body.Consider removing the manual icon in the body since the header variant already communicates the informational nature:
♻️ Consider removing redundant icon
<Form id="community-provider-form" onSubmit={submit}> <Split hasGutter> - <SplitItem> - <Icon size="xl" status="info"> - <InfoCircleIcon /> - </Icon> - </SplitItem> <SplitItem> <Content component={ContentVariants.p}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx` around lines 43 - 47, The ModalHeader already uses titleIconVariant="info" to indicate intent, so remove the redundant manual InfoCircleIcon rendered in the modal body (the JSX that imports/uses InfoCircleIcon) to avoid duplicate icons; locate the InfoCircleIcon usage in the operator-hub-community-provider-modal component and delete that element and any adjacent wrapper solely used for presenting the icon while keeping the surrounding explanatory text and translations intact.frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx (1)
67-96: Simplify RadioonChangehandlers using hardcoded values.PatternFly v6's Radio
onChangesignature is(event: React.FormEvent<HTMLInputElement>, checked: boolean) => void. The current implementation extracts value via(e.target as HTMLInputElement).value, which works but is unnecessarily verbose. Since each Radio already has its value available as a constant, use a closure instead:♻️ Suggested refactoring
<Radio id="approval-strategy-automatic" name="approval-strategy" 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} /> <Radio id="approval-strategy-manual" name="approval-strategy" value={InstallPlanApproval.Manual} label={t('olm~Manual')} 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} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx` around lines 67 - 96, The two Radio onChange handlers for the approval strategy (the Radio components with id "approval-strategy-automatic" and "approval-strategy-manual") should be simplified to use closures that set the known constant values instead of reading e.target.value; update the onChange for the Automatic radio to call setSelectedApprovalStrategy(InstallPlanApproval.Automatic) and for the Manual radio to call setSelectedApprovalStrategy(InstallPlanApproval.Manual), matching the PatternFly v6 signature (event, checked) => void and avoiding the cast (e.target as HTMLInputElement).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx`:
- Around line 119-139: The form id "resource-requirements-form" is hard-coded
and can collide when multiple Modal instances mount; generate a unique
per-instance id (e.g., using React's useId() or a useRef/uuid initialized once)
inside the component that renders <Form id=...> and the footer <Button
form=...>, assign it to a local const like formId and replace the literal
"resource-requirements-form" with that formId for both the Form id and the
Button form props so the submit handler (submit) always targets the correct
instance.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsx`:
- Around line 16-17: The test mock for ModalFooterWithAlerts currently only
renders children and bypasses PF submit wiring; update the jest.mock for
ModalFooterWithAlerts to render a minimal submit button that forwards form
submission (e.g., render a <button type="submit">) so tests exercise the
component's real submit path; ensure the mock's exported ModalFooterWithAlerts
(used by InstallPlanApprovalModal tests) still renders children and includes the
submit button so assertions trigger via clicking the button rather than directly
submitting the form.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`:
- Around line 386-448: The modal can be dismissed during uninstall because the
<Modal> and its onClose live in UninstallOperatorModalOverlay while the
disabling logic (isSubmitInProgress, operandsDeleteInProgress,
operatorUninstallInProgress) is inside UninstallOperatorModal; fix by either
moving the <Modal> and onClose back into UninstallOperatorModal so it can gate
closure based on those flags, or add a closeDisabled/isBlocked prop on
UninstallOperatorModalOverlay and pass a boolean (isSubmitInProgress ||
operandsDeleteInProgress || operatorUninstallInProgress) so the overlay prevents
backdrop/X/Esc dismissal while operations are in progress.
---
Nitpick comments:
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx`:
- Around line 64-75: The Button uses both type="submit" with
form="disable-default-source-form" and an onClick={submit} handler that calls
preventDefault(), which bypasses the form's onSubmit; remove the redundant
onClick from the Button and rely on the form's onSubmit={submit} instead so
keyboard Enter works and the external submit pattern is consistent—update the
Button component (id="confirm-action", data-test="confirm-action") to remove
onClick and keep type="submit" and form="disable-default-source-form".
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx`:
- Around line 75-79: The FormGroup label "Enabled default sources" is not tied
to the checkbox controls because fieldId="enabled-default-sources" doesn't match
the checkboxes' ids (they use id={source.name}); update the markup so the group
label is programmatically associated with the controls by either (a) turning the
FormGroup into a proper fieldset/legend (wrap checkboxes in a <fieldset> with a
<legend> that contains the label) or (b) keep FormGroup and give the label an id
(e.g., "enabled-default-sources-label") and add
aria-labelledby="enabled-default-sources-label" on the checkbox container (or
add aria-describedby/aria-labelledby on each checkbox to reference that label),
adjusting the ids used in the map (where source.name is used for id) accordingly
so assistive tech receives the group context for the checkboxes rendered by the
component that maps source.name to checkbox ids.
- Around line 107-116: The Button currently has both type="submit" and
form="edit-default-sources-form" but also onClick={submit}, which creates a
secondary submit path and calls preventDefault on the click event; remove the
onClick prop from the Button (leave type, form, id, isLoading/isDisabled and
data-test intact) and ensure the form element with id
"edit-default-sources-form" uses the submit handler (submit) via its onSubmit so
the form owns submission semantics; verify the submit function is wired to the
form's onSubmit and that any event.preventDefault() is applied to the form
submit event handler, not a button click.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx`:
- Around line 67-96: The two Radio onChange handlers for the approval strategy
(the Radio components with id "approval-strategy-automatic" and
"approval-strategy-manual") should be simplified to use closures that set the
known constant values instead of reading e.target.value; update the onChange for
the Automatic radio to call
setSelectedApprovalStrategy(InstallPlanApproval.Automatic) and for the Manual
radio to call setSelectedApprovalStrategy(InstallPlanApproval.Manual), matching
the PatternFly v6 signature (event, checked) => void and avoiding the cast
(e.target as HTMLInputElement).
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx`:
- Around line 43-47: The ModalHeader already uses titleIconVariant="info" to
indicate intent, so remove the redundant manual InfoCircleIcon rendered in the
modal body (the JSX that imports/uses InfoCircleIcon) to avoid duplicate icons;
locate the InfoCircleIcon usage in the operator-hub-community-provider-modal
component and delete that element and any adjacent wrapper solely used for
presenting the icon while keeping the surrounding explanatory text and
translations intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62551f67-1223-4f40-a40d-51f4c3fd05f1
📒 Files selected for processing (13)
frontend/packages/operator-lifecycle-manager/integration-tests-cypress/views/operator.view.tsfrontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/subscription-channel-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/uninstall-operator-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/installplan-preview-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/subscription-channel-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-community-provider-modal.tsx
| <Form id="resource-requirements-form" onSubmit={(e) => submit(e)}> | ||
| <Grid hasGutter> | ||
| <GridItem>{props.description}</GridItem> | ||
| <ResourceRequirements | ||
| cpu={cpu} | ||
| memory={memory} | ||
| storage={storage} | ||
| onChangeCPU={setCPU} | ||
| onChangeMemory={setMemory} | ||
| onChangeStorage={setStorage} | ||
| path={path} | ||
| /> | ||
| </Grid> | ||
| </Form> | ||
| </ModalBody> | ||
| <ModalSubmitFooter | ||
| errorMessage={errorMessage} | ||
| inProgress={inProgress} | ||
| submitText={t('public~Save')} | ||
| cancel={cancel} | ||
| /> | ||
| </form> | ||
| <ModalFooterWithAlerts errorMessage={errorMessage}> | ||
| <Button | ||
| type="submit" | ||
| variant="primary" | ||
| form="resource-requirements-form" | ||
| isLoading={inProgress} |
There was a problem hiding this comment.
Avoid a shared form id across modal instances.
resource-requirements-form is hard-coded for every render, but the submit button depends on that id via form. If two instances of this modal are mounted at the same time, the DOM ends up with duplicate ids and the footer button can submit the wrong form. Please generate a per-instance id and reuse it for both the Form and the Button.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx`
around lines 119 - 139, The form id "resource-requirements-form" is hard-coded
and can collide when multiple Modal instances mount; generate a unique
per-instance id (e.g., using React's useId() or a useRef/uuid initialized once)
inside the component that renders <Form id=...> and the footer <Button
form=...>, assign it to a local const like formId and replace the literal
"resource-requirements-form" with that formId for both the Form id and the
Button form props so the submit handler (submit) always targets the correct
instance.
| jest.mock('@console/shared/src/components/modals/ModalFooterWithAlerts', () => ({ | ||
| ModalFooterWithAlerts: jest.fn(({ children }) => <div>{children}</div>), |
There was a problem hiding this comment.
Preserve the footer submit path in this mock.
This mock reduces ModalFooterWithAlerts to a passive wrapper, so the spec now bypasses the PF v6 footer/button wiring and submits the <form> directly. That means a broken form association or disabled submit button in InstallPlanApprovalModal would still pass here. Please render a minimal submit button in the mock and drive the submit assertions through that button instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsx`
around lines 16 - 17, The test mock for ModalFooterWithAlerts currently only
renders children and bypasses PF submit wiring; update the jest.mock for
ModalFooterWithAlerts to render a minimal submit button that forwards form
submission (e.g., render a <button type="submit">) so tests exercise the
component's real submit path; ensure the mock's exported ModalFooterWithAlerts
(used by InstallPlanApprovalModal tests) still renders children and includes the
submit button so assertions trigger via clicking the button rather than directly
submitting the form.
| <> | ||
| <ModalHeader | ||
| title={t('olm~Uninstall Operator?')} | ||
| titleIconVariant="warning" | ||
| data-test-id="modal-title" | ||
| /> | ||
| <ModalBody> | ||
| {showInstructions && ( | ||
| <> | ||
| <p> | ||
| <Trans t={t} ns="olm"> | ||
| Operator <strong>{{ name }}</strong> will be removed from{' '} | ||
| <strong>{{ namespace }}</strong>. | ||
| </Trans> | ||
| </p> | ||
| {!optedOut && <>{instructions}</>} | ||
| {uninstallMessage && ( | ||
| <> | ||
| <Title headingLevel="h2" className="pf-v6-u-mb-sm"> | ||
| {t('olm~Message from Operator developer')} | ||
| </Title> | ||
| <p> | ||
| <LinkifyExternal>{uninstallMessage}</LinkifyExternal> | ||
| </p> | ||
| </> | ||
| )} | ||
| {!optedOut && <>{operandsSection}</>} | ||
| </> | ||
| )} | ||
| {operandsDeleteInProgress && ( | ||
| <OperandDeleteProgress total={operands.length} remaining={operandsRemaining} /> | ||
| )} | ||
| {operatorUninstallInProgress && ( | ||
| <div> | ||
| <p>{t('olm~Uninstalling the Operator...')}</p> | ||
| </div> | ||
| )} | ||
| {isSubmitFinished && results} | ||
| <Form id="uninstall-operator-form" onSubmit={submit}> | ||
| {showInstructions && ( | ||
| <> | ||
| <p> | ||
| <Trans t={t} ns="olm"> | ||
| Operator <strong>{{ name }}</strong> will be removed from{' '} | ||
| <strong>{{ namespace }}</strong>. | ||
| </Trans> | ||
| </p> | ||
| {!optedOut && <>{instructions}</>} | ||
| {uninstallMessage && ( | ||
| <> | ||
| <Title headingLevel="h2" className="pf-v6-u-mb-sm"> | ||
| {t('olm~Message from Operator developer')} | ||
| </Title> | ||
| <p> | ||
| <LinkifyExternal>{uninstallMessage}</LinkifyExternal> | ||
| </p> | ||
| </> | ||
| )} | ||
| {!optedOut && <>{operandsSection}</>} | ||
| </> | ||
| )} | ||
| {operandsDeleteInProgress && ( | ||
| <OperandDeleteProgress total={operands.length} remaining={operandsRemaining} /> | ||
| )} | ||
| {operatorUninstallInProgress && ( | ||
| <div> | ||
| <p>{t('olm~Uninstalling the Operator...')}</p> | ||
| </div> | ||
| )} | ||
| {isSubmitFinished && results} | ||
| </Form> | ||
| </ModalBody> | ||
| <ModalSubmitFooter | ||
| inProgress={isSubmitInProgress} | ||
| cancel={cancel} | ||
| submitDanger={!isSubmitFinished} // if submit finished show a non-danger 'OK' | ||
| submitText={isSubmitFinished ? t('olm~OK') : t('olm~Uninstall')} | ||
| submitDisabled={isSubmitInProgress} | ||
| /> | ||
| </form> | ||
| <ModalFooterWithAlerts errorMessage={operatorUninstallErrorMessage}> | ||
| <Button | ||
| type="submit" | ||
| variant={isSubmitFinished ? 'primary' : 'danger'} | ||
| form="uninstall-operator-form" | ||
| isLoading={isSubmitInProgress} | ||
| isDisabled={isSubmitInProgress} | ||
| data-test="confirm-action" | ||
| id="confirm-action" | ||
| > | ||
| {isSubmitFinished ? t('olm~OK') : t('olm~Uninstall')} | ||
| </Button> | ||
| <Button | ||
| variant="link" | ||
| onClick={cancel} | ||
| isDisabled={isSubmitInProgress} | ||
| data-test-id="modal-cancel-action" | ||
| > | ||
| {t('public~Cancel')} | ||
| </Button> | ||
| </ModalFooterWithAlerts> | ||
| </> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the file
find . -type f -name "uninstall-operator-modal.tsx" 2>/dev/nullRepository: openshift/console
Length of output: 158
🏁 Script executed:
# Get the total line count to understand file size
wc -l frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsxRepository: openshift/console
Length of output: 160
🏁 Script executed:
# Read the file to understand the structure - focusing on key areas
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | head -100Repository: openshift/console
Length of output: 4932
🏁 Script executed:
# Check around line 667 where the Modal component is allegedly moved
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | sed -n '650,680p'Repository: openshift/console
Length of output: 1190
🏁 Script executed:
# Check lines 439-443 to see what's disabled
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | sed -n '430,450p'Repository: openshift/console
Length of output: 811
🏁 Script executed:
# Check what triggers isSubmitInProgress to understand the operation flow
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | sed -n '200,250p'Repository: openshift/console
Length of output: 2300
🏁 Script executed:
# Check the submit handler and promise handling
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | sed -n '300,350p'Repository: openshift/console
Length of output: 2039
Move Modal control back into the component or gate dismissal during operations.
The <Modal> and onClose handler are currently in UninstallOperatorModalOverlay (line 667), while only button actions disable during isSubmitInProgress (line 442). This allows users to dismiss the modal via the X button, ESC key, or backdrop click while uninstall or operand-deletion polling is running. The component remains exported as self-contained but has lost dismissal control—either keep the <Modal> in UninstallOperatorModal itself, or pass a isBlocked/closeDisabled signal to the overlay to prevent closure during operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`
around lines 386 - 448, The modal can be dismissed during uninstall because the
<Modal> and its onClose live in UninstallOperatorModalOverlay while the
disabling logic (isSubmitInProgress, operandsDeleteInProgress,
operatorUninstallInProgress) is inside UninstallOperatorModal; fix by either
moving the <Modal> and onClose back into UninstallOperatorModal so it can gate
closure based on those flags, or add a closeDisabled/isBlocked prop on
UninstallOperatorModalOverlay and pass a boolean (isSubmitInProgress ||
operandsDeleteInProgress || operatorUninstallInProgress) so the overlay prevents
backdrop/X/Esc dismissal while operations are in progress.
|
/label tide/merge-method-squash |
- 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 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447-olm
branch
from
d080648 to
36ea34c
Compare
openshift-ci
bot
added
component/core
…iew-modal For read-only/informational modals with a single dismiss button, the OK button should use variant="primary" as it's the main action. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447-olm
branch
from
8cda7fe to
a8a4b54
Compare
Add missing aria-labelledby attributes for WCAG 2.1 compliance and restore
historical cancel button behavior across all 8 migrated modals.
Accessibility fixes:
- Add labelId to all ModalHeader components
- Add matching aria-labelledby to all Modal wrapper components
- Ensures screen readers can properly announce modal titles
UX fixes:
- Remove isDisabled={inProgress} from all cancel buttons
- Restores historical pattern where users can cancel during async operations
- Aligns with console-wide modal behavior (see ModalSubmitFooter history)
Modals updated:
- disable-default-source-modal.tsx
- edit-default-sources-modal.tsx
- installplan-approval-modal.tsx
- installplan-preview-modal.tsx
- subscription-channel-modal.tsx
- uninstall-operator-modal.tsx
- update-strategy-modal.tsx
- operator-hub-community-provider-modal.tsx
Follows patterns from PR openshift#16123 (CONSOLE-4447-app-2).
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@rhamilto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest |
2 participants
Summary
This PR migrates all OLM modals from deprecated PatternFly modal components to PatternFly v6 Modal components, continuing the work from #16015.
Migrated modals
Test plan
cd frontend && yarn test packages/operator-lifecycle-manager/src/components/modals/__tests__🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
Tests