ENG-1857 Validate Obsidian importer with Roam-origin nodes - #1225
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. 1 Skipped Deployment
|
f06e484 to
42b7d97
Compare
![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: 42b7d97044
ℹ️ 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 { nodeKeys } = await getImportedNodesInfo({ | ||
| queryEngine, | ||
| plugin, | ||
| client, | ||
| }); |
There was a problem hiding this comment.
When a user moves an imported note outside import/, getImportedNodesInfo no longer detects it because QueryEngine.getImportedNodePages() explicitly restricts its scan to that folder. The node is therefore offered for import again; if selected, findExistingImportedFile() still finds the relocated file across the whole vault and processFileContent() replaces its contents with the remote payload, potentially destroying edits made after relocation. Duplicate discovery should include imported notes regardless of their current folder.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Removes the Obsidian unit-test harness this branch introduced (vitest, test:unit, sharedNodeImport.test.ts). The app had no test infra, and the extracted-for-testability helpers had started shaping production code around the tests rather than the feature. sharedNodeImport.ts goes with it, its pieces moving to where they belong. - dedupe and refresh scans find imported notes wherever they now live, not only under import/: a relocated note was re-offered for import and then overwritten in place - node type resolution is handed over from computeImportPreview through precomputedData instead of re-querying the same concept/schema rows at import time; the direct fetch remains for refreshImportedFile - my_concepts lookups filter arity = 0 and use generated row types with explicit null narrowing instead of hand-written types behind casts - failed imports log why: query error, no node type schema, no content - processFileContent returns TFile; its unreachable error arm, the assertion that arm forced, and the unused originalFilePath param are gone, along with the now-always-defined optional params - getImportedNodeKey is the single definition of the spaceId:localId key - getAvailableImportPath covers both the new-file and rename collisions - drops gray-matter, unused since frontmatter parsing was removed
importSelectedNodes now returns the per node reasons it collected, matching what refreshAllImportedFiles already returns, and the modal logs them next to its summary Notice the way the refresh command does. Before this, a node that could not be imported was counted and dropped in silence. refreshImportedFile passes the collected reason through instead of the fixed "Failed to refresh imported file" string, so refreshAllImportedFiles reports something usable. failed is derived from the collected reasons rather than tracked in its own counter: every path out of a node either succeeds or records exactly one reason, so a separate count was a second source of truth. Reasons carry the instance id because titles are not unique across source spaces, which is the case this branch exists to handle.
1 participant
Summary
Validates and fixes the existing Obsidian shared-node importer for Roam-origin nodes. The importer now:
my_conceptsschema rows before mapping it locallynodeInstanceId, mappednodeTypeId,importedFromRid,lastModified,authorId)Root Cause
Roam-origin shared node markdown does not include the Obsidian-style frontmatter that the importer was previously parsing from
rawContent. That meant the importer could create the file, fail to findnodeTypeId/nodeInstanceId, and then delete the file as invalid.Decisions worth your attention
Duplicate detection is no longer scoped to
import/.QueryEngine.getImportedNodePages()filtered onpath("import"); theimportedFromRid+nodeInstanceIdfrontmatter pair is what actually identifies an imported node. With the folder filter, a note moved out ofimport/was offered for import again, and becausefindExistingImportedFilesearches the whole vault, confirming that import overwrote the relocated file's body — losing edits made after the move. The filter is gone from both the DataCore query and the vault-iteration fallback. That method has three callers, so this also fixesrefreshAllImportedFilesandbuildEndpointToFileMapskipping relocated notes. Those two are outside this ticket's stated scope; I judged a shared "which notes came from elsewhere" query having no business caring about the folder to be a fix rather than scope creep, but flagging it since it widens the blast radius of a 2-line change..eq("arity", 0)on themy_conceptslookups, per @maparent on #1147 (syncDgNodesToSupabase.ts: "add arity = 0 to avoid relation instances") and #1214 (discoverSharedNodes.ts: "Add.eq(\"arity\", 0)to avoid relations"). Applied to the node-type schema query too, matching the existingis_schema = true+arity = 0pairing atsyncDgNodesToSupabase.ts:186,:519andtemplateImport.ts:92— those sites are also the evidence that node-type schemas carryarity = 0(compute_arity_localreturnsCOALESCE(jsonb_array_length(literal_content->'roles'), 0), so node types compute 0 and relation types compute 2).Node type resolution is handed over from the preview rather than re-queried.
computeImportPreviewalready ran the same instance → schema lookup, andprecomputedDataexists to avoid re-querying, so the preview now buildssourceNodeTypeIdByKey(its schema select gainedid) and passes it through.fetchSourceNodeTypeIdssurvives as the fallback forrefreshImportedFile, which has no preview — the same shapekeyToRelationEndpointIdalready uses.Failure reporting follows
refreshAllImportedFiles. It returns{ success, failed, errors }and its caller shows an aggregate Notice thenconsole.errors the array (registerCommands.ts:167); the importer now does the same. Per-node reasons are not surfaced in the UI because no path in this app does that — that would be an app-wide change.apps/obsidianhas no PostHog integration, so there is no telemetry path here either.Opening the import modal now costs one DB query. Duplicate keys are space-scoped, and mapping
importedFromRid→spaceIdrequires the DB, so the local-only frontmatter scan becamegetImportedNodesInfo(one batched space lookup).Parity notes on the frontmatter write.
authorIdwasif (authorId) record.authorId = …and is now written unconditionally — reachable only forauthorId: 0, which is not a real id. Theif (mappedNodeTypeId !== undefined)guard is gone becausemapNodeTypeIdToLocalreturnsPromise<string>and falls back tosourceNodeTypeId, so it could never be false.Removed:
gray-matter(this PR removed its last usage),getLocalNodeInstanceIds(replaced bygetImportedNodesInfo), andprocessFileContent's error return arm (it has no failing path left, so the union, the caller's error branch, and theresult.file!assertion it forced all went with it). The lockfile is edited surgically — a fullpnpm install --lockfile-onlychurned 180 unrelated lines of peer-dependency hashes;pnpm install --frozen-lockfileaccepts the 3-line edit.No unit test harness. An earlier revision of this branch added vitest plus tests for four extracted helpers.
apps/obsidianhad no test infra, and the extraction had started shaping production code around the tests (a parameter existed only so a test could assertObject.assignpreserves keys). Harness, tests, and the file they were extracted into are all removed; validation for this ticket is the runtime flow below. A standalone infra PR is the right way in if we want Obsidian unit tests.Known Limits
[[CLM]]into[[CLM|CLM]]. Semantic body and nested bullets are preserved; byte-for-byte fidelity is covered by ENG-1882.fullcontent variant is built as# ${title}\n\n${body}(roamToCrossAppConverters.ts:38), so an imported Roam note opens with an H1 duplicating its own title. Obsidian-originfullis body-only, per the direct/full split. Not changed here; documented for ENG-1882.Runtime Proof
42b7d970, before the review fixes ind5ec65bcanda0da822a. It does not cover the current code and needs re-driving. Changed since: themy_conceptsquery shape (arity filters), where node types come from (preview handoff), the duplicate-detection scan scope, and the frontmatter write path.Recorded against
42b7d970, through the real Obsidian command:Discourse Graph: Import nodes from another spacein the Obsidian plugin.eng1890testgroups.test-1-graph:[[EVD]] - This is a Evidence page. - {Source}[[CLM]] - cat -1node,0relations, and no new schemas/relations.import/test-1-graph/[[CLM]] - cat -.mdwith mapped frontmatter and the Roam markdown body.Cells the re-drive must cover, none of which the run above exercised:
arity = 0filters return rows — if that reading is wrong, node type resolution returns nothing and every import fails)(1).mdimport/is still recognised as already imported when the modal is reopenedValidation
pnpm --filter @discourse-graphs/obsidian check-typespnpm --filter @discourse-graphs/obsidian build— 0 errorseslint --max-warnings 0/prettier --checkon changed files; no eslint warning falls on a line this PR touches