ContextQMD
Back to Source Monitor

Rails Codebase Audit - SourceMonitor

tasks/completed/codebase_audit_2025.md

0.13.034.3 KB
Original Source

Rails Codebase Audit - SourceMonitor

Date: October 2025 Overall Grade: B+ Codebase Health: Strong with optimization opportunities

Executive Summary

This comprehensive audit analyzed architecture, code quality, Rails conventions, and frontend patterns across the entire SourceMonitor Rails codebase. The application demonstrates excellent engineering practices with 60+ well-designed service objects, modern Hotwire/Turbo integration, and clean separation of concerns.

Key Strengths:

  • ✅ Extensive use of service objects (60+ in lib/source_monitor)
  • ✅ Modern frontend with Import Maps, Turbo, and Stimulus
  • ✅ No callback hell or fat models
  • ✅ Security-conscious with consistent parameter sanitization
  • ✅ Proper eager loading in most queries

Areas for Improvement:

  • 🔴 1 critical fat controller (356 lines)
  • 🔴 1 N+1 query in sources index
  • 🔴 1 inline script violating CSP
  • 🟠 6 high-severity DRY violations
  • 🟡 11 medium-severity issues

Total Issues Identified: 32 (3 critical, 6 high, 11 medium, 12 low)

Table of Contents

  1. Critical Issues
  2. High Severity Issues
  3. Medium Severity Issues
  4. Low Severity Issues
  5. Positive Findings
  6. Remediation Plan
  7. Detailed Issue Analysis

Critical Issues

1. Fat SourcesController (356 lines)

Severity: 🔴 CRITICAL Location: app/controllers/source_monitor/sources_controller.rb Impact: High technical debt, difficult to test, poor maintainability

Problem: The SourcesController violates the single responsibility principle with multiple methods exceeding 40 lines:

  • destroy (lines 83-143): 61 lines - complex Turbo Stream response building mixed with business logic
  • bulk_scrape_flash_payload (lines 297-343): 47 lines - complex presentation logic
  • respond_to_bulk_scrape (lines 251-295): 45 lines - duplicated response patterns
  • index (lines 14-35): 22 lines - direct analytics object instantiation

Specific Issues:

destroy method (61 lines)

ruby
def destroy
  search_params = sanitized_search_params
  @source.destroy
  message = "Source deleted"

  respond_to do |format|
    format.turbo_stream do
      base_scope = Source.all
      query = base_scope.ransack(search_params)
      query.sorts = [ "created_at desc" ] if query.sorts.blank?
      sources = query.result

      metrics = SourceMonitor::Analytics::SourcesIndexMetrics.new(...)
      # ... 40+ more lines of Turbo Stream building
    end
  end
end

Responsibilities mixed in one method:

  • Database deletion
  • Query rebuilding
  • Metrics recalculation
  • Partial rendering
  • Redirect handling
  • Toast notifications

Solution:

Extract service objects and presenters:

ruby
# app/services/source_monitor/sources/destroy_service.rb
module SourceMonitor
  module Sources
    class DestroyService
      def initialize(source:, search_params:, redirect_to: nil)
        @source = source
        @search_params = search_params
        @redirect_to = redirect_to
      end

      def call
        @source.destroy

        Result.new(
          success: true,
          message: "Source deleted",
          redirect_location: safe_redirect_path,
          updated_query: rebuild_query,
          metrics: recalculate_metrics
        )
      end

      private

      def rebuild_query
        base_scope = Source.all
        query = base_scope.ransack(@search_params)
        query.sorts = ["created_at desc"] if query.sorts.blank?
        query
      end

      def recalculate_metrics
        sources = rebuild_query.result
        SourceMonitor::Analytics::SourcesIndexMetrics.new(
          base_scope: Source.all,
          result_scope: sources,
          search_params: @search_params
        )
      end
    end
  end
end

