[ENG-1858] Materialize Obsidian-origin markdown into Roam#1171
Conversation
Add a Roam materializer that turns one Obsidian-origin CrossAppNode into a local page: parse content.full markdown into a Roam block tree, then create the page or update the one already imported from the same source (matched by sourceNodeRid, no duplicate), persisting imported source identity via the ENG-1856 helpers so it can be re-found and refreshed. - markdownToRoamBlocks: pure markdown -> InputTextNode[] (frontmatter strip, ATX headings -> Roam heading blocks, list nesting by indentation); unit-tested. - materializeSharedNode: create / update-by-RID / skip-if-unchanged, a title- collision guard that won't overwrite local pages, and a structured MaterializeResult for ENG-1859 to tally counts. Stacked on ENG-1856 (source-identity storage). Node-type reconciliation and the import command/UI are intentionally deferred (node-typing design question / 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
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| await Promise.all( | ||
| blocks.map((node, order) => | ||
| createBlock({ parentUid: pageUid, node, order }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🔴 Page blocks may appear in wrong order because parallel creation races on positional insertion
New child blocks are all created concurrently (Promise.all at apps/roam/src/utils/materializeSharedNode.ts:32-36) with explicit positional order values, so the final on-page ordering depends on the server processing sequence.
Impact: An updated or newly imported page can display its content blocks in a scrambled order.
Roam API order parameter race under concurrent writes
In replacePageBody, after all existing children are deleted the code fires every createBlock call simultaneously:
await Promise.all(
blocks.map((node, order) =>
createBlock({ parentUid: pageUid, node, order }),
),
);The order parameter tells Roam "insert this block at position N among the parent's current children." When multiple requests arrive concurrently the server resolves each order against whatever child list exists at the moment it processes that request. If the request for order=2 is processed before order=0, position 2 is evaluated against zero existing children, producing an incorrect final arrangement.
The safe pattern (used elsewhere in Roam extension code) is sequential creation:
for (let i = 0; i < blocks.length; i++) {
await createBlock({ parentUid: pageUid, node: blocks[i], order: i });
}This guarantees each block sees the prior siblings already in place.
| await Promise.all( | |
| blocks.map((node, order) => | |
| createBlock({ parentUid: pageUid, node, order }), | |
| ), | |
| ); | |
| for (let i = 0; i < blocks.length; i++) { | |
| await createBlock({ parentUid: pageUid, node: blocks[i], order: i }); | |
| } | |
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 8bec5b7a6a
ℹ️ 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 listItem = LIST_ITEM_RE.exec(line); | ||
| if (listItem) { | ||
| const indent = listItem[1].replace(/\t/g, " ").length; | ||
| appendBlock({ text: listItem[2] }, indent); |
There was a problem hiding this comment.
When the imported markdown contains a list item that is also an ATX heading, such as - ## Source from the repo's Roam markdown exporter (pageToMarkdown.ts places the list prefix before the heading prefix), this branch captures the whole ## Source as plain text and never sets heading. Importing or refreshing those shared nodes loses Roam section-heading formatting and leaves literal markdown markers in the block; parse listItem[2] for HEADING_RE before appending the list node.
Useful? React with 👍 / 👎.
1 participant
What
Adds the Roam materializer: turn one Obsidian-origin shared node (
CrossAppNode) into a local Roam page, or update the page already imported from the same source — no duplicates — preserving imported source identity for later refresh.apps/roam/src/utils/markdownToRoamBlocks.ts— puremarkdown -> InputTextNode[](strip leading YAML frontmatter; ATX headings -> Roam heading blocks clamped to 1–3; list nesting by indentation; other lines -> blocks). Wikilinks/refs pass through. Unit-tested.apps/roam/src/utils/materializeSharedNode.ts— orchestration: dedup bysourceNodeRid(ENG-1856findImportedNodeUidByRid), create new page (createPage({title, tree})) / update existing (rename if title changed, clear + rewrite body) / skip whensourceModifiedAtunchanged; title-collision guard that refuses to overwrite a non-imported local page; identity persisted via ENG-1856writeImportedSourceIdentity. Returns a structuredMaterializeResultfor ENG-1859 to tallycreated/updated/skipped/failed.__tests__/markdownToRoamBlocks.test.ts— 9 parser cases.Base is
eng-1856-...(source-identity storage), notmain. ENG-1856 is a draft-dependent baseline (PR #1161, not yet merged/approved). If ENG-1856's exported surface (importedSourceIdentity.ts) changes, this branch needs a history-preserving update. Review/merge ENG-1856 first.Open question for @sid597 (node-type mapping)
The ENG-1856 boundary note assigns node-type mapping to ENG-1858 ("mirroring Obsidian's
mapNodeTypeIdToLocal"). Obsidian types a note via frontmatter; Roam types a page via its title-format regex — so "mapping" here means reconciling the source title against local Roam node formats (and possibly creating local types). That's a genuine cross-grammar design decision, so MVP0 keeps the source's (already typed) title verbatim — an imported node is recognized as its type only when the title already matches a local format — and defers active type reconciliation/creation.node.nodeTypeis intentionally unused for now. Want me to (a) reformat titles to the matched local format, (b) also auto-create missing local types like Obsidian, or (c) keep verbatim (current)?MVP0 behavior / scope
failed(don't clobber local content). Richer collision UX is out of scope (issue notes it as follow-up).CrossAppNode(incl.content.full); fetching/assembling frommy_contentsis ENG-1859's job (mirrors Obsidian'sfetchNodeContentForImport).Cross-app node import enabledflag.Testing
markdownToRoamBlocksunit-tested (9 cases). vitest isn't installed in the worktree, so it runs in CI; pure logic also proven locally viatsx(9/9).materializeSharedNodeneeds the Roam runtime → runtime proof owed (human) + Loom owed. See proof steps in the Linear issue / worklog handoff.Linear
ENG-1858