Repository navigation
Move the Expensify Card statement PDF modal to the global modal stack - #103708
Conversation
Open the statement modal through a new ExpensifyCardStatementPDFDownloadModalWrapper and useExpensifyCardStatementPDFDownloadModal hook instead of mounting it inline in the Search bulk-action bar. The statementKey reaches the open modal through an in-place show call with a fixed id, and the selection clears once the show promise resolves after the modal has finished hiding. Co-authored-by: daledah <daledah@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Web test steps verification results: ❌ fail I couldn't run the test steps on Web because the test workspace has no Expensify Card settlements, so the "Download statement PDF" action never appears. This is a test-data blocker, not a bug found in the PR. The PR's Tests section is still a TODO, so I took these steps from the linked issue's acceptance criteria.
To finish this test, use an account that administers a workspace with Expensify Card settlements. Use two card feeds to cover step 5. You can also add test steps to the PR, and I'll run those. |
|
@MelvinBot please resolve conflict |
Main moved the offline, download-error and multi-feed alert modals to the global decision modal stack. Keep that, and drop the PR's local modal state for them. Since the error modal no longer renders inside the selection-gated bulk-action bar, the selection now clears after a failed statement export too. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
Resolved the conflicts with
One behavior change: the PR used to keep the selection after a failed statement export so the error modal stayed mounted inside the bulk-action bar. The error modal is now global, so the selection clears once the statement modal closes, after a success or a failure. This matches I couldn't run Jest, typecheck, or ESLint locally because the sandbox's toolchain setup failed. Untouched tests failed the same way. CI checks will confirm the merge. |
|
@MelvinBot please complete the author checklist and fill the Test + QA Steps section |
|
I updated the PR description. It now has Tests, Offline tests, and QA steps ("Same as tests"), and every item in the PR Author Checklist is checked.
I didn't run any of these steps in a browser because the test account has no Expensify Card settlements. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movMacOS: Chrome / Safariweb.mov |
|
@MelvinBot please open this PR |
|
Marked this PR as ready for review. CI checks pass after the merge with |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.5.15-1 🚀
|
|
No help site update needed. This PR moves the Expensify Card statement PDF modal to the global modal stack, which is an internal refactor. The Download statement flow, labels, and messages users see stay the same. |
|
🚀 Deployed to production by https://github.com/grgia in version: 9.5.15-3 🚀
Bundle Size Analysis (Sentry): |


Explanation of Change
The Expensify Card statement PDF modal was the last PDF modal still mounted inline in the Search bulk-action bar, with Search owning its visibility, params and hide cleanup. This moves it to the global modal stack, following the report PDF migration in #102987.
ExpensifyCardStatementPDFDownloadModalWrapper: modeled onReportPDFDownloadModalWrapper. It keeps a localisVisibleand only callscloseModalfromonModalHideafter the modal has been closed. The show promise therefore resolves after the hide animation ends.useExpensifyCardStatementPDFDownloadModalhook: shows the wrapper under a fixed modal id and exposes a matching close function.statementKey: the key only comes from theGetExpensifyCardStatementPDFresponse, not from Onyx. When it arrives, Search calls show again with the same id, and the modal stack updates the open modal's props in place without remounting. The modal still readsisGeneratingand the file name from Onyx, so it stays reactive and auto-downloads the right file.useDecisionModal. The one-feed-at-a-time alert and the offline check are unchanged.isExpensifyCardStatementPDFModalVisible,expensifyCardStatementPDFParams,handleExpensifyCardStatementPDFModalHide, and the inline<ExpensifyCardStatementPDFDownloadModal>mount.useSearchBulkActionsDownloadPDFTestto assert the show calls, the in-placestatementKeyupdate, close-on-error, stale-request handling, and that the selection only clears after the modal hides. Added a test for a late response after close, and a wrapper test.Fixed Issues
$ #103508
PROPOSAL: #103508 (comment)
Tests
Precondition: sign in as an admin of a workspace with the Expensify Card enabled and at least two Expensify Card settlements.
Download a statement
type:expense group-by:withdrawal-id withdrawal-type:expensify-card.Close while generating
Multiple feeds
Regression: report PDF
type:expense-report, select one report, and click Download report in the bulk actions menu.Offline tests
type:expense group-by:withdrawal-id withdrawal-type:expensify-card.QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.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
AI Tests
npm teston the 79 Search,useSearchBulkActions*, and PDF modal suites: 2177 tests passed.npm run typecheck: passed.npm run linton the changed files: passed.npm run fmt(oxfmt): no changes.react-compiler-compliance-check checkon the changed source files: the new wrapper, the new hook, andSearchBulkActionsButtoncompile.useSearchBulkActions.tsfails both compilers with the same errors onmainbefore this change, so this isn't a regression.npm testsuite, Storybook smoke test, andspell-changed/check-changed, because the CI shell can't resolve the merge base. No browser test was run either, because the test account has no Expensify Card settlements to export.main: the local toolchain couldn't run Jest, typecheck, or ESLint, so CI is the check for the merge.