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 TwitterText regex for valid domains in mentions to support more domains #25890

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mhlz
Copy link

@mhlz mhlz commented Jul 10, 2023

TwitterText already has a regex to check for valid domains that allows a few domains the current Regex did not allow. Mainly it allows mentions of accounts on domains with emoji in their domain names to be processed correctly.

Mentions like @test@🌈🌈🌈.st will now work correctly.

This would fix #20422 (and maybe other special characters in domains as well?) and thus solve the biggest hurdle for having a mastodon instance on domains with emojis.

@renchap
Copy link
Sponsor Member

renchap commented Jul 10, 2023

Could you add a test for this?

@mhlz
Copy link
Author

mhlz commented Jul 12, 2023

I added a test for my specific case. Are there any other tests you'd like me to add? :)

@renchap renchap added the to backport PR needed to be backported label Jul 12, 2023
@renchap renchap requested a review from a team July 12, 2023 16:40
@renchap renchap added this to the 4.2.0 milestone Jul 21, 2023
@ClearlyClaire
Copy link
Contributor

I'm not sure whether this is a good idea, emoji are not in general properly supported in domain names, and I'm not sure this is worth the additional complexity and possible edge cases.

There is also an aspect of mention processing that happens client-side, for auto-completion, unexpected mentions check, and counting characters. This PR only addresses the server-side aspect, which would lead to inconsistent character count when mentioning @test@🌈🌈🌈.st for instance

@mhlz
Copy link
Author

mhlz commented Jul 26, 2023

Auto completion on the client side already inserts @test@🌈🌈🌈.st correctly, which is why this issue came up in the first place (the server will currently treat this as a mention of the local user @test)

I'd be happy to make the necessary client side changes as well, if you could point me to the right places.

And as far as emojis in domain names in general go, every browser I tested supports them just fine (sometimes converting to punycode (Firefox), sometimes not (Safari)).

@ClearlyClaire
Copy link
Contributor

Auto completion on the client side already inserts @test@🌈🌈🌈.st correctly, which is why this issue came up in the first place (the server will currently treat this as a mention of the local user @test)

Ah, I missed this bit of context.

I'd be happy to make the necessary client side changes as well, if you could point me to the right places.

This currently happens through a regexp in app/javascript/mastodon/features/compose/util/counter.js

@mhlz
Copy link
Author

mhlz commented Sep 3, 2023

@ClearlyClaire I just pushed the changes to app/javascript/mastodon/features/compose/util/counter.js. I basically did the same thing I did in the ruby backend: Replace the domain part of the mention with twitter-text's valid domain regex and it looks like the compose form is now counting correctly and mentions work correctly :)

Let me know if there's anything else!

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@renchap renchap modified the milestones: 4.2.0, 4.3.0 Oct 31, 2023
@mhlz
Copy link
Author

mhlz commented Nov 9, 2023

I fixed the linting issues. Still want to get this merged, please let me know if there's anything else I should do :)

@mhlz
Copy link
Author

mhlz commented Jan 23, 2024

@renchap @ClearlyClaire I still would like to get this merged. I updated the fork again, is there anything else you need to me to change?

TwitterText already has a regex to check for valid domains that allows a
few domains the current Regex did not allow. Mainly it allows mentions
of accounts on domains with emoji in their domain names to be processed
correctly.

Mentions like `@test@🌈🌈🌈.st` will now work correctly.
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (61a0ec6) 85.08% compared to head (cfe038b) 85.08%.

❗ Current head cfe038b differs from pull request most recent head 3930f4a. Consider uploading reports for the commit 3930f4a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25890   +/-   ##
=======================================
  Coverage   85.08%   85.08%           
=======================================
  Files        1038     1038           
  Lines       28159    28159           
  Branches     4533     4533           
=======================================
  Hits        23959    23959           
  Misses       3039     3039           
  Partials     1161     1161           

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to backport PR needed to be backported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emojis in domain names break mentions
3 participants