commit 53c5f735f5d1ae887c26029575e4a982f7d30a95 Author: Tu Shaokun <2801884530@qq.com> Date: Sat Nov 29 10:06:40 2025 +0800 Initial commit: AI Code Review Guide skill - Comprehensive code review skill for Claude Code - Support for React 19, Vue 3, Rust, TypeScript - TanStack Query v5, Suspense & Streaming patterns - ~3000 lines of review guidelines and checklists diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..3f7a9f0 --- /dev/null +++ b/.gitignore @@ -0,0 +1,25 @@ +# OS files +.DS_Store +Thumbs.db + +# Editor files +*.swp +*.swo +*~ +.idea/ +.vscode/ + +# Python +__pycache__/ +*.py[cod] +*.egg-info/ +.eggs/ +dist/ +build/ + +# Logs +*.log + +# Local config +.env +.env.local diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..0649f69 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,102 @@ +# Contributing to AI Code Review Guide + +Thank you for your interest in contributing! This document provides guidelines for contributing to this project. + +## How to Contribute + +### Reporting Issues + +- Use GitHub Issues to report bugs or suggest enhancements +- Provide clear descriptions and examples +- Include relevant code snippets when applicable + +### Submitting Changes + +1. Fork the repository +2. Create a feature branch (`git checkout -b feature/your-feature`) +3. Make your changes +4. Commit with clear messages (`git commit -m "Add: description"`) +5. Push to your fork (`git push origin feature/your-feature`) +6. Open a Pull Request + +## Contribution Types + +### Adding New Language Support + +When adding a new programming language: + +1. Add a new section in `SKILL.md` with: + - Language-specific review patterns + - Common anti-patterns with ❌/✅ examples + - A review checklist + +2. Add corresponding entries in `references/common-bugs-checklist.md` + +3. Format: + ```markdown + ### [Language] Code Review + + ```[language] + // ❌ Bad pattern + code example + + // ✅ Good pattern + code example + ``` + + #### [Language] Review Checklist + - [ ] Check item 1 + - [ ] Check item 2 + ``` + +### Adding Framework Patterns + +When adding framework-specific patterns: + +1. Ensure you reference official documentation +2. Include version numbers (e.g., "React 19", "Vue 3.4+") +3. Provide working code examples +4. Add corresponding checklist items + +### Improving Existing Content + +- Fix typos or grammatical errors +- Update outdated patterns +- Add more edge cases +- Improve code examples + +## Code Example Guidelines + +### Format + +- Use ❌ for anti-patterns +- Use ✅ for recommended patterns +- Include comments explaining why + +### Quality + +- Examples should be realistic, not contrived +- Show both the problem and the solution +- Keep examples focused and minimal + +## Commit Message Format + +``` +Type: Short description + +Longer description if needed + +- Detail 1 +- Detail 2 +``` + +Types: +- `Add`: New feature or content +- `Fix`: Bug fix or correction +- `Update`: Update existing content +- `Refactor`: Restructure without changing behavior +- `Docs`: Documentation only changes + +## Questions? + +Feel free to open an issue for any questions about contributing. diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..8217668 --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2025 tt-a1i + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md new file mode 100644 index 0000000..54f416b --- /dev/null +++ b/README.md @@ -0,0 +1,141 @@ +# AI Code Review Guide + +> A comprehensive code review skill for Claude Code, covering React 19, Vue 3, Rust, TypeScript, and more. + +[![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://opensource.org/licenses/MIT) + +## Overview + +This is a Claude Code skill designed to help developers conduct effective code reviews. It provides: + +- **Language-specific patterns** for React 19, Vue 3, Rust, TypeScript/JavaScript, Python, SQL +- **Modern framework support** including React Server Components, TanStack Query v5, Suspense & Streaming +- **Comprehensive checklists** for security, performance, and code quality +- **Best practices** for giving constructive feedback + +## Features + +### Supported Languages & Frameworks + +| Category | Coverage | +|----------|----------| +| **React** | Hooks rules, useEffect patterns, useMemo/useCallback, React 19 Actions (useActionState, useFormStatus, useOptimistic), Server Components, Suspense & Streaming | +| **Vue 3** | Composition API, reactivity system, defineProps/defineEmits, watch cleanup | +| **Rust** | Ownership & borrowing, unsafe code review, async/await, error handling (thiserror vs anyhow) | +| **TypeScript** | Type safety, async/await patterns, common pitfalls | +| **TanStack Query** | v5 best practices, queryOptions, useSuspenseQuery, optimistic updates | + +### Content Statistics + +- **SKILL.md**: ~1,800 lines of review guidelines and code examples +- **common-bugs-checklist.md**: ~1,200 lines of bug patterns and checklists +- Additional reference files for security review, PR templates, and more + +## Installation + +### For Claude Code Users + +Copy the skill to your Claude Code plugins directory: + +```bash +# Clone the repository +git clone https://github.com/tt-a1i/ai-code-review-guide.git + +# Copy to Claude Code skills directory +cp -r ai-code-review-guide ~/.claude/commands/code-review-excellence +``` + +Or add to your existing Claude Code plugin: + +```bash +cp -r ai-code-review-guide/SKILL.md ~/.claude/plugins/your-plugin/skills/code-review/ +cp -r ai-code-review-guide/references ~/.claude/plugins/your-plugin/skills/code-review/ +``` + +## Usage + +Once installed, you can invoke the skill in Claude Code: + +``` +Use code-review-excellence skill to review this PR +``` + +Or reference it in your custom commands. + +## File Structure + +``` +ai-code-review-guide/ +├── SKILL.md # Main skill definition +├── README.md # This file +├── LICENSE # MIT License +├── CONTRIBUTING.md # Contribution guidelines +├── references/ +│ ├── common-bugs-checklist.md # Language-specific bug patterns +│ ├── security-review-guide.md # Security review checklist +│ └── code-review-best-practices.md +├── assets/ +│ ├── review-checklist.md # Quick reference checklist +│ └── pr-review-template.md # PR review comment template +└── scripts/ + └── pr-analyzer.py # PR complexity analyzer +``` + +## Key Topics Covered + +### React 19 + +- `useActionState` - Unified form state management +- `useFormStatus` - Access parent form status without prop drilling +- `useOptimistic` - Optimistic UI updates with automatic rollback +- Server Actions integration with Next.js 15+ + +### Suspense & Streaming SSR + +- Suspense boundary design patterns +- Error Boundary integration +- Next.js 15 streaming with `loading.tsx` +- `use()` Hook for Promise consumption + +### TanStack Query v5 + +- `queryOptions` for type-safe query definitions +- `useSuspenseQuery` best practices +- Optimistic updates (simplified v5 approach) +- `isPending` vs `isLoading` vs `isFetching` + +### Rust + +- Ownership patterns and common pitfalls +- `unsafe` code review requirements (SAFETY comments) +- Async/await patterns (avoiding blocking in async context) +- Error handling (thiserror for libraries, anyhow for applications) + +## Contributing + +Contributions are welcome! Please read [CONTRIBUTING.md](CONTRIBUTING.md) for guidelines. + +### Areas for Contribution + +- Additional language support (Go, Java, C++, etc.) +- More framework-specific patterns +- Translations to other languages +- Bug pattern submissions + +## License + +This project is licensed under the MIT License - see the [LICENSE](LICENSE) file for details. + +## Acknowledgments + +- Inspired by effective code review practices from the software engineering community +- React documentation for React 19 features +- TanStack Query documentation +- TkDodo's blog for React Query best practices + +## References + +- [React v19 Official Documentation](https://react.dev/blog/2024/12/05/react-19) +- [TanStack Query v5 Documentation](https://tanstack.com/query/latest) +- [Vue 3 Composition API](https://vuejs.org/guide/extras/composition-api-faq.html) +- [Rust API Guidelines](https://rust-lang.github.io/api-guidelines/) diff --git a/SKILL.md b/SKILL.md new file mode 100644 index 0000000..344e2eb --- /dev/null +++ b/SKILL.md @@ -0,0 +1,1774 @@ +--- +name: code-review-excellence +description: Master effective code review practices to provide constructive feedback, catch bugs early, and foster knowledge sharing while maintaining team morale. Use when reviewing pull requests, establishing review standards, or mentoring developers. +--- + +# Code Review Excellence + +Transform code reviews from gatekeeping to knowledge sharing through constructive feedback, systematic analysis, and collaborative improvement. + +## When to Use This Skill + +- Reviewing pull requests and code changes +- Establishing code review standards for teams +- Mentoring junior developers through reviews +- Conducting architecture reviews +- Creating review checklists and guidelines +- Improving team collaboration +- Reducing code review cycle time +- Maintaining code quality standards + +## Core Principles + +### 1. The Review Mindset + +**Goals of Code Review:** +- Catch bugs and edge cases +- Ensure code maintainability +- Share knowledge across team +- Enforce coding standards +- Improve design and architecture +- Build team culture + +**Not the Goals:** +- Show off knowledge +- Nitpick formatting (use linters) +- Block progress unnecessarily +- Rewrite to your preference + +### 2. Effective Feedback + +**Good Feedback is:** +- Specific and actionable +- Educational, not judgmental +- Focused on the code, not the person +- Balanced (praise good work too) +- Prioritized (critical vs nice-to-have) + +```markdown +❌ Bad: "This is wrong." +✅ Good: "This could cause a race condition when multiple users + access simultaneously. Consider using a mutex here." + +❌ Bad: "Why didn't you use X pattern?" +✅ Good: "Have you considered the Repository pattern? It would + make this easier to test. Here's an example: [link]" + +❌ Bad: "Rename this variable." +✅ Good: "[nit] Consider `userCount` instead of `uc` for + clarity. Not blocking if you prefer to keep it." +``` + +### 3. Review Scope + +**What to Review:** +- Logic correctness and edge cases +- Security vulnerabilities +- Performance implications +- Test coverage and quality +- Error handling +- Documentation and comments +- API design and naming +- Architectural fit + +**What Not to Review Manually:** +- Code formatting (use Prettier, Black, etc.) +- Import organization +- Linting violations +- Simple typos + +## Review Process + +### Phase 1: Context Gathering (2-3 minutes) + +```markdown +Before diving into code, understand: + +1. Read PR description and linked issue +2. Check PR size (>400 lines? Ask to split) +3. Review CI/CD status (tests passing?) +4. Understand the business requirement +5. Note any relevant architectural decisions +``` + +### Phase 2: High-Level Review (5-10 minutes) + +```markdown +1. **Architecture & Design** + - Does the solution fit the problem? + - Are there simpler approaches? + - Is it consistent with existing patterns? + - Will it scale? + +2. **File Organization** + - Are new files in the right places? + - Is code grouped logically? + - Are there duplicate files? + +3. **Testing Strategy** + - Are there tests? + - Do tests cover edge cases? + - Are tests readable? +``` + +### Phase 3: Line-by-Line Review (10-20 minutes) + +```markdown +For each file: + +1. **Logic & Correctness** + - Edge cases handled? + - Off-by-one errors? + - Null/undefined checks? + - Race conditions? + +2. **Security** + - Input validation? + - SQL injection risks? + - XSS vulnerabilities? + - Sensitive data exposure? + +3. **Performance** + - N+1 queries? + - Unnecessary loops? + - Memory leaks? + - Blocking operations? + +4. **Maintainability** + - Clear variable names? + - Functions doing one thing? + - Complex code commented? + - Magic numbers extracted? +``` + +### Phase 4: Summary & Decision (2-3 minutes) + +```markdown +1. Summarize key concerns +2. Highlight what you liked +3. Make clear decision: + - ✅ Approve + - 💬 Comment (minor suggestions) + - 🔄 Request Changes (must address) +4. Offer to pair if complex +``` + +## Review Techniques + +### Technique 1: The Checklist Method + +```markdown +## Security Checklist +- [ ] User input validated and sanitized +- [ ] SQL queries use parameterization +- [ ] Authentication/authorization checked +- [ ] Secrets not hardcoded +- [ ] Error messages don't leak info + +## Performance Checklist +- [ ] No N+1 queries +- [ ] Database queries indexed +- [ ] Large lists paginated +- [ ] Expensive operations cached +- [ ] No blocking I/O in hot paths + +## Testing Checklist +- [ ] Happy path tested +- [ ] Edge cases covered +- [ ] Error cases tested +- [ ] Test names are descriptive +- [ ] Tests are deterministic +``` + +### Technique 2: The Question Approach + +Instead of stating problems, ask questions to encourage thinking: + +```markdown +❌ "This will fail if the list is empty." +✅ "What happens if `items` is an empty array?" + +❌ "You need error handling here." +✅ "How should this behave if the API call fails?" + +❌ "This is inefficient." +✅ "I see this loops through all users. Have we considered + the performance impact with 100k users?" +``` + +### Technique 3: Suggest, Don't Command + +```markdown +## Use Collaborative Language + +❌ "You must change this to use async/await" +✅ "Suggestion: async/await might make this more readable: + ```typescript + async function fetchUser(id: string) { + const user = await db.query('SELECT * FROM users WHERE id = ?', id); + return user; + } + ``` + What do you think?" + +❌ "Extract this into a function" +✅ "This logic appears in 3 places. Would it make sense to + extract it into a shared utility function?" +``` + +### Technique 4: Differentiate Severity + +```markdown +Use labels to indicate priority: + +🔴 [blocking] - Must fix before merge +🟡 [important] - Should fix, discuss if disagree +🟢 [nit] - Nice to have, not blocking +💡 [suggestion] - Alternative approach to consider +📚 [learning] - Educational comment, no action needed +🎉 [praise] - Good work, keep it up! + +Example: +"🔴 [blocking] This SQL query is vulnerable to injection. + Please use parameterized queries." + +"🟢 [nit] Consider renaming `data` to `userData` for clarity." + +"🎉 [praise] Excellent test coverage! This will catch edge cases." +``` + +## Language-Specific Patterns + +### Python Code Review + +```python +# Check for Python-specific issues + +# ❌ Mutable default arguments +def add_item(item, items=[]): # Bug! Shared across calls + items.append(item) + return items + +# ✅ Use None as default +def add_item(item, items=None): + if items is None: + items = [] + items.append(item) + return items + +# ❌ Catching too broad +try: + result = risky_operation() +except: # Catches everything, even KeyboardInterrupt! + pass + +# ✅ Catch specific exceptions +try: + result = risky_operation() +except ValueError as e: + logger.error(f"Invalid value: {e}") + raise + +# ❌ Using mutable class attributes +class User: + permissions = [] # Shared across all instances! + +# ✅ Initialize in __init__ +class User: + def __init__(self): + self.permissions = [] +``` + +### TypeScript/JavaScript Code Review + +```typescript +// Check for TypeScript-specific issues + +// ❌ Using any defeats type safety +function processData(data: any) { // Avoid any + return data.value; +} + +// ✅ Use proper types +interface DataPayload { + value: string; +} +function processData(data: DataPayload) { + return data.value; +} + +// ❌ Not handling async errors +async function fetchUser(id: string) { + const response = await fetch(`/api/users/${id}`); + return response.json(); // What if network fails? +} + +// ✅ Handle errors properly +async function fetchUser(id: string): Promise { + try { + const response = await fetch(`/api/users/${id}`); + if (!response.ok) { + throw new Error(`HTTP ${response.status}`); + } + return await response.json(); + } catch (error) { + console.error('Failed to fetch user:', error); + throw error; + } +} + +// ❌ Mutation of props +function UserProfile({ user }: Props) { + user.lastViewed = new Date(); // Mutating prop! + return
{user.name}
; +} + +// ✅ Don't mutate props +function UserProfile({ user, onView }: Props) { + useEffect(() => { + onView(user.id); // Notify parent to update + }, [user.id]); + return
{user.name}
; +} +``` + +### React Code Review + +React 审查重点：Hooks 规则、性能优化的适度性、组件设计、以及现代 React 19/RSC 模式。 + +```tsx +// === Hooks 规则 === + +// ❌ 条件调用 Hooks — 违反 Hooks 规则 +function BadComponent({ isLoggedIn }) { + if (isLoggedIn) { + const [user, setUser] = useState(null); // Error! + } + return
...
; +} + +// ✅ Hooks 必须在组件顶层调用 +function GoodComponent({ isLoggedIn }) { + const [user, setUser] = useState(null); + if (!isLoggedIn) return ; + return
{user?.name}
; +} + +// === useEffect 常见错误 === + +// ❌ 依赖数组缺失或不完整 +function BadEffect({ userId }) { + const [user, setUser] = useState(null); + useEffect(() => { + fetchUser(userId).then(setUser); + }, []); // 缺少 userId 依赖！ +} + +// ✅ 完整的依赖数组 +function GoodEffect({ userId }) { + const [user, setUser] = useState(null); + useEffect(() => { + let cancelled = false; + fetchUser(userId).then(data => { + if (!cancelled) setUser(data); + }); + return () => { cancelled = true; }; // 清理函数 + }, [userId]); +} + +// ❌ useEffect 用于派生状态（反模式） +function BadDerived({ items }) { + const [filteredItems, setFilteredItems] = useState([]); + useEffect(() => { + setFilteredItems(items.filter(i => i.active)); + }, [items]); // 不必要的 effect + 额外渲染 + return ; +} + +// ✅ 直接在渲染时计算，或用 useMemo +function GoodDerived({ items }) { + const filteredItems = useMemo( + () => items.filter(i => i.active), + [items] + ); + return ; +} + +// ❌ useEffect 用于事件响应 +function BadEventEffect() { + const [query, setQuery] = useState(''); + useEffect(() => { + if (query) { + analytics.track('search', { query }); // 应该在事件处理器中 + } + }, [query]); +} + +// ✅ 在事件处理器中执行副作用 +function GoodEvent() { + const [query, setQuery] = useState(''); + const handleSearch = (q: string) => { + setQuery(q); + analytics.track('search', { query: q }); + }; +} + +// === useMemo / useCallback 过度使用 === + +// ❌ 过度优化 — 常量不需要 useMemo +function OverOptimized() { + const config = useMemo(() => ({ timeout: 5000 }), []); // 无意义 + const handleClick = useCallback(() => { + console.log('clicked'); + }, []); // 如果不传给 memo 组件，无意义 +} + +// ✅ 只在需要时优化 +function ProperlyOptimized() { + const config = { timeout: 5000 }; // 简单对象直接定义 + const handleClick = () => console.log('clicked'); +} + +// ❌ useCallback 依赖总是变化 +function BadCallback({ data }) { + // data 每次渲染都是新对象，useCallback 无效 + const process = useCallback(() => { + return data.map(transform); + }, [data]); +} + +// ✅ useMemo + useCallback 配合 React.memo 使用 +const MemoizedChild = React.memo(function Child({ onClick, items }) { + return
{items.length}
; +}); + +function Parent({ rawItems }) { + const items = useMemo(() => processItems(rawItems), [rawItems]); + const handleClick = useCallback(() => { + console.log(items.length); + }, [items]); + return ; +} + +// === 组件设计 === + +// ❌ 在组件内定义组件 — 每次渲染都创建新组件 +function BadParent() { + function ChildComponent() { // 每次渲染都是新函数！ + return
child
; + } + return ; +} + +// ✅ 组件定义在外部 +function ChildComponent() { + return
child
; +} +function GoodParent() { + return ; +} + +// ❌ Props 总是新对象引用 +function BadProps() { + return ( + {}} // 每次渲染新函数 + /> + ); +} + +// ✅ 稳定的引用 +const style = { color: 'red' }; +function GoodProps() { + const handleClick = useCallback(() => {}, []); + return ; +} + +// === Error Boundaries & Suspense === + +// ❌ 没有错误边界 +function BadApp() { + return ( + }> + {/* 错误会导致整个应用崩溃 */} + + ); +} + +// ✅ Error Boundary 包裹 Suspense +function GoodApp() { + return ( + }> + }> + + + + ); +} + +// === React 19 / Server Components === + +// ❌ 在 Server Component 中使用客户端特性 +// app/page.tsx (Server Component by default) +function BadServerComponent() { + const [count, setCount] = useState(0); // Error! No hooks in RSC + return ; +} + +// ✅ 交互逻辑提取到 Client Component +// app/counter.tsx +'use client'; +function Counter() { + const [count, setCount] = useState(0); + return ; +} + +// app/page.tsx (Server Component) +function GoodServerComponent() { + const data = await fetchData(); // 可以直接 await + return ( +
+

