[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. 1 Skipped Deployment
|
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 comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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".
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.
Only writeImportedSourceIdentity needs to observe the props-write result (materializeSharedNode deletes the created page when the identity write fails), so expose that as setBlockPropsAsync and keep the default export fire-and-forget. This reverts the signature change that forced void at eight call sites unrelated to this feature.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
eng-1858.mp4
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.