Defer Diff Loading on Changes Tab

src/api/app/assets/javascripts/webui/request.js

@@ -185,13 +185,21 @@ function loadDiffs(element){
   });
 }
 
-function loadChanges(requestNumber, requestActionId) { // jshint ignore:line
-  $('.loading-diff').removeClass('invisible');
-  var url = '/request/' + requestNumber + '/request_action/' + requestActionId + '/changes';
+function loadChanges() { // jshint ignore:line
+  $('.tab-content.sourcediff .loading').removeClass('invisible');
+
+  // Take the parameters from the container data
+  var url = $('#sourcediff-container').data('url');
+  var diffToSupersededId = $('#sourcediff-container').data('diff-to-superseded-id');
+  var queryString = diffToSupersededId ? '?diff_to_superseded=' + diffToSupersededId : '';
+
   $.ajax({
-    url: url,
+    url: url + queryString,
     success: function() {
-      $('.loading-diff').addClass('invisible');
+      $('.tab-content.sourcediff .loading').addClass('invisible');
+    },
+    error: function() {
+      $('#sourcediff-container .result').text('Something went wrong while loading changes.');
     }
   });
 }

src/api/app/components/sourcediff_component.html.haml

@@ -2,9 +2,12 @@
   .col
     - if @action.diff_not_cached
       .clearfix.mb-2.text-center
-        .btn.btn-outline-primary.cache-refresh{ title: 'Refresh results', onclick: "loadChanges(#{@bs_request.number}, #{@action.id});" }
+        .btn.btn-outline-primary.cache-refresh{ title: 'Refresh results', onclick: "loadChanges()" }
           Crunching the latest data. Refresh again in a few seconds
           %i.fas.fa-sync-alt{ id: "cache#0-reload" }
+        .text-center.p-4.loading.invisible
+          %i.fas.fa-spinner.fa-spin.me-1
+          Loading changes...
     - else
       - (@action.webui_sourcediff).each do |sourcediff|
         .clearfix.mb-2

src/api/app/controllers/webui/request_controller.rb

@@ -34,7 +34,6 @@ def show
       render :beta_show
     else
       @diff_limit = params[:full_diff] ? 0 : nil
-      @diff_to_superseded_id = params[:diff_to_superseded]
       @is_author = @bs_request.creator == User.possibly_nobody.login
 
       @is_target_maintainer = @bs_request.is_target_maintainer?(User.session)
@@ -355,12 +354,12 @@ def new_state
   end
 
   def set_superseded_request
-    return unless params[:diff_to_superseded]
+    return unless (@diff_to_superseded_id = params[:diff_to_superseded])
 
-    @diff_to_superseded = @bs_request.superseding.find_by(number: params[:diff_to_superseded])
+    @diff_to_superseded = @bs_request.superseding.find_by(number: @diff_to_superseded_id)
     return if @diff_to_superseded
 
-    flash[:error] = "Request #{params[:diff_to_superseded]} does not exist or is not superseded by request #{@bs_request.number}."
+    flash[:error] = "Request #{@diff_to_superseded_id} does not exist or is not superseded by request #{@bs_request.number}."
     nil
   end
 
@@ -517,8 +516,6 @@ def prepare_request_data
     @is_target_maintainer = @bs_request.is_target_maintainer?(User.session)
     @my_open_reviews = ReviewsFinder.new(@bs_request.reviews).open_reviews_for_user(User.session).reject(&:staging_project?)
 
-    @diff_to_superseded_id = params[:diff_to_superseded]
-
     # Handling request actions
     @action ||= @actions.first
     action_index = @actions.index(@action)

src/api/app/views/webui/request/_changes_content.html.haml

@@ -0,0 +1,12 @@
+- if diff_to_superseded
+  You're reviewing changes from
+  = link_to("superseded request ##{diff_to_superseded.number}.", request_show_path(number: diff_to_superseded.number))
+  This only represents a small part of changes which are included in this request.
+  = surround('(', ')') do
+    = link_to('See the full changes for request', request_changes_path(number: bs_request.number))
+- else
+  - bs_request.superseding.each do |supersed|
+    See the changes from
+    = link_to "superseded request ##{supersed.number}", request_changes_path(bs_request, diff_to_superseded: supersed)
+
+= render SourcediffComponent.new(bs_request: bs_request, action: action)

src/api/app/views/webui/request/changes.html.haml

@@ -14,18 +14,16 @@
         locals: { bs_request: @bs_request, action: @action, issues: @issues,
                   actions_count: @actions.count, active_tab: @active_tab }
     .container.p-4
-      .tab-content.sourcediff
-        - if @diff_to_superseded
-          You're reviewing changes from
-          = link_to("superseded request ##{@diff_to_superseded.number}.", request_show_path(number: @diff_to_superseded))
-          This only represents a small part of changes which are included in this request.
-          = surround('(', ')') do
-            = link_to('See the full changes for request', request_changes_path(number: @bs_request.number))
-        - else
-          - @bs_request.superseding.each do |supersed|
-            See the changes from
-            = link_to "superseded request ##{supersed.number}", request_changes_path(@bs_request, diff_to_superseded: supersed)
-        = render SourcediffComponent.new(bs_request: @bs_request, action: @action)
+      .tab-content.sourcediff{ data: { url: request_action_changes_path(@bs_request.number, @action.id),
+                                       diff_to_superseded_id: @diff_to_superseded_id },
+                               id: 'sourcediff-container' }
+        .result
+          // The content of this div is set by JavaScript which calls the partial ../_changes_content.html.haml
+          .text-center.p-4
+            %i.fas.fa-spinner.fa-spin.me-1
+            Loading changes...
+          :javascript
+            loadChanges();
   = render DeleteConfirmationDialogComponent.new(modal_id: 'delete-comment-modal',
                                                  method: :delete,
                                                  options: { modal_title: 'Delete comment?', remote: true })

src/api/app/views/webui/request/request_action_changes.js.erb

@@ -1 +1 @@
-$('.tab-content.sourcediff').html("<%= escape_javascript(render(SourcediffComponent.new(bs_request: @bs_request, action: @action))) %>");
+$('.tab-content.sourcediff').html("<%= escape_javascript(render(partial: 'webui/request/changes_content', locals: { bs_request: @bs_request, action: @action, diff_to_superseded: @diff_to_superseded })) %>");