CONSOLE-5012: Create shared error modal launcher using React Context#15946
CONSOLE-5012: Create shared error modal launcher using React Context#15946rhamilto wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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. |
openshift-ci
bot
added
component/core
rhamilto
changed the title
rhamilto
force-pushed
the
CONSOLE-5012-4
branch
from
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. |
rhamilto
force-pushed
the
CONSOLE-5012-4
branch
from
0bbf1a7 to
cd8a85e
Compare
openshift-ci
bot
added
the
kind/i18n
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 |
rhamilto
force-pushed
the
CONSOLE-5012-4
branch
from
cd8a85e to
d5aa79b
Compare
…veNodeToGroup This commit completes the migration of confirmModal to the modern overlay pattern and refactors moveNodeToGroup to use React Context, eliminating global state race conditions and improving testability. ## Changes ### Removed deprecated confirmModal - Deleted confirm-modal.tsx and its imperative launcher - Removed confirmModal from modals/index.ts exports - Removed translation key from public.json ### Migrated to useWarningModal overlay pattern - Updated topology moveNodeToGroup to use useWarningModal with proper cancel handling - Updated StopNodeMaintenanceModal to remove imperative API, keeping only hook-based approach - Updated all metal3-plugin maintenance components to use hook-based approach - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread ### Refactored moveNodeToGroup to use React Context - Created MoveNodeHandlersProvider using React Context pattern - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests covering all scenarios) ### Fixed accessibility violation - Blur active element before launching confirmation modal - Prevents focus from remaining on SVG elements in topology graph - Fixes browser warning: "Blocked aria-hidden on an element because its descendant retained focus" ## Benefits - Eliminates global state race conditions - Follows React Context pattern established in 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 - Resolves accessibility violations with aria-hidden elements The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), consistent with the error modal handler pattern. 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. |
|
/retest |
frontend/packages/console-shared/src/utils/__mocks__/error-modal-handler.tsx
Outdated
Show resolved
Hide resolved
| let moduleErrorModalLauncher: ((props: ErrorModalProps) => void) | null = null; | ||
|
|
||
| /** | ||
| * Component that syncs the error modal launcher to module-level for non-React contexts. |
There was a problem hiding this comment.
Can we expect to eventually remove all uses of error modal which are outside of React render flow?
i.e. do we have lots of cases where the error modal is used outside of a React component?
There was a problem hiding this comment.
Claude says
# Error Modal Non-React Usage Report
## Summary
This report documents all locations where error modals are used outside React contexts in the OpenShift Console codebase. These usages justify the hybrid approach with both `useErrorModalLauncher()` hook (for React components) and `launchErrorModal()` function (for non-React contexts).
**Total: 10 non-React usage locations across 7 files**
## Detailed Breakdown
### 1. Topology - Move Node to Group (2 usages)
**Files:**
- `packages/topology/src/utils/moveNodeToGroup.tsx:62`
- `packages/topology/src/utils/moveNodeToGroup.tsx:76`
**Context:**
- Used in `.catch()` callbacks within promise chains
- Called from drag-and-drop `end` event handler (non-React context)
- Executes when moving nodes between application groups fails
**Code Example:**
```typescript
updateTopologyResourceApplication(node, targetGroup?.getLabel())
.catch((err) => {
const error = err.message;
if (onError) {
onError(error);
} else {
launchErrorModal({ error });
}
});
Why Non-React Context?
- Callback from PatternFly Topology drag source spec
- Executes outside React render cycle
- No access to hooks
2. Topology - Create Connection
File: packages/topology/src/components/graph-view/components/componentUtils.ts:350
Context:
- Used in
.catch()callback when creating visual connectors - Called from
createVisualConnector()utility function - Triggered during drag-and-drop connection creation
Code Example:
createConnection(source, target, null).catch((error) => {
launchErrorModal({
title: i18next.t('topology~Error creating connection'),
error: error.message,
});
});Why Non-React Context?
- Called from drag-and-drop callback
- Utility function, not a React component
- Asynchronous promise chain
3. Topology - Move Edge Connection
File: packages/topology/src/components/graph-view/components/componentUtils.ts:313
Context:
- Used in edge drag source spec
endhandler - Executes when moving connector edges between nodes
Code Example:
callback(edge.getSource(), dropResult, edge.getTarget()).catch((error) => {
launchErrorModal({ title, error: error.message });
});Why Non-React Context?
- Drag source spec callback (imperative API)
- Outside React component lifecycle
- No hook access
4. Topology - Delete Edge
File: packages/topology/src/actions/edgeActions.ts:106
Context:
- Used in confirmation modal's
onConfirmcallback - Executes after user confirms edge deletion
- Promise chain error handler
Code Example:
onConfirm: () => {
return removeTopologyResourceConnection(element, resource).catch((err) => {
if (err) {
launchErrorModal({
title: t('topology~Error deleting connector'),
error: err.message,
});
}
});
}Why Non-React Context?
- Modal callback (outside normal React flow)
- Asynchronous operation
- Error handling in promise chain
5. Knative - Event Source Sink Connection
File: packages/knative-plugin/src/topology/components/knativeComponentUtils.ts:191
Context:
- Edge drag source spec for event sources
- Moves event source sink connections
Code Example:
createSinkConnection(edge.getSource(), dropResult).catch((error) => {
launchErrorModal({
title: i18next.t('knative-plugin~Error moving event source sink'),
error: error.message,
});
});Why Non-React Context?
- Drag source
endhandler - Imperative callback API
- No React context available
6. Knative - Kafka Connection (Drag)
File: packages/knative-plugin/src/topology/components/knativeComponentUtils.ts:224
Context:
- Edge drag source spec for Kafka connections
- Handles moving Kafka connector edges
Code Example:
createEventSourceKafkaConnection(edge.getSource(), dropResult).catch((error) => {
launchErrorModal({
title: i18next.t('knative-plugin~Error moving event source kafka connector'),
error: error?.message,
});
});Why Non-React Context?
- Drag source callback
- Outside React render
- Asynchronous operation
7. Knative - PubSub Connection
File: packages/knative-plugin/src/topology/components/knativeComponentUtils.ts:261
Context:
- Edge drag source spec for PubSub connections
- Error handling for sink PubSub connections
Code Example:
createSinkPubSubConnection(edge, dropResult).catch((error) => {
launchErrorModal({
title: i18next.t('knative-plugin~Error while sink'),
error: error.message,
});
});Why Non-React Context?
- Drag source spec callback
- Imperative API
- No hook support
8. Knative - Create Kafka Connection (Utility)
File: packages/knative-plugin/src/topology/create-connector-utils.ts:18
Context:
- Utility function for creating Kafka connections
- Called from topology connector creation flow
Code Example:
const createKafkaConnection = (source: Node, target: Node) =>
createEventSourceKafkaConnection(source, target)
.then(() => null)
.catch((error) => {
launchErrorModal({
title: i18next.t('knative-plugin~Error moving event source kafka connector'),
error: error.message,
});
});Why Non-React Context?
- Pure utility function
- Called from multiple contexts
- Cannot use React hooks
9. Shipwright - Download Logs
File: packages/shipwright-plugin/src/components/logs/logs-utils.ts:84
Context:
- Async utility function for downloading build run logs
- Called by React components but error handling is in utility layer
Code Example:
export const getBuildRunPodLogs = (buildRun: TaskRunKind): Promise<ContainerLogOptions[]> => {
return k8sGet<PodKind>(PodModel, podName, ns)
.then((pod) => {
return /* process logs */;
})
.catch((err) => {
launchErrorModal({
error: err.message || i18next.t('shipwright-plugin~Error downloading logs.'),
});
return [];
});
};Why Non-React Context?
- Async utility function
- Promise chain error handler
- Shared by multiple components
10. Dev Console - Expose Route
File: packages/dev-console/src/components/pipeline-section/pipeline/pipeline-template-utils.ts:931
Context:
- Async utility for exposing webhook routes
- Error handling in try/catch block
Code Example:
export const exposeWebhookRoute = async (/* params */) => {
try {
const route = createRoute(/* params */);
await k8sCreate(RouteModel, route, { ns });
} catch (e) {
launchErrorModal({
title: 'Error Exposing Route',
error: e.message || 'Unknown error exposing the Webhook route',
});
}
};Why Non-React Context?
- Async utility function
- Try/catch error handling
- Called from multiple contexts
Usage Categories
By Type
| Category | Count | Percentage |
|---|---|---|
| Drag/Drop Operations | 7 | 70% |
| Async Utility Functions | 2 | 20% |
| User-Triggered Actions | 1 | 10% |
| Total | 10 | 100% |
By Package
| Package | Count | Files |
|---|---|---|
| topology | 4 | 3 files |
| knative-plugin | 4 | 2 files |
| shipwright-plugin | 1 | 1 file |
| dev-console | 1 | 1 file |
| Total | 10 | 7 files |
Can These Be Refactored to React Contexts?
Short Answer: No, most cannot be easily refactored.
Detailed Analysis
Drag/Drop Operations (7/10 usages)
- ❌ Cannot refactor - These are event handlers from PatternFly's topology library
- Execute outside React's render cycle
- Callbacks in imperative third-party APIs
- No access to React hooks or context
Async Utility Functions (2/10 usages)
- ❌ Should not refactor - These are pure utility functions
- Called from multiple contexts (some React, some not)
- Maintaining as utilities provides better separation of concerns
- Would require passing error handlers through multiple layers
Modal Callbacks (1/10 usage)
⚠️ Difficult to refactor - Confirmation modal callbacks- Execute outside normal React flow
- Would require restructuring modal architecture
Why the Hybrid Approach is Necessary
The codebase requires both patterns:
useErrorModalLauncher() Hook (React Context)
✅ Use in React components
✅ Direct access to useOverlay()
✅ Type-safe, idiomatic React
launchErrorModal() Function (Module-Level)
✅ Use in imperative callbacks
✅ Use in async utility functions
✅ Use in third-party library callbacks
✅ Use in promise chains
Architecture Benefits
- Single Source of Truth: Both APIs use the same underlying
OverlayProvider - Proper Lifecycle:
SyncErrorModalLaunchermanages initialization/cleanup - No Duplication: Shared implementation across all contexts
- Type Safety: Both APIs use the same
ErrorModalPropsinterface - Testable: Mock implementation supports both patterns
Conclusion
The current implementation correctly addresses a real architectural need:
- 10 locations require non-React error modal launching
- 70% are drag/drop operations (imperative callbacks)
- Cannot be easily refactored to pure React patterns without major architectural changes
The hybrid approach with SyncErrorModalLauncher + module-level reference is the appropriate solution for this codebase's requirements.
Related
- PR: #15946
- Jira: CONSOLE-5012
- Implementation:
packages/console-shared/src/utils/error-modal-handler.tsx
Report generated: 2026-02-03
ed87acf to
9360bd7
Compare
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
…veNodeToGroup This commit completes the migration of confirmModal to the modern overlay pattern and refactors moveNodeToGroup to use React Context, eliminating global state race conditions and improving testability. - Deleted confirm-modal.tsx and its imperative launcher - Removed confirmModal from modals/index.ts exports - Removed translation key from public.json - Updated topology moveNodeToGroup to use useWarningModal with proper cancel handling - Updated StopNodeMaintenanceModal to remove imperative API, keeping only hook-based approach - Updated all metal3-plugin maintenance components to use hook-based approach - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread - Created MoveNodeHandlersProvider using React Context pattern - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests covering all scenarios) - Blur active element before launching confirmation modal - Prevents focus from remaining on SVG elements in topology graph - Fixes browser warning: "Blocked aria-hidden on an element because its descendant retained focus" - Eliminates global state race conditions - Follows React Context pattern established in 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 - Resolves accessibility violations with aria-hidden elements The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), consistent with the error modal handler pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…veNodeToGroup This commit completes the migration of confirmModal to the modern overlay pattern and refactors moveNodeToGroup to use React Context, eliminating global state race conditions and improving testability. - Deleted confirm-modal.tsx and its imperative launcher - Removed confirmModal from modals/index.ts exports - Removed translation key from public.json - Updated topology moveNodeToGroup to use useWarningModal with proper cancel handling - Updated StopNodeMaintenanceModal to remove imperative API, keeping only hook-based approach - Updated all metal3-plugin maintenance components to use hook-based approach - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread - Created MoveNodeHandlersProvider using React Context pattern - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests covering all scenarios) - Blur active element before launching confirmation modal - Prevents focus from remaining on SVG elements in topology graph - Fixes browser warning: "Blocked aria-hidden on an element because its descendant retained focus" - Eliminates global state race conditions - Follows React Context pattern established in 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 - Resolves accessibility violations with aria-hidden elements The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), consistent with the error modal handler pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…veNodeToGroup This commit completes the migration of confirmModal to the modern overlay pattern and refactors moveNodeToGroup to use React Context, eliminating global state race conditions and improving testability. - Deleted confirm-modal.tsx and its imperative launcher - Removed confirmModal from modals/index.ts exports - Removed translation key from public.json - Updated topology moveNodeToGroup to use useWarningModal with proper cancel handling - Updated StopNodeMaintenanceModal to remove imperative API, keeping only hook-based approach - Updated all metal3-plugin maintenance components to use hook-based approach - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread - Created MoveNodeHandlersProvider using React Context pattern - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests covering all scenarios) - Blur active element before launching confirmation modal - Prevents focus from remaining on SVG elements in topology graph - Fixes browser warning: "Blocked aria-hidden on an element because its descendant retained focus" - Eliminates global state race conditions - Follows React Context pattern established in 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 - Resolves accessibility violations with aria-hidden elements The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), consistent with the error modal handler pattern. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
/retest |
|
/lgtm |
|
@yanpzhan, this is ready for verification when you are. |
- 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__/error-modal-handler.tsx - Mock utilities for testing - Remove deprecated errorModal() function from public/components/modals This establishes a React Context-based pattern for error modals that eliminates global state race conditions, improves testability, and provides a consistent API across all packages. The hybrid approach supports both React hooks and imperative API calls via module-level reference with proper cleanup via React lifecycle. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5012-4
branch
from
9360bd7 to
ca5fb20
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, rhamilto, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis change introduces a new centralized error modal handler utility providing both React and non-React interfaces for error modal launching. A new 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…veNodeToGroup This commit completes the migration of confirmModal to the modern overlay pattern and refactors moveNodeToGroup to use React Context, eliminating global state race conditions and improving testability. - Deleted confirm-modal.tsx and its imperative launcher - Removed confirmModal from modals/index.ts exports - Removed translation key from public.json - Updated topology moveNodeToGroup to use useWarningModal with proper cancel handling - Updated StopNodeMaintenanceModal to remove imperative API, keeping only hook-based approach - Updated all metal3-plugin maintenance components to use hook-based approach - Fixed useWarningModal to properly call closeOverlay() and filter it from props spread - Created MoveNodeHandlersProvider using React Context pattern - Created SyncMoveNodeHandlers component to sync handlers for non-React contexts - Added useMoveNodeHandlers() hook for accessing handlers - Wrapped Topology component with MoveNodeHandlersProvider - Deprecated old useSetupMoveNodeToGroupHandlers hooks with warnings - Added comprehensive unit tests (8 tests covering all scenarios) - Blur active element before launching confirmation modal - Prevents focus from remaining on SVG elements in topology graph - Fixes browser warning: "Blocked aria-hidden on an element because its descendant retained focus" - Eliminates global state race conditions - Follows React Context pattern established in 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 - Resolves accessibility violations with aria-hidden elements The hybrid approach (Context + module reference) supports both React components (via useMoveNodeHandlers hook) and non-React drag-drop callbacks (via moveNodeToGroup function), consistent with 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 |
6 participants
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).
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 />insideOverlayProviderMigrated to Shared Pattern
packages/topology/src/utils/moveNodeToGroup.tsxglobalErrorHandlerto sharedlaunchErrorModal()setMoveNodeToGroupErrorHandler()anduseSetupMoveNodeToGroupErrorHandler()useMoveNodeToGroup()hookpackages/topology/src/components/graph-view/Topology.tsxuseSetupMoveNodeToGroupErrorHandler()initialization (no longer needed)Migrated to New API
Updated to use
launchErrorModalin:Cleanup
errorModalfunction frompublic/components/modalsresetErrorModalMocks()helper from mock fileBenefits
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 Path
Before:
errorModalfunctionAfter:
app.tsx)Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor