[WIP] CONSOLE-4447: Migrate @console/shared modals to PatternFly v6 Modal components#16079
[WIP] CONSOLE-4447: Migrate @console/shared modals to PatternFly v6 Modal components#16079rhamilto wants to merge 9 commits intoopenshift:mainfrom
Conversation
Update Cancel button variants from "secondary" to "link" in modern PatternFly modals to follow PatternFly standards. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… wrapper - Migrate useCopyCodeModal, FavoriteButton, and TourStepComponent from deprecated Modal wrapper to modern PatternFly Modal components - Remove deprecated Modal wrapper (packages/console-shared/src/components/modal/) - Migrate CatalogDetailsModal and operator-hub-items from deprecated PatternFly Modal to modern Modal components - Preserve ocs-modal CSS class for catalog modal positioning - Fix FavoriteButton form submission bug by adding preventDefault - Fix Guided Tour accessibility warning by closing Help dropdown and blurring focus before starting tour Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Migrate DeleteModal from deprecated factory/modal components to modern PatternFly v6 Modal components - Create reusable ModalFooterWithAlerts component for alert display - Update configure-count-modal and configure-machine-autoscaler-modal to use modern Modal components and ModalFooterWithAlerts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…r to useOverlay Assisted by: Claude Code
Replace history object usage with useNavigate hook in console-shared package. Changes: - ActionMenuItem.tsx: Replace history.push with navigate - CatalogTile.tsx: Replace history.push with navigate - CatalogView.tsx: Replace history.push with navigate - catalog-utils.tsx: Accept NavigateFunction as parameter - dynamic-form/index.tsx: Replace history.goBack with navigate(-1) - error-boundary.tsx: Use componentDidUpdate instead of key prop for location-based reset - DeleteResourceModal.tsx: Replace history.push with navigate - MultiTabListPage.tsx: Replace history.push with navigate Migration patterns: - Components: Added useNavigate() hook - Utilities: Parameter injection for NavigateFunction - ErrorBoundary: Pass locationPathname as prop and use componentDidUpdate to reset error state only when there's an active error AND location changes. This avoids unnecessary component remounting during tab navigation. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…odal - Add LazyConsolePluginModalOverlay export with lazy loading - Update clusterserviceversion.tsx to use lazy-loaded modal - Migrate DeleteHPAModal from deprecated factory/modal to modern PatternFly Modal - Use ModalHeader, ModalBody, and ModalFooterWithAlerts components - Replace manual state management with usePromiseHandler hook Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Rename launchOverlay to launchModal in useHPAActions and ConsolePlugins - Rename DeleteHPAModalProvider to DeleteHPAModalOverlay - Rename DeleteHPAModalProviderProps to DeleteHPAModalOverlayProps - Export DeleteHPAModalOverlay with named export instead of default Assisted-by: Claude
|
@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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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/dev-console
Migrates DeleteHPAModal, ConsolePluginModal, and DeleteResourceModal from deprecated factory modal components to modern PatternFly v6 Modal. Changes: - Replace ModalWrapper with Modal component with state management - Replace ModalTitle with ModalHeader - Replace ModalSubmitFooter with ModalFooterWithAlerts - Replace HTML form with PatternFly Form component - Use modern Button components with consistent loading/disabled states - Maintain Formik integration in DeleteResourceModal Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-4447-shared
branch
from
f3a9470 to
0294782
Compare
rhamilto
changed the title
openshift-ci
bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThis pull request modernizes modal and navigation patterns across the console. Custom Modal components are replaced with PatternFly Modal subcomponents (ModalHeader, ModalBody, ModalFooterWithAlerts). Modal invocation shifts from direct function calls to overlay-based patterns using OverlayComponent and lazy-loaded variants. Navigation transitions from history utilities to the useNavigate hook. Action factory functions convert to hook-based implementations with memoization. Button styling updates change Cancel actions from secondary to link variants. A new ModalFooterWithAlerts component centralizes error message and footer action rendering. Custom modal wrapper files are removed in favor of PatternFly dependencies. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-4447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![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 (1)
frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx (1)
106-110:⚠️ Potential issue | 🟡 MinorSet starred state explicitly after save instead of toggling.
At Line 109, toggling (
prev => !prev) can produce the wrong state in re-entrant flows. This path is an “add favorite” success path, so set it totruedeterministically.Proposed fix
- setIsStarred((prev) => !prev); + setIsStarred(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx` around lines 106 - 110, The add-favorite success flow currently toggles the starred state using setIsStarred(prev => !prev), which can produce incorrect state in re-entrant scenarios; update the handler so after creating newFavorite and calling setFavorites(updatedFavorites) you call setIsStarred(true) explicitly (instead of toggling) and leave setError('') as is to deterministically mark the item starred; reference setFavorites, newFavorite, updatedFavorites and setIsStarred in the change.
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/components/error/error-boundary.tsx (1)
37-40: Add explicit type annotation for constructor parameter.The
propsparameter should be explicitly typed for better type safety and IDE support.🔧 Proposed fix
- constructor(props) { + constructor(props: ErrorBoundaryInnerProps) { super(props); this.state = this.defaultState; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/error/error-boundary.tsx` around lines 37 - 40, The constructor's parameter is untyped; add an explicit props type annotation to the constructor signature (e.g., use the component's Props interface or React.ComponentProps type) so TypeScript can validate props for the ErrorBoundary class; update the constructor declaration that currently reads "constructor(props)" to include the proper type (matching the component's Props interface), keep the super(props) call and initialization of this.state = this.defaultState unchanged.frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsx (1)
67-75: Prefer PatternFly Button withcomponent={Link}over manually-styled Link.Using raw CSS classes (
pf-v6-c-button pf-m-primary) on aLinkelement is fragile and bypasses PatternFly's built-in focus management, loading states, and future-proof styling. Therole="button"attribute partially compensates but doesn't provide full parity with an actual Button.Consider using PatternFly's
Buttoncomponent with thecomponentprop for proper a11y semantics and consistent styling:♻️ Suggested refactor using Button component
+import { Button } from '@patternfly/react-core'; ... - <Link - data-test="catalog-details-modal-cta" - className="pf-v6-c-button pf-m-primary co-catalog-page__overlay-action" - to={to} - role="button" - onClick={onClose} - > - {label} - </Link> + <Button + data-test="catalog-details-modal-cta" + className="co-catalog-page__overlay-action" + component={(props) => <Link {...props} to={to} />} + onClick={onClose} + > + {label} + </Button>Note:
Buttonis already available from the existing@patternfly/react-coreimport—just add it to the destructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsx` around lines 67 - 75, Replace the raw <Link> styled with PF classes in CatalogDetailsModal with PatternFly's Button component configured to render as a Link: import Button from `@patternfly/react-core`, remove the manual pf-v6-c-button pf-m-primary classes and role="button", and render <Button component={Link} to={to} onClick={onClose} data-test="catalog-details-modal-cta" variant="primary">{label}</Button> so you keep the same props/behavior (to, onClose, data-test, label) but gain PF focus/loading/a11y handling.frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx (1)
161-188: Use a single submit path for Save to avoid duplicate side effects.Line 161 already handles submission via
onSubmit, but Line 187 also calls the same handler fromonClick. Consolidate to form submit only to avoid accidental double execution ofsetFavorites/telemetry.Proposed fix
- <Button - key="confirm" - variant="primary" - onClick={handleConfirmStar} - form="confirm-favorite-form" - > + <Button key="confirm" variant="primary" type="submit" form="confirm-favorite-form"> {t('Save')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx` around lines 161 - 188, The form currently calls handleConfirmStar via Form onSubmit ("confirm-favorite-form") while the primary Button also invokes handleConfirmStar in its onClick, causing duplicate side effects; remove the Button's onClick and make the Button submit the form (set type="submit" or rely on form prop) so the single submit path uses the Form's onSubmit handler (handleConfirmStar) and the TextInput named "name" validation flows through the form.frontend/packages/helm-plugin/src/actions/types.ts (1)
13-13: This type appears to be unused and can likely be removed or its necessity should be justified.The
HelmActionsScopetype has no references anywhere in the codebase, no imports, and no usages of theredirectfield. The recent commit migrating to PatternFly v6 Modals may have made this type obsolete. Rather than hardening the type, consider:
- Removing it if it's no longer part of the public API surface
- If it must remain as a public export, document its purpose and consumers
- The concern about boolean redirect values is not substantiated—no such assignments exist in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/actions/types.ts` at line 13, HelmActionsScope (and its optional redirect field) appears unused; either remove the exported type from frontend/packages/helm-plugin/src/actions/types.ts or document and justify it if it must remain public. To fix: search for the symbol HelmActionsScope and any references to redirect, and if none exist remove the type export and run a build/tests; if it must stay, add a clear JSDoc above HelmActionsScope describing its purpose, consumers, and why redirect exists (including expected value types), and keep the export. Ensure any public API docs are updated when retaining the type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`:
- Around line 162-165: The FormGroup label and TextInput id are mismatched:
FormGroup uses fieldId="input-name" while the TextInput sets
id="confirm-favorite-form-name"; update one so they match (e.g., set TextInput
id to "input-name" or change FormGroup fieldId to "confirm-favorite-form-name")
so the label is properly associated with the input; ensure the change is applied
in the FavoriteButton component where FormGroup and TextInput are defined and
keep any data-test attributes intact.
In
`@frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsx`:
- Line 59: ModalHeader wrapping CatalogItemHeader may produce spacing/alignment
issues; update CatalogDetailsModal.scss to add specific rules targeting
ModalHeader and CatalogItemHeader (e.g., normalize ModalHeader padding/margin,
align .pf-c-modal__header content, and override CatalogItemHeader margins) or,
if preferred, visually verify and document that the current composition renders
correctly across breakpoints; make the change by editing styles referenced from
CatalogDetailsModal (look for ModalHeader and CatalogItemHeader in that
component) and include concise comments in the SCSS indicating why the overrides
are needed.
In `@frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsx`:
- Around line 65-72: The Remove button currently disables when errorMessage is
set, preventing retries after a transient failure; update the Button in
DeleteHPAModal (the Button with props onClick={handleSubmit},
form="delete-hpa-modal-form", isLoading={inProgress}, isDisabled={inProgress ||
!!errorMessage}) to only disable based on inProgress (i.e., remove the "||
!!errorMessage" part) so users can retry while the modal is open; keep
errorMessage shown via ModalFooterWithAlerts and preserve
isLoading={inProgress}.
In
`@frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx`:
- Around line 63-70: The confirm button is being disabled when
status.submitError exists which prevents retries; in DeleteResourceModal change
the isDisabled logic on the Button (rendered inside ModalFooterWithAlerts) to
not include status.submitError so the button remains enabled for retries (i.e.,
only disable when isSubmitting or !isValid), and also clear or reset
status.submitError at the start of the onSubmit flow in the onSubmit handler so
the previous error doesn't block UI state; reference the Button props,
status.submitError, isSubmitting, isValid, and the onSubmit function to
implement these two changes.
In
`@frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx`:
- Line 4: The file uses React.FC and React.ReactNode types without importing
them; add explicit type-only imports and update the component signature to use
those imports: import type { FC, ReactNode } from 'react' and replace
React.FC<ModalFooterWithAlertsProps> (and any React.ReactNode usages referenced
by ModalFooterWithAlertsProps) with FC<ModalFooterWithAlertsProps> and ReactNode
respectively so TypeScript compiles under jsx: "react-jsx"; ensure
ModalFooterWithAlerts and ModalFooterWithAlertsProps references are updated to
use the imported types.
In `@frontend/packages/dev-console/src/actions/context-menu.ts`:
- Around line 13-23: The function useDeleteApplicationAction currently declares
a return type of Action but returns null and also assumes resourceModel is
resolved before use; update the signature to return Action | null (or always
return a valid Action) and add a runtime guard that early-returns null if
resourceModel is falsy before any use (e.g., before calling asAccessReview or
using resourceModel in the memoized callback). Ensure callers that pass
kindObj/from useK8sModel wait for inFlight or handle the potential null return
from useDeleteApplicationAction so asAccessReview is never invoked with an
undefined model.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx`:
- Around line 1066-1077: The Link with data-test-id "operator-install-btn" is
only visually disabled via the 'pf-m-disabled' class but still
keyboard-focusable and activatable; update the component (the Link rendering
using installLink and checking detailsItem.obj / detailsItem.isInstalling) to
either render a proper disabled Button when disabled, or augment the Link with
proper semantics: add aria-disabled={true} when !detailsItem.obj ||
detailsItem.isInstalling, set tabIndex={-1} when disabled, and attach an onClick
handler that calls event.preventDefault() / stops navigation when disabled;
ensure the logic uses the same condition used for the 'pf-m-disabled' class so
the visual, keyboard, and screen-reader states stay consistent.
---
Outside diff comments:
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`:
- Around line 106-110: The add-favorite success flow currently toggles the
starred state using setIsStarred(prev => !prev), which can produce incorrect
state in re-entrant scenarios; update the handler so after creating newFavorite
and calling setFavorites(updatedFavorites) you call setIsStarred(true)
explicitly (instead of toggling) and leave setError('') as is to
deterministically mark the item starred; reference setFavorites, newFavorite,
updatedFavorites and setIsStarred in the change.
---
Nitpick comments:
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`:
- Around line 161-188: The form currently calls handleConfirmStar via Form
onSubmit ("confirm-favorite-form") while the primary Button also invokes
handleConfirmStar in its onClick, causing duplicate side effects; remove the
Button's onClick and make the Button submit the form (set type="submit" or rely
on form prop) so the single submit path uses the Form's onSubmit handler
(handleConfirmStar) and the TextInput named "name" validation flows through the
form.
In
`@frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsx`:
- Around line 67-75: Replace the raw <Link> styled with PF classes in
CatalogDetailsModal with PatternFly's Button component configured to render as a
Link: import Button from `@patternfly/react-core`, remove the manual
pf-v6-c-button pf-m-primary classes and role="button", and render <Button
component={Link} to={to} onClick={onClose} data-test="catalog-details-modal-cta"
variant="primary">{label}</Button> so you keep the same props/behavior (to,
onClose, data-test, label) but gain PF focus/loading/a11y handling.
In `@frontend/packages/console-shared/src/components/error/error-boundary.tsx`:
- Around line 37-40: The constructor's parameter is untyped; add an explicit
props type annotation to the constructor signature (e.g., use the component's
Props interface or React.ComponentProps type) so TypeScript can validate props
for the ErrorBoundary class; update the constructor declaration that currently
reads "constructor(props)" to include the proper type (matching the component's
Props interface), keep the super(props) call and initialization of this.state =
this.defaultState unchanged.
In `@frontend/packages/helm-plugin/src/actions/types.ts`:
- Line 13: HelmActionsScope (and its optional redirect field) appears unused;
either remove the exported type from
frontend/packages/helm-plugin/src/actions/types.ts or document and justify it if
it must remain public. To fix: search for the symbol HelmActionsScope and any
references to redirect, and if none exist remove the type export and run a
build/tests; if it must stay, add a clear JSDoc above HelmActionsScope
describing its purpose, consumers, and why redirect exists (including expected
value types), and keep the export. Ensure any public API docs are updated when
retaining the type.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (47)
frontend/packages/console-app/src/actions/creators/hpa-factory.tsfrontend/packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsxfrontend/packages/console-app/src/components/favorite/FavoriteButton.tsxfrontend/packages/console-app/src/components/modals/add-group-users-modal.tsxfrontend/packages/console-app/src/components/tour/TourStepComponent.tsxfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsxfrontend/packages/console-shared/src/components/catalog/CatalogTile.tsxfrontend/packages/console-shared/src/components/catalog/catalog-view/CatalogView.tsxfrontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.scssfrontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsxfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsxfrontend/packages/console-shared/src/components/dynamic-form/index.tsxfrontend/packages/console-shared/src/components/error/error-boundary.tsxfrontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsxfrontend/packages/console-shared/src/components/hpa/index.tsfrontend/packages/console-shared/src/components/index.tsfrontend/packages/console-shared/src/components/modal/Modal.tsxfrontend/packages/console-shared/src/components/modal/index.tsfrontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsxfrontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsxfrontend/packages/console-shared/src/components/modals/CreateProjectModal.tsxfrontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsxfrontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsxfrontend/packages/console-shared/src/components/modals/index.tsfrontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsxfrontend/packages/console-shared/src/hooks/useCopyCodeModal.tsxfrontend/packages/dev-console/src/actions/context-menu.tsfrontend/packages/dev-console/src/actions/providers.tsxfrontend/packages/helm-plugin/src/actions/creators.tsfrontend/packages/helm-plugin/src/actions/providers.tsfrontend/packages/helm-plugin/src/actions/types.tsfrontend/packages/knative-plugin/src/components/test-function/TestFunctionModal.tsxfrontend/packages/metal3-plugin/src/components/modals/PowerOffHostModal.tsxfrontend/packages/metal3-plugin/src/components/modals/RestartHostModal.tsxfrontend/packages/metal3-plugin/src/components/modals/StartNodeMaintenanceModal.tsxfrontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsxfrontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsxfrontend/packages/operator-lifecycle-manager/src/components/registry-poll-interval-details.tsxfrontend/public/components/masthead/masthead-toolbar.tsxfrontend/public/components/modals/configure-count-modal.tsxfrontend/public/components/modals/configure-machine-autoscaler-modal.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/delete-namespace-modal.tsxfrontend/public/components/secrets/create-secret/SecretFormWrapper.tsxfrontend/public/components/utils/button-bar.tsxfrontend/public/locales/en/public.json
💤 Files with no reviewable changes (3)
- frontend/packages/console-shared/src/components/index.ts
- frontend/packages/console-shared/src/components/modal/index.ts
- frontend/packages/console-shared/src/components/modal/Modal.tsx
| <FormGroup label={t('Name')} isRequired fieldId="input-name"> | ||
| <TextInput | ||
| id="confirm-favorite-form-name" | ||
| data-test="input-name" |
There was a problem hiding this comment.
Fix label/input association for accessibility.
Line 162 uses fieldId="input-name" while Line 164 sets id="confirm-favorite-form-name". These should match so the label is correctly bound to the input.
Proposed fix
- <FormGroup label={t('Name')} isRequired fieldId="input-name">
+ <FormGroup label={t('Name')} isRequired fieldId="confirm-favorite-form-name">📝 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.
| <FormGroup label={t('Name')} isRequired fieldId="input-name"> | |
| <TextInput | |
| id="confirm-favorite-form-name" | |
| data-test="input-name" | |
| <FormGroup label={t('Name')} isRequired fieldId="confirm-favorite-form-name"> | |
| <TextInput | |
| id="confirm-favorite-form-name" | |
| data-test="input-name" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/console-app/src/components/favorite/FavoriteButton.tsx`
around lines 162 - 165, The FormGroup label and TextInput id are mismatched:
FormGroup uses fieldId="input-name" while the TextInput sets
id="confirm-favorite-form-name"; update one so they match (e.g., set TextInput
id to "input-name" or change FormGroup fieldId to "confirm-favorite-form-name")
so the label is properly associated with the input; ensure the change is applied
in the FavoriteButton component where FormGroup and TextInput are defined and
keep any data-test attributes intact.
| <CatalogDetailsPanel item={item} /> | ||
| </StackItem> | ||
| </Stack> | ||
| <ModalHeader>{modalHeader}</ModalHeader> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the CatalogDetailsModal files
find . -name "CatalogDetailsModal.*" -type fRepository: openshift/console
Length of output: 241
🏁 Script executed:
# Examine the TypeScript file, focusing on line 59 and surrounding context
wc -l ./frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsxRepository: openshift/console
Length of output: 153
🏁 Script executed:
# Read the entire TypeScript file
cat -n ./frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsxRepository: openshift/console
Length of output: 3487
🏁 Script executed:
# Read the SCSS file to check for ModalHeader styling overrides
cat -n ./frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.scssRepository: openshift/console
Length of output: 129
Add custom CSS to handle ModalHeader and CatalogItemHeader composition or confirm visual output.
The SCSS file contains no custom styling for ModalHeader or CatalogItemHeader. While wrapping CatalogItemHeader inside ModalHeader is a valid composition pattern in PatternFly v6, without explicit CSS overrides, ModalHeader's default padding and internal structure may cause spacing or alignment issues with CatalogItemHeader. Either add custom CSS rules (e.g., in CatalogDetailsModal.scss) to normalize the header spacing, or explicitly verify and document that the visual output renders correctly—particularly checking header spacing, padding, and alignment within the modal overlay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-shared/src/components/catalog/details/CatalogDetailsModal.tsx`
at line 59, ModalHeader wrapping CatalogItemHeader may produce spacing/alignment
issues; update CatalogDetailsModal.scss to add specific rules targeting
ModalHeader and CatalogItemHeader (e.g., normalize ModalHeader padding/margin,
align .pf-c-modal__header content, and override CatalogItemHeader margins) or,
if preferred, visually verify and document that the current composition renders
correctly across breakpoints; make the change by editing styles referenced from
CatalogDetailsModal (look for ModalHeader and CatalogItemHeader in that
component) and include concise comments in the SCSS indicating why the overrides
are needed.
| <ModalFooterWithAlerts errorMessage={errorMessage}> | ||
| <Button | ||
| variant="danger" | ||
| onClick={handleSubmit} | ||
| form="delete-hpa-modal-form" | ||
| isLoading={inProgress} | ||
| isDisabled={inProgress || !!errorMessage} | ||
| data-test="confirm-action" |
There was a problem hiding this comment.
Don’t lock out retries after a failed delete.
Line 71 disables the Remove button when errorMessage is set, which prevents retry in the same modal session after transient failures.
🔧 Proposed fix
- isDisabled={inProgress || !!errorMessage}
+ isDisabled={inProgress || !hpaName}📝 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.
| <ModalFooterWithAlerts errorMessage={errorMessage}> | |
| <Button | |
| variant="danger" | |
| onClick={handleSubmit} | |
| form="delete-hpa-modal-form" | |
| isLoading={inProgress} | |
| isDisabled={inProgress || !!errorMessage} | |
| data-test="confirm-action" | |
| <ModalFooterWithAlerts errorMessage={errorMessage}> | |
| <Button | |
| variant="danger" | |
| onClick={handleSubmit} | |
| form="delete-hpa-modal-form" | |
| isLoading={inProgress} | |
| isDisabled={inProgress || !hpaName} | |
| data-test="confirm-action" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsx`
around lines 65 - 72, The Remove button currently disables when errorMessage is
set, preventing retries after a transient failure; update the Button in
DeleteHPAModal (the Button with props onClick={handleSubmit},
form="delete-hpa-modal-form", isLoading={inProgress}, isDisabled={inProgress ||
!!errorMessage}) to only disable based on inProgress (i.e., remove the "||
!!errorMessage" part) so users can retry while the modal is open; keep
errorMessage shown via ModalFooterWithAlerts and preserve
isLoading={inProgress}.
| <ModalFooterWithAlerts errorMessage={status && status.submitError}> | ||
| <Button | ||
| variant="danger" | ||
| onClick={onSubmit} | ||
| form="delete-resource-modal-form" | ||
| isLoading={isSubmitting} | ||
| isDisabled={(status && !!status.submitError) || !isValid || isSubmitting} | ||
| data-test="confirm-action" |
There was a problem hiding this comment.
Retry is blocked after the first failed delete attempt.
Line 69 disables the confirm action when status.submitError exists. After a failed request (Lines 98-100), users cannot retry without closing and reopening the modal.
💡 Suggested fix
- isDisabled={(status && !!status.submitError) || !isValid || isSubmitting}
+ isDisabled={!isValid || isSubmitting}Also applies to: 98-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx`
around lines 63 - 70, The confirm button is being disabled when
status.submitError exists which prevents retries; in DeleteResourceModal change
the isDisabled logic on the Button (rendered inside ModalFooterWithAlerts) to
not include status.submitError so the button remains enabled for retries (i.e.,
only disable when isSubmitting or !isValid), and also clear or reset
status.submitError at the start of the onSubmit flow in the onSubmit handler so
the previous error doesn't block UI state; reference the Button props,
status.submitError, isSubmitting, isValid, and the onSubmit function to
implement these two changes.
| import { AlertGroup, Flex, ModalFooter } from '@patternfly/react-core'; | ||
| import { ErrorMessage, InfoMessage } from '@console/internal/components/utils/button-bar'; | ||
|
|
||
| export const ModalFooterWithAlerts: React.FC<ModalFooterWithAlertsProps> = ({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the actual file
FILE='frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx'
echo "=== File contents ==="
cat -n "$FILE"Repository: openshift/console
Length of output: 1189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check TypeScript config files
echo "=== TypeScript config files ==="
fd 'tsconfig.*\.json' frontend/packages/console-shared --max-results 5 -exec sh -c 'echo "--- $1 ---" && head -50 "$1"' sh {} \;
echo
echo "=== Root tsconfig ==="
head -50 tsconfig.json || echo "No tsconfig.json in root"Repository: openshift/console
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for similar React.FC patterns in the same directory and nearby files
echo "=== React.FC usage in console-shared ==="
rg 'React\.(FC|ReactNode|FunctionComponent)' frontend/packages/console-shared/src/components/modals/ -C 1 -n
echo
echo "=== React import patterns in same directory ==="
rg '^import.*React' frontend/packages/console-shared/src/components/modals/ -nRepository: openshift/console
Length of output: 954
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check how other files in the repo handle React.FC
echo "=== React.FC patterns across codebase ==="
rg 'React\.FC<' frontend/packages/console-shared/src/components/ | head -20
echo
echo "=== Files with direct FC imports ==="
rg 'import.*\{.*FC\b' frontend/packages/console-shared/src/components/ | head -10Repository: openshift/console
Length of output: 1498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Root tsconfig.json ==="
head -80 tsconfig.json
echo
echo "=== Frontend package tsconfig ==="
find frontend -name "tsconfig.json" -not -path "*/node_modules/*" | head -5 | while read f; do
echo "--- $f ---"
head -50 "$f"
echo
doneRepository: openshift/console
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check package.json or similar to see if there's jsx/module settings
echo "=== Package JSX settings ==="
if [ -f "frontend/packages/console-shared/package.json" ]; then
cat frontend/packages/console-shared/package.json | head -30
fi
echo
echo "=== Check if React is explicitly imported elsewhere in the codebase ==="
rg 'import.*React\s+from' frontend/packages/console-shared/src/components/ | head -5Repository: openshift/console
Length of output: 408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find all tsconfig files ==="
find . -maxdepth 3 -name "tsconfig*.json" -type f 2>/dev/null | head -20
echo
echo "=== Check frontend root tsconfig ==="
if [ -f "frontend/tsconfig.json" ]; then
head -100 frontend/tsconfig.json
fiRepository: openshift/console
Length of output: 2409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check build/webpack config for JSX settings
echo "=== Webpack or build config JSX settings ==="
find . -maxdepth 2 -name "*.config.js" -o -name "*.config.ts" 2>/dev/null | head -5
echo
echo "=== Check babel config ==="
find . -maxdepth 3 -name ".babelrc*" -o -name "babel.config.*" 2>/dev/null | head -5Repository: openshift/console
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any existing React.FC usage patterns with React imports
echo "=== Check how FetchProgressModal imports React ==="
head -10 frontend/packages/console-shared/src/components/modals/FetchProgressModal.tsx
echo
echo "=== Compare: Check a file using React.FC without imports ==="
grep -A5 "import\|React\.FC\|ReactNode" frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx | head -20Repository: openshift/console
Length of output: 803
Add explicit type imports to fix TypeScript compilation error.
Using React.FC and React.ReactNode without importing the React namespace will fail in this configuration. The project consistently uses direct type imports (import type { FC, ReactNode } from 'react') as seen across the console-shared package. This aligns with the jsx: "react-jsx" setting in tsconfig, which doesn't require React runtime imports but still requires explicit type imports.
Suggested fix
+import type { FC, ReactNode } from 'react';
import { AlertGroup, Flex, ModalFooter } from '@patternfly/react-core';
import { ErrorMessage, InfoMessage } from '@console/internal/components/utils/button-bar';
-export const ModalFooterWithAlerts: React.FC<ModalFooterWithAlertsProps> = ({
+export const ModalFooterWithAlerts: FC<ModalFooterWithAlertsProps> = ({
children,
errorMessage,
message,
}) => (
@@
type ModalFooterWithAlertsProps = {
- children: React.ReactNode;
+ children: ReactNode;
message?: string;
errorMessage?: string;
};Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-shared/src/components/modals/ModalFooterWithAlerts.tsx`
at line 4, The file uses React.FC and React.ReactNode types without importing
them; add explicit type-only imports and update the component signature to use
those imports: import type { FC, ReactNode } from 'react' and replace
React.FC<ModalFooterWithAlertsProps> (and any React.ReactNode usages referenced
by ModalFooterWithAlertsProps) with FC<ModalFooterWithAlertsProps> and ReactNode
respectively so TypeScript compiles under jsx: "react-jsx"; ensure
ModalFooterWithAlerts and ModalFooterWithAlertsProps references are updated to
use the imported types.
| export const useDeleteApplicationAction = ( | ||
| application: TopologyApplicationObject, | ||
| resourceModel: K8sModel, | ||
| ): Action => { | ||
| // accessReview needs a resource but group is not a k8s resource, | ||
| // so currently picking the first resource to do the rbac checks (might change in future) | ||
| const primaryResource = application.resources[0].resource; | ||
| return { | ||
| id: 'delete-application', | ||
| label: i18next.t('devconsole~Delete application'), | ||
| cta: () => { | ||
| const reqs = []; | ||
| deleteResourceModal({ | ||
| blocking: true, | ||
| resourceName: application.name, | ||
| resourceType: ApplicationModel.label, | ||
| onSubmit: () => { | ||
| application.resources.forEach((resource) => { | ||
| reqs.push(cleanUpWorkload(resource.resource)); | ||
| }); | ||
| return Promise.all(reqs); | ||
| }, | ||
| }); | ||
| }, | ||
| accessReview: asAccessReview(resourceModel, primaryResource, 'delete'), | ||
| }; | ||
| const { t } = useTranslation(); | ||
| const launchModal = useOverlay(); | ||
|
|
||
| return useMemo(() => { | ||
| if (!application?.resources?.[0]?.resource) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the context-menu.ts file to see the actual implementation
wc -l frontend/packages/dev-console/src/actions/context-menu.tsRepository: openshift/console
Length of output: 121
🏁 Script executed:
# Read the function around lines 13-50 to see full implementation
sed -n '1,80p' frontend/packages/dev-console/src/actions/context-menu.ts | cat -nRepository: openshift/console
Length of output: 3096
🏁 Script executed:
# Check how useDeleteApplicationAction is used in providers.tsx
rg -n -B2 -A5 'useDeleteApplicationAction' frontend/packages/dev-console/src/actions/Repository: openshift/console
Length of output: 2481
🏁 Script executed:
# Verify the Action type definition - where it comes from
rg -n 'from.*Action|import.*Action' frontend/packages/dev-console/src/actions/context-menu.ts | head -20Repository: openshift/console
Length of output: 131
🏁 Script executed:
# Check if there's any null safety in the consumers
rg -n 'deleteApplicationAction|useDeleteApplicationAction' frontend/packages/dev-console/src/ -lRepository: openshift/console
Length of output: 174
🏁 Script executed:
# See the full context of how deleteApplicationAction is used in the return
sed -n '296,320p' frontend/packages/dev-console/src/actions/providers.tsx | cat -nRepository: openshift/console
Length of output: 1299
🏁 Script executed:
# Check the disabledActionsFilter mentioned in the original review
rg -n -B5 -A10 'disabledActionsFilter' frontend/packages/dev-console/src/actions/add-resources.ts | head -40Repository: openshift/console
Length of output: 159
🏁 Script executed:
# Check the return type of useK8sModel
rg -n -B3 -A3 'useK8sModel' frontend/packages/dev-console/src/actions/providers.tsx | head -20Repository: openshift/console
Length of output: 918
🏁 Script executed:
# Verify the Action type definition to understand the contract
rg -n 'type Action' frontend/packages/dev-console/src/actions/ -A5 2>/dev/null || rg -n 'interface Action' frontend/packages/dev-console/src/ -A5 | head -20Repository: openshift/console
Length of output: 583
Fix type contract mismatch and guard unresolved model reference.
The function declares return type Action (line 16) but returns null (line 22), creating a TypeScript type violation. Additionally, resourceModel is a required parameter but the consumer passes kindObj from useK8sModel() before the inFlight loading check, allowing undefined to reach line 44's asAccessReview() call.
🔧 Proposed fix
export const useDeleteApplicationAction = (
application: TopologyApplicationObject,
resourceModel: K8sModel,
-): Action => {
+): Action | null => {
const { t } = useTranslation();
const launchModal = useOverlay();
return useMemo(() => {
- if (!application?.resources?.[0]?.resource) {
+ if (!resourceModel || !application?.resources?.[0]?.resource) {
return null;
}📝 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.
| export const useDeleteApplicationAction = ( | |
| application: TopologyApplicationObject, | |
| resourceModel: K8sModel, | |
| ): Action => { | |
| // accessReview needs a resource but group is not a k8s resource, | |
| // so currently picking the first resource to do the rbac checks (might change in future) | |
| const primaryResource = application.resources[0].resource; | |
| return { | |
| id: 'delete-application', | |
| label: i18next.t('devconsole~Delete application'), | |
| cta: () => { | |
| const reqs = []; | |
| deleteResourceModal({ | |
| blocking: true, | |
| resourceName: application.name, | |
| resourceType: ApplicationModel.label, | |
| onSubmit: () => { | |
| application.resources.forEach((resource) => { | |
| reqs.push(cleanUpWorkload(resource.resource)); | |
| }); | |
| return Promise.all(reqs); | |
| }, | |
| }); | |
| }, | |
| accessReview: asAccessReview(resourceModel, primaryResource, 'delete'), | |
| }; | |
| const { t } = useTranslation(); | |
| const launchModal = useOverlay(); | |
| return useMemo(() => { | |
| if (!application?.resources?.[0]?.resource) { | |
| return null; | |
| } | |
| export const useDeleteApplicationAction = ( | |
| application: TopologyApplicationObject, | |
| resourceModel: K8sModel, | |
| ): Action | null => { | |
| const { t } = useTranslation(); | |
| const launchModal = useOverlay(); | |
| return useMemo(() => { | |
| if (!resourceModel || !application?.resources?.[0]?.resource) { | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/packages/dev-console/src/actions/context-menu.ts` around lines 13 -
23, The function useDeleteApplicationAction currently declares a return type of
Action but returns null and also assumes resourceModel is resolved before use;
update the signature to return Action | null (or always return a valid Action)
and add a runtime guard that early-returns null if resourceModel is falsy before
any use (e.g., before calling asAccessReview or using resourceModel in the
memoized callback). Ensure callers that pass kindObj/from useK8sModel wait for
inFlight or handle the potential null return from useDeleteApplicationAction so
asAccessReview is never invoked with an undefined model.
| <Link | ||
| className={css( | ||
| 'pf-v6-c-button', | ||
| 'pf-m-primary', | ||
| { | ||
| 'pf-m-disabled': !detailsItem.obj || detailsItem.isInstalling, | ||
| }, | ||
| 'co-catalog-page__overlay-action', | ||
| )} | ||
| data-test-id="operator-install-btn" | ||
| to={installLink} | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
FILE='frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx'
# First, check if file exists and get line count
if [ -f "$FILE" ]; then
echo "=== File exists, checking size ==="
wc -l "$FILE"
else
echo "=== File not found ==="
exit 1
fiRepository: openshift/console
Length of output: 196
🏁 Script executed:
#!/bin/bash
FILE='frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx'
# Read lines 1050-1090 to see full context around the Link component
echo "=== Checking lines 1050-1090 ==="
sed -n '1050,1090p' "$FILE"Repository: openshift/console
Length of output: 1697
🏁 Script executed:
#!/bin/bash
FILE='frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx'
# Search for any aria-disabled or tabIndex in the operator-install-btn context
echo "=== Searching for aria-disabled in the file ==="
rg 'aria-disabled' "$FILE"
echo "=== Searching for operator-install-btn and surrounding context ==="
rg -n 'operator-install-btn' "$FILE" -B2 -A15Repository: openshift/console
Length of output: 927
Link still lacks semantic disabled state and activation guards.
The operator-install-btn Link (line 1066) applies only the visual pf-m-disabled CSS class without preventing keyboard activation. This allows users to navigate to and activate the link via keyboard despite it appearing disabled.
- Missing
aria-disabledfor screen reader announcement - Missing
tabIndex={-1}to remove from keyboard focus when disabled - Missing
onClickhandler to prevent navigation on activation when disabled
Notably, the adjacent "Uninstall" Button (line 1081) uses isDisabled prop, showing the component library supports proper disabled semantics—the Link should follow the same pattern. To fix, either:
- Add
aria-disabled, managetabIndexdynamically, and guard navigation inonClick, or - Conditionally render a disabled button instead of a navigable link when
!detailsItem.obj || detailsItem.isInstalling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.tsx`
around lines 1066 - 1077, The Link with data-test-id "operator-install-btn" is
only visually disabled via the 'pf-m-disabled' class but still
keyboard-focusable and activatable; update the component (the Link rendering
using installLink and checking detailsItem.obj / detailsItem.isInstalling) to
either render a proper disabled Button when disabled, or augment the Link with
proper semantics: add aria-disabled={true} when !detailsItem.obj ||
detailsItem.isInstalling, set tabIndex={-1} when disabled, and attach an onClick
handler that calls event.preventDefault() / stops navigation when disabled;
ensure the logic uses the same condition used for the 'pf-m-disabled' class so
the visual, keyboard, and screen-reader states stay consistent.
|
@rhamilto: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
Migrates DeleteHPAModal, ConsolePluginModal, and DeleteResourceModal from deprecated factory modal components to modern PatternFly v6 Modal.
Note: This PR includes changes from #16015 (as well as other unmerged PRs), so they should merge first.
Changes
ModalWrapperwithModalcomponent with state managementModalTitlewithModalHeaderModalSubmitFooterwithModalFooterWithAlerts<form>with PatternFlyFormcomponentButtoncomponents with consistent loading/disabled statesTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
UI/Style Updates
Bug Fixes
Refactoring