feat: Add Tax, Billable, and Reimbursable selectors in the Split details page - #94434
feat: Add Tax, Billable, and Reimbursable selectors in the Split details page#94434truph01 wants to merge 32 commits into
Conversation
|
@codex review |
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.
|
![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: bccd150adf
ℹ️ 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".
| reimbursable: transactionDetails?.reimbursable, | ||
| billable: transactionDetails?.billable, | ||
| taxCode: transactionDetails?.taxCode, | ||
| taxAmount: transactionDetails?.taxAmount, |
There was a problem hiding this comment.
When a taxable expense is split into multiple new split expenses and the user saves without opening each tax row, every SplitExpense initialized here carries the original transaction's full taxAmount; the new updateSplitTransactions payload then sends that value for each split. For example, splitting an expense with $10 tax into two lines sends $10 tax on both child transactions, corrupting tax totals unless the user manually edits each one. This should be prorated/recomputed for the split amount or omitted until explicitly set.
Useful? React with 👍 / 👎.
| taxCode: split?.taxCode, | ||
| taxAmount: split?.taxAmount, | ||
| taxValue: split?.taxValue, |
There was a problem hiding this comment.
These per-split tax fields are now sent to the API, but the optimistic transaction built later in this same function still uses originalTransactionDetails.taxCode/taxAmount/taxValue instead of the current splitExpense values. When a user changes a split's tax rate or tax amount and saves, offline mode and the immediate post-save UI/search state continue showing the original tax values until the server response arrives, which makes the new editor appear not to have saved the tax change locally.
Useful? React with 👍 / 👎.
| splitExpenseDraftTransaction?.comment?.originalTransactionID, | ||
| ]); | ||
|
|
||
| console.log('>>>>>>>>>>>>>>>>>>', splitExpenseDraftTransaction); |
There was a problem hiding this comment.
This console.log runs on every render of the split details page and prints the full draft transaction, including merchant, amount, tax, report, and participant-related data, into production browser/native logs. Since this page is opened for real expenses, it leaks sensitive expense details and adds noisy render-time logging; remove the debug statement before shipping.
Useful? React with 👍 / 👎.
|
@hoangzinh 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: 3b4042d749
ℹ️ 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".
| const mileageRate = resolveSplitMileageRate({transaction, policy, isSelfDMSplit, personalPolicyOutputCurrency}); | ||
| const {unit, rate} = mileageRate; | ||
|
|
||
| const totalTaxAmount = transaction?.taxAmount ?? 0; |
There was a problem hiding this comment.
For workspace expenses, raw transaction.taxAmount is stored with the opposite sign (see getTransactionDetails/getTaxAmount), while the rest of this split flow uses the normalized transactionDetails.taxAmount. When a user clicks “make splits even” on a taxable workspace expense, this line redistributes the raw negative tax, so the draft splits carry negative tax amounts into the update payload instead of the positive per-split tax amounts used by initial/date-range distribution.
Useful? React with 👍 / 👎.
| reimbursable: transactionDetails?.reimbursable, | ||
| billable: transactionDetails?.billable, |
There was a problem hiding this comment.
Saving these new per-split reimbursable/billable values here only works for brand-new split creation. In the existing-split update path, updateSplitTransactions overwrites currentSplit.reimbursable and currentSplit.billable from the old splitTransaction before building the final UPDATE_SPLIT_TRANSACTION payload, so toggling either switch on an already-created split appears to save locally but the API receives the old values and the change is lost on sync.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot Explain more about this concern
There was a problem hiding this comment.
The reviewer is flagging a mismatch between where you write the toggled values and where they get consumed when the split already exists on the server.
What the reviewer means
-
In
updateSplitExpenseFieldyou correctly copy the edited values into the draft split —SplitExpenseItems.ts:620-621. That's fine for the draft. -
But in
updateSplitTransactions, the existing-split path (if (splitTransaction)) overwrites those values back to the old server values before building the field-change payload —SplitTransactionUpdate.ts:749-752:currentSplit.reimbursable = splitTransaction.reimbursable; currentSplit.billable = splitTransaction.billable;
transactionChangesis built from{...currentSplit}just above at:736, so after this overwritereimbursable/billableequal the old values. The diff loop at:754then deletes any key wherenewValue === oldValue, so those two fields drop out oftransactionChangesand never reachgetUpdateMoneyRequestParamsat:796. For a brand-new split (splitTransactionis falsy) this block is skipped — that's why the reviewer says it "only works for brand-new split creation."
Does it still hold on the current commit?
Partly. The splits API param now carries the new values — SplitTransactionUpdate.ts:278-279 — so the server side should persist a toggle on an existing split. The gap the overwrite still leaves is on the optimistic/local side: because the fields are stripped from transactionChanges, the individual split transaction's optimistic Onyx update (and its modifiedExpense action) won't reflect the toggle until the server response arrives. So offline, or in the moment right after saving, the split can still show the old reimbursable/billable.
How to confirm: toggle reimbursable/billable on an already-created split, save while offline (or watch the split's transaction in Onyx before the API responds), and check whether it reflects the new value immediately. If it snaps back to the old value until the response lands, the concern is real for the optimistic path and worth fixing (e.g. don't reset currentSplit.reimbursable/billable to the old values, or add them explicitly to transactionChanges). If it updates immediately, it's already covered.
There was a problem hiding this comment.
I tested and it fine
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| titleStyle={styles.flex1} | ||
| /> | ||
| )} | ||
| {shouldShowReimbursable && ( |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The Reimbursable (lines 483-494) and Billable (lines 495-506) toggle rows are structurally identical View + Text + Switch blocks that differ only in the translation key, the isOn default, and the field written by updateSplitExpenseDraftField. Duplicating this markup increases maintenance overhead — any change to the row layout, styling, or switch behavior must be made in two places.
Extract a small reusable toggle-row component (or local render helper) parameterized by label key, current value, default, and field name, and render it twice:
function SplitToggleRow({labelKey, isOn, field}: {labelKey: TranslationPaths; isOn: boolean; field: keyof Transaction}) {
const styles = useThemeStyles();
const {translate} = useLocalize();
return (
<View style={[styles.flexRow, styles.optionRow, styles.justifyContentBetween, styles.alignItemsCenter, styles.mh5]}>
<Text>{translate(labelKey)}</Text>
<Switch
accessibilityLabel={translate(labelKey)}
isOn={isOn}
onToggle={(value) => updateSplitExpenseDraftField({[field]: value})}
/>
</View>
);
}
// Usage:
{shouldShowReimbursable && <SplitToggleRow labelKey="common.reimbursable" isOn={splitExpenseDraftTransaction?.reimbursable ?? true} field="reimbursable" />}
{shouldShowBillable && <SplitToggleRow labelKey="common.billable" isOn={splitExpenseDraftTransaction?.billable ?? false} field="billable" />}Reviewed at: 3b4042d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
@truph01 can you add your comments for AI code review, please?
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-04.at.18.12.43.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-04.at.18.05.19.mov |
| titleStyle={styles.flex1} | ||
| /> | ||
| )} | ||
| {shouldShowReimbursable && ( |
There was a problem hiding this comment.
@truph01 can you add your comments for AI code review, please?
| splitExpense.taxAmount ?? | ||
| calculateIOUAmount(splitExpenses.length - 1, originalTransactionDetails?.taxAmount ?? 0, originalTransactionDetails?.currency ?? CONST.CURRENCY.USD, false), | ||
| taxValue: splitExpense.taxValue ?? originalTransactionDetails?.taxValue, | ||
| billable: originalTransactionDetails?.billable, |
There was a problem hiding this comment.
Can you check if we can update billable and reimbursable for a split transaction?
| } | ||
|
|
||
| function updateSplitExpenseDraftField(fields: Partial<OnyxTypes.Transaction>) { | ||
| Onyx.merge(`${ONYXKEYS.COLLECTION.SPLIT_TRANSACTION_DRAFT}${CONST.IOU.OPTIMISTIC_TRANSACTION_ID}`, fields); |
There was a problem hiding this comment.
Should we pass transactionID, instead of hardcoded CONST.IOU.OPTIMISTIC_TRANSACTION_ID?
There was a problem hiding this comment.
I think we can leave it as is, since none of the current callers need to pass a transaction ID other than CONST.IOU.OPTIMISTIC_TRANSACTION_ID.
|
Great! @truph01 let's try to get this over the line today then :) |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b2d89927e
ℹ️ 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".
| reimbursable: split?.reimbursable, | ||
| billable: split?.billable, | ||
| taxCode: split?.taxCode, | ||
| taxAmount: convertToBackendAmount(calculateTaxAmount(split?.taxValue, split.amount, getCurrencyDecimals(originalTransactionDetails?.currency ?? CONST.CURRENCY.USD))), |
There was a problem hiding this comment.
When a taxable expense has rounding remainders or a manually adjusted tax amount, the draft splitExpenses already carry per-split taxAmount values that sum back to the original tax, but this payload ignores those values and recalculates each child independently from taxValue and split.amount. For example, a $1.00 expense at 10% inclusive tax has a 9¢ tax that the split initializer distributes as 5¢/4¢, but saving sends 5¢/5¢ here, changing the total tax even if the user never edited tax. Please send the stored split tax amount (with the existing backend sign normalization) or explicitly reconcile the remainder before building the API/optimistic transactions.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ 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". |
@hoangzinh I fixed this by always updating the tax amount whenever the split amount changes. |
|
@truph01 all goods. Can you resolve conflicts? Then I will approve the PR |
|
@hoangzinh All good now |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #94038 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
@trjExpensify @truph01 I found a new case, and I'm unsure expected behavior:
Then what is taxAmount of the 2nd split expense? A. $0.12 (because the total original Screen.Recording.2026-08-05.at.09.13.43.mov |
|
@hoangzinh IMO, I think it should be:
The tax amount should be calculated based on the expense amount and its tax rate, rather than being derived from the total tax amount. |
|
It makes sense. Let's update it @truph01 |
|
@hoangzinh can you perform this exact step on Classic and share the result? |
|
@trjExpensify I'm unable to log in on OD. It always redirects me to ND after login |
Go to a page like this directly: https://www.expensify.com/admin_policies or in your test account, click the "Switch to Expensify Classic" button. |
|
@trjExpensify in OD, it's Screen.Recording.2026-08-06.at.21.38.20.mov |
|
That's the one then! 👍 |
|
@truph01, can you update ☝️ this today, please? |
@hoangzinh I tested the current behavior in PR, and it is already 0.29: Screen.Recording.2026-08-07.at.11.54.54.mov |
@truph01 can you update taxRate first, then adjust amount to $4 after that? |
|
like this: Screen.Recording.2026-08-07.at.15.27.34.mov |
5 participants
Explanation of Change
src/types/onyx/IOU.ts— AddtaxCode?: string,taxAmount?: number,taxValue?: numberto theSplitExpenseinterface.SplitExpenseEditPage.tsx— Add UI rows for Tax Rate, Reimbursable, and Billable (gated by the appropriate policy flags), navigating to the existingMONEY_REQUEST_STEP_TAX_RATE,MONEY_REQUEST_STEP_BILLABLE, andMONEY_REQUEST_STEP_REIMBURSABLEroutes.SplitExpenseItems.ts(initSplitExpenseItemData) — SeedtaxCode/taxAmount/taxValuefrom the parent transaction when initializing split items.SplitExpenseItems.ts(updateSplitExpenseField) — CarrytaxCode/taxAmount/taxValuethrough from the draft transaction when saving a split edit.SplitTransactionUpdate.ts— IncludetaxCode/taxAmount/taxValuein thesplitsAPI payload and in the optimistic Onyx merge.SplitTransactionSplitsParamtype — AddtaxCode,taxAmount,taxValuefields.Fixed Issues
$ #94038
PROPOSAL: #94038 (comment)
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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
Screen.Recording.2026-07-08.at.16.51.54.mov