[WAITING ON CHECKLIST] [$250] Multiple requests to SearchForReport when searching for a user

[m-natarajan]

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.63-11
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: @iwiznia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1713544197266279

Action Performed:

1. Click the search icon from the LHN
2. Open network tab
3. Type something in the search bar

Expected Result:

Shouldn't be numerous requests to SearchForReport

Actual Result:

Numerous requests to SearchForReport is sent

For proposals - see this comment:

We should either add or change the code so that it debounces the calls in a "sane" manner, probably the same as we did for the userIsTyping events

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

Add any screenshot/video evidence

Recording.2998.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c8c8aa1341923a49
• Upwork Job ID: 1782550098085855232
• Last Price Increase: 2024-04-29
• Automatic offers:
• ahmedGaber93 | Reviewer | 0
• tienifr | Contributor | 0

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[ShridharGoel]

Proposal

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

What is the root cause of that problem?

This is not a bug, but we can make some improvements.

When the input text is changed, a new API call is made to get the new data based on the input.

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

As of now, the debounce time that is being used is 300 ms. We can change it to a higher value like 400 ms or 500 ms for the report search by passing the value in delay of useDebouncedState via ChatFinderPage.

Another thing can be to cancel the previous API calls when a new call is made. So, if the input text is changed and a new call is made, then all previous search API calls which haven't completed will be cancelled. This can be implemented using AbortController.

[iwiznia]

This was omitted from the issue description:

We should either add or change the code so that it debounces the calls in a "sane" manner, probably the same as we did for the userIsTyping events

Please take it into consideration for the proposals.

[jliexpensify]

Whoop sorry, missed this one yesterday - I can repro.

[melvin-bot]

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

[melvin-bot]

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

[tienifr]

Proposal

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

Numerous requests to SearchForReport is sent

What is the root cause of that problem?

In here, we're using debouncing to update the search value, so if the user stops and continues typing after 300ms, it will result in 2 API calls. For each 300ms delay after continuing typing, there'll be an API call.

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

The 300ms debounce looks too low, since it's common that the user will stop for 300ms between each word of typing.

We should:

1. Increase the debounce timing to around 1 or 1.5 seconds (this debounce time can be adjusted to lower values according to requirement). FYI 1.5 seconds is the userIsTyping event timeout (see here), the userIsTyping event was referred to in the OP.

We should either add or change the code so that it debounces the calls in a "sane" manner, probably the same as we did for the userIsTyping events

This can be done by adding the second param for the delay here

Update: Since this debounced value will affect the live filtering debounce too, we might want to separate the debounceValue for searchInServer (500ms debounce) and debounceValue for live filtering and other operations (300ms). More details here.

The steps to do this:

• Add a new useDebouncedState for the server search

const [_, debouncedSearchValueInServer, setSearchValueInServer] = useDebouncedState('', 500);

• When setting the search value here and here, call setSearchValueInServer too, we can add a new method updateSearchValue to update both the debounced value for filtering and for server search

const updateSearchValue = useCallback((value: string) => {
    setSearchValue(value);
    setSearchValueInServer(value);
}, [setSearchValue, setSearchValueInServer]);

Then use it in 2 places above

• In here, use debouncedSearchValueInServer instead

useEffect(() => {
    Report.searchInServer(debouncedSearchValueInServer.trim());
}, [debouncedSearchValueInServer]);

An alternative is we can refactor the useDebouncedState to accept an array of delays, and returns an array of debounced value associated with each of those delays.

Another simple way to do this is to use useDebounce like here for the search in server operation, with a 500ms debounce. This way, we don't have to maintain a separate state for the searchValueInServer.

We might want to consider having no debounce for live filtering, but there might be performance issues as mentioned here.

2. After this is done, there's a problem. Now when the user finishes typing, they will always have to wait for 1/1.5 seconds for the search to begin and the loading indicator to show, even if they press Enter to submit the input.

This is because we're always waiting for the debounce value, without regards to if the user manually submits the input or not.

The ideal UX in this case would be to start the search immediately after the user submits the input (eg. via Enter), because this means the user is done with typing and wants the result immediately.

To do this, we need to:

• Add and expose a method flushDebounce from useDebouncedState, which when called, will flush the value debounce and set the state immediately

const flushDebounce = () => {
    debouncedSetDebouncedValue.flush();
}

