fix: allow multiline text in markdown links#895
fix: allow multiline text in markdown links#895mccoychang wants to merge 1 commit intoExpensify:mainfrom
Conversation
The MARKDOWN_LINK_REGEX excluded \r and \n from the link text character class, causing multiline markdown links like [line1\nline2](url) to not be rendered as clickable hyperlinks. This removes \r\n from the negated character classes so that newlines in link text are matched. The existing modifyTextForUrlLinks already converts newlines to <br /> via the 'newline' filter rule, so multiline links render correctly with preserved line breaks. Fixes Expensify/App#80233
|
All contributors have signed the CLA ✍️ ✅ |
melvin-bot
bot
requested review from
marcochavezf
and removed request for
a team
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Code Review: fix: allow multiline text in markdown links
Summary
This PR fixes Expensify/App#80233 by removing the \r\n exclusion from the character classes in MARKDOWN_LINK_REGEX, allowing newlines in the link label portion of markdown links (e.g. [line1\nline2](url)). The change is minimal (1 line in source + updated tests) and well-scoped.
Analysis
Regex change in lib/ExpensiMark.ts (line 76):
The change modifies three occurrences of [^\[\]\r\n] to [^\[\]] within MARKDOWN_LINK_REGEX. This removes the restriction that prevented the regex from matching newline characters inside the [...] label portion of markdown links.
Correctness -- LGTM. The existing modifyTextForUrlLinks method already applies filterRules: ['bold', 'strikethrough', 'italic', 'newline'] to the matched link text (line ~1073), which converts \n into <br />. So multiline labels will render with proper line breaks inside the anchor tag. The test expectations confirm this: <a href="...">Expensi<br />fy</a>.
Consistency with image regex. The MARKDOWN_IMAGE_REGEX on the very next line already uses [^\]\[]* without \r\n exclusions, so this change makes the link regex consistent with the image regex. Good.
URL portion remains protected. The URL part of the regex uses MARKDOWN_URL_REGEX from Url.js, which is built from character classes like [-a-z0-9], [-\w$@.+!*:(),=%~] etc. that inherently do not match newlines. So multiline URLs are still correctly rejected, as confirmed by the existing test: 'Before [Expensify](https://exa\nmple.com) after' still returns [].
Edge case: heading + multiline label. Line ~1059-1060 of modifyTextForUrlLinks has a guard: if (match[1].includes('</h1>')) -- this skips link rendering when the label contains an </h1> tag (from a markdown heading preceding a multiline link). This guard still works correctly with the expanded regex.
Potential Concerns
-
ReDoS risk (minor). The nested quantifier pattern
((?:[^\[\]]*(?:\[[^\[\]]*][^\[\]]*)*)*)with[^\[\]]*now matching newlines means the*quantifiers can match longer strings. However, the alternation structure (non-bracket chars OR bracketed groups) should avoid catastrophic backtracking since brackets serve as clear delimiters. The existing image regex has the same pattern without issue. Low risk. -
Whitespace-only label formatting. The replacement function at line ~313 has a guard:
if (!g1.trim()) { return match; }-- this means a label that is only whitespace/newlines (e.g.[\n\n](url)) won't be converted to a link, which is correct behavior. -
Minor unrelated style change. Line 116 in the test file changes
const {attrCachingFn, attrCache}toconst { attrCachingFn, attrCache }(added spaces inside braces). This is a trivial formatting change, but it's unrelated to the fix. Consider reverting to keep the diff focused, or ignore if auto-formatter produced it.
Test Coverage
- Updated existing tests properly reflect the new behavior (multiline labels now produce
<a>tags with<br />in the label text). extractLinksInMarkdownCommenttest correctly verifies multiline label links are now extracted.- Multiline URL test case is preserved and still expects no link extraction -- good.
- PR description mentions all 418 tests pass.
Verdict
The change is small, well-reasoned, and correctly leverages the existing newline filter rule in modifyTextForUrlLinks. The URL portion of the regex naturally excludes newlines, so only the label portion is affected. Tests are updated appropriately.
No blocking issues found.
🤖 This comment was generated with the assistance of an AI tool.
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@claude review |
|
@mccoychang can you merge from main? For some reason GH is not allowing me to merge it: 
|
|
@marcochavezf we can ignore this PR. The linked issue was closed. And @mccoychang didn't follow contributing process (not assigned). |
|
Closing this PR as I didn't follow the proper contributing process. I'll submit proposals through the correct workflow going forward. Thanks for the feedback! |
3 participants
Summary
Fixes Expensify/App#80233 — multiline markdown links were not rendered as clickable hyperlinks.
Root Cause
MARKDOWN_LINK_REGEXat line 76 ofExpensiMark.tsused[^\\[\\]\\r\\n]in its character classes, which explicitly excluded carriage returns and newlines from the link text. This meant any markdown link with newlines in the label (e.g.[line1\nline2](url)) would fail to match and render as plain text.Changes
lib/ExpensiMark.ts: Removed\\r\\nfrom the three negated character classes inMARKDOWN_LINK_REGEX([^\\[\\]\\r\\n]→[^\\[\\]]). The existingmodifyTextForUrlLinksmethod already applies the'newline'filter rule, which converts\nto<br />in link text — so multiline links render correctly with preserved line breaks.__tests__/ExpensiMark-test.js: Updated 2 existing tests to reflect the new expected behavior:<a>tags (previously expected to fail)extractLinksInMarkdownCommentnow extracts URLs from multiline label linksTest Results
All 418 tests pass (10 test suites, 0 failures).