Skip to content

Conversation

@sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Jan 20, 2026

Remove createModalLauncher usage from DeleteHPAModal and ConsolePluginModal to eliminate React Router wrapper conflicts. Both modals now use the useOverlay pattern with OverlayComponent wrappers. This change is part of CONSOLE-5012 to unblock the React Router v7 upgrade.

Modals Migrated

  1. DeleteHPAModal (packages/console-shared/src/components/hpa/DeleteHPAModal.tsx)

    • Migrated to DeleteHPAModalOverlay: OverlayComponent
    • Consumer updated: packages/console-app/src/actions/creators/hpa-factory.ts
  2. ConsolePluginModal (packages/console-shared/src/components/modals/ConsolePluginModal.tsx)

    • Migrated to ConsolePluginModalOverlay: OverlayComponent
    • Backwards-compatible wrapper: consolePluginModal() function in index.ts
    • Consumer updated: packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx

Changes

  • Exported modal overlays as OverlayComponent using the *ModalOverlay naming convention
  • Wrapped modal components with ModalWrapper and connected to overlay lifecycle (closeOverlay)
  • Updated consumers to use useOverlay hook with launchModal() instead of direct modal calls
  • Added backwards-compatible consolePluginModal() wrapper for gradual migration

Scope Note

Consumer in clusterserviceversion.tsx intentionally excluded:

  • The ConsolePluginModal usage in packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx uses the backwards-compatible consolePluginModal() wrapper
  • OLM consumer updates are being handled separately in PR #15968 to avoid conflicts
  • The backwards-compatible wrapper internally calls the new ConsolePluginModalOverlay pattern, so the migration is still functional

Assisted by: Claude Code

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2026
@openshift-ci openshift-ci bot requested review from rhamilto and spadgett January 20, 2026 18:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 20, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sg00dwin
Once this PR has been reviewed and has the lgtm label, please assign vikram-raj for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added component/core Related to console core functionality component/olm Related to OLM component/shared Related to console-shared labels Jan 20, 2026
@sg00dwin sg00dwin changed the title [WIP] Migrate modals from createModalLauncher to useOverlay [WIP] CONSOLE-5049: Migrate modals from createModalLauncher to useOverlay Jan 20, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 20, 2026
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 20, 2026

@sg00dwin: This pull request references CONSOLE-5049 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 sub-task to target the "4.22.0" version, but no target version was set.

Details

In response to this:

Remove createModalLauncher usage from DeleteHPAModal and ConsolePluginModal to eliminate React Router wrapper conflicts. Both modals now use the useOverlay pattern with Provider wrappers. This change is part of CONSOLE-5012 to unblock the React Router v7 upgrade.

Assisted by: Claude Code

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.

@sg00dwin
Copy link
Member Author

/jira refresh

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 20, 2026

@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue.

Details

In response to this:

/jira refresh

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.

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, @sg00dwin. Just one observation.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 21, 2026

@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue.

Details

In response to this:

Remove createModalLauncher usage from DeleteHPAModal and ConsolePluginModal to eliminate React Router wrapper conflicts. Both modals now use the useOverlay pattern with Provider wrappers. This change is part of CONSOLE-5012 to unblock the React Router v7 upgrade.

Modals refactored
DeleteHPAModal.tsx
ConsolePluginModal.tsx

Assisted by: Claude Code

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.

@sg00dwin sg00dwin force-pushed the CONSOLE-5049-migration-from-createModalLauncher branch from f2fb0b5 to 3517ef6 Compare January 21, 2026 17:44
@sg00dwin sg00dwin changed the title [WIP] CONSOLE-5049: Migrate modals from createModalLauncher to useOverlay CONSOLE-5049: Migrate modals from createModalLauncher to useOverlay Jan 21, 2026
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 21, 2026
Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing Claude caught as I was working on other migrations

@sg00dwin sg00dwin force-pushed the CONSOLE-5049-migration-from-createModalLauncher branch from 3517ef6 to 582b790 Compare January 21, 2026 20:03
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 21, 2026

@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue.

Details

In response to this:

Remove createModalLauncher usage from DeleteHPAModal and ConsolePluginModal to eliminate React Router wrapper conflicts. Both modals now use the useOverlay pattern with Provider wrappers. This change is part of CONSOLE-5012 to unblock the React Router v7 upgrade.

Modals refactored
DeleteHPAModal.tsx
ConsolePluginModal.tsx

Assisted by: Claude Code

Summary by CodeRabbit

  • Refactor
  • Updated modal handling for HPA deletion and console plugin configuration to use a consistent overlay-based presentation pattern, improving internal consistency across modal interactions.

✏️ Tip: You can customize this high-level summary in your review settings.

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 21, 2026

📝 Walkthrough

Walkthrough

