Skip to content
Development

Cleaning up a quarter's worth of dead code in one PR

By Victor Da Luz
railsrubyrefactoringdev-logblog-manager

I’ve been sitting on a backlog of small hygiene items in blog-manager - leftover columns from a Postiz integration I ripped out, a settings lookup that hits the database on every single call, and a few Ruby-side filters that should’ve been SQL from the start. None of it was urgent enough to fix on its own, so it kept getting deferred. This was the “just do all of it in one PR” issue.

Dead Postiz columns

Before I built native Medium and Dev.to publishing, this app used Postiz as a syndication middleman. That integration is gone, but the columns it left behind weren’t: 7 on posts (postiz_channels, postiz_error, postiz_group_id, postiz_posted_at, postiz_publish, postiz_scheduled_for, postiz_status plus its index), 2 on app_settings (postiz_api_key, postiz_base_url). I grepped the whole app for postiz first - the only hits left were comments explaining why something isn’t done the old Postiz way anymore. Safe to drop. One migration, mirroring the pattern from when I removed the Hashnode and earlier Dev.to/Medium integrations:

class RemovePostizIntegration < ActiveRecord::Migration[8.1]
  def change
    remove_index :posts, :postiz_status
    remove_column :posts, :postiz_channels, :text
    # ...six more remove_column calls
    remove_column :app_settings, :postiz_api_key, :string
    remove_column :app_settings, :postiz_base_url, :string
  end
end

Memoizing AppSetting.current, and why Rails.cache was the wrong tool

AppSetting.current does first_or_create! on every call. There are 11 call sites across controllers, jobs, and views, so a single page render can hit the settings table half a dozen times for a row that almost never changes.

My first instinct was Rails.cache.fetch. Then I remembered AppSetting has encrypts :dev_to_api_key, :medium_bridge_token, :listmonk_api_token, :pexels_api_key - and production’s cache store is solid_cache_store, which persists to a database table. encrypts only protects the column at rest in its own table. Once the record is loaded, those attributes are plaintext in memory. If I cache the whole loaded object, Rails Marshals that plaintext into the cache store’s table. I’d have quietly defeated the encryption by caching around it.

The app already had the right tool sitting there unused for this purpose - Current < ActiveSupport::CurrentAttributes, currently just holding the session:

class Current < ActiveSupport::CurrentAttributes
  attribute :session, :app_setting
  delegate :user, to: :session, allow_nil: true
end
def self.current
  Current.app_setting ||= first_or_create!
end

CurrentAttributes is in-process, thread/fiber-local, and Rails resets it automatically around every controller request and every Active Job execution - which matters here because this app runs a separate Solid Queue worker process. A settings change in the web process is never stale in the worker beyond the job that’s currently running, and nothing ever touches a persistent store. Same call-count reduction, none of the risk.

Moving Ruby-side filters into SQL

Three places were loading posts and filtering in Ruby instead of the database. The controller’s syndication filters mapped filter names to predicate methods, then did @posts.select(&predicate) after loading the whole (~80-row) scope. The dashboard loaded every kept post into an array just to .count(&:needs_medium?) four times. And the newsletter picker was the one with an actual bug: Post.kept.by_pub_date.limit(50).select { |post| post.live_url.present? }. It filters after limiting. If 50 unpublished posts happen to sort ahead of the publishable ones, the newsletter picker silently shows zero selectable posts even though plenty exist just past that boundary.

I added scopes mirroring the existing instance predicates:

scope :needs_medium,       -> { where(medium_status: [ :not_posted, :failed ]) }
scope :needs_devto,        -> { where(devto_status: [ :not_posted, :failed ]) }
scope :syndication_failed, -> { where(medium_status: :failed).or(where(devto_status: :failed)) }
scope :awaiting_publish,   -> { where(medium_status: :draft).or(where(devto_status: :draft)) }
scope :with_live_url,      -> { joins(:blog).where.not(pub_date: nil).where.not(blogs: { base_url: [ nil, "" ] }) }

and flipped the newsletter picker to filter before limiting: Post.kept.includes(:blog).with_live_url.by_pub_date.limit(50).

To make sure the SQL scopes actually matched the Ruby predicates they replaced (rather than just trusting my own translation), I wrote an equivalence test:

assert_equal Post.all.select(&:needs_medium?).map(&:id).sort, Post.needs_medium.pluck(:id).sort

Cheap insurance against an off-by-one enum value silently changing behavior.

The one thing I almost got wrong

The issue described the code in publish_devto_article after an early redirect_to(...) and return as “unreachable… delete it.” I almost did. Looking closer, that’s not throwaway code - it’s the entire native Dev.to publish flow (API key checks, live URL, hero image, GitHub token, job enqueue), built earlier and gated off pending verification against the real Forem API. Deleting it would’ve meant rebuilding the whole thing from git history later.

I gated it with an explicit flag instead:

DEVTO_PUBLISHING_ENABLED = false
# ...
def publish_devto_article
  return redirect_to(@post, alert: DEVTO_DISABLED_MESSAGE) unless DEVTO_PUBLISHING_ENABLED
  # full publish flow, untouched, ready to flip on once verified
end

Same behavior today, and enabling it later becomes a one-line change instead of an archaeology project.

Also in this batch: deleted an unused hello_controller.js (Stimulus generator boilerplate, zero references), and fixed an HTML-escaping gap in the newsletter campaign builder - post titles were escaped but the href wasn’t, so a post title or URL with an & would have produced malformed HTML in the newsletter body.

What’s next

None of this changes user-facing behavior except the newsletter picker under-fill fix, which is a bug fix, not a feature. 282 tests, rubocop, and brakeman all green before merging.

Related reading