fix: state management flow to avoid data loss by pasyukevich · Pull Request #283 · Expensify/react-native-onyx

pasyukevich · 2023-07-24T13:01:16Z

Details

Related Issues

Expensify/App#22122
Expensify/App#21881

Automated Tests

Linked PRs

github-actions · 2023-07-24T13:01:31Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

pasyukevich · 2023-07-24T13:28:10Z

I have read the CLA Document and I hereby sign the CLA

Julesssss

@pasyukevich can you add some manual test cases to the PR description, please? So reviewers know how to verify the fix.

Julesssss

Hmm, this doesn't seem to resolve the Android splash bug for me. To test I manually changed the code in the App node modules file. Am I missing something?:

pasyukevich · 2023-07-26T08:56:42Z

@Julesssss could you provide details on how you managed to reproduce the issue?

I will take a look

Julesssss · 2023-07-26T09:21:50Z

Yeah, so internally we use Ngrok for dev testing. A lot of us have noticed that NewDot is bricked on Android emulators with the chrome app: Expensify/App#22122

But perhaps that is a separate issue after all... oh well.

We still need some tests that we can use to verify this fix though.

pasyukevich · 2023-07-26T12:33:27Z

@Julesssss I'm in the process of preparing the instruction to verify the fix. However, it has become more challenging to do so with my previous flow, after updating to the latest version of the app. Now, I'm observing that in most instances, 'setState' is not invoked before 'setWithOnyxState' when using the same input.

When it comes to testing it locally, I've simply modified 'withOnyx.js' and 'web.development.js' inside the node_modules by incorporating this fix for both the mobile and web platforms.

pasyukevich · 2023-07-27T11:37:39Z

Hi @Julesssss

I'm not able to reproduce the issue on the latest app version, so I prepared steps on how to reproduce it on the old app but with the latest Onyx library

Step-by-step on how to reproduce the broken flow for Onyx

