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.

Sign up for GitHub

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

Jump to bottom

fix: no trimming for link name #773

Merged
merged 4 commits into from
Aug 9, 2024
Merged

fix: no trimming for link name #773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
265d496
fix: no trimming for link name
dominictb Aug 5, 2024
303cd94
fix: prettier
dominictb Aug 5, 2024
ec546ab
fix: only keep spaces if shouldKeepRawInput=true
dominictb Aug 5, 2024
a6109cb
fix: add extra line
dominictb Aug 5, 2024
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
6 changes: 6 additions & 0 deletions __tests__/ExpensiMark-HTML-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,12 @@ test('Test markdown and url links with inconsistent starting and closing parens'
expect(parser.replace(testString)).toBe(resultString);
});

test('Test link: Keep spaces at both end for shouldKeepRawInput=true', () => {
const testString = '[ link ](https://www.expensify.com)';
const resultString = '<a href=\"https://www.expensify.com\" data-raw-href=\"https://www.expensify.com\" data-link-variant=\"labeled\" target=\"_blank\" rel=\"noreferrer noopener\"> link </a>';
expect(parser.replace(testString, {shouldKeepRawInput: true})).toBe(resultString);
});

test('Test autolink replacement to avoid parsing nested links', () => {
const testString =
'[click google.com *here*](google.com) ' +
Expand Down
4 changes: 2 additions & 2 deletions lib/ExpensiMark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ export default class ExpensiMark {
if (!g1.trim()) {
return match;
}
return `<a href="${Str.sanitizeURL(g2)}" data-raw-href="${g2}" data-link-variant="labeled" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;
return `<a href="${Str.sanitizeURL(g2)}" data-raw-href="${g2}" data-link-variant="labeled" target="_blank" rel="noreferrer noopener">${g1}</a>`;
},
},

Expand Down Expand Up @@ -298,7 +298,7 @@ export default class ExpensiMark {
{
name: 'reportMentions',

regex: /(?<![^ \n*~_])(#[\p{Ll}0-9-]{1,80})(?![^<]*(?:<\/pre>|<\/code>))/gimu,
regex: /(?<![^ \n*~_])(#[\p{Ll}0-9-]{1,80})(?![^<]*(?:<\/pre>|<\/code>|<\/a>))/gimu,
Copy link
Contributor

Choose a reason for hiding this comment

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

@dominictb Could you explain why we need to change this regex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expensify-common/__tests__/ExpensiMark-HTML-test.js

Line 2271 in 4f2e0bc

test('room mention with space inside link should not be rendered', () => {
if we don't change the regexp, this test will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dominictb In this case, shouldKeepRawInput is false, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I don't think this test relate to our change

Copy link
Contributor Author

@dominictb dominictb Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we keep the old regex, then in markdown input we'll see:

image

Based on what I saw in the unit test here, the report mention markdown syntax should not be applied if it is inside a link. So this is not expected right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I'm under the assumption that before this change, the #room text shouldn't have the report-mention markdown if it is inside a link markdown syntax (see the image above)

However, after my change, the #room text will now have the report-mention markdown style (check the image below), but I believe this only happens in the markdown input, not when we save/send the message as we have already trim the extra space at that point, hence the #room text won't have the report-mention markdown style. So, the question is: is this acceptable to show the report-mention markdown style in the markdown input in this case or not? It might be confusing for the user because what they saw when typing and when viewing the sent message is 2 different thing.

image

Copy link
Contributor

@DylanDylann DylanDylann Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dominictb Yeah, agree that the report-mention markdown style shouldn't be rendered. But my curious why we have different behaviors when trimming spaces. Could you refer to a code link for this point?

but I believe this only happens in the markdown input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But my curious why we have different behaviors when trimming spaces

that is because we're trying to match and apply report-mention rule to the link name, and since the original regex of the report-mention doesn't account for the case where link name can have spaces on both end -> hence the mentioned problem

The solution is to update the regex, making sure that if the mention-report is between <a/> tag, we won't render the markdown style. We have the same logic to prevent applying mention-report markdown style in pre and code block, so it should be straightforward

but I believe this only happens in the markdown input

We only keep the space in when shouldKeepRawInput = true, and this particular problem only happens when the link name have redundant spaces on either end. So, it only happens in the markdown input (shouldKeepRawInput = true), but not when we display the sent message in the app (``shouldKeepRawInput = false`)

Copy link
Contributor

@DylanDylann DylanDylann Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I understand your problem and solution. But the RCA isn't clear to me

But my curious why we have different behaviors when trimming spaces. Could you refer to a code link for this point?

For this question, I am curious why the old mention-report regex works well when we trim spaces but It fails when we keep trailing spaces

Screenshot 2024-08-06 at 19 59 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, see. It dues to the negative lookbehind. Sorry for my confusion

replacement: '<mention-report>$1</mention-report>',
},

Expand Down
Loading