Fix review comments

including refactoring, disabling sourcepos for pipelines that
don't need it, and minimizing spec changes by disabling
sourcepos when not testing for it explicitly.

lib/banzai/filter/markdown_engines/common_mark.rb

@@ -50,7 +50,7 @@ module Banzai
         private
 
         def render_options
-          @context&.dig(:no_sourcepos) ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS
+          @context[:no_sourcepos] ? RENDER_OPTIONS : RENDER_OPTIONS_SOURCEPOS
         end
       end
     end

lib/banzai/filter/spaced_link_filter.rb

@@ -73,7 +73,8 @@ module Banzai
         html = Banzai::Filter::MarkdownFilter.call(transform_markdown(match), context)
 
         # link is wrapped in a <p>, so strip that off
-        html.sub(/<p[^>]*>/, '').chomp('</p>')
+        p_node = Nokogiri::HTML.fragment(html).at_css('p')
+        p_node ? p_node.children.to_html : html
       end
 
       def spaced_link_filter(text)

lib/banzai/pipeline/broadcast_message_pipeline.rb

@@ -14,6 +14,12 @@ module Banzai
           Filter::ExternalLinkFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end

lib/banzai/pipeline/email_pipeline.rb

@@ -11,7 +11,8 @@ module Banzai
 
       def self.transform_context(context)
         super(context).merge(
-          only_path: false
+          only_path: false,
+          no_sourcepos: true
         )
       end
     end

lib/banzai/pipeline/single_line_pipeline.rb

@@ -27,6 +27,12 @@ module Banzai
           Filter::CommitReferenceFilter
         ]
       end
+
+      def self.transform_context(context)
+        super(context).merge(
+          no_sourcepos: true
+        )
+      end
     end
   end
 end

spec/helpers/issuables_helper_spec.rb

@@ -173,6 +173,7 @@ describe IssuablesHelper do
     before do
       allow(helper).to receive(:current_user).and_return(user)
       allow(helper).to receive(:can?).and_return(true)
+      stub_commonmark_sourcepos_disabled
     end
 
     it 'returns the correct json for an issue' do
@@ -193,7 +194,7 @@ describe IssuablesHelper do
         projectNamespace: @project.namespace.path,
         initialTitleHtml: issue.title,
         initialTitleText: issue.title,
-        initialDescriptionHtml: '<p data-sourcepos="1:1-1:10" dir="auto">issue text</p>',
+        initialDescriptionHtml: '<p dir="auto">issue text</p>',
         initialDescriptionText: 'issue text',
         initialTaskStatus: '0 of 0 tasks completed'
       }

spec/lib/banzai/filter/markdown_filter_spec.rb

@@ -30,21 +30,21 @@ describe Banzai::Filter::MarkdownFilter do
       end
 
       it 'adds language to lang attribute when specified' do
