feat(analytics): implement Google Analytics#1533
Open
graycyrus wants to merge 5 commits into
Open
Conversation
…tracking Add GA4 via react-ga4 with triple-gate (IS_DEV, GA_MEASUREMENT_ID, analyticsEnabled consent). Track route changes via AppShell useLocation effect, plus events for app_open, onboarding lifecycle, account connect, chat send, and skill install/uninstall. All events validated against an allowlist — no PII, message content, or credentials leave the app. Update privacy disclosure text and capability catalog to reflect the new analytics scope. Closes tinyhumansai#1479
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GA4 integration (init, pageview, event APIs) guarded by env/dev/consent; wires init into app startup; emits page-view and allowlisted feature events across onboarding, accounts, conversations, and skill flows; updates config/deps, tests GA behavior, and updates privacy/docs. ChangesGoogle Analytics 4 Integration with Privacy Gating
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🧹 Nitpick comments (2)
app/src/services/analytics.ts (1)
198-210: 💤 Low valueConsider removing the redundant
ReactGA.setcall insyncAnalyticsConsent.Lines 206-208 call
ReactGA.set({ allow_ad_personalization_signals: false })every time consent is toggled, but this value is already set tofalseininitGA()(line 244) and should never change. Since the actual consent enforcement happens via thegaEnabledflag that gatestrackPageViewandtrackEvent, this re-set is redundant.♻️ Proposed simplification
export function syncAnalyticsConsent(enabled: boolean): void { const client = Sentry.getClient(); if (client && !enabled) { void Sentry.flush(2000); } // Update the GA consent shadow and toggle ad-personalization signals. gaEnabled = enabled; if (gaInitialized) { - ReactGA.set({ allow_ad_personalization_signals: false }); console.debug(`[analytics] GA consent updated: enabled=${enabled}`); } }🤖 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 `@app/src/services/analytics.ts` around lines 198 - 210, The syncAnalyticsConsent function contains a redundant call to ReactGA.set({ allow_ad_personalization_signals: false }) which is already established in initGA and never changes; remove that ReactGA.set call from syncAnalyticsConsent, keep updating the gaEnabled shadow and the console.debug line (adjust the debug message if desired) and rely on initGA to set allow_ad_personalization_signals once; references: function syncAnalyticsConsent, initGA, gaEnabled, gaInitialized, and ReactGA.set.app/src/services/__tests__/analytics.test.ts (1)
409-417: ⚡ Quick winEnsure
console.warnspy is always restoredLine 410 creates a global spy, but restore on Line 416 won’t run if an assertion throws first. Wrap the body in
try/finallyto prevent cross-test leakage.Suggested patch
test('drops events not in the allowlist and logs a warning', async () => { const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); - const { initGA, trackEvent } = await freshAnalytics(); - initGA(); - trackEvent('internal_debug_event'); - expect(hoisted.gaEvent).not.toHaveBeenCalled(); - expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('internal_debug_event')); - warnSpy.mockRestore(); + try { + const { initGA, trackEvent } = await freshAnalytics(); + initGA(); + trackEvent('internal_debug_event'); + expect(hoisted.gaEvent).not.toHaveBeenCalled(); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('internal_debug_event')); + } finally { + warnSpy.mockRestore(); + } });🤖 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 `@app/src/services/__tests__/analytics.test.ts` around lines 409 - 417, The test creates a global console.warn spy (warnSpy) but only restores it after assertions, risking leaks if an assertion throws; wrap the body of the test that calls freshAnalytics(), initGA(), trackEvent('internal_debug_event'), and the assertions in a try/finally so warnSpy.mockRestore() is always executed in the finally block to guarantee restoration even on failure.
🤖 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 `@app/src/main.tsx`:
- Around line 54-57: The trackEvent('app_open', { version: APP_VERSION }) call
is firing for overlay/mascot windows as well; wrap it so it only runs for the
main/non-standalone window. Add or call a single predicate (e.g. isMainWindow(),
isMainAppWindow(), or check window.type/window.name/IPC-provided flag) before
invoking trackEvent in main.tsx, leaving initSentry() and initGA()
unchanged—only call trackEvent when that predicate returns true so
overlays/mascots do not emit app_open.
In `@app/src/pages/onboarding/pages/ContextPage.tsx`:
- Around line 16-19: The onNext handler currently calls completeAndExit()
without handling rejection; update the onNext callback that wraps trackEvent and
completeAndExit to await or chain a .catch on completeAndExit (the function name
completeAndExit in this component) and handle errors explicitly—e.g., log the
error via the existing logger or trackEvent an error event and surface user
feedback (toast/modal) as appropriate—so any failure in completeAndExit is not
an unhandled rejection and the failure path is observable.
In `@app/src/services/webviewAccountService.ts`:
- Around line 312-329: The code currently fires
trackEvent('account_connect_success', connectSuccessParams) unconditionally,
which counts warm re-opens (payload.state === 'reused') as new connects; update
the logic in the webview reveal path and the else branch so that trackEvent is
only called when payload.state !== 'reused' (i.e., a real new connect).
Specifically, inside the invoke(...).finally() and the fallback else, check
payload.state and only call trackEvent('account_connect_success',
connectSuccessParams) when payload.state !== 'reused' while still always
dispatching setAccountStatus({ accountId, status: 'open' }) as before.
---
Nitpick comments:
In `@app/src/services/__tests__/analytics.test.ts`:
- Around line 409-417: The test creates a global console.warn spy (warnSpy) but
only restores it after assertions, risking leaks if an assertion throws; wrap
the body of the test that calls freshAnalytics(), initGA(),
trackEvent('internal_debug_event'), and the assertions in a try/finally so
warnSpy.mockRestore() is always executed in the finally block to guarantee
restoration even on failure.
In `@app/src/services/analytics.ts`:
- Around line 198-210: The syncAnalyticsConsent function contains a redundant
call to ReactGA.set({ allow_ad_personalization_signals: false }) which is
already established in initGA and never changes; remove that ReactGA.set call
from syncAnalyticsConsent, keep updating the gaEnabled shadow and the
console.debug line (adjust the debug message if desired) and rely on initGA to
set allow_ad_personalization_signals once; references: function
syncAnalyticsConsent, initGA, gaEnabled, gaInitialized, and ReactGA.set.
🪄 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: b180a648-aa89-4a26-89df-5fd57aca37b7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.claude/memory.md.gitignoreapp/.env.exampleapp/package.jsonapp/src/App.tsxapp/src/components/settings/panels/PrivacyPanel.tsxapp/src/components/skills/InstallSkillDialog.tsxapp/src/components/skills/UninstallSkillConfirmDialog.tsxapp/src/features/privacy/whatLeavesItems.tsapp/src/main.tsxapp/src/pages/Accounts.tsxapp/src/pages/Conversations.tsxapp/src/pages/onboarding/OnboardingLayout.tsxapp/src/pages/onboarding/pages/ContextPage.tsxapp/src/pages/onboarding/pages/SkillsPage.tsxapp/src/pages/onboarding/pages/WelcomePage.tsxapp/src/services/__tests__/analytics.test.tsapp/src/services/analytics.tsapp/src/services/webviewAccountService.tsapp/src/utils/config.tssrc/openhuman/about_app/catalog.rs
…ection, skip reused accounts - Gate app_open event to main window only (skip overlay/mascot windows) - Add .catch() to completeAndExit() in ContextPage to prevent unhandled rejection - Skip account_connect_success for reused/warm-reopened accounts
graycyrus
commented
May 12, 2026
Contributor
Author
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — feat(analytics): implement Google Analytics
Clean, well-structured implementation. Privacy model is solid — triple-gated init, allowlisted events, no PII, ad personalization off. Tests are thorough. CodeRabbit feedback addressed. See inline comments below.
Recommendation: Approve (pending CI green on coverage).
…sent allow_ad_personalization_signals is already set unconditionally in initGA() — no need to re-set it on every consent toggle.
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
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
react-ga4with triple-gate:IS_DEV,GA_MEASUREMENT_IDenv var, andanalyticsEnableduser consent toggleAppShelluseLocation()effectapp_open, onboarding lifecycle (onboarding_start,onboarding_step_complete,onboarding_complete),account_connect_start/success,chat_message_sent,skill_install/skill_uninstallabout_app/catalog.rsConfiguration
Set
VITE_GA_MEASUREMENT_ID=G-XXXXXXXXXXinapp/.env.localto enable. Leave blank or omit to disable. GA is always disabled in dev mode (IS_DEV=true).Privacy
Test plan
pnpm typecheck— cleanpnpm lint— 0 errors (39 pre-existing warnings)pnpm format:check— cleanpnpm build— successpnpm test:unit— 2075 passed, 0 failedcargo check(core + Tauri shell) — cleanCloses #1479
Summary by CodeRabbit
New Features
Documentation
Tests
Chores