# app/presenters/source_monitor/sources/turbo_stream_presenter.rb
module SourceMonitor
  module Sources
    class TurboStreamPresenter
      def initialize(source:, responder:)
        @source = source
        @responder = responder
      end

      def render_deletion(metrics:, query:)
        @responder.remove_row(@source)
        @responder.remove("source_monitor_sources_empty_state")
        render_heatmap_update(metrics)
        render_empty_state_if_needed(query)
        self
      end

      private

      def render_heatmap_update(metrics)
        @responder.replace(
          "source_monitor_sources_heatmap",
          partial: "source_monitor/sources/fetch_interval_heatmap",
          locals: {
            fetch_interval_distribution: metrics.fetch_interval_distribution,
            selected_bucket: metrics.selected_fetch_interval_bucket,
            search_params: @search_params
          }
        )
      end

      def render_empty_state_if_needed(query)
        unless query.result.exists?
          @responder.append(
            "source_monitor_sources_table_body",
            partial: "source_monitor/sources/empty_state_row"
          )
        end
      end
    end
  end
end

# Simplified controller:
def destroy
  service = SourceMonitor::Sources::DestroyService.new(
    source: @source,
    search_params: sanitized_search_params,
    redirect_to: params[:redirect_to]
  )

  result = service.call

  respond_to do |format|
    format.turbo_stream do
      responder = SourceMonitor::TurboStreams::StreamResponder.new
      presenter = SourceMonitor::Sources::TurboStreamPresenter.new(
        source: @source,
        responder: responder
      )

      presenter.render_deletion(
        metrics: result.metrics,
        query: result.updated_query
      )

      responder.append_redirect_if_present(result.redirect_location)
      responder.toast(message: result.message, level: :success)

      render turbo_stream: responder.render(view_context)
    end

    format.html do
      redirect_to source_monitor.sources_path, notice: result.message
    end
  end
end

Estimated Effort: 6-8 hours

2. N+1 Query in Sources Index

Severity: 🔴 CRITICAL Location:

  • Controller: app/controllers/source_monitor/sources_controller.rb:20
  • View: app/views/source_monitor/sources/_row.html.erb:3

Impact: Performance degradation with large datasets

  • 100 sources = 100+ database queries
  • 2-5 second page load increase
  • Database connection pool exhaustion under load

Problem:

The view calls SourceMonitor::Analytics::SourceActivityRates.rate_for(source) for each source when item_activity_rates is nil or incomplete:

erb
<% activity_rate = rate_map.fetch(source.id, nil) %>
<% activity_rate = SourceMonitor::Analytics::SourceActivityRates.rate_for(source) if activity_rate.nil? %>

This triggers a database query per source to count items:

ruby
# lib/source_monitor/analytics/source_activity_rates.rb:17-21
def self.rate_for(source)
  return 0.0 if source.items_count.to_i.zero?

  recent_count = source.items.where("created_at > ?", 7.days.ago).count
  recent_count.to_f / 7.0
end

Current Controller Code:

ruby
def index
  base_scope = Source.all
  @search_params = sanitized_search_params
  @q = base_scope.ransack(@search_params)
  @q.sorts = [ "created_at desc" ] if @q.sorts.blank?

  @sources = @q.result  # ⚠️ No activity rates pre-calculation

  # ... metrics calculated but activity rates may be incomplete
end

Solution:

Ensure activity rates are ALWAYS pre-calculated for all sources:

ruby
def index
  base_scope = Source.all
  @search_params = sanitized_search_params
  @q = base_scope.ransack(@search_params)
  @q.sorts = [ "created_at desc" ] if @q.sorts.blank?

  @sources = @q.result

  @search_term = @search_params[SEARCH_FIELD.to_s].to_s.strip
  @search_field = SEARCH_FIELD

  metrics = SourceMonitor::Analytics::SourcesIndexMetrics.new(
    base_scope:,
    result_scope: @sources,
    search_params: @search_params
  )

  @fetch_interval_distribution = metrics.fetch_interval_distribution
  @fetch_interval_filter = metrics.fetch_interval_filter
  @selected_fetch_interval_bucket = metrics.selected_fetch_interval_bucket
  @item_activity_rates = metrics.item_activity_rates

  # ✅ ADD THIS: Ensure we have rates for ALL sources in the current page
  # This prevents the view from calling rate_for individually
  source_ids = @sources.pluck(:id)
  source_ids.each do |id|
    @item_activity_rates[id] ||= 0.0
  end
end

Update the view to never fall back:

erb
<% rate_map = local_assigns[:item_activity_rates] || {} %>
<% activity_rate = rate_map.fetch(source.id, 0.0) %>
<!-- Remove the fallback that causes N+1 -->

