Skip to content

ENG-2289 Roam import destroys PDF, audio and video embeds - #1465

Open
maparent wants to merge 1 commit into
mainfrom
eng-2289-roam-import-destroys-pdf-audio-and-video-embeds
Open

maparent wants to merge 1 commit into
mainfrom
eng-2289-roam-import-destroys-pdf-audio-and-video-embeds

Conversation

@maparent

@maparent maparent commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

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

  • Ran $scope-check against ENG-2289 and the final diff.
  • Scope beyond Done When: protectMediaEmbeds also wraps {{[[iframe]]: url}} and {{[[youtube]]: url}}. Done When names only pdf, audio and video.
  • Required now: Same parser defect, same syntactic shape, same call sites — the keyword list is one alternation in a single regex, so covering them costs nothing and excluding them would leave a known-identical break in place. Nothing in the current import flow requires them specifically.
  • Anyone affected or consulted: Linear comments here.
  • Decision: No.

Local delegated full review

  • Ran a comprehensive review of the entire final diff in a subagent with a fresh context. Use $dg-delegated-full-review when no other full-review workflow is available.

https://linear.app/discourse-graphs/issue/ENG-2289/roam-import-destroys-pdf-audio-and-video-embeds


Devin Review

@vercel

vercel Bot commented Sep 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
discourse-graph Skipped Skipped Sep 18, 2026 8:03pm UTC

Request Review

@supabase

supabase Bot commented Sep 18, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@linear-code

linear-code Bot commented Sep 18, 2026

Copy link
Copy Markdown

ENG-2289

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

Devin Review

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<>()\[\]{}"']+`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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.

Devin Review


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<>()\[\]{}"']+`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Suggested change
const URL_PATTERN = String.raw`https?://[^\s<>()\[\]{}"']+`;
const URL_PATTERN = String.raw`https?://[^\s<>"']+`;

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@maparent
maparent force-pushed the eng-2289-roam-import-destroys-pdf-audio-and-video-embeds branch from 9666835 to bb35581 Compare September 18, 2026 19:33
@maparent
maparent force-pushed the eng-2289-roam-import-destroys-pdf-audio-and-video-embeds branch from bb35581 to 58abd4b Compare September 18, 2026 19:36
@maparent
maparent force-pushed the eng-2289-roam-import-destroys-pdf-audio-and-video-embeds branch from 58abd4b to 36d510e Compare September 18, 2026 20:02
@maparent
maparent requested a review from mdroidian September 18, 2026 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant