Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.46k stars 2.82k forks source link

[HOLD for payment][Bug] After typing even a mildly long comment in the Android compose box, the whole app starts to hang #11321

Closed mvtglobally closed 1 year ago

mvtglobally commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open a chat -- even an empty one
  2. Just start typing a ton on the keyboard. Not copy/pasting, just finger-smash the keyboard for about 30s

Expected Result:

App should respond quickly

Actual Result:

After typing even a mildly long comment in the Android compose box, the whole app starts to hang.

Workaround:

unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.3-0 Reproducible in staging?: Reproducible in production?: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation image - 2022-09-27T002415 487

Expensify/Expensify Issue URL: Issue reported by: @JmillsExpensify @quinthar Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1663172358453739

View all open jobs on GitHub

strepanier03 commented 1 year ago

Latest on this in Slack HERE

2022-11-09_12-15-35
kadiealexander commented 1 year ago

See above, not overdue.

hannojg commented 1 year ago

Hey guys, going to continue looking for a fix today. So we just know the cause of the issue, but don't have a fix yet. Working on it 😼 😊 !

melvin-bot[bot] commented 1 year ago

@Beamanator, @hannojg, @sobitneupane, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

Beamanator commented 1 year ago

@hannojg still working on a fix! 👍

JmillsExpensify commented 1 year ago

On that note, @hannojg it would be good to circle back on the plan from here. I don't see a PR linked to this issue, are you working on that somewhere else, or are you still trying to nail down the exact fix?

hannojg commented 1 year ago

Hey, I am still nailing down a fix. The issue is present in react-native (so not specific to expensify code usage), and it might be an android bug, so I will have to create an issue ticket. Will link PRs and external issues as soon as I am there, but so far there are no code changes to expensify that I could share. Still working on it and will provide results today! 😊

hannojg commented 1 year ago

Here is the react native issue: https://github.com/facebook/react-native/issues/35350

Will now check whether this is to fix in react native's code or whether its an android bug.

JmillsExpensify commented 1 year ago

Nice thanks for the update here and in Slack as well. Forward we go!

hannojg commented 1 year ago

Update: I am still looking for a fix within the react-native source code. I don't think it's an android native bug, as I am unable to reproduce the issue just by using android native. Technical-specific updates can be found in the named RN issue ✌️

hannojg commented 1 year ago

