ENG-2154 Undecorate Obsidian node titles into core_title on publish - #1318
ENG-2154 Undecorate Obsidian node titles into core_title on publish#1318sid597 wants to merge 5 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.
|
![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: 88e00370ee
ℹ️ 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".
| export const discourseNodeInstanceToLocalConcept = ( | ||
| context: SupabaseContext, | ||
| nodeData: ObsidianDiscourseNodeData, | ||
| nodeTypesById: Record<string, DiscourseNode>, |
There was a problem hiding this comment.
The converter only had the type id, and the format lives on the type. The single caller already builds this map for discourseRelationTripleSchemaToLocalConcept, so it comes in the same way.
| core_title: extractContentFromTitle( | ||
| nodeTypesById[nodeData.nodeTypeId]?.format ?? "", | ||
| nodeData.file.basename, | ||
| ), |
There was a problem hiding this comment.
extractContentFromTitle already returns the title when the format is empty or does not match, which is the fallback ENG-2153 specifies. The ?? "" covers a file whose nodeTypeId no longer matches a configured type.
The write is unconditional on purpose: upsert_concepts replaces literal_content wholesale, so a conditional write would let a later sync erase the key.
… unused accountLocalId param
| const contentIndex = placeholders.findIndex( | ||
| (name) => name.toLowerCase() === "content", | ||
| ); | ||
| const capture = contentIndex >= 0 ? match[contentIndex + 1] : match[1]; |
There was a problem hiding this comment.
getDiscourseNodeFormatExpression turns every {[a-zA-Z]+} placeholder into one (.*?) group, in order, and the scan above uses the same pattern, so the capture index is the placeholder index + 1. A matched-but-empty capture stays "" on purpose: the old || title fallback wrote the decorated basename as core_title, a non-null value ENG-2155's backfill probe would treat as already migrated. Formats with metacharacters the expression builder does not escape still fall back to the basename — ENG-2176.
…bsidian-node-titles-into-core_title-on-publish
| const literal_content: Record<string, Json> = { | ||
| label: nodeData.file.basename, | ||
| core_title: extractContentFromTitle( | ||
| nodeTypesById[nodeData.nodeTypeId]?.format ?? "", |
There was a problem hiding this comment.
nodeData.nodeTypeId is the typed copy of frontmatter.nodeTypeId (same value; getDiscourseNodes copies it out). The destructured nodeTypeId on line 171 exists to strip the key from otherData before it becomes source_data, and line 187 keeps using it with the pre-existing as string — switching that to nodeData.nodeTypeId would drop an assertion, left as is to avoid churn on a PR under review.
1 participant
eng-2154.mp4
This is what we are following:
This PR is the top path of the diagram: undecorate on publish, Obsidian.
Obsidian publishes the decorated file name, so importers receive Obsidian's title grammar. This writes the undecorated content as
literal_content.core_titleon publish (contract from ENG-2153).discourseNodeInstanceToLocalConceptextracts{content}from the file basename with the node type's format and writes it ascore_title. The existing keys stay.core_titleequal to the basename. Importers fall back to the direct content text when the key is absent, so old rows stay valid. Backfill is ENG-2155.upsert_conceptsreplacesliteral_contentwholesale. Publish and periodic sync both flow through this one converter.No tests: apps/obsidian has no test infrastructure and we decided not to add it in this PR; ENG-2176 moves the extractor where a test runner exists.
From review:
extractContentFromTitlenow returns the{content}capture by placeholder position and preserves a matched-but-empty capture as"". The format's placeholders map 1:1 to the expression's capture groups, so the capture index is the placeholder index. This replaces the ticket's accepted first-capture limitation: ENG-2155's backfill treats any non-nullcore_titleas already set, so a wrong value written before a later fix would never be repaired — cheaper to fix before rows are written. Same extraction rule as the Roam side (ENG-2153 Undecorate Roam node titles into core_title on publish #1317), with one asymmetry tracked in ENG-2176: Roam's placeholder scan ({[\w\d-]+}) is wider than its builder's grammar, Obsidian's matches its builder exactly.BulkImportCandidate.extractedContent, which nothing reads.discourseNodeInstanceToLocalConcepttakes named parameters, matching the sibling converters in the file and the AGENTS.md rule.accountLocalIdparameter ofconvertDgToSupabaseConceptsis removed — pre-existing on main, removed here because this PR touches the signature andeslint --max-warnings 0flags the file.DiscourseContextView's inline copy — which can render a different Content string than what gets published) into a shared, testable package.Notes for review:
{content}publishes the first placeholder's capture (format validation only requires some{var}) — same rule as Roam.core_titlerecomputes when a file's title changes and on full sync. A format edit alone does not rewrite existing rows: it also does not rename existing files, so stored values stay consistent with the basenames they came from.labelstays alongsidecore_title:labelis the decorated basename,core_titlethe undecorated content — an importer cannot undecorate a foreign app's title grammar, so both cross the boundary.