Skip to content

Conversation

@sid597
Copy link
Collaborator

@sid597 sid597 commented Oct 31, 2025

While reviewing ENG-298 I missed to point out logic that was removed from initializeSupabaseSync was actually needed. So currently if we load the main plugin to any graph it automatically starts syncing even if the graph has "Opted-in" to do so.

Before this sync the logic was, on load check supabase if this graph's url exist and if it doesn't don't sync. We removed this check and now we sync by default.

In this PR I am adding the logic again for initializeSupabaseSync callsite in index.ts. Now the new intended flow is:

  • On load check if the url exist in supabase, if it does initialize
  • If it doesn't we don't initialize and wait for user to do it manually, which they do by going to settings and clicking on the button. Then we start the sync process in the same and all future sessions of this graph

Summary by CodeRabbit

  • Bug Fixes & Improvements
    • Enhanced error notifications: Sync operation failures now automatically generate both in-app toast alerts and email notifications to inform users of any issues.
    • Refined sync initialization with conditional activation logic to improve reliability and system stability.

@linear
Copy link

linear bot commented Oct 31, 2025

@supabase
Copy link

supabase bot commented Oct 31, 2025

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

sid597 commented Oct 31, 2025

@sid597
Copy link
Collaborator Author

sid597 commented Oct 31, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

✅ Actions performed

Full review triggered.

@sid597 sid597 marked this pull request as ready for review October 31, 2025 16:24
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

📝 Walkthrough

Walkthrough

Conditional Supabase sync initialization gated by Space table existence check. Refactored sync initialization from async to synchronous with enhanced error handling via toast notifications and error email alerts.

Changes

Cohort / File(s) Summary
Conditional sync initialization
apps/roam/src/index.ts
Added getRoamUrl and createClient imports; replaced unconditional sync initialization with logic that only calls initializeSupabaseSync() if a Space row exists for the current Roam URL.
Sync initialization and error handling
apps/roam/src/utils/syncDgNodesToSupabase.ts
Changed initializeSupabaseSync signature from async to synchronous ((): void); refactored to clear timeouts and schedule initial sync run without async client setup. Enhanced endSyncTask error paths with danger toast notifications and error email dispatch. Removed success-case toast logic.

Sequence Diagram(s)

sequenceDiagram
    participant Index as index.ts
    participant Client as createClient()
    participant DB as Space table
    participant Sync as initializeSupabaseSync()
    
    rect rgb(220, 240, 255)
    Note over Index,DB: Before: Unconditional sync
    Index->>Sync: initializeSupabaseSync()
    end
    
    rect rgb(240, 255, 240)
    Note over Index,DB: After: Conditional sync
    Index->>Client: createClient()
    Client-->>Index: client
    Index->>DB: Query Space where url = getRoamUrl()
    alt Space row exists
        DB-->>Index: data found
        Index->>Sync: initializeSupabaseSync()
        Sync->>Sync: Clear timeout, set doSync=true
        Sync->>Sync: Schedule createOrUpdateDiscourseEmbedding
    else Space row not found
        DB-->>Index: no data
        Note over Index: Skip initialization
    end
    end
Loading
sequenceDiagram
    participant Task as endSyncTask()
    participant Toast as Toast
    participant Email as sendErrorEmail()
    
    rect rgb(255, 245, 245)
    Note over Task,Email: Error handling flow (new)
    Task->>Task: Check for errors
    alt RPC failure or catch
        Task->>Toast: render danger toast
        Task->>Email: sendErrorEmail(context)
    else Success
        Note over Task: No toast (removed)
    end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Logic flow change: Conditional initialization adds conditional branching that requires verification of Space table query correctness and Roam URL matching logic.
  • Signature refactor: Conversion from async to synchronous requires confirming all callers are updated and no blocking operations remain.
  • Error handling additions: Multiple error paths (RPC failure, catch block) with toast + email notifications need validation for proper context passing and notification delivery.
  • Cross-file coordination: Changes span two files with interdependent logic; verify the conditional initialization in index.ts properly gates the sync setup.

