-
Notifications
You must be signed in to change notification settings - Fork 4
[ENG-1176] Sync DG node from Obsidian to database #665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis 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
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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 |
There was a problem hiding this 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.logwhich will appear in production. Consider usingconsole.debugconsistently. Also, whenerroris truthy from the query, it's silently ignored andDEFAULT_TIMEis 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
errorfrom Supabase RPC is aPostgrestErrorobject, not anErrorinstance. Theinstanceof Errorcheck will always be false. The error object has amessageproperty 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.debugto 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 forlocal_reference_contentto avoid type assertions.The
as string[]assertion relies onlocal_reference_contentcontaining 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 forlocal_reference_contentto 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
fetchcall to the embeddings API has no timeout. If the API is slow or unresponsive, this could block the sync indefinitely. Consider usingAbortControllerwith 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?.userIdis 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.
batchSizeduplicates theEMBEDDING_BATCH_SIZEconstant 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
📒 Files selected for processing (5)
apps/obsidian/src/utils/conceptConversion.tsapps/obsidian/src/utils/registerCommands.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/utils/upsertNodesAsContentWithEmbeddings.tsapps/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
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes 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.tsapps/website/app/utils/llm/cors.tsapps/obsidian/src/utils/conceptConversion.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/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.tsapps/obsidian/src/utils/conceptConversion.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/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.tsapps/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.tsapps/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.tsapps/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.tsapps/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.mdenables 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, whilestartsWith()on line 14 would also match those same origins. The current logic is correct and theincludes()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
syncModeEnabledsetting- Uses the
checkCallbackpattern appropriately- Provides user feedback via
Noticefor 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 viaprocessFrontMatteris the appropriate Obsidian API for this.
349-352: Silent catch may hide critical sync failures.The
.catch()only logs the error but allowsinitializeSupabaseSyncto 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.
nodesTypesToLocalConceptsconverts 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 missingon line 102 could beconstif 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.
| // 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 | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| // 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.
67907da to
52c27e2
Compare
d53a70e to
9fd200b
Compare
9fd200b to
baa665d
Compare
52c27e2 to
5c3819e
Compare
5c3819e to
42d3d0f
Compare
baa665d to
3f40d5c
Compare
efaa9f9 to
4faa158
Compare
| const allowedOrigins = [ | ||
| "https://roamresearch.com", | ||
| "http://localhost:3000", | ||
| "app://obsidian.md", |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!)
3f40d5c to
1006400
Compare
4faa158 to
015387d
Compare
1006400 to
ee79a54
Compare
015387d to
45c80e5
Compare
45c80e5 to
c973f5c
Compare

https://www.loom.com/share/13afdd2b3eb247d1af5012714085ecd3
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.