From 764aa35e35731db7613c8404fb77d7f99ee4b3f6 Mon Sep 17 00:00:00 2001 From: Audric Ackermann Date: Mon, 25 Nov 2024 15:02:23 +1100 Subject: [PATCH] chore: address PR reviews --- .../job_runners/jobs/AvatarDownloadJob.ts | 10 ++---- .../utils/job_runners/jobs/GroupInviteJob.ts | 13 +++----- .../jobs/GroupPendingRemovalsJob.ts | 11 +++---- .../utils/job_runners/jobs/GroupPromoteJob.ts | 9 ++---- .../utils/job_runners/jobs/GroupSyncJob.ts | 12 +++---- .../utils/job_runners/jobs/UserSyncJob.ts | 2 +- ts/state/selectors/groups.ts | 32 ++++++++++++------- .../libsession_wrapper_metagroup_test.ts | 7 ++-- 8 files changed, 44 insertions(+), 52 deletions(-) diff --git a/ts/session/utils/job_runners/jobs/AvatarDownloadJob.ts b/ts/session/utils/job_runners/jobs/AvatarDownloadJob.ts index 3a0fd7dc4d..2f3e57c5c8 100644 --- a/ts/session/utils/job_runners/jobs/AvatarDownloadJob.ts +++ b/ts/session/utils/job_runners/jobs/AvatarDownloadJob.ts @@ -17,7 +17,7 @@ import { } from '../PersistedJob'; const defaultMsBetweenRetries = 10000; -const defaultMaxAttemps = 3; +const defaultMaxAttempts = 3; /** * Returns true if the provided conversationId is a private chat and that we should add an Avatar Download Job to the list of jobs to run. @@ -74,11 +74,7 @@ class AvatarDownloadJob extends PersistedJob { Partial< Pick< AvatarDownloadPersistedData, - | 'nextAttemptTimestamp' - | 'identifier' - | 'maxAttempts' - | 'delayBetweenRetries' - | 'currentRetry' + 'nextAttemptTimestamp' | 'identifier' | 'maxAttempts' | 'currentRetry' > >) { super({ @@ -86,7 +82,7 @@ class AvatarDownloadJob extends PersistedJob { identifier: identifier || v4(), conversationId, delayBetweenRetries: defaultMsBetweenRetries, - maxAttempts: isNumber(maxAttempts) ? maxAttempts : defaultMaxAttemps, + maxAttempts: isNumber(maxAttempts) ? maxAttempts : defaultMaxAttempts, nextAttemptTimestamp: nextAttemptTimestamp || Date.now() + defaultMsBetweenRetries, currentRetry: isNumber(currentRetry) ? currentRetry : 0, }); diff --git a/ts/session/utils/job_runners/jobs/GroupInviteJob.ts b/ts/session/utils/job_runners/jobs/GroupInviteJob.ts index ed2b413e67..0ad040cdd2 100644 --- a/ts/session/utils/job_runners/jobs/GroupInviteJob.ts +++ b/ts/session/utils/job_runners/jobs/GroupInviteJob.ts @@ -24,6 +24,7 @@ import { MessageQueue } from '../../../sending'; import { NetworkTime } from '../../../../util/NetworkTime'; import { SubaccountUnrevokeSubRequest } from '../../../apis/snode_api/SnodeRequestTypes'; import { GroupSync } from './GroupSyncJob'; +import { DURATION } from '../../../constants'; const defaultMsBetweenRetries = 10000; const defaultMaxAttempts = 1; @@ -34,7 +35,7 @@ type JobExtraArgs = { inviteAsAdmin: boolean; /** * When inviting a member, we usually only want to sent a message to his swarm. - * In the case of a invitation resend process though, we also want to make sure his token is unrevoked from the group's swarm. + * In the case of an invitation resend process though, we also want to make sure his token is unrevoked from the group's swarm. * */ forceUnrevoke: boolean; @@ -151,11 +152,7 @@ class GroupInviteJob extends PersistedJob { Partial< Pick< GroupInvitePersistedData, - | 'nextAttemptTimestamp' - | 'identifier' - | 'maxAttempts' - | 'delayBetweenRetries' - | 'currentRetry' + 'nextAttemptTimestamp' | 'identifier' | 'maxAttempts' | 'currentRetry' > >) { super({ @@ -298,7 +295,7 @@ class GroupInviteJob extends PersistedJob { } public getJobTimeoutMs(): number { - return 15000; + return 15 * DURATION.SECONDS; } } @@ -321,7 +318,7 @@ function updateFailedStateForMember(groupPk: GroupPubkeyType, member: PubkeyType if (!thisGroupFailure) { thisGroupFailure = { failedMembers: [], - debouncedCall: debounce(displayFailedInvitesForGroup, 1000), // TODO change to 5000 + debouncedCall: debounce(displayFailedInvitesForGroup, 5 * DURATION.SECONDS), }; } diff --git a/ts/session/utils/job_runners/jobs/GroupPendingRemovalsJob.ts b/ts/session/utils/job_runners/jobs/GroupPendingRemovalsJob.ts index 4269a9056b..9a4ec1492b 100644 --- a/ts/session/utils/job_runners/jobs/GroupPendingRemovalsJob.ts +++ b/ts/session/utils/job_runners/jobs/GroupPendingRemovalsJob.ts @@ -36,6 +36,7 @@ import { WithSecretKey, } from '../../../types/with'; import { groupInfoActions } from '../../../../state/ducks/metaGroups'; +import { DURATION } from '../../../constants'; const defaultMsBetweenRetries = 10000; const defaultMaxAttempts = 1; @@ -98,11 +99,7 @@ class GroupPendingRemovalsJob extends PersistedJob >) { super({ @@ -135,7 +132,7 @@ class GroupPendingRemovalsJob extends PersistedJob m.removedStatus === 'REMOVED_MEMBER_AND_MESSAGES') + .filter(m => m.memberStatus === 'REMOVED_MEMBER_AND_MESSAGES') .map(m => m.pubkeyHex); const sessionIdsHex = pendingRemovals.map(m => m.pubkeyHex); @@ -274,7 +271,7 @@ class GroupPendingRemovalsJob extends PersistedJob { Partial< Pick< GroupPromotePersistedData, - | 'nextAttemptTimestamp' - | 'identifier' - | 'maxAttempts' - | 'delayBetweenRetries' - | 'currentRetry' + 'nextAttemptTimestamp' | 'identifier' | 'maxAttempts' | 'currentRetry' > >) { super({ @@ -153,7 +150,7 @@ class GroupPromoteJob extends PersistedJob { } public getJobTimeoutMs(): number { - return 15000; + return 15 * DURATION.SECONDS; } } diff --git a/ts/session/utils/job_runners/jobs/GroupSyncJob.ts b/ts/session/utils/job_runners/jobs/GroupSyncJob.ts index bbdfa16996..57d88ed840 100644 --- a/ts/session/utils/job_runners/jobs/GroupSyncJob.ts +++ b/ts/session/utils/job_runners/jobs/GroupSyncJob.ts @@ -157,9 +157,9 @@ async function pushChangesToGroupSwarmIfNeeded({ ]); const result = await MessageSender.sendEncryptedDataToSnode({ - // Note: this is on purpose that supplementalKeysSubRequest is before pendingConfigRequests - // as this is to avoid a race condition where a device is polling right - // while we are posting the configs (already encrypted with the new keys) + // Note: this is on purpose that supplementalKeysSubRequest is before pendingConfigRequests. + // This is to avoid a race condition where a device is polling while we + // are posting the configs (already encrypted with the new keys) sortedSubRequests, destination: groupPk, method: 'sequence', @@ -198,7 +198,7 @@ async function pushChangesToGroupSwarmIfNeeded({ class GroupSyncJob extends PersistedJob { constructor({ - identifier, // this has to be the pubkey to which we + identifier, // this has to be the group's pubkey nextAttemptTimestamp, maxAttempts, currentRetry, @@ -241,8 +241,6 @@ class GroupSyncJob extends PersistedJob { groupPk: thisJobDestination, extraStoreRequests: [], }); - - // eslint-disable-next-line no-useless-catch } catch (e) { window.log.warn('GroupSyncJob failed with', e.message); return RunJobResult.RetryJobIfPossible; @@ -251,7 +249,7 @@ class GroupSyncJob extends PersistedJob { `GroupSyncJob ${ed25519Str(thisJobDestination)} run() took ${Date.now() - start}ms` ); - // this is a simple way to make sure whatever happens here, we update the lastest timestamp. + // this is a simple way to make sure whatever happens here, we update the latest timestamp. // (a finally statement is always executed (no matter if exception or returns in other try/catch block) this.updateLastTickTimestamp(); } diff --git a/ts/session/utils/job_runners/jobs/UserSyncJob.ts b/ts/session/utils/job_runners/jobs/UserSyncJob.ts index 545489e2a3..e0de293ee7 100644 --- a/ts/session/utils/job_runners/jobs/UserSyncJob.ts +++ b/ts/session/utils/job_runners/jobs/UserSyncJob.ts @@ -188,7 +188,7 @@ class UserSyncJob extends PersistedJob { } finally { window.log.debug(`UserSyncJob run() took ${Date.now() - start}ms`); - // this is a simple way to make sure whatever happens here, we update the lastest timestamp. + // this is a simple way to make sure whatever happens here, we update the latest timestamp. // (a finally statement is always executed (no matter if exception or returns in other try/catch block) this.updateLastTickTimestamp(); } diff --git a/ts/state/selectors/groups.ts b/ts/state/selectors/groups.ts index bf896084df..5bf75f9c35 100644 --- a/ts/state/selectors/groups.ts +++ b/ts/state/selectors/groups.ts @@ -108,8 +108,12 @@ function getMemberPromotionNotSent(state: StateType, pubkey: PubkeyType, convo?: function getMemberPendingRemoval(state: StateType, pubkey: PubkeyType, convo?: GroupPubkeyType) { const members = getMembersOfGroup(state, convo); - const removedStatus = findMemberInMembers(members, pubkey)?.removedStatus; - return removedStatus !== 'NOT_REMOVED'; + const removedStatus = findMemberInMembers(members, pubkey)?.memberStatus; + return ( + removedStatus === 'REMOVED_UNKNOWN' || + removedStatus === 'REMOVED_MEMBER' || + removedStatus === 'REMOVED_MEMBER_AND_MESSAGES' + ); } export function getLibMembersCount(state: StateType, convo?: GroupPubkeyType): Array { @@ -317,32 +321,38 @@ export function useStateOf03GroupMembers(convoId?: string) { switch (item.memberStatus) { case 'INVITE_FAILED': case 'INVITE_NOT_SENT': - stateSortingOrder = -5; + stateSortingOrder = -50; break; case 'INVITE_SENDING': - stateSortingOrder = -4; + stateSortingOrder = -40; break; case 'INVITE_SENT': - stateSortingOrder = -3; + stateSortingOrder = -30; + break; + case 'REMOVED_UNKNOWN': // fallback, hopefully won't happen in production + case 'REMOVED_MEMBER': // we want pending removal members at the end + case 'REMOVED_MEMBER_AND_MESSAGES': + stateSortingOrder = -20; break; case 'PROMOTION_FAILED': case 'PROMOTION_NOT_SENT': - stateSortingOrder = -2; + stateSortingOrder = -15; break; case 'PROMOTION_SENDING': - stateSortingOrder = -1; + stateSortingOrder = -10; break; case 'PROMOTION_SENT': stateSortingOrder = 0; break; case 'PROMOTION_ACCEPTED': - stateSortingOrder = 1; + stateSortingOrder = 10; break; case 'INVITE_ACCEPTED': - stateSortingOrder = 2; + stateSortingOrder = 20; break; - case 'UNKNOWN': - stateSortingOrder = 5; // just a fallback, hopefully won't happen in production + case 'INVITE_UNKNOWN': // fallback, hopefully won't happen in production + case 'PROMOTION_UNKNOWN': // fallback, hopefully won't happen in production + stateSortingOrder = 50; break; default: diff --git a/ts/test/session/unit/libsession_wrapper/libsession_wrapper_metagroup_test.ts b/ts/test/session/unit/libsession_wrapper/libsession_wrapper_metagroup_test.ts index a05c2bfe3d..1899027008 100644 --- a/ts/test/session/unit/libsession_wrapper/libsession_wrapper_metagroup_test.ts +++ b/ts/test/session/unit/libsession_wrapper/libsession_wrapper_metagroup_test.ts @@ -25,7 +25,6 @@ function emptyMember(pubkeyHex: PubkeyType): GroupMemberGet { url: null, }, nominatedAdmin: false, - removedStatus: 'NOT_REMOVED', pubkeyHex, }; } @@ -299,8 +298,7 @@ describe('libsession_metagroup', () => { expect(metaGroupWrapper.memberGetAll().length).to.be.deep.eq(1); const expected: GroupMemberGet = { ...emptyMember(member), - removedStatus: 'REMOVED_MEMBER_AND_MESSAGES', - memberStatus: 'INVITE_ACCEPTED', // marking a member as pending removal auto-marks him as accepted (so we don't retry sending an invite) + memberStatus: 'REMOVED_MEMBER_AND_MESSAGES', }; expect(metaGroupWrapper.memberGetAll().length).to.be.deep.eq(1); expect(metaGroupWrapper.memberGetAll()[0]).to.be.deep.eq(expected); @@ -312,8 +310,7 @@ describe('libsession_metagroup', () => { expect(metaGroupWrapper.memberGetAll().length).to.be.deep.eq(1); const expected: GroupMemberGet = { ...emptyMember(member), - removedStatus: 'REMOVED_MEMBER', - memberStatus: 'INVITE_ACCEPTED', // marking a member as pending removal auto-marks him as accepted (so we don't retry sending an invite) + memberStatus: 'REMOVED_MEMBER', }; expect(metaGroupWrapper.memberGetAll()).to.be.deep.eq([expected]); });