ENG-1301: Import relations when a node is imported by trangdoan982 · Pull Request #847 · DiscourseGraphs/discourse-graph

trangdoan982 · 2026-03-02T15:19:09Z

https://www.loom.com/share/407aff42d138483d9c59e20d55a24aec

https://www.loom.com/share/f299d265128d4fe4b79db9c7542e7fcb

linear · 2026-03-02T15:19:12Z

ENG-1301 Import relations when a node is imported

supabase · 2026-03-02T15:19:15Z

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

maparent

Some of this we'll look at together. Let's plan a joint session.

apps/obsidian/src/services/QueryEngine.ts

apps/obsidian/src/utils/importRelations.ts

devin-ai-integration

Devin Review found 1 new potential issue.

View 13 additional findings in Devin Review.

devin-ai-integration · 2026-03-09T15:59:05Z

apps/obsidian/src/utils/importRelations.ts

+          .from("my_concepts")
+          .select("id, source_local_id")
+          .in("id", [sourceSchemaId, destSchemaId]);
+        if (schemaRows && schemaRows.length === 2) {


🔴 length === 2 check silently skips triple creation for self-referencing or same-schema relations

In importRelationsForImportedNodes, the check conceptSchemas.length === 2 at line 287 fails when sourceData.id === destData.id (a self-referencing relation where both endpoints are the same concept), because .in('id', [X, X]) with duplicate IDs returns only 1 row. Similarly, schemaRows.length === 2 at line 300 fails when sourceSchemaId === destSchemaId (both endpoints share the same node type schema). This silently skips the findOrCreateTriple call, meaning no discourse relation triple is created in settings for these relations. The codebase explicitly supports self-referencing relations — see apps/obsidian/src/components/canvas/utils/relationJsonUtils.ts:91 which handles relation.sourceId === relation.destinationId.

Was this helpful? React with 👍 or 👎 to provide feedback.

Interesting; I am not sure we want to support that but we don't exclude it yet.

trangdoan982 · 2026-03-09T20:55:54Z

packages/database/scripts/createGroup.ts

@@ -0,0 +1,176 @@
+/**


this script is mostly for my local testing to create group quickly. i've been using it for a while and it doesn't touch any part of the codebase but happy to delete it as well

trangdoan982 · 2026-03-09T21:07:01Z

@maparent here are some big changes since the last time you reviewed:

I addressed your comments on query optimization. Thank you for the notes!
import the relation triple as well (not just relation type). the order is import node type -> relation type -> relation triplet. so the triplet will use whatever is resolved from previous steps
add the "provisional" marker on the imported relation. but no UI change yet. all imported relations will have this field marked to false for now
added importPreview page where we tell users how many schemas and relations are being imported implicitly.

biggest change is how we resolve the endpoint of a relation. now every imported relation's source/dest will resolve to Rid -> this way we can parse both local and remote node using 1 method. my argument is that it's also good to know that the relation was created when the node was imported. but let me know what you think.

Cases I tested:

import relations with nodes imported from the same vault
Vault 1: import node from vault2 -> create a relation btw local node and imported node -> publish local node -> Vault 2 import node from vault 1, see that the relations were resolved correctly

maparent

Ok. Of my comments, two are real code change requests, and after that I'll approve.
Otherwise, thanks for all the great work with optimizations, and I like how you handled fallbacks within the QueryEngine.
I did some testing, that seems to work great.
In terms of behaviour... I'm actually not sure why we need the preview mode in most cases. We're asking people to confirm a choice they just made. There is one good reason: when we import a schema, because it has unfortunate consequences: We cannot edit or remove it. (Also unhappy with that.)
SO two recommendation:

Make handling imported schemas a very high priority after this is merged.
Could we skip the validation screen when there is no schema import?
The latter is a design decision, and may need to be validated with Joel, Johnny and/or Michael. So not sure we should block merge on it. But I think it's worth asking: I really don't see the point of that validation step if there's no schema change.

apps/obsidian/src/services/QueryEngine.ts

maparent · 2026-03-10T23:25:12Z

apps/obsidian/src/utils/importRelations.ts

+          .from("my_concepts")
+          .select("id, source_local_id")
+          .in("id", [sourceSchemaId, destSchemaId]);
+        if (schemaRows && schemaRows.length === 2) {


Interesting; I am not sure we want to support that but we don't exclude it yet.

apps/obsidian/src/utils/importPreview.ts

maparent · 2026-03-10T23:45:48Z

apps/obsidian/src/utils/importPreview.ts

+  // Batch fetch schema_id for all endpoint concepts
+  const conceptIdToSchemaId = new Map<number, number>();
+  if (allEndpointConceptIds.length > 0) {
+    const { data: conceptSchemas } = await client


I think that's right but checking: allConceptEndpointIds should be larger than selectedNotes, right?
I had a doubt because of the filter for local+imported nodes in L195.
(Otherwise I'd think about whether we could get the data for this by adding id to the select at line 86... but almost certainly overkill.)

yes it should >= selectedNodes. you want to add the check here instead of >0 check?

maparent · 2026-03-10T23:49:40Z

apps/obsidian/src/utils/importPreview.ts

+
+    // Check if this triplet already exists in discourseRelations
+    // We need to check using the mapped local ids (match by id first, then by name)
+    const resolveLocalNodeTypeId = (


Not vital, since not hitting the db, but I'd have cached this, it's still a search per instance. Oh well.

not sure what you mean here?

maparent · 2026-03-11T00:59:27Z

I'm having second thoughts. I agreed, and I think it was the right call, that imported schemata should not be edited in the short term. But I think that schemata (imported or not) should be delete-able, as long as they have no live instances. Again, this would do much to mitigate the consequences of importation. We'd need buy-on on that, and I'm willing to help to code it, but I think that importing un-delete-able schemata is a design mistake.
Whether we correct this in this PR or another is a judgment call.

graphite-app · 2026-03-12T22:48:00Z

apps/obsidian/src/utils/importRelations.ts

+            const { mapNodeTypeIdToLocal } = await import("./importNodes.js");
+            mappedSourceNodeTypeId = await mapNodeTypeIdToLocal({
+              plugin,
+              client,
+              sourceSpaceId: spaceId,
+              sourceSpaceUri: spaceUri,
+              sourceNodeTypeId: remoteSourceNodeTypeId,
+            });
+            mappedDestNodeTypeId = await mapNodeTypeIdToLocal({
+              plugin,
+              client,
+              sourceSpaceId: spaceId,
+              sourceSpaceUri: spaceUri,
+              sourceNodeTypeId: remoteDestNodeTypeId,
+            });
+          }


Incorrect Import Statement Inside Loop

The dynamic import statement is executed inside the relation processing loop:

const { mapNodeTypeIdToLocal } = await import("./importNodes.js");

This will attempt to import the module on every iteration, which is inefficient and could cause issues. The import should be moved outside the function at the top of the file:

import { mapNodeTypeIdToLocal } from "./importNodes.js";

Or if circular dependency is a concern, import it once before the loop starts.

Suggested change

const { mapNodeTypeIdToLocal } = await import("./importNodes.js");

mappedSourceNodeTypeId = await mapNodeTypeIdToLocal({

plugin,

client,

sourceSpaceId: spaceId,

sourceSpaceUri: spaceUri,

sourceNodeTypeId: remoteSourceNodeTypeId,

});

mappedDestNodeTypeId = await mapNodeTypeIdToLocal({

plugin,

client,

sourceSpaceId: spaceId,

sourceSpaceUri: spaceUri,

sourceNodeTypeId: remoteDestNodeTypeId,

});

}

mappedSourceNodeTypeId = await mapNodeTypeIdToLocal({

plugin,

client,

sourceSpaceId: spaceId,

sourceSpaceUri: spaceUri,

sourceNodeTypeId: remoteSourceNodeTypeId,

});

mappedDestNodeTypeId = await mapNodeTypeIdToLocal({

plugin,

client,

sourceSpaceId: spaceId,

sourceSpaceUri: spaceUri,

sourceNodeTypeId: remoteDestNodeTypeId,

});

}

Spotted by Graphite

Is this helpful? React 👍 or 👎 to let us know.

Actually this is worth fixing!

maparent · 2026-03-13T13:43:50Z

I approved but the checks are all failing. More annoying, the validate check passes locally, and the error message is obviously wrong.
The lint issue is real, and has to do with createGroup.ts. You could fix or remove the file.
If you fix, it's asking about adding a variable to turbo, I would suppress that warning instead.

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

maparent · 2026-03-13T13:53:23Z

The build error was fixed by rebasing on main. It seems the CI does its own flawed rebasing.

maparent · 2026-03-13T13:54:02Z

Ok still approved but do address the graphite comment, and the lint issue.

trangdoan982 · 2026-03-13T15:54:32Z

yes thank you

This comment was marked as resolved.

Sign in to view

trangdoan982 requested a review from maparent March 2, 2026 15:38

maparent requested changes Mar 4, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

trangdoan982 force-pushed the eng-1301-import-relations-when-a-node-is-imported branch from a03a9e9 to da9c498 Compare March 4, 2026 17:20

This comment was marked as resolved.

Sign in to view

trangdoan982 commented Mar 4, 2026

View reviewed changes

apps/obsidian/src/utils/importRelations.ts Show resolved Hide resolved

trangdoan982 force-pushed the eng-1301-import-relations-when-a-node-is-imported branch from da9c498 to 7a86600 Compare March 9, 2026 15:54

devin-ai-integration bot reviewed Mar 9, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

trangdoan982 commented Mar 9, 2026

View reviewed changes

trangdoan982 requested a review from maparent March 9, 2026 21:07

maparent requested changes Mar 11, 2026

View reviewed changes

trangdoan982 requested a review from maparent March 12, 2026 22:41

graphite-app bot reviewed Mar 12, 2026

View reviewed changes

maparent approved these changes Mar 13, 2026

View reviewed changes

trangdoan982 and others added 7 commits March 13, 2026 09:48

wip

79b7aae

fix lint and format

9b7dd49

Update apps/obsidian/src/utils/importNodes.ts

ffd2a69

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

use subqueries to avoid resolveRelationEndpoints

46df1a2

Update apps/obsidian/src/utils/importRelations.ts

e332f02

Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>

wip

22e6679

separate and optimize query #1

035ae30

trangdoan982 added 9 commits March 13, 2026 09:48

refactor query #2

88d29c8

add import preview

9bffc5a

checkpoint. have it work but need further restructuring

4592b1e

optimize queries with QueryEngine

4dbcd68

add provisional field

c951b06

address PR comment

11e3770

format

fa37ed1

address PR comments

e7c4fbe

address PR comments

3479f98

maparent force-pushed the eng-1301-import-relations-when-a-node-is-imported branch from 3bc25c4 to 3479f98 Compare March 13, 2026 13:50

Conversation

trangdoan982 commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Mar 2, 2026

Uh oh!

supabase bot commented Mar 2, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

maparent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trangdoan982 commented Mar 9, 2026

Uh oh!

maparent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maparent commented Mar 11, 2026

Uh oh!

graphite-app bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maparent commented Mar 13, 2026

Uh oh!

maparent commented Mar 13, 2026

Uh oh!

maparent commented Mar 13, 2026

Uh oh!

trangdoan982 commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

trangdoan982 commented Mar 2, 2026 •

edited

Loading

devin-ai-integration bot Mar 9, 2026 •

edited

Loading

trangdoan982 commented Mar 13, 2026 •

edited

Loading