Rails Code Review Skill (Konvenit Guidelines)

Perform thorough code reviews following Konvenit's Rails development standards.

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

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 at least some tests to catch simple runtime exceptions
• • 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

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

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

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

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

# ✅ Correct service object pattern
class CreateUser < BaseService
  attr_accessor :params

  def call
    # Business logic here
  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
class ActiveUsersQuery
  def self.call(scope = User.all)
    scope
      .where(active: true)
      .where("last_login_at > ?", 30.days.ago)
      .order(last_login_at: :desc)
  end
end

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

# ❌ Bad: Callback with side effects
class User < ApplicationRecord
  after_create :send_welcome_email, :create_default_settings
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

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 authentifikator gem for authentication
• • Use cancancan for authorization
• • Define rules in Authorization::Ability
• • Use web_access block for authentication
• • Use check_authorization and authorize_resource

# ✅ Correct auth pattern
class InvitationsController < ApplicationController
  web_access do
    allow_logged_in :employee
    allow_logged_in :client, if: :team_group_admin?
  end

  check_authorization
  authorize_resource :invitation
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

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

# ❌ Bad: N+1 query
@posts = Post.all
# In view: post.author.name

# ✅ Good: Eager loading
@posts = Post.includes(:author)

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

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

  def perform(payment_id)
    payment = Payment.find(payment_id)
    PaymentProcessor.new(payment).call
  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

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)

Testing

• • Every Ruby code change must be covered by specs — no exceptions
• • Use RSpec with Arrange-Act-Assert pattern
• • Use FactoryBot, not fixtures
• • Test service objects thoroughly
• • Use system specs for happy path
• • System tests must fail when JavaScript is broken
• • Test DB constraints, not just validations
• • Use data-testid attributes for stable selectors
• • Mock external dependencies (RSpec mocks or VCR)
• • Don't over-test controllers — focus on behavior

# ❌ Bad: Code change without specs
# PR contains only: app/services/create_user.rb

# ✅ Good: Code change with corresponding specs
# PR contains:
#   app/services/create_user.rb
#   spec/services/create_user_spec.rb

JavaScript & CSS

JavaScript

• • Minimize JavaScript — use only when absolutely necessary
• • Use Hotwire (Turbo + Stimulus) for interactivity
• • Prefer server-rendered views
• • No complex frontend build systems unless justified
• • Use vanilla JS and web platform APIs where possible
• • Use import maps or ES Modules
• • Ensure JS behavior covered by system tests

CSS

• • Every CSS class is a carrying cost — don't let it grow unconstrained
• • Avoid inline styles unless dynamically generated
• • Keep CSS modular, scoped, predictable
• • Avoid !important and global overrides

Error Handling

• • Use custom exception classes when appropriate
• • Handle errors gracefully in controllers
• • Log errors appropriately (Rollbar)
• • Provide meaningful error messages
• • Never swallow exceptions or fail silently

Rails Version Compatibility

• • No deprecated Rails methods used
• • Check for breaking changes in target Rails version
• • Gems compatible with current/target Rails version
• • No monkey patches that could break on upgrade

Common Code Smells to Flag

• • God objects: Classes with too many responsibilities (>200 lines)
• • Long methods: Methods over 10-15 lines
• • Long parameter lists: More than 3-4 parameters
• • Feature envy: Method using another object's data more than its own
• • Primitive obsession: Using primitives instead of small objects
• • Data clumps: Same group of variables appearing together repeatedly
• • Shotgun surgery: Single change requires edits across many files

Thread Safety & Concurrency

Rails applications run in multi-threaded environments (Puma, Sidekiq). Thread-unsafe code can cause race conditions, data corruption, and intermittent bugs.

Class-Level Instance Variables (Most Common Issue)

• • Never use class-level instance variables (@variable at class level)
• • Use class variables (@@variable) or class instance variables carefully
• • Prefer thread_mattr_accessor or class_attribute for thread-safe class-level state
• • Use RequestStore or Current attributes for request-scoped data

# ❌ CRITICAL: Not thread-safe - will cause race conditions
class UserService
  @current_user = nil  # Class instance variable - DANGEROUS

  def self.process(user)
    @current_user = user  # Race condition! Multiple threads will overwrite this
    # ... logic using @current_user
  end
end

# ❌ CRITICAL: Not thread-safe - shared mutable state
class CacheManager
  @@cache = {}  # Class variable - shared across threads, not thread-safe

  def self.store(key, value)
    @@cache[key] = value  # Race condition!
  end
end

# ✅ Good: Use Rails thread-safe alternatives
class UserService
  thread_mattr_accessor :current_user  # Thread-safe storage

  def self.process(user)
    self.current_user = user
    # ... logic
  ensure
    self.current_user = nil  # Clean up
  end
end

# ✅ Good: Use RequestStore for request-scoped data
class UserService
  def self.process(user)
    RequestStore.store[:current_user] = user
    # ... logic
  end
end

