Skip to content

Fix tax updating for changing waypoints or currency #73045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luacmartins merged 3 commits into from
Oct 22, 2025
Merged

Fix tax updating for changing waypoints or currency #73045

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0459a32
Remove usePrevious tracking to fix waypoints not updating tax
roryabraham Oct 21, 2025
13021fa
Fix manual currency change case too
roryabraham Oct 21, 2025
47c93ba
Merge branch 'main' into Rory-TaxUpdatingForWaypoints
roryabraham Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 16 additions & 41 deletions src/components/MoneyRequestConfirmationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,27 +332,19 @@ function MoneyRequestConfirmationList({

const shouldShowTax = isTaxTrackingEnabled(isPolicyExpenseChat, policy, isDistanceRequest, isPerDiemRequest);

const previousTransactionAmount = usePrevious(transaction?.amount);
const previousTransactionCurrency = usePrevious(transaction?.currency);
const previousTransactionModifiedCurrency = usePrevious(transaction?.modifiedCurrency);
const previousCustomUnitRateID = usePrevious(customUnitRateID);
useEffect(() => {
// previousTransaction is in the condition because if it is falsy, it means this is the first time the useEffect is triggered after we load it, so we should calculate the default
// tax even if the other parameters are the same against their previous values.
if (
!shouldShowTax ||
!transaction ||
!transactionID ||
(transaction.taxCode &&
previousTransactionModifiedCurrency === transaction.modifiedCurrency &&
previousTransactionCurrency === transaction.currency &&
previousCustomUnitRateID === customUnitRateID)
) {
// Set the default tax code when conditions change
if (!shouldShowTax || !transaction || !transactionID) {
return;
}
const defaultTaxCode = getDefaultTaxCode(policy, transaction);
setMoneyRequestTaxRate(transactionID, defaultTaxCode ?? '');
}, [customUnitRateID, policy, previousCustomUnitRateID, previousTransactionCurrency, previousTransactionModifiedCurrency, shouldShowTax, transaction, transactionID]);
const currentTaxCode = transaction.taxCode ?? '';

// Update tax code if it's different from what should be the default
if (defaultTaxCode !== currentTaxCode) {
setMoneyRequestTaxRate(transactionID, defaultTaxCode ?? '');
}
}, [customUnitRateID, policy, shouldShowTax, transaction, transactionID]);
Copy link
Contributor

@github-actions github-actions bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-6 (docs)

Instead of passing entire objects as dependencies, specify individual object properties to create more granular dependency tracking and reduce unnecessary hook executions.

}, [customUnitRateID, policy, shouldShowTax, transaction?.taxCode, transaction?.currency, transaction?.modifiedCurrency, transactionID]);

Copy link
Contributor Author

@roryabraham roryabraham Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion is not correct as written. In theory I agree with the principle, but in practice there are a bunch of transaction fields used within this effect, and while I could enumerate them all, it would make the effect more brittle - changing one of the TransactionUtils functions to use an additional property would result in the effect not being re-run when it needs to.

This is the kind of thing React Compiler could probably memoize much more effectively than we could manually, because it could deeply analyze the usage of all variables in your function scope and apply the kind of granular memoization that this comment strives for, but can do it reliably at build time.


const isMovingTransactionFromTrackExpense = isMovingTransactionFromTrackExpenseUtil(action);

Expand Down Expand Up @@ -487,19 +479,9 @@ function MoneyRequestConfirmationList({
}
}, [shouldCalculateDistanceAmount, isReadOnly, distanceRequestAmount, transactionID, currency, isTypeSplit, isPolicyExpenseChat, selectedParticipantsProp, transaction]);

const previousTaxCode = usePrevious(transaction?.taxCode);

// Calculate and set tax amount in transaction draft
useEffect(() => {
if (
!shouldShowTax ||
!transaction ||
(transaction.taxAmount !== undefined &&
previousTransactionAmount === transaction.amount &&
previousTransactionCurrency === transaction.currency &&
previousCustomUnitRateID === customUnitRateID &&
previousTaxCode === transaction.taxCode)
) {
if (!shouldShowTax || !transaction) {
return;
}

Expand All @@ -520,20 +502,13 @@ function MoneyRequestConfirmationList({
const taxPercentage = getTaxValue(policy, transaction, taxCode) ?? '';
const taxAmount = calculateTaxAmount(taxPercentage, taxableAmount, transaction.currency);
const taxAmountInSmallestCurrencyUnits = convertToBackendAmount(Number.parseFloat(taxAmount.toString()));
setMoneyRequestTaxAmount(transaction.transactionID, taxAmountInSmallestCurrencyUnits);

// Only update if the calculated tax amount is different from the current one
if (transaction.taxAmount !== taxAmountInSmallestCurrencyUnits) {
setMoneyRequestTaxAmount(transaction.transactionID, taxAmountInSmallestCurrencyUnits);
}
}
}, [
policy,
shouldShowTax,
previousTransactionAmount,
previousTransactionCurrency,
transaction,
isDistanceRequest,
customUnitRateID,
previousCustomUnitRateID,
previousTaxCode,
distance,
]);
}, [policy, shouldShowTax, transaction, isDistanceRequest, customUnitRateID, distance]);
Copy link
Contributor

@github-actions github-actions bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ PERF-6 (docs)

Instead of passing entire objects as dependencies, specify individual object properties to create more granular dependency tracking and reduce unnecessary hook executions.

}, [policy, shouldShowTax, transaction?.amount, transaction?.taxAmount, transaction?.taxCode, transaction?.currency, transaction?.transactionID, isDistanceRequest, customUnitRateID, distance]);


// If completing a split expense fails, set didConfirm to false to allow the user to edit the fields again
if (isEditingSplitBill && didConfirm) {
Expand Down
Loading