Conversation
|
@Krishna2323 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}
| }, [defaultTaxCode, transactionID]); | ||
| const defaultTaxCode = getDefaultTaxCode(policy, transaction); | ||
| setMoneyRequestTaxRate(transactionID, defaultTaxCode ?? ''); | ||
| }, [customUnitRateID, policy, previousCustomUnitRateID, previousTransactionCurrency, previousTransactionModifiedCurrency, shouldShowTax, transaction, transactionID]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability.
Consider replacing transaction in the dependency array with specific transaction properties that are actually used in the effect:
}, [customUnitRateID, policy, previousCustomUnitRateID, previousTransactionCurrency, previousTransactionModifiedCurrency, shouldShowTax, transaction?.transactionID, transaction?.taxCode, transaction?.modifiedCurrency, transaction?.currency, transactionID]);
| shouldShowTax, | ||
| previousTransactionAmount, | ||
| previousTransactionCurrency, | ||
| transaction, |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability.
Consider replacing transaction in the dependency array with specific transaction properties that are actually used in the effect:
}, [
policy,
shouldShowTax,
previousTransactionAmount,
previousTransactionCurrency,
transaction?.transactionID,
transaction?.taxCode,
transaction?.amount,
transaction?.currency,
transaction?.taxAmount,
isDistanceRequest,
customUnitRateID,
previousCustomUnitRateID,
previousTaxCode,
distance,
]);
| setMoneyRequestTaxAmount(transaction.transactionID, taxAmountInSmallestCurrencyUnits); | ||
| } | ||
| }, [ | ||
| policy, |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability.
Consider replacing policy in the dependency array with specific policy properties that are actually used in the effect. Based on the effect logic, you likely only need:
}, [
policy?.id,
// ... other dependencies
]);
| const previousCustomUnitRateID = usePrevious(customUnitRateID); | ||
| useEffect(() => { | ||
| if (!transactionID) { | ||
| // 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 |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects as dependencies causes hooks to re-execute whenever any property changes, even unrelated ones. Specifying individual properties creates more granular dependency tracking, reducing unnecessary hook executions and improving performance predictability.
Consider replacing policy in the dependency array with specific policy properties that are actually used in the effect. Based on the getDefaultTaxCode function call, you likely only need:
}, [customUnitRateID, policy?.id, previousCustomUnitRateID, previousTransactionCurrency, previousTransactionModifiedCurrency, shouldShowTax, transaction?.transactionID, transaction?.taxCode, transaction?.modifiedCurrency, transaction?.currency, transactionID]);
Codecov Report❌ Patch coverage is
... and 316 files with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
Straight revert of these 3 PRs:
Explanation of Change
#73045 caused this deploy blocker, and we tried to fix it with two follow up CP'ed PRs, but more deploy blockers came up. Lets revert all of it so we can unblock the deploy.
Fixed Issues
$ #73309
$ #73411
$ #73413
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
MacOS: Desktop