From c1664b0f98d481adadb1d17da8fa674635afbc86 Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Tue, 17 Feb 2026 15:06:51 -0500 Subject: [PATCH 1/7] CONSOLE-5012: Create shared error modal launcher using React Context - Add error-modal-handler.tsx to console-shared package with: - SyncErrorModalLauncher - Zero-render component that syncs launcher for non-React contexts - useErrorModalLauncher() - Hook for launching error modals from React components - launchErrorModal() - Function for launching from non-React contexts (callbacks, promises) - Add SyncErrorModalLauncher to app.tsx inside OverlayProvider - Single initialization point for the entire application - Reuses existing OverlayProvider instead of creating new context - Migrate packages to use launchErrorModal: - topology: componentUtils.ts, edgeActions.ts - knative-plugin: knativeComponentUtils.ts, create-connector-utils.ts - shipwright-plugin: logs-utils.ts - dev-console: pipeline-template-utils.ts - Update moveNodeToGroup.tsx to support error handling: - Add setMoveNodeToGroupErrorHandler() for managing error handler - Add useSetupMoveNodeToGroupErrorHandler() hook for Topology component - Update moveNodeToGroup() to accept optional onError callback - Deprecate useMoveNodeToGroup() hook - Add comprehensive test coverage: - error-modal-handler.spec.tsx - 5 unit tests covering all scenarios - __mocks__/errorModalHandler.ts - Global mock for testing - Fixed useWarningModal to properly call closeOverlay() on modal close/confirm Co-Authored-By: Claude Sonnet 4.5 --- .../src/hooks/useWarningModal.tsx | 9 +- .../utils/__mocks__/error-modal-handler.tsx | 10 +- .../utils/__mocks__/warning-modal-handler.tsx | 12 ++ .../__tests__/error-modal-handler.spec.tsx | 39 ++--- .../src/utils/error-modal-handler.tsx | 53 ++++--- .../src/utils/warning-modal-handler.tsx | 114 +++++++++++++++ .../StartingMaintenancePopoverContent.tsx | 5 +- .../UnderMaintenancePopoverContent.tsx | 5 +- .../src/components/maintenance/actions.tsx | 12 +- .../modals/StopNodeMaintenanceModal.tsx | 31 +--- .../locales/en/shipwright-plugin.json | 1 - .../src/components/logs/logs-utils.ts | 136 +----------------- .../topology/src/utils/moveNodeToGroup.tsx | 62 ++++---- frontend/public/components/app.tsx | 4 +- .../components/modals/confirm-modal.tsx | 72 ---------- frontend/public/components/modals/index.ts | 6 - frontend/public/locales/en/public.json | 1 - 17 files changed, 241 insertions(+), 331 deletions(-) create mode 100644 frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx create mode 100644 frontend/packages/console-shared/src/utils/warning-modal-handler.tsx delete mode 100644 frontend/public/components/modals/confirm-modal.tsx diff --git a/frontend/packages/console-shared/src/hooks/useWarningModal.tsx b/frontend/packages/console-shared/src/hooks/useWarningModal.tsx index 7e87c92821e..a6e5e60d269 100644 --- a/frontend/packages/console-shared/src/hooks/useWarningModal.tsx +++ b/frontend/packages/console-shared/src/hooks/useWarningModal.tsx @@ -1,23 +1,28 @@ -import type { FC } from 'react'; import { useState, useCallback } from 'react'; import type { WarningModalProps } from '@patternfly/react-component-groups'; import { WarningModal } from '@patternfly/react-component-groups'; +import type { OverlayComponent } from '@console/dynamic-plugin-sdk/src/app/modal-support/OverlayProvider'; import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; /** * ControlledWarningModal is a wrapper around WarningModal that manages its open state. */ -const ControlledWarningModal: FC = (props) => { +export const ControlledWarningModal: OverlayComponent = ({ + closeOverlay, + ...props +}) => { const [isOpen, setIsOpen] = useState(true); const onClose: WarningModalProps['onClose'] = (e) => { setIsOpen(false); props.onClose?.(e); + closeOverlay?.(); }; const onConfirm: WarningModalProps['onConfirm'] = () => { setIsOpen(false); props.onConfirm?.(); + closeOverlay?.(); }; return ; diff --git a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx index 3c92f4646f8..fed53fcb047 100644 --- a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx @@ -1,10 +1,18 @@ /** * Mock implementation of error-modal-handler for Jest tests + * + * Note: This mock provides a simplified SyncModalLaunchers that doesn't call + * useSyncWarningModalLauncher. Tests that need warning modal functionality + * should explicitly mock warning-modal-handler or use the real implementation. */ export const mockLaunchErrorModal = jest.fn(); -export const SyncErrorModalLauncher = () => null; +export const useSyncErrorModalLauncher = jest.fn(); + +// Simplified component that doesn't sync warning modals +// Tests needing both error and warning modals should not use this mock +export const SyncModalLaunchers = () => null; export const useErrorModalLauncher = jest.fn(() => mockLaunchErrorModal); diff --git a/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx new file mode 100644 index 00000000000..ed2c2f14393 --- /dev/null +++ b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx @@ -0,0 +1,12 @@ +/** + * Mock for warning-modal-handler + * Used in Jest tests to avoid rendering actual modals + */ + +export const mockLaunchWarningModal = jest.fn(() => Promise.resolve()); + +export const useSyncWarningModalLauncher = jest.fn(); + +export const useWarningModalLauncher = jest.fn(() => mockLaunchWarningModal); + +export const launchWarningModal = mockLaunchWarningModal; diff --git a/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx b/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx index 25e452acdef..4965ee42445 100644 --- a/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx +++ b/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx @@ -1,6 +1,6 @@ import { render } from '@testing-library/react'; import { - SyncErrorModalLauncher, + SyncModalLaunchers, useErrorModalLauncher, launchErrorModal, } from '../error-modal-handler'; @@ -16,9 +16,9 @@ describe('error-modal-handler', () => { jest.clearAllMocks(); }); - describe('SyncErrorModalLauncher', () => { + describe('SyncModalLaunchers', () => { it('should sync the launcher on mount', () => { - render(); + render(); // Call the module-level function launchErrorModal({ error: 'Test error', title: 'Test' }); @@ -31,20 +31,14 @@ describe('error-modal-handler', () => { }); it('should cleanup launcher on unmount', () => { - const { unmount } = render(); + const { unmount } = render(); unmount(); - // Should log error instead of crashing - const consoleError = jest.spyOn(console, 'error').mockImplementation(); - launchErrorModal({ error: 'Test error' }); - - expect(consoleError).toHaveBeenCalledWith( - expect.stringContaining('Error modal launcher not initialized'), - expect.any(Object), - ); - - consoleError.mockRestore(); + // Should throw error when launcher is not initialized + expect(() => { + launchErrorModal({ error: 'Test error' }); + }).toThrow('Error modal launcher not initialized'); }); }); @@ -70,7 +64,7 @@ describe('error-modal-handler', () => { describe('launchErrorModal', () => { it('should launch error modal when launcher is initialized', () => { - render(); + render(); launchErrorModal({ error: 'Connection failed', @@ -83,17 +77,10 @@ describe('error-modal-handler', () => { }); }); - it('should log error when launcher is not initialized', () => { - const consoleError = jest.spyOn(console, 'error').mockImplementation(); - - launchErrorModal({ error: 'Test error' }); - - expect(consoleError).toHaveBeenCalledWith( - expect.stringContaining('Error modal launcher not initialized'), - { error: 'Test error' }, - ); - - consoleError.mockRestore(); + it('should throw error when launcher is not initialized', () => { + expect(() => { + launchErrorModal({ error: 'Test error' }); + }).toThrow('Error modal launcher not initialized'); }); }); }); diff --git a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx index 7b52fdc6370..9d6c48607cb 100644 --- a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx @@ -2,38 +2,51 @@ import { useCallback, useEffect } from 'react'; import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; import type { ErrorModalProps } from '@console/internal/components/modals/error-modal'; import { ErrorModal } from '@console/internal/components/modals/error-modal'; +import { useSyncWarningModalLauncher } from './warning-modal-handler'; // Module-level reference for non-React contexts -// This is populated by SyncErrorModalLauncher and should not be set directly +// This is populated by useSyncErrorModalLauncher and should not be set directly let moduleErrorModalLauncher: ((props: ErrorModalProps) => void) | null = null; /** - * Component that syncs the error modal launcher to module-level for non-React contexts. - * This should be mounted once in the app root, after OverlayProvider. - * - * @example - * ```tsx - * const App = () => ( - * - * - * - * - * ); - * ``` + * Hook that syncs the error modal launcher to module-level for non-React contexts. + * Use SyncModalLaunchers component instead of calling this directly. */ -export const SyncErrorModalLauncher = () => { +export const useSyncErrorModalLauncher = () => { const launcher = useOverlay(); useEffect(() => { - moduleErrorModalLauncher = (props: ErrorModalProps) => { + const errorModalLauncher = (props: ErrorModalProps) => { launcher(ErrorModal, props); }; + moduleErrorModalLauncher = errorModalLauncher; return () => { - moduleErrorModalLauncher = null; + // Only clear if we're still the active launcher + if (moduleErrorModalLauncher === errorModalLauncher) { + moduleErrorModalLauncher = null; + } }; }, [launcher]); +}; +/** + * Component that syncs both error and warning modal launchers to module-level for non-React contexts. + * This should be mounted once in the app root, after OverlayProvider. + * + * @example + * ```tsx + * const App = () => ( + * + * + * + * + * ); + * ``` + */ +export const SyncModalLaunchers = () => { + useSyncErrorModalLauncher(); + useSyncWarningModalLauncher(); return null; }; @@ -70,7 +83,7 @@ export const useErrorModalLauncher = (): ((props: ErrorModalProps) => void) => { /** * Launch an error modal from non-React contexts (callbacks, promises, utilities). - * The SyncErrorModalLauncher component must be mounted in the app root. + * The SyncModalLaunchers component must be mounted in the app root. * * @deprecated Use React component modals within component code instead. * For new code, write modals directly within React components using useOverlay or other modal patterns. @@ -91,10 +104,8 @@ export const launchErrorModal = (props: ErrorModalProps): void => { if (moduleErrorModalLauncher) { moduleErrorModalLauncher(props); } else { - // eslint-disable-next-line no-console - console.error( - 'Error modal launcher not initialized. Ensure SyncErrorModalLauncher is mounted after OverlayProvider.', - props, + throw new Error( + 'Error modal launcher not initialized. Ensure SyncModalLaunchers is mounted after OverlayProvider.', ); } }; diff --git a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx new file mode 100644 index 00000000000..cd673579f0f --- /dev/null +++ b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx @@ -0,0 +1,114 @@ +import { useCallback, useEffect } from 'react'; +import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; +import type { ControlledWarningModalProps } from '../hooks/useWarningModal'; +import { ControlledWarningModal } from '../hooks/useWarningModal'; + +// Module-level reference for non-React contexts +// This is populated by useSyncWarningModalLauncher and should not be set directly +let moduleWarningModalLauncher: ((props: ControlledWarningModalProps) => void) | null = null; + +/** + * Hook that syncs the warning modal launcher to module-level for non-React contexts. + * This should be called once in the app root, after OverlayProvider. + * Use SyncModalLaunchers component from error-modal-handler instead of calling this directly. + */ +export const useSyncWarningModalLauncher = () => { + const launcher = useOverlay(); + + useEffect(() => { + const warningModalLauncher = (props: ControlledWarningModalProps) => { + launcher(ControlledWarningModal, props); + }; + moduleWarningModalLauncher = warningModalLauncher; + + return () => { + // Only clear if we're still the active launcher + if (moduleWarningModalLauncher === warningModalLauncher) { + moduleWarningModalLauncher = null; + } + }; + }, [launcher]); +}; + +/** + * Hook to launch warning modals from React components. + * Must be used within an OverlayProvider. + * Use `useWarningModal` instead for better React integration. + * + * @example + * ```tsx + * const MyComponent = () => { + * const launchWarningModal = useWarningModalLauncher(); + * + * const handleWarning = () => { + * launchWarningModal({ + * title: 'Are you sure?', + * children: 'This action cannot be undone.', + * confirmButtonLabel: 'Continue', + * onConfirm: () => console.log('Confirmed'), + * }); + * }; + * + * // ... + * }; + * ``` + */ +export const useWarningModalLauncher = (): ((props: ControlledWarningModalProps) => void) => { + const launcher = useOverlay(); + + return useCallback( + (props: ControlledWarningModalProps) => { + launcher(ControlledWarningModal, props); + }, + [launcher], + ); +}; + +/** + * Launch a warning modal from non-React contexts (callbacks, promises, utilities). + * The SyncWarningModalLauncher component must be mounted in the app root. + * + * @deprecated Use React component modals within component code instead. + * For new code, write modals directly within React components using useWarningModal. + * This function should only be used for legacy non-React contexts like promise callbacks. + * + * @param props - Warning modal properties (title, children, confirmButtonLabel, etc.) + * @returns Promise that resolves (undefined) when confirmed, rejects with Error('User cancelled') when canceled/closed + * + * @example + * ```tsx + * // In a promise callback or utility function + * launchWarningModal({ + * title: 'Delete Resource', + * children: 'Are you sure you want to delete this resource?', + * confirmButtonLabel: 'Delete', + * }).then(() => { + * // User confirmed - proceed with action + * deleteResource(); + * }).catch((error) => { + * // User canceled - error.message === 'User cancelled' + * console.log('Action cancelled by user'); + * }); + * ``` + */ +export const launchWarningModal = ( + props: Omit, +): Promise => { + return new Promise((resolve, reject) => { + if (moduleWarningModalLauncher) { + moduleWarningModalLauncher({ + ...props, + onConfirm: () => { + resolve(); + }, + onClose: () => { + reject(new Error('User cancelled')); + }, + }); + } else { + throw new Error( + 'Warning modal launcher not initialized. Ensure SyncWarningModalLauncher is mounted after OverlayProvider.', + ); + } + }); +}; diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx index 75580611700..ff6f436ff03 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx @@ -20,7 +20,7 @@ import { getNodeMaintenanceLastError, getNodeMaintenancePendingPods, } from '../../selectors'; -import stopNodeMaintenanceModal from '../modals/StopNodeMaintenanceModal'; +import { useStopNodeMaintenanceModal } from '../modals/StopNodeMaintenanceModal'; import MaintenancePopoverPodList from './MaintenancePopoverPodList'; type StartingMaintenancePopoverContentProps = { @@ -31,6 +31,7 @@ const StartingMaintenancePopoverContent: FC { const { t } = useTranslation(); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); const reason = getNodeMaintenanceReason(nodeMaintenance); const creationTimestamp = getNodeMaintenanceCreationTimestamp(nodeMaintenance); const lastError = getNodeMaintenanceLastError(nodeMaintenance); @@ -78,7 +79,7 @@ const StartingMaintenancePopoverContent: FC
- diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx index c19b66070d4..43d659ca3fe 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx @@ -10,7 +10,7 @@ import { useTranslation } from 'react-i18next'; import type { K8sResourceKind } from '@console/internal/module/k8s'; import { Timestamp } from '@console/shared/src/components/datetime/Timestamp'; import { getNodeMaintenanceReason, getNodeMaintenanceCreationTimestamp } from '../../selectors'; -import stopNodeMaintenanceModal from '../modals/StopNodeMaintenanceModal'; +import { useStopNodeMaintenanceModal } from '../modals/StopNodeMaintenanceModal'; type UnderMaintenancePopoverContentProps = { nodeMaintenance: K8sResourceKind; @@ -20,6 +20,7 @@ const UnderMaintenancePopoverContent: FC = nodeMaintenance, }) => { const { t } = useTranslation(); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); const reason = getNodeMaintenanceReason(nodeMaintenance); const creationTimestamp = getNodeMaintenanceCreationTimestamp(nodeMaintenance); @@ -43,7 +44,7 @@ const UnderMaintenancePopoverContent: FC =
- diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx index 9e4ff59176d..8ffc3bd0411 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx @@ -6,7 +6,7 @@ import type { NodeKind } from '@console/internal/module/k8s'; import { useMaintenanceCapability } from '../../hooks/useMaintenanceCapability'; import { findNodeMaintenance } from '../../selectors'; import { useStartNodeMaintenanceModalLauncher } from '../modals/StartNodeMaintenanceModal'; -import stopNodeMaintenanceModal from '../modals/StopNodeMaintenanceModal'; +import { useStopNodeMaintenanceModal } from '../modals/StopNodeMaintenanceModal'; export const useNodeMaintenanceActions: ExtensionHook = (resource) => { const { t } = useTranslation(); @@ -24,9 +24,10 @@ export const useNodeMaintenanceActions: ExtensionHook = (res namespaced: false, }); - const actions = useMemo(() => { - const nodeMaintenance = findNodeMaintenance(maintenances, resource.metadata.name); + const nodeMaintenance = findNodeMaintenance(maintenances, resource.metadata.name); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); + const actions = useMemo(() => { let action: Action = { id: 'start-node-maintenance', label: t('metal3-plugin~Start Maintenance'), @@ -38,13 +39,12 @@ export const useNodeMaintenanceActions: ExtensionHook = (res action = { id: 'stop-node-maintenance', label: t('metal3-plugin~Stop Maintenance'), - cta: () => stopNodeMaintenanceModal(nodeMaintenance, t), + cta: () => stopNodeMaintenanceModalLauncher(), insertBefore: 'edit-labels', }; } return [action]; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [maintenances, resource.metadata.name, t]); + }, [t, nodeMaintenance, startNodeMaintenanceModal, stopNodeMaintenanceModalLauncher]); return [actions, loading, loadError]; }; diff --git a/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx b/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx index 28d3421b346..148622fd458 100644 --- a/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx +++ b/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx @@ -1,6 +1,4 @@ -import type { TFunction } from 'i18next'; import { Trans, useTranslation } from 'react-i18next'; -import { confirmModal } from '@console/internal/components/modals/confirm-modal'; import type { K8sResourceKind } from '@console/internal/module/k8s'; import { k8sKill } from '@console/internal/module/k8s'; import { useWarningModal } from '@console/shared/src/hooks/useWarningModal'; @@ -27,28 +25,11 @@ const getMaintenanceModel = (nodeMaintenance: K8sResourceKind) => { return NodeMaintenanceKubevirtAlphaModel; }; -const stopNodeMaintenanceModal = (nodeMaintenance: K8sResourceKind, t: TFunction) => { - const reason = getNodeMaintenanceReason(nodeMaintenance); - const reasonLabel = reason ? `(${reason})` : ''; - const nodeName = getNodeMaintenanceNodeName(nodeMaintenance); - return confirmModal({ - title: t('metal3-plugin~Stop maintenance'), - message: ( - - Are you sure you want to stop maintenance {reasonLabel} on node{' '} - {nodeName}? - - ), - btnText: t('metal3-plugin~Stop maintenance'), - executeFn: () => k8sKill(getMaintenanceModel(nodeMaintenance), nodeMaintenance), - }); -}; - -export const useStopNodeMaintenanceModal = (nodeMaintenance: K8sResourceKind) => { +export const useStopNodeMaintenanceModal = (nodeMaintenance: K8sResourceKind | undefined) => { const { t } = useTranslation(); - const reason = getNodeMaintenanceReason(nodeMaintenance); + const reason = nodeMaintenance ? getNodeMaintenanceReason(nodeMaintenance) : ''; const reasonLabel = reason ? `(${reason})` : ''; - const nodeName = getNodeMaintenanceNodeName(nodeMaintenance); + const nodeName = nodeMaintenance ? getNodeMaintenanceNodeName(nodeMaintenance) : ''; const stopNodeMaintenanceModalLauncher = useWarningModal({ title: t('metal3-plugin~Stop maintenance'), children: ( @@ -58,9 +39,9 @@ export const useStopNodeMaintenanceModal = (nodeMaintenance: K8sResourceKind) => ), confirmButtonLabel: t('metal3-plugin~Stop maintenance'), - onConfirm: () => k8sKill(getMaintenanceModel(nodeMaintenance), nodeMaintenance), + onConfirm: nodeMaintenance + ? () => k8sKill(getMaintenanceModel(nodeMaintenance), nodeMaintenance) + : () => Promise.resolve(), }); return stopNodeMaintenanceModalLauncher; }; - -export default stopNodeMaintenanceModal; diff --git a/frontend/packages/shipwright-plugin/locales/en/shipwright-plugin.json b/frontend/packages/shipwright-plugin/locales/en/shipwright-plugin.json index e3362388c58..269033f39c5 100644 --- a/frontend/packages/shipwright-plugin/locales/en/shipwright-plugin.json +++ b/frontend/packages/shipwright-plugin/locales/en/shipwright-plugin.json @@ -96,7 +96,6 @@ "Started": "Started", "Unknown failure condition": "Unknown failure condition", "Failure on task {{taskName}} - check logs for details.": "Failure on task {{taskName}} - check logs for details.", - "Error downloading logs.": "Error downloading logs.", "An error occurred while retrieving the requested logs.": "An error occurred while retrieving the requested logs.", "Unknown error retrieving logs": "Unknown error retrieving logs", "Download": "Download", diff --git a/frontend/packages/shipwright-plugin/src/components/logs/logs-utils.ts b/frontend/packages/shipwright-plugin/src/components/logs/logs-utils.ts index fdbc4806e13..4bb21513235 100644 --- a/frontend/packages/shipwright-plugin/src/components/logs/logs-utils.ts +++ b/frontend/packages/shipwright-plugin/src/components/logs/logs-utils.ts @@ -1,21 +1,12 @@ -import { saveAs } from 'file-saver'; -import i18next from 'i18next'; import * as _ from 'lodash'; -import { coFetchText } from '@console/internal/co-fetch'; import { LOG_SOURCE_RESTARTING, LOG_SOURCE_RUNNING, LOG_SOURCE_TERMINATED, LOG_SOURCE_WAITING, - LineBuffer, } from '@console/internal/components/utils'; -import { PodModel } from '@console/internal/models'; -import type { PodKind, ContainerSpec, ContainerStatus } from '@console/internal/module/k8s'; -import { resourceURL, k8sGet } from '@console/internal/module/k8s'; -import { launchErrorModal } from '@console/shared/src/utils/error-modal-handler'; -import type { TaskRunKind } from '../../types'; +import type { ContainerSpec, ContainerStatus, PodKind } from '@console/internal/module/k8s'; import { ComputedStatus, SucceedConditionReason } from './log-snippet-types'; -import { getTaskRunLog } from './tekton-results'; export const containerToLogSourceStatus = (container: ContainerStatus): string => { if (!container) { @@ -67,131 +58,6 @@ export const getRenderContainers = ( }; }; -const getOrderedStepsFromPod = (name: string, ns: string): Promise => { - return k8sGet(PodModel, name, ns) - .then((pod: PodKind) => { - return getSortedContainerStatus( - pod.spec.containers ?? [], - pod.status?.containerStatuses ?? [], - ); - }) - .catch((err) => { - launchErrorModal({ - error: err.message || i18next.t('shipwright-plugin~Error downloading logs.'), - }); - return []; - }); -}; - -type StepsWatchUrl = { - [key: string]: { - name: string; - steps: { [step: string]: WatchURLStatus }; - taskRunPath: string; - }; -}; - -type WatchURLStatus = { - status: string; - url: string; -}; - -export const getDownloadAllLogsCallback = ( - sortedTaskRunNames: string[], - taskRuns: TaskRunKind[], - namespace: string, - pipelineRunName: string, -): (() => Promise) => { - const getWatchUrls = async (): Promise => { - const stepsList: ContainerStatus[][] = await Promise.all( - sortedTaskRunNames.map((currTask) => { - const { status } = taskRuns.find((t) => t.metadata.name === currTask) ?? {}; - return getOrderedStepsFromPod(status?.podName, namespace); - }), - ); - return sortedTaskRunNames.reduce((acc, currTask, i) => { - const taskRun = taskRuns.find((t) => t.metadata.name === currTask); - const pipelineTaskName = taskRun?.spec.taskRef?.name ?? taskRun?.metadata.name; - const { status } = taskRun; - const podName = status?.podName; - const steps = stepsList[i]; - const allStepUrls = steps.reduce((stepUrls, currentStep) => { - const { name } = currentStep; - const currentStatus = containerToLogSourceStatus(currentStep); - if (currentStatus === LOG_SOURCE_WAITING) return stepUrls; - const urlOpts = { - ns: namespace, - name: podName, - path: 'log', - queryParams: { - container: name, - follow: 'true', - }, - }; - return { - ...stepUrls, - [name]: { - status: currentStatus, - url: resourceURL(PodModel, urlOpts), - } as WatchURLStatus, - }; - }, {}); - acc[currTask] = { - name: pipelineTaskName, - steps: { ...allStepUrls }, - taskRunPath: taskRun.metadata?.annotations?.['results.tekton.dev/record'], - }; - return acc; - }, {}); - }; - - const fetchLogs = async (tasksPromise: Promise) => { - const tasks = await tasksPromise; - let allLogs = ''; - for (const currTask of sortedTaskRunNames) { - const task = tasks[currTask]; - const steps = Object.keys(task.steps); - allLogs += `${task.name}\n\n`; - if (steps.length > 0) { - for (const step of steps) { - const { url, status } = task.steps[step]; - const getContentPromise = coFetchText(url).then((logs) => { - return `${step.toUpperCase()}\n\n${logs}\n\n`; - }); - allLogs += - status === LOG_SOURCE_TERMINATED - ? // If we are done, we want this log content - // eslint-disable-next-line no-await-in-loop - await getContentPromise - : // If we are not done, let's not wait indefinitely - // eslint-disable-next-line no-await-in-loop - await Promise.race([ - getContentPromise, - new Promise((resolve) => { - setTimeout(() => resolve(''), 1000); - }), - ]); - } - } else { - // eslint-disable-next-line no-await-in-loop - allLogs += await getTaskRunLog(task.taskRunPath).then( - (log) => `${tasks[currTask].name.toUpperCase()}\n\n${log}\n\n`, - ); - } - } - const buffer = new LineBuffer(null); - buffer.ingest(allLogs); - const blob = buffer.getBlob({ - type: 'text/plain;charset=utf-8', - }); - saveAs(blob, `${pipelineRunName}.log`); - return null; - }; - return (): Promise => { - return fetchLogs(getWatchUrls()); - }; -}; - export const pipelineRunStatus = (pipelineRun): ComputedStatus => { const conditions = _.get(pipelineRun, ['status', 'conditions'], []); if (conditions.length === 0) return null; diff --git a/frontend/packages/topology/src/utils/moveNodeToGroup.tsx b/frontend/packages/topology/src/utils/moveNodeToGroup.tsx index ac238aa1a7b..cec2cc0c225 100644 --- a/frontend/packages/topology/src/utils/moveNodeToGroup.tsx +++ b/frontend/packages/topology/src/utils/moveNodeToGroup.tsx @@ -1,7 +1,8 @@ import type { Node } from '@patternfly/react-topology'; +import i18next from 'i18next'; import { Trans } from 'react-i18next'; -import { confirmModal } from '@console/internal/components/modals'; import { launchErrorModal } from '@console/shared/src/utils/error-modal-handler'; +import { launchWarningModal } from '@console/shared/src/utils/warning-modal-handler'; import { updateTopologyResourceApplication } from './topology-utils'; export const moveNodeToGroup = ( @@ -37,35 +38,38 @@ export const moveNodeToGroup = ( // t('topology~Remove') const btnTextKey = targetGroup ? 'topology~Move' : 'topology~Remove'; - return new Promise((resolve, reject) => { - confirmModal({ - titleKey, - message, - btnTextKey, - close: () => { - reject(); - }, - cancel: () => { - reject(); - }, - executeFn: () => { - return updateTopologyResourceApplication( - node, - targetGroup ? targetGroup.getLabel() : null, - ) - .then(resolve) - .catch((err) => { - const error = err.message; - if (onError) { - onError(error); - } else { - launchErrorModal({ error }); - } - reject(err); - }); - }, + // Blur active element to prevent aria-hidden focus violations when modal opens + // This fixes the browser warning: "Blocked aria-hidden on an element because its + // descendant retained focus" when focus remains on SVG elements during drag-drop + if ( + document.activeElement instanceof HTMLElement || + document.activeElement instanceof SVGElement + ) { + document.activeElement.blur(); + } + + return launchWarningModal({ + title: i18next.t(titleKey), + children: message, + confirmButtonLabel: i18next.t(btnTextKey), + titleIconVariant: null, + }) + .then(() => + updateTopologyResourceApplication(node, targetGroup ? targetGroup.getLabel() : null), + ) + .catch((err) => { + // Only show error modal if it's not a user cancellation + if (err.message && err.message !== 'User cancelled') { + const error = err.message; + if (onError) { + onError(error); + } else { + launchErrorModal({ error }); + } + } + // Always re-throw to signal the operation failed (whether cancelled or errored) + throw err; }); - }); } return updateTopologyResourceApplication(node, targetGroup.getLabel()).catch((err) => { diff --git a/frontend/public/components/app.tsx b/frontend/public/components/app.tsx index 31dcac1b84d..18a7f8a4365 100644 --- a/frontend/public/components/app.tsx +++ b/frontend/public/components/app.tsx @@ -53,7 +53,7 @@ import { QuickStartDrawer } from '@console/app/src/components/quick-starts/Quick import { ModalProvider } from '@console/dynamic-plugin-sdk/src/app/modal-support/ModalProvider'; import { OverlayProvider } from '@console/dynamic-plugin-sdk/src/app/modal-support/OverlayProvider'; import ToastProvider from '@console/shared/src/components/toast/ToastProvider'; -import { SyncErrorModalLauncher } from '@console/shared/src/utils/error-modal-handler'; +import { SyncModalLaunchers } from '@console/shared/src/utils/error-modal-handler'; import { useTelemetry } from '@console/shared/src/hooks/useTelemetry'; import { useDebounceCallback } from '@console/shared/src/hooks/debounce'; import { LOGIN_ERROR_PATH } from '@console/internal/module/auth'; @@ -319,7 +319,7 @@ const App: FC<{ - + }> {contextProviderExtensions.reduce( (children, e) => ( diff --git a/frontend/public/components/modals/confirm-modal.tsx b/frontend/public/components/modals/confirm-modal.tsx deleted file mode 100644 index 5601758848f..00000000000 --- a/frontend/public/components/modals/confirm-modal.tsx +++ /dev/null @@ -1,72 +0,0 @@ -import { Translation } from 'react-i18next'; -import { usePromiseHandler } from '@console/shared/src/hooks/promise-handler'; -import type { ReactNode, FormEvent, FC } from 'react'; -import { - createModalLauncher, - ModalTitle, - ModalBody, - ModalSubmitFooter, - ModalComponentProps, -} from '../factory/modal'; - -interface ConfirmModalProps extends ModalComponentProps { - btnText?: string | ReactNode; - btnTextKey?: string; - cancelText?: string | ReactNode; - cancelTextKey?: string; - executeFn: (e?: FormEvent, opts?: { supressNotifications: boolean }) => Promise; - message?: string | ReactNode; - messageKey?: string; - title?: string | ReactNode; - titleKey?: string; - submitDanger?: boolean; -} - -const ConfirmModal: FC = (props) => { - const [handlePromise, inProgress, errorMessage] = usePromiseHandler(); - - const submit = (event: FormEvent) => { - event.preventDefault(); - - handlePromise( - props.executeFn(null, { - supressNotifications: true, - }), - ).then(props.close); - }; - - const { - title, - titleKey, - message, - messageKey, - btnText, - btnTextKey, - cancelText, - cancelTextKey, - submitDanger, - cancel, - } = props; - - return ( - - {(t) => ( -
- {titleKey ? t(titleKey) : title} - {messageKey ? t(messageKey) : message} - - - )} -
- ); -}; - -/** @deprecated use `useWarningModal` instead */ -export const confirmModal = createModalLauncher(ConfirmModal); diff --git a/frontend/public/components/modals/index.ts b/frontend/public/components/modals/index.ts index 801c5599513..4cf88b5cb79 100644 --- a/frontend/public/components/modals/index.ts +++ b/frontend/public/components/modals/index.ts @@ -11,12 +11,6 @@ export const configureJobParallelismModal = (props) => m.configureJobParallelismModal(props), ); -/** @deprecated use `useWarningModal` instead */ -export const confirmModal = (props) => - import('./confirm-modal' /* webpackChunkName: "confirm-modal" */).then((m) => - m.confirmModal(props), - ); - // Lazy-loaded OverlayComponent for Configure Namespace Pull Secret Modal export const LazyConfigureNamespacePullSecretModalOverlay = lazy(() => import( diff --git a/frontend/public/locales/en/public.json b/frontend/public/locales/en/public.json index 2884855fcee..3ce74d9ebe4 100644 --- a/frontend/public/locales/en/public.json +++ b/frontend/public/locales/en/public.json @@ -911,7 +911,6 @@ "Recreate": "Recreate", "Shut down all existing pods before creating new ones": "Shut down all existing pods before creating new ones", "Edit update strategy": "Edit update strategy", - "Confirm": "Confirm", "Delete {{kind}}?": "Delete {{kind}}?", "Are you sure you want to delete <2>{{resourceName}} in namespace <5>{{namespace}}?": "Are you sure you want to delete <2>{{resourceName}} in namespace <5>{{namespace}}?", "Are you sure you want to delete <2>{{resourceName}}?": "Are you sure you want to delete <2>{{resourceName}}?", From 4c7cd998ccbf8b20b28d04aa8fa797895eefa922 Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Thu, 26 Feb 2026 14:21:43 -0500 Subject: [PATCH 2/7] Refactor launchWarningModal to use callbacks instead of Promises Changes the imperative launchWarningModal API from Promise-based to callback-based to better align with React patterns and the underlying modal system architecture. Rationale: - React hooks and PatternFly modals use callback patterns (onConfirm, onClose) - The overlay system passes closeOverlay as a callback, not a Promise - Promise wrapping creates an impedance mismatch with the underlying APIs - Callbacks are more idiomatic in React contexts Changes: - Updated launchWarningModal signature to accept onConfirm and onCancel callbacks - Modified moveNodeToGroup to wrap the callback API in a Promise for its needs - Updated mock implementation to use callbacks - Added comprehensive test suite (8 tests) for the callback-based API - Updated documentation and JSDoc examples This change addresses reviewer feedback on PR #15948 questioning why Promises were used instead of simple callback functions. Co-Authored-By: Claude Sonnet 4.5 --- .../utils/__mocks__/warning-modal-handler.tsx | 5 +- .../__tests__/warning-modal-handler.spec.tsx | 232 ++++++++++++++++++ .../src/utils/warning-modal-handler.tsx | 69 +++--- .../topology/src/utils/moveNodeToGroup.tsx | 50 ++-- 4 files changed, 301 insertions(+), 55 deletions(-) create mode 100644 frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx diff --git a/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx index ed2c2f14393..fc25713b131 100644 --- a/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx @@ -3,7 +3,10 @@ * Used in Jest tests to avoid rendering actual modals */ -export const mockLaunchWarningModal = jest.fn(() => Promise.resolve()); +export const mockLaunchWarningModal = jest.fn((props, onConfirm) => { + // Immediately call onConfirm by default to simulate user confirming + onConfirm?.(); +}); export const useSyncWarningModalLauncher = jest.fn(); diff --git a/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx b/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx new file mode 100644 index 00000000000..dd845762e57 --- /dev/null +++ b/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx @@ -0,0 +1,232 @@ +import { render } from '@testing-library/react'; +import { + useSyncWarningModalLauncher, + useWarningModalLauncher, + launchWarningModal, +} from '../warning-modal-handler'; + +// Mock useOverlay +const mockLauncher = jest.fn(); +jest.mock('@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay', () => ({ + useOverlay: () => mockLauncher, +})); + +describe('warning-modal-handler', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('useSyncWarningModalLauncher', () => { + it('should sync the launcher on mount', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + render(); + + const onConfirm = jest.fn(); + const onCancel = jest.fn(); + + // Call the module-level function + launchWarningModal( + { + title: 'Test Warning', + children: 'Are you sure?', + confirmButtonLabel: 'Confirm', + }, + onConfirm, + onCancel, + ); + + // Should have called the mocked overlay launcher + expect(mockLauncher).toHaveBeenCalledWith(expect.anything(), { + title: 'Test Warning', + children: 'Are you sure?', + confirmButtonLabel: 'Confirm', + onConfirm: expect.any(Function), + onClose: expect.any(Function), + }); + }); + + it('should cleanup launcher on unmount', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + const { unmount } = render(); + + unmount(); + + // Should throw error when launcher is not initialized + expect(() => { + launchWarningModal( + { + title: 'Test Warning', + children: 'Are you sure?', + }, + jest.fn(), + ); + }).toThrow('Warning modal launcher not initialized'); + }); + }); + + describe('useWarningModalLauncher', () => { + it('should return a function that launches warning modals', () => { + let capturedLauncher: any; + + const TestComponent = () => { + capturedLauncher = useWarningModalLauncher(); + return null; + }; + + render(); + + const onConfirm = jest.fn(); + capturedLauncher({ + title: 'Test Warning', + children: 'Test message', + confirmButtonLabel: 'OK', + onConfirm, + }); + + expect(mockLauncher).toHaveBeenCalledWith(expect.anything(), { + title: 'Test Warning', + children: 'Test message', + confirmButtonLabel: 'OK', + onConfirm, + }); + }); + }); + + describe('launchWarningModal', () => { + it('should launch warning modal when launcher is initialized', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + render(); + + const onConfirm = jest.fn(); + const onCancel = jest.fn(); + + launchWarningModal( + { + title: 'Delete Resource', + children: 'Are you sure you want to delete?', + confirmButtonLabel: 'Delete', + }, + onConfirm, + onCancel, + ); + + expect(mockLauncher).toHaveBeenCalledWith(expect.anything(), { + title: 'Delete Resource', + children: 'Are you sure you want to delete?', + confirmButtonLabel: 'Delete', + onConfirm: expect.any(Function), + onClose: expect.any(Function), + }); + }); + + it('should throw error when launcher is not initialized', () => { + expect(() => { + launchWarningModal( + { + title: 'Test', + children: 'Test message', + }, + jest.fn(), + ); + }).toThrow('Warning modal launcher not initialized'); + }); + + it('should call onConfirm callback when user confirms', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + render(); + + const onConfirm = jest.fn(); + const onCancel = jest.fn(); + + launchWarningModal( + { + title: 'Confirm Action', + children: 'Proceed?', + }, + onConfirm, + onCancel, + ); + + // Get the onConfirm callback that was passed to the launcher + const launcherCall = mockLauncher.mock.calls[0]; + const launcherProps = launcherCall[1]; + launcherProps.onConfirm(); + + expect(onConfirm).toHaveBeenCalled(); + expect(onCancel).not.toHaveBeenCalled(); + }); + + it('should call onCancel callback when user cancels', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + render(); + + const onConfirm = jest.fn(); + const onCancel = jest.fn(); + + launchWarningModal( + { + title: 'Confirm Action', + children: 'Proceed?', + }, + onConfirm, + onCancel, + ); + + // Get the onClose callback that was passed to the launcher + const launcherCall = mockLauncher.mock.calls[0]; + const launcherProps = launcherCall[1]; + launcherProps.onClose(); + + expect(onCancel).toHaveBeenCalled(); + expect(onConfirm).not.toHaveBeenCalled(); + }); + + it('should handle optional callbacks gracefully', () => { + const TestComponent = () => { + useSyncWarningModalLauncher(); + return null; + }; + + render(); + + // Call without callbacks + expect(() => { + launchWarningModal({ + title: 'Info', + children: 'Just showing info', + }); + }).not.toThrow(); + + expect(mockLauncher).toHaveBeenCalled(); + + // Verify calling the callbacks doesn't throw + const launcherCall = mockLauncher.mock.calls[0]; + const launcherProps = launcherCall[1]; + + expect(() => { + launcherProps.onConfirm(); + launcherProps.onClose(); + }).not.toThrow(); + }); + }); +}); diff --git a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx index cd673579f0f..a89b8c709a4 100644 --- a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx @@ -66,49 +66,54 @@ export const useWarningModalLauncher = (): ((props: ControlledWarningModalProps) /** * Launch a warning modal from non-React contexts (callbacks, promises, utilities). - * The SyncWarningModalLauncher component must be mounted in the app root. + * The SyncModalLaunchers component must be mounted in the app root. * * @deprecated Use React component modals within component code instead. * For new code, write modals directly within React components using useWarningModal. * This function should only be used for legacy non-React contexts like promise callbacks. * * @param props - Warning modal properties (title, children, confirmButtonLabel, etc.) - * @returns Promise that resolves (undefined) when confirmed, rejects with Error('User cancelled') when canceled/closed + * @param onConfirm - Optional callback invoked when user confirms the action + * @param onCancel - Optional callback invoked when user cancels or closes the modal * * @example * ```tsx - * // In a promise callback or utility function - * launchWarningModal({ - * title: 'Delete Resource', - * children: 'Are you sure you want to delete this resource?', - * confirmButtonLabel: 'Delete', - * }).then(() => { - * // User confirmed - proceed with action - * deleteResource(); - * }).catch((error) => { - * // User canceled - error.message === 'User cancelled' - * console.log('Action cancelled by user'); - * }); + * // In a utility function or non-React context + * launchWarningModal( + * { + * title: 'Delete Resource', + * children: 'Are you sure you want to delete this resource?', + * confirmButtonLabel: 'Delete', + * }, + * () => { + * // User confirmed - proceed with action + * deleteResource(); + * }, + * () => { + * // User canceled + * console.log('Action cancelled by user'); + * } + * ); * ``` */ export const launchWarningModal = ( props: Omit, -): Promise => { - return new Promise((resolve, reject) => { - if (moduleWarningModalLauncher) { - moduleWarningModalLauncher({ - ...props, - onConfirm: () => { - resolve(); - }, - onClose: () => { - reject(new Error('User cancelled')); - }, - }); - } else { - throw new Error( - 'Warning modal launcher not initialized. Ensure SyncWarningModalLauncher is mounted after OverlayProvider.', - ); - } - }); + onConfirm?: () => void, + onCancel?: () => void, +): void => { + if (moduleWarningModalLauncher) { + moduleWarningModalLauncher({ + ...props, + onConfirm: () => { + onConfirm?.(); + }, + onClose: () => { + onCancel?.(); + }, + }); + } else { + throw new Error( + 'Warning modal launcher not initialized. Ensure SyncModalLaunchers is mounted after OverlayProvider.', + ); + } }; diff --git a/frontend/packages/topology/src/utils/moveNodeToGroup.tsx b/frontend/packages/topology/src/utils/moveNodeToGroup.tsx index cec2cc0c225..657fb882482 100644 --- a/frontend/packages/topology/src/utils/moveNodeToGroup.tsx +++ b/frontend/packages/topology/src/utils/moveNodeToGroup.tsx @@ -48,28 +48,34 @@ export const moveNodeToGroup = ( document.activeElement.blur(); } - return launchWarningModal({ - title: i18next.t(titleKey), - children: message, - confirmButtonLabel: i18next.t(btnTextKey), - titleIconVariant: null, - }) - .then(() => - updateTopologyResourceApplication(node, targetGroup ? targetGroup.getLabel() : null), - ) - .catch((err) => { - // Only show error modal if it's not a user cancellation - if (err.message && err.message !== 'User cancelled') { - const error = err.message; - if (onError) { - onError(error); - } else { - launchErrorModal({ error }); - } - } - // Always re-throw to signal the operation failed (whether cancelled or errored) - throw err; - }); + return new Promise((resolve, reject) => { + launchWarningModal( + { + title: i18next.t(titleKey), + children: message, + confirmButtonLabel: i18next.t(btnTextKey), + titleIconVariant: null, + }, + () => { + // User confirmed - proceed with move/remove + updateTopologyResourceApplication(node, targetGroup ? targetGroup.getLabel() : null) + .then(() => resolve()) + .catch((err) => { + const error = err.message; + if (onError) { + onError(error); + } else { + launchErrorModal({ error }); + } + reject(err); + }); + }, + () => { + // User cancelled - reject to signal operation was cancelled + reject(new Error('User cancelled')); + }, + ); + }); } return updateTopologyResourceApplication(node, targetGroup.getLabel()).catch((err) => { From 688729991d079934e8d181297fa3c3b7a3e0a779 Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Mon, 2 Mar 2026 10:16:27 -0500 Subject: [PATCH 3/7] Fix infinite re-render loop in useStopNodeMaintenanceModal The refactoring from Promise-based confirmModal to hook-based useWarningModal introduced an infinite re-render loop in the Node list page, causing "Maximum update depth exceeded" errors. Root cause: - useStopNodeMaintenanceModal was called with nodeMaintenance as a parameter at hook initialization time - This created unstable dependencies that triggered infinite updates in ActionsHookResolver's useEffect Solution: - Changed useStopNodeMaintenanceModal to accept no parameters - Returns a callback that accepts nodeMaintenance when called - Moved nodeMaintenance computation inside useMemo in actions.tsx - Excluded modal launcher from dependency arrays (documented pattern) - JSX is now created at call time, not during hook initialization This matches the pattern used in topology/edgeActions.ts and console-app/useReplicationControllerActions.ts where modal launchers are intentionally excluded from deps to prevent infinite loops. Co-Authored-By: Claude Sonnet 4.5 --- .../baremetal-hosts/host-actions-provider.ts | 8 ++-- .../StartingMaintenancePopoverContent.tsx | 8 +++- .../UnderMaintenancePopoverContent.tsx | 8 +++- .../src/components/maintenance/actions.tsx | 12 +++-- .../modals/StopNodeMaintenanceModal.tsx | 46 +++++++++++-------- 5 files changed, 53 insertions(+), 29 deletions(-) diff --git a/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts b/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts index 163db208b7e..831f004571e 100644 --- a/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts +++ b/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts @@ -125,18 +125,20 @@ export const useRemoveNodeMaintenanceAction = ( ) => { const { t } = useTranslation(); const hidden = !nodeName || !hasNodeMaintenanceCapability || !nodeMaintenance; - const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(); const factory = useMemo( () => ({ removeNodeMaintenance: () => ({ id: 'remove-node-maintenance', label: t('metal3-plugin~Stop Maintenance'), - cta: stopNodeMaintenanceModalLauncher, + cta: nodeMaintenance ? () => stopNodeMaintenanceModalLauncher(nodeMaintenance) : undefined, accessReview: host && asAccessReview(maintenanceModel, host, 'delete'), }), }), + // Missing stopNodeMaintenanceModalLauncher dependency - intentionally excluded to prevent + // infinite re-renders when used in action hooks. // eslint-disable-next-line react-hooks/exhaustive-deps - [host, t], + [host, t, nodeMaintenance], ); const action = useMemo(() => (!hidden ? [factory.removeNodeMaintenance()] : []), [ factory, diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx index ff6f436ff03..ca49ea582fd 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/StartingMaintenancePopoverContent.tsx @@ -31,7 +31,7 @@ const StartingMaintenancePopoverContent: FC { const { t } = useTranslation(); - const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(); const reason = getNodeMaintenanceReason(nodeMaintenance); const creationTimestamp = getNodeMaintenanceCreationTimestamp(nodeMaintenance); const lastError = getNodeMaintenanceLastError(nodeMaintenance); @@ -79,7 +79,11 @@ const StartingMaintenancePopoverContent: FC
- diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx index 43d659ca3fe..8d178084668 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/UnderMaintenancePopoverContent.tsx @@ -20,7 +20,7 @@ const UnderMaintenancePopoverContent: FC = nodeMaintenance, }) => { const { t } = useTranslation(); - const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(); const reason = getNodeMaintenanceReason(nodeMaintenance); const creationTimestamp = getNodeMaintenanceCreationTimestamp(nodeMaintenance); @@ -44,7 +44,11 @@ const UnderMaintenancePopoverContent: FC =
- diff --git a/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx b/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx index 8ffc3bd0411..b19f8139cbc 100644 --- a/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx +++ b/frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx @@ -24,10 +24,11 @@ export const useNodeMaintenanceActions: ExtensionHook = (res namespaced: false, }); - const nodeMaintenance = findNodeMaintenance(maintenances, resource.metadata.name); - const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(nodeMaintenance); + const stopNodeMaintenanceModalLauncher = useStopNodeMaintenanceModal(); const actions = useMemo(() => { + const nodeMaintenance = findNodeMaintenance(maintenances, resource.metadata.name); + let action: Action = { id: 'start-node-maintenance', label: t('metal3-plugin~Start Maintenance'), @@ -39,12 +40,15 @@ export const useNodeMaintenanceActions: ExtensionHook = (res action = { id: 'stop-node-maintenance', label: t('metal3-plugin~Stop Maintenance'), - cta: () => stopNodeMaintenanceModalLauncher(), + cta: () => stopNodeMaintenanceModalLauncher(nodeMaintenance), insertBefore: 'edit-labels', }; } return [action]; - }, [t, nodeMaintenance, startNodeMaintenanceModal, stopNodeMaintenanceModalLauncher]); + // missing startNodeMaintenanceModal and stopNodeMaintenanceModalLauncher dependencies, + // that causes max depth exceeded error + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [maintenances, resource.metadata.name, t]); return [actions, loading, loadError]; }; diff --git a/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx b/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx index 148622fd458..f5503248075 100644 --- a/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx +++ b/frontend/packages/metal3-plugin/src/components/modals/StopNodeMaintenanceModal.tsx @@ -1,3 +1,4 @@ +import { useCallback } from 'react'; import { Trans, useTranslation } from 'react-i18next'; import type { K8sResourceKind } from '@console/internal/module/k8s'; import { k8sKill } from '@console/internal/module/k8s'; @@ -25,23 +26,32 @@ const getMaintenanceModel = (nodeMaintenance: K8sResourceKind) => { return NodeMaintenanceKubevirtAlphaModel; }; -export const useStopNodeMaintenanceModal = (nodeMaintenance: K8sResourceKind | undefined) => { +export const useStopNodeMaintenanceModal = () => { const { t } = useTranslation(); - const reason = nodeMaintenance ? getNodeMaintenanceReason(nodeMaintenance) : ''; - const reasonLabel = reason ? `(${reason})` : ''; - const nodeName = nodeMaintenance ? getNodeMaintenanceNodeName(nodeMaintenance) : ''; - const stopNodeMaintenanceModalLauncher = useWarningModal({ - title: t('metal3-plugin~Stop maintenance'), - children: ( - - Are you sure you want to stop maintenance {reasonLabel} on node{' '} - {nodeName}? - - ), - confirmButtonLabel: t('metal3-plugin~Stop maintenance'), - onConfirm: nodeMaintenance - ? () => k8sKill(getMaintenanceModel(nodeMaintenance), nodeMaintenance) - : () => Promise.resolve(), - }); - return stopNodeMaintenanceModalLauncher; + const launchWarningModal = useWarningModal(); + + return useCallback( + (nodeMaintenance: K8sResourceKind) => { + const reason = getNodeMaintenanceReason(nodeMaintenance); + const reasonLabel = reason ? `(${reason})` : ''; + const nodeName = getNodeMaintenanceNodeName(nodeMaintenance); + + launchWarningModal({ + title: t('metal3-plugin~Stop maintenance'), + children: ( + + Are you sure you want to stop maintenance {reasonLabel} on node{' '} + {nodeName}? + + ), + confirmButtonLabel: t('metal3-plugin~Stop maintenance'), + onConfirm: () => k8sKill(getMaintenanceModel(nodeMaintenance), nodeMaintenance), + }); + }, + // Missing launchWarningModal dependency - intentionally excluded to prevent infinite re-renders. + // The modal launcher function reference changes when its props change, but we capture + // nodeMaintenance at call time via closure, so the stale reference is safe here. + // eslint-disable-next-line react-hooks/exhaustive-deps + [t], + ); }; From 72c92d6e20c50520ae853d9372721d67248d56f7 Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Mon, 2 Mar 2026 16:01:50 -0500 Subject: [PATCH 4/7] Remove redundant useWarningModalLauncher and simplify Stop Maintenance 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 --- .../utils/__mocks__/warning-modal-handler.tsx | 2 -- .../__tests__/warning-modal-handler.spec.tsx | 34 +----------------- .../src/utils/warning-modal-handler.tsx | 36 +------------------ .../baremetal-hosts/host-actions-provider.ts | 2 +- 4 files changed, 3 insertions(+), 71 deletions(-) diff --git a/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx index fc25713b131..6662d7cc69a 100644 --- a/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/__mocks__/warning-modal-handler.tsx @@ -10,6 +10,4 @@ export const mockLaunchWarningModal = jest.fn((props, onConfirm) => { export const useSyncWarningModalLauncher = jest.fn(); -export const useWarningModalLauncher = jest.fn(() => mockLaunchWarningModal); - export const launchWarningModal = mockLaunchWarningModal; diff --git a/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx b/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx index dd845762e57..6f6fe0b9761 100644 --- a/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx +++ b/frontend/packages/console-shared/src/utils/__tests__/warning-modal-handler.spec.tsx @@ -1,9 +1,5 @@ import { render } from '@testing-library/react'; -import { - useSyncWarningModalLauncher, - useWarningModalLauncher, - launchWarningModal, -} from '../warning-modal-handler'; +import { useSyncWarningModalLauncher, launchWarningModal } from '../warning-modal-handler'; // Mock useOverlay const mockLauncher = jest.fn(); @@ -72,34 +68,6 @@ describe('warning-modal-handler', () => { }); }); - describe('useWarningModalLauncher', () => { - it('should return a function that launches warning modals', () => { - let capturedLauncher: any; - - const TestComponent = () => { - capturedLauncher = useWarningModalLauncher(); - return null; - }; - - render(); - - const onConfirm = jest.fn(); - capturedLauncher({ - title: 'Test Warning', - children: 'Test message', - confirmButtonLabel: 'OK', - onConfirm, - }); - - expect(mockLauncher).toHaveBeenCalledWith(expect.anything(), { - title: 'Test Warning', - children: 'Test message', - confirmButtonLabel: 'OK', - onConfirm, - }); - }); - }); - describe('launchWarningModal', () => { it('should launch warning modal when launcher is initialized', () => { const TestComponent = () => { diff --git a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx index a89b8c709a4..62effc17152 100644 --- a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect } from 'react'; +import { useEffect } from 'react'; import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; import type { ControlledWarningModalProps } from '../hooks/useWarningModal'; import { ControlledWarningModal } from '../hooks/useWarningModal'; @@ -30,40 +30,6 @@ export const useSyncWarningModalLauncher = () => { }, [launcher]); }; -/** - * Hook to launch warning modals from React components. - * Must be used within an OverlayProvider. - * Use `useWarningModal` instead for better React integration. - * - * @example - * ```tsx - * const MyComponent = () => { - * const launchWarningModal = useWarningModalLauncher(); - * - * const handleWarning = () => { - * launchWarningModal({ - * title: 'Are you sure?', - * children: 'This action cannot be undone.', - * confirmButtonLabel: 'Continue', - * onConfirm: () => console.log('Confirmed'), - * }); - * }; - * - * // ... - * }; - * ``` - */ -export const useWarningModalLauncher = (): ((props: ControlledWarningModalProps) => void) => { - const launcher = useOverlay(); - - return useCallback( - (props: ControlledWarningModalProps) => { - launcher(ControlledWarningModal, props); - }, - [launcher], - ); -}; - /** * Launch a warning modal from non-React contexts (callbacks, promises, utilities). * The SyncModalLaunchers component must be mounted in the app root. diff --git a/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts b/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts index 831f004571e..fd74f206f31 100644 --- a/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts +++ b/frontend/packages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts @@ -131,7 +131,7 @@ export const useRemoveNodeMaintenanceAction = ( removeNodeMaintenance: () => ({ id: 'remove-node-maintenance', label: t('metal3-plugin~Stop Maintenance'), - cta: nodeMaintenance ? () => stopNodeMaintenanceModalLauncher(nodeMaintenance) : undefined, + cta: () => stopNodeMaintenanceModalLauncher(nodeMaintenance), accessReview: host && asAccessReview(maintenanceModel, host, 'delete'), }), }), From 1ab1d3b7fc09ff63c6c393e38a1b45609fa8342a Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Mon, 2 Mar 2026 16:14:33 -0500 Subject: [PATCH 5/7] Refactor modal handlers to use shared useErrorModal/useWarningModal hooks - 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 --- .../utils/__mocks__/error-modal-handler.tsx | 4 +- .../__tests__/error-modal-handler.spec.tsx | 26 +------ .../src/utils/error-modal-handler.tsx | 75 ++++++++++--------- .../src/utils/warning-modal-handler.tsx | 10 +-- 4 files changed, 45 insertions(+), 70 deletions(-) diff --git a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx index fed53fcb047..9dcc3b15330 100644 --- a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx @@ -10,10 +10,10 @@ export const mockLaunchErrorModal = jest.fn(); export const useSyncErrorModalLauncher = jest.fn(); +export const useErrorModal = jest.fn(() => mockLaunchErrorModal); + // Simplified component that doesn't sync warning modals // Tests needing both error and warning modals should not use this mock export const SyncModalLaunchers = () => null; -export const useErrorModalLauncher = jest.fn(() => mockLaunchErrorModal); - export const launchErrorModal = mockLaunchErrorModal; diff --git a/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx b/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx index 4965ee42445..2fe92f0137c 100644 --- a/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx +++ b/frontend/packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsx @@ -1,9 +1,5 @@ import { render } from '@testing-library/react'; -import { - SyncModalLaunchers, - useErrorModalLauncher, - launchErrorModal, -} from '../error-modal-handler'; +import { SyncModalLaunchers, launchErrorModal } from '../error-modal-handler'; // Mock useOverlay const mockLauncher = jest.fn(); @@ -42,26 +38,6 @@ describe('error-modal-handler', () => { }); }); - describe('useErrorModalLauncher', () => { - it('should return a function that launches error modals', () => { - let capturedLauncher: any; - - const TestComponent = () => { - capturedLauncher = useErrorModalLauncher(); - return null; - }; - - render(); - - capturedLauncher({ error: 'Test error', title: 'Test Title' }); - - expect(mockLauncher).toHaveBeenCalledWith(expect.anything(), { - error: 'Test error', - title: 'Test Title', - }); - }); - }); - describe('launchErrorModal', () => { it('should launch error modal when launcher is initialized', () => { render(); diff --git a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx index 9d6c48607cb..f3b9add10f8 100644 --- a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx @@ -8,17 +8,51 @@ import { useSyncWarningModalLauncher } from './warning-modal-handler'; // This is populated by useSyncErrorModalLauncher and should not be set directly let moduleErrorModalLauncher: ((props: ErrorModalProps) => void) | null = null; +/** + * Hook to launch error modals from React components. + * Must be used within an OverlayProvider. + * + * @example + * ```tsx + * const MyComponent = () => { + * const launchErrorModal = useErrorModal(); + * + * const handleError = (error: Error) => { + * launchErrorModal({ + * title: 'Operation Failed', + * error: error.message, + * }); + * }; + * + * // ... + * }; + * ``` + */ +export const useErrorModal = ( + props?: Partial, +): ((overrides?: ErrorModalProps) => void) => { + const launcher = useOverlay(); + return useCallback( + (overrides?: ErrorModalProps) => { + const mergedProps: ErrorModalProps = { + error: '', + ...(props || {}), + ...(overrides || {}), + }; + launcher(ErrorModal, mergedProps); + }, + [launcher, props], + ); +}; + /** * Hook that syncs the error modal launcher to module-level for non-React contexts. * Use SyncModalLaunchers component instead of calling this directly. */ export const useSyncErrorModalLauncher = () => { - const launcher = useOverlay(); + const errorModalLauncher = useErrorModal(); useEffect(() => { - const errorModalLauncher = (props: ErrorModalProps) => { - launcher(ErrorModal, props); - }; moduleErrorModalLauncher = errorModalLauncher; return () => { @@ -27,7 +61,7 @@ export const useSyncErrorModalLauncher = () => { moduleErrorModalLauncher = null; } }; - }, [launcher]); + }, [errorModalLauncher]); }; /** @@ -50,37 +84,6 @@ export const SyncModalLaunchers = () => { return null; }; -/** - * Hook to launch error modals from React components. - * Must be used within an OverlayProvider. - * - * @example - * ```tsx - * const MyComponent = () => { - * const launchErrorModal = useErrorModalLauncher(); - * - * const handleError = (error: Error) => { - * launchErrorModal({ - * title: 'Operation Failed', - * error: error.message, - * }); - * }; - * - * // ... - * }; - * ``` - */ -export const useErrorModalLauncher = (): ((props: ErrorModalProps) => void) => { - const launcher = useOverlay(); - - return useCallback( - (props: ErrorModalProps) => { - launcher(ErrorModal, props); - }, - [launcher], - ); -}; - /** * Launch an error modal from non-React contexts (callbacks, promises, utilities). * The SyncModalLaunchers component must be mounted in the app root. diff --git a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx index 62effc17152..35dab142c7e 100644 --- a/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/warning-modal-handler.tsx @@ -1,7 +1,6 @@ import { useEffect } from 'react'; -import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; import type { ControlledWarningModalProps } from '../hooks/useWarningModal'; -import { ControlledWarningModal } from '../hooks/useWarningModal'; +import { useWarningModal } from '../hooks/useWarningModal'; // Module-level reference for non-React contexts // This is populated by useSyncWarningModalLauncher and should not be set directly @@ -13,12 +12,9 @@ let moduleWarningModalLauncher: ((props: ControlledWarningModalProps) => void) | * Use SyncModalLaunchers component from error-modal-handler instead of calling this directly. */ export const useSyncWarningModalLauncher = () => { - const launcher = useOverlay(); + const warningModalLauncher = useWarningModal(); useEffect(() => { - const warningModalLauncher = (props: ControlledWarningModalProps) => { - launcher(ControlledWarningModal, props); - }; moduleWarningModalLauncher = warningModalLauncher; return () => { @@ -27,7 +23,7 @@ export const useSyncWarningModalLauncher = () => { moduleWarningModalLauncher = null; } }; - }, [launcher]); + }, [warningModalLauncher]); }; /** From a1399115ae96a95cc2fa50c57e094d27f7e9ebf3 Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Mon, 2 Mar 2026 16:38:15 -0500 Subject: [PATCH 6/7] Remove duplicate useErrorModal hook and consolidate into useErrorModalLauncher 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 --- .../utils/__mocks__/error-modal-handler.tsx | 2 - .../src/utils/error-modal-handler.tsx | 44 ++----------------- .../public/components/modals/error-modal.tsx | 16 ++++++- 3 files changed, 17 insertions(+), 45 deletions(-) diff --git a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx index 9dcc3b15330..a37ceb32119 100644 --- a/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx @@ -10,8 +10,6 @@ export const mockLaunchErrorModal = jest.fn(); export const useSyncErrorModalLauncher = jest.fn(); -export const useErrorModal = jest.fn(() => mockLaunchErrorModal); - // Simplified component that doesn't sync warning modals // Tests needing both error and warning modals should not use this mock export const SyncModalLaunchers = () => null; diff --git a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx index f3b9add10f8..5a0218d4879 100644 --- a/frontend/packages/console-shared/src/utils/error-modal-handler.tsx +++ b/frontend/packages/console-shared/src/utils/error-modal-handler.tsx @@ -1,56 +1,18 @@ -import { useCallback, useEffect } from 'react'; -import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; +import { useEffect } from 'react'; import type { ErrorModalProps } from '@console/internal/components/modals/error-modal'; -import { ErrorModal } from '@console/internal/components/modals/error-modal'; +import { useErrorModalLauncher } from '@console/internal/components/modals/error-modal'; import { useSyncWarningModalLauncher } from './warning-modal-handler'; // Module-level reference for non-React contexts // This is populated by useSyncErrorModalLauncher and should not be set directly let moduleErrorModalLauncher: ((props: ErrorModalProps) => void) | null = null; -/** - * Hook to launch error modals from React components. - * Must be used within an OverlayProvider. - * - * @example - * ```tsx - * const MyComponent = () => { - * const launchErrorModal = useErrorModal(); - * - * const handleError = (error: Error) => { - * launchErrorModal({ - * title: 'Operation Failed', - * error: error.message, - * }); - * }; - * - * // ... - * }; - * ``` - */ -export const useErrorModal = ( - props?: Partial, -): ((overrides?: ErrorModalProps) => void) => { - const launcher = useOverlay(); - return useCallback( - (overrides?: ErrorModalProps) => { - const mergedProps: ErrorModalProps = { - error: '', - ...(props || {}), - ...(overrides || {}), - }; - launcher(ErrorModal, mergedProps); - }, - [launcher, props], - ); -}; - /** * Hook that syncs the error modal launcher to module-level for non-React contexts. * Use SyncModalLaunchers component instead of calling this directly. */ export const useSyncErrorModalLauncher = () => { - const errorModalLauncher = useErrorModal(); + const errorModalLauncher = useErrorModalLauncher(); useEffect(() => { moduleErrorModalLauncher = errorModalLauncher; diff --git a/frontend/public/components/modals/error-modal.tsx b/frontend/public/components/modals/error-modal.tsx index 6ef082221c3..fb10bf1d49a 100644 --- a/frontend/public/components/modals/error-modal.tsx +++ b/frontend/public/components/modals/error-modal.tsx @@ -53,9 +53,21 @@ export const ErrorModal: OverlayComponent = (props) => { ); }; -export const useErrorModalLauncher = (props) => { +export const useErrorModalLauncher = ( + props?: Partial, +): ((overrides?: ErrorModalProps) => void) => { const launcher = useOverlay(); - return useCallback(() => launcher(ErrorModal, props), [launcher, props]); + return useCallback( + (overrides?: ErrorModalProps) => { + const mergedProps: ErrorModalProps = { + error: '', + ...(props || {}), + ...(overrides || {}), + }; + launcher(ErrorModal, mergedProps); + }, + [launcher, props], + ); }; export type ErrorModalProps = { From 18f6bd305550231411c76dafc483b52a0bc0366d Mon Sep 17 00:00:00 2001 From: Robb Hamilton Date: Mon, 2 Mar 2026 16:38:31 -0500 Subject: [PATCH 7/7] Make ControlledWarningModal component private 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 --- frontend/packages/console-shared/src/hooks/useWarningModal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/packages/console-shared/src/hooks/useWarningModal.tsx b/frontend/packages/console-shared/src/hooks/useWarningModal.tsx index a6e5e60d269..853d468a20e 100644 --- a/frontend/packages/console-shared/src/hooks/useWarningModal.tsx +++ b/frontend/packages/console-shared/src/hooks/useWarningModal.tsx @@ -7,7 +7,7 @@ import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/us /** * ControlledWarningModal is a wrapper around WarningModal that manages its open state. */ -export const ControlledWarningModal: OverlayComponent = ({ +const ControlledWarningModal: OverlayComponent = ({ closeOverlay, ...props }) => {