mirror of
https://github.com/supabase/supabase.git
synced 2026-05-06 08:56:46 -04:00
chore: split Copilot review guidelines into topic-specific files (#43926)
## Context Noticed while working on #43913 that `copilot-instructions.md` is currently at ~4,600 characters. Per [GitHub's docs on Copilot code review](https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review): > Copilot code review only reads the first 4,000 characters of any custom instruction file. Any instructions beyond this limit will not affect the reviews generated by Copilot code review. This means the testing section at the bottom of our current file isn't being read during reviews. ## Proposal Split the single file into path-specific instruction files under `.github/instructions/`, following [GitHub's recommended pattern for repository custom instructions](https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions): > These are specified in one or more `NAME.instructions.md` files within or below the `.github/instructions` directory. > If the path you specify matches a file that Copilot is working on, and a repository-wide custom instructions file also exists, then the instructions from both files are used. This gives us separate files that each stay under the 4K limit and get combined automatically by Copilot during reviews: | File | Size | Scope | |------|------|-------| | `copilot-instructions.md` | 929 chars | General repo context + pointers | | `instructions/studio-telemetry.instructions.md` | 3,570 chars | Telemetry rules for `apps/studio/**` | | `instructions/studio-testing.instructions.md` | 1,228 chars | Testing rules for `apps/studio/**` | Note: Copilot reads instructions from the **base branch** of a PR, not the feature branch — so these won't take effect until merged to master. ### New telemetry guidance The telemetry file adds guidance we've been missing — specifically around feature-flagged rollouts: - Flag PRs that use `usePHFlag`/`useFlag` to gate behavior but don't capture the flag state in telemetry - Flag rollouts that track flag state but not user response to the new behavior - Documents the raw flag pattern (read via `usePHFlag`, not coerced wrapper hooks) to avoid the `undefined`→`false` data quality bug we hit in #43913 ### What didn't change All existing telemetry and testing rules are preserved — nothing was removed, just reorganized. The telemetry rules still reference `.claude/skills/telemetry-standards/SKILL.md` as the authoritative source. ## References - [Adding repository custom instructions for GitHub Copilot](https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions) — file structure, path-specific instructions, frontmatter format - [Using Copilot code review](https://docs.github.com/en/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review) — 4K character limit, base branch behavior ## Open questions Would love the team's input on: - Does the file split make sense, or would you prefer keeping everything in one file (and trimming to fit)? - Are there other topics that should get their own instruction file? - Any concerns with the new feature flag telemetry guidance?
This commit is contained in:
Sean Oliver
@@ -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.)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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<boolean>('myBooleanFlag') // for boolean flags
|
||||
track('event_name', {
|
||||
...(flagValue !== undefined && { myFlagEnabled: flagValue }),
|
||||
})
|
||||
```
|
||||
|
||||
For string-valued flags (e.g., experiment variants), use `usePHFlag<string>('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`
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user