[WIP] feat: Integrate ExportDownloadStatusModal into report-level CSV export flow#93796
[WIP] feat: Integrate ExportDownloadStatusModal into report-level CSV export flow#93796truph01 wants to merge 4 commits into
Conversation
melvin-bot
Bot
requested review from
daledah and
joekaufmanexpensify
and removed request for
a team
melvin-bot
Bot
requested review from
a team and
mollfpr
and removed request for
a team
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /> | ||
| ) : null; | ||
|
|
||
| const exportDownloadStatusModal = !!activeExportID && ( |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This exportDownloadStatusModal block is duplicated verbatim in MoneyReportHeaderSecondaryActions.tsx. Both consumers build the exact same <ExportDownloadStatusModal> element from the same useExportActions return values (activeExportID, handleExportModalClose) with identical props, including the hardcoded failedBody={translate('exportDownload.csvFailedBody')}. Duplicating this wiring in every consumer raises maintenance overhead and bug risk — a change to the modal's props must be made in multiple places.
Consider encapsulating the modal rendering inside useExportActions (e.g. return a ready-to-render exportDownloadStatusModal node or a small <ExportStatusModal /> wrapper) so each consumer only renders {exportDownloadStatusModal} without re-declaring the element:
// in useExportActions
const exportDownloadStatusModal = !!activeExportID && (
<ExportDownloadStatusModal
exportID={activeExportID}
isVisible
onClose={handleExportModalClose}
failedBody={translate('exportDownload.csvFailedBody')}
/>
);
return {..., exportDownloadStatusModal};Reviewed at: bafa623 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed by creating a shared eportDownloadStatusModal
| }; | ||
|
|
||
| const exportDownloadStatusModal = !!activeExportID && ( | ||
| <ExportDownloadStatusModal |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This exportDownloadStatusModal block is identical to the one added in MoneyReportHeaderSelectionDropdown.tsx. The same <ExportDownloadStatusModal> element is constructed from the same useExportActions values with the same props in both files. This duplicated wiring should be consolidated.
Move the modal construction into useExportActions and return it (or a thin wrapper component), so both consumers just render {exportDownloadStatusModal}:
const {exportActionEntries, exportDownloadStatusModal} = useExportActions({...});
// ...
return (
<>
{exportDownloadStatusModal}
<MoneyReportHeaderKYCDropdown ... />
</>
);Reviewed at: bafa623 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed by creating a shared eportDownloadStatusModal
![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: bafa623212
ℹ️ 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 exportDownloadStatusModal = !!activeExportID && ( | ||
| <ExportDownloadStatusModal | ||
| exportID={activeExportID} | ||
| isVisible | ||
| onClose={handleExportModalClose} | ||
| failedBody={translate('exportDownload.csvFailedBody')} | ||
| /> |
There was a problem hiding this comment.
This modal only covers the header selection dropdown, but on narrow layouts with mobile selection mode enabled MoneyReportHeader returns before rendering MoneyReportHeaderActions (src/components/MoneyReportHeader.tsx:98), so this component is not mounted. The active report selection export control is still MoneyRequestReportView/SelectionToolbar, which calls queueExportSearchWithTemplate without progress tracking and then shows the legacy confirm (src/components/MoneyRequestReportView/SelectionToolbar/index.tsx:94 and :103). As a result, mobile users exporting selected report expenses never send an exportID and never see the new status/download modal; please wire the same tracked modal into that toolbar too.
Useful? React with 👍 / 👎.
truph01
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5eade0f36
ℹ️ 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".
| return; | ||
| } | ||
| if (activeExportID) { | ||
| clearExportDownload(activeExportID, activeExportDownload); |
There was a problem hiding this comment.
When the ready/failed modal is closed with its Close button, ExportDownloadStatusModal.handleClose already calls clearExportDownload(exportID, ...) before invoking this onClose prop. This handler then calls clearExportDownload again, so every newly wired report/selection export close sends two ClearExportDownload writes; keep the parent handler limited to hiding the modal/selection cleanup or otherwise make only one layer own the API clear.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Explanation of Change
ExportDownloadStatusModalinto the report header export menu (useExportActions), replacing the old static "Concierge will send" confirmation so report-level template CSV exports get the same preparing → ready/Concierge → download experience as the Search bulk-export flow.beginExportWithTemplatenow queues with progress tracking (queueExportSearchWithTemplate(..., true)), tracks the exportID, and its two consumers (MoneyReportHeaderSelectionDropdownandMoneyReportHeaderSecondaryActions) render the modal. TheDOWNLOAD_CSVdirect download is left as-is.Fixed Issues
$ #93777
PROPOSAL:
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari