Merge branch 'diff-file-view-optimisations' into 'master'

Diff file view optimisations

See merge request gitlab-org/gitlab!47091

app/helpers/diff_helper.rb

@@ -64,15 +64,17 @@ module DiffHelper
     else
       # `sub` and substring-ing would destroy HTML-safeness of `line`
       if line.start_with?('+', '-', ' ')
-        line.dup.tap do |line|
-          line[0] = ''
-        end
+        line[1, line.length]
       else
         line
       end
     end
   end
 
+  def diff_link_number(line_type, match, text)
+    line_type == match ? " " : text
+  end
+
   def parallel_diff_discussions(left, right, diff_file)
     return unless @grouped_diff_discussions
 

app/views/projects/commit/diff_files.html.haml

-- diff_files = diffs.diff_files
-
-= render partial: 'projects/diffs/file', collection: diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment, diff_page_context: 'is-commit' }
+= render partial: 'projects/diffs/file', collection: diffs.diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment, diff_page_context: 'is-commit' }

app/views/projects/diffs/_file.html.haml

@@ -14,16 +14,15 @@
         = submodule_diff_compare_link(diff_file)
 
     - unless diff_file.submodule?
-      - blob = diff_file.blob
       .file-actions.d-none.d-sm-block
-        - if blob&.readable_text?
+        - if diff_file.blob&.readable_text?
           = link_to '#', class: 'js-toggle-diff-comments gl-button btn active has-tooltip', title: _("Toggle comments for this file"), disabled: @diff_notes_disabled do
             = sprite_icon('comment')
           \
           - if editable_diff?(diff_file)
             - link_opts = @merge_request.persisted? ? { from_merge_request_iid: @merge_request.iid } : {}
             = edit_blob_button(@merge_request.source_project, @merge_request.source_branch, diff_file.new_path,
-                             blob: blob, link_opts: link_opts)
+                             blob: diff_file.blob, link_opts: link_opts)
 
         - if image_diff && image_replaced
           = view_file_button(diff_file.old_content_sha, diff_file.old_path, project, replaced: true)

app/views/projects/diffs/_file_header.html.haml

-- show_toggle = local_assigns.fetch(:show_toggle, true)
-
-- if show_toggle
+- if local_assigns.fetch(:show_toggle, true)
   %i.fa.diff-toggle-caret.fa-fw
 
 - if diff_file.submodule?
-  - blob = diff_file.blob
   %span
     = sprite_icon('archive')
 
     %strong.file-title-name
-      = submodule_link(blob, diff_file.content_sha, diff_file.repository)
+      = submodule_link(diff_file.blob, diff_file.content_sha, diff_file.repository)
 
-    = copy_file_path_button(blob.path)
+    = copy_file_path_button(diff_file.blob.path)
 - else
   = conditional_link_to url.present?, url do
     = blob_icon diff_file.b_mode, diff_file.file_path

app/views/projects/diffs/_line.html.haml

-- email = local_assigns.fetch(:email, false)
 - plain = local_assigns.fetch(:plain, false)
 - discussions = local_assigns.fetch(:discussions, nil)
-- type = line.type
 - line_code = diff_file.line_code(line)
 - if discussions && line.discussable?
   - line_discussions = discussions[line_code]
-%tr.line_holder{ class: type, id: (line_code unless plain) }
-  - case type
+
+%tr.line_holder{ class: line.type, id: (line_code unless plain) }
+  - case line.type
   - when 'match'
     = diff_match_line line.old_pos, line.new_pos, text: line.text
   - when 'old-nonewline', 'new-nonewline'
@@ -14,21 +13,21 @@
     %td.new_line.diff-line-num
     %td.line_content.match= line.text
   - else
-    %td.old_line.diff-line-num{ class: [type, ("js-avatar-container" if !plain)], data: { linenumber: line.old_pos } }
-      - link_text = type == "new" ? " " : line.old_pos
+    %td.old_line.diff-line-num{ class: [line.type, ("js-avatar-container" if !plain)], data: { linenumber: line.old_pos } }
       - if plain
-        = link_text
+        = diff_link_number(line.type, "new", line.old_pos)
       - else
-        = add_diff_note_button(line_code, diff_file.position(line), type)
-        %a{ href: "##{line_code}", data: { linenumber: link_text } }
-    %td.new_line.diff-line-num{ class: type, data: { linenumber: line.new_pos } }
-      - link_text = type == "old" ? " " : line.new_pos
+        = add_diff_note_button(line_code, diff_file.position(line), line.type)
+        %a{ href: "##{line_code}", data: { linenumber: diff_link_number(line.type, "new", line.old_pos) } }
+
+    %td.new_line.diff-line-num{ class: line.type, data: { linenumber: line.new_pos } }
       - if plain
-        = link_text
+        = diff_link_number(line.type, "old", line.new_pos)
       - else
-        %a{ href: "##{line_code}", data: { linenumber: link_text } }
-    %td.line_content{ class: type }<
-      - if email
+        %a{ href: "##{line_code}", data: { linenumber: diff_link_number(line.type, "old", line.new_pos) } }
+
+    %td.line_content{ class: line.type }<
+      - if local_assigns.fetch(:email, false)
         %pre= line.rich_text
       - else
         = diff_line_content(line.rich_text)

spec/helpers/diff_helper_spec.rb

@@ -130,6 +130,38 @@ RSpec.describe DiffHelper do
     end
   end
 
+  describe "#diff_link_number" do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:line) do
+      double(:line, type: line_type)
+    end
+
+    # This helper is used to generate the line numbers on the
+    # diff lines. It essentially just returns a blank string
+    # on the old/new lines. The following table tests all the
+    # possible permutations for clarity.
+
+    where(:line_type, :match, :line_number, :expected_return_value) do
+      "new"           | "new" | 1  | " "
+      "new"           | "old" | 2  | 2
+      "old"           | "new" | 3  | 3
+      "old"           | "old" | 4  | " "
+      "new-nonewline" | "new" | 5  | 5
+      "new-nonewline" | "old" | 6  | 6
+      "old-nonewline" | "new" | 7  | 7
+      "old-nonewline" | "old" | 8  | 8
+      "match"         | "new" | 9  | 9
+      "match"         | "old" | 10 | 10
+    end
+
+    with_them do
+      it "returns the expected value" do
+        expect(helper.diff_link_number(line.type, match, line_number)).to eq(expected_return_value)
+      end
+    end
+  end
+
   describe "#mark_inline_diffs" do
     let(:old_line) { %{abc 'def'} }
     let(:new_line) { %{abc "def"} }