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 2 commits into
Conversation
|
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. 1 Skipped Deployment
|
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.
| 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}
![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".
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.
e0f31a9 to
906ad40
Compare
A matched-but-empty {content} capture now yields "" instead of falling
back to the decorated title, so the decorate/undecorate round trip holds
for empty content. Build the node-type format map once per batch instead
of calling getDiscourseNodes() per node, mirroring the sync path's
formatByNodeTypeUid.
| 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), |
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).
|
Both bot findings addressed in 7d91389: a matched-but-empty |
1 participant
eng-2153.mp4
This is what we are following:
This PR is the top path of the diagram: undecorate on publish, Roam.
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).
From review of #1318:
getDiscourseNodeFormatExpressionescapes only[ ] ? . +, so a format containing other regex metacharacters (e.g. parens) breaks the placeholder-to-capture-group mapping this extraction relies on; those titles fall back to the decorated title incore_title. Cross-app fix (both builders plus placeholder-grammar unification) is tracked in ENG-2176.