ENG-2157 Decorate imported node titles in Obsidian from core_title - #1330
ENG-2157 Decorate imported node titles in Obsidian from core_title#1330sid597 wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Annotations on the non-obvious decisions in this diff.
| }; | ||
|
|
||
| export const fetchNodeTypeSchemasForInstances = async ({ | ||
| type NodeInstanceImportInfo = { |
There was a problem hiding this comment.
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.
| .from("my_concepts") | ||
| .select("source_local_id, schema_id") | ||
| .select( | ||
| "source_local_id, schema_id, core_title:literal_content->>core_title", |
There was a problem hiding this comment.
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.
| nodeTypeId: row.source_local_id, | ||
| name: row.name, | ||
| }); | ||
| if (schemaIds.length > 0) { |
There was a problem hiding this comment.
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 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, { |
There was a problem hiding this comment.
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.
| result.set(row.source_local_id, { | ||
| schema: | ||
| row.schema_id === null ? undefined : schemasById.get(row.schema_id), | ||
| coreTitle: row.core_title ?? undefined, |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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.
| continue; | ||
| } | ||
|
|
||
| const mappedNodeTypeId = await mapNodeTypeIdToLocal({ |
There was a problem hiding this comment.
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.
|
|
||
| const localNodeType = getNodeTypeById(plugin, mappedNodeTypeId); | ||
| const coreTitle = nodeImportInfo?.coreTitle; | ||
| const titleForFileName = |
There was a problem hiding this comment.
The decoration. We rebuild the title from core_title using the local type's format, so a Roam-origin [[CLM]] - x lands as whatever this vault calls a claim. It falls back to the incoming title when there's no core_title or the local type has no format. decorateTitle is pure, so a re-import or a refresh computes the same name and the rename guard below leaves the file alone.
| contentFilePath && contentFilePath.includes("/") | ||
| ? sanitizePathForImport(contentFilePath) | ||
| : `${sanitizedFileName}.md`; | ||
| ? sanitizePathForImport(contentFilePath.replace(/\/[^/]*$/, "")) |
There was a problem hiding this comment.
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.
|
|
||
| for (const { nodeTypeId, name } of nodeTypeSchemasByInstance.values()) { | ||
| for (const { schema } of nodeImportInfoByInstance.values()) { | ||
| if (!schema) continue; |
There was a problem hiding this comment.
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.
![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: c2b5f237de
ℹ️ 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".
| const pathUnderImport = sourceFolder | ||
| ? `${sourceFolder}/${sanitizedFileName}.md` | ||
| : `${sanitizedFileName}.md`; |
There was a problem hiding this comment.
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 👍 / 👎.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…annot fill
decorateTitle now returns null for formats without a {content} placeholder or
with placeholders such as {Source}: substituting the empty string dropped the
source from a Roam-format Evidence name and produced a title that no longer
matched the format. The Obsidian format-expression helper reuses the shared
placeholder pattern so decorate and match agree.
1 participant
eng-2157.mp4
Obsidian's import used the incoming title verbatim, so a node published from Roam arrived carrying
[[CLM]] -and ignored what the local vault calls that node type. This rebuilds the file name fromcore_titlewith the local node type's format, so the same node lands asCLM - sleep improves memoryin a vault that formats claims that way. Fallback is the incoming title, used whencore_titleis absent (rows published before ENG-2153/2154) or the local type has no format.Three pieces:
importNodes.tsreads the single JSON key by path (core_title:literal_content->>core_title), so the import-preview path that shares it stays free of the payload column. postgrest-js types that projection asstring, but it is null for rows published before ENG-2153, so the?? undefinedis load-bearing.processFileContentinto the per-node loop, because the local format has to be known before the file name is built.processFileContentnow only writes the file and frontmatter. Its only error came from that resolution, so it returns aTFileinstead of a result union.decorateTitle, a pure helper inpackages/database/src/libbecause ENG-2156 needs the same transform in Roam. I didn't reuseformatNodeName: it splits the compiled regex source on the first(.*?), so a multi-placeholder format produces a name ending in a literal(.*?).decorateTitlesubstitutes every placeholder,{content}with the core title and the rest with the empty string.Re-import and refresh don't rename an unchanged file:
decorateTitleis deterministic and onesanitizedFileNamefeeds both the create path and the rename guard. A related fix falls out: Obsidian-origin nodes that lived in a subfolder used to be created at their full source path, skipping the decorated name, and a later refresh would then rename them. The create path now keeps the folders and replaces the last segment.Tests for the helper live in
packages/database(apps/obsidian has no test runner), so the ticket's test bullet is covered for the pure part and theimportNodeswiring is not. That part needs a demo.Known and deferred:
core_titleis still a bareliteral_contentkey with no entry incrossAppContracts.ts(written by ENG-2153/2154, read here). A type created from a Roam schema keeps Roam's[[CLM]] - {content}format unvalidated and decorates as[[CLM]] - x.md; this PR makes that deterministic rather than introducing it.{Source}decorates to the empty string (ENG-2140).