CONSOLE-5012: Migrate remaining modals to useOverlay pattern#15990
CONSOLE-5012: Migrate remaining modals to useOverlay pattern#15990openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
Conversation
rhamilto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| ); | ||
| export const labelsModalLauncher = createModalLauncher<LabelsModalProps>(LabelsModal); | ||
|
|
||
| export const podSelectorModal = createModalLauncher<PodSelectorModalProps>((props) => { |
|
@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-6
branch
from
80f61b0 to
e1b29f9
Compare
📝 WalkthroughWalkthroughThis change refactors the modal system from direct launcher functions to an overlay-based architecture. Modal components are converted to OverlayComponents and accessed via lazy-loaded Lazy*Overlay wrappers through a generic launchModal function. A new error-modal-handler utility enables error modal launches from both React (useErrorModalLauncher hook) and non-React contexts (launchErrorModal function) via a SyncErrorModalLauncher component. PubSub, move-node, and node-maintenance modals are updated to follow the overlay pattern. Multiple components replace direct modal invocations with useOverlay-based launchers. Legacy modal launchers (confirmModal, createModalLauncher exports) are removed across the codebase. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@frontend/packages/dev-console/src/components/pipeline-section/pipeline/__tests__/pipeline-template-utils.spec.ts`:
- Around line 19-23: Remove the unused referenceForModel mock from the jest.mock
block in the test file: delete the referenceForModel property so the mock only
exports k8sCreate and k8sUpdate (to match the actual imported symbols like
k8sCreate and k8sUpdate); this keeps the mocked API surface consistent with the
imports and avoids dead code in the jest.mock declaration.
In
`@frontend/packages/dev-console/src/components/pipeline-section/pipeline/pipeline-template-utils.ts`:
- Around line 931-934: The modal title and fallback message are hardcoded;
update the call to launchErrorModal in pipeline-template-utils.ts to use
i18next.t(...) for the title (e.g., i18next.t('pipelines~Error Exposing Route'))
and for the fallback error string instead of 'Unknown error exposing the Webhook
route'; ensure i18next is imported where launchErrorModal is called and pass the
translated strings, keeping the existing use of e.message when present.
In
`@frontend/packages/knative-plugin/src/components/pub-sub/PubSubModalLauncher.ts`:
- Around line 31-37: The Promise returned by addPubSubConnectionModal (created
in launchPubSubModalRef) can remain unresolved when the overlay is dismissed via
ModalWrapper.onClose (Escape/outside click); update the modal overlay logic so
the Promise is always settled: in the object passed to launchPubSubModalRef for
PubSubModalOverlay, add a settled-tracking wrapper around the provided
close/cancel handlers and expose a cleanup handler (e.g., onUnmount or wrap
props.closeOverlay) that checks the settled flag and calls reject(new
Error('User cancelled via overlay')) if not already settled; implement the
tracking in PubSubModalOverlay (or the wrapper you pass in) so
unmount/ModalWrapper.onClose triggers that cleanup to reject the Promise when
dismissed outside of explicit close() or cancel().
In
`@frontend/packages/knative-plugin/src/topology/components/knativeComponentUtils.ts`:
- Around line 261-264: The error modal title is grammatically incorrect in the
launchErrorModal call using i18next.t('knative-plugin~Error while sink'); update
the message to a clear, grammatical string (e.g., "Error while creating sink" or
"Error configuring sink") and adjust the i18n key/value accordingly so
launchErrorModal({ title: i18next.t('knative-plugin~...'), error: error.message
}) shows the corrected title; ensure you update the corresponding translation
entry for the chosen key.
In `@frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx`:
- Around line 47-48: The memoized actions array uses the
startNodeMaintenanceModal launcher but it's missing from the useMemo dependency
list; update the useMemo dependencies to include the startNodeMaintenanceModal
reference returned by useStartNodeMaintenanceModalLauncher so the memoized
callback re-computes when the launcher changes. Locate the useMemo call that
currently depends on [maintenances, resource.metadata.name, t,
stopNodeMaintenanceModalLauncher] and add startNodeMaintenanceModal (the value
from useStartNodeMaintenanceModalLauncher) to that array to avoid stale
closures.
In
`@frontend/packages/topology/src/components/graph-view/components/componentUtils.ts`:
- Around line 139-155: The catch currently swallows all errors from
moveNodeToGroup; change it to distinguish user cancellation from real failures
by inspecting the thrown error (e.g., error.name === 'AbortError' or a
project-specific CancelledError/flag) and only ignore those; for other errors
surface them via the app's notification/logger and rethrow or return a failure
so the UI can show feedback. Update the catch block surrounding moveNodeToGroup
(and the subsequent optimistic action involving controller.getNodeById and
dropResult.appendChild) to log/notify on non-cancellation errors and avoid
silently swallowing them.
🧹 Nitpick comments (7)
frontend/public/components/modals/error-modal.tsx (1)
18-37: RemoveModalErrorContent— unused legacy pattern.
ModalErrorContentimplements the older modal factory pattern and is not referenced elsewhere in the codebase.ErrorModalprovides the modern overlay-based approach. Removing the legacy component reduces maintenance burden and clarifies the single source of truth for error modal behavior.frontend/packages/knative-plugin/src/components/pub-sub/PubSubModalLauncher.ts (2)
21-21: Add TypeScript typing forpropsparameter.The
propsparameter lacks type annotation, making itany. This reduces type safety and IDE support. Consider typing it properly based onPubSubModalOverlay's expected props.-export const addPubSubConnectionModal = (props) => { +export const addPubSubConnectionModal = (props: Omit<Parameters<typeof import('./PubSubController').PubSubModalOverlay>[0], 'close' | 'cancel'>) => {Alternatively, define a dedicated type and export it for consumers.
22-39: Potential race condition between launcher check and invocation.There's a window between checking
launchPubSubModalRefon line 23 and using it on line 32 where the ref could be nullified (e.g., ifSyncPubSubModalLauncherunmounts). While unlikely in practice, consider capturing the ref in a local variable for safety.export const addPubSubConnectionModal = (props) => { return import('./PubSubController' /* webpackChunkName: "pub-sub-connectors" */).then((m) => { - if (!launchPubSubModalRef) { + const launcher = launchPubSubModalRef; + if (!launcher) { // eslint-disable-next-line no-console console.error( 'PubSub modal launcher not initialized. Make sure SyncPubSubModalLauncher is rendered in the topology component tree.', ); return Promise.reject(new Error('Modal launcher not available')); } return new Promise<void>((resolve, reject) => { - launchPubSubModalRef(m.PubSubModalOverlay, { + launcher(m.PubSubModalOverlay, { ...props,frontend/packages/topology/src/actions/edgeActions.ts (1)
104-111: Redundantif (err)check in catch block.The
if (err)conditional is unnecessary since the.catch()handler already implies an error occurred. While technically safe, it's misleading—Promise rejections can legitimately passundefinedas the rejection reason, and this check would silently skip showing the error modal in that edge case.Consider removing the conditional or handling the undefined case explicitly:
return removeTopologyResourceConnection(element, resource).catch((err) => { - if (err) { - launchErrorModal({ - title: t('topology~Error deleting connector'), - error: err.message, - }); - } + launchErrorModal({ + title: t('topology~Error deleting connector'), + error: err?.message || t('topology~An unknown error occurred'), + }); });frontend/packages/knative-plugin/src/topology/components/knativeComponentUtils.ts (1)
190-195: Inconsistent optional chaining for error.message access.Line 226 uses
error?.message(defensive) while lines 193 and 263 useerror.messagedirectly. Iferrorcould ever beundefinedor lack amessageproperty, the latter would throw. Standardize the approach:createSinkConnection(edge.getSource(), dropResult).catch((error) => { launchErrorModal({ title: i18next.t('knative-plugin~Error moving event source sink'), - error: error.message, + error: error?.message, }); });Apply the same pattern to line 263.
Also applies to: 223-228, 260-265
frontend/packages/topology/src/utils/__tests__/moveNodeToGroup.spec.tsx (1)
88-99: Incomplete test coverage for handler cleanup verification.The test acknowledges cleanup "is tested indirectly through the error case" but doesn't actually verify the cleanup occurred. After unmount, a subsequent call to
moveNodeToGroupshould throw "Move node handlers not initialized" - that would be a concrete verification.💡 Suggested enhancement for explicit cleanup verification
it('should cleanup handlers on unmount', () => { const { unmount } = render( <MoveNodeHandlersProvider> <SyncMoveNodeHandlers /> </MoveNodeHandlersProvider>, ); unmount(); - // After unmount, handlers should be cleaned up - // This is tested indirectly through the error case + // After unmount, moveNodeToGroup should throw "not initialized" + const mockNode = { + getId: () => 'test', + getLabel: () => 'Test', + getParent: () => ({ getId: () => 'graph' }), + getGraph: () => ({ getId: () => 'graph' }), + }; + await expect(moveNodeToGroup(mockNode as Node, null)).rejects.toThrow( + 'Move node handlers not initialized', + ); });frontend/packages/topology/src/utils/moveNodeToGroup.tsx (1)
118-203: Defer handler initialization until confirmation is required.
Right now the function throws even when the node has no source group (no confirmation needed). This makes non‑group moves unnecessarily dependent on handler initialization.♻️ Suggested change
- const handlers = moduleHandlers; - if (!handlers) { - throw new Error( - 'Move node handlers not initialized. Ensure MoveNodeHandlersProvider is mounted.', - ); - } - - // If moving from a group, show confirmation - if (sourceGroup) { + // If moving from a group, show confirmation + if (sourceGroup) { + const handlers = moduleHandlers; + if (!handlers) { + throw new Error( + 'Move node handlers not initialized. Ensure MoveNodeHandlersProvider is mounted.', + ); + }
...v-console/src/components/pipeline-section/pipeline/__tests__/pipeline-template-utils.spec.ts
Show resolved
Hide resolved
...end/packages/dev-console/src/components/pipeline-section/pipeline/pipeline-template-utils.ts
Outdated
Show resolved
Hide resolved
frontend/packages/knative-plugin/src/components/pub-sub/PubSubModalLauncher.ts
Show resolved
Hide resolved
frontend/packages/knative-plugin/src/topology/components/knativeComponentUtils.ts
Outdated
Show resolved
Hide resolved
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [maintenances, resource.metadata.name, t]); | ||
| }, [maintenances, resource.metadata.name, t, stopNodeMaintenanceModalLauncher]); |
There was a problem hiding this comment.
Missing startNodeMaintenanceModal in useMemo dependencies.
The startNodeMaintenanceModal launcher is used in the memoized callback (line 34) but isn't included in the dependency array. If the launcher reference changes (e.g., when resource.metadata.name changes and useStartNodeMaintenanceModalLauncher returns a new function), the memoized action will hold a stale reference. Since you've already added stopNodeMaintenanceModalLauncher, adding the start launcher keeps behavior symmetric and correct.
🔧 Proposed fix
return [action];
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [maintenances, resource.metadata.name, t, stopNodeMaintenanceModalLauncher]);
+ }, [maintenances, resource.metadata.name, t, stopNodeMaintenanceModalLauncher, startNodeMaintenanceModal]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [maintenances, resource.metadata.name, t]); | |
| }, [maintenances, resource.metadata.name, t, stopNodeMaintenanceModalLauncher]); | |
| }, [maintenances, resource.metadata.name, t, stopNodeMaintenanceModalLauncher, startNodeMaintenanceModal]); |
🤖 Prompt for AI Agents
In `@frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx`
around lines 47 - 48, The memoized actions array uses the
startNodeMaintenanceModal launcher but it's missing from the useMemo dependency
list; update the useMemo dependencies to include the startNodeMaintenanceModal
reference returned by useStartNodeMaintenanceModalLauncher so the memoized
callback re-computes when the launcher changes. Locate the useMemo call that
currently depends on [maintenances, resource.metadata.name, t,
stopNodeMaintenanceModalLauncher] and add startNodeMaintenanceModal (the value
from useStartNodeMaintenanceModalLauncher) to that array to avoid stale
closures.
frontend/packages/topology/src/components/graph-view/components/componentUtils.ts
Outdated
Show resolved
Hide resolved
openshift-ci
bot
added
component/dev-console
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
force-pushed
the
CONSOLE-5012-6
branch
3 times, most recently
from
d8a4c5a to
ae6ccb5
Compare
|
/retest |
1 similar comment
|
/retest |
rhamilto
changed the title
This change migrates several modals from the deprecated createModalLauncher pattern to the modern useOverlay/OverlayComponent pattern for better accessibility and consistency. Migrated modals: - Labels and Pod Selector modals - Annotations modal - PubSub modal (with accessibility-safe pattern for non-React contexts) - Alert Routing modal - Column Management modal Key changes: - Created OverlayComponent wrappers for all migrated modals - Exported lazy-loaded overlay components from modals/index.ts - Updated all usages to call launchModal() with useOverlay() hook - Added SyncPubSubModalLauncher for topology connector accessibility - Maintained user settings HOC compatibility in Column Management modal The PubSub modal uses a module-level reference synced via React Context to support non-React topology connectors while maintaining proper accessibility through the OverlayProvider. All modals follow consistent naming: *ModalOverlay and Lazy*ModalOverlay Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Replace all usages of DeleteOverlay with DeleteModalOverlay for consistency. Remove the deprecated DeleteOverlay component which used the older Backdrop/Modal pattern. Updated files: - useDefaultOperandActions.ts - OLM operand delete action - useOperatorActions.ts - OLM CSV delete action - host-actions-provider.ts - Metal3 bare metal host delete action - delete-modal.tsx - removed DeleteOverlay component All delete actions now use the modern OverlayComponent pattern through DeleteModalOverlay. Also updated variable naming from launcher to launchModal for consistency. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
rhamilto
force-pushed
the
CONSOLE-5012-6
branch
from
35518fc to
3a4fcbc
Compare
| ); | ||
| }; | ||
|
|
||
| export const DeleteOverlay: FC<DeleteModalProps> = (props) => { |
There was a problem hiding this comment.
This is a better pattern using newer components, but the goal is to get to a single delete modal knowing we will refactor the old one in future work.
|
This PR is not dependent on /assign @jhadvig Tech debt, so assigning labels |
openshift-ci
bot
added
px-approved
|
/unassign @vojtechszocs |
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, 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 |
jhadvig
added
the
plugin-api-approved
|
Regression tests about modals on cluster launched against the pr passed. |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@rhamilto: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Summary
Migrates several modals from the deprecated
createModalLauncherpattern to the modernuseOverlay/OverlayComponentpattern for better accessibility and consistency. Also consolidates delete modal implementations.Migrated modals:
DeleteOverlay, unified onDeleteModalOverlay)Key changes:
OverlayComponentwrappers for all migrated modalsmodals/index.tslaunchModal()withuseOverlay()hookSyncPubSubModalLauncherfor topology connector accessibilityDeleteOverlaycomponentDeleteModalOverlayconst launchModal = useOverlay()throughoutThe PubSub modal uses a module-level reference synced via React Context to support non-React topology connectors while maintaining proper accessibility through the OverlayProvider.
All modals follow consistent naming:
*ModalOverlayandLazy*ModalOverlayFiles modified:
Core modal migrations:
public/components/modals/labels-modal.tsx- Labels and Pod Selector modalspublic/components/modals/tags.tsx- Annotations modalpublic/components/modals/alert-routing-modal.tsx- Alert Routing modalpublic/components/modals/column-management-modal.tsx- Column Management modalpublic/components/modals/index.ts- Lazy-loaded exportspackages/knative-plugin/src/components/pub-sub/PubSubController.tsx- PubSub modalpackages/knative-plugin/src/components/pub-sub/PubSubModalLauncher.ts- PubSub launcherDelete modal consolidation:
public/components/modals/delete-modal.tsx- RemovedDeleteOverlay, keptDeleteModalOverlaypackages/operator-lifecycle-manager/src/actions/useDefaultOperandActions.ts- OLM operand deletepackages/operator-lifecycle-manager/src/actions/useOperatorActions.ts- OLM CSV deletepackages/metal3-plugin/src/components/baremetal-hosts/host-actions-provider.ts- Metal3 host deleteUsage updates:
packages/console-app/src/actions/hooks/useCommonActions.ts- Common actions for labels/annotationspackages/console-app/src/components/data-view/ConsoleDataView.tsx- Column managementpublic/components/filter-toolbar.tsx- Column managementpublic/components/monitoring/alertmanager/alertmanager-config.tsx- Alert routingpackages/topology/src/components/graph-view/Topology.tsx- PubSub launcher syncRelated PRs:
Test plan
🤖 Generated with Claude Code