mirror of
https://github.com/stablyai/orca.git
synced 2026-10-12 16:03:03 +00:00
WIP: Changes before auto-review fixes
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
brennanb2025
Orca
@@ -0,0 +1,41 @@
|
||||
# Design Review Context
|
||||
|
||||
## Document Info
|
||||
- Path: docs/onboarding-funnel-cohort-addendum.md
|
||||
- Type: Technical Spec (Addendum to onboarding-funnel-telemetry.md)
|
||||
- Review started: 2026-05-07
|
||||
|
||||
## Design Direction
|
||||
- Direction confirmed: pending
|
||||
- Chosen approach: Add two booleans (`is_first_repo`, `is_first_workspace`) as cohort properties on 7 onboarding-related events. No new events.
|
||||
- Alternatives considered: TBD (Phase 0.5)
|
||||
- Key UX decisions: TBD (Phase 0.5)
|
||||
|
||||
## Iteration State
|
||||
Current iteration: 1
|
||||
Last completed phase: Setup
|
||||
|
||||
## Addressed Issues (Do Not Re-report)
|
||||
<!-- Issues that were fixed in the design doc. Reviewers should not re-report these. -->
|
||||
<!-- Format: [iteration] | [severity] | [category] | [issue summary] | [how addressed] -->
|
||||
|
||||
[Initially empty - populated after each fix phase]
|
||||
|
||||
## Skipped Issues (Accepted Risks)
|
||||
<!-- Issues reviewed but deemed acceptable for this context. Do not re-report. -->
|
||||
<!-- Format: [iteration] | [severity] | [category] | [reason skipped] | [issue summary] -->
|
||||
<!-- NOTE: Only P2/P3 issues may be skipped. P0/P1 must always be addressed. -->
|
||||
|
||||
[Initially empty - populated during triage phase]
|
||||
|
||||
## Invalidated Findings (Do Not Re-report)
|
||||
<!-- Findings that were challenged and determined to be false positives or noise. -->
|
||||
<!-- Reviewers in future iterations must NOT resurface these. -->
|
||||
<!-- Format: [iteration] | [original severity] | [finding summary] | [reason invalidated] -->
|
||||
|
||||
[Initially empty - populated after each validation phase]
|
||||
|
||||
## Findings History
|
||||
<!-- Running log of findings across iterations for convergence tracking -->
|
||||
|
||||
[Initially empty]
|
||||
+34
-54
@@ -2,79 +2,59 @@
|
||||
|
||||
## Branch Info
|
||||
|
||||
- Base: origin/main (b5d2b288 — branch is at HEAD of main, all changes are uncommitted)
|
||||
- Base: origin/main
|
||||
- Current: brennanb2025/posthog-checkin
|
||||
- All "diff" is uncommitted changes (no commits yet on top of main)
|
||||
|
||||
## Changed Files Summary
|
||||
|
||||
- A docs/onboarding-funnel-telemetry.md
|
||||
- A src/main/ipc/worktree-create-error-class.test.ts
|
||||
- A src/main/ipc/worktree-create-error-class.ts
|
||||
- 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)
|
||||
|
||||
<!-- In scope: issues on these lines OR caused by these changes. Out of scope: unrelated pre-existing issues -->
|
||||
|
||||
| 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
|
||||
| File | Changed Lines |
|
||||
| --------------------------------------------------- | -------------------------------------- |
|
||||
| docs/onboarding-funnel-telemetry.md | 1-174 (new file) |
|
||||
| src/main/ipc/worktree-create-error-class.test.ts | 1-71 (new file) |
|
||||
| src/main/ipc/worktree-create-error-class.ts | 1-55 (new file) |
|
||||
| src/main/ipc/worktrees.ts | 43, 162-189 |
|
||||
| src/renderer/src/components/sidebar/AddRepoDialog.tsx | 14, 197, 215, 224-232, 258, 412 |
|
||||
| src/shared/telemetry-events.test.ts | 109-162 |
|
||||
| src/shared/telemetry-events.ts | 76-102, 217-231, 249, 251 |
|
||||
|
||||
## 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.
|
||||
- worktree-create-error-class.ts:17-20 | Low | Hypothetical Buffer-stderr handling. Current throws all use bare Error('...') without .stderr.
|
||||
- worktrees.ts:168 | Low | Optional comment about future single-caller assumption; not a correctness issue.
|
||||
- AddRepoDialog.tsx:411-414 | Low | Inline arrow onSkip is purely consistency nit; no functional impact.
|
||||
- AddRepoDialog.tsx:411-414, :197, :215, :228-231 | Low | Double-click guards: track is fire-and-forget; button unmounts on close.
|
||||
- telemetry-events.test.ts:115 | Low | Test loop type (use schema.options); maintainability nit.
|
||||
- telemetry-events.test.ts:137-165 | Low | Missing source enum rejection test; redundant with workspace_created coverage.
|
||||
- telemetry-events.ts:87 | Low | Unused export AddRepoSetupStepAction; consistent with peer-type exports.
|
||||
- worktrees.ts:168 | Low | Optional comment about emit placement vs pre-validation throws.
|
||||
- docs/onboarding-funnel-telemetry.md | Low | Multiple stale line citations (telemetry-events.ts, AddRepoDialog.tsx, worktrees.ts, client.ts). Cosmetic doc drift; design doc is for humans, not load-bearing for runtime; symbols stable enough to find via grep.
|
||||
- docs/onboarding-funnel-telemetry.md:124 | Low | Doc forward-look claim that workspace_composer_opened is "mostly redundant" — debatable but doc-only Phase-2 sketch, not implementation.
|
||||
- worktree-create-error-class.ts:19 | Low | Only stderr harvested, not cause/stdout/output. Existing throws all use bare Error('...'); no real-world failures fall through today.
|
||||
- worktrees.ts:188 | Low | track() then throw — track() is a sync void function (client.ts:256). 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.
|
||||
1. **Medium** worktrees.ts:168 — `add_repo_setup_step_action.create_worktree` fires for every `worktrees:create` IPC call, regardless of source. Funnel question is "fraction of users who add a repo bail vs continue", but `worktrees:create` is invoked from many surfaces (WorktreeJumpPalette, useComposerState, launch-work-item-direct, Landing, SidebarHeader, TaskPage). Other 4 actions (skip/configure/open_existing/back) fire only from AddRepoSetupStep. Found by: Claude (×2), Codex.
|
||||
Fix: Move `create_worktree` emit to the renderer, in `handleCreateWorktree` at AddRepoDialog.tsx:204-212, mirroring the other 4 affordances. The SetupStep "Create" click already has a clean renderer-side seam there. Remove the main-side emit at worktrees.ts:168 and update its comment.
|
||||
|
||||
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'`).
|
||||
2. **Medium** worktree-create-error-class.ts:28 — `'already exists'` is too broad: matches Orca's deliberate `Branch "X" already exists` (worktree-remote.ts:97) AND the `orca-runtime.ts:2909` `Branch "X" already exists locally/on a remote` AND any future git-native `fatal: ... already exists` message. Current code already keeps `'already has pr'` and `'could not find an available worktree name'` as specific markers. Found by: Claude.
|
||||
Fix: Replace the bare `'already exists'` substring with the two concrete suffixes Orca actually throws — `'already exists locally'` and `'already exists on a remote'` — plus the existing `'already has pr'` and `'could not find an available worktree name'`. The bare-`'already exists. Pick a different worktree name.'` throw at worktree-remote.ts:97 also matches `'already exists. pick'` (lowercased). Add that anchor too (or the simpler `'already exists. pick'`).
|
||||
|
||||
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.
|
||||
3. **Low→Medium** worktree-create-error-class.ts:50 — `'no git provider'` is bucketed as `git_failed`, but the related SSH-precondition error `'SSH connection is not available'` (worktree-remote.ts:148) falls through to `unknown`. Two SSH-precondition errors → two different buckets. Found by: Claude.
|
||||
Fix: Drop `'no git provider'` from the git_failed list so both SSH-precondition errors consistently bucket to `unknown`. The schema-evolution doctrine prefers narrow + consistent over broad-and-incoherent.
|
||||
|
||||
4. **Medium** AddRepoDialog.tsx:236-240 — Implicit-close on the Setup step (X icon, ESC, outside-click) calls `closeModal()` and `resetState()` without firing `add_repo_setup_step_action`. Functionally equivalent to Skip but invisible to the funnel; under-reports bail-rate. Found by: Claude.
|
||||
Fix: In `onOpenChange`, when `step === 'setup' && !open`, emit `add_repo_setup_step_action` with `action: 'skip'`. Guard against double-fire when the explicit Skip button (line 411-415) calls `closeModal()` which re-enters `onOpenChange(false)` — use a ref-flag that the explicit Skip handler sets, and the implicit handler reads-and-resets. Alternatively, have the explicit Skip handler NOT call `closeModal()` directly and let the dialog close itself via `onOpenChange`, but that's a larger refactor; the ref-flag is the local minimum.
|
||||
|
||||
## Iteration State
|
||||
|
||||
|
||||
@@ -0,0 +1,152 @@
|
||||
# Onboarding Funnel — Cohort Addendum
|
||||
|
||||
Addendum to [`onboarding-funnel-telemetry.md`](./onboarding-funnel-telemetry.md). Adds cohort dimensionality to the events shipping in Phase 1 (and recommends extending two existing events for symmetry). No new events; only new properties.
|
||||
|
||||
## Goal
|
||||
|
||||
Make the funnel queryable by user cohort, not just by event sequence. The headline product question is "where do **new** users drop off?" — not "where do users drop off." The current schema cannot answer the first question without a multi-step session join through every user's full history, because every event fires identically for first-time users and for someone adding their 6th repo.
|
||||
|
||||
A first-time user clicking Skip on the Setup step is "I don't understand what to do here." A returning user with 5 repos clicking Skip is "I'll set this up later." These are not the same UX problem and shouldn't be averaged together. Today they are.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Not adding session-start or session-age events. App-session boundaries are already inferable from `app_opened` plus the device clock; a dedicated event would duplicate work.
|
||||
- Not adding time-on-step instrumentation. The funnel is about transitions, not dwell. Dwell is a separate question with a separate cost (clocks, idle detection, pause-vs-active state).
|
||||
- Not adding a synthetic `onboarding_complete` event. That state is `first agent_started for distinct_id` and is computable in PostHog without a new event.
|
||||
- Not adding any property that approaches uniqueness — no exact repo count, no first-seen timestamp, no install age. Cohort signals are buckets, not measurements.
|
||||
|
||||
## The cohort signal
|
||||
|
||||
Two booleans, computed at emit time from local state the renderer/main process already has:
|
||||
|
||||
### `is_first_repo: boolean`
|
||||
|
||||
True when, at the moment the event fires, the user has **zero repos** in their local store (i.e., this `repo_added` will be their first, or this Setup-step interaction follows their first `repo_added`).
|
||||
|
||||
Once this becomes false for a `distinct_id`, it should never become true again — repos are not deleted as part of normal onboarding flow. (If a user deletes all their repos and re-adds one, they technically re-enter the cohort. Acceptable: that user is in a re-onboarding state and the funnel question still applies.)
|
||||
|
||||
### `is_first_workspace: boolean`
|
||||
|
||||
True when, at the moment the event fires, the user has **zero workspaces** across all repos. The transition from "has a repo, no workspace" to "has a workspace" is the load-bearing step of onboarding — most of the time-to-value gap lives here. `is_first_workspace` lets us isolate the cohort that's mid-conversion.
|
||||
|
||||
## Where to add the properties
|
||||
|
||||
I audited every event in `telemetry-events.ts:245-260`. The table below covers all events that touch the onboarding journey or whose failure modes plausibly skew by cohort. Events that don't (`settings_changed`, `telemetry_opted_in`, `telemetry_opted_out`) are deliberately excluded — see "Events deliberately not extended" below.
|
||||
|
||||
| Event | `is_first_repo` | `is_first_workspace` | Rationale |
|
||||
|---|---|---|---|
|
||||
| `app_opened` (existing) | yes | yes | Session heartbeat. Both booleans here let us answer "what fraction of sessions are by first-time users?" and "of users who ever opened the app, what fraction never reach the funnel?" without a window function over full history. |
|
||||
| `repo_added` (existing) | yes | yes | Marks "user got a repo." Both booleans needed: `is_first_repo` for first-time vs returning, `is_first_workspace` to identify the unusual cohort that has added repos before but never created a workspace (a real symptom we'd want to see). |
|
||||
| `add_repo_setup_step_action` (Phase 1) | yes | yes | Which Setup-step action does each cohort take? Headline diagnostic for the 60% drop. |
|
||||
| `workspace_created` (existing) | yes | yes | Marks "user got a workspace." Both true = onboarding milestone; both false = routine add. The shape that makes onboarding completion measurable. |
|
||||
| `workspace_create_failed` (Phase 1) | yes | yes | Failure modes likely skew by cohort — first-time users hit `permission_denied` (misconfigured SSH); power users hit `path_collision` (naming conflicts). Cohort split is essential for prioritization. |
|
||||
| `agent_started` (existing) | yes | yes | End of the onboarding funnel. Both booleans true = "user just completed onboarding," the canonical conversion signal. |
|
||||
| `agent_error` (existing) | yes | yes | First-time users hitting `binary_not_found` is an onboarding blocker (CLI not installed); power users hitting it is an environment regression. Same enum value, completely different priority — cohort split is what disambiguates. |
|
||||
|
||||
Total: 7 events × 2 properties = 14 property additions. Five of those events already exist; two ship in Phase 1.
|
||||
|
||||
Adding properties to existing events is additive-safe per the schema-evolution doctrine in `telemetry-events.ts:238-244`. Historical events have these properties as `undefined`; dashboards built on the new fields naturally exclude pre-rollout data, which is correct (we don't have ground truth on cohort for pre-rollout users anyway).
|
||||
|
||||
### Events deliberately not extended
|
||||
|
||||
- **`settings_changed`** — settings activity isn't onboarding-specific. A first-time user flipping `experimentalAgentDashboard` and a power user flipping the same toggle are answering different questions, but the cohort dimension doesn't change *what* we'd do with the data. Skip until proven necessary.
|
||||
- **`telemetry_opted_in` / `telemetry_opted_out`** — opt cohort skew is interesting but orthogonal to onboarding flow. Adding the booleans here would also be slightly self-referential (the consent toggle deciding whether to tag itself). Skip.
|
||||
|
||||
## Why booleans, not buckets
|
||||
|
||||
A bucketed enum (`prior_repo_count: '0' | '1' | '2_4' | '5_plus'`) would carry strictly more information for the same privacy cost. We chose booleans anyway because:
|
||||
|
||||
- The product question is binary: "is this an onboarding moment or a routine action?" Buckets answer a question we don't have yet.
|
||||
- Adding a bucketed enum later is additive-safe; reducing dimensionality after the fact is not. Start narrow.
|
||||
- A boolean is unambiguous at the call site. Bucket boundaries (`2_4` vs `5_plus`) require a decision the author has to defend in code review.
|
||||
|
||||
If Phase 1+addendum data shows we want to distinguish "second repo" from "tenth repo," we add `prior_repo_count_bucket` then. Until that question is real, don't pay for it.
|
||||
|
||||
## Where the values come from
|
||||
|
||||
`is_first_repo` is computable from the same store the existing `repo_added` deduplication uses (`repos.ts:41-49`). The repo count is in process memory; no IPC or filesystem call needed.
|
||||
|
||||
`is_first_workspace` is computable from the workspace store. For renderer-side emit sites (the four pure-UI actions in `add_repo_setup_step_action`), the workspace count is already in the renderer's React state. For main-side emit sites (`worktrees:create`), the count is in main's worktree registry. Both paths are O(1) reads — no database query, no async wait.
|
||||
|
||||
The cost is a single field read per emit site. Approximately 2 lines per emit site × 7 emit sites for each boolean ≈ 28 lines beyond the Phase 1 17, plus the schema additions (~7 lines). Total Phase 1 + addendum ≈ 50 LOC. The accessor functions for both booleans are reused across all emit sites — write once.
|
||||
|
||||
## Privacy guardrails
|
||||
|
||||
- **Booleans only.** No counts, no first-seen timestamps, no install dates. The signal is "in-cohort or not" — one bit per dimension, two bits per event.
|
||||
- **No retroactive backfill.** Historical events keep `is_first_repo: undefined`. The dashboard treats undefined as "unknown cohort," not as a default. We do not run a migration to label past events.
|
||||
- **No cross-event correlation in code.** The classifier for each event reads its own local state. We do not introduce a "user lifecycle" service that tracks onboarding status across events — that would be an unbounded surface for cohort PII.
|
||||
|
||||
The properties are subject to the same `.strict()` Zod schema enforcement as everything else; an event with a free-text first-repo claim would be dropped by the validator.
|
||||
|
||||
## SQL once this lands
|
||||
|
||||
**The headline question — onboarding-cohort Setup-step funnel:**
|
||||
|
||||
```sql
|
||||
SELECT
|
||||
properties.action,
|
||||
count(*) AS n
|
||||
FROM events
|
||||
WHERE event = 'add_repo_setup_step_action'
|
||||
AND properties.is_first_repo = true
|
||||
AND timestamp > now() - INTERVAL 30 DAY
|
||||
GROUP BY properties.action
|
||||
ORDER BY n DESC
|
||||
```
|
||||
|
||||
**Cohort-split workspace creation success rate:**
|
||||
|
||||
```sql
|
||||
SELECT
|
||||
properties.is_first_repo AS first_repo_cohort,
|
||||
countIf(event = 'workspace_created') AS ok,
|
||||
countIf(event = 'workspace_create_failed') AS fail,
|
||||
ok / (ok + fail) AS success_rate
|
||||
FROM events
|
||||
WHERE timestamp > now() - INTERVAL 7 DAY
|
||||
GROUP BY first_repo_cohort
|
||||
```
|
||||
|
||||
**Onboarding completion rate (per-day cohort funnel from first repo to first agent):**
|
||||
|
||||
```sql
|
||||
WITH first_repo AS (
|
||||
SELECT distinct_id, min(timestamp) AS t0
|
||||
FROM events
|
||||
WHERE event = 'repo_added' AND properties.is_first_repo = true
|
||||
GROUP BY distinct_id
|
||||
),
|
||||
first_agent AS (
|
||||
SELECT distinct_id, min(timestamp) AS t1
|
||||
FROM events
|
||||
WHERE event = 'agent_started' AND properties.is_first_repo = true
|
||||
GROUP BY distinct_id
|
||||
)
|
||||
SELECT
|
||||
toDate(first_repo.t0) AS day,
|
||||
count() AS started,
|
||||
countIf(first_agent.t1 IS NOT NULL) AS completed,
|
||||
completed / started AS completion_rate
|
||||
FROM first_repo
|
||||
LEFT JOIN first_agent USING distinct_id
|
||||
GROUP BY day
|
||||
ORDER BY day
|
||||
```
|
||||
|
||||
Without the cohort booleans, the equivalent queries require either a window function over every user's full event history (to derive cohort membership at query time) or a maintained materialized view. With the booleans, every cohort question is a single `WHERE` clause.
|
||||
|
||||
## Backward compatibility
|
||||
|
||||
Existing dashboards on `workspace_created`, `repo_added`, `agent_started` continue to work unchanged — the new properties are optional and existing aggregations don't reference them. New cohort-aware dashboards naturally start at the rollout boundary.
|
||||
|
||||
The `funnel_step` event sketched in the parent doc's deferred section should carry the same two booleans when it lands, for the same reasons. Add to the schema sketch.
|
||||
|
||||
## Rollout
|
||||
|
||||
Land alongside Phase 1. The marginal cost is small (~15 LOC), and shipping cohort dimensionality after-the-fact means a window of Phase 1 data without it — exactly the data we'd want to slice.
|
||||
|
||||
## Open questions
|
||||
|
||||
- **Should `is_first_repo` survive a "delete all repos and start over" cycle?** Argument for resetting it to true: that user is in re-onboarding mode, and the funnel question still applies. Argument against: dashboards would need a tiebreaker for the rare distinct_id with multiple `is_first_repo: true` event runs. Default: do not reset. A user who has ever had a repo is not a new user. If this turns out to misclassify a meaningful population, revisit.
|
||||
- **Should we also add `is_first_session: boolean` (true on the user's very first `app_opened`)?** Probably yes, but it lives outside this addendum because it requires a session-tracking mechanism that doesn't exist today. Defer.
|
||||
- **Do we need `prior_repo_count_bucket` in Phase 2?** Only if `is_first_repo: false` shows wide variance worth splitting. Defer until the data tells us.
|
||||
@@ -1,7 +1,8 @@
|
||||
// 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.
|
||||
// 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 './worktree-create-error-class'
|
||||
@@ -26,6 +27,25 @@ describe('classifyWorkspaceCreateError', () => {
|
||||
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'
|
||||
|
||||
@@ -17,27 +17,34 @@ 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 : ''}`
|
||||
// Why: throw sites mix capitalization ('Worktree created...' vs lowercased
|
||||
// git stderr); normalize once so all anchors below can be lowercase literals.
|
||||
const text = `${message} ${typeof stderr === 'string' ? stderr : ''}`.toLowerCase()
|
||||
|
||||
if (text.includes('Could not resolve a default base ref')) {
|
||||
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')
|
||||
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')
|
||||
) {
|
||||
if (text.includes('eacces') || text.includes('eperm') || text.includes('permission denied')) {
|
||||
return 'permission_denied'
|
||||
}
|
||||
if (text.includes('fatal:') || text.includes('git ') || text.includes('worktree')) {
|
||||
// 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.
|
||||
if (
|
||||
text.includes('fatal:') ||
|
||||
text.includes('git worktree') ||
|
||||
text.includes('created but not found in listing') ||
|
||||
text.includes('no git provider')
|
||||
) {
|
||||
return 'git_failed'
|
||||
}
|
||||
return 'unknown'
|
||||
|
||||
Reference in New Issue
Block a user