This PR refactors modal invocation patterns across multiple console packages, transitioning from direct modal launcher functions to an overlay-based architecture using the useOverlay hook and OverlayComponent providers. Affected areas include HPA (Horizontal Pod Autoscaler) action management, Console Plugin configuration modals, and cluster service version components. The changes replace createModalLauncher exports with provider components that wrap modal logic within ModalWrapper, while components now acquire overlay launchers through useOverlay() hooks to trigger modal display. Public exports and component signatures are updated accordingly to reflect the new pattern.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'CONSOLE-5049: Migrate modals from createModalLauncher to useOverlay' accurately and specifically summarizes the main change across all modified files—replacing the createModalLauncher pattern with the useOverlay hook-based pattern for modal invocations.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx (1)

243-283: Pass csvPluginsCount into the modal launch.
Line 278 opens the modal without csvPluginsCount, so ConsolePluginModal falls back to the single-plugin copy path. Since csvPluginsCount is computed at Line 257, forward it to preserve the operator-scoped messaging.

🛠️ Suggested fix
-                    launchOverlay(ConsolePluginModalProvider, {
-                      consoleOperatorConfig,
-                      pluginName,
-                      trusted,
-                    })
+                    launchOverlay(ConsolePluginModalProvider, {
+                      consoleOperatorConfig,
+                      csvPluginsCount,
+                      pluginName,
+                      trusted,
+                    })
🤖 Fix all issues with AI agents
In `@frontend/packages/console-app/src/actions/creators/hpa-factory.ts`:
- Around line 116-117: The eslint-disable comment incorrectly references
launchModal while the hook variable is named launchOverlay, causing a
stale/confusing comment; update the comment to reference launchOverlay (or
remove the disable if you can re-enable react-hooks/exhaustive-deps) near the
place where launchOverlay is used (the hook/variable named launchOverlay in
hpa-factory.ts) so the dependency comment accurately reflects the actual
dependency and prevents confusion for future maintainers.
- Around line 102-107: The delete CTA currently always passes only
relatedHPAs[0] to DeleteHPAModalProvider, which prevents deleting additional
HPAs; modify the cta in hpa-factory.ts to check relatedHPAs.length and when >1
either open a selector (e.g., launch a small picker overlay listing relatedHPAs
so the user can choose which HPA to delete) or pass the entire relatedHPAs array
to a revised DeleteHPAModalProvider that shows a list with delete actions, and
when length === 1 continue to pass relatedHPAs[0]; update references to
relatedHPAs, launchOverlay, and DeleteHPAModalProvider accordingly.

