Skip to content

Conversation

@trangdoan982
Copy link
Collaborator

@trangdoan982 trangdoan982 commented Jan 3, 2026

https://www.loom.com/share/13afdd2b3eb247d1af5012714085ecd3

Summary by CodeRabbit

Release Notes

  • New Features

    • Added "Sync Discourse Nodes to Supabase" command to synchronize Obsidian discourse nodes to Supabase with embeddings and dependency management.
  • Improvements

    • Enhanced cross-origin support for Obsidian app integration and improved origin validation logic.

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

@supabase
Copy link

supabase bot commented Jan 3, 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 ↗︎.

Copy link
Collaborator Author

trangdoan982 commented Jan 3, 2026

@trangdoan982 trangdoan982 changed the title sync all nodes on load F1A: Publish DG node from Obsidian to database Jan 3, 2026
@trangdoan982 trangdoan982 marked this pull request as ready for review January 3, 2026 20:22
@trangdoan982 trangdoan982 changed the title F1A: Publish DG node from Obsidian to database [ENG-1176] Publish DG node from Obsidian to database Jan 3, 2026
@linear
Copy link

linear bot commented Jan 3, 2026

@trangdoan982
Copy link
Collaborator Author

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 3, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 3, 2026

📝 Walkthrough

Walkthrough

This PR introduces a complete synchronization pipeline for Obsidian discourse nodes to Supabase. It adds utilities to convert nodes to concepts with dependency ordering, orchestrates sync operations including modification detection, integrates embedding API calls with batched processing, and registers a new Obsidian command to trigger the full sync workflow.

Changes

Cohort / File(s) Summary
Concept Conversion & Ordering
apps/obsidian/src/utils/conceptConversion.ts
New module: exports four functions to convert Obsidian discourse data (schema and instance nodes) to LocalConceptDataInput, derive related concept IDs, and recursively order concepts by dependency; tracks missing dependencies.
Sync Orchestration
apps/obsidian/src/utils/syncDgNodesToSupabase.ts
New orchestrator createOrUpdateDiscourseEmbedding that coordinates sync lifecycle: determines last sync time, detects node modifications/filename changes, converts discourse nodes to concepts with dependency ordering, and upserts via RPC; introduces ObsidianDiscourseNodeData type.
Command Registration
apps/obsidian/src/utils/registerCommands.ts
Adds new Obsidian command "Sync Discourse Nodes to Supabase" gated by syncModeEnabled; calls createOrUpdateDiscourseEmbedding and surfaces success/failure notices.
Content & Embedding Upsert
apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
New module: transforms ObsidianDiscourseNodeData to LocalContentDataInput, batches text inputs (200 per request) to fetch embeddings from /embeddings/openai/small, enriches content entries, and upserts in 200-item batches via Supabase RPC with validation.
CORS Configuration
apps/website/app/utils/llm/cors.ts
Expands allowed origins to include app://obsidian.md; adds direct exact-origin match check alongside existing prefix and Vercel preview logic.

Sequence Diagram(s)

sequenceDiagram
    participant User as Obsidian User
    participant Cmd as registerCommands
    participant Sync as syncDgNodesToSupabase
    participant CConv as conceptConversion
    participant Emb as upsertNodesAsContentWithEmbeddings
    participant EmbAPI as Embeddings API
    participant SB as Supabase RPC
    
    User->>Cmd: Execute "Sync Discourse Nodes" command
    activate Cmd
    Cmd->>Sync: createOrUpdateDiscourseEmbedding()
    activate Sync
    
    rect rgb(230, 245, 250)
      note over Sync: Change Detection Phase
      Sync->>Sync: Determine lastSyncTime
      Sync->>Sync: Collect nodes modified since sync
      Sync->>Sync: Detect filename changes
    end
    
    rect rgb(240, 250, 230)
      note over Sync, CConv: Concept Conversion Phase
      Sync->>CConv: Convert discourse nodes to LocalConceptDataInput
      CConv->>CConv: Extract related concept IDs
      CConv->>CConv: Order concepts by dependency
      CConv-->>Sync: Ordered concepts + missing deps
      Sync->>SB: upsert_concepts RPC (ordered concepts)
    end
    
    rect rgb(250, 240, 230)
      note over Sync, Emb: Embedding & Content Phase
      Sync->>Emb: upsertNodesToSupabaseAsContentWithEmbeddings()
      activate Emb
      Emb->>Emb: Convert nodes to LocalContentDataInput
      Emb->>EmbAPI: POST batch (≤200 texts)
      EmbAPI-->>Emb: Embedding vectors
      Emb->>Emb: Enrich content with embedding_inline
      Emb->>SB: upsert_content RPC (batches ≤200)
      deactivate Emb
    end
    
    Sync-->>Cmd: Sync complete
    deactivate Sync
    Cmd->>User: Show success notice
    deactivate Cmd
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • #343: Implements the same sync/embedding/concept conversion pipeline for a different app integration (Roam), with nearly identical orchestration and conversion logic.
  • #202: Introduces the database-side bulk upsert types and RPC functions (content_local_input, upsert_content) that this PR's embedding and concept upsert code directly targets.
  • #220: Adds equivalent discourse node → local concept conversion utilities (discourseNodeSchemaToLocalConcept, discourseNodeBlockToLocalConcept) for a different app integration.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately describes the main change: syncing Discourse Graph nodes from Obsidian to the database, which aligns with the core functionality added across the changed files.

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: 1