Estimated Effort: 1-2 hours

3. Inline JavaScript in View

Severity: 🔴 CRITICAL Location: app/views/source_monitor/shared/_turbo_visit.html.erb:3-8 Impact: CSP violations, untestable code, violates Rails conventions

Problem:

erb
<script>
  (() => {
    const options = { action: "<%= action %>" };
    Turbo.visit("<%= j url %>", options);
  })();
</script>

Issues:

  1. CSP Violations: Inline scripts blocked by strict Content Security Policies
  2. Maintainability: JavaScript logic in ERB templates is harder to test
  3. Separation of Concerns: Business logic mixed with presentation
  4. Missed Opportunity: Could use Turbo's built-in mechanisms

Solution Options:

Option A: Turbo Stream Action (Recommended)

ruby
# In controller where redirect is needed
respond_to do |format|
  format.turbo_stream do
    render turbo_stream: turbo_stream.action(:redirect, url)
  end
end

Create custom Turbo Stream action:

javascript
// app/assets/javascripts/source_monitor/turbo_actions.js
import { StreamActions } from "@hotwired/turbo";

StreamActions.redirect = function () {
	const url = this.getAttribute("url");
	const action = this.getAttribute("action") || "advance";
	Turbo.visit(url, { action });
};

Option B: Stimulus Controller

javascript
// app/assets/javascripts/source_monitor/controllers/redirect_controller.js
import { Controller } from "@hotwired/stimulus";

export default class extends Controller {
	static values = {
		url: String,
		action: { type: String, default: "advance" },
	};

	connect() {
		Turbo.visit(this.urlValue, { action: this.actionValue });
	}
}

Usage:

erb
<div data-controller="redirect"
     data-redirect-url-value="<%= url %>"
     data-redirect-action-value="<%= action %>"></div>

Estimated Effort: 1-2 hours

High Severity Issues

4. default_scope Anti-pattern

Severity: 🟠 HIGH Location: app/models/source_monitor/item.rb:13 Impact: Hidden behavior, counter cache issues, association problems

Problem:

ruby
default_scope { where(deleted_at: nil) }
scope :with_deleted, -> { unscope(where: :deleted_at) }
scope :only_deleted, -> { with_deleted.where.not(deleted_at: nil) }

default_scope is considered an anti-pattern because:

  1. Affects ALL queries globally, including associations
  2. Hard to reason about behavior across codebase
  3. Causes unexpected bugs with eager loading
  4. Requires explicit unscope calls
  5. Makes testing more complex

Evidence of Problems:

ruby
# In item.rb (lines 71-72)
SourceMonitor::Source.decrement_counter(:items_count, source_id) if source_id

# The counter cache is manually managed because default_scope makes
# automatic counter cache unreliable

Solution:

Remove default_scope and use explicit scoping:

ruby
# app/models/source_monitor/item.rb
# Remove: default_scope { where(deleted_at: nil) }

# Add explicit scope
scope :active, -> { where(deleted_at: nil) }
scope :deleted, -> { where.not(deleted_at: nil) }
scope :with_deleted, -> { unscope(where: :deleted_at) }

# Update associations in source.rb
has_many :all_items, class_name: "SourceMonitor::Item", inverse_of: :source, dependent: :destroy
has_many :items, -> { active }, class_name: "SourceMonitor::Item", inverse_of: :source

# Update scopes that use items
scope :recent, -> { active.order(Arel.sql("published_at DESC NULLS LAST, created_at DESC")) }
scope :pending_scrape, -> { active.where(scraped_at: nil) }

# Update controllers to explicitly use .active
def index
  base_scope = Item.active.includes(:source)  # Explicit!
  # ...
end

Estimated Effort: 4-6 hours (requires testing all Item queries)

5. DRY Violation: URL Validation Logic

Severity: 🟠 HIGH Location:

  • app/models/source_monitor/source.rb:108-118
  • app/models/source_monitor/item.rb:77-87

Impact: Maintenance overhead, duplicated logic in 5 methods across 2 files

Problem:

Both models contain nearly identical URL validation methods:

ruby
# Source model
def feed_url_must_be_http_or_https
  return if feed_url.blank?
  errors.add(:feed_url, "must be a valid HTTP(S) URL") if url_invalid?(:feed_url)
end

