eng-1135 define internalError #608
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
maparent
force-pushed
the
eng-1135-replacing-sendmail-with-explicit-posthog-errors
branch
from
6e47ec6 to
b581e2d
Compare
📝 WalkthroughWalkthroughAdds a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Component (UI / util)
participant internalError as internalError()
participant PostHog as PostHog
participant EmailSvc as sendErrorEmail
participant Toast as ToastSystem
participant Console as Console (dev)
Component->>internalError: internalError({ error, type?, userMessage?, context?, sendEmail?, forceSendInDev? })
Note right of internalError: Normalize error → Error object\nEnrich context (app, graphName, version, buildDate, username)
alt development & not forceSendInDev
internalError->>Console: console.error(error, enrichedContext)
else
internalError->>PostHog: captureException(error, enrichedContext)
PostHog-->>internalError: ack
end
alt sendEmail !== false
internalError->>EmailSvc: sendErrorEmail({ error, type, context })
EmailSvc-->>internalError: ack / error
end
alt userMessage provided
internalError->>Toast: show({ id, message:userMessage, intent: danger, timeout:5s })
Toast-->>internalError: displayed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 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 (12)📓 Common learnings📚 Learning: 2025-12-13T00:42:41.524ZApplied to files:
📚 Learning: 2025-11-25T00:52:41.934ZApplied to files:
📚 Learning: 2025-11-25T00:52:41.934ZApplied to files:
📚 Learning: 2025-11-25T00:52:41.934ZApplied to files:
📚 Learning: 2025-11-25T00:52:41.934ZApplied to files:
📚 Learning: 2025-06-19T19:43:43.380ZApplied to files:
📚 Learning: 2025-11-25T00:52:41.934ZApplied to files:
📚 Learning: 2025-06-22T10:40:52.752ZApplied to files:
📚 Learning: 2025-11-05T21:57:14.909ZApplied to files:
📚 Learning: 2025-06-17T23:37:45.289ZApplied to files:
📚 Learning: 2025-06-17T23:37:45.289ZApplied to files:
🧬 Code graph analysis (1)apps/roam/src/index.ts (1)
🔇 Additional comments (6)
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: 1
🧹 Nitpick comments (3)
apps/roam/src/components/canvas/useRoamStore.ts (1)
137-158: Remove unusederrorMessageparameter.The
errorMessageparameter is no longer used after migrating tointernalError, but callers still pass it (lines 172, 182). This creates dead code.const handleStoreError = ({ error, - errorMessage, }: { error: Error; - errorMessage: string; }): void => { setError(error); setLoading(false); const snapshotSize = initialSnapshot ? JSON.stringify(initialSnapshot).length : 0; internalError({ error, type: "roam-store-error", context: { pageUid, snapshotSize, ...(snapshotSize < 10000 ? { initialSnapshot } : {}), }, }); };And update the call sites:
-handleStoreError({ - error: e as Error, - errorMessage: "Failed to create TLStore", -}); +handleStoreError({ error: e as Error });apps/roam/src/components/CreateRelationDialog.tsx (1)
95-99: Consider passing an Error object for consistency.While
internalErrorhandles string errors by converting them, other call sites in this PR consistently passErrorobjects. For consistency and to preserve stack traces:internalError({ type: "create-relation-error", - error: - "Create Relation dialog: Could not find identify node downstream", + error: new Error( + "Create Relation dialog: Could not identify node downstream", + ), });Also note: "find identify" appears to be a typo - should be "identify" or "find".
apps/roam/src/utils/internalError.ts (1)
7-17: Add explicit return type annotation.As per coding guidelines, functions should have explicit return types.
const internalError = ({ error, userMessage, type, context, }: { error: unknown; type?: string; userMessage?: string; context?: Properties; -}) => { +}): void => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/roam/src/components/CreateRelationDialog.tsx(4 hunks)apps/roam/src/components/Export.tsx(5 hunks)apps/roam/src/components/canvas/Clipboard.tsx(7 hunks)apps/roam/src/components/canvas/Tldraw.tsx(2 hunks)apps/roam/src/components/canvas/useRoamStore.ts(3 hunks)apps/roam/src/utils/internalError.ts(1 hunks)apps/roam/src/utils/syncDgNodesToSupabase.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/internalError.tsapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/utils/syncDgNodesToSupabase.tsapps/roam/src/components/canvas/Tldraw.tsx
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/internalError.tsapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/utils/syncDgNodesToSupabase.tsapps/roam/src/components/canvas/Tldraw.tsx
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/internalError.tsapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/utils/syncDgNodesToSupabase.tsapps/roam/src/components/canvas/Tldraw.tsx
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/internalError.tsapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/utils/syncDgNodesToSupabase.tsapps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/utils/internalError.tsapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsxapps/roam/src/utils/syncDgNodesToSupabase.tsapps/roam/src/components/canvas/Tldraw.tsx
🧠 Learnings (11)
📚 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/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/Export.tsxapps/roam/src/components/canvas/useRoamStore.tsapps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsx
📚 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/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/src/components/Export.tsxapps/roam/src/components/CreateRelationDialog.tsx
📚 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/**/*.{ts,tsx,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/components/Export.tsx
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsx
📚 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/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsx
📚 Learning: 2025-12-07T20:54:19.988Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:19.988Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 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/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/canvas/Clipboard.tsx
📚 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-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
🧬 Code graph analysis (1)
apps/roam/src/utils/internalError.ts (1)
apps/roam/src/utils/getVersion.ts (1)
getVersionWithDate(9-24)
🔇 Additional comments (14)
apps/roam/src/components/canvas/Tldraw.tsx (1)
89-89: LGTM!The migration to the centralized
internalErrorutility is clean. The error type follows the new kebab-case convention, and the context includes useful debugging information (title,lastActions).Also applies to: 511-518
apps/roam/src/components/canvas/useRoamStore.ts (1)
306-312: LGTM!The error handling in
performUpgradecorrectly uses the centralized utility with appropriate type and context.apps/roam/src/components/Export.tsx (2)
312-315: LGTM!Good improvement switching from
lettoconstfor these calculations, enforcing immutability.
471-476: LGTM!The migration to
internalErroris consistent. Both error handlers use the same type"export-error", provide a user-facing message, and the export handler includes useful debugging context.Also applies to: 690-696
apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
63-67: LGTM!The migration correctly replaces
sendErrorEmailwith the centralizedinternalErrorutility. The error type follows the kebab-case convention, and the toast notification is preserved separately for immediate user feedback.apps/roam/src/components/CreateRelationDialog.tsx (1)
119-130: LGTM!The error handling for relation matching edge cases is appropriate, with descriptive messages that include relevant context (types, IDs) for debugging.
apps/roam/src/utils/internalError.ts (3)
33-42: LGTM!The error normalization logic correctly handles the three cases: string errors, non-Error objects (via JSON serialization with fallback), and actual Error instances. This ensures
posthog.captureExceptionalways receives a proper Error object.
22-23: Remove redundantposthog.identify()call from error handler.The user is already identified at app startup in
index.ts. Callingposthog.identify(username)on every error is redundant and violates PostHog best practices, which recommend identifying only during identity transitions (login, signup), not on every event. Delete the identify call and letposthog.captureException()use the existing session identity.
43-43: No changes needed. Theposthog.captureException(error, context)call uses the correct API signature for PostHog JavaScript SDK v1.261.7, with proper typing.apps/roam/src/components/canvas/Clipboard.tsx (5)
60-60: LGTM!The import statement is clean and follows the existing import conventions in the file.
143-151: LGTM!The early return pattern is correct here since the clipboard cannot function without a user UID. Setting
isInitializedtotruebefore returning ensures the component doesn't remain in an initializing state indefinitely.
190-195: LGTM!The error handling follows the same pattern as the initialization error. The context information (clipboardBlockUid and pageCount) will be helpful for debugging state persistence issues.
434-439: LGTM!Good error handling with appropriate fallback behavior. Setting
discourseNodesto an empty array ensures the component gracefully degrades when discourse nodes cannot be fetched.
594-598: LGTM!Good defensive programming. Creating a proper Error object with a descriptive message and returning early prevents attempting to create a shape with an invalid node type.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/components/CreateRelationDialog.tsx (1)
25-26: EnsureinternalErrorsupports string errors; consider adding structured contextIf
internalErrorexpects anError, these calls currently lose stack traces by passing a string (Line 95-99, 118-122, 126-130, 276-279). Consider:internalError({ type: "create-relation-error", - error: "Create Relation dialog: Could not identify node downstream", + error: new Error("Create Relation dialog: Could not identify node downstream"), + context: { + selectedRelationName, + sourceNodeUid, + selectedTargetUid: targetUid, + selectedTargetTitle: targetTitle, + }, });Also applies to: 95-99, 118-122, 126-130, 276-279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/roam/src/components/CreateRelationDialog.tsx(4 hunks)apps/roam/src/components/Export.tsx(5 hunks)apps/roam/src/components/canvas/Clipboard.tsx(7 hunks)apps/roam/src/components/canvas/Tldraw.tsx(2 hunks)apps/roam/src/components/canvas/useRoamStore.ts(5 hunks)apps/roam/src/utils/internalError.ts(1 hunks)apps/roam/src/utils/syncDgNodesToSupabase.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/roam/src/components/canvas/Clipboard.tsx
- apps/roam/src/utils/syncDgNodesToSupabase.ts
- apps/roam/src/components/canvas/useRoamStore.ts
- apps/roam/src/utils/internalError.ts
🧰 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/components/CreateRelationDialog.tsxapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Tldraw.tsx
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/components/CreateRelationDialog.tsxapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Tldraw.tsx
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/components/CreateRelationDialog.tsxapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Tldraw.tsx
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/components/CreateRelationDialog.tsxapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Tldraw.tsx
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/CreateRelationDialog.tsxapps/roam/src/components/Export.tsxapps/roam/src/components/canvas/Tldraw.tsx
🧠 Learnings (9)
📚 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/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 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/** : Implement the Discourse Graph protocol in the Roam Research extension
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 Learning: 2025-12-07T20:54:19.988Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:19.988Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 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/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 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/components/CreateRelationDialog.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/CreateRelationDialog.tsx
📚 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/components/Export.tsx
🔇 Additional comments (2)
apps/roam/src/components/Export.tsx (1)
225-226: Const-ification looks goodMaking
propsand the bounds intermediatesconstreduces accidental reassignment risk and reads cleaner.Also applies to: 311-316
apps/roam/src/components/canvas/Tldraw.tsx (1)
502-533: Remove incorrect dependency suggestion; title capture is safeThe
tldraw:errorlistener correctly uses an empty dependency array. The component is instantiated fresh for each distinct canvas title, sotitlewill not stale. Addingtitleto dependencies would unnecessarily re-register the event listener.Regarding
lastActionsRef.current: it's already capped at 5 items by design (lines showingif (lastActionsRef.current.length > 5) lastActionsRef.current.shift()), making the payload reasonable. This pattern is consistent with other context passed tointernalError(e.g., snapshot data inuseRoamStore.ts). PostHog serializes these objects without issue, so no sanitization is required.
| internalError({ | ||
| error: e as Error, | ||
| type: "export-error", | ||
| userMessage: | ||
| "Looks like there was an error. The team has been notified.", | ||
| intent: "danger", | ||
| }); | ||
| sendErrorEmail({ | ||
| error, | ||
| type: "Export Dialog Failed", | ||
| context: { activeExportType, filename, results }, | ||
| }).catch(() => {}); | ||
| }); | ||
| setDialogOpen(true); | ||
| setError((e as Error).message); | ||
| } finally { |
There was a problem hiding this comment.
Don’t send full results to telemetry; don’t surface raw exception messages to users
context: { ..., results }(Line 695) can leak user data into PostHog and can be huge.setError((e as Error).message)(Line 698) may expose sensitive/internal details in the dialog.
Suggested diff:
internalError({
error: e as Error,
type: "export-error",
userMessage:
"Looks like there was an error. The team has been notified.",
- context: { activeExportType, filename, results },
+ context: {
+ activeExportType,
+ filename,
+ resultsCount: Array.isArray(results) ? results.length : undefined,
+ // optionally: sample a few UIDs only, if needed for debugging
+ sampleUids: Array.isArray(results) ? results.slice(0, 10).map((r) => r.uid) : undefined,
+ },
});
setDialogOpen(true);
-setError((e as Error).message);
+setError("Export failed. Please try again.");🤖 Prompt for AI Agents
In apps/roam/src/components/Export.tsx around lines 690 to 699, the telemetry
call currently includes full results and the UI exposes the raw exception
message; remove results from the telemetry context (replace with non-sensitive
metadata such as resultCount, exportType, filename or boolean flags) and keep
the actual Error object in internalError for debugging, and change setError to a
generic user-facing message (e.g., "Looks like there was an error. The team has
been notified.") so internal exception text is not shown to users.
There was a problem hiding this comment.
Good catch on fullResults. setError is for display, should not leak anything.
There was a problem hiding this comment.
@maparent Thank you for confirming! I'm glad we're aligned on both points:
- Telemetry context: Replacing
resultswith safer metadata likeresultsCountand optionally a smallsampleUidsarray will keep debugging useful while protecting user data. - Error display: Using a generic message in
setError()(while still passing the full Error tointernalErrorfor internal logging) prevents leaking implementation details to users.
Feel free to ping me if you'd like a second look once you've updated the code! 🐰
|
@CodeRabbit yes internalError does the string-to-error conversion internally. |
|
Tip For best results, initiate chat on the files or code changes. @maparent, thank you for confirming! That's a helpful design decision – having ✏️ Learnings added
🧠 Learnings used |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Generally looks good.
Just the one code request: keep sendErrorEmail in place, I don't want to remove that.
Also, could you please record a loom video demonstrating this new functionality? Could you also include (either in the loom video or with text+screenshot) some information on where this can be found in posthog, as well as what it looks like/features it enables/etc.
Also note: could we try to keep the PR names in sync with the linear task names please? They are generally more informative and when the PR is a different name it can be harder to locate if you are going from one platform to the other. Thanks.
apps/roam/src/utils/internalError.ts
Outdated
| error = new Error(typeof error); | ||
| } | ||
| } | ||
| posthog.captureException(error, context); |
There was a problem hiding this comment.
also add sendErrorEmail so we continue to get the same error emails.
|
Added back sendErrorEmail as an option. I could do a loom video, but honestly nothing much to show; there is no ux for sending an error yet. Should I repurpose the send Email button in the admin panel, esp. now that it does send an email? But what you do need indeed is where to look for errors: https://us.posthog.com/project/113823/error_tracking Now… there are now more errors there than I expected; it seems that enabling error tracking enabled autocapture. I disabled that now, and the error tracking panel still shows. Two more things need to be added:
|
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Now… there are now more errors there than I expected; it seems that enabling error tracking enabled autocapture. I disabled that now, and the error tracking panel still shows.
"... and the error tracking panel still shows."
What does the error tracking panel still show? Does it still show all of the autocaptured errors? We need to confirm that we can get only the errors we intentional report before merging this.
This is a good example of what a loom video would help catch before PR review.
|
Rephrasing: The error tracking panel (as given by the URL) would be not only empty, but would not even show a empty table for errors; only a "please activate autocapture". I did activate, thinking it needed to also be activated in code, but the activation was somehow in the shared state? Anyhow, deactivated now, and the table with past error still shows, and I did not get back to the "Please activate" empty page. |
|
And since I (fortunately) cannot go back to the previous un-initialized state, I could not make a very useful loom video here. |
maparent
force-pushed
the
eng-1135-replacing-sendmail-with-explicit-posthog-errors
branch
from
7c78589 to
abc3b18
Compare
|
Can you confirm that we are only getting the errors we intentionally report? |
|
Hmm… actually, a few came through after I cut off the autocapture setting, so I'm puzzled. (Fewer than before though?) Some of them come from the website, looking into that, but not all of them. Doing some improvement on posthog setup while I'm there. |
|
Ok. I am looking at the errors that leaked through between my turning off autocapture on the server and now, and they come from other instances. I suspect that some of us started roam during those hours when autocapture was on, the posthog init caught that, and those roam instances will keep that setting until reloaded. Anyhow it started through action on the posthog website, and has nothing to do with the code here, which was not checked in. So I say there is no reason not to check this in. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
maparent
force-pushed
the
eng-1135-replacing-sendmail-with-explicit-posthog-errors
branch
from
67258a0 to
1da8082
Compare
|
(btw I was wrong about there being website events; they are in a separate project, as appropriate.) |
maparent
force-pushed
the
eng-1135-replacing-sendmail-with-explicit-posthog-errors
branch
from
e6760cc to
5505b71
Compare
created an internalError handler that either logs to console.error or sends a report to posthog; and optionally shows a toast to the user. This should provide a pattern that combines many common use cases.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.