Refuse comments whose anchor doesn't resolve — no pin, no comment

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);

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,35 +74,45 @@ 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
       authorize!(@thread, :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
       authorize!(@thread, :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
       authorize!(@thread, :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

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

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 <br/> tags, and the browser
+        # reads the label back without them — "first<br/>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 <br>/<br/> 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(/<br\s*\/?>/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

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 %>"></textarea>
     </div>
+    <%# Server-side create errors land here (turbo_stream.update) — e.g. a
+        selection whose anchor doesn't resolve against the plan source. %>
+    <div class="comment-form__error text-sm" id="new-comment-form-error"></div>
     <div class="comment-form__actions">
       <button type="submit" class="btn btn--primary btn--sm">Comment</button>
       <button type="button" class="btn btn--secondary btn--sm" data-action="coplan--text-selection#cancelComment">Cancel</button>

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

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 <br/> tags" do
+        md = "```mermaid\nflowchart LR\n  Queue[\"assignment — first<br/>fetching device wins\"] --> Printer\n```"
+        # The browser reads the rendered label back without the tag —
+        # "first<br/>fetching" is selected as "firstfetching".
+        assert_anchor_resolves(md, "firstfetching device wins", "first<br/>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
 

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