diff --git a/hub/app/controllers/canvas_api/improvement_notes_controller.rb b/hub/app/controllers/canvas_api/improvement_notes_controller.rb index b3b15709..d37e9a22 100644 --- a/hub/app/controllers/canvas_api/improvement_notes_controller.rb +++ b/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 } diff --git a/hub/app/controllers/canvas_api/repository_review_comments_controller.rb b/hub/app/controllers/canvas_api/repository_review_comments_controller.rb index 0eb2890f..7ef21198 100644 --- a/hub/app/controllers/canvas_api/repository_review_comments_controller.rb +++ b/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, diff --git a/hub/app/controllers/concerns/has_review_comment_context.rb b/hub/app/controllers/concerns/has_review_comment_context.rb index 8d949f9b..c31360db 100644 --- a/hub/app/controllers/concerns/has_review_comment_context.rb +++ b/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 } diff --git a/hub/app/controllers/concerns/improvement_note_pages.rb b/hub/app/controllers/concerns/improvement_note_pages.rb new file mode 100644 index 00000000..a04dce52 --- /dev/null +++ b/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 diff --git a/hub/app/controllers/concerns/review_comment_pages.rb b/hub/app/controllers/concerns/review_comment_pages.rb index f54d98a0..55d0534a 100644 --- a/hub/app/controllers/concerns/review_comment_pages.rb +++ b/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 diff --git a/hub/app/models/build.rb b/hub/app/models/build.rb index 79d85145..c5411c87 100644 --- a/hub/app/models/build.rb +++ b/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 diff --git a/hub/app/models/canvas_comment_page.rb b/hub/app/models/canvas_comment_page.rb new file mode 100644 index 00000000..1146ec1e --- /dev/null +++ b/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 diff --git a/hub/app/models/improvement_comment.rb b/hub/app/models/improvement_comment.rb new file mode 100644 index 00000000..38d13d85 --- /dev/null +++ b/hub/app/models/improvement_comment.rb @@ -0,0 +1,303 @@ +# == Schema Information +# +# Table name: improvement_comments +# Database name: primary +# +# id :integer not null, primary key +# anchor :json +# author_snapshot :json +# body :text +# capture :text +# deleted_at :datetime +# note_revision :integer +# note_status :string +# position :integer default(0), not null +# role :string default("author"), not null +# status :string default("open"), not null +# thread_key :string not null +# thread_version :integer +# created_at :datetime not null +# updated_at :datetime not null +# build_id :integer +# local_id :string not null +# prototype_id :integer not null +# root_id :integer +# user_id :integer not null +# +# Indexes +# +# idx_on_prototype_id_created_at_id_3f3dd1473e (prototype_id,created_at,id) +# idx_on_prototype_id_user_id_local_id_bce29f2a3a (prototype_id,user_id,local_id) UNIQUE +# improvement_comments_root_identity (id,prototype_id,user_id,local_id) UNIQUE +# improvement_comments_thread_identity (id,prototype_id,user_id,thread_key) UNIQUE +# index_improvement_comments_on_build_id (build_id) +# index_improvement_comments_on_prototype_id (prototype_id) +# index_improvement_comments_on_root_id_and_position (root_id,position) UNIQUE +# index_improvement_comments_on_user_id (user_id) +# +# Foreign Keys +# +# [build_id, prototype_id] ([build_id, prototype_id] => builds[id, prototype_id]) ON DELETE => restrict +# [root_id, prototype_id, user_id, thread_key] ([root_id, prototype_id, user_id, thread_key] => improvement_comments[id, prototype_id, user_id, local_id]) ON DELETE => restrict +# prototype_id (prototype_id => prototypes.id) ON DELETE => restrict +# user_id (user_id => users.id) ON DELETE => restrict +# +# Check Constraints +# +# improvement_comments_anchor (anchor IS NULL OR (json_valid(anchor) AND json_type(anchor) = 'object' AND length(CAST(anchor AS BLOB)) <= 16384)) +# improvement_comments_author (author_snapshot IS NULL OR (json_valid(author_snapshot) AND json_type(author_snapshot) = 'object')) +# improvement_comments_body (CASE WHEN deleted_at IS NULL THEN body IS NOT NULL AND length(trim(body)) > 0 AND length(CAST(body AS BLOB)) <= 10000 ELSE body IS NULL AND author_snapshot IS NULL END) +# improvement_comments_capture (capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000) +# improvement_comments_local_id (length(local_id) = 36 AND length(thread_key) = 36) +# improvement_comments_role_status ((role = 'author' AND status IN ('open', 'sent', 'ready', 'blocked')) OR (role = 'copilot' AND status IN ('ready', 'blocked') AND author_snapshot IS NULL)) +# improvement_comments_shape ((root_id IS NULL AND position = 0 AND role = 'author' AND thread_key = local_id AND build_id IS NOT NULL AND anchor IS NOT NULL AND note_revision > 0 AND note_revision IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND note_status IS NOT NULL) OR (root_id IS NOT NULL AND root_id <> id AND position > 0 AND build_id IS NULL AND anchor IS NULL AND capture IS NULL AND note_revision IS NULL AND note_status IS NULL AND thread_version IS NULL)) +# improvement_comments_thread_version (thread_version IS NULL OR thread_version IN (1, 2)) +# +# Stores one private improvement request or agent response, independently of reviews. +class ImprovementComment < ApplicationRecord + PAGE_MAXIMUM_BYTES = 2 * 1024 * 1024 + MAXIMUM_NOTES = 200 + + class Conflict < StandardError; end + + belongs_to :prototype + belongs_to :user + belongs_to :build, optional: true + belongs_to :root, class_name: "ImprovementComment", optional: true, inverse_of: :replies + has_many :replies, -> { order(:position) }, + class_name: "ImprovementComment", foreign_key: :root_id, + inverse_of: :root, dependent: :restrict_with_exception + has_many :request_links, -> { order(:position) }, + class_name: "ImprovementCommentReply", foreign_key: :response_id, + inverse_of: :response, dependent: :restrict_with_exception + has_many :requests, through: :request_links + has_many :response_links, + class_name: "ImprovementCommentReply", foreign_key: :request_id, + inverse_of: :request, dependent: :restrict_with_exception + has_many :responses, through: :response_links + + has_object :thread_contract + + before_validation :set_thread_identity, on: :create + before_save :clear_deleted_snapshot + after_save :save_conversation, if: :root_comment? + + scope :roots, -> { where(root_id: nil) } + + validates :local_id, format: { with: ImprovementComment::ThreadContract::UUID } + validates :role, inclusion: { in: %w[author copilot] } + validates :status, inclusion: { in: %w[open sent ready blocked] } + validates :note_status, inclusion: { in: %w[open sent ready resolved blocked] }, if: :root_comment? + validates :note_revision, numericality: { only_integer: true, greater_than: 0 }, if: :root_comment? + validates :build, presence: true, if: :root_comment? + validates :body, presence: true, if: -> { deleted_at.nil? } + validates :body, absence: true, if: -> { deleted_at.present? } + validates :position, numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validate :validate_message + validate :validate_identity, on: :update + validate -> { thread_contract.validate_record }, if: :root_comment? + + # Saves one complete client revision and its response links in a single transaction. + def self.synchronize!(prototype:, user:, attributes:) + prototype.with_lock do + note = prototype.improvement_notes.find_or_initialize_by(user:, local_id: attributes[:local_id]) + revision = attributes.fetch(:revision) + thread = attributes[:thread] + deleted = attributes.fetch(:deleted) + if note.note_thread.present? && !attributes.key?(:thread) + raise Conflict, "Upgrade the client to preserve this note's thread." + end + if note.persisted? && note.anchor != attributes[:anchor] + raise Conflict, "A saved note's target cannot change." + end + if note.persisted? && attributes.key?(:build_number) && attributes[:build_number] != note.build.number + raise Conflict, "A saved note's Build cannot change." + end + if note.persisted? && revision < note.note_revision + raise Conflict, "The Hub note has a newer revision." + 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: attributes[:status]) + retry_note.thread_contract.validate_record + raise ActiveRecord::RecordInvalid, retry_note if retry_note.errors.any? + end + same = note.body == (deleted ? nil : attributes[:body]) && + note.note_status == attributes[:status] && + note.deleted_at.present? == deleted && + note.capture == attributes[:capture] && + note.note_thread == (deleted ? nil : thread) + raise Conflict, "This note revision has different content." unless same + + return note + end + raise Conflict, "A deleted note cannot be restored." if note.deleted_at.present? + if attributes[:previous_revision] != (note.persisted? ? note.note_revision : 0) + raise Conflict, "The Hub note changed since its last synchronization." + end + if note.new_record? && prototype.improvement_notes.where(user:).count >= MAXIMUM_NOTES + raise Conflict, "Too many improvement notes." + end + + if note.new_record? + note.anchor = attributes[:anchor] + note.build = prototype.builds.find_by(number: attributes[:build_number]) + end + note.assign_attributes \ + body: deleted ? nil : attributes[:body], + note_revision: revision, + note_status: attributes[:status], + note_thread: thread, + capture: attributes[:capture] + note.deleted_at = Time.current if deleted + note.save! + note + end + end + + # Identifies the anchored request that owns the conversation. + def root_comment? + root_id.nil? + end + + # Keeps mutations limited to the creator, even when readers can explicitly list notes. + def editable_by?(actor) + actor.present? && actor.id == user_id && deleted_at.nil? + end + + # Accepts the existing wire format without storing a JSON conversation. + def note_thread=(value) + @incoming_thread = value + end + + # Reconstructs the wire conversation from message rows and ordered response links. + def note_thread + return @incoming_thread if instance_variable_defined?(:@incoming_thread) + + conversation_payload(thread_version, status, author_snapshot) + end + + # Reads the saved conversation for append-only validation before accepting a revision. + def note_thread_in_database + return if new_record? + + conversation_payload(thread_version_in_database, status_in_database, author_snapshot_in_database) + end + + private + + # Clears display history before the database checks the tombstone. + def clear_deleted_snapshot + return if deleted_at.nil? + + self.author_snapshot = nil + self.thread_version = nil + end + + # Uses the root UUID to enforce conversation boundaries on both foreign keys of a reply link. + def set_thread_identity + self.thread_key ||= root ? root.local_id : local_id + end + + # Applies shared anchor rules and checks the root or reply shape without review callbacks. + def validate_message + errors.add(:prototype, "must be a reviewable branch") unless prototype&.reviewable? + if root_comment? + ReviewComment::AnchorContract.new(self).validate_record + if build && (build.prototype_id != prototype_id || build.state != "successful") + errors.add(:build, "must be a successful Build of this prototype") + end + errors.add(:role, "must be author for the original request") unless role == "author" + errors.add(:position, "must be zero for the original request") unless position == 0 + errors.add(:thread_key, "must identify the original request") unless thread_key == local_id + else + unless root&.root_comment? && root.prototype_id == prototype_id && root.user_id == user_id && root.local_id == thread_key + errors.add(:root, "must belong to this creator's conversation") + end + errors.add(:root, "cannot accept replies after deletion") if root&.deleted_at.present? && deleted_at.nil? + errors.add(:position, "must be positive for a reply") unless position&.positive? + %i[build_id anchor capture note_revision note_status thread_version].each do |attribute| + errors.add(attribute, "must be absent for replies") if public_send(attribute).present? + end + end + if body && body.bytesize > ReviewComment::BODY_MAXIMUM_BYTES + errors.add(:body, "must be #{ReviewComment::BODY_MAXIMUM_BYTES} UTF-8 bytes or fewer") + end + if role == "copilot" + errors.add(:author_snapshot, "must be absent for agent responses") if author_snapshot.present? + errors.add(:status, "must be ready or blocked") unless %w[ready blocked].include?(status) + if body && body.encode(Encoding::UTF_16LE).bytesize > 4_000 + errors.add(:body, "must be 2000 UTF-16 code units or fewer") + end + end + if capture.present? && (capture.bytesize > 1_500_000 || !capture.match?(/\Adata:image\/png;base64,[A-Za-z0-9+\/]+=*\z/)) + errors.add(:capture, "must be a bounded PNG capture") + end + end + + # Keeps message identities and saved history immutable, except request statuses and deletion. + def validate_identity + %i[prototype_id build_id user_id root_id local_id thread_key role position anchor].each do |attribute| + errors.add(attribute, "cannot change") if public_send("will_save_change_to_#{attribute}?") + end + if deleted_at_in_database && will_save_change_to_deleted_at? + errors.add(:deleted_at, "cannot change") + end + return if root_comment? || deleted_at.present? + + errors.add(:body, "cannot change") if will_save_change_to_body? + errors.add(:status, "cannot change on agent responses") if role == "copilot" && will_save_change_to_status? + if author_snapshot_in_database.present? && will_save_change_to_author_snapshot? + errors.add(:author_snapshot, "cannot change") + end + end + + # Preserves exact wire order, including the order of requests answered by an agent. + def conversation_payload(version, original_status, original_author) + return if version.nil? + + payload = { + "version" => version, + "originalStatus" => original_status, + "messages" => replies.map do |message| + value = { "id" => message.local_id, "role" => message.role, "body" => message.body, "status" => message.status } + value["author"] = message.author_snapshot if message.author_snapshot + value["requestIds"] = message.request_links.map { _1.request.local_id } if message.role == "copilot" + value + end + } + payload["originalAuthor"] = original_author if original_author + payload + end + + # Persists validated message changes with the root's revision; deleted rows retain no text. + def save_conversation + if deleted_at.present? + replies.update_all(body: nil, author_snapshot: nil, deleted_at:, updated_at: Time.current) + elsif instance_variable_defined?(:@incoming_thread) && @incoming_thread + thread = @incoming_thread + update_columns \ + thread_version: thread["version"], + status: thread["originalStatus"], + author_snapshot: thread["originalAuthor"] + messages = { local_id => self } + thread["messages"].each_with_index do |input, index| + message = replies.find_or_initialize_by(local_id: input["id"]) + message.assign_attributes \ + prototype:, user:, thread_key: local_id, + position: index + 1, role: input["role"], body: input["body"], + status: input["status"], author_snapshot: input["author"] + message.save! + if message.role == "copilot" && message.request_links.empty? + input["requestIds"].each_with_index do |request_id, position| + message.request_links.create!(request: messages.fetch(request_id), prototype:, user:, thread_key: local_id, position:) + end + end + messages[message.local_id] = message + end + end + remove_instance_variable(:@incoming_thread) if instance_variable_defined?(:@incoming_thread) + replies.reset + end +end diff --git a/hub/app/models/review_comment/note_thread_contract.rb b/hub/app/models/improvement_comment/thread_contract.rb similarity index 96% rename from hub/app/models/review_comment/note_thread_contract.rb rename to hub/app/models/improvement_comment/thread_contract.rb index f4b123b4..57f7c184 100644 --- a/hub/app/models/review_comment/note_thread_contract.rb +++ b/hub/app/models/improvement_comment/thread_contract.rb @@ -1,5 +1,5 @@ -# Validates author requests and immutable Copilot answers without creating review replies. -class ReviewComment::NoteThreadContract < ActiveRecord::AssociatedObject +# Validates the wire conversation before saving its individual messages and response links. +class ImprovementComment::ThreadContract < ActiveRecord::AssociatedObject VERSION = 2 MAXIMUM_BYTES = 32_768 MAXIMUM_MESSAGES = 100 @@ -15,7 +15,6 @@ def validate_record end validate_history(previous, thread) if previous.present? return if thread.nil? - return invalid("is only allowed on improvement notes") unless record.improvement_note? return invalid("must be a JSON object") unless thread.is_a?(Hash) return invalid("must be #{MAXIMUM_BYTES} UTF-8 bytes or fewer") if JSON.generate(thread).bytesize > MAXIMUM_BYTES return invalid("version must be 1 or #{VERSION}") unless thread["version"].is_a?(Integer) && [ 1, VERSION ].include?(thread["version"]) diff --git a/hub/app/models/improvement_comment_reply.rb b/hub/app/models/improvement_comment_reply.rb new file mode 100644 index 00000000..fd290cb4 --- /dev/null +++ b/hub/app/models/improvement_comment_reply.rb @@ -0,0 +1,58 @@ +# == Schema Information +# +# Table name: improvement_comment_replies +# Database name: primary +# +# id :integer not null, primary key +# position :integer not null +# thread_key :string not null +# prototype_id :integer not null +# request_id :integer not null +# response_id :integer not null +# user_id :integer not null +# +# Indexes +# +# idx_on_response_id_request_id_1aa61357ff (response_id,request_id) UNIQUE +# improvement_comment_replies_thread (prototype_id,user_id,thread_key) +# index_improvement_comment_replies_on_prototype_id (prototype_id) +# index_improvement_comment_replies_on_request_id (request_id) +# index_improvement_comment_replies_on_response_id_and_position (response_id,position) UNIQUE +# index_improvement_comment_replies_on_user_id (user_id) +# +# Foreign Keys +# +# [request_id, prototype_id, user_id, thread_key] ([request_id, prototype_id, user_id, thread_key] => improvement_comments[id, prototype_id, user_id, thread_key]) ON DELETE => restrict +# [response_id, prototype_id, user_id, thread_key] ([response_id, prototype_id, user_id, thread_key] => improvement_comments[id, prototype_id, user_id, thread_key]) ON DELETE => restrict +# +# Check Constraints +# +# improvement_comment_replies_shape (response_id <> request_id AND position >= 0) +# +# Links an agent response to one of the requests it answers in the same conversation. +class ImprovementCommentReply < ApplicationRecord + belongs_to :response, class_name: "ImprovementComment", inverse_of: :request_links + belongs_to :request, class_name: "ImprovementComment", inverse_of: :response_links + belongs_to :prototype + belongs_to :user + + validate :same_conversation + validate :identity_is_unchanged, on: :update + + private + + # Allows agent responses to answer only earlier author requests in their own conversation. + def same_conversation + unless response&.role == "copilot" && request&.role == "author" && + response.root_id == (request.root_id || request.id) && + response.position > request.position && + [ response, request ].all? { _1.prototype_id == prototype_id && _1.user_id == user_id && _1.thread_key == thread_key } + errors.add(:request, "must be an earlier author request in this response's conversation") + end + end + + # Keeps the requests answered by a saved response immutable. + def identity_is_unchanged + errors.add(:base, "Saved response links cannot change.") if changed? + end +end diff --git a/hub/app/models/learning/collection.rb b/hub/app/models/learning/collection.rb index e161c1b5..77837da1 100644 --- a/hub/app/models/learning/collection.rb +++ b/hub/app/models/learning/collection.rb @@ -73,7 +73,7 @@ def target end def threads - existing = ReviewComment.ordinary.where(created_at: ..@captured_at) + existing = ReviewComment.where(created_at: ..@captured_at) roots = existing.where(prototype: @repository.reviewable_prototypes, root_id: nil).where.not(build_id: nil) .includes(:user, :build, :prototype, :resolved_by).order(:created_at, :id).to_a replies = existing.where(root_id: roots.map(&:id)).includes(:user, :prototype) diff --git a/hub/app/models/learning/overview.rb b/hub/app/models/learning/overview.rb index bb50de82..b907a7d6 100644 --- a/hub/app/models/learning/overview.rb +++ b/hub/app/models/learning/overview.rb @@ -127,7 +127,7 @@ def evidence(reference, comments, events) # The comments this store's evidence names, when they come from this Hub and this repository. def review_comments(references) ids = references.filter_map { _1.dig("source", "commentId") } - ReviewComment.ordinary.where(id: ids.uniq, prototype: @repository.reviewable_prototypes) + ReviewComment.where(id: ids.uniq, prototype: @repository.reviewable_prototypes) .includes(:user, :prototype).index_by(&:id) end diff --git a/hub/app/models/learning/run.rb b/hub/app/models/learning/run.rb index d4ed331c..749920f6 100644 --- a/hub/app/models/learning/run.rb +++ b/hub/app/models/learning/run.rb @@ -157,7 +157,7 @@ def promote!(run, user) def due?(repository) last = where(repository:, state: "succeeded").order(:started_at).last - comments = ReviewComment.ordinary.where(prototype: repository.reviewable_prototypes) + comments = ReviewComment.where(prototype: repository.reviewable_prototypes) return comments.exists? if last.nil? last.comments_remaining.positive? || last.suggestions_remaining.positive? || diff --git a/hub/app/models/prototype.rb b/hub/app/models/prototype.rb index 15d6c094..c772389e 100644 --- a/hub/app/models/prototype.rb +++ b/hub/app/models/prototype.rb @@ -63,9 +63,9 @@ class Prototype < ApplicationRecord has_many :builds has_many :branch_banner_dismissals, dependent: :delete_all has_many :prototype_introductions, dependent: :delete_all - has_many :review_comments, -> { where(kind: "review") }, dependent: :restrict_with_exception - has_many :improvement_notes, -> { where(kind: "improvement_note") }, - class_name: "ReviewComment", dependent: :restrict_with_exception + has_many :review_comments, dependent: :restrict_with_exception + has_many :improvement_notes, -> { where(root_id: nil) }, + class_name: "ImprovementComment", dependent: :restrict_with_exception has_many :share_links, dependent: :restrict_with_exception validates :branch_name, presence: true, length: { maximum: 255 } diff --git a/hub/app/models/review_comment.rb b/hub/app/models/review_comment.rb index 99132b78..ac396115 100644 --- a/hub/app/models/review_comment.rb +++ b/hub/app/models/review_comment.rb @@ -7,19 +7,13 @@ # activity_version :integer # anchor :json # body :text -# capture :text # deleted_at :datetime # edited_at :datetime -# kind :string default("review"), not null # learning_removal_due_at :datetime -# note_revision :integer -# note_status :string -# note_thread :json # resolved_at :datetime # created_at :datetime not null # updated_at :datetime not null # build_id :integer -# local_id :string # prototype_id :integer not null # resolved_by_id :integer # root_id :integer @@ -27,14 +21,13 @@ # # Indexes # -# index_review_comments_on_build_id (build_id) -# index_review_comments_on_id_and_prototype (id,prototype_id) UNIQUE -# index_review_comments_on_learning_removal_due_at (learning_removal_due_at) WHERE learning_removal_due_at IS NOT NULL -# index_review_comments_on_prototype_and_created_at (prototype_id,created_at DESC,id DESC) -# index_review_comments_on_prototype_id_and_user_id_and_local_id (prototype_id,user_id,local_id) UNIQUE -# index_review_comments_on_resolved_by_id (resolved_by_id) -# index_review_comments_on_root_id (root_id) -# index_review_comments_on_user_id (user_id) +# index_review_comments_on_build_id (build_id) +# index_review_comments_on_id_and_prototype (id,prototype_id) UNIQUE +# index_review_comments_on_learning_removal_due_at (learning_removal_due_at) WHERE learning_removal_due_at IS NOT NULL +# index_review_comments_on_prototype_and_created_at (prototype_id,created_at DESC,id DESC) +# index_review_comments_on_resolved_by_id (resolved_by_id) +# index_review_comments_on_root_id (root_id) +# index_review_comments_on_user_id (user_id) # # Foreign Keys # @@ -53,11 +46,9 @@ # review_comments_body (CASE WHEN deleted_at IS NULL THEN body IS NOT NULL AND length(trim(body)) > 0 AND length(CAST(body AS BLOB)) <= 10000 ELSE body IS NULL END) # review_comments_deletion_time (deleted_at IS NULL OR deleted_at >= created_at) # review_comments_edit_state (edited_at IS NULL OR (deleted_at IS NULL AND edited_at >= created_at)) -# review_comments_kind ((kind = 'review' AND local_id IS NULL AND note_revision IS NULL AND note_status IS NULL AND capture IS NULL) OR (kind = 'improvement_note' AND root_id IS NULL AND build_id IS NULL AND local_id IS NOT NULL AND length(local_id) = 36 AND note_revision IS NOT NULL AND note_revision > 0 AND note_status IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND (capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000))) -# review_comments_note_thread (note_thread IS NULL OR (kind = 'improvement_note' AND CASE WHEN json_valid(note_thread) THEN json_type(note_thread) = 'object' ELSE 0 END AND length(CAST(note_thread AS BLOB)) BETWEEN 2 AND 32768)) # review_comments_resolution ((resolved_at IS NULL AND resolved_by_id IS NULL) OR (root_id IS NULL AND resolved_at IS NOT NULL AND resolved_by_id IS NOT NULL)) # review_comments_root_not_self (root_id IS NULL OR root_id <> id) -# review_comments_thread_shape ((root_id IS NULL AND anchor IS NOT NULL AND (build_id IS NOT NULL OR kind = 'improvement_note')) OR (kind = 'review' AND root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)) +# review_comments_thread_shape ((root_id IS NULL AND build_id IS NOT NULL AND anchor IS NOT NULL) OR (root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)) # # Stores one root comment or reply in a Prototype review thread. class ReviewComment < ApplicationRecord @@ -81,7 +72,6 @@ class ReviewComment < ApplicationRecord VIEWPORT_DIMENSION_MAXIMUM = 100_000 DEFAULT_PAGE_SIZE = 200 MAXIMUM_PAGE_SIZE = 200 - NOTE_PAGE_MAXIMUM_BYTES = 2 * 1024 * 1024 # How long Hub waits, after a deletion or after the removal job last started, before it queues the # removal of a deleted comment's text from the learning store again. It is longer than any wait # between the job's own retries, so Hub never adds a second job while the first still tries. @@ -123,7 +113,6 @@ class TooManyMentions < StandardError; end dependent: :delete_all has_object :anchor_contract - has_object :note_thread_contract has_object :thread has_object :mentions @@ -163,26 +152,17 @@ def self.create_with_mentions!(prototype:, user:, body:, **attributes) end before_validation :set_initial_activity_version, on: :create - before_save :clear_deleted_note_thread - after_create :record_author_read, if: -> { root_comment? && !improvement_note? } - after_create :record_analytics_creation, unless: :improvement_note? + after_create :record_author_read, if: :root_comment? + after_create :record_analytics_creation before_destroy :remove_notifications scope :roots, -> { where(root_id: nil) } scope :not_deleted, -> { where(deleted_at: nil) } scope :unresolved, -> { where(resolved_at: nil) } - scope :ordinary, -> { where(kind: "review") } - scope :improvement_notes, -> { where(kind: "improvement_note") } validates :body, presence: true, if: -> { deleted_at.nil? } validates :body, absence: true, if: -> { deleted_at.present? } - validates :kind, inclusion: { in: %w[review improvement_note] } - validates :build, presence: true, if: -> { root_comment? && !improvement_note? } - validates :note_status, inclusion: { in: %w[open sent ready resolved blocked] }, if: :improvement_note? - validates :note_revision, numericality: { only_integer: true, greater_than: 0 }, if: :improvement_note? - validates :local_id, format: { with: /\A[0-9a-f]{8}-(?:[0-9a-f]{4}-){3}[0-9a-f]{12}\z/ }, if: :improvement_note? - validate :improvement_note_shape - validate -> { note_thread_contract.validate_record } + validates :build, presence: true, if: :root_comment? validates :build, absence: true, if: :reply? validates :anchor, presence: true, if: :root_comment? validates :anchor, absence: true, if: :reply? @@ -203,41 +183,26 @@ def self.create_with_mentions!(prototype:, user:, body:, **attributes) class << self # Returns comments newest first with a signed token for the next page. - def page_for(prototype, cursor:, page_size:, improvement_notes: "exclude", &fits) + def page_for(prototype, cursor:, page_size:) paginate \ - filtered_notes(improvement_notes).where(prototype:).includes(:build, :user, :resolved_by, :reactions, comment_mentions: :github_identity), - purpose: "prototype/#{prototype.id}/#{improvement_notes}", + where(prototype:).includes(:build, :user, :resolved_by, :reactions, comment_mentions: :github_identity), + purpose: "prototype/#{prototype.id}", cursor:, - page_size:, - maximum_payload_bytes: improvement_notes == "exclude" ? nil : NOTE_PAGE_MAXIMUM_BYTES, - &fits + page_size: end # Returns one bounded stream across every reviewable branch in the repository. - def page_for_repository(repository, viewer:, cursor:, page_size:, improvement_notes: "exclude", &fits) - paginate \ - filtered_notes(improvement_notes).where(prototype: repository.reviewable_prototypes).includes(:prototype, :build, :user, :resolved_by, :reactions, comment_mentions: :github_identity), - purpose: "repository/#{repository.id}/viewer/#{viewer.id}/#{improvement_notes}", - cursor:, - page_size:, - maximum_payload_bytes: improvement_notes == "exclude" ? nil : NOTE_PAGE_MAXIMUM_BYTES, - &fits - end - - # Reads one creator's notes without materializing a whole collection of captures. - def improvement_note_page_for(prototype, viewer:, cursor:, page_size:, &fits) + def page_for_repository(repository, viewer:, cursor:, page_size:) paginate \ - improvement_notes.where(prototype:, user: viewer).includes(:user), - purpose: "prototype/#{prototype.id}/improvement-notes/viewer/#{viewer.id}", + where(prototype: repository.reviewable_prototypes).includes(:prototype, :build, :user, :resolved_by, :reactions, comment_mentions: :github_identity), + purpose: "repository/#{repository.id}/viewer/#{viewer.id}", cursor:, - page_size:, - maximum_payload_bytes: NOTE_PAGE_MAXIMUM_BYTES, - &fits + page_size: end # Reads only visible starting comments, with filters bound into the cursor. def summary_page_for_repository(repository, viewer:, cursor:, page_size:, filters:, &fits) - relation = ordinary.roots.not_deleted.where(prototype: repository.reviewable_prototypes) + relation = roots.not_deleted.where(prototype: repository.reviewable_prototypes) relation = relation.where(resolved_at: nil) if filters[:resolution] == "unresolved" relation = relation.where.not(resolved_at: nil) if filters[:resolution] == "resolved" relation = relation.where(created_at: filters[:from]..) if filters[:from] @@ -265,29 +230,14 @@ def reply_page_for(root, viewer:, cursor:, page_size:, &fits) private - # Opts into author notes only on explicit authorized API reads. - def filtered_notes(filter) - case filter - when "exclude" then ordinary - when "include" then all - when "only" then improvement_notes - else raise InvalidPagination - end - end - # Uses the same signed keyset pagination for prototype and repository readers. - def paginate(relation, purpose:, cursor:, page_size:, direction: :desc, activity_version: nil, maximum_payload_bytes: nil, &fits) + def paginate(relation, purpose:, cursor:, page_size:, direction: :desc, activity_version: nil, &fits) raise InvalidPagination if !page_size.is_a?(Integer) || !(1..MAXIMUM_PAGE_SIZE).cover?(page_size) relation = before_cursor(relation, purpose, cursor, direction:, activity_version:) if cursor.present? - relation = relation.reorder(created_at: direction, id: direction) - if maximum_payload_bytes - records, has_more = payload_bounded_records(relation, page_size:, maximum_payload_bytes:) - else - records = relation.limit(page_size + 1).to_a - has_more = records.length > page_size - records = records.first(page_size) - end + records = relation.reorder(created_at: direction, id: direction).limit(page_size + 1).to_a + has_more = records.length > page_size + records = records.first(page_size) make_page = lambda do |count| more = has_more || count < records.length next_cursor = encode_cursor(purpose, records[count - 1], activity_version:) if more && count.positive? @@ -314,24 +264,6 @@ def paginate(relation, purpose:, cursor:, page_size:, direction: :desc, activity best end - # Measures payloads in SQLite before loading captures, using one snapshot for both reads. - def payload_bounded_records(relation, page_size:, maximum_payload_bytes:) - transaction do - lengths = %w[body anchor capture note_thread].map do |column| - "COALESCE(length(CAST(review_comments.#{column} AS BLOB)), 0)" - end - sizes = relation.except(:includes).limit(page_size + 1).pluck(Arel.sql(lengths.join(" + "))) - bytes = 0 - count = sizes.first(page_size).take_while do |size| - bytes += size - bytes <= maximum_payload_bytes - end.length - raise ResponseTooLarge if count.zero? && sizes.any? - - [ relation.limit(count).to_a, sizes.length > count ] - end - end - # Continues past the signed page position in the requested order. def before_cursor(relation, purpose, cursor, direction:, activity_version:) payload = Rails.application.message_verifier("review-comment-cursor").verified \ @@ -398,29 +330,8 @@ def remove_from_learning_store!(by: nil) self.class.where(id:).update_all(learning_removal_due_at: nil) end - # Separates author improvement work from ordinary review threads. - def improvement_note? - kind == "improvement_note" - end - private - # Deletes conversation text with the original body, retaining the existing tombstone. - def clear_deleted_note_thread - self.note_thread = nil if improvement_note? && deleted_at.present? - end - - # Keeps notes standalone, bounded and outside ordinary review interactions. - def improvement_note_shape - return unless improvement_note? - - errors.add(:root, "must be absent for improvement notes") if root_id.present? - errors.add(:build, "must be absent for improvement notes") if build_id.present? - if capture.present? && (capture.bytesize > 1_500_000 || !capture.match?(/\Adata:image\/png;base64,[A-Za-z0-9+\/]+=*\z/)) - errors.add(:capture, "must be a bounded PNG capture") - end - end - # Cleans polymorphic resources when a message is physically removed. def remove_notifications Notification.remove_for_message!(self) @@ -486,7 +397,6 @@ def root_is_direct return if root.nil? errors.add(:root, "must be a root comment") if root.root_id.present? - errors.add(:root, "cannot be an improvement note") if root.improvement_note? errors.add(:root, "must belong to this prototype") if root.prototype_id != prototype_id end @@ -502,7 +412,7 @@ def resolution_matches_root # Prevents thread identity and anchor fields from changing after creation. def identity_is_unchanged - %i[prototype_id build_id user_id anchor root_id kind local_id].each do |attribute| + %i[prototype_id build_id user_id anchor root_id].each do |attribute| errors.add(attribute, "cannot change") if public_send("will_save_change_to_#{attribute}?") end end diff --git a/hub/db/migrate/20261013080000_add_improvement_notes_to_review_comments.rb b/hub/db/migrate/20261013080000_add_improvement_notes_to_review_comments.rb deleted file mode 100644 index ab2e5e40..00000000 --- a/hub/db/migrate/20261013080000_add_improvement_notes_to_review_comments.rb +++ /dev/null @@ -1,20 +0,0 @@ -class AddImprovementNotesToReviewComments < ActiveRecord::Migration[8.1] - def change - add_column :review_comments, :kind, :string, null: false, default: "review" - add_column :review_comments, :local_id, :string - add_column :review_comments, :note_revision, :integer - add_column :review_comments, :note_status, :string - add_column :review_comments, :capture, :text - add_index :review_comments, %i[prototype_id user_id local_id], unique: true - add_check_constraint :review_comments, - "(kind = 'review' AND local_id IS NULL AND note_revision IS NULL AND note_status IS NULL AND capture IS NULL) OR " \ - "(kind = 'improvement_note' AND root_id IS NULL AND build_id IS NULL AND local_id IS NOT NULL AND length(local_id) = 36 AND note_revision IS NOT NULL AND note_revision > 0 AND note_status IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND (capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000))", - name: "review_comments_kind" - remove_check_constraint :review_comments, - "(root_id IS NULL AND build_id IS NOT NULL AND anchor IS NOT NULL) OR (root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)", - name: "review_comments_thread_shape" - add_check_constraint :review_comments, - "(root_id IS NULL AND anchor IS NOT NULL AND (build_id IS NOT NULL OR kind = 'improvement_note')) OR (kind = 'review' AND root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)", - name: "review_comments_thread_shape" - end -end diff --git a/hub/db/migrate/20261019140000_add_note_thread_to_review_comments.rb b/hub/db/migrate/20261019140000_add_note_thread_to_review_comments.rb deleted file mode 100644 index 3b7a37a8..00000000 --- a/hub/db/migrate/20261019140000_add_note_thread_to_review_comments.rb +++ /dev/null @@ -1,10 +0,0 @@ -class AddNoteThreadToReviewComments < ActiveRecord::Migration[8.1] - def change - add_column :review_comments, :note_thread, :json - add_check_constraint :review_comments, - "note_thread IS NULL OR (kind = 'improvement_note' AND " \ - "CASE WHEN json_valid(note_thread) THEN json_type(note_thread) = 'object' ELSE 0 END AND " \ - "length(CAST(note_thread AS BLOB)) BETWEEN 2 AND 32768)", - name: "review_comments_note_thread" - end -end diff --git a/hub/db/migrate/20261019140000_create_improvement_comments.rb b/hub/db/migrate/20261019140000_create_improvement_comments.rb new file mode 100644 index 00000000..da443abb --- /dev/null +++ b/hub/db/migrate/20261019140000_create_improvement_comments.rb @@ -0,0 +1,77 @@ +class CreateImprovementComments < ActiveRecord::Migration[8.1] + def change + create_table :improvement_comments do |t| + t.references :prototype, null: false, foreign_key: { on_delete: :restrict } + t.references :user, null: false, foreign_key: { on_delete: :restrict } + t.integer :build_id + t.integer :root_id + t.string :local_id, null: false + t.string :thread_key, null: false + t.integer :position, null: false, default: 0 + t.string :role, null: false, default: "author" + t.string :status, null: false, default: "open" + t.text :body + t.json :author_snapshot + t.json :anchor + t.text :capture + t.integer :note_revision + t.string :note_status + t.integer :thread_version + t.datetime :deleted_at + t.timestamps + end + add_index :improvement_comments, %i[prototype_id user_id local_id], unique: true + add_index :improvement_comments, %i[id prototype_id user_id local_id], unique: true, name: "improvement_comments_root_identity" + add_index :improvement_comments, %i[id prototype_id user_id thread_key], unique: true, name: "improvement_comments_thread_identity" + add_index :improvement_comments, %i[root_id position], unique: true + add_index :improvement_comments, %i[prototype_id created_at id] + add_index :improvement_comments, :build_id + add_foreign_key :improvement_comments, :builds, + column: %i[build_id prototype_id], primary_key: %i[id prototype_id], on_delete: :restrict + add_foreign_key :improvement_comments, :improvement_comments, + column: %i[root_id prototype_id user_id thread_key], + primary_key: %i[id prototype_id user_id local_id], on_delete: :restrict + add_check_constraint :improvement_comments, + "(root_id IS NULL AND position = 0 AND role = 'author' AND thread_key = local_id AND build_id IS NOT NULL AND anchor IS NOT NULL AND note_revision > 0 AND note_revision IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND note_status IS NOT NULL) OR " \ + "(root_id IS NOT NULL AND root_id <> id AND position > 0 AND build_id IS NULL AND anchor IS NULL AND capture IS NULL AND note_revision IS NULL AND note_status IS NULL AND thread_version IS NULL)", + name: "improvement_comments_shape" + add_check_constraint :improvement_comments, + "(role = 'author' AND status IN ('open', 'sent', 'ready', 'blocked')) OR (role = 'copilot' AND status IN ('ready', 'blocked') AND author_snapshot IS NULL)", + name: "improvement_comments_role_status" + add_check_constraint :improvement_comments, + "CASE WHEN deleted_at IS NULL THEN body IS NOT NULL AND length(trim(body)) > 0 AND length(CAST(body AS BLOB)) <= 10000 ELSE body IS NULL AND author_snapshot IS NULL END", + name: "improvement_comments_body" + add_check_constraint :improvement_comments, + "thread_version IS NULL OR thread_version IN (1, 2)", name: "improvement_comments_thread_version" + add_check_constraint :improvement_comments, + "length(local_id) = 36 AND length(thread_key) = 36", name: "improvement_comments_local_id" + add_check_constraint :improvement_comments, + "anchor IS NULL OR (json_valid(anchor) AND json_type(anchor) = 'object' AND length(CAST(anchor AS BLOB)) <= 16384)", + name: "improvement_comments_anchor" + add_check_constraint :improvement_comments, + "author_snapshot IS NULL OR (json_valid(author_snapshot) AND json_type(author_snapshot) = 'object')", + name: "improvement_comments_author" + add_check_constraint :improvement_comments, + "capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000", name: "improvement_comments_capture" + + create_table :improvement_comment_replies do |t| + t.integer :response_id, null: false + t.integer :request_id, null: false + t.references :prototype, null: false + t.references :user, null: false + t.string :thread_key, null: false + t.integer :position, null: false + end + add_index :improvement_comment_replies, %i[response_id request_id], unique: true + add_index :improvement_comment_replies, %i[response_id position], unique: true + add_index :improvement_comment_replies, :request_id + add_index :improvement_comment_replies, %i[prototype_id user_id thread_key], name: "improvement_comment_replies_thread" + %i[response_id request_id].each do |column| + add_foreign_key :improvement_comment_replies, :improvement_comments, + column: [ column, :prototype_id, :user_id, :thread_key ], + primary_key: %i[id prototype_id user_id thread_key], on_delete: :restrict + end + add_check_constraint :improvement_comment_replies, + "response_id <> request_id AND position >= 0", name: "improvement_comment_replies_shape" + end +end diff --git a/hub/db/schema.rb b/hub/db/schema.rb index c98c2a46..8afba18d 100644 --- a/hub/db/schema.rb +++ b/hub/db/schema.rb @@ -303,6 +303,60 @@ t.index ["proof_digest"], name: "index_hub_admin_enrollments_on_proof_digest", unique: true end + create_table "improvement_comment_replies", force: :cascade do |t| + t.integer "response_id", null: false + t.integer "request_id", null: false + t.integer "prototype_id", null: false + t.integer "user_id", null: false + t.string "thread_key", null: false + t.integer "position", null: false + t.index ["prototype_id", "user_id", "thread_key"], name: "improvement_comment_replies_thread" + t.index ["prototype_id"], name: "index_improvement_comment_replies_on_prototype_id" + t.index ["request_id"], name: "index_improvement_comment_replies_on_request_id" + t.index ["response_id", "position"], name: "index_improvement_comment_replies_on_response_id_and_position", unique: true + t.index ["response_id", "request_id"], name: "idx_on_response_id_request_id_1aa61357ff", unique: true + t.index ["user_id"], name: "index_improvement_comment_replies_on_user_id" + t.check_constraint "response_id <> request_id AND position >= 0", name: "improvement_comment_replies_shape" + end + + create_table "improvement_comments", force: :cascade do |t| + t.integer "prototype_id", null: false + t.integer "user_id", null: false + t.integer "build_id" + t.integer "root_id" + t.string "local_id", null: false + t.string "thread_key", null: false + t.integer "position", default: 0, null: false + t.string "role", default: "author", null: false + t.string "status", default: "open", null: false + t.text "body" + t.json "author_snapshot" + t.json "anchor" + t.text "capture" + t.integer "note_revision" + t.string "note_status" + t.integer "thread_version" + t.datetime "deleted_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["build_id"], name: "index_improvement_comments_on_build_id" + t.index ["id", "prototype_id", "user_id", "local_id"], name: "improvement_comments_root_identity", unique: true + t.index ["id", "prototype_id", "user_id", "thread_key"], name: "improvement_comments_thread_identity", unique: true + t.index ["prototype_id", "created_at", "id"], name: "idx_on_prototype_id_created_at_id_3f3dd1473e" + t.index ["prototype_id", "user_id", "local_id"], name: "idx_on_prototype_id_user_id_local_id_bce29f2a3a", unique: true + t.index ["prototype_id"], name: "index_improvement_comments_on_prototype_id" + t.index ["root_id", "position"], name: "index_improvement_comments_on_root_id_and_position", unique: true + t.index ["user_id"], name: "index_improvement_comments_on_user_id" + t.check_constraint "(role = 'author' AND status IN ('open', 'sent', 'ready', 'blocked')) OR (role = 'copilot' AND status IN ('ready', 'blocked') AND author_snapshot IS NULL)", name: "improvement_comments_role_status" + t.check_constraint "(root_id IS NULL AND position = 0 AND role = 'author' AND thread_key = local_id AND build_id IS NOT NULL AND anchor IS NOT NULL AND note_revision > 0 AND note_revision IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND note_status IS NOT NULL) OR (root_id IS NOT NULL AND root_id <> id AND position > 0 AND build_id IS NULL AND anchor IS NULL AND capture IS NULL AND note_revision IS NULL AND note_status IS NULL AND thread_version IS NULL)", name: "improvement_comments_shape" + t.check_constraint "CASE WHEN deleted_at IS NULL THEN body IS NOT NULL AND length(trim(body)) > 0 AND length(CAST(body AS BLOB)) <= 10000 ELSE body IS NULL AND author_snapshot IS NULL END", name: "improvement_comments_body" + t.check_constraint "anchor IS NULL OR (json_valid(anchor) AND json_type(anchor) = 'object' AND length(CAST(anchor AS BLOB)) <= 16384)", name: "improvement_comments_anchor" + t.check_constraint "author_snapshot IS NULL OR (json_valid(author_snapshot) AND json_type(author_snapshot) = 'object')", name: "improvement_comments_author" + t.check_constraint "capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000", name: "improvement_comments_capture" + t.check_constraint "length(local_id) = 36 AND length(thread_key) = 36", name: "improvement_comments_local_id" + t.check_constraint "thread_version IS NULL OR thread_version IN (1, 2)", name: "improvement_comments_thread_version" + end + create_table "learning_runs", force: :cascade do |t| t.integer "repository_id", null: false t.integer "requested_by_id" @@ -472,31 +526,22 @@ t.datetime "updated_at", null: false t.integer "user_id", null: false t.datetime "learning_removal_due_at" - t.string "kind", default: "review", null: false - t.string "local_id" - t.integer "note_revision" - t.string "note_status" - t.text "capture" - t.json "note_thread" t.index ["build_id"], name: "index_review_comments_on_build_id" t.index ["id", "prototype_id"], name: "index_review_comments_on_id_and_prototype", unique: true t.index ["learning_removal_due_at"], name: "index_review_comments_on_learning_removal_due_at", where: "learning_removal_due_at IS NOT NULL" t.index ["prototype_id", "created_at", "id"], name: "index_review_comments_on_prototype_and_created_at", order: { created_at: :desc, id: :desc } - t.index ["prototype_id", "user_id", "local_id"], name: "index_review_comments_on_prototype_id_and_user_id_and_local_id", unique: true t.index ["resolved_by_id"], name: "index_review_comments_on_resolved_by_id" t.index ["root_id"], name: "index_review_comments_on_root_id" t.index ["user_id"], name: "index_review_comments_on_user_id" - t.check_constraint "(kind = 'review' AND local_id IS NULL AND note_revision IS NULL AND note_status IS NULL AND capture IS NULL) OR (kind = 'improvement_note' AND root_id IS NULL AND build_id IS NULL AND local_id IS NOT NULL AND length(local_id) = 36 AND note_revision IS NOT NULL AND note_revision > 0 AND note_status IS NOT NULL AND note_status IN ('open', 'sent', 'ready', 'resolved', 'blocked') AND (capture IS NULL OR length(CAST(capture AS BLOB)) <= 1500000))", name: "review_comments_kind" t.check_constraint "(resolved_at IS NULL AND resolved_by_id IS NULL) OR (root_id IS NULL AND resolved_at IS NOT NULL AND resolved_by_id IS NOT NULL)", name: "review_comments_resolution" t.check_constraint "(root_id IS NULL AND activity_version IS NOT NULL AND activity_version > 0) OR (root_id IS NOT NULL AND activity_version IS NULL)", name: "review_comments_activity_version" - t.check_constraint "(root_id IS NULL AND anchor IS NOT NULL AND (build_id IS NOT NULL OR kind = 'improvement_note')) OR (kind = 'review' AND root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)", name: "review_comments_thread_shape" + t.check_constraint "(root_id IS NULL AND build_id IS NOT NULL AND anchor IS NOT NULL) OR (root_id IS NOT NULL AND build_id IS NULL AND anchor IS NULL)", name: "review_comments_thread_shape" t.check_constraint "CASE WHEN deleted_at IS NULL THEN body IS NOT NULL AND length(trim(body)) > 0 AND length(CAST(body AS BLOB)) <= 10000 ELSE body IS NULL END", name: "review_comments_body" t.check_constraint "anchor IS NULL OR json_type(anchor) = 'object'", name: "review_comments_anchor_object" t.check_constraint "anchor IS NULL OR json_valid(anchor)", name: "review_comments_anchor_json" t.check_constraint "anchor IS NULL OR length(CAST(anchor AS BLOB)) BETWEEN 2 AND 16384", name: "review_comments_anchor_size" t.check_constraint "deleted_at IS NULL OR deleted_at >= created_at", name: "review_comments_deletion_time" t.check_constraint "edited_at IS NULL OR (deleted_at IS NULL AND edited_at >= created_at)", name: "review_comments_edit_state" - t.check_constraint "note_thread IS NULL OR (kind = 'improvement_note' AND CASE WHEN json_valid(note_thread) THEN json_type(note_thread) = 'object' ELSE 0 END AND length(CAST(note_thread AS BLOB)) BETWEEN 2 AND 32768)", name: "review_comments_note_thread" t.check_constraint "root_id IS NULL OR root_id <> id", name: "review_comments_root_not_self" end @@ -716,6 +761,12 @@ add_foreign_key "copilot_connections", "users", on_delete: :cascade add_foreign_key "fresh_authorizations", "repositories", on_delete: :nullify add_foreign_key "fresh_authorizations", "users", on_delete: :cascade + add_foreign_key "improvement_comment_replies", "improvement_comments", column: ["request_id", "prototype_id", "user_id", "thread_key"], primary_key: ["id", "prototype_id", "user_id", "thread_key"], on_delete: :restrict + add_foreign_key "improvement_comment_replies", "improvement_comments", column: ["response_id", "prototype_id", "user_id", "thread_key"], primary_key: ["id", "prototype_id", "user_id", "thread_key"], on_delete: :restrict + add_foreign_key "improvement_comments", "builds", column: ["build_id", "prototype_id"], primary_key: ["id", "prototype_id"], on_delete: :restrict + add_foreign_key "improvement_comments", "improvement_comments", column: ["root_id", "prototype_id", "user_id", "thread_key"], primary_key: ["id", "prototype_id", "user_id", "local_id"], on_delete: :restrict + add_foreign_key "improvement_comments", "prototypes", on_delete: :restrict + add_foreign_key "improvement_comments", "users", on_delete: :restrict add_foreign_key "learning_runs", "repositories", on_delete: :cascade add_foreign_key "learning_runs", "users", column: "requested_by_id", on_delete: :nullify add_foreign_key "notification_resources", "notifications", on_delete: :cascade diff --git a/hub/docs/prototype-canvas.md b/hub/docs/prototype-canvas.md index dab8a09a..b9489391 100644 --- a/hub/docs/prototype-canvas.md +++ b/hub/docs/prototype-canvas.md @@ -2,17 +2,19 @@ ## Improvement-note persistence -Hub provides a persistence API for author-owned Pick & polish notes on a reviewable Prototype branch, without requiring a Build. This server contract does not enable a new canvas interface or local synchronization workflow. +Hub provides a persistence API for author-owned Pick & polish notes tied to a successful Build of a reviewable Prototype branch. Unpublished improvements remain on the author's device until their captured source matches a successful published Build, even if the branch already has an older Build. This server contract does not enable a new canvas interface or local synchronization workflow. + +`ImprovementComment` stores one request or agent response per row, separately from `ReviewComment`. Replies point directly to their original request through `root_id`; an explicit position preserves the client's conversation order across synchronization. `ImprovementCommentReply` links an agent response to each earlier request it answers. Composite foreign keys keep these links inside the same creator's conversation. Hub assembles the existing `thread` payload from these rows rather than storing a JSON conversation or request-ID lists. The root owns the synchronization revision and aggregate status; each request retains its own work status. Paired clients read and save their own notes at `GET` and `POST /api/canvas/repositories/:repository_github_id/prototypes/:prototype_id/improvement_notes`. Every request requires repository read access; writes freshly check write access and share the ordinary comment rate limit. Edit and delete flags require both ownership of an undeleted note and repository write access. Prototype metadata includes the authenticated `viewerId` so clients can identify the note owner. The creator collection includes deletion tombstones and never returns another creator's notes. Creator collections and paired comment lists that include notes return at most 2 MiB of JSON per page, including captures. Clients must pass the opaque `nextCursor` string as the next request's `cursor` query parameter while `hasMore` is true and combine every page before synchronizing local state. The final page returns `nextCursor: null` and `hasMore: false`; every creator page includes `threadVersion`. `pageSize` remains an upper record limit, not a promise that every page contains that many notes. Hub measures stored payload sizes before loading captures and binds creator cursors to both the Prototype and authenticated owner. Cursors do not expire or get consumed, but pages reflect current saved data rather than a collection-wide snapshot. -Each save supplies a stable `local_id`, a positive `revision`, the last synchronized `previous_revision`, `body`, `status`, `deleted`, an `anchor`, and an optional PNG `capture`. Scalar note fields must be strings or null; required values are checked before saving. Exact retries are idempotent. Conflicting content at the same revision, a stale previous revision, a changed target, or resurrection of a deleted note is rejected. A creator can retain at most 200 notes, including tombstones. +Each save supplies a stable `local_id`, a positive `revision`, the last synchronized `previous_revision`, `body`, `status`, `deleted`, an `anchor`, and an optional PNG `capture`. A new note also requires a positive integer `build_number` identifying a successful Build of the same Prototype. Hub returns that immutable identity as `buildNumber`; later updates and deletions retain it rather than following the current checkout. The paired client must verify the captured source against that Build before its first upload, just as for unpublished review comments. Hub checks the Build identity but cannot inspect the author's local working tree. Scalar note fields must be strings or null; required values are checked before saving. Exact retries are idempotent. Conflicting content at the same revision, a stale previous revision, a changed target or Build, or resurrection of a deleted note is rejected. A creator can retain at most 200 notes, including tombstones. The collection advertises `threadVersion: 2`. Notes may retain a nullable `thread` for older clients, or use version 1 or 2 conversation payloads. Threads keep the original body immutable, append author requests and Copilot answers in order, and allow only request-status changes to saved messages. Version 2 supports owner identity snapshots; saved snapshots cannot change or disappear. Snapshots must identify the authenticated owner by immutable user ID, but may retain a historical login from before a rename, including on first synchronization. Login and avatar snapshots are display history, not authorization. Answers reference only preceding requests. A note stays open while any request is open and can resolve only after every request is ready. -Threads are limited to 100 messages and 32,768 UTF-8 bytes. Author requests share the ordinary 10,000-byte body limit; Copilot answers allow 2,000 UTF-16 code units. Clients must preserve existing threads when saving or deleting; older clients receive an upgrade conflict rather than silently dropping history. Deletion clears the body and thread while retaining the tombstone and revision. +Threads are limited to 100 messages and 32,768 UTF-8 bytes. Author requests share the ordinary 10,000-byte body limit; Copilot answers allow 2,000 UTF-16 code units. Clients must preserve existing threads when saving or deleting; older clients receive an upgrade conflict rather than silently dropping history. Deletion clears every message's text and author snapshot while retaining tombstones and the root revision. Ordinary review interfaces, summaries, unread state, notifications, analytics, and learning exclude these notes. Paired Prototype and repository comment lists can explicitly opt in with `improvement_notes=include` or `only`; ordinary comment mutation endpoints cannot edit notes or accept thread payloads. See [Review collaboration](review-collaboration.md) for ordinary conversations. diff --git a/hub/docs/review-collaboration.md b/hub/docs/review-collaboration.md index cb7f7b3f..3e1a6f19 100644 --- a/hub/docs/review-collaboration.md +++ b/hub/docs/review-collaboration.md @@ -2,7 +2,7 @@ Repository readers can discuss specific elements or highlighted regions inside a Prototype preview. Hub keeps those conversations with the Prototype branch so they remain available across Builds. -Author-owned [Pick & polish notes](prototype-canvas.md#improvement-note-persistence) use the same comment database but are separate from review conversations. Ordinary interfaces, summaries, unread state, notifications, and learning exclude them. Authorized paired comment list APIs include them only with an explicit `improvement_notes=include` or `only` filter; the default is `exclude`. The dedicated improvement-note collection returns only the authenticated creator's notes. +Author-owned [Pick & polish notes](prototype-canvas.md#improvement-note-persistence) use separate `ImprovementComment` records, not review conversations. Ordinary interfaces, summaries, unread state, notifications, and learning exclude them. Authorized paired comment list APIs include them only with an explicit `improvement_notes=include` or `only` filter; the default is `exclude`. The dedicated improvement-note collection returns only the authenticated creator's notes. ## Conversations diff --git a/hub/test/integration/canvas_improvement_note_threads_test.rb b/hub/test/integration/canvas_improvement_note_threads_test.rb index db89d27a..056625f7 100644 --- a/hub/test/integration/canvas_improvement_note_threads_test.rb +++ b/hub/test/integration/canvas_improvement_note_threads_test.rb @@ -18,6 +18,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest token_digest: Digest::SHA256.hexdigest(token), expires_at: 7.days.from_now @headers = { "Authorization" => "Bearer #{token}" } @prototype = prototypes.create(repository: @repository) + @build = builds.create_successful(prototype: @prototype) @path = canvas_api_improvement_notes_path(@repository.route_id, @prototype) @comments_path = canvas_api_review_comments_path(@repository.route_id, @prototype) end @@ -26,10 +27,11 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest input = note_input input[:status] = "sent" input[:thread]["originalStatus"] = "sent" - assert_no_difference [ "Build.count", "Notification.count", "ReviewThreadRead.count" ] do + assert_no_difference [ "ReviewComment.count", "Build.count", "Notification.count", "ReviewThreadRead.count", "Analytics::Event.count" ] do post_note(input) assert_response :success assert_equal input[:thread], response.parsed_body.fetch("thread") + assert_equal @build.number, response.parsed_body.fetch("buildNumber") followup = author_message input = next_revision(input) @@ -45,6 +47,9 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest assert_response :success assert_equal "open", response.parsed_body.fetch("status") assert_equal "open", response.parsed_body.fetch("thread").fetch("messages").first.fetch("status") + saved = @prototype.improvement_notes.sole + assert_equal "ready", saved.status + assert_equal "open", saved.note_status input = next_revision(input) input[:status] = "sent" @@ -54,7 +59,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest input = next_revision(input) input[:status] = "ready" input[:thread]["messages"].first["status"] = "ready" - input[:thread]["messages"] << copilot_message(followup["id"]) + input[:thread]["messages"] << copilot_message(input[:local_id], followup["id"]) post_note(input) assert_response :success input = next_revision(input).merge(status: "resolved") @@ -68,8 +73,21 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest assert_equal input[:thread], response.parsed_body.fetch("notes").sole.fetch("thread") assert_equal input[:body], response.parsed_body.fetch("notes").sole.fetch("body") assert_equal 1, @prototype.improvement_notes.count - assert_empty @prototype.improvement_notes.sole.replies - assert_nil @prototype.improvement_notes.sole.build_id + root = @prototype.improvement_notes.sole + assert_equal @build, root.build + messages = root.replies.index_by(&:local_id) + assert_equal input[:thread].fetch("messages").pluck("id").sort, messages.keys.sort + assert messages.values.all? { _1.root_id == root.id && _1.prototype_id == @prototype.id } + assert messages.values.all? { _1.build_id.nil? } + input[:thread].fetch("messages").each do |message| + row = messages.fetch(message.fetch("id")) + assert_equal message.values_at("role", "body", "status"), [ row.role, row.body, row.status ] + next unless message.fetch("role") == "copilot" + + requests = ImprovementCommentReply.where(response_id: row.id).pluck(:request_id) + expected = message.fetch("requestIds").map { _1 == root.local_id ? root.id : messages.fetch(_1).id } + assert_equal expected.sort, requests.sort + end [ @comments_path, canvas_api_repository_review_comments_path(@repository.route_id) ].each do |path| get path, headers: @headers @@ -87,7 +105,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest post_note(input) assert_response :success saved = @prototype.improvement_notes.sole - assert_no_difference "ReviewComment.count" do + assert_no_difference [ "ImprovementComment.count", "ImprovementCommentReply.count" ] do post_note(input) assert_response :success end @@ -96,8 +114,15 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest post_note(changed) assert_response :conflict assert_equal input[:thread], saved.reload.note_thread - post_note(next_revision(changed)) - assert_response :unprocessable_entity + rejected = next_revision(changed) + followup = author_message + rejected[:thread]["messages"].concat([ followup, copilot_message(followup.fetch("id")) ]) + assert_no_difference [ "ImprovementComment.count", "ImprovementCommentReply.count" ] do + post_note(rejected) + assert_response :unprocessable_entity + end + assert_equal 1, saved.reload.note_revision + assert_equal input[:thread], saved.note_thread post_note(next_revision(input).merge(body: "Changed original")) assert_response :unprocessable_entity post_note(next_revision(input).merge(anchor: input[:anchor].merge("selector" => "#different"))) @@ -167,12 +192,12 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest assert_response :success own = @prototype.improvement_notes.sole other = @prototype.improvement_notes.create! \ - user: users.create(github_id: 1002, login: "mona"), local_id: SecureRandom.uuid, + build: @build, user: users.create(github_id: 1002, login: "mona"), local_id: SecureRandom.uuid, body: "Other author's original", note_revision: 1, note_status: "open", anchor: input[:anchor], note_thread: input[:thread] get @path, headers: @headers assert_equal [ own.id ], response.parsed_body.fetch("notes").pluck("id") - assert_no_difference [ "ReviewComment.count", "Notification.count", "ReviewThreadRead.count" ] do + assert_no_difference [ "ReviewComment.count", "ImprovementComment.count", "Notification.count", "ReviewThreadRead.count" ] do [ own, other ].each do |note| patch "#{@comments_path}/#{note.id}", params: { review_comment: { body: "Edited via ordinary API", note_thread: nil } }, @@ -193,7 +218,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest end test "ordinary comments reject thread parameters without mutating their bodies" do - build = builds.create_successful(prototype: @prototype) + build = @build comment = ReviewComment.create! \ prototype: @prototype, build:, user: @viewer, body: "Ordinary comment", anchor: note_input[:anchor] [ :thread, :note_thread ].each do |field| @@ -209,15 +234,19 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest assert_response :bad_request end assert_equal "Ordinary comment", comment.reload.body - assert_nil comment.note_thread assert_empty comment.replies end test "thread aware deletion clears all conversation text and cannot be undone" do input = note_input - input[:thread]["messages"] << copilot_message(input[:local_id]) + followup = author_message + input[:thread]["messages"] << followup + input[:thread]["messages"] << copilot_message(input[:local_id], followup.fetch("id")) post_note(input) assert_response :success + root = @prototype.improvement_notes.sole + conversation_ids = [ root.id, *root.replies.pluck(:id) ] + assert_equal 3, conversation_ids.length deleted = next_revision(input).merge(deleted: true) post_note(deleted) assert_response :success @@ -232,6 +261,9 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest assert_response :conflict get @path, headers: @headers assert_nil response.parsed_body.fetch("notes").sole.fetch("thread") + assert_nil root.reload.body + assert_nil root.note_thread + assert_empty ImprovementComment.where(id: conversation_ids).where.not(body: nil) end test "all note collections paginate complete captures within the serialized response budget" do @@ -253,7 +285,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest (notes.length + 1).times do batches = [] subscriber = ->(event) do - batches << event.payload.fetch(:record_count) if event.payload[:class_name] == "ReviewComment" + batches << event.payload.fetch(:record_count) if event.payload[:class_name] == "ImprovementComment" end ActiveSupport::Notifications.subscribed(subscriber, "instantiation.active_record") do get path, params: filters.merge(cursor:).compact, headers: @headers @@ -353,7 +385,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest end invalid << [ :local_id, [ input[:local_id] ] ] - assert_no_difference "ReviewComment.count" do + assert_no_difference "ImprovementComment.count" do invalid.each do |field, value| post_note(next_revision(input).merge(field => value)) assert_response :bad_request, "#{field} must reject #{value.inspect}" @@ -393,6 +425,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest test "first uploads retain historical owner logins after a rename and freeze both author snapshots" do input = note_input + @viewer.update!(login: "mona_enterprise") author = { "id" => @viewer.id, "login" => @viewer.login, "avatarUrl" => nil } input[:thread].merge!("version" => 2, "originalAuthor" => author) input[:thread]["messages"] << author_message.merge("author" => author.deep_dup) @@ -466,7 +499,7 @@ class CanvasImprovementNoteThreadsTest < ActionDispatch::IntegrationTest def create_note(**attributes) input = note_input @prototype.improvement_notes.create! \ - user: @viewer, local_id: input[:local_id], body: input[:body], note_revision: 1, + build: @build, user: @viewer, local_id: input[:local_id], body: input[:body], note_revision: 1, note_status: "open", anchor: input[:anchor], note_thread: input[:thread], **attributes end @@ -482,13 +515,13 @@ def author_message { "id" => SecureRandom.uuid, "role" => "author", "body" => "New follow-up", "status" => "open" } end - def copilot_message(request_id) - { "id" => SecureRandom.uuid, "role" => "copilot", "body" => "Saved answer", "status" => "ready", "requestIds" => [ request_id ] } + def copilot_message(*request_ids) + { "id" => SecureRandom.uuid, "role" => "copilot", "body" => "Saved answer", "status" => "ready", "requestIds" => request_ids } end def note_input { - local_id: SecureRandom.uuid, revision: 1, previous_revision: 0, + local_id: SecureRandom.uuid, build_number: @build.number, revision: 1, previous_revision: 0, body: "Read-only original", status: "open", deleted: false, capture: "", thread: { "version" => 1, "originalStatus" => "open", "messages" => [] }, anchor: { diff --git a/hub/test/integration/canvas_repository_comments_test.rb b/hub/test/integration/canvas_repository_comments_test.rb index 91b84aeb..4b194389 100644 --- a/hub/test/integration/canvas_repository_comments_test.rb +++ b/hub/test/integration/canvas_repository_comments_test.rb @@ -430,21 +430,19 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest end end - test "improvement notes need no Build and stay out of ordinary reviews and summaries" do + test "published improvements stay separate from ordinary reviews and summaries" do prototype = prototypes.create(repository: @repository) ordinary = create_comment(prototype, 1) - note = ReviewComment.create! \ + note = ImprovementComment.create! \ prototype:, + build: ordinary.build, user: @author, - kind: "improvement_note", local_id: SecureRandom.uuid, note_revision: 1, note_status: "open", body: "Polish the Save button", anchor: ordinary.anchor - assert_nil note.build_id - assert_empty note.thread_reads assert_not_includes prototype.review_comments, note collection = Learning::Collection.new(Learning::Store.new(@repository)).log assert_equal [ ordinary.id ], collection[:calls].sole[:result][:threads].pluck(:id) @@ -459,6 +457,7 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest comments = JSON.parse(response.body).fetch("comments") assert_equal [ note.id ], comments.pluck("id") assert_equal "improvement_note", comments.first.fetch("kind") + assert_equal ordinary.build.number, comments.first.fetch("buildNumber") assert_equal false, comments.first.fetch("viewerCanEdit") get path, params: { improvement_notes: "include" }, headers: @headers assert_equal [ note.id, ordinary.id ], JSON.parse(response.body).fetch("comments").pluck("id") @@ -473,25 +472,41 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest path = canvas_api_improvement_notes_path(@repository.route_id, prototype) input = { local_id: SecureRandom.uuid, revision: 1, previous_revision: 0, body: "Polish this", status: "open", - deleted: false, anchor: ordinary.anchor, capture: "" + build_number: ordinary.build.number, deleted: false, anchor: ordinary.anchor, capture: "" } post path, params: input, headers: @headers, as: :json assert_response :forbidden stub_github_permissions([ repository_payload(2001, "github/prototype") ], { 2001 => "push" }) - assert_no_difference "Notification.count" do - assert_difference "ReviewComment.count", 1 do + queued = builds.create(prototype:, number: 2, commit_sha: "b" * 40) + foreign_prototype = prototypes.create(repository: @repository, branch_name: "prototype/foreign") + foreign = builds.create_successful(prototype: foreign_prototype, number: 3) + unpublished = prototypes.create(repository: @repository, branch_name: "prototype/unpublished") + unpublished_path = canvas_api_improvement_notes_path(@repository.route_id, unpublished) + assert_no_difference "ImprovementComment.count" do + [ input.except(:build_number), input.merge(build_number: queued.number), + input.merge(build_number: foreign.number) ].each do |invalid| + post path, params: invalid, headers: @headers, as: :json + assert_response :unprocessable_entity + end + post unpublished_path, params: input, headers: @headers, as: :json + assert_response :unprocessable_entity + end + assert_no_difference [ "ReviewComment.count", "Notification.count", "ReviewThreadRead.count" ] do + assert_difference "ImprovementComment.count", 1 do post path, params: input, headers: @headers, as: :json assert_response :success end end - assert_no_difference "ReviewComment.count" do + assert_no_difference "ImprovementComment.count" do post path, params: input, headers: @headers, as: :json assert_response :success end post path, params: input.merge(body: "Different same revision"), headers: @headers, as: :json assert_response :conflict - post path, params: input.merge(revision: 2, previous_revision: 1, body: "Edited"), headers: @headers, as: :json + post path, params: input.except(:build_number).merge(revision: 2, previous_revision: 1, body: "Edited"), headers: @headers, as: :json assert_response :success + post path, params: input.merge(revision: 3, previous_revision: 2, build_number: foreign.number), headers: @headers, as: :json + assert_response :conflict post path, params: input.merge(revision: 4, previous_revision: 1, body: "Stale clone with more edits"), headers: @headers, as: :json assert_response :conflict post path, params: input.merge(revision: 3, anchor: input[:anchor].merge("selector" => "#other")), headers: @headers, as: :json @@ -502,8 +517,10 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest assert_equal [ @viewer.login ], JSON.parse(response.body).fetch("notes").map { _1.fetch("author").fetch("login") } note = prototype.improvement_notes.first! assert_equal @viewer, note.user + assert_equal ordinary.build, note.build assert_equal "Edited", note.body prototype.improvement_notes.create! \ + build: ordinary.build, user: @author, local_id: SecureRandom.uuid, body: "Another author's note", @@ -520,10 +537,46 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest assert_response :conflict end + test "mixed pagination retains reviews and notes with the same id and creation time" do + prototype = prototypes.create(repository: @repository) + ordinary = create_comment(prototype, 1) + timestamp = ordinary.created_at.change(usec: 0) + ordinary.update_columns(created_at: timestamp) + note = ImprovementComment.create! \ + id: ordinary.id, prototype:, build: ordinary.build, user: @author, local_id: SecureRandom.uuid, + note_revision: 1, note_status: "open", body: "Polish the Save button", + anchor: ordinary.anchor, created_at: timestamp + + [ @path, canvas_api_review_comments_path(@repository.route_id, prototype) ].each do |path| + received = [] + cursor = nil + 2.times do |index| + get path, params: { pageSize: 1, improvement_notes: "include", cursor: }.compact, headers: @headers + assert_response :success + page = response.parsed_body + assert_equal 1, page.fetch("comments").length + received.concat(page.fetch("comments").map { _1.values_at("kind", "id") }) + assert_equal index.zero?, page.fetch("hasMore") + cursor = page.fetch("nextCursor") + index.zero? ? assert_not_nil(cursor) : assert_nil(cursor) + end + assert_equal [ [ "improvement_note", note.id ], [ "root", ordinary.id ] ], received.sort + + [ [ {}, "root", ordinary.id ], [ { improvement_notes: "only" }, "improvement_note", note.id ] ].each do |filters, kind, id| + get path, params: filters.merge(pageSize: 1), headers: @headers + assert_response :success + page = response.parsed_body + assert_equal [ [ kind, id ] ], page.fetch("comments").map { _1.values_at("kind", "id") } + assert_equal false, page.fetch("hasMore") + assert_nil page.fetch("nextCursor") + end + end + end + test "note writes reject oversized bodies before JSON parsing or repository authorization" do prototype = prototypes.create(repository: @repository) path = canvas_api_improvement_notes_path(@repository.route_id, prototype) - assert_no_difference "ReviewComment.count" do + assert_no_difference [ "ReviewComment.count", "ImprovementComment.count" ] do post path, params: "{" * 1_600_001, headers: @headers.merge("CONTENT_TYPE" => "application/json") assert_response :content_too_large @@ -536,7 +589,7 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest path = canvas_api_improvement_notes_path(@repository.route_id, prototype) input = { local_id: SecureRandom.uuid, revision: 1, previous_revision: 0, body: "Polish this", - status: "open", deleted: false, anchor: ordinary.anchor, capture: "" + build_number: ordinary.build.number, status: "open", deleted: false, anchor: ordinary.anchor, capture: "" } stub_github_permissions([ repository_payload(2001, "github/prototype") ], { 2001 => "push" }) store = ActiveSupport::Cache::MemoryStore.new @@ -546,7 +599,7 @@ class CanvasRepositoryCommentsTest < ActionDispatch::IntegrationTest post path, params: input, headers: @headers, as: :json assert_response :success end - assert_no_difference "ReviewComment.count" do + assert_no_difference "ImprovementComment.count" do post path, params: input.merge(local_id: SecureRandom.uuid), headers: @headers, as: :json assert_response :too_many_requests assert_equal "60", response.headers["Retry-After"] diff --git a/hub/test/models/improvement_comment_test.rb b/hub/test/models/improvement_comment_test.rb new file mode 100644 index 00000000..5ee7da1f --- /dev/null +++ b/hub/test/models/improvement_comment_test.rb @@ -0,0 +1,51 @@ +require "test_helper" + +class ImprovementCommentTest < ActiveSupport::TestCase + test "the database rejects duplicate message identities within an owner's prototype" do + root = create_note + duplicate = root.replies.first.dup + duplicate.position = root.replies.maximum(:position) + 1 + + assert_raises(ActiveRecord::StatementInvalid) { duplicate.save!(validate: false) } + assert_equal 2, root.replies.count + end + + test "the database keeps answer links unique and inside their conversation" do + root = create_note + other = create_note(prototype: root.prototype, user: root.user) + response = root.replies.find_by!(role: "copilot") + link = ImprovementCommentReply.find_by!(response_id: response.id) + + assert_raises(ActiveRecord::StatementInvalid) { link.dup.save!(validate: false) } + assert_raises(ActiveRecord::StatementInvalid) { link.update_columns(request_id: other.id) } + assert_equal root.id, link.reload.request_id + end + + private + + def create_note(prototype: prototypes.create, user: users.create) + local_id = SecureRandom.uuid + build = prototype.builds.first || builds.create_successful(prototype:) + ImprovementComment.create! \ + prototype:, build:, user:, local_id:, note_revision: 1, note_status: "open", + body: "Original request", anchor: valid_anchor, + note_thread: { + "version" => 1, "originalStatus" => "open", + "messages" => [ + { "id" => SecureRandom.uuid, "role" => "author", "body" => "Follow-up request", "status" => "open" }, + { "id" => SecureRandom.uuid, "role" => "copilot", "body" => "Saved answer", "status" => "ready", "requestIds" => [ local_id ] } + ] + } + end + + def valid_anchor + { + "schemaVersion" => 1, "kind" => "element", + "route" => { "pathname" => "/", "search" => "", "hash" => "" }, + "selector" => "#save", "accessibleName" => "Save", "tagName" => "button", "selectedText" => "", + "rect" => { "x" => 20, "y" => 30, "width" => 100, "height" => 32 }, + "viewport" => { "width" => 1280, "height" => 720, "scrollX" => 0, "scrollY" => 0 }, + "elementOffset" => { "x" => 0.5, "y" => 0.5 } + } + end +end diff --git a/hub/test/models/learning/run_test.rb b/hub/test/models/learning/run_test.rb index 12817e61..c7ebcfc9 100644 --- a/hub/test/models/learning/run_test.rb +++ b/hub/test/models/learning/run_test.rb @@ -56,10 +56,10 @@ class Learning::RunTest < ActiveSupport::TestCase comment = learning_comment(@repository, @mona, "prototype/signup", "The field needs a label.") Learning::Run.request!(repository: @repository, user: @admin).find_learnings! travel 1.minute - ReviewComment.create! \ + ImprovementComment.create! \ prototype: comment.prototype, + build: comment.build, user: @mona, - kind: "improvement_note", local_id: SecureRandom.uuid, note_revision: 1, note_status: "open", diff --git a/hub/test/models/review_comment_note_thread_test.rb b/hub/test/models/review_comment_note_thread_test.rb deleted file mode 100644 index cc6a25a7..00000000 --- a/hub/test/models/review_comment_note_thread_test.rb +++ /dev/null @@ -1,291 +0,0 @@ -require "test_helper" - -class ReviewCommentNoteThreadTest < ActiveSupport::TestCase - test "allows owner attribution of unknown legacy authors without changing saved request content" do - note = build_note - message = author_message - note.note_thread = thread_payload(messages: [ message ]) - note.save! - author = { "id" => note.user_id, "login" => note.user.login, "avatarUrl" => nil } - note.note_thread["version"] = 2 - note.note_thread["originalAuthor"] = author - note.note_thread["messages"][0]["author"] = author - note.save! - assert_equal message["body"], note.reload.note_thread["messages"][0]["body"] - note.note_thread["messages"][0]["body"] = "Changed original" - assert_not note.save - note.reload - note.note_thread["messages"][0].delete("author") - assert_not note.save - end - - test "keeps version two author snapshots immutable and rejects foreign attribution" do - note = build_note - author = { "id" => note.user_id, "login" => note.user.login, "avatarUrl" => "https://avatars.githubusercontent.com/u/42" } - note.note_thread = thread_payload.merge("version" => 2, "originalAuthor" => author) - note.save! - note.note_thread["originalAuthor"]["avatarUrl"] = "https://avatars.githubusercontent.com/u/43" - assert_not note.save - note.reload - note.note_thread["messages"] << author_message.merge("author" => author) - note.save! - note.note_thread["messages"][0]["author"]["id"] += 1 - assert_not note.save - note.reload - note.note_thread["version"] = 1 - note.note_thread.delete("originalAuthor") - assert_not note.save - foreign = build_note(prototype: note.prototype, user: note.user) - foreign.note_thread = thread_payload.merge("version" => 2, "originalAuthor" => author.merge("id" => note.user_id + 1)) - assert_not foreign.valid? - end - - test "version two snapshots accept Enterprise Managed User logins but reject foreign identities" do - owner = users.create(login: "mona_enterprise") - note = build_note(user: owner) - author = { "id" => owner.id, "login" => owner.login, "avatarUrl" => nil } - note.note_thread = thread_payload(messages: [ author_message.merge("author" => author.deep_dup) ]) - .merge("version" => 2, "originalAuthor" => author) - note.save! - assert_equal author, note.reload.note_thread.fetch("originalAuthor") - assert_equal author, note.note_thread.fetch("messages").sole.fetch("author") - - [ "original", "followup" ].each do |position| - candidate = build_note(user: owner, prototype: note.prototype) - candidate.note_thread = thread_payload(messages: [ author_message.merge("author" => author.deep_dup) ]) - .merge("version" => 2, "originalAuthor" => author.deep_dup) - invalid_author = author.merge("id" => owner.id + 1) - if position == "original" - candidate.note_thread["originalAuthor"] = invalid_author - else - candidate.note_thread["messages"].first["author"] = invalid_author - end - assert_not candidate.valid?, "#{position} must reject #{invalid_author.inspect}" - assert_not_empty candidate.errors[:note_thread] - end - end - - test "legacy notes stay standalone while threaded notes derive status from all requests" do - note = build_note - assert note.valid? - assert_nil note.note_thread - note.note_thread = thread_payload - - %w[open sent blocked ready].each do |status| - note.note_thread["originalStatus"] = status - note.note_status = status - assert note.valid?, note.errors.full_messages.inspect - end - note.note_status = "resolved" - assert note.valid? - note.note_thread["originalStatus"] = "open" - assert_not note.valid? - note.note_thread["originalStatus"] = "resolved" - assert_not note.valid? - - %w[open sent blocked ready].repeated_permutation(2) do |original, followup| - note.note_thread = thread_payload(original:, messages: [ author_message(status: followup) ]) - expected = %w[open sent blocked ready].find { [ original, followup ].include?(_1) } - note.note_status = expected - assert note.valid?, "#{original}/#{followup}: #{note.errors.full_messages}" - note.note_status = (expected == "ready" ? "blocked" : "ready") - assert_not note.valid? - end - end - - test "rejects malformed shapes statuses identities and request references" do - note = build_note - author = author_message - answer = copilot_message(note.local_id) - invalid_threads = [ - [], "text", 1, {}, - thread_payload.merge("version" => 1.0), - thread_payload.merge("version" => 3), - thread_payload.merge("originalStatus" => "resolved"), - thread_payload.merge("author" => "fabricated"), - thread_payload.merge("messages" => {}), - thread_payload(messages: [ nil ]), - thread_payload(messages: [ author.merge("role" => "user") ]), - thread_payload(messages: [ author.merge("id" => "not-a-uuid") ]), - thread_payload(messages: [ author.merge("id" => note.local_id) ]), - thread_payload(messages: [ author, author ]), - thread_payload(messages: [ author.merge("body" => " \n") ]), - thread_payload(messages: [ author.merge("body" => 123) ]), - thread_payload(messages: [ author.merge("status" => "resolved") ]), - thread_payload(messages: [ author.merge("createdAt" => Time.current.iso8601) ]), - thread_payload(messages: [ answer.merge("status" => "sent") ]), - thread_payload(messages: [ answer.merge("body" => "") ]), - thread_payload(messages: [ answer.except("requestIds") ]), - thread_payload(messages: [ answer.merge("requestIds" => []) ]), - thread_payload(messages: [ answer.merge("requestIds" => [ note.local_id, note.local_id ]) ]), - thread_payload(messages: [ answer.merge("requestIds" => [ SecureRandom.uuid ]) ]), - thread_payload(messages: [ copilot_message(author["id"]), author ]), - thread_payload(messages: [ answer, copilot_message(answer["id"]) ]) - ] - invalid_threads.each do |thread| - note.note_thread = thread - assert_not note.valid?, thread.inspect - assert note.errors[:note_thread].any?, note.errors.full_messages.inspect - end - note.note_thread = thread_payload(messages: [ author, copilot_message(note.local_id, author["id"]) ]) - assert note.valid?, note.errors.full_messages.inspect - end - - test "enforces UTF8 author bytes and JavaScript UTF16 answer length" do - note = build_note - author = author_message(body: "é" * 5_000) - answer = copilot_message(note.local_id).merge("body" => "😀" * 1_000) - note.note_thread = thread_payload(messages: [ author, answer ]) - assert note.valid?, note.errors.full_messages.inspect - note.note_thread["messages"][0]["body"] += "é" - assert_not note.valid? - note.note_thread["messages"][0]["body"] = "Valid" - note.note_thread["messages"][1]["body"] += "a" - assert_not note.valid? - note.note_thread["messages"][1]["body"] = "a" * 2_000 - assert note.valid?, note.errors.full_messages.inspect - note.note_thread["messages"][1]["body"] += "a" - assert_not note.valid? - note.note_thread["messages"][1]["body"] = "\xFF".dup.force_encoding(Encoding::UTF_8) - assert_not note.valid? - assert_includes note.errors[:note_thread], "must contain valid UTF-8 text" - end - - test "bounds message count and complete serialized UTF8 thread size" do - note = build_note - note.note_thread = thread_payload(messages: 100.times.map { author_message }) - assert note.valid?, note.errors.full_messages.inspect - note.note_thread["messages"] << author_message - assert_not note.valid? - - note.note_thread = thread_payload(messages: 4.times.map { author_message(body: "<" * 7_500) }) - remaining = 32_768 - JSON.generate(note.note_thread).bytesize - note.note_thread["messages"][0]["body"] += "a" * remaining - assert_equal 32_768, JSON.generate(note.note_thread).bytesize - assert note.valid?, note.errors.full_messages.inspect - note.note_thread["messages"][0]["body"] += "é" - assert_not note.valid? - note.note_thread["messages"][0]["body"].delete_suffix!("é") - note.save! - assert_equal 32_768, JSON.generate(note.reload.note_thread).bytesize - end - - test "legacy backfill preserves the original while unthreaded edits and deletion remain valid" do - note = build_note - note.save! - note.update!(body: "Legacy client edit") - note.assign_attributes(body: "Changed during backfill", note_thread: thread_payload) - assert_not note.save - assert_includes note.errors[:body], "cannot change when saving a thread" - assert_equal "Legacy client edit", note.reload.body - assert_nil note.note_thread - note.update!(note_thread: thread_payload) - assert_equal "Legacy client edit", note.reload.body - - deleted = build_note(prototype: note.prototype, user: note.user) - deleted.save! - deleted.update!(body: nil, deleted_at: Time.current, note_thread: thread_payload) - assert_nil deleted.reload.body - assert_nil deleted.note_thread - end - - test "saved original and history are immutable except author status and appended messages" do - note = build_note - author = author_message - note.note_thread = thread_payload(messages: [ author, copilot_message(note.local_id, author["id"]) ]) - note.save! - original = note.note_thread.deep_dup - changes = [ - ->(thread) { thread["messages"].pop }, - ->(thread) { thread["messages"].reverse! }, - ->(thread) { thread["messages"][0]["id"] = SecureRandom.uuid }, - ->(thread) { thread["messages"][0]["body"] = "Edited" }, - ->(thread) { thread["messages"][0]["role"] = "copilot" }, - ->(thread) { thread["messages"][1]["body"] = "Edited answer" }, - ->(thread) { thread["messages"][1]["status"] = "blocked" }, - ->(thread) { thread["messages"][1]["requestIds"] = [ note.local_id ] } - ] - changes.each do |change| - note.note_thread = original.deep_dup - change.call(note.note_thread) - assert_not note.save - assert_equal original, note.reload.note_thread - end - note.note_thread = nil - assert_not note.save - note.reload.body = "Edited original" - assert_not note.save - - note.reload - note.note_thread["originalStatus"] = "sent" - note.note_thread["messages"][0]["status"] = "blocked" - note.note_status = "sent" - note.note_thread["messages"] << author_message(status: "ready") - assert note.save, note.errors.full_messages.inspect - end - - test "ordinary comments reject threads in model and database and notes cannot get review replies" do - build = builds.create_successful - ordinary = ReviewComment.create! \ - prototype: build.prototype, build:, user: users.create, body: "Review", anchor: valid_anchor - ordinary.note_thread = thread_payload - assert_not ordinary.save - assert_raises(ActiveRecord::StatementInvalid) { ordinary.update_columns(note_thread: thread_payload) } - - note = build_note(prototype: build.prototype, user: ordinary.user) - note.note_thread = thread_payload - note.save! - [ [], "text", { "padding" => "a" * 32_768 } ].each do |thread| - assert_raises(ActiveRecord::StatementInvalid) { note.update_columns(note_thread: thread) } - end - assert_no_difference [ "ReviewComment.count", "Notification.count", "ReviewThreadRead.count" ] do - assert_raises(ActiveRecord::RecordInvalid) do - note.thread.create_reply!(user: note.user, body: "Forbidden ordinary reply") - end - end - end - - test "deletion clears conversation text and cannot restore the original" do - note = build_note - note.note_thread = thread_payload(messages: [ author_message, copilot_message(note.local_id) ]) - note.save! - note.update!(body: nil, deleted_at: Time.current) - assert_nil note.reload.note_thread - assert_nil note.body - note.assign_attributes(body: "Restored", deleted_at: nil, note_thread: thread_payload) - assert_not note.save - end - - private - - def build_note(**attributes) - ReviewComment.new \ - prototype: attributes.fetch(:prototype) { prototypes.create }, - user: attributes.fetch(:user) { users.create }, - kind: "improvement_note", local_id: SecureRandom.uuid, note_revision: 1, - note_status: "open", body: "Original request", anchor: valid_anchor - end - - def thread_payload(original: "open", messages: []) - { "version" => 1, "originalStatus" => original, "messages" => messages } - end - - def author_message(status: "open", body: "Follow-up request") - { "id" => SecureRandom.uuid, "role" => "author", "body" => body, "status" => status } - end - - def copilot_message(*request_ids) - { "id" => SecureRandom.uuid, "role" => "copilot", "body" => "Prior answer", "status" => "ready", "requestIds" => request_ids } - end - - def valid_anchor - { - "schemaVersion" => 1, "kind" => "element", - "route" => { "pathname" => "/", "search" => "", "hash" => "" }, - "selector" => "#save", "accessibleName" => "Save", "tagName" => "button", "selectedText" => "", - "rect" => { "x" => 20, "y" => 30, "width" => 100, "height" => 32 }, - "viewport" => { "width" => 1280, "height" => 720, "scrollX" => 0, "scrollY" => 0 }, - "elementOffset" => { "x" => 0.5, "y" => 0.5 } - } - end -end