Invert list manually on android #12820

hannojg · 2022-11-17T12:20:26Z

Details

This is a workaround for the issue:

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

There is a react-native issue that we are about to solve upstream:

[Android] Inverted FlatList + Text Input causing ANR on android 13 facebook/react-native#35350

However, it can take some more time until we find a fix for the issue upstream. Until then we settled to do the workaround:

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

Important: For this PR to work correctly this change in the react-native code base is needed:

feat: add verticalScrollbarPosition prop to ScrollView react-native#33

Details for the workaround can be found in this comment: #11321 (comment)

Fixed Issues

$ #11321
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Verify that no errors appear in the JS console
Reproduction of the bug on main, test that this PR solves the issue:
- Run on android 13 (I use a nexus 4 android API 33 emulator)
- Open the app
- Open a chat / report
- Enter text rapidly
  - I use the help of adb:

adb shell input text "Lorem\ ipsum\ dolor\ sit\ amet,\ consetetur\ sadipscing\ elitr,\ sed\ diam\ nonumy\ eirmod\ tempor\ invidunt\ ut\ labore\ et\ dolore\ magna\ aliquyam\ erat,\ sed\ diam\ voluptua.\ At\ vero\ eos\ et\ accusam\ et\ justo\ duo\ dolores\ et\ ea\ rebum.\ Stet\ clita\ kasd\ gubergren,\ no\ sea\ takimata\ sanctus\ est\ Lorem\ ipsum\ dolor\ sit\ amet.\ Lorem\ ipsum\ dolor\ sit\ amet,\ consetetur\ sadipscing\ elitr,\ sed\ diam\ nonumy\ eirmod\ tempor\ invidunt\ ut\ labore\ et\ dolore\ magna\ aliquyam\ erat,\ sed\ diam\ voluptua.\ At\ vero\ eos\ et\ accusam\ et\ justo\ duo\ dolores\ et\ ea\ rebum.\ Stet\ clita\ kasd\ gubergren,\ no\ sea\ takimata\ sanctus\ est\ Lorem\ ipsum\ dolor\ sit\ amet."

On main the app gets frozen. On this PR the app stays alive

Offline tests

QA Steps

Verify that no errors appear in the JS console

On Android, go to some chat report
Enter text rapidly
continue so the composers starts to overflow or in other words continue rapidly typing for a while
Change chat and do it in another one
Verify you havent notices any particular slowdown of the app (it did not freeze as it does in production)

Confirm the above steps with high traffic account.

On other platforms, confirms that scrolling in the chat report work fine, sending messages too, loading old messages when scrolling back in history.

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

I posted a video here from a Pixel 4 running android 13. On the main branch this testing case does crash:

`main` (broken)	`this branch`
main_branch.mov	android.mov

as per: facebook/react-native#35350 (comment)

melvin-bot · 2022-11-17T12:50:49Z

@Beamanator @sobitneupane One of you needs to 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]

mountiny

This looks good to me, I need to test tho, had some issues with the Android emulator before.

mountiny · 2022-11-21T11:26:48Z

Making sure this is on hold Expensify/react-native#33

Beamanator · 2022-11-21T12:52:46Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Beamanator

Tested great on Android!! Going to test on iOS & all other platforms to make sure I don't see any weird bugs 👍

Also requesting a few tiny comment changes since we like to start comments with capitals :D

src/components/InvertedFlatList/index.android.js

src/styles/styles.js

Co-authored-by: Alex Beaman <dabeamanator@gmail.com>

mountiny · 2022-11-21T15:37:24Z

We dont like workarounds, lets not call this a workaround in the title :D

Beamanator

Looking great, still on hold for the react-native fork deploy

AndrewGable · 2022-11-21T23:52:29Z

I published the fork, please let me know if you have any issues with this version:

