Details

Reviewers

kamil
tomek

Commits

rCOMM258b2493c244: [native] Migrate to new id schema
rCOMM82d61f13c7c1: [native] Migrate to new id schema

Summary

https://linear.app/comm/issue/ENG-261/migrate-sqlite-to-the-new-id-schema
https://linear.app/comm/issue/ENG-3996/migrate-the-redux-state
ID schema migration for native.

Depends on D8382, D8384, D8352

Test Plan

Run the old version of the app, create some drafts, close the app
Run the new version of the app, check if drafts still exist
Explore the app, check if there are any problems
Compare redux and sqlite from before and after, and check if all ids were converted

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Jun 30 2023, 3:23 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJun 30 2023, 3:23 AM

michal added a child revision: D8386: [lib] Enable new id schema.Jun 30 2023, 3:25 AM

michal edited parent revisions, added: D8353: [native] Add support for async migrations; removed: D8352: [web] Move createAsyncMigrate to lib.Jun 30 2023, 3:27 AM

Harbormaster completed remote builds in B20635: Diff 28287.Jun 30 2023, 3:41 AM

michal requested review of this revision.Jun 30 2023, 3:41 AM

I think there is a performance problem here, you call getClientDBStore() at least twice, which is a very heavy operation and executes in sequence things that can be wrapped in Promise.all. Can you refactor this to:

call getClientDBStore() only once and make the entire logic operate on this result
create promises and put them into Promise.all?

native/redux/persist.js
598–606	can this be defined in a separate function with a descriptive name?
602	I think it's better to use `keyserverPrefixID` instead of 256
611–620	my comments from `updateClientDBMessageStoreThreads` also applies here

This revision now requires changes to proceed.Jul 3 2023, 7:21 AM

kamil mentioned this in D8384: [native] Add utils for migrating message store.Jul 3 2023, 7:50 AM

Get all stores at the same time and await all updates concurrently. Extract conversion functions.

Harbormaster completed remote builds in B20711: Diff 28380.Jul 4 2023, 6:13 AM

Looks a lot better!

lib/utils/migration-utils.js
68 ↗	(On Diff #28380)	I saw this expression multiple times, I would prefer to create a simple function, sth like `attachKeyserverID()` but up to you
native/redux/persist.js
642 ↗	(On Diff #28380)	I think it is better to avoid magic numbers