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

feat: ordered mention suggestions #42553

Merged
merged 3 commits into from
Jun 14, 2024
Merged

feat: ordered mention suggestions #42553

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a2f068
ordered mention suggestions
gijoe0295 May 23, 2024
61ddeb3
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/fe…
gijoe0295 Jun 1, 2024
7572842
Merge branch 'main' of https://github.com/gijoe0295/App into gijoe/fe…
gijoe0295 Jun 6, 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
54 changes: 47 additions & 7 deletions src/pages/home/report/ReportActionCompose/SuggestionMention.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,29 @@
import {Str} from 'expensify-common';
import lodashMapValues from 'lodash/mapValues';
import lodashSortBy from 'lodash/sortBy';
import type {ForwardedRef} from 'react';
import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState} from 'react';
import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react';
import {useOnyx} from 'react-native-onyx';
import type {OnyxCollection} from 'react-native-onyx';
import * as Expensicons from '@components/Icon/Expensicons';
import type {Mention} from '@components/MentionSuggestions';
import MentionSuggestions from '@components/MentionSuggestions';
import {usePersonalDetails} from '@components/OnyxProvider';
import useArrowKeyFocusManager from '@hooks/useArrowKeyFocusManager';
import useCurrentReportID from '@hooks/useCurrentReportID';
import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails';
import useDebounce from '@hooks/useDebounce';
import useLocalize from '@hooks/useLocalize';
import * as LoginUtils from '@libs/LoginUtils';
import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils';
import getPolicyEmployeeAccountIDs from '@libs/PolicyEmployeeListUtils';
import * as ReportUtils from '@libs/ReportUtils';
import * as SuggestionsUtils from '@libs/SuggestionUtils';
import {isValidRoomName} from '@libs/ValidationUtils';
import * as ReportUserActions from '@userActions/Report';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {PersonalDetailsList, Report} from '@src/types/onyx';
import type {PersonalDetails, PersonalDetailsList, Report} from '@src/types/onyx';
import type {SuggestionsRef} from './ReportActionCompose';
import type {SuggestionProps} from './Suggestions';

Expand All @@ -45,6 +48,14 @@ const defaultSuggestionsValues: SuggestionValues = {
prefixType: '',
};

type SuggestionPersonalDetailsList = Record<
string,
| (PersonalDetails & {
weight: number;
})
| null
>;

function SuggestionMention(
{value, selection, setSelection, updateComment, isAutoSuggestionPickerLarge, measureParentContainer, isComposerFocused, isGroupPolicyReport, policyID}: SuggestionProps,
ref: ForwardedRef<SuggestionsRef>,
Expand All @@ -58,6 +69,36 @@ function SuggestionMention(
const currentUserPersonalDetails = useCurrentUserPersonalDetails();
const isMentionSuggestionsMenuVisible = !!suggestionValues.suggestedMentions.length && suggestionValues.shouldShowSuggestionMenu;

const currentReportID = useCurrentReportID();
const currentReport = reports?.[`${ONYXKEYS.COLLECTION.REPORT}${currentReportID?.currentReportID}`] ?? null;
// Smaller weight means higher order in suggestion list
const getPersonalDetailsWeight = useCallback(
(detail: PersonalDetails, policyEmployeeAccountIDs: number[]): number => {
if (ReportUtils.isReportParticipant(detail.accountID, currentReport)) {
return 0;
}
if (policyEmployeeAccountIDs.includes(detail.accountID)) {
return 1;
}
return 2;
},
[currentReport],
);
const weightedPersonalDetails: PersonalDetailsList | SuggestionPersonalDetailsList = useMemo(() => {
const policyEmployeeAccountIDs = getPolicyEmployeeAccountIDs(policyID);
if (!ReportUtils.isGroupChat(currentReport) && !ReportUtils.doesReportBelongToWorkspace(currentReport, policyEmployeeAccountIDs, policyID)) {
return personalDetails;
}
Comment on lines +89 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this condition is met in my case, that is why weighted ordering didn't work. Removing it helped and showed group members first. I think we don't need this condition. Can we remove it safely?
@gijoe0295

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per OP:

when in a group chat or a chat connected to a workspace

Removing this condition would introduce unnecessary computation of weightedPersonalDetails for normal reports.

I think there's some incorrect data with your currentReport. If it was a group chat, the condition would work properly. Maybe let's log out then log in to see if it got the correct data?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I confused the cases in the condition, that's why my conclusion was wrong

I see that for a report to be a group chat it should have chatType=group but it is empty for this case. Maybe it is a thing with older groups. I checked this with multiple old groups in two accounts and the same true for all

Below is currentReport, policyEmployeeAccountIDs, policyID

{
    "reportID": "1164088907436698",
    "reportName": "Chat Report",
    "type": "chat",
    "chatType": "",
    "ownerAccountID": 0,
    "managerID": 0,
    "policyID": "_FAKE_",
    "participants": {
        "14579716": {
            "hidden": false
        },
        "14582230": {
            "hidden": false
        },
        "14640187": {
            "hidden": false
        },
        "14640366": {
            "hidden": false
        },
        "14640421": {
            "hidden": false
        }
    },
    "participantAccountIDs": [
        14582230,
        14640187,
        14640366,
        14640421
    ],
    "visibleChatMemberAccountIDs": [
        14582230,
        14640187,
        14640366,
        14640421
    ],
    "isPinned": false,
    "lastReadTime": "2024-04-12 15:49:29.672",
    "lastReadSequenceNumber": 0,
    "lastVisibleActionCreated": "2024-04-12 15:49:29.672",
    "lastVisibleActionLastModified": "2024-04-12 15:49:29.672",
    "lastMessageText": "@alitoshmatov2001+test@gmail.com  Hello world, new @alitoshmatov2001+android@gm…",
    "lastActionType": "ADDCOMMENT",
    "lastActorAccountID": "14579716",
    "notificationPreference": "always",
    "stateNum": 0,
    "statusNum": 0,
    "oldPolicyName": "",
    "isOwnPolicyExpenseChat": false,
    "lastMessageHtml": "<mention-user>@alitoshmatov2001+test@gmail.com</mention-user>  Hello world, new <mention-user>@alitoshmatov2001+android@gmail.com</mention-user>",
    "hasOutstandingChildRequest": false,
    "writeCapability": "all",
    "description": "",
    "total": 0,
    "unheldTotal": 0,
    "currency": "USD",
    "isWaitingOnBankAccount": false,
    "nonReimbursableTotal": 0,
    "isCancelledIOU": false,
    "permissions": [
        "read",
        "write"
    ],
    "submitterPayPalMeAddress": "",
    "welcomeMessage": "",
    "errorFields": {}
}
Screenshot 2024-06-05 at 3 19 25 AM

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 see that for a report to be a group chat it should have chatType=group but it is empty for this case.

I think this is another bug and not related to this PR. Before group chats, we had group DMs. Maybe the migration from group DMs had some problems.

cc @yuwenmemon We found several group chats with empty chatType (should be group), do you have any clue on this? Should we ignore it since it is not related to this 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 we should proceed with this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@gijoe0295 Yeah maybe let's raise in expensify-bugs: https://expensify.enterprise.slack.com/archives/C049HHMV9SM

return lodashMapValues(personalDetails, (detail) =>
detail
? {
...detail,
weight: getPersonalDetailsWeight(detail, policyEmployeeAccountIDs),
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: A more apt name could be maybe something like orderWeight

}
: null,
);
}, [policyID, currentReport, personalDetails, getPersonalDetailsWeight]);

const [highlightedMentionIndex, setHighlightedMentionIndex] = useArrowKeyFocusManager({
isActive: isMentionSuggestionsMenuVisible,
maxIndex: suggestionValues.suggestedMentions.length - 1,
Expand Down Expand Up @@ -190,7 +231,7 @@ function SuggestionMention(
);

const getUserMentionOptions = useCallback(
(personalDetailsParam: PersonalDetailsList, searchValue = ''): Mention[] => {
(personalDetailsParam: PersonalDetailsList | SuggestionPersonalDetailsList, searchValue = ''): Mention[] => {
const suggestions = [];

if (CONST.AUTO_COMPLETE_SUGGESTER.HERE_TEXT.includes(searchValue.toLowerCase())) {
Expand Down Expand Up @@ -231,8 +272,7 @@ function SuggestionMention(
return true;
});

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- nullish coalescing cannot be used if left side can be empty string
const sortedPersonalDetails = lodashSortBy(filteredPersonalDetails, (detail) => detail?.displayName || detail?.login);
const sortedPersonalDetails = lodashSortBy(filteredPersonalDetails, ['weight', 'displayName', 'login']);
sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => {
suggestions.push({
text: formatLoginPrivateDomain(PersonalDetailsUtils.getDisplayNameOrDefault(detail), detail?.login),
Expand Down Expand Up @@ -320,7 +360,7 @@ function SuggestionMention(
};

if (isMentionCode(suggestionWord) && prefixType === '@') {
const suggestions = getUserMentionOptions(personalDetails, prefix);
const suggestions = getUserMentionOptions(weightedPersonalDetails, prefix);
nextState.suggestedMentions = suggestions;
nextState.shouldShowSuggestionMenu = !!suggestions.length;
}
Expand All @@ -340,7 +380,7 @@ function SuggestionMention(
}));
setHighlightedMentionIndex(0);
},
[isComposerFocused, value, isGroupPolicyReport, setHighlightedMentionIndex, resetSuggestions, getUserMentionOptions, personalDetails, getRoomMentionOptions, reports],
[isComposerFocused, value, isGroupPolicyReport, setHighlightedMentionIndex, resetSuggestions, getUserMentionOptions, weightedPersonalDetails, getRoomMentionOptions, reports],
);

useEffect(() => {
Expand Down
Loading