CONSOLE-4447: Migrate core modals to PatternFly v6 modal components (part 1)#16100
CONSOLE-4447: Migrate core modals to PatternFly v6 modal components (part 1)#16100rhamilto 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/knative
rhamilto
changed the title
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. |
|
/retest |
1 similar comment
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447-core
branch
3 times, most recently
from
4ba1ef0 to
ae6f268
Compare
|
/retest |
1 similar comment
|
/retest |
rhamilto
force-pushed
the
CONSOLE-4447-core
branch
from
ae6f268 to
ee674f7
Compare
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci
bot
added
the
docs-approved
openshift-ci
bot
added
the
px-approved
rhamilto
force-pushed
the
CONSOLE-4447-core
branch
from
ee674f7 to
1120313
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. |
📝 WalkthroughWalkthroughThis pull request systematically refactors multiple modal components across the OpenShift Console frontend to adopt a consistent PatternFly-based modal architecture. The changes migrate away from a custom modal wrapper pattern (ModalWrapper, ModalTitle, ModalSubmitFooter) toward PatternFly's native Modal components (ModalHeader, ModalBody, ModalVariant). Each modal now uses ModalFooterWithAlerts for consistent error and loading state presentation, and implements local isOpen state management within the overlay component. Forms are explicitly wired to submit buttons via form attributes. One public API is renamed (AddSecretToWorkloadModalProvider to AddSecretToWorkloadModalOverlay), and its props signature is extended with ModalComponentProps. The majority of modal logic remains functionally equivalent; the changes are primarily architectural and compositional. A minor CSS rule (.co-m-radio-desc margin) is removed, likely to align with the refactored form layouts. 🚥 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
78-99:⚠️ Potential issue | 🟠 MajorRelax the MIME check and trust JSON.parse instead.
The browser
File.typeproperty is unreliable for JSON uploads. Per the W3C File API specification,File.typereturns either a parsable MIME type or an empty string if the browser cannot determine one. In practice, browsers determine type based on file extension rather than content, so users uploading.jsonfiles may see different MIME types across browsers and operating systems. The strict checkfile.type !== 'application/json'will reject valid JSON files before content validation runs.Remove the MIME type gate and let
JSON.parseserve as the source of truth. You can still guide users toward JSON files with theacceptattribute on the input element.Possible fix
const file = event.target.files?.[0]; - if (!file || file.type !== 'application/json') { + if (!file) { setInvalidJson(true); return; } const reader = new FileReader(); reader.onload = (e) => {<input type="file" accept=".json,application/json" id="namespace-pull-secret-file" onChange={onFileChange} aria-describedby="namespace-pull-secret-file-help" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/configure-ns-pull-secret-modal.tsx` around lines 78 - 99, The file upload handler onFileChange currently rejects files based on file.type which is unreliable; remove the MIME gate that checks file.type !== 'application/json' and instead rely on JSON.parse inside the FileReader.onload to validate content (keep calling setInvalidJson(true) on parse errors and setFileData(input) on success), and update the corresponding file input to use accept=".json,application/json" so users are guided to select JSON files without enforcing MIME-based rejection.
🧹 Nitpick comments (3)
frontend/public/components/modals/managed-resource-save-modal.tsx (2)
21-25: Consider defensive error handling ifonSubmitcan throw or is async.The submit handler calls
props.close()unconditionally afterprops.onSubmit(). IfonSubmitis asynchronous or can throw, the modal will close regardless of success or failure. Since this appears to be a confirmation modal (user acknowledges the managed resource warning), the risk is likely low, but wrapping in try/catch or awaiting would be more defensive.🛡️ Optional: Add defensive handling
- const submit = (event) => { + const submit = async (event) => { event.preventDefault(); - props.onSubmit(); - props.close(); + try { + await props.onSubmit(); + props.close(); + } catch (e) { + // If onSubmit is sync and cannot fail, this is a no-op + console.error('Failed to submit:', e); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/managed-resource-save-modal.tsx` around lines 21 - 25, The submit handler in submit currently calls props.onSubmit() and then props.close() unconditionally; make submit async, await props.onSubmit(), and wrap the await in a try/catch so the modal only closes on success (call props.close() after a successful await), and handle or rethrow errors in the catch (e.g., surface an error state or call an onError callback) to avoid closing the modal if onSubmit throws or rejects.
74-89: Conditional rendering pattern works but bypasses PatternFly Modal animations.The current implementation uses
isOpen ? <Modal isOpen ...> : null, which unmounts the Modal immediately on close. Notice that theisOpenprop is hardcoded totrue—it doesn't control visibility since the conditional rendering already does. This works functionally but loses PatternFly's exit animations and may not properly manage focus trapping on the way out.PatternFly's Modal is designed to remain mounted and use the
isOpenprop to orchestrate enter/exit transitions. Consider refactoring to leverage this:♻️ Keep Modal mounted and use isOpen for state management
export const ManagedResourceSaveModalOverlay: OverlayComponent<ManagedResourceSaveModalProps> = ( props, ) => { const [isOpen, setIsOpen] = useState(true); const handleClose = () => { setIsOpen(false); }; + const handleAfterClose = () => { + props.closeOverlay(); + }; + - return isOpen ? ( + return ( <Modal variant={ModalVariant.small} - isOpen + isOpen={isOpen} onClose={handleClose} + afterClose={handleAfterClose} aria-labelledby="managed-resource-save-modal-title" > <ManagedResourceSaveModal {...props} close={handleClose} /> </Modal> - ) : null; + ); };Alternatively, if PatternFly's Modal lifecycle callbacks don't align with your cleanup timing, the current conditional pattern is acceptable—just document why animation loss is acceptable for this overlay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/managed-resource-save-modal.tsx` around lines 74 - 89, The Modal is being unmounted by the conditional render which bypasses PatternFly exit animations; instead keep the Modal mounted and control visibility with the isOpen state: remove the ternary that returns null and render <Modal ...> unconditionally, set its isOpen prop to the component's isOpen state (the useState variable named isOpen), keep onClose wired to handleClose, and continue passing close={handleClose} into ManagedResourceSaveModal so focus/cleanup runs when Modal finishes its exit transition.frontend/public/components/modals/labels-modal.tsx (1)
117-135: The FormGroup label is correctly associated with SelectorInput—both hardcode the same id value.Verification confirms
SelectorInputhardcodesid: 'tags-input'on its underlying input (selector-input.jsx, line 149), which matches thefieldId="tags-input"inFormGroup. The label-to-control wiring works correctly and will announce properly to assistive technologies.However, improve maintainability by wiring the id explicitly: pass
fieldIdas a prop toSelectorInputinstead of both hardcoding'tags-input'. This eliminates the fragile coupling if someone changes one without the other. Consider updatingSelectorInputto accept an optionalidorinputIdprop, defaulting to'tags-input'for backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/modals/labels-modal.tsx` around lines 117 - 135, FormGroup currently uses fieldId="tags-input" while SelectorInput hardcodes id:'tags-input' internally; make the association explicit by adding a prop (e.g., id or inputId) to SelectorInput and pass the FormGroup's fieldId into SelectorInput from labels-modal (replace hardcoded value usage in SelectorInput with the new prop while defaulting to 'tags-input' for backward compatibility), updating the SelectorInput implementation (selector-input.jsx) to consume that prop and apply it to its underlying input so label-to-control wiring remains correct and maintainable.
🤖 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/modals/add-secret-to-workload.tsx`:
- Around line 278-287: The Button component with id "confirm-action" currently
has an onClick={submit} which creates a secondary submission path that bypasses
native form validation; remove the onClick prop from the Button in
add-secret-to-workload.tsx so the button relies on type="submit" and
form="co-add-secret-to-workload" to trigger the form's onSubmit handler (submit)
and allow the browser/FormGroup (mountPath isRequired) validation to run
normally.
- Around line 307-315: The modal currently spreads {...props} after explicitly
passing close={handleClose} and cancel={handleClose}, allowing incoming
props.close/props.cancel to override handleClose and break cleanup; update the
JSX so that {...props} is spread first on the Modal/AddSecretToWorkloadModal and
then explicitly pass close={handleClose} and cancel={handleClose} (referencing
AddSecretToWorkloadModal, Modal, handleClose) so handleClose (which calls
setIsOpen(false) and closeOverlay()) cannot be overwritten by incoming props.
- Around line 196-219: The FormGroup that renders the radio group (label "Add
secret as") is reusing the form's id via fieldId="co-add-secret-to-workload",
breaking label-to-field association; update that FormGroup's fieldId to a unique
identifier such as "co-add-secret-to-workload__add-as" (same file's
add-secret-to-workload modal, FormGroup for the radio group) so the label
correctly targets the radio controls.
In `@frontend/public/components/modals/configure-cluster-upstream-modal.tsx`:
- Around line 96-142: The FormGroup used as the radio group currently has an
unrelated fieldId ("co-add-secret-to-workload") and each Radio uses a different
name ("config-default" vs "config-custom"), breaking native radio grouping;
update the FormGroup.fieldId to a meaningful id like "cluster-version-config"
and make both Radio.name values identical (e.g., "cluster-version-config") so
the radios form a proper native group while keeping the existing
isChecked/onChange handlers in the Radio components.
In `@frontend/public/components/modals/configure-ns-pull-secret-modal.tsx`:
- Around line 312-325: The cancel button and the modal overlay/header close
should be disabled while the submit chain is running: when inProgress is true,
make the Cancel Button (the one calling cancel with
data-test-id="modal-cancel-action") disabled or hidden the same as the primary
Button, and ensure the modal's close handler (the header X / overlay onClose) is
no-op or prevented while inProgress is true (so the header close cannot dismiss
mid-submit). Update both the Footer block containing
ModalFooterWithAlerts/Button and the other occurrences referenced (lines
~341-347) to guard the modal close path and the cancel action using the
inProgress flag so no dismissal can occur until the create/patch completes.
- Around line 300-304: The modal currently renders uploaded pull-secret contents
into the DOM via fileData inside CodeBlock/CodeBlockCode (in
ConfigureNsPullSecretModal), which exposes sensitive tokens; instead stop
rendering raw fileData and replace it with a non-sensitive UI: show the uploaded
filename or a generic "file loaded" message and a validation/status badge, keep
any validation logic that inspects fileData but do not inject the raw JSON into
JSX or aria text, and ensure any existing references to fileData for display are
removed or replaced with a safeDisplayName variable used by the UI components.
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx`:
- Around line 221-235: Disable all dismiss paths while the patch is pending: add
inProgress guards to the Cancel button and the modal close handler so users
cannot dismiss while k8sPatch is running. Specifically, set the Cancel <Button>
(the one using cancel and data-test-id="modal-cancel-action") to
isDisabled={inProgress} and modify the modal's onClose/closeOverlay handler to
early-return when inProgress is true (or wrap cancel to check inProgress before
calling closeOverlay). Ensure ModalFooterWithAlerts/Modal onClose behavior and
the cancel function both respect the inProgress flag so neither Cancel nor
clicking outside/close icon will close the modal while the request is in flight.
In `@frontend/public/components/modals/expand-pvc-modal.tsx`:
- Around line 80-88: The RequestSizeInput is receiving a translated string for
its name prop (name={t('public~requestSize')}), which causes generated form
field names (e.g., requestSizeValue, requestSizeUnit) to vary by locale; change
the prop to a static identifier (name="requestSize") in the ExpandPVCModal where
RequestSizeInput is rendered so the form field names remain consistent across
locales and match how create-pvc.tsx uses it.
In `@frontend/public/components/modals/labels-modal.tsx`:
- Around line 120-122: The interpolated "for" fragment is missing the public
namespace; update the i18n call used where descriptionKey is checked (the t(...)
expression that produces "{{description}} for") to use the public namespace
(e.g., change the t invocation for the interpolated string so it uses "public~"
or move the whole interpolation into a single public~ keyed string) so the "for"
fragment is localized consistently; locate the t call alongside descriptionKey
in the labels-modal component and apply the namespace there.
- Line 23: The LabelsModalOverlay is incorrectly coupled to ModalComponentProps
and silently overrides provided cancel/close handlers; remove the import of
ModalComponentProps and stop inheriting it in LabelsModalProps, instead declare
a local prop type that includes only the needed fields (e.g., labels, onSave,
isOpen) and excludes cancel/close so callers cannot pass them; update
LabelsModalOverlay to accept the new LabelsModalProps and keep using its
internal handleClose for closing logic; search for any consumers passing
cancel/close and update them to use the modal's public API (onSave or open
state) if needed.
In `@frontend/public/components/modals/remove-volume-modal.tsx`:
- Line 147: The JSX spreads props after passing controlled handlers, so any
close or cancel in props can override handleClose; change the order to spread
props first in the RemoveVolumeModal element (so {...props} comes before
close={handleClose} cancel={handleClose}) to ensure the controlled handlers
(handleClose / closeOverlay via RemoveVolumeModalProps) always take precedence
and cannot be overridden by incoming props.
---
Outside diff comments:
In `@frontend/public/components/modals/configure-ns-pull-secret-modal.tsx`:
- Around line 78-99: The file upload handler onFileChange currently rejects
files based on file.type which is unreliable; remove the MIME gate that checks
file.type !== 'application/json' and instead rely on JSON.parse inside the
FileReader.onload to validate content (keep calling setInvalidJson(true) on
parse errors and setFileData(input) on success), and update the corresponding
file input to use accept=".json,application/json" so users are guided to select
JSON files without enforcing MIME-based rejection.
---
Nitpick comments:
In `@frontend/public/components/modals/labels-modal.tsx`:
- Around line 117-135: FormGroup currently uses fieldId="tags-input" while
SelectorInput hardcodes id:'tags-input' internally; make the association
explicit by adding a prop (e.g., id or inputId) to SelectorInput and pass the
FormGroup's fieldId into SelectorInput from labels-modal (replace hardcoded
value usage in SelectorInput with the new prop while defaulting to 'tags-input'
for backward compatibility), updating the SelectorInput implementation
(selector-input.jsx) to consume that prop and apply it to its underlying input
so label-to-control wiring remains correct and maintainable.
In `@frontend/public/components/modals/managed-resource-save-modal.tsx`:
- Around line 21-25: The submit handler in submit currently calls
props.onSubmit() and then props.close() unconditionally; make submit async,
await props.onSubmit(), and wrap the await in a try/catch so the modal only
closes on success (call props.close() after a successful await), and handle or
rethrow errors in the catch (e.g., surface an error state or call an onError
callback) to avoid closing the modal if onSubmit throws or rejects.
- Around line 74-89: The Modal is being unmounted by the conditional render
which bypasses PatternFly exit animations; instead keep the Modal mounted and
control visibility with the isOpen state: remove the ternary that returns null
and render <Modal ...> unconditionally, set its isOpen prop to the component's
isOpen state (the useState variable named isOpen), keep onClose wired to
handleClose, and continue passing close={handleClose} into
ManagedResourceSaveModal so focus/cleanup runs when Modal finishes its exit
transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da7a2443-cc05-4147-bae8-f2b3d51f226b
📒 Files selected for processing (11)
frontend/packages/console-shared/src/hooks/useLabelsModal.tsxfrontend/public/components/modals/add-secret-to-workload.tsxfrontend/public/components/modals/alert-routing-modal.tsxfrontend/public/components/modals/configure-cluster-upstream-modal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/modals/configure-update-strategy-modal.tsxfrontend/public/components/modals/expand-pvc-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/managed-resource-save-modal.tsxfrontend/public/components/modals/remove-volume-modal.tsxfrontend/public/style/_common.scss
💤 Files with no reviewable changes (1)
- frontend/public/style/_common.scss
| <Form id="co-add-secret-to-workload" onSubmit={submit}> | ||
| <FormGroup | ||
| label={t('public~Add this secret to workload')} | ||
| isRequired | ||
| fieldId="co-add-secret-to-workload__workload" | ||
| > | ||
| <ConsoleSelect | ||
| items={workloadOptions} | ||
| selectedKey={selectedWorkloadUID} | ||
| title={selectWorkloadPlaceholder} | ||
| onChange={onWorkloadChange} | ||
| autocompleteFilter={autocompleteFilter} | ||
| autocompletePlaceholder={selectWorkloadPlaceholder} | ||
| id="co-add-secret-to-workload__workload" | ||
| data-test="add-secret-to-workload-button" | ||
| /> | ||
| </FormGroup> | ||
| <FormGroup | ||
| label={t('public~Add secret as')} | ||
| isRequired | ||
| role="radiogroup" | ||
| fieldId="co-add-secret-to-workload" | ||
| isStack | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/public/components/modals/add-secret-to-workload.tsx | sed -n '190,320p'Repository: openshift/console
Length of output: 5565
🏁 Script executed:
find frontend/public/components/modals -name "*.tsx" -type f | head -10Repository: openshift/console
Length of output: 756
🏁 Script executed:
rg -A 8 'role="radiogroup"' frontend/public/components/modals/ -t tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
rg -A 8 'role="radiogroup"' frontend/public/components/modals/ --type tsRepository: openshift/console
Length of output: 2962
🏁 Script executed:
grep -n 'id="co-add-secret-to-workload"' frontend/public/components/modals/configure-cluster-upstream-modal.tsx | head -5Repository: openshift/console
Length of output: 43
🏁 Script executed:
grep -n '<Form' frontend/public/components/modals/configure-cluster-upstream-modal.tsx | head -5Repository: openshift/console
Length of output: 293
🏁 Script executed:
sed -n '95,180p' frontend/public/components/modals/configure-cluster-upstream-modal.tsx | cat -nRepository: openshift/console
Length of output: 3899
🏁 Script executed:
rg 'aria-labelledby' frontend/public/components/modals/add-secret-to-workload.tsxRepository: openshift/console
Length of output: 119
🏁 Script executed:
rg -B 2 -A 2 'fieldId=' frontend/public/components/modals/configure-ns-pull-secret-modal.tsx | head -20Repository: openshift/console
Length of output: 315
🏁 Script executed:
rg 'fieldId="co-add-secret-to-workload' frontend/public/components/modals/ --type tsRepository: openshift/console
Length of output: 697
Use a unique fieldId for the radio group.
The FormGroup at line 217 reuses the form's id (co-add-secret-to-workload) instead of using a unique identifier. This breaks the label-to-field association; the "Add secret as" label will reference the form element rather than the radio group. Change fieldId="co-add-secret-to-workload" to fieldId="co-add-secret-to-workload__add-as" to match the namespace pattern used by adjacent form fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/add-secret-to-workload.tsx` around lines
196 - 219, The FormGroup that renders the radio group (label "Add secret as") is
reusing the form's id via fieldId="co-add-secret-to-workload", breaking
label-to-field association; update that FormGroup's fieldId to a unique
identifier such as "co-add-secret-to-workload__add-as" (same file's
add-secret-to-workload modal, FormGroup for the radio group) so the label
correctly targets the radio controls.
| <Button | ||
| type="submit" | ||
| variant="primary" | ||
| onClick={submit} | ||
| isLoading={inProgress} | ||
| isDisabled={inProgress} | ||
| data-test="confirm-action" | ||
| id="confirm-action" | ||
| form="co-add-secret-to-workload" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd frontend/public/components/modals && wc -l add-secret-to-workload.tsxRepository: openshift/console
Length of output: 91
🏁 Script executed:
cd frontend/public/components/modals && sed -n '190,200p' add-secret-to-workload.tsxRepository: openshift/console
Length of output: 552
🏁 Script executed:
cd frontend/public/components/modals && sed -n '275,290p' add-secret-to-workload.tsxRepository: openshift/console
Length of output: 572
🏁 Script executed:
cd frontend/public/components/modals && sed -n '1,50p' add-secret-to-workload.tsx | head -100Repository: openshift/console
Length of output: 2215
🏁 Script executed:
cd frontend/public/components/modals && grep -n "const submit" add-secret-to-workload.tsxRepository: openshift/console
Length of output: 118
🏁 Script executed:
cd frontend/public/components/modals && grep -n "mountPath" add-secret-to-workload.tsx | head -20Repository: openshift/console
Length of output: 246
🏁 Script executed:
cd frontend/public/components/modals && sed -n '100,180p' add-secret-to-workload.tsxRepository: openshift/console
Length of output: 2816
🏁 Script executed:
cd frontend/public/components/modals && sed -n '220,275p' add-secret-to-workload.tsxRepository: openshift/console
Length of output: 2254
🏁 Script executed:
cd frontend/public/components/modals && sed -n '125,145p' alert-routing-modal.tsxRepository: openshift/console
Length of output: 621
Remove the onClick handler to let the form's validation run.
The button's onClick={submit} creates a secondary submission path that bypasses native form validation. Since the button already targets the form via form="co-add-secret-to-workload" and carries type="submit", the form's onSubmit handler fires automatically on click. The current setup allows submission without validating mountPath even though it's marked isRequired in the FormGroup. This diverges from the cleaner pattern in alert-routing-modal.tsx Lines 129–136, which relies entirely on the button's type and form attribute without a direct onClick handler.
♻️ Suggested fix
<Button
type="submit"
variant="primary"
- onClick={submit}
isLoading={inProgress}
isDisabled={inProgress}
data-test="confirm-action"
id="confirm-action"
form="co-add-secret-to-workload"📝 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="primary" | |
| onClick={submit} | |
| isLoading={inProgress} | |
| isDisabled={inProgress} | |
| data-test="confirm-action" | |
| id="confirm-action" | |
| form="co-add-secret-to-workload" | |
| > | |
| <Button | |
| type="submit" | |
| variant="primary" | |
| isLoading={inProgress} | |
| isDisabled={inProgress} | |
| data-test="confirm-action" | |
| id="confirm-action" | |
| form="co-add-secret-to-workload" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/add-secret-to-workload.tsx` around lines
278 - 287, The Button component with id "confirm-action" currently has an
onClick={submit} which creates a secondary submission path that bypasses native
form validation; remove the onClick prop from the Button in
add-secret-to-workload.tsx so the button relies on type="submit" and
form="co-add-secret-to-workload" to trigger the form's onSubmit handler (submit)
and allow the browser/FormGroup (mountPath isRequired) validation to run
normally.
| return isOpen ? ( | ||
| <Modal | ||
| variant={ModalVariant.small} | ||
| isOpen | ||
| onClose={handleClose} | ||
| aria-labelledby="add-secret-to-workload-modal-title" | ||
| > | ||
| <AddSecretToWorkloadModal close={handleClose} cancel={handleClose} {...props} /> | ||
| </Modal> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the AddSecretToWorkloadModal type definitions
fd -t f "add-secret-to-workload" --exec cat {} \;Repository: openshift/console
Length of output: 12095
🏁 Script executed:
# Search for AddSecretToWorkloadModalProps definition
rg "AddSecretToWorkloadModalProps" -t ts -t tsx -A 5 -B 2Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check the type definition for ModalComponentProps
rg "ModalComponentProps" -t ts -t tsx -A 5 -B 2Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Examine the referenced alert-routing-modal.tsx at line 161
fd "alert-routing-modal.tsx" --exec cat {} \;Repository: openshift/console
Length of output: 5910
🏁 Script executed:
# Examine the referenced configure-cluster-upstream-modal.tsx at line 210
fd "configure-cluster-upstream-modal.tsx" --exec cat {} \;Repository: openshift/console
Length of output: 7555
🏁 Script executed:
# Search for ModalComponentProps definition in factory files
find . -name "factory*" -o -name "modal.ts" -o -name "modal.tsx" | head -20Repository: openshift/console
Length of output: 795
🏁 Script executed:
# Search for ModalComponentProps type definition
rg "type ModalComponentProps" -A 5Repository: openshift/console
Length of output: 447
🏁 Script executed:
# Also check for interface ModalComponentProps
rg "interface ModalComponentProps" -A 5Repository: openshift/console
Length of output: 43
Reorder props to ensure modal cleanup executes properly.
AddSecretToWorkloadModalProps extends ModalComponentProps, which defines optional close and cancel properties. Since {...props} spreads after the explicit prop assignments on the current line, any close or cancel from props will overwrite handleClose, breaking the guarantee that both setIsOpen(false) and closeOverlay() execute together. This differs from the patterns in alert-routing-modal.tsx (Line 161) and configure-cluster-upstream-modal.tsx (Line 210), which correctly spread props first.
Suggested fix
- <AddSecretToWorkloadModal close={handleClose} cancel={handleClose} {...props} />
+ <AddSecretToWorkloadModal {...props} close={handleClose} cancel={handleClose} />📝 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.
| return isOpen ? ( | |
| <Modal | |
| variant={ModalVariant.small} | |
| isOpen | |
| onClose={handleClose} | |
| aria-labelledby="add-secret-to-workload-modal-title" | |
| > | |
| <AddSecretToWorkloadModal close={handleClose} cancel={handleClose} {...props} /> | |
| </Modal> | |
| return isOpen ? ( | |
| <Modal | |
| variant={ModalVariant.small} | |
| isOpen | |
| onClose={handleClose} | |
| aria-labelledby="add-secret-to-workload-modal-title" | |
| > | |
| <AddSecretToWorkloadModal {...props} close={handleClose} cancel={handleClose} /> | |
| </Modal> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/add-secret-to-workload.tsx` around lines
307 - 315, The modal currently spreads {...props} after explicitly passing
close={handleClose} and cancel={handleClose}, allowing incoming
props.close/props.cancel to override handleClose and break cleanup; update the
JSX so that {...props} is spread first on the Modal/AddSecretToWorkloadModal and
then explicitly pass close={handleClose} and cancel={handleClose} (referencing
AddSecretToWorkloadModal, Modal, handleClose) so handleClose (which calls
setIsOpen(false) and closeOverlay()) cannot be overwritten by incoming props.
| <FormGroup | ||
| label={t('public~Configuration')} | ||
| role="radiogroup" | ||
| fieldId="co-add-secret-to-workload" | ||
| isStack | ||
| > | ||
| <Radio | ||
| name="config-default" | ||
| id="config-default" | ||
| onChange={() => { | ||
| setCustomSelected(false); | ||
| setInvalidCustomURL(false); | ||
| }} | ||
| label={t('public~Default')} | ||
| isChecked={!customSelected} | ||
| body={ | ||
| !customSelected && ( | ||
| <> | ||
| <TextInput | ||
| id={'cluster-version-default-upstream-server-url'} | ||
| type="url" | ||
| readOnly | ||
| value={CLUSTER_VERSION_DEFAULT_UPSTREAM_SERVER_URL_PLACEHOLDER} | ||
| readOnlyVariant="default" | ||
| /> | ||
| <FormHelperText className="pf-v6-u-mt-sm"> | ||
| <HelperText> | ||
| <HelperTextItem> | ||
| {t('public~Receive update information from Red Hat.')} | ||
| </HelperTextItem> | ||
| </HelperText> | ||
| </FormHelperText> | ||
| </> | ||
| ) | ||
| } | ||
| /> | ||
| <Radio | ||
| name="config-custom" | ||
| id="config-custom" | ||
| onChange={() => { | ||
| setCustomSelected(true); | ||
| setTimeout(() => { | ||
| customURLInputRef.current?.focus(); | ||
| }, 0); | ||
| }} | ||
| label={t('public~Custom update service')} | ||
| isChecked={customSelected} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l frontend/public/components/modals/configure-cluster-upstream-modal.tsxRepository: openshift/console
Length of output: 135
🏁 Script executed:
sed -n '90,150p' frontend/public/components/modals/configure-cluster-upstream-modal.tsxRepository: openshift/console
Length of output: 2308
🏁 Script executed:
# Check if "co-add-secret-to-workload" appears elsewhere in this file or is reused elsewhere
rg "co-add-secret-to-workload" frontend/public/components/modals/configure-cluster-upstream-modal.tsxRepository: openshift/console
Length of output: 108
🏁 Script executed:
# Check if this pattern (different name attributes for grouped radios) appears elsewhere in the file
grep -n "name=" frontend/public/components/modals/configure-cluster-upstream-modal.tsxRepository: openshift/console
Length of output: 139
🏁 Script executed:
# Look at similar FormGroup + Radio patterns in the codebase to understand best practices
rg -A 8 'role="radiogroup"' frontend/public/components/ -t tsx | head -60Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for radio group patterns in TypeScript files
rg -A 8 'role="radiogroup"' frontend/public/components/ -t ts | head -80Repository: openshift/console
Length of output: 7090
🏁 Script executed:
# Also check in JSX files within the components directory
find frontend/public/components -name "*.tsx" -o -name "*.jsx" | xargs grep -l "role=\"radiogroup\"" 2>/dev/null | head -5Repository: openshift/console
Length of output: 387
🏁 Script executed:
# Look at one example of proper radio group usage
fd "\.tsx$" frontend/public/components | head -1 | xargs grep -B 3 -A 15 'role="radiogroup"' | head -40Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Let me check the add-secret-to-workload.tsx to confirm the fieldId is copied from there
sed -n '40,80p' frontend/public/components/modals/add-secret-to-workload.tsx | grep -A 15 "FormGroup"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check the correct pattern: what should the name attributes be for radios in the same group?
sed -n '40,80p' frontend/public/components/modals/configure-ns-pull-secret-modal.tsx | grep -B 2 -A 10 "radiogroup"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check add-secret-to-workload.tsx structure
grep -n "FormGroup\|name=" frontend/public/components/modals/add-secret-to-workload.tsx | head -20Repository: openshift/console
Length of output: 667
🏁 Script executed:
# Check configure-ns-pull-secret-modal.tsx structure
grep -n "FormGroup\|name=" frontend/public/components/modals/configure-ns-pull-secret-modal.tsx | head -20Repository: openshift/console
Length of output: 326
🏁 Script executed:
# Get the full FormGroup and Radio section from configure-cluster-upstream-modal.tsx to understand context
sed -n '96,145p' frontend/public/components/modals/configure-cluster-upstream-modal.tsxRepository: openshift/console
Length of output: 1820
🏁 Script executed:
# Check the form ID to understand naming convention
grep -n "form\|Form" frontend/public/components/modals/configure-cluster-upstream-modal.tsx | head -10Repository: openshift/console
Length of output: 629
🏁 Script executed:
# Verify PatternFly FormGroup and Radio component expectations
web_search "PatternFly radio group name attribute same group best practice"Repository: openshift/console
Length of output: 111
🌐 Web query:
PatternFly Radio component same name attribute radio group
💡 Result:
In PatternFly Radio (React), the name prop is the HTML name attribute used to group radios—so:
- All radios in the same group must share the same
name(so only one can be selected). (patternfly.org) - Different radio groups on the same page must use different
namevalues, or they’ll behave like one big group (selecting in one group will unselect the other). (This is standard HTML radio behavior; PatternFly’snamemaps to that.) (patternfly.org)
Example:
<Radio id="opt-a" name="billingMode" label="Monthly" />
<Radio id="opt-b" name="billingMode" label="Annual" />
<Radio id="ship-a" name="shippingSpeed" label="Standard" />
<Radio id="ship-b" name="shippingSpeed" label="Express" />If you’re labeling the group with PatternFly FormGroup, pass role="radiogroup" for accessibility. (patternfly.org)
Citations:
- 1: https://www.patternfly.org/components/forms/radio/
- 2: https://www.patternfly.org/components/forms/radio/
- 3: https://www.patternfly.org/components/forms/form
Fix the radio-group name attributes and fieldId.
Lines 103 and 133 assign different name values ("config-default" and "config-custom"), which breaks native HTML radio grouping semantics. Both radios must share the same name to enable keyboard navigation and proper screen-reader labeling. Additionally, line 99's fieldId="co-add-secret-to-workload" is copied from an unrelated modal and should match this form's purpose (e.g., "cluster-version-config"). The controlled state keeps them visually exclusive, but the DOM no longer represents a single, properly labeled radio group.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/configure-cluster-upstream-modal.tsx`
around lines 96 - 142, The FormGroup used as the radio group currently has an
unrelated fieldId ("co-add-secret-to-workload") and each Radio uses a different
name ("config-default" vs "config-custom"), breaking native radio grouping;
update the FormGroup.fieldId to a meaningful id like "cluster-version-config"
and make both Radio.name values identical (e.g., "cluster-version-config") so
the radios form a proper native group while keeping the existing
isChecked/onChange handlers in the Radio components.
| {fileData && ( | ||
| <GridItem span={9} smOffset={3}> | ||
| <CodeBlock> | ||
| <CodeBlockCode>{fileData}</CodeBlockCode> | ||
| </CodeBlock> |
There was a problem hiding this comment.
Don’t echo the uploaded pull-secret contents back into the DOM.
This file can contain reusable auth data or tokens. Rendering the raw JSON in a CodeBlock unnecessarily exposes secret material in the UI and accessibility tree; a filename, validation badge, or generic “file loaded” message is safer.
Safer pattern
- {fileData && (
- <GridItem span={9} smOffset={3}>
- <CodeBlock>
- <CodeBlockCode>{fileData}</CodeBlockCode>
- </CodeBlock>
- </GridItem>
- )}
+ {fileData && (
+ <GridItem span={9} smOffset={3}>
+ <Alert
+ isInline
+ className="co-alert"
+ variant="success"
+ title={t('public~File loaded')}
+ >
+ {t('public~The Docker config file was validated and is ready to save.')}
+ </Alert>
+ </GridItem>
+ )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/configure-ns-pull-secret-modal.tsx` around
lines 300 - 304, The modal currently renders uploaded pull-secret contents into
the DOM via fileData inside CodeBlock/CodeBlockCode (in
ConfigureNsPullSecretModal), which exposes sensitive tokens; instead stop
rendering raw fileData and replace it with a non-sensitive UI: show the uploaded
filename or a generic "file loaded" message and a validation/status badge, keep
any validation logic that inspects fileData but do not inject the raw JSON into
JSX or aria text, and ensure any existing references to fileData for display are
removed or replaced with a safeDisplayName variable used by the UI components.
| <ModalFooterWithAlerts errorMessage={errorMessage}> | ||
| <Button | ||
| type="submit" | ||
| variant="primary" | ||
| isLoading={inProgress} | ||
| isDisabled={inProgress} | ||
| data-test="confirm-action" | ||
| form="configure-update-strategy-form" | ||
| > | ||
| {t('public~Save')} | ||
| </Button> | ||
| <Button variant="link" onClick={cancel} data-test-id="modal-cancel-action"> | ||
| {t('public~Cancel')} | ||
| </Button> | ||
| </ModalFooterWithAlerts> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the overlay close contract.
fd 'OverlayProvider\.(ts|tsx)$' frontend -t f -x sed -n '1,240p' {}
# Inspect the legacy modal footer behavior for in-progress submits.
sed -n '1,320p' frontend/public/components/factory/modal.tsx
rg -n -C3 'ModalSubmitFooter|inProgress|cancel' frontend/public/components/factory/modal.tsx
# Re-open the current flow with surrounding context.
rg -n -C4 'handlePromise|handleClose|closeOverlay|inProgress' \
frontend/public/components/modals/configure-update-strategy-modal.tsxRepository: openshift/console
Length of output: 14713
Disable every dismiss path while the patch is pending.
Only Save is gated by inProgress (lines 225–226). Cancel and the modal's onClose handler (lines 232, 253) remain active during the patch, allowing users to dismiss while k8sPatch runs in the background. This sacrifices error feedback and completion visibility. While closeOverlay is idempotent and double-calling it after the overlay is already gone is safe, the poor UX should be corrected: block Cancel/onClose while the request is in flight, or guard the close path with an inProgress check.
The legacy footer (ModalSubmitFooter) had the same issue—cancel was never gated on inProgress—so this is not a regression, but a pattern that should be improved here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx` around
lines 221 - 235, Disable all dismiss paths while the patch is pending: add
inProgress guards to the Cancel button and the modal close handler so users
cannot dismiss while k8sPatch is running. Specifically, set the Cancel <Button>
(the one using cancel and data-test-id="modal-cancel-action") to
isDisabled={inProgress} and modify the modal's onClose/closeOverlay handler to
early-return when inProgress is true (or wrap cancel to check inProgress before
calling closeOverlay). Ensure ModalFooterWithAlerts/Modal onClose behavior and
the cancel function both respect the inProgress flag so neither Cancel nor
clicking outside/close icon will close the modal while the request is in flight.
| <RequestSizeInput | ||
| name={t('public~requestSize')} | ||
| required | ||
| onChange={handleRequestSizeInputChange} | ||
| defaultRequestSizeUnit={requestSizeUnit} | ||
| defaultRequestSizeValue={requestSizeValue} | ||
| dropdownUnits={dropdownUnits} | ||
| testID="pvc-expand-size-input" | ||
| minValue={defaultSize[0]} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how RequestSizeInput uses the `name` prop to determine if translation is appropriate.
# Find the RequestSizeInput component definition
ast-grep --pattern $'const RequestSizeInput = ($$$) => {
$$$
}'
# Also search for class-based or FC definition
rg -n -A 20 'RequestSizeInput.*=' frontend/public/components/utils/request-size-inputRepository: openshift/console
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Find where RequestSizeInput is defined
rg -n "RequestSizeInput" frontend/public/components --type tsx --type ts -l
# Also search for where it's imported/exported
rg -n "export.*RequestSizeInput" frontend/public --type tsx --type tsRepository: openshift/console
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for RequestSizeInput definition without file type constraint
rg -n "RequestSizeInput" frontend/public/components --max-count=10
# Search for where it's exported
rg -n "export.*RequestSizeInput"Repository: openshift/console
Length of output: 1325
🏁 Script executed:
#!/bin/bash
# Read the RequestSizeInput component definition
cat -n frontend/public/components/utils/request-size-input.tsxRepository: openshift/console
Length of output: 3863
🏁 Script executed:
#!/bin/bash
# Check how RequestSizeInput is called in other places to see the pattern
rg -B 2 -A 8 "<RequestSizeInput" frontend/public/components/storage/create-pvc.tsxRepository: openshift/console
Length of output: 433
🏁 Script executed:
#!/bin/bash
# Verify the exact code in expand-pvc-modal.tsx around line 80
sed -n '75,92p' frontend/public/components/modals/expand-pvc-modal.tsxRepository: openshift/console
Length of output: 744
Use a static technical identifier for the name prop instead of a translated string.
Line 81 passes name={t('public~requestSize')}, but RequestSizeInput constructs HTML form field names from this prop (e.g., requestSizeValue, requestSizeUnit). Using a translated string means form field names vary by locale, breaking form serialization consistency and violating form best practices. Use a static identifier like create-pvc.tsx does: name="requestSize".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/expand-pvc-modal.tsx` around lines 80 - 88,
The RequestSizeInput is receiving a translated string for its name prop
(name={t('public~requestSize')}), which causes generated form field names (e.g.,
requestSizeValue, requestSizeUnit) to vary by locale; change the prop to a
static identifier (name="requestSize") in the ExpandPVCModal where
RequestSizeInput is rendered so the form field names remain consistent across
locales and match how create-pvc.tsx uses it.
| ModalTitle, | ||
| ModalWrapper, | ||
| } from '../factory/modal'; | ||
| import { ModalComponentProps } from '../factory/modal'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ModalComponentProps definition =="
fd 'modal\.tsx?$' frontend/public/components/factory | while read -r file; do
echo "== $file =="
rg -n -C3 'export (type|interface) ModalComponentProps' "$file"
done
echo
echo "== Labels modal usages =="
rg -n -C2 '\bLabelsModalOverlay\b|\bLabelsModalProps\b' frontend -g '!**/*.test.*' -g '!**/*.spec.*'
echo
echo "== useLabelsModal hook =="
fd 'useLabelsModal\.tsx?$' frontend | while read -r file; do
echo "== $file =="
sed -n '1,220p' "$file"
doneRepository: openshift/console
Length of output: 4210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full labels-modal.tsx file with line numbers
echo "== Full labels-modal.tsx content =="
wc -l frontend/public/components/modals/labels-modal.tsx
echo
echo "== Lines 150-210 (context around line 182 and 164) =="
sed -n '150,210p' frontend/public/components/modals/labels-modal.tsx | cat -n --vRepository: openshift/console
Length of output: 536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full labels-modal.tsx file with line numbers
echo "== Full labels-modal.tsx (all 197 lines) =="
sed -n '1,197p' frontend/public/components/modals/labels-modal.tsx | nl -baRepository: openshift/console
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full labels-modal.tsx file with line numbers using cat
echo "== Full labels-modal.tsx (all 197 lines) =="
cat -n frontend/public/components/modals/labels-modal.tsxRepository: openshift/console
Length of output: 7878
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All references to LabelsModal(Overlay|Props) in codebase =="
rg -n 'LabelsModal(Overlay|Props)' frontend --type ts --type tsx
echo
echo "== Check OverlayComponent type definition =="
rg -n -C5 'type OverlayComponent' frontend --type ts --type tsx | head -50
echo
echo "== Check if any code passes cancel or close to any modal overlay =="
rg -n 'cancel\s*=' frontend/public/components/modals --type ts --type tsx | head -20Repository: openshift/console
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All references to LabelsModal(Overlay|Props) in codebase =="
rg -n 'LabelsModal(Overlay|Props)' frontend
echo
echo "== Check OverlayComponent type definition =="
rg -n -C5 'type OverlayComponent' frontend | head -50
echo
echo "== Check if any code passes cancel or close to LabelsModalOverlay =="
rg -n -B3 -A3 'LabelsModalOverlay' frontend | grep -E 'cancel|close'Repository: openshift/console
Length of output: 2989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== useCommonActions context around LabelsModalOverlay call (line 104) =="
sed -n '95,115p' frontend/packages/console-app/src/actions/hooks/useCommonActions.ts | cat -n
echo
echo "== Full useCommonActions file to understand launchModal usage =="
wc -l frontend/packages/console-app/src/actions/hooks/useCommonActions.ts
head -120 frontend/packages/console-app/src/actions/hooks/useCommonActions.ts | tail -40 | cat -nRepository: openshift/console
Length of output: 2912
Decouple LabelsModalOverlay from ModalComponentProps contract.
The overlay owns the cancel and close callbacks (line 182 explicitly overrides them with handleClose), yet the public LabelsModalProps type inherits optional cancel/close from ModalComponentProps. This creates a silent contract violation: the type suggests callers can provide these, but the overlay always discards them. The unnecessary import of ModalComponentProps on line 23 couples the modal to the factory layer without benefit.
🧹 One way to decouple the overlay from `../factory/modal`
-import { ModalComponentProps } from '../factory/modal';
...
-type BaseLabelsModalProps = {
+type LabelsModalOwnProps = {
descriptionKey?: string;
isPodSelector?: boolean;
kind: K8sModel;
labelClassName?: string;
messageKey?: string;
messageVariables?: { [key: string]: string };
- path: string;
resource: K8sResourceKind;
-} & ModalComponentProps;
-export type LabelsModalProps = Omit<BaseLabelsModalProps, 'path'>;
+};
+
+type LabelsModalInjectedProps = {
+ cancel: () => void;
+ close: () => void;
+};
+
+type BaseLabelsModalProps = LabelsModalOwnProps &
+ LabelsModalInjectedProps & {
+ path: string;
+ };
+
+export type LabelsModalProps = LabelsModalOwnProps;
+type LabelsModalInternalProps = LabelsModalProps & LabelsModalInjectedProps;
...
-const LabelsModal: FC<LabelsModalProps> = (props) => (
+const LabelsModal: FC<LabelsModalInternalProps> = (props) => (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/labels-modal.tsx` at line 23, The
LabelsModalOverlay is incorrectly coupled to ModalComponentProps and silently
overrides provided cancel/close handlers; remove the import of
ModalComponentProps and stop inheriting it in LabelsModalProps, instead declare
a local prop type that includes only the needed fields (e.g., labels, onSave,
isOpen) and excludes cancel/close so callers cannot pass them; update
LabelsModalOverlay to accept the new LabelsModalProps and keep using its
internal handleClose for closing logic; search for any consumers passing
cancel/close and update them to use the modal's public API (onSave or open
state) if needed.
| {descriptionKey | ||
| ? t('{{description}} for', { description: t(descriptionKey) }) | ||
| : t('public~Labels for')}{' '} |
There was a problem hiding this comment.
Keep this interpolated label in the public namespace.
Line 121 is the only user-facing string here without public~, so the "for" fragment will stay English in localized builds.
💬 Suggested fix
- ? t('{{description}} for', { description: t(descriptionKey) })
+ ? t('public~{{description}} for', { description: t(descriptionKey) })📝 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.
| {descriptionKey | |
| ? t('{{description}} for', { description: t(descriptionKey) }) | |
| : t('public~Labels for')}{' '} | |
| {descriptionKey | |
| ? t('public~{{description}} for', { description: t(descriptionKey) }) | |
| : t('public~Labels for')}{' '} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/labels-modal.tsx` around lines 120 - 122,
The interpolated "for" fragment is missing the public namespace; update the i18n
call used where descriptionKey is checked (the t(...) expression that produces
"{{description}} for") to use the public namespace (e.g., change the t
invocation for the interpolated string so it uses "public~" or move the whole
interpolation into a single public~ keyed string) so the "for" fragment is
localized consistently; locate the t call alongside descriptionKey in the
labels-modal component and apply the namespace there.
| onClose={handleClose} | ||
| aria-labelledby="remove-volume-modal-title" | ||
| > | ||
| <RemoveVolumeModal close={handleClose} cancel={handleClose} {...props} /> |
There was a problem hiding this comment.
Props spread order may override controlled close handlers.
The close and cancel props are set before {...props} is spread, meaning if props contains close or cancel (which RemoveVolumeModalProps allows as optional), they would override handleClose. This could bypass closeOverlay() and leave the overlay in an inconsistent state.
Spread props first so the controlled handlers always take precedence.
🛠️ Proposed fix
- <RemoveVolumeModal close={handleClose} cancel={handleClose} {...props} />
+ <RemoveVolumeModal {...props} close={handleClose} cancel={handleClose} />📝 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.
| <RemoveVolumeModal close={handleClose} cancel={handleClose} {...props} /> | |
| <RemoveVolumeModal {...props} close={handleClose} cancel={handleClose} /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/components/modals/remove-volume-modal.tsx` at line 147, The
JSX spreads props after passing controlled handlers, so any close or cancel in
props can override handleClose; change the order to spread props first in the
RemoveVolumeModal element (so {...props} comes before close={handleClose}
cancel={handleClose}) to ensure the controlled handlers (handleClose /
closeOverlay via RemoveVolumeModalProps) always take precedence and cannot be
overridden by incoming props.
|
/retest |
1 similar comment
|
/retest |
|
/retest |
|
/unassign @sg00dwin |
rhamilto
changed the title
|
@rhamilto: The following test 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 |
5 participants
Summary
This PR migrates 9 core modal components from deprecated factory modal components to PatternFly v6 Modal components.
Note: This PR includes changes from #16015, so it should merge first.
Changes
Modal Files Migrated:
Hook Updated:
Pattern Applied:
ModalTitle,ModalSubmitFooter,ModalWrapper(deprecated factory components)Modal,ModalHeader,ModalBody,ModalVariantModalFooterWithAlertsfrom @console/shared<form>with React Fragment<>Formcomponent with id for form submissionformattribute<p>tags with<Content component={ContentVariants.p}>titleIconVariantfor warning/danger icons instead of custom icon componentsuseStateforisOpenmanagementTesting
No existing tests were found for these modals. Manual testing instructions are available for each modal.
Statistics
🤖 Generated with Claude Code
Summary by CodeRabbit
Style
Refactor