Refactor ConfirmModal usage to useConfirmModal in workspace accounting and general pages (Part 1)#77919
Refactor ConfirmModal usage to useConfirmModal in workspace accounting and general pages (Part 1)#77919thelullabyy wants to merge 37 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
ZhenjaHorbach and
joekaufmanexpensify
and removed request for
a team
|
@ZhenjaHorbach Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Make it as ready to get reviews from AI reviewer |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Refactor, removing my review |
|
Kindly help on review this PR @ZhenjaHorbach |
|
Sorry for delay |
|
And don't forget to check this comment |
| const canPerformDowngrade = () => canModifyPlan(ownerPolicies, policy); | ||
| const isDowngraded = isCollectPolicy(policy); |
There was a problem hiding this comment.
Why did we remove useMemo?
There was a problem hiding this comment.
I think we are going to remove useMemo, useCallback soon
There was a problem hiding this comment.
| showConfirmModal({ | ||
| title: translate('common.leaveWorkspace'), | ||
| prompt: confirmModalPrompt(), | ||
| confirmText: translate('common.buttonConfirm'), | ||
| shouldShowCancelButton: false, | ||
| success: true, | ||
| }); |
There was a problem hiding this comment.
Is it okay that this modal doesn't have any event after pressing submit?
There was a problem hiding this comment.
I believe it is fine, we also don't have any action in original code
There was a problem hiding this comment.
Looks weird 😅
But okay if it was like this before us
| ), | ||
| confirmText: translate('common.proceed'), | ||
| cancelText: translate('common.cancel'), | ||
| }).then((result) => { |
There was a problem hiding this comment.
I think it is fine to keep using .then here as it requires to update core component SelectionList
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-01-12.15.57.26.mov2026-01-12.15.57.59.mov2026-01-12.15.58.28.mov2026-01-12.15.59.00.movAndroid: mWeb Chrome2026-01-12.16.05.09.mov2026-01-12.16.05.30.mov2026-01-12.16.05.54.mov2026-01-12.16.06.28.mov2026-01-12.16.07.26.moviOS: HybridApp2026-01-12.15.37.40.mov2026-01-12.15.39.44.mov2026-01-12.15.40.59.mov2026-01-12.15.41.21.mov2026-01-12.15.42.11.moviOS: mWeb SafariMacOS: Chrome / Safari2026-01-12.14.39.07.mov2026-01-12.14.41.08.mov2026-01-12.14.42.26.mov2026-01-12.14.43.46.mov2026-01-12.14.48.40.mov2026-01-12.14.53.16.mov2026-01-12.14.56.39.mov |
|
@ZhenjaHorbach Kindly check again. Issues were fixed |
|
@ZhenjaHorbach Kindly review again |
I still have this issue |
|
The issue is fixed @ZhenjaHorbach Screen.Recording.2026-03-23.at.20.55.29.mov |
|
Looks like the current behaviour is not the same as on staging 2026-03-23.19.21.29.movBut agree that it's impossible to fix without modifying the core component I think it's better to revert changes for this modal and fix it in a separate PR @waterim |
|
@ZhenjaHorbach conflicts are resolved. Besides, I fixed the mismatch behavior in the modal Screen.Recording.2026-03-25.at.00.36.08.mov |
| cancelText: translate('common.cancel'), | ||
| success: false, | ||
| shouldShowCancelButton: false, | ||
| // eslint-disable-next-line react-hooks/refs |
| cancelText: translate('common.cancel'), | ||
| success: false, | ||
| shouldShowCancelButton: false, | ||
| // eslint-disable-next-line react-hooks/refs |
| const {setIsDeletingPaidWorkspace, isLoadingBill}: {setIsDeletingPaidWorkspace: (value: boolean) => void; isLoadingBill: boolean | undefined} = | ||
| usePayAndDowngrade(continueDeleteWorkspace); | ||
|
|
||
| const hideDeleteWorkspaceErrorModal = useCallback(() => { |
There was a problem hiding this comment.
I suppose we don't need memoization here
|
@MelvinBot |
|
|
||
| // Resolves the modal promise without closing the modal | ||
| // Used for async confirmation flows where the modal stays open with loading state | ||
| const resolveModal: ModalContextType['resolveModal'] = (data = {action: 'CONFIRM'}) => { |
There was a problem hiding this comment.
We can use ModalActions.CONFIRM here instead 'CONFIRM'
| } | ||
| }; | ||
|
|
||
| const closeModal: ModalContextType['closeModal'] = (data = {action: 'CLOSE'}) => { |
There was a problem hiding this comment.
But let's update here also
PR ReviewSummary: This PR refactors 5 workspace pages from inline Issues Found1. -const canPerformDowngrade = useMemo(() => canModifyPlan(ownerPolicies, policy), [ownerPolicies, policy]);
+const canPerformDowngrade = () => canModifyPlan(ownerPolicies, policy);This is now a function reference (always truthy), but later used as: if (!canPerformDowngrade) {
return <NotFoundPage />;
}This guard will never trigger — 2.
3.
4. Duplicate error modal logic in WorkspacesListPage.tsx The same "show error modal" block appears in three places — the 5. The 6. Missing
7. In 8. Test failure: The failing CI test is Minor Notes
Next Steps: Reply with |
|
|
||
| const continueActionRef = useRef(continueAction); | ||
| // eslint-disable-next-line react-hooks/refs | ||
| continueActionRef.current = continueAction; |
There was a problem hiding this comment.
Can we add a comment here on why we added this?
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3603178512
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!canPerformDowngrade) { | ||
| return <NotFoundPage />; |
There was a problem hiding this comment.
canPerformDowngrade was changed to a function, but this guard now checks the function object instead of its boolean result. Because function references are always truthy, the NotFound guard is bypassed and non-authorized users can still access downgrade UI/flows (the same pattern is also used in onDowngradeToTeam). This regresses the permission check that previously blocked users who cannot modify the plan.
Useful? React with 👍 / 👎.
| }).then((result) => { | ||
| if (result.action !== ModalActions.CONFIRM) { | ||
| return; | ||
| } | ||
| confirmLeaveAndHideModal(); |
There was a problem hiding this comment.
This confirm handler closes over confirmLeaveAndHideModal from the same render where setPolicyIDToLeave(item.policyID) was just dispatched, so it can run with stale policyToLeave data (often undefined, or a previously selected workspace). In that case pressing confirm does nothing or targets the wrong workspace. The previous modal-state flow re-rendered before confirm, so this is a regression in the leave action path.
Useful? React with 👍 / 👎.
4 participants
Explanation of Change
For the first batch, I refactored these pages:
Fixed Issues
$ #76692
PROPOSAL: #76692 (comment)
Tests
Test 1: WorkspaceDuplicateSelectFeaturesForm
Test 2: WorkspaceMoreFeaturesPage
Test 3: WorkspaceDowngradePage
Test 4: WorkspacesListPage
Test 5: WorkspaceOverviewPage
Offline tests
QA Steps
Please refer to Test part
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2025-12-23.at.11.51.38.mov
Android: mWeb Chrome
web-android.mov
iOS: Native
ios.mov
iOS: mWeb Safari
web-ios.mov
MacOS: Chrome / Safari
WorkspaceDuplicateSelectFeaturesForm.mov
WorkspaceMoreFeaturesPage.mov
WorkspaceDowngradePage.mov
WorkspacesListPage.mov
WorkspaceOverviewPage.mov