CONSOLE-5012: Migrate modals to overlay pattern #15939
Conversation
|
@rhamilto: No Jira issue with key CONSOLE-5212 exists in the tracker at https://issues.redhat.com/. 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
bot
added
the
do-not-merge/work-in-progress
|
@rhamilto: No Jira issue with key CONSOLE-5212 exists in the tracker at https://issues.redhat.com/. 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
bot
added
component/core
|
@rhamilto: No Jira issue with key CONSOLE-5212 exists in the tracker at https://issues.redhat.com/. 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. |
1 similar comment
|
@rhamilto: No Jira issue with key CONSOLE-5212 exists in the tracker at https://issues.redhat.com/. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
aebae95 to
0ec415a
Compare
|
@rhamilto: This pull request references CONSOLE-5012 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-5012 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. |
1 similar comment
|
@rhamilto: This pull request references CONSOLE-5012 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. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request refactors the modal invocation system from direct function calls to an overlay-based provider pattern. Modal components are wrapped in new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts`:
- Around line 1-6: Guard against application.resources being undefined by
defaulting to an empty array before iterating: replace direct calls like
application.resources.forEach(...) with (application.resources ??
[]).forEach(...) (or Array.isArray check) in the delete action and any cleanup
loops that touch application.resources (e.g., the delete modal / cleanup logic
using application.resources), and apply the same change to all other occurrences
in this file where application.resources is iterated.
In `@frontend/packages/knative-plugin/src/actions/creators.ts`:
- Around line 120-153: The useMemo in useDeleteKnativeServiceResource is
including the launchModal value from useOverlay in its dependency array which
forces the memo to recreate on every render; remove launchModal from the
dependencies and leave [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag]
instead, and add the same explanatory comment used elsewhere: "missing
launchModal dependency, that causes max depth exceeded error."; apply the same
change/comment pattern to SecretKeySelector's hook where launchModal is
currently included.
In `@frontend/public/components/modals/configure-cluster-upstream-modal.tsx`:
- Around line 169-178: The provider currently spreads {...props} after passing
cancel/close to ConfigureClusterUpstreamModal so caller-supplied cancel/close
can override the provider's closeOverlay; update
ConfigureClusterUpstreamModalProvider to pass {...props} first and then
explicitly set cancel={props.closeOverlay} and close={props.closeOverlay} (or
otherwise ensure the provider's closeOverlay wins) so the ModalWrapper/provider
always controls closing; refer to ConfigureClusterUpstreamModalProvider,
ConfigureClusterUpstreamModal, props, and closeOverlay when making the change.
In `@frontend/public/components/modals/index.ts`:
- Around line 97-102: The lazy import for LazyClonePVCModalProvider should use
the module default export to match the pattern used by
LazyExpandPVCModalProvider and LazyConfigureClusterUpstreamModalProvider; update
the .then handler to return { default: m.default } (or simply use m.default)
instead of { default: m.ClonePVCModalProvider } so it consistently consumes the
component via the module's default export (the component is exported both named
and default as ClonePVCModalProvider).
🧹 Nitpick comments (6)
frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/OverlayProvider.tsx (1)
55-57: Suspense addition is appropriate for lazy-loaded modal providers.The
fallback={null}is the right choice here—you don't want a loading spinner flashing before the modal materializes. This cleanly supports the lazy provider pattern (React.lazy()) being rolled out across the modal migrations.One consideration: if a lazy chunk fails to load (network hiccup, 404'd chunk after a deployment), the error will propagate upward with no local catch. You might want to wrap this in an
ErrorBoundaryto gracefully handle chunk loading failures without potentially unmounting the entire overlay context tree.💡 Optional: Add ErrorBoundary for chunk loading resilience
+import ErrorBoundary from '@console/shared/src/components/error/ErrorBoundary'; +// Or implement a minimal local ErrorBoundary if preferred // Inside the render: {_.map(componentsMap, (c, id) => ( - <Suspense key={id} fallback={null}> - <c.Component {...c.props} closeOverlay={() => closeOverlay(id)} /> - </Suspense> + <ErrorBoundary key={id} FallbackComponent={() => null}> + <Suspense fallback={null}> + <c.Component {...c.props} closeOverlay={() => closeOverlay(id)} /> + </Suspense> + </ErrorBoundary> ))}This ensures a failed lazy import doesn't take down the entire overlay system—graceful degradation for production resilience.
frontend/packages/console-app/src/actions/hooks/useRetryRolloutAction.ts (1)
98-100: Avoid stalelaunchModalby stabilizing it instead of suppressing deps.Suppressing exhaustive-deps can capture a stale overlay context if the provider remounts; use a ref to keep the latest
launchModaland drop the eslint disable.♻️ Proposed refactor
-import { useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; @@ const { t } = useTranslation(); const launchModal = useOverlay(); + const launchModalRef = useRef(launchModal); + useEffect(() => { + launchModalRef.current = launchModal; + }, [launchModal]); @@ - cta: () => - retryRollout(rcModel, rc).catch((err) => launchModal(ErrorModal, { error: err.message })), + cta: () => + retryRollout(rcModel, rc).catch((err) => + launchModalRef.current(ErrorModal, { error: err.message }), + ), @@ - // missing launchModal dependency, that causes max depth exceeded error - // eslint-disable-next-line react-hooks/exhaustive-deps [t, dcModel, rcModel, rc, canRetry, resource],frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx (1)
279-285: Guard against callers overridingcloseOverlay.
Because...propscomes last, any passedcancel/closewould override the overlay close handler. Prefer spreading first, then explicitly wiringcloseOverlayto enforce correct dismissal.♻️ Suggested tweak
-export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { +export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ClonePVCModal {...rest} cancel={closeOverlay} close={closeOverlay} /> </ModalWrapper> ); };Also applies to: 287-288
frontend/public/components/modals/expand-pvc-modal.tsx (1)
99-105: Provider implementation follows the established pattern.The
ModalWrappercorrectly receivesblockingandonClose. WiringcloseOverlayto bothcancelandcloseensures the modal dismisses properly on either action.One minor note:
{...props}spreadscloseOverlayintoExpandPVCModal, butExpandPVCModaldestructures onlyresource,kind,close, andcancel. The extra prop is harmless but slightly redundant. Consider destructuring explicitly if you want cleaner prop forwarding:♻️ Optional cleanup
-export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => { +export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ExpandPVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ExpandPVCModal cancel={closeOverlay} close={closeOverlay} {...rest} /> </ModalWrapper> ); };frontend/packages/console-app/src/actions/hooks/useDeploymentActions.ts (1)
170-178: Note:resourceLimitsModalstill uses legacy pattern.The
EditResourceLimitsaction still invokesresourceLimitsModaldirectly rather than throughlaunchModal. Given the PR title indicates WIP, this is presumably planned for a future commit. Just flagging for tracking purposes.frontend/public/components/modals/configure-update-strategy-modal.tsx (1)
241-253: LGTM - Clean provider implementation following the overlay pattern.The migration correctly wraps
ConfigureUpdateStrategyModalinsideModalWrapperwithblockingbehavior and wirescloseOverlayto bothcancelandclosehandlers. The backward-compatible default export maintains API stability.Minor observation: The spread
{...props}passescloseOverlayto the inner modal which doesn't use it. This is harmless but consider destructuring to keep props clean:✨ Optional cleanup
-export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ( - props, -) => { +export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> + <ModalWrapper blocking onClose={closeOverlay}> <ConfigureUpdateStrategyModal - cancel={props.closeOverlay} - close={props.closeOverlay} - {...props} + cancel={closeOverlay} + close={closeOverlay} + {...rest} /> </ModalWrapper> ); };
frontend/public/components/modals/configure-cluster-upstream-modal.tsx
Outdated
Show resolved
Hide resolved
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
0ec415a to
08d46e0
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
component/shared
|
@rhamilto: This pull request references CONSOLE-5012 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. |
This commit migrates all instances of confirmModal to use the modern overlay pattern with useWarningModal, completing the modal migration effort started in previous PRs. Changes: - Removed deprecated confirmModal launcher and confirm-modal.tsx file - Migrated topology moveNodeToGroup to use useWarningModal with proper cancel handling via global handler pattern - Updated StopNodeMaintenanceModal to remove deprecated imperative API and keep only the useStopNodeMaintenanceModal hook - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread to prevent React DOM warnings - Added try-catch in topology drag-drop to prevent optimistic update when user cancels the move operation - Updated all metal3-plugin maintenance components to use the hook-based approach This PR builds upon and depends on: - openshift#15932 (ResourceLimitsModal migration) - openshift#15939 (DeletePDBModal migration) - openshift#15940 (ConfigureUnschedulableModal migration) - openshift#15946 (Global error modal utility) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
openshift-ci
bot
added
px-approved
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@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. |
This commit refactors configure-update-strategy-modal to use the OverlayComponent pattern, eliminating createModalLauncher. Changes: - Refactored configure-update-strategy-modal.tsx to use OverlayComponent - Added ConfigureUpdateStrategyModalProvider with ModalWrapper - Added LazyConfigureUpdateStrategyModalProvider with React.lazy() - Updated useDeploymentActions to use the new lazy provider - Removed deprecated configureUpdateStrategyModal from index.ts This change maintains backward compatibility through default exports and preserves lazy loading functionality. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit migrates the configure namespace pull secret modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern. Changes: - Exported ConfigureNamespacePullSecretModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified namespace.jsx to use useOverlay hook instead of direct modal call Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tern This commit migrates the configure cluster upstream modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern and fixes react-modal accessibility warnings. Changes: - Exported ConfigureClusterUpstreamModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified details-page.tsx to use useOverlay hook instead of direct modal call - Removed unnecessary TFunction prop from modal type definition - Fixed react-modal warnings by using useLayoutEffect for global app element setup - Added explicit appElement prop to ModalWrapper for robust timing handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit migrates the managed resource save modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern. Changes: - Exported ManagedResourceSaveModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified edit-yaml.tsx to use useOverlay hook instead of direct modal call - Removed unnecessary 'kind' prop from modal invocation - Updated type definition to extend ModalComponentProps Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The remove-user-modal was already replaced by useWarningModal in group.tsx and is no longer used anywhere in the codebase. Changes: - Deleted remove-user-modal.tsx - Removed LazyRemoveUserModalProvider export from index.ts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
78a4f2c to
1664f88
Compare
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, 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 |
5 participants
Summary
This PR migrates several modals from the legacy
createModalLauncherpattern to the newOverlayComponentpattern, improving code consistency and maintainability. All modals follow the*ModalOverlaynaming convention to clearly distinguish from the deprecatedModalProviderpattern.Note: This PR builds on #15932, which should merge first.
Changes
This PR includes the following modal migrations:
ConfigureUpdateStrategyModalOverlaycomponentConfigureNamespacePullSecretModalOverlaycomponentConfigureClusterUpstreamModalOverlaycomponentManagedResourceSaveModalOverlaycomponentNaming Convention
All modal overlay components and exports follow the
*ModalOverlaynaming pattern:ConfigureUpdateStrategyModalOverlay,ConfigureNamespacePullSecretModalOverlay, etc.LazyConfigureUpdateStrategyModalOverlay,LazyConfigureNamespacePullSecretModalOverlay, etc.This naming convention:
ModalProviderpatternOverlayComponenttypeMigration Pattern
Each migration follows a consistent pattern:
OverlayComponentusing the*ModalOverlaynaming conventionModalWrapperand connect to overlay lifecycle (closeOverlay)React.lazy()with matching naminguseOverlayhook withlaunchModal()instead of direct modal calls{...props}before explicitcancelandclosepropsTechnical Details
ModalComponentPropsinterfacereact-modalapp element setup handled in App component (from CONSOLE-5012: Migrate delete modals and PVC modals to overlay pattern #15932)*ModalOverlaynaming across all exports and referencesExample
Test Plan
*ModalOverlaypatternBenefits
*ModalOverlaynaming pattern across all modalsModalProviderpatternuseOverlay()🤖 Generated with Claude Code