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

kbecciv commented 10 months ago

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 https://github.com/Expensify/App/pull/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 "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
[x] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/93399543/7edf62e1-2fb8-4105-98fe-61fd1c96cc66

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[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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[bot] commented 10 months ago

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

orshikh-dev commented 10 months ago

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:

https://github.com/Expensify/App/blob/a61572e1d794c219592d806b4be11001b8e39f93/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L111-L160

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

https://github.com/Expensify/App/blob/a61572e1d794c219592d806b4be11001b8e39f93/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L115-L124

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

https://github.com/Expensify/App/blob/a61572e1d794c219592d806b4be11001b8e39f93/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L147-L159

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

https://github.com/Expensify/App/blob/a61572e1d794c219592d806b4be11001b8e39f93/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L147-L159

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

https://github.com/Expensify/App/blob/a61572e1d794c219592d806b4be11001b8e39f93/src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js#L124

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?

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 commented 10 months ago

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[bot] commented 10 months ago

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

puneetlath commented 10 months ago

Thoughts on the proposals @mananjadhav?

mananjadhav commented 10 months ago

Will review this today.

mananjadhav commented 10 months ago

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

🎀 👀 🎀 C+ reviewed.

melvin-bot[bot] commented 10 months ago

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

puneetlath commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

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[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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

rojiphil commented 10 months ago

@puneetlath Looking forward to your feedback to the comment here Or do we wait until @marcaaron is back from OOO

melvin-bot[bot] commented 9 months ago

@puneetlath, @mananjadhav 12 days overdue now... This issue's end is nigh!

melvin-bot[bot] commented 9 months ago

@puneetlath @mananjadhav this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

melvin-bot[bot] commented 9 months ago

This issue has not been updated in over 14 days. @puneetlath, @mananjadhav eroding to Weekly issue.

melvin-bot[bot] commented 9 months ago

@puneetlath @mananjadhav this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:

Decide whether any proposals currently meet our guidelines and can be approved as-is today
If no proposals meet that standard, please take this issue internal and treat it as one of your highest priorities
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

puneetlath commented 9 months ago

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

mananjadhav commented 9 months ago

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

rojiphil commented 9 months ago

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

situchan commented 9 months ago

interested in reviewing this

puneetlath commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

situchan commented 9 months ago

Reviewed all conversations. reviewing proposals.

puneetlath commented 9 months ago

Let us know your thoughts @situchan

situchan commented 8 months ago

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 https://github.com/Expensify/App/pull/32962 and just deployed to staging

melvin-bot[bot] commented 8 months ago

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

puneetlath commented 8 months ago

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.

Expensify / App