diff --git a/.claude/commands/pr-review.md b/.claude/commands/pr-review.md index 80290a59..1f569344 100644 --- a/.claude/commands/pr-review.md +++ b/.claude/commands/pr-review.md @@ -1,175 +1,85 @@ --- -description: Review a PR (or current branch's diff against main) against kaapi-frontend conventions and personal review checklist. +description: Review a PR (or current branch's diff against main) against kaapi-frontend conventions. argument-hint: [PR number | "branch"] +allowed-tools: Bash(gh pr view:*), Bash(gh pr diff:*), Bash(gh pr list:*), Bash(git diff:*), Bash(git log:*), Read, Grep, Glob --- -You are reviewing a pull request (or the current branch's diff against `main`) in this Next.js 16 (App Router) + React 19 + TypeScript (strict) + Tailwind CSS 4 frontend, with a BFF layer in `app/api/*` that proxies to the FastAPI backend. +You are reviewing a pull request (or the current branch's diff against `main`) in this Next.js 16 (App Router) + React 19 + TypeScript + Tailwind CSS 4 frontend, with a BFF layer in `app/api/*` that proxies to the FastAPI backend. Argument from the user: `$ARGUMENTS` +> **Scope of this command — kaapi-specific only.** For _generic_ review depth (correctness bugs, perf, async/race conditions, a11y, generic security), lean on the `superpowers` code-review skills and the built-in `/code-review`. This checklist covers only what those can't know: kaapi-frontend's conventions, file layout, and contracts. Run them alongside each other; don't duplicate the generic findings here. + ## Gather the diff 1. Argument is a PR number → `gh pr view ` + `gh pr diff `. 2. Argument is empty → `gh pr list` and ask which one. Exception: argument is "branch" / "this branch" / "my changes" → `git diff main...HEAD` + `git log main..HEAD --oneline`. -3. `Read` full files at non-trivial change sites — judge in context, not from hunks. Components hide bugs in the parts of the file the diff didn't touch (effects, memo deps, conditional renders). +3. `Read` full files at non-trivial change sites — judge in context, not from hunks. Bugs hide in the parts of the file the diff didn't touch (effects, memo deps, conditional renders). 4. `Grep` for duplication, reused literals, unused symbols, near-identical components/hooks. -## What to check +## What to check (kaapi conventions) -Skip any section in the output that has nothing notable. +The authoritative rules live in `.claude/rules/*` and `CLAUDE.md` — they auto-load by path. Flag any drift from them. Summary of the highest-signal checks: -### Conventions +### Conventions & structure -- File size: **no file over 500 LOC** (per `CLAUDE.md`). If a change pushes a component past it, push to split — extract subcomponents / hooks / utils / types into their own files. -- Single Responsibility: a component that fetches, transforms, and renders is doing three things. Data fetching → custom hook in `app/hooks/`. Business logic → util in `app/lib/utils//`. Component stays presentational. -- Import alias: use `@/...` (from project root), not relative `../../...` chains. tsconfig paths set `@/*` → `./*`. -- `"use client"` directive only where actually needed (state, effects, event handlers, browser APIs). Server components by default in App Router; flag client directives that wrap pure markup. -- Tailwind first; reach for `app/globals.css` only when Tailwind can't express it. Inline `style={{ ... }}` is a smell — flag it unless the value is genuinely dynamic (computed pixel offset, runtime color from data). -- Color tokens via the design system (`text-text-primary`, `bg-accent-primary`, `border-border`, etc.) — not raw `text-gray-500` / hex literals. Cross-check against `Button.tsx` / `globals.css` for the canonical palette. -- TypeScript strict: no `any` (use `unknown` + narrowing, or a real type). `// eslint-disable-next-line @typescript-eslint/no-explicit-any` needs a justifying comment, not a silent suppression. -- Type hints on every prop / return. Prefer `interface` for public component props, `type` for unions / utility shapes — match the file's surroundings. -- `npm` is the runner; lockfile is `package-lock.json`. Flag stray `yarn.lock` / `pnpm-lock.yaml` / `bun.lockb` changes. +- **File size**: no file over **500 LOC** (`max-lines` ESLint error). A change pushing a component past it must split — subcomponents/hooks/utils/types into their own files. See `.claude/rules/code-quality.md`. +- **SRP**: a component that fetches + transforms + renders is doing three things. Fetching → hook in `app/hooks/`; logic → util in `app/lib/utils//`; component stays presentational. +- **Imports**: `@/...` alias, never relative `../../..` chains. +- **`"use client"`** only where state/effects/handlers/browser APIs are actually used — flag it on pure markup. -### Reuse (DRY) — grep before approving +### Styling (`.claude/rules/styling.md`) -- New component: check `app/components/` first. `Button`, `Modal`, `Field`, `MultiSelect`, `Toast`, `Loader`, `PageHeader`, `InfoTooltip`, `CodeBlock`, `ErrorModal`, `GatePopover` already exist. Composing one beats authoring a new one. -- New icon: **must** live in `app/components/icons/` as a hand-authored React component, exported from `app/components/icons/index.ts`. Inline SVGs in feature code are a blocker — name the existing icon if there is one. -- New hook: check `app/hooks/` (`useToast`, `useConfigs`, `useCollections`, `useInfiniteScroll`, `usePaginatedList`, `useConfigPersistence`, `useSttData`). -- New util: check `app/lib/utils.ts`, `app/lib/utils//`, `app/lib/promptEditorUtils.ts`, `app/lib/configFetchers.ts`. Domain-specific helpers belong under `app/lib/utils//`, not feature folders. -- New type: shared types live in `app/lib/types/` and `app/lib/models.ts` — flag locally redefined shapes that duplicate an existing one. -- Constants: `app/lib/constants.ts`. Storage keys go in `STORAGE_KEYS`; events use a `kaapi:*` namespace (see `AUTH_EXPIRED_EVENT`, `CACHE_INVALIDATED_EVENT`). -- Three near-identical components or three copy-pasted fetchers usually collapse into one parameterized version. +- Tailwind first; `app/globals.css` only for what Tailwind can't express. Inline `style={{}}` only for genuinely dynamic values. +- Design tokens (`text-text-primary`, `bg-accent-primary`, `border-border`, status colors) — **not** raw `text-gray-500` / hex. +- **No `cn()` / `clsx()`** — compose with template literals + ternaries and variant `Record`s. -### App Router & routing +### Reuse / DRY — grep before approving (`.claude/rules/*`) -- New routes land under the correct group: `app/(auth)/` (unauthenticated) or `app/(main)/` (authenticated app surface). Don't put authenticated features at the root. -- Public, guest-only, and superuser-only routes must be reflected in `middleware.ts` (`PUBLIC_ROUTES`, `GUEST_ONLY_ROUTES`, `PATHNAME_STARTS_WITH`). Adding a `/settings/*` page without confirming superuser gating is a security issue. -- `loading.tsx` / `error.tsx` / `not-found.tsx` for non-trivial route segments where they help UX. -- Dynamic segments use `[id]` (not `[slug]` unless it's actually a slug). Params typed; `params` is async in Next 15+/16 (`params: Promise<{ id: string }>`). -- `metadata` / `generateMetadata` set for top-level pages; don't ship `Untitled`. +- New component → check `app/components/` (`Button`, `Modal`, `Field`, `MultiSelect`, `Toast`, `Loader`, `PageHeader`, `InfoTooltip`, `CodeBlock`, `ErrorModal`, `GatePopover`). +- New icon → **must** be a hand-authored React component in `app/components/icons/` (domain subfolder + barrel export). **Inline SVG in feature code is a blocker.** +- New hook → check `app/hooks/` (`useToast`, `useConfigs`, `useCollections`, `useInfiniteScroll`, `usePaginatedList`, `useConfigPersistence`). +- New util → check `app/lib/utils.ts`, `app/lib/utils//`. New type → `app/lib/types/` / `app/lib/models.ts` (flag locally redefined shapes). +- Constants → `app/lib/constants.ts`. Storage keys via `STORAGE_KEYS`; events in the `kaapi:*` namespace. -### BFF / API route handlers (`app/api/*`) +### App Router & routing -- Use `apiClient(request, endpoint, options)` from `@/app/lib/apiClient` — never raw `fetch(BACKEND_URL...)`. The client relays `X-API-KEY` + `Cookie` automatically. -- Route handlers stay thin: parse → call `apiClient` → relay. Business logic does not belong here. -- Endpoint paths preserved verbatim — the recent "remove trailing slashes" migration is a contract with the backend; flag drift in either direction. -- Error response shape: `{ error, details }` with the original status when possible (see `app/api/evaluations/route.ts` for the template). Don't swallow non-2xx into 200. -- Don't log request bodies, tokens, or cookies. `console.error("Proxy error:", error)` is the existing convention — match it. -- New route handler → confirm the equivalent backend endpoint exists; don't ship a 404 proxy. +- New routes land in the correct group: `app/(auth)/` (unauthenticated) or `app/(main)/` (authenticated). +- **Route gating reflected in `middleware.ts`** (`PUBLIC_ROUTES`, `GUEST_ONLY_ROUTES`, superuser-only `/settings/*`). Adding a `/settings/*` page without confirming superuser gating is a security issue. +- `params` is async in Next 15+/16 (`params: Promise<{ id: string }>`). Dynamic segments `[id]`. -### Client-side data fetching +### BFF / API route handlers (`app/api/*`) — `.claude/rules/data-fetching.md` -- Use `clientFetch(endpoint, options)` for browser calls so 401 refresh + `AUTH_EXPIRED_EVENT` are wired in. Raw `fetch("/api/...")` in a component bypasses this and is a bug. -- SWR is used selectively — adopt it for _cached, revalidated_ reads (lists, dashboards). One-shot mutations stay on `clientFetch`. Don't sprinkle `useSWR` everywhere "for parity." -- Error messages surfaced to users come from `extractErrorMessage` (reads `error || message || detail`) — don't reinvent the parser. -- Loading / error / empty states: all three must be handled. A list that renders `[]` silently when the API fails is broken UX. +- Use `apiClient(request, endpoint, options)` (or `guardrailsClient`) — never raw `fetch(BACKEND_URL...)`. Route handlers stay thin: parse → call client → relay. +- Forward backend `status` through (`NextResponse.json(data, { status })`); don't swallow non-2xx into 200. Endpoint paths preserved verbatim (trailing-slash contract). +- Don't log request bodies/tokens/cookies; `console.error("Proxy error:", error)` is the convention. -### React 19 / Hooks +### Client-side data fetching -- Hook rules: top-level only, no calls inside conditionals or loops. Flag any custom hook that doesn't start with `use`. -- `useEffect` dependency arrays: every referenced value listed, or document why it's intentionally omitted. Stale-closure bugs are easy to miss in review — read the effect body and trace each identifier. -- `useEffect` for derivation is an antipattern — compute during render or with `useMemo`. Effects are for syncing with external systems. -- `useCallback` / `useMemo` only when there's a real perf reason (passed to memoized children, expensive computation). Wrapping every function is noise. -- Cleanup functions on effects that start timers, subscribe, or fetch (abort on unmount via `AbortController`). -- `key` props on list items: stable, unique, **not** the array index when the list reorders / filters. -- State shape: derived state is a bug magnet — if `selected` can be computed from `items` + `selectedId`, don't store both. +- Browser calls use `clientFetch` / `apiFetch` so 401 refresh + `AUTH_EXPIRED_EVENT` are wired in. Raw `fetch("/api/...")` in a component is a bug. +- User-facing error messages come from `extractErrorMessage` (`error || message || detail`) — don't reinvent the parser. +- SWR is used selectively (cached, revalidated reads). Don't sprinkle `useSWR` everywhere. +- Loading / error / empty states all handled — a list that renders `[]` on API failure is broken UX. ### Context & global state -- Auth → `useAuth()` from `@/app/lib/context/AuthContext`. App / sidebar → `useApp()` from `@/app/lib/context/AppContext`. Toasts → `useToast()` from `@/app/hooks/useToast` (or `@/app/components/Toast`). Don't reach into the cookies / `localStorage` directly when a context exposes the same data. -- Zustand stores live in `app/lib/store/` — use one when state is shared across distant components and prop-drilling is painful. Don't introduce a new store for what context already covers. -- `localStorage` access goes through `STORAGE_KEYS` in `constants.ts`, never bare strings. - -### Forms & validation - -- Validation messages user-facing — say what's wrong, not "Invalid input". Required-field errors flagged inline near the field. -- Disable submit while a request is in flight; don't let users double-click into duplicate requests. -- Controlled inputs: every `value` paired with `onChange`. Mixed controlled/uncontrolled (`value={x ?? undefined}`) produces React warnings — flag it. - -### Accessibility - -- Buttons are `