Improve MergeRequest tab-persisting behaviorrs-persist-tab-selection-more-betterer

Now uses the path instead of the hash.

See discussion in #728.

3 files changed, 56 insertions, 35 deletions

diff --git a/app/assets/javascripts/merge_request.js.coffee b/app/assets/javascripts/merge_request.js.coffee
index 3937c428e24..a5a2caf1263 100644
--- a/app/assets/javascripts/merge_request.js.coffee
+++ b/app/assets/javascripts/merge_request.js.coffee
@@ -26,7 +26,7 @@ class @MergeRequest
     @commits_loaded = @opts.commits_loaded or false
 
     this.bindEvents()
-    this.activateTabFromHash()
+    this.activateTabFromPath()
 
     this.initMergeWidget()
     this.$('.show-all-commits').on 'click', =>
@@ -82,19 +82,16 @@ class @MergeRequest
   bindEvents: ->
     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)}")
+      tab_action = $target.data('action')
 
       # Lazy-load diffs
-      if $target.data('action') == 'diffs'
+      if tab_action == 'diffs'
         this.loadDiff() unless @diffs_loaded
-        $('.diff-header').trigger("sticky_kit:recalc")
+        $('.diff-header').trigger('sticky_kit:recalc')
+
+      # Skip tab-persisting behavior on MergeRequests#new
+      unless @opts.action == 'new'
+        @setCurrentAction(tab_action)
 
     this.$('.accept_merge_request').on 'click', ->
       $('.automerge_widget.can_be_merged').hide()
@@ -112,27 +109,51 @@ class @MergeRequest
       this.$('.remove_source_branch_in_progress').hide()
       this.$('.remove_source_branch_widget.failed').show()
 
-  # Activates a tab section based on the `#!` URL hash
+  # Activate a tab based on the current URL path
   #
-  # If no hash value is present (i.e., on the initial page load), the first tab
-  # is selected by default.
+  # If the current action is 'show' or 'new' (i.e., initial page load),
+  # activates the first tab, otherwise activates the tab corresponding to the
+  # current action (diffs, commits).
+  activateTabFromPath: ->
+    if @opts.action == 'show' || @opts.action == 'new'
+      this.$('.merge-request-tabs a[data-toggle="tab"]:first').tab('show')
+    else
+      this.$(".merge-request-tabs a[data-action='#{@opts.action}']").tab('show')
+
+  # Replaces the current Merge Request-specific action in the URL with a new one
   #
-  # ... unless the current controller action is `diffs`, in which case that tab
-  # is selected instead. Fun, right?
+  # If the action is "notes", the URL is reset to the standard
+  # `MergeRequests#show` route.
   #
-  # Note: We use a `#!` instead of a standard URL hash for two reasons:
+  # Examples:
   #
-  # 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")
+  #   location.pathname # => "/namespace/project/merge_requests/1"
+  #   setCurrentAction('diffs')
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #   setCurrentAction('notes')
+  #   location.pathname # => "/namespace/project/merge_requests/1"
+  #
+  #   location.pathname # => "/namespace/project/merge_requests/1/diffs"
+  #   setCurrentAction('commits')
+  #   location.pathname # => "/namespace/project/merge_requests/1/commits"
+  setCurrentAction: (action) ->
+    # Normalize action, just to be safe
+    action = 'notes' if action == 'show'
+
+    # Remove a trailing '/commits' or '/diffs'
+    new_state = location.pathname.replace(/\/(commits|diffs)\/?$/, '')
+
+    # Append the new action if we're on a tab other than 'notes'
+    unless action == 'notes'
+      new_state += "/#{action}"
+
+    # Replace the current history state with the new one without breaking
+    # Turbolinks' history.
+    #
+    # See https://github.com/rails/turbolinks/issues/363
+    history.replaceState {turbolinks: true, url: new_state}, '', new_state
 
   showState: (state) ->
     $('.automerge_widget').hide()
@@ -161,7 +182,7 @@ class @MergeRequest
   loadDiff: (event) ->
     $.ajax
       type: 'GET'
-      url: this.$('.merge-request-tabs .diffs-tab a').data('source') + ".json"
+      url: this.$('.merge-request-tabs .diffs-tab a').attr('href') + ".json"
       beforeSend: =>
         this.$('.mr-loading-status .loading').show()
       complete: =>
diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml
index e83b7649928..9a2edbf0a8c 100644
--- a/app/views/projects/merge_requests/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/_new_submit.html.haml
@@ -20,12 +20,12 @@
 .mr-compare.merge-request
   %ul.nav.nav-tabs.merge-request-tabs
     %li.commits-tab
-      = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+      = link_to url_for(params), data: {target: '#commits', action: 'commits', toggle: 'tab'} do
         = icon('history')
         Commits
         %span.badge= @commits.size
     %li.diffs-tab
-      = link_to '#diffs', data: {action: 'diffs', toggle: 'tab'} do
+      = link_to url_for(params), data: {target: '#diffs', action: 'diffs', toggle: 'tab'} do
         = icon('list-alt')
         Changes
         %span.badge= @diffs.size
@@ -56,7 +56,7 @@
 :javascript
   var merge_request
   merge_request = new MergeRequest({
-    action: 'diffs',
+    action: 'new',
     diffs_loaded: true,
     commits_loaded: true
   });
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 0d894e360ea..bf056462b70 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -38,17 +38,17 @@
   - if @commits.present?
     %ul.nav.nav-tabs.merge-request-tabs
       %li.notes-tab
-        = link_to '#notes', data: {action: 'notes', toggle: 'tab'} do
+        = link_to namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#notes', action: 'notes', toggle: 'tab'} do
           = icon('comments')
           Discussion
           %span.badge= @merge_request.mr_and_commit_notes.user.count
       %li.commits-tab
-        = link_to '#commits', data: {action: 'commits', toggle: 'tab'} do
+        = link_to commits_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#commits', action: 'commits', toggle: 'tab'} do
           = icon('history')
           Commits
           %span.badge= @commits.size
       %li.diffs-tab
-        = link_to '#diffs', data: {source: diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), action: 'diffs', toggle: 'tab'} do
+        = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), data: {target: '#diffs', action: 'diffs', toggle: 'tab'} do
           = icon('list-alt')
           Changes
           %span.badge= @merge_request.diffs.size