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:

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

# ❌ 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

# ❌ 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

# ❌ 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

# ❌ 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

# ❌ 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

# ❌ 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

# ❌ 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

# ✅ 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:

# 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

# ✅ 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

# ✅ 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

# ✅ 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):

# ✅ 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):

# ❌ 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)

# ❌ 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

# ❌ 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 (<nav>, <main>, <article>)
• Form labels properly associated with inputs
• ARIA attributes used appropriately
• Color contrast meets WCAG standards
• Keyboard navigation works correctly
• Alt text for images

Authentication & Authorization

• Use Rails 8 built-in authentication (has_secure_password, authenticate_by, generated Authentication concern)
• Use cancancan for authorization
• Define abilities in app/models/ability.rb
• Use load_and_authorize_resource or authorize! for every action
• Authorization checks on ALL actions that modify data — not just some
• Server-side authorization always — never rely on client-side hiding alone
• Scope queries to current user where appropriate

# app/controllers/application_controller.rb
class ApplicationController < ActionController::Base
  include Authentication  # Rails 8 generated concern
  before_action :require_authentication
end

# ✅ CanCanCan — load and authorize in one call
class PostsController < ApplicationController
  load_and_authorize_resource

  def index
    # @posts already loaded and scoped by cancancan
  end
end

# app/models/ability.rb
class Ability
  include CanCan::Ability

  def initialize(user)
    user ||= User.new  # guest

    if user.admin?
      can :manage, :all
    else
      can :read, :all
      can :manage, Post, user_id: user.id
    end
  end
end

# ❌ Bad: Authorization only in view — server is unprotected
<% if current_user.admin? %>
  <%= link_to "Delete", post_path(@post), method: :delete %>
<% end %>

# Controller has no check — anyone can send DELETE request
def destroy
  @post = Post.find(params[:id])
  @post.destroy
end

# ✅ Good: Server-side authorization
def destroy
  @post = Post.find(params[:id])
  authorize! :destroy, @post  # cancancan check
  @post.destroy
  redirect_to posts_path
end

Database & ActiveRecord

• Meaningful migration names with timestamps
• Always add indexes for foreign keys and frequently queried columns
• Use dependent: :destroy or dependent: :delete_all appropriately
• Validate at both model and database level
• Use database constraints for data integrity
• Use transactions for multi-step operations

Enums

• Always use explicit hash syntax for enums to avoid reordering bugs
• Be aware of auto-generated scopes and bang methods
• Use _prefix or _suffix options when enum names could clash

# ❌ Bad: Array syntax — reordering will break existing data
enum status: [:draft, :published, :archived]

# ✅ Good: Explicit hash — values are stable
enum status: { draft: 0, published: 1, archived: 2 }

# ✅ Good: With prefix to avoid method name clashes
enum status: { active: 0, inactive: 1 }, _prefix: true
# Generates: status_active?, status_inactive?

Model Validations & DB Constraints

• presence: ↔ NOT NULL constraint in DB
• uniqueness: ↔ unique index in DB (note: validates_uniqueness_of alone is not race-condition safe — always back with a unique index)
• Length constraints in both model and DB layers
• Numeric constraints (numericality:) matched by DB check constraints where critical

# ✅ Good: Both layers
# Migration
add_column :users, :email, :string, null: false
add_index :users, :email, unique: true

# Model
class User < ApplicationRecord
  validates :email, presence: true, uniqueness: true
end

Input Validation

• Validate all user inputs at model level
• Use format validations for emails, URLs, etc.
• Sanitize inputs before persistence

class User < ApplicationRecord
  validates :email, format: { with: URI::MailTo::EMAIL_REGEXP }
  validates :age, numericality: { only_integer: true, greater_than: 0 }
  validates :website, format: { with: /\Ahttps?:\/\// }, allow_blank: true

  before_validation :sanitize_inputs

  private

  def sanitize_inputs
    self.name = name.strip if name.present?
    self.bio = ActionController::Base.helpers.sanitize(bio) if bio.present?
  end
end

ActiveRecord Query Safety

• Use find_each / in_batches for bulk processing instead of .each on large sets
• Avoid .all without pagination on large tables
• Be aware of pluck vs select trade-offs (pluck loads into memory immediately)
• Check for missing limit on unbounded queries
• Avoid update_all / delete_all without adequate where clauses
• Use exists? instead of present? or any? for existence checks (avoids loading records)

# ❌ Bad: Loads all records into memory
User.all.each { |u| u.update(synced: true) }

# ✅ Good: Processes in batches
User.where(synced: false).find_each(batch_size: 500) do |user|
  user.update(synced: true)
end

# ❌ Bad: Loads records just to check existence
if User.where(email: email).present?

# ✅ Good: SQL-level existence check
if User.where(email: email).exists?

# ❌ Bad: Unbounded delete
User.where(role: "guest").delete_all  # Could delete millions

# ✅ Good: Scoped and controlled
User.where(role: "guest").where("created_at < ?", 1.year.ago).in_batches.delete_all

N+1 Query Detection & Prevention

• Check for association access in loops — the most common N+1 pattern
• Use Bullet gem in development to detect N+1s automatically
• Understand when to use includes, preload, eager_load, and joins

# ❌ RED FLAG: Accessing associations in loops
@posts.each do |post|
  post.author.name       # N+1 — queries author for EACH post
  post.comments.count    # N+1 — queries comments for EACH post
end

# ✅ includes — Rails picks preload or eager_load automatically
@posts = Post.includes(:author)

# ✅ preload — separate queries (better for large datasets)
Post.preload(:author, :comments)

# ✅ eager_load — LEFT OUTER JOIN (needed when filtering on association)
Post.eager_load(:author).where(authors: { active: true })

# ✅ joins — for filtering only, does NOT load association data
Post.joins(:author).where(authors: { country: "US" })
# Still need includes if you access the association after:
Post.joins(:author).includes(:author).where(authors: { country: "US" })

# ✅ Nested eager loading
Post.includes(comments: :author)

Counter Caches

# ❌ Bad: Count query per post in a loop
@posts.each { |post| post.comments.count }  # N+1

# ✅ Good: Counter cache — zero queries for count
class Comment < ApplicationRecord
  belongs_to :post, counter_cache: true
end

# Migration
add_column :posts, :comments_count, :integer, default: 0
Post.find_each { |post| Post.reset_counters(post.id, :comments) }

# Now free:
@posts.each { |post| post.comments_count }  # No query!

Bullet Gem Setup

# Gemfile
gem "bullet", group: :development

# config/environments/development.rb
config.after_initialize do
  Bullet.enable = true
  Bullet.alert = true
  Bullet.bullet_logger = true
  Bullet.console = true
end

Performance

• Use eager loading (includes, preload, joins) to avoid N+1
• Add database indexes for frequently queried columns
• Use counter caches when appropriate
• Implement pagination for large datasets
• Cache expensive operations

Caching

• Use fragment caching for expensive view partials
• Use Russian doll caching for nested associations
• Cache keys include version/timestamp for automatic expiration
• Avoid caching user-specific content in shared caches
• Use Rails.cache.fetch with explicit expires_in for data caching
• Low-level caching for expensive computations or API calls

# ✅ Good: Fragment caching with touch
# Model
class Comment < ApplicationRecord
  belongs_to :post, touch: true
end

# View (HAML)
- cache @post do
  = render @post.comments

# ✅ Good: Low-level caching
def expensive_stats
  Rails.cache.fetch("user_stats:#{id}", expires_in: 1.hour) do
    calculate_stats
  end
end

Background Jobs

• Use meaningful queue names reflecting priority/purpose
• Keep jobs idempotent — safe to retry
• Use explicit job classes, not inline jobs
• Place in app/jobs/ organized by domain
• Delegate to service objects
• Accept primitive arguments only (IDs, strings) — not full objects (objects can change between enqueue and execution)
• Set proper retry counts and dead-set handling
• Avoid long-running jobs blocking queues
• Consider unique job constraints if duplicate prevention matters

# ❌ Bad: Passing full object — can be stale when executed
class ProcessPaymentJob < ApplicationJob
  def perform(payment)
    payment.process!
  end
end

# ✅ Correct job pattern
class ProcessPaymentJob < ApplicationJob
  queue_as :payments
  sidekiq_options retry: 3

  def perform(payment_id)
    payment = Payment.find(payment_id)
    PaymentProcessor.new(payment).call
  rescue ActiveRecord::RecordNotFound
    # Record deleted between enqueue and execution — safe to skip
    Rails.logger.warn("Payment #{payment_id} not found, skipping")
  end
end

API Structure

• Version APIs from day one (/api/v1/)
• Use Jbuilder for serialization
• Return consistent error formats
• Use HTTP status codes correctly (200, 201, 422, 404, 500)
• Inherit from Api::V1::BaseController

# ✅ Correct API controller
class Api::V1::BaseController < ApplicationController
  respond_to :json

  rescue_from ActiveRecord::RecordNotFound, with: :not_found
  rescue_from ActiveRecord::RecordInvalid, with: :unprocessable_entity

  private

  def not_found
    render json: { error: "Resource not found" }, status: :not_found
  end
end

Serialization

• Jbuilder templates cached where appropriate
• Only expose necessary attributes (no over-fetching)
• Use partial templates for reusable JSON structures
• Consistent date/time formatting across API
• Enum values serialized as human-readable strings, not integers

Serialization Safety

• Never use Marshal.load on untrusted data (security risk — allows arbitrary code execution)
• Use YAML.safe_load instead of YAML.load (prevents object deserialization attacks)
• JSON parsing should handle malformed input gracefully with rescue

# ❌ CRITICAL: Remote code execution risk
data = Marshal.load(params[:data])

# ❌ CRITICAL: Arbitrary object instantiation
config = YAML.load(user_input)

# ✅ Good: Safe deserialization
config = YAML.safe_load(user_input, permitted_classes: [Date, Time])

# ✅ Good: Safe JSON parsing
begin
  data = JSON.parse(raw_body)
rescue JSON::ParserError => e
  render json: { error: "Invalid JSON" }, status: :bad_request
end

API Rate Limiting

• Rate limiting implemented for API endpoints
• Per-IP and per-token limits configured

# Use Rack::Attack for rate limiting
class Rack::Attack
  throttle("api/ip", limit: 300, period: 5.minutes) do |req|
    req.ip if req.path.start_with?("/api/")
  end

  throttle("api/token", limit: 100, period: 1.minute) do |req|
    req.env["HTTP_AUTHORIZATION"] if req.path.start_with?("/api/")
  end
end

Mailers & Notifications

• Mailers inherit from ApplicationMailer
• Email templates exist for both HTML and plain text
• Subject lines use I18n keys
• Mailers tested with email previews (test/mailers/previews/)
• Background jobs used for email delivery (deliver_later)
• Unsubscribe links included where required

# ✅ Correct mailer pattern
class UserMailer < ApplicationMailer
  def welcome(user)
    @user = user
    mail(
      to: @user.email,
      subject: I18n.t("mailers.user_mailer.welcome.subject")
    )
  end
end

Security

• Always use strong parameters
• Sanitize user input
• Use Rails CSRF protection
• Use secure headers
• No hardcoded secrets or credentials
• No mass assignment vulnerabilities

# ❌ Bad: Permit all
params.require(:user).permit!

# ✅ Good: Explicit whitelist
params.require(:user).permit(:name, :email)

SQL Injection — Detection Patterns

Look for these red flags during review:

# ❌ String interpolation in SQL
User.where("email = '#{params[:email]}'")
User.find_by_sql("SELECT * FROM users WHERE id = #{params[:id]}")
ActiveRecord::Base.connection.execute("DELETE FROM posts WHERE id = #{params[:id]}")

# ✅ Safe alternatives
User.where("email = ?", params[:email])            # Parameterized
User.where(email: params[:email])                   # Hash conditions
User.where("email = :email", email: params[:email]) # Named placeholders
User.find_by_sql(["SELECT * FROM users WHERE email = ?", params[:email]])

XSS — Detection Patterns

# ❌ Dangerous
<%= params[:message].html_safe %>
<%= raw @comment.body %>
<script>var msg = "<%= @message %>";</script>

# ✅ Safe
<%= @user.bio %>                                          # Rails auto-escapes
<%= sanitize @comment.body, tags: %w(strong em a) %>      # Controlled allowlist
<script>var msg = <%= @message.to_json %>;</script>        # JSON-escaped

# ✅ Use data attributes instead of inline JS
<div data-message="<%= @message %>"></div>
<script>
  const message = document.querySelector("[data-message]").dataset.message;
</script>

Mass Assignment — Detection Patterns

# ❌ Red flags to catch
params.require(:user).permit!                                    # Permits everything
User.create(params[:user])                                       # No strong params
current_user.attributes = params[:user]                          # Direct assignment
params[:product].each { |k, v| product.send("#{k}=", v) }       # Dynamic assignment

# ✅ Conditional permissions for role-based access
def user_params
  if current_user.admin?
    params.require(:user).permit(:name, :email, :role, :status)
  else
    params.require(:user).

…(truncated)