Fix member import crash and missing navigation after confirmation#84799
Fix member import crash and missing navigation after confirmation#84799stitesExpensify merged 5 commits intomainfrom
Conversation
Memoize showModal/closeModal in ModalProvider to prevent infinite re-render loop, add stable modal id for deduplication, and move navigation into closeImportPageAndModal to match other import pages. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
…embersConfirmationPage Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Fixed the failing |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot |
|
PR has been marked as ready for review. 🚀 |
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
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const modalPromisesStack = useRef<Record<string, CloseModalPromiseWithResolvers>>({}); | ||
|
|
||
| const showModal: ModalContextType['showModal'] = ({component, props, id, isCloseable = true}) => { | ||
| const showModal: ModalContextType['showModal'] = useCallback(({component, props, id, isCloseable = true}) => { |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-0 (docs)
React Compiler is enabled in this codebase and automatically memoizes closures based on their captured variables. The useCallback wrapping showModal (line 45), useCallback wrapping closeModal (line 68), and useMemo for contextValue (line 91) are manual memoization that the compiler should handle.
The functions only reference stable values: setModalStack (state setter), modalPromisesStack (ref), modalIDRef (ref), and module-level imports (Log, CONST). With enableTreatRefLikeIdentifiersAsRefs: true in the babel config, the compiler will produce stable references for these functions automatically.
Remove useCallback from both showModal and closeModal, remove the useMemo wrapper from contextValue, and pass {{showModal, closeModal}} directly to the Provider. If the compiler cannot handle this file for some reason, add a // eslint-disable-next-line react-compiler/react-compiler comment explaining why manual memoization is needed.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This proposal contradicts the fix that fixes the crash because of causing new function references on every re-render without memoization
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-03-11.13.53.19.movAndroid: mWeb Chrome2026-03-11.13.51.58.moviOS: HybridApp2026-03-11.13.53.19.moviOS: mWeb Safari2026-03-11.13.51.58.movMacOS: Chrome / Safari2026-03-11.13.48.56.mov |
|
@MelvinBot |
…rtedMembersPage When importing members with missing roles, the confirmation page now navigates back when the import succeeds, letting ImportedMembersPage's existing ImportSpreadsheetConfirmModal (which has an isFocused guard) display the success modal. This matches the reviewer's expectation that the confirm details screen closes while the modal remains visible. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Good point! I've updated the confirmation page so that when the import succeeds ( Changes in 6d98c1c:
|
|
@stitesExpensify 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] |
|
@MelvinBot But we need to fix a bug where, after successfully adding members, the confirm details screen resets the added users 
|
Reverts the approach of delegating the success modal to ImportedMembersPage. Instead, keeps ImportSpreadsheetConfirmModal on ImportedMembersConfirmationPage for consistency with other import screens (categories, tags, per diem). Fixes: - Navigation after "Got it": moves Navigation.goBack() into closeImportPageAndModal (the onModalHide callback is overridden by ConfirmModalWrapper so it never fires). - Modal reopening: calls closeImportPage() in closeImportPageAndModal to immediately clear shouldFinalModalBeOpened before navigating, preventing ImportedMembersPage's modal from firing during the transition. - Data reset behind modal: freezes the displayed member list once import starts using a ref, so the UI doesn't flash empty data when the policy updates with the newly imported members. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Reverted the last commit and fixed the bugs. Here's what changed in 5efabaa: Reverted: The Fixed — Navigation after "Got it": Moved Fixed — Modal reopening: Fixed — Data reset behind modal: When the import API succeeds, the policy's employee list updates, causing |
|
@MelvinBot 
|
|
The failing check is Analysis: The PR changes Evidence: The same
This is a pre-existing flaky test, not caused by this PR's changes. Re-running the test suite should resolve it. |
Add shouldShowConfirmModal and isFocused guards to isVisible prop, matching the pattern used in ImportedMembersPage. Without these synchronous guards, the async Onyx update to shouldFinalModalBeOpened creates a window where the useEffect re-fires and reopens the modal. Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
Fixed in 5ee4e23. The modal was reopening because Root cause: When the user presses "Got it", Fix: Added |
|
LGTM! |
stitesExpensify
left a comment
stitesExpensify
left a comment
There was a problem hiding this comment.
Looking good, one question about the GH action bot post
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @stitesExpensify 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/stitesExpensify in version: 9.3.36-0 🚀
|
This was not true. ModalProvider was compiled with React Compiler, so wrapping these functions with manual memoization was redundant and unnecessary. |
Strange @MelvinBot |
|
Created follow-up draft PR to remove the redundant manual memoization from ModalProvider: #85035 Since Please mark it as "Ready for review" when it is ready for review. |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.36-10 🚀
|
5 participants
Explanation of Change
This PR fixes two issues with the workspace members import from spreadsheet flow:
1. App crash (React error #185 — Maximum update depth exceeded):
showModalandcloseModalinModalProviderwere not memoized, causing new function references on every re-render. SinceshowConfirmModal(which derives fromshowModal) was in theuseEffectdependency array ofImportSpreadsheetConfirmModal, each call toshowConfirmModaltriggered a state update → re-render → new function reference → effect re-fires → infinite loop. Fix: wrap both functions withuseCallbackand memoize the context value withuseMemo. Additionally, pass a stableidtoshowConfirmModalso the existing deduplication logic inModalProviderprevents duplicate modals as a defense-in-depth measure.2. No navigation after pressing "Got it" on success modal:
ImportedMembersPageandImportedMembersConfirmationPageplaced navigation logic in theonModalHidecallback, butConfirmModalWrapperoverridesonModalHidewith its own handler, silently dropping the navigation callback. This left users stuck on the import page after a successful import. Fix: moveNavigation.goBack()directly intocloseImportPageAndModal, matching the pattern used by all other import pages (categories, tags, per diem).Fixed Issues
$ #84489
PROPOSAL: #84489 (comment)
Tests
Offline tests
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
Web: Chrome
Import spreadsheet screen:
Column mapping screen:
Import success modal (no crash):
Members list after clicking "Got it" (navigation works):
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari