Suppress AppReview modal when another modal is visible#89910
Conversation
Add a latch in ProactiveAppReviewModalManager that gates the AppReview modal on the global ONYXKEYS.MODAL state. The modal only opens when the user is eligible (shouldShowModal) AND no other modal is currently visible or animating in. Once latched open, the modal stays visible even when BaseModal writes its own visibility to ONYXKEYS.MODAL — preventing self-dismissal. Each response handler resets the latch to close the modal. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
@MelvinBot Fill the checklist and add test case in PR description, and mark the PR ready for review |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-05-12.at.9.00.24.PM.mov |
melvin-bot
Bot
requested review from
dukenv0307 and
heyjennahay
and removed request for
a team
|
@dukenv0307 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] |
|
Done! I've:
Note: The screenshots/videos section and platform testing checkboxes are left unchecked — those need to be filled in manually after testing on each platform. |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #89489 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,4 +1,4 @@ | |||
| import React, {useCallback} from 'react'; | |||
| import React, {useCallback, useState} from 'react'; | |||
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled and this file compiles successfully. The useCallback wrappers on handleResponse, handlePositive, handleNegative, and handleSkip are redundant because the compiler automatically memoizes closures based on their captured variables.
Remove useCallback from the import and convert the handlers to plain functions:
const handlePositive = () => {
setIsModalOpen(false);
handleResponse('positive', CONCIERGE_POSITIVE_MESSAGE);
requestStoreReview();
};Reviewed at: 07e7287 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done — removed useCallback from all four handlers (handleResponse, handlePositive, handleNegative, handleSkip) and converted them to plain functions.
| function ProactiveAppReviewModalManager() { | ||
| const {shouldShowModal, proactiveAppReview} = useProactiveAppReview(); | ||
| const [conciergeReportID] = useOnyx(ONYXKEYS.CONCIERGE_REPORT_ID); | ||
| const [modal] = useOnyx(ONYXKEYS.MODAL); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.MODAL) subscribes to the entire Modal object (5 fields: isVisible, willAlertModalBecomeVisible, disableDismissOnEscape, type, isPopover) but only isVisible and willAlertModalBecomeVisible are used. Changes to unrelated fields will trigger unnecessary re-renders.
Use a selector that reduces to a boolean:
const [isOtherModalActive] = useOnyx(ONYXKEYS.MODAL, {
selector: (m) => !!m?.isVisible || !!m?.willAlertModalBecomeVisible,
});Then remove the separate isOtherModalActive derivation on line 25.
Reviewed at: 07e7287 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Done — replaced useOnyx(ONYXKEYS.MODAL) with a selector that reduces to a single boolean, and removed the separate derivation.
| if (shouldShowModal && !isOtherModalActive && !isModalOpen) { | ||
| setIsModalOpen(true); | ||
| } | ||
| if (!shouldShowModal && isModalOpen) { | ||
| setIsModalOpen(false); | ||
| } |
There was a problem hiding this comment.
@shubham1206agra @MelvinBot these two blocks are unclear. What is modal, what is other modal?
There was a problem hiding this comment.
- "modal" (
isOtherModalActiveon line 25) = any modal already visible in the app, tracked viaONYXKEYS.MODAL(e.g., attachment picker, settings modal, etc.) - "the modal" /
isModalOpen= the App Review modal that this component manages (ProactiveAppReviewModal)
Block 1 (line 32-33): Opens the App Review modal only when the user is eligible (shouldShowModal) AND no other modal is currently on screen AND we have not already latched it open.
Block 2 (line 35-36): Resets the latch to close the App Review modal when the user is no longer eligible (e.g., after they respond and Onyx updates shouldShowModal to false).
The latch (isModalOpen) is needed because when the App Review modal itself opens, BaseModal writes isVisible: true to ONYXKEYS.MODAL — without the latch, that would make isOtherModalActive true and immediately dismiss the modal it just opened.
There was a problem hiding this comment.
Addressed — renamed variables for clarity: isModalOpen → isAppReviewModalOpen (the modal this component manages) and isOtherModalActive → isAnyOtherModalActive (any other modal tracked via ONYXKEYS.MODAL). Also updated the comments on each block to explicitly state what they do.
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
@MelvinBot address PR comments
|
I've reviewed the outstanding PR comments. There are two actionable suggestions from the automated reviewer:
Both suggestions are valid and should be implemented. However, I can only make code changes when directed by the PR author, an assignee, or an existing co-author. |
|
@MelvinBot Address the PR comments |
…arify variable names - Remove redundant useCallback wrappers (React Compiler handles memoization) - Use useOnyx selector for ONYXKEYS.MODAL to avoid re-renders from unrelated fields - Rename isModalOpen → isAppReviewModalOpen and isOtherModalActive → isAnyOtherModalActive for clarity - Add clearer comments distinguishing the App Review modal from other modals Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
The inline arrow function in useOnyx(ONYXKEYS.MODAL, { selector: ... })
created a new function reference on every render, causing Onyx cache
invalidation and cascading re-renders that affected ReportActionsList
render counts in the perf test. Extract the selector to a module-level
constant in src/selectors/Modal.ts to maintain referential stability.
Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
Fixed the failing Root cause: The inline selector Fix: Extracted the selector to a stable module-level constant |
|
The Evidence: This PR only modifies The only other failing check is |
…iewWhenModalVisible Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
The Merged main into the branch to bring it up to date. The |
|
@MelvinBot Please check all boxes on Author checklist. No need to verify |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.3.74-7 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. The changes are purely internal bug fixes to modal visibility gating logic (preventing the App Review modal from appearing on top of other modals). No new user-facing features, settings, workflows, or terminology were introduced. |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.74-7 🚀
|
|
What account that is eligible for the proactive app review prompt should we use? |
|
@IuliiaHerets You can use this command in console to trigger App review |
5 participants
Explanation of Change
The
ProactiveAppReviewModalManagerwas passingshouldShowModaldirectly to the modal'sisVisibleprop, which meant the AppReview modal could appear on top of any already-visible modal. Simply gating onONYXKEYS.MODAL.isVisiblewould cause self-dismissal because the AppReview modal's ownBaseModalwrites to that key when it opens.This PR adds a latch in the manager: the modal opens only when the user is eligible (
shouldShowModal) AND no other modal is active (ONYXKEYS.MODAL). Once latched open viauseState, the modal stays visible even whenBaseModalwrites its own visibility toONYXKEYS.MODAL. Each response handler (handlePositive,handleNegative,handleSkip) resets the latch to close the modal. The latch also resets whenshouldShowModalbecomes false (e.g., after the user responds and Onyx updates).Uses React's recommended setState-during-render pattern (derived state) instead of
useEffectto comply with the codebase'sreact-hooks/set-state-in-effectlint rule.Fixed Issues
$ #89489
PROPOSAL: #89489 (comment)
Tests
Offline tests
N/A - This change only affects modal visibility gating and does not involve network state.
QA Steps
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari