# Rails Code Review > Rails code review skill following Konvenit guidelines. Use when the user asks to review Rails code, check for best practices, audit controllers/models/views, or wants feedback on their Rails application code. Triggers on phrases like "review my code", "check this controller", "audit my Rails app", "code review", "best practices check". Enforces service objects, presenters, HAML, double quotes, Konvenit-specific patterns, and the LGTM/OLGTM PR workflow. Use when this capability is needed. - Skill: `tomevault-io/rails-code-review` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/rails-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/rails-code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/rails-code-review --- # Rails Code Review Skill (Konvenit Guidelines) Perform thorough code reviews following Konvenit's Rails development standards. ## Rules Reference This skill enforces the rules defined in `~/.claude/rules/`. When reviewing, apply these: | Rule File | Covers | |-----------|--------| | `ruby.md` | Style, naming, conditionals, collections, strings | | `architecture.md` | Service objects, presenters, finders, form objects | | `controllers.md` | Thin controllers, strong params, REST conventions | | `models.md` | ActiveRecord, enums, validations, callbacks, associations | | `database.md` | Query optimization, N+1, transactions | | `migrations.md` | Zero-downtime, constraints, rollback safety | | `views.md` | HAML, I18n, partials, semantic HTML | | `testing.md` | TDD philosophy, test types, real objects over mocks | | `rspec.md` | RSpec syntax, subject/let, matchers, FactoryBot | | `capybara.md` | Capybara system spec style, finders, matchers, actions, scoping | | `api.md` | Versioning, Jbuilder, error formats | | `jobs.md` | Idempotency, queue naming, retry strategies | | `security.md` | Auth, CSRF, XSS, session management, mass assignment, file uploads | | `design.md` | SOLID principles, Sandi Metz rules, Law of Demeter | | `mailers.md` | Naming, HTML+text templates, background delivery | | `performance.md` | filter_map, flat_map, match?, Struct over OpenStruct | | `backend.md` | Error handling, caching | | `frontend.md` | Hotwire/Turbo/Stimulus, minimal JS, CSS discipline | | `hotwire.md` | Turbo Frames, Turbo Streams, morphing, Stimulus controller catalog | | `caching.md` | HTTP caching, Russian doll, Solid Cache, counter caches | | `delegated-types.md` | Delegated types vs STI, Contactable pattern | | `webhooks.md` | SSRF protection, delivery lifecycle, signature verification | | `activestorage.md` | Attachment removal, direct uploads, custom keys | | `javascript.md` | JavaScript style (Airbnb), Hotwire/Turbo/Stimulus | | `css.md` | CSS methodology, BEM, variables, specificity | | `git.md` | Commit messages, branch naming, PR hygiene | --- ## Core Mindset - Optimize for **low carrying cost**, not short-term delivery speed - Prioritize **consistency over cleverness** - Be explicit, clear, and pragmatic — no premature abstraction - If trade-off required: **cut scope, not quality** --- ## Reviewing Code Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code). Then: - **Communicate which ideas you feel strongly about and those you don't** - **Identify ways to simplify the code while still solving the problem** - **If discussions turn too philosophical or academic, move the discussion offline to a regular Friday afternoon technique discussion** - In the meantime, let the author make the final decision on alternative implementations. - **Offer alternative implementations** - But assume the author already considered them. - "What do you think about using a custom validator here?" - **Seek to understand the author's perspective** - **Approve the pull request** - **Remember that you are here to provide feedback, not to be a gatekeeper** - When suggesting changes using the "Add a suggestion" feature: - **Communicate clearly which lines you suggest adding/removing** - **Test the suggested changes to validate it works whenever possible** - **When not possible, let the pull request author know that you did not test the suggestion** - This applies to code and information in general. - Be cautious with information you got from an unofficial source _like an LLM or blog post_. - **Provide some context to let the author know why you're suggesting the change** --- ## PR Workflow & Review Culture > **PULL REQUESTS WITH FAILING SPECS ARE NOT APPROVABLE!!!** ### Why Code Reviews - Knowledge transfer across the team - Increased team awareness of changes - Finding alternative solutions ### PR Creator Responsibilities - The **creator of a pull request is responsible** for getting it approved - Assign two other developers to do the review - Remind them if someone forgot - Bring it up in the daily standup if it takes too long ### Preconditions Before Review - [ ] Tests are passing (green) - [ ] The branch is mergeable (no conflicts) ### Review Outcomes **Approved with OLGTM** (minor issues): - Fix it immediately in the current feature branch - If it is an urgent fix, create a review PR and put the link to the origin PR in the description **Approved with LGTM**: - DevOp will merge it to master **Rightly rejected**: - Put the ticket back to TODO and start the process again - If any code *except for changes affecting only specs* has been modified, the ticket **must be retested by QA** - When PR is ready and successfully tested, **re-request the review** — otherwise the reviewer won't notice ### As Reviewer - Be positive - Ask questions — don't dictate a change: - `"What do you think about ...?"` - `"Did you consider ...?"` - `"Can you clarify ...?"` - If you reject the PR, **explain precisely why** — prove that the PR is wrong instead of asking the author to prove it isn't - If there is a reason to not review a pull request (e.g. failing specs), write a comment — otherwise it looks like you're ignoring it ### As Reviewed Developer - Don't take it personally - Try to respond to every comment - Push commits based on earlier rounds of feedback as **isolated commits** to the branch — do not squash until the branch is ready to merge, so reviewers can read individual updates based on their earlier feedback ### Review Mindset - Accept that many programming decisions are opinions — discuss tradeoffs, state your preference, and resolve quickly - Avoid selective ownership of the code ("mine", "not mine", "yours") - Be explicit — people don't always understand your intentions online - Be humble --- ## Code Review Guidelines ### General Principles - [ ] Code follows Konvenit Guidelines (Ruby, Rails, JavaScript) - [ ] Code is maintainable — classes are well-structured and properly named - [ ] If you have trouble understanding concepts, flag it — this indicates structure can be improved - [ ] Explain where you see structural issues or have a hard time grasping concepts - [ ] **Avoid requesting changes based on personal preference** — create Rubocop issue instead - [ ] Check if changes could **unintentionally break production** or have undesirable consequences for users - [ ] Verify all changes are covered by tests - [ ] comments should explain **why**, not **what** — if code is unclear, suggest refactoring instead. - [ ] avoid useless comments at all costs — they add to carrying cost without value - [ ] Suggest concrete improvements with code example - [ ] **EVERY CODE CHANGE MUST BE COVERED BY SPECS** — no exceptions!!! ### Controller Review - [ ] New response formats (format.json, format.xml) in existing controllers are covered by request specs - [ ] New controller actions are covered by request specs ### Security Review - [ ] Check for **SQL injection** vulnerabilities: - Use `quote` for manual escaping - Use parameterized queries: `where("foo LIKE ?", "%#{arg}%")` - Never interpolate user input directly into SQL - [ ] Check for **Cross-Site Scripting (XSS)**: - `html_safe` and `raw` are almost never required - Use `content_tag` helpers instead - If needed, start with empty SafeBuffer: `"".html_safe` and concatenate - [ ] **No production credentials/keys/certificates in commits** (dev/test data is allowed) - [ ] Check for **thread safety issues**: - No class-level instance variables (`@var` at class level) - No unsynchronized shared mutable state - Service objects don't rely on class-level state ### Migration Review - [ ] For large data manipulations, consider moving to a **rake task** for more control - [ ] Check if `NOT NULL` constraints make sense - [ ] ⚠️ **WARNING**: Adding default values to big tables can run a long time - [ ] No superfluous/unused columns - [ ] Check if **indexes** make sense, or if any are missing - [ ] Check if **unique indexes** make sense - [ ] Check if **foreign keys** make sense ### Gemfile & Dependencies - [ ] New gems are necessary and well-maintained - [ ] Gem versions are pinned appropriately (`~>` for patch updates) - [ ] No duplicate or overlapping gems - [ ] Security vulnerabilities checked (`bundle audit`) - [ ] License compatibility verified for commercial projects - [ ] Gemfile organized by purpose (authentication, testing, assets, etc.) ### Configuration & Environment - [ ] Sensitive config uses Rails credentials or ENV vars - [ ] No environment-specific code outside `config/environments/` - [ ] Feature flags properly implemented (if used) - [ ] Database configuration uses sensible pool sizes - [ ] Timezone handling is explicit (`Time.zone`, not `Time.now`) - [ ] No hardcoded URLs or domain names ### Internationalization - [ ] User-facing strings use I18n keys, not hardcoded text - [ ] I18n keys are namespaced logically (`en.controllers.users.create.success`) - [ ] Pluralization rules defined where needed - [ ] Date/time formatting uses I18n helpers - [ ] Any new or changed I18n keys are present in **all** locale files (e.g. `en.yml`, `de.yml`, `fr.yml`) — missing keys in any locale are a bug - [ ] Translation values are spell-checked — typos in locale files appear directly in the UI ### Logging & Observability - [ ] Appropriate log levels used (debug, info, warn, error) - [ ] No sensitive data logged (passwords, tokens, PII) - [ ] Key business events logged for analytics - [ ] Performance-critical operations instrumented - [ ] Structured logging used for searchability ### Data Privacy & Compliance - [ ] PII (Personally Identifiable Information) handling documented - [ ] Data retention policies implemented where needed - [ ] User data export capability (if required by law) - [ ] User data deletion capability (if required by law) - [ ] Audit trails for sensitive operations ## Review Process ### What to Review (Quick Check) - **Naming** — clear, descriptive, following conventions - **Complexity** — can the code be simplified? - **Test Coverage** — are changes covered by specs? - **Style** — should be enforced by Rubocop, not manual review ### Step-by-Step 1. Read the code carefully 2. Check against each rule category below 3. Provide specific, actionable feedback with line references 4. Suggest concrete improvements with code examples 5. Prioritize issues: 🔴 Critical, 🟡 Warning, 🟢 Suggestion --- ## Code Style & Formatting ### Strings & Quotes - [ ] **Always use double quotes** for strings (single quotes only when specifically needed) - [ ] Use string interpolation, not concatenation - [ ] Use `%()` for strings with many quotes - [ ] Use heredocs for multi-line strings - [ ] Use `String#strip` for cleaning whitespace ### Indentation & Layout - [ ] 2 spaces for indentation (no tabs) - [ ] Line length under 100 characters - [ ] All files end with a newline - [ ] Trailing commas in multi-line collections ### Naming Conventions - [ ] `snake_case` for variables, methods, file names - [ ] `PascalCase` for class names - [ ] `SCREAMING_SNAKE_CASE` for constants - [ ] Descriptive names — avoid abbreviations ### Conditionals - [ ] Use guard clauses to reduce nesting - [ ] Use modifier conditionals for simple one-liners - [ ] Use `unless` sparingly — only for simple negative conditions - [ ] **Never use `unless` with `else`** — use `if` instead ### Arrays & Hashes - [ ] Use `%w[]` and `%i[]` for word and symbol arrays - [ ] Use `Hash#fetch` when handling missing keys matters - [ ] Use `Hash.new` with block for complex default values ### Rails-Specific - [ ] Use Rails time helpers instead of Ruby `Time` methods - [ ] Prefer `present?` and `blank?` over `nil?` and `empty?` - [ ] Use safe navigation (`&.`) for potentially nil objects - [ ] Prefer Rails collection methods over raw SQL when possible --- ## SOLID Principles Check code against all five SOLID principles during review. ### Single Responsibility Principle (SRP) - [ ] Each class has **one, and only one, reason to change** - [ ] Class name clearly describes its single purpose - [ ] No class names containing "And", "Manager", or "Handler" (smell for multiple responsibilities) - [ ] Methods within a class operate on the same data/concern ```ruby # ❌ Bad: Multiple responsibilities in one class class User < ApplicationRecord validates :email, presence: true def send_welcome_email # Notification concern UserMailer.welcome_email(self).deliver_now end def generate_activity_report # Reporting concern activities.map { |a| "#{a.created_at}: #{a.description}" }.join("\n") end def sync_to_crm # External integration concern CrmApi.create_contact(email: email, name: name) end end # ✅ Good: Each class has one responsibility class User < ApplicationRecord validates :email, presence: true end class UserNotifier def initialize(user) @user = user end def send_welcome_email UserMailer.welcome_email(@user).deliver_now end end class UserCrmSync def initialize(user) @user = user end def sync CrmApi.create_contact(email: @user.email, name: @user.name) end end ``` ### Open/Closed Principle (OCP) - [ ] Code is **open for extension, closed for modification** - [ ] No long `case`/`if-elsif` chains that grow with new types - [ ] Use polymorphism or strategy pattern where behavior varies by type ```ruby # ❌ Bad: Must modify existing code for each new format class ReportGenerator def generate(report_type, data) case report_type when :pdf then generate_pdf(data) when :csv then generate_csv(data) # Adding new format requires modifying this class end end end # ✅ Good: New formats added without modifying existing code class ReportService def initialize(generator) @generator = generator end def create_report(data) @generator.generate(data) end end class PdfReportGenerator def generate(data) # PDF logic end end # Adding XML doesn't touch existing code class XmlReportGenerator def generate(data) # XML logic end end ``` ### Liskov Substitution Principle (LSP) - [ ] Subclasses are **substitutable** for their parent class without breaking behavior - [ ] No type checking (`is_a?`, `kind_of?`) before calling methods - [ ] Subclasses don't remove or restrict parent functionality - [ ] Subclasses don't throw exceptions the parent doesn't throw ```ruby # ❌ Bad: Subclass breaks the parent contract class Bird def fly "Flying high!" end end class Penguin < Bird def fly raise "Penguins can't fly!" # LSP violation end end # ✅ Good: Better abstraction hierarchy class Bird def move raise NotImplementedError end end class FlyingBird < Bird def move "Flying high!" end end class Penguin < Bird def move "Swimming fast!" end end ``` ### Interface Segregation Principle (ISP) - [ ] No class is forced to implement methods it doesn't use - [ ] Concerns/modules are **small and focused** on a single behavior - [ ] No empty or no-op method implementations to satisfy an interface ```ruby # ❌ Bad: Fat concern forces unrelated behavior module Publishable extend ActiveSupport::Concern def publish; end def unpublish; end def schedule_publication(time); end def generate_social_media_post; end # Not all publishable things need this end # ✅ Good: Segregated concerns — include only what you need module Publishable extend ActiveSupport::Concern def publish update!(published: true, published_at: Time.zone.now) end def unpublish update!(published: false) end end module Schedulable extend ActiveSupport::Concern def schedule_publication(time) update!(scheduled_for: time) end end class BlogPost < ApplicationRecord include Publishable include Schedulable end class Comment < ApplicationRecord include Publishable # No scheduling needed for comments end ``` ### Dependency Inversion Principle (DIP) - [ ] High-level modules **don't depend on low-level modules** — both depend on abstractions - [ ] No direct instantiation of hard-coded dependencies - [ ] Dependencies are injected, making code testable and swappable ```ruby # ❌ Bad: Tightly coupled to concrete implementations class OrderProcessor def process(order) payment = StripePaymentGateway.new payment.charge(order.amount) email = SmtpEmailService.new email.send_confirmation(order.user.email) end end # ✅ Good: Dependencies injected class OrderProcessor def initialize(payment_gateway:, email_service:) @payment_gateway = payment_gateway @email_service = email_service end def process(order) @payment_gateway.charge(order.amount) @email_service.send_confirmation(order.user.email) end end # Easy to swap and test processor = OrderProcessor.new( payment_gateway: StripePaymentGateway.new, email_service: SendgridEmailService.new ) ``` --- ## Law of Demeter **"Only talk to your immediate friends"** — an object should only call methods on itself, its parameters, objects it creates, or its direct instance variables. - [ ] No **train wrecks** (chained method calls through multiple objects) - [ ] Use `delegate` or wrapper methods to hide navigation ```ruby # ❌ Bad: Train wreck — reaching through objects order.user.shipping_address.city.tax_rate @post.author.profile.avatar_url # ✅ Good: Delegation hides the chain class Order delegate :tax_rate, to: :user end class User delegate :tax_rate, to: :shipping_address end # ✅ Good: Rails delegate with prefix class Post < ApplicationRecord delegate :avatar_url, to: :author, prefix: true # Generates: post.author_avatar_url end ``` --- ## Architecture Patterns ### Controllers - [ ] **Keep controllers thin** — delegate to service objects - [ ] **Only one instance variable per action**, named after the resource - [ ] Use strong parameters - [ ] Use `before_action` for common operations - [ ] Follow REST conventions - [ ] Handle errors gracefully with proper HTTP status codes ```ruby # ❌ Bad: Fat controller with business logic def create @user = User.new(user_params) @user.status = "pending" @user.activation_token = SecureRandom.hex(20) UserMailer.welcome(@user).deliver_later if @user.save # ... more logic end # ✅ Good: Delegate to service object def create @user = CreateUser.call(params: user_params) end ``` ### Service Objects - [ ] **Always use service objects** for complex business logic - [ ] Place in `app/services/` - [ ] Name with verb + noun format (`CreateUser`, `ProcessPayment`) - [ ] Single public `call` method - [ ] Return a result object - [ ] Inherit from `BaseService` - [ ] **Establish a convention**: services either raise on failure OR return result objects — pick one and enforce it consistently across the project ```ruby # ✅ Correct service object pattern class CreateUser < BaseService attr_accessor :params def call # Business logic here end end ``` ### Result Object Pattern When services return result objects, use a consistent pattern across the project: ```ruby # app/services/result.rb class Result attr_reader :value, :error def initialize(success:, value: nil, error: nil) @success = success @value = value @error = error end def success? @success end def failure? !@success end def self.success(value = nil) new(success: true, value: value) end def self.failure(error) new(success: false, error: error) end end # ✅ Service using Result class ProcessOrder < BaseService attr_accessor :order_params def call order = Order.create(order_params) return Result.failure(order.errors) unless order.persisted? charge = PaymentService.charge(order) return Result.failure(charge.error) if charge.failure? Result.success(order) end end # ✅ Controller usage def create result = ProcessOrder.call(order_params: order_params) if result.success? redirect_to result.value else @errors = result.error render :new end end ``` ### Presenter Objects - [ ] **Use presenters** for complex view-specific logic - [ ] Place in `app/presenters/` - [ ] Name with noun + `Presenter` format (`UserPresenter`, `OrderPresenter`) - [ ] Inherit from `ApplicationPresenter` - [ ] Use `o.` to access the underlying object ```ruby # ✅ Correct presenter pattern class UserPresenter < ApplicationPresenter def full_name "#{o.first_name} #{o.last_name}".strip end def formatted_created_at o.created_at.strftime("%B %d, %Y") end def status_badge_class o.active? ? "badge-success" : "badge-danger" end end ``` ### Form Objects - [ ] Place in `app/forms/` (if used) - [ ] Name with noun + `Form` format (`UserRegistrationForm`) - [ ] Include `ActiveModel::Model` or inherit from `BaseForm` - [ ] Handle validation logic for complex forms - [ ] Return a result indicating success/failure ```ruby # ✅ Form object pattern class UserRegistrationForm include ActiveModel::Model attr_accessor :email, :password, :terms_accepted validates :email, :password, presence: true validates :terms_accepted, acceptance: true def save return false unless valid? # Create user and related records end end ``` ### Query Objects - [ ] Place in `app/queries/` (if used) - [ ] Name descriptively (`ActiveUsersQuery`, `RecentOrdersQuery`) - [ ] Return an `ActiveRecord::Relation` for chaining - [ ] Single public method (`.call` or `.all`) - [ ] Properly scoped and indexed ```ruby # ✅ Query object pattern — chainable class PostQuery def initialize(relation = Post.all) @relation = relation end def recent @relation.where("created_at > ?", 1.week.ago) end def popular @relation.where("views_count > ?", 1000) end def by_author(author) @relation.where(author: author) end def trending recent.popular.order(views_count: :desc) end end # Usage PostQuery.new.trending PostQuery.new.by_author(current_user).recent ``` ### Models - [ ] Keep models focused on data and simple validations - [ ] **No business logic in models** — extract to service objects - [ ] Use scopes for common queries - [ ] **Avoid callbacks** (`before_*`, `after_*`) unless absolutely necessary - [ ] Prefer explicit orchestration over callbacks ### Callback Rules **Acceptable callbacks** (data normalization within the same record): ```ruby # ✅ OK: Normalizing data before validation class User < ApplicationRecord before_validation :normalize_email private def normalize_email self.email = email.downcase.strip if email.present? end end # ✅ OK: Setting calculated fields on the same record class Post < ApplicationRecord before_save :generate_slug private def generate_slug self.slug = title.parameterize if slug.blank? end end ``` **Unacceptable callbacks** (side effects, external calls, other models): ```ruby # ❌ Bad: Email sending in callback — breaks seed scripts, bulk imports class User < ApplicationRecord after_create :send_welcome_email end # ❌ Bad: External API calls in callback — slows every save class Order < ApplicationRecord after_save :sync_to_crm end # ❌ Bad: Updating other models in callback — hidden dependencies class Comment < ApplicationRecord after_create :update_post_stats end # ✅ Good: Explicit orchestration in service class CreateUser < BaseService def call user = User.create!(params) UserMailer.welcome(user).deliver_later DefaultSettings.create_for(user) user end end ``` **If you must use callbacks for side effects, prefer `after_commit`** (transaction-aware, won't fire on rollback). ### Concerns & Modules - [ ] Concerns should be **small and focused** on a single behavior - [ ] Avoid "junk drawer" concerns that collect unrelated methods - [ ] Prefer composition (service objects) over deep concern hierarchies - [ ] Concerns should not depend on specific model implementation details - [ ] Name concerns after the behavior they provide (`Archivable`, `Searchable`) ```ruby # ❌ Bad: Junk drawer concern — unrelated methods lumped together module UserHelpers extend ActiveSupport::Concern def full_name; end def send_notification; end def calculate_discount; end def export_to_csv; end end # ✅ Good: Focused concern, reused by multiple models module Archivable extend ActiveSupport::Concern included do scope :archived, -> { where.not(archived_at: nil) } scope :active, -> { where(archived_at: nil) } end def archive! update!(archived_at: Time.zone.now) end def archived? archived_at.present? end end ``` --- ## Routing - [ ] **RESTful routes preferred** over custom routes - [ ] Avoid deeply nested routes (max 1 level of nesting) - [ ] No unused/dead routes - [ ] Use `resources` over individual `get`/`post` definitions - [ ] Route constraints for parameter validation where applicable - [ ] API routes namespaced properly (`namespace :api do namespace :v1 do`) - [ ] Use `only:` or `except:` to limit generated routes to what's actually used - [ ] Member and collection routes should be rare — prefer new controllers instead ```ruby # ❌ Bad: Deeply nested and custom routes resources :companies do resources :departments do resources :employees do member do post :activate post :deactivate end end end end # ✅ Good: Shallow nesting, separate controllers for actions resources :companies, only: %i[index show] do resources :departments, only: %i[index show], shallow: true end resources :departments do resources :employees, only: %i[index create], shallow: true end # Separate controller for activation resources :employee_activations, only: %i[create destroy] ``` --- ## Views & Templates - [ ] **Prefer HAML over ERB** - [ ] Extract complex views into presenters - [ ] No database queries in views - [ ] Use `data-testid` attributes for test selectors ### Accessibility (A11y) - [ ] Semantic HTML tags used (`