Roam analytics #37
Roam analytics #37
Conversation
|
@sid597 is attempting to deploy a commit to the Discourse Graphs Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request integrates PostHog analytics into the Roam project. It adds the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![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 (1)
apps/roam/src/index.ts (1)
55-55: Consider delaying analytics initialization until explicit user interaction.Currently, analytics is initialized immediately at extension load. If you want to respect user privacy preferences or application performance, consider deferring initialization until the user grants consent or after a user action indicates engagement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
apps/roam/package.json(1 hunks)apps/roam/src/index.ts(1 hunks)
🔇 Additional comments (2)
apps/roam/src/index.ts (1)
45-50: Verify compliance with data privacy and consent policies.
PostHog initialization may gather user analytics data, potentially including PII. Ensure compliance with your privacy requirements and any relevant regulations (e.g., GDPR) by anonymizing user data or prompting for user consent.
apps/roam/package.json (1)
32-32: Ensure the newly added dependency is safely updated and secure.
When introducing posthog-js, confirm you’re using the recommended or latest stable version. Additionally, run security scanners (like npm audit) to verify no newly introduced vulnerabilities.
✅ Verification successful
posthog-js appears secure, but other dependencies need attention
The version of posthog-js (1.203.2) is recent and doesn't show any known vulnerabilities. However, the npm audit revealed several other security concerns in the project's dependencies that should be addressed:
-
High severity vulnerabilities:
cross-spawn: ReDoS vulnerabilityd3-color: ReDoS vulnerabilitysemver: ReDoS vulnerabilityws: DoS vulnerability when handling requests with many headers
-
Moderate severity vulnerabilities:
axios: CSRF vulnerabilityfollow-redirects: URL parsing and header handling issuesnanoid: Predictable results with non-integer values
Recommended actions:
- Run
npm audit fixto address issues that don't require breaking changes - Carefully review and test after running
npm audit fix --forcefor issues requiring breaking changes - Consider updating these dependencies individually with proper testing to maintain stability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known vulnerabilities in `posthog-js` using npm audit
npm audit --omit dev
Length of output: 2887
|
Took too many hours to figure out the properties, and how to turn them off, turns out we can see everything and cutomise many things in the ui itself. |
|
added capture event for query drawer but could not test them in resultsgraph because the page is not present. Tested other events |
|
So I think the A couple possible solutions:
|
|
will do so after this pr is done |
|
@mdroidian how is "Instantiated Discourse Relation" different from "Discourse Relation created"? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (13)
apps/roam/src/components/canvas/CanvasDrawer.tsx (1)
165-166: Monitor frequency of "canvas_drawer_opened"
Capturing the event here makes sense. Just ensure that repeated toggling of the drawer does not flood analytics with excessive events.apps/roam/src/components/QueryBuilder.tsx (1)
194-200: Capture usage carefully
Logging"query_block_rendered"is useful for measuring usage. Double-check there’s no risk of overcounting if the block re-renders often due to state changes or React lifecycle events.apps/roam/src/components/QueryDrawer.tsx (2)
24-24: Initialize PostHog and handle edge cases.Be sure to confirm that PostHog is initialized before capturing analytics events throughout this file, in case the user’s environment blocks analytics or PostHog initialization fails.
297-301: Ensure minimal overhead for frequent captures.Calling
posthog.captureevery time a new query is created is likely acceptable. Just confirm that frequent calls do not degrade performance, especially if your code is in a tight loop or called repeatedly by user interactions.apps/roam/src/components/DiscourseContext.tsx (1)
30-30: Validate PostHog initialization.Similarly to other files, ensure that PostHog is initialized before usage to prevent potential runtime errors if PostHog fails to load or is blocked by the user.
apps/roam/src/components/ExportDiscourseContext.tsx (1)
28-28: Prevent raw usage of PostHog.As with other files, ensure that the PostHog dependency is properly guarded in case of blocked requests or missing credentials. A quick fallback can help avoid unhandled promise rejections if network connectivity fails.
apps/roam/src/components/ResultsView/views/Kanban.tsx (2)
25-25: Ensure consistent import ordering.
While importingposthogis valid, you might consider grouping analytics imports (likeposthog) together, distinct from React imports and utility imports, for clarity.
283-284: Avoid repetitive event captures if component re-renders frequently.
Theposthog.capture("kanban_created", { page: page })call could fire repeatedly ifKanbanre-renders. Consider using a ref or a check (e.g.,useEffector aonce-like guard) to ensure the event fires only when genuinely creating the Kanban rather than on every render.+ useEffect(() => { + posthog.capture("kanban_created", { page }); + }, []);apps/roam/src/components/canvas/Tldraw.tsx (1)
730-734: Refine naming for capture events.
"Canvas node created" is a clear event name, but ensure a consistent naming format across the codebase (e.g., snake_case, kebab-case, or PascalCase). Unified naming can help analytics queries remain consistent.apps/roam/src/components/ResultsView/ResultsView.tsx (3)
42-42: Use environment variables for analytics.
In a production environment, consider loadingposthogkeys or config from environment variables or a secure source; do not commit them to version control if possible.
555-560: Event naming consistency.
"query_view_type_changed" might align better with your other event naming patterns if kept parallel, such as "Query View Type Changed." Keep a consistent naming across your analytics calls.
902-910: Consider one unified function for column view updates and analytics capture.
You might wraponItemSelectwith a single function that captures analytics first and then callsonViewChange. This helps avoid duplication and ensures analytics are always captured.onItemSelect={(m) => { + logColumnViewChange(m, mode, parentUid); onViewChange({ mode: m, column, value }, i); }}apps/roam/src/index.ts (1)
29-31: Group initialization logs under debug conditions.
Consider wrappingconsole.log("PostHog initialized");with a debug or environment check if you prefer separation between dev and production logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
apps/roam/package.json(1 hunks)apps/roam/src/components/DiscourseContext.tsx(2 hunks)apps/roam/src/components/DiscourseNodeMenu.tsx(2 hunks)apps/roam/src/components/ExportDiscourseContext.tsx(2 hunks)apps/roam/src/components/QueryBuilder.tsx(3 hunks)apps/roam/src/components/QueryDrawer.tsx(5 hunks)apps/roam/src/components/ResultsView/ResultsView.tsx(4 hunks)apps/roam/src/components/ResultsView/views/Kanban.tsx(2 hunks)apps/roam/src/components/canvas/CanvasDrawer.tsx(2 hunks)apps/roam/src/components/canvas/Tldraw.tsx(4 hunks)apps/roam/src/index.ts(1 hunks)apps/roam/src/utils/createDiscourseNode.ts(2 hunks)apps/roam/src/utils/triplesToBlocks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/roam/package.json
🔇 Additional comments (20)
apps/roam/src/utils/triplesToBlocks.ts (1)
3-3: Ensure minimal analytics overhead
This import of posthog is appropriate for analytics, but verify that the overhead is kept minimal if this function is frequently invoked.
apps/roam/src/utils/createDiscourseNode.ts (2)
15-15: Confirm minimal import
posthog import is consistent with the rest of the analytics changes. Ensure the library does not negatively impact function performance.
32-36: Flag potential PII
The properties text, configPageUid, and newPageUid might contain identifying information. Confirm that these fields cannot store personal data. If they can, anonymize or redact them prior to invoking posthog.capture.
apps/roam/src/components/canvas/CanvasDrawer.tsx (1)
12-12: Check for consistent dependency usage
The import aligns well with existing usage; no immediate issues found.
apps/roam/src/components/QueryBuilder.tsx (2)
26-26: Streamline analytics usage
posthog import is consistent with other files.
216-219: Validate event triggers
Confirm that posthog.capture("query_builder_page_rendered") only fires once per page render, to avoid spamming analytics data.
apps/roam/src/components/DiscourseNodeMenu.tsx (2)
28-28: Consider error handling or fallback for PostHog initialization.
You’re adding the PostHog import here, which is good. However, ensure that posthog is properly initialized within your app before usage to prevent potential runtime errors. If network connectivity to PostHog fails, consider adding a safeguard to avoid throwing unhandled exceptions.
70-74: Confirm that no PII is inadvertently logged.
This new posthog.capture("new_discourse_node_type_created") event includes both nodeType and text. If text can contain user-generated content or sensitive data, ensure it does not violate PII or security requirements.
apps/roam/src/components/QueryDrawer.tsx (3)
59-62: Check event naming consistency.
"query_drawer_view_saved_query" is descriptive, but confirm that naming is consistent across your analytics events for easier data querying. Also verify that no PII or user-specific IDs are included here without anonymization.
375-379: Event usage clarity.
"query_drawer_opened" is triggered once the drawer loads. This looks fine. Consider capturing additional context if needed (e.g., user roles or environment data) — but be mindful about exposing PII.
393-400: Verify correctness of run flow and blockUid usage.
Capturing "query_drawer_rendered" when rendering the overlay is consistent with your analytics approach. However, ensure that blockUid is the correct, non-sensitive identifier. If it’s user-specific, consider anonymizing or hashing it.
apps/roam/src/components/DiscourseContext.tsx (1)
348-350: Confirm event property compliance with PII policy.
posthog.capture("show_discourse_context_result", { uid: uid }) logs a raw uid. If there is a chance this could be user-related PII, consider hashing or anonymizing this value to comply with privacy guidelines.
apps/roam/src/components/ExportDiscourseContext.tsx (1)
557-561: Check for potential PII in export analytics.
Logging "discourse_graph_export_button_clicked" with the current degreesIn and degreesOut is fine, but confirm that no user or page-specific data is included inadvertently. If user data needs to be included, consider hashing or removing sensitive details.
apps/roam/src/components/canvas/Tldraw.tsx (3)
61-61: Double-check environment variable usage for analytics.
Storing keys in code is sometimes unavoidable, but if there's a chance to place it in environment variables for more security, please do so.
600-602: Caution with storing user data.
You're passing potentially user-generated data (triples) to posthog.capture("Canvas Relations Created", {...}). Ensure that any PII within these data structures is sanitized to comply with privacy standards.
851-853: Conditionally log "Canvas-mounted" event.
When the app re-initializes or re-renders, this event might be repeatedly captured. Add checks or gating logic so it only fires once.
apps/roam/src/components/ResultsView/ResultsView.tsx (1)
1046-1051: Guard the export event.
You capture an event when the user clicks "Share Data." If the user cancels or never proceeds with the actual export, we might record misleading analytics. Consider capturing again upon successful export completion or clarifying the event name to "Export Initiated."
apps/roam/src/index.ts (3)
27-28: Revisit API key handling to prevent exposure.
This key is most likely sensitive. If possible, retrieve it from environment variables or a secure store. Committing the key to the repo can pose security risks.
32-51: Ensure you truly need these denied properties.
While this is explicitly disallowing certain default properties, confirm that these are indeed the right set for your privacy rules and that no further sensitive fields slip through, especially if your data shape changes.
57-57: Check for race conditions or call order concerns.
By calling initPostHog() before many other steps, you might capture early events. Verify that analytics calls from other modules happen only after PostHog is truly initialized.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
apps/roam/src/components/canvas/Tldraw.tsx (2)
Line range hint
764-770: Improve error handling and user feedbackThe error handling could be improved in several ways:
- Error messages like "Shape not found" could be more user-friendly
- Error handling is scattered across different handlers
- Missing React Error Boundary for graceful fallbacks
Consider implementing a centralized error handling utility:
// errorUtils.ts export const handleCanvasError = (error: Error | string, context?: string) => { const message = typeof error === 'string' ? error : error.message; renderToast({ id: "tldraw-error", intent: "warning", content: `${context ? `${context}: ` : ''}${message}`, }); }; // Usage in component if (!shape) { return handleCanvasError("Unable to locate the requested shape on canvas", "Navigation Error"); }Also, consider adding an Error Boundary:
class CanvasErrorBoundary extends React.Component { componentDidCatch(error: Error) { handleCanvasError(error, "Canvas Error"); } render() { return this.props.children; } }Also applies to: 776-783
Line range hint
919-947: Enhance accessibility supportThe canvas implementation lacks proper accessibility features:
- Missing ARIA labels and roles
- Limited keyboard navigation support
- No screen reader considerations
Add proper accessibility support:
<div className={`z-10 h-full w-full overflow-hidden rounded-md border border-gray-300 bg-white ${ maximized ? "absolute inset-0" : "relative" }`} id={`roamjs-tldraw-canvas-container`} ref={containerRef} + role="application" + aria-label="Discourse Graph Canvas" tabIndex={-1} + aria-describedby="canvas-instructions" > + <div id="canvas-instructions" className="sr-only"> + Use arrow keys to navigate the canvas. Press Space to pan, and Shift+Space to zoom. + </div>Consider adding keyboard shortcuts help:
const KEYBOARD_SHORTCUTS = { pan: 'Space', zoom: 'Shift + Space', select: 'S', // ... add more shortcuts }; const KeyboardHelp = () => ( <div role="dialog" aria-label="Keyboard shortcuts"> {Object.entries(KEYBOARD_SHORTCUTS).map(([action, key]) => ( <div key={action}> <kbd>{key}</kbd>: {action} </div> ))} </div> );
🧹 Nitpick comments (4)
apps/roam/src/components/canvas/Tldraw.tsx (2)
Line range hint
764-897: Optimize performance in event handlersThe event handlers contain complex logic that could impact performance:
- Complex calculations in event handlers
- Nested conditions increasing cognitive complexity
- Multiple DOM manipulations
Consider these optimizations:
- Extract complex logic into separate functions:
const handleShapeInteraction = (e: TLPointerEvent, shape: TLShape) => { if (!isValidForInteraction(e, shape)) return; if (e.shiftKey) { handleShiftKeyInteraction(shape); } else if (e.ctrlKey || e.metaKey) { handleCtrlKeyInteraction(shape); } }; const isValidForInteraction = (e: TLPointerEvent, shape: TLShape) => { const validModifier = e.shiftKey || e.ctrlKey || e.metaKey; return validModifier && !app.selectedIds.length && shape?.props.uid; };
- Debounce frequent operations:
const debouncedCenterOnPoint = debounce( (x: number, y: number) => { app.centerOnPoint(x, y, { duration: 500, easing: (t) => t * t }); }, 100 );
Line range hint
4-4: Address remaining code quality issuesSeveral improvements could enhance code maintainability:
- TODO comment needs addressing
- CSS should be extracted to a separate file
- Magic numbers should be constants
Consider these improvements:
- Extract CSS to a separate file:
// styles/canvas.css .canvas-container { z-index: 10; height: 100%; width: 100%; overflow: hidden; border-radius: 0.375rem; border: 1px solid #d1d5db; background-color: white; } // Import in component import './styles/canvas.css';
- Extract magic numbers:
const CONSTANTS = { ANIMATION_DURATION: 500, DEFAULT_HEIGHT: '70vh', Z_INDEX: 10, } as const;Would you like me to help implement these improvements or create GitHub issues to track them?
Also applies to: 919-947
apps/roam/src/components/QueryDrawer.tsx (2)
24-24: Consider centralizing analytics logicInstead of importing PostHog directly in components, consider creating a centralized analytics service to manage all tracking events. This would:
- Ensure consistent event naming and properties
- Make it easier to manage PII filtering
- Simplify future analytics changes
Create an analytics service like this:
// services/analytics.ts import posthog from 'posthog-js'; export const Analytics = { queryDrawer: { viewSavedQuery: (queryUid: string, isSavedToPage: boolean) => { posthog.capture('Query drawer: view saved query', { queryUid, isSavedToPage, }); }, // ... other events }, };
Line range hint
375-391: Enhance analytics context for drawer openingThe current implementation only tracks a static source. Consider:
- Adding more context about where the drawer was opened from
- Including the trigger action (e.g., button click, keyboard shortcut)
-export const openQueryDrawer = (onloadArgs: OnloadArgs) => { +export const openQueryDrawer = (onloadArgs: OnloadArgs, context?: { source: string; trigger: string }) => { posthog.capture("Query drawer: opened", { - source: "drawer_open", + source: context?.source || "drawer_open", + trigger: context?.trigger || "unknown", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/roam/src/components/DiscourseContext.tsx(2 hunks)apps/roam/src/components/DiscourseNodeMenu.tsx(2 hunks)apps/roam/src/components/ExportDiscourseContext.tsx(2 hunks)apps/roam/src/components/QueryBuilder.tsx(3 hunks)apps/roam/src/components/QueryDrawer.tsx(5 hunks)apps/roam/src/components/ResultsView/ResultsView.tsx(4 hunks)apps/roam/src/components/ResultsView/views/Kanban.tsx(2 hunks)apps/roam/src/components/canvas/CanvasDrawer.tsx(2 hunks)apps/roam/src/components/canvas/Tldraw.tsx(4 hunks)apps/roam/src/index.ts(1 hunks)apps/roam/src/utils/createDiscourseNode.ts(2 hunks)apps/roam/src/utils/triplesToBlocks.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/roam/src/index.ts
- apps/roam/src/utils/createDiscourseNode.ts
- apps/roam/src/components/DiscourseNodeMenu.tsx
- apps/roam/src/components/canvas/CanvasDrawer.tsx
- apps/roam/src/utils/triplesToBlocks.ts
- apps/roam/src/components/ExportDiscourseContext.tsx
- apps/roam/src/components/ResultsView/views/Kanban.tsx
- apps/roam/src/components/DiscourseContext.tsx
- apps/roam/src/components/QueryBuilder.tsx
- apps/roam/src/components/ResultsView/ResultsView.tsx
🔇 Additional comments (1)
apps/roam/src/components/QueryDrawer.tsx (1)
59-62: Verify PII handling in analytics events
The queryUid being tracked could potentially contain sensitive information. Consider:
- Verifying if
queryUidcontains or could contain PII - Implementing a sanitization function for analytics properties
Run this script to check for potential PII in query UIDs:
✅ Verification successful
Based on the analysis of the code, I can now provide a final response:
queryUid is a safe identifier, not containing PII
The queryUid used in analytics is a Roam page identifier that is:
- Generated from a fixed page title pattern
discourse-graph/queries/${target} - Used to reference saved queries in a structured format
- Not containing any user-input or personal information
The analytics event captures:
queryUid: A system-generated page identifierisSavedToPage: A boolean flag
The rationale for this verification:
- The code shows that queryUids are generated from a fixed page path pattern using
getPageUidByPageTitle("discourse-graph/queries/${target}") - Saved queries are stored in a structured format with system-generated identifiers
- The implementation shows no user data or personal information being incorporated into the uid generation process
- The analytics event is using these system identifiers to track feature usage, not user data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential PII patterns in query UIDs
# Look for email-like patterns or personally identifiable text in queries
rg -A 3 'queryUid.*' | rg -i 'email|name|address|phone|ssn|social|security'
Length of output: 77
Script:
#!/bin/bash
# Let's first understand how queryUid is generated/used
ast-grep --pattern 'queryUid'
Length of output: 272
Script:
#!/bin/bash
# Let's also check for any uid generation patterns
rg -A 5 'uid.*='
Length of output: 65555
Script:
#!/bin/bash
# Check the type definition of the uid
fd "types?.ts" --exec rg -A 3 "interface.*Query"
Length of output: 50
Script:
#!/bin/bash
# Let's check how queryUid is generated in the context of saved queries
rg -B 3 -A 3 "queryUid.*="
Length of output: 727
Script:
#!/bin/bash
# Check for any PII-related patterns in the query implementation
rg -A 5 "Query.*{"
Length of output: 100
Script:
#!/bin/bash
# Check how the uid is used in the analytics context
rg -A 3 "posthog.*capture.*uid"
Length of output: 33
Script:
#!/bin/bash
# Let's check how queries are saved and what they contain
rg -A 5 'discourse-graph/queries'
Length of output: 2137
Script:
#!/bash
# Check the implementation around saved queries
ast-grep --pattern 'function.*Query'
Length of output: 38
Script:
#!/bin/bash
# Check for any sensitive data patterns in query-related code
rg -A 5 "saved.*query" -i
Length of output: 6825
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/Tldraw.tsx (1)
600-602: LGTM!The "Canvas Relations: Created" event captures appropriate non-PII data. Consider adding more metrics like
relationCountfor better analytics insights.posthog.capture("Canvas Relations: Created", { relation: relationLabel, + relationCount: newTriples.length, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(2 hunks)apps/roam/src/components/canvas/Tldraw.tsx(3 hunks)apps/roam/src/components/settings/QueryPagesPanel.tsx(2 hunks)apps/roam/src/utils/createDiscourseNode.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/roam/src/utils/createDiscourseNode.ts
🔇 Additional comments (6)
apps/roam/src/components/canvas/Tldraw.tsx (2)
61-61: LGTM!The PostHog analytics import is correctly placed at the top of the file.
846-848:⚠️ Potential issueReview PII handling in analytics event
The "Canvas: mounted" event captures the title parameter which could potentially contain PII. Consider capturing non-PII metrics instead.
posthog.capture("Canvas: mounted", { - title: title, + hasTitle: !!title, + titleLength: title.length, });Likely invalid or redundant comment.
apps/roam/src/components/settings/QueryPagesPanel.tsx (2)
2-2: Ensure event naming consistency and watch for PII.Importing
posthoghere seems fine, though be mindful of event naming consistency across the codebase. Also confirm thatvaluedoes not contain sensitive or personally identifiable data.
50-50: Safeguard user data in analytics properties.Capturing the “newType” property is useful. However, confirm that this property isn’t storing user-specific or private info that might qualify as PII.
apps/roam/src/components/canvas/DiscourseNodeUtil.tsx (2)
29-29: Validate import usage.Importing
posthoglooks correct. Be sure it’s properly initialized elsewhere before capturing events to avoid uninitialized analytics calls.
390-395: Avoid inadvertently capturing PII in “text” property.Although logging
uid,text, andactionis valuable, verify thattextdoes not contain any user-identifying details if the user or block data can be considered sensitive. Consider sanitizing or hashing the content before capturing if it is at risk of exposing PII.Would you like help creating a sanitization utility for these analytics events?
|
@sid597 my OCD required me to rename the capture events 😅 Just pull these changes in before doing the final edits. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/roam/src/components/canvas/Tldraw.tsx (1)
846-848: Add error handling for analytics callsThe analytics events should be wrapped in try-catch blocks to gracefully handle potential failures.
Apply this diff to add error handling:
-posthog.capture("Canvas: Mounted", { - title: title, -}); +try { + posthog.capture("Canvas: Mounted", { + hasTitle: !!title, + }); +} catch (error) { + console.error("Failed to capture analytics event:", error); +}apps/roam/src/components/ResultsView/ResultsView.tsx (1)
42-42: Consider centralizing PostHog initialization and error handling.The PostHog import suggests direct usage. Consider creating a wrapper service to centralize analytics initialization, error handling, and PII protection.
Create an analytics service that provides:
- Centralized error handling
- Standardized event naming
- Consistent PII protection
- Type safety for events and properties
Example implementation:
// services/analytics.ts import posthog from 'posthog-js'; type AnalyticsEvent = { results_view_layout_changed: { pageUidHash: string | null; oldLayout: string; newLayout: string; }; // ... other events }; export const analytics = { track<T extends keyof AnalyticsEvent>( event: T, properties: AnalyticsEvent[T] ) { try { posthog.capture(event, properties); } catch (error) { console.error(`Failed to capture analytics event ${event}:`, error); } }, hashPageUid(uid: string | null): string | null { return uid ? window.btoa(uid).slice(0, 10) : null; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/roam/src/components/DiscourseContext.tsx(2 hunks)apps/roam/src/components/DiscourseNodeMenu.tsx(2 hunks)apps/roam/src/components/ExportDiscourseContext.tsx(2 hunks)apps/roam/src/components/QueryBuilder.tsx(3 hunks)apps/roam/src/components/QueryDrawer.tsx(6 hunks)apps/roam/src/components/ResultsView/ResultsView.tsx(4 hunks)apps/roam/src/components/canvas/CanvasDrawer.tsx(2 hunks)apps/roam/src/components/canvas/DiscourseNodeUtil.tsx(2 hunks)apps/roam/src/components/canvas/Tldraw.tsx(3 hunks)apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx(2 hunks)apps/roam/src/components/settings/QueryPagesPanel.tsx(2 hunks)apps/roam/src/utils/createDiscourseNode.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/roam/src/utils/createDiscourseNode.ts
- apps/roam/src/components/ExportDiscourseContext.tsx
- apps/roam/src/components/canvas/DiscourseNodeUtil.tsx
- apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx
- apps/roam/src/components/canvas/CanvasDrawer.tsx
- apps/roam/src/components/DiscourseNodeMenu.tsx
- apps/roam/src/components/DiscourseContext.tsx
- apps/roam/src/components/settings/QueryPagesPanel.tsx
- apps/roam/src/components/QueryDrawer.tsx
- apps/roam/src/components/QueryBuilder.tsx
🔇 Additional comments (5)
apps/roam/src/components/canvas/Tldraw.tsx (2)
600-602: Review analytics event data for PII exposureThe analytics event "Canvas Relation: Created" captures the relation label which could potentially contain sensitive information.
Apply this diff to minimize PII exposure:
posthog.capture("Canvas Relation: Created", { - relation: relationLabel, + hasRelation: !!relationLabel, });
61-61: Verify PostHog initializationThe PostHog import is present, but we should verify its proper initialization.
✅ Verification successful
PostHog is properly initialized in the codebase
PostHog is correctly initialized in both the website and roam applications:
- In
apps/roam/src/index.ts, PostHog is initialized with a specific project key and configuration- The import in
apps/roam/src/components/canvas/Tldraw.tsxis valid as it's used within the same application where PostHog is initialized🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for PostHog initialization in the codebase # Look for PostHog initialization rg -A 5 "posthog\.init" # Check for environment variables or configuration related to PostHog rg -l "POSTHOG|posthog" --type yaml --type jsonLength of output: 858
apps/roam/src/components/ResultsView/ResultsView.tsx (3)
902-913: 🛠️ Refactor suggestionImprove analytics implementation for column view changes.
The analytics event for column view changes needs similar improvements.
Apply this diff:
-posthog.capture("Results View: Column View Changed", { - pageUid: parentUid, - oldMode: mode, - newMode: m, -}); +try { + posthog.capture("results_view_column_changed", { + pageUidHash: parentUid ? window.btoa(parentUid).slice(0, 10) : null, + oldMode: mode, + newMode: m, + }); +} catch (error) { + console.error("Failed to capture analytics event:", error); +}Likely invalid or redundant comment.
555-560: 🛠️ Refactor suggestionAdd error handling and protect PII in analytics event.
The analytics event for layout changes needs improvements in error handling and PII protection.
Apply this diff to improve the implementation:
-posthog.capture("Results View: Layout Changed", { - pageUid: parentUid, - oldLayout: layoutMode, - newLayout: l.id, -}); +try { + posthog.capture("results_view_layout_changed", { + pageUidHash: parentUid ? window.btoa(parentUid).slice(0, 10) : null, + oldLayout: layoutMode, + newLayout: l.id, + }); +} catch (error) { + console.error("Failed to capture analytics event:", error); +}Changes made:
- Added error handling for analytics calls
- Protected PII by hashing the pageUid
- Standardized event name to snake_case
Likely invalid or redundant comment.
1049-1054: 🛠️ Refactor suggestionEnhance analytics for share data events.
The share data analytics event needs improvements in error handling, PII protection, and additional metadata.
Apply this diff:
-posthog.capture("Results View: Share Data Clicked", { - parentUid: parentUid, - resultCount: results.length, - columnCount: columns.length, - layout: layoutMode, -}); +try { + posthog.capture("results_view_share_clicked", { + pageUidHash: parentUid ? window.btoa(parentUid).slice(0, 10) : null, + resultCount: results.length, + columnCount: columns.length, + layout: layoutMode, + hasFilters: columnFilters.length > 0, + hasSort: activeSort.length > 0, + }); +} catch (error) { + console.error("Failed to capture analytics event:", error); +}Changes made:
- Added error handling
- Protected PII
- Standardized event name
- Added relevant metadata about filters and sorting
Likely invalid or redundant comment.
|
@mdroidian ready for review |
|
good catch my bad, its not required in React only nodejs |
2 participants
Initially
This will likely evolve over time, but initially basic usage would be good:
For the following we have to configure the dashboard, based on what we are collecting in the PII we would be able to query for some and maybe not for others.
Note: we'll have to be careful with Personally Identifiable Information.
Going forward
Then for some interesting usage stats, things like:
At a high level, if it's a feature we are investing time into, we should have some insight into it.
Eventually, creating user-funnels from emerging patterns could inform new features, dev paths for new plugins, ux optimizations, etc.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
posthog-jsversion^1.203.2.