Skip to content

Optimize report ordering in the LHN #10784

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
tgolen merged 50 commits into from
Sep 13, 2022
Merged

Optimize report ordering in the LHN #10784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
fed705a
Remove the last pieces out of state
tgolen Aug 25, 2022
44a2ff6
Simplify the check for unread reports
tgolen Aug 27, 2022
57a4c2e
Simplify the map of unread reports
tgolen Aug 27, 2022
4b40492
Correct outdated comment
tgolen Aug 27, 2022
00ebe57
Rename method to be more apparent
tgolen Aug 27, 2022
103b126
Access filtered report options direction from render
tgolen Aug 27, 2022
bb57fb0
Remove the import of unused library
tgolen Aug 27, 2022
f3a7fa0
Fix a prop warning
tgolen Aug 27, 2022
35cfd23
Add some performance timing for filtering the reports
tgolen Aug 27, 2022
8e8ba1f
Rename method to be more clear
tgolen Aug 27, 2022
6a26a03
Remove mobile preoptimization
tgolen Aug 27, 2022
d973776
Correct the language in a comment
tgolen Aug 27, 2022
b770d0c
Remove old code
tgolen Aug 30, 2022
8a6ea06
Merge branch 'main' into tgolen-remove-deriveState
tgolen Aug 30, 2022
9626972
Rename method to make more sense and add comments
tgolen Aug 30, 2022
6091608
Add comments to the logic for ordering reports
tgolen Aug 30, 2022
56d6d34
Add a todo
tgolen Aug 30, 2022
853f8b8
Merge branch 'main' into tgolen-optimize-sidebarordering
tgolen Aug 31, 2022
44e258c
Fix proptype warnings
tgolen Aug 31, 2022
bd05cec
Remove unnecessary binding
tgolen Sep 1, 2022
475f0ed
Remove debug code that is not needed
tgolen Sep 1, 2022
80615c9
Fix proptype warning
tgolen Sep 1, 2022
de15223
Don't cast activeReportID to string
tgolen Sep 1, 2022
8512aa0
Remove all data processing
tgolen Sep 1, 2022
ee56ec4
Add report actions to the propTypes
tgolen Sep 1, 2022
e7f7b33
WIP on profiling what makes things rerender when switching reports
tgolen Sep 1, 2022
8057f21
Fix proptype warning
tgolen Sep 1, 2022
ff49948
WIP optimizing create option
tgolen Sep 1, 2022
7c51a86
WIP optimizing create option
tgolen Sep 1, 2022
fd60c45
WIP optimizing create option
tgolen Sep 1, 2022
9fad973
Finished optimizing createOption
tgolen Sep 1, 2022
ceb2ea9
Remove memoization and imports
tgolen Sep 1, 2022
2ef3622
Memoize getting the option list items
tgolen Sep 1, 2022
9ceb909
Fix lint issues
tgolen Sep 1, 2022
25825d0
Merge branch 'main' into tgolen-optimize-sidebarordering
tgolen Sep 5, 2022
6eb03cf
Remove unused HOC
tgolen Sep 5, 2022
42ece5b
Add error boundary to rendered component in tests
tgolen Sep 5, 2022
66d147d
Add an early return to protect against null reports
tgolen Sep 5, 2022
d03e6fb
Add a method comment about performance
tgolen Sep 5, 2022
47aa0b8
Keep policy name as a string and simplify the return
tgolen Sep 5, 2022
99532c2
Simplify the return statement
tgolen Sep 5, 2022
988e7c6
Fix the check for unread reports
tgolen Sep 5, 2022
4036325
Update src/libs/OptionsListUtils.js
tgolen Sep 6, 2022
34a60c9
Merge branch 'main' into tgolen-optimize-sidebarordering
tgolen Sep 8, 2022
a41a30a
Fix the active report equality check
tgolen Sep 8, 2022
6b87f56
Fix the active report equality check
tgolen Sep 8, 2022
629c4c4
Add a comment about why we use an error boundary
tgolen Sep 8, 2022
55bd67f
Fix return data type
tgolen Sep 8, 2022
84b1be4
Remove an unnecessary string cast
tgolen Sep 8, 2022
5ca3b5e
Add JSDocs
tgolen Sep 8, 2022
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
1 change: 1 addition & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ const CONST = {
SWITCH_REPORT: 'switch_report',
SIDEBAR_LOADED: 'sidebar_loaded',
PERSONAL_DETAILS_FORMATTED: 'personal_details_formatted',
SIDEBAR_LINKS_FILTER_REPORTS: 'sidebar_links_filter_reports',
COLD: 'cold',
REPORT_ACTION_ITEM_LAYOUT_DEBOUNCE_TIME: 1500,
TOOLTIP_SENSE: 1000,
Expand Down
10 changes: 8 additions & 2 deletions src/components/Tooltip/TooltipRenderedOnPageBody.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ const propTypes = {

/** Any additional amount to manually adjust the horizontal position of the tooltip.
A positive value shifts the tooltip to the right, and a negative value shifts it to the left. */
shiftHorizontal: PropTypes.number.isRequired,
shiftHorizontal: PropTypes.number,

/** Any additional amount to manually adjust the vertical position of the tooltip.
A positive value shifts the tooltip down, and a negative value shifts it up. */
shiftVertical: PropTypes.number.isRequired,
shiftVertical: PropTypes.number,

/** Text to be shown in the tooltip */
text: PropTypes.string.isRequired,
Expand All @@ -43,6 +43,11 @@ const propTypes = {
numberOfLines: PropTypes.number.isRequired,
};

const defaultProps = {
shiftHorizontal: 0,
shiftVertical: 0,
};

// Props will change frequently.
// On every tooltip hover, we update the position in state which will result in re-rendering.
// We also update the state on layout changes which will be triggered often.
Expand Down Expand Up @@ -132,5 +137,6 @@ class TooltipRenderedOnPageBody extends React.PureComponent {
}

TooltipRenderedOnPageBody.propTypes = propTypes;
TooltipRenderedOnPageBody.defaultProps = defaultProps;

export default TooltipRenderedOnPageBody;
244 changes: 154 additions & 90 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import _ from 'underscore';
import Onyx from 'react-native-onyx';
import lodashGet from 'lodash/get';
import lodashOrderBy from 'lodash/orderBy';
import memoizeOne from 'memoize-one';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../ONYXKEYS';
import CONST from '../CONST';
Expand Down Expand Up @@ -143,39 +142,64 @@ function getParticipantNames(personalDetailList) {
return participantNames;
}

/**
* A very optimized method to remove unique items from an array.
* Taken from https://stackoverflow.com/a/9229821/9114791
*
* @param {Array} items
* @returns {Array}
*/
function uniqFast(items) {
Comment thread
roryabraham marked this conversation as resolved.
const seenItems = {};
const result = [];
let j = 0;
for (let i = 0; i < items.length; i++) {
const item = items[i];
if (seenItems[item] !== 1) {
seenItems[item] = 1;
result[j++] = item;

@marcaaron marcaaron Sep 5, 2022

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.

Wait it is slower to push an item into an array than do this? 🤯

@tgolen tgolen Sep 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the general answer is "yes, it's faster", but there are differences between JS engines which make the testing somewhat unreliable. Generally, push() does some extra bits of processing from the base JS object that arrays extend from, whereas using an index approach bypassing that little bit of overhead.

}
}
return result;
}

/**
* Returns a string with all relevant search terms.
* Default should be serachable by policy/domain name but not by participants.
*
* This method must be incredibly performant. It was found to be a big performance bottleneck
* when dealing with accounts that have thousands of reports. For loops are more efficient than _.each
* Array.prototype.push.apply is faster than using the spread operator, and concat() is faster than push().
*
* @param {Object} report
* @param {String} reportName
* @param {Array} personalDetailList
* @param {Boolean} isChatRoomOrPolicyExpenseChat
* @return {String}
*/
function getSearchText(report, reportName, personalDetailList, isChatRoomOrPolicyExpenseChat) {
const searchTerms = [];
let searchTerms = [];

if (!isChatRoomOrPolicyExpenseChat) {
_.each(personalDetailList, (personalDetail) => {
searchTerms.push(personalDetail.displayName);
searchTerms.push(personalDetail.login.replace(/\./g, ''));
});
for (let i = 0; i < personalDetailList.length; i++) {

@roryabraham roryabraham Sep 5, 2022

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.

Maybe we should leave a note explaining why _.each is not here

@tgolen tgolen Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment about it in the method docs, because I think it should also explain why spread operators aren't being used.

const personalDetail = personalDetailList[i];
searchTerms = searchTerms.concat([personalDetail.displayName, personalDetail.login.replace(/\./g, '')]);
}
}
if (report) {
searchTerms.push(...reportName);
searchTerms.push(..._.map(reportName.split(','), name => name.trim()));
Array.prototype.push.apply(searchTerms, reportName.split(''));

@luacmartins luacmartins Sep 5, 2022

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.

Maybe add a comment to clarify why we are using Array.prototype.push.apply so people don't move back to array.push?

@marcaaron marcaaron Sep 5, 2022

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.

I'm curious about the explanation and +1

Array.prototype.push.apply(searchTerms, reportName.split(','));

if (isChatRoomOrPolicyExpenseChat) {
const chatRoomSubtitle = ReportUtils.getChatRoomSubtitle(report, policies);
searchTerms.push(...chatRoomSubtitle);
searchTerms.push(..._.map(chatRoomSubtitle.split(','), name => name.trim()));
Array.prototype.push.apply(searchTerms, chatRoomSubtitle.split(''));
Array.prototype.push.apply(searchTerms, chatRoomSubtitle.split(','));
} else {
searchTerms.push(...report.participants);
searchTerms = searchTerms.concat(report.participants);
}
}

return _.unique(searchTerms).join(' ');
return uniqFast(searchTerms).join(' ');
}

/**
Expand Down Expand Up @@ -217,80 +241,118 @@ function createOption(logins, personalDetails, report, reportActions = {}, {
showChatPreviewLine = false,
forcePolicyNamePreview = false,
}) {
const isChatRoom = ReportUtils.isChatRoom(report);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
const result = {

@marcaaron marcaaron Sep 5, 2022

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.

It's hard for me to tell what is style and what is an improvement. Is there some benefit to declaring the object with all the properties it might have instead of assigning them later?

@tgolen tgolen Sep 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My reason for this change is that before, 100% of the logic was running regardless if there was a report or not. There is logic for reports AND personal details but for the LHN, we only need the logic for the reports. By being more strategic with the if statements, it now means that it only runs the minimum necessary logic depending on if there is a report or not.

@marcaaron marcaaron Sep 9, 2022

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.

That sounds intuitive to me. Mostly asking as I'm curious which of these changes had the biggest impact on performance (for my own understanding).

@tgolen tgolen Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I could have taken better notes while doing this, but in the end, I don't think there are hugely practical takeaways. At large scale, ANY piece of code can become inefficient, and the same thing that fixes one bottleneck isn't going to fix all bottle necks and shouldn't be used everywhere just for the sake of performance (like uniqFast()).

I think the better takeaway for anyone would be to learn how to do performance testing. This is something that I could do a LOU about because it seems like very few of our engineers are familiar with how to do this.

Just for a quick sample though, here were the notes that I did take:

getSearchText
before 59.8ms
after 39.2ms

createOptions
before 117.2ms
after 115ms
  • those times are the total amount of time those methods took during a report switch (which is like ~8 re-renders of the LHN)

The changes to getSearchText() were to stop using .push() and _.unique()

text: null,
alternateText: null,
brickRoadIndicator: null,
icons: null,
tooltipText: null,
ownerEmail: null,
subtitle: null,
participantsList: null,
login: null,
reportID: null,
phoneNumber: null,
payPalMeAddress: null,
isUnread: null,
hasDraftComment: false,
keyForList: null,
searchText: null,
isDefaultRoom: false,
isPinned: false,
hasOutstandingIOU: false,
iouReportID: null,
isIOUReportOwner: null,
iouReportAmount: 0,
isChatRoom: false,
isArchivedRoom: false,
shouldShowSubscript: false,
isPolicyExpenseChat: false,
};

const personalDetailMap = getPersonalDetailsForLogins(logins, personalDetails);
const personalDetailList = _.values(personalDetailMap);
const isArchivedRoom = ReportUtils.isArchivedRoom(report);
const isDefaultRoom = ReportUtils.isDefaultRoom(report);

@luacmartins luacmartins Sep 5, 2022

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.

Should we initialize isDefaultRoom in results too?

@tgolen tgolen Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes! This came from me solving a merge conflict. isDefaultRoom didn't exist when I started refactoring so it got kind of left out.

const hasMultipleParticipants = personalDetailList.length > 1 || isChatRoom || isPolicyExpenseChat;
const personalDetail = personalDetailList[0];
const hasOutstandingIOU = lodashGet(report, 'hasOutstandingIOU', false);
const iouReport = hasOutstandingIOU
? lodashGet(iouReports, `${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`, {})
: {};

const lastActorDetails = report ? _.find(personalDetailList, {login: report.lastActorEmail}) : null;
const lastMessageTextFromReport = ReportUtils.isReportMessageAttachment({text: lodashGet(report, 'lastMessageText', ''), html: lodashGet(report, 'lastMessageHtml', '')})
? `[${Localize.translateLocal('common.attachment')}]`
: Str.htmlDecode(lodashGet(report, 'lastMessageText', ''));
let lastMessageText = report && hasMultipleParticipants && lastActorDetails
? `${lastActorDetails.displayName}: `
: '';
lastMessageText += report ? lastMessageTextFromReport : '';

if (isPolicyExpenseChat && isArchivedRoom) {
const archiveReason = lodashGet(lastReportActions[report.reportID], 'originalMessage.reason', CONST.REPORT.ARCHIVE_REASON.DEFAULT);
lastMessageText = Localize.translate(preferredLocale, `reportArchiveReasons.${archiveReason}`, {
displayName: lodashGet(lastActorDetails, 'displayName', report.lastActorEmail),
policyName: ReportUtils.getPolicyName(report, policies),
});
}
let hasMultipleParticipants = personalDetailList.length > 1;
let subtitle;

const tooltipText = ReportUtils.getReportParticipantsTitle(lodashGet(report, ['participants'], []));
const subtitle = ReportUtils.getChatRoomSubtitle(report, policies);
const reportName = ReportUtils.getReportName(report, personalDetailMap, policies);
let alternateText;
if (isChatRoom || isPolicyExpenseChat) {
alternateText = (showChatPreviewLine && !forcePolicyNamePreview && lastMessageText)
? lastMessageText
: subtitle;
if (report) {
result.isChatRoom = ReportUtils.isChatRoom(report);
result.isDefaultRoom = ReportUtils.isDefaultRoom(report);
result.isArchivedRoom = ReportUtils.isArchivedRoom(report);
result.isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
result.shouldShowSubscript = result.isPolicyExpenseChat && !report.isOwnPolicyExpenseChat && !result.isArchivedRoom;

@roryabraham roryabraham Sep 5, 2022

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.

NAB because I know you didn't introduce this variable name, but shouldShowSubscript is not very descriptive / the context for it is lost in this file.

@tgolen tgolen Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It looks like it could maybe be renamed to shouldShowSubscriptAvatar, which is a small avatar that is below the main line of text?

result.brickRoadIndicator = getBrickRoadIndicatorStatusForReport(report, reportActions);
result.ownerEmail = report.ownerEmail;
result.reportID = report.reportID;
result.isUnread = ReportUtils.isUnread(report);
result.hasDraftComment = report.hasDraft;
result.isPinned = report.isPinned;
result.iouReportID = report.iouReportID;
result.keyForList = String(report.reportID);
result.tooltipText = ReportUtils.getReportParticipantsTitle(report.participants || []);
result.hasOutstandingIOU = report.hasOutstandingIOU;

hasMultipleParticipants = personalDetailList.length > 1 || result.isChatRoom || result.isPolicyExpenseChat;
subtitle = ReportUtils.getChatRoomSubtitle(report, policies);

let lastMessageTextFromReport = '';
if (ReportUtils.isReportMessageAttachment({text: report.lastMessageText, html: report.lastMessageHtml})) {
lastMessageTextFromReport = `[${Localize.translateLocal('common.attachment')}]`;
} else {
lastMessageTextFromReport = Str.htmlDecode(report ? report.lastMessageText : '');
}

const lastActorDetails = personalDetailMap[report.lastActorEmail] || null;
let lastMessageText = hasMultipleParticipants && lastActorDetails
? `${lastActorDetails.displayName}: `
: '';
lastMessageText += report ? lastMessageTextFromReport : '';

if (result.isPolicyExpenseChat && result.isArchivedRoom) {
const archiveReason = (lastReportActions[report.reportID] && lastReportActions[report.reportID].originalMessage && lastReportActions[report.reportID].originalMessage.reason)
|| CONST.REPORT.ARCHIVE_REASON.DEFAULT;
lastMessageText = Localize.translate(preferredLocale, `reportArchiveReasons.${archiveReason}`, {
displayName: archiveReason.displayName || report.lastActorEmail,
policyName: ReportUtils.getPolicyName(report, policies),
});
}

if (result.isChatRoom || result.isPolicyExpenseChat) {
result.alternateText = (showChatPreviewLine && !forcePolicyNamePreview && lastMessageText)
? lastMessageText
: subtitle;
} else {
result.alternateText = (showChatPreviewLine && lastMessageText)
? lastMessageText
: Str.removeSMSDomain(personalDetail.login);
}
} else {
alternateText = (showChatPreviewLine && lastMessageText)
? lastMessageText
: Str.removeSMSDomain(personalDetail.login);
result.keyForList = personalDetail.login;
}
return {
text: reportName,
alternateText,
brickRoadIndicator: getBrickRoadIndicatorStatusForReport(report, reportActions),
icons: ReportUtils.getIcons(report, personalDetails, policies, lodashGet(personalDetail, ['avatar'])),
tooltipText,
ownerEmail: lodashGet(report, ['ownerEmail']),
subtitle,
participantsList: personalDetailList,

// It doesn't make sense to provide a login in the case of a report with multiple participants since
// there isn't any one single login to refer to for a report.
login: !hasMultipleParticipants ? personalDetail.login : null,
reportID: report ? report.reportID : null,
phoneNumber: !hasMultipleParticipants ? personalDetail.phoneNumber : null,
payPalMeAddress: !hasMultipleParticipants ? personalDetail.payPalMeAddress : null,
isUnread: ReportUtils.isUnread(report),
hasDraftComment: lodashGet(report, 'hasDraft', false),
keyForList: report ? String(report.reportID) : personalDetail.login,
searchText: getSearchText(report, reportName, personalDetailList, isChatRoom || isPolicyExpenseChat),
isPinned: lodashGet(report, 'isPinned', false),
hasOutstandingIOU,
iouReportID: lodashGet(report, 'iouReportID'),
isIOUReportOwner: lodashGet(iouReport, 'ownerEmail', '') === currentUserLogin,
iouReportAmount: lodashGet(iouReport, 'total', 0),
isChatRoom,
isArchivedRoom,
isDefaultRoom,
shouldShowSubscript: isPolicyExpenseChat && !report.isOwnPolicyExpenseChat && !isArchivedRoom,
isPolicyExpenseChat,
};

if (result.hasOutstandingIOU) {
const iouReport = iouReports[`${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`] || null;
if (iouReport) {
result.isIOUReportOwner = iouReport.ownerEmail === currentUserLogin;
result.iouReportAmount = iouReport.total;
}
}

if (!hasMultipleParticipants) {
result.login = personalDetail.login;
result.phoneNumber = personalDetail.phoneNumber;
result.payPalMeAddress = personalDetail.payPalMeAddress;
}

const reportName = ReportUtils.getReportName(report, personalDetailMap, policies);
result.text = reportName;
result.subtitle = subtitle;
result.participantsList = personalDetailList;
result.icons = ReportUtils.getIcons(report, personalDetails, policies, personalDetail.avatar);
result.searchText = getSearchText(report, reportName, personalDetailList, result.isChatRoom || result.isPolicyExpenseChat);

return result;
}

/**
Expand Down Expand Up @@ -351,7 +413,7 @@ function isCurrentUser(userDetails) {
*
* @param {Object} reports
* @param {Object} personalDetails
* @param {Number} activeReportID
* @param {String} activeReportID

@marcaaron marcaaron Sep 5, 2022

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.

When did reportID become strings and are all reportID now strings in JS?

@tgolen tgolen Sep 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is being discussed in Slack to make sure we're getting to the right conclusion: https://expensify.slack.com/archives/C02HWMSMZEC/p1662382397049269

Regardless, we're not treating them consistently everywhere yet, but we want to!

* @param {Object} options
* @returns {Object}
* @private
Expand Down Expand Up @@ -404,20 +466,24 @@ function getOptions(reports, personalDetails, activeReportID, {

const allReportOptions = [];
_.each(orderedReports, (report) => {
if (!report) {
return;
}
const isChatRoom = ReportUtils.isChatRoom(report);
const isDefaultRoom = ReportUtils.isDefaultRoom(report);
const isPolicyExpenseChat = ReportUtils.isPolicyExpenseChat(report);
const logins = lodashGet(report, ['participants'], []);
const logins = report.participants || [];

@roryabraham roryabraham Sep 5, 2022

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.

Do we know for sure that report is not null or undefined?

@luacmartins luacmartins Sep 5, 2022

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.

report is defined as the _.each iterator, so I think it should always be defined

@roryabraham roryabraham Sep 5, 2022

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.

That was my first thought, but what if you have [{ // some real object }, null, { // some real object }] ? No idea if that can actually happen in this case though:

image

@luacmartins luacmartins Sep 5, 2022

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.

Hmm that's a good point. I'm not sure either 🤔

@tgolen tgolen Sep 5, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there are a few things here... _.each doesn't protect against null members in a collection, so it's possible for report to be null.

This particular line of code (and several lines down below it) need to have null protection. There are two solutions around this typically...

  1. Use _.compact for arrays to clear out falsy values or use _.pluck for objects
  2. Add an early return if report is null

I think I'm going to do number 2 and then remove all the report null checks


// Report data can sometimes be incomplete. If we have no logins or reportID then we will skip this entry.
const shouldFilterNoParticipants = _.isEmpty(logins) && !isChatRoom && !isDefaultRoom && !isPolicyExpenseChat;
if (!report || !report.reportID || shouldFilterNoParticipants) {
if (!report.reportID || shouldFilterNoParticipants) {

@marcaaron marcaaron Sep 5, 2022

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.

Should we do this sooner on line 469?

if (!report || !report.reportID) {
    return;
}

@tgolen tgolen Sep 6, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We probably could. I'm honestly a bit perplexed by this and the comment. Since reports are indexed in Onyx by their ID, I don't think it should ever be possible for a report to not have an ID. If it is possible, I think it is a bug that we should track down and fix. So, I'd be more willing to either remove this check entirely, or at least add a log if we detect there is no reportID.

@marcaaron marcaaron Sep 9, 2022

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.

Since reports are indexed in Onyx by their ID, I don't think it should ever be possible for a report to not have an ID. If it is possible, I think it is a bug that we should track down and fix.

I'm feeling cautious about removing that since it looks like we use the "report without a reportID" as a kind of "hook" to fetch the report. Which was the expected behavior at one point. But also looks like there's maybe some further cleanup to do here:

https://github.com/Expensify/App/blame/c9b41bff8b728073db85f38782b591a0fd4a9772/src/libs/actions/Report.js#L1135-L1137

We could solve this by sending the report and comment when a new message is added.

@marcaaron marcaaron Sep 9, 2022

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.

oh wait scratch everything I just said haha that code looks for report.reportID && !reportName (but we should still clean that up)

@marcaaron marcaaron Sep 9, 2022

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.

Still feeling cautious and agree we should try to figure out if/why a report object can be merged with no reportID (and a log would help)

@tgolen tgolen Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

About the best I can offer in that case then is to add a log somewhere to detect if a report ID is missing. I don't think this code is the right place for it though. I would add it somewhere in Onyx.update() probably? Something like:

if (key starts with "report_" and value has no reportID) {
    Log to the server
    return early
}

@tgolen tgolen Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but that doesn't really work because of doing something like Onyx.merge('report_123', {reportName: 'blah'})

@tgolen tgolen Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I kind of just feel here like we need to trust that we would never have a report without a reportID. And if it happens, we will find out about it in buggy behavior and crashes.

return;
}

const hasDraftComment = lodashGet(report, 'hasDraft', false);
const iouReportOwner = lodashGet(report, 'hasOutstandingIOU', false)
? lodashGet(iouReports, [`${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`, 'ownerEmail'], '')
const hasDraftComment = report.hasDraft || false;
const iouReport = report.iouReportID && iouReports[`${ONYXKEYS.COLLECTION.REPORT_IOUS}${report.iouReportID}`];
const iouReportOwner = report.hasOutstandingIOU && iouReport
? iouReport.ownerEmail
: '';

const reportContainsIOUDebt = iouReportOwner && iouReportOwner !== currentUserLogin;
Expand All @@ -431,7 +497,7 @@ function getOptions(reports, personalDetails, activeReportID, {
const shouldFilterReportIfRead = hideReadReports && !ReportUtils.isUnread(report);
const shouldFilterReport = shouldFilterReportIfEmpty || shouldFilterReportIfRead;

if (report.reportID !== activeReportID
if (report.reportID.toString() !== activeReportID
&& (!report.isPinned || isDefaultRoom)
&& !hasDraftComment
&& shouldFilterReport
Expand Down Expand Up @@ -762,7 +828,7 @@ function getMemberInviteOptions(
* @param {Object} reportActions
* @returns {Object}
*/
function calculateSidebarOptions(reports, personalDetails, activeReportID, priorityMode, betas, reportActions) {
function getSidebarOptions(reports, personalDetails, activeReportID, priorityMode, betas, reportActions) {
let sideBarOptions = {
prioritizeIOUDebts: true,
prioritizeReportsWithDraftComments: true,
Expand All @@ -786,8 +852,6 @@ function calculateSidebarOptions(reports, personalDetails, activeReportID, prior
});
}

const getSidebarOptions = memoizeOne(calculateSidebarOptions);

/**
* Helper method that returns the text to be used for the header's message and title (if any)
*
Expand Down
Loading