-
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
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,76 @@ | ||
| 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("preserves an empty content capture instead of falling back to the title", () => { | ||
| expect( | ||
| extractContentFromTitle("[[EVD]] - - @smith2020", { | ||
| format: "[[EVD]] - {content} - {Source}", | ||
| }), | ||
| ).toBe(""); | ||
| }); | ||
|
|
||
| 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,20 @@ 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"; | ||
|
|
||
| export const getFormatByNodeTypeUid = (): Map<string, string> => | ||
| new Map(getDiscourseNodes().map((node) => [node.type, node.format])); | ||
|
|
||
| const getCoreTitle = ( | ||
| title: string, | ||
| nodeTypeUid: string, | ||
| formatByNodeTypeUid: Map<string, string>, | ||
| ): string => | ||
| extractContentFromTitle(title, { | ||
| format: formatByNodeTypeUid.get(nodeTypeUid) ?? "", | ||
| }); | ||
|
|
||
| const FULL_MARKDOWN_OPTS = { | ||
| refs: true, | ||
|
|
@@ -64,6 +78,7 @@ const buildFullInlineContent = ({ | |
|
|
||
| export const fullContentNodeToCrossApp = ( | ||
| node: RoamFullContentNode, | ||
| formatByNodeTypeUid: Map<string, string>, | ||
| ): CrossAppNode => { | ||
| const title = node.node_title ?? node.text; | ||
|
|
||
|
|
@@ -73,6 +88,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, formatByNodeTypeUid), | ||
|
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, | ||
|
|
@@ -106,6 +122,7 @@ export const nodeUidsWithTypeToCrossApp = async ( | |
| const userUidByEid = Object.fromEntries( | ||
| userRows.map((r) => [r[":db/id"] as number, r[":user/uid"] as string]), | ||
| ); | ||
| const formatByNodeTypeUid = getFormatByNodeTypeUid(); | ||
| const results = nodeRows.map((row) => { | ||
| const uid = row[":block/uid"] as string; | ||
| const title = row[":node/title"] as string; | ||
|
|
@@ -122,6 +139,7 @@ export const nodeUidsWithTypeToCrossApp = async ( | |
| authorId: userUid, | ||
| createdAt: new Date(createdTime), | ||
| modifiedAt: new Date(Math.max(editTime, pageEditTime)), | ||
| coreTitle: getCoreTitle(title, typesByUid[uid], formatByNodeTypeUid), | ||
| 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; | ||
| }); | ||
|
|
||
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.
getFormatByNodeTypeUid()is a second lookup. These nodes are built ingetSharedRoamNodesWithFullContentUpdatesSince, andmatchingNodeType.formatis already there when you setnode_type_id.Could we put
formatonRoamFullContentNodeat that construction site and skip the map here?