diff --git a/apps/roam/src/utils/__tests__/assetDegradation.test.ts b/apps/roam/src/utils/__tests__/assetDegradation.test.ts new file mode 100644 index 000000000..f755e3038 --- /dev/null +++ b/apps/roam/src/utils/__tests__/assetDegradation.test.ts @@ -0,0 +1,248 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { CrossAppNode } from "@repo/database/crossAppContracts"; +import type { DGSupabaseClient } from "@repo/database/lib/client"; +import { contentTypes } from "@repo/content-model"; +import type { SharedNode } from "@repo/database/lib/sharedNodes"; +import { MAX_PUBLISHED_ASSET_BYTES } from "@repo/database/lib/assetLimits"; +import { publishNodeAssets, summarizeAssetResults } from "../publishNodeAssets"; +import { importNodeAssets } from "../importNodeAssets"; +import { mirrorAssetToRoamStorage } from "../mirrorAssetToRoamStorage"; + +/** + * The degradation path, followed across both transfers rather than within one. + * + * The published markdown of the first half is the input to the second, so what a + * destination actually receives for an asset that never made it into shared storage is + * asserted rather than assumed: publication reports the failure and leaves the token, + * and import leaves that same token alone because no row matches it. The two halves are + * covered separately in `publishNodeAssets.test.ts` and `importNodeAssets.test.ts`; what + * is only visible here is that they agree on what passes between them. + */ + +vi.mock("../mirrorAssetToRoamStorage", () => ({ + mirrorAssetToRoamStorage: vi.fn(), +})); +const mirror = vi.mocked(mirrorAssetToRoamStorage); + +const roamAsset = (name: string) => + `https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2F${name}.png?alt=media&token=9f1c07a4`; + +const STORED = roamAsset("stored"); +const UNREADABLE = roamAsset("unreadable"); +const OVERSIZED = roamAsset("oversized"); +const EXTERNAL = "https://example.org/not-an-asset.png"; + +const MARKDOWN = [ + "# Sleep improves memory consolidation", + "", + `![](${STORED})`, + `![](${UNREADABLE})`, + `![](${OVERSIZED})`, + `[a paper](${EXTERNAL})`, + "", + "- Supported by [[EVD]] - Rasch & Born 2013", +].join("\n"); + +const SOURCE_LOCAL_ID = "tgWb6JozF"; + +const node: CrossAppNode = { + localId: SOURCE_LOCAL_ID, + nodeType: "rCLM0schema", + coreTitle: "Sleep improves memory consolidation", + content: { + direct: { value: "Sleep improves memory consolidation" }, + full: { contentType: contentTypes.markdown, value: MARKDOWN }, + }, + createdAt: new Date("2026-06-12T14:00:00.000Z"), + modifiedAt: new Date("2026-06-12T15:00:00.000Z"), + authorId: "maparent", +}; + +const sharedNode = { + rid: "orn:roam.node:MAPLab/tgWb6JozF", + sourceLocalId: SOURCE_LOCAL_ID, + spaceId: 20, + spaceName: "MAPLab", + spaceUri: "roam:MAPLab", + platform: "Roam", + title: "Sleep improves memory consolidation", + created: null, + lastModified: "2026-06-12T15:00:00.000Z", + directMetadata: null, +} as unknown as SharedNode; + +type Row = { + filepath: string; + filehash: string; + source_path: string | null; +}; + +/** + * One store standing in for Supabase across both halves: publication inserts into it and + * import reads back out of it, so the rows the destination sees are the rows publication + * actually wrote. + */ +const makeSharedStorage = () => { + const rows: Row[] = []; + const thenable = (result: unknown) => ({ + then: (resolve: (value: unknown) => unknown) => + Promise.resolve(result).then(resolve), + }); + const selectChain = () => { + const chain = { + eq: () => chain, + in: () => chain, + order: () => chain, + then: (resolve: (value: unknown) => unknown) => + Promise.resolve({ data: rows, error: null }).then(resolve), + }; + return chain; + }; + const client = { + rpc: vi.fn((_fn: string, { hashvalue }: { hashvalue: string }) => + Promise.resolve({ + data: rows.some((row) => row.filehash === hashvalue), + error: null, + }), + ), + storage: { + from: vi.fn(() => ({ + upload: vi.fn().mockResolvedValue({ error: null }), + })), + }, + from: vi.fn(() => ({ + select: vi.fn(() => selectChain()), + delete: vi.fn(() => { + const chain = { + eq: () => chain, + notIn: () => chain, + then: (resolve: (value: unknown) => unknown) => + Promise.resolve({ error: null }).then(resolve), + }; + return chain; + }), + insert: vi.fn((inserted: Row) => { + rows.push({ + filepath: inserted.filepath, + filehash: inserted.filehash, + source_path: inserted.source_path ?? null, + }); + return thenable({ error: null }); + }), + })), + } as unknown as DGSupabaseClient; + return { client, rows }; +}; + +/** Roam's storage: one asset readable, one unreadable, one past the publish cap. */ +const stubRoamStorage = () => { + vi.stubGlobal( + "fetch", + vi.fn((input: string) => { + const url = input.split("?")[0] ?? input; + if (url === UNREADABLE.split("?")[0]) + return Promise.resolve({ + ok: false, + status: 500, + } as unknown as Response); + const size = + url === OVERSIZED.split("?")[0] ? MAX_PUBLISHED_ASSET_BYTES + 1 : 7; + if (!input.includes("alt=media")) + return Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + name: "imgs/app/MAPLab/stored.png", + contentType: "image/png", + size: String(size), + metadata: { "file-name": "diagram.png" }, + }), + } as unknown as Response); + return Promise.resolve({ + ok: true, + status: 200, + arrayBuffer: () => + Promise.resolve(new TextEncoder().encode("PNGDATA").buffer), + } as unknown as Response); + }), + ); +}; + +const THIS_GRAPHS_COPY = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FOtherGraph%2FaB3dEf.png?alt=media&token=1122"; + +describe("asset degradation across both transfers", () => { + let storage: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + storage = makeSharedStorage(); + stubRoamStorage(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + const publish = () => + publishNodeAssets({ + client: storage.client, + spaceId: 20, + nodes: [node], + }); + + it("leaves the token of every asset it could not store in the published markdown, and reports each one", async () => { + const summary = summarizeAssetResults(await publish()); + + expect(node.content.full?.value).toBe(MARKDOWN); + expect(summary.failed.map((f) => f.sourceRef)).toEqual([UNREADABLE]); + expect(summary.tooLarge.map((s) => s.sourceRef)).toEqual([OVERSIZED]); + expect(summary.copied).toBe(1); + expect(storage.rows.map((r) => r.filepath)).toEqual([STORED]); + }); + + it("imports the published markdown with its body intact, rewriting only what was stored", async () => { + await publish(); + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: storage.rows[0].filehash, + url: THIS_GRAPHS_COPY, + }); + + const { markdown, report } = await importNodeAssets({ + client: storage.client, + sharedNode, + markdown: MARKDOWN, + }); + + // Only the asset that reached shared storage was mirrored, so only its token moved. + expect(mirror).toHaveBeenCalledTimes(1); + expect(markdown).toContain(`![](${THIS_GRAPHS_COPY})`); + // The rest of the node arrives exactly as published: the two tokens that never + // became rows still point at Roam's world-readable originals, which is what makes + // them render, and the external link was never ours to touch. + expect(markdown).toContain(`![](${UNREADABLE})`); + expect(markdown).toContain(`![](${OVERSIZED})`); + expect(markdown).toContain(`[a paper](${EXTERNAL})`); + expect(markdown).toContain("# Sleep improves memory consolidation"); + expect(markdown).toContain("- Supported by [[EVD]] - Rasch & Born 2013"); + expect(report).toMatchObject({ mirrored: 1, reused: 0, skipped: [] }); + }); + + it("reports an asset that fails on the way in, leaving its token and the node body untouched", async () => { + await publish(); + mirror.mockRejectedValue(new Error("upload refused")); + + const { markdown, report } = await importNodeAssets({ + client: storage.client, + sharedNode, + markdown: MARKDOWN, + }); + + expect(markdown).toBe(MARKDOWN); + expect(report.failed).toEqual([ + { sourceRef: STORED, message: "upload refused" }, + ]); + }); +}); diff --git a/apps/roam/src/utils/__tests__/importNodeAssets.test.ts b/apps/roam/src/utils/__tests__/importNodeAssets.test.ts new file mode 100644 index 000000000..d3441d3c2 --- /dev/null +++ b/apps/roam/src/utils/__tests__/importNodeAssets.test.ts @@ -0,0 +1,362 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { DGSupabaseClient } from "@repo/database/lib/client"; +import type { SharedNode } from "@repo/database/lib/sharedNodes"; +import { importNodeAssets } from "../importNodeAssets"; +import { mirrorAssetToRoamStorage } from "../mirrorAssetToRoamStorage"; + +vi.mock("../mirrorAssetToRoamStorage", () => ({ + mirrorAssetToRoamStorage: vi.fn(), +})); + +const mirror = vi.mocked(mirrorAssetToRoamStorage); + +const IMAGE_REF = "attachments/diagram.png"; +const FILE_REF = "attachments/report.docx"; +const MIRRORED = "https://firebasestorage.googleapis.com/v0/b/f/o/x?alt=media"; + +const sharedNode = { + rid: "orn:obsidian.note:vault-a/node-1", + sourceLocalId: "node-1", + spaceId: 20, + spaceName: "Vault A", + spaceUri: "obsidian:vault-a", + platform: "Obsidian", + title: "REM sleep and recall", + created: null, + lastModified: "2026-06-14T15:00:00.000Z", + directMetadata: null, +} as unknown as SharedNode; + +type Row = { filepath: string; filehash: string; source_path: string | null }; + +const clientWithReferences = ( + rows: Row[], + error?: { message: string }, +): { client: DGSupabaseClient; from: ReturnType } => { + const result = error ? { data: null, error } : { data: rows, error: null }; + const chain = { + eq: vi.fn(() => chain), + order: vi.fn(() => chain), + then: (resolve: (value: unknown) => unknown) => + Promise.resolve(result).then(resolve), + }; + const from = vi.fn(() => ({ select: vi.fn(() => chain) })); + return { client: { from } as unknown as DGSupabaseClient, from }; +}; + +const row = ( + filepath: string, + hash: string, + sourcePath: string | null = null, +): Row => ({ + filepath, + filehash: hash, + source_path: sourcePath, +}); + +beforeEach(() => { + vi.clearAllMocks(); +}); + +describe("importNodeAssets", () => { + it("points the markdown at this graph's copies and counts what it uploaded", async () => { + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }); + + const result = await importNodeAssets({ + client: clientWithReferences([row(IMAGE_REF, "h1")]).client, + sharedNode, + markdown: `![](${IMAGE_REF})`, + }); + + expect(result.markdown).toBe(`![](${MIRRORED})`); + expect(result.report).toEqual({ + mirrored: 1, + reused: 0, + skipped: [], + failed: [], + }); + }); + + it("counts a copy this graph already held separately from one it uploaded", async () => { + mirror.mockResolvedValue({ + status: "reused", + contentHash: "h1", + url: MIRRORED, + }); + + const { report } = await importNodeAssets({ + client: clientWithReferences([row(IMAGE_REF, "h1")]).client, + sharedNode, + markdown: `![](${IMAGE_REF})`, + }); + + expect(report).toMatchObject({ mirrored: 0, reused: 1 }); + }); + + it("passes the recorded name through, so a non-media link can be labelled", async () => { + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: "h2", + url: MIRRORED, + }); + + const result = await importNodeAssets({ + client: clientWithReferences([row(FILE_REF, "h2", "report.docx")]).client, + sharedNode, + markdown: `[](${FILE_REF})`, + }); + + expect(mirror).toHaveBeenCalledWith( + expect.objectContaining({ contentHash: "h2", sourcePath: "report.docx" }), + ); + expect(result.markdown).toBe(`[report.docx](${MIRRORED})`); + }); + + it("imports the node with its content intact when one asset fails", async () => { + mirror + .mockResolvedValueOnce({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }) + .mockRejectedValueOnce(new Error("upload refused")); + + const markdown = `![](${IMAGE_REF}) and [](${FILE_REF})`; + const result = await importNodeAssets({ + client: clientWithReferences([row(IMAGE_REF, "h1"), row(FILE_REF, "h2")]) + .client, + sharedNode, + markdown, + }); + + // The resolved asset is still rewritten; the failed one keeps the token it arrived + // with, which is the degradation path rather than a broken node. + expect(result.markdown).toBe(`![](${MIRRORED}) and [](${FILE_REF})`); + expect(result.report.mirrored).toBe(1); + expect(result.report.failed).toEqual([ + { sourceRef: FILE_REF, message: "upload refused" }, + ]); + }); + + it("reports an oversized asset and leaves its token in place", async () => { + mirror.mockResolvedValue({ + status: "skipped", + contentHash: "h1", + reason: "too-large", + size: 9_000_000, + limit: 6_291_456, + }); + + const markdown = `![](${IMAGE_REF})`; + const result = await importNodeAssets({ + client: clientWithReferences([row(IMAGE_REF, "h1")]).client, + sharedNode, + markdown, + }); + + expect(result.markdown).toBe(markdown); + expect(result.report.skipped).toEqual([ + { + sourceRef: IMAGE_REF, + reason: "too-large", + size: 9_000_000, + limit: 6_291_456, + }, + ]); + }); + + it("imports a node whose every asset fails, reporting each one", async () => { + mirror.mockRejectedValue(new Error("storage unreachable")); + + const markdown = `![](${IMAGE_REF}) and [](${FILE_REF})`; + const result = await importNodeAssets({ + client: clientWithReferences([row(IMAGE_REF, "h1"), row(FILE_REF, "h2")]) + .client, + sharedNode, + markdown, + }); + + expect(result.markdown).toBe(markdown); + expect(result.report.failed).toHaveLength(2); + }); + + it("does not fail the node when the references cannot be read", async () => { + const markdown = `![](${IMAGE_REF})`; + const result = await importNodeAssets({ + client: clientWithReferences([], { message: "permission denied" }).client, + sharedNode, + markdown, + }); + + expect(result.markdown).toBe(markdown); + expect(result.report.failed).toEqual([ + { + sourceRef: sharedNode.rid, + message: expect.stringContaining("permission denied") as string, + }, + ]); + expect(mirror).not.toHaveBeenCalled(); + }); + + // A node published from Roam and imported into a second Roam graph. The token is a URL + // the importing graph could render directly, and it is still resolved through its row + // and copied: recognising a storage URL in order to skip the copy would put origin + // detection back into the destination, and it would leave this graph's page depending + // on a blob the origin graph's owner can delete. + it("stores its own copy of a Roam-origin asset rather than passing the origin URL through", async () => { + const originUrl = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FOriginGraph%2FlqP2ioVNC3.png?alt=media&token=9f1c07a4"; + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }); + + const result = await importNodeAssets({ + client: clientWithReferences([ + row(originUrl, "h1", "CleanShot 2025-11-16 at 17.14.44@2x.png"), + ]).client, + sharedNode, + markdown: `![](${originUrl})`, + }); + + expect(mirror).toHaveBeenCalledTimes(1); + expect(result.markdown).toBe(`![](${MIRRORED})`); + expect(result.markdown).not.toContain("OriginGraph"); + expect(result.report.mirrored).toBe(1); + }); + + it("copies nothing for a node with no recorded references", async () => { + const markdown = `![](${IMAGE_REF})`; + const result = await importNodeAssets({ + client: clientWithReferences([]).client, + sharedNode, + markdown, + }); + + expect(result.markdown).toBe(markdown); + expect(result.report).toEqual({ + mirrored: 0, + reused: 0, + skipped: [], + failed: [], + }); + expect(mirror).not.toHaveBeenCalled(); + }); + + it("hands each caller its own report, so one node's failure is not another's", async () => { + const first = await importNodeAssets({ + client: clientWithReferences([]).client, + sharedNode, + markdown: "no assets here", + }); + const second = await importNodeAssets({ + client: clientWithReferences([]).client, + sharedNode, + markdown: "none here either", + }); + + first.report.failed.push({ sourceRef: "x", message: "mine alone" }); + + expect(second.report.failed).toEqual([]); + }); + + it("counts one upload when two tokens name identical bytes", async () => { + mirror + .mockResolvedValueOnce({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }) + .mockResolvedValueOnce({ + status: "reused", + contentHash: "h1", + url: MIRRORED, + }); + + const { report } = await importNodeAssets({ + client: clientWithReferences([ + row(IMAGE_REF, "h1"), + row("attachments/copy.png", "h1"), + ]).client, + sharedNode, + markdown: `![](${IMAGE_REF}) ![](attachments/copy.png)`, + }); + + // Not `reused: 1`: this run uploaded those bytes itself a moment earlier, and a first + // import reporting a cache hit would be a lie about where the copy came from. + expect(report).toMatchObject({ mirrored: 1, reused: 0 }); + // Both references are still mirrored. Deduplication belongs to the registry inside + // `mirrorAssetToRoamStorage`, which is what turns the second call into a reuse; this + // module's job is only to count blobs rather than tokens. + expect(mirror).toHaveBeenCalledTimes(2); + }); + + it("copies nothing for a reference the fetched markdown never makes", async () => { + const { client } = clientWithReferences([ + row("attachments/only-in-frontmatter.png", "h1"), + ]); + + const { report } = await importNodeAssets({ + client, + sharedNode, + markdown: "A body that mentions no assets at all.", + }); + + // The row outlived its token — stripped frontmatter, or a publish whose best-effort + // cleanup failed. Uploading it would spend the user's storage permanently on bytes + // no block can reference. + expect(mirror).not.toHaveBeenCalled(); + expect(report).toMatchObject({ mirrored: 0, reused: 0 }); + }); + + it("copies a reference the markdown percent-encodes", async () => { + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }); + + const result = await importNodeAssets({ + client: clientWithReferences([row("my folder/d.png", "h1")]).client, + sharedNode, + markdown: `![](my%20folder/d.png)`, + }); + + expect(mirror).toHaveBeenCalledTimes(1); + expect(result.markdown).toBe(`![](${MIRRORED})`); + }); + + it("copies a reference whose name forces an encoding encodeURI would not apply", async () => { + // `fig#1.png` is written `fig%231.png`, because `#` starts a fragment. Deriving the + // spellings forward would miss it and drop the asset; reading the tokens the rewriter + // will act on cannot, because it is the same set. + mirror.mockResolvedValue({ + status: "mirrored", + contentHash: "h1", + url: MIRRORED, + }); + + const result = await importNodeAssets({ + client: clientWithReferences([row("fig#1.png", "h1")]).client, + sharedNode, + markdown: `![](fig%231.png)`, + }); + + expect(mirror).toHaveBeenCalledTimes(1); + expect(result.markdown).toBe(`![](${MIRRORED})`); + }); + + it("copies nothing for a node with no content", async () => { + const { client, from } = clientWithReferences([row(IMAGE_REF, "h1")]); + const result = await importNodeAssets({ client, sharedNode, markdown: "" }); + + expect(result.markdown).toBe(""); + // Not even the reference query runs: there is nothing a rewrite could apply to. + expect(from).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts index 5cf18f4c0..0ed0cdaab 100644 --- a/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts +++ b/apps/roam/src/utils/__tests__/materializeSharedNode.test.ts @@ -10,6 +10,7 @@ import { readImportedSourceIdentity, writeImportedSourceIdentity, } from "~/utils/importedSourceIdentity"; +import { importNodeAssets } from "~/utils/importNodeAssets"; import { materializeSharedNode } from "~/utils/materializeSharedNode"; vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ @@ -22,12 +23,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 +99,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 +138,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 +176,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 +262,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 +316,7 @@ describe("materializeSharedNode", () => { pageUid: EXISTING_PAGE_UID, sourceModifiedAt: sharedNode.lastModified, sourceNodeRid: sharedNode.rid, + assets: NO_ASSETS, }); expect(blockFromMarkdown).toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).toHaveBeenCalledWith({ @@ -467,8 +488,42 @@ describe("materializeSharedNode", () => { expect(updatePage).not.toHaveBeenCalled(); }); - it("refuses to clobber a page that was not imported from this source", async () => { + 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 = { + mirrored: 1, + reused: 0, + skipped: [], + failed: [{ sourceRef: "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, from } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); const result = await materializeSharedNode({ client, sharedNode }); @@ -480,10 +535,13 @@ describe("materializeSharedNode", () => { }); expect(pageFromMarkdown).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + // The asset stage never ran, so nothing was uploaded. A rejected import must leave no + // residue: a copy into Roam storage cannot be undone or even found afterwards. + expect(from).not.toHaveBeenCalledWith("my_file_references"); }); it("fails the rename before touching content when the new title collides", async () => { - const { client } = clientWithFullContent({ text: FULL_MARKDOWN }); + const { client, from } = clientWithFullContent({ text: FULL_MARKDOWN }); mockedFindImportedNodeUidBySourceRid.mockResolvedValue(EXISTING_PAGE_UID); mockedGetPageTitleByPageUid.mockReturnValue("EVD - old title"); mockedGetPageUidByPageTitle.mockReturnValue("unrelated-page-uid"); @@ -499,6 +557,7 @@ describe("materializeSharedNode", () => { expect(mockedDeleteBlock).not.toHaveBeenCalled(); expect(updatePage).not.toHaveBeenCalled(); expect(mockedWriteImportedSourceIdentity).not.toHaveBeenCalled(); + expect(from).not.toHaveBeenCalledWith("my_file_references"); }); it("imports a Roam-origin node and strips the duplicated title heading", async () => { @@ -515,6 +574,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/getErrorMessage.ts b/apps/roam/src/utils/getErrorMessage.ts new file mode 100644 index 000000000..2d1decb45 --- /dev/null +++ b/apps/roam/src/utils/getErrorMessage.ts @@ -0,0 +1,19 @@ +/** + * The message to show for something that was thrown, wherever it came from. + * + * Lives on its own because both sides of an import need it: `materializeSharedNode` for + * its stage failures, and the asset stage it calls for the ones it reports instead of + * throwing. Putting it in either would make the other import its caller. + * + * Supabase reports a failed query as a plain object carrying `message`, not as an + * `Error`, so a message is read from either shape. Stringifying the object instead would + * put `[object Object]` in front of the one person who needs to know what went wrong. + */ +export const getErrorMessage = (error: unknown): string => { + if (error instanceof Error) return error.message; + if (typeof error === "object" && error !== null && "message" in error) { + const { message } = error; + if (typeof message === "string") return message; + } + return String(error); +}; diff --git a/apps/roam/src/utils/importNodeAssets.ts b/apps/roam/src/utils/importNodeAssets.ts new file mode 100644 index 000000000..8d11a58b5 --- /dev/null +++ b/apps/roam/src/utils/importNodeAssets.ts @@ -0,0 +1,208 @@ +import type { DGSupabaseClient } from "@repo/database/lib/client"; +import type { SharedNode } from "@repo/database/lib/sharedNodes"; +import { getErrorMessage } from "./getErrorMessage"; +import { mirrorAssetToRoamStorage } from "./mirrorAssetToRoamStorage"; +import { + collectAssetTokens, + lookupCandidates, + rewriteAssetLinks, + type ResolvedAsset, +} from "./rewriteAssetLinks"; + +/** + * The asset stage of materialization: copy the bytes an imported node references into + * this graph's storage, and point the node's markdown at those copies. + * + * It runs between fetching the content and replacing the page's blocks, because the + * markdown it returns is what gets written. Nothing here can fail the node: an asset that + * cannot be copied leaves its token exactly as published, and is reported instead of + * thrown. + * + * What that token then does depends on where it came from, and only one case is benign. A + * Roam-origin token is a public Firebase URL, so the block still renders the asset from + * the origin graph's storage. An Obsidian-origin token is a vault path this graph cannot + * resolve; left verbatim, `![[attachments/diagram.png]]` is a page reference to Roam, so + * a failed Obsidian asset leaves a link to an empty page named after a vault path. + * Rewriting it to something inert would be the fix, and it is not this stage's to make: + * the contract requires an unresolved token to survive unchanged, which is what lets a + * later re-import resolve it once the bytes are there. + */ + +export type SkippedImport = { + sourceRef: string; + reason: "too-large"; + size: number; + limit: number; +}; + +export type FailedImport = { + sourceRef: string; + message: string; +}; + +export type AssetImportReport = { + /** Uploaded into this graph's storage by this run. */ + mirrored: number; + /** Already held by this graph, so no upload was needed. */ + reused: number; + skipped: SkippedImport[]; + failed: FailedImport[]; +}; + +/** + * A fresh report per call, never a shared constant. Callers own what they are handed and + * the arrays are mutable, so one `report.failed.push(...)` on a returned object would + * otherwise attribute one node's failure to every asset-free node in the session. + */ +const emptyReport = (): AssetImportReport => ({ + mirrored: 0, + reused: 0, + skipped: [], + failed: [], +}); + +type ReferenceRow = { + filepath: string; + filehash: string; + source_path: string | null; +}; + +/** + * The references recorded against the published node, which are the only things this + * stage resolves. A node with no rows has no assets to copy, whether because it + * references none or because none could be stored when it was published. + */ +const fetchNodeReferences = async ({ + client, + sharedNode, +}: { + client: DGSupabaseClient; + sharedNode: SharedNode; +}): Promise => { + const { data, error } = await client + .from("my_file_references") + .select("filepath, filehash, source_path") + .eq("space_id", sharedNode.spaceId) + .eq("source_local_id", sharedNode.sourceLocalId) + // Ordered so a repeated import does the same thing twice. Where two references share + // a hash, the first one mirrored decides the uploaded file's extension, because the + // second reuses its URL; without an order, which name that is comes down to whatever + // Postgres returned first. + .order("filepath"); + if (error) throw error; + return (data ?? []).flatMap((row): ReferenceRow[] => + typeof row.filepath === "string" && typeof row.filehash === "string" + ? [ + { + filepath: row.filepath, + filehash: row.filehash, + source_path: + typeof row.source_path === "string" ? row.source_path : null, + }, + ] + : [], + ); +}; + +export const importNodeAssets = async ({ + client, + sharedNode, + markdown, +}: { + client: DGSupabaseClient; + sharedNode: SharedNode; + markdown: string; +}): Promise<{ markdown: string; report: AssetImportReport }> => { + if (!markdown) return { markdown, report: emptyReport() }; + + let references: ReferenceRow[]; + try { + references = await fetchNodeReferences({ client, sharedNode }); + } catch (error) { + // The node still imports, with every asset token left as published. Reported as one + // failure rather than none, because "no rows" and "could not read the rows" produce + // the same content and must not look the same to a reader. + return { + markdown, + report: { + ...emptyReport(), + failed: [ + { + sourceRef: sharedNode.rid, + message: `Could not read the asset references of "${sharedNode.title}": ${getErrorMessage(error)}`, + }, + ], + }, + }; + } + if (!references.length) return { markdown, report: emptyReport() }; + + // Only the references this content actually makes. A row can outlive its token two + // ways: `publishNodeAssets` cleans stale rows best-effort and logs rather than fails, and + // the markdown fetched here has had its frontmatter or title heading stripped, so an + // asset referenced only there has a row and no token. Copying one would spend the user's + // storage, permanently, on bytes no block will ever point at. + // + // The set comes from the rewriter's own reading of the text, not from re-deriving the + // spellings a path might take. Generating them forward cannot work: a note writes + // `fig#1.png` as `fig%231.png`, and `encodeURI` leaves `#` and `?` alone, so a filter + // built that way drops an asset the rewrite would have resolved. + const resolvable = new Set( + collectAssetTokens(markdown).flatMap(lookupCandidates), + ); + const referenced = references.filter(({ filepath }) => + resolvable.has(filepath), + ); + if (!referenced.length) return { markdown, report: emptyReport() }; + + const resolved: ResolvedAsset[] = []; + const report = emptyReport(); + /** + * Counts are per distinct blob, not per reference. Two tokens for identical bytes are + * one upload, and reporting the second as `reused` would tell a user on a first-ever + * import that this graph already held something it had just fetched. + */ + const handledHashes = new Set(); + + // Sequential on purpose. Two references to identical content share a hash, and the + // registry is what stops the second one uploading again; running them together would + // race that check and mirror the same bytes twice. + for (const reference of referenced) { + try { + const result = await mirrorAssetToRoamStorage({ + client, + contentHash: reference.filehash, + sourcePath: reference.source_path, + }); + if (result.status === "skipped") { + report.skipped.push({ + sourceRef: reference.filepath, + reason: result.reason, + size: result.size, + limit: result.limit, + }); + continue; + } + if (!handledHashes.has(reference.filehash)) { + handledHashes.add(reference.filehash); + if (result.status === "mirrored") report.mirrored += 1; + else report.reused += 1; + } + resolved.push({ + sourceRef: reference.filepath, + url: result.url, + sourcePath: reference.source_path, + }); + } catch (error) { + report.failed.push({ + sourceRef: reference.filepath, + message: getErrorMessage(error), + }); + } + } + + return { + markdown: rewriteAssetLinks({ markdown, assets: resolved }), + report, + }; +}; diff --git a/apps/roam/src/utils/importSharedNodes.ts b/apps/roam/src/utils/importSharedNodes.ts index 6a4f23abc..0d9c3931b 100644 --- a/apps/roam/src/utils/importSharedNodes.ts +++ b/apps/roam/src/utils/importSharedNodes.ts @@ -1,9 +1,7 @@ import type { DGSupabaseClient } from "@repo/database/lib/client"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; -import { - getErrorMessage, - materializeSharedNode, -} from "./materializeSharedNode"; +import { getErrorMessage } from "./getErrorMessage"; +import { materializeSharedNode } from "./materializeSharedNode"; import { resolveSharedNodeTypes } from "./resolveSharedNodeTypes"; export type FailedSharedNodeImport = { diff --git a/apps/roam/src/utils/materializeSharedNode.ts b/apps/roam/src/utils/materializeSharedNode.ts index 6fbd7cac0..5be3e43fb 100644 --- a/apps/roam/src/utils/materializeSharedNode.ts +++ b/apps/roam/src/utils/materializeSharedNode.ts @@ -19,10 +19,13 @@ import { writeImportedSourceIdentity, 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" @@ -48,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 = @@ -72,9 +86,6 @@ type RoamMarkdownApi = { export const getRoamMarkdownApi = (): RoamMarkdownApi => window.roamAlphaAPI.data as unknown as RoamMarkdownApi; -export const getErrorMessage = (error: unknown): string => - error instanceof Error ? error.message : String(error); - const isImportUpToDate = ({ sourceModifiedAt, storedModifiedAt, @@ -161,6 +172,48 @@ const fetchFullMarkdown = async ({ return { markdown: markdown.trim() ? markdown : "" }; }; +/** + * The title check both import paths make, extracted so materialization can make it before + * anything irrevocable happens. + * + * A collision imports nothing and tells the user to rename the other page, which reads as + * a clean no-op — but copying an asset into Roam storage cannot be undone, and Roam + * exposes no way to list or delete what was uploaded (see `mirrorAssetToRoamStorage`). + * Running the asset stage first would leave a failed import charging the user's storage + * for blobs nothing references and nothing can find. The check is two synchronous reads, + * so making it early costs nothing. + * + * 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, @@ -170,12 +223,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 { @@ -235,13 +284,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); @@ -366,16 +414,52 @@ export const materializeSharedNode = async ({ stage: "fetch-content", }); - return importedPageUid + // Before the assets, because a collision imports nothing while an upload cannot be + // taken back. Both import paths check again; this one exists to keep a rejected import + // from spending the user's Roam storage on blobs no page will reference. + const collision = titleCollisionFailure({ + identity, + importedPageUid: importedPageUid ?? undefined, + title: validated.title, + }); + 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. + // + // Guarded like every other awaited step. The stage reports its own per-asset failures + // and catches its reference query, so nothing known throws out of it today; what this + // covers is the residue — the link rewrite, and whatever a later edit adds outside + // those guards. Without it such a throw leaves `materializeSharedNode` with a + // stage-less rejection, and its callers can only report that 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 1c8c1f784..4c08acfba 100644 --- a/apps/roam/src/utils/mirrorAssetToRoamStorage.ts +++ b/apps/roam/src/utils/mirrorAssetToRoamStorage.ts @@ -4,6 +4,7 @@ import { } from "@repo/database/lib/assetLimits"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import { readMirroredAssetUrl, recordMirroredAsset } from "./assetRegistry"; +import { getErrorMessage } from "./getErrorMessage"; /** * Copies one asset out of shared storage and into this graph's own Roam storage. @@ -31,9 +32,6 @@ import { readMirroredAssetUrl, recordMirroredAsset } from "./assetRegistry"; * in-flight map of hash to promise in this module first. */ -const getErrorMessage = (error: unknown): string => - error instanceof Error ? error.message : String(error); - /** The bucket `addFile` writes to, keyed by content hash. */ const SHARED_ASSET_BUCKET = "assets"; diff --git a/apps/roam/src/utils/refreshImportedNode.ts b/apps/roam/src/utils/refreshImportedNode.ts index a3ae9208a..1a4b668bd 100644 --- a/apps/roam/src/utils/refreshImportedNode.ts +++ b/apps/roam/src/utils/refreshImportedNode.ts @@ -2,10 +2,8 @@ import { getSharedNodeByRid } from "@repo/database/lib/sharedNodes"; import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; import { readImportedSourceIdentity } from "./importedSourceIdentity"; import internalError from "./internalError"; -import { - getErrorMessage, - materializeSharedNode, -} from "./materializeSharedNode"; +import { getErrorMessage } from "./getErrorMessage"; +import { materializeSharedNode } from "./materializeSharedNode"; import { resolveSharedNodeTypes } from "./resolveSharedNodeTypes"; import { getLoggedInClient } from "./supabaseContext"; diff --git a/apps/roam/src/utils/rewriteAssetLinks.ts b/apps/roam/src/utils/rewriteAssetLinks.ts index 29dfceed3..30d00f532 100644 --- a/apps/roam/src/utils/rewriteAssetLinks.ts +++ b/apps/roam/src/utils/rewriteAssetLinks.ts @@ -242,7 +242,7 @@ const TRAILING_PUNCTUATION = /[.,;:!?]+$/; * `metadataCache`, while the note itself holds `my%20folder/d.png` — so any vault path * with a space in it needs the decoded form to match. */ -const lookupCandidates = (ref: string): string[] => { +export const lookupCandidates = (ref: string): string[] => { const candidates = [ref]; const withoutPunctuation = ref.replace(TRAILING_PUNCTUATION, ""); if (withoutPunctuation !== ref) candidates.push(withoutPunctuation); @@ -257,6 +257,84 @@ const lookupCandidates = (ref: string): string[] => { return candidates; }; +/** + * What one match of `LINK_PATTERN` refers to, read from the capture groups in the order + * the pattern lists its branches. + * + * Shared with `collectAssetTokens` so that the tokens a caller can see are exactly the + * tokens this file will rewrite. Anything deriving that set independently drifts from it, + * and a token missing from the caller's set is an asset silently dropped. + */ +const parseMatch = ( + groups: (string | undefined)[], +): + | { + ref: string; + form: ReferenceForm; + declaredKind?: AssetKind; + linkText: string; + } + | undefined => { + const [ + imageAlt, + imageRef, + linkLabel, + linkRef, + bracketedMediaKind, + bracketedMediaRef, + mediaKind, + mediaRef, + embedRef, + wikiRef, + bareRef, + ] = groups; + const ref = + imageRef ?? + linkRef ?? + bracketedMediaRef ?? + mediaRef ?? + embedRef ?? + wikiRef ?? + bareRef; + if (ref === undefined) return undefined; + + const form: ReferenceForm = + imageRef !== undefined || + bracketedMediaRef !== undefined || + mediaRef !== undefined || + embedRef !== undefined + ? "embed" + : bareRef !== undefined + ? "bare" + : "link"; + + return { + ref, + form, + declaredKind: (bracketedMediaKind ?? mediaKind) as AssetKind | undefined, + // A wikilink embed carries no separate text, so its label comes from the asset. + linkText: imageRef ? (imageAlt ?? "") : (linkLabel ?? ""), + }; +}; + +/** + * Every token this markdown refers an asset by, as `rewriteAssetLinks` will read them. + * + * A caller deciding which recorded references are worth acting on has to ask the text the + * same question the rewrite will ask it. Widening each of these through + * `lookupCandidates` yields exactly the set of `sourceRef` values that would resolve, so + * a caller's set and the rewriter's are equal by construction rather than by agreement. + */ +export const collectAssetTokens = (markdown: string): string[] => { + const tokens: string[] = []; + for (const match of markdown.matchAll(LINK_PATTERN)) { + const [, ...groups] = match; + const parsed = parseMatch(groups); + if (parsed) tokens.push(parsed.ref); + } + return tokens; +}; + export const rewriteAssetLinks = ({ markdown, assets, @@ -272,28 +350,9 @@ export const rewriteAssetLinks = ({ // One capture group per branch, in the order the pattern lists them. The trailing // offset and input arguments the replacer also receives are simply not destructured. (match: string, ...groups: (string | undefined)[]) => { - const [ - imageAlt, - imageRef, - linkLabel, - linkRef, - bracketedMediaKind, - bracketedMediaRef, - mediaKind, - mediaRef, - embedRef, - wikiRef, - bareRef, - ] = groups; - const ref = - imageRef ?? - linkRef ?? - bracketedMediaRef ?? - mediaRef ?? - embedRef ?? - wikiRef ?? - bareRef; - if (ref === undefined) return match; + const parsed = parseMatch(groups); + if (!parsed) return match; + const { ref, form, declaredKind, linkText } = parsed; const candidates = lookupCandidates(ref); const matched = candidates.find((candidate) => byRef.has(candidate)); @@ -301,25 +360,11 @@ export const rewriteAssetLinks = ({ const asset = byRef.get(matched); if (!asset) return match; - const form: ReferenceForm = - imageRef !== undefined || - bracketedMediaRef !== undefined || - mediaRef !== undefined || - embedRef !== undefined - ? "embed" - : bareRef !== undefined - ? "bare" - : "link"; - const context: ReferenceContext = { - form, - declaredKind: (bracketedMediaKind ?? mediaKind) as - | AssetKind - | undefined, - }; - - // A wikilink embed carries no separate text, so its label comes from the asset. - const linkText = imageRef ? (imageAlt ?? "") : (linkLabel ?? ""); - const rewritten = render({ asset, linkText, context }); + const rewritten = render({ + asset, + linkText, + context: { form, declaredKind }, + }); // Punctuation only comes back on a bare URL, where it was the sentence's rather // than the link's. Inside `![](…)` or `{{[[pdf]]: …}}` the token is delimited