Decompose MoneyRequestConfirmationList into view-model hooks#88720
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
marcochavezf and
trjExpensify
and removed request for
a team
|
@marcochavezf 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors MoneyRequestConfirmationList by extracting its business logic, Onyx subscriptions, validation, derivations, and footer rendering into dedicated hooks and a footer component—aiming to reduce component size and limit re-renders in the Create Expense confirmation flow.
Changes:
- Added a suite of confirmation-focused hooks (policy categories/tags, report subscription, validation/confirm action, amount/tax/distance derivations, split participants, receipt training, section building, and form-error management).
- Introduced
ConfirmationFooterContentas a dedicated CTA + error surface subtree. - Updated
MoneyRequestConfirmationListto consume the new hooks/components and removed inlined logic/imports accordingly.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/MoneyRequestConfirmationList/hooks/useTransactionReportForConfirmation.ts | Adds a narrowed report subscription (type-only selector) for confirmation logic. |
| src/components/MoneyRequestConfirmationList/hooks/useTaxAmount.ts | Extracts tax code/value selection and tax amount derivation into a hook. |
| src/components/MoneyRequestConfirmationList/hooks/useSplitParticipants.tsx | Extracts split participant row construction and split share dispatch/reset UI. |
| src/components/MoneyRequestConfirmationList/hooks/useReceiptTraining.ts | Extracts test-receipt/test-drive/Manager McTest training tooltip state. |
| src/components/MoneyRequestConfirmationList/hooks/usePolicyTagsForConfirmation.ts | Extracts policy-tag Onyx subscription + derived tag list ordering. |
| src/components/MoneyRequestConfirmationList/hooks/usePolicyCategoriesForConfirmation.ts | Extracts real/draft policy categories subscriptions with fallback. |
| src/components/MoneyRequestConfirmationList/hooks/useFormErrorManagement.ts | Centralizes form error state, debouncing, violation clearing, and error message derivation. |
| src/components/MoneyRequestConfirmationList/hooks/useDistanceRequestState.ts | Extracts distance request rate/unit/currency derivation and recalculation flags. |
| src/components/MoneyRequestConfirmationList/hooks/useConfirmationValidation.ts | Extracts pure validation checks into a reusable hook returning structured results. |
| src/components/MoneyRequestConfirmationList/hooks/useConfirmationSections.ts | Extracts SelectionList section-building (split vs non-split) into a hook. |
| src/components/MoneyRequestConfirmationList/hooks/useConfirmationCtaText.ts | Extracts confirm CTA label derivation into a hook returning dropdown options. |
| src/components/MoneyRequestConfirmationList/hooks/useConfirmationAmount.ts | Extracts confirmation amount formatting + per-attendee calculation logic. |
| src/components/MoneyRequestConfirmationList/hooks/useConfirmAction.ts | Extracts confirm click handler (invoice routing, validate, confirm/send-money side effects). |
| src/components/MoneyRequestConfirmationList/ConfirmationFooterContent.tsx | Adds a dedicated footer component for CTA + error rendering and tooltip wiring. |
| src/components/MoneyRequestConfirmationList.tsx | Wires the extracted hooks/components into the main confirmation list and removes inlined logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isSplitModified = !!transaction?.splitShares && Object.keys(transaction.splitShares).some((key) => transaction.splitShares?.[Number(key) ?? -1]?.isModified); | ||
|
|
There was a problem hiding this comment.
isSplitModified is computed on every render via Object.keys(...).some(...). In the original component this was memoized; keeping it un-memoized here can reintroduce avoidable work during typing in split inputs. Consider restoring useMemo keyed off transaction?.splitShares (or deriving it inside the existing splitParticipants memo).
| const isSplitModified = !!transaction?.splitShares && Object.keys(transaction.splitShares).some((key) => transaction.splitShares?.[Number(key) ?? -1]?.isModified); | |
| const isSplitModified = useMemo(() => { | |
| if (!transaction?.splitShares) { | |
| return false; | |
| } | |
| return Object.keys(transaction.splitShares).some((key) => transaction.splitShares?.[Number(key) ?? -1]?.isModified); | |
| }, [transaction?.splitShares]); |
There was a problem hiding this comment.
No needed as react compiler memoizes this automatically
| type UseConfirmationSectionsParams = { | ||
| isTypeSplit: boolean; | ||
| shouldHideToSection: boolean; | ||
| canEditParticipant: boolean; | ||
| payeePersonalDetails: OnyxEntry<OnyxTypes.PersonalDetails> | CurrentUserPersonalDetails; | ||
| splitParticipants: MoneyRequestConfirmationListItem[]; | ||
| selectedParticipants: Participant[]; | ||
| getSplitSectionHeader: () => React.JSX.Element; | ||
| }; |
There was a problem hiding this comment.
UseConfirmationSectionsParams references React.JSX.Element but this module doesn’t import React, which will fail TypeScript compilation under strict (no global React namespace). Import React as a type (or switch the type to JSX.Element).
|
@mkhutornyi all yours |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
Code ReviewClean decomposition overall — the hook boundaries are well-chosen and the validation/action split is a nice improvement. Two issues to flag: 1.
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
mkhutornyi
left a comment
mkhutornyi
left a comment
There was a problem hiding this comment.
Please remove all manual memoizations in components and new hooks
![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: a4653a0d51
ℹ️ 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".
|
We should have unit tests for all new hooks. |
Regression & Consistency AuditI did a line-by-line comparison of every extracted hook against the original Correction to my earlier reviewMy earlier comment about 1.
|
|
Here are suggested unit tests for the new hooks. I've prioritized the hooks with the most logic — All tests go in
|
|
@OlimpiaZurek please add unit tests with the help of Melvin's suggestions. |
|
Ping me when this is ready @mkhutornyi @OlimpiaZurek |
|
@vit It’s ready now |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Minor things but good to address please @OlimpiaZurek
@marcochavezf I will be ooo, can you please handle the pr review?
| iouType: IOUType; | ||
| confirm: (params: PaymentActionParams) => void; | ||
| iouCurrencyCode: string; | ||
| policyID: string | undefined; | ||
| reportID: string; | ||
| isConfirmed: boolean | undefined; | ||
| isConfirming: boolean | undefined; | ||
| isLoadingReceipt: boolean; | ||
| splitOrRequestOptions: Array<DropdownOption<string>>; | ||
| errorMessage: string | undefined; | ||
| expensesNumber: number; | ||
| showRemoveExpenseConfirmModal: (() => void) | undefined; | ||
| shouldShowProductTrainingTooltip: boolean; | ||
| renderProductTrainingTooltip: () => React.ReactElement; |
| iouType: IOUType; | ||
| policy: OnyxEntry<OnyxTypes.Policy>; | ||
| transactionID: string | undefined; | ||
| reportID: string; | ||
| routeError: boolean | string | null | undefined; | ||
| formError: TranslationPaths | ''; | ||
| selectedParticipants: Participant[]; | ||
| isDelegateAccessRestricted: boolean; | ||
| validate: (paymentType?: PaymentMethodType) => {errorKey: TranslationPaths; shouldSetDidConfirmSplit?: boolean} | {errorKey: null} | null; | ||
| setFormError: (error: TranslationPaths | '') => void; | ||
| setDidConfirmSplit: (value: boolean) => void; | ||
| showDelegateNoAccessModal: () => void; | ||
| onConfirm?: (selectedParticipants: Participant[]) => void; | ||
| onSendMoney?: (paymentMethod: PaymentMethodType | undefined) => void; |
| transaction: OnyxEntry<OnyxTypes.Transaction>; | ||
| policy: OnyxEntry<OnyxTypes.Policy>; | ||
| policyForMovingExpenses: OnyxEntry<OnyxTypes.Policy> | undefined; | ||
| isDistanceRequest: boolean; | ||
| isMovingTransactionFromTrackExpense: boolean; | ||
| customUnitRateID: string; | ||
| distance: number; | ||
| previousTransactionCurrency: string | undefined; |
| isTypeSplit: boolean; | ||
| shouldShowReadOnlySplits: boolean; | ||
| payeePersonalDetails: OnyxEntry<OnyxTypes.PersonalDetails> | CurrentUserPersonalDetails; | ||
| selectedParticipants: Participant[]; | ||
| transaction: OnyxEntry<OnyxTypes.Transaction>; | ||
| iouAmount: number; | ||
| iouCurrencyCode: string | undefined; |
| currentUserPersonalDetails: CurrentUserPersonalDetails; | ||
| isEditingSplitBill: boolean | undefined; | ||
| isMerchantRequired: boolean | undefined; | ||
| isMerchantEmpty: boolean; |
| type ValidationResult = {errorKey: TranslationPaths; shouldSetDidConfirmSplit?: boolean} | {errorKey: null}; | ||
|
|
||
| type UseConfirmationValidationParams = { | ||
| transaction: OnyxEntry<OnyxTypes.Transaction>; |
| type MoneyRequestConfirmationListItem = (Participant & {keyForList: string}) | OptionData; | ||
|
|
||
| type UseConfirmationSectionsParams = { | ||
| isTypeSplit: boolean; |
| expensesNumber: number; | ||
| isTypeInvoice: boolean; | ||
| isTypeTrackExpense: boolean; | ||
| isTypeSplit: boolean; | ||
| isTypeRequest: boolean; | ||
| iouAmount: number; | ||
| iouType: IOUType; | ||
| policy: OnyxEntry<OnyxTypes.Policy>; | ||
| formattedAmount: string; | ||
| receiptPath: string | number; | ||
| isDistanceRequestWithPendingRoute: boolean; | ||
| isPerDiemRequest: boolean; | ||
| isNewManualExpenseFlowEnabled: boolean; |
| transaction: OnyxEntry<OnyxTypes.Transaction>; | ||
| iouAmount: number; | ||
| iouCurrencyCode: string | undefined; | ||
| iouAttendees: ReturnType<typeof getAttendees>; | ||
| isDistanceRequest: boolean; | ||
| isDistanceRequestWithPendingRoute: boolean; | ||
| shouldCalculateDistanceAmount: boolean; | ||
| distanceRequestAmount: number; | ||
| distanceCurrency: string | undefined; | ||
| isPerDiemRequest: boolean; | ||
| prevCurrency: string | undefined; | ||
| currency: string | undefined; | ||
| prevSubRates: SubRates; |
|
@codex @MelvinBot review |
Code Review (updated)Clean decomposition — logic correctness is preserved throughout, and the 1,211→625 line reduction is a meaningful readability win. All CI checks pass, tests cover the critical paths well. Two items to flag: 1. Performance: blanket memoization removal in a high-traffic componentAll
I understand this was done per reviewer request. If this is intentional (relying on React Compiler or hooks-level optimization), it would be worth noting in the PR description for future context. Otherwise, consider re-adding at minimum 2. Test gap:
|
I think this one is valid. Should we pick one of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e01a902053
ℹ️ 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".
Agreed, changed |
|
🚧 @marcochavezf 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/marcochavezf in version: 9.3.65-0 🚀
Bundle Size Analysis (Sentry): |
|
I reviewed the changes in this PR. It is a pure internal refactoring — decomposing There are no user-facing changes: no new features, no removed features, no UI label changes, no navigation changes, and no settings changes. The expense confirmation screen renders and behaves identically before and after this PR. No help site documentation changes are required. |
Explanation of Change
MoneyRequestConfirmationList had grown to ~1,211 lines, mixing rendering with business logic for policy/category/tag fetching, distance/tax/amount derivation, split-share dispatch, validation, error translation, and section building. This PR extracts that logic into focused hooks and one component:
useFormErrorManagement— owns form-error state + translated errorMessage, replacing the inline fallback ladderuseSplitParticipants— owns split rows and onSplitShareChange dispatch, so the parent no longer derives the callbackuseConfirmationSections— owns the SelectionList section data; parent no longer imports Section or section helpersuseTransactionReportForConfirmation— fetches the thread report via useOnyx with a narrow .type-only selectorusePolicyCategoriesForConfirmation / usePolicyTagsForConfirmation— own the collection subs (draft + real), eliminating parent re-renders on every POLICY_CATEGORIES / POLICY_TAGS fireuseDistanceRequestState / useTaxAmount / useConfirmationAmount / useConfirmationCtaText— own their respective derivation chains-
useReceiptTraining— owns test-receipt detection + product-training tooltip wiring; parent no longer calls useProductTrainingContextConfirmationFooterContent— renders the CTA + error surface as a dedicated subtree, replacing the inline listFooterContent JSXDead code removed: unused translate/useLocalize, duplicate routeError, inline onSplitShareChange and errorMessage memos, and parent imports of setIndividualShare, convertToBackendAmount, getIOUConfirmationOptionsFromPayeePersonalDetail, isSelectedManagerMcTest, useProductTrainingContext, and Section.
The net result is MoneyRequestConfirmationList going from ~1,211 lines to ~625 lines, with data subscriptions and action logic owned by the hooks that have the right data access
Fixed Issues
$ #88725
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as tests
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