From 7b5d2077d065bf45554a0aa0b218ba585387f3e7 Mon Sep 17 00:00:00 2001
From: Kelvin Chou <kelvin@squareup.com>
Date: Mon, 3 Aug 2026 12:58:52 -0700
Subject: [PATCH 1/2] Keep non-rendered text out of comment anchors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A selection swept across a Mermaid diagram invisibly captures the SVG's
embedded <style> sheet: textContent includes it even though it never
renders. The stored anchor then re-matches that CSS on every visit, and
highlightAnchors() wraps <mark>s inside the <style> element. A <style>
only parses its direct child text, so the re-parented chunk drops out of
the sheet and the whole diagram loses its styling — black node boxes and
black-filled edge paths.

The corruption is theme-dependent, which made it look like a light-mode
bug: the anchor stores one theme's CSS text, so it only matches (and
only corrupts) when the diagram renders in that same theme.

Fix: define one rendered-text model — every text node under the content
target except those inside style/script/noscript — and use it everywhere
offsets are computed: anchor capture, context extraction, occurrence
counting, and highlighting. Existing anchors that already carry CSS text
simply stop matching, which degrades to the standard "content changed,
no highlight" behavior instead of corrupting the page.

Verified against a real plan with 71 comment threads: before, two
CSS-carrying anchors put marks inside the diagram's <style> and broke
all styling; after, every legitimate anchor still highlights, no marks
land in any <style>, and all four diagrams keep their full stylesheets.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 .../coplan/text_selection_controller.js       | 77 ++++++++++++-------
 1 file changed, 49 insertions(+), 28 deletions(-)

diff --git a/engine/app/javascript/controllers/coplan/text_selection_controller.js b/engine/app/javascript/controllers/coplan/text_selection_controller.js
index 12e9678..71e5aa0 100644
--- a/engine/app/javascript/controllers/coplan/text_selection_controller.js
+++ b/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

From a1f4fbe7a02fd226d39096a1102543313649bf8e Mon Sep 17 00:00:00 2001
From: Kelvin Chou <kelvin@squareup.com>
Date: Thu, 6 Aug 2026 10:38:50 -0700
Subject: [PATCH 2/2] Add system specs for anchors vs Mermaid stylesheets
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two regression tests for the black-diagram bug, driving the real
pipeline (Mermaid rendered in the browser, real Stimulus controllers):

- A stored anchor carrying stylesheet text (the shape of anchors
  captured before the fix) must not put a <mark> inside the SVG's
  <style> — the sheet keeps zero element children and a parseable rule
  list — while label and prose anchors keep highlighting.
- A selection swept across a diagram must capture anchor text without
  the stylesheet: visible text only, no CSS.

Both fail against the previous controller — the capture test reproduces
the exact poisoned-anchor shape observed in production ("The diagram
#coplan-mermaid-1{font-family:...") — and pass with the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 spec/system/mermaid_anchor_spec.rb | 133 +++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)
 create mode 100644 spec/system/mermaid_anchor_spec.rb

diff --git a/spec/system/mermaid_anchor_spec.rb b/spec/system/mermaid_anchor_spec.rb
new file mode 100644
index 0000000..3f1487a
--- /dev/null
+++ b/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