Possibly related PRs

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title references three distinct changes that are all present in the changeset: removing the success banner (reflected in the endSyncTask modifications), sending error emails on failure (added error reporting in error paths), and fixing the on-load check to conditionally initialize based on whether the graph URL exists in Supabase (the core logic change in index.ts). All three aspects align with the actual code modifications, and the title accurately reflects the primary objectives stated in the PR description. While the phrasing of "fix on load check if we need to initialize by default" is slightly compact, it conveys the essential change: restoring the conditional initialization logic that was previously removed.

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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38648ed and 0c5a005.

📒 Files selected for processing (2)
  • apps/roam/src/index.ts (3 hunks)
  • apps/roam/src/utils/syncDgNodesToSupabase.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Prefer type over interface
Use explicit return types for functions
Avoid any types when possible

Files:

  • apps/roam/src/index.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
**/*.{ts,tsx,js,jsx}

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

**/*.{ts,tsx,js,jsx}: Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use Prettier with the project's configuration
Maintain consistent naming conventions: PascalCase for components and types
Maintain consistent naming conventions: camelCase for variables and functions
Maintain consistent naming conventions: UPPERCASE for constants

Files:

  • apps/roam/src/index.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
apps/roam/**/*.{js,jsx,ts,tsx}

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

apps/roam/**/*.{js,jsx,ts,tsx}: Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API

Files:

  • apps/roam/src/index.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
🧠 Learnings (10)
📓 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.
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.

Applied to files:

  • apps/roam/src/index.ts
📚 Learning: 2025-05-30T14:37:30.215Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/api/supabase/rpc/search-content/route.ts:53-56
Timestamp: 2025-05-30T14:37:30.215Z
Learning: The search-content route (apps/website/app/api/supabase/rpc/search-content/route.ts) is intentionally designed to be platform-agnostic rather than Roam-specific. The generic "Platform" terminology (like subsetPlatformIds) is used because the route will support multiple platforms in the near future, not just Roam.

Applied to files:

  • apps/roam/src/index.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/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-06-19T19:43:43.380Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 226
File: apps/roam/src/components/settings/HomePersonalSettings.tsx:123-149
Timestamp: 2025-06-19T19:43:43.380Z
Learning: The "Fetch Embeddings for nodes" button in HomePersonalSettings.tsx is for testing purposes only, so it doesn't require production-level error handling or user feedback improvements.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts

@sid597 sid597 requested review from maparent and mdroidian October 31, 2025 16:42
@sid597 sid597 force-pushed the eng-800-what-is-the-right-approach-to-handle-update-embeddings branch from 38648ed to fc9d787 Compare November 2, 2025 04:31
@sid597 sid597 force-pushed the eng-992-remove-the-banner-that-shows-up-every-time-we-load-a-graph branch from 9152f2c to a879a5c Compare November 2, 2025 04:31
@sid597 sid597 merged commit 3c44ab0 into eng-800-what-is-the-right-approach-to-handle-update-embeddings Nov 2, 2025
7 of 8 checks passed
@github-project-automation github-project-automation bot moved this to Done in General Nov 2, 2025
@sid597 sid597 deleted the eng-992-remove-the-banner-that-shows-up-every-time-we-load-a-graph branch November 4, 2025 15:53
sid597 added a commit that referenced this pull request Nov 4, 2025
…fix on load check if we need to initialize by default (#527)

* remove success banner for suggestive mode, send error email on fail, fix on load check if we need to initialize by default

* address coderabbit review

* address review
sid597 added a commit that referenced this pull request Nov 8, 2025
…fix on load check if we need to initialize by default (#527)

* remove success banner for suggestive mode, send error email on fail, fix on load check if we need to initialize by default

* address coderabbit review

* address review
@sid597 sid597 mentioned this pull request Nov 8, 2025
sid597 added a commit that referenced this pull request Nov 16, 2025
* handle update embeddings button

* 1. Fix query to get imported nodes edit data 2. remove isabled arg 3. Fix getAllDiscourseNodesSince to get nodes per node type, use map to store and get non duplicate data 4. getNodesByType

* remove success banner for suggestive mode, send error email on fail, fix on load check if we need to initialize by default (#527)

* remove success banner for suggestive mode, send error email on fail, fix on load check if we need to initialize by default

* address coderabbit review

* address review

* address review

* remove console log

* use logged in client

* remove supabase based guarding

* remove unused
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants