Wrap display-name-based mentions in mention tags #6892

acelaya · 2025-03-14T16:14:15Z

Part of #6886

This PR ensures that display-name-based mentions can be converted to mention tags while saving the annotation, and from mention tags back to plain text when entering edit mode.

display-name-mentions-2025-03-18_10.33.44.mp4

There are a few considerations/topics to discuss:

Only one mention mode at a time is currently supported. Either all mentions are username-based or all are display-name-based. We can try to consolidate this in future if we think it makes sense.
We are not checking what kind of mention mode is "in place" when saving annotations outside of the annotation editor, like during auto save or when importing annotations.
The later is in fact tricky to handle, as one could export annotations from a public context (where username-based mentions were used), and the import them in a display-name-mention context.
I'll address this issue separately.
In order to wrap display-name mentions in mention tags, we rely on a map of tracked applied suggestions.
If an "unknown" mention is found (for example, one that was manually typed), we need to decide what to do. This PR simply keeps that as plain text as written, but we may try to handle it as an invalid mention instead.

Not covered by this PR:

Display-name mentions should not show a popover on hover. That is not addressed here.

codecov · 2025-03-14T16:18:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.43%. Comparing base (06d8974) to head (dd60dfe).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6892   +/-   ##
=======================================
  Coverage   99.42%   99.43%           
=======================================
  Files         273      273           
  Lines       10523    10562   +39     
  Branches     2529     2539   +10     
=======================================
+ Hits        10463    10502   +39     
  Misses         60       60

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

acelaya · 2025-03-18T09:23:47Z

src/sidebar/helpers/mentions.ts

