Skip to content
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

Jump to bottom

fix: improve getDisplayNameForParticipant performance #45907

Merged
merged 3 commits into from
Jul 22, 2024
Merged

fix: improve getDisplayNameForParticipant performance #45907

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f29352c
fix: improve getDisplayNameForParticipant performance
OlimpiaZurek Jul 22, 2024
1bb4618
lint fix
OlimpiaZurek Jul 22, 2024
82b5016
changes after cr
OlimpiaZurek Jul 22, 2024
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
19 changes: 17 additions & 2 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1859,6 +1859,9 @@ function getPersonalDetailsForAccountID(accountID: number): Partial<PersonalDeta
}

const hiddenTranslation = Localize.translateLocal('common.hidden');

const phoneNumberCache: Record<string, string> = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other functions we could use this cache in this Utils file? Lets see if we can use it there in next PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way forward for this would be to reuse the global cache proposed by Kacper in the other PR so we can swap every usage with this cached one in one go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to be the only place in this file where the formatPhoneNumber is used


/**
* Get the displayName for a single report participant.
*/
Expand All @@ -1868,8 +1871,20 @@ function getDisplayNameForParticipant(accountID?: number, shouldUseShortForm = f
}

const personalDetails = getPersonalDetailsForAccountID(accountID);
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const formattedLogin = LocalePhoneNumber.formatPhoneNumber(personalDetails.login || '');
if (!personalDetails) {
return '';
}
Comment on lines +1874 to +1876
Copy link
Contributor

@kacper-mikolajczak kacper-mikolajczak Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this early return was added in the PR. What is its impact on the function flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to avoid unnecessary calculations and function calls if personalDetails is not available. I think this minor optimization may help in scenarios where the function may be called frequently.


const login = personalDetails.login ?? '';

// Check if the phone number is already cached
let formattedLogin = phoneNumberCache[login];
if (!formattedLogin) {
formattedLogin = LocalePhoneNumber.formatPhoneNumber(login);
// Store the formatted phone number in the cache
phoneNumberCache[login] = formattedLogin;
}

// This is to check if account is an invite/optimistically created one
// and prevent from falling back to 'Hidden', so a correct value is shown
// when searching for a new user
Expand Down
Loading