CONSOLE-4711: Replace confirmModal for useWarningModal #15448

Mylanos · 2025-08-29T14:47:41Z

still missing instances in metal plugin, some references in knative integration tests

console/frontend/packages/knative-plugin/integration-tests/support/step-definitions/eventing/eventing-broker-trigger.ts

Line 106 in 477e212

cy.get(topologyPO.graph.confirmModal).click();

and also missing the migration of moveNodeToGroup action within topology.

openshift-ci-robot · 2025-08-29T14:47:44Z

@Mylanos: This pull request references CONSOLE-4711 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.20.0" version, but no target version was set.

Details

In response to this:

still missing instances in metal plugin, some references in knative integration tests

console/frontend/packages/knative-plugin/integration-tests/support/step-definitions/eventing/eventing-broker-trigger.ts

Line 106 in 477e212

cy.get(topologyPO.graph.confirmModal).click();

and also missing the migration of moveNodeToGroup action within topology.

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.

Mylanos · 2025-08-29T14:48:20Z

/label tide-merge-method-squash

openshift-ci · 2025-08-29T14:48:49Z

@Mylanos: The label(s) /label tide-merge-method-squash cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, ux-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, run-integration-tests, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, ok-to-test, plugin-api-approved, plugin-api-changed, stability-fix-approved, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

Details

In response to this:

/label tide-merge-method-squash

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 kubernetes-sigs/prow repository.

Mylanos · 2025-08-29T14:49:19Z

/label tide/merge-method-squash

Mylanos · 2025-09-01T19:01:12Z

/retest

Mylanos · 2025-09-02T10:24:45Z

/retest

logonoff · 2025-09-02T20:50:05Z

/cc

logonoff · 2025-09-03T13:27:36Z

frontend/packages/console-shared/src/hooks/useWarningModal.tsx

+/**
+ * useWarningModalWithProps returns a WarningModal launcher that accepts override props
+ */
+export type WarningModalCallbackWithProps = (overrides: Omit<WarningModalProps, 'isOpen'>) => void;


we can probably dry up Omit<WarningModalProps, 'isOpen'> as ControlledWarningModalProps

logonoff · 2025-09-03T13:29:54Z

frontend/packages/console-shared/src/hooks/useWarningModal.tsx

+export const useWarningModalWithProps = (
+  props?: Partial<Omit<WarningModalProps, 'isOpen'>>,
+): WarningModalCallbackWithProps => {
+  const launcher = useOverlay();
+  return (overrides) => {
+    launcher<WarningModalProps>(ControlledWarningModal, { ...(props || {}), ...overrides });
+  };
+};


this might look cleaner

Suggested change

export const useWarningModalWithProps = (

props?: Partial<Omit<WarningModalProps, 'isOpen'>>,

): WarningModalCallbackWithProps => {

const launcher = useOverlay();

return (overrides) => {

launcher<WarningModalProps>(ControlledWarningModal, { ...(props || {}), ...overrides });

};

};

export const useWarningModalWithProps = (

props: Partial<Omit<WarningModalProps, 'isOpen'>> = {},

): WarningModalCallbackWithProps => {

const launcher = useOverlay();

return (overrides) => {

launcher<WarningModalProps>(ControlledWarningModal, { ...props, ...overrides });

};

};

i'm also wondering if we should just include overrides in the callback of useWarningModal, so we don't need to have two hooks here, or if we should be doing further refactors to make this hook not necessary

For the first note, we would have to change the prop to props: Partial<Omit<WarningModalProps, 'isOpen'>> = { children: <></>} since Partial<Omit<WarningModalProps, 'isOpen'>> require children and we are assigning to this type, which doesn't seem to be as clean as the fallback {}.

For the second note I was thinking about that as well, but there is a problem that the ControlledWarningModal props provided to the launcher would have to define a default children prop since its a required WarningModalProps PF prop.
Something like this:

Suggested change

export const useWarningModalWithProps = (

props?: Partial<Omit<WarningModalProps, 'isOpen'>>,

): WarningModalCallbackWithProps => {

const launcher = useOverlay();

return (overrides) => {

launcher<WarningModalProps>(ControlledWarningModal, { ...(props || {}), ...overrides });

};

};

export const useWarningModal = (

props: Partial<ControlledWarningModalProps>,

): WarningModalCallbackWithProps => {

const launcher = useOverlay();

return useCallback((overrides) => {

const mergedProps: WarningModalProps = {

children: <></>, // Default children

...(props || {}),

...(overrides || {})

};

launcher<WarningModalProps>(ControlledWarningModal, mergedProps);

}, [launcher, props]);

};

export type WarningModalCallbackWithProps = (overrides?: ControlledWarningModalProps) => void;

Seems like cleaner solution to me WDYT? @logonoff

LGTM. maybe children can be null?

That works as well.

logonoff · 2025-09-03T13:34:27Z

frontend/packages/topology/src/actions/edgeActions.ts

+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [kindObj, resource, t],


why don't we need openConfirm in the deps array?

Because adding it to the dependency list causes max depth exceeded errors when opening the actions list - similarly as in https://github.com/openshift/console/pull/15448/files#diff-e977f2b41b8b2fed19e80f900dec493780f87d0476f70280768f6b6957c56ba0R96
will add a comment.

logonoff · 2025-09-04T12:23:54Z

/label px-approved
/label docs-approved

…allback

christoph-jerolimov · 2025-09-04T18:44:33Z

/uncc

Mylanos · 2025-09-05T07:55:09Z

