ENG-1101 Create reified relation via suggestion mode #598
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change modifies block creation in Changes
Sequence DiagramsequenceDiagram
actor User
participant SuggestionsBody as SuggestionsBody<br/>(Component)
participant Settings as Extension<br/>Settings
participant DiscourseUtil as Discourse<br/>Utilities
participant Roam as Roam<br/>Graph API
participant Toast as Toast<br/>Notifications
User->>SuggestionsBody: Trigger block creation
SuggestionsBody->>Settings: Check "use-reified-relations"
alt Setting enabled
SuggestionsBody->>DiscourseUtil: findDiscourseNodeByTitleAndUid (source)
alt Node found
DiscourseUtil-->>SuggestionsBody: source node
SuggestionsBody->>DiscourseUtil: findDiscourseNodeByTitleAndUid (target)
alt Node found
DiscourseUtil-->>SuggestionsBody: target node
SuggestionsBody->>DiscourseUtil: Validate relation type
alt Valid relation exists
DiscourseUtil-->>SuggestionsBody: relation validated
SuggestionsBody->>Roam: createReifiedRelation(source, target, relation)
Roam-->>SuggestionsBody: success
SuggestionsBody->>User: Reified relation created
else No valid relation
SuggestionsBody->>Toast: Show error (relation not found)
Toast-->>User: Error message
end
else Node not found
SuggestionsBody->>Toast: Show error (target node not found)
Toast-->>User: Error message
end
else Node not found
SuggestionsBody->>Toast: Show error (source node not found)
Toast-->>User: Error message
end
else Setting disabled
SuggestionsBody->>Roam: createBlock (original flow)
Roam-->>SuggestionsBody: success
SuggestionsBody->>User: Standard block created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/roam/src/components/SuggestionsBody.tsx (3)
311-324: MovediscourseNodecheck before the API call for efficiency.The check for
discourseNode === falseat line 316 should be placed before callingfindDiscourseNodeByTitleAndUid. SincediscourseNodeis already computed (it's a memoized value), checking it first avoids an unnecessary function call when it's falsy.Apply this diff:
if (getSetting("use-reified-relations")) { + if (discourseNode === false) { + renderToast({ + id: "suggestions-create-block-error", + content: "Could not identify type of source", + intent: "danger", + timeout: 5000, + }); + return; + } const selectedNodeType = findDiscourseNodeByTitleAndUid({ uid: node.uid, title: node.title as string, }); - if (discourseNode === false) { - renderToast({ - id: "suggestions-create-block-error", - content: "Could not identify type of source", - intent: "danger", - timeout: 5000, - }); - return; - } if (selectedNodeType === false) {
341-367: Add error handling forcreateReifiedRelationcalls.The
createReifiedRelationcalls are awaited but not wrapped in a try/catch. If the operation fails, the error will propagate unhandled and the user won't receive feedback. Consider wrapping in try/catch with appropriate error toast.if (relevantRelns.length) { if (relevantRelns.length > 1) { // I don't want to panick the user with this. // TODO: Maybe think of adding a relation type picker? console.warn("Picking an arbitrary relation"); } const rel = relevantRelns[0]; - if (rel.destination === selectedNodeType.type) - await createReifiedRelation({ - sourceUid: tagUid, - destinationUid: node.uid, - relationBlockUid: rel.id, - }); - else - await createReifiedRelation({ - sourceUid: node.uid, - destinationUid: tagUid, - relationBlockUid: rel.id, - }); + try { + if (rel.destination === selectedNodeType.type) { + await createReifiedRelation({ + sourceUid: tagUid, + destinationUid: node.uid, + relationBlockUid: rel.id, + }); + } else { + await createReifiedRelation({ + sourceUid: node.uid, + destinationUid: tagUid, + relationBlockUid: rel.id, + }); + } + } catch (error) { + console.error("Failed to create reified relation:", error); + renderToast({ + id: "suggestions-create-block-error", + content: "Failed to create relation", + intent: "danger", + timeout: 5000, + }); + return; + } } else {
342-346: Noted: Multiple relation handling.The TODO notes the need for a relation type picker when multiple relevant relations exist. The current behavior of picking the first one may surprise users.
Would you like me to open an issue to track implementing a relation type picker for this scenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/SuggestionsBody.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
apps/roam/src/components/SuggestionsBody.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/components/SuggestionsBody.tsx
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/components/SuggestionsBody.tsx
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/components/SuggestionsBody.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/SuggestionsBody.tsx
🧠 Learnings (10)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-12-07T20:54:19.988Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:19.988Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
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`.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/SuggestionsBody.tsx
🧬 Code graph analysis (1)
apps/roam/src/components/SuggestionsBody.tsx (3)
apps/roam/src/utils/extensionSettings.ts (1)
getSetting(3-11)apps/roam/src/utils/findDiscourseNode.ts (1)
findDiscourseNodeByTitleAndUid(23-43)apps/roam/src/utils/createReifiedBlock.ts (1)
createReifiedRelation(111-131)
🔇 Additional comments (1)
apps/roam/src/components/SuggestionsBody.tsx (1)
22-32: LGTM!The new imports are appropriate for the added reified relation functionality and follow the existing import patterns in the file.
| const selectedNodeType = findDiscourseNodeByTitleAndUid({ | ||
| uid: node.uid, | ||
| title: node.title as string, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the SuggestionsBody.tsx file and check the context around lines 312-315
fd "SuggestionsBody.tsx" --type fRepository: DiscourseGraphs/discourse-graph
Length of output: 119
🏁 Script executed:
# Search for Result type definition from roamjs-components
rg "export.*Result" --type ts -A 5 | head -100Repository: DiscourseGraphs/discourse-graph
Length of output: 4394
🏁 Script executed:
# Search for SuggestedNode type definition
rg "type SuggestedNode" --type ts -A 10Repository: DiscourseGraphs/discourse-graph
Length of output: 1472
🏁 Script executed:
# Check the findDiscourseNodeByTitleAndUid function signature
rg "findDiscourseNodeByTitleAndUid" --type ts -B 2 -A 5Repository: DiscourseGraphs/discourse-graph
Length of output: 4891
🏁 Script executed:
# Get the exact context around lines 312-315 in SuggestionsBody.tsx
sed -n '300,330p' apps/roam/src/components/SuggestionsBody.tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 1097
🏁 Script executed:
# Check the Result type definition more carefully
cat -n apps/roam/src/utils/types.ts | head -20Repository: DiscourseGraphs/discourse-graph
Length of output: 598
🏁 Script executed:
# Check other usages of findDiscourseNodeByTitleAndUid to see what property they pass
rg "findDiscourseNodeByTitleAndUid" --type ts -B 2 -A 8Repository: DiscourseGraphs/discourse-graph
Length of output: 6792
🏁 Script executed:
# Look at the full SuggestedNode definition and context where it's created
rg "hydeFilteredNodes|SuggestedNode" --type ts -B 3 -A 3 | head -80Repository: DiscourseGraphs/discourse-graph
Length of output: 5818
🏁 Script executed:
# Search for where hydeFilteredNodes is set/updated
rg "setHydeFilteredNodes" --type ts -B 5 -A 5Repository: DiscourseGraphs/discourse-graph
Length of output: 2764
🏁 Script executed:
# Check the hyde.ts file for SuggestedNode creation
rg "SuggestedNode|Result" apps/roam/src/utils/hyde.ts -B 2 -A 8Repository: DiscourseGraphs/discourse-graph
Length of output: 6033
🏁 Script executed:
# Get the exact context of handleCreateBlock to see the full usage
sed -n '310,330p' apps/roam/src/components/SuggestionsBody.tsxRepository: DiscourseGraphs/discourse-graph
Length of output: 787
Fix type assertion for node.title—use node.text instead.
SuggestedNode (which extends Result) has a text property, not title. The type assertion as string masks accessing a non-existent property; pass node.text to findDiscourseNodeByTitleAndUid instead.
🤖 Prompt for AI Agents
In apps/roam/src/components/SuggestionsBody.tsx around lines 312-315, the code
passes node.title as a string to findDiscourseNodeByTitleAndUid but
SuggestedNode/Result has a text property not title; replace the call to pass
node.text (ensuring it's a string or safely cast/guarded) instead of node.title
as string so the function receives the correct value and the bogus type
assertion is removed.
https://linear.app/discourse-graphs/issue/ENG-1101/create-reified-relation-via-suggestion-mode
Create a reified relation from the suggestion mode.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.