-
Notifications
You must be signed in to change notification settings - Fork 7
ENG-1867 Import cross-app relations into Roam as tentative relations #1302
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
The head ref may contain hidden characters: "eng-1867-Import-cross-app\u2013relations-in-roam-as-tentative"
Changes from all commits
204d0e1
1d5da16
b09d310
477adf2
6413bc1
7b1fa33
d0dcb0f
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 |
|---|---|---|
|
|
@@ -10,12 +10,15 @@ import { getSubTree } from "roamjs-components/util"; | |
| import getSettingValueFromTree from "roamjs-components/util/getSettingValueFromTree"; | ||
| import internalError from "~/utils/internalError"; | ||
| import { getSetting } from "~/utils/extensionSettings"; | ||
| import { getRoamMarkdownApi } from "~/utils/materializeSharedNode"; | ||
|
|
||
| import type { RoamBasicNode } from "roamjs-components/types"; | ||
| import discourseConfigRef from "~/utils/discourseConfigRef"; | ||
| import { roamNodeToCondition } from "~/utils/parseQuery"; | ||
| import type { DiscourseRelation } from "~/utils/getDiscourseRelations"; | ||
| import type { DiscourseNode } from "~/utils/getDiscourseNodes"; | ||
| import getDiscourseNodes, { | ||
| type DiscourseNode, | ||
| } from "~/utils/getDiscourseNodes"; | ||
| import type { Condition } from "~/utils/types"; | ||
| import { z } from "zod"; | ||
| import { | ||
|
|
@@ -266,7 +269,6 @@ const getLegacyPersonalLeftSidebarSetting = (): unknown[] => { | |
| "Result-limit": section.settings?.resultLimit?.value ?? 0, | ||
| }, | ||
| })); | ||
| /* eslint-enable @typescript-eslint/naming-convention */ | ||
| }; | ||
|
|
||
| const getLegacyPersonalSetting = (keys: string[]): unknown => { | ||
|
|
@@ -533,7 +535,7 @@ const getLegacyDiscourseNodeSetting = ( | |
| "key-image-option": rawCanvas["key-image-option"] || "first-image", | ||
| "query-builder-alias": rawCanvas["query-builder-alias"] || "", | ||
| }; | ||
| /* eslint-enable @typescript-eslint/naming-convention */ | ||
|
|
||
| const attributes = Object.fromEntries( | ||
| getSubTree({ tree, key: "Attributes" }).children.map((c) => [ | ||
| c.text, | ||
|
|
@@ -717,7 +719,6 @@ const FEATURE_FLAG_LEGACY_MAP: Record< | |
| text: "(BETA) Left Sidebar", | ||
| }).value, | ||
| }; | ||
| /* eslint-enable @typescript-eslint/naming-convention */ | ||
|
|
||
| export const getFeatureFlag = (key: keyof FeatureFlags): boolean => { | ||
| return bulkReadSettings().featureFlags[key]; | ||
|
|
@@ -954,7 +955,7 @@ const getRawDiscourseNodeBlockProps = ( | |
| } | ||
|
|
||
| return isRecord(blockProps) && Object.keys(blockProps).length > 0 | ||
| ? (blockProps as Record<string, json>) | ||
| ? blockProps | ||
| : undefined; | ||
| }; | ||
|
|
||
|
|
@@ -1058,7 +1059,7 @@ const addConditionUids = (conditions: SchemaCondition[]): Condition[] => | |
| target: c.target, | ||
| not: c.not, | ||
| }; | ||
| }) as Condition[]; | ||
| }); | ||
|
|
||
| const toDiscourseNode = (settings: DiscourseNodeSettings): DiscourseNode => ({ | ||
| text: settings.text, | ||
|
|
@@ -1085,34 +1086,74 @@ const toDiscourseNode = (settings: DiscourseNodeSettings): DiscourseNode => ({ | |
| : undefined, | ||
| }); | ||
|
|
||
| const getUnusedShortcut = (label: string): string => { | ||
| const candidateShortcut = label.slice(0, 1).toUpperCase(); | ||
| const existingShortcuts = new Set( | ||
| getDiscourseNodes() | ||
| .map((n) => n.shortcut.toUpperCase()) | ||
| .filter(Boolean), | ||
| ); | ||
| return existingShortcuts.has(candidateShortcut) ? "" : candidateShortcut; | ||
| }; | ||
|
|
||
| // getAllDiscourseNodes skips prop-less pages, so invalidate only after the props write settles. | ||
| export const createDiscourseNodeType = async ({ | ||
| text, | ||
| label, | ||
| shortcut, | ||
| format, | ||
| template, | ||
| }: { | ||
| text: string; | ||
| shortcut: string; | ||
| format: string; | ||
| label: string; | ||
| shortcut?: string; | ||
| format?: string; | ||
| template?: string; | ||
| }): Promise<DiscourseNode> => { | ||
| if (shortcut === undefined) shortcut = getUnusedShortcut(label); | ||
| format = format ?? `[[${label.slice(0, 3).toUpperCase()}]] - {content}`; | ||
| const tree = [ | ||
| { | ||
| text: "Shortcut", | ||
| children: [{ text: shortcut }], | ||
| }, | ||
| { | ||
| text: "Tag", | ||
| children: [{ text: "" }], | ||
| }, | ||
| { | ||
| text: "Format", | ||
| children: [{ text: format }], | ||
| }, | ||
| ]; | ||
| if (template !== undefined) { | ||
| tree.push({ | ||
| text: "Template", | ||
| children: [], | ||
| }); | ||
| } | ||
| const pageUid = await createPage({ | ||
| title: `${DISCOURSE_NODE_PAGE_PREFIX}${text}`, | ||
| tree: [ | ||
| { text: "Shortcut", children: [{ text: shortcut }] }, | ||
| { text: "Tag", children: [{ text: "" }] }, | ||
| { text: "Format", children: [{ text: format }] }, | ||
| ], | ||
| title: `discourse-graph/nodes/${label}`, | ||
| tree, | ||
| }); | ||
|
|
||
| let templateTree: RoamBasicNode[] | undefined; | ||
| if (template !== undefined) { | ||
| const tree = getBasicTreeByParentUid(pageUid); | ||
| const templateUid = tree[3].uid; | ||
|
Member
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.
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: |
||
| await getRoamMarkdownApi().block.fromMarkdown({ | ||
| location: { "parent-uid": templateUid, order: "last" }, | ||
| "markdown-string": template, | ||
| }); | ||
| templateTree = getBasicTreeByParentUid(templateUid); | ||
| } | ||
| const settings = DiscourseNodeSchema.parse({ | ||
| text, | ||
| text: label, | ||
| type: pageUid, | ||
| shortcut, | ||
| format, | ||
| template: templateTree, | ||
| }); | ||
| setBlockProps(pageUid, settings); | ||
| await setBlockPropsAsync(pageUid, settings); | ||
| invalidateDiscourseNodeTypeCaches(); | ||
|
|
||
| return toDiscourseNode(settings); | ||
| }; | ||
|
|
||
|
|
@@ -1224,9 +1265,7 @@ export const getAllDiscourseNodes = (): DiscourseNode[] => { | |
| ); | ||
| } else { | ||
| // Try migrating legacy field shapes before dropping the node. | ||
| const migrated = migrateNodeBlockProps( | ||
| blockProps as Record<string, json>, | ||
| ); | ||
| const migrated = migrateNodeBlockProps(blockProps); | ||
| const retryResult = DiscourseNodeSchema.safeParse(migrated); | ||
| if (retryResult.success) { | ||
| setBlockProps(pageUid, retryResult.data, false); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,17 +136,21 @@ export const createReifiedRelation = async ({ | |
| sourceUid, | ||
| relationBlockUid, | ||
| destinationUid, | ||
| tentative, | ||
| }: { | ||
| sourceUid: string; | ||
| relationBlockUid: string; | ||
| destinationUid: string; | ||
| }): Promise<string | undefined> => { | ||
| tentative?: boolean; | ||
|
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. adding the tentative flag |
||
| }): Promise<string> => { | ||
| const parameterUids: Record<string, string> = { | ||
| sourceUid, | ||
| destinationUid, | ||
| ...(tentative !== undefined && { tentative: String(tentative) }), | ||
| }; | ||
| return await createReifiedBlock({ | ||
| destinationBlockUid: await getOrCreateRelationPageUid(), | ||
| schemaUid: relationBlockUid, | ||
| parameterUids: { | ||
| sourceUid, | ||
| destinationUid, | ||
| }, | ||
| parameterUids, | ||
| }); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import discourseConfigRef from "~/utils/discourseConfigRef"; | ||
| import createBlock from "roamjs-components/writes/createBlock"; | ||
| import { setGlobalSetting } from "~/components/settings/utils/accessors"; | ||
| import { GLOBAL_KEYS } from "~/components/settings/utils/settingKeys"; | ||
|
|
||
| export const createRelationSchema = async ({ | ||
| label, | ||
| complement, | ||
| source, | ||
| destination, | ||
| }: { | ||
| label: string; | ||
| complement: string; | ||
| source: string; | ||
| destination: string; | ||
| }) => { | ||
| const grammarNode = discourseConfigRef.tree.find( | ||
| (node) => node.text === "grammar", | ||
| ); | ||
| const relationsNode = grammarNode?.children.find( | ||
| (node) => node.text === "relations", | ||
| ); | ||
| if (!relationsNode) throw new Error("Cannot find the relation grammar"); | ||
| const blockUid = await createBlock({ | ||
| parentUid: relationsNode.uid, | ||
|
maparent marked this conversation as resolved.
|
||
| order: "last", | ||
| node: { | ||
| text: label, | ||
| children: [ | ||
| { | ||
| text: "source", | ||
| children: [{ text: source }], | ||
| }, | ||
| { | ||
| text: "destination", | ||
| children: [{ text: destination }], | ||
| }, | ||
| { | ||
| text: "complement", | ||
| children: [{ text: complement }], | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
| setGlobalSetting([GLOBAL_KEYS.relations, blockUid], { | ||
| label, | ||
| source, | ||
| destination, | ||
| complement, | ||
| ifConditions: [], | ||
|
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.
When Useful? React with 👍 / 👎.
Member
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. |
||
| }); | ||
| return blockUid; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { | |
| type SettingsSnapshot, | ||
| } from "~/components/settings/utils/accessors"; | ||
| import discourseConfigRef from "./discourseConfigRef"; | ||
| import { getStoredRelationsEnabled } from "~/utils/storedRelations"; | ||
|
|
||
| export type Triple = readonly [string, string, string]; | ||
| export type DiscourseRelation = { | ||
|
|
@@ -47,6 +48,7 @@ const getDiscourseRelations = (snapshot?: SettingsSnapshot) => { | |
| const grammarNode = getGrammarNode(); | ||
| const relationsNode = getRelationsNode(grammarNode); | ||
| const relationNodes = relationsNode?.children || DEFAULT_RELATION_VALUES; | ||
| const storedRelationsEnabled = getStoredRelationsEnabled(); | ||
| const discourseRelations = relationNodes.flatMap( | ||
| (r: InputTextNode, i: number) => { | ||
| const tree = (r?.children || []) as TextNode[]; | ||
|
|
@@ -58,6 +60,9 @@ const getDiscourseRelations = (snapshot?: SettingsSnapshot) => { | |
| complement: getSettingValueFromTree({ tree, key: "Complement" }), | ||
| }; | ||
| const ifNode = tree.find(matchNodeText("if"))?.children || []; | ||
| if (ifNode.length === 0 && storedRelationsEnabled) { | ||
|
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. No triples on a new-style relation created from scratch |
||
| return [{ ...data, triples: [] }]; | ||
| } | ||
| return ifNode.map((node) => ({ | ||
| ...data, | ||
| triples: node.children | ||
|
|
||
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.
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 inaccessors.ts, so the context matters.