PR Reviewer

This skill provides comprehensive pull request reviews with focus on code quality, security, performance, and modern design practices.

Quick Start

To review a PR:

# Option 1: Fetch PR from GitHub
gh pr view 123 --json files,title,body

# Option 2: Review local changes
git diff main...feature-branch

Then ask: "Please review this PR"

Output Language

IMPORTANT: Always provide all review feedback, comments, and analysis in Chinese (简体中文). Code examples and technical terms can remain in English, but all explanatory text, descriptions, and recommendations must be in Chinese.

Instructions

When reviewing a pull request, follow these steps:

1. Understand the Context

• Read the PR title and description carefully
• Understand what the PR aims to achieve
• Identify the scope: bug fix, feature, refactor, etc.
• Note any mentioned requirements or acceptance criteria

2. Analyze Changed Files

• Use gh pr view <number> --json files or git diff to see all changes
• Read each modified file completely
• Pay special attention to:
  • New files (completely new functionality)
  • Deleted files (removed functionality)
  • Large changes (potential complexity issues)
  • Configuration files (environment impact)

3. Review Code Quality

Check for:

Naming and Readability:

• Variable and function names are descriptive and follow conventions
• Code is self-documenting with clear intent
• Complex logic has explanatory comments
• No magic numbers or hardcoded values

Structure and Design:

• Functions are focused and single-purpose (under 50 lines ideally)
• Proper separation of concerns
• DRY principle followed (no unnecessary duplication)
• Appropriate use of design patterns
• Follows project's existing architecture and patterns

Error Handling:

• Edge cases are handled
• Error messages are informative
• Proper error propagation
• No swallowed exceptions

Code Smells:

• No overly complex conditionals
• No deeply nested code (max 3 levels)
• No god classes or functions
• No premature optimization

4. Security Review

Identify potential vulnerabilities:

Input Validation:

• All user input is validated and sanitized
• Type checking and bounds checking
• No trust in client-side data

Common Vulnerabilities:

• SQL Injection: Check for raw SQL with user input
• XSS: Check for unescaped output in templates/HTML
• CSRF: Verify CSRF tokens on state-changing operations
• Path Traversal: Validate file paths and prevent directory access
• Command Injection: Check for shell command execution with user input
• Insecure Deserialization: Be cautious with pickle, eval, etc.

Authentication & Authorization:

• Proper authentication checks
• Authorization verified before sensitive operations
• Session management is secure
• No hardcoded credentials or secrets

Data Protection:

• Sensitive data is encrypted
• No secrets in code or logs
• Proper use of environment variables
• Secure password handling (hashing, salting)

5. Performance Analysis

Look for:

Database Issues:

• N+1 query problems (queries in loops)
• Missing indexes on filtered/joined columns
• Fetching unnecessary data
• Inefficient joins or subqueries

Memory and Resources:

• Potential memory leaks (unclosed connections, event listeners)
• Large data structures held in memory
• Inefficient algorithms (O(n²) when O(n) possible)
• Resource cleanup (file handles, database connections)

Caching and Optimization:

• Opportunities for caching
• Redundant computations
• Unnecessary re-renders (React, Vue, etc.)
• Blocking operations that could be async

Scalability:

• Code works with large datasets
• No hardcoded limits
• Efficient pagination
• Proper use of streaming for large files

6. Testing Coverage

Verify:

• New functionality has tests
• Edge cases are tested
• Error conditions are tested
• Tests are meaningful, not just for coverage
• Tests follow AAA pattern (Arrange, Act, Assert)
• No flaky or brittle tests

7. Design and Architecture

Assess:

Modern Best Practices:

• Uses language/framework idioms correctly
• Follows community conventions (PEP 8, Airbnb style, etc.)
• Leverages standard library appropriately
• No reinventing the wheel

API Design:

• Consistent naming conventions
• Proper HTTP methods and status codes
• Versioning strategy if applicable
• Clear request/response contracts

Dependencies:

• No unnecessary dependencies
• Dependencies are up-to-date and maintained
• Proper version pinning
• License compatibility

Maintainability:

• Code is easy to modify and extend
• Clear boundaries between modules
• Documentation for complex logic
• Migration path if breaking changes

7.5. Frontend-Specific Review (前端专项检查)

IMPORTANT: For frontend repositories, pay special attention to these areas:

React/Vue/Angular Components

Component Design:

• Components are small and focused (under 200 lines)
• Props are clearly typed (TypeScript interfaces or PropTypes)
• Component naming follows conventions (PascalCase for components)
• No business logic in presentational components
• Proper component composition (avoid prop drilling)
• Use of compound components pattern where appropriate

State Management:

• Local state for UI-only concerns
• Global state for shared data (Redux, Zustand, Pinia, etc.)
• No unnecessary state (derive when possible)
• State updates are immutable
• Proper use of Context (avoid overuse causing re-renders)
• No stale closures in useEffect/computed

Hooks Best Practices (React):

• Dependency arrays are complete and correct
• No missing dependencies in useEffect/useCallback/useMemo
• Custom hooks for reusable logic
• No hooks called conditionally
• useCallback for function props passed to memoized children
• useMemo only for expensive computations
• Proper cleanup in useEffect

Performance Optimization:

• React.memo/Vue memo for expensive components
• Key prop on list items (not using index as key)
• Lazy loading for routes and heavy components
• Code splitting with dynamic imports
• Image optimization (next/image, lazy loading, WebP format)
• Virtual scrolling for long lists (react-window, vue-virtual-scroller)
• Debouncing/throttling for frequent events (scroll, resize, input)
• Avoid inline functions in render (causes re-renders)

Reactivity Issues (Vue):

• Proper use of ref vs reactive
• No losing reactivity when destructuring
• Computed vs methods appropriately used
• Watch with immediate and deep options correctly set

CSS and Styling

CSS Architecture:

• Consistent methodology (BEM, CSS Modules, Tailwind, CSS-in-JS)
• No global CSS pollution
• Scoped styles (CSS Modules, scoped in Vue, styled-components)
• Responsive design (mobile-first approach)
• Proper use of CSS variables/design tokens
• No hardcoded colors/spacing (use theme/design system)

Performance:

• No expensive CSS selectors (avoid universal *, deep nesting)
• Critical CSS inlined for above-the-fold content
• No layout thrashing (batch DOM reads/writes)
• GPU-accelerated animations (transform, opacity)
• Avoid triggering reflows (check will-change usage)

Accessibility:

• Proper color contrast ratios (WCAG AA: 4.5:1)
• Focus indicators visible and styled
• No outline: none without custom focus styles
• Responsive text sizing (rem/em, not px)

Accessibility (a11y)

Semantic HTML:

• Use semantic tags (nav, main, article, section, header, footer)
• Proper heading hierarchy (h1 → h2 → h3, no skipping)
• Button for actions, links for navigation
• Form labels associated with inputs

ARIA and Keyboard:

• ARIA attributes only when semantic HTML insufficient
• aria-label/aria-labelledby for screen readers
• Keyboard navigation works (Tab, Enter, Escape, Arrow keys)
• Focus management for modals/dialogs
• role attributes used correctly
• aria-live for dynamic content announcements

Testing:

• Use tools like axe-core, Lighthouse accessibility audit
• Test with keyboard only (no mouse)
• Test with screen reader (VoiceOver, NVDA)

Frontend Security

XSS Prevention:

• Never use dangerouslySetInnerHTML/v-html with user input
• Sanitize HTML if necessary (DOMPurify)
• Escape user content in templates
• Validate input on both client and server

Content Security Policy:

• CSP headers configured
• No inline scripts/styles in production
• Trusted sources for external resources

Authentication & Tokens:

• JWT/tokens stored securely (httpOnly cookies preferred)
• No sensitive data in localStorage/sessionStorage
• Proper token refresh flow
• Auto-logout on token expiration

Dependencies:

• Regular npm audit / yarn audit
• No known vulnerable packages
• SRI for CDN resources

API Integration

HTTP Requests:

• Proper error handling (network errors, 4xx, 5xx)
• Loading states for async operations
• Retry logic for failed requests (with exponential backoff)
• Request cancellation for unmounted components (AbortController)
• Timeout configuration

Data Fetching:

• Use libraries like React Query, SWR, Apollo (better caching)
• Avoid waterfalls (parallel requests when possible)
• Proper cache invalidation strategy
• Optimistic updates for better UX
• Stale-while-revalidate pattern

GraphQL Specific:

• No over-fetching or under-fetching
• Proper query batching
• Fragment colocation with components
• Persisted queries for production

Build and Bundle

Bundle Size:

• Check bundle size impact (use bundlephobia)
• Tree-shaking working correctly
• No unused dependencies in bundle
• Proper externals configuration
• Analyze bundle with webpack-bundle-analyzer

Code Splitting:

• Route-based splitting
• Component lazy loading for heavy components
• Dynamic imports for conditional features
• Vendor chunk separation

Assets:

• Images optimized (compressed, modern formats)
• SVG sprites for icons
• Font loading strategy (font-display: swap)
• Service Worker for offline support (if applicable)

TypeScript (if applicable)

Type Safety:

• No any types (use unknown if truly needed)
• Proper interface definitions for props and state
• Type guards for runtime checks
• Generic types used appropriately
• No type assertions (as) unless absolutely necessary

Type Organization:

• Shared types in dedicated files
• No duplicate type definitions
• Proper use of utility types (Pick, Omit, Partial, etc.)

Testing

Unit Tests:

• Component behavior tested, not implementation
• Test user interactions (click, input, etc.)
• Test different states and props combinations
• Mock external dependencies
• No snapshot testing as only test

Integration Tests:

• Critical user flows tested
• API mocking (MSW recommended)
• Test error states

E2E Tests:

• Happy paths for critical features
• Use Cypress, Playwright, or similar

Browser Compatibility

Cross-browser Support:

• Check target browser support (package.json browserslist)
• Polyfills for needed features
• No bleeding-edge APIs without fallbacks
• CSS prefixes where needed (autoprefixer)
• Test in target browsers (Chrome, Firefox, Safari, Edge)

User Experience

Loading States:

• Skeleton screens or spinners for loading
• Progressive rendering
• No layout shift (reserve space for dynamic content)

Error States:

• User-friendly error messages
• Error boundaries (React) to catch crashes
• Fallback UI for errors
• Retry mechanisms

Form UX:

• Inline validation (not just on submit)
• Clear error messages next to fields
• Disabled submit button while submitting
• Success feedback after submission
• Preserve form data on navigation back

Animations:

• Smooth transitions (not jarring)
• Respect prefers-reduced-motion
• No unnecessary animations
• Animation duration reasonable (200-300ms)

8. Provide Structured Feedback

CRITICAL: All feedback must be written in Chinese. Use the following format:

## 总结
[简要概述：这个 PR 做了什么？整体评估]

## ✅ 优点
- [突出显示良好的实践和实现得好的功能]

## 🚨 严重问题
[合并前必须修复的问题]

### 安全
- [任何安全漏洞]

### Bug
- [逻辑错误或明显的 bug]

## ⚠️ 重要建议
[应该解决的问题]

### 代码质量
- [命名、结构、复杂度问题]

### 性能
- [性能方面的担忧]

### 设计
- [架构或设计改进]

## 💡 次要建议
[可选的改进项]

### 风格和一致性
- [风格指南违规、小的不一致]

### 文档
- [缺失或不清晰的文档]

## ✅ 测试
[测试覆盖率和质量评估]

## 总体建议
- [ ] 批准 - 可以合并
- [ ] 批准并附带次要意见 - 处理次要问题后可合并
- [ ] 请求更改 - 必须先解决严重/重要问题才能合并
- [ ] 需要讨论 - 需求不明确或架构方面的疑虑

9. Be Constructive and Specific

Remember: All feedback examples below should be translated to Chinese when providing actual reviews.

Good feedback (示例 - 实际使用时用中文): ✅ "第 42 行的 processData 函数由于嵌套循环具有 O(n²) 的时间复杂度。建议使用哈希映射将其降低到 O(n)。示例: const lookup = new Map(data.map(item => [item.id, item]))"

