CONSOLE-5102: Migrate ExportApplicationModal and rollback-modal to useOverlay#16063
CONSOLE-5102: Migrate ExportApplicationModal and rollback-modal to useOverlay#16063sg00dwin wants to merge 5 commits intoopenshift:mainfrom
Conversation
Assisted by Claude Code
Convert rollback-modal.jsx to TypeScript and replace createModalLauncher with RollbackModalOverlay using ModalWrapper. Update consumer hooks (useReplicaSetActions, useReplicationControllerActions) to use useOverlay with LazyRollbackModalOverlay. Assisted by Claude Code
openshift-ci-robot
added
the
jira/valid-reference
|
@sg00dwin: This pull request references CONSOLE-5102 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sg00dwin: This pull request references CONSOLE-5102 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/core
|
@sg00dwin: This pull request references CONSOLE-5102 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 the modal invocation system across OpenShift Console from direct function-based launchers (via 🚥 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 Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (1)
frontend/packages/dev-console/src/actions/export-application.ts (1)
12-28: Extract shared export-modal launch flow to a single helper.This try/catch launch sequence duplicates the logic in
frontend/packages/topology/src/components/export-app/ExportApplication.tsx(Line 34-Line 46). Consolidating it would reduce drift risk for future changes (props, telemetry, fallback behavior).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/actions/export-application.ts` around lines 12 - 28, The export modal launch try/catch logic is duplicated; extract it into a shared helper (e.g., launchExportModal) that both exportApplicationAction and the ExportApplication React component call: the helper should accept (name, namespace, toast, launchModal) and internally call getExportResource(name, namespace) in a try/catch, then invoke launchModal(LazyExportApplicationModalOverlay, { name, namespace, exportResource?, toast }) passing exportResource only on success; place the helper in a common util/module reachable by frontend/packages/dev-console and frontend/packages/topology and replace the duplicated blocks in exportApplicationAction and ExportApplication.tsx to call this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx`:
- Around line 251-258: The LazyExportApplicationModalOverlay lazy export is
defined inside ExportApplicationModal.tsx which prevents proper code splitting;
move that lazy wrapper into a new module (e.g., ExportApplicationModal.lazy.ts)
that imports React.lazy and dynamically imports './ExportApplicationModal' then
returns { default: m.ExportApplicationModalOverlay }, remove the
LazyExportApplicationModalOverlay export from ExportApplicationModal.tsx, and
update all callers (e.g., imports in ExportApplication.tsx and
export-application.ts) to import LazyExportApplicationModalOverlay from the new
ExportApplicationModal.lazy.ts so ExportApplicationModal.tsx is only loaded when
the modal is actually needed.
---
Nitpick comments:
In `@frontend/packages/dev-console/src/actions/export-application.ts`:
- Around line 12-28: The export modal launch try/catch logic is duplicated;
extract it into a shared helper (e.g., launchExportModal) that both
exportApplicationAction and the ExportApplication React component call: the
helper should accept (name, namespace, toast, launchModal) and internally call
getExportResource(name, namespace) in a try/catch, then invoke
launchModal(LazyExportApplicationModalOverlay, { name, namespace,
exportResource?, toast }) passing exportResource only on success; place the
helper in a common util/module reachable by frontend/packages/dev-console and
frontend/packages/topology and replace the duplicated blocks in
exportApplicationAction and ExportApplication.tsx to call this helper.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
frontend/packages/console-app/src/actions/hooks/useReplicaSetActions.tsfrontend/packages/console-app/src/actions/hooks/useReplicationControllerActions.tsfrontend/packages/dev-console/src/actions/export-application.tsfrontend/packages/dev-console/src/components/add/AddCardItem.tsxfrontend/packages/topology/src/components/export-app/ExportApplication.tsxfrontend/packages/topology/src/components/export-app/ExportApplicationModal.tsxfrontend/public/components/modals/index.tsfrontend/public/components/modals/rollback-modal.tsx
| // Lazy-loaded OverlayComponent for Export Application Modal | ||
| export const LazyExportApplicationModalOverlay = lazy(() => | ||
| import('./ExportApplicationModal' /* webpackChunkName: "export-application-modal" */).then( | ||
| (m) => ({ | ||
| default: m.ExportApplicationModalOverlay, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Verify the export definition and structure
echo "=== Checking LazyExportApplicationModalOverlay export in ExportApplicationModal.tsx ==="
rg -n "export const LazyExportApplicationModalOverlay" frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx
echo ""
echo "=== Checking all exports from ExportApplicationModal.tsx ==="
rg -n "^export" frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx | head -20
echo ""
echo "=== Checking imports of LazyExportApplicationModalOverlay (callers) ==="
rg -n "LazyExportApplicationModalOverlay" --include="*.ts" --include="*.tsx" frontend/packages/topology/ frontend/packages/dev-console/Repository: openshift/console
Length of output: 1124
🏁 Script executed:
#!/bin/bash
# Fix the command with correct rg syntax
echo "=== Searching for imports of LazyExportApplicationModalOverlay ==="
rg -n "LazyExportApplicationModalOverlay" -t ts -t tsx
echo ""
echo "=== Checking how LazyExportApplicationModalOverlay is imported (checking for 'from' statements) ==="
rg -n "from.*ExportApplicationModal.*LazyExportApplicationModalOverlay|import.*LazyExportApplicationModalOverlay" -t ts -t tsx
echo ""
echo "=== Reading ExportApplicationModal.tsx to understand full module structure ==="
wc -l frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsxRepository: openshift/console
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Broader search without type restrictions
echo "=== Searching for any usage of LazyExportApplicationModalOverlay in entire repo ==="
rg "LazyExportApplicationModalOverlay"
echo ""
echo "=== Reading ExportApplicationModal.tsx to understand structure ==="
cat -n frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx | tail -20Repository: openshift/console
Length of output: 1974
🏁 Script executed:
#!/bin/bash
# Check what ExportApplication.tsx imports from ExportApplicationModal.tsx
echo "=== Checking imports in ExportApplication.tsx ==="
rg -A 2 "from.*ExportApplicationModal" frontend/packages/topology/src/components/export-app/ExportApplication.tsx
echo ""
echo "=== Checking imports in export-application.ts ==="
rg -A 2 "from.*ExportApplicationModal" frontend/packages/dev-console/src/actions/export-application.ts
echo ""
echo "=== Checking if any other exports from ExportApplicationModal.tsx are used ==="
rg "ExportApplicationModal\b|ExportApplicationModalProps\b" frontend/packages/topology/src/components/export-app/ExportApplication.tsx frontend/packages/dev-console/src/actions/export-application.tsRepository: openshift/console
Length of output: 953
Move lazy export to separate module for effective code splitting.
LazyExportApplicationModalOverlay is the only export from ExportApplicationModal.tsx that callers import, yet the static import still pulls the entire 258-line module into the parent chunk. The dynamic import inside lazy() is ineffective here—webpack already has access to ExportApplicationModalOverlay through the static import. Separating the lazy export to a dedicated file (e.g., ExportApplicationModal.lazy.ts) would allow ExportApplicationModal.tsx to load only when the modal is actually needed, enabling the intended code split.
Suggested approach
Create ExportApplicationModal.lazy.ts:
import { lazy } from 'react';
export const LazyExportApplicationModalOverlay = lazy(() =>
import('./ExportApplicationModal').then((m) => ({
default: m.ExportApplicationModalOverlay,
})),
);Then update imports in ExportApplication.tsx and export-application.ts to reference the new file instead of ExportApplicationModal.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/topology/src/components/export-app/ExportApplicationModal.tsx`
around lines 251 - 258, The LazyExportApplicationModalOverlay lazy export is
defined inside ExportApplicationModal.tsx which prevents proper code splitting;
move that lazy wrapper into a new module (e.g., ExportApplicationModal.lazy.ts)
that imports React.lazy and dynamically imports './ExportApplicationModal' then
returns { default: m.ExportApplicationModalOverlay }, remove the
LazyExportApplicationModalOverlay export from ExportApplicationModal.tsx, and
update all callers (e.g., imports in ExportApplication.tsx and
export-application.ts) to import LazyExportApplicationModalOverlay from the new
ExportApplicationModal.lazy.ts so ExportApplicationModal.tsx is only loaded when
the modal is actually needed.
There was a problem hiding this comment.
Since we weren't lazy loading before, let's undo that to simplify the changes.
There was a problem hiding this comment.
@rhamilto, understood! Removing the lazy loading will keep this migration focused on the core pattern change from createModalLauncher to useOverlay. You can export just the ExportApplicationModalOverlay component directly and have consumers import and use it synchronously—no need for the LazyExportApplicationModalOverlay wrapper.
This simplifies the PR and makes the migration more straightforward to review. Lazy loading optimizations can always be added as a separate follow-up if needed.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
| // Lazy-loaded OverlayComponent for Export Application Modal | ||
| export const LazyExportApplicationModalOverlay = lazy(() => | ||
| import('./ExportApplicationModal' /* webpackChunkName: "export-application-modal" */).then( | ||
| (m) => ({ | ||
| default: m.ExportApplicationModalOverlay, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Since we weren't lazy loading before, let's undo that to simplify the changes.
|
tide/merge-method-squash |
| type ExportApplicationActionType = { | ||
| namespace: string; | ||
| toast: ToastContextType; | ||
| launchModal: (component: React.ComponentType<any>, props: Record<string, any>) => void; |
There was a problem hiding this comment.
| launchModal: (component: React.ComponentType<any>, props: Record<string, any>) => void; | |
| launchModal: LaunchOverlay; |
There was a problem hiding this comment.
Whoops. Had the wrong suggestion. Fixed.
| }: ExportApplicationActionType) => { | ||
| const name = EXPORT_CR_NAME; | ||
| return handleExportApplication(name, namespace, toast); | ||
| try { |
There was a problem hiding this comment.
You're duplicating code by removing handleExportApplication. Is there a reason for that?
There was a problem hiding this comment.
No good reason. I've restored handleExportApplication as a shared helper (now accepting launchModal as a parameter instead of calling createModalLauncher internally), and updated both consumers to call it.
…cation Refactor handleExportApplication to accept launchModal as a parameter instead of calling createModalLauncher internally. Both ExportApplication and exportApplicationAction now call the shared helper rather than duplicating the try/catch + getExportResource + launchModal logic. Assisted by Claude
| toast, | ||
| }); | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
We should keep the console.warn?
rhamilto
added
the
tide/merge-method-squash
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, sg00dwin 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 |
openshift-ci
bot
added
the
approved
|
/retest |
|
/retest |
|
@sg00dwin: The following test 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. |
3 participants
Summary
Migrates the remaining modals from the legacy
createModalLauncherpattern to the modernOverlayComponentpattern. All modals follow the*ModalOverlaynaming convention to clearly distinguish from the deprecatedModalProviderpattern.Changes
Modals Migrated (2 total)
ExportApplicationModal (
packages/topology/src/components/export-app/ExportApplicationModal.tsx)ExportApplicationModalOverlay: OverlayComponentLazyExportApplicationModalOverlaycreateModalLauncherusage andhandleExportApplicationhelperlaunchModalthroughAddCardItemcallback props to support plugin$codeRefcode pathRollbackModal (
public/components/modals/rollback-modal.tsx).jsxto.tsxwith proper TypeScript typesRollbackModalOverlay: OverlayComponentLazyRollbackModalOverlaycreateModalLauncherusageUpdated Consumers
ExportApplication.tsx— UsesuseOverlay()withLazyExportApplicationModalOverlayexport-application.ts— ReceiveslaunchModalvia callback props fromAddCardItemAddCardItem.tsx— AddeduseOverlay()and passeslaunchModalin callback props alongsidenamespaceandtoastuseReplicaSetActions.ts— UsesuseOverlay()withLazyRollbackModalOverlayuseReplicationControllerActions.ts— UsesuseOverlay()withLazyRollbackModalOverlaymodals/index.ts— Updated withLazyRollbackModalOverlaylazy exportMigration Pattern
Each migration follows the consistent pattern established in #15932, #15939, and #15940:
OverlayComponentusing the*ModalOverlaynaming conventionModalWrapperand connect to overlay lifecycle (closeOverlay)React.lazy()with webpack chunk names for code splittinguseOverlayhook withlaunchModal(){...props}before explicitcancelandclosepropslaunchModalto all hook dependency arraysNaming Convention
All modal overlay components and exports follow the
*ModalOverlaynaming pattern:ExportApplicationModalOverlay,RollbackModalOverlayLazyExportApplicationModalOverlay,LazyRollbackModalOverlayTest Plan
yarn lintpassesyarn testpassesRelated
Assisted by Claude Code
Summary by CodeRabbit