{data.title}

+ {/* 客户端组件 */} +
+ ); +} + +// ❌ 'use client' 放置不当 — 整个树都变成客户端 +// layout.tsx +'use client'; // 这会让所有子组件都成为客户端组件 +export default function Layout({ children }) { ... } + +// ✅ 只在需要交互的组件使用 'use client' +// 将客户端逻辑隔离到叶子组件 +``` + +#### React Review Checklist + +```markdown +## Hooks 规则 + +- [ ] Hooks 在组件/自定义 Hook 顶层调用 +- [ ] 没有条件/循环中调用 Hooks +- [ ] useEffect 依赖数组完整 +- [ ] useEffect 有清理函数（订阅/定时器/请求） +- [ ] 没有用 useEffect 计算派生状态 + +## 性能优化（适度原则） + +- [ ] useMemo/useCallback 只用于真正需要的场景 +- [ ] React.memo 配合稳定的 props 引用 +- [ ] 没有在组件内定义子组件 +- [ ] 没有在 JSX 中创建新对象/函数（除非传给非 memo 组件） +- [ ] 长列表使用虚拟化（react-window/react-virtual） + +## 组件设计 + +- [ ] 组件职责单一，不超过 200 行 +- [ ] 逻辑与展示分离（Custom Hooks） +- [ ] Props 接口清晰，使用 TypeScript +- [ ] 避免 Props Drilling（考虑 Context 或组合） + +## 状态管理 + +- [ ] 状态就近原则（最小必要范围） +- [ ] 复杂状态用 useReducer +- [ ] 全局状态用 Context 或状态库 +- [ ] 避免不必要的状态（派生 > 存储） + +## 错误处理 + +- [ ] 关键区域有 Error Boundary +- [ ] Suspense 配合 Error Boundary 使用 +- [ ] 异步操作有错误处理 + +## Server Components (RSC) + +- [ ] 'use client' 只用于需要交互的组件 +- [ ] Server Component 不使用 Hooks/事件处理 +- [ ] 客户端组件尽量放在叶子节点 +- [ ] 数据获取在 Server Component 中进行 + +## 测试 + +- [ ] 使用 @testing-library/react +- [ ] 用 screen 查询元素 +- [ ] 用 userEvent 代替 fireEvent +- [ ] 优先使用 *ByRole 查询 +- [ ] 测试行为而非实现细节 +``` + +### React 19 Actions & Forms + +React 19 引入了 Actions 系统和新的表单处理 Hooks，简化异步操作和乐观更新。 + +```tsx +// === useActionState (替代 useFormState) === + +// ❌ 传统方式：多个状态变量 +function OldForm() { + const [isPending, setIsPending] = useState(false); + const [error, setError] = useState(null); + const [data, setData] = useState(null); + + const handleSubmit = async (formData: FormData) => { + setIsPending(true); + setError(null); + try { + const result = await submitForm(formData); + setData(result); + } catch (e) { + setError(e.message); + } finally { + setIsPending(false); + } + }; +} + +// ✅ React 19: useActionState 统一管理 +import { useActionState } from 'react'; + +function NewForm() { + const [state, formAction, isPending] = useActionState( + async (prevState, formData: FormData) => { + try { + const result = await submitForm(formData); + return { success: true, data: result }; + } catch (e) { + return { success: false, error: e.message }; + } + }, + { success: false, data: null, error: null } + ); + + return ( +
+ + + {state.error &&

{state.error}

} +
+ ); +} + +// === useFormStatus === + +// ❌ Props 透传表单状态 +function BadSubmitButton({ isSubmitting }) { + return ; +} + +// ✅ useFormStatus 访问父
状态（无需 props） +import { useFormStatus } from 'react-dom'; + +function SubmitButton() { + const { pending, data, method, action } = useFormStatus(); + // 注意：必须在 内部的子组件中使用 + return ( + + ); +} + +// ❌ useFormStatus 在 form 同级组件中调用——不工作 +function BadForm() { + const { pending } = useFormStatus(); // 这里无法获取状态！ + return ( + + +
+ ); +} + +// ✅ useFormStatus 必须在 form 的子组件中 +function GoodForm() { + return ( +
+ {/* useFormStatus 在这里面调用 */} + + ); +} + +// === useOptimistic === + +// ❌ 等待服务器响应再更新 UI +function SlowLike({ postId, likes }) { + const [likeCount, setLikeCount] = useState(likes); + const [isPending, setIsPending] = useState(false); + + const handleLike = async () => { + setIsPending(true); + const newCount = await likePost(postId); // 等待... + setLikeCount(newCount); + setIsPending(false); + }; +} + +// ✅ useOptimistic 即时反馈，失败自动回滚 +import { useOptimistic } from 'react'; + +function FastLike({ postId, likes }) { + const [optimisticLikes, addOptimisticLike] = useOptimistic( + likes, + (currentLikes, increment: number) => currentLikes + increment + ); + + const handleLike = async () => { + addOptimisticLike(1); // 立即更新 UI + try { + await likePost(postId); // 后台同步 + } catch { + // React 自动回滚到 likes 原值 + } + }; + + return ; +} + +// === 配合 Server Actions (Next.js 15+) === + +// ❌ 客户端调用 API +'use client'; +function ClientForm() { + const handleSubmit = async (formData: FormData) => { + const res = await fetch('/api/submit', { + method: 'POST', + body: formData, + }); + // ... + }; +} + +// ✅ Server Action + useActionState +// actions.ts +'use server'; +export async function createPost(prevState: any, formData: FormData) { + const title = formData.get('title'); + await db.posts.create({ title }); + revalidatePath('/posts'); + return { success: true }; +} + +// form.tsx +'use client'; +import { createPost } from './actions'; + +function PostForm() { + const [state, formAction, isPending] = useActionState(createPost, null); + return ( +
+ + + + ); +} +``` + +#### React 19 Forms Checklist + +```markdown +## useActionState +- [ ] 使用 useActionState 替代多个 useState（isPending/error/data） +- [ ] Action 函数返回新状态，不是直接 setState +- [ ] 正确处理错误状态和成功状态 +- [ ] 配合 Server Actions 时标记 'use server' + +## useFormStatus +- [ ] 在
的子组件中调用，不是同级 +- [ ] 不依赖 props 传递表单状态 +- [ ] 只用于读取状态，不用于触发提交 + +## useOptimistic +- [ ] 用于需要即时反馈的操作（点赞、收藏等） +- [ ] 提供合理的乐观更新逻辑 +- [ ] 服务端失败时 UI 会自动回滚 +- [ ] 不用于关键数据（如支付） +``` + +### Suspense & Streaming SSR + +Suspense 和 Streaming 是 React 18+ 的核心特性，在 2025 年的 Next.js 15 等框架中广泛使用。 + +```tsx +// === 基础 Suspense 用法 === + +// ❌ 传统加载状态管理 +function OldComponent() { + const [data, setData] = useState(null); + const [isLoading, setIsLoading] = useState(true); + + useEffect(() => { + fetchData().then(setData).finally(() => setIsLoading(false)); + }, []); + + if (isLoading) return ; + return ; +} + +// ✅ Suspense 声明式加载状态 +function NewComponent() { + return ( + }> + {/* 内部使用 use() 或支持 Suspense 的数据获取 */} + + ); +} + +// === 多个独立 Suspense 边界 === + +// ❌ 单一边界——所有内容一起加载 +function BadLayout() { + return ( + }> +
+ {/* 慢 */} + {/* 快 */} + + ); +} + +// ✅ 独立边界——各部分独立流式传输 +function GoodLayout() { + return ( + <> +
{/* 立即显示 */} +
+ }> + {/* 独立加载 */} + + }> + {/* 独立加载 */} + +
+ + ); +} + +// === Error Boundary 配合 Suspense === + +// ❌ Suspense 没有错误处理 +function NoErrorHandling() { + return ( + }> + {/* 抛错会导致整个应用崩溃 */} + + ); +} + +// ✅ Error Boundary 包裹 Suspense +import { ErrorBoundary } from 'react-error-boundary'; + +function WithErrorHandling() { + return ( + } + onError={(error) => logError(error)} + > + }> + + + + ); +} + +// === Next.js 15 Streaming === + +// app/page.tsx - 自动 Streaming +export default async function Page() { + // 这个 await 不会阻塞整个页面 + const data = await fetchSlowData(); + return
{data}
; +} + +// app/loading.tsx - 自动 Suspense 边界 +export default function Loading() { + return ; +} + +// === 嵌套 Suspense 策略 === + +// ❌ 过深嵌套——难以调试 +function OverNested() { + return ( + + + + + + + + + + ); +} + +// ✅ 按用户体验需求分层 +function WellStructured() { + return ( + }> {/* 应用骨架 */} + + }> + {/* 导航优先 */} + + }> + {/* 主内容 */} + + + + ); +} + +// === use() Hook (React 19) === + +// ✅ 在组件中读取 Promise +import { use } from 'react'; + +function Comments({ commentsPromise }) { + const comments = use(commentsPromise); // 自动触发 Suspense + return ( +
    + {comments.map(c =>
  • {c.text}
    • )} +