npm notice === Tarball Details ===
npm notice name:          @expensify/react-native
npm notice version:       0.70.4-alpha.2
npm notice filename:      @expensify/react-native-0.70.4-alpha.2.tgz
npm notice package size:  74.7 MB
npm notice unpacked size: 114.4 MB
npm notice shasum:        f09e9b2737e07156c5f12c60aa2747036466f40f
npm notice integrity:     sha512-JpB56JP1YXrhu[...]gOYki++hVwg7A==
npm notice total files:   3338
npm notice
npm notice Publishing to https://registry.npmjs.org/
+ @expensify/react-native@0.70.4-alpha.2
Published to npm 0.70.4-alpha.2

mountiny

@AndrewGable Thank you so much for doing this!

Taking this off hold, @hannojg can you please remove the patch and update the RN version? and retest.

hannojg · 2022-11-22T09:30:10Z

Alright, updated @mountiny!

There was one change applied to the ios/project.pbxproj when I ran pod install but I think this change is fine.

mountiny

thank you @hannojg for the urgency, the code looks good to me!

ios/NewExpensify.xcodeproj/project.pbxproj

mountiny

I have a struggle bus with my Android emulator it is showing as offline, then it is not responsive at all and already spent couple of hours on this 💩

@Beamanator Would you be able to give this a little retest?

Beamanator · 2022-11-22T13:24:21Z

@vitHoracek sorry for the trouble, ya I'll test now!

mountiny

AppPull12820.mov

Ok, managed to get this stuff running, had to set DNS servers locally so the emulator has access to internet 🤦

All works great for me, great job @hannojg 🎉

Teste iOS too and works fine too!

mountiny · 2022-11-22T13:39:02Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Screen.Recording.2022-11-22.at.13.40.42.mov

Android

AppPull12820.mov

Beamanator

Testing super well! Excited to get this merged!

OSBotify · 2022-11-22T14:06:14Z

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

mvtglobally · 2022-11-22T16:25:24Z

@mountiny @Beamanator
Do you want us to QA this one? Pls update steps if any validation is required

mountiny · 2022-11-22T16:33:38Z

@mvtglobally Thanks for the bump havent noticed. I have updated the QA steps

OSBotify · 2022-11-22T16:35:51Z

🚀 Deployed to staging by @mountiny in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-22T17:45:35Z

🚀 Deployed to staging by @mountiny in version: 1.2.30-0 🚀

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

OSBotify · 2022-11-24T00:03:57Z

🚀 Deployed to production by @luacmartins in version: 1.2.30-0 🚀

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

ref: Expensify/App#12820

* fix: apply workaround for android anr ref: Expensify/App#12820 * chore: simplify the cell renderer component * fix: update snapshots

workaround: invert list manually on android

6176fcf

as per: facebook/react-native#35350 (comment)

hannojg mentioned this pull request Nov 17, 2022

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

Closed

hannojg added 2 commits November 17, 2022 13:33

formatting

9e39948

fix props spreading

d058feb

hannojg marked this pull request as ready for review November 17, 2022 12:50

hannojg requested a review from a team as a code owner November 17, 2022 12:50

melvin-bot bot requested review from Beamanator and sobitneupane and removed request for a team November 17, 2022 12:50

hannojg added 2 commits November 17, 2022 13:54

revert formatting changes

55345d1

add notice

54a8a29

hannojg force-pushed the fix/workaround-android-13-ANR-issue branch from 81c3b37 to 54a8a29 Compare November 17, 2022 12:56

mountiny assigned hannojg Nov 21, 2022

mountiny reviewed Nov 21, 2022

View reviewed changes

mountiny changed the title ~~workaround: invert list manually on android~~ [HOLD] workaround: invert list manually on android Nov 21, 2022

Beamanator requested changes Nov 21, 2022

View reviewed changes

src/components/InvertedFlatList/index.android.js Outdated Show resolved Hide resolved

src/components/InvertedFlatList/index.android.js Outdated Show resolved Hide resolved

src/styles/styles.js Outdated Show resolved Hide resolved

hannojg and others added 3 commits November 21, 2022 16:11

Update src/components/InvertedFlatList/index.android.js

ea48ddc

Co-authored-by: Alex Beaman <dabeamanator@gmail.com>

Update src/styles/styles.js

333be85

Co-authored-by: Alex Beaman <dabeamanator@gmail.com>

