refactor(hub): separate published improvement comments

hub/app/controllers/canvas_api/improvement_notes_controller.rb

@@ -5,19 +5,20 @@ class CanvasApi::ImprovementNotesController < CanvasApi::BaseController
   prepend_before_action -> { reject_large_request(maximum: 1_600_000) }, only: :create
   before_action :require_writer, only: :create
   rescue_from ActiveRecord::RecordInvalid, with: :invalid_note
+  rescue_from ImprovementComment::Conflict, with: ->(error) { render json: { error: error.message }, status: :conflict }
 
   # @route GET /api/canvas/repositories/:repository_github_id/prototypes/:prototype_id/improvement_notes (canvas_api_improvement_notes)
   def index
-    if @prototype.improvement_notes.where(user: Current.user).limit(201).count > 200
+    if @prototype.improvement_notes.where(user: Current.user).limit(201).count > ImprovementComment::MAXIMUM_NOTES
       return render json: { error: "Too many improvement notes." }, status: :conflict
     end
 
-    page = ReviewComment.improvement_note_page_for \
-      @prototype,
-      viewer: Current.user,
+    page = CanvasCommentPage.read \
+      notes: @prototype.improvement_notes.where(user: Current.user),
+      purpose: "prototype/#{@prototype.id}/improvement-notes/viewer/#{Current.user.id}",
       cursor: pagination_cursor,
       page_size: page_size do |candidate|
-        note_page_json(candidate).to_json.bytesize <= ReviewComment::NOTE_PAGE_MAXIMUM_BYTES
+        note_page_json(candidate).to_json.bytesize <= ImprovementComment::PAGE_MAXIMUM_BYTES
       end
     render json: note_page_json(page)
   end
@@ -33,68 +34,23 @@ def create
     return head :bad_request if params.key?(:thread) && !params[:thread].is_a?(ActionController::Parameters)
     return head :bad_request if params.key?(:note_thread)
     return head :bad_request if %i[local_id body status capture].any? { !params[_1].nil? && !params[_1].is_a?(String) }
-
-    thread = params[:thread]&.to_unsafe_h
-
-    @prototype.with_lock do
-      note = @prototype.improvement_notes.find_or_initialize_by(user: Current.user, local_id: params[:local_id])
-      if note.note_thread.present? && !params.key?(:thread)
-        return render json: { error: "Upgrade the client to preserve this note's thread." }, status: :conflict
-      end
-      if note.persisted? && note.anchor != params[:anchor].to_unsafe_h
-        return render json: { error: "A saved note's target cannot change." }, status: :conflict
-      end
-
-      if note.persisted? && revision < note.note_revision
-        return render json: { error: "The Hub note has a newer revision." }, status: :conflict
-      end
-      if note.persisted? && revision == note.note_revision
-        if note.deleted_at.present? && thread
-          retry_note = note.dup
-          retry_note.assign_attributes(note_thread: thread, note_status: params[:status])
-          retry_note.note_thread_contract.validate_record
-          raise ActiveRecord::RecordInvalid, retry_note if retry_note.errors.any?
-        end
-        same = note.body == (params[:deleted] ? nil : params[:body]) &&
-          note.note_status == params[:status] &&
-          note.anchor == params[:anchor]&.to_unsafe_h &&
-          note.deleted_at.present? == params[:deleted] &&
-          note.capture == params[:capture] &&
-          note.note_thread == (params[:deleted] ? nil : thread)
-        return render json: { error: "This note revision has different content." }, status: :conflict unless same
-
-        return render json: review_comment_json(note)
-      end
-      return head :conflict if note.deleted_at.present?
-      if previous_revision != (note.persisted? ? note.note_revision : 0)
-        return render json: { error: "The Hub note changed since its last synchronization." }, status: :conflict
-      end
-      if note.new_record?
-        return head :conflict if @prototype.improvement_notes.where(user: Current.user).count >= 200
-
-        note.anchor = params[:anchor]&.to_unsafe_h
-      end
-      note.assign_attributes \
-        body: params[:deleted] ? nil : params[:body],
-        note_revision: revision,
-        note_status: params[:status],
-        note_thread: thread,
-        capture: params[:capture]
-      if params[:deleted]
-        note.created_at ||= Time.current
-        note.deleted_at = Time.current
-      end
-      note.save!
-      render json: review_comment_json(note), status: :ok
+    if params.key?(:build_number) && (!params[:build_number].is_a?(Integer) || !params[:build_number].positive?)
+      return head :bad_request
     end
