diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 5da940f7ca..ee3e1c41ad 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -3,8 +3,10 @@ name: code-review description: >- Senior engineer code review focused on catching issues before they become PR comments. Reviews only changed lines, categorizes issues by priority, and fixes - them one by one. Use when the user says "code review", "review my code", - "review this branch", or wants pre-PR feedback. + them one by one. Includes a focused ui/goose2 refactor-quality pass for + maintainability, decomposition, layering, type hygiene, duplication, and dead + code. Use when the user says "code review", "review my code", "review this + branch", or wants pre-PR feedback. --- # Pre-PR Code Review @@ -37,17 +39,24 @@ You are a senior engineer conducting a thorough code review. Review **only the l **Only proceed with the review once you have the specific list of files to review.** -## Review Checklist +## Review Passes -### React Best Practices +Run these as passes, then consolidate findings before presenting them. A finding should appear once, even if multiple sections support it. + +- Use the baseline pass for correctness, regressions, async state, API/backend contracts, accessibility, i18n completeness, CI failures, and obvious cleanup. +- For `ui/goose2` maintainability, use `UI Refactor Quality` as the authoritative pass for decomposition, layering, hooks vs helpers, type hygiene, duplication, naming, module boundaries, and refactor structure. +- Do not duplicate the same underlying concern across the baseline pass and the `UI Refactor Quality` pass. Prefer the `UI Refactor Quality` framing for `ui/goose2` maintainability issues. + +### Baseline Safety Pass + +#### React Best Practices - **Components**: Are functional components with hooks used consistently? - **State Management**: Is `useState` and `useEffect` used properly? Any unnecessary re-renders? - **Props**: Are prop types properly defined with TypeScript interfaces? - **Keys**: Are list items using proper unique keys (not array indices)? - **Hooks Rules**: Are hooks called at the top level and in the correct order? -- **Custom Hooks**: Could any logic be extracted into reusable custom hooks? -### TypeScript Best Practices +#### TypeScript Best Practices - **const vs let vs var**: Is `const` used by default? Is `let` only used when reassignment is needed? Is `var` avoided entirely? - **Type Safety**: Are types explicit and avoiding `any`? Are proper interfaces/types defined? - **Type Assertions**: Are type assertions (`as`) used sparingly and only when necessary? @@ -56,7 +65,7 @@ You are a senior engineer conducting a thorough code review. Review **only the l - **Optional Chaining**: Is optional chaining (`?.`) used appropriately for potentially undefined values? - **Enums vs Union Types**: Are union types preferred over enums where appropriate? -### Design System & Styling +#### Design System & Styling - **Component Usage**: Are design system components used instead of raw HTML elements (`