rails-code-review
Compare original and translation side by side
🇺🇸
Original
English🇨🇳
Translation
ChineseRails Code Review (The Rails Way)
Rails代码评审（Rails最佳实践版）
When reviewing Rails code, analyze it against the following areas. When writing new code, follow rails-code-conventions (principles, logging, path rules) and rails-stack-conventions (stack-specific UI and Rails patterns).
Core principle: Review early, review often. Self-review before PR. Re-review after significant changes.
当评审Rails代码时，可从以下维度进行分析。当编写新代码时，请遵循rails-code-conventions（编码原则、日志规范、路径规则）和rails-stack-conventions（技术栈专属UI和Rails设计模式）。
核心原则： 早评审，常评审。提PR前先完成自评审，代码发生重大变更后需要重新评审。
HARD-GATE: After implementation (before PR)
硬门槛：实现完成后（提PR前）
After green tests + linters pass + YARD + doc updates:
1. Self-review the full branch diff using the Review Order below.
2. Fix Critical items; resolve or ticket Suggestion items.
3. Only then open the PR.
generate-tasks must include a "Code review before merge" task.After green tests + linters pass + YARD + doc updates:
1. Self-review the full branch diff using the Review Order below.
2. Fix Critical items; resolve or ticket Suggestion items.
3. Only then open the PR.
generate-tasks must include a "Code review before merge" task.Quick Reference
快速参考
| Area | Key Checks |
|---|---|
| Routing | RESTful, shallow nesting, named routes, constraints |
| Controllers | Skinny, strong params, |
| Models | Structure order, |
| Queries | N+1 prevention, |
| Migrations | Reversible, indexed, foreign keys, concurrent indexes |
| Security | Strong params, parameterized queries, no |
| Caching | Fragment caching, nested caching, ETags |
| Jobs | Idempotent, retriable, appropriate backend |
| 检查维度 | 核心检查项 |
|---|---|
| 路由 | RESTful、浅嵌套、命名路由、路由约束 |
| 控制器 | 保持精简、强参数、 |
| 模型 | 结构顺序规范、 |
| 查询 | N+1问题预防、优先使用 |
| 迁移 | 支持回滚、添加索引、外键约束、使用并发索引 |
| 安全 | 强参数校验、参数化查询、不滥用 |
| 缓存 | 片段缓存、嵌套缓存、ETags |
| 任务 | 幂等性、支持重试、使用合适的后端执行 |
Review Order
评审顺序
Work through the diff in this sequence. See REVIEW_CHECKLIST.md for the full per-area check criteria.
Configuration → Routing → Controllers → Views → Models → Associations → Queries → Migrations → Validations → I18n → Sessions → Security → Caching → Jobs → Tests
Critical checks to spot immediately:
ruby
undefined按照以下顺序依次检查diff，完整的各维度检查标准请查看REVIEW_CHECKLIST.md。
配置 → 路由 → 控制器 → 视图 → 模型 → 关联关系 → 查询 → 迁移 → 校验 → I18n → 会话 → 安全 → 缓存 → 任务 → 测试
需要立即识别的严重问题：
ruby
undefinedN+1 — one query per record in a collection
N+1 — one query per record in a collection
posts.each { |post| post.author.name } # Bad
posts.includes(:author).each { |post| post.author.name } # Good
posts.each { |post| post.author.name } # Bad
posts.includes(:author).each { |post| post.author.name } # Good
Privilege escalation via permit!
Privilege escalation via permit!
params.require(:user).permit! # Bad — never in production
params.require(:user).permit(:name, :email) # Good
**Additional Critical patterns:**
- **Business logic in controller action** (multi-step domain workflow) — flag as Critical; extract to a service object. A controller action doing more than coordinate (call one service, handle response) is a Critical finding.
- **Missing authorization check on sensitive action** — flag as Critical.params.require(:user).permit! # Bad — never in production
params.require(:user).permit(:name, :email) # Good
**其他严重问题模式：**
- **控制器动作中包含业务逻辑**（多步骤领域工作流）—— 标记为严重问题；需要提取到服务对象中。如果控制器动作的职责超出协调范围（调用单个服务、处理响应），就属于严重问题。
- **敏感操作缺少权限校验** —— 标记为严重问题。Severity Levels
严重级别
Use these levels when reporting findings:
| Level | Meaning | Action |
|---|---|---|
| Critical | Security risk, data loss, or crash | Block merge — must fix before approval; mandatory re-review after fix |
| Suggestion | Convention violation or performance concern | Fix in this PR; create a tracked follow-up ticket only if the fix requires significant redesign |
| Nice to have | Style improvement, minor optimization | Optional — author's discretion; no follow-up required |
上报问题时使用以下级别：
| 级别 | 含义 | 处理要求 |
|---|---|---|
| 严重（Critical） | 存在安全风险、数据丢失或崩溃风险 | 阻止合并 —— 审批前必须修复；修复后必须重新评审 |
| 建议（Suggestion） | 违反约定或存在性能隐患 | 需在本次PR中修复；仅当修复需要重大架构调整时可以创建跟进工单后续处理 |
| 可优化（Nice to have） | 代码风格优化、小型优化 | 可选 —— 由作者自行决定是否修改；不需要后续跟进 |
Re-Review Loop
重评审流程
When critical or significant findings were addressed, re-review before merging:
Review → Categorize findings (Critical / Suggestion / Nice to have)
→ Developer addresses findings
→ Critical findings fixed? → Re-review the diff
→ Suggestion items resolved or ticketed?
→ All green → Approve PRRe-review triggers:
- Any Critical finding was present → mandatory re-review after fixes
- More than 3 Suggestion items addressed → re-review recommended
- Logic or architecture changed during feedback → re-review required
Skip re-review only when: All findings were Nice to have or single-line fixes with zero logic change.
当严重问题或重大问题被修复后，合并前需要进行重评审：
Review → Categorize findings (Critical / Suggestion / Nice to have)
→ Developer addresses findings
→ Critical findings fixed? → Re-review the diff
→ Suggestion items resolved or ticketed?
→ All green → Approve PR重评审触发条件：
- 存在任意严重问题 → 修复后必须强制重评审
- 修复超过3个建议级别问题 → 建议重评审
- 反馈处理过程中修改了逻辑或架构 → 必须重评审
仅在以下情况可以跳过重评审： 所有问题都是可优化级别，或者是没有逻辑变更的单行修复。
Pitfalls
常见误区
| Pitfall | What to do |
|---|---|
| "Skinny controller" means move to model | Move to services — avoid fat models |
| Skipping N+1 check because "it's just one query" | One query per record in a collection is N+1 |
| Privilege escalation risk — always whitelist attributes |
| Index added in same migration as column | On large tables, separate migration with |
| Callbacks for business logic | Callbacks are for persistence-level concerns, not orchestration |
| Approving after Critical fix without re-reviewing | A fix can introduce new issues — re-review is mandatory |
| Controller action > ~15 lines | Extract to service — controllers orchestrate, not implement |
| Model with > 3 callbacks | Extract to service or observer |
| XSS risk — escape or sanitize first |
| Migration combining schema change and data backfill | Split: schema migration first, then data migration |
| 误区 | 正确做法 |
|---|---|
| 认为“瘦控制器”就是把逻辑移到模型 | 移到服务对象中 —— 避免胖模型 |
| 因为“只是一次查询”就跳过N+1检查 | 集合中每条记录触发一次查询就是N+1问题 |
为了方便使用 | 存在权限提升风险 —— 永远要对属性做白名单校验 |
| 在添加列的同一个迁移中添加索引 | 对于大表，要拆分迁移，使用 |
| 使用回调处理业务逻辑 | 回调仅用于持久化层面的逻辑，不适合做流程编排 |
| 严重问题修复后没有重评审就批准 | 修复可能引入新问题 —— 重评审是必须的 |
| 控制器动作代码超过~15行 | 提取到服务对象中 —— 控制器只负责流程编排，不负责具体实现 |
| 模型的回调超过3个 | 提取到服务对象或者observer中 |
对用户提供的内容使用 | 存在XSS风险 —— 先做转义或者sanitize处理 |
| 同一个迁移中同时包含schema变更和数据回填 | 拆分：先执行schema迁移，再执行数据迁移 |
Integration
关联技能
| Skill | When to chain |
|---|---|
| rails-review-response | When the developer receives feedback and must decide what to implement |
| rails-architecture-review | When review reveals structural problems |
| rails-security-review | When review reveals security concerns |
| rails-migration-safety | When reviewing migrations on large tables |
| refactor-safely | When review suggests refactoring |
| 技能 | 调用场景 |
|---|---|
| rails-review-response | 当开发者收到评审反馈，需要决定要实现哪些修改时 |
| rails-architecture-review | 当评审发现架构问题时 |
| rails-security-review | 当评审发现安全隐患时 |
| rails-migration-safety | 当评审大表的迁移脚本时 |
| refactor-safely | 当评审建议进行重构时 |