Paths

Table of Contentst

-
lib/hooks/
-
hooks/
4/7
ops-hooks.js

[lib] Create a wrapper over dispatch
ClosedPublic
Actions

Authored by tomek on Mar 21 2024, 8:26 AM.

Details

Reviewers

kamil
inka
will

Commits

rCOMM8a655eac7f4e: [lib] Create a wrapper over dispatch

Summary

We're adding a messageID field to an action if it is provided

https://linear.app/comm/issue/ENG-7106/create-a-context

Depends on D11368

Test Plan

Use the new dispatch function and verify that the store gets updated. Check if the ops are being processed and that the queue's length decreases with each processing.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Mar 21 2024, 8:26 AM

Herald added a subscriber: ashoat. · View Herald TranscriptMar 21 2024, 8:26 AM

tomek added a child revision: D11370: [lib] Resolve the promise after processing is done.Mar 21 2024, 8:31 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 21 2024, 8:37 AM

Harbormaster failed remote builds in B27650: Diff 38271!

Instead of creating a context we can have a simple hook

tomek retitled this revision from [lib] Create a context providing a wrapper over dispatch to [lib] Create a wrapper over dispatch.Mar 27 2024, 9:02 AM

tomek edited the summary of this revision. (Show Details)

tomek edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B27710: Diff 38373.Mar 27 2024, 9:18 AM

tomek requested review of this revision.Mar 27 2024, 9:18 AM

kamil accepted this revision.Mar 28 2024, 3:54 AM

kamil added inline comments.

lib/hooks/ops-hooks.js
9	this name feels a bit confusing to me - maybe something like `useDispatchWithMessageID()`? But don't feel strongly, up to you

This revision is now accepted and ready to land.Mar 28 2024, 3:54 AM

Rename

Harbormaster completed remote builds in B27763: Diff 38443.Mar 28 2024, 7:19 AM

Closed by commit rCOMM8a655eac7f4e: [lib] Create a wrapper over dispatch (authored by tomek). · Explain WhyMar 28 2024, 8:11 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMM8a655eac7f4e: [lib] Create a wrapper over dispatch.

Revision Contents
Changeset List

Path

Size

lib/

hooks/

ops-hooks.js

21 lines

Diff 38373

View Options

lib/hooks/ops-hooks.js

// @flow // @flow

import invariant from 'invariant';

import * as React from 'react'; import * as React from 'react';

import { setPeerDeviceListsActionType } from 'lib/actions/aux-user-actions.js';

import { createUserKeysBackupActionTypes } from 'lib/actions/backup-actions.js'; import { createUserKeysBackupActionTypes } from 'lib/actions/backup-actions.js';

import {

useBroadcastDeviceListUpdates,

useGetAndUpdateDeviceListsForUsers,

} from 'lib/hooks/peer-list-hooks.js';

import { useCheckIfPrimaryDevice } from 'lib/hooks/primary-device-hooks.js'; import { useCheckIfPrimaryDevice } from 'lib/hooks/primary-device-hooks.js';

import { isLoggedIn } from 'lib/selectors/user-selectors.js'; import { isLoggedIn, getAllPeerDevices } from 'lib/selectors/user-selectors.js';

import { signDeviceListUpdate } from 'lib/shared/device-list-utils.js';

import { IdentityClientContext } from 'lib/shared/identity-client-context.js';

import { useStaffAlert } from 'lib/shared/staff-utils.js'; import { useStaffAlert } from 'lib/shared/staff-utils.js';

import type {

RawDeviceList,

SignedDeviceList,

} from 'lib/types/identity-service-types.js';

import {

composeRawDeviceList,

rawDeviceListFromSignedList,

} from 'lib/utils/device-list-utils.js';

import { getMessageForException } from 'lib/utils/errors.js'; import { getMessageForException } from 'lib/utils/errors.js';

import { useDispatchActionPromise } from 'lib/utils/redux-promise-utils.js'; import { useDispatchActionPromise } from 'lib/utils/redux-promise-utils.js';

import { useDispatch } from 'lib/utils/redux-utils.js';

import { usingRestoreFlow } from 'lib/utils/services-utils.js';

import { useClientBackup } from './use-client-backup.js'; import { useClientBackup } from './use-client-backup.js';

import { useGetBackupSecretForLoggedInUser } from './use-get-backup-secret.js'; import { useGetBackupSecretForLoggedInUser } from './use-get-backup-secret.js';

import { commCoreModule } from '../native-modules.js'; import { commCoreModule } from '../native-modules.js';

import { useSelector } from '../redux/redux-utils.js'; import { useSelector } from '../redux/redux-utils.js';

import { useStaffCanSee } from '../utils/staff-utils.js'; import { useStaffCanSee } from '../utils/staff-utils.js';

const millisecondsPerDay = 24 * 60 * 60 * 1000; const millisecondsPerDay = 24 * 60 * 60 * 1000;

async function reorderAndSignDeviceList(

thisDeviceID: string,

currentDeviceList: RawDeviceList,

): Promise<{

+rawList: RawDeviceList,

+signedList: SignedDeviceList,

}> {

const currentDevices = [...currentDeviceList.devices];

const thisDeviceIndex = currentDevices.indexOf(thisDeviceID);

if (thisDeviceIndex < 0) {

throw new Error("Device list doesn't contain current device ID");

}

const newDevices =

thisDeviceIndex === 0

? currentDevices

: [thisDeviceID, ...currentDevices.splice(thisDeviceIndex, 1)];

const rawList = composeRawDeviceList(newDevices);

const signedList = await signDeviceListUpdate(rawList);

return { rawList, signedList };

}