Call flushDebounce when users submit the input (when there's no focused item). This will make sure the search will begin immediately when the user wants it to.

What alternative solutions did you explore? (Optional)

We can abort existing SearchForReport API call when the user starts typing more, because another API call will be made very soon after the debounce timing. But this won't reduce the load on the back-end since the API request already reaches the back-end.

[jliexpensify]

@ahmedGaber93 bump as we have 2 proposals

[ahmedGaber93]

Reviewing today.

[ahmedGaber93]

@ShridharGoel @tienifr Thanks for the proposal.

The both proposals suggest increasing debounce time, and @tienifr proposal point 2 suggest UX changes that allow the user to submit the input to start search immediately.

I think increasing debounce time to 1000ms like isUserTyping will reduce API calls, but also the live search result will be more late, but I think we can accept that for more API performance, and we're already displaying the saved data in onyx storage without needing to wait to API response.

The UX changes by @tienifr proposal point 2 is not the best for search immediately purpose, it should be for full result purpose and in most cases submit action will not affect on the result.

@iwiznia There is another behavior appear after increasing the time, the result for the saved data in onyx storage will also debounce because filtering data depend on debouncedSearchValue change. Should we also solve this?

Screen.Recording.2024-04-25.at.7.48.39.AM.mov

[iwiznia]

I think increasing debounce time to 1000ms like isUserTyping will reduce API calls, but also the live search result will be more late,

I don't think that's true, that's what leading is for:

App/src/pages/home/report/ReportActionCompose/ComposerWithSuggestions/ComposerWithSuggestions.tsx

Line 196 in 64695c8

leading: true,

the result for the saved data in onyx storage will also debounce because filtering data depend on debouncedSearchValue change.

Don't get why this is bad, what are the consequences of it?

[ahmedGaber93]

Don't get why this is bad, what are the consequences of it?

@iwiznia Filtering the data every 1000ms seem bad UX, for example, if we have 4 items a1, a2, b1, b2.

• when user opens the search page, he will see the 4 items by default.
• if user type a in the search, he will still see the 4 items a1, a2, b1, b2 for 1000ms util filtering data run, and then he will see the filtered result a1, a2 which seem slow response as the 4 items already displayed for the user but filtering wait 1000ms to run.

I don't think this is worth, so I asked you.

[ahmedGaber93]

I don't think that's true, that's what leading is for:

@iwiznia I guess your response is for this part but also the live search result will be more late, if that as I understand from the debounce docs and testing it, leading will affect only on the first debounce call (leading edge).

If user start typing and type first letter a, the search API will run immediately without waiting (leading edge), but after that typing any search value will wait the delay time then call search API.

So I think the live search result from API will be more late than current.

[iwiznia]

Yeah,1s might not be the best value to use as it feels quite high.

[ahmedGaber93]

@iwiznia I think the only available improvement right now is increasing the debounce time, can we move forward with this improvement? Or this not enough?

If we can move forward, I think 500ms may be good.

[tienifr]

@iwiznia There is another behavior appear after increasing the time, the result for the saved data in onyx storage will also debounce because filtering data depend on debouncedSearchValue change. Should we also solve this?

@ahmedGaber93 I believe we can avoid this by having a separate debounce time for API call (1s) and keeping the current debounce time for filtering as is (300ms, so it's interactive right after the user types). We don't have to make them the same value.

Then we have the best of both worlds here. What do you think?

[iwiznia]

Sounds good. Still not sure if 1s is the best number... doing some live tests will probably help out to know.

[tienifr]

Still not sure if 1s is the best number... doing some live tests will probably help out to know.

@iwiznia Since the backend search API call itself now takes around 2.5s - 4s, I think "delaying calling the API" for an additional few hundreds milliseconds won't make a difference to real users.

As long as the live search is smooth (for which we will keep the current debounce time), I think 1s is ok for the delay of the API call.

[ahmedGaber93]

@ahmedGaber93 I believe we can avoid this by having a separate debounce time for API call (1s) and keeping the current debounce time for filtering as is (300ms, so it's interactive right after the user types). We don't have to make them the same value.

@tienifr yes, I think you need to follow that if we decide to use long debounce time for API >= 1s.

[melvin-bot]

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

[melvin-bot]

@jliexpensify @techievivek @ahmedGaber93 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[jliexpensify]

Not overdue

[melvin-bot]

📣 @ahmedGaber93 🎉 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]

📣 @tienifr 🎉 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 📖

[melvin-bot]

This issue has not been updated in over 15 days. @jliexpensify, @techievivek, @ahmedGaber93, @tienifr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.77-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Performance: reduce SearchForReport requests #41729

If no regressions arise, payment will be issued on 2024-06-06. 🎊

For reference, here are some details about the assignees on this issue:

• @ahmedGaber93 requires payment automatic offer (Reviewer)
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR:
• [@ahmedGaber93] 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:
• [@ahmedGaber93] 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:
• [@ahmedGaber93] Determine if we should create a regression test for this bug.
• [@ahmedGaber93] 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.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[jliexpensify]

Payment Summary

• C: @tienifr $250
• C+: @ahmedGaber93 $250

Upwork job

Just waiting on the checklist!

[jliexpensify]

Bump @ahmedGaber93 for the checklist!

[jliexpensify]

Payments made and job closed.

[ahmedGaber93]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ahmedGaber93] The PR that introduced the bug has been identified. Link to the PR: N/A
• [@ahmedGaber93] 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
• [@ahmedGaber93] 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
• [@ahmedGaber93] Determine if we should create a regression test for this bug.
• [@ahmedGaber93] 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.

This issue is for improving performance and reduce API calls, So I think there is no offending PR, and need for regression test.