From 6dfcde87f5fef3bb0b21ce99bc0e866eff83ff29 Mon Sep 17 00:00:00 2001 From: sid597 Date: Wed, 29 Jul 2026 11:59:18 +0530 Subject: [PATCH 1/5] [ENG-1858] Materialize Obsidian-origin markdown into Roam Adds materializeSharedNode, which consumes one Obsidian-origin SharedNode payload: fetches the full-variant markdown, strips YAML frontmatter, converts the body to a Roam block tree, and creates the page or updates the existing one found by sourceNodeRid (rename included) so re-imports never duplicate. Source identity is written via the ENG-1856 helpers after content writes succeed; failures return actionable reasons without touching stored identity. markdownToRoamBlocks is a pure structural parser (headings nest content, lists nest by indentation, fenced code stays whole); fidelity limits are documented under ENG-1882. No UI or call site here - the import action lands with ENG-1859. --- .../__tests__/markdownToRoamBlocks.test.ts | 157 +++++++++++ .../__tests__/materializeSharedNode.test.ts | 262 ++++++++++++++++++ apps/roam/src/utils/markdownToRoamBlocks.ts | 111 ++++++++ apps/roam/src/utils/materializeSharedNode.ts | 133 +++++++++ 4 files changed, 663 insertions(+) create mode 100644 apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts create mode 100644 apps/roam/src/utils/__tests__/materializeSharedNode.test.ts create mode 100644 apps/roam/src/utils/markdownToRoamBlocks.ts create mode 100644 apps/roam/src/utils/materializeSharedNode.ts diff --git a/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts b/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts new file mode 100644 index 000000000..fd336ddd9 --- /dev/null +++ b/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, it } from "vitest"; +import { markdownToRoamBlocks } from "~/utils/markdownToRoamBlocks"; + +describe("markdownToRoamBlocks", () => { + it("strips YAML frontmatter and splits paragraphs on blank lines", () => { + expect( + markdownToRoamBlocks( + [ + "---", + "nodeTypeId: claim", + "nodeInstanceId: node-1", + "---", + "", + "First paragraph line one", + "line two", + "", + "Second paragraph", + ].join("\n"), + ), + ).toEqual([ + { text: "First paragraph line one\nline two", children: [] }, + { text: "Second paragraph", children: [] }, + ]); + }); + + it("nests content under headings by heading level", () => { + expect( + markdownToRoamBlocks( + ["# Top", "intro", "## Sub", "detail", "# Next", "tail"].join("\n"), + ), + ).toEqual([ + { + text: "Top", + heading: 1, + children: [ + { text: "intro", children: [] }, + { + text: "Sub", + heading: 2, + children: [{ text: "detail", children: [] }], + }, + ], + }, + { + text: "Next", + heading: 1, + children: [{ text: "tail", children: [] }], + }, + ]); + }); + + it("clamps headings deeper than Roam's maximum but keeps their nesting depth", () => { + expect( + markdownToRoamBlocks(["### Three", "#### Four", "text"].join("\n")), + ).toEqual([ + { + text: "Three", + heading: 3, + children: [ + { + text: "Four", + heading: 3, + children: [{ text: "text", children: [] }], + }, + ], + }, + ]); + }); + + it("nests list items by indentation and strips list markers", () => { + expect( + markdownToRoamBlocks( + [ + "- parent", + " - child", + " - grandchild", + "- sibling", + "1. ordered", + ].join("\n"), + ), + ).toEqual([ + { + text: "parent", + children: [ + { + text: "child", + children: [{ text: "grandchild", children: [] }], + }, + ], + }, + { text: "sibling", children: [] }, + { text: "ordered", children: [] }, + ]); + }); + + it("keeps a loose list nested after blank lines between items", () => { + expect( + markdownToRoamBlocks(["- parent", "", " - child"].join("\n")), + ).toEqual([ + { + text: "parent", + children: [{ text: "child", children: [] }], + }, + ]); + }); + + it("nests tab-indented list items", () => { + expect(markdownToRoamBlocks(["- parent", "\t- child"].join("\n"))).toEqual([ + { + text: "parent", + children: [{ text: "child", children: [] }], + }, + ]); + }); + + it("keeps a fenced code block as a single block, including blank lines", () => { + expect( + markdownToRoamBlocks( + ["```js", "const a = 1;", "", "const b = 2;", "```"].join("\n"), + ), + ).toEqual([ + { + text: "```js\nconst a = 1;\n\nconst b = 2;\n```", + children: [], + }, + ]); + }); + + it("keeps an unclosed fence as a single block through the end of input", () => { + expect(markdownToRoamBlocks(["```", "code", "more"].join("\n"))).toEqual([ + { text: "```\ncode\nmore", children: [] }, + ]); + }); + + it("returns no blocks for empty or frontmatter-only markdown", () => { + expect(markdownToRoamBlocks("")).toEqual([]); + expect( + markdownToRoamBlocks(["---", "nodeTypeId: claim", "---"].join("\n")), + ).toEqual([]); + }); + + it("treats an unclosed frontmatter delimiter as content", () => { + expect(markdownToRoamBlocks(["---", "not frontmatter"].join("\n"))).toEqual( + [{ text: "---\nnot frontmatter", children: [] }], + ); + }); + + it("handles CRLF line endings", () => { + expect(markdownToRoamBlocks("# Title\r\ntext\r\n")).toEqual([ + { + text: "Title", + heading: 1, + children: [{ text: "text", children: [] }], + }, + ]); + }); +}); diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts new file mode 100644 index 000000000..7d1e2c4fd --- /dev/null +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -0,0 +1,262 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; +import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; +import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; +import createBlock from "roamjs-components/writes/createBlock"; +import createPage from "roamjs-components/writes/createPage"; +import deleteBlock from "roamjs-components/writes/deleteBlock"; +import type { DGSupabaseClient } from "@repo/database/lib/client"; +import type { SharedNode } from "@repo/database/lib/sharedNodes"; +import { + findImportedNodeUidBySourceRid, + writeImportedSourceIdentity, +} from "~/utils/importedSourceIdentity"; +import { materializeSharedNode } from "~/utils/materializeSharedNode"; + +vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ + default: vi.fn(), +})); +vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ + default: vi.fn(), +})); +vi.mock("roamjs-components/queries/getShallowTreeByParentUid", () => ({ + default: vi.fn(), +})); +vi.mock("roamjs-components/writes/createBlock", () => ({ default: vi.fn() })); +vi.mock("roamjs-components/writes/createPage", () => ({ default: vi.fn() })); +vi.mock("roamjs-components/writes/deleteBlock", () => ({ default: vi.fn() })); +vi.mock("~/utils/importedSourceIdentity", () => ({ + findImportedNodeUidBySourceRid: vi.fn(), + writeImportedSourceIdentity: vi.fn(), +})); + +const mockedGetPageTitleByPageUid = vi.mocked(getPageTitleByPageUid); +const mockedGetPageUidByPageTitle = vi.mocked(getPageUidByPageTitle); +const mockedGetShallowTreeByParentUid = vi.mocked(getShallowTreeByParentUid); +const mockedCreateBlock = vi.mocked(createBlock); +const mockedCreatePage = vi.mocked(createPage); +const mockedDeleteBlock = vi.mocked(deleteBlock); +const mockedFindImportedNodeUidBySourceRid = vi.mocked( + findImportedNodeUidBySourceRid, +); +const mockedWriteImportedSourceIdentity = vi.mocked( + writeImportedSourceIdentity, +); + +const EXISTING_PAGE_UID = "existing-page-uid"; + +const updatePage = vi.fn(); + +const sharedNode: SharedNode = { + rid: "orn:obsidian.note:vault-a/node-1", + sourceLocalId: "node-1", + spaceId: 20, + spaceName: "Research vault", + spaceUri: "obsidian:vault-a", + platform: "Obsidian", + title: "EVD - REM sleep and recall", + created: "2026-06-14T12:30:00.000Z", + lastModified: "2026-06-14T15:00:00.000Z", + authorId: 7, + directMetadata: null, +}; + +const FULL_MARKDOWN = [ + "---", + "nodeTypeId: evidence", + "---", + "", + "# Findings", + "REM sleep improves recall", +].join("\n"); + +const FULL_TREE = [ + { + text: "Findings", + heading: 1, + children: [{ text: "REM sleep improves recall", children: [] }], + }, +]; + +const clientWithFullContent = ({ + text, + error, +}: { + text?: string | null; + error?: { message: string }; +}): { client: DGSupabaseClient; from: ReturnType } => { + const maybeSingle = vi + .fn() + .mockResolvedValue( + error + ? { data: null, error } + : { data: text === undefined ? null : { text }, error: null }, + ); + const eq = vi.fn(); + const chain = { eq, maybeSingle }; + eq.mockReturnValue(chain); + const from = vi + .fn() + .mockReturnValue({ select: vi.fn().mockReturnValue(chain) }); + return { client: { from } as unknown as DGSupabaseClient, from }; +}; + +beforeEach(() => { + vi.clearAllMocks(); + (globalThis as { window: unknown }).window = { + roamAlphaAPI: { updatePage }, + }; + mockedGetShallowTreeByParentUid.mockReturnValue([]); +}); + +describe("materializeSharedNode", () => { + it("creates a Roam page with the parsed markdown and stores source identity", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); + mockedGetPageUidByPageTitle.mockReturnValue(""); + mockedCreatePage.mockResolvedValue("new-page-uid"); + + await expect( + materializeSharedNode({ client, sharedNode }), + ).resolves.toEqual({ status: "created", pageUid: "new-page-uid" }); + expect(mockedCreatePage).toHaveBeenCalledWith({ + title: sharedNode.title, + tree: FULL_TREE, + }); + expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ + pageUid: "new-page-uid", + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }); + }); + + it("updates the existing imported page instead of creating a duplicate", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); + mockedGetPageTitleByPageUid.mockReturnValue(sharedNode.title); + mockedGetShallowTreeByParentUid.mockReturnValue([ + { uid: "old-block", text: "stale" }, + ]); + + await expect( + materializeSharedNode({ client, sharedNode }), + ).resolves.toEqual({ status: "updated", pageUid: EXISTING_PAGE_UID }); + expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(updatePage).not.toHaveBeenCalled(); + expect(mockedDeleteBlock).toHaveBeenCalledWith("old-block"); + expect(mockedCreateBlock).toHaveBeenCalledWith({ + parentUid: EXISTING_PAGE_UID, + order: 0, + node: FULL_TREE[0], + }); + expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ + pageUid: EXISTING_PAGE_UID, + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }); + }); + + it("renames the imported page when the source title changed", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); + mockedGetPageTitleByPageUid.mockReturnValue("EVD - old title"); + mockedGetPageUidByPageTitle.mockReturnValue(""); + + await expect( + materializeSharedNode({ client, sharedNode }), + ).resolves.toEqual({ status: "updated", pageUid: EXISTING_PAGE_UID }); + expect(updatePage).toHaveBeenCalledWith({ + page: { uid: EXISTING_PAGE_UID, title: sharedNode.title }, + }); + }); + + it("fails without writing when a local page already uses the title", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); + mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.status).toBe("failed"); + expect(result.status === "failed" && result.reason).toContain( + sharedNode.title, + ); + expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + }); + + it("fails the rename without touching content when the new title collides", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); + mockedGetPageTitleByPageUid.mockReturnValue("EVD - old title"); + mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.status).toBe("failed"); + expect(updatePage).not.toHaveBeenCalled(); + expect(mockedDeleteBlock).not.toHaveBeenCalled(); + expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + }); + + it("rejects nodes that are not Obsidian-origin before fetching content", async () => { + const { client, from } = clientWithFullContent({ text: FULL_MARKDOWN }); + + const result = await materializeSharedNode({ + client, + sharedNode: { ...sharedNode, platform: "Roam" }, + }); + + expect(result.status).toBe("failed"); + expect(result.status === "failed" && result.reason).toContain("Roam"); + expect(from).not.toHaveBeenCalled(); + }); + + it("fails with the fetch error and keeps identity untouched", async () => { + const { client } = clientWithFullContent({ + error: { message: "permission denied" }, + }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.status).toBe("failed"); + expect(result.status === "failed" && result.reason).toContain( + "permission denied", + ); + expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + }); + + it("materializes a title-only page when the node has no full content", async () => { + const { client } = clientWithFullContent({}); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); + mockedGetPageUidByPageTitle.mockReturnValue(""); + mockedCreatePage.mockResolvedValue("new-page-uid"); + + await expect( + materializeSharedNode({ client, sharedNode }), + ).resolves.toEqual({ status: "created", pageUid: "new-page-uid" }); + expect(mockedCreatePage).toHaveBeenCalledWith({ + title: sharedNode.title, + tree: [], + }); + }); + + it("reports the source RID when a write fails partway", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); + mockedGetPageTitleByPageUid.mockReturnValue(sharedNode.title); + mockedCreateBlock.mockRejectedValue(new Error("block write failed")); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.status).toBe("failed"); + expect(result.status === "failed" && result.reason).toContain( + sharedNode.rid, + ); + expect(result.status === "failed" && result.reason).toContain( + "block write failed", + ); + expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/roam/src/utils/markdownToRoamBlocks.ts b/apps/roam/src/utils/markdownToRoamBlocks.ts new file mode 100644 index 000000000..d6b12b534 --- /dev/null +++ b/apps/roam/src/utils/markdownToRoamBlocks.ts @@ -0,0 +1,111 @@ +import { normalizeLineEndings } from "@repo/content-model"; +import type { InputTextNode } from "roamjs-components/types/native"; + +type BlockNode = InputTextNode & { children: InputTextNode[] }; + +const FRONTMATTER_DELIMITER = /^---\s*$/; +const HEADING = /^(#{1,6})\s+(.+)$/; +const LIST_ITEM = /^([ \t]*)(?:[-*+]|\d+[.)])[ \t]+(.+)$/; +const ROAM_MAX_HEADING_LEVEL = 3; + +const stripFrontmatter = (lines: string[]): string[] => { + if (lines.length === 0 || !FRONTMATTER_DELIMITER.test(lines[0])) return lines; + const closingIndex = lines.findIndex( + (line, index) => index > 0 && FRONTMATTER_DELIMITER.test(line), + ); + return closingIndex === -1 ? lines : lines.slice(closingIndex + 1); +}; + +const indentWidth = (whitespace: string): number => + [...whitespace].reduce((width, char) => width + (char === "\t" ? 4 : 1), 0); + +export const markdownToRoamBlocks = (markdown: string): InputTextNode[] => { + const lines = stripFrontmatter(normalizeLineEndings(markdown).split("\n")); + const roots: InputTextNode[] = []; + const headingStack: { level: number; node: BlockNode }[] = []; + let listStack: { indent: number; node: BlockNode }[] = []; + let paragraphLines: string[] = []; + + const sectionBlocks = (): InputTextNode[] => + headingStack.length + ? headingStack[headingStack.length - 1].node.children + : roots; + + const flushParagraph = (): void => { + if (!paragraphLines.length) return; + sectionBlocks().push({ text: paragraphLines.join("\n"), children: [] }); + paragraphLines = []; + }; + + for (let index = 0; index < lines.length; index++) { + const line = lines[index]; + + if (line.startsWith("```") || line.startsWith("~~~")) { + flushParagraph(); + listStack = []; + const fence = line.slice(0, 3); + const fenceLines = [line]; + while (index + 1 < lines.length && !lines[index + 1].startsWith(fence)) { + index++; + fenceLines.push(lines[index]); + } + if (index + 1 < lines.length) { + index++; + fenceLines.push(lines[index]); + } + sectionBlocks().push({ text: fenceLines.join("\n"), children: [] }); + continue; + } + + const headingMatch = HEADING.exec(line); + if (headingMatch) { + flushParagraph(); + listStack = []; + const level = headingMatch[1].length; + while ( + headingStack.length && + headingStack[headingStack.length - 1].level >= level + ) { + headingStack.pop(); + } + const node: BlockNode = { + text: headingMatch[2].trimEnd(), + heading: Math.min(level, ROAM_MAX_HEADING_LEVEL), + children: [], + }; + sectionBlocks().push(node); + headingStack.push({ level, node }); + continue; + } + + const listMatch = LIST_ITEM.exec(line); + if (listMatch) { + flushParagraph(); + const indent = indentWidth(listMatch[1]); + while ( + listStack.length && + listStack[listStack.length - 1].indent >= indent + ) { + listStack.pop(); + } + const node: BlockNode = { text: listMatch[2].trimEnd(), children: [] }; + const siblings = listStack.length + ? listStack[listStack.length - 1].node.children + : sectionBlocks(); + siblings.push(node); + listStack.push({ indent, node }); + continue; + } + + if (!line.trim()) { + flushParagraph(); + continue; + } + + listStack = []; + paragraphLines.push(line.trimEnd()); + } + + flushParagraph(); + return roots; +}; diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts new file mode 100644 index 000000000..d16138fff --- /dev/null +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -0,0 +1,133 @@ +import type { DGSupabaseClient } from "@repo/database/lib/client"; +import type { SharedNode } from "@repo/database/lib/sharedNodes"; +import type { InputTextNode } from "roamjs-components/types/native"; +import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; +import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; +import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; +import createBlock from "roamjs-components/writes/createBlock"; +import createPage from "roamjs-components/writes/createPage"; +import deleteBlock from "roamjs-components/writes/deleteBlock"; +import { + findImportedNodeUidBySourceRid, + writeImportedSourceIdentity, +} from "./importedSourceIdentity"; +import { markdownToRoamBlocks } from "./markdownToRoamBlocks"; + +export type MaterializeSharedNodeResult = + | { status: "created" | "updated"; pageUid: string } + | { status: "failed"; reason: string }; + +const fetchFullMarkdown = async ({ + client, + sharedNode, +}: { + client: DGSupabaseClient; + sharedNode: SharedNode; +}): Promise<{ markdown: string } | { error: string }> => { + const { data, error } = await client + .from("my_contents") + .select("text") + .eq("space_id", sharedNode.spaceId) + .eq("source_local_id", sharedNode.sourceLocalId) + .eq("variant", "full") + .maybeSingle(); + if (error) return { error: error.message }; + return { markdown: data?.text ?? "" }; +}; + +const createImportedPage = async ({ + sharedNode, + tree, +}: { + sharedNode: SharedNode; + tree: InputTextNode[]; +}): Promise => { + const collidingPageUid = getPageUidByPageTitle(sharedNode.title); + if (collidingPageUid) { + return { + status: "failed", + reason: `A page titled "${sharedNode.title}" already exists in this graph but was not imported from "${sharedNode.spaceName}". Rename or remove that page, then import again.`, + }; + } + const pageUid = await createPage({ title: sharedNode.title, tree }); + writeImportedSourceIdentity({ + pageUid, + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }); + return { status: "created", pageUid }; +}; + +const updateImportedPage = async ({ + pageUid, + sharedNode, + tree, +}: { + pageUid: string; + sharedNode: SharedNode; + tree: InputTextNode[]; +}): Promise => { + const localTitle = getPageTitleByPageUid(pageUid); + if (localTitle !== sharedNode.title) { + const collidingPageUid = getPageUidByPageTitle(sharedNode.title); + if (collidingPageUid) { + return { + status: "failed", + reason: `Cannot rename the imported page "${localTitle}" to "${sharedNode.title}": another page already has that title. Rename or remove that page, then import again.`, + }; + } + await window.roamAlphaAPI.updatePage({ + page: { uid: pageUid, title: sharedNode.title }, + }); + } + for (const { uid } of getShallowTreeByParentUid(pageUid)) { + await deleteBlock(uid); + } + for (const [order, node] of tree.entries()) { + await createBlock({ parentUid: pageUid, order, node }); + } + writeImportedSourceIdentity({ + pageUid, + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }); + return { status: "updated", pageUid }; +}; + +export const materializeSharedNode = async ({ + client, + sharedNode, +}: { + client: DGSupabaseClient; + sharedNode: SharedNode; +}): Promise => { + if (sharedNode.platform !== "Obsidian") { + return { + status: "failed", + reason: `Cannot import "${sharedNode.title}": materialization only supports Obsidian-origin nodes, and this node comes from ${sharedNode.platform}.`, + }; + } + try { + const content = await fetchFullMarkdown({ client, sharedNode }); + if ("error" in content) { + return { + status: "failed", + reason: `Could not fetch the content of "${sharedNode.title}" from "${sharedNode.spaceName}": ${content.error}`, + }; + } + const tree = markdownToRoamBlocks(content.markdown); + const importedPageUid = await findImportedNodeUidBySourceRid( + sharedNode.rid, + ); + return importedPageUid + ? await updateImportedPage({ pageUid: importedPageUid, sharedNode, tree }) + : await createImportedPage({ sharedNode, tree }); + } catch (error) { + return { + status: "failed", + reason: `Materializing "${sharedNode.title}" (${sharedNode.rid}) failed: ${ + error instanceof Error ? error.message : String(error) + }`, + }; + } +}; From 17cc8f97b8309041c745f7e94a3f45ffa9fca30d Mon Sep 17 00:00:00 2001 From: sid597 Date: Wed, 29 Jul 2026 12:41:06 +0530 Subject: [PATCH 2/5] [ENG-1858] Materialize via Roam's native fromMarkdown API Replaces the hand-rolled markdown parser with Roam's native data.page.fromMarkdown / data.block.fromMarkdown, so Roam owns markdown conversion. stripFrontmatter moves to @repo/content-model next to normalizeLineEndings. Results now carry the source identity and a failure stage so ENG-1859 can report per-node outcomes. Input is validated at the boundary (platform, RID shape, modified time, full content type). setBlockProps returns its update promise so the identity write can be awaited; existing call sites keep fire-and-forget behavior via void. On create, a page whose identity cannot be stored is removed again; on update, replacement blocks land before old ones are deleted. --- apps/roam/src/components/canvas/Clipboard.tsx | 2 +- .../src/components/canvas/canvasSyncMode.ts | 2 +- .../settings/DiscourseNodeConfigPanel.tsx | 2 +- .../components/settings/utils/accessors.ts | 4 +- .../src/components/settings/utils/init.ts | 8 +- .../utils/migrateLegacyToBlockProps.ts | 2 +- .../__tests__/importedSourceIdentity.test.ts | 5 +- .../__tests__/markdownToRoamBlocks.test.ts | 157 -------- .../__tests__/materializeSharedNode.test.ts | 296 ++++++++++----- apps/roam/src/utils/importedSourceIdentity.ts | 6 +- apps/roam/src/utils/markdownToRoamBlocks.ts | 111 ------ apps/roam/src/utils/materializeSharedNode.ts | 341 ++++++++++++++---- apps/roam/src/utils/migrateRelations.ts | 2 +- apps/roam/src/utils/setBlockProps.ts | 9 +- apps/roam/src/utils/supabaseContext.ts | 2 +- .../content-model/src/__tests__/text.test.ts | 47 +++ packages/content-model/src/text/index.ts | 18 + 17 files changed, 571 insertions(+), 443 deletions(-) delete mode 100644 apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts delete mode 100644 apps/roam/src/utils/markdownToRoamBlocks.ts create mode 100644 packages/content-model/src/__tests__/text.test.ts diff --git a/apps/roam/src/components/canvas/Clipboard.tsx b/apps/roam/src/components/canvas/Clipboard.tsx index e7ac171ac..b3d11457a 100644 --- a/apps/roam/src/components/canvas/Clipboard.tsx +++ b/apps/roam/src/components/canvas/Clipboard.tsx @@ -202,7 +202,7 @@ export const ClipboardProvider = ({ if (!isInitialized || !clipboardBlockUid) return; try { - setBlockProps(clipboardBlockUid, { + void setBlockProps(clipboardBlockUid, { [CLIPBOARD_PROP_KEY]: pages, [CLIPBOARD_SHOW_NODES_ON_CANVAS_PROP_KEY]: showNodesOnCanvas, }); diff --git a/apps/roam/src/components/canvas/canvasSyncMode.ts b/apps/roam/src/components/canvas/canvasSyncMode.ts index 692b07579..20d8979d9 100644 --- a/apps/roam/src/components/canvas/canvasSyncMode.ts +++ b/apps/roam/src/components/canvas/canvasSyncMode.ts @@ -42,7 +42,7 @@ const setRoamJsQueryBuilderProps = ({ pageUid: string; nextRjsqb: Record; }): void => { - setBlockProps(pageUid, { + void setBlockProps(pageUid, { [QUERY_BUILDER_PROP_KEY]: nextRjsqb, }); }; diff --git a/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx index 01a2c289d..f42ef9e07 100644 --- a/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx @@ -110,7 +110,7 @@ const DiscourseNodeConfigPanel: React.FC = ({ }, ], }).then((valueUid) => { - setBlockProps( + void setBlockProps( valueUid, DiscourseNodeSchema.parse({ text: label, diff --git a/apps/roam/src/components/settings/utils/accessors.ts b/apps/roam/src/components/settings/utils/accessors.ts index 1c1fa0ad5..538ec3512 100644 --- a/apps/roam/src/components/settings/utils/accessors.ts +++ b/apps/roam/src/components/settings/utils/accessors.ts @@ -657,7 +657,7 @@ const setBlockPropAtPath = ( return currentContext[currentKey]; }, updatedProps); - setBlockProps(blockUid, updatedProps, false); + void setBlockProps(blockUid, updatedProps, false); }; const setBlockPropBasedSettings = ({ @@ -1193,7 +1193,7 @@ export const getAllDiscourseNodes = (): DiscourseNode[] => { ); const retryResult = DiscourseNodeSchema.safeParse(migrated); if (retryResult.success) { - setBlockProps(pageUid, retryResult.data, false); + void setBlockProps(pageUid, retryResult.data, false); nodes.push( toDiscourseNode({ ...retryResult.data, diff --git a/apps/roam/src/components/settings/utils/init.ts b/apps/roam/src/components/settings/utils/init.ts index 9d6116766..5394ff5da 100644 --- a/apps/roam/src/components/settings/utils/init.ts +++ b/apps/roam/src/components/settings/utils/init.ts @@ -163,7 +163,7 @@ const initializeSettingsBlockProps = ( Object.keys(existingProps).length === 0 || !schema.safeParse(existingProps).success ) { - setBlockProps(uid, defaults, false); + void setBlockProps(uid, defaults, false); } // Reconcile placeholder relation keys with real block UIDs. @@ -261,7 +261,11 @@ const reconcileRelationKeys = ( } if (changed) { - setBlockProps(globalBlockUid, { Relations: reconciledRelations }, false); + void setBlockProps( + globalBlockUid, + { Relations: reconciledRelations }, + false, + ); } }; diff --git a/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts b/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts index da29fa11b..88c9c61f2 100644 --- a/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts +++ b/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts @@ -105,7 +105,7 @@ const migrateSection = ({ return true; } - setBlockProps(blockUid, parsedLegacy, false); + void setBlockProps(blockUid, parsedLegacy, false); onWrite?.(); console.log(`${LOG_PREFIX} ${label}: migrated`); return true; diff --git a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts index 3a0954bd6..602b37cb3 100644 --- a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts +++ b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts @@ -29,6 +29,7 @@ const setRoamAlphaApi = (): void => { block: { props: Record; uid: string }; }) => { propsByUid.set(block.uid, block.props); + return Promise.resolve(); }, ), }, @@ -75,7 +76,7 @@ describe("imported source identity metadata", () => { expect(readImportedSourceIdentity(PAGE_UID)).toBeUndefined(); }); - it("writes the source RID and modified time while preserving sibling metadata", () => { + it("writes the source RID and modified time while preserving sibling metadata", async () => { propsByUid.set(PAGE_UID, { [DISCOURSE_GRAPH_PROP_NAME]: { "relation-migration": { relationUid: 1718000000000 }, @@ -83,7 +84,7 @@ describe("imported source identity metadata", () => { "other-extension": { enabled: true }, }); - writeImportedSourceIdentity({ + await writeImportedSourceIdentity({ pageUid: PAGE_UID, sourceModifiedAt: SOURCE_MODIFIED_AT, sourceNodeRid: SOURCE_NODE_RID, diff --git a/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts b/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts deleted file mode 100644 index fd336ddd9..000000000 --- a/apps/roam/src/utils/__tests__/markdownToRoamBlocks.test.ts +++ /dev/null @@ -1,157 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { markdownToRoamBlocks } from "~/utils/markdownToRoamBlocks"; - -describe("markdownToRoamBlocks", () => { - it("strips YAML frontmatter and splits paragraphs on blank lines", () => { - expect( - markdownToRoamBlocks( - [ - "---", - "nodeTypeId: claim", - "nodeInstanceId: node-1", - "---", - "", - "First paragraph line one", - "line two", - "", - "Second paragraph", - ].join("\n"), - ), - ).toEqual([ - { text: "First paragraph line one\nline two", children: [] }, - { text: "Second paragraph", children: [] }, - ]); - }); - - it("nests content under headings by heading level", () => { - expect( - markdownToRoamBlocks( - ["# Top", "intro", "## Sub", "detail", "# Next", "tail"].join("\n"), - ), - ).toEqual([ - { - text: "Top", - heading: 1, - children: [ - { text: "intro", children: [] }, - { - text: "Sub", - heading: 2, - children: [{ text: "detail", children: [] }], - }, - ], - }, - { - text: "Next", - heading: 1, - children: [{ text: "tail", children: [] }], - }, - ]); - }); - - it("clamps headings deeper than Roam's maximum but keeps their nesting depth", () => { - expect( - markdownToRoamBlocks(["### Three", "#### Four", "text"].join("\n")), - ).toEqual([ - { - text: "Three", - heading: 3, - children: [ - { - text: "Four", - heading: 3, - children: [{ text: "text", children: [] }], - }, - ], - }, - ]); - }); - - it("nests list items by indentation and strips list markers", () => { - expect( - markdownToRoamBlocks( - [ - "- parent", - " - child", - " - grandchild", - "- sibling", - "1. ordered", - ].join("\n"), - ), - ).toEqual([ - { - text: "parent", - children: [ - { - text: "child", - children: [{ text: "grandchild", children: [] }], - }, - ], - }, - { text: "sibling", children: [] }, - { text: "ordered", children: [] }, - ]); - }); - - it("keeps a loose list nested after blank lines between items", () => { - expect( - markdownToRoamBlocks(["- parent", "", " - child"].join("\n")), - ).toEqual([ - { - text: "parent", - children: [{ text: "child", children: [] }], - }, - ]); - }); - - it("nests tab-indented list items", () => { - expect(markdownToRoamBlocks(["- parent", "\t- child"].join("\n"))).toEqual([ - { - text: "parent", - children: [{ text: "child", children: [] }], - }, - ]); - }); - - it("keeps a fenced code block as a single block, including blank lines", () => { - expect( - markdownToRoamBlocks( - ["```js", "const a = 1;", "", "const b = 2;", "```"].join("\n"), - ), - ).toEqual([ - { - text: "```js\nconst a = 1;\n\nconst b = 2;\n```", - children: [], - }, - ]); - }); - - it("keeps an unclosed fence as a single block through the end of input", () => { - expect(markdownToRoamBlocks(["```", "code", "more"].join("\n"))).toEqual([ - { text: "```\ncode\nmore", children: [] }, - ]); - }); - - it("returns no blocks for empty or frontmatter-only markdown", () => { - expect(markdownToRoamBlocks("")).toEqual([]); - expect( - markdownToRoamBlocks(["---", "nodeTypeId: claim", "---"].join("\n")), - ).toEqual([]); - }); - - it("treats an unclosed frontmatter delimiter as content", () => { - expect(markdownToRoamBlocks(["---", "not frontmatter"].join("\n"))).toEqual( - [{ text: "---\nnot frontmatter", children: [] }], - ); - }); - - it("handles CRLF line endings", () => { - expect(markdownToRoamBlocks("# Title\r\ntext\r\n")).toEqual([ - { - text: "Title", - heading: 1, - children: [{ text: "text", children: [] }], - }, - ]); - }); -}); diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index 7d1e2c4fd..9efde7528 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -2,8 +2,6 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; -import createBlock from "roamjs-components/writes/createBlock"; -import createPage from "roamjs-components/writes/createPage"; import deleteBlock from "roamjs-components/writes/deleteBlock"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; @@ -22,8 +20,6 @@ vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ vi.mock("roamjs-components/queries/getShallowTreeByParentUid", () => ({ default: vi.fn(), })); -vi.mock("roamjs-components/writes/createBlock", () => ({ default: vi.fn() })); -vi.mock("roamjs-components/writes/createPage", () => ({ default: vi.fn() })); vi.mock("roamjs-components/writes/deleteBlock", () => ({ default: vi.fn() })); vi.mock("~/utils/importedSourceIdentity", () => ({ findImportedNodeUidBySourceRid: vi.fn(), @@ -33,8 +29,6 @@ vi.mock("~/utils/importedSourceIdentity", () => ({ const mockedGetPageTitleByPageUid = vi.mocked(getPageTitleByPageUid); const mockedGetPageUidByPageTitle = vi.mocked(getPageUidByPageTitle); const mockedGetShallowTreeByParentUid = vi.mocked(getShallowTreeByParentUid); -const mockedCreateBlock = vi.mocked(createBlock); -const mockedCreatePage = vi.mocked(createPage); const mockedDeleteBlock = vi.mocked(deleteBlock); const mockedFindImportedNodeUidBySourceRid = vi.mocked( findImportedNodeUidBySourceRid, @@ -44,7 +38,12 @@ const mockedWriteImportedSourceIdentity = vi.mocked( ); const EXISTING_PAGE_UID = "existing-page-uid"; +const GENERATED_PAGE_UID = "generated-page-uid"; +const pageFromMarkdown = vi.fn(); +const blockFromMarkdown = vi.fn(); +const pageCreate = vi.fn(); +const pageDelete = vi.fn(); const updatePage = vi.fn(); const sharedNode: SharedNode = { @@ -70,28 +69,25 @@ const FULL_MARKDOWN = [ "REM sleep improves recall", ].join("\n"); -const FULL_TREE = [ - { - text: "Findings", - heading: 1, - children: [{ text: "REM sleep improves recall", children: [] }], - }, -]; +const MATERIALIZED_MARKDOWN = "# Findings\nREM sleep improves recall"; const clientWithFullContent = ({ text, + contentType = "text/obsidian+markdown", error, }: { text?: string | null; + contentType?: string | null; error?: { message: string }; }): { client: DGSupabaseClient; from: ReturnType } => { - const maybeSingle = vi - .fn() - .mockResolvedValue( - error - ? { data: null, error } - : { data: text === undefined ? null : { text }, error: null }, - ); + const maybeSingle = vi.fn().mockResolvedValue( + error + ? { data: null, error } + : { + data: text === undefined ? null : { text, content_type: contentType }, + error: null, + }, + ); const eq = vi.fn(); const chain = { eq, maybeSingle }; eq.mockReturnValue(chain); @@ -104,33 +100,75 @@ const clientWithFullContent = ({ beforeEach(() => { vi.clearAllMocks(); (globalThis as { window: unknown }).window = { - roamAlphaAPI: { updatePage }, + roamAlphaAPI: { + updatePage, + util: { generateUID: vi.fn(() => GENERATED_PAGE_UID) }, + data: { + block: { fromMarkdown: blockFromMarkdown }, + page: { + fromMarkdown: pageFromMarkdown, + create: pageCreate, + delete: pageDelete, + }, + }, + }, }; mockedGetShallowTreeByParentUid.mockReturnValue([]); + mockedGetPageUidByPageTitle.mockReturnValue(""); + mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); }); describe("materializeSharedNode", () => { - it("creates a Roam page with the parsed markdown and stores source identity", async () => { + it("creates a Roam page from the markdown body and stores source identity", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); - mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); - mockedGetPageUidByPageTitle.mockReturnValue(""); - mockedCreatePage.mockResolvedValue("new-page-uid"); await expect( materializeSharedNode({ client, sharedNode }), - ).resolves.toEqual({ status: "created", pageUid: "new-page-uid" }); - expect(mockedCreatePage).toHaveBeenCalledWith({ - title: sharedNode.title, - tree: FULL_TREE, + ).resolves.toEqual({ + success: true, + action: "created", + pageUid: GENERATED_PAGE_UID, + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }); + expect(pageFromMarkdown).toHaveBeenCalledWith({ + page: { title: sharedNode.title, uid: GENERATED_PAGE_UID }, + "markdown-string": MATERIALIZED_MARKDOWN, }); expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ - pageUid: "new-page-uid", + pageUid: GENERATED_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, }); }); - it("updates the existing imported page instead of creating a duplicate", async () => { + it("stores the source modified time as canonical UTC", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + + const result = await materializeSharedNode({ + client, + sharedNode: { ...sharedNode, lastModified: "2026-06-14T17:00:00+02:00" }, + }); + + expect(result.sourceModifiedAt).toBe("2026-06-14T15:00:00.000Z"); + expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith( + expect.objectContaining({ sourceModifiedAt: "2026-06-14T15:00:00.000Z" }), + ); + }); + + it("creates a title-only page when the node has no full content", async () => { + const { client } = clientWithFullContent({}); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.success).toBe(true); + expect(pageCreate).toHaveBeenCalledWith({ + page: { title: sharedNode.title, uid: GENERATED_PAGE_UID }, + }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); + }); + + it("replaces the existing imported page instead of creating a duplicate", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); mockedGetPageTitleByPageUid.mockReturnValue(sharedNode.title); @@ -140,52 +178,54 @@ describe("materializeSharedNode", () => { await expect( materializeSharedNode({ client, sharedNode }), - ).resolves.toEqual({ status: "updated", pageUid: EXISTING_PAGE_UID }); - expect(mockedCreatePage).not.toHaveBeenCalled(); - expect(updatePage).not.toHaveBeenCalled(); - expect(mockedDeleteBlock).toHaveBeenCalledWith("old-block"); - expect(mockedCreateBlock).toHaveBeenCalledWith({ - parentUid: EXISTING_PAGE_UID, - order: 0, - node: FULL_TREE[0], - }); - expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ + ).resolves.toEqual({ + success: true, + action: "updated", pageUid: EXISTING_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); + expect(updatePage).not.toHaveBeenCalled(); + expect(blockFromMarkdown).toHaveBeenCalledWith({ + location: { "parent-uid": EXISTING_PAGE_UID, order: "last" }, + "markdown-string": MATERIALIZED_MARKDOWN, + }); + expect(mockedDeleteBlock).toHaveBeenCalledWith("old-block"); + expect(blockFromMarkdown.mock.invocationCallOrder[0]).toBeLessThan( + mockedDeleteBlock.mock.invocationCallOrder[0], + ); }); it("renames the imported page when the source title changed", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); mockedGetPageTitleByPageUid.mockReturnValue("EVD - old title"); - mockedGetPageUidByPageTitle.mockReturnValue(""); - await expect( - materializeSharedNode({ client, sharedNode }), - ).resolves.toEqual({ status: "updated", pageUid: EXISTING_PAGE_UID }); + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.success).toBe(true); expect(updatePage).toHaveBeenCalledWith({ page: { uid: EXISTING_PAGE_UID, title: sharedNode.title }, }); }); - it("fails without writing when a local page already uses the title", async () => { + it("refuses to clobber a page that was not imported from this source", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); - mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); const result = await materializeSharedNode({ client, sharedNode }); - expect(result.status).toBe("failed"); - expect(result.status === "failed" && result.reason).toContain( - sharedNode.title, - ); - expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(result).toMatchObject({ + success: false, + sourceNodeRid: sharedNode.rid, + error: { stage: "title-collision" }, + }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); }); - it("fails the rename without touching content when the new title collides", async () => { + it("fails the rename before touching content when the new title collides", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); mockedGetPageTitleByPageUid.mockReturnValue("EVD - old title"); @@ -193,13 +233,18 @@ describe("materializeSharedNode", () => { const result = await materializeSharedNode({ client, sharedNode }); - expect(result.status).toBe("failed"); - expect(updatePage).not.toHaveBeenCalled(); + expect(result).toMatchObject({ + success: false, + pageUid: EXISTING_PAGE_UID, + error: { stage: "title-collision" }, + }); + expect(blockFromMarkdown).not.toHaveBeenCalled(); expect(mockedDeleteBlock).not.toHaveBeenCalled(); + expect(updatePage).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); }); - it("rejects nodes that are not Obsidian-origin before fetching content", async () => { + it("rejects a non-Obsidian source before fetching content", async () => { const { client, from } = clientWithFullContent({ text: FULL_MARKDOWN }); const result = await materializeSharedNode({ @@ -207,56 +252,143 @@ describe("materializeSharedNode", () => { sharedNode: { ...sharedNode, platform: "Roam" }, }); - expect(result.status).toBe("failed"); - expect(result.status === "failed" && result.reason).toContain("Roam"); + expect(result).toMatchObject({ + success: false, + error: { stage: "validate-input" }, + }); + expect(result.success === false && result.error.message).toContain("Roam"); expect(from).not.toHaveBeenCalled(); }); - it("fails with the fetch error and keeps identity untouched", async () => { + it("rejects a source identifier that is not a RID", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + + const result = await materializeSharedNode({ + client, + sharedNode: { ...sharedNode, rid: "not-a-rid" }, + }); + + expect(result).toMatchObject({ + success: false, + sourceNodeRid: "not-a-rid", + error: { stage: "validate-input" }, + }); + }); + + it("rejects an invalid source modified time", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + + const result = await materializeSharedNode({ + client, + sharedNode: { ...sharedNode, lastModified: "not-a-date" }, + }); + + expect(result).toMatchObject({ + success: false, + error: { stage: "validate-input" }, + }); + }); + + it("rejects a full content type Roam cannot materialize", async () => { + const { client } = clientWithFullContent({ + text: `# ${sharedNode.title}\n\nbody`, + contentType: "text/markdown", + }); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result).toMatchObject({ + success: false, + error: { stage: "fetch-content" }, + }); + expect(result.success === false && result.error.message).toContain( + "text/markdown", + ); + expect(pageFromMarkdown).not.toHaveBeenCalled(); + }); + + it("fails with the fetch error and keeps identity in the result", async () => { const { client } = clientWithFullContent({ error: { message: "permission denied" }, }); - mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); const result = await materializeSharedNode({ client, sharedNode }); - expect(result.status).toBe("failed"); - expect(result.status === "failed" && result.reason).toContain( + expect(result).toMatchObject({ + success: false, + sourceNodeRid: sharedNode.rid, + error: { stage: "fetch-content" }, + }); + expect(result.success === false && result.error.message).toContain( "permission denied", ); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); }); - it("materializes a title-only page when the node has no full content", async () => { - const { client } = clientWithFullContent({}); - mockedFindImportedNodeUidBySourceRid.mockResolvedValue(null); - mockedGetPageUidByPageTitle.mockReturnValue(""); - mockedCreatePage.mockResolvedValue("new-page-uid"); + it("removes a new page when its source identity cannot be stored", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedWriteImportedSourceIdentity.mockRejectedValue( + new Error("props write failed"), + ); - await expect( - materializeSharedNode({ client, sharedNode }), - ).resolves.toEqual({ status: "created", pageUid: "new-page-uid" }); - expect(mockedCreatePage).toHaveBeenCalledWith({ - title: sharedNode.title, - tree: [], + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result).toMatchObject({ + success: false, + error: { stage: "write-source-identity" }, + }); + expect(pageDelete).toHaveBeenCalledWith({ + page: { uid: GENERATED_PAGE_UID }, + }); + expect(result.success === false && result.pageUid).toBeUndefined(); + }); + + it("reports the orphaned page uid when cleanup also fails", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedWriteImportedSourceIdentity.mockRejectedValue( + new Error("props write failed"), + ); + pageDelete.mockRejectedValue(new Error("delete failed")); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result).toMatchObject({ + success: false, + pageUid: GENERATED_PAGE_UID, + error: { stage: "write-source-identity" }, }); }); - it("reports the source RID when a write fails partway", async () => { + it("keeps the updated page when refreshing identity fails on re-import", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); mockedGetPageTitleByPageUid.mockReturnValue(sharedNode.title); - mockedCreateBlock.mockRejectedValue(new Error("block write failed")); + mockedWriteImportedSourceIdentity.mockRejectedValue( + new Error("props write failed"), + ); const result = await materializeSharedNode({ client, sharedNode }); - expect(result.status).toBe("failed"); - expect(result.status === "failed" && result.reason).toContain( - sharedNode.rid, - ); - expect(result.status === "failed" && result.reason).toContain( - "block write failed", + expect(result).toMatchObject({ + success: false, + pageUid: EXISTING_PAGE_UID, + error: { stage: "write-source-identity" }, + }); + expect(pageDelete).not.toHaveBeenCalled(); + }); + + it("reports a failed lookup of the existing import", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedFindImportedNodeUidBySourceRid.mockRejectedValue( + new Error("datalog query failed"), ); - expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result).toMatchObject({ + success: false, + error: { stage: "find-imported-node" }, + }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); }); }); diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index 17f16eb10..3f810adfd 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -37,7 +37,7 @@ export const readImportedSourceIdentity = ( ): ImportedSourceIdentity | undefined => parseImportedSourceIdentity(getBlockProps(pageUid)); -export const writeImportedSourceIdentity = ({ +export const writeImportedSourceIdentity = async ({ pageUid, sourceModifiedAt, sourceNodeRid, @@ -45,11 +45,11 @@ export const writeImportedSourceIdentity = ({ pageUid: string; sourceModifiedAt: string; sourceNodeRid: string; -}): void => { +}): Promise => { const existing = getBlockProps(pageUid)[DISCOURSE_GRAPH_PROP_NAME]; const discourseGraphProps = isJsonObject(existing) ? existing : {}; - setBlockProps(pageUid, { + await setBlockProps(pageUid, { [DISCOURSE_GRAPH_PROP_NAME]: { ...discourseGraphProps, [IMPORTED_FROM_PROP_KEY]: { diff --git a/apps/roam/src/utils/markdownToRoamBlocks.ts b/apps/roam/src/utils/markdownToRoamBlocks.ts deleted file mode 100644 index d6b12b534..000000000 --- a/apps/roam/src/utils/markdownToRoamBlocks.ts +++ /dev/null @@ -1,111 +0,0 @@ -import { normalizeLineEndings } from "@repo/content-model"; -import type { InputTextNode } from "roamjs-components/types/native"; - -type BlockNode = InputTextNode & { children: InputTextNode[] }; - -const FRONTMATTER_DELIMITER = /^---\s*$/; -const HEADING = /^(#{1,6})\s+(.+)$/; -const LIST_ITEM = /^([ \t]*)(?:[-*+]|\d+[.)])[ \t]+(.+)$/; -const ROAM_MAX_HEADING_LEVEL = 3; - -const stripFrontmatter = (lines: string[]): string[] => { - if (lines.length === 0 || !FRONTMATTER_DELIMITER.test(lines[0])) return lines; - const closingIndex = lines.findIndex( - (line, index) => index > 0 && FRONTMATTER_DELIMITER.test(line), - ); - return closingIndex === -1 ? lines : lines.slice(closingIndex + 1); -}; - -const indentWidth = (whitespace: string): number => - [...whitespace].reduce((width, char) => width + (char === "\t" ? 4 : 1), 0); - -export const markdownToRoamBlocks = (markdown: string): InputTextNode[] => { - const lines = stripFrontmatter(normalizeLineEndings(markdown).split("\n")); - const roots: InputTextNode[] = []; - const headingStack: { level: number; node: BlockNode }[] = []; - let listStack: { indent: number; node: BlockNode }[] = []; - let paragraphLines: string[] = []; - - const sectionBlocks = (): InputTextNode[] => - headingStack.length - ? headingStack[headingStack.length - 1].node.children - : roots; - - const flushParagraph = (): void => { - if (!paragraphLines.length) return; - sectionBlocks().push({ text: paragraphLines.join("\n"), children: [] }); - paragraphLines = []; - }; - - for (let index = 0; index < lines.length; index++) { - const line = lines[index]; - - if (line.startsWith("```") || line.startsWith("~~~")) { - flushParagraph(); - listStack = []; - const fence = line.slice(0, 3); - const fenceLines = [line]; - while (index + 1 < lines.length && !lines[index + 1].startsWith(fence)) { - index++; - fenceLines.push(lines[index]); - } - if (index + 1 < lines.length) { - index++; - fenceLines.push(lines[index]); - } - sectionBlocks().push({ text: fenceLines.join("\n"), children: [] }); - continue; - } - - const headingMatch = HEADING.exec(line); - if (headingMatch) { - flushParagraph(); - listStack = []; - const level = headingMatch[1].length; - while ( - headingStack.length && - headingStack[headingStack.length - 1].level >= level - ) { - headingStack.pop(); - } - const node: BlockNode = { - text: headingMatch[2].trimEnd(), - heading: Math.min(level, ROAM_MAX_HEADING_LEVEL), - children: [], - }; - sectionBlocks().push(node); - headingStack.push({ level, node }); - continue; - } - - const listMatch = LIST_ITEM.exec(line); - if (listMatch) { - flushParagraph(); - const indent = indentWidth(listMatch[1]); - while ( - listStack.length && - listStack[listStack.length - 1].indent >= indent - ) { - listStack.pop(); - } - const node: BlockNode = { text: listMatch[2].trimEnd(), children: [] }; - const siblings = listStack.length - ? listStack[listStack.length - 1].node.children - : sectionBlocks(); - siblings.push(node); - listStack.push({ indent, node }); - continue; - } - - if (!line.trim()) { - flushParagraph(); - continue; - } - - listStack = []; - paragraphLines.push(line.trimEnd()); - } - - flushParagraph(); - return roots; -}; diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index d16138fff..d23c545f1 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -1,21 +1,115 @@ +import { contentTypes, stripFrontmatter } from "@repo/content-model"; import type { DGSupabaseClient } from "@repo/database/lib/client"; +import { isRid } from "@repo/database/lib/rid"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; -import type { InputTextNode } from "roamjs-components/types/native"; import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; -import createBlock from "roamjs-components/writes/createBlock"; -import createPage from "roamjs-components/writes/createPage"; import deleteBlock from "roamjs-components/writes/deleteBlock"; import { findImportedNodeUidBySourceRid, writeImportedSourceIdentity, } from "./importedSourceIdentity"; -import { markdownToRoamBlocks } from "./markdownToRoamBlocks"; + +type MaterializationStage = + | "validate-input" + | "fetch-content" + | "find-imported-node" + | "title-collision" + | "create-page" + | "replace-page-content" + | "update-page-title" + | "write-source-identity"; + +type SourceIdentity = { + sourceModifiedAt: string; + sourceNodeRid: string; +}; + +type MaterializationFailure = SourceIdentity & { + success: false; + pageUid?: string; + error: { + message: string; + stage: MaterializationStage; + }; +}; + +type MaterializationSuccess = SourceIdentity & { + success: true; + action: "created" | "updated"; + pageUid: string; +}; export type MaterializeSharedNodeResult = - | { status: "created" | "updated"; pageUid: string } - | { status: "failed"; reason: string }; + | MaterializationFailure + | MaterializationSuccess; + +type RoamMarkdownApi = { + block: { + fromMarkdown: (args: { + location: { "parent-uid": string; order: "last" }; + "markdown-string": string; + }) => Promise; + }; + page: { + fromMarkdown: (args: { + page: { title: string; uid: string }; + "markdown-string": string; + }) => Promise; + }; +}; + +const getRoamMarkdownApi = (): RoamMarkdownApi => + window.roamAlphaAPI.data as unknown as RoamMarkdownApi; + +const getErrorMessage = (error: unknown): string => + error instanceof Error ? error.message : String(error); + +const failure = ({ + error, + identity, + message, + pageUid, + stage, +}: { + error?: unknown; + identity: SourceIdentity; + message: string; + pageUid?: string; + stage: MaterializationStage; +}): MaterializationFailure => ({ + ...identity, + success: false, + ...(pageUid ? { pageUid } : {}), + error: { + message: error ? `${message}: ${getErrorMessage(error)}` : message, + stage, + }, +}); + +const validateSharedNode = ( + sharedNode: SharedNode, +): { error: string } | { sourceModifiedAt: string; title: string } => { + if (sharedNode.platform !== "Obsidian") + return { + error: `Materialization only supports Obsidian-origin nodes, and this node comes from ${sharedNode.platform}`, + }; + + if (!isRid(sharedNode.rid)) + return { error: `Source node RID "${sharedNode.rid}" is not a RID` }; + + const modifiedAt = new Date(sharedNode.lastModified); + if (Number.isNaN(modifiedAt.getTime())) + return { + error: `Source modified time "${sharedNode.lastModified}" is invalid`, + }; + + const title = sharedNode.title.trim(); + if (!title) return { error: "Source node title is required" }; + + return { sourceModifiedAt: modifiedAt.toISOString(), title }; +}; const fetchFullMarkdown = async ({ client, @@ -26,72 +120,148 @@ const fetchFullMarkdown = async ({ }): Promise<{ markdown: string } | { error: string }> => { const { data, error } = await client .from("my_contents") - .select("text") + .select("text, content_type") .eq("space_id", sharedNode.spaceId) .eq("source_local_id", sharedNode.sourceLocalId) .eq("variant", "full") .maybeSingle(); if (error) return { error: error.message }; - return { markdown: data?.text ?? "" }; + if (!data?.text) return { markdown: "" }; + if (data.content_type !== contentTypes.obsidianMarkdown) + return { + error: `Unsupported full content type "${data.content_type}" — expected "${contentTypes.obsidianMarkdown}"`, + }; + return { markdown: stripFrontmatter(data.text).trim() }; }; const createImportedPage = async ({ - sharedNode, - tree, + identity, + markdown, + title, }: { - sharedNode: SharedNode; - tree: InputTextNode[]; + identity: SourceIdentity; + markdown: string; + title: string; }): Promise => { - const collidingPageUid = getPageUidByPageTitle(sharedNode.title); - if (collidingPageUid) { - return { - status: "failed", - reason: `A page titled "${sharedNode.title}" already exists in this graph but was not imported from "${sharedNode.spaceName}". Rename or remove that page, then import again.`, - }; + if (getPageUidByPageTitle(title)) + return failure({ + identity, + message: `A page titled "${title}" already exists and was not imported from "${identity.sourceNodeRid}". Rename or remove that page, then import again`, + stage: "title-collision", + }); + + const pageUid = window.roamAlphaAPI.util.generateUID(); + try { + if (markdown) { + await getRoamMarkdownApi().page.fromMarkdown({ + page: { title, uid: pageUid }, + "markdown-string": markdown, + }); + } else { + await window.roamAlphaAPI.data.page.create({ + page: { title, uid: pageUid }, + }); + } + } catch (error) { + return failure({ + error, + identity, + message: `Failed to create a Roam page for "${title}"`, + stage: "create-page", + }); } - const pageUid = await createPage({ title: sharedNode.title, tree }); - writeImportedSourceIdentity({ - pageUid, - sourceModifiedAt: sharedNode.lastModified, - sourceNodeRid: sharedNode.rid, - }); - return { status: "created", pageUid }; + + try { + await writeImportedSourceIdentity({ pageUid, ...identity }); + } catch (error) { + let cleanupError: unknown; + try { + await window.roamAlphaAPI.data.page.delete({ page: { uid: pageUid } }); + } catch (caughtCleanupError) { + cleanupError = caughtCleanupError; + } + + return failure({ + error, + identity, + message: cleanupError + ? `Created "${title}" but could not store its source identity, and removing the page failed too (${getErrorMessage(cleanupError)}) — delete the page manually before re-importing` + : `Created "${title}" and removed it again because its source identity could not be stored`, + ...(cleanupError ? { pageUid } : {}), + stage: "write-source-identity", + }); + } + + return { ...identity, success: true, action: "created", pageUid }; }; const updateImportedPage = async ({ + identity, + markdown, pageUid, - sharedNode, - tree, + title, }: { + identity: SourceIdentity; + markdown: string; pageUid: string; - sharedNode: SharedNode; - tree: InputTextNode[]; + title: string; }): Promise => { const localTitle = getPageTitleByPageUid(pageUid); - if (localTitle !== sharedNode.title) { - const collidingPageUid = getPageUidByPageTitle(sharedNode.title); - if (collidingPageUid) { - return { - status: "failed", - reason: `Cannot rename the imported page "${localTitle}" to "${sharedNode.title}": another page already has that title. Rename or remove that page, then import again.`, - }; + const needsRename = localTitle !== title; + if (needsRename && getPageUidByPageTitle(title)) + return failure({ + identity, + message: `Cannot rename the imported page "${localTitle}" to "${title}": another page already has that title. Rename or remove that page, then import again`, + pageUid, + stage: "title-collision", + }); + + try { + const previousChildren = getShallowTreeByParentUid(pageUid); + if (markdown) { + await getRoamMarkdownApi().block.fromMarkdown({ + location: { "parent-uid": pageUid, order: "last" }, + "markdown-string": markdown, + }); } - await window.roamAlphaAPI.updatePage({ - page: { uid: pageUid, title: sharedNode.title }, + await Promise.all(previousChildren.map(({ uid }) => deleteBlock(uid))); + } catch (error) { + return failure({ + error, + identity, + message: `Failed to replace the content of "${localTitle}"`, + pageUid, + stage: "replace-page-content", }); } - for (const { uid } of getShallowTreeByParentUid(pageUid)) { - await deleteBlock(uid); + + if (needsRename) { + try { + await window.roamAlphaAPI.updatePage({ page: { uid: pageUid, title } }); + } catch (error) { + return failure({ + error, + identity, + message: `Content of "${localTitle}" was replaced, but the page could not be renamed to "${title}"`, + pageUid, + stage: "update-page-title", + }); + } } - for (const [order, node] of tree.entries()) { - await createBlock({ parentUid: pageUid, order, node }); + + try { + await writeImportedSourceIdentity({ pageUid, ...identity }); + } catch (error) { + return failure({ + error, + identity, + message: `Content of "${title}" was replaced, but its source identity could not be refreshed`, + pageUid, + stage: "write-source-identity", + }); } - writeImportedSourceIdentity({ - pageUid, - sourceModifiedAt: sharedNode.lastModified, - sourceNodeRid: sharedNode.rid, - }); - return { status: "updated", pageUid }; + + return { ...identity, success: true, action: "updated", pageUid }; }; export const materializeSharedNode = async ({ @@ -101,33 +271,56 @@ export const materializeSharedNode = async ({ client: DGSupabaseClient; sharedNode: SharedNode; }): Promise => { - if (sharedNode.platform !== "Obsidian") { - return { - status: "failed", - reason: `Cannot import "${sharedNode.title}": materialization only supports Obsidian-origin nodes, and this node comes from ${sharedNode.platform}.`, - }; - } + const rawIdentity: SourceIdentity = { + sourceModifiedAt: sharedNode.lastModified, + sourceNodeRid: sharedNode.rid, + }; + + const validated = validateSharedNode(sharedNode); + if ("error" in validated) + return failure({ + identity: rawIdentity, + message: validated.error, + stage: "validate-input", + }); + + const identity: SourceIdentity = { + sourceModifiedAt: validated.sourceModifiedAt, + sourceNodeRid: sharedNode.rid, + }; + + const content = await fetchFullMarkdown({ client, sharedNode }).catch( + (error: unknown) => ({ error: getErrorMessage(error) }), + ); + if ("error" in content) + return failure({ + identity, + message: `Could not fetch the content of "${sharedNode.title}" from "${sharedNode.spaceName}": ${content.error}`, + stage: "fetch-content", + }); + + let importedPageUid: string | null; try { - const content = await fetchFullMarkdown({ client, sharedNode }); - if ("error" in content) { - return { - status: "failed", - reason: `Could not fetch the content of "${sharedNode.title}" from "${sharedNode.spaceName}": ${content.error}`, - }; - } - const tree = markdownToRoamBlocks(content.markdown); - const importedPageUid = await findImportedNodeUidBySourceRid( - sharedNode.rid, - ); - return importedPageUid - ? await updateImportedPage({ pageUid: importedPageUid, sharedNode, tree }) - : await createImportedPage({ sharedNode, tree }); + importedPageUid = await findImportedNodeUidBySourceRid(sharedNode.rid); } catch (error) { - return { - status: "failed", - reason: `Materializing "${sharedNode.title}" (${sharedNode.rid}) failed: ${ - error instanceof Error ? error.message : String(error) - }`, - }; + return failure({ + error, + identity, + message: `Failed to look up an existing import of "${sharedNode.title}"`, + stage: "find-imported-node", + }); } + + return importedPageUid + ? updateImportedPage({ + identity, + markdown: content.markdown, + pageUid: importedPageUid, + title: validated.title, + }) + : createImportedPage({ + identity, + markdown: content.markdown, + title: validated.title, + }); }; diff --git a/apps/roam/src/utils/migrateRelations.ts b/apps/roam/src/utils/migrateRelations.ts index 14f103671..30cb90655 100644 --- a/apps/roam/src/utils/migrateRelations.ts +++ b/apps/roam/src/utils/migrateRelations.ts @@ -55,7 +55,7 @@ const migrateRelations = async (): Promise => { ); migrationData[uid] = new Date().valueOf(); dgData[MIGRATION_PROP_NAME] = migrationData; - setBlockProps(rel.source, { [DISCOURSE_GRAPH_PROP_NAME]: dgData }); + void setBlockProps(rel.source, { [DISCOURSE_GRAPH_PROP_NAME]: dgData }); numProcessed++; } } catch (error) { diff --git a/apps/roam/src/utils/setBlockProps.ts b/apps/roam/src/utils/setBlockProps.ts index 1f8d9d52d..93dacd6be 100644 --- a/apps/roam/src/utils/setBlockProps.ts +++ b/apps/roam/src/utils/setBlockProps.ts @@ -23,7 +23,7 @@ const setBlockProps = ( uid: string, newProps: Record, denormalize: boolean = false, -) => { +): Promise => { const rawBaseProps = getRawBlockProps(uid); const baseProps = denormalize ? rawBaseProps : normalizeProps(rawBaseProps); if (typeof baseProps === "object" && !Array.isArray(baseProps)) { @@ -33,10 +33,11 @@ const setBlockProps = ( ? (deNormalizeProps(newProps) as Record) : newProps), } as Record; - window.roamAlphaAPI.data.block.update({ block: { uid, props } }); - return props; + return window.roamAlphaAPI.data.block + .update({ block: { uid, props } }) + .then(() => props); } - return baseProps; + return Promise.resolve(baseProps); }; export const testSetBlockProps = ( diff --git a/apps/roam/src/utils/supabaseContext.ts b/apps/roam/src/utils/supabaseContext.ts index 5c9d8fef0..4146d914c 100644 --- a/apps/roam/src/utils/supabaseContext.ts +++ b/apps/roam/src/utils/supabaseContext.ts @@ -38,7 +38,7 @@ const getOrCreateSpacePassword = () => { if (existing && typeof existing === "string") return existing; // use a uuid as password, at least cryptographically safe const password = crypto.randomUUID(); - setBlockProps(settingsConfigPageUid, { + void setBlockProps(settingsConfigPageUid, { "space-user-password": password, }); return password; diff --git a/packages/content-model/src/__tests__/text.test.ts b/packages/content-model/src/__tests__/text.test.ts new file mode 100644 index 000000000..c7c913d35 --- /dev/null +++ b/packages/content-model/src/__tests__/text.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import { normalizeLineEndings, stripFrontmatter } from "../text/index.js"; + +describe("normalizeLineEndings", () => { + it("converts CRLF and lone CR to LF", () => { + expect(normalizeLineEndings("a\r\nb\rc\n")).toBe("a\nb\nc\n"); + }); +}); + +describe("stripFrontmatter", () => { + it("removes a leading YAML frontmatter block and following blank lines", () => { + expect( + stripFrontmatter( + [ + "---", + "nodeTypeId: claim", + "nodeInstanceId: node-1", + "---", + "", + "# Body", + ].join("\n"), + ), + ).toBe("# Body"); + }); + + it("handles CRLF frontmatter", () => { + expect(stripFrontmatter("---\r\nkey: value\r\n---\r\nBody")).toBe("Body"); + }); + + it("returns markdown unchanged when there is no frontmatter", () => { + expect(stripFrontmatter("# Body\ntext")).toBe("# Body\ntext"); + }); + + it("treats a delimiter that never closes as content", () => { + expect(stripFrontmatter("---\nnot frontmatter")).toBe( + "---\nnot frontmatter", + ); + }); + + it("does not treat a mid-document rule as frontmatter", () => { + expect(stripFrontmatter("intro\n---\noutro")).toBe("intro\n---\noutro"); + }); + + it("returns an empty string for frontmatter-only markdown", () => { + expect(stripFrontmatter("---\nkey: value\n---\n")).toBe(""); + }); +}); diff --git a/packages/content-model/src/text/index.ts b/packages/content-model/src/text/index.ts index eed1bf8c7..0671808ed 100644 --- a/packages/content-model/src/text/index.ts +++ b/packages/content-model/src/text/index.ts @@ -1,2 +1,20 @@ export const normalizeLineEndings = (text: string): string => text.replace(/\r\n?/g, "\n"); + +const FRONTMATTER_DELIMITER = "---"; + +export const stripFrontmatter = (markdown: string): string => { + const normalized = normalizeLineEndings(markdown); + if (!normalized.startsWith(`${FRONTMATTER_DELIMITER}\n`)) return normalized; + + const lines = normalized.split("\n"); + const closingIndex = lines.findIndex( + (line, index) => index > 0 && line.trimEnd() === FRONTMATTER_DELIMITER, + ); + if (closingIndex === -1) return normalized; + + return lines + .slice(closingIndex + 1) + .join("\n") + .replace(/^\n+/, ""); +}; From 9e55349910c5caddca73ebacb3b0e3c8181f273c Mon Sep 17 00:00:00 2001 From: sid597 Date: Wed, 29 Jul 2026 12:57:03 +0530 Subject: [PATCH 3/5] [ENG-1858] Fetch only the original full content row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Content schema allows derived rows per (space_id, source_local_id, variant) — non-original rows carry original = NULL, so only (…, variant, original) is unique. Without the original filter a derived full representation makes maybeSingle() fail with a multiple-rows error and a valid node cannot be materialized. From Codex review on #1267. --- .../src/utils/__tests__/materializeSharedNode.test.ts | 11 ++++++++--- apps/roam/src/utils/materializeSharedNode.ts | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index 9efde7528..16eb09fa6 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -79,7 +79,11 @@ const clientWithFullContent = ({ text?: string | null; contentType?: string | null; error?: { message: string }; -}): { client: DGSupabaseClient; from: ReturnType } => { +}): { + client: DGSupabaseClient; + eq: ReturnType; + from: ReturnType; +} => { const maybeSingle = vi.fn().mockResolvedValue( error ? { data: null, error } @@ -94,7 +98,7 @@ const clientWithFullContent = ({ const from = vi .fn() .mockReturnValue({ select: vi.fn().mockReturnValue(chain) }); - return { client: { from } as unknown as DGSupabaseClient, from }; + return { client: { from } as unknown as DGSupabaseClient, eq, from }; }; beforeEach(() => { @@ -120,7 +124,7 @@ beforeEach(() => { describe("materializeSharedNode", () => { it("creates a Roam page from the markdown body and stores source identity", async () => { - const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + const { client, eq } = clientWithFullContent({ text: FULL_MARKDOWN }); await expect( materializeSharedNode({ client, sharedNode }), @@ -131,6 +135,7 @@ describe("materializeSharedNode", () => { sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, }); + expect(eq).toHaveBeenCalledWith("original", true); expect(pageFromMarkdown).toHaveBeenCalledWith({ page: { title: sharedNode.title, uid: GENERATED_PAGE_UID }, "markdown-string": MATERIALIZED_MARKDOWN, diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index d23c545f1..c12c6fbd5 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -124,6 +124,7 @@ const fetchFullMarkdown = async ({ .eq("space_id", sharedNode.spaceId) .eq("source_local_id", sharedNode.sourceLocalId) .eq("variant", "full") + .eq("original", true) .maybeSingle(); if (error) return { error: error.message }; if (!data?.text) return { markdown: "" }; From f6dc8f8fabf517aec835f28b2f35813b26c12bfb Mon Sep 17 00:00:00 2001 From: sid597 Date: Wed, 29 Jul 2026 13:18:06 +0530 Subject: [PATCH 4/5] [ENG-1858] Keep leading indentation when trimming the markdown body trim() on the stripped body also removed the first line's indentation, so a body starting with an indented code block degraded to plain text in Roam. trimBlankLines (content-model) strips only surrounding blank lines; a whitespace-only body still collapses to the title-only path. From Codex review on #1267. --- .../__tests__/materializeSharedNode.test.ts | 30 +++++++++++++++++++ apps/roam/src/utils/materializeSharedNode.ts | 9 ++++-- .../content-model/src/__tests__/text.test.ts | 18 ++++++++++- packages/content-model/src/text/index.ts | 3 ++ 4 files changed, 57 insertions(+), 3 deletions(-) diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index 16eb09fa6..eb562462a 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -161,6 +161,36 @@ describe("materializeSharedNode", () => { ); }); + it("preserves the indentation of a leading indented code block", async () => { + const { client } = clientWithFullContent({ + text: ["---", "nodeTypeId: evidence", "---", "", " const a = 1;"].join( + "\n", + ), + }); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.success).toBe(true); + expect(pageFromMarkdown).toHaveBeenCalledWith({ + page: { title: sharedNode.title, uid: GENERATED_PAGE_UID }, + "markdown-string": " const a = 1;", + }); + }); + + it("creates a title-only page when the body is only whitespace", async () => { + const { client } = clientWithFullContent({ + text: ["---", "nodeTypeId: evidence", "---", "", " ", ""].join("\n"), + }); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result.success).toBe(true); + expect(pageCreate).toHaveBeenCalledWith({ + page: { title: sharedNode.title, uid: GENERATED_PAGE_UID }, + }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); + }); + it("creates a title-only page when the node has no full content", async () => { const { client } = clientWithFullContent({}); diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index c12c6fbd5..5646db8d1 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -1,4 +1,8 @@ -import { contentTypes, stripFrontmatter } from "@repo/content-model"; +import { + contentTypes, + stripFrontmatter, + trimBlankLines, +} from "@repo/content-model"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import { isRid } from "@repo/database/lib/rid"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; @@ -132,7 +136,8 @@ const fetchFullMarkdown = async ({ return { error: `Unsupported full content type "${data.content_type}" — expected "${contentTypes.obsidianMarkdown}"`, }; - return { markdown: stripFrontmatter(data.text).trim() }; + const markdown = trimBlankLines(stripFrontmatter(data.text)); + return { markdown: markdown.trim() ? markdown : "" }; }; const createImportedPage = async ({ diff --git a/packages/content-model/src/__tests__/text.test.ts b/packages/content-model/src/__tests__/text.test.ts index c7c913d35..ed05450ea 100644 --- a/packages/content-model/src/__tests__/text.test.ts +++ b/packages/content-model/src/__tests__/text.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "vitest"; -import { normalizeLineEndings, stripFrontmatter } from "../text/index.js"; +import { + normalizeLineEndings, + stripFrontmatter, + trimBlankLines, +} from "../text/index.js"; describe("normalizeLineEndings", () => { it("converts CRLF and lone CR to LF", () => { @@ -7,6 +11,18 @@ describe("normalizeLineEndings", () => { }); }); +describe("trimBlankLines", () => { + it("removes surrounding blank lines but keeps line indentation", () => { + expect(trimBlankLines("\n \n code\n text\n\n \n")).toBe( + " code\n text", + ); + }); + + it("leaves a whitespace-only single line for the caller to judge", () => { + expect(trimBlankLines(" ")).toBe(" "); + }); +}); + describe("stripFrontmatter", () => { it("removes a leading YAML frontmatter block and following blank lines", () => { expect( diff --git a/packages/content-model/src/text/index.ts b/packages/content-model/src/text/index.ts index 0671808ed..50f1d7281 100644 --- a/packages/content-model/src/text/index.ts +++ b/packages/content-model/src/text/index.ts @@ -1,6 +1,9 @@ export const normalizeLineEndings = (text: string): string => text.replace(/\r\n?/g, "\n"); +export const trimBlankLines = (text: string): string => + text.replace(/^(?:[ \t]*\n)+/, "").replace(/(?:\n[ \t]*)+$/, ""); + const FRONTMATTER_DELIMITER = "---"; export const stripFrontmatter = (markdown: string): string => { From 2d3e6906f37ff130ff6ae1cde8e08ce446931f1c Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 30 Jul 2026 11:06:08 +0530 Subject: [PATCH 5/5] [ENG-1858] Confine the awaitable setBlockProps change to the importer Only writeImportedSourceIdentity needs to observe the props-write result (materializeSharedNode deletes the created page when the identity write fails), so expose that as setBlockPropsAsync and keep the default export fire-and-forget. This reverts the signature change that forced void at eight call sites unrelated to this feature. --- apps/roam/src/components/canvas/Clipboard.tsx | 2 +- apps/roam/src/components/canvas/canvasSyncMode.ts | 2 +- .../components/settings/DiscourseNodeConfigPanel.tsx | 2 +- apps/roam/src/components/settings/utils/accessors.ts | 4 ++-- apps/roam/src/components/settings/utils/init.ts | 8 ++------ .../settings/utils/migrateLegacyToBlockProps.ts | 2 +- apps/roam/src/utils/importedSourceIdentity.ts | 4 ++-- apps/roam/src/utils/migrateRelations.ts | 2 +- apps/roam/src/utils/setBlockProps.ts | 12 ++++++++++-- apps/roam/src/utils/supabaseContext.ts | 2 +- 10 files changed, 22 insertions(+), 18 deletions(-) diff --git a/apps/roam/src/components/canvas/Clipboard.tsx b/apps/roam/src/components/canvas/Clipboard.tsx index b3d11457a..e7ac171ac 100644 --- a/apps/roam/src/components/canvas/Clipboard.tsx +++ b/apps/roam/src/components/canvas/Clipboard.tsx @@ -202,7 +202,7 @@ export const ClipboardProvider = ({ if (!isInitialized || !clipboardBlockUid) return; try { - void setBlockProps(clipboardBlockUid, { + setBlockProps(clipboardBlockUid, { [CLIPBOARD_PROP_KEY]: pages, [CLIPBOARD_SHOW_NODES_ON_CANVAS_PROP_KEY]: showNodesOnCanvas, }); diff --git a/apps/roam/src/components/canvas/canvasSyncMode.ts b/apps/roam/src/components/canvas/canvasSyncMode.ts index 20d8979d9..692b07579 100644 --- a/apps/roam/src/components/canvas/canvasSyncMode.ts +++ b/apps/roam/src/components/canvas/canvasSyncMode.ts @@ -42,7 +42,7 @@ const setRoamJsQueryBuilderProps = ({ pageUid: string; nextRjsqb: Record; }): void => { - void setBlockProps(pageUid, { + setBlockProps(pageUid, { [QUERY_BUILDER_PROP_KEY]: nextRjsqb, }); }; diff --git a/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx index f42ef9e07..01a2c289d 100644 --- a/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseNodeConfigPanel.tsx @@ -110,7 +110,7 @@ const DiscourseNodeConfigPanel: React.FC = ({ }, ], }).then((valueUid) => { - void setBlockProps( + setBlockProps( valueUid, DiscourseNodeSchema.parse({ text: label, diff --git a/apps/roam/src/components/settings/utils/accessors.ts b/apps/roam/src/components/settings/utils/accessors.ts index 538ec3512..1c1fa0ad5 100644 --- a/apps/roam/src/components/settings/utils/accessors.ts +++ b/apps/roam/src/components/settings/utils/accessors.ts @@ -657,7 +657,7 @@ const setBlockPropAtPath = ( return currentContext[currentKey]; }, updatedProps); - void setBlockProps(blockUid, updatedProps, false); + setBlockProps(blockUid, updatedProps, false); }; const setBlockPropBasedSettings = ({ @@ -1193,7 +1193,7 @@ export const getAllDiscourseNodes = (): DiscourseNode[] => { ); const retryResult = DiscourseNodeSchema.safeParse(migrated); if (retryResult.success) { - void setBlockProps(pageUid, retryResult.data, false); + setBlockProps(pageUid, retryResult.data, false); nodes.push( toDiscourseNode({ ...retryResult.data, diff --git a/apps/roam/src/components/settings/utils/init.ts b/apps/roam/src/components/settings/utils/init.ts index 5394ff5da..9d6116766 100644 --- a/apps/roam/src/components/settings/utils/init.ts +++ b/apps/roam/src/components/settings/utils/init.ts @@ -163,7 +163,7 @@ const initializeSettingsBlockProps = ( Object.keys(existingProps).length === 0 || !schema.safeParse(existingProps).success ) { - void setBlockProps(uid, defaults, false); + setBlockProps(uid, defaults, false); } // Reconcile placeholder relation keys with real block UIDs. @@ -261,11 +261,7 @@ const reconcileRelationKeys = ( } if (changed) { - void setBlockProps( - globalBlockUid, - { Relations: reconciledRelations }, - false, - ); + setBlockProps(globalBlockUid, { Relations: reconciledRelations }, false); } }; diff --git a/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts b/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts index 88c9c61f2..da29fa11b 100644 --- a/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts +++ b/apps/roam/src/components/settings/utils/migrateLegacyToBlockProps.ts @@ -105,7 +105,7 @@ const migrateSection = ({ return true; } - void setBlockProps(blockUid, parsedLegacy, false); + setBlockProps(blockUid, parsedLegacy, false); onWrite?.(); console.log(`${LOG_PREFIX} ${label}: migrated`); return true; diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index 3f810adfd..64588918a 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -1,7 +1,7 @@ import type { Rid } from "@repo/database/crossAppContracts"; import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; import getBlockProps, { type json } from "./getBlockProps"; -import setBlockProps from "./setBlockProps"; +import { setBlockPropsAsync } from "./setBlockProps"; export type ImportedSourceIdentity = { sourceModifiedAt: string; @@ -49,7 +49,7 @@ export const writeImportedSourceIdentity = async ({ const existing = getBlockProps(pageUid)[DISCOURSE_GRAPH_PROP_NAME]; const discourseGraphProps = isJsonObject(existing) ? existing : {}; - await setBlockProps(pageUid, { + await setBlockPropsAsync(pageUid, { [DISCOURSE_GRAPH_PROP_NAME]: { ...discourseGraphProps, [IMPORTED_FROM_PROP_KEY]: { diff --git a/apps/roam/src/utils/migrateRelations.ts b/apps/roam/src/utils/migrateRelations.ts index 30cb90655..14f103671 100644 --- a/apps/roam/src/utils/migrateRelations.ts +++ b/apps/roam/src/utils/migrateRelations.ts @@ -55,7 +55,7 @@ const migrateRelations = async (): Promise => { ); migrationData[uid] = new Date().valueOf(); dgData[MIGRATION_PROP_NAME] = migrationData; - void setBlockProps(rel.source, { [DISCOURSE_GRAPH_PROP_NAME]: dgData }); + setBlockProps(rel.source, { [DISCOURSE_GRAPH_PROP_NAME]: dgData }); numProcessed++; } } catch (error) { diff --git a/apps/roam/src/utils/setBlockProps.ts b/apps/roam/src/utils/setBlockProps.ts index 93dacd6be..ed3732a29 100644 --- a/apps/roam/src/utils/setBlockProps.ts +++ b/apps/roam/src/utils/setBlockProps.ts @@ -19,7 +19,7 @@ export const deNormalizeProps = (props: json): json => ) : props; -const setBlockProps = ( +export const setBlockPropsAsync = ( uid: string, newProps: Record, denormalize: boolean = false, @@ -40,12 +40,20 @@ const setBlockProps = ( return Promise.resolve(baseProps); }; +const setBlockProps = ( + uid: string, + newProps: Record, + denormalize: boolean = false, +): void => { + void setBlockPropsAsync(uid, newProps, denormalize); +}; + export const testSetBlockProps = ( title: string, newProps: Record, ) => { const uid = getPageUidByPageTitle(title); - return uid ? setBlockProps(uid, newProps) : null; + return uid ? setBlockPropsAsync(uid, newProps) : null; }; export default setBlockProps; diff --git a/apps/roam/src/utils/supabaseContext.ts b/apps/roam/src/utils/supabaseContext.ts index 4146d914c..5c9d8fef0 100644 --- a/apps/roam/src/utils/supabaseContext.ts +++ b/apps/roam/src/utils/supabaseContext.ts @@ -38,7 +38,7 @@ const getOrCreateSpacePassword = () => { if (existing && typeof existing === "string") return existing; // use a uuid as password, at least cryptographically safe const password = crypto.randomUUID(); - void setBlockProps(settingsConfigPageUid, { + setBlockProps(settingsConfigPageUid, { "space-user-password": password, }); return password;