Files
Alaister YoungandAlaister Young d845768fcf chore(claude): add react-hook-form skill (#48431)
Adds a Claude skill encoding correct React Hook Form usage, so
AI-written form code follows best practices instead of copying the
anti-patterns common in older Studio code (prop-form
`form.watch()`/`formState` subscriptions, subscription-only watches,
unguarded `valueAsNumber`, `?? undefined` controlled values, defaults
computed from unloaded queries).

**Added:**
- `.claude/skills/react-hook-form/SKILL.md` — subscription model
(`useWatch`/`useFormState` with `control`), canonical zod + `FormField`
composition (layout deferred to `studio-ui-patterns`), `values:` option
for async data, null normalization for controlled inputs, number-input
handling, dirty-state and gating rules, plus a fix-what-you-touch policy
aligned with the `no-use-watch` lint ratchet

**Changed:**
- `.claude/CLAUDE.md` and `apps/studio/CLAUDE.md` — register the skill
in the skill lists/table
- `.coderabbit.yaml` — add the skill to the existing Studio
code-guidelines entry so CodeRabbit applies it when reviewing Studio
code

Benchmarked on three real form tasks (adding a live-updating field to
`ThroughputField`, a new sheet form with async + nullable data, a
review-changes step in `EditBucketModal`), each run with and without the
skill: 13/13 assertions with the skill vs 8/13 baseline. The baseline
shipped a genuine bug in one task — a `null` server default flowed into
a `''` its own schema rejected, making Save unreachable — which the
skill run avoided.

## To test

- Ask Claude Code to add a field to any Studio form and check it loads
the skill (it's in the studio CLAUDE.md skill table) and uses
`useWatch({ control, name })` rather than `form.watch`
- Skim `SKILL.md` for anything that contradicts current form conventions
— `apps/design-system` demos remain the layout source of truth

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Documentation**
* Added a new monorepo “react-hook-form” skill guide with recommended
patterns for safe form subscriptions, wiring, default values,
reset/submission flows, and common anti-patterns.
* Updated Studio skills/load guidance to expand and reorder the skills
matrix, including form logic and copywriting guidance.
* Updated required skill coverage so `react-hook-form` is included for
any form-related work.
* **Chores**
* Expanded automated review enforcement so Studio form code is checked
against the new “react-hook-form” skill guidance.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Alaister Young <10985857+alaister@users.noreply.github.com>
2026-07-29 16:52:03 +08:00

87 lines
9.7 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Supabase Studio
Next.js pages router + TanStack Start (mid-migration, see below), React 19. Dev server: `pnpm dev:studio` → http://localhost:8082.
## Skills — load before working
Load the skills matching the task; stack them when a task spans areas:
| Task | Additional skills |
| ------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------- |
| Query/mutation hooks, query keys (`data/**`) | `studio-queries` |
| UI: pages, forms, tables, charts, sheets, empty states | `studio-ui-patterns` |
| Form logic: react-hook-form fields, watch/formState, reset, number inputs | `react-hook-form` |
| Displaying API errors | `studio-error-handling` |
| Tests (deciding, writing, reviewing) | `studio-testing`, then `studio-mock-api-tests` (component/MSW) or `studio-e2e-tests` (Playwright) |
| PostHog event tracking | `telemetry-standards` |
| SQL against user databases | `safe-sql-execution` |
| Logs Explorer SQL, `data/logs` | `clickhouse-logs-queries` |
| Component API design, boolean-prop refactors | `vercel-composition-patterns` |
| User-facing copy | `copywriting` |
## TanStack Start migration
Studio is migrating from the Next.js pages router (`pages/**`) to TanStack Start (`routes/**`). Both runtimes ship side-by-side; the `STUDIO_FRAMEWORK` env var selects which one `pnpm dev`/`build` runs (default: `next`, resolved in `scripts/dispatch.js`). Full route map and strategy: `TANSTACK_MIGRATION.md`.
- **Never delete a page file.** Most `routes/**` files are thin wrappers re-exporting the default export of their `pages/**` counterpart, so the Next file is load-bearing for both runtimes until the final cleanup pass.
- Pure page-body edits propagate to the route automatically. Mirror a change by hand into the corresponding `routes/**` file only when it touches what the route duplicates: `getLayout`/layout wrapping, page titles or other `staticData` (incl. `skip*Layout` flags), `withAuth`, or redirect paths.
- A new page under `pages/**` needs a matching route under `routes/**` plus a checklist entry in `TANSTACK_MIGRATION.md`.
- New code uses native TanStack APIs — no `next/router` or `next/link`. The `compat/next/` shims exist only for legacy re-exported pages.
- `routeTree.gen.ts` is generated by the Vite plugin — never hand-edit.
## Orientation
- **Data layer** — all platform API calls go through `data/fetchers.ts` (`openapi-fetch`, typed by the generated `api-types` package) with `handleError`; never raw `fetch`. One folder per resource in `data/`, most with a `keys.ts` query-key factory.
- **State** — valtio for global state (`state/`), nuqs for URL state, react-hook-form + zod for forms.
- **Platform vs self-hosted** — `IS_PLATFORM` gates platform-only behavior; `withAuth` is a no-op when self-hosted.
- **Telemetry** — `useTrack()` from `lib/telemetry/track`; event types live in `packages/common/telemetry-constants.ts`.
- **Tests** — default to including relevant tests with any change: a couple of unit tests for extracted logic, component tests for UI behavior, E2E only when the scope demands it (`studio-testing` has the decision tree). Not every PR needs them, but "no tests" should be a considered choice, not the default. Tooling: vitest + MSW; component tests use `customRender` + `addAPIMock` from `tests/lib/`; unhandled network requests fail tests. Don't `vi.mock('@/data/...')`.
- **Shortcuts** — use the registry in `state/shortcuts/` and `components/ui/Shortcut*.tsx`; keep `G then …` chords for navigation; no one-off keyboard listeners.
- **Reuse first** — before writing a new hook or helper, search for an existing one (`hooks/`, `lib/`, `packages/common`, `packages/ui-patterns`). If you do need a new one, make it as reusable as possible: general naming, no page-specific coupling, placed where other callers can find it.
- Co-locate sub-components with their parent; avoid barrel re-export files.
## Code style
Older Studio code predates some of these conventions. For new or modified code, follow them rather than mirroring nearby legacy patterns:
- **Booleans** read as `is`/`has`/`can`/`should`. Derive them from existing state (`const isFormValid = name.length > 0 && email.includes('@')`) — mirroring a derivable value into `useState` synced by `useEffect` is a bug pattern. Give multi-condition logic a name (`const canShowAddButton = !isSchemaLocked && canUpdateColumns && …`) instead of inlining the chain in JSX.
- **Ternaries**: one is fine for a binary choice; never nest them. Anything bigger flattens — early returns in statement position, sibling `&&` blocks in JSX.
- **Fetch states** render with early returns at the top level, or a flat `&&` chain with mutually exclusive guards inline — never a nested ternary:
```tsx
// Top level: early return per state
if (isLoading) return <GenericSkeletonLoader />
if (isError) return <AlertError error={error} subject="Failed to retrieve data" />
if (isSuccess && data.length === 0) return <EmptyState />
return <DataDisplay data={data} />
// Inline: flat `&&` blocks, mutually exclusive guards
<div>
{isLoading && <ShimmeringLoader />}
{isError && <AlertError error={error} />}
{isSuccess && data.length === 0 && <EmptyState />}
{isSuccess && data.length > 0 && <DataDisplay data={data} />}
</div>
```
- **`useEffect` is for synchronizing with external systems** (subscriptions, DOM, timers) — not for deriving data (compute it in render), reacting to user actions (do it in the handler), or fetching (React Query). Older code uses effects for all of these; don't copy it.
- **State** stays as local as possible — lift it only when it's actually shared. Related form fields belong in a single react-hook-form + zod form, not parallel `useState` calls.
- **Component size**: split at ~200–300 lines — or sooner when a component grows multiple distinct UI sections, tangled conditional rendering, or clusters of unrelated `useState`. Extract repeated JSX into small components, non-trivial pure logic into `.utils.ts` functions (which get unit tests), and reusable stateful logic into custom hooks.
- **Memoization is not the default**: `useMemo`/`useCallback` only for measured expense or referential stability a memoized child depends on.
- **TypeScript**: avoid `as` casts — where external data enters, parse it with zod (`schema.parse`/`safeParse`) instead. Model multi-state values as discriminated unions (`{ status: 'success'; data: T } | { status: 'error'; error: Error }`) rather than independent boolean flags.
- **Naming**: prop callbacks are `onX`, internal handlers are `handleX`. Custom hooks return objects, not tuples.
- **Refactoring**: when you move or extract code into a new module, update every importer to point at the new location directly — do **not** leave a re-export shim in the old file "for backward compatibility." It's a one-line import change per consumer, and keeping shims around makes the codebase messy and the true source of a symbol ambiguous.
## Defaults that differ here
- **ESLint warnings are ratcheted in CI**: the per-rule occurrence count must not increase, so a new `any`, unresolved `exhaustive-deps` warning, or default export fails the build even though it's "only a warning". Check locally with `pnpm --filter studio run lint:ratchet`.
- **Clipboard**: `copyToClipboard` from `'ui'`, and never `await` anything before calling it (Safari requires the write inside the user gesture; lint-enforced) — pass a Promise as the argument instead.
- **`useParams()` comes from `'common'`**, not `next/navigation` — it camelCases keys and returns `string | undefined`.
- **Permissions**: `useAsyncCheckPermissions` from `hooks/misc/useCheckPermissions` (returns `can: true` when self-hosted).
- **Gating**: `useIsFeatureEnabled` for product features, `useFlag` from `'common'` for feature flags — two different systems.
- **Dates**: `dayjs` (plugins pre-loaded at both entries, `pages/_app.tsx` and `routes/__root.tsx`), not `date-fns`. **Toasts**: `toast` from `'sonner'`.
- **Import split**: `'ui'` = primitives, `'ui-patterns'` = composed patterns (`ConfirmationModal`, …), `@ui/*` = alias into `packages/ui/src`. Icons come from `lucide-react`.
- **New tables** use `@tanstack/react-table`; `react-data-grid` is banned for new code.
- **Ad-hoc SQL** against the user's database goes through `executeSql` / `useExecuteSqlMutation` (`data/sql/execute-sql-mutation`).
- **Confirmations**: `ConfirmationModal` / `TextConfirmModal` from `ui-patterns`, never `window.confirm`. Disabled buttons needing an explanation use `ButtonTooltip`; inline warnings use `Admonition`.