[WIP] CONSOLE-5012: Migrate confirmModal to overlay pattern #15948
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
component/core
|
@rhamilto: This pull request references CONSOLE-5012 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request introduces a modal infrastructure refactor across the OpenShift Console codebase, shifting from a direct modal launcher pattern (via 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
frontend/packages/console-shared/src/components/formik-fields/EnvironmentField.tsx (1)
64-80: StabilizelaunchModalreference instead of suppressing hook depsThe dependency exhaustive-deps suppression masks a real issue:
useOverlay()returns an unstable reference becauseOverlayContext.Providercreates a new value object on every render, even though the underlying callback is memoized. AddinglaunchModalto deps would trigger infinite re-runs (max depth exceeded), so the ref pattern below is the right approach at the consumer level.♻️ Proposed refactor
-import { useMemo, useState, useCallback, useEffect } from 'react'; +import { useMemo, useState, useCallback, useEffect, useRef } from 'react'; const { t } = useTranslation(); const launchModal = useOverlay(); +const launchModalRef = useRef(launchModal); +useEffect(() => { + launchModalRef.current = launchModal; +}, [launchModal]); useEffect(() => { Promise.all([k8sGet(ConfigMapModel, null, namespace), k8sGet(SecretModel, null, namespace)]) .then(([nsConfigMaps, nsSecrets]) => { setConfigMaps(nsConfigMaps); setSecrets(nsSecrets); }) .catch(async (err) => { if (err?.response?.status !== 403) { try { - await launchModal(ErrorModal, { error: err?.message }); + await launchModalRef.current(ErrorModal, { error: err?.message }); // eslint-disable-next-line no-empty } catch (e) {} } }); - // missing launchModal dependency, that causes max depth exceeded error - // eslint-disable-next-line react-hooks/exhaustive-deps }, [namespace]);frontend/packages/topology/src/utils/moveNodeToGroup.tsx (1)
41-108: Fix the type contract:targetGroupshould be optional andupdateTopologyResourceApplicationmust acceptstring | null.The function declares
targetGroup: Nodeas required, but the implementation treats it as optional (see line 54'stargetGroup?.getLabel()and line 58's conditional check). Line 82 passesnulltoupdateTopologyResourceApplication, which expectsapplication: stringper the function signature in topology-utils.ts:137–140. Additionally, line 102 will crash ifsourceGroupis falsy andtargetGroupis somehow undefined, calling.getLabel()on a falsy value.Update the type signature to match the implementation, and widen
updateTopologyResourceApplication'sapplicationparameter tostring | nullto safely handle the "remove from application" case:Proposed fix
export const moveNodeToGroup = ( node: Node, - targetGroup: Node, + targetGroup: Node | null, onError?: (error: string) => void, ): Promise<void> => {Then in topology-utils.ts:
export const updateTopologyResourceApplication = ( item: Node, - application: string, + application: string | null, ): Promise<any> => {and add an early guard:
const itemData = item?.getData(); const resource = getResource(item); - if (!item || !resource || !_.size(itemData.resources)) { + if (!item || !resource || application === null || !_.size(itemData.resources)) { return Promise.reject(); }frontend/packages/shipwright-plugin/src/actions/useBuildRunActions.ts (1)
4-4: Typo: Double slash in import path.The import path contains
actions//hookswith a double slash. While most bundlers normalize this, it's a minor inconsistency that should be cleaned up.Suggested fix
-import { useCommonResourceActions } from '@console/app/src/actions//hooks/useCommonResourceActions'; +import { useCommonResourceActions } from '@console/app/src/actions/hooks/useCommonResourceActions';frontend/packages/shipwright-plugin/src/actions/useBuildActions.ts (1)
75-89: Remove the duplicate Delete action at line 89.The
useCommonResourceActionshook returns all common actions including Delete viaObject.values(commonActions)at line 75. Explicitly pushingcommonActions.Deleteagain at line 89 creates a duplicate entry in the actions menu.
🤖 Fix all issues with AI agents
In
`@frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx`:
- Around line 279-283: The spread props currently come after explicit close
props in ClonePVCModalProvider which allows props.closeOverlay to be overridden;
update the JSX so {...props} is spread first and then pass
cancel={props.closeOverlay} and close={props.closeOverlay} (affecting the
ClonePVCModal inside ModalWrapper) to ensure closeOverlay always wins and cannot
be overridden by incoming props.
In
`@frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx`:
- Around line 282-287: The provider currently spreads props after passing
cancel/close to RestorePVCModal, allowing a caller to override those with their
own cancel/close; change RestorePVCModalProvider so you destructure closeOverlay
from props first (e.g., const { closeOverlay, ...rest } = props) then render
<RestorePVCModal cancel={closeOverlay} close={closeOverlay} {...rest} /> inside
ModalWrapper, ensuring props.closeOverlay cannot be overridden by
caller-supplied props.
In
`@frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx`:
- Around line 51-64: The provider currently spreads props after assigning cancel
and close which allows caller props to override them; update
ConfigureUnschedulableModalProvider to destructure props into { closeOverlay,
...rest } (or extract known keys) and then render ConfigureUnschedulableModal
with {...rest} followed by explicit cancel={closeOverlay} and
close={closeOverlay} so the overlay's close handlers cannot be overridden;
ensure ModalWrapper still uses closeOverlay for onClose and pass the same
closeOverlay to ConfigureUnschedulableModal.
In `@frontend/packages/console-shared/src/utils/error-modal-handler.ts`:
- Around line 35-50: The cleanup unconditionally clears the global launcher
causing newer launchers to be removed; fix useSetupGlobalErrorModalLauncher by
capturing the specific errorModalLauncher reference you set via
setGlobalErrorModalLauncher and, in the return cleanup, only call
setGlobalErrorModalLauncher(null) if the current global launcher equals that
captured reference (so use a stable local/ref for errorModalLauncher and guard
the cleanup), referencing the existing functions errorModalLauncher,
useSetupGlobalErrorModalLauncher, and setGlobalErrorModalLauncher to locate and
implement the guarded cleanup.
In `@frontend/packages/knative-plugin/src/actions/creators.ts`:
- Around line 4-5: The SDK currently forces imports from an internal path to
reach useOverlay; update the SDK public exports so useOverlay can be imported
from '@console/dynamic-plugin-sdk' by re-exporting the modal overlay API: add an
export for the modal-support module (e.g., export * from './modal-support') from
the SDK app index or alternatively export the dynamic-core-api module from the
SDK root so that the symbol useOverlay is publicly available; modify the module
that aggregates app exports to re-export useOverlay (and related modal types) so
consumers can import { useOverlay } from '@console/dynamic-plugin-sdk'.
In `@frontend/public/components/modals/configure-update-strategy-modal.tsx`:
- Around line 241-250: The provider currently spreads {...props} after setting
cancel/close so a caller can override cancel/close; update
ConfigureUpdateStrategyModalProvider to spread props first into
ConfigureUpdateStrategyModal (or omit cancel/close from the spread) and then
pass cancel={props.closeOverlay} and close={props.closeOverlay} to enforce using
props.closeOverlay; locate the ConfigureUpdateStrategyModalProvider and
ConfigureUpdateStrategyModal usages to apply this change so closeOverlay cannot
be overridden by caller props.
In `@frontend/public/components/modals/delete-modal.tsx`:
- Around line 183-188: The DeleteModalProvider currently spreads props after
passing cancel and close, allowing a caller to override cancel/close; in the
DeleteModalProvider component (and the ModalWrapper usage) spread {...props}
first and then pass cancel={props.closeOverlay} and close={props.closeOverlay}
so the enforced overlay handlers cannot be overridden by caller-provided props
(refer to DeleteModalProvider, ModalWrapper, DeleteModal, and
props.closeOverlay).
In `@frontend/public/components/modals/delete-pvc-modal.tsx`:
- Around line 87-91: The DeletePVCModalProvider currently spreads {...props}
after explicit cancel/close props which allows callers to override
props.closeOverlay; change the prop order in the JSX so you spread {...props}
first and then pass cancel={props.closeOverlay} close={props.closeOverlay} (on
ModalWrapper keep onClose={props.closeOverlay}) to ensure DeletePVCModal and
ModalWrapper always use props.closeOverlay for teardown and cannot be overridden
by incoming props.
♻️ Duplicate comments (2)
frontend/packages/console-app/src/actions/hooks/useJobActions.ts (1)
55-57: Same launcher-dep suppression pattern—please confirm stability.If
useConfigureCountModal()returns a new function over time, omitting it from deps can leave the action stale. This mirrors the earlier launcher-dep concern.frontend/packages/console-app/src/actions/providers/build-config-provider.ts (1)
36-38: Launcher-dep suppression repeated here—confirm the hook’s launcher is stable.Same dependency omission pattern as in other action hooks; please ensure the launcher identity is stable or the action can become stale.
Also applies to: 71-73
🧹 Nitpick comments (16)
frontend/public/components/utils/webhooks.tsx (1)
90-115: TheisLoadeddependency may cause an unnecessary double-fetch.The effect sets
isLoaded = trueat line 111, andisLoadedis also listed in the dependency array. On initial mount:
isLoadedisfalse→ effect runs → secrets fetched →setLoaded(true)isLoadedchanges totrue→ effect re-runs → secrets fetched againConsider adding a guard or removing
isLoadedfrom deps if it's only meant to track completion:useEffect(() => { - if (!canGetSecret) { + if (!canGetSecret || isLoaded) { return; }Alternatively, if the intent is to allow re-fetching when
secretNameschange,isLoadedshould be removed from the dependency array entirely—it's not a meaningful trigger.
On the
launchModalsuppression: I see this pattern across the PR for avoiding the max-depth error. For long-term maintainability, theuseOverlayhook should ideally return a stable (memoized) function reference. If that's not feasible right now, auseRefwrapper is a cleaner alternative to eslint-disable:const launchModalRef = useRef(launchModal); launchModalRef.current = launchModal; // then use launchModalRef.current inside the effectThis keeps the linter happy and documents the intent more explicitly than a suppression comment.
frontend/packages/console-shared/src/hooks/useWarningModal.tsx (1)
37-52: Consider documenting memoization expectations.The dependency array at line 50 includes
props, which is an object reference. If callers pass inline objects (e.g.,useWarningModal({ title: 'Title' })), the callback will be recreated on every render. This is fine for most use cases, but callers needing a stable callback reference should memoize their props.Consider adding a note in the JSDoc (around line 30-36) mentioning this behavior, especially since other parts of the codebase may rely on stable callback references for effect dependencies.
📝 Suggested documentation addition
/** * useWarningModal is a hook that provides a way to launch a WarningModal. * Supports two usage patterns: * - Pass props at hook initialization time: useWarningModal({ title: 'Title' }) * - Pass props in the callback: const launch = useWarningModal(); launch({ title: 'Title' }) * - Mix both (overrides have priority): const launch = useWarningModal({ title: 'Title' }); launch({ children: <Content /> }) + * + * Note: If a stable callback reference is required, memoize the props object passed + * to this hook using useMemo to prevent the callback from being recreated on each render. */frontend/packages/knative-plugin/src/components/add/SecretKeySelector.tsx (1)
51-63: ConfirmuseOverlay()provides a stable function or capture it safely.
Line 51–63 suppresses exhaustive-deps and dropslaunchModal. IfuseOverlay()ever returns a new function (e.g., provider change), this effect will keep a stale launcher and may miss error modals. Please confirm stability; if not guaranteed, capture it via a ref while keeping the namespace-only fetch behavior.♻️ Optional safer pattern (ref keeps latest launcher)
-import { useState, useEffect } from 'react'; +import { useState, useEffect, useRef } from 'react'; ... - const launchModal = useOverlay(); + const launchModal = useOverlay(); + const launchModalRef = useRef(launchModal); + useEffect(() => { + launchModalRef.current = launchModal; + }, [launchModal]); ... - if (err?.response?.status !== 403) { - launchModal(ErrorModal, { error: err?.message }); - } + if (err?.response?.status !== 403) { + launchModalRef.current(ErrorModal, { error: err?.message }); + }frontend/packages/metal3-plugin/src/components/maintenance/actions.tsx (2)
42-42: Minor inconsistency in CTA assignment patternLine 34 assigns
cta: startNodeMaintenanceModaldirectly as a function reference, whereas here you wrap withcta: () => stopNodeMaintenanceModalLauncher(). Both work, but the inconsistency is a bit puzzling.If both launchers have the same signature (no-arg functions returning void/Promise), prefer consistency—either direct reference for both or arrow wrappers for both. This aids readability and avoids questions about subtle differences.
♻️ Suggested alignment (if signatures match)
- cta: () => stopNodeMaintenanceModalLauncher(), + cta: stopNodeMaintenanceModalLauncher,
27-28: Guard theuseStopNodeMaintenanceModalhook call or add undefined handling
findNodeMaintenancereturnsundefinedwhen no matching maintenance is found, yetuseStopNodeMaintenanceModal(nodeMaintenance)is invoked unconditionally at line 28. The hook immediately computes modal content by callinggetNodeMaintenanceReason(nodeMaintenance)andgetNodeMaintenanceNodeName(nodeMaintenance)with potentially undefined data. Additionally,getMaintenanceModel(nodeMaintenance)(invoked in theonConfirmcallback) accessesnodeMaintenance.apiVersionwithout checking if it exists.While the launcher is only invoked when
nodeMaintenanceis truthy (line 38), this pattern wastes renders and violates the hook's contract—the type signature doesn't reflect that undefined is acceptable. This can be fragile if the selector functions don't handle undefined gracefully.Either guard the hook call with a conditional or update
useStopNodeMaintenanceModalto explicitly handle and return a no-op launcher whennodeMaintenanceis undefined.frontend/packages/knative-plugin/src/actions/creators.ts (1)
130-130: Unnecessary template literal.Use a plain string literal here since there's no interpolation.
✏️ Suggested fix
- id: `delete-resource`, + id: 'delete-resource',frontend/packages/console-app/src/actions/hooks/useRetryRolloutAction.ts (1)
98-100: Re-enablereact-hooks/exhaustive-depsand includelaunchModalin the dependency array.
useOverlayreturns a stable launcher—launchOverlayis memoized withuseCallback(..., [])inOverlayProvider(line 40), so its identity never changes. The suppression is unnecessary; including it in the deps array will not cause infinite loops. Keeping the lint rule enabled ensures future maintainers catch unintended stale closures.frontend/packages/console-app/src/actions/hooks/useCommonActions.ts (1)
47-85: Avoid suppressing exhaustive-deps; verifyuseOverlaystability.
launchModalis omitted from deps with an eslint-disable. IfuseOverlayever returns a new function, this can capture a stale launcher. Prefer stabilizing the hook output (or memoizing) so the dependency can be included and the suppression removed.Also applies to: 174-176
frontend/packages/console-app/src/actions/hooks/usePVCActions.ts (1)
7-11: Inconsistent lazy-loading for modal providers.
LazyExpandPVCModalProviderandLazyClonePVCModalProviderare lazy-loaded, butDeletePVCModalProvideris imported directly. For consistency and code-splitting benefits, consider using a lazy-loaded variant forDeletePVCModalProvideras well.Does React lazy loading improve bundle size for modals?frontend/public/components/modals/managed-resource-save-modal.tsx (1)
53-54: Consider excludingcloseOverlayfrom prop spread.The
{...props}spread will passcloseOverlayto the innerManagedResourceSaveModal, which doesn't use it (it usescloseinstead). While React ignores unknown props on function components, explicitly destructuring to excludecloseOverlaywould be cleaner:♻️ Suggested refinement
export const ManagedResourceSaveModalProvider: OverlayComponent<ManagedResourceSaveModalProps> = ( props, ) => { + const { closeOverlay, ...rest } = props; return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ManagedResourceSaveModal close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ManagedResourceSaveModal close={closeOverlay} {...rest} /> </ModalWrapper> ); };frontend/public/components/namespace.jsx (1)
878-882: Simplify:pullSecret: undefinedis redundant.Passing
pullSecret: undefinedexplicitly is unnecessary since the prop is already optional inConfigureNamespacePullSecretProps. This adds noise without semantic value.♻️ Suggested simplification
const modal = () => launchModal(LazyConfigureNamespacePullSecretModalProvider, { namespace, - pullSecret: undefined, });frontend/packages/console-app/src/components/modals/resource-limits/index.ts (1)
4-9: The.then(m => ({ default: m.default }))is redundant.Since
ResourceLimitsModalLauncheralready exportsResourceLimitsModalProvideras its default export, the transform is unnecessary.React.lazyexpects a module with adefaultexport, which is already satisfied.♻️ Simplified lazy export
export const LazyResourceLimitsModalProvider = lazy(() => - import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */).then( - (m) => ({ - default: m.default, - }), - ), + import('./ResourceLimitsModalLauncher' /* webpackChunkName: "resource-limits-modal" */), );That said, if this pattern is intentional for consistency across the codebase or to handle potential named-export scenarios in the future, feel free to keep it.
frontend/packages/dev-console/src/actions/context-menu.ts (1)
14-40: Inconsistency:DeleteApplicationActionstill usesi18next.t()directly.This function uses
i18next.t()for translations (line 23), while the newuseDeleteResourceActionhook usesuseTranslation(). SinceDeleteApplicationActionis not a hook, it can't useuseTranslation, but this creates an inconsistency in how translations are handled within the same file.Consider migrating
DeleteApplicationActionto a hook (useDeleteApplicationAction) in a future iteration to align with the hook-based pattern if the broader migration effort supports it.frontend/packages/console-app/src/components/modals/resource-limits/ResourceLimitsModalLauncher.tsx (1)
64-70: Prop spread order could inadvertently overrideclose.The spread
{...props}afterclose={props.closeOverlay}means ifpropssomehow contains acloseproperty, it would override the intended value. WhileOverlayComponentprops shouldn't includeclose(they providecloseOverlay), defensive ordering is safer:♻️ Defensive prop ordering
export const ResourceLimitsModalProvider: OverlayComponent<ResourceLimitsModalLauncherProps> = ( props, ) => ( <ModalWrapper blocking onClose={props.closeOverlay}> - <ResourceLimitsModalLauncher close={props.closeOverlay} {...props} /> + <ResourceLimitsModalLauncher {...props} close={props.closeOverlay} /> </ModalWrapper> );This ensures
closeis always bound tocloseOverlayregardless of what's inprops.frontend/public/components/modals/configure-ns-pull-secret-modal.tsx (1)
317-329: Same prop spread order concern; alsocloseOverlayleaks into child props.Two observations:
Prop ordering: The spread
{...props}after explicitcancelandcloseassignments could override them ifpropscontains those keys.
closeOverlayprop leakage: TheOverlayComponenttype injectscloseOverlayinto props. Spreading{...props}passescloseOverlaytoConfigureNamespacePullSecret, which doesn't expect it. This may cause a React warning about unknown DOM attributes if the prop propagates further.♻️ Destructure to exclude closeOverlay and fix ordering
-export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = ( - props, -) => { +export const ConfigureNamespacePullSecretModalProvider: OverlayComponent<ConfigureNamespacePullSecretProps> = ({ + closeOverlay, + ...restProps +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> + <ModalWrapper blocking onClose={closeOverlay}> <ConfigureNamespacePullSecret - cancel={props.closeOverlay} - close={props.closeOverlay} - {...props} + {...restProps} + cancel={closeOverlay} + close={closeOverlay} /> </ModalWrapper> ); };This prevents
closeOverlayfrom leaking and ensurescancel/closeare always correctly bound.frontend/public/components/modals/index.ts (1)
87-92: Minor formatting inconsistency.
LazyClonePVCModalProviderandLazyRestorePVCModalProvideruse single-line.then((m) => ({ default: m.default }))while the other six lazy providers use multi-line formatting. Consider aligning for consistency and easier diffing across providers.📝 Suggested formatting alignment
// Lazy-loaded OverlayComponent for Clone PVC Modal export const LazyClonePVCModalProvider = lazy(() => import( '@console/app/src/components/modals/clone/clone-pvc-modal' /* webpackChunkName: "clone-pvc-modal" */ - ).then((m) => ({ default: m.default })), + ).then((m) => ({ + default: m.default, + })), );// Lazy-loaded OverlayComponent for Restore PVC Modal export const LazyRestorePVCModalProvider = lazy(() => import( '@console/app/src/components/modals/restore-pvc/restore-pvc-modal' /* webpackChunkName: "restore-pvc-modal" */ - ).then((m) => ({ default: m.default })), + ).then((m) => ({ + default: m.default, + })), );Also applies to: 113-118
frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-app/src/components/nodes/modals/ConfigureUnschedulableModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/utils/error-modal-handler.ts
Outdated
Show resolved
Hide resolved
frontend/public/components/modals/configure-update-strategy-modal.tsx
Outdated
Show resolved
Hide resolved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
3 times, most recently
from
d102d05 to
274b735
Compare
|
/retest |
1 similar comment
|
/retest |
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
from
274b735 to
73300e0
Compare
|
/retest |
|
/assign @yanpzhan Tech debt, so assigning labels |
openshift-ci
bot
added
the
docs-approved
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
from
73300e0 to
3705789
Compare
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
rhamilto
force-pushed
the
CONSOLE-5012-5
branch
from
3705789 to
afcde7b
Compare
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@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 |
openshift-merge-robot
added
the
needs-rebase
|
@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-5
branch
from
3f80ef3 to
923a78b
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@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-5
branch
2 times, most recently
from
fbd0f24 to
1970b4a
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
force-pushed
the
CONSOLE-5012-5
branch
from
1970b4a to
bd179b9
Compare
- 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>
…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
force-pushed
the
CONSOLE-5012-5
branch
from
bd179b9 to
1d3a776
Compare
|
/retest |
3 similar comments
|
/retest |
|
/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
This PR completes the modal migration effort by migrating all instances of
confirmModalto use the modern overlay pattern withuseWarningModaland refactorsmoveNodeToGroupto use React Context for confirmation handling.Note: This PR builds on:
Changes
Removed deprecated confirmModal
confirm-modal.tsxand its imperative launcherMigrated to useWarningModal overlay pattern
useWarningModalwith proper cancel handlingStopNodeMaintenanceModalto remove imperative API, keeping only hook-based approachuseWarningModalto properly callcloseOverlay()and filter it from props spreadRefactored moveNodeToGroup to use React Context
MoveNodeHandlersProviderfor confirmation modal handling onlySyncMoveNodeHandlerscomponent to sync handlers for non-React contextsuseMoveNodeHandlers()hook for accessing confirmation handlerMoveNodeHandlersProviderlaunchErrorModal()from CONSOLE-5012: Create shared error modal launcher using React Context #15946 for error handling (not custom handler)useSetupMoveNodeToGroupHandlershooks with warningsFixed accessibility violation
Architecture Benefits
launchErrorModal()from CONSOLE-5012: Create shared error modal launcher using React Context #15946The hybrid approach (Context + module reference) supports both React components (via
useMoveNodeHandlershook) and non-React drag-drop callbacks (viamoveNodeToGroupfunction), consistent with the error modal handler pattern.Technical Details
Accessibility Fix - aria-hidden Focus Violation
Problem: When the confirmation modal opened during drag-drop operations, focus remained on an SVG
<g>element in the topology graph. PatternFly's Modal setsaria-hidden="true"on the #app div, creating an accessibility violation where a focused element is hidden from assistive technology users.Browser Warning (Fixed):
Solution: Blur the active element before launching the modal:
React Context Pattern
Before (Global State):
After (React Context + Shared Error Handler):
Key Improvements:
launchErrorModal()functionHybrid Approach:
useMoveNodeHandlers()hook for confirmationsmoveNodeToGroup()functionSyncMoveNodeHandlersbridges the two worldslaunchErrorModal()from CONSOLE-5012: Create shared error modal launcher using React Context #15946useWarningModal Fix
Fixed prop spreading issue that caused React DOM warnings:
Test Plan
Automated Tests ✅
Manual Testing
launchErrorModal()handlercloseOverlayprop in consoleBreaking Changes
None - deprecated hooks remain functional with console warnings for backward compatibility.
Related
🤖 Generated with Claude Code