Persist current merge request tab selection via URL

Closes internal https://dev.gitlab.org/gitlab/gitlabhq/issues/2350

app/assets/javascripts/merge_request.js.coffee

@@ -3,15 +3,30 @@
 #= require task_list
 
 class @MergeRequest
+  # Initialize MergeRequest behavior
+  #
+  # Options:
+  #   action         - String, current controller action
+  #   diffs_loaded   - Boolean, have diffs been pre-rendered server-side?
+  #                    (default: true if `action` is 'diffs', otherwise false)
+  #   commits_loaded - Boolean, have commits been pre-rendered server-side?
+  #                    (default: false)
+  #
+  #   check_enable           - Boolean, whether to check automerge status
+  #   url_to_automerge_check - String, URL to use to check automerge status
+  #   current_status         - String, current automerge status
+  #   ci_enable              - Boolean, whether a CI service is enabled
+  #   url_to_ci_check        - String, URL to use to check CI status
+  #
   constructor: (@opts) ->
     @initContextWidget()
     this.$el = $('.merge-request')
-    @diffs_loaded = if @opts.action == 'diffs' then true else false
-    @commits_loaded = false
 
-    this.activateTab(@opts.action)
+    @diffs_loaded = @opts.diffs_loaded or @opts.action == 'diffs'
+    @commits_loaded = @opts.commits_loaded or false
 
     this.bindEvents()
+    this.activateTabFromHash()
 
     this.initMergeWidget()
     this.$('.show-all-commits').on 'click', =>
@@ -65,8 +80,21 @@ class @MergeRequest
       , 'json'
 
   bindEvents: ->
-    this.$('.merge-request-tabs').on 'click', 'li', (event) =>
-      this.activateTab($(event.currentTarget).data('action'))
+    this.$('.merge-request-tabs a[data-toggle="tab"]').on 'shown.bs.tab', (e) =>
+      $target = $(e.target)
+
+      # Nothing else to be done if we're on the first tab
+      return if $target.data('action') == 'notes'
+
+      # Persist current tab selection via URL
+      href = $target.attr('href')
+      if href.substr(0,1) == '#'
+        location.replace("#!#{href.substr(1)}")
+
+      # Lazy-load diffs
+      if $target.data('action') == 'diffs'
+        this.loadDiff() unless @diffs_loaded
+        $('.diff-header').trigger("sticky_kit:recalc")
 
     this.$('.accept_merge_request').on 'click', ->
       $('.automerge_widget.can_be_merged').hide()
@@ -84,21 +112,27 @@ class @MergeRequest
       this.$('.remove_source_branch_in_progress').hide()
       this.$('.remove_source_branch_widget.failed').show()
 
-  activateTab: (action) ->
-    this.$('.merge-request-tabs li').removeClass 'active'
-    this.$('.tab-content').hide()
-    switch action
-      when 'diffs'
-        this.$('.merge-request-tabs .diffs-tab').addClass 'active'
-        this.loadDiff() unless @diffs_loaded
-        this.$('.diffs').show()
-        $(".diff-header").trigger("sticky_kit:recalc")
-      when 'commits'
-        this.$('.merge-request-tabs .commits-tab').addClass 'active'
-        this.$('.commits').show()
-      else
-        this.$('.merge-request-tabs .notes-tab').addClass 'active'
-        this.$('.notes').show()
+  # Activates a tab section based on the `#!` URL hash
+  #
+  # If no hash value is present (i.e., on the initial page load), the first tab
+  # is selected by default.
+  #
+  # ... unless the current controller action is `diffs`, in which case that tab
+  # is selected instead. Fun, right?
+  #
+  # Note: We use a `#!` instead of a standard URL hash for two reasons:
+  #
+  # 1. Prevents the hash acting like an anchor and scrolling the page.
+  # 2. Prevents mutating browser history.
+  activateTabFromHash: ->
+    # Correct the hash if we came here directly via the `/diffs` path
+    if location.hash == '' and @opts.action == 'diffs'
+      location.replace('#!diffs')
+
+    if location.hash == ''
+      this.$('.merge-request-tabs a[data-toggle="tab"]:first').tab('show')
+    else if location.hash.substr(0,2) == '#!'
+      this.$(".merge-request-tabs a[href='##{location.hash.substr(2)}']").tab("show")
 
   showState: (state) ->
     $('.automerge_widget').hide()
@@ -127,7 +161,7 @@ class @MergeRequest
   loadDiff: (event) ->
     $.ajax
       type: 'GET'
-      url: this.$('.merge-request-tabs .diffs-tab a').attr('href') + ".json"
+      url: this.$('.merge-request-tabs .diffs-tab a').data('source') + ".json"
       beforeSend: =>
         this.$('.mr-loading-status .loading').show()
       complete: =>

