[WIP] CONSOLE-5012: Migrate confirmModal to overlay pattern #15948
Conversation
Migrates PVC-related modals (expand, clone, delete, restore) from the legacy createModalLauncher pattern to the modern OverlayComponent pattern with lazy loading. Changes include: - Convert modal exports to OverlayComponent providers - Implement lazy loading for modal components - Update action hooks to use useOverlay() and launchModal() - Set react-modal app element globally in App component - Maintain backward compatibility with existing modal exports Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit completes the migration from createModalLauncher to the OverlayComponent pattern for delete modals and related actions. Changes: - Refactored delete-modal.tsx to use OverlayComponent pattern - Added LazyDeleteModalProvider with React.lazy() for code splitting - Converted action creators to hooks to comply with React rules: * DeleteResourceAction → useDeleteResourceAction (context-menu.ts) * deleteKnativeServiceResource → useDeleteKnativeServiceResource (creators.ts) - Updated all consumers to use new hook-based actions: * useCommonActions - uses LazyDeleteModalProvider with launchModal * deployment-provider - uses useDeleteResourceAction hook * deploymentconfig-provider - uses useDeleteResourceAction hook * knative-plugin providers - uses useDeleteKnativeServiceResource hook - Updated public/components/modals/index.ts with lazy export - Eliminated all deleteModal() function calls Note: DeleteApplicationAction remains as a non-hook function creator since it doesn't use modals and doesn't need to comply with hooks rules. This change maintains backward compatibility through default exports and preserves lazy loading functionality. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove launchModal from dependency arrays in pre-existing hooks to prevent infinite loops. These files were not modified as part of the modal migration work, but had the same issue. 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>
This commit migrates the ConfigureUnschedulableModal from the legacy createModalLauncher pattern to the new OverlayComponent pattern. Changes: - Exported ConfigureUnschedulableModalProvider as OverlayComponent - Updated modals/index.ts to lazy-load the modal provider - Modified useNodeActions hook to use useOverlay hook - Removed createModalLauncher usage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add error-modal-handler.ts to console-shared package with: - launchGlobalErrorModal() for launching modals from non-React contexts - useSetupGlobalErrorModalLauncher() hook for initialization - setGlobalErrorModalLauncher() for managing global state - Migrate all packages to use shared launcher: - topology: Update Topology.tsx, componentUtils.ts, edgeActions.ts - knative-plugin: Update knativeComponentUtils.ts, create-connector-utils.ts - shipwright-plugin: Update logs-utils.ts, BuildRunDetailsPage.tsx - dev-console: Update pipeline-template-utils.ts, ImportPage.tsx, DeployImagePage.tsx - Initialize launcher in root components: - topology: Topology.tsx (for topology drag/drop contexts) - shipwright-plugin: BuildRunDetailsPage.tsx (for standalone log viewing) - dev-console: ImportPage.tsx, DeployImagePage.tsx (for import flows) - Remove topology-specific errorModalHandler implementation - Remove deprecated errorModal function from public/components/modals This consolidates error modal launching into a single reusable utility, eliminating code duplication across packages and ensuring error modals work correctly in all contexts (topology, standalone pages, import flows). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
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>
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
|
[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
|
@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
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
| export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> |
There was a problem hiding this comment.
Prevent closeOverlay from being overridden by spread props.
Same pattern as the PVC delete modal—spread props should come first so closeOverlay always wins.
✅ Suggested fix
- <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} />
+ <ClonePVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} />📝 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 ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ClonePVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} /> | |
| </ModalWrapper> |
🤖 Prompt for AI Agents
In
`@frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx`
around lines 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.
| export const RestorePVCModalProvider: OverlayComponent<RestorePVCModalProps> = (props) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <RestorePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> | ||
| ); |
There was a problem hiding this comment.
Ensure closeOverlay can’t be overridden by caller props.
...props is currently spread after cancel/close, so a caller-provided cancel/close could override closeOverlay and leave the overlay open. Prefer extracting closeOverlay and spreading the rest first.
🔧 Suggested fix
-export const RestorePVCModalProvider: OverlayComponent<RestorePVCModalProps> = (props) => {
- return (
- <ModalWrapper blocking onClose={props.closeOverlay}>
- <RestorePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} />
- </ModalWrapper>
- );
-};
+export const RestorePVCModalProvider: OverlayComponent<RestorePVCModalProps> = ({
+ closeOverlay,
+ ...props
+}) => {
+ return (
+ <ModalWrapper blocking onClose={closeOverlay}>
+ <RestorePVCModal {...props} cancel={closeOverlay} close={closeOverlay} />
+ </ModalWrapper>
+ );
+};📝 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 RestorePVCModalProvider: OverlayComponent<RestorePVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <RestorePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| ); | |
| export const RestorePVCModalProvider: OverlayComponent<RestorePVCModalProps> = ({ | |
| closeOverlay, | |
| ...props | |
| }) => { | |
| return ( | |
| <ModalWrapper blocking onClose={closeOverlay}> | |
| <RestorePVCModal {...props} cancel={closeOverlay} close={closeOverlay} /> | |
| </ModalWrapper> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In
`@frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx`
around lines 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.
| export const ConfigureUnschedulableModalProvider: OverlayComponent<ConfigureUnschedulableModalProps> = ( | ||
| props, | ||
| ) => ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <ConfigureUnschedulableModal | ||
| cancel={props.closeOverlay} | ||
| close={props.closeOverlay} | ||
| {...props} | ||
| /> | ||
| </ModalWrapper> | ||
| ); | ||
|
|
||
| // For backward compatibility with the index.ts re-export | ||
| export default ConfigureUnschedulableModalProvider; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and its location
find . -name "ConfigureUnschedulableModal.tsx" -type fRepository: openshift/console
Length of output: 152
🏁 Script executed:
# Check the type definition for ConfigureUnschedulableModalProps
rg -n "ConfigureUnschedulableModalProps" -A 5 --type ts --type tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for all usage of ConfigureUnschedulableModalProvider
rg -n "ConfigureUnschedulableModalProvider" -B 2 -A 2 --type ts --type tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for ConfigureUnschedulableModalProps type definition without file type filter
rg -n "ConfigureUnschedulableModalProps" -A 5Repository: openshift/console
Length of output: 2073
🏁 Script executed:
# Read the file to understand the full context
cat -n ./frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsxRepository: openshift/console
Length of output: 2537
🏁 Script executed:
# Find ModalComponentProps type definition
rg -n "type ModalComponentProps" -A 10Repository: openshift/console
Length of output: 817
🏁 Script executed:
# Search for usage of ConfigureUnschedulableModalProvider
rg -n "ConfigureUnschedulableModalProvider\|ConfigureUnschedulableModal" --type-list | head -20Repository: openshift/console
Length of output: 450
🏁 Script executed:
# Broader search for ConfigureUnschedulableModalProvider usage
rg "ConfigureUnschedulableModalProvider\|LazyConfigureUnschedulableModalProvider" -B 3 -A 3Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Search for files that import or reference ConfigureUnschedulableModal or the provider
rg "ConfigureUnschedulableModal" -lRepository: openshift/console
Length of output: 285
🏁 Script executed:
# Check index.ts for the re-export mentioned in the comment
find . -path "*/nodes/modals/index.ts" -type fRepository: openshift/console
Length of output: 129
🏁 Script executed:
# Look for where LazyConfigureUnschedulableModalProvider might be defined or used
rg "LazyConfigureUnschedulableModalProvider\|lazy.*ConfigureUnschedulableModal" -B 2 -A 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check the index.ts re-export
cat -n ./frontend/packages/console-app/src/components/nodes/modals/index.tsRepository: openshift/console
Length of output: 448
🏁 Script executed:
# Check menu-actions.tsx to see how ConfigureUnschedulableModalProvider is used
cat -n ./frontend/packages/console-app/src/components/nodes/menu-actions.tsxRepository: openshift/console
Length of output: 4208
🏁 Script executed:
# Check OverlayComponent type to understand the contract
rg "type OverlayComponent\|interface OverlayComponent" -A 5Repository: openshift/console
Length of output: 43
Prevent cancel and close from being overridden by caller props.
Currently, spreading props after setting cancel and close allows a caller-supplied value to override closeOverlay, leaving the overlay open. While the current call site only passes { resource: obj }, destructuring first and then spreading followed by explicit cancel/close assignment is a safer defensive pattern.
🛠️ Suggested fix
-export const ConfigureUnschedulableModalProvider: OverlayComponent<ConfigureUnschedulableModalProps> = (
- props,
-) => (
- <ModalWrapper blocking onClose={props.closeOverlay}>
- <ConfigureUnschedulableModal
- cancel={props.closeOverlay}
- close={props.closeOverlay}
- {...props}
- />
- </ModalWrapper>
-);
+export const ConfigureUnschedulableModalProvider: OverlayComponent<ConfigureUnschedulableModalProps> = (
+ props,
+) => {
+ const { closeOverlay, ...rest } = props;
+ return (
+ <ModalWrapper blocking onClose={closeOverlay}>
+ <ConfigureUnschedulableModal {...rest} cancel={closeOverlay} close={closeOverlay} />
+ </ModalWrapper>
+ );
+};🤖 Prompt for AI Agents
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`
around lines 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.
| export const useSetupGlobalErrorModalLauncher = () => { | ||
| const launcher = useOverlay(); | ||
|
|
||
| useEffect(() => { | ||
| const errorModalLauncher = (props: ErrorModalProps) => { | ||
| launcher<ErrorModalProps>(ErrorModal, props); | ||
| }; | ||
|
|
||
| setGlobalErrorModalLauncher(errorModalLauncher); | ||
|
|
||
| return () => { | ||
| setGlobalErrorModalLauncher(null); | ||
| }; | ||
| // Intentionally only run once on mount | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
Avoid clearing a newer global launcher on unmount.
If multiple roots call the setup hook, the current cleanup will null the launcher even when another root has replaced it, leading to silent failures in non-React contexts. Guard the cleanup so it only clears the launcher it set.
🐛 Proposed fix (guarded cleanup)
useEffect(() => {
const errorModalLauncher = (props: ErrorModalProps) => {
launcher<ErrorModalProps>(ErrorModal, props);
};
setGlobalErrorModalLauncher(errorModalLauncher);
return () => {
- setGlobalErrorModalLauncher(null);
+ if (globalErrorModalLauncher === errorModalLauncher) {
+ setGlobalErrorModalLauncher(null);
+ }
};
// Intentionally only run once on mount
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);🤖 Prompt for AI Agents
In `@frontend/packages/console-shared/src/utils/error-modal-handler.ts` around
lines 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.
| import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; | ||
| import { LazyDeleteModalProvider } from '@console/internal/components/modals'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for useOverlay exports in the SDK
rg -n "export.*useOverlay" --type=ts --type=tsx frontend/packages/console-dynamic-plugin-sdk/src/Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check the main entry point of the SDK to see what's publicly exported
find frontend/packages/console-dynamic-plugin-sdk/src -name "index.ts" -o -name "index.tsx" | head -5Repository: openshift/console
Length of output: 413
🏁 Script executed:
# Check the structure around the useOverlay file
find frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support -type f | head -20Repository: openshift/console
Length of output: 392
🏁 Script executed:
# Check app/index.ts for modal-support exports
cat frontend/packages/console-dynamic-plugin-sdk/src/app/index.tsRepository: openshift/console
Length of output: 238
🏁 Script executed:
# Check if there's a root index.ts in the SDK
cat frontend/packages/console-dynamic-plugin-sdk/src/index.tsRepository: openshift/console
Length of output: 457
🏁 Script executed:
# Check if useOverlay is exported from modal-support directory itself
ls -la frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/Repository: openshift/console
Length of output: 432
🏁 Script executed:
# Check if there's an index.ts in modal-support
ls -la frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/index.ts 2>&1Repository: openshift/console
Length of output: 183
🏁 Script executed:
# Search for other usages of useOverlay across the codebase
rg "from.*modal-support.*useOverlay|from.*dynamic-plugin-sdk.*useOverlay" --type=tsRepository: openshift/console
Length of output: 7979
🏁 Script executed:
# Check if dynamic-core-api.ts is exported from the root SDK index
cat frontend/packages/console-dynamic-plugin-sdk/src/index.ts | grep -A5 -B5 "dynamic-core-api"Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Check what else is in dynamic-core-api.ts
head -50 frontend/packages/console-dynamic-plugin-sdk/src/api/dynamic-core-api.tsRepository: openshift/console
Length of output: 1546
🏁 Script executed:
# Get the full dynamic-core-api.ts file to see all exports
cat frontend/packages/console-dynamic-plugin-sdk/src/api/dynamic-core-api.tsRepository: openshift/console
Length of output: 44736
🏁 Script executed:
# Check root index.ts for any reference to api/ exports
grep -n "api\|dynamic-core" frontend/packages/console-dynamic-plugin-sdk/src/index.tsRepository: openshift/console
Length of output: 145
Expose useOverlay through the SDK's public API surface.
The current import reaches into the SDK's internal src/app/modal-support/ path. While useOverlay is exported by dynamic-core-api.ts, that module is not re-exported from the SDK's public API entry point (@console/dynamic-plugin-sdk), making the internal path the only viable import option. This pattern affects 41+ files across the codebase.
To ensure forward compatibility, either:
- Add
export * from './modal-support'tofrontend/packages/console-dynamic-plugin-sdk/src/app/index.ts, or - Export
dynamic-core-api.tsfrom the root SDK index
This will allow importing as import { useOverlay } from '@console/dynamic-plugin-sdk' instead of the internal path.
🤖 Prompt for AI Agents
In `@frontend/packages/knative-plugin/src/actions/creators.ts` around lines 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'.
| export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ( | ||
| props, | ||
| ) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <ConfigureUpdateStrategyModal | ||
| cancel={props.closeOverlay} | ||
| close={props.closeOverlay} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
Ensure closeOverlay can’t be overridden by caller props.
...props is spread after cancel/close, so a caller-provided cancel/close could override closeOverlay. Recommend spreading first and then enforcing the overlay handlers.
🔧 Suggested fix
-export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = (
- props,
-) => {
- return (
- <ModalWrapper blocking onClose={props.closeOverlay}>
- <ConfigureUpdateStrategyModal
- cancel={props.closeOverlay}
- close={props.closeOverlay}
- {...props}
- />
- </ModalWrapper>
- );
-};
+export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ({
+ closeOverlay,
+ ...props
+}) => {
+ return (
+ <ModalWrapper blocking onClose={closeOverlay}>
+ <ConfigureUpdateStrategyModal {...props} cancel={closeOverlay} close={closeOverlay} />
+ </ModalWrapper>
+ );
+};🤖 Prompt for AI Agents
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx` around
lines 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.
| export const DeleteModalProvider: OverlayComponent<DeleteModalProps> = (props) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <DeleteModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> | ||
| ); |
There was a problem hiding this comment.
Ensure closeOverlay can’t be overridden by caller props.
...props is spread after cancel/close, so a caller-provided cancel/close could override closeOverlay. Safer to spread first and then enforce the overlay handlers.
🔧 Suggested fix
-export const DeleteModalProvider: OverlayComponent<DeleteModalProps> = (props) => {
- return (
- <ModalWrapper blocking onClose={props.closeOverlay}>
- <DeleteModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} />
- </ModalWrapper>
- );
-};
+export const DeleteModalProvider: OverlayComponent<DeleteModalProps> = ({
+ closeOverlay,
+ ...props
+}) => {
+ return (
+ <ModalWrapper blocking onClose={closeOverlay}>
+ <DeleteModal {...props} cancel={closeOverlay} close={closeOverlay} />
+ </ModalWrapper>
+ );
+};📝 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 DeleteModalProvider: OverlayComponent<DeleteModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <DeleteModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| ); | |
| export const DeleteModalProvider: OverlayComponent<DeleteModalProps> = ({ | |
| closeOverlay, | |
| ...props | |
| }) => { | |
| return ( | |
| <ModalWrapper blocking onClose={closeOverlay}> | |
| <DeleteModal {...props} cancel={closeOverlay} close={closeOverlay} /> | |
| </ModalWrapper> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/public/components/modals/delete-modal.tsx` around lines 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).
| export const DeletePVCModalProvider: OverlayComponent<DeletePVCModalProps> = (props) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <DeletePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> |
There was a problem hiding this comment.
Prevent closeOverlay from being overridden by spread props.
Spreading {...props} last allows any close/cancel passed in to override closeOverlay, which can break overlay teardown. Prefer spreading first, then overriding with closeOverlay.
✅ Suggested fix
- <DeletePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} />
+ <DeletePVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} />📝 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 DeletePVCModalProvider: OverlayComponent<DeletePVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <DeletePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| export const DeletePVCModalProvider: OverlayComponent<DeletePVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <DeletePVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} /> | |
| </ModalWrapper> |
🤖 Prompt for AI Agents
In `@frontend/public/components/modals/delete-pvc-modal.tsx` around lines 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.
|
@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. |
2 participants
Summary
This PR migrates all instances of
confirmModalto use the modern overlay pattern withuseWarningModal, completing the modal migration effort.Changes
confirmModallauncher andconfirm-modal.tsxfilemoveNodeToGroupto useuseWarningModalwith proper cancel handlingStopNodeMaintenanceModalto remove deprecated imperative APIuseWarningModalto properly handlecloseOverlaypropTechnical Details
Topology Move Node Migration
The topology
moveNodeToGroupfunction usedconfirmModalimperatively. Since it's not a React component, I implemented a global handler pattern similar to the existing error handler:setMoveNodeToGroupConfirmHandlerto set a global confirmation handleruseSetupMoveNodeToGroupHandlershook that usesuseWarningModalStopNodeMaintenanceModal Migration
stopNodeMaintenanceModalfunctionuseStopNodeMaintenanceModalhook that already useduseWarningModaluseWarningModal Fix
closeOverlayprop warning by properly excluding it from the props spreadcloseOverlay()calls in bothonCloseandonConfirmhandlers to properly remove the modal from DOMDependencies
This PR builds upon and depends on:
Test Plan
closeOverlayprop🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.