Reapply onyx upgrade use cache with fixes #24041

hannojg · 2023-08-02T09:05:46Z

Details

Recently we tried to merge this performance improvement PR:

[NoQA] Perf/onyx upgrade use cache directly #23749

but it caused a bunch of regressions. This PR is reapplying the changes + fixes the found regressions

Fixes:

[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] Home address - Page constantly crashes if insert values and quit the app #24006
- Fixed in this PR: Fix: Remove nullish values on set and on merge when no value is in storage react-native-onyx#292
[HOLD for payment 2023-08-10] App crashes when clicking 'Split bill' button for newly created account #23951 (cf5ba3d83fb315de2bb3320bc64f68365739f3e7)
[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] IOU - When a new IOU is opened, the sum of the previous IOU is displayed in the modal window #23973 (8558d74)
[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] [$1000] IOU - Double "request money" tab if request money #23962 (ceb3aad0d2319ff937b86b100872b7b0076c1951)
Fix participant selection being buggy in request money

I asked @Beamanator if we can run a full regression test suite, as this PR includes very heavy onyx changes!

Fixed Issues

$ #22644
PROPOSAL:

Tests

Test the reproduction steps from the issues and confirm that the bugs aren't happening
Verify that no errors appear in the JS console

Offline tests

n/a

QA Steps

Same as testing

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-08-02.at.14.18.54.mov

Mobile Web - Chrome

mweb-Android.mov

Mobile Web - Safari

mWeb-ios.mp4

Desktop

Uploading desktop.mov…

iOS

ios.mp4

Android

Screen.Recording.2023-08-02.at.14.27.25.mov

Expensify#23973

Expensify#23951

melvin-bot · 2023-08-02T12:06:53Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

hannojg · 2023-08-02T12:08:46Z

Just as a side note: the code of the initial IOU onyx values has been 2 years old, i tried to check every use case and it seems to be safe to be removed now

…rade-use-cache-with-fixes

hannojg · 2023-08-04T07:37:11Z

Still waiting for

Fix: Remove nullish values on set and on merge when no value is in storage react-native-onyx#292

to get merged

…rade-use-cache-with-fixes

melvin-bot · 2023-08-07T15:36:03Z

@mollfpr 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]

mvtglobally · 2023-08-07T17:43:48Z

We are seeing issue #23962 on this build. It is fixed in Staging and PROD. Should we log it again?

AndrewGable · 2023-08-07T20:18:42Z

@mvtglobally - I believe @hannojg fixed this, we are rebuilding a new build now with the fix.

mvtglobally · 2023-08-07T20:22:26Z

@AndrewGable so it was a bug?
Also, will we need to start from scratch on a new build? We are about ~30% complete

mvtglobally · 2023-08-15T09:23:53Z

@hannojg regression is completed.
Should we just retest the issues logged this round?

hannojg · 2023-08-15T09:41:50Z

Thanks for letting me know and for testing! I still have to fix the last issue. Will let you know when it's good for testing (probably in the next few hours!)

mountiny · 2023-08-15T13:46:53Z

Triggered a new build

OSBotify · 2023-08-15T14:16:17Z

🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪

android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/24041/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/24041/index.html

desktop 💻	web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/24041/NewExpensify.dmg	https://24041.pr-testing.expensify.com

hannojg · 2023-08-15T14:20:01Z

@mvtglobally The new builds are ready, can your team please test again? Thanks!

hannojg · 2023-08-15T15:12:36Z

@mollfpr can you check as well please?

mountiny · 2023-08-15T15:37:42Z

Asked QA for a review

mountiny · 2023-08-15T16:31:35Z

@situchan have you been able to check this one out?

situchan · 2023-08-15T16:33:42Z

on it now

situchan

Looks good so far.

situchan · 2023-08-15T16:50:09Z

src/components/EmojiPicker/EmojiPicker.js

-        if (isEmojiPickerVisible) {
-            Keyboard.dismiss();
-        }


What issue was fixed by this removal?

This one #24041 (comment)

This was originally added in #8221 to fix emoji select on first tap not working on android.
I confirmed that's not reproducible after revert.

8.mov

Hi, in my test, after removing this code, the keyboard no longer appears when dismissing the emoji modal on Android native.

test.mp4

How about moving Keyboard.dismiss() into the onPress handler in EmojiPickerButton? This could fix #24041 (comment) without impacting Android platform.

App/src/components/EmojiPicker/EmojiPickerButton.js

Lines 47 to 53 in 00efce4

