-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix: Avatar changes when from offline to online #32090
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
bcb57a3
fix: Avatar changes when from offline to online
anyongjitiger 7e9768e
fix: Avatar changes when from offline to online
anyongjitiger 3d2086a
Merge remote-tracking branch 'upstream/main'
anyongjitiger 132199c
Merge remote-tracking branch 'upstream/main'
anyongjitiger 61c4ad0
Merge remote-tracking branch 'upstream/main'
anyongjitiger a9d8b7c
Merge remote-tracking branch 'upstream/main'
anyongjitiger 533d90f
fix: remove redundant validation
anyongjitiger 87fb8e7
fix: use newDot avatar
anyongjitiger 0bac8d7
Merge remote-tracking branch 'upstream/main'
anyongjitiger 874fe34
merge branch expensify main
anyongjitiger f66f6a1
fix: remove param in getDefaultAvatarURL
anyongjitiger 674d060
merge
anyongjitiger fe5a7d9
fix: prettier
anyongjitiger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,13 @@ | ||
import Str from 'expensify-common/lib/str'; | ||
import _ from 'lodash'; | ||
import Onyx, {OnyxEntry} from 'react-native-onyx'; | ||
import {SvgProps} from 'react-native-svg'; | ||
import {ValueOf} from 'type-fest'; | ||
import * as defaultAvatars from '@components/Icon/DefaultAvatars'; | ||
import {ConciergeAvatar, FallbackAvatar} from '@components/Icon/Expensicons'; | ||
import CONST from '@src/CONST'; | ||
import ONYXKEYS from '@src/ONYXKEYS'; | ||
import {PersonalDetails} from '@src/types/onyx'; | ||
import Login from '@src/types/onyx/Login'; | ||
import hashCode from './hashCode'; | ||
|
||
|
@@ -13,6 +17,12 @@ type AvatarSource = React.FC<SvgProps> | string; | |
|
||
type LoginListIndicator = ValueOf<typeof CONST.BRICK_ROAD_INDICATOR_STATUS> | ''; | ||
|
||
let allPersonalDetails: OnyxEntry<Record<string, PersonalDetails>>; | ||
Onyx.connect({ | ||
key: ONYXKEYS.PERSONAL_DETAILS_LIST, | ||
callback: (val) => (allPersonalDetails = _.isEmpty(val) ? {} : val), | ||
}); | ||
|
||
/** | ||
* Searches through given loginList for any contact method / login with an error. | ||
* | ||
|
@@ -68,12 +78,19 @@ function hashText(text: string, range: number): number { | |
return Math.abs(hashCode(text.toLowerCase())) % range; | ||
} | ||
|
||
/** | ||
* Generate a random accountID base on searchValue. | ||
*/ | ||
function generateAccountID(searchValue: string): number { | ||
return hashText(searchValue, 2 ** 32); | ||
} | ||
|
||
/** | ||
* Helper method to return the default avatar associated with the given accountID | ||
* @param [accountID] | ||
* @returns | ||
*/ | ||
function getDefaultAvatar(accountID = -1): React.FC<SvgProps> { | ||
function getDefaultAvatar(accountID = -1, avatarURL?: string): React.FC<SvgProps> { | ||
if (accountID <= 0) { | ||
return FallbackAvatar; | ||
} | ||
|
@@ -83,22 +100,35 @@ function getDefaultAvatar(accountID = -1): React.FC<SvgProps> { | |
|
||
// There are 24 possible default avatars, so we choose which one this user has based | ||
// on a simple modulo operation of their login number. Note that Avatar count starts at 1. | ||
const accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange; | ||
|
||
// When creating a chat, we generate an avatar using an ID and the backend response will modify the ID to the actual user ID. | ||
// But the avatar link still corresponds to the original ID-generated link. So we extract the SVG image number from the backend's link instead of using the user ID directly | ||
let accountIDHashBucket: AvatarRange; | ||
if (avatarURL) { | ||
const match = avatarURL.match(/(?<=default-avatar_)\d+(?=\.)/); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have one more default avatar prefix |
||
const lastDigit = match && parseInt(match[0], 10); | ||
accountIDHashBucket = lastDigit as AvatarRange; | ||
} else { | ||
accountIDHashBucket = ((accountID % CONST.DEFAULT_AVATAR_COUNT) + 1) as AvatarRange; | ||
} | ||
return defaultAvatars[`Avatar${accountIDHashBucket}`]; | ||
} | ||
|
||
/** | ||
* Helper method to return default avatar URL associated with login | ||
*/ | ||
function getDefaultAvatarURL(accountID: string | number = '', isNewDot = false): string { | ||
function getDefaultAvatarURL(accountID: string | number = ''): string { | ||
if (Number(accountID) === CONST.ACCOUNT_ID.CONCIERGE) { | ||
return CONST.CONCIERGE_ICON_URL; | ||
} | ||
// To ensure that the avatar remains unchanged and matches the one returned by the backend, | ||
// utilize an optimistic ID generated from the email instead of directly using the user ID. | ||
const email = allPersonalDetails?.[accountID]?.login; | ||
const originalOptimisticAccountID = email ? generateAccountID(email) : accountID; | ||
|
||
// Note that Avatar count starts at 1 which is why 1 has to be added to the result (or else 0 would result in a broken avatar link) | ||
const accountIDHashBucket = (Number(accountID) % (isNewDot ? CONST.DEFAULT_AVATAR_COUNT : CONST.OLD_DEFAULT_AVATAR_COUNT)) + 1; | ||
const avatarPrefix = isNewDot ? `default-avatar` : `avatar`; | ||
const accountIDHashBucket = (Number(originalOptimisticAccountID) % CONST.DEFAULT_AVATAR_COUNT) + 1; | ||
const avatarPrefix = `default-avatar`; | ||
|
||
return `${CONST.CLOUDFRONT_URL}/images/avatars/${avatarPrefix}_${accountIDHashBucket}.png`; | ||
} | ||
|
@@ -135,7 +165,7 @@ function isDefaultAvatar(avatarSource?: AvatarSource): boolean { | |
* @param accountID - the accountID of the user | ||
*/ | ||
function getAvatar(avatarSource: AvatarSource, accountID?: number): AvatarSource { | ||
return isDefaultAvatar(avatarSource) ? getDefaultAvatar(accountID) : avatarSource; | ||
return isDefaultAvatar(avatarSource) ? getDefaultAvatar(accountID, avatarSource as string) : avatarSource; | ||
} | ||
|
||
/** | ||
|
@@ -146,7 +176,7 @@ function getAvatar(avatarSource: AvatarSource, accountID?: number): AvatarSource | |
* @param accountID - the accountID of the user | ||
*/ | ||
function getAvatarUrl(avatarURL: string, accountID: number): string { | ||
return isDefaultAvatar(avatarURL) ? getDefaultAvatarURL(accountID, true) : avatarURL; | ||
return isDefaultAvatar(avatarURL) ? getDefaultAvatarURL(accountID) : avatarURL; | ||
} | ||
|
||
/** | ||
|
@@ -184,13 +214,6 @@ function getSmallSizeAvatar(avatarSource: AvatarSource, accountID?: number): Ava | |
return `${source.substring(0, lastPeriodIndex)}_128${source.substring(lastPeriodIndex)}`; | ||
} | ||
|
||
/** | ||
* Generate a random accountID base on searchValue. | ||
*/ | ||
function generateAccountID(searchValue: string): number { | ||
return hashText(searchValue, 2 ** 32); | ||
} | ||
|
||
/** | ||
* Gets the secondary phone login number | ||
*/ | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should avoid this pattern.
ref: https://expensify.slack.com/archives/C01GTK53T8Q/p1688630003862989
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.
The link doesn't work for me for some reason 😅 Can you post some more detail here maybe?
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.
Bump on this question
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.
Managed to get the link to load by pasting it into a chat with myself on Slack! 😆
As far as I can tell we aren't memoizing any of the calls to
getDefaultAvatarURL
so I don't think the issue is particularly relevant here. Many of the calls togetDefaultAvatarURL
are also fromReportUtils
which also uses this pattern so it wouldn't be a very useful optimization 😄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.
@Julesssss Friendly bump on the above - I think we should be good to merge!