- * special tag, as long as they are surrounded by "empty" space (space, tab, new
- * line, or beginning/end of the whole text).
+ * Wrap all occurrences of @mention in provided text into the corresponding
+ * special tag, as long as they are surrounded by boundary chars.
 *
 * For example: `@someuser` with the `hypothes.is` authority would become
 *  `<a data-hyp-mention data-userid="acct:[email protected]">@someuser</a>`
 */
 export function wrapMentions(text: string, authority: string): string {


Maybe this should be renamed to wrapUsernameMentions for consistency with the new wrapDisplayNameMentions

acelaya · 2025-03-18T09:27:17Z

src/sidebar/services/annotations.ts

-  async save(annotation: Annotation) {
+  async save(
+    annotation: Annotation,
+    mentionsOptions: MentionsOptions = { mentionMode: 'username' },


This param should be mandatory, as some cases where annotations are saved are currently not taking into consideration the mention mode:

Autosave.

Annotations import.

I'll address those separately.

OK. Worth adding a TODO here about this?

acelaya · 2025-03-18T09:45:33Z

src/sidebar/components/Annotation/AnnotationEditor.tsx

+  const usersDisplayNamesMapRef = useRef<Map<string, UserItem>>(
+    new Map(
+      mentionMode === 'username'
+        ? []
+        : // If the annotation is being edited, it may have mentions. Use them to
+          // initialize the display names map
+          annotation.mentions
+            ?.filter(mention => !!mention.display_name)
+            .map(({ userid, username, display_name: displayName }) => [
+              displayName!,
+              { userid, username, displayName },
+            ]),
+    ),
+  );


I'm not 100% sure if a ref is the right way to track the map of display names. If the annotation changed while the editor is open, it could cause the old mentions to still be incorrectly tracked, but as far as I can tell, that's something that cannot happen, as every annotation has its own editor component instance.

I didn't use a memoized value because the map starts with an initial list of students, but then it needs to be editable.

I didn't use a regular piece of state (useState) because the component doesn't need to re-render when the map changes. Also, trying to address the issue I explained above with a piece of state would require updating it in a side effect, which is tricky to do right and usually discouraged.

Actually, I'm thinking a possible solution would be to not initialize this here based on annotation.mentions. Instead, use it to track inserted suggestions only, and at the moment of saving the annotation, then "merging" that with annotation.mentions. That way the map will match the annotation at the moment of saving.

When you begin editing an annotation, a new draft is created with tags and text initialized from the values at the point when editing starts. If the annotation were updated on another system while the draft was being edited, what we probably ought to do is inform the user of the conflict when they try to save. This has never been an issue in practice so we haven't implemented it. The mention text <-> user info map is initialized here at the same time as the draft, which I think is the correct thing to do.

robertknight

This generally looks good and works as expected. I noted a few issues that can be handled in follow up work (handling display names containing square brackets, handling display name changes to a mentioned user when editing an existing annotation) and requested a few comments and small changes in the tests.

src/sidebar/components/Annotation/AnnotationEditor.tsx

robertknight · 2025-03-18T10:57:10Z

src/sidebar/components/Annotation/AnnotationEditor.tsx

+      mentionMode === 'username'
+        ? []
+        : // If the annotation is being edited, it may have mentions. Use them to
+          // initialize the display names map


What will happen when editing an existing annotation with a mention, where the user has changed their display name since the mention was originally created? To handle that I think we'd need to use the current display name when unwrapping the mention, rather than whatever the mention text was originally. I think that can be left out of this PR, but should be tracked somewhere.

Yeah, good point. In fact, I think this also applies to username-based mentions. If their username has changed, we should unwrap as the most recent username, otherwise when wrapping back it will generate an invalid userid.

I created this issue to track it #6906

robertknight · 2025-03-18T11:01:15Z

src/sidebar/components/Annotation/AnnotationEditor.tsx

      toastMessenger.success(successMessage, { visuallyHidden: true });
+      usersDisplayNamesMapRef.current = new Map();


You could also use the map's clear method. This would require that none of the functions that the map has been passed to ever modify the map.

src/sidebar/components/Annotation/test/AnnotationEditor-test.js

robertknight · 2025-03-18T11:22:49Z

src/sidebar/helpers/mentions.ts

+// Pattern that matches display names.
+// Display names can have any amount of characters, except opening or closing
+// square brackets, which are used to delimit the display name itself.
+const DISPLAY_NAME_PAT = String.raw`[^\]^[]*`;


Display names in Hypothesis can contain square brackets, although we expect this to be rare. When inserting mentions and unwrapping text we should do something sensible, such as stripping these characters from the inserted text. This can be handled in a follow-up PR as long as we're tracking it somewhere.

I see this is tracked in #6891.

robertknight · 2025-03-18T11:25:03Z

src/sidebar/helpers/mentions.ts

+    }
+
+    const [atChar, ...rest] = mention;
+    return `${atChar}[${rest.join('')}]`;


As noted above, we'll need to strip out or otherwise handle square brackets in the display name (rest) here.

src/sidebar/services/annotations.ts

robertknight · 2025-03-18T11:30:18Z

src/sidebar/services/annotations.ts

+        : mentionsOptions.mentionMode === 'display-name'
+          ? (text: string) =>
+              wrapDisplayNameMentions(text, mentionsOptions.usersMap)
+          : (text: string) => wrapMentions(text, authority);


This is OK, although I find that ternaries in JS can get difficult to follow when nested.

Yeah, I tend to agree. Let me see if I can find a better way to express this.

robertknight · 2025-03-18T11:30:44Z

src/sidebar/services/annotations.ts

-  async save(annotation: Annotation) {
+  async save(
+    annotation: Annotation,
+    mentionsOptions: MentionsOptions = { mentionMode: 'username' },


OK. Worth adding a TODO here about this?

acelaya force-pushed the wrap-display-name-mentions branch 8 times, most recently from 3433676 to c33e390 Compare March 18, 2025 09:19

acelaya commented Mar 18, 2025

View reviewed changes

acelaya force-pushed the wrap-display-name-mentions branch from c33e390 to 299df1b Compare March 18, 2025 09:40

acelaya marked this pull request as ready for review March 18, 2025 09:41

acelaya commented Mar 18, 2025

View reviewed changes

acelaya requested a review from robertknight March 18, 2025 09:45

acelaya force-pushed the wrap-display-name-mentions branch from 299df1b to 9e08dbb Compare March 18, 2025 09:50

robertknight approved these changes Mar 18, 2025

View reviewed changes

Wrap display-name-based mentions in mention tags

dd60dfe

acelaya force-pushed the wrap-display-name-mentions branch from 9e08dbb to dd60dfe Compare March 18, 2025 13:46

acelaya merged commit b047586 into main Mar 18, 2025
4 checks passed

acelaya deleted the wrap-display-name-mentions branch March 18, 2025 13:50

acelaya mentioned this pull request Mar 18, 2025

Refactor AnnotationsService._applyDraftChanges to remove condition nesting #6907

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap display-name-based mentions in mention tags #6892

Wrap display-name-based mentions in mention tags #6892

acelaya commented Mar 14, 2025 •

edited

Loading

codecov bot commented Mar 14, 2025 •

edited

Loading

acelaya Mar 18, 2025

acelaya Mar 18, 2025

robertknight Mar 18, 2025

acelaya Mar 18, 2025 •

edited

Loading

acelaya Mar 18, 2025

robertknight Mar 18, 2025

robertknight left a comment

robertknight Mar 18, 2025

acelaya Mar 18, 2025

acelaya Mar 18, 2025

robertknight Mar 18, 2025

robertknight Mar 18, 2025

robertknight Mar 18, 2025

robertknight Mar 18, 2025

robertknight Mar 18, 2025

acelaya Mar 18, 2025

robertknight Mar 18, 2025

		toastMessenger.success(successMessage, { visuallyHidden: true });
		usersDisplayNamesMapRef.current = new Map();

Wrap display-name-based mentions in mention tags #6892

Wrap display-name-based mentions in mention tags #6892

Conversation

acelaya commented Mar 14, 2025 • edited Loading

codecov bot commented Mar 14, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Mar 14, 2025 •

edited

Loading

codecov bot commented Mar 14, 2025 •

edited

Loading

acelaya Mar 18, 2025 •

edited

Loading