Page MenuHomePhabricator

[web] Redux migration to clear `UNSUPPORTED<UPDATE_RELATIONSHIP>` messages
ClosedPublic

Authored by atul on Apr 17 2024, 11:12 AM.
Tags
None
Referenced Files
F3358367: D11680.diff
Sun, Nov 24, 4:26 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Unknown Object (File)
Sun, Nov 3, 9:37 AM
Subscribers
None

Details

Summary

Effectively D11669, but for web instead of native.


Depends on D11669

Test Plan

Before migration (note messageID=84025):

image.png (884×1 px, 135 KB)

After migration (note absence of messageID=84025:

image.png (1×1 px, 153 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D11680 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.

modify invariant message

atul published this revision for review.Apr 17 2024, 11:16 AM
atul added inline comments.
web/redux/persist.js
416–420 ↗(On Diff #39192)

Opted for this instead of something like

6e04a6.png (184×1 px, 20 KB)

so that this migration doesn't fail silently if message in SQLite is malformed.

atul edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Apr 17 2024, 3:24 PM

Worried about invariants

web/redux/persist.js
391 ↗(On Diff #39192)

Typo: early-exit

403 ↗(On Diff #39192)

Why is this invariant necessary? What makes it so that stores.store should exist here?

416–420 ↗(On Diff #39192)

Do we really want to crash the migration? What happens if the migration "fails silently"? What's the input on the user experience?

This revision now requires changes to proceed.Apr 17 2024, 3:24 PM
web/redux/persist.js
403 ↗(On Diff #39192)

I was following the pattern in migration 3:

fbacb2.png (212×1 px, 58 KB)

Without the invariant, we get the following flow errors:

3f0434.png (1×2 px, 295 KB)

That said, the following would probably be a cleaner approach and avoid use of invariant:

// 2. Get existing `stores` from SQLite.
const stores = await sharedWorker.schedule({
  type: workerRequestMessageTypes.GET_CLIENT_STORE,
});

const messages: ?$ReadOnlyArray<ClientDBMessageInfo> =
  stores?.store?.messages;

if (messages === null || messages === undefined || messages.length === 0) {
  return state;
}

get rid of first invariant

atul marked an inline comment as done.

fix typo + remove second invaraint

web/redux/persist.js
416–420 ↗(On Diff #39192)

If message.content doesn't exist for a UPDATE_RELATIONSHIP message it won't be removed from localDB, but this situation should not be possible.

Updated to go with the previous check instead to avoid possibility of crashing mid-migration.

This revision is now accepted and ready to land.Apr 19 2024, 6:42 AM
This revision was landed with ongoing or failed builds.Apr 19 2024, 11:41 AM
This revision was automatically updated to reflect the committed changes.