[ENG-1391] Migrate relations into vault relations.json as objects#748
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
trangdoan982
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request refactors relation persistence from a frontmatter-based system to a dedicated relations.json store, introducing node-instance IDs for node identification. Components throughout the canvas and relationship UI layers are updated to use the new relation store APIs and async operations, with legacy frontmatter integration removed and replaced by a migration pathway. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/obsidian/src/components/canvas/overlays/RelationPanel.tsx`:
- Around line 427-447: The current code always calls addRelation which can
create duplicate relation records; instead, after resolving files with
resolveLinkedFileFromSrc and obtaining sourceId/destId via
getNodeInstanceIdForFile, first check for an existing relation instance that
matches relationTypeId, sourceId and destId (e.g., via the relations store or a
findRelation helper), and if found use its id, otherwise call addRelation;
finally call editor.updateShape to set shape.meta.relationInstanceId to the
existing or newly created id (update the same editor.updateShape/shape.meta
usage).
In `@apps/obsidian/src/components/canvas/utils/relationJsonUtils.ts`:
- Around line 37-46: The helper currently only queries forward-direction
duplicates using findRelationBySourceDestinationType(data, sourceId, destId,
relationTypeId) which allows reversed edges to be added; update the guard to
also check for a reverse-direction match by calling
findRelationBySourceDestinationType(data, destId, sourceId, relationTypeId) (or
extend findRelationBySourceDestinationType to accept a flag for bidirectional
checks), and if either call returns a result return { alreadyExisted: true,
relationInstanceId: existing.id } (use the matching instance's id); keep using
loadRelations and preserve existing return shape.
In `@apps/obsidian/src/components/RelationshipSection.tsx`:
- Around line 213-226: Before calling addRelation, query existing relations
between the resolved node instance IDs and the chosen relation type and skip or
reuse the existing relation ID if found; specifically after resolving sourceId
and destId via getNodeInstanceIdForFile(plugin, activeFile) and
getNodeInstanceIdForFile(plugin, selectedNode), call the repository/query method
that lists relations for a node (or a helper you already have) to check for an
existing relation with type === selectedRelationType and source === sourceId and
destination === destId, and only call addRelation(plugin, { type:
selectedRelationType, source: sourceId, destination: destId }) when no matching
relation exists (or return/reuse the existing relation’s ID instead of inserting
a duplicate).
In `@apps/obsidian/src/utils/relationsStore.ts`:
- Around line 322-334: The code currently calls
removeRelationLinkFromFrontmatter(...) while migrated relations are only
in-memory and then calls saveRelations(plugin, data) afterwards, risking data
loss if saving fails; fix by persisting relations before mutating frontmatter:
either (A) after updating data and setting data.lastModified call await
saveRelations(plugin, data) immediately (or inside the per-file loop) and only
then call removeRelationLinkFromFrontmatter(...) for that file, or (B) if you
prefer batching, defer all removeRelationLinkFromFrontmatter(...) calls until
after a successful await saveRelations(plugin, data) so frontmatter cleanup
happens only after relations are safely saved.
🧹 Nitpick comments (8)
apps/obsidian/src/utils/nodeInstanceId.ts (1)
9-13: Use named parameters forensureNodeInstanceId.This function takes 3 parameters; switching to object destructuring avoids call-site mix‑ups as the API expands.
♻️ Proposed refactor
-export const ensureNodeInstanceId = async ( - plugin: DiscourseGraphPlugin, - file: TFile, - frontmatter: Record<string, unknown>, -): Promise<string> => { +export const ensureNodeInstanceId = async ({ + plugin, + file, + frontmatter, +}: { + plugin: DiscourseGraphPlugin; + file: TFile; + frontmatter: Record<string, unknown>; +}): Promise<string> => {As per coding guidelines: Use named parameters (object destructuring) when a function has more than 2 parameters.
apps/obsidian/src/index.ts (1)
42-44: Consider persisting a migration flag/version to avoid re-scanning the vault on every load.Once migration succeeds, storing a “completed” marker (settings or relations.json version) would keep startup fast in large vaults.
apps/obsidian/src/components/canvas/shapes/DiscourseRelationShape.tsx (1)
1213-1227: Update naming/logging to reflect relations.json persistence (not frontmatter).The method name and error message still reference frontmatter even though persistence moved to relations.json; renaming helps avoid confusion when debugging.
apps/obsidian/src/components/canvas/overlays/RelationPanel.tsx (1)
562-567: Cache node-instance → file resolution to avoid repeated vault scans.
getFileForNodeInstanceIdscans all markdown files each call; doing this inside the loop makes relation loading scale poorly. Consider building a map once percomputeRelationscall.apps/obsidian/src/components/RelationshipSection.tsx (2)
31-34: Add explicit return types to component functions.♻️ Suggested typings
-const AddRelationship = ({ +const AddRelationship = ({ activeFile, onRelationsChange, -}: AddRelationshipProps) => { +}: AddRelationshipProps): JSX.Element => {-const CurrentRelationships = ({ +const CurrentRelationships = ({ activeFile, relationsVersion, -}: CurrentRelationshipsProps) => { +}: CurrentRelationshipsProps): JSX.Element | null => {-export const RelationshipSection = ({ +export const RelationshipSection = ({ activeFile, -}: RelationshipSectionProps) => { +}: RelationshipSectionProps): JSX.Element => {As per coding guidelines: Use explicit return types for functions in TypeScript.
Also applies to: 351-354, 534-552
405-413: Consider cachinggetFileForNodeInstanceIdlookups inloadCurrentRelationships.That helper scans the vault each call; building a one-time map per load avoids O(N*M) behavior as relations grow.
apps/obsidian/src/utils/relationsStore.ts (2)
97-118: Consider documenting the duplicate-handling expectation.
addRelationdoes not check for duplicates (same source/destination/type), which is fine sincemigrateFrontmatterRelationsToRelationsJsonhandles this. However, other callers might inadvertently create duplicates.Consider adding a JSDoc comment clarifying that callers are responsible for deduplication, or optionally add a check:
📝 Optional: Add documentation or duplicate check
+/** + * Adds a new relation to the store. Does not check for duplicates—callers + * should use findRelationBySourceDestinationType beforehand if needed. + */ export const addRelation = async ( plugin: DiscourseGraphPlugin, params: AddRelationParams, ): Promise<string> => {
161-175: Potential performance concern for large vaults.This function performs a linear scan through all markdown files. For vaults with thousands of files, repeated calls could become slow.
If this function is called frequently (e.g., when rendering multiple relations), consider building an in-memory index of
nodeInstanceId → TFilethat's updated on file changes.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const sourceNodeInstanceId = await ensureNodeInstanceId( | ||
| plugin, | ||
| file, | ||
| frontmatter as Record<string, unknown>, | ||
| ); |
There was a problem hiding this comment.
🔴 Migration can generate duplicate nodeInstanceIds for the same file due to stale cache
The migrateFrontmatterRelationsToRelationsJson function can generate multiple different nodeInstanceId values for the same file, causing created relations to reference invalid/orphaned IDs.
Root Cause
When the migration iterates through files, it calls ensureNodeInstanceId which:
- Checks the passed
frontmatterobject for an existingnodeInstanceId - If not found, generates a new UUID and writes it to the file
The bug occurs because:
- When processing file A that links to file B,
ensureNodeInstanceIdis called for B (line 324-328), generating and writingid-B1 - Later when processing file B itself (line 299-303), the
frontmatteris read frommetadataCache.getFileCache(file)which may be stale (not yet reflecting the previous write) - Since the stale
frontmatterhas nonodeInstanceId, a NEW idid-B2is generated and written, overwritingid-B1 - The relation created earlier with destination
id-B1now points to a non-existent node
Impact: Relations created during migration may reference orphaned node IDs, effectively losing the relationship data. This is a data corruption issue that occurs silently during plugin initialization.
Example scenario:
// File A links to B, File B links to A
// Processing A: ensureNodeInstanceId(A) -> "id-A", ensureNodeInstanceId(B) -> "id-B1"
// Relation added: id-A -> id-B1
// Processing B: cache for B is stale, ensureNodeInstanceId(B) -> "id-B2" (overwrites!)
// Now relation id-A -> id-B1 is orphaned
Prompt for agents
Fix the migration function to track already-generated nodeInstanceIds in memory to avoid generating duplicates. Add a Map<string, string> (file.path -> nodeInstanceId) at the start of migrateFrontmatterRelationsToRelationsJson. Before calling ensureNodeInstanceId, check if we already have an ID for that file path in our map. If so, use the cached ID. If not, call ensureNodeInstanceId and store the result in the map. This ensures each file gets exactly one nodeInstanceId regardless of how many times it's encountered during migration (as source or as target of a link).
Was this helpful? React with 👍 or 👎 to provide feedback.
| await addRelation(plugin, { | ||
| type: selectedRelationType, | ||
| source: sourceId, | ||
| destination: destId, | ||
| }); |
There was a problem hiding this comment.
🔴 Relation direction ignored when adding relationship - always uses activeFile as source
When adding a new relation in RelationshipSection.tsx, the code ignores the isSource flag from the selected relation type option. The addRelationship function always stores the relation with activeFile as source and selectedNode as destination (lines 237-241), regardless of whether the user selected a "complement" relation type where the direction should be reversed.
Root Cause
The dropdown selection only stores the relation type ID (option.id) at line 287, losing the isSource information:
onSelect={(option) => option && setSelectedRelationType(option.id)}Then in addRelationship, the relation is always created with the same direction:
await addRelation(plugin, {
type: selectedRelationType,
source: sourceId, // always activeFile
destination: destId, // always selectedNode
});Compare this to RelationPanel.tsx:366-367 which correctly handles direction:
const sourceFile = isSource ? currentFile : targetFile;
const destFile = isSource ? targetFile : currentFile;Impact: When a user selects a complement relation (e.g., "is supported by" instead of "supports"), the relation is stored with incorrect source/destination. This causes relations to display incorrectly when viewed from either node, and the semantic meaning of the relation is inverted.
Prompt for agents
In apps/obsidian/src/components/RelationshipSection.tsx, the AddRelationship component needs to track the isSource flag when the user selects a relation type.
1. Add state to track the selected isSource: add useState for selectedIsSource similar to selectedRelationType
2. Update the DropdownSelect onSelect handler at line 287 to also store the isSource value:
onSelect={(option) => {
if (option) {
setSelectedRelationType(option.id);
setSelectedIsSource(option.isSource);
}
}}
3. In the addRelationship function around lines 237-241, use selectedIsSource to determine the correct source and destination:
const actualSource = selectedIsSource ? sourceId : destId;
const actualDest = selectedIsSource ? destId : sourceId;
await addRelation(plugin, {
type: selectedRelationType,
source: actualSource,
destination: actualDest,
});
4. Add selectedIsSource to the useCallback dependency array.
5. Reset selectedIsSource in resetState function.
Was this helpful? React with 👍 or 👎 to provide feedback.
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
Some changes, but overall we'll be good to go.
trangdoan982
deleted the
eng-1391-migrate-relation-creation-existing-relation-into
branch
2 participants
https://www.loom.com/share/a87c77fe07034cce9daf5fd4b55ab2e3
Summary by CodeRabbit
Release Notes
New Features
Refactor