Skip to content

ENG-2154 Undecorate Obsidian node titles into core_title on publish #1318

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.

Sign up for GitHub

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

Open
sid597 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ENG-2154 Undecorate Obsidian node titles into core_title on publish #1318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions apps/obsidian/src/utils/conceptConversion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type { DiscourseNodeInVault } from "./getDiscourseNodes";
import type { LocalConceptDataInput } from "@repo/database/inputTypes";
import type { ObsidianDiscourseNodeData } from "./syncDgNodesToSupabase";
import type { Json } from "@repo/database/dbTypes";
import { extractContentFromTitle } from "./extractContentFromTitle";

/**
* Get extra data (author, timestamps) from file metadata
Expand Down Expand Up @@ -160,12 +161,17 @@ export const discourseRelationTripleSchemaToLocalConcept = ({
export const discourseNodeInstanceToLocalConcept = (
context: SupabaseContext,
nodeData: ObsidianDiscourseNodeData,
nodeTypesById: Record<string, DiscourseNode>,

@sid597 sid597 Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

): LocalConceptDataInput => {
Comment on lines 161 to 165

@devin-ai-integration devin-ai-integration Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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).
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

const extraData = getNodeExtraData(nodeData.file, context.userId);
const { nodeInstanceId, nodeTypeId, importedFromRid, ...otherData } =
nodeData.frontmatter;
const literal_content: Record<string, Json> = {
label: nodeData.file.basename,
core_title: extractContentFromTitle(
nodeTypesById[nodeData.nodeTypeId]?.format ?? "",
nodeData.file.basename,
Comment on lines +171 to +173

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Escape literal metacharacters before deriving core titles

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 👍 / 👎.

),
Comment on lines +171 to +174

@devin-ai-integration devin-ai-integration Bot Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +171 to +174

@sid597 sid597 Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

source_data: otherData as unknown as Json,
};
if (importedFromRid && typeof importedFromRid === "string")
Expand Down
2 changes: 1 addition & 1 deletion apps/obsidian/src/utils/syncDgNodesToSupabase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ const convertDgToSupabaseConcepts = async ({
.filter((n) => !!n);

const nodeInstanceToLocalConcepts = nodesSince.map((node) => {
return discourseNodeInstanceToLocalConcept(context, node);
return discourseNodeInstanceToLocalConcept(context, node, nodeTypesById);
});

const relationInstancesData = await loadRelations(plugin);
Expand Down