diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index fbf5c442531..75e6f46f14c 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,5 +1,41 @@ # Copilot Code Review Instructions +## Review Policy — Read This First + +You are a code reviewer for a large TypeScript/Next.js/React monorepo. Your reviews must be **low-noise and high-signal**. The team acts on fewer than 20% of default Copilot suggestions, so every comment you leave must earn its place. + +### Confidence Threshold + +Only comment when you are **>85% confident** the issue is a real bug, security vulnerability, or logic error. If you are unsure, do not comment. Silence is better than noise. + +### What NOT to Comment On + +Our CI pipeline already validates the following. **Never comment on these topics:** + +- **Formatting or whitespace** — Prettier runs on every PR +- **Linting issues** — ESLint with auto-fix runs on every PR +- **Type errors** — TypeScript strict-mode typecheck runs on every PR +- **Typos or spelling** — Automated typo detection runs on every PR +- **Missing tests for trivial changes** — Handled by topic-specific test instructions +- **Import ordering or grouping** — Handled by linter +- **Naming style preferences** (camelCase vs snake_case debates) — Follow existing file conventions +- **Accessibility attributes on shadcn/Radix UI components** — See `studio-shadcn-components.instructions.md` for details + +### What TO Comment On (Priority Order) + +1. **Logic errors and bugs** — Off-by-one, null derefs, wrong conditional, unreachable code, incorrect early returns +2. **Security vulnerabilities** — XSS, SQL injection, auth bypass, secrets in code, unsafe `dangerouslySetInnerHTML` +3. **Race conditions and async bugs** — Missing `await`, unhandled promise rejections, stale closures, effect cleanup issues +4. **Data loss risks** — Destructive operations without confirmation, missing error handling on writes +5. **API contract violations** — Wrong HTTP method, missing auth headers, incorrect request/response shapes + +### Comment Style + +- **Be advisory, not prescriptive.** Use "Consider..." or "This may..." — never demand changes. +- **One comment per distinct issue.** Do not leave multiple comments about the same underlying problem. +- **No self-contradictions.** If you suggest a change, do not then flag a problem with your own suggestion. +- **Do not comment on individual commits.** Review the final state of the PR diff only. + ## Repo Context This is a TypeScript/Next.js/React monorepo: @@ -11,16 +47,13 @@ This is a TypeScript/Next.js/React monorepo: ## Topic-Specific Guidelines -Detailed review rules are in path-specific instruction files under `.github/instructions/`: +Path-specific rules in `.github/instructions/`: - **Telemetry**: `studio-telemetry.instructions.md` — event naming, property conventions, feature flag measurement - **Testing**: `studio-testing.instructions.md` — test strategy, extraction patterns, coverage expectations +- **Error Handling**: `studio-error-handling.instructions.md` — error classification, `ErrorMatcher` usage +- **E2E Tests**: `studio-e2e-tests.instructions.md` — selector priority, anti-patterns (`waitForTimeout`, `force: true`) +- **Composition Patterns**: `studio-composition-patterns.instructions.md` — avoid boolean props, use compound components +- **shadcn/Radix Components**: `studio-shadcn-components.instructions.md` — accessibility handled by primitives, do not flag -These files are scoped to `apps/studio/` and applied automatically by Copilot during reviews. - -## References - -For the full, authoritative versions of these standards: - -- Telemetry: `.claude/skills/telemetry-standards/SKILL.md` -- Testing: `.claude/skills/studio-testing/SKILL.md` +These files are scoped to `apps/studio/` and applied automatically during reviews. diff --git a/.github/instructions/studio-composition-patterns.instructions.md b/.github/instructions/studio-composition-patterns.instructions.md new file mode 100644 index 00000000000..ff62a03db49 --- /dev/null +++ b/.github/instructions/studio-composition-patterns.instructions.md @@ -0,0 +1,93 @@ +--- +applyTo: "apps/studio/**" +--- + +# React Composition Patterns Review Rules + +All comments are **advisory**. + +## Core Principle + +Avoid boolean prop proliferation. Use composition (compound components, explicit variants, children) instead of boolean flags to customize behavior. + +## When to Flag + +### 1. Boolean Prop Proliferation (HIGH) + +Flag components accumulating boolean props like `isThread`, `isEditing`, `showAttachments`. Each boolean doubles the state space. + +```tsx +// BAD — unclear intent, combinatorial explosion + + +// GOOD — self-documenting variants + + +``` + +### 2. Render Props Instead of Children (MEDIUM) + +Flag `renderX` callback props when `children` composition would work. + +```tsx +// BAD — render prop for structure + } /> + +// GOOD — compound component + + + + +``` + +### 3. UI Coupled to State Implementation (MEDIUM) + +Flag UI components calling specific state hooks like `useGlobalChannelState()` directly. The provider should own the state implementation; UI should only use a generic context interface. + +```tsx +// BAD — UI knows HOW state is managed +const state = useGlobalChannelState(channelId) + +// GOOD — provider owns implementation, UI uses context + + {/* reads from context */} + +``` + +### 4. State Trapped in Child Components (MEDIUM) + +Flag state that siblings or dialogs need but can't access without prop drilling or refs. Lift it into a provider. + +```tsx +// BAD — sibling can't access state +function ForwardComposer() { + const [state, setState] = useState(init) +} +// ForwardButton is a sibling and can't reach state + +// GOOD — provider at shared ancestor + + {/* can access state */} + {/* can also access state */} + +``` + +### 5. React 19 API Updates + +Flag `forwardRef` and `useContext` in new code — use `ref` as a regular prop and `use()` instead. + +```tsx +// BAD +const Input = forwardRef((props, ref) => ) +const value = useContext(MyContext) + +// GOOD +function Input({ ref, ...props }) { return } +const value = use(MyContext) +``` + +## Key Principle + +Lift state → Compose UI → Inject via generic context → No boolean prop proliferation. + +Canonical standard: `.claude/skills/vercel-composition-patterns/SKILL.md` diff --git a/.github/instructions/studio-e2e-tests.instructions.md b/.github/instructions/studio-e2e-tests.instructions.md new file mode 100644 index 00000000000..1b35fa0dc05 --- /dev/null +++ b/.github/instructions/studio-e2e-tests.instructions.md @@ -0,0 +1,79 @@ +--- +applyTo: "e2e/studio/**,apps/studio/**" +--- + +# Studio E2E Test Review Rules + +All comments are **advisory**. + +## Selector Priority (best to worst) + +1. **`getByRole` with accessible name** — most robust, tests accessibility + ```typescript + page.getByRole('button', { name: 'Save' }) + ``` + +2. **`getByTestId`** — stable, explicit test hooks + ```typescript + page.getByTestId('table-editor-side-panel') + ``` + +3. **`getByText` with exact match** — good for unique text + ```typescript + page.getByText('Data API Access', { exact: true }) + ``` + +4. **`locator` with CSS** — use sparingly, more fragile + ```typescript + page.locator('[data-state="open"]') + ``` + +## Patterns to Flag + +- **XPath selectors** — fragile to DOM changes + ```typescript + // BAD + locator('xpath=ancestor::div[contains(@class, "space-y")]') + ``` + +- **Parent traversal with `locator('..')`** — breaks when structure changes + ```typescript + // BAD + element.locator('..').getByRole('button') + ``` + +- **`waitForTimeout`** — never use; wait for something specific instead + ```typescript + // BAD + await page.waitForTimeout(1000) + + // GOOD — wait for UI element + await expect(page.getByText('Success')).toBeVisible() + + // GOOD — wait for API response + const apiPromise = waitForApiResponse(page, 'pg-meta', ref, 'query?key=table-create') + await saveButton.click() + await apiPromise + ``` + +- **`force: true` on clicks** — make elements visible first instead + ```typescript + // BAD + await menuButton.click({ force: true }) + + // GOOD — hover to reveal, then click + await tableRow.hover() + await expect(menuButton).toBeVisible() + await menuButton.click() + ``` + +- **Broad `filter({ hasText })` on generic elements** — may match multiple elements; scope to specific containers instead + +## Good Practices to Encourage + +- Scope selectors to containers: `page.getByTestId('side-panel').getByRole('switch')` +- Add `aria-label` to icon-only buttons in source code for better test selectors +- Use `test.describe.configure({ mode: 'serial' })` for tests sharing database state +- Add messages to expects: `await expect(locator, 'why').toBeVisible({ timeout: 30000 })` + +Canonical standard: `.claude/skills/e2e-studio-tests/SKILL.md` diff --git a/.github/instructions/studio-error-handling.instructions.md b/.github/instructions/studio-error-handling.instructions.md new file mode 100644 index 00000000000..bb4144e46f4 --- /dev/null +++ b/.github/instructions/studio-error-handling.instructions.md @@ -0,0 +1,43 @@ +--- +applyTo: "apps/studio/**" +--- + +# Studio Error Handling Review Rules + +All comments are **advisory**. + +## Architecture + +Errors flow: `handleError()` → throws typed subclass → React Query catches → `ErrorMatcher` reads `errorType` → renders troubleshooting. The component does an O(1) lookup — it never does regex matching. + +## When to Flag + +- PR passes `error.message` instead of the full `error` object to `ErrorMatcher` — the class type is lost +- PR puts regex patterns in `error-mappings.tsx` — they belong in `data/error-patterns.ts` +- PR uses `Object.assign` to stamp `errorType` on an error — should throw a proper subclass instead +- PR passes a raw URL string for support links — should use `supportFormParams={{ projectRef }}` +- PR puts the page title inside the error mapping — it belongs on the `` caller +- PR adds callback props (`onDebugWithAI`, `onRestartProject`) to troubleshooting components — use hooks inside them instead + +## Correct Usage + +```tsx +{isError && ( + +)} +``` + +## Key Files + +| File | Purpose | +|------|---------| +| `data/error-patterns.ts` | `{ pattern, ErrorClass }` array — regex lives here | +| `types/api-errors.ts` | Error classes, `KnownErrorType` union | +| `ErrorMatcher.tsx` | Reads `errorType`, looks up mapping, renders | +| `error-mappings.tsx` | `Record` | + +Canonical standard: `.claude/skills/studio-error-handling/SKILL.md` diff --git a/.github/instructions/studio-shadcn-components.instructions.md b/.github/instructions/studio-shadcn-components.instructions.md new file mode 100644 index 00000000000..ee61129adb0 --- /dev/null +++ b/.github/instructions/studio-shadcn-components.instructions.md @@ -0,0 +1,53 @@ +--- +applyTo: "apps/studio/**" +--- + +# shadcn/Radix UI Component Review Rules + +All comments are **advisory**. + +## Core Principle + +This project uses **shadcn/ui** components built on **Radix UI** primitives (from `packages/ui/`). These components provide comprehensive accessibility out-of-the-box. **Do not flag missing accessibility attributes that are already handled by the underlying Radix primitives.** + +## Components with Built-In Accessibility — Do NOT Flag + +The following components (imported from `ui`) already handle ARIA roles, keyboard navigation, focus management, and screen reader support automatically via Radix UI primitives: + +| Component | What Radix Handles | +|-----------|-------------------| +| `Dialog`, `AlertDialog` | `role="dialog"`, `aria-modal`, focus trapping, ESC to close | +| `DropdownMenu`, `ContextMenu` | `role="menu"` / `role="menuitem"`, arrow key navigation | +| `Select` | `role="combobox"`, `aria-expanded`, keyboard selection | +| `Tabs` | `role="tablist"` / `role="tab"` / `role="tabpanel"`, `aria-selected`, arrow keys | +| `Checkbox` | `role="checkbox"`, `aria-checked`, Space to toggle | +| `RadioGroup` | `role="radio"`, `aria-checked`, arrow key navigation | +| `Switch` | `role="switch"`, `aria-checked`, keyboard toggle | +| `Tooltip` | Trigger/content association, show/hide timing | +| `Accordion`, `Collapsible` | `aria-expanded`, Enter/Space to toggle | +| `Popover`, `HoverCard` | Focus management, dismiss on ESC | +| `Slider` | `role="slider"`, `aria-valuemin/max/now`, arrow keys | +| `Toggle`, `ToggleGroup` | `aria-pressed`, keyboard support | +| `ScrollArea` | Accessible scrollbar replacement | +| `NavigationMenu` | `role="navigation"`, keyboard navigation | + +### Specifically, Never Flag These + +- Missing `role` on `Dialog`, `AlertDialog`, `DropdownMenu`, `Select`, `Tabs`, `RadioGroup`, or other Radix-based components — roles are set by the primitive +- Missing `aria-modal` on `Dialog` or `AlertDialog` — set automatically +- Missing `aria-expanded` on `Accordion`, `Collapsible`, `Select`, or `DropdownMenu` triggers — managed by Radix state +- Missing `aria-selected` on `Tabs` — managed by `TabsPrimitive` +- Missing `aria-checked` on `Checkbox`, `RadioGroup`, or `Switch` — managed by Radix state +- Missing keyboard event handlers (`onKeyDown`, `onKeyUp`) on interactive Radix components — keyboard support is built-in +- Missing focus management in `Dialog` or `AlertDialog` — focus trapping is automatic +- Missing `aria-label` on `DialogClose` or `AlertDialogCancel` — these render a visible `Close` + +## What TO Flag + +Only flag accessibility issues for: + +1. **Custom interactive elements** not using Radix primitives (e.g., a `
` that should be a `