def website_url_must_be_http_or_https
  return if website_url.blank?
  errors.add(:website_url, "must be a valid HTTP(S) URL") if url_invalid?(:website_url)
end

# Item model
def url_must_be_http
  errors.add(:url, "must be a valid HTTP(S) URL") if url_invalid?(:url)
end

def canonical_url_must_be_http
  errors.add(:canonical_url, "must be a valid HTTP(S) URL") if url_invalid?(:canonical_url)
end

def comments_url_must_be_http
  errors.add(:comments_url, "must be a valid HTTP(S) URL") if url_invalid?(:comments_url)
end

Solution:

Extend the UrlNormalizable concern to handle validation declaratively:

ruby
# lib/source_monitor/models/url_normalizable.rb
module SourceMonitor
  module Models
    module UrlNormalizable
      extend ActiveSupport::Concern

      class_methods do
        def normalizes_urls(*attributes)
          return if attributes.empty?

          before_validation :normalize_configured_urls
          self.normalized_url_attributes += attributes.map(&:to_sym)
          self.normalized_url_attributes.uniq!
        end

        def validates_url_format(*attributes)
          attributes.each do |attribute|
            validate :"validate_#{attribute}_format"

            define_method :"validate_#{attribute}_format" do
              return if self[attribute].blank?
              errors.add(attribute, "must be a valid HTTP(S) URL") if url_invalid?(attribute)
            end
          end
        end
      end

      # ... rest of concern
    end
  end
end

# Then in models:
class Source < ApplicationRecord
  normalizes_urls :feed_url, :website_url
  validates_url_format :feed_url, :website_url
end

class Item < ApplicationRecord
  normalizes_urls :url, :canonical_url, :comments_url
  validates_url_format :url, :canonical_url, :comments_url
end

Estimated Effort: 2-3 hours

6. DRY Violation: Log Model Scopes

Severity: 🟠 HIGH Location:

  • app/models/source_monitor/fetch_log.rb:14-21
  • app/models/source_monitor/scrape_log.rb:11-18

Impact: Duplicate validations, scopes, and attribute defaults

Problem:

Both log models share identical code:

ruby
# FetchLog
validates :started_at, presence: true
validates :duration_ms, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true

scope :recent, -> { order(started_at: :desc) }
scope :successful, -> { where(success: true) }
scope :failed, -> { where(success: false) }

attribute :metadata, default: -> { {} }

# ScrapeLog - IDENTICAL
validates :started_at, presence: true
validates :duration_ms, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true

scope :recent, -> { order(started_at: :desc) }
scope :successful, -> { where(success: true) }
scope :failed, -> { where(success: false) }

attribute :metadata, default: -> { {} }

Solution:

Create shared concern:

ruby
# app/models/concerns/source_monitor/loggable.rb
module SourceMonitor
  module Loggable
    extend ActiveSupport::Concern

    included do
      attribute :metadata, default: -> { {} }

      validates :started_at, presence: true
      validates :duration_ms, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true

      scope :recent, -> { order(started_at: :desc) }
      scope :successful, -> { where(success: true) }
      scope :failed, -> { where(success: false) }
    end
  end
end

# Then use in models:
class FetchLog < ApplicationRecord
  include SourceMonitor::Loggable
  belongs_to :source

  validates :source, presence: true
  validates :items_created, :items_updated, :items_failed,
            numericality: { greater_than_or_equal_to: 0 }
end

class ScrapeLog < ApplicationRecord
  include SourceMonitor::Loggable
  belongs_to :item
  belongs_to :source

  validates :item, :source, presence: true
  validates :content_length, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true
end

Estimated Effort: 1-2 hours

7. DRY Violation: Turbo Stream Response Pattern

Severity: 🟠 HIGH Location:

  • app/controllers/source_monitor/sources_controller.rb:219-249, 251-295
  • app/controllers/source_monitor/items_controller.rb:52-72

Impact: 50+ lines repeated 5+ times, inconsistent responses

Problem:

Pattern repeated across multiple actions:

