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

[Performance] Simplify isOneOnOneChat Logic and Improve Performance in getParticipantsAccountIDsForDisplay #47336

Closed
gedu opened this issue Aug 13, 2024 · 14 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2

Comments

@gedu
Copy link
Contributor

gedu commented Aug 13, 2024

Problem

The current implementation of the isOneOnOneChat function uses a complex approach involving mapping and filtering over the participants in the report object. This includes parsing string keys to numbers and filtering out the current user, which can be streamlined for better readability and performance. Additionally, the getParticipantsAccountIDsForDisplay function includes extra mapping and string-to-number parsing inside filters, leading to inefficiencies.

Solution

Refactor the isOneOnOneChat function to a simpler approach that avoids unnecessary mapping and filtering. Instead, directly manipulate the participant count and presence of the current user to determine if the chat is one-on-one. Likewise, improve the getParticipantsAccountIDsForDisplay function by moving the string-to-number parsing out of the filter to increase efficiency.

@gedu
Copy link
Contributor Author

gedu commented Aug 13, 2024

Hi, I'm Edu from Callstack and I want to work on this issue

Copy link

melvin-bot bot commented Aug 14, 2024

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

@Beamanator
Copy link
Contributor

discussing a bit in slack here

@ZhenjaHorbach
Copy link
Contributor

@Beamanator
I have a small question 😅

Since I reviewed this performance issue, am I eligible for a payment?

And If yes

Looks like production deploy automation failed: This should be on [HOLD for Payment 2024-09-06] according to prod deploy checklist, confirmed in slack

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

This issue has not been updated in over 15 days. @Beamanator 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 melvin-bot bot added the Monthly KSv2 label Sep 9, 2024
@Beamanator Beamanator added Weekly KSv2 Bug Something is broken. Auto assigns a BugZero manager. and removed Reviewing Has a PR in review Monthly KSv2 labels Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

Triggered auto assignment to @OfstadC (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.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 9, 2024
@Beamanator
Copy link
Contributor

@gedu any update on the discussions you linked here? #47300 (comment)

@ZhenjaHorbach good call, i do believe you deserve payment! I think we can wait to see if the performance issue caused a regression or not before doing payment 🙏

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Sep 9, 2024

@gedu any update on the discussions you linked here? #47300 (comment)

@ZhenjaHorbach good call, i do believe you deserve payment! I think we can wait to see if the performance issue caused a regression or not before doing payment 🙏

Thanks !
And I don't mind waiting a little longer 😃
Although this PR has been in production for almost 2 weeks now

@OfstadC
Copy link
Contributor

OfstadC commented Sep 11, 2024

@Beamanator Let me know what you need from me here 😃

@Beamanator
Copy link
Contributor

I ping'd @gedu in the PR here, I think it makes sense to pay this out since, as @ZhenjaHorbach mentioned, the PR has been in prod for 2 weeks 🙏

@gedu
Copy link
Contributor Author

gedu commented Sep 12, 2024

The PR is merged and without regressions, should be all good, @Beamanator replied on the PR

@OfstadC
Copy link
Contributor

OfstadC commented Sep 12, 2024

@ZhenjaHorbach Could you please accept the offer here? Then I can issue payment 😃

@ZhenjaHorbach
Copy link
Contributor

@ZhenjaHorbach Could you please accept the offer here? Then I can issue payment 😃

Done !

@OfstadC
Copy link
Contributor

OfstadC commented Sep 12, 2024

Payment Summary

@OfstadC OfstadC closed this as completed Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2
Projects
None yet
Development

No branches or pull requests

4 participants