-        result = filter("```html\nsome code\n```")
+        result = filter("```html\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code lang="html">')
+        expect(result).to start_with('<pre><code lang="html">')
       end
 
       it 'does not add language to lang attribute when not specified' do
-        result = filter("```\nsome code\n```")
+        result = filter("```\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code>')
+        expect(result).to start_with('<pre><code>')
       end
 
       it 'works with utf8 chars in language' do
-        result = filter("```日\nsome code\n```")
+        result = filter("```日\nsome code\n```", no_sourcepos: true)
 
-        expect(result).to start_with('<pre data-sourcepos="1:1-3:3"><code lang="日">')
+        expect(result).to start_with('<pre><code lang="日">')
       end
     end
 
@@ -80,7 +80,7 @@ describe Banzai::Filter::MarkdownFilter do
       end
 
       it 'disables data-sourcepos' do
-        result = filter('test', { no_sourcepos: true })
+        result = filter('test', no_sourcepos: true)
 
         expect(result).to eq '<p>test</p>'
       end
@@ -109,9 +109,9 @@ describe Banzai::Filter::MarkdownFilter do
       [^1]: a footnote
       MD
 
-      result = filter(text)
+      result = filter(text, no_sourcepos: true)
 
-      expect(result).to include('<td data-sourcepos="3:2-3:12">foot <sup')
+      expect(result).to include('<td>foot <sup')
       expect(result).to include('<section class="footnotes">')
     end
   end

spec/lib/banzai/pipeline/description_pipeline_spec.rb

@@ -8,11 +8,15 @@ describe Banzai::Pipeline::DescriptionPipeline do
 
     output = described_class.to_html(html, project: spy)
 
-    output.gsub!(%r{\A<p #{MarkdownFeature::SOURCEPOS_REGEX} dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
+    output.gsub!(%r{\A<p dir="auto">(.*)</p>(.*)\z}, '\1\2') if unwrap
 
     output
   end
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   it 'uses a limited whitelist' do
     doc = parse('# Description')
 

spec/lib/banzai/pipeline/full_pipeline_spec.rb

@@ -54,6 +54,8 @@ describe Banzai::Pipeline::FullPipeline do
     end
 
     it 'properly adds the necessary ids and classes' do
+      stub_commonmark_sourcepos_disabled
+
       expect(html.lines.map(&:strip).join("\n")).to eq filtered_footnote
     end
   end

spec/models/concerns/cache_markdown_field_spec.rb

@@ -67,14 +67,17 @@ describe CacheMarkdownField do
   end
 
   let(:markdown) { '`Foo`' }
-  let(:html)     { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Foo</code></p>' }
+  let(:html)     { '<p dir="auto"><code>Foo</code></p>' }
 
   let(:updated_markdown) { '`Bar`' }
-  let(:updated_html)     { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Bar</code></p>' }
-  let(:updated_redcarpet_html) { '<p dir="auto"><code>Bar</code></p>' }
+  let(:updated_html)     { '<p dir="auto"><code>Bar</code></p>' }
 
   let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
 
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   describe '.attributes' do
     it 'excludes cache attributes' do
       expect(thing.attributes.keys.sort).to eq(%w[bar baz foo])
@@ -96,16 +99,13 @@ describe CacheMarkdownField do
   context 'a changed markdown field' do
     shared_examples 'with cache version' do |cache_version|
       let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
-      let(:updated_version_html) do
-        cache_version == CacheMarkdownField::CACHE_REDCARPET_VERSION ? updated_redcarpet_html : updated_html
-      end
 
       before do
         thing.foo = updated_markdown
         thing.save
       end
 
-      it { expect(thing.foo_html).to eq(updated_version_html) }
+      it { expect(thing.foo_html).to eq(updated_html) }
       it { expect(thing.cached_markdown_version).to eq(cache_version) }
     end
 
@@ -272,9 +272,6 @@ describe CacheMarkdownField do
   describe '#refresh_markdown_cache!' do
     shared_examples 'with cache version' do |cache_version|
       let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
-      let(:updated_version_html) do
-        cache_version == CacheMarkdownField::CACHE_REDCARPET_VERSION ? updated_redcarpet_html : updated_html
-      end
 
       before do
         thing.foo = updated_markdown
@@ -283,7 +280,7 @@ describe CacheMarkdownField do
       it 'fills all html fields' do
         thing.refresh_markdown_cache!
 
-        expect(thing.foo_html).to eq(updated_version_html)
+        expect(thing.foo_html).to eq(updated_html)
         expect(thing.foo_html_changed?).to be_truthy
         expect(thing.baz_html_changed?).to be_truthy
       end
@@ -298,7 +295,7 @@ describe CacheMarkdownField do
       it 'saves the changes using #update_columns' do
         expect(thing).to receive(:persisted?).and_return(true)
         expect(thing).to receive(:update_columns)
-          .with("foo_html" => updated_version_html, "baz_html" => "", "cached_markdown_version" => cache_version)
+          .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => cache_version)
 
         thing.refresh_markdown_cache!
       end

spec/models/concerns/cacheable_attributes_spec.rb

@@ -159,6 +159,10 @@ describe CacheableAttributes do
 
     describe 'edge cases' do
       describe 'caching behavior', :use_clean_rails_memory_store_caching do
+        before do
+          stub_commonmark_sourcepos_disabled
+        end
+
         it 'retrieves upload fields properly' do
           ar_record = create(:appearance, :with_logo)
           ar_record.cache!
@@ -177,7 +181,7 @@ describe CacheableAttributes do
           cache_record = Appearance.current
 
           expect(cache_record.description).to eq('**Hello**')
-          expect(cache_record.description_html).to eq('<p data-sourcepos="1:1-1:9" dir="auto"><strong>Hello</strong></p>')
+          expect(cache_record.description_html).to eq('<p dir="auto"><strong>Hello</strong></p>')
         end
       end
     end

spec/models/concerns/redactable_spec.rb

@@ -1,6 +1,10 @@
 require 'spec_helper'
 
 describe Redactable do
+  before do
+    stub_commonmark_sourcepos_disabled
+  end
+
   shared_examples 'model with redactable field' do
     it 'redacts unsubscribe token' do
       model[field] = 'some text /sent_notifications/00000000000000000000000000000000/unsubscribe more text'
@@ -35,7 +39,7 @@ describe Redactable do
 
       expected = 'some text /sent_notifications/REDACTED/unsubscribe more text'
       expect(model[field]).to eq expected
-      expect(model["#{field}_html"]).to eq "<p data-sourcepos=\"1:1-1:60\" dir=\"auto\">#{expected}</p>"
+      expect(model["#{field}_html"]).to eq "<p dir=\"auto\">#{expected}</p>"
     end
   end
 

spec/requests/api/markdown_spec.rb

@@ -7,6 +7,8 @@ describe API::Markdown do
     let(:user) {} # No-op. It gets overwritten in the contexts below.
 
     before do
+      stub_commonmark_sourcepos_disabled
+
       post api("/markdown", user), params: params
     end
 
@@ -15,7 +17,7 @@ describe API::Markdown do
         expect(response).to have_http_status(201)
         expect(response.headers["Content-Type"]).to eq("application/json")
         expect(json_response).to be_a(Hash)
-        expect(json_response["html"]).to eq("<p data-sourcepos=\"1:1-1:28\">#{text}</p>")
+        expect(json_response["html"]).to eq("<p>#{text}</p>")
       end
     end
 

spec/serializers/group_child_entity_spec.rb

@@ -10,6 +10,7 @@ describe GroupChildEntity do
 
   before do
     allow(request).to receive(:current_user).and_return(user)
+    stub_commonmark_sourcepos_disabled
   end
 
   shared_examples 'group child json' do
@@ -102,7 +103,7 @@ describe GroupChildEntity do
       let(:description) { ':smile:' }
 
       it 'has the correct markdown_description' do
-        expect(json[:markdown_description]).to eq('<p data-sourcepos="1:1-1:7" dir="auto"><gl-emoji title="smiling face with open mouth and smiling eyes" data-name="smile" data-unicode-version="6.0">😄</gl-emoji></p>')
+        expect(json[:markdown_description]).to eq('<p dir="auto"><gl-emoji title="smiling face with open mouth and smiling eyes" data-name="smile" data-unicode-version="6.0">😄</gl-emoji></p>')
       end
     end
 

spec/support/helpers/markdown_feature.rb

@@ -10,8 +10,6 @@
 class MarkdownFeature
   include FactoryBot::Syntax::Methods
 
-  SOURCEPOS_REGEX = 'data-sourcepos="\d*:\d*-\d*:\d*"'.freeze
-
   attr_reader :fixture_path
 
   def initialize(fixture_path = Rails.root.join('spec/fixtures/markdown.md.erb'))

spec/support/helpers/stub_gitlab_calls.rb

@@ -52,6 +52,12 @@ module StubGitlabCalls
       .and_return(stub_container_registry_blob)
   end
 
+  def stub_commonmark_sourcepos_disabled
+    allow_any_instance_of(Banzai::Filter::MarkdownEngines::CommonMark)
+      .to receive(:render_options)
+      .and_return(Banzai::Filter::MarkdownEngines::CommonMark::RENDER_OPTIONS)
+  end
+
   private
 
   def stub_container_registry_tag_manifest