From b98a1c5f7b5179de86cc0fa75c1316161cdeea83 Mon Sep 17 00:00:00 2001 From: Shubham Agrawal Date: Thu, 7 Aug 2025 15:15:19 +0530 Subject: [PATCH 1/6] Refactored localeCompare in ReportUtils (part 3) --- src/libs/ReportUtils.ts | 30 +++++------------------------- tests/unit/ReportUtilsTest.ts | 10 +++++----- 2 files changed, 10 insertions(+), 30 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 16edc52f0e17..a4e0db257ef1 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -2698,38 +2698,18 @@ function getDefaultGroupAvatar(reportID?: string): IconAsset { * The Avatar sources can be URLs or Icon components according to the chat type. */ function getIconsForParticipants(participants: number[], personalDetails: OnyxInputOrEntry): Icon[] { - const participantDetails: ParticipantDetails[] = []; const participantsList = participants || []; + const avatars: Icon[] = []; for (const accountID of participantsList) { const avatarSource = personalDetails?.[accountID]?.avatar ?? FallbackAvatar; const displayNameLogin = personalDetails?.[accountID]?.displayName ? personalDetails?.[accountID]?.displayName : personalDetails?.[accountID]?.login; - participantDetails.push([accountID, displayNameLogin ?? '', avatarSource, personalDetails?.[accountID]?.fallbackIcon ?? '']); - } - - const sortedParticipantDetails = participantDetails.sort((first, second) => { - // First sort by displayName/login - const displayNameLoginOrder = localeCompareLibs(first[1], second[1]); - if (displayNameLoginOrder !== 0) { - return displayNameLoginOrder; - } - - // Then fallback on accountID as the final sorting criteria. - // This will ensure that the order of avatars with same login/displayName - // stay consistent across all users and devices - return first[0] - second[0]; - }); - - // Now that things are sorted, gather only the avatars (second element in the array) and return those - const avatars: Icon[] = []; - - for (const sortedParticipantDetail of sortedParticipantDetails) { const userIcon = { - id: sortedParticipantDetail[0], - source: sortedParticipantDetail[2], + id: accountID, + source: avatarSource, type: CONST.ICON_TYPE_AVATAR, - name: sortedParticipantDetail[1], - fallbackIcon: sortedParticipantDetail[3], + name: displayNameLogin ?? '', + fallbackIcon: personalDetails?.[accountID]?.fallbackIcon ?? '', }; avatars.push(userIcon); } diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 473350f42442..faa6bbed29ce 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -440,14 +440,14 @@ describe('ReportUtils', () => { }); describe('getIconsForParticipants', () => { - it('returns sorted avatar source by name, then accountID', () => { + it('returns sorted avatar source', () => { const participants = getIconsForParticipants([1, 2, 3, 4, 5], participantsPersonalDetails); expect(participants).toHaveLength(5); - expect(participants.at(0)?.source).toBeInstanceOf(Function); - expect(participants.at(0)?.name).toBe('(833) 240-3627'); - expect(participants.at(0)?.id).toBe(4); - expect(participants.at(0)?.type).toBe('avatar'); + expect(participants.at(3)?.source).toBeInstanceOf(Function); + expect(participants.at(3)?.name).toBe('(833) 240-3627'); + expect(participants.at(3)?.id).toBe(4); + expect(participants.at(3)?.type).toBe('avatar'); expect(participants.at(1)?.source).toBeInstanceOf(Function); expect(participants.at(1)?.name).toBe('floki@vikings.net'); From f7c5068ac70df9166b64ed108e43691f6a715343 Mon Sep 17 00:00:00 2001 From: Shubham Agrawal Date: Thu, 7 Aug 2025 15:21:10 +0530 Subject: [PATCH 2/6] Fixed localeCompare usage --- .../ReportActionAvatars/ReportActionAvatar.tsx | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/components/ReportActionAvatars/ReportActionAvatar.tsx b/src/components/ReportActionAvatars/ReportActionAvatar.tsx index b7369d806e67..e19276611fa4 100644 --- a/src/components/ReportActionAvatars/ReportActionAvatar.tsx +++ b/src/components/ReportActionAvatars/ReportActionAvatar.tsx @@ -6,6 +6,7 @@ import type {ValueOf} from 'type-fest'; import Avatar from '@components/Avatar'; import Icon from '@components/Icon'; import {WorkspaceBuilding} from '@components/Icon/WorkspaceDefaultAvatars'; +import type {LocaleContextProps} from '@components/LocaleContextProvider'; import PressableWithoutFocus from '@components/Pressable/PressableWithoutFocus'; import Text from '@components/Text'; import Tooltip from '@components/Tooltip'; @@ -17,7 +18,6 @@ import useTheme from '@hooks/useTheme'; import useThemeIllustrations from '@hooks/useThemeIllustrations'; import useThemeStyles from '@hooks/useThemeStyles'; import {getCardFeedIcon} from '@libs/CardUtils'; -import localeCompare from '@libs/LocaleCompare'; import {getUserDetailTooltipText} from '@libs/ReportUtils'; import type {AvatarSource} from '@libs/UserUtils'; import Navigation from '@navigation/Navigation'; @@ -287,7 +287,7 @@ function ReportActionAvatarSubscript({ const getIconDisplayName = (icon: IconType, personalDetails: OnyxInputOrEntry) => icon.id ? (personalDetails?.[icon.id]?.displayName ?? personalDetails?.[icon.id]?.login ?? '') : ''; -function sortIconsByName(icons: IconType[], personalDetails: OnyxInputOrEntry) { +function sortIconsByName(icons: IconType[], personalDetails: OnyxInputOrEntry, localeCompare: LocaleContextProps['localeCompare']) { return icons.sort((first, second) => { // First sort by displayName/login const displayNameLoginOrder = localeCompare(getIconDisplayName(first, personalDetails), getIconDisplayName(second, personalDetails)); @@ -328,6 +328,7 @@ function ReportActionAvatarMultipleHorizontal({ const theme = useTheme(); const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); + const {localeCompare} = useLocalize(); const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, { canBeMissing: true, @@ -343,13 +344,13 @@ function ReportActionAvatarMultipleHorizontal({ let avatars: IconType[] = unsortedIcons; if (sortAvatars?.includes(CONST.REPORT_ACTION_AVATARS.SORT_BY.NAME)) { - avatars = sortIconsByName(unsortedIcons, personalDetails); + avatars = sortIconsByName(unsortedIcons, personalDetails, localeCompare); } else if (sortAvatars?.includes(CONST.REPORT_ACTION_AVATARS.SORT_BY.ID)) { avatars = lodashSortBy(unsortedIcons, (icon) => icon.id); } return sortAvatars?.includes(CONST.REPORT_ACTION_AVATARS.SORT_BY.REVERSE) ? avatars.reverse() : avatars; - }, [unsortedIcons, personalDetails, sortAvatars]); + }, [unsortedIcons, personalDetails, sortAvatars, localeCompare]); const avatarRows = useMemo(() => { // If we're not displaying avatars in rows or the number of icons is less than or equal to the max avatars in a row, return a single row From e3fbe0bd57379d565343058b69095953e97e2611 Mon Sep 17 00:00:00 2001 From: Shubham Agrawal Date: Thu, 7 Aug 2025 15:25:11 +0530 Subject: [PATCH 3/6] Fixed lint --- src/libs/ReportUtils.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index a4e0db257ef1..f80e7d677604 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -269,8 +269,6 @@ type SpendBreakdown = { totalDisplaySpend: number; }; -type ParticipantDetails = [number, string, AvatarSource, AvatarSource]; - type OptimisticAddCommentReportAction = Pick< ReportAction, | 'reportActionID' From 42ace3b41f381cca70d0216991f60a27cd9254d9 Mon Sep 17 00:00:00 2001 From: Shubham Agrawal Date: Sat, 23 Aug 2025 09:58:41 +0530 Subject: [PATCH 4/6] Added tests --- .../ReportActionAvatar.tsx | 22 ++----------------- src/libs/ReportUtils.ts | 19 ++++++++++++++++ tests/unit/ReportUtilsTest.ts | 21 +++++++++++++++++- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/components/ReportActionAvatars/ReportActionAvatar.tsx b/src/components/ReportActionAvatars/ReportActionAvatar.tsx index 3bfce99f3078..9e3776670b34 100644 --- a/src/components/ReportActionAvatars/ReportActionAvatar.tsx +++ b/src/components/ReportActionAvatars/ReportActionAvatar.tsx @@ -7,7 +7,6 @@ import type {UpperCaseCharacters} from 'type-fest/source/internal'; import Avatar from '@components/Avatar'; import Icon from '@components/Icon'; import {WorkspaceBuilding} from '@components/Icon/WorkspaceDefaultAvatars'; -import type {LocaleContextProps} from '@components/LocaleContextProvider'; import PressableWithoutFocus from '@components/Pressable/PressableWithoutFocus'; import Text from '@components/Text'; import Tooltip from '@components/Tooltip'; @@ -19,14 +18,14 @@ import useTheme from '@hooks/useTheme'; import useThemeIllustrations from '@hooks/useThemeIllustrations'; import useThemeStyles from '@hooks/useThemeStyles'; import {getCardFeedIcon} from '@libs/CardUtils'; -import {getUserDetailTooltipText} from '@libs/ReportUtils'; +import {getUserDetailTooltipText, sortIconsByName} from '@libs/ReportUtils'; import type {AvatarSource} from '@libs/UserUtils'; import Navigation from '@navigation/Navigation'; import variables from '@styles/variables'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import ROUTES from '@src/ROUTES'; -import type {CompanyCardFeed, OnyxInputOrEntry, PersonalDetailsList} from '@src/types/onyx'; +import type {CompanyCardFeed} from '@src/types/onyx'; import type {Icon as IconType} from '@src/types/onyx/OnyxCommon'; type SortingOptions = ValueOf; @@ -286,23 +285,6 @@ function ReportActionAvatarSubscript({ ); } -const getIconDisplayName = (icon: IconType, personalDetails: OnyxInputOrEntry) => - icon.id ? (personalDetails?.[icon.id]?.displayName ?? personalDetails?.[icon.id]?.login ?? '') : ''; - -function sortIconsByName(icons: IconType[], personalDetails: OnyxInputOrEntry, localeCompare: LocaleContextProps['localeCompare']) { - return icons.sort((first, second) => { - // First sort by displayName/login - const displayNameLoginOrder = localeCompare(getIconDisplayName(first, personalDetails), getIconDisplayName(second, personalDetails)); - if (displayNameLoginOrder !== 0) { - return displayNameLoginOrder; - } - - // Then fallback on accountID as the final sorting criteria. - // This will ensure that the order of avatars with same login/displayName - // stay consistent across all users and devices - return Number(first?.id) - Number(second?.id); - }); -} function ReportActionAvatarMultipleHorizontal({ isHovered = false, diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts index 22fa210e3e69..4e2e1c28abec 100644 --- a/src/libs/ReportUtils.ts +++ b/src/libs/ReportUtils.ts @@ -3351,6 +3351,24 @@ function getIcons( return getIconsForParticipants(participantAccountIDs, personalDetails); } +const getIconDisplayName = (icon: Icon, personalDetails: OnyxInputOrEntry) => + icon.id ? (personalDetails?.[icon.id]?.displayName ?? personalDetails?.[icon.id]?.login ?? '') : ''; + +function sortIconsByName(icons: Icon[], personalDetails: OnyxInputOrEntry, localeCompare: LocaleContextProps['localeCompare']) { + return icons.sort((first, second) => { + // First sort by displayName/login + const displayNameLoginOrder = localeCompare(getIconDisplayName(first, personalDetails), getIconDisplayName(second, personalDetails)); + if (displayNameLoginOrder !== 0) { + return displayNameLoginOrder; + } + + // Then fallback on accountID as the final sorting criteria. + // This will ensure that the order of avatars with same login/displayName + // stay consistent across all users and devices + return Number(first?.id) - Number(second?.id); + }); +} + function getDisplayNamesWithTooltips( personalDetailsList: PersonalDetails[] | PersonalDetailsList | OptionData[], shouldUseShortForm: boolean, @@ -11583,6 +11601,7 @@ export { getUpgradeWorkspaceMessage, getDowngradeWorkspaceMessage, getIcons, + sortIconsByName, getIconsForParticipants, getIndicatedMissingPaymentMethod, getLastVisibleMessage, diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index c316f169bb41..73585deb1fd2 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -71,6 +71,7 @@ import { shouldReportBeInOptionList, shouldReportShowSubscript, shouldShowFlagComment, + sortIconsByName, sortOutstandingReportsBySelected, temporary_getMoneyRequestOptions, } from '@libs/ReportUtils'; @@ -377,7 +378,7 @@ describe('ReportUtils', () => { }); describe('getIconsForParticipants', () => { - it('returns sorted avatar source', () => { + it('returns avatar source', () => { const participants = getIconsForParticipants([1, 2, 3, 4, 5], participantsPersonalDetails); expect(participants).toHaveLength(5); @@ -393,6 +394,24 @@ describe('ReportUtils', () => { }); }); + describe('sortIconsByName', () => { + it('returns sorted avatar source', () => { + const participants = getIconsForParticipants([1, 2, 3, 4, 5], participantsPersonalDetails); + const sortedParticipants = sortIconsByName(participants, participantsPersonalDetails, localeCompare); + expect(sortedParticipants).toHaveLength(5); + + expect(sortedParticipants.at(0)?.source).toBeInstanceOf(Function); + expect(sortedParticipants.at(0)?.name).toBe('(833) 240-3627'); + expect(sortedParticipants.at(0)?.id).toBe(4); + expect(sortedParticipants.at(0)?.type).toBe('avatar'); + + expect(sortedParticipants.at(1)?.source).toBeInstanceOf(Function); + expect(sortedParticipants.at(1)?.name).toBe('floki@vikings.net'); + expect(sortedParticipants.at(1)?.id).toBe(2); + expect(sortedParticipants.at(1)?.type).toBe('avatar'); + }); + }); + describe('getWorkspaceIcon', () => { it('should not use cached icon when avatar is updated', () => { // Given a new workspace and a expense chat with undefined `policyAvatar` From 5b5fb58b5f69ede57e0149b9e06827ac30f552d1 Mon Sep 17 00:00:00 2001 From: Shubham Agrawal <58412969+shubham1206agra@users.noreply.github.com> Date: Sat, 23 Aug 2025 10:01:09 +0530 Subject: [PATCH 5/6] Apply suggestions from code review --- tests/unit/ReportUtilsTest.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 73585deb1fd2..1c78c4877dfb 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -395,7 +395,7 @@ describe('ReportUtils', () => { }); describe('sortIconsByName', () => { - it('returns sorted avatar source', () => { + it('returns sorted avatar source by name, then accountID', () => { const participants = getIconsForParticipants([1, 2, 3, 4, 5], participantsPersonalDetails); const sortedParticipants = sortIconsByName(participants, participantsPersonalDetails, localeCompare); expect(sortedParticipants).toHaveLength(5); From def052340a473692ec78efe04e3633b4897d1c1f Mon Sep 17 00:00:00 2001 From: Shubham Agrawal Date: Sat, 23 Aug 2025 10:07:24 +0530 Subject: [PATCH 6/6] Fixed lint --- tests/unit/ReportUtilsTest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/ReportUtilsTest.ts b/tests/unit/ReportUtilsTest.ts index 1c78c4877dfb..432946572a2f 100644 --- a/tests/unit/ReportUtilsTest.ts +++ b/tests/unit/ReportUtilsTest.ts @@ -339,7 +339,7 @@ describe('ReportUtils', () => { '1', ); - expect(title).toBeCalledWith( + expect(title).toHaveBeenCalledWith( expect.objectContaining({ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment testDriveURL: expect.any(String), @@ -368,7 +368,7 @@ describe('ReportUtils', () => { '1', ); - expect(description).toBeCalledWith( + expect(description).toHaveBeenCalledWith( expect.objectContaining({ // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment testDriveURL: expect.any(String),