250 lines
10 KiB
Markdown
250 lines
10 KiB
Markdown
|
# 代码审查清单 (Code Review Checklist)
|
|||
|
|
|
|||
|
|
> 按语言和框架分类的审查要点，以及 PR 审查流程 SOP。
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## TypeScript/JavaScript 审查清单
|
|||
|
|
|
|||
|
|
### 类型安全
|
|||
|
|
|
|||
|
|
- [ ] **禁止裸 `any`**：所有 `any` 必须有注释说明原因，或替换为具体类型 / `unknown`
|
|||
|
|
- [ ] **类型断言最小化**：`as` 断言需要注释理由；优先使用类型守卫 (type guard)
|
|||
|
|
- [ ] **泛型正确性**：泛型参数有约束 (`extends`)，不使用无约束的 `<T>`
|
|||
|
|
- [ ] **严格模式兼容**：`strictNullChecks` 下无隐式 `undefined` 访问
|
|||
|
|
- [ ] **联合类型穷举**：`switch` 对联合类型使用 `exhaustive check`（never 兜底）
|
|||
|
|
- [ ] **外部数据验证**：API 响应、用户输入使用 Zod / io-ts 运行时校验
|
|||
|
|
|
|||
|
|
```typescript
|
|||
|
|
// ❌ 裸 any，无校验
|
|||
|
|
function handleResponse(data: any) {
|
|||
|
|
return data.user.name;
|
|||
|
|
}
|
|||
|
|
|
|||
|
|
// ✅ 运行时校验 + 类型推导
|
|||
|
|
const ResponseSchema = z.object({
|
|||
|
|
user: z.object({ name: z.string() }),
|
|||
|
|
});
|
|||
|
|
function handleResponse(raw: unknown) {
|
|||
|
|
const data = ResponseSchema.parse(raw);
|
|||
|
|
return data.user.name; // 类型安全
|
|||
|
|
}
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### 异步安全
|
|||
|
|
|
|||
|
|
- [ ] **Promise 错误处理**：每个 `await` 都在 `try-catch` 中，或链式 `.catch()`
|
|||
|
|
- [ ] **竞态条件**：组件卸载后不更新 state（AbortController / cleanup function）
|
|||
|
|
- [ ] **并发控制**：批量请求使用 `Promise.allSettled` 而非 `Promise.all`（避免一个失败全部丢弃）
|
|||
|
|
- [ ] **超时处理**：网络请求设置合理的 timeout
|
|||
|
|
- [ ] **重试机制**：幂等操作配备指数退避 (exponential backoff)
|
|||
|
|
|
|||
|
|
```typescript
|
|||
|
|
// ❌ 竞态条件：组件卸载后仍设置 state
|
|||
|
|
useEffect(() => {
|
|||
|
|
fetchData().then(data => setData(data));
|
|||
|
|
}, [id]);
|
|||
|
|
|
|||
|
|
// ✅ 使用 AbortController 取消
|
|||
|
|
useEffect(() => {
|
|||
|
|
const controller = new AbortController();
|
|||
|
|
fetchData(id, { signal: controller.signal })
|
|||
|
|
.then(data => setData(data))
|
|||
|
|
.catch(err => {
|
|||
|
|
if (!controller.signal.aborted) reportError(err);
|
|||
|
|
});
|
|||
|
|
return () => controller.abort();
|
|||
|
|
}, [id]);
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### React 特定
|
|||
|
|
|
|||
|
|
- [ ] **Hooks 依赖数组**：`useEffect` / `useMemo` / `useCallback` 依赖完整且无多余项
|
|||
|
|
- [ ] **key 属性**：列表渲染使用稳定唯一 key，禁止用 index 作 key（排序/删除场景）
|
|||
|
|
- [ ] **不必要的 re-render**：大组件拆分，使用 `React.memo` / `useMemo` 防止子树重渲染
|
|||
|
|
- [ ] **状态提升 vs 下沉**：state 放在最近的共同祖先，不过度提升到顶层
|
|||
|
|
- [ ] **Context 粒度**：避免巨大 Context 导致不相关组件重渲染
|
|||
|
|
- [ ] **受控 vs 非受控**：表单组件模式一致，不混用
|
|||
|
|
|
|||
|
|
### Next.js 特定
|
|||
|
|
|
|||
|
|
- [ ] **Server / Client 边界**：`'use client'` 声明仅在必要时添加，Server Component 优先
|
|||
|
|
- [ ] **数据获取位置**：Server Component 中直接 fetch，不在 Client Component 中请求可在服务端获取的数据
|
|||
|
|
- [ ] **缓存策略**：`fetch` 的 `cache` / `revalidate` 选项合理；理解 ISR vs SSR vs SSG
|
|||
|
|
- [ ] **Metadata**：页面导出 `metadata` 或 `generateMetadata` 以支持 SEO
|
|||
|
|
- [ ] **Loading / Error 边界**：`loading.tsx` 和 `error.tsx` 文件完整
|
|||
|
|
- [ ] **Route Handler 安全**：API route 有鉴权中间件，参数经过校验
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Python 审查清单
|
|||
|
|
|
|||
|
|
### 类型标注
|
|||
|
|
|
|||
|
|
- [ ] **typing 模块使用**：函数签名完整标注参数和返回值类型
|
|||
|
|
- [ ] **Pydantic model 验证**：API 入参使用 Pydantic BaseModel，字段有 `Field` 约束
|
|||
|
|
- [ ] **Optional 显式标注**：可为 None 的字段使用 `Optional[T]` 或 `T | None`
|
|||
|
|
- [ ] **泛型容器**：使用 `list[str]` 而非 `List`（Python 3.9+）
|
|||
|
|
- [ ] **TypeAlias**：复杂类型定义使用 `TypeAlias` 提高可读性
|
|||
|
|
|
|||
|
|
```python
|
|||
|
|
# ❌ 无类型标注，无验证
|
|||
|
|
def create_user(data):
|
|||
|
|
return db.execute(f"INSERT INTO users VALUES ({data['name']})")
|
|||
|
|
|
|||
|
|
# ✅ Pydantic 验证 + 类型标注
|
|||
|
|
class CreateUserRequest(BaseModel):
|
|||
|
|
name: str = Field(..., min_length=1, max_length=100)
|
|||
|
|
email: EmailStr
|
|||
|
|
|
|||
|
|
async def create_user(request: CreateUserRequest) -> UserResponse:
|
|||
|
|
return await user_service.create(request)
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### FastAPI 特定
|
|||
|
|
|
|||
|
|
- [ ] **依赖注入**：数据库 session、当前用户等通过 `Depends()` 注入
|
|||
|
|
- [ ] **错误处理**：使用 `HTTPException` 返回合适状态码；全局异常处理器兜底
|
|||
|
|
- [ ] **中间件链**：CORS、日志、限流中间件顺序正确
|
|||
|
|
- [ ] **路由组织**：使用 `APIRouter` 按模块分组；路径命名 RESTful
|
|||
|
|
- [ ] **响应模型**：`response_model` 显式声明，避免泄露内部字段
|
|||
|
|
- [ ] **后台任务**：耗时操作使用 `BackgroundTasks` 或 Celery
|
|||
|
|
|
|||
|
|
### 异步安全
|
|||
|
|
|
|||
|
|
- [ ] **asyncio 使用**：不在 async 函数中调用同步阻塞 I/O（使用 `run_in_executor`）
|
|||
|
|
- [ ] **连接池管理**：数据库连接池大小合理；async session 正确关闭
|
|||
|
|
- [ ] **并发限制**：使用 `asyncio.Semaphore` 控制并发数
|
|||
|
|
- [ ] **上下文变量**：使用 `contextvars` 而非全局变量传递请求级数据
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## Go 审查清单
|
|||
|
|
|
|||
|
|
### 错误处理
|
|||
|
|
|
|||
|
|
- [ ] **error wrapping**：使用 `fmt.Errorf("context: %w", err)` 包装错误，保留链路
|
|||
|
|
- [ ] **sentinel errors**：预定义错误使用 `errors.New` 并以 `Err` 前缀命名
|
|||
|
|
- [ ] **自定义 error**：复杂错误实现 `Error()` 接口，携带结构化上下文
|
|||
|
|
- [ ] **errors.Is / As**：错误判断使用 `errors.Is` 而非 `==`
|
|||
|
|
- [ ] **不忽略错误**：`_ = someFunc()` 必须有注释说明原因
|
|||
|
|
|
|||
|
|
```go
|
|||
|
|
// ❌ 错误被忽略，无上下文
|
|||
|
|
data, _ := json.Marshal(user)
|
|||
|
|
db.Exec("INSERT INTO users VALUES (?)", data)
|
|||
|
|
|
|||
|
|
// ✅ 错误包装，逐层传递
|
|||
|
|
data, err := json.Marshal(user)
|
|||
|
|
if err != nil {
|
|||
|
|
return fmt.Errorf("序列化用户数据失败: %w", err)
|
|||
|
|
}
|
|||
|
|
if _, err := db.Exec("INSERT INTO users VALUES (?)", data); err != nil {
|
|||
|
|
return fmt.Errorf("插入用户记录失败: %w", err)
|
|||
|
|
}
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### 并发安全
|
|||
|
|
|
|||
|
|
- [ ] **goroutine 泄漏**：每个 goroutine 有明确退出条件（context cancel / done channel）
|
|||
|
|
- [ ] **mutex 使用**：共享数据用 `sync.Mutex` 或 `sync.RWMutex` 保护；锁粒度合理
|
|||
|
|
- [ ] **channel 模式**：有缓冲 vs 无缓冲选择合理；`select` 配 `default` 或 `context.Done()`
|
|||
|
|
- [ ] **sync.WaitGroup**：`Add` 在 goroutine 外调用；`Done` 使用 `defer`
|
|||
|
|
- [ ] **data race**：运行 `go test -race` 检测竞态
|
|||
|
|
|
|||
|
|
### Gin 特定
|
|||
|
|
|
|||
|
|
- [ ] **中间件**：鉴权、日志、Recovery 中间件顺序正确
|
|||
|
|
- [ ] **参数绑定**：使用 `ShouldBindJSON` / `ShouldBindQuery` 而非手动取值
|
|||
|
|
- [ ] **响应格式**：统一使用 `c.JSON()` 返回标准格式 `{ code, message, data }`
|
|||
|
|
- [ ] **路由分组**：`v1 := r.Group("/api/v1")` 按版本和模块分组
|
|||
|
|
- [ ] **优雅关停**：使用 `signal.Notify` + `srv.Shutdown(ctx)` 处理退出
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## PR 审查流程 SOP
|
|||
|
|
|
|||
|
|
### 审查前准备
|
|||
|
|
|
|||
|
|
1. **理解上下文**：阅读 PR 描述和关联 issue，明确变更目的
|
|||
|
|
2. **检查 CI 状态**：确认自动化测试通过、lint 无报错
|
|||
|
|
3. **浏览变更范围**：先看 file changes 概览，判断影响面
|
|||
|
|
4. **本地检出**（可选）：复杂变更建议本地运行验证
|
|||
|
|
|
|||
|
|
### 审查步骤（按优先级）
|
|||
|
|
|
|||
|
|
```
|
|||
|
|
Step 1: 架构审查
|
|||
|
|
- 变更是否符合系统架构设计？
|
|||
|
|
- 模块职责划分是否合理？
|
|||
|
|
- 依赖方向是否正确（不反向依赖）？
|
|||
|
|
|
|||
|
|
Step 2: 逻辑正确性
|
|||
|
|
- 业务逻辑是否正确？边界条件是否覆盖？
|
|||
|
|
- 数据流转是否完整？状态管理是否一致？
|
|||
|
|
|
|||
|
|
Step 3: 安全检查
|
|||
|
|
- 输入是否经过验证和清理？
|
|||
|
|
- 是否存在注入、XSS、CSRF 风险？
|
|||
|
|
- 敏感数据是否加密 / 脱敏？
|
|||
|
|
|
|||
|
|
Step 4: 性能评估
|
|||
|
|
- 是否引入 N+1 查询、大数据量循环？
|
|||
|
|
- 缓存策略是否合理？
|
|||
|
|
- 数据库索引是否匹配新查询？
|
|||
|
|
|
|||
|
|
Step 5: 代码风格
|
|||
|
|
- 命名是否清晰、符合团队约定？
|
|||
|
|
- 代码组织是否合理？是否过度抽象？
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
### 反馈规范
|
|||
|
|
|
|||
|
|
- **区分严重度**：每条反馈标注级别（Blocker / Warning / Suggestion / Nit）
|
|||
|
|
- **提供替代方案**：指出问题的同时给出建议的修改代码
|
|||
|
|
- **解释原因**：说明为什么需要修改，而不是只说"改一下"
|
|||
|
|
- **肯定优点**：发现好的设计和实现也要给予正面反馈
|
|||
|
|
- **对事不对人**：审查代码而非审查人，使用"这段代码"而非"你的代码"
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 严重度分类标准
|
|||
|
|
|
|||
|
|
| 级别 | 标记 | 定义 | 示例 | 处理要求 |
|
|||
|
|
|------|------|------|------|----------|
|
|||
|
|
| **Critical** | `[BLOCKER]` | 必须修复才能合并。存在正确性、安全性或数据完整性问题 | SQL 注入漏洞；竞态条件导致数据不一致；未处理的空指针异常 | 必须修复后重新审查 |
|
|||
|
|
| **Warning** | `[WARNING]` | 强烈建议修复。存在性能隐患、可维护性问题或潜在 Bug | N+1 查询问题；缺少错误处理；过大的函数需要拆分 | 应当修复，可协商延期 |
|
|||
|
|
| **Suggestion** | `[SUGGESTION]` | 改进建议。提升代码质量但不影响功能 | 可以使用更清晰的命名；建议提取公共方法；增加类型约束 | 可选，鼓励采纳 |
|
|||
|
|
| **Nit** | `[NIT]` | 细微问题。风格偏好或微小优化 | 空行数量；import 排序；注释措辞 | 可选，不阻塞合并 |
|
|||
|
|
|
|||
|
|
### 审查反馈模板
|
|||
|
|
|
|||
|
|
```markdown
|
|||
|
|
**[BLOCKER]** SQL 注入风险
|
|||
|
|
- 文件: `src/services/user.ts:42`
|
|||
|
|
- 问题: 用户输入直接拼接到 SQL 查询字符串
|
|||
|
|
- 建议:
|
|||
|
|
```typescript
|
|||
|
|
// 使用参数化查询
|
|||
|
|
const user = await db.query('SELECT * FROM users WHERE id = $1', [userId]);
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
**[SUGGESTION]** 建议提取复用函数
|
|||
|
|
- 文件: `src/utils/format.ts:15-28`
|
|||
|
|
- 问题: 格式化逻辑在多处重复
|
|||
|
|
- 建议: 提取为 `formatCurrency()` 工具函数
|
|||
|
|
```
|
|||
|
|
|
|||
|
|
---
|
|||
|
|
|
|||
|
|
## 审查自检清单（审查者使用）
|
|||
|
|
|
|||
|
|
### 提交审查前自检
|
|||
|
|
|
|||
|
|
- [ ] 是否审查了所有变更文件？
|
|||
|
|
- [ ] 是否理解了变更的业务目的？
|
|||
|
|
- [ ] Critical 问题是否都已标注？
|
|||
|
|
- [ ] 反馈是否具有可操作性（有建议、有代码）？
|
|||
|
|
- [ ] 是否检查了测试覆盖？
|
|||
|
|
- [ ] 是否有正面反馈？
|