[WIP] CONSOLE-5012: Migrate modals to overlay pattern #15939
Conversation
|
@rhamilto: No Jira issue with key CONSOLE-5212 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@rhamilto: No Jira issue with key CONSOLE-5212 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. |
openshift-ci
bot
added
component/core
|
@rhamilto: No Jira issue with key CONSOLE-5212 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
|
@rhamilto: No Jira issue with key CONSOLE-5212 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. |
rhamilto
changed the title
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. |
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
aebae95 to
0ec415a
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. |
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. |
|
@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. |
📝 WalkthroughWalkthroughThis pull request converts the modal system from direct launcher patterns to overlay-based provider components with lazy loading support. Multiple action hooks and modal exports are refactored to use 🚥 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: 4
🤖 Fix all issues with AI agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts`:
- Around line 1-6: Guard against application.resources being undefined by
defaulting to an empty array before iterating: replace direct calls like
application.resources.forEach(...) with (application.resources ??
[]).forEach(...) (or Array.isArray check) in the delete action and any cleanup
loops that touch application.resources (e.g., the delete modal / cleanup logic
using application.resources), and apply the same change to all other occurrences
in this file where application.resources is iterated.
In `@frontend/packages/knative-plugin/src/actions/creators.ts`:
- Around line 120-153: The useMemo in useDeleteKnativeServiceResource is
including the launchModal value from useOverlay in its dependency array which
forces the memo to recreate on every render; remove launchModal from the
dependencies and leave [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag]
instead, and add the same explanatory comment used elsewhere: "missing
launchModal dependency, that causes max depth exceeded error."; apply the same
change/comment pattern to SecretKeySelector's hook where launchModal is
currently included.
In `@frontend/public/components/modals/configure-cluster-upstream-modal.tsx`:
- Around line 169-178: The provider currently spreads {...props} after passing
cancel/close to ConfigureClusterUpstreamModal so caller-supplied cancel/close
can override the provider's closeOverlay; update
ConfigureClusterUpstreamModalProvider to pass {...props} first and then
explicitly set cancel={props.closeOverlay} and close={props.closeOverlay} (or
otherwise ensure the provider's closeOverlay wins) so the ModalWrapper/provider
always controls closing; refer to ConfigureClusterUpstreamModalProvider,
ConfigureClusterUpstreamModal, props, and closeOverlay when making the change.
In `@frontend/public/components/modals/index.ts`:
- Around line 97-102: The lazy import for LazyClonePVCModalProvider should use
the module default export to match the pattern used by
LazyExpandPVCModalProvider and LazyConfigureClusterUpstreamModalProvider; update
the .then handler to return { default: m.default } (or simply use m.default)
instead of { default: m.ClonePVCModalProvider } so it consistently consumes the
component via the module's default export (the component is exported both named
and default as ClonePVCModalProvider).
🧹 Nitpick comments (6)
frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/OverlayProvider.tsx (1)
55-57: Suspense addition is appropriate for lazy-loaded modal providers.The
fallback={null}is the right choice here—you don't want a loading spinner flashing before the modal materializes. This cleanly supports the lazy provider pattern (React.lazy()) being rolled out across the modal migrations.One consideration: if a lazy chunk fails to load (network hiccup, 404'd chunk after a deployment), the error will propagate upward with no local catch. You might want to wrap this in an
ErrorBoundaryto gracefully handle chunk loading failures without potentially unmounting the entire overlay context tree.💡 Optional: Add ErrorBoundary for chunk loading resilience
+import ErrorBoundary from '@console/shared/src/components/error/ErrorBoundary'; +// Or implement a minimal local ErrorBoundary if preferred // Inside the render: {_.map(componentsMap, (c, id) => ( - <Suspense key={id} fallback={null}> - <c.Component {...c.props} closeOverlay={() => closeOverlay(id)} /> - </Suspense> + <ErrorBoundary key={id} FallbackComponent={() => null}> + <Suspense fallback={null}> + <c.Component {...c.props} closeOverlay={() => closeOverlay(id)} /> + </Suspense> + </ErrorBoundary> ))}This ensures a failed lazy import doesn't take down the entire overlay system—graceful degradation for production resilience.
frontend/packages/console-app/src/actions/hooks/useRetryRolloutAction.ts (1)
98-100: Avoid stalelaunchModalby stabilizing it instead of suppressing deps.Suppressing exhaustive-deps can capture a stale overlay context if the provider remounts; use a ref to keep the latest
launchModaland drop the eslint disable.♻️ Proposed refactor
-import { useMemo } from 'react'; +import { useEffect, useMemo, useRef } from 'react'; @@ const { t } = useTranslation(); const launchModal = useOverlay(); + const launchModalRef = useRef(launchModal); + useEffect(() => { + launchModalRef.current = launchModal; + }, [launchModal]); @@ - cta: () => - retryRollout(rcModel, rc).catch((err) => launchModal(ErrorModal, { error: err.message })), + cta: () => + retryRollout(rcModel, rc).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 [t, dcModel, rcModel, rc, canRetry, resource],frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx (1)
279-285: Guard against callers overridingcloseOverlay.
Because...propscomes last, any passedcancel/closewould override the overlay close handler. Prefer spreading first, then explicitly wiringcloseOverlayto enforce correct dismissal.♻️ Suggested tweak
-export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = (props) => { +export const ClonePVCModalProvider: OverlayComponent<ClonePVCModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ClonePVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ClonePVCModal {...rest} cancel={closeOverlay} close={closeOverlay} /> </ModalWrapper> ); };Also applies to: 287-288
frontend/public/components/modals/expand-pvc-modal.tsx (1)
99-105: Provider implementation follows the established pattern.The
ModalWrappercorrectly receivesblockingandonClose. WiringcloseOverlayto bothcancelandcloseensures the modal dismisses properly on either action.One minor note:
{...props}spreadscloseOverlayintoExpandPVCModal, butExpandPVCModaldestructures onlyresource,kind,close, andcancel. The extra prop is harmless but slightly redundant. Consider destructuring explicitly if you want cleaner prop forwarding:♻️ Optional cleanup
-export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = (props) => { +export const ExpandPVCModalProvider: OverlayComponent<ExpandPVCModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> - <ExpandPVCModal cancel={props.closeOverlay} close={props.closeOverlay} {...props} /> + <ModalWrapper blocking onClose={closeOverlay}> + <ExpandPVCModal cancel={closeOverlay} close={closeOverlay} {...rest} /> </ModalWrapper> ); };frontend/packages/console-app/src/actions/hooks/useDeploymentActions.ts (1)
170-178: Note:resourceLimitsModalstill uses legacy pattern.The
EditResourceLimitsaction still invokesresourceLimitsModaldirectly rather than throughlaunchModal. Given the PR title indicates WIP, this is presumably planned for a future commit. Just flagging for tracking purposes.frontend/public/components/modals/configure-update-strategy-modal.tsx (1)
241-253: LGTM - Clean provider implementation following the overlay pattern.The migration correctly wraps
ConfigureUpdateStrategyModalinsideModalWrapperwithblockingbehavior and wirescloseOverlayto bothcancelandclosehandlers. The backward-compatible default export maintains API stability.Minor observation: The spread
{...props}passescloseOverlayto the inner modal which doesn't use it. This is harmless but consider destructuring to keep props clean:✨ Optional cleanup
-export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ( - props, -) => { +export const ConfigureUpdateStrategyModalProvider: OverlayComponent<ConfigureUpdateStrategyModalProps> = ({ + closeOverlay, + ...rest +}) => { return ( - <ModalWrapper blocking onClose={props.closeOverlay}> + <ModalWrapper blocking onClose={closeOverlay}> <ConfigureUpdateStrategyModal - cancel={props.closeOverlay} - close={props.closeOverlay} - {...props} + cancel={closeOverlay} + close={closeOverlay} + {...rest} /> </ModalWrapper> ); };
| import { useMemo } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { Action, K8sModel } from '@console/dynamic-plugin-sdk'; | ||
| import { useOverlay } from '@console/dynamic-plugin-sdk/src/app/modal-support/useOverlay'; | ||
| import { TopologyApplicationObject } from '@console/dynamic-plugin-sdk/src/extensions/topology-types'; | ||
| import { deleteModal } from '@console/internal/components/modals'; | ||
| import { LazyDeleteModalProvider } from '@console/internal/components/modals'; |
There was a problem hiding this comment.
Guard application.resources before forEach.
If application.resources is undefined, the delete action will throw before cleanup. Safer to default to an empty array.
🔧 Proposed fix
- onSubmit: () => {
- application.resources.forEach((resource) => {
- reqs.push(cleanUpWorkload(resource.resource));
- });
- return Promise.all(reqs);
- },
+ onSubmit: () => {
+ (application.resources ?? []).forEach((resource) => {
+ reqs.push(cleanUpWorkload(resource.resource));
+ });
+ return Promise.all(reqs);
+ },Also applies to: 28-37
🤖 Prompt for AI Agents
In `@frontend/packages/dev-console/src/actions/context-menu.ts` around lines 1 -
6, Guard against application.resources being undefined by defaulting to an empty
array before iterating: replace direct calls like
application.resources.forEach(...) with (application.resources ??
[]).forEach(...) (or Array.isArray check) in the delete action and any cleanup
loops that touch application.resources (e.g., the delete modal / cleanup logic
using application.resources), and apply the same change to all other occurrences
in this file where application.resources is iterated.
| export const useDeleteKnativeServiceResource = ( | ||
| kind: K8sKind, | ||
| obj: K8sResourceKind, | ||
| serviceTypeValue: ServiceTypeValue, | ||
| serviceCreatedFromWebFlag: boolean, | ||
| ): Action => ({ | ||
| id: `delete-resource`, | ||
| label: | ||
| serviceTypeValue === ServiceTypeValue.Function | ||
| ? i18next.t('knative-plugin~Delete Function') | ||
| : i18next.t('knative-plugin~Delete Service'), | ||
| cta: () => | ||
| deleteModal( | ||
| serviceCreatedFromWebFlag | ||
| ? { | ||
| kind, | ||
| resource: obj, | ||
| deleteAllResources: () => cleanUpWorkload(obj), | ||
| } | ||
| : { | ||
| kind, | ||
| resource: obj, | ||
| }, | ||
| ), | ||
| accessReview: asAccessReview(kind, obj, 'delete'), | ||
| }); | ||
| ): Action => { | ||
| const launchModal = useOverlay(); | ||
|
|
||
| return useMemo<Action>( | ||
| () => ({ | ||
| id: `delete-resource`, | ||
| label: | ||
| serviceTypeValue === ServiceTypeValue.Function | ||
| ? i18next.t('knative-plugin~Delete Function') | ||
| : i18next.t('knative-plugin~Delete Service'), | ||
| cta: () => | ||
| launchModal( | ||
| LazyDeleteModalProvider, | ||
| serviceCreatedFromWebFlag | ||
| ? { | ||
| kind, | ||
| resource: obj, | ||
| deleteAllResources: () => cleanUpWorkload(obj), | ||
| } | ||
| : { | ||
| kind, | ||
| resource: obj, | ||
| }, | ||
| ), | ||
| accessReview: asAccessReview(kind, obj, 'delete'), | ||
| }), | ||
| [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag, launchModal], | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/packages/knative-plugin/src/actions/creators.ts | sed -n '120,153p'Repository: openshift/console
Length of output: 1267
🏁 Script executed:
# Search for launchModal usage with eslint-disable across action hooks
fd -e ts -e tsx frontend/packages/console-app/src/actions | head -20Repository: openshift/console
Length of output: 530
🏁 Script executed:
# Find all instances of launchModal with eslint-disable comments
rg -n 'launchModal' -B2 -A2 --type ts -g '!*.test.ts' | grep -A2 -B2 'eslint-disable'Repository: openshift/console
Length of output: 4983
🏁 Script executed:
# Locate and examine the useOverlay hook implementation
find . -name '*useOverlay*' -o -name '*overlay*' -type f | grep -E '\.(ts|tsx)$' | head -10Repository: openshift/console
Length of output: 143
🏁 Script executed:
# Check for launchModal in providers.ts
rg -n 'launchModal' frontend/packages/knative-plugin/src/providers.ts -B3 -A3Repository: openshift/console
Length of output: 150
🏁 Script executed:
cat frontend/packages/console-dynamic-plugin-sdk/src/app/modal-support/useOverlay.tsRepository: openshift/console
Length of output: 2059
🏁 Script executed:
# Find knative providers file
fd 'providers' frontend/packages/knative-plugin/src/ -type fRepository: openshift/console
Length of output: 231
🏁 Script executed:
# Search for launchModal pattern specifically in knative-plugin
rg -n 'launchModal' frontend/packages/knative-plugin/src/ -B2 -A2Repository: openshift/console
Length of output: 2841
Exclude launchModal from the dependency array to align with established codebase patterns.
The useOverlay hook returns a context value that is recreated on each render, and including it in the dependency array causes useMemo to unnecessarily recreate the action object on every re-render. This cascades into parent components using useMemo (like those in providers.ts), triggering re-render loops.
The codebase has an established pattern across multiple action hooks in console-app that explicitly excludes launchModal with the comment: "missing launchModal dependency, that causes max depth exceeded error." Apply the same pattern here:
}),
- [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag, launchModal],
+ // missing launchModal dependency, that causes max depth exceeded error
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag],
);
};This also applies to SecretKeySelector.tsx (line 61), which includes launchModal in its dependency array.
📝 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 useDeleteKnativeServiceResource = ( | |
| kind: K8sKind, | |
| obj: K8sResourceKind, | |
| serviceTypeValue: ServiceTypeValue, | |
| serviceCreatedFromWebFlag: boolean, | |
| ): Action => ({ | |
| id: `delete-resource`, | |
| label: | |
| serviceTypeValue === ServiceTypeValue.Function | |
| ? i18next.t('knative-plugin~Delete Function') | |
| : i18next.t('knative-plugin~Delete Service'), | |
| cta: () => | |
| deleteModal( | |
| serviceCreatedFromWebFlag | |
| ? { | |
| kind, | |
| resource: obj, | |
| deleteAllResources: () => cleanUpWorkload(obj), | |
| } | |
| : { | |
| kind, | |
| resource: obj, | |
| }, | |
| ), | |
| accessReview: asAccessReview(kind, obj, 'delete'), | |
| }); | |
| ): Action => { | |
| const launchModal = useOverlay(); | |
| return useMemo<Action>( | |
| () => ({ | |
| id: `delete-resource`, | |
| label: | |
| serviceTypeValue === ServiceTypeValue.Function | |
| ? i18next.t('knative-plugin~Delete Function') | |
| : i18next.t('knative-plugin~Delete Service'), | |
| cta: () => | |
| launchModal( | |
| LazyDeleteModalProvider, | |
| serviceCreatedFromWebFlag | |
| ? { | |
| kind, | |
| resource: obj, | |
| deleteAllResources: () => cleanUpWorkload(obj), | |
| } | |
| : { | |
| kind, | |
| resource: obj, | |
| }, | |
| ), | |
| accessReview: asAccessReview(kind, obj, 'delete'), | |
| }), | |
| [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag, launchModal], | |
| ); | |
| }; | |
| export const useDeleteKnativeServiceResource = ( | |
| kind: K8sKind, | |
| obj: K8sResourceKind, | |
| serviceTypeValue: ServiceTypeValue, | |
| serviceCreatedFromWebFlag: boolean, | |
| ): Action => { | |
| const launchModal = useOverlay(); | |
| return useMemo<Action>( | |
| () => ({ | |
| id: `delete-resource`, | |
| label: | |
| serviceTypeValue === ServiceTypeValue.Function | |
| ? i18next.t('knative-plugin~Delete Function') | |
| : i18next.t('knative-plugin~Delete Service'), | |
| cta: () => | |
| launchModal( | |
| LazyDeleteModalProvider, | |
| serviceCreatedFromWebFlag | |
| ? { | |
| kind, | |
| resource: obj, | |
| deleteAllResources: () => cleanUpWorkload(obj), | |
| } | |
| : { | |
| kind, | |
| resource: obj, | |
| }, | |
| ), | |
| accessReview: asAccessReview(kind, obj, 'delete'), | |
| }), | |
| // missing launchModal dependency, that causes max depth exceeded error | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| [kind, obj, serviceTypeValue, serviceCreatedFromWebFlag], | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In `@frontend/packages/knative-plugin/src/actions/creators.ts` around lines 120 -
153, The useMemo in useDeleteKnativeServiceResource is including the launchModal
value from useOverlay in its dependency array which forces the memo to recreate
on every render; remove launchModal from the dependencies and leave [kind, obj,
serviceTypeValue, serviceCreatedFromWebFlag] instead, and add the same
explanatory comment used elsewhere: "missing launchModal dependency, that causes
max depth exceeded error."; apply the same change/comment pattern to
SecretKeySelector's hook where launchModal is currently included.
| export const ConfigureClusterUpstreamModalProvider: OverlayComponent<ConfigureClusterUpstreamModalProps> = ( | ||
| props, | ||
| ) => { | ||
| return ( | ||
| <ModalWrapper blocking onClose={props.closeOverlay}> | ||
| <ConfigureClusterUpstreamModal | ||
| cancel={props.closeOverlay} | ||
| close={props.closeOverlay} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
Avoid overriding closeOverlay via prop spread.
{...props} comes after cancel/close, so any caller-supplied cancel/close would override closeOverlay and potentially prevent the overlay from closing. Flip the order so the provider always controls closing.
🔧 Proposed fix
<ConfigureClusterUpstreamModal
- cancel={props.closeOverlay}
- close={props.closeOverlay}
- {...props}
+ {...props}
+ cancel={props.closeOverlay}
+ close={props.closeOverlay}
/>📝 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 ConfigureClusterUpstreamModalProvider: OverlayComponent<ConfigureClusterUpstreamModalProps> = ( | |
| props, | |
| ) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ConfigureClusterUpstreamModal | |
| cancel={props.closeOverlay} | |
| close={props.closeOverlay} | |
| {...props} | |
| /> | |
| export const ConfigureClusterUpstreamModalProvider: OverlayComponent<ConfigureClusterUpstreamModalProps> = ( | |
| props, | |
| ) => { | |
| return ( | |
| <ModalWrapper blocking onClose={props.closeOverlay}> | |
| <ConfigureClusterUpstreamModal | |
| {...props} | |
| cancel={props.closeOverlay} | |
| close={props.closeOverlay} | |
| /> |
🤖 Prompt for AI Agents
In `@frontend/public/components/modals/configure-cluster-upstream-modal.tsx`
around lines 169 - 178, The provider currently spreads {...props} after passing
cancel/close to ConfigureClusterUpstreamModal so caller-supplied cancel/close
can override the provider's closeOverlay; update
ConfigureClusterUpstreamModalProvider to pass {...props} first and then
explicitly set cancel={props.closeOverlay} and close={props.closeOverlay} (or
otherwise ensure the provider's closeOverlay wins) so the ModalWrapper/provider
always controls closing; refer to ConfigureClusterUpstreamModalProvider,
ConfigureClusterUpstreamModal, props, and closeOverlay when making the change.
| // Lazy-loaded OverlayComponent for Clone PVC Modal | ||
| export const LazyClonePVCModalProvider = lazy(() => | ||
| import( | ||
| '@console/app/src/components/modals/clone/clone-pvc-modal' /* webpackChunkName: "clone-pvc-modal" */ | ||
| ).then((m) => m.default(props)); | ||
| ).then((m) => ({ default: m.ClonePVCModalProvider })), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the clone-pvc-modal file
fd -e tsx -e ts "clone-pvc-modal" --type fRepository: openshift/console
Length of output: 138
🏁 Script executed:
# Inspect the clone-pvc-modal.tsx file to check export pattern
cat -n "frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx" | head -50Repository: openshift/console
Length of output: 2515
🏁 Script executed:
# Check file length and view the export statements at the end
wc -l "frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx"
tail -30 "frontend/packages/console-app/src/components/modals/clone/clone-pvc-modal.tsx"Repository: openshift/console
Length of output: 942
🏁 Script executed:
# Verify the pattern used by other modals in the index.ts file
grep -A 3 ".then((m)" "frontend/public/components/modals/index.ts" | head -40Repository: openshift/console
Length of output: 1042
🏁 Script executed:
# View the specific lines around LazyClonePVCModalProvider to understand the pattern
sed -n '90,110p' "frontend/public/components/modals/index.ts"Repository: openshift/console
Length of output: 867
Use m.default to align with other OverlayComponent lazy-loaded modals.
Line 102 uses m.ClonePVCModalProvider (named export) while LazyExpandPVCModalProvider (line 93) and LazyConfigureClusterUpstreamModalProvider (line 105) both use m.default. Since clone-pvc-modal.tsx exports ClonePVCModalProvider as both a named and default export, change this to m.default for consistency:
Diff
export const LazyClonePVCModalProvider = lazy(() =>
import(
'@console/app/src/components/modals/clone/clone-pvc-modal' /* webpackChunkName: "clone-pvc-modal" */
- ).then((m) => ({ default: m.ClonePVCModalProvider })),
+ ).then((m) => ({ default: m.default })),
);🤖 Prompt for AI Agents
In `@frontend/public/components/modals/index.ts` around lines 97 - 102, The lazy
import for LazyClonePVCModalProvider should use the module default export to
match the pattern used by LazyExpandPVCModalProvider and
LazyConfigureClusterUpstreamModalProvider; update the .then handler to return {
default: m.default } (or simply use m.default) instead of { default:
m.ClonePVCModalProvider } so it consistently consumes the component via the
module's default export (the component is exported both named and default as
ClonePVCModalProvider).
0ec415a to
08d46e0
Compare
openshift-merge-robot
added
the
needs-rebase
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>
openshift-ci
bot
added
component/shared
openshift-ci
bot
added
the
kind/cypress
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
08d46e0 to
6911c9a
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 |
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>
This commit refactors configure-update-strategy-modal to use the OverlayComponent pattern, eliminating createModalLauncher. Changes: - Refactored configure-update-strategy-modal.tsx to use OverlayComponent - Added ConfigureUpdateStrategyModalProvider with ModalWrapper - Added LazyConfigureUpdateStrategyModalProvider with React.lazy() - Updated useDeploymentActions to use the new lazy provider - Removed deprecated configureUpdateStrategyModal from index.ts This change maintains backward compatibility through default exports and preserves lazy loading functionality. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit migrates the configure namespace pull secret modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern. Changes: - Exported ConfigureNamespacePullSecretModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified namespace.jsx to use useOverlay hook instead of direct modal call Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tern This commit migrates the configure cluster upstream modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern and fixes react-modal accessibility warnings. Changes: - Exported ConfigureClusterUpstreamModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified details-page.tsx to use useOverlay hook instead of direct modal call - Removed unnecessary TFunction prop from modal type definition - Fixed react-modal warnings by using useLayoutEffect for global app element setup - Added explicit appElement prop to ModalWrapper for robust timing handling Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit migrates the managed resource save modal from the legacy createModalLauncher pattern to the new OverlayComponent pattern. Changes: - Exported ManagedResourceSaveModalProvider as OverlayComponent - Updated index.ts to lazy-load the modal provider - Modified edit-yaml.tsx to use useOverlay hook instead of direct modal call - Removed unnecessary 'kind' prop from modal invocation - Updated type definition to extend ModalComponentProps Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The remove-user-modal was already replaced by useWarningModal in group.tsx and is no longer used anywhere in the codebase. Changes: - Deleted remove-user-modal.tsx - Removed LazyRemoveUserModalProvider export from index.ts Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5012-2
branch
from
6911c9a to
d088693
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. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@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
Continues the migration of modals from the legacy
createModalLauncherpattern to the newOverlayComponentpattern using theuseOverlayhook. This PR migrates 4 additional modals and removes 1 orphaned modal.Note: This PR builds on #15932.
Changes
Modals Migrated (4 total)
All modals now follow the
OverlayComponentpattern with consistent implementation:ConfigureUpdateStrategyModal (
public/components/modals/configure-update-strategy-modal.tsx)ConfigureUpdateStrategyModalProvider: OverlayComponentLazyConfigureUpdateStrategyModalProviderwith lazy loadinguseDeploymentActionshookConfigureNamespacePullSecretModal (
public/components/modals/configure-ns-pull-secret-modal.tsx)ConfigureNamespacePullSecretModalProvider: OverlayComponentnamespace.jsxto useuseOverlay()hookConfigureClusterUpstreamModal (
public/components/modals/configure-cluster-upstream-modal.tsx)ConfigureClusterUpstreamModalProvider: OverlayComponentdetails-page.tsxto useuseOverlay()hookManagedResourceSaveModal (
public/components/modals/managed-resource-save-modal.tsx)ManagedResourceSaveModalProvider: OverlayComponentedit-yaml.tsxto useuseOverlay()hookCleanup
public/components/modals/remove-user-modal.tsx)useWarningModalingroup.tsxLazyRemoveUserModalProviderexport from index.tsPattern Example
Benefits
Testing
Related
🤖 Generated with Claude Code