Repository navigation
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces telemetry opt-in/opt-out functionality by adding database schema support, TRPC endpoints, analytics gating logic, a React synchronization component, and settings UI controls to allow users to manage telemetry collection preferences. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as BehaviorSettings UI
participant TRPC as Settings TRPC
participant DB as Local DB
participant Sync as TelemetrySync
participant PostHog
participant Analytics
User->>UI: Toggle telemetry switch
UI->>TRPC: setTelemetryEnabled(enabled)
TRPC->>DB: Update settings.telemetry_enabled
DB-->>TRPC: Confirmed
TRPC-->>UI: Mutation resolves
Sync->>TRPC: Subscribe to getTelemetryEnabled
TRPC->>DB: Read telemetry_enabled
DB-->>TRPC: Return value
TRPC-->>Sync: telemetryEnabled state
Sync->>PostHog: Call opt_in/opt_out_capturing()
PostHog-->>Sync: Confirmed
Analytics->>DB: Check isTelemetryEnabled
DB-->>Analytics: Return telemetry_enabled
alt Telemetry Enabled
Analytics->>Analytics: Proceed with tracking
else Telemetry Disabled
Analytics->>Analytics: Skip tracking
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/desktop/src/main/lib/analytics/index.ts`:
- Around line 26-32: The isTelemetryEnabled function currently swallows DB
errors; update its catch block to log the error with context before returning
the default value — e.g., use console.error with a prefixed message like
"[analytics/read-telemetry] Failed to read telemetry setting:" and include the
caught error object; keep the rest of the logic
(localDb.select().from(settings).get(), DEFAULT_TELEMETRY_ENABLED) unchanged so
it still returns the default on failure.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/analytics/index.ts (1)
39-45: Prefer a params object fortrackto avoid positional arguments.
This aligns with the 2+ params guideline and improves call-site clarity; update call sites accordingly.Proposed refactor
-export function track( - event: string, - properties?: Record<string, unknown>, -): void { +export function track({ + event, + properties, +}: { + event: string; + properties?: Record<string, unknown>; +}): void {As per coding guidelines: Functions with 2+ parameters should accept a single params object with named properties instead of positional arguments.
Comment on lines
+26
to
+32
| function isTelemetryEnabled(): boolean { | ||
| try { | ||
| const row = localDb.select().from(settings).get(); | ||
| return row?.telemetryEnabled ?? DEFAULT_TELEMETRY_ENABLED; | ||
| } catch { | ||
| return DEFAULT_TELEMETRY_ENABLED; | ||
| } |
Contributor
There was a problem hiding this comment.
Log telemetry read failures instead of silently falling back.
The empty catch hides DB errors and makes telemetry state issues hard to diagnose.
Proposed fix
- } catch {
- return DEFAULT_TELEMETRY_ENABLED;
- }
+ } catch (error) {
+ console.error(
+ "[analytics/isTelemetryEnabled] Failed to read telemetry setting:",
+ error,
+ );
+ return DEFAULT_TELEMETRY_ENABLED;
+ }As per coding guidelines: Never swallow errors silently; at minimum log them with context. Use prefixed console logging with pattern [domain/operation] message for all logging.
📝 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.
Suggested change
| function isTelemetryEnabled(): boolean { | |
| try { | |
| const row = localDb.select().from(settings).get(); | |
| return row?.telemetryEnabled ?? DEFAULT_TELEMETRY_ENABLED; | |
| } catch { | |
| return DEFAULT_TELEMETRY_ENABLED; | |
| } | |
| function isTelemetryEnabled(): boolean { | |
| try { | |
| const row = localDb.select().from(settings).get(); | |
| return row?.telemetryEnabled ?? DEFAULT_TELEMETRY_ENABLED; | |
| } catch (error) { | |
| console.error( | |
| "[analytics/isTelemetryEnabled] Failed to read telemetry setting:", | |
| error, | |
| ); | |
| return DEFAULT_TELEMETRY_ENABLED; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/main/lib/analytics/index.ts` around lines 26 - 32, The
isTelemetryEnabled function currently swallows DB errors; update its catch block
to log the error with context before returning the default value — e.g., use
console.error with a prefixed message like "[analytics/read-telemetry] Failed to
read telemetry setting:" and include the caught error object; keep the rest of
the logic (localDb.select().from(settings).get(), DEFAULT_TELEMETRY_ENABLED)
unchanged so it still returns the default on failure.
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit