fix: accounts not syncing between devices bug #11801
Conversation
Jonathansoufer
added
needs-dev-review
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
Jonathansoufer
added
the
skip-sonar-cloud
Jonathansoufer
added
Run Smoke E2E
|
|
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| const memoAccounts = useMemo(() => accounts.map((account) => account.address),[accounts]); |
There was a problem hiding this comment.
Question around this usage of useMemo, (and the useCallback in this file).
I know you didn't implement it but is it needed?
If we're not having performance issues around these functions then it can be removed. We have a lot of useMemo and useCallback usage in the app and we're allocating device memory to each one. Removing any unnecessary usage would be great.
| useEffect(() => { | ||
| const updateAndfetchAccountSettings = useCallback(async () => { | ||
| try { | ||
| const memoAccounts: string[] = memoizedAccounts; | ||
| update(memoAccounts); | ||
| } catch { | ||
| setError('Failed to get account settings'); | ||
| } finally { | ||
| setLoading(false); | ||
| Engine.context.NotificationServicesController.checkAccountsPresence( | ||
| memoAccounts, | ||
| ).then((result) => { | ||
| dispatch(updateAccountState(result)); | ||
| return result; | ||
| }); | ||
| } catch (err) { | ||
| Logger.log(err, 'Failed to get account settings'); | ||
| } | ||
| }, [memoizedAccounts, update]); | ||
| }, [dispatch, memoAccounts]); | ||
|
|
||
| return { | ||
| data, | ||
| initialLoading: loading, | ||
| error, | ||
| accountsBeingUpdated, | ||
| update, | ||
| }; | ||
| return { updateAndfetchAccountSettings }; |
There was a problem hiding this comment.
Same as above. Do we need to wrap this in a useCallback?
|
Looks good @Jonathansoufer, just a couple of comments. Also, what's the purpose of the |
| // refetch the account settings from the controller | ||
| await updateAndfetchAccountSettings(); | ||
| // refetch the notifications from the controller | ||
| await refetch(); |
There was a problem hiding this comment.
Is it necessary to execute this sequentially, or can it be done in parallel using Promise.all? Doing it in parallel could potentially improve performance.
| return result; | ||
| }); | ||
| } catch (err) { | ||
| Logger.log(err, 'Failed to get account settings'); |
There was a problem hiding this comment.
I noticed the use of then() alongside a try-catch block for handling the asynchronous function. To maintain consistency and clarity, it might be a good idea to choose between using one of the following approaches:
Using then() and catch(): This will maintain the current promise-chaining style:
Engine.context.NotificationServicesController.checkAccountsPresence(memoAccounts)
.then((result) => {
dispatch(updateAccountState(result));
return result;
})
.catch((err) => {
Logger.log('Failed to get account settings:', err);
});
Using async/await with try-catch: This will make the asynchronous handling more streamlined:
try {
const result = await Engine.context.NotificationServicesController.checkAccountsPresence(memoAccounts);
dispatch(updateAccountState(result));
return result;
} catch (err) {
Logger.log('Failed to get account settings:', err);
}
Both methods are valid, but it's important to stick to one approach for consistency
|
great work @Jonathansoufer , i just left two small comments regarding consistency and perf improvement |
Description
This PR fixed an issue preventing mobile accounts to sync its states (enable/disable) with other clients (platform and extension). NOTIFY-1218
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
account-sync-fix.mov
Pre-merge author checklist
Pre-merge reviewer checklist