ENG-1011 Create Function to Record Relation Triples in Roam

[maparent]

https://linear.app/discourse-graphs/issue/ENG-1011/create-function-to-record-relation-triples-in-roam

Summary by CodeRabbit

• Chores
  • Added utility functions to support internal system operations related to block and relation management.

[linear]

ENG-1011 Create Function to Record Relation Triples in Roam

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[maparent]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

Introduces a new utility module that enables creation of reified blocks and relations within a Roam discourse graph system. The module provides functions to create parameterized blocks under a destination and conditionally establish reified relations between source and destination blocks when a user setting is enabled.

Changes

Cohort / File(s)	Summary
New utility module for reified block creation apps/roam/src/utils/createReifiedBlock.ts	Adds createReifiedBlock() to create parameterized blocks with discourse-graph namespace properties, and createReifiedRelation() to conditionally create reified relations based on a user setting. Includes internal caching of relation page UID with lazy initialization.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant createReifiedRelation
    participant getSetting
    participant createReifiedBlock
    participant getRelationPageUid
    participant Cache
    participant roamAPI as Roam API<br/>(lookup/create page)
    
    Caller->>createReifiedRelation: createReifiedRelation(sourceUid, relationBlockUid, destinationUid)
    createReifiedRelation->>getSetting: getSetting("use-reified-relations")
    
    alt Setting enabled
        getSetting-->>createReifiedRelation: true
        createReifiedRelation->>getRelationPageUid: getRelationPageUid()
        getRelationPageUid->>Cache: Check cached uid
        
        alt Cache hit
            Cache-->>getRelationPageUid: cached uid
        else Cache miss
            getRelationPageUid->>roamAPI: getPageUidByPageTitle("roam/js/discourse-graph/relations")
            
            alt Page found
                roamAPI-->>getRelationPageUid: uid
            else Page not found
                getRelationPageUid->>roamAPI: createPage(title)
                roamAPI-->>getRelationPageUid: new uid
            end
            
            getRelationPageUid->>Cache: Store uid
        end
        
        getRelationPageUid-->>createReifiedRelation: relationPageUid
        createReifiedRelation->>createReifiedBlock: createReifiedBlock(relationPageUid, relationBlockUid, {sourceUid, destinationUid})
        createReifiedBlock->>roamAPI: createBlock, setBlockProps
        roamAPI-->>createReifiedBlock: new block uid
        createReifiedBlock-->>createReifiedRelation: uid
        createReifiedRelation-->>Caller: uid
    else Setting disabled
        getSetting-->>createReifiedRelation: false
        createReifiedRelation-->>Caller: undefined
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Caching logic in getRelationPageUid(): Verify that the lazy initialization and cache reuse pattern correctly handles concurrent access if applicable
• Parameter passing structure: Confirm that parameterUids Record structure and discourse-graph namespace property formatting match expected schema

Pre-merge checks

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main change: creation of utility functions to record relation triples in Roam, matching the changeset's new createReifiedBlock and createReifiedRelation functions.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

apps/roam/src/utils/createReifiedBlock.ts (2)

14-21: Add error handling for block creation.

The block creation logic is correct, but there's no error handling if createBlock fails. Consider wrapping in try-catch or allowing the error to propagate with proper context.

---

7-11: Consider using named parameters for improved clarity.

The function has 3 parameters. Per coding guidelines, consider using object destructuring for better readability and maintainability.

