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

[HOLD] Revert "[TS migration] Migrate 'PersonalDetailsUtils.js' lib to TypeScript" #33438

Closed

Conversation

iwiznia
Copy link
Contributor

@iwiznia iwiznia commented Dec 21, 2023

@iwiznia iwiznia requested a review from a team as a code owner December 21, 2023 18:38
@melvin-bot melvin-bot bot requested review from jasperhuangg and removed request for a team December 21, 2023 18:39
Copy link

melvin-bot bot commented Dec 21, 2023

@jasperhuangg Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@paultsimura
Copy link
Contributor

@iwiznia could you please share a piece of context here as well?🙏🏼
I don't have access to that channel apparently, but have made changes to this TS file in my another PR that's under review currently

@muttmuure
Copy link
Contributor

Performance in heavy accounts WITH lots of personal details (aka internal employee accounts) has severely degraded since this migration hit staging 3 days ago.

Performance in heavy accounts WITHOUT lots of personal details appears to be unaffected by the performance regression.

@jasperhuangg
Copy link
Contributor

@iwiznia there appear to be conflicts now, can you sort those out?

@jasperhuangg
Copy link
Contributor

Heading OOO for the holidays, so it might be best to assign another engineer

@MonilBhavsar
Copy link
Contributor

Conflict resolved. Let's first confirm with the build if this revert actually fixes the issue

@MonilBhavsar MonilBhavsar changed the title Revert "[TS migration] Migrate 'PersonalDetailsUtils.js' lib to TypeScript" [HOLD] Revert "[TS migration] Migrate 'PersonalDetailsUtils.js' lib to TypeScript" Dec 22, 2023
@muttmuure
Copy link
Contributor

@TMisiukiewicz please could you work with @MonilBhavsar on this today?

@fabioh8010
Copy link
Contributor

@BartoszGrajdek Tagging you as you was the author of the PR.

@MonilBhavsar
Copy link
Contributor

Not sure why it's not building. I'll run this against prod DB on my account and see if it fixes the issue

@mountiny
Copy link
Contributor

Running the build

@OSBotify
Copy link
Contributor

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖 iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/33438/index.html ❌ FAILED ❌
Android The QR code can't be generated, because the iOS build failed
Desktop 💻 Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/33438/NewExpensify.dmg https://33438.pr-testing.expensify.com
Desktop Web

@iwiznia iwiznia requested a review from a team December 22, 2023 19:14
@melvin-bot melvin-bot bot removed the request for review from a team December 22, 2023 19:14
Copy link

melvin-bot bot commented Dec 22, 2023

@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@iwiznia iwiznia requested review from a team and removed request for jasperhuangg December 22, 2023 19:15
@melvin-bot melvin-bot bot requested review from hayata-suenaga and removed request for a team December 22, 2023 19:15
Copy link

melvin-bot bot commented Dec 22, 2023

@hayata-suenaga Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@situchan
Copy link
Contributor

Confused which one is the exact offending PR.
To fix #33326, 2 PRs were already reverted today.
Or is this to fix any other performance regression other than #33326?

@BartoszGrajdek
Copy link
Contributor

Hi, author of the PR here 😅 As you can guess I'm not really a fan of having it reverted, so I'd love to pinpoint what exactly is causing the performance issue.

My guess is that it's most likely due to some of the underscore/lodash removals, if you could provide some reproduction steps I would love to help out 🙏🏻

@iwiznia
Copy link
Contributor Author

iwiznia commented Dec 22, 2023

I don't know the steps exactly. I think you have to have lots of people you know, so the list is large.

@muttmuure can you provide @BartoszGrajdek with steps to reproduce the issue please?

@situchan
Copy link
Contributor

cc: @mountiny as you're author of this revert

@muttmuure
Copy link
Contributor

The repro steps so far are:

  1. Have an account with access to lots of personal details (ex. first name, last name)
  2. Open search
  3. The app (so far on web, mobile, desktop, mWeb) hangs and eventually crashes

@BartoszGrajdek
Copy link
Contributor

So I guess that high-traffic account won't be enough for this, right? Since you can see only email address of the contact

@mountiny
Copy link
Contributor

@iwiznia i think we might be able to close this one in case this was resolved by this revert pr #33496 which was hard to confirm because people are out but from what Callstack developer suggested the new phone number methods/regexp just make the search page explode if you have thousands of personal details with phone number as primary login

@iwiznia
Copy link
Contributor Author

iwiznia commented Dec 26, 2023

Oh wow! That's nice to hear, closing then.

@iwiznia iwiznia closed this Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants