CONSOLE-5049: Migrate OLM modals to overlay pattern #15968
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
openshift-ci
bot
added
component/core
|
@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 pull request refactors modal management across OpenShift Console from a createModalLauncher-based pattern to an overlay-based provider pattern. Modal files export OverlayComponent-wrapped providers instead of launcher functions. Consumer code uses the useOverlay hook to launch these providers with appropriate props. Updated files include modals for taints, subscriptions, install plans, resource requirements, update strategy, and related components. Props types are restructured to match provider requirements. Test files are updated to mock useOverlay and useK8sModel hooks. Public API signatures change from exported launcher functions to exported provider components and types. 🚥 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx (2)
657-665:⚠️ Potential issue | 🟡 MinorRemove unused
k8sGetandk8sPatchprops from the modal type and provider spread.The modal imports
k8sGetResource(line 16) andk8sPatch(line 43) directly from their respective modules and uses those in the implementation (lines 144, 176), never touching the passed-in props. These props are defined inUninstallOperatorModalProps(lines 683–684) but the modal component doesn't destructure them (lines 65–71), yet they're still being spread from the provider (line 662). This adds unnecessary coupling and type noise without providing any value.Remove
k8sGetandk8sPatchfromUninstallOperatorModalPropsandUninstallOperatorModalProviderPropstype definitions, then strip them from the spread inUninstallOperatorModalProviderto keep the API surface clean.
434-444:⚠️ Potential issue | 🟡 MinorRemove unused
UninstallOperatorOverlayexport.This component is never imported or referenced anywhere in the codebase—all usage has migrated to
UninstallOperatorModalProviderwithModalWrapper. Additionally, line 440 has an invalid trailing semicolon in the JSX that should be removed if you keep this code during the refactor.Consider deleting
UninstallOperatorOverlayentirely (lines 434–444) to eliminate dead code and avoid confusion about which modal pattern to use.
🤖 Fix all issues with AI agents
In
`@frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx`:
- Around line 156-191: The overlay is launched before the K8s model finishes
loading; update ResourceRequirementsModalLink to destructure the loading flag
from useK8sModel (e.g., const [model, inFlight] = useK8sModel(...)), prevent
launchOverlay in onClick when inFlight is true (early return) and pass a
disabled prop to the Button (and optionally a tooltip or aria-busy) while
inFlight so the modal cannot be opened until the model is ready.
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`:
- Around line 667-677: UninstallOperatorModalProviderProps declares k8sGet and
k8sPatch but UninstallOperatorModal doesn't use them (it imports k8sPatch from
`@console/internal/module/k8s` and uses k8sGetResource elsewhere), so either
remove k8sGet and k8sPatch from UninstallOperatorModalProviderProps (and
corresponding UninstallOperatorModalProps) to avoid dead API surface, or
refactor UninstallOperatorModal to accept and use the injected props
(k8sGet/k8sPatch) instead of importing k8sPatch directly; update all
consumers/tests to pass the props if you choose injection, or remove the unused
prop declarations if you choose cleanup.
In
`@frontend/packages/operator-lifecycle-manager/src/components/subscription.tsx`:
- Around line 431-439: The code calls launcher(UninstallOperatorModalProvider,
...) during render which is a side effect; move this logic into a React
useEffect hook so it runs after render when the URL contains showDelete.
Specifically, create a useEffect that checks new
URLSearchParams(window.location.search).has('showDelete'), calls
launcher(UninstallOperatorModalProvider, { k8sKill, k8sGet, k8sPatch,
subscription: obj }), then calls removeQueryArgument('showDelete'); include
k8sKill, k8sGet, k8sPatch, obj (subscription) in the effect dependencies as
appropriate (or an empty array if you only want it once) to avoid repeated
launches on rerenders.
In `@frontend/public/components/modals/index.ts`:
- Around line 68-71: The exported taintsModal currently calls
m.TaintsModalProvider(props) which invokes the overlay component instead of
returning a launcher; either remove this vestigial export if unused, or change
it to the overlay-launcher pattern used by tolerationsModal: dynamically import
TaintsModalProvider but return a launcher (e.g., via createModalLauncher or a
function that calls launchOverlay with the component and closeOverlay) so the
component is passed to launchOverlay/useOverlay rather than invoked directly;
update references to taintsModal to use the new launcher or delete the export if
no consumers remain.
🧹 Nitpick comments (1)
frontend/packages/operator-lifecycle-manager/src/components/modals/subscription-channel-modal.tsx (1)
116-121: Consider documenting thecloseOverlayinjection more explicitly in the type.The comment is helpful, but for better TypeScript ergonomics, you might consider extending the props type to explicitly include
closeOverlayeven though it's auto-injected. This would improve IDE autocomplete for consumers and make the contract clearer.type SubscriptionChannelModalProviderProps = { subscription: SubscriptionKind; pkg: PackageManifestKind; k8sUpdate: (kind: K8sKind, newObj: K8sResourceKind) => Promise<any>; } & { closeOverlay?: () => void }; // injected by OverlayComponentThat said, if
OverlayComponent<T>already handles this via its generic constraint, the current approach is acceptable.
| export const ResourceRequirementsModalLink: FC<ResourceRequirementsModalLinkProps> = (props) => { | ||
| const { obj, type, path } = props; | ||
| const { t } = useTranslation(); | ||
| const launchOverlay = useOverlay(); | ||
| const [model] = useK8sModel(referenceFor(obj)); | ||
| const none = t('public~None'); | ||
| const { cpu, memory, 'ephemeral-storage': storage } = _.get(obj.spec, `${path}.${type}`, {}); | ||
|
|
||
| const onClick = () => { | ||
| const modal = createModalLauncher(ResourceRequirementsModal); | ||
| const description = t('olm~Define the resource {{type}} for this {{kind}} instance.', { | ||
| type, | ||
| kind: obj.kind, | ||
| }); | ||
| const title = t('olm~{{kind}} Resource {{type}}', { | ||
| kind: obj.kind, | ||
| type: _.capitalize(type), | ||
| }); | ||
| const onClick = () => { | ||
| const description = t('olm~Define the resource {{type}} for this {{kind}} instance.', { | ||
| type, | ||
| kind: obj.kind, | ||
| }); | ||
| const title = t('olm~{{kind}} Resource {{type}}', { | ||
| kind: obj.kind, | ||
| type: _.capitalize(type), | ||
| }); | ||
|
|
||
| return modal({ title, description, obj, model, type, path }); | ||
| }; | ||
| launchOverlay(ResourceRequirementsModalProvider, { | ||
| title, | ||
| description, | ||
| obj, | ||
| model, | ||
| type, | ||
| path, | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <Button | ||
| icon={<PencilAltIcon />} | ||
| iconPosition="end" | ||
| type="button" | ||
| isInline | ||
| data-test-id="configure-modal-btn" | ||
| onClick={onClick} | ||
| variant="link" | ||
| > | ||
| {t('olm~CPU: {{cpu}}, Memory: {{memory}}, Storage: {{storage}}', { | ||
| cpu: cpu || none, | ||
| memory: memory || none, | ||
| storage: storage || none, | ||
| })} | ||
| </Button> | ||
| ); | ||
| }, | ||
| ); | ||
| return ( | ||
| <Button | ||
| icon={<PencilAltIcon />} | ||
| iconPosition="end" | ||
| type="button" | ||
| isInline | ||
| data-test-id="configure-modal-btn" | ||
| onClick={onClick} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find useK8sModel hook definition
fd -e ts -e tsx -path '*/hooks/*' | xargs grep -l "useK8sModel" | head -5Repository: openshift/console
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for useK8sModel export/definition
rg "export.*useK8sModel|function useK8sModel" -A 20 -B 2Repository: openshift/console
Length of output: 2556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the specific file to see actual usage
cat -n frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx | sed -n '156,191p'Repository: openshift/console
Length of output: 1306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete useK8sModel implementation
cat -n frontend/packages/console-dynamic-plugin-sdk/src/utils/k8s/hooks/useK8sModel.tsRepository: openshift/console
Length of output: 1958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the type definition for UseK8sModel
rg "type UseK8sModel|interface UseK8sModel" -A 5 -B 2Repository: openshift/console
Length of output: 1436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if getK8sModel can return undefined
rg "getK8sModel" -A 10 | head -40Repository: openshift/console
Length of output: 2750
Guard overlay launch until the model is loaded.
useK8sModel returns [K8sModel, inFlight] where the second element indicates loading state. Launching the modal while inFlight is true risks passing an undefined model to k8sUpdate. Gate the overlay launch and disable the button until the model is ready.
🔧 Suggested fix
- const [model] = useK8sModel(referenceFor(obj));
+ const [model, inFlight] = useK8sModel(referenceFor(obj));
@@
const onClick = () => {
+ if (inFlight || !model) {
+ return;
+ }
const description = t('olm~Define the resource {{type}} for this {{kind}} instance.', {
@@
<Button
icon={<PencilAltIcon />}
iconPosition="end"
type="button"
isInline
data-test-id="configure-modal-btn"
+ isDisabled={inFlight || !model}
onClick={onClick}🤖 Prompt for AI Agents
In
`@frontend/packages/operator-lifecycle-manager/src/components/descriptors/spec/resource-requirements.tsx`
around lines 156 - 191, The overlay is launched before the K8s model finishes
loading; update ResourceRequirementsModalLink to destructure the loading flag
from useK8sModel (e.g., const [model, inFlight] = useK8sModel(...)), prevent
launchOverlay in onClick when inFlight is true (early return) and pass a
disabled prop to the Button (and optionally a tooltip or aria-busy) while
inFlight so the modal cannot be opened until the model is ready.
| type UninstallOperatorModalProviderProps = { | ||
| k8sKill: (kind: K8sKind, resource: K8sResourceKind, options: any, json: any) => Promise<any>; | ||
| k8sGet: (kind: K8sKind, name: string, namespace: string) => Promise<K8sResourceKind>; | ||
| k8sPatch: ( | ||
| kind: K8sKind, | ||
| resource: K8sResourceKind, | ||
| data: { op: string; path: string; value: any }[], | ||
| ) => Promise<any>; | ||
| subscription: K8sResourceKind; | ||
| csv?: K8sResourceKind; | ||
| }; |
There was a problem hiding this comment.
Props type includes k8sGet and k8sPatch but these appear unused by the modal.
The UninstallOperatorModalProviderProps defines k8sGet and k8sPatch, but the UninstallOperatorModal component doesn't destructure these from props. The modal imports k8sPatch directly from @console/internal/module/k8s (line 43) and uses k8sGetResource from the dynamic-plugin-sdk (line 16).
If these props are intended for future testability/mocking, consider either:
- Actually using the props in the modal instead of direct imports
- Removing them from the props types if they're not needed
This is a minor inconsistency but could cause confusion for future maintainers.
🔧 Option 1: Remove unused props
type UninstallOperatorModalProviderProps = {
k8sKill: (kind: K8sKind, resource: K8sResourceKind, options: any, json: any) => Promise<any>;
- k8sGet: (kind: K8sKind, name: string, namespace: string) => Promise<K8sResourceKind>;
- k8sPatch: (
- kind: K8sKind,
- resource: K8sResourceKind,
- data: { op: string; path: string; value: any }[],
- ) => Promise<any>;
subscription: K8sResourceKind;
csv?: K8sResourceKind;
};And similarly for UninstallOperatorModalProps.
📝 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.
| type UninstallOperatorModalProviderProps = { | |
| k8sKill: (kind: K8sKind, resource: K8sResourceKind, options: any, json: any) => Promise<any>; | |
| k8sGet: (kind: K8sKind, name: string, namespace: string) => Promise<K8sResourceKind>; | |
| k8sPatch: ( | |
| kind: K8sKind, | |
| resource: K8sResourceKind, | |
| data: { op: string; path: string; value: any }[], | |
| ) => Promise<any>; | |
| subscription: K8sResourceKind; | |
| csv?: K8sResourceKind; | |
| }; | |
| type UninstallOperatorModalProviderProps = { | |
| k8sKill: (kind: K8sKind, resource: K8sResourceKind, options: any, json: any) => Promise<any>; | |
| subscription: K8sResourceKind; | |
| csv?: K8sResourceKind; | |
| }; |
🤖 Prompt for AI Agents
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`
around lines 667 - 677, UninstallOperatorModalProviderProps declares k8sGet and
k8sPatch but UninstallOperatorModal doesn't use them (it imports k8sPatch from
`@console/internal/module/k8s` and uses k8sGetResource elsewhere), so either
remove k8sGet and k8sPatch from UninstallOperatorModalProviderProps (and
corresponding UninstallOperatorModalProps) to avoid dead API surface, or
refactor UninstallOperatorModal to accept and use the injected props
(k8sGet/k8sPatch) instead of importing k8sPatch directly; update all
consumers/tests to pass the props if you choose injection, or remove the unused
prop declarations if you choose cleanup.
| if (new URLSearchParams(window.location.search).has('showDelete')) { | ||
| uninstallOperatorModal(); | ||
| launcher(UninstallOperatorModalProvider, { | ||
| k8sKill, | ||
| k8sGet, | ||
| k8sPatch, | ||
| subscription: obj, | ||
| }); | ||
| removeQueryArgument('showDelete'); | ||
| } |
There was a problem hiding this comment.
Invoking overlay launcher during render is a side effect—move to useEffect.
Calling launcher() directly in the component body (outside of an event handler or useEffect) executes during render, which violates React's rules against side effects in render. If a re-render occurs, this could launch the modal multiple times or cause unpredictable behavior.
Wrap this logic in a useEffect to handle the query param trigger cleanly:
Proposed fix
+ useEffect(() => {
+ if (new URLSearchParams(window.location.search).has('showDelete')) {
+ launcher(UninstallOperatorModalProvider, {
+ k8sKill,
+ k8sGet,
+ k8sPatch,
+ subscription: obj,
+ });
+ removeQueryArgument('showDelete');
+ }
+ }, [launcher, obj]);
+
- if (new URLSearchParams(window.location.search).has('showDelete')) {
- launcher(UninstallOperatorModalProvider, {
- k8sKill,
- k8sGet,
- k8sPatch,
- subscription: obj,
- });
- removeQueryArgument('showDelete');
- }📝 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.
| if (new URLSearchParams(window.location.search).has('showDelete')) { | |
| uninstallOperatorModal(); | |
| launcher(UninstallOperatorModalProvider, { | |
| k8sKill, | |
| k8sGet, | |
| k8sPatch, | |
| subscription: obj, | |
| }); | |
| removeQueryArgument('showDelete'); | |
| } | |
| useEffect(() => { | |
| if (new URLSearchParams(window.location.search).has('showDelete')) { | |
| launcher(UninstallOperatorModalProvider, { | |
| k8sKill, | |
| k8sGet, | |
| k8sPatch, | |
| subscription: obj, | |
| }); | |
| removeQueryArgument('showDelete'); | |
| } | |
| }, [launcher, obj]); |
🤖 Prompt for AI Agents
In `@frontend/packages/operator-lifecycle-manager/src/components/subscription.tsx`
around lines 431 - 439, The code calls launcher(UninstallOperatorModalProvider,
...) during render which is a side effect; move this logic into a React
useEffect hook so it runs after render when the URL contains showDelete.
Specifically, create a useEffect that checks new
URLSearchParams(window.location.search).has('showDelete'), calls
launcher(UninstallOperatorModalProvider, { k8sKill, k8sGet, k8sPatch,
subscription: obj }), then calls removeQueryArgument('showDelete'); include
k8sKill, k8sGet, k8sPatch, obj (subscription) in the effect dependencies as
appropriate (or an empty array if you only want it once) to avoid repeated
launches on rerenders.
There was a problem hiding this comment.
Task: Move overlay launcher call from render to useEffect
The current code calls launcher() directly during component render, which violates React's rules against side effects. This needs to be wrapped in a useEffect hook to run after render.
Change required:
- Wrap the showDelete query param check and launcher invocation in a
useEffect - Include
launcherandobjin the dependency array (k8sKill,k8sGet,k8sPatchare stable module imports and don't need to be in deps)
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
| export const taintsModal = (props) => | ||
| import('./taints-modal' /* webpackChunkName: "taints-modal" */).then((m) => m.taintsModal(props)); | ||
| import('./taints-modal' /* webpackChunkName: "taints-modal" */).then((m) => | ||
| m.TaintsModalProvider(props), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the taints-modal implementation
find . -name "taints-modal*" -type f | head -20Repository: openshift/console
Length of output: 113
🏁 Script executed:
# Check the taints-modal implementation
cat -n frontend/public/components/modals/taints-modal.tsx | head -100Repository: openshift/console
Length of output: 3986
🏁 Script executed:
# Search for consumers of taintsModal from the barrel export
rg -n "from ['\"].*components/modals['\"]" --type=ts --type=tsx | head -30Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Search for direct taintsModal usage patterns
rg -n "taintsModal" --type=ts --type=tsx | grep -v "index.ts" | grep -v "taints-modal.tsx"Repository: openshift/console
Length of output: 88
🏁 Script executed:
# Check the modals index.ts for context around the taintsModal export
cat -n frontend/public/components/modals/index.ts | sed -n '60,75p'Repository: openshift/console
Length of output: 808
🏁 Script executed:
# Search for TaintsModalProvider export in taints-modal.tsx
rg -n "TaintsModalProvider" frontend/public/components/modals/taints-modal.tsxRepository: openshift/console
Length of output: 223
🏁 Script executed:
# Search for consumers of taintsModal from the barrel export
rg -n "from.*modals['\"]" --type=ts | grep -v index.tsRepository: openshift/console
Length of output: 3512
🏁 Script executed:
# Check what's exported from taints-modal
rg -n "export" frontend/public/components/modals/taints-modal.tsxRepository: openshift/console
Length of output: 133
🏁 Script executed:
# Compare with other modal patterns - check tolerations-modal for pattern
rg -n "export|Modal" frontend/public/components/modals/tolerations-modal.tsx | head -30Repository: openshift/console
Length of output: 672
🏁 Script executed:
# Check if there are actual direct usages of taintsModal pattern
rg -n "taintsModal\(" --type=tsRepository: openshift/console
Length of output: 43
🏁 Script executed:
# Search specifically for taintsModal imports
rg -n "taintsModal" --type=ts --type=jsRepository: openshift/console
Length of output: 144
🏁 Script executed:
# Check what OverlayComponent actually is
rg -n "type OverlayComponent|interface OverlayComponent" --type=tsRepository: openshift/console
Length of output: 238
🏁 Script executed:
# Get context around the OverlayComponent definition
rg -B5 -A10 "type OverlayComponent" --type=tsRepository: openshift/console
Length of output: 1895
🏁 Script executed:
# Check if there are any recent commits changing this pattern
git log --oneline --all -- frontend/public/components/modals/taints-modal.tsx | head -5Repository: openshift/console
Length of output: 367
Remove or update this barrel export—it's using an incorrect pattern that doesn't work with the overlay system.
The current export calls m.TaintsModalProvider(props) directly, which invokes the OverlayComponent as a function. This returns unreachable JSX instead of mounting the modal via the overlay system. The component expects to be passed to launchOverlay() with the closeOverlay callback, not invoked standalone.
However, searching the codebase shows no active consumers of this taintsModal export, suggesting it's vestigial from the overlay migration. Either remove this export if all consumers have migrated to launchOverlay/useOverlay, or update it to match the proper pattern (like tolerationsModal still using createModalLauncher).
🤖 Prompt for AI Agents
In `@frontend/public/components/modals/index.ts` around lines 68 - 71, The
exported taintsModal currently calls m.TaintsModalProvider(props) which invokes
the overlay component instead of returning a launcher; either remove this
vestigial export if unused, or change it to the overlay-launcher pattern used by
tolerationsModal: dynamically import TaintsModalProvider but return a launcher
(e.g., via createModalLauncher or a function that calls launchOverlay with the
component and closeOverlay) so the component is passed to
launchOverlay/useOverlay rather than invoked directly; update references to
taintsModal to use the new launcher or delete the export if no consumers remain.
There was a problem hiding this comment.
We should be using the lazy pattern
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what's actually exported from taints-modal.tsx
echo "=== Exports from taints-modal.tsx ==="
rg -n "^export" frontend/public/components/modals/taints-modal.tsx
echo -e "\n=== Check if TaintsModalOverlay exists ==="
rg -n "TaintsModalOverlay" frontend/public/components/modals/taints-modal.tsx
echo -e "\n=== Check for any usages of taintsModal from the barrel export ==="
rg -n "import.*taintsModal.*from.*modals" --type=ts --type=tsx
echo -e "\n=== Check index.ts imports ==="
cat frontend/public/components/modals/index.ts | grep -A 3 "taintsModal"
Length of output: 594
🏁 Script executed:
#!/bin/bash
# Search for any usages of taintsModal from the barrel export
echo "=== Search for taintsModal imports ==="
rg -n "taintsModal" --type=ts --type=js | grep -v "index.ts" | grep -v "taints-modal.tsx"
echo -e "\n=== Check imports from modals barrel that might use taintsModal ==="
rg -n "from ['\"].*components/modals['\"]" --type=ts | head -20
echo -e "\n=== Check for direct imports from taints-modal ==="
rg -n "from ['\"].*taints-modal" --type=ts
Length of output: 2903
@rhamilto, you're absolutely right! The export should use the lazy pattern to align with the other overlay-based modals in this file. Here's the corrected code:
// Lazy-loaded OverlayComponent for Taints Modal
export const LazyTaintsModalOverlay = lazy(() =>
import('./taints-modal' /* webpackChunkName: "taints-modal" */).then((m) => ({
default: m.TaintsModalOverlay,
})),
);This follows the same pattern as other overlay modals like LazyDeleteModalOverlay, LazyExpandPVCModalOverlay, etc. The current function-based export taintsModal should be removed since:
- It incorrectly calls the OverlayComponent as a function instead of providing it to the lazy loader
- No code in the repository uses this barrel export (the one consumer in
useCommonActions.tsimportsTaintsModalOverlaydirectly from the source file)
The lazy pattern ensures the modal is code-split into its own chunk and loaded on-demand, which is the correct approach for overlay-based modals in the new pattern.
openshift-merge-robot
added
the
needs-rebase
frontend/packages/console-app/src/actions/hooks/useCommonActions.ts
Outdated
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/src/actions/hooks/useSubscriptionActions.ts
Outdated
Show resolved
Hide resolved
Remove React Router wrapper from taints modal to unblock Router v7 upgrade. Part of CONSOLE-5012 to eliminate all createModalLauncher usage. Assisted by: Claude Code
Converts the modal to use the new overlay pattern, replacing createModalLauncher with useOverlay hook. Assisted by: Claude Code
sg00dwin
force-pushed
the
CONSOLE-5049-olm-modals-migration
branch
from
91f6307 to
1bb42db
Compare
openshift-merge-robot
removed
the
needs-rebase
|
[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 |
frontend/packages/console-app/src/actions/hooks/useCommonActions.ts
Outdated
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx
Outdated
Show resolved
Hide resolved
...end/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx
Outdated
Show resolved
Hide resolved
...end/packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx
Outdated
Show resolved
Hide resolved
- Consolidate modal prop types using ModalComponentProps pattern - Remove duplicate OverlayProps types across all OLM modals - Fix resource loading race condition in resource-requirements - Wrap modal launch in useEffect to avoid render side effects - Remove unused k8sGet and k8sPatch props from UninstallOperatorModal - Remove vestigial taintsModal barrel export Assisted by: Claude Code
rhamilto
left a comment
rhamilto
left a comment
There was a problem hiding this comment.
Claude isn't great about following patterns until it knows what you want. You have to tell it. Note a couple of comments apply in more than one place.
| cancel?: () => void; | ||
| close?: () => void; | ||
| k8sKill: (kind: K8sKind, resource: K8sResourceKind, options: any, json: any) => Promise<any>; | ||
| k8sGet: (kind: K8sKind, name: string, namespace: string) => Promise<K8sResourceKind>; |
There was a problem hiding this comment.
k8sGet and k8sPatch are still needed.
| }; | ||
|
|
||
| const DisableDefaultSourceModalOverlay: OverlayComponent<DisableDefaultSourceModalOverlayProps> = ( | ||
| export type DisableDefaultSourceModalProps = { |
There was a problem hiding this comment.
Our existing convention is to put types at the end.
| kind={props.kind} | ||
| operatorHub={props.operatorHub} | ||
| sourceName={props.sourceName} | ||
| {...props} |
There was a problem hiding this comment.
I'd keep these as they were to minimize unnecessary changes.
|
@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. |
…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
splitting. Fix export pattern violations (use export const), add modals/index.ts with lazy exports, and update all consumers to use lazy imports. Restore k8sGet and k8sPatch props to UninstallOperatorModal per code review feedback. Assisted by Claude
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@sg00dwin: The following tests failed, say
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. |
| resourceKind: K8sKind; | ||
| resource: NodeKind; | ||
| close: () => void; | ||
| resource: K8sResourceKind; |
There was a problem hiding this comment.
Should not have been included. Reverted
| close?: () => void; | ||
| } & ModalComponentProps; | ||
|
|
||
| export const UpdateStrategyModalOverlay: OverlayComponent<UpdateStrategyModalProps> = (props) => { |
There was a problem hiding this comment.
Move this before type UpdateStrategyModalProps
4 participants
Description
Migrates OLM and related modals from the deprecated
createModalLauncherpattern to the modernOverlayComponentpattern. This is part of CONSOLE-5012 to modernize modal handling across the console.Changes
Modals Migrated (8 total)
All modals now follow the
OverlayComponentpattern with consistent implementation:TaintsModal (
public/components/modals/taints-modal.tsx)TaintsModalProvider: OverlayComponentcreateModalLauncherusageUpdateStrategyModal (
packages/operator-lifecycle-manager/src/components/modals/update-strategy-modal.tsx)UpdateStrategyModalProvider: OverlayComponentcreateModalLauncherusageEditDefaultSourcesModal (
packages/operator-lifecycle-manager/src/components/modals/edit-default-sources-modal.tsx)EditDefaultSourcesModalProvider: OverlayComponentcreateModalLauncherusageDisableDefaultSourceModal (
packages/operator-lifecycle-manager/src/components/modals/disable-default-source-modal.tsx)DisableDefaultSourceModalProvider: OverlayComponentcreateModalLauncherusageInstallPlanApprovalModal (
packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.tsx)InstallPlanApprovalModalProvider: OverlayComponentcreateModalLauncherusageSubscriptionChannelModal (
packages/operator-lifecycle-manager/src/components/modals/subscription-channel-modal.tsx)SubscriptionChannelModalProvider: OverlayComponentcreateModalLauncherusageInstallPlanPreviewModal (
packages/operator-lifecycle-manager/src/components/modals/installplan-preview-modal.tsx)InstallPlanPreviewModalProvider: OverlayComponentcreateModalLauncherusageUninstallOperatorModal (
packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx)UninstallOperatorModalProvider: OverlayComponentcreateModalLauncherusageUpdated to Use New Overlay Modals
Replaced direct modal function calls with
useOverlay():useCommonActions.ts: Changed fromtaintsModaltolaunchOverlay(TaintsModalProvider, ...)useOperatorActions.ts: Changed to uselauncher(UninstallOperatorModalProvider, ...)in action ctauseSubscriptionActions.ts: Changed to uselauncher(UninstallOperatorModalProvider, ...)in action ctasubscription.tsx: Updated to useuseOverlay()for InstallPlanApprovalModal, SubscriptionChannelModal, and UninstallOperatorModalinstall-plan.tsx: Changed to uselaunchOverlay(InstallPlanPreviewModalProvider, ...)configure-update-strategy.tsx: Changed to uselaunchOverlay(UpdateStrategyModalProvider, ...)operator-hub-details.tsx: Changed to uselaunchOverlay(EditDefaultSourcesModalProvider, ...)catalog-source-provider.ts: Changed to uselaunchOverlay(DisableDefaultSourceModalProvider, ...)React Hooks Dependency Array Updates
Added
launcher/launchOverlayto dependency arrays where used:useCommonActions.ts: AddedlaunchOverlayto dependency arrayuseOperatorActions.ts: Addedlauncherto dependency arrayuseSubscriptionActions.ts: Addedlauncherto dependency arraycatalog-source-provider.ts: AddedlaunchOverlayto dependency arrayuninstall-operator-modal.tsx: AddeduseCallbackfor functions used in useEffect dependencies to satisfy exhaustive-deps rulesNote: While
launchOverlayfromuseOverlay()is referentially stable, it's included in dependency arrays for correctness and to satisfy exhaustive-deps linting rules.Naming Convention
All modal components use named exports following team standards per feedback from rhamilto on PR #15935:
TaintsModalProvider,UninstallOperatorModalProvider,UpdateStrategyModalProvider, etc.import { TaintsModalProvider } from ...public/components/modals/index.tsto use named exportRelated
Assisted-by: Claude Code
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.