feat (desktop): disable telemetry option in settings#1048
Conversation
📝 WalkthroughWalkthroughThe PR adds telemetry toggle functionality to the desktop application. It introduces a new Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as BehaviorSettings UI
participant TRPC as TRPC Router
participant DB as Database
participant Sync as TelemetrySync
participant PostHog as PostHog
participant Analytics as Analytics Module
User->>UI: Toggle telemetry switch
UI->>UI: Optimistic update
UI->>TRPC: setTelemetryEnabled({ enabled })
TRPC->>DB: Upsert settings.telemetry_enabled
DB-->>TRPC: Success
TRPC-->>UI: { success: true }
UI->>TRPC: Invalidate getTelemetryEnabled
TRPC->>DB: Query telemetry_enabled
DB-->>TRPC: Value
TRPC-->>Sync: Updated state
Sync->>Sync: Effect triggered
alt enabled === true
Sync->>PostHog: posthog.opt_in_capturing()
else enabled === false
Sync->>PostHog: posthog.opt_out_capturing()
end
PostHog-->>Sync: Updated
Analytics->>DB: Check isTelemetryEnabled()
DB-->>Analytics: telemetry_enabled value
alt Telemetry disabled
Analytics->>Analytics: Skip track() execution
else Telemetry enabled
Analytics->>PostHog: Send event
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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-33: The catch in isTelemetryEnabled silently swallows errors;
update the catch block to log the caught error with context using the prefixed
pattern (e.g. "[analytics/isTelemetryEnabled]") before returning
DEFAULT_TELEMETRY_ENABLED—capture the exception from the failing call to
localDb.select().from(settings).get() and call console.error (or the module's
logger) with a clear message and the error object to ensure errors are surfaced
for debugging.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
410-428: Extract the settings row id to a named constant.
id: 1is a magic number; consider a module‑levelSETTINGS_ROW_IDfor clarity and consistency.Based on learnings: Avoid magic numbers by extracting them to named constants at module top.♻️ Suggested refactor
+const SETTINGS_ROW_ID = 1;- .values({ id: 1, telemetryEnabled: input.enabled }) + .values({ id: SETTINGS_ROW_ID, telemetryEnabled: input.enabled })
| function isTelemetryEnabled(): boolean { | ||
| try { | ||
| const row = localDb.select().from(settings).get(); | ||
| return row?.telemetryEnabled ?? DEFAULT_TELEMETRY_ENABLED; | ||
| } catch { | ||
| return DEFAULT_TELEMETRY_ENABLED; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add error logging in the catch block.
The catch block silently swallows errors without any logging. Per coding guidelines, errors should at minimum be logged with context to aid debugging.
🛡️ Proposed fix to add error logging
function isTelemetryEnabled(): boolean {
try {
const row = localDb.select().from(settings).get();
return row?.telemetryEnabled ?? DEFAULT_TELEMETRY_ENABLED;
- } catch {
+ } catch (error) {
+ console.error("[analytics/telemetry] Failed to read telemetry setting:", error);
return DEFAULT_TELEMETRY_ENABLED;
}
}As per coding guidelines: "Never swallow errors silently; at minimum log them with context" and "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.
| 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/telemetry] 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 - 33, The catch
in isTelemetryEnabled silently swallows errors; update the catch block to log
the caught error with context using the prefixed pattern (e.g.
"[analytics/isTelemetryEnabled]") before returning
DEFAULT_TELEMETRY_ENABLED—capture the exception from the failing call to
localDb.select().from(settings).get() and call console.error (or the module's
logger) with a clear message and the error object to ensure errors are surfaced
for debugging.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features