-
Notifications
You must be signed in to change notification settings - Fork 7
ENG-2153 Undecorate Roam node titles into core_title on publish #1317
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
base: main
Are you sure you want to change the base?
Changes from all commits
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,38 @@ | ||
| import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
| import type { SupabaseContext } from "~/utils/supabaseContext"; | ||
|
|
||
| vi.mock("~/utils/getBlockProps", () => ({ default: () => ({}) })); | ||
| vi.mock("~/utils/getDiscourseNodes", () => ({ default: () => [] })); | ||
| vi.mock("~/utils/getDiscourseRelations", () => ({ default: () => [] })); | ||
| vi.mock("~/utils/createReifiedBlock", () => ({ | ||
| DISCOURSE_GRAPH_PROP_NAME: "discourse-graph", | ||
| })); | ||
| vi.mock("roamjs-components/queries/getPageTitleByPageUid", () => ({ | ||
| default: () => "", | ||
| })); | ||
|
|
||
| import { discourseNodeBlockToLocalConcept } from "~/utils/conceptConversion"; | ||
|
|
||
| const context = { spaceId: 42 } as SupabaseContext; | ||
|
|
||
| describe("discourseNodeBlockToLocalConcept", () => { | ||
| beforeEach(() => { | ||
| (globalThis as { window: unknown }).window = { | ||
| roamAlphaAPI: { | ||
| q: () => [["author-1", "page-1", 1000, 2000]], | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| it("writes the core title into literal_content", () => { | ||
| const concept = discourseNodeBlockToLocalConcept(context, { | ||
| nodeUid: "node-1", | ||
| schemaUid: "schema-1", | ||
| text: "CLM - my claim", | ||
| coreTitle: "my claim", | ||
| }); | ||
| expect(concept.literal_content).toEqual({ core_title: "my claim" }); | ||
| expect(concept.name).toBe("CLM - my claim"); | ||
| expect(concept.source_local_id).toBe("node-1"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import extractContentFromTitle from "~/utils/extractContentFromTitle"; | ||
|
|
||
| describe("extractContentFromTitle", () => { | ||
| it("extracts the content from a title matching the format", () => { | ||
| expect( | ||
| extractContentFromTitle("[[CLM]] - my claim", { | ||
| format: "[[CLM]] - {content}", | ||
| }), | ||
| ).toBe("my claim"); | ||
| }); | ||
|
|
||
| it("returns the title when the type has no format", () => { | ||
| expect(extractContentFromTitle("my claim", { format: "" })).toBe( | ||
| "my claim", | ||
| ); | ||
| }); | ||
|
|
||
| it("returns the title when it does not match the format", () => { | ||
| expect( | ||
| extractContentFromTitle("random page", { | ||
| format: "[[CLM]] - {content}", | ||
| }), | ||
| ).toBe("random page"); | ||
| }); | ||
|
|
||
| it("extracts the content from a format with a {Source} placeholder", () => { | ||
| expect( | ||
| extractContentFromTitle("[[EVD]] - finding - @smith2020", { | ||
| format: "[[EVD]] - {content} - {Source}", | ||
| }), | ||
| ).toBe("finding"); | ||
| }); | ||
|
|
||
| it('keeps a trailing content containing " - " whole', () => { | ||
| expect( | ||
| extractContentFromTitle("[[CLM]] - a - b", { | ||
| format: "[[CLM]] - {content}", | ||
| }), | ||
| ).toBe("a - b"); | ||
| }); | ||
|
|
||
| it('extracts the shortest match when the content contains " - " before another placeholder (accepted for v0)', () => { | ||
| expect( | ||
| extractContentFromTitle("[[EVD]] - a - b - @smith2020", { | ||
| format: "[[EVD]] - {content} - {Source}", | ||
| }), | ||
| ).toBe("a"); | ||
| }); | ||
|
|
||
| it("round trips a title built from the core title", () => { | ||
| const coreTitle = "sleep improves memory"; | ||
| const simpleFormat = "[[CLM]] - {content}"; | ||
| expect( | ||
| extractContentFromTitle(simpleFormat.replace("{content}", coreTitle), { | ||
| format: simpleFormat, | ||
| }), | ||
| ).toBe(coreTitle); | ||
|
|
||
| const sourceFormat = "[[EVD]] - {content} - {Source}"; | ||
| const title = sourceFormat | ||
| .replace("{content}", coreTitle) | ||
| .replace("{Source}", "@smith2020"); | ||
| expect(extractContentFromTitle(title, { format: sourceFormat })).toBe( | ||
| coreTitle, | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,14 @@ import { toMarkdown } from "./pageToMarkdown"; | |
| import getFullTreeByParentUid from "roamjs-components/queries/getFullTreeByParentUid"; | ||
| import getPageViewType from "roamjs-components/queries/getPageViewType"; | ||
| import { contentTypes } from "@repo/content-model"; | ||
| import getDiscourseNodes from "./getDiscourseNodes"; | ||
| import extractContentFromTitle from "./extractContentFromTitle"; | ||
|
|
||
| const getCoreTitle = (title: string, nodeTypeUid: string): string => { | ||
| const format = | ||
| getDiscourseNodes().find((node) => node.type === nodeTypeUid)?.format ?? ""; | ||
| return extractContentFromTitle(title, { format }); | ||
|
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 a valid formatted title has an empty or whitespace-only Useful? React with 👍 / 👎. |
||
| }; | ||
|
Comment on lines
+21
to
+25
Contributor
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. 🟡 Node type settings are re-read for every node during sync and publish, slowing large syncs The node type configuration is re-read from Roam for each node being converted ( Per-node configuration parsing in getCoreTitle
Compare the approach used in Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| const FULL_MARKDOWN_OPTS = { | ||
| refs: true, | ||
|
|
@@ -73,6 +81,7 @@ export const fullContentNodeToCrossApp = ( | |
| createdAt: new Date(node.created || Date.now()), | ||
| modifiedAt: new Date(node.last_modified || Date.now()), | ||
| nodeType: node.node_type_id, | ||
| coreTitle: getCoreTitle(title, node.node_type_id), | ||
|
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. Nothing reads this |
||
| content: { | ||
| direct: { | ||
| localId: node.source_local_id, | ||
|
|
@@ -122,6 +131,7 @@ export const nodeUidsWithTypeToCrossApp = async ( | |
| authorId: userUid, | ||
| createdAt: new Date(createdTime), | ||
| modifiedAt: new Date(Math.max(editTime, pageEditTime)), | ||
| coreTitle: getCoreTitle(title, typesByUid[uid]), | ||
| content: { | ||
| direct: { | ||
| localId: uid, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { | |
| nodeTypeSince, | ||
| } from "./getAllDiscourseNodesSince"; | ||
| import getDiscourseNodeFormatExpression from "./getDiscourseNodeFormatExpression"; | ||
| import extractContentFromTitle from "./extractContentFromTitle"; | ||
| import { cleanupOrphanedNodes } from "./cleanupOrphanedNodes"; | ||
| import { | ||
| getLoggedInClient, | ||
|
|
@@ -667,11 +668,17 @@ export const convertDgToSupabaseConcepts = async ({ | |
| return discourseNodeSchemaToLocalConcept(context, node); | ||
| }); | ||
|
|
||
| const formatByNodeTypeUid = new Map( | ||
| allNodeTypes.map((nodeType) => [nodeType.type, nodeType.format]), | ||
| ); | ||
| const nodeBlockToLocalConcepts = nodesSince.map((node) => { | ||
| const localConcept = discourseNodeBlockToLocalConcept(context, { | ||
| nodeUid: node.source_local_id, | ||
| schemaUid: node.type, | ||
| text: node.node_title ? `${node.node_title} ${node.text}` : node.text, | ||
| coreTitle: extractContentFromTitle(node.node_title ?? node.text, { | ||
|
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. Inlined instead of reusing the publish-side helper because |
||
| format: formatByNodeTypeUid.get(node.type) ?? "", | ||
| }), | ||
| }); | ||
| return localConcept; | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,6 +74,10 @@ type InlineCrossAppTypedContent = InlineCrossAppContent & { | |
| // A node instance | ||
| export type CrossAppNode = CrossAppBase & { | ||
| nodeType: LocalId; | ||
| // The title stripped of the node type's title format ("[[CLM]] - {content}" | ||
| // -> the {content} part). Equals the title when the type has no format or | ||
| // the title does not match it. | ||
| coreTitle: string; | ||
|
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. Required on purpose. |
||
| content: { | ||
| direct: InlineCrossAppContent; | ||
| full?: InlineCrossAppTypedContent; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,9 @@ export const crossAppNodeToDbConcept = ( | |
| name: node.content.direct.value, | ||
| author_local_id: node.authorId, | ||
| schema_represented_by_local_id: node.nodeType, | ||
| literal_content: { | ||
| core_title: node.coreTitle, | ||
|
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 migration needed: |
||
| }, | ||
| contents_inline: filterUndefinedArray([ | ||
| crossAppNodeToDbContent(node, "direct"), | ||
| crossAppNodeToDbContent(node, "full"), | ||
|
|
||
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.
Callers only carry
{uid, type}, so the format lookup lives in the converter. CallinggetDiscourseNodes()per node is cheap: the accessor is version-cached.