Skip to content

[CP Stg] Change tax on confirm page #73361

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
blimpich merged 3 commits into from
Oct 23, 2025
Merged

[CP Stg] Change tax on confirm page #73361

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f218ce
Correctly track changes to currency to update taxes
roryabraham Oct 23, 2025
9fead48
Improve comments
roryabraham Oct 23, 2025
423d2c9
Simplify solution at the cost of 0 or 1 additional renders
roryabraham Oct 23, 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
15 changes: 5 additions & 10 deletions src/components/MoneyRequestConfirmationList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,19 +332,14 @@ function MoneyRequestConfirmationList({

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

// Update the tax code when the default changes (for example, because the transaction currency changed)
const defaultTaxCode = getDefaultTaxCode(policy, transaction) ?? '';
Comment thread
blimpich marked this conversation as resolved.
Comment thread
blimpich marked this conversation as resolved.
Comment thread
blimpich marked this conversation as resolved.
Comment thread
roryabraham marked this conversation as resolved.

@github-actions github-actions Bot Oct 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ PERF-6 (docs)

The defaultTaxCode computation should be moved inside the useEffect or properly memoized to avoid running expensive operations on every render. Currently, getDefaultTaxCode(policy, transaction) is called on every render, which could include complex policy and transaction lookups.

// Option 1: Move computation inside useEffect
useEffect(() => {
    if (!transactionID) {
        return;
    }
    const defaultTaxCode = getDefaultTaxCode(policy, transaction) ?? '';
    setMoneyRequestTaxRate(transactionID, defaultTaxCode);
}, [policy, transaction, transactionID]);

// Option 2: Properly memoize the computation
const defaultTaxCode = useMemo(() => getDefaultTaxCode(policy, transaction) ?? '', [policy, transaction]);

useEffect(() => {
// Set the default tax code when conditions change
if (!shouldShowTax || !transaction || !transactionID) {

@linhvovan29546 linhvovan29546 Dec 10, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We removed the shouldShowTax flag, which caused this issue #75056 (comment). In some cases we need to return early so we applied the fix in #76068.

if (!transactionID) {
return;
}
const defaultTaxCode = getDefaultTaxCode(policy, transaction);
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]);
setMoneyRequestTaxRate(transactionID, defaultTaxCode);
Comment thread
blimpich marked this conversation as resolved.
}, [defaultTaxCode, transactionID]);
Comment thread
roryabraham marked this conversation as resolved.
Comment thread
roryabraham marked this conversation as resolved.

@github-actions github-actions Bot Oct 23, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ PERF-6 (docs)

The dependency array should specify individual object properties instead of entire objects. Currently, passing defaultTaxCode as a dependency means the effect will run whenever getDefaultTaxCode() returns a different reference, but we should be more specific about which properties actually matter.

// Instead of relying on defaultTaxCode computed outside, specify the actual dependencies:
useEffect(() => {
    if (!transactionID) {
        return;
    }
    const defaultTaxCode = getDefaultTaxCode(policy, transaction) ?? '';
    setMoneyRequestTaxRate(transactionID, defaultTaxCode);
}, [policy?.taxRates, policy?.defaultExternalID, transaction?.currency, transaction?.amount, transactionID]);

This creates more granular dependency tracking and reduces unnecessary hook executions when unrelated policy or transaction properties change.


const isMovingTransactionFromTrackExpense = isMovingTransactionFromTrackExpenseUtil(action);

Expand Down
Loading