# ✅ Good: Use Rails Current attributes
class Current < ActiveSupport::CurrentAttributes
  attribute :user, :request_id
end

class UserService
  def self.process(user)
    Current.user = user
    # ... logic
  end
end

# ✅ Good: Pass as parameter (best approach)
class UserService
  def self.process(user)
    new(user).call
  end

  def initialize(user)
    @user = user  # Instance variable - safe
  end

  def call
    # ... logic using @user
  end
end

Common Thread Safety Issues

1. Memoization Without Synchronization

# ❌ Bad: Race condition in memoization
def config
  @config ||= load_config  # Multiple threads can call load_config
end

# ✅ Good: Thread-safe memoization
def config
  @config ||= Concurrent::LazyRegister.new { load_config }
end

# ✅ Good: Use Rails.cache for shared data
def config
  Rails.cache.fetch("app_config", expires_in: 1.hour) do
    load_config
  end
end

2. Shared Mutable Collections

# ❌ Bad: Shared array modified by multiple threads
class EventTracker
  @@events = []  # Not thread-safe

  def self.track(event)
    @@events << event  # Race condition!
  end
end

# ✅ Good: Use thread-safe data structures
require "concurrent"

class EventTracker
  @events = Concurrent::Array.new

  def self.track(event)
    @events << event  # Thread-safe
  end
end

# ✅ Better: Use proper logging/event system
class EventTracker
  def self.track(event)
    Rails.logger.info("Event: #{event}")
    # or use proper event tracking service
  end
end

3. Lazy Initialization in Class Methods

# ❌ Bad: Not thread-safe initialization
class ApiClient
  def self.instance
    @instance ||= new  # Race condition!
  end
end

# ✅ Good: Use Rails' thread-safe class_attribute
class ApiClient
  class_attribute :_instance

  def self.instance
    self._instance ||= new
  end
end

# ✅ Better: Use proper singleton pattern
class ApiClient
  include Singleton

  def call
    # ... API logic
  end
end

4. Global State Modification

# ❌ Bad: Modifying global/class state
class FeatureFlag
  @@enabled_features = Set.new

  def self.enable(feature)
    @@enabled_features << feature  # Not thread-safe
  end

  def self.enabled?(feature)
    @@enabled_features.include?(feature)
  end
end

# ✅ Good: Use database or Rails.cache
class FeatureFlag
  def self.enable(feature)
    Rails.cache.write("feature:#{feature}", true)
  end

  def self.enabled?(feature)
    Rails.cache.read("feature:#{feature}") || false
  end
end

Thread Safety Checklist

• • No class-level instance variables (@var at class level)
• • No unsynchronized class variables (@@var)
• • No shared mutable state between requests
• • Memoization uses thread-safe approaches
• • Class methods don't rely on shared state
• • Use thread_mattr_accessor or class_attribute for class-level storage
• • Use Current attributes or RequestStore for request-scoped data
• • Background jobs don't share state across instances
• • Service objects receive dependencies as parameters
• • Singleton patterns use proper synchronization

When Thread Safety Matters Most

• •Background job processors (Sidekiq runs multi-threaded)
• •Service objects called from multiple threads
• •Class-level caching or memoization
• •API clients and external service wrappers
• •Any code that modifies class-level state
• •Concern modules included in multiple classes

Safe Patterns Summary

# ✅ SAFE: Instance variables in instance methods
class UserService
  def initialize(user)
    @user = user  # Safe - each instance has its own
  end
end

# ✅ SAFE: Local variables
def process
  user = User.find(params[:id])  # Safe - method scope
end

# ✅ SAFE: Constants
class Config
  API_ENDPOINT = "https://api.example.com"  # Safe - immutable
end

# ✅ SAFE: Database/cache for shared state
def config
  Rails.cache.fetch("config") { load_config }
end

# ✅ SAFE: Thread-local storage
Thread.current[:user] = user

# ✅ SAFE: RequestStore (request-scoped)
RequestStore.store[:user] = user

# ✅ SAFE: Current attributes (request-scoped)
Current.user = user

also consider the review ruby code See: ../review-ruby-code/skill.md

Output Format

## Code Review: [filename]

### Summary
Brief overview of code quality and main concerns.

### Metrics Summary
- Total issues: X
- Critical: X | Warnings: X | Suggestions: X
- Files reviewed: X
- Test coverage: X% (if available)

### Issues Found

#### 🔴 Critical
- **[Issue]**: Description
  - Line: X
  - Problem: Explanation
  - Fix: Code suggestion

#### 🟡 Warnings
- **[Issue]**: Description
  - Line: X
  - Suggestion: How to improve

#### 🟢 Suggestions
- Minor improvements and style suggestions

### What's Good
- Highlight positive patterns found

### Dependencies Changed (if applicable)
- Added: [list]
- Updated: [list]
- Removed: [list]
- Security concerns: [list]

### Recommended Changes
Prioritized list of changes to make.

### Next Steps
1. [Prioritized action items]
2. ...

Quick Reference: What to Flag