quinthar
commented
4 months ago Upgrading to HIGH as it's fundamentally about making it work correctly.
Open quinthar opened 5 months ago
quinthar
commented
4 months ago Upgrading to HIGH as it's fundamentally about making it work correctly.
melvin-bot[bot]
commented
4 months ago Job added to Upwork: https://www.upwork.com/jobs/~01432274cc2b58438c
melvin-bot[bot]
commented
4 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @fedirjh (External)
melvin-bot[bot]
commented
4 months ago Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
melvin-bot[bot]
commented
4 months ago Platforms in OP are ✅)
shubham1206agra
commented
4 months ago Finalize order of LHN in #focus mode
This should be fixed with getOrderedReportIDs
We should modify the getOrderedReportIDs logic to the above set of rules to get the desired result.
"pinned chats" refers to chats that are actually pinned, as well as any chat with a "green/red brick road" (GBR/RBR), which are treated as if they are pinned.
result.allReportErrors = OptionsListUtils.getAllReportErrors(report, reportActions) as OnyxCommon.Errors;
result.brickRoadIndicator = Object.keys(result.allReportErrors ?? {}).length !== 0 ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';These lines of code was used to determine the RBR indicator in SideBar, which will be only used during rendering options after getting sorted reports.
We need to use this inside getOrderedReportIDs to get the RBR reports to get them alongside pinned and GBR reports.
When sorting alphabetically, it should only sort the alphanumeric characters -- other characters should be ignored.
For this we need to use the replace replace(/[^0-9a-z]/gi, '') to remove non-alphanumeric characters.
Only caveat right now is that after replace string can get empty, and in js, this will get highest preference in sorting order.
@quinthar I need your opinion on this: should we push these reports down by temporarily replacing empty string with Z (as this has the lowest priority in localeCompare) or let it be up (this may be weird as emoji only threads might get highest priority)?
Edit: The chatReportSelector inside SidebarLinksData has some fields missing (isPolicyExpenseChat, isOwnPolicyExpenseChat, isCancelledIOU) due to which title of the report is determined incorrectly by getOrderedReportIDs. Adding these fixed the order issue with policy expense reports.
thebigshotsam
commented
4 months ago When the LHN (Left Hand Navigation) is in "focus" mode, the existing sorting logic was not aligning with the expected behavior. The sorting criteria were not correctly implemented, specifically for pinned reports and those in "focus" mode.
The LHN sorting logic has been reviewed and updated to meet the following requirements:
Focus Mode:
Most Recent Mode:
The existing sorting function has been modified to achieve the desired behavior. Here are the key updates:
Grouping:
Sorting:
Combining Arrays:
allReportsSorted).Final Sorting:
Extracting ReportIDs:
The sortLHNReports function should be used to obtain the sorted list of reportIDs for the LHN.
const sortedLHNReports = sortLHNReports(reportsToDisplay, isInDefaultMode);
// Function to sort LHN reports
function sortLHNReports(reportsToDisplay: Report[], isInDefaultMode: boolean): string[] {
const pinnedAndGBRReports: Report[] = [];
const draftReports: Report[] = [];
const nonArchivedReports: Report[] = [];
const archivedReports: Report[] = [];
// Calculate properties and categorize reports
reportsToDisplay.forEach((report) => {
report.displayName = ReportUtils.getReportName(report);
report.iouReportAmount = ReportUtils.getMoneyRequestReimbursableTotal(report, allReports);
const isPinned = report.isPinned ?? false;
const reportAction = ReportActionsUtils.getReportAction(report.parentReportID ?? '', report.parentReportActionID ?? '');
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) {
pinnedAndGBRReports.push(report);
} else if (report.hasDraft) {
draftReports.push(report);
} else if (ReportUtils.isArchivedRoom(report)) {
archivedReports.push(report);
} else {
nonArchivedReports.push(report);
}
});
// Sort each group of reports accordingly
pinnedAndGBRReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
draftReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
if (isInDefaultMode) {
// Sort non-archived reports based on most recent activity and alphabetically
nonArchivedReports.sort((a, b) => {
const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0;
return compareDates || compareDisplayNames;
});
// For archived reports, ensure most recent reports are at the top by reversing the order
archivedReports.sort((a, b) => (a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0));
} else {
// Sort non-archived and archived reports alphabetically
nonArchivedReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
archivedReports.sort((a, b) => (a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0));
}
// Combine all groups into a single array for final sorting
const allReportsSorted: Report[] = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports];
// Sort the combined array based on specified criteria
allReportsSorted.sort((a, b) => {
const comparePinned = (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0);
const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0;
const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
// First, sort by pinned status, then by display names, and finally by dates in most recent mode
return comparePinned || compareDisplayNames || compareDates;
});
// Extract reportIDs from the final sorted array
const LHNReports = allReportsSorted.map((report) => report.reportID);
// Update the cache with the sorted reportIDs
setWithLimit(reportIDsCache, cachedReportsKey, LHNReports);
return LHNReports;
}
// Function to compare string dates
function compareStringDates(dateStringA: string, dateStringB: string): number {
const dateA = new Date(dateStringA);
const dateB = new Date(dateStringB);
return dateB.getTime() - dateA.getTime();
}
The current sorting logic in the Last Heard Navigation (LHN) feature doesn't handle non-alphanumeric characters appropriately when sorting alphabetically. The requirement is to ignore these characters during alphabetical sorting for both "Pinned/GBR Reports" and "Draft Reports."pinnedAndGBRReports and draftReports in the sortLHNReports function.pinnedAndGBRReports.sort((a, b) => {
const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});
draftReports.sort((a, b) => {
const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});
// ... (previous code)// Sort each group of reports accordingly pinnedAndGBRReports.sort((a, b) => { const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, ''); const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, ''); return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0; });
draftReports.sort((a, b) => { const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, ''); const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, ''); return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0; });
// ... (remaining code)
// Sort the combined array based on specified criteria allReportsSorted.sort((a, b) => { const comparePinned = (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0); const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0; const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
// First, sort by pinned status, then by display names, and finally by dates in most recent mode
return comparePinned || compareDisplayNames || compareDates;});
// ... (remaining code)
## Benefits:
- This modification ensures that non-alphanumeric characters are ignored during the sorting of pinnedAndGBRReports and draftReports, aligning with the specified requirements. Additionally, the existing logic for GBR and RBR verification in the sorting process remains intact.
- The proposed changes will enhance the alphabetical sorting logic by ignoring non-alphanumeric characters, aligning with the specified requirements.
Expensify account email: connect@sakshamtiwari.co.in
Upwork Profile Link: https://www.upwork.com/freelancers/~018f9e03301b35bff0📣 @thebigshotsam! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>Added some minor comment in the proposal.
melvin-bot[bot]
commented
4 months ago @JmillsExpensify @fedirjh this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks!
melvin-bot[bot]
commented
4 months ago Current assignee @fedirjh is eligible for the Internal assigner, not assigning anyone new.
quinthar
commented
4 months ago What are the next steps? Who are we waiting on, and what are waiting on them to do?
fedirjh
commented
4 months ago What are the next steps?
Nex steps should be reviewing the proposals.
fedirjh
commented
4 months ago Review the LHN logic for sorting in focus mode to understand what it's currently doing, and verify it's correct
@shubham1206agra , @thebigshotsam I think we should review the current implementation before moving on to the solution. Could any of you explain what the current behavior is? How are the reports ordered?
shubham1206agra
commented
4 months ago Right now, it tries to arrange itself correctly according to the order provided in the description, but due to some bugs and special characters, the order is a little off.
You can check my proposal for the same.
shubham1206agra
commented
4 months ago One thing we need to change is the inclusion of RBR in the pinned / GBR report.
thebigshotsam
commented
4 months ago Pinned/GBR reports (pinnedAndGBRReports) and Draft reports (draftReports) are sorted alphabetically based on the displayName property.
pinnedAndGBRReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));
draftReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));For non-archived reports (nonArchivedReports), the sorting is based on the display mode. In default mode, the reports are sorted by the combination of the lastVisibleActionCreated date and displayName. In GSD mode, they are sorted alphabetically by displayName.
if (isInDefaultMode) {
nonArchivedReports.sort((a, b) => {
const compareDates = a?.lastVisibleActionCreated && b?.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
const compareDisplayNames = a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0;
return compareDates || compareDisplayNames;
});
} else {
nonArchivedReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));
}Similar to non-archived reports, archived reports (archivedReports) are sorted based on the display mode, either by the combination of lastVisibleActionCreated and displayName in default mode or alphabetically by displayName in GSD mode.
archivedReports.sort((a, b) => (a?.displayName && b?.displayName ? a.displayName.toLowerCase().localeCompare(b.displayName.toLowerCase()) : 0));The sorted groups are concatenated into a single array (LHNReports), and this array, along with the cache key, is stored in the cache.
const LHNReports = [...pinnedAndGBRReports, ...draftReports, ...nonArchivedReports, ...archivedReports].map((report) => report.reportID);
setWithLimit(reportIDsCache, cachedReportsKey, LHNReports);The sorting involves a combination of alphabetical sorting based on the displayName property and, in some cases, sorting by the lastVisibleActionCreated date. The sorting order depends on the display mode (default or GSD).
thebigshotsam
commented
4 months ago The current sorting logic in the Last Heard Navigation (LHN) feature doesn't handle non-alphanumeric characters appropriately when sorting alphabetically. The requirement is to ignore these characters during alphabetical sorting for both "Pinned/GBR Reports" and "Draft Reports."
pinnedAndGBRReports and draftReports in the sortLHNReports function.pinnedAndGBRReports.sort((a, b) => {
const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});
draftReports.sort((a, b) => {
const cleanA = a?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
const cleanB = b?.displayName?.replace(/[^0-9a-zA-Z]/g, '');
return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0;
});
// ... (previous code)// Sort each group of reports accordingly pinnedAndGBRReports.sort((a, b) => { const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, ''); const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, ''); return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0; });
draftReports.sort((a, b) => { const cleanA = a.displayName?.replace(/[^0-9a-zA-Z]/g, ''); const cleanB = b.displayName?.replace(/[^0-9a-zA-Z]/g, ''); return cleanA && cleanB ? cleanA.localeCompare(cleanB) : 0; });
// ... (remaining code)
// Sort the combined array based on specified criteria allReportsSorted.sort((a, b) => { const comparePinned = (b.isPinned ? 1 : 0) - (a.isPinned ? 1 : 0); const compareDisplayNames = a.displayName && b.displayName ? a.displayName.localeCompare(b.displayName) : 0; const compareDates = a.lastVisibleActionCreated && b.lastVisibleActionCreated ? compareStringDates(b.lastVisibleActionCreated, a.lastVisibleActionCreated) : 0;
// First, sort by pinned status, then by display names, and finally by dates in most recent mode
return comparePinned || compareDisplayNames || compareDates;});
// ... (remaining code)
## Benefits:
- This modification ensures that non-alphanumeric characters are ignored during the sorting of pinnedAndGBRReports and draftReports, aligning with the specified requirements. Additionally, the existing logic for GBR and RBR verification in the sorting process remains intact.
- The proposed changes will enhance the alphabetical sorting logic by ignoring non-alphanumeric characters, aligning with the specified requirements.
## Dependencies:
- Ensure compatibility and minimal impact on existing functionalities.Right now, it tries to arrange itself correctly according to the order provided in the description, but due to some bugs and special characters, the order is a little off.
Thanks for the feedbacks , So the current bugs are :
We need to use this inside
getOrderedReportIDsto get the RBR reports to get them alongside pinned and GBR reports.
@shubham1206agra Assuming that the code will looks like this code snippet, won't this code introduce any possible performance regression for HT account ?
const hasRoadIndicator = Object.keys(OptionsListUtils.getAllReportErrors(report, ReportActionsUtils.getAllReportActions(report.reportID)) ?? {}).length !== 0;
if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction) || hasRoadIndicator) {
pinnedAndGBRReports.push(report);
}Right now, it tries to arrange itself correctly according to the order provided in the description, but due to some bugs and special characters, the order is a little off.
Thanks for the feedbacks , So the current bugs are :
* RBR are not showing as pinned. * Sorting funtion does not ignore non alphabetical characters.We need to use this inside
getOrderedReportIDsto get the RBR reports to get them alongside pinned and GBR reports.@shubham1206agra Assuming that the code will looks like this code snippet, won't this code introduce any possible performance regression for HT account ?
const hasRoadIndicator = Object.keys(OptionsListUtils.getAllReportErrors(report, ReportActionsUtils.getAllReportActions(report.reportID)) ?? {}).length !== 0; if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction) || hasRoadIndicator) { pinnedAndGBRReports.push(report); }
I fear the same actually. But there's no other way to identify RBR in reports as errors is only contained in reportActions. @quinthar What's your opinion on this? Should we keep RBR reports as pinned or not do this (inclusion of RBR reports may become very slow as we have to iterate on every messages right now to get the RBR status of the report)?
quinthar
commented
4 months ago Ah, great question. However, I'm not sure I understand. Right now the client already correctly shows the RBR red dot in the LHN, it's just not sorted correctly. Given that the client is already doing whatever is necessary to get the red dot, why can't we just use that -- why would we need to do some new expensive calculation to determine it?
thebigshotsam
commented
4 months ago @quinthar, thanks for raising this point. Let's break down the approach:
It's mentioned that the client correctly displays the RBR red dot in the LHN, indicating that the information about RBR status is available client-side. If this is the case, we can leverage this existing client-side information in our server-side logic. The question is valid: why introduce a new expensive calculation if the client is already obtaining the necessary details?
Review Pinning Logic:
report.isPinned is accurately set for RBR reports, aligning with the client-side representation.Utilize Client-Side Information:
Address Sorting Issue:
// Inside the forEach loop where reports are processed// ... existing code ...
reportsToDisplay.forEach((report) => { // ... existing code ...
const isPinned = report.isPinned ?? false;
// Check if it's an RBR report and adjust isPinned accordingly if (ReportUtils.isRedBrickRoadReport(report)) { // Assuming RBR reports should be treated as pinned isPinned = true; }
const reportAction = ReportActionsUtils.getReportAction(report.parentReportID ?? '', report.parentReportActionID ?? ''); if (isPinned || ReportUtils.requiresAttentionFromCurrentUser(report, reportAction)) { pinnedAndGBRReports.push(report); } else if (report.hasDraft) { draftReports.push(report); } else if (ReportUtils.isArchivedRoom(report)) { archivedReports.push(report); } else { nonArchivedReports.push(report); } });
// ... existing code ...
### Client-Side vs. Server-Side Handling:
Given the existing functionality on the client side, it's worth exploring a solution that optimally integrates with this setup rather than introducing additional complexity. This can potentially lead to a more efficient and streamlined approach.Ah, great question. However, I'm not sure I understand. Right now the client already correctly shows the RBR red dot in the LHN, it's just not sorted correctly. Given that the client is already doing whatever is necessary to get the red dot, why can't we just use that -- why would we need to do some new expensive calculation to determine it?
Actually all the RBR calculation are deferred to later steps while rendering reports. I can adjust the steps to make calculation not redundant, but might still be a costly calculation.
Let me check if I can have some optimizations in the logic, and will let you know here.
youssef-lr
commented
4 months ago @shubham1206agra I looked into this, and it seems like we already call getAllReportErrors any time the LHN re-renders, if it's not affecting performance now then I think it's fine if we also call it when re-ordering reports, which doesn't happen as often as when the LHN re-renders.
Also, I think we should include checking for violations inside getAllReportsErorrs, you can check here how we extract the violation message. I think we'll probably need a new method to extract the messages as we can't use that hook outside a component. Also, only violations with type violation or warning should be taken into account here. (cc @lindboe as I see you're working on client side violations)
I can adjust the steps to make calculation not redundant
If there's a way to make that function somehow only triggered when its dependencies are changed, it would be great, but I'm not sure how feasible is that given that it lives outside components.
What do you think?
Edit: looks like there's a PR in the works that's adding a helper function to get violations
shubham1206agra
commented
4 months ago @youssef-lr Can you hold this on https://github.com/Expensify/App/pull/31448 in that case?
melvin-bot[bot]
commented
4 months ago @JmillsExpensify, @fedirjh Huh... This is 4 days overdue. Who can take care of this?
melvin-bot[bot]
commented
4 months ago @JmillsExpensify, @fedirjh Still overdue 6 days?! Let's take care of this!
quinthar
commented
4 months ago Looks like https://github.com/Expensify/App/pull/31448 was merged; taking off of hold. What's the next step here, who's doing it, and when will it be done?
quinthar
commented
3 months ago Bump, what's the next step here?
fedirjh
commented
3 months ago If there's a way to make that function somehow only triggered when its dependencies are changed, it would be great, but I'm not sure how feasible is that given that it lives outside components.
cc @youssef-lr @shubham1206agra I think we can have some kind of optimisation for this one. We can get an ids array of reports that have errors within the SidebarLinksData component then pass the array to the getOrderedReportIDs , we can wrap the new const with a memo as follows :
const reportsWithErrorsIds = useMemo(
() => {
const reportKeys = Object.keys(allReportActions)
return reportKeys.filter(
reportKey => {
const report = chatReports[reportKey.replace('reportActions_','report_')] ?? {};
const allReportsActions = allReportActions[reportKey].map(
reportAction => ({[reportAction.reportActionID]: reportAction})
);
return (OptionsListUtils.getAllReportErrors(
report,
allReportsActions
) ?? {}).length > 0
})
}
,[allReportActions, chatReports]
)For the violations, I think the merged PR already implemented the necessary changes.
shubham1206agra
commented
3 months ago @fedirjh Again here problem would be any new message would trigger the change in whole structure.
fedirjh
commented
3 months ago Again here problem would be any new message would trigger the change in whole structure.
@shubham1206agra I think we are already subscribing to allReportActions inside SidebarLinksData
We should just update the selector to optimize the re-rendering, we should check what are the necessary props for detecting the errors. By using a memo, we will avoid any extra re-rendering as the re-rendering is done only when the reportsWithErrorsIds changes.
fedirjh
commented
3 months ago cc @hannojg Would like to get your input on this issue, since you have implemented the LHN optimisation.
hannojg
commented
3 months ago Yeah sure, let me read through all it! Give me a moment to write an insight from my perspective
lindboe
commented
3 months ago Also, I think we should include checking for violations inside getAllReportsErorrs, you can check here how we extract the violation message. I think we'll probably need a new method to extract the messages as we can't use that hook outside a component. Also, only violations with type violation or warning should be taken into account here. (cc @lindboe as I see you're working on client side violations)
@cead22 I think this is something you need to comment on (and cc @cdanwards ). I think the plan we talked about, and are implementing in https://github.com/Expensify/App/issues/31411, and is in the violations design doc, is to NOT include violations in allReportErrors, but instead always append violations logic as an extra check in-line:
In src/libs/SidebarUtils.js we need to update this line with
result.brickRoadIndicator = !_.isEmpty(result.allReportErrors) || ReportUtils.transactionThreadHasViolations(report) ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';That way src/components/LHNOptionsList/OptionRowLHN.js will show the RBRIn order to show the reports with violations at the top of the LHN, In src/libs/SidebarUtils.js We’ll update this to return true if
Object.keys(result.allReportErrors ?? {}).length !== 0 || ReportUtils.transactionThreadHasViolations(report)
I'm not very familiar with the problem this ticket describes; are we going to run into problems with a conflicting approach here @youssef-lr ?
hannojg
commented
3 months ago We can get an ids array of reports that have errors …
Minor thing, but if we are already iterating over that array, let's use an object structure as return value. Later in getOrderedReportsID the lookup time for a report action will just be O(1)
… then pass the array to the getOrderedReportIDs
Sounds good to me!
we should check what are the necessary props for detecting the errors [and add that to the selector]
Yes, agreed.
As you @fedirjh pointed out, by following the existing scheme of implementation in SidebarLinksData we should be guarded against performance pitfalls 😊
youssef-lr
commented
3 months ago @fedirjh nice yeah, I was thinking about memoizing report errors as well. I think we can take that a step further and memoize it in OptionRowLHNData with these dependencies [report.errors, report.errorFields, reportActions] (if eslint won't complain, or we can eslint-disable-next-line react-hooks/exhaustive-deps).
For the report we only care about those 2 props to decide if it has an error. This way the function (which iterates over every report action) will not be triggered if an irrelevant report prop changes.
We can pass the memozied reportErrors keyed by reportID to SidebarUtils.getOptionData here then inside getAllReportErrors we can get the errors by accessing the object:
result.allReportErrors = allReportErrors[report.reportID] as OnyxCommon.Errors;
We should just update the selector to optimize the re-rendering
I think for reportActions we only care about theerrors prop. Maybe we can have the selector only return that?
Curious for your thoughts @fedirjh @hannojg
youssef-lr
commented
3 months ago I'm not very familiar with the problem this ticket describes; are we going to run into problems with a conflicting approach here @youssef-lr ?
@lindboe this ticket is about pinning reports that have an RBR, the RBR could be due to an API error, a smartscan error, or a violation. I'm down to not having the violations included in getAllReportErrors, whatever you guys decide. What's nice about violations is they have their own Onyx key so there's no performance issues here.
I think the plan we talked about, and are implementing in https://github.com/Expensify/App/issues/31411, and is in the violations design doc, is to NOT include violations in allReportErrors, but instead always append violations logic as an extra check in-line
Yeah this looks good too!
fedirjh
commented
3 months ago I think we can take that a step further and memoize it in
OptionRowLHNDatawith these dependencies[report.errors, report.errorFields, reportActions]
@youssef-lr Why do we need to do that? The report ordering logic is done in SidebarLinksData which has a higher hierarchy order than OptionRowLHNData.
fedirjh
commented
3 months ago I think for reportActions we only care about theerrors prop. Maybe we can have the selector only return that?
I believe there are additional properties that need to be added to the selector. For instance, to verify whether the reportActions contain a smartscan error, we should retrieve the actionName and originalMessage.type.
cead22
commented
3 months ago I think the plan we talked about, and are implementing in #31411, and is in the violations design doc, is to NOT include violations in
allReportErrors, but instead always append violations logic as an extra check in-line:
@lindboe I think you can proceed as planned there, and the refactor to put violations in allReportErrors as part of this issue seems like it should work fine
luacmartins
commented
3 months ago I can help move this forward. @fedirjh would you mind giving us a summary of the current proposal we're exploring and the many optimizations we mentioned in this thread?
fedirjh
commented
3 months ago @luacmartins , we are in agreement with this proposal shared by @shubham1206agra, and all expected outcomes have been achieved. However, we have some concerns about performance, particularly in addressing the following requirement:
Verify that RBR chats are showing as pinned
The process of retrieving all errors for a specific report using getAllReportErrors requires iterating over all its reportActions, potentially posing performance challenges in HT accounts.
it seems like we already call
getAllReportErrorsany time the LHN re-renders, if it's not affecting performance now then I think it's fine if we also call it when re-ordering reports, which doesn't happen as often as when the LHN re-renders.
@youssef-lr has observed that we are already fetching all errors for every report. However, it's important to note that the current implementation separates the logic for ordering reports and displaying the RBR. The report ordering logic is housed in SidebarLinksData, positioned at a higher hierarchy order than OptionRowLHNData, where the RBR display logic is implemented. Check this comment
Each instance of OptionRowLHNData operates independently within the sidebar. Every row has unique dependencies tied to the report. Currently, any modifications to one report trigger a re-render solely for the corresponding row.
The challenges we face are as follows:
SidebarLinksData to make report ordering work as expected, this will involve:
props within the reportsActions collection that are required for this task (explained in this comment). reports and all reportActions at the same time.reportAction prompts :
SidebarLinksData, subsequently triggering a re-render for all instances of OptionRowLHNData. The proposed solution outlined in this comment introduces optimizations by obtaining a list with IDs of reports with errors to avoid unnecessary re-renders if the list remains unchanged. However, the costly calculation will persist with each reportAction change.
luacmartins
commented
3 months ago @fedirjh thanks for the detailed analysis and highlighting the performance concerns.
Given that we're in agreement on @shubham1206agra's proposal, maybe the next step would be for @shubham1206agra to create a POC and time it on a HT account. That will help us confirm that we will indeed have performance degradation and how much of an issue that'll be. From there, we can explore solutions to mitigate any performance issues.
@hannojg would the reassure performance tests be a good way to measure the impact on SidebarLinksData render time?
mountiny
commented
3 months ago @hannojg would the reassure performance tests be a good way to measure the impact on SidebarLinksData render time?
We have a ressure tests for LHN but I dont think they do specifically mock pinned and GBR/RBR chats https://github.com/Expensify/App/blob/6cfdd6f47fe5f932b70f440104accffea586ef60/tests/perf-test/SidebarLinks.perf-test.js#L63-L77
@OlimpiaZurek Do you think we could add a scenario for this case?
luacmartins
commented
3 months ago Hmm I think at this point we just need to measure the performance rendering the LHN with the new logic. Asserting the correct order would be done in a separate test I imagine.
hannojg
commented
3 months ago I just checked the reassure tests and it seems that they create 10k mock action items, which seems to be a good setup to test the performance. In addition to that it should be covered by the e2e performance regression tests. So I think in terms of tests we should be on a safe ground.
luacmartins
commented
3 months ago Sounds good, so I think we can go ahead with a POC and see what the tests reveal.
mountiny
commented
3 months ago Hmm I think at this point we just need to measure the performance rendering the LHN with the new logic. Asserting the correct order would be done in a separate test I imagine.
Fair, what I meant is that the mocks probably do not add any variation to the type of reports which could make the sorting faster than real usecase with different report types, archived chats, GBR, hidden reports etc
luacmartins
commented
3 months ago Yea, I'm sure the real use case will be different and most likely slower. I think we start simple with the existing tests and then move on to more complex cases.
Problem: When the LHN is in "focus" mode, its sorting should be:
However, it doesn't seem to be. Here's from @iwiznia:
Solution: Review the LHN logic for sorting in focus mode to understand what it's currently doing, and verify it's correct. The correct behavior should be:
#focus mode:
most recent mode:
Additionally:
So, for this issue please:
You up to the challenge? Let's see those proposals!!
Upwork Automation - Do Not Edit