From 7113d870d787789dd4c0c2777e25b198a7cedfff Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Sat, 5 Sep 2026 13:46:22 -0400 Subject: [PATCH] ENG-2219-Add an asset copy stage to Roam node materialization --- .../__tests__/materializeSharedNode.test.ts | 101 +++++++++++++++ apps/roam/src/utils/materializeSharedNode.ts | 118 +++++++++++++++--- .../src/utils/mirrorAssetToRoamStorage.ts | 4 +- 3 files changed, 204 insertions(+), 19 deletions(-) diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index 5cf18f4c0..c57c4b254 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -10,6 +10,10 @@ import { readImportedSourceIdentity, writeImportedSourceIdentity, } from "~/utils/importedSourceIdentity"; +import { + importNodeAssets, + type AssetImportReport, +} from "~/utils/importNodeAssets"; import { materializeSharedNode } from "~/utils/materializeSharedNode"; vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ @@ -22,12 +26,16 @@ vi.mock("roamjs-components/queries/getShallowTreeByParentUid", () => ({ default: vi.fn(), })); vi.mock("roamjs-components/writes/deleteBlock", () => ({ default: vi.fn() })); +vi.mock("~/utils/importNodeAssets", () => ({ + importNodeAssets: vi.fn(), +})); vi.mock("~/utils/importedSourceIdentity", () => ({ findImportedNodeUidBySourceRid: vi.fn(), readImportedSourceIdentity: vi.fn(), writeImportedSourceIdentity: vi.fn(), })); +const mockedImportNodeAssets = vi.mocked(importNodeAssets); const mockedGetPageTitleByPageUid = vi.mocked(getPageTitleByPageUid); const mockedGetPageUidByPageTitle = vi.mocked(getPageUidByPageTitle); const mockedGetShallowTreeByParentUid = vi.mocked(getShallowTreeByParentUid); @@ -94,6 +102,13 @@ const FULL_MARKDOWN = [ const MATERIALIZED_MARKDOWN = "# Findings\nREM sleep improves recall"; +/** + * What the asset stage reports for a node with no recorded references, which every node + * in this suite is. A skipped import replaces no content, so it runs no asset stage and + * carries no report at all. + */ +const NO_ASSETS = { mirrored: 0, reused: 0, skipped: [], failed: [] }; + const clientWithFullContent = ({ text, contentType = "text/obsidian+markdown", @@ -126,6 +141,12 @@ const clientWithFullContent = ({ beforeEach(() => { vi.clearAllMocks(); + // Passing the markdown through unchanged, which is what an asset-free node does. The + // stage is mocked rather than left to the client stub: that stub's select chain is not + // thenable, so the real stage used to see no rows and report nothing by accident. + mockedImportNodeAssets.mockImplementation(({ markdown }) => + Promise.resolve({ markdown, report: NO_ASSETS }), + ); (globalThis as { window: unknown }).window = { roamAlphaAPI: { updatePage, @@ -158,6 +179,7 @@ describe("materializeSharedNode", () => { pageUid: GENERATED_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, + assets: NO_ASSETS, }); expect(eq).toHaveBeenCalledWith("original", true); expect(pageFromMarkdown).toHaveBeenCalledWith({ @@ -243,6 +265,7 @@ describe("materializeSharedNode", () => { pageUid: EXISTING_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, + assets: NO_ASSETS, }); expect(pageFromMarkdown).not.toHaveBeenCalled(); expect(updatePage).not.toHaveBeenCalled(); @@ -296,6 +319,7 @@ describe("materializeSharedNode", () => { pageUid: EXISTING_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, + assets: NO_ASSETS, }); expect(blockFromMarkdown).toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ @@ -467,6 +491,40 @@ describe("materializeSharedNode", () => { expect(updatePage).not.toHaveBeenCalled(); }); + it("writes the markdown the asset stage rewrote, and carries its report", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + const REWRITTEN = "![](https://firebasestorage.googleapis.com/v0/b/f/o/x)"; + const report: AssetImportReport = { + mirrored: 1, + reused: 0, + skipped: [], + failed: [{ sourceLocator: "attachments/big.png", message: "too big" }], + }; + mockedImportNodeAssets.mockResolvedValue({ markdown: REWRITTEN, report }); + + const result = await materializeSharedNode({ client, sharedNode }); + + // The page gets the rewritten markdown, not the published markdown: the copies it + // points at exist by now, and this is the only step that writes them. + expect(pageFromMarkdown).toHaveBeenCalledWith( + expect.objectContaining({ "markdown-string": REWRITTEN }), + ); + expect(result).toMatchObject({ success: true, assets: report }); + }); + + it("reports an asset stage that rejects as its own stage", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedImportNodeAssets.mockRejectedValue(new Error("rewrite blew up")); + + const result = await materializeSharedNode({ client, sharedNode }); + + expect(result).toMatchObject({ + success: false, + error: { stage: "copy-assets" }, + }); + expect(pageFromMarkdown).not.toHaveBeenCalled(); + }); + it("refuses to clobber a page that was not imported from this source", async () => { const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); @@ -480,6 +538,9 @@ describe("materializeSharedNode", () => { }); expect(pageFromMarkdown).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + // The asset stage never ran, so nothing was uploaded. Why a rejected import must + // leave no residue is on `mirrorAssetToRoamStorage`. + expect(mockedImportNodeAssets).not.toHaveBeenCalled(); }); it("fails the rename before touching content when the new title collides", async () => { @@ -499,6 +560,45 @@ describe("materializeSharedNode", () => { expect(mockedDeleteBlock).not.toHaveBeenCalled(); expect(updatePage).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + expect(mockedImportNodeAssets).not.toHaveBeenCalled(); + }); + + /** + * The pre-check has to test the title that actually gets written, which is the decorated + * one whenever the node carries a core title and a type. Testing the raw title instead + * fails both ways: a decorated collision uploads before being rejected, and a raw + * collision rejects an import that would have succeeded. + */ + it("checks the decorated title, so a decorated collision uploads nothing", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedGetPageUidByPageTitle.mockImplementation((title: string) => + title === DECORATED_TITLE ? "unrelated-page-uid" : "", + ); + + const result = await materializeSharedNode({ + client, + sharedNode: decoratedSharedNode, + nodeType: NODE_TYPE, + }); + + expect(result).toMatchObject({ error: { stage: "title-collision" } }); + expect(mockedImportNodeAssets).not.toHaveBeenCalled(); + }); + + it("does not reject on the raw title when the decorated one is free", async () => { + const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + mockedGetPageUidByPageTitle.mockImplementation((title: string) => + title === decoratedSharedNode.title ? "unrelated-page-uid" : "", + ); + + const result = await materializeSharedNode({ + client, + sharedNode: decoratedSharedNode, + nodeType: NODE_TYPE, + }); + + expect(result).toMatchObject({ success: true }); + expect(mockedImportNodeAssets).toHaveBeenCalled(); }); it("imports a Roam-origin node and strips the duplicated title heading", async () => { @@ -515,6 +615,7 @@ describe("materializeSharedNode", () => { pageUid: GENERATED_PAGE_UID, sourceModifiedAt: roamSharedNode.lastModified, sourceNodeRid: roamSharedNode.rid, + assets: NO_ASSETS, }); expect(pageFromMarkdown).toHaveBeenCalledWith({ page: { title: roamSharedNode.title, uid: GENERATED_PAGE_UID }, diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index 3740a72bc..f1e8d5b1f 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -20,10 +20,12 @@ import { type ImportedSourceIdentity, } from "./importedSourceIdentity"; import { getErrorMessage } from "./getErrorMessage"; +import { importNodeAssets, type AssetImportReport } from "./importNodeAssets"; type MaterializationStage = | "validate-input" | "fetch-content" + | "copy-assets" | "find-imported-node" | "title-collision" | "create-page" @@ -49,6 +51,17 @@ type MaterializationSuccess = SourceIdentity & { success: true; action: "created" | "updated" | "skipped"; pageUid: string; + /** + * What the asset stage did. Absent on a skipped import, which replaces no content and + * so copies nothing. An asset that could not be copied appears here rather than + * failing the node. + * + * Nothing reads it yet, and that is the intended state: `importSharedNodes` and + * `refreshImportedNode` both discard it, so a degraded asset is currently invisible to + * the user. Surfacing cross-app failures is ENG-1877's work, and this field exists so + * that ticket has a shape to read rather than a behaviour to add first. + */ + assets?: AssetImportReport; }; export type MaterializeSharedNodeResult = @@ -159,6 +172,49 @@ const fetchFullMarkdown = async ({ return { markdown: markdown.trim() ? markdown : "" }; }; +/** + * The title check both import paths make, extracted so materialization can make it before + * the asset stage runs. + * + * A collision imports nothing and tells the user to rename the other page, which reads as + * a clean no-op. Running the asset stage first would owe a rollback instead, and an upload + * cannot be rolled back safely (see `mirrorAssetToRoamStorage`). The check is two + * synchronous reads, so ordering it first avoids the question. + * + * Sequencing does not solve it. `importSharedNodes` runs one node at a time, which rules + * out a race inside a single run, but nothing serializes two users importing at once. + * + * Still made again inside the two paths: they are exported behaviour in their own right, + * and the message belongs with the check rather than being duplicated at the call site. + */ +const titleCollisionFailure = ({ + identity, + importedPageUid, + title, +}: { + identity: SourceIdentity; + importedPageUid?: string; + title: string; +}): MaterializationFailure | undefined => { + if (!importedPageUid) + return getPageUidByPageTitle(title) + ? 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", + }) + : undefined; + + const localTitle = getPageTitleByPageUid(importedPageUid); + if (localTitle === title || !getPageUidByPageTitle(title)) return undefined; + 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: importedPageUid, + stage: "title-collision", + }); +}; + const createImportedPage = async ({ identity, markdown, @@ -168,12 +224,8 @@ const createImportedPage = async ({ markdown: string; title: string; }): Promise => { - 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 collision = titleCollisionFailure({ identity, title }); + if (collision) return collision; const pageUid = window.roamAlphaAPI.util.generateUID(); try { @@ -233,13 +285,12 @@ const updateImportedPage = async ({ }): Promise => { const localTitle = getPageTitleByPageUid(pageUid); 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", - }); + const collision = titleCollisionFailure({ + identity, + importedPageUid: pageUid, + title, + }); + if (collision) return collision; try { const previousChildren = getShallowTreeByParentUid(pageUid); @@ -364,16 +415,49 @@ export const materializeSharedNode = async ({ stage: "fetch-content", }); - return importedPageUid + // Before the assets, so a rejected import uploads nothing. See `titleCollisionFailure`. + const collision = titleCollisionFailure({ + identity, + importedPageUid: importedPageUid ?? undefined, + title: pageTitle, + }); + if (collision) return collision; + + // Between fetching the content and replacing the page with it: the markdown written + // below is the rewritten one, and the copies it points at exist by then. + // + // Nothing known throws out of the stage today: it reports its per-asset failures and + // catches its reference query. This covers the residue, the link rewrite and whatever a + // later edit adds outside those guards. Without it such a throw leaves a stage-less + // rejection, which callers can only report as an unexplained error. + const assets = await importNodeAssets({ + client, + sharedNode, + markdown: content.markdown, + }).catch((error: unknown) => ({ error })); + if ("error" in assets) + return failure({ + error: assets.error, + identity, + message: `Failed to copy the assets of "${sharedNode.title}"`, + stage: "copy-assets", + }); + const { markdown, report } = assets; + + const result = await (importedPageUid ? updateImportedPage({ identity, - markdown: content.markdown, + markdown, pageUid: importedPageUid, title: pageTitle, }) : createImportedPage({ identity, - markdown: content.markdown, + markdown, title: pageTitle, - }); + })); + + // Carried on success only. A node that failed to import has a stage of its own to + // report, and the assets it did or did not copy are not what the reader needs. + return result.success ? { ...result, assets: report } : result; }; diff --git a/apps/roam/src/utils/mirrorAssetToRoamStorage.ts b/apps/roam/src/utils/mirrorAssetToRoamStorage.ts index 49adca592..bfeadd28a 100644 --- a/apps/roam/src/utils/mirrorAssetToRoamStorage.ts +++ b/apps/roam/src/utils/mirrorAssetToRoamStorage.ts @@ -24,8 +24,8 @@ import { getErrorMessage } from "./getErrorMessage"; * registry keyed by content hash (see `assetRegistry`), so a blob this call uploaded may * already have been resolved by another user's import. A failed import leaves its copies * in place instead, and callers order their work so a rejected import never uploads at - * all. Roam exposes no way to list a graph's files, so an orphan cannot be swept up - * afterwards either. + * all (see `titleCollisionFailure`). Roam exposes no way to list a graph's files, so an + * orphan cannot be swept up afterwards either. * * **Call this one asset at a time.** The registry read and the matching write are * separated by a download and an upload, so callers running it under `Promise.all` all