chore: improve user experience and help determine future development direction#1132
chore: improve user experience and help determine future development direction#1132ananaBMaster merged 3 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 75d6390 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
dosubot
bot
added
the
size:XL
Code Review: PostHog Analytics IntegrationSecurity1. 2. PII filtering is well done ✅ 3. Build config ordering may be fragile Privacy / Legal4. Opt-out by default on Chrome/Edge — needs legal review Code Quality5. 6. Unnecessary defensive check 7. Minor race condition in Performance8. PostHog SDK bundle size — Worth verifying 9. Fire-and-forget pattern is correct ✅ Test Coverage Gaps10.
11. 12. Jotai atom ( 13. 14. No integration test for the full message flow Summary
Overall this is a well-architected analytics integration. The main action items are: (1) add a consent mechanism for GDPR compliance, (2) guard |
|
Documentation Updates 2 document(s) were updated by changes in this PR: Build and Development Environment SetupView Changes@@ -126,29 +126,44 @@
This safeguard helps ensure that API keys are not inadvertently included in production bundles.
-#### Required Google Client ID for Extension Packaging
-When creating a production zip of the browser extension (using `pnpm zip`, `pnpm zip:edge`, `pnpm zip:firefox`, or `pnpm zip:all`), the build system requires the environment variable `WXT_GOOGLE_CLIENT_ID` to be set. If this variable is missing, the build will fail with an error message listing the missing variables.
+#### Required Environment Variables for Extension Packaging
+When creating a production zip of the browser extension (using `pnpm zip`, `pnpm zip:edge`, `pnpm zip:firefox`, or `pnpm zip:all`), the build system requires the following environment variables to be set:
+
+- **`WXT_GOOGLE_CLIENT_ID`**: Google OAuth client ID for extension authentication
+- **`WXT_POSTHOG_API_KEY`**: PostHog API key for anonymous opt-in analytics tracking
+- **`WXT_POSTHOG_HOST`**: PostHog host URL for analytics tracking
+
+If any of these variables are missing, the build will fail with an error message listing the missing variables.
+
+**Purpose of Analytics Variables:**
+The PostHog environment variables enable anonymous, opt-in analytics tracking to help improve user experience and guide future development direction. Analytics is enabled by default on Chromium-based browsers and disabled by default on Firefox. Users can opt out anytime in Settings → Config → About.
- **Migration:**
- - Set `WXT_GOOGLE_CLIENT_ID` in your `.env.production` file or export it in your environment before running any zip command (`pnpm zip`, `pnpm zip:edge`, `pnpm zip:firefox`, or `pnpm zip:all`).
+ - Set all required variables in your `.env.production` file or export them in your environment before running any zip command (`pnpm zip`, `pnpm zip:edge`, `pnpm zip:firefox`, or `pnpm zip:all`).
- All zip builds are affected by this check; development builds (`pnpm dev`, etc.) are unchanged.
**Example:**
```sh
# .env.production
WXT_GOOGLE_CLIENT_ID=your-google-client-id
-```
-
-Or, set it in your shell before running the zip command:
+WXT_POSTHOG_API_KEY=your-posthog-api-key
+WXT_POSTHOG_HOST=https://your-posthog-host.com
+```
+
+Or, set them in your shell before running the zip command:
```sh
export WXT_GOOGLE_CLIENT_ID=your-google-client-id
+export WXT_POSTHOG_API_KEY=your-posthog-api-key
+export WXT_POSTHOG_HOST=https://your-posthog-host.com
pnpm zip:all
```
-If the variable is missing, you will see an error like:
+If any variables are missing, you will see an error like:
```
Missing required environment variables for zip:
- WXT_GOOGLE_CLIENT_ID
+ - WXT_POSTHOG_API_KEY
+ - WXT_POSTHOG_HOST
Set them in .env.production or your environment.
```
@@ -417,4 +432,4 @@
---
-**Note:** When packaging the browser extension for production (zip builds), you must set `WXT_GOOGLE_CLIENT_ID` in your environment or `.env.production`. This is a required step for successful production packaging.
+**Note:** When packaging the browser extension for production (zip builds), you must set `WXT_GOOGLE_CLIENT_ID`, `WXT_POSTHOG_API_KEY`, and `WXT_POSTHOG_HOST` in your environment or `.env.production`. These variables are required for successful production packaging.How to contribute Read FrogView Changes@@ -41,6 +41,13 @@
```bash
cp apps/website/.env.example apps/website/.env.local
```
+
+ **Extension Build Environment Variables**
+ When building zip releases of the extension (`WXT_ZIP_MODE=true`), additional environment variables are required:
+ - `WXT_POSTHOG_API_KEY`: PostHog API key for analytics tracking
+ - `WXT_POSTHOG_HOST`: PostHog host URL for analytics tracking
+
+ These variables enable anonymous analytics tracking (opt-in by default on Chromium-based browsers, opt-out on Firefox) to help improve user experience. The build will fail validation if these are missing during a zip build.
4. **Start Development Server**
```bashHow did I do? Any feedback? |
Code Review: PR #1132 — Analytics/Telemetry IntegrationOverall Assessment: 🟡 Acceptable — merge with required fixesThe analytics implementation is well-architected with good privacy defaults and clean separation of concerns. However, there are 2 critical test gaps and several minor issues to address before merging. 🟢 Strengths
🔴 Critical Issues1.
|
| Module | Tests | Status |
|---|---|---|
background/analytics.ts |
8 | 🟢 Good |
utils/analytics.ts |
5 | 🟡 Missing trackFeatureAttempt() |
utils/atoms/analytics.ts |
0 | 🔴 None |
constants/analytics.ts |
2 | 🟢 Good |
🔒 Privacy/Security Checklist
- No PII in event properties (only feature, surface, outcome, latency_ms)
- Anonymous distinct ID (UUID)
- User opt-out respected via
analyticsEnabledAtom - Property allowlist prevents accidental data leakage
- No URLs, page content, or translated text collected
- No session recording, no autocapture
- Disabled by default on Firefox
- Does NOT respect browser DNT header (
respect_dntconfig) — acceptable for opt-in system but worth noting
📋 Recommendations
Must fix before merge:
- Add tests for
trackFeatureAttempt()(success, failure, tracking-failure paths) - Add tests for
analyticsEnabledAtom(init, persistence, visibility sync, cleanup)
Should fix:
3. Replace console.error with logger.error in src/utils/atoms/analytics.ts
4. Refactor property allowlist to data-driven approach
5. Remove or document the defensive typeof logger.warn check
Nice to have:
6. Validate env vars are non-empty strings, not just truthy
7. Add JSDoc comments explaining fire-and-forget analytics design choice
📝 Additional Notes
- The PR title says "chore: improve user experience" which is vague — consider a more descriptive title like
feat: add opt-in anonymous feature usage analytics - The PR description template is unfilled (no type selected, no description, no related issue). Please fill it out for reviewer context.
- The
posthog-jsdependency adds bundle size — worth noting the impact in the PR description.
🤖 Generated with Claude Code
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
16 issues found across 56 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/entrypoints/selection.content/selection-toolbar/translate-button/index.tsx">
<violation number="1" location="src/entrypoints/selection.content/selection-toolbar/translate-button/index.tsx:184">
P2: Guard the success analytics call with `runIdRef.current === runId` so canceled or superseded translations are not counted as successful uses.</violation>
<violation number="2" location="src/entrypoints/selection.content/selection-toolbar/translate-button/index.tsx:256">
P2: Guard failure analytics with the current `runId` check so errors from superseded translation runs are not reported as real failures.</violation>
</file>
<file name="src/entrypoints/options/pages/custom-actions/action-config-form/output-schema-field.tsx">
<violation number="1" location="src/entrypoints/options/pages/custom-actions/action-config-form/output-schema-field.tsx:402">
P2: Render `meta.errors` like the other form fields; raw `.join(", ")` will turn schema error objects into `[object Object]` and hide output-schema validation messages.</violation>
</file>
<file name="src/utils/atoms/analytics.ts">
<violation number="1" location="src/utils/atoms/analytics.ts:32">
P1: Reading this boolean preference through `storageAdapter.get` breaks persisted `false` values, so an analytics opt-out resets to the default on remount or visibility refresh.</violation>
</file>
<file name=".changeset/add-anonymous-analytics.md">
<violation number="1" location=".changeset/add-anonymous-analytics.md:2">
P2: Use a minor changeset here; this file describes a new feature, so `patch` under-versions the release.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:104">
P2: Update `pnpm-lock.yaml` alongside these dependency changes so installs stay reproducible.</violation>
</file>
<file name="src/entrypoints/background/analytics.ts">
<violation number="1" location="src/entrypoints/background/analytics.ts:126">
P2: Reset the cached PostHog client promise when initialization fails so later events can retry.</violation>
</file>
<file name="src/entrypoints/subtitles.content/ui/subtitles-translate-button.tsx">
<violation number="1" location="src/entrypoints/subtitles.content/ui/subtitles-translate-button.tsx:23">
P2: Skip the auto-start callback when subtitles are already visible, otherwise remounts can emit duplicate `video_subtitles_auto` usage events.</violation>
</file>
<file name="src/locales/ru.yml">
<violation number="1" location="src/locales/ru.yml:581">
P3: Fix the typo in the new Russian mission text: `знанияемких` should be `знаниеёмких`.</violation>
</file>
<file name="src/entrypoints/options/pages/custom-actions/action-config-form/provider-field.tsx">
<violation number="1" location="src/entrypoints/options/pages/custom-actions/action-config-form/provider-field.tsx:54">
P2: Joining `meta.errors` directly can render validation objects as `[object Object]` instead of their message text.</violation>
</file>
<file name="src/entrypoints/translation-hub/components/translation-card.tsx">
<violation number="1" location="src/entrypoints/translation-hub/components/translation-card.tsx:44">
P2: This wraps the stale-response discard path in `trackFeatureAttempt`, so superseded translations are now reported as successful Translation Hub uses even though the UI ignores them.</violation>
</file>
<file name="src/entrypoints/options/pages/custom-actions/action-config-form/icon-field.tsx">
<violation number="1" location="src/entrypoints/options/pages/custom-actions/action-config-form/icon-field.tsx:54">
P2: Keep mapping schema issues to `.message`; joining the raw errors array can render `[object Object]` instead of the validation text.</violation>
</file>
<file name="src/entrypoints/subtitles.content/universal-adapter.ts">
<violation number="1" location="src/entrypoints/subtitles.content/universal-adapter.ts:247">
P2: Don't record a successful feature-use event until subtitle translation has actually succeeded; this currently counts setup-starts as successes even when translation later fails.</violation>
</file>
<file name="src/entrypoints/host.content/translation-control/page-translation.ts">
<violation number="1" location="src/entrypoints/host.content/translation-control/page-translation.ts:162">
P1: Roll back the active state in this catch block. `start()` marks translation enabled before the remaining async setup finishes, so a later failure leaves the manager stuck active and blocks retries.</violation>
</file>
<file name="wxt.config.ts">
<violation number="1" location="wxt.config.ts:93">
P1: `WXT_POSTHOG_API_KEY` is now both forbidden and required, so production zip builds will always fail.</violation>
</file>
<file name="src/utils/atoms/theme.ts">
<violation number="1" location="src/utils/atoms/theme.ts:34">
P2: Guard the visibility-triggered storage read so it cannot overwrite a newer watcher update.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) | ||
|
|
||
| baseAnalyticsEnabledAtom.onMount = (setAtom) => { | ||
| void storageAdapter.get( |
There was a problem hiding this comment.
P1: Reading this boolean preference through storageAdapter.get breaks persisted false values, so an analytics opt-out resets to the default on remount or visibility refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/atoms/analytics.ts, line 32:
<comment>Reading this boolean preference through `storageAdapter.get` breaks persisted `false` values, so an analytics opt-out resets to the default on remount or visibility refresh.</comment>
<file context>
@@ -0,0 +1,59 @@
+)
+
+baseAnalyticsEnabledAtom.onMount = (setAtom) => {
+ void storageAdapter.get(
+ ANALYTICS_ENABLED_STORAGE_KEY,
+ DEFAULT_ANALYTICS_ENABLED,
</file context>
| catch (error) { | ||
| if (trackedContext) { | ||
| void trackFeatureUsed({ | ||
| ...trackedContext, | ||
| outcome: "failure", | ||
| }) | ||
| } | ||
| throw error | ||
| } |
There was a problem hiding this comment.
P1: Roll back the active state in this catch block. start() marks translation enabled before the remaining async setup finishes, so a later failure leaves the manager stuck active and blocks retries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/host.content/translation-control/page-translation.ts, line 162:
<comment>Roll back the active state in this catch block. `start()` marks translation enabled before the remaining async setup finishes, so a later failure leaves the manager stuck active and blocks retries.</comment>
<file context>
@@ -95,44 +106,68 @@ export class PageTranslationManager implements IPageTranslationManager {
+ })
+ }
+ }
+ catch (error) {
+ if (trackedContext) {
+ void trackFeatureUsed({
</file context>
| catch (error) { | |
| if (trackedContext) { | |
| void trackFeatureUsed({ | |
| ...trackedContext, | |
| outcome: "failure", | |
| }) | |
| } | |
| throw error | |
| } | |
| catch (error) { | |
| void sendMessage("setAndNotifyPageTranslationStateChangedByManager", { | |
| enabled: false, | |
| }) | |
| this.isPageTranslating = false | |
| this.walkId = null | |
| this.dontWalkIntoElementsCache = new WeakSet() | |
| this.stopDocumentTitleTracking() | |
| if (this.intersectionObserver) { | |
| this.intersectionObserver.disconnect() | |
| this.intersectionObserver = null | |
| } | |
| this.mutationObservers.forEach(observer => observer.disconnect()) | |
| this.mutationObservers = [] | |
| void removeAllTranslatedWrapperNodes() | |
| if (trackedContext) { | |
| void trackFeatureUsed({ | |
| ...trackedContext, | |
| outcome: "failure", | |
| }) | |
| } | |
| throw error | |
| } |
| // Check required env vars only for zip builds | ||
| if (process.env.WXT_ZIP_MODE) { | ||
| const requiredEnvVars = ["WXT_GOOGLE_CLIENT_ID"] | ||
| const requiredEnvVars = [ |
There was a problem hiding this comment.
P1: WXT_POSTHOG_API_KEY is now both forbidden and required, so production zip builds will always fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wxt.config.ts, line 93:
<comment>`WXT_POSTHOG_API_KEY` is now both forbidden and required, so production zip builds will always fail.</comment>
<file context>
@@ -90,7 +90,11 @@ export default defineConfig({
// Check required env vars only for zip builds
if (process.env.WXT_ZIP_MODE) {
- const requiredEnvVars = ["WXT_GOOGLE_CLIENT_ID"]
+ const requiredEnvVars = [
+ "WXT_GOOGLE_CLIENT_ID",
+ "WXT_POSTHOG_API_KEY",
</file context>
| if (!isAbortError(error)) { | ||
| void trackFeatureUsed({ | ||
| ...analyticsContext, | ||
| outcome: "failure", | ||
| }) |
There was a problem hiding this comment.
P2: Guard failure analytics with the current runId check so errors from superseded translation runs are not reported as real failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/selection.content/selection-toolbar/translate-button/index.tsx, line 256:
<comment>Guard failure analytics with the current `runId` check so errors from superseded translation runs are not reported as real failures.</comment>
<file context>
@@ -225,13 +240,25 @@ export function TranslateButton() {
setError(createSelectionToolbarRuntimeError("translate", error))
}
+
+ if (!isAbortError(error)) {
+ void trackFeatureUsed({
+ ...analyticsContext,
</file context>
| if (!isAbortError(error)) { | |
| void trackFeatureUsed({ | |
| ...analyticsContext, | |
| outcome: "failure", | |
| }) | |
| if (!isAbortError(error) && runIdRef.current === runId) { | |
| void trackFeatureUsed({ | |
| ...analyticsContext, | |
| outcome: "failure", | |
| }) | |
| } |
| setIsTranslating(false) | ||
| setError(createSelectionToolbarPrecheckError("translate", "providerUnavailable")) | ||
| } | ||
| void trackFeatureUsed({ |
There was a problem hiding this comment.
P2: Guard the success analytics call with runIdRef.current === runId so canceled or superseded translations are not counted as successful uses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/selection.content/selection-toolbar/translate-button/index.tsx, line 184:
<comment>Guard the success analytics call with `runIdRef.current === runId` so canceled or superseded translations are not counted as successful uses.</comment>
<file context>
@@ -174,6 +181,10 @@ export function TranslateButton() {
setIsTranslating(false)
setError(createSelectionToolbarPrecheckError("translate", "providerUnavailable"))
}
+ void trackFeatureUsed({
+ ...analyticsContext,
+ outcome: "failure",
</file context>
| } | ||
|
|
||
| return result | ||
| return await trackFeatureAttempt( |
There was a problem hiding this comment.
P2: This wraps the stale-response discard path in trackFeatureAttempt, so superseded translations are now reported as successful Translation Hub uses even though the UI ignores them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/translation-hub/components/translation-card.tsx, line 44:
<comment>This wraps the stale-response discard path in `trackFeatureAttempt`, so superseded translations are now reported as successful Translation Hub uses even though the UI ignores them.</comment>
<file context>
@@ -39,22 +41,30 @@ export function TranslationCard({ providerId, isExpanded, onExpandedChange }: Tr
- }
-
- return result
+ return await trackFeatureAttempt(
+ createFeatureUsageContext(
+ ANALYTICS_FEATURE.TRANSLATION_HUB,
</file context>
| {field.state.meta.errors.length > 0 && ( | ||
| <span className="text-sm font-normal text-destructive"> | ||
| {field.state.meta.errors.map(error => typeof error === "string" ? error : error?.message).join(", ")} | ||
| {field.state.meta.errors.join(", ")} |
There was a problem hiding this comment.
P2: Keep mapping schema issues to .message; joining the raw errors array can render [object Object] instead of the validation text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/options/pages/custom-actions/action-config-form/icon-field.tsx, line 54:
<comment>Keep mapping schema issues to `.message`; joining the raw errors array can render `[object Object]` instead of the validation text.</comment>
<file context>
@@ -51,7 +51,7 @@ export const IconField = withForm({
{field.state.meta.errors.length > 0 && (
<span className="text-sm font-normal text-destructive">
- {field.state.meta.errors.map(error => typeof error === "string" ? error : error?.message).join(", ")}
+ {field.state.meta.errors.join(", ")}
</span>
)}
</file context>
| {field.state.meta.errors.join(", ")} | |
| {field.state.meta.errors.map(error => typeof error === "string" ? error : error?.message).join(", ")} |
| return | ||
| } | ||
|
|
||
| await this.processSubtitles() |
There was a problem hiding this comment.
P2: Don't record a successful feature-use event until subtitle translation has actually succeeded; this currently counts setup-starts as successes even when translation later fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/entrypoints/subtitles.content/universal-adapter.ts, line 247:
<comment>Don't record a successful feature-use event until subtitle translation has actually succeeded; this currently counts setup-starts as successes even when translation later fails.</comment>
<file context>
@@ -189,45 +191,75 @@ export class UniversalVideoAdapter {
+ return
+ }
+
+ await this.processSubtitles()
+ if (analyticsContext) {
+ void trackFeatureUsed({
</file context>
| const handleVisibilityChange = () => { | ||
| if (document.visibilityState === "visible") { | ||
| logger.info("baseThemeModeAtom onMount handleVisibilityChange when: ", new Date()) | ||
| void storageAdapter.get<ThemeMode>(THEME_STORAGE_KEY, DEFAULT_THEME_MODE, themeModeSchema).then(setAtom) |
There was a problem hiding this comment.
P2: Guard the visibility-triggered storage read so it cannot overwrite a newer watcher update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/atoms/theme.ts, line 34:
<comment>Guard the visibility-triggered storage read so it cannot overwrite a newer watcher update.</comment>
<file context>
@@ -27,5 +28,16 @@ baseThemeModeAtom.onMount = (setAtom: (newValue: ThemeMode) => void) => {
+ const handleVisibilityChange = () => {
+ if (document.visibilityState === "visible") {
+ logger.info("baseThemeModeAtom onMount handleVisibilityChange when: ", new Date())
+ void storageAdapter.get<ThemeMode>(THEME_STORAGE_KEY, DEFAULT_THEME_MODE, themeModeSchema).then(setAtom)
+ }
+ }
</file context>
| about: | ||
| title: О приложении | ||
| description: Read Frog появился в апреле 2025 года | ||
| mission: Read Frog стремится расширить равный доступ к качественному образованию в эпоху ИИ, делая обучение, максимально похожее на занятия с настоящим преподавателем, доступным по цене в языках, медицине и других знанияемких областях. |
There was a problem hiding this comment.
P3: Fix the typo in the new Russian mission text: знанияемких should be знаниеёмких.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/locales/ru.yml, line 581:
<comment>Fix the typo in the new Russian mission text: `знанияемких` should be `знаниеёмких`.</comment>
<file context>
@@ -575,6 +575,13 @@ options:
+ about:
+ title: О приложении
+ description: Read Frog появился в апреле 2025 года
+ mission: Read Frog стремится расширить равный доступ к качественному образованию в эпоху ИИ, делая обучение, максимально похожее на занятия с настоящим преподавателем, доступным по цене в языках, медицине и других знанияемких областях.
+ analytics:
+ title: Помочь улучшить пользовательский опыт
</file context>
| mission: Read Frog стремится расширить равный доступ к качественному образованию в эпоху ИИ, делая обучение, максимально похожее на занятия с настоящим преподавателем, доступным по цене в языках, медицине и других знанияемких областях. | |
| mission: Read Frog стремится расширить равный доступ к качественному образованию в эпоху ИИ, делая обучение, максимально похожее на занятия с настоящим преподавателем, доступным по цене в языках, медицине и других знаниеёмких областях. |
1 participant
Type of Changes
Description
Related Issue
Closes #
How Has This Been Tested?
Screenshots
Checklist
Additional Information
Summary by cubic
Add anonymous, opt‑out feature usage tracking powered by
posthog-jsto understand which features work well and where to improve. Adds a simple toggle in Options → Config → About and instruments key translation and TTS flows with minimal latency/outcome data.New Features
analyticsEnabledand anonymous install ID; registersextension_version.feature_usedacross: page translation (popup, floating button, context menu, shortcut, auto, touch), selection translation, input translation, subtitles (manual/auto), text‑to‑speech (incl. TTS settings preview), and Translation Hub.createFeatureUsageContext,trackFeatureUsed/trackFeatureAttempt) capture latency and outcome only.Migration
WXT_POSTHOG_API_KEYandWXT_POSTHOG_HOST(validated in build).analyticsContextwhen toggling page translation.posthog-js; bumpswxtto0.20.19.Written for commit 0ddc69d. Summary will update on new commits.