From 3b546d4a9241e16985f720d90f6905b729c7d390 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Thu, 13 Aug 2026 11:29:02 -0500 Subject: [PATCH] =?UTF-8?q?Refuse=20comments=20whose=20anchor=20doesn't=20?= =?UTF-8?q?resolve=20=E2=80=94=20no=20pin,=20no=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A comment thread whose anchor never resolved to source positions renders nowhere: no highlight, no popover, no way to reach it from the page. Yet create happily persisted it and told the user "Comment added." — a comment posted into the void. CommentThread now validates on create that a present anchor resolved. Resolution moves from before_create to before_validation (on: :create) so validation can see its result. Both stay create-only: a resolved thread whose content later drifts is the out_of_date flow, not a validity problem. On refusal, the selection form keeps the draft and shows the error inline (new #new-comment-form-error region, updated via turbo stream with a 422); a plain HTML post redirects with an alert. The JSON API already returned 422 for RecordInvalid, so agents were covered. The rule also surfaced a resolver gap: mermaid labels line-break on literal
tags, and the browser reads the label back without them — "first
fetching" is selected as "firstfetching", which never matched the source and would now be refused despite being a real, visible pin. The resolver gains a pass that drops the tags from the stripped text, carrying the position map along, so these anchors resolve (and now survive edits via OT instead of going out_of_date unconditionally). Specs that relied on silently-unresolved anchors are the proof of the hole: the factory's :with_anchor trait and several request specs anchored to text their plans never contained. Co-Authored-By: Claude Opus 5 --- .../assets/stylesheets/coplan/application.css | 10 +++ .../coplan/comment_threads_controller.rb | 46 +++++++++----- .../coplan/text_selection_controller.js | 3 + engine/app/models/coplan/comment_thread.rb | 51 ++++++++++++++-- .../_new_comment_form.html.erb | 3 + spec/factories/comment_threads.rb | 5 +- spec/models/comment_thread_anchor_spec.rb | 61 +++++++++++++++++-- spec/requests/api/v1/plans_spec.rb | 4 +- spec/requests/comment_threads_spec.rb | 38 +++++++++++- spec/system/mermaid_anchor_spec.rb | 7 ++- 10 files changed, 196 insertions(+), 32 deletions(-) diff --git a/engine/app/assets/stylesheets/coplan/application.css b/engine/app/assets/stylesheets/coplan/application.css index ad37943c..24597746 100644 --- a/engine/app/assets/stylesheets/coplan/application.css +++ b/engine/app/assets/stylesheets/coplan/application.css @@ -1859,6 +1859,16 @@ img.avatar { border-radius: 0 var(--radius) var(--radius) 0; } +/* Server-side create errors (e.g. an anchor that doesn't resolve). + Empty until a failed submit fills it, so it takes no space at rest. */ +.comment-form__error { + color: var(--color-danger); +} + +.comment-form__error:not(:empty) { + margin-bottom: var(--space-sm); +} + .comment-form__actions { display: flex; gap: var(--space-sm); diff --git a/engine/app/controllers/coplan/comment_threads_controller.rb b/engine/app/controllers/coplan/comment_threads_controller.rb index f945fe2d..ab12a9e1 100644 --- a/engine/app/controllers/coplan/comment_threads_controller.rb +++ b/engine/app/controllers/coplan/comment_threads_controller.rb @@ -3,7 +3,7 @@ class CommentThreadsController < ApplicationController include ActionView::RecordIdentifier before_action :set_plan - before_action :set_thread, only: [:resolve, :accept, :discard, :reopen] + before_action :set_thread, only: [ :resolve, :accept, :discard, :reopen ] def create authorize!(@plan, :show?) @@ -30,13 +30,21 @@ def create # Atomic: a thread without its first comment is an empty orphan whose # anchor still highlights. comment = nil - ActiveRecord::Base.transaction do - thread.save! - comment = thread.comments.create!( - author_type: "human", - author_id: current_user.id, - body_markdown: thread_params[:body_markdown] - ) + begin + ActiveRecord::Base.transaction do + thread.save! + comment = thread.comments.create!( + author_type: "human", + author_id: current_user.id, + body_markdown: thread_params[:body_markdown] + ) + end + rescue ActiveRecord::RecordInvalid => e + # Most likely an anchor that doesn't resolve — a thread that would + # render nowhere. Refused here rather than created invisible. The + # selection form stays open (its reset checks for success) and + # shows the message; the 422 lets programmatic clients fall back. + return render_comment_error(e.record.errors.full_messages.to_sentence) end CreateNotificationsJob.perform_later( @@ -54,7 +62,7 @@ def create # authenticity tokens. The inline copy for the actor stays # request-scoped. Broadcaster.append_to(@plan, target: "plan-threads", partial: "coplan/comment_threads/thread_popover", locals: locals) - html = render_to_string(partial: "coplan/comment_threads/thread_popover", locals: locals, formats: [:html]) + html = render_to_string(partial: "coplan/comment_threads/thread_popover", locals: locals, formats: [ :html ]) inline_streams << turbo_stream.append("plan-threads", html) end @@ -66,7 +74,7 @@ def resolve @thread.resolve!(current_user) CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread resolved.", streams: [stream]) + respond_with_stream_or_redirect("Thread resolved.", streams: [ stream ]) end def accept @@ -74,7 +82,7 @@ def accept @thread.accept!(current_user) CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread accepted.", streams: [stream]) + respond_with_stream_or_redirect("Thread accepted.", streams: [ stream ]) end def discard @@ -82,7 +90,7 @@ def discard @thread.discard!(current_user) CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread discarded.", streams: [stream]) + respond_with_stream_or_redirect("Thread discarded.", streams: [ stream ]) end def reopen @@ -90,11 +98,21 @@ def reopen @thread.update!(status: "pending", resolved_by_user: nil) CreateNotificationsJob.perform_later(comment_thread_id: @thread.id, actor_id: current_user.id, reason: "status_change") stream = broadcast_thread_replace(@thread) - respond_with_stream_or_redirect("Thread reopened.", streams: [stream]) + respond_with_stream_or_redirect("Thread reopened.", streams: [ stream ]) end private + def render_comment_error(message) + respond_to do |format| + format.turbo_stream do + render turbo_stream: turbo_stream.update("new-comment-form-error", message), + status: :unprocessable_content + end + format.html { redirect_to plan_path(@plan), alert: message } + end + end + def set_plan @plan = Plan.find(params[:plan_id]) end @@ -123,7 +141,7 @@ def respond_with_stream_or_redirect(message, streams: []) def broadcast_thread_replace(thread) locals = { thread: thread, plan: @plan } Broadcaster.replace_to(@plan, target: dom_id(thread), partial: "coplan/comment_threads/thread_popover", locals: locals) - html = render_to_string(partial: "coplan/comment_threads/thread_popover", locals: locals, formats: [:html]) + html = render_to_string(partial: "coplan/comment_threads/thread_popover", locals: locals, formats: [ :html ]) turbo_stream.replace(dom_id(thread), html) end end diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js index 02f82e7a..79d87ff5 100644 --- a/engine/app/javascript/controllers/coplan/text_selection_controller.js +++ b/engine/app/javascript/controllers/coplan/text_selection_controller.js @@ -229,6 +229,9 @@ export default class extends Controller { this.anchorPreviewTarget.style.display = "none" const textarea = this.formTarget.querySelector("textarea") if (textarea) textarea.value = "" + // A create error from the previous attempt shouldn't greet the next one. + const error = this.formTarget.querySelector("#new-comment-form-error") + if (error) error.textContent = "" this.selectedText = null this.selectedContext = null this.selectedOccurrence = null diff --git a/engine/app/models/coplan/comment_thread.rb b/engine/app/models/coplan/comment_thread.rb index 9cc0d7a8..73e276f6 100644 --- a/engine/app/models/coplan/comment_thread.rb +++ b/engine/app/models/coplan/comment_thread.rb @@ -17,7 +17,14 @@ class CommentThread < ApplicationRecord validates :status, presence: true, inclusion: { in: STATUSES } - before_create :resolve_anchor_position + # Resolution runs before validation so validation can see its result: + # a thread whose anchor never resolved renders nowhere — no highlight, + # no popover, no way to reach it from the page — so it is refused at + # the door rather than created invisible. Create-only on both: a + # resolved thread whose content later drifts is the out_of_date flow, + # not a validity problem. + before_validation :resolve_anchor_position, on: :create + validate :anchor_must_resolve, on: :create scope :open_threads, -> { where(status: OPEN_STATUSES) } scope :current, -> { where(out_of_date: false) } @@ -67,7 +74,7 @@ def self.mark_out_of_date_for_new_version!(new_version) begin new_range = Plans::TransformRange.transform_through_versions( - [thread.anchor_start, thread.anchor_end], + [ thread.anchor_start, thread.anchor_end ], intervening ) thread.update_columns( @@ -166,8 +173,8 @@ def anchor_context_with_highlight(chars: 100) content = plan.current_content return nil unless content.present? - context_start = [anchor_start - chars, 0].max - context_end = [anchor_end + chars, content.length].min + context_start = [ anchor_start - chars, 0 ].max + context_end = [ anchor_end + chars, content.length ].min before = content[context_start...anchor_start] anchor = content[anchor_start...anchor_end] @@ -182,6 +189,12 @@ def self.strip_markdown(content) private + def anchor_must_resolve + return if anchor_text.blank? || anchor_start.present? + + errors.add(:anchor_text, "doesn't match the plan content — the comment would have nowhere to appear") + end + def resolve_anchor_position return unless anchor_text.present? @@ -205,11 +218,20 @@ def resolve_anchor_position normalized_anchor = anchor_text.gsub("\t", " ") stripped_ranges = find_all_occurrences(stripped, normalized_anchor) + # Mermaid labels line-break on literal
tags, and the browser + # reads the label back without them — "first
fetching" renders + # (and gets selected) as "firstfetching". Drop the tags from the + # search text; the position map keeps pointing at the source. + if stripped_ranges.empty? + stripped, pos_map = remove_break_tags(stripped, pos_map) + stripped_ranges = find_all_occurrences(stripped, normalized_anchor) + end + ranges = stripped_ranges.map do |s, e| raw_start = first_real_pos(pos_map, s, :forward) raw_end = first_real_pos(pos_map, e - 1, :backward) next nil unless raw_start && raw_end - [raw_start, raw_end + 1] + [ raw_start, raw_end + 1 ] end.compact end @@ -221,6 +243,23 @@ def resolve_anchor_position end end + # Removes
/
tags from stripped text, carrying the position + # map along so matches still resolve to raw source positions. + def remove_break_tags(text, pos_map) + kept = +"" + map = [] + last = 0 + text.scan(//i) do + m = Regexp.last_match + kept << text[last...m.begin(0)] + map.concat(pos_map[last...m.begin(0)]) + last = m.end(0) + end + kept << text[last..] + map.concat(pos_map[last..]) + [ kept, map ] + end + # Finds the nearest non-sentinel (-1) position in the pos_map, # scanning forward or backward from the given index. def first_real_pos(pos_map, idx, direction) @@ -236,7 +275,7 @@ def find_all_occurrences(text, search) ranges = [] start_pos = 0 while (idx = text.index(search, start_pos)) - ranges << [idx, idx + search.length] + ranges << [ idx, idx + search.length ] start_pos = idx + search.length end ranges diff --git a/engine/app/views/coplan/comment_threads/_new_comment_form.html.erb b/engine/app/views/coplan/comment_threads/_new_comment_form.html.erb index 708ca81d..a02826d6 100644 --- a/engine/app/views/coplan/comment_threads/_new_comment_form.html.erb +++ b/engine/app/views/coplan/comment_threads/_new_comment_form.html.erb @@ -12,6 +12,9 @@ data-controller="coplan--comment-form" data-coplan--comment-form-search-url-value="<%= search_users_path %>"> + <%# Server-side create errors land here (turbo_stream.update) — e.g. a + selection whose anchor doesn't resolve against the plan source. %> +
diff --git a/spec/factories/comment_threads.rb b/spec/factories/comment_threads.rb index 68a23b92..f486f183 100644 --- a/spec/factories/comment_threads.rb +++ b/spec/factories/comment_threads.rb @@ -6,8 +6,11 @@ status { "pending" } out_of_date { false } + # Threads refuse anchors that don't resolve against the plan content, + # so the anchor here is a real substring of the plan factory's default + # content_markdown. trait :with_anchor do - anchor_text { "some anchor text" } + anchor_text { "Some content here" } end trait :with_positioned_anchor do diff --git a/spec/models/comment_thread_anchor_spec.rb b/spec/models/comment_thread_anchor_spec.rb index 5d8c5b76..7dc910d8 100644 --- a/spec/models/comment_thread_anchor_spec.rb +++ b/spec/models/comment_thread_anchor_spec.rb @@ -13,6 +13,48 @@ plan end + describe "anchor_must_resolve on create" do + # A thread whose anchor never resolved renders nowhere: no highlight, + # no popover, no path to it from the page. Refused at the door rather + # than created invisible. + it "refuses a thread whose anchor resolves nowhere" do + expect { + plan.comment_threads.create!( + plan_version: plan.current_plan_version, + created_by_user: user, anchor_text: "text the plan never says" + ) + }.to raise_error(ActiveRecord::RecordInvalid, /nowhere to appear/) + end + + it "refuses an occurrence beyond the ones that exist" do + expect { + plan.comment_threads.create!( + plan_version: plan.current_plan_version, + created_by_user: user, anchor_text: "unit tests", anchor_occurrence: 3 + ) + }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "allows a thread with no anchor at all" do + thread = plan.comment_threads.create!( + plan_version: plan.current_plan_version, created_by_user: user + ) + expect(thread).to be_persisted + end + + # Content drift after creation is the out_of_date flow, not a validity + # problem — an old thread must stay updatable. + it "does not re-litigate the anchor on update" do + thread = plan.comment_threads.create!( + plan_version: plan.current_plan_version, + created_by_user: user, anchor_text: "unit tests" + ) + thread.update_columns(anchor_text: "text no longer in the plan", anchor_start: nil, anchor_end: nil) + + expect(thread.reload.update(status: "todo")).to be true + end + end + describe "resolve_anchor_position on create" do it "resolves anchor_text to character positions" do thread = plan.comment_threads.create!( @@ -153,6 +195,13 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) assert_anchor_resolves(md, "run", "run") end + it "mermaid label text broken by
tags" do + md = "```mermaid\nflowchart LR\n Queue[\"assignment — first
fetching device wins\"] --> Printer\n```" + # The browser reads the rendered label back without the tag — + # "first
fetching" is selected as "firstfetching". + assert_anchor_resolves(md, "firstfetching device wins", "first
fetching device wins") + end + it "heading text (strips # markers)" do assert_anchor_resolves( "# My Heading\n\nContent here.", @@ -238,7 +287,7 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) version2 = CoPlan::PlanVersion.create!( plan: plan, revision: 2, content_markdown: new_content, actor_type: "human", actor_id: user.id, - operations_json: [{ "op" => "replace_exact", "resolved_range" => [anchor_pos, anchor_pos + 7], "new_range" => [anchor_pos, anchor_pos + 7], "delta" => 0 }] + operations_json: [ { "op" => "replace_exact", "resolved_range" => [ anchor_pos, anchor_pos + 7 ], "new_range" => [ anchor_pos, anchor_pos + 7 ], "delta" => 0 } ] ) plan.update!(current_plan_version: version2, current_revision: 2) @@ -259,7 +308,7 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) version2 = CoPlan::PlanVersion.create!( plan: plan, revision: 2, content_markdown: new_content, actor_type: "human", actor_id: user.id, - operations_json: [{ "op" => "replace_exact", "resolved_range" => [unit_test_pos, unit_test_pos + 10], "new_range" => [unit_test_pos, unit_test_pos + 17], "delta" => 7 }] + operations_json: [ { "op" => "replace_exact", "resolved_range" => [ unit_test_pos, unit_test_pos + 10 ], "new_range" => [ unit_test_pos, unit_test_pos + 17 ], "delta" => 7 } ] ) plan.update!(current_plan_version: version2, current_revision: 2) @@ -283,7 +332,7 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) version2 = CoPlan::PlanVersion.create!( plan: plan, revision: 2, content_markdown: new_content, actor_type: "human", actor_id: user.id, - operations_json: [{ "op" => "replace_exact", "resolved_range" => [first_pos, first_pos + first_len], "new_range" => [first_pos, first_pos + new_len], "delta" => new_len - first_len }] + operations_json: [ { "op" => "replace_exact", "resolved_range" => [ first_pos, first_pos + first_len ], "new_range" => [ first_pos, first_pos + new_len ], "delta" => new_len - first_len } ] ) plan.update!(current_plan_version: version2, current_revision: 2) @@ -305,7 +354,7 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) version2 = CoPlan::PlanVersion.create!( plan: plan, revision: 2, content_markdown: new_content, actor_type: "human", actor_id: user.id, - operations_json: [{ "op" => "replace_exact", "resolved_range" => [anchor_pos, anchor_pos + 7], "new_range" => [anchor_pos, anchor_pos + 7], "delta" => 0 }] + operations_json: [ { "op" => "replace_exact", "resolved_range" => [ anchor_pos, anchor_pos + 7 ], "new_range" => [ anchor_pos, anchor_pos + 7 ], "delta" => 0 } ] ) plan.update!(current_plan_version: version2, current_revision: 2) @@ -317,12 +366,12 @@ def assert_anchor_resolves(markdown, dom_text, expected_raw, occurrence: nil) describe "#anchor_valid?" do it "returns true for non-outdated thread" do - thread = create(:comment_thread, plan: plan, anchor_text: "some text") + thread = create(:comment_thread, plan: plan, anchor_text: "First section") expect(thread.anchor_valid?).to be true end it "returns false for outdated thread" do - thread = create(:comment_thread, plan: plan, anchor_text: "some text", out_of_date: true) + thread = create(:comment_thread, plan: plan, anchor_text: "First section", out_of_date: true) expect(thread.anchor_valid?).to be false end diff --git a/spec/requests/api/v1/plans_spec.rb b/spec/requests/api/v1/plans_spec.rb index c81986e2..dc4d022c 100644 --- a/spec/requests/api/v1/plans_spec.rb +++ b/spec/requests/api/v1/plans_spec.rb @@ -276,13 +276,13 @@ def alice_placement it "comments returns thread list with anchor_text" do thread = create(:comment_thread, :with_anchor, plan: plan, - plan_version: plan.current_plan_version, created_by_user: alice, anchor_text: "original roadmap text") + plan_version: plan.current_plan_version, created_by_user: alice) get comments_api_v1_plan_path(plan), headers: headers expect(response).to have_http_status(:success) threads = JSON.parse(response.body) expect(threads).to be_a(Array) matching = threads.find { |t| t["id"] == thread.id } - expect(matching["anchor_text"]).to eq("original roadmap text") + expect(matching["anchor_text"]).to eq("Some content here") end describe "GET /api/v1/plans/:id/snapshot" do diff --git a/spec/requests/comment_threads_spec.rb b/spec/requests/comment_threads_spec.rb index 8b0fa1c2..ed338bd6 100644 --- a/spec/requests/comment_threads_spec.rb +++ b/spec/requests/comment_threads_spec.rb @@ -3,7 +3,16 @@ RSpec.describe "CommentThreads", type: :request do let(:alice) { create(:coplan_user, :admin) } let(:bob) { create(:coplan_user) } - let(:plan) { create(:plan, :considering, created_by_user: alice) } + + # Threads refuse anchors that don't resolve, so the plan has to actually + # say the thing these specs anchor to. + let(:plan) do + create(:plan, :considering, created_by_user: alice).tap do |p| + version = create(:plan_version, plan: p, revision: 2, actor_id: alice.id, + content_markdown: "## Ambition\n\nOur goal is world domination by Q3.\n") + p.update_columns(current_plan_version_id: version.id, current_revision: 2) + end + end before { sign_in_as(alice) } @@ -19,10 +28,37 @@ expect(response).to redirect_to(plan_path(plan)) thread = CoPlan::CommentThread.last expect(thread.anchor_text).to eq("world domination") + expect(thread.anchor_start).to be_present # resolved at the door expect(thread.status).to eq("todo") # author's own comments start as todo expect(thread.plan_version_id).to eq(plan.current_plan_version_id) end + # A thread whose anchor never resolved renders nowhere — no highlight, no + # popover, no way to reach it. "Comment posted" followed by nothing + # visible is worse than a refusal. + describe "when the anchor doesn't resolve against the plan" do + it "refuses to create the thread" do + expect { + post plan_comment_threads_path(plan), params: { + comment_thread: { anchor_text: "text the plan never says", body_markdown: "Lost forever." } + } + }.not_to change { [ CoPlan::CommentThread.count, CoPlan::Comment.count ] } + + expect(response).to redirect_to(plan_path(plan)) + expect(flash[:alert]).to include("nowhere to appear") + end + + it "tells a turbo-stream client with a 422 so it can fall back" do + post plan_comment_threads_path(plan), + params: { comment_thread: { anchor_text: "text the plan never says", body_markdown: "Lost." } }, + headers: { "Accept" => "text/vnd.turbo-stream.html, text/html" } + + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include("new-comment-form-error") + expect(response.body).to include("nowhere to appear") + end + end + it "broadcasts the popover via requestless partial render, never request-scoped HTML" do # The popover contains reply/action forms; request-rendered HTML embeds # the actor's session authenticity token, which must not be broadcast. diff --git a/spec/system/mermaid_anchor_spec.rb b/spec/system/mermaid_anchor_spec.rb index 3f1487a1..6da3ee2f 100644 --- a/spec/system/mermaid_anchor_spec.rb +++ b/spec/system/mermaid_anchor_spec.rb @@ -57,8 +57,11 @@ def wait_for_diagram # An anchor that carries stylesheet text — the shape of anchors captured # by sweeping a selection across a diagram before capture excluded # non-rendered text. This string appears verbatim in the