+ ); +} + +// 父组件创建 Promise，子组件消费 +function Post({ postId }) { + const commentsPromise = fetchComments(postId); // 不 await + return ( +
+ + }> + + +
+ ); +} +``` + +#### Suspense & Streaming Checklist + +```markdown +## Suspense 边界设计 +- [ ] 按用户体验需求划分 Suspense 边界 +- [ ] 独立内容使用独立的 Suspense 边界 +- [ ] 避免过深嵌套（2-3 层为宜） +- [ ] 提供有意义的 fallback（骨架屏 > 简单 Spinner） + +## 错误处理 +- [ ] 每个 Suspense 有对应的 Error Boundary +- [ ] Error Boundary 提供恢复机制（retry 按钮） +- [ ] 错误被正确记录和上报 + +## Streaming SSR (Next.js) +- [ ] 慢数据使用 Suspense 包裹，不阻塞整个页面 +- [ ] loading.tsx 用于路由级别加载状态 +- [ ] 理解 Server Component vs Client Component 边界 +- [ ] 避免在 layout 层级使用会触发全页 Suspense 的数据获取 + +## 性能考虑 +- [ ] 优先显示重要内容（Navigation, Header） +- [ ] 非关键内容延迟加载 +- [ ] 使用 startTransition 处理非紧急更新 +``` + +### TanStack Query (React Query) v5 + +TanStack Query 是 React 生态中最流行的数据获取库，v5 是当前稳定版本。 + +```tsx +// === 基础配置 === + +// ❌ 不正确的默认配置 +const queryClient = new QueryClient(); // 默认配置可能不适合 + +// ✅ 生产环境推荐配置 +const queryClient = new QueryClient({ + defaultOptions: { + queries: { + staleTime: 1000 * 60 * 5, // 5 分钟内数据视为新鲜 + gcTime: 1000 * 60 * 30, // 30 分钟后垃圾回收（v5 重命名） + retry: 3, + refetchOnWindowFocus: false, // 根据需求决定 + }, + }, +}); + +// === 使用 queryOptions (v5 新增) === + +// ❌ 重复定义 queryKey 和 queryFn +function Component1() { + const { data } = useQuery({ + queryKey: ['users', userId], + queryFn: () => fetchUser(userId), + }); +} + +function prefetchUser(queryClient, userId) { + queryClient.prefetchQuery({ + queryKey: ['users', userId], // 重复！ + queryFn: () => fetchUser(userId), // 重复！ + }); +} + +// ✅ queryOptions 统一定义，类型安全 +import { queryOptions } from '@tanstack/react-query'; + +const userQueryOptions = (userId: string) => + queryOptions({ + queryKey: ['users', userId], + queryFn: () => fetchUser(userId), + }); + +function Component1({ userId }) { + const { data } = useQuery(userQueryOptions(userId)); +} + +function prefetchUser(queryClient, userId) { + queryClient.prefetchQuery(userQueryOptions(userId)); +} + +// getQueryData 也是类型安全的 +const user = queryClient.getQueryData(userQueryOptions(userId).queryKey); + +// === 避免常见陷阱 === + +// ❌ staleTime 为 0 导致过度请求 +useQuery({ + queryKey: ['data'], + queryFn: fetchData, + // staleTime 默认为 0，每次组件挂载都会 refetch +}); + +// ✅ 设置合理的 staleTime +useQuery({ + queryKey: ['data'], + queryFn: fetchData, + staleTime: 1000 * 60, // 1 分钟内不会重新请求 +}); + +// ❌ 在 queryFn 中使用不稳定的引用 +function BadQuery({ filters }) { + useQuery({ + queryKey: ['items'], // queryKey 没有包含 filters！ + queryFn: () => fetchItems(filters), // filters 变化不会触发重新请求 + }); +} + +// ✅ queryKey 包含所有影响数据的参数 +function GoodQuery({ filters }) { + useQuery({ + queryKey: ['items', filters], // filters 是 queryKey 的一部分 + queryFn: () => fetchItems(filters), + }); +} + +// === Suspense 模式 (v5 稳定) === + +// ❌ 使用 useQuery + enabled 实现条件查询 +function BadSuspenseQuery({ userId }) { + const { data } = useSuspenseQuery({ + queryKey: ['user', userId], + queryFn: () => fetchUser(userId), + enabled: !!userId, // useSuspenseQuery 不支持 enabled！ + }); +} + +// ✅ 组件组合实现条件渲染 +function GoodSuspenseQuery({ userId }) { + // useSuspenseQuery 保证 data 是 T 不是 T | undefined + const { data } = useSuspenseQuery({ + queryKey: ['user', userId], + queryFn: () => fetchUser(userId), + }); + return ; +} + +function Parent({ userId }) { + if (!userId) return ; + return ( + }> + + + ); +} + +// === 乐观更新 (v5 简化) === + +// ❌ 手动管理缓存的乐观更新（复杂） +const mutation = useMutation({ + mutationFn: updateTodo, + onMutate: async (newTodo) => { + await queryClient.cancelQueries({ queryKey: ['todos'] }); + const previousTodos = queryClient.getQueryData(['todos']); + queryClient.setQueryData(['todos'], (old) => [...old, newTodo]); + return { previousTodos }; + }, + onError: (err, newTodo, context) => { + queryClient.setQueryData(['todos'], context.previousTodos); + }, + onSettled: () => { + queryClient.invalidateQueries({ queryKey: ['todos'] }); + }, +}); + +// ✅ v5 简化：使用 variables 进行乐观 UI +function TodoList() { + const { data: todos } = useQuery(todosQueryOptions); + const { mutate, variables, isPending } = useMutation({ + mutationFn: addTodo, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['todos'] }); + }, + }); + + return ( +
    + {todos?.map(todo => )} + {/* 乐观显示正在添加的 todo */} + {isPending && } +
+ ); +} + +// === 状态字段变化 (v5) === + +// v4: isLoading 表示首次加载或后续获取 +// v5: isPending 表示没有数据，isLoading = isPending && isFetching + +const { data, isPending, isFetching, isLoading } = useQuery({...}); + +// isPending: 缓存中没有数据（首次加载） +// isFetching: 正在请求中（包括后台刷新） +// isLoading: isPending && isFetching（首次加载中） + +// ❌ v4 代码直接迁移 +if (isLoading) return ; // v5 中行为可能不同 + +// ✅ 明确意图 +if (isPending) return ; // 没有数据时显示加载 +// 或 +if (isLoading) return ; // 首次加载中 +``` + +#### TanStack Query Checklist + +```markdown +## 查询配置 +- [ ] 使用 queryOptions 统一定义查询 +- [ ] queryKey 包含所有影响数据的参数 +- [ ] 设置合理的 staleTime（不是默认的 0） +- [ ] gcTime（原 cacheTime）根据需求配置 + +## Suspense 模式 +- [ ] 使用 useSuspenseQuery（不是 useQuery + suspense: true） +- [ ] 不在 useSuspenseQuery 中使用 enabled +- [ ] 条件查询通过组件组合实现 + +## Mutations +- [ ] 成功后 invalidateQueries 相关查询 +- [ ] 使用 variables 实现简单乐观更新 +- [ ] 复杂场景使用 onMutate/onError/onSettled + +## 性能 +- [ ] 避免过多 useQuery 调用（>100 个查询可能影响性能） +- [ ] 大列表考虑分页或虚拟化 +- [ ] 使用 select 只订阅需要的数据 + +## v5 迁移 +- [ ] cacheTime → gcTime +- [ ] 理解 isPending vs isLoading 区别 +- [ ] useQuery 单一对象参数（移除重载） +``` + +### Vue 3 Code Review + +```vue + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +#### Vue 3 Review Checklist + +```markdown +## 响应性系统 +- [ ] ref 用于基本类型，reactive 用于对象 +- [ ] 没有解构 reactive 对象（或使用了 toRefs） +- [ ] props 传递给 composable 时保持了响应性 +- [ ] shallowRef/shallowReactive 用于大型对象优化 + +## Props & Emits +- [ ] defineProps 使用 TypeScript 类型声明 +- [ ] 复杂默认值使用 withDefaults + 工厂函数 +- [ ] defineEmits 有完整的类型定义 +- [ ] 没有直接修改 props + +## Watchers +- [ ] watch/watchEffect 有适当的清理函数 +- [ ] 异步 watch 处理了竞态条件 +- [ ] flush: 'post' 用于 DOM 操作的 watcher +- [ ] 避免过度使用 watcher（优先用 computed） + +## 模板 +- [ ] v-for 使用唯一且稳定的 key +- [ ] v-if 和 v-for 没有在同一元素上 +- [ ] 事件处理使用 kebab-case +- [ ] 大型列表使用虚拟滚动 + +## Composables +- [ ] 相关逻辑提取到 composables +- [ ] composables 返回响应式引用（不是 .value） +- [ ] 纯函数不要包装成 composable +- [ ] 副作用在组件卸载时清理 + +## 性能 +- [ ] 大型组件拆分为小组件 +- [ ] 使用 defineAsyncComponent 懒加载 +- [ ] 避免不必要的响应式转换 +- [ ] v-memo 用于昂贵的列表渲染 +``` + +### Rust Code Review + +Rust 审查的重点：编译器能捕获内存安全问题，但审查者需要关注编译器无法检测的问题——业务逻辑、API 设计、性能和可维护性。 + +```rust +// === 所有权与借用 === + +// ❌ clone() 是"Rust 的胶带"——用于绕过借用检查器 +fn bad_process(data: &Data) -> Result<()> { + let owned = data.clone(); // 为什么需要 clone？ + expensive_operation(owned) +} + +// ✅ 审查时问：clone 是否必要？能否用借用？ +fn good_process(data: &Data) -> Result<()> { + expensive_operation(data) // 传递引用 +} + +// ❌ Arc> 可能隐藏不必要的共享状态 +struct BadService { + cache: Arc>>, // 真的需要共享？ +} + +// ✅ 考虑是否需要共享，或者设计可以避免 +struct GoodService { + cache: HashMap, // 单一所有者 +} + +// === Unsafe 代码审查（最关键！）=== + +// ❌ unsafe 没有安全文档——这是红旗 +unsafe fn bad_transmute(t: T) -> U { + std::mem::transmute(t) +} + +// ✅ 每个 unsafe 必须解释：为什么安全？什么不变量？ +/// Transmutes `T` to `U`. +/// +/// # Safety +/// +/// - `T` and `U` must have the same size and alignment +/// - `T` must be a valid bit pattern for `U` +/// - The caller ensures no references to `t` exist after this call +unsafe fn documented_transmute(t: T) -> U { + // SAFETY: Caller guarantees size/alignment match and bit validity + std::mem::transmute(t) +} + +// === 异步代码 === + +// ❌ 在 async 上下文中阻塞——会饿死其他任务 +async fn bad_async() { + let data = std::fs::read_to_string("file.txt").unwrap(); // 阻塞！ + std::thread::sleep(Duration::from_secs(1)); // 阻塞！ +} + +// ✅ 使用异步 API +async fn good_async() { + let data = tokio::fs::read_to_string("file.txt").await?; + tokio::time::sleep(Duration::from_secs(1)).await; +} + +// ❌ 跨 .await 持有 std::sync::Mutex——可能死锁 +async fn bad_lock(mutex: &std::sync::Mutex) { + let guard = mutex.lock().unwrap(); + async_operation().await; // 持锁等待！ + process(&guard); +} + +// ✅ 最小化锁范围，或使用 tokio::sync::Mutex +async fn good_lock(mutex: &std::sync::Mutex) { + let data = mutex.lock().unwrap().clone(); // 立即释放 + async_operation().await; + process(&data); +} + +// === 错误处理 === + +// ❌ 库代码用 anyhow——调用者无法 match 错误 +pub fn parse_config(s: &str) -> anyhow::Result { ... } + +// ✅ 库用 thiserror，应用用 anyhow +#[derive(Debug, thiserror::Error)] +pub enum ConfigError { + #[error("invalid syntax at line {line}: {message}")] + Syntax { line: usize, message: String }, + #[error("missing required field: {0}")] + MissingField(String), + #[error(transparent)] + Io(#[from] std::io::Error), +} + +pub fn parse_config(s: &str) -> Result { ... } + +// ❌ 吞掉错误上下文 +fn bad_error() -> Result<()> { + operation().map_err(|_| anyhow!("failed"))?; // 原始错误丢失 + Ok(()) +} + +// ✅ 使用 context 保留错误链 +fn good_error() -> Result<()> { + operation().context("failed to perform operation")?; + Ok(()) +} + +// === 性能 === + +// ❌ 不必要的 collect——中间分配 +fn bad_sum(items: &[i32]) -> i32 { + items.iter() + .filter(|x| **x > 0) + .collect::>() // 不必要！ + .iter() + .sum() +} + +// ✅ 惰性迭代 +fn good_sum(items: &[i32]) -> i32 { + items.iter().filter(|x| **x > 0).sum() +} + +// ❌ 字符串拼接在循环中重复分配 +fn bad_concat(items: &[&str]) -> String { + let mut s = String::new(); + for item in items { + s = s + item; // 每次都重新分配！ + } + s +} + +// ✅ 预分配或用 join +fn good_concat(items: &[&str]) -> String { + items.join("") // 或用 with_capacity +} + +// === Trait 设计 === + +// ❌ 过度抽象——不是 Java，不需要 Interface 一切 +trait Processor { fn process(&self); } +trait Handler { fn handle(&self); } +trait Manager { fn manage(&self); } // Trait 过多 + +// ✅ 只在需要多态时创建 trait +// 具体类型通常更简单、更快 +``` + +#### Rust Review Checklist + +```markdown +## 编译器不能捕获的问题 + +**业务逻辑正确性** +- [ ] 边界条件处理正确 +- [ ] 状态机转换完整 +- [ ] 并发场景下的竞态条件 + +**API 设计** +- [ ] 公共 API 难以误用 +- [ ] 类型签名清晰表达意图 +- [ ] 错误类型粒度合适 + +## 所有权与借用 + +- [ ] clone() 是有意为之，文档说明了原因 +- [ ] Arc> 真的需要共享状态吗？ +- [ ] RefCell 的使用有正当理由 +- [ ] 生命周期不过度复杂 + +## Unsafe 代码（最重要） + +- [ ] 每个 unsafe 块有 SAFETY 注释 +- [ ] unsafe fn 有 # Safety 文档节 +- [ ] 解释了为什么是安全的，不只是做什么 +- [ ] 列出了必须维护的不变量 +- [ ] unsafe 边界尽可能小 +- [ ] 考虑过是否有 safe 替代方案 + +## 异步/并发 + +- [ ] 没有在 async 中阻塞（std::fs、thread::sleep） +- [ ] 没有跨 .await 持有 std::sync 锁 +- [ ] spawn 的任务满足 'static +- [ ] 锁的获取顺序一致 +- [ ] Channel 缓冲区大小合理 + +## 错误处理 + +- [ ] 库：thiserror 定义结构化错误 +- [ ] 应用：anyhow + context +- [ ] 没有生产代码 unwrap/expect +- [ ] 错误消息对调试有帮助 +- [ ] must_use 返回值被处理 + +## 性能 + +- [ ] 避免不必要的 collect() +- [ ] 大数据传引用 +- [ ] 字符串用 with_capacity 或 write! +- [ ] impl Trait vs Box 选择合理 +- [ ] 热路径避免分配 + +## 代码质量 + +- [ ] cargo clippy 零警告 +- [ ] cargo fmt 格式化 +- [ ] 文档注释完整 +- [ ] 测试覆盖边界条件 +- [ ] 公共 API 有文档示例 +``` + +## Advanced Review Patterns + +### Pattern 1: Architectural Review + +```markdown +When reviewing significant changes: + +1. **Design Document First** + - For large features, request design doc before code + - Review design with team before implementation + - Agree on approach to avoid rework + +2. **Review in Stages** + - First PR: Core abstractions and interfaces + - Second PR: Implementation + - Third PR: Integration and tests + - Easier to review, faster to iterate + +3. **Consider Alternatives** + - "Have we considered using [pattern/library]?" + - "What's the tradeoff vs. the simpler approach?" + - "How will this evolve as requirements change?" +``` + +### Pattern 2: Test Quality Review + +```typescript +// ❌ Poor test: Implementation detail testing +test('increments counter variable', () => { + const component = render(); + const button = component.getByRole('button'); + fireEvent.click(button); + expect(component.state.counter).toBe(1); // Testing internal state +}); + +// ✅ Good test: Behavior testing +test('displays incremented count when clicked', () => { + render(); + const button = screen.getByRole('button', { name: /increment/i }); + fireEvent.click(button); + expect(screen.getByText('Count: 1')).toBeInTheDocument(); +}); + +// Review questions for tests: +// - Do tests describe behavior, not implementation? +// - Are test names clear and descriptive? +// - Do tests cover edge cases? +// - Are tests independent (no shared state)? +// - Can tests run in any order? +``` + +### Pattern 3: Security Review + +```markdown +## Security Review Checklist + +### Authentication & Authorization +- [ ] Is authentication required where needed? +- [ ] Are authorization checks before every action? +- [ ] Is JWT validation proper (signature, expiry)? +- [ ] Are API keys/secrets properly secured? + +### Input Validation +- [ ] All user inputs validated? +- [ ] File uploads restricted (size, type)? +- [ ] SQL queries parameterized? +- [ ] XSS protection (escape output)? + +### Data Protection +- [ ] Passwords hashed (bcrypt/argon2)? +- [ ] Sensitive data encrypted at rest? +- [ ] HTTPS enforced for sensitive data? +- [ ] PII handled according to regulations? + +### Common Vulnerabilities +- [ ] No eval() or similar dynamic execution? +- [ ] No hardcoded secrets? +- [ ] CSRF protection for state-changing operations? +- [ ] Rate limiting on public endpoints? +``` + +## Giving Difficult Feedback + +### Pattern: The Sandwich Method (Modified) + +```markdown +Traditional: Praise + Criticism + Praise (feels fake) + +Better: Context + Specific Issue + Helpful Solution + +Example: +"I noticed the payment processing logic is inline in the +controller. This makes it harder to test and reuse. + +[Specific Issue] +The calculateTotal() function mixes tax calculation, +discount logic, and database queries, making it difficult +to unit test and reason about. + +[Helpful Solution] +Could we extract this into a PaymentService class? That +would make it testable and reusable. I can pair with you +on this if helpful." +``` + +### Handling Disagreements + +```markdown +When author disagrees with your feedback: + +1. **Seek to Understand** + "Help me understand your approach. What led you to + choose this pattern?" + +2. **Acknowledge Valid Points** + "That's a good point about X. I hadn't considered that." + +3. **Provide Data** + "I'm concerned about performance. Can we add a benchmark + to validate the approach?" + +4. **Escalate if Needed** + "Let's get [architect/senior dev] to weigh in on this." + +5. **Know When to Let Go** + If it's working and not a critical issue, approve it. + Perfection is the enemy of progress. +``` + +## Best Practices + +1. **Review Promptly**: Within 24 hours, ideally same day +2. **Limit PR Size**: 200-400 lines max for effective review +3. **Review in Time Blocks**: 60 minutes max, take breaks +4. **Use Review Tools**: GitHub, GitLab, or dedicated tools +5. **Automate What You Can**: Linters, formatters, security scans +6. **Build Rapport**: Emoji, praise, and empathy matter +7. **Be Available**: Offer to pair on complex issues +8. **Learn from Others**: Review others' review comments + +## Common Pitfalls + +- **Perfectionism**: Blocking PRs for minor style preferences +- **Scope Creep**: "While you're at it, can you also..." +- **Inconsistency**: Different standards for different people +- **Delayed Reviews**: Letting PRs sit for days +- **Ghosting**: Requesting changes then disappearing +- **Rubber Stamping**: Approving without actually reviewing +- **Bike Shedding**: Debating trivial details extensively + +## Templates + +### PR Review Comment Template + +```markdown +## Summary +[Brief overview of what was reviewed] + +## Strengths +- [What was done well] +- [Good patterns or approaches] + +## Required Changes +🔴 [Blocking issue 1] +🔴 [Blocking issue 2] + +## Suggestions +💡 [Improvement 1] +💡 [Improvement 2] + +## Questions +❓ [Clarification needed on X] +❓ [Alternative approach consideration] + +## Verdict +✅ Approve after addressing required changes +``` + +## Resources + +- **references/code-review-best-practices.md**: Comprehensive review guidelines +- **references/common-bugs-checklist.md**: Language-specific bugs to watch for +- **references/security-review-guide.md**: Security-focused review checklist +- **assets/pr-review-template.md**: Standard review comment template +- **assets/review-checklist.md**: Quick reference checklist +- **scripts/pr-analyzer.py**: Analyze PR complexity and suggest reviewers diff --git a/assets/pr-review-template.md b/assets/pr-review-template.md new file mode 100644 index 0000000..33f7e71 --- /dev/null +++ b/assets/pr-review-template.md @@ -0,0 +1,114 @@ +# PR Review Template + +Copy and use this template for your code reviews. + +--- + +## Summary + +[Brief overview of what was reviewed - 1-2 sentences] + +**PR Size:** [Small/Medium/Large] (~X lines) +**Review Time:** [X minutes] + +## Strengths + +- [What was done well] +- [Good patterns or approaches used] +- [Improvements from previous code] + +## Required Changes + +🔴 **[blocking]** [Issue description] +> [Code location or example] +> [Suggested fix or explanation] + +🔴 **[blocking]** [Issue description] +> [Details] + +## Important Suggestions + +🟡 **[important]** [Issue description] +> [Why this matters] +> [Suggested approach] + +## Minor Suggestions + +🟢 **[nit]** [Minor improvement suggestion] + +💡 **[suggestion]** [Alternative approach to consider] + +## Questions + +❓ [Clarification needed about X] + +❓ [Question about design decision Y] + +## Security Considerations + +- [ ] No hardcoded secrets +- [ ] Input validation present +- [ ] Authorization checks in place +- [ ] No SQL/XSS injection risks + +## Test Coverage + +- [ ] Unit tests added/updated +- [ ] Edge cases covered +- [ ] Error cases tested + +## Verdict + +**[ ] ✅ Approve** - Ready to merge +**[ ] 💬 Comment** - Minor suggestions, can merge +**[ ] 🔄 Request Changes** - Must address blocking issues + +--- + +## Quick Copy Templates + +### Blocking Issue +``` +🔴 **[blocking]** [Title] + +[Description of the issue] + +**Location:** `file.ts:123` + +**Suggested fix:** +\`\`\`typescript +// Your suggested code +\`\`\` +``` + +### Important Suggestion +``` +🟡 **[important]** [Title] + +[Why this is important] + +**Consider:** +- Option A: [description] +- Option B: [description] +``` + +### Minor Suggestion +``` +🟢 **[nit]** [Suggestion] + +Not blocking, but consider [improvement]. +``` + +### Praise +``` +🎉 **[praise]** Great work on [specific thing]! + +[Why this is good] +``` + +### Question +``` +❓ **[question]** [Your question] + +I'm curious about the decision to [X]. Could you explain [Y]? +``` diff --git a/assets/review-checklist.md b/assets/review-checklist.md new file mode 100644 index 0000000..4ff3b8b --- /dev/null +++ b/assets/review-checklist.md @@ -0,0 +1,121 @@ +# Code Review Quick Checklist + +Quick reference checklist for code reviews. + +## Pre-Review (2 min) + +- [ ] Read PR description and linked issue +- [ ] Check PR size (<400 lines ideal) +- [ ] Verify CI/CD status (tests passing?) +- [ ] Understand the business requirement + +## Architecture & Design (5 min) + +- [ ] Solution fits the problem +- [ ] Consistent with existing patterns +- [ ] No simpler approach exists +- [ ] Will it scale? +- [ ] Changes in right location + +## Logic & Correctness (10 min) + +- [ ] Edge cases handled +- [ ] Null/undefined checks present +- [ ] Off-by-one errors checked +- [ ] Race conditions considered +- [ ] Error handling complete +- [ ] Correct data types used + +## Security (5 min) + +- [ ] No hardcoded secrets +- [ ] Input validated/sanitized +- [ ] SQL injection prevented +- [ ] XSS prevented +- [ ] Authorization checks present +- [ ] Sensitive data protected + +## Performance (3 min) + +- [ ] No N+1 queries +- [ ] Expensive operations optimized +- [ ] Large lists paginated +- [ ] No memory leaks +- [ ] Caching considered where appropriate + +## Testing (5 min) + +- [ ] Tests exist for new code +- [ ] Edge cases tested +- [ ] Error cases tested +- [ ] Tests are readable +- [ ] Tests are deterministic + +## Code Quality (3 min) + +- [ ] Clear variable/function names +- [ ] No code duplication +- [ ] Functions do one thing +- [ ] Complex code commented +- [ ] No magic numbers + +## Documentation (2 min) + +- [ ] Public APIs documented +- [ ] README updated if needed +- [ ] Breaking changes noted +- [ ] Complex logic explained + +--- + +## Severity Labels + +| Label | Meaning | Action | +|-------|---------|--------| +| 🔴 `[blocking]` | Must fix | Block merge | +| 🟡 `[important]` | Should fix | Discuss if disagree | +| 🟢 `[nit]` | Nice to have | Non-blocking | +| 💡 `[suggestion]` | Alternative | Consider | +| ❓ `[question]` | Need clarity | Respond | +| 🎉 `[praise]` | Good work | Celebrate! | + +--- + +## Decision Matrix + +| Situation | Decision | +|-----------|----------| +| Critical security issue | 🔴 Block, fix immediately | +| Breaking change without migration | 🔴 Block | +| Missing error handling | 🟡 Should fix | +| No tests for new code | 🟡 Should fix | +| Style preference | 🟢 Non-blocking | +| Minor naming improvement | 🟢 Non-blocking | +| Clever but working code | 💡 Suggest simpler | + +--- + +## Time Budget + +| PR Size | Target Time | +|---------|-------------| +| < 100 lines | 10-15 min | +| 100-400 lines | 20-40 min | +| > 400 lines | Ask to split | + +--- + +## Red Flags + +Watch for these patterns: + +- `// TODO` in production code +- `console.log` left in code +- Commented out code +- `any` type in TypeScript +- Empty catch blocks +- `unwrap()` in Rust production code +- Magic numbers/strings +- Copy-pasted code blocks +- Missing null checks +- Hardcoded URLs/credentials diff --git a/references/code-review-best-practices.md b/references/code-review-best-practices.md new file mode 100644 index 0000000..8c6b9cd --- /dev/null +++ b/references/code-review-best-practices.md @@ -0,0 +1,136 @@ +# Code Review Best Practices + +Comprehensive guidelines for conducting effective code reviews. + +## Review Philosophy + +### Goals of Code Review + +**Primary Goals:** +- Catch bugs and edge cases before production +- Ensure code maintainability and readability +- Share knowledge across the team +- Enforce coding standards consistently +- Improve design and architecture decisions + +**Secondary Goals:** +- Mentor junior developers +- Build team culture and trust +- Document design decisions through discussions + +### What Code Review is NOT + +- A gatekeeping mechanism to block progress +- An opportunity to show off knowledge +- A place to nitpick formatting (use linters) +- A way to rewrite code to personal preference + +## Review Timing + +### When to Review + +| Trigger | Action | +|---------|--------| +| PR opened | Review within 24 hours, ideally same day | +| Changes requested | Re-review within 4 hours | +| Blocking issue found | Communicate immediately | + +### Time Allocation + +- **Small PR (<100 lines)**: 10-15 minutes +- **Medium PR (100-400 lines)**: 20-40 minutes +- **Large PR (>400 lines)**: Request to split, or 60+ minutes + +## Review Depth Levels + +### Level 1: Skim Review (5 minutes) +- Check PR description and linked issues +- Verify CI/CD status +- Look at file changes overview +- Identify if deeper review needed + +### Level 2: Standard Review (20-30 minutes) +- Full code walkthrough +- Logic verification +- Test coverage check +- Security scan + +### Level 3: Deep Review (60+ minutes) +- Architecture evaluation +- Performance analysis +- Security audit +- Edge case exploration + +## Communication Guidelines + +### Tone and Language + +**Use collaborative language:** +- "What do you think about..." instead of "You should..." +- "Could we consider..." instead of "This is wrong" +- "I'm curious about..." instead of "Why didn't you..." + +**Be specific and actionable:** +- Include code examples when suggesting changes +- Link to documentation or past discussions +- Explain the "why" behind suggestions + +### Handling Disagreements + +1. **Seek to understand**: Ask clarifying questions +2. **Acknowledge valid points**: Show you've considered their perspective +3. **Provide data**: Use benchmarks, docs, or examples +4. **Escalate if needed**: Involve senior dev or architect +5. **Know when to let go**: Not every hill is worth dying on + +## Review Prioritization + +### Must Fix (Blocking) +- Security vulnerabilities +- Data corruption risks +- Breaking changes without migration +- Critical performance issues +- Missing error handling for user-facing features + +### Should Fix (Important) +- Test coverage gaps +- Moderate performance concerns +- Code duplication +- Unclear naming or structure +- Missing documentation for complex logic + +### Nice to Have (Non-blocking) +- Style preferences beyond linting +- Minor optimizations +- Additional test cases +- Documentation improvements + +## Anti-Patterns to Avoid + +### Reviewer Anti-Patterns +- **Rubber stamping**: Approving without actually reviewing +- **Bike shedding**: Debating trivial details extensively +- **Scope creep**: "While you're at it, can you also..." +- **Ghosting**: Requesting changes then disappearing +- **Perfectionism**: Blocking for minor style preferences + +### Author Anti-Patterns +- **Mega PRs**: Submitting 1000+ line changes +- **No context**: Missing PR description or linked issues +- **Defensive responses**: Arguing every suggestion +- **Silent updates**: Making changes without responding to comments + +## Metrics and Improvement + +### Track These Metrics +- Time to first review +- Review cycle time +- Number of review rounds +- Defect escape rate +- Review coverage percentage + +### Continuous Improvement +- Hold retrospectives on review process +- Share learnings from escaped bugs +- Update checklists based on common issues +- Celebrate good reviews and catches diff --git a/references/common-bugs-checklist.md b/references/common-bugs-checklist.md new file mode 100644 index 0000000..97e2e63 --- /dev/null +++ b/references/common-bugs-checklist.md @@ -0,0 +1,1227 @@ +# Common Bugs Checklist + +Language-specific bugs and issues to watch for during code review. + +## Universal Issues + +### Logic Errors +- [ ] Off-by-one errors in loops and array access +- [ ] Incorrect boolean logic (De Morgan's law violations) +- [ ] Missing null/undefined checks +- [ ] Race conditions in concurrent code +- [ ] Incorrect comparison operators (== vs ===, = vs ==) +- [ ] Integer overflow/underflow +- [ ] Floating point comparison issues + +### Resource Management +- [ ] Memory leaks (unclosed connections, listeners) +- [ ] File handles not closed +- [ ] Database connections not released +- [ ] Event listeners not removed +- [ ] Timers/intervals not cleared + +### Error Handling +- [ ] Swallowed exceptions (empty catch blocks) +- [ ] Generic exception handling hiding specific errors +- [ ] Missing error propagation +- [ ] Incorrect error types thrown +- [ ] Missing finally/cleanup blocks + +## TypeScript/JavaScript + +### Type Issues +```typescript +// ❌ Using any defeats type safety +function process(data: any) { return data.value; } + +// ✅ Use proper types +interface Data { value: string; } +function process(data: Data) { return data.value; } +``` + +### Async/Await Pitfalls +```typescript +// ❌ Missing await +async function fetch() { + const data = fetchData(); // Missing await! + return data.json(); +} + +// ❌ Unhandled promise rejection +async function risky() { + const result = await fetchData(); // No try-catch + return result; +} + +// ✅ Proper error handling +async function safe() { + try { + const result = await fetchData(); + return result; + } catch (error) { + console.error('Fetch failed:', error); + throw error; + } +} +``` + +### React Specific + +#### Hooks 规则违反 +```tsx +// ❌ 条件调用 Hooks — 违反 Hooks 规则 +function BadComponent({ show }) { + if (show) { + const [value, setValue] = useState(0); // Error! + } + return
...
; +} + +// ✅ Hooks 必须在顶层无条件调用 +function GoodComponent({ show }) { + const [value, setValue] = useState(0); + if (!show) return null; + return
{value}
; +} + +// ❌ 循环中调用 Hooks +function BadLoop({ items }) { + items.forEach(item => { + const [selected, setSelected] = useState(false); // Error! + }); +} + +// ✅ 将状态提升或使用不同的数据结构 +function GoodLoop({ items }) { + const [selectedIds, setSelectedIds] = useState>(new Set()); + return items.map(item => ( + + )); +} +``` + +#### useEffect 常见错误 +```tsx +// ❌ 依赖数组不完整 — stale closure +function StaleClosureExample({ userId, onSuccess }) { + const [data, setData] = useState(null); + useEffect(() => { + fetchData(userId).then(result => { + setData(result); + onSuccess(result); // onSuccess 可能是 stale 的！ + }); + }, [userId]); // 缺少 onSuccess 依赖 +} + +// ✅ 完整的依赖数组 +useEffect(() => { + fetchData(userId).then(result => { + setData(result); + onSuccess(result); + }); +}, [userId, onSuccess]); + +// ❌ 无限循环 — 在 effect 中更新依赖 +function InfiniteLoop() { + const [count, setCount] = useState(0); + useEffect(() => { + setCount(count + 1); // 触发重渲染，又触发 effect + }, [count]); // 无限循环！ +} + +// ❌ 缺少清理函数 — 内存泄漏 +function MemoryLeak({ userId }) { + const [user, setUser] = useState(null); + useEffect(() => { + fetchUser(userId).then(setUser); // 组件卸载后仍然调用 setUser + }, [userId]); +} + +// ✅ 正确的清理 +function NoLeak({ userId }) { + const [user, setUser] = useState(null); + useEffect(() => { + let cancelled = false; + fetchUser(userId).then(data => { + if (!cancelled) setUser(data); + }); + return () => { cancelled = true; }; + }, [userId]); +} + +// ❌ useEffect 用于派生状态（反模式） +function BadDerived({ items }) { + const [total, setTotal] = useState(0); + useEffect(() => { + setTotal(items.reduce((a, b) => a + b.price, 0)); + }, [items]); // 不必要的 effect + 额外渲染 +} + +// ✅ 直接计算或用 useMemo +function GoodDerived({ items }) { + const total = useMemo( + () => items.reduce((a, b) => a + b.price, 0), + [items] + ); +} + +// ❌ useEffect 用于事件响应 +function BadEvent() { + const [query, setQuery] = useState(''); + useEffect(() => { + if (query) logSearch(query); // 应该在事件处理器中 + }, [query]); +} + +// ✅ 副作用在事件处理器中 +function GoodEvent() { + const handleSearch = (q: string) => { + setQuery(q); + logSearch(q); + }; +} +``` + +#### useMemo / useCallback 误用 +```tsx +// ❌ 过度优化 — 常量不需要 memo +function OverOptimized() { + const config = useMemo(() => ({ api: '/v1' }), []); // 无意义 + const noop = useCallback(() => {}, []); // 无意义 +} + +// ❌ 空依赖的 useMemo（可能隐藏 bug） +function EmptyDeps({ user }) { + const greeting = useMemo(() => `Hello ${user.name}`, []); + // user 变化时 greeting 不更新！ +} + +// ❌ useCallback 依赖总是变化 +function UselessCallback({ data }) { + const process = useCallback(() => { + return data.map(transform); + }, [data]); // 如果 data 每次都是新引用，完全无效 +} + +// ❌ useMemo/useCallback 没有配合 React.memo +function Parent() { + const data = useMemo(() => compute(), []); + const handler = useCallback(() => {}, []); + return ; + // Child 没有用 React.memo，这些优化毫无意义 +} + +// ✅ 正确的优化组合 +const MemoChild = React.memo(function Child({ data, onClick }) { + return ; +}); + +function Parent() { + const data = useMemo(() => expensiveCompute(), [dep]); + const handler = useCallback(() => {}, []); + return ; +} +``` + +#### 组件设计问题 +```tsx +// ❌ 在组件内定义组件 +function Parent() { + // 每次渲染都创建新的 Child 函数，导致完全重新挂载 + const Child = () =>
child
; + return ; +} + +// ✅ 组件定义在外部 +const Child = () =>
child
; +function Parent() { + return ; +} + +// ❌ Props 总是新引用 — 破坏 memo +function BadProps() { + return ( + handle()} // 每次渲染新函数 + items={data.filter(x => x)} // 每次渲染新数组 + /> + ); +} + +// ❌ 直接修改 props +function MutateProps({ user }) { + user.name = 'Changed'; // 永远不要这样做！ + return
{user.name}
; +} +``` + +#### Server Components 错误 (React 19+) +```tsx +// ❌ 在 Server Component 中使用客户端 API +// app/page.tsx (默认是 Server Component) +export default function Page() { + const [count, setCount] = useState(0); // Error! + useEffect(() => {}, []); // Error! + return ; // Error! +} + +// ✅ 交互逻辑移到 Client Component +// app/counter.tsx +'use client'; +export function Counter() { + const [count, setCount] = useState(0); + return ; +} + +// app/page.tsx +import { Counter } from './counter'; +export default async function Page() { + const data = await fetchData(); // Server Component 可以直接 await + return ; +} + +// ❌ 在父组件标记 'use client'，整个子树变成客户端 +// layout.tsx +'use client'; // 坏主意！所有子组件都变成客户端组件 +export default function Layout({ children }) { ... } +``` + +#### 测试常见错误 +```tsx +// ❌ 使用 container 查询 +const { container } = render(); +const button = container.querySelector('button'); // 不推荐 + +// ✅ 使用 screen 和语义查询 +render(); +const button = screen.getByRole('button', { name: /submit/i }); + +// ❌ 使用 fireEvent +fireEvent.click(button); + +// ✅ 使用 userEvent +await userEvent.click(button); + +// ❌ 测试实现细节 +expect(component.state.isOpen).toBe(true); + +// ✅ 测试行为 +expect(screen.getByRole('dialog')).toBeVisible(); + +// ❌ 等待同步查询 +await screen.getByText('Hello'); // getBy 是同步的 + +// ✅ 异步用 findBy +await screen.findByText('Hello'); // findBy 会等待 +``` + +### React Common Mistakes Checklist +- [ ] Hooks 不在顶层调用（条件/循环中） +- [ ] useEffect 依赖数组不完整 +- [ ] useEffect 缺少清理函数 +- [ ] useEffect 用于派生状态计算 +- [ ] useMemo/useCallback 过度使用 +- [ ] useMemo/useCallback 没配合 React.memo +- [ ] 在组件内定义子组件 +- [ ] Props 是新对象/函数引用（传给 memo 组件时） +- [ ] 直接修改 props +- [ ] 列表缺少 key 或用 index 作为 key +- [ ] Server Component 使用客户端 API +- [ ] 'use client' 放在父组件导致整个树客户端化 +- [ ] 测试使用 container 查询而非 screen +- [ ] 测试实现细节而非行为 + +### React 19 Actions & Forms 错误 + +```tsx +// === useActionState 错误 === + +// ❌ 在 Action 中直接 setState 而不是返回状态 +const [state, action] = useActionState(async (prev, formData) => { + setSomeState(newValue); // 错误！应该返回新状态 +}, initialState); + +// ✅ 返回新状态 +const [state, action] = useActionState(async (prev, formData) => { + const result = await submitForm(formData); + return { ...prev, data: result }; // 返回新状态 +}, initialState); + +// ❌ 忘记处理 isPending +const [state, action] = useActionState(submitAction, null); +return ; // 用户可以重复点击 + +// ✅ 使用 isPending 禁用按钮 +const [state, action, isPending] = useActionState(submitAction, null); +return ; + +// === useFormStatus 错误 === + +// ❌ 在 form 同级调用 useFormStatus +function Form() { + const { pending } = useFormStatus(); // 永远是 undefined！ + return ; +} + +// ✅ 在子组件中调用 +function SubmitButton() { + const { pending } = useFormStatus(); + return ; +} +function Form() { + return
; +} + +// === useOptimistic 错误 === + +// ❌ 用于关键业务操作 +function PaymentButton() { + const [optimisticPaid, setPaid] = useOptimistic(false); + const handlePay = async () => { + setPaid(true); // 危险：显示已支付但可能失败 + await processPayment(); + }; +} + +// ❌ 没有处理回滚后的 UI 状态 +const [optimisticLikes, addLike] = useOptimistic(likes); +// 失败后 UI 回滚，但用户可能困惑为什么点赞消失了 + +// ✅ 提供失败反馈 +const handleLike = async () => { + addLike(1); + try { + await likePost(); + } catch { + toast.error('点赞失败，请重试'); // 通知用户 + } +}; +``` + +### React 19 Forms Checklist +- [ ] useActionState 返回新状态而不是 setState +- [ ] useActionState 正确使用 isPending 禁用提交 +- [ ] useFormStatus 在 form 子组件中调用 +- [ ] useOptimistic 不用于关键业务（支付、删除等） +- [ ] useOptimistic 失败时有用户反馈 +- [ ] Server Action 正确标记 'use server' + +### Suspense & Streaming 错误 + +```tsx +// === Suspense 边界错误 === + +// ❌ 整个页面一个 Suspense——慢内容阻塞快内容 +function BadPage() { + return ( + }> + {/* 快 */} + {/* 慢——阻塞整个页面 */} + {/* 快 */} + + ); +} + +// ✅ 独立边界，互不阻塞 +function GoodPage() { + return ( + <> + + }> + + + + + ); +} + +// ❌ 没有 Error Boundary +function NoErrorHandling() { + return ( + }> + {/* 抛错导致白屏 */} + + ); +} + +// ✅ Error Boundary + Suspense +function WithErrorHandling() { + return ( + }> + }> + + + + ); +} + +// === use() Hook 错误 === + +// ❌ 在组件外创建 Promise（每次渲染新 Promise） +function BadUse() { + const data = use(fetchData()); // 每次渲染都创建新 Promise！ + return
{data}
; +} + +// ✅ 在父组件创建，通过 props 传递 +function Parent() { + const dataPromise = useMemo(() => fetchData(), []); + return ; +} +function Child({ dataPromise }) { + const data = use(dataPromise); + return
{data}
; +} + +// === Next.js Streaming 错误 === + +// ❌ 在 layout.tsx 中 await 慢数据——阻塞所有子页面 +// app/layout.tsx +export default async function Layout({ children }) { + const config = await fetchSlowConfig(); // 阻塞整个应用！ + return {children}; +} + +// ✅ 将慢数据放在页面级别或使用 Suspense +// app/layout.tsx +export default function Layout({ children }) { + return ( + }> + {children} + + ); +} +``` + +### Suspense Checklist +- [ ] 慢内容有独立的 Suspense 边界 +- [ ] 每个 Suspense 有对应的 Error Boundary +- [ ] fallback 是有意义的骨架屏（不是简单 spinner） +- [ ] use() 的 Promise 不在渲染时创建 +- [ ] 没有在 layout 中 await 慢数据 +- [ ] 嵌套层级不超过 3 层 + +### TanStack Query 错误 + +```tsx +// === 查询配置错误 === + +// ❌ queryKey 不包含查询参数 +function BadQuery({ userId, filters }) { + const { data } = useQuery({ + queryKey: ['users'], // 缺少 userId 和 filters！ + queryFn: () => fetchUsers(userId, filters), + }); + // userId 或 filters 变化时数据不会更新 +} + +// ✅ queryKey 包含所有影响数据的参数 +function GoodQuery({ userId, filters }) { + const { data } = useQuery({ + queryKey: ['users', userId, filters], + queryFn: () => fetchUsers(userId, filters), + }); +} + +// ❌ staleTime: 0 导致过度请求 +const { data } = useQuery({ + queryKey: ['data'], + queryFn: fetchData, + // 默认 staleTime: 0，每次组件挂载/窗口聚焦都会 refetch +}); + +// ✅ 设置合理的 staleTime +const { data } = useQuery({ + queryKey: ['data'], + queryFn: fetchData, + staleTime: 5 * 60 * 1000, // 5 分钟内不会自动 refetch +}); + +// === useSuspenseQuery 错误 === + +// ❌ useSuspenseQuery + enabled（不支持） +const { data } = useSuspenseQuery({ + queryKey: ['user', userId], + queryFn: () => fetchUser(userId), + enabled: !!userId, // 错误！useSuspenseQuery 不支持 enabled +}); + +// ✅ 条件渲染实现 +function UserQuery({ userId }) { + const { data } = useSuspenseQuery({ + queryKey: ['user', userId], + queryFn: () => fetchUser(userId), + }); + return ; +} + +function Parent({ userId }) { + if (!userId) return ; + return ( + }> + + + ); +} + +// === Mutation 错误 === + +// ❌ Mutation 成功后不 invalidate 查询 +const mutation = useMutation({ + mutationFn: updateUser, + // 忘记 invalidate，UI 显示旧数据 +}); + +// ✅ 成功后 invalidate 相关查询 +const mutation = useMutation({ + mutationFn: updateUser, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['users'] }); + }, +}); + +// ❌ 乐观更新不处理回滚 +const mutation = useMutation({ + mutationFn: updateTodo, + onMutate: async (newTodo) => { + queryClient.setQueryData(['todos'], (old) => [...old, newTodo]); + // 没有保存旧数据，失败后无法回滚！ + }, +}); + +// ✅ 完整的乐观更新 +const mutation = useMutation({ + mutationFn: updateTodo, + onMutate: async (newTodo) => { + await queryClient.cancelQueries({ queryKey: ['todos'] }); + const previous = queryClient.getQueryData(['todos']); + queryClient.setQueryData(['todos'], (old) => [...old, newTodo]); + return { previous }; + }, + onError: (err, newTodo, context) => { + queryClient.setQueryData(['todos'], context.previous); + }, + onSettled: () => { + queryClient.invalidateQueries({ queryKey: ['todos'] }); + }, +}); + +// === v5 迁移错误 === + +// ❌ 使用废弃的 API +const { data, isLoading } = useQuery(['key'], fetchFn); // v4 语法 + +// ✅ v5 单一对象参数 +const { data, isPending } = useQuery({ + queryKey: ['key'], + queryFn: fetchFn, +}); + +// ❌ 混淆 isPending 和 isLoading +if (isLoading) return ; +// v5 中 isLoading = isPending && isFetching + +// ✅ 根据意图选择 +if (isPending) return ; // 没有缓存数据 +// 或 +if (isFetching) return ; // 正在后台刷新 +``` + +### TanStack Query Checklist +- [ ] queryKey 包含所有影响数据的参数 +- [ ] 设置了合理的 staleTime（不是默认 0） +- [ ] useSuspenseQuery 不使用 enabled +- [ ] Mutation 成功后 invalidate 相关查询 +- [ ] 乐观更新有完整的回滚逻辑 +- [ ] v5 使用单一对象参数语法 +- [ ] 理解 isPending vs isLoading vs isFetching + +### TypeScript/JavaScript Common Mistakes +- [ ] `==` instead of `===` +- [ ] Modifying array/object during iteration +- [ ] `this` context lost in callbacks +- [ ] Missing `key` prop in lists +- [ ] Closure capturing loop variable +- [ ] parseInt without radix parameter + +## Vue 3 + +### 响应性丢失 +```vue + + + + + +``` + +### Props 响应性传递 +```vue + + + + + +``` + +### Watch 清理 +```vue + + + + + +``` + +### Computed 副作用 +```vue + + + + + +``` + +### 模板常见错误 +```vue + + + + + +``` + +### Common Mistakes +- [ ] 解构 reactive 对象丢失响应性 +- [ ] props 传递给 composable 时未保持响应性 +- [ ] watch 异步回调无清理函数 +- [ ] computed 中产生副作用 +- [ ] v-for 使用 index 作为 key（列表会重排时） +- [ ] v-if 和 v-for 在同一元素上 +- [ ] defineProps 未使用 TypeScript 类型声明 +- [ ] withDefaults 对象默认值未使用工厂函数 +- [ ] 直接修改 props（而不是 emit） +- [ ] watchEffect 依赖不明确导致过度触发 + +## Python + +### Mutable Default Arguments +```python +# ❌ Bug: List shared across all calls +def add_item(item, items=[]): + items.append(item) + return items + +# ✅ Correct +def add_item(item, items=None): + if items is None: + items = [] + items.append(item) + return items +``` + +### Exception Handling +```python +# ❌ Catching everything, including KeyboardInterrupt +try: + risky_operation() +except: + pass + +# ✅ Catch specific exceptions +try: + risky_operation() +except ValueError as e: + logger.error(f"Invalid value: {e}") + raise +``` + +### Class Attributes +```python +# ❌ Shared mutable class attribute +class User: + permissions = [] # Shared across all instances! + +# ✅ Initialize in __init__ +class User: + def __init__(self): + self.permissions = [] +``` + +### Common Mistakes +- [ ] Using `is` instead of `==` for value comparison +- [ ] Forgetting `self` parameter in methods +- [ ] Modifying list while iterating +- [ ] String concatenation in loops (use join) +- [ ] Not closing files (use `with` statement) + +## Rust + +### 所有权与借用 + +```rust +// ❌ Use after move +let s = String::from("hello"); +let s2 = s; +println!("{}", s); // Error: s was moved + +// ✅ Clone if needed (but consider if clone is necessary) +let s = String::from("hello"); +let s2 = s.clone(); +println!("{}", s); // OK + +// ❌ 用 clone() 绕过借用检查器（反模式） +fn process(data: &Data) { + let owned = data.clone(); // 不必要的 clone + do_something(owned); +} + +// ✅ 正确使用借用 +fn process(data: &Data) { + do_something(data); // 传递引用 +} + +// ❌ 在结构体中存储借用（通常是坏主意） +struct Parser<'a> { + input: &'a str, // 生命周期复杂化 + position: usize, +} + +// ✅ 使用拥有的数据 +struct Parser { + input: String, // 拥有数据，简化生命周期 + position: usize, +} + +// ❌ 迭代时修改集合 +let mut vec = vec![1, 2, 3]; +for item in &vec { + vec.push(*item); // Error: cannot borrow as mutable +} + +// ✅ 收集到新集合 +let vec = vec![1, 2, 3]; +let new_vec: Vec<_> = vec.iter().map(|x| x * 2).collect(); +``` + +### Unsafe 代码审查 + +```rust +// ❌ unsafe 没有安全注释 +unsafe { + ptr::write(dest, value); +} + +// ✅ 必须有 SAFETY 注释说明不变量 +// SAFETY: dest 指针由 Vec::as_mut_ptr() 获得，保证： +// 1. 指针有效且已对齐 +// 2. 目标内存未被其他引用借用 +// 3. 写入不会超出分配的容量 +unsafe { + ptr::write(dest, value); +} + +// ❌ unsafe fn 没有 # Safety 文档 +pub unsafe fn from_raw_parts(ptr: *mut T, len: usize) -> Self { ... } + +// ✅ 必须文档化安全契约 +/// Creates a new instance from raw parts. +/// +/// # Safety +/// +/// - `ptr` must have been allocated via `GlobalAlloc` +/// - `len` must be less than or equal to the allocated capacity +/// - The caller must ensure no other references to the memory exist +pub unsafe fn from_raw_parts(ptr: *mut T, len: usize) -> Self { ... } + +// ❌ 跨模块 unsafe 不变量 +mod a { + pub fn set_flag() { FLAG = true; } // 安全代码影响 unsafe +} +mod b { + pub unsafe fn do_thing() { + if FLAG { /* assumes FLAG means something */ } + } +} + +// ✅ 将 unsafe 边界封装在单一模块 +mod safe_wrapper { + // 所有 unsafe 逻辑在一个模块内 + // 对外提供 safe API +} +``` + +### 异步/并发 + +```rust +// ❌ 在异步上下文中阻塞 +async fn bad_fetch(url: &str) -> Result { + let resp = reqwest::blocking::get(url)?; // 阻塞整个运行时！ + Ok(resp.text()?) +} + +// ✅ 使用异步版本 +async fn good_fetch(url: &str) -> Result { + let resp = reqwest::get(url).await?; + Ok(resp.text().await?) +} + +// ❌ 跨 .await 持有 Mutex +async fn bad_lock(mutex: &Mutex) { + let guard = mutex.lock().unwrap(); + some_async_op().await; // 持锁跨越 await！ + drop(guard); +} + +// ✅ 缩短锁持有时间 +async fn good_lock(mutex: &Mutex) { + let data = { + let guard = mutex.lock().unwrap(); + guard.clone() // 获取数据后立即释放锁 + }; + some_async_op().await; + // 处理 data +} + +// ❌ 在异步函数中使用 std::sync::Mutex +async fn bad_async_mutex(mutex: &std::sync::Mutex) { + let _guard = mutex.lock().unwrap(); // 可能死锁 + tokio::time::sleep(Duration::from_secs(1)).await; +} + +// ✅ 使用 tokio::sync::Mutex（如果必须跨 await） +async fn good_async_mutex(mutex: &tokio::sync::Mutex) { + let _guard = mutex.lock().await; + tokio::time::sleep(Duration::from_secs(1)).await; +} + +// ❌ 忘记 Future 是惰性的 +fn bad_spawn() { + let future = async_operation(); // 没有执行！ + // future 被丢弃，什么都没发生 +} + +// ✅ 必须 await 或 spawn +async fn good_spawn() { + async_operation().await; // 执行 + // 或 + tokio::spawn(async_operation()); // 后台执行 +} + +// ❌ spawn 任务缺少 'static +async fn bad_spawn_lifetime(data: &str) { + tokio::spawn(async { + println!("{}", data); // Error: data 不是 'static + }); +} + +// ✅ 使用 move 或 Arc +async fn good_spawn_lifetime(data: String) { + tokio::spawn(async move { + println!("{}", data); // OK: 拥有数据 + }); +} +``` + +### 错误处理 + +```rust +// ❌ 生产代码中使用 unwrap/expect +fn bad_parse(input: &str) -> i32 { + input.parse().unwrap() // panic! +} + +// ✅ 正确传播错误 +fn good_parse(input: &str) -> Result { + input.parse() +} + +// ❌ 吞掉错误信息 +fn bad_error_handling() -> Result<()> { + match operation() { + Ok(v) => Ok(v), + Err(_) => Err(anyhow!("operation failed")) // 丢失原始错误 + } +} + +// ✅ 使用 context 添加上下文 +fn good_error_handling() -> Result<()> { + operation().context("failed to perform operation")?; + Ok(()) +} + +// ❌ 库代码使用 anyhow（应该用 thiserror） +// lib.rs +pub fn parse_config(path: &str) -> anyhow::Result { + // 调用者无法区分错误类型 +} + +// ✅ 库代码用 thiserror 定义错误类型 +#[derive(Debug, thiserror::Error)] +pub enum ConfigError { + #[error("failed to read config file: {0}")] + Io(#[from] std::io::Error), + #[error("invalid config format: {0}")] + Parse(#[from] serde_json::Error), +} + +pub fn parse_config(path: &str) -> Result { + // 调用者可以 match 不同错误 +} + +// ❌ 忽略 must_use 返回值 +fn bad_ignore_result() { + some_fallible_operation(); // 警告：unused Result +} + +// ✅ 显式处理或标记忽略 +fn good_handle_result() { + let _ = some_fallible_operation(); // 显式忽略 + // 或 + some_fallible_operation().ok(); // 转换为 Option +} +``` + +### 性能陷阱 + +```rust +// ❌ 不必要的 collect +fn bad_process(items: &[i32]) -> i32 { + items.iter() + .filter(|x| **x > 0) + .collect::>() // 不必要的分配 + .iter() + .sum() +} + +// ✅ 惰性迭代 +fn good_process(items: &[i32]) -> i32 { + items.iter() + .filter(|x| **x > 0) + .sum() +} + +// ❌ 循环中重复分配 +fn bad_loop() -> String { + let mut result = String::new(); + for i in 0..1000 { + result = result + &i.to_string(); // 每次迭代都重新分配！ + } + result +} + +// ✅ 预分配或使用 push_str +fn good_loop() -> String { + let mut result = String::with_capacity(4000); // 预分配 + for i in 0..1000 { + write!(result, "{}", i).unwrap(); // 原地追加 + } + result +} + +// ❌ 过度使用 clone +fn bad_clone(data: &HashMap>) -> Vec { + data.get("key").cloned().unwrap_or_default() +} + +// ✅ 返回引用或使用 Cow +fn good_ref(data: &HashMap>) -> &[u8] { + data.get("key").map(|v| v.as_slice()).unwrap_or(&[]) +} + +// ❌ 大结构体按值传递 +fn bad_pass(data: LargeStruct) { ... } // 拷贝整个结构体 + +// ✅ 传递引用 +fn good_pass(data: &LargeStruct) { ... } + +// ❌ Box 用于小型已知类型 +fn bad_trait_object() -> Box> { + Box::new(vec![1, 2, 3].into_iter()) +} + +// ✅ 使用 impl Trait +fn good_impl_trait() -> impl Iterator { + vec![1, 2, 3].into_iter() +} + +// ❌ retain 比 filter+collect 慢（某些场景） +vec.retain(|x| x.is_valid()); // O(n) 但常数因子大 + +// ✅ 如果不需要原地修改，考虑 filter +let vec: Vec<_> = vec.into_iter().filter(|x| x.is_valid()).collect(); +``` + +### 生命周期与引用 + +```rust +// ❌ 返回局部变量的引用 +fn bad_return_ref() -> &str { + let s = String::from("hello"); + &s // Error: s will be dropped +} + +// ✅ 返回拥有的数据或静态引用 +fn good_return_owned() -> String { + String::from("hello") +} + +// ❌ 生命周期过度泛化 +fn bad_lifetime<'a, 'b>(x: &'a str, y: &'b str) -> &'a str { + x // 'b 没有被使用 +} + +// ✅ 简化生命周期 +fn good_lifetime(x: &str, _y: &str) -> &str { + x // 编译器自动推断 +} + +// ❌ 结构体持有多个相关引用但生命周期独立 +struct Bad<'a, 'b> { + name: &'a str, + data: &'b [u8], // 通常应该是同一个生命周期 +} + +// ✅ 相关数据使用相同生命周期 +struct Good<'a> { + name: &'a str, + data: &'a [u8], +} +``` + +### Rust 审查清单 + +**所有权与借用** +- [ ] clone() 是有意为之，不是绕过借用检查器 +- [ ] 避免在结构体中存储借用（除非必要） +- [ ] Rc/Arc 使用合理，没有隐藏不必要的共享状态 +- [ ] 没有不必要的 RefCell（运行时检查 vs 编译时） + +**Unsafe 代码** +- [ ] 每个 unsafe 块有 SAFETY 注释 +- [ ] unsafe fn 有 # Safety 文档 +- [ ] 安全不变量被清晰记录 +- [ ] unsafe 边界尽可能小 + +**异步/并发** +- [ ] 没有在异步上下文中阻塞 +- [ ] 没有跨 .await 持有 std::sync 锁 +- [ ] spawn 的任务满足 'static 约束 +- [ ] Future 被正确 await 或 spawn +- [ ] 锁的顺序一致（避免死锁） + +**错误处理** +- [ ] 库代码使用 thiserror，应用代码使用 anyhow +- [ ] 错误有足够的上下文信息 +- [ ] 没有在生产代码中 unwrap/expect +- [ ] must_use 返回值被正确处理 + +**性能** +- [ ] 避免不必要的 collect() +- [ ] 大数据结构传引用 +- [ ] 字符串拼接使用 String::with_capacity 或 write! +- [ ] impl Trait 优于 Box（当可能时） + +**类型系统** +- [ ] 善用 newtype 模式增加类型安全 +- [ ] 枚举穷尽匹配（没有 _ 通配符隐藏新变体） +- [ ] 生命周期尽可能简化 + +## SQL + +### Injection Vulnerabilities +```sql +-- ❌ String concatenation (SQL injection risk) +query = "SELECT * FROM users WHERE id = " + user_id + +-- ✅ Parameterized queries +query = "SELECT * FROM users WHERE id = ?" +cursor.execute(query, (user_id,)) +``` + +### Performance Issues +- [ ] Missing indexes on filtered/joined columns +- [ ] SELECT * instead of specific columns +- [ ] N+1 query patterns +- [ ] Missing LIMIT on large tables +- [ ] Inefficient subqueries vs JOINs + +### Common Mistakes +- [ ] Not handling NULL comparisons correctly +- [ ] Missing transactions for related operations +- [ ] Incorrect JOIN types +- [ ] Case sensitivity issues +- [ ] Date/timezone handling errors + +## API Design + +### REST Issues +- [ ] Inconsistent resource naming +- [ ] Wrong HTTP methods (POST for idempotent operations) +- [ ] Missing pagination for list endpoints +- [ ] Incorrect status codes +- [ ] Missing rate limiting + +### Data Validation +- [ ] Missing input validation +- [ ] Incorrect data type validation +- [ ] Missing length/range checks +- [ ] Not sanitizing user input +- [ ] Trusting client-side validation + +## Testing + +### Test Quality Issues +- [ ] Testing implementation details instead of behavior +- [ ] Missing edge case tests +- [ ] Flaky tests (non-deterministic) +- [ ] Tests with external dependencies +- [ ] Missing negative tests (error cases) +- [ ] Overly complex test setup diff --git a/references/security-review-guide.md b/references/security-review-guide.md new file mode 100644 index 0000000..80d10bc --- /dev/null +++ b/references/security-review-guide.md @@ -0,0 +1,265 @@ +# Security Review Guide + +Security-focused code review checklist based on OWASP Top 10 and best practices. + +## Authentication & Authorization + +### Authentication +- [ ] Passwords hashed with strong algorithm (bcrypt, argon2) +- [ ] Password complexity requirements enforced +- [ ] Account lockout after failed attempts +- [ ] Secure password reset flow +- [ ] Multi-factor authentication for sensitive operations +- [ ] Session tokens are cryptographically random +- [ ] Session timeout implemented + +### Authorization +- [ ] Authorization checks on every request +- [ ] Principle of least privilege applied +- [ ] Role-based access control (RBAC) properly implemented +- [ ] No privilege escalation paths +- [ ] Direct object reference checks (IDOR prevention) +- [ ] API endpoints protected appropriately + +### JWT Security +```typescript +// ❌ Insecure JWT configuration +jwt.sign(payload, 'weak-secret'); + +// ✅ Secure JWT configuration +jwt.sign(payload, process.env.JWT_SECRET, { + algorithm: 'RS256', + expiresIn: '15m', + issuer: 'your-app', + audience: 'your-api' +}); + +// ❌ Not verifying JWT properly +const decoded = jwt.decode(token); // No signature verification! + +// ✅ Verify signature and claims +const decoded = jwt.verify(token, publicKey, { + algorithms: ['RS256'], + issuer: 'your-app', + audience: 'your-api' +}); +``` + +## Input Validation + +### SQL Injection Prevention +```python +# ❌ Vulnerable to SQL injection +query = f"SELECT * FROM users WHERE id = {user_id}" + +# ✅ Use parameterized queries +cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,)) + +# ✅ Use ORM with proper escaping +User.objects.filter(id=user_id) +``` + +### XSS Prevention +```typescript +// ❌ Vulnerable to XSS +element.innerHTML = userInput; + +// ✅ Use textContent for plain text +element.textContent = userInput; + +// ✅ Use DOMPurify for HTML +element.innerHTML = DOMPurify.sanitize(userInput); + +// ✅ React automatically escapes (but watch dangerouslySetInnerHTML) +return
{userInput}
; // Safe +return
; // Dangerous! +``` + +### Command Injection Prevention +```python +# ❌ Vulnerable to command injection +os.system(f"convert {filename} output.png") + +# ✅ Use subprocess with list arguments +subprocess.run(['convert', filename, 'output.png'], check=True) + +# ✅ Validate and sanitize input +import shlex +safe_filename = shlex.quote(filename) +``` + +### Path Traversal Prevention +```typescript +// ❌ Vulnerable to path traversal +const filePath = `./uploads/${req.params.filename}`; + +// ✅ Validate and sanitize path +const path = require('path'); +const safeName = path.basename(req.params.filename); +const filePath = path.join('./uploads', safeName); + +// Verify it's still within uploads directory +if (!filePath.startsWith(path.resolve('./uploads'))) { + throw new Error('Invalid path'); +} +``` + +## Data Protection + +### Sensitive Data Handling +- [ ] No secrets in source code +- [ ] Secrets stored in environment variables or secret manager +- [ ] Sensitive data encrypted at rest +- [ ] Sensitive data encrypted in transit (HTTPS) +- [ ] PII handled according to regulations (GDPR, etc.) +- [ ] Sensitive data not logged +- [ ] Secure data deletion when required + +### Configuration Security +```yaml +# ❌ Secrets in config files +database: + password: "super-secret-password" + +# ✅ Reference environment variables +database: + password: ${DATABASE_PASSWORD} +``` + +### Error Messages +```typescript +// ❌ Leaking sensitive information +catch (error) { + return res.status(500).json({ + error: error.stack, // Exposes internal details + query: sqlQuery // Exposes database structure + }); +} + +// ✅ Generic error messages +catch (error) { + logger.error('Database error', { error, userId }); // Log internally + return res.status(500).json({ + error: 'An unexpected error occurred' + }); +} +``` + +## API Security + +### Rate Limiting +- [ ] Rate limiting on all public endpoints +- [ ] Stricter limits on authentication endpoints +- [ ] Per-user and per-IP limits +- [ ] Graceful handling when limits exceeded + +### CORS Configuration +```typescript +// ❌ Overly permissive CORS +app.use(cors({ origin: '*' })); + +// ✅ Restrictive CORS +app.use(cors({ + origin: ['https://your-app.com'], + methods: ['GET', 'POST'], + credentials: true +})); +``` + +### HTTP Headers +```typescript +// Security headers to set +app.use(helmet({ + contentSecurityPolicy: { + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'"], + styleSrc: ["'self'", "'unsafe-inline'"], + } + }, + hsts: { maxAge: 31536000, includeSubDomains: true }, + noSniff: true, + xssFilter: true, + frameguard: { action: 'deny' } +})); +``` + +## Cryptography + +### Secure Practices +- [ ] Using well-established algorithms (AES-256, RSA-2048+) +- [ ] Not implementing custom cryptography +- [ ] Using cryptographically secure random number generation +- [ ] Proper key management and rotation +- [ ] Secure key storage (HSM, KMS) + +### Common Mistakes +```typescript +// ❌ Weak random generation +const token = Math.random().toString(36); + +// ✅ Cryptographically secure random +const crypto = require('crypto'); +const token = crypto.randomBytes(32).toString('hex'); + +// ❌ MD5/SHA1 for passwords +const hash = crypto.createHash('md5').update(password).digest('hex'); + +// ✅ Use bcrypt or argon2 +const bcrypt = require('bcrypt'); +const hash = await bcrypt.hash(password, 12); +``` + +## Dependency Security + +### Checklist +- [ ] Dependencies from trusted sources only +- [ ] No known vulnerabilities (npm audit, cargo audit) +- [ ] Dependencies kept up to date +- [ ] Lock files committed (package-lock.json, Cargo.lock) +- [ ] Minimal dependency usage +- [ ] License compliance verified + +### Audit Commands +```bash +# Node.js +npm audit +npm audit fix + +# Python +pip-audit +safety check + +# Rust +cargo audit + +# General +snyk test +``` + +## Logging & Monitoring + +### Secure Logging +- [ ] No sensitive data in logs (passwords, tokens, PII) +- [ ] Logs protected from tampering +- [ ] Appropriate log retention +- [ ] Security events logged (login attempts, permission changes) +- [ ] Log injection prevented + +```typescript +// ❌ Logging sensitive data +logger.info(`User login: ${email}, password: ${password}`); + +// ✅ Safe logging +logger.info('User login attempt', { email, success: true }); +``` + +## Security Review Severity Levels + +| Severity | Description | Action | +|----------|-------------|--------| +| **Critical** | Immediate exploitation possible, data breach risk | Block merge, fix immediately | +| **High** | Significant vulnerability, requires specific conditions | Block merge, fix before release | +| **Medium** | Moderate risk, defense in depth concern | Should fix, can merge with tracking | +| **Low** | Minor issue, best practice violation | Nice to fix, non-blocking | +| **Info** | Suggestion for improvement | Optional enhancement | diff --git a/scripts/pr-analyzer.py b/scripts/pr-analyzer.py new file mode 100644 index 0000000..0068aa9 --- /dev/null +++ b/scripts/pr-analyzer.py @@ -0,0 +1,356 @@ +#!/usr/bin/env python3 +""" +PR Analyzer - Analyze PR complexity and suggest review approach. + +Usage: + python pr-analyzer.py [--diff-file FILE] [--stats] + + Or pipe diff directly: + git diff main...HEAD | python pr-analyzer.py +""" + +import sys +import re +import argparse +from collections import defaultdict +from dataclasses import dataclass +from typing import List, Dict, Optional + + +@dataclass +class FileStats: + """Statistics for a single file.""" + filename: str + additions: int = 0 + deletions: int = 0 + is_test: bool = False + is_config: bool = False + language: str = "unknown" + + +@dataclass +class PRAnalysis: + """Complete PR analysis results.""" + total_files: int + total_additions: int + total_deletions: int + files: List[FileStats] + complexity_score: float + size_category: str + estimated_review_time: int + risk_factors: List[str] + suggestions: List[str] + + +def detect_language(filename: str) -> str: + """Detect programming language from filename.""" + extensions = { + '.py': 'Python', + '.js': 'JavaScript', + '.ts': 'TypeScript', + '.tsx': 'TypeScript/React', + '.jsx': 'JavaScript/React', + '.rs': 'Rust', + '.go': 'Go', + '.java': 'Java', + '.rb': 'Ruby', + '.sql': 'SQL', + '.md': 'Markdown', + '.json': 'JSON', + '.yaml': 'YAML', + '.yml': 'YAML', + '.toml': 'TOML', + '.css': 'CSS', + '.scss': 'SCSS', + '.html': 'HTML', + } + for ext, lang in extensions.items(): + if filename.endswith(ext): + return lang + return 'unknown' + + +def is_test_file(filename: str) -> bool: + """Check if file is a test file.""" + test_patterns = [ + r'test_.*\.py$', + r'.*_test\.py$', + r'.*\.test\.(js|ts|tsx)$', + r'.*\.spec\.(js|ts|tsx)$', + r'tests?/', + r'__tests__/', + ] + return any(re.search(p, filename) for p in test_patterns) + + +def is_config_file(filename: str) -> bool: + """Check if file is a configuration file.""" + config_patterns = [ + r'\.env', + r'config\.', + r'\.json$', + r'\.yaml$', + r'\.yml$', + r'\.toml$', + r'Cargo\.toml$', + r'package\.json$', + r'tsconfig\.json$', + ] + return any(re.search(p, filename) for p in config_patterns) + + +def parse_diff(diff_content: str) -> List[FileStats]: + """Parse git diff output and extract file statistics.""" + files = [] + current_file = None + + for line in diff_content.split('\n'): + # New file header + if line.startswith('diff --git'): + if current_file: + files.append(current_file) + # Extract filename from "diff --git a/path b/path" + match = re.search(r'b/(.+)$', line) + if match: + filename = match.group(1) + current_file = FileStats( + filename=filename, + language=detect_language(filename), + is_test=is_test_file(filename), + is_config=is_config_file(filename), + ) + elif current_file: + if line.startswith('+') and not line.startswith('+++'): + current_file.additions += 1 + elif line.startswith('-') and not line.startswith('---'): + current_file.deletions += 1 + + if current_file: + files.append(current_file) + + return files + + +def calculate_complexity(files: List[FileStats]) -> float: + """Calculate complexity score (0-1 scale).""" + if not files: + return 0.0 + + total_changes = sum(f.additions + f.deletions for f in files) + + # Base complexity from size + size_factor = min(total_changes / 1000, 1.0) + + # Factor for number of files + file_factor = min(len(files) / 20, 1.0) + + # Factor for non-test code ratio + test_lines = sum(f.additions + f.deletions for f in files if f.is_test) + non_test_ratio = 1 - (test_lines / max(total_changes, 1)) + + # Factor for language diversity + languages = set(f.language for f in files if f.language != 'unknown') + lang_factor = min(len(languages) / 5, 1.0) + + complexity = ( + size_factor * 0.4 + + file_factor * 0.2 + + non_test_ratio * 0.2 + + lang_factor * 0.2 + ) + + return round(complexity, 2) + + +def categorize_size(total_changes: int) -> str: + """Categorize PR size.""" + if total_changes < 50: + return "XS (Extra Small)" + elif total_changes < 200: + return "S (Small)" + elif total_changes < 400: + return "M (Medium)" + elif total_changes < 800: + return "L (Large)" + else: + return "XL (Extra Large) - Consider splitting" + + +def estimate_review_time(files: List[FileStats], complexity: float) -> int: + """Estimate review time in minutes.""" + total_changes = sum(f.additions + f.deletions for f in files) + + # Base time: ~1 minute per 20 lines + base_time = total_changes / 20 + + # Adjust for complexity + adjusted_time = base_time * (1 + complexity) + + # Minimum 5 minutes, maximum 120 minutes + return max(5, min(120, int(adjusted_time))) + + +def identify_risk_factors(files: List[FileStats]) -> List[str]: + """Identify potential risk factors in the PR.""" + risks = [] + + total_changes = sum(f.additions + f.deletions for f in files) + test_changes = sum(f.additions + f.deletions for f in files if f.is_test) + + # Large PR + if total_changes > 400: + risks.append("Large PR (>400 lines) - harder to review thoroughly") + + # No tests + if test_changes == 0 and total_changes > 50: + risks.append("No test changes - verify test coverage") + + # Low test ratio + if total_changes > 100 and test_changes / max(total_changes, 1) < 0.2: + risks.append("Low test ratio (<20%) - consider adding more tests") + + # Security-sensitive files + security_patterns = ['.env', 'auth', 'security', 'password', 'token', 'secret'] + for f in files: + if any(p in f.filename.lower() for p in security_patterns): + risks.append(f"Security-sensitive file: {f.filename}") + break + + # Database changes + for f in files: + if 'migration' in f.filename.lower() or f.language == 'SQL': + risks.append("Database changes detected - review carefully") + break + + # Config changes + config_files = [f for f in files if f.is_config] + if config_files: + risks.append(f"Configuration changes in {len(config_files)} file(s)") + + return risks + + +def generate_suggestions(files: List[FileStats], complexity: float, risks: List[str]) -> List[str]: + """Generate review suggestions.""" + suggestions = [] + + total_changes = sum(f.additions + f.deletions for f in files) + + if total_changes > 800: + suggestions.append("Consider splitting this PR into smaller, focused changes") + + if complexity > 0.7: + suggestions.append("High complexity - allocate extra review time") + suggestions.append("Consider pair reviewing for critical sections") + + if "No test changes" in str(risks): + suggestions.append("Request test additions before approval") + + # Language-specific suggestions + languages = set(f.language for f in files) + if 'TypeScript' in languages or 'TypeScript/React' in languages: + suggestions.append("Check for proper type usage (avoid 'any')") + if 'Rust' in languages: + suggestions.append("Check for unwrap() usage and error handling") + if 'SQL' in languages: + suggestions.append("Review for SQL injection and query performance") + + if not suggestions: + suggestions.append("Standard review process should suffice") + + return suggestions + + +def analyze_pr(diff_content: str) -> PRAnalysis: + """Perform complete PR analysis.""" + files = parse_diff(diff_content) + + total_additions = sum(f.additions for f in files) + total_deletions = sum(f.deletions for f in files) + total_changes = total_additions + total_deletions + + complexity = calculate_complexity(files) + risks = identify_risk_factors(files) + suggestions = generate_suggestions(files, complexity, risks) + + return PRAnalysis( + total_files=len(files), + total_additions=total_additions, + total_deletions=total_deletions, + files=files, + complexity_score=complexity, + size_category=categorize_size(total_changes), + estimated_review_time=estimate_review_time(files, complexity), + risk_factors=risks, + suggestions=suggestions, + ) + + +def print_analysis(analysis: PRAnalysis, show_files: bool = False): + """Print analysis results.""" + print("\n" + "=" * 60) + print("PR ANALYSIS REPORT") + print("=" * 60) + + print(f"\n📊 SUMMARY") + print(f" Files changed: {analysis.total_files}") + print(f" Additions: +{analysis.total_additions}") + print(f" Deletions: -{analysis.total_deletions}") + print(f" Total changes: {analysis.total_additions + analysis.total_deletions}") + + print(f"\n📏 SIZE: {analysis.size_category}") + print(f" Complexity score: {analysis.complexity_score}/1.0") + print(f" Estimated review time: ~{analysis.estimated_review_time} minutes") + + if analysis.risk_factors: + print(f"\n⚠️ RISK FACTORS:") + for risk in analysis.risk_factors: + print(f" • {risk}") + + print(f"\n💡 SUGGESTIONS:") + for suggestion in analysis.suggestions: + print(f" • {suggestion}") + + if show_files: + print(f"\n📁 FILES:") + # Group by language + by_lang: Dict[str, List[FileStats]] = defaultdict(list) + for f in analysis.files: + by_lang[f.language].append(f) + + for lang, lang_files in sorted(by_lang.items()): + print(f"\n [{lang}]") + for f in lang_files: + prefix = "🧪" if f.is_test else "⚙️" if f.is_config else "📄" + print(f" {prefix} {f.filename} (+{f.additions}/-{f.deletions})") + + print("\n" + "=" * 60) + + +def main(): + parser = argparse.ArgumentParser(description='Analyze PR complexity') + parser.add_argument('--diff-file', '-f', help='Path to diff file') + parser.add_argument('--stats', '-s', action='store_true', help='Show file details') + args = parser.parse_args() + + # Read diff from file or stdin + if args.diff_file: + with open(args.diff_file, 'r') as f: + diff_content = f.read() + elif not sys.stdin.isatty(): + diff_content = sys.stdin.read() + else: + print("Usage: git diff main...HEAD | python pr-analyzer.py") + print(" python pr-analyzer.py -f diff.txt") + sys.exit(1) + + if not diff_content.strip(): + print("No diff content provided") + sys.exit(1) + + analysis = analyze_pr(diff_content) + print_analysis(analysis, show_files=args.stats) + + +if __name__ == '__main__': + main()