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

Debounce search page and reduce number of tooltips created #3669

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

marcaaron
Copy link
Contributor

Details

Noticed we were creating a ton of tooltips for the "rooms" that are returning in search results. Sometimes 100+ tooltips which caused performance to degrade. Also added some debounce in since we are filtering the options rather often and triggering re-renders. When we can wait until the user is done typing to do this.

Fixed Issues

Fixes #3601

Tests

QA Steps

  1. Command + k to bring up search function
  2. Verify typing is smooth
  3. Verify search results appear reasonably fast

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Jun 18, 2021
@marcaaron marcaaron requested a review from a team as a code owner June 18, 2021 00:54
@MelvinBot MelvinBot requested review from robertjchen and removed request for a team June 18, 2021 00:55
Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great fix! 👍

@robertjchen robertjchen merged commit ec5c4b5 into main Jun 18, 2021
@robertjchen robertjchen deleted the marcaaron-improveSearchSpeed branch June 18, 2021 09:15
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@Julesssss
Copy link
Contributor

Hey @marcaaron, I believe this line introduced a regression to the IOU Request flow. If you create an IOU from within a DM, participantList is null, causing a crash.

@Julesssss
Copy link
Contributor

Here's the issue, I've assigned it to you as I'm about to end my day. Feel free to assign it to me for Monday if you aren't able to pick this up.

@marcaaron marcaaron added the DeployBlockerCash This issue or pull request should block deployment label Jun 18, 2021
@roryabraham roryabraham added DeployBlockerCash This issue or pull request should block deployment and removed DeployBlockerCash This issue or pull request should block deployment labels Jun 18, 2021
@marcaaron
Copy link
Contributor Author

Fix was merged #3675

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.71-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Julesssss
Copy link
Contributor

Julesssss commented Jun 21, 2021

Thanks for fixing 👍

I was a bit unsure about the deployBlocker label in this case, as this PR hadn't yet been deployed. Does it make sense to apply the label to an issue that hasn't yet been deployed to staging? Wouldn't that have the potential to block an in-progress release candidate for a deployBlocker issue that isn't part of that release? Or are we excluding deployBlocker issues that are not yet on staging?

CC @roryabraham

@roryabraham
Copy link
Contributor

The deployBlocker label doesn't actually "physically" block anything – the only way we check to see if all deploy blockers are resolved is by looking at the issue body for the open StagingDeployCash. If there are any unchecked boxes, then we'll block the production deploy.

So it's safe to add the DeployBlockerCash label to a PR that has not yet been deployed. If another staging or production deploy happens, then the deploy blocker should be added to the checklist. Does that make sense?

@Julesssss
Copy link
Contributor

Awesome, yes it does. Thanks for explaining!

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.73-3🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@fedirjh
Copy link
Contributor

fedirjh commented Apr 22, 2023

Hey there, I want to point out that this PR introduced a regression in #17200 where the search list displays an invalid phone number error but still displays the phone number in the result for a brief moment. The headerMessage should’ve been debounced with debouncedUpdateOptions which would ensure data consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Performance] CMD + K is slow and laggy
6 participants