Rails Code Review (The Rails Way)

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.

HARD-GATE: After implementation (before PR)

Code review is part of delivery, not only "when someone comments on GitHub."

After implementation + green tests + linters pass + YARD + doc updates:

1. Run a self-review on the full branch diff using the Review Order below.
2. Fix Critical items; address Suggestion items or ticket follow-ups explicitly.
3. Only then open the PR or hand off for human review.

Skipping self-review treats the plan as unfinished. generate-tasks must end
with a "Code review before merge" parent task.

Quick Reference

Review Order

1. Configuration & Environments — Encrypted credentials, Zeitwerk autoloading, per-environment logging.
2. Routing — RESTful resources/resource, max one level nesting (prefer shallow), named routes, constraints.
3. Controllers — Action order (index, show, new, edit, create, update, destroy), strong params with permit, before_action with only:/except:, skinny controllers, respond_to for formats.
4. Action View — Partials and layouts, no logic in views (use helpers/presenters), content_for/yield, Rails helpers over raw HTML.
5. ActiveRecord Models — Structure order: extends, includes, constants, attributes, enums, associations, delegations, validations, scopes, callbacks, class methods, instance methods. Use inverse_of, explicit enum values, validates (not validates_presence_of), scopes for reusable queries, limit callbacks.
6. Associations — dependent: for orphaned records, through: for many-to-many, STI only when justified.
7. Queries — includes/preload/eager_load for N+1, exists? over present?, pluck for arrays, find_each for large sets, insert_all for bulk, load_async (Rails 7+), transactions for atomicity.
8. Migrations — Reversible (change), indexes on WHERE/JOIN columns, add_reference with foreign_key: true.
9. Validations — Built-in validators, conditional (if:/unless:), custom validators for complex rules.
10. I18n — User-facing strings via I18n, lazy lookup in views, locale from user preferences or headers.
11. Sessions & Cookies — No complex objects in session, signed/encrypted cookies, flash for temporary messages.
12. Security — Strong params, parameterized queries, no unnecessary raw/html_safe, protect_from_forgery, CSP headers, masked sensitive data in logs.
13. Caching & Performance — Fragment caching, nested caching, Rails.cache, ETags, EXPLAIN for slow queries.
14. Background Jobs — Active Job, idempotent and retriable, appropriate backend.
15. Testing (RSpec) — BDD, descriptive blocks, let/let!, FactoryBot, shared examples, mocked external services.

Severity Levels

Use these levels when reporting findings:

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 PR

Re-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.

Common Mistakes

Red Flags

• Controller action longer than ~15 lines of logic
• Model with more than 3 callbacks
• permit! anywhere in production code
• Query without eager loading inside a loop
• Migration combining schema change and data backfill
• html_safe or raw on user-provided content
• Approving a PR after Critical findings without re-reviewing
• Using "should", "probably" when claiming something passes

Integration