ENG-1857 Import Roam-origin shared nodes through the existing Obsidian importer - #1266
ENG-1857 Import Roam-origin shared nodes through the existing Obsidian importer#1266sid597 wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
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.
Inline notes to orient review — decisions and parity accounting are in the PR description.
| name: string; | ||
| }; | ||
|
|
||
| export const fetchNodeTypeSchemasForInstances = async ({ |
There was a problem hiding this comment.
Roam's full variant is body-only markdown (buildFullMarkdown in apps/roam/src/utils/roamToCrossAppConverters.ts emits # Title + body, no YAML), so there is no embedded frontmatter to read identity from — this resolves each instance's node type from the source space's Concept rows instead. It consolidates the two queries computeImportPreview was already running inline (parity table in the description). name is consumed by the preview caller only; the import path uses nodeTypeId. Both queries carry arity = 0 so relation instances / relation-type schemas (arity 2) can't slip in regardless of what ids a caller passes.
| @@ -939,9 +1009,6 @@ const sanitizePathForImport = (path: string): string => { | |||
|
|
|||
| type ParsedFrontmatter = { | |||
There was a problem hiding this comment.
Trimmed to what this parse actually feeds: only nodeTypeId is read from content frontmatter now. The nodeInstanceId/publishedToGroups/authorId reads further down this file (refreshImportedFile) come from the metadata cache — a different object — not this parse.
| const sourceNodeTypeId = | ||
| typeof frontmatter.nodeTypeId === "string" | ||
| ? frontmatter.nodeTypeId | ||
| : nodeTypeIdFromConcept; |
There was a problem hiding this comment.
Ordering: content frontmatter wins when present, so Obsidian-origin imports behave exactly as before; the Concept-derived id is only reached by frontmatter-less (Roam-origin) content. For Obsidian-origin the two values are identical by construction — publish writes schema_represented_by_local_id from the same frontmatter it embeds in the content.
| record.nodeTypeId = mappedNodeTypeId; | ||
| } | ||
| record.nodeTypeId = mappedNodeTypeId; | ||
| record.nodeInstanceId = nodeInstanceId; |
There was a problem hiding this comment.
This line is what makes duplicate prevention work for Roam-origin nodes: findExistingImportedFile (re-import updates in place) and getLocalNodeInstanceIds (already-imported nodes drop out of the modal) both match on nodeInstanceId + importedFromRid frontmatter. Obsidian-origin content already carries this value, so there it's an idempotent rewrite.
| } | ||
| record.nodeTypeId = mappedNodeTypeId; | ||
| record.nodeInstanceId = nodeInstanceId; | ||
| record.importedFromRid = spaceUriAndLocalIdToRid( |
There was a problem hiding this comment.
Now built from the caller-supplied nodeInstanceId — same value the removed frontmatter read produced. Rid shape is unchanged for Roam spaces: their space URL is https-based and the https branch of spaceUriAndLocalIdToRid ignores the "note" subtype, matching what buildSharedNodes produces for Roam platforms. Identity stays carried once as the RID (per the #1161/#1214 direction).
| spaceName, | ||
| }); | ||
|
|
||
| const nodeTypeSchemasByInstance = await fetchNodeTypeSchemasForInstances({ |
There was a problem hiding this comment.
Batched once per space, outside the per-node loop — mirrors where the preview step does the same resolution.
| name: string; | ||
| }>) { | ||
| const sourceNodeTypeId = schema.source_local_id; | ||
| const nodeTypeSchemasByInstance = await fetchNodeTypeSchemasForInstances({ |
There was a problem hiding this comment.
Drop-in replacement for the two inline queries that lived here — same filters plus arity = 0, select adds id (keys the instance→schema map), and null-data now logs instead of silently continuing. Dedup behavior is preserved by the existing seenNodeTypeIds / nodeTypeIdToName.has checks: the map yields one entry per instance rather than unique schemas, but outcomes are identical.
| ]); | ||
| }); | ||
|
|
||
| it("builds a Roam-origin shared node with a URL-based rid", () => { |
There was a problem hiding this comment.
Roam-origin fixture: for an https space URL the rid is <spaceUrl>/<uid> (no orn: subtype), platform Roam; timestamps go in as production-shape Z-less naive UTC and come out normalized ISO.
![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: 22770dc2de
ℹ️ 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".
…kup cannot delete an existing imported file
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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-1857.mp4
Problem
The producer side of the M1 contract is merged (ENG-1848
fullvariant, ENG-1849 schema metadata), and Roam-origin shared nodes already show up in Obsidian's import modal — but importing one was guaranteed to fail. Roam'sfullvariant is body-only markdown (# Title+ body, no YAML — seebuildFullMarkdowninapps/roam/src/utils/roamToCrossAppConverters.ts), whileprocessFileContenthard-requirednodeTypeId+nodeInstanceIdfrontmatter inside the content: every Roam node hit "importedNode missing sourceNodeTypeId" and the created file was deleted. Roam-imported files also never gotnodeInstanceIdwritten to frontmatter, so duplicate prevention (findExistingImportedFile,getLocalNodeInstanceIds) could never match them and they would re-list as importable forever.What changed
fetchNodeTypeSchemasForInstances(importNodes.ts): batched per-space lookup of each instance's node-type schema (source_local_id+ name) from the source space's Concept rows. This consolidates the two inline queriescomputeImportPreviewalready ran — the importer now uses the same resolution instead of duplicating it. Both queries carryarity = 0per the data-layer invariant (node instances and node-type schemas; relation-type schemas haveroles→ arity 2 and must be excluded).processFileContent: now takesnodeInstanceId(the caller always has it) andnodeTypeIdFromConcept. Content frontmatter still wins when present, so Obsidian-origin imports behave exactly as before; the Concept-derived id is only reached by frontmatter-less (Roam-origin) content. For Obsidian-origin the two values are identical by construction (publish writesschema_represented_by_local_idfrom the same frontmatter). It also writesnodeInstanceIdinto the imported file's frontmatter — idempotent for Obsidian-origin, and what makes stable-identity dedup work for Roam-origin.computeImportPreview: inline instance→schema queries replaced with the shared helper (also drops twoas Array<...>casts).buildSharedNodes— URL-based rid (https://roamresearch.com/#/app/<graph>/<uid>), platformRoam, production-shape (Z-less) timestamps in, normalized ISO out.Removals (dead code inside the touched signatures)
originalFilePathparam onprocessFileContent— declared and passed, never read (was an eslint warning on main).if (mappedNodeTypeId !== undefined)—mapNodeTypeIdToLocalalways returnsstring.nodeInstanceIddirectly now; the frontmatter read it guarded is gone.ParsedFrontmatter.nodeInstanceId/publishedToGroups/authorId— never read from this parse (the reads inrefreshImportedFileare metadata-cache frontmatter, a different object).Extraction parity (preview queries → shared helper)
computeImportPreview)fetchNodeTypeSchemasForInstances)space_id, is_schema=false, in(source_local_id)arity=0select source_local_id, nameselect id, source_local_id, name+arity=0idkeys the instance→schema mapif (!conceptRows) continue(silent)console.error+ empty mapseenNodeTypeIds/nodeTypeIdToName.hasas Array<{...}>casts ×2Identity notes
importedFromRidsemantics are unchanged for Roam spaces: their space URL ishttps://roamresearch.com/#/app/<graph>, and the https branch ofspaceUriAndLocalIdToRidignores the"note"subtype — matching the ridbuildSharedNodesproduces for Roam platforms exactly.nodeInstanceId+importedFromRidfrontmatter →findExistingImportedFile(re-import updates in place) andgetLocalNodeInstanceIds(already-imported nodes drop out of the modal).Markdown fidelity gaps (documented per ticket; follow-up ticket candidates)
literal_contentis{label, template?}only; a Roam type with no name match in Obsidian gets a synthesized format (CLA - {content}), not Roam's real one.template; Obsidian treatsnodeType.templateas a template filename, so an unmatched Roam type with a template gets an unusable value.# TitleH1 duplication — Roam'sfullembeds the title as H1 and the Obsidian filename carries it too (producer shape from ENG-1848, not changed here).{{[[query]]}},{{[[table]]}},{{[[kanban]]}}, attributes (foo::),^^highlights^^(≠ Obsidian==),#[[multi-word tags]]arrive verbatim; unresolved[[links]]become alias-form dead links.full(optional per ENG-2016) fail import with a counted error — pre-existing importer behavior, unchanged.Out of scope (per ticket)
New import UI, materializer rewrite, relations/assets/refresh behavior.
refreshAllImportedFilesstill refreshes file-by-file (pre-existing loop); the new per-space lookup rides that pattern.Testing
packages/databasevitest 8/8 (new Roam fixture included); eslint delta 0 vs main (the 17 remaining warnings pre-date this PR, minus one this PR removes); tsc clean on touched files; plugin builds with 0 errors.🤖 Generated with Claude Code