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

kbecciv · 2023-11-02T21:25:35Z

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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.

Tap on "+" button.
Tap on "Request money".
Select "Manual".
Enter any amount and tap on "Next".
Search for a specific HT account, such as "[email protected]".

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?

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 · 2023-11-02T21:25:39Z

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

melvin-bot · 2023-11-02T21:25:40Z

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

melvin-bot · 2023-11-02T21:25:42Z

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 · 2023-11-02T21:25:44Z

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

orshikh-dev · 2023-11-03T07:20:38Z

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 [email protected]

Existed user:
If the email address is valid, it will be added into sections:
[email protected] is enough, not required to be whole email, just valid, existed email pattern is enough

Non-existed users with valid emails:
[email protected] <= here we know this email is valid and found existed user with this email
[email protected]
[email protected]
[email protected]
[email protected]
. . .
[email protected] <= 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?

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:

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 · 2023-11-03T10:45:35Z

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. [email protected]), 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.

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]);

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 · 2023-11-06T13:25:40Z

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

puneetlath · 2023-11-06T19:20:24Z

Thoughts on the proposals @mananjadhav?

mananjadhav · 2023-11-07T12:40:00Z

Will review this today.

mananjadhav · 2023-11-07T19:36:06Z

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

🎀 👀 🎀 C+ reviewed.

melvin-bot · 2023-11-07T19:36:12Z

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

puneetlath · 2023-11-07T22:45:54Z

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 · 2023-11-08T06:09:37Z

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 · 2023-11-08T08:26:35Z

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 [email protected]) 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 · 2023-11-08T22:45:01Z

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 · 2023-11-08T22:47:59Z

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 · 2023-11-10T13:34:34Z

@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 · 2023-11-10T21:54:46Z

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 · 2023-11-11T06:35:32Z

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 · 2023-11-13T14:29:28Z

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

melvin-bot · 2023-11-30T23:40:05Z

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

melvin-bot · 2023-12-05T23:43:45Z

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

melvin-bot · 2023-12-06T11:07:08Z

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

puneetlath · 2023-12-06T16:26:30Z

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

mananjadhav · 2023-12-06T16:35:52Z

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

rojiphil · 2023-12-06T16:37:29Z

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

situchan · 2023-12-06T16:44:05Z

interested in reviewing this

puneetlath · 2023-12-06T16:50:14Z

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

melvin-bot · 2023-12-12T11:06:09Z

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

situchan · 2023-12-12T13:34:49Z

Reviewed all conversations. reviewing proposals.

puneetlath · 2023-12-15T15:53:45Z

Let us know your thoughts @situchan

situchan · 2023-12-19T04:23:45Z

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 · 2023-12-22T11:08:19Z

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

puneetlath · 2023-12-22T21:58:10Z

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.

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 2, 2023

melvin-bot bot changed the title ~~iOS - Request Money - Searching for specific HT account while using HT account has an odd behavior~~ [$500] iOS - Request Money - Searching for specific HT account while using HT account has an odd behavior Nov 2, 2023

melvin-bot bot assigned puneetlath Nov 2, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 2, 2023

melvin-bot bot assigned mananjadhav Nov 2, 2023

melvin-bot bot added the Overdue label Nov 6, 2023

melvin-bot bot removed the Overdue label Nov 6, 2023

melvin-bot bot added the Overdue label Nov 10, 2023

melvin-bot bot added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 30, 2023

melvin-bot bot added the Overdue label Dec 4, 2023

melvin-bot bot removed the Overdue label Dec 6, 2023

puneetlath assigned situchan and unassigned mananjadhav Dec 6, 2023

melvin-bot bot added the Overdue label Dec 8, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 12, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 15, 2023

melvin-bot bot added Overdue and removed Overdue labels Dec 19, 2023

puneetlath closed this as completed Dec 22, 2023

melvin-bot bot removed the Overdue label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kbecciv commented Nov 2, 2023 •

edited

Loading

melvin-bot bot commented Nov 2, 2023

melvin-bot bot commented Nov 2, 2023

melvin-bot bot commented Nov 2, 2023

melvin-bot bot commented Nov 2, 2023

orshikh-dev commented Nov 3, 2023 •

edited

Loading

rojiphil commented Nov 3, 2023

melvin-bot bot commented Nov 6, 2023

puneetlath commented Nov 6, 2023

mananjadhav commented Nov 7, 2023

mananjadhav commented Nov 7, 2023

melvin-bot bot commented Nov 7, 2023

puneetlath commented Nov 7, 2023

marcaaron commented Nov 8, 2023

rojiphil commented Nov 8, 2023 •

edited

Loading

marcaaron commented Nov 8, 2023

marcaaron commented Nov 8, 2023

rojiphil commented Nov 10, 2023

marcaaron commented Nov 10, 2023

rojiphil commented Nov 11, 2023

melvin-bot bot commented Nov 13, 2023

melvin-bot bot commented Nov 30, 2023

melvin-bot bot commented Dec 5, 2023

melvin-bot bot commented Dec 6, 2023

puneetlath commented Dec 6, 2023

mananjadhav commented Dec 6, 2023

rojiphil commented Dec 6, 2023

situchan commented Dec 6, 2023

puneetlath commented Dec 6, 2023

melvin-bot bot commented Dec 12, 2023

situchan commented Dec 12, 2023

puneetlath commented Dec 15, 2023

situchan commented Dec 19, 2023

melvin-bot bot commented Dec 22, 2023

puneetlath commented Dec 22, 2023

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

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

Comments

kbecciv commented Nov 2, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Nov 2, 2023

melvin-bot bot commented Nov 2, 2023

melvin-bot bot commented Nov 2, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Nov 2, 2023

orshikh-dev commented Nov 3, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

rojiphil commented Nov 3, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Nov 6, 2023

puneetlath commented Nov 6, 2023

mananjadhav commented Nov 7, 2023

mananjadhav commented Nov 7, 2023

melvin-bot bot commented Nov 7, 2023

puneetlath commented Nov 7, 2023

marcaaron commented Nov 8, 2023

rojiphil commented Nov 8, 2023 • edited Loading

marcaaron commented Nov 8, 2023

marcaaron commented Nov 8, 2023

rojiphil commented Nov 10, 2023

marcaaron commented Nov 10, 2023

rojiphil commented Nov 11, 2023

melvin-bot bot commented Nov 13, 2023

melvin-bot bot commented Nov 30, 2023

melvin-bot bot commented Dec 5, 2023

melvin-bot bot commented Dec 6, 2023

puneetlath commented Dec 6, 2023

mananjadhav commented Dec 6, 2023

rojiphil commented Dec 6, 2023

situchan commented Dec 6, 2023

puneetlath commented Dec 6, 2023

melvin-bot bot commented Dec 12, 2023

situchan commented Dec 12, 2023

puneetlath commented Dec 15, 2023

situchan commented Dec 19, 2023

melvin-bot bot commented Dec 22, 2023

puneetlath commented Dec 22, 2023

kbecciv commented Nov 2, 2023 •

edited

Loading

orshikh-dev commented Nov 3, 2023 •

edited

Loading

rojiphil commented Nov 8, 2023 •

edited

Loading