Fix wallet statement PDF download on hybrid app#83234
Conversation
Add encrypted auth token and email parameter to the secure PDF URL when downloading wallet statements, matching the existing pattern used by downloadReportPDF. Without these, the native download manager on hybrid apps can't authenticate with the secure endpoint, causing it to return an HTML error page instead of a PDF. Co-authored-by: Benjamin Limpich <blimpich@users.noreply.github.com>
Replace namespace import `* as Browser` with named import
`{isMobileSafari}` to satisfy ESLint no-restricted-syntax rule,
and reorder imports to match Prettier configuration.
Co-authored-by: Benjamin Limpich <blimpich@users.noreply.github.com>
|
Fixed the failing checks:
|
|
The failing checks have already been fixed. The initial commit had two issues in
These were resolved in commit |
melvin-bot
bot
requested review from
JmillsExpensify and
linhvovan29546
and removed request for
a team
|
@linhvovan29546 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] |
|
product review not needed |
![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: e1682eabdc
ℹ️ 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 (walletStatement?.[yearMonth] && currentUserLogin) { | ||
| // We already have a file URL for this statement, so we can download it immediately |
There was a problem hiding this comment.
When currentUserLogin is temporarily undefined (e.g., during initial Onyx hydration), this guard fails even if walletStatement[yearMonth] already exists, so execution falls through to generateStatementPDF(yearMonth). After that request flips isGenerating back to false, the effect calls processDownload() again and repeats the same path, causing repeated GET_STATEMENT_PDF requests and a stuck download spinner until session email appears. Previously, existing statements could be downloaded immediately without this retry loop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. When currentUserLogin is undefined during Onyx hydration but walletStatement[yearMonth] already exists, the combined guard caused a fallthrough to generateStatementPDF, triggering repeated API requests.
Fixed in 77724d7 by separating the conditions: the outer if now checks only for an existing statement, and a nested guard returns early (resetting isDownloading) when the session email isn't available yet. This prevents the unnecessary regeneration loop while still waiting for currentUserLogin to become available before attempting the download.
|
The only failing check is |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Please address this feedback #83234 (comment) |
…able When walletStatement[yearMonth] already exists but currentUserLogin is temporarily undefined during Onyx hydration, return early instead of falling through to generateStatementPDF. This prevents repeated GET_STATEMENT_PDF requests and a stuck download spinner. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
melvin-bot
bot
requested review from
blimpich
and removed request for
a team
|
The only failing check is |
|
|
||
| setIsDownloading(true); | ||
| if (walletStatement?.[yearMonth]) { | ||
| if (!currentUserLogin) { |
There was a problem hiding this comment.
@MelvinBot Why don’t we return early here?
if (isWalletStatementGenerating|| !currentUserLogin) {
return;
}
There was a problem hiding this comment.
You're right, that's cleaner. Moved the !currentUserLogin check to the top of processDownload as an early return alongside isWalletStatementGenerating in a1eaec8.
|
Good suggestion — but there's a reason we can't do that. If we moved the if (isWalletStatementGenerating || !currentUserLogin) {
return;
}…we'd block the initial The current structure keeps the |
|
The only failing check is PR Reviewer Checklist ( |
|
|
||
| function WalletStatementPage({route}: WalletStatementPageProps) { | ||
| const [walletStatement] = useOnyx(ONYXKEYS.WALLET_STATEMENT); | ||
| const [currentUserLogin] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector}); |
There was a problem hiding this comment.
| const [currentUserLogin] = useOnyx(ONYXKEYS.SESSION, {selector: emailSelector}); | |
| const {login: currentUserLogin} = useCurrentUserPersonalDetails(); |
There was a problem hiding this comment.
Applied in f47b559. Replaced useOnyx(ONYXKEYS.SESSION, {selector: emailSelector}) with useCurrentUserPersonalDetails() and removed the unused emailSelector import.
|
The only failing check is PR Reviewer Checklist ( A reviewer needs to copy and complete the reviewer checklist as a comment on this PR to resolve this check. |
|
@MelvinBot Apply this suggestion #83234 (comment) |
Replace the useOnyx(ONYXKEYS.SESSION, {selector: emailSelector}) call
with useCurrentUserPersonalDetails() hook to get the current user's
login, following the preferred pattern for accessing user details.
Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
@MelvinBot Please update the test and QA steps to the following: |
|
The only failing check is PR Reviewer Checklist ( This check will resolve once a reviewer copies and completes the PR Reviewer Checklist in a comment on this PR. No code fix is needed. |
|
Updated the Tests and QA Steps sections in the PR description with the new steps as requested. |
|
@MelvinBot No offline test is needed, so please remove it. |
Per review feedback, check !currentUserLogin alongside isWalletStatementGenerating at the top of processDownload instead of as a nested guard inside the existing-statement branch. Co-authored-by: Linh Vo <linhvovan29546@users.noreply.github.com>
|
Done — removed the offline test steps from the PR description and replaced with "N/A - No offline-specific behavior is changed in this PR." |
|
The only failing check is PR Reviewer Checklist ( |
|
@MelvinBot please update the test and QA steps for Mobile Safari. On Mobile Safari, tapping the download button opens the PDF in the browser instead of downloading the file. |
|
Updated the PR description with Mobile Safari notes in three places:
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-24.at.10.41.34.movAndroid: mWeb ChromeScreen.Recording.2026-02-24.at.10.54.21.moviOS: HybridAppScreen.Recording.2026-02-24.at.10.33.27.moviOS: mWeb SafariScreen.Recording.2026-02-24.at.10.29.23.movMacOS: Chrome / SafariScreen.Recording.2026-02-24.at.09.53.05.mov |
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
@blimpich With the new change, Mobile Safari now opens the PDF in the browser when taping the download button instead of downloading the file. This matches other Mobile Safari download behavior (see fileDownload with isMobileSafari() flag). What do you think?
|
I think it's fine 👍 |
|
🚧 @blimpich 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! 🧪🧪
|
3 participants
Explanation of Change
On the hybrid app (Android/iOS), downloading a wallet statement PDF from the monthly statement page produces a file that cannot be opened. The secure PDF endpoint requires authentication, but the download URL was being constructed without an encrypted auth token or the user's email. On web, the browser automatically sends session cookies so the request succeeds, but in the hybrid app the native download manager does not share those cookies, causing the backend to return an HTML error/login page instead of a PDF.
This fix adds the encrypted auth token and email parameter to the secure PDF URL, matching the existing pattern used by
downloadReportPDFinsrc/libs/actions/Report/index.ts. It also URL-encodes the filename and downloadName parameters for correctness, and passesBrowser.isMobileSafari()tofileDownloadconsistent with the report PDF download flow.Fixed Issues
$ #80365
Tests
Precondition: Have a new account without a balance that shows in your monthly report or open via this link https://staging.new.expensify.com/statements/202504
Note (Mobile Safari): On Mobile Safari, tapping the download button opens the PDF in the browser instead of downloading the file. This is expected behavior consistent with how report PDF downloads work on Mobile Safari (see
fileDownloadwithisMobileSafari()flag).Offline tests
N/A - No offline-specific behavior is changed in this PR.
QA Steps
Precondition: Have a new account without a balance that shows in your monthly report or open via this link https://staging.new.expensify.com/statements/202504
Note (Mobile Safari): On Mobile Safari, tapping the download button opens the PDF in the browser instead of downloading the file. This is expected behavior consistent with how report PDF downloads work on Mobile Safari (see
fileDownloadwithisMobileSafari()flag).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
N/A - Web downloads work via browser cookies
iOS: Native
iOS: mWeb Safari
On Mobile Safari, tapping the download button opens the PDF in the browser instead of downloading the file. This is expected behavior — Mobile Safari does not support programmatic file downloads, so
fileDownloadopens the PDF URL in a new tab for the user to view/save manually. This is consistent with how report PDF downloads already work.MacOS: Chrome / Safari
N/A - Web downloads work via browser cookies