ruby
# Pattern repeated in multiple action responses:
refreshed = @source.reload
respond_to do |format|
  format.turbo_stream do
    responder = SourceMonitor::TurboStreams::StreamResponder.new

    responder.replace_details(
      refreshed,
      partial: "source_monitor/sources/details_wrapper",
      locals: { source: refreshed }
    )

    responder.replace_row(
      refreshed,
      partial: "source_monitor/sources/row",
      locals: { source: refreshed, item_activity_rates: {...} }
    )

    responder.toast(message:, level:, delay_ms: 5000)
    render turbo_stream: responder.render(view_context)
  end

  format.html do
    redirect_to source_monitor.source_path(refreshed), notice: message
  end
end

Solution:

Extract to controller concern:

ruby
# app/controllers/concerns/source_monitor/turbo_streamable.rb
module SourceMonitor
  module TurboStreamable
    extend ActiveSupport::Concern

    private

    def respond_with_turbo_update(record, message:, level: :info, status: :ok, &customizer)
      refreshed = record.reload

      respond_to do |format|
        format.turbo_stream do
          responder = SourceMonitor::TurboStreams::StreamResponder.new

          # Standard replacements
          replace_record_views(responder, refreshed)

          # Allow custom turbo streams
          customizer&.call(responder, refreshed)

          responder.toast(message: message, level: level, delay_ms: 5000)
          render turbo_stream: responder.render(view_context), status: status
        end

        format.html do
          redirect_to polymorphic_path([:source_monitor, refreshed]), notice: message
        end
      end
    end

    def replace_record_views(responder, record)
      resource_name = record.class.name.demodulize.underscore

      responder.replace_details(
        record,
        partial: "source_monitor/#{resource_name.pluralize}/details_wrapper",
        locals: { resource_name.to_sym => record }
      )

      responder.replace_row(
        record,
        partial: "source_monitor/#{resource_name.pluralize}/row",
        locals: row_locals(record)
      )
    end
  end
end

# Then in controller:
class SourcesController < ApplicationController
  include SourceMonitor::TurboStreamable

  def fetch
    SourceMonitor::Fetching::FetchRunner.enqueue(@source.id)
    respond_with_turbo_update(@source, message: "Fetch has been enqueued")
  end
end

Estimated Effort: 3-4 hours

8. DRY Violation: Ransack Query Setup

Severity: 🟠 HIGH Location:

  • app/controllers/source_monitor/sources_controller.rb:14-23, 90-93
  • app/controllers/source_monitor/items_controller.rb:14-18

Impact: Default sort logic scattered, inconsistent query building

Problem:

Ransack setup duplicated:

ruby
# SourcesController#index
base_scope = Source.all
@search_params = sanitized_search_params
@q = base_scope.ransack(@search_params)
@q.sorts = [ "created_at desc" ] if @q.sorts.blank?
@sources = @q.result

# SourcesController#destroy (turbo_stream format)
base_scope = Source.all
query = base_scope.ransack(search_params)
query.sorts = [ "created_at desc" ] if query.sorts.blank?
sources = query.result

Solution:

Enhance SanitizesSearchParams concern:

ruby
# app/controllers/concerns/source_monitor/sanitizes_search_params.rb
module SourceMonitor
  module SanitizesSearchParams
    extend ActiveSupport::Concern

    class_methods do
      def searchable_with(scope:, default_sorts: ["created_at desc"])
        define_method(:search_scope) { scope }
        define_method(:default_search_sorts) { default_sorts }
      end
    end

    private

    def build_search_query(scope = nil, params: sanitized_search_params)
      base = scope || search_scope
      query = base.ransack(params)
      query.sorts = default_search_sorts if query.sorts.blank?
      query
    end
  end
end

# Then in controllers:
class SourcesController < ApplicationController
  include SourceMonitor::SanitizesSearchParams
  searchable_with scope: -> { Source.all }, default_sorts: ["created_at desc"]

  def index
    @search_params = sanitized_search_params
    @q = build_search_query
    @sources = @q.result
  end
end

Estimated Effort: 2-3 hours

9. Missing NOT NULL Constraints

Severity: 🟠 HIGH Location: test/dummy/db/schema.rb:55-60 Impact: Data integrity risk, no database-level validation

Problem:

Critical fields lack NOT NULL constraints:

ruby
t.string "guid"   # Should be NOT NULL
t.string "url"    # Should be NOT NULL

Models have validations but these are only enforced at application level, not database level.

Solution:

Create migration:

