diff --git a/.claude/skills/telemetry-standards/SKILL.md b/.claude/skills/telemetry-standards/SKILL.md index 0ed2f7f85b..b8075526ce 100644 --- a/.claude/skills/telemetry-standards/SKILL.md +++ b/.claude/skills/telemetry-standards/SKILL.md @@ -14,9 +14,9 @@ reviewing PRs that touch tracking or when implementing new tracking. **Format:** `[object]_[verb]` in snake_case -**Approved verbs only:** -opened, clicked, submitted, created, removed, updated, retrieved, intended, evaluated, added, -enabled, disabled, copied, exposed, failed, converted +**Approved verbs only** (canonical list — derived from `packages/common/telemetry-constants.ts`): +opened, clicked, submitted, created, removed, updated, intended, evaluated, added, +enabled, disabled, copied, exposed, failed, converted, closed, completed, applied, sent, moved **Flag these:** - Unapproved verbs (saved, viewed, seen, pressed, etc.) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9686ea3c8a..fbf5c44253 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -9,88 +9,14 @@ This is a TypeScript/Next.js/React monorepo: - `apps/docs/` — Documentation - `packages/common/` — Shared code including telemetry definitions ---- +## Topic-Specific Guidelines -## Telemetry Review Rules +Detailed review rules are in path-specific instruction files under `.github/instructions/`: -These rules apply to changes in `apps/studio/` and `packages/common/telemetry-constants.ts`. All comments are **advisory** — suggest, do not request changes. +- **Telemetry**: `studio-telemetry.instructions.md` — event naming, property conventions, feature flag measurement +- **Testing**: `studio-testing.instructions.md` — test strategy, extraction patterns, coverage expectations -### When to Review for Telemetry - -1. **Changes to `packages/common/telemetry-constants.ts`** — validate event naming, property conventions, and JSDoc accuracy. -2. **Growth-oriented components adding user interactions without tracking** — suggest adding telemetry when a PR adds buttons, forms, toggles, or modals in components that affect user acquisition, activation, or conversion: - - Onboarding / getting started flows - - Connect / setup wizards - - Upgrade / billing CTAs and modals - - A/B experiment variants (anything using `usePHFlag`) - -When tracking is missing, comment: _"This adds a user interaction that may benefit from tracking."_ Then propose an event name and `useTrack()` call. - -### Event Naming - -Format: `[object]_[verb]` in snake_case. - -Prefer verbs that already exist in `packages/common/telemetry-constants.ts` (reuse existing patterns wherever possible). Common examples include: `opened`, `clicked`, `submitted`, `created`, `removed`, `updated`, `retrieved`, `intended`, `evaluated`, `added`, `enabled`, `disabled`, `copied`, `exposed`, `failed`, `converted`, `closed`, `completed`, `applied`, `sent`. - -Flag these: -- Inconsistent or overly generic verbs that don't match existing patterns (e.g. `saved`, `viewed`, `seen`, `pressed`) -- Wrong order: `click_product_card` → should be `product_card_clicked` -- Wrong casing: `productCardClicked` → should be `product_card_clicked` -- Passive view tracking on page load (`dashboard_viewed`, `page_loaded`) — exception: `_exposed` events for A/B experiments are valid - -### Event Properties - -- **camelCase** for new events; match existing convention when extending an event -- Names must be self-explanatory — flag generic names like `label`, `value`, `name`, `data` -- Check `packages/common/telemetry-constants.ts` for similar events and verify property names are consistent (e.g., don't use `aiType` if related events use `assistantType`) -- Never track PII (emails, names, IPs) - -### Event Implementation - -- Import `useTrack` from `lib/telemetry/track` — prefer `useTrack` for new telemetry and avoid introducing new `useSendEventMutation` usage -- New events must have a TypeScript interface in `packages/common/telemetry-constants.ts`: - - Include `@group Events` and `@source` JSDoc tags; add `@page` when applicable (for page-specific events) - - Add the interface to the `TelemetryEvent` union type -- Flag `@source` descriptions that don't match the actual implementation; when `@page` is present, validate that it matches the actual page usage - -### Correct Pattern - -```typescript -import { useTrack } from 'lib/telemetry/track' - -const track = useTrack() -track('product_card_clicked', { - productType: 'database', - planTier: 'pro', - source: 'dashboard', -}) -``` - ---- - -## Testing Review Rules - -These rules apply to changes in `apps/studio/` that add or modify React components or utility functions. All comments are **advisory**. - -### Core Principle - -Push logic out of React components into pure `.utils.ts` functions, then test those functions exhaustively. Only use component tests for complex UI interactions. - -### When to Comment - -- PR adds **business logic inline in a component** that could be extracted to a `ComponentName.utils.ts` file next to the component and unit tested at `tests/components/.../ComponentName.utils.test.ts` -- PR adds a **utility function without test coverage** -- PR uses **component tests for pure logic** that should be a unit test on a pure function -- PR adds a **feature used in both self-hosted and platform** without E2E test consideration - -### Which Test Type to Suggest - -- **Pure transformation** (parse, format, validate, compute) → extract to `.utils.ts` + unit test with vitest -- **Complex UI interaction** → component test with `customRender` (or E2E if shared with self-hosted) -- **E2E tests** should cover both click interactions AND keyboard shortcuts -- **No tests at all** for non-trivial changes → nudge to add coverage - ---- +These files are scoped to `apps/studio/` and applied automatically by Copilot during reviews. ## References diff --git a/.github/instructions/studio-telemetry.instructions.md b/.github/instructions/studio-telemetry.instructions.md new file mode 100644 index 0000000000..3892a6fde6 --- /dev/null +++ b/.github/instructions/studio-telemetry.instructions.md @@ -0,0 +1,59 @@ +--- +applyTo: "apps/studio/**,packages/common/telemetry*" +--- + +# Studio Telemetry Review Rules + +All comments are **advisory** — suggest, do not request changes. + +## When to Flag Missing Telemetry + +Use judgment — not every PR needs telemetry. But **always flag** when: + +1. **Changes to `packages/common/telemetry-constants.ts`** — validate event naming, property conventions, and JSDoc accuracy. +2. **PostHog feature flags without measurement.** If a PR uses `usePHFlag` or PostHog-backed hooks like `useDataApiGrantTogglesEnabled` to gate behavior, the flag state should be captured in a telemetry event so the rollout can be measured. Flag if the flag value isn't included in a relevant `track()` call. (Note: `useFlag` from `common` reads ConfigCat flags, not PostHog — different system, different guidance.) +3. **Feature-flagged rollouts without outcome tracking.** If a flag gates new behavior, there should be telemetry on both the flag state *and* how users respond to the new behavior (e.g., toggle clicks, opt-in actions). +4. **Growth-oriented components adding user interactions without tracking** — onboarding flows, setup wizards, upgrade CTAs, A/B experiment variants. + +When tracking is missing, comment: _"This adds a user interaction (or feature flag) that may benefit from tracking."_ Then propose an event name and `useTrack()` call. + +## Feature Flag Telemetry Pattern + +When capturing a PostHog flag value for telemetry, read the raw flag via `usePHFlag('flagName')` — **not** through wrapper hooks that coerce `undefined` to `false`. Use conditional spread so the property is omitted (not false) when the flag store hasn't loaded: + +```typescript +const flagValue = usePHFlag('myBooleanFlag') // for boolean flags +track('event_name', { + ...(flagValue !== undefined && { myFlagEnabled: flagValue }), +}) +``` + +For string-valued flags (e.g., experiment variants), use `usePHFlag('flagName')` instead. + +## Event Naming + +Format: `[object]_[verb]` in snake_case. + +Prefer verbs already in use in `packages/common/telemetry-constants.ts`: `opened`, `clicked`, `submitted`, `created`, `removed`, `updated`, `intended`, `evaluated`, `added`, `enabled`, `disabled`, `copied`, `exposed`, `failed`, `converted`, `closed`, `completed`, `applied`, `sent`, `moved`. + +Flag: unapproved verbs (`saved`, `viewed`, `pressed`), wrong order (`click_product_card`), wrong casing (`productCardClicked`), passive view tracking on page load (exception: `_exposed` events for A/B experiments). + +## Event Properties + +- **camelCase** for new events; match existing convention when extending +- Self-explanatory names — flag generic (`label`, `value`, `name`, `data`) +- Check `telemetry-constants.ts` for consistency with similar events +- Never track PII + +## Event Implementation + +- Use `useTrack` from `lib/telemetry/track` — avoid introducing new `useSendEventMutation` usage +- New events need a TypeScript interface in `telemetry-constants.ts` with `@group Events` and `@source` JSDoc tags (add `@page` when applicable for page-specific events), added to the `TelemetryEvent` union + +```typescript +import { useTrack } from 'lib/telemetry/track' +const track = useTrack() +track('product_card_clicked', { productType: 'database', planTier: 'pro' }) +``` + +Canonical standards: `.claude/skills/telemetry-standards/SKILL.md` diff --git a/.github/instructions/studio-testing.instructions.md b/.github/instructions/studio-testing.instructions.md new file mode 100644 index 0000000000..0bd11a4b94 --- /dev/null +++ b/.github/instructions/studio-testing.instructions.md @@ -0,0 +1,29 @@ +--- +applyTo: "apps/studio/**" +--- + +# Studio Testing Review Rules + +All comments are **advisory**. + +## Core Principle + +Push logic out of React components into pure `.utils.ts` functions, then test those functions exhaustively. Only use component tests for complex UI interactions. + +## When to Comment + +- PR adds **business logic inline in a component** that could be extracted to a `ComponentName.utils.ts` file next to the component and unit tested at `tests/components/.../ComponentName.utils.test.ts` +- PR adds a **utility function without test coverage** +- PR uses **component tests for pure logic** that should be a unit test on a pure function +- PR adds a **feature used in both self-hosted and platform** without E2E test consideration + +## Which Test Type to Suggest + +- **Pure transformation** (parse, format, validate, compute) → extract to `.utils.ts` + unit test with vitest +- **Complex UI interaction** → component test with `customRender` (or E2E if shared with self-hosted) +- **E2E tests** should cover both click interactions AND keyboard shortcuts +- **No tests at all** for non-trivial changes → nudge to add coverage + +## Reference + +See `.claude/skills/studio-testing/SKILL.md` for the full testing standard.