Refactor avatar fetching and updating logic

ts/models/conversations.preload.ts

@@ -50,7 +50,6 @@ import {
 import { getDraftPreview } from '../util/getDraftPreview.preload.js';
 import { hasDraft } from '../util/hasDraft.std.js';
 import { hydrateStoryContext } from '../util/hydrateStoryContext.preload.js';
-import * as Conversation from '../types/Conversation.node.js';
 import type {
   StickerType,
   StickerWithHydratedData,
@@ -5131,6 +5130,16 @@ export class ConversationModel {
       return;
     }
 
+    const existingProfileAvatar = this.get('profileAvatar');
+
+    if (
+      existingProfileAvatar?.path &&
+      (await doesAttachmentExist(existingProfileAvatar.path)) &&
+      existingProfileAvatar.url === avatarUrl
+    ) {
+      return;
+    }
+
     const avatar = await doGetAvatar(avatarUrl);
 
     // If decryptionKey isn't provided, use the one from the model
@@ -5149,18 +5158,16 @@ export class ConversationModel {
     // decrypt
     const decrypted = decryptProfile(avatar, updatedDecryptionKey);
 
-    // update the conversation avatar only if hash differs
-    if (decrypted) {
-      const newAttributes = await Conversation.maybeUpdateProfileAvatar(
-        this.attributes,
-        {
-          data: decrypted,
-          writeNewAttachmentData,
-          deleteAttachmentData: maybeDeleteAttachmentFile,
-          doesAttachmentExist,
-        }
-      );
-      this.set(newAttributes);
+    const newAttachment = await writeNewAttachmentData(decrypted);
+    this.set({
+      profileAvatar: {
+        url: avatarUrl,
+        ...newAttachment,
+      },
+    });
+
+    if (existingProfileAvatar?.path) {
+      await maybeDeleteAttachmentFile(existingProfileAvatar.path);
     }
   }
 

ts/services/contactSync.preload.ts

@@ -15,13 +15,11 @@ import {
   getAttachment,
   getAttachmentFromBackupTier,
 } from '../textsecure/WebAPI.preload.js';
-import * as Conversation from '../types/Conversation.node.js';
 import * as Errors from '../types/errors.std.js';
 import type { ValidateConversationType } from '../model-types.d.ts';
 import type { ConversationModel } from '../models/conversations.preload.js';
 import { validateConversation } from '../util/validateConversation.dom.js';
 import {
-  writeNewAttachmentData,
   maybeDeleteAttachmentFile,
   doesAttachmentExist,
 } from '../util/migrations.preload.js';
@@ -70,24 +68,19 @@ async function updateConversationFromContactSync(
   });
 
   // Update the conversation avatar only if new avatar exists and hash differs
-  const { avatar } = details;
-  if (avatar && avatar.path) {
-    const newAttributes = await Conversation.maybeUpdateAvatar(
-      conversation.attributes,
-      {
-        newAvatar: avatar,
-        writeNewAttachmentData,
-        deleteAttachmentData: maybeDeleteAttachmentFile,
-        doesAttachmentExist,
-      }
-    );
-    conversation.set(newAttributes);
-  } else {
-    const { attributes } = conversation;
-    if (attributes.avatar && attributes.avatar.path) {
-      await maybeDeleteAttachmentFile(attributes.avatar.path);
+  const { avatar: newAvatar } = details;
+  const oldAvatar = conversation.get('avatar');
+  const avatarHasChanged = newAvatar?.hash !== oldAvatar?.hash;
+  const oldAvatarExistsOnDisk =
+    oldAvatar?.path && (await doesAttachmentExist(oldAvatar.path));
+
+  if (avatarHasChanged || !oldAvatarExistsOnDisk) {
+    conversation.set({ avatar: newAvatar });
+    if (oldAvatar?.path) {
+      await maybeDeleteAttachmentFile(oldAvatar.path);
     }
-    conversation.set({ avatar: null });
+  } else if (newAvatar?.path) {
+    await maybeDeleteAttachmentFile(newAvatar.path);
   }
 
   if (isInitialSync) {

ts/sql/Client.preload.ts

@@ -13,7 +13,7 @@ import * as Bytes from '../Bytes.std.js';
 import { createLogger } from '../logging/log.std.js';
 import * as Errors from '../types/errors.std.js';
 
-import { deleteExternalFiles } from '../types/Conversation.node.js';
+import { deleteExternalFiles } from '../types/Conversation.std.js';
 import { createBatcher } from '../util/batcher.std.js';
 import { assertDev, softAssert } from '../util/assert.std.js';
 import { mapObjectWithSpec } from '../util/mapObjectWithSpec.std.js';
@@ -561,7 +561,7 @@ async function removeConversation(id: string): Promise<void> {
   if (existing) {
     await writableChannel.removeConversation(id);
     await deleteExternalFiles(existing, {
-      deleteAttachmentData: maybeDeleteAttachmentFile,
+      maybeDeleteAttachmentFile,
     });
   }
 }

ts/types/Conversation.node.ts

@@ -1,129 +0,0 @@
-// Copyright 2018 Signal Messenger, LLC
-// SPDX-License-Identifier: AGPL-3.0-only
-
-import type { ConversationAttributesType } from '../model-types.d.ts';
-import type { ContactAvatarType } from './Avatar.std.js';
-import type { LocalAttachmentV2Type } from './Attachment.std.js';
-import { computeHash } from '../Crypto.node.js';
-import { createLogger } from '../logging/log.std.js';
-
-const log = createLogger('Conversation');
-
-export type BuildAvatarUpdaterOptions = Readonly<{
-  data?: Uint8Array;
-  newAvatar?: ContactAvatarType;
-  deleteAttachmentData: (path: string) => Promise<{ wasDeleted: boolean }>;
-  doesAttachmentExist: (path: string) => Promise<boolean>;
-  writeNewAttachmentData: (data: Uint8Array) => Promise<LocalAttachmentV2Type>;
-}>;
-
-// This function is ready to handle raw avatar data as well as an avatar which has
-//   already been downloaded to disk.
-// Scenarios that go to disk today:
-//   - During a contact sync (see ContactsParser.ts)
-// Scenarios that stay in memory today:
-//   - models/Conversations/setProfileAvatar
-function buildAvatarUpdater({ field }: { field: 'avatar' | 'profileAvatar' }) {
-  return async (
-    conversation: Readonly<ConversationAttributesType>,
-    {
-      data,
-      newAvatar,
-      deleteAttachmentData,
-      doesAttachmentExist,
-      writeNewAttachmentData,
-    }: BuildAvatarUpdaterOptions
-  ): Promise<ConversationAttributesType> => {
-    if (!conversation || (!data && !newAvatar)) {
-      return conversation;
-    }
-
-    const oldAvatar = conversation[field];
-    const newHash = data ? computeHash(data) : undefined;
-
-    if (!oldAvatar || !oldAvatar.hash) {
-      if (newAvatar) {
-        return {
-          ...conversation,
-          [field]: newAvatar,
-        };
-      }
-      if (data) {
-        return {
-          ...conversation,
-          [field]: {
-            hash: newHash,
-            ...(await writeNewAttachmentData(data)),
-          },
-        };
-      }
-      throw new Error('buildAvatarUpdater: neither newAvatar or newData');
-    }
-
-    const { hash, path } = oldAvatar;
-    const exists = path && (await doesAttachmentExist(path));
-    if (!exists) {
-      log.warn(`buildAvatarUpdater: attachment ${path} did not exist`);
-    }
-
-    if (exists) {
-      if (newAvatar && hash && hash === newAvatar.hash) {
-        if (newAvatar.path) {
-          await deleteAttachmentData(newAvatar.path);
-        }
-        return conversation;
-      }
-      if (data && hash && hash === newHash) {
-        return conversation;
-      }
-    }
-
-    if (path) {
-      await deleteAttachmentData(path);
-    }
-
-    if (newAvatar) {
-      return {
-        ...conversation,
-        [field]: newAvatar,
-      };
-    }
-    if (data) {
-      return {
-        ...conversation,
-        [field]: {
-          hash: newHash,
-          ...(await writeNewAttachmentData(data)),
-        },
-      };
-    }
-
-    throw new Error('buildAvatarUpdater: neither newAvatar or newData');
-  };
-}
-
-export const maybeUpdateAvatar = buildAvatarUpdater({ field: 'avatar' });
-export const maybeUpdateProfileAvatar = buildAvatarUpdater({
-  field: 'profileAvatar',
-});
-
-export async function deleteExternalFiles(
-  conversation: ConversationAttributesType,
-  {
-    deleteAttachmentData,
-  }: Pick<BuildAvatarUpdaterOptions, 'deleteAttachmentData'>
-): Promise<void> {
-  if (!conversation) {
-    return;
-  }
-
-  const { avatar, profileAvatar } = conversation;
-
-  if (avatar && avatar.path) {
-    await deleteAttachmentData(avatar.path);
-  }
-
-  if (profileAvatar && profileAvatar.path) {
-    await deleteAttachmentData(profileAvatar.path);
-  }
-}

ts/types/Conversation.std.ts

@@ -0,0 +1,29 @@
+// Copyright 2018 Signal Messenger, LLC
+// SPDX-License-Identifier: AGPL-3.0-only
+
+import type { ConversationAttributesType } from '../model-types.d.ts';
+
+export async function deleteExternalFiles(
+  conversation: ConversationAttributesType,
+  {
+    maybeDeleteAttachmentFile,
+  }: {
+    maybeDeleteAttachmentFile: (
+      path: string
+    ) => Promise<{ wasDeleted: boolean }>;
+  }
+): Promise<void> {
+  if (!conversation) {
+    return;
+  }
+
+  const { avatar, profileAvatar } = conversation;
+
+  if (avatar && avatar.path) {
+    await maybeDeleteAttachmentFile(avatar.path);
+  }
+
+  if (profileAvatar && profileAvatar.path) {
+    await maybeDeleteAttachmentFile(profileAvatar.path);
+  }
+}