Okay, I have a proposal available for a workaround until I get to fix the issue upstream at react native. The workaround is a bit ugly, and involves changing the react-native source code … however I am a bit uncertain how long it will take to get the issue fixed upstream, so I think it might be worth it. Until then I think the workaround will be valuable enough (android 13 users can't really use the app rn).

Will put up a PR in a moment …

quinthar commented 1 year ago

It's 100% fine to fix our fork of RN; we don't really care if it gets merged upstream. We would always prefer to fix the bug where it actually happens, even if it's never merged into mainline.

On Wed, Nov 16, 2022 at 2:29 PM Hanno J. Gödecke @.***> wrote:

Okay, I have a proposal available for a workaround until I get to fix the issue upstream at react native. The workaround is a bit ugly, involves changing the react-native source code … BUT I am a bit uncertain how long it will take to get the issue fixed upstream. Until then I think the workaround will be valuable enough (android 13 users can't really use the app rn).

Will put up a PR in a moment …

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1317759106, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUSDCKPN4PDPU77MZ4DWIVN3RANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

hannojg commented 1 year ago

Yes, a 100%, it's just that the workaround isn't a fix. For the fix itself, I have to dive deep into react native's code. However, for the workaround, I still need to modify some react-native code - so that's only temporarily until we have a proper fix.

quinthar commented 1 year ago

to clarify, do you have an actual fix? Why do the workaround?

On Wed, Nov 16, 2022 at 2:35 PM Hanno J. Gödecke @.***> wrote:

Yes, a 100%, it's just that the workaround isn't a fix. For the fix itself, I have to dive deep into react native's code. However, for the workaround, I still need to modify some react-native code - so that's only temporarily until we have a proper fix.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1317764812, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUQYCZT7O3ELSYKTU4TWIVOR3ANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

hannojg commented 1 year ago

No, i don't have a proper fix yet. We can also wait until I have a proper fix.

quinthar commented 1 year ago

Oh if we don't have a fix, then yes, let's do the workaround.

On Wed, Nov 16, 2022 at 2:39 PM Hanno J. Gödecke @.***> wrote:

No, i don't have a proper fix yet. We can also wait until I have a proper fix.

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1317769114, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUWTA47F5DD3AZFSUITWIVPCRANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

hannojg commented 1 year ago

A draft is up for circumventing the issue until a proper fix is implemented: https://github.com/Expensify/App/pull/12820

quinthar commented 1 year ago

Can you describe the underlying problem at a high level, as well as how the workaround works?

On Thu, Nov 17, 2022, 4:21 AM Hanno J. Gödecke @.***> wrote:

A draft is up for circumventing the issue until a proper fix is implemented:

12820 https://github.com/Expensify/App/pull/12820

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1318556842, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUTUNFBZUQ4AKGDJG2TWIYPK5ANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

hannojg commented 1 year ago

The underlying problem

To achieve the inverted list effect react native simply flips the FlatList vertically with scaleY: -1. It then also flips all items with the same transformation. On the native side this translates to the following transformation commands, which get applied to the view:

The problem now is, that on android 13 a bug was introduced that causes ANRs when rotateX and scaleY are set and we simultaneously render stuff (animations, entering text). It's a bit hard to tell exactly what's going wrong on the android side, but that's also nothing we can really help with.

I am currently in the process of filing a bug report to the android OSS project. Will link soon.

How the workaround works

We manually set the styles for the FlatList and its items to achieve the inverted effect. However, we add a scaleX: -1, which changes the transform operations on the native side to skip rotateX: 180 (it's then only rotateY: -180). This way we circumvent the bug.

When doing this the only issue is, that as we have mirrored the FlatList on the X-Axis the ScrollBar will be on the wrong side. That's why I added a prop called verticalScrollbarPosition to the ScrollView, so we can change its position.

Long term

I think it would be best to get the issue fixed on android and then revert the workaround. Another solution could be to modify how react native applies transformations, something I want to look into as well (its a bit redundant to apply transformations that are not really needed).

quinthar commented 1 year ago

Wow, very interesting!

On Thu, Nov 17, 2022 at 9:07 AM Hanno J. Gödecke @.***> wrote:

The underlying problem

To achieve the inverted list effect react native simply flips the FlatList vertically with scaleY: -1. It then also flips all items with the same transformation. On the native side this translates to the following transformation commands, which get applied to the view:

  • transformX: 0
  • transformY: 0
  • rotate: 180
  • rotateY: 0
  • rotateX: 180 (now the view looks as usual again as we reverted the previous rotation)
  • scaleX: 0
  • scaleY: -1

The problem now is, that on android 13 a bug was introduced that causes ANRs when rotateX and scaleY are set and we simultaneously render stuff (animations, entering text). It's a bit hard to tell exactly what's going wrong on the android side, but that's also nothing we can really help with.

I am currently in the process of filing a bug report to the android OSS project. Will link soon. How the workaround works

We manually set the styles for the FlatList and its items to achieve the inverted effect. However, we add a scaleX: -1, which changes the transform operations on the native side to skip rotateX: 180 (it's then only rotateY: -180). This way we circumvent the bug.

When doing this the only issue is, that as we have mirrored the FlatList on the X-Axis the ScrollBar will be on the wrong side. That's why I added a prop called verticalScrollbarPosition to the ScrollView, so we can change its position. Long term

I think it would be best to get the issue fixed on android and then revert the workaround. Another solution could be to modify how react native applies transformations, something I want to look into as well (its a bit redundant to apply transformations that are not really needed).

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1318944507, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUSGV6FKY6KEIBRTRFDWIZQ6TANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

quinthar commented 1 year ago

I'm curious if you have an ETA for when this workaround can be merged?

melvin-bot[bot] commented 1 year ago

@Beamanator, @hannojg, @sobitneupane, @kadiealexander Whoops! This issue is 2 days overdue. Let's get this updated quick!

JmillsExpensify commented 1 year ago

Closing the loop. PR is here. Discussion also happening in slack.

Beamanator commented 1 year ago

Yep 👍 PR is approved, just need to get a quick change to our react-native fork published, then we can incorporate that change in App 👍

mountiny commented 1 year ago

The PR has been merged 🎉 lets wait to see how this will perform in staging

JmillsExpensify commented 1 year ago

Looks like this is on production at this point. @quinthar Has mobile performance improved for this case?

quinthar commented 1 year ago

yes, this seems to be fixed, it's miraculous, thank you!

On Sat, Nov 26, 2022 at 6:27 PM Jason Mills @.***> wrote:

Looks like this is on production at this point. @quinthar https://github.com/quinthar Has mobile performance improved for this case?

— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/11321#issuecomment-1328156297, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEMNUUJIL65N4XQUCH6RO3WKLBIDANCNFSM6AAAAAAQWMQIPE . You are receiving this because you were mentioned.Message ID: @.***>

JmillsExpensify commented 1 year ago

Woo! Go team. @kadiealexander We should make sure that contributors and related are paid out before closing the issue. Thanks!

kadiealexander commented 1 year ago

https://expensify.slack.com/archives/C01SKUP7QR0/p1669595062240009

kadiealexander commented 1 year ago

The old Upwork job closed, so please apply here @hannojg and @sobitneupane and I'll issue payment for you both. :)

https://www.upwork.com/jobs/~015cde3e5ee4ee738f https://www.upwork.com/ab/applicants/1597366602525372416/job-details

hannojg commented 1 year ago

Sorry @kadiealexander, but I can't open any of those links 😅 can you help me get access to it? (I am part of #expensify-margelo and #expensify-open-source on slack)

kadiealexander commented 1 year ago

Ah! To receive this bonus you may need to set up a profile in Upwork, which is how we typically pay our freelance contributors.

sobitneupane commented 1 year ago

@kadiealexander I didn't review the proposal and PR. So, I am not eligible for payment.

JmillsExpensify commented 1 year ago

@kadiealexander I think we have some confusion here, sorry if I caused it. @hannojg is working with on a special engagement. And then it sounds like @sobitneupane did not review the linked PR, so when I say "make sure" I mainly meant let's double check we're all good here before closing.

kadiealexander commented 1 year ago

@hannojg after internal discussion, you're eligible for the $500 bonus for this issue if you wish. You'll just need to create an Upwork profile to process the payment. Please let me know how you wish to proceed!

kadiealexander commented 1 year ago

Discussing payment delivery here.

hannojg commented 1 year ago

Payment is handled separately through a invoice by Margelo!

mountiny commented 1 year ago

In that case we can close this one, nice job @hannojg!

justinfauci commented 1 year ago

@hannojg Thank you so much for your work on this, it solved a major problem we were having with our messenger for Android 13 devices.

JmillsExpensify commented 1 year ago

Woo! Thanks everyone.