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
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

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

Typo: early-exit

403

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

416–420

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

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

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.