Log into the app locally using the main branch.
Shut down the current environment.
Fetch this branch from the forked repo - https://github.com/pasyukevich/Expensify-app/tree/test-report-expanded-latest-onyx (This is the older app version with the port updated to 8082, Onyx version updated, and the Test component prepared).
Install the necessary modules and run the app.
Open any report, type a few lines of text, and expand the composer.
Retrieve the report ID from the URL.
Navigate to Test.js in the 'src/libs/Navigation/AppNavigator'.
Replace 'PLACE_HERE_ID_OF_REPORT' with the actual report ID.
Open CentralPanelNavigator.js
Replace ReportScreenWrapper with the Test component (use 'import Test from '../Test').
Refresh the page after autorefresh.
Observe that Expensify logs as true in the console, while Test logs as false (in both instances, this.props.isSidebarLoaded is logged).
Apply the Onyx changes from this PR in web.development.js located in the node_modules. (to apply the same changes for mobile - withOnyx.js)
Refresh the page again and note that 'Test' is now true.

aimane-chnaif · 2023-07-27T12:29:39Z

@pasyukevich can you please clarify exactly what issue existed before this change and fixed after this change?

pasyukevich · 2023-07-27T12:58:56Z

@aimane-chnaif Sure

This change should fix the flow when the Onyx variable is not updated to the latest

SetState is called for the Onyx instance and adding an updated flag to the state
setWithOnyxState is called and tempState is set, the previous value erased
As a result we have the initial value in the current state

trjExpensify · 2023-08-07T09:55:17Z

This PR has stalled for a couple of weeks. Can we keep it moving @aimane-chnaif?

aimane-chnaif · 2023-08-07T09:56:20Z

yes, sure. sorry for delay

aimane-chnaif · 2023-08-07T09:56:36Z

@pasyukevich please fix conflict

pasyukevich · 2023-08-07T12:52:31Z

@aimane-chnaif PR is updated

pasyukevich · 2023-08-07T12:56:27Z

I see that the update was applied to the Onyx library that was aimed to fix the same problem, but during my test, it did not work well in the described flow

With this branch after cleaning node modules and reinstalling them I was able to reproduce the composer issue. - Expensify/App#21881

I found that the approach with function in the setState fixes this issue, but the manual assign is not (as it is now done in the wIthOnyx)

aimane-chnaif · 2023-08-16T13:18:36Z

I will test after Expensify/App#24041 (which bumps this repo version) is merged in E/App.

pasyukevich · 2023-08-16T13:24:56Z

@aimane-chnaif this flow is not reproducible on the current app version due to the page commits

best choice is to use this branch of app with updated Onyx library version

I can prepare an updated branch and flow with newer Onyx and recheck

aimane-chnaif · 2023-08-16T13:27:04Z

I can prepare an updated branch and flow with newer Onyx and recheck

Sure, thanks!

pasyukevich · 2023-08-21T10:54:11Z

I created a new test branch with updated Onyx test-report-expanded-latest-onyx

Instruction updated - #283 (comment)

This behavior is still reproducible

mountiny · 2023-08-21T21:32:30Z

@hannojg @Szymon20000 Could you help review this PR and issue its attempting to solve, your experience with Onyx might be handy, thanks!

marcaaron · 2023-08-22T01:01:56Z

lib/withOnyx.js

-                this.setState(stateUpdate); // Trigger a render
+                // Retain previous state to avoid data loss during pre-load updates.
+                // This handles race conditions where setWithOnyxState might be called after setState
+                this.setState(prevState => ({...this.tempState, ...prevState, loading: false}));


What is the race condition exactly? Is there any example? Can we break this problem down a bit further in the comment? Who is calling setState() and why should we ever prefer the previous state of the withOnyx() component over the initial value being set here?

Comment updated

Exact flow described here

That's pretty interesting.

So ultimately, sounds like we have made the bad assumption here that setState() can never be called before setWithOnyxState().

If we got here and the state already has the key then we could
in theory remove it from tempState? That approach might be a little more obvious vs. spreading the prevState after the tempState. Though they would achieve the same result. Thoughts?

Yeah, I agree with you that it would be more straightforward to remove properties from the tempState that already exist in prevState

I'd assume that you mean something like omit

I will recheck the flow again with the update and apply changes

@marcaaron PR updated with omit approach

lib/withOnyx.js

Signed-off-by: Yauheni Pasiukevich <pasyukevich@live.com>

marcaaron

LGTM thanks for the change

marcaaron · 2023-08-29T21:11:33Z

All yours @Julesssss !

Julesssss

Okay, we'll have to be careful to test this in the App PR that bumps the Onyx lib.

aimane-chnaif · 2023-08-30T11:02:39Z

Okay, we'll have to be careful to test this in the App PR that bumps the Onyx lib.

I will be taking responsible for this

Julesssss · 2023-08-30T13:01:34Z

Sweet. I created a bump PR here but I'm struggling to build Desktop now...

Julesssss · 2023-08-30T13:09:09Z

Actually, the Desktop error seems unrelated

pasyukevich force-pushed the bugfix/state-missing branch from 69a49b0 to f8cd766 Compare July 25, 2023 08:49

pasyukevich marked this pull request as ready for review July 26, 2023 08:06

pasyukevich requested a review from a team as a code owner July 26, 2023 08:06

melvin-bot bot requested review from Li357 and removed request for a team July 26, 2023 08:06

pasyukevich mentioned this pull request Jul 26, 2023

[Awaiting payment] [$1000] Web - Compose box is distorted and messages are not loaded on reload after expand in compose box Expensify/App#21881

Closed

6 tasks

Julesssss assigned pasyukevich Jul 26, 2023

Julesssss requested a review from aimane-chnaif July 26, 2023 08:32

Julesssss reviewed Jul 26, 2023

View reviewed changes

trjExpensify mentioned this pull request Jul 26, 2023

[HOLD on react-native-onyx#283] [$1000] Loading screen does not go away until a hard fresh when opening staging web Expensify/App#22122

Closed

6 tasks

pasyukevich force-pushed the bugfix/state-missing branch from f8cd766 to 5dc9847 Compare August 7, 2023 12:47

marcaaron reviewed Aug 22, 2023

View reviewed changes

pasyukevich force-pushed the bugfix/state-missing branch from 5dc9847 to 222b0dd Compare August 22, 2023 14:01

fix: state management flow to avoid data loss

8af5aaf

Signed-off-by: Yauheni Pasiukevich <pasyukevich@live.com>

pasyukevich force-pushed the bugfix/state-missing branch from 222b0dd to 8af5aaf Compare August 29, 2023 10:58

marcaaron approved these changes Aug 29, 2023

View reviewed changes

Julesssss approved these changes Aug 30, 2023

View reviewed changes

Julesssss merged commit f3141b9 into Expensify:main Aug 30, 2023

Conversation

pasyukevich commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Related Issues

Automated Tests

Linked PRs

Uh oh!

github-actions bot commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pasyukevich commented Jul 24, 2023

Uh oh!

Julesssss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

pasyukevich commented Jul 26, 2023

Uh oh!

Julesssss commented Jul 26, 2023

Uh oh!

pasyukevich commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pasyukevich commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aimane-chnaif commented Jul 27, 2023

Uh oh!

pasyukevich commented Jul 27, 2023

Uh oh!

trjExpensify commented Aug 7, 2023

Uh oh!

aimane-chnaif commented Aug 7, 2023

Uh oh!

aimane-chnaif commented Aug 7, 2023

Uh oh!

pasyukevich commented Aug 7, 2023

Uh oh!

pasyukevich commented Aug 7, 2023

Uh oh!

aimane-chnaif commented Aug 16, 2023

Uh oh!

pasyukevich commented Aug 16, 2023

Uh oh!

aimane-chnaif commented Aug 16, 2023

Uh oh!

pasyukevich commented Aug 21, 2023

Uh oh!

mountiny commented Aug 21, 2023

Uh oh!

marcaaron Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

pasyukevich Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

pasyukevich Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

marcaaron Aug 22, 2023

Choose a reason for hiding this comment

Uh oh!

pasyukevich Aug 24, 2023

Choose a reason for hiding this comment

Uh oh!

pasyukevich Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron commented Aug 29, 2023

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

pasyukevich commented Jul 24, 2023 •

edited

Loading

github-actions bot commented Jul 24, 2023 •

edited

Loading

Julesssss left a comment •

edited

Loading

pasyukevich commented Jul 26, 2023 •

edited

Loading

pasyukevich commented Jul 27, 2023 •

edited

Loading