ENG-1867 Import cross-app relations into Roam as tentative relations - #1302
Hidden character warning
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
452cc49 to
1928997
Compare
1928997 to
70f63a7
Compare
70f63a7 to
5ce9525
Compare
5ce9525 to
69aedbc
Compare
69aedbc to
9036ffd
Compare
9036ffd to
f4ef064
Compare
f4ef064 to
c3e095b
Compare
c3e095b to
b062e59
Compare
b0d1031 to
a360ea2
Compare
a360ea2 to
5f45a0d
Compare
5f45a0d to
6699f70
Compare
6699f70 to
84c96a5
Compare
84c96a5 to
d0dcb0f
Compare
|
Ok. Addressed most issues. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0dcb0f2bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let templateTree: RoamBasicNode[] | undefined; | ||
| if (template !== undefined) { | ||
| const tree = getBasicTreeByParentUid(pageUid); | ||
| const templateUid = tree[3].uid; |
There was a problem hiding this comment.
tree[3] assumes Template is always the 4th child. That’s true for the tree you just built, but it isn’t a stable way to find the block.
If someone later adds, removes, or reorders a setting above Template, this still compiles and then writes markdown into the wrong parent. Easy to miss because the 3 is disconnected from the "Template" key.
Elsewhere we look these up by name with getSubTree. Same thing here:
const templateUid = getSubTree({ tree, key: "Template" }).uid;
| label: string; | ||
| shortcut?: string; | ||
| format?: string; | ||
| template?: string; |
There was a problem hiding this comment.
It looks like you are expecting a Markdown string here? Normally, in Roam, templates are RoamBasicNode[]. So this is going against the expected pattern. Let's explicitly called out as a comment on ln 1109, or a custom type, or even JSONDoc above the function.
It may be true that CrossApp* will generally expect Markdown, but this is currently in accessors.ts, so the context matters.
There was a problem hiding this comment.
Actually broadening the type.
d0dcb0f to
2e6a60d
Compare
|
Made a few changes. I choose to allow relations with no condition vs creating an empty condition. I also broadened the input type. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e6a60d7cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sourceUid, | ||
| destinationUid, | ||
| relationBlockUid, | ||
| tentative: true, |
There was a problem hiding this comment.
Exclude tentative relations from accepted-relation queries
When stored relations are enabled, this imported relation is immediately treated as an accepted fact: registerDiscourseDatalogTranslators matches every reified block using only sourceUid, destinationUid, and hasSchema, and no Roam consumer filters the newly added tentative property. Consequently, merely importing shared nodes can change query, context, and canvas results before the user accepts the relation; exclude tentative records from normal relation paths until confirmation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is valid, but I think it's a new scope.
https://linear.app/discourse-graphs/issue/ENG-1867/import-cross-app-relations-into-roam-as-tentative-relations
Code walkthrough:
https://www.loom.com/share/e3cfe2ce63a34382a2b23200cb9e0b1e
Note: Since the code walkthrough I removed the RidToId in the discovery results.
Also added the correction to getDiscourseRelations, and code to delete an incorrect imported relation.
Simple demo:
https://www.loom.com/share/a0dd5ca98a194536a6d4350ec96b8bcb
Demo with type creation:
https://www.loom.com/share/bc8336651d7243f1b6f809ee6e886d9f
Quick note on templates
https://www.loom.com/share/5bfd0fffc27446af9d5d88383c7e276a