mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 09:25:06 +03:00
chore: updated copilot instructions (#44247)
## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? Improve code review guidelines for copilot --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Ali WaseemandCopilot authored and GitHub committed
1 parent
5e6e2ec0c1
commit
3ece134d52
5 files changed
+310
-9
No files matched your search
@@ -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.
|
||||
@@ -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
|
||||
<Composer isThread isDMThread isEditing isForwarding={false} />
|
||||
|
||||
// GOOD — self-documenting variants
|
||||
<ThreadComposer channelId="abc" />
|
||||
<EditMessageComposer messageId="xyz" />
|
||||
```
|
||||
|
||||
### 2. Render Props Instead of Children (MEDIUM)
|
||||
|
||||
Flag `renderX` callback props when `children` composition would work.
|
||||
|
||||
```tsx
|
||||
// BAD — render prop for structure
|
||||
<Composer renderFooter={() => <F />} />
|
||||
|
||||
// GOOD — compound component
|
||||
<Composer.Footer>
|
||||
<Composer.Formatting />
|
||||
<Composer.Emojis />
|
||||
</Composer.Footer>
|
||||
```
|
||||
|
||||
### 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
|
||||
<ChannelProvider channelId={channelId}>
|
||||
<Composer /> {/* reads from context */}
|
||||
</ChannelProvider>
|
||||
```
|
||||
|
||||
### 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
|
||||
<ForwardMessageProvider>
|
||||
<Composer /> {/* can access state */}
|
||||
<ForwardButton /> {/* can also access state */}
|
||||
</ForwardMessageProvider>
|
||||
```
|
||||
|
||||
### 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) => <input ref={ref} />)
|
||||
const value = useContext(MyContext)
|
||||
|
||||
// GOOD
|
||||
function Input({ ref, ...props }) { return <input ref={ref} /> }
|
||||
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`
|
||||
@@ -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`
|
||||
@@ -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 `<ErrorMatcher>` caller
|
||||
- PR adds callback props (`onDebugWithAI`, `onRestartProject`) to troubleshooting components — use hooks inside them instead
|
||||
|
||||
## Correct Usage
|
||||
|
||||
```tsx
|
||||
{isError && (
|
||||
<ErrorMatcher
|
||||
title="Failed to load tables"
|
||||
error={error}
|
||||
supportFormParams={{ projectRef }}
|
||||
/>
|
||||
)}
|
||||
```
|
||||
|
||||
## 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<KnownErrorType, { id, Troubleshooting }>` |
|
||||
|
||||
Canonical standard: `.claude/skills/studio-error-handling/SKILL.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 `<span className="sr-only">Close</span>`
|
||||
|
||||
## What TO Flag
|
||||
|
||||
Only flag accessibility issues for:
|
||||
|
||||
1. **Custom interactive elements** not using Radix primitives (e.g., a `<div onClick>` that should be a `<button>`)
|
||||
2. **Icon-only buttons** missing an accessible label — `<Button>` alone does not add one; use `aria-label` or `<span className="sr-only">`
|
||||
3. **Missing `Label` association** — form inputs should be paired with `<Label htmlFor="...">` or wrapped in a `<Field>` component
|
||||
4. **Images missing `alt` text** — not handled by any component library
|
||||
5. **Color-only state indicators** — state changes should not rely solely on color
|
||||
Reference in new issue
Block a user