diff --git a/apps/roam/src/utils/__tests__/assetRegistry.test.ts b/apps/roam/src/utils/__tests__/assetRegistry.test.ts new file mode 100644 index 000000000..2739a5354 --- /dev/null +++ b/apps/roam/src/utils/__tests__/assetRegistry.test.ts @@ -0,0 +1,243 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; +import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; +import createBlock from "roamjs-components/writes/createBlock"; +import createPage from "roamjs-components/writes/createPage"; +import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; +import type { json } from "~/utils/getBlockProps"; +import { + ASSET_REGISTRY_BLOCK_TEXT, + ASSET_REGISTRY_PAGE_TITLE, + ASSET_REGISTRY_PROP_KEY, + readAssetRegistry, + readMirroredAssetUrl, + recordMirroredAsset, +} from "~/utils/assetRegistry"; + +vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ + default: vi.fn(), +})); +vi.mock("roamjs-components/queries/getShallowTreeByParentUid", () => ({ + default: vi.fn(), +})); +vi.mock("roamjs-components/writes/createBlock", () => ({ default: vi.fn() })); +vi.mock("roamjs-components/writes/createPage", () => ({ default: vi.fn() })); + +const mockedGetPageUidByPageTitle = vi.mocked(getPageUidByPageTitle); +const mockedGetShallowTreeByParentUid = vi.mocked(getShallowTreeByParentUid); +const mockedCreateBlock = vi.mocked(createBlock); +const mockedCreatePage = vi.mocked(createPage); + +const PAGE_UID = "registry-page-uid"; +const BLOCK_UID = "registry-block-uid"; +const HASH = "a".repeat(64); +const OTHER_HASH = "b".repeat(64); +const URL = "https://firebasestorage.googleapis.com/v0/b/x/o/one?alt=media"; +const OTHER_URL = + "https://firebasestorage.googleapis.com/v0/b/x/o/two?alt=media"; + +const propsByUid = new Map>(); + +/** + * A stand-in for the graph: pages and their children exist only once something has + * created them, so an absent registry behaves the way a fresh graph does. + */ +const graph = { pages: new Map() }; + +const setRoamAlphaApi = (): void => { + (globalThis as { window: unknown }).window = { + roamAlphaAPI: { + data: { + block: { + update: ({ + block, + }: { + block: { uid: string; props: Record }; + }) => { + propsByUid.set(block.uid, block.props); + return Promise.resolve(); + }, + }, + }, + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": propsByUid.get(uid) ?? {}, + }), + }, + }; +}; + +const originalWindow = (globalThis as { window?: unknown }).window; + +afterEach(() => { + // Vitest isolates files by default, so this matters only if isolation is off or this + // suite is merged into another: a leaked fake roamAlphaAPI would fail a distant file. + (globalThis as { window?: unknown }).window = originalWindow; + vi.restoreAllMocks(); +}); + +beforeEach(() => { + propsByUid.clear(); + graph.pages.clear(); + vi.clearAllMocks(); + setRoamAlphaApi(); + + mockedGetPageUidByPageTitle.mockImplementation((title: string) => + graph.pages.has(title) ? PAGE_UID : "", + ); + mockedGetShallowTreeByParentUid.mockImplementation((parentUid: string) => + parentUid === PAGE_UID + ? (graph.pages.get(ASSET_REGISTRY_PAGE_TITLE) ?? []) + : [], + ); + mockedCreatePage.mockImplementation(({ title }: { title: string }) => { + graph.pages.set(title, []); + return Promise.resolve(PAGE_UID); + }); + mockedCreateBlock.mockImplementation( + ({ node }: { node: { text?: string } }) => { + graph.pages + .get(ASSET_REGISTRY_PAGE_TITLE) + ?.push({ uid: BLOCK_UID, text: node.text ?? "" }); + return Promise.resolve(BLOCK_UID); + }, + ); +}); + +describe("asset registry", () => { + it("returns an empty registry, and creates nothing, when the page is absent", () => { + expect(readAssetRegistry()).toEqual({}); + expect(readMirroredAssetUrl(HASH)).toBeUndefined(); + expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(mockedCreateBlock).not.toHaveBeenCalled(); + }); + + it("creates the page and the named block on the first write", async () => { + await recordMirroredAsset({ contentHash: HASH, url: URL }); + + expect(mockedCreatePage).toHaveBeenCalledWith({ + title: ASSET_REGISTRY_PAGE_TITLE, + }); + expect(mockedCreateBlock).toHaveBeenCalledWith( + expect.objectContaining({ + node: { text: ASSET_REGISTRY_BLOCK_TEXT }, + parentUid: PAGE_UID, + }), + ); + expect(propsByUid.get(BLOCK_UID)).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: { + [ASSET_REGISTRY_PROP_KEY]: { [HASH]: URL }, + }, + }); + }); + + it("reads back what it wrote, accumulating across writes", async () => { + await recordMirroredAsset({ contentHash: HASH, url: URL }); + await recordMirroredAsset({ contentHash: OTHER_HASH, url: OTHER_URL }); + + expect(readAssetRegistry()).toEqual({ + [HASH]: URL, + [OTHER_HASH]: OTHER_URL, + }); + expect(readMirroredAssetUrl(OTHER_HASH)).toBe(OTHER_URL); + }); + + it("reuses the existing block rather than creating a second one", async () => { + await recordMirroredAsset({ contentHash: HASH, url: URL }); + mockedCreatePage.mockClear(); + mockedCreateBlock.mockClear(); + + await recordMirroredAsset({ contentHash: OTHER_HASH, url: OTHER_URL }); + + expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(mockedCreateBlock).not.toHaveBeenCalled(); + }); + + it("leaves unrelated discourse-graph props on the block untouched", async () => { + graph.pages.set(ASSET_REGISTRY_PAGE_TITLE, [ + { uid: BLOCK_UID, text: ASSET_REGISTRY_BLOCK_TEXT }, + ]); + propsByUid.set(BLOCK_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: { somethingElse: "keep me" }, + }); + + await recordMirroredAsset({ contentHash: HASH, url: URL }); + + expect(propsByUid.get(BLOCK_UID)).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: { + somethingElse: "keep me", + [ASSET_REGISTRY_PROP_KEY]: { [HASH]: URL }, + }, + }); + }); + + it("ignores a malformed registry rather than throwing", () => { + graph.pages.set(ASSET_REGISTRY_PAGE_TITLE, [ + { uid: BLOCK_UID, text: ASSET_REGISTRY_BLOCK_TEXT }, + ]); + propsByUid.set(BLOCK_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: { + [ASSET_REGISTRY_PROP_KEY]: { [HASH]: 7, [OTHER_HASH]: OTHER_URL }, + }, + }); + + expect(readAssetRegistry()).toEqual({ [OTHER_HASH]: OTHER_URL }); + }); + + it("ignores a block on the page that is not the registry block", () => { + // A page with blocks but no registry block is the orphan condition, so this warns. + // Asserted below; muted here. + vi.spyOn(console, "warn").mockImplementation(() => {}); + graph.pages.set(ASSET_REGISTRY_PAGE_TITLE, [ + { uid: "other-block", text: "Some note the user wrote" }, + ]); + propsByUid.set("other-block", { + [DISCOURSE_GRAPH_PROP_NAME]: { + [ASSET_REGISTRY_PROP_KEY]: { [HASH]: URL }, + }, + }); + + expect(readAssetRegistry()).toEqual({}); + }); + + it("warns once when the page outlives its registry block, however many lookups follow", async () => { + // Read back first, so the warning is known unspent whatever ran before this test. + await recordMirroredAsset({ contentHash: HASH, url: URL }); + expect(readAssetRegistry()).toEqual({ [HASH]: URL }); + + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + graph.pages.set(ASSET_REGISTRY_PAGE_TITLE, [ + { uid: BLOCK_UID, text: "Sync Asset Registry (renamed)" }, + ]); + + expect(readAssetRegistry()).toEqual({}); + expect(readMirroredAssetUrl(HASH)).toBeUndefined(); + expect(readMirroredAssetUrl(OTHER_HASH)).toBeUndefined(); + + expect(warn).toHaveBeenCalledTimes(1); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining(ASSET_REGISTRY_BLOCK_TEXT), + ); + }); + + it("recreates an orphaned registry block without announcing a future repair", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + graph.pages.set(ASSET_REGISTRY_PAGE_TITLE, [ + { uid: "some-other-block", text: "Sync Asset Registry (renamed)" }, + ]); + + await recordMirroredAsset({ contentHash: HASH, url: URL }); + + expect(mockedCreatePage).not.toHaveBeenCalled(); + expect(mockedCreateBlock).toHaveBeenCalledWith( + expect.objectContaining({ parentUid: PAGE_UID }), + ); + expect(warn).not.toHaveBeenCalled(); + }); + + it("says nothing on a graph that has never imported an asset", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + expect(readAssetRegistry()).toEqual({}); + expect(warn).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/roam/src/utils/assetRegistry.ts b/apps/roam/src/utils/assetRegistry.ts new file mode 100644 index 000000000..be912a69c --- /dev/null +++ b/apps/roam/src/utils/assetRegistry.ts @@ -0,0 +1,146 @@ +import createBlock from "roamjs-components/writes/createBlock"; +import createPage from "roamjs-components/writes/createPage"; +import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; +import getShallowTreeByParentUid from "roamjs-components/queries/getShallowTreeByParentUid"; +import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; +import getBlockProps, { isJsonObject } from "./getBlockProps"; +import { setBlockPropsAsync } from "./setBlockProps"; + +/** + * A graph-level memo of the assets this graph has already mirrored into Roam storage. + * + * `file.upload` writes a fresh random URL on every call, so without a memo an asset + * referenced by three imported nodes becomes three blobs, and every re-import adds + * another. It keys on the content hash rather than the reference, because shared storage + * deduplicates by hash and keying on the reference re-inflates what the bucket collapsed. + * + * A cache, not a source of truth: a lost registry costs a rebuild, not data. A page's + * URLs are recoverable from its own blocks, and a URL's asset from the uploaded file + * name, `imported-`. + */ + +export const ASSET_REGISTRY_PAGE_TITLE = + "roam/js/discourse-graph/imported-assets"; +export const ASSET_REGISTRY_BLOCK_TEXT = "Sync Asset Registry"; +export const ASSET_REGISTRY_PROP_KEY = "assetRegistry"; + +/** SHA-256 of the shared-storage bytes -> the URL of this graph's own copy. */ +export type AssetRegistry = Record; + +/** + * The registry is addressed by page title and block text, both of which a user can edit + * or delete and neither of which the extension can defend. + */ +const locateRegistry = (): { + pageUid: string | undefined; + blockUid: string | undefined; +} => { + const pageUid = getPageUidByPageTitle(ASSET_REGISTRY_PAGE_TITLE) || undefined; + if (!pageUid) return { pageUid: undefined, blockUid: undefined }; + + return { + pageUid, + blockUid: getShallowTreeByParentUid(pageUid).find( + ({ text }) => text === ASSET_REGISTRY_BLOCK_TEXT, + )?.uid, + }; +}; + +/** + * An orphaned registry is one condition, not one per asset, so a reader looking up N + * hashes is told once. It is survivable, since the next write recreates the registry, + * but silent otherwise, and a graph that re-uploads everything it already holds deserves + * an explanation. + * + * Cleared by a reachable registry, so a graph orphaned, repaired, and orphaned again + * warns twice, which is true both times. Only the detectable half warns: a page that + * outlived its block. A deleted page is indistinguishable from a graph that has never + * imported an asset, and Roam reaps a page left with no blocks, so that case passes + * silently and costs one round of re-uploads. + */ +let hasWarnedOrphanedRegistry = false; + +const warnOrphanedRegistry = (): void => { + if (hasWarnedOrphanedRegistry) return; + hasWarnedOrphanedRegistry = true; + console.warn( + `The Discourse Graph asset registry is unreachable: [[${ASSET_REGISTRY_PAGE_TITLE}]] has no block reading "${ASSET_REGISTRY_BLOCK_TEXT}". Anything recorded there is lost, and every asset this graph already holds will be uploaded again.`, + ); +}; + +const getOrCreateRegistryBlockUid = async (): Promise => { + const { pageUid, blockUid } = locateRegistry(); + if (blockUid) return blockUid; + + // No warning on this path: it recreates what it found missing, in this same call. + return createBlock({ + node: { text: ASSET_REGISTRY_BLOCK_TEXT }, + parentUid: + pageUid ?? (await createPage({ title: ASSET_REGISTRY_PAGE_TITLE })), + order: "last", + }); +}; + +const registryFromProps = (blockUid: string): AssetRegistry => { + const discourseGraphProps = + getBlockProps(blockUid)[DISCOURSE_GRAPH_PROP_NAME]; + if (!isJsonObject(discourseGraphProps)) return {}; + + const registry = discourseGraphProps[ASSET_REGISTRY_PROP_KEY]; + if (!isJsonObject(registry)) return {}; + + return Object.fromEntries( + Object.entries(registry).filter( + (entry): entry is [string, string] => typeof entry[1] === "string", + ), + ); +}; + +/** + * Reads the registry without touching the graph. An absent page or block is an empty + * registry, not something to create. Only a write brings the page into being. + */ +export const readAssetRegistry = (): AssetRegistry => { + const { pageUid, blockUid } = locateRegistry(); + if (blockUid) { + hasWarnedOrphanedRegistry = false; + return registryFromProps(blockUid); + } + if (pageUid) warnOrphanedRegistry(); + return {}; +}; + +export const readMirroredAssetUrl = (contentHash: string): string | undefined => + readAssetRegistry()[contentHash]; + +/** + * Records the URL of this graph's copy of an asset, creating the registry page and + * block if they are absent. + * + * Read-modify-write, deliberately not serialized. Two overlapping writes (two tabs on + * the same graph, or a future caller that stops awaiting each asset) keep only the last + * entry, costing one redundant upload on the next import, which ENG-2216 priced in. The + * merge guarantees something narrower: sibling keys under `DISCOURSE_GRAPH_PROP_NAME` + * survive, because the props are re-read here rather than replaced wholesale. + */ +export const recordMirroredAsset = async ({ + contentHash, + url, +}: { + contentHash: string; + url: string; +}): Promise => { + const blockUid = await getOrCreateRegistryBlockUid(); + const discourseGraphProps = + getBlockProps(blockUid)[DISCOURSE_GRAPH_PROP_NAME]; + + await setBlockPropsAsync(blockUid, { + [DISCOURSE_GRAPH_PROP_NAME]: { + ...(isJsonObject(discourseGraphProps) ? discourseGraphProps : {}), + [ASSET_REGISTRY_PROP_KEY]: { + ...registryFromProps(blockUid), + [contentHash]: url, + }, + }, + }); +}; diff --git a/apps/roam/src/utils/getBlockProps.ts b/apps/roam/src/utils/getBlockProps.ts index f8c839f1c..982cc3c2a 100644 --- a/apps/roam/src/utils/getBlockProps.ts +++ b/apps/roam/src/utils/getBlockProps.ts @@ -6,6 +6,18 @@ export type json = | json[] | { [key: string]: json }; +/** + * Whether a props value is a plain JSON object, and so safe to spread or index. + * + * Roam hands back whatever was written, so every step into a props tree has to be + * narrowed. `undefined` is accepted because an absent key yields it under + * `noUncheckedIndexedAccess`, and "not an object" is the right answer for it. + */ +export const isJsonObject = ( + value: json | undefined, +): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + export const normalizeProps = (props: json): json => typeof props === "object" ? props === null diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index 0b3775f25..efc7aecc1 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -1,6 +1,6 @@ import type { Rid } from "@repo/database/crossAppContracts"; import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; -import getBlockProps, { type json } from "./getBlockProps"; +import getBlockProps, { isJsonObject, type json } from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; export type ImportedSourceIdentity = { @@ -12,9 +12,6 @@ export const IMPORTED_FROM_PROP_KEY = "importedFrom"; const SOURCE_NODE_RID_KEY = "sourceNodeRid"; const SOURCE_MODIFIED_AT_KEY = "sourceModifiedAt"; -const isJsonObject = (value: json): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); - const parseImportedSourceIdentity = ( props: Record, ): ImportedSourceIdentity | undefined => {