Repository navigation
[Due for payment 2026-11-05] [$175] Decouple file download modal UI from report and Expensify Card business logic #104440
Description
Context
Follow-up to #103708, which was merged with this cleanup considered non-blocking because ExpensifyCardStatementPDFDownloadModalWrapper followed the existing ReportPDFDownloadModalWrapper pattern. Related report migration: #102987.
Problem
The global modal layer currently contains domain-specific wrappers for reports and Expensify Card statements. Both duplicate visibility/hide-animation lifecycle handling and render feature-specific components, even though they ultimately share PDFDownloadModal.
src/components/Modal should contain business-logic-agnostic UI building blocks, with no knowledge of reports or Expensify Card statements. This is an architectural cleanup, not another migration to the global modal stack.
Proposed cleanup
- Provide a shared, generic
FileDownloadModalin the modal layer, reusing the existing PDF download UI where appropriate. - Keep presentation, generic download states, visibility, and hide-animation lifecycle in the shared UI. Pass status, display text, and callbacks rather than domain identifiers or parameters.
- Move/keep domain-specific behavior behind
useExpensifyCardStatementPDFDownloadModalanduseReportPDFDownloadModal: generation requests, Onyx subscriptions, filename resolution, download actions, error handling, and domain-specific cancellation. - Remove the duplicated domain-specific global wrappers. The modal layer should not import report/card-statement components or access their domain-specific Onyx keys.
Acceptance criteria
- Both hooks consume the same business-logic-agnostic modal UI;
src/components/Modalhas no report/card-statement business logic. - Open modals remain reactive as generation state and filenames arrive, without remounting or reopening a dismissed modal.
- Preserve automatic/manual downloads, loading/success/error states, existing close-on-download behavior, offline behavior, and report stall logging.
- Preserve report cancellation semantics, including submit-via-PDF, and clear Search selection only after the modal finishes hiding.
- Preserve stale-response protection and existing web/native presentation and dismissal behavior.
- Update tests to cover the shared UI lifecycle and both domain-specific hook flows. No intended user-visible behavior change.
Issue Owner
Current Issue Owner: @daledahUpwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~022113954745613435178
- Upwork Job ID: 2113954745613435178
- Last Price Increase: 2026-10-24
Activity
daledah commentedon Oct 21, 2026
@MelvinBot please implement this issue in one PR, following these steps:
Move
src/components/PDFDownloadModal.tsxinto the modal layer assrc/components/Modal/FileDownloadModal.tsx. It only takes presentation props (status,message,onDownload,onClose,onModalHide, button flags). No Onyx, no report or card-statement imports.Add a single
src/components/Modal/Global/FileDownloadModalWrapper.tsxthat ownsisVisibleand callscloseModalonce fromonModalHide. DeleteReportPDFDownloadModalWrapper.tsxandExpensifyCardStatementPDFDownloadModalWrapper.tsx.Move the logic of
ReportPDFDownloadModal.tsxintouseReportPDFDownloadModal: activereportIDin state, Onyx subscriptions,useStallLogger,downloadReportPDF, and the cancel rule (onCancelonly while the filename is still null). Show the wrapper with a fixedidand re-callshowModalwith the sameidwhile open so props update in place; track open state so a late update never reopens a dismissed modal.Same for
ExpensifyCardStatementPDFDownloadModal.tsxintouseExpensifyCardStatementPDFDownloadModal:statementParamsin state,EXPENSIFY_CARD_STATEMENTsubscription,downloadExpensifyCardStatementPDF, keep the fixed id andcloseModalByID. Leave therequestIDguard and "clear selection after the promise" logic inuseSearchBulkActionsuntouched.Switch
MoneyReportHeaderModals.openPDFDownloadtouseReportPDFDownloadModal, then deleteReportPDFDownloadModal.tsxandExpensifyCardStatementPDFDownloadModal.tsx. Nothing undersrc/components/Modal/**should import report or card-statement code or Onyx keys.Tests: add
FileDownloadModalWrapperTest,useReportPDFDownloadModalTestanduseExpensifyCardStatementPDFDownloadModalTestcovering the hide lifecycle, in-place updates, cancel semantics and no-reopen after dismiss. Port and removeReportPDFDownloadModalTestandExpensifyCardStatementPDFDownloadModalWrapperTest, update the mocks inuseSearchBulkActionsDownloadPDFTest.MelvinBot commentedon Oct 21, 2026
🤖 Draft PR created: Decouple the file download modal from report and Expensify Card logic
Please mark it as "Ready for review" or ask me to do that for you when it is ready for review.
All 6 steps are implemented in one PR:
Modal/FileDownloadModal.tsxtakes presentation props only.statusis a newCONST.FILE_DOWNLOAD_STATUS(generating / ready / failed).Modal/Global/FileDownloadModalWrapper.tsxownsisVisibleand callscloseModalonce fromonModalHide. Both domain wrappers are deleted.useReportPDFDownloadModalanduseExpensifyCardStatementPDFDownloadModalown the Onyx and download logic. They re-show the same fixed id to update the open modal in place. An open-state ref is cleared on dismiss, so a late update never reopens the modal.MoneyReportHeaderModals.openPDFDownloadnow uses the report hook.useSearchBulkActionslogic is unchanged.Checks: typecheck, lint, spell check, and the React Compiler check (Babel and OXC) passed. 298 tests across 20 related Jest suites passed, including the 3 new test files.
Not verified in a browser: I tried a web UI check, but the test browser session was blank because its onboarding setup failed. The modal has not been exercised manually.
Out of scope: the existing
Modal/Global/HoldMenuModalWrapper.tsxstill reads report data from Onyx. I left it unchanged.Your fork doesn't give me push access, so the branch is on upstream. The PR has a comment with steps to move it to your fork.
view run · no recording available
[-]Decouple file download modal UI from report and Expensify Card business logic[/-][+][$175] Decouple file download modal UI from report and Expensify Card business logic[/+]melvin-bot commentedon Oct 24, 2026
Job added to Upwork: https://www.upwork.com/jobs/~022113954745613435178
mitarachim commentedon Oct 27, 2026
Deploy Blocker #105206 was identified to be related to the PR #104534 that attempted to fix this issue.
melvin-bot commentedon Oct 27, 2026
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
[-][$175] Decouple file download modal UI from report and Expensify Card business logic[/-][+][Due for payment 2026-11-05] [$175] Decouple file download modal UI from report and Expensify Card business logic[/+]melvin-bot commentedon Oct 29, 2026
@daledah
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.5.24-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2026-11-05. 🎊
The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the Contributor+ Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]. If no checklist is needed for this issue, you can click: [no checklist button]
daledah commentedon Oct 29, 2026
Contributor+ Checklist:
[Contributor] The offending PR and associated issue have been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.
Link to the comment on the PR: N/A, refactor
[Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.
Link to discussion:
[Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
Regression Test Proposal
Precondition:
Test:
Do we agree 👍 or 👎