diff --git a/.agents/skills/deep-review/roles/frontend-reviewer.md b/.agents/skills/deep-review/roles/frontend-reviewer.md index 96d7e9104b8..b54d0c173ad 100644 --- a/.agents/skills/deep-review/roles/frontend-reviewer.md +++ b/.agents/skills/deep-review/roles/frontend-reviewer.md @@ -6,6 +6,7 @@ - Map every user-visible state: loading, polling, error, empty, abandoned, and the transitions between them. Find the gaps. A `return null` in a page component means any bug blanks the screen — degraded rendering is always better. Form state that vanishes on navigation is a lost route. - Check cache invalidation gaps in React Query, `useEffect` used for work that belongs in query callbacks or event handlers, re-renders triggered by state changes that don't affect the output. +- Audit the diff against the FE rule contract in `.claude/docs/FRONTEND_PATTERNS.md` (FE1 to FE10) and cite rule IDs in findings. - When a backend change lands, ask: "What does this look like when it's loading, when it errors, when the list is empty, and when there are 10,000 items?" **Scope boundaries:** You review frontend code. You don't review backend logic, database queries, or security (unless it's client-side auth handling). diff --git a/.agents/skills/frontend-review b/.agents/skills/frontend-review new file mode 120000 index 00000000000..dc4e487b48c --- /dev/null +++ b/.agents/skills/frontend-review @@ -0,0 +1 @@ +../../.claude/skills/frontend-review \ No newline at end of file diff --git a/.claude/skills/code-review/SKILL.md b/.claude/skills/code-review/SKILL.md index 96036cfc3a3..4624d8501ca 100644 --- a/.claude/skills/code-review/SKILL.md +++ b/.claude/skills/code-review/SKILL.md @@ -38,6 +38,9 @@ quality problems. - **Concurrency**: Race conditions, deadlocks, missing synchronization - **Resources**: Leaks, unclosed handles, missing cleanup - **Error handling**: Swallowed errors, missing validation, panic paths +- **Frontend** (`site/src/`): audit against the FE rule IDs in + [Frontend Patterns](../../docs/FRONTEND_PATTERNS.md) and cite the rule ID + in findings (for example, "FE7: re-typed query key") ## What NOT to Comment On diff --git a/.claude/skills/frontend-review/SKILL.md b/.claude/skills/frontend-review/SKILL.md new file mode 100644 index 00000000000..6da33f37b87 --- /dev/null +++ b/.claude/skills/frontend-review/SKILL.md @@ -0,0 +1,94 @@ +--- +name: frontend-review +description: Diff-scoped self-review of frontend changes under site/src against the FE pattern rules (FE1-FE10) before creating or updating a PR. Use whenever a branch diff touches site/src files. +--- + +# Frontend Review + +Audit the current branch diff against the frontend rule contract in +[`.claude/docs/FRONTEND_PATTERNS.md`](../../docs/FRONTEND_PATTERNS.md) and fix +every violation before the PR is created or updated. This is a self-review +gate: its purpose is to catch the findings reviewers would otherwise post, +before they see the PR. + +## When to run + +- Before creating a PR whose diff touches `site/src/`. +- Before pushing significant new commits to an existing frontend PR. +- Skip only when the diff touches no files under `site/src/`. + +## Workflow + +1. Collect the diff: `git diff --merge-base main -- site/src` (add + `site/e2e` if touched). Use `origin/main` instead when the checkout has an + `origin` remote and the local `main` may be stale. List the changed files. +2. For each changed file, audit against each FE rule using the checklist + below. Read the full file when the diff alone cannot answer a check (for + example, whether a story's `play` function exercises the new behavior). +3. Report results as a per-rule verdict table (see Output format). Every FAIL + must carry `file:line` and a one-line reason. +4. Fix all FAIL findings with the smallest safe diff. Re-run the audit until + every rule passes or a remaining finding is explicitly justified. +5. Include unresolved justifications in the PR description so reviewers see + them up front. + +## Per-rule diff checklist + +- **FE1 (Storybook coverage)**: Does any changed component or page alter + user-visible behavior? Then a changed or added `.stories.tsx` must exist, + and its `play` function must perform the new interaction (open the menu, + submit the form), not merely render. Interaction tests added to `.test.tsx` + files are a FAIL unless they cover pure logic. +- **FE2 (types)**: Search the diff for `any`, `as unknown as`, non-null + assertions in any form (`x!.y`, `items[0]!`, `fn()!`, `value! as T`), and + new `as` casts. Check that API data uses types from `api/typesGenerated.ts`. +- **FE3 (reuse/scope)**: For each new component, hook, or helper, search + `site/src/components/` and sibling folders for an existing equivalent. + Flag near-duplicates, hand-assembled versions of wrapped primitives, dead + branches, and unrelated changes bundled into the diff. +- **FE4 (comments)**: Read every comment line the diff adds or edits. Flag + any comment that restates the identifier, assertion, or control flow. + Verify surviving comments are factually correct. +- **FE5 (UI states)**: For each view rendering server data, confirm loading, + error, empty, and refetch handling. Flag form or selection state that a + background refetch would reset. +- **FE6 (a11y)**: Flag interactive elements that are keyboard-unreachable, + `aria-label`s that replace visible label text, `aria-*` props that the + underlying primitive overwrites, and visually-hidden elements still in the + tab order. +- **FE7 (react-query)**: Flag direct `API.*`/`fetch` calls in components, + string-literal query keys (must import the constant from `api/queries/`), + `isLoading || isFetching` patterns, missing invalidation on mutation paths + (including partial failure), and `mutateAsync` in `try/catch` with an empty + catch. +- **FE8 (effects)**: For every added or modified `useEffect`, apply the + decision tree in FRONTEND_PATTERNS.md. Flag derived state via + `setState`-in-effect, fetches triggered by effects, new dependencies on + effects that own connections, and effects that only write refs nobody + reads. +- **FE9 (fixtures)**: Flag inline entity literals that duplicate or deviate + from `Mock*` fixtures in `site/src/testHelpers/`, and shared pre-wired + query objects instead of per-story inline `{ key, data }` wiring. +- **FE10 (test queries)**: Flag `querySelector`, class-name substring + matches, geometry assertions, `behavior: "smooth"` dependence, and + locale-less `toLocaleString()` in changed tests and stories. + +## Output format + +``` +FE1 PASS +FE2 FAIL site/src/pages/FooPage/FooPage.tsx:42 `as unknown as Workspace` cast +FE3 PASS +... +``` + +One line per rule. FAIL lines carry every finding (repeat the rule ID for +multiple findings). After fixes, print the re-run table. The audit is done +when all rules PASS or remaining FAILs have a written justification. + +## Notes + +- This audit does not replace `pnpm check`, `pnpm lint`, `pnpm format`, or + tests; run those too (see site/AGENTS.md Pre-PR Checklist). +- Report findings in the current diff only. Do not refactor pre-existing + violations in untouched code; note them at most. diff --git a/site/AGENTS.md b/site/AGENTS.md index 610553655f4..915674b3c20 100644 --- a/site/AGENTS.md +++ b/site/AGENTS.md @@ -273,6 +273,11 @@ Debug logs and pprof dumps use the same job name and commit SHA convention. 3. `pnpm format` - Format code consistently 4. `pnpm test` - Run affected unit tests 5. Visual check in Storybook if component changes +6. If the diff touches `site/src/`, run the `frontend-review` skill + (discovered from `.agents/skills/`; canonical copy at + [.claude/skills/frontend-review](../.claude/skills/frontend-review/SKILL.md)): + audit the diff against FE1 to FE10 and fix every FAIL before creating + the PR ## Migration (MUI → shadcn) (Emotion → Tailwind)