diff --git a/apps/roam/src/utils/__tests__/fixtures/fileReferenceHarness.ts b/apps/roam/src/utils/__tests__/fixtures/fileReferenceHarness.ts new file mode 100644 index 000000000..726e2a040 --- /dev/null +++ b/apps/roam/src/utils/__tests__/fixtures/fileReferenceHarness.ts @@ -0,0 +1,170 @@ +import { vi } from "vitest"; +import type { DGSupabaseClient } from "@repo/database/lib/client"; + +export const IMAGE = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2FlqP2ioVNC3.png?alt=media&token=9f1c07a4"; + +type Row = { + space_id?: unknown; + source_local_id?: unknown; + filepath?: unknown; + filehash?: unknown; + source_path?: unknown; +}; + +/** + * A stand-in for Supabase covering what the stage leans on: `my_file_references` answers + * per node, `file_exists` answers from the rows already written, and a delete honours the + * `eq`/`notIn` filters so cleanup can be asserted rather than assumed. + */ +export const makeClient = () => { + const rows: Row[] = []; + const upload = vi.fn().mockResolvedValue({ error: null }); + const thenable = (result: unknown) => ({ + then: (resolve: (value: unknown) => unknown) => + Promise.resolve(result).then(resolve), + }); + + type Filter = (row: Row) => boolean; + const filtered = (filters: Filter[]) => + rows.filter((row) => filters.every((f) => f(row))); + + /** Set to make the reference read fail, as an offline client would. */ + let selectError: { message: string } | null = null; + + const selects: number[] = []; + const deletes: number[] = []; + + const selectBuilder = (filters: Filter[]) => { + const builder = { + eq: (column: string, value: unknown) => + selectBuilder([ + ...filters, + (row) => row[column as keyof Row] === value, + ]), + in: (column: string, values: unknown[]) => + selectBuilder([ + ...filters, + (row) => values.includes(row[column as keyof Row]), + ]), + then: (resolve: (value: unknown) => unknown) => { + selects.push(1); + return Promise.resolve( + selectError + ? { data: null, error: selectError } + : { data: filtered(filters), error: null }, + ).then(resolve); + }, + }; + return builder; + }; + + const deleteBuilder = (filters: Filter[]) => ({ + eq: (column: string, value: unknown) => + deleteBuilder([...filters, (row) => row[column as keyof Row] === value]), + notIn: (column: string, values: unknown[]) => + deleteBuilder([ + ...filters, + (row) => !values.includes(row[column as keyof Row]), + ]), + then: (resolve: (value: unknown) => unknown) => { + deletes.push(1); + for (const row of filtered(filters)) rows.splice(rows.indexOf(row), 1); + return Promise.resolve({ error: null }).then(resolve); + }, + }); + + /** Set to make the content upload fail, so what follows it can be asserted. */ + let contentUploadError: { message: string } | null = null; + + const rpc = vi.fn((fn: string, { hashvalue }: { hashvalue?: string }) => + Promise.resolve( + fn === "upsert_content" && contentUploadError + ? { data: null, error: contentUploadError } + : { data: rows.some((row) => row.filehash === hashvalue), error: null }, + ), + ); + const tableOperations = () => ({ + select: vi.fn(() => selectBuilder([])), + delete: vi.fn(() => deleteBuilder([])), + insert: vi.fn((row: Row) => { + rows.push({ ...row }); + return thenable({ error: null }); + }), + update: vi.fn(() => { + const builder = { + eq: vi.fn(() => builder), + then: thenable({ error: null }).then, + }; + return builder; + }), + }); + // Typed with the table name so a test can assert which table was touched. + const from = + vi.fn<(table: string) => ReturnType>( + tableOperations, + ); + + const client = { + rpc, + storage: { from: vi.fn(() => ({ upload })) }, + from, + } as unknown as DGSupabaseClient; + return { + client, + rpc, + from, + rows, + upload, + filepaths: () => rows.map((r) => r.filepath), + selectCount: () => selects.length, + deleteCount: () => deletes.length, + failReferenceRead: (message: string) => { + selectError = { message }; + }, + failContentUpload: (message: string) => { + contentUploadError = { message }; + }, + }; +}; + +/** + * Both reads an asset takes: the descriptor over `fetch`, and the bytes through Roam. + * Returns Roam's `get` so a test can assert that nothing was transferred. + */ +export const mockAssetReads = ({ + size = 7, + bytes = "PNGDATA", + descriptorOk = true, +}: { + size?: number; + bytes?: string; + descriptorOk?: boolean; +}) => { + vi.stubGlobal( + "fetch", + vi.fn(() => + Promise.resolve({ + ok: descriptorOk, + status: descriptorOk ? 200 : 500, + json: () => + Promise.resolve({ + name: "imgs/app/MAPLab/lqP2ioVNC3.png", + contentType: "image/png", + size: String(size), + metadata: { "file-name": "diagram.png" }, + }), + } as unknown as Response), + ), + ); + const get = vi.fn(() => + Promise.resolve(new File([bytes], "diagram.png", { type: "image/png" })), + ); + vi.stubGlobal("window", { + roamAlphaAPI: { + file: { get }, + graph: { name: "MAPLab", isEncrypted: false }, + }, + }); + return { get }; +}; diff --git a/apps/roam/src/utils/__tests__/publishNodeAssets.test.ts b/apps/roam/src/utils/__tests__/publishNodeAssets.test.ts index 782d453bd..c5b3a3ab1 100644 --- a/apps/roam/src/utils/__tests__/publishNodeAssets.test.ts +++ b/apps/roam/src/utils/__tests__/publishNodeAssets.test.ts @@ -1,119 +1,13 @@ 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 { MAX_ASSET_BYTES } from "@repo/database/lib/assetLimits"; import { publishNodeAssets, summarizeAssetResults } from "../publishNodeAssets"; - -const IMAGE = - "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2FlqP2ioVNC3.png?alt=media&token=9f1c07a4"; - -type Row = { - space_id?: unknown; - source_local_id?: unknown; - filepath?: unknown; - filehash?: unknown; - source_path?: unknown; -}; - -/** - * A stand-in for Supabase covering what the stage leans on: `my_file_references` answers - * per node, `file_exists` answers from the rows already written, and a delete honours the - * `eq`/`notIn` filters so cleanup can be asserted rather than assumed. - */ -const makeClient = () => { - const rows: Row[] = []; - const upload = vi.fn().mockResolvedValue({ error: null }); - const thenable = (result: unknown) => ({ - then: (resolve: (value: unknown) => unknown) => - Promise.resolve(result).then(resolve), - }); - - type Filter = (row: Row) => boolean; - const filtered = (filters: Filter[]) => - rows.filter((row) => filters.every((f) => f(row))); - - /** Set to make the reference read fail, as an offline client would. */ - let selectError: { message: string } | null = null; - - const selects: number[] = []; - const deletes: number[] = []; - - const selectBuilder = (filters: Filter[]) => { - const builder = { - eq: (column: string, value: unknown) => - selectBuilder([ - ...filters, - (row) => row[column as keyof Row] === value, - ]), - in: (column: string, values: unknown[]) => - selectBuilder([ - ...filters, - (row) => values.includes(row[column as keyof Row]), - ]), - then: (resolve: (value: unknown) => unknown) => { - selects.push(1); - return Promise.resolve( - selectError - ? { data: null, error: selectError } - : { data: filtered(filters), error: null }, - ).then(resolve); - }, - }; - return builder; - }; - - const deleteBuilder = (filters: Filter[]) => ({ - eq: (column: string, value: unknown) => - deleteBuilder([...filters, (row) => row[column as keyof Row] === value]), - notIn: (column: string, values: unknown[]) => - deleteBuilder([ - ...filters, - (row) => !values.includes(row[column as keyof Row]), - ]), - then: (resolve: (value: unknown) => unknown) => { - deletes.push(1); - for (const row of filtered(filters)) rows.splice(rows.indexOf(row), 1); - return Promise.resolve({ error: null }).then(resolve); - }, - }); - - 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 })) }, - from: vi.fn(() => ({ - select: vi.fn(() => selectBuilder([])), - delete: vi.fn(() => deleteBuilder([])), - insert: vi.fn((row: Row) => { - rows.push({ ...row }); - return thenable({ error: null }); - }), - update: vi.fn(() => { - const builder = { - eq: vi.fn(() => builder), - then: thenable({ error: null }).then, - }; - return builder; - }), - })), - } as unknown as DGSupabaseClient; - return { - client, - rows, - upload, - filepaths: () => rows.map((r) => r.filepath), - selectCount: () => selects.length, - deleteCount: () => deletes.length, - failReferenceRead: (message: string) => { - selectError = { message }; - }, - }; -}; +import { + IMAGE, + makeClient, + mockAssetReads, +} from "./fixtures/fileReferenceHarness"; const nodeWith = (markdown: string): CrossAppNode => ({ localId: "tgWb6JozF", @@ -130,47 +24,6 @@ const nodeWith = (markdown: string): CrossAppNode => ({ const MARKDOWN = `# Sleep improves memory consolidation\n\n![](${IMAGE})\n\n- Supported by [[EVD]] - Rasch & Born 2013\n`; -/** - * Both reads an asset takes: the descriptor over `fetch`, and the bytes through Roam. - * Returns Roam's `get` so a test can assert that nothing was transferred. - */ -const mockAssetReads = ({ - size = 7, - bytes = "PNGDATA", - descriptorOk = true, -}: { - size?: number; - bytes?: string; - descriptorOk?: boolean; -}) => { - vi.stubGlobal( - "fetch", - vi.fn(() => - Promise.resolve({ - ok: descriptorOk, - status: descriptorOk ? 200 : 500, - json: () => - Promise.resolve({ - name: "imgs/app/MAPLab/lqP2ioVNC3.png", - contentType: "image/png", - size: String(size), - metadata: { "file-name": "diagram.png" }, - }), - } as unknown as Response), - ), - ); - const get = vi.fn(() => - Promise.resolve(new File([bytes], "diagram.png", { type: "image/png" })), - ); - vi.stubGlobal("window", { - roamAlphaAPI: { - file: { get }, - graph: { name: "MAPLab", isEncrypted: false }, - }, - }); - return { get }; -}; - describe("publishNodeAssets", () => { let harness: ReturnType; diff --git a/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts b/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts new file mode 100644 index 000000000..33bdf08d7 --- /dev/null +++ b/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts @@ -0,0 +1,173 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { CrossAppNode } from "@repo/database/crossAppContracts"; +import { contentTypes } from "@repo/content-model"; +import type { RoamFullContentNode } from "~/utils/convertRoamNodeToFullContent"; +import type { SupabaseContext } from "~/utils/supabaseContext"; +import { + IMAGE, + makeClient, + mockAssetReads, +} from "./fixtures/fileReferenceHarness"; + +const mocks = vi.hoisted(() => ({ + /** The markdown Roam would render for each node, keyed by uid. */ + markdownByUid: new Map(), +})); + +// The sync module reads Roam globals when these load. None of them is on the path +// under test. +vi.mock("~/utils/getDiscourseNodes", () => ({ default: () => [] })); +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); +vi.mock("~/components/settings/utils/accessors", () => ({ + isSyncEnabled: () => false, +})); + +vi.mock("~/utils/roamToCrossAppConverters", () => ({ + fullContentNodeToCrossApp: (node: RoamFullContentNode): CrossAppNode => ({ + localId: node.source_local_id, + authorId: node.author_local_id, + nodeType: node.node_type_id, + coreTitle: node.text, + createdAt: new Date(node.created), + modifiedAt: new Date(node.last_modified), + content: { + direct: { localId: node.source_local_id, value: node.text }, + full: { + localId: node.source_local_id, + value: mocks.markdownByUid.get(node.source_local_id) ?? "", + contentType: contentTypes.roamMarkdown, + scale: "document", + }, + }, + }), +})); + +import { upsertSharedNodesFullContentWithAssets } from "~/utils/syncDgNodesToSupabase"; + +const NODE_UID = "tgWb6JozF"; +const SECOND_IMAGE = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2FsecondImage.png?alt=media&token=1a2b3c4d"; + +const sharedNode: RoamFullContentNode = { + author_local_id: "maparent", + source_local_id: NODE_UID, + created: Date.parse("2026-06-12T14:00:00.000Z"), + last_modified: Date.parse("2026-06-12T15:00:00.000Z"), + text: "[[CLM]] - Sleep improves memory consolidation", + node_type_id: "rCLM0schema", + format: "[[CLM]] - {content}", +}; + +const context = { spaceId: 20, userId: 7 } as SupabaseContext; + +const withImages = (...images: string[]): string => + [ + "# Sleep improves memory consolidation", + ...images.map((image) => `![](${image})`), + "- Supported by [[EVD]] - Rasch & Born 2013", + ].join("\n\n"); + +describe("upsertSharedNodesFullContentWithAssets", () => { + let harness: ReturnType; + + const syncWithMarkdown = async (markdown: string) => { + mocks.markdownByUid.set(NODE_UID, markdown); + return upsertSharedNodesFullContentWithAssets({ + nodes: [sharedNode], + supabaseClient: harness.client, + context, + phases: {}, + }); + }; + + beforeEach(() => { + harness = makeClient(); + mocks.markdownByUid.clear(); + mockAssetReads({}); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("records a reference for an image added after the node was shared", async () => { + await syncWithMarkdown(withImages()); + expect(harness.rows).toHaveLength(0); + + await syncWithMarkdown(withImages(IMAGE)); + + expect(harness.rows).toEqual([ + expect.objectContaining({ source_local_id: NODE_UID, filepath: IMAGE }), + ]); + }); + + it("uploads the full content before recording references", async () => { + await syncWithMarkdown(withImages(IMAGE)); + + const { rpc, from } = harness; + const contentUpload = rpc.mock.calls.findIndex( + ([fn]) => fn === "upsert_content", + ); + const referenceWrite = from.mock.calls.findIndex( + ([table]) => table === "FileReference", + ); + expect(contentUpload).toBeGreaterThanOrEqual(0); + expect(referenceWrite).toBeGreaterThanOrEqual(0); + expect(rpc.mock.invocationCallOrder[contentUpload]).toBeLessThan( + from.mock.invocationCallOrder[referenceWrite], + ); + }); + + it("records no reference when the content upload fails", async () => { + // FileReference has a foreign key to Content, so a reference must never outlive a + // failed content upload. + harness.failContentUpload("upsert_content failed"); + + await expect(syncWithMarkdown(withImages(IMAGE))).rejects.toThrow(); + + expect(harness.rows).toHaveLength(0); + expect( + harness.from.mock.calls.some(([table]) => table === "FileReference"), + ).toBe(false); + }); + + it("neither adds nor removes references when only the text changes", async () => { + await syncWithMarkdown(withImages(IMAGE)); + const rowsBefore = [...harness.rows]; + + await syncWithMarkdown(`${withImages(IMAGE)}\n\n- A new bullet`); + + expect(harness.rows).toEqual(rowsBefore); + expect(harness.deleteCount()).toBe(0); + }); + + it("removes the reference to an image the node no longer embeds", async () => { + await syncWithMarkdown(withImages(IMAGE, SECOND_IMAGE)); + expect(harness.filepaths()).toEqual([IMAGE, SECOND_IMAGE]); + + await syncWithMarkdown(withImages(SECOND_IMAGE)); + + expect(harness.filepaths()).toEqual([SECOND_IMAGE]); + }); + + it("does nothing when no shared node changed", async () => { + // Timed even when empty, so a sync with no shared changes still reports both + // phases rather than dropping them from the series. + const phases: Record = {}; + + await expect( + upsertSharedNodesFullContentWithAssets({ + nodes: [], + supabaseClient: harness.client, + context, + phases, + }), + ).resolves.toEqual([]); + + expect(harness.rpc).not.toHaveBeenCalled(); + expect(Object.keys(phases)).toEqual([ + "upsertFullContent", + "publishSharedNodeAssets", + ]); + }); +}); diff --git a/apps/roam/src/utils/convertRoamNodeToFullContent.ts b/apps/roam/src/utils/convertRoamNodeToFullContent.ts index 3f55110a8..e0c97c9a1 100644 --- a/apps/roam/src/utils/convertRoamNodeToFullContent.ts +++ b/apps/roam/src/utils/convertRoamNodeToFullContent.ts @@ -1,3 +1,4 @@ +import type { CrossAppNode } from "@repo/database/crossAppContracts"; import { crossAppNodeToDbContent } from "@repo/database/lib/crossAppConverters"; import { fullContentNodeToCrossApp } from "./roamToCrossAppConverters"; import type { LocalContentDataInput } from "@repo/database/inputTypes"; @@ -13,16 +14,24 @@ export type RoamFullContentNode = { node_title?: string; }; +/** Paired so the asset stage reuses the markdown rather than rebuilding it from Roam. */ +export type ConvertedFullContent = { + node: CrossAppNode; + content: LocalContentDataInput; +}; + export const convertRoamNodeToFullContent = ({ nodes, }: { nodes: RoamFullContentNode[]; -}): LocalContentDataInput[] => +}): ConvertedFullContent[] => nodes.flatMap((node) => { try { const crossAppNode = fullContentNodeToCrossApp(node); const fullContent = crossAppNodeToDbContent(crossAppNode, "full"); - return fullContent === undefined ? [] : [fullContent]; + return fullContent === undefined + ? [] + : [{ node: crossAppNode, content: fullContent }]; } catch (error) { console.error( `convertRoamNodeToFullContent: failed to build full markdown for ${node.source_local_id}:`, diff --git a/apps/roam/src/utils/syncDgNodesToSupabase.ts b/apps/roam/src/utils/syncDgNodesToSupabase.ts index 7fbc8ba55..843d74a72 100644 --- a/apps/roam/src/utils/syncDgNodesToSupabase.ts +++ b/apps/roam/src/utils/syncDgNodesToSupabase.ts @@ -24,6 +24,11 @@ import { convertRoamNodeToFullContent, type RoamFullContentNode, } from "./convertRoamNodeToFullContent"; +import { + publishNodeAssets, + summarizeAssetResults, + type NodeAssetResult, +} from "./publishNodeAssets"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import { intersection } from "@repo/utils/setOperations"; import { CORE_TITLE_PROBE_SELECT } from "@repo/database/lib/coreTitleBackfill"; @@ -804,24 +809,44 @@ const upsertNodesToSupabaseAsContent = async ( await uploadContentBatches({ content, supabaseClient, context }); }; -const upsertRoamNodesToSupabaseAsFullContent = async ({ +/** The asset stage runs here too, so an asset added after sharing still gets a row. */ +export const upsertSharedNodesFullContentWithAssets = async ({ nodes, supabaseClient, context, + phases, }: { nodes: RoamFullContentNode[]; supabaseClient: DGSupabaseClient; context: SupabaseContext; -}): Promise => { - if (nodes.length === 0) { - return; - } - - const fullContent = convertRoamNodeToFullContent({ nodes }); - await uploadContentBatches({ - content: fullContent, - supabaseClient, - context, + phases: SyncPhaseDurations; +}): Promise => { + // Building the markdown is the expensive half of the upload, so it stays inside the + // phase it has always been timed under. + const converted = await measureSyncPhase({ + phase: "upsertFullContent", + phases, + operation: async () => { + const converted = convertRoamNodeToFullContent({ nodes }); + await uploadContentBatches({ + content: converted.map(({ content }) => content), + supabaseClient, + context, + }); + return converted; + }, + }); + // A failed upload throws above, so every converted node now has the Content row + // that publishNodeAssets requires. + return measureSyncPhase({ + phase: "publishSharedNodeAssets", + phases, + operation: () => + publishNodeAssets({ + client: supabaseClient, + spaceId: context.spaceId, + nodes: converted.map(({ node }) => node), + }), }); }; @@ -904,6 +929,29 @@ const reportCoreTitleBackfill = ({ }); }; +/** + * A failed copy is not retried until the node changes again, so the counts are the only + * standing signal that a shared node's asset never reached storage. + */ +const reportSharedNodeAssets = (results: NodeAssetResult[]): void => { + if (results.length === 0) return; + const { copied, unchanged, distinctBlobs, tooLarge, failed } = + summarizeAssetResults(results); + posthog.capture("Sync shared node assets", { + copied, + unchanged, + distinctBlobs, + tooLarge: tooLarge.length, + failed: failed.length, + }); + if (failed.length > 0) { + console.warn( + `Sync could not copy ${failed.length} shared node assets`, + failed, + ); + } +}; + const getAllMissingOrNewDiscourseNodes = async ({ supabaseClient, spaceId, @@ -1445,16 +1493,14 @@ export const createOrUpdateDiscourseEmbedding = async ( activeContext, ), }); - await measureSyncPhase({ - phase: "upsertFullContent", - phases, - operation: () => - upsertRoamNodesToSupabaseAsFullContent({ - nodes: sharedFullContentNodes, - supabaseClient: activeSupabaseClient, - context: activeContext, - }), - }); + reportSharedNodeAssets( + await upsertSharedNodesFullContentWithAssets({ + nodes: sharedFullContentNodes, + supabaseClient: activeSupabaseClient, + context: activeContext, + phases, + }), + ); await measureSyncPhase({ phase: "convertConcepts", phases,