Page MenuHomePhabricator
Differential D5001

[native] Stop persisting `messageIDs` in `messageStore.threads`
ClosedPublic
Actions

Authored by kamil on Sep 1 2022, 2:14 AM.
Tags
None
Referenced Files
F3397250: D5001.diff
Sun, Dec 1, 4:57 PM
Unknown Object (File)
Fri, Nov 29, 5:39 PM
Unknown Object (File)
Fri, Nov 29, 4:53 PM
Unknown Object (File)
Wed, Nov 13, 4:34 PM
Unknown Object (File)
Wed, Nov 13, 4:34 PM
Unknown Object (File)
Wed, Nov 13, 4:34 PM
Unknown Object (File)
Wed, Nov 13, 4:34 PM
Unknown Object (File)
Wed, Nov 13, 4:34 PM
View All Files
Subscribers
• abosh
ashoat
atul

Details

Reviewers
tomek
atul
• abosh
Commits
rCOMM76679970b6b9: [native] Stop persisting `messageIDs` in `messageStore.threads`
Summary

Revision for task ENG-1430.

We do want to stop persisting messageIDs since they can be deterministically generated based on SQLite output (and we already do it in code).
Transformation is needed to provide ability to omit deeply nested fields, also, outbound function (second argument) assigns an empty array to messageIDs before re-hydration because we always expect this field to be defined.

Test Plan
  1. Make some interaction on app, close, and check if on application run after persist/REHYDRATE we have empty messageIDs in state (it'll mean that data stopped being persisted)
  2. Close app, kill keyserver and run app (to make sure we interact only with SQLite) to check if messages and threads were computed properly.

Diff Detail

Repository
rCOMM Comm
Branch
threads-rework
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil created this revision.Sep 1 2022, 2:14 AM
kamil held this revision as a draft.
Herald added subscribers: • abosh, atul, ashoat. · View Herald TranscriptSep 1 2022, 2:14 AM
kamil updated this revision to Diff 16159.Sep 1 2022, 2:22 AM

Remove unused check

Harbormaster completed remote builds in B11756: Diff 16158.Sep 1 2022, 2:24 AM
kamil published this revision for review.Sep 1 2022, 2:30 AM
kamil edited the summary of this revision. (Show Details)
kamil edited the test plan for this revision. (Show Details)
kamil added a reviewer: atul.
Harbormaster completed remote builds in B11757: Diff 16159.Sep 1 2022, 2:32 AM
kamil added a reviewer: • abosh.Sep 1 2022, 2:45 AM
tomek requested changes to this revision.Sep 2 2022, 2:44 AM
tomek added inline comments.
native/redux/persist.js
395 ↗(On Diff #16159)

Is it a good idea to mutate a part of the state?

This revision now requires changes to proceed.Sep 2 2022, 2:44 AM
kamil updated this revision to Diff 16240.Sep 2 2022, 5:25 AM
kamil edited the summary of this revision. (Show Details)

avoid state mutation

kamil added inline comments.Sep 2 2022, 5:29 AM
native/redux/persist.js
395 ↗(On Diff #16159)

Of course not. state is only persisted data that will be merged to the actual state so it's safe to mutate it here but it's still an antipattern so fixing this. Good call, thanks

Harbormaster completed remote builds in B11835: Diff 16240.Sep 2 2022, 5:35 AM
tomek accepted this revision.Sep 2 2022, 6:40 AM
tomek added inline comments.
native/redux/persist.js
396

The reason behind one bug was that we were returning a value without messageIDs while rehydrating. Is there an option to add type annotation to the return value here to make sure that it's always correct?

This revision is now accepted and ready to land.Sep 2 2022, 6:40 AM
kamil updated this revision to Diff 16312.Sep 5 2022, 5:00 AM

improve types

kamil updated this revision to Diff 16313.Sep 5 2022, 5:04 AM

improve types

Harbormaster completed remote builds in B11880: Diff 16312.Sep 5 2022, 5:11 AM
Harbormaster completed remote builds in B11881: Diff 16313.
Closed by commit rCOMM76679970b6b9: [native] Stop persisting `messageIDs` in `messageStore.threads` (authored by kamil). · Explain WhySep 6 2022, 8:24 AM
This revision was automatically updated to reflect the committed changes.
kamil added a commit: rCOMM76679970b6b9: [native] Stop persisting `messageIDs` in `messageStore.threads`.
ashoat added a comment.Sep 6 2022, 6:06 PM

How are we determining messageIDs now? It appears that you just landed a diff that completely wipes out messageIDs from Redux, without any logic to generate the messageIDs from SQLite... doesn't this completely break master??

kamil added a comment.Sep 7 2022, 2:36 AM
In D5001#147677, @ashoat wrote:

How are we determining messageIDs now? It appears that you just landed a diff that completely wipes out messageIDs from Redux, without any logic to generate the messageIDs from SQLite... doesn't this completely break master??

Hi @ashoat,
I don't think it breaks master. We already have a code responsible for generating messageIDs, check this action and to be precise those lines. (I said this in Summary but that we already do it in code is not an accurate explanation so sorry for that). Also, the second point of Test Plan should guarantee workable master since in that case without network connection and persisted data we rely on SQLite and it worked as supposed to.

ashoat added a comment.Sep 7 2022, 7:51 AM

Oops, you're absolutely right. My apologies for the confusion 😅 after being gone for over a week, my memory was not so great

Revision Contents
Changeset List

PathSize
native/
redux/
21 lines

Diff 16240

View Options

native/redux/persist.js

Loading...