Fix aci value in mentions

ts/quill/memberRepository.ts

@@ -5,9 +5,37 @@ import Fuse from 'fuse.js';
 import { get } from 'lodash';
 
 import type { ConversationType } from '../state/ducks/conversations';
-import type { ServiceIdString } from '../types/ServiceId';
+import type { AciString } from '../types/ServiceId';
+import { isAciString } from '../types/ServiceId';
 import { filter, map } from '../util/iterables';
 import { removeDiacritics } from '../util/removeDiacritics';
+import { isNotNil } from '../util/isNotNil';
+
+export type MemberType = Omit<ConversationType, 'serviceId'> &
+  Readonly<{
+    aci: AciString;
+  }>;
+
+function toMember({
+  serviceId,
+  ...restOfConvo
+}: ConversationType): MemberType | undefined {
+  if (!isAciString(serviceId)) {
+    return undefined;
+  }
+
+  return {
+    ...restOfConvo,
+    aci: serviceId,
+  };
+}
+
+// Exported for testing
+export function _toMembers(
+  conversations: ReadonlyArray<ConversationType>
+): Array<MemberType> {
+  return conversations.map(toMember).filter(isNotNil);
+}
 
 const FUSE_OPTIONS = {
   location: 0,
@@ -17,7 +45,7 @@ const FUSE_OPTIONS = {
   minMatchCharLength: 1,
   keys: ['name', 'firstName', 'profileName', 'title'],
   getFn(
-    conversation: Readonly<ConversationType>,
+    conversation: Readonly<MemberType>,
     path: string | Array<string>
   ): ReadonlyArray<string> | string {
     // It'd be nice to avoid this cast, but Fuse's types don't allow it.
@@ -40,21 +68,21 @@ const FUSE_OPTIONS = {
 };
 
 export class MemberRepository {
+  private members: ReadonlyArray<MemberType>;
   private isFuseReady = false;
 
-  private fuse: Fuse<ConversationType> = new Fuse<ConversationType>(
+  private fuse = new Fuse<MemberType>([], FUSE_OPTIONS);
-    [],
-    FUSE_OPTIONS
-  );
 
-  constructor(private members: ReadonlyArray<ConversationType> = []) {}
+  constructor(conversations: ReadonlyArray<ConversationType> = []) {
+    this.members = _toMembers(conversations);
+  }
 
-  updateMembers(members: ReadonlyArray<ConversationType>): void {
+  updateMembers(conversations: ReadonlyArray<ConversationType>): void {
-    this.members = members;
+    this.members = _toMembers(conversations);
     this.isFuseReady = false;
   }
 
-  getMembers(omit?: ConversationType): ReadonlyArray<ConversationType> {
+  getMembers(omit?: Pick<MemberType, 'id'>): ReadonlyArray<MemberType> {
     if (omit) {
       return this.members.filter(({ id }) => id !== omit.id);
     }
@@ -62,26 +90,22 @@ export class MemberRepository {
     return this.members;
   }
 
-  getMemberById(id?: string): ConversationType | undefined {
+  getMemberById(id?: string): MemberType | undefined {
     return id
       ? this.members.find(({ id: memberId }) => memberId === id)
       : undefined;
   }
 
-  getMemberByServiceId(
+  getMemberByAci(aci?: AciString): MemberType | undefined {
-    serviceId?: ServiceIdString
+    return aci
-  ): ConversationType | undefined {
+      ? this.members.find(({ aci: memberAci }) => memberAci === aci)
-    return serviceId
-      ? this.members.find(
-          ({ serviceId: memberServiceId }) => memberServiceId === serviceId
-        )
       : undefined;
   }
 
   search(
     pattern: string,
-    omit?: ConversationType
+    omit?: Pick<MemberType, 'id'>
-  ): ReadonlyArray<ConversationType> {
+  ): ReadonlyArray<MemberType> {
     if (!this.isFuseReady) {
       this.fuse.setCollection(this.members);
       this.isFuseReady = true;

ts/quill/mentions/completion.tsx

@@ -13,9 +13,10 @@ import { createPortal } from 'react-dom';
 import type { ConversationType } from '../../state/ducks/conversations';
 import { Avatar, AvatarSize } from '../../components/Avatar';
 import type { LocalizerType, ThemeType } from '../../types/Util';
-import type { MemberRepository } from '../memberRepository';
+import type { MemberType, MemberRepository } from '../memberRepository';
 import type { PreferredBadgeSelectorType } from '../../state/selectors/badges';
 import { matchBlotTextPartitions } from '../util';
+import type { MentionBlotValue } from '../util';
 import { handleOutsideClick } from '../../util/handleOutsideClick';
 import { sameWidthModifier } from '../../util/popperUtil';
 import { UserText } from '../../components/UserText';
@@ -32,7 +33,7 @@ export type MentionCompletionOptions = {
 const MENTION_REGEX = /(?:^|\W)@([-+\p{L}\p{M}\p{N}]*)$/u;
 
 export class MentionCompletion {
-  results: ReadonlyArray<ConversationType>;
+  results: ReadonlyArray<MemberType>;
 
   index: number;
 
@@ -106,7 +107,7 @@ export class MentionCompletion {
     this.clearResults();
   }
 
-  possiblyShowMemberResults(): ReadonlyArray<ConversationType> {
+  possiblyShowMemberResults(): ReadonlyArray<MemberType> {
     const range = this.quill.getSelection();
 
     if (range) {
@@ -121,7 +122,7 @@ export class MentionCompletion {
       if (leftTokenTextMatch) {
         const [, leftTokenText] = leftTokenTextMatch;
 
-        let results: ReadonlyArray<ConversationType> = [];
+        let results: ReadonlyArray<MemberType> = [];
 
         const memberRepository = this.options.memberRepositoryRef.current;
 
@@ -194,7 +195,7 @@ export class MentionCompletion {
   }
 
   insertMention(
-    mention: ConversationType,
+    member: MemberType,
     index: number,
     range: number,
     withTrailingSpace = false
@@ -202,6 +203,11 @@ export class MentionCompletion {
     // The mention + space we add won't be formatted unless we manually provide attributes
     const attributes = this.getAttributesForInsert(range - 1);
 
+    const mention: MentionBlotValue = {
+      aci: member.aci,
+      title: member.title,
+    };
+
     const delta = new Delta()
       .retain(index)
       .delete(range)
@@ -261,7 +267,7 @@ export class MentionCompletion {
               {memberResults.map((member, index) => (
                 <button
                   type="button"
-                  key={member.serviceId}
+                  key={member.aci}
                   id={`mention-result--${member.name}`}
                   role="option button"
                   aria-selected={memberResultsIndex === index}

ts/quill/mentions/matchers.ts

@@ -21,11 +21,10 @@ export const matchMention: (
 
       if (node.classList.contains('MessageBody__at-mention')) {
         const { id } = node.dataset;
-        const conversation = memberRepository.getMemberById(id);
+        const member = memberRepository.getMemberById(id);
 
-        if (conversation && conversation.serviceId) {
+        if (member && member.aci) {
-          const { serviceId: aci } = conversation;
+          const { aci } = member;
-          assertDev(isAciString(aci), 'Mentioned conversation has no ACI');
           return new Delta().insert(
             {
               mention: {
@@ -43,17 +42,14 @@ export const matchMention: (
       if (node.classList.contains('mention-blot')) {
         const { aci } = node.dataset;
         assertDev(isAciString(aci), 'Mentioned blot has invalid ACI');
-        const conversation = memberRepository.getMemberByServiceId(aci);
+        const member = memberRepository.getMemberByAci(aci);
 
-        if (conversation && conversation.serviceId) {
+        if (member && member.aci) {
-          assertDev(
+          assertDev(member.aci === aci, 'Mentioned member has no ACI');
-            conversation.serviceId === aci,
-            'Mentioned conversation has no ACI'
-          );
           return new Delta().insert(
             {
               mention: {
-                title: title || conversation.title,
+                title: title || member.title,
                 aci,
               },
             },

ts/test-electron/quill/mentions/completion_test.tsx

@@ -11,7 +11,8 @@ import type { MutableRefObject } from 'react';
 import type { MentionCompletionOptions } from '../../../quill/mentions/completion';
 import { MentionCompletion } from '../../../quill/mentions/completion';
 import type { ConversationType } from '../../../state/ducks/conversations';
-import { MemberRepository } from '../../../quill/memberRepository';
+import { MemberRepository, _toMembers } from '../../../quill/memberRepository';
+import type { MemberType } from '../../../quill/memberRepository';
 import { ThemeType } from '../../../types/Util';
 import { getDefaultConversationWithServiceId } from '../../../test-both/helpers/getDefaultConversation';
 import { setupI18n } from '../../../util/setupI18n';
@@ -28,7 +29,7 @@ const me: ConversationType = getDefaultConversationWithServiceId({
   isMe: true,
 });
 
-const members: Array<ConversationType> = [
+const conversations: Array<ConversationType> = [
   getDefaultConversationWithServiceId({
     id: '555444',
     title: 'Mahershala Ali',
@@ -62,6 +63,8 @@ const members: Array<ConversationType> = [
   me,
 ];
 
+const members = _toMembers(conversations);
+
 describe('MentionCompletion', () => {
   let mockQuill: Omit<
     Partial<{ [K in keyof Quill]: SinonStub }>,
@@ -73,7 +76,7 @@ describe('MentionCompletion', () => {
 
   beforeEach(function beforeEach() {
     const memberRepositoryRef: MutableRefObject<MemberRepository> = {
-      current: new MemberRepository(members),
+      current: new MemberRepository(conversations),
     };
 
     const options: MentionCompletionOptions = {
@@ -106,7 +109,7 @@ describe('MentionCompletion', () => {
   describe('onTextChange', () => {
     let possiblyShowMemberResultsStub: sinon.SinonStub<
       [],
-      ReadonlyArray<ConversationType>
+      ReadonlyArray<MemberType>
     >;
 
     beforeEach(() => {
@@ -159,7 +162,7 @@ describe('MentionCompletion', () => {
   describe('completeMention', () => {
     describe('given a completable mention', () => {
       let insertMentionStub: SinonStub<
-        [ConversationType, number, number, (boolean | undefined)?],
+        [MemberType, number, number, (boolean | undefined)?],
         void
       >;
 

ts/test-node/quill/memberRepository_test.ts

@@ -3,9 +3,9 @@
 
 import { assert } from 'chai';
 
-import { generateAci } from '../../types/ServiceId';
+import { generateAci, isAciString } from '../../types/ServiceId';
 import type { ConversationType } from '../../state/ducks/conversations';
-import { MemberRepository } from '../../quill/memberRepository';
+import { MemberRepository, _toMembers } from '../../quill/memberRepository';
 import { getDefaultConversationWithServiceId } from '../../test-both/helpers/getDefaultConversation';
 
 const UNKNOWN_SERVICE_ID = generateAci();
@@ -34,7 +34,7 @@ const memberShia: ConversationType = getDefaultConversationWithServiceId({
   areWeAdmin: false,
 });
 
-const members: Array<ConversationType> = [memberMahershala, memberShia];
+const conversations: Array<ConversationType> = [memberMahershala, memberShia];
 
 const singleMember: ConversationType = getDefaultConversationWithServiceId({
   id: '666777',
@@ -51,91 +51,97 @@ const singleMember: ConversationType = getDefaultConversationWithServiceId({
 describe('MemberRepository', () => {
   describe('#updateMembers', () => {
     it('updates with given members', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
-      assert.deepEqual(memberRepository.getMembers(), members);
+      assert.deepEqual(
+        memberRepository.getMembers(),
+        _toMembers(conversations)
+      );
 
-      const updatedMembers = [...members, singleMember];
+      const updatedConversations = [...conversations, singleMember];
-      memberRepository.updateMembers(updatedMembers);
+      memberRepository.updateMembers(updatedConversations);
-      assert.deepEqual(memberRepository.getMembers(), updatedMembers);
+      assert.deepEqual(
+        memberRepository.getMembers(),
+        _toMembers(updatedConversations)
+      );
     });
   });
 
   describe('#getMemberById', () => {
     it('returns undefined when there is no search id', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
       assert.isUndefined(memberRepository.getMemberById());
     });
 
     it('returns a matched member', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
       assert.isDefined(memberRepository.getMemberById('555444'));
     });
 
     it('returns undefined when it does not have the member', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
       assert.isUndefined(memberRepository.getMemberById(UNKNOWN_SERVICE_ID));
     });
   });
 
-  describe('#getMemberByServiceId', () => {
+  describe('#getMemberByAci', () => {
     it('returns undefined when there is no search serviceId', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
-      assert.isUndefined(memberRepository.getMemberByServiceId());
+      assert.isUndefined(memberRepository.getMemberByAci());
     });
 
     it('returns a matched member', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
-      assert.isDefined(
+      const aci = memberMahershala.serviceId;
-        memberRepository.getMemberByServiceId(memberMahershala.serviceId)
+      if (!isAciString(aci)) {
-      );
+        throw new Error('Service id not ACI');
+      }
+      assert.isDefined(memberRepository.getMemberByAci(aci));
     });
 
     it('returns undefined when it does not have the member', () => {
-      const memberRepository = new MemberRepository(members);
+      const memberRepository = new MemberRepository(conversations);
-      assert.isUndefined(
+      assert.isUndefined(memberRepository.getMemberByAci(UNKNOWN_SERVICE_ID));
-        memberRepository.getMemberByServiceId(UNKNOWN_SERVICE_ID)
-      );
     });
   });
 
   describe('#search', () => {
     describe('given a prefix-matching string on last name', () => {
       it('returns the match', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('a');
-        assert.deepEqual(results, [memberMahershala]);
+        assert.deepEqual(results, _toMembers([memberMahershala]));
       });
     });
 
     describe('given a prefix-matching string on first name', () => {
       it('returns the match', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('ma');
-        assert.deepEqual(results, [memberMahershala]);
+        assert.deepEqual(results, _toMembers([memberMahershala]));
       });
     });
 
     describe('given a prefix-matching string on profile name', () => {
       it('returns the match', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('sr');
-        assert.deepEqual(results, [memberShia]);
+        assert.deepEqual(results, _toMembers([memberShia]));
       });
     });
 
     describe('given a prefix-matching string on name', () => {
       it('returns the match', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('dude');
-        assert.deepEqual(results, [memberShia]);
+        assert.deepEqual(results, _toMembers([memberShia]));
       });
     });
 
     describe('given a prefix-matching string on title', () => {
       it('returns the match', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('bud');
-        assert.deepEqual(results, [memberShia]);
+        assert.deepEqual(results, _toMembers([memberShia]));
       });
 
       it('handles titles with Unicode bidi characters, which some contacts have', () => {
@@ -148,13 +154,13 @@ describe('MemberRepository', () => {
           memberShiaBidi,
         ]);
         const results = memberRepository.search('bud');
-        assert.deepEqual(results, [memberShiaBidi]);
+        assert.deepEqual(results, _toMembers([memberShiaBidi]));
       });
     });
 
     describe('given a match in the middle of a name', () => {
       it('returns zero matches', () => {
-        const memberRepository = new MemberRepository(members);
+        const memberRepository = new MemberRepository(conversations);
         const results = memberRepository.search('e');
         assert.deepEqual(results, []);
       });