Decompose MoneyRequestConfirmationList into view-model hooks#88720
Decompose MoneyRequestConfirmationList into view-model hooks#88720OlimpiaZurek wants to merge 3 commits intoExpensify:mainfrom
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}
| enterKeyEventListenerPriority={1} | ||
| useKeyboardShortcuts | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| isLoading={isConfirmed || isConfirming} |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing lacks a justification comment. The disable is needed because isConfirmed and isConfirming are boolean | undefined and || intentionally treats false as falsy (desired for isLoading), but that reasoning should be documented.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Using || because we want undefined and false to both be treated as falsy for isLoading
isLoading={isConfirmed || isConfirming}Reviewed at: 1fec011 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| enterKeyEventListenerPriority={1} | ||
| useKeyboardShortcuts | ||
| // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
| isLoading={isConfirmed || isConfirming || isLoadingReceipt} |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
This eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing lacks a justification comment. Same situation as above -- || is intentional because isConfirmed, isConfirming, and isLoadingReceipt include boolean | undefined types.
Add a justification comment, for example:
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Using || because we want undefined and false to both be treated as falsy for isLoading
isLoading={isConfirmed || isConfirming || isLoadingReceipt}Reviewed at: 1fec011 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
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]); |
| const getSplitSectionHeader = () => ( | ||
| <View style={[styles.mt2, styles.mb1, styles.flexRow, styles.justifyContentBetween]}> | ||
| <Text style={[styles.ph5, styles.textLabelSupporting]}>{translate('iou.participants')}</Text> | ||
| {!shouldShowReadOnlySplits && !!isSplitModified && ( | ||
| <PressableWithFeedback | ||
| onPress={() => { | ||
| // Dismiss the keyboard so that MoneyRequestAmountInput's useEffect syncs the new amount. | ||
| // Without this, the effect skips the update while the input is focused (see formatAmountOnBlur guard). | ||
| Keyboard.dismiss(); | ||
| resetSplitShares(transaction); | ||
| }} | ||
| accessibilityLabel={CONST.ROLE.BUTTON} | ||
| role={CONST.ROLE.BUTTON} | ||
| shouldUseAutoHitSlop | ||
| sentryLabel={CONST.SENTRY_LABEL.REQUEST_CONFIRMATION_LIST.RESET_SPLIT_SHARES} | ||
| > | ||
| <Text style={[styles.pr5, styles.textLabelSupporting, styles.link]}>{translate('common.reset')}</Text> | ||
| </PressableWithFeedback> | ||
| )} | ||
| </View> |
There was a problem hiding this comment.
getSplitSectionHeader is recreated on every render (not wrapped in useCallback/useMemo). Since useConfirmationSections depends on this function, it will invalidate that memo each render and can cause unnecessary section recalculation. Make the header builder stable (e.g. useCallback) or return the header element directly as a memoized value.
| const getSplitSectionHeader = () => ( | |
| <View style={[styles.mt2, styles.mb1, styles.flexRow, styles.justifyContentBetween]}> | |
| <Text style={[styles.ph5, styles.textLabelSupporting]}>{translate('iou.participants')}</Text> | |
| {!shouldShowReadOnlySplits && !!isSplitModified && ( | |
| <PressableWithFeedback | |
| onPress={() => { | |
| // Dismiss the keyboard so that MoneyRequestAmountInput's useEffect syncs the new amount. | |
| // Without this, the effect skips the update while the input is focused (see formatAmountOnBlur guard). | |
| Keyboard.dismiss(); | |
| resetSplitShares(transaction); | |
| }} | |
| accessibilityLabel={CONST.ROLE.BUTTON} | |
| role={CONST.ROLE.BUTTON} | |
| shouldUseAutoHitSlop | |
| sentryLabel={CONST.SENTRY_LABEL.REQUEST_CONFIRMATION_LIST.RESET_SPLIT_SHARES} | |
| > | |
| <Text style={[styles.pr5, styles.textLabelSupporting, styles.link]}>{translate('common.reset')}</Text> | |
| </PressableWithFeedback> | |
| )} | |
| </View> | |
| const getSplitSectionHeader = useCallback( | |
| () => ( | |
| <View style={[styles.mt2, styles.mb1, styles.flexRow, styles.justifyContentBetween]}> | |
| <Text style={[styles.ph5, styles.textLabelSupporting]}>{translate('iou.participants')}</Text> | |
| {!shouldShowReadOnlySplits && !!isSplitModified && ( | |
| <PressableWithFeedback | |
| onPress={() => { | |
| // Dismiss the keyboard so that MoneyRequestAmountInput's useEffect syncs the new amount. | |
| // Without this, the effect skips the update while the input is focused (see formatAmountOnBlur guard). | |
| Keyboard.dismiss(); | |
| resetSplitShares(transaction); | |
| }} | |
| accessibilityLabel={CONST.ROLE.BUTTON} | |
| role={CONST.ROLE.BUTTON} | |
| shouldUseAutoHitSlop | |
| sentryLabel={CONST.SENTRY_LABEL.REQUEST_CONFIRMATION_LIST.RESET_SPLIT_SHARES} | |
| > | |
| <Text style={[styles.pr5, styles.textLabelSupporting, styles.link]}>{translate('common.reset')}</Text> | |
| </PressableWithFeedback> | |
| )} | |
| </View> | |
| ), | |
| [ | |
| styles.mt2, | |
| styles.mb1, | |
| styles.flexRow, | |
| styles.justifyContentBetween, | |
| styles.ph5, | |
| styles.textLabelSupporting, | |
| styles.pr5, | |
| styles.link, | |
| translate, | |
| shouldShowReadOnlySplits, | |
| isSplitModified, | |
| transaction, | |
| ], |
| 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).
| type UseConfirmationValidationParams = { | ||
| transaction: OnyxTypes.Transaction | undefined; | ||
| transactionReport: OnyxTypes.Report | undefined; | ||
| transactionID: string | undefined; | ||
| iouType: IOUType; | ||
| iouAmount: number; | ||
| iouMerchant: string | undefined; | ||
| iouCategory: string; | ||
| iouCurrencyCode: string; | ||
| iouAttendees: Attendee[]; | ||
| policy: OnyxTypes.Policy | undefined; | ||
| policyTags: OnyxTypes.PolicyTagLists | undefined; | ||
| policyTagLists: ReturnType<typeof getTagListsFn>; | ||
| policyCategories: OnyxTypes.PolicyCategories | undefined; | ||
| selectedParticipants: Participant[]; |
There was a problem hiding this comment.
The hook parameter types don’t match how it’s called from MoneyRequestConfirmationList: transaction, transactionReport, policy, policyTags, policyCategories are passed as OnyxEntry<...> (nullable), but the hook types only allow ... | undefined. With strict TS this is not assignable (null isn’t allowed). Update the param types to accept OnyxEntry (or include null) consistently.
|
@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". |
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