[$250] Chat-On adding spade symbol, link is changed to text.

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.4
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Tap on a report
3. Enter govinda.com
4. Long tap message and select edit comment
5. Enter °•●●□■ along with link name and save it
6. Note link can be seen
7. Long tap message and select edit comment
8. Now include ♤ along with link name and save it
9. Note link is shown as text
10. Long tap message and select edit comment
11. Remove spade symbol and save it
12. Note now shown as link

Expected Result:

On adding spade symbol, link must not be changed to text.

Actual Result:

On adding spade symbol, link is changed to text.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6532762_1720075009162.az_recorder_20240704_083446_compress_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a1dba0f8a6c08731
• Upwork Job ID: 1808902546673127823
• Last Price Increase: 2024-07-04
• Automatic offers:
• rayane-djouah | Reviewer | 103069056

Issue Owner

Current Issue Owner: @rayane-djouah

[melvin-bot]

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb

[twisterdotcom]

Okay... this is super niche. I'm going to make it Monthly because this is super niche.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a1dba0f8a6c08731

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah (External)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Adding spade symbol to the link text doesn't convert it to link.

What is the root cause of that problem?

The spade symbol is an emoji, while the others are not emoji.

Based on the link markdown replacement logic, we don't allow emojis to be inside the link text.
https://github.com/Expensify/expensify-common/blob/2610f15e7fb8d98d19b3841146c693df7be0a5c9/lib/ExpensiMark.ts#L249-L256

It's to fix this issue where the link underline partly behind the emoji character which doesn't look good, so we disable emoji inside link text.

What changes do you think we should make in order to solve the problem?

Allow the emoji back to be inside the link text by removing the emoji check condition here.

This is how it looks when an emoji is inside the link text.

GitHub also allows emoji to be inside link text

[nkdengineer]

Proposal

Please re-state the problem that we are trying to solve in this issue.

On adding spade symbol, link is changed to text.

What is the root cause of that problem?

In https://github.com/Expensify/expensify-common/blob/2610f15e7fb8d98d19b3841146c693df7be0a5c9/lib/ExpensiMark.ts#L252, we don't match link text that has any emojis.

This was an oversight from this PR Expensify/expensify-common#536, we should not allow link text with only emojis because it will not look good (described in the issue description), but if emojis are with other texts, we should still allow it. That will be the behavior consistent with Slack.

What changes do you think we should make in order to solve the problem?

Put the method containsOnlyEmojis inside expensify-common (as a method of Str)

In https://github.com/Expensify/expensify-common/blob/2610f15e7fb8d98d19b3841146c693df7be0a5c9/lib/ExpensiMark.ts#L252, remove g1.match(Constants.CONST.REG_EXP.EMOJIS) and use containsOnlyEmojis to check if g1 contains only emojis.

What alternative solutions did you explore? (Optional)

In addition to that that check, we could check that length of g1 is 1, so if the link text contains only 1 emoji, it will not be matched as link text.

[gawandeabhishek]

Proposal

Please re-state the problem that we are trying to solve in this issue.

We are trying to resolve the issue where special characters, such as the spade symbol (♤), in comments are being displayed as plain text instead of clickable links on Expensify's staging site.

What is the root cause of that problem?

The root cause of the problem is that the special characters are not being properly encoded and sanitized, leading to issues in rendering them correctly in HTML comments. This results in the characters being displayed as plain text instead of clickable links.

What changes do you think we should make in order to solve the problem?

To solve this issue, we utilized the he, dompurify, and jsdom libraries. These libraries help ensure that special characters are correctly encoded and sanitized before being rendered. Here is the updated code that solves the issue:

In RenderCommentHTML.tsx:

const sanitizedHtml = sanitizeHTML(html);
const decodedHtml = he.decode(sanitizedHtml);

const commentHtml = source === 'email' ? `<email-comment>${decodedHtml}</email-comment>` : `<comment>${decodedHtml}</comment>`;

return <RenderHTML html={commentHtml} />;

In ReportActionItemMessageEdit.tsx:

const [draft, setDraft] = useState(() => {
    if (draftMessage) {
        emojisPresentBefore.current = EmojiUtils.extractEmojis(draftMessage);
    }
    return he.decode(draftMessage);
});

const updateDraft = useCallback(
    (newDraftInput: string) => {
        const { text: newDraft, emojis, cursorPosition } = EmojiUtils.replaceAndExtractEmojis(newDraftInput, preferredSkinTone, preferredLocale);

        if (emojis?.length > 0) {
            const newEmojis = EmojiUtils.getAddedEmojis(emojis, emojisPresentBefore.current);
            if (newEmojis?.length > 0) {
                insertedEmojis.current = [...insertedEmojis.current, ...newEmojis];
                debouncedUpdateFrequentlyUsedEmojis();
            }
        }
        emojisPresentBefore.current = emojis;
        const safeDraft = sanitizeHTML(he.encode(newDraft));
        setDraft(he.decode(safeDraft));
    }
);

In sanitizeHTML.ts:

import { JSDOM } from 'jsdom';
import DOMPurify from 'dompurify';
import he from 'he';

// Initialize DOMPurify with JSDOM
const window = new JSDOM('').window;
const purify = DOMPurify(window);

export default function sanitizeHTML(input: string): string {
    // Encode HTML entities
    const encodedHTML = he.encode(input);

    // Sanitize the encoded HTML
    const sanitizedHTML = purify.sanitize(encodedHTML, {
        USE_PROFILES: { html: true }
    });

    // Decode HTML entities
    const decodedHTML = he.decode(sanitizedHTML);

    return decodedHTML;
}

Why we used this solution:

• he library: This library is used to encode and decode HTML entities. It ensures that special characters are properly converted to their HTML entity equivalents and back, preventing issues with character rendering.
• dompurify library: This library is used to sanitize HTML content. It removes any potentially malicious content and ensures that the HTML is safe to be rendered.
• jsdom library: This library provides a JavaScript implementation of the DOM, enabling server-side manipulation of HTML content. It helps in parsing and sanitizing the HTML content correctly.

By using these libraries, we ensure that special characters are correctly handled and rendered, fixing the issue of them being displayed as plain text instead of clickable links.

What alternative solutions did you explore? (Optional)

Alternative solutions considered included manually encoding and sanitizing the HTML content. However, this approach was prone to errors and did not provide the comprehensive security and reliability offered by the he, dompurify, and jsdom libraries.

Video Demonstration

To visually illustrate the problem and the solution implemented, please watch the following screencast. In this video, we demonstrate how special characters in comments on Expensify's staging site were displaying incorrectly and how we used the he, dompurify, and jsdom libraries to encode, decode, and sanitize HTML content effectively.

Recording.2024-07-07.163622.mp4

[gawandeabhishek]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/~01d738f432141b6ec7?viewMode=1

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[gawandeabhishek]

Proposal

Updated

[rayane-djouah]

Reviewing proposals

[rayane-djouah]

@bernhardoj's proposal LGTM

It seems that the "Emoji anchor text with hyperlink doesn't look right" bug is old and has been fixed. Therefore, we don't need the condition that excludes text with emojis from being the anchor text for links or emails. Here is the result when removing the condition:

Screen.Recording.2024-07-09.at.11.43.47.PM.mov

We need to revert Expensify/expensify-common#536 and Expensify/expensify-common#583 in expensify-common and update the version in App

🎀👀🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[nkdengineer]

@rayane-djouah I think it still doesn't look good which is why we fixed it earlier in #44835 (comment)

Actual Result:
Emoji only is accepted as an anchor text and it doesn't look pleasing to the eye

Also it's inconsistent with Slack, in Slack, single-emoji will not be accepted as anchor text.

As you can see in the screenshot, emoji-only (the one above) in Slack will not be accepted as anchor text. Meanwhile emoji-with-text (the one below) will be accepted. My proposal will make it consistent with Slack.

Looking forward to hear what @francoisl thinks what should be expected here

[francoisl]

The original argument that it doesn't look good when there is an emoji in a link is subjective, and so different people will naturally have different opinions on whether they should be accepted or not.

I believe we try to stay as close as possible to "standard" markdown when possible for our parser, and from a quick look at this RFC, which points to this site for the syntax, I don't see anything mentioned about restricting specific characters or emojis in links.

Let's go with @bernhardoj's proposal 👍

[melvin-bot]

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[bernhardoj]

expensi-common PR is here

cc: @rayane-djouah

[bernhardoj]

App PR is ready

cc: @rayane-djouah

[rayane-djouah]

@francoisl - a fix for this issue has already landed in the app via #45556 that updated the version of expensify-common to 2.0.49 which includes our changes, and it's already on staging.

We need to directly QA test this on that PR. How can we inform Applause QA team about this?

QA Steps:

1. Send a link with emoji as text ([😄 link](google.com)) to any user
2. Verify 😄 link is rendered as a link

I've tested that it's working well on staging: #45310 (comment)

[francoisl]

Hm in that case I think we can just close this and issue payments, what do you think @twisterdotcom?

[twisterdotcom]

We can close yes. Given the work done, I'm okay with still paying out, even though we'll need to just do away with it - but are you @bernhardoj and @rayane-djouah okay with just $125 given this isn't code we'll end up using and it didn't require lots of testing?

[bernhardoj]

I think we are still eligible for full payment. The App PR is just updating the expensify-common library version. Most of the work done is in the expensify-common repo.

#45556 updates the expensify-common version to a higher one which includes our fix.

[rayane-djouah]

I agree with @bernhardoj, Most of the work was done in the expensify-common repo PR: Expensify/expensify-common#749
The second PR #45310 was just for updating the expensify-common version in the app and isn't a code we'll end up not using.

I think #45556 updated the version without following a proper process for issues tracking, and this wasn't the only issue affected, see for example: #37357 (comment) and #44032 (comment)

[twisterdotcom]

Ah okay great, if this is the central issue for payments for all of them, then cool. I agree. Let's go with the full amount of $250 each. Payment Summary coming.

[twisterdotcom]

Payment Summary:

• @bernhardoj paid $250 via newDot Requests (Contributor)
• @rayane-djouah paid $250 via Upwork here (C+)

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj