diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index dca2034a4..b0bfb5d3b 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/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", diff --git a/apps/roam/src/utils/__tests__/protectMediaEmbeds.test.ts b/apps/roam/src/utils/__tests__/protectMediaEmbeds.test.ts new file mode 100644 index 000000000..89823c629 --- /dev/null +++ b/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]]: }}`); + }); + + 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); + }); +}); diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index 4f9036732..f16448f35 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/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))); diff --git a/apps/roam/src/utils/protectMediaEmbeds.ts b/apps/roam/src/utils/protectMediaEmbeds.ts new file mode 100644 index 000000000..1cd7fc343 --- /dev/null +++ b/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}}}`; + });