Comment on lines 102 to 107
cta: () => {
launchOverlay(DeleteHPAModalProvider, {
workload: resource,
hpa: relatedHPAs[0],
});
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for DeleteHPAModal usage and related patterns
rg -n -C5 'deleteHPAModal|DeleteHPAModal' --type=ts --type=tsx

Repository: openshift/console

Length of output: 88


🏁 Script executed:

# Look at the hpa-factory.ts file to understand the relatedHPAs population
cat -n frontend/packages/console-app/src/actions/creators/hpa-factory.ts | head -150

Repository: openshift/console

Length of output: 5067


🏁 Script executed:

# Check for any comments or tests about multiple HPA scenarios
rg -n -C3 'relatedHPAs|multiple.*HPA|HPA.*multiple' --type=ts --type=tsx

Repository: openshift/console

Length of output: 88


🏁 Script executed:

# Find and examine the doesHpaMatch function
rg -n -C10 'export.*doesHpaMatch|function.*doesHpaMatch' --type=ts

Repository: openshift/console

Length of output: 1237


🏁 Script executed:

# Look at the DeleteHPAModal component
find . -name 'DeleteHPAModal*' -type f | head -5

Repository: openshift/console

Length of output: 133


🏁 Script executed:

# Search for any tests or comments about multiple HPAs
rg -n 'multiple.*HPA|HPA.*multiple|relatedHPAs.*length|relatedHPAs\[' --type=ts | head -20

Repository: openshift/console

Length of output: 586


🏁 Script executed:

# Check if there are any related issues or TODOs
rg -n 'TODO|FIXME|BUG' frontend/packages/console-app/src/actions/creators/hpa-factory.ts

Repository: openshift/console

Length of output: 43


🏁 Script executed:

# Read the DeleteHPAModal component
cat -n ./frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsx

Repository: openshift/console

Length of output: 4439


🏁 Script executed:

# Search for test files related to HPA
fd -e test.ts -e spec.ts hpa | head -20

Repository: openshift/console

Length of output: 208


🏁 Script executed:

# Check for tests in hpa-factory.ts
rg -n 'test|describe|it(' frontend/packages/console-app/src/actions/creators/ | grep -i hpa

Repository: openshift/console

Length of output: 145


🏁 Script executed:

# Check the HPA utils tests
cat -n frontend/packages/console-shared/src/utils/__tests__/hpa-utils.spec.ts

Repository: openshift/console

Length of output: 728


🏁 Script executed:

# Also check the dev-console HPA tests
cat -n frontend/packages/dev-console/src/components/hpa/__tests__/hpa-utils.spec.ts | head -100

Repository: openshift/console

Length of output: 5392


🏁 Script executed:

# Search for any architectural decisions or comments about multiple HPAs
rg -n 'multiple|single|one.*HPA|HPA.*one' frontend/packages/console-shared/src/utils/hpa-utils.ts frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsx

Repository: openshift/console

Length of output: 350


Handle multiple HPAs targeting the same workload in the delete action.

The delete action passes relatedHPAs[0] to the DeleteHPAModal without checking if multiple HPAs target the workload. While Kubernetes discourages it, the API allows multiple HPAs per workload. When this occurs, only the first HPA can be deleted through the UI—users with multiple HPAs won't be able to remove the others. Either add a selection mechanism when multiple HPAs are present, warn the user, or document this as an unsupported edge case.

🤖 Prompt for AI Agents
In `@frontend/packages/console-app/src/actions/creators/hpa-factory.ts` around
lines 102 - 107, The delete CTA currently always passes only relatedHPAs[0] to
DeleteHPAModalProvider, which prevents deleting additional HPAs; modify the cta
in hpa-factory.ts to check relatedHPAs.length and when >1 either open a selector
(e.g., launch a small picker overlay listing relatedHPAs so the user can choose
which HPA to delete) or pass the entire relatedHPAs array to a revised
DeleteHPAModalProvider that shows a list with delete actions, and when length
=== 1 continue to pass relatedHPAs[0]; update references to relatedHPAs,
launchOverlay, and DeleteHPAModalProvider accordingly.

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of suggestions that came from the review of #15932

@sg00dwin sg00dwin force-pushed the CONSOLE-5049-migration-from-createModalLauncher branch from 582b790 to 0eb2387 Compare January 30, 2026 22:24
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 3, 2026

/test e2e-gcp-console

1 similar comment
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 3, 2026

/test e2e-gcp-console

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: doesn't this belong in #15968 since it's the OLM PR?

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 5, 2026

@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue.

Details

In response to this:

Remove createModalLauncher usage from DeleteHPAModal and ConsolePluginModal to eliminate React Router wrapper conflicts. Both modals now use the useOverlay pattern with OverlayComponent wrappers. This change is part of CONSOLE-5012 to unblock the React Router v7 upgrade.

Modals Migrated

  1. DeleteHPAModal (packages/console-shared/src/components/hpa/DeleteHPAModal.tsx)
  • Migrated to DeleteHPAModalOverlay: OverlayComponent
  • Consumer updated: packages/console-app/src/actions/creators/hpa-factory.ts
  1. ConsolePluginModal (packages/console-shared/src/components/modals/ConsolePluginModal.tsx)
  • Migrated to ConsolePluginModalOverlay: OverlayComponent
  • Backwards-compatible wrapper: consolePluginModal() function in index.ts
  • Consumer updated: packages/console-app/src/components/console-operator/ConsoleOperatorConfig.tsx

Changes

  • Exported modal overlays as OverlayComponent using the *ModalOverlay naming convention
  • Wrapped modal components with ModalWrapper and connected to overlay lifecycle (closeOverlay)
  • Updated consumers to use useOverlay hook with launchModal() instead of direct modal calls
  • Added backwards-compatible consolePluginModal() wrapper for gradual migration

Scope Note

Consumer in clusterserviceversion.tsx intentionally excluded:

  • The ConsolePluginModal usage in packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx uses the backwards-compatible consolePluginModal() wrapper
  • OLM consumer updates are being handled separately in PR #15968 to avoid conflicts
  • The backwards-compatible wrapper internally calls the new ConsolePluginModalOverlay pattern, so the migration is still functional

Assisted by: Claude Code

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.

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clusterserviceversion.tsx does probably belong in this PR since that's where the modal is being changed as it's resulting in the need to keep consolePluginModal around and #15968 doesn't include it. Plus we'll want to add LazyConsolePluginModal and use it in both clusterserviceversion.tsx and ConsoleOperatorConfig.tsx.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2026
Assisted by: Claude Code
…enshift#15968

The ConsolePluginModal consumer changes in clusterserviceversion.tsx are
being migrated in PR openshift#15968 (OLM modals migration). This PR should only
focus on the modal component migrations (DeleteHPAModal and ConsolePluginModal).

The file now uses the backwards-compatible consolePluginModal() function
which internally calls the new ConsolePluginModalOverlay pattern.

Assisted by: Claude Code
Updated DeleteHPAModal and ConsolePluginModal to follow the patterns
from PR openshift#15939 and the modal-migration-guide.md:

1. Export Pattern: Changed from const + export { } to export const
2. Type Positioning: Moved overlay props types to bottom of files
3. ModalComponentProps: Updated ConsolePluginModalProps to extend it

Changes:
- DeleteHPAModal.tsx: export const pattern, type at bottom
- ConsolePluginModal.tsx: export const pattern, extends ModalComponentProps,
  removed explicit cancel/close props, moved overlay props to bottom

Assisted by: Claude Code
@sg00dwin sg00dwin force-pushed the CONSOLE-5049-migration-from-createModalLauncher branch from fd3b54e to f405e08 Compare February 10, 2026 19:41
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2026
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 10, 2026

@sg00dwin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-console f405e08 link true /test e2e-gcp-console
ci/prow/okd-scos-images f405e08 link true /test okd-scos-images

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/core Related to console core functionality component/olm Related to OLM component/shared Related to console-shared jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants