[$500] iOS - Request Money - Searching for specific HT account while using HT account has an odd behavior

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.3.95.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #30591

Action Performed:

Pre-requisite: user must be logged in with a HT account.

1. Tap on "+" button.
2. Tap on "Request money".
3. Select "Manual".
4. Enter any amount and tap on "Next".
5. Search for a specific HT account, such as "applausetester+vd_htmain@applause.expensifail.com".

Expected Result:

The search results should only show the requested account.

Actual Result:

The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6261440_1698958778867.Upyl2566_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c1e5f3f5dad45859
• Upwork Job ID: 1720190449397870592
• Last Price Increase: 2023-11-02

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01c1e5f3f5dad45859

[melvin-bot]

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[orshikh-dev]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars.

What is the root cause of that problem?

It shows an existing account with a specific existing email and it also adds a non-existed account with non-existed email.

When we are searching with some text, search input sends a search term for every single character. In every request, it will call this method, this method returns all list options:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 111 to 160 in a61572e

	const sections = useMemo(() => {
	const newSections = [];
	let indexOffset = 0;
	const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(
	searchTerm,
	participants,
	newChatOptions.recentReports,
	newChatOptions.personalDetails,
	personalDetails,
	true,
	indexOffset,
	);
	newSections.push(formatResults.section);
	indexOffset = formatResults.newIndexOffset;
	if (maxParticipantsReached) {
	return newSections;
	}
	newSections.push({
	title: translate('common.recents'),
	data: newChatOptions.recentReports,
	shouldShow: !_.isEmpty(newChatOptions.recentReports),
	indexOffset,
	});
	indexOffset += newChatOptions.recentReports.length;
	newSections.push({
	title: translate('common.contacts'),
	data: newChatOptions.personalDetails,
	shouldShow: !_.isEmpty(newChatOptions.personalDetails),
	indexOffset,
	});
	indexOffset += newChatOptions.personalDetails.length;
	if (newChatOptions.userToInvite && !OptionsListUtils.isCurrentUser(newChatOptions.userToInvite)) {
	newSections.push({
	undefined,
	data: _.map([newChatOptions.userToInvite], (participant) => {
	const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false);
	return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails);
	}),
	shouldShow: true,
	indexOffset,
	});
	}
	return newSections;
	}, [maxParticipantsReached, newChatOptions, participants, personalDetails, translate, searchTerm]);

If we find a user with a specific email, that user will be added to sections

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 115 to 124 in a61572e

	const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(
	searchTerm,
	participants,
	newChatOptions.recentReports,
	newChatOptions.personalDetails,
	personalDetails,
	true,
	indexOffset,
	);
	newSections.push(formatResults.section);

And if a user does not exist, we add non-existing users into sections as well,

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 147 to 159 in a61572e

	if (newChatOptions.userToInvite && !OptionsListUtils.isCurrentUser(newChatOptions.userToInvite)) {
	newSections.push({
	undefined,
	data: _.map([newChatOptions.userToInvite], (participant) => {
	const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false);
	return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails);
	}),
	shouldShow: true,
	indexOffset,
	});
	}
	return newSections;

There are 2 possible cases:
CASE 1: THE USER DOES NOT EXIST
Non-existing user is added into sections, no issues:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Lines 147 to 159 in a61572e

	if (newChatOptions.userToInvite && !OptionsListUtils.isCurrentUser(newChatOptions.userToInvite)) {
	newSections.push({
	undefined,
	data: _.map([newChatOptions.userToInvite], (participant) => {
	const isPolicyExpenseChat = lodashGet(participant, 'isPolicyExpenseChat', false);
	return isPolicyExpenseChat ? OptionsListUtils.getPolicyExpenseReportOption(participant) : OptionsListUtils.getParticipantsOption(participant, personalDetails);
	}),
	shouldShow: true,
	indexOffset,
	});
	}
	return newSections;

CASE 2: USER EXIST
In this case, the user will be added to the section, just like this:

App/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js

Line 124 in a61572e

newSections.push(formatResults.section);

But, in every single valid email address, it will be a non-existent user:
For example applausetester+vd_htmain@applause.expensifail.com

Existed user:
If the email address is valid, it will be added into sections:
applausetester+vd_htmain@applause.e is enough, not required to be whole email, just valid, existed email pattern is enough

Non-existed users with valid emails:
applausetester+vd_htmain@applause.e <= here we know this email is valid and found existed user with this email
applausetester+vd_htmain@applause.ex
applausetester+vd_htmain@applause.exp
applausetester+vd_htmain@applause.expe
applausetester+vd_htmain@applause.expen
. . .
applausetester+vd_htmain@applause.expensifail.co <= till, not whole email address

That's why the avatar is changing because, for every valid email, for every single character, it will send a request, in every request, the non-exist user(all different user) will be added into sections, in this example, for all characters in this text [xpensifail.co]. The email was so long, so we didn't see it was changing in the row.

Please see from this video:
https://drive.google.com/file/d/1LOnmzTz-fSW9oCk-p55pkh0PlKRmRBv0/view?usp=sharing

What changes do you think we should make in order to solve the problem?

1. We need to improve UX. We can make some changes to the email address, maybe we can concat from the middle, or concat from the head, now it is concating from the tail.

Concating from head:

Concating from middle:

2. We can make other UX design changes, user can easily understand what is going on.

What alternative solutions did you explore? (Optional)

Or we can set a limit on email length, so it can't be that long to be out of the screen. In example, 42 character length email:

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[rojiphil]

Proposal

Please re-state the problem that we are trying to solve in this issue.

In request money participants selector page, the search results keeps getting constantly updated (i.e avatar, search result itself) even after the user has stopped entering the search value. This is the problem we are trying to solve here.

What is the root cause of that problem?

When a long search text is entered (e.g. applausetester+vd_htmain@applause.expensifail.com), this will generate many API requests to the server for reports as shown here. Since each of these responses will come back at some time later and trigger the useEffect here, updation of the sections here will get delayed in a HT environment with lots of results.

What changes do you think we should make in order to solve the problem?

We can solve this problem by debouncing the updation of chat options thereby saving processing time which will matter a lot in a HT environment.

1. We can move the contents of useEffect here into a new function updateChatOptions like this

    const updateChatOptions = useCallback(() => {
       // Contents of useEffect here  
    }, [betas, reports, participants, personalDetails, translate, searchTerm, setNewChatOptions, iouType, isDistanceRequest]);

2. Debouncing the updateChatOptions can be done like this here

    useEffect(() => {
        const debouncedFetchChatOptions = _.debounce(updateChatOptions, 200);
        debouncedFetchChatOptions();
        return () => {
            debouncedFetchChatOptions.cancel();
        };
    }, [updateChatOptions]);

What alternative solutions did you explore? (Optional)

[melvin-bot]

@puneetlath, @mananjadhav Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

Thoughts on the proposals @mananjadhav?

[mananjadhav]

Will review this today.

[mananjadhav]

I can see the multiple API calls for the search. I think @rojiphil's proposal would work.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

Current assignee @puneetlath is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[puneetlath]

I think it also makes sense to debounce the API calls for this. Would just like a second opinion from @marcaaron since I believe he implemented this.

[marcaaron]

Since each of these responses will come back at some time later and trigger the useEffect here, updation of the sections here will get delayed in a HT environment with lots of results.

API calls happening "too many times"? Not sure I really buy that.

There's a debounce for the API calls already. Maybe they happen too many times, but what does it have to do with:

The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars

Do we feel like we have a clear understanding of what this bug report is about?

[rojiphil]

Since each of these responses will come back at some time later and trigger the useEffect here, updation of the sections here will get delayed in a HT environment with lots of results.

API calls happening "too many times"? Not sure I really buy that.

There's a debounce for the API calls already. Maybe they happen too many times, but what does it have to do with:

The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars

Do we feel like we have a clear understanding of what this bug report is about?

@marcaaron
Agree that there is debounce for the API calls already. But it looks like when it is called too many times (I.e. due to long mail id applausetester+vd_htmain@applause.expensifail.com) in a HT environment, rendering gets stacked up for each of these responses until it gets rendered sequentially. As a result, we can see constantly changing avatars even after the user has stopped entering the email.
Does this make sense?

There is a similar approach used here in TaskShareDestinationSelectorModal where we additionally debounce the handling of responses along with debounced API calls. Could be related to this.

[marcaaron]

As a result, we can see constantly changing avatars even after the user has stopped entering the email. Does this make sense?

Backing up, why does the avatar change? It changes on every render? I would look at that first before considering the API requests as the solution (but perhaps both can end up in the final proposal).

[marcaaron]

There is a similar approach used here in TaskShareDestinationSelectorModal where we additionally debounce the handling of responses along with debounced API calls. Could be related to this.

To solve a rendering performance issue.

So what bug are we solving? I think it needs clarifying.

[rojiphil]

@marcaaron

To solve a rendering performance issue.

Thanks for pointing this out. My bad. Just needed a simple blame check to reach here.

Backing up, why does the avatar change? It changes on every render? I would look at that first before considering the API requests as the solution (but perhaps both can end up in the final proposal).

So what bug are we solving? I think it needs clarifying.

From here and here, avatars can change for users not yet saved in Onyx as it depends on the searchValue. In our case, since the searchValue keeps changing, the avatar keeps changing due to the side effect call to getFilteredOptions here.

As shown in the test video of the issue, since the avatars keep changing even after the user has stopped keying in, it seems like the side effects of incoming data from API response in a HT environment is resulting in a performance problem.
Makes sense? No?

[marcaaron]

Thanks! I'm going OOO soon so @puneetlath can hopefully continue this convo with you @rojiphil.

Without doing a deep dive - I would expect that the avatar color would change only if the login itself changes.

IMO it feels like the problem here is that we are regenerating an optimistic accountID on each render and inside the getOptions() method.

it seems like the side effects of incoming data from API response in a HT environment is resulting in a performance problem

This ticket is unrelated to performance AFAICT...

Bug that was reported:

The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars.

[rojiphil]

 I would expect that the avatar color would change only if the login itself changes.
IMO it feels like the problem here is that we are regenerating an optimistic accountID on each render and inside the getOptions() method.

Regenerating an optimistic accountID based on searchValue within getOptions() seems to be an intentional change as per this PR here. Not sure if we want to revisit this.

Bug that was reported:
The account being searched is at the top of the list, but then reappears at the bottom constantly changing avatars.

Meanwhile, on having a closer observation of the test video attached to this issue, I noticed a massive lag from 0:34 sec onwards (i.e. after the spinner stops).
Doesn’t this seem to be the real issue here?
Also, this looks similar to the one mentioned in issue here.
Maybe, the bug description here is somewhat misleading.

[melvin-bot]

@puneetlath, @mananjadhav Eep! 4 days overdue now. Issues have feelings too...

[melvin-bot]

Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.

[melvin-bot]

@puneetlath, @mananjadhav Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@puneetlath, @mananjadhav Huh... This is 4 days overdue. Who can take care of this?

[puneetlath]

Sorry for the delay. @mananjadhav what are your thoughts on all this?

[mananjadhav]

@puneetlath I am not sure if I can get to this sooner, can you please reassign?

[rojiphil]

@puneetlath
The last discussion we had was here.
We need to continue from there.

[situchan]

interested in reviewing this

[puneetlath]

Sounds great, thanks @situchan. Maybe you could start by reviewing the conversation and summarizing where we're at, along with your opinion.

[melvin-bot]

@puneetlath, @situchan Eep! 4 days overdue now. Issues have feelings too...

[situchan]

Reviewed all conversations. reviewing proposals.

[puneetlath]

Let us know your thoughts @situchan

[situchan]

I tested on latest main and the behavior is not that odd.
@rojiphil can you please retest and share video?
FYI: debounce was introduced in #32962 and just deployed to staging

[melvin-bot]

@puneetlath, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

I'm going to go ahead and close this issue based on @situchan's experience. @rojiphil feel free to let us know if you think otherwise and we can reopen if there is something to fix.