From e991ead4f6f112eab1ed2d11e434a2e1a62aea32 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 08:58:54 -0400 Subject: [PATCH 1/8] Start: load admin home badge counts in one request Co-Authored-By: Claude Opus 5.5 (1M context) From 75b024365983e57119fde754e3b257a9d25f1fe8 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 09:01:14 -0400 Subject: [PATCH 2/8] Load admin home badge counts in one request Each badge was its own lazy frame, so opening admin home fired three slow count requests at once. Puma runs three threads, so they filled every thread and the admin's next click queued behind them until the counts finished. One frame request holds a single thread and leaves the rest free for navigation. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/admin/home_controller.rb | 34 ++++------ .../admin/home/_count_badge_frame.html.erb | 3 - app/views/admin/home/badge_counts.html.erb | 7 +++ app/views/admin/home/index.html.erb | 7 ++- config/routes.rb | 4 +- ...home-clicks-without-waiting-for-badges.yml | 9 +++ spec/requests/admin/home_spec.rb | 62 ++++++++----------- 7 files changed, 58 insertions(+), 68 deletions(-) delete mode 100644 app/views/admin/home/_count_badge_frame.html.erb create mode 100644 app/views/admin/home/badge_counts.html.erb create mode 100644 db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index 2da18cbe46..e0559180b9 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -17,35 +17,23 @@ def index @pending_change_requests_count = ProfileChangeRequest.pending.count end - def data_health_count + def badge_counts authorize! :home, to: :index? - render_count_badge(:data_health_count, title: DATA_HEALTH_BADGE_TITLE) { DataHealth.checks.sum(&:count) } - end - - def author_credit_count - authorize! :home, to: :index? - - render_count_badge(:author_credit_count) { DataHealth::Worklists.new.author_credit.sum(&:count) } - end - - def unsynced_features_count - authorize! Feature, to: :create? - - render_count_badge(:unsynced_features_count) { FeatureCatalog.new.missing_count } + @data_health_count = safe_count { DataHealth.checks.sum(&:count) } + @author_credit_count = safe_count { DataHealth::Worklists.new.author_credit.sum(&:count) } + @unsynced_features_count = safe_count { FeatureCatalog.new.missing_count } if allowed_to?(:create?, Feature) + render :badge_counts end private - # A badge is a nicety, so a failing count renders an empty frame and leaves the button usable. - def render_count_badge(frame_id, title: nil) - count = begin - yield - rescue StandardError => e - Rails.error.report(e) - nil - end - render partial: "count_badge_frame", locals: { frame_id: frame_id, count: count, title: title } + # A badge is a nicety, so a failing count leaves its badge empty and the button usable. + def safe_count + yield + rescue StandardError => e + Rails.error.report(e) + nil end end end diff --git a/app/views/admin/home/_count_badge_frame.html.erb b/app/views/admin/home/_count_badge_frame.html.erb deleted file mode 100644 index 3ace5eae81..0000000000 --- a/app/views/admin/home/_count_badge_frame.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= turbo_frame_tag frame_id do %> - <%= render "count_badge", count: count, title: title %> -<% end %> diff --git a/app/views/admin/home/badge_counts.html.erb b/app/views/admin/home/badge_counts.html.erb new file mode 100644 index 0000000000..8ffd28679a --- /dev/null +++ b/app/views/admin/home/badge_counts.html.erb @@ -0,0 +1,7 @@ +<%= turbo_frame_tag :badge_counts do %> + <%= turbo_stream.update "data_health_count", partial: "count_badge", locals: { count: @data_health_count, title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> + <%= turbo_stream.update "author_credit_count", partial: "count_badge", locals: { count: @author_credit_count } %> + <% if @unsynced_features_count %> + <%= turbo_stream.update "unsynced_features_count", partial: "count_badge", locals: { count: @unsynced_features_count } %> + <% end %> +<% end %> diff --git a/app/views/admin/home/index.html.erb b/app/views/admin/home/index.html.erb index 09bec057d4..5bfa72201b 100644 --- a/app/views/admin/home/index.html.erb +++ b/app/views/admin/home/index.html.erb @@ -159,7 +159,7 @@ <% end %> <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> Data health - <%= turbo_frame_tag :data_health_count, src: admin_home_data_health_count_path, loading: :lazy %> + <%= tag.span id: "data_health_count" %> <% end %> <% if allowed_to?(:import?, EventRegistration) %> <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> @@ -168,17 +168,18 @@ <% end %> <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> Author credit divergences - <%= turbo_frame_tag :author_credit_count, src: admin_home_author_credit_count_path, loading: :lazy %> + <%= tag.span id: "author_credit_count" %> <% end %> <% if allowed_to?(:create?, Feature) %> <%= button_to import_features_path, class: button_classes(:primary_outline), form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> Sync features & tips - <%= turbo_frame_tag :unsynced_features_count, src: admin_home_unsynced_features_count_path, loading: :lazy %> + <%= tag.span id: "unsynced_features_count" %> <% end %> <% end %> + <%= turbo_frame_tag :badge_counts, src: admin_home_badge_counts_path %> diff --git a/config/routes.rb b/config/routes.rb index 1a6281748f..29ecf53714 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,9 +56,7 @@ namespace :admin do get "/", to: "home#index" # admin home page - get "home/data_health_count", to: "home#data_health_count", as: "home_data_health_count" - get "home/author_credit_count", to: "home#author_credit_count", as: "home_author_credit_count" - get "home/unsynced_features_count", to: "home#unsynced_features_count", as: "home_unsynced_features_count" + get "home/badge_counts", to: "home#badge_counts", as: "home_badge_counts" get "activities/events", to: "ahoy_activities#index", as: "activities_events" get "activities/events/:id", to: "ahoy_activities#show", as: "activities_event" get "activities/visits", to: "ahoy_activities#visits", as: "activities_visits" diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml new file mode 100644 index 0000000000..84a04c23f0 --- /dev/null +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -0,0 +1,9 @@ +name: "Admin home links work while the action counts load" +area: reporting +display_status: admin_facing +released_on: 2026-10-25 +pr_number: 2847 +summary: >- + The counts on the admin home's Data health, Author credit divergences, and + Sync features buttons now load together in the background, so clicking + through to another page no longer waits for them to finish. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index c28b71416f..09f81def13 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -82,72 +82,62 @@ expect(response.body).to include(%(href="#{organization_statuses_path}")) end - it "loads the action counts in lazy frames" do + it "loads every action count in one frame request so the badges hold a single server thread" do get "/admin" - expect(response.body).to include( - %(src="#{admin_home_data_health_count_path}"), - %(src="#{admin_home_author_credit_count_path}"), - %(src="#{admin_home_unsynced_features_count_path}") - ) + expect(response.body.scan(/]* src=/).size).to eq(1) + expect(response.body).to include(%(src="#{admin_home_badge_counts_path}")) + expect(response.body).to include(%(id="data_health_count"), %(id="author_credit_count"), %(id="unsynced_features_count")) end - describe "count badges" do - it "renders the Data health total inside its frame" do - allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 2), instance_double(DataHealth::Check, count: 3) ]) - - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + describe "badge counts" do + def get_badge_counts + get admin_home_badge_counts_path, headers: { "Turbo-Frame" => "badge_counts" } + end - expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="data_health_count")) - expect(response.body).to match(%r{]*>5}) - expect(response.body).to include("Data checks only") + def badge_stream(target) + response.body[%r{.*?}m] end - it "renders the Author credit total inside its frame" do + before do + allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 2), instance_double(DataHealth::Check, count: 3) ]) entries = [ 1, 4 ].map { |count| DataHealth::Worklists::Entry.new("Label", count, "record", "/") } allow_any_instance_of(DataHealth::Worklists).to receive(:author_credit).and_return(entries) - - get admin_home_author_credit_count_path, headers: { "Turbo-Frame" => "author_credit_count" } - - expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="author_credit_count")) - expect(response.body).to match(%r{]*>5}) - end - - it "renders how many seeded features aren't synced yet" do allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(3) + end - get admin_home_unsynced_features_count_path, headers: { "Turbo-Frame" => "unsynced_features_count" } + it "updates each badge from inside the frame" do + get_badge_counts expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="unsynced_features_count")) - expect(response.body).to match(%r{]*>3}) + expect(response.body).to include(%(id="badge_counts")) + expect(badge_stream("data_health_count")).to match(%r{]*>5}).and include("Data checks only") + expect(badge_stream("author_credit_count")).to match(%r{]*>5}) + expect(badge_stream("unsynced_features_count")).to match(%r{]*>3}) end it "renders no badge when nothing is waiting" do allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 0) ]) - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_badge_counts - expect(response.body).to include(%(id="data_health_count")) - expect(response.body).not_to include("rounded-full") + expect(badge_stream("data_health_count")).not_to include("rounded-full") end - it "renders an empty frame when a count fails" do + it "leaves only the failing badge empty when a count fails" do allow(DataHealth).to receive(:checks).and_raise(StandardError, "boom") - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_badge_counts expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="data_health_count")) - expect(response.body).not_to include("rounded-full") + expect(badge_stream("data_health_count")).not_to include("rounded-full") + expect(badge_stream("author_credit_count")).to match(%r{]*>5}) end it "denies a non-admin" do sign_in create(:user) - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_badge_counts expect(response).not_to have_http_status(:ok) end From abd70a72c74b83d138b29c9bdc8bc185da027277 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 11:41:03 -0400 Subject: [PATCH 3/8] Badge Data health and Author credit with a yes/no check A count has to visit every flagged row, and author credit builds all four sections in Ruby to get its total. Asking whether any row exists stops at the first one, which keeps the badge request short; the pages still show the counts. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/admin/home_controller.rb | 17 +++--- .../author_credit_divergence_query.rb | 49 ++++++++++++++--- app/services/data_health/check.rb | 3 ++ .../data_health/unconfigured_app_settings.rb | 2 + app/views/admin/home/_badge.html.erb | 3 ++ app/views/admin/home/_count_badge.html.erb | 4 +- app/views/admin/home/badge_counts.html.erb | 7 --- app/views/admin/home/badges.html.erb | 7 +++ app/views/admin/home/index.html.erb | 8 +-- config/routes.rb | 2 +- ...home-clicks-without-waiting-for-badges.yml | 9 ++-- spec/requests/admin/home_spec.rb | 49 +++++++++-------- .../author_credit_divergence_query_spec.rb | 54 +++++++++++++++++++ spec/services/data_health/checks_spec.rb | 8 +++ 14 files changed, 164 insertions(+), 58 deletions(-) create mode 100644 app/views/admin/home/_badge.html.erb delete mode 100644 app/views/admin/home/badge_counts.html.erb create mode 100644 app/views/admin/home/badges.html.erb diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index e0559180b9..34a7a8386b 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -2,7 +2,8 @@ module Admin class HomeController < ApplicationController include AdminCardsHelper - DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are counted separately on the Data health page.".freeze + RESOLVE_BADGE = "Resolve".freeze + DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index authorize! :home, to: :index? @@ -17,19 +18,19 @@ def index @pending_change_requests_count = ProfileChangeRequest.pending.count end - def badge_counts + def badges authorize! :home, to: :index? - @data_health_count = safe_count { DataHealth.checks.sum(&:count) } - @author_credit_count = safe_count { DataHealth::Worklists.new.author_credit.sum(&:count) } - @unsynced_features_count = safe_count { FeatureCatalog.new.missing_count } if allowed_to?(:create?, Feature) - render :badge_counts + @data_health_flagged = safely { DataHealth.checks.any?(&:flagged?) } + @author_credit_flagged = safely { AuthorCreditDivergenceQuery.new.any? } + @unsynced_features_count = safely { FeatureCatalog.new.missing_count } if allowed_to?(:create?, Feature) + render :badges end private - # A badge is a nicety, so a failing count leaves its badge empty and the button usable. - def safe_count + # A badge is a nicety, so a failing check leaves its badge empty and the button usable. + def safely yield rescue StandardError => e Rails.error.report(e) diff --git a/app/services/author_credit_divergence_query.rb b/app/services/author_credit_divergence_query.rb index 53ac4a8b71..18ac5d3052 100644 --- a/app/services/author_credit_divergence_query.rb +++ b/app/services/author_credit_divergence_query.rb @@ -67,6 +67,11 @@ def call ) end + # Whether any section has a row, stopping at the first one rather than building every section. + def any? + unauthored_any? || preference_any? + end + private attr_reader :person_id, :type, :preference, :include_reconciled @@ -91,16 +96,39 @@ def includes_for(model) includes end + def preference_scope(model) + scope = scoped(model).where.not(author_credit_preference: nil) + preference ? scope.where(author_credit_preference: preference) : scope + end + def preference_groups - records = models.flat_map do |model| - scope = scoped(model).where.not(author_credit_preference: nil) - scope = scope.where(author_credit_preference: preference) if preference - scope.select(&:author_credit_diverged?) - end + records = models.flat_map { |model| preference_scope(model).select(&:author_credit_diverged?) } group_by_person(records) end + def preference_any? + models.any? do |model| + preference_scope(model).find_each.any? do |record| + record.author_credit_diverged? && counted_person?(record.credit_governing_person) + end + end + end + + # Every author-less record lands in exactly one of legacy, creator, or unattributed. + def unauthored_any? + authorable_models.any? do |model| + scoped(model).where(author_id: nil).find_each.any? do |record| + creator = record.created_by&.person + if record.legacy_author_name_text.present? || creator.blank? + !(preference || person_id) + else + counted_person?(creator) + end + end + end + end + # No credited person here, which is the whole problem — so no person filter. def legacy_groups groups = authorable_models.flat_map do |model| @@ -171,9 +199,7 @@ def group_by_person(records, &grouper) end def build_group(person, records) - return nil if person.blank? - return nil if person_id.present? && person.id != person_id.to_i - return nil if person.author_credit_reconciled_at.present? && !include_reconciled + return nil unless counted_person?(person) PersonGroup.new( person: person, @@ -182,6 +208,13 @@ def build_group(person, records) ) end + def counted_person?(person) + return false if person.blank? + return false if person_id.present? && person.id != person_id.to_i + + person.author_credit_reconciled_at.blank? || include_reconciled + end + def sorted(records) records.sort_by { |record| [ record.class.name, record.id ] } end diff --git a/app/services/data_health/check.rb b/app/services/data_health/check.rb index e55e317e74..dfe0ebffac 100644 --- a/app/services/data_health/check.rb +++ b/app/services/data_health/check.rb @@ -25,6 +25,9 @@ def count def any? = count.positive? + # Stops at the first wrong row, for callers that only need to know whether there is one. + def flagged? = scope.exists? + def page(number) ordered_scope.paginate(page: number, per_page: PER_PAGE, total_entries: count) end diff --git a/app/services/data_health/unconfigured_app_settings.rb b/app/services/data_health/unconfigured_app_settings.rb index b229748c04..80a0ed39a3 100644 --- a/app/services/data_health/unconfigured_app_settings.rb +++ b/app/services/data_health/unconfigured_app_settings.rb @@ -20,6 +20,8 @@ def count @count ||= missing.any? ? 1 : 0 end + def flagged? = any? + def lists_rows? = false def scope = Setting.none diff --git a/app/views/admin/home/_badge.html.erb b/app/views/admin/home/_badge.html.erb new file mode 100644 index 0000000000..2ba299de7d --- /dev/null +++ b/app/views/admin/home/_badge.html.erb @@ -0,0 +1,3 @@ +<% if text.present? %> + <%= tag.span text, class: "bg-primary ms-1 rounded-full px-2 py-0.5 text-xs text-white ring-1 ring-white", title: local_assigns[:title] %> +<% end %> diff --git a/app/views/admin/home/_count_badge.html.erb b/app/views/admin/home/_count_badge.html.erb index 3b2b91cc56..ba2144ad3a 100644 --- a/app/views/admin/home/_count_badge.html.erb +++ b/app/views/admin/home/_count_badge.html.erb @@ -1,3 +1 @@ -<% if count.to_i.positive? %> - <%= tag.span number_with_delimiter(count), class: "bg-primary ms-1 rounded-full px-2 py-0.5 text-xs text-white ring-1 ring-white", title: local_assigns[:title] %> -<% end %> +<%= render "badge", text: (number_with_delimiter(count) if count.to_i.positive?), title: local_assigns[:title] %> diff --git a/app/views/admin/home/badge_counts.html.erb b/app/views/admin/home/badge_counts.html.erb deleted file mode 100644 index 8ffd28679a..0000000000 --- a/app/views/admin/home/badge_counts.html.erb +++ /dev/null @@ -1,7 +0,0 @@ -<%= turbo_frame_tag :badge_counts do %> - <%= turbo_stream.update "data_health_count", partial: "count_badge", locals: { count: @data_health_count, title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> - <%= turbo_stream.update "author_credit_count", partial: "count_badge", locals: { count: @author_credit_count } %> - <% if @unsynced_features_count %> - <%= turbo_stream.update "unsynced_features_count", partial: "count_badge", locals: { count: @unsynced_features_count } %> - <% end %> -<% end %> diff --git a/app/views/admin/home/badges.html.erb b/app/views/admin/home/badges.html.erb new file mode 100644 index 0000000000..17ed9993dc --- /dev/null +++ b/app/views/admin/home/badges.html.erb @@ -0,0 +1,7 @@ +<%= turbo_frame_tag :badges do %> + <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::RESOLVE_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> + <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::RESOLVE_BADGE if @author_credit_flagged) } %> + <% if @unsynced_features_count %> + <%= turbo_stream.update "unsynced_features_badge", partial: "count_badge", locals: { count: @unsynced_features_count } %> + <% end %> +<% end %> diff --git a/app/views/admin/home/index.html.erb b/app/views/admin/home/index.html.erb index 5bfa72201b..043192cab4 100644 --- a/app/views/admin/home/index.html.erb +++ b/app/views/admin/home/index.html.erb @@ -159,7 +159,7 @@ <% end %> <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> Data health - <%= tag.span id: "data_health_count" %> + <%= tag.span id: "data_health_badge" %> <% end %> <% if allowed_to?(:import?, EventRegistration) %> <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> @@ -168,18 +168,18 @@ <% end %> <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> Author credit divergences - <%= tag.span id: "author_credit_count" %> + <%= tag.span id: "author_credit_badge" %> <% end %> <% if allowed_to?(:create?, Feature) %> <%= button_to import_features_path, class: button_classes(:primary_outline), form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> Sync features & tips - <%= tag.span id: "unsynced_features_count" %> + <%= tag.span id: "unsynced_features_badge" %> <% end %> <% end %> - <%= turbo_frame_tag :badge_counts, src: admin_home_badge_counts_path %> + <%= turbo_frame_tag :badges, src: admin_home_badges_path %> diff --git a/config/routes.rb b/config/routes.rb index 29ecf53714..681ff67253 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,7 +56,7 @@ namespace :admin do get "/", to: "home#index" # admin home page - get "home/badge_counts", to: "home#badge_counts", as: "home_badge_counts" + get "home/badges", to: "home#badges", as: "home_badges" get "activities/events", to: "ahoy_activities#index", as: "activities_events" get "activities/events/:id", to: "ahoy_activities#show", as: "activities_event" get "activities/visits", to: "ahoy_activities#visits", as: "activities_visits" diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml index 84a04c23f0..7f6711a945 100644 --- a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -1,9 +1,10 @@ -name: "Admin home links work while the action counts load" +name: "Admin home links work while the action badges load" area: reporting display_status: admin_facing released_on: 2026-10-25 pr_number: 2847 summary: >- - The counts on the admin home's Data health, Author credit divergences, and - Sync features buttons now load together in the background, so clicking - through to another page no longer waits for them to finish. + The badges on the admin home's action buttons now load together in the + background, so clicking through to another page no longer waits for them. + Data health and Author credit divergences show "Resolve" when something needs + attention; open the page for the full count. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index 09f81def13..aa74b7abd3 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -82,17 +82,17 @@ expect(response.body).to include(%(href="#{organization_statuses_path}")) end - it "loads every action count in one frame request so the badges hold a single server thread" do + it "loads every action badge in one frame request so the badges hold a single server thread" do get "/admin" expect(response.body.scan(/]* src=/).size).to eq(1) - expect(response.body).to include(%(src="#{admin_home_badge_counts_path}")) - expect(response.body).to include(%(id="data_health_count"), %(id="author_credit_count"), %(id="unsynced_features_count")) + expect(response.body).to include(%(src="#{admin_home_badges_path}")) + expect(response.body).to include(%(id="data_health_badge"), %(id="author_credit_badge"), %(id="unsynced_features_badge")) end - describe "badge counts" do - def get_badge_counts - get admin_home_badge_counts_path, headers: { "Turbo-Frame" => "badge_counts" } + describe "badges" do + def get_badges + get admin_home_badges_path, headers: { "Turbo-Frame" => "badges" } end def badge_stream(target) @@ -100,44 +100,47 @@ def badge_stream(target) end before do - allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 2), instance_double(DataHealth::Check, count: 3) ]) - entries = [ 1, 4 ].map { |count| DataHealth::Worklists::Entry.new("Label", count, "record", "/") } - allow_any_instance_of(DataHealth::Worklists).to receive(:author_credit).and_return(entries) + allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false), instance_double(DataHealth::Check, flagged?: true) ]) + allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(true) allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(3) end - it "updates each badge from inside the frame" do - get_badge_counts + it "asks Resolve on Data health and Author credit, and counts unsynced features" do + get_badges expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="badge_counts")) - expect(badge_stream("data_health_count")).to match(%r{]*>5}).and include("Data checks only") - expect(badge_stream("author_credit_count")).to match(%r{]*>5}) - expect(badge_stream("unsynced_features_count")).to match(%r{]*>3}) + expect(response.body).to include(%(id="badges")) + expect(badge_stream("data_health_badge")).to match(%r{]*>Resolve}).and include("Data checks only") + expect(badge_stream("author_credit_badge")).to match(%r{]*>Resolve}) + expect(badge_stream("unsynced_features_badge")).to match(%r{]*>3}) end it "renders no badge when nothing is waiting" do - allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 0) ]) + allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false) ]) + allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(false) + allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(0) - get_badge_counts + get_badges - expect(badge_stream("data_health_count")).not_to include("rounded-full") + expect(badge_stream("data_health_badge")).not_to include("rounded-full") + expect(badge_stream("author_credit_badge")).not_to include("rounded-full") + expect(badge_stream("unsynced_features_badge")).not_to include("rounded-full") end - it "leaves only the failing badge empty when a count fails" do + it "leaves only the failing badge empty when a check fails" do allow(DataHealth).to receive(:checks).and_raise(StandardError, "boom") - get_badge_counts + get_badges expect(response).to have_http_status(:ok) - expect(badge_stream("data_health_count")).not_to include("rounded-full") - expect(badge_stream("author_credit_count")).to match(%r{]*>5}) + expect(badge_stream("data_health_badge")).not_to include("rounded-full") + expect(badge_stream("author_credit_badge")).to match(%r{]*>Resolve}) end it "denies a non-admin" do sign_in create(:user) - get_badge_counts + get_badges expect(response).not_to have_http_status(:ok) end diff --git a/spec/services/author_credit_divergence_query_spec.rb b/spec/services/author_credit_divergence_query_spec.rb index e362488b54..a3e94bca6b 100644 --- a/spec/services/author_credit_divergence_query_spec.rb +++ b/spec/services/author_credit_divergence_query_spec.rb @@ -69,6 +69,60 @@ def diverged_story end end + describe "#any?" do + let(:personless_user) { create(:user, person: nil) } + + def expect_any_to_match(query) + expect(query.any?).to eq(!query.call.empty?) + end + + it "is false when every record credits its author cleanly" do + person.update!(display_name_preference: "full_name") + create(:story, created_by: author_user, author: person, author_credit_preference: nil) + + expect(described_class.new(type: "Story").any?).to be(false) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "is true for a diverged snapshot" do + diverged_story + + expect(described_class.new.any?).to be(true) + expect_any_to_match(described_class.new) + end + + it "is true for a legacy name, a creator fallback, and an unattributed record" do + [ + -> { create(:workshop, created_by: author_user, author: nil, full_name: "Legacy Name") }, + -> { create(:story, created_by: author_user, author: nil) }, + -> { create(:story, created_by: personless_user, author: nil) } + ].each do |make| + record = make.call + + expect(described_class.new.any?).to be(true) + expect_any_to_match(described_class.new) + record.destroy! + end + end + + it "is false when the only creator fallback belongs to a reconciled person" do + create(:story, created_by: author_user, author: nil) + person.update!(author_credit_reconciled_at: Time.current) + + expect(described_class.new(type: "Story").any?).to be(false) + expect(described_class.new(type: "Story", include_reconciled: "1").any?).to be(true) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "honors the person filter" do + create(:story, created_by: personless_user, author: nil) + diverged_story + + expect(described_class.new(person_id: person.id + 9999).any?).to be(false) + expect(described_class.new(person_id: person.id).any?).to be(true) + end + end + describe "filters" do before { diverged_story } diff --git a/spec/services/data_health/checks_spec.rb b/spec/services/data_health/checks_spec.rb index b6c404335d..c5f4627fd3 100644 --- a/spec/services/data_health/checks_spec.rb +++ b/spec/services/data_health/checks_spec.rb @@ -25,6 +25,14 @@ end end + it "flags exactly the checks that count rows" do + create(:story) + + described_class.checks.each do |check| + expect(check.flagged?).to eq(check.any?), "#{check.key} flags #{check.flagged?} but counts #{check.count}" + end + end + it "keeps every check's scope a relation, so counting doesn't load the table" do described_class.checks.each do |check| expect(check.scope).to be_a(ActiveRecord::Relation), "#{check.key} returned #{check.scope.class}" From ffeebd8a37b0fc97eb9b6f25bf3d3ea4ee8ab4f7 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 11:48:51 -0400 Subject: [PATCH 4/8] Label the yes/no badges Review Flagged items sometimes only need a look, not a fix, so Review fits better than Resolve. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/admin/home_controller.rb | 2 +- app/views/admin/home/badges.html.erb | 4 ++-- ...10-25-admin-home-clicks-without-waiting-for-badges.yml | 2 +- spec/requests/admin/home_spec.rb | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index 34a7a8386b..4dbf453360 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -2,7 +2,7 @@ module Admin class HomeController < ApplicationController include AdminCardsHelper - RESOLVE_BADGE = "Resolve".freeze + REVIEW_BADGE = "Review".freeze DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index diff --git a/app/views/admin/home/badges.html.erb b/app/views/admin/home/badges.html.erb index 17ed9993dc..e448131a8c 100644 --- a/app/views/admin/home/badges.html.erb +++ b/app/views/admin/home/badges.html.erb @@ -1,6 +1,6 @@ <%= turbo_frame_tag :badges do %> - <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::RESOLVE_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> - <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::RESOLVE_BADGE if @author_credit_flagged) } %> + <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> + <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @author_credit_flagged) } %> <% if @unsynced_features_count %> <%= turbo_stream.update "unsynced_features_badge", partial: "count_badge", locals: { count: @unsynced_features_count } %> <% end %> diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml index 7f6711a945..c43435a4c1 100644 --- a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -6,5 +6,5 @@ pr_number: 2847 summary: >- The badges on the admin home's action buttons now load together in the background, so clicking through to another page no longer waits for them. - Data health and Author credit divergences show "Resolve" when something needs + Data health and Author credit divergences show "Review" when something needs attention; open the page for the full count. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index aa74b7abd3..e9ed406fc9 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -105,13 +105,13 @@ def badge_stream(target) allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(3) end - it "asks Resolve on Data health and Author credit, and counts unsynced features" do + it "shows Review on Data health and Author credit, and counts unsynced features" do get_badges expect(response).to have_http_status(:ok) expect(response.body).to include(%(id="badges")) - expect(badge_stream("data_health_badge")).to match(%r{]*>Resolve}).and include("Data checks only") - expect(badge_stream("author_credit_badge")).to match(%r{]*>Resolve}) + expect(badge_stream("data_health_badge")).to match(%r{]*>Review}).and include("Data checks only") + expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) expect(badge_stream("unsynced_features_badge")).to match(%r{]*>3}) end @@ -134,7 +134,7 @@ def badge_stream(target) expect(response).to have_http_status(:ok) expect(badge_stream("data_health_badge")).not_to include("rounded-full") - expect(badge_stream("author_credit_badge")).to match(%r{]*>Resolve}) + expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) end it "denies a non-admin" do From a515e6f099743cebebd723016a33e4368bf8b4c7 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 12:05:26 -0400 Subject: [PATCH 5/8] Answer the author credit badge in SQL Proving the worklist is empty meant loading every author-less record into Ruby. The same rules as SQL exists? queries answer without loading a record, so the badge stays cheap after every item is reviewed. A spec holds any? to the full query under every filter so the two can't drift. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../author_credit_divergence_query.rb | 81 +++++++++++-------- .../author_credit_divergence_query_spec.rb | 38 +++++++++ 2 files changed, 86 insertions(+), 33 deletions(-) diff --git a/app/services/author_credit_divergence_query.rb b/app/services/author_credit_divergence_query.rb index 18ac5d3052..a0e0188d41 100644 --- a/app/services/author_credit_divergence_query.rb +++ b/app/services/author_credit_divergence_query.rb @@ -67,9 +67,9 @@ def call ) end - # Whether any section has a row, stopping at the first one rather than building every section. + # Whether any section has a row, in SQL so even an empty worklist never loads a record. def any? - unauthored_any? || preference_any? + authorable_models.any? { |model| unauthored_divergences(model).exists? || preference_divergences(model).exists? } end private @@ -96,37 +96,57 @@ def includes_for(model) includes end - def preference_scope(model) - scope = scoped(model).where.not(author_credit_preference: nil) - preference ? scope.where(author_credit_preference: preference) : scope - end - def preference_groups - records = models.flat_map { |model| preference_scope(model).select(&:author_credit_diverged?) } + records = models.flat_map do |model| + scope = scoped(model).where.not(author_credit_preference: nil) + scope = scope.where(author_credit_preference: preference) if preference + scope.select(&:author_credit_diverged?) + end group_by_person(records) end - def preference_any? - models.any? do |model| - preference_scope(model).find_each.any? do |record| - record.author_credit_diverged? && counted_person?(record.credit_governing_person) - end + # SQL twin of AuthorCreditable#author_credit_diverged? plus build_group's person rules. + def preference_divergences(model) + table = model.table_name + scope = model.where.not(author_credit_preference: nil) + .joins("INNER JOIN #{Person.table_name} authors ON authors.id = #{table}.author_id") + .where("TRIM(#{table}.author_credit_preference) <> ''") + .where("#{table}.author_credit_preference <> CASE WHEN authors.anonymous_contributions THEN 'anonymous' " \ + "ELSE COALESCE(NULLIF(TRIM(authors.display_name_preference), ''), 'full_name') END") + scope = scope.where(author_credit_preference: preference) if preference + counted_people(scope, :authors) + end + + # Every author-less record lands in exactly one of legacy, creator, or unattributed; + # only the creator section survives a preference or person filter. + def unauthored_divergences(model) + scope = model.where(author_id: nil).joins( + "LEFT JOIN #{User.table_name} creators ON creators.id = #{model.table_name}.created_by_id " \ + "LEFT JOIN #{Person.table_name} creator_people ON creator_people.id = creators.person_id" + ) + legacy = legacy_name_present(model) + creator_fallback = counted_people(scope, :creator_people) + if preference || person_id + return legacy ? creator_fallback.where.not(legacy) : creator_fallback end + + unattributed_or_creator = scope.where(creator_people: { id: nil }).or(creator_fallback) + legacy ? unattributed_or_creator.or(scope.where(legacy)) : unattributed_or_creator end - # Every author-less record lands in exactly one of legacy, creator, or unattributed. - def unauthored_any? - authorable_models.any? do |model| - scoped(model).where(author_id: nil).find_each.any? do |record| - creator = record.created_by&.person - if record.legacy_author_name_text.present? || creator.blank? - !(preference || person_id) - else - counted_person?(creator) - end - end - end + def legacy_name_present(model) + model.legacy_author_name_columns.map do |qualified_column| + table, column = qualified_column.split(".") + Arel::Nodes::NamedFunction.new("COALESCE", [ Arel::Table.new(table)[column], Arel::Nodes.build_quoted("") ]) + .matches_regexp("[^[:space:]]") + end.reduce(:or) + end + + def counted_people(scope, person_alias) + scope = scope.where.not(person_alias => { id: nil }) + scope = scope.where(person_alias => { id: person_id.to_i }) if person_id.present? + include_reconciled ? scope : scope.where(person_alias => { author_credit_reconciled_at: nil }) end # No credited person here, which is the whole problem — so no person filter. @@ -199,7 +219,9 @@ def group_by_person(records, &grouper) end def build_group(person, records) - return nil unless counted_person?(person) + return nil if person.blank? + return nil if person_id.present? && person.id != person_id.to_i + return nil if person.author_credit_reconciled_at.present? && !include_reconciled PersonGroup.new( person: person, @@ -208,13 +230,6 @@ def build_group(person, records) ) end - def counted_person?(person) - return false if person.blank? - return false if person_id.present? && person.id != person_id.to_i - - person.author_credit_reconciled_at.blank? || include_reconciled - end - def sorted(records) records.sort_by { |record| [ record.class.name, record.id ] } end diff --git a/spec/services/author_credit_divergence_query_spec.rb b/spec/services/author_credit_divergence_query_spec.rb index a3e94bca6b..e8ff047aed 100644 --- a/spec/services/author_credit_divergence_query_spec.rb +++ b/spec/services/author_credit_divergence_query_spec.rb @@ -114,6 +114,44 @@ def expect_any_to_match(query) expect_any_to_match(described_class.new(type: "Story")) end + it "treats a whitespace-only legacy name as no legacy name" do + create(:workshop, created_by: author_user, author: nil, full_name: " ") + person.update!(author_credit_reconciled_at: Time.current) + + expect(described_class.new(type: "Workshop").any?).to be(false) + expect_any_to_match(described_class.new(type: "Workshop")) + end + + it "compares a snapshot against an anonymous or blank-preference author" do + story = create(:story, created_by: author_user, author: person) + person.update_columns(anonymous_contributions: true) + story.update_columns(author_credit_preference: "anonymous") + expect(described_class.new(type: "Story").any?).to be(false) + + story.update_columns(author_credit_preference: "full_name") + expect(described_class.new(type: "Story").any?).to be(true) + + person.update_columns(anonymous_contributions: false, display_name_preference: "") + expect(described_class.new(type: "Story").any?).to be(false) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "agrees with the full query under every filter" do + diverged_story + create(:story, created_by: personless_user, author: nil) + create(:workshop, created_by: personless_user, author: nil, full_name: "Legacy Name") + reconciled = create(:user, :with_person) + reconciled.person.update!(author_credit_reconciled_at: Time.current) + create(:story, created_by: reconciled, author: nil) + + [ {}, { type: "Story" }, { type: "Resource" }, { person_id: person.id }, { person_id: reconciled.person.id }, + { person_id: reconciled.person.id, include_reconciled: "1" }, { preference: "first_name_only" }, + { preference: "anonymous" }, { include_reconciled: "1" } ].each do |filters| + query = described_class.new(**filters) + expect(query.any?).to eq(!query.call.empty?), "disagrees for #{filters}" + end + end + it "honors the person filter" do create(:story, created_by: personless_user, author: nil) diverged_story From 6bd9071c215169a6361c23922cfc36582ccb44cd Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 12:16:58 -0400 Subject: [PATCH 6/8] Flag unsynced features from the newest 20 seeds Counting unsynced features read all 520 seed files on every admin home visit. New seeds land at the end of the date-sorted folder, so the newest 20 tell us whether a sync is due; the badge says New instead of a count it can no longer give honestly. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/admin/home_controller.rb | 3 ++- app/services/feature_catalog.rb | 16 +++++++----- app/views/admin/home/badges.html.erb | 4 +-- ...home-clicks-without-waiting-for-badges.yml | 8 +++--- spec/requests/admin/home_spec.rb | 8 +++--- spec/services/feature_catalog_spec.rb | 25 +++++++++++++------ 6 files changed, 38 insertions(+), 26 deletions(-) diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index 4dbf453360..1f6f56ceb9 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -3,6 +3,7 @@ class HomeController < ApplicationController include AdminCardsHelper REVIEW_BADGE = "Review".freeze + NEW_BADGE = "New".freeze DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index @@ -23,7 +24,7 @@ def badges @data_health_flagged = safely { DataHealth.checks.any?(&:flagged?) } @author_credit_flagged = safely { AuthorCreditDivergenceQuery.new.any? } - @unsynced_features_count = safely { FeatureCatalog.new.missing_count } if allowed_to?(:create?, Feature) + @features_unsynced = safely { FeatureCatalog.new.recent_missing? } if allowed_to?(:create?, Feature) render :badges end diff --git a/app/services/feature_catalog.rb b/app/services/feature_catalog.rb index a5f9581a8f..82dff2ab3c 100644 --- a/app/services/feature_catalog.rb +++ b/app/services/feature_catalog.rb @@ -6,6 +6,7 @@ # left entirely alone. class FeatureCatalog DATA_DIR = Rails.root.join("db/seeds/features") + RECENT_SEEDS = 20 SYNCED_FIELDS = %i[ area display_status released_on action_path pr_number @@ -49,20 +50,23 @@ def import! Result.new(created, updated, skipped) end - def missing_count - names = entries.map { |entry| entry.fetch("name") } + # Reads only the newest seeds, so a backdated seed merged after a sync waits for a newer one to flag it. + def recent_missing?(limit: RECENT_SEEDS) + names = seed_paths.last(limit).map { |path| load_seed(path).fetch("name") } existing = Feature.where(name: names).pluck(:name).to_set(&:downcase) - names.count { |name| existing.exclude?(name.downcase) } + names.any? { |name| existing.exclude?(name.downcase) } end def entries - Dir[File.join(@dir, "*.yml")].sort.map do |path| - YAML.safe_load_file(path, permitted_classes: [ Date ]) - end + seed_paths.map { |path| load_seed(path) } end private + def seed_paths = Dir[File.join(@dir, "*.yml")].sort + + def load_seed(path) = YAML.safe_load_file(path, permitted_classes: [ Date ]) + # Keyed by downcased name to match the case-insensitive column collation. def existing_features(names) Feature.with_rich_text_rhino_description.where(name: names).index_by { |feature| feature.name.downcase } diff --git a/app/views/admin/home/badges.html.erb b/app/views/admin/home/badges.html.erb index e448131a8c..0cb735f136 100644 --- a/app/views/admin/home/badges.html.erb +++ b/app/views/admin/home/badges.html.erb @@ -1,7 +1,5 @@ <%= turbo_frame_tag :badges do %> <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @author_credit_flagged) } %> - <% if @unsynced_features_count %> - <%= turbo_stream.update "unsynced_features_badge", partial: "count_badge", locals: { count: @unsynced_features_count } %> - <% end %> + <%= turbo_stream.update "unsynced_features_badge", partial: "badge", locals: { text: (Admin::HomeController::NEW_BADGE if @features_unsynced) } %> <% end %> diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml index c43435a4c1..d231ec58ba 100644 --- a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -4,7 +4,7 @@ display_status: admin_facing released_on: 2026-10-25 pr_number: 2847 summary: >- - The badges on the admin home's action buttons now load together in the - background, so clicking through to another page no longer waits for them. - Data health and Author credit divergences show "Review" when something needs - attention; open the page for the full count. + Admin home's action badges now load together in the background, so clicks + no longer wait for them. Data health and Author credit show "Review" when + something needs attention; Sync features & tips shows "New" when there are + features to sync. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index e9ed406fc9..bfb77aee42 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -102,23 +102,23 @@ def badge_stream(target) before do allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false), instance_double(DataHealth::Check, flagged?: true) ]) allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(true) - allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(3) + allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(true) end - it "shows Review on Data health and Author credit, and counts unsynced features" do + it "shows Review on Data health and Author credit, and New on Sync features" do get_badges expect(response).to have_http_status(:ok) expect(response.body).to include(%(id="badges")) expect(badge_stream("data_health_badge")).to match(%r{]*>Review}).and include("Data checks only") expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) - expect(badge_stream("unsynced_features_badge")).to match(%r{]*>3}) + expect(badge_stream("unsynced_features_badge")).to match(%r{]*>New}) end it "renders no badge when nothing is waiting" do allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false) ]) allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(false) - allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(0) + allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(false) get_badges diff --git a/spec/services/feature_catalog_spec.rb b/spec/services/feature_catalog_spec.rb index ead803231d..5537bac6c9 100644 --- a/spec/services/feature_catalog_spec.rb +++ b/spec/services/feature_catalog_spec.rb @@ -143,17 +143,17 @@ end end - describe "#missing_count" do + describe "#recent_missing?" do let(:seed_dir) { Pathname(Dir.mktmpdir("features_seed")) } before do - %w[One Two].each do |label| - seed_dir.join("2026-08-01-seed-#{label.downcase}.yml").write(<<~YAML) + %w[One Two Three].each_with_index do |label, index| + seed_dir.join("2026-08-0#{index + 1}-seed-#{label.downcase}.yml").write(<<~YAML) name: "Seed feature #{label}" area: events display_status: user_facing summary: "Seeded." - released_on: 2026-08-01 + released_on: 2026-08-0#{index + 1} YAML end end @@ -162,16 +162,25 @@ subject(:catalog) { described_class.new(dir: seed_dir) } - it "counts seeds with no feature of that name yet, ignoring case" do + it "is true while a recent seed has no feature of that name yet, ignoring case" do create(:feature, name: "seed feature one") + create(:feature, name: "seed feature two") - expect(catalog.missing_count).to eq(1) + expect(catalog.recent_missing?).to be(true) end - it "is zero once everything is synced" do + it "is false once everything is synced" do catalog.import! - expect(catalog.missing_count).to eq(0) + expect(catalog.recent_missing?).to be(false) + end + + it "looks only at the newest seeds" do + create(:feature, name: "Seed feature Two") + create(:feature, name: "Seed feature Three") + + expect(catalog.recent_missing?(limit: 2)).to be(false) + expect(catalog.recent_missing?(limit: 3)).to be(true) end end From 67585b653e6198e2f3bd0d8c9d8d48fe5079c124 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 25 Oct 2026 12:19:39 -0400 Subject: [PATCH 7/8] Label the Sync features badge Review too One word across the three action badges reads as one signal: open it, there's something to look at. Co-Authored-By: Claude Opus 5.5 (1M context) --- app/controllers/admin/home_controller.rb | 1 - app/views/admin/home/badges.html.erb | 2 +- ...26-10-25-admin-home-clicks-without-waiting-for-badges.yml | 5 ++--- spec/requests/admin/home_spec.rb | 4 ++-- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index 1f6f56ceb9..ab63aa8e1c 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -3,7 +3,6 @@ class HomeController < ApplicationController include AdminCardsHelper REVIEW_BADGE = "Review".freeze - NEW_BADGE = "New".freeze DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index diff --git a/app/views/admin/home/badges.html.erb b/app/views/admin/home/badges.html.erb index 0cb735f136..98fb190eb9 100644 --- a/app/views/admin/home/badges.html.erb +++ b/app/views/admin/home/badges.html.erb @@ -1,5 +1,5 @@ <%= turbo_frame_tag :badges do %> <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @author_credit_flagged) } %> - <%= turbo_stream.update "unsynced_features_badge", partial: "badge", locals: { text: (Admin::HomeController::NEW_BADGE if @features_unsynced) } %> + <%= turbo_stream.update "unsynced_features_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @features_unsynced) } %> <% end %> diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml index d231ec58ba..b025953609 100644 --- a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -5,6 +5,5 @@ released_on: 2026-10-25 pr_number: 2847 summary: >- Admin home's action badges now load together in the background, so clicks - no longer wait for them. Data health and Author credit show "Review" when - something needs attention; Sync features & tips shows "New" when there are - features to sync. + no longer wait for them. Data health, Author credit, and Sync features & tips + show "Review" when something needs attention. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index bfb77aee42..e9ad5e5ae4 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -105,14 +105,14 @@ def badge_stream(target) allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(true) end - it "shows Review on Data health and Author credit, and New on Sync features" do + it "shows Review on Data health, Author credit, and Sync features" do get_badges expect(response).to have_http_status(:ok) expect(response.body).to include(%(id="badges")) expect(badge_stream("data_health_badge")).to match(%r{]*>Review}).and include("Data checks only") expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) - expect(badge_stream("unsynced_features_badge")).to match(%r{]*>New}) + expect(badge_stream("unsynced_features_badge")).to match(%r{]*>Review}) end it "renders no badge when nothing is waiting" do From b99acae46941be3f1af890da4c46118cbeb81db4 Mon Sep 17 00:00:00 2001 From: maebeale Date: Sun, 25 Oct 2026 12:58:30 -0400 Subject: [PATCH 8/8] Admin home: one plain lazy frame for action buttons; document Turbo rules (#2855) * Render admin home action buttons in one plain lazy frame The badges frame answered with turbo-stream tags tucked inside a frame response, mixing the two mechanisms. Wrapping the button row in a single target="_top" frame keeps one request for every badge while staying a pure frame swap, and moves the pending change-request count out of the full-page render so the skeleton does none of the frame's work. Co-Authored-By: Claude Opus 5.5 (1M context) * Document Justin's Turbo review rules in the AI files Reviews keep flagging the same Turbo patterns (skeleton double queries, several lazy actions per page, streams inside frame responses, inline controller streams, turbo: false workarounds), and none were written down, so agents kept reintroducing them. Co-Authored-By: Claude Opus 5.5 (1M context) * Keep reviewer names out of the Turbo rules intro Co-Authored-By: Claude Opus 5.5 (1M context) --------- Co-authored-by: Claude Opus 5.5 (1M context) --- .github/copilot-instructions.md | 24 +++++++++ CLAUDE.md | 24 +++++++++ app/controllers/admin/home_controller.rb | 9 ++-- app/views/admin/home/_action_buttons.html.erb | 27 ++++++++++ app/views/admin/home/action_buttons.html.erb | 7 +++ app/views/admin/home/badges.html.erb | 5 -- app/views/admin/home/index.html.erb | 31 ++--------- config/routes.rb | 2 +- spec/requests/admin/home_spec.rb | 52 +++++++++++-------- 9 files changed, 119 insertions(+), 62 deletions(-) create mode 100644 app/views/admin/home/_action_buttons.html.erb create mode 100644 app/views/admin/home/action_buttons.html.erb delete mode 100644 app/views/admin/home/badges.html.erb diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 49c20f1655..bb22b6c9c0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -300,6 +300,9 @@ follow this). Match the existing pattern: - The controller `index` branches on `turbo_frame_request?`: the frame request builds the filtered/paginated scope and renders the results view; otherwise it renders the full `index`. +- **The skeleton request runs none of the frame's queries.** Build scopes, counts, + and totals only inside the `turbo_frame_request?` branch; a `@total = scope.count` + above the branch runs on both requests, so the page queries twice. - **Name the frame-response view after its Turbo frame tag id** — the `turbo_frame_tag` id, the controller render target, and the view filename are one shared `_results` token (e.g. `turbo_frame_tag :grants_results` ← @@ -347,6 +350,27 @@ follow this). Match the existing pattern: `data-turbo-permanent` (plus an `id`) on any TomSelect wrapper so the morph doesn't strip its widget. Workshop logs is the reference. +## Turbo frames & streams + +Rules from past PR reviews: + +- **One lazy request per page.** When several parts of a page load lazily (badges, + counts, panels), load them from one action, not one frame per part — each lazy + request holds a Puma thread, so the user's next click queues behind them. Wrap + the parts in one frame (admin home `action_buttons` is the reference). +- **Keep a frame response a pure frame swap — no `` tags inside it.** + If a frame's response needs to update elements outside the frame, widen the + frame to contain them. Some older results views still tuck streams into their + frame; don't copy them. +- **Answer stream requests with a `*.turbo_stream.erb` template** via + `respond_to { |format| format.turbo_stream }`, not inline `render turbo_stream:` + in the controller — the template keeps markup in views and out of actions. +- **Don't set `turbo: false` to make something work — find the root cause.** + Links that leave a frame use `data: { turbo_frame: "_top" }` (or a `target: "_top"` + frame); keep `turbo: false` for downloads, new tabs, and pages that truly need a + full load. Turbo already disables a form's submitter while it submits, so a Turbo + form needs no double-submit guard (`submit_once` is for forms that stay non-Turbo). + ## Features & tips page (`/features`) The login-gated **Features & tips** page lists shipped, user-facing features diff --git a/CLAUDE.md b/CLAUDE.md index 726450a05b..ee0e35df00 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -310,6 +310,9 @@ follow this). Match the existing pattern: - The controller `index` branches on `turbo_frame_request?`: the frame request builds the filtered/paginated scope and renders the results view; otherwise it renders the full `index`. +- **The skeleton request runs none of the frame's queries.** Build scopes, counts, + and totals only inside the `turbo_frame_request?` branch; a `@total = scope.count` + above the branch runs on both requests, so the page queries twice. - **Name the frame-response view after its Turbo frame tag id** — the `turbo_frame_tag` id, the controller render target, and the view filename are one shared `_results` token (e.g. `turbo_frame_tag :grants_results` ← @@ -357,6 +360,27 @@ follow this). Match the existing pattern: `data-turbo-permanent` (plus an `id`) on any TomSelect wrapper so the morph doesn't strip its widget. Workshop logs is the reference. +## Turbo frames & streams + +Rules from past PR reviews: + +- **One lazy request per page.** When several parts of a page load lazily (badges, + counts, panels), load them from one action, not one frame per part — each lazy + request holds a Puma thread, so the user's next click queues behind them. Wrap + the parts in one frame (admin home `action_buttons` is the reference). +- **Keep a frame response a pure frame swap — no `` tags inside it.** + If a frame's response needs to update elements outside the frame, widen the + frame to contain them. Some older results views still tuck streams into their + frame; don't copy them. +- **Answer stream requests with a `*.turbo_stream.erb` template** via + `respond_to { |format| format.turbo_stream }`, not inline `render turbo_stream:` + in the controller — the template keeps markup in views and out of actions. +- **Don't set `turbo: false` to make something work — find the root cause.** + Links that leave a frame use `data: { turbo_frame: "_top" }` (or a `target: "_top"` + frame); keep `turbo: false` for downloads, new tabs, and pages that truly need a + full load. Turbo already disables a form's submitter while it submits, so a Turbo + form needs no double-submit guard (`submit_once` is for forms that stay non-Turbo). + ## Features & tips page (`/features`) The login-gated **Features & tips** page lists shipped, user-facing features diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index ab63aa8e1c..f08f29c5cb 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -6,7 +6,7 @@ class HomeController < ApplicationController DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index - authorize! :home, to: :index? + authorize! :home @system_cards = system_cards @user_content_cards = user_content_cards @@ -15,21 +15,20 @@ def index @deduper_cards = deduper_cards @system_settings_cards = system_settings_cards @deprecated_data_cards = deprecated_data_cards - @pending_change_requests_count = ProfileChangeRequest.pending.count end - def badges + def action_buttons authorize! :home, to: :index? + @pending_change_requests_count = safely { ProfileChangeRequest.pending.count } @data_health_flagged = safely { DataHealth.checks.any?(&:flagged?) } @author_credit_flagged = safely { AuthorCreditDivergenceQuery.new.any? } @features_unsynced = safely { FeatureCatalog.new.recent_missing? } if allowed_to?(:create?, Feature) - render :badges end private - # A badge is a nicety, so a failing check leaves its badge empty and the button usable. + # A badge is a nicety, so a failing check leaves its badge empty and the buttons usable. def safely yield rescue StandardError => e diff --git a/app/views/admin/home/_action_buttons.html.erb b/app/views/admin/home/_action_buttons.html.erb new file mode 100644 index 0000000000..fc1cc4ef73 --- /dev/null +++ b/app/views/admin/home/_action_buttons.html.erb @@ -0,0 +1,27 @@ +
+ <%= link_to profile_change_requests_path, class: button_classes(:primary_outline) do %> + Changes requested + <%= render "count_badge", count: local_assigns[:pending_change_requests_count] %> + <% end %> + <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> + Data health + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:data_health_flagged]), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE %> + <% end %> + <% if allowed_to?(:import?, EventRegistration) %> + <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> + Import registrants + <% end %> + <% end %> + <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> + Author credit divergences + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:author_credit_flagged]) %> + <% end %> + <% if allowed_to?(:create?, Feature) %> + <%= button_to import_features_path, + class: button_classes(:primary_outline), + form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> + Sync features & tips + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:features_unsynced]) %> + <% end %> + <% end %> +
diff --git a/app/views/admin/home/action_buttons.html.erb b/app/views/admin/home/action_buttons.html.erb new file mode 100644 index 0000000000..972c14abd5 --- /dev/null +++ b/app/views/admin/home/action_buttons.html.erb @@ -0,0 +1,7 @@ +<%= turbo_frame_tag :action_buttons, target: "_top" do %> + <%= render "action_buttons", + pending_change_requests_count: @pending_change_requests_count, + data_health_flagged: @data_health_flagged, + author_credit_flagged: @author_credit_flagged, + features_unsynced: @features_unsynced %> +<% end %> diff --git a/app/views/admin/home/badges.html.erb b/app/views/admin/home/badges.html.erb deleted file mode 100644 index 98fb190eb9..0000000000 --- a/app/views/admin/home/badges.html.erb +++ /dev/null @@ -1,5 +0,0 @@ -<%= turbo_frame_tag :badges do %> - <%= turbo_stream.update "data_health_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @data_health_flagged), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE } %> - <%= turbo_stream.update "author_credit_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @author_credit_flagged) } %> - <%= turbo_stream.update "unsynced_features_badge", partial: "badge", locals: { text: (Admin::HomeController::REVIEW_BADGE if @features_unsynced) } %> -<% end %> diff --git a/app/views/admin/home/index.html.erb b/app/views/admin/home/index.html.erb index 043192cab4..530c00881c 100644 --- a/app/views/admin/home/index.html.erb +++ b/app/views/admin/home/index.html.erb @@ -152,34 +152,9 @@
Actions -
- <%= link_to profile_change_requests_path, class: button_classes(:primary_outline) do %> - Changes requested - <%= render "count_badge", count: @pending_change_requests_count %> - <% end %> - <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> - Data health - <%= tag.span id: "data_health_badge" %> - <% end %> - <% if allowed_to?(:import?, EventRegistration) %> - <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> - Import registrants - <% end %> - <% end %> - <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> - Author credit divergences - <%= tag.span id: "author_credit_badge" %> - <% end %> - <% if allowed_to?(:create?, Feature) %> - <%= button_to import_features_path, - class: button_classes(:primary_outline), - form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> - Sync features & tips - <%= tag.span id: "unsynced_features_badge" %> - <% end %> - <% end %> -
- <%= turbo_frame_tag :badges, src: admin_home_badges_path %> + <%= turbo_frame_tag :action_buttons, src: admin_home_action_buttons_path, target: "_top" do %> + <%= render "action_buttons" %> + <% end %>
diff --git a/config/routes.rb b/config/routes.rb index 681ff67253..5fd77b9a13 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,7 +56,7 @@ namespace :admin do get "/", to: "home#index" # admin home page - get "home/badges", to: "home#badges", as: "home_badges" + get "home/action_buttons", to: "home#action_buttons", as: "home_action_buttons" get "activities/events", to: "ahoy_activities#index", as: "activities_events" get "activities/events/:id", to: "ahoy_activities#show", as: "activities_event" get "activities/visits", to: "ahoy_activities#visits", as: "activities_visits" diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index e9ad5e5ae4..0295a5be96 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -82,21 +82,24 @@ expect(response.body).to include(%(href="#{organization_statuses_path}")) end - it "loads every action badge in one frame request so the badges hold a single server thread" do + it "renders the action buttons right away and fills their badges from one frame request" do + create(:profile_change_request) + get "/admin" expect(response.body.scan(/]* src=/).size).to eq(1) - expect(response.body).to include(%(src="#{admin_home_badges_path}")) - expect(response.body).to include(%(id="data_health_badge"), %(id="author_credit_badge"), %(id="unsynced_features_badge")) + expect(response.body).to include(%(src="#{admin_home_action_buttons_path}")) + expect(response.body).to include(%(href="#{admin_data_health_path}"), %(href="#{author_credit_divergences_path}")) + expect(response.body[%r{}m]).not_to include("rounded-full") end - describe "badges" do - def get_badges - get admin_home_badges_path, headers: { "Turbo-Frame" => "badges" } + describe "action buttons frame" do + def get_action_buttons + get admin_home_action_buttons_path, headers: { "Turbo-Frame" => "action_buttons" } end - def badge_stream(target) - response.body[%r{.*?}m] + def button(label) + response.body[%r{<(?:a|button)[^>]*>(?:(?!).)*#{label}.*?}m] end before do @@ -105,14 +108,18 @@ def badge_stream(target) allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(true) end - it "shows Review on Data health, Author credit, and Sync features" do - get_badges + it "shows Review on Data health, Author credit, and Sync features, and the pending change request count" do + create_list(:profile_change_request, 2) + + get_action_buttons expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="badges")) - expect(badge_stream("data_health_badge")).to match(%r{]*>Review}).and include("Data checks only") - expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) - expect(badge_stream("unsynced_features_badge")).to match(%r{]*>Review}) + expect(response.body).to include(%(id="action_buttons"), %(target="_top")) + expect(response.body).not_to include("]*>2}) + expect(button("Data health")).to match(%r{]*>Review}).and include("Data checks only") + expect(button("Author credit divergences")).to match(%r{]*>Review}) + expect(button("Sync features")).to match(%r{]*>Review}) end it "renders no badge when nothing is waiting" do @@ -120,27 +127,26 @@ def badge_stream(target) allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(false) allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(false) - get_badges + get_action_buttons - expect(badge_stream("data_health_badge")).not_to include("rounded-full") - expect(badge_stream("author_credit_badge")).not_to include("rounded-full") - expect(badge_stream("unsynced_features_badge")).not_to include("rounded-full") + expect(response.body).to include("Data health", "Author credit divergences", "Sync features") + expect(response.body).not_to include("rounded-full") end - it "leaves only the failing badge empty when a check fails" do + it "keeps the buttons and leaves only the failing badge empty when a check fails" do allow(DataHealth).to receive(:checks).and_raise(StandardError, "boom") - get_badges + get_action_buttons expect(response).to have_http_status(:ok) - expect(badge_stream("data_health_badge")).not_to include("rounded-full") - expect(badge_stream("author_credit_badge")).to match(%r{]*>Review}) + expect(button("Data health")).not_to include("rounded-full") + expect(button("Author credit divergences")).to match(%r{]*>Review}) end it "denies a non-admin" do sign_in create(:user) - get_badges + get_action_buttons expect(response).not_to have_http_status(:ok) end