From 79c9ea2ce15897c59ffb2abe83ac2cfa9525bb94 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 27 Nov 2025 11:08:45 -0500 Subject: [PATCH] Remove a bunch of N+1s related to notification and comment rendering --- app/controllers/notifications/trays_controller.rb | 2 +- app/models/card/assignable.rb | 2 +- app/models/comment.rb | 3 ++- app/models/notification.rb | 2 +- app/views/cards/_messages.html.erb | 2 +- app/views/cards/comments/reactions/_reactions.html.erb | 2 +- test/controllers/cards/assignments_controller_test.rb | 4 ++-- test/models/card_test.rb | 2 +- 8 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/controllers/notifications/trays_controller.rb b/app/controllers/notifications/trays_controller.rb index 26a1f06e5..c2bac662a 100644 --- a/app/controllers/notifications/trays_controller.rb +++ b/app/controllers/notifications/trays_controller.rb @@ -1,6 +1,6 @@ class Notifications::TraysController < ApplicationController def show - @notifications = Current.user.notifications.unread.ordered.limit(100) + @notifications = Current.user.notifications.preloaded.unread.ordered.limit(100) # Invalidate on the whole set instead of the unread set since the max updated at in the unread set # can stay the same when reading old notifications. diff --git a/app/models/card/assignable.rb b/app/models/card/assignable.rb index c43c50526..be0dd257e 100644 --- a/app/models/card/assignable.rb +++ b/app/models/card/assignable.rb @@ -15,7 +15,7 @@ module Card::Assignable end def assigned_to?(user) - assignments.exists? assignee: user + assignments.any? { |a| a.assignee_id == user.id } end def assigned? diff --git a/app/models/comment.rb b/app/models/comment.rb index a42b205fc..f81c465ad 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -4,11 +4,12 @@ class Comment < ApplicationRecord belongs_to :account, default: -> { card.account } belongs_to :card, touch: true belongs_to :creator, class_name: "User", default: -> { Current.user } - has_many :reactions, dependent: :delete_all + has_many :reactions, -> { order(:created_at) }, dependent: :delete_all has_rich_text :body scope :chronologically, -> { order created_at: :asc, id: :desc } + scope :preloaded, -> { with_rich_text_body.includes(reactions: :reacter) } scope :by_system, -> { joins(:creator).where(creator: { role: "system" }) } scope :by_user, -> { joins(:creator).where.not(creator: { role: "system" }) } diff --git a/app/models/notification.rb b/app/models/notification.rb index 416506783..545f6ab75 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -14,7 +14,7 @@ class Notification < ApplicationRecord after_destroy_commit :broadcast_read after_create :bundle - scope :preloaded, -> { preload(:creator, :account, source: [ :board, :creator ]) } + scope :preloaded, -> { preload(:creator, :account, source: [ :board, :creator, { eventable: [ :closure, :board, :assignments ] } ]) } delegate :notifiable_target, to: :source delegate :card, to: :source diff --git a/app/views/cards/_messages.html.erb b/app/views/cards/_messages.html.erb index c92c94f7c..402a39ec0 100644 --- a/app/views/cards/_messages.html.erb +++ b/app/views/cards/_messages.html.erb @@ -1,6 +1,6 @@ <%= messages_tag(card) do %> <% if card.published? %> - <%= render partial: "cards/comments/comment", collection: card.comments.chronologically, cached: true %> + <%= render partial: "cards/comments/comment", collection: card.comments.preloaded.chronologically, cached: true %> <%= render "cards/comments/new", card: card %> <%= render "cards/comments/watchers", card: card %> diff --git a/app/views/cards/comments/reactions/_reactions.html.erb b/app/views/cards/comments/reactions/_reactions.html.erb index 6de217842..ba3d491eb 100644 --- a/app/views/cards/comments/reactions/_reactions.html.erb +++ b/app/views/cards/comments/reactions/_reactions.html.erb @@ -1,7 +1,7 @@ <%= turbo_frame_tag comment, :reacting do %>
- <%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions.includes(:reacter).ordered %> + <%= render partial: "cards/comments/reactions/reaction", collection: comment.reactions %>
<%= turbo_frame_tag comment, :new_reaction do %> diff --git a/test/controllers/cards/assignments_controller_test.rb b/test/controllers/cards/assignments_controller_test.rb index 9cfcc469f..c823174a8 100644 --- a/test/controllers/cards/assignments_controller_test.rb +++ b/test/controllers/cards/assignments_controller_test.rb @@ -11,12 +11,12 @@ class Cards::AssignmentsControllerTest < ActionDispatch::IntegrationTest end test "create" do - assert_changes "cards(:logo).assigned_to?(users(:david))", from: false, to: true do + assert_changes "cards(:logo).reload.assigned_to?(users(:david))", from: false, to: true do post card_assignments_path(cards(:logo)), params: { assignee_id: users(:david).id }, as: :turbo_stream assert_meta_replaced(cards(:logo)) end - assert_changes "cards(:logo).assigned_to?(users(:david))", from: true, to: false do + assert_changes "cards(:logo).reload.assigned_to?(users(:david))", from: true, to: false do post card_assignments_path(cards(:logo)), params: { assignee_id: users(:david).id }, as: :turbo_stream assert_meta_replaced(cards(:logo)) end diff --git a/test/models/card_test.rb b/test/models/card_test.rb index 4858e7203..e18ce43f8 100644 --- a/test/models/card_test.rb +++ b/test/models/card_test.rb @@ -37,7 +37,7 @@ class CardTest < ActiveSupport::TestCase assert_difference({ -> { cards(:logo).assignees.count } => -1, -> { Event.count } => +1 }) do cards(:logo).toggle_assignment users(:kevin) end - assert_not cards(:logo).assigned_to?(users(:kevin)) + assert_not cards(:logo).reload.assigned_to?(users(:kevin)) unassign_event = Event.last assert_equal "card_unassigned", unassign_event.action assert_equal [ users(:kevin) ], unassign_event.assignees