+
+    attributes = params.permit(:local_id, :revision, :previous_revision, :build_number, :body, :status, :deleted, :capture).to_h.symbolize_keys
+    attributes[:anchor] = params[:anchor].to_unsafe_h
+    attributes[:thread] = params[:thread].to_unsafe_h if params.key?(:thread)
+    note = ImprovementComment.synchronize!(prototype: @prototype, user: Current.user, attributes:)
+    render json: improvement_note_json(note), status: :ok
   end
 
   private
 
   def note_page_json(page)
     {
-      threadVersion: ReviewComment::NoteThreadContract::VERSION,
-      notes: page.records.map { review_comment_json(_1) },
+      threadVersion: ImprovementComment::ThreadContract::VERSION,
+      notes: page.records.map { improvement_note_json(_1) },
       nextCursor: page.next_cursor,
       hasMore: page.has_more
     }

hub/app/controllers/canvas_api/repository_review_comments_controller.rb

@@ -4,14 +4,18 @@ class CanvasApi::RepositoryReviewCommentsController < CanvasApi::BaseController
   # @route GET /api/canvas/repositories/:repository_github_id/review_comments (canvas_api_repository_review_comments)
   def index
     filter = improvement_notes_filter
-    page = ReviewComment.page_for_repository \
-      @repository,
-      viewer: Current.user,
-      cursor: pagination_cursor,
-      page_size: page_size,
-      improvement_notes: filter do |candidate|
-        filter == "exclude" || review_comment_page_json(candidate).to_json.bytesize <= ReviewComment::NOTE_PAGE_MAXIMUM_BYTES
-      end
+    page = if filter == "exclude"
+      ReviewComment.page_for_repository(@repository, viewer: Current.user, cursor: pagination_cursor, page_size:)
+    else
+      CanvasCommentPage.read \
+        notes: ImprovementComment.roots.where(prototype: @repository.reviewable_prototypes),
+        reviews: filter == "include" ? ReviewComment.where(prototype: @repository.reviewable_prototypes) : nil,
+        purpose: "repository/#{@repository.id}/viewer/#{Current.user.id}/#{filter}",
+        cursor: pagination_cursor,
+        page_size: page_size do |candidate|
+          review_comment_page_json(candidate).to_json.bytesize <= ImprovementComment::PAGE_MAXIMUM_BYTES
+        end
+    end
     render json: review_comment_page_json(page)
   end
 
