Uh oh!
There was an error while loading. Please reload this page.
feat(telemetry): replace Loft analytics with PostHog and add Netlify update hosting - #303
Merged
Conversation
…update hosting Part A: Desktop update hosting on Netlify via dl.devsy.sh - Configure electron-builder for dual publish (generic + github) - Add Netlify site config (sites/dl-devsy-sh/) - Add CI step to deploy update metadata (latest*.yml) to Netlify Part B: Replace Loft analytics with PostHog - Replace analytics.loft.rocks HTTP client with PostHog Go SDK - Remove custom double-buffering (PostHog SDK handles batching) - Add posthog-node to desktop app with machine ID hashing - Add IPC bridge for renderer → main analytics tracking - Instrument app lifecycle, update lifecycle, workspace CRUD, provider actions, and page navigation events - Update telemetry documentation for PostHog BREAKING CHANGE: analytics.loft.rocks endpoint removed
✅ Deploy Preview for devsydev canceled.
|
📝 WalkthroughWalkthroughIntegrates PostHog telemetry across desktop and CLI, instruments desktop lifecycle, IPC, updater, and renderer page views, rewrites CLI telemetry client to use posthog-go, and adds release workflow steps to publish desktop update metadata to Netlify. ChangesPostHog Analytics System Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/telemetry/analytics/client.go (1)
80-87: 💤 Low value
Flush()closes the client, making it unusable for subsequent events.
Close()terminates the PostHog client permanently. IfRecordEventis called afterFlush(), it will fail because the underlying client is closed. Consider either:
- Renaming to
Close()to clarify the terminal nature, or- Adding a guard to prevent
RecordEventfrom panicking on a closed client.♻️ Option: Add state tracking to guard against post-close usage
type client struct { phClient posthog.Client + closed bool } func (c *client) RecordEvent(event Event) { + if c.closed {+ return+ } eventData, ok := event["event"] // ... rest of method } func (c *client) Flush() { if Dry { return } + c.closed = true if err := c.phClient.Close(); err != nil { log.Debugf("error flushing PostHog client: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/telemetry/analytics/client.go` around lines 80 - 87, Flush() calls c.phClient.Close() which permanently closes the PostHog client and makes the client unusable; update the client to track closed state and guard RecordEvent (or rename Flush to Close if you prefer a terminal API). Specifically, add a closed bool (or atomic/Mutex-protected flag) to the client struct, set it to true after c.phClient.Close() inside Flush(), and have RecordEvent check that flag and no-op or return early if closed to avoid panics; ensure any concurrent access is synchronized (use sync.Mutex or atomic operations) and keep the symbols: client struct, Flush(), RecordEvent(), and c.phClient.Close() to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Around line 171-179: The deploy step currently gated by "if: runner.os ==
'Linux'" (the block that sets NETLIFY_AUTH_TOKEN/NETLIFY_SITE_ID, runs npm
install -g netlify-cli, creates netlify-deploy/desktop and cp
desktop/release/latest*.yml, then runs "netlify deploy --prod
--dir=netlify-deploy --site=\"$NETLIFY_SITE_ID\"") only runs on Linux and misses
metadata from other matrix runners; remove that OS guard and instead move this
logic into a dedicated final job (e.g., "deploy") that depends_on the matrix
jobs and uses actions/download-artifact to fetch each job's uploaded metadata
artifacts into netlify-deploy before running the same netlify-cli steps, leaving
NETLIFY_* env usage intact.
- Around line 175-179: The Netlify deploy step uses the root netlify.toml
instead of the desktop-specific config and wrong publish dir; update the netlify
deploy invocation to explicitly point to the desktop config and publish
directory by replacing the current netlify CLI call (the line invoking "netlify
deploy --prod --dir=netlify-deploy --site=\"$NETLIFY_SITE_ID\"") with one that
includes --config sites/dl-devsy-sh/netlify.toml and --dir
netlify-deploy/desktop (keep --prod and --site="$NETLIFY_SITE_ID").
In `@desktop/src/main/analytics.ts`:
- Around line 7-9: The code uses a hard-coded POSTHOG_API_KEY
("phc_PLACEHOLDER") and initializes PostHog unconditionally, which can silently
drop telemetry; change POSTHOG_API_KEY to be loaded from config/env and add a
guard before calling PostHog.init (or whatever initialize function is used) to
check that POSTHOG_API_KEY is non-empty and not equal to "phc_PLACEHOLDER"; if
the key is invalid, do not call PostHog.init, emit a clear warning/error via the
existing logger, and ensure downstream code (e.g., any sendEvent/track wrappers)
no-ops when analytics is disabled so release builds never attempt to send using
the placeholder key.
In `@desktop/src/main/ipc.ts`:
- Around line 649-657: The ipc handler for "analytics_track" currently forwards
arbitrary payloads to trackEvent; change the handler (ipcMain.handle
"analytics_track") to validate and constrain args before calling trackEvent:
enforce args.name is a string and whitelist it against an allowed eventNames set
(reject or map unknown names), ensure args.properties is an object of primitive
values (no nested objects/arrays) with a maximum number of keys (e.g., 20) and
maximum string length per value (e.g., 256 chars), strip or redact values
matching PII patterns, and return/ignore invalid keys rather than forwarding
them; on validation failure, log a sanitized warning and do not call trackEvent.
In `@desktop/src/renderer/src/App.svelte`:
- Around line 92-94: The subscription is sending raw route paths (in
unsubLocation / location.subscribe) to analyticsTrack("page_view", { path })
which can leak dynamic IDs; update the callback to sanitize or normalize the
path before sending by implementing a sanitizePath/normalizeRoute helper that
strips or replaces workspace/provider/machine ID segments (or maps them to
placeholders) and call analyticsTrack("page_view", { path: sanitizedPath })
instead so telemetry contains only safe, non-identifying route patterns.
In `@pkg/telemetry/analytics/client.go`:
- Around line 39-57: The machineID extracted with machineID, _ :=
eventData["machine_id"].(string) can be empty and PostHog will reject events;
update the send path in pkg/telemetry/analytics/client.go to validate machineID
after that assertion and handle empties before calling c.phClient.Enqueue (or
when building posthog.Capture): if machineID == "" then either log a warning
including eventType (and the raw eventData), and generate a stable fallback
distinct id (e.g., a UUID) or skip sending the event explicitly, then proceed to
call c.phClient.Enqueue with the validated non-empty DistinctId; ensure this
check happens regardless of Dry and reference the symbols machineID, eventType,
buildProperties, posthog.Capture, and c.phClient.Enqueue so the fix is applied
at the correct spot.
---
Nitpick comments:
In `@pkg/telemetry/analytics/client.go`:
- Around line 80-87: Flush() calls c.phClient.Close() which permanently closes
the PostHog client and makes the client unusable; update the client to track
closed state and guard RecordEvent (or rename Flush to Close if you prefer a
terminal API). Specifically, add a closed bool (or atomic/Mutex-protected flag)
to the client struct, set it to true after c.phClient.Close() inside Flush(),
and have RecordEvent check that flag and no-op or return early if closed to
avoid panics; ensure any concurrent access is synchronized (use sync.Mutex or
atomic operations) and keep the symbols: client struct, Flush(), RecordEvent(),
and c.phClient.Close() to locate the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdc0de1c-71eb-43a2-aab3-22956e1c521c
⛔ Files ignored due to path filters (2)
desktop/package-lock.jsonis excluded by!**/package-lock.jsongo.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/workflows/release.ymldesktop/electron-builder.ymldesktop/package.jsondesktop/src/main/analytics.tsdesktop/src/main/index.tsdesktop/src/main/ipc.tsdesktop/src/main/machine-id.tsdesktop/src/main/updater.tsdesktop/src/renderer/src/App.sveltedesktop/src/renderer/src/lib/ipc/commands.tsdesktop/src/renderer/src/lib/ipc/mock.tsdocs/pages/other-topics/telemetry.mdxgo.modpkg/telemetry/analytics/buffer.gopkg/telemetry/analytics/client.gopkg/telemetry/analytics/types.gosites/dl-devsy-sh/netlify.tomlsites/dl-devsy-sh/public/desktop/.gitkeep
💤 Files with no reviewable changes (2)
- pkg/telemetry/analytics/types.go
- pkg/telemetry/analytics/buffer.go
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Move Netlify deploy to dedicated job collecting metadata from all matrix runners via upload/download-artifact pattern - Use sites/dl-devsy-sh/netlify.toml config explicitly in deploy - Guard against placeholder PostHog API key in both Go and TS clients - Validate and constrain analytics_track IPC payloads (max 20 keys, 64-char key limit, 256-char string value limit) - Normalize route paths in page_view events to strip dynamic IDs (/workspaces/:id, /providers/:id, /machines/:id) - Guard against empty machine_id in Go PostHog client - Remove unused createHash import from analytics.ts
…t token PostHog project tokens (phc_...) are public write-only keys — safe to commit. Add .gitleaksignore and golangci exclusion to suppress false-positive secret detection for these entries.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
desktop/src/renderer/src/App.svelte (1)
85-88: ⚡ Quick winPreserve static routes before dynamic normalization.
Current matching rewrites
/workspaces/newand/providers/addas:id, which mixes create-flow traffic into detail-page metrics. Handle known static paths first, then apply the dynamic-ID regex.Proposed patch
function normalizeAnalyticsPath(path: string): string { + if (path === "/workspaces/new") return "/workspaces/new"+ if (path === "/providers/add") return "/providers/add" if (/^\/workspaces\/[^/]+$/.test(path)) return "/workspaces/:id" if (/^\/providers\/[^/]+$/.test(path)) return "/providers/:id" if (/^\/machines\/[^/]+$/.test(path)) return "/machines/:id" return path }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/renderer/src/App.svelte` around lines 85 - 88, The normalizeAnalyticsPath function currently normalizes dynamic ID routes before checking static routes, causing static paths like "/workspaces/new" and "/providers/add" to be rewritten as "/workspaces/:id" or "/providers/:id"; update normalizeAnalyticsPath to first check and return known static routes (e.g., "/workspaces/new", "/providers/add", any other create/edit static paths) before applying the ID regexes for "/workspaces/:id", "/providers/:id", and "/machines/:id" so that create-flow and other static endpoints are preserved in analytics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@desktop/src/renderer/src/App.svelte`:
- Around line 85-88: The normalizeAnalyticsPath function currently normalizes
dynamic ID routes before checking static routes, causing static paths like
"/workspaces/new" and "/providers/add" to be rewritten as "/workspaces/:id" or
"/providers/:id"; update normalizeAnalyticsPath to first check and return known
static routes (e.g., "/workspaces/new", "/providers/add", any other create/edit
static paths) before applying the ID regexes for "/workspaces/:id",
"/providers/:id", and "/machines/:id" so that create-flow and other static
endpoints are preserved in analytics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7fce686-96f1-4adf-98af-28364076a64c
📒 Files selected for processing (7)
.github/workflows/release.yml.gitleaksignore.golangci.yamldesktop/src/main/analytics.tsdesktop/src/main/ipc.tsdesktop/src/renderer/src/App.sveltepkg/telemetry/analytics/client.go
✅ Files skipped from review due to trivial changes (2)
- .golangci.yaml
- .gitleaksignore
🚧 Files skipped from review as they are similar to previous changes (3)
- desktop/src/main/ipc.ts
- desktop/src/main/analytics.ts
- pkg/telemetry/analytics/client.go
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Part A — Desktop Update Hosting on Netlify: Configures electron-builder for dual publish (generic on
dl.devsy.sh+ GitHub Releases fallback), adds Netlify site config, and adds a CI step to deploy update metadata (latest*.yml) to Netlify after each release build. The actual installers still download from GitHub Releases — only the metadata check goes through Netlify, separating update check traffic from download counts.Part B — Replace Loft Analytics with PostHog: Replaces the custom HTTP client posting to
analytics.loft.rocks/v1/insertwith the official PostHog Go SDK (posthog-go). Removes the custom double-buffering layer since PostHog handles batching internally. Addsposthog-nodeto the desktop Electron app with event tracking for app lifecycle (open/close), update lifecycle (check/download/install), workspace CRUD, provider actions, page navigation, and error occurrences. All events respect the existingDEVSY_DISABLE_TELEMETRY=trueopt-out. Updates telemetry documentation.Setup required before first use:
phc_PLACEHOLDERAPI key constants inpkg/telemetry/analytics/client.goanddesktop/src/main/analytics.tswith a real PostHog project API keyNETLIFY_AUTH_TOKENandNETLIFY_SITE_IDsecrets to the GitHub repositorydl.devsy.shDNS to point to itSummary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Chores