/retest

Mylanos · 2025-09-05T12:39:47Z

/retest

Mylanos · 2025-09-08T05:37:23Z

/retest

Mylanos · 2025-09-08T14:38:14Z

/assign yapei

yanpzhan · 2025-09-10T12:44:11Z

Regression test on cluster launched against the pr passed.
/verified by @yanpzhan

openshift-ci-robot · 2025-09-10T12:44:24Z

@yanpzhan: This PR has been marked as verified by @yanpzhan.

Details

In response to this:

Regression test on cluster launched against the pr passed.
/verified by @yanpzhan

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.

logonoff

/lgtm
/label qe-approved

openshift-ci-robot · 2025-09-10T13:00:27Z

@Mylanos: This pull request references CONSOLE-4711 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.21.0" version, but no target version was set.

Details

In response to this:

still missing instances in metal plugin, some references in knative integration tests

console/frontend/packages/knative-plugin/integration-tests/support/step-definitions/eventing/eventing-broker-trigger.ts

Line 106 in 477e212

cy.get(topologyPO.graph.confirmModal).click();

and also missing the migration of moveNodeToGroup action within topology.

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 · 2025-09-10T13:01:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logonoff, Mylanos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [Mylanos,logonoff]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-09-10T14:01:24Z

/retest-required

Remaining retests: 0 against base HEAD 343b6dd and 2 for PR HEAD 0858c30 in total

openshift-ci · 2025-09-10T23:44:17Z

@Mylanos: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 29, 2025

openshift-ci bot requested review from Lucifergene and christoph-jerolimov August 29, 2025 14:48

openshift-ci bot added component/shared Related to console-shared component/topology Related to topology labels Aug 29, 2025

openshift-ci bot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated component/knative Related to knative-plugin labels Aug 29, 2025

openshift-ci bot requested a review from logonoff September 2, 2025 20:50

openshift-ci bot added the kind/cypress Related to Cypress e2e integration testing label Sep 3, 2025

logonoff reviewed Sep 3, 2025

View reviewed changes

Mylanos force-pushed the RefactorConfirmModal branch from 24bbcd9 to 5b4fffb Compare September 3, 2025 16:08

Mylanos added 6 commits September 4, 2025 09:35

migrate confirm nav unpin

4438930

migrate replication controller confirm modal

94a10eb

migrate helm plugin to use useWarningModal instead of confirmModal

6f93eb3

migrate internal console confirmModals to useWarningModal

c96fe63

migrate topology confirmModals to useWarningModal

1e5291e

migrate pipelines plugin to use useWarningModal instead of confirmModal

499d77c

update i18n

e1ebbb9

Mylanos force-pushed the RefactorConfirmModal branch from 5b4fffb to 138de44 Compare September 4, 2025 07:53

openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR labels Sep 4, 2025

Mylanos added 5 commits September 4, 2025 15:48

fix integration tests, reduce getResource calls

cba299d

update the cancel button

4082d24

consolidate testing of the warning modals, fix tests

7b6b6f9

Adjust the useWarningModal to accept override props in the returned c…

d06ad00

…allback

comment nit

0858c30

Mylanos force-pushed the RefactorConfirmModal branch from 138de44 to 0858c30 Compare September 4, 2025 13:49

openshift-ci bot removed the request for review from christoph-jerolimov September 4, 2025 18:44

openshift-ci bot assigned yapei Sep 8, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Sep 10, 2025

logonoff approved these changes Sep 10, 2025

View reviewed changes

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 10, 2025

openshift-ci bot assigned logonoff Sep 10, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2025

openshift-merge-bot bot merged commit 156d98c into openshift:main Sep 10, 2025
8 checks passed

		// eslint-disable-next-line react-hooks/exhaustive-deps
		[kindObj, resource, t],

CONSOLE-4711: Replace confirmModal for useWarningModal #15448

CONSOLE-4711: Replace confirmModal for useWarningModal #15448

Uh oh!

Conversation

Mylanos commented Aug 29, 2025

Uh oh!

openshift-ci-robot commented Aug 29, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mylanos commented Aug 29, 2025

Uh oh!

openshift-ci bot commented Aug 29, 2025

Uh oh!

Mylanos commented Aug 29, 2025

Uh oh!

Mylanos commented Sep 1, 2025

Uh oh!

Mylanos commented Sep 2, 2025

Uh oh!

logonoff commented Sep 2, 2025

Uh oh!

logonoff Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

logonoff Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Mylanos Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

logonoff Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mylanos Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

logonoff Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Mylanos Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logonoff commented Sep 4, 2025

Uh oh!

christoph-jerolimov commented Sep 4, 2025

Uh oh!

Mylanos commented Sep 5, 2025

Uh oh!

Mylanos commented Sep 5, 2025

Uh oh!

Mylanos commented Sep 8, 2025

Uh oh!

Mylanos commented Sep 8, 2025

Uh oh!

yanpzhan commented Sep 10, 2025

Uh oh!

openshift-ci-robot commented Sep 10, 2025

Uh oh!

logonoff left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 10, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Sep 10, 2025

Uh oh!

openshift-ci-robot commented Sep 10, 2025

Uh oh!

openshift-ci bot commented Sep 10, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

openshift-ci-robot commented Aug 29, 2025 •

edited by openshift-ci bot

Loading

logonoff Sep 3, 2025 •

edited

Loading

Mylanos Sep 3, 2025 •

edited

Loading

openshift-ci-robot commented Sep 10, 2025 •

edited by openshift-ci bot

Loading