Roam: Eng-512 update node upsertdelete to include concepts #316
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThis change removes two API route files responsible for fetching and deleting discourse nodes via HTTP endpoints and refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant RoamCleanup as cleanupOrphanedNodes
participant Supabase as SupabaseClient
RoamCleanup->>Supabase: getAllNodesFromSupabase(spaceId)
Supabase-->>RoamCleanup: Returns all node UIDs
RoamCleanup->>RoamCleanup: Determine orphaned node UIDs
RoamCleanup->>Supabase: deleteNodesFromSupabase(orphanedUIDs, spaceId)
Supabase-->>RoamCleanup: Deletes nodes and related data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/roam/src/utils/cleanupOrphanedNodes.ts (2)
4-31: Clarify function purpose and namingThe function name
getAllNodesFromSupabasesuggests retrieving all nodes, but it specifically queries concepts whereschema_idis not null. This appears to fetch concept instances rather than all discourse nodes. Consider renaming to better reflect its purpose, such asgetConceptInstancesFromSupabaseor documenting why only concepts with schemas are relevant for orphan cleanup.
53-97: Consider the implications of partial deletion on errorsThe function performs multiple delete operations without transaction support. If an error occurs after deleting concepts but before deleting content, it could leave orphaned content records. While I understand Supabase lacks transaction support (per retrieved learnings), consider documenting this limitation or implementing a retry mechanism for failed deletions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/utils/cleanupOrphanedNodes.ts(2 hunks)apps/website/app/api/supabase/delete-discourse-nodes/route.ts(0 hunks)apps/website/app/api/supabase/get-all-discourse-nodes/route.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/website/app/api/supabase/get-all-discourse-nodes/route.ts
- apps/website/app/api/supabase/delete-discourse-nodes/route.ts
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-06-07T02:56:57.093Z
Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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.
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-07-19T22:33:54.521Z
Learning: You are working on the api routes for Discourse Graph which uses NextJS app router.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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.
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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
PR: DiscourseGraphs/discourse-graph#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.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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/.
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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.
📚 Learning: the supabase client does not offer transaction support, so idempotent upserts with proper conflict r...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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/cleanupOrphanedNodes.ts
📚 Learning: in apps/roam/src/utils/getalldiscoursenodessince.ts, the user confirmed that querying for `?title` w...
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: in the getalldiscoursenodessince function in apps/roam/src/utils/getalldiscoursenodessince.ts, date ...
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: the search-content route (apps/website/app/api/supabase/rpc/search-content/route.ts) is intentionall...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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/utils/cleanupOrphanedNodes.ts
📚 Learning: in the discoursenode interface from apps/roam/src/utils/getdiscoursenodes.ts, the field `type` serve...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: applies to apps/roam/**/*.{js,jsx,ts,tsx} : use the roamalphaapi documentation from https://roamrese...
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#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/utils/cleanupOrphanedNodes.ts
📚 Learning: in apps/website/app/utils/supabase/dbutils.ts, expanding the known_embeddings and default_dimensions...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: applies to apps/roam/**/*.{js,jsx,ts,tsx} : use roam depot/extension api documentation from https://...
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#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/utils/cleanupOrphanedNodes.ts
📚 Learning: in the discourse-graph codebase, types.gen.ts is not accessible from supabase edge functions, requir...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: in the discoursenode interface from apps/roam/src/utils/getdiscoursenodes.ts, the field `node.type` ...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: in the `match_embeddings_for_subset_nodes` sql function in packages/database/supabase/schemas/embedd...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/embedding.sql:66-95
Timestamp: 2025-05-20T14:04:19.632Z
Learning: In the `match_embeddings_for_subset_nodes` SQL function in packages/database/supabase/schemas/embedding.sql, the number of results is implicitly limited by the length of the input array parameter `p_subset_roam_uids` since the function filters content using `WHERE c.source_local_id = ANY(p_subset_roam_uids)`.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: the packages/database directory in the discourse-graph repository has a unique structure as a databa...
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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/roam/src/utils/cleanupOrphanedNodes.ts
🔇 Additional comments (3)
apps/roam/src/utils/cleanupOrphanedNodes.ts (3)
33-51: LGTM!The Roam query correctly identifies UIDs that no longer exist in the graph using a negative existence check. Good error handling.
99-121: Well-structured cleanup implementationThe function properly handles the cleanup flow with appropriate error checking and early returns for optimization. Good separation of concerns by passing the Supabase client and spaceId to helper functions.
1-2: Good refactoring to eliminate API dependenciesThe refactoring to use direct Supabase client queries instead of external REST API endpoints improves efficiency and reduces network overhead. This architectural change appropriately consolidates discourse node management within the Roam app context.
maparent
reviewed
Aug 4, 2025
maparent
requested changes
Aug 4, 2025
Collaborator
maparent
left a comment
maparent
left a comment
There was a problem hiding this comment.
ok. Curious about why only removing instances and not schemas. Otherwise lgtm.
Collaborator
|
One of my comments fell outside the review because I'm still learning how GitHub does reviews... |
maparent
approved these changes
Aug 7, 2025
Collaborator
Author
|
cc @mdroidian I merged this pr without checking that you haven't reviewed it, I thought only maparent is going to review. I can revert the merge |
Collaborator
Author
|
cc @mdroidian I merged this pr without checking that you have'nt reviewed it, I thought only maparent is going to review. I can revert the merge |
Contributor
All good |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Chores