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

apps/roam/src/utils/__tests__/materializeSharedNode.test.ts

@@ -119,6 +119,9 @@ const FULL_MARKDOWN = [
 
 const MATERIALIZED_MARKDOWN = "# Findings\nREM sleep improves recall";
 
+const ASSET_URL =
+  "https://firebasestorage.googleapis.com/v0/b/f.appspot.com/o/imgs%2Fapp%2Fgraph%2Fx.pdf?alt=media&token=abc";
+
 const NO_ASSETS = { mirrored: 0, reused: 0, skipped: [], failed: [] };
 
 const clientWithFullContent = ({
@@ -1008,6 +1011,47 @@ describe("materializeSharedNode", () => {
     });
   });
 
+  // Without the brackets Roam's parser destroys the embed. See `protectMediaEmbeds`.
+  it("brackets a media embed's URL before handing the markdown to Roam", async () => {
+    const embed = `{{[[pdf]]: ${ASSET_URL}}}`;
+    const { client } = clientWithFullContent({
+      text: `# ${roamSharedNode.title}\n\n${embed}\n`,
+      contentType: "text/roam+markdown",
+    });
+
+    const result = await materializeSharedNode({
+      client,
+      sharedNode: roamSharedNode,
+    });
+
+    expect(result.success).toBe(true);
+    expect(pageFromMarkdown).toHaveBeenCalledWith({
+      page: { title: roamSharedNode.title, uid: GENERATED_PAGE_UID },
+      "markdown-string": `{{[[pdf]]: <${ASSET_URL}>}}`,
+    });
+  });
+
+  it("brackets a media embed's URL when replacing an imported page too", async () => {
+    const embed = `{{[[audio]]: ${ASSET_URL}}}`;
+    const { client } = clientWithFullContent({
+      text: `# ${roamSharedNode.title}\n\n${embed}\n`,
+      contentType: "text/roam+markdown",
+    });
+    mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID);
+    mockedGetPageTitleByPageUid.mockReturnValue(roamSharedNode.title);
+
+    const result = await materializeSharedNode({
+      client,
+      sharedNode: roamSharedNode,
+    });
+
+    expect(result.success).toBe(true);
+    expect(blockFromMarkdown).toHaveBeenCalledWith({
+      location: { "parent-uid": EXISTING_PAGE_UID, order: "last" },
+      "markdown-string": `{{[[audio]]: <${ASSET_URL}>}}`,
+    });
+  });
+
   it("keeps a first line that does not match the shared title exactly", async () => {
     const { client } = clientWithFullContent({
       text: "# Some other heading\n\n- body",

apps/roam/src/utils/__tests__/protectMediaEmbeds.test.ts

@@ -0,0 +1,95 @@
+import { describe, expect, it } from "vitest";
+import { protectMediaEmbeds } from "../protectMediaEmbeds";
+
+const ASSET_URL =
+  "https://firebasestorage.googleapis.com/v0/b/f.appspot.com/o/imgs%2Fapp%2Fgraph%2F5AwymdkFOm.pdf?alt=media&token=70592b01-5df2-40ca-b898-c2c21079373c";
+
+/**
+ * Every keyword below was run through `block.fromMarkdown` in a graph. Wrapped, each came
+ * back as it went in, except that parentheses come back percent-encoded; bare, each came
+ * back mangled.
+ *
+ * Roam documents none of this, so the parser changing has to fail here rather than in a
+ * user's page.
+ */
+describe("protectMediaEmbeds", () => {
+  it.each(["pdf", "audio", "video", "iframe", "youtube"])(
+    "wraps a %s embed's URL",
+    (keyword) => {
+      expect(protectMediaEmbeds(`{{[[${keyword}]]: ${ASSET_URL}}}`)).toBe(
+        `{{[[${keyword}]]: <${ASSET_URL}>}}`,
+      );
+      expect(protectMediaEmbeds(`{{${keyword}: ${ASSET_URL}}}`)).toBe(
+        `{{${keyword}: <${ASSET_URL}>}}`,
+      );
+    },
+  );
+
+  it("keeps the spacing an embed was written with", () => {
+    expect(protectMediaEmbeds(`{{[[pdf]]:${ASSET_URL}}}`)).toBe(
+      `{{[[pdf]]:<${ASSET_URL}>}}`,
+    );
+    expect(protectMediaEmbeds(`{{[[pdf]]:   ${ASSET_URL}  }}`)).toBe(
+      `{{[[pdf]]:   <${ASSET_URL}>  }}`,
+    );
+  });
+
+  it("leaves a URL's percent-escapes exactly as written", () => {
+    const lowercased = ASSET_URL.replace("%2F", "%2f");
+    expect(protectMediaEmbeds(`{{[[pdf]]: ${lowercased}}}`)).toBe(
+      `{{[[pdf]]: <${lowercased}>}}`,
+    );
+  });
+
+  it("wraps an embed a caller marked, without disturbing the marker", () => {
+    expect(
+      protectMediaEmbeds(`{{[[pdf]]: ${ASSET_URL}}} (Failed to import)`),
+    ).toBe(`{{[[pdf]]: <${ASSET_URL}>}} (Failed to import)`);
+  });
+
+  it("wraps a URL holding parentheses, which a bare-URL pattern would stop at", () => {
+    const parenthesised = "https://en.wikipedia.org/wiki/Foo_(bar).pdf";
+    expect(protectMediaEmbeds(`{{[[pdf]]: ${parenthesised}}}`)).toBe(
+      `{{[[pdf]]: <${parenthesised}>}}`,
+    );
+  });
+
+  it("wraps an external embed, which no asset row covers", () => {
+    expect(
+      protectMediaEmbeds(`{{[[pdf]]: https://example.org/paper.pdf}}`),
+    ).toBe(`{{[[pdf]]: <https://example.org/paper.pdf>}}`);
+  });
+
+  it("wraps every embed in a multi-line note", () => {
+    expect(
+      protectMediaEmbeds(
+        `Protocol: {{[[pdf]]: ${ASSET_URL}}}\nRecording: {{[[audio]]: ${ASSET_URL}}}`,
+      ),
+    ).toBe(
+      `Protocol: {{[[pdf]]: <${ASSET_URL}>}}\nRecording: {{[[audio]]: <${ASSET_URL}>}}`,
+    );
+  });
+
+  it("is idempotent, so a wrapped embed is never wrapped twice", () => {
+    const once = protectMediaEmbeds(`{{[[pdf]]: ${ASSET_URL}}}`);
+    expect(protectMediaEmbeds(once)).toBe(once);
+  });
+
+  // Roam writes this one out as a markdown link whether or not the URL is wrapped, so
+  // matching it would claim a protection that does not exist.
+  it("leaves a URL holding square brackets alone", () => {
+    const bracketed = "{{[[pdf]]: https://example.org/a[b].pdf}}";
+    expect(protectMediaEmbeds(bracketed)).toBe(bracketed);
+  });
+
+  // These survive `fromMarkdown` as they are, so wrapping them would buy nothing.
+  it.each([
+    ["an image embed", `![](${ASSET_URL})`],
+    ["a bare URL", ASSET_URL],
+    ["a markdown link", `[the paper](${ASSET_URL})`],
+    ["a component that carries no URL", `{{[[TODO]]}}`],
+    ["an embed of a vault path", `{{[[pdf]]: attachments/paper.pdf}}`],
+  ])("leaves %s alone", (_name, markdown) => {
+    expect(protectMediaEmbeds(markdown)).toBe(markdown);
+  });
+});

apps/roam/src/utils/materializeSharedNode.ts

@@ -22,6 +22,7 @@ import {
 } from "./importedSourceIdentity";
 import { getErrorMessage } from "./getErrorMessage";
 import { importNodeAssets, type AssetImportReport } from "./importNodeAssets";
+import { protectMediaEmbeds } from "./protectMediaEmbeds";
 import {
   MISSING_SOURCE_PLACEHOLDER,
   schemaHasSourceSlot,
@@ -272,7 +273,7 @@ const createImportedPage = async ({
     if (markdown) {
       await getRoamMarkdownApi().page.fromMarkdown({
         page: { title, uid: pageUid },
-        "markdown-string": markdown,
+        "markdown-string": protectMediaEmbeds(markdown),
       });
     } else {
       await window.roamAlphaAPI.data.page.create({
@@ -337,7 +338,7 @@ const updateImportedPage = async ({
     if (markdown) {
       await getRoamMarkdownApi().block.fromMarkdown({
         location: { "parent-uid": pageUid, order: "last" },
-        "markdown-string": markdown,
+        "markdown-string": protectMediaEmbeds(markdown),
       });
     }
     await Promise.all(previousChildren.map(({ uid }) => deleteBlock(uid)));

apps/roam/src/utils/protectMediaEmbeds.ts

@@ -0,0 +1,49 @@
+/**
+ * Protects Roam's media embeds from Roam's own markdown parser.
+ *
+ * `{{[[pdf]]: url}}` is not markdown, so `fromMarkdown` autolinks the URL and swallows the
+ * closing `}}`. The braces come back percent-encoded in the link's destination and raw in
+ * its text, so the two differ and the block is written as `{{[[pdf]]: [url}}](url%7D%7D)`.
+ * Wrapping the URL in `<>` ends the autolink explicitly, so Roam writes the URL back bare.
+ *
+ * Only this shape needs it. A bare URL and `![](url)` already survive the round trip.
+ */
+
+/**
+ * Every component that carries a bare URL in this shape, not only the ones an imported
+ * asset can become: the mangling is the parser's and hits all of them.
+ */
+const MEDIA_KEYWORDS = "pdf|audio|video|iframe|youtube";
+
+/** Roam accepts both spellings. */
+const MEDIA_KEYWORD_PATTERN = String.raw`\[\[(?:${MEDIA_KEYWORDS})\]\]|${MEDIA_KEYWORDS}`;
+
+/**
+ * Deliberately not the class the identically named pattern in `rewriteAssetLinks.ts` uses:
+ * `}}` ends the match here, so parentheses need not. They come back percent-encoded, so a
+ * reader sees `a%28b%29.pdf` where the author wrote `a(b).pdf`, which beats a broken embed.
+ * Square brackets stay excluded: they diverge the same way the braces do above, and
+ * wrapping cannot save them.
+ *
+ * Excluding `<` and `>` is what makes this idempotent: a wrapped embed cannot match.
+ */
+const URL_PATTERN = String.raw`https?://[^\s<>\[\]{}"']+`;
+
+/** Whitespace is captured rather than normalised, so a rewrite costs the two brackets only. */
+const MEDIA_EMBED = new RegExp(
+  String.raw`\{\{(${MEDIA_KEYWORD_PATTERN}):([^\S\n]*)(${URL_PATTERN})([^\S\n]*)\}\}`,
+  "g",
+);
+
+/**
+ * Applied to the whole markdown rather than where embeds are rendered: an import passes
+ * through every embed it finds no asset row for, and those are mangled the same way.
+ *
+ * An embed inside a code block or code span gains a visible `<>`. Accepted: cosmetic
+ * there, where mangling is not.
+ */
+export const protectMediaEmbeds = (markdown: string): string =>
+  markdown.replace(MEDIA_EMBED, (_match: string, ...args: unknown[]) => {
+    const [keyword, before, url, after] = args as string[];
+    return `{{${keyword}:${before}<${url}>${after}}}`;
+  });