feat: Enable multi-select Download as PDF#92275
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@truph01 do you think we need to replace the |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
daledah and
trjExpensify
and removed request for
a team
![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: 43857b14fe
ℹ️ 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".
| const failureData: AnyOnyxUpdate[] = [ | ||
| { | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| key: onyxKey, | ||
| value: null, | ||
| }, |
There was a problem hiding this comment.
If ExportReportsToPDF fails after the optimistic PREPARING state is shown, this clears the Onyx entry instead of replacing it with FAILED. ExportDownloadStatusModal keeps rendering the last defined export via usePreviousDefined, so the user remains stuck in the non-dismissible “Preparing download...” state with no failure UI or close path. Please write a failed state here so the modal can recover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated to:
const failureData: AnyOnyxUpdate[] = [
{
onyxMethod: Onyx.METHOD.SET,
key: onyxKey,
value: {state: CONST.EXPORT_DOWNLOAD.STATE.FAILED},
},
];
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| fileDownload(translate, downloadURL); | ||
| }, [isReady, downloadURL, translate, shouldSendFromConcierge]); | ||
| downloadFile(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line react-hooks/exhaustive-deps comment lacks a justification explaining why certain dependencies are intentionally omitted from the dependency array. The downloadFile function is called inside the effect but excluded from the deps -- reviewers and future maintainers need to understand why.
Add a comment explaining the reason, for example:
// We intentionally omit downloadFile from deps to avoid re-triggering the download
// when its closure variables (environment, encryptedAuthToken, etc.) change.
// The download should only fire when the export transitions to ready.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [isReady, fileName, shouldSendFromConcierge]);Reviewed at: 43857b1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Is this ready to be tested on an adhoc build or does it need the backend PR you're alluding to first? |
It needs the backend's PR. cc @mollfpr How's BE PR going? |
|
@codex review |
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Web and Android look good! @trjExpensify all yours to have a look 🙌 |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Core PR for a WN project. 👍
|
@mollfpr All yours! |
|
@truph01 could you resolve the conflict when you available? Thanks! |
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc7cf17f90
ℹ️ 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".
| await exportReportToPDF({reportID: reportIDForPDF}); | ||
| setPdfReportID(reportIDForPDF); | ||
| setIsPdfModalVisible(true); | ||
| const exportID = exportReportsToPDF(selectedReportIDs.map(Number)); |
There was a problem hiding this comment.
For multi-report PDF downloads, converting selectedReportIDs with Number can corrupt real report IDs because they are stored as strings and often exceed JavaScript's safe integer range. For example, a report ID like 8759152536123291182 is rounded to 8759152536123292000 before being JSON-stringified, so the backend is asked to export non-existent or wrong reports whenever selected IDs are larger than Number.MAX_SAFE_INTEGER.
Useful? React with 👍 / 👎.
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Re-request @trjExpensify review, and @daledah, could you update the reviewer checklist? It seems we have updated the reviewer checklist and have a different count. |
|
@mollfpr sure, i'm checking again |
|
@daledah any update on the checklist? |
|
@mollfpr I've updated the checklist and reviewed the code changes again. Everything LGTM. |
|
🚧 @mollfpr 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! 🧪🧪
|
4 participants
Explanation of Change
Fixed Issues
$ #90300
PROPOSAL:
Tests
Click on "Download file" button.
Verify the pdf files are downloaded.
Do step 1 - 4
Click on “Send me the file when it’s ready”
Wait a few minute and click Go to Concierge
Verify the pdf files is send by Concierge
Offline tests
QA Steps
Click on "Download file" button.
Verify the pdf files are downloaded.
Do step 1 - 4
Click on “Send me the file when it’s ready”
Wait a few minute and click Go to Concierge
Verify the pdf files is send by Concierge
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
Screen.Recording.2026-06-17.at.14.15.58.mov
iOS: Native
iOS: mWeb Safari
Screen.Recording.2026-06-17.at.14.13.47.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-17.at.14.08.25.mov