Use most recent username or display name when rendering mentions

Author: acelaya

src/sidebar/components/Annotation/AnnotationBody.tsx

@@ -6,6 +6,7 @@ import type { Annotation } from '../../../types/api';
 import type { SidebarSettings } from '../../../types/config';
 import { isThirdPartyUser } from '../../helpers/account-id';
 import { isHidden } from '../../helpers/annotation-metadata';
+import type { MentionMode } from '../../helpers/mentions';
 import { applyTheme } from '../../helpers/theme';
 import { withServices } from '../../service-context';
 import { useSidebarStore } from '../../store';
@@ -80,10 +81,13 @@ function AnnotationBody({ annotation, settings }: AnnotationBodyProps) {
 
   const textStyle = applyTheme(['annotationFontFamily'], settings);
 
-  const shouldLinkTags = useMemo(
-    () => annotation && !isThirdPartyUser(annotation?.user, defaultAuthority),
+  const authorIsThirdParty = useMemo(
+    () => isThirdPartyUser(annotation.user, defaultAuthority),
     [annotation, defaultAuthority],
   );
+  const mentionMode: MentionMode = authorIsThirdParty
+    ? 'display-name'
+    : 'username';
 
   const createTagSearchURL = (tag: string) => {
     return store.getLink('search.tag', { tag });
@@ -108,6 +112,7 @@ function AnnotationBody({ annotation, settings }: AnnotationBodyProps) {
             style={textStyle}
             mentions={annotation.mentions}
             mentionsEnabled={mentionsEnabled}
+            mentionMode={mentionMode}
           />
         </Excerpt>
       )}
@@ -122,7 +127,9 @@ function AnnotationBody({ annotation, settings }: AnnotationBodyProps) {
                       key={tag}
                       tag={tag}
                       href={
-                        shouldLinkTags ? createTagSearchURL(tag) : undefined
+                        !authorIsThirdParty
+                          ? createTagSearchURL(tag)
+                          : undefined
                       }
                     />
                   );

src/sidebar/components/MarkdownEditor.tsx

@@ -634,6 +634,7 @@ export default function MarkdownEditor({
           style={textStyle}
           mentions={mentions}
           mentionsEnabled={mentionsEnabled}
+          mentionMode={mentionMode}
         />
       ) : (
         <TextArea

src/sidebar/components/MarkdownView.tsx

@@ -10,7 +10,7 @@ import {
 } from 'preact/hooks';
 
 import type { Mention } from '../../types/api';
-import type { InvalidUsername } from '../helpers/mentions';
+import type { InvalidMentionContent, MentionMode } from '../helpers/mentions';
 import { processAndReplaceMentionElements } from '../helpers/mentions';
 import { replaceLinksWithEmbeds } from '../media-embedder';
 import { renderMathAndMarkdown } from '../render-markdown';
@@ -55,6 +55,7 @@ export type MarkdownViewProps = {
   classes?: string;
   style?: Record<string, string>;
   mentions?: Mention[];
+  mentionMode: MentionMode;
 
   /**
    * Whether the at-mentions feature ir enabled or not.
@@ -67,7 +68,7 @@ export type MarkdownViewProps = {
   clearTimeout_?: typeof clearTimeout;
 };
 
-type PopoverContent = Mention | InvalidUsername | null;
+type PopoverContent = Mention | InvalidMentionContent | null;
 
 /**
  * A component which renders markdown as HTML, replaces recognized links with
@@ -81,6 +82,7 @@ export default function MarkdownView(props: MarkdownViewProps) {
     style,
     mentions = [],
     mentionsEnabled = false,
+    mentionMode,
     setTimeout_ = setTimeout,
     clearTimeout_ = clearTimeout,
   } = props;
@@ -94,7 +96,7 @@ export default function MarkdownView(props: MarkdownViewProps) {
   const mentionsPopoverRef = useRef<HTMLElement>(null);
 
   const elementToMentionMap = useRef(
-    new Map<HTMLElement, Mention | InvalidUsername>(),
+    new Map<HTMLElement, Mention | InvalidMentionContent>(),
   );
   const [popoverContent, setPopoverContent] = useState<PopoverContent>(null);
   const popoverContentTimeout = useRef<ReturnType<typeof setTimeout> | null>();
@@ -145,8 +147,9 @@ export default function MarkdownView(props: MarkdownViewProps) {
     elementToMentionMap.current = processAndReplaceMentionElements(
       content.current!,
       mentions,
+      mentionMode,
     );
-  }, [mentions]);
+  }, [mentionMode, mentions]);
 
   // Monitor mouse position when mentions popover is visible and hide when it
   // goes outside the anchor or popover.

src/sidebar/components/mentions/MentionPopoverContent.tsx

@@ -1,10 +1,10 @@
 import { formatDateTime } from '@hypothesis/frontend-shared';
 
 import type { Mention } from '../../../types/api';
-import type { InvalidUsername } from '../../helpers/mentions';
+import type { InvalidMentionContent } from '../../helpers/mentions';
 
 export type MentionPopoverContent = {
-  content: Mention | InvalidUsername;
+  content: Mention | InvalidMentionContent;
 };
 
 /**

src/sidebar/components/test/MarkdownView-test.js

@@ -18,6 +18,7 @@ describe('MarkdownView', () => {
     return mount(
       <MarkdownView
         markdown=""
+        mentionMode="username"
         {...props}
         setTimeout_={callback => setTimeout(callback, 0)}
         clearTimeout_={fakeClearTimeout}
@@ -98,13 +99,17 @@ describe('MarkdownView', () => {
     });
   });
 
-  [undefined, [{}]].forEach(mentions => {
+  [
+    { mentions: undefined, mentionMode: 'username' },
+    { mentions: [{}], mentionMode: 'display-name' },
+  ].forEach(({ mentions, mentionMode }) => {
     it('renders mention tags based on provided mentions', () => {
-      createComponent({ mentions });
+      createComponent({ mentions, mentionMode });
       assert.calledWith(
         fakeProcessAndReplaceMentionElements,
         sinon.match.any,
         mentions ?? [],
+        mentionMode,
       );
     });
   });

src/sidebar/helpers/mentions.ts

@@ -77,9 +77,6 @@ export function wrapDisplayNameMentions(
     DISPLAY_NAME_MENTIONS_PAT,
     (match, precedingChar, displayName) => {
       const suggestion = usersMap.get(displayName);
-
-      // TODO Should we still build a mention tag so that it renders as an
-      //      invalid mention instead of plain text?
       if (!suggestion) {
         return `${precedingChar}${displayNameMention(displayName)}`;
       }
@@ -150,26 +147,27 @@ export function unwrapMentions({
 }
 
 /**
- * A username for a mention that the backend "discarded" as invalid. Possible
- * reasons are: the user does not exist, belongs to a different group, is
- * nipsa'd, etc.
+ * The content of a mention tag that the backend "discarded" as invalid.
+ * Possible reasons are: the user does not exist, belongs to a different group,
+ * is nipsa'd, etc.
  */
-export type InvalidUsername = string;
+export type InvalidMentionContent = string;
 
 /**
  * Replace an unprocessed mention tag with another element that represents the
  * proper type of mention ('link', 'no-link' or 'invalid').
  */
 function processAndReplaceMention(
   unprocessedMention: HTMLElement,
-  mention?: Mention,
-): [HTMLElement, Mention | InvalidUsername] {
-  const username = unprocessedMention.textContent ?? '';
-  const mentionOrUsername = mention ?? username;
+  mention: Mention | undefined,
+  mentionMode: MentionMode,
+): [HTMLElement, Mention | InvalidMentionContent] {
+  const originalTagContent = unprocessedMention.textContent ?? '';
+  const mentionOrInvalidContent = mention ?? originalTagContent;
 
   // If this mention element has already been processed, return as is
   if (unprocessedMention.hasAttribute('data-hyp-mention-type')) {
-    return [unprocessedMention, mentionOrUsername];
+    return [unprocessedMention, mentionOrInvalidContent];
   }
 
   const type =
@@ -180,7 +178,20 @@ function processAndReplaceMention(
 
   processedMention.setAttribute('data-hyp-mention', '');
   processedMention.setAttribute('data-hyp-mention-type', type);
-  processedMention.textContent = username;
+
+  // For valid mentions, resolve the most recent username or display name, in
+  // case it has changed since the mention was created.
+  // The only exception is when a valid mention with an empty display name is
+  // resolved, in which case we fall back to the original tag content.
+  if (!mention) {
+    processedMention.textContent = originalTagContent;
+  } else if (mentionMode === 'username') {
+    processedMention.textContent = `@${mention.username}`;
+  } else {
+    processedMention.textContent = mention.display_name
+      ? `@${mention.display_name}`
+      : originalTagContent;
+  }
 
   if (type === 'link') {
     // If the mention exists in the list of mentions and contains a link, render
@@ -197,7 +208,7 @@ function processAndReplaceMention(
   }
 
   unprocessedMention.replaceWith(processedMention);
-  return [processedMention, mentionOrUsername];
+  return [processedMention, mentionOrInvalidContent];
 }
 
 /**
@@ -212,7 +223,8 @@ function processAndReplaceMention(
 export function processAndReplaceMentionElements(
   element: HTMLElement,
   mentions: Mention[],
-): Map<HTMLElement, Mention | InvalidUsername> {
+  mentionMode: MentionMode,
+): Map<HTMLElement, Mention | InvalidMentionContent> {
   const mentionElements = element.querySelectorAll('[data-hyp-mention]');
   const foundMentions = new Map<HTMLElement, Mention | string>();
 
@@ -223,7 +235,9 @@ export function processAndReplaceMentionElements(
       ? mentions.find(m => m.userid === mentionUserId)
       : undefined;
 
-    foundMentions.set(...processAndReplaceMention(htmlMentionElement, mention));
+    foundMentions.set(
+      ...processAndReplaceMention(htmlMentionElement, mention, mentionMode),
+    );
   }
 
   return foundMentions;

src/sidebar/helpers/test/mentions-test.js

@@ -321,7 +321,11 @@ describe('processAndReplaceMentionElements', () => {
       <p>Mention without ID: <a data-hyp-mention="">@user_id_missing</a></p>
     `;
 
-    const result = processAndReplaceMentionElements(container, mentions);
+    const result = processAndReplaceMentionElements(
+      container,
+      mentions,
+      'username',
+    );
     assert.equal(result.size, 4);
 
     const [
@@ -391,7 +395,11 @@ describe('processAndReplaceMentionElements', () => {
       invalidProcessedMention,
     );
 
-    const result = processAndReplaceMentionElements(container, mentions);
+    const result = processAndReplaceMentionElements(
+      container,
+      mentions,
+      'username',
+    );
     assert.equal(result.size, 3);
 
     const [
@@ -409,6 +417,50 @@ describe('processAndReplaceMentionElements', () => {
     assert.equal(thirdElement, invalidProcessedMention);
     assert.equal(thirdMention, '@invalid');
   });
+
+  [
+    {
+      mentionMode: 'username',
+      oldContent: '@janedoe',
+      mention: { username: 'janedoe_updated' },
+      expectedContent: '@janedoe_updated',
+    },
+    {
+      mentionMode: 'display-name',
+      oldContent: '@Jane Doe',
+      mention: { display_name: 'Jane Doe Updated' },
+      expectedContent: '@Jane Doe Updated',
+    },
+    {
+      mentionMode: 'display-name',
+      oldContent: '@Jane Doe',
+      mention: { display_name: '' },
+      expectedContent: '@Jane Doe',
+    },
+  ].forEach(({ mentionMode, oldContent, mention, expectedContent }) => {
+    it('returns most recent usernames or display names', () => {
+      const mentions = [
+        {
+          userid: 'acct:[email protected]',
+          ...mention,
+        },
+      ];
+      const container = document.createElement('div');
+      container.innerHTML = mentionElement({
+        username: 'janedoe',
+        content: oldContent,
+      }).outerHTML;
+
+      const result = processAndReplaceMentionElements(
+        container,
+        mentions,
+        mentionMode,
+      );
+      const [[mentionEl]] = [...result.entries()];
+
+      assert.equal(mentionEl.textContent, expectedContent);
+    });
+  });
 });
 
 // To make these tests more predictable, we place the `$` sign in the position