feat(posthog): delegate SDK install to @posthog/wizard#129
Conversation
WalkthroughThe PostHog setup command is refactored to remove framework detection, package installation, and embedded template file generation. The new flow ensures a dashboard connection exists, then delegates SDK integration to the official PostHog wizard launched via ChangesPostHog Setup Command Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 replaces all custom PostHog SDK installation logic (framework detection, package manager utilities, code templates) with a two-step delegation flow: (1) ensure the InsForge dashboard has a PostHog connection via
Confidence Score: 5/5Safe to merge — the production code is well-structured and the previously raised interactive-stdio and version-drift concerns have been addressed. The refactor correctly delegates all SDK installation to the official wizard. The two-step flow is cleanly separated, SIGINT cancellation is handled gracefully, and JSON mode avoids blocking on the interactive wizard. The one test-quality issue in the --json mode case is cosmetic and does not affect production behaviour. src/commands/posthog/setup.test.ts — the --json mode test contains a dead first runSetup call that silently fails; otherwise no files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant CLI as insforge posthog setup
participant API as InsForge API
participant W as @posthog/wizard (npx)
U->>CLI: insforge posthog setup
CLI->>API: startPosthogCliFlow(projectId)
alt already connected
API-->>CLI: "{ type: connected }"
CLI->>API: fetchPosthogConnection(projectId)
API-->>CLI: "{ kind: connected, connection }"
CLI-->>U: PostHog already connected
else not yet connected
API-->>CLI: "{ type: authorize, authorizeUrl }"
CLI-->>U: Print / open authorizeUrl
loop Poll every 2s (max 15 min)
CLI->>API: pollPosthogConnection
API-->>CLI: connected?
end
CLI-->>U: Connection received
end
alt --json mode
CLI-->>U: "JSON { wizardSkipped: true, wizardCommand }"
else interactive mode
CLI->>W: "spawnSync npx -y @posthog/wizard@latest"
W-->>U: Interactive TUI
W-->>CLI: exit code
alt exit 0
CLI-->>U: Done.
else exit 130 / SIGINT
CLI-->>U: Setup cancelled.
else other non-zero
CLI-->>U: CLIError
end
end
Reviews (2): Last reviewed commit: "fix(posthog): address review (json-mode ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Good scope reduction overall — delegating SDK wiring to @posthog/wizard removes a lot of fragile framework-specific code, and the two-step flow is clearly explained. However, two blocking issues need to be addressed before this merges.
Requirements context
No matching spec/plan found under docs/specs/ (the three existing specs are for the diagnose command and db-migrations). Assessment is based on the PR description and body alone.
Findings
Critical
Software Engineering — No tests for new behavior
src/commands/posthog/setup.ts is the core of this PR (70 lines added, replacing 429), yet no test file is added or updated for it. The deleted framework-detect.test.ts is correctly removed alongside its subject, but the new two-step flow has zero coverage:
ensureDashboardConnection— two code paths (already-connectedfast path with sanity-fetch vs. OAuth + poll), each testable by mockingstartPosthogCliFlow/fetchPosthogConnection/pollPosthogConnection.- Wizard error handling — the
wizardResult.errorbranch and theexitCode !== 0branch. --jsonflag interactions.
The insforge-development skill (this project's own TDD convention) is explicit: "Every new behavior has at least one test. Every bug fix has a regression test." No tests = does not meet the merge bar.
Functionality — JSON mode is non-functional (setup.ts:100-111)
const wizardResult = spawnSync('npx', ['-y', '@posthog/wizard@latest'], {
stdio: opts.json ? 'pipe' : 'inherit', // <-- problem
env: process.env,
});@posthog/wizard is an interactive tool: it prompts for PostHog project selection and OAuth. With stdio: 'pipe', the wizard's stdin is a closed/empty pipe — any read blocks indefinitely or gets EOF immediately, causing the wizard to exit non-zero. That triggers the exitCode !== 0 branch and a CLIError, so outputJson({ success: true, ...result }) never runs. insforge posthog setup --json will always fail.
The options to fix this are:
- Run the wizard unconditionally with
stdio: 'inherit'(dropping--jsonsupport for this command, which is documented as a behavioral change anyway), or - Document that
--jsononly covers the dashboard-connection step and skip the wizard in JSON mode (returning early afterensureDashboardConnection), or - Use async
spawnand stream wizard output toprocess.stdout/stderrwhile still capturing the exit code.
Suggestion
Performance — spawnSync blocks the event loop for the full wizard duration (setup.ts:100)
spawnSync is synchronous and holds the V8 event loop until the wizard exits — which involves OAuth round-trips and package installation, potentially 5+ minutes. Signal handlers registered on the parent process (SIGTERM, SIGINT) won't fire during this time in the expected way.
The repo's established pattern for long-running subprocesses is async spawn wrapped in a Promise (see src/lib/flyctl.ts:112). Using async spawn with stdio: 'inherit' achieves the same TTY-passthrough behavior without blocking.
Functionality — Exit code 130 treated as a hard error (setup.ts:108-110)
if (exitCode !== 0) {
throw new CLIError(`PostHog wizard exited with code ${exitCode}.`);
}If the user presses Ctrl+C inside the wizard, it exits with code 130 (SIGINT). The current code surfaces this as "PostHog wizard exited with code 130" — a confusing error for an intentional cancellation. Consider special-casing 130 (and signal === 'SIGINT' on the spawnSync result) to print a friendlier "Setup cancelled." message and exit cleanly.
Information
Design — runConnectFlow return value is silently discarded (setup.ts:148)
await runConnectFlow(projectId, token, startResult.authorizeUrl, opts);
return 'newly-connected';runConnectFlow returns Promise<PosthogConnectionResponse> but the result is unused. This is harmless — the connection object is no longer needed — but the return type is now misleading. Consider changing runConnectFlow to return Promise<void> to signal intent clearly.
Security — @posthog/wizard@latest is unpinned
The command always downloads and executes the current @posthog/wizard@latest at runtime. This is a deliberate design choice (wizard manages its own upgrades) and acceptable for delegating to a first-party tool, but worth acknowledging: if PostHog's npm account were compromised, any published @latest would run in the user's environment with their full filesystem access. Pinning to a minor range (e.g. @posthog/wizard@^0) rather than @latest would limit blast radius from a hypothetical supply-chain incident.
Verdict
request_changes — two Critical findings (missing tests for new behavior; JSON mode always fails). Suggestion items can be addressed in a follow-up if preferred, but the Critical items must be resolved before merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/commands/posthog/setup.ts (1)
93-97: ⚡ Quick winWarn users to select the same PostHog project in the wizard.
The comment above
runSetup()already notes that the dashboard OAuth and wizard OAuth can land on different PostHog projects, but the user-facing copy never says that. If the wizard is completed against a different project, setup still reports success while the app sends data somewhere the InsForge dashboard is not watching.Suggested copy
if (!opts.json) { outputInfo('Running the official PostHog setup wizard to wire PostHog into your app...'); + outputInfo( + pc.yellow('In the wizard, choose the same PostHog project you connected to the InsForge dashboard.'), + ); outputInfo( pc.dim('(it will open a browser for OAuth and let you pick a PostHog project)'), ); }🤖 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.ts` around lines 93 - 97, The user-facing wizard messaging does not warn that the OAuth browser flow must target the same PostHog project as the dashboard, so update the pre-wizard output in setup.ts (the outputInfo calls before runSetup()) to include a clear warning that users must select the same PostHog project in the OAuth wizard as their InsForge dashboard; change the copy emitted before invoking runSetup() to explicitly tell users to pick the same project (and optionally how to verify it) so successful setup won’t point telemetry to a different PostHog project.
🤖 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.ts`:
- Around line 100-103: The current spawnSync call that invokes '@posthog/wizard'
in the posthog setup (the wizardResult / spawnSync invocation in setup.ts) uses
stdio: opts.json ? 'pipe' : 'inherit', which breaks the wizard's interactive
OAuth flow when opts.json is true; update the invocation so that when opts.json
is true you either add the '--ci' flag to the arguments (i.e., spawn
'@posthog/wizard' with ['-y','@posthog/wizard@latest','--ci'] for
non-interactive mode) or detect opts.json and fail fast with a clear error
message indicating JSON mode is incompatible with the interactive wizard (so
callers can switch to CI mode or not use --json); ensure you reference opts.json
and the spawnSync call/wizardResult when making the change.
- Around line 100-103: The spawnSync call that runs 'npx' (the wizardResult =
spawnSync('npx', ...) in src/commands/posthog/setup.ts) is not Windows-safe;
update the spawnSync options to run via a shell (e.g., add shell: true to the
options object) so Windows can resolve npx.cmd (alternatively invoke cmd.exe /c
or use exec), and keep existing stdio/env handling (preserve opts.json logic) so
the child runs correctly on all platforms.
---
Nitpick comments:
In `@src/commands/posthog/setup.ts`:
- Around line 93-97: The user-facing wizard messaging does not warn that the
OAuth browser flow must target the same PostHog project as the dashboard, so
update the pre-wizard output in setup.ts (the outputInfo calls before
runSetup()) to include a clear warning that users must select the same PostHog
project in the OAuth wizard as their InsForge dashboard; change the copy emitted
before invoking runSetup() to explicitly tell users to pick the same project
(and optionally how to verify it) so successful setup won’t point telemetry to a
different PostHog project.
🪄 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: 3c7aacbc-2b88-4e46-9098-81cb01b9edd5
📒 Files selected for processing (13)
src/commands/posthog/setup.tssrc/lib/api/posthog.tssrc/lib/framework-detect.test.tssrc/lib/framework-detect.tssrc/lib/package-manager.tssrc/templates/posthog/astro/posthog-init.ts.txtsrc/templates/posthog/index.tssrc/templates/posthog/next-app/layout-snippet.tsx.txtsrc/templates/posthog/next-app/posthog-provider.tsx.txtsrc/templates/posthog/next-pages/_app.tsx.txtsrc/templates/posthog/sveltekit/hooks.client.ts.txtsrc/templates/posthog/vite-react/main-snippet.tsx.txtsrc/types/text-imports.d.ts
💤 Files with no reviewable changes (12)
- src/templates/posthog/next-app/layout-snippet.tsx.txt
- src/templates/posthog/next-app/posthog-provider.tsx.txt
- src/lib/framework-detect.ts
- src/templates/posthog/vite-react/main-snippet.tsx.txt
- src/templates/posthog/sveltekit/hooks.client.ts.txt
- src/templates/posthog/index.ts
- src/templates/posthog/next-pages/_app.tsx.txt
- src/templates/posthog/astro/posthog-init.ts.txt
- src/lib/framework-detect.test.ts
- src/types/text-imports.d.ts
- src/lib/api/posthog.ts
- src/lib/package-manager.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 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/commands/posthog/setup.ts">
<violation number="1" location="src/commands/posthog/setup.ts:100">
P1: Use a Windows-safe command when launching the wizard (`npx.cmd` on win32, or `cmd /c`) to avoid `ENOENT` failures on Windows.</violation>
<violation number="2" location="src/commands/posthog/setup.ts:101">
P2: Using `stdio: 'pipe'` in JSON mode breaks the wizard’s interactive flow; `@posthog/wizard` expects terminal interaction in normal mode.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
… win32 npx, tests)
jwfing
left a comment
jwfing
left a comment
There was a problem hiding this comment.
Summary
Clean, well-scoped refactor that correctly delegates SDK wiring to @posthog/wizard while keeping the InsForge dashboard connection step; the code is leaner and the tests cover the main flows.
Requirements context
No /docs/superpowers/ directory exists in this repo — assessing against the PR description and code intent alone.
Findings
Critical
(none)
Suggestion
[Software Engineering — Test quality] JSON mode test has an unintentional first-run failure
src/commands/posthog/setup.test.ts:234-263
The 'skips wizard, emits JSON with wizardCommand' test calls await runSetup(['--skip-browser']) before the JSON-mode run, but never configures spawnSyncMock. After beforeEach's mockReset(), spawnSyncMock() returns undefined, so wizardResult.error throws a TypeError, handleError calls process.exit(1), and the first run silently fails. spawnSyncMock.mockClear() then hides the evidence before the real assertion. The final assertions still validate the correct behaviour, but the first call is dead weight and may confuse future maintainers.
Fix: either add spawnSyncMock.mockReturnValue(spawnOk()) before the first call, or simply drop the first runSetup call entirely — the API mocks set at lines 236–242 remain configured for the second (JSON-mode) invocation without it.
[Functionality — Error message accuracy] forbidden from the sanity-check fetchPosthogConnection reports "not-connected"
src/commands/posthog/setup.ts:167-173
When startPosthogCliFlow returns { type: 'connected' } but the subsequent fetchPosthogConnection sanity-check returns { kind: 'forbidden' }, the code throws:
'cli-start reported connected, but /connection returned not-connected. Try again, or check the dashboard.'
A permissions error is not the same as not-connected — the user will be confused about why retrying would help. Worth handling kind === 'forbidden' distinctly with a message pointing at project access, consistent with how pollPosthogConnection handles it (exit code 5, explicit forbidden message at posthog.ts:181).
[Functionality — Result accuracy] SIGINT-via-signal reports wizardExitCode: 1 instead of 130
src/commands/posthog/setup.ts:128-138
const exitCode = wizardResult.status ?? 1;
if (wizardResult.signal === 'SIGINT' || exitCode === 130) { ... }When the wizard is killed by SIGINT (Node sets status: null, signal: 'SIGINT'), exitCode resolves to 1 via the ?? 1 fallback. The resulting SetupResult then carries wizardExitCode: 1, even though the conventional exit code for SIGINT is 130. Consumers that inspect wizardExitCode to distinguish cancellation from a real wizard failure will misread this. Consider const exitCode = wizardResult.signal === 'SIGINT' ? 130 : (wizardResult.status ?? 1).
Information
env: process.env in spawnSync is redundant
src/commands/posthog/setup.ts:120-123
spawnSync inherits process.env by default when env is omitted; passing it explicitly has no effect. Safe to drop.
Verdict
approved (informational — no Critical findings; human approval via the approve flow is still required)
Three Suggestion-level items worth addressing before merge, but none are blocking. The JSON mode test issue is the most important to fix — it should either configure spawnSyncMock or remove the redundant first runSetup call so the test reads cleanly.
2 participants
Summary by CodeRabbit
New Features
Chores