🧹 Nitpick comments (7)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (2)

34-48: Replace debug logging with console.debug; handle query errors explicitly.

Lines 45-46 use console.log which will appear in production. Consider using console.debug consistently. Also, when error is truthy from the query, it's silently ignored and DEFAULT_TIME is returned - this might mask database issues.

🔎 Proposed fix
-  console.log("data", data);
-  console.log("error", error);
-  return new Date(data?.last_modified || DEFAULT_TIME);
+  if (error) {
+    console.error("Failed to fetch last sync time:", error);
+    return DEFAULT_TIME;
+  }
+  console.debug("Last sync data:", data);
+  return new Date(data?.last_modified ?? DEFAULT_TIME);

325-333: Simplify error handling - Supabase errors are already objects.

The error from Supabase RPC is a PostgrestError object, not an Error instance. The instanceof Error check will always be false. The error object has a message property directly.

🔎 Proposed fix
   if (error) {
-    const errorMessage =
-      error instanceof Error
-        ? error.message
-        : typeof error === "string"
-          ? error
-          : JSON.stringify(error, null, 2);
-    throw new Error(`upsert_concepts failed: ${errorMessage}`);
+    throw new Error(`upsert_concepts failed: ${error.message}`);
   }
apps/obsidian/src/utils/conceptConversion.ts (2)

73-76: Replace console.log with console.debug for production code.

Debug logging should use console.debug to allow filtering in production environments.

🔎 Proposed fix
-  console.log(
+  console.debug(
     `[discourseNodeBlockToLocalConcept] Converting concept: represented_by_local_id=${nodeData.nodeInstanceId}, name="${nodeData.file.basename}"`,
   );

82-91: Consider stronger typing for local_reference_content to avoid type assertions.

The as string[] assertion relies on local_reference_content containing only string or array-of-string values. While current usage aligns with this expectation, the field's type (Json | null) doesn't guarantee this. Add a type guard or improve the database type definition for local_reference_content to explicitly specify the expected structure (e.g., Record<string, string | string[]>), eliminating the need for the assertion.

apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts (3)

56-90: Consider adding timeout to external API calls.

The fetch call to the embeddings API has no timeout. If the API is slow or unresponsive, this could block the sync indefinitely. Consider using AbortController with a reasonable timeout.

🔎 Proposed fix
+  const FETCH_TIMEOUT_MS = 30000; // 30 seconds per batch
+
   for (let i = 0; i < allNodesTexts.length; i += EMBEDDING_BATCH_SIZE) {
     const batch = allNodesTexts.slice(i, i + EMBEDDING_BATCH_SIZE);
+    const controller = new AbortController();
+    const timeoutId = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS);
+
     const response = await fetch(nextApiRoot() + "/embeddings/openai/small", {
       method: "POST",
       headers: { "Content-Type": "application/json" },
       body: JSON.stringify({ input: batch }),
+      signal: controller.signal,
     });
+    clearTimeout(timeoutId);

143-146: Silent return on missing context may mask configuration issues.

Returning early without throwing when context?.userId is missing prevents callers from knowing the operation was skipped. Consider throwing an error or returning a status indicator.

🔎 Proposed fix
   if (!context?.userId) {
-    console.error("No Supabase context found.");
-    return;
+    throw new Error("No Supabase context found - userId is required");
   }

177-185: Use EMBEDDING_BATCH_SIZE constant instead of duplicate magic number.

batchSize duplicates the EMBEDDING_BATCH_SIZE constant defined at line 10. Use the constant for consistency.

🔎 Proposed fix
-  const batchSize = 200;
-
-  const chunk = <T>(array: T[], size: number): T[][] => {
-    const chunks: T[][] = [];
-    for (let i = 0; i < array.length; i += size) {
-      chunks.push(array.slice(i, i + size));
-    }
-    return chunks;
-  };
-
   await uploadBatches(
-    chunk(nodesWithEmbeddings, batchSize),
+    chunk(nodesWithEmbeddings, EMBEDDING_BATCH_SIZE),
     supabaseClient,
     context,
   );
 };
+
+const chunk = <T>(array: T[], size: number): T[][] => {
+  const chunks: T[][] = [];
+  for (let i = 0; i < array.length; i += size) {
+    chunks.push(array.slice(i, i + size));
+  }
+  return chunks;
+};
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d53a70e and 67907da.

📒 Files selected for processing (5)
  • apps/obsidian/src/utils/conceptConversion.ts
  • apps/obsidian/src/utils/registerCommands.ts
  • apps/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
  • apps/website/app/utils/llm/cors.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefer type over interface in TypeScript
Use explicit return types for functions
Avoid any types 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
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability

Files:

  • apps/obsidian/src/utils/registerCommands.ts
  • apps/website/app/utils/llm/cors.ts
  • apps/obsidian/src/utils/conceptConversion.ts
  • apps/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
apps/obsidian/**

📄 CodeRabbit inference engine (.cursor/rules/obsidian.mdc)

apps/obsidian/**: Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Follow the Obsidian style guide from help.obsidian.md/style-guide and docs.obsidian.md/Developer+policies for UI and code styling
Use Lucide and custom Obsidian icons alongside detailed elements to provide visual representation of features in platform-native UI

Files:

  • apps/obsidian/src/utils/registerCommands.ts
  • apps/obsidian/src/utils/conceptConversion.ts
  • apps/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
apps/website/**

📄 CodeRabbit inference engine (.cursor/rules/website.mdc)

Prefer existing dependencies from apps/website/package.json when adding dependencies to the website application

Files:

  • apps/website/app/utils/llm/cors.ts
🧠 Learnings (12)
📓 Common learnings
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: 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.
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.
📚 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/conceptConversion.ts
  • apps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-23T20:16:40.021Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-06-23T20:16:40.021Z
Learning: The user maparent can assume acyclicity in dependency graphs for the orderConceptsByDependency function in the discourse-graph repository, which simplifies the implementation by eliminating the need for cycle detection.

Applied to files:

  • apps/obsidian/src/utils/conceptConversion.ts
📚 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/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.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/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.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
📚 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-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/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/obsidian/src/utils/syncDgNodesToSupabase.ts
  • apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.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/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.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/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts
🧬 Code graph analysis (3)
apps/obsidian/src/utils/registerCommands.ts (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
  • createOrUpdateDiscourseEmbedding (210-279)
apps/obsidian/src/utils/conceptConversion.ts (2)
packages/database/src/inputTypes.ts (1)
  • LocalConceptDataInput (22-24)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
  • ObsidianDiscourseNodeData (22-29)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (6)
apps/obsidian/src/utils/supabaseContext.ts (3)
  • SupabaseContext (12-17)
  • getSupabaseContext (56-106)
  • getLoggedInClient (110-165)
apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts (1)
  • upsertNodesToSupabaseAsContentWithEmbeddings (132-192)
apps/obsidian/src/types.ts (1)
  • DiscourseNode (3-13)
apps/obsidian/src/utils/conceptConversion.ts (3)
  • discourseNodeSchemaToLocalConcept (29-48)
  • discourseNodeBlockToLocalConcept (53-77)
  • orderConceptsByDependency (124-145)
packages/database/src/inputTypes.ts (1)
  • LocalConceptDataInput (22-24)
packages/database/src/dbTypes.ts (1)
  • Json (2-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Graphite / mergeability_check
  • GitHub Check: Graphite / mergeability_check
🔇 Additional comments (10)
apps/website/app/utils/llm/cors.ts (2)

3-7: LGTM! Appropriate origin allowlist for Obsidian integration.

Adding app://obsidian.md enables CORS for the Obsidian desktop app, which uses this custom protocol scheme. This aligns with the broader Obsidian-to-Supabase sync functionality introduced in this PR.


12-15: Minor redundancy in origin matching logic.

The includes() check on line 13 will match exact origins, while startsWith() on line 14 would also match those same origins. The current logic is correct and the includes() provides a small optimization for exact matches, but note they overlap for the current allowlist entries.

apps/obsidian/src/utils/registerCommands.ts (1)

136-157: LGTM! Well-structured sync command with proper error handling.

The command correctly:

  • Gates execution behind syncModeEnabled setting
  • Uses the checkCallback pattern appropriately
  • Provides user feedback via Notice for both success and failure
  • Logs errors to console for debugging

The implementation follows the established patterns in this file.

apps/obsidian/src/utils/syncDgNodesToSupabase.ts (3)

53-70: LGTM! Robust nodeInstanceId handling.

The function correctly checks for existing IDs and generates UUIDs when missing using crypto.randomUUID(). The frontmatter update via processFrontMatter is the appropriate Obsidian API for this.


349-352: Silent catch may hide critical sync failures.

The .catch() only logs the error but allows initializeSupabaseSync to complete "successfully". Callers won't know if the initial sync failed. Consider whether this is intentional behavior or if the error should propagate.


297-316: Schema concepts are regenerated on every sync.

nodesTypesToLocalConcepts converts all node types to concepts on every sync, even if they haven't changed. This works due to idempotent upserts (per learnings), but consider caching or diffing schema concepts if performance becomes a concern with many node types.

Based on learnings, idempotent upserts with proper conflict resolution are the preferred approach, so this is acceptable.

apps/obsidian/src/utils/conceptConversion.ts (2)

96-119: Recursive dependency ordering looks correct for acyclic graphs.

Per the retrieved learning, acyclicity can be assumed, so no cycle detection is needed. The algorithm correctly:

  • Processes dependencies before the concept itself
  • Removes processed concepts from remainder to avoid reprocessing
  • Tracks missing dependencies

Minor note: let missing on line 102 could be const if you prefer immutable bindings, but the current approach is functional.


124-145: LGTM! Clean dependency ordering implementation.

The function handles edge cases (empty input, missing represented_by_local_id) and returns both ordered concepts and missing dependencies for caller awareness.

apps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.ts (2)

22-45: LGTM! Clean node-to-content transformation.

The conversion correctly maps Obsidian node data to the LocalContentDataInput format, using file basename as text and preserving frontmatter as metadata.


107-127: LGTM! Batch upload with proper error propagation.

The function correctly iterates over batches, calls the RPC, and throws on error. This aligns with the learning that partial successes should not trigger backoff - only complete failures.

Comment on lines 233 to 253
// For testing: only sync nodes from specific folder
// TODO: Remove this after testing
const testFolderPath =
"/Users/trang.doan/Documents/Trang Doan Obsidian/Trang Doan/testSyncNodes";
const allNodeInstances = await getAllDiscourseNodesSince({
plugin,
lastSyncTime,
supabaseClient,
context,
testFolderPath, // Remove this parameter to sync all nodes
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Remove hardcoded test folder path before merging.

This hardcoded absolute path will prevent the sync from working for any user other than the developer. The TODO comment indicates awareness, but this should not be merged in this state.

🔎 Proposed fix
-    // Get all discourse nodes modified since last sync
-    // For testing: only sync nodes from specific folder
-    // TODO: Remove this after testing
-    const testFolderPath =
-      "/Users/trang.doan/Documents/Trang Doan Obsidian/Trang Doan/testSyncNodes";
     const allNodeInstances = await getAllDiscourseNodesSince({
       plugin,
       lastSyncTime,
       supabaseClient,
       context,
-      testFolderPath, // Remove this parameter to sync all nodes
     });
📝 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.

Suggested change
// For testing: only sync nodes from specific folder
// TODO: Remove this after testing
const testFolderPath =
"/Users/trang.doan/Documents/Trang Doan Obsidian/Trang Doan/testSyncNodes";
const allNodeInstances = await getAllDiscourseNodesSince({
plugin,
lastSyncTime,
supabaseClient,
context,
testFolderPath, // Remove this parameter to sync all nodes
});
const allNodeInstances = await getAllDiscourseNodesSince({
plugin,
lastSyncTime,
supabaseClient,
context,
});
🤖 Prompt for AI Agents
In apps/obsidian/src/utils/syncDgNodesToSupabase.ts around lines 233–243, remove
the hardcoded testFolderPath constant and stop passing testFolderPath to
getAllDiscourseNodesSince so the function will sync all nodes for any user;
either delete the test-only block (and the accompanying TODO) or gate it behind
a clearly named env/config flag (e.g., TEST_SYNC_FOLDER) and read that value
instead so the default behavior is to pass no folder param (or undefined) to
getAllDiscourseNodesSince, ensuring production merges do not contain an absolute
developer file path.

@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 67907da to 52c27e2 Compare January 5, 2026 15:48
@maparent maparent force-pushed the eng-1177-authenticate-obsidian-against-dg-space branch from d53a70e to 9fd200b Compare January 5, 2026 15:48
@trangdoan982 trangdoan982 requested a review from maparent January 5, 2026 15:58
@trangdoan982 trangdoan982 changed the base branch from eng-1177-authenticate-obsidian-against-dg-space to graphite-base/665 January 6, 2026 16:38
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 52c27e2 to 5c3819e Compare January 6, 2026 16:45
@trangdoan982 trangdoan982 changed the base branch from graphite-base/665 to eng-1177-authenticate-obsidian-against-dg-space January 6, 2026 16:45
@trangdoan982 trangdoan982 changed the title [ENG-1176] Publish DG node from Obsidian to database [ENG-1176] Sync DG node from Obsidian to database Jan 6, 2026
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 5c3819e to 42d3d0f Compare January 6, 2026 17:17
@trangdoan982 trangdoan982 force-pushed the eng-1177-authenticate-obsidian-against-dg-space branch from baa665d to 3f40d5c Compare January 6, 2026 17:17
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch 2 times, most recently from efaa9f9 to 4faa158 Compare January 7, 2026 03:53
const allowedOrigins = [
"https://roamresearch.com",
"http://localhost:3000",
"app://obsidian.md",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious if this is portable? I fear it may be Mac-specific.

node: DiscourseNode;
accountLocalId: string;
}): LocalConceptDataInput => {
const now = new Date().toISOString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ouch. I realize you do not have the values for individual schemas in the data.json, but maybe we could use the dates of the data.json file itself? That way we won't get a new value every time we invoke this function.

is_schema: true,
author_local_id: accountLocalId,
created: now,
// TODO: get the template or any other info to put into literal_content jsonb
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say this is worth doing now. Also: The concept has a description field which could be taken straight from the node.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my bad, concept conversion will be followed up on another ticket

const fullContent = await plugin.app.vault.read(node.file);
const fullEntry: LocalContentDataInput = {
...baseEntry,
text: fullContent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, and I think we should remove the yaml frontmatter here; it would mostly belong to the corresponding concept.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We agreed to keep the Yaml front matter. But it should also be put in the corresponding concept as json.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(in another PR, that is!)

@trangdoan982 trangdoan982 force-pushed the eng-1177-authenticate-obsidian-against-dg-space branch from 3f40d5c to 1006400 Compare January 7, 2026 23:46
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 4faa158 to 015387d Compare January 7, 2026 23:46
@trangdoan982 trangdoan982 force-pushed the eng-1177-authenticate-obsidian-against-dg-space branch from 1006400 to ee79a54 Compare January 8, 2026 20:00
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 015387d to 45c80e5 Compare January 8, 2026 20:00
Base automatically changed from eng-1177-authenticate-obsidian-against-dg-space to main January 8, 2026 20:03
@trangdoan982 trangdoan982 force-pushed the 01-03-sync_all_nodes_on_load branch from 45c80e5 to c973f5c Compare January 9, 2026 00:09
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