CONSOLE-5049: Migrate DeleteResourceModal to useOverlay pattern #16022
Conversation
|
@sg00dwin: No Jira issue with key MCONSOLE-5049 exists in the tracker at https://issues.redhat.com/. 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
|
@sg00dwin: No Jira issue with key MCONSOLE-5049 exists in the tracker at https://issues.redhat.com/. 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. |
|
[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/helm
|
@sg00dwin: No Jira issue with key MCONSOLE-5049 exists in the tracker at https://issues.redhat.com/. 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 migrates modal patterns across multiple frontend packages from a launcher-based approach to an overlay component architecture. DeleteResourceModal, UninstallOperatorModal, and related action creators are refactored to use OverlayComponent-based wrappers and React hooks. Modal launchers are replaced with lazy-loaded overlay components. Navigation is updated to use route-based navigation instead of history manipulation. Action creators (DeleteApplicationAction, getHelmDeleteAction) are converted to hook-based patterns. The redirect field in HelmActionsScope changes from boolean to string. Modal props are simplified and aligned with the new overlay-driven flow. 🚥 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 1
🤖 Fix all issues with AI agents
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`:
- Around line 650-656: The hook useUninstallOperatorModal currently depends on
the entire props object which forces callers to memoize; change it to
destructure the specific fields from UninstallOperatorModalProps and include
those individual fields (e.g., kind, resource, onClose, etc.) in the useCallback
dependency array instead of the whole props object, or switch to the established
pattern used by useRemoveModalLauncher that takes launcher and explicit prop
values (depend on [launchModal, /* individual props */]) and pass the
reconstructed props into UninstallOperatorModalOverlay when calling launchModal;
update the signature of useUninstallOperatorModal accordingly so callers need
not memoize an inline object.
🧹 Nitpick comments (3)
frontend/packages/helm-plugin/src/actions/creators.ts (1)
1-49: Encode Helm release params before building the delete URL.Release names/namespaces can include characters that should be URL-encoded; encoding avoids edge-case request failures and keeps the query string well-formed.
♻️ Proposed hardening
- return coFetchJSON.delete( - `/api/helm/release/async?name=${releaseName}&ns=${namespace}&version=${releaseVersion}`, - null, - null, - -1, - ); + return coFetchJSON.delete( + `/api/helm/release/async?name=${encodeURIComponent( + releaseName, + )}&ns=${encodeURIComponent(namespace)}&version=${encodeURIComponent( + String(releaseVersion), + )}`, + null, + null, + -1, + );frontend/packages/console-shared/src/components/modals/DeleteResourceModal.tsx (1)
98-113: Overlay wrapper is functional but verbose.The explicit prop passing works correctly. The
blockingprop onModalWrapperis appropriate for a destructive delete operation—preventing accidental dismissal via overlay click.Consider simplifying with a spread operator for maintainability:
♻️ Optional: Reduce verbosity with spread
export const DeleteResourceModalOverlay: OverlayComponent<DeleteResourceModalProps> = (props) => { + const { closeOverlay, ...rest } = props; return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <DeleteResourceModal - close={props.closeOverlay} - cancel={props.closeOverlay} - resourceName={props.resourceName} - resourceType={props.resourceType} - actionLabel={props.actionLabel} - actionLabelKey={props.actionLabelKey} - redirect={props.redirect} - onSubmit={props.onSubmit} - /> + <ModalWrapper blocking onClose={closeOverlay}> + <DeleteResourceModal close={closeOverlay} cancel={closeOverlay} {...rest} /> </ModalWrapper> ); };frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx (1)
660-672: Remove unused type props to reduce API surface.
k8sGetandk8sPatchare defined inUninstallOperatorModalPropsbut never used. The component destructures onlycancel,close,csv,k8sKill, andsubscription(line 59). Instead,k8sPatchis imported directly from@console/internal/module/k8s(line 37) and used at line 171, while the component relies onk8sGetResourcefrom the SDK (line 10). Removing these props cleans up the type contract and eliminates confusion about the actual API surface.🧹 Clean up unused type props
export type UninstallOperatorModalProps = { cancel?: () => void; close?: () => void; 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; };
| export const useUninstallOperatorModal = (props: UninstallOperatorModalProps) => { | ||
| const launcher = useOverlay(); | ||
| const launchModal = useOverlay(); | ||
| return useCallback( | ||
| () => launcher<UninstallOperatorModalProviderProps>(UninstallOperatorModalProvider, props), | ||
| [launcher, props], | ||
| () => launchModal<UninstallOperatorModalOverlayProps>(UninstallOperatorModalOverlay, props), | ||
| [launchModal, props], | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all useOverlay-based hooks to understand the pattern
rg -n -B2 -A10 'useOverlay\(\)' --type=ts --type=tsx | head -100Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check the specific hook and its usage
rg -n -B5 -A15 'export const useUninstallOperatorModal' --type=ts --type=tsxRepository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for how useUninstallOperatorModal is called in the codebase
rg -n 'useUninstallOperatorModal' --type=ts --type=tsx -B2 -A2 | head -80Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check useOperatorActions.ts to see if memoization was added
rg -n -B5 -A20 'useUninstallOperatorModal\|useOperatorActions' --type=ts --type=tsx | grep -A20 'useOperatorActions' | head -60Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check UninstallOperatorModalProps type to understand what props are
rg -n 'interface UninstallOperatorModalProps\|type UninstallOperatorModalProps' --type=ts --type=tsx -A8Repository: openshift/console
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Find all useOverlay-based hooks (search without type restriction)
rg -n -B2 -A10 'useOverlay\(\)' | head -120Repository: openshift/console
Length of output: 10016
🏁 Script executed:
#!/bin/bash
# Check the specific hook location and implementation
cat -n frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx | sed -n '640,670p'Repository: openshift/console
Length of output: 1402
🏁 Script executed:
#!/bin/bash
# Search for UninstallOperatorModalProps definition
rg -n 'interface UninstallOperatorModalProps|type UninstallOperatorModalProps' -A8Repository: openshift/console
Length of output: 1351
🏁 Script executed:
#!/bin/bash
# Search for how useUninstallOperatorModal is used
rg -n 'useUninstallOperatorModal' -B3 -A3Repository: openshift/console
Length of output: 6776
🏁 Script executed:
#!/bin/bash
# Check if useOperatorActions.ts uses memoization
rg -n 'useOperatorActions\|useUninstallOperatorModal' -B5 -A15 frontend/packages/operator-lifecycle-manager/src/Repository: openshift/console
Length of output: 43
Dependency on props object in useCallback is a footgun for callers.
This hook includes props in the dependency array, requiring callers to memoize. While useOperatorActions.ts correctly memoizes uninstallOperatorModalProps, both useSubscriptionActions.ts (line 34) and subscription.tsx (line 431) pass inline objects, causing unnecessary re-renders and callback churn on every render.
Other similar modal hooks across the codebase (useDeleteNamespaceModalLauncher, useErrorModalLauncher, useAddSecretToWorkloadModal) follow the same risky pattern. A safer alternative exists in useRemoveModalLauncher, which destructures and depends on individual properties ([launcher, kind, resource, volume]) rather than object identity.
Either document this requirement prominently or restructure to depend on individual props rather than object identity. The latter is safer for maintainability since it eliminates the implicit contract that callers must memoize.
🤖 Prompt for AI Agents
In
`@frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.tsx`
around lines 650 - 656, The hook useUninstallOperatorModal currently depends on
the entire props object which forces callers to memoize; change it to
destructure the specific fields from UninstallOperatorModalProps and include
those individual fields (e.g., kind, resource, onClose, etc.) in the useCallback
dependency array instead of the whole props object, or switch to the established
pattern used by useRemoveModalLauncher that takes launcher and explicit prop
values (depend on [launchModal, /* individual props */]) and pass the
reconstructed props into UninstallOperatorModalOverlay when calling launchModal;
update the signature of useUninstallOperatorModal accordingly so callers need
not memoize an inline object.
rhamilto
changed the title
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. |
| subscription, | ||
| csv: resource, | ||
| }), | ||
| [subscription, resource], |
There was a problem hiding this comment.
Missing props in the dependency array
There was a problem hiding this comment.
eslint is saying these are unnecessary dependencies since they are outer scope values (module imports) that don't cause re-renders
yarn eslint frontend/packages/operator-lifecycle-manager/src/actions/useOperatorActions.ts
28:5 error React Hook useMemo has unnecessary dependencies: 'k8sGet', 'k8sKill', and 'k8sPatch'. Either exclude them or remove the dependency array. Outer scope values like 'k8sKill' aren't valid dependencies because mutating them doesn't re-render the component
…er v6 Update DeleteResourceModal, dev-console actions, and helm-plugin actions from history.push() to useNavigate() for React Router v7 compatibility. Assisted by: Claude Code
sg00dwin
force-pushed
the
CONSOLE-5049-router-dependent-modals
branch
from
b34ed9f to
234ba09
Compare
|
@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. |
|
@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
changed the title
|
/assign @yanpzhan Tech debt, so assigning labels |
openshift-ci
bot
added
px-approved
4 participants
This pull request migrates two OLM modals from the legacy
createModalLauncherpattern and React Router v5history.push()to the modernOverlayComponentpattern withuseNavigate(). This migration is part of CONSOLE-5012 (modal migration) and removes blockers for the React Router v7 upgrade (CONSOLE-5049).Changes Made
Modals Migrated (2 total)
1. DeleteResourceModal - Deletes operator-managed resources
Files Modified
DeleteResourceModal:
packages/operator-lifecycle-manager/src/components/modals/DeleteResourceModal.tsxcreateModalLauncherimport and exportuseOverlayimportDeleteResourceOverlaywithOverlayComponenttypeModalWrapperwith proper prop spreadingmodals/index.tspackages/operator-lifecycle-manager/src/components/modals/index.ts(created)LazyDeleteResourceOverlayLazyUninstallOperatorModalOverlaypackages/operator-lifecycle-manager/src/actions/useOperatorActions.tsUninstallOperatorOverlaytouseUninstallOperatorModallauncher→launchModal(migration guide compliance)uninstallOperatorModalTechnical Implementation
Pattern Consolidation
The modal had four conflicting patterns in the partially migrated state:
createModalLauncherexport (unused)UninstallOperatorOverlay(manual Backdrop + Modal)UninstallOperatorModalProvider(used ModalWrapper)useUninstallOperatorModal(non-standard but working)Migrated to:
ModalWrapper*Overlaysuffix)This pattern:
Consumers
Updated:
useOperatorActions.ts- Changed from incorrect direct overlay to hook patternVerified (no changes needed):
useSubscriptionActions.ts- Already using hook pattern correctlysubscription.tsx- Already using hook pattern correctlyBug Fixes
1. Infinite Render Loop
Problem:
The props object passed to
useUninstallOperatorModalwas created fresh on every render, causinguseCallbackto return a new function each time, triggeringuseMemoto recompute infinitely.Error:
Solution:
Memoized the props object to only change when dependencies change:
File:
useOperatorActions.ts:19-312. Network Error Handling
Problem:
When the network is offline, the error object doesn't have a
responseproperty, causingerr.response.statusto crash with "Cannot read properties of undefined (reading 'status')".Error:
Solution:
Added optional chaining to safely access the response status:
Summary by CodeRabbit
Release Notes
Bug Fixes
Refactor