Update src/components/InvertedFlatList/index.android.js

1819556

Co-authored-by: Alex Beaman <dabeamanator@gmail.com>

mountiny changed the title ~~[HOLD] workaround: invert list manually on android~~ [HOLD] Invert list manually on android Nov 21, 2022

Beamanator previously approved these changes Nov 21, 2022

View reviewed changes

mountiny changed the title ~~[HOLD] Invert list manually on android~~ Invert list manually on android Nov 22, 2022

mountiny requested changes Nov 22, 2022

View reviewed changes

bump react native version to 0.70.4-alpha.2 including patch

67660c5

hannojg dismissed Beamanator’s stale review via 67660c5 November 22, 2022 09:29

hannojg requested review from mountiny and removed request for sobitneupane November 22, 2022 09:30

mountiny reviewed Nov 22, 2022

View reviewed changes

ios/NewExpensify.xcodeproj/project.pbxproj Show resolved Hide resolved

mountiny approved these changes Nov 22, 2022

View reviewed changes

Beamanator approved these changes Nov 22, 2022

View reviewed changes

mountiny merged commit 433546a into Expensify:main Nov 22, 2022

OSBotify mentioned this pull request Nov 22, 2022

Deploy Checklist: New Expensify 2022-11-22 #12934

Closed

40 tasks

mananjadhav mentioned this pull request Dec 16, 2022

[$1000] Skeleton-view is right oriented when scrolling up and loading new messages on Android #13636

Closed

vanGalilea mentioned this pull request Feb 7, 2023

Typing in chat freezes the app on Android GetStream/stream-chat-react-native#1810

Closed

santhoshvai added a commit to GetStream/stream-chat-react-native that referenced this pull request Feb 10, 2023

fix: apply workaround for android anr

d1918a7

ref: Expensify/App#12820

santhoshvai mentioned this pull request Feb 10, 2023

fix: apply workaround for android anr GetStream/stream-chat-react-native#1964

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invert list manually on android #12820

Invert list manually on android #12820

hannojg commented Nov 17, 2022 •

edited by mountiny

Loading

melvin-bot bot commented Nov 17, 2022

mountiny left a comment

mountiny commented Nov 21, 2022

Beamanator commented Nov 21, 2022 •

edited

Loading

Beamanator left a comment

mountiny commented Nov 21, 2022

Beamanator left a comment

AndrewGable commented Nov 21, 2022

mountiny left a comment

hannojg commented Nov 22, 2022

mountiny left a comment

mountiny left a comment

Beamanator commented Nov 22, 2022

mountiny left a comment

mountiny commented Nov 22, 2022 •

edited

Loading

Beamanator left a comment

OSBotify commented Nov 22, 2022

mvtglobally commented Nov 22, 2022

mountiny commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 24, 2022

Invert list manually on android #12820

Invert list manually on android #12820

Conversation

hannojg commented Nov 17, 2022 • edited by mountiny Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

melvin-bot bot commented Nov 17, 2022

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Nov 21, 2022

Beamanator commented Nov 21, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

Beamanator left a comment

Choose a reason for hiding this comment

mountiny commented Nov 21, 2022

Beamanator left a comment

Choose a reason for hiding this comment

AndrewGable commented Nov 21, 2022

mountiny left a comment

Choose a reason for hiding this comment

hannojg commented Nov 22, 2022

mountiny left a comment

Choose a reason for hiding this comment

mountiny left a comment

Choose a reason for hiding this comment

Beamanator commented Nov 22, 2022

mountiny left a comment

Choose a reason for hiding this comment

mountiny commented Nov 22, 2022 • edited Loading

Reviewer Checklist

Screenshots/Videos

Beamanator left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 22, 2022

mvtglobally commented Nov 22, 2022

mountiny commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 22, 2022

OSBotify commented Nov 24, 2022

hannojg commented Nov 17, 2022 •

edited by mountiny

Loading

Beamanator commented Nov 21, 2022 •

edited

Loading

mountiny commented Nov 22, 2022 •

edited

Loading