ENG-1858 Materialize Obsidian-origin markdown into Roam - #1224
Conversation
|
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.
|
9b7eaf7 to
9eb8f9f
Compare
b977291 to
dbb154b
Compare
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:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Strip Obsidian frontmatter before handing markdown to Roam. The `full` content variant is the raw note file, so the YAML block was materializing as literal Roam blocks. Adds `stripFrontmatter` to @repo/content-model. Determine Obsidian origin from the `Platform` enum that discovery already carries, instead of sniffing an `obsidian:` prefix off the RID, and check RID shape with `isRid`. Refuse to create a page whose title is already held by a page that was not imported from this source, so the MVP0 collision behaviour is defined. Replace page content before renaming the title, so a markdown failure leaves the existing page fully untouched. Store `sourceModifiedAt` as canonical UTC, and accept only `text/obsidian+markdown` for `full` — no producer emits `text/markdown`. Restore `setBlockProps` as the single block-props writer by returning its update promise, replacing the copy of its body in `writeImportedSourceIdentity`. Existing call sites keep their current fire-and-forget behaviour via `void`. Narrow the untyped-API cast to the `fromMarkdown` members that are actually missing from roamjs-components, and stop declaring a return value the code ignores.
|
Ok. First Rid because it's an easy question: I think we could augment ridToSpaceUriAndLocalId so it also delivers the Platform in the structure. The information is present for the platforms we know, and I think it's a safe assumption that the Rids we design will always include that information. Would that help? |
|
Stripping Frontmatter is more complex. Right now we put the frontmatter in two places: the Concept's literal_content (under |
2 participants
What
A Roam materializer for one Obsidian-origin
CrossAppNode: create a local page from the shared markdown, or update the page already imported from the same source RID instead of duplicating it, preserving imported source identity for later refresh.Base is
main(ENG-1856 landed in #1218).Behaviour
First import — validate the input, refuse if the title is already held by a page we did not import, create the page from the markdown body via Roam's native
data.page.fromMarkdown, then store the source RID and source-modified time. If the identity write fails the new page is removed, so no untracked partial import is left behind.Re-import — find the page by source RID, materialize replacement blocks and delete the previously captured children only after the replacement succeeds, then rename the title, then refresh source identity.
Failures return a
stage(validate-input,find-imported-node,title-collision,create-page,replace-page-content,update-page-title,write-source-identity) and never drop the source RID or timestamp, so ENG-1859 can report per-node outcomes.Decisions a reviewer will want
Obsidian frontmatter is stripped before Roam sees the markdown. The
fullcontent variant is the raw note file —apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts:74-81sets it tovault.read(node.file), frontmatter included (node.frontmatteris stored additionally inmetadata, not instead). Roam has no frontmatter concept, so passing it through materialized---/nodeTypeId:/… as literal blocks. The mirror-direction importer does the same thing with gray-matter (apps/obsidian/src/utils/importNodes.ts:948).stripFrontmatterlives in@repo/content-modelnext tonormalizeLineEndingsrather than as a Roam-local copy, since it is pure text logic both apps care about. Open question for @maparent: the contract saysdirect= title,full= body — should the Obsidian producer be stripping frontmatter at publish time instead? This strip is needed either way for rows already published.Origin comes from the
Platformenum, not the RID string.discoverSharedNodesalready surfacessourceApp: Enums<"Platform">frommy_spaces.platform, somaterializeObsidianNodetakes it as input rather than testing the RID for anobsidian:prefix — that prefix test also fails for the web-URL RID form thatspaceUriAndLocalIdToRidemits forhttp-shaped space URIs. RID shape is checked withisRid.Title collision is refused, not resolved. Roam page titles are unique and
findImportedNodeUidBySourceRidcannot see a page the user authored locally. Creating over one is the only genuinely destructive outcome available here, so the create path returnstitle-collisionand writes nothing. Richer collision UX is out of scope per the issue; this is the documented MVP0 behaviour.setBlockPropsstays the only block-props writer. Making the identity write observable needs an awaitable write, and the reason it was not awaitable is a floating promise insidesetBlockProps(which already carried ano-floating-promiseswarning). Rather than keep a second copy of the merge logic inwriteImportedSourceIdentity,setBlockPropsnow returns the update promise. The 10 existing call sites are prefixed withvoid, which preserves their current fire-and-forget behaviour exactly — whether any of them should await is pre-existing and unchanged by this PR.node.nodeTypeis intentionally unused. Obsidian types a note through frontmatter; Roam types a page through its title-format regex. Reconciling those is a cross-grammar decision, so MVP0 keeps the source title verbatim — an imported node is recognised as its type only when the title already matches a local format. Active type reconciliation/creation is follow-up work.Only
text/obsidian+markdownis accepted forfull. No producer emitstext/markdownfor an Obsidian node, andtext/markdownpayloads embed the title as an H1 (buildFullMarkdowninroamToCrossAppConverters.ts), which would duplicate the title into the page body.Scope
Materializer API only — nothing calls it yet. ENG-1859 wires the discovery selection UI and multi-node reporting; it is blocked by this issue. Relations, assets and refresh-all are ENG-1872 / ENG-1860.
Over the size guideline at ~15 files. The coupling: 341 lines are the two test files,
setBlockProps+ its 10voidcall sites are one mechanical change needed to make the identity write awaitable, andstripFrontmatteris 20 lines in@repo/content-model. Splitting thesetBlockPropschange into its own base PR is fine if preferred.Validation
pnpm --filter roam test— 12 files / 63 tests passpnpm --filter @repo/content-model test— 2 files / 9 tests passpnpm --filter roam check-types,pnpm --filter @repo/content-model check-types— passpnpm --filter roam build— 0 errorsDiscourseNodeConfigPanel.tsxandsupabaseContext.tsare pre-existing, on lines this PR does not touch)Runtime proof still owed
data.page.fromMarkdownanddata.block.fromMarkdownare not inroamjs-components'roamAlphaAPItype, so their signatures are declared locally and every test mocks that declaration. Green tests here do not prove the real API behaves as declared. Not yet driven against a live graph: the created/updated page shapes, whetherpage.fromMarkdownhonours a caller-supplied uid, whatpage.updatewith"merge-pages": falsedoes when the target title is taken, and how Obsidian wikilinks/embeds land in Roam blocks (markdown fidelity limits are ENG-1882). Do not merge on the unit tests alone.Known boundary
Roam mutations are not transactional. If replacement markdown is imported but deleting a previous child fails, the result reports
replace-page-contentand the page can briefly contain both new and leftover old blocks.Also worth a follow-up:
findImportedNodeUidBySourceRid(from #1218) takes the first row of an unordered datalog result, so if two pages ever carry the same source RID, which one a re-import overwrites is non-deterministic.Linear
ENG-1858