Skip to content

Conversation

@trangdoan982
Copy link
Collaborator

@trangdoan982 trangdoan982 commented Jan 15, 2026

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

https://www.loom.com/share/2116c7341cc8462aa5e03e0c4a017ec8

Summary by CodeRabbit

Release Notes

  • New Features
    • Obsidian plugin now automatically monitors file changes and synchronizes updates to the Discourse Graph database with debounced processing.
    • Enhanced node synchronization capabilities with support for tracking file renames, deletions, and content changes.
    • Improved change detection to identify and clean up orphaned nodes during synchronization.
    • Added support for Obsidian app origins in request validation.

✏️ Tip: You can customize this high-level summary in your review settings.

@linear
Copy link

linear bot commented Jan 15, 2026

@supabase
Copy link

supabase bot commented Jan 15, 2026

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 ↗︎.

@trangdoan982 trangdoan982 force-pushed the eng-1178-f9-automatic-local-remote-sync-nodes-relations branch from 36b6a0d to eb65f2c Compare January 15, 2026 17:03
@trangdoan982
Copy link
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Introduces 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

Cohort / File(s) Summary
Obsidian Plugin Initialization
apps/obsidian/src/index.ts
Adds FileChangeListener instantiation in onload with error handling and cleanup in unload; new listener field with conditional initialization and null reset.
Obsidian File Change Detection
apps/obsidian/src/utils/fileChangeListener.ts
New FileChangeListener class that monitors Obsidian vault events (create, modify, delete, rename), debounces file changes per path (5s delay), detects DG nodes via frontmatter validation, queues changes by type, processes queued changes calling syncDiscourseNodeChanges and cleanupOrphanedNodes.
Obsidian Sync Pipeline
apps/obsidian/src/utils/syncDgNodesToSupabase.ts, apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
Removes "new" from ChangeType union; introduces DiscourseNodeFileChange type and public sync orchestration functions (syncDiscourseNodeChanges, syncSpecificFiles, cleanupOrphanedNodes); adds path-based node collection and change-type merging helpers; refactors createOrUpdateDiscourseEmbedding to call syncChangedNodesToSupabase; initializes orphan cleanup post-sync. Removes "new" change-type shortcut logic from variant determination.
Roam Sync Pipeline
apps/roam/src/utils/syncDgNodesToSupabase.ts
Adds getDiscourseNodesByUids helper to fetch DG nodes by block UIDs and syncChangedNodesToSupabase to upsert content/embeddings; introduces public syncSpecificBlocks(blockUids) to orchestrate Roam block syncing.
Website CORS Configuration
apps/website/app/utils/llm/cors.ts
Adds isObsidianOrigin helper to detect "app://" origins; extends isAllowedOrigin to permit Obsidian app origins alongside existing whitelist.
Data File
new.json
New root-level JSON file containing two document entries with embedding vectors, variant metadata, and change-type representations.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title references ENG-1178 and mentions 'Automatic local remote sync nodes relations', which aligns with the PR's core objective of syncing discourse nodes between local Obsidian vault and remote Supabase, including handling node relations and orphaned nodes.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: cleanupOrphanedNodes silently returns 0 on error.

Returning 0 on 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.source doesn'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 -100
apps/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 .md file 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

📥 Commits

Reviewing files that changed from the base of the PR and between 709c3af and eb65f2c.

📒 Files selected for processing (7)
  • apps/obsidian/src/index.ts
  • apps/obsidian/src/utils/fileChangeListener.ts
  • apps/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/website/app/utils/llm/cors.ts
  • new.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
Prefer type over interface for TypeScript
Use explicit return types for functions in TypeScript
Avoid any types 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.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/website/app/utils/llm/cors.ts
  • apps/obsidian/src/index.ts
  • apps/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.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/website/app/utils/llm/cors.ts
  • apps/obsidian/src/index.ts
  • apps/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.ts
  • apps/obsidian/src/index.ts
  • apps/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.ts
  • apps/obsidian/src/index.ts
  • apps/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.ts
  • apps/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.ts
  • apps/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.ts
  • apps/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.ts
  • apps/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.ts
  • apps/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 - syncSpecificBlocks implementation 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 syncSpecificFiles approach, 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: syncSpecificFiles is not currently used in the codebase and has inaccurate documentation.

The function is exported and documented as "Used by FileChangeListener," but FileChangeListener actually calls syncDiscourseNodeChanges directly (line 262 of fileChangeListener.ts), not syncSpecificFiles. Additionally, syncSpecificFiles is 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 syncModeEnabled is true (same guard as initializeSupabaseSync)
  • Uses try/catch with graceful fallback to null on 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 to null, consistent with tagNodeHandler cleanup above (lines 361-364).

apps/obsidian/src/utils/fileChangeListener.ts (10)

1-17: LGTM!

Imports are minimal and appropriate. The QueuedChange type and DEBOUNCE_DELAY_MS constant are well-defined, following naming conventions.


22-33: LGTM!

Class properties are well-organized with appropriate private visibility. The state management pattern with isProcessing and hasPendingOrphanCleanup flags 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 title and content changes for new nodes, while the modify handler appropriately queues only content changes.


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 oldPath for 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.

@trangdoan982 trangdoan982 force-pushed the eng-1178-f9-automatic-local-remote-sync-nodes-relations branch from a3cae4d to eb65f2c Compare January 15, 2026 17:54
@trangdoan982 trangdoan982 requested a review from maparent January 15, 2026 21:46
Copy link
Collaborator

@maparent maparent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return;
}

this.pendingCreates.add(file.path);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved during sync meeting: newly created node might not have nodeTypeId yet -> miss being in the queue. but agreed that in the future handleMetadataChangess will handle this case

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we resolved it, but not sure anymore. It is handled by handleMetadataChanges now, afaik. (In the future: of that file's lifecycle, not in a future version of the code, right?)
So I still don't see why pendingCreates exists.

@trangdoan982 trangdoan982 force-pushed the eng-1178-f9-automatic-local-remote-sync-nodes-relations branch from 97b3983 to 777b079 Compare January 20, 2026 17:21
@trangdoan982 trangdoan982 requested a review from maparent January 20, 2026 17:21
Copy link
Collaborator

@maparent maparent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one pending coderabbit issue, and I may be missing something but I still don't see why we need pendingCreates.

@trangdoan982
Copy link
Collaborator Author

@maparent my bad i missed that one comment somehow. just addressed it. the pendingCreates is because when file is first created, the event is triggered, but to update the nodeTypeId to turn it into a Discourse node is an async function, so the pendingCreates make sure we actually process the files when it has the nodeTypeId in the frontmatter, not just when files are created.
but we also agreed that it should be eventually moved to handleMetadataChange, but pushing back this change until we have a clear mechanism to sync relations instances as well.

@trangdoan982 trangdoan982 requested a review from maparent January 20, 2026 23:43
Copy link
Collaborator

@maparent maparent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed on why you need pendingCreates. Please just add a comment to the effect that, if someone creates a file, quits and relaunches obsidian, and then makes it a discourse node, it won't be caught until the next global sync operation (probably through another obsidian relaunch.)
But I think that's an acceptable limitation.
(The alternative is to hit the db every time we have a metadata change, which is also a bit much.)

@maparent maparent requested a review from mdroidian January 21, 2026 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants