.vbw-planning/milestones/07-rails-audit-and-refactoring/07-rails-audit-round-2/07-01-RESEARCH.md
5 parallel agents audited the entire SourceMonitor engine codebase across models, controllers, services/jobs/pipeline, views/frontend, and testing layers.
| Severity | Remaining | Already Fixed |
|---|---|---|
| HIGH | 6 | 0 |
| MEDIUM | 17 | 10 |
| LOW | 21 | 9 |
| Total | 44 | 19 |
Overall verdict: The codebase is well-structured for an engine of this complexity. Routes follow CRUD conventions, concerns are well-scoped, and the Hotwire frontend has solid fundamentals. Recent milestone work (phases 01-06) already resolved ~19 findings. The remaining gaps are primarily business logic in jobs (violates "shallow jobs" convention) and duplicated logic across pipeline layers.
Note: 19 findings from the initial 63 were already addressed by commits in the recent ui-fixes-and-smart-scraping milestone. These are marked
strikethroughbelow. The counts above reflect only unresolved findings.
app/jobs/source_monitor/log_cleanup_job.rb:42-49batch.delete_all on FetchLog/ScrapeLog records. These models have has_one :log_entry, dependent: :destroy, but delete_all skips callbacks, orphaning LogEntry records.loggable_type/loggable_id, then delete the log records. Or use destroy_in_batches.dependent: :destroy declaration shows cascade was intended.app/jobs/source_monitor/import_opml_job.rb:14-157SourceMonitor::ImportSessions::OPMLImporter service. The job becomes a 5-line delegation.app/jobs/source_monitor/scrape_item_job.rb:14-57mark_processing!, mark_failed!, clear_inflight!), and re-enqueues itself with a delay.Scraping::Runner. Job becomes a one-liner delegation.time_until_scrape_allowed duplicates near-identical logic in Scraping::Enqueuer#time_rate_limited?. Two places to maintain the same business rule.app/jobs/source_monitor/download_content_images_job.rb:17-49SourceMonitor::Images::Processor. Job delegates with a single call.app/jobs/source_monitor/scrape_item_job.rb:47-57 and lib/source_monitor/scraping/enqueuer.rb:129-143min_scrape_interval. The Enqueuer defers the job, and the Job re-checks and defers again.ScrapeItemJob. The Enqueuer already handles deferral at enqueue time. If race conditions are a concern, have the job delegate to a runner that calls the Enqueuer.Source.destroy_all in pagination tests is not parallel-safetest/controllers/source_monitor/sources_controller_test.rb:252,264,276,286,297,311,322Source.destroy_all to get a clean slate for pagination counting. With thread-based parallelism, this can race with other threads.TEST_CONVENTIONS.md section 6.health_status default mismatch between model and databaseapp/models/source_monitor/source.rb:37 vs db/schema.rb:384attribute :health_status, :string, default: "working" but schema has default: "healthy". A Source created in Ruby gets "working", one via raw SQL gets "healthy".validates :health_status, inclusion: { in: HEALTH_STATUS_VALUES }.health_status validationapp/models/source_monitor/source.rb:44-51fetch_status has an inclusion validation; health_status has none. Any arbitrary string can be stored.HEALTH_STATUS_VALUES = %w[healthy working declining failing].freeze and validates :health_status, inclusion: { in: HEALTH_STATUS_VALUES }.Item#soft_delete! counter cache fragilityapp/models/source_monitor/item.rb:69-83Source.decrement_counter(:items_count, source_id) after update_columns. No corresponding restore! method to re-increment.restore! method for symmetry. Consider extracting soft-delete into a concern.sync_log_entry callback across 3 log modelsapp/models/source_monitor/fetch_log.rb:28, scrape_log.rb:20, health_check_log.rb:20after_save :sync_log_entry callbacks.Loggable concern.rescue_from ActiveRecord::RecordNotFoundapp/controllers/source_monitor/application_controller.rbset_source across 7 controllerssource_fetches_controller.rb, source_retries_controller.rb, source_bulk_scrapes_controller.rb, source_health_checks_controller.rb, source_health_resets_controller.rb, source_favicon_fetches_controller.rb, source_scrape_tests_controller.rbdef set_source; @source = Source.find(params[:source_id]); end.SetSource concern.fallback_user_id creates users in host-app tablesapp/controllers/source_monitor/import_sessions_controller.rb:244-276Rails.env.development? or remove entirely. An engine should never create records in host-app tables.app/controllers/source_monitor/import_sessions/opml_parser.rb (128 lines), entry_annotation.rb (187 lines), health_check_management.rb (112 lines), bulk_configuration.rb (106 lines)lib/ or app/services/ classes. Controller concerns become thin wrappers.SourcesController#index has 47 lines of query orchestrationa6d7148 (extract sources index metrics) + 795b7b8 (SourcesFilterPresenter) + cafefc2 (FilterDropdownComponent)BulkScrapeEnablementsController contains business logicapp/controllers/source_monitor/bulk_scrape_enablements_controller.rb:13-19update_all with field combination for enabling scraping is in the controller.Source.enable_scraping!(ids) class method.update_column usage in ImportSessions flowapp/controllers/source_monitor/import_sessions_controller.rb (11 calls)current_step and selected_source_ids changes.ImportSession#advance_to!(step).app/jobs/source_monitor/favicon_fetch_job.rb:17-42SourceUpdater#enqueue_favicon_fetch_if_needed.Favicons::FetchService. Consolidate cooldown in Favicons::CooldownCheck.app/jobs/source_monitor/import_session_health_check_job.rb:18-63ImportSessions::HealthCheckUpdater.app/jobs/source_monitor/source_health_check_job.rb:29-83toast_payload, broadcast_outcome, trigger_fetch_if_degraded are all presentation/side-effect logic.Health::SourceHealthCheckOrchestrator.e03723d added Result structs to completion handlers; 5bd538a wired Result usage in FetchRunnerFeedFetcher::Result still lacks success?. No shared base SourceMonitor::Result class exists yet.4ff8884 (extract FetchFeedJob retry orchestrator service)feed_fetcher.rb:331, scraping/state.rb:68, source_health_check_job.rb:46-47rescue StandardError => nil silently swallows failures.Rails.logger.warn in rescue blocks.lib/source_monitor/fetching/stalled_fetch_reconciler.rb:107where("arguments::jsonb -> 'arguments' ->> 0 = ?", source.id.to_s) — fragile if SolidQueue changes serialization.items/_details.html.erb:6-7, sources/_bulk_scrape_modal.html.erb:3-11item.scrape_logs.order(...).limit(5) and similar queries directly in ERB.sources/_details.html.erb was partially addressed by SourceDetailsPresenter (7c2604b), but items and bulk scrape modal still have inline queries.sources/_row.html.erb, sources/_details.html.erb, dashboard/_recent_activity.html.erb, items/_details.html.erb, items/index.html.erb, logs/index.html.erb<span class="inline-flex items-center rounded-full ..."> with conditional spinners.StatusBadgeComponent.IconComponent was added (caa4e69) for SVG icons, but status badges are a separate pattern that still needs extraction.SourceDetailsPresenter added in 7c2604bItemDetailsPresenter, FetchLogPresenter, ScrapeLogPresenter, SourceRowPresenter still missing.role="dialog" and aria-modalsources/_bulk_scrape_modal.html.erb, sources/_bulk_scrape_enable_modal.html.erb<div> elements.role="dialog", aria-modal="true", aria-labelledby pointing to heading.app/assets/javascripts/source_monitor/controllers/modal_controller.jsinert attribute on background elements.app/views/source_monitor/logs/index.html.erbform_with uses local: true disabling Turbo. Full page reload on filter.font-semibold vs font-medium, padding variations.@apply CSS classes or a ButtonComponent.ApplicationHelper is 333 lines with mixed concernsapp/helpers/source_monitor/application_helper.rbStatusBadgeHelper, FaviconHelper, PaginationHelper, FetchIntervalHelper, ExternalLinkHelper.SourcesFilterPresenter (795b7b8) and FilterDropdownComponent (cafefc2) extracted some filter logic, but the helper itself is still large.create_item! factory underused18692f6 centralized factory helpers into ModelFactories moduleItem.create! instead of the now-centralized create_item!. Migration to the shared factories is incomplete.new action delegates to create in ImportSessionsController (GET creates records)import_sessions_controller.rb:35-37 — quickBulkScrapeEnablementsController accesses params without strong params wrapperbulk_scrape_enablements_controller.rb:6 — quickSourceScrapeTestsController#create builds result hash inline (should be presenter)source_scrape_tests_controller.rb:14-24 — shortSourceHealthChecksController embeds Tailwind classes in controllersource_health_checks_controller.rb:25-31 — quickrescue StandardError in action controllers6bcd0ac and 19bb3b8 (transient vs fatal error classification in FaviconFetchJob and DownloadContentImagesJob) + 911c17e (deadlock rescue + error logging)SanitizesSearchParams uses to_unsafe_h without documentationconcerns/source_monitor/sanitizes_search_params.rb:45 — quickSourcesController#update contains conditional job-enqueue logicsources_controller.rb:94-109 — shortSource::SearchableAttributes concernsource.rb:79-103 — shortscraping_enabled / scraping_disabled scopessource.rb — quickItemContent#compute_feed_word_count reaches through association (minor Demeter violation)item_content.rb:33-39 — quickImportHistory missing chronological validation and JSONB attribute declarationsimport_history.rb — quickFetchFeedJob#should_run? scheduling logic4ff8884 extracted retry orchestrator, reducing job logic. should_run? guard still exists but is simpler.feed_fetcher.rb:393-404, item_creator.rb:179-197 — mediumfeed_fetcher.rb:30-36 — quickImages::Downloader creates raw Faraday connection instead of HTTP.clientimages/downloader.rb:46-58 — quickLogs::Query is good; Scheduler queries could be extractedscheduler.rb:55-89, scraping/scheduler.rb:38-45 — shortfetching/cloudflare_bypass.rb:39-49 — quick_pagination.html.erb partialitems/index.html.erb:124-146, logs/index.html.erb:186-209 — shortsource_scrape_tests/show.html.erb:8-56, _result.html.erb:12-60 — quickPanelComponent)_form_errors partial)sources/new.html.erb:4-13, edit.html.erb:4-13 — quick491fae1 (simplify dropdown controller and remove JS globals)applyLevelDelay() method15e7d53 (remove dead JS error delay override and document constants)aria-label, should use IconComponent4c56789 replaced inline SVGs with IconComponent. Check if this specific dismiss button was included.FilterDropdownComponent uses inline onchange instead of Stimulus actionfilter_dropdown_component.rb:48,55 — shortconfigure_authentication helper across 4 test files54617b8 created SystemTestHelpers module with shared purge method. Some lib tests may still inline it.| # | Finding | Severity | Effort | Category |
|---|---|---|---|---|
| 1 | H1 — Fix LogCleanupJob orphaned LogEntry records | HIGH | short | Data integrity |
| 2 | H5 — Remove duplicated scrape rate-limiting from ScrapeItemJob | HIGH | quick | DRY |
| 3 | M6 — Extract set_source into shared concern | MEDIUM | quick | DRY |
| 4 | M1+M2 — Align health_status default + add validation | MEDIUM | quick | Correctness |
| 5 | M4 — Move sync_log_entry callback into Loggable concern | MEDIUM | quick | DRY |
| 6 | M5 — Add rescue_from RecordNotFound | MEDIUM | short | Robustness |
| 7 | H4 — Extract DownloadContentImagesJob orchestration | HIGH | short | Convention |
| 8 | H6 — Fix pagination test parallel-safety | HIGH | medium | Test reliability |
| 9 | H2 — Extract ImportOpmlJob business logic to service | HIGH | medium | Convention |
| 10 | M20 — Create StatusBadgeComponent | MEDIUM | medium | DRY/Consistency |
Previously in Top 10, now resolved: M9 (SourcesController#index metrics — extracted to presenters), M16 (retry logic consolidation — extracted to RetryOrchestrator service)
The audit identified many areas where the codebase excels:
source_fetches, source_retries, etc.)inverse_of, dependent: :destroy, and thorough indexingModelFactories) have good defaults with SecureRandom for parallel safetyTEST_CONVENTIONS.md with clear guidanceSourceMonitor.reset_configuration!if defined?(ActiveStorage) checksSources::Params.sanitize with explicit allowlistGenerated by /rails-audit command. Re-run to refresh findings.