Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use most recent username or display name when unwrapping mention tags #6908

Merged
merged 1 commit into from
Mar 20, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/sidebar/components/MarkdownEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -585,8 +585,8 @@ export default function MarkdownEditor({
const input = useRef<HTMLTextAreaElement>(null);

const textWithoutMentionTags = useMemo(
() => unwrapMentions(text, mentionMode),
[mentionMode, text],
() => unwrapMentions({ text, mentionMode, mentions }),
[mentionMode, mentions, text],
);

useEffect(() => {
Expand Down
45 changes: 37 additions & 8 deletions src/sidebar/helpers/mentions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,27 +96,56 @@ export function wrapDisplayNameMentions(
// some assumptions about the tag content in order to be able to get away
// with it. Specifically we assume that these tags will not contain `<` or
// `>` in attribute values or the HTML content of the tag.
const MENTION_TAG_RE = /<a[^>]\bdata-hyp-mention\b[^>]*>([^<]+)<\/a>/g;
const MENTION_TAG_RE = /<a[^>]\bdata-hyp-mention\b[^>]*>@([^<]+)<\/a>/g;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm updating this to expect the tag content to start with an @ char. This has some pros and cons:

The main pro is that the logic to get the value without the @ char is now simpler, as we can rely on the captured value verbatim.

The cons are:

  1. We no longer match mention tags if they do not contain the @ char. I don't know if we'll ever need/want to support this.
  2. We don't match mention tags if their content starts with @ but it's not trimmed (contains empty characters between the opening tag and its content).

I reckon these cons are not really a problem, because the unwrapMentions function was already assuming the first character was the @ character, but there might be other side effects I missed here.


export type UnwrapMentionsOptions = {
text: string;
mentionMode: MentionMode;
mentions?: Mention[];
};

/**
* Replace all mentions wrapped in the special `<a data-hyp-mention />` tag with
* their plain-text representation.
*
* The plain-text representation depends on the mention mode:
* - `username`: @username
* - `display-name`: @[Display Name]
*
* If a list of mentions is provided, the tag's userid will be matched against
* it, so that the plain-text version uses the most recent username or display
* name, in case they have changed since the mention was created.
* If a list is not provided or the mention is not found, the tag's content will
* be used as username or display name.
*/
export function unwrapMentions(text: string, mentionMode: MentionMode) {
export function unwrapMentions({
text,
mentionMode,
mentions = [],
}: UnwrapMentionsOptions) {
// Use a regex rather than HTML parser to replace the mentions in order
// to avoid modifying any of the content outside of the replaced tags. This
// includes avoiding modifications such as encoding characters that will
// happen when parsing and re-serializing HTML via eg. `innerHTML`.
return text.replace(MENTION_TAG_RE, (match, mention) => {
if (mentionMode === 'username') {
return mention;
}
return text.replace(MENTION_TAG_RE, (match, tagContent) => {
// Even though we need to capture the tag content via a regex for the
// reasons explained above, we can use regular DOM APIs to get the userid
const tempElement = document.createElement('div');
tempElement.innerHTML = match;
const userid = tempElement
.querySelector('a[data-hyp-mention]')
?.getAttribute('data-userid');
Comment on lines +131 to +137
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to evolve the MENTION_TAG_RE regular expression, so that it would also capture the userid in one go, but I ended up in a rabbit whole of a) a regexp that would capture the userid if present but would not match if the attributes were in a different order, or b) a regexp that would match regardless of the order of the attributes, but would not capture the userid.

I ended up splitting it in two steps because:

  • The regular expression is simpler and more predictable when only relying in one attribute (data-hyp-mention).
  • Reading the data-userid attribute can be done with easier to understand and more reliable DOM APIs.

I could have used .dataset.userid instead of getAttribute('data-userid') but that requires casting the result of querySelector() to HTMLElement. I thought this was simpler.


const mention = mentions.find(
({ original_userid }) => original_userid === userid,
);

const [atChar, ...rest] = mention;
return `${atChar}[${rest.join('')}]`;
return mentionMode === 'username'
? `@${mention?.username ?? tagContent}`
: // Using || rather than ?? for the display name, to avoid setting an
// empty string if the user used to have a display name and has been
// removed since the mention was created
`@[${mention?.display_name || tagContent}]`;
});
}

Expand Down
88 changes: 86 additions & 2 deletions src/sidebar/helpers/test/mentions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,10 @@ Hello ${mentionTag('jane.doe', 'example.com')}.`,

describe('unwrapMentions - `username` mode', () => {
it('removes wrapping mention tags', () => {
assert.equal(unwrapMentions(textWithTags, 'username'), text);
assert.equal(
unwrapMentions({ text: textWithTags, mentionMode: 'username' }),
text,
);
});
});
});
Expand Down Expand Up @@ -211,7 +214,88 @@ Hello ${mentionTag('jane.doe', 'example.com')}.`,

describe('unwrapMentions - `display-name` mode', () => {
it('removes wrapping mention tags', () => {
assert.equal(unwrapMentions(textWithTags, 'display-name'), text);
assert.equal(
unwrapMentions({ text: textWithTags, mentionMode: 'display-name' }),
text,
);
});
});
});

describe('unwrapMentions', () => {
[
// Mention not found. Tag content kept
{
mentionMode: 'username',
mentions: [],
text: `Hello ${mentionTag('jane_doe')}`,
expectedResult: 'Hello @jane_doe',
},
{
mentionMode: 'display-name',
mentions: [],
text: `Hello ${displayNameMentionTag('Jane Doe', 'jane_doe')}`,
expectedResult: 'Hello @[Jane Doe]',
},

// Mention found with a new username
{
mentionMode: 'username',
mentions: [
{
original_userid: 'acct:[email protected]',
username: 'jane_edited',
},
],
text: `Hello ${mentionTag('jane_doe')}`,
expectedResult: 'Hello @jane_edited',
},

// Mention found with a new display name
{
mentionMode: 'display-name',
mentions: [
{
original_userid: 'acct:[email protected]',
display_name: 'My new name',
},
],
text: `Hello ${displayNameMentionTag('Jane Doe', 'jane_doe')}`,
expectedResult: 'Hello @[My new name]',
},

// Mention found with a now empty display name
{
mentionMode: 'display-name',
mentions: [
{
original_userid: 'acct:[email protected]',
display_name: '',
},
],
text: `Hello ${displayNameMentionTag('Jane Doe', 'jane_doe')}`,
expectedResult: 'Hello @[Jane Doe]',
},

// data-userid not present
{
mentionMode: 'username',
mentions: [],
text: 'Hello <a data-hyp-mention="">@user_id_missing</a>',
expectedResult: 'Hello @user_id_missing',
},
{
mentionMode: 'display-name',
mentions: [],
text: 'Hello <a data-hyp-mention="">@User ID Missing</a>',
expectedResult: 'Hello @[User ID Missing]',
},
].forEach(({ text, mentionMode, mentions, expectedResult }) => {
it('replaces mention tag with current username or display name', () => {
assert.equal(
unwrapMentions({ text, mentionMode, mentions }),
expectedResult,
);
});
});
});
Expand Down