diff --git a/src/main/index.ts b/src/main/index.ts index 104f31b6e84..ba293c7a190 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -16,6 +16,7 @@ import { closeAllWatchers } from './ipc/filesystem-watcher' import { registerCoreHandlers } from './ipc/register-core-handlers' import { registerMobileHandlers } from './ipc/mobile' import { initTelemetry, shutdownTelemetry, trackAppOpenedOnce } from './telemetry/client' +import { initCohortClassifier } from './telemetry/cohort-classifier' import { resolveConsent } from './telemetry/consent' import { triggerStartupNotificationRegistration } from './ipc/notifications' import { OrcaRuntimeService } from './runtime/orca-runtime' @@ -399,6 +400,13 @@ app.whenReady().then(async () => { // the Store reference, seeds common props, and resets per-session burst // caps. Actual transport initialization is still gated by both flags. initTelemetry(store) + // Why: cohort-classifier reads the repo count synchronously at every emit + // for cohort-extended events. The Store has been sync-loaded above, and + // this init runs before any IPC handler is registered and before any + // window loads — so the classifier is hydrated before any `track()` call, + // regardless of whether it originates from the renderer, an IPC handler, + // or `trackAppOpenedOnce` / `did-finish-load`. + initCohortClassifier(store) stats = new StatsCollector() claudeUsage = new ClaudeUsageStore(store) codexUsage = new CodexUsageStore(store) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 0b37df9d699..3b4a1748cab 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -28,6 +28,7 @@ import { applyTerminalAttributionEnv } from '../attribution/terminal-attribution import { registerPty, unregisterPty } from '../memory/pty-registry' import { track } from '../telemetry/client' import { classifyError } from '../telemetry/classify-error' +import { getCohortAtEmit } from '../telemetry/cohort-classifier' import { agentKindSchema, launchSourceSchema, @@ -945,7 +946,8 @@ export function registerPtyHandlers( const classified = classifyError(err) track('agent_error', { agent_kind: errorAgentKind, - error_class: classified.error_class + error_class: classified.error_class, + ...getCohortAtEmit() }) } throw err @@ -1101,7 +1103,8 @@ export function registerPtyHandlers( track('agent_started', { agent_kind: agentKindParse.data, launch_source: launchSourceParse.data, - request_kind: requestKindParse.data + request_kind: requestKindParse.data, + ...getCohortAtEmit() }) } } diff --git a/src/main/ipc/repos.ts b/src/main/ipc/repos.ts index b6239163cdb..712ea66e3b7 100644 --- a/src/main/ipc/repos.ts +++ b/src/main/ipc/repos.ts @@ -30,6 +30,7 @@ import { getSshGitProvider } from '../providers/ssh-git-dispatch' import { getActiveMultiplexer } from './ssh' import { normalizeSparseDirectories } from './sparse-checkout-directories' import { track } from '../telemetry/client' +import { getCohortAtEmit } from '../telemetry/cohort-classifier' import type { RepoMethod } from '../../shared/telemetry-events' // Why: `method` answers "which entry point did the user take?", not "what did @@ -46,7 +47,11 @@ function emitRepoAdded(method: RepoMethod, alreadyExisted: boolean): void { if (alreadyExisted) { return } - track('repo_added', { method }) + // Why: cohort must read AFTER `store.addRepo()` lands so the just-added + // repo is counted — every call site below already emits post-addRepo, so + // `getCohortAtEmit()` here returns the user's Nth `repo_added` as `N`. + // See docs/onboarding-funnel-cohort-addendum.md §Read-vs-write ordering. + track('repo_added', { method, ...getCohortAtEmit() }) } // Why: module-scoped so the abort handle survives window re-creation on macOS. @@ -391,6 +396,7 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v // other invocation is using it. Leaking a freshly-made empty folder on // a rare race is strictly safer than deleting a directory the winning // call (and the user) now owns. + emitRepoAdded('folder_picker', true) return { repo: raceWinner } } diff --git a/src/main/ipc/telemetry.test.ts b/src/main/ipc/telemetry.test.ts index 9e1b9231cc5..2fcfbf8bfad 100644 --- a/src/main/ipc/telemetry.test.ts +++ b/src/main/ipc/telemetry.test.ts @@ -16,13 +16,15 @@ const { trackMock, setOptInMock, persistBannerAcknowledgeMock, - consumeConsentMutationTokenMock + consumeConsentMutationTokenMock, + getCohortAtEmitMock } = vi.hoisted(() => ({ handleMock: vi.fn(), trackMock: vi.fn(), setOptInMock: vi.fn(), persistBannerAcknowledgeMock: vi.fn(), - consumeConsentMutationTokenMock: vi.fn() + consumeConsentMutationTokenMock: vi.fn(), + getCohortAtEmitMock: vi.fn() })) vi.mock('electron', () => ({ ipcMain: { handle: handleMock } })) @@ -34,6 +36,9 @@ vi.mock('../telemetry/client', () => ({ vi.mock('../telemetry/burst-cap', () => ({ consumeConsentMutationToken: consumeConsentMutationTokenMock })) +vi.mock('../telemetry/cohort-classifier', () => ({ + getCohortAtEmit: getCohortAtEmitMock +})) import { _resetStoreForTests, registerTelemetryHandlers } from './telemetry' @@ -82,6 +87,8 @@ describe('telemetry IPC handlers', () => { persistBannerAcknowledgeMock.mockReset() consumeConsentMutationTokenMock.mockReset() consumeConsentMutationTokenMock.mockReturnValue(true) + getCohortAtEmitMock.mockReset() + getCohortAtEmitMock.mockReturnValue({ nth_repo_added: 0 }) _resetStoreForTests() }) afterEach(() => { @@ -102,12 +109,53 @@ describe('telemetry IPC handlers', () => { // ── telemetry:track ────────────────────────────────────────────────── - it('forwards a well-typed track call to track()', () => { + it('forwards a well-typed track call to track() and injects cohort for COHORT_EXTENDED events', () => { registerWith({ installId: 'x', existedBeforeTelemetryRelease: false, optedIn: true }) + getCohortAtEmitMock.mockReturnValue({ nth_repo_added: 2 }) const handler = handlers.get('telemetry:track')! handler({}, 'app_opened', {}) expect(trackMock).toHaveBeenCalledTimes(1) - expect(trackMock).toHaveBeenCalledWith('app_opened', {}) + expect(trackMock).toHaveBeenCalledWith('app_opened', { nth_repo_added: 2 }) + }) + + // The IPC handler's selectivity is load-bearing: schemas are `.strict()`, + // so injecting `nth_repo_added` on a non-cohort event would silently + // drop the entire event at the validator. Events outside `COHORT_EXTENDED` + // must reach `track()` unmodified. + it('does NOT inject cohort on events outside COHORT_EXTENDED', () => { + registerWith({ installId: 'x', existedBeforeTelemetryRelease: false, optedIn: true }) + const handler = handlers.get('telemetry:track')! + handler({}, 'settings_changed', { setting_key: 'editorAutoSave', value_kind: 'bool' }) + expect(trackMock).toHaveBeenCalledTimes(1) + expect(trackMock).toHaveBeenCalledWith('settings_changed', { + setting_key: 'editorAutoSave', + value_kind: 'bool' + }) + expect(getCohortAtEmitMock).not.toHaveBeenCalled() + }) + + // The renderer-only Setup-step events fire from React `onClick` and + // depend on the IPC handler injecting cohort — call sites stay + // synchronous and pass only their own props. + it('injects cohort for add_repo_setup_step_action (renderer-only event)', () => { + registerWith({ installId: 'x', existedBeforeTelemetryRelease: false, optedIn: true }) + getCohortAtEmitMock.mockReturnValue({ nth_repo_added: 1 }) + const handler = handlers.get('telemetry:track')! + handler({}, 'add_repo_setup_step_action', { action: 'skip' }) + expect(trackMock).toHaveBeenCalledWith('add_repo_setup_step_action', { + action: 'skip', + nth_repo_added: 1 + }) + }) + + // Fail-soft: a degraded classifier returns `{ nth_repo_added: undefined }`. + // The schemas declare the field optional, so the event still validates. + it('forwards undefined cohort when the classifier returns undefined', () => { + registerWith({ installId: 'x', existedBeforeTelemetryRelease: false, optedIn: true }) + getCohortAtEmitMock.mockReturnValue({ nth_repo_added: undefined }) + const handler = handlers.get('telemetry:track')! + handler({}, 'app_opened', {}) + expect(trackMock).toHaveBeenCalledWith('app_opened', { nth_repo_added: undefined }) }) it('drops track calls with a non-string name', () => { @@ -129,12 +177,13 @@ describe('telemetry IPC handlers', () => { it('treats null/undefined props as an empty object', () => { registerWith({ installId: 'x', existedBeforeTelemetryRelease: false, optedIn: true }) + getCohortAtEmitMock.mockReturnValue({ nth_repo_added: 0 }) const handler = handlers.get('telemetry:track')! handler({}, 'app_opened', null) handler({}, 'app_opened', undefined) expect(trackMock).toHaveBeenCalledTimes(2) - expect(trackMock).toHaveBeenNthCalledWith(1, 'app_opened', {}) - expect(trackMock).toHaveBeenNthCalledWith(2, 'app_opened', {}) + expect(trackMock).toHaveBeenNthCalledWith(1, 'app_opened', { nth_repo_added: 0 }) + expect(trackMock).toHaveBeenNthCalledWith(2, 'app_opened', { nth_repo_added: 0 }) }) // ── telemetry:setOptIn — input narrowing ───────────────────────────── diff --git a/src/main/ipc/telemetry.ts b/src/main/ipc/telemetry.ts index 1bfddc07d41..c8e15815092 100644 --- a/src/main/ipc/telemetry.ts +++ b/src/main/ipc/telemetry.ts @@ -38,8 +38,10 @@ import { ipcMain } from 'electron' import { consumeConsentMutationToken } from '../telemetry/burst-cap' import { persistBannerAcknowledgeWithoutEmitting, setOptIn, track } from '../telemetry/client' +import { getCohortAtEmit } from '../telemetry/cohort-classifier' import { resolveConsent, type ConsentState } from '../telemetry/consent' import type { Store } from '../persistence' +import { isCohortExtendedEvent } from '../../shared/telemetry-events' import type { EventName, EventProps } from '../../shared/telemetry-events' import type { OptInVia } from '../../shared/telemetry-events' @@ -114,12 +116,24 @@ export function registerTelemetryHandlers(store: Store): void { if (props !== null && props !== undefined && typeof props !== 'object') { return } + // Inject cohort here, at the IPC entry, only for events whose schemas + // declare `nth_repo_added` (see `COHORT_EXTENDED` in telemetry-events.ts). + // The selectivity is load-bearing: schemas are `.strict()`, so adding + // `nth_repo_added` to an event that does not declare it would fail Zod + // validation and silently drop the entire event. The renderer call sites + // stay synchronous (matching the existing fire-and-forget shape) and + // avoid an extra IPC round-trip to fetch cohort. + const eventName = name as EventName + const baseProps = (props ?? {}) as Record + const finalProps = isCohortExtendedEvent(eventName) + ? { ...baseProps, ...getCohortAtEmit() } + : baseProps // The casts to `EventName` / `EventProps` here are // pass-through only — this file does NOT pretend the renderer's // name/props are type-safe. The validator inside `track()` is the // single enforcement point at runtime; these casts only feed the // typed channel that the validator will re-check. - track(name as EventName, (props ?? {}) as EventProps) + track(eventName, finalProps as EventProps) }) ipcMain.handle('telemetry:setOptIn', (_event, optedIn: unknown): Promise | void => { diff --git a/src/main/ipc/workspace-create-error-classifier.test.ts b/src/main/ipc/workspace-create-error-classifier.test.ts new file mode 100644 index 00000000000..b4cc075d103 --- /dev/null +++ b/src/main/ipc/workspace-create-error-classifier.test.ts @@ -0,0 +1,76 @@ +// Spot-check the classifier against the load-bearing throws in +// worktree-remote.ts. Not exhaustive — substring widening still requires +// explicit review per the schema-evolution doctrine in +// telemetry-events.ts. Add fixtures here when adding new buckets or when +// a renamed throw site needs regression coverage. + +import { describe, expect, it } from 'vitest' +import { classifyWorkspaceCreateError } from './workspace-create-error-classifier' + +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 a branch-already-exists-locally throw as path_collision', () => { + const err = new Error( + 'Branch "feature/foo" already exists locally. Pick a different worktree name.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets an existing-PR collision throw as path_collision', () => { + const err = new Error( + 'Branch "feature/foo" already has PR #42. Pick a different worktree name.' + ) + expect(classifyWorkspaceCreateError(err)).toBe('path_collision') + }) + + it('buckets the post-create listing-miss throw as git_failed', () => { + const err = new Error('Worktree created but not found in listing') + expect(classifyWorkspaceCreateError(err)).toBe('git_failed') + }) + + 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('falls through to unknown for SSH-precondition errors', () => { + const err = new Error('SSH connection is not available. Please reconnect and try again.') + 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/workspace-create-error-classifier.ts b/src/main/ipc/workspace-create-error-classifier.ts new file mode 100644 index 00000000000..5a24493fd62 --- /dev/null +++ b/src/main/ipc/workspace-create-error-classifier.ts @@ -0,0 +1,53 @@ +// 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 { + // Why: throw sites mix capitalization ('Worktree created...' vs lowercased + // git messages); normalize once so all anchors below can be lowercase literals. + const text = (error instanceof Error ? error.message : '').toLowerCase() + + if (text.includes('could not resolve a default base ref')) { + return 'base_ref_missing' + } + if ( + text.includes('already exists locally') || + text.includes('already exists on a remote') || + text.includes('already exists. pick') || + 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')) { + return 'permission_denied' + } + // Why: anchors are intentionally specific to true git failures from + // worktree-remote.ts. Bare 'git ' / 'worktree' would mis-bucket SSH-relay + // and sparse-checkout validation errors (which the design doc routes to + // 'unknown'); 'created but not found in listing' covers the formerly + // miscased 'Worktree created but not found in listing' throw. + // SSH-precondition failures (e.g. 'no git provider', 'SSH connection is + // not available') deliberately fall through to 'unknown' — they're not + // git failures and bucketing them here would mask connectivity issues. + if ( + text.includes('fatal:') || + text.includes('git worktree') || + text.includes('created but not found in listing') + ) { + return 'git_failed' + } + return 'unknown' +} diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index f85a4915255..8bbd318c7a0 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -45,7 +45,9 @@ import { killAllProcessesForWorktree } from '../runtime/worktree-teardown' import { getLocalPtyProvider } from './pty' import { removeWorktreeSymlinks } from './worktree-symlinks' import { track } from '../telemetry/client' +import { getCohortAtEmit } from '../telemetry/cohort-classifier' import { workspaceSourceSchema, type WorkspaceSource } from '../../shared/telemetry-events' +import { classifyWorkspaceCreateError } from './workspace-create-error-classifier' // Why: worktrees discovered on disk (not created via Orca's UI) have no // persisted WorktreeMeta, so mergeWorktree falls back to `lastActivityAt: 0`. @@ -229,10 +231,27 @@ 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) + 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), + ...getCohortAtEmit() + }) + throw error + } // Why: emit `workspace_created` only after the underlying create has // resolved (the helpers throw on failure, so reaching this line means @@ -242,11 +261,10 @@ 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 + from_existing_branch: typeof args.baseBranch === 'string' && args.baseBranch.length > 0, + ...getCohortAtEmit() }) return result diff --git a/src/main/persistence.ts b/src/main/persistence.ts index 8f88719b97b..c8166b85ef9 100644 --- a/src/main/persistence.ts +++ b/src/main/persistence.ts @@ -470,6 +470,16 @@ export class Store { return this.state.repos.map((repo) => this.hydrateRepo(repo)) } + /** + * O(1) read of the persisted repo count. Use this when you only need the + * count (e.g. cohort-classifier) — `getRepos()` hydrates each repo and + * may run a synchronous git subprocess via `getGitUsername()`, which is + * wasteful when the caller only reads `.length`. + */ + getRepoCount(): number { + return this.state.repos.length + } + getRepo(id: string): Repo | undefined { const repo = this.state.repos.find((r) => r.id === id) return repo ? this.hydrateRepo(repo) : undefined diff --git a/src/main/telemetry/client.ts b/src/main/telemetry/client.ts index 03b8effc7f9..c346bd41b5f 100644 --- a/src/main/telemetry/client.ts +++ b/src/main/telemetry/client.ts @@ -33,6 +33,7 @@ import { PostHog } from 'posthog-node' import type { CommonProps, EventName, EventProps, OptInVia } from '../../shared/telemetry-events' import type { Store } from '../persistence' import { consumeBurstToken, resetBurstCapsForSession } from './burst-cap' +import { getCohortAtEmit } from './cohort-classifier' import { resolveConsent, type ConsentState } from './consent' import { commonPropsSchema, validate } from './validator' @@ -436,7 +437,10 @@ export function trackAppOpenedOnce(): void { return } appOpenedTrackedThisSession = true - track('app_opened', {}) + // Why: `nth_repo_added: 0` on `app_opened` is the canonical session-zero + // / pre-repo cohort signal — a user who has launched but never added a + // repo. See docs/onboarding-funnel-cohort-addendum.md. + track('app_opened', { ...getCohortAtEmit() }) } export async function shutdownTelemetry(): Promise { diff --git a/src/main/telemetry/cohort-classifier.test.ts b/src/main/telemetry/cohort-classifier.test.ts new file mode 100644 index 00000000000..2d1593cde8e --- /dev/null +++ b/src/main/telemetry/cohort-classifier.test.ts @@ -0,0 +1,101 @@ +// Pins the cohort-classifier contract: synchronous read of +// `store.getRepoCount()`, fail-soft to `undefined` on any failure mode, +// at most one warn per session. See docs/onboarding-funnel-cohort-addendum.md. + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../shared/types' +import type { Store } from '../persistence' +import { + _resetSessionWarnFlagForTests, + _setStoreForTests, + getCohortAtEmit, + initCohortClassifier +} from './cohort-classifier' + +function makeFakeStore(getRepos: () => Repo[]): Store { + // Both reads delegate to the same `getRepos` thunk so existing tests stay + // meaningful: a throw or a list change is observed identically through + // either accessor. + return { + getRepos: vi.fn(getRepos), + getRepoCount: vi.fn(() => getRepos().length) + } as unknown as Store +} + +function makeRepos(n: number): Repo[] { + return Array.from({ length: n }, (_, i) => ({ id: `repo-${i}` }) as unknown as Repo) +} + +describe('cohort-classifier', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + _setStoreForTests(null) + _resetSessionWarnFlagForTests() + }) + afterEach(() => { + vi.restoreAllMocks() + _setStoreForTests(null) + }) + + it('returns the current repo count', () => { + initCohortClassifier(makeFakeStore(() => makeRepos(3))) + expect(getCohortAtEmit()).toEqual({ nth_repo_added: 3 }) + }) + + // The session-zero / pre-repo cohort signal: a launch with no repos must + // emit `0`, not `undefined`. Filtering `nth_repo_added = 0` on + // `app_opened` is the canonical way to isolate the pre-repo cohort. + it('returns 0 (not undefined) for an empty repo list', () => { + initCohortClassifier(makeFakeStore(() => [])) + expect(getCohortAtEmit()).toEqual({ nth_repo_added: 0 }) + }) + + it('returns undefined when the store is not initialized', () => { + expect(getCohortAtEmit()).toEqual({ nth_repo_added: undefined }) + }) + + it('returns undefined and never throws when getRepos throws', () => { + initCohortClassifier( + makeFakeStore(() => { + throw new Error('disk fault') + }) + ) + expect(() => getCohortAtEmit()).not.toThrow() + expect(getCohortAtEmit()).toEqual({ nth_repo_added: undefined }) + }) + + // Why: a degraded boot should not flood stderr; the warn-once flag is the + // breadcrumb mechanism for "why is some chunk of last week's data + // missing nth_repo_added?" without burning logs on every emit. + it('warns at most once per session even across many degraded calls', () => { + initCohortClassifier( + makeFakeStore(() => { + throw new Error('disk fault') + }) + ) + const warnSpy = console.warn as unknown as ReturnType + for (let i = 0; i < 50; i++) { + getCohortAtEmit() + } + expect(warnSpy).toHaveBeenCalledTimes(1) + }) + + // The session-warn flag resets on initCohortClassifier so a fresh + // process gets a fresh breadcrumb budget. + it('resets the warn flag when reinitialized', () => { + initCohortClassifier( + makeFakeStore(() => { + throw new Error('first') + }) + ) + getCohortAtEmit() + initCohortClassifier( + makeFakeStore(() => { + throw new Error('second') + }) + ) + getCohortAtEmit() + const warnSpy = console.warn as unknown as ReturnType + expect(warnSpy).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/telemetry/cohort-classifier.ts b/src/main/telemetry/cohort-classifier.ts new file mode 100644 index 00000000000..80c01bff7c6 --- /dev/null +++ b/src/main/telemetry/cohort-classifier.ts @@ -0,0 +1,64 @@ +// Single source of truth for cohort state attached to telemetry events. +// See docs/onboarding-funnel-cohort-addendum.md. +// +// `nth_repo_added` is the count of repos the user has at the moment the +// event fires — read from `store.getRepoCount()`. The rule is single +// and consistent: the value reflects current store state at emit time. +// On `repo_added` the read happens *after* `store.addRepo` lands, so the +// user's Nth repo addition emits `N` (the just-landed write is included). +// On every other event, the same read returns whatever the count is — +// including `0` for a brand-new user on `app_opened` who has never added +// a repo. That `0` is the canonical session-zero / pre-repo cohort signal, +// not a sentinel and not "undefined." +// +// Failure mode: this module never throws. On any read error or +// store-not-yet-initialized condition, `getCohortAtEmit` returns +// `{ nth_repo_added: undefined }`. The schemas declare the field +// `.optional()`, so an event with an undefined cohort still validates and +// emits — it just lands without the cohort property. This preserves the +// telemetry rule "must never crash the app." + +import type { Store } from '../persistence' + +let storeRef: Store | null = null + +// Session-scoped flag analogous to `appOpenedTrackedThisSession` in +// `client.ts`: emit one debug breadcrumb per session if the classifier +// has to fail soft, so missing cohort data has a corresponding log line +// without flooding stderr. +let warnedThisSession = false + +export function initCohortClassifier(store: Store): void { + storeRef = store + warnedThisSession = false +} + +export function getCohortAtEmit(): { nth_repo_added: number | undefined } { + if (!storeRef) { + warnOnce('store not initialized') + return { nth_repo_added: undefined } + } + try { + const length = storeRef.getRepoCount() + return { nth_repo_added: length } + } catch (err) { + warnOnce(err instanceof Error ? err.message : String(err)) + return { nth_repo_added: undefined } + } +} + +function warnOnce(reason: string): void { + if (warnedThisSession) { + return + } + warnedThisSession = true + console.warn('[telemetry-cohort] classifier returned undefined', { reason }) +} + +export function _setStoreForTests(store: Store | null): void { + storeRef = store +} + +export function _resetSessionWarnFlagForTests(): void { + warnedThisSession = false +} diff --git a/src/main/telemetry/validator.test.ts b/src/main/telemetry/validator.test.ts index d6619448f34..bd0bcf349bd 100644 --- a/src/main/telemetry/validator.test.ts +++ b/src/main/telemetry/validator.test.ts @@ -102,6 +102,54 @@ describe('validate', () => { expect(result.ok).toBe(false) }) + // ── Cohort property (nth_repo_added) ──────────────────────────────── + // Pin the schema contract from + // docs/onboarding-funnel-cohort-addendum.md. The field is optional so a + // classifier degraded-mode `undefined` still validates; rejected shapes + // (negative, non-integer, string) must drop. + + it('accepts app_opened with nth_repo_added=0 (the session-zero cohort signal)', () => { + const result = validate('app_opened', { nth_repo_added: 0 }) + expect(result.ok).toBe(true) + }) + + it('accepts repo_added with nth_repo_added=1', () => { + const result = validate('repo_added', { method: 'folder_picker', nth_repo_added: 1 }) + expect(result.ok).toBe(true) + }) + + it('accepts events without nth_repo_added (classifier degraded mode)', () => { + const result = validate('agent_started', { + agent_kind: 'claude-code', + launch_source: 'command_palette', + request_kind: 'new' + }) + expect(result.ok).toBe(true) + }) + + it('rejects negative nth_repo_added', () => { + const result = validate('app_opened', { nth_repo_added: -1 } as never) + expect(result.ok).toBe(false) + }) + + it('rejects non-integer nth_repo_added', () => { + const result = validate('app_opened', { nth_repo_added: 1.5 } as never) + expect(result.ok).toBe(false) + }) + + it('rejects nth_repo_added on a non-cohort event (settings_changed)', () => { + // The IPC handler relies on this rejection: schemas are `.strict()`, + // so injecting `nth_repo_added` on an event whose schema does not + // declare it must drop the entire event. The selectivity guard in + // `telemetry:track` is what prevents that from happening in practice. + const result = validate('settings_changed', { + setting_key: 'editorAutoSave', + value_kind: 'bool', + nth_repo_added: 1 + } as never) + expect(result.ok).toBe(false) + }) + // Rate-limit: at most one warn per event name per 60s. We cannot easily // control Date.now() without mocking time, so the coarse assertion is // that repeat-dropping the same event name does not emit a warn on every diff --git a/src/renderer/src/components/sidebar/AddRepoDialog.tsx b/src/renderer/src/components/sidebar/AddRepoDialog.tsx index b7ff3fbe95e..6bdd1e63b14 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() }, @@ -200,6 +202,8 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { ) const handleCreateWorktree = useCallback(() => { + // Why: Setup-step "Create" affordance — fires on click intent, not on IPC arrival, mirroring the other 4 actions in this dialog. + track('add_repo_setup_step_action', { action: 'create_worktree' }) // Why: small delay so the Add Project dialog close animation finishes before // the composer modal takes focus; otherwise the dialog teardown can steal // the first focus frame from the composer's prompt textarea. @@ -210,6 +214,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,11 +223,33 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { // Why: handleBack reuses resetState which already aborts clones and resets all fields. const handleBack = resetState + const handleSkip = useCallback(() => { + track('add_repo_setup_step_action', { action: 'skip' }) + closeModal() + resetState() + }, [closeModal, 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 ( { if (!open) { + // Why: Radix only fires onOpenChange for internal triggers (X icon, ESC, + // outside-click), so this branch only runs for implicit closes — explicit + // Skip is handled on its own renderer-side click handler. Implicit closes + // on the Setup step are funnel-equivalent to Skip. + if (step === 'setup') { + track('add_repo_setup_step_action', { action: 'skip' }) + } closeModal() resetState() } @@ -243,7 +270,7 @@ const AddRepoDialog = React.memo(function AddRepoDialog() { {step === 'setup' && (