✅ "第 127 行：用户输入直接拼接到 SQL 中，存在 SQL 注入漏洞。请使用参数化查询: db.query('SELECT * FROM users WHERE id = ?', [userId])"

Poor feedback (反例): ❌ "这段代码效率低" ❌ "命名不好" ❌ "这里有安全问题"

10. Consider Context

Remember to:

• Distinguish between blocking issues vs. suggestions
• Consider the PR's scope (don't request unrelated changes)
• Acknowledge good practices
• Be respectful and collaborative
• Focus on the code, not the person
• Provide alternatives, not just criticism

Examples

Example 1: Security Issue

# ❌ Vulnerable to SQL injection
query = f"SELECT * FROM users WHERE email = '{email}'"
cursor.execute(query)

# ✅ Fixed with parameterized query
query = "SELECT * FROM users WHERE email = %s"
cursor.execute(query, (email,))

Review comment (中文): "第 42 行：SQL 注入漏洞。email 参数直接插入到查询字符串中，允许攻击者注入恶意 SQL。请使用参数化查询，如建议中所示。"

Example 2: Performance Issue

// ❌ N+1 query problem
const users = await User.findAll();
for (const user of users) {
  user.posts = await Post.findAll({ where: { userId: user.id } });
}

// ✅ Fixed with eager loading
const users = await User.findAll({
  include: [{ model: Post }]
});

Review comment (中文): "第 56-59 行：N+1 查询问题。这会执行 1 次用户查询 + N 次文章查询（每个用户一次）。如果有 1000 个用户，就是 1001 次数据库查询。使用预加载最多用 2 次查询获取所有数据。"

Example 3: Code Quality

// ❌ Unclear, complex function
function p(d) {
  if (d.t === 'a' && d.v > 0 && d.s === 1) {
    return d.v * 1.1;
  } else if (d.t === 'b' && d.v > 0) {
    return d.v * 1.05;
  }
  return d.v;
}

// ✅ Clear, maintainable function
function calculatePriceWithDiscount(order) {
  const { type, value, status } = order;

  if (type === 'premium' && value > 0 && status === 'active') {
    return value * 1.1;
  }

  if (type === 'basic' && value > 0) {
    return value * 1.05;
  }

  return value;
}

Review comment (中文): "第 23 行：函数名和参数不够描述性。建议重命名为 calculatePriceWithDiscount，并使用结构合理的 order 对象。这样可以让代码自文档化。"

Best Practices

1. Review thoroughly but efficiently - Focus on high-impact issues first
2. Test locally when needed - Pull the branch if something is unclear
3. Ask questions - If intent is unclear, ask the author
4. Praise good code - Acknowledge well-written sections
5. Link to documentation - Support suggestions with references
6. Consider alternatives - Sometimes there are multiple valid approaches
7. Review your own comments - Ensure they're clear and actionable
8. Follow up - Check if your feedback was addressed

Common Pitfalls to Avoid

• ❌ Nitpicking style when there's a linter
• ❌ Requesting changes outside the PR scope
• ❌ Being vague ("this is bad")
• ❌ Making it personal ("you should know better")
• ❌ Bike-shedding (arguing about trivial details)
• ❌ Requesting perfect code (good enough can be good enough)
• ❌ Not explaining why something is an issue

Requirements

For GitHub PR review:

gh auth login  # One-time setup

For local diff review, just use git:

git diff main...feature-branch

Advanced Usage

For detailed security checklist and performance optimization patterns, see checklist.md.

For frontend repositories, see comprehensive frontend-specific checklist in frontend-checklist.md, which includes:

• React/Vue/Angular best practices
• Hooks and reactivity issues
• CSS/Styling architecture
• Accessibility (a11y) requirements
• Performance optimization (bundle size, runtime, Core Web Vitals)
• TypeScript patterns
• Frontend security (XSS, CSP, token storage)
• Testing strategies (unit, integration, e2e)
• Browser compatibility

For language-specific review guidelines (Python, Go, etc.), ask me to review with the language context in mind.