Resolve "Projects::MergeRequestsController#show is slow (implement skeleton loading)"

app/assets/javascripts/notes.js

@@ -16,6 +16,10 @@ import Autosize from 'autosize';
 import 'vendor/jquery.caret'; // required by jquery.atwho
 import 'vendor/jquery.atwho';
 import AjaxCache from '~/lib/utils/ajax_cache';
+import Vue from 'vue';
+import syntaxHighlight from '~/syntax_highlight';
+import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
+import { __ } from '~/locale';
 import axios from './lib/utils/axios_utils';
 import { getLocationHash } from './lib/utils/url_utility';
 import Flash from './flash';
@@ -99,6 +103,13 @@ export default class Notes {
       $('.note-edit-form').clone()
         .addClass('mr-note-edit-form').insertAfter('.note-edit-form');
     }
+
+    const hash = getLocationHash();
+    const $anchor = hash && document.getElementById(hash);
+
+    if ($anchor) {
+      this.loadLazyDiff({ currentTarget: $anchor });
+    }
   }
 
   setViewType(view) {
@@ -135,6 +146,8 @@ export default class Notes {
     this.$wrapperEl.on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm);
     // toggle commit list
     this.$wrapperEl.on('click', '.system-note-commit-list-toggler', this.toggleCommitList);
+
+    this.$wrapperEl.on('click', '.js-toggle-lazy-diff', this.loadLazyDiff);
     // fetch notes when tab becomes visible
     this.$wrapperEl.on('visibilitychange', this.visibilityChange);
     // when issue status changes, we need to refresh data
@@ -173,6 +186,7 @@ export default class Notes {
     this.$wrapperEl.off('keydown', '.js-note-text');
     this.$wrapperEl.off('click', '.js-comment-resolve-button');
     this.$wrapperEl.off('click', '.system-note-commit-list-toggler');
+    this.$wrapperEl.off('click', '.js-toggle-lazy-diff');
     this.$wrapperEl.off('ajax:success', '.js-main-target-form');
     this.$wrapperEl.off('ajax:success', '.js-discussion-note-form');
     this.$wrapperEl.off('ajax:complete', '.js-main-target-form');
@@ -1207,6 +1221,60 @@ export default class Notes {
     return this.notesCountBadge.text(parseInt(this.notesCountBadge.text(), 10) + updateCount);
   }
 
+  static renderPlaceholderComponent($container) {
+    const el = $container.find('.js-code-placeholder').get(0);
+    new Vue({ // eslint-disable-line no-new
+      el,
+      components: {
+        SkeletonLoadingContainer,
+      },
+      render(createElement) {
+        return createElement('skeleton-loading-container');
+      },
+    });
+  }
+
+  static renderDiffContent($container, data) {
+    const { discussion_html } = data;
+    const lines = $(discussion_html).find('.line_holder');
+    lines.addClass('fade-in');
+    $container.find('tbody').prepend(lines);
+    const fileHolder = $container.find('.file-holder');
+    $container.find('.line-holder-placeholder').remove();
+    syntaxHighlight(fileHolder);
+  }
+
+  static renderDiffError($container) {
+    $container.find('.line_content').html(
+      $(`
+        <div class="nothing-here-block">
+          ${__('Unable to load the diff.')} <a class="js-toggle-lazy-diff" href="javascript:void(0)">Try again</a>?
+        </div>
+      `),
+    );
+  }
+
+  loadLazyDiff(e) {
+    const $container = $(e.currentTarget).closest('.js-toggle-container');
+    Notes.renderPlaceholderComponent($container);
+
+    $container.find('.js-toggle-lazy-diff').removeClass('js-toggle-lazy-diff');
+
+    const tableEl = $container.find('tbody');
+    if (tableEl.length === 0) return;
+
+    const fileHolder = $container.find('.file-holder');
+    const url = fileHolder.data('linesPath');
+
+    axios.get(url)
+      .then(({ data }) => {
+        Notes.renderDiffContent($container, data);
+      })
+      .catch(() => {
+        Notes.renderDiffError($container);
+      });
+  }
+
   toggleCommitList(e) {
     const $element = $(e.currentTarget);
     const $closestSystemCommitList = $element.siblings('.system-note-commit-list');

app/controllers/projects/discussions_controller.rb

@@ -19,6 +19,12 @@ class Projects::DiscussionsController < Projects::ApplicationController
     render_discussion
   end
 
+  def show
+    render json: {
+      discussion_html: view_to_html_string('discussions/_diff_with_notes', discussion: discussion, expanded: true)
+    }
+  end
+
   private
 
   def render_discussion

app/views/discussions/_diff_with_notes.html.haml

@@ -2,8 +2,12 @@
 - blob = discussion.blob
 - discussions = { discussion.original_line_code => [discussion] }
 - diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file'
+- diff_data = {}
+- expanded = discussion.expanded? || local_assigns.fetch(:expanded, nil)
+- unless expanded
+  - diff_data = { lines_path: project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion) }
 
-.diff-file.file-holder{ class: diff_file_class }
+.diff-file.file-holder{ class: diff_file_class, data: diff_data }
   .js-file-title.file-title.file-title-flex-parent
     .file-header-content
       = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false
@@ -11,17 +15,24 @@
   - if diff_file.text?
     .diff-content.code.js-syntax-highlight
       %table
-        = render partial: "projects/diffs/line",
-          collection: discussion.truncated_diff_lines,
-          as: :line,
-          locals: { diff_file: diff_file,
-            discussions: discussions,
-            discussion_expanded: true,
-            plain: true }
+        - if expanded
+          - discussions = { discussion.original_line_code => [discussion] }
+          = render partial: "projects/diffs/line",
+            collection: discussion.truncated_diff_lines,
+            as: :line,
+            locals: { diff_file: diff_file,
+              discussions: discussions,
+              discussion_expanded: true,
+              plain: true }
+        - else
+          %tr.line_holder.line-holder-placeholder
+            %td.old_line.diff-line-num
+            %td.new_line.diff-line-num
+            %td.line_content
+              .js-code-placeholder
+          = render "discussions/diff_discussion", discussions: [discussion], expanded: true
   - else
     - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff'
-
     = render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false }
-
     .note-container
       = render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse_class: true }