app/views/projects/merge_requests/_new_submit.html.haml

@@ -19,30 +19,31 @@
 
 .mr-compare.merge-request
   %ul.nav.nav-tabs.merge-request-tabs
-    %li.commits-tab{data: {action: 'commits', toggle: 'tab'}}
-      = link_to url_for(params) do
-        %i.fa.fa-history
+    %li.commits-tab
+      = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+        = icon('history')
         Commits
         %span.badge= @commits.size
-    %li.diffs-tab{data: {action: 'diffs', toggle: 'tab'}}
-      = link_to url_for(params) do
-        %i.fa.fa-list-alt
+    %li.diffs-tab
+      = link_to '#diffs', data: {action: 'diffs', toggle: 'tab'} do
+        = icon('list-alt')
         Changes
         %span.badge= @diffs.size
 
-  .commits.tab-content
-    = render "projects/commits/commits", project: @project
-  .diffs.tab-content
-    - if @diffs.present?
-      = render "projects/diffs/diffs", diffs: @diffs, project: @project
-    - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
-      .alert.alert-danger
-        %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
-        %p To preserve performance the line changes are not shown.
-    - else
-      .alert.alert-danger
-        %h4 This comparison includes a huge diff.
-        %p To preserve performance the line changes are not shown.
+  .tab-content
+    #commits.commits.tab-pane
+      = render "projects/commits/commits", project: @project
+    #diffs.diffs.tab-pane
+      - if @diffs.present?
+        = render "projects/diffs/diffs", diffs: @diffs, project: @project
+      - elsif @commits.size > MergeRequestDiff::COMMITS_SAFE_SIZE
+        .alert.alert-danger
+          %h4 This comparison includes more than #{MergeRequestDiff::COMMITS_SAFE_SIZE} commits.
+          %p To preserve performance the line changes are not shown.
+      - else
+        .alert.alert-danger
+          %h4 This comparison includes a huge diff.
+          %p To preserve performance the line changes are not shown.
 
 :javascript
   $('.assign-to-me-link').on('click', function(e){
@@ -55,6 +56,8 @@
 :javascript
   var merge_request
   merge_request = new MergeRequest({
-    action: 'commits'
+    action: 'diffs',
+    diffs_loaded: true,
+    commits_loaded: true
   });
 

app/views/projects/merge_requests/_show.html.haml

@@ -25,7 +25,7 @@
           %span.pull-right
             .btn-group
               %a.btn.dropdown-toggle{ data: {toggle: :dropdown} }
-                %i.fa.fa-download
+                = icon('download')
                 Download as
                 %span.caret
               %ul.dropdown-menu
@@ -37,29 +37,30 @@
 
   - if @commits.present?
     %ul.nav.nav-tabs.merge-request-tabs
-      %li.notes-tab{data: {action: 'notes', toggle: 'tab'}}
-        = link_to merge_request_path(@merge_request) do
-          %i.fa.fa-comments
+      %li.notes-tab
+        = link_to '#notes', data: {action: 'notes', toggle: 'tab'} do
+          = icon('comments')
           Discussion
           %span.badge= @merge_request.mr_and_commit_notes.user.count
-      %li.commits-tab{data: {action: 'commits', toggle: 'tab'}}
-        = link_to merge_request_path(@merge_request), title: 'Commits' do
-          %i.fa.fa-history
+      %li.commits-tab
+        = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+          = icon('history')
           Commits
           %span.badge= @commits.size
-      %li.diffs-tab{data: {action: 'diffs', toggle: 'tab'}}
-        = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) do
-          %i.fa.fa-list-alt
+      %li.diffs-tab
+        = link_to '#diffs', data: {source: diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), action: 'diffs', toggle: 'tab'} do
+          = icon('list-alt')
           Changes
           %span.badge= @merge_request.diffs.size
 
-  .notes.tab-content.voting_notes#notes{ class: (controller.action_name == 'show') ? "" : "hide" }
-    = render "projects/merge_requests/discussion"
-  .commits.tab-content
-    = render "projects/merge_requests/show/commits"
-  .diffs.tab-content
-    - if current_page?(action: 'diffs')
-      = render "projects/merge_requests/show/diffs"
+  .tab-content
+    #notes.notes.tab-pane.voting_notes
+      = render "projects/merge_requests/discussion"
+    #commits.commits.tab-pane
+      = render "projects/merge_requests/show/commits"
+    #diffs.diffs.tab-pane
+      - if current_page?(action: 'diffs')
+        = render "projects/merge_requests/show/diffs"
 
   .mr-loading-status
     = spinner

features/steps/project/merge_requests.rb

@@ -113,7 +113,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I click on the Changes tab via Javascript' do
-    find('.diffs-tab').click
+    within '.merge-request-tabs' do
+      click_link 'Changes'
+    end
+
     sleep 2
   end