[$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800

lanitochka17 · 2023-11-23T21:07:27Z

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.4.2-3
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:

Action Performed:

Go to the Search
Enter a character and remove it
Notice the loader is not immediately hiding with the removing character, taking 3 to 4 seconds to disappear

Expected Result:

When removing a character, the loader should be promptly removed with it because the data is already loaded

Actual Result:

After removing the character, the loader is eventually removed, but there is a noticeable delay of 3 to 4 seconds before it disappears, creating the impression that data is still loading/searching

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6288586_1700749110780.2023-11-23_18-55-42.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0141bb6ce1d327d478
Upwork Job ID: 1727796034918838272
Last Price Increase: 2023-11-30
Automatic offers:

jjcoffee | Reviewer | 27910413
paultsimura | Contributor | 27910414

melvin-bot · 2023-11-23T21:07:32Z

Job added to Upwork: https://www.upwork.com/jobs/~0141bb6ce1d327d478

melvin-bot · 2023-11-23T21:07:33Z

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

melvin-bot · 2023-11-23T21:07:35Z

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-23T21:07:37Z

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

FitseTLT · 2023-11-23T21:34:03Z

Proposal

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

Search - Delayed Removal of Loader When Removing a Character from the Search Field

What is the root cause of that problem?

Normally, whenever there is a change of searchInput it will call Report.searchInServer that queries the server and set the loading indicator state on until it gets back the data from the server, so for empty search input as in here

App/src/pages/SearchPage.js

Lines 86 to 92 in 4952c39

    
           onChangeText(searchValue = '') { 
        
               if (searchValue.length) { 
        
                   Report.searchInServer(searchValue); 
        
               } 
        
               this.setState({searchValue}, this.debouncedUpdateOptions); 
        
           }

we are not calling Report.searchInServer but we are not resetting IS_SEARCHING_FOR_REPORTS so it will only reset when the last query to the server resolves, hence the lag to remove the loader occurs.

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

We should set the IS_SEARCHING_FOR_REPORTS to false when the search value is empty

App/src/pages/SearchPage.js

Lines 86 to 92 in 4952c39

    
           onChangeText(searchValue = '') { 
        
               if (searchValue.length) { 
        
                   Report.searchInServer(searchValue); 
        
               } 
        
               this.setState({searchValue}, this.debouncedUpdateOptions); 
        
           }

to

onChangeText(searchValue = '') {
        if (searchValue.length) {
            Report.searchInServer(searchValue);
        }
        else
        Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
        this.setState({searchValue}, this.debouncedUpdateOptions);
    }

What alternative solutions did you explore? (Optional)

ZhenjaHorbach · 2023-11-23T21:44:50Z

Proposal

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

Search - Delayed Removal of Loader When Removing a Character from the Search Field

What is the root cause of that problem?

When we change the input we make a request to the server and show the loader using ONYXKEYS.IS_SEARCHING_FOR_REPORTS value from ONYX. During this period of time, we can clear the input and as a result, we are still waiting for a response and showing a loader until we receive a response from the server
That's why the loader is still showing

App/src/libs/actions/Report.js

Lines 2501 to 2502 in f52c59d

    
           Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true); 
        
           debouncedSearchInServer(searchInput);

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

Since we will in any case disable the loader after the server responds and it makes no sense to forcefully change the value in ONYX, in my opinion, it is best not to show the loader when we have an empty input

So we can update this line like

isLoadingNewOptions={!!this.state.searchValue && this.props.isSearchingForReports}

App/src/pages/SearchPage.js

Line 212 in 7a68a3d

isLoadingNewOptions={this.props.isSearchingForReports}

Also I noticed the same problem on Start chat screen
So it would be a good idea to check all the places where it is used OptionsSelector

What alternative solutions did you explore? (Optional)

Also, we can add a new prop for OptionsSelector which will be called shouldDisableLoadingWhenEmptyValue. Which will disable the loader when the list is empty at the BaseOptionsList level to pass this param in SearchPage or other places. Or we can make this behaviour by default

Plus I don’t like why when we hover over the loader we see a pointer cursor
This looks weird
Therefore, I would suggest fixing this too

DylanDylann · 2023-11-24T02:28:50Z

Proposal

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

Search - Delayed Removal of Loader When Removing a Character from the Search Field
This problem happen in many pages, we need to fix all

What is the root cause of that problem?

When we update search input, we will set IS_SEARCHING_FOR_REPORTS is true in optimistic data and reset it to false in successData and failureData. So even though we clear input, IS_SEARCHING_FOR_REPORTS is only set to false when API is done.

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

This problem happen in NewChatPage, SearchPage, MoneyRequestParticipantsSelector, TaskShareDestinationSelectorModal,...
In all these pages, we need add condition to hide the loading spinner if the search value is empty.

What alternative solutions did you explore? (Optional)

paultsimura · 2023-11-24T08:55:00Z

Proposal

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

When quickly typing and removing a character in search, the spinner is still loading.

What is the root cause of that problem?

We show the spinner when ONYXKEYS.IS_SEARCHING_FOR_REPORTS === true. This value is not set to false until the API request is completed.

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

First of all, it's worth noticing that such a behavior happens in all the pages where we currently use Report.searchInServer(). And in all the pages, we use the same logic:

if (text.length) {
    Report.searchInServer(text);
}

To fix this behavior, we should update Report.searchInServer() in the following way:

Set ONYXKEYS.IS_SEARCHING_FOR_REPORTS to false when the input is blank
Check the input to be blank – !searchInput.trim().length – so we don't show the spinner when entering a bunch of empty spaces

if (isNetworkOffline || !searchInput.trim().length) {
    Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false);
    return;
}

App/src/libs/actions/Report.js

Lines 2496 to 2507 in 39d2d98

    
           function searchInServer(searchInput) { 
        
               if (isNetworkOffline) { 
        
                   Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false); 
        
                   return; 
        
               } 
        
               // Why not set this in optimistic data? It won't run until the API request happens and while the API request is debounced 
        
               // we want to show the loading state right away. Otherwise, we will see a flashing UI where the client options are sorted and 
        
               // tell the user there are no options, then we start searching, and tell them there are no options again. 
        
               Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true); 
        
               debouncedSearchInServer(searchInput); 
        
           }

We should also remove all the if (text.length) ... checks from the pages where we use the Report.searchInServer(), as this will be handled inside the function.

Result

Screen.Recording.2023-11-24.at.09.55.16.mov

What alternative solutions did you explore? (Optional)

faisal-ibrahim · 2023-11-24T12:46:15Z

Proposal

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

The issue at hand (Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800) involves the delayed removal of a loader when search input emptied from the search field.

What is the root cause of that problem?

The root cause of the problem lies in the asynchronous nature of the API call triggered by the onChangeText function. When the user quickly emptied the input before the API call completes, the loader is not dismissed as expected.

onChangeText(searchValue = '') { 
     if (searchValue.length) { 
         Report.searchInServer(searchValue); 
     } 
  
     this.setState({searchValue}, this.debouncedUpdateOptions); 
 }

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

The quick workaround involves updating the Report.searchInServer function to check if the input value is empty before initiating the API call. If the input is empty, the loader display from the previous API call is immediately removed.

function searchInServer(searchInput) { 
    if (isNetworkOffline || searchInput.trim() === '') { 
        Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false); 
        return; 
    }

    // Set the loading state right away
    Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true); 
    debouncedSearchInServer(searchInput); 
}

We also need to eliminate the string length/empty checks from the calls to the Report.searchInServer method.
For example:

 onChangeText(searchValue = '') { 
     Report.searchInServer(searchValue); 
     this.setState({searchValue}, this.debouncedUpdateOptions); 
 }

This solution improves code readability and maintainability.

What alternative solutions did you explore? (Optional)

An alternative solution is proposed to optimize the process further by maintaining a hashmap of already searched terms. This avoids unnecessary API calls when the user repeats a search term, enhancing performance.

...
const alreadySearchedTerms = new Set();


....
function searchInServer(searchInput) { 
    if (isNetworkOffline || alreadySearchedTerms.has(searchInput.trim())) { 
        Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, false); 
        return; 
    }

    // Set the loading state right away
    Onyx.set(ONYXKEYS.IS_SEARCHING_FOR_REPORTS, true); 
    debouncedSearchInServer(searchInput); 
}

This alternative solution minimizes API calls by checking if the search term has been used before, providing a more efficient search mechanism. The hashmap is reset on page load to ensure the latest data is considered.

Note: This approach introduces a caching mechanism. A potential drawback of caching is that it may not always provide the most up-to-date data during the search period. However, if immediate data updates are crucial, then additional API call should also be triggered when the search input is empty to fetch the latest information.

melvin-bot · 2023-11-24T12:46:20Z

📣 @faisal-ibrahim! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

faisal-ibrahim · 2023-11-24T13:44:33Z

