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. decorateTitle returns null when the format cannot be rebuilt from the core title alone: no {content} placeholder, or a second placeholder such as {Source} that the database has no value for. Roam's shipped Evidence format is that case, so an Evidence note keeps its incoming title rather than landing as [[EVD]] - x - with the source dropped. 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 👍 / 👎.
There was a problem hiding this comment.
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.
|
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.
94ea2de to
907869b
Compare
1 participant
eng-2157.mp4
This is what we are following:
This PR is the bottom path of the diagram: decorate on import, Obsidian.
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), the local type has no format, or the format cannot be rebuilt from the core title alone (no{content}placeholder, or a second placeholder such as{Source}whose value the database does not hold). That last case is why Roam's shipped[[EVD]] - {content} - {Source}keeps the incoming title: substituting the empty string dropped the source from a name that used to carry it and left a dangling separator.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. It returnsnullwhen the format cannot be rebuilt, and the caller falls back. 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(.*?). The placeholder pattern is exported asFORMAT_PLACEHOLDERandgetDiscourseNodeFormatExpressionnow uses it, so decorate and match cannot drift apart.decorateTitleis deterministic and onesanitizedFileNamefeeds both the create path and the rename guard, so once a file carries the decorated name, re-import and refresh leave it alone. Files imported before this change carry the incoming title, so their next refresh renames them once (and rewrites inbound wikilinks); that is the intended migration. 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.Merge order: nothing on
mainwritescore_titleyet (#1317 / #1318 are open). Merged alone, everycoreTitleis undefined and the import keeps today's names; the decoration in the demo needs those producers. The subfolder create-path fix is live regardless.Refactor parity (what moved and what changed on the way):
fetchNodeTypeSchemasForInstances→Map<id, Schema>fetchNodeImportInfoForInstances→Map<id, {schema?, coreTitle?}>schemaIds/ schema-fetch errorschema: undefinedprocessFileContentimportSelectedNodesloopconsole.error+continueinstead of a result unionmapNodeTypeIdToLocalafter the vault writeprocessFileContent→{file} | {error}TFileresult.file!is gonesanitizePathForImport(contentFilePath)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 is on the demo.Known and deferred, each pre-existing and left as it was:
core_titleis a bareliteral_contentkey on both sides:CrossAppNode.coreTitleis the contract field (ENG-2153), the serialized key has no shared constant. The producers live on ENG-2153 Undecorate Roam node titles into core_title on publish #1317/ENG-2154 Undecorate Obsidian node titles into core_title on publish #1318 and this PR is based onmain, so the constant is a follow-up once those land.formatNodeName(local node creation) anddecorateTitle(import) now disagree on multi-placeholder formats; converging them needs a ticket becauseformatNodeName'snulldoubles as form validation.mapNodeTypeIdToLocalruns onemy_conceptsquery per imported node, as it did insideprocessFileContentbefore; batching it is not this ticket.[[CLM]] - {content}format without going throughcheckInvalidChars, so it decorates as[[CLM]] - x.md; the incoming title carried the same brackets before this change.