[ENG-1177] Authenticate Obsidian user for publishing #656
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@trangdoan982: I'll conduct a full review of all the changes in this PR. Let me analyze the code for the Obsidian user authentication feature. ✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughAdds Supabase-backed sync support to the Obsidian plugin: new dependencies, build-time SUPABASE env injection, new settings (spacePassword, accountLocalId, syncModeEnabled), UI toggle to enable sync, and utilities to create/cache Supabase context and logged-in client. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as GeneralSettings UI
participant Plugin as Obsidian Plugin
participant Utils as supabaseContext utils
participant DB as `@repo/database`
participant Supabase as Supabase Service
User->>UI: Toggle "(BETA) Sync mode"
UI->>Plugin: save settings (syncModeEnabled=true)
Plugin->>Utils: initializeSupabaseSync(plugin)
alt context obtained
Utils->>Utils: derive vaultName, accountLocalId, spacePassword
Utils->>DB: fetchOrCreateSpace({..., password, accountLocalId?})
DB-->>Utils: spaceId, space data
Utils->>DB: fetchOrCreatePlatformAccount({spaceId, accountLocalId, accountName?})
DB-->>Utils: platform account, dg_account id
Utils->>Supabase: createLoggedInClient({platform, spaceId, password, accountLocalId?})
Supabase-->>Utils: authenticated client
Utils-->>Plugin: cached context + client
Plugin->>UI: show success Notice
else failure
Utils-->>Plugin: error
Plugin->>UI: show error Notice
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/obsidian/src/index.ts (1)
37-39: Add error handling for sync initialization during startup.The
initializeSupabaseSynccall can throw an error (when context creation fails), but there's no error handling here. Unhandled promise rejections during plugin load could cause silent failures that users won't be aware of.🔎 Proposed error handling
if (this.settings.syncModeEnabled === true) { - void initializeSupabaseSync(this); + void initializeSupabaseSync(this).catch((error) => { + console.error("Failed to initialize Supabase sync on startup:", error); + // Optionally notify user + }); }apps/obsidian/scripts/compile.ts (2)
107-111: Consider removing or limiting environment variable logging.Line 111 logs the entire
dbEnvobject to the console during builds. While the Supabase anon key is meant to be public, other environment values might contain sensitive information. Consider removing this log statement or limiting it to development builds only.🔎 Suggested improvement
const dbEnv = envContents(); - console.log("dbEnv", dbEnv); + // Only log in verbose mode or remove entirely + if (process.env.VERBOSE) { + console.log("Loaded environment variables for bundling"); + }
123-131: Inconsistent fallback handling for environment variables.Lines 124-126 and 127-129 use
nullas the fallback when environment variables are missing, but line 130 uses an empty string ("") forNEXT_API_ROOT. This inconsistency could lead to different runtime behavior.Consider using a consistent fallback strategy:
🔎 Proposed consistency fix
"process.env.SUPABASE_URL": dbEnv.SUPABASE_URL ? `"${dbEnv.SUPABASE_URL}"` : "null", "process.env.SUPABASE_ANON_KEY": dbEnv.SUPABASE_ANON_KEY ? `"${dbEnv.SUPABASE_ANON_KEY}"` : "null", - "process.env.NEXT_API_ROOT": `"${dbEnv.NEXT_API_ROOT || ""}"`, + "process.env.NEXT_API_ROOT": dbEnv.NEXT_API_ROOT + ? `"${dbEnv.NEXT_API_ROOT}"` + : "null",apps/obsidian/src/utils/supabaseContext.ts (2)
21-27: Consider using cryptographically secure random generation for account IDs.The function uses
Math.random()to generate the account local ID suffix. While this may be sufficient for uniqueness, using a cryptographically secure method likecrypto.randomUUID()(which is already used for passwords on line 36) would be more robust and consistent.🔎 Proposed improvement using crypto API
const generateAccountLocalId = (vaultName: string): string => { - const randomSuffix = Math.random() - .toString(36) - .substring(2, 8) - .toUpperCase(); + // Generate 6 random hex characters using crypto API + const randomSuffix = crypto.randomUUID().substring(0, 6).toUpperCase(); return `${vaultName}-${randomSuffix}`; };
55-119: Module-level cache prevents context refresh.Line 19 defines
contextCacheat the module level, and line 58 only initializes context if the cache is null. This means:
- Context is cached for the entire lifetime of the plugin
- No mechanism exists to refresh the context if credentials change
- Multiple plugin instances (unlikely in Obsidian) would share the same cache
While this is likely fine for the Obsidian single-instance model, consider whether users might need to re-initialize or switch contexts during a session.
If context refresh is ever needed, consider:
- Adding a
clearCache()function for testing or resets- Tying the cache to the plugin instance rather than module scope
- Adding a TTL or refresh mechanism
Example cache clearing function
export const clearSupabaseContext = (): void => { contextCache = null; _loggedInClient = null; };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/obsidian/package.jsonapps/obsidian/scripts/compile.tsapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/constants.tsapps/obsidian/src/index.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/scripts/compile.tsapps/obsidian/src/types.tsapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/index.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/constants.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/scripts/compile.tsapps/obsidian/src/types.tsapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/index.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/obsidian/src/constants.tsapps/obsidian/package.json
🧠 Learnings (13)
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:38:52.779Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-31T17:38:52.779Z
Learning: The dbDotEnv functionality in packages/database/src/dbDotEnv.js is designed for Node.js environments only and will not be used in browser contexts.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:37:17.173Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.173Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
apps/obsidian/scripts/compile.tsapps/obsidian/src/utils/supabaseContext.ts
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
apps/obsidian/scripts/compile.tsapps/obsidian/package.json
📚 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/scripts/compile.tsapps/obsidian/package.json
📚 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.tsapps/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/supabaseContext.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/supabaseContext.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
📚 Learning: 2025-07-08T14:51:55.299Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-08T14:51:55.299Z
Learning: DiscourseGraphs maintainers prefer not to add narrowly typed definitions for individual `process.env` keys to avoid unnecessary coupling.
Applied to files:
apps/obsidian/src/constants.ts
📚 Learning: 2025-11-25T00:52:36.174Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-11-25T00:52:36.174Z
Learning: Applies to apps/obsidian/** : Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-08-27T13:59:31.143Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/database/cucumber.json:3-4
Timestamp: 2025-08-27T13:59:31.143Z
Learning: In the discourse-graph monorepo, repo/* imports work correctly through standard npm package resolution where packages/*/package.json files have "name": "repo/..." and consuming apps reference these packages by name in their dependencies. The imports resolve properly in both local development and deployment environments.
Applied to files:
apps/obsidian/package.json
🧬 Code graph analysis (4)
apps/obsidian/src/components/GeneralSettings.tsx (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
apps/obsidian/src/index.ts (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
apps/obsidian/src/utils/supabaseContext.ts (3)
packages/database/src/dbTypes.ts (1)
Enums(1763-1778)packages/database/src/lib/contextFunctions.ts (3)
fetchOrCreateSpaceDirect(82-124)fetchOrCreatePlatformAccount(143-170)createLoggedInClient(126-141)packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (2)
apps/obsidian/src/index.ts (1)
DiscourseGraphPlugin(27-351)apps/obsidian/src/utils/supabaseContext.ts (1)
getSupabaseContext(55-119)
🔇 Additional comments (9)
apps/obsidian/package.json (1)
40-42: LGTM! Dependencies added appropriately.The three new dependencies (@repo/database, @repo/utils, @supabase/supabase-js) are correctly added to support the Supabase integration feature. The workspace protocol for internal packages and catalog for external dependencies aligns with the monorepo structure.
apps/obsidian/src/types.ts (1)
37-39: LGTM! Type definitions are appropriate.The three optional fields added to the Settings type are correctly typed and follow the project's naming conventions. Making them optional ensures backward compatibility with existing settings.
apps/obsidian/src/constants.ts (1)
73-75: LGTM! Default settings are appropriate.The new default values are sensible:
syncModeEnabled: falseprovides a safe opt-in defaultspacePasswordandaccountLocalIdasundefinedallow for lazy generation when first neededapps/obsidian/scripts/compile.ts (1)
13-26: LGTM! Environment loading with clear error handling.The code properly loads database environment variables with helpful error messages guiding developers to build the database module first. The try-catch pattern appropriately distinguishes between missing modules and other errors.
apps/obsidian/src/components/GeneralSettings.tsx (3)
161-163: LGTM! State management implemented correctly.The state initialization properly handles the optional
syncModeEnabledfield with a nullish coalescing operator, and the toggle handler correctly usesuseCallbackto prevent unnecessary re-renders.Also applies to: 197-200
221-231: LGTM! Excellent error handling for sync initialization.The post-save initialization logic properly:
- Only runs when sync mode is enabled
- Awaits the async operation
- Catches and logs errors
- Provides clear user feedback for both success and failure cases
This is the correct pattern for user-initiated operations.
336-351: LGTM! UI implementation follows established patterns.The sync mode UI follows the same structure and styling as other settings in the component. The "(BETA)" prefix appropriately signals that this is an experimental feature.
apps/obsidian/src/utils/supabaseContext.ts (2)
29-40: LGTM! Secure password generation and persistence.The function correctly uses
crypto.randomUUID()for generating secure passwords and properly persists them to plugin settings. The async/await pattern is handled correctly.
123-142: The session refresh approach in the code is correct and requires no changes.The current implementation already handles session refresh properly. The
auth.getSession()method automatically attempts to refresh the access token with the refresh token if the access token is expired (per Supabase documentation). The code correctly checks for errors after callinggetSession()and clears the client if the refresh fails (e.g., due to an invalid refresh token or policy enforcement). This is the appropriate pattern.The proposed fix adds unnecessary complexity without improving the behavior—the current code already achieves the intended session renewal through Supabase's built-in auto-refresh mechanism.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
11-14: Consider removing sensitive identifiers from production logs.The success log currently includes
spaceIdanduserId, which may expose sensitive information in production environments. Consider logging a generic success message or making the detailed log conditional on a development flag.
🧹 Nitpick comments (3)
apps/obsidian/src/components/GeneralSettings.tsx (1)
221-231: Consider initializing sync only when enabled state changes.Currently,
initializeSupabaseSyncis called every time settings are saved ifsyncModeEnabledis true. While the context cache prevents redundant initialization, it's more efficient to only call this when sync mode is toggled from false to true.🔎 Suggested optimization
Track the previous sync state and only initialize on enable:
const [syncModeEnabled, setSyncModeEnabled] = useState<boolean>( plugin.settings.syncModeEnabled ?? false, ); + const [previousSyncMode, setPreviousSyncMode] = useState<boolean>( + plugin.settings.syncModeEnabled ?? false, + ); // ... in handleSave ... plugin.settings.syncModeEnabled = syncModeEnabled; // ... save other settings ... await plugin.saveSettings(); new Notice("General settings saved"); setHasUnsavedChanges(false); - if (syncModeEnabled) { + if (syncModeEnabled && !previousSyncMode) { try { await initializeSupabaseSync(plugin); new Notice("Sync mode initialized successfully"); + setPreviousSyncMode(true); } catch (error) { console.error("Failed to initialize sync mode:", error); new Notice( `Failed to initialize sync mode: ${error instanceof Error ? error.message : String(error)}`, ); } } + if (!syncModeEnabled && previousSyncMode) { + setPreviousSyncMode(false); + }apps/obsidian/src/utils/supabaseContext.ts (2)
21-27: Prefer crypto-based randomness for unique identifiers.The function uses
Math.random()to generate the account local ID suffix, which is not cryptographically secure and has potential collision risks. Consider usingcrypto.randomUUID()(already used forspacePasswordat line 36) for better uniqueness guarantees.🔎 Proposed improvement
const generateAccountLocalId = (vaultName: string): string => { - const randomSuffix = Math.random() - .toString(36) - .substring(2, 8) - .toUpperCase(); + const randomSuffix = crypto.randomUUID().substring(0, 6).toUpperCase(); return `${vaultName}-${randomSuffix}`; };
135-140: Consider recreating client immediately on session error.When session refresh fails, the function sets
loggedInClientto null and returns null, requiring the caller to retry. For better resilience, consider recreating the client immediately within the same call.🔎 Suggested improvement
} else { // renew session const { error } = await loggedInClient.auth.getSession(); if (error) { - loggedInClient = null; + // Recreate client on session error + const context = await getSupabaseContext(plugin); + if (context === null) { + loggedInClient = null; + return null; + } + loggedInClient = await createLoggedInClient( + context.platform, + context.spaceId, + context.spacePassword, + ); } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/obsidian/package.jsonapps/obsidian/scripts/compile.tsapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/constants.tsapps/obsidian/src/index.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
🧰 Additional context used
📓 Path-based instructions (2)
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/package.jsonapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/scripts/compile.tsapps/obsidian/src/constants.tsapps/obsidian/src/index.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
**/*.{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/components/GeneralSettings.tsxapps/obsidian/scripts/compile.tsapps/obsidian/src/constants.tsapps/obsidian/src/index.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-11-25T00:52:36.174Z
Learning: Applies to apps/obsidian/** : Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
📚 Learning: 2025-11-25T00:52:36.174Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-11-25T00:52:36.174Z
Learning: Applies to apps/obsidian/** : Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/obsidian/package.json
📚 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/package.jsonapps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-27T13:59:31.143Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/database/cucumber.json:3-4
Timestamp: 2025-08-27T13:59:31.143Z
Learning: In the discourse-graph monorepo, repo/* imports work correctly through standard npm package resolution where packages/*/package.json files have "name": "repo/..." and consuming apps reference these packages by name in their dependencies. The imports resolve properly in both local development and deployment environments.
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
apps/obsidian/package.jsonapps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:38:52.779Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-31T17:38:52.779Z
Learning: The dbDotEnv functionality in packages/database/src/dbDotEnv.js is designed for Node.js environments only and will not be used in browser contexts.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:37:17.173Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.173Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
apps/obsidian/scripts/compile.tsapps/obsidian/src/utils/supabaseContext.ts
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-07-08T14:51:55.299Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-08T14:51:55.299Z
Learning: DiscourseGraphs maintainers prefer not to add narrowly typed definitions for individual `process.env` keys to avoid unnecessary coupling.
Applied to files:
apps/obsidian/src/constants.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.tsapps/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/supabaseContext.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/supabaseContext.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/supabaseContext.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/obsidian/src/utils/supabaseContext.ts
🧬 Code graph analysis (3)
apps/obsidian/src/components/GeneralSettings.tsx (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
apps/obsidian/src/index.ts (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (2)
apps/obsidian/src/index.ts (1)
DiscourseGraphPlugin(27-351)apps/obsidian/src/utils/supabaseContext.ts (1)
getSupabaseContext(55-119)
🔇 Additional comments (2)
apps/obsidian/scripts/compile.ts (1)
121-129: LGTM: Standard Supabase client configuration.Injecting
SUPABASE_ANON_KEYinto the client bundle is the correct approach for Supabase client-side authentication. The anon key is designed to be public and is properly protected by Row Level Security (RLS) policies on the database side.apps/obsidian/src/utils/supabaseContext.ts (1)
62-76: Vault name extraction logic is defensive and appropriate.The code properly checks for
getName()availability before using it, and falls back to adapter-based extraction when needed. The fallback to"obsidian-vault"ensures the function always returns a valid name.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/obsidian/scripts/compile.ts (1)
14-26: Improve error message with actionable instructions.The error message on line 21 could be more helpful by specifying the exact command needed to build the database module.
🔎 Suggested improvement
- console.error("Build the database module before compiling obsidian"); + console.error("Database module not found. Run 'pnpm run build' in packages/database before compiling obsidian.");apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
4-15: Consider adding more diagnostic information to the error.The error message on line 9 is generic. When
getSupabaseContextreturns null, it would be helpful to include more context about why initialization failed (e.g., missing credentials, network issues, invalid configuration).🔎 Suggested improvement
export const initializeSupabaseSync = async ( plugin: DiscourseGraphPlugin, ): Promise<void> => { const context = await getSupabaseContext(plugin); if (!context) { - throw new Error("Failed to initialize Supabase sync: could not create context"); + throw new Error( + "Failed to initialize Supabase sync: could not create context. " + + "Verify that spacePassword and accountLocalId are configured in plugin settings." + ); } console.log("Supabase sync initialized successfully", { spaceId: context.spaceId, userId: context.userId, }); };packages/database/package.json (1)
48-48: Consider upgrading tslib to 2.8.1.The tslib version 2.5.1 (from May 2023) is significantly outdated. The latest stable version is 2.8.1, with no breaking changes between versions—only bug fixes and export adjustments. The upgrade is safe and recommended for staying current with maintenance releases.
apps/obsidian/src/components/GeneralSettings.tsx (1)
220-231: Consider user feedback when sync initialization fails.The error handling is good, but if sync initialization fails after settings are saved,
syncModeEnabledremainstruein settings while sync isn't actually working. Consider reverting the setting on failure or providing clearer guidance.🔎 Suggested improvement
if (syncModeEnabled) { try { await initializeSupabaseSync(plugin); new Notice("Sync mode initialized successfully"); } catch (error) { console.error("Failed to initialize sync mode:", error); + // Revert the setting since sync failed + plugin.settings.syncModeEnabled = false; + setSyncModeEnabled(false); + await plugin.saveSettings(); new Notice( `Failed to initialize sync mode: ${error instanceof Error ? error.message : String(error)}`, ); } }packages/database/src/lib/contextFunctions.ts (1)
180-191: Verify Obsidian account existence before fetch.This assumes the PlatformAccount was already created in the
create-spaceedge function. If the account doesn't exist (e.g., edge function partially failed), this will throw a cryptic error.Consider adding a more descriptive error message that guides the user to re-enable sync mode.
🔎 Suggested improvement
if (platform === "Obsidian") { const result = await supabase .from("PlatformAccount") .select("id") .eq("account_local_id", accountLocalId) .eq("platform", platform) .single(); if (result.error) throw Error(result.error.message); - if (!result.data) throw Error("Account not found"); + if (!result.data) throw Error("Platform account not found. Try disabling and re-enabling sync mode."); return result.data.id; }apps/obsidian/src/utils/supabaseContext.ts (2)
21-28: Consider cryptographic randomness for accountLocalId suffix.
Math.random()is not cryptographically secure. While this is an identifier rather than a secret, usingcrypto.randomUUID()(already used for password) would be more consistent and reduce collision probability.🔎 Suggested improvement
const generateAccountLocalId = (vaultName: string): string => { - const randomSuffix = Math.random().toString(36).substring(2, 8).toUpperCase(); + const randomSuffix = crypto.randomUUID().substring(0, 8).toUpperCase(); const sanitizedVaultName = vaultName .replace(/\s+/g, "") .replace(/[^a-zA-Z0-9]/g, "") .replace(/-+/g, "-"); return `${sanitizedVaultName}${randomSuffix}@database.discoursegraphs.com`; };
56-106: Module-level cache with plugin parameter may cause issues.The
contextCacheis module-level, butgetSupabaseContextaccepts apluginparameter. If the plugin reference changes (e.g., plugin reload), the cached context may refer to stale settings. Consider keying the cache by vault name or clearing it on plugin unload.🔎 Suggested pattern
// Option 1: Key by vault name const contextCacheByVault: Map<string, SupabaseContext> = new Map(); // Option 2: Export a cache clear function for plugin lifecycle export const clearSupabaseContextCache = (): void => { contextCache = null; loggedInClient = null; };
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/obsidian/package.jsonapps/obsidian/scripts/compile.tsapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/constants.tsapps/obsidian/src/index.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/roam/src/utils/supabaseContext.tspackages/database/package.jsonpackages/database/src/lib/contextFunctions.tspackages/database/supabase/functions/create-space/index.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/constants.tsapps/obsidian/src/index.tsapps/obsidian/scripts/compile.tsapps/obsidian/src/components/GeneralSettings.tsxpackages/database/supabase/functions/create-space/index.tsapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/syncDgNodesToSupabase.tsapps/roam/src/utils/supabaseContext.tspackages/database/src/lib/contextFunctions.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/constants.tsapps/obsidian/src/index.tsapps/obsidian/scripts/compile.tsapps/obsidian/package.jsonapps/obsidian/src/components/GeneralSettings.tsxapps/obsidian/src/utils/supabaseContext.tsapps/obsidian/src/types.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/utils/supabaseContext.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/utils/supabaseContext.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/utils/supabaseContext.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/supabaseContext.ts
🧠 Learnings (20)
📓 Common learnings
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-07-08T14:51:55.299Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-08T14:51:55.299Z
Learning: DiscourseGraphs maintainers prefer not to add narrowly typed definitions for individual `process.env` keys to avoid unnecessary coupling.
Applied to files:
apps/obsidian/src/constants.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/index.tsapps/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/index.tsapps/obsidian/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:38:52.779Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-31T17:38:52.779Z
Learning: The dbDotEnv functionality in packages/database/src/dbDotEnv.js is designed for Node.js environments only and will not be used in browser contexts.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-08-31T17:37:17.173Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 400
File: packages/database/src/dbDotEnv.js:24-31
Timestamp: 2025-08-31T17:37:17.173Z
Learning: The "all" variant for SUPABASE_USE_DB should only be used in createEnv.mts and not in the dbDotEnv module. It was incorrectly added to dbDotEnv.d.ts but should be removed to maintain proper separation of concerns.
Applied to files:
apps/obsidian/scripts/compile.tsapps/obsidian/src/utils/supabaseContext.tsapps/roam/src/utils/supabaseContext.ts
📚 Learning: 2025-09-11T23:54:41.861Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.861Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
apps/obsidian/scripts/compile.tsapps/obsidian/package.jsonpackages/database/package.json
📚 Learning: 2025-08-27T14:02:46.242Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/database/cucumber.json:3-4
Timestamp: 2025-08-27T14:02:46.242Z
Learning: The sandbox environment used for script execution in the discourse-graph repository lacks TypeScript compiler (tsc) and related tooling, making it unreliable for validating TypeScript-based build processes or test configurations. Should rely on developer confirmation rather than sandbox execution results for TypeScript projects.
Applied to files:
apps/obsidian/scripts/compile.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/scripts/compile.tsapps/obsidian/package.jsonpackages/database/package.json
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
apps/obsidian/scripts/compile.ts
📚 Learning: 2025-11-25T00:52:36.174Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-11-25T00:52:36.174Z
Learning: Applies to apps/obsidian/** : Prefer existing dependencies from apps/obsidian/package.json when adding dependencies to the Obsidian plugin
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-08-27T13:59:31.143Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/database/cucumber.json:3-4
Timestamp: 2025-08-27T13:59:31.143Z
Learning: In the discourse-graph monorepo, repo/* imports work correctly through standard npm package resolution where packages/*/package.json files have "name": "repo/..." and consuming apps reference these packages by name in their dependencies. The imports resolve properly in both local development and deployment environments.
Applied to files:
apps/obsidian/package.json
📚 Learning: 2025-08-27T02:23:45.696Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.696Z
Learning: When reviewing TypeScript configuration changes, check if settings like "declaration" are inherited from base configs before suggesting to add them explicitly. Many monorepo packages extend base configurations that already include necessary compiler options.
Applied to files:
apps/obsidian/src/components/GeneralSettings.tsx
📚 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/supabaseContext.tsapps/roam/src/utils/supabaseContext.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/supabaseContext.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/supabaseContext.tspackages/database/src/lib/contextFunctions.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.
Applied to files:
apps/obsidian/src/utils/supabaseContext.tsapps/roam/src/utils/supabaseContext.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:
packages/database/src/lib/contextFunctions.ts
🧬 Code graph analysis (6)
apps/obsidian/src/index.ts (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
apps/obsidian/src/components/GeneralSettings.tsx (1)
apps/obsidian/src/utils/syncDgNodesToSupabase.ts (1)
initializeSupabaseSync(4-15)
packages/database/supabase/functions/create-space/index.ts (1)
packages/database/src/lib/contextFunctions.ts (3)
asPostgrestFailure(23-41)SpaceRecord(15-15)spaceAnonUserEmail(7-8)
apps/obsidian/src/utils/supabaseContext.ts (3)
packages/database/src/dbTypes.ts (1)
Enums(1763-1778)packages/database/src/lib/contextFunctions.ts (3)
fetchOrCreateSpaceDirect(86-128)fetchOrCreatePlatformAccount(157-203)createLoggedInClient(130-155)packages/database/src/lib/client.ts (1)
DGSupabaseClient(9-9)
apps/roam/src/utils/supabaseContext.ts (1)
packages/database/src/lib/contextFunctions.ts (1)
createLoggedInClient(130-155)
packages/database/src/lib/contextFunctions.ts (2)
packages/database/src/lib/client.ts (2)
DGSupabaseClient(9-9)createClient(11-21)apps/website/app/utils/supabase/server.ts (1)
createClient(9-52)
🔇 Additional comments (19)
apps/obsidian/src/types.ts (1)
37-40: LGTM!The new optional fields for Supabase sync integration are well-typed and consistent with the settings flow established in other files.
apps/obsidian/src/index.ts (1)
38-46: LGTM! Error handling properly implemented.The sync initialization now includes proper error handling with both console logging and user-visible Notice, addressing the previous review concern about unhandled promise rejections.
apps/obsidian/scripts/compile.ts (1)
60-60: Ensurewindow.TSLibis available before the plugin loads.The esbuild configuration externalizes tslib and maps it to
window.TSLib, assuming the global will be available at runtime. Without explicit initialization of this global (either by loading tslib separately or through another mechanism), the plugin will encounter undefined reference errors when tslib utilities are needed. Verify that either:
- tslib is loaded as a global before the plugin initializes, or
- Obsidian provides this global in the version specified in manifest.json (minAppVersion: 1.7.0)
apps/obsidian/src/components/GeneralSettings.tsx (3)
161-163: LGTM!State initialization follows the established pattern in the file, with appropriate nullish coalescing for the default value.
197-200: LGTM!The toggle handler follows the same pattern as other handlers in this component and correctly marks changes as unsaved.
336-351: LGTM!The UI implementation follows the existing checkbox pattern used for
showIdsInFrontmatter. The "(BETA)" label appropriately signals the feature's maturity.apps/roam/src/utils/supabaseContext.ts (1)
101-105: LGTM!The call correctly adapts to the new object-based signature of
createLoggedInClient. Roam appropriately omitsaccountLocalIdsince it uses the anonymous user flow viaspaceAnonUserEmail.apps/obsidian/src/constants.ts (1)
73-75: LGTM!Default values are appropriate:
undefinedfor credentials that will be auto-generated on first sync enablement, andfalsefor the opt-in sync toggle.packages/database/src/lib/contextFunctions.ts (2)
17-21: LGTM!The type extension cleanly adds the new optional fields for Obsidian support while maintaining backward compatibility with Roam.
130-155: LGTM!The refactor to object destructuring improves readability and makes the optional
accountLocalIdparameter explicit. The email derivation logic correctly handles both platforms:
- Obsidian: uses
accountLocalIddirectly as email (the generated email-formatted identifier)- Roam: falls back to the space-based anonymous email
packages/database/supabase/functions/create-space/index.ts (4)
64-69: LGTM!The validation correctly enforces
accountLocalIdandaccountNamerequirements only for the Obsidian platform, maintaining backward compatibility with Roam.
104-127: LGTM!The platform-specific setup correctly differentiates:
- Obsidian: uses the provided
accountLocalIdas email (real user account withagent_type: "person")- Roam: uses the generated anonymous email (anonymous account)
This aligns with Obsidian's vault-per-user model vs Roam's shared graph model.
179-197: LGTM - idempotent upsert pattern.The PlatformAccount upsert uses proper conflict resolution with
onConflict: "account_local_id,platform"andignoreDuplicates: false, ensuring idempotent behavior on retries. This aligns with the established pattern for multi-step database operations without transactions. Based on learnings, this is the preferred approach.
137-148: Error handling is already sufficiently specific for debugging.The error response includes
signInError.message, which contains the detailed error from Supabase (e.g., "Invalid login credentials", rate limit errors, network issues). This provides adequate debugging information. The logic correctly allowsinvalid_credentialserrors to proceed with user creation and rejects all other authentication errors immediately.apps/obsidian/src/utils/supabaseContext.ts (4)
12-17: LGTM!The
SupabaseContexttype cleanly defines the context structure needed for Obsidian integration.
30-41: LGTM!Password generation uses
crypto.randomUUID()which is cryptographically secure. The pattern of checking existing settings before generating is correct.
140-162: LGTM - session renewal with re-authentication fallback.The session renewal logic properly handles:
- Checking existing session validity
- Clearing stale client on error
- Re-creating context and client on failure
This ensures resilient authentication handling.
66-67: No action needed—the URL format is intentional for Obsidian. Thespace${accountLocalId}identifier is appropriate for local-first platforms, whereas Roam uses actual HTTP URLs (https://roamresearch.com/#/app/...). The URL field serves as a unique identifier in the database, not necessarily as a web URL. The code comment already documents this intentional design.apps/obsidian/package.json (1)
40-42: Cannot verify without diff context.The current package.json shows @supabase/supabase-js and workspace packages already present with consistent versioning patterns across the monorepo. However, without seeing the actual changes (what was added vs. modified), the original review's assessment that these are "new dependencies" cannot be confirmed. Bundle size impact assessment requires build tooling unavailable in this environment.
trangdoan982
force-pushed
the
eng-1177-authenticate-obsidian-against-dg-space
branch
from
2f3d498 to
d53a70e
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Sorry did not see it was waiting for me. Will prioritize this. |
maparent
force-pushed
the
eng-1177-authenticate-obsidian-against-dg-space
branch
from
d53a70e to
9fd200b
Compare
|
Note: We agreed to revert back to roam-style anonymous users for now. |
trangdoan982
force-pushed
the
graphite-base/656
branch
from
dc946c0 to
52ff873
Compare
trangdoan982
force-pushed
the
eng-1177-authenticate-obsidian-against-dg-space
branch
from
baa665d to
3f40d5c
Compare
trangdoan982
force-pushed
the
graphite-base/656
branch
from
dc946c0 to
52ff873
Compare
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
Good overall, one addition and one point may benefit from research.
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
Somehow that comment is pending in a separate review? Making sure you see it.
trangdoan982
force-pushed
the
graphite-base/656
branch
from
dcf4537 to
52ff873
Compare
trangdoan982
force-pushed
the
eng-1177-authenticate-obsidian-against-dg-space
branch
from
3f40d5c to
1006400
Compare
|
Graphite seems to be changing the base again? Make sure the base remains main! (Again this is the base here in GitHub interface, nothing to do with git rebasing.) |
trangdoan982
force-pushed
the
eng-1177-authenticate-obsidian-against-dg-space
branch
from
1006400 to
ee79a54
Compare
trangdoan982
deleted the
eng-1177-authenticate-obsidian-against-dg-space
branch
https://www.loom.com/share/40904660f20b4a478019f86363d4c12f
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.