CONSOLE-5012: Migrate confirmModal to overlay pattern#15948
CONSOLE-5012: Migrate confirmModal to overlay pattern#15948openshift-merge-bot[bot] merged 7 commits intoopenshift:mainfrom
Conversation
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
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
component/core
|
@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 introduces a modal infrastructure refactor across the OpenShift Console codebase, shifting from a direct modal launcher pattern (via 🚥 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/packages/console-shared/src/components/formik-fields/EnvironmentField.tsx (1)
64-80: StabilizelaunchModalreference instead of suppressing hook depsThe dependency exhaustive-deps suppression masks a real issue:
useOverlay()returns an unstable reference becauseOverlayContext.Providercreates a new value object on every render, even though the underlying callback is memoized. AddinglaunchModalto deps would trigger infinite re-runs (max depth exceeded), so the ref pattern below is the right approach at the consumer level.♻️ Proposed refactor
-import { useMemo, useState, useCallback, useEffect } from 'react'; +import { useMemo, useState, useCallback, useEffect, useRef } from 'react'; const { t } = useTranslation(); const launchModal = useOverlay(); +const launchModalRef = useRef(launchModal); +useEffect(() => { + launchModalRef.current = launchModal; +}, [launchModal]); useEffect(() => { Promise.all([k8sGet(ConfigMapModel, null, namespace), k8sGet(SecretModel, null, namespace)]) .then(([nsConfigMaps, nsSecrets]) => { setConfigMaps(nsConfigMaps); setSecrets(nsSecrets); }) .catch(async (err) => { if (err?.response?.status !== 403) { try { - await launchModal(ErrorModal, { error: err?.message }); + await launchModalRef.current(ErrorModal, { error: err?.message }); // eslint-disable-next-line no-empty } catch (e) {} } }); - // missing launchModal dependency, that causes max depth exceeded error - // eslint-disable-next-line react-hooks/exhaustive-deps }, [namespace]);frontend/packages/topology/src/utils/moveNodeToGroup.tsx (1)
41-108: Fix the type contract:targetGroupshould be optional andupdateTopologyResourceApplicationmust acceptstring | null.The function declares
targetGroup: Nodeas required, but the implementation treats it as optional (see line 54'stargetGroup?.getLabel()and line 58's conditional check). Line 82 passesnulltoupdateTopologyResourceApplication, which expectsapplication: stringper the function signature in topology-utils.ts:137–140. Additionally, line 102 will crash ifsourceGroupis falsy andtargetGroupis somehow undefined, calling.getLabel()on a falsy value.Update the type signature to match the implementation, and widen
updateTopologyResourceApplication'sapplicationparameter tostring | nullto safely handle the "remove from application" case:Proposed fix
export const moveNodeToGroup = ( node: Node, - targetGroup: Node, + targetGroup: Node | null, onError?: (error: string) => void, ): Promise<void> => {Then in topology-utils.ts:
export const updateTopologyResourceApplication = ( item: Node, - application: string, + application: string | null, ): Promise<any> => {and add an early guard:
const itemData = item?.getData(); const resource = getResource(item); - if (!item || !resource || !_.size(itemData.resources)) { + if (!item || !resource || application === null || !_.size(itemData.resources)) { return Promise.reject(); }frontend/packages/shipwright-plugin/src/actions/useBuildRunActions.ts (1)
4-4: Typo: Double slash in import path.The import path contains
actions//hookswith a double slash. While most bundlers normalize this, it's a minor inconsistency that should be cleaned up.Suggested fix
-import { useCommonResourceActions } from '@console/app/src/actions//hooks/useCommonResourceActions'; +import { useCommonResourceActions } from '@console/app/src/actions/hooks/useCommonResourceActions';frontend/packages/shipwright-plugin/src/actions/useBuildActions.ts (1)
75-89: Remove the duplicate Delete action at line 89.The
useCommonResourceActionshook returns all common actions including Delete viaObject.values(commonActions)at line 75. Explicitly pushingcommonActions.Deleteagain at line 89 creates a duplicate entry in the actions menu.
🤖 Fix all issues with AI agents
In
`@frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx`:
- Around line 279-283: The spread props currently come after explicit close
props in ClonePVCModalProvider which allows props.closeOverlay to be overridden;
update the JSX so {...props} is spread first and then pass
cancel={props.closeOverlay} and close={props.closeOverlay} (affecting the
ClonePVCModal inside ModalWrapper) to ensure closeOverlay always wins and cannot
be overridden by incoming props.
In
`@frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx`:
- Around line 282-287: The provider currently spreads props after passing
cancel/close to RestorePVCModal, allowing a caller to override those with their
own cancel/close; change RestorePVCModalProvider so you destructure closeOverlay
from props first (e.g., const { closeOverlay, ...rest } = props) then render
<RestorePVCModal cancel={closeOverlay} close={closeOverlay} {...rest} /> inside
ModalWrapper, ensuring props.closeOverlay cannot be overridden by
caller-supplied props.
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`:
- Around line 51-64: The provider currently spreads props after assigning cancel
and close which allows caller props to override them; update
ConfigureUnschedulableModalProvider to destructure props into { closeOverlay,
...rest } (or extract known keys) and then render ConfigureUnschedulableModal
with {...rest} followed by explicit cancel={closeOverlay} and
close={closeOverlay} so the overlay's close handlers cannot be overridden;
ensure ModalWrapper still uses closeOverlay for onClose and pass the same
closeOverlay to ConfigureUnschedulableModal.
In `@frontend/packages/console-shared/src/utils/error-modal-handler.ts`:
- Around line 35-50: The cleanup unconditionally clears the global launcher
causing newer launchers to be removed; fix useSetupGlobalErrorModalLauncher by
capturing the specific errorModalLauncher reference you set via
setGlobalErrorModalLauncher and, in the return cleanup, only call
setGlobalErrorModalLauncher(null) if the current global launcher equals that
captured reference (so use a stable local/ref for errorModalLauncher and guard
the cleanup), referencing the existing functions errorModalLauncher,
useSetupGlobalErrorModalLauncher, and setGlobalErrorModalLauncher to locate and
implement the guarded cleanup.
In `@frontend/packages/knative-plugin/src/actions/creators.ts`:
- Around line 4-5: The SDK currently forces imports from an internal path to
reach useOverlay; update the SDK public exports so useOverlay can be imported
from '@console/dynamic-plugin-sdk' by re-exporting the modal overlay API: add an
export for the modal-support module (e.g., export * from './modal-support') from
the SDK app index or alternatively export the dynamic-core-api module from the
SDK root so that the symbol useOverlay is publicly available; modify the module
that aggregates app exports to re-export useOverlay (and related modal types) so
consumers can import { useOverlay } from '@console/dynamic-plugin-sdk'.
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx`:
- Around line 241-250: The provider currently spreads {...props} after setting
cancel/close so a caller can override cancel/close; update
ConfigureUpdateStrategyModalProvider to spread props first into
ConfigureUpdateStrategyModal (or omit cancel/close from the spread) and then
pass cancel={props.closeOverlay} and close={props.closeOverlay} to enforce using
props.closeOverlay; locate the ConfigureUpdateStrategyModalProvider and
ConfigureUpdateStrategyModal usages to apply this change so closeOverlay cannot
be overridden by caller props.
In `@frontend/public/components/modals/delete-modal.tsx`:
- Around line 183-188: The DeleteModalProvider currently spreads props after
passing cancel and close, allowing a caller to override cancel/close; in the
DeleteModalProvider component (and the ModalWrapper usage) spread {...props}
first and then pass cancel={props.closeOverlay} and close={props.closeOverlay}
so the enforced overlay handlers cannot be overridden by caller-provided props
(refer to DeleteModalProvider, ModalWrapper, DeleteModal, and
props.closeOverlay).
In `@frontend/public/components/modals/delete-pvc-modal.tsx`:
- Around line 87-91: The DeletePVCModalProvider currently spreads {...props}
after explicit cancel/close props which allows callers to override
props.closeOverlay; change the prop order in the JSX so you spread {...props}
first and then pass cancel={props.closeOverlay} close={props.closeOverlay} (on
ModalWrapper keep onClose={props.closeOverlay}) to ensure DeletePVCModal and
ModalWrapper always use props.closeOverlay for teardown and cannot be overridden
by incoming props.
♻️ Duplicate comments (2)
frontend/packages/console-app/src/actions/hooks/useJobActions.ts (1)
55-57: Same launcher-dep suppression pattern—please confirm stability.If
useConfigureCountModal()returns a new function over time, omitting it from deps can leave the action stale. This mirrors the earlier launcher-dep concern.frontend/packages/console-app/src/actions/providers/build-config-provider.ts (1)
36-38: Launcher-dep suppression repeated here—confirm the hook’s launcher is stable.Same dependency omission pattern as in other action hooks; please ensure the launcher identity is stable or the action can become stale.
Also applies to: 71-73
🧹 Nitpick comments (16)
frontend/public/components/utils/webhooks.tsx (1)
90-115: TheisLoadeddependency may cause an unnecessary double-fetch.The effect sets
isLoaded = trueat line 111, andisLoadedis also listed in the dependency array. On initial mount:
isLoadedisfalse→ effect runs → secrets fetched →setLoaded(true)isLoadedchanges totrue→ effect re-runs → secrets fetched againConsider adding a guard or removing
isLoadedfrom deps if it's only meant to track completion:useEffect(() => { - if (!canGetSecret) { + if (!canGetSecret || isLoaded) { return; }Alternatively, if the intent is to allow re-fetching when
secretNameschange,isLoadedshould be removed from the dependency array entirely—it's not a meaningful trigger.
On the
launchModalsuppression: I see this pattern across the PR for avoiding the max-depth error. For long-term maintainability, theuseOverlayhook should ideally return a stable (memoized) function reference. If that's not feasible right now, auseRefwrapper is a cleaner alternative to eslint-disable:const launchModalRef = useRef(launchModal); launchModalRef.current = launchModal; // then use launchModalRef.current inside the effectThis keeps the linter happy and documents the intent more explicitly than a suppression comment.
frontend/packages/console-shared/src/hooks/useWarningModal.tsx (1)
37-52: Consider documenting memoization expectations.The dependency array at line 50 includes
props, which is an object reference. If callers pass inline objects (e.g.,useWarningModal({ title: 'Title' })), the callback will be recreated on every render. This is fine for most use cases, but callers needing a stable callback reference should memoize their props.Consider adding a note in the JSDoc (around line 30-36) mentioning this behavior, especially since other parts of the codebase may rely on stable callback references for effect dependencies.
📝 Suggested documentation addition
/** * useWarningModal is a hook that provides a way to launch a WarningModal. * Supports two usage patterns: * - Pass props at hook initialization time: useWarningModal({ title: 'Title' }) * - Pass props in the callback: const launch = useWarningModal(); launch({ title: 'Title' }) * - Mix both (overrides have priority): const launch = useWarningModal({ title: 'Title' }); launch({ children: <Content /> }) + * + * Note: If a stable callback reference is required, memoize the props object passed + * to this hook using useMemo to prevent the callback from being recreated on each render. */frontend/packages/knative-plugin/src/components/add/SecretKeySelector.tsx (1)
51-63: ConfirmuseOverlay()provides a stable function or capture it safely.
Line 51–63 suppresses exhaustive-deps and dropslaunchModal. IfuseOverlay()ever returns a new function (e.g., provider change), this effect will keep a stale launcher and may miss error modals. Please confirm stability; if not guaranteed, capture it via a ref while keeping the namespace-only fetch behavior.♻️ Optional safer pattern (ref keeps latest launcher)
-import { useState, useEffect } from 'react'; +import { useState, useEffect, useRef } from 'react'; ... - const launchModal = useOverlay(); + const launchModal = useOverlay(); + const launchModalRef = useRef(launchModal); + useEffect(() => { + launchModalRef.current = launchModal; + }, [launchModal]); ... - if (err?.response?.status !== 403) { - launchModal(ErrorModal, { error: err?.message }); - } + if (err?.response?.status !== 403) { + launchModalRef.current(ErrorModal, { error: err?.message }); + }frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx (2)
42-42: Minor inconsistency in CTA assignment patternLine 34 assigns
cta: startNodeMaintenanceModaldirectly as a function reference, whereas here you wrap withcta: () => stopNodeMaintenanceModalLauncher(). Both work, but the inconsistency is a bit puzzling.If both launchers have the same signature (no-arg functions returning void/Promise), prefer consistency—either direct reference for both or arrow wrappers for both. This aids readability and avoids questions about subtle differences.
♻️ Suggested alignment (if signatures match)
- cta: () => stopNodeMaintenanceModalLauncher(), + cta: stopNodeMaintenanceModalLauncher,
27-28: Guard theuseStopNodeMaintenanceModalhook call or add undefined handling
findNodeMaintenancereturnsundefinedwhen no matching maintenance is found, yetuseStopNodeMaintenanceModal(nodeMaintenance)is invoked unconditionally at line 28. The hook immediately computes modal content by callinggetNodeMaintenanceReason(nodeMaintenance)andgetNodeMaintenanceNodeName(nodeMaintenance)with potentially undefined data. Additionally,getMaintenanceModel(nodeMaintenance)(invoked in theonConfirmcallback) accessesnodeMaintenance.apiVersionwithout checking if it exists.While the launcher is only invoked when
nodeMaintenanceis truthy (line 38), this pattern wastes renders and violates the hook's contract—the type signature doesn't reflect that undefined is acceptable. This can be fragile if the selector functions don't handle undefined gracefully.Either guard the hook call with a conditional or update
useStopNodeMaintenanceModalto explicitly handle and return a no-op launcher whennodeMaintenanceis undefined.frontend/packages/knative-plugin/src/actions/creators.ts (1)
130-130: Unnecessary template literal.Use a plain string literal here since there's no interpolation.
✏️ Suggested fix
- id: `delete-resource`, + id: 'delete-resource',frontend/packages/console-app/src/actions/hooks/useRetryRolloutAction.ts (1)
98-100: Re-enablereact-hooks/exhaustive-depsand includelaunchModalin the dependency array.
useOverlayreturns a stable launcher—launchOverlayis memoized withuseCallback(..., [])inOverlayProvider(line 40), so its identity never changes. The suppression is unnecessary; including it in the deps array will not cause infinite loops. Keeping the lint rule enabled ensures future maintainers catch unintended stale closures.frontend/packages/console-app/src/actions/hooks/useCommonActions.ts (1)
47-85: Avoid suppressing exhaustive-deps; verifyuseOverlaystability.
launchModalis omitted from deps with an eslint-disable. IfuseOverlayever returns a new function, this can capture a stale launcher. Prefer stabilizing the hook output (or memoizing) so the dependency can be included and the suppression removed.Also applies to: 174-176
frontend/packages/console-app/src/actions/hooks/usePVCActions.ts (1)
7-11: Inconsistent lazy-loading for modal providers.
LazyExpandPVCModalProviderandLazyClonePVCModalProviderare lazy-loaded, butDeletePVCModalProvideris imported directly. For consistency and code-splitting benefits, consider using a lazy-loaded variant forDeletePVCModalProvideras well.Does React lazy loading improve bundle size for modals?frontend/public/components/modals/managed-resource-save-modal.tsx (1)
53-54: Consider excludingcloseOverlayfrom prop spread.The
{...props}spread will passcloseOverlayto the innerManagedResourceSaveModal, which doesn't use it (it usescloseinstead). While React ignores unknown props on function components, explicitly destructuring to excludecloseOverlaywould be cleaner:♻️ Suggested refinement
export const ManagedResourceSaveModalProvider: OverlayComponent<ManagedResourceSaveModalProps> = ( props, ) => { + const { closeOverlay, ...rest } = props; return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ManagedResourceSaveModal close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ManagedResourceSaveModal close={closeOverlay} {...rest} /> </ModalWrapper> ); };frontend/public/components/namespace.jsx (1)
878-882: Simplify:pullSecret: undefinedis redundant.Passing
pullSecret: undefinedexplicitly is unnecessary since the prop is already optional inConfigureNamespacePullSecretProps. This adds noise without semantic value.♻️ Suggested simplification
const modal = () => launchModal(LazyConfigureNamespacePullSecretModalProvider, { namespace, - pullSecret: undefined, });frontend/packages/console-app/src/components/modals/resource-limits/index.ts (1)
4-9: The.then(m => ({ default: m.default }))is redundant.Since
ResourceLimitsModalLauncheralready exportsResourceLimitsModalProvideras its default export, the transform is unnecessary.React.lazyexpects a module with adefaultexport, which is already satisfied.♻️ Simplified lazy export
export const LazyResourceLimitsModalProvider = lazy(() => - import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */).then( - (m) => ({ - default: m.default, - }), - ), + import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */), );That said, if this pattern is intentional for consistency across the codebase or to handle potential named-export scenarios in the future, feel free to keep it.
frontend/packages/dev-console/src/actions/context-menu.ts (1)
14-40: Inconsistency:DeleteApplicationActionstill usesi18next.t()directly.This function uses
i18next.t()for translations (line 23), while the newuseDeleteResourceActionhook usesuseTranslation(). SinceDeleteApplicationActionis not a hook, it can't useuseTranslation, but this creates an inconsistency in how translations are handled within the same file.Consider migrating
DeleteApplicationActionto a hook (useDeleteApplicationAction) in a future iteration to align with the hook-based pattern if the broader migration effort supports it.frontend/packages/console-app/src/components/modals/resource-limits/ResourceLimitsModalLauncher.tsx (1)
64-70: Prop spread order could inadvertently overrideclose.The spread
{...props}afterclose={props.closeOverlay}means ifpropssomehow contains acloseproperty, it would override the intended value. WhileOverlayComponentprops shouldn't includeclose(they providecloseOverlay), defensive ordering is safer:♻️ Defensive prop ordering
export const ResourceLimitsModalProvider: OverlayComponent<ResourceLimitsModalLauncherProps> = ( props, ) => ( <ModalWrapper blocking onClose={props.closeOverlay}> - <ResourceLimitsModalLauncher close={props.closeOverlay} {...props} /> + <ResourceLimitsModalLauncher {...props} close={props.closeOverlay} /> </ModalWrapper> );This ensures
closeis always bound tocloseOverlayregardless of what's inprops.frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
317-329: Same prop spread order concern; alsocloseOverlayleaks into child props.Two observations:
Prop ordering: The spread
{...props}after explicitcancelandcloseassignments could override them ifpropscontains those keys.
closeOverlayprop leakage: TheOverlayComponenttype injectscloseOverlayinto props. Spreading{...props}passescloseOverlaytoConfigureNamespacePullSecret, which doesn't expect it. This may cause a React warning about unknown DOM attributes if the prop propagates further.♻️ Destructure to exclude closeOverlay and fix ordering
-export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = ( - props, -) => { +export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = ({ + closeOverlay, + ...restProps +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> + <ModalWrapper blocking onClose={closeOverlay}> <ConfigureNamespacePullSecret - cancel={props.closeOverlay} - close={props.closeOverlay} - {...props} + {...restProps} + cancel={closeOverlay} + close={closeOverlay} /> </ModalWrapper> ); };This prevents
closeOverlayfrom leaking and ensurescancel/closeare always correctly bound.frontend/public/components/modals/index.ts (1)
87-92: Minor formatting inconsistency.
LazyClonePVCModalProviderandLazyRestorePVCModalProvideruse single-line.then((m) => ({ default: m.default }))while the other six lazy providers use multi-line formatting. Consider aligning for consistency and easier diffing across providers.📝 Suggested formatting alignment
// Lazy-loaded OverlayComponent for Clone PVC Modal export const LazyClonePVCModalProvider = lazy(() => import( '@console/app/src/components/modals/clone/clone-pvc-modal' /* webpackChunkName: "clone-pvc-modal" */ - ).then((m) => ({ default: m.default })), + ).then((m) => ({ + default: m.default, + })), );// Lazy-loaded OverlayComponent for Restore PVC Modal export const LazyRestorePVCModalProvider = lazy(() => import( '@console/app/src/components/modals/restore-pvc/restore-pvc-modal' /* webpackChunkName: "restore-pvc-modal" */ - ).then((m) => ({ default: m.default })), + ).then((m) => ({ + default: m.default, + })), );Also applies to: 113-118
frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/utils/error-modal-handler.ts
Outdated
Show resolved
Hide resolved
frontend/public/components/modals/configure-update-strategy-modal.tsx
Outdated
Show resolved
Hide resolved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
3 times, most recently
from
d102d05 to
274b735
Compare
|
/retest |
1 similar comment
|
/retest |
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
from
274b735 to
73300e0
Compare
|
/retest |
|
/assign @yanpzhan Tech debt, so assigning labels |
Great catch, @yanpzhan. This should be fixed. Screen.Recording.2026-03-02.at.10.18.30.AM.mov@logonoff or @vojtechszocs, mind re-tagging after fix? |
| const launcher = useOverlay(); | ||
|
|
||
| useEffect(() => { | ||
| const warningModalLauncher = (props: ControlledWarningModalProps) => { | ||
| launcher<ControlledWarningModalProps>(ControlledWarningModal, props); | ||
| }; | ||
| moduleWarningModalLauncher = warningModalLauncher; | ||
|
|
||
| return () => { | ||
| // Only clear if we're still the active launcher | ||
| if (moduleWarningModalLauncher === warningModalLauncher) { | ||
| moduleWarningModalLauncher = null; |
There was a problem hiding this comment.
why can't we directly use useWarningModal here?
There was a problem hiding this comment.
Claude says,
useWarningModal is a React hook that must be called inside a React component. It uses useOverlay() and useCallback(), which are React hooks.
launchWarningModal is a plain JavaScript function that works in non-React contexts like:
Promise callbacks (.then(), .catch())
Utility functions
Event handlers outside components
Legacy code that isn't inside a React component
There was a problem hiding this comment.
I mean like
const warningModal = useWarningModal();
useEffect(() => {
moduleWarningModalLauncher = warningModal;
return () => {
// Only clear if we're still the active launcher
if (moduleWarningModalLauncher === warningModal) {
moduleWarningModalLauncher = null;
}
};
}, [launcher]);There was a problem hiding this comment.
Ah. Now we understand. Updated. And added a useErrorModal to do the same.
| export const useWarningModalLauncher = (): ((props: ControlledWarningModalProps) => void) => { | ||
| const launcher = useOverlay(); | ||
|
|
||
| return useCallback( | ||
| (props: ControlledWarningModalProps) => { | ||
| launcher<ControlledWarningModalProps>(ControlledWarningModal, props); | ||
| }, | ||
| [launcher], |
There was a problem hiding this comment.
Good catch. Removed/replaced.
| id: 'remove-node-maintenance', | ||
| label: t('metal3-plugin~Stop Maintenance'), | ||
| cta: stopNodeMaintenanceModalLauncher, | ||
| cta: nodeMaintenance ? () => stopNodeMaintenanceModalLauncher(nodeMaintenance) : undefined, |
There was a problem hiding this comment.
does this mean that if nodeMaintenance is falsy this will do nothing? maybe we should hide this action entirely in that case?
There was a problem hiding this comment.
Ternary shouldn't be there at all. Updated.
…e action - Remove useWarningModalLauncher hook (duplicates useWarningModal functionality) - Update tests and mocks to remove useWarningModalLauncher references - Simplify Stop Maintenance action CTA (action is already hidden when nodeMaintenance is falsy) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ooks - Create useErrorModal hook (similar to useWarningModal pattern) - Refactor useSyncErrorModalLauncher to use useErrorModal - Refactor useSyncWarningModalLauncher to use useWarningModal - Remove redundant useErrorModalLauncher hook - Update tests and mocks accordingly This eliminates code duplication and establishes a consistent pattern for both error and warning modal launchers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
|
||
| export const useSyncWarningModalLauncher = jest.fn(); | ||
|
|
||
| export const useWarningModalLauncher = jest.fn(() => mockLaunchWarningModal); |
| * }; | ||
| * ``` | ||
| */ | ||
| export const useErrorModal = ( |
There was a problem hiding this comment.
I think we already have a useErrorModalLauncher too
| export const ControlledWarningModal: OverlayComponent<WarningModalProps> = ({ | ||
| closeOverlay, | ||
| ...props | ||
| }) => { |
There was a problem hiding this comment.
this doesn't need to be exported anymore
| export const ControlledWarningModal: OverlayComponent<WarningModalProps> = ({ | |
| closeOverlay, | |
| ...props | |
| }) => { | |
| const ControlledWarningModal: OverlayComponent<WarningModalProps> = ({ | |
| closeOverlay, | |
| ...props | |
| }) => { |
…lLauncher Enhanced the existing useErrorModalLauncher to support flexible prop merging, eliminating the need for a duplicate useErrorModal hook. This ensures a single source of truth for error modal launching while maintaining the same flexible API pattern used by useWarningModal. Changes: - Enhanced useErrorModalLauncher to accept optional props at initialization and optional overrides at call time, with proper merging of both - Removed duplicate useErrorModal hook from error-modal-handler.tsx - Updated useSyncErrorModalLauncher to use the enhanced useErrorModalLauncher - Removed useErrorModal from mock file to match production code - Added proper TypeScript types for improved type safety This maintains consistency across modal handlers and follows the same pattern established for warning modals. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ControlledWarningModal is an internal implementation detail of useWarningModal and should not be exported. Only the hook and its types should be public API. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto, vojtechszocs 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 |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/assign @yanpzhan |
|
Checked on cluster launched against the pr, regression test about confirm modal pages passed. |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by 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 |
6 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
@rhamilto: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
8 participants
Summary
This PR continues the modal migration effort by removing the deprecated
confirmModal(which usedcreateModalLauncher) and migrating to the modern overlay pattern with an imperativelaunchWarningModal()API.API Design Change: Updated
launchWarningModalto use callback-based API instead of Promises to better align with React patterns and the underlying modal system architecture.Changes
Removed deprecated confirmModal
createModalLauncherpattern usageCreated imperative warning modal API
launchErrorModal()onConfirmandonCancelcallbacksuseOverlayhooktitleIconVariantprop (defaults to 'warning' per PatternFly)Unified modal launcher initialization with race condition protection
SyncErrorModalLauncher→SyncModalLaunchersin error-modal-handler.tsxConsolidated modal hook implementations
useErrorModalhook - consolidated into existinguseErrorModalLauncheruseErrorModalLauncherto support flexible prop merging (initialization + runtime)ControlledWarningModalcomponent private - internal implementation detailMigrated moveNodeToGroup to use launchWarningModal
launchWarningModal()instead ofconfirmModal()<Trans>components with<strong>tags for bold text formattingonErrorparameter for custom error handlinglaunchErrorModal()for error displaytitleIconVariant: nullFixed accessibility violation
Fixed useWarningModal type safety
ControlledWarningModalcomponent private in useWarningModal.tsxOverlayComponenttype instead of manual prop definitioncloseOverlay()on modal dismissUpdated metal3-plugin modals
stopNodeMaintenanceModaltouseStopNodeMaintenanceModalhook in StopNodeMaintenanceModal.tsxnodeMaintenancetype support (K8sResourceKind | undefined)Updated shipwright-plugin
getOrderedStepsFromPod(no references found - 20 lines removed)getDownloadAllLogsCallback(no references found - 111 lines removed)Test infrastructure
useErrorModalmockconsole.errortothrow Errorwhen launcher not initializedArchitecture Benefits
useErrorModalLauncherimplementation (removed duplicateuseErrorModal)SyncModalLauncherscomponent for all imperative modalsTechnical Details
Modal Hook Consolidation
Problem: We had duplicate implementations of error modal hooks:
useErrorModalLauncher(inpublic/components/modals/error-modal.tsx) - original, inflexible APIuseErrorModal(inpackages/console-shared/src/utils/error-modal-handler.tsx) - duplicate with flexible APISolution: Enhanced the existing
useErrorModalLauncherto support the flexible pattern, eliminating duplication:Benefits:
useWarningModaluseErrorModalLauncher(props)()oruseErrorModalLauncher()(props)Callback-Based API Design
The imperative
launchWarningModaluses callbacks instead of Promises to align with React patterns:Why callbacks over Promises?
useWarningModalhook acceptsonConfirm/onClosecallbacksWarningModalcomponent uses callback props, not PromisescloseOverlayis a callback, not a PromisemoveNodeToGroupdoes)API signature:
Example usage:
Optional Warning Icon
The
ControlledWarningModalwrapper passes through thetitleIconVariantprop, allowing callers to:titleIconVariant(PatternFly defaults to'warning')titleIconVariant: nulltitleIconVariant: 'danger' | 'success' | 'info'or a custom iconExample usage in moveNodeToGroup.tsx:
Race Condition Protection
Problem: If components mount/unmount out of order, the cleanup could clear a newer launcher.
Solution: Reference-based cleanup in both launchers (error-modal-handler.tsx, warning-modal-handler.tsx):
This prevents race conditions where unmounting could clear a newer launcher that was set by a remounting component.
Accessibility Fix - aria-hidden Focus Violation
Problem: When the confirmation modal opened during drag-drop operations, focus remained on an SVG
<g>element in the topology graph. PatternFly's Modal setsaria-hidden="true"on the #app div, creating an accessibility violation.Browser Warning (Fixed):
Solution: Blur the active element before launching the modal (moveNodeToGroup.tsx):
Test Plan
Automated Tests ✅
Manual Testing Checklist
Breaking Changes
None - this is a refactoring that maintains the same functionality.
Related
🤖 Generated with Claude Code