onPress={() => {

if (!EmojiPickerAction.emojiPickerRef.current.isEmojiPickerVisible) {

EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor.current, undefined, () => {}, props.reportAction);

} else {

EmojiPickerAction.emojiPickerRef.current.hideEmojiPicker();

}

}}

Hey @ntdiary

You are saying

the keyboard no longer appears

The code here is about dismissing the keyboard. I'd never expect this code to open the keyboard?

Ah, this may seem a bit unbelievable 😂, but let me explain a bit more: as shown in the image above, for android, if we don't manually call blur ( or dismiss), the focus state inside the composer input box will not change when opening the emoji modal. Then after we close the modal, even though we call focus on the composer input box, because the internal focus state did not change, the part where the keyboard is supposed to show gets ignored.

So here, dismiss can clean up the internal focus state, allowing the subsequent focus to successfully show the keyboard.

oh wow, I didn't know that, thanks for sharing!! So you say we should add this code back? 😊

I was also surprised when I first discovered this android behavior. And don't worry, I just made a note here, we still have another issue and may do something there as needed. 😄

situchan · 2023-08-15T17:03:48Z

src/components/OptionsSelector/BaseOptionsSelector.js

@@ -339,6 +327,8 @@ class BaseOptionsSelector extends Component {
                selectTextOnFocus
                blurOnSubmit={Boolean(this.state.allOptions.length)}
                spellCheck={false}
+                autoFocus={this.props.autoFocus}
+                shouldDelayFocus={this.props.shouldDelayFocus}


Confirmed that this fixes Keyboard opens multiple times when selecting members to request money.
Curious about the root cause though?

This is mostly an order of event issue.
By this we call textInputRef.focus in the useEffect of the BaseTextInput.js component. This call will happen later than a call in componentDidMount. A (simplified) general rule in react is is that componentDidMount will always run before any useEffect hooks in the current batch of work.

Now, when we navigate to a new screen there is some code in react-navigation that will dismiss the keyboard when we transition to a new screen. When the focus happens on the class component componentDidMount the order of events is the following:

Transition to a new screen starts

Component gets rendered, calls .focus(). The keyboard starts to open

Transition causes Keyboard.dismiss to get called

(If we are lucky in terms of timing), the timeout of selectTextOnFocus kicks in and focuses the input again (keyboard opens again) [if you are curious check the JS code of the react-native-web textinput component]

Now, we call the focus from the useEffect, and the order of events is roughly this:

Transition to a new screen starts

Component gets rendered

Transition causes Keyboard.dismiss to get called

useEffect fires focusing the input

situchan

Tested all mentioned issues in this PR. All are not reproducible.
Still testing more cases

situchan · 2023-08-15T18:07:59Z

src/components/Form.js

@@ -83,7 +83,6 @@ const defaultProps = {
    isSubmitButtonVisible: true,
    formState: {
        isLoading: false,
-        errors: null,


NAB: I assume crash was fixed in onyx repo. Now what's this removal for?

The crash wasn't fixed in the onyx repo. It is invalid when looking at the logic of the component to have errors as null. It's never expected to be null.
Its a fix for this bug:

Can't find the exact bug ticket at the moment. I know it was one of the bugs that was found during going through the regressions here: https://expensify.slack.com/archives/C01GTK53T8Q/p1690897222550089

Have to leave for vacation in a few minutes so I can't find it right now, sorry 😓

Nvm. Hopefully this PR and #18648 would be merged without further change requests from you. Have a good vacation!

mvtglobally · 2023-08-15T20:47:12Z

Looks pass on mWeb. We are checking other platforms https://github.com/Expensify/App/assets/43995119/d28158f7-b4c1-4783-be84-b557628d87a4

mountiny · 2023-08-15T21:42:17Z

@situchan could you fill out the checklist please?

situchan · 2023-08-15T23:13:26Z

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:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- 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 is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
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.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
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

situchan · 2023-08-15T23:42:51Z

Screenshots/Videos

[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] Home address - Page constantly crashes if insert values and quit the app #24006

1.mov

[HOLD for payment 2023-08-10] App crashes when clicking 'Split bill' button for newly created account #23951

2.mov

[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] IOU - When a new IOU is opened, the sum of the previous IOU is displayed in the modal window #23973

3.mov

[HOLD for payment 2023-08-10] [HOLD for payment 2023-08-08] [$1000] IOU - Double "request money" tab if request money #23962
Fix participant selection being buggy in request money

4.mov

IOU - App crashes when returning from IOU detail page after requesting money

5.mov

Chat-Emoji picker is not focused and different selection visual on first and next emoji

6.mov

Keyboard opens multiple times when selecting members to request money

7.mov

situchan

We did good amount of tests but still there might be edge cases we missed, which will be discovered by our good bug reporters.
After merge, I will keep watching slack channel if any bug is reported related to this PR, so that we can fix quickly before being deploy blocker.

marcaaron · 2023-08-16T00:09:42Z

src/libs/actions/IOU.js

-    Navigation.navigate(ROUTES.getMoneyRequestRoute(iouType, reportID));
+    return resetMoneyRequestInfo(`${iouType}${reportID}`).then(() => {
+        Navigation.navigate(ROUTES.getMoneyRequestRoute(iouType, reportID));
+    });


It's pretty uncommon to have to wait for a merge to navigate. If we are going to chain off of Onyx.merge() I think we should leave some kind of comment to explain why this is the exception to the rule.

I think the problem here is that now when we have withOnyx improvement we may sometimes read outdate value. For instance we have a screen that operates on value "X". Let say we had a flow that first modifies the value "X" then navigates to our screen. Before what was happening is withOnyx wrapped screen was waiting for onyx values by waiting for connect method and then info about value "X". in connect method there were a lot of promises so we were sure that our modification was succeeded before we actually rendered content of the new screen. Now it's possible that we will modify the "X" navigate to screen read previous value of "X" from cache then once modification takes place we render the component one more time.

waiting for merge prevents reading outdate value from what I understand

I think we can easily miss some cases as previously we didn't need to care about it as we were sure that we won't read from onyx before the modification happen.
Maybe we should persist what is stale and don't read from cache when it is or indicate that we don't worry about reading outdated data.

Maybe we can do it in a bit safer way (but you better now how many such cases we can have in the app. Maybe those from the pr are the only ones)
If we don't mind reading outdated data (want to show messages as soon as possible we can add a flag)

withOnyx({ allowReadingStaleData: true })

Then on every merge/set/remove we update two simple maps so we know if the data in cache is up to date
StaleStorageLastOperation[key] = operation_number_that_modifies_the_key
toDoOperations.push(operationId)
Then
isDataStale = (key) => (toDoOperation.has(StaleStorageLastOperation[key])).

if the data is stale we don't get it from cache in withOnyx wrapper unless someone set allowReadingStaleData to true

marcaaron · 2023-08-16T00:12:37Z

src/libs/actions/IOU.js

        }
-        Navigation.navigate(ROUTES.getMoneyRequestConfirmationRoute(iouType, reportID));
+        Promise.all(promises).then(() => {


Similar comment here. Pushing all these merge promises into the array has made this code harder to reason about I think. We should document every reason why we will need to await an Onyx.merge() call.

src/libs/actions/QueuedOnyxUpdates.js

sophiepintoraetz · 2023-09-14T01:29:46Z

Can someone clarify for me what further action is happening here? I have an issue that's waiting on this PR deploying.

mountiny · 2023-09-14T07:55:35Z

@sophiepintoraetz seems like the automation here failed, but it should be deployed now so you can take your issue off hold

hannojg added 2 commits August 2, 2023 10:04

Update onyx

f462336

fix resetting IOU on new request

8558d74

Expensify#23973

hannojg force-pushed the reapply-onyx-upgrade-use-cache-with-fixes branch from ceb3aad to bcc91e6 Compare August 2, 2023 11:53

remove IOU with invalid data from initial values

cd62597

Expensify#23951

hannojg force-pushed the reapply-onyx-upgrade-use-cache-with-fixes branch from bcc91e6 to cd62597 Compare August 2, 2023 12:06

Merge branch 'main' of github.com:Expensify/App into reapply-onyx-upg…

c022440

…rade-use-cache-with-fixes

This was referenced Aug 4, 2023

[HOLD on #24041] [$1000] User B's LHN is not updated with money request message sent by User A to User B #23694

Closed

Fix: Remove nullish values on set and on merge when no value is in storage Expensify/react-native-onyx#292

Merged

Merge branch 'main' of github.com:Expensify/App into reapply-onyx-upg…

79c8565

…rade-use-cache-with-fixes

hannojg mentioned this pull request Aug 7, 2023

[$4000] Android - Chat - Empty chat (black screen) is displayed instead of messages #17870

Closed

6 tasks

Merge branch 'main' of github.com:Expensify/App into reapply-onyx-upg…

cb43f6b

…rade-use-cache-with-fixes

hannojg changed the title ~~[WIP] Reapply onyx upgrade use cache with fixes~~ Reapply onyx upgrade use cache with fixes Aug 7, 2023

hannojg marked this pull request as ready for review August 7, 2023 15:35

hannojg requested a review from a team as a code owner August 7, 2023 15:35

melvin-bot bot requested review from mollfpr and removed request for a team August 7, 2023 15:36

AndrewGable added the Ready To Build label Aug 7, 2023

hannojg marked this pull request as draft August 7, 2023 15:37

update onyx

a3e0e82

hannojg marked this pull request as ready for review August 7, 2023 15:39

hayata-suenaga mentioned this pull request Aug 7, 2023

Bump onyx version [NO QA] #24212

Closed

56 tasks

hannojg added 2 commits August 7, 2023 21:30

fix issue with doubling IOU request

1c92993

add missing semicolon

0a230de

mountiny assigned hannojg Aug 15, 2023

fix: leave auto focus logic to input component

b0d81a4

situchan reviewed Aug 15, 2023

View reviewed changes

mountiny self-requested a review August 15, 2023 17:10

situchan reviewed Aug 15, 2023

View reviewed changes

hayata-suenaga mentioned this pull request Aug 15, 2023

Implement TypeScript in New Expensify #16665

Closed

20 tasks

situchan approved these changes Aug 16, 2023

View reviewed changes

marcaaron requested changes Aug 16, 2023

View reviewed changes

WoLewicki mentioned this pull request Aug 16, 2023

[Performance] The Report screen has unnecessary rerenders #23253

Closed

aimane-chnaif mentioned this pull request Aug 16, 2023

fix: state management flow to avoid data loss Expensify/react-native-onyx#283

Merged

fedirjh mentioned this pull request Aug 17, 2023

[Waiting for payment][$1000] LHN - Face issue when two user click each another at same time #23437

Closed

6 tasks

marcaaron mentioned this pull request Aug 17, 2023

Bump Onyx to 1.0.61 #25426

Merged

59 tasks

mountiny merged commit b0d81a4 into Expensify:main Aug 18, 2023
21 of 31 checks passed

ntdiary mentioned this pull request Aug 24, 2023

[$4000] Android - Edit message - keyboard is dismissed after selecting an emoji @Tushu17 #9252

Closed

bernhardoj mentioned this pull request Sep 1, 2023

[HOLD for payment 2023-09-27] [$1000] Link Request Money not work #26214

Closed

6 tasks

	onPress={() => {
	if (!EmojiPickerAction.emojiPickerRef.current.isEmojiPickerVisible) {
	EmojiPickerAction.showEmojiPicker(props.onModalHide, props.onEmojiSelected, emojiPopoverAnchor.current, undefined, () => {}, props.reportAction);
	} else {
	EmojiPickerAction.emojiPickerRef.current.hideEmojiPicker();
	}
	}}

Reapply onyx upgrade use cache with fixes #24041

Reapply onyx upgrade use cache with fixes #24041

Conversation

hannojg commented Aug 2, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Aug 2, 2023

hannojg commented Aug 2, 2023

hannojg commented Aug 4, 2023

melvin-bot bot commented Aug 7, 2023

mvtglobally commented Aug 7, 2023

AndrewGable commented Aug 7, 2023

mvtglobally commented Aug 7, 2023

mvtglobally commented Aug 15, 2023

hannojg commented Aug 15, 2023 • edited Loading

mountiny commented Aug 15, 2023

OSBotify commented Aug 15, 2023

hannojg commented Aug 15, 2023

hannojg commented Aug 15, 2023

mountiny commented Aug 15, 2023

mountiny commented Aug 15, 2023

situchan commented Aug 15, 2023

situchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntdiary Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntdiary Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

situchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvtglobally commented Aug 15, 2023

mountiny commented Aug 15, 2023

situchan commented Aug 15, 2023

Reviewer Checklist

Screenshots/Videos

situchan commented Aug 15, 2023

Screenshots/Videos

situchan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szymon20000 Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiepintoraetz commented Sep 14, 2023

mountiny commented Sep 14, 2023

hannojg commented Aug 2, 2023 •

edited

Loading

hannojg commented Aug 15, 2023 •

edited

Loading

ntdiary Aug 24, 2023 •

edited

Loading

ntdiary Aug 24, 2023 •

edited

Loading

Szymon20000 Aug 16, 2023 •

edited

Loading