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

Make mentionRegex stricter #1

Merged
merged 2 commits into from
Aug 11, 2023
Merged

Make mentionRegex stricter #1

merged 2 commits into from
Aug 11, 2023

Conversation

HoodieRocks
Copy link
Contributor

Description

This pull request addresses an issue where the existing mentionRegex was being matched in unintended contexts, such as emails and URLs containing the "@" symbol. The proposed changes make the mentionRegex stricter to prevent false positives and improve the accuracy of mentions in the remark-mentions plugin.

Changes Made

  • Modified the mentionRegex to make it stricter and prevent unintended matches in emails and URLs containing "@" symbols.
  • Added a new test case to validate the correctness of the updated mentionRegex implementation.

Checklist

  • Updated mentionRegex to be stricter and more accurate.
  • Added a new test case to cover the changes.
  • Verified that all existing tests pass with the updated regex.
  • Ran the test suite locally to ensure compatibility.

This pull request aims to enhance the accuracy of the remark-mentions plugin and improve its behaviour when dealing with mentions in various contexts. Please review and provide feedback.

Thank you!

@FinnRG
Copy link
Owner

FinnRG commented Aug 10, 2023

Thank you for your PR. Lookbehind assertions have only recently gained support in safari, so could you maybe replace (?<!\S) with just \s or would this introduce other edge cases?

@HoodieRocks
Copy link
Contributor Author

\s can capture a leading space, which messes up parsing, resulting in something like:

source:   'This is @test'
expected: 'This is [**@test**](/test)\n'
actual:   'This is[** @test**](/test)\n'

Any mention that's at the beginning of a line won't be picked up, but that can be fixed with ($:^|\s).

Do you have a suggestion for trimming the space?

@FinnRG
Copy link
Owner

FinnRG commented Aug 11, 2023

@HoodieRocks Good point, what if we use (?:^|\s) and separate the leading white space in the replaceMention function like this (just a rough draft):

  function replaceMention(value, username) {
    let whitespace = [];
    // Separate leading white space
    if (value.indexOf("@") > 0) {
      whitespace.push({
        type: "text",
        value: value.substring(0, value.indexOf("@")),
      });
    }

    return [
      ...whitespace,
      {
        type: "link",
        url: opts.usernameLink(username),
        children: [
          { type: "strong", children: [{ type: "text", value: value.trim() }] }, // Trim the username here
        ],
      },
    ];
  }

@HoodieRocks
Copy link
Contributor Author

That code worked perfectly. The only difference I made was adding the type.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (0ff4c57) 100.00% compared to head (442a571) 100.00%.
Report is 3 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #1   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           48        66   +18     
=========================================
+ Hits            48        66   +18     
Files Changed Coverage Δ
lib/index.js 100.00% <100.00%> (ø)

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

@FinnRG
Copy link
Owner

FinnRG commented Aug 11, 2023

Thank you for your PR. I'll publish version 1.1 shortly.

@FinnRG FinnRG merged commit 3fbe703 into FinnRG:main Aug 11, 2023
2 checks passed
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.

3 participants