From eec212b386ec9e6fb141e78d05decbeb528730bb Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 31 Aug 2026 17:12:31 +0530 Subject: [PATCH 1/6] ENG-1869 Review and accept imported relation instances in Roam --- apps/roam/src/components/DiscourseContext.tsx | 121 ++++++------ .../components/TentativeRelationInstances.tsx | 185 ++++++++++++++++++ .../getDiscourseContextResults.test.ts | 68 +++++++ .../__tests__/tentativeRelations.test.ts | 163 +++++++++++++++ apps/roam/src/utils/createReifiedBlock.ts | 23 ++- .../src/utils/getDiscourseContextResults.ts | 14 ++ apps/roam/src/utils/importedSourceIdentity.ts | 9 +- apps/roam/src/utils/tentativeRelations.ts | 54 +++++ 8 files changed, 574 insertions(+), 63 deletions(-) create mode 100644 apps/roam/src/components/TentativeRelationInstances.tsx create mode 100644 apps/roam/src/utils/__tests__/tentativeRelations.test.ts create mode 100644 apps/roam/src/utils/tentativeRelations.ts diff --git a/apps/roam/src/components/DiscourseContext.tsx b/apps/roam/src/components/DiscourseContext.tsx index 92fd5cdba..ad41ef58a 100644 --- a/apps/roam/src/components/DiscourseContext.tsx +++ b/apps/roam/src/components/DiscourseContext.tsx @@ -5,6 +5,7 @@ import getDiscourseContextResults from "~/utils/getDiscourseContextResults"; import ResultsView from "./results-view/ResultsView"; import posthog from "posthog-js"; import { CreateRelationButton } from "./CreateRelationDialog"; +import TentativeRelationInstances from "./TentativeRelationInstances"; import { useDiscourseContextMutationRefresh } from "~/utils/discourseContextMutationRefresh"; export type DiscourseContextResults = Awaited< @@ -172,9 +173,11 @@ export const ContextContent = ({ uid, results, overlayRefresh }: Props) => { }); const [tabId, setTabId] = useState(0); const [groupByTarget, setGroupByTarget] = useState(false); - return queryResults.length ? ( + return ( <> - - setTabId(Number(e))} - vertical - renderActiveTabPanelOnly - > - {queryResults.map((r, i) => ( - setTabId(Number(e))} + vertical + renderActiveTabPanelOnly + > + {queryResults.map((r, i) => ( + + } /> + ))} + {debouncedLoading && ( +
+ +
+ )} +
+ + setGroupByTarget((e.target as HTMLInputElement).checked) + } + /> + +
+ + + ) : debouncedLoading && !results ? ( + {}} vertical> + +
+
} /> - ))} - {debouncedLoading && ( -
- -
- )} -
- - setGroupByTarget((e.target as HTMLInputElement).checked) - } - /> + + ) : ( +
+ No discourse relations found.
- + )} + - ) : debouncedLoading && !results ? ( - {}} vertical> - -
-
- } - /> - - ) : ( -
- No discourse relations found. - -
); }; diff --git a/apps/roam/src/components/TentativeRelationInstances.tsx b/apps/roam/src/components/TentativeRelationInstances.tsx new file mode 100644 index 000000000..2d8e645c7 --- /dev/null +++ b/apps/roam/src/components/TentativeRelationInstances.tsx @@ -0,0 +1,185 @@ +import React, { useCallback, useEffect, useState } from "react"; +import { Button, Classes, Tag } from "@blueprintjs/core"; +import { render as renderToast } from "roamjs-components/components/Toast"; +import deleteBlock from "roamjs-components/writes/deleteBlock"; +import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; +import { ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; +import getDiscourseRelations from "~/utils/getDiscourseRelations"; +import { + refreshDiscourseContextsForMutatedUids, + useDiscourseContextMutationRefresh, +} from "~/utils/discourseContextMutationRefresh"; +import { + acceptTentativeRelationInstance, + getTentativeRelationInstances, + type TentativeRelationInstance, +} from "~/utils/tentativeRelations"; +import type { ImportedSourceIdentity } from "~/utils/importedSourceIdentity"; + +type TentativeRelationRow = TentativeRelationInstance & { + label: string; + otherText: string; + provenance: string; +}; + +const buildProvenance = (importedFrom?: ImportedSourceIdentity): string => { + if (!importedFrom) return ""; + const { spaceUri, sourceLocalId } = ridToSpaceUriAndLocalId( + importedFrom.sourceNodeRid, + ); + const sourceApp = spaceUri.startsWith("http") + ? undefined + : spaceUri.split(":")[0]; + const modifiedAt = new Date(importedFrom.sourceModifiedAt); + const modified = Number.isNaN(modifiedAt.getTime()) + ? undefined + : modifiedAt.toLocaleString(); + return [sourceApp, spaceUri, sourceLocalId, modified] + .filter(Boolean) + .join(" · "); +}; + +const toErrorMessage = (error: unknown): string => + error instanceof Error ? error.message : String(error); + +const TentativeRelationInstances = ({ + uid, +}: { + uid: string; +}): React.JSX.Element | null => { + const [rows, setRows] = useState([]); + const [pending, setPending] = useState<{ + uid: string; + action: "accept" | "remove"; + } | null>(null); + + const loadRows = useCallback(async () => { + const instances = await getTentativeRelationInstances(); + const relevant = instances.filter( + (instance) => + instance.sourceUid === uid || instance.destinationUid === uid, + ); + const relationById = new Map(getDiscourseRelations().map((r) => [r.id, r])); + setRows( + relevant.map((instance) => { + const isOutgoing = instance.sourceUid === uid; + const otherUid = isOutgoing + ? instance.destinationUid + : instance.sourceUid; + const schema = relationById.get(instance.schemaUid); + const label = + (isOutgoing ? schema?.label : schema?.complement) || + schema?.label || + "Unknown relation"; + return { + ...instance, + label, + otherText: getPageTitleByPageUid(otherUid) || otherUid, + provenance: buildProvenance(instance.importedFrom), + }; + }), + ); + }, [uid]); + + useEffect(() => { + void loadRows(); + }, [loadRows]); + + const onMutationRefresh = useCallback(() => void loadRows(), [loadRows]); + useDiscourseContextMutationRefresh({ uid, onMutationRefresh }); + + const onAccept = async (row: TentativeRelationRow): Promise => { + setPending({ uid: row.relationUid, action: "accept" }); + try { + await acceptTentativeRelationInstance({ relationUid: row.relationUid }); + renderToast({ + id: "accept-relation-success", + content: "Relation accepted", + intent: "success", + }); + refreshDiscourseContextsForMutatedUids({ + uids: [row.sourceUid, row.destinationUid], + }); + } catch (error) { + renderToast({ + id: "accept-relation-error", + content: `Could not accept relation: ${toErrorMessage(error)}`, + intent: "danger", + }); + } finally { + setPending(null); + } + }; + + const onRemove = async (row: TentativeRelationRow): Promise => { + setPending({ uid: row.relationUid, action: "remove" }); + try { + await deleteBlock(row.relationUid); + renderToast({ + id: "remove-relation-success", + content: "Relation removed", + intent: "success", + }); + refreshDiscourseContextsForMutatedUids({ + uids: [row.sourceUid, row.destinationUid], + }); + } catch (error) { + renderToast({ + id: "remove-relation-error", + content: `Could not remove relation: ${toErrorMessage(error)}`, + intent: "danger", + }); + } finally { + setPending(null); + } + }; + + if (!rows.length) return null; + + return ( +
+
+ Imported relations pending review ({rows.length}) +
+ {rows.map((row) => ( +
+
+
+ {row.label} {row.otherText} +
+ {row.provenance && ( +
+ {row.provenance} +
+ )} +
+
+ ))} +
+ ); +}; + +export default TentativeRelationInstances; diff --git a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts index f95e761ce..f32cc5844 100644 --- a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts +++ b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts @@ -7,6 +7,7 @@ const mocks = vi.hoisted(() => ({ fireQuery: vi.fn(), generateUID: vi.fn(), getSetting: vi.fn(), + getTentativeRelationInstances: vi.fn(), })); vi.mock("~/utils/deriveDiscourseNodeAttribute", () => ({ @@ -34,6 +35,10 @@ vi.mock("~/utils/getDiscourseRelations", () => ({ default: () => [], })); +vi.mock("~/utils/tentativeRelations", () => ({ + getTentativeRelationInstances: mocks.getTentativeRelationInstances, +})); + import getDiscourseContextResults from "~/utils/getDiscourseContextResults"; const makeNode = ({ @@ -66,6 +71,7 @@ describe("getDiscourseContextResults", () => { mocks.generateUID.mockReturnValue("condition"); mocks.getSetting.mockReturnValue(true); mocks.findDiscourseNode.mockReturnValue({ type: "CLM" }); + mocks.getTentativeRelationInstances.mockResolvedValue([]); }); it("regroups all-relation reified query results by schema order", async () => { @@ -160,4 +166,66 @@ describe("getDiscourseContextResults", () => { expect(onResult).toHaveBeenNthCalledWith(1, results[0]); expect(onResult).toHaveBeenNthCalledWith(2, results[1]); }); + + it("excludes tentative imported relation instances from reified results", async () => { + const onResult = vi.fn(); + const nodes: DiscourseNode[] = [ + makeNode({ type: "CLM", text: "Claim" }), + makeNode({ type: "QUE", text: "Question" }), + makeNode({ type: "EVD", text: "Evidence" }), + ]; + const relations: DiscourseRelation[] = [ + { + id: "supports", + label: "Supports", + complement: "Supported By", + source: "CLM", + destination: "QUE", + triples: [], + }, + { + id: "informs", + label: "Informs", + complement: "Informed By", + source: "EVD", + destination: "CLM", + triples: [], + }, + ]; + + mocks.fireQuery.mockResolvedValue([ + { + text: "Evidence A", + uid: "evidence-a", + relationUid: "informs", + effectiveSource: "evidence-a", + }, + { + text: "Question A", + uid: "question-a", + relationUid: "supports", + effectiveSource: "claim-a", + }, + ]); + mocks.getTentativeRelationInstances.mockResolvedValue([ + { + relationUid: "rel-block-1", + schemaUid: "supports", + sourceUid: "claim-a", + destinationUid: "question-a", + }, + ]); + + const results = await getDiscourseContextResults({ + uid: "claim-a", + nodes, + relations, + onResult, + }); + + expect(results).toHaveLength(1); + expect(results[0].label).toBe("Informed By"); + expect(Object.keys(results[0].results)).toEqual(["evidence-a"]); + expect(onResult).toHaveBeenCalledTimes(1); + }); }); diff --git a/apps/roam/src/utils/__tests__/tentativeRelations.test.ts b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts new file mode 100644 index 000000000..17bebf10b --- /dev/null +++ b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts @@ -0,0 +1,163 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { + DISCOURSE_GRAPH_PROP_NAME, + strictQueryForReifiedBlocks, +} from "~/utils/createReifiedBlock"; +import { + acceptTentativeRelationInstance, + getTentativeRelationInstances, +} from "~/utils/tentativeRelations"; +import type { json } from "~/utils/getBlockProps"; + +vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ + default: () => "relations-page", +})); + +const RELATION_UID = "rel-block-1"; +const SOURCE_NODE_RID = "orn:obsidian.note:vault-a/relation-1"; +const SOURCE_MODIFIED_AT = "2026-08-21T15:00:00.000Z"; + +const propsByUid = new Map>(); +const query = vi.fn(); +const update = vi.fn( + ({ block }: { block: { props: Record; uid: string } }) => { + propsByUid.set(block.uid, block.props); + return Promise.resolve(); + }, +); + +const setRoamAlphaApi = (): void => { + (globalThis as { window: unknown }).window = { + roamAlphaAPI: { + data: { + async: { q: query }, + block: { update }, + }, + pull: (_pattern: string, [, uid]: [string, string]) => ({ + ":block/props": propsByUid.get(uid) ?? {}, + }), + }, + }; +}; + +const tentativeRelationProps = (): Record => ({ + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + tentative: "true", + importedFrom: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, +}); + +beforeEach(() => { + propsByUid.clear(); + query.mockReset(); + update.mockClear(); + setRoamAlphaApi(); +}); + +describe("getTentativeRelationInstances", () => { + it("returns only tentative relations with their source identity", async () => { + const tentativeProps = tentativeRelationProps(); + propsByUid.set(RELATION_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: tentativeProps, + }); + query.mockResolvedValue([ + [RELATION_UID, tentativeProps], + [ + "rel-block-2", + { + sourceUid: "evidence-a", + destinationUid: "claim-a", + hasSchema: "informs", + }, + ], + ]); + + expect(await getTentativeRelationInstances()).toEqual([ + { + relationUid: RELATION_UID, + schemaUid: "supports", + sourceUid: "claim-a", + destinationUid: "question-a", + importedFrom: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, + }, + ]); + }); +}); + +describe("acceptTentativeRelationInstance", () => { + it("removes the tentative flag while preserving identity and provenance", async () => { + propsByUid.set(RELATION_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: tentativeRelationProps(), + }); + + await acceptTentativeRelationInstance({ relationUid: RELATION_UID }); + + expect(propsByUid.get(RELATION_UID)).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: { + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + importedFrom: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, + }, + }); + }); + + it("is a no-op for a relation that is already accepted", async () => { + propsByUid.set(RELATION_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: { + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + }, + }); + + await acceptTentativeRelationInstance({ relationUid: RELATION_UID }); + + expect(update).not.toHaveBeenCalled(); + }); + + it("throws when the relation block cannot be read", async () => { + await expect( + acceptTentativeRelationInstance({ relationUid: "missing" }), + ).rejects.toThrow(/could not be read/); + expect(update).not.toHaveBeenCalled(); + }); +}); + +describe("strictQueryForReifiedBlocks", () => { + it("matches imported relation blocks despite annotation keys", async () => { + query.mockResolvedValue([[RELATION_UID, tentativeRelationProps()]]); + + expect( + await strictQueryForReifiedBlocks({ + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + }), + ).toBe(RELATION_UID); + }); + + it("still rejects blocks with extra role keys", async () => { + query.mockResolvedValue([ + [RELATION_UID, { ...tentativeRelationProps(), contextUid: "context-a" }], + ]); + + expect( + await strictQueryForReifiedBlocks({ + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + }), + ).toBeNull(); + }); +}); diff --git a/apps/roam/src/utils/createReifiedBlock.ts b/apps/roam/src/utils/createReifiedBlock.ts index c4d044fae..a7c76fbd9 100644 --- a/apps/roam/src/utils/createReifiedBlock.ts +++ b/apps/roam/src/utils/createReifiedBlock.ts @@ -3,8 +3,20 @@ import createPage from "roamjs-components/writes/createPage"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; export const DISCOURSE_GRAPH_PROP_NAME = "discourse-graph"; +export const TENTATIVE_PROP_KEY = "tentative"; +export const IMPORTED_FROM_PROP_KEY = "importedFrom"; -const SANE_ROLE_NAME_RE = new RegExp(/^[\w\-]*$/); +// Annotations describe a relation's review/provenance state; they are not part +// of its identity, so lookups by role parameters must ignore them. +const RELATION_ANNOTATION_KEYS = new Set([ + TENTATIVE_PROP_KEY, + IMPORTED_FROM_PROP_KEY, +]); + +const countRoleKeys = (params: Record): number => + Object.keys(params).filter((k) => !RELATION_ANNOTATION_KEYS.has(k)).length; + +const SANE_ROLE_NAME_RE = new RegExp(/^[\w-]*$/); export const strictQueryForReifiedBlocks = async ( parameterUids: Record, @@ -26,9 +38,9 @@ export const strictQueryForReifiedBlocks = async ( ...paramsAsSeq.map(([, v]) => v), )) as [string, Record][]; // post-filtering because cannot filter by number of keys in datascript - const numParams = Object.keys(parameterUids).length; + const numParams = countRoleKeys(parameterUids); const resultF = result - .filter(([, params]) => Object.keys(params).length === numParams) + .filter(([, params]) => countRoleKeys(params) === numParams) .map(([uid]) => uid); if (resultF.length > 1) { const paramsAsText = Object.entries(parameterUids) @@ -106,6 +118,7 @@ export type ReifiedRelationData = { sourceUid: string; destinationUid: string; hasSchema: string; + tentative?: string; importedFromRid?: string; }; @@ -146,7 +159,9 @@ export const createReifiedRelation = async ({ const parameterUids: Record = { sourceUid, destinationUid, - ...(tentative !== undefined && { tentative: String(tentative) }), + ...(tentative !== undefined && { + [TENTATIVE_PROP_KEY]: String(tentative), + }), }; return await createReifiedBlock({ destinationBlockUid: await getOrCreateRelationPageUid(), diff --git a/apps/roam/src/utils/getDiscourseContextResults.ts b/apps/roam/src/utils/getDiscourseContextResults.ts index 8962dd5cf..a69719bba 100644 --- a/apps/roam/src/utils/getDiscourseContextResults.ts +++ b/apps/roam/src/utils/getDiscourseContextResults.ts @@ -12,6 +12,7 @@ import { ANY_RELATION_NAME, ANY_RELATION_REGEX, } from "./deriveDiscourseNodeAttribute"; +import { getTentativeRelationInstances } from "./tentativeRelations"; const resultCache: Record>> = {}; const CACHE_TIMEOUT = 1000 * 60 * 5; @@ -278,10 +279,23 @@ const getDiscourseContextResults = async ({ resultsWithRelation.length > 0 && resultsWithRelation[0].results.length > 0 ) { + const tentativeKeys = new Set( + (await getTentativeRelationInstances()).map( + (t) => `${t.schemaUid}|${t.sourceUid}|${t.destinationUid}`, + ), + ); + const isTentativeResult = (r: Result): boolean => { + const source = r.effectiveSource as string; + const destination = source === targetUid ? r.uid : targetUid; + return tentativeKeys.has( + `${r.relationUid as string}|${source}|${destination}`, + ); + }; const byRel: Record = {}; const results = resultsWithRelation[0].results; resultsWithRelation = []; for (const r of results) { + if (isTentativeResult(r)) continue; const relKey = `${r.relationUid as string}-${r.effectiveSource !== targetUid}`; byRel[relKey] = byRel[relKey] || []; byRel[relKey].push(r); diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index 64588918a..ff7ef311d 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -1,5 +1,8 @@ import type { Rid } from "@repo/database/crossAppContracts"; -import { DISCOURSE_GRAPH_PROP_NAME } from "./createReifiedBlock"; +import { + DISCOURSE_GRAPH_PROP_NAME, + IMPORTED_FROM_PROP_KEY, +} from "./createReifiedBlock"; import getBlockProps, { type json } from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; @@ -8,11 +11,11 @@ export type ImportedSourceIdentity = { sourceNodeRid: Rid; }; -export const IMPORTED_FROM_PROP_KEY = "importedFrom"; +export { IMPORTED_FROM_PROP_KEY }; const SOURCE_NODE_RID_KEY = "sourceNodeRid"; const SOURCE_MODIFIED_AT_KEY = "sourceModifiedAt"; -const isJsonObject = (value: json): value is Record => +export const isJsonObject = (value: json): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); const parseImportedSourceIdentity = ( diff --git a/apps/roam/src/utils/tentativeRelations.ts b/apps/roam/src/utils/tentativeRelations.ts new file mode 100644 index 000000000..ff8c05917 --- /dev/null +++ b/apps/roam/src/utils/tentativeRelations.ts @@ -0,0 +1,54 @@ +import getBlockProps from "./getBlockProps"; +import { setBlockPropsAsync } from "./setBlockProps"; +import { + DISCOURSE_GRAPH_PROP_NAME, + TENTATIVE_PROP_KEY, + getReifiedRelations, +} from "./createReifiedBlock"; +import { + isJsonObject, + readImportedSourceIdentity, + type ImportedSourceIdentity, +} from "./importedSourceIdentity"; + +export type TentativeRelationInstance = { + relationUid: string; + schemaUid: string; + sourceUid: string; + destinationUid: string; + importedFrom?: ImportedSourceIdentity; +}; + +export const getTentativeRelationInstances = async (): Promise< + TentativeRelationInstance[] +> => { + const relations = await getReifiedRelations(); + return relations + .filter((r) => r.tentative === "true") + .map((r) => ({ + relationUid: r.relationId, + schemaUid: r.hasSchema, + sourceUid: r.sourceUid, + destinationUid: r.destinationUid, + importedFrom: readImportedSourceIdentity(r.relationId), + })); +}; + +export const acceptTentativeRelationInstance = async ({ + relationUid, +}: { + relationUid: string; +}): Promise => { + const existing = getBlockProps(relationUid)[DISCOURSE_GRAPH_PROP_NAME]; + if (!isJsonObject(existing) || typeof existing.sourceUid !== "string") { + throw new Error( + "The relation block could not be read. It may have been deleted; refresh and try again.", + ); + } + if (existing[TENTATIVE_PROP_KEY] === undefined) return; + const accepted = { ...existing }; + delete accepted[TENTATIVE_PROP_KEY]; + await setBlockPropsAsync(relationUid, { + [DISCOURSE_GRAPH_PROP_NAME]: accepted, + }); +}; From c8112923ed06364d060053659e2f23a194a04a59 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 3 Sep 2026 00:09:33 +0530 Subject: [PATCH 2/6] ENG-1869 Address pre-PR review findings on tentative relation acceptance --- apps/roam/src/components/DiscourseContext.tsx | 127 +++++++++--------- .../components/TentativeRelationInstances.tsx | 107 ++++++++------- .../getDiscourseContextResults.test.ts | 61 ++++++++- .../__tests__/importedSourceIdentity.test.ts | 6 +- .../__tests__/tentativeRelations.test.ts | 78 +++++++---- apps/roam/src/utils/createReifiedBlock.ts | 32 ++++- apps/roam/src/utils/getBlockProps.ts | 3 + apps/roam/src/utils/importedSourceIdentity.ts | 26 ++-- apps/roam/src/utils/tentativeRelations.ts | 38 +----- 9 files changed, 296 insertions(+), 182 deletions(-) diff --git a/apps/roam/src/components/DiscourseContext.tsx b/apps/roam/src/components/DiscourseContext.tsx index ad41ef58a..37026355b 100644 --- a/apps/roam/src/components/DiscourseContext.tsx +++ b/apps/roam/src/components/DiscourseContext.tsx @@ -173,11 +173,10 @@ export const ContextContent = ({ uid, results, overlayRefresh }: Props) => { }); const [tabId, setTabId] = useState(0); const [groupByTarget, setGroupByTarget] = useState(false); - return ( + const [tentativeCount, setTentativeCount] = useState(0); + const body = queryResults.length ? ( <> - {queryResults.length ? ( - <> - - setTabId(Number(e))} - vertical - renderActiveTabPanelOnly - > - {queryResults.map((r, i) => ( - - } - /> - ))} - {debouncedLoading && ( -
- -
- )} -
- - setGroupByTarget((e.target as HTMLInputElement).checked) - } - /> - -
- - - ) : debouncedLoading && !results ? ( - {}} vertical> + setTabId(Number(e))} + vertical + renderActiveTabPanelOnly + > + {queryResults.map((r, i) => ( -
-
+ + } + /> + ))} + {debouncedLoading && ( +
+ +
+ )} +
+ + setGroupByTarget((e.target as HTMLInputElement).checked) } /> - - ) : ( -
- No discourse relations found.
- )} - + + + ) : debouncedLoading && !results ? ( + {}} vertical> + +
+
+ } + /> + + ) : tentativeCount ? null : ( +
+ No discourse relations found. + +
+ ); + return ( + <> + {body} + ); }; diff --git a/apps/roam/src/components/TentativeRelationInstances.tsx b/apps/roam/src/components/TentativeRelationInstances.tsx index 2d8e645c7..b7083d58d 100644 --- a/apps/roam/src/components/TentativeRelationInstances.tsx +++ b/apps/roam/src/components/TentativeRelationInstances.tsx @@ -1,10 +1,14 @@ -import React, { useCallback, useEffect, useState } from "react"; +import React, { useCallback, useEffect, useMemo, useState } from "react"; import { Button, Classes, Tag } from "@blueprintjs/core"; import { render as renderToast } from "roamjs-components/components/Toast"; import deleteBlock from "roamjs-components/writes/deleteBlock"; import getPageTitleByPageUid from "roamjs-components/queries/getPageTitleByPageUid"; -import { ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; +import posthog from "posthog-js"; +import { isRid, ridToSpaceUriAndLocalId } from "@repo/database/lib/rid"; import getDiscourseRelations from "~/utils/getDiscourseRelations"; +import internalError from "~/utils/internalError"; +import { getErrorMessage } from "~/utils/materializeSharedNode"; +import { getStoredRelationsEnabled } from "~/utils/storedRelations"; import { refreshDiscourseContextsForMutatedUids, useDiscourseContextMutationRefresh, @@ -23,30 +27,25 @@ type TentativeRelationRow = TentativeRelationInstance & { }; const buildProvenance = (importedFrom?: ImportedSourceIdentity): string => { - if (!importedFrom) return ""; + if (!importedFrom || !isRid(importedFrom.sourceNodeRid)) return ""; const { spaceUri, sourceLocalId } = ridToSpaceUriAndLocalId( importedFrom.sourceNodeRid, ); - const sourceApp = spaceUri.startsWith("http") - ? undefined - : spaceUri.split(":")[0]; const modifiedAt = new Date(importedFrom.sourceModifiedAt); const modified = Number.isNaN(modifiedAt.getTime()) ? undefined : modifiedAt.toLocaleString(); - return [sourceApp, spaceUri, sourceLocalId, modified] - .filter(Boolean) - .join(" · "); + return `from ${[spaceUri, sourceLocalId, modified].filter(Boolean).join(" · ")}`; }; -const toErrorMessage = (error: unknown): string => - error instanceof Error ? error.message : String(error); - const TentativeRelationInstances = ({ uid, + onCountChange, }: { uid: string; + onCountChange?: (count: number) => void; }): React.JSX.Element | null => { + const storedRelationsEnabled = useMemo(() => getStoredRelationsEnabled(), []); const [rows, setRows] = useState([]); const [pending, setPending] = useState<{ uid: string; @@ -54,32 +53,32 @@ const TentativeRelationInstances = ({ } | null>(null); const loadRows = useCallback(async () => { + if (!storedRelationsEnabled) return; const instances = await getTentativeRelationInstances(); const relevant = instances.filter( (instance) => instance.sourceUid === uid || instance.destinationUid === uid, ); const relationById = new Map(getDiscourseRelations().map((r) => [r.id, r])); - setRows( - relevant.map((instance) => { - const isOutgoing = instance.sourceUid === uid; - const otherUid = isOutgoing - ? instance.destinationUid - : instance.sourceUid; - const schema = relationById.get(instance.schemaUid); - const label = - (isOutgoing ? schema?.label : schema?.complement) || - schema?.label || - "Unknown relation"; - return { - ...instance, - label, - otherText: getPageTitleByPageUid(otherUid) || otherUid, - provenance: buildProvenance(instance.importedFrom), - }; - }), - ); - }, [uid]); + const nextRows = relevant.map((instance) => { + const isOutgoing = instance.sourceUid === uid; + const otherUid = isOutgoing + ? instance.destinationUid + : instance.sourceUid; + const schema = relationById.get(instance.schemaUid); + const label = + (isOutgoing ? schema?.label : schema?.complement || schema?.label) || + "Unknown relation"; + return { + ...instance, + label, + otherText: getPageTitleByPageUid(otherUid) || otherUid, + provenance: buildProvenance(instance.importedFrom), + }; + }); + setRows(nextRows); + onCountChange?.(nextRows.length); + }, [uid, storedRelationsEnabled, onCountChange]); useEffect(() => { void loadRows(); @@ -89,9 +88,13 @@ const TentativeRelationInstances = ({ useDiscourseContextMutationRefresh({ uid, onMutationRefresh }); const onAccept = async (row: TentativeRelationRow): Promise => { - setPending({ uid: row.relationUid, action: "accept" }); + posthog.capture("Discourse Context: Accept Tentative Relation Triggered", { + instanceUid: row.instanceUid, + uid, + }); + setPending({ uid: row.instanceUid, action: "accept" }); try { - await acceptTentativeRelationInstance({ relationUid: row.relationUid }); + await acceptTentativeRelationInstance({ instanceUid: row.instanceUid }); renderToast({ id: "accept-relation-success", content: "Relation accepted", @@ -101,10 +104,12 @@ const TentativeRelationInstances = ({ uids: [row.sourceUid, row.destinationUid], }); } catch (error) { - renderToast({ - id: "accept-relation-error", - content: `Could not accept relation: ${toErrorMessage(error)}`, - intent: "danger", + internalError({ + error, + type: "Accept Tentative Relation Failed", + context: { instanceUid: row.instanceUid }, + userMessage: `Could not accept relation: ${getErrorMessage(error)}`, + sendEmail: false, }); } finally { setPending(null); @@ -112,9 +117,13 @@ const TentativeRelationInstances = ({ }; const onRemove = async (row: TentativeRelationRow): Promise => { - setPending({ uid: row.relationUid, action: "remove" }); + posthog.capture("Discourse Context: Remove Tentative Relation Triggered", { + instanceUid: row.instanceUid, + uid, + }); + setPending({ uid: row.instanceUid, action: "remove" }); try { - await deleteBlock(row.relationUid); + await deleteBlock(row.instanceUid); renderToast({ id: "remove-relation-success", content: "Relation removed", @@ -124,17 +133,19 @@ const TentativeRelationInstances = ({ uids: [row.sourceUid, row.destinationUid], }); } catch (error) { - renderToast({ - id: "remove-relation-error", - content: `Could not remove relation: ${toErrorMessage(error)}`, - intent: "danger", + internalError({ + error, + type: "Remove Tentative Relation Failed", + context: { instanceUid: row.instanceUid }, + userMessage: `Could not remove relation: ${getErrorMessage(error)}`, + sendEmail: false, }); } finally { setPending(null); } }; - if (!rows.length) return null; + if (!storedRelationsEnabled || !rows.length) return null; return (
@@ -142,7 +153,7 @@ const TentativeRelationInstances = ({ Imported relations pending review ({rows.length})
{rows.map((row) => ( -
+
{row.label} {row.otherText} @@ -162,7 +173,7 @@ const TentativeRelationInstances = ({ title="Accept relation" disabled={pending !== null} loading={ - pending?.uid === row.relationUid && pending.action === "accept" + pending?.uid === row.instanceUid && pending.action === "accept" } onClick={() => void onAccept(row)} /> @@ -172,7 +183,7 @@ const TentativeRelationInstances = ({ title="Remove relation" disabled={pending !== null} loading={ - pending?.uid === row.relationUid && pending.action === "remove" + pending?.uid === row.instanceUid && pending.action === "remove" } onClick={() => void onRemove(row)} /> diff --git a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts index f32cc5844..14c5a7de4 100644 --- a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts +++ b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts @@ -209,7 +209,7 @@ describe("getDiscourseContextResults", () => { ]); mocks.getTentativeRelationInstances.mockResolvedValue([ { - relationUid: "rel-block-1", + instanceUid: "rel-block-1", schemaUid: "supports", sourceUid: "claim-a", destinationUid: "question-a", @@ -228,4 +228,63 @@ describe("getDiscourseContextResults", () => { expect(Object.keys(results[0].results)).toEqual(["evidence-a"]); expect(onResult).toHaveBeenCalledTimes(1); }); + + it("excludes tentative instances where the active node is the destination", async () => { + const nodes: DiscourseNode[] = [ + makeNode({ type: "CLM", text: "Claim" }), + makeNode({ type: "QUE", text: "Question" }), + makeNode({ type: "EVD", text: "Evidence" }), + ]; + const relations: DiscourseRelation[] = [ + { + id: "supports", + label: "Supports", + complement: "Supported By", + source: "CLM", + destination: "QUE", + triples: [], + }, + { + id: "informs", + label: "Informs", + complement: "Informed By", + source: "EVD", + destination: "CLM", + triples: [], + }, + ]; + + mocks.fireQuery.mockResolvedValue([ + { + text: "Evidence A", + uid: "evidence-a", + relationUid: "informs", + effectiveSource: "evidence-a", + }, + { + text: "Question A", + uid: "question-a", + relationUid: "supports", + effectiveSource: "claim-a", + }, + ]); + mocks.getTentativeRelationInstances.mockResolvedValue([ + { + instanceUid: "rel-block-2", + schemaUid: "informs", + sourceUid: "evidence-a", + destinationUid: "claim-a", + }, + ]); + + const results = await getDiscourseContextResults({ + uid: "claim-a", + nodes, + relations, + }); + + expect(results).toHaveLength(1); + expect(results[0].label).toBe("Supports"); + expect(Object.keys(results[0].results)).toEqual(["question-a"]); + }); }); diff --git a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts index 602b37cb3..c07375b7a 100644 --- a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts +++ b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts @@ -1,9 +1,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import { DISCOURSE_GRAPH_PROP_NAME } from "~/utils/createReifiedBlock"; +import { + DISCOURSE_GRAPH_PROP_NAME, + IMPORTED_FROM_PROP_KEY, +} from "~/utils/createReifiedBlock"; import { findImportedNodeUidBySourceRid, getImportedSourceRids, - IMPORTED_FROM_PROP_KEY, readImportedSourceIdentity, writeImportedSourceIdentity, } from "~/utils/importedSourceIdentity"; diff --git a/apps/roam/src/utils/__tests__/tentativeRelations.test.ts b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts index 17bebf10b..8fb467588 100644 --- a/apps/roam/src/utils/__tests__/tentativeRelations.test.ts +++ b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts @@ -1,6 +1,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { DISCOURSE_GRAPH_PROP_NAME, + createReifiedRelation, strictQueryForReifiedBlocks, } from "~/utils/createReifiedBlock"; import { @@ -51,6 +52,16 @@ const tentativeRelationProps = (): Record => ({ }, }); +const acceptedRelationProps = (): Record => ({ + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + importedFrom: { + sourceModifiedAt: SOURCE_MODIFIED_AT, + sourceNodeRid: SOURCE_NODE_RID, + }, +}); + beforeEach(() => { propsByUid.clear(); query.mockReset(); @@ -60,12 +71,8 @@ beforeEach(() => { describe("getTentativeRelationInstances", () => { it("returns only tentative relations with their source identity", async () => { - const tentativeProps = tentativeRelationProps(); - propsByUid.set(RELATION_UID, { - [DISCOURSE_GRAPH_PROP_NAME]: tentativeProps, - }); query.mockResolvedValue([ - [RELATION_UID, tentativeProps], + [RELATION_UID, tentativeRelationProps()], [ "rel-block-2", { @@ -78,7 +85,7 @@ describe("getTentativeRelationInstances", () => { expect(await getTentativeRelationInstances()).toEqual([ { - relationUid: RELATION_UID, + instanceUid: RELATION_UID, schemaUid: "supports", sourceUid: "claim-a", destinationUid: "question-a", @@ -97,43 +104,68 @@ describe("acceptTentativeRelationInstance", () => { [DISCOURSE_GRAPH_PROP_NAME]: tentativeRelationProps(), }); - await acceptTentativeRelationInstance({ relationUid: RELATION_UID }); + await acceptTentativeRelationInstance({ instanceUid: RELATION_UID }); expect(propsByUid.get(RELATION_UID)).toEqual({ - [DISCOURSE_GRAPH_PROP_NAME]: { - sourceUid: "claim-a", - destinationUid: "question-a", - hasSchema: "supports", - importedFrom: { - sourceModifiedAt: SOURCE_MODIFIED_AT, - sourceNodeRid: SOURCE_NODE_RID, - }, - }, + [DISCOURSE_GRAPH_PROP_NAME]: acceptedRelationProps(), }); }); it("is a no-op for a relation that is already accepted", async () => { propsByUid.set(RELATION_UID, { - [DISCOURSE_GRAPH_PROP_NAME]: { - sourceUid: "claim-a", - destinationUid: "question-a", - hasSchema: "supports", - }, + [DISCOURSE_GRAPH_PROP_NAME]: acceptedRelationProps(), }); - await acceptTentativeRelationInstance({ relationUid: RELATION_UID }); + await acceptTentativeRelationInstance({ instanceUid: RELATION_UID }); expect(update).not.toHaveBeenCalled(); }); it("throws when the relation block cannot be read", async () => { await expect( - acceptTentativeRelationInstance({ relationUid: "missing" }), + acceptTentativeRelationInstance({ instanceUid: "missing" }), ).rejects.toThrow(/could not be read/); expect(update).not.toHaveBeenCalled(); }); }); +describe("createReifiedRelation", () => { + it("promotes a matching tentative import instead of returning it hidden", async () => { + query.mockResolvedValue([[RELATION_UID, tentativeRelationProps()]]); + propsByUid.set(RELATION_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: tentativeRelationProps(), + }); + + const uid = await createReifiedRelation({ + sourceUid: "claim-a", + destinationUid: "question-a", + relationBlockUid: "supports", + }); + + expect(uid).toBe(RELATION_UID); + expect(propsByUid.get(RELATION_UID)).toEqual({ + [DISCOURSE_GRAPH_PROP_NAME]: acceptedRelationProps(), + }); + }); + + it("leaves a matching tentative import untouched during re-import", async () => { + query.mockResolvedValue([[RELATION_UID, tentativeRelationProps()]]); + propsByUid.set(RELATION_UID, { + [DISCOURSE_GRAPH_PROP_NAME]: tentativeRelationProps(), + }); + + const uid = await createReifiedRelation({ + sourceUid: "claim-a", + destinationUid: "question-a", + relationBlockUid: "supports", + tentative: true, + }); + + expect(uid).toBe(RELATION_UID); + expect(update).not.toHaveBeenCalled(); + }); +}); + describe("strictQueryForReifiedBlocks", () => { it("matches imported relation blocks despite annotation keys", async () => { query.mockResolvedValue([[RELATION_UID, tentativeRelationProps()]]); diff --git a/apps/roam/src/utils/createReifiedBlock.ts b/apps/roam/src/utils/createReifiedBlock.ts index a7c76fbd9..c191dc44c 100644 --- a/apps/roam/src/utils/createReifiedBlock.ts +++ b/apps/roam/src/utils/createReifiedBlock.ts @@ -1,6 +1,8 @@ import createBlock from "roamjs-components/writes/createBlock"; import createPage from "roamjs-components/writes/createPage"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; +import getBlockProps, { isJsonObject, type json } from "./getBlockProps"; +import { setBlockPropsAsync } from "./setBlockProps"; export const DISCOURSE_GRAPH_PROP_NAME = "discourse-graph"; export const TENTATIVE_PROP_KEY = "tentative"; @@ -53,6 +55,28 @@ export const strictQueryForReifiedBlocks = async ( return resultF.length > 0 ? resultF[0] : null; }; +// Deliberate local creation of a relation counts as user acceptance, so a +// dedupe hit on a tentative imported block promotes it instead of silently +// returning a block the UI hides as pending review. +export const acceptTentativeRelationInstance = async ({ + instanceUid, +}: { + instanceUid: string; +}): Promise => { + const existing = getBlockProps(instanceUid)[DISCOURSE_GRAPH_PROP_NAME]; + if (!isJsonObject(existing) || typeof existing.sourceUid !== "string") { + throw new Error( + "The relation block could not be read. It may have been deleted; refresh and try again.", + ); + } + if (existing[TENTATIVE_PROP_KEY] === undefined) return; + const accepted = { ...existing }; + delete accepted[TENTATIVE_PROP_KEY]; + await setBlockPropsAsync(instanceUid, { + [DISCOURSE_GRAPH_PROP_NAME]: accepted, + }); +}; + const createReifiedBlock = async ({ destinationBlockUid, schemaUid, @@ -68,7 +92,12 @@ const createReifiedBlock = async ({ hasSchema: schemaUid, }; const existing = await strictQueryForReifiedBlocks(data); - if (existing !== null) return existing; + if (existing !== null) { + if (parameterUids[TENTATIVE_PROP_KEY] === undefined) { + await acceptTentativeRelationInstance({ instanceUid: existing }); + } + return existing; + } const newUid = window.roamAlphaAPI.util.generateUID(); await createBlock({ node: { @@ -119,6 +148,7 @@ export type ReifiedRelationData = { destinationUid: string; hasSchema: string; tentative?: string; + importedFrom?: json; importedFromRid?: string; }; diff --git a/apps/roam/src/utils/getBlockProps.ts b/apps/roam/src/utils/getBlockProps.ts index f8c839f1c..88d76e1a1 100644 --- a/apps/roam/src/utils/getBlockProps.ts +++ b/apps/roam/src/utils/getBlockProps.ts @@ -6,6 +6,9 @@ export type json = | json[] | { [key: string]: json }; +export const isJsonObject = (value: json): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + export const normalizeProps = (props: json): json => typeof props === "object" ? props === null diff --git a/apps/roam/src/utils/importedSourceIdentity.ts b/apps/roam/src/utils/importedSourceIdentity.ts index ff7ef311d..93b48b562 100644 --- a/apps/roam/src/utils/importedSourceIdentity.ts +++ b/apps/roam/src/utils/importedSourceIdentity.ts @@ -3,7 +3,7 @@ import { DISCOURSE_GRAPH_PROP_NAME, IMPORTED_FROM_PROP_KEY, } from "./createReifiedBlock"; -import getBlockProps, { type json } from "./getBlockProps"; +import getBlockProps, { isJsonObject, type json } from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; export type ImportedSourceIdentity = { @@ -11,21 +11,14 @@ export type ImportedSourceIdentity = { sourceNodeRid: Rid; }; -export { IMPORTED_FROM_PROP_KEY }; const SOURCE_NODE_RID_KEY = "sourceNodeRid"; const SOURCE_MODIFIED_AT_KEY = "sourceModifiedAt"; -export const isJsonObject = (value: json): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); - -const parseImportedSourceIdentity = ( - props: Record, +export const parseSourceIdentity = ( + importedFrom: json | undefined, ): ImportedSourceIdentity | undefined => { - const discourseGraphProps = props[DISCOURSE_GRAPH_PROP_NAME]; - if (!isJsonObject(discourseGraphProps)) return undefined; - - const importedFrom = discourseGraphProps[IMPORTED_FROM_PROP_KEY]; - if (!isJsonObject(importedFrom)) return undefined; + if (importedFrom === undefined || !isJsonObject(importedFrom)) + return undefined; const sourceModifiedAt = importedFrom[SOURCE_MODIFIED_AT_KEY]; const sourceNodeRid = importedFrom[SOURCE_NODE_RID_KEY]; @@ -35,6 +28,15 @@ const parseImportedSourceIdentity = ( return { sourceModifiedAt, sourceNodeRid }; }; +const parseImportedSourceIdentity = ( + props: Record, +): ImportedSourceIdentity | undefined => { + const discourseGraphProps = props[DISCOURSE_GRAPH_PROP_NAME]; + if (!isJsonObject(discourseGraphProps)) return undefined; + + return parseSourceIdentity(discourseGraphProps[IMPORTED_FROM_PROP_KEY]); +}; + export const readImportedSourceIdentity = ( pageUid: string, ): ImportedSourceIdentity | undefined => diff --git a/apps/roam/src/utils/tentativeRelations.ts b/apps/roam/src/utils/tentativeRelations.ts index ff8c05917..1b7465364 100644 --- a/apps/roam/src/utils/tentativeRelations.ts +++ b/apps/roam/src/utils/tentativeRelations.ts @@ -1,18 +1,13 @@ -import getBlockProps from "./getBlockProps"; -import { setBlockPropsAsync } from "./setBlockProps"; +import { getReifiedRelations } from "./createReifiedBlock"; import { - DISCOURSE_GRAPH_PROP_NAME, - TENTATIVE_PROP_KEY, - getReifiedRelations, -} from "./createReifiedBlock"; -import { - isJsonObject, - readImportedSourceIdentity, + parseSourceIdentity, type ImportedSourceIdentity, } from "./importedSourceIdentity"; +export { acceptTentativeRelationInstance } from "./createReifiedBlock"; + export type TentativeRelationInstance = { - relationUid: string; + instanceUid: string; schemaUid: string; sourceUid: string; destinationUid: string; @@ -26,29 +21,10 @@ export const getTentativeRelationInstances = async (): Promise< return relations .filter((r) => r.tentative === "true") .map((r) => ({ - relationUid: r.relationId, + instanceUid: r.relationId, schemaUid: r.hasSchema, sourceUid: r.sourceUid, destinationUid: r.destinationUid, - importedFrom: readImportedSourceIdentity(r.relationId), + importedFrom: parseSourceIdentity(r.importedFrom), })); }; - -export const acceptTentativeRelationInstance = async ({ - relationUid, -}: { - relationUid: string; -}): Promise => { - const existing = getBlockProps(relationUid)[DISCOURSE_GRAPH_PROP_NAME]; - if (!isJsonObject(existing) || typeof existing.sourceUid !== "string") { - throw new Error( - "The relation block could not be read. It may have been deleted; refresh and try again.", - ); - } - if (existing[TENTATIVE_PROP_KEY] === undefined) return; - const accepted = { ...existing }; - delete accepted[TENTATIVE_PROP_KEY]; - await setBlockPropsAsync(relationUid, { - [DISCOURSE_GRAPH_PROP_NAME]: accepted, - }); -}; From 39031041e5af9bd4b9d07039f5d365d86f19fc36 Mon Sep 17 00:00:00 2001 From: sid597 Date: Thu, 3 Sep 2026 00:49:11 +0530 Subject: [PATCH 3/6] ENG-1869 Address second review pass findings --- apps/roam/src/components/DiscourseContext.tsx | 4 +-- .../components/TentativeRelationInstances.tsx | 15 ++++---- .../getDiscourseContextResults.test.ts | 28 +++++---------- .../utils/__tests__/importSharedNodes.test.ts | 2 ++ .../__tests__/importedSourceIdentity.test.ts | 2 ++ .../__tests__/tentativeRelations.test.ts | 34 ++++++++++++++++++- apps/roam/src/utils/createReifiedBlock.ts | 18 +++++++--- .../src/utils/getDiscourseContextResults.ts | 8 ++--- apps/roam/src/utils/tentativeRelations.ts | 22 ++++++++++-- 9 files changed, 90 insertions(+), 43 deletions(-) diff --git a/apps/roam/src/components/DiscourseContext.tsx b/apps/roam/src/components/DiscourseContext.tsx index 37026355b..9a2ec5013 100644 --- a/apps/roam/src/components/DiscourseContext.tsx +++ b/apps/roam/src/components/DiscourseContext.tsx @@ -249,9 +249,9 @@ export const ContextContent = ({ uid, results, overlayRefresh }: Props) => { } /> - ) : tentativeCount ? null : ( + ) : (
- No discourse relations found. + {!tentativeCount && No discourse relations found.}
); diff --git a/apps/roam/src/components/TentativeRelationInstances.tsx b/apps/roam/src/components/TentativeRelationInstances.tsx index b7083d58d..058dcb1e6 100644 --- a/apps/roam/src/components/TentativeRelationInstances.tsx +++ b/apps/roam/src/components/TentativeRelationInstances.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useEffect, useMemo, useState } from "react"; +import React, { useCallback, useEffect, useState } from "react"; import { Button, Classes, Tag } from "@blueprintjs/core"; import { render as renderToast } from "roamjs-components/components/Toast"; import deleteBlock from "roamjs-components/writes/deleteBlock"; @@ -13,8 +13,8 @@ import { refreshDiscourseContextsForMutatedUids, useDiscourseContextMutationRefresh, } from "~/utils/discourseContextMutationRefresh"; +import { acceptTentativeRelationInstance } from "~/utils/createReifiedBlock"; import { - acceptTentativeRelationInstance, getTentativeRelationInstances, type TentativeRelationInstance, } from "~/utils/tentativeRelations"; @@ -43,9 +43,8 @@ const TentativeRelationInstances = ({ onCountChange, }: { uid: string; - onCountChange?: (count: number) => void; + onCountChange: (count: number) => void; }): React.JSX.Element | null => { - const storedRelationsEnabled = useMemo(() => getStoredRelationsEnabled(), []); const [rows, setRows] = useState([]); const [pending, setPending] = useState<{ uid: string; @@ -53,7 +52,7 @@ const TentativeRelationInstances = ({ } | null>(null); const loadRows = useCallback(async () => { - if (!storedRelationsEnabled) return; + if (!getStoredRelationsEnabled()) return; const instances = await getTentativeRelationInstances(); const relevant = instances.filter( (instance) => @@ -77,8 +76,8 @@ const TentativeRelationInstances = ({ }; }); setRows(nextRows); - onCountChange?.(nextRows.length); - }, [uid, storedRelationsEnabled, onCountChange]); + onCountChange(nextRows.length); + }, [uid, onCountChange]); useEffect(() => { void loadRows(); @@ -145,7 +144,7 @@ const TentativeRelationInstances = ({ } }; - if (!storedRelationsEnabled || !rows.length) return null; + if (!rows.length) return null; return (
diff --git a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts index 14c5a7de4..ca291f0b7 100644 --- a/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts +++ b/apps/roam/src/utils/__tests__/getDiscourseContextResults.test.ts @@ -7,7 +7,7 @@ const mocks = vi.hoisted(() => ({ fireQuery: vi.fn(), generateUID: vi.fn(), getSetting: vi.fn(), - getTentativeRelationInstances: vi.fn(), + getTentativeOnlyRelationKeys: vi.fn(), })); vi.mock("~/utils/deriveDiscourseNodeAttribute", () => ({ @@ -36,7 +36,7 @@ vi.mock("~/utils/getDiscourseRelations", () => ({ })); vi.mock("~/utils/tentativeRelations", () => ({ - getTentativeRelationInstances: mocks.getTentativeRelationInstances, + getTentativeOnlyRelationKeys: mocks.getTentativeOnlyRelationKeys, })); import getDiscourseContextResults from "~/utils/getDiscourseContextResults"; @@ -71,7 +71,7 @@ describe("getDiscourseContextResults", () => { mocks.generateUID.mockReturnValue("condition"); mocks.getSetting.mockReturnValue(true); mocks.findDiscourseNode.mockReturnValue({ type: "CLM" }); - mocks.getTentativeRelationInstances.mockResolvedValue([]); + mocks.getTentativeOnlyRelationKeys.mockResolvedValue(new Set()); }); it("regroups all-relation reified query results by schema order", async () => { @@ -207,14 +207,9 @@ describe("getDiscourseContextResults", () => { effectiveSource: "claim-a", }, ]); - mocks.getTentativeRelationInstances.mockResolvedValue([ - { - instanceUid: "rel-block-1", - schemaUid: "supports", - sourceUid: "claim-a", - destinationUid: "question-a", - }, - ]); + mocks.getTentativeOnlyRelationKeys.mockResolvedValue( + new Set(["supports|claim-a|question-a"]), + ); const results = await getDiscourseContextResults({ uid: "claim-a", @@ -268,14 +263,9 @@ describe("getDiscourseContextResults", () => { effectiveSource: "claim-a", }, ]); - mocks.getTentativeRelationInstances.mockResolvedValue([ - { - instanceUid: "rel-block-2", - schemaUid: "informs", - sourceUid: "evidence-a", - destinationUid: "claim-a", - }, - ]); + mocks.getTentativeOnlyRelationKeys.mockResolvedValue( + new Set(["informs|evidence-a|claim-a"]), + ); const results = await getDiscourseContextResults({ uid: "claim-a", diff --git a/apps/roam/src/utils/__tests__/importSharedNodes.test.ts b/apps/roam/src/utils/__tests__/importSharedNodes.test.ts index d89d70400..8b466d7d0 100644 --- a/apps/roam/src/utils/__tests__/importSharedNodes.test.ts +++ b/apps/roam/src/utils/__tests__/importSharedNodes.test.ts @@ -14,6 +14,8 @@ vi.mock("~/utils/materializeSharedNode", async () => { return { ...actual, materializeSharedNode: vi.fn() }; }); +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + const mockedMaterializeSharedNode = vi.mocked(materializeSharedNode); const client = {} as DGSupabaseClient; diff --git a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts index c07375b7a..160f2ab33 100644 --- a/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts +++ b/apps/roam/src/utils/__tests__/importedSourceIdentity.test.ts @@ -11,6 +11,8 @@ import { } from "~/utils/importedSourceIdentity"; import type { json } from "~/utils/getBlockProps"; +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + const SOURCE_NODE_RID = "orn:obsidian.note:vault-a/node-1"; const SOURCE_MODIFIED_AT = "2026-06-14T15:00:00.000Z"; const PAGE_UID = "page-uid"; diff --git a/apps/roam/src/utils/__tests__/tentativeRelations.test.ts b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts index 8fb467588..88144c8d4 100644 --- a/apps/roam/src/utils/__tests__/tentativeRelations.test.ts +++ b/apps/roam/src/utils/__tests__/tentativeRelations.test.ts @@ -1,11 +1,12 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { DISCOURSE_GRAPH_PROP_NAME, + acceptTentativeRelationInstance, createReifiedRelation, strictQueryForReifiedBlocks, } from "~/utils/createReifiedBlock"; import { - acceptTentativeRelationInstance, + getTentativeOnlyRelationKeys, getTentativeRelationInstances, } from "~/utils/tentativeRelations"; import type { json } from "~/utils/getBlockProps"; @@ -14,6 +15,8 @@ vi.mock("roamjs-components/queries/getPageUidByPageTitle", () => ({ default: () => "relations-page", })); +vi.mock("~/utils/internalError", () => ({ default: vi.fn() })); + const RELATION_UID = "rel-block-1"; const SOURCE_NODE_RID = "orn:obsidian.note:vault-a/relation-1"; const SOURCE_MODIFIED_AT = "2026-08-21T15:00:00.000Z"; @@ -98,6 +101,35 @@ describe("getTentativeRelationInstances", () => { }); }); +describe("getTentativeOnlyRelationKeys", () => { + it("keeps a triple visible when an accepted twin asserts it too", async () => { + query.mockResolvedValue([ + [RELATION_UID, tentativeRelationProps()], + [ + "rel-block-accepted-twin", + { + sourceUid: "claim-a", + destinationUid: "question-a", + hasSchema: "supports", + }, + ], + [ + "rel-block-3", + { + sourceUid: "evidence-a", + destinationUid: "claim-a", + hasSchema: "informs", + tentative: "true", + }, + ], + ]); + + expect(await getTentativeOnlyRelationKeys()).toEqual( + new Set(["informs|evidence-a|claim-a"]), + ); + }); +}); + describe("acceptTentativeRelationInstance", () => { it("removes the tentative flag while preserving identity and provenance", async () => { propsByUid.set(RELATION_UID, { diff --git a/apps/roam/src/utils/createReifiedBlock.ts b/apps/roam/src/utils/createReifiedBlock.ts index c191dc44c..fe2b848f2 100644 --- a/apps/roam/src/utils/createReifiedBlock.ts +++ b/apps/roam/src/utils/createReifiedBlock.ts @@ -3,6 +3,7 @@ import createPage from "roamjs-components/writes/createPage"; import getPageUidByPageTitle from "roamjs-components/queries/getPageUidByPageTitle"; import getBlockProps, { isJsonObject, type json } from "./getBlockProps"; import { setBlockPropsAsync } from "./setBlockProps"; +import internalError from "./internalError"; export const DISCOURSE_GRAPH_PROP_NAME = "discourse-graph"; export const TENTATIVE_PROP_KEY = "tentative"; @@ -55,9 +56,6 @@ export const strictQueryForReifiedBlocks = async ( return resultF.length > 0 ? resultF[0] : null; }; -// Deliberate local creation of a relation counts as user acceptance, so a -// dedupe hit on a tentative imported block promotes it instead of silently -// returning a block the UI hides as pending review. export const acceptTentativeRelationInstance = async ({ instanceUid, }: { @@ -93,8 +91,20 @@ const createReifiedBlock = async ({ }; const existing = await strictQueryForReifiedBlocks(data); if (existing !== null) { + // Deliberate local creation counts as user acceptance, so a dedupe hit on + // a tentative imported block promotes it instead of returning a block the + // UI hides as pending review. Best-effort: the relation exists either way. if (parameterUids[TENTATIVE_PROP_KEY] === undefined) { - await acceptTentativeRelationInstance({ instanceUid: existing }); + try { + await acceptTentativeRelationInstance({ instanceUid: existing }); + } catch (error) { + internalError({ + error, + type: "Promote Tentative Relation On Create Failed", + context: { instanceUid: existing }, + sendEmail: false, + }); + } } return existing; } diff --git a/apps/roam/src/utils/getDiscourseContextResults.ts b/apps/roam/src/utils/getDiscourseContextResults.ts index a69719bba..cbc1a1925 100644 --- a/apps/roam/src/utils/getDiscourseContextResults.ts +++ b/apps/roam/src/utils/getDiscourseContextResults.ts @@ -12,7 +12,7 @@ import { ANY_RELATION_NAME, ANY_RELATION_REGEX, } from "./deriveDiscourseNodeAttribute"; -import { getTentativeRelationInstances } from "./tentativeRelations"; +import { getTentativeOnlyRelationKeys } from "./tentativeRelations"; const resultCache: Record>> = {}; const CACHE_TIMEOUT = 1000 * 60 * 5; @@ -279,11 +279,7 @@ const getDiscourseContextResults = async ({ resultsWithRelation.length > 0 && resultsWithRelation[0].results.length > 0 ) { - const tentativeKeys = new Set( - (await getTentativeRelationInstances()).map( - (t) => `${t.schemaUid}|${t.sourceUid}|${t.destinationUid}`, - ), - ); + const tentativeKeys = await getTentativeOnlyRelationKeys(); const isTentativeResult = (r: Result): boolean => { const source = r.effectiveSource as string; const destination = source === targetUid ? r.uid : targetUid; diff --git a/apps/roam/src/utils/tentativeRelations.ts b/apps/roam/src/utils/tentativeRelations.ts index 1b7465364..6aa0386aa 100644 --- a/apps/roam/src/utils/tentativeRelations.ts +++ b/apps/roam/src/utils/tentativeRelations.ts @@ -1,11 +1,10 @@ import { getReifiedRelations } from "./createReifiedBlock"; +import { normalizeProps } from "./getBlockProps"; import { parseSourceIdentity, type ImportedSourceIdentity, } from "./importedSourceIdentity"; -export { acceptTentativeRelationInstance } from "./createReifiedBlock"; - export type TentativeRelationInstance = { instanceUid: string; schemaUid: string; @@ -25,6 +24,23 @@ export const getTentativeRelationInstances = async (): Promise< schemaUid: r.hasSchema, sourceUid: r.sourceUid, destinationUid: r.destinationUid, - importedFrom: parseSourceIdentity(r.importedFrom), + importedFrom: + r.importedFrom === undefined + ? undefined + : parseSourceIdentity(normalizeProps(r.importedFrom)), })); }; + +// A triple is only excluded from accepted results when no accepted block +// asserts it too: a graph can hold both a tentative import and an accepted +// local twin of the same triple, and the accepted one must stay visible. +export const getTentativeOnlyRelationKeys = async (): Promise> => { + const relations = await getReifiedRelations(); + const accepted = new Set(); + const tentative = new Set(); + for (const r of relations) { + const key = `${r.hasSchema}|${r.sourceUid}|${r.destinationUid}`; + (r.tentative === "true" ? tentative : accepted).add(key); + } + return new Set([...tentative].filter((key) => !accepted.has(key))); +}; From 3a1438ae8f21b75c92fe30561c4b71387ace8bc2 Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 00:18:47 +0530 Subject: [PATCH 4/6] ENG-1869 Reuse relation schemas with multiple query patterns --- .../__tests__/importSharedRelations.test.ts | 97 +++++++++++++++++++ apps/roam/src/utils/importSharedRelations.ts | 8 +- 2 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/importSharedRelations.test.ts diff --git a/apps/roam/src/utils/__tests__/importSharedRelations.test.ts b/apps/roam/src/utils/__tests__/importSharedRelations.test.ts new file mode 100644 index 000000000..3883df788 --- /dev/null +++ b/apps/roam/src/utils/__tests__/importSharedRelations.test.ts @@ -0,0 +1,97 @@ +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 getDiscourseRelations from "~/utils/getDiscourseRelations"; +import { createRelationSchema } from "~/utils/createRelationSchema"; + +vi.hoisted(() => { + vi.stubGlobal("window", { roamAlphaAPI: { graph: { name: "local" } } }); +}); +vi.mock("~/utils/getDiscourseRelations", () => ({ default: vi.fn() })); +vi.mock("~/utils/getDiscourseNodes", () => ({ + default: () => [{ type: "local-claim", text: "Claim" }], +})); +vi.mock("~/utils/importedSourceIdentity", () => ({ + getImportedSourceRids: async () => new Set(), + findImportedNodeUidBySourceRid: vi.fn(), + writeImportedSourceIdentity: vi.fn(), +})); +vi.mock("~/components/settings/utils/accessors", () => ({ + createDiscourseNodeType: vi.fn(), +})); +vi.mock("~/utils/createRelationSchema", () => ({ + createRelationSchema: vi.fn(), +})); +vi.mock("~/utils/createReifiedBlock", () => ({ + getReifiedRelations: async () => [], + createReifiedRelation: vi.fn(), +})); +vi.mock("roamjs-components/writes", () => ({ deleteBlock: vi.fn() })); +vi.mock("~/utils/discoverSharedRelations", () => ({ + discoverSharedRelations: async () => ({ + relations: [], + relTypeSchemas: [], + nodeSchemas: [ + { + localId: "claim", + rid: "orn:obsidian.schema:remote/claim", + label: "Claim", + authorId: "author", + createdAt: new Date("2026-09-07"), + }, + ], + relTripleSchemas: [ + { + localId: "supports", + rid: "orn:obsidian.schema:remote/supports", + label: "Supports", + complement: "Supported by", + sourceType: "claim", + destinationType: "claim", + authorId: "author", + createdAt: new Date("2026-09-07"), + }, + ], + }), +})); + +const relation = (id: string): DiscourseRelation => ({ + id, + label: "Supports", + complement: "Supported by", + source: "local-claim", + destination: "local-claim", + triples: [], +}); +const client = {} as DGSupabaseClient; + +beforeEach(() => vi.clearAllMocks()); + +describe("importSharedRelations schema matching", () => { + it("reuses one schema when its query patterns produce multiple matches", async () => { + vi.mocked(getDiscourseRelations).mockReturnValue([ + { + ...relation("local-supports"), + triples: [["source", "references", "destination"]], + }, + { + ...relation("local-supports"), + triples: [["source", "is in page", "destination"]], + }, + ]); + await expect(importSharedRelations(client, 7)).resolves.toBeUndefined(); + expect(createRelationSchema).not.toHaveBeenCalled(); + }); + + it("rejects matches to two different schemas", async () => { + vi.mocked(getDiscourseRelations).mockReturnValue([ + relation("supports-one"), + relation("supports-two"), + ]); + await expect(importSharedRelations(client, 7)).rejects.toThrow( + "multiple matches", + ); + expect(createRelationSchema).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/roam/src/utils/importSharedRelations.ts b/apps/roam/src/utils/importSharedRelations.ts index adb7aa5cd..2011acbbc 100644 --- a/apps/roam/src/utils/importSharedRelations.ts +++ b/apps/roam/src/utils/importSharedRelations.ts @@ -139,11 +139,13 @@ const matchImportedRelationSchemas = async ( r.source === source && r.destination === destination, ); - if (match.length > 1) { + // Each query pattern can produce a match for the same local schema. + const matchIds = [...new Set(match.map(({ id }) => id))]; + if (matchIds.length > 1) { throw new Error("multiple matches"); } - if (match.length === 1) { - blockUid = match[0].id; + if (matchIds.length === 1) { + blockUid = matchIds[0]; } else { blockUid = await createRelationSchema({ label, From 70f8aca1b1c8a35c2ce28bfe986f050c6ab3b08c Mon Sep 17 00:00:00 2001 From: sid597 Date: Mon, 7 Sep 2026 01:07:20 +0530 Subject: [PATCH 5/6] Handle pending relation load failures and defer the empty state --- apps/roam/src/components/DiscourseContext.tsx | 6 +- .../components/TentativeRelationInstances.tsx | 69 ++++++++----- .../tentativeRelationLoading.test.ts | 98 +++++++++++++++++++ 3 files changed, 145 insertions(+), 28 deletions(-) create mode 100644 apps/roam/src/utils/__tests__/tentativeRelationLoading.test.ts diff --git a/apps/roam/src/components/DiscourseContext.tsx b/apps/roam/src/components/DiscourseContext.tsx index 9a2ec5013..206c3ea50 100644 --- a/apps/roam/src/components/DiscourseContext.tsx +++ b/apps/roam/src/components/DiscourseContext.tsx @@ -173,7 +173,9 @@ export const ContextContent = ({ uid, results, overlayRefresh }: Props) => { }); const [tabId, setTabId] = useState(0); const [groupByTarget, setGroupByTarget] = useState(false); - const [tentativeCount, setTentativeCount] = useState(0); + const [tentativeCount, setTentativeCount] = useState( + undefined, + ); const body = queryResults.length ? ( <>