ENG-2156 Decorate imported node titles in Roam from core_title - #1331
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Guideposts for the non-obvious decisions in this diff.
|
|
||
| const CONCEPT_COLUMNS = | ||
| "is_schema, last_modified, schema_id, source_local_id, space_id"; | ||
| "core_title:literal_content->>core_title, is_schema, last_modified, schema_id, source_local_id, space_id"; |
There was a problem hiding this comment.
core_title lives inside literal_content, so this selects the one JSON key by path instead of the whole column. Listing queries must not pull payload columns, and literal_content is named in db-domain-invariants as one. postgrest-js types the alias as string; it is null at runtime when the key is absent, which is why SharedConcept widens it to string | null and buildSharedNodes maps null to undefined.
| { | ||
| rid, | ||
| sourceLocalId: node.source_local_id, | ||
| schemaId: node.schema_id, |
There was a problem hiding this comment.
The query already fetched schema_id and dropped it. Roam needs it to find the node type schema the title format comes from. The null guard above this block is what lets the field be required rather than optional.
| spaceUri: space.url, | ||
| platform: space.platform, | ||
| title: direct.text, | ||
| coreTitle: node.core_title ?? undefined, |
There was a problem hiding this comment.
Optional on purpose. Nodes published before ENG-2153 carry no core_title, and those keep their incoming title verbatim.
| } from "./getDiscourseNodes"; | ||
| import internalError from "./internalError"; | ||
|
|
||
| const SCHEMA_COLUMNS = |
There was a problem hiding this comment.
Roam publishes the node type format flat in literal_content (ENG-2158). Obsidian nests it under literal_content.source_data, because its frontmatter is dual-homed there. Both are read by JSON path, so this select carries no payload column.
| source_data_format: string | null; | ||
| }; | ||
|
|
||
| const findOrCreateNodeType = async ( |
There was a problem hiding this comment.
Match order is id, then name, then create (MG, team chat 2026-08-19). Obsidian already runs the same algorithm in mapNodeTypeIdToLocal (apps/obsidian/src/utils/importNodes.ts:1059). A created type reuses the remote schema's source_local_id as its page uid, so the next import from that space matches on id instead of matching on name again.
| .eq("is_relation", false) | ||
| .in("id", schemaIds); | ||
| if (error) { | ||
| internalError({ |
There was a problem hiding this comment.
Decoration is cosmetic, so neither a failed schema query nor a failed type creation aborts the import. The affected nodes land with their incoming title and we hear about it in PostHog.
| } | ||
|
|
||
| const nodeTypeBySchemaId = new Map<number, DiscourseNode>(); | ||
| for (const schema of data) { |
There was a problem hiding this comment.
Sequential on purpose. Creating a type invalidates the node type caches, and two remote schemas can share a name, so the second iteration has to see what the first created.
| }: { | ||
| client: DGSupabaseClient; | ||
| sharedNode: SharedNode; | ||
| nodeType?: Pick<DiscourseNode, "format">; |
There was a problem hiding this comment.
Only the format is used, so the param asks for only that. Resolution happens in the callers, which keeps this function free of a new I/O stage. Passing nothing reproduces today's behavior exactly.
| sourceModifiedAt: validated.sourceModifiedAt, | ||
| sourceNodeRid: sharedNode.rid, | ||
| }; | ||
| const pageTitle = |
There was a problem hiding this comment.
Computed once, before the create/update branch, so both paths and the title collision checks agree. validateSharedNode stays pure and still validates the incoming title. fetchFullMarkdown keeps using the raw sharedNode.title to strip a Roam origin H1, which a test pins.
| text: string; | ||
| shortcut: string; | ||
| format: string; | ||
| uid?: string; |
There was a problem hiding this comment.
The settings panel must not pass a uid, since Roam generates one. The importer must, to reuse the remote type id so later imports match on id. createPage already accepts an optional uid and falls back to generateUID().
![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: 564ee2d636
ℹ️ 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".
| return createDiscourseNodeType({ | ||
| text: schema.name, | ||
| shortcut: "", | ||
| format: schema.format ?? schema.source_data_format ?? "", | ||
| uid: schema.source_local_id, | ||
| }); |
There was a problem hiding this comment.
When Use new settings store is false—which is the schema default—getDiscourseNodes reads only discourseConfigRef.nodes, but this path always creates the type through the block-prop-only createDiscourseNodeType. An unknown remote type is therefore available only through the returned object for the current import batch; after a reload it is not recognized as a configured node type, and subsequent imports can try to create it again. Create the type through the active legacy store or dual-write both stores.
Useful? React with 👍 / 👎.
| const pageTitle = | ||
| sharedNode.coreTitle && nodeType?.format | ||
| ? decorateTitle(nodeType.format, sharedNode.coreTitle) | ||
| : validated.title; |
There was a problem hiding this comment.
For a page imported before this change, or after its local node-type format changes, the stored source timestamp can already equal lastModified while the local title still differs from this new pageTitle. The unchanged up-to-date branch below returns skipped without comparing titles, so selecting the node again in the discovery dialog never applies the decoration unless the source content changes or the user separately invokes forced refresh. Include title equality in the skip decision so existing imports receive the new naming behavior.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Imported nodes used to land in Roam with the source's title verbatim, so an Obsidian
EVD - REM sleep and recallbecame a page Roam's format regex didn't recognize as an[[EVD]]. The importer now rebuilds the title from the publisher'score_titlewith the local node type's format, and falls back to the incoming title whenever either piece is missing.The pull query already fetched
schema_idand threw it away. It now carriesschemaIdandcoreTitleontoSharedNode, both read as JSON scalars by path (literal_content->>core_title) rather than by pullingliteral_content, so discovery still selects no payload columns. postgrest-js types those projections asstring; they are null at runtime for rows without the key, which is what the?? undefinedhandles.resolveSharedNodeTypesmaps each shared node to a local node type in one batched query: match a local type whose uid equals the remote schema'ssource_local_id, else match on name, else create a local type from the schema. A created type reuses the remote id as its page uid, so the next import from that space matches on id. This is the same algorithm Obsidian runs inmapNodeTypeIdToLocal. One deliberate difference: when a schema carries no format, Obsidian inventsABC - {content}and we store an empty format instead, which leaves those titles verbatim until ENG-2158's format reaches the schema row. Types created this way get an empty shortcut; the user can set one in settings. Resolution is best-effort: a schema row RLS hides, a failed query, or a failed type creation reports to PostHog and leaves those nodes with their incoming title rather than failing the import.Refresh is unchanged in shape: it resolves the type for its one node and passes it through, and
updateImportedPagestill renames only when the title differs, so refreshing an already-decorated page rewrites nothing. A format with{Source}decorates to an empty source and the title ends in" - "for now.Stacked on
eng-2157-decorate-imported-node-titles-in-obsidian-from-core_title(#1330) for the shareddecorateTitlehelper; this PR's diff is against that branch.Deferred: resolving
{Source}into a real source (ENG-2142), prop-based node identification (ENG-2133), a review/accept flow for types created during import (v0 creates them directly), and animportedFrombreadcrumb on created types (Roam'sDiscourseNodehas no such field yet). Until ENG-1861's imported-uid filter (#1286) lands, periodic sync treats decorated imported pages as local nodes. Created types from Obsidian schemas get 26-char page uids (node_<nanoid>); nothing in our code reads uid shape, but it shows in URLs.