Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LOW: Implement filtering for search pages. #37619

Closed
TMisiukiewicz opened this issue Mar 1, 2024 · 56 comments
Closed

LOW: Implement filtering for search pages. #37619

TMisiukiewicz opened this issue Mar 1, 2024 · 56 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering NewFeature Something to build that is a new item. Weekly KSv2

Comments

@TMisiukiewicz
Copy link
Contributor

TMisiukiewicz commented Mar 1, 2024

Problem

Based on our measurements in high-traffic accounts, the search page takes an average of 4100ms to re-render on every keystroke. This is a problem because the user will notice a significant lag between changing their search term and the results list updating.
The root cause of this problem is getSearchText, which is called for every keystroke and is inefficient because it reconstructs all the options in the results list using createOption over and over.

Solution

Let's speed up getSearchText by refactoring it such that, instead of re-generating all the result options every time the search terms change, we just generate the options once and filter them based on the current search term.
To achieve this, we recommend using a rank-value sorting, where a higher number means it will appear higher in the list:

  • Acronym - item’s acronym is the given value
  • Contains - item contains the given value
  • Word starts with - if one of the item’s words starts with the given value
  • Starts with - if the item starts with the given value
  • Equals - case-insensitive equality
  • Case sensitive - case-sensitive equality

Note that this will maintain the current search result ordering, just implement it in a different way. By making this change, we can improve the following metrics:

  • open_search → Before: ~5900ms → After: ~4900ms (~1000ms improvement)
  • load_search_options → Before: ~4100ms → After: ~3200ms (~900ms improvement)
  • search query changed after initial load → Before: 4100ms → After: 350ms (~12x improvement)

We can apply this technique everywhere in the app where we use a search bar to filter option results.

Issue OwnerCurrent Issue Owner: @stephanieelliott
Copy link

melvin-bot bot commented Mar 1, 2024

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

@roryabraham roryabraham added the NewFeature Something to build that is a new item. label Mar 1, 2024
Copy link

melvin-bot bot commented Mar 1, 2024

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Mar 1, 2024
@roryabraham
Copy link
Contributor

Making @TMisiukiewicz the issue owner here since for now we're just awaiting a PR

@TMisiukiewicz
Copy link
Contributor Author

Already incorporated the algorithm into the codebase and created an util responsible for filtering, now working on implementing it into the first search page

@TMisiukiewicz
Copy link
Contributor Author

I just opened a draft PR with an example implementation in Search Page, I would be more than happy to get some initial feedback! @roryabraham how should we approach implementing it in other search pages? Should we implement filtering in all required pages with this single PR or should we get this one merged first and then implement each one separately?
Once we migrate each search page to use filtering, we will be able to stop using searchText property which will improve the performance of generating options

@roryabraham
Copy link
Contributor

how should we approach implementing it in other search pages? Should we implement filtering in all required pages with this single PR or should we get this one merged first and then implement each one separately?

let's definitely break it up into a series of smaller PRs if there's a straightforward way to do that. It might feel like more work but things always tend to go smoother this way 🙂

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Mar 12, 2024
@DylanDylann
Copy link
Contributor

@TMisiukiewicz Could you help to make a video about this problem on staging?

I also see this problem

the search page takes an average of 4100ms to re-render on every keystroke.

But It seems the RCA is from SearchForReports API takes a long time to respond

Screen.Recording.2024-03-13.at.16.51.08.mov

Please correct me if I missed anything

@TMisiukiewicz
Copy link
Contributor Author

hi @DylanDylann, the server search is not related to the changes made in this PR. This PR improves the performance of the local search by replacing options generation with filtering. On staging, you can enable showing verbose logs in the console and look for the messages: Timing:staging.new.expensify.load_search_options, when you type in the search input, it will give you the amount of time it takes to load the options that should be visible on the list. When testing this PR, this message won't be displayed after typing because it's not re-generating the options anymore - results should be displayed quicker.

Staging:

Screen.Recording.2024-03-13.at.11.42.32.mov

Adhoc:

Screen.Recording.2024-03-13.at.11.45.05.mov

As you can see in the Adhoc build, the load_search_options is registered once server search is resolved, not when user stops typing (it's probably because response from the server updates some reports and there is a need of generating it once again)

@DylanDylann
Copy link
Contributor

@TMisiukiewicz Thanks. I see that in your PR.

@stephanieelliott
Copy link
Contributor

PR is moving along, seems like we are ready to test 🚀

@stephanieelliott
Copy link
Contributor

PR is waiting on your review @roryabraham!

@roryabraham
Copy link
Contributor

Sorry for the delay. I'll try to review tomorrow. It's a large PR so something I need to carve out a bit more time for

@stephanieelliott
Copy link
Contributor

PR is being actively reviewed

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

This issue has not been updated in over 15 days. @TMisiukiewicz, @stephanieelliott, @roryabraham 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!

@TMisiukiewicz
Copy link
Contributor Author

Since we already have first filtering implemented, I'll be moving forward with using it in the rest of search pages. I'll create separate PR for each one of them

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jun 25, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. Jun 25, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jun 25, 2024
Copy link

melvin-bot bot commented Jun 25, 2024

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

Copy link

melvin-bot bot commented Jun 25, 2024

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

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

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

Copy link

melvin-bot bot commented Jun 25, 2024

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

  • [@TMisiukiewicz] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 3, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. Jul 3, 2024
Copy link

melvin-bot bot commented Jul 3, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 3, 2024

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

  • [@TMisiukiewicz] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 10, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. [HOLD for payment 2024-07-17] [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 10, 2024

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

  • [@TMisiukiewicz] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Jul 15, 2024
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2024-07-17] [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. [HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. Jul 15, 2024
Copy link

melvin-bot bot commented Jul 15, 2024

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

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

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

Copy link

melvin-bot bot commented Jul 15, 2024

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

  • [@TMisiukiewicz] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@roryabraham roryabraham changed the title [HOLD for payment 2024-07-22] [HOLD for payment 2024-07-17] [HOLD for payment 2024-07-10] [HOLD for payment 2024-07-02] [HOLD for payment 2024-06-11] LOW: Implement filtering for search pages. LOW: Implement filtering for search pages. Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering NewFeature Something to build that is a new item. Weekly KSv2
Projects
Development

No branches or pull requests

6 participants