perf: decompose MoneyRequestConfirmationList#86958
perf: decompose MoneyRequestConfirmationList#86958OlimpiaZurek wants to merge 4 commits intoExpensify:mainfrom
Conversation
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.
|
|
@ShridharGoel 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0e61b000
ℹ️ 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".
| endSpan(CONST.TELEMETRY.SPAN_CONFIRMATION_LIST_READY); | ||
| }, [transaction?.transactionID]); | ||
| // Derive iou values from transaction instead of receiving as props (CLEAN-REACT-PATTERNS-2) | ||
| const iouAmount = transaction?.amount ?? 0; |
There was a problem hiding this comment.
Read split draft amount from normalized transaction value
SplitBillDetailsPage passes SPLIT_TRANSACTION_DRAFT entries that keep edits in modifiedAmount (via setDraftSplitTransaction/getUpdatedTransaction), but this component now derives iouAmount from transaction.amount directly. In the edit-split flow where the original amount is 0, users can change the amount and still fail confirmation because the stale iouAmount stays 0 and trips the isEditingSplitBill && iouAmount === 0 guard, so completion is blocked. Please derive amount (and related fields) through the existing transaction getters (getAmount/getCurrency or getTransactionDetails) so draft modifications are respected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yes, already fixed
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-04-03.at.6.59.17.PM.movAndroid: mWeb ChromeScreen.Recording.2026-04-03.at.6.58.19.PM.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-04-03.at.6.50.11.PM.movMacOS: Chrome / SafariScreen.Recording.2026-04-03.at.6.11.55.PM.movScreen.Recording.2026-04-03.at.6.08.12.PM.movScreen.Recording.2026-04-03.at.6.57.01.PM.movScreen.Recording.2026-04-03.at.6.56.26.PM.mov |
|
Bug: Negative amount changes to positive Screen.Recording.2026-04-03.at.6.54.21.PM.mov |
Fixed |
|
No product review needed |
Explanation of Change
1. Extract side effects into 5 focused components:
MoneyRequestConfirmationListhad 15 useEffect hooks handling unrelated concerns: telemetry, tax calculation, distance amount computation, category/tag auto-selection, split validation, split rebalancing, merchant auto-population, rate validation, and more. These are unrelated responsibilities tangled in a single component (CLEAN-REACT-PATTERNS-4).This PR extracts 12 of those effects into 5 focused side-effect-only components.
2. Remove redundant props:
The component received a full transaction object AND 11 individually extracted props. All 3 parent components extracted these from the same transaction they passed — pure redundancy.
Now the component derives these values internally from the transaction it already receives. React.memo comparator simplified from 35 → 25 prop comparisons.
Result: Create Expense flow
🟢 Duration Diff: -73.900 ms (-2.4%)
🟢 Commit Count Diff: -12
Fixed Issues
$ #81849, #86986
PROPOSAL:
Tests
Offline tests
QA Steps
same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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