-export const createReifiedBlock = async (
-  destinationBlockUid: string,
-  schemaUid: string,
-  parameterUids: Record<string, string>,
-): Promise<string> => {
+export const createReifiedBlock = async ({
+  destinationBlockUid,
+  schemaUid,
+  parameterUids,
+}: {
+  destinationBlockUid: string;
+  schemaUid: string;
+  parameterUids: Record<string, string>;
+}): Promise<string> => {

Note: This would require updating call sites as well.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1642ff7 and 441cffa.

📒 Files selected for processing (1)

• apps/roam/src/utils/createReifiedBlock.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Prefer type over interface
Use explicit return types for functions
Avoid any types when possible

Files:

• apps/roam/src/utils/createReifiedBlock.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants

Files:

• apps/roam/src/utils/createReifiedBlock.ts

apps/roam/**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API

Files:

• apps/roam/src/utils/createReifiedBlock.ts

🔇 Additional comments (3)

apps/roam/src/utils/createReifiedBlock.ts (3)

1-5: LGTM!

The imports are properly structured and use appropriate paths.

---

47-63: Original review comment is incorrect.

The getSetting function is synchronous (returns T, not Promise<T>), so line 52 is correct as written. There is no issue with the condition check at line 53—authorized will be a boolean value, not a Promise.

The concern about getSetting being async and causing an "always truthy Promise" condition does not apply here.

Likely an incorrect or invalid review comment.

---

22-30: (A) – I need the official Roam API docs to verify if roamAlphaAPI.data.block.update is async.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Address the TODO and add safety check for roamAlphaAPI.

Two concerns:

1. The TODO about ensuring uniqueness should be resolved or tracked properly.
2. window.roamAlphaAPI might be undefined if the API is not loaded, which would cause a runtime error.

Consider adding a guard:

+  if (!window.roamAlphaAPI?.util) {
+    throw new Error("Roam API not available");
+  }
   const newUid = window.roamAlphaAPI.util.generateUID();

Would you like me to open an issue to track the uniqueness TODO?

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export const createReifiedBlock = async (
	destinationBlockUid: string,
	schemaUid: string,
	parameterUids: Record<string, string>,
	): Promise<string> => {
	// TODO/Question: Should we try to ensure uniqueness?
	const newUid = window.roamAlphaAPI.util.generateUID();
	export const createReifiedBlock = async (
	destinationBlockUid: string,
	schemaUid: string,
	parameterUids: Record<string, string>,
	): Promise<string> => {
	// TODO/Question: Should we try to ensure uniqueness?
	if (!window.roamAlphaAPI?.util) {
	throw new Error("Roam API not available");
	}
	const newUid = window.roamAlphaAPI.util.generateUID();

🤖 Prompt for AI Agents

In apps/roam/src/utils/createReifiedBlock.ts around lines 7–13, resolve the TODO
and guard against a missing Roam API: first check that window.roamAlphaAPI
exists and throw or return a clear error if not, then remove the bare TODO and
implement a UID-uniqueness loop that generates a UID, checks for an existing
entity with that UID via the Roam API (e.g., a pull/query or a util lookup),
retrying up to a small maxAttempts (e.g., 5–10) before failing with an explicit
error; if you cannot guarantee uniqueness beyond retries, replace the TODO with
a comment referencing an opened issue and create that issue to track stronger
uniqueness guarantees.

[maparent]

none of this package will be activated without roamAlphaAPI, not a concern.
I think if there is an error in this code, I will let it percolate.
I was hesitant on TODO scope creep, but decided it's in scope.

[coderabbitai]

@maparent, understood! I appreciate the clarification:

• The roamAlphaAPI check isn't necessary given the package activation guarantees
• You'll address the uniqueness TODO as part of this PR since it's in scope
• Error propagation approach is clear

Thanks for the context on the architectural constraints! 👍

---

🧠 Learnings used

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.886Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.

---

Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API

---

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to `window.roamAlphaAPI` is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.

[maparent]

@CodeRabbit review last commit

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the last commit on the PR branch
git log -1 --format="%H %s" origin/eng-1011-create-function-to-record-relation-triples-in-roam

# Get the full diff of the last commit
git show origin/eng-1011-create-function-to-record-relation-triples-in-roam --stat

# Get the detailed diff of the last commit
git show origin/eng-1011-create-function-to-record-relation-triples-in-roam

Length of output: 740

---

🏁 Script executed:

#!/bin/bash
# Get PR details including commits using GitHub CLI
gh pr view 534 --json commits,headRefName,headRefOid

# List all commits in the PR
gh pr view 534 --json commits --jq '.commits[] | "\(.oid[0:7]) \(.messageHeadline)"'

# Get the last commit hash
LAST_COMMIT=$(gh pr view 534 --json commits --jq '.commits[-1].oid')
echo "Last commit: $LAST_COMMIT"

# Get the diff of the last commit
gh api repos/DiscourseGraphs/discourse-graph/commits/$LAST_COMMIT

Length of output: 8264

---

🏁 Script executed:

#!/bin/bash
# Get the diff of the last commit with more context
gh api repos/DiscourseGraphs/discourse-graph/commits/7de68d8cc41adb16bc46b24c1c4ddaf58c1ff768 --jq '.files[0].patch'

# Get the full file content at this commit
gh api repos/DiscourseGraphs/discourse-graph/contents/apps/roam/src/utils/createReifiedBlock.ts?ref=7de68d8cc41adb16bc46b24c1c4ddaf58c1ff768 --jq '.content' | base64 -d

Length of output: 5820

---

@maparent Thank you for the commit! I've reviewed commit 7de68d8 ("ensure relation uniqueness"). Here's my analysis:

✅ Positive Changes

1. Addresses uniqueness requirement - Successfully implements the TODO about ensuring uniqueness
2. Good separation of concerns - queryForReifiedBlocks and checkForReifiedBlock provide reusable query functionality
3. Prevents duplicates - Returns existing block UID instead of creating duplicates

🔍 Issues & Suggestions

1. Inefficient UID Generation (Line 49)

const newUid = window.roamAlphaAPI.util.generateUID();
// TODO: Check that the parameterUids keys correspond to the schema
const data = {
  ...parameterUids,
  hasSchema: schemaUid,
};
const exists = await checkForReifiedBlock(data);
if (exists !== null) return exists;

The newUid is generated even when an existing block is found. Move the generation after the existence check:

const data = {
  ...parameterUids,
  hasSchema: schemaUid,
};
const exists = await checkForReifiedBlock(data);
if (exists !== null) return exists;

const newUid = window.roamAlphaAPI.util.generateUID();

2. Confusing Return Type in queryForReifiedBlocks

The union type string | string[] | null is complex:

• strict=true, found → returns string
• strict=true, not found → returns null
• strict=false → returns string[]

Suggestion: Consider simplifying by always returning string[] and let callers handle empty arrays:

export const queryForReifiedBlocks = async (
  parameterUids: Record<string, string>,
  strict: boolean = true,
): Promise<string[]> => {
  // ... query logic ...
  if (strict) {
    return result
      .filter(([, params]) => Object.keys(params).length === paramsAsSeq.length)
      .map(([uid]) => uid);
  }
  return result.map(([uid]) => uid);
};

export const checkForReifiedBlock = async (
  parameterUids: Record<string, string>,
): Promise<string | null> => {
  const results = await queryForReifiedBlocks(parameterUids, true);
  return results.length > 0 ? results[0] : null;
};

3. Unnecessary Defensive Check (Line 35)

if (Array.isArray(result)) return result[0]; // this should never happen with strict

This check is never triggered when strict=true. If you simplify the return type as suggested above, this becomes unnecessary.

4. Query Precision

The Datalog query finds blocks containing all required keys but doesn't ensure ONLY those keys exist—the strict filter handles this post-query. This is correct but might return false positives to be filtered. Consider documenting this behavior in a comment.

5. Missing await on setBlockProps? (Line 65)

setBlockProps(newUid, {
  // eslint-disable-next-line @typescript-eslint/naming-convention
  "discourse-graph": data,
});

Verify if setBlockProps is synchronous or if the missing await is intentional. If it's async and you're not awaiting, the function might return before props are set.

📝 Minor Notes

• The TODO comment about checking parameter keys against schema appears twice (lines 11 and 51)—consider addressing this in a follow-up
• Great job removing the old TODO about uniqueness!

Would you like me to help address any of these issues, particularly the UID generation efficiency or the return type simplification?

[maparent]

@CodeRabbit review last commit

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR details including commits
gh pr view 534 --json commits,headRefOid --jq '{lastCommit: .commits[-1], headOid: .headRefOid}'

# Get the last commit hash
LAST_COMMIT=$(gh pr view 534 --json commits --jq '.commits[-1].oid')
echo "=== Last commit: $LAST_COMMIT ==="

# Get commit details
gh api repos/DiscourseGraphs/discourse-graph/commits/$LAST_COMMIT --jq '{
  sha: .sha,
  message: .commit.message,
  author: .commit.author.name,
  date: .commit.author.date,
  files: [.files[] | {filename: .filename, status: .status, additions: .additions, deletions: .deletions}]
}'

# Get the diff for each file
for file in $(gh api repos/DiscourseGraphs/discourse-graph/commits/$LAST_COMMIT --jq '.files[].filename'); do
  echo "=== Diff for $file ==="
  gh api repos/DiscourseGraphs/discourse-graph/commits/$LAST_COMMIT --jq ".files[] | select(.filename == \"$file\") | .patch"
done

Length of output: 4337

---

@maparent Thank you for the update! I've reviewed commit 9c34b27 ("separate cases for clean signatures"). Excellent refactoring! 🎉

✅ Major Improvements

You've addressed all the key issues from my previous review:

1. ✅ Fixed UID generation efficiency - The newUid is now generated after the duplicate check (Line 67), preventing unnecessary UID generation when duplicates exist.

2. ✅ Simplified return types - Split the confusing string | string[] | null union into two clear functions:

   • queryForReifiedBlocks(): consistently returns Promise<string[]>
   • strictQueryForReifiedBlocks(): returns Promise<string | null>

3. ✅ Better API design - Changed from positional parameters to an object parameter with named properties, making the function more maintainable and self-documenting.

4. ✅ Explicit duplicate control - Added allowDuplicates parameter (defaults to true for safety) and explicitly set to false for relations with a helpful comment.

🌟 Nice Additions

1. Warning for multiple strict results (Lines 37-43) - Great debugging aid if the query returns multiple matches when only one is expected.

2. Internal utility function - queryForReifiedBlocksUtil provides a clean foundation for both query variants.

📝 Minor Observations

1. Still missing await on setBlockProps (Line 71):

setBlockProps(newUid, {
  // eslint-disable-next-line @typescript-eslint/naming-convention
  "discourse-graph": data,
});

If setBlockProps returns a Promise, consider awaiting it to ensure properties are set before the function returns. If it's intentionally fire-and-forget, a comment would clarify the intent.

2. TODO remains (Line 57) - The comment about checking parameter keys against schema is still present. Consider creating a follow-up issue if this validation is important.

3. Default behavior - allowDuplicates: true by default is conservative and safe, giving callers flexibility while preventing duplicates where it matters (relations).

🎯 Overall Assessment

This is a well-executed refactoring that significantly improves code clarity and addresses the feedback from the previous review. The separation of concerns between strict and non-strict queries makes the API much more intuitive. Great work! 👍

[maparent]

@CodeRabbit: setBlockProps does not need await. I am leaving the todo intentionally. Thanks for pushing me on api clarity.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@maparent, thank you for the clarifications! I've noted that setBlockProps is synchronous and doesn't require await. The API improvements you made really do make the code much more maintainable—appreciate you taking the time to refactor based on the feedback!

Best of luck with the rest of the PR! 🚀

(ᵔᴥᵔ) ♪

---

✏️ Learnings added

Learnt from: maparent
URL: https://github.com/DiscourseGraphs/discourse-graph/pull/534

Timestamp: 2025-11-06T13:48:34.985Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[mdroidian]

Before doing a full review, could you please add the required loom video

Done When
Behavior is verified via a loom video by manually invoking the function and confirming correct output structure.

[sid597]

A stub calling function would be helpful for review, maybe add it to onChange for the button Reified Relation Triples button?

[sid597]

I spent some time on reviewing by adding the stub function, the function does creates the block and store the properties. Then I went into rabbithole of what is going on and why, following threads of discussion and its all very interesting.

[sid597]

A video of expected input and output would be useful, not sure if I reproduced it right

[maparent]

Here is the loom video as requested in the task. I thought those were only for UX PRs?
https://www.loom.com/share/2937c0cb53e042f48b9d874d9ca807d1

[mdroidian]

Here is the loom video as requested in the task. I thought those were only for UX PRs?

Videos are required by default for UX PRs, but most PRs would benefit from including one as well. That said, if a task explicitly requests a video, the point is moot.

[maparent]

Of course, just asking why the video was requested in this specific task.

[mdroidian]

@maparent is this one ready for review now? If so, please change the status from Draft.

[mdroidian]

Approved to not block, but quite a few things I am unclear on based on the task.