Refactor: ConfirmModal to useConfirmModal | Batch 2#84405
Refactor: ConfirmModal to useConfirmModal | Batch 2#84405mohammadjafarinejad wants to merge 5 commits intoExpensify:mainfrom
Conversation
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.
|
melvin-bot
bot
requested review from
ZhenjaHorbach
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] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| shouldReverseStackedButtons: true, | ||
| prompt: translate(hasError ? locationErrorMessage : 'receipt.locationAccessMessage'), | ||
| }) | ||
| .then(({action}) => { |
There was a problem hiding this comment.
❌ PERF-15 (docs)
Same concern as the Android variant: this refactoring moves onDeny, resetPermissionFlow, and handlePermissionResult (which calls onGrant/onDeny) from ConfirmModal event handler props into a .then() chain inside useEffect. The suppressed react-hooks/exhaustive-deps hides these as stale dependencies. In the old code, ConfirmModal received fresh callback references on each re-render; now they are frozen at effect-run time. There is also no cleanup to discard the promise chain if the effect re-runs.
Consider adding an ignore flag:
useEffect(() => {
if (!startPermissionFlow) {
return;
}
let ignore = false;
getLocationPermission().then((status) => {
if (ignore) return;
// ...
confirmModal.showConfirmModal({...}).then(({action}) => {
if (ignore) return;
// ... rest of handler
});
});
return () => { ignore = true; };
}, [startPermissionFlow]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: ca52d8c10e
ℹ️ 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 (action !== ModalActions.CONFIRM) { | ||
| onDeny?.(); | ||
| return; |
There was a problem hiding this comment.
This now routes every non-confirm close through onDeny, so backdrop/Escape/back-button dismissals are handled the same as tapping Not now. The prior ConfirmModal path had a dedicated onBackdropPress that only closed/reset the modal, so this changes behavior: dismissing the sheet can now continue the flow without location (e.g., submitting from SubmitDetailsPage) instead of just closing the prompt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@ZhenjaHorbach, I think if we want to consider this, we may need to add BACKDROP to ModalActions or a way to distinguish "user explicitly denied" from "user dismissed the modal". WDYT?
App/src/components/Modal/Global/ModalContext.tsx
Lines 6 to 9 in ae2f783
There was a problem hiding this comment.
We have onBackdropPress for showConfirmModal
Can we use it?
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@mohammadjafarinejad |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
@mohammadjafarinejad |
|
@mohammadjafarinejad |
|
Sorry for the delay, I'll update PRs tomorrow |
|
@ZhenjaHorbach PR is Ready for re-review. |
3 participants
Explanation of Change
Refactor
ConfirmModalusage touseConfirmModalin the following files:Fixed Issues
$ #76696
PROPOSAL:
Tests
1. Delegate "Not Allowed" modal (
DelegateNoAccessModalProvider)Setup: Log in as a delegate account that is limited to "submitter only" access.
Steps:
2. Location Permission modal — Web (index.tsx)
Prerequisite — Reset Chrome location permission & clear the app cache
Scenario A — Permission not yet granted (first-time prompt):
Scenario B — Permission blocked (already denied):
Scenario C — Grant permission externally while the modal is open:
onGrantis called exactly once (no duplicate navigation).3. Location Permission modal — Android (
LocationPermissionModal/index.android.tsx)Scenario A — First-time prompt:
Scenario B — Permission blocked:
4. Supportal no-access modal (
SupportalPermissionDeniedModalProvider)In the console, set the Onyx key to simulate a blocked command:
In the console, verify the Onyx key was cleared:
Steps:
ONYXKEYS.SUPPORTAL_PERMISSION_DENIEDis cleared after dismissal (no modal re-appears on navigation or re-render).Offline tests
Same as tests.
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
Android_LocationPermission_Test1.mp4
Android_LocationPermission_Test2.mp4
Android: mWeb Chrome
AndroidWeb_LocationPermission_Test1.mp4
iOS: Native
_IOS_LocationPermission_Test1.mp4
iOS: mWeb Safari
IOSWeb_LocationPermission_Test1.mp4
MacOS: Chrome / Safari
WEB_.SupportalPermissionDeniedModalProvider.mp4
WEB_DelegateNoAccessModalProvider.mp4
Web_LocationPermission_Test1.mp4
Web_LocationPermission_Test2.mp4
Web_LocationPermission_Test3.mp4