[ENG-1178] Automatic local remote sync nodes relations #706
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
trangdoan982
force-pushed
the
eng-1178-f9-automatic-local-remote-sync-nodes-relations
branch
from
36b6a0d to
eb65f2c
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughIntroduces FileChangeListener utility to the Obsidian plugin that monitors vault file events, debounces changes, and automatically syncs file modifications to Supabase. Includes refactored sync orchestration utilities across Obsidian and Roam platforms, CORS origin validation for Obsidian app, and sample data file. Changes
Sequence Diagram(s)sequenceDiagram
participant Obsidian as Obsidian Plugin
participant Listener as FileChangeListener
participant Queue as Change Queue
participant Sync as Sync Pipeline
participant Supabase as Supabase
Obsidian->>Listener: initialize()
Listener->>Obsidian: register vault event listeners
Obsidian->>Listener: file event (create/modify/delete/rename)
Listener->>Listener: detect if DG node
Listener->>Queue: queue change (filePath, changeType, oldPath)
Note over Queue: Debounce 5 seconds
Queue->>Sync: [debounce timer] process queued changes
Sync->>Sync: collect changed DG nodes
Sync->>Supabase: syncDiscourseNodeChanges()
Supabase-->>Sync: confirm sync
Sync->>Supabase: cleanupOrphanedNodes()
Supabase-->>Sync: deleted orphan count
Sync-->>Obsidian: sync complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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/utils/fileChangeListener.ts`:
- Around line 149-167: The fallback in handleFileRename is unreachable because
getAbstractFileByPath(oldPath) will return null after a rename; remove that dead
branch and instead track DG node paths: add a private knownDgNodePaths:
Set<string> updated in handleFileCreate (add file.path) and handleFileDelete
(delete file.path), then in handleFileRename use knownDgNodePaths.has(oldPath)
to detect a renamed DG node (delete oldPath and add file.path) and preserve the
existing queueChange(file.path, "title", oldPath) behavior when appropriate;
keep using isDiscourseNode(file) for the normal path and ensure knownDgNodePaths
is kept in sync on all create/delete/rename operations.
In `@apps/website/app/utils/llm/cors.ts`:
- Around line 12-19: The isObsidianOrigin helper is overly permissive
(origin.startsWith("app://")) and redundant given allowedOrigins and the prefix
check in isAllowedOrigin; remove isObsidianOrigin and update isAllowedOrigin to
stop calling it (so isAllowedOrigin relies only on allowedOrigins.includes,
allowedOrigins.some(...), and isVercelPreviewUrl), or if you need a specific
Obsidian allowance, replace isObsidianOrigin with a strict equality check for
the exact Obsidian origin (e.g., compare origin === "app://obsidian.md") and
reference the symbols isObsidianOrigin, isAllowedOrigin, and allowedOrigins when
making the change.
🧹 Nitpick comments (3)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
703-738:cleanupOrphanedNodessilently returns 0 on error.Returning
0on error (line 736) makes it impossible for callers to distinguish between "no orphans found" and "operation failed." This could mask issues during sync initialization.Consider throwing the error or returning a result type that indicates success/failure:
♻️ Suggested approach
-export const cleanupOrphanedNodes = async ( - plugin: DiscourseGraphPlugin, -): Promise<number> => { +type CleanupResult = { success: true; deletedCount: number } | { success: false; error: Error }; + +export const cleanupOrphanedNodes = async ( + plugin: DiscourseGraphPlugin, +): Promise<CleanupResult> => { try { // ... existing logic - return orphanedNodeIds.length; + return { success: true, deletedCount: orphanedNodeIds.length }; } catch (error) { console.error("cleanupOrphanedNodes: Process failed:", error); - return 0; + return { success: false, error: error instanceof Error ? error : new Error(String(error)) }; } };apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
505-509: Regex escaping may be insufficient for special characters.The escaping only handles backslashes and quotes, but regex patterns may contain other special characters (e.g.,
$,^,(,),[,],{,},|,.,*,+,?) that could cause issues when embedded in the Datalog query string.Consider using a more comprehensive escaping approach or validating that
regex.sourcedoesn't contain problematic characters.#!/bin/bash # Check what getDiscourseNodeFormatExpression returns to understand potential regex patterns ast-grep --pattern 'getDiscourseNodeFormatExpression($_) { $$$ }' # Also check existing format expressions in the codebase rg -n "format.*:" --type ts -A2 -B2 | head -100apps/obsidian/src/utils/fileChangeListener.ts (1)
136-144: Consider tracking DG node paths for more efficient delete handling.Since frontmatter isn't available after deletion, any
.mdfile deletion triggers orphan cleanup. This works correctly but may cause unnecessary cleanup calls for non-DG files.A future optimization could maintain a set of known DG node paths to check against during deletion, though the current approach is functionally correct.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/obsidian/src/index.tsapps/obsidian/src/utils/fileChangeListener.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.tsapps/roam/src/utils/syncDgNodesToSupabase.tsapps/website/app/utils/llm/cors.tsnew.json
💤 Files with no reviewable changes (1)
- apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Follow responsive design principles
Prefertypeoverinterfacefor TypeScript
Use explicit return types for functions in TypeScript
Avoidanytypes in TypeScript when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Prefer small, focused functions over inline code
Extract complex logic into well-named functions
Function names should describe their purpose clearly
Choose descriptive function names that make comments unnecessary
Break down complex operations into smaller, meaningful functions
Prefer early returns over nested conditionals for better readability
Prefer util functions for reusable logic and common operations
Add comments only when necessary; descriptive names should minimize the need for comments
Explain the why, not the what in comments, focusing on reasoning, trade-offs, and approaches
Document limitations, known bugs, or edge cases where behavior may not align with expectations
Files:
apps/obsidian/src/utils/fileChangeListener.tsapps/roam/src/utils/syncDgNodesToSupabase.tsapps/website/app/utils/llm/cors.tsapps/obsidian/src/index.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
{packages/ui/**,apps/**}/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use platform-native UI components first with shadcn/ui as a fallback
Files:
apps/obsidian/src/utils/fileChangeListener.tsapps/roam/src/utils/syncDgNodesToSupabase.tsapps/website/app/utils/llm/cors.tsapps/obsidian/src/index.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
apps/obsidian/**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
Prefer existing dependencies from package.json for the Obsidian plugin
Files:
apps/obsidian/src/utils/fileChangeListener.tsapps/obsidian/src/index.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
apps/obsidian/**/*.{md,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/obsidian/AGENTS.md)
apps/obsidian/**/*.{md,ts,tsx,js,jsx}: Utilize the icon anchor in embedded images to tweak spacing around icons for proper alignment with adjacent text
Icons should be surrounded by parenthesis in references (e.g., ( lucide-cog.svg > icon ))
Files:
apps/obsidian/src/utils/fileChangeListener.tsapps/obsidian/src/index.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
apps/roam/**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (apps/roam/AGENTS.md)
Prefer existing dependencies from package.json
Files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/roam/AGENTS.md)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj for API integration
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU for extension development
Files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
🧠 Learnings (13)
📚 Learning: 2025-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-12-07T20:54:20.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:20.007Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
apps/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-08-26T01:52:35.443Z
Learnt from: mdroidian
Repo: DiscourseGraphs/discourse-graph PR: 369
File: apps/roam/src/utils/pluginTimer.ts:14-21
Timestamp: 2025-08-26T01:52:35.443Z
Learning: In the discourse-graph codebase, initPluginTimer() is only called once during the runExtension flow in apps/roam/src/index.ts, so re-initialization scenarios are not a concern for this specific usage pattern.
Applied to files:
apps/obsidian/src/index.ts
📚 Learning: 2025-07-21T14:22:20.752Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
Applied to files:
apps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
apps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
apps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
apps/obsidian/src/utils/syncDgNodesToSupabase.ts
🧬 Code graph analysis (1)
apps/obsidian/src/index.ts (1)
apps/obsidian/src/utils/fileChangeListener.ts (1)
FileChangeListener(22-310)
🪛 Biome (2.1.2)
new.json
[error] 1-2: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 2-2: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 3-330: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 330-330: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 331-345: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 346-346: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 346-346: Missing closing quote
The closing quote must be on the same line.
(parse)
[error] 347-347: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 347-347: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 348-348: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
🔇 Additional comments (15)
apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
588-635: LGTM -syncSpecificBlocksimplementation follows existing patterns.The function correctly:
- Validates client/context availability before proceeding
- Filters node types by
backedBy === "user"- Uses a baseline timestamp of epoch for targeted syncs
- Logs progress and errors appropriately
- Re-throws errors to allow caller handling
The architecture mirrors the Obsidian
syncSpecificFilesapproach, maintaining consistency across platforms.apps/obsidian/src/utils/syncDgNodesToSupabase.ts (2)
553-557: LGTM - Concept upsert optimization is correct.Only upserting concepts for nodes with title changes is a sensible optimization since concepts store the title, and content-only changes don't affect the concept representation. The comment clearly explains the reasoning.
625-635:syncSpecificFilesis not currently used in the codebase and has inaccurate documentation.The function is exported and documented as "Used by FileChangeListener," but FileChangeListener actually calls
syncDiscourseNodeChangesdirectly (line 262 offileChangeListener.ts), notsyncSpecificFiles. Additionally,syncSpecificFilesis never imported or called anywhere else in the codebase. Either remove this unused function or update it with accurate documentation if it's intended as a public utility API. If kept, consider allowing callers to specify change types rather than hardcoding["content"].Likely an incorrect or invalid review comment.
apps/obsidian/src/index.ts (2)
48-55: LGTM - FileChangeListener integration follows established patterns.The integration correctly:
- Only initializes when
syncModeEnabledis true (same guard asinitializeSupabaseSync)- Uses try/catch with graceful fallback to
nullon failure- Follows the same defensive pattern used for
TagNodeHandler(lines 130-136)
366-369: LGTM - Cleanup follows the established pattern.The cleanup correctly calls
cleanup()before setting tonull, consistent withtagNodeHandlercleanup above (lines 361-364).apps/obsidian/src/utils/fileChangeListener.ts (10)
1-17: LGTM!Imports are minimal and appropriate. The
QueuedChangetype andDEBOUNCE_DELAY_MSconstant are well-defined, following naming conventions.
22-33: LGTM!Class properties are well-organized with appropriate private visibility. The state management pattern with
isProcessingandhasPendingOrphanCleanupflags is solid for preventing race conditions.
38-82: LGTM!Event registration follows Obsidian's API patterns correctly. The separation between vault events (using
EventRef) and metadata cache events (using callback reference) is appropriate given the different APIs.
87-106: LGTM!The DG node detection logic is thorough—checking file type, extension, frontmatter presence, and validating against configured node types.
111-131: LGTM!The create handler correctly queues both
titleandcontentchanges for new nodes, while the modify handler appropriately queues onlycontentchanges.
172-189: LGTM!The placeholder is well-documented and correctly filters to only DG nodes. Good to have the metadata change hook in place for future relation sync implementation.
194-216: LGTM!Clean implementation of change queuing with proper merging of change types and preservation of the original
oldPathfor renames.
221-229: LGTM!Standard debounce implementation with proper cleanup of the previous timer.
234-281: LGTM!Solid queue processing with proper concurrency guard. The error handling correctly preserves the queue and orphan cleanup flag for potential retries.
Consider adding exponential backoff or a retry limit in the future to prevent infinite retry loops on persistent failures, but this is acceptable for initial implementation.
286-309: LGTM!Thorough cleanup implementation that properly removes all event listeners and resets internal state. The type assertion for the metadata callback is necessary due to Obsidian API typing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private handleFileRename(file: TAbstractFile, oldPath: string): void { | ||
| if (!this.isDiscourseNode(file)) { | ||
| // Check if the old file was a DG node (in case it lost nodeTypeId) | ||
| const oldFile = this.plugin.app.vault.getAbstractFileByPath(oldPath); | ||
| if (oldFile instanceof TFile) { | ||
| const oldCache = this.plugin.app.metadataCache.getFileCache(oldFile); | ||
| if (oldCache?.frontmatter?.nodeTypeId) { | ||
| console.debug( | ||
| `File renamed from DG node: ${oldPath} -> ${file.path}`, | ||
| ); | ||
| this.queueChange(file.path, "title", oldPath); | ||
| } | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| console.debug(`File renamed: ${oldPath} -> ${file.path}`); | ||
| this.queueChange(file.path, "title", oldPath); | ||
| } |
There was a problem hiding this comment.
Bug: Unreachable fallback logic for renamed files.
The fallback block (lines 151-161) attempts to check if the old file was a DG node by calling getAbstractFileByPath(oldPath). However, the rename event fires after the rename completes, so the file at oldPath no longer exists—this will always return null.
If you need to detect DG nodes that lost their nodeTypeId during rename, consider either:
- Maintaining a set of known DG node paths that gets updated on create/delete
- Checking the metadata cache using the new path (since the file object is the same, just moved)
Proposed fix: Remove dead code or implement path tracking
Option 1 - Remove unreachable code:
private handleFileRename(file: TAbstractFile, oldPath: string): void {
if (!this.isDiscourseNode(file)) {
- // Check if the old file was a DG node (in case it lost nodeTypeId)
- const oldFile = this.plugin.app.vault.getAbstractFileByPath(oldPath);
- if (oldFile instanceof TFile) {
- const oldCache = this.plugin.app.metadataCache.getFileCache(oldFile);
- if (oldCache?.frontmatter?.nodeTypeId) {
- console.debug(
- `File renamed from DG node: ${oldPath} -> ${file.path}`,
- );
- this.queueChange(file.path, "title", oldPath);
- }
- }
return;
}
console.debug(`File renamed: ${oldPath} -> ${file.path}`);
this.queueChange(file.path, "title", oldPath);
}Option 2 - Track known DG paths (add to class):
private knownDgNodePaths: Set<string> = new Set();
// Update in handleFileCreate after isDiscourseNode check:
this.knownDgNodePaths.add(file.path);
// Update in handleFileDelete:
this.knownDgNodePaths.delete(file.path);
// Then in handleFileRename:
if (!this.isDiscourseNode(file)) {
if (this.knownDgNodePaths.has(oldPath)) {
this.knownDgNodePaths.delete(oldPath);
// Handle the case where a DG node was renamed but lost its nodeTypeId
}
return;
}
this.knownDgNodePaths.delete(oldPath);
this.knownDgNodePaths.add(file.path);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleFileRename(file: TAbstractFile, oldPath: string): void { | |
| if (!this.isDiscourseNode(file)) { | |
| // Check if the old file was a DG node (in case it lost nodeTypeId) | |
| const oldFile = this.plugin.app.vault.getAbstractFileByPath(oldPath); | |
| if (oldFile instanceof TFile) { | |
| const oldCache = this.plugin.app.metadataCache.getFileCache(oldFile); | |
| if (oldCache?.frontmatter?.nodeTypeId) { | |
| console.debug( | |
| `File renamed from DG node: ${oldPath} -> ${file.path}`, | |
| ); | |
| this.queueChange(file.path, "title", oldPath); | |
| } | |
| } | |
| return; | |
| } | |
| console.debug(`File renamed: ${oldPath} -> ${file.path}`); | |
| this.queueChange(file.path, "title", oldPath); | |
| } | |
| private handleFileRename(file: TAbstractFile, oldPath: string): void { | |
| if (!this.isDiscourseNode(file)) { | |
| return; | |
| } | |
| console.debug(`File renamed: ${oldPath} -> ${file.path}`); | |
| this.queueChange(file.path, "title", oldPath); | |
| } |
🤖 Prompt for AI Agents
In `@apps/obsidian/src/utils/fileChangeListener.ts` around lines 149 - 167, The
fallback in handleFileRename is unreachable because
getAbstractFileByPath(oldPath) will return null after a rename; remove that dead
branch and instead track DG node paths: add a private knownDgNodePaths:
Set<string> updated in handleFileCreate (add file.path) and handleFileDelete
(delete file.path), then in handleFileRename use knownDgNodePaths.has(oldPath)
to detect a renamed DG node (delete oldPath and add file.path) and preserve the
existing queueChange(file.path, "title", oldPath) behavior when appropriate;
keep using isDiscourseNode(file) for the normal path and ensure knownDgNodePaths
is kept in sync on all create/delete/rename operations.
| const deleteNodesFromSupabase = async ( | ||
| nodeInstanceIds: string[], | ||
| supabaseClient: DGSupabaseClient, | ||
| spaceId: number, | ||
| ): Promise<void> => { | ||
| try { | ||
| if (nodeInstanceIds.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const { data: contentData, error: contentError } = await supabaseClient | ||
| .from("Content") | ||
| .select("id") | ||
| .eq("space_id", spaceId) | ||
| .in("source_local_id", nodeInstanceIds); | ||
|
|
||
| if (contentError) { | ||
| console.error("Failed to get content from Supabase:", contentError); | ||
| } | ||
|
|
||
| const contentIds = contentData?.map((c: { id: number }) => c.id) || []; | ||
|
|
||
| if (contentIds.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const { error: conceptError } = await supabaseClient | ||
| .from("Concept") | ||
| .delete() | ||
| .in("represented_by_id", contentIds) | ||
| .eq("is_schema", false); | ||
|
|
||
| if (conceptError) { | ||
| console.error("Failed to delete concepts from Supabase:", conceptError); | ||
| } | ||
|
|
||
| const { error: contentDeleteError } = await supabaseClient | ||
| .from("Content") | ||
| .delete() | ||
| .in("id", contentIds); | ||
|
|
||
| if (contentDeleteError) { | ||
| console.error( | ||
| "Failed to delete content from Supabase:", | ||
| contentDeleteError, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error in deleteNodesFromSupabase:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Silent failures in deleteNodesFromSupabase may hide data inconsistencies.
Errors during concept deletion (line 100-102) and content deletion (line 109-113) are logged but execution continues. This means the caller won't know if the deletion partially failed, potentially leaving orphaned records in one table but not another.
Consider either:
- Throwing on error to allow caller handling
- Returning a result object indicating partial success/failure
- Using a transaction-like pattern (though Supabase doesn't support transactions natively)
💡 Possible approach using result object
+type DeleteResult = {
+ success: boolean;
+ conceptsDeleted: boolean;
+ contentDeleted: boolean;
+};
+
const deleteNodesFromSupabase = async (
nodeInstanceIds: string[],
supabaseClient: DGSupabaseClient,
spaceId: number,
-): Promise<void> => {
+): Promise<DeleteResult> => {
+ const result: DeleteResult = { success: true, conceptsDeleted: false, contentDeleted: false };
// ... existing logic, updating result accordingly
+ return result;
};There was a problem hiding this comment.
agreed this is worth looking at.
Also: Delete Documents?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to Documents table or documents-related tables in the codebase
echo "=== Searching for 'Documents' table references ==="
rg -i "from\(['\"]Documents['\"]" --type=ts -C2
echo -e "\n=== Searching for table names in database types ==="
rg "from\(['\"]" packages/database/ --type=ts -A1 | head -50
echo -e "\n=== Searching for schema/migration files ==="
fd -e sql -e migration . | head -20
echo -e "\n=== Searching for table creation statements ==="
rg "CREATE TABLE" -i --type=sql -C2 | head -50
echo -e "\n=== Searching for Content and Concept table relationships ==="
rg "represented_by_id|embedding_inline" packages/database/ --type=ts -C3
Length of output: 17746
🏁 Script executed:
#!/bin/bash
# Search for Document table schema and relationships
echo "=== Document table schema and relationships ==="
rg -i "CREATE TABLE.*Document" packages/database/supabase/migrations/ --type=sql -A20
echo -e "\n=== Content table references to Document ==="
rg "document_id" packages/database/supabase/migrations/ --type=sql -C3
echo -e "\n=== Document references in TypeScript types ==="
rg "document_id|Document:" packages/database/src/dbTypes.ts -C2
echo -e "\n=== Foreign key constraints for Document ==="
rg "FOREIGN KEY.*document|document.*REFERENCES" packages/database/supabase/migrations/ --type=sql -C2
echo -e "\n=== Check for CASCADE deletes ==="
rg "ON DELETE CASCADE|ON DELETE" packages/database/supabase/migrations/ --type=sql -C1 | grep -i document
Length of output: 77066
@maparent, you're absolutely right on both counts!
1. Silent failures: The error handling currently logs but continues, so the caller has no visibility into partial failures. This could leave orphaned records in either the Concept or Content tables.
2. Missing Document deletion: The function is missing a critical step. Looking at the schema, each discourse node corresponds to a Document record, and Content has a foreign key to Document with ON DELETE CASCADE. The current code deletes Concepts and Content but leaves the Document records orphaned.
The deletion order should be:
- Delete Concepts (by
represented_by_id) - Delete Documents (which will CASCADE delete Content automatically)
Or keep the current explicit deletions but add Document cleanup at the end. The CASCADE relationship means deleting Documents first is cleaner and ensures Content is always removed.
Should I plan changes to:
- Add Document deletion to
deleteNodesFromSupabase? - Improve error handling (throw vs return status)?
🧬 Code Graph Analysis Results
<no_relevant_code_snippets>
🧠 Learnings used
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
apps/website/app/utils/llm/cors.ts
Outdated
| const isObsidianOrigin = (origin: string): boolean => | ||
| origin.startsWith("app://"); | ||
|
|
||
| const isAllowedOrigin = (origin: string): boolean => | ||
| allowedOrigins.includes(origin) || | ||
| allowedOrigins.some((allowed) => origin.startsWith(allowed)) || | ||
| isVercelPreviewUrl(origin); | ||
| isVercelPreviewUrl(origin) || | ||
| isObsidianOrigin(origin); |
There was a problem hiding this comment.
Security concern: isObsidianOrigin is overly permissive.
The check origin.startsWith("app://") allows any app:// scheme origin, not just Obsidian. Since "app://obsidian.md" is already in the allowedOrigins array (line 6), and the existing allowedOrigins.some((allowed) => origin.startsWith(allowed)) on line 17 already handles prefix matching, this additional check is both redundant and less secure.
Consider removing isObsidianOrigin entirely or restricting it to the specific Obsidian origin:
🔒 Suggested fix
-const isObsidianOrigin = (origin: string): boolean =>
- origin.startsWith("app://");
-
const isAllowedOrigin = (origin: string): boolean =>
allowedOrigins.includes(origin) ||
allowedOrigins.some((allowed) => origin.startsWith(allowed)) ||
- isVercelPreviewUrl(origin) ||
- isObsidianOrigin(origin);
+ isVercelPreviewUrl(origin);🤖 Prompt for AI Agents
In `@apps/website/app/utils/llm/cors.ts` around lines 12 - 19, The
isObsidianOrigin helper is overly permissive (origin.startsWith("app://")) and
redundant given allowedOrigins and the prefix check in isAllowedOrigin; remove
isObsidianOrigin and update isAllowedOrigin to stop calling it (so
isAllowedOrigin relies only on allowedOrigins.includes,
allowedOrigins.some(...), and isVercelPreviewUrl), or if you need a specific
Obsidian allowance, replace isObsidianOrigin with a strict equality check for
the exact Obsidian origin (e.g., compare origin === "app://obsidian.md") and
reference the symbols isObsidianOrigin, isAllowedOrigin, and allowedOrigins when
making the change.
a3cae4d to
eb65f2c
Compare
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
I like the queue and debounce! But I can think of edge cases.
I think that handling changes in metadata especially may require more thinking than what I wrote in the comments.
| * Handle metadata changes (placeholder for relation metadata) | ||
| */ | ||
| private handleMetadataChange(file: TFile): void { | ||
| if (!this.isDiscourseNode(file)) { |
There was a problem hiding this comment.
What of the case where a discourseNode stops being a discourseNode because someone removed the nodeTypeId was removed?
Not sure you would have access to the old value in the metadataCache, OTH?
Also: What happens if someone changes or removes the nodeTypeId?
| } | ||
| } | ||
|
|
||
| this.changeQueue.clear(); |
There was a problem hiding this comment.
This comes after a lot of async work; new changes may have happened in the meantime and be lost. I suggest making the clear immediately after line 251, when the events are copied, so getting the queue is atomic. Then you'd want a try/catch, so you can put elements back if it fails... and then worry about recurring errors, or partial treatment...
Ok even better: I suggest dequeuing elements one by one as they are processed.
| return; | ||
| } | ||
|
|
||
| this.pendingCreates.add(file.path); |
There was a problem hiding this comment.
trying to understand why you add non-DNode files to pendingCreates. Is it because they might become DNodes? But then that would be handled in the handleMetadataChange, right?
I may be missing something but this does not feel needed.
| } | ||
|
|
||
| return ( | ||
| data |
There was a problem hiding this comment.
You're getting both variants here (which I think is right) so you may want to pipe it through a set or deduplicate somehow.
| const deleteNodesFromSupabase = async ( | ||
| nodeInstanceIds: string[], | ||
| supabaseClient: DGSupabaseClient, | ||
| spaceId: number, | ||
| ): Promise<void> => { | ||
| try { | ||
| if (nodeInstanceIds.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const { data: contentData, error: contentError } = await supabaseClient | ||
| .from("Content") | ||
| .select("id") | ||
| .eq("space_id", spaceId) | ||
| .in("source_local_id", nodeInstanceIds); | ||
|
|
||
| if (contentError) { | ||
| console.error("Failed to get content from Supabase:", contentError); | ||
| } | ||
|
|
||
| const contentIds = contentData?.map((c: { id: number }) => c.id) || []; | ||
|
|
||
| if (contentIds.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const { error: conceptError } = await supabaseClient | ||
| .from("Concept") | ||
| .delete() | ||
| .in("represented_by_id", contentIds) | ||
| .eq("is_schema", false); | ||
|
|
||
| if (conceptError) { | ||
| console.error("Failed to delete concepts from Supabase:", conceptError); | ||
| } | ||
|
|
||
| const { error: contentDeleteError } = await supabaseClient | ||
| .from("Content") | ||
| .delete() | ||
| .in("id", contentIds); | ||
|
|
||
| if (contentDeleteError) { | ||
| console.error( | ||
| "Failed to delete content from Supabase:", | ||
| contentDeleteError, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error in deleteNodesFromSupabase:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
agreed this is worth looking at.
Also: Delete Documents?
| ): Map<string, ChangeType[]> => { | ||
| const changeTypesByPath = new Map<string, ChangeType[]>(); | ||
|
|
||
| for (const change of fileChanges) { |
There was a problem hiding this comment.
This looks like maybe it could be written more tersely using Array.reduce?
But I think it would be even simpler and clearer to do it at the moment of addition, in the fileChangeListeners.
https://www.loom.com/share/f4d56e1519934b93b2a623a4e587d69c
https://www.loom.com/share/2116c7341cc8462aa5e03e0c4a017ec8
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.