ruby
# db/migrate/YYYYMMDDHHMMSS_add_not_null_constraints_to_items.rb
class AddNotNullConstraintsToItems < ActiveRecord::Migration[8.0]
  def up
    # First, clean up any existing invalid data
    SourceMonitor::Item.where(guid: nil).find_each do |item|
      item.update_column(:guid, item.content_fingerprint || SecureRandom.uuid)
    end

    SourceMonitor::Item.where(url: nil).find_each do |item|
      item.update_column(:url, item.canonical_url || 'https://unknown.example.com')
    end

    # Now add the constraints
    change_column_null :source_monitor_items, :guid, false
    change_column_null :source_monitor_items, :url, false
  end

  def down
    change_column_null :source_monitor_items, :guid, true
    change_column_null :source_monitor_items, :url, true
  end
end

Estimated Effort: 2-3 hours

Medium Severity Issues

10. Non-RESTful Routes

Severity: 🟡 MEDIUM Location: config/routes.rb:8-14

Problem:

ruby
resources :items, only: %i[index show] do
  post :scrape, on: :member  # Non-RESTful
end

resources :sources do
  post :fetch, on: :member        # Non-RESTful
  post :retry, on: :member        # Non-RESTful
  post :scrape_all, on: :member   # Non-RESTful
end

These are actions/commands, not resource updates.

Solution:

ruby
# Option 1: Nested resources
resources :sources do
  resource :fetch, only: [:create], controller: 'source_fetches'
  resource :retry, only: [:create], controller: 'source_retries'
  resource :bulk_scrape, only: [:create], controller: 'source_bulk_scrapes'
end

# Option 2: Explicit command namespace
namespace :commands do
  resources :sources, only: [] do
    post :fetch, on: :member
    post :retry, on: :member
    post :scrape_all, on: :member
  end
end

Estimated Effort: 3-4 hours

11. Multiple after_initialize Callbacks

Severity: 🟡 MEDIUM Location: app/models/source_monitor/source.rb:32-34

Problem:

ruby
after_initialize :ensure_hash_defaults, if: :new_record?
after_initialize :ensure_fetch_status_default
after_initialize :ensure_health_defaults

Solution:

Use Rails attribute API:

ruby
attribute :scrape_settings, default: -> { {} }
attribute :custom_headers, default: -> { {} }
attribute :metadata, default: -> { {} }
attribute :fetch_status, :string, default: "idle"
attribute :health_status, :string, default: "healthy"

# Remove after_initialize callbacks

Estimated Effort: 1 hour

12. Scope with Complex Logic

Severity: 🟡 MEDIUM Location: app/models/source_monitor/source.rb:20-23

Problem:

ruby
scope :due_for_fetch, lambda {
  now = Time.current
  active.where(arel_table[:next_fetch_at].eq(nil).or(arel_table[:next_fetch_at].lteq(now)))
}

Complex logic with variables should be a class method.

Solution:

ruby
def self.due_for_fetch(reference_time: Time.current)
  active.where(
    arel_table[:next_fetch_at].eq(nil).or(arel_table[:next_fetch_at].lteq(reference_time))
  )
end

Estimated Effort: 30 minutes

13. Over-Engineering: Complex Flash Message Building

Severity: 🟡 MEDIUM Location: app/controllers/source_monitor/sources_controller.rb:297-343

Problem: 47 lines of conditional logic in controller

