diff --git a/ui/goose2/.agents/skills/code-review/SKILL.md b/ui/goose2/.agents/skills/code-review/SKILL.md new file mode 100644 index 0000000000..f8cec9b42e --- /dev/null +++ b/ui/goose2/.agents/skills/code-review/SKILL.md @@ -0,0 +1,190 @@ +--- +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. +--- + +# Pre-PR Code Review + +You are a senior engineer conducting a thorough code review. Review **only the lines that changed** in this branch (via `git diff main...HEAD`) and provide actionable feedback on code quality. Do not flag issues in unchanged code. + +## Determine Files to Review + +**Before starting the review**, identify which files to review by checking: + +1. **Run git commands** to check both: + - Committed changes: `git diff --name-only main...HEAD` + - Unstaged/staged changes: `git status --short` + +2. **Ask the user which set to review** if both exist: + - If there are both committed changes AND unstaged/staged changes, ask: "I see you have both committed changes and unstaged/staged changes. Which would you like me to review?" + - **Option A**: Committed changes in this branch (compare against main) + - **Option B**: Current unstaged/staged changes + - **Option C**: Both + +3. **Proceed automatically** if only one set exists: + - If only committed changes exist → review those + - If only unstaged/staged changes exist → review those + - If neither exist → inform the user there are no changes to review + +4. **Get the file list** based on the user's choice: + - For committed changes: Use `git diff --name-only main...HEAD` + - For unstaged/staged: Use `git diff --name-only` and `git diff --cached --name-only` + - Filter to only include files that exist (some may be deleted) + +**Only proceed with the review once you have the specific list of files to review.** + +## Review Checklist + +### 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 +- **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? +- **Non-null Assertions**: Are non-null assertions (`!`) avoided? They bypass TypeScript's null safety and hide bugs. Use proper null checks or optional chaining instead. +- **React Ref Types**: Are React refs properly typed as nullable (`useRef(null)` with `RefObject`)? Refs are null on first render and during unmount. +- **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 +- **Component Usage**: Are design system components used instead of raw HTML elements (`