Remove duplicates from the new Group and Workspace Invite page #7986
Conversation
| @@ -108,17 +108,21 @@ class NewChatPage extends Component { | |||
| } | |||
| } | |||
|
|
|||
| const filterText = _.reduce(this.state.selectedOptions, (str, {login}) => `${str} ${login}`, ''); | |||
There was a problem hiding this comment.
This just takes all the filtered users and puts their name in a big string right? How come we do this over just putting them in an array and doing .includes?
There was a problem hiding this comment.
This O(n). That would be O(n*n)
There was a problem hiding this comment.
How come? I wasn't able to find details about runtime complexity about string.includes() but I assume string.includes() in the worst case still searches the entire string that includes all the names
I found this SO on array.includes() which says it uses linear search and so the worst scenario for finding something is O(n).
So if that is the case the runtime wouldn't be different
There was a problem hiding this comment.
Ok. I will do this. It's no biggy. It can never hurt the app performance in any way.
There was a problem hiding this comment.
but this is the breakout:
=> 111 O(n)
=> 112 O(n)
= O(n).
New changes.
_.filter(this.state.recentReports, ({login}) => !this.state.selectionOptions.find({slogin} => login === slogin));There are two nested loops thus O(n*n).
There was a problem hiding this comment.
Do you still think change is needed? Let me know.
There was a problem hiding this comment.
Thanks for exploring this with me. I think they're both O(n*n) since _.reduce is also O(n) along with the string.includes O(n).
Though my biggest concern about it was that using a space delimiter is that it might lead to some edge cases with spaces in logins versus something more explicit like an array. But after thinking about it some more this might not be that big of a deal since the logins are emails/numbers
There was a problem hiding this comment.
That being said, thanks again for looking into this but I think what you have here will be okay for our use case
There was a problem hiding this comment.
Yeah. Overlooked it. Thanks for understanding. I will update the screenshot in few mintues.
|
To push this forward, I just need the IOS screenshot and for the iOS tests to pass |
|
@thienlnam Updated. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @thienlnam in version: 1.1.42-0 🚀
|
|
🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀
|
|
The formation of string with delimiter caused an regression here - #21783 |
|
That's how the search was designed. |
Details
Fixed Issues
$ #5768
Tests
PR Review Checklist
Contributor (PR Author) Checklist
mainbefore submitting my PR for review### Fixed Issuessection abovesrc/languages/*files (if applicable)Styling.md) for all style edits I madePR Reviewer Checklist
mainbefore submitting the PR### Fixed Issuessection abovesrc/languages/*files (if applicable)QA Steps
Open the New Group page.
Select a few (two) users.
Search for some text that results in those two users.
Observe that when the search matches with any of the selected contacts
No Result foundmessage is not shown.Observe that when the search does not match with any of the selected contacts and there are no search results, the
No Result foundmessage is shown.Repeat the step on Workspace Member invite page.
Run the above tests on each platform on staging.
Tested On
Screenshots
Web | Desktop
Mobile Web
iOS
Android