@@ -22,7 +26,7 @@ def review_comment_page_json(page)
     {
       comments: page.records.map do |comment|
         prototype = comment.prototype
-        review_comment_json(comment).merge \
+        canvas_comment_json(comment).merge \
           prototype: { id: prototype.id, name: prototype.name, branch: prototype.branch_name }
       end,
       nextCursor: page.next_cursor,

hub/app/controllers/concerns/has_review_comment_context.rb

@@ -17,20 +17,25 @@ module HasReviewCommentContext
   # Uses the same page shape for browser and paired extension readers.
   def render_review_comment_page
     filter = is_a?(CanvasApi::BaseController) ? improvement_notes_filter : "exclude"
-    page = ReviewComment.page_for \
-      @prototype,
-      cursor: pagination_cursor,
-      page_size: page_size,
-      improvement_notes: filter do |candidate|
-        filter == "exclude" || review_comment_page_json(candidate).to_json.bytesize <= ReviewComment::NOTE_PAGE_MAXIMUM_BYTES
-      end
+    page = if filter == "exclude"
+      ReviewComment.page_for(@prototype, cursor: pagination_cursor, page_size:)
+    else
+      CanvasCommentPage.read \
+        notes: @prototype.improvement_notes,
+        reviews: filter == "include" ? @prototype.review_comments : nil,
+        purpose: "prototype/#{@prototype.id}/#{filter}",
+        cursor: pagination_cursor,
+        page_size: page_size do |candidate|
+          review_comment_page_json(candidate).to_json.bytesize <= ImprovementComment::PAGE_MAXIMUM_BYTES
+        end
+    end
     render json: review_comment_page_json(page)
   end
 
   def review_comment_page_json(page)
     prepare_viewer_thread_reads(page.records)
     {
-      comments: page.records.map { review_comment_json(_1) },
+      comments: page.records.map { canvas_comment_json(_1) },
       nextCursor: page.next_cursor,
       hasMore: page.has_more
     }

hub/app/controllers/concerns/improvement_note_pages.rb

@@ -0,0 +1,26 @@
+# Keeps the existing canvas note payload independent of review comment serialization.
+module ImprovementNotePages
+  private
+
+  # Renders one root and its relational conversation using the existing client contract.
+  def improvement_note_json(note)
+    can_write = note.editable_by?(Current.user) &&
+      @repository.authorization.sufficient?(@repository_permission, minimum: :write)
+    {
+      id: note.id, kind: "improvement_note", localId: note.local_id,
+      revision: note.note_revision, status: note.note_status,
+      buildNumber: note.build.number,
+      body: note.body, anchor: note.anchor, capture: note.capture,
+      thread: note.note_thread,
+      deletedAt: note.deleted_at&.iso8601(6),
+      createdAt: note.created_at.iso8601(6), updatedAt: note.updated_at.iso8601(6),
+      author: author_json(note.user),
+      viewerCanEdit: can_write, viewerCanDelete: can_write
+    }
+  end
+
+  # Selects serialization at the feed boundary, not within either model's behavior.
+  def canvas_comment_json(comment)
+    comment.is_a?(ImprovementComment) ? improvement_note_json(comment) : review_comment_json(comment)
+  end
+end

hub/app/controllers/concerns/review_comment_pages.rb

@@ -1,6 +1,7 @@
 # Shares pagination and serialization without selecting an authentication or prototype context.
 module ReviewCommentPages
   extend ActiveSupport::Concern
+  include ImprovementNotePages
 
   included do
     rescue_from ReviewComment::InvalidPagination, with: :invalid_pagination
@@ -39,21 +40,6 @@ def invalid_pagination
   end
 
   def review_comment_json(comment)
-    if comment.improvement_note?
-      can_write = comment.editable_by?(Current.user) &&
-        @repository.authorization.sufficient?(@repository_permission, minimum: :write)
-      return {
-        id: comment.id, kind: "improvement_note", localId: comment.local_id,
-        revision: comment.note_revision, status: comment.note_status,
-        body: comment.body, anchor: comment.anchor, capture: comment.capture,
-        thread: comment.note_thread,
-        deletedAt: comment.deleted_at&.iso8601(6),
-        createdAt: comment.created_at.iso8601(6), updatedAt: comment.updated_at.iso8601(6),
-        author: author_json(comment.user),
-        viewerCanEdit: can_write,
-        viewerCanDelete: can_write
-      }
-    end
     payload = {
       id: comment.id,
       kind: comment.root_comment? ? "root" : "reply",
@@ -86,7 +72,7 @@ def review_comment_json(comment)
   end
 
   def prepare_viewer_thread_reads(comments)
-    root_ids = comments.filter_map { _1.id if _1.root_comment? && !_1.improvement_note? }
+    root_ids = comments.filter_map { _1.id if _1.is_a?(ReviewComment) && _1.root_comment? }
     @viewer_thread_reads = ReviewThreadRead.where \
       root_id: root_ids,
       user: Current.user

hub/app/models/build.rb

@@ -78,6 +78,7 @@ class Build < ApplicationRecord
   after_update :record_analytics_publication, if: -> { saved_change_to_state? && state == "successful" }
   has_many :webhook_deliveries, dependent: :nullify
   has_many :review_comments, dependent: :restrict_with_exception
+  has_many :improvement_comments, dependent: :restrict_with_exception
   has_many :share_links, dependent: :restrict_with_exception
 
   has_object :package_import

hub/app/models/canvas_comment_page.rb

@@ -0,0 +1,75 @@
+# Paginates the explicitly requested canvas feed across separate review and improvement tables.
+class CanvasCommentPage
+  # Selects small identities first so a page never loads a collection of large captures.
+  def self.read(notes:, purpose:, cursor:, page_size:, reviews: nil, &fits)
+    raise ReviewComment::InvalidPagination unless page_size.is_a?(Integer) && (1..ReviewComment::MAXIMUM_PAGE_SIZE).cover?(page_size)
+
+    connection = ImprovementComment.connection
+    queries = [
+      notes.reselect(Arel.sql("id, created_at, 'improvement' AS source, " \
+        "COALESCE(length(CAST(body AS BLOB)), 0) + COALESCE(length(CAST(anchor AS BLOB)), 0) + " \
+        "COALESCE(length(CAST(capture AS BLOB)), 0) + COALESCE(length(CAST(author_snapshot AS BLOB)), 0) + 512 + " \
+        "COALESCE((SELECT SUM(COALESCE(length(CAST(messages.body AS BLOB)), 0) + " \
+        "COALESCE(length(CAST(messages.author_snapshot AS BLOB)), 0) + 256) " \
+        "FROM improvement_comments messages WHERE messages.root_id = improvement_comments.id), 0) + " \
+        "(SELECT COUNT(*) * 40 FROM improvement_comment_replies links WHERE links.prototype_id = improvement_comments.prototype_id " \
+        "AND links.user_id = improvement_comments.user_id AND links.thread_key = improvement_comments.local_id) AS bytes")).to_sql
+    ]
+    if reviews
+      queries << reviews.reselect(Arel.sql("id, created_at, 'review' AS source, " \
+        "COALESCE(length(CAST(body AS BLOB)), 0) + COALESCE(length(CAST(anchor AS BLOB)), 0) AS bytes")).to_sql
+    end
+    verifier = Rails.application.message_verifier("canvas-comment-cursor")
+    condition = ""
+    if cursor.present?
+      payload = verifier.verified(cursor, purpose:)
+      raise ReviewComment::InvalidPagination unless payload.is_a?(Hash) &&
+        payload["id"].is_a?(Integer) && payload["id"].positive? && %w[review improvement].include?(payload["source"])
+
+      begin
+        time = Time.iso8601(payload.fetch("created_at"))
+      rescue ArgumentError, KeyError, TypeError
+        raise ReviewComment::InvalidPagination
+      end
+      condition = ImprovementComment.sanitize_sql_array([
+        "WHERE (created_at, id, source) < (?, ?, ?)", time, payload["id"], payload["source"]
+      ])
+    end
+    ImprovementComment.transaction do
+      entries = connection.select_all \
+        "SELECT * FROM (#{queries.join(" UNION ALL ")}) #{condition} " \
+        "ORDER BY created_at DESC, id DESC, source DESC LIMIT #{page_size + 1}"
+      bytes = 0
+      selected = entries.first(page_size).take_while do |entry|
+        bytes += entry["bytes"]
+        bytes <= ImprovementComment::PAGE_MAXIMUM_BYTES
+      end
+      raise ReviewComment::ResponseTooLarge if selected.empty? && entries.any?
+
+      notes_by_id = notes.where(id: selected.filter_map { _1["id"] if _1["source"] == "improvement" })
+        .includes(:prototype, :build, :user, replies: { request_links: :request }).index_by(&:id)
+      reviews_by_id = reviews ? reviews.where(id: selected.filter_map { _1["id"] if _1["source"] == "review" })
+        .includes(:prototype, :build, :user, :resolved_by, :reactions, comment_mentions: :github_identity).index_by(&:id) : {}
+      records = selected.map { (_1["source"] == "review" ? reviews_by_id : notes_by_id).fetch(_1["id"]) }
+      loop do
+        more = entries.length > records.length
+        last = records.last
+        next_cursor = if more && last
+          payload = {
+            "created_at" => last.created_at.iso8601(6), "id" => last.id,
+            "source" => last.is_a?(ImprovementComment) ? "improvement" : "review"
+          }
+          verifier.generate \
+            payload,
+            purpose:
+        end
+        page = ReviewComment::Page.new(records:, next_cursor:, has_more: more)
+        return page if !fits || fits.call(page)
+
+        raise ReviewComment::ResponseTooLarge if records.length <= 1
+
+        records = records[...-1]
+      end
+    end
+  end
+end