-
Notifications
You must be signed in to change notification settings - Fork 6
[ENG-1852] Keep Roam shared node content fresh #1147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ad939d4
21f9280
4d23961
fddb012
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import type { TreeNode } from "roamjs-components/types"; | ||
| import type { CrossAppNode } from "@repo/database/crossAppContracts"; | ||
| import { buildFullMarkdown } from "./convertRoamNodeToFullContent"; | ||
| import { contentTypes } from "@repo/content-model"; | ||
|
|
||
| const block = (text: string, children: TreeNode[] = []): TreeNode => ({ | ||
| text, | ||
| children, | ||
| order: 0, | ||
| parents: [], | ||
| uid: "", | ||
| heading: 0, | ||
| open: true, | ||
| viewType: "bullet", | ||
| blockViewType: "outline", | ||
| editTime: new Date(0), | ||
| textAlign: "left", | ||
| props: { imageResize: {}, iframe: {} }, | ||
| }); | ||
|
|
||
| const title = "Sleep improves memory consolidation"; | ||
|
|
||
| const blocks: TreeNode[] = [ | ||
| block( | ||
| "Multiple studies show that sleep after learning strengthens memory traces.", | ||
| ), | ||
| block("Supporting evidence:", [block("[[EVD]] - Rasch & Born 2013")]), | ||
| ]; | ||
|
|
||
| export const roamClaimFullMarkdownSimpleExample: { | ||
| title: string; | ||
| blocks: TreeNode[]; | ||
| full: CrossAppNode["content"]["full"]; | ||
| } = { | ||
| title, | ||
| blocks, | ||
| full: { | ||
| contentType: contentTypes.roamMarkdown, | ||
| value: buildFullMarkdown({ title, blocks }), | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,23 @@ | ||
| import { toMarkdown } from "./pageToMarkdown"; | ||
| import { type RoamDiscourseNodeData } from "./getAllDiscourseNodesSince"; | ||
| import { type DiscourseNode } from "./getDiscourseNodes"; | ||
| import getFullTreeByParentUid from "roamjs-components/queries/getFullTreeByParentUid"; | ||
| import getPageViewType from "roamjs-components/queries/getPageViewType"; | ||
| import type { TreeNode, ViewType } from "roamjs-components/types"; | ||
| import { contentTypes } from "@repo/content-model"; | ||
| import type { CrossAppNode } from "@repo/database/crossAppContracts"; | ||
| import { crossAppNodeToDbContent } from "@repo/database/lib/crossAppConverters"; | ||
| import type { LocalContentDataInput } from "@repo/database/inputTypes"; | ||
|
|
||
| export type RoamFullContentNode = { | ||
| author_local_id: string; | ||
| source_local_id: string; | ||
| created: string | number; | ||
| last_modified: string | number; | ||
| text: string; | ||
| node_type_id: string; | ||
| node_title?: string; | ||
| }; | ||
|
|
||
| const FULL_MARKDOWN_OPTS = { | ||
| refs: true, | ||
| embeds: true, | ||
|
|
@@ -38,26 +50,34 @@ export const buildFullMarkdown = ({ | |
| export const convertRoamNodeToFullContent = ({ | ||
| nodes, | ||
| }: { | ||
| nodes: RoamDiscourseNodeData[]; | ||
| nodes: RoamFullContentNode[]; | ||
| }): LocalContentDataInput[] => | ||
| nodes.flatMap((node) => { | ||
| try { | ||
| const title = node.node_title ?? node.text; | ||
| const blocks = getFullTreeByParentUid(node.source_local_id).children; | ||
| const viewType = getPageViewType(title) || "bullet"; | ||
| return [ | ||
| { | ||
| author_local_id: node.author_local_id, | ||
| source_local_id: node.source_local_id, | ||
| created: new Date(node.created || Date.now()).toISOString(), | ||
| last_modified: new Date( | ||
| node.last_modified || Date.now(), | ||
| ).toISOString(), | ||
| text: buildFullMarkdown({ title, blocks, viewType }), | ||
| variant: "full", | ||
| scale: "document", | ||
| const crossAppNode: CrossAppNode = { | ||
| author: { localId: node.author_local_id }, | ||
| localId: node.source_local_id, | ||
| createdAt: new Date(node.created || Date.now()), | ||
| modifiedAt: new Date(node.last_modified || Date.now()), | ||
| nodeType: { localId: node.node_type_id }, | ||
| content: { | ||
| direct: { | ||
| localId: node.source_local_id, | ||
| value: title, | ||
| }, | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maparent @mdroidian
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could have a variant that returns all contents as an array, would that be more useful to you?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was saying that here I am passing
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! Yes, full should be optional.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a conference now, I'll make a task soon.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not see a case where the title is optional; do you?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case direct should be optional because it is not needed by the function that will consume this data
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... Are you sure that sharing does not include the title at all? |
||
| full: { | ||
| localId: node.source_local_id, | ||
| value: buildFullMarkdown({ title, blocks, viewType }), | ||
| contentType: contentTypes.roamMarkdown, | ||
| scale: "document", | ||
| }, | ||
| }, | ||
| ]; | ||
| }; | ||
| const fullContent = crossAppNodeToDbContent(crossAppNode, "full"); | ||
| return fullContent === undefined ? [] : [fullContent]; | ||
| } catch (error) { | ||
| console.error( | ||
| `convertRoamNodeToFullContent: failed to build full markdown for ${node.source_local_id}:`, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the intent to replace this with CrossAppContent eventually?