Page MenuHomePhabricator

Move Store Loaded to redux
ClosedPublic

Authored by marcin on Nov 23 2022, 6:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 2:23 PM
Unknown Object (File)
Tue, Nov 19, 2:23 PM
Unknown Object (File)
Tue, Nov 19, 2:23 PM
Unknown Object (File)
Tue, Nov 19, 2:23 PM
Unknown Object (File)
Tue, Nov 19, 2:21 PM
Unknown Object (File)
Tue, Nov 19, 2:11 PM
Unknown Object (File)
Mon, Nov 18, 10:15 AM
Unknown Object (File)
Mon, Nov 4, 3:43 PM
Subscribers

Details

Summary

This differential introduces store loaded to redux and makes sure it is not persisted. There are components that check storeLoaded flag to ensure data from SQLite was inserted to redux. Setting this flag in react state after redux actions are dispatched introduces some sort of race condition. Therefore we should keep this flag in redux and set it in reducer in response to the same action that inserts SQLite data.

Test Plan

Make sure eslint and flow do not complain. add code in reducer in native that sets this flag to true. Add logging to make sure this flag is false after re-starting application.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Nov 23 2022, 9:31 AM

Why this diff doesn't have reviewers set?

Are we planning to do something similar on web?

What is the purpose of this flag? (I guess the following diffs will answer that question, but it would be better for the summary to do so)

The test plan should also describe what was done to verify that the flag is not persisted.

This revision now requires changes to proceed.Nov 23 2022, 9:31 AM
In D5709#170217, @tomek wrote:

Why this diff doesn't have reviewers set?

I have always been under impression that, arcanist will not allow diff to be submitted if reviewers are not set. So I didn't realize I forgot to add reviewers after diff was submitted.

Are we planning to do something similar on web?
What is the purpose of this flag? (I guess the following diffs will answer that question, but it would be better for the summary to do so)

Currently we keep storeLoaded flag in react state and set it after actions that insert SQLite data to redux are dispatched. This is not correct since there are components that rely on this flag to check if SQLite data are present in redux, and reducing action might happen after this flag is set. If everything works correctly it is a race condition. Therefore we moved this flag to redux and decided to set it in reducer in reaction to action that inserts SQLite data to be sure that data and this flag are set "transactionally".

marcin edited the summary of this revision. (Show Details)
marcin edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Nov 25 2022, 4:28 AM

Introduce action to set exclusively storeLoaded field in redux and implement its reduction in native reducer

This revision was automatically updated to reflect the committed changes.