From d20d38e33cfb1a33f22bbcedfa65a39d417986df Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 31 Aug 2026 17:14:24 +0530 Subject: [PATCH 01/17] ENG-2131 Review and accept imported relation types and triples in Roam --- .../src/components/CreateRelationDialog.tsx | 3 +- apps/roam/src/components/SuggestionsBody.tsx | 6 +- .../DiscourseRelationTool.tsx | 8 +- .../DiscourseRelationUtil.tsx | 8 +- apps/roam/src/components/canvas/Tldraw.tsx | 18 ++- .../roam/src/components/canvas/canvasUtils.ts | 6 +- .../settings/DiscourseRelationConfigPanel.tsx | 146 +++++++++++++++++- .../relationSchemaAcceptance.test.ts | 132 ++++++++++++++++ apps/roam/src/utils/publishNodesToGroups.ts | 7 +- .../src/utils/relationSchemaAcceptance.ts | 59 +++++++ 10 files changed, 382 insertions(+), 11 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts create mode 100644 apps/roam/src/utils/relationSchemaAcceptance.ts diff --git a/apps/roam/src/components/CreateRelationDialog.tsx b/apps/roam/src/components/CreateRelationDialog.tsx index c1d08b7012..b283161f8f 100644 --- a/apps/roam/src/components/CreateRelationDialog.tsx +++ b/apps/roam/src/components/CreateRelationDialog.tsx @@ -8,6 +8,7 @@ import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageU import getDiscourseRelations, { type DiscourseRelation, } from "~/utils/getDiscourseRelations"; +import { excludeProvisionalRelationSchemas } from "~/utils/relationSchemaAcceptance"; import { createReifiedRelation } from "~/utils/createReifiedBlock"; import { getStoredRelationsEnabled } from "~/utils/storedRelations"; import findDiscourseNode from "~/utils/findDiscourseNode"; @@ -291,7 +292,7 @@ const prepareRelData = ( ): RelWithDirection[] => { nodeTitle = nodeTitle || getPageTitleByPageUid(targetNodeUid).trim(); const discourseNodeSchemas = getDiscourseNodes(); - const relations = getDiscourseRelations(); + const relations = excludeProvisionalRelationSchemas(getDiscourseRelations()); const nodeSchema = findDiscourseNode({ uid: targetNodeUid, title: nodeTitle, diff --git a/apps/roam/src/components/SuggestionsBody.tsx b/apps/roam/src/components/SuggestionsBody.tsx index c81d4dc00d..bcfe68ac79 100644 --- a/apps/roam/src/components/SuggestionsBody.tsx +++ b/apps/roam/src/components/SuggestionsBody.tsx @@ -21,6 +21,7 @@ import getDiscourseContextResults from "~/utils/getDiscourseContextResults"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; import findDiscourseNode from "~/utils/findDiscourseNode"; import getDiscourseRelations from "~/utils/getDiscourseRelations"; +import { excludeProvisionalRelationSchemas } from "~/utils/relationSchemaAcceptance"; import getDiscourseNodes from "~/utils/getDiscourseNodes"; import normalizePageTitle from "roamjs-components/queries/normalizePageTitle"; import { type RelationDetails } from "~/utils/hyde"; @@ -232,7 +233,10 @@ const SuggestionsBody = ({ () => findDiscourseNode({ uid: tagUid }), [tagUid], ); - const allRelations = useMemo(() => getDiscourseRelations(), []); + const allRelations = useMemo( + () => excludeProvisionalRelationSchemas(getDiscourseRelations()), + [], + ); const allNodes = useMemo(() => getDiscourseNodes(), []); const validRelations = useMemo(() => { diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx index 3669a62022..44d11ea5df 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx @@ -350,7 +350,9 @@ export const createAllRelationShapeTools = ( override onEnter = () => { this.didTimeout = false; - const selectedRelations = discourseContext.relations[name] || []; + const selectedRelations = ( + discourseContext.relations[name] || [] + ).filter((r) => !discourseContext.provisionalRelationIds.has(r.id)); const hasIncompleteSelectedRelation = selectedRelations.some( (relation) => !isRelationComplete(relation), ); @@ -384,7 +386,7 @@ export const createAllRelationShapeTools = ( target && isDiscourseNodeShape(target) ? getDiscourseNodeTypeId({ shape: target }) : undefined; - const relation = discourseContext.relations[name].find( + const relation = selectedRelations.find( (r) => r.source === targetNodeTypeId || r.destination === targetNodeTypeId, @@ -392,7 +394,7 @@ export const createAllRelationShapeTools = ( if (relation) { this.shapeType = relation.id; } else { - const acceptableTypes = discourseContext.relations[name] + const acceptableTypes = selectedRelations .flatMap((r) => [ discourseContext.nodes[r.source]?.text, discourseContext.nodes[r.destination]?.text, diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx index 9af71910d3..fa13e49969 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx @@ -1774,7 +1774,9 @@ export class BaseDiscourseRelationUtil extends ShapeUtil isReverse: boolean; matchingRelation: DiscourseRelation | null; } { - const relationsWithLabel = discourseContext.relations[label]; + const relationsWithLabel = discourseContext.relations[label]?.filter( + (r) => !discourseContext.provisionalRelationIds.has(r.id), + ); if (!relationsWithLabel) { return { isDirect: false, isReverse: false, matchingRelation: null }; } @@ -1794,7 +1796,9 @@ export class BaseDiscourseRelationUtil extends ShapeUtil } getValidTargetTypes(label: string, sourceNodeType: string): string[] { - const relationsWithLabel = discourseContext.relations[label]; + const relationsWithLabel = discourseContext.relations[label]?.filter( + (r) => !discourseContext.provisionalRelationIds.has(r.id), + ); if (!relationsWithLabel) return []; const targets = new Set(); diff --git a/apps/roam/src/components/canvas/Tldraw.tsx b/apps/roam/src/components/canvas/Tldraw.tsx index 505833d02b..954887087b 100644 --- a/apps/roam/src/components/canvas/Tldraw.tsx +++ b/apps/roam/src/components/canvas/Tldraw.tsx @@ -120,6 +120,7 @@ import posthog from "posthog-js"; import { getPersonalSetting } from "~/components/settings/utils/accessors"; import { PERSONAL_KEYS } from "~/components/settings/utils/settingKeys"; import { json, normalizeProps } from "~/utils/getBlockProps"; +import { isProvisionalRelationSchema } from "~/utils/relationSchemaAcceptance"; import { onPageRefObserverChange } from "~/utils/pageRefObserverHandlers"; declare global { @@ -133,6 +134,9 @@ export type DiscourseContextType = { nodes: Record; // { [Relation.Label] => DiscourseRelation[] } relations: Record; + // Imported, not-yet-accepted relation schemas; excluded from relation + // creation but kept in `relations` so existing shapes still render. + provisionalRelationIds: Set; lastAppEvent: string; lastActions: HistoryEntry[]; }; @@ -140,6 +144,7 @@ export type DiscourseContextType = { export const discourseContext: DiscourseContextType = { nodes: {}, relations: {}, + provisionalRelationIds: new Set(), lastAppEvent: "", lastActions: [], }; @@ -766,6 +771,11 @@ const TldrawCanvasShared = ({ }, {} as Record, ); + discourseContext.provisionalRelationIds = new Set( + relations + .filter((r) => isProvisionalRelationSchema(r.id)) + .map((r) => r.id), + ); return relations; }, []); const allRelationsById = useMemo(() => { @@ -778,7 +788,13 @@ const TldrawCanvasShared = ({ return Object.keys(allRelationsById); }, [allRelationsById]); const allRelationNames = useMemo(() => { - return Object.keys(discourseContext.relations); + return Object.entries(discourseContext.relations) + .filter(([, relations]) => + relations.some( + (r) => !discourseContext.provisionalRelationIds.has(r.id), + ), + ) + .map(([name]) => name); }, []); const allNodes = useMemo(() => { const allNodes = getDiscourseNodes(); diff --git a/apps/roam/src/components/canvas/canvasUtils.ts b/apps/roam/src/components/canvas/canvasUtils.ts index 5bb71dbbc4..6b065e7fc2 100644 --- a/apps/roam/src/components/canvas/canvasUtils.ts +++ b/apps/roam/src/components/canvas/canvasUtils.ts @@ -16,8 +16,12 @@ export const isDiscourseNodeShape = ( } }; +// Creation-facing list: provisional imported relation schemas are excluded so +// they cannot be used to create new relations on the canvas. export const getAllRelations = () => - Object.values(discourseContext.relations).flat(); + Object.values(discourseContext.relations) + .flat() + .filter((r) => !discourseContext.provisionalRelationIds.has(r.id)); export const checkConnectionType = ( relation: { source: string; destination: string }, diff --git a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx index 3f85a04ba0..e457f80001 100644 --- a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx @@ -10,6 +10,7 @@ import { SpinnerSize, Tab, Tabs, + Tag, Tooltip, HTMLTable, ControlGroup, @@ -65,6 +66,13 @@ import { type RelationSort, type RelationSortColumn, } from "~/utils/sortRelations"; +import { + acceptImportedRelationSchema, + readRelationSchemaImportMeta, + type RelationSchemaImportMeta, +} from "~/utils/relationSchemaAcceptance"; +import { getReifiedRelations } from "~/utils/createReifiedBlock"; +import { ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; const DEFAULT_SELECTED_RELATION = { display: "none", @@ -976,6 +984,16 @@ type Relation = { source: string | undefined; destination: string | undefined; }; +type ImportedRelation = Relation & { importMeta: RelationSchemaImportMeta }; + +const ROAM_SPACE_URI_PREFIX = "https://roamresearch.com/#/app/"; + +const formatImportedSource = (sourceNodeRid: string): string => { + const { spaceUri } = ridToSpaceUriAndLocalId(sourceNodeRid); + return spaceUri.startsWith(ROAM_SPACE_URI_PREFIX) + ? spaceUri.slice(ROAM_SPACE_URI_PREFIX.length) + : spaceUri; +}; const DiscourseRelationConfigPanel = ({ uid, parentUid, @@ -1037,6 +1055,16 @@ const DiscourseRelationConfigPanel = ({ : visibleRelations, [nodes, sort, visibleRelations], ); + const { localRelations, importedRelations } = useMemo(() => { + const local: Relation[] = []; + const imported: ImportedRelation[] = []; + for (const rel of sortedRelations) { + const importMeta = readRelationSchemaImportMeta(rel.uid); + if (importMeta) imported.push({ ...rel, importMeta }); + else local.push(rel); + } + return { localRelations: local, importedRelations: imported }; + }, [sortedRelations]); const editingRelationInfo = useMemo( () => editingRelation ? getFullTreeByParentUid(editingRelation) : undefined, @@ -1079,6 +1107,30 @@ const DiscourseRelationConfigPanel = ({ }, 50); }); }; + const handleAcceptImported = (rel: Relation) => { + void acceptImportedRelationSchema(rel.uid).then(() => { + setRelations(refreshRelations()); + }); + }; + const handleDeleteImported = (rel: Relation) => { + void getReifiedRelations().then((reifiedRelations) => { + const inUseCount = reifiedRelations.filter( + (r) => r.hasSchema === rel.uid, + ).length; + if (inUseCount > 0) { + renderToast({ + id: "discourse-relation-delete-blocked", + intent: Intent.WARNING, + content: `Cannot delete this imported relation: ${inUseCount} relation ${ + inUseCount === 1 ? "instance uses" : "instances use" + } it in this graph.`, + }); + setDeleteConfirmation(null); + return; + } + handleDelete(rel); + }); + }; const handleDuplicate = (rel: Relation) => { const text = rel.text; const copyTree = getBasicTreeByParentUid(rel.uid); @@ -1183,7 +1235,7 @@ const DiscourseRelationConfigPanel = ({ - {sortedRelations.map((rel) => ( + {localRelations.map((rel) => ( handleEdit(rel)}> {nodes[rel.source || ""]?.label} @@ -1243,6 +1295,98 @@ const DiscourseRelationConfigPanel = ({ ))} + {importedRelations.length > 0 && ( + <> +

Imported relations

+

+ Imported relations are read-only. Accept a relation to enable it for + local use. +

+ + + + Source + Relation + Destination + From + Status + Actions + + + + {importedRelations.map((rel) => ( + + + {nodes[rel.source || ""]?.label} + + {rel.text} + + {nodes[rel.destination || ""]?.label} + + + {formatImportedSource( + rel.importMeta.importedFrom.sourceNodeRid, + )} + + + {rel.importMeta.status === "provisional" ? ( + + Provisional + + ) : ( + + Accepted + + )} + + + {rel.importMeta.status === "provisional" && ( + + + + + + ))} + + + + )} ); }; diff --git a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts new file mode 100644 index 0000000000..e45710ca9a --- /dev/null +++ b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts @@ -0,0 +1,132 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; +import { IMPORTED_FROM_PROP_KEY } from "~/utils/importedSourceIdentity"; +import { + acceptImportedRelationSchema, + excludeProvisionalRelationSchemas, + isProvisionalRelationSchema, + readRelationSchemaImportMeta, + RELATION_SCHEMA_STATUS_PROP_KEY, +} from "~/utils/relationSchemaAcceptance"; +import type { json } from "~/utils/getBlockProps"; + +const SOURCE_NODE_RID = "orn:obsidian.schema:vault-a/relation-type-1"; +const SOURCE_MODIFIED_AT = "2026-08-01T12:00:00.000Z"; +const SCHEMA_UID = "relation-schema-uid"; + +const importedFromProps = { + [IMPORTED_FROM_PROP_KEY]: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, +}; + +const propsByUid = new Map>(); + +const setRoamAlphaApi = (): void => { + (globalThis as { window: unknown }).window = { + roamAlphaAPI: { + data: { + block: { + update: vi.fn( + ({ + block, + }: { + block: { props: Record; uid: string }; + }) => { + propsByUid.set(block.uid, block.props); + return Promise.resolve(); + }, + ), + }, + }, + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": propsByUid.get(uid) ?? {}, + }), + }, + }; +}; + +beforeEach(() => { + propsByUid.clear(); + setRoamAlphaApi(); +}); + +describe("relation schema import meta", () => { + it("returns undefined for local schemas without imported provenance", () => { + expect(readRelationSchemaImportMeta(SCHEMA_UID)).toBeUndefined(); + expect(isProvisionalRelationSchema(SCHEMA_UID)).toBe(false); + }); + + it("treats imported schemas without a status as provisional", () => { + propsByUid.set(SCHEMA_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: importedFromProps, + }); + + expect(readRelationSchemaImportMeta(SCHEMA_UID)).toEqual({ + importedFrom: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, + status: "provisional", + }); + expect(isProvisionalRelationSchema(SCHEMA_UID)).toBe(true); + }); + + it("treats imported schemas with an accepted status as accepted", () => { + propsByUid.set(SCHEMA_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: { + ...importedFromProps, + [RELATION_SCHEMA_STATUS_PROP_KEY]: "accepted", + }, + }); + + expect(readRelationSchemaImportMeta(SCHEMA_UID)?.status).toBe("accepted"); + expect(isProvisionalRelationSchema(SCHEMA_UID)).toBe(false); + }); +}); + +describe("acceptImportedRelationSchema", () => { + it("marks the schema accepted while preserving imported provenance", async () => { + propsByUid.set(SCHEMA_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: importedFromProps, + "other-extension": { enabled: true }, + }); + + await acceptImportedRelationSchema(SCHEMA_UID); + + expect(propsByUid.get(SCHEMA_UID)).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: { + ...importedFromProps, + [RELATION_SCHEMA_STATUS_PROP_KEY]: "accepted", + }, + "other-extension": { enabled: true }, + }); + expect(readRelationSchemaImportMeta(SCHEMA_UID)?.status).toBe("accepted"); + }); +}); + +describe("excludeProvisionalRelationSchemas", () => { + it("filters provisional schemas but keeps local and accepted ones", () => { + propsByUid.set("provisional-uid", { + [DISCOURSE_GRAPH_PROP_NAME]: importedFromProps, + }); + propsByUid.set("accepted-uid", { + [DISCOURSE_GRAPH_PROP_NAME]: { + ...importedFromProps, + [RELATION_SCHEMA_STATUS_PROP_KEY]: "accepted", + }, + }); + + const relations = [ + { id: "local-uid" }, + { id: "provisional-uid" }, + { id: "accepted-uid" }, + ]; + + expect(excludeProvisionalRelationSchemas(relations)).toEqual([ + { id: "local-uid" }, + { id: "accepted-uid" }, + ]); + }); +}); diff --git a/apps/roam/src/utils/publishNodesToGroups.ts b/apps/roam/src/utils/publishNodesToGroups.ts index f7936572fb..70794d576c 100644 --- a/apps/roam/src/utils/publishNodesToGroups.ts +++ b/apps/roam/src/utils/publishNodesToGroups.ts @@ -32,6 +32,7 @@ import { SOURCE_SLOT } from "./sourceSlot"; import renderToast from "roamjs-components/components/Toast"; import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; import { publishNodeAssets, type NodeAssetResult } from "./publishNodeAssets"; +import { excludeProvisionalRelationSchemas } from "./relationSchemaAcceptance"; export type NodeUidWithType = { uid: string; @@ -128,7 +129,11 @@ export const gatherCorrespondingRelations = async ({ relationTripleSchemas: CrossAppRelationTripleSchema[]; relevantRelationIdsPerGroupId: Record; }> => { - const allRelationsSchemas = getDiscourseRelations(); + // Excluding provisional schemas here also drops their relation instances: + // relationSchemaIds below only keeps instances whose schema is in this map. + const allRelationsSchemas = excludeProvisionalRelationSchemas( + getDiscourseRelations(), + ); const allRelationSchemasById = Object.fromEntries( allRelationsSchemas.map((s) => [s.id, s]), ); diff --git a/apps/roam/src/utils/relationSchemaAcceptance.ts b/apps/roam/src/utils/relationSchemaAcceptance.ts new file mode 100644 index 0000000000..7c71445452 --- /dev/null +++ b/apps/roam/src/utils/relationSchemaAcceptance.ts @@ -0,0 +1,59 @@ +import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; +import getBlockProps from "./getBlockProps"; +import { setBlockPropsAsync } from "./setBlockProps"; +import { + isJsonObject, + readImportedSourceIdentity, + type ImportedSourceIdentity, +} from "./importedSourceIdentity"; + +export type RelationSchemaImportStatus = "provisional" | "accepted"; + +export const RELATION_SCHEMA_STATUS_PROP_KEY = "status"; + +export type RelationSchemaImportMeta = { + importedFrom: ImportedSourceIdentity; + status: RelationSchemaImportStatus; +}; + +// Origin (importedFrom) and acceptance (status) are stored as separate props so +// accepting never erases provenance; a schema with origin but no accepted +// status is provisional, which also covers schemas imported before acceptance +// existed. +export const readRelationSchemaImportMeta = ( + relationSchemaUid: string, +): RelationSchemaImportMeta | undefined => { + const importedFrom = readImportedSourceIdentity(relationSchemaUid); + if (importedFrom === undefined) return undefined; + const discourseGraphProps = + getBlockProps(relationSchemaUid)[DISCOURSE_GRAPH_PROP_NAME]; + const status = + isJsonObject(discourseGraphProps) && + discourseGraphProps[RELATION_SCHEMA_STATUS_PROP_KEY] === "accepted" + ? "accepted" + : "provisional"; + return { importedFrom, status }; +}; + +export const isProvisionalRelationSchema = ( + relationSchemaUid: string, +): boolean => + readRelationSchemaImportMeta(relationSchemaUid)?.status === "provisional"; + +export const excludeProvisionalRelationSchemas = ( + relations: T[], +): T[] => + relations.filter((relation) => !isProvisionalRelationSchema(relation.id)); + +export const acceptImportedRelationSchema = async ( + relationSchemaUid: string, +): Promise => { + const existing = getBlockProps(relationSchemaUid)[DISCOURSE_GRAPH_PROP_NAME]; + const discourseGraphProps = isJsonObject(existing) ? existing : {}; + await setBlockPropsAsync(relationSchemaUid, { + [DISCOURSE_GRAPH_PROP_NAME]: { + ...discourseGraphProps, + [RELATION_SCHEMA_STATUS_PROP_KEY]: "accepted", + }, + }); +}; From 13352df22523c8851b045ea49e526fc5e01138ec Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 3 Sep 2026 00:49:13 +0530 Subject: [PATCH 02/17] ENG-2131 Address pre-PR review findings --- .../DiscourseRelationTool.tsx | 7 +- .../DiscourseRelationUtil.tsx | 55 +++-- apps/roam/src/components/canvas/Tldraw.tsx | 9 +- .../roam/src/components/canvas/canvasUtils.ts | 13 +- .../canvas/overlays/relationCreation.ts | 8 +- .../src/components/canvas/uiOverrides.tsx | 6 +- .../settings/DiscourseRelationConfigPanel.tsx | 228 ++++++++++-------- apps/roam/src/utils/canonicalRoamUrl.ts | 2 +- apps/roam/src/utils/importedSourceIdentity.ts | 2 +- .../src/utils/relationSchemaAcceptance.ts | 19 +- 10 files changed, 199 insertions(+), 150 deletions(-) diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx index 44d11ea5df..997c8c4d46 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx @@ -8,7 +8,10 @@ import { DiscourseRelationShape, getRelationColor, } from "./DiscourseRelationUtil"; -import { discourseContext } from "~/components/canvas/Tldraw"; +import { + discourseContext, + isAcceptedRelationSchema, +} from "~/components/canvas/Tldraw"; import { dispatchToastEvent } from "~/components/canvas/ToastListener"; import { isRelationComplete } from "~/utils/isRelationComplete"; import { @@ -352,7 +355,7 @@ export const createAllRelationShapeTools = ( const selectedRelations = ( discourseContext.relations[name] || [] - ).filter((r) => !discourseContext.provisionalRelationIds.has(r.id)); + ).filter(isAcceptedRelationSchema); const hasIncompleteSelectedRelation = selectedRelations.some( (relation) => !isRelationComplete(relation), ); diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx index fa13e49969..2e94da16b4 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx @@ -66,7 +66,11 @@ import { import { createReifiedRelation } from "~/utils/createReifiedBlock"; import { getStoredRelationsEnabled } from "~/utils/storedRelations"; import type { DiscourseRelation } from "~/utils/getDiscourseRelations"; -import { discourseContext, isPageUid } from "~/components/canvas/Tldraw"; +import { + discourseContext, + isAcceptedRelationSchema, + isPageUid, +} from "~/components/canvas/Tldraw"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; /** @@ -663,11 +667,11 @@ export const createAllRelationShapeUtils = ( isDirect, isReverse, matchingRelation: foundRelation, - } = this.checkConnectionTypeAcrossLabel( - relation.label, + } = this.checkConnectionTypeAcrossLabel({ + label: relation.label, sourceNodeType, targetNodeType, - ); + }); const matchingRelation = foundRelation ?? relation; if (!isDirect && !isReverse) { @@ -1043,11 +1047,11 @@ export const createAllRelationShapeUtils = ( const endNodeType = getDiscourseNodeTypeId({ shape: endNode }); const { isReverse, matchingRelation } = - this.checkConnectionTypeAcrossLabel( - relation.label, - startNodeType, - endNodeType, - ); + this.checkConnectionTypeAcrossLabel({ + label: relation.label, + sourceNodeType: startNodeType, + targetNodeType: endNodeType, + }); const effectiveRelation = matchingRelation ?? relation; @@ -1765,18 +1769,24 @@ export class BaseDiscourseRelationUtil extends ShapeUtil return checkConnectionType(relation, sourceNodeType, targetNodeType); } - checkConnectionTypeAcrossLabel( - label: string, - sourceNodeType: string, - targetNodeType: string, - ): { + checkConnectionTypeAcrossLabel({ + label, + sourceNodeType, + targetNodeType, + includeProvisional, + }: { + label: string; + sourceNodeType: string; + targetNodeType: string; + includeProvisional?: boolean; + }): { isDirect: boolean; isReverse: boolean; matchingRelation: DiscourseRelation | null; } { - const relationsWithLabel = discourseContext.relations[label]?.filter( - (r) => !discourseContext.provisionalRelationIds.has(r.id), - ); + const relationsWithLabel = includeProvisional + ? discourseContext.relations[label] + : discourseContext.relations[label]?.filter(isAcceptedRelationSchema); if (!relationsWithLabel) { return { isDirect: false, isReverse: false, matchingRelation: null }; } @@ -1797,7 +1807,7 @@ export class BaseDiscourseRelationUtil extends ShapeUtil getValidTargetTypes(label: string, sourceNodeType: string): string[] { const relationsWithLabel = discourseContext.relations[label]?.filter( - (r) => !discourseContext.provisionalRelationIds.has(r.id), + isAcceptedRelationSchema, ); if (!relationsWithLabel) return []; @@ -1818,11 +1828,14 @@ export class BaseDiscourseRelationUtil extends ShapeUtil const relation = relations.find((r) => r.id === relationId); if (!relation) return false; - const { isDirect, isReverse } = this.checkConnectionTypeAcrossLabel( - relation.label, + // Validates handle drags of arrows that already exist, so provisional + // relations stay re-bindable; only creation paths filter them out. + const { isDirect, isReverse } = this.checkConnectionTypeAcrossLabel({ + label: relation.label, sourceNodeType, targetNodeType, - ); + includeProvisional: true, + }); return isDirect || isReverse; } diff --git a/apps/roam/src/components/canvas/Tldraw.tsx b/apps/roam/src/components/canvas/Tldraw.tsx index 954887087b..7849df75a9 100644 --- a/apps/roam/src/components/canvas/Tldraw.tsx +++ b/apps/roam/src/components/canvas/Tldraw.tsx @@ -149,6 +149,9 @@ export const discourseContext: DiscourseContextType = { lastActions: [], }; +export const isAcceptedRelationSchema = (relation: { id: string }): boolean => + !discourseContext.provisionalRelationIds.has(relation.id); + let activeCanvasPageUid: string | null = null; let activeCanvasEditor: Editor | null = null; @@ -789,11 +792,7 @@ const TldrawCanvasShared = ({ }, [allRelationsById]); const allRelationNames = useMemo(() => { return Object.entries(discourseContext.relations) - .filter(([, relations]) => - relations.some( - (r) => !discourseContext.provisionalRelationIds.has(r.id), - ), - ) + .filter(([, relations]) => relations.some(isAcceptedRelationSchema)) .map(([name]) => name); }, []); const allNodes = useMemo(() => { diff --git a/apps/roam/src/components/canvas/canvasUtils.ts b/apps/roam/src/components/canvas/canvasUtils.ts index 6b065e7fc2..2e8058deba 100644 --- a/apps/roam/src/components/canvas/canvasUtils.ts +++ b/apps/roam/src/components/canvas/canvasUtils.ts @@ -3,7 +3,10 @@ import { DiscourseNodeUtil, DiscourseNodeShape, } from "~/components/canvas/DiscourseNodeUtil"; -import { discourseContext } from "~/components/canvas/Tldraw"; +import { + discourseContext, + isAcceptedRelationSchema, +} from "~/components/canvas/Tldraw"; export const isDiscourseNodeShape = ( editor: Editor, @@ -16,12 +19,10 @@ export const isDiscourseNodeShape = ( } }; -// Creation-facing list: provisional imported relation schemas are excluded so -// they cannot be used to create new relations on the canvas. -export const getAllRelations = () => +export const getCreatableRelations = () => Object.values(discourseContext.relations) .flat() - .filter((r) => !discourseContext.provisionalRelationIds.has(r.id)); + .filter(isAcceptedRelationSchema); export const checkConnectionType = ( relation: { source: string; destination: string }, @@ -40,7 +41,7 @@ export const hasValidRelationTypes = ( sourceNodeType: string, targetNodeType: string, ): boolean => - getAllRelations().some( + getCreatableRelations().some( (r) => (r.source === sourceNodeType && r.destination === targetNodeType) || (r.source === targetNodeType && r.destination === sourceNodeType), diff --git a/apps/roam/src/components/canvas/overlays/relationCreation.ts b/apps/roam/src/components/canvas/overlays/relationCreation.ts index ede2fc6aff..0daeefd060 100644 --- a/apps/roam/src/components/canvas/overlays/relationCreation.ts +++ b/apps/roam/src/components/canvas/overlays/relationCreation.ts @@ -13,7 +13,7 @@ import { createOrUpdateArrowBinding } from "~/components/canvas/DiscourseRelatio import { getDiscourseNodeTypeId } from "~/components/canvas/DiscourseNodeUtil"; import { checkConnectionType, - getAllRelations, + getCreatableRelations, isDiscourseNodeShape, } from "~/components/canvas/canvasUtils"; import type { DiscourseRelation } from "~/utils/getDiscourseRelations"; @@ -93,7 +93,7 @@ export const getValidRelationTypesBetween = ( const validTypes: RelationTypeOption[] = []; const seenLabels = new Set(); - for (const relation of getAllRelations()) { + for (const relation of getCreatableRelations()) { if (!isRelationComplete(relation)) continue; const { isDirect, isReverse } = checkConnectionType( relation, @@ -129,7 +129,9 @@ export const createDefaultRelationBetweenNodes = async ({ sourceId: TLShapeId; targetId: TLShapeId; }): Promise => { - const selectedRelation = getAllRelations().find((r) => r.id === relationId); + const selectedRelation = getCreatableRelations().find( + (r) => r.id === relationId, + ); if (!selectedRelation) return null; const sourceNode = editor.getShape(sourceId); diff --git a/apps/roam/src/components/canvas/uiOverrides.tsx b/apps/roam/src/components/canvas/uiOverrides.tsx index d3d8cffd2d..cd03d2c6e6 100644 --- a/apps/roam/src/components/canvas/uiOverrides.tsx +++ b/apps/roam/src/components/canvas/uiOverrides.tsx @@ -66,7 +66,7 @@ import { getValidRelationTypesBetween, persistRelationArrow, } from "./overlays/relationCreation"; -import { getAllRelations } from "./canvasUtils"; +import { getCreatableRelations } from "./canvasUtils"; import { createOrUpdateArrowBinding } from "./DiscourseRelationShape/helpers"; import DiscourseGraphPanel from "./DiscourseToolPanel"; import type { CanvasNodeShortcuts } from "~/components/settings/utils/zodSchema"; @@ -300,7 +300,9 @@ const convertArrowToRelation = async ({ const boundNodes = getArrowBoundNodeInfo(editor, arrow); if (!boundNodes) return null; - const selectedRelation = getAllRelations().find((r) => r.id === relationId); + const selectedRelation = getCreatableRelations().find( + (r) => r.id === relationId, + ); if (!selectedRelation) return null; const sourceNode = editor.getShape(boundNodes.startId); diff --git a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx index e457f80001..457a718774 100644 --- a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx @@ -22,6 +22,7 @@ import React, { useCallback, useEffect, useMemo, + useReducer, useRef, useState, } from "react"; @@ -73,6 +74,9 @@ import { } from "~/utils/relationSchemaAcceptance"; import { getReifiedRelations } from "~/utils/createReifiedBlock"; import { ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; +import { ROAM_URL_PREFIX } from "~/utils/canonicalRoamUrl"; +import { discourseContext } from "~/components/canvas/Tldraw"; +import internalError from "~/utils/internalError"; const DEFAULT_SELECTED_RELATION = { display: "none", @@ -986,12 +990,10 @@ type Relation = { }; type ImportedRelation = Relation & { importMeta: RelationSchemaImportMeta }; -const ROAM_SPACE_URI_PREFIX = "https://roamresearch.com/#/app/"; - const formatImportedSource = (sourceNodeRid: string): string => { const { spaceUri } = ridToSpaceUriAndLocalId(sourceNodeRid); - return spaceUri.startsWith(ROAM_SPACE_URI_PREFIX) - ? spaceUri.slice(ROAM_SPACE_URI_PREFIX.length) + return spaceUri.startsWith(ROAM_URL_PREFIX) + ? spaceUri.slice(ROAM_URL_PREFIX.length) : spaceUri; }; const DiscourseRelationConfigPanel = ({ @@ -1055,16 +1057,17 @@ const DiscourseRelationConfigPanel = ({ : visibleRelations, [nodes, sort, visibleRelations], ); - const { localRelations, importedRelations } = useMemo(() => { - const local: Relation[] = []; - const imported: ImportedRelation[] = []; - for (const rel of sortedRelations) { - const importMeta = readRelationSchemaImportMeta(rel.uid); - if (importMeta) imported.push({ ...rel, importMeta }); - else local.push(rel); - } - return { localRelations: local, importedRelations: imported }; - }, [sortedRelations]); + // Acceptance lives in block props, not in the relations state, so the split + // below is recomputed on every render and accepting bumps this reducer to + // trigger one. + const [, refreshImportMeta] = useReducer((version: number) => version + 1, 0); + const localRelations: Relation[] = []; + const importedRelations: ImportedRelation[] = []; + for (const rel of sortedRelations) { + const importMeta = readRelationSchemaImportMeta(rel.uid); + if (importMeta) importedRelations.push({ ...rel, importMeta }); + else localRelations.push(rel); + } const editingRelationInfo = useMemo( () => editingRelation ? getFullTreeByParentUid(editingRelation) : undefined, @@ -1108,28 +1111,50 @@ const DiscourseRelationConfigPanel = ({ }); }; const handleAcceptImported = (rel: Relation) => { - void acceptImportedRelationSchema(rel.uid).then(() => { - setRelations(refreshRelations()); - }); + void acceptImportedRelationSchema(rel.uid) + .then(() => { + // Make the acceptance visible to canvases that are already mounted. + discourseContext.provisionalRelationIds.delete(rel.uid); + posthog.capture("Discourse Relation: Accepted", { + relationUid: rel.uid, + }); + refreshImportMeta(); + }) + .catch((error: unknown) => { + internalError({ + error, + type: "Discourse Relation: Accept failed", + userMessage: "Could not accept the imported relation.", + }); + }); }; const handleDeleteImported = (rel: Relation) => { - void getReifiedRelations().then((reifiedRelations) => { - const inUseCount = reifiedRelations.filter( - (r) => r.hasSchema === rel.uid, - ).length; - if (inUseCount > 0) { - renderToast({ - id: "discourse-relation-delete-blocked", - intent: Intent.WARNING, - content: `Cannot delete this imported relation: ${inUseCount} relation ${ - inUseCount === 1 ? "instance uses" : "instances use" - } it in this graph.`, + void getReifiedRelations() + .then((reifiedRelations) => { + const inUseCount = reifiedRelations.filter( + (r) => r.hasSchema === rel.uid, + ).length; + if (inUseCount > 0) { + renderToast({ + id: "discourse-relation-delete-blocked", + intent: Intent.WARNING, + content: `Cannot delete this imported relation: ${inUseCount} relation ${ + inUseCount === 1 ? "instance uses" : "instances use" + } it in this graph.`, + }); + setDeleteConfirmation(null); + return; + } + handleDelete(rel); + }) + .catch((error: unknown) => { + internalError({ + error, + type: "Discourse Relation: Delete imported check failed", + userMessage: + "Could not check whether this imported relation is in use.", }); - setDeleteConfirmation(null); - return; - } - handleDelete(rel); - }); + }); }; const handleDuplicate = (rel: Relation) => { const text = rel.text; @@ -1305,84 +1330,87 @@ const DiscourseRelationConfigPanel = ({ - Source - Relation - Destination + {renderSortableHeader("Source", "source")} + {renderSortableHeader("Relation", "relation")} + {renderSortableHeader("Destination", "destination")} From Status Actions - {importedRelations.map((rel) => ( - - - {nodes[rel.source || ""]?.label} - - {rel.text} - - {nodes[rel.destination || ""]?.label} - - - {formatImportedSource( - rel.importMeta.importedFrom.sourceNodeRid, - )} - - - {rel.importMeta.status === "provisional" ? ( - - Provisional - - ) : ( - - Accepted - - )} - - - {rel.importMeta.status === "provisional" && ( - + {importedRelations.map((rel) => { + const isProvisional = rel.importMeta.status === "provisional"; + return ( + + + {nodes[rel.source || ""]?.label} + + {rel.text} + + {nodes[rel.destination || ""]?.label} + + + {formatImportedSource( + rel.importMeta.importedFrom.sourceNodeRid, + )} + + + {isProvisional ? ( + + Provisional + + ) : ( + + Accepted + + )} + + + {isProvisional && ( + + - - - - ))} + intent={Intent.DANGER} + onClick={() => handleDeleteImported(rel)} + className={`mx-1 ${ + deleteConfirmation !== rel.uid ? "invisible" : "" + }`} + > + Confirm + + + + + ); + })} diff --git a/apps/roam/src/utils/canonicalRoamUrl.ts b/apps/roam/src/utils/canonicalRoamUrl.ts index bb73c860c7..07f4f6e63e 100644 --- a/apps/roam/src/utils/canonicalRoamUrl.ts +++ b/apps/roam/src/utils/canonicalRoamUrl.ts @@ -1,4 +1,4 @@ -const ROAM_URL_PREFIX = "https://roamresearch.com/#/app/"; +export const ROAM_URL_PREFIX = "https://roamresearch.com/#/app/"; const canonicalRoamUrl = (graphName = window.roamAlphaAPI.graph.name) => ROAM_URL_PREFIX + graphName; export default canonicalRoamUrl; diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index abff26d962..77774efb12 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -27,7 +27,7 @@ export const parseSourceIdentity = ( return { sourceModifiedAt, sourceNodeRid }; }; -const parseImportedSourceIdentity = ( +export const parseImportedSourceIdentity = ( props: Record, ): ImportedSourceIdentity | undefined => { const discourseGraphProps = props[DISCOURSE_GRAPH_PROP_NAME]; diff --git a/apps/roam/src/utils/relationSchemaAcceptance.ts b/apps/roam/src/utils/relationSchemaAcceptance.ts index 7c71445452..4bcb0f5efd 100644 --- a/apps/roam/src/utils/relationSchemaAcceptance.ts +++ b/apps/roam/src/utils/relationSchemaAcceptance.ts @@ -3,17 +3,18 @@ import getBlockProps from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; import { isJsonObject, - readImportedSourceIdentity, + parseImportedSourceIdentity, type ImportedSourceIdentity, } from "./importedSourceIdentity"; -export type RelationSchemaImportStatus = "provisional" | "accepted"; +type ImportStatus = "provisional" | "accepted"; export const RELATION_SCHEMA_STATUS_PROP_KEY = "status"; +const ACCEPTED_STATUS: ImportStatus = "accepted"; export type RelationSchemaImportMeta = { importedFrom: ImportedSourceIdentity; - status: RelationSchemaImportStatus; + status: ImportStatus; }; // Origin (importedFrom) and acceptance (status) are stored as separate props so @@ -23,14 +24,14 @@ export type RelationSchemaImportMeta = { export const readRelationSchemaImportMeta = ( relationSchemaUid: string, ): RelationSchemaImportMeta | undefined => { - const importedFrom = readImportedSourceIdentity(relationSchemaUid); + const props = getBlockProps(relationSchemaUid); + const importedFrom = parseImportedSourceIdentity(props); if (importedFrom === undefined) return undefined; - const discourseGraphProps = - getBlockProps(relationSchemaUid)[DISCOURSE_GRAPH_PROP_NAME]; + const discourseGraphProps = props[DISCOURSE_GRAPH_PROP_NAME]; const status = isJsonObject(discourseGraphProps) && - discourseGraphProps[RELATION_SCHEMA_STATUS_PROP_KEY] === "accepted" - ? "accepted" + discourseGraphProps[RELATION_SCHEMA_STATUS_PROP_KEY] === ACCEPTED_STATUS + ? ACCEPTED_STATUS : "provisional"; return { importedFrom, status }; }; @@ -53,7 +54,7 @@ export const acceptImportedRelationSchema = async ( await setBlockPropsAsync(relationSchemaUid, { [DISCOURSE_GRAPH_PROP_NAME]: { ...discourseGraphProps, - [RELATION_SCHEMA_STATUS_PROP_KEY]: "accepted", + [RELATION_SCHEMA_STATUS_PROP_KEY]: ACCEPTED_STATUS, }, }); }; From a09c65b7bf5a83151d7c9fc7784d70dc660e65bf Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 00:41:31 +0530 Subject: [PATCH 03/17] Fix imported relation matching and refresh the grammar cache --- .../__tests__/importSharedRelations.test.ts | 20 +++++++++++++++++++ apps/roam/src/utils/importSharedRelations.ts | 3 +++ 2 files changed, 23 insertions(+) diff --git a/apps/roam/src/utils/__tests__/importSharedRelations.test.ts b/apps/roam/src/utils/__tests__/importSharedRelations.test.ts index 80e4d95c8b..50b8725e4f 100644 --- a/apps/roam/src/utils/__tests__/importSharedRelations.test.ts +++ b/apps/roam/src/utils/__tests__/importSharedRelations.test.ts @@ -2,12 +2,15 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import type { DiscourseRelation } from "~/utils/getDiscourseRelations"; import { importSharedRelations } from "~/utils/importSharedRelations"; +import refreshConfigTree from "~/utils/refreshConfigTree"; +import { writeImportedSourceIdentity } from "~/utils/importedSourceIdentity"; import getDiscourseRelations from "~/utils/getDiscourseRelations"; import { createRelationSchema } from "~/utils/createRelationSchema"; vi.hoisted(() => { vi.stubGlobal("window", { roamAlphaAPI: { graph: { name: "local" } } }); }); +vi.mock("~/utils/refreshConfigTree", () => ({ default: vi.fn() })); vi.mock("~/utils/getDiscourseRelations", () => ({ default: vi.fn() })); vi.mock("~/utils/getDiscourseNodes", () => ({ default: () => [{ type: "local-claim", text: "Claim" }], @@ -70,6 +73,23 @@ const client = {} as DGSupabaseClient; beforeEach(() => vi.clearAllMocks()); describe("importSharedRelations schema matching", () => { + it("refreshes the grammar after storing a new schema and its provenance", async () => { + vi.mocked(getDiscourseRelations).mockReturnValue([]); + vi.mocked(createRelationSchema).mockResolvedValue("imported-supports"); + await importSharedRelations(client, 7); + expect(writeImportedSourceIdentity).toHaveBeenCalledWith( + expect.objectContaining({ + pageUid: "imported-supports", + sourceNodeRid: "orn:obsidian.schema:remote/supports", + }), + ); + expect(refreshConfigTree).toHaveBeenCalledOnce(); + expect( + vi.mocked(refreshConfigTree).mock.invocationCallOrder[0], + ).toBeGreaterThan( + vi.mocked(writeImportedSourceIdentity).mock.invocationCallOrder[0], + ); + }); it("reuses one schema when its query patterns produce multiple matches", async () => { vi.mocked(getDiscourseRelations).mockReturnValue([ { diff --git a/apps/roam/src/utils/importSharedRelations.ts b/apps/roam/src/utils/importSharedRelations.ts index 70732bfc43..5b0262f0aa 100644 --- a/apps/roam/src/utils/importSharedRelations.ts +++ b/apps/roam/src/utils/importSharedRelations.ts @@ -27,6 +27,7 @@ import { import { discoverSharedRelations } from "./discoverSharedRelations"; import { DGSupabaseClient } from "@repo/database/lib/client"; import { deleteBlock } from "roamjs-components/writes"; +import refreshConfigTree from "./refreshConfigTree"; const matchImportedNodeSchemas = async ( nodeSchemas: CrossAppNodeSchema[], @@ -254,4 +255,6 @@ export const importSharedRelations = async ( ); ridToLocalId = { ...ridToLocalId, ...relationSchemaMap }; await importRelations(ridToLocalId, relations); + // Legacy settings read the cached grammar, including newly imported schemas. + refreshConfigTree(); }; From e44c69812c16a18fec90a63abe96723cf33fc974 Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 00:43:44 +0530 Subject: [PATCH 04/17] Read current relation choices when opening the creation dialog --- apps/roam/src/components/CreateRelationDialog.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/roam/src/components/CreateRelationDialog.tsx b/apps/roam/src/components/CreateRelationDialog.tsx index b283161f8f..1d2403ca58 100644 --- a/apps/roam/src/components/CreateRelationDialog.tsx +++ b/apps/roam/src/components/CreateRelationDialog.tsx @@ -406,7 +406,8 @@ export const CreateRelationButton = ( text="Add relation" disabled={extProps === null} onClick={() => { - renderCreateRelationDialog(extProps); + // A schema may have been accepted since this button last rendered. + renderCreateRelationDialog(relationProps); }} /> ); From f497684b4604a6eacad45a4f48d47d3725c69858 Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 01:22:16 +0530 Subject: [PATCH 05/17] ENG-2131 Align imported relation actions with row content --- .../settings/DiscourseRelationConfigPanel.tsx | 77 ++++++++++--------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx index 457a718774..92552d93b0 100644 --- a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx @@ -1366,47 +1366,50 @@ const DiscourseRelationConfigPanel = ({ )} - - {isProvisional && ( - + +
+ {isProvisional && ( + + - + {deleteConfirmation === rel.uid && ( + <> + + + + )} +
); From b07daacab34941ca09fc92a74f86ef73a5d7deb9 Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 01:34:03 +0530 Subject: [PATCH 06/17] ENG-2131 Keep delete confirmation within the actions column --- .../settings/DiscourseRelationConfigPanel.tsx | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx index 92552d93b0..fac714bd5f 100644 --- a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx @@ -1368,46 +1368,46 @@ const DiscourseRelationConfigPanel = ({
- {isProvisional && ( - - + ) : ( + <> + {isProvisional && ( + +
From f2b15b4497a23620d5aa98109cd21467addd4b55 Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 02:21:20 +0530 Subject: [PATCH 07/17] ENG-2131 Refresh mounted relation creation after schema changes --- .../src/components/CreateRelationDialog.tsx | 2 + apps/roam/src/components/SuggestionsBody.tsx | 6 +- apps/roam/src/components/canvas/Tldraw.tsx | 42 ++++- .../settings/DiscourseRelationConfigPanel.tsx | 31 ++-- .../__tests__/deleteRelationSchema.test.ts | 79 ++++++++ .../relationSchemaCreationRefresh.test.ts | 170 ++++++++++++++++++ apps/roam/src/utils/deleteRelationSchema.ts | 18 ++ .../src/utils/relationSchemaAcceptance.ts | 11 +- apps/roam/src/utils/relationSchemaChanges.ts | 39 ++++ 9 files changed, 371 insertions(+), 27 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/deleteRelationSchema.test.ts create mode 100644 apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts create mode 100644 apps/roam/src/utils/deleteRelationSchema.ts create mode 100644 apps/roam/src/utils/relationSchemaChanges.ts diff --git a/apps/roam/src/components/CreateRelationDialog.tsx b/apps/roam/src/components/CreateRelationDialog.tsx index 1d2403ca58..fa4b335f0d 100644 --- a/apps/roam/src/components/CreateRelationDialog.tsx +++ b/apps/roam/src/components/CreateRelationDialog.tsx @@ -1,3 +1,4 @@ +import { useRelationSchemaRevision } from "~/utils/relationSchemaChanges"; import React, { useState, useMemo } from "react"; import { Dialog, Classes, Label, Button, Callout } from "@blueprintjs/core"; import renderOverlay from "roamjs-components/util/renderOverlay"; @@ -388,6 +389,7 @@ export const renderCreateRelationDialog = ( export const CreateRelationButton = ( props: CreateRelationDialogProps & { fill?: boolean }, ): React.JSX.Element | null => { + useRelationSchemaRevision(); const { fill = false, ...relationProps } = props; const storedRelationsEnabled = getStoredRelationsEnabled(); if (!storedRelationsEnabled) return null; diff --git a/apps/roam/src/components/SuggestionsBody.tsx b/apps/roam/src/components/SuggestionsBody.tsx index bcfe68ac79..ff055b7034 100644 --- a/apps/roam/src/components/SuggestionsBody.tsx +++ b/apps/roam/src/components/SuggestionsBody.tsx @@ -1,3 +1,4 @@ +import { useRelationSchemaRevision } from "~/utils/relationSchemaChanges"; import React, { useMemo, useState, useEffect, useCallback } from "react"; import { Button, @@ -233,9 +234,12 @@ const SuggestionsBody = ({ () => findDiscourseNode({ uid: tagUid }), [tagUid], ); + const relationSchemaRevision = useRelationSchemaRevision(); const allRelations = useMemo( () => excludeProvisionalRelationSchemas(getDiscourseRelations()), - [], + // Acceptance and deletion invalidate the relation data stored outside React. + // eslint-disable-next-line react-hooks/exhaustive-deps + [relationSchemaRevision], ); const allNodes = useMemo(() => getDiscourseNodes(), []); diff --git a/apps/roam/src/components/canvas/Tldraw.tsx b/apps/roam/src/components/canvas/Tldraw.tsx index 7849df75a9..5c5bb62e55 100644 --- a/apps/roam/src/components/canvas/Tldraw.tsx +++ b/apps/roam/src/components/canvas/Tldraw.tsx @@ -1,3 +1,7 @@ +import { + isRelationSchemaDeleted, + useRelationSchemaRevision, +} from "~/utils/relationSchemaChanges"; import React, { useState, useRef, @@ -150,7 +154,8 @@ export const discourseContext: DiscourseContextType = { }; export const isAcceptedRelationSchema = (relation: { id: string }): boolean => - !discourseContext.provisionalRelationIds.has(relation.id); + !discourseContext.provisionalRelationIds.has(relation.id) && + !isRelationSchemaDeleted(relation.id); let activeCanvasPageUid: string | null = null; let activeCanvasEditor: Editor | null = null; @@ -774,11 +779,7 @@ const TldrawCanvasShared = ({ }, {} as Record, ); - discourseContext.provisionalRelationIds = new Set( - relations - .filter((r) => isProvisionalRelationSchema(r.id)) - .map((r) => r.id), - ); + return relations; }, []); const allRelationsById = useMemo(() => { @@ -790,11 +791,34 @@ const TldrawCanvasShared = ({ const allRelationIds = useMemo(() => { return Object.keys(allRelationsById); }, [allRelationsById]); + const relationSchemaRevision = useRelationSchemaRevision(); + const registeredRelationNames = useMemo( + () => [...new Set(allRelations.map((relation) => relation.label))], + [allRelations], + ); const allRelationNames = useMemo(() => { + discourseContext.provisionalRelationIds = new Set( + allRelations + .filter((r) => isProvisionalRelationSchema(r.id)) + .map((r) => r.id), + ); return Object.entries(discourseContext.relations) .filter(([, relations]) => relations.some(isAcceptedRelationSchema)) .map(([name]) => name); - }, []); + // Acceptance and deletion invalidate the relation data stored outside React. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [allRelations, relationSchemaRevision]); + useEffect(() => { + const editor = appRef.current; + if (!editor) return; + const tool = editor.getCurrentToolId(); + if ( + registeredRelationNames.includes(tool) && + !allRelationNames.includes(tool) + ) { + editor.setCurrentTool("select"); + } + }, [allRelationNames, registeredRelationNames]); const allNodes = useMemo(() => { const allNodes = getDiscourseNodes(); discourseContext.nodes = Object.fromEntries( @@ -1040,7 +1064,9 @@ const TldrawCanvasShared = ({ static override isLockable = true; }; const discourseNodeTools = createNodeShapeTools(allNodes); - const discourseRelationTools = createAllRelationShapeTools(allRelationNames); + const discourseRelationTools = createAllRelationShapeTools( + registeredRelationNames, + ); const referencedNodeTools = createAllReferencedNodeTools( allAddReferencedNodeByAction, ); diff --git a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx index fac714bd5f..73f6bf5883 100644 --- a/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx +++ b/apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx @@ -75,7 +75,7 @@ import { import { getReifiedRelations } from "~/utils/createReifiedBlock"; import { ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; import { ROAM_URL_PREFIX } from "~/utils/canonicalRoamUrl"; -import { discourseContext } from "~/components/canvas/Tldraw"; +import { deleteRelationSchema } from "~/utils/deleteRelationSchema"; import internalError from "~/utils/internalError"; const DEFAULT_SELECTED_RELATION = { @@ -1100,21 +1100,13 @@ const DiscourseRelationConfigPanel = ({ setEditingRelation(rel.uid); }; - const handleDelete = (rel: Relation) => { - void deleteBlock(rel.uid).then(() => { - const { [rel.uid]: _, ...remaining } = getGlobalSettings().Relations; - setGlobalSetting([GLOBAL_KEYS.relations], remaining); - setTimeout(() => { - refreshConfigTree(); - setRelations(refreshRelations()); - }, 50); - }); + const handleDelete = async (rel: Relation): Promise => { + await deleteRelationSchema(rel.uid); + setRelations(refreshRelations()); }; const handleAcceptImported = (rel: Relation) => { void acceptImportedRelationSchema(rel.uid) .then(() => { - // Make the acceptance visible to canvases that are already mounted. - discourseContext.provisionalRelationIds.delete(rel.uid); posthog.capture("Discourse Relation: Accepted", { relationUid: rel.uid, }); @@ -1145,14 +1137,13 @@ const DiscourseRelationConfigPanel = ({ setDeleteConfirmation(null); return; } - handleDelete(rel); + return handleDelete(rel); }) .catch((error: unknown) => { internalError({ error, - type: "Discourse Relation: Delete imported check failed", - userMessage: - "Could not check whether this imported relation is in use.", + type: "Discourse Relation: Delete imported failed", + userMessage: "Could not delete the imported relation.", }); }); }; @@ -1299,7 +1290,13 @@ const DiscourseRelationConfigPanel = ({ intent={Intent.DANGER} onClick={(e) => { e.stopPropagation(); - handleDelete(rel); + void handleDelete(rel).catch((error: unknown) => { + internalError({ + error, + type: "Discourse Relation: Delete failed", + userMessage: "Could not delete the relation.", + }); + }); }} className={`mx-1 ${ deleteConfirmation !== rel.uid ? "opacity-0" : "" diff --git a/apps/roam/src/utils/__tests__/deleteRelationSchema.test.ts b/apps/roam/src/utils/__tests__/deleteRelationSchema.test.ts new file mode 100644 index 0000000000..9af73b4b27 --- /dev/null +++ b/apps/roam/src/utils/__tests__/deleteRelationSchema.test.ts @@ -0,0 +1,79 @@ +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { deleteRelationSchema } from "~/utils/deleteRelationSchema"; +import { + isRelationSchemaDeleted, + subscribeToRelationSchemaChanges, +} from "~/utils/relationSchemaChanges"; + +const mocks = vi.hoisted(() => ({ + deleteBlock: vi.fn(), + setSetting: vi.fn(), + refresh: vi.fn(), +})); +vi.mock("roamjs-components/writes/deleteBlock", () => ({ + default: mocks.deleteBlock, +})); +vi.mock("~/components/settings/utils/accessors", () => ({ + getGlobalSettings: () => ({ + Relations: { deleted: { label: "supports" }, kept: { label: "opposes" } }, + }), + setGlobalSetting: mocks.setSetting, +})); +vi.mock("~/utils/refreshConfigTree", () => ({ default: mocks.refresh })); +beforeEach(() => { + vi.resetAllMocks(); + vi.useFakeTimers(); +}); +afterEach(() => { + vi.useRealTimers(); +}); + +it("blocks creation immediately after deletion and waits for configuration refresh", async () => { + const listener = vi.fn(); + const unsubscribe = subscribeToRelationSchemaChanges(listener); + try { + const finished = vi.fn(); + const pending = deleteRelationSchema("deleted").then(finished); + await Promise.resolve(); + expect(isRelationSchemaDeleted("deleted")).toBe(true); + expect(listener).toHaveBeenCalledOnce(); + expect(mocks.setSetting).toHaveBeenCalledWith(["Relations"], { + kept: { label: "opposes" }, + }); + expect(finished).not.toHaveBeenCalled(); + await vi.runAllTimersAsync(); + await pending; + expect(mocks.refresh).toHaveBeenCalledOnce(); + expect(finished).toHaveBeenCalledOnce(); + } finally { + unsubscribe(); + } +}); +it("propagates a failed block deletion without disabling the schema", async () => { + mocks.deleteBlock.mockRejectedValueOnce(new Error("delete failed")); + await expect(deleteRelationSchema("failed-delete")).rejects.toThrow( + "delete failed", + ); + expect(isRelationSchemaDeleted("failed-delete")).toBe(false); + expect(mocks.setSetting).not.toHaveBeenCalled(); +}); +it("propagates setting failures while keeping the deleted schema unavailable", async () => { + mocks.setSetting.mockImplementationOnce(() => { + throw new Error("settings failed"); + }); + await expect(deleteRelationSchema("failed-settings")).rejects.toThrow( + "settings failed", + ); + expect(isRelationSchemaDeleted("failed-settings")).toBe(true); +}); +it("propagates configuration refresh failures to the caller", async () => { + mocks.refresh.mockImplementationOnce(() => { + throw new Error("refresh failed"); + }); + const pending = expect( + deleteRelationSchema("failed-refresh"), + ).rejects.toThrow("refresh failed"); + await vi.runAllTimersAsync(); + await pending; + expect(isRelationSchemaDeleted("failed-refresh")).toBe(true); +}); diff --git a/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts b/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts new file mode 100644 index 0000000000..1977ee7fde --- /dev/null +++ b/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts @@ -0,0 +1,170 @@ +// @vitest-environment jsdom +import React from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { act } from "react-dom/test-utils"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { CreateRelationButton } from "~/components/CreateRelationDialog"; +import SuggestionsBody from "~/components/SuggestionsBody"; +import { acceptImportedRelationSchema } from "~/utils/relationSchemaAcceptance"; +import { markRelationSchemaDeleted } from "~/utils/relationSchemaChanges"; +import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; +import { IMPORTED_FROM_PROP_KEY } from "~/utils/importedSourceIdentity"; +import type { json } from "~/utils/getBlockProps"; + +const mocks = vi.hoisted(() => ({ + search: vi.fn().mockResolvedValue([]), + props: new Map>(), +})); +vi.mock("~/utils/hyde", () => ({ performHydeSearch: mocks.search })); +vi.mock("~/utils/getDiscourseNodes", () => ({ + default: () => [ + { type: "source", text: "Source", format: "{content}" }, + { type: "target", text: "Target", format: "{content}" }, + ], +})); +vi.mock("~/utils/findDiscourseNode", () => ({ + default: () => ({ type: "source" }), +})); +vi.mock("~/utils/getDiscourseRelations", () => ({ + default: () => [ + { + id: "mounted-schema", + source: "source", + destination: "target", + label: "supports", + complement: "supported by", + triples: [], + }, + ], +})); +vi.mock("~/utils/getDiscourseContextResults", () => ({ + default: vi.fn().mockResolvedValue([]), +})); +vi.mock("~/utils/storedRelations", () => ({ + getStoredRelationsEnabled: () => true, +})); +vi.mock("~/components/settings/utils/accessors", () => ({ + getGlobalSetting: () => [], +})); +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); +vi.mock("~/utils/notifySuggestiveModeAdoption", () => ({ + notifyBlockSuggestionAdded: vi.fn(), + notifyRelationSuggestionAdded: vi.fn(), +})); +vi.mock("~/utils/discourseContextMutationRefresh", () => ({ + refreshDiscourseContextsForMutatedUids: vi.fn(), +})); +vi.mock("roamjs-components/queries/getAllPageNames", () => ({ + default: () => [], +})); +vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ + default: () => "Source page", +})); +vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ + default: () => "source-page", +})); +vi.mock("roamjs-components/components/AutocompleteInput", () => ({ + default: () => null, +})); +vi.mock("roamjs-components/components/MenuItemSelect", () => ({ + default: () => null, +})); +vi.mock("roamjs-components/util/renderOverlay", () => ({ default: vi.fn() })); +vi.mock("roamjs-components/components/Toast", () => ({ render: vi.fn() })); +vi.mock("posthog-js", () => ({ default: { capture: vi.fn() } })); + +let container: HTMLDivElement; +let root: Root; +beforeEach(() => { + mocks.props.set("mounted-schema", { + [DISCOURSE_GRAPH_PROP_NAME]: { + [IMPORTED_FROM_PROP_KEY]: { + sourceNodeRid: "orn:obsidian.schema:vault/relation", + sourceModifiedAt: "2026-08-01T00:00:00.000Z", + }, + }, + }); + Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + Object.assign(window, { + roamAlphaAPI: { + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": mocks.props.get(uid) ?? {}, + }), + data: { + block: { + update: vi.fn( + ({ + block, + }: { + block: { uid: string; props: Record }; + }) => { + mocks.props.set(block.uid, block.props); + return Promise.resolve(); + }, + ), + }, + backend: { q: vi.fn().mockResolvedValue([]) }, + }, + }, + }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); +afterEach(() => { + act(() => root.unmount()); + container.remove(); + vi.clearAllMocks(); +}); +const clickAllPages = async (): Promise => { + const button = [...container.querySelectorAll("button")].find( + (b) => b.textContent === "All Pages", + ); + expect(button).toBeDefined(); + await act(() => { + button?.click(); + return Promise.resolve(); + }); +}; +it("refreshes both mounted creation interfaces after acceptance and deletion", async () => { + await act(() => { + root.render( + React.createElement( + React.Fragment, + null, + React.createElement(CreateRelationButton, { + sourceNodeUid: "source-page", + }), + React.createElement(SuggestionsBody, { + tag: "Source page", + blockUid: "suggestions-block", + }), + ), + ); + return Promise.resolve(); + }); + const addButton = [...container.querySelectorAll("button")].find( + (b) => b.textContent === "Add relation", + ); + expect(addButton?.disabled).toBe(true); + await clickAllPages(); + expect(mocks.search).toHaveBeenLastCalledWith( + expect.objectContaining({ validTypes: [], uniqueRelationTypeTriplets: [] }), + ); + await act(async () => { + await acceptImportedRelationSchema("mounted-schema"); + }); + expect(addButton?.disabled).toBe(false); + await clickAllPages(); + expect(mocks.search).toHaveBeenLastCalledWith( + expect.objectContaining({ validTypes: ["target"] }), + ); + act(() => { + markRelationSchemaDeleted("mounted-schema"); + }); + expect(addButton?.disabled).toBe(true); + await clickAllPages(); + expect(mocks.search).toHaveBeenLastCalledWith( + expect.objectContaining({ validTypes: [], uniqueRelationTypeTriplets: [] }), + ); +}); diff --git a/apps/roam/src/utils/deleteRelationSchema.ts b/apps/roam/src/utils/deleteRelationSchema.ts new file mode 100644 index 0000000000..c22e8dac4c --- /dev/null +++ b/apps/roam/src/utils/deleteRelationSchema.ts @@ -0,0 +1,18 @@ +import deleteBlock from "roamjs-components/writes/deleteBlock"; +import { + getGlobalSettings, + setGlobalSetting, +} from "~/components/settings/utils/accessors"; +import { GLOBAL_KEYS } from "~/components/settings/utils/settingKeys"; +import refreshConfigTree from "./refreshConfigTree"; +import { markRelationSchemaDeleted } from "./relationSchemaChanges"; + +export const deleteRelationSchema = async (uid: string): Promise => { + await deleteBlock(uid); + markRelationSchemaDeleted(uid); + const remaining = { ...getGlobalSettings().Relations }; + delete remaining[uid]; + setGlobalSetting([GLOBAL_KEYS.relations], remaining); + await new Promise((resolve) => setTimeout(resolve, 50)); + refreshConfigTree(); +}; diff --git a/apps/roam/src/utils/relationSchemaAcceptance.ts b/apps/roam/src/utils/relationSchemaAcceptance.ts index 4bcb0f5efd..7b80796594 100644 --- a/apps/roam/src/utils/relationSchemaAcceptance.ts +++ b/apps/roam/src/utils/relationSchemaAcceptance.ts @@ -1,3 +1,7 @@ +import { + isRelationSchemaDeleted, + notifyRelationSchemaChange, +} from "./relationSchemaChanges"; import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; import getBlockProps from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; @@ -44,7 +48,11 @@ export const isProvisionalRelationSchema = ( export const excludeProvisionalRelationSchemas = ( relations: T[], ): T[] => - relations.filter((relation) => !isProvisionalRelationSchema(relation.id)); + relations.filter( + (relation) => + !isRelationSchemaDeleted(relation.id) && + !isProvisionalRelationSchema(relation.id), + ); export const acceptImportedRelationSchema = async ( relationSchemaUid: string, @@ -57,4 +65,5 @@ export const acceptImportedRelationSchema = async ( [RELATION_SCHEMA_STATUS_PROP_KEY]: ACCEPTED_STATUS, }, }); + notifyRelationSchemaChange(); }; diff --git a/apps/roam/src/utils/relationSchemaChanges.ts b/apps/roam/src/utils/relationSchemaChanges.ts new file mode 100644 index 0000000000..da84c786fb --- /dev/null +++ b/apps/roam/src/utils/relationSchemaChanges.ts @@ -0,0 +1,39 @@ +import { useEffect, useState } from "react"; + +let revision = 0; +const listeners = new Set<() => void>(); +const deletedSchemaIds = new Set(); + +export const subscribeToRelationSchemaChanges = ( + listener: () => void, +): (() => void) => { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +}; + +export const notifyRelationSchemaChange = (): void => { + revision += 1; + listeners.forEach((listener) => listener()); +}; + +export const markRelationSchemaDeleted = (uid: string): void => { + // Mounted canvases retain schema metadata to render their existing shapes. + deletedSchemaIds.add(uid); + notifyRelationSchemaChange(); +}; + +export const isRelationSchemaDeleted = (uid: string): boolean => + deletedSchemaIds.has(uid); + +export const useRelationSchemaRevision = (): number => { + const [currentRevision, setCurrentRevision] = useState(revision); + useEffect(() => { + const update = (): void => setCurrentRevision(revision); + const unsubscribe = subscribeToRelationSchemaChanges(update); + update(); + return unsubscribe; + }, []); + return currentRevision; +}; From 3907bca5dab8398b81ceec45f0b688aed44defe6 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 17 Sep 2026 23:29:46 +0530 Subject: [PATCH 08/17] ENG-2131 Keep getAllRelations for the canvas callers added on main --- apps/roam/src/components/canvas/canvasUtils.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/roam/src/components/canvas/canvasUtils.ts b/apps/roam/src/components/canvas/canvasUtils.ts index 2e8058deba..837d58457b 100644 --- a/apps/roam/src/components/canvas/canvasUtils.ts +++ b/apps/roam/src/components/canvas/canvasUtils.ts @@ -19,10 +19,11 @@ export const isDiscourseNodeShape = ( } }; +export const getAllRelations = () => + Object.values(discourseContext.relations).flat(); + export const getCreatableRelations = () => - Object.values(discourseContext.relations) - .flat() - .filter(isAcceptedRelationSchema); + getAllRelations().filter(isAcceptedRelationSchema); export const checkConnectionType = ( relation: { source: string; destination: string }, From 72f9aa6bb5c3606c496d959e7b1f5ae1633ffcba Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 17 Sep 2026 23:33:53 +0530 Subject: [PATCH 09/17] ENG-2131 Mock internalError in the acceptance test after the main merge --- apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts index e45710ca9a..376a3b4912 100644 --- a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts +++ b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts @@ -10,6 +10,8 @@ import { } from "~/utils/relationSchemaAcceptance"; import type { json } from "~/utils/getBlockProps"; +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + const SOURCE_NODE_RID = "orn:obsidian.schema:vault-a/relation-type-1"; const SOURCE_MODIFIED_AT = "2026-08-01T12:00:00.000Z"; const SCHEMA_UID = "relation-schema-uid"; From 51a63f0faa898599464a3e8c22385bd0bc8e8e0a Mon Sep 17 00:00:00 2001 From: sid597 Date: Fri, 18 Sep 2026 21:58:23 +0530 Subject: [PATCH 10/17] ENG-2131 Preserve shared helper imports after rebase --- .../src/utils/__tests__/relationSchemaAcceptance.test.ts | 6 ++++-- .../utils/__tests__/relationSchemaCreationRefresh.test.ts | 6 ++++-- apps/roam/src/utils/relationSchemaAcceptance.ts | 3 +-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts index 376a3b4912..2625d00fcf 100644 --- a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts +++ b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts @@ -1,6 +1,8 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; -import { IMPORTED_FROM_PROP_KEY } from "~/utils/importedSourceIdentity"; +import { + DISCOURSE_GRAPH_PROP_NAME, + IMPORTED_FROM_PROP_KEY, +} from "~/utils/createReifiedBlock"; import { acceptImportedRelationSchema, excludeProvisionalRelationSchemas, diff --git a/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts b/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts index 1977ee7fde..5830f8051a 100644 --- a/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts +++ b/apps/roam/src/utils/__tests__/relationSchemaCreationRefresh.test.ts @@ -7,8 +7,10 @@ import { CreateRelationButton } from "~/components/CreateRelationDialog"; import SuggestionsBody from "~/components/SuggestionsBody"; import { acceptImportedRelationSchema } from "~/utils/relationSchemaAcceptance"; import { markRelationSchemaDeleted } from "~/utils/relationSchemaChanges"; -import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; -import { IMPORTED_FROM_PROP_KEY } from "~/utils/importedSourceIdentity"; +import { + DISCOURSE_GRAPH_PROP_NAME, + IMPORTED_FROM_PROP_KEY, +} from "~/utils/createReifiedBlock"; import type { json } from "~/utils/getBlockProps"; const mocks = vi.hoisted(() => ({ diff --git a/apps/roam/src/utils/relationSchemaAcceptance.ts b/apps/roam/src/utils/relationSchemaAcceptance.ts index 7b80796594..af8a882375 100644 --- a/apps/roam/src/utils/relationSchemaAcceptance.ts +++ b/apps/roam/src/utils/relationSchemaAcceptance.ts @@ -3,10 +3,9 @@ import { notifyRelationSchemaChange, } from "./relationSchemaChanges"; import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; -import getBlockProps from "./getBlockProps"; +import getBlockProps, { isJsonObject } from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; import { - isJsonObject, parseImportedSourceIdentity, type ImportedSourceIdentity, } from "./importedSourceIdentity"; From 2abeb888122dba6c435791ae0f4c4eed1ec06bea Mon Sep 17 00:00:00 2001 From: sid597 Date: Wed, 23 Sep 2026 22:40:32 +0530 Subject: [PATCH 11/17] ENG-2131 Keep mounted canvases from reloading on schema changes Accepting or deleting a relation schema re-rendered TldrawCanvasShared. In local sync mode that rebuilds the store adapter args, so useRoamStore made a new store from the page-load snapshot and the next edit saved it over the page. The canvas now only listens for schema changes to leave a deleted relation tool, the tool panel refreshes its own list, and isAcceptedRelationSchema reads acceptance live instead of from a canvas-owned cache. --- .../DiscourseRelationTool.tsx | 6 +- .../DiscourseRelationUtil.tsx | 7 +- .../components/canvas/DiscourseToolPanel.tsx | 9 +- apps/roam/src/components/canvas/Tldraw.tsx | 59 +++---- .../roam/src/components/canvas/canvasUtils.ts | 9 +- .../__tests__/discourseGraphPanel.test.ts | 144 ++++++++++++++++++ .../relationSchemaAcceptance.test.ts | 18 +++ .../src/utils/relationSchemaAcceptance.ts | 11 +- 8 files changed, 205 insertions(+), 58 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/discourseGraphPanel.test.ts diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx index 997c8c4d46..50069d1dbf 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationTool.tsx @@ -8,10 +8,8 @@ import { DiscourseRelationShape, getRelationColor, } from "./DiscourseRelationUtil"; -import { - discourseContext, - isAcceptedRelationSchema, -} from "~/components/canvas/Tldraw"; +import { discourseContext } from "~/components/canvas/Tldraw"; +import { isAcceptedRelationSchema } from "~/utils/relationSchemaAcceptance"; import { dispatchToastEvent } from "~/components/canvas/ToastListener"; import { isRelationComplete } from "~/utils/isRelationComplete"; import { diff --git a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx index 2e94da16b4..9ee4f3c4e6 100644 --- a/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx +++ b/apps/roam/src/components/canvas/DiscourseRelationShape/DiscourseRelationUtil.tsx @@ -66,11 +66,8 @@ import { import { createReifiedRelation } from "~/utils/createReifiedBlock"; import { getStoredRelationsEnabled } from "~/utils/storedRelations"; import type { DiscourseRelation } from "~/utils/getDiscourseRelations"; -import { - discourseContext, - isAcceptedRelationSchema, - isPageUid, -} from "~/components/canvas/Tldraw"; +import { discourseContext, isPageUid } from "~/components/canvas/Tldraw"; +import { isAcceptedRelationSchema } from "~/utils/relationSchemaAcceptance"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; /** diff --git a/apps/roam/src/components/canvas/DiscourseToolPanel.tsx b/apps/roam/src/components/canvas/DiscourseToolPanel.tsx index e9fda93f8a..e1ee82a1cf 100644 --- a/apps/roam/src/components/canvas/DiscourseToolPanel.tsx +++ b/apps/roam/src/components/canvas/DiscourseToolPanel.tsx @@ -15,6 +15,8 @@ import { useAtom } from "@tldraw/state-react"; import { TOOL_ARROW_ICON_SVG, NODE_COLOR_ICON_SVG } from "~/icons"; import { getDiscourseNodeColors } from "~/utils/getDiscourseNodeColors"; import { DEFAULT_WIDTH, DEFAULT_HEIGHT } from "./Tldraw"; +import { hasAcceptedRelationSchema } from "./canvasUtils"; +import { useRelationSchemaRevision } from "~/utils/relationSchemaChanges"; import { DEFAULT_STYLE_PROPS, DISCOURSE_NODE_SHAPE_TYPE, @@ -76,7 +78,12 @@ const DiscourseGraphPanel = ({ [editor], ); - const uniqueRelations = useMemo(() => [...new Set(relations)], [relations]); + const relationSchemaRevision = useRelationSchemaRevision(); + const uniqueRelations = useMemo( + () => [...new Set(relations)].filter(hasAcceptedRelationSchema), + // eslint-disable-next-line react-hooks/exhaustive-deps + [relations, relationSchemaRevision], + ); const currentNodeTool = nodes.find((node) => node.type === currentToolId); const currentRelationTool = uniqueRelations.find( diff --git a/apps/roam/src/components/canvas/Tldraw.tsx b/apps/roam/src/components/canvas/Tldraw.tsx index 5c5bb62e55..235d06ef6e 100644 --- a/apps/roam/src/components/canvas/Tldraw.tsx +++ b/apps/roam/src/components/canvas/Tldraw.tsx @@ -1,7 +1,4 @@ -import { - isRelationSchemaDeleted, - useRelationSchemaRevision, -} from "~/utils/relationSchemaChanges"; +import { subscribeToRelationSchemaChanges } from "~/utils/relationSchemaChanges"; import React, { useState, useRef, @@ -58,7 +55,7 @@ import { import "tldraw/tldraw.css"; import tldrawStyles from "./tldrawStyles"; import { DragHandleOverlay } from "./overlays/DragHandleOverlay"; -import { isDiscourseNodeShape } from "./canvasUtils"; +import { hasAcceptedRelationSchema, isDiscourseNodeShape } from "./canvasUtils"; import getDiscourseNodes, { DiscourseNode } from "~/utils/getDiscourseNodes"; import getDiscourseRelations, { DiscourseRelation, @@ -124,7 +121,6 @@ import posthog from "posthog-js"; import { getPersonalSetting } from "~/components/settings/utils/accessors"; import { PERSONAL_KEYS } from "~/components/settings/utils/settingKeys"; import { json, normalizeProps } from "~/utils/getBlockProps"; -import { isProvisionalRelationSchema } from "~/utils/relationSchemaAcceptance"; import { onPageRefObserverChange } from "~/utils/pageRefObserverHandlers"; declare global { @@ -138,9 +134,6 @@ export type DiscourseContextType = { nodes: Record; // { [Relation.Label] => DiscourseRelation[] } relations: Record; - // Imported, not-yet-accepted relation schemas; excluded from relation - // creation but kept in `relations` so existing shapes still render. - provisionalRelationIds: Set; lastAppEvent: string; lastActions: HistoryEntry[]; }; @@ -148,15 +141,10 @@ export type DiscourseContextType = { export const discourseContext: DiscourseContextType = { nodes: {}, relations: {}, - provisionalRelationIds: new Set(), lastAppEvent: "", lastActions: [], }; -export const isAcceptedRelationSchema = (relation: { id: string }): boolean => - !discourseContext.provisionalRelationIds.has(relation.id) && - !isRelationSchemaDeleted(relation.id); - let activeCanvasPageUid: string | null = null; let activeCanvasEditor: Editor | null = null; @@ -791,34 +779,29 @@ const TldrawCanvasShared = ({ const allRelationIds = useMemo(() => { return Object.keys(allRelationsById); }, [allRelationsById]); - const relationSchemaRevision = useRelationSchemaRevision(); const registeredRelationNames = useMemo( () => [...new Set(allRelations.map((relation) => relation.label))], [allRelations], ); - const allRelationNames = useMemo(() => { - discourseContext.provisionalRelationIds = new Set( - allRelations - .filter((r) => isProvisionalRelationSchema(r.id)) - .map((r) => r.id), - ); - return Object.entries(discourseContext.relations) - .filter(([, relations]) => relations.some(isAcceptedRelationSchema)) - .map(([name]) => name); - // Acceptance and deletion invalidate the relation data stored outside React. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [allRelations, relationSchemaRevision]); - useEffect(() => { - const editor = appRef.current; - if (!editor) return; - const tool = editor.getCurrentToolId(); - if ( - registeredRelationNames.includes(tool) && - !allRelationNames.includes(tool) - ) { - editor.setCurrentTool("select"); - } - }, [allRelationNames, registeredRelationNames]); + const allRelationNames = useMemo( + () => registeredRelationNames.filter(hasAcceptedRelationSchema), + [registeredRelationNames], + ); + useEffect( + () => + subscribeToRelationSchemaChanges(() => { + const editor = appRef.current; + if (!editor) return; + const tool = editor.getCurrentToolId(); + if ( + registeredRelationNames.includes(tool) && + !hasAcceptedRelationSchema(tool) + ) { + editor.setCurrentTool("select"); + } + }), + [registeredRelationNames], + ); const allNodes = useMemo(() => { const allNodes = getDiscourseNodes(); discourseContext.nodes = Object.fromEntries( diff --git a/apps/roam/src/components/canvas/canvasUtils.ts b/apps/roam/src/components/canvas/canvasUtils.ts index 837d58457b..714e65fc8e 100644 --- a/apps/roam/src/components/canvas/canvasUtils.ts +++ b/apps/roam/src/components/canvas/canvasUtils.ts @@ -3,10 +3,8 @@ import { DiscourseNodeUtil, DiscourseNodeShape, } from "~/components/canvas/DiscourseNodeUtil"; -import { - discourseContext, - isAcceptedRelationSchema, -} from "~/components/canvas/Tldraw"; +import { discourseContext } from "~/components/canvas/Tldraw"; +import { isAcceptedRelationSchema } from "~/utils/relationSchemaAcceptance"; export const isDiscourseNodeShape = ( editor: Editor, @@ -25,6 +23,9 @@ export const getAllRelations = () => export const getCreatableRelations = () => getAllRelations().filter(isAcceptedRelationSchema); +export const hasAcceptedRelationSchema = (relationLabel: string): boolean => + !!discourseContext.relations[relationLabel]?.some(isAcceptedRelationSchema); + export const checkConnectionType = ( relation: { source: string; destination: string }, sourceNodeType: string, diff --git a/apps/roam/src/utils/__tests__/discourseGraphPanel.test.ts b/apps/roam/src/utils/__tests__/discourseGraphPanel.test.ts new file mode 100644 index 0000000000..b32714b8ed --- /dev/null +++ b/apps/roam/src/utils/__tests__/discourseGraphPanel.test.ts @@ -0,0 +1,144 @@ +// @vitest-environment jsdom +import React from "react"; +import { createRoot, type Root } from "react-dom/client"; +import { act } from "react-dom/test-utils"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import DiscourseGraphPanel from "~/components/canvas/DiscourseToolPanel"; +import { + DISCOURSE_GRAPH_PROP_NAME, + IMPORTED_FROM_PROP_KEY, +} from "~/utils/createReifiedBlock"; +import { acceptImportedRelationSchema } from "~/utils/relationSchemaAcceptance"; +import { markRelationSchemaDeleted } from "~/utils/relationSchemaChanges"; +import type { json } from "~/utils/getBlockProps"; + +const mocks = vi.hoisted(() => ({ + props: new Map>(), + discourseContext: { + nodes: {}, + relations: {}, + }, +})); +vi.mock("tldraw", () => { + const editor = { + getCurrentToolId: () => "discourse-tool", + getZoomLevel: () => 1, + setCurrentTool: vi.fn(), + }; + return { + useEditor: () => editor, + useValue: (_name: string, compute: () => unknown) => compute(), + useQuickReactor: vi.fn(), + createShapeId: vi.fn(), + Vec: class {}, + Box: class {}, + FONT_FAMILIES: { sans: "sans" }, + }; +}); +vi.mock("@tldraw/state-react", () => ({ + useAtom: (_name: string, initial: () => unknown) => { + const value = initial(); + return { get: () => value, set: vi.fn() }; + }, +})); +vi.mock("~/components/canvas/Tldraw", () => ({ + DEFAULT_WIDTH: 160, + DEFAULT_HEIGHT: 64, + discourseContext: mocks.discourseContext, +})); +vi.mock("~/components/canvas/DiscourseNodeUtil", () => ({ + DiscourseNodeUtil: class {}, + DEFAULT_STYLE_PROPS: {}, + DISCOURSE_NODE_SHAPE_TYPE: "discourse-node", + FONT_SIZES: { s: 12 }, +})); +vi.mock( + "~/components/canvas/DiscourseRelationShape/DiscourseRelationUtil", + () => ({ getRelationColor: () => "black" }), +); +vi.mock("~/components/settings/DiscourseNodeCanvasSettings", () => ({ + formatHexColor: () => "", +})); +vi.mock("~/utils/getDiscourseNodeColors", () => ({ + getDiscourseNodeColors: () => ({ backgroundColor: "", textColor: "" }), +})); +vi.mock("~/icons", () => ({ + TOOL_ARROW_ICON_SVG: "", + NODE_COLOR_ICON_SVG: "", +})); +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + +const importedFrom = { + [IMPORTED_FROM_PROP_KEY]: { + sourceNodeRid: "orn:obsidian.schema:vault/relation", + sourceModifiedAt: "2026-08-01T00:00:00.000Z", + }, +}; + +let container: HTMLDivElement; +let root: Root; +beforeEach(() => { + mocks.props.clear(); + mocks.props.set("imported-supports", { + [DISCOURSE_GRAPH_PROP_NAME]: importedFrom, + }); + mocks.discourseContext.relations = { + opposes: [{ id: "local-opposes" }], + supports: [{ id: "imported-supports" }], + }; + Object.assign(globalThis, { IS_REACT_ACT_ENVIRONMENT: true }); + Object.assign(window, { + roamAlphaAPI: { + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": mocks.props.get(uid) ?? {}, + }), + data: { + block: { + update: ({ + block, + }: { + block: { uid: string; props: Record }; + }) => { + mocks.props.set(block.uid, block.props); + return Promise.resolve(); + }, + }, + }, + }, + }); + container = document.createElement("div"); + document.body.appendChild(container); + root = createRoot(container); +}); +afterEach(() => { + act(() => root.unmount()); + container.remove(); +}); + +const listedRelations = (): string[] => + [...container.querySelectorAll("[data-drag_item_index] > span:last-child")] + .map((span) => span.textContent ?? "") + .filter((text) => text in mocks.discourseContext.relations); + +it("refilters relation tools itself when a schema is accepted or deleted", async () => { + await act(() => { + root.render( + React.createElement(DiscourseGraphPanel, { + nodes: [], + relations: ["opposes", "supports"], + }), + ); + return Promise.resolve(); + }); + expect(listedRelations()).toEqual(["opposes"]); + + await act(async () => { + await acceptImportedRelationSchema("imported-supports"); + }); + expect(listedRelations()).toEqual(["opposes", "supports"]); + + act(() => { + markRelationSchemaDeleted("imported-supports"); + }); + expect(listedRelations()).toEqual(["opposes"]); +}); diff --git a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts index 2625d00fcf..ac2b0ca78e 100644 --- a/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts +++ b/apps/roam/src/utils/__tests__/relationSchemaAcceptance.test.ts @@ -6,10 +6,12 @@ import { import { acceptImportedRelationSchema, excludeProvisionalRelationSchemas, + isAcceptedRelationSchema, isProvisionalRelationSchema, readRelationSchemaImportMeta, RELATION_SCHEMA_STATUS_PROP_KEY, } from "~/utils/relationSchemaAcceptance"; +import { markRelationSchemaDeleted } from "~/utils/relationSchemaChanges"; import type { json } from "~/utils/getBlockProps"; vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); @@ -134,3 +136,19 @@ describe("excludeProvisionalRelationSchemas", () => { ]); }); }); + +describe("isAcceptedRelationSchema", () => { + it("reads acceptance and deletion live, without a refresh step", async () => { + const schema = { id: "live-schema-uid" }; + propsByUid.set(schema.id, { + [DISCOURSE_GRAPH_PROP_NAME]: importedFromProps, + }); + expect(isAcceptedRelationSchema(schema)).toBe(false); + + await acceptImportedRelationSchema(schema.id); + expect(isAcceptedRelationSchema(schema)).toBe(true); + + markRelationSchemaDeleted(schema.id); + expect(isAcceptedRelationSchema(schema)).toBe(false); + }); +}); diff --git a/apps/roam/src/utils/relationSchemaAcceptance.ts b/apps/roam/src/utils/relationSchemaAcceptance.ts index af8a882375..983684d5bd 100644 --- a/apps/roam/src/utils/relationSchemaAcceptance.ts +++ b/apps/roam/src/utils/relationSchemaAcceptance.ts @@ -44,14 +44,13 @@ export const isProvisionalRelationSchema = ( ): boolean => readRelationSchemaImportMeta(relationSchemaUid)?.status === "provisional"; +export const isAcceptedRelationSchema = (relation: { id: string }): boolean => + !isRelationSchemaDeleted(relation.id) && + !isProvisionalRelationSchema(relation.id); + export const excludeProvisionalRelationSchemas = ( relations: T[], -): T[] => - relations.filter( - (relation) => - !isRelationSchemaDeleted(relation.id) && - !isProvisionalRelationSchema(relation.id), - ); +): T[] => relations.filter(isAcceptedRelationSchema); export const acceptImportedRelationSchema = async ( relationSchemaUid: string, From 7576d3f2b045f39b056a92b9e4fe3b690e0e23e5 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Tue, 22 Sep 2026 11:02:16 -0400 Subject: [PATCH 12/17] ENG-2288 Retry a failed asset copy by bumping the Roam node's edit time --- apps/roam/src/components/Export.tsx | 2 +- .../utils/__tests__/requestAssetRetry.test.ts | 199 ++++++++++++++++++ .../__tests__/syncSharedNodeAssets.test.ts | 31 ++- apps/roam/src/utils/requestAssetRetry.ts | 96 +++++++++ apps/roam/src/utils/syncDgNodesToSupabase.ts | 40 +++- 5 files changed, 360 insertions(+), 8 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/requestAssetRetry.test.ts create mode 100644 apps/roam/src/utils/requestAssetRetry.ts diff --git a/apps/roam/src/components/Export.tsx b/apps/roam/src/components/Export.tsx index daa5a1040d..65b5a31bff 100644 --- a/apps/roam/src/components/Export.tsx +++ b/apps/roam/src/components/Export.tsx @@ -927,7 +927,7 @@ const ExportDialog: ExportDialogComponent = ({ messages.push( `${assets.failed.length} file${ assets.failed.length === 1 ? "" : "s" - } could not be copied.`, + } could not be copied. Publish again to retry.`, ); renderToast({ content: messages.join(" "), diff --git a/apps/roam/src/utils/__tests__/requestAssetRetry.test.ts b/apps/roam/src/utils/__tests__/requestAssetRetry.test.ts new file mode 100644 index 0000000000..2fdf6dc23c --- /dev/null +++ b/apps/roam/src/utils/__tests__/requestAssetRetry.test.ts @@ -0,0 +1,199 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; +import type { json } from "~/utils/getBlockProps"; +import internalError from "~/utils/internalError"; +import type { NodeAssetResult } from "~/utils/publishNodeAssets"; +import { + NEEDS_REFRESH_PROP_KEY, + requestAssetRetries, +} from "~/utils/requestAssetRetry"; + +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + +const NODE_UID = "tgWb6JozF"; +const OTHER_NODE_UID = "Xk3pQr8sT"; +const IMAGE = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2FlqP2ioVNC3.png?alt=media&token=5e6f7a8b"; +const SECOND_IMAGE = + "https://firebasestorage.googleapis.com/v0/b/firescript-577a2.appspot.com/o/imgs%2Fapp%2FMAPLab%2FsecondImage.png?alt=media&token=1a2b3c4d"; + +type Block = { page: string; text: string }; + +const blocks = new Map(); +const propsByUid = new Map>(); +const update = vi.fn( + ({ block }: { block: { uid: string; props: Record } }) => { + propsByUid.set(block.uid, block.props); + return Promise.resolve(); + }, +); + +const query = vi.fn((_query: string, pageUid: string, url: string) => + Promise.resolve( + [...blocks.entries()] + .filter(([, block]) => block.page === pageUid && block.text.includes(url)) + .map(([uid]) => uid), + ), +); + +const failed = (sourceLocalId: string, sourceRef: string): NodeAssetResult => ({ + status: "failed", + sourceLocalId, + sourceRef, + error: "descriptor 500", +}); + +const copied = (sourceLocalId: string, sourceRef: string): NodeAssetResult => + ({ + status: "copied", + sourceLocalId, + sourceRef, + contentHash: "abc", + }) as NodeAssetResult; + +beforeEach(() => { + blocks.clear(); + propsByUid.clear(); + update.mockClear(); + query.mockClear(); + vi.mocked(internalError).mockClear(); + (globalThis as { window: unknown }).window = { + roamAlphaAPI: { + data: { async: { q: query }, block: { update } }, + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": propsByUid.get(uid) ?? {}, + }), + }, + }; +}); + +describe("requestAssetRetries", () => { + it("marks the block holding a failed asset", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + + const retried = await requestAssetRetries([failed(NODE_UID, IMAGE)]); + + expect(retried).toEqual(new Set([NODE_UID])); + expect(propsByUid.get("imgBlock")).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: { [NEEDS_REFRESH_PROP_KEY]: true }, + }); + }); + + it("writes nothing when every asset copied", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + + const retried = await requestAssetRetries([copied(NODE_UID, IMAGE)]); + + expect(retried.size).toBe(0); + expect(update).not.toHaveBeenCalled(); + }); + + it("keeps the block's existing props, including other discourse-graph data", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + propsByUid.set("imgBlock", { + other: "kept", + [DISCOURSE_GRAPH_PROP_NAME]: { importedFrom: { sourceNodeRid: "rid" } }, + }); + + await requestAssetRetries([failed(NODE_UID, IMAGE)]); + + expect(propsByUid.get("imgBlock")).toEqual({ + other: "kept", + [DISCOURSE_GRAPH_PROP_NAME]: { + importedFrom: { sourceNodeRid: "rid" }, + [NEEDS_REFRESH_PROP_KEY]: true, + }, + }); + }); + + it("writes once per node, however many of its assets failed", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + blocks.set("secondBlock", { + page: NODE_UID, + text: `![](${SECOND_IMAGE})`, + }); + + await requestAssetRetries([ + failed(NODE_UID, IMAGE), + failed(NODE_UID, SECOND_IMAGE), + ]); + + expect(update).toHaveBeenCalledTimes(1); + }); + + it("does not retry an asset that only reaches the node from another page", async () => { + blocks.set("refSource", { + page: OTHER_NODE_UID, + text: `![](${IMAGE})`, + }); + + const retried = await requestAssetRetries([failed(NODE_UID, IMAGE)]); + + expect(retried.size).toBe(0); + expect(update).not.toHaveBeenCalled(); + }); + + it("uses another failed asset's block when the first has none on the page", async () => { + blocks.set("secondBlock", { + page: NODE_UID, + text: `![](${SECOND_IMAGE})`, + }); + + const retried = await requestAssetRetries([ + failed(NODE_UID, IMAGE), + failed(NODE_UID, SECOND_IMAGE), + ]); + + expect(retried).toEqual(new Set([NODE_UID])); + expect(propsByUid.has("secondBlock")).toBe(true); + }); + + it("writes props only, so the block's last editor is kept", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + + await requestAssetRetries([failed(NODE_UID, IMAGE)]); + + const [[{ block }]] = update.mock.calls; + expect(Object.keys(block).sort()).toEqual(["props", "uid"]); + }); + + it("reports a failed write and still marks the other nodes", async () => { + blocks.set("imgBlock", { + page: NODE_UID, + text: `![](${IMAGE})`, + }); + blocks.set("otherBlock", { + page: OTHER_NODE_UID, + text: `![](${IMAGE})`, + }); + update.mockRejectedValueOnce(new Error("write refused")); + + const retried = await requestAssetRetries([ + failed(NODE_UID, IMAGE), + failed(OTHER_NODE_UID, IMAGE), + ]); + + expect(retried).toEqual(new Set([OTHER_NODE_UID])); + expect(internalError).toHaveBeenCalledWith( + expect.objectContaining({ + type: "Asset retry failed", + context: { sourceLocalId: NODE_UID }, + }), + ); + }); +}); diff --git a/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts b/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts index 33bdf08d75..e73bc8c54b 100644 --- a/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts +++ b/apps/roam/src/utils/__tests__/syncSharedNodeAssets.test.ts @@ -18,6 +18,8 @@ const mocks = vi.hoisted(() => ({ // under test. vi.mock("~/utils/getDiscourseNodes", () => ({ default: () => [] })); vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); +const capture = vi.hoisted(() => vi.fn()); +vi.mock("posthog-js", () => ({ default: { capture } })); vi.mock("~/components/settings/utils/accessors", () => ({ isSyncEnabled: () => false, })); @@ -42,7 +44,10 @@ vi.mock("~/utils/roamToCrossAppConverters", () => ({ }), })); -import { upsertSharedNodesFullContentWithAssets } from "~/utils/syncDgNodesToSupabase"; +import { + reportSharedNodeAssets, + upsertSharedNodesFullContentWithAssets, +} from "~/utils/syncDgNodesToSupabase"; const NODE_UID = "tgWb6JozF"; const SECOND_IMAGE = @@ -171,3 +176,27 @@ describe("upsertSharedNodesFullContentWithAssets", () => { ]); }); }); + +describe("reportSharedNodeAssets", () => { + it("keeps the download token out of the failure event", () => { + reportSharedNodeAssets({ + results: [ + { + status: "failed", + sourceLocalId: NODE_UID, + sourceRef: IMAGE, + error: `Could not fetch asset (HTTP 500): ${IMAGE}`, + }, + ], + retried: new Set([NODE_UID]), + }); + + const imagePath = IMAGE.split("?")[0]; + expect(capture).toHaveBeenCalledWith("Sync shared node asset failed", { + sourceLocalId: NODE_UID, + sourceRef: imagePath, + error: `Could not fetch asset (HTTP 500): ${imagePath}`, + retryScheduled: true, + }); + }); +}); diff --git a/apps/roam/src/utils/requestAssetRetry.ts b/apps/roam/src/utils/requestAssetRetry.ts new file mode 100644 index 0000000000..d0ae61548e --- /dev/null +++ b/apps/roam/src/utils/requestAssetRetry.ts @@ -0,0 +1,96 @@ +import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; +import getBlockProps, { isJsonObject } from "./getBlockProps"; +import internalError from "./internalError"; +import type { NodeAssetResult } from "./publishNodeAssets"; +import { setBlockPropsAsync } from "./setBlockProps"; + +/** + * Makes the next sync re-upload a node whose asset copy failed. Any block write moves the + * page's `:page/edit-time`, which the sync compares against the start of its last + * successful run. Retries are unbounded. + * + * A props write, not a string rewrite: rewriting the string reassigns `:edit/user` to + * whoever runs the sync. That a props write leaves it alone is undocumented. + */ + +/** + * Written, never read, and never cleared: clearing it would re-upload the node once more. + * Whether an asset reached storage is answered by its `FileReference` row. + */ +export const NEEDS_REFRESH_PROP_KEY = "needsRefresh"; + +/** + * An asset that reaches the markdown through a block ref or an embed lives on another + * page, where a write would not move this node's edit time. Those are not retried. + */ +const findBlocksReferencingAsset = async ({ + pageUid, + assetUrl, +}: { + pageUid: string; + assetUrl: string; +}): Promise => + (await window.roamAlphaAPI.data.async.q( + `[:find [?uid ...] + :in $ ?page-uid ?url + :where + [?page :block/uid ?page-uid] + [?block :block/page ?page] + [?block :block/string ?text] + [(clojure.string/includes? ?text ?url)] + [?block :block/uid ?uid]]`, + pageUid, + assetUrl, + )) as unknown as string[]; + +/** `discourse-graph` may already hold other data, so the flag is merged into it. */ +const markBlockForAssetRetry = async (blockUid: string): Promise => { + const existing = getBlockProps(blockUid)[DISCOURSE_GRAPH_PROP_NAME]; + const discourseGraphProps = isJsonObject(existing) ? existing : {}; + await setBlockPropsAsync(blockUid, { + [DISCOURSE_GRAPH_PROP_NAME]: { + ...discourseGraphProps, + [NEEDS_REFRESH_PROP_KEY]: true, + }, + }); +}; + +/** + * One write per node is enough: the re-upload retries every asset of the node, including + * those with no block on the page. Returns the nodes marked. + */ +export const requestAssetRetries = async ( + results: NodeAssetResult[], +): Promise> => { + const failedByNode = new Map(); + for (const result of results) { + if (result.status !== "failed") continue; + const assetUrls = failedByNode.get(result.sourceLocalId) ?? []; + assetUrls.push(result.sourceRef); + failedByNode.set(result.sourceLocalId, assetUrls); + } + + const retried = new Set(); + for (const [pageUid, assetUrls] of failedByNode) { + try { + for (const assetUrl of assetUrls) { + const [blockUid] = await findBlocksReferencingAsset({ + pageUid, + assetUrl, + }); + if (blockUid === undefined) continue; + await markBlockForAssetRetry(blockUid); + retried.add(pageUid); + break; + } + } catch (error) { + internalError({ + error, + type: "Asset retry failed", + context: { sourceLocalId: pageUid }, + sendEmail: false, + }); + } + } + return retried; +}; diff --git a/apps/roam/src/utils/syncDgNodesToSupabase.ts b/apps/roam/src/utils/syncDgNodesToSupabase.ts index 843d74a725..6bbc28bd9b 100644 --- a/apps/roam/src/utils/syncDgNodesToSupabase.ts +++ b/apps/roam/src/utils/syncDgNodesToSupabase.ts @@ -29,6 +29,7 @@ import { summarizeAssetResults, type NodeAssetResult, } from "./publishNodeAssets"; +import { requestAssetRetries } from "./requestAssetRetry"; import type { DGSupabaseClient } from "@repo/database/lib/client"; import { intersection } from "@repo/utils/setOperations"; import { CORE_TITLE_PROBE_SELECT } from "@repo/database/lib/coreTitleBackfill"; @@ -930,10 +931,23 @@ 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. + * A Roam asset URL's query string carries a download token that grants access to the + * file, so it must not reach analytics. The path still identifies the asset. */ -const reportSharedNodeAssets = (results: NodeAssetResult[]): void => { +const withoutUrlQueries = (text: string): string => + text.replace(/(https?:\/\/[^\s?]+)\?[^\s)]*/g, "$1"); + +/** + * Retries are unbounded, so the per-asset events are how a node that keeps failing is + * found. + */ +export const reportSharedNodeAssets = ({ + results, + retried, +}: { + results: NodeAssetResult[]; + retried: ReadonlySet; +}): void => { if (results.length === 0) return; const { copied, unchanged, distinctBlobs, tooLarge, failed } = summarizeAssetResults(results); @@ -944,6 +958,16 @@ const reportSharedNodeAssets = (results: NodeAssetResult[]): void => { tooLarge: tooLarge.length, failed: failed.length, }); + // Per node, unlike the summary: the same file failing in two nodes is two retries. + for (const result of results) { + if (result.status !== "failed") continue; + posthog.capture("Sync shared node asset failed", { + sourceLocalId: result.sourceLocalId, + sourceRef: withoutUrlQueries(result.sourceRef), + error: withoutUrlQueries(result.error), + retryScheduled: retried.has(result.sourceLocalId), + }); + } if (failed.length > 0) { console.warn( `Sync could not copy ${failed.length} shared node assets`, @@ -1493,14 +1517,18 @@ export const createOrUpdateDiscourseEmbedding = async ( activeContext, ), }); - reportSharedNodeAssets( - await upsertSharedNodesFullContentWithAssets({ + const sharedNodeAssetResults = await upsertSharedNodesFullContentWithAssets( + { nodes: sharedFullContentNodes, supabaseClient: activeSupabaseClient, context: activeContext, phases, - }), + }, ); + reportSharedNodeAssets({ + results: sharedNodeAssetResults, + retried: await requestAssetRetries(sharedNodeAssetResults), + }); await measureSyncPhase({ phase: "convertConcepts", phases, From a51f9afd067cd180932b85b99c4e1bedbb983fe0 Mon Sep 17 00:00:00 2001 From: Marc-Antoine Parent Date: Mon, 21 Sep 2026 10:55:36 -0400 Subject: [PATCH 13/17] ENG-2202 Reconcile Roam node schema metadata between sync and publish Entire-Checkpoint: 01M327E9K9BPPZ1R8T0PTZ1KX7 --- .../utils/__tests__/conceptConversion.test.ts | 33 +++++- .../roamToCrossAppConverters.test.ts | 17 +++ .../__tests__/schemaMetadataParity.test.ts | 111 ++++++++++++++++++ apps/roam/src/utils/conceptConversion.ts | 26 +--- apps/roam/src/utils/nodeTemplateContent.ts | 24 ++++ .../src/utils/roamToCrossAppConverters.ts | 2 + .../lib/__tests__/crossAppConverters.test.ts | 10 +- .../database/src/lib/crossAppConverters.ts | 1 + 8 files changed, 197 insertions(+), 27 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/schemaMetadataParity.test.ts create mode 100644 apps/roam/src/utils/nodeTemplateContent.ts diff --git a/apps/roam/src/utils/__tests__/conceptConversion.test.ts b/apps/roam/src/utils/__tests__/conceptConversion.test.ts index 67252fc56f..c73c7c836f 100644 --- a/apps/roam/src/utils/__tests__/conceptConversion.test.ts +++ b/apps/roam/src/utils/__tests__/conceptConversion.test.ts @@ -108,23 +108,50 @@ describe("discourseNodeSchemaToLocalConcept source slot", () => { }); }); + // Not covered: an upsert whose author_local_id doesn't resolve nulls author_id. it("carries the type author as author_local_id", () => { const concept = discourseNodeSchemaToLocalConcept(CONTEXT, nodeType({})); expect(concept.author_local_id).toBe("author-1"); }); +}); - it("keeps the label and template it already carried", () => { +describe("discourseNodeSchemaToLocalConcept label and template", () => { + it("writes the template body to template_content, with no template title", () => { const concept = discourseNodeSchemaToLocalConcept( CONTEXT, - nodeType({ template: [{ text: "Question:" }] }), + nodeType({ + template: [{ text: "Question:", children: [{ text: "Answer" }] }], + }), ); expect(concept.literal_content).toEqual({ label: "Evidence", format: "[[EVD]] - {content} - {Source}", - template: "* Question:\n", + template_content: "* Question:\n * Answer\n \n", roles: ["sourceDocument"], }); }); + + it.each([ + ["no template", undefined], + ["an empty template", []], + ["a template of only components", [{ text: "{{query block}}" }]], + ])("omits template_content for %s", (_label, template) => { + const concept = discourseNodeSchemaToLocalConcept( + CONTEXT, + nodeType({ template }), + ); + expect(concept.literal_content).not.toHaveProperty("template_content"); + expect(concept.literal_content).not.toHaveProperty("template"); + }); + + it("keeps a slash-separated name whole in label and name", () => { + const concept = discourseNodeSchemaToLocalConcept( + CONTEXT, + nodeType({ text: "Evidence/Figure" }), + ); + expect(concept.name).toBe("Evidence/Figure"); + expect(concept.literal_content).toMatchObject({ label: "Evidence/Figure" }); + }); }); describe("discourseNodeBlockToLocalConcept core title", () => { diff --git a/apps/roam/src/utils/__tests__/roamToCrossAppConverters.test.ts b/apps/roam/src/utils/__tests__/roamToCrossAppConverters.test.ts index a050e04cfa..74992d828d 100644 --- a/apps/roam/src/utils/__tests__/roamToCrossAppConverters.test.ts +++ b/apps/roam/src/utils/__tests__/roamToCrossAppConverters.test.ts @@ -221,6 +221,23 @@ describe("nodeSchemaToCrossApp format", () => { }); }); +describe("nodeSchemaToCrossApp template", () => { + it("carries the template body, with no template title", () => { + const schema = convertSchema( + nodeSchema({ template: [{ text: "Question:" }] }), + ); + expect(schema?.template).toBe("* Question:\n"); + expect(schema?.templateTitle).toBeUndefined(); + }); + + it.each([ + ["no template", undefined], + ["an empty template", []], + ])("leaves the template out for %s", (_label, template) => { + expect(convertSchema(nodeSchema({ template }))?.template).toBeUndefined(); + }); +}); + describe("nodeSchemaToCrossApp source slot", () => { it("adds a sourceDocument slot definition pointing at the Source node type", () => { mockedGetDiscourseNodes.mockReturnValue([ diff --git a/apps/roam/src/utils/__tests__/schemaMetadataParity.test.ts b/apps/roam/src/utils/__tests__/schemaMetadataParity.test.ts new file mode 100644 index 0000000000..b9aa2d1029 --- /dev/null +++ b/apps/roam/src/utils/__tests__/schemaMetadataParity.test.ts @@ -0,0 +1,111 @@ +import { describe, expect, it, vi } from "vitest"; +import type { LocalConceptDataInput } from "@repo/database/inputTypes"; +import type { DiscourseNode } from "~/utils/getDiscourseNodes"; + +vi.mock("roamjs-components/queries/getFullTreeByParentUid", () => ({ + default: () => ({ children: [] }), +})); +vi.mock("roamjs-components/queries/getPageViewType", () => ({ + default: () => "bullet", +})); +vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ + default: () => "", +})); +vi.mock("~/utils/pageToMarkdown", () => ({ toMarkdown: () => "" })); +vi.mock("~/utils/getDiscourseRelations", () => ({ default: () => [] })); +vi.mock("~/utils/getDiscourseNodes", () => ({ + default: () => [SOURCE_TYPE], +})); + +// Sync reads the author through q, publish through pull; both see one creator. +vi.hoisted(() => { + (globalThis as { window?: unknown }).window = { + roamAlphaAPI: { + util: { generateUID: () => "someUid" }, + q: () => [["author-1", "page-1", 1000, 2000]], + pull: () => ({ + ":create/time": 1000, + ":page/edit-time": 2000, + ":create/user": { ":user/uid": "author-1" }, + }), + }, + }; +}); + +import { crossAppNodeSchemaToDbConcept } from "@repo/database/lib/crossAppConverters"; +import { discourseNodeSchemaToLocalConcept } from "~/utils/conceptConversion"; +import { nodeSchemaToCrossApp } from "~/utils/roamToCrossAppConverters"; + +const CONTEXT = { spaceId: 1, userId: 2 } as never; + +const nodeType = (overrides: Partial): DiscourseNode => ({ + text: "Evidence", + type: "_EVD-node", + shortcut: "e", + format: "[[EVD]] - {content} - {Source}", + specification: [], + backedBy: "user", + canvasSettings: {}, + ...overrides, +}); + +const SOURCE_TYPE = nodeType({ + text: "Source", + type: "src-node", + format: "@{content}", +}); + +// Timestamps and space fields are transport details that differ by path. +const schemaMetadata = (concept: LocalConceptDataInput) => ({ + name: concept.name, + source_local_id: concept.source_local_id, + author_local_id: concept.author_local_id, + literal_content: concept.literal_content, + local_reference_content: concept.local_reference_content, +}); + +const viaSync = (node: DiscourseNode) => + schemaMetadata(discourseNodeSchemaToLocalConcept(CONTEXT, node)); + +const viaPublish = (node: DiscourseNode) => { + const schema = nodeSchemaToCrossApp(node); + if (!schema) throw new Error("publish produced no schema"); + return schemaMetadata(crossAppNodeSchemaToDbConcept(schema)); +}; + +describe("node schema metadata from sync and publish", () => { + it.each([ + [ + "a template and a source slot", + nodeType({ + template: [{ text: "Question:", children: [{ text: "Answer" }] }], + }), + ], + ["a source slot and no template", nodeType({})], + [ + "a template and no source slot", + nodeType({ + text: "Claim", + type: "clm", + format: "[[CLM]] - {content}", + template: [{ text: "Grounds:" }], + }), + ], + [ + "neither a template nor a source slot", + nodeType({ text: "Claim", type: "clm", format: "[[CLM]] - {content}" }), + ], + [ + "a template of only components", + nodeType({ template: [{ text: "{{x}}" }] }), + ], + ["a slash-separated name", nodeType({ text: "Evidence/Figure" })], + ])("match for a schema with %s", (_label, node) => { + expect(viaPublish(node)).toEqual(viaSync(node)); + }); + + it("both carry the creator as author_local_id", () => { + expect(viaSync(nodeType({})).author_local_id).toBe("author-1"); + expect(viaPublish(nodeType({})).author_local_id).toBe("author-1"); + }); +}); diff --git a/apps/roam/src/utils/conceptConversion.ts b/apps/roam/src/utils/conceptConversion.ts index 3792ce0d7d..08a277c15b 100644 --- a/apps/roam/src/utils/conceptConversion.ts +++ b/apps/roam/src/utils/conceptConversion.ts @@ -1,4 +1,3 @@ -import { InputTextNode } from "roamjs-components/types"; import getBlockProps from "./getBlockProps"; import { DiscourseNode } from "./getDiscourseNodes"; import { @@ -8,6 +7,7 @@ import { sourceIdOfNode, } from "./sourceSlot"; import extractContentFromTitle from "./extractContentFromTitle"; +import { nodeTemplateContent } from "./nodeTemplateContent"; import getDiscourseRelations from "./getDiscourseRelations"; import type { DiscourseRelation } from "./getDiscourseRelations"; import type { SupabaseContext } from "~/utils/supabaseContext"; @@ -65,33 +65,17 @@ const getNodeExtraData = ( /* eslint-enable @typescript-eslint/naming-convention */ }; -const indent = (s: string): string => - s - .split("\n") - .map((l) => " " + l) - .join("\n") + "\n"; - -const templateToText = (template: InputTextNode[]): string => - template - .filter((itn) => !itn.text.startsWith("{{")) - .map( - (itn) => - `* ${itn.text}\n${itn.children?.length ? indent(templateToText(itn.children)) : ""}`, - ) - .join(""); - export const discourseNodeSchemaToLocalConcept = ( context: SupabaseContext, node: DiscourseNode, ): LocalConceptDataInput => { - const titleParts = node.text.split("/"); - const label = titleParts[titleParts.length - 1] ?? node.text; const literalContent: Record = { - label, + label: node.text, format: node.format, }; - if (node.template !== undefined) - literalContent.template = templateToText(node.template); + const templateContent = nodeTemplateContent(node.template); + if (templateContent !== undefined) + literalContent.template_content = templateContent; const hasSourceSlot = schemaHasSourceSlot(node); if (hasSourceSlot) literalContent.roles = [SOURCE_SLOT]; return { diff --git a/apps/roam/src/utils/nodeTemplateContent.ts b/apps/roam/src/utils/nodeTemplateContent.ts new file mode 100644 index 0000000000..b1782d286e --- /dev/null +++ b/apps/roam/src/utils/nodeTemplateContent.ts @@ -0,0 +1,24 @@ +import type { InputTextNode } from "roamjs-components/types"; + +const indent = (s: string): string => + s + .split("\n") + .map((l) => " " + l) + .join("\n") + "\n"; + +const templateToText = (template: InputTextNode[]): string => + template + .filter((itn) => !itn.text.startsWith("{{")) + .map( + (itn) => + `* ${itn.text}\n${itn.children?.length ? indent(templateToText(itn.children)) : ""}`, + ) + .join(""); + +// Shared by sync and publish so both write, or both omit, the same value. +export const nodeTemplateContent = ( + template: InputTextNode[] | undefined, +): string | undefined => { + const text = templateToText(template ?? []); + return text.length > 0 ? text : undefined; +}; diff --git a/apps/roam/src/utils/roamToCrossAppConverters.ts b/apps/roam/src/utils/roamToCrossAppConverters.ts index 5d91074690..8b076b40cb 100644 --- a/apps/roam/src/utils/roamToCrossAppConverters.ts +++ b/apps/roam/src/utils/roamToCrossAppConverters.ts @@ -17,6 +17,7 @@ import getPageViewType from "roamjs-components/queries/getPageViewType"; import { contentTypes } from "@repo/content-model"; import getDiscourseNodes from "./getDiscourseNodes"; import extractContentFromTitle from "./extractContentFromTitle"; +import { nodeTemplateContent } from "./nodeTemplateContent"; import { SOURCE_SLOT, schemaHasSourceSlot, @@ -225,6 +226,7 @@ export const nodeSchemaToCrossApp = ( createdAt: new Date(createdTime), modifiedAt: new Date(Math.max(pageEditTime, createdTime)), format: s.format, + template: nodeTemplateContent(s.template), ...(hasSourceSlot ? { slotDefinitions: { [SOURCE_SLOT]: sourceSlotSchemaId() } } : {}), diff --git a/packages/database/src/lib/__tests__/crossAppConverters.test.ts b/packages/database/src/lib/__tests__/crossAppConverters.test.ts index c5f1b52c42..33be178f3b 100644 --- a/packages/database/src/lib/__tests__/crossAppConverters.test.ts +++ b/packages/database/src/lib/__tests__/crossAppConverters.test.ts @@ -30,6 +30,7 @@ describe("crossAppNodeSchemaToDbConcept", () => { format: "[[CLM]] - {content}", }); expect(concept.literal_content).toEqual({ + label: "Some concept", format: "[[CLM]] - {content}", }); }); @@ -42,15 +43,17 @@ describe("crossAppNodeSchemaToDbConcept", () => { templateTitle: "Claim template", }); expect(concept.literal_content).toEqual({ + label: "Some concept", format: "[[CLM]] - {content}", template: "Claim template", template_content: "* Evidence\n", }); }); - it("omits literal_content when no keys are set", () => { + it("writes the label into literal_content as well as the name", () => { const concept = crossAppNodeSchemaToDbConcept(baseSchema); - expect(concept.literal_content).toBeUndefined(); + expect(concept.name).toBe("Some concept"); + expect(concept.literal_content).toEqual({ label: "Some concept" }); }); it("stores slot definitions as roles plus local reference content", () => { @@ -60,6 +63,7 @@ describe("crossAppNodeSchemaToDbConcept", () => { slotDefinitions: { evidence: "evidence-type", claim: "claim-type" }, }); expect(result.literal_content).toEqual({ + label: "Some concept", template: "Template Title", roles: ["evidence", "claim"], }); @@ -74,7 +78,7 @@ describe("crossAppNodeSchemaToDbConcept", () => { ...baseSchema, slotDefinitions: {}, }); - expect(result).not.toHaveProperty("literal_content"); + expect(result.literal_content).toEqual({ label: "Some concept" }); expect(result).not.toHaveProperty("local_reference_content"); }); }); diff --git a/packages/database/src/lib/crossAppConverters.ts b/packages/database/src/lib/crossAppConverters.ts index f6c71a8229..471e0aa555 100644 --- a/packages/database/src/lib/crossAppConverters.ts +++ b/packages/database/src/lib/crossAppConverters.ts @@ -98,6 +98,7 @@ export const crossAppNodeSchemaToDbConcept = ( ): LocalConceptDataInput => { const slots = Object.keys(node.slotDefinitions ?? {}); const literalInfo = filterUndefined({ + label: node.label, template: node.templateTitle, template_content: node.template, format: node.format, From 2494a670740ba3c49f2b24f6f2e300d3db3f4ff1 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 24 Sep 2026 22:40:07 +0530 Subject: [PATCH 14/17] ENG-2296 Simplify the Roam import dialog and import feedback Hide already imported nodes and drop the source app, source ID, and status columns. Sort by source space, title, or modified, newest first by default. Rename the dialog and command to Import shared nodes, give the dialog a fixed height, and show import progress and the result in one area that links each imported node by its Roam title. --- .../components/DiscoverSharedNodesDialog.tsx | 221 ++++++++++++------ .../utils/__tests__/importSharedNodes.test.ts | 33 ++- .../utils/__tests__/sortSharedNodes.test.ts | 79 +++++++ apps/roam/src/utils/importSharedNodes.ts | 8 +- .../utils/registerCommandPaletteCommands.ts | 2 +- apps/roam/src/utils/sortSharedNodes.ts | 54 +++++ 6 files changed, 322 insertions(+), 75 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/sortSharedNodes.test.ts create mode 100644 apps/roam/src/utils/sortSharedNodes.ts diff --git a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx index ba53226c46..5bdbb8cc73 100644 --- a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx +++ b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx @@ -5,15 +5,18 @@ import { Classes, Dialog, HTMLTable, + Icon, InputGroup, Intent, NonIdealState, + ProgressBar, Spinner, - Tag, Tooltip, } from "@blueprintjs/core"; import React, { useCallback, useEffect, useMemo, useState } from "react"; +import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; import createOverlayRender from "roamjs-components/util/createOverlayRender"; +import openBlockInSidebar from "roamjs-components/writes/openBlockInSidebar"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; import { discoverSharedNodes } from "~/utils/discoverSharedNodes"; import { @@ -23,6 +26,13 @@ import { } from "~/utils/importSharedNodes"; import { importSharedRelations } from "~/utils/importSharedRelations"; import internalError from "~/utils/internalError"; +import { + DEFAULT_SHARED_NODE_SORT, + getNextSharedNodeSort, + sortSharedNodes, + type SharedNodeSort, + type SharedNodeSortColumn, +} from "~/utils/sortSharedNodes"; import { getLoggedInClient, getSupabaseContext } from "~/utils/supabaseContext"; const IMPORT_ERROR_TYPE = "Shared node import failed"; @@ -33,13 +43,11 @@ const formatModifiedAt = (modifiedAt: string): string => const SharedNodeRow = ({ node, - alreadyImported, selected, selectionDisabled, onToggleSelected, }: { node: SharedNode; - alreadyImported: boolean; selected: boolean; selectionDisabled: boolean; onToggleSelected: () => void; @@ -54,9 +62,6 @@ const SharedNodeRow = ({ onChange={onToggleSelected} /> - - {node.platform} -
{node.spaceName} @@ -77,39 +82,72 @@ const SharedNodeRow = ({ {node.title}
- - {node.sourceLocalId ? ( -
- {node.sourceLocalId} -
- ) : ( - Not provided - )} - {formatModifiedAt(node.lastModified)} - - {alreadyImported ? ( - - Imported - - ) : ( - Available - )} - ); +const SortableHeader = ({ + label, + column, + sort, + onSort, +}: { + label: string; + column: SharedNodeSortColumn; + sort: SharedNodeSort; + onSort: (column: SharedNodeSortColumn) => void; +}): React.ReactElement => { + const isActive = sort.column === column; + return ( + + + + ); +}; + +const ImportedNodeLink = ({ + pageUid, + onOpenInMainWindow, +}: { + pageUid: string; + onOpenInMainWindow: () => void; +}): React.ReactElement => ( + { + if (event.shiftKey) { + void openBlockInSidebar(pageUid); + return; + } + void window.roamAlphaAPI.ui.mainWindow.openPage({ + page: { uid: pageUid }, + }); + onOpenInMainWindow(); + }} + > + {getPageTitleByPageUid(pageUid)} + +); + const ImportResultsSummary = ({ results, + onOpenInMainWindow, }: { results: SharedNodeImportItem[]; + onOpenInMainWindow: () => void; }) => { const importedCount = results.filter( (item) => item.status === "imported", @@ -118,30 +156,39 @@ const ImportResultsSummary = ({ (item) => item.status === "skipped", ).length; const failedImports = results.filter(isFailedSharedNodeImport); - const warnings = results.flatMap((item) => - item.status !== "failed" && item.warning - ? [{ sharedNode: item.sharedNode, message: item.warning }] - : [], + const completedImports = results.flatMap((item) => + item.status === "failed" ? [] : [item], ); - const importNotices = [...failedImports, ...warnings]; + const warningCount = completedImports.filter((item) => item.warning).length; return ( 0 ? Intent.WARNING : Intent.SUCCESS} - title={`${importedCount} imported, ${skippedCount} skipped, ${failedImports.length} failed${warnings.length > 0 ? `, ${warnings.length} with warnings` : ""}`} + intent={ + failedImports.length > 0 || warningCount > 0 + ? Intent.WARNING + : Intent.SUCCESS + } + title={`${importedCount} imported, ${skippedCount} skipped, ${failedImports.length} failed${warningCount > 0 ? `, ${warningCount} with warnings` : ""}`} > {skippedCount > 0 && (
Skipped nodes were already up to date in this graph.
)} - {importNotices.length > 0 && ( -
    - {importNotices.map((item) => ( -
  • - {item.sharedNode.title}:{" "} - {item.message} -
    • - ))} -
- )} +
    + {failedImports.map((item) => ( +
  • + {item.sharedNode.title}:{" "} + {item.message} +
    • + ))} + {completedImports.map((item) => ( +
  • + + {item.warning && `: ${item.warning}`} +
    • + ))} +
); }; @@ -152,6 +199,7 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { const [loading, setLoading] = useState(true); const [error, setError] = useState(""); const [searchTerm, setSearchTerm] = useState(""); + const [sort, setSort] = useState(DEFAULT_SHARED_NODE_SORT); const [selectedRids, setSelectedRids] = useState>(new Set()); const [spaceId, setSpaceId] = useState(0); const [importProgress, setImportProgress] = useState<{ @@ -201,10 +249,19 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { void loadNodes(); }, [loadNodes]); + const availableNodes = useMemo( + () => + sortSharedNodes({ + nodes: nodes.filter((node) => !importedRids.has(node.rid)), + sort, + }), + [importedRids, nodes, sort], + ); + const visibleNodes = useMemo(() => { const normalizedSearch = searchTerm.trim().toLocaleLowerCase(); - if (!normalizedSearch) return nodes; - return nodes.filter((node) => + if (!normalizedSearch) return availableNodes; + return availableNodes.filter((node) => [ node.platform, node.spaceName, @@ -213,7 +270,7 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { node.sourceLocalId, ].some((value) => value.toLocaleLowerCase().includes(normalizedSearch)), ); - }, [nodes, searchTerm]); + }, [availableNodes, searchTerm]); const visibleRids = visibleNodes.map((node) => node.rid); const allVisibleSelected = @@ -238,6 +295,10 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { }); }; + const handleSort = (column: SharedNodeSortColumn): void => { + setSort((currentSort) => getNextSharedNodeSort({ currentSort, column })); + }; + const importSelectedNodes = async (): Promise => { const selectedNodes = nodes.filter((node) => selectedRids.has(node.rid)); @@ -302,13 +363,16 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { canOutsideClickClose={!importing} enforceFocus={false} isCloseButtonShown={!importing} - style={{ width: "min(68rem, calc(100vw - 2rem))" }} + style={{ + width: "min(68rem, calc(100vw - 2rem))", + height: "min(48rem, calc(100vh - 4rem))", + }} isOpen onClose={onClose} - title="Discover shared nodes" + title="Import shared nodes" >
@@ -333,7 +397,23 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => {
- {importResults && } + {importProgress ? ( + + + + ) : ( + importResults && ( + + ) + )} {loading ? (
@@ -351,7 +431,9 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => {
@@ -370,12 +452,24 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { onChange={toggleAllVisibleSelected} /> - Source app - Source space - Title - Source ID - Modified - Status + + + @@ -383,7 +477,6 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { toggleNodeSelected(node.rid)} selected={selectedRids.has(node.rid)} selectionDisabled={importing} @@ -399,7 +492,7 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { {loading || error ? "" - : `${visibleNodes.length} of ${nodes.length} nodes`} + : `${visibleNodes.length} of ${availableNodes.length} nodes`}
diff --git a/apps/roam/src/utils/__tests__/importSharedNodes.test.ts b/apps/roam/src/utils/__tests__/importSharedNodes.test.ts index 50a0f01b57..e0f75c6ce7 100644 --- a/apps/roam/src/utils/__tests__/importSharedNodes.test.ts +++ b/apps/roam/src/utils/__tests__/importSharedNodes.test.ts @@ -109,9 +109,17 @@ describe("importSharedNodes", () => { }); expect(items).toEqual([ - { sharedNode: sharedNodes[0], status: "imported" }, - { sharedNode: sharedNodes[1], status: "imported" }, - { sharedNode: sharedNodes[2], status: "skipped" }, + { + sharedNode: sharedNodes[0], + status: "imported", + pageUid: "page-node-1", + }, + { + sharedNode: sharedNodes[1], + status: "imported", + pageUid: "page-node-2", + }, + { sharedNode: sharedNodes[2], status: "skipped", pageUid: "page-node-3" }, { sharedNode: sharedNodes[3], status: "failed", @@ -270,6 +278,7 @@ describe("importSharedNodes", () => { { sharedNode: sharedNodes[0], status: "imported", + pageUid: "page-node-1", warning: "No source was published with this node.", }, ]); @@ -294,7 +303,11 @@ describe("importSharedNodes", () => { status: "failed", message: "roam api unavailable", }, - { sharedNode: sharedNodes[1], status: "imported" }, + { + sharedNode: sharedNodes[1], + status: "imported", + pageUid: "page-node-2", + }, ]); }); it("imports a discovered source this graph lacks before the node that names it", async () => { @@ -331,8 +344,8 @@ describe("importSharedNodes", () => { mockedMaterializeSharedNode.mock.calls.map(([args]) => args.sharedNode), ).toEqual([source, evidence]); expect(items).toEqual([ - { sharedNode: source, status: "imported" }, - { sharedNode: evidence, status: "imported" }, + { sharedNode: source, status: "imported", pageUid: "page-source" }, + { sharedNode: evidence, status: "imported", pageUid: "page-evidence" }, ]); expect(onProgress.mock.calls).toEqual([ [1, 2], @@ -361,7 +374,9 @@ describe("importSharedNodes", () => { expect(mockedFindImportedNodeUidBySourceRid).toHaveBeenCalledWith( source.rid, ); - expect(items).toEqual([{ sharedNode: evidence, status: "imported" }]); + expect(items).toEqual([ + { sharedNode: evidence, status: "imported", pageUid: "page-evidence" }, + ]); }); it("does not add a source that is selected", async () => { @@ -403,7 +418,9 @@ describe("importSharedNodes", () => { expect(mockedFindImportedNodeUidBySourceRid).not.toHaveBeenCalled(); expect(mockedMaterializeSharedNode).toHaveBeenCalledTimes(1); - expect(items).toEqual([{ sharedNode: evidence, status: "imported" }]); + expect(items).toEqual([ + { sharedNode: evidence, status: "imported", pageUid: "page-evidence" }, + ]); }); it("adds a source once when several nodes name it by a bare id", async () => { diff --git a/apps/roam/src/utils/__tests__/sortSharedNodes.test.ts b/apps/roam/src/utils/__tests__/sortSharedNodes.test.ts new file mode 100644 index 0000000000..10e55de7bc --- /dev/null +++ b/apps/roam/src/utils/__tests__/sortSharedNodes.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from "vitest"; +import { + DEFAULT_SHARED_NODE_SORT, + getNextSharedNodeSort, + sortSharedNodes, +} from "~/utils/sortSharedNodes"; + +const older = { + spaceName: "beta vault", + title: "Claim 10", + lastModified: "2026-06-14T09:00:00.000Z", +}; +const newer = { + spaceName: "Alpha graph", + title: "Claim 9", + lastModified: "2026-06-14T15:00:00.000Z", +}; +const newest = { + spaceName: "Gamma vault", + title: "evidence", + lastModified: "2026-06-14T14:00:00.000-02:00", +}; + +describe("getNextSharedNodeSort", () => { + it("sorts a newly clicked column descending", () => { + expect( + getNextSharedNodeSort({ + currentSort: DEFAULT_SHARED_NODE_SORT, + column: "title", + }), + ).toEqual({ column: "title", direction: "descending" }); + }); + + it("toggles the active column between descending and ascending", () => { + const ascending = getNextSharedNodeSort({ + currentSort: DEFAULT_SHARED_NODE_SORT, + column: "lastModified", + }); + expect(ascending).toEqual({ + column: "lastModified", + direction: "ascending", + }); + expect( + getNextSharedNodeSort({ currentSort: ascending, column: "lastModified" }), + ).toEqual(DEFAULT_SHARED_NODE_SORT); + }); +}); + +describe("sortSharedNodes", () => { + it("defaults to most recently modified first, comparing instants", () => { + expect( + sortSharedNodes({ + nodes: [older, newest, newer], + sort: DEFAULT_SHARED_NODE_SORT, + }), + ).toEqual([newest, newer, older]); + }); + + it("sorts text columns ignoring case and comparing numbers by value", () => { + expect( + sortSharedNodes({ + nodes: [newest, older, newer], + sort: { column: "title", direction: "ascending" }, + }), + ).toEqual([newer, older, newest]); + expect( + sortSharedNodes({ + nodes: [newest, older, newer], + sort: { column: "spaceName", direction: "descending" }, + }), + ).toEqual([newest, older, newer]); + }); + + it("does not reorder the input", () => { + const nodes = [older, newest, newer]; + sortSharedNodes({ nodes, sort: DEFAULT_SHARED_NODE_SORT }); + expect(nodes).toEqual([older, newest, newer]); + }); +}); diff --git a/apps/roam/src/utils/importSharedNodes.ts b/apps/roam/src/utils/importSharedNodes.ts index f51db3c5ae..dbb8cb8cfb 100644 --- a/apps/roam/src/utils/importSharedNodes.ts +++ b/apps/roam/src/utils/importSharedNodes.ts @@ -16,7 +16,12 @@ export type FailedSharedNodeImport = { }; export type SharedNodeImportItem = - | { sharedNode: SharedNode; status: "imported" | "skipped"; warning?: string } + | { + sharedNode: SharedNode; + status: "imported" | "skipped"; + pageUid: string; + warning?: string; + } | FailedSharedNodeImport; export const isFailedSharedNodeImport = ( @@ -100,6 +105,7 @@ export const importSharedNodes = async ({ ? { sharedNode, status: result.action === "skipped" ? "skipped" : "imported", + pageUid: result.pageUid, ...(result.warning ? { warning: result.warning } : {}), } : { sharedNode, status: "failed", message: result.error.message }, diff --git a/apps/roam/src/utils/registerCommandPaletteCommands.ts b/apps/roam/src/utils/registerCommandPaletteCommands.ts index 17b215fb39..dd905aafc3 100644 --- a/apps/roam/src/utils/registerCommandPaletteCommands.ts +++ b/apps/roam/src/utils/registerCommandPaletteCommands.ts @@ -486,7 +486,7 @@ export const registerCommandPaletteCommands = (onloadArgs: OnloadArgs) => { void addCommand("DG: Export - Discourse graph", exportDiscourseGraph); void addCommand("DG: Open - Discourse settings", renderSettingsPopup); if (isSyncEnabled()) { - void addCommand("DG: Discover shared nodes", discoverSharedNodes); + void addCommand("DG: Import shared nodes", discoverSharedNodes); } if (isNodeSharingEnabled()) { void addCommand( diff --git a/apps/roam/src/utils/sortSharedNodes.ts b/apps/roam/src/utils/sortSharedNodes.ts new file mode 100644 index 0000000000..5c458f45d6 --- /dev/null +++ b/apps/roam/src/utils/sortSharedNodes.ts @@ -0,0 +1,54 @@ +import type { SharedNode } from "@repo/database/lib/sharedNodes"; + +export type SharedNodeSortColumn = "spaceName" | "title" | "lastModified"; + +export type SharedNodeSort = { + column: SharedNodeSortColumn; + direction: "ascending" | "descending"; +}; + +type SortableSharedNode = Pick; + +export const DEFAULT_SHARED_NODE_SORT: SharedNodeSort = { + column: "lastModified", + direction: "descending", +}; + +export const getNextSharedNodeSort = ({ + currentSort, + column, +}: { + currentSort: SharedNodeSort; + column: SharedNodeSortColumn; +}): SharedNodeSort => ({ + column, + direction: + currentSort.column === column && currentSort.direction === "descending" + ? "ascending" + : "descending", +}); + +const compareByColumn = + (column: SharedNodeSortColumn) => + (left: SortableSharedNode, right: SortableSharedNode): number => + column === "lastModified" + ? Date.parse(left.lastModified) - Date.parse(right.lastModified) + : left[column].localeCompare(right[column], undefined, { + numeric: true, + sensitivity: "base", + }); + +export const sortSharedNodes = ({ + nodes, + sort, +}: { + nodes: readonly T[]; + sort: SharedNodeSort; +}): T[] => { + const compare = compareByColumn(sort.column); + return [...nodes].sort((left, right) => + sort.direction === "ascending" + ? compare(left, right) + : compare(right, left), + ); +}; From b564f5c234af412b1b110d0361d12e9b16cccfa0 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 24 Sep 2026 22:56:48 +0530 Subject: [PATCH 15/17] ENG-2296 Show import results before importing relations Set the node results and failed-node selection before the relations import, so a relations failure no longer leaves hidden rows selected without a result list. Stop searching the source app and source ID, which the table no longer shows. --- .../src/components/DiscoverSharedNodesDialog.tsx | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx index 5bdbb8cc73..ccbf5ce3df 100644 --- a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx +++ b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx @@ -262,13 +262,9 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { const normalizedSearch = searchTerm.trim().toLocaleLowerCase(); if (!normalizedSearch) return availableNodes; return availableNodes.filter((node) => - [ - node.platform, - node.spaceName, - node.spaceUri, - node.title, - node.sourceLocalId, - ].some((value) => value.toLocaleLowerCase().includes(normalizedSearch)), + [node.spaceName, node.spaceUri, node.title].some((value) => + value.toLocaleLowerCase().includes(normalizedSearch), + ), ); }, [availableNodes, searchTerm]); @@ -321,7 +317,6 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { newlyImportedRids.forEach((rid) => next.add(rid)); return next; }); - await importSharedRelations(client, spaceId, [...importedRids]); setImportResults(results); const failedImports = results.filter(isFailedSharedNodeImport); setSelectedRids( @@ -340,6 +335,7 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { sendEmail: false, }); } + await importSharedRelations(client, spaceId, [...importedRids]); } catch (importError) { internalError({ error: importError, From 4c60e7dbc20d9bd1661e7ff30c31df5b86fc51e7 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 24 Sep 2026 23:07:12 +0530 Subject: [PATCH 16/17] ENG-2296 Explain relation import failures and scroll short dialogs Report a relations failure after a successful node import as its own message, so it no longer reads as a node import failure next to a success summary. Let the dialog body scroll when a short window can't fit the search, result, and table. --- .../components/DiscoverSharedNodesDialog.tsx | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx index ccbf5ce3df..5b73099d56 100644 --- a/apps/roam/src/components/DiscoverSharedNodesDialog.tsx +++ b/apps/roam/src/components/DiscoverSharedNodesDialog.tsx @@ -19,6 +19,7 @@ import createOverlayRender from "roamjs-components/util/createOverlayRender"; import openBlockInSidebar from "roamjs-components/writes/openBlockInSidebar"; import type { SharedNode } from "@repo/database/lib/sharedNodes"; import { discoverSharedNodes } from "~/utils/discoverSharedNodes"; +import { getErrorMessage } from "~/utils/getErrorMessage"; import { importSharedNodes, isFailedSharedNodeImport, @@ -335,7 +336,16 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { sendEmail: false, }); } - await importSharedRelations(client, spaceId, [...importedRids]); + await importSharedRelations(client, spaceId, [...importedRids]).catch( + (relationsError: unknown) => + internalError({ + error: relationsError, + type: IMPORT_ERROR_TYPE, + context: { operation: IMPORT_ERROR_OPERATION }, + sendEmail: false, + userMessage: `The nodes were imported, but their relations were not: ${getErrorMessage(relationsError)}`, + }), + ); } catch (importError) { internalError({ error: importError, @@ -368,9 +378,10 @@ const DiscoverSharedNodesDialog = ({ onClose }: { onClose: () => void }) => { title="Import shared nodes" >
Date: Thu, 24 Sep 2026 17:48:49 +0000 Subject: [PATCH 17/17] Derive Obsidian import file names from titles Obsidian can create Imported node titles arriving from Roam carry page references and tags ([[EVD]] - x - [[@Smith 2020]]), and the import path only stripped the OS file-name set, so the brackets reached the note's file name. Obsidian creates such a file but cannot link to it, so the node imports and then sits disconnected from the rest of the graph. Unwrap page references and drop the characters Obsidian rejects in file names (the set checkInvalidChars already enforces when a user creates a node), in a function of its own with tests. Asset paths keep the narrower rule. A title that sanitizes to nothing falls back to the node instance id. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01BmGT261gNCnNMmW8h6HXbn --- .../src/utils/__tests__/noteFileName.test.ts | 58 +++++++++++++++++++ apps/obsidian/src/utils/importNodes.ts | 5 +- apps/obsidian/src/utils/noteFileName.ts | 17 ++++++ 3 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 apps/obsidian/src/utils/__tests__/noteFileName.test.ts create mode 100644 apps/obsidian/src/utils/noteFileName.ts diff --git a/apps/obsidian/src/utils/__tests__/noteFileName.test.ts b/apps/obsidian/src/utils/__tests__/noteFileName.test.ts new file mode 100644 index 0000000000..9e7db80b5c --- /dev/null +++ b/apps/obsidian/src/utils/__tests__/noteFileName.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, it } from "vitest"; +import { noteFileNameFromTitle } from "~/utils/noteFileName"; + +describe("noteFileNameFromTitle", () => { + it("keeps a plain title", () => { + expect(noteFileNameFromTitle("CLM - sleep improves memory")).toBe( + "CLM - sleep improves memory", + ); + }); + + it("unwraps Roam page references in the title", () => { + expect( + noteFileNameFromTitle( + "[[EVD]] - REM sleep aids recall - [[@Smith 2020]]", + ), + ).toBe("EVD - REM sleep aids recall - @Smith 2020"); + }); + + it("drops the characters Obsidian rejects in file names", () => { + expect(noteFileNameFromTitle('ac:d"e/f\\g|h?i*j#k^l[m]n')).toBe( + "abcdefghijklmn", + ); + }); + + it("drops a stray bracket that is not a page reference", () => { + expect(noteFileNameFromTitle("[[unclosed - [tag] - x")).toBe( + "unclosed - tag - x", + ); + }); + + it("drops a slash rather than making a folder", () => { + expect(noteFileNameFromTitle("Projects/Alpha")).toBe("ProjectsAlpha"); + }); + + it("collapses whitespace left behind", () => { + expect(noteFileNameFromTitle(" #tag - x ")).toBe("tag - x"); + }); + + it("returns an empty string when nothing survives", () => { + expect(noteFileNameFromTitle("[[]]")).toBe(""); + expect(noteFileNameFromTitle("#")).toBe(""); + }); + + it("unwraps nested and adjacent references", () => { + expect(noteFileNameFromTitle("[[a [[b]] c]]")).toBe("a b c"); + expect(noteFileNameFromTitle("[[EVD]][[x]]")).toBe("EVDx"); + }); + + it("does not treat a pipe inside a reference as an alias", () => { + expect(noteFileNameFromTitle("[[Page|alias]]")).toBe("Pagealias"); + }); + + it("passes non-ASCII titles through", () => { + expect( + noteFileNameFromTitle("[[EVD]] - Schlaf verbessert Gedächtnis"), + ).toBe("EVD - Schlaf verbessert Gedächtnis"); + }); +}); diff --git a/apps/obsidian/src/utils/importNodes.ts b/apps/obsidian/src/utils/importNodes.ts index 11225dd16e..c0d472f70c 100644 --- a/apps/obsidian/src/utils/importNodes.ts +++ b/apps/obsidian/src/utils/importNodes.ts @@ -37,6 +37,7 @@ import { } from "./importedNodeContent"; import { decorateTitle } from "@repo/database/lib/decorateTitle"; import { buildSchemaRid, findLocalNodeTypeMatch } from "./schemaMatching"; +import { noteFileNameFromTitle } from "./noteFileName"; type PublishedNode = { source_local_id: string; @@ -1725,7 +1726,9 @@ const importNodes = async ({ coreTitle !== undefined && localNodeType ? decorateTitle(localNodeType.format, coreTitle) : null; - const sanitizedFileName = sanitizeFileName(decoratedTitle ?? fileName); + const sanitizedFileName = + noteFileNameFromTitle(decoratedTitle ?? fileName) || + node.nodeInstanceId; let finalFilePath: string; if (existingFile) { diff --git a/apps/obsidian/src/utils/noteFileName.ts b/apps/obsidian/src/utils/noteFileName.ts new file mode 100644 index 0000000000..817db29395 --- /dev/null +++ b/apps/obsidian/src/utils/noteFileName.ts @@ -0,0 +1,17 @@ +// Titles arrive from other apps in their own syntax: Roam node titles carry page +// references ([[EVD]] - x - [[@Smith 2020]]) and tags. Obsidian creates a file with +// those characters in its name but cannot link to it, so references are unwrapped +// to keep the page name and the rest of the link characters (#^[]|, the set +// checkInvalidChars enforces on node type formats) are dropped with the OS set. +// A slash is dropped rather than made a folder: a Roam namespace and an Obsidian +// folder are not the same thing. Returns "" when nothing survives; the caller +// picks the fallback name. +const PAGE_REFERENCE = /\[\[([^\]]*)\]\]/g; +const REJECTED_IN_FILE_NAMES = /[<>:"/\\|?*#^[\]]/g; + +export const noteFileNameFromTitle = (title: string): string => + title + .replace(PAGE_REFERENCE, "$1") + .replace(REJECTED_IN_FILE_NAMES, "") + .replace(/\s+/g, " ") + .trim();