rails-code-review

name rails-code-review license MIT description Reviews Rails pull requests, focusing on controller/model conventions, migration safety, query performance, and Rails Way compliance. Covers routing, ActiveRecord, security, caching, and background jobs. Use when reviewing existing Rails code for quality, conducting a PR review, or doing a code review on Ruby on Rails (RoR) code. 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