Solution: Extract to presenter (see Issue #1 solution)

Estimated Effort: 2-3 hours

14. Manual Counter Cache Updates

Severity: 🟡 MEDIUM Location: app/models/source_monitor/item.rb:71

Problem:

ruby
SourceMonitor::Source.decrement_counter(:items_count, source_id) if source_id

Manual updates are error-prone.

Solution:

ruby
def soft_delete!(timestamp: Time.current)
  return if deleted?

  self.class.transaction do
    self.deleted_at = timestamp
    save!(validate: false)
    source.touch if source
  end
end

Estimated Effort: 2 hours

15. Overly Permissive Nested Parameters

Severity: 🟡 MEDIUM Location: app/controllers/source_monitor/sources_controller.rb:211-213

Problem:

ruby
scrape_settings: [
  { selectors: %i[content title] }
]

Permits any keys under scrape_settings.

Solution:

ruby
def source_params
  permitted = params.require(:source).permit(
    :name,
    :feed_url,
    # ...
    scrape_settings: {
      selectors: [:content, :title],
      timeout: [],
      javascript_enabled: []
    }
  )
end

Estimated Effort: 1 hour

16-20. Additional Medium Issues

  • 16. Missing Temporal State Concern - Extract time-based state checks
  • 17. Inconsistent Naming - log_filter_status vs filter_fetch_logs
  • 18. Poor Naming - integer_param doesn't convey sanitization
  • 19. Subqueries vs JOINs - Dashboard queries could use JOINs
  • 20. Missing Association Defaults - Add default ordering to associations

Combined Estimated Effort: 6-8 hours

Low Severity Issues

21. Search Forms Trigger Full Page Reloads

Location: app/views/source_monitor/sources/index.html.erb:9

Solution: Add Turbo Frame targeting

Estimated Effort: 2 hours

22. Pagination Triggers Full Page Reloads

Location: app/views/source_monitor/items/index.html.erb:136-146

Solution: Add data: { turbo_frame: "..." } to links

Estimated Effort: 1 hour

23. Global Event Listener

Location: app/assets/javascripts/source_monitor/application.js:19-21

Problem:

javascript
document.addEventListener("turbo:submit-end", () => {
	document.dispatchEvent(new CustomEvent("feed-monitor:form-finished"));
});

Never cleaned up, purpose unclear.

Solution: Document, move to Stimulus, or remove

Estimated Effort: 30 minutes

24-32. Additional Low-Priority Issues

  • 24. Magic Numbers - Toast delays (5000ms vs 6000ms)
  • 25. Inconsistent Variable Naming - refreshed vs @source
  • 26. Missing Parameter Validation - Bulk scrape selection
  • 27. Missing Check Constraint - fetch_status enum
  • 28. Inconsistent Callbacks - Some have conditions, others don't
  • 29. Complex Content Attribute - assign_content_attribute pattern
  • 30. Dropdown Async Import - Could be simplified
  • 31. Missing Performance Indexes - Activity rates, due_for_fetch
  • 32. Database Views - Dashboard queries could use views

Combined Estimated Effort: 4-6 hours

Positive Findings

✅ Excellent Service Object Architecture

60+ well-designed service objects in lib/source_monitor/:

  • SourceMonitor::Fetching::FetchRunner - Coordinates feed fetching
  • SourceMonitor::Scraping::Enqueuer - Handles scrape job queuing
  • SourceMonitor::Scraping::BulkSourceScraper - Bulk scraping orchestration
  • SourceMonitor::Analytics::SourcesIndexMetrics - Metrics calculation
  • SourceMonitor::Dashboard::Queries - Dashboard data queries
  • SourceMonitor::TurboStreams::StreamResponder - Turbo Stream building

Strengths:

  • Clear single responsibility
  • Well-tested in isolation
  • Reusable across controllers and jobs
  • Return value objects (Result structs)

✅ Modern Frontend Architecture (92/100 Score)

CategoryScore
Dependency Management100/100
Stimulus Usage95/100
Turbo Integration90/100
Code Organization95/100
Performance90/100
Maintainability85/100

Strengths:

  • Import Maps with Propshaft (no webpack/node_modules)
  • 4 well-structured Stimulus controllers
  • Effective Turbo Frames and Streams
  • No jQuery or legacy patterns
  • No inline event handlers (onclick, etc.)
  • Progressive enhancement

✅ Skinny, Focused Models

  • Source (129 lines) - Proper size with validations and scopes
  • Item (109 lines) - Clean soft delete logic
  • FetchLog (26 lines) - Simple log record
  • ScrapeLog (30 lines) - Simple log record

No fat models found!

✅ No Callback Hell

  • Only 3 after_initialize callbacks for defaults
  • No problematic before_save, after_save, before_destroy
  • Business logic in service objects, not callbacks

✅ Security-Conscious

  • Consistent use of SourceMonitor::Security::ParameterSanitizer
  • Proper Ransack whitelisting
  • Strong parameters throughout

✅ Proper Eager Loading

Most queries use .includes() appropriately:

ruby
base_scope = Item.includes(:source)
@sources = Source.includes(:fetch_logs).all

Remediation Plan

Phase 1: Critical Fixes (8-12 hours)

Priority: Must fix immediately

  1. Refactor SourcesController (6-8 hours)

    • Extract Sources::DestroyService
    • Extract Sources::TurboStreamPresenter
    • Extract Scraping::BulkResultPresenter

  2. Fix N+1 Query (1-2 hours)

    • Pre-calculate activity rates in index action
    • Update view to remove fallback

  3. Remove Inline Script (1-2 hours)

    • Replace _turbo_visit.html.erb with Turbo Stream action
    • Create custom redirect stream action

Deliverable: 356-line controller reduced to <150 lines, no N+1 queries, no inline JS

Phase 2: High-Impact DRY Violations (12-16 hours)

Priority: High impact on maintainability

  1. URL Validation Concern (2-3 hours)

    • Add validates_url_format to UrlNormalizable
    • Update Source and Item models

  2. Loggable Concern (1-2 hours)

    • Extract shared log behavior
    • Update FetchLog and ScrapeLog

  3. TurboStreamable Concern (3-4 hours)

    • Extract response building pattern
    • Update all controllers

  4. Enhanced SearchParams (2-3 hours)

    • Add build_search_query helper
    • Update both controllers

  5. Replace default_scope (4-6 hours)

    • Use explicit .active scope
    • Update all Item queries
    • Test thoroughly

  6. Database Constraints (2-3 hours)

    • Migration for NOT NULL on guid, url
    • Data cleanup script

Deliverable: 150+ lines of duplicated code eliminated, explicit scoping

Phase 3: Medium Priority (10-15 hours)

Priority: Quality of life improvements

  1. Consolidate Callbacks (1 hour)
  2. Convert Complex Scopes (30 min)
  3. Extract Flash Message Builder (2-3 hours)
  4. Turbo Frame Search (2 hours)
  5. Turbo Frame Pagination (1 hour)
  6. Fix Counter Cache (2 hours)
  7. Tighten Strong Params (1 hour)
  8. Refactor RESTful Routes (3-4 hours) - Optional

Deliverable: Improved UX, cleaner code organization

Phase 4: Polish & Optimization (6-10 hours)

Priority: Nice-to-have

  1. Extract Constants (30 min)
  2. Clean Up Event Listener (30 min)
  3. Rename Methods (1 hour)
  4. Add Performance Indexes (2-3 hours)
  5. Add Check Constraints (1-2 hours)
  6. Database Views (2-3 hours)

Deliverable: Optimized performance, consistent naming

Total Effort Estimate

PhaseHoursPriority
Phase 1: Critical8-12Must Do
Phase 2: High DRY12-16Should Do
Phase 3: Medium10-15Nice to Have
Phase 4: Polish6-10Optional
TOTAL36-53-

Week 1: Critical Fixes

  • Focus on Phase 1 (SourcesController, N+1, inline JS)
  • Immediate impact on code quality and performance

Week 2-3: DRY Violations

  • Phase 2 (concerns, default_scope, constraints)
  • High maintainability impact

Week 4: Polish

  • Cherry-pick Phase 3/4 items based on team priorities
  • Focus on items with highest ROI

Metrics Summary

Issues by Severity

SeverityCount% of Total
Critical39%
High619%
Medium1134%
Low1238%
TOTAL32100%

Issues by Category

CategoryCount
Architecture & Design8
Code Quality (DRY)9
Rails Conventions7
Frontend5
Database3

Code Health Metrics

Service Objects: 60+ ✅
Fat Controllers: 1 (SourcesController)
Fat Models: 0 ✅
Callback Hell: 0 ✅
N+1 Queries: 1 (sources#index)
Inline Scripts: 1 (_turbo_visit.html.erb)
Frontend Score: 92/100 ✅
Overall Grade: B+

Conclusion

This is a well-architected Rails application with strong engineering fundamentals. The 60+ service objects, modern Hotwire integration, and clean models demonstrate excellent design principles.

The issues identified are primarily opportunities for optimization rather than fundamental flaws. The critical issues (fat controller, N+1 query, inline script) are addressable in 8-12 hours and will bring immediate benefits.

Recommendation: Execute Phase 1 immediately, then evaluate ROI for Phase 2-4 based on team capacity and priorities.

Report Generated: January 2025 Analysis Depth: Comprehensive (4 specialized agents) Files Analyzed: 50+ (controllers, models, views, JavaScript, config) Lines of Code Reviewed: 5,000+