[ENG-1858] Materialize Obsidian-origin markdown into Roam - #1267
Conversation
Adds materializeSharedNode, which consumes one Obsidian-origin SharedNode payload: fetches the full-variant markdown, strips YAML frontmatter, converts the body to a Roam block tree, and creates the page or updates the existing one found by sourceNodeRid (rename included) so re-imports never duplicate. Source identity is written via the ENG-1856 helpers after content writes succeed; failures return actionable reasons without touching stored identity. markdownToRoamBlocks is a pure structural parser (headings nest content, lists nest by indentation, fenced code stays whole); fidelity limits are documented under ENG-1882. No UI or call site here - the import action lands with ENG-1859.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
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:
|
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Inline notes to orient the review — one per non-obvious decision.
| .eq("variant", "full") | ||
| .maybeSingle(); | ||
| if (error) return { error: error.message }; | ||
| return { markdown: data?.text ?? "" }; |
There was a problem hiding this comment.
Fetch happens at import-action time, deliberately not during discovery (per the listing-vs-import rework on #1214). Same lookup shape as Obsidian's fetchNodeContent in importNodes.ts, including the one-row-per-variant assumption its maybeSingle makes. A missing row materializes a title-only page rather than failing — full is optional per ENG-2016. text itself is NOT NULL in content.sql; the nullable type (and the ??) comes from my_contents view typegen, so the live path through ?? is row absence.
| tree: InputTextNode[]; | ||
| }): Promise<MaterializeSharedNodeResult> => { | ||
| const collidingPageUid = getPageUidByPageTitle(sharedNode.title); | ||
| if (collidingPageUid) { |
There was a problem hiding this comment.
MVP0 title-collision behavior: fail with an actionable reason and write nothing (same on the rename path in updateImportedPage). Final collision UX is out of scope per the ticket. Related: per ENG-1856's boundary note, node-type mapping is ENG-1858's decision — MVP0 keeps the source title verbatim, so a title matching a local node format is picked up by Roam's existing format recognition. Verbatim vs reformat vs auto-create is an open question in the PR description.
| for (const [order, node] of tree.entries()) { | ||
| await createBlock({ parentUid: pageUid, order, node }); | ||
| } | ||
| writeImportedSourceIdentity({ |
There was a problem hiding this comment.
Identity is written only after content writes succeed — on this update path nothing touches the existing importedFrom props until then, so a mid-write failure can't drop source identity (ticket Done-When #3). Same ordering on the create path above.
| return { status: "updated", pageUid }; | ||
| }; | ||
|
|
||
| export const materializeSharedNode = async ({ |
There was a problem hiding this comment.
No production caller yet by design: ENG-1859 (import action) is blocked by this ticket and consumes this. That's also why failures are returned as reason strings instead of toast/PostHog here — reporting belongs to the action surface, matching how DiscoverSharedNodesDialog handles it with internalError.
| const indentWidth = (whitespace: string): number => | ||
| [...whitespace].reduce((width, char) => width + (char === "\t" ? 4 : 1), 0); | ||
|
|
||
| export const markdownToRoamBlocks = (markdown: string): InputTextNode[] => { |
There was a problem hiding this comment.
Searched for an existing markdown→blocks path before writing this: roamjs-components/marked is the opposite direction (Roam text → HTML), importDiscourseGraph consumes already-structured JSON, and the export utils all go Roam→markdown. marked's lexer would be a new direct dependency and would still need this tree-building on top of its flat tokens (nesting content under headings is a Roam-import convention, not a token property). Known fidelity limits (tables, setext headings, indented fences, ordered-list numbering, continuation lines) are ENG-1882's documentation scope.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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: 6dfcde87f5
ℹ️ 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".
Replaces the hand-rolled markdown parser with Roam's native data.page.fromMarkdown / data.block.fromMarkdown, so Roam owns markdown conversion. stripFrontmatter moves to @repo/content-model next to normalizeLineEndings. Results now carry the source identity and a failure stage so ENG-1859 can report per-node outcomes. Input is validated at the boundary (platform, RID shape, modified time, full content type). setBlockProps returns its update promise so the identity write can be awaited; existing call sites keep fire-and-forget behavior via void. On create, a page whose identity cannot be stored is removed again; on update, replacement blocks land before old ones are deleted.
sid597
left a comment
sid597
left a comment
There was a problem hiding this comment.
Refreshed inline notes for the native-API design (the earlier review's anchors are outdated).
| }; | ||
| }; | ||
|
|
||
| const getRoamMarkdownApi = (): RoamMarkdownApi => |
There was a problem hiding this comment.
data.page.fromMarkdown / data.block.fromMarkdown exist on the runtime roamAlphaAPI but not in roamjs-components' types, hence this local declaration. Preference order applied here: native Roam API > roamjs-components > hand-rolling — Roam owns markdown→blocks conversion, so fidelity follows Roam's importer rather than code we maintain (remaining fidelity questions are ENG-1882's scope).
| }, | ||
| }); | ||
|
|
||
| const validateSharedNode = ( |
There was a problem hiding this comment.
Boundary validation before any I/O: platform from the Platform enum (not a RID prefix test, which fails for https-shaped space URIs), RID shape via isRid, and the modified time canonicalized with toISOString() so what lands in block props is always UTC-canonical.
There was a problem hiding this comment.
Context from maparent on #1224 (comment): he proposes augmenting ridToSpaceUriAndLocalId to also return the Platform (the info is present in the RID forms we design). No change here — for discovery-driven imports the platform arrives server-sourced on the SharedNode (my_spaces.platform), which stays the source of truth. Where the augmented helper pays off is ENG-1860's refresh flow, which starts from a stored importedFrom.sourceNodeRid with no Space row in hand (today that costs a Space-table lookup via getSpaceNameIdFromRid). Under discussion — adopt there if/when he lands it.
| return { sourceModifiedAt: modifiedAt.toISOString(), title }; | ||
| }; | ||
|
|
||
| const fetchFullMarkdown = async ({ |
There was a problem hiding this comment.
Content fetched at import-action time, deliberately not during discovery (per the listing-vs-import rework on #1214). A missing full row materializes a title-only page — full is optional per ENG-2016. Only text/obsidian+markdown is accepted: Roam-produced text/markdown embeds the title as an H1 (buildFullMarkdown), which would duplicate the title into the body. text is NOT NULL in content.sql; the nullable type comes from the my_contents view typegen.
| markdown: string; | ||
| title: string; | ||
| }): Promise<MaterializeSharedNodeResult> => { | ||
| if (getPageUidByPageTitle(title)) |
There was a problem hiding this comment.
MVP0 collision behavior: a page we did not import holds this title, so creating over it is the only destructive outcome available — refuse with an actionable reason and write nothing. Final collision UX is out of scope per the ticket. Node-type mapping is deliberately deferred (title kept verbatim; open question in the PR description).
| } catch (error) { | ||
| let cleanupError: unknown; | ||
| try { | ||
| await window.roamAlphaAPI.data.page.delete({ page: { uid: pageUid } }); |
There was a problem hiding this comment.
If the identity write fails after page creation, the page is removed again so no untracked partial import is left behind — re-importing stays clean. If the cleanup itself fails, the result carries the orphaned pageUid so the caller can surface it.
| try { | ||
| const previousChildren = getShallowTreeByParentUid(pageUid); | ||
| if (markdown) { | ||
| await getRoamMarkdownApi().block.fromMarkdown({ |
There was a problem hiding this comment.
Replacement blocks are appended before the previous children are deleted, so the page is never empty mid-flight; the rename was already collision-checked before any mutation. Identity is refreshed last — a failure at any stage returns that stage plus the identity, and never erases the stored importedFrom props.
| } as Record<string, json>; | ||
| window.roamAlphaAPI.data.block.update({ block: { uid, props } }); | ||
| return props; | ||
| return window.roamAlphaAPI.data.block |
There was a problem hiding this comment.
This was a floating promise — callers could not observe whether the props write landed, which the materializer needs before reporting success. Returning the update promise makes it awaitable; the 9 existing call sites are prefixed with void to preserve their fire-and-forget behavior exactly. Whether any of them should await is pre-existing and unchanged by this PR.
|
|
||
| const FRONTMATTER_DELIMITER = "---"; | ||
|
|
||
| export const stripFrontmatter = (markdown: string): string => { |
There was a problem hiding this comment.
Obsidian's full variant is the raw note file including YAML frontmatter (upsertNodesAsContentWithEmbeddings.ts stores vault.read(file)); Roam has no frontmatter concept, so it must be stripped before fromMarkdown. Placed here next to normalizeLineEndings as pure text logic both apps care about. Open question: should the Obsidian producer strip at publish time instead? This is needed either way for rows already published.
There was a problem hiding this comment.
Resolving the open question above with maparent's ruling from #1224 (comment): the dual-home design stays — frontmatter lives in literal_content.source_data (jsonb, indexed, key order lost) and in the full body (lossless), so the producer will not strip it and consumer-side stripping is the accepted MVP0 shape (what this helper does). He flags Roam-side stripping itself as acknowledged debt: "Roam should not have to care about Obsidian specifics… it should not have to strip the frontmatter either" — to be revisited with the atJSON transfer, where strippable regions get marked platform-agnostically (ENG-1882's v1 handoff).
Related future scope from the same comment: translating frontmatter into the Roam properties block is a separate piece of work and should read from source_data, not from re-parsing the full text. Deliberately not ticketed yet — under discussion first.
The Content schema allows derived rows per (space_id, source_local_id, variant) — non-original rows carry original = NULL, so only (…, variant, original) is unique. Without the original filter a derived full representation makes maybeSingle() fail with a multiple-rows error and a valid node cannot be materialized. From Codex review on #1267.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e55349910
ℹ️ 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 { | ||
| error: `Unsupported full content type "${data.content_type}" — expected "${contentTypes.obsidianMarkdown}"`, | ||
| }; | ||
| return { markdown: stripFrontmatter(data.text).trim() }; |
There was a problem hiding this comment.
When an Obsidian note's first body line is an indented Markdown code block (for example, four leading spaces after the frontmatter), calling .trim() removes that indentation before fromMarkdown receives it, converting the code block into ordinary text. Strip only the blank lines surrounding the body rather than all leading whitespace so valid Markdown structure is preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f6dc8f8: trim() replaced with trimBlankLines (in @repo/content-model next to the other text helpers), which strips only surrounding blank lines so a leading indented code block keeps its indentation; a whitespace-only body still collapses to the title-only path. Both behaviors are pinned by tests ("preserves the indentation of a leading indented code block", "creates a title-only page when the body is only whitespace").
trim() on the stripped body also removed the first line's indentation, so a body starting with an indented code block degraded to plain text in Roam. trimBlankLines (content-model) strips only surrounding blank lines; a whitespace-only body still collapses to the title-only path. From Codex review on #1267.
1 participant
Implements the ENG-1858 materializer on top of what is merged on main (ENG-1855 discovery, ENG-1856 identity storage, ENG-2019 shared-node payload).
What this adds
materializeSharedNode({ client, sharedNode })— consumes one Obsidian-originSharedNodepayload:isRid, modified-time validity, non-empty title),fullcontent variant at import-action time and strips frontmatter,data.page.fromMarkdownon create,data.block.fromMarkdownon update — so Roam owns markdown→blocks conversion,sourceNodeRidalready exists, updates it in place (replacement blocks land before old ones are deleted; rename included) — re-importing never duplicates,importedFromidentity via the ENG-1856 helpers only after content writes succeed, and awaits it,{ success: true, action }or{ success: false, error: { message, stage } }so ENG-1859 can report per-node outcomes; a created page whose identity cannot be stored is removed again (no untracked partial import).stripFrontmatterin@repo/content-modelnext tonormalizeLineEndings— pure text logic both apps care about.setBlockPropsnow returns itsblock.updatepromise so the identity write is observable; the 9 existing call sites are prefixed withvoid, preserving their fire-and-forget behavior exactly. Whether any of them should await is pre-existing and unchanged here.fullis absent.Decisions to check in review
fromMarkdownAPI.data.page.fromMarkdown/data.block.fromMarkdownare not in roamjs-components' types, so their signatures are declared locally and tests mock that declaration — green tests do not prove the real API behaves as declared. Runtime demo owed before merge (see below).DiscoverSharedNodesDialog'sinternalErrorpattern.fullvariant → title-only page rather than failure (fullis optional per ENG-2016), via typeddata.page.create.text/obsidian+markdownis accepted forfull— Roam-producedtext/markdownembeds the title as an H1 (buildFullMarkdown), which would duplicate the title into the body.mapNodeTypeIdToLocal) needs a call; per ENG-1856's boundary note this decision belongs to ENG-1858.sourceModifiedAtis validated and canonicalized withtoISOString()before storage — no naive parsing of DB values (the only PostgREST reads here aretext/content_type).DiscourseNodeConfigPanel.tsxandsupabaseContext.tssit on lines this PR does not touch.Out of scope (per ticket)
Import discovery UI / multi-select (ENG-1859), relations & assets (ENG-1872), refresh-all (ENG-1860), markdown fidelity documentation (ENG-1882).
Verification
eslint --max-warnings 0on changed files,prettier,tsc --noEmit --skipLibCheck: clean (roam + content-model).fromMarkdownbehavior (markdown fidelity, caller-supplied uid handling) can only be proven on a live graph.