Contributor details
Your Expensify account email: faisal.im048@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/faisalibrahim

melvin-bot · 2023-11-24T13:44:35Z

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

JmillsExpensify · 2023-11-24T19:51:45Z

@jjcoffee thoughts on the proposals so far?

jjcoffee · 2023-11-27T10:22:18Z

Thanks for the proposals everyone! All of the proposed solutions here are workable, but I personally prefer @paultsimura's proposal as it makes the code more DRY by moving the repeated uses of if (searchValue.length) to a single use within searchInServer. I also like the extra bit of handling of multiple empty spaces.

🎀👀🎀 C+ reviewed

melvin-bot · 2023-11-27T10:22:26Z

Triggered auto assignment to @Gonals, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

DylanDylann · 2023-11-27T10:52:23Z

@jjcoffee The meaning of IS_SEARCHING_FOR_REPORTS field is checking whether if search API is executing or not. So I see that @paultsimura's proposal will make the meaning of this field become wrong and confusing. We shouldn't reset ONYXKEYS.IS_SEARCHING_FOR_REPORTS to false while the search API is executing.

cc @Gonals

ZhenjaHorbach · 2023-11-27T11:09:05Z

@jjcoffee The meaning of IS_SEARCHING_FOR_REPORTS field is checking whether if search API is executing or not. So I see that @paultsimura's proposal will make the meaning of this field become wrong and confusing. We shouldn't reset ONYXKEYS.IS_SEARCHING_FOR_REPORTS to false while the search API is executing.

cc @Gonals

Agree

jjcoffee · 2023-11-27T11:18:39Z

@DylanDylann I appreciate your point, however once we clear the input (or have empty spaces), the originally initiated searchInServer request isn't going to be displayed so in essence we are dismissing that request and from the FE's perspective nothing is loading (even if the actual API request is!), and we are not longer searching, so having IS_SEARCHING_FOR_REPORTS set to false makes perfect sense to me here.

DylanDylann · 2023-11-27T12:17:44Z

It's up to our expected. I think IS_SEARCHING_FOR_REPORTS is global variable, we should keep the meaning of this field as it's name.

JmillsExpensify · 2023-11-29T00:26:32Z

Working through proposals

jjcoffee · 2023-11-30T14:01:29Z

Just waiting on @Gonals to assign @paultsimura per this comment.

melvin-bot · 2023-11-30T16:00:08Z

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot · 2023-12-01T10:25:37Z

📣 @jjcoffee 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

melvin-bot · 2023-12-01T10:25:42Z

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

paultsimura · 2023-12-01T18:16:25Z

Thanks. The PR is ready for review: #32371

paultsimura · 2023-12-06T21:08:50Z

The PR was deployed to prod, but the automation did not trigger: #32371 (comment). Is this a false alert or a broken automation?

cc: @Gonals

jjcoffee · 2023-12-07T16:21:51Z

Had the same on a couple other issues on the same deploy so there must have been something up with the automation! @JmillsExpensify Could you add the Awaiting Payment label and update the title? Thanks!

paultsimura · 2023-12-11T18:29:03Z

@JmillsExpensify a friendly bump, please:

Could you add the Awaiting Payment label and update the title? Thanks!

JmillsExpensify · 2023-12-13T05:19:57Z

Payment summary:

Issue reporter: N/A
Contributor: $500 @paultsimura
Contributor+: $500 @jjcoffee

JmillsExpensify · 2023-12-13T16:34:53Z

No comments on the above, so I'm going to issue payments now.

JmillsExpensify · 2023-12-13T16:38:16Z

All contributors paid, though @jjcoffee would you mind completing the BZ checklist first so we can close this out?

jjcoffee · 2023-12-13T17:23:21Z

The PR that introduced the bug has been identified. Link to the PR: N/A - this is how Report.searchInServer has always worked
The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: N/A
A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
Determine if we should create a regression test for this bug. Yes
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Go to the Search
Enter a character and remove it
Verify that the loading spinner is removed almost immediately
Enter a set of empty spaces
Verify that the loader does not appear

Do we agree 👍 or 👎

jjcoffee · 2023-12-13T17:23:46Z

@JmillsExpensify Thanks for processing the payment, and apologies for missing the checklist. Done now!

JmillsExpensify · 2023-12-14T17:29:17Z

Thank you! All looks good. I'll get that regression test added.

lanitochka17 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 23, 2023

