feat(posthog): track cli_posthog_invoked on posthog setup#143
Conversation
WalkthroughThe PR extends CLI analytics by introducing a ChangesSetup Command PostHog Analytics Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds analytics tracking for
Confidence Score: 5/5Safe to merge — the change adds a fire-and-forget analytics call behind an existing guard, the client is properly flushed in a finally block, and the previous gap in test coverage has been closed. The new trackPosthog function is a direct copy of the well-tested trackPayments pattern, placed after both project-linked and auth guards so it never fires for unauthenticated runs. The finally-scoped shutdownAnalytics() mirrors what other commands do and is correctly awaited. Tests now mock the analytics module and assert both the happy path and the failure-flush path. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as insforge posthog setup
participant Config as config.js
participant Analytics as analytics.ts
participant API as posthog API
participant PH as PostHog (analytics)
CLI->>Config: getProjectConfig()
Config-->>CLI: proj
CLI->>Config: getAccessToken()
Config-->>CLI: token
CLI->>Analytics: trackPosthog('setup', proj)
Analytics->>PH: "captureEvent(project_id, 'cli_posthog_invoked', {...})"
CLI->>API: ensureDashboardConnection(projectId, token)
API-->>CLI: dashboardConnection result
Note over CLI: finally block always runs
CLI->>Analytics: shutdownAnalytics()
Analytics->>PH: client.shutdown() (flush queue)
Reviews (2): Last reviewed commit: "test(cli): cover cli_posthog_invoked tel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/posthog/setup.test.ts (1)
14-14: ⚡ Quick winAdd direct assertions for analytics lifecycle introduced by this PR.
Line 14 updates the config mock, but tests still don’t verify the new behavior:
trackPosthog('setup', ...)andshutdownAnalytics()(including error paths). Please add explicit assertions so this integration is protected.Suggested test shape
+const analyticsMock = vi.hoisted(() => ({ + trackPosthog: vi.fn(), + shutdownAnalytics: vi.fn().mockResolvedValue(undefined), +})); +vi.mock('../../lib/analytics.js', () => analyticsMock); ... beforeEach(() => { + analyticsMock.trackPosthog.mockReset(); + analyticsMock.shutdownAnalytics.mockReset(); + analyticsMock.shutdownAnalytics.mockResolvedValue(undefined); ... }); ... it('tracks setup invocation and always shuts down analytics', async () => { apiMock.startPosthogCliFlow.mockResolvedValue({ type: 'connected' }); apiMock.fetchPosthogConnection.mockResolvedValue({ kind: 'connected', connection: { apiKey: 'phc_', host: 'h', posthogProjectId: '1' }, }); await runSetup(['--skip-browser']); + expect(analyticsMock.trackPosthog).toHaveBeenCalledWith( + 'setup', + expect.objectContaining({ project_id: 'p1' }), + ); + expect(analyticsMock.shutdownAnalytics).toHaveBeenCalledOnce(); });🤖 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 `@src/commands/posthog/setup.test.ts` at line 14, Update the tests to explicitly assert the analytics lifecycle: add spies/mocks for trackPosthog and shutdownAnalytics and assert trackPosthog('setup', ...) is called with the expected payload (including FAKE_PROJECT_ID) when the setup flow succeeds, and add a separate test that simulates an error during setup and asserts shutdownAnalytics() is still called (and any error handling path is exercised). Use the existing test file's mocks to attach jest.fn() spies to trackPosthog and shutdownAnalytics, verify call counts and arguments for both success and error cases, and ensure the error-path test asserts shutdownAnalytics was invoked even when an exception is thrown.
🤖 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 `@src/commands/posthog/setup.test.ts`:
- Line 14: Update the tests to explicitly assert the analytics lifecycle: add
spies/mocks for trackPosthog and shutdownAnalytics and assert
trackPosthog('setup', ...) is called with the expected payload (including
FAKE_PROJECT_ID) when the setup flow succeeds, and add a separate test that
simulates an error during setup and asserts shutdownAnalytics() is still called
(and any error handling path is exercised). Use the existing test file's mocks
to attach jest.fn() spies to trackPosthog and shutdownAnalytics, verify call
counts and arguments for both success and error cases, and ensure the error-path
test asserts shutdownAnalytics was invoked even when an exception is thrown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9866085-7c38-4f69-a308-80a05ef09c0c
📒 Files selected for processing (3)
src/commands/posthog/setup.test.tssrc/commands/posthog/setup.tssrc/lib/analytics.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Adds cli_posthog_invoked analytics tracking to insforge posthog setup — the implementation itself is correct, but the primary new behavior has no test coverage, which violates the project's TDD requirement.
Requirements context
No /docs/superpowers/ directory exists in this repo. Assessment is against the PR description (track step 2 of the "connect → setup" funnel via cli_posthog_invoked, flush on exit) and the project conventions in .claude/skills/insforge-development/SKILL.md.
Findings
Critical
Software Engineering — Missing analytics mock and test assertion (src/commands/posthog/setup.test.ts)
The test file does not mock ../../lib/analytics.js. As a result:
trackPosthog('setup', proj)— the entire new behavior this PR introduces — has zero test coverage. Remove that call fromrunSetupand every test still passes.shutdownAnalytics()runs for real in every test. It is a no-op only becausePOSTHOG_API_KEYis unset in the test runner. If that env var is ever set (e.g., in a CI environment with analytics credentials), tests would make live network calls to PostHog.
The established pattern in this repo is to mock analytics.js and assert on specific calls. Both src/commands/projects/link.test.ts and src/commands/ai/setup.test.ts follow this pattern:
// link.test.ts and ai/setup.test.ts
vi.mock('../../lib/analytics.js', () => ({
captureEvent: vi.fn(),
trackCommand: vi.fn(), // or trackPayments, etc.
shutdownAnalytics: vi.fn(async () => {}),
}));
// …and then inside the relevant test:
const { trackCommand } = await import('../../lib/analytics.js');
expect(trackCommand).toHaveBeenCalledWith(…);The fix for setup.test.ts should:
- Add
vi.mock('../../lib/analytics.js', () => ({ trackPosthog: vi.fn(), shutdownAnalytics: vi.fn(async () => {}) })). - Add a test (or assertion within an existing test) that
trackPosthogis called with'setup'and the project config after a successful invocation. - Optionally assert
shutdownAnalyticsis called once in thefinallypath.
The insforge-development skill is explicit: "Every new behavior has at least one test" and "Every bug fix has a regression test that fails on main and passes on your branch." This is a blocking requirement before merge.
Suggestion
Software Engineering — Code duplication in src/lib/analytics.ts
trackPosthog (lines 66–80) is structurally identical to trackDiagnose (37–46) and trackPayments (48–62), differing only in the event name string. With three copies of the same pattern, a single private helper would prevent drift:
function trackSubcommand(
eventName: string,
subcommand: string,
config: ProjectConfig,
properties?: Record<string, unknown>,
): void {
captureEvent(config.project_id, eventName, {
subcommand,
project_id: config.project_id,
project_name: config.project_name,
org_id: config.org_id,
region: config.region,
oss_mode: config.project_id === FAKE_PROJECT_ID,
...properties,
});
}
export const trackDiagnose = (s: string, c: ProjectConfig) => trackSubcommand('cli_diagnose_invoked', s, c);
export const trackPayments = (s: string, c: ProjectConfig, p?: Record<string, unknown>) => trackSubcommand('cli_payments_invoked', s, c, p);
export const trackPosthog = (s: string, c: ProjectConfig, p?: Record<string, unknown>) => trackSubcommand('cli_posthog_invoked', s, c, p);Non-blocking, but the duplication will compound as more commands are instrumented.
Software Engineering — Duplicated funnel comment (src/commands/posthog/setup.ts:89-90)
The comment // Step 2 of the "dashboard connect → CLI posthog setup" funnel… appears verbatim in both analytics.ts:64-65 and setup.ts:89-90. The analytics file is the canonical place for this context; the call-site comment in setup.ts is redundant.
Information
Software Engineering — subcommand: string genericity in trackPosthog
The subcommand parameter follows the same signature as trackDiagnose/trackPayments, which have multiple callers (diagnose list, diagnose run, etc.). The posthog command currently only calls trackPosthog('setup', …), so the parameter has one concrete value. This is consistent with the existing pattern and not wrong, but worth noting if posthog gains more subcommands.
Security — no security-relevant changes in this PR
Properties sent to PostHog (project_id, project_name, org_id, region, oss_mode) match what other track* functions already send. No new PII surface.
Performance — no performance-relevant changes in this PR
shutdownAnalytics() is awaited in finally, adding a bounded network flush on exit. This is intentional and matches the pattern used by other commands.
Verdict
request_changes — one Critical item: the trackPosthog call (the core new behavior) has no test, and analytics.js is not mocked, allowing live network calls if POSTHOG_API_KEY is set in CI. Please add the analytics mock and at least one assertion that the event fires before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/commands/posthog/setup.test.ts`:
- Around line 205-212: The test "still flushes analytics when setup fails"
should explicitly assert the failure path: capture the result of runSetup (call
runSetup(['--skip-browser']) into a variable), assert it indicates non-zero exit
(e.g., result.exitCode !== 0 or the equivalent returned value), and then assert
analyticsMock.shutdownAnalytics was called (keep the existing expect on
analyticsMock.shutdownAnalytics). Ensure you still mock
apiMock.startPosthogCliFlow and apiMock.fetchPosthogConnection as currently done
so the failure path is exercised.
🪄 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: 71f3c8ea-5aa0-4f21-b1be-430321d6a32b
📒 Files selected for processing (2)
src/commands/posthog/setup.test.tssrc/commands/posthog/setup.ts
💤 Files with no reviewable changes (1)
- src/commands/posthog/setup.ts
| it('still flushes analytics when setup fails', async () => { | ||
| apiMock.startPosthogCliFlow.mockResolvedValue({ type: 'connected' }); | ||
| apiMock.fetchPosthogConnection.mockResolvedValue({ kind: 'not-connected' }); | ||
|
|
||
| await runSetup(['--skip-browser']); | ||
|
|
||
| expect(analyticsMock.shutdownAnalytics).toHaveBeenCalledOnce(); | ||
| }); |
There was a problem hiding this comment.
Assert the failure path explicitly in the analytics failure test.
Line [209] discards the runSetup result, so this test can still pass if setup unexpectedly succeeds. Capture the result and assert non-zero exit, then keep analytics assertions in the same test.
Suggested patch
it('still flushes analytics when setup fails', async () => {
apiMock.startPosthogCliFlow.mockResolvedValue({ type: 'connected' });
apiMock.fetchPosthogConnection.mockResolvedValue({ kind: 'not-connected' });
- await runSetup(['--skip-browser']);
+ const r = await runSetup(['--skip-browser']);
+ expect(r.exitCode).toBeGreaterThan(0);
+ expect(analyticsMock.trackPosthog).toHaveBeenCalledOnce();
expect(analyticsMock.shutdownAnalytics).toHaveBeenCalledOnce();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('still flushes analytics when setup fails', async () => { | |
| apiMock.startPosthogCliFlow.mockResolvedValue({ type: 'connected' }); | |
| apiMock.fetchPosthogConnection.mockResolvedValue({ kind: 'not-connected' }); | |
| await runSetup(['--skip-browser']); | |
| expect(analyticsMock.shutdownAnalytics).toHaveBeenCalledOnce(); | |
| }); | |
| it('still flushes analytics when setup fails', async () => { | |
| apiMock.startPosthogCliFlow.mockResolvedValue({ type: 'connected' }); | |
| apiMock.fetchPosthogConnection.mockResolvedValue({ kind: 'not-connected' }); | |
| const r = await runSetup(['--skip-browser']); | |
| expect(r.exitCode).toBeGreaterThan(0); | |
| expect(analyticsMock.trackPosthog).toHaveBeenCalledOnce(); | |
| expect(analyticsMock.shutdownAnalytics).toHaveBeenCalledOnce(); | |
| }); |
🤖 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 `@src/commands/posthog/setup.test.ts` around lines 205 - 212, The test "still
flushes analytics when setup fails" should explicitly assert the failure path:
capture the result of runSetup (call runSetup(['--skip-browser']) into a
variable), assert it indicates non-zero exit (e.g., result.exitCode !== 0 or the
equivalent returned value), and then assert analyticsMock.shutdownAnalytics was
called (keep the existing expect on analyticsMock.shutdownAnalytics). Ensure you
still mock apiMock.startPosthogCliFlow and apiMock.fetchPosthogConnection as
currently done so the failure path is exercised.
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Code Review — feat(posthog): track cli_posthog_invoked on posthog setup
Summary: Clean, well-scoped analytics addition that correctly mirrors the established trackDiagnose / trackPayments pattern. No blocking issues found.
Requirements context
No /docs/superpowers/ directory exists in this repo. Review is assessed against the PR description and surrounding code conventions.
The stated intent — track cli_posthog_invoked to measure step 2 of the "connect → setup" funnel, linked to backend posthog_connect_started via project_id — is fully implemented as described.
Findings
Critical
(none)
Suggestion
[Software Engineering — tests] src/commands/posthog/setup.test.ts:205–212
The failure-path test only exercises the case where the setup flow fails after auth/project checks pass (the not-connected data-drift path). There is no test asserting that trackPosthog is not called when getProjectConfig() returns null or getAccessToken() returns null. The code currently places the tracking call after both guards (setup.ts:89), which is correct, but no regression test would catch someone accidentally moving it above those guards in a future edit. A quick test with configMock.getProjectConfig.mockReturnValue(null) verifying trackPosthog call count remains 0 would close this gap.
[Software Engineering — tests] src/commands/posthog/setup.test.ts:91
analyticsMock.shutdownAnalytics.mockClear() vs analyticsMock.trackPosthog.mockReset() — the asymmetry is intentional and correct (shutdownAnalytics was initialized with vi.fn(async () => {}) and mockReset would strip that async implementation, breaking await shutdownAnalytics() in the finally block). It's subtle enough that a comment like // mockClear not mockReset — preserves the async impl would help future readers avoid "fixing" it.
Information
[Functionality] src/commands/posthog/setup.ts:89
trackPosthog fires immediately after auth confirmation and before knowing whether PostHog is already connected or will require OAuth. This is a deliberate design choice (measuring intent at step 2, not outcome) and is consistent with how trackDiagnose is used elsewhere in diagnose commands. Worth noting for analytics consumers: the event count will exceed "successful new connections" by design.
[Software Engineering] src/lib/analytics.ts:66–80
trackPosthog is an exact structural copy of trackDiagnose (same field list, same FAKE_PROJECT_ID oss_mode sentinel, same optional properties spread). This is fine for now, but as more trackX functions accumulate, a shared helper that takes (eventName, config, extra?) could reduce the repetition. Out of scope for this PR — just flagging for a future cleanup if the pattern keeps expanding.
Verdict
approved (informational — explicit GitHub approval is a separate human action)
All four review dimensions are clean:
- Software engineering: Follows established patterns exactly; tests cover success and failure flush paths.
- Functionality: Event placement is correct (post-auth, pre-async work);
shutdownAnalyticsis always awaited viafinally. - Security: No new PII, no secrets, no auth changes, no new dependencies.
- Performance: Fire-and-forget queue;
shutdownis the terminal flush, consistent with every other command.
2 participants
Summary by cubic
Track
cli_posthog_invokedwhen runninginsforge posthog setupto measure step 2 of the connect → setup funnel. Links to backendposthog_connect_startedviaproject_idand flushes analytics on exit (success or failure).trackPosthog('setup', config)to emitcli_posthog_invokedwithproject_id,project_name,org_id,region, andoss_mode(viaFAKE_PROJECT_ID).shutdownAnalytics()in the commandfinallyto flush events.FAKE_PROJECT_IDfor OSS runs.Written for commit e54a6e4. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Note
Track
cli_posthog_invokedanalytics event on posthog setuptrackPosthog(subcommand, config, properties?)function inanalytics.tsthat captures acli_posthog_invokedevent with project/org/region/oss_mode properties.trackPosthog('setup', proj)early in therunSetupflow after project and token validation.finallyblock in the setup command action to always flush analytics viashutdownAnalytics(), regardless of success or failure.Changes since #143 opened
runSetupfunction [e54a6e4]Macroscope summarized c0418cf.