Hyperlink markdown doesn't work if the ] is preceded by an (

[isagoico]

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

---

Action Performed:

1. Navigate to a conversation
2. Send this message
   [Yo (click here to see a cool cat)](https://c8.alamy.com/compes/ha11pc/cookie-cat-con-sombrero-de-cowboy-y-sun-glass-ha11pc.jpg)
3. Check the message in the history

Expected Result:

The link should be hyperlinked even if there is a ) before the ].

Actual Result:

Link is not hyperlinked. Checked on Github and it gets hyperlinked correctly so this seems like an issue.
Important: Same happens for other punctuation marks.

Example
Yo (click here to see a cool cat)

Workaround:

User can send the link without the formatting

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.0.80-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1627271189249400

Markdown doesn't work when ] is preceded by )

[MelvinBot]

Triggered auto assignment to @AndrewGable (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[MelvinBot]

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[NicMendonca]

Posted job: https://www.upwork.com/jobs/~01eb625fbec00ecdbf

[mananjadhav]

Proposal:

This can be fixed by updating the regex in ExpensiMark.js of expensify-common. Can handle other characters too by updating the regex.

{
                name: "link",
                process: (textToProcess, replacement) => {
                    const regex = new RegExp(
                        `\\[((?:[\\w\\s\\d!?&#;:\\(\\)\\/\\-\\.\\+=<>,@\\[\\]‘’“”]|(?:<code>.+<\\/code>))+)\\]\\(${URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`,
                        "gi"
                    );
                    return this.modifyTextForUrlLinks(
                        regex,
                        textToProcess,
                        replacement
                    );
                },

                // We use a function here to check if there is already a https:// on the link.
                // If there is not, we force the link to be absolute by prepending '//' to the target.
                replacement: (match, g1, g2, g3) =>
                    `<a href="${
                        g3 && g3.includes("http") ? "" : "http://"
                    }${g2}" target="_blank">${g1}</a>`,
            },

[Santhosh-Sellavel]

Proposal

Changes should be done against expensify-common.

In the following lines.
https://github.com/Expensify/expensify-common/blob/7d8408c5c78792394eee8e079f115b1380221a23/lib/ExpensiMark.js#L72-L81

We missed handling for () also additionally we missed for$, %, *, | symbol,

Need replace regex prefix \\[((?:[\\w\\s\\d!?&#;:\\/\\-\\.\\+=<>,

with string to for () handling,
\[((?:[\w\s\d!?&#;:\/\-\.\+=<>(),

for other characters also.
\[((?:[\w\s\d!?&#;:\/\-\.\+=<>()$|%*,

[UHaider]

As mentioned in the issue it is happening for other punctuations as well, we can update the regex for link rule, this will handle {}*() along with existing supported punctuations

Here is the proposal

Proposal

                process: (textToProcess, replacement) => {
                    const regex = new RegExp(
                        `\\[((?:[\\w\\s\\d!?&#;:\\/\\-\\.\\+=<>,@\\[\\]\\(\\)\\'\\"\\*\\{\\}‘’“”]|(?:<code>.+<\\/code>))+)\\]\\(${URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`,
                        'gi'
                    );
                    return this.modifyTextForUrlLinks(regex, textToProcess, replacement);
                },

                // We use a function here to check if there is already a https:// on the link.
                // If there is not, we force the link to be absolute by prepending '//' to the target.
                replacement: (match, g1, g2, g3) => (
                    `<a href="${g3 && g3.includes('http') ? '' : 'http://'}${g2}" target="_blank">${g1}</a>`
                ),
            },

DEMO
https://user-images.githubusercontent.com/17588033/127191146-e832a99d-99fa-41be-9755-f627550ab954.mov

[parasharrajat]

I would suggest please run the test suite on e-common and mention that in your proposal if that is a pass.

[AndrewGable]

I agree @parasharrajat. @mananjadhav since you were the first to comment, can you verify that the tests pass with your proposed regex? If so, we will hire you for the solution. Thank you!

[mananjadhav]

@AndrewGable @parasharrajat Sure. I'll add a test in a while and update here with the result.

[mananjadhav]

@AndrewGable @parasharrajat I've raised a PR with the test cases and the regex change.

Changed the approach to ensure none of the existing test cases break and also support multiple other special characters.

[AndrewGable]

Looks great on first pass, I will review it further shortly. @NicMendonca - Can you please hire @mananjadhav via Upwork? Thanks!

[NicMendonca]

@mananjadhav hired!

[AndrewGable]

@mananjadhav Please open an App PR with the updated hash. Thanks!

[mananjadhav]

@AndrewGable Raised PR. Would you be needing screenshots for this PR? Tests in expensify-common cover the testing. Let me know.

[AndrewGable]

Yes please, screenshots are required for all PRs 👍

[mananjadhav]

@AndrewGable Screencasts added for all platforms. Let me know if anything else is required to merge this.

[mananjadhav]

@NicMendonca Any updates on Upwork for this?

[NicMendonca]

@mananjadhav yup! As per this information here, contracts are paid 7 days after the pull request is merged to allow for regression testing. Payment is scheduled to be issued in 2 days.