Skip to content

Refactored localeCompare in ReportUtils (part 3) #68101

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
mountiny merged 7 commits into from
Aug 25, 2025
Merged

Refactored localeCompare in ReportUtils (part 3) #68101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b98a1c5
Refactored localeCompare in ReportUtils (part 3)
shubham1206agra Aug 7, 2025
f7c5068
Fixed localeCompare usage
shubham1206agra Aug 7, 2025
e3fbe0b
Fixed lint
shubham1206agra Aug 7, 2025
b0f512b
Merge branch 'Expensify:main' into refactor-onyx-45
shubham1206agra Aug 23, 2025
42ace3b
Added tests
shubham1206agra Aug 23, 2025
5b5fb58
Apply suggestions from code review
shubham1206agra Aug 23, 2025
def0523
Fixed lint
shubham1206agra Aug 23, 2025
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
27 changes: 5 additions & 22 deletions src/components/ReportActionAvatars/ReportActionAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,14 @@ 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 {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<typeof CONST.REPORT_ACTION_AVATARS.SORT_BY>;
Expand Down Expand Up @@ -286,23 +285,6 @@ function ReportActionAvatarSubscript({
</View>
);
}
const getIconDisplayName = (icon: IconType, personalDetails: OnyxInputOrEntry<PersonalDetailsList>) =>
icon.id ? (personalDetails?.[icon.id]?.displayName ?? personalDetails?.[icon.id]?.login ?? '') : '';

function sortIconsByName(icons: IconType[], personalDetails: OnyxInputOrEntry<PersonalDetailsList>) {
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,
Expand Down Expand Up @@ -330,6 +312,7 @@ function ReportActionAvatarMultipleHorizontal({
const theme = useTheme();
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {localeCompare} = useLocalize();

const [personalDetails] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST, {
canBeMissing: true,
Expand All @@ -345,13 +328,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);
Comment thread
shubham1206agra marked this conversation as resolved.
} 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
Expand Down
51 changes: 24 additions & 27 deletions src/libs/ReportUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,6 @@
totalDisplaySpend: number;
};

type ParticipantDetails = [number, string, AvatarSource, AvatarSource];

type OptimisticAddCommentReportAction = Pick<
ReportAction<typeof CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT>,
| 'reportActionID'
Expand Down Expand Up @@ -914,7 +912,7 @@
const parsedReportActionMessageCache: Record<string, string> = {};

let conciergeReportID: OnyxEntry<string>;
Onyx.connect({

Check warning on line 915 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.CONCIERGE_REPORT_ID,
callback: (value) => {
conciergeReportID = value;
Expand All @@ -922,7 +920,7 @@
});

const defaultAvatarBuildingIconTestID = 'SvgDefaultAvatarBuilding Icon';
Onyx.connect({

Check warning on line 923 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.SESSION,
callback: (value) => {
// When signed out, val is undefined
Expand All @@ -940,7 +938,7 @@
let allPersonalDetails: OnyxEntry<PersonalDetailsList>;
let allPersonalDetailLogins: string[];
let currentUserPersonalDetails: OnyxEntry<PersonalDetails>;
Onyx.connect({

Check warning on line 941 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
callback: (value) => {
if (currentUserAccountID) {
Expand All @@ -952,14 +950,14 @@
});

let allReportsDraft: OnyxCollection<Report>;
Onyx.connect({

Check warning on line 953 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_DRAFT,
waitForCollectionCallback: true,
callback: (value) => (allReportsDraft = value),
});

let allPolicies: OnyxCollection<Policy>;
Onyx.connect({

Check warning on line 960 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.POLICY,
waitForCollectionCallback: true,
callback: (value) => (allPolicies = value),
Expand All @@ -967,7 +965,7 @@

let allReports: OnyxCollection<Report>;
let reportsByPolicyID: ReportByPolicyMap;
Onyx.connect({

Check warning on line 968 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT,
waitForCollectionCallback: true,
callback: (value) => {
Expand Down Expand Up @@ -1008,14 +1006,14 @@
});

let allBetas: OnyxEntry<Beta[]>;
Onyx.connect({

Check warning on line 1009 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.BETAS,
callback: (value) => (allBetas = value),
});

let allTransactions: OnyxCollection<Transaction> = {};
let reportsTransactions: Record<string, Transaction[]> = {};
Onyx.connect({

Check warning on line 1016 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.TRANSACTION,
waitForCollectionCallback: true,
callback: (value) => {
Expand All @@ -1041,7 +1039,7 @@
});

let allReportActions: OnyxCollection<ReportActions>;
Onyx.connect({

Check warning on line 1042 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_ACTIONS,
waitForCollectionCallback: true,
callback: (actions) => {
Expand All @@ -1054,7 +1052,7 @@

let allReportMetadata: OnyxCollection<ReportMetadata>;
const allReportMetadataKeyValue: Record<string, ReportMetadata> = {};
Onyx.connect({

Check warning on line 1055 in src/libs/ReportUtils.ts

View workflow job for this annotation

GitHub Actions / Changed files ESLint check 

Onyx.connect() is deprecated. Use useOnyx() hook instead and pass the data as parameters to a pure function
key: ONYXKEYS.COLLECTION.REPORT_METADATA,
waitForCollectionCallback: true,
callback: (value) => {
Expand Down Expand Up @@ -2739,38 +2737,18 @@
* The Avatar sources can be URLs or Icon components according to the chat type.
*/
function getIconsForParticipants(participants: number[], personalDetails: OnyxInputOrEntry<PersonalDetailsList>): 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) => {
Copy link
Copy Markdown
Contributor Author

@shubham1206agra shubham1206agra Aug 7, 2025

Choose a reason for hiding this comment

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

Context: Removed the sorting from here as major refactor revolving this happened where we are sorting later for multiple icons anyway.

Copy link
Copy Markdown
Contributor

@DylanDylann DylanDylann Aug 9, 2025

Choose a reason for hiding this comment

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

@shubham1206agra Could you please give more details to explain why we remove the sort function here?

Copy link
Copy Markdown
Contributor Author

@shubham1206agra shubham1206agra Aug 10, 2025

Choose a reason for hiding this comment

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

We don't need to sort because sorting is happening for icons case-by-case basis now in different place.

Copy link
Copy Markdown
Contributor

@DylanDylann DylanDylann Aug 11, 2025

Choose a reason for hiding this comment

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

@shubham1206agra Sorry, where is the sorting happening? Could you please provide an example?

Copy link
Copy Markdown
Contributor Author

@shubham1206agra shubham1206agra Aug 11, 2025

Choose a reason for hiding this comment

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

https://github.com/Expensify/App/pull/68101/files#r2266151754

Copy link
Copy Markdown
Contributor

@DylanDylann DylanDylann Aug 11, 2025

Choose a reason for hiding this comment

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

@shubham1206agra Since we removed the sorting logic from getIconsForParticipants, we now need to ensure that sorting is applied wherever this function is used. Could you please confirm that all such places have been covered?

Copy link
Copy Markdown
Contributor Author

@shubham1206agra shubham1206agra Aug 11, 2025

Choose a reason for hiding this comment

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

@DylanDylann I have discussed about this here https://expensify.slack.com/archives/C08CZDJFJ77/p1754547372604689. So you can proceed as usual here

// 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);
}
Expand Down Expand Up @@ -3373,6 +3351,24 @@
return getIconsForParticipants(participantAccountIDs, personalDetails);
}

const getIconDisplayName = (icon: Icon, personalDetails: OnyxInputOrEntry<PersonalDetailsList>) =>
icon.id ? (personalDetails?.[icon.id]?.displayName ?? personalDetails?.[icon.id]?.login ?? '') : '';

function sortIconsByName(icons: Icon[], personalDetails: OnyxInputOrEntry<PersonalDetailsList>, 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,
Expand Down Expand Up @@ -11605,6 +11601,7 @@
getUpgradeWorkspaceMessage,
getDowngradeWorkspaceMessage,
getIcons,
sortIconsByName,
getIconsForParticipants,
getIndicatedMissingPaymentMethod,
getLastVisibleMessage,
Expand Down
33 changes: 26 additions & 7 deletions tests/unit/ReportUtilsTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ import {
shouldReportBeInOptionList,
shouldReportShowSubscript,
shouldShowFlagComment,
sortIconsByName,
sortOutstandingReportsBySelected,
temporary_getMoneyRequestOptions,
} from '@libs/ReportUtils';
Expand Down Expand Up @@ -338,7 +339,7 @@ describe('ReportUtils', () => {
'1',
);

expect(title).toBeCalledWith(
expect(title).toHaveBeenCalledWith(
expect.objectContaining<OnboardingTaskLinks>({
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
testDriveURL: expect.any(String),
Expand Down Expand Up @@ -367,7 +368,7 @@ describe('ReportUtils', () => {
'1',
);

expect(description).toBeCalledWith(
expect(description).toHaveBeenCalledWith(
expect.objectContaining<OnboardingTaskLinks>({
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
testDriveURL: expect.any(String),
Expand All @@ -377,14 +378,14 @@ describe('ReportUtils', () => {
});

describe('getIconsForParticipants', () => {
it('returns sorted avatar source by name, then accountID', () => {
it('returns 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');
Expand All @@ -393,6 +394,24 @@ describe('ReportUtils', () => {
});
});

describe('sortIconsByName', () => {
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);

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`
Expand Down
Loading