melvin-bot bot changed the title ~~Search - Delayed Removal of Loader When Removing a Character from the Search Field~~ [$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field Nov 23, 2023

melvin-bot bot assigned JmillsExpensify Nov 23, 2023

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

melvin-bot bot assigned jjcoffee Nov 23, 2023

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

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

melvin-bot bot assigned Gonals Nov 27, 2023

Gonals assigned paultsimura Dec 1, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 1, 2023

paultsimura mentioned this issue Dec 1, 2023

fix: Discard loading state on empty search request #32371

Merged

47 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Dec 1, 2023

JmillsExpensify closed this as completed Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800

[$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800

lanitochka17 commented Nov 23, 2023 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Nov 23, 2023

melvin-bot bot commented Nov 23, 2023

melvin-bot bot commented Nov 23, 2023

melvin-bot bot commented Nov 23, 2023

FitseTLT commented Nov 23, 2023 •

edited

Loading

ZhenjaHorbach commented Nov 23, 2023 •

edited

Loading

DylanDylann commented Nov 24, 2023 •

edited

Loading

paultsimura commented Nov 24, 2023 •

edited

Loading

faisal-ibrahim commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

faisal-ibrahim commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

JmillsExpensify commented Nov 24, 2023

jjcoffee commented Nov 27, 2023

melvin-bot bot commented Nov 27, 2023

DylanDylann commented Nov 27, 2023 •

edited

Loading

ZhenjaHorbach commented Nov 27, 2023

jjcoffee commented Nov 27, 2023

DylanDylann commented Nov 27, 2023

JmillsExpensify commented Nov 29, 2023

jjcoffee commented Nov 30, 2023

melvin-bot bot commented Nov 30, 2023

melvin-bot bot commented Dec 1, 2023

melvin-bot bot commented Dec 1, 2023

paultsimura commented Dec 1, 2023

paultsimura commented Dec 6, 2023

jjcoffee commented Dec 7, 2023 •

edited

Loading

paultsimura commented Dec 11, 2023

JmillsExpensify commented Dec 13, 2023

JmillsExpensify commented Dec 13, 2023

JmillsExpensify commented Dec 13, 2023

jjcoffee commented Dec 13, 2023

jjcoffee commented Dec 13, 2023

JmillsExpensify commented Dec 14, 2023

[$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800

[$500] Search - Delayed Removal of Loader When Removing a Character from the Search Field #31800

Comments

lanitochka17 commented Nov 23, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Nov 23, 2023

melvin-bot bot commented Nov 23, 2023

melvin-bot bot commented Nov 23, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Nov 23, 2023

FitseTLT commented Nov 23, 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)

ZhenjaHorbach commented Nov 23, 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)

DylanDylann commented Nov 24, 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)

paultsimura commented Nov 24, 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?

Result

What alternative solutions did you explore? (Optional)

faisal-ibrahim commented Nov 24, 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 24, 2023

faisal-ibrahim commented Nov 24, 2023

melvin-bot bot commented Nov 24, 2023

JmillsExpensify commented Nov 24, 2023

jjcoffee commented Nov 27, 2023

melvin-bot bot commented Nov 27, 2023

DylanDylann commented Nov 27, 2023 • edited Loading

ZhenjaHorbach commented Nov 27, 2023

jjcoffee commented Nov 27, 2023

DylanDylann commented Nov 27, 2023

JmillsExpensify commented Nov 29, 2023

jjcoffee commented Nov 30, 2023

melvin-bot bot commented Nov 30, 2023

melvin-bot bot commented Dec 1, 2023

melvin-bot bot commented Dec 1, 2023

paultsimura commented Dec 1, 2023

paultsimura commented Dec 6, 2023

jjcoffee commented Dec 7, 2023 • edited Loading

paultsimura commented Dec 11, 2023

JmillsExpensify commented Dec 13, 2023

JmillsExpensify commented Dec 13, 2023

JmillsExpensify commented Dec 13, 2023

jjcoffee commented Dec 13, 2023

jjcoffee commented Dec 13, 2023

JmillsExpensify commented Dec 14, 2023

lanitochka17 commented Nov 23, 2023 •

edited by melvin-bot bot

Loading

FitseTLT commented Nov 23, 2023 •

edited

Loading

ZhenjaHorbach commented Nov 23, 2023 •

edited

Loading

DylanDylann commented Nov 24, 2023 •

edited

Loading

paultsimura commented Nov 24, 2023 •

edited

Loading

DylanDylann commented Nov 27, 2023 •

edited

Loading

jjcoffee commented Dec 7, 2023 •

edited

Loading