From c57287efd2c98f3c333899453b1405171ba03a40 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 1 Oct 2026 12:09:05 +0200 Subject: [PATCH 1/4] perf(coaches): cache the wall of fame body with a daily key Cache the rendered wall of fame template in Solid Cache, keyed on the UTC date, selected year, page number, and locale, with a 24-hour expires_in. Warm requests skip the distinct coach count, the grouped top-coach query, and the coach card render, which accounted for ~6% of all controller allocation churn on the site (issue #2974). The layout renders fresh on every request so fingerprinted asset URLs and meta tags are never stale; bump the v1 key segment when the view or its partials change. --- app/controllers/dashboard_controller.rb | 21 +++++-- spec/controllers/dashboard_controller_spec.rb | 56 +++++++++++++++++++ 2 files changed, 72 insertions(+), 5 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index ec832794c..5d9e25870 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -31,11 +31,22 @@ def faq; end def about; end def wall_of_fame - @coaches_count = WorkshopInvitation.to_coaches.attended.distinct.count(:member_id) - coaches = Member.where(id: top_coach_query - .year(year_param)) - .includes(:skills) - @pagy, @coaches = pagy(coaches) + key = "coaches/wall_of_fame/v1/#{Time.zone.today}/#{year_param}/#{params[:page] || 1}/#{I18n.locale}" + body = Rails.cache.fetch(key, expires_in: 24.hours) do + @coaches_count = WorkshopInvitation.to_coaches.attended.distinct.count(:member_id) + coaches = Member.where(id: top_coach_query + .year(year_param)) + .includes(:skills) + @pagy, @coaches = pagy(coaches) + render_to_string(layout: false) + end + # The layout renders fresh so asset URLs and meta tags are never stale; + # bump v1 when the wall_of_fame view or its partials change. + # The cached body is fully rendered template output; `.html_safe` prevents + # double-escaping it. `render html:` escapes the string otherwise. + # rubocop:disable Rails/OutputSafety + render html: body.html_safe, layout: 'application' + # rubocop:enable Rails/OutputSafety end def participant_guide; end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index c3ae81f3b..0de4d98d4 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -28,4 +28,60 @@ def assigns(symbol) expect(assigns(:coaches).first.name).to start_with('Coach') end end + + describe 'GET #wall_of_fame caching' do + render_views + + around do |example| + original_cache = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + example.run + Rails.cache = original_cache + end + + let!(:workshop) { Fabricate(:workshop, date_and_time: Time.zone.now) } + + before do + Fabricate(:attended_coach, + member: Fabricate(:member, name: 'Cached', surname: 'Coach'), + workshop:) + end + + it 'stores the rendered body for 24 hours under a date, year, page, and locale key' do + expected_key = "coaches/wall_of_fame/v1/#{Time.zone.today}/#{Time.zone.now.year}/1/en" + + allow(Rails.cache).to receive(:fetch) + .with(expected_key, expires_in: 24.hours) + .and_call_original + + get :wall_of_fame + + expect(response.body).to include('Cached Coach') + expect(response.body).to include(' Date: Thu, 1 Oct 2026 12:25:12 +0200 Subject: [PATCH 2/4] fix(review): normalize page in cache key and strip junk params from cached links Apply ce-code-review findings on the wall of fame cache: - Coerce page to an integer with pagy's own coercion so arbitrary strings cannot expand the cache key space. - Keep only the year (and pagy's page key) in pagy links so the filling request's junk query params are never frozen into the cached body. --- app/controllers/dashboard_controller.rb | 29 +++++++++++++------ spec/controllers/dashboard_controller_spec.rb | 26 +++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 5d9e25870..6690f25be 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -31,15 +31,7 @@ def faq; end def about; end def wall_of_fame - key = "coaches/wall_of_fame/v1/#{Time.zone.today}/#{year_param}/#{params[:page] || 1}/#{I18n.locale}" - body = Rails.cache.fetch(key, expires_in: 24.hours) do - @coaches_count = WorkshopInvitation.to_coaches.attended.distinct.count(:member_id) - coaches = Member.where(id: top_coach_query - .year(year_param)) - .includes(:skills) - @pagy, @coaches = pagy(coaches) - render_to_string(layout: false) - end + body = Rails.cache.fetch(wall_of_fame_cache_key, expires_in: 24.hours) { render_wall_of_fame_body } # The layout renders fresh so asset URLs and meta tags are never stale; # bump v1 when the wall_of_fame view or its partials change. # The cached body is fully rendered template output; `.html_safe` prevents @@ -53,6 +45,25 @@ def participant_guide; end private + def wall_of_fame_cache_key + # Match pagy's page coercion so the cache key and the rendered page always + # agree, and arbitrary strings cannot expand the key space. + page = [params[:page].to_s.to_i, 1].max + "coaches/wall_of_fame/v1/#{Time.zone.today}/#{year_param}/#{page}/#{I18n.locale}" + end + + def render_wall_of_fame_body + @coaches_count = WorkshopInvitation.to_coaches.attended.distinct.count(:member_id) + coaches = Member.where(id: top_coach_query + .year(year_param)) + .includes(:skills) + # pagy copies every request param into pagination links; keep only the + # year the links must preserve so the filling request's junk params are + # not frozen into the cached body. + @pagy, @coaches = pagy(coaches, querify: ->(params) { params.keep_if { |k, _| %w[year page].include?(k) } }) + render_to_string(layout: false) + end + def year_param params.permit(:year)[:year]&.to_i || Time.zone.today.year end diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index 0de4d98d4..aa69126e4 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -83,5 +83,31 @@ def assigns(symbol) expect(Rails.cache.read("#{base_key}/2024/1/en")).to be_present expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/2/en")).to be_present end + + it 'coerces non-numeric page values into the integer page for the cache key' do + get :wall_of_fame, params: { page: '3/de' } + + base_key = "coaches/wall_of_fame/v1/#{Time.zone.today}" + expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/3/en")).to be_present + # A page string cannot place one locale's body under another locale's key. + expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/3/de")).to be_nil + end + + it 'keeps unrelated query params out of the cached pagination links' do + 21.times do |i| + Fabricate(:attended_coach, + member: Fabricate(:member, name: "Wall#{i}", surname: 'Coach'), + workshop:) + end + + get :wall_of_fame, params: { fbclid: 'spam' } + + # The layout's og:url mirrors the request URL and is never cached, so + # assert against the cached body itself. + base_key = "coaches/wall_of_fame/v1/#{Time.zone.today}" + cached = Rails.cache.read("#{base_key}/#{Time.zone.now.year}/1/en") + expect(cached).to include('page=2') + expect(cached).not_to include('fbclid') + end end end From 91fbd7191c2b05f0c33d46f4afb1fdde1a69a07b Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 1 Oct 2026 13:30:31 +0200 Subject: [PATCH 3/4] perf(coaches): keep past-year wall of fame entries out of the daily rotation Past years' coach attendance is fixed, so only the current year needs a daily cache rotation. Past-year keys drop the date segment and carry no explicit expiry: entries age out via Solid Cache's max_age (2 weeks by default), so profile edits on old coach cards still propagate, just slower. Junk years stay on the dated, expiring path so they cannot create immortal entries. --- app/controllers/dashboard_controller.rb | 14 ++++-- spec/controllers/dashboard_controller_spec.rb | 44 +++++++++++++++---- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 6690f25be..68e465d96 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -31,9 +31,12 @@ def faq; end def about; end def wall_of_fame - body = Rails.cache.fetch(wall_of_fame_cache_key, expires_in: 24.hours) { render_wall_of_fame_body } + options = past_year? ? {} : { expires_in: 24.hours } + body = Rails.cache.fetch(wall_of_fame_cache_key, **options) { render_wall_of_fame_body } # The layout renders fresh so asset URLs and meta tags are never stale; - # bump v1 when the wall_of_fame view or its partials change. + # bump v2 when the wall_of_fame view or its partials change. + # Past-year entries carry no explicit expires_in and age out via Solid + # Cache's max_age (2 weeks by default); current-year entries expire daily. # The cached body is fully rendered template output; `.html_safe` prevents # double-escaping it. `render html:` escapes the string otherwise. # rubocop:disable Rails/OutputSafety @@ -45,11 +48,16 @@ def participant_guide; end private + def past_year? + (2013...Time.zone.today.year).cover?(year_param) + end + def wall_of_fame_cache_key # Match pagy's page coercion so the cache key and the rendered page always # agree, and arbitrary strings cannot expand the key space. page = [params[:page].to_s.to_i, 1].max - "coaches/wall_of_fame/v1/#{Time.zone.today}/#{year_param}/#{page}/#{I18n.locale}" + date_segment = past_year? ? nil : "#{Time.zone.today}/" + "coaches/wall_of_fame/v2/#{date_segment}#{year_param}/#{page}/#{I18n.locale}" end def render_wall_of_fame_body diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb index aa69126e4..9f2fbcd08 100644 --- a/spec/controllers/dashboard_controller_spec.rb +++ b/spec/controllers/dashboard_controller_spec.rb @@ -47,8 +47,8 @@ def assigns(symbol) workshop:) end - it 'stores the rendered body for 24 hours under a date, year, page, and locale key' do - expected_key = "coaches/wall_of_fame/v1/#{Time.zone.today}/#{Time.zone.now.year}/1/en" + it 'stores the current-year body for 24 hours under a date, year, page, and locale key' do + expected_key = "coaches/wall_of_fame/v2/#{Time.zone.today}/#{Time.zone.now.year}/1/en" allow(Rails.cache).to receive(:fetch) .with(expected_key, expires_in: 24.hours) @@ -63,6 +63,33 @@ def assigns(symbol) expect(Rails.cache.read(expected_key)).to be_present end + it 'stores the past-year body without a date segment or explicit expiry' do + past_workshop = Fabricate(:workshop, date_and_time: Time.zone.local(2013, 6, 1)) + Fabricate(:attended_coach, + member: Fabricate(:member, name: 'Cached', surname: 'Coach'), + workshop: past_workshop) + expected_key = 'coaches/wall_of_fame/v2/2013/1/en' + + allow(Rails.cache).to receive(:fetch) + .with(expected_key) + .and_call_original + + get :wall_of_fame, params: { year: 2013 } + + expect(response.body).to include('Cached Coach') + expect(Rails.cache).to have_received(:fetch) + .with(expected_key) + expect(Rails.cache.read(expected_key)).to be_present + end + + it 'serves junk years from a dated, expiring key' do + get :wall_of_fame, params: { year: 1999 } + + base_key = "coaches/wall_of_fame/v2/#{Time.zone.today}" + expect(Rails.cache.read("#{base_key}/1999/1/en")).to be_present + expect(Rails.cache.read('coaches/wall_of_fame/v2/1999/1/en')).to be_nil + end + it 'serves the cached body without re-rendering from the database' do get :wall_of_fame expect(response.body).to include('Cached Coach') @@ -75,19 +102,18 @@ def assigns(symbol) it 'rotates the cache key with the year and page parameters' do get :wall_of_fame - get :wall_of_fame, params: { year: 2024 } + get :wall_of_fame, params: { year: 2013 } get :wall_of_fame, params: { page: 2 } - base_key = "coaches/wall_of_fame/v1/#{Time.zone.today}" - expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/1/en")).to be_present - expect(Rails.cache.read("#{base_key}/2024/1/en")).to be_present - expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/2/en")).to be_present + expect(Rails.cache.read("coaches/wall_of_fame/v2/#{Time.zone.today}/#{Time.zone.now.year}/1/en")).to be_present + expect(Rails.cache.read('coaches/wall_of_fame/v2/2013/1/en')).to be_present + expect(Rails.cache.read("coaches/wall_of_fame/v2/#{Time.zone.today}/#{Time.zone.now.year}/2/en")).to be_present end it 'coerces non-numeric page values into the integer page for the cache key' do get :wall_of_fame, params: { page: '3/de' } - base_key = "coaches/wall_of_fame/v1/#{Time.zone.today}" + base_key = "coaches/wall_of_fame/v2/#{Time.zone.today}" expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/3/en")).to be_present # A page string cannot place one locale's body under another locale's key. expect(Rails.cache.read("#{base_key}/#{Time.zone.now.year}/3/de")).to be_nil @@ -104,7 +130,7 @@ def assigns(symbol) # The layout's og:url mirrors the request URL and is never cached, so # assert against the cached body itself. - base_key = "coaches/wall_of_fame/v1/#{Time.zone.today}" + base_key = "coaches/wall_of_fame/v2/#{Time.zone.today}" cached = Rails.cache.read("#{base_key}/#{Time.zone.now.year}/1/en") expect(cached).to include('page=2') expect(cached).not_to include('fbclid') From 7db5d01e82a8d102c0b27b5b693a16e24f0ceff3 Mon Sep 17 00:00:00 2001 From: Morgan Roderick Date: Thu, 1 Oct 2026 14:02:43 +0200 Subject: [PATCH 4/4] refactor(review): drop wall_of_fame caching explanation comment per PR feedback --- app/controllers/dashboard_controller.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 68e465d96..da242f0ee 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -33,12 +33,6 @@ def about; end def wall_of_fame options = past_year? ? {} : { expires_in: 24.hours } body = Rails.cache.fetch(wall_of_fame_cache_key, **options) { render_wall_of_fame_body } - # The layout renders fresh so asset URLs and meta tags are never stale; - # bump v2 when the wall_of_fame view or its partials change. - # Past-year entries carry no explicit expires_in and age out via Solid - # Cache's max_age (2 weeks by default); current-year entries expire daily. - # The cached body is fully rendered template output; `.html_safe` prevents - # double-escaping it. `render html:` escapes the string otherwise. # rubocop:disable Rails/OutputSafety render html: body.html_safe, layout: 'application' # rubocop:enable Rails/OutputSafety