CONSOLE-4447: Migrate core modals to PatternFly 6 Modal components (part 2)#16123
CONSOLE-4447: Migrate core modals to PatternFly 6 Modal components (part 2)#16123rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
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
component/core
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@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 the modal system across multiple components in the frontend. The primary change migrates modals from legacy 🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/modals/rollback-modal.tsx (1)
122-124:⚠️ Potential issue | 🟡 MinorAdd
.catch()for consistency and to suppress unhandled rejection warnings.
submitDCRollback(line 93) includes.catch(() => {})after the promise chain, butsubmitDeploymentRollbackdoes not. WhileusePromiseHandlerhandles error state internally, the missing catch can produce console warnings for unhandled rejections.🔧 Proposed fix
- handlePromise(k8sPatch(DeploymentModel, deployment, patch)).then(() => { - props.close(); - }); + handlePromise(k8sPatch(DeploymentModel, deployment, patch)) + .then(() => { + props.close(); + }) + .catch(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/rollback-modal.tsx` around lines 122 - 124, The promise returned by handlePromise(k8sPatch(DeploymentModel, deployment, patch)) in submitDeploymentRollback lacks a .catch handler, which can produce unhandled rejection warnings; update submitDeploymentRollback to append .catch(() => {}) (matching submitDCRollback) to the promise chain so errors are swallowed for console noise while usePromiseHandler still manages state—locate the handlePromise call around k8sPatch/DeploymentModel/deployment/patch and add the .catch accordingly before finishing with props.close().frontend/public/components/modals/cluster-channel-modal.tsx (1)
45-46:⚠️ Potential issue | 🟠 MajorPotential null pointer dereference when
semver.parsereturnsnull.
semver.parse()returnsnullwhen the version string is invalid or unparseable. Lines 96-107 accessversion.majorandversion.minorunconditionally within the!channelsExistbranch, which would throw ifversionisnull.Consider adding a fallback or guard:
🛡️ Proposed fix with null guard
const version = semver.parse(getLastCompletedUpdate(cv)); + const versionDisplay = version ? `${version.major}.${version.minor}` : 'X.Y';Then use
versionDisplayin the placeholder and helper text templates:- version: `stable-${version.major}.${version.minor}`, + version: `stable-${versionDisplay}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/cluster-channel-modal.tsx` around lines 45 - 46, semver.parse(getLastCompletedUpdate(cv)) can return null so avoid dereferencing version; change the logic in cluster-channel-modal.tsx around the const version = semver.parse(getLastCompletedUpdate(cv)) and subsequent !channelsExist branch to first compute a safe versionDisplay (e.g., if version is null use a fallback like "unknown" or the original string) and only access version.major/major/ minor when version is non-null; replace direct uses of version.major/version.minor in placeholder/helper text with the safe versionDisplay and add a guard before any code that assumes version is an object.
🧹 Nitpick comments (6)
frontend/public/components/modals/rollback-modal.tsx (1)
228-228: Minor inconsistency in disabled logic vs. error display condition.Line 208 evaluates
!loadError && !deploymentError(checks existence ofloadError), while line 228 checksloadError?.message || deploymentError. IfloadErrorexists but lacks amessageproperty, the Alert would render but the button would remain enabled.Consider aligning these conditions:
♻️ Suggested alignment
- isDisabled={!!(loadError?.message || deploymentError)} + isDisabled={!!(loadError || deploymentError)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/rollback-modal.tsx` at line 228, The disabled condition for the action button in rollback-modal.tsx is inconsistent with the Alert rendering: the Alert uses loadError?.message but the button uses isDisabled={!!(loadError?.message || deploymentError)}, so if loadError exists without a message the Alert may render while the button remains enabled; update the logic so both use the same predicate—either change the button's isDisabled to use !!(loadError || deploymentError) or change the Alert rendering to check loadError?.message—ensuring the symbols referenced (loadError, deploymentError, isDisabled) are used consistently.frontend/public/components/modals/taints-modal.tsx (1)
107-123: Consider using PatternFly TextInput for consistency.The current approach wraps raw
<input>elements in spans withpf-v6-c-form-controlclass. For full PatternFly 6 alignment and better maintainability, consider migrating toTextInputcomponents. This is optional given the migration scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/taints-modal.tsx` around lines 107 - 123, Replace the raw input elements wrapped in spans in the taints modal with PatternFly TextInput components to ensure consistent styling; specifically change the inputs rendering c.key and c.value (the elements using value={c.key} / value={c.value} and onChange={(e) => change(e, i, 'key'|'value')}) to use TextInput from PatternFly, wire its value and onChange handlers to call the existing change function (adapting the TextInput onChange signature as needed), and remove the manual pf-v6-c-form-control spans so the component (taints-modal.tsx) uses TextInput for both key and value fields consistently.frontend/public/components/modals/tolerations-modal.tsx (1)
242-248: TolerationsModalOverlay uses ModalWrapper while TaintsModalOverlay uses PF Modal directly.The
TolerationsModalOverlaywraps content inModalWrapper(which usesreact-modalinternally per the relevant snippet), while the similarTaintsModalOverlayuses<Modal>from PatternFly directly. This creates an architectural inconsistency.If
ModalWrapperis required for theblockingbehavior, consider documenting this or aligning the pattern across all modals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/tolerations-modal.tsx` around lines 242 - 248, TolerationsModalOverlay is inconsistent with TaintsModalOverlay because it uses ModalWrapper (which delegates to react-modal) while TaintsModalOverlay uses PatternFly's Modal; align them by either replacing ModalWrapper with PatternFly's Modal in TolerationsModalOverlay or by updating TaintsModalOverlay to use ModalWrapper so both overlays share the same modal implementation. Locate TolerationsModalOverlay and TaintsModalOverlay and decide on a single modal component (ModalWrapper or Modal), then update the chosen overlay(s) to use that component and ensure blocking/close behavior is preserved (prop names: blocking, onClose/closeOverlay) and add a brief comment documenting why ModalWrapper is required if you keep it.frontend/packages/console-app/src/actions/hooks/useCommonActions.ts (1)
13-13: Lazy-loadTaintsModalOverlayfor consistency with other modals.
LazyTolerationsModalOverlayis imported as a lazy component (line 10), butTaintsModalOverlayuses a direct import (line 13). To maintain architectural consistency and enable code splitting for this modal, create aLazyTaintsModalOverlayexport inmodals/index.tsand update the imports and usage inuseCommonActions.ts(lines 13 and 146) to use the lazy variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/actions/hooks/useCommonActions.ts` at line 13, TaintsModalOverlay is directly imported while other modals use lazy wrappers; add a LazyTaintsModalOverlay export in the modals index (matching the pattern used by LazyTolerationsModalOverlay) and then replace the direct TaintsModalOverlay import in the useCommonActions hook with LazyTaintsModalOverlay and update its usage where the taints modal is opened so the lazy component is used instead of the direct import.frontend/public/components/modals/cluster-update-modal.tsx (2)
269-281: Minor a11y note:fieldIddoesn't match any child elementid.The
FormGroupspecifiesfieldId="update-options", but neither Radio button uses thatid. PatternFly usesfieldIdto associate the label with a form control. Since these are radio buttons with individualidattributes, this is functionally fine, but for strict a11y compliance, consider wrapping the radios in arole="radiogroup"witharia-labelledbypointing to the group label, or removing thefieldIdsince the radios self-label.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/cluster-update-modal.tsx` around lines 269 - 281, The FormGroup currently sets fieldId="update-options" but no child control uses that id; update the markup around the Radio buttons (the group rendered inside FormGroup in cluster-update-modal.tsx) to ensure proper accessibility by either removing the unused fieldId from FormGroup or by wrapping the radios in a container with role="radiogroup" and aria-labelledby that points to the FormGroup label id (e.g., the element rendered by FormGroup); adjust the radio container and/or label id so the label is programmatically associated with the radio group while keeping each Radio's individual id intact.
151-157: Consider adding explicit.catch()for completeness.The
handlePromiseutility captures errors internally, but the.then(() => close())chain lacks a trailing.catch(). Ifclose()ever throws (unlikely but defensive), it would be an unhandled rejection. This is minor sincecloseOverlayis a simple state setter.♻️ Optional: Add catch for defensive error handling
handlePromise( Promise.all([ k8sPatch(ClusterVersionModel, cv, patch), ...MCPsToResumePromises, ...MCPsToPausePromises, ]), - ).then(() => close()); + ) + .then(() => close()) + .catch(() => {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/cluster-update-modal.tsx` around lines 151 - 157, The Promise chain using handlePromise(Promise.all([k8sPatch(ClusterVersionModel, cv, patch), ...MCPsToResumePromises, ...MCPsToPausePromises])).then(() => close()) should add a trailing .catch() to defensively handle any exceptions thrown by close() (or any unexpected errors after handlePromise resolves); update the call site referencing handlePromise, k8sPatch, ClusterVersionModel, MCPsToResumePromises, MCPsToPausePromises and the close function to append .catch(err => { /* handle or log error, e.g. console.error or processLogger */ }) so rejections are not left unhandled.
🤖 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/public/components/cluster-settings/cluster-settings.tsx`:
- Around line 912-920: The effect that checks URLSearchParams and calls
launchModal (with LazyClusterUpdateModalOverlay or
LazyClusterChannelModalOverlay) can re-run when the cv object reference changes
and re-create URLSearchParams each time, causing duplicate modals; change the
logic so the URL param is read/memoized once (e.g., compute a
hasShowVersions/hasShowChannels boolean outside or via useMemo/useRef) and add a
local ref flag (e.g., modalOpenedRef) to ensure launchModal is only invoked once
per page visit, and still call removeQueryArgument after launching; keep
launchModal, removeQueryArgument and the overlay identifiers unchanged while
guarding repeated invocations.
In `@frontend/public/components/modals/cluster-channel-modal.tsx`:
- Around line 136-147: The Modal in ClusterChannelModalOverlay is missing an
aria-labelledby attribute so screen readers won't pick up the ModalHeader title;
update the Modal element in the ClusterChannelModalOverlay component to include
aria-labelledby="cluster-channel-modal-title" (the id used by the
ClusterChannelModal/ModalHeader title) so the Modal's accessible name is
properly wired when rendering <ClusterChannelModal ... />.
In `@frontend/public/components/modals/cluster-more-updates-modal.tsx`:
- Line 84: The Close button translation call is missing the `public~` namespace
prefix; inside the ClusterMoreUpdatesModal component replace the bare t('Close')
usage with the namespaced key t('public~Close') so it matches the other
translations in this file (e.g., the other t('public~...') calls) and keeps
consistent i18n lookup for the Close button.
In `@frontend/public/components/modals/column-management-modal.tsx`:
- Line 157: The DataList id contains a typo: change the id value
"defalt-column-management" to "default-column-management" wherever it appears
(e.g., in the DataList element identified by id="defalt-column-management") to
correct the spelling for consistency with any tests or accessibility references;
update any corresponding references (tests, labels, aria attributes) that rely
on this id to use "default-column-management" as well.
In `@frontend/public/components/modals/delete-pvc-modal.tsx`:
- Around line 79-88: The Delete Button currently uses isLoading={inProgress} but
remains clickable; update the Button (the element with id="confirm-action" and
form="delete-pvc-form") to also set isDisabled={inProgress} so the button is
actually disabled during the in-flight delete action; keep the existing
isLoading prop and ensure the change is applied in delete-pvc-modal.tsx where
the Button component is rendered.
- Around line 59-64: The Modal is missing an aria-labelledby attribute to
associate it with the ModalHeader title; update the parent <Modal> component
that contains ModalHeader to include aria-labelledby="delete-pvc-modal-title"
(matching the ModalHeader's labelId) so screen readers correctly link the dialog
to its visible title; locate the Modal wrapper around the
ModalHeader/ModalFooter in delete-pvc-modal.tsx and add the aria-labelledby prop
with that ID.
In `@frontend/public/components/modals/taints-modal.tsx`:
- Line 136: The Tooltip content currently uses the literal string "Remove";
update it to use the translation function like the aria-label does so it reads
Tooltip content={t('Remove')}. Locate the Tooltip element in the TaintsModal
component (the Tooltip wrapper around the remove button) and replace the static
"Remove" text with t('Remove') to ensure i18n consistency.
---
Outside diff comments:
In `@frontend/public/components/modals/cluster-channel-modal.tsx`:
- Around line 45-46: semver.parse(getLastCompletedUpdate(cv)) can return null so
avoid dereferencing version; change the logic in cluster-channel-modal.tsx
around the const version = semver.parse(getLastCompletedUpdate(cv)) and
subsequent !channelsExist branch to first compute a safe versionDisplay (e.g.,
if version is null use a fallback like "unknown" or the original string) and
only access version.major/major/ minor when version is non-null; replace direct
uses of version.major/version.minor in placeholder/helper text with the safe
versionDisplay and add a guard before any code that assumes version is an
object.
In `@frontend/public/components/modals/rollback-modal.tsx`:
- Around line 122-124: The promise returned by
handlePromise(k8sPatch(DeploymentModel, deployment, patch)) in
submitDeploymentRollback lacks a .catch handler, which can produce unhandled
rejection warnings; update submitDeploymentRollback to append .catch(() => {})
(matching submitDCRollback) to the promise chain so errors are swallowed for
console noise while usePromiseHandler still manages state—locate the
handlePromise call around k8sPatch/DeploymentModel/deployment/patch and add the
.catch accordingly before finishing with props.close().
---
Nitpick comments:
In `@frontend/packages/console-app/src/actions/hooks/useCommonActions.ts`:
- Line 13: TaintsModalOverlay is directly imported while other modals use lazy
wrappers; add a LazyTaintsModalOverlay export in the modals index (matching the
pattern used by LazyTolerationsModalOverlay) and then replace the direct
TaintsModalOverlay import in the useCommonActions hook with
LazyTaintsModalOverlay and update its usage where the taints modal is opened so
the lazy component is used instead of the direct import.
In `@frontend/public/components/modals/cluster-update-modal.tsx`:
- Around line 269-281: The FormGroup currently sets fieldId="update-options" but
no child control uses that id; update the markup around the Radio buttons (the
group rendered inside FormGroup in cluster-update-modal.tsx) to ensure proper
accessibility by either removing the unused fieldId from FormGroup or by
wrapping the radios in a container with role="radiogroup" and aria-labelledby
that points to the FormGroup label id (e.g., the element rendered by FormGroup);
adjust the radio container and/or label id so the label is programmatically
associated with the radio group while keeping each Radio's individual id intact.
- Around line 151-157: The Promise chain using
handlePromise(Promise.all([k8sPatch(ClusterVersionModel, cv, patch),
...MCPsToResumePromises, ...MCPsToPausePromises])).then(() => close()) should
add a trailing .catch() to defensively handle any exceptions thrown by close()
(or any unexpected errors after handlePromise resolves); update the call site
referencing handlePromise, k8sPatch, ClusterVersionModel, MCPsToResumePromises,
MCPsToPausePromises and the close function to append .catch(err => { /* handle
or log error, e.g. console.error or processLogger */ }) so rejections are not
left unhandled.
In `@frontend/public/components/modals/rollback-modal.tsx`:
- Line 228: The disabled condition for the action button in rollback-modal.tsx
is inconsistent with the Alert rendering: the Alert uses loadError?.message but
the button uses isDisabled={!!(loadError?.message || deploymentError)}, so if
loadError exists without a message the Alert may render while the button remains
enabled; update the logic so both use the same predicate—either change the
button's isDisabled to use !!(loadError || deploymentError) or change the Alert
rendering to check loadError?.message—ensuring the symbols referenced
(loadError, deploymentError, isDisabled) are used consistently.
In `@frontend/public/components/modals/taints-modal.tsx`:
- Around line 107-123: Replace the raw input elements wrapped in spans in the
taints modal with PatternFly TextInput components to ensure consistent styling;
specifically change the inputs rendering c.key and c.value (the elements using
value={c.key} / value={c.value} and onChange={(e) => change(e, i,
'key'|'value')}) to use TextInput from PatternFly, wire its value and onChange
handlers to call the existing change function (adapting the TextInput onChange
signature as needed), and remove the manual pf-v6-c-form-control spans so the
component (taints-modal.tsx) uses TextInput for both key and value fields
consistently.
In `@frontend/public/components/modals/tolerations-modal.tsx`:
- Around line 242-248: TolerationsModalOverlay is inconsistent with
TaintsModalOverlay because it uses ModalWrapper (which delegates to react-modal)
while TaintsModalOverlay uses PatternFly's Modal; align them by either replacing
ModalWrapper with PatternFly's Modal in TolerationsModalOverlay or by updating
TaintsModalOverlay to use ModalWrapper so both overlays share the same modal
implementation. Locate TolerationsModalOverlay and TaintsModalOverlay and decide
on a single modal component (ModalWrapper or Modal), then update the chosen
overlay(s) to use that component and ensure blocking/close behavior is preserved
(prop names: blocking, onClose/closeOverlay) and add a brief comment documenting
why ModalWrapper is required if you keep it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3e14f62-3642-41a1-94ff-b76592c36de2
📒 Files selected for processing (15)
frontend/packages/console-app/src/actions/hooks/useCommonActions.tsfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/public/components/modals/__tests__/column-management-modal.spec.tsxfrontend/public/components/modals/add-users-modal.tsxfrontend/public/components/modals/cluster-channel-modal.tsxfrontend/public/components/modals/cluster-more-updates-modal.tsxfrontend/public/components/modals/cluster-update-modal.tsxfrontend/public/components/modals/column-management-modal.tsxfrontend/public/components/modals/delete-pvc-modal.tsxfrontend/public/components/modals/error-modal.tsxfrontend/public/components/modals/index.tsfrontend/public/components/modals/rollback-modal.tsxfrontend/public/components/modals/taints-modal.tsxfrontend/public/components/modals/tolerations-modal.tsxfrontend/public/locales/en/public.json
💤 Files with no reviewable changes (1)
- frontend/public/components/modals/add-users-modal.tsx
| useEffect(() => { | ||
| if (new URLSearchParams(window.location.search).has('showVersions')) { | ||
| launchModal(LazyClusterUpdateModalOverlay, { cv }); | ||
| removeQueryArgument('showVersions'); | ||
| } else if (new URLSearchParams(window.location.search).has('showChannels')) { | ||
| launchModal(LazyClusterChannelModalOverlay, { cv }); | ||
| removeQueryArgument('showChannels'); | ||
| } | ||
| }, [launchModal, cv, removeQueryArgument]); |
There was a problem hiding this comment.
Potential issue with useEffect modal launching on URL params.
The useEffect creates new URLSearchParams on each execution and has cv in the dependency array. If cv object reference changes (common with K8s watch resources), this effect will re-run. Combined with the query param removal happening asynchronously, there's a potential race condition where the modal could be launched multiple times.
Consider:
- Using a ref to track if the modal has already been opened for the current URL params
- Moving the URLSearchParams check outside the effect body or memoizing it
🛡️ Suggested defensive pattern
+ import { useRef } from 'react';
...
+ const modalLaunchedRef = useRef(false);
+
useEffect(() => {
+ if (modalLaunchedRef.current) return;
+ const params = new URLSearchParams(window.location.search);
- if (new URLSearchParams(window.location.search).has('showVersions')) {
+ if (params.has('showVersions')) {
+ modalLaunchedRef.current = true;
launchModal(LazyClusterUpdateModalOverlay, { cv });
removeQueryArgument('showVersions');
- } else if (new URLSearchParams(window.location.search).has('showChannels')) {
+ } else if (params.has('showChannels')) {
+ modalLaunchedRef.current = true;
launchModal(LazyClusterChannelModalOverlay, { cv });
removeQueryArgument('showChannels');
}
}, [launchModal, cv, removeQueryArgument]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/cluster-settings/cluster-settings.tsx` around
lines 912 - 920, The effect that checks URLSearchParams and calls launchModal
(with LazyClusterUpdateModalOverlay or LazyClusterChannelModalOverlay) can
re-run when the cv object reference changes and re-create URLSearchParams each
time, causing duplicate modals; change the logic so the URL param is
read/memoized once (e.g., compute a hasShowVersions/hasShowChannels boolean
outside or via useMemo/useRef) and add a local ref flag (e.g., modalOpenedRef)
to ensure launchModal is only invoked once per page visit, and still call
removeQueryArgument after launching; keep launchModal, removeQueryArgument and
the overlay identifiers unchanged while guarding repeated invocations.
| export const ClusterChannelModalOverlay: OverlayComponent<ClusterChannelModalProps> = (props) => { | ||
| return ( | ||
| <Modal | ||
| isOpen | ||
| onClose={props.closeOverlay} | ||
| variant={ModalVariant.small} | ||
| data-test="channel-modal" | ||
| > | ||
| <ClusterChannelModal {...props} close={props.closeOverlay} cancel={props.closeOverlay} /> | ||
| </Modal> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly 6 Modal aria-labelledby ModalHeader integration
💡 Result:
In PatternFly 6, wire the modal’s accessible name by making the Modal’s aria-labelledby point at the title element inside ModalHeader:
import { Modal, ModalHeader, ModalBody, ModalFooter, Button } from '@patternfly/react-core';
const titleId = 'my-modal-title';
const descId = 'my-modal-desc';
<Modal
isOpen={isOpen}
onClose={onClose}
aria-labelledby={titleId} // should include the ModalHeader labelId
aria-describedby={descId} // optional; should match ModalHeader descriptorId
>
<ModalHeader
title="Delete project?"
labelId={titleId} // id of the modal title element
description="This action cannot be undone."
descriptorId={descId} // id of the modal description element
/>
<ModalBody>...</ModalBody>
<ModalFooter>...</ModalFooter>
</Modal>Modal.aria-labelledby is explicitly documented to “include the ModalHeader labelId,” and ModalHeader.labelId is the “Id of the modal title.” [1] In the HTML/accessibility guidance, aria-labelledby is the id of the element that labels (titles) the modal; if there’s no title, use aria-label instead. [2][3]
🏁 Script executed:
cd frontend/public/components/modals && cat -n cluster-channel-modal.tsx | head -100Repository: openshift/console
Length of output: 4680
🏁 Script executed:
cd frontend/public/components/modals && sed -n '130,150p' cluster-channel-modal.tsxRepository: openshift/console
Length of output: 546
Add aria-labelledby to the Modal to properly wire the accessible title.
Per PatternFly 6 documentation, the Modal component requires an explicit aria-labelledby attribute that references the ModalHeader's labelId. Add aria-labelledby="cluster-channel-modal-title" to the Modal wrapper (line 138) to ensure screen readers correctly announce the modal's accessible name.
Suggested fix
<Modal
isOpen
onClose={props.closeOverlay}
variant={ModalVariant.small}
data-test="channel-modal"
aria-labelledby="cluster-channel-modal-title"
>
<ClusterChannelModal {...props} close={props.closeOverlay} cancel={props.closeOverlay} />
</Modal>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/cluster-channel-modal.tsx` around lines 136
- 147, The Modal in ClusterChannelModalOverlay is missing an aria-labelledby
attribute so screen readers won't pick up the ModalHeader title; update the
Modal element in the ClusterChannelModalOverlay component to include
aria-labelledby="cluster-channel-modal-title" (the id used by the
ClusterChannelModal/ModalHeader title) so the Modal's accessible name is
properly wired when rendering <ClusterChannelModal ... />.
| onClick={cancel} | ||
| data-test="more-updates-modal-close-button" | ||
| > | ||
| {t('Close')} |
There was a problem hiding this comment.
Translation key missing namespace prefix.
Other translations in this file use the public~ namespace prefix (e.g., lines 36, 47, 48). For consistency, the Close button text should also use the namespace prefix.
🌐 Proposed fix
- {t('Close')}
+ {t('public~Close')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {t('Close')} | |
| {t('public~Close')} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/cluster-more-updates-modal.tsx` at line 84,
The Close button translation call is missing the `public~` namespace prefix;
inside the ClusterMoreUpdatesModal component replace the bare t('Close') usage
with the namespaced key t('public~Close') so it matches the other translations
in this file (e.g., the other t('public~...') calls) and keeps consistent i18n
lookup for the Close button.
| </label> | ||
| <DataList | ||
| aria-label={t('public~Default column list')} | ||
| id="defalt-column-management" |
There was a problem hiding this comment.
Typo in DataList id: "defalt" should be "default".
Minor typo that could cause issues if this id is referenced elsewhere for testing or accessibility.
✏️ Proposed fix
<DataList
aria-label={t('public~Default column list')}
- id="defalt-column-management"
+ id="default-column-management"
isCompact
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id="defalt-column-management" | |
| <DataList | |
| aria-label={t('public~Default column list')} | |
| id="default-column-management" | |
| isCompact | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/column-management-modal.tsx` at line 157,
The DataList id contains a typo: change the id value "defalt-column-management"
to "default-column-management" wherever it appears (e.g., in the DataList
element identified by id="defalt-column-management") to correct the spelling for
consistency with any tests or accessibility references; update any corresponding
references (tests, labels, aria attributes) that rely on this id to use
"default-column-management" as well.
| <ModalHeader | ||
| title={t('public~Delete PersistentVolumeClaim')} | ||
| titleIconVariant="warning" | ||
| data-test-id="modal-title" | ||
| labelId="delete-pvc-modal-title" | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly 6 Modal aria-labelledby usage with ModalHeader labelId
💡 Result:
In PatternFly 6 React, wire the modal’s accessible name to the actual title element by using the same ID for:
Modalaria-labelledbyModalHeaderlabelId(this is the ID applied to the rendered modal title) [1]
Example:
<Modal
isOpen
aria-labelledby="my-modal-title"
aria-describedby="my-modal-description"
onClose={onClose}
>
<ModalHeader
title="Delete project?"
labelId="my-modal-title"
description="This action cannot be undone."
descriptorId="my-modal-description"
/>
<ModalBody>...</ModalBody>
<ModalFooter>...</ModalFooter>
</Modal>Notes:
- PatternFly’s docs explicitly say
Modal’saria-labelledby“should include the ModalHeaderlabelId” [1], and thataria-labelledbyis required when a visible title is present [2][3]. - If you don’t render a header/title, use
Modal’saria-labelinstead [1][2].
Sources: [1] [2] [3]
Add aria-labelledby to the Modal component for accessibility compliance.
The labelId="delete-pvc-modal-title" on ModalHeader correctly creates an id for the title element, but the parent <Modal> component needs aria-labelledby="delete-pvc-modal-title" to properly associate the dialog with its title for screen readers. This is a required PatternFly 6 pattern when a visible title is present.
Proposed fix
- <Modal isOpen onClose={props.closeOverlay} variant={ModalVariant.small}>
+ <Modal
+ isOpen
+ onClose={props.closeOverlay}
+ variant={ModalVariant.small}
+ aria-labelledby="delete-pvc-modal-title"
+ >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/delete-pvc-modal.tsx` around lines 59 - 64,
The Modal is missing an aria-labelledby attribute to associate it with the
ModalHeader title; update the parent <Modal> component that contains ModalHeader
to include aria-labelledby="delete-pvc-modal-title" (matching the ModalHeader's
labelId) so screen readers correctly link the dialog to its visible title;
locate the Modal wrapper around the ModalHeader/ModalFooter in
delete-pvc-modal.tsx and add the aria-labelledby prop with that ID.
| <Button | ||
| type="submit" | ||
| variant="danger" | ||
| isLoading={inProgress} | ||
| form="delete-pvc-form" | ||
| data-test="confirm-action" | ||
| id="confirm-action" | ||
| > | ||
| {t('public~Delete')} | ||
| </Button> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PatternFly 6 Button isLoading prop does it disable button automatically
💡 Result:
No. In PatternFly v6, Button’s isLoading only adds progress/loading styling—it does not automatically disable the button. Disabling is controlled separately via isDisabled (or isAriaDisabled). [1]
Typical pattern:
<Button isLoading={isLoading} isDisabled={isLoading}>
Save
</Button>[1] (staging-v6.patternfly.org)
Citations:
Add isDisabled={inProgress} to prevent double-submission during delete.
PatternFly v6's isLoading prop only adds loading styling—it does not disable the button. Users can click the Delete button multiple times while the operation is in flight, causing unintended behavior. Pair isLoading with isDisabled={inProgress}:
Required fix
<Button
type="submit"
variant="danger"
isLoading={inProgress}
+ isDisabled={inProgress}
form="delete-pvc-form"
data-test="confirm-action"
id="confirm-action"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| type="submit" | |
| variant="danger" | |
| isLoading={inProgress} | |
| form="delete-pvc-form" | |
| data-test="confirm-action" | |
| id="confirm-action" | |
| > | |
| {t('public~Delete')} | |
| </Button> | |
| <Button | |
| type="submit" | |
| variant="danger" | |
| isLoading={inProgress} | |
| isDisabled={inProgress} | |
| form="delete-pvc-form" | |
| data-test="confirm-action" | |
| id="confirm-action" | |
| > | |
| {t('public~Delete')} | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/delete-pvc-modal.tsx` around lines 79 - 88,
The Delete Button currently uses isLoading={inProgress} but remains clickable;
update the Button (the element with id="confirm-action" and
form="delete-pvc-form") to also set isDisabled={inProgress} so the button is
actually disabled during the in-flight delete action; keep the existing
isLoading prop and ensure the change is applied in delete-pvc-modal.tsx where
the Button component is rendered.
| /> | ||
| </Td> | ||
| <Td isActionCell> | ||
| <Tooltip content="Remove"> |
There was a problem hiding this comment.
Missing translation for Tooltip content.
The Tooltip content "Remove" should use the translation function for i18n compliance. You're already using t('Remove') for the aria-label on line 144, so this should be consistent.
🌐 Proposed fix
- <Tooltip content="Remove">
+ <Tooltip content={t('Remove')}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Tooltip content="Remove"> | |
| <Tooltip content={t('Remove')}> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/taints-modal.tsx` at line 136, The Tooltip
content currently uses the literal string "Remove"; update it to use the
translation function like the aria-label does so it reads Tooltip
content={t('Remove')}. Locate the Tooltip element in the TaintsModal component
(the Tooltip wrapper around the remove button) and replace the static "Remove"
text with t('Remove') to ensure i18n consistency.
28b122d to
f71e9b8
Compare
|
@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
force-pushed
the
CONSOLE-4447-app-2
branch
from
f71e9b8 to
d5b553e
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
force-pushed
the
CONSOLE-4447-app-2
branch
3 times, most recently
from
91860c3 to
0007a6d
Compare
Addresses code review feedback with fixes for accessibility, error handling, i18n, null safety, and code quality:
Accessibility improvements:
- Add aria-labelledby to Modal components linking to ModalHeader labelId
- Fix FormGroup in cluster-update-modal to use role="radiogroup" instead of unused fieldId
- Add aria-label to TextInput components in taints-modal
- Add isDisabled to delete button while operation in progress
Error handling:
- Add .catch(() => {}) to promise chains in cluster-update-modal and rollback-modal
i18n fixes:
- Add missing 'public~' namespace prefix to Close button translation
- Translate Tooltip content in taints-modal
Null safety:
- Add defensive fallback for semver.parse in cluster-channel-modal using nullish coalescing
Code quality:
- Fix duplicate modal rendering in cluster-settings using useRef to track modal state
- Use useQueryParamsMutator hook's getQueryArgument for URL params
- Replace raw input elements with PatternFly TextInput components in taints-modal
- Fix inconsistent error checking in rollback-modal
- Fix typo: defalt-column-management → default-column-management
- Migrate taints-modal to lazy loading pattern (LazyTaintsModalOverlay)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447-app-2
branch
from
0007a6d to
82af86a
Compare
|
@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
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Tech debt |
openshift-ci
bot
added
docs-approved
|
@rhamilto: all tests passed! 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. |
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>
|
/assign @vikram-raj |
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>
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
changed the title
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>
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>
4 participants
Summary
Migrates multiple modals from legacy
createModalLauncher/ModalWrapperto the modernuseOverlaypattern with PatternFly 6 Modal components.Migrated Modals
Cluster Settings & Updates:
cluster-channel-modal- Update cluster channel selectioncluster-more-updates-modal- Display additional update pathscluster-update-modal- Cluster version updates with MCP pause optionsResource Management:
delete-pvc-modal- Delete PersistentVolumeClaim with extension point supportrollback-modal- Rollback deployments and deployment configscolumn-management-modal- Manage table column visibilityNode Configuration:
taints-modal- Edit node taints with key/value/effect configurationtolerations-modal- Edit pod/node tolerationsUtility Modals:
error-modal- Generic error display modal (addeduseErrorModalLauncherhook)configure-count-modal- Configure replica/parallelism countsCode Review Fixes
Accessibility improvements:
aria-labelledbyto all Modal components linking to ModalHeaderlabelIdrole="radiogroup"instead of unusedfieldIdaria-labelto TextInput components in taints-modalisDisabledto delete button while operation in progressError handling:
.catch(() => {})to promise chains in cluster-update-modal and rollback-modali18n fixes:
public~namespace prefix to Close button translationNull safety:
semver.parsein cluster-channel-modal using nullish coalescingCode quality:
useRefto track modal stateuseQueryParamsMutatorhook'sgetQueryArgumentfor URL paramsdefalt-column-management→default-column-managementLazyTaintsModalOverlay)modals/index.tsTest plan
yarn i18nto update translation keys🤖 Generated with Claude Code