Skip to content

ENG-2157 Decorate imported node titles in Obsidian from core_title #1330

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 3 commits into
base: main
Choose a base branch
from
Open

ENG-2157 Decorate imported node titles in Obsidian from core_title #1330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9c36e4
ENG-2157 Add decorateTitle helper for rebuilding titles from core_title
sid597 Aug 23, 2026
c2b5f23
ENG-2157 Decorate imported node titles in Obsidian from core_title
sid597 Aug 23, 2026
94ea2de
Keep the incoming title when the format has placeholders core_title c…
sid597 Aug 23, 2026
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
4 changes: 3 additions & 1 deletion apps/obsidian/src/utils/getDiscourseNodeFormatExpression.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { FORMAT_PLACEHOLDER } from "@repo/database/lib/decorateTitle";

export const getDiscourseNodeFormatExpression = (format: string) =>
format
? new RegExp(
`^${format
.replace(/(\[|\]|\?|\.|\+)/g, "\\$1")
.replace(/{[a-zA-Z]+}/g, "(.*?)")}$`,
.replace(FORMAT_PLACEHOLDER, "(.*?)")}$`,
"s",
)
: /$^/;
185 changes: 96 additions & 89 deletions apps/obsidian/src/utils/importNodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import {
} from "./importRelations";
import { createTemplateFile } from "./templates";
import { resolveFolderForSpaceUri } from "./importFolderMetadata";
import { getNodeTypeById } from "./typeUtils";
import { decorateTitle } from "@repo/database/lib/decorateTitle";

type PublishedNode = {
source_local_id: string;
Expand Down Expand Up @@ -327,20 +329,27 @@ type NodeTypeSchemaForInstance = {
name: string;
};

export const fetchNodeTypeSchemasForInstances = async ({
type NodeInstanceImportInfo = {

@sid597 sid597 Aug 23, 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.

NodeInstanceImportInfo nests the existing NodeTypeSchemaForInstance instead of flattening its fields. nodeTypeId and name only ever get set together from one schema row, so two independent optional fields would describe a state this code can't produce. The map now holds an entry for every visible instance rather than only the schema-resolved ones. That's what lets a core title reach the caller when the schema lookup comes back empty.

schema?: NodeTypeSchemaForInstance;
coreTitle?: string;
};

export const fetchNodeImportInfoForInstances = async ({
client,
spaceId,
nodeInstanceIds,
}: {
client: DGSupabaseClient;
spaceId: number;
nodeInstanceIds: string[];
}): Promise<Map<string, NodeTypeSchemaForInstance>> => {
const result = new Map<string, NodeTypeSchemaForInstance>();
}): Promise<Map<string, NodeInstanceImportInfo>> => {
const result = new Map<string, NodeInstanceImportInfo>();

const { data: instanceRows, error: instanceError } = await client
.from("my_concepts")
.select("source_local_id, schema_id")
.select(
"source_local_id, schema_id, core_title:literal_content->>core_title",

@sid597 sid597 Aug 23, 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.

This reads the single JSON key by path instead of selecting the whole literal_content column. computeImportPreview shares this query and only wants node type names, so the preview path stays free of the jsonb payload. The key is the bare core_title that ENG-2153 writes.

)
.eq("space_id", spaceId)
.eq("is_schema", false)
.eq("is_relation", false)
Expand All @@ -358,35 +367,42 @@ export const fetchNodeTypeSchemasForInstances = async ({
.filter((id): id is number => id !== null),
),
];
if (schemaIds.length === 0) return result;

const { data: schemaRows, error: schemaError } = await client
.from("my_concepts")
.select("id, source_local_id, name")
.eq("space_id", spaceId)
.eq("is_schema", true)
.eq("is_relation", false)
.in("id", schemaIds);

if (schemaError || !schemaRows) {
console.error("Error fetching node type schemas:", schemaError);
return result;
}

const schemasById = new Map<number, NodeTypeSchemaForInstance>();
for (const row of schemaRows) {
if (row.id !== null && row.source_local_id !== null && row.name !== null) {
schemasById.set(row.id, {
nodeTypeId: row.source_local_id,
name: row.name,
});
if (schemaIds.length > 0) {

@sid597 sid597 Aug 23, 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 schema fetch moved inside an if instead of guarding two early returns. Both former early exits, no schema ids and a failed schema fetch, now fall through to the merge loop so core titles survive them. On a schema error the caller gets titles with no nodeTypeId, which it already guards for.

const { data: schemaRows, error: schemaError } = await client
.from("my_concepts")
.select("id, source_local_id, name")
.eq("space_id", spaceId)
.eq("is_schema", true)
.eq("is_relation", false)
.in("id", schemaIds);

if (schemaError || !schemaRows) {
console.error("Error fetching node type schemas:", schemaError);
} else {
for (const row of schemaRows) {
if (
row.id !== null &&
row.source_local_id !== null &&
row.name !== null
) {
schemasById.set(row.id, {
nodeTypeId: row.source_local_id,
name: row.name,
});
}
}
}
}

for (const row of instanceRows) {
if (row.source_local_id === null || row.schema_id === null) continue;
const schema = schemasById.get(row.schema_id);
if (schema) result.set(row.source_local_id, schema);
if (row.source_local_id === null) continue;
result.set(row.source_local_id, {

@sid597 sid597 Aug 23, 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.

One write per instance instead of two passes over instanceRows. schema is genuinely undefined here when schema_id is null or the schema row isn't visible under RLS.

schema:
row.schema_id === null ? undefined : schemasById.get(row.schema_id),
coreTitle: row.core_title ?? undefined,

@sid597 sid597 Aug 23, 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 ?? undefined looks redundant because postgrest-js types a ->> projection as plain string. It doesn't model a JSON path extraction as nullable, and this one is. The key is absent on every row published before ENG-2153, so Postgres returns NULL and PostgREST sends null. I pinned the declared type down with a temporary Exact<A, B> assertion before relying on it, so the guard stays.

});
}

return result;
Expand Down Expand Up @@ -1159,46 +1175,26 @@ export const mapNodeTypeIdToLocal = async ({

const processFileContent = async ({
plugin,
client,
sourceSpaceId,
sourceSpaceUri,
rawContent,
filePath,
importedCreatedAt,
importedModifiedAt,
authorId,
nodeInstanceId,
nodeTypeIdFromConcept,
nodeTypeId,
}: {
plugin: DiscourseGraphPlugin;
client: DGSupabaseClient;
sourceSpaceId: number;
sourceSpaceUri: string;
rawContent: string;
filePath: string;
importedCreatedAt?: number;
importedModifiedAt?: number;
authorId?: number;
nodeInstanceId: string;
nodeTypeIdFromConcept?: string;
}): Promise<
{ file: TFile; error?: never } | { file?: never; error: string }
> => {
// 1. Parse frontmatter from rawContent (metadataCache is updated async and is
// often empty immediately after create/modify) and resolve the node type
// before any vault write, so a failed lookup leaves existing files untouched.
const { frontmatter } = parseFrontmatter(rawContent);
const sourceNodeTypeId =
typeof frontmatter.nodeTypeId === "string"
? frontmatter.nodeTypeId
: nodeTypeIdFromConcept;
if (sourceNodeTypeId === undefined) {
return {
error: "importedNode missing sourceNodeTypeId",
};
}

// 2. Create or update the file with the fetched content.
nodeTypeId: string;
}): Promise<TFile> => {

@sid597 sid597 Aug 23, 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.

processFileContent returns a TFile instead of a result union. Its only error came from the node type derivation, which moved up to the caller. That left the error branch and the caller's result.file! unreachable, so both are gone.

// Create or update the file with the fetched content.
// On create, set file metadata (ctime/mtime) to original vault dates via vault adapter.
let file: TFile | null = plugin.app.vault.getFileByPath(filePath);
const stat =
Expand All @@ -1214,19 +1210,11 @@ const processFileContent = async ({
await plugin.app.vault.process(file, () => rawContent, stat);
}

const mappedNodeTypeId = await mapNodeTypeIdToLocal({
plugin,
client,
sourceSpaceId,
sourceSpaceUri,
sourceNodeTypeId,
});

await plugin.app.fileManager.processFrontMatter(
file,
(fm) => {
const record = fm as Record<string, unknown>;
record.nodeTypeId = mappedNodeTypeId;
record.nodeTypeId = nodeTypeId;
record.nodeInstanceId = nodeInstanceId;
record.importedFromRid = spaceUriAndLocalIdToRid(
sourceSpaceUri,
Expand All @@ -1239,7 +1227,7 @@ const processFileContent = async ({
stat,
);

return { file };
return file;
};

export const importSelectedNodes = async ({
Expand Down Expand Up @@ -1308,7 +1296,7 @@ export const importSelectedNodes = async ({
spaceName,
});

const nodeTypeSchemasByInstance = await fetchNodeTypeSchemasForInstances({
const nodeImportInfoByInstance = await fetchNodeImportInfoForInstances({
client,
spaceId,
nodeInstanceIds: nodes.map((n) => n.nodeInstanceId),
Expand Down Expand Up @@ -1355,19 +1343,58 @@ export const importSelectedNodes = async ({
const originalNodePath: string | undefined =
contentFilePath ?? node.filePath;

// Sanitize file name
const sanitizedFileName = sanitizeFileName(fileName);
const nodeImportInfo = nodeImportInfoByInstance.get(
node.nodeInstanceId,
);

// Parse frontmatter from content (metadataCache is updated async and is
// often empty immediately after create/modify) and resolve the node type
// before any vault write, so a failed lookup leaves existing files untouched.
const { frontmatter } = parseFrontmatter(content);

@sid597 sid597 Aug 23, 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.

This derivation moved up from processFileContent, and its comment came with it. We parse the raw content rather than metadataCache because the cache is often empty right after a write. Resolving before any vault write still means a failed lookup leaves existing files untouched.

const sourceNodeTypeId =
typeof frontmatter.nodeTypeId === "string"
? frontmatter.nodeTypeId
: nodeImportInfo?.schema?.nodeTypeId;
if (sourceNodeTypeId === undefined) {
console.error(
`Error processing file content for node ${node.nodeInstanceId}:`,
"importedNode missing sourceNodeTypeId",
);
failedCount++;
processedCount++;
onProgress?.(processedCount, totalNodes);
continue;
}

const mappedNodeTypeId = await mapNodeTypeIdToLocal({

@sid597 sid597 Aug 23, 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.

mapNodeTypeIdToLocal now runs before the file is written rather than after. That's the point of the move, since the local node type has to exist before we can read its format. The function itself is unchanged. One consequence worth knowing: it can create a node type as a side effect, so a failure later in the loop can leave a type behind for a node that didn't import.

plugin,
client,
sourceSpaceId: spaceId,
sourceSpaceUri: spaceUri,
sourceNodeTypeId,
});

const localNodeType = getNodeTypeById(plugin, mappedNodeTypeId);
const coreTitle = nodeImportInfo?.coreTitle;
const decoratedTitle =
coreTitle !== undefined && localNodeType
? decorateTitle(localNodeType.format, coreTitle)
: null;
const sanitizedFileName = sanitizeFileName(decoratedTitle ?? fileName);
let finalFilePath: string;

if (existingFile) {
// Update existing file - use its current path
finalFilePath = existingFile.path;
} else {
// Preserve source vault folder structure under import/{vaultName} when we have filePath from Content
const pathUnderImport =
const sourceFolder =
contentFilePath && contentFilePath.includes("/")
? sanitizePathForImport(contentFilePath)
: `${sanitizedFileName}.md`;
? sanitizePathForImport(contentFilePath.replace(/\/[^/]*$/, ""))

@sid597 sid597 Aug 23, 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.

Obsidian-origin nodes that lived in a subfolder used to be created at their full source path, which skipped the decorated name entirely. A later refresh would then compare against the decorated basename and rename the file. Keeping the folders but replacing the last segment makes create and refresh agree.

: "";
const pathUnderImport = sourceFolder
? `${sourceFolder}/${sanitizedFileName}.md`
: `${sanitizedFileName}.md`;
Comment on lines +1395 to +1397

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 23, 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.

P1 Badge Prevent decorated imports from overwriting filename collisions

When two selected nodes in the same source folder produce the same sanitizedFileName—for example, multi-placeholder titles with the same core_title after the other placeholders are erased—this assigns both nodes the same path. Because processFileContent treats any file already at that path as an update, the second import overwrites the first node's content and identity frontmatter while both are reported as successful. Allocate a unique path for new imports, as the existing-file rename path already does.

Useful? React with 👍 / 👎.

@sid597 sid597 Aug 23, 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.

Confirmed, and the case you describe is closed by the placeholder gate in 94ea2de: a format with a placeholder other than {content} no longer decorates, so two nodes can only share a derived name when their incoming titles would have collided too (one placeholder, distinct type prefixes, unique source titles). The create path has never allocated a unique name when the target exists; that gap predates this PR and is listed in the description as deferred rather than fixed here.

finalFilePath = `${importFolderPath}/${pathUnderImport}`;

// Ensure all parent folders exist (e.g. import/VaultName/Discourse Nodes/SubFolder)
Expand All @@ -1380,38 +1407,18 @@ export const importSelectedNodes = async ({
}
}

// Process the file content (maps nodeTypeId, handles frontmatter, stores import timestamps)
// This updates existing file or creates new one
const result = await processFileContent({
const processedFile = await processFileContent({
plugin,
client,
sourceSpaceId: spaceId,
sourceSpaceUri: spaceUri,
rawContent: content,
filePath: finalFilePath,
importedCreatedAt: createdAt,
importedModifiedAt: modifiedAt,
authorId,
nodeInstanceId: node.nodeInstanceId,
nodeTypeIdFromConcept: nodeTypeSchemasByInstance.get(
node.nodeInstanceId,
)?.nodeTypeId,
nodeTypeId: mappedNodeTypeId,
});

if (result.error) {
console.error(
`Error processing file content for node ${node.nodeInstanceId}:`,
result.error,
);
failedCount++;
processedCount++;
onProgress?.(processedCount, totalNodes);
continue;
}

// typescript should not need this assertion?
const processedFile = result.file!;

// Import assets for this node (use originalNodePath so assets go under import/{space}/ relative to note)
const assetImportResult = await importAssetsForNode({
plugin,
Expand Down
9 changes: 6 additions & 3 deletions apps/obsidian/src/utils/importPreview.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
getImportedNodesInfo,
getLocalNodeKeyToEndpointId,
} from "./relationsStore";
import { fetchNodeTypeSchemasForInstances, getSpaceUris } from "./importNodes";
import { fetchNodeImportInfoForInstances, getSpaceUris } from "./importNodes";
import { QueryEngine } from "~/services/QueryEngine";
import {
fetchRelationInstancesFromSpace,
Expand Down Expand Up @@ -82,13 +82,16 @@ export const computeImportPreview = async ({
}

for (const [spaceId, nodes] of nodesBySpace.entries()) {
const nodeTypeSchemasByInstance = await fetchNodeTypeSchemasForInstances({
const nodeImportInfoByInstance = await fetchNodeImportInfoForInstances({
client,
spaceId,
nodeInstanceIds: nodes.map((n) => n.nodeInstanceId),
});

for (const { nodeTypeId, name } of nodeTypeSchemasByInstance.values()) {
for (const { schema } of nodeImportInfoByInstance.values()) {
if (!schema) continue;

@sid597 sid597 Aug 23, 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.

Preview only wants node type names, so it skips entries whose schema didn't resolve. One check instead of two, since nesting makes the co-presence of nodeTypeId and name structural.

const { nodeTypeId, name } = schema;

// Track name for triplet resolution
if (!nodeTypeIdToName.has(nodeTypeId)) {
nodeTypeIdToName.set(nodeTypeId, name);
Expand Down
35 changes: 35 additions & 0 deletions packages/database/src/lib/__tests__/decorateTitle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { describe, expect, it } from "vitest";
import { decorateTitle } from "../decorateTitle";

describe("decorateTitle", () => {
it("substitutes the core title for {content}", () => {
expect(decorateTitle("[[CLM]] - {content}", "sleep improves memory")).toBe(
"[[CLM]] - sleep improves memory",
);
expect(decorateTitle("CLM - {content}", "sleep improves memory")).toBe(
"CLM - sleep improves memory",
);
});

it("matches the content placeholder case-insensitively", () => {
expect(decorateTitle("QUE - {Content}", "why")).toBe("QUE - why");
});

it("returns null for a format with placeholders the core title cannot fill", () => {
expect(
decorateTitle("[[EVD]] - {content} - {Source}", "REM sleep and recall"),
).toBeNull();
});

it("returns null for a format without a content placeholder", () => {
expect(decorateTitle("", "anything")).toBeNull();
expect(decorateTitle("CLM", "anything")).toBeNull();
});

it("keeps a core title that contains the separator or replacement patterns", () => {
expect(decorateTitle("CLM - {content}", "a - b")).toBe("CLM - a - b");
expect(decorateTitle("CLM - {content}", "costs $& more")).toBe(
"CLM - costs $& more",
);
});
});
Loading