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

Fix boundaries during mention parsing #1625

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Nov 21, 2024

Description

I believe the intention behind what should be parsed as a mention is anything that starts with an @ and only contains valid username characters (+ profile path) and is delimited by whitespace. However, the current code does not do that but allows characters before @ and after the username:

// current mention regexp on client
/@([\w_]+(?:\/[\w_]+)?)/gi

// current mention regexp on server
/\B@[\w_]+/gi

This results in the following—IMO broken—highlighting on the client:

2024-11-21-012019_601x75_scrot

I also think client and server might not parse the same stuff in some cases. This means that a mention that the client would show as a mention wouldn't be considered a mention on the server so the stacker wouldn't actually get a notification.

This PR intends to fix that. Ideally, we would be able to simply use \b to match word boundarise, but since @ isn't considered a word character and characters like : are also considered a boundary, this doesn't work. So I decided to use positive lookbehind (?<=\s|^) and positive lookahead (?=\s|$) to fix the highlighting:

// new mention regexp on client and server
/(?<=\s|^)@([\w_]+(?:\/[\w_]+)?)(?=\s|$)/gi

This results in the following highlighting on the client:

2024-11-21-012030_594x73_scrot

Since only updating the regexp is not enough but this also requires testing of:

  1. mentions with and w/o profile path
  2. user popovers
  3. territory mentions (since the client combines both regular expressions)
  4. parsing on server (server now uses the same regexp but the handling is different)

This turned out to be quite a rabbit hole, so I’m leaving this PR as a source of future inspiration and as a reminder, that there is something wrong with mentions IMO.

and because we don't have enough draft PRs already

Additional Context

Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?

Checklist

Are your changes backwards compatible? Please answer below:

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

Did you introduce any new environment variables? If so, call them out explicitly here:

@ekzyis ekzyis added the bug label Nov 21, 2024
@ekzyis ekzyis marked this pull request as draft November 21, 2024 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant