[HOLD] refactor: Decompose SettlementButton into variant components by report type#85367
[HOLD] refactor: Decompose SettlementButton into variant components by report type#85367TaduJR wants to merge 22 commits intoExpensify:mainfrom
Conversation
melvin-bot
bot
requested review from
linhvovan29546 and
trjExpensify
and removed request for
a team
|
@linhvovan29546 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}
|
@MelvinBot Could you please review this PR? |
PR ReviewCI is green (all checks pass except the PR Author Checklist, which is expected). I reviewed the full diff — here are my findings. Overall AssessmentThis is a well-structured decomposition. The original Specific ObservationsArchitecture (positive)
Items to address
TestsGood test coverage:
The VerdictThis PR is a solid refactoring that improves maintainability without changing behavior. The items above are mostly minor design observations, not blockers. Items 1 and 6 (referential stability of |
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
…up-SettlementButton
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
Could you please keep the hook's return focused on what it actually computes?
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
Sure, Done. |
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppScreen.Recording.2026-03-25.at.20.49.57.moviOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-03-25.at.20.39.24.mov |
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
I don’t think this actually accomplishes the goal of decomposition. Yes, it breaks logic up into separate hooks a bit, but:
- all the hooks are still called in the SettlementButton unconditionally
- we still have configuration props for different forms of the component
That’s exactly what I meant here #79387 (comment) |
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
…ementButton and pure utils
|
Sorry for the delay I caused by forgetting that we discussed doing it in a follow-up. But at minimum, we should update the PR title and description accordingly, because the PR does not do what it says it does. Personally I'd focus on just doing the decomposition correctly rather than doing this PR which doesn't really have any effect beyond better organization. |
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
TaduJR
changed the title
|
@codex review |
No worries, but I already implemented the follow-up here thinking you wanted to handle it here. The variant component decomposition is already implemented in this PR the latest commit add |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
|
@MelvinBot Please review this PR |
PR Review (Updated)This is an updated review since the PR has evolved significantly. The variant component decomposition ( All CI checks pass. Here are my findings on the current state: What works well
Items to consider
VerdictThe decomposition is now genuine — the router is hook-free, each variant is self-contained, and invoice-only subscriptions don't mount for other report types. The code is correct and all checks pass. Items 1–3 above are cleanup suggestions that could reduce boilerplate, not blockers. Item 4 is a minor behavioral equivalence check worth confirming. |
There was a problem hiding this comment.
NAB: I think we can split useSettlementData into narrower as well that would save 2-3 Onyx subscriptions per variant. That's meaningful but not huge. The bigger wins are:
- USER_WALLET removed from Expense and Invoice paths (wallet is IOU-only)
- BANK_ACCOUNT_LIST + FUND_LIST removed from IOU path (IOU uses wallet, not bank accounts directly)
- usePermissions() removed from Expense and IOU (beta flags are invoice-only)
- useActiveAdminPolicies() removed from Invoice (policy selection is IOU-only)
Combined with the duplicate subscriptions in usePaymentGuard/useApproveAction (BETAS, CONCIERGE_REPORT_ID, OWNER_BILLING), the total potential savings is ~5-6 subscriptions per variant — roughly a 30% reduction from the current state.
But this should be handled in a separate PR
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
This PR is mostly a code reorganization (despite adding variants), not a major performance improvement. I think we still need to handle performance work in follow-up PRs per variant
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx
roryabraham
left a comment
roryabraham
left a comment
There was a problem hiding this comment.
I think there's a lot of good here, but it's still not really accomplishing the goal of decomposition. Ultimately, I think it's not really practical to decompose SettlementButton without first decomposing ButtonWithDropdownMenu to use composition-via-JSX rather than composition-via-config.
current:
// ExpenseSettlementButton (trimmed)
function ExpenseSettlementButton({shouldHidePaymentOptions, shouldShowApproveButton, ...}) {
// ... hooks, data fetching ...
let paymentButtonOptions: Array<DropdownOption<string>>;
if (shouldHidePaymentOptions && shouldShowApproveButton) {
paymentButtonOptions = [approveButtonOption];
} else {
const buttonOptions = [];
if (shouldShowBusinessBankAccountOptions) {
for (const account of businessBankAccountOptionList) {
buttonOptions.push({text: account.text, icon: ..., value: ..., onSelected: () => { ... }});
}
}
if (shouldShowPayElsewhereOption) {
buttonOptions.push({text: ..., icon: ..., value: ...});
}
if (shouldShowApproveButton) {
buttonOptions.push(approveButtonOption);
}
paymentButtonOptions = buttonOptions;
}
return <BaseSettlementButton paymentButtonOptions={paymentButtonOptions} ... />;
}Compositional (if ButtonWithDropdownMenu accepted children and was also compositional):
// ExpenseSettlementButton -- only knows about expense payment options
function ExpenseSettlementButton({onPress, children, ...props}) {
// ... hooks, data fetching ...
return (
<KYCWall ...>
{(triggerKYCFlow, buttonRef) => (
<ButtonWithDropdownMenu onPress={...} ...>
{shouldShowBusinessBankAccountOptions && businessBankAccountOptionList.length === 0 && (
<ButtonWithDropdownMenu.Option
text={translate('iou.settleExpensify')}
icon={Bank}
value={CONST.PAYMENT_METHODS.BUSINESS_BANK_ACCOUNT}
/>
)}
{shouldShowBusinessBankAccountOptions && businessBankAccountOptionList.map((account) => (
<ButtonWithDropdownMenu.Option
key={account.methodID}
text={account.text}
icon={account.icon}
value={CONST.PAYMENT_METHODS.BUSINESS_BANK_ACCOUNT}
onSelected={() => onPress({paymentType: CONST.IOU.PAYMENT_TYPE.VBBA, ...})}
/>
))}
{shouldShowPayElsewhereOption && (
<ButtonWithDropdownMenu.Option
text={translate('iou.payElsewhere')}
icon={Cash}
value={CONST.IOU.PAYMENT_TYPE.ELSEWHERE}
/>
)}
{children}
</ButtonWithDropdownMenu>
)}
</KYCWall>
);
}
// Consumer (PayPrimaryAction) -- composes what it needs
{shouldShowPayButton ? (
<AnimatedActionButton ...>
<ExpenseSettlementButton onPress={confirmPayment} ...>
{shouldShowApproveButton && (
<ButtonWithDropdownMenu.Option
text={translate('iou.approve', {formattedAmount})}
icon={ThumbsUp}
value={CONST.IOU.REPORT_ACTION_TYPE.APPROVE}
disabled={shouldDisableApproveButton}
onSelected={confirmApproval ?? handleApprove}
/>
)}
</ExpenseSettlementButton>
</AnimatedActionButton>
) : shouldShowApproveButton ? (
<AnimatedActionButton ...>
<ApproveButton onPress={confirmApproval ?? handleApprove} ... />
</AnimatedActionButton>
) : null}That would enable us to eliminate boolean flags such as shouldShowApproveButton, and overall implement this refactor correctly. I think the lesson is that decomposition needs to start at the lowest level and then work up.
I think our next step should be to HOLD or close this PR for now, create a separate PR to refactor ButtonWithDropdownMenu to support composition via children/JSX, eliminating config-array driven rendering. Then come back to this, using JSX to properly compose variants of the settlement button and nested options in the dropdown menu, without SettlementButton/index.tsx acting as a switch or without megalithic props.
…up-SettlementButton
TaduJR
changed the title
|
Thanks for sticking with this @TaduJR and @linhvovan29546, you've been doing a great job. It's just a big, challenging refactor, and it's really valuable to get it right. |
Oh don't mention it, Please.
Yea, but learning and researching a lot. That makes it super exciting for me. Thanks so much for following up @roryabraham |
…up-SettlementButton # Conflicts: # src/components/SettlementButton/index.tsx # src/hooks/useBulkPayOptions.ts
5 participants
Explanation of Change
Decomposes
SettlementButtoninto variant components by report type (ExpenseSettlementButton,InvoiceSettlementButton,IOUSettlementButton) with a sharedBaseSettlementButtonrendering shell. A hook-free router selects the correct variant based oniouReport?.type, so each variant only mounts the hooks it needs invoice-only subscriptions no longer mount for expense/IOU reports. Shared logic extracted intouseSettlementData,usePaymentGuard,useApproveAction,settlementUtils, anduseInvoicePaymentOptions. Also migratesusePaymentOptions/useBulkPayOptionsto share centralized data viauseSettlementData.Fixed Issues
$ #79387
PROPOSAL:
Tests
Prerequisites
Test 1 — Pay from Report Preview in Chat
Test 2 — Pay from Report Header
Test 3 — Invoice Room
Test 4 — Search Results
Test 5 — Pay Someone (Quick Action)
Test 6 — Bulk Pay from Search
Offline tests
Same as 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 stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Mac-Chrome.mp4