Optimize profile avatar uploads and sync urls

ts/components/AvatarPreview.tsx

@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: AGPL-3.0-only
 
 import type { CSSProperties } from 'react';
-import React, { useEffect, useRef, useState } from 'react';
+import React, { useEffect, useState } from 'react';
 import { noop } from 'lodash';
 
 import * as log from '../logging/log';
@@ -46,10 +46,6 @@ export const AvatarPreview = ({
   onClick,
   style = {},
 }: PropsType): JSX.Element => {
-  const startingAvatarPathRef = useRef<undefined | string>(
-    avatarValue ? undefined : avatarPath
-  );
-
   const [avatarPreview, setAvatarPreview] = useState<Uint8Array | undefined>();
 
   // Loads the initial avatarPath if one is provided, but only if we're in editable mode.
@@ -60,8 +56,7 @@ export const AvatarPreview = ({
       return;
     }
 
-    const startingAvatarPath = startingAvatarPathRef.current;
-    if (!startingAvatarPath) {
+    if (!avatarPath) {
       return noop;
     }
 
@@ -69,14 +64,12 @@ export const AvatarPreview = ({
 
     (async () => {
       try {
-        const buffer = await imagePathToBytes(startingAvatarPath);
+        const buffer = await imagePathToBytes(avatarPath);
         if (shouldCancel) {
           return;
         }
         setAvatarPreview(buffer);
-        if (onAvatarLoaded) {
-          onAvatarLoaded(buffer);
-        }
+        onAvatarLoaded?.(buffer);
       } catch (err) {
         if (shouldCancel) {
           return;
@@ -92,7 +85,7 @@ export const AvatarPreview = ({
     return () => {
       shouldCancel = true;
     };
-  }, [onAvatarLoaded, isEditable]);
+  }, [avatarPath, onAvatarLoaded, isEditable]);
 
   // Ensures that when avatarValue changes we generate new URLs
   useEffect(() => {

ts/components/ProfileEditor.stories.tsx

@@ -25,7 +25,7 @@ const stories = storiesOf('Components/ProfileEditor', module);
 const createProps = (overrideProps: Partial<PropsType> = {}): PropsType => ({
   aboutEmoji: overrideProps.aboutEmoji,
   aboutText: text('about', overrideProps.aboutText || ''),
-  avatarPath: overrideProps.avatarPath,
+  profileAvatarPath: overrideProps.profileAvatarPath,
   clearUsernameSave: action('clearUsernameSave'),
   conversationId: '123',
   color: overrideProps.color || getRandomColor(),
@@ -64,7 +64,7 @@ stories.add('Full Set', () => {
       {...createProps({
         aboutEmoji: '🙏',
         aboutText: 'Live. Laugh. Love',
-        avatarPath: '/fixtures/kitten-3-64-64.jpg',
+        profileAvatarPath: '/fixtures/kitten-3-64-64.jpg',
         onSetSkinTone: setSkinTone,
         familyName: getLastName(),
         skinTone,

ts/components/ProfileEditor.tsx

@@ -9,6 +9,7 @@ import type { AvatarColorType } from '../types/Colors';
 import { AvatarColors } from '../types/Colors';
 import type {
   AvatarDataType,
+  AvatarUpdateType,
   DeleteAvatarFromDiskActionType,
   ReplaceAvatarActionType,
   SaveAvatarToDiskActionType,
@@ -58,14 +59,14 @@ type PropsExternalType = {
   onEditStateChanged: (editState: EditState) => unknown;
   onProfileChanged: (
     profileData: ProfileDataType,
-    avatarBuffer?: Uint8Array
+    avatar: AvatarUpdateType
   ) => unknown;
 };
 
 export type PropsDataType = {
   aboutEmoji?: string;
   aboutText?: string;
-  avatarPath?: string;
+  profileAvatarPath?: string;
   color?: AvatarColorType;
   conversationId: string;
   familyName?: string;
@@ -211,7 +212,7 @@ function mapSaveStateToEditState({
 export const ProfileEditor = ({
   aboutEmoji,
   aboutText,
-  avatarPath,
+  profileAvatarPath,
   clearUsernameSave,
   color,
   conversationId,
@@ -254,9 +255,16 @@ export const ProfileEditor = ({
     UsernameEditState.Editing
   );
 
+  const [startingAvatarPath, setStartingAvatarPath] =
+    useState(profileAvatarPath);
+
+  const [oldAvatarBuffer, setOldAvatarBuffer] = useState<
+    Uint8Array | undefined
+  >(undefined);
   const [avatarBuffer, setAvatarBuffer] = useState<Uint8Array | undefined>(
     undefined
   );
+  const [isLoadingAvatar, setIsLoadingAvatar] = useState(true);
   const [stagedProfile, setStagedProfile] = useState<ProfileDataType>({
     aboutEmoji,
     aboutText,
@@ -285,6 +293,9 @@ export const ProfileEditor = ({
   // To make AvatarEditor re-render less often
   const handleAvatarChanged = useCallback(
     (avatar: Uint8Array | undefined) => {
+      // Do not display stale avatar from disk anymore.
+      setStartingAvatarPath(undefined);
+
       setAvatarBuffer(avatar);
       setEditState(EditState.None);
       onProfileChanged(
@@ -295,10 +306,11 @@ export const ProfileEditor = ({
             ? trim(stagedProfile.familyName)
             : undefined,
         },
-        avatar
+        { oldAvatar: oldAvatarBuffer, newAvatar: avatar }
       );
+      setOldAvatarBuffer(avatar);
     },
-    [onProfileChanged, stagedProfile]
+    [onProfileChanged, stagedProfile, oldAvatarBuffer]
   );
 
   const getFullNameText = () => {
@@ -405,9 +417,14 @@ export const ProfileEditor = ({
   };
 
   // To make AvatarEditor re-render less often
-  const handleAvatarLoaded = useCallback(avatar => {
-    setAvatarBuffer(avatar);
-  }, []);
+  const handleAvatarLoaded = useCallback(
+    avatar => {
+      setAvatarBuffer(avatar);
+      setOldAvatarBuffer(avatar);
+      setIsLoadingAvatar(false);
+    },
+    [setAvatarBuffer, setOldAvatarBuffer, setIsLoadingAvatar]
+  );
 
   let content: JSX.Element;
 
@@ -415,7 +432,7 @@ export const ProfileEditor = ({
     content = (
       <AvatarEditor
         avatarColor={color || AvatarColors[0]}
-        avatarPath={avatarPath}
+        avatarPath={startingAvatarPath}
         avatarValue={avatarBuffer}
         conversationId={conversationId}
         conversationTitle={getFullNameText()}
@@ -430,6 +447,7 @@ export const ProfileEditor = ({
     );
   } else if (editState === EditState.ProfileName) {
     const shouldDisableSave =
+      isLoadingAvatar ||
       !stagedProfile.firstName ||
       (stagedProfile.firstName === fullName.firstName &&
         stagedProfile.familyName === fullName.familyName) ||
@@ -502,7 +520,10 @@ export const ProfileEditor = ({
                 familyName: stagedProfile.familyName,
               });
 
-              onProfileChanged(stagedProfile, avatarBuffer);
+              onProfileChanged(stagedProfile, {
+                oldAvatar: oldAvatarBuffer,
+                newAvatar: avatarBuffer,
+              });
               handleBack();
             }}
           >
@@ -513,8 +534,9 @@ export const ProfileEditor = ({
     );
   } else if (editState === EditState.Bio) {
     const shouldDisableSave =
-      stagedProfile.aboutText === fullBio.aboutText &&
-      stagedProfile.aboutEmoji === fullBio.aboutEmoji;
+      isLoadingAvatar ||
+      (stagedProfile.aboutText === fullBio.aboutText &&
+        stagedProfile.aboutEmoji === fullBio.aboutEmoji);
 
     content = (
       <>
@@ -613,7 +635,10 @@ export const ProfileEditor = ({
                 aboutText: stagedProfile.aboutText,
               });
 
-              onProfileChanged(stagedProfile, avatarBuffer);
+              onProfileChanged(stagedProfile, {
+                oldAvatar: oldAvatarBuffer,
+                newAvatar: avatarBuffer,
+              });
               handleBack();
             }}
           >
@@ -689,7 +714,7 @@ export const ProfileEditor = ({
       <>
         <AvatarPreview
           avatarColor={color}
-          avatarPath={avatarPath}
+          avatarPath={startingAvatarPath}
           avatarValue={avatarBuffer}
           conversationTitle={getFullNameText()}
           i18n={i18n}

ts/components/ProfileEditorModal.tsx

@@ -7,6 +7,7 @@ import { ConfirmationDialog } from './ConfirmationDialog';
 import type { PropsType as ProfileEditorPropsType } from './ProfileEditor';
 import { ProfileEditor, EditState } from './ProfileEditor';
 import type { ProfileDataType } from '../state/ducks/conversations';
+import type { AvatarUpdateType } from '../types/Avatar';
 
 export type PropsDataType = {
   hasError: boolean;
@@ -15,7 +16,7 @@ export type PropsDataType = {
 type PropsType = {
   myProfileChanged: (
     profileData: ProfileDataType,
-    avatarBuffer?: Uint8Array
+    avatar: AvatarUpdateType
   ) => unknown;
   toggleProfileEditor: () => unknown;
   toggleProfileEditorHasError: () => unknown;

ts/models/conversations.ts

@@ -1846,6 +1846,7 @@ export class ConversationModel extends window.Backbone
       avatarPath: this.getAbsoluteAvatarPath(),
       avatarHash: this.getAvatarHash(),
       unblurredAvatarPath: this.getAbsoluteUnblurredAvatarPath(),
+      profileAvatarPath: this.getAbsoluteProfileAvatarPath(),
       color,
       conversationColor: this.getConversationColor(),
       customColor,
@@ -5019,6 +5020,11 @@ export class ConversationModel extends window.Backbone
     return avatarPath ? getAbsoluteAttachmentPath(avatarPath) : undefined;
   }
 
+  getAbsoluteProfileAvatarPath(): string | undefined {
+    const avatarPath = this.get('profileAvatar')?.path;
+    return avatarPath ? getAbsoluteAttachmentPath(avatarPath) : undefined;
+  }
+
   getAbsoluteUnblurredAvatarPath(): string | undefined {
     const unblurredAvatarPath = this.get('unblurredAvatarPath');
     return unblurredAvatarPath

ts/services/storageRecordOps.ts

@@ -178,7 +178,10 @@ export function toAccountRecord(
   if (conversation.get('profileFamilyName')) {
     accountRecord.familyName = conversation.get('profileFamilyName') || '';
   }
-  accountRecord.avatarUrl = window.storage.get('avatarUrl') || '';
+  const avatarUrl = window.storage.get('avatarUrl');
+  if (avatarUrl !== undefined) {
+    accountRecord.avatarUrl = avatarUrl;
+  }
   accountRecord.noteToSelfArchived = Boolean(conversation.get('isArchived'));
   accountRecord.noteToSelfMarkedUnread = Boolean(
     conversation.get('markedUnread')
@@ -895,7 +898,6 @@ export async function mergeAccountRecord(
 ): Promise<MergeResultType> {
   let details = new Array<string>();
   const {
-    avatarUrl,
     linkPreviews,
     notDiscoverableByPhoneNumber,
     noteToSelfArchived,
@@ -1146,10 +1148,9 @@ export async function mergeAccountRecord(
       { viaStorageServiceSync: true }
     );
 
-    if (avatarUrl) {
-      await conversation.setProfileAvatar(avatarUrl, profileKey);
-      window.storage.put('avatarUrl', avatarUrl);
-    }
+    const avatarUrl = dropNull(accountRecord.avatarUrl);
+    await conversation.setProfileAvatar(avatarUrl, profileKey);
+    window.storage.put('avatarUrl', avatarUrl);
   }
 
   const { hasConflict, details: extraDetails } = doesRecordHavePendingChanges(

ts/services/writeProfile.ts

@@ -11,10 +11,11 @@ import { getProfile } from '../util/getProfile';
 import { singleProtoJobQueue } from '../jobs/singleProtoJobQueue';
 import { strictAssert } from '../util/assert';
 import { isWhitespace } from '../util/whitespaceStringUtil';
+import type { AvatarUpdateType } from '../types/Avatar';
 
 export async function writeProfile(
   conversation: ConversationType,
-  avatarBuffer?: Uint8Array
+  avatar: AvatarUpdateType
 ): Promise<void> {
   // Before we write anything we request the user's profile so that we can
   // have an up-to-date paymentAddress to be able to include it when we write
@@ -41,7 +42,7 @@ export async function writeProfile(
 
   const [profileData, encryptedAvatarData] = await encryptProfileData(
     conversation,
-    avatarBuffer
+    avatar
   );
   const avatarRequestHeaders = await window.textsecure.messaging.putProfile(
     profileData
@@ -50,37 +51,49 @@ export async function writeProfile(
   // Upload the avatar if provided
   // delete existing files on disk if avatar has been removed
   // update the account's avatar path and hash if it's a new avatar
-  let profileAvatar:
-    | {
-        hash: string;
-        path: string;
-      }
-    | undefined;
-  if (avatarRequestHeaders && encryptedAvatarData && avatarBuffer) {
-    await window.textsecure.messaging.uploadAvatar(
+  const { newAvatar } = avatar;
+  let maybeProfileAvatarUpdate: {
+    profileAvatar?:
+      | {
+          hash: string;
+          path: string;
+        }
+      | undefined;
+  } = {};
+  if (profileData.sameAvatar) {
+    log.info('writeProfile: not updating avatar');
+  } else if (avatarRequestHeaders && encryptedAvatarData && newAvatar) {
+    log.info('writeProfile: uploading new avatar');
+    const avatarUrl = await window.textsecure.messaging.uploadAvatar(
       avatarRequestHeaders,
       encryptedAvatarData
     );
 
-    const hash = await computeHash(avatarBuffer);
+    const hash = await computeHash(newAvatar);
 
     if (hash !== avatarHash) {
+      log.info('writeProfile: removing old avatar and saving the new one');
       const [path] = await Promise.all([
-        window.Signal.Migrations.writeNewAttachmentData(avatarBuffer),
+        window.Signal.Migrations.writeNewAttachmentData(newAvatar),
         avatarPath
           ? window.Signal.Migrations.deleteAttachmentData(avatarPath)
           : undefined,
       ]);
-      profileAvatar = {
-        hash,
-        path,
+      maybeProfileAvatarUpdate = {
+        profileAvatar: { hash, path },
       };
     }
-  } else if (avatarPath) {
-    await window.Signal.Migrations.deleteAttachmentData(avatarPath);
-  }
 
-  const profileAvatarData = profileAvatar ? { profileAvatar } : {};
+    await window.storage.put('avatarUrl', avatarUrl);
+  } else if (avatarPath) {
+    log.info('writeProfile: removing avatar');
+    await Promise.all([
+      window.Signal.Migrations.deleteAttachmentData(avatarPath),
+      window.storage.put('avatarUrl', undefined),
+    ]);
+
+    maybeProfileAvatarUpdate = { profileAvatar: undefined };
+  }
 
   // Update backbone, update DB, run storage service upload
   model.set({
@@ -88,7 +101,7 @@ export async function writeProfile(
     aboutEmoji,
     profileName: firstName,
     profileFamilyName: familyName,
-    ...profileAvatarData,
+    ...maybeProfileAvatarUpdate,
   });
 
   dataInterface.updateConversation(model.attributes);

ts/state/ducks/conversations.ts

@@ -63,7 +63,7 @@ import {
   getMe,
   getUsernameSaveState,
 } from '../selectors/conversations';
-import type { AvatarDataType } from '../../types/Avatar';
+import type { AvatarDataType, AvatarUpdateType } from '../../types/Avatar';
 import { getDefaultAvatars } from '../../types/Avatar';
 import { getAvatarData } from '../../util/getAvatarData';
 import { isSameAvatarData } from '../../util/isSameAvatarData';
@@ -121,6 +121,7 @@ export type ConversationType = {
   avatars?: Array<AvatarDataType>;
   avatarPath?: string;
   avatarHash?: string;
+  profileAvatarPath?: string;
   unblurredAvatarPath?: string;
   areWeAdmin?: boolean;
   areWePending?: boolean;
@@ -1095,7 +1096,7 @@ function saveUsername({
 
 function myProfileChanged(
   profileData: ProfileDataType,
-  avatarBuffer?: Uint8Array
+  avatar: AvatarUpdateType
 ): ThunkAction<
   void,
   RootStateType,
@@ -1111,7 +1112,7 @@ function myProfileChanged(
           ...conversation,
           ...profileData,
         },
-        avatarBuffer
+        avatar
       );
 
       // writeProfile above updates the backbone model which in turn updates

ts/state/smart/ProfileEditorModal.ts

@@ -17,7 +17,7 @@ function mapStateToProps(
 ): Omit<PropsDataType, 'onEditStateChange' | 'onProfileChanged'> &
   ProfileEditorModalPropsType {
   const {
-    avatarPath,
+    profileAvatarPath,
     avatars: userAvatarData = [],
     aboutText,
     aboutEmoji,
@@ -34,7 +34,7 @@ function mapStateToProps(
   return {
     aboutEmoji,
     aboutText,
-    avatarPath,
+    profileAvatarPath,
     color,
     conversationId,
     familyName,

ts/test-electron/util/encryptProfileData_test.ts

@@ -10,13 +10,17 @@ import {
   decryptProfileName,
   decryptProfile,
 } from '../../Crypto';
+import type { ConversationType } from '../../state/ducks/conversations';
 import { UUID } from '../../types/UUID';
 import { encryptProfileData } from '../../util/encryptProfileData';
 
 describe('encryptProfileData', () => {
-  it('encrypts and decrypts properly', async () => {
-    const keyBuffer = getRandomBytes(32);
-    const conversation = {
+  let keyBuffer: Uint8Array;
+  let conversation: ConversationType;
+
+  beforeEach(() => {
+    keyBuffer = getRandomBytes(32);
+    conversation = {
       aboutEmoji: '🐢',
       aboutText: 'I like turtles',
       familyName: 'Kid',
@@ -33,8 +37,13 @@ describe('encryptProfileData', () => {
       title: '',
       type: 'direct' as const,
     };
+  });
 
-    const [encrypted] = await encryptProfileData(conversation);
+  it('encrypts and decrypts properly', async () => {
+    const [encrypted] = await encryptProfileData(conversation, {
+      oldAvatar: undefined,
+      newAvatar: undefined,
+    });
 
     assert.isDefined(encrypted.version);
     assert.isDefined(encrypted.name);
@@ -83,4 +92,22 @@ describe('encryptProfileData', () => {
       assert.isDefined(encrypted.aboutEmoji);
     }
   });
+
+  it('sets sameAvatar to true when avatars are the same', async () => {
+    const [encrypted] = await encryptProfileData(conversation, {
+      oldAvatar: new Uint8Array([1, 2, 3]),
+      newAvatar: new Uint8Array([1, 2, 3]),
+    });
+
+    assert.isTrue(encrypted.sameAvatar);
+  });
+
+  it('sets sameAvatar to false when avatars are different', async () => {
+    const [encrypted] = await encryptProfileData(conversation, {
+      oldAvatar: new Uint8Array([1, 2, 3]),
+      newAvatar: new Uint8Array([4, 5, 6, 7]),
+    });
+
+    assert.isFalse(encrypted.sameAvatar);
+  });
 });

ts/textsecure/WebAPI.ts

@@ -701,6 +701,7 @@ export type ProfileRequestDataType = {
   about: string | null;
   aboutEmoji: string | null;
   avatar: boolean;
+  sameAvatar: boolean;
   commitment: string;
   name: string;
   paymentAddress: string | null;

ts/types/Avatar.ts

@@ -65,6 +65,11 @@ export type SaveAvatarToDiskActionType = (
   conversationId?: string
 ) => unknown;
 
+export type AvatarUpdateType = Readonly<{
+  oldAvatar: Uint8Array | undefined;
+  newAvatar: Uint8Array | undefined;
+}>;
+
 const groupIconColors = [
   'A180',
   'A120',

ts/types/Storage.d.ts

@@ -107,7 +107,7 @@ export type StorageAccessType = {
   typingIndicators: boolean;
   sealedSenderIndicators: boolean;
   storageFetchComplete: boolean;
-  avatarUrl: string;
+  avatarUrl: string | undefined;
   manifestVersion: number;
   storageCredentials: StorageServiceCredentials;
   'storage-service-error-records': Array<UnknownRecord>;

ts/util/encryptProfileData.ts

@@ -10,11 +10,12 @@ import {
   encryptProfile,
   encryptProfileItemWithPadding,
 } from '../Crypto';
+import type { AvatarUpdateType } from '../types/Avatar';
 import { deriveProfileKeyCommitment, deriveProfileKeyVersion } from './zkgroup';
 
 export async function encryptProfileData(
   conversation: ConversationType,
-  avatarBuffer?: Uint8Array
+  { oldAvatar, newAvatar }: AvatarUpdateType
 ): Promise<[ProfileRequestDataType, Uint8Array | undefined]> {
   const {
     aboutEmoji,
@@ -55,10 +56,12 @@ export async function encryptProfileData(
       )
     : null;
 
-  const encryptedAvatarData = avatarBuffer
-    ? encryptProfile(avatarBuffer, keyBuffer)
+  const encryptedAvatarData = newAvatar
+    ? encryptProfile(newAvatar, keyBuffer)
     : undefined;
 
+  const sameAvatar = Bytes.areEqual(oldAvatar, newAvatar);
+
   const profileData = {
     version: deriveProfileKeyVersion(profileKey, uuid),
     name: Bytes.toBase64(bytesName),
@@ -66,7 +69,8 @@ export async function encryptProfileData(
     aboutEmoji: bytesAboutEmoji ? Bytes.toBase64(bytesAboutEmoji) : null,
     badgeIds: (badges || []).map(({ id }) => id),
     paymentAddress: window.storage.get('paymentAddress') || null,
-    avatar: Boolean(avatarBuffer),
+    avatar: Boolean(newAvatar),
+    sameAvatar,
     commitment: deriveProfileKeyCommitment(profileKey, uuid),
   };
 

ts/util/lint/exceptions.json

@@ -7226,13 +7226,6 @@
     "reasonCategory": "falseMatch",
     "updated": "2021-05-05T23:11:22.692Z"
   },
-  {
-    "rule": "React-useRef",
-    "path": "ts/components/AvatarPreview.tsx",
-    "line": "  const startingAvatarPathRef = useRef<undefined | string>(",
-    "reasonCategory": "usageTrusted",
-    "updated": "2021-08-03T21:17:38.615Z"
-  },
   {
     "rule": "React-useRef",
     "path": "ts/components/AvatarTextEditor.tsx",
@@ -8079,4 +8072,4 @@
     "reasonCategory": "usageTrusted",
     "updated": "2021-09-17T21:02:59.414Z"
   }
-]
+]