[WIP] 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
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
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>
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
d088693 to
161091f
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. |
|
@coderabbitai 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. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/knative-plugin/src/actions/providers.ts (1)
119-119: Potential logic issue:||should likely be&&for readiness check.Using
||meansisReadyistrueif either set of actions is ready. IfcommonActionsReadyistruebutdeploymentActionsReadyisfalse, you'll build the actions array with potentially stale/undefineddeploymentActions.EditResourceLimits. This can cause runtime errors or incorrect UI state.🔧 Proposed fix
- const isReady = commonActionsReady || deploymentActionsReady; + const isReady = commonActionsReady && deploymentActionsReady;
🤖 Fix all issues with AI agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts`:
- Around line 42-60: The useMemo in useDeleteResourceAction incorrectly includes
the launchModal reference (returned from useOverlay) in its dependency array,
which breaks memoization because useOverlay returns a new function each render;
remove launchModal from the dependency array so the memoized Action stays
referentially stable. Locate useDeleteResourceAction and update the useMemo
dependencies to [t, kind, obj] (keeping asAccessReview and
LazyDeleteModalProvider usage unchanged); if lint rules complain, add a brief
eslint-disable-next-line comment scoped to that dependency line explaining that
launchModal is intentionally excluded because useOverlay returns a new
reference.
In `@frontend/public/components/modals/configure-ns-pull-secret-modal.tsx`:
- Around line 317-326: The prop spread {...props} is placed after the explicit
cancel and close props so caller-supplied cancel/close can override
props.closeOverlay; in ConfigureNamespacePullSecretModalProvider move the spread
to before the explicit assignments so ConfigureNamespacePullSecret receives
{...props} first and then cancel={props.closeOverlay} close={props.closeOverlay}
to ensure the modal always uses props.closeOverlay for both handlers.
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx`:
- Around line 241-250: The modal provider is being exported incorrectly and its
close handlers can be overridden; ensure the module's public export is the
provider component ConfigureUpdateStrategyModalProvider (not the inner
ConfigureUpdateStrategy form) so the ModalWrapper and overlay behavior are
preserved, and inside ConfigureUpdateStrategyModalProvider pass {...props}
before explicitly setting cancel={props.closeOverlay} and
close={props.closeOverlay} when rendering ConfigureUpdateStrategyModal so
caller-provided handlers cannot override the provider's close control; update
the index.ts export to export ConfigureUpdateStrategyModalProvider as the
lazy-loaded component and keep ModalWrapper wrapping
ConfigureUpdateStrategyModal unchanged.
In `@frontend/public/components/modals/index.ts`:
- Around line 48-55: The lazy import currently maps to m.ConfigureUpdateStrategy
(a plain FC) but should map to the overlay provider export; update the promise
resolution in LazyConfigureUpdateStrategyModalProvider to return default:
m.ConfigureUpdateStrategyModalProvider so the lazy-loaded component is the
OverlayComponent<ConfigureUpdateStrategyModalProps> provider rather than the
plain modal component; ensure the exported name
LazyConfigureUpdateStrategyModalProvider remains unchanged and that the import
string './configure-update-strategy-modal' stays the same.
In `@frontend/public/components/modals/managed-resource-save-modal.tsx`:
- Around line 49-55: In ManagedResourceSaveModalProvider, the current prop order
passes close={props.closeOverlay} before {...props}, allowing a caller-supplied
close in {...props} to override the provider's handler; change the JSX to spread
{...props} first and then explicitly set close={props.closeOverlay} on the
ManagedResourceSaveModal (within the ModalWrapper) so the provider-controlled
close handler cannot be overridden.
🧹 Nitpick comments (3)
frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx (1)
278-288: Provider implementation looks solid.The
ClonePVCModalProvidercorrectly:
- Types as
OverlayComponent<ClonePVCModalProps>matching the SDK contract- Uses
blockingonModalWrapperto prevent accidental dismissal during form interaction- Wires
closeOverlayto bothcancelandclose, ensuring consistent teardownOne minor observation:
{...props}spreadscloseOverlayintoClonePVCModal, but the inner component only destructuresclose,cancel, andresource, so this is harmless. If you want to be explicit and avoid passing extra props to the DOM, you could destructure and omitcloseOverlay:♻️ Optional: explicit prop handling
-export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { +export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = ({ + closeOverlay, + ...restProps +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ClonePVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ClonePVCModal {...restProps} cancel={closeOverlay} close={closeOverlay} /> </ModalWrapper> ); };frontend/packages/console-app/src/actions/hooks/useVolumeSnapshotActions.ts (1)
4-55: StabilizelaunchModalto preserve memoization.
GivenlaunchModalis documented as unstable per render, including it in the deps makesfactoryandactionsre-create every render, which can ripple into unnecessary re-renders downstream. Prefer omitting it with an eslint comment (as done elsewhere) or wrapping via a stable ref/callback.♻️ Suggested adjustment (minimal)
- const factory = useMemo( + // eslint-disable-next-line react-hooks/exhaustive-deps -- launchModal is unstable per render + const factory = useMemo( () => ({ [VolumeSnapshotActionCreator.RestorePVC]: () => ({ id: 'clone-pvc', label: t('console-app~Restore as new PVC'), disabled: !resource?.status?.readyToUse, tooltip: !resource?.status?.readyToUse ? t('console-app~Volume Snapshot is not Ready') : '', cta: () => launchModal(LazyRestorePVCModalProvider, { resource, }), accessReview: asAccessReview(VolumeSnapshotModel, resource, 'create'), }), }), - [t, resource, launchModal], + [t, resource], );frontend/public/components/factory/modal.tsx (1)
34-41: Guard againstappElementbeing null.
document.querySelectorcan return null (tests, plugin contexts). A fallback (e.g.,document.body) avoids react-modal warnings and preserves a11y behavior.♻️ Suggested hardening
- const appElement = document.querySelector('#app-content') || document.querySelector('#app'); + const appElement = + document.querySelector('#app-content') || + document.querySelector('#app') || + document.body;
frontend/public/components/modals/configure-ns-pull-secret-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/public/components/modals/configure-update-strategy-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/public/components/modals/managed-resource-save-modal.tsx
Outdated
Show resolved
Hide resolved
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
161091f to
e0b7cdb
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
force-pushed
the
CONSOLE-5012-2
branch
from
e0b7cdb to
a831711
Compare
Additional ImprovementsBug Fix: Undefined Kind Parameter HandlingFixed a potential crash in delete action hooks when Files Updated:
Why This Matters:
Affected Callers (now safe):
Code OrganizationAlso improved code organization in 🤖 Generated with Claude Code |
|
@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. |
|
/retest |
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>
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
a831711 to
78a4f2c
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. |
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 |
2 similar comments
|
/retest |
|
/retest |
|
@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 |
3 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