Keep non-rendered text out of comment anchors

engine/app/javascript/controllers/coplan/text_selection_controller.js

@@ -1,5 +1,13 @@
 import { Controller } from "@hotwired/stimulus"
 
+// Elements whose text is never rendered but still appears in textContent —
+// e.g. the <style> sheet Mermaid embeds inside its SVG. Their text must stay
+// out of the anchor text model (capture, occurrence counting, highlighting):
+// wrapping a <mark> inside a <style> re-parents part of the CSS out of the
+// sheet (a <style> only parses its direct child text), which strips the
+// diagram's styling and renders it as unstyled black shapes.
+const NON_RENDERED_TEXT_SELECTOR = "style, script, noscript"
+
 export default class extends Controller {
   static targets = ["content", "popover", "form", "anchorInput", "contextInput", "occurrenceInput", "anchorPreview", "anchorQuote", "threads"]
   static values = { focusThread: String }
@@ -109,13 +117,18 @@ export default class extends Controller {
       if (clampTarget) range.setEndAfter(clampTarget)
     }
 
-    // Extract text using the range's cloneContents().textContent so it
-    // matches this.contentTarget.textContent (used for occurrence lookup
-    // and highlighting). selection.toString() can differ — e.g. tables
-    // produce tab-separated text via toString() but not via textContent.
+    // Extract text from the range's cloneContents() so it matches the
+    // rendered text model (used for occurrence lookup and highlighting).
+    // selection.toString() can differ — e.g. tables produce tab-separated
+    // text via toString() but not via textContent. Drop non-rendered text
+    // first: a selection swept across a Mermaid diagram invisibly picks up
+    // its SVG <style> sheet, and an anchor carrying that CSS re-corrupts
+    // the diagram on every future visit.
     // Normalize whitespace (collapse runs of spaces/tabs/newlines) so the
     // stored anchor_text matches the server-side canonical form.
-    const text = this._normalizeWhitespace(range.cloneContents().textContent).trim()
+    const fragment = range.cloneContents()
+    fragment.querySelectorAll(NON_RENDERED_TEXT_SELECTOR).forEach(el => el.remove())
+    const text = this._normalizeWhitespace(fragment.textContent).trim()
 
     if (text.length < 1) {
       this.popoverTarget.style.display = "none"
@@ -233,7 +246,7 @@ export default class extends Controller {
     })
 
     // Build full text for position lookups
-    this.fullText = this.contentTarget.textContent
+    this.fullText = this._renderedText()
 
     const highlighted = this.findAndHighlight(anchor, occurrence, "anchor-highlight--active")
     if (highlighted) {
@@ -512,7 +525,7 @@ export default class extends Controller {
 
   extractContext(range, selectedText) {
     // Grab surrounding text for disambiguation
-    const fullText = this.contentTarget.textContent
+    const fullText = this._renderedText()
     const selIndex = fullText.indexOf(selectedText)
     if (selIndex === -1) return ""
 
@@ -540,7 +553,7 @@ export default class extends Controller {
   // Uses whitespace-normalized matching for consistency with findAndHighlight.
   computeOccurrence(range, text) {
     const offset = this.getSelectionOffset(range)
-    const fullText = this.contentTarget.textContent
+    const fullText = this._renderedText()
     const { normText, origIndices } = this._buildNormalizedMap(fullText)
     const normSearch = this._normalizeWhitespace(text)
 
@@ -560,19 +573,35 @@ export default class extends Controller {
   getSelectionOffset(range) {
     if (!range || !this.contentTarget) return 0
 
-    const walker = document.createTreeWalker(this.contentTarget, NodeFilter.SHOW_TEXT, null)
     let offset = 0
+    for (const node of this._renderedTextNodes()) {
+      if (range.startContainer === node) return offset + range.startOffset
+      offset += node.textContent.length
+    }
+
+    return offset
+  }
+
+  // The rendered text model: every text node under the content target except
+  // those inside non-rendered elements (see NON_RENDERED_TEXT_SELECTOR).
+  // Anchor capture, occurrence counting, and highlighting must all walk this
+  // same sequence — mixing it with raw textContent shifts every offset.
+  _renderedTextNodes() {
+    const walker = document.createTreeWalker(this.contentTarget, NodeFilter.SHOW_TEXT, null)
+    const nodes = []
     let node
 
     while ((node = walker.nextNode())) {
-      if (range.startContainer === node) {
-        offset += range.startOffset
-        break
-      }
-      offset += node.textContent.length
+      if (!node.parentElement?.closest(NON_RENDERED_TEXT_SELECTOR)) nodes.push(node)
     }
 
-    return offset
+    return nodes
+  }
+
+  _renderedText() {
+    let text = ""
+    for (const node of this._renderedTextNodes()) text += node.textContent
+    return text
   }
 
   highlightAnchors() {
@@ -588,7 +617,7 @@ export default class extends Controller {
     this.contentTarget.normalize()
 
     // Build full text once for position lookups
-    this.fullText = this.contentTarget.textContent
+    this.fullText = this._renderedText()
 
     const threads = this.element.querySelectorAll("[data-anchor-text]")
     threads.forEach(thread => {
@@ -771,19 +800,11 @@ export default class extends Controller {
   highlightAtIndexAll(startIndex, length, className) {
     if (startIndex < 0 || length <= 0) return []
 
-    const walker = document.createTreeWalker(
-      this.contentTarget,
-      NodeFilter.SHOW_TEXT,
-      null,
-      false
-    )
-
     const textNodes = []
-    let fullText = ""
-    let node
-    while (node = walker.nextNode()) {
-      textNodes.push({ node, start: fullText.length })
-      fullText += node.textContent
+    let offset = 0
+    for (const node of this._renderedTextNodes()) {
+      textNodes.push({ node, start: offset })
+      offset += node.textContent.length
     }
 
     const matchEnd = startIndex + length

spec/system/mermaid_anchor_spec.rb

@@ -0,0 +1,133 @@
+require "rails_helper"
+
+# Mermaid diagrams embed a <style> sheet inside their rendered SVG. That text
+# is present in textContent but never rendered, so it must stay out of the
+# comment-anchor text model: an anchor carrying stylesheet text re-matches it
+# on every visit, and a <mark> wrapped inside the <style> re-parents part of
+# the CSS out of the sheet — the diagram loses all styling and renders as
+# black shapes.
+#
+# These specs drive the real pipeline (Mermaid loads from the CDN pinned in
+# the importmap and renders in the browser), so they need network access —
+# same as CI, where the runner fetches it fresh.
+RSpec.describe "Comment anchors and Mermaid diagrams", type: :system do
+  let(:user) { create(:coplan_user, email: "testuser@example.com") }
+
+  let(:plan_content) do
+    <<~MARKDOWN
+      # Payment Flow
+
+      ## The diagram
+
+      ```mermaid
+      flowchart TB
+          A[Gateway] -->|routes to| B[Ledger]
+          B --> C[Event feed]
+      ```
+
+      The ledger records every movement.
+    MARKDOWN
+  end
+
+  let(:plan) do
+    p = CoPlan::Plan.create!(title: "Diagram Plan", created_by_user: user)
+    version = CoPlan::PlanVersion.create!(
+      plan: p, revision: 1,
+      content_markdown: plan_content, actor_type: "human", actor_id: user.id
+    )
+    p.update!(current_plan_version: version, current_revision: 1)
+    p
+  end
+
+  before do
+    visit sign_in_path
+    fill_in "Email address", with: user.email
+    click_button "Sign In"
+    expect(page).to have_current_path(root_path)
+    expect(page).to have_button("Menu")
+  end
+
+  # Mermaid is fetched from the CDN and renders asynchronously.
+  def wait_for_diagram
+    expect(page).to have_css(".mermaid-diagram svg", wait: 15)
+  end
+
+  describe "highlighting stored anchors" do
+    before do
+      # 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 <style> of
+      # every Mermaid SVG and nowhere in the rendered text.
+      poisoned = create(:comment_thread, plan: plan, anchor_text: 'font-family:"trebuchet ms"')
+      create(:comment, comment_thread: poisoned, author_id: user.id)
+
+      # A legitimate anchor on a diagram node label — marks inside rendered
+      # SVG labels are supported and must keep working.
+      label = create(:comment_thread, plan: plan, anchor_text: "Event feed")
+      create(:comment, comment_thread: label, author_id: user.id)
+
+      # A prose anchor whose mark signals that the post-render highlight pass
+      # has completed, so the absence assertions below don't run too early.
+      prose = create(:comment_thread, plan: plan, anchor_text: "records every movement")
+      create(:comment, comment_thread: prose, author_id: user.id)
+    end
+
+    it "keeps marks out of the SVG stylesheet and keeps label anchors working" do
+      visit plan_path(plan)
+      wait_for_diagram
+
+      # Highlights re-apply after Mermaid settles; wait for the prose and
+      # label marks from that same pass before asserting absences.
+      expect(page).to have_css("mark.anchor-highlight", text: "records every movement", wait: 10)
+      expect(page).to have_css(".mermaid-diagram svg mark.anchor-highlight", text: "Event feed", wait: 10)
+
+      style_state = page.evaluate_script(<<~JS)
+        (() => {
+          const style = document.querySelector(".mermaid-diagram svg style");
+          if (!style) return { present: false };
+          return {
+            present: true,
+            elementChildren: style.children.length,
+            cssRules: style.sheet ? style.sheet.cssRules.length : 0
+          };
+        })()
+      JS
+
+      expect(style_state["present"]).to be(true)
+      # A mark inside the <style> would appear as an element child and break
+      # the sheet; an intact sheet parses to a non-empty rule list.
+      expect(style_state["elementChildren"]).to eq(0)
+      expect(style_state["cssRules"]).to be > 0
+    end
+  end
+
+  describe "capturing a selection swept across a diagram" do
+    it "excludes the SVG stylesheet from the anchor text" do
+      visit plan_path(plan)
+      wait_for_diagram
+
+      page.execute_script(<<~JS)
+        const content = document.querySelector('[data-coplan--text-selection-target="content"]');
+        const heading = content.querySelector("h2");
+        const diagram = content.querySelector(".mermaid-diagram");
+        const range = document.createRange();
+        range.setStart(heading.firstChild, 0);
+        range.setEndAfter(diagram);
+        const sel = window.getSelection();
+        sel.removeAllRanges();
+        sel.addRange(range);
+        content.dispatchEvent(new MouseEvent("mouseup", { bubbles: true }));
+      JS
+
+      expect(page).to have_css(".comment-popover", visible: true, wait: 3)
+      find(".comment-popover button", text: "Comment").click
+
+      anchor_value = page.evaluate_script(
+        %{document.querySelector('[name="comment_thread[anchor_text]"]').value}
+      )
+      expect(anchor_value).to include("The diagram")
+      expect(anchor_value).to include("Gateway")
+      expect(anchor_value).not_to include("font-family")
+    end
+  end
+end