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 1 commit 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
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| export const discourseNodeInstanceToLocalConcept = ( | ||
| context: SupabaseContext, | ||
| nodeData: ObsidianDiscourseNodeData, | ||
| nodeTypesById: Record<string, DiscourseNode>, | ||
| ): LocalConceptDataInput => { |
There was a problem hiding this comment.
🟡 New third argument added positionally instead of using named parameters as the repo style guide requires
The node-type lookup map is passed as a third positional argument (discourseNodeInstanceToLocalConcept(context, node, nodeTypesById) at apps/obsidian/src/utils/syncDgNodesToSupabase.ts:597), even though the project rules require object-style named parameters once a function takes more than two inputs, so future callers can easily mix up the order.
Impact: Callers can silently pass arguments in the wrong order, and the code diverges from the project's stated style.
Style rule and sibling-function precedent
AGENTS.md (TypeScript Guidelines): "Use named parameters (object destructuring) when a function has more than 2 parameters". discourseNodeInstanceToLocalConcept now has three positional parameters (apps/obsidian/src/utils/conceptConversion.ts:161-165). The sibling converters the author says this mirrors (discourseRelationTripleSchemaToLocalConcept at apps/obsidian/src/utils/conceptConversion.ts:109-118, relationInstanceToLocalConcept at apps/obsidian/src/utils/conceptConversion.ts:190-200) all use a single destructured object.
Prompt for agents
Refactor discourseNodeInstanceToLocalConcept in apps/obsidian/src/utils/conceptConversion.ts to take a single destructured object parameter ({ context, nodeData, nodeTypesById }) instead of three positional parameters, matching discourseRelationTripleSchemaToLocalConcept and relationInstanceToLocalConcept in the same file and the AGENTS.md rule about named parameters for functions with more than two parameters. Update the single call site in apps/obsidian/src/utils/syncDgNodesToSupabase.ts (~line 597).
Was this helpful? React with 👍 or 👎 to provide feedback.
| core_title: extractContentFromTitle( | ||
| nodeTypesById[nodeData.nodeTypeId]?.format ?? "", | ||
| nodeData.file.basename, | ||
| ), |
There was a problem hiding this comment.
🟡 New title-extraction behavior ships without any unit tests, contrary to repository rules
The new derivation of the published title field is added (extractContentFromTitle call at apps/obsidian/src/utils/conceptConversion.ts:171-174) without accompanying unit tests, which the repository rules require for new functionality.
Impact: Regressions in how published titles are derived from file names would go undetected.
Rule reference
AGENTS.md Testing section: "Write unit tests for new functionality" / "Ensure tests are meaningful and maintainable"; CONTRIBUTING.md step 4: "Write tests that validate your change and/or fix." The PR description acknowledges no tests were added because apps/obsidian lacks test infrastructure.
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: 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".
| core_title: extractContentFromTitle( | ||
| nodeTypesById[nodeData.nodeTypeId]?.format ?? "", | ||
| nodeData.file.basename, |
There was a problem hiding this comment.
For node formats containing regex metacharacters that the settings validator permits, this publishes an incorrect core_title because extractContentFromTitle passes the format through getDiscourseNodeFormatExpression, which does not escape characters such as parentheses or *. For example, the valid format Claim (draft) - {content} and basename Claim (draft) - A claim do not match the generated regex, so the fallback stores the entire decorated basename instead of A claim; escape all literal portions of the format before using this extractor for published data.
Useful? React with 👍 / 👎.
| 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.
1 participant
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, so the ticket's test bullet is deferred.
Known limitation, accepted in the ticket:
extractContentFromTitlereturns the first capture group, so a format where{content}is not the first placeholder extracts the wrong text. Obsidian's validation requires{content}and formats areXXX - {content}in practice.