-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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.
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
perf: reduce getAllReportTransactions usage in ReportPreview #46500
perf: reduce getAllReportTransactions usage in ReportPreview #46500
Conversation
@mkhutornyi 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] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-07-31.at.6.04.16.PM.movAndroid: mWeb ChromeScreen.Recording.2024-07-31.at.6.06.37.PM.moviOS: NativeScreen.Recording.2024-07-31.at.5.49.37.PM.moviOS: mWeb SafariScreen.Recording.2024-07-31.at.5.55.33.PM.movMacOS: Chrome / SafariScreen.Recording.2024-07-31.at.5.45.13.PM.movMacOS: DesktopScreen.Recording.2024-07-31.at.6.28.18.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, just few Comments..
src/components/MoneyReportHeader.tsx
Outdated
@@ -141,7 +141,7 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
} | |||
setPaymentType(type); | |||
setRequestType(CONST.IOU.REPORT_ACTION_TYPE.PAY); | |||
if (ReportUtils.hasHeldExpenses(moneyRequestReport.reportID)) { | |||
if (TransactionUtils.isAnyTransactionOnHold(allTransactions)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we create a variable for TransactionUtils.isAnyTransactionOnHold(allTransactions)
i see its being used few times in this file..
areAllRequestsBeingSmartScanned: ReportUtils.areAllRequestsBeingSmartScanned(iouReportID, action), | ||
hasOnlyTransactionsWithPendingRoutes: ReportUtils.hasOnlyTransactionsWithPendingRoutes(iouReportID), | ||
hasNonReimbursableTransactions: ReportUtils.hasNonReimbursableTransactions(iouReportID), | ||
hasMissingSmartscanFields: TransactionUtils.hasAnyTransactionMissingFields(allTransactions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasMissingSmartscanFields: TransactionUtils.hasAnyTransactionMissingFields(allTransactions), | |
hasMissingSmartscanFields: TransactionUtils.hasAnyTransactionMissingSmartscanFields(allTransactions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's keep it specific to smart scan fields vs any missing field.
const {hasMissingSmartscanFields, areAllRequestsBeingSmartScanned, hasOnlyTransactionsWithPendingRoutes, hasNonReimbursableTransactions} = useMemo( | ||
() => ({ | ||
hasMissingSmartscanFields: ReportUtils.hasMissingSmartscanFields(iouReportID), | ||
areAllRequestsBeingSmartScanned: ReportUtils.areAllRequestsBeingSmartScanned(iouReportID, action), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
areAllRequestsBeingSmartScanned
in ReportUtils is unused now, can we remove it please
const {hasMissingSmartscanFields, areAllRequestsBeingSmartScanned, hasOnlyTransactionsWithPendingRoutes, hasNonReimbursableTransactions} = useMemo( | ||
() => ({ | ||
hasMissingSmartscanFields: ReportUtils.hasMissingSmartscanFields(iouReportID), | ||
areAllRequestsBeingSmartScanned: ReportUtils.areAllRequestsBeingSmartScanned(iouReportID, action), | ||
hasOnlyTransactionsWithPendingRoutes: ReportUtils.hasOnlyTransactionsWithPendingRoutes(iouReportID), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above ^ for hasOnlyTransactionsWithPendingRoutes
src/components/MoneyReportHeader.tsx
Outdated
const navigateBackToAfterDelete = useRef<Route>(); | ||
const hasScanningReceipt = ReportUtils.getTransactionsWithReceipts(moneyRequestReport?.reportID).some((t) => TransactionUtils.isReceiptBeingScanned(t)); | ||
const transactionIDs = TransactionUtils.getAllReportTransactions(moneyRequestReport?.reportID).map((t) => t.transactionID); | ||
const transactionIDs = allTransactions.map((t) => t.transactionID); | ||
const allHavePendingRTERViolation = TransactionUtils.allHavePendingRTERViolation(transactionIDs); | ||
// allTransactions in TransactionUtils might have stale data | ||
const hasOnlyHeldExpenses = ReportUtils.hasOnlyHeldExpenses(moneyRequestReport.reportID, transactions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use TransactionUtils.areAllExpensesOnHold
here also ?
Not sure if this is relevant for App/tests/actions/EnforceActionExportRestrictions.ts Lines 7 to 10 in d2fdb27
|
Yep, I think we should pursue that approach, using edit: Maybe this is irrelevant if we can move the access of all report transactions into a util file only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work and performance improvement. Overall I think it's a good idea. I would like to see us consolidate things even further so that the report utils have a mapping of reportIDs to all their transactions, and that can be the single points where we access them.
areAllRequestsBeingSmartScanned: ReportUtils.areAllRequestsBeingSmartScanned(iouReportID, action), | ||
hasOnlyTransactionsWithPendingRoutes: ReportUtils.hasOnlyTransactionsWithPendingRoutes(iouReportID), | ||
hasNonReimbursableTransactions: ReportUtils.hasNonReimbursableTransactions(iouReportID), | ||
hasMissingSmartscanFields: TransactionUtils.hasAnyTransactionMissingFields(allTransactions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's keep it specific to smart scan fields vs any missing field.
src/libs/TransactionUtils.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep one util function for each separate idea. Now we're adding multiple ways to do the same thing by having these transaction utils that mirror the report utils.
Instead, could we please require that all the report utils operating on allReportTransactions
take them in as a parameter? Actually, now that I think about that, can't we have ReportUtils create a mapping of reportIDs to their transactions as part of the Onyx connection to all transactions, then each util can easily lookup all transactions for a report?
95312c2
to
2714eec
Compare
@ishpaul777 @neil-marcellini thanks for your comments! I already pushed latest changes to the branch
Thanks for bringing the idea of mapping @neil-marcellini, it helped me reduce the diff a lot 🎉
I'll create a follow up PR with this change later this week 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, looks really clean now. Thanks so much! Since we still have some components using the getAllTransactions util, the follow up moving to useOnyx will be a good idea.
@@ -542,6 +542,7 @@ Onyx.connect({ | |||
}); | |||
|
|||
let allTransactions: OnyxCollection<Transaction> = {}; | |||
let reportsTransactions: Record<string, Transaction[]> = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: Maybe rename to reportIDsToTransactions
?
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.15-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.15-9 🚀
|
@@ -98,17 +98,16 @@ function MoneyReportHeader({policy, report: moneyRequestReport, transactionThrea | |||
const [isHoldMenuVisible, setIsHoldMenuVisible] = useState(false); | |||
const [paymentType, setPaymentType] = useState<PaymentMethodType>(); | |||
const [requestType, setRequestType] = useState<ActionHandledType>(); | |||
const allTransactions = useMemo(() => TransactionUtils.getAllReportTransactions(moneyRequestReport?.reportID), [moneyRequestReport?.reportID]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents the header from being refreshed when transactions for the report change causing #47084
Details
Each time
getAllReportTransactions
is called, it's doing filtering on all transactions to get only those matching the report ID. Instead of calling this function on multiple levels inReportPreview
, let's get report transactions on top-level and use newly created utils to avoid filtering this list.In total, there were 12 function calls running
getAllReportTransactions
. It was reduced to call it only once. In an example profile trace from Jason function was running for more than 500ms in total, I think with this change we can reduce it to around 100ms.Fixed Issues
$ #45528
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop