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). Built-in types are in the list on purpose: a remote schema named Page resolves to Roam's Page rather than creating a user type that getDiscourseNodes would then prefer over the built-in. 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. Two remote schemas can share a name, so the second iteration has to see what the first created, and that only works once the legacy cache is refreshed: createDiscourseNodeType invalidates the new-store cache, refreshConfigTree() in findOrCreateNodeType repopulates discourseConfigRef.nodes for the default (store flag off) path.
| }: { | ||
| 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. decorateTitle returns null for a format it cannot rebuild from the core title (no {content}, or a second placeholder such as {Source}), so those nodes keep the incoming title rather than landing as [[EVD]] - x -. Until #1317/#1318 land nothing on main writes core_title, and every import takes the fallback arm.
| 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 👍 / 👎.
There was a problem hiding this comment.
The symptom is real but the cause is the in-session cache, not persistence. createDiscourseNodeType writes the legacy page tree (discourse-graph/nodes/<name> with Shortcut/Tag/Format children) and refreshConfigTree at index.ts:58 rescans it on load, so the type survives a reload under either store. What was missing was the refresh within the session: with the store flag off, getDiscourseNodes reads discourseConfigRef.nodes, which only refreshConfigTree() repopulates. Fixed in 4463f2a by calling it after a successful create, mirroring DiscourseNodeConfigPanel.createNodeType.
| 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 👍 / 👎.
There was a problem hiding this comment.
Intentional, and now stated in the description: the up-to-date skip predates this PR and stays timestamp-only. A page imported before this change gets its decorated title on its next Refresh (force path, which goes through updateImportedPage and renames when the title differs); re-selecting an up-to-date page in the discovery dialog remains a no-op, as before. Folding a title comparison into the skip would make the dialog rename pages the user did not ask to change, which is a separate decision from decoration.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const findOrCreateNodeType = async ( | ||
| schema: SharedNodeSchema, | ||
| ): Promise<DiscourseNode | undefined> => { | ||
| const localNodeTypes = getDiscourseNodes(); |
There was a problem hiding this comment.
No excludeDefaultNodes filter here on purpose. The built-in Page and Block types are synthesized by getDiscourseNodes only while no configured type shares their name, so creating a user type named Page from a remote schema would shadow the built-in (and its has title specification) for every consumer. Letting the built-ins take part in the name match resolves such a schema to the built-in instead.
| const nodeType = await createDiscourseNodeType({ | ||
| text: schema.name, | ||
| shortcut: "", | ||
| format: schema.source_data_format || schema.format || "", |
There was a problem hiding this comment.
Same precedence and operator as Obsidian's parseSchemaLiteralContent (importNodes.ts:1044): source_data.format first, then the flat key, with || so an empty string falls through. No producer writes both keys today, so the order only matters for the parity claim.
| format: schema.source_data_format || schema.format || "", | ||
| uid: schema.source_local_id, | ||
| }); | ||
| refreshConfigTree(); |
There was a problem hiding this comment.
Mirrors DiscourseNodeConfigPanel.createNodeType, which pairs createDiscourseNodeType with refreshConfigTree(). The accessor only invalidates the new-store cache; with Use new settings store off (the default) getDiscourseNodes reads discourseConfigRef.nodes, which this call repopulates. Without it the created type was invisible for the rest of the session and every later import or refresh re-entered the create branch, where createPage then failed on the existing title. The PostHog capture sits after the create so a failed creation is not counted as a created type.
| } | ||
| } | ||
|
|
||
| return nodeTypeBySchemaId; |
There was a problem hiding this comment.
Keyed by schema id rather than re-keyed by rid: both callers already hold sharedNode.schemaId, so the rid map was a second map that only renamed the first.
| `${format | ||
| .replace(/(\[|\]|\?|\.|\+)/g, "\\$1") | ||
| .replace(/{[a-zA-Z]+}/g, "(.*?)")}`; | ||
| .replace(FORMAT_PLACEHOLDER, "(.*?)")}`; |
There was a problem hiding this comment.
The placeholder pattern now comes from decorateTitle, so the regex that decides what extractContentFromTitle captures and the one decorateTitle rebuilds from are the same object. #1330 does the same for the Obsidian helper.
| const pageTitle = | ||
| (sharedNode.coreTitle && nodeType | ||
| ? decorateTitle(nodeType.format, sharedNode.coreTitle) | ||
| : null) ?? validated.title; |
There was a problem hiding this comment.
decorateTitle returns null when the format carries a placeholder other than {content} (this graph's Evidence format has {Source}, and the shared row holds nothing to put there), so the incoming title is kept rather than writing a half-filled name the format regex would not match. Filling that slot is ENG-2142; once it lands, these titles decorate too.
createDiscourseNodeType only invalidates the new-store cache; with the store flag off getDiscourseNodes reads discourseConfigRef.nodes, so a created type stayed invisible and every later import re-entered the create branch. Mirror the settings panel and call refreshConfigTree after a successful create, and only count the type as created once the create resolved. Built-in types now take part in name matching so a remote schema named Page resolves to Roam's Page instead of creating a user type that shadows it. The resolver returns the map keyed by schema id, which both callers already hold. Format precedence follows the Obsidian reader (source_data first, ||), and the Roam format-expression helper reuses the shared placeholder pattern.
4463f2a to
7df22d0
Compare
1 participant
eng-2156.mp4
This is what we are following:
This PR is the bottom path of the diagram: decorate on import, Roam.
Imported nodes used to land in Roam with the source's title verbatim, so an Obsidian
CLM - This is a test Claimbecame a page Roam's format regex didn't recognize as a[[CLM]]. 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 or the format cannot be rebuilt from the core title alone (decorateTitlereturnsnullfor a format without{content}or with a second placeholder such as{Source}). That is why this graph's Evidence type keeps Obsidian's title in the demo: substituting an empty source produced[[EVD]] - x -, which Roam's own format regex does not match once the trailing space is trimmed. Filling the{Source}slot is ENG-2142 (mapsourceto the Roam referenced node on pull); once that lands, Evidence titles decorate like the rest.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.schemaIdis the row id, used only to correlate schema rows within one request; it is never persisted.resolveSharedNodeTypesmaps each schema id 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. Built-in types take part in the name match, so a remote schema namedPageresolves to Roam's Page instead of creating a user type that would shadow it. A created type reuses the remote id as its page uid, so the next import from that space matches on id. After a create the resolver callsrefreshConfigTree(), as the settings panel does:createDiscourseNodeTypeonly invalidates the new-store cache, and withUse new settings storeoff (the default)getDiscourseNodesreadsdiscourseConfigRef.nodes, which only that call repopulates. Without it the created type stayed invisible for the session and every later import or refresh re-entered the create branch. This is the same algorithm Obsidian runs inmapNodeTypeIdToLocal, with the same format precedence (source_data.format, then the flat key,||). 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.I did not reuse
CrossAppNodeSchemafor the schema rows: its converter (dbNodeSchemasToCrossApp) selects the whole row includingliteral_contentand needs space and account maps, and the contract has noformatfield. The localSharedNodeSchemais a five-column projection ofmy_concepts, not a cross-app payload. Addingformatto the contract is ENG-2158's call.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. Pages imported before this change get decorated on their next Refresh; selecting them again in the discovery dialog while they are up to date stays a no-op, as it was.Nothing on
mainwritescore_titleyet (#1317 / #1318 are open), so merged ahead of those every import takes the fallback arm and the live effect is type resolution and creation; the demo ran against a local database with the producers applied.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.createDiscourseNodeTypehas no cleanup if the props write fails after the page is created (the settings panel has the same exposure); the import path reports the failure and leaves the node undecorated.