diff --git a/db/migrate/20260527202401_add_deleted_at_to_coplan_comments.co_plan.rb b/db/migrate/20260527202401_add_deleted_at_to_coplan_comments.co_plan.rb new file mode 100644 index 00000000..413e7ad5 --- /dev/null +++ b/db/migrate/20260527202401_add_deleted_at_to_coplan_comments.co_plan.rb @@ -0,0 +1,6 @@ +# This migration comes from co_plan (originally 20260527000000) +class AddDeletedAtToCoplanComments < ActiveRecord::Migration[8.1] + def change + add_column :coplan_comments, :deleted_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index 4d8f5344..4df44811 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -73,6 +73,7 @@ t.text "body_markdown", null: false t.string "comment_thread_id", limit: 36, null: false t.datetime "created_at", null: false + t.datetime "deleted_at" t.datetime "updated_at", null: false t.index ["comment_thread_id", "created_at"], name: "index_coplan_comments_on_comment_thread_id_and_created_at" end diff --git a/engine/app/controllers/coplan/api/v1/comments_controller.rb b/engine/app/controllers/coplan/api/v1/comments_controller.rb index 61dc4287..87611b1c 100644 --- a/engine/app/controllers/coplan/api/v1/comments_controller.rb +++ b/engine/app/controllers/coplan/api/v1/comments_controller.rb @@ -85,6 +85,41 @@ def discard render json: { thread_id: thread.id, status: thread.status } end + def destroy + # Scope the lookup to this plan's comments so an ID from another + # plan returns 404 rather than being acted on. (The policy also + # gates on authorship, but scoping here keeps the resource + # boundary explicit and the 404 correct.) + comment = @plan.comments.find_by(id: params[:id]) + + unless comment + render json: { error: "Comment not found" }, status: :not_found + return + end + + policy = CommentPolicy.new(current_user, comment) + unless policy.delete? + render json: { error: "Not authorized" }, status: :forbidden + return + end + + Comments::SoftDelete.call(comment: comment, actor: current_user) + + thread = comment.comment_thread + if thread.reload.empty? + Broadcaster.remove_to(@plan, target: ActionView::RecordIdentifier.dom_id(thread)) + else + Broadcaster.replace_to( + @plan, + target: ActionView::RecordIdentifier.dom_id(comment), + partial: "coplan/comments/comment", + locals: { comment: comment } + ) + end + + render json: { comment_id: comment.id, deleted_at: comment.deleted_at } + end + def reply thread = @plan.comment_threads.find_by(id: params[:id]) unless thread diff --git a/engine/app/controllers/coplan/comments_controller.rb b/engine/app/controllers/coplan/comments_controller.rb index d8f71529..b29df543 100644 --- a/engine/app/controllers/coplan/comments_controller.rb +++ b/engine/app/controllers/coplan/comments_controller.rb @@ -34,6 +34,32 @@ def create end end + def destroy + comment = @thread.comments.find(params[:id]) + policy = CommentPolicy.new(current_user, comment) + unless policy.delete? + redirect_to plan_path(@plan), alert: "Not authorized to delete this comment." and return + end + + Comments::SoftDelete.call(comment: comment, actor: current_user) + + if @thread.reload.empty? + Broadcaster.remove_to(@plan, target: ActionView::RecordIdentifier.dom_id(@thread)) + else + Broadcaster.replace_to( + @plan, + target: ActionView::RecordIdentifier.dom_id(comment), + partial: "coplan/comments/comment", + locals: { comment: comment } + ) + end + + respond_to do |format| + format.turbo_stream { render turbo_stream: [] } + format.html { redirect_to plan_path(@plan), notice: "Comment deleted." } + end + end + private def set_plan diff --git a/engine/app/controllers/coplan/plans_controller.rb b/engine/app/controllers/coplan/plans_controller.rb index 10498ce7..48ec8648 100644 --- a/engine/app/controllers/coplan/plans_controller.rb +++ b/engine/app/controllers/coplan/plans_controller.rb @@ -58,7 +58,7 @@ def index def show authorize!(@plan, :show?) - @threads = @plan.comment_threads.includes(:comments, :created_by_user).order(:created_at) + @threads = @plan.comment_threads.with_kept_comments.includes(:comments, :created_by_user).order(:created_at) @references = @plan.references.order(reference_type: :asc, created_at: :desc) PlanViewer.track(plan: @plan, user: current_user) end diff --git a/engine/app/helpers/coplan/plan_events_helper.rb b/engine/app/helpers/coplan/plan_events_helper.rb index 5ebc520e..4d5210f7 100644 --- a/engine/app/helpers/coplan/plan_events_helper.rb +++ b/engine/app/helpers/coplan/plan_events_helper.rb @@ -42,6 +42,13 @@ def render_event_summary(event) url = event.before_value.to_s label = title || url safe_join(["Removed reference ", content_tag(:span, label, class: "history-split__event-link")]) + when "comment_deleted" + preview = event.metadata.is_a?(Hash) ? event.metadata["body_preview"].to_s.presence : nil + if preview + safe_join(["Deleted comment: ", content_tag(:em, preview)]) + else + "Deleted comment" + end else # Fallback for unknown / future event types — still useful, never blank. "#{event.event_type}: #{event.before_value || "—"} → #{event.after_value || "—"}" diff --git a/engine/app/javascript/controllers/coplan/comment_actions_controller.js b/engine/app/javascript/controllers/coplan/comment_actions_controller.js new file mode 100644 index 00000000..c725a122 --- /dev/null +++ b/engine/app/javascript/controllers/coplan/comment_actions_controller.js @@ -0,0 +1,21 @@ +import { Controller } from "@hotwired/stimulus" + +// Reveals per-viewer actions (currently just Delete) when this comment +// belongs to the signed-in user. Broadcasts render once for all viewers +// with no current_user, so the server emits the affordance for every +// human comment and lets each browser decide whether to show it. The +// server still enforces auth on submit — this is UX, not security. +export default class extends Controller { + static values = { authorId: String, authorType: String } + static targets = ["delete"] + + connect() { + const me = document.querySelector("meta[name='coplan-current-user-id']")?.content + const isMine = this.authorTypeValue === "human" && + !!me && + this.authorIdValue === me + if (isMine && this.hasDeleteTarget) { + this.deleteTarget.hidden = false + } + } +} diff --git a/engine/app/models/coplan/comment.rb b/engine/app/models/coplan/comment.rb index 156546e4..99457ee7 100644 --- a/engine/app/models/coplan/comment.rb +++ b/engine/app/models/coplan/comment.rb @@ -14,12 +14,22 @@ class Comment < ApplicationRecord after_create_commit :track_comment_created # Runs on save (not just create) so adding a mention via edit also # notifies. ProcessMentions uses find_or_create_by to dedupe. - after_save_commit :process_mentions, if: :saved_change_to_body_markdown? + after_save_commit :process_mentions, if: -> { saved_change_to_body_markdown? && !deleted? } + + scope :kept, -> { where(deleted_at: nil) } def agent? agent_name.present? || author_type.in?(%w[local_agent cloud_persona]) end + def deleted? + deleted_at.present? + end + + def soft_delete! + update!(deleted_at: Time.current) + end + # Resolves the comment author to a CoPlan::User instance, or nil for # author types that don't map to a user (cloud_persona, system). def author diff --git a/engine/app/models/coplan/comment_thread.rb b/engine/app/models/coplan/comment_thread.rb index 7ca40d11..9cc0d7a8 100644 --- a/engine/app/models/coplan/comment_thread.rb +++ b/engine/app/models/coplan/comment_thread.rb @@ -23,6 +23,16 @@ class CommentThread < ApplicationRecord scope :current, -> { where(out_of_date: false) } scope :active, -> { where(status: OPEN_STATUSES, out_of_date: false) } scope :archived, -> { where("status NOT IN (?) OR out_of_date = ?", OPEN_STATUSES, true) } + # Threads with at least one non-deleted comment. A thread whose only + # comments have all been soft-deleted is effectively gone — hide it + # from the doc view rather than leaving a dangling anchor + popover. + scope :with_kept_comments, -> { + where(id: CoPlan::Comment.kept.select(:comment_thread_id)) + } + + def empty? + comments.kept.none? + end # Transforms anchor positions through intervening version edits using OT. # Threads without positional data (anchor_start/anchor_end/anchor_revision) diff --git a/engine/app/models/coplan/plan.rb b/engine/app/models/coplan/plan.rb index 205feb93..9b7ba952 100644 --- a/engine/app/models/coplan/plan.rb +++ b/engine/app/models/coplan/plan.rb @@ -26,6 +26,7 @@ class Plan < ApplicationRecord has_many :plan_collaborators, dependent: :destroy has_many :collaborators, through: :plan_collaborators, source: :user has_many :comment_threads, dependent: :destroy + has_many :comments, through: :comment_threads has_many :edit_sessions, dependent: :destroy has_one :edit_lease, dependent: :destroy has_many :plan_tags, dependent: :destroy diff --git a/engine/app/models/coplan/plan_event.rb b/engine/app/models/coplan/plan_event.rb index 648cec51..48bf7f7e 100644 --- a/engine/app/models/coplan/plan_event.rb +++ b/engine/app/models/coplan/plan_event.rb @@ -23,6 +23,7 @@ class PlanEvent < ApplicationRecord tag_removed reference_added reference_removed + comment_deleted ].freeze belongs_to :plan diff --git a/engine/app/policies/coplan/comment_policy.rb b/engine/app/policies/coplan/comment_policy.rb new file mode 100644 index 00000000..e408a4d5 --- /dev/null +++ b/engine/app/policies/coplan/comment_policy.rb @@ -0,0 +1,7 @@ +module CoPlan + class CommentPolicy < ApplicationPolicy + def delete? + record.author_type == "human" && record.author_id == user&.id + end + end +end diff --git a/engine/app/services/coplan/comments/soft_delete.rb b/engine/app/services/coplan/comments/soft_delete.rb new file mode 100644 index 00000000..a75fedef --- /dev/null +++ b/engine/app/services/coplan/comments/soft_delete.rb @@ -0,0 +1,40 @@ +module CoPlan + module Comments + # Soft-deletes a comment and records the event in the plan history feed + # in a single transaction. Idempotent — calling on an already-deleted + # comment is a no-op so retries or double-clicks don't write duplicate + # history entries. + class SoftDelete + BODY_PREVIEW_LENGTH = 120 + + def self.call(**kwargs) + new(**kwargs).call + end + + def initialize(comment:, actor:) + @comment = comment + @actor = actor + end + + def call + return @comment if @comment.deleted? + + ActiveRecord::Base.transaction do + @comment.soft_delete! + Plans::LogEvent.call( + plan: @comment.comment_thread.plan, + actor: @actor, + event_type: "comment_deleted", + metadata: { + comment_id: @comment.id, + thread_id: @comment.comment_thread_id, + body_preview: @comment.body_markdown.to_s.truncate(BODY_PREVIEW_LENGTH) + } + ) + end + + @comment + end + end + end +end diff --git a/engine/app/services/coplan/plans/log_event.rb b/engine/app/services/coplan/plans/log_event.rb index 5d551ac7..c975488f 100644 --- a/engine/app/services/coplan/plans/log_event.rb +++ b/engine/app/services/coplan/plans/log_event.rb @@ -80,6 +80,7 @@ def default_field_for(event_type) when "plan_type_changed" then "plan_type" when "tag_added", "tag_removed" then "tags" when "reference_added", "reference_removed" then "references" + when "comment_deleted" then "comments" end end diff --git a/engine/app/views/coplan/comments/_comment.html.erb b/engine/app/views/coplan/comments/_comment.html.erb index 5e35fd80..e5a27b8e 100644 --- a/engine/app/views/coplan/comments/_comment.html.erb +++ b/engine/app/views/coplan/comments/_comment.html.erb @@ -1,18 +1,41 @@ -
-
- <% author = comment_author_user(comment) %> - <% if author %> - <%= user_avatar(author) %> +<%# Per-viewer affordances (Delete) are hidden client-side by the + comment_actions Stimulus controller — broadcasts render once with no + `current_user`, so we ship the button on every human comment and let + the browser remove it for non-authors. Server still enforces auth. %> +
+ <% if comment.deleted? %> +
+ Comment deleted +
+ <% else %> +
+ <% author = comment_author_user(comment) %> + <% if author %> + <%= user_avatar(author) %> + <% end %> + <%= comment_author_name(comment) %> + <% if comment.agent? %> + agent + <% end %> + · <%= time_ago_in_words(comment.created_at) %> ago +
+
+ <%= cache(comment) do %> + <%= render_markdown(comment.body_markdown) %> + <% end %> +
+ <% if comment.author_type == "human" %> + <% end %> - <%= comment_author_name(comment) %> - <% if comment.agent? %> - agent - <% end %> - · <%= time_ago_in_words(comment.created_at) %> ago -
-
- <%= cache(comment) do %> - <%= render_markdown(comment.body_markdown) %> - <% end %> -
+ <% end %>
diff --git a/engine/app/views/layouts/coplan/application.html.erb b/engine/app/views/layouts/coplan/application.html.erb index 1bed9ccc..f0d3ec14 100644 --- a/engine/app/views/layouts/coplan/application.html.erb +++ b/engine/app/views/layouts/coplan/application.html.erb @@ -6,6 +6,9 @@ <%= csrf_meta_tags %> <%= csp_meta_tag %> + <% if signed_in? %> + + <% end %> <% if CoPlan.configuration.web_push_configured? %> diff --git a/engine/config/routes.rb b/engine/config/routes.rb index d99b94b3..31e713f7 100644 --- a/engine/config/routes.rb +++ b/engine/config/routes.rb @@ -14,7 +14,7 @@ patch :discard patch :reopen end - resources :comments, only: [:create] + resources :comments, only: [:create, :destroy] end end @@ -42,6 +42,9 @@ patch :resolve, on: :member patch :discard, on: :member end + # Deletes an individual comment (by comment ID, not thread ID). + # Distinct from the routes above, which key off thread ID. + delete "comments/:id/delete", to: "comments#destroy", as: :destroy_comment resources :references, only: [:index, :create, :destroy] end resources :references, only: [] do diff --git a/engine/db/migrate/20260527000000_add_deleted_at_to_coplan_comments.rb b/engine/db/migrate/20260527000000_add_deleted_at_to_coplan_comments.rb new file mode 100644 index 00000000..9f283fb8 --- /dev/null +++ b/engine/db/migrate/20260527000000_add_deleted_at_to_coplan_comments.rb @@ -0,0 +1,5 @@ +class AddDeletedAtToCoplanComments < ActiveRecord::Migration[8.1] + def change + add_column :coplan_comments, :deleted_at, :datetime + end +end diff --git a/spec/models/comment_spec.rb b/spec/models/comment_spec.rb index 0221a31f..f0d2380b 100644 --- a/spec/models/comment_spec.rb +++ b/spec/models/comment_spec.rb @@ -41,4 +41,28 @@ }.not_to have_enqueued_job(CoPlan::NotificationJob) end end + + describe "soft delete" do + let(:comment) { create(:comment) } + + it "soft_delete! sets deleted_at" do + expect { comment.soft_delete! }.to change { comment.reload.deleted_at }.from(nil) + expect(comment.deleted?).to be(true) + end + + it "kept scope excludes deleted comments" do + kept = create(:comment) + deleted = create(:comment) + deleted.soft_delete! + + expect(CoPlan::Comment.kept).to include(kept) + expect(CoPlan::Comment.kept).not_to include(deleted) + end + + it "does not re-fire ProcessMentions when soft-deleting" do + comment # create before block, so ProcessMentions runs once on creation + expect(CoPlan::Comments::ProcessMentions).not_to receive(:call) + comment.soft_delete! + end + end end diff --git a/spec/models/comment_thread_spec.rb b/spec/models/comment_thread_spec.rb index dda729d3..82355c7c 100644 --- a/spec/models/comment_thread_spec.rb +++ b/spec/models/comment_thread_spec.rb @@ -9,6 +9,28 @@ expect(thread_record).to be_valid end + describe "kept comments lifecycle" do + it "with_kept_comments excludes threads whose only comment is soft-deleted" do + with_live = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: user) + create(:comment, comment_thread: with_live) + + all_deleted = create(:comment_thread, plan: plan, plan_version: plan.current_plan_version, created_by_user: user) + c = create(:comment, comment_thread: all_deleted) + c.soft_delete! + + result = CoPlan::CommentThread.with_kept_comments + expect(result).to include(with_live) + expect(result).not_to include(all_deleted) + end + + it "#empty? is true when all comments are soft-deleted, false otherwise" do + c = create(:comment, comment_thread: thread_record) + expect(thread_record).not_to be_empty + c.soft_delete! + expect(thread_record.reload).to be_empty + end + end + it "validates status inclusion" do thread_record.status = "invalid" expect(thread_record).not_to be_valid diff --git a/spec/policies/coplan/comment_policy_spec.rb b/spec/policies/coplan/comment_policy_spec.rb new file mode 100644 index 00000000..ba07cd37 --- /dev/null +++ b/spec/policies/coplan/comment_policy_spec.rb @@ -0,0 +1,37 @@ +require "rails_helper" + +RSpec.describe CoPlan::CommentPolicy do + let(:author) { create(:coplan_user) } + let(:other_user) { create(:coplan_user) } + let(:plan_author) { create(:coplan_user) } + let(:plan) { create(:plan, created_by_user: plan_author) } + let(:thread) { create(:comment_thread, plan: plan, created_by_user: author) } + + describe "#delete?" do + it "allows the human author to delete their own comment" do + comment = create(:comment, comment_thread: thread, author_type: "human", author_id: author.id) + expect(described_class.new(author, comment).delete?).to be(true) + end + + it "forbids another user from deleting someone else's comment" do + comment = create(:comment, comment_thread: thread, author_type: "human", author_id: author.id) + expect(described_class.new(other_user, comment).delete?).to be(false) + end + + it "forbids the plan author from deleting comments they did not write" do + comment = create(:comment, comment_thread: thread, author_type: "human", author_id: author.id) + expect(described_class.new(plan_author, comment).delete?).to be(false) + end + + it "forbids deleting agent comments even when caller matches author_id" do + token = create(:api_token, user: author) + comment = create(:comment, comment_thread: thread, author_type: "local_agent", author_id: token.id, agent_name: "Amp") + expect(described_class.new(author, comment).delete?).to be(false) + end + + it "forbids deleting when no user is present" do + comment = create(:comment, comment_thread: thread, author_type: "human", author_id: author.id) + expect(described_class.new(nil, comment).delete?).to be(false) + end + end +end diff --git a/spec/requests/api/v1/comments_spec.rb b/spec/requests/api/v1/comments_spec.rb index 6157569d..f9d6367d 100644 --- a/spec/requests/api/v1/comments_spec.rb +++ b/spec/requests/api/v1/comments_spec.rb @@ -128,6 +128,51 @@ expect(body["error"]).to include("Agent name") end + describe "DELETE destroy" do + let!(:comment) do + create(:comment, comment_thread: thread_record, author_type: "human", author_id: alice.id, body_markdown: "to be deleted") + end + + it "soft-deletes the human author's own comment via hook auth" do + allow(CoPlan.configuration).to receive(:api_authenticate).and_return(->(_req) { { external_id: alice.external_id } }) + + delete api_v1_plan_destroy_comment_path(plan, id: comment.id), as: :json + expect(response).to have_http_status(:ok) + expect(comment.reload.deleted_at).to be_present + end + + it "forbids agent (token auth) callers from deleting their own agent comment" do + agent_comment = create(:comment, + comment_thread: thread_record, + author_type: "local_agent", + author_id: alice_token.id, + agent_name: "Amp", + body_markdown: "agent output") + + delete api_v1_plan_destroy_comment_path(plan, id: agent_comment.id), + headers: headers, + as: :json + expect(response).to have_http_status(:forbidden) + expect(agent_comment.reload.deleted_at).to be_nil + end + + it "forbids a different human from deleting" do + bob = create(:coplan_user) + allow(CoPlan.configuration).to receive(:api_authenticate).and_return(->(_req) { { external_id: bob.external_id } }) + + delete api_v1_plan_destroy_comment_path(plan, id: comment.id), as: :json + expect(response).to have_http_status(:forbidden) + expect(comment.reload.deleted_at).to be_nil + end + + it "returns 404 for a missing comment" do + allow(CoPlan.configuration).to receive(:api_authenticate).and_return(->(_req) { { external_id: alice.external_id } }) + + delete api_v1_plan_destroy_comment_path(plan, id: "nonexistent-id"), as: :json + expect(response).to have_http_status(:not_found) + end + end + it "create comment requires auth" do post api_v1_plan_comments_path(plan), params: { body_markdown: "No auth" }, diff --git a/spec/requests/comments_spec.rb b/spec/requests/comments_spec.rb index be1e74c4..13cf315f 100644 --- a/spec/requests/comments_spec.rb +++ b/spec/requests/comments_spec.rb @@ -18,4 +18,37 @@ expect(comment.author_type).to eq("human") expect(comment.author_id).to eq(alice.id) end + + describe "DELETE destroy" do + let!(:comment) do + create(:comment, comment_thread: thread_record, author_type: "human", author_id: alice.id, body_markdown: "to be deleted") + end + + it "soft-deletes the author's own comment" do + expect { + delete plan_comment_thread_comment_path(plan, thread_record, comment) + }.to change { comment.reload.deleted_at }.from(nil) + expect(response).to redirect_to(plan_path(plan)) + end + + it "redirects with alert when the user is not the comment author" do + bob = create(:coplan_user) + bobs_comment = create(:comment, comment_thread: thread_record, author_type: "human", author_id: bob.id, body_markdown: "alice can't touch this") + + delete plan_comment_thread_comment_path(plan, thread_record, bobs_comment) + expect(bobs_comment.reload.deleted_at).to be_nil + expect(flash[:alert]).to be_present + end + + it "empties the thread when the last kept comment is deleted" do + delete plan_comment_thread_comment_path(plan, thread_record, comment) + expect(thread_record.reload).to be_empty + end + + it "leaves the thread populated when a reply remains" do + create(:comment, comment_thread: thread_record, author_type: "human", author_id: alice.id, body_markdown: "reply") + delete plan_comment_thread_comment_path(plan, thread_record, comment) + expect(thread_record.reload).not_to be_empty + end + end end diff --git a/spec/services/coplan/comments/soft_delete_spec.rb b/spec/services/coplan/comments/soft_delete_spec.rb new file mode 100644 index 00000000..07d9bd20 --- /dev/null +++ b/spec/services/coplan/comments/soft_delete_spec.rb @@ -0,0 +1,46 @@ +require "rails_helper" + +RSpec.describe CoPlan::Comments::SoftDelete do + let(:user) { create(:coplan_user) } + let(:plan) { create(:plan, created_by_user: user) } + let(:thread) { create(:comment_thread, plan: plan, created_by_user: user) } + let(:comment) { create(:comment, comment_thread: thread, author_type: "human", author_id: user.id, body_markdown: "Hello world") } + + describe ".call" do + it "soft-deletes the comment" do + expect { + described_class.call(comment: comment, actor: user) + }.to change { comment.reload.deleted_at }.from(nil) + end + + it "writes a comment_deleted PlanEvent with body preview metadata" do + expect { + described_class.call(comment: comment, actor: user) + }.to change { CoPlan::PlanEvent.where(event_type: "comment_deleted").count }.by(1) + + event = CoPlan::PlanEvent.where(event_type: "comment_deleted").last + expect(event.plan).to eq(plan) + expect(event.actor_user).to eq(user) + expect(event.metadata).to include( + "comment_id" => comment.id, + "thread_id" => thread.id, + "body_preview" => "Hello world" + ) + end + + it "is idempotent — no extra PlanEvent on a second call" do + described_class.call(comment: comment, actor: user) + expect { + described_class.call(comment: comment, actor: user) + }.not_to change { CoPlan::PlanEvent.count } + end + + it "truncates long bodies in the preview" do + long = "x" * 500 + comment.update!(body_markdown: long) + described_class.call(comment: comment, actor: user) + preview = CoPlan::PlanEvent.where(event_type: "comment_deleted").last.metadata["body_preview"] + expect(preview.length).to be <= described_class::BODY_PREVIEW_LENGTH + end + end +end