From d3fd5120ddbfaabe1268a36675a8d920db1f1c65 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 7 May 2026 11:28:16 -0700 Subject: [PATCH] WIP: Changes before auto-review fixes Co-authored-by: Orca --- 00-review-context.md | 83 +++++++++ docs/onboarding-funnel-telemetry.md | 174 ++++++++++++++++++ .../ipc/worktree-create-error-class.test.ts | 51 +++++ src/main/ipc/worktree-create-error-class.ts | 44 +++++ src/main/ipc/worktrees.ts | 35 +++- .../src/components/sidebar/AddRepoDialog.tsx | 15 +- src/shared/telemetry-events.test.ts | 54 ++++++ src/shared/telemetry-events.ts | 44 +++++ 8 files changed, 493 insertions(+), 7 deletions(-) create mode 100644 00-review-context.md create mode 100644 docs/onboarding-funnel-telemetry.md create mode 100644 src/main/ipc/worktree-create-error-class.test.ts create mode 100644 src/main/ipc/worktree-create-error-class.ts diff --git a/00-review-context.md b/00-review-context.md new file mode 100644 index 00000000000..05b4a081e1c --- /dev/null +++ b/00-review-context.md @@ -0,0 +1,83 @@ +# Review Context + +## Branch Info + +- Base: origin/main (b5d2b288 — branch is at HEAD of main, all changes are uncommitted) +- Current: brennanb2025/posthog-checkin +- All "diff" is uncommitted changes (no commits yet on top of main) + +## Changed Files Summary + +- M src/main/ipc/worktrees.ts +- M src/renderer/src/components/sidebar/AddRepoDialog.tsx +- M src/shared/telemetry-events.test.ts +- M src/shared/telemetry-events.ts +- A docs/onboarding-funnel-telemetry.md (untracked) +- A src/main/ipc/worktree-create-error-class.ts (untracked) +- A src/main/ipc/worktree-create-error-class.test.ts (untracked) + +## Changed Line Ranges (PR Scope) + + + +| File | Changed Lines | +|------|---------------| +| src/main/ipc/worktrees.ts | 43 (added import), 162-189 (added try/catch + telemetry), 198 (removed line) | +| src/renderer/src/components/sidebar/AddRepoDialog.tsx | 14 (import), 197, 215, 224-232 (handleSetupStepBack), 258 (onClick rebind), 412 (skip onClick) | +| src/shared/telemetry-events.ts | 76-102 (new schemas), 217-231 (event schemas), 249, 251 (registry entries) | +| src/shared/telemetry-events.test.ts | 109-162 (new test blocks) | +| src/main/ipc/worktree-create-error-class.ts | ALL (new file, 1-44) | +| src/main/ipc/worktree-create-error-class.test.ts | ALL (new file, 1-51) | +| docs/onboarding-funnel-telemetry.md | ALL (new file) | + +## Review Standards Reference + +- Follow /review-code standards +- Focus on: correctness, security, performance, maintainability +- Priority levels: Critical > High > Medium > Low + +## File Categories + +### Backend/IPC +- src/main/ipc/worktrees.ts +- src/main/ipc/worktree-create-error-class.ts +- src/main/ipc/worktree-create-error-class.test.ts + +### Frontend/UI +- src/renderer/src/components/sidebar/AddRepoDialog.tsx + +### Utility/Common +- src/shared/telemetry-events.ts +- src/shared/telemetry-events.test.ts + +### Docs (skipped from code review; reviewed only as context) +- docs/onboarding-funnel-telemetry.md + +## Skipped Issues (Do Not Re-validate) + +- worktree-create-error-class.ts:17-20 | Low | Hypothetical Buffer-stderr handling. Current throws all use bare Error('...') without .stderr; existing code does not need this defensive code per "don't add validation for scenarios that can't happen". | Buffer stderr handling missing +- worktrees.ts:168 | Low | Optional comment about future single-caller assumption. Not a correctness issue. | Comment about future caller of worktrees:create +- AddRepoDialog.tsx:411-414 | Low | Inline arrow onSkip is purely consistency nit; no functional impact. | Inline onSkip arrow not memoized +- AddRepoDialog.tsx:411-414, :197, :215, :228-231 | Low | Double-click guards: track is fire-and-forget; button unmounts on close; impact bounded; precedent (appOpenedTrackedThisSession) is only for useEffect-driven emits. | Skip/Open/Configure/Back double-click guards +- telemetry-events.test.ts:115 | Low | Test loop type (use addRepoSetupStepActionSchema.options); maintainability nit only — test still runtime-fails on typo. | Test loop should use schema.options +- telemetry-events.test.ts:137-165 | Low | Missing source enum rejection test; redundant with workspace_created coverage of same schema. | Missing source enum test +- telemetry-events.ts:87 | Low | Unused export AddRepoSetupStepAction; consistent with peer-type exports (RepoMethod, OptInVia). No fix needed per agent. | Unused type export +- worktrees.ts:168 | Low | Optional comment about emit placement vs pre-validation throws. | Optional emit-placement comment + +## Issues Validated as False Positive + +- worktrees.ts:182 (Codex): track() in catch wrapping concern. track() does not throw — `validate()` returns a result object, posthog.capture is internally guarded, all early-returns inside track() prevent throws. Existing pattern across the codebase wraps no track() call. FP. + +## Issues to Fix (Phase 3) + +1. **High** worktree-create-error-class.ts:18-43 — Case-sensitivity gap: `text.includes('worktree')` is lowercase but real throw site at worktree-remote.ts:203,441 is `'Worktree created but not found in listing'` (capital W). Found by: Claude (×2). Fix: lowercase-normalize the text once before substring checks. + +2. **Medium** worktree-create-error-class.ts:40 — Overly broad `'git '` and `'worktree'` substrings cause SSH relay errors (worktree-remote.ts:190, "The SSH relay has not registered the worktree path yet") to bucket into `git_failed` instead of `unknown`. Found by: Claude (×2). Fix: replace broad anchors with specific markers (`'fatal:'`, `'git worktree'`, `'created but not found'`, `'no git provider'`). + +3. **Medium** worktree-create-error-class.test.ts:1-4 — Header comment over-promises: claims to "fixture the actual throw sites" but only covers 3 of ~10 throw sites in worktree-remote.ts. Found by: Claude. Fix: adjust the comment to honestly describe spot-coverage of load-bearing throws. + +## Iteration State + +Current iteration: 1 +Last completed phase: Validation +Files fixed this iteration: [] diff --git a/docs/onboarding-funnel-telemetry.md b/docs/onboarding-funnel-telemetry.md new file mode 100644 index 00000000000..8c2319251ed --- /dev/null +++ b/docs/onboarding-funnel-telemetry.md @@ -0,0 +1,174 @@ +# Onboarding Funnel Telemetry + +## Goal + +Identify where new users drop off between launching Orca and getting an agent running, with enough granularity to fix it. Today the funnel is `app_opened → repo_added → workspace_created → agent_started`, which collapses ~10 distinct UI surfaces into 4 visible signals. About 60% of users who add a repo never create a workspace (PostHog funnel: ``, measured `` — citation pending), and we have no data on which of the intermediate surfaces is responsible. + +This document proposes the minimum set of events that, together with what already exists, gives the funnel no invisible drop-off points. + +## System context + +Every existing `track()` call site lives in main. New events that originate in the renderer cross the IPC boundary through a bridge that already ships: preload `telemetryTrack` (`src/preload/index.ts:803-815`) → IPC `telemetry:track` handler (`src/main/ipc/telemetry.ts:102-123`) → existing `track()`, with the typed renderer wrapper at `src/renderer/src/lib/telemetry.ts:50-65`. The main-side handler is the single point where the consent gate, burst-cap, and Zod validator run (`client.ts:256-310`). + +``` +┌──────────┐ onClick / mount ┌──────────────────┐ telemetry:track ┌───────────────────┐ capture() ┌─────────┐ +│ Renderer │ ──────────────────► │ preload bridge │ ─────────────────► │ main: track() │ ──────────► │ PostHog │ +│ (React) │ │ window.api.tele- │ │ consent + cap + │ │ │ +│ │ │ metry.track │ │ Zod validate │ │ │ +└──────────┘ └──────────────────┘ └───────────────────┘ └─────────┘ +``` + +The renderer is untrusted relative to telemetry payloads. Names and properties crossing the bridge are revalidated by the same Zod schema used for in-process emits; malformed entries are dropped silently to match the existing validator behavior. + +## Non-goals + +- Not adding per-keystroke instrumentation. Composer-form abandonment is captured by "composer opened, no workspace_created." +- Not tracking which agent or repo a user picked at this layer — that lives on `workspace_created` / `agent_started` properties already. +- Not changing the privacy contract. Every new event respects the consent gate at `client.ts:283-286`, attaches no PII, and routes through the same validator. + +## The path being measured + +For a new user with no repos, the observable journey is: + +``` +1. app launches [client.ts:434, app_opened] +2. Landing screen [Landing.tsx] +3. click "Add Project" → AddRepoDialog opens +4. pick entry method: Browse / Clone / Remote / Create [AddRepoDialog.tsx:31, step state] +5. sub-step (clone/remote/create) succeeds OR errors +6. AddRepoDialog Setup step shown [AddRepoSetupStep.tsx] + repo_added fires here [repos.ts:49] +7. Setup step action: Create worktree / Open existing / Configure / Skip / Back +8. NewWorkspaceComposer opens [NewWorkspaceComposerCard.tsx] +9. user fills form, submits +10. worktrees:create IPC succeeds OR errors [worktrees.ts:150] + workspace_created fires on success [worktrees.ts:176] +11. terminal opens, agent process spawns + agent_started fires [pty.ts:1084] +``` + +Steps 1, 6, 10, 11 emit events today. Steps 2-5, 7-9 are invisible. + +## Proposed events + +Maximum-value-first. Two events ship now because they each close a question that is currently impossible to answer; the rest are deferred behind a single generic `funnel_step` event so the registry doesn't grow one entry per UI surface. "Load-bearing" means the event closes an invisible-today question; "high"/"medium" calibrate marginal value once the load-bearing pair is live. + +### First ship + +#### 1. `add_repo_setup_step_action` — **load-bearing** + +**Fires:** when the user picks an action from `AddRepoSetupStep` (or backs out via the dialog's "Add another project" arrow). The five actions split across two emit sites: + +| Action | Has main-process IPC seam? | Emit site | +|---|---|---| +| `create_worktree` | yes (`worktrees:create`) | main-side, at the IPC handler entry | +| `open_existing` | no (renderer-only `activateAndRevealWorktree`, `AddRepoDialog.tsx:194-200`) | renderer-side `onClick`, via the IPC bridge | +| `configure` | no (opens a renderer-only modal) | renderer-side `onClick`, via the IPC bridge | +| `skip` | no (pure UI dismiss) | renderer-side `onClick`, via the IPC bridge | +| `back` | no (returns to entry-method picker, `AddRepoDialog.tsx:243-251`) | renderer-side `onClick`, via the IPC bridge | + +Only `create_worktree` has a main-process seam; the other four are pure-UI transitions and emit from the renderer because there is nowhere else to put them. This is a deliberate departure from "all telemetry lives in main" — requiring a main-side trace for Skip/Configure/Back/open_existing would mean inventing IPC calls that exist only to carry telemetry. + +**Properties:** `action: 'create_worktree' | 'configure' | 'skip' | 'open_existing' | 'back'` +**Tells us:** what fraction of users who add a repo immediately bail vs continue to workspace creation. This is almost certainly the biggest drop in the existing `repo_added → workspace_created` step (the ~60% drop cited in Goal). Without this, that drop is a single black box; with it, we can split "user hit Skip" (a UX problem we can fix) from "user opened the composer and abandoned mid-form" (a different UX problem) from "user opened composer and the create call failed" (a bug). +**Strength:** high. The action enum has five discrete values that map 1:1 to affordances on a single screen. No interpretation needed. +**Cost:** ~7 lines across the five call sites. The renderer→main bridge already exists (see "System context") — no new infra. + +**Caveat:** folder-mode (non-git) repos auto-close the dialog at `AddRepoDialog.tsx:131-134` and never reach the Setup step, so they emit `repo_added` with no follow-on `add_repo_setup_step_action`. Funnel queries on `repo_added → add_repo_setup_step_action` should expect a legitimate drop here, not a failure to instrument. + +#### 2. `workspace_create_failed` — **load-bearing** + +**Fires:** when `worktrees:create` throws inside `createLocalWorktree` / `createRemoteWorktree`. +**Properties:** `source: WorkspaceSource`, `error_class: 'git_failed' | 'path_collision' | 'permission_denied' | 'base_ref_missing' | 'unknown'`. +**Tells us:** how many of the missing `workspace_created` events are user abandonment vs system failure. Today, the comment at `worktrees.ts:166-173` explicitly notes that `workspace_created` only fires after the helpers resolve — a thrown helper is silent. That silence is fine for the success-rate denominator on creation, but it leaves the failure rate completely unobservable. +**Strength:** high. Errors thrown by a single function with a known taxonomy. Caveat: if a future refactor adds a new throw site, the event needs to be wired there too — same maintenance burden as `agent_error`. + +The `error_class` values are intentionally a separate enum from `agent_error.error_class` (`['binary_not_found', 'unknown']`). Different domain — `agent_error` covers PTY-spawn failures, this covers git/filesystem failures during worktree creation. The schema-evolution comment at `telemetry-events.ts:68-69` warns against speculatively widening enums across domains; merging them would lock both domains to the union forever. + +**Cost:** wrap *only* the `await createLocalWorktree(...)` / `await createRemoteWorktree(...)` calls in a try/catch (`worktrees.ts:162-164`), classify the error, fire the event, rethrow. ~10 lines. The pre-validation throws above those calls (`Repo not found`, `Folder mode does not support creating worktrees` at `worktrees.ts:154-159`) are intentionally NOT instrumented — they signal IPC-shape bugs, not the user-visible git/filesystem failures the funnel cares about, and would otherwise bucket into `unknown` and pollute the failure taxonomy. + +### The renderer→main bridge + +Event #1 is the first *funnel* event to fire from a React `onClick`, but the bridge it rides on already exists and is reused as-is — no new infra: + +- **Preload exposure: `window.api.telemetryTrack(name, props)`** at `src/preload/index.ts:803-815`, mapped to the `telemetry:track` IPC channel. +- **Main-side handler at `src/main/ipc/telemetry.ts:102-123`** is a thin shim that narrows `name`/`props` at the boundary then calls the existing `track()`. All three guards — consent gate (`client.ts:283-286`), burst-cap, Zod validator (`client.ts:256-310`) — apply unchanged. The bridge is not a parallel pipeline; it is an entry point into the same one. +- **Typed renderer wrapper at `src/renderer/src/lib/telemetry.ts:50-65`** gives call sites `EventMap`-based type safety while keeping the preload surface deliberately loose (the validator is the single enforcement point at runtime). +- **The renderer is untrusted relative to telemetry payloads.** Names and props are revalidated by the registry's `.strict()` Zod schema; unknown event names and unknown/extra properties are dropped silently, matching how the in-process validator already behaves. + +The bridge is also reused for any future renderer-originated event, including the deferred `funnel_step` cases below. + +### Deferred to `funnel_step` + +Events #3-#6 in earlier drafts each had a distinct event name (`add_repo_method_selected`, `add_repo_failed`, `add_repo_dialog_opened`, `workspace_composer_opened`). Shipping four named events for four UI surfaces grows the registry linearly with the funnel and creates four enum-design decisions we'd have to get right up front. Instead, when any of these prove necessary after observing #1+#2 data, fold them into a single generic event: + +``` +funnel_step + funnel: 'onboarding_v1' + step: enum // e.g. 'add_repo_dialog_opened', + // 'add_repo_method_selected', + // 'add_repo_failed', + // 'workspace_composer_opened' + source?: enum // surface-specific, e.g. 'landing' | 'sidebar' | 'composer_no_repo' + action?: enum // surface-specific, e.g. method picked + error_class?: enum // surface-specific failure taxonomy +``` + +Adding a new `step` value is additive-safe per the schema-evolution doctrine in `telemetry-events.ts`. New funnels get a new `funnel` value; the same event carries them. + +Sketches of what each deferred case would look like as a `step`: + +- `step: 'add_repo_dialog_opened'`, `source: 'landing' | 'sidebar' | 'composer_no_repo'` — splits the longest invisible gap (`app_opened → repo_added`) into "never opened the dialog" vs "opened and abandoned." Diminishing returns once #1+#2 are live. +- `step: 'add_repo_method_selected'`, `action: 'folder_picker' | 'clone_url' | 'drag_drop' | ...` — visibility into which entry methods users *try* before any succeed. The `action` enum must be unified with `repo_added.method` (`'folder_picker' | 'clone_url' | 'drag_drop'` per `telemetry-events.ts:73`) or explicitly extended with documented superset semantics; otherwise the join `repo_added{method=X} / funnel_step{action=X}` silently breaks. +- `step: 'add_repo_failed'`, `action: `, `error_class: 'user_cancelled' | 'clone_network_error' | 'git_init_failed' | ...` — paired with the previous step to compute per-method success rates with cancels broken out from infra errors. The `error_class` design is the load-bearing part; conflating cancels with errors muddies both metrics. +- `step: 'workspace_composer_opened'`, `source: WorkspaceSource` — composer-mid-form abandonment. Mostly redundant with `add_repo_setup_step_action.action='create_worktree'` once #1 ships. + +For any "X opened" step (dialog/composer), the firing site is a `useEffect` on a modal's open state, which double-fires under React StrictMode and on remounts. Use a session-scoped dedup analogous to `appOpenedTrackedThisSession` in `client.ts:98` to avoid inflating the numerator. + +## What already exists and should not be re-added + +- `app_opened` is the session heartbeat. Don't add another. +- `repo_added` already carries `method` and dedupes re-adds (`repos.ts:41-49`). +- `workspace_created` already carries `source` and `from_existing_branch`. +- `agent_started` already carries `agent_kind`, `launch_source`, `request_kind`. + +## Properties to keep off these events + +In line with the existing schema discipline at `telemetry-events.ts:142-148` (every event uses `.strict()` Zod schemas): + +- No file paths, repo URLs, or display names. +- No clone URLs (already excluded from `repo_added`). +- No raw error messages — only an `error_class` enum value. The classifier for `workspace_create_failed` reads `error.message` strings (the throw sites in `worktree-remote.ts` are bare `throw new Error('...')`, some interpolating user-controlled content like branch names) to bucket into the enum, but those strings never cross the wire. Widening `error_class` later requires explicit review of the substring match set, not silent regex expansion. +- No partial form contents (workspace name, branch name, etc.). +- `value_kind` only on settings-style events, never the value itself. + +## Rollout + +**Phase 1 (this PR): #1 and #2.** Combined cost ~17 lines across the five Setup-step call sites and the `worktrees:create` try/catch. The renderer→main bridge already ships (see "System context"); no new IPC plumbing. Closes the two largest unknowns: Setup-step drop-off and creation failure rate. Both metrics are immediately actionable — Setup-step splits the ~60% drop into UX vs bug; failed-create gives us the failure-rate denominator we lack today. + +**Phase 2 (only if Phase 1 data shows residual unknowns): `funnel_step`.** Land the generic event with whichever `step` values the Phase 1 data actually demands, in priority order: +1. `add_repo_dialog_opened` if `app_opened → repo_added` is still a black box. +2. `add_repo_method_selected` + `add_repo_failed` together — `method_selected` without `failed` produces a misleading conversion rate. +3. `workspace_composer_opened` last; mostly redundant with #1. + +Defer indefinitely if Phase 1 answers the questions on its own. + +## What Phase 1 gives us + +``` +app_opened (existing) + ↓ +repo_added (existing) + ↓ +add_repo_setup_step_action [#1] + ↓ (action = create_worktree) +workspace_created ──or──► workspace_create_failed (existing / [#2]) + ↓ +agent_started (existing) +``` + +The two transitions that were previously single black boxes (`repo_added → workspace_created` and "missing `workspace_created` events") now have measurable numerators and denominators. Remaining invisible gaps are the pre-`repo_added` surfaces, which Phase 2 addresses if needed. + +## Open questions + +- **Should we add a `landing_cta_clicked` step under `funnel_step`?** Only if Phase 1 plus a `step: 'add_repo_dialog_opened'` reveals that most `app_opened` sessions never open the AddRepo dialog. Defer. diff --git a/src/main/ipc/worktree-create-error-class.test.ts b/src/main/ipc/worktree-create-error-class.test.ts new file mode 100644 index 00000000000..3116fea9988 --- /dev/null +++ b/src/main/ipc/worktree-create-error-class.test.ts @@ -0,0 +1,51 @@ +// Pin the classifier's substring set: widening it later requires explicit +// review per the schema-evolution doctrine in telemetry-events.ts. These +// cases fixture the actual throw sites in worktree-remote.ts so a refactor +// that renames the user-facing error strings shows up here as a regression. + +import { describe, expect, it } from 'vitest' +import { classifyWorkspaceCreateError } from './worktree-create-error-class' + +describe('classifyWorkspaceCreateError', () => { + it('buckets the missing-base-ref throw as base_ref_missing', () => { + const err = new Error( + 'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('base_ref_missing') + }) + + it('buckets a branch-already-exists throw as path_collision', () => { + const err = new Error('Branch "feature/foo" already exists. Pick a different worktree name.') + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets the suffix-exhaustion throw as path_collision', () => { + const err = new Error( + 'Could not find an available worktree name for "feature". Pick a different worktree name.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets EACCES errors as permission_denied', () => { + const err = Object.assign(new Error("EACCES: permission denied, mkdir '/tmp/x'"), { + code: 'EACCES' + }) + expect(classifyWorkspaceCreateError(err)).toBe('permission_denied') + }) + + it('buckets generic git errors as git_failed', () => { + const err = new Error('fatal: not a git repository') + expect(classifyWorkspaceCreateError(err)).toBe('git_failed') + }) + + it('falls through to unknown for unrecognised errors', () => { + const err = new Error('something completely unexpected') + expect(classifyWorkspaceCreateError(err)).toBe('unknown') + }) + + it('handles non-Error values without throwing', () => { + expect(classifyWorkspaceCreateError('a bare string')).toBe('unknown') + expect(classifyWorkspaceCreateError(undefined)).toBe('unknown') + expect(classifyWorkspaceCreateError(null)).toBe('unknown') + }) +}) diff --git a/src/main/ipc/worktree-create-error-class.ts b/src/main/ipc/worktree-create-error-class.ts new file mode 100644 index 00000000000..4b7b636ede8 --- /dev/null +++ b/src/main/ipc/worktree-create-error-class.ts @@ -0,0 +1,44 @@ +// Bucket errors thrown by `createLocalWorktree` / `createRemoteWorktree` into +// the `workspace_create_failed` event's `error_class` enum. +// +// Why: the throw sites are bare `throw new Error('...')` calls in +// worktree-remote.ts, some of which interpolate user-controlled content +// (branch names, paths). The classifier reads `error.message` to bucket, but +// the matched strings never cross the wire — only the enum value does. The +// schema discipline at telemetry-events.ts §"Properties to keep off these +// events" is what makes that safe. +// +// Substring set is intentionally narrow: per the schema-evolution doctrine, +// widening this match set later requires explicit review, not silent regex +// expansion. Anything we cannot confidently bucket falls through to `unknown`. + +import type { WorkspaceCreateErrorClass } from '../../shared/telemetry-events' + +export function classifyWorkspaceCreateError(error: unknown): WorkspaceCreateErrorClass { + const message = error instanceof Error ? error.message : '' + const stderr = error instanceof Error ? (error as { stderr?: unknown }).stderr : '' + const text = `${message} ${typeof stderr === 'string' ? stderr : ''}` + + if (text.includes('Could not resolve a default base ref')) { + return 'base_ref_missing' + } + if ( + text.includes('already exists') || + text.includes('already has PR') || + text.includes('Could not find an available worktree name') + ) { + return 'path_collision' + } + if ( + text.includes('EACCES') || + text.includes('EPERM') || + text.includes('permission denied') || + text.includes('Permission denied') + ) { + return 'permission_denied' + } + if (text.includes('fatal:') || text.includes('git ') || text.includes('worktree')) { + return 'git_failed' + } + return 'unknown' +} diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 77274212631..662f673a578 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -40,6 +40,7 @@ import { getLocalPtyProvider } from './pty' import { removeWorktreeSymlinks } from './worktree-symlinks' import { track } from '../telemetry/client' import { workspaceSourceSchema, type WorkspaceSource } from '../../shared/telemetry-events' +import { classifyWorkspaceCreateError } from './worktree-create-error-class' // Why: worktrees discovered on disk (not created via Orca's UI) have no // persisted WorktreeMeta, so mergeWorktree falls back to `lastActivityAt: 0`. @@ -158,10 +159,34 @@ export function registerWorktreeHandlers( throw new Error('Folder mode does not support creating worktrees.') } - // Remote repos route all git operations through the relay - const result = repo.connectionId - ? await createRemoteWorktree(args, repo, store, mainWindow) - : await createLocalWorktree(args, repo, store, mainWindow, runtime) + // Why: the only Setup-step affordance that has a main-process seam to + // ride. The other four (skip/configure/open_existing/back) are pure-UI + // transitions and emit from the renderer (see AddRepoDialog.tsx). + // Firing here covers the load-bearing case where the create request + // actually landed — a renderer-side emit on the SetupStep "Create" + // click would also count abandoned composer sessions. + track('add_repo_setup_step_action', { action: 'create_worktree' }) + + const sourceParse = workspaceSourceSchema.safeParse(args.telemetrySource) + const source: WorkspaceSource = sourceParse.success ? sourceParse.data : 'unknown' + + let result: CreateWorktreeResult + try { + // Why: only wrap the helpers themselves. The pre-validation throws + // above (`Repo not found`, `Folder mode does not support creating + // worktrees`) signal IPC-shape bugs, not the user-visible + // git/filesystem failures the funnel cares about — bucketing them + // into `unknown` would pollute the failure taxonomy. + result = repo.connectionId + ? await createRemoteWorktree(args, repo, store, mainWindow) + : await createLocalWorktree(args, repo, store, mainWindow, runtime) + } catch (error) { + track('workspace_create_failed', { + source, + error_class: classifyWorkspaceCreateError(error) + }) + throw error + } // Why: emit `workspace_created` only after the underlying create has // resolved (the helpers throw on failure, so reaching this line means @@ -171,8 +196,6 @@ export function registerWorktreeHandlers( // baseBranch; an unspecified baseBranch means "branch from default // HEAD", which is the not-from-existing-branch case. We never send // the branch name itself. - const sourceParse = workspaceSourceSchema.safeParse(args.telemetrySource) - const source: WorkspaceSource = sourceParse.success ? sourceParse.data : 'unknown' track('workspace_created', { source, from_existing_branch: typeof args.baseBranch === 'string' && args.baseBranch.length > 0 diff --git a/src/renderer/src/components/sidebar/AddRepoDialog.tsx b/src/renderer/src/components/sidebar/AddRepoDialog.tsx index b7ff3fbe95e..79a59cf846a 100644 --- a/src/renderer/src/components/sidebar/AddRepoDialog.tsx +++ b/src/renderer/src/components/sidebar/AddRepoDialog.tsx @@ -11,6 +11,7 @@ import { } from '@/components/ui/dialog' import { Button } from '@/components/ui/button' import { activateAndRevealWorktree } from '@/lib/worktree-activation' +import { track } from '@/lib/telemetry' import { RemoteStep, CloneStep, useRemoteRepo } from './AddRepoSteps' import { CreateStep, useCreateRepo } from './AddRepoCreateStep' import { SetupStep } from './AddRepoSetupStep' @@ -193,6 +194,7 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { const handleOpenWorktree = useCallback( (worktree: Worktree) => { + track('add_repo_setup_step_action', { action: 'open_existing' }) activateAndRevealWorktree(worktree.id) closeModal() }, @@ -210,6 +212,7 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { }, [closeModal, openModal, repoId]) const handleConfigureRepo = useCallback(() => { + track('add_repo_setup_step_action', { action: 'configure' }) closeModal() openSettingsTarget({ pane: 'repo', repoId }) openSettingsPage() @@ -218,6 +221,15 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { // Why: handleBack reuses resetState which already aborts clones and resets all fields. const handleBack = resetState + // Why: only the Setup step's "Add another project" back arrow counts as a + // funnel event — the in-flight Back arrows on clone/remote/create are not + // a Setup-step affordance. Keeping the emit scoped to this handler avoids + // also tagging mid-clone backs. + const handleSetupStepBack = useCallback(() => { + track('add_repo_setup_step_action', { action: 'back' }) + handleBack() + }, [handleBack]) + return ( Add another project @@ -397,6 +409,7 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { onCreateWorktree={handleCreateWorktree} onConfigureRepo={handleConfigureRepo} onSkip={() => { + track('add_repo_setup_step_action', { action: 'skip' }) closeModal() resetState() }} diff --git a/src/shared/telemetry-events.test.ts b/src/shared/telemetry-events.test.ts index 1a6f76a864a..5b6adaa69ff 100644 --- a/src/shared/telemetry-events.test.ts +++ b/src/shared/telemetry-events.test.ts @@ -110,6 +110,60 @@ describe('agent_started schema', () => { }) }) +describe('add_repo_setup_step_action schema', () => { + it('accepts each of the five Setup-step actions', () => { + for (const action of ['create_worktree', 'configure', 'skip', 'open_existing', 'back']) { + const parsed = eventSchemas.add_repo_setup_step_action.safeParse({ action }) + expect(parsed.success).toBe(true) + } + }) + + it('rejects unknown action enum values', () => { + const parsed = eventSchemas.add_repo_setup_step_action.safeParse({ + action: 'export_to_pdf' + }) + expect(parsed.success).toBe(false) + }) + + it('rejects extra keys via .strict()', () => { + const parsed = eventSchemas.add_repo_setup_step_action.safeParse({ + action: 'skip', + repo_name: 'orca' // raw repo names are UGC — must not cross the wire + }) + expect(parsed.success).toBe(false) + }) +}) + +describe('workspace_create_failed schema', () => { + it('accepts a valid payload', () => { + const parsed = eventSchemas.workspace_create_failed.safeParse({ + source: 'sidebar', + error_class: 'git_failed' + }) + expect(parsed.success).toBe(true) + }) + + it('rejects unknown error_class values', () => { + const parsed = eventSchemas.workspace_create_failed.safeParse({ + source: 'sidebar', + error_class: 'cosmic_ray' + }) + expect(parsed.success).toBe(false) + }) + + // Core invariant mirroring agent_error: raw error strings never cross the + // wire. If this test ever flips, the failure-rate lane is leaking UGC — + // revert the offending schema change. + it('rejects error_message via .strict()', () => { + const parsed = eventSchemas.workspace_create_failed.safeParse({ + source: 'sidebar', + error_class: 'git_failed', + error_message: 'fatal: cannot create work tree at /Users/alice/secret' + }) + expect(parsed.success).toBe(false) + }) +}) + describe('settings_changed schema', () => { it('accepts whitelisted setting keys', () => { for (const key of SETTINGS_CHANGED_WHITELIST) { diff --git a/src/shared/telemetry-events.ts b/src/shared/telemetry-events.ts index a12d8ec0f74..5f47f2d89a1 100644 --- a/src/shared/telemetry-events.ts +++ b/src/shared/telemetry-events.ts @@ -73,6 +73,33 @@ export type ErrorClass = z.infer export const repoMethodSchema = z.enum(['folder_picker', 'clone_url', 'drag_drop']) export type RepoMethod = z.infer +// Five Setup-step affordances the user can pick after `repo_added` fires (see +// AddRepoSetupStep). One enum because every value lives on the same screen and +// the funnel question is "which one did they pick" — adding a sixth value +// later is additive-safe per the schema-evolution doctrine below. +export const addRepoSetupStepActionSchema = z.enum([ + 'create_worktree', + 'configure', + 'skip', + 'open_existing', + 'back' +]) +export type AddRepoSetupStepAction = z.infer + +// Deliberately a separate enum from `errorClassSchema` (PTY-spawn taxonomy): +// different domain — this one buckets git/filesystem failures thrown by +// `createLocalWorktree` / `createRemoteWorktree`. Merging the two would lock +// both domains to the union forever, which the schema-evolution comment +// below warns against. +export const workspaceCreateErrorClassSchema = z.enum([ + 'git_failed', + 'path_collision', + 'permission_denied', + 'base_ref_missing', + 'unknown' +]) +export type WorkspaceCreateErrorClass = z.infer + export const workspaceSourceSchema = z.enum([ 'command_palette', 'sidebar', @@ -187,6 +214,21 @@ const settingsChangedSchema = z const telemetryOptedInSchema = z.object({ via: optInViaSchema }).strict() const telemetryOptedOutSchema = z.object({ via: optInViaSchema }).strict() +const addRepoSetupStepActionEventSchema = z + .object({ action: addRepoSetupStepActionSchema }) + .strict() + +// Why: same enum-only discipline as `agent_error` — `.strict()` rejects raw +// error strings if a future call site tries to attach `error_message` / +// `error_stack`. The classifier in worktrees.ts reads `error.message` to +// bucket into the enum, but those strings never cross the wire. +const workspaceCreateFailedSchema = z + .object({ + source: workspaceSourceSchema, + error_class: workspaceCreateErrorClassSchema + }) + .strict() + // ── Event registry: the one record the validator consumes ─────────────── // // The validator does `eventSchemas[name].safeParse(props)`. `EventMap` is @@ -204,7 +246,9 @@ export const eventSchemas = { app_opened: emptySchema, repo_added: repoAddedSchema, + add_repo_setup_step_action: addRepoSetupStepActionEventSchema, workspace_created: workspaceCreatedSchema, + workspace_create_failed: workspaceCreateFailedSchema, agent_started: agentStartedSchema, agent_error: agentErrorSchema,