ENG-2153 Undecorate Roam node titles into core_title on publish - #1317
ENG-2153 Undecorate Roam node titles into core_title on publish#1317sid597 wants to merge 1 commit into
Conversation
Extract {content} from the page title with the node type's format and
write it as literal_content.core_title in all three concept producers
(publish, periodic sync, full-content). coreTitle is required on
CrossAppNode so a producer omitting it fails to compile: upsert_concepts
replaces literal_content wholesale, so an omitted key would erase
core_title on the next re-upsert. Falls back to the full title when the
type has no format or the title does not match it.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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:
|
| // 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; |
There was a problem hiding this comment.
Required on purpose. upsert_concepts replaces literal_content wholesale and defaults it to {}, so a producer that omits the key erases core_title on the next re-upsert. Required makes "every producer writes it" a compile error.
| author_local_id: node.authorId, | ||
| schema_represented_by_local_id: node.nodeType, | ||
| literal_content: { | ||
| core_title: node.coreTitle, |
There was a problem hiding this comment.
No migration needed: literal_content jsonb already exists (MAP confirmed on the ticket). name and the direct content keep the decorated title; core_title is added next to it.
| import getDiscourseNodes from "./getDiscourseNodes"; | ||
| import extractContentFromTitle from "./extractContentFromTitle"; | ||
|
|
||
| const getCoreTitle = (title: string, nodeTypeUid: string): string => { |
There was a problem hiding this comment.
Callers only carry {uid, type}, so the format lookup lives in the converter. Calling getDiscourseNodes() per node is cheap: the accessor is version-cached.
| 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), |
There was a problem hiding this comment.
Nothing reads this coreTitle yet: this producer feeds upsert_content, not upsert_concepts. It is set so any CrossAppNode that reaches concept conversion carries it (the ticket lists all three producers).
| 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, { |
There was a problem hiding this comment.
Inlined instead of reusing the publish-side helper because allNodeTypes is already in scope; the helper would refetch via getDiscourseNodes(). node_title ?? node.text matters for block-backed types: node_title is the format-matched page title, text is the block string.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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.
🟡 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 (getDiscourseNodes() at apps/roam/src/utils/roamToCrossAppConverters.ts:23) instead of once per batch, so syncing or publishing many nodes repeats the same expensive work thousands of times.
Impact: Large or initial syncs take noticeably longer and can block the UI, and the work is entirely wasted in the full-content path where the extracted title is not used.
Per-node configuration parsing in getCoreTitle
getCoreTitle calls getDiscourseNodes() on every invocation. It is invoked once per node from fullContentNodeToCrossApp (apps/roam/src/utils/roamToCrossAppConverters.ts:84) and inside the nodeRows.map of nodeUidsWithTypeToCrossApp (apps/roam/src/utils/roamToCrossAppConverters.ts:134).
getDiscourseNodes is only cached on the new-settings-store path (apps/roam/src/components/settings/utils/accessors.ts:1153-1157); on the legacy discourseConfigRef path it re-parses every node type's config tree (specification conditions, canvas settings, template) on each call (apps/roam/src/utils/getDiscourseNodes.ts:113-160).
fullContentNodeToCrossApp runs for every node in a sync via convertRoamNodeToFullContent (apps/roam/src/utils/convertRoamNodeToFullContent.ts:20-23, called at apps/roam/src/utils/syncDgNodesToSupabase.ts:805), and that path only uses the full content — the computed coreTitle is discarded.
Compare the approach used in convertDgToSupabaseConcepts, which builds a format lookup map once (apps/roam/src/utils/syncDgNodesToSupabase.ts:671-673).
Prompt for agents
getCoreTitle in apps/roam/src/utils/roamToCrossAppConverters.ts calls getDiscourseNodes() once per node. On the legacy settings-store path getDiscourseNodes re-parses every node type's configuration tree on each call (no cache), and the function is called per node from fullContentNodeToCrossApp and from the nodeRows.map inside nodeUidsWithTypeToCrossApp, so a sync of thousands of nodes repeats the parsing thousands of times. Additionally, the full-content path (convertRoamNodeToFullContent -> crossAppNodeToDbContent(node, 'full')) never reads coreTitle, so that work is wasted there. Consider building a format-by-node-type map once per batch (as convertDgToSupabaseConcepts does) and passing it in, or memoizing the format lookup, and avoid computing coreTitle where it is not consumed.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: e0f31a9870
ℹ️ 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".
| 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.
When a valid formatted title has an empty or whitespace-only {content} capture (for example, [[EVD]] - - @smith2020), this call stores the decorated title instead of an empty core_title. extractContentFromTitle currently evaluates the trimmed capture with || title, so an empty matched value falls through to the original title; the same behavior affects periodic sync. Preserve a successfully matched empty string rather than treating it as no match.
Useful? React with 👍 / 👎.
1 participant
Roam publishes the decorated page title (
[[CLM]] - my claim) as the node's content value, so an importing app receives Roam's title grammar instead of the content. This PR extracts{content}from the title with the node type's format and writes it asliteral_content.core_titlein all three concept producers (publish, periodic sync, full-content). The decorated title stays as the direct content value.If the type has no format, or the title does not match it,
core_titleequals the title.coreTitleis required onCrossAppNode:upsert_conceptsreplacesliteral_contentwholesale, so a producer that omits the key would erasecore_titleon the next sync. Required turns that mistake into a compile error.No DB migration:
literal_contentjsonb already exists (MAP confirmed on the ticket). Tests cover formatted, unformatted, and non-matching titles, a format with{Source}, content containing" - "(non-greedy shortest match, accepted for v0), and the round trip.Backfill of existing rows is ENG-2155. Decorating on import is ENG-2156 (Roam) and ENG-2157 (Obsidian).