Expand PostHog telemetry to close audited coverage gaps#376
Conversation
Adds events at every gap surfaced by the May coverage audit except allow-path hook tracking (intentional skip — volume + noise; deny/instruct already cover the interesting cases). Server-side - CLI lifecycle in bin/failproofai.mjs: cli_install_success / cli_install_failure / cli_uninstall_success / cli_uninstall_failure / cli_list_invoked / cli_parse_error / cli_unexpected_error / hook_dispatch_error - Hook handler in src/hooks/handler.ts: hook_stdin_error, hook_payload_parse_error - Policy evaluator in src/hooks/policy-evaluator.ts: policy_evaluation_error (builtin crashes — distinct from existing custom_hook_error) - Manager / loader / install-prompt: custom_policy_validation_failed, custom_hooks_load_error, policy_params_validation_warning, scope_validation_failed, hook_write_failed, multi_scope_warning_shown, cli_detection_summary, beta_policies_installed - scripts/postinstall.mjs: first_install + version_changed via a new ~/.failproofai/last-version sentinel file Web UI in app/policies/hooks-client.tsx - policies_tab_switched, activity_filter_changed (debounced), activity_row_toggled, activity_copy_clicked, activity_pagination_changed, cli_selection_toggled, cli_install_remove_submitted, cli_reinstall_submitted, policy_config_modal_opened, policy_config_modal_closed, action_error_displayed, hooks_install_from_error_clicked All events reuse the existing helpers (trackHookEvent / trackInstallEvent / captureClientEvent) and honor FAILPROOFAI_TELEMETRY_DISABLED=1. No new wrappers. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds comprehensive PostHog telemetry instrumentation across the CLI, Web-UI, and hook system. It captures CLI command success/failure outcomes, hook processing errors with classification, installation history, and user interactions in the policies dashboard, while respecting the ChangesPostHog telemetry coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/policies/hooks-client.tsx (2)
212-245: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftAdd unit tests for the new Web-UI telemetry triggers.
This file introduces multiple new client telemetry behaviors, but the provided new test file only covers manager/install-prompt server-side events. Please add focused unit tests under
__tests__/hooks/for the new UI capture points.As per coding guidelines, "Always add unit tests for new behaviour. Place tests in tests/. Unit tests live in tests/hooks/, e2e tests in tests/e2e/hooks/."
Also applies to: 364-473, 988-1004, 1448-1460, 1562-1595
🤖 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/policies/hooks-client.tsx` around lines 212 - 245, Add unit tests under __tests__/hooks/ that verify the new client-side telemetry capture calls in this file: specifically test the CopyButton component's handleCopy behavior (import CopyButton and simulate click) to assert usePostHog.capture is called with "activity_copy_clicked" and the correct field value (and default "unknown"), covering both navigator.clipboard.writeText success and the textarea fallback path; similarly add focused tests for every other UI capture point introduced in this file (the captures at the ranges noted) by mocking usePostHog, simulating the UI interaction, and asserting the exact event name and payload are emitted.
1142-1153:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack modal close telemetry on save path too.
policy_config_modal_closedis emitted for cancel/close, but not when Save closes the modal. Add a close event for the save-driven close so opened/closed metrics stay consistent.Also applies to: 1177-1180
🤖 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/policies/hooks-client.tsx` around lines 1142 - 1153, handleSaveParams currently closes the modal by calling setConfiguringPolicy(null) after a successful save but does not emit the "policy_config_modal_closed" telemetry event, causing open/close metrics to be inconsistent; update handleSaveParams to emit the same telemetry event used for cancel/close (call the telemetry emitter with "policy_config_modal_closed" and any required payload) immediately when the modal is closed on save (after awaiting updatePolicyParamsAction and before/after setConfiguringPolicy as your convention dictates), and apply the same change to the analogous save handler around lines 1177-1180 so both save-driven closes and cancel-driven closes emit the same metric; reference functions: handleSaveParams, updatePolicyParamsAction, setConfiguringPolicy, reload, and fireActionError to locate where to add the telemetry call.
🧹 Nitpick comments (1)
app/policies/hooks-client.tsx (1)
478-488: ⚡ Quick winKeep state updater callbacks side-effect free.
capture(...)is currently called insidesetState(prev => ...)updaters. Move telemetry emission outside the updater and compute next state first; this avoids non-idiomatic side effects and duplicate/dev-mode quirks.Also applies to: 1052-1063
🤖 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/policies/hooks-client.tsx` around lines 478 - 488, The state updater passed to setExpandedRow (the setExpandedRow(prev => { ... }) block) performs side effects by calling capture(...) inside the updater; compute the next state first (e.g., determine const next = prev === idx ? null : idx outside the setState callback), call capture(...) after computing next (using items[idx] for decision/policyName/eventType), and then call setExpandedRow(next) so the updater remains side-effect free; apply the same refactor to the duplicate block around the 1052-1063 area.
🤖 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/policies/hooks-client.tsx`:
- Around line 451-466: The telemetry call capture("activity_filter_changed",
...) is firing on initial mount; update the effect surrounding that call in
app/policies/hooks-client.tsx to skip the first render by adding a persistent
flag (e.g., useRef isFirstRender) and returning early on first run, then set the
flag to false so subsequent changes run the existing capture logic; reference
the capture invocation and the effect that reads filterDecision,
filterEventType, filterPolicy, filterSessionId, and filterCli when locating
where to add the isFirstRender guard.
In `@CHANGELOG.md`:
- Line 6: The changelog entry describing the expanded PostHog telemetry (the
long one listing server-side events like cli_install_success/failure,
hook_dispatch_error, policy_evaluation_error, etc., and web-UI events via
usePostHog() in app/policies/hooks-client.tsx) is missing the PR reference
suffix; update that single-line entry to append the PR number in the required
format (for example " (`#376`)") so it becomes one line with the description
followed by the PR reference.
In `@scripts/postinstall.mjs`:
- Around line 176-182: The branch that sends version change telemetry assumes
previousVersion !== currentVersion, making the "reinstall" (cmp === 0) path
unreachable; update the logic around previousVersion/currentVersion so
same-version reinstalls are detectable and reported: compute cmp =
compareSemver(previousVersion, currentVersion) regardless of the outer condition
(or change the condition to allow previousVersion === currentVersion to reach
this block), then call trackInstallEvent("version_changed", { from_version:
previousVersion, to_version: currentVersion, direction: cmp < 0 ? "upgrade" :
cmp > 0 ? "downgrade" : "reinstall", platform: platform() }) so cmp === 0
correctly yields "reinstall" (refer to previousVersion, currentVersion,
compareSemver, trackInstallEvent, platform).
- Around line 141-157: The compareSemver comparator treats longer identifiers as
greater so "1.0.0-beta" > "1.0.0"; flip the tie-break when one side runs out of
identifiers so prerelease identifiers are lower precedence than the plain
release. In the compareSemver function change the undefined-token checks: when
ai === undefined return 1 (the shorter/release is greater) and when bi ===
undefined return -1; keep the existing numeric and lexicographic comparisons
otherwise so prerelease strings like "beta" compare below the corresponding
release.
---
Outside diff comments:
In `@app/policies/hooks-client.tsx`:
- Around line 212-245: Add unit tests under __tests__/hooks/ that verify the new
client-side telemetry capture calls in this file: specifically test the
CopyButton component's handleCopy behavior (import CopyButton and simulate
click) to assert usePostHog.capture is called with "activity_copy_clicked" and
the correct field value (and default "unknown"), covering both
navigator.clipboard.writeText success and the textarea fallback path; similarly
add focused tests for every other UI capture point introduced in this file (the
captures at the ranges noted) by mocking usePostHog, simulating the UI
interaction, and asserting the exact event name and payload are emitted.
- Around line 1142-1153: handleSaveParams currently closes the modal by calling
setConfiguringPolicy(null) after a successful save but does not emit the
"policy_config_modal_closed" telemetry event, causing open/close metrics to be
inconsistent; update handleSaveParams to emit the same telemetry event used for
cancel/close (call the telemetry emitter with "policy_config_modal_closed" and
any required payload) immediately when the modal is closed on save (after
awaiting updatePolicyParamsAction and before/after setConfiguringPolicy as your
convention dictates), and apply the same change to the analogous save handler
around lines 1177-1180 so both save-driven closes and cancel-driven closes emit
the same metric; reference functions: handleSaveParams,
updatePolicyParamsAction, setConfiguringPolicy, reload, and fireActionError to
locate where to add the telemetry call.
---
Nitpick comments:
In `@app/policies/hooks-client.tsx`:
- Around line 478-488: The state updater passed to setExpandedRow (the
setExpandedRow(prev => { ... }) block) performs side effects by calling
capture(...) inside the updater; compute the next state first (e.g., determine
const next = prev === idx ? null : idx outside the setState callback), call
capture(...) after computing next (using items[idx] for
decision/policyName/eventType), and then call setExpandedRow(next) so the
updater remains side-effect free; apply the same refactor to the duplicate block
around the 1052-1063 area.
🪄 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: e3c33aa6-9851-4189-bedd-0c6e8e01a0e6
📒 Files selected for processing (10)
CHANGELOG.md__tests__/hooks/new-telemetry.test.tsapp/policies/hooks-client.tsxbin/failproofai.mjsscripts/postinstall.mjssrc/hooks/custom-hooks-loader.tssrc/hooks/handler.tssrc/hooks/install-prompt.tssrc/hooks/manager.tssrc/hooks/policy-evaluator.ts
- CHANGELOG: append (#376) PR ref to the new feature entry per repo convention. - postinstall.mjs: rewrite compareSemver to handle prerelease tags per semver §11 (a release is greater than the same version + prerelease). Previous string-split approach treated "1.0.0-beta" as newer than "1.0.0", flipping upgrade/downgrade telemetry for beta → stable transitions. - postinstall.mjs: drop the `previousVersion !== currentVersion` guard so `direction: "reinstall"` (cmp === 0) is reachable when users re-run `npm install -g` on the same version. - hooks-client.tsx: skip the first render in the activity_filter_changed debounced effect via a useRef guard. Filters initialize from URL params, so the initial firing wasn't a user action — only emit on real changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat: first-run install prompt on bare `failproofai` invocation
PostHog showed only ~10% of npm-installed users ever ran
`failproofai policies --install` (470 unique installers → 48 over 90d).
The no-args dashboard launch now detects "zero hooks installed across any
detected CLI" and offers to run the existing interactive policy-selection
inline. Non-TTY falls through to the dashboard with a short hint.
- src/hooks/first-run-nudge.ts: opt-out via FAILPROOFAI_NO_FIRST_RUN=1,
walks every detected CLI/scope to skip if anything is already set up,
emits four PostHog events (first_run_nudge_{shown,accepted,declined,
skipped_noninteractive}) so the uplift is measurable.
- bin/failproofai.mjs: args.length === 0 guard before launch("start"),
try/catch-wrapped so the nudge cannot block the dashboard.
- scripts/postinstall.mjs: "Next steps" block for !configured &&
!registered (the brand-new-user case the existing printHooksWarning
doesn't cover).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs: document the first-run install prompt + FAILPROOFAI_NO_FIRST_RUN
Reflect the new no-args behavior shipped in the previous commit. Quickstart
in README and introduction.mdx call out that `failproofai policies --install`
is now optional — running bare `failproofai` will offer to do it. Env-vars
reference gets a new First-run prompt section for FAILPROOFAI_NO_FIRST_RUN.
Chinese mirror (docs/zh/introduction.mdx) and the 14 translated env-vars
files are intentionally left for the translation-sync PR pattern (see #371).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs: add CHANGELOG entry for first-run prompt docs update
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix: drop conflict markers left in CHANGELOG by rebase onto main
The rebase auto-merge for #376's Features entry left literal <<<<<<<
markers around the two coexisting bullets. Both entries are valid;
they're now side-by-side under 0.0.11-beta.2.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Summary
app/policies/hooks-client.tsxviausePostHog()— tab switches, debounced filter changes, row expand, copy buttons, pagination, CLI selection / Apply / Reinstall, modal open/close, error display, install-from-error.trackHookEvent,trackInstallEvent,captureClientEvent) — no new wrappers — and honorFAILPROOFAI_TELEMETRY_DISABLED=1.~/.failproofai/last-versionsentinel file enablesfirst_installvsversion_changeddiscrimination inscripts/postinstall.mjs.Server-side events
bin/failproofai.mjs—cli_install_success/cli_install_failure/cli_uninstall_success/cli_uninstall_failure/cli_list_invoked/cli_parse_error/cli_unexpected_error/hook_dispatch_errorsrc/hooks/handler.ts—hook_stdin_error,hook_payload_parse_errorsrc/hooks/policy-evaluator.ts—policy_evaluation_error(builtin crashes; distinct from existingcustom_hook_error)src/hooks/manager.ts—custom_policy_validation_failed,policy_params_validation_warning,scope_validation_failed,hook_write_failed,multi_scope_warning_shown,beta_policies_installedsrc/hooks/custom-hooks-loader.ts—custom_hooks_load_error(tagged withis_convention+convention_scope)src/hooks/install-prompt.ts—cli_detection_summary(detected vs explicit vs auto-defaulted)scripts/postinstall.mjs—first_install,version_changed(with upgrade/downgrade/reinstall direction)Test plan
bun run lint— clean (1 pre-existing warning, unrelated)bun --bun tsc --noEmit— cleanbun run test:run— 1628 tests pass (1623 existing + 5 new in__tests__/hooks/new-telemetry.test.ts)bun run build:cli— builds clean--version,policies --garbage(parse_error path),echo 'not json' | --hook PreToolUse(payload parse error path) all behave correctlygh run watchafter push🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests