Improve deleted tax behavior #78472
Improve deleted tax behavior #78472
Conversation
melvin-bot
bot
requested review from
thesahindia
and removed request for
a team
|
@thesahindia 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] |
melvin-bot
bot
requested review from
JmillsExpensify,
flaviadefaria,
heyjennahay,
joekaufmanexpensify and
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| transaction: currentTransaction, | ||
| }); | ||
|
|
||
| if (!isTaxDeleted) { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
The deletedTaxOption object is created inline within the useMemo callback. While this is memoized, the object is recreated on every dependency change. Since this object is being spread into section data and passed as props to list items, it would be better to ensure stable references.
Consider extracting the object properties into the dependency array or restructuring to avoid recreation when the actual values havent changed:
const deletedTaxOption = useMemo(() => {
if (!isTaxDeleted) {
return null;
}
return {
code: transaction?.taxCode,
text: transaction?.taxValue ?? ,
keyForList: transaction?.taxCode ?? ,
searchText: transaction?.taxValue ?? ,
tooltipText: transaction?.taxValue ?? ,
isDisabled: true,
isSelected: true,
};
}, [isTaxDeleted, transaction?.taxCode, transaction?.taxValue]);
const sections = useMemo(() => {
const baseSections = getTaxRatesSection({
policy,
searchValue,
localeCompare,
selectedOptions,
transaction: currentTransaction,
});
if (!deletedTaxOption) {
return baseSections;
}
return baseSections.map((section) => ({
...section,
data: [...section.data, deletedTaxOption],
}));
}, [policy, searchValue, localeCompare, selectedOptions, currentTransaction, deletedTaxOption]);
|
|
||
| const shouldShowTextInput = !isTaxRatesCountBelowThreshold; | ||
|
|
||
| const isTaxDeleted = |
There was a problem hiding this comment.
❌ PERF-2 (docs)
The .find() method is being used to check if a taxCode exists as a key in an object, but this unnecessarily iterates through all keys. A simple property lookup would be more efficient.
Replace this:
const isTaxDeleted =
!!transaction?.taxCode && transaction?.taxValue !== undefined && Object.keys(taxRates?.taxes ?? {}).find((taxCode) => taxCode === transaction?.taxCode) === undefined;With a direct property check:
const isTaxDeleted =
!!transaction?.taxCode &&
transaction?.taxValue !== undefined &&
!taxRates?.taxes?.[transaction.taxCode];This avoids creating an array of all keys and iterating through them.
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.
|
1 participant
Explanation of Change
Handle deleted tax rates by detecting removed codes, showing them as disabled/selected in the picker, and clearing the tax on submit when editing
Fixed Issues
$ #73033
PROPOSAL: #73033 (comment)
Tests
Same QA step
Offline tests
QA Steps
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