analytics in sdk #1115
analytics in sdk #1115
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a comprehensive update to the Screenpipe JavaScript SDK, focusing on analytics integration and expanding example projects. The changes span multiple components of the SDK, including the browser and node SDKs, common types, and several new example projects. Key modifications include updating the package versions, adding PostHog-based analytics capabilities, and introducing new methods for event tracking within the Sequence DiagramsequenceDiagram
participant User
participant BrowserPipe
participant Analytics
participant PostHog
User->>BrowserPipe: Invoke method
BrowserPipe->>Analytics: initAnalyticsIfNeeded()
Analytics->>PostHog: Initialize
PostHog-->>Analytics: Initialization complete
BrowserPipe->>Analytics: captureEvent()
Analytics->>PostHog: Send event
PostHog-->>Analytics: Event recorded
This sequence diagram illustrates the typical flow of analytics initialization and event capturing in the updated SDK, showing how methods trigger analytics setup and event tracking through the PostHog service. Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
screenpipe-js/examples/basic-transcription/tsconfig.json (1)
22-26: Consider enabling stricter checks for better code quality.While disabling these checks makes development easier, enabling them could prevent potential issues:
noUnusedLocals: Helps identify dead codenoUnusedParameters: Ensures all parameters are utilizednoPropertyAccessFromIndexSignature: Prevents accidental property accessThis is particularly important for analytics where unused variables might indicate missing data collection points.
- "noUnusedLocals": false, - "noUnusedParameters": false, - "noPropertyAccessFromIndexSignature": false + "noUnusedLocals": true, + "noUnusedParameters": true, + "noPropertyAccessFromIndexSignature": truescreenpipe-js/browser-sdk/src/index.ts (1)
57-64: Use consistent parameter names in interface methodsFor clarity and consistency, consider using the same parameter name for event names in both
captureEventandcaptureMainFeatureEventmethods. Renaming the parameters toeventNamewould improve readability.Apply this diff to update the parameter names:
export interface BrowserPipe { // ... captureEvent: ( - event: string, + eventName: string, properties?: Record<string, any> ) => Promise<void>; captureMainFeatureEvent: ( - name: string, + eventName: string, properties?: Record<string, any> ) => Promise<void>; }And update the implementations accordingly.
screenpipe-js/common/analytics.ts (1)
8-13: Enhance initialization logicThe current initialization logic could be more robust with proper error handling and cleanup.
Consider this enhanced implementation:
function initPosthog() { if (!initialized) { - posthog.init(POSTHOG_KEY, { api_host: POSTHOG_HOST }); - initialized = true; + try { + posthog.init(POSTHOG_KEY, { api_host: POSTHOG_HOST }); + initialized = true; + } catch (error) { + console.error('Failed to initialize PostHog:', error); + throw error; + } } + return initialized; }screenpipe-js/examples/query-screenpipe/index.ts (1)
10-16: Extract configuration values to constantsTime-related values and query parameters should be configurable constants.
+const QUERY_CONFIG = { + MINUTES_AGO: 5, + RESULT_LIMIT: 10, + CONTENT_TYPE: "all" as const +}; + // get content from last 5 minutes -const fiveMinutesAgo = new Date(Date.now() - 5 * 60 * 1000).toISOString(); +const fiveMinutesAgo = new Date(Date.now() - QUERY_CONFIG.MINUTES_AGO * 60 * 1000).toISOString(); const results = await pipe.queryScreenpipe({ startTime: fiveMinutesAgo, - limit: 10, - contentType: "all", // can be "text", "vision", or "all" + limit: QUERY_CONFIG.RESULT_LIMIT, + contentType: QUERY_CONFIG.CONTENT_TYPE, });screenpipe-js/common/types.ts (1)
209-209: Add JSDoc documentation for analyticsEnabled settingThe new setting should be documented for better developer experience.
+ /** + * Whether to enable analytics collection. + * When enabled, usage data will be collected using PostHog. + * @default true + */ analyticsEnabled: boolean;screenpipe-js/node-sdk/src/SettingsManager.ts (1)
32-32: Consider privacy implications of default-enabled analyticsAnalytics being enabled by default (
analyticsEnabled: true) might raise privacy concerns. Consider:
- Making it opt-in by default
- Adding clear documentation about what data is collected
- Ensuring GDPR compliance
screenpipe-js/node-sdk/src/index.ts (2)
234-240: Consider rate limiting event capture methodsThe public event capture methods could benefit from rate limiting to prevent excessive analytics events.
+ private rateLimiter = new Map<string, number>(); + private readonly RATE_LIMIT_MS = 1000; // 1 event per second + public async captureEvent( eventName: string, properties?: Record<string, any> ) { + const now = Date.now(); + const lastCall = this.rateLimiter.get(eventName) || 0; + if (now - lastCall < this.RATE_LIMIT_MS) { + return; + } + this.rateLimiter.set(eventName, now); await this.initAnalyticsIfNeeded(); return captureEvent(eventName, properties); }Also applies to: 242-248
52-54: Enhance error tracking with more contextThe error tracking events could include more context to aid debugging:
- Stack traces for errors
- Request/response data (excluding sensitive information)
- System state information
Also applies to: 57-60, 125-128, 131-134, 149-151, 188-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
screenpipe-js/browser-sdk/bun.lockbis excluded by!**/bun.lockbscreenpipe-js/examples/basic-transcription/bun.lockbis excluded by!**/bun.lockbscreenpipe-js/examples/capture-main-feature/bun.lockbis excluded by!**/bun.lockbscreenpipe-js/examples/query-screenpipe/bun.lockbis excluded by!**/bun.lockbscreenpipe-js/examples/vision-monitor/bun.lockbis excluded by!**/bun.lockbscreenpipe-js/node-sdk/bun.lockbis excluded by!**/bun.lockb
📒 Files selected for processing (30)
screenpipe-js/browser-sdk/package.json(2 hunks)screenpipe-js/browser-sdk/src/index.ts(6 hunks)screenpipe-js/common/analytics.ts(1 hunks)screenpipe-js/common/types.ts(1 hunks)screenpipe-js/examples/basic-transcription/.gitignore(1 hunks)screenpipe-js/examples/basic-transcription/README.md(1 hunks)screenpipe-js/examples/basic-transcription/index.ts(1 hunks)screenpipe-js/examples/basic-transcription/package.json(1 hunks)screenpipe-js/examples/basic-transcription/tsconfig.json(1 hunks)screenpipe-js/examples/capture-main-feature/.gitignore(1 hunks)screenpipe-js/examples/capture-main-feature/README.md(1 hunks)screenpipe-js/examples/capture-main-feature/index.ts(1 hunks)screenpipe-js/examples/capture-main-feature/package.json(1 hunks)screenpipe-js/examples/capture-main-feature/tsconfig.json(1 hunks)screenpipe-js/examples/query-screenpipe/.gitignore(1 hunks)screenpipe-js/examples/query-screenpipe/README.md(1 hunks)screenpipe-js/examples/query-screenpipe/index.ts(1 hunks)screenpipe-js/examples/query-screenpipe/package.json(1 hunks)screenpipe-js/examples/query-screenpipe/tsconfig.json(1 hunks)screenpipe-js/examples/vision-monitor/.gitignore(1 hunks)screenpipe-js/examples/vision-monitor/README.md(1 hunks)screenpipe-js/examples/vision-monitor/index.ts(1 hunks)screenpipe-js/examples/vision-monitor/package.json(1 hunks)screenpipe-js/examples/vision-monitor/tsconfig.json(1 hunks)screenpipe-js/node-sdk/package.json(2 hunks)screenpipe-js/node-sdk/src/Scheduler.ts(0 hunks)screenpipe-js/node-sdk/src/SettingsManager.ts(1 hunks)screenpipe-js/node-sdk/src/index.ts(7 hunks)screenpipe-js/node-sdk/tests/loadPipeConfig.test.ts(0 hunks)screenpipe-js/node-sdk/tests/scheduler.test.ts(0 hunks)
💤 Files with no reviewable changes (3)
- screenpipe-js/node-sdk/src/Scheduler.ts
- screenpipe-js/node-sdk/tests/loadPipeConfig.test.ts
- screenpipe-js/node-sdk/tests/scheduler.test.ts
✅ Files skipped from review due to trivial changes (14)
- screenpipe-js/examples/basic-transcription/README.md
- screenpipe-js/examples/capture-main-feature/README.md
- screenpipe-js/examples/query-screenpipe/README.md
- screenpipe-js/examples/vision-monitor/README.md
- screenpipe-js/examples/vision-monitor/.gitignore
- screenpipe-js/examples/vision-monitor/tsconfig.json
- screenpipe-js/examples/capture-main-feature/.gitignore
- screenpipe-js/examples/vision-monitor/package.json
- screenpipe-js/examples/capture-main-feature/package.json
- screenpipe-js/examples/query-screenpipe/tsconfig.json
- screenpipe-js/examples/basic-transcription/package.json
- screenpipe-js/examples/query-screenpipe/.gitignore
- screenpipe-js/examples/basic-transcription/.gitignore
- screenpipe-js/examples/query-screenpipe/package.json
🧰 Additional context used
🪛 Gitleaks (8.21.2)
screenpipe-js/common/analytics.ts
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-ubuntu
- GitHub Check: test-windows
- GitHub Check: test-macos
- GitHub Check: test-windows
- GitHub Check: test-linux
- GitHub Check: Run Windows OCR benchmark
- GitHub Check: test-windows
- GitHub Check: Run Apple OCR benchmark
- GitHub Check: test-macos
- GitHub Check: test-ubuntu
🔇 Additional comments (9)
screenpipe-js/examples/basic-transcription/tsconfig.json (3)
3-10: LGTM! Language feature settings are well configured.The configuration enables modern JavaScript features and includes necessary DOM types, which aligns well with the SDK's analytics capabilities.
11-16: Verify bundler compatibility with all target environments.The
"moduleResolution": "bundler"setting is relatively new. Consider adding a comment to document minimum TypeScript version requirements or providing a fallback configuration for older environments.Run this script to check TypeScript version requirements in the project:
17-21: Consider implications of skipLibCheck.While
skipLibCheck: trueimproves compilation performance, it might hide type inconsistencies in dependencies. This is particularly important for an analytics SDK where type safety is crucial for accurate data collection.Run this script to check for potential type issues in dependencies:
screenpipe-js/examples/capture-main-feature/tsconfig.json (1)
1-27: Identical configuration to basic-transcription example.This configuration matches the one in
basic-transcription/tsconfig.json. The previous review comments apply here as well.screenpipe-js/examples/query-screenpipe/index.ts (1)
9-10:⚠️ Potential issueRemove hardcoded file path
The example contains a hardcoded file path which may not work for other users.
Use a relative path instead:
- "1. paste this in a new terminal: 'open /Users/louisbeaumont/Documents/screenpipe/screenpipe-js/examples/vision-monitor/screenshots/viewer.html'" + "1. paste this in a new terminal: 'open ./screenshots/viewer.html'"Likely invalid or redundant comment.
screenpipe-js/node-sdk/src/SettingsManager.ts (1)
30-30: Verify memory implications of increased context sizeThe
aiMaxContextCharshas been increased 4x from 128K to 512K characters. While this provides more context for AI processing, it could significantly impact memory usage.✅ Verification successful
Memory implications of increased context size are acceptable
The increased context size from 128K to 512K is safe as it:
- Has a hard upper limit enforced by UI controls (1K-512K range)
- Is used only for basic content length validation
- Does not involve complex memory management or buffering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any memory-related configurations or warnings in the codebase rg -i "memory|heap|ram" -g "!{*.lock,package-lock.json,yarn.lock,dist/*}"Length of output: 78657
Script:
#!/bin/bash # Find usage of aiMaxContextChars and surrounding context rg -A 5 -B 5 "aiMaxContextChars" --type ts # Look for memory-related code or comments near AI context handling ast-grep --pattern 'aiMaxContextChars = $_' -l ts # Search for memory management related code in TypeScript files rg -i "(memory|heap|buffer).*(limit|max|size)" --type tsLength of output: 17217
screenpipe-js/node-sdk/src/index.ts (1)
224-232: LGTM: Analytics initialization is well-implementedThe
initAnalyticsIfNeededmethod properly:
- Checks initialization state
- Respects user settings
- Requires user identification
screenpipe-js/node-sdk/package.json (1)
30-30: Verify PostHog version compatibilityThe PostHog JS SDK version
1.205.0is not the latest. Please verify compatibility with the latest version.screenpipe-js/browser-sdk/package.json (1)
30-30: Maintain consistent PostHog versionsSame comment as in node-sdk about PostHog version verification.
| private userId?: string; | ||
| private userProperties?: Record<string, any>; | ||
|
|
||
| private async initAnalyticsIfNeeded() { | ||
| if (this.analyticsInitialized || !this.userId) return; | ||
|
|
||
| try { | ||
| const response = await fetch("http://localhost:3030/settings"); | ||
| if (response.ok) { | ||
| const settings = await response.json(); | ||
| if (settings.analyticsEnabled) { | ||
| await identifyUser(this.userId, this.userProperties); | ||
| this.analyticsInitialized = true; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.error("failed to fetch settings:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Initialize userId and userProperties to enable analytics
The userId and userProperties are not being set anywhere in the BrowserPipeImpl class. As a result, the initAnalyticsIfNeeded method will not initialize analytics because it checks if (this.analyticsInitialized || !this.userId) return;. This means analytics may never be initialized, and events will not be properly captured.
Consider adding a method to set the user identity. Apply this diff to add a setUser method:
class BrowserPipeImpl implements BrowserPipe {
private analyticsInitialized = false;
private userId?: string;
private userProperties?: Record<string, any>;
+ public setUser(userId: string, userProperties?: Record<string, any>) {
+ this.userId = userId;
+ this.userProperties = userProperties;
+ }
private async initAnalyticsIfNeeded() {
if (this.analyticsInitialized || !this.userId) return;
// ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private userId?: string; | |
| private userProperties?: Record<string, any>; | |
| private async initAnalyticsIfNeeded() { | |
| if (this.analyticsInitialized || !this.userId) return; | |
| try { | |
| const response = await fetch("http://localhost:3030/settings"); | |
| if (response.ok) { | |
| const settings = await response.json(); | |
| if (settings.analyticsEnabled) { | |
| await identifyUser(this.userId, this.userProperties); | |
| this.analyticsInitialized = true; | |
| } | |
| } | |
| } catch (error) { | |
| console.error("failed to fetch settings:", error); | |
| } | |
| } | |
| private analyticsInitialized = false; | |
| private userId?: string; | |
| private userProperties?: Record<string, any>; | |
| public setUser(userId: string, userProperties?: Record<string, any>) { | |
| this.userId = userId; | |
| this.userProperties = userProperties; | |
| } | |
| private async initAnalyticsIfNeeded() { | |
| if (this.analyticsInitialized || !this.userId) return; | |
| try { | |
| const response = await fetch("http://localhost:3030/settings"); | |
| if (response.ok) { | |
| const settings = await response.json(); | |
| if (settings.analyticsEnabled) { | |
| await identifyUser(this.userId, this.userProperties); | |
| this.analyticsInitialized = true; | |
| } | |
| } | |
| } catch (error) { | |
| console.error("failed to fetch settings:", error); | |
| } | |
| } |
| async function startScreenRecorder() { | ||
| console.log("let's send events when our main feature is used ..."); | ||
|
|
||
| await pipe.captureEvent("less_useful_feature", { | ||
| dog: "woof", | ||
| }); | ||
|
|
||
| await pipe.captureMainFeatureEvent("very_useful_feature", { | ||
| cat: "meow", | ||
| }); | ||
| } | ||
|
|
||
| startScreenRecorder().catch(console.error); |
There was a problem hiding this comment.
Set user identity before capturing events
To ensure analytics events are properly captured, you need to set the user ID before calling captureEvent and captureMainFeatureEvent. Since pipe now requires the userId to initialize analytics, consider setting it using the setUser method.
Apply this diff to set the user ID before capturing events:
async function startScreenRecorder() {
+ pipe.setUser("user123", { role: "developer" });
+
console.log("let's send events when our main feature is used ...");
await pipe.captureEvent("less_useful_feature", {
dog: "woof",
});
await pipe.captureMainFeatureEvent("very_useful_feature", {
cat: "meow",
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function startScreenRecorder() { | |
| console.log("let's send events when our main feature is used ..."); | |
| await pipe.captureEvent("less_useful_feature", { | |
| dog: "woof", | |
| }); | |
| await pipe.captureMainFeatureEvent("very_useful_feature", { | |
| cat: "meow", | |
| }); | |
| } | |
| startScreenRecorder().catch(console.error); | |
| async function startScreenRecorder() { | |
| pipe.setUser("user123", { role: "developer" }); | |
| console.log("let's send events when our main feature is used ..."); | |
| await pipe.captureEvent("less_useful_feature", { | |
| dog: "woof", | |
| }); | |
| await pipe.captureMainFeatureEvent("very_useful_feature", { | |
| cat: "meow", | |
| }); | |
| } | |
| startScreenRecorder().catch(console.error); |
| async function monitorTranscriptions() { | ||
| console.log("starting transcription monitor..."); | ||
| console.log( | ||
| "please watch this video: https://youtu.be/UF8uR6Z6KLc?t=180" | ||
| ); | ||
|
|
||
| for await (const chunk of pipe.streamTranscriptions()) { | ||
| const text = chunk.choices[0].text; | ||
| const isFinal = chunk.choices[0].finish_reason === "stop"; | ||
| const device = chunk.metadata?.device; | ||
|
|
||
| console.log(`[${device}] ${isFinal ? "final:" : "partial:"} ${text}`); | ||
| } | ||
| } | ||
|
|
||
| monitorTranscriptions().catch(console.error); |
There was a problem hiding this comment.
Initialize user identity before starting transcription monitoring
To ensure analytics events are properly captured during transcription streaming, you need to set the user ID before starting the stream. Consider setting the user identity using the setUser method.
Apply this diff to set the user ID before starting transcription monitoring:
async function monitorTranscriptions() {
+ pipe.setUser("user123", { role: "user" });
+
console.log("starting transcription monitor...");
console.log(
"please watch this video: https://youtu.be/UF8uR6Z6KLc?t=180"
);
for await (const chunk of pipe.streamTranscriptions()) {
const text = chunk.choices[0].text;
const isFinal = chunk.choices[0].finish_reason === "stop";
const device = chunk.metadata?.device;
console.log(`[${device}] ${isFinal ? "final:" : "partial:"} ${text}`);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function monitorTranscriptions() { | |
| console.log("starting transcription monitor..."); | |
| console.log( | |
| "please watch this video: https://youtu.be/UF8uR6Z6KLc?t=180" | |
| ); | |
| for await (const chunk of pipe.streamTranscriptions()) { | |
| const text = chunk.choices[0].text; | |
| const isFinal = chunk.choices[0].finish_reason === "stop"; | |
| const device = chunk.metadata?.device; | |
| console.log(`[${device}] ${isFinal ? "final:" : "partial:"} ${text}`); | |
| } | |
| } | |
| monitorTranscriptions().catch(console.error); | |
| async function monitorTranscriptions() { | |
| pipe.setUser("user123", { role: "user" }); | |
| console.log("starting transcription monitor..."); | |
| console.log( | |
| "please watch this video: https://youtu.be/UF8uR6Z6KLc?t=180" | |
| ); | |
| for await (const chunk of pipe.streamTranscriptions()) { | |
| const text = chunk.choices[0].text; | |
| const isFinal = chunk.choices[0].finish_reason === "stop"; | |
| const device = chunk.metadata?.device; | |
| console.log(`[${device}] ${isFinal ? "final:" : "partial:"} ${text}`); | |
| } | |
| } | |
| monitorTranscriptions().catch(console.error); |
| export async function identifyUser( | ||
| userId: string, | ||
| properties?: Record<string, any> | ||
| ): Promise<void> { | ||
| initPosthog(); | ||
| posthog.identify(userId, properties); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation to analytics functions
The analytics functions should handle errors and validate inputs properly.
Consider this enhanced implementation:
export async function identifyUser(
userId: string,
properties?: Record<string, any>
): Promise<void> {
- initPosthog();
- posthog.identify(userId, properties);
+ try {
+ if (!userId?.trim()) {
+ throw new Error('userId is required');
+ }
+ initPosthog();
+ await Promise.resolve(posthog.identify(userId, properties));
+ } catch (error) {
+ console.error('Failed to identify user:', error);
+ throw error;
+ }
}Apply similar error handling to captureEvent and captureMainFeatureEvent.
Also applies to: 23-29, 31-40
| const POSTHOG_KEY = "phc_Bt8GoTBPgkCpDrbaIZzJIEYt0CrJjhBiuLaBck1clce"; | ||
| const POSTHOG_HOST = "https://eu.i.posthog.com"; |
There was a problem hiding this comment.
Critical: Move PostHog API key to environment variables
Hardcoding API keys in source code is a security risk. The key should be moved to environment variables.
Replace the hardcoded key with an environment variable:
-const POSTHOG_KEY = "phc_Bt8GoTBPgkCpDrbaIZzJIEYt0CrJjhBiuLaBck1clce";
+const POSTHOG_KEY = process.env.POSTHOG_API_KEY || '';Also, add validation:
function initPosthog() {
if (!initialized) {
+ if (!POSTHOG_KEY) {
+ throw new Error('PostHog API key is required');
+ }
posthog.init(POSTHOG_KEY, { api_host: POSTHOG_HOST });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Gitleaks (8.21.2)
3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| // create screenshots directory | ||
| await fs.mkdir("screenshots", { recursive: true }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory creation
The directory creation should handle potential errors.
- // create screenshots directory
- await fs.mkdir("screenshots", { recursive: true });
+ const SCREENSHOTS_DIR = "screenshots";
+ try {
+ await fs.mkdir(SCREENSHOTS_DIR, { recursive: true });
+ } catch (error) {
+ console.error(`Failed to create ${SCREENSHOTS_DIR} directory:`, error);
+ throw error;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // create screenshots directory | |
| await fs.mkdir("screenshots", { recursive: true }); | |
| const SCREENSHOTS_DIR = "screenshots"; | |
| try { | |
| await fs.mkdir(SCREENSHOTS_DIR, { recursive: true }); | |
| } catch (error) { | |
| console.error(`Failed to create ${SCREENSHOTS_DIR} directory:`, error); | |
| throw error; | |
| } | |
| if (image) { | ||
| const filename = `screenshots/${timestamp}-${window_name}.png`; | ||
| // save to archive | ||
| await fs.writeFile(filename, Buffer.from(image, "base64")); | ||
| // update latest for viewer | ||
| await fs.writeFile( | ||
| "screenshots/latest.png", | ||
| Buffer.from(image, "base64") | ||
| ); | ||
| console.log(`saved screenshot: ${filename}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for file operations
File write operations should handle errors properly.
if (image) {
- const filename = `screenshots/${timestamp}-${window_name}.png`;
- // save to archive
- await fs.writeFile(filename, Buffer.from(image, "base64"));
- // update latest for viewer
- await fs.writeFile(
- "screenshots/latest.png",
- Buffer.from(image, "base64")
- );
- console.log(`saved screenshot: ${filename}`);
+ try {
+ const filename = `${SCREENSHOTS_DIR}/${timestamp}-${window_name}.png`;
+ const imageBuffer = Buffer.from(image, "base64");
+
+ // save to archive
+ await fs.writeFile(filename, imageBuffer);
+ // update latest for viewer
+ await fs.writeFile(`${SCREENSHOTS_DIR}/latest.png`, imageBuffer);
+
+ console.log(`saved screenshot: ${filename}`);
+ } catch (error) {
+ console.error('Failed to save screenshot:', error);
+ // Continue execution as this is not a critical error
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
screenpipe-js/node-sdk/src/index.ts (2)
58-61: Enhance error tracking by capturing error detailsConsider including the error message in analytics to help with debugging.
await captureEvent("error_occurred", { feature: "notification", error: "send_failed", + error_message: error.message, });
Line range hint
68-83: Add analytics for input control actionsConsider tracking input control actions to gather usage insights.
try { + await captureEvent("input_control_sent", { + action_type: action.type, + success: true, + }); return data.success; } catch (error) { + await captureEvent("error_occurred", { + feature: "input_control", + action_type: action.type, + error: "send_failed", + error_message: error.message, + }); console.error("failed to control input:", error); return false; }screenpipe-js/browser-sdk/src/index.ts (1)
188-190: Enhance stream analytics with additional metadataConsider adding more metadata to stream events to better understand usage patterns:
- Stream duration
- Number of chunks processed
- Error counts
Example implementation:
+ private streamStartTime?: number; + private chunkCount = 0; + private errorCount = 0; async *streamTranscriptions() { try { + this.streamStartTime = Date.now(); + this.chunkCount = 0; + this.errorCount = 0; await this.captureEvent("stream_started", { feature: "transcription", }); // ... existing code ... } finally { await this.captureEvent("stream_ended", { feature: "transcription", + duration_ms: Date.now() - (this.streamStartTime ?? 0), + chunks_processed: this.chunkCount, + error_count: this.errorCount, }); } }Also applies to: 227-229, 241-243, 261-263
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
screenpipe-js/browser-sdk/package.json(2 hunks)screenpipe-js/browser-sdk/src/index.ts(6 hunks)screenpipe-js/node-sdk/package.json(2 hunks)screenpipe-js/node-sdk/src/index.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- screenpipe-js/node-sdk/package.json
- screenpipe-js/browser-sdk/package.json
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test-windows
- GitHub Check: test-ubuntu
- GitHub Check: test-linux
- GitHub Check: test-macos
- GitHub Check: test-windows
- GitHub Check: Run Tesseract OCR benchmark
- GitHub Check: test-windows
- GitHub Check: Run Apple OCR benchmark
- GitHub Check: test-ubuntu
- GitHub Check: Run STT benchmark
- GitHub Check: Run Windows OCR benchmark
- GitHub Check: test-macos
🔇 Additional comments (1)
screenpipe-js/browser-sdk/src/index.ts (1)
165-176: Add analytics for input control actionsSimilar to NodePipe, consider tracking input control actions to gather usage insights.
| private analyticsInitialized = false; | ||
| private analyticsEnabled = true; | ||
| private userId?: string; | ||
| private userProperties?: Record<string, any>; |
There was a problem hiding this comment.
Add method to set user identity and align default values
Similar to the issue identified in BrowserPipeImpl, there's no way to set userId and userProperties. Additionally, analyticsEnabled defaults to true here but false in BrowserPipeImpl.
Apply this diff to add the setter and align defaults:
- private analyticsEnabled = true;
+ private analyticsEnabled = false;
private userId?: string;
private userProperties?: Record<string, any>;
+
+ public setUser(userId: string, userProperties?: Record<string, any>) {
+ this.userId = userId;
+ this.userProperties = userProperties;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private analyticsInitialized = false; | |
| private analyticsEnabled = true; | |
| private userId?: string; | |
| private userProperties?: Record<string, any>; | |
| private analyticsInitialized = false; | |
| private analyticsEnabled = false; | |
| private userId?: string; | |
| private userProperties?: Record<string, any>; | |
| public setUser(userId: string, userProperties?: Record<string, any>) { | |
| this.userId = userId; | |
| this.userProperties = userProperties; | |
| } |
| private async initAnalyticsIfNeeded() { | ||
| if (this.analyticsInitialized || !this.userId) return; | ||
|
|
||
| const settings = await this.settings.getAll(); | ||
| this.analyticsEnabled = settings.analyticsEnabled; | ||
| if (settings.analyticsEnabled) { | ||
| await identifyUser(this.userId, this.userProperties); | ||
| this.analyticsInitialized = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for user identification
The identifyUser call could fail, but errors are not handled.
if (settings.analyticsEnabled) {
- await identifyUser(this.userId, this.userProperties);
- this.analyticsInitialized = true;
+ try {
+ await identifyUser(this.userId, this.userProperties);
+ this.analyticsInitialized = true;
+ } catch (error) {
+ console.error("Failed to identify user:", error);
+ await captureEvent("error_occurred", {
+ feature: "analytics",
+ error: "user_identification_failed",
+ error_message: error.message,
+ });
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| await captureEvent("search_performed", { | ||
| content_type: params.contentType, | ||
| result_count: data.pagination.total, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize event capture method usage
There's inconsistent usage of capture methods:
- Direct usage:
await captureEvent("search_performed"...) - Instance method:
await this.captureEvent("notification_sent"...)
This inconsistency could lead to events being captured without proper initialization checks.
Standardize to use instance methods throughout the class:
- await captureEvent("search_performed", {
+ await this.captureEvent("search_performed", {
content_type: params.contentType,
result_count: data.pagination.total,
});Also applies to: 268-284
| try { | ||
| const settings = { analyticsEnabled: false }; // TODO: impl settings browser side somehow ... | ||
| this.analyticsEnabled = settings.analyticsEnabled; | ||
| if (settings.analyticsEnabled) { | ||
| await identifyUser(this.userId, this.userProperties); | ||
| this.analyticsInitialized = true; | ||
| } | ||
| } catch (error) { | ||
| console.error("failed to fetch settings:", error); | ||
| } |
There was a problem hiding this comment.
Implement browser-side settings management
The TODO comment indicates that settings management is not implemented, and analytics are currently hardcoded to be disabled.
Consider implementing one of these approaches:
- Store settings in localStorage/sessionStorage
- Fetch settings from an API endpoint
- Accept settings through constructor/method
Example implementation using localStorage:
try {
- const settings = { analyticsEnabled: false }; // TODO: impl settings browser side somehow ...
+ const storedSettings = localStorage.getItem('screenpipe_settings');
+ const settings = storedSettings
+ ? JSON.parse(storedSettings)
+ : { analyticsEnabled: false };
this.analyticsEnabled = settings.analyticsEnabled;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const settings = { analyticsEnabled: false }; // TODO: impl settings browser side somehow ... | |
| this.analyticsEnabled = settings.analyticsEnabled; | |
| if (settings.analyticsEnabled) { | |
| await identifyUser(this.userId, this.userProperties); | |
| this.analyticsInitialized = true; | |
| } | |
| } catch (error) { | |
| console.error("failed to fetch settings:", error); | |
| } | |
| try { | |
| const storedSettings = localStorage.getItem('screenpipe_settings'); | |
| const settings = storedSettings | |
| ? JSON.parse(storedSettings) | |
| : { analyticsEnabled: false }; | |
| this.analyticsEnabled = settings.analyticsEnabled; | |
| if (settings.analyticsEnabled) { | |
| await identifyUser(this.userId, this.userProperties); | |
| this.analyticsInitialized = true; | |
| } | |
| } catch (error) { | |
| console.error("failed to fetch settings:", error); | |
| } |
.
idea is to allow dev to collect analytics and then we provide them dashboard thing - ...
Summary by CodeRabbit
Release Notes
New Features
Improvements
Examples
Chores
SchedulerandloadPipeConfigfunctionality.