function BackupHandler(): null { function BackupHandler(): null {

const loggedIn = useSelector(isLoggedIn); const loggedIn = useSelector(isLoggedIn);

const staffCanSee = useStaffCanSee(); const staffCanSee = useStaffCanSee();

const isBackground = useSelector( const isBackground = useSelector(

state => state.lifecycleState === 'background', state => state.lifecycleState === 'background',

); );

const canPerformBackupOperation = loggedIn && !isBackground; const canPerformBackupOperation = loggedIn && !isBackground;

const checkIfPrimaryDevice = useCheckIfPrimaryDevice(); const checkIfPrimaryDevice = useCheckIfPrimaryDevice();

const { showAlertToStaff } = useStaffAlert(); const { showAlertToStaff } = useStaffAlert();

const latestBackupInfo = useSelector( const latestBackupInfo = useSelector(

state => state.backupStore.latestBackupInfo, state => state.backupStore.latestBackupInfo,

); );

const userIdentifier = useSelector(state => state.currentUserInfo?.username); const userIdentifier = useSelector(state => state.currentUserInfo?.username);

const dispatchActionPromise = useDispatchActionPromise(); const dispatchActionPromise = useDispatchActionPromise();

const { createUserKeysBackup, retrieveLatestBackupInfo, getBackupUserKeys } = const { createUserKeysBackup, retrieveLatestBackupInfo, getBackupUserKeys } =

useClientBackup(); useClientBackup();

const getBackupSecret = useGetBackupSecretForLoggedInUser(); const getBackupSecret = useGetBackupSecretForLoggedInUser();

const backupUploadInProgress = React.useRef<boolean>(false); const backupUploadInProgress = React.useRef<boolean>(false);

const [handlerStarted, setHandlerStarted] = React.useState(false); const [handlerStarted, setHandlerStarted] = React.useState(false);

const identityContext = React.useContext(IdentityClientContext);

invariant(identityContext, 'Identity context should be set');

const dispatch = useDispatch();

const getAndUpdateDeviceListsForUsers = useGetAndUpdateDeviceListsForUsers();

const broadcastDeviceListUpdates = useBroadcastDeviceListUpdates();

const allPeerDevices = useSelector(getAllPeerDevices);

React.useEffect(() => { React.useEffect(() => {

if (!staffCanSee) { if (!staffCanSee) {

return; return;

} }

void (async () => { void (async () => {

const isPrimaryDevice = await checkIfPrimaryDevice(); const isPrimaryDevice = await checkIfPrimaryDevice();

if (!isPrimaryDevice) { if (!isPrimaryDevice) {

▲ Show 20 Lines • Show All 88 Lines • ▼ Show 20 Lines if (

!handlerStarted || !handlerStarted ||

backupUploadInProgress.current backupUploadInProgress.current

) { ) {

return; return;

} }

void (async () => { void (async () => {

backupUploadInProgress.current = true; backupUploadInProgress.current = true;

const isPrimaryDevice = await checkIfPrimaryDevice(); const isPrimaryDevice = await checkIfPrimaryDevice();

if (!isPrimaryDevice) { const { getAuthMetadata, identityClient } = identityContext;

const { userID, deviceID } = await getAuthMetadata();

let currentDeviceList, currentUserPlatformDetails, deviceListIsSigned;

try {

if (!userID) {

throw new Error('Missing auth metadata');

kamilUnsubmitted

Not Done

if (!userID) {

- throw new Error('Missing auth metadata');

+ throw new Error('Missing userID');

}

if (!userIdentifier) {

Maybe or merge with condition below?

kamil: Maybe or merge with condition below?

bartekAuthorUnsubmitted

Done

Good idea

bartek: Good idea

}

if (!userIdentifier) {

throw new Error('Missing userIdentifier');

}

const deviceListsResponse = await identityClient.getDeviceListsForUsers(

[userID],

);

currentDeviceList = deviceListsResponse.usersSignedDeviceLists[userID];

currentUserPlatformDetails =

deviceListsResponse.usersDevicesPlatformDetails[userID];

if (!currentDeviceList || !currentUserPlatformDetails) {

throw new Error('Device list not found for current user');

}

deviceListIsSigned = !!currentDeviceList.curPrimarySignature;

if (!isPrimaryDevice && deviceListIsSigned) {

backupUploadInProgress.current = false;

return;

}

} catch (err) {

const message = getMessageForException(err) ?? 'unknown error';

showAlertToStaff('Error fetching current device list:', message);

console.log('Error fetching current device list:', message);

backupUploadInProgress.current = false; backupUploadInProgress.current = false;

return; return;

bartekAuthorUnsubmitted

Done

This whole block can be replaced with selector call when ENG-9763 is done

bartek: This whole block can be replaced with selector call when [[ https://linear.app/comm/issue/ENG…

} }

if (latestBackupInfo) { if (isPrimaryDevice && latestBackupInfo) {

const timestamp = latestBackupInfo.timestamp; const timestamp = latestBackupInfo.timestamp;

if (timestamp >= Date.now() - millisecondsPerDay) { if (timestamp >= Date.now() - millisecondsPerDay) {

backupUploadInProgress.current = false; backupUploadInProgress.current = false;

return; return;

} }

try { try {

await testUserKeysRestore(); await testUserKeysRestore();

} catch (err) { } catch (err) {

const message = getMessageForException(err) ?? 'unknown error'; const message = getMessageForException(err) ?? 'unknown error';

showAlertToStaff('Error restoring User Keys backup', message); showAlertToStaff('Error restoring User Keys backup', message);

console.log('Error restoring User Keys backup:', message); console.log('Error restoring User Keys backup:', message);

} }

try { try {

const shouldDoMigration =

usingRestoreFlow && !latestBackupInfo && !deviceListIsSigned;

if (!shouldDoMigration && !isPrimaryDevice) {

backupUploadInProgress.current = false;

return;

}

const promise = (async () => { const promise = (async () => {

if (shouldDoMigration) {

if (!userID || !deviceID) {

throw new Error('Missing auth metadata');

}

const { updateDeviceList } = identityClient;

invariant(

updateDeviceList,

'updateDeviceList() should be defined on native. ' +

'Are you calling it on a non-primary device?',

);

// 1. upload UserKeys (without updating the store)

let backupID = await createUserKeysBackup();

// 2. create in-memory device list (reorder and sign)

const newDeviceList = await reorderAndSignDeviceList(

deviceID,

rawDeviceListFromSignedList(currentDeviceList),

);

// 3. UpdateDeviceList RPC transaction

await updateDeviceList(newDeviceList.signedList);

dispatch({

type: setPeerDeviceListsActionType,

payload: {

deviceLists: { [userID]: newDeviceList.rawList },

usersPlatformDetails: {

[userID]: currentUserPlatformDetails,

});

// 4. fetch backupID again and compare

const fetchedBackupInfo =

await retrieveLatestBackupInfo(userIdentifier);

if (!fetchedBackupInfo.backupID !== backupID) {

backupID = await createUserKeysBackup();

}

kamilUnsubmitted

Not Done

shouldn't we call this in a loop, and also check if we're still primary before uploading backup keys?

kamil: shouldn't we call this in a loop, and also check if we're still primary before uploading backup…

bartekAuthorUnsubmitted

Done

I thought about this a bit and considered a few scenarios. After doing some testing, I think this works the best:

The device list is updated at this point.
Retry it 3-5 times.
If it fails, don't set the backupID locally, so the next time this hook is run, it will trigger uploading backup again.
Because the device list is updated, other devices will no longer be trying to upload it.

bartek: I thought about this a bit and considered a few scenarios. After doing some testing, I think…

// 5. Broadcast update to peers

void getAndUpdateDeviceListsForUsers([userID]);

void broadcastDeviceListUpdates(

allPeerDevices.filter(id => id !== deviceID),

);

// 6. Set store value (dispatchActionPromise success return value)

return {

backupID,

timestamp: Date.now(),

};

} else {

const backupID = await createUserKeysBackup(); const backupID = await createUserKeysBackup();

return { return {

backupID, backupID,

timestamp: Date.now(), timestamp: Date.now(),

}; };

}

})(); })();

void dispatchActionPromise(createUserKeysBackupActionTypes, promise); void dispatchActionPromise(createUserKeysBackupActionTypes, promise);

await promise; await promise;

} catch (err) { } catch (err) {

const message = getMessageForException(err) ?? 'unknown error'; const message = getMessageForException(err) ?? 'unknown error';

showAlertToStaff('Error creating User Keys backup', message); showAlertToStaff('Error creating User Keys backup', message);

console.log('Error creating User Keys backup:', message); console.log('Error creating User Keys backup:', message);

} }

kamilUnsubmitted

Not Done

Now, this covers both regular backup upload and migration, I would prefer to somehow differentiate (use different try-catch) so this could be easier to debug if needed

kamil: Now, this covers both regular backup upload and migration, I would prefer to somehow…

bartekAuthorUnsubmitted

Done

Good idea, but two try-catch blocks introduced too much nesting here, so I'll make error message conditional basing on shouldDoMigration

bartek: Good idea, but two try-catch blocks introduced too much nesting here, so I'll make error…

backupUploadInProgress.current = false; backupUploadInProgress.current = false;

})(); })();

}, [ }, [

allPeerDevices,

broadcastDeviceListUpdates,

canPerformBackupOperation, canPerformBackupOperation,

checkIfPrimaryDevice, checkIfPrimaryDevice,

createUserKeysBackup, createUserKeysBackup,

dispatch,

dispatchActionPromise, dispatchActionPromise,

getAndUpdateDeviceListsForUsers,

handlerStarted, handlerStarted,

identityContext,

latestBackupInfo, latestBackupInfo,

retrieveLatestBackupInfo,

showAlertToStaff, showAlertToStaff,

staffCanSee, staffCanSee,

testUserKeysRestore, testUserKeysRestore,

userIdentifier,

]); ]);

return null; return null;

} }

export default BackupHandler; export default BackupHandler;

[lib] Create a wrapper over dispatchClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 38373

lib/hooks/ops-hooks.js

[lib] Create a wrapper over dispatch
ClosedPublic
Actions

Revision Contents
Changeset List