Optimize profile avatar uploads and sync urls

Co-authored-by: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com>
signalapp · Mar 16, 2022 · 09d9cd4 · 09d9cd4
1 parent 36bb201
commit 09d9cd4
Show file tree

Hide file tree

Showing 15 changed files with 146 additions and 76 deletions.
diff --git a/ts/components/AvatarPreview.tsx b/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,23 +56,20 @@ export const AvatarPreview = ({
       return;
     }
 
-    const startingAvatarPath = startingAvatarPathRef.current;
-    if (!startingAvatarPath) {
+    if (!avatarPath) {
       return noop;
     }
 
     let shouldCancel = false;
 
     (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(() => {

diff --git a/ts/components/ProfileEditor.stories.tsx b/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,

diff --git a/ts/components/ProfileEditor.tsx b/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,17 +417,22 @@ 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;
 
   if (editState === EditState.BetterAvatar) {
     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}

diff --git a/ts/components/ProfileEditorModal.tsx b/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;

diff --git a/ts/models/conversations.ts b/ts/models/conversations.ts
@@ -1812,6 +1812,7 @@ export class ConversationModel extends window.Backbone
       avatarPath: this.getAbsoluteAvatarPath(),
       avatarHash: this.getAvatarHash(),
       unblurredAvatarPath: this.getAbsoluteUnblurredAvatarPath(),
+      profileAvatarPath: this.getAbsoluteProfileAvatarPath(),
       color,
       conversationColor: this.getConversationColor(),
       customColor,
@@ -4947,6 +4948,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

diff --git a/ts/services/storageRecordOps.ts b/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(

diff --git a/ts/services/writeProfile.ts b/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,45 +51,57 @@ 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 },
       };
     }
+
+    await window.storage.put('avatarUrl', avatarUrl);
   } else if (avatarPath) {
-    await window.Signal.Migrations.deleteAttachmentData(avatarPath);
-  }
+    log.info('writeProfile: removing avatar');
+    await Promise.all([
+      window.Signal.Migrations.deleteAttachmentData(avatarPath),
+      window.storage.put('avatarUrl', undefined),
+    ]);
 
-  const profileAvatarData = profileAvatar ? { profileAvatar } : {};
+    maybeProfileAvatarUpdate = { profileAvatar: undefined };
+  }
 
   // Update backbone, update DB, run storage service upload
   model.set({
     about: aboutText,
     aboutEmoji,
     profileName: firstName,
     profileFamilyName: familyName,
-    ...profileAvatarData,
+    ...maybeProfileAvatarUpdate,
   });
 
   dataInterface.updateConversation(model.attributes);