-
Notifications
You must be signed in to change notification settings - Fork 670
[WIP] CONSOLE-5012: Create shared error modal launcher using React Context #15946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@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. |
|
Skipping CI for Draft Pull Request. |
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
ace764a to
0bbf1a7
Compare
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
1 similar comment
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@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. |
0bbf1a7 to
cd8a85e
Compare
This commit migrates all instances of confirmModal to use the modern overlay pattern with useWarningModal, completing the modal migration effort started in previous PRs. Changes: - Removed deprecated confirmModal launcher and confirm-modal.tsx file - Migrated topology moveNodeToGroup to use useWarningModal with proper cancel handling via global handler pattern - Updated StopNodeMaintenanceModal to remove deprecated imperative API and keep only the useStopNodeMaintenanceModal hook - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread to prevent React DOM warnings - Added try-catch in topology drag-drop to prevent optimistic update when user cancels the move operation - Updated all metal3-plugin maintenance components to use the hook-based approach This PR builds upon and depends on: - openshift#15932 (ResourceLimitsModal migration) - openshift#15939 (DeletePDBModal migration) - openshift#15940 (ConfigureUnschedulableModal migration) - openshift#15946 (Global error modal utility) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
This commit migrates all instances of confirmModal to use the modern overlay pattern with useWarningModal, completing the modal migration effort started in previous PRs. Changes: - Removed deprecated confirmModal launcher and confirm-modal.tsx file - Migrated topology moveNodeToGroup to use useWarningModal with proper cancel handling via global handler pattern - Updated StopNodeMaintenanceModal to remove deprecated imperative API and keep only the useStopNodeMaintenanceModal hook - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread to prevent React DOM warnings - Added try-catch in topology drag-drop to prevent optimistic update when user cancels the move operation - Updated all metal3-plugin maintenance components to use the hook-based approach This PR builds upon and depends on: - openshift#15932 (ResourceLimitsModal migration) - openshift#15939 (DeletePDBModal migration) - openshift#15940 (ConfigureUnschedulableModal migration) - openshift#15946 (Global error modal utility) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
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>
6f9a46a to
f071fd2
Compare
This commit migrates all instances of confirmModal to use the modern overlay pattern with useWarningModal, completing the modal migration effort started in previous PRs. Changes: - Removed deprecated confirmModal launcher and confirm-modal.tsx file - Migrated topology moveNodeToGroup to use useWarningModal with proper cancel handling via global handler pattern - Updated StopNodeMaintenanceModal to remove deprecated imperative API and keep only the useStopNodeMaintenanceModal hook - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread to prevent React DOM warnings - Added try-catch in topology drag-drop to prevent optimistic update when user cancels the move operation - Updated all metal3-plugin maintenance components to use the hook-based approach This PR builds upon and depends on: - openshift#15932 (ResourceLimitsModal migration) - openshift#15939 (DeletePDBModal migration) - openshift#15940 (ConfigureUnschedulableModal migration) - openshift#15946 (Global error modal utility) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/assign @yanpzhan Tech debt, so assigning labels |
This commit refactors the global error modal launcher implementation to use React Context instead of module-level global state, addressing architectural concerns about race conditions and testability. The implementation reuses the existing OverlayProvider instead of creating a separate provider, using a lightweight sync component to bridge React and non-React contexts. Changes: - Refactored error-modal-handler to use OverlayProvider directly - Created SyncErrorModalLauncher - zero-render component to sync module variable - Added useErrorModalLauncher() hook that wraps useOverlay() - Added launchErrorModal() function for non-React contexts (callbacks, utilities) - Renamed launchGlobalErrorModal → launchErrorModal - Removed useSetupGlobalErrorModalLauncher() initialization hook - Added SyncErrorModalLauncher to app.tsx inside OverlayProvider - Removed initialization calls from Topology.tsx, ImportPage.tsx, DeployImagePage.tsx, and BuildRunDetailsPage.tsx Benefits: - Eliminates global state race conditions - Improves testability (overlay context can be mocked) - Single initialization point in app root - Reuses existing OverlayProvider infrastructure - No unnecessary provider nesting - Simpler architecture with fewer concepts - Maintains backward compatibility for non-React contexts via module reference - Proper lifecycle management via useEffect cleanup The hybrid approach ensures both React components (via useErrorModalLauncher hook) and non-React utilities (via launchErrorModal function) can launch error modals. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
This commit migrates all instances of confirmModal to use the modern overlay pattern with useWarningModal, completing the modal migration effort started in previous PRs. Changes: - Removed deprecated confirmModal launcher and confirm-modal.tsx file - Migrated topology moveNodeToGroup to use useWarningModal with proper cancel handling via global handler pattern - Updated StopNodeMaintenanceModal to remove deprecated imperative API and keep only the useStopNodeMaintenanceModal hook - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread to prevent React DOM warnings - Added try-catch in topology drag-drop to prevent optimistic update when user cancels the move operation - Updated all metal3-plugin maintenance components to use the hook-based approach This PR builds upon and depends on: - openshift#15932 (ResourceLimitsModal migration) - openshift#15939 (DeletePDBModal migration) - openshift#15940 (ConfigureUnschedulableModal migration) - openshift#15946 (Global error modal utility) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit refactors the moveNodeToGroup global handler pattern to use React Context instead of module-level variables, addressing architectural concerns and following the same pattern established in openshift#15946. Changes: - Created MoveNodeHandlersProvider using React Context - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Refactored moveNodeToGroup to use context-based handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests, all passing) Benefits: - Eliminates global state race conditions - Follows React Context pattern from error modal handler (openshift#15946) - Improves testability with mockable context - Single provider initialization around Topology component - Proper cleanup via React lifecycle - Type-safe with clear error messages - Maintains backward compatibility via deprecated hooks The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), similar to the error modal handler pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit refactors the moveNodeToGroup global handler pattern to use React Context instead of module-level variables, addressing architectural concerns and following the same pattern established in openshift#15946. Changes: - Created MoveNodeHandlersProvider using React Context - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Refactored moveNodeToGroup to use context-based handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests, all passing) Benefits: - Eliminates global state race conditions - Follows React Context pattern from error modal handler (openshift#15946) - Improves testability with mockable context - Single provider initialization around Topology component - Proper cleanup via React lifecycle - Type-safe with clear error messages - Maintains backward compatibility via deprecated hooks The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), similar to the error modal handler pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@rhamilto: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Summary
Creates a shared error modal launcher utility in the console-shared package using React Context to eliminate code duplication and ensure error modals work correctly across all contexts (topology, standalone pages, import flows).
Note: This PR builds on:
Architecture
React Context Pattern
This implementation reuses the existing
OverlayProviderinstead of creating a new context provider:Dual API for Different Contexts
React Components - Use the hook:
Non-React Code (callbacks, promises, utilities) - Use the function:
Changes
New Shared Utility
packages/console-shared/src/utils/error-modal-handler.tsxSyncErrorModalLauncher- Component that syncs the launcher for non-React contextsuseErrorModalLauncher()- Hook for launching error modals from React componentslaunchErrorModal()- Function for launching from non-React contexts (callbacks, promises)Test Coverage
packages/console-shared/src/utils/__tests__/error-modal-handler.spec.tsxpackages/console-shared/src/utils/__mocks__/error-modal-handler.tsxSingle Initialization Point
public/components/app.tsx<SyncErrorModalLauncher />insideOverlayProviderRemoved Multiple Initialization Points
Removed
useSetupGlobalErrorModalLauncher()calls from:packages/topology/src/components/graph-view/Topology.tsxpackages/shipwright-plugin/src/components/buildrun-details/BuildRunDetailsPage.tsxpackages/dev-console/src/components/import/ImportPage.tsxpackages/dev-console/src/components/import/DeployImagePage.tsxMigrated to New API
Updated
launchGlobalErrorModal→launchErrorModalin:Cleanup
errorModalHandlerimplementationerrorModalfunction frompublic/components/modalsBenefits
Test Plan
Automated Tests
Manual Testing
useErrorModalLauncher()hooklaunchErrorModal()functionTechnical Details
Hybrid Approach
The implementation uses a hybrid pattern to support both React and non-React code:
useErrorModalLauncher()hook which directly usesuseOverlay()launchErrorModal()function which accesses a module-level referenceSyncErrorModalLaunchersyncs the launcher to the module-level variableThis approach:
Migration from Global State
Before:
After:
app.tsx)Related
Jira: CONSOLE-5012
🤖 Generated with Claude Code