Repository navigation
Replace ProcessMoneyReportHoldMenu with useHoldMenuModal in SelectionToolbar and report preview - #104532
Conversation
… the global modal Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
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.
|
|
🤖 Android test steps verification results: ❌ fail None of the six test cases ran, so this result says nothing about the PR's code. The setup never produced a payable report that still had a held expense, and then the app froze on an unrelated "Create expense" action. A Metro restart did not fix the freeze.
The PR's Tests section is still a placeholder, so I used the acceptance criteria from [Batch 3] Replace ProcessMoneyReportHoldMenu with useHoldMenuModal in SelectionToolbar and report preview (2 modals). To get a real result, add steps to the Tests section that produce a payable report with a held expense. For example, an IOU between two accounts where the payer holds one expense. Then ask me again. |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
|
@MelvinBot |
|
🤖 Web test steps verification results: The hold menu now opens from both the report preview and the selection toolbar, and both pay flows finish. In selection mode, "Pay only" showed $0.00 instead of $10.00, and console errors could not be captured on web.
PR body updated:
|
|
@MelvinBot |
…MoneyReportHoldMenu Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # src/components/MoneyRequestReportView/SelectionToolbar/index.tsx
|
@ZhenjaHorbach 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] |
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. |
|
I merged Conflict detailsThe only conflict was in Checks I ran after the merge:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const formattedAmount = getTotalAmountForIOUReportPreviewButton(iouReport, policy, reportPreviewAction, transactions, convertToDisplayString); | ||
|
|
||
| // Only the pay flow opens this menu; approve surfaces its partial/full choice in the approve dropdown instead. |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This new openHoldMenu helper duplicates logic that now lives in three places: here, in useSelectionModeReportActions.ts, and in MoneyReportHeaderModals.tsx. All three compute hasOnlyHeldExpenses / getNonHeldAndFullAmount, assemble the same showHoldMenu({... nonHeldAmount, fullAmount, hasNonHeldExpenses, transactionCount ...}) payload, and then defer the open by one frame on iOS with the identical if (getPlatform() === CONST.PLATFORM.IOS) { requestAnimationFrame(...) } block. This is a DRY violation that will drift as the hold-menu payload evolves.
Extract the shared payload assembly and the iOS deferral into a single reusable helper (for example a small function or an addition to useHoldMenuModal) and call it from all three sites. At minimum, factor the verbatim iOS deferral block into a shared helper so the requestAnimationFrame timing logic exists in one place:
// e.g. src/libs/runAfterTouchFrame.ts
function openAfterTouchFrame(open: () => void) {
if (getPlatform() === CONST.PLATFORM.IOS) {
requestAnimationFrame(open);
return;
}
open();
}Reviewed at: c8158a6 | 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 in 3265ee6. I added src/libs/HoldMenuUtils.ts with two helpers:
getHoldMenuAmountsbuildsnonHeldAmount,fullAmount,hasNonHeldExpenses, andtransactionCount.openAfterTouchFrameholds the iOS one-frame deferral. It returns a promise, soMoneyReportHeaderModalskeeps itsPromise<void>contract.
All three callers now use them: PayActionButton, useSelectionModeReportActions, and MoneyReportHeaderModals. Each caller passes only what differs: the IDs, the payment fields, and onConfirm. tests/unit/HoldMenuUtilsTest.ts covers both helpers.
| const {nonHeldAmount, fullAmount, hasValidNonHeldAmount} = getNonHeldAndFullAmount(report, shouldShowPayButton, transactions, convertToDisplayString); | ||
| const hasOnlyHeldExpenses = hasOnlyHeldExpensesReportUtils(transactions); | ||
|
|
||
| const openHoldMenu = (requestType: ActionHandledType, paymentType?: PaymentMethodType, methodID?: number, onConfirm?: () => void) => { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This openHoldMenu helper duplicates the one added to PayActionButton.tsx (and the equivalent already in MoneyReportHeaderModals.tsx): the same showHoldMenu({... nonHeldAmount, fullAmount, hasNonHeldExpenses, transactionCount ...}) payload assembly and the same if (getPlatform() === CONST.PLATFORM.IOS) { requestAnimationFrame(() => open()); return; } iOS deferral. Keeping three near-identical copies means any change to the hold-menu contract or the deferral timing must be applied in multiple files.
Consolidate the shared payload building and the iOS deferral into a single reusable helper (or fold it into useHoldMenuModal.showHoldMenu) and have each caller pass only the data that actually differs (e.g. the selection-mode onConfirm that clears the selection).
Reviewed at: c8158a6 | 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 in 3265ee6. This hook now calls the shared getHoldMenuAmounts and openAfterTouchFrame helpers from src/libs/HoldMenuUtils.ts. PayActionButton and MoneyReportHeaderModals call them too. The hook passes only its own data: the VBBA-only methodID and the onConfirm that clears the selection.
| @@ -74,14 +85,45 @@ function PayActionButton() { | |||
|
|
|||
| const formattedAmount = getTotalAmountForIOUReportPreviewButton(iouReport, policy, reportPreviewAction, transactions, convertToDisplayString); | |||
|
|
|||
There was a problem hiding this comment.
❌ CONSISTENCY-15 (docs)
This comment joins two independent sentences with a semicolon, which the plain-comment-style rule calls out. Split it into two sentences:
// Only the pay flow opens this menu. Approve surfaces its partial/full choice in the approve dropdown instead.Reviewed at: c8158a6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@MelvinBot |
Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
I fixed the merge conflict by merging Conflict: Verification
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot |
|
I added your changes in commit 2b1ab3d. The hold menu now waits one frame with Two files no longer matched your diff after the earlier
Verification
|
|
Okay |
|
conflicts |
|
@MelvinBot |
…MoneyReportHoldMenu Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com> # Conflicts: # src/components/ProcessMoneyReportHoldMenu.tsx
|
I fixed the conflicts by merging There was one conflict. Why it was safe to keep the deletionOn This PR's I couldn't run typecheck or Jest in this environment, so CI will be the first check on the merged code. |
|
@roryabraham |
|
@MelvinBot |
…fault-export Co-authored-by: Yauheni Horbach <ZhenjaHorbach@users.noreply.github.com>
|
I fixed the ESLint failure and pushed the change in ab75b25. The only error was
I couldn't run ESLint here, so the new CI run will confirm the fix. The other failing check, |
|
@MelvinBot are the failing reassure tests related? |
|
No, the failing Reassure check looks unrelated to this PR. It's a flaky render-count measurement that is failing the same way on unrelated branches today. EvidenceWhat failed: Why it looks like noise:
Re-running the job should likely clear it. |
|
🚧 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.27-0 🚀
|
|
No help site update needed: this PR swaps the old hold menu component for the shared hold menu modal. The prompts, the options, and the payment flow users see stay the same. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
ProcessMoneyReportHoldMenuwas an older, pay-only copy of the globalHoldMenuModalWrapper. This PR moves its last two callers touseHoldMenuModal().showHoldMenu, as the report header already does, and deletes the old component.useSelectionModeReportActions): bothonHoldMenuOpenhandlers now callshowHoldMenu. They passrequestType,paymentType,methodID(VBBA only), the amounts, andonConfirm.onConfirmnow runs the lifecycle callback that was dropped before (_onConfirm, for example marking the report as paid elsewhere), the same way the header dropdown does, then clears the selection. The local hold-menu state,handleHoldMenuClose, andhandleHoldMenuConfirmare removed, along with the hold-menu block inSelectionToolbar.PayActionButton.confirmPayment): callsshowHoldMenudirectly. It computes the amounts withgetNonHeldAndFullAmount, passes the report IDs (the wrapper reads the reports from Onyx), and passesonConfirm: startAnimation.ReportPreviewHoldMenu, the provider'sholdMenustate,onHoldMenuOpen/onHoldMenuClose, andReportPreviewHoldMenuContextare removed.ActionHandledTypeimports now point at@hooks/useHoldMenuSubmit.MoneyReportHeaderModals.Expected differences:
ExpenseReportListItemalready work this way.transactionCountnow excludes expenses pending deletion. The hook only receives those transactions, and the count is only used in the "all expenses on hold" prompt.Tests: deleted
ReportPreviewHoldMenuTest.tsx. Removed the provider-level hold-menu test fromMoneyRequestReportPreview.test.tsx; that wiring no longer exists. UpdatedPayActionButtonTest.tsxanduseSelectionModeReportActions.test.tsto assert theshowHoldMenucalls, including VBBAmethodIDforwarding and the forwarded lifecycleonConfirm.AI checks run locally:
npm run typecheckpassed. ESLint on all changed files passed.react-compiler-compliance-check check <changed files>passed.oxfmtwas applied.jest --findRelatedTestson the changed source files passed: 23 suites, 495 tests.spell-changedcould not run becausegit merge-basefailed in the shallow clone. An automated web run of the Tests steps passed the report preview and selection mode pay flows (screenshots under MacOS: Chrome / Safari). Other platforms are still untested.Fixed Issues
$ #102977
PROPOSAL:
Tests
Setup (one workspace admin account is enough)
Report preview
Selection mode
Dismiss
Offline tests
QA Steps
Same as Tests.
On iOS native, also verify the hold menu opens right after you tap Pay in both the report preview and selection mode, with no stuck or frozen screen.
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
Tested on web (Chrome) by MelvinBot. Report preview: Pay → Mark as paid opens the hold menu; Pay only pays $10 and moves the held expense to a new report.
Selection mode: Pay in the selection toolbar → Mark as paid opens the hold menu; Pay $30.00 pays the full report and clears the selection.