CONSOLE-5049: Migrate modals from createModalLauncher to useOverlay #15935
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sg00dwin The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/core
sg00dwin
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@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. 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. |
|
/jira refresh |
|
@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue. 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. |
frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsx
Outdated
Show resolved
Hide resolved
|
@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue. 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. |
sg00dwin
force-pushed
the
CONSOLE-5049-migration-from-createModalLauncher
branch
from
f2fb0b5 to
3517ef6
Compare
sg00dwin
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
rhamilto
left a comment
rhamilto
left a comment
There was a problem hiding this comment.
One thing Claude caught as I was working on other migrations
frontend/packages/console-app/src/actions/creators/hpa-factory.ts
Outdated
Show resolved
Hide resolved
…r to useOverlay Assisted by: Claude Code
sg00dwin
force-pushed
the
CONSOLE-5049-migration-from-createModalLauncher
branch
from
3517ef6 to
582b790
Compare
|
@sg00dwin: This pull request references CONSOLE-5049 which is a valid jira issue. 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 PR refactors modal invocation patterns across multiple console packages, transitioning from direct modal launcher functions to an overlay-based architecture using the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 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: PasscsvPluginsCountinto the modal launch.
Line 278 opens the modal withoutcsvPluginsCount, soConsolePluginModalfalls back to the single-plugin copy path. SincecsvPluginsCountis 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.
| cta: () => { | ||
| launchOverlay(DeleteHPAModalProvider, { | ||
| workload: resource, | ||
| hpa: relatedHPAs[0], | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for DeleteHPAModal usage and related patterns
rg -n -C5 'deleteHPAModal|DeleteHPAModal' --type=ts --type=tsxRepository: 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 -150Repository: 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=tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
# Find and examine the doesHpaMatch function
rg -n -C10 'export.*doesHpaMatch|function.*doesHpaMatch' --type=tsRepository: openshift/console
Length of output: 1237
🏁 Script executed:
# Look at the DeleteHPAModal component
find . -name 'DeleteHPAModal*' -type f | head -5Repository: 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 -20Repository: 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.tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Read the DeleteHPAModal component
cat -n ./frontend/packages/console-shared/src/components/hpa/DeleteHPAModal.tsxRepository: openshift/console
Length of output: 4439
🏁 Script executed:
# Search for test files related to HPA
fd -e test.ts -e spec.ts hpa | head -20Repository: 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 hpaRepository: 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.tsRepository: 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 -100Repository: 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.tsxRepository: 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.
|
@sg00dwin: 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. |
3 participants
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
✏️ Tip: You can customize this high-level summary in your review settings.