Add privacy-bounded product analytics across ADE surfaces#808
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughChangesAnalytics contracts and desktop service
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@copilot review but do not make fixes |
|
@codex review |
a840079 to
d47a58c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d47a58cb23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (key === "action") { | ||
| if (typeof value !== "string" || value.length > 256) return null; | ||
| const raw = value.trim(); | ||
| return raw === "open" || isMeaningfulUsageAction(raw) ? raw.toLowerCase() : null; |
There was a problem hiding this comment.
Preserve canonical action values
When the usage exporter sends any of the camelCase actions that already passed isMeaningfulUsageAction (for example work.startCliSession, chat.respondToInput, or lanes.createFromUnstaged), this lowercases the value before it reaches PostHog. Those values no longer match the canonical action taxonomy used by the local ledger and docs, so action breakdowns or filters will split/miss those events; preserve the validated raw action value or define a separate lowercase taxonomy consistently.
Useful? React with 👍 / 👎.
| const uuid = input.clientEventId?.length === 36 && UUID_VALUE.test(input.clientEventId) | ||
| ? input.clientEventId.toLowerCase() | ||
| : randomUUID(); |
There was a problem hiding this comment.
Regenerate non-random client event IDs
For IPC/sync/TUI/web captures that provide clientEventId, this forwards any UUID version 1-8 verbatim as PostHog's uuid. Version 1 UUIDs can contain a node/MAC identifier and time-ordered UUIDs expose timestamps, so a client-supplied insert id can bypass the content-free analytics boundary; accept only random v4 IDs generated by ADE or regenerate/hash anything else before sending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/ade-cli/src/tuiClient/productAnalytics.ts (1)
17-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding unit tests for
deriveTuiAnalyticsScreen.This pure function has several priority-ordered branches that silently determine analytics screen attribution; a small table-driven test would guard against regressions when new panes/states are added.
🤖 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 `@apps/ade-cli/src/tuiClient/productAnalytics.ts` around lines 17 - 28, Add a table-driven unit test for deriveTuiAnalyticsScreen covering each priority-ordered branch, including terminal control, drawer, details with empty and non-empty right panes, add mode, grid view, and the default chat screen. Assert the expected screen identifier for each input and include overlapping-state cases to verify the existing precedence order.apps/ade-cli/src/adeRpcServer.ts (1)
3341-3350: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDuplicated
isUserClientgate; also verify it classifies TUI/CLI sessions correctly.The 4-line
isUserClientcheck is duplicated verbatim between the listing filter and the invocation gate. Extracting a shared helper avoids the two enforcement points drifting apart if identity-shape semantics change later. Separately, please confirm that interactive TUI sessions (which callanalytics.captureviaapps/ade-cli/src/tuiClient/productAnalytics.ts) are actually classified asisUserClient(i.e. never carrychatSessionId/runId/stepId/attemptId) across both the headless CLI and the desktop socket-backed ADE RPC path — otherwise legitimate product-analytics capture could be silently rejected.♻️ Proposed helper extraction
+function isUserClientSession(session: SessionState): boolean { + return !session.identity.runId + && !session.identity.stepId + && !session.identity.attemptId + && !session.identity.chatSessionId; +} + // list_ade_actions - const isUserClient = !session.identity.runId - && !session.identity.stepId - && !session.identity.attemptId - && !session.identity.chatSessionId; + const isUserClient = isUserClientSession(session); // run_ade_action - const isUserClient = !session.identity.runId - && !session.identity.stepId - && !session.identity.attemptId - && !session.identity.chatSessionId; + const isUserClient = isUserClientSession(session);Also applies to: 3398-3401
🤖 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 `@apps/ade-cli/src/adeRpcServer.ts` around lines 3341 - 3350, Extract the repeated identity-shape check into a shared helper near the affected listing and invocation logic, and use it for both analytics action filtering and the invocation gate. Verify the helper classifies headless CLI, interactive TUI, and desktop socket-backed sessions as user clients when identity.runId, stepId, attemptId, and chatSessionId are all absent; preserve rejection for sessions carrying any of those fields.Source: Path instructions
apps/web/vite.config.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate PostHog token-validation regex across two build configs.
apps/web/vite.config.tsandapps/desktop/tsup.config.tsboth hardcode the identical/^phc_[A-Za-z0-9_-]{8,}$/regex and error message; extracting a small shared helper would prevent the two copies from drifting if the token format or messaging changes.
apps/web/vite.config.ts#L1-15: import a sharedassertPublicPostHogToken(token)helper (e.g. from a small shared Node util) instead of inlining the regex/throw here.apps/desktop/tsup.config.ts#L3-6: use the same shared helper instead of its own copy of the regex/throw.🤖 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 `@apps/web/vite.config.ts` at line 1, Extract the duplicated PostHog token validation from the web Vite configuration and desktop tsup configuration into a shared Node utility exposing assertPublicPostHogToken(token). Update both configuration entry points to import and call this helper, removing their local regex and error-message implementations while preserving the existing validation behavior.apps/web/src/app/pages/TermsPage.tsx (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
ANALYTICS_FEATURE_ATTRIBUTEconstant instead of hardcoding the attribute name.
ShipShowcase.tsx(same PR) applies this attribute via the exportedANALYTICS_FEATURE_ATTRIBUTEconstant; this file hardcodes the literaldata-ade-analytics-featurestring instead. If the attribute name ever changes inmarketingAnalytics.ts, this file's links will silently stop being tracked while other call sites keep working.♻️ Proposed fix
-import { MARKETING_FEATURES } from "../../lib/marketingAnalytics"; +import { ANALYTICS_FEATURE_ATTRIBUTE, MARKETING_FEATURES } from "../../lib/marketingAnalytics";- <a className="focus-ring rounded-md text-muted-fg hover:text-fg" href={LINKS.github} data-ade-analytics-feature={MARKETING_FEATURES.VIEW_GITHUB} target="_blank" rel="noreferrer"> + <a className="focus-ring rounded-md text-muted-fg hover:text-fg" href={LINKS.github} {...{ [ANALYTICS_FEATURE_ATTRIBUTE]: MARKETING_FEATURES.VIEW_GITHUB }} target="_blank" rel="noreferrer"> GitHub </a> - <a className="focus-ring rounded-md text-muted-fg hover:text-fg" href={LINKS.docs} data-ade-analytics-feature={MARKETING_FEATURES.VIEW_DOCS} target="_blank" rel="noreferrer"> + <a className="focus-ring rounded-md text-muted-fg hover:text-fg" href={LINKS.docs} {...{ [ANALYTICS_FEATURE_ATTRIBUTE]: MARKETING_FEATURES.VIEW_DOCS }} target="_blank" rel="noreferrer"> Docs </a>Also applies to: 49-49, 52-52
🤖 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 `@apps/web/src/app/pages/TermsPage.tsx` at line 5, Update the TermsPage link attributes to use the shared ANALYTICS_FEATURE_ATTRIBUTE constant imported from marketingAnalytics instead of the hardcoded data-ade-analytics-feature string. Apply this consistently to all affected links while preserving their existing analytics values.
🤖 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 `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 431-432: Preserve peer analytics consent across project-host
handoffs: update the handoff snapshot construction in dispose() to include
productAnalyticsEnabled, and restore it in adoptHandedOffPeers() alongside
rosterSubscribed using the existing fail-closed boolean behavior. Keep
registerPeer’s default for genuinely new connections unchanged.
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 4742-4780: Register an `analytics.flush` handler alongside the
existing analytics handlers in `syncRemoteCommandService` that delegates to
`productAnalyticsService.flush()` and safely handles the service being
unconfigured, so the shutdown action invoked by `app.tsx` succeeds and drains
queued analytics. The `app.tsx` call site requires no direct change.
In `@apps/desktop/src/main/services/analytics/dailyUsageAnalytics.ts`:
- Around line 105-127: Update the overall capture in captureDailyUsageAnalytics
to derive provider_count and model_count from the same fallback arrays used
elsewhere: prefer stats.adeProviders and stats.adeModels, falling back to
stats.providers and stats.models. Preserve the existing count behavior while
ensuring these totals match the arrays used for topProvider and topModel
aggregation.
In `@apps/desktop/src/main/services/analytics/productAnalyticsService.ts`:
- Around line 748-783: Update the synchronous failure handling in the
posthog.capture flow to roll back the quota reservation before recording the
transport_error drop. Decrement state.quota.accepted and
state.quota.acceptedByEvent[input.event], remove the corresponding recent
timestamp from state.quota.minuteWindows[input.event], and clear the dedupe
entry when applicable, then persist the corrected state and return the existing
rejection result.
- Around line 302-333: Replace the blocking Atomics.wait retry in
acquireStateLock with a non-blocking retry/backoff mechanism, such as scheduling
subsequent lock attempts asynchronously, while preserving stale-lock cleanup,
the waitMs deadline, and null-on-timeout behavior. Update the callers capture,
getStatus, and setEnabled as needed so lock acquisition never blocks the
Electron main process.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 2058-2075: Wrap the error-path productAnalyticsService?.capture
call in the global IPC error interceptor with its own try/catch, matching the
protected success-path capture nearby. Ensure any synchronous analytics
exception is contained so the interceptor still rethrows the original error
unchanged.
In `@apps/desktop/src/renderer/webclient/adapter/analytics.ts`:
- Around line 88-112: The failed consent path in syncClientConsent must not
immediately tear down the shared infra.client connection. Replace the direct
disconnect in the catch block with retry handling for the
analytics.setClientEnabled request, and only apply a narrowly scoped disconnect
after the final retry failure if required to preserve fail-closed behavior; keep
unrelated pairing, session, and lane RPCs connected.
In `@scripts/posthog/provision.mjs`:
- Around line 471-477: Update the top-level catch in the isDirectRun block to
redact using the same trimmed API key value produced by configFromEnv, rather
than the raw process.env[API_KEY_ENV] value. Reuse the existing normalized
configuration or trimming logic so redaction matches the runtime key while
preserving current error handling.
---
Nitpick comments:
In `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 3341-3350: Extract the repeated identity-shape check into a shared
helper near the affected listing and invocation logic, and use it for both
analytics action filtering and the invocation gate. Verify the helper classifies
headless CLI, interactive TUI, and desktop socket-backed sessions as user
clients when identity.runId, stepId, attemptId, and chatSessionId are all
absent; preserve rejection for sessions carrying any of those fields.
In `@apps/ade-cli/src/tuiClient/productAnalytics.ts`:
- Around line 17-28: Add a table-driven unit test for deriveTuiAnalyticsScreen
covering each priority-ordered branch, including terminal control, drawer,
details with empty and non-empty right panes, add mode, grid view, and the
default chat screen. Assert the expected screen identifier for each input and
include overlapping-state cases to verify the existing precedence order.
In `@apps/web/src/app/pages/TermsPage.tsx`:
- Line 5: Update the TermsPage link attributes to use the shared
ANALYTICS_FEATURE_ATTRIBUTE constant imported from marketingAnalytics instead of
the hardcoded data-ade-analytics-feature string. Apply this consistently to all
affected links while preserving their existing analytics values.
In `@apps/web/vite.config.ts`:
- Line 1: Extract the duplicated PostHog token validation from the web Vite
configuration and desktop tsup configuration into a shared Node utility exposing
assertPublicPostHogToken(token). Update both configuration entry points to
import and call this helper, removing their local regex and error-message
implementations while preserving the existing validation behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85480d54-4295-48ce-8c32-a9a32c0a76c8
📥 Commits
Reviewing files that changed from the base of the PR and between c6d56cb and a84007984e0fc13b0b7a4e721be1a835b53b3cc2.
⛔ Files ignored due to path filters (8)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/ARCHITECTURE.mdis excluded by!docs/**docs/README.mdis excluded by!docs/**docs/features/ade-code/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/web-client/README.mdis excluded by!docs/**docs/logging.mdis excluded by!docs/**
📒 Files selected for processing (106)
.agents/skills/test/SKILL.md.github/scripts/ios-testflight-internal-build-bump-asc.sh.github/workflows/ci.yml.github/workflows/ios-testflight-internal-build-bump-self-hosted.yml.github/workflows/release-core.ymlapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/productAnalyticsRemoteCommand.tsapps/ade-cli/src/services/sync/syncHostService.test.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/appPolling.test.tsxapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/cli.tsxapps/ade-cli/src/tuiClient/productAnalytics.tsapps/ade-cli/tsup.config.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/analytics/agentTurnProductAnalytics.tsapps/desktop/src/main/services/analytics/dailyUsageAnalytics.tsapps/desktop/src/main/services/analytics/productAnalyticsPolicy.tsapps/desktop/src/main/services/analytics/productAnalyticsService.test.tsapps/desktop/src/main/services/analytics/productAnalyticsService.tsapps/desktop/src/main/services/analytics/usageProductAnalyticsExporter.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/state/kvDb.migrations.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/usage/usageStatsStore.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/analytics/ProductAnalyticsLifecycle.tsxapps/desktop/src/renderer/components/app/AppShell.aiStatus.test.tsxapps/desktop/src/renderer/components/app/AppShell.tsxapps/desktop/src/renderer/components/settings/GeneralSection.tsxapps/desktop/src/renderer/components/settings/ProductAnalyticsSection.tsxapps/desktop/src/renderer/webclient/adapter/__tests__/adapter.test.tsapps/desktop/src/renderer/webclient/adapter/analytics.tsapps/desktop/src/renderer/webclient/adapter/index.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/productAnalytics.tsapps/desktop/src/shared/types/sync.tsapps/desktop/tsup.config.tsapps/ios/ADE/App/ADEApp.swiftapps/ios/ADE/App/ADEAppDelegate.swiftapps/ios/ADE/App/ContentView.swiftapps/ios/ADE/App/DeepLinkRouter.swiftapps/ios/ADE/Info.plistapps/ios/ADE/PrivacyInfo.xcprivacyapps/ios/ADE/Services/Dictation/DictationController.swiftapps/ios/ADE/Services/ProductAnalytics.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Components/GlobalDictationPill.swiftapps/ios/ADE/Views/Cto/CtoSettingsScreen.swiftapps/ios/ADE/Views/Files/FilesDetailScreen.swiftapps/ios/ADE/Views/Files/FilesSearchScreen.swiftapps/ios/ADE/Views/Lanes/LaneDetailScreen.swiftapps/ios/ADE/Views/PRs/CreatePrWizardView.swiftapps/ios/ADE/Views/PRs/PrDetailScreen.swiftapps/ios/ADE/Views/PersonalChats/PersonalChatsScreen.swiftapps/ios/ADE/Views/Settings/ConnectionSettingsView.swiftapps/ios/ADE/Views/Settings/SettingsAnalyticsSection.swiftapps/ios/ADE/Views/Work/TerminalSessionScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkSessionDestinationView.swiftapps/ios/ADEClip/PrivacyInfo.xcprivacyapps/ios/ADETests/ADETests.swiftapps/ios/ADETests/ProductAnalyticsPolicyTests.swiftapps/ios/ADEWidgets/PrivacyInfo.xcprivacyapps/ios/Scripts/validate-posthog-project-token.shapps/web/.env.exampleapps/web/README.mdapps/web/package.jsonapps/web/src/app/App.tsxapps/web/src/app/pages/DownloadPage.tsxapps/web/src/app/pages/NotFoundPage.tsxapps/web/src/app/pages/OpenPage.tsxapps/web/src/app/pages/PairPage.tsxapps/web/src/app/pages/PrivacyPage.tsxapps/web/src/app/pages/TermsPage.tsxapps/web/src/components/CopyButton.tsxapps/web/src/components/LinkButton.tsxapps/web/src/components/MarketingAnalyticsBridge.tsxapps/web/src/components/SiteFooter.tsxapps/web/src/components/SiteHeader.tsxapps/web/src/components/editorial/BackCover.tsxapps/web/src/components/editorial/FeatureGrid.tsxapps/web/src/components/editorial/Lede.tsxapps/web/src/components/editorial/Masthead.tsxapps/web/src/components/editorial/ShipShowcase.tsxapps/web/src/lib/marketingAnalytics.test.tsapps/web/src/lib/marketingAnalytics.tsapps/web/src/lib/marketingAnalyticsBrowser.tsapps/web/tsconfig.jsonapps/web/vite.config.tspackage.jsonscripts/posthog/dashboard-spec.mjsscripts/posthog/provision.mjsscripts/posthog/provision.test.mjs
|
@codex review |
d47a58c to
3bfdf3e
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Validation
Cost controls
Summary by CodeRabbit
New Features
Documentation
Tests
Greptile Summary
This PR adds privacy-bounded product analytics across ADE surfaces. The main changes are:
Confidence Score: 4/5
Safe to merge after minor redirect hardening.
The reviewed analytics paths apply consent checks, allowlists, local budgets, dedupe, and hashed identifiers. The remaining issue is non-blocking hardening for direct Capture API redirects.
apps/desktop/src/main/services/analytics/productAnalyticsService.tsandapps/web/src/lib/marketingAnalyticsBrowser.ts.What T-Rex did
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as Desktop/Web/iOS/TUI UI participant Consent as Consent Preference participant Host as Runtime/Sync Host participant Policy as Analytics Policy + Budget participant PH as PostHog Capture API UI->>Consent: read or set analytics preference UI->>Host: capture screen/feature/app event Host->>Policy: bind surface/project, sanitize, dedupe, reserve quota alt disabled, unconfigured, invalid, duplicate, or over budget Policy-->>Host: dropped reason Host-->>UI: capture result else accepted Policy->>PH: direct capture payload PH-->>Policy: HTTP result Policy-->>Host: accepted Host-->>UI: capture result end Host->>Policy: usage-ledger export / daily summary Policy->>PH: bounded internal analytics events%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UI as Desktop/Web/iOS/TUI UI participant Consent as Consent Preference participant Host as Runtime/Sync Host participant Policy as Analytics Policy + Budget participant PH as PostHog Capture API UI->>Consent: read or set analytics preference UI->>Host: capture screen/feature/app event Host->>Policy: bind surface/project, sanitize, dedupe, reserve quota alt disabled, unconfigured, invalid, duplicate, or over budget Policy-->>Host: dropped reason Host-->>UI: capture result else accepted Policy->>PH: direct capture payload PH-->>Policy: HTTP result Policy-->>Host: accepted Host-->>UI: capture result end Host->>Policy: usage-ledger export / daily summary Policy->>PH: bounded internal analytics eventsReviews (3): Last reviewed commit: "ship: iteration 1 — harden cross-process..." | Re-trigger Greptile