CONSOLE-5012: Migrate delete modals and PVC modals to overlay pattern #15932
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. |
openshift-ci
bot
added
component/core
rhamilto
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@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. |
📝 WalkthroughWalkthroughThe PR replaces direct modal launchers with Overlay-based provider components and lazy-loaded provider wrappers. Modal implementations (Clone, Expand, Delete, Restore, DeleteModal, etc.) now export OverlayComponent providers that wrap modals in ModalWrapper and wire closeOverlay. Action hooks and action creators were updated to call useOverlay/launchModal with Lazy*ModalProvider providers; several action factories were converted to hook forms (e.g., useDeleteResourceAction, useDeleteApplicationAction, useDeleteKnativeServiceResource). Modal.setAppElement is now initialized once in App via useEffect, per-call initialization removed, and OverlayProvider renders dynamic components inside Suspense. Some memo dependency arrays dropped launchModal with eslint-disable comments. 🚥 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
🤖 Fix all issues with AI agents
In `@frontend/public/components/modals/delete-pvc-modal.tsx`:
- Around line 9-16: Restore a default export so existing callers importing the
default don't break: export the modal component previously exposed as the
default (the DeletePVCModal component used with createModalLauncher) as the
file's default export in addition to any named exports; locate the
DeletePVCModal symbol (and the modal wrapper/props usage such as
ModalComponentProps) and add a default export for it so index
re-exports/back-compat continue to work.
In `@frontend/public/components/modals/index.ts`:
- Line 3: OverlayProvider is rendering lazy components from componentsMap
without a Suspense boundary causing runtime throws; import Suspense from 'react'
and wrap each mapped provider render (the map that returns <c.Component
{...c.props} closeOverlay={() => closeOverlay(id)} />) in <Suspense
fallback={null}> so each lazy modal provider is suspended safely; apply the same
change to the LazyClonePVCModalProvider and LazyRestorePVCModalProvider render
blocks so their lazy components are also wrapped with Suspense.
🧹 Nitpick comments (2)
frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx (1)
279-285: Provider implementation looks solid; consider explicit prop destructuring for clarity.The pattern is correct:
blockingprevents accidental dismissal during form entry, and mapping bothcancelandclosetocloseOverlayaligns with howClonePVCModaluses them (success path at line 132, cancel button at line 269).One minor refinement: spreading
...propspassescloseOverlayto the inner modal which doesn't consume it. While harmless, destructuring makes the prop threading more explicit:✨ Optional: explicit prop destructuring
-export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { +export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = ({ + closeOverlay, + ...modalProps +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ClonePVCModal cancel={closeOverlay} close={closeOverlay} {...modalProps} /> </ModalWrapper> ); };frontend/public/components/modals/expand-pvc-modal.tsx (1)
4-11: Keepclosenon‑optional to prevent runtimeundefinedcalls.Line 97 now inherits
close?: () => voidfromModalComponentProps, but Line 47 callsclose()unguarded. Consider keepingcloserequired inExpandPVCModalProps(even if you still reuse the shared type) to avoid accidental misuse in tests or future direct renders.♻️ Suggested type tightening
export type ExpandPVCModalProps = { kind: K8sKind; resource: K8sResourceKind; -} & ModalComponentProps; +} & Required<Pick<ModalComponentProps, 'close'>> & + Pick<ModalComponentProps, 'cancel'>;Also applies to: 97-108
rhamilto
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@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. |
1 similar comment
|
@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. |
openshift-ci
bot
added
component/dashboard
rhamilto
force-pushed
the
CONSOLE-5012
branch
3 times, most recently
from
7354328 to
3dc1895
Compare
rhamilto
force-pushed
the
CONSOLE-5012
branch
from
3dc1895 to
b6487b4
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. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/public/components/modals/delete-modal.tsx (1)
171-181: Remove the unusedDeleteOverlaycomponent.
DeleteOverlayis dead code—it's not imported or referenced anywhere in the codebase. The canonical pattern for this modal isDeleteModalProvider, which correctly usesModalWrapperand delegates to theOverlayProvidersystem. This pattern is consistently applied across other modals (expand-pvc-modal,delete-pvc-modal,remove-volume-modal, etc.). RemoveDeleteOverlayto reduce confusion and keep the modal architecture clear.frontend/packages/console-app/src/actions/providers/deployment-provider.ts (1)
17-70: GuarduseDeleteResourceActioncall against undefined model.
useK8sModelcan returnundefinedwhen the model is still loading (inFlight=true), butuseDeleteResourceActionis invoked unconditionally on line 17 without null-safety checks. The hook immediately dereferenceskind.kindin its useMemo (context-menu.ts, line 55), causing a runtime error during model load:Cannot read property 'kind' of undefined.Move the
useDeleteResourceActioncall into thedeploymentActionsuseMemo whereisReadyprotects execution, or add a guard:const deleteResourceAction = kindObj ? useDeleteResourceAction(kindObj, resource) : undefined;and handle the conditional safely in the array spread.
🤖 Fix all issues with AI agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts`:
- Around line 52-64: Remove launchModal from the useMemo dependency array and
add the standard ESLint suppression to prevent the exhaustive-deps warning;
specifically, keep the memoized object creation (id, label, cta calling
launchModal(LazyDeleteModalProvider, { kind, resource: obj, deleteAllResources:
() => cleanUpWorkload(obj) }), accessReview: asAccessReview(kind, obj,
'delete')) but change the dependency list to [t, kind, obj] and place the
single-line ESLint disable comment (react-hooks/exhaustive-deps) immediately
above the dependency array to match the established pattern.
- Around line 27-40: The delete flow in useDeleteApplicationAction still uses
the legacy deleteResourceModal; refactor it to use the overlay pattern by
replacing deleteResourceModal(...) with launchModal(LazyDeleteModalProvider,
...) mirroring the approach in useDeleteResourceAction: pass equivalent
parameters (blocking, resourceName, resourceType) and wire the onSubmit to call
cleanUpWorkload for each item in application.resources and return Promise.all of
those promises; ensure the modal provider receives the same confirmation/props
shape as used by LazyDeleteModalProvider so behavior matches the existing delete
path.
In `@frontend/public/components/modals/expand-pvc-modal.tsx`:
- Around line 99-105: The spread {...props} in ExpandPVCModalProvider can
overwrite the explicit cancel and close bindings; update the provider so the
modal receives the overlay-controlled handlers last (or omit cancel/close from
the spread) — locate ExpandPVCModalProvider and change the JSX to either spread
props before passing cancel={props.closeOverlay} close={props.closeOverlay}, or
destructure props to remove cancel/close (e.g. const { cancel, close, ...rest }
= props) and then render <ExpandPVCModal {...rest} cancel={props.closeOverlay}
close={props.closeOverlay} /> so the overlay's closeOverlay always wins.
♻️ Duplicate comments (1)
frontend/packages/console-app/src/actions/providers/deploymentconfig-provider.ts (1)
18-69: Same model-availability concern as deployment-provider.This file uses the same
useDeleteResourceAction(kindObj, resource)path. Please ensurekindObjis neverundefinedduring render, for the same reason noted in deployment-provider (context-menu.ts Lines 47-65 dereferenceskind.kind).
🧹 Nitpick comments (7)
frontend/packages/console-app/src/actions/providers/machine-config-pool-provider.ts (1)
15-29: Avoid stalelaunchModalby stabilizing it instead of disabling deps.
launchModalis captured incta, but omitted from the dependency list. IfuseOverlay()ever returns a new function (e.g., provider changes), the memoized action can call a stale reference. Prefer stabilizing the handler (ref/useEvent) and keep deps correct rather than disabling the rule.♻️ Suggested refactor (stable ref, no eslint-disable)
-import { useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; @@ const launchModal = useOverlay(); + const launchModalRef = useRef(launchModal); + useEffect(() => { + launchModalRef.current = launchModal; + }, [launchModal]); @@ - cta: () => - togglePaused(MachineConfigPoolModel, obj).catch((err) => - launchModal(ErrorModal, { error: err.message }), - ), + cta: () => + togglePaused(MachineConfigPoolModel, obj).catch((err) => + launchModalRef.current(ErrorModal, { error: err.message }), + ), @@ - // missing launchModal dependency, that causes max depth exceeded error - // eslint-disable-next-line react-hooks/exhaustive-deps [obj, t], );frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/OverlayProvider.tsx (1)
55-57: Consider a non‑null Suspense fallback for overlay UX/a11y.With
fallback={null}, a lazy overlay can appear to do nothing on launch; screen readers also get no loading status. Consider using the project’s standard loading component (PatternFly spinner/LoadingInline/etc.) as the fallback to provide feedback while the overlay code is fetched. Please verify the desired behavior with React Suspense semantics in this app.frontend/packages/console-app/src/actions/providers/build-config-provider.ts (1)
36-38: The eslint-disable here is justified, but the assumption needs correction.The
launchOverlayfunction returned byuseOverlay()is already referentially stable—it's wrapped inuseCallbackwith an empty dependency array inOverlayProvider(line 40). The reason for omitting it isn't instability; including it creates a circular dependency pattern that triggers the "max depth exceeded" infinite re-render loop. This is a pragmatic but fragile workaround.Since the pattern is consistent across this codebase (usePVCActions, useVolumeSnapshotActions, useCommonActions), consider whether the root cause—why modal closures create circular deps—should be addressed at the hook level rather than suppressed across multiple consumers. If this is the intended design, document it in
useOverlay's JSDoc to help future maintainers understand why consumers must skip this dependency.frontend/public/components/modals/expand-pvc-modal.tsx (1)
35-52: Consider handling the patch failure case.The
submitcallback chains.then()without a.catch()or error handling branch. WhileusePromiseHandlercaptures the error for display viaerrorMessage, the promise chain could be clearer about the failure path—particularly sinceclose()andnavigate()only run on success.The current implementation is functionally correct since
handlePromisemanages errors internally, but explicit handling improves readability for future maintainers.frontend/packages/dev-console/src/actions/context-menu.ts (1)
1-11: Unused import:deleteResourceModalstill imported but only used in one hook.If
useDeleteApplicationActionis migrated to the overlay pattern, thedeleteResourceModalimport (line 9) can be removed entirely. Currently it creates a mixed dependency footprint.frontend/public/components/modals/index.ts (1)
89-94: Inconsistent export resolution in lazy PVC modal providers.
LazyClonePVCModalProviderresolves via named exportm.ClonePVCModalProvider, whileLazyExpandPVCModalProviderandLazyRestorePVCModalProviderresolve viam.default. While all three source files export both the named export and a default (so no functional breakage), this inconsistency reduces clarity and makes the code harder to maintain.Consider standardizing on one approach—either use default exports consistently, or use named exports consistently across all three providers.
frontend/packages/knative-plugin/src/actions/providers.ts (1)
98-151: Consider collapsing the two delete‑action hooks into one. This trims duplicate hook work and simplifies the dependency list without changing behavior.♻️ Suggested simplification
- const deleteKnativeServiceFromWebAction = useDeleteKnativeServiceResource( - kindObj, - resource, - serviceTypeValue, - true, - ); - const deleteKnativeServiceAction = useDeleteKnativeServiceResource( - kindObj, - resource, - serviceTypeValue, - false, - ); + const createdFromWeb = + resource?.metadata?.annotations?.['openshift.io/generated-by'] === 'OpenShiftWebConsole'; + const deleteKnativeServiceAction = useDeleteKnativeServiceResource( + kindObj, + resource, + serviceTypeValue, + createdFromWeb, + ); @@ - ...(resource.metadata.annotations?.['openshift.io/generated-by'] === - 'OpenShiftWebConsole' - ? [deleteKnativeServiceFromWebAction] - : [deleteKnativeServiceAction]), + deleteKnativeServiceAction, @@ - deleteKnativeServiceFromWebAction, - deleteKnativeServiceAction, + deleteKnativeServiceAction,
| cta: () => { | ||
| const reqs = []; | ||
| deleteResourceModal({ | ||
| blocking: true, | ||
| resourceName: application.name, | ||
| resourceType: ApplicationModel.label, | ||
| onSubmit: () => { | ||
| application.resources.forEach((resource) => { | ||
| reqs.push(cleanUpWorkload(resource.resource)); | ||
| }); | ||
| return Promise.all(reqs); | ||
| }, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/packages/dev-console/src/actions/context-menu.ts | head -70Repository: openshift/console
Length of output: 178
🏁 Script executed:
rg -n "deleteResourceModal" --type ts -g '!*.test.*' | head -30Repository: openshift/console
Length of output: 178
🏁 Script executed:
rg -n "launchModal.*LazyDeleteModalProvider" --type ts -g '!*.test.*' | head -20Repository: openshift/console
Length of output: 288
🏁 Script executed:
rg -n "deleteResourceModal\|launchModal" frontend/packages/dev-console/src/actions/context-menu.ts -A 2 -B 2Repository: openshift/console
Length of output: 43
🏁 Script executed:
head -20 frontend/packages/dev-console/src/actions/context-menu.tsRepository: openshift/console
Length of output: 1053
🏁 Script executed:
sed -n '13,100p' frontend/packages/dev-console/src/actions/context-menu.tsRepository: openshift/console
Length of output: 1746
🏁 Script executed:
rg "deleteResourceModal" frontend/packages/dev-console/src/actions/context-menu.ts -B 2 -A 5Repository: openshift/console
Length of output: 742
Inconsistent modal pattern in delete actions.
useDeleteApplicationAction continues using the legacy deleteResourceModal from @console/shared, while useDeleteResourceAction below it uses the new launchModal(LazyDeleteModalProvider, ...) overlay pattern. Both export delete actions from the same file, creating inconsistent modal patterns.
Given the PR's stated objective to migrate delete modals to the overlay pattern, clarify whether the legacy deleteResourceModal should be refactored to use launchModal(LazyDeleteModalProvider, ...) with appropriate parameters (similar to useDeleteResourceAction), or if this pattern difference is intentional.
🤖 Prompt for AI Agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts` around lines 27 -
40, The delete flow in useDeleteApplicationAction still uses the legacy
deleteResourceModal; refactor it to use the overlay pattern by replacing
deleteResourceModal(...) with launchModal(LazyDeleteModalProvider, ...)
mirroring the approach in useDeleteResourceAction: pass equivalent parameters
(blocking, resourceName, resourceType) and wire the onSubmit to call
cleanUpWorkload for each item in application.resources and return Promise.all of
those promises; ensure the modal provider receives the same confirmation/props
shape as used by LazyDeleteModalProvider so behavior matches the existing delete
path.
| return useMemo( | ||
| () => ({ | ||
| id: `delete-resource`, | ||
| label: t('devconsole~Delete {{kind}}', { kind: kind.kind }), | ||
| cta: () => | ||
| launchModal(LazyDeleteModalProvider, { | ||
| kind, | ||
| resource: obj, | ||
| deleteAllResources: () => cleanUpWorkload(obj), | ||
| }), | ||
| accessReview: asAccessReview(kind, obj, 'delete'), | ||
| }), | ||
| accessReview: asAccessReview(kind, obj, 'delete'), | ||
| }); | ||
| [t, kind, obj, launchModal], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check how other files in console-app handle launchModal in dependency arrays
rg -n "launchModal" -A 3 -B 1 frontend/packages/console-app/src/actions/ | head -100Repository: openshift/console
Length of output: 178
🏁 Script executed:
# Also check the dev-console package for comparison
rg -n "launchModal" -A 3 -B 1 frontend/packages/dev-console/src/actions/ | head -150Repository: openshift/console
Length of output: 1234
🏁 Script executed:
# Search for eslint-disable comments related to launchModal across the repo
rg "eslint-disable.*launchModal|launchModal.*eslint-disable" -n frontend/Repository: openshift/console
Length of output: 43
🏁 Script executed:
# Find the useOverlay hook definition to understand launchModal's stability
fd -e ts -e tsx | xargs rg -l "useOverlay" | head -5Repository: openshift/console
Length of output: 426
🏁 Script executed:
# Find useOverlay hook definition
rg -n "export.*useOverlay|function useOverlay" -A 5 frontend/Repository: openshift/console
Length of output: 1418
🏁 Script executed:
# Check how other packages use launchModal in useMemo/useCallback
rg -B 5 -A 5 "useMemo|useCallback" frontend/packages/ | rg -B 5 -A 5 "launchModal" | head -200Repository: openshift/console
Length of output: 19822
🏁 Script executed:
# Specifically check knative and metal3 actions for pattern
cat frontend/packages/knative-plugin/src/actions/creators.ts 2>/dev/null | head -100Repository: openshift/console
Length of output: 4242
Remove launchModal from the dependency array.
The established pattern across the codebase (console-app, knative-plugin) excludes launchModal from useMemo dependency arrays with an ESLint disable comment citing "missing launchModal dependency, that causes max depth exceeded error." This avoids circular dependency issues. Align with this pattern by removing launchModal from [t, kind, obj, launchModal] and adding the standard ESLint disable.
🤖 Prompt for AI Agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts` around lines 52 -
64, Remove launchModal from the useMemo dependency array and add the standard
ESLint suppression to prevent the exhaustive-deps warning; specifically, keep
the memoized object creation (id, label, cta calling
launchModal(LazyDeleteModalProvider, { kind, resource: obj, deleteAllResources:
() => cleanUpWorkload(obj) }), accessReview: asAccessReview(kind, obj,
'delete')) but change the dependency list to [t, kind, obj] and place the
single-line ESLint disable comment (react-hooks/exhaustive-deps) immediately
above the dependency array to match the established pattern.
| export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <ExpandPVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | ||
| </ModalWrapper> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Props spread order may override cancel/close bindings.
The spread {...props} comes after the explicit cancel={props.closeOverlay} and close={props.closeOverlay}. If the caller passes cancel or close in props, they will override the closeOverlay bindings, which is likely unintended since the overlay system expects to control modal dismissal.
Proposed fix
export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => {
return (
<ModalWrapper blocking onClose={props.closeOverlay}>
- <ExpandPVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} />
+ <ExpandPVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} />
</ModalWrapper>
);
};📝 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.
| export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ExpandPVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> | |
| </ModalWrapper> | |
| ); | |
| }; | |
| export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ExpandPVCModal {...props} cancel={props.closeOverlay} close={props.closeOverlay} /> | |
| </ModalWrapper> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/public/components/modals/expand-pvc-modal.tsx` around lines 99 -
105, The spread {...props} in ExpandPVCModalProvider can overwrite the explicit
cancel and close bindings; update the provider so the modal receives the
overlay-controlled handlers last (or omit cancel/close from the spread) — locate
ExpandPVCModalProvider and change the JSX to either spread props before passing
cancel={props.closeOverlay} close={props.closeOverlay}, or destructure props to
remove cancel/close (e.g. const { cancel, close, ...rest } = props) and then
render <ExpandPVCModal {...rest} cancel={props.closeOverlay}
close={props.closeOverlay} /> so the overlay's closeOverlay always wins.
3ec43f2 to
df61f3f
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
the
kind/cypress
Migrates PVC-related modals (expand, clone, delete, restore) from the legacy createModalLauncher pattern to the modern OverlayComponent pattern with lazy loading. Changes include: - Convert modal exports to OverlayComponent providers - Implement lazy loading for modal components - Update action hooks to use useOverlay() and launchModal() - Set react-modal app element globally in App component - Maintain backward compatibility with existing modal exports Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5012
branch
from
df61f3f to
2cbcd8e
Compare
openshift-merge-robot
removed
the
needs-rebase
|
[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
force-pushed
the
CONSOLE-5012
branch
from
2cbcd8e to
8015522
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. |
This commit completes the migration from createModalLauncher to the OverlayComponent pattern for delete modals and related actions. Changes: - Refactored delete-modal.tsx to use OverlayComponent pattern - Added LazyDeleteModalProvider with React.lazy() for code splitting - Converted action creators to hooks to comply with React rules: * DeleteResourceAction → useDeleteResourceAction (context-menu.ts) * deleteKnativeServiceResource → useDeleteKnativeServiceResource (creators.ts) - Updated all consumers to use new hook-based actions: * useCommonActions - uses LazyDeleteModalProvider with launchModal * deployment-provider - uses useDeleteResourceAction hook * deploymentconfig-provider - uses useDeleteResourceAction hook * knative-plugin providers - uses useDeleteKnativeServiceResource hook - Updated public/components/modals/index.ts with lazy export - Eliminated all deleteModal() function calls Note: DeleteApplicationAction remains as a non-hook function creator since it doesn't use modals and doesn't need to comply with hooks rules. This change maintains backward compatibility through default exports and preserves lazy loading functionality. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove launchModal from dependency arrays in pre-existing hooks to prevent infinite loops. These files were not modified as part of the modal migration work, but had the same issue. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5012
branch
from
8015522 to
ebc2c5d
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: 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: 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
Summary
Migrates delete modals and PVC-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 (5 total)
All modals now follow the
OverlayComponentpattern with consistent implementation:DeleteModal (
public/components/modals/delete-modal.tsx)DeleteModalProvider: OverlayComponentLazyDeleteModalProviderwith lazy loadingDeletePVCModal (
public/components/modals/delete-pvc-modal.tsx)DeletePVCModalProvider: OverlayComponentExpandPVCModal (
public/components/modals/expand-pvc-modal.tsx)ExpandPVCModalProvider: OverlayComponentLazyExpandPVCModalProviderClonePVCModal (
packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx)ClonePVCModalProvider: OverlayComponentLazyClonePVCModalProviderRestorePVCModal (
packages/console-app/src/components/modals/restore-pvc/restore-pvc-modal.tsx)RestorePVCModalProvider: OverlayComponentLazyRestorePVCModalProviderAction Creators Converted to Hooks (3 total)
To integrate with the React hooks system and
useOverlay()pattern:useDeleteResourceAction(packages/dev-console/src/actions/context-menu.ts)DeleteResourceActionfunction to hookuseOverlay()andLazyDeleteModalProviderdeployment-provider.ts,deploymentconfig-provider.tsuseDeleteApplicationAction(packages/dev-console/src/actions/context-menu.ts)deleteResourceModalfrom@console/shareduseDeleteKnativeServiceResource(packages/knative-plugin/src/actions/creators.ts)deleteKnativeServiceResourcefunction to hookcleanUpWorkloadbased onserviceCreatedFromWebFlagknative-plugin/src/actions/providers.tsReact Hooks Dependency Fixes (Critical)
Fixed infinite loop issues caused by
launchModalin dependency arrays:useCommonActions.ts: RemovedlaunchModalandlaunchCountModalfrom dependency arrayusePVCActions.ts: RemovedlaunchModalfrom dependency arrayuseVolumeSnapshotActions.ts: RemovedlaunchModalfrom dependency arrayRationale:
launchModalfromuseOverlay()maintains referential stability and doesn't need to be in dependency arrays. Including it causes "max depth exceeded" errors.Infrastructure Updates
OverlayProvider.tsx: Added<Suspense>wrapper for lazy-loaded overlay componentsmodals/index.ts: Updated exports to use consistentReact.lazy()pattern with all modals using.then((m) => ({ default: m.default }))Pattern Example
Benefits
Testing
Related