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

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Mar 19, 2025

Closes #6906

So far, when unwrapping an a[data-hyp-mention] tag into its plain-text representation, we were always relying on the hardcoded content of the tag.

This PR ensures the tag is matched against a list of Mention objects, so that we unwrap to the most recent username or display name instead, ensuring the mention will continue working after wrapping it back, and it will also display the value users expect.

This PR does not cover doing the same when rendering a mention tag. There's a separate issue for that #6909

Todo

  • Add tests that cover the logic when username or display name have a different value in the mentions list.

Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.43%. Comparing base (e9d189e) to head (3b8b249).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6908   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         273      273           
  Lines       10569    10574    +5     
  Branches     2543     2545    +2     
=======================================
+ Hits        10509    10514    +5     
  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.

Comment on lines +131 to +137
// 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');
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.

@acelaya acelaya force-pushed the up-to-date-unwrapped-mentions branch from 7e9408d to e47a881 Compare March 19, 2025 10:55
@@ -96,27 +96,53 @@ 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.

@acelaya acelaya force-pushed the up-to-date-unwrapped-mentions branch from e47a881 to 9c2ea67 Compare March 19, 2025 11:01
@acelaya acelaya marked this pull request as ready for review March 19, 2025 11:04
@acelaya acelaya requested a review from robertknight March 19, 2025 11:04
Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

LGTM with requests for a few added comments and a revised test description.

@acelaya acelaya force-pushed the up-to-date-unwrapped-mentions branch from 9c2ea67 to 3b8b249 Compare March 20, 2025 09:03
@acelaya acelaya merged commit 61173a4 into main Mar 20, 2025
4 checks passed
@acelaya acelaya deleted the up-to-date-unwrapped-mentions branch March 20, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use most recent username or display name when unwrapping mention tags
2 participants