rails-code-review

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.

Pre-flight Checks

Before starting the review, verify the following:

• Rails project detected: Confirm you're in a Rails application (check for config/application.rb, Gemfile with rails, or bin/rails)
• Diff available: Ensure you have access to the code changes (git diff, PR diff, or file paths)
• Files exist: Verify all files referenced in the diff exist in the repository
• Review scope clear: Confirm whether reviewing a full PR, a specific feature, or targeted files

If pre-flight checks fail:

• Not a Rails project → Use appropriate review skill for the technology stack
• No diff available → Request the diff or file paths from the user
• Files missing → Flag as Critical and request clarification before proceeding

HARD-GATE: After implementation (before 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.

Quick Reference

Area	Key Checks
Routing	RESTful, shallow nesting, named routes, constraints
Controllers	Skinny, strong params, before_action scoping
Models	Structure order, inverse_of, enum values, scopes over callbacks
Queries	N+1 prevention, exists? over present?, find_each for batches
Migrations	Reversible, indexed, foreign keys, concurrent indexes
Security	Strong params, parameterized queries, no html_safe abuse
Caching	Fragment caching, nested caching, ETags
Jobs	Idempotent, retriable, appropriate backend

Review Order

Work through the diff in this sequence. Deep criteria: REVIEW_CHECKLIST.md. One-page PR baseline: assets/checklist.md. Finding examples (JSON + comment shape): assets/examples.md.

Configuration → Routing → Controllers → Views → Models → Associations → Queries → Migrations → Validations → I18n → Sessions → Security → Caching → Jobs → Tests

Edge case handling:

• Empty diff: If no files changed, state "No code changes to review" and skip to conclusion
• Large diff (>50 files): Prioritize Critical checks first, then sample key files for Suggestion items; flag for targeted follow-up review
• Single file change: Apply all relevant review areas to that file; don't skip areas just because diff is small
• Test-only changes: Focus on test quality, coverage, and test organization; skip application code checks

Critical checks to spot immediately:

# 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

# Privilege escalation via permit!
params.require(:user).permit!                # Bad — never in production
params.require(:user).permit(:name, :email)  # Good

Always Critical (flag every occurrence as Critical):

• params.require(...).permit! — mass-assignment / privilege escalation
• html_safe or raw applied to user-supplied content — XSS
• Missing authorization check on a sensitive action
• Business logic inside a controller action — pricing, tax, discount, multi-step workflow, or any domain calculation inline. A controller action that does more than coordinate (call one service, render response) is Critical, not a Suggestion.
• Unparameterized / string-interpolated SQL — injection
• Destructive migration without a safe path on large tables

Severity levels

Use only these labels (no High/Low, P0–P2, etc.): Critical | Suggestion | Nice to have.

• Critical — security, data loss, crash, or any Always Critical rule → block merge; re-diff after fix.
• Suggestion — conventions / performance → fix in PR, or ticket if redesign is large.
• Nice to have — small style or micro-optimization → optional for the author.

Output style

Group findings under ### Critical / ### Suggestion / ### Nice to have (omit empty sections). Do not use a single flat list mixed by severity.

## Review — <PR title or area>

### Critical
- [path/to/file.rb:LINE] (Area) One-line risk. **Mitigation:** concrete next step.

### Suggestion
- [path/to/file.rb:LINE] (Area) … **Mitigation:** …

### Nice to have
- …

**Actions required:** <one line per severity level that appeared — e.g. Critical → block merge + re-review; Suggestion → …>

Template rules: each bullet is [file:line] (Area) + risk + Mitigation: (required). Tag (Area) from: Controllers, Routing, Views, Models, Queries, Migrations, Validations, Security, Caching, Jobs, Tests — across the whole review, cover ≥4 distinct areas when the diff touches that many surfaces.

Output validation:

• Verify all file paths in [file:line] references exist in the repository
• Ensure line numbers are within the valid range for each file
• Check that each finding includes a required Mitigation: field
• Confirm severity sections are properly categorized
• Validate that Actions required: section summarizes all findings accurately

If file references are invalid:

• Skip the finding and note: [path/to/file.rb:LINE] — File not found in repository, skipping
• Request clarification from the user before including in final review

Re-review before merge

Re-diff the branch after any Critical fix (mandatory), after >3 Suggestion fixes or any logic/architecture change during feedback (recommended), or whenever the fix could alter queries, auth, or migrations. Skip only for Nice to have-only feedback or trivial one-line edits with no behavior change.

Review anti-patterns (adds to checklist, does not replace it)

• Thin controller → fat model: extract orchestration to services (PORO / *.call), not giant model methods.
• N+1 in dev: small seeds hide N+1 — if associations run inside a loop, count queries (request spec, rack-mini-profiler, logs) instead of assuming “it’s fast here.”
• Hot-table migrations: add concurrent indexes and heavy backfills in separate deploy steps from reversible schema changes (chain rails-migration-safety when unsure).
• Callbacks vs jobs: persistence hooks only; external I/O and multi-step workflows belong in services/jobs with clear idempotency.

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