Improve UI performance for no-op profile fetches

ts/models/conversations.ts

@@ -93,6 +93,10 @@ const COLORS = [
 const THREE_HOURS = 3 * 60 * 60 * 1000;
 const FIVE_MINUTES = 1000 * 60 * 5;
 
+const ATTRIBUTES_THAT_DONT_INVALIDATE_PROPS_CACHE = new Set([
+  'profileLastFetchedAt',
+]);
+
 type CustomError = Error & {
   identifier?: string;
   number?: string;
@@ -289,6 +293,13 @@ export class ConversationModel extends window.Backbone
     // We clear our cached props whenever we change so that the next call to format() will
     //   result in refresh via a getProps() call. See format() below.
     this.on('change', () => {
+      const isPropsCacheStillValid = Object.keys(this.changed).every(key =>
+        ATTRIBUTES_THAT_DONT_INVALIDATE_PROPS_CACHE.has(key)
+      );
+      if (isPropsCacheStillValid) {
+        return;
+      }
+
       if (this.cachedProps) {
         this.oldCachedProps = this.cachedProps;
       }
@@ -1342,6 +1353,8 @@ export class ConversationModel extends window.Backbone
   //   maintains a cache, and protects against reentrant calls.
   // Note: When writing code inside this function, do not call .format() on a conversation
   //   unless you are sure that it's not this very same conversation.
+  // Note: If you start relying on an attribute that is in
+  //   `ATTRIBUTES_THAT_DONT_INVALIDATE_PROPS_CACHE`, remove it from that list.
   private getProps(): ConversationType {
     // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
     const color = this.getColor()!;

ts/routineProfileRefresh.ts

@@ -1,13 +1,17 @@
 // Copyright 2021 Signal Messenger, LLC
 // SPDX-License-Identifier: AGPL-3.0-only
 
+// We use `for ... of` to deal with iterables in several places in this file.
+/* eslint-disable no-restricted-syntax */
+
 import { isNil, sortBy } from 'lodash';
+import PQueue from 'p-queue';
 
 import * as log from './logging/log';
 import { assert } from './util/assert';
 import { missingCaseError } from './util/missingCaseError';
 import { isNormalNumber } from './util/isNormalNumber';
-import { map, take } from './util/iterables';
+import { take } from './util/iterables';
 import { isOlderThan } from './util/timestamp';
 import { ConversationModel } from './models/conversations';
 
@@ -52,23 +56,37 @@ export async function routineProfileRefresh({
 
   let totalCount = 0;
   let successCount = 0;
-  await Promise.all(
-    map(conversationsToRefresh, async (conversation: ConversationModel) => {
-      totalCount += 1;
-      try {
-        await conversation.getProfile(
-          conversation.get('uuid'),
-          conversation.get('e164')
-        );
-        successCount += 1;
-      } catch (err) {
-        window.log.error(
-          'routineProfileRefresh: failed to fetch a profile',
-          err?.stack || err
-        );
-      }
-    })
-  );
+
+  async function refreshConversation(
+    conversation: ConversationModel
+  ): Promise<void> {
+    window.log.info(
+      `routineProfileRefresh: refreshing profile for ${conversation.idForLogging()}`
+    );
+
+    totalCount += 1;
+    try {
+      await conversation.getProfile(
+        conversation.get('uuid'),
+        conversation.get('e164')
+      );
+      window.log.info(
+        `routineProfileRefresh: refreshed profile for ${conversation.idForLogging()}`
+      );
+      successCount += 1;
+    } catch (err) {
+      window.log.error(
+        `routineProfileRefresh: refreshed profile for ${conversation.idForLogging()}`,
+        err?.stack || err
+      );
+    }
+  }
+
+  const refreshQueue = new PQueue({ concurrency: 5, timeout: 1000 * 60 * 2 });
+  for (const conversation of conversationsToRefresh) {
+    refreshQueue.add(() => refreshConversation(conversation));
+  }
+  await refreshQueue.onIdle();
 
   log.info(
     `routineProfileRefresh: successfully refreshed ${successCount} out of ${totalCount} conversation(s)`
@@ -112,9 +130,6 @@ function* getFilteredConversations(
 
   const conversationIdsSeen = new Set<string>([ourConversationId]);
 
-  // We use a `for` loop (instead of something like `forEach`) because we want to be able
-  //   to yield. We use `for ... of` for readability.
-  // eslint-disable-next-line no-restricted-syntax
   for (const conversation of sorted) {
     const type = conversation.get('type');
     switch (type) {
@@ -129,7 +144,6 @@ function* getFilteredConversations(
         }
         break;
       case 'group':
-        // eslint-disable-next-line no-restricted-syntax
         for (const member of conversation.getMembers()) {
           if (
             !conversationIdsSeen.has(member.id) &&

ts/state/ducks/conversations.ts

@@ -1410,8 +1410,9 @@ export function reducer(
     let { showArchived } = state;
 
     const existing = conversationLookup[id];
-    // In the change case we only modify the lookup if we already had that conversation
-    if (!existing) {
+    // We only modify the lookup if we already had that conversation and the conversation
+    //   changed.
+    if (!existing || data === existing) {
       return state;
     }