ENG-2219 Add an asset copy stage to Roam node materialization

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 },

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<MaterializeSharedNodeResult> => {
-  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<MaterializeSharedNodeResult> => {
   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;
 };

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