-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1149 add try / catch to migration when querying for relation patterns #624
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: eng-1135-replacing-sendmail-with-explicit-posthog-errors
Are you sure you want to change the base?
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughWrapped per-item relation fetch handling in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (5)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
Files:
apps/roam/**/*.{js,ts,tsx,jsx,json}📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Files:
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Files:
apps/roam/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Files:
apps/roam/**📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Files:
🧠 Learnings (5)📓 Common learnings📚 Learning: 2025-12-13T00:42:41.524ZApplied to files:
📚 Learning: 2025-10-18T18:58:16.100ZApplied to files:
📚 Learning: 2025-06-17T23:37:45.289ZApplied to files:
📚 Learning: 2025-06-17T23:42:29.279ZApplied to files:
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/utils/getRelationData.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/roam/src/utils/getRelationData.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/getRelationData.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/getRelationData.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/getRelationData.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/getRelationData.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: When reviewing SQL functions in the discourse-graph codebase, be mindful that suggesting additional explicit error handling may introduce unnecessary database queries. Functions that use appropriate ON CONFLICT clauses may already handle errors sufficiently without needing extra validation queries.
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, database functions that use STRICT clauses and ON CONFLICT statements already have appropriate error handling. Suggesting additional explicit error handling may be unnecessary and could introduce performance overhead through extra database queries.
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
📚 Learning: 2025-12-13T00:42:41.524Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-13T00:42:41.524Z
Learning: The `internalError` utility in apps/roam/src/utils/internalError.ts handles string-to-error conversion internally. Callers can pass strings directly as the error parameter without needing to convert them to Error objects first.
Applied to files:
apps/roam/src/utils/getRelationData.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `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/getRelationData.ts
📚 Learning: 2025-06-17T23:42:29.279Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:95-104
Timestamp: 2025-06-17T23:42:29.279Z
Learning: In the DiscourseGraphs/discourse-graph codebase, DiscourseRelation type properties are either string or Triple[], and the STANDARD_ROLES filter in conceptConversion.ts excludes Triple[] properties, so only string values remain after filtering.
Applied to files:
apps/roam/src/utils/getRelationData.ts
🔇 Additional comments (1)
apps/roam/src/utils/getRelationData.ts (1)
5-5: LGTM! Import added correctly.The
internalErrorimport is appropriate for the error handling being introduced.
|
I'm adding a loom video showing no error, which is not that illustrative... except that I have not broken the system. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
https://linear.app/discourse-graphs/issue/ENG-1149/add-try-catch-to-migration-when-querying-for-relation-patterns
Added a try-catch to getRelationData, this will also affect export.
I used the new internalError, I think it's the simplest way to alert both the user and the developers.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.