[ENG-1852] Keep Roam shared node content fresh - #1147
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 554884cf58
ℹ️ 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".
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
554884c to
116e32c
Compare
116e32c to
ad939d4
Compare
| import type { LocalContentDataInput } from "@repo/database/inputTypes"; | ||
| import { contentTypes } from "@repo/content-model"; | ||
|
|
||
| export type RoamFullContentNode = { |
There was a problem hiding this comment.
Is the intent to replace this with CrossAppContent eventually?
…raph into eng-1852-keep-roam-shared-node-content-fresh-in-the-sync-loop
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| direct: { | ||
| localId: node.source_local_id, | ||
| value: title, | ||
| }, |
There was a problem hiding this comment.
@maparent @mdroidian crossAppNode requires direct to be present but crossAppNodeToDbContent converter explicitly requires us to pass the variant type, so should we update the crossAppNode.content an or case?
There was a problem hiding this comment.
It requires to pass the variant type to choose which of the two contents you're getting from the node. Will probably be updated with format when we have multiple format. I don't think we need to be an or case, or at least I don't see how it follows.
There was a problem hiding this comment.
We could have a variant that returns all contents as an array, would that be more useful to you?
There was a problem hiding this comment.
I was saying that here I am passing content.direct and content.full but direct will not be used anywhere it is here because the crossAppNode type requires both direct and full.
There was a problem hiding this comment.
Ah! Yes, full should be optional.
There was a problem hiding this comment.
In a conference now, I'll make a task soon.
There was a problem hiding this comment.
I do not see a case where the title is optional; do you?
There was a problem hiding this comment.
In this case direct should be optional because it is not needed by the function that will consume this data
There was a problem hiding this comment.
Hmm... Are you sure that sharing does not include the title at all?
Possible underlying assumption: Do you expect the title to also be part of the full content?
In Obsidian we found it easier to separate the title from the content, so we assume that they are disjoint.
2 participants
https://www.loom.com/share/2dcd282d7a0f4a878c8780cbca56c820
Summary
:page/edit-timeor title-level:edit/time.fullmarkdown content without running embeddings, while keeping direct/direct_and_description embedding updates on the existing changed-node path.Manual demo validation:
105833and shared Roam page UIDtzmTaBmqT([[CLM]] - cat -).ResourceAccessandContent.variant = direct, but noContent.variant = full; ran the existing Roam Supabase sync path; confirmed a newfullrow was created with Roam markdowncontent.
fullexisted, edited the Roam page body; ran the existing Roam Supabase sync path again; confirmed the existingfullrow updated with the new page content.