From cc5c27e2ed7f57deb8fe48c4557cba3b8792d70f Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 27 Apr 2026 15:09:09 -0400 Subject: [PATCH] Speed up comment creation flow Comment creation in production took ~460-830ms with only ~1ms in views and ~200ms in DB. The remaining time was synchronous Ruby work blocking the request: - Notifications were created and badge broadcasts pushed inline. - Each thread popover broadcast re-parsed the entire plan markdown via CommentThread#anchor_occurrence_index -> strip_markdown. - The comment partial fired a per-comment User.find_by lookup, and Comment#first_comment_in_thread? did an order+first scan on every insert. Changes: - Move Notifications::Create off the request path via the new CreateNotificationsJob (controllers + API). - Memoize Plan#stripped_content so repeated thread popover renders share one parse of the plan markdown. - Batch unread badge counts into a single grouped query in Notifications::Create#broadcast_badge_updates. - Extract Comment#author so the helper layer no longer issues queries directly; helper just memoizes the lookup per request. - Replace Comment#first_comment_in_thread? with a race-safe 'no comments with smaller id' check (UUID v7 IDs are sortable). - Wrap render_markdown in a fragment cache keyed by the comment record. Amp-Thread-ID: https://ampcode.com/threads/T-019dcf4e-942f-779b-9f51-12a2fcfb8e1d Co-authored-by: Amp --- .../coplan/api/v1/comments_controller.rb | 16 ++++++++-------- .../coplan/comment_threads_controller.rb | 14 +++++++------- .../controllers/coplan/comments_controller.rb | 6 +++--- engine/app/helpers/coplan/comments_helper.rb | 14 ++++---------- .../app/jobs/coplan/create_notifications_job.rb | 16 ++++++++++++++++ engine/app/models/coplan/comment.rb | 13 ++++++++++++- engine/app/models/coplan/comment_thread.rb | 2 +- engine/app/models/coplan/plan.rb | 10 ++++++++++ .../app/services/coplan/notifications/create.rb | 13 ++++++------- .../app/views/coplan/comments/_comment.html.erb | 4 +++- 10 files changed, 70 insertions(+), 38 deletions(-) create mode 100644 engine/app/jobs/coplan/create_notifications_job.rb diff --git a/engine/app/controllers/coplan/api/v1/comments_controller.rb b/engine/app/controllers/coplan/api/v1/comments_controller.rb index 3e9bebe..61dc428 100644 --- a/engine/app/controllers/coplan/api/v1/comments_controller.rb +++ b/engine/app/controllers/coplan/api/v1/comments_controller.rb @@ -25,10 +25,10 @@ def create ) reason = comment.agent? ? "agent_response" : "new_comment" - Notifications::Create.call( - comment_thread: thread, + CreateNotificationsJob.perform_later( + comment_thread_id: thread.id, actor_id: api_actor_id, - comment: comment, + comment_id: comment.id, reason: reason ) @@ -59,7 +59,7 @@ def resolve end thread.resolve!(current_user) - Notifications::Create.call(comment_thread: thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_update(thread) render json: { thread_id: thread.id, status: thread.status } @@ -79,7 +79,7 @@ def discard end thread.discard!(current_user) - Notifications::Create.call(comment_thread: thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_update(thread) render json: { thread_id: thread.id, status: thread.status } @@ -100,10 +100,10 @@ def reply ) reason = comment.agent? ? "agent_response" : "reply" - Notifications::Create.call( - comment_thread: thread, + CreateNotificationsJob.perform_later( + comment_thread_id: thread.id, actor_id: api_actor_id, - comment: comment, + comment_id: comment.id, reason: reason ) diff --git a/engine/app/controllers/coplan/comment_threads_controller.rb b/engine/app/controllers/coplan/comment_threads_controller.rb index d254d5c..ee6ffd6 100644 --- a/engine/app/controllers/coplan/comment_threads_controller.rb +++ b/engine/app/controllers/coplan/comment_threads_controller.rb @@ -31,10 +31,10 @@ def create body_markdown: params[:comment_thread][:body_markdown] ) - Notifications::Create.call( - comment_thread: thread, + CreateNotificationsJob.perform_later( + comment_thread_id: thread.id, actor_id: current_user.id, - comment: comment, + comment_id: comment.id, reason: "new_comment" ) @@ -53,7 +53,7 @@ def create def resolve authorize!(@thread, :resolve?) @thread.resolve!(current_user) - Notifications::Create.call(comment_thread: @thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_replace(@thread) respond_with_stream_or_redirect("Thread resolved.") end @@ -61,7 +61,7 @@ def resolve def accept authorize!(@thread, :accept?) @thread.accept!(current_user) - Notifications::Create.call(comment_thread: @thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_replace(@thread) respond_with_stream_or_redirect("Thread accepted.") end @@ -69,7 +69,7 @@ def accept def discard authorize!(@thread, :discard?) @thread.discard!(current_user) - Notifications::Create.call(comment_thread: @thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_replace(@thread) respond_with_stream_or_redirect("Thread discarded.") end @@ -77,7 +77,7 @@ def discard def reopen authorize!(@thread, :reopen?) @thread.update!(status: "pending", resolved_by_user: nil) - Notifications::Create.call(comment_thread: @thread, actor_id: current_user.id, reason: "status_change") + CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") broadcast_thread_replace(@thread) respond_with_stream_or_redirect("Thread reopened.") end diff --git a/engine/app/controllers/coplan/comments_controller.rb b/engine/app/controllers/coplan/comments_controller.rb index 0f657e7..d8f7152 100644 --- a/engine/app/controllers/coplan/comments_controller.rb +++ b/engine/app/controllers/coplan/comments_controller.rb @@ -12,10 +12,10 @@ def create body_markdown: params[:comment][:body_markdown] ) - Notifications::Create.call( - comment_thread: @thread, + CreateNotificationsJob.perform_later( + comment_thread_id: @thread.id, actor_id: current_user.id, - comment: comment, + comment_id: comment.id, reason: "reply" ) diff --git a/engine/app/helpers/coplan/comments_helper.rb b/engine/app/helpers/coplan/comments_helper.rb index 29debfc..98c597a 100644 --- a/engine/app/helpers/coplan/comments_helper.rb +++ b/engine/app/helpers/coplan/comments_helper.rb @@ -13,16 +13,10 @@ def comment_author_name(comment) end def comment_author_user(comment) - case comment.author_type - when "human" - CoPlan::User.find_by(id: comment.author_id) - when "local_agent" - CoPlan::User - .joins(:api_tokens) - .where(coplan_api_tokens: { id: comment.author_id }) - .first - else - nil + @_comment_author_cache ||= {} + cache_key = "#{comment.author_type}:#{comment.author_id}" + @_comment_author_cache.fetch(cache_key) do + @_comment_author_cache[cache_key] = comment.author end end end diff --git a/engine/app/jobs/coplan/create_notifications_job.rb b/engine/app/jobs/coplan/create_notifications_job.rb new file mode 100644 index 0000000..ebe658e --- /dev/null +++ b/engine/app/jobs/coplan/create_notifications_job.rb @@ -0,0 +1,16 @@ +module CoPlan + class CreateNotificationsJob < ApplicationJob + queue_as :default + + def perform(comment_thread_id:, actor_id:, comment_id: nil, reason:) + thread = CommentThread.find(comment_thread_id) + comment = comment_id ? Comment.find(comment_id) : nil + Notifications::Create.call( + comment_thread: thread, + actor_id: actor_id, + comment: comment, + reason: reason + ) + end + end +end diff --git a/engine/app/models/coplan/comment.rb b/engine/app/models/coplan/comment.rb index 57ab987..633ba34 100644 --- a/engine/app/models/coplan/comment.rb +++ b/engine/app/models/coplan/comment.rb @@ -15,10 +15,21 @@ def agent? agent_name.present? || author_type.in?(%w[local_agent cloud_persona]) end + # Resolves the comment author to a CoPlan::User instance, or nil for + # author types that don't map to a user (cloud_persona, system). + def author + case author_type + when "human" + CoPlan::User.find_by(id: author_id) + when "local_agent" + CoPlan::User.joins(:api_tokens).where(coplan_api_tokens: { id: author_id }).first + end + end + private def first_comment_in_thread? - self == comment_thread.comments.order(:created_at).first + !comment_thread.comments.where("id < ?", id).exists? end def notify_plan_author diff --git a/engine/app/models/coplan/comment_thread.rb b/engine/app/models/coplan/comment_thread.rb index d86c2f5..5270cc9 100644 --- a/engine/app/models/coplan/comment_thread.rb +++ b/engine/app/models/coplan/comment_thread.rb @@ -124,7 +124,7 @@ def anchor_occurrence_index # When anchor_start is known, count occurrences before it. if anchor_start.present? - stripped, pos_map = self.class.strip_markdown(content) + stripped, pos_map = plan.stripped_content # Map raw anchor_start to its position in the stripped string. # Use >= to find the closest valid position if anchor_start falls # on a stripped formatting character. diff --git a/engine/app/models/coplan/plan.rb b/engine/app/models/coplan/plan.rb index 79e1dbe..bb02aac 100644 --- a/engine/app/models/coplan/plan.rb +++ b/engine/app/models/coplan/plan.rb @@ -40,6 +40,16 @@ def current_content current_plan_version&.content_markdown end + # Memoized stripped-markdown + position map for the current content. + # Reused by multiple CommentThread#anchor_occurrence_index calls within + # the same request to avoid re-parsing the full plan for each thread. + def stripped_content + @stripped_content ||= begin + content = current_content + content.present? ? Plans::MarkdownTextExtractor.call(content) : [+"", []] + end + end + def tag_names tags.pluck(:name) end diff --git a/engine/app/services/coplan/notifications/create.rb b/engine/app/services/coplan/notifications/create.rb index ed9502d..d1f932d 100644 --- a/engine/app/services/coplan/notifications/create.rb +++ b/engine/app/services/coplan/notifications/create.rb @@ -17,7 +17,7 @@ def call subscriber_ids.delete(@actor_id) return if subscriber_ids.empty? - notifications = subscriber_ids.map do |user_id| + subscriber_ids.each do |user_id| Notification.create!( user_id: user_id, plan_id: @comment_thread.plan_id, @@ -27,8 +27,7 @@ def call ) end - broadcast_badge_updates(notifications) - notifications + broadcast_badge_updates(subscriber_ids) end private @@ -75,13 +74,13 @@ def thread_participant_ids ids end - def broadcast_badge_updates(notifications) - notifications.group_by(&:user_id).each do |user_id, _| - count = Notification.where(user_id: user_id).unread.count + def broadcast_badge_updates(subscriber_ids) + counts = Notification.where(user_id: subscriber_ids).unread.group(:user_id).count + subscriber_ids.each do |user_id| Broadcaster.update_to( "coplan_notifications:#{user_id}", target: "inbox-badge", - html: count.to_s + html: (counts[user_id] || 0).to_s ) end end diff --git a/engine/app/views/coplan/comments/_comment.html.erb b/engine/app/views/coplan/comments/_comment.html.erb index 0b4e1fa..5e35fd8 100644 --- a/engine/app/views/coplan/comments/_comment.html.erb +++ b/engine/app/views/coplan/comments/_comment.html.erb @@ -11,6 +11,8 @@ ยท <%= time_ago_in_words(comment.created_at) %> ago
- <%= render_markdown(comment.body_markdown) %> + <%= cache(comment) do %> + <%= render_markdown(comment.body_markdown) %> + <% end %>