Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
| const MEDIA_KEYWORD = String.raw`\[\[(?:pdf|audio|video)\]\]|pdf|audio|video`; | ||
|
|
||
| /** Excluding `<` and `>` is also what makes this idempotent: a wrapped embed cannot match. */ | ||
| const URL_PATTERN = String.raw`https?://[^\s<>()\[\]{}"']+`; |
There was a problem hiding this comment.
🟡 Parenthesized media URLs remain unprotected
For an embed URL containing literal parentheses, protectMediaEmbeds leaves the media embed unchanged. fromMarkdown then mangles the valid embed, losing the PDF, audio, or video.
Learn more
HTTP URLs can contain literal parentheses in their path or query. The URL pattern excludes both characters, so the complete media-embed pattern cannot match such an embed. The unchanged embed then reaches createImportedPage or updateImportedPage, where Roam applies the parser behavior this utility exists to prevent.
Example: {{[[pdf]]: https://example.org/files/report(1).pdf}} remains unchanged. The expected protected form is {{[[pdf]]: <https://example.org/files/report(1).pdf>}}.
Recommended fix: Allow balanced literal parentheses in URL_PATTERN, using the media embed's whitespace and closing }} as delimiters. Add unit and materialization tests for parenthesized URLs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const MEDIA_KEYWORD = String.raw`\[\[(?:pdf|audio|video)\]\]|pdf|audio|video`; | ||
|
|
||
| /** Excluding `<` and `>` is also what makes this idempotent: a wrapped embed cannot match. */ | ||
| const URL_PATTERN = String.raw`https?://[^\s<>()\[\]{}"']+`; |
There was a problem hiding this comment.
The URL pattern excludes parentheses (), brackets [], and braces {}, but these are valid URL characters. URLs like https://example.com/file(1).pdf or Wikipedia URLs with parentheses (e.g., https://en.wikipedia.org/wiki/File_(computing)) will not match the regex completely. The regex will fail to match the entire embed, leaving such URLs unprotected and vulnerable to being mangled by Roam's parser.
Fix: Allow these characters in the URL pattern:
const URL_PATTERN = String.raw`https?://[^\s<>"']+`;The pattern should only exclude characters that definitively end a URL in this context (<> for already-wrapped URLs and quotes), plus whitespace. The outer regex already handles the }} terminator.
| const URL_PATTERN = String.raw`https?://[^\s<>()\[\]{}"']+`; | |
| const URL_PATTERN = String.raw`https?://[^\s<>"']+`; |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
9666835 to
bb35581
Compare
bb35581 to
58abd4b
Compare
58abd4b to
36d510e
Compare
Reviewer brief
Straightforward: Protect some URLs before Roam parses markdown, because its parser gets confused by Roam syntax in markdown.
Verification
Many JS tests in Roam.
Loom video
https://www.loom.com/share/639a635e327a4bb1a1de356d1d259159
Scope check
$scope-checkagainst ENG-2289 and the final diff.Done When:protectMediaEmbedsalso wraps{{[[iframe]]: url}}and{{[[youtube]]: url}}.Done Whennames only pdf, audio and video.Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.https://linear.app/discourse-graphs/issue/ENG-2289/roam-import-destroys-pdf-audio-and-video-embeds