app/views/discussions/_discussion.html.haml

@@ -8,7 +8,7 @@
       .discussion.js-toggle-container{ data: { discussion_id: discussion.id } }
         .discussion-header
           .discussion-actions
-            %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" }
+            %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button", class: ("js-toggle-lazy-diff" unless expanded) }
               - if expanded
                 = icon("chevron-up")
               - else

changelogs/unreleased/35475-lazy-diff.yml

+---
+title: lazy load diffs on merge request discussions
+merge_request:
+author:
+type: performance

config/routes/project.rb

@@ -130,7 +130,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
           post :bulk_update
         end
 
-        resources :discussions, only: [], constraints: { id: /\h{40}/ } do
+        resources :discussions, only: [:show], constraints: { id: /\h{40}/ } do
           member do
             post :resolve
             delete :resolve, action: :unresolve

spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb

@@ -108,6 +108,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           it 'shows resolved discussion when toggled' do
             find(".timeline-content .discussion[data-discussion-id='#{note.discussion_id}'] .discussion-toggle-button").click
 
+            expect(page.find(".line-holder-placeholder")).to be_visible
             expect(page.find(".timeline-content #note_#{note.id}")).to be_visible
           end
         end

spec/features/merge_request/user_scrolls_to_note_on_load_spec.rb

@@ -5,15 +5,18 @@ describe 'Merge request > User scrolls to note on load', :js do
   let(:user) { project.creator }
   let(:merge_request) { create(:merge_request, source_project: project, author: user) }
   let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: project) }
+  let(:resolved_note) { create(:diff_note_on_merge_request, :resolved, noteable: merge_request, project: project) }
   let(:fragment_id) { "#note_#{note.id}" }
+  let(:collapsed_fragment_id) { "#note_#{resolved_note.id}" }
 
   before do
     sign_in(user)
     page.current_window.resize_to(1000, 300)
-    visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
   end
 
-  it 'scrolls down to fragment' do
+  it 'scrolls note into view' do
+    visit "#{project_merge_request_path(project, merge_request)}#{fragment_id}"
+
     page_height = page.current_window.size[1]
     page_scroll_y = page.evaluate_script("window.scrollY")
     fragment_position_top = page.evaluate_script("Math.round($('#{fragment_id}').offset().top)")
@@ -23,4 +26,13 @@ describe 'Merge request > User scrolls to note on load', :js do
     expect(fragment_position_top).to be >= page_scroll_y
     expect(fragment_position_top).to be < (page_scroll_y + page_height)
   end
+
+  it 'expands collapsed notes' do
+    visit "#{project_merge_request_path(project, merge_request)}#{collapsed_fragment_id}"
+    note_element = find(collapsed_fragment_id)
+    note_container = note_element.ancestor('.js-toggle-container')
+
+    expect(note_element.visible?).to eq true
+    expect(note_container.find('.line_content.noteable_line.old', match: :first).visible?).to eq true
+  end
 end