Merge branch '18435-autocomplete-is-not-performant' into 'master'

Make autocomplete request performant

## What does this MR do?

Restricts the `autocomplete_sources`action to returning the `json`of one specific `at` type at a time.

Makes sure all commands start by immediately showing the `loading` dropdown.

If the `loading` dropdown is active, we will request and load in the data for that specific `at` type. We manually trigger the filter again. This time, if `loading` dropdown is not active, initiate default filter behaviour and use the correct data template.

Also fixes an issue where `setup` was called multiple times by only setting up and destroying atwho on a focused/blured input.

## Are there points in the code the reviewer needs to double check?

## Why was this MR needed?

`autocomplete_sources` was requested on page load and is large.

## Screenshots (if relevant)

3 requests for 3 different first-time references.

![Screen_Shot_2016-10-14_at_14.43.12](/uploads/0b04eab0d0bf3638d6d6e431036b8089/Screen_Shot_2016-10-14_at_14.43.12.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #22802

Closes #18435

See merge request !6856

app/assets/javascripts/gfm_auto_complete.js.es6

@@ -2,19 +2,28 @@
 
 // Creates the variables for setting up GFM auto-completion
 (function() {
-  if (window.GitLab == null) {
+  if (window.gl == null) {
-    window.GitLab = {};
+    window.gl = {};
   }
 
   function sanitize(str) {
     return str.replace(/<(?:.|\n)*?>/gm, '');
   }
 
-  window.GitLab.GfmAutoComplete = {
+  window.gl.GfmAutoComplete = {
-    dataLoading: false,
+    dataSources: {},
-    dataLoaded: false,
+    defaultLoadingData: ['loading'],
     cachedData: {},
-    dataSource: '',
+    isLoadingData: {},
+    atTypeMap: {
+      ':': 'emojis',
+      '@': 'members',
+      '#': 'issues',
+      '!': 'mergeRequests',
+      '~': 'labels',
+      '%': 'milestones',
+      '/': 'commands'
+    },
     // Emoji
     Emoji: {
       template: '<li>${name} <img alt="${name}" height="20" src="${path}" width="20" /></li>'
@@ -35,33 +44,31 @@
       template: '<li>${title}</li>'
     },
     Loading: {
-      template: '<li><i class="fa fa-refresh fa-spin"></i> Loading...</li>'
+      template: '<li style="pointer-events: none;"><i class="fa fa-refresh fa-spin"></i> Loading...</li>'
     },
     DefaultOptions: {
       sorter: function(query, items, searchKey) {
-        // Highlight first item only if at least one char was typed
+        if (gl.GfmAutoComplete.isLoading(items)) {
-        this.setting.highlightFirst = this.setting.alwaysHighlightFirst || query.length > 0;
-        if ((items[0].name != null) && items[0].name === 'loading') {
           return items;
         }
         return $.fn.atwho["default"].callbacks.sorter(query, items, searchKey);
       },
       filter: function(query, data, searchKey) {
-        if (data[0] === 'loading') {
+        if (gl.GfmAutoComplete.isLoading(data)) {
+          gl.GfmAutoComplete.togglePreventSelection.call(this, true);
+          gl.GfmAutoComplete.fetchData(this.$inputor, this.at);
           return data;
-        }
+        } else {
+          gl.GfmAutoComplete.togglePreventSelection.call(this, false);
           return $.fn.atwho["default"].callbacks.filter(query, data, searchKey);
+        }
       },
       beforeInsert: function(value) {
         if (value && !this.setting.skipSpecialCharacterTest) {
           var withoutAt = value.substring(1);
           if (withoutAt && /[^\w\d]/.test(withoutAt)) value = value.charAt() + '"' + withoutAt + '"';
         }
-        if (!window.GitLab.GfmAutoComplete.dataLoaded) {
-          return this.at;
-        } else {
         return value;
-        }
       },
       matcher: function (flag, subtext) {
         // The below is taken from At.js source
@@ -86,69 +93,46 @@
         }
       }
     },
-    setup: _.debounce(function(input) {
+    setup: function(input) {
       // Add GFM auto-completion to all input fields, that accept GFM input.
       this.input = input || $('.js-gfm-input');
-      // destroy previous instances
+      this.setupLifecycle();
-      this.destroyAtWho();
+    },
-      // set up instances
+    setupLifecycle() {
-      this.setupAtWho();
+      this.input.each((i, input) => {
-
+        const $input = $(input);
-      if (this.dataSource && !this.dataLoading && !this.cachedData) {
+        $input.off('focus.setupAtWho').on('focus.setupAtWho', this.setupAtWho.bind(this, $input));
-        this.dataLoading = true;
-        return this.fetchData(this.dataSource)
-          .done((data) => {
-            this.dataLoading = false;
-            this.loadData(data);
       });
-        };
+    },
-
+    setupAtWho: function($input) {
-      if (this.cachedData != null) {
-        return this.loadData(this.cachedData);
-      }
-    }, 1000),
-    setupAtWho: function() {
       // Emoji
-      this.input.atwho({
+      $input.atwho({
         at: ':',
-        displayTpl: (function(_this) {
+        displayTpl: function(value) {
-          return function(value) {
+          return value.path != null ? this.Emoji.template : this.Loading.template;
-            if (value.path != null) {
+        }.bind(this),
-              return _this.Emoji.template;
-            } else {
-              return _this.Loading.template;
-            }
-          };
-        })(this),
         insertTpl: ':${name}:',
-        data: ['loading'],
         startWithSpace: false,
         skipSpecialCharacterTest: true,
+        data: this.defaultLoadingData,
         callbacks: {
           sorter: this.DefaultOptions.sorter,
-          filter: this.DefaultOptions.filter,
           beforeInsert: this.DefaultOptions.beforeInsert,
-          matcher: this.DefaultOptions.matcher
+          filter: this.DefaultOptions.filter
         }
       });
       // Team Members
-      this.input.atwho({
+      $input.atwho({
         at: '@',
-        displayTpl: (function(_this) {
+        displayTpl: function(value) {
-          return function(value) {
+          return value.username != null ? this.Members.template : this.Loading.template;
-            if (value.username != null) {
+        }.bind(this),
-              return _this.Members.template;
-            } else {
-              return _this.Loading.template;
-            }
-          };
-        })(this),
         insertTpl: '${atwho-at}${username}',
         searchKey: 'search',
-        data: ['loading'],
         startWithSpace: false,
         alwaysHighlightFirst: true,
         skipSpecialCharacterTest: true,
+        data: this.defaultLoadingData,
         callbacks: {
           sorter: this.DefaultOptions.sorter,
           filter: this.DefaultOptions.filter,
@@ -179,20 +163,14 @@
           }
         }
       });
-      this.input.atwho({
+      $input.atwho({
         at: '#',
         alias: 'issues',
         searchKey: 'search',
-        displayTpl: (function(_this) {
+        displayTpl: function(value) {
-          return function(value) {
+          return value.title != null ? this.Issues.template : this.Loading.template;
-            if (value.title != null) {
+        }.bind(this),
-              return _this.Issues.template;
+        data: this.defaultLoadingData,
-            } else {
-              return _this.Loading.template;
-            }
-          };
-        })(this),
-        data: ['loading'],
         insertTpl: '${atwho-at}${id}',
         startWithSpace: false,
         callbacks: {
@@ -214,26 +192,21 @@
           }
         }
       });
-      this.input.atwho({
+      $input.atwho({
         at: '%',
         alias: 'milestones',
         searchKey: 'search',
-        displayTpl: (function(_this) {
-          return function(value) {
-            if (value.title != null) {
-              return _this.Milestones.template;
-            } else {
-              return _this.Loading.template;
-            }
-          };
-        })(this),
         insertTpl: '${atwho-at}${title}',
-        data: ['loading'],
+        displayTpl: function(value) {
+          return value.title != null ? this.Milestones.template : this.Loading.template;
+        }.bind(this),
         startWithSpace: false,
+        data: this.defaultLoadingData,
         callbacks: {
           matcher: this.DefaultOptions.matcher,
           sorter: this.DefaultOptions.sorter,
           beforeInsert: this.DefaultOptions.beforeInsert,
+          filter: this.DefaultOptions.filter,
           beforeSave: function(milestones) {
             return $.map(milestones, function(m) {
               if (m.title == null) {
@@ -248,21 +221,15 @@
           }
         }
       });
-      this.input.atwho({
+      $input.atwho({
         at: '!',
         alias: 'mergerequests',
         searchKey: 'search',
-        displayTpl: (function(_this) {
-          return function(value) {
-            if (value.title != null) {
-              return _this.Issues.template;
-            } else {
-              return _this.Loading.template;
-            }
-          };
-        })(this),
-        data: ['loading'],
         startWithSpace: false,
+        displayTpl: function(value) {
+          return value.title != null ? this.Issues.template : this.Loading.template;
+        }.bind(this),
+        data: this.defaultLoadingData,
         insertTpl: '${atwho-at}${id}',
         callbacks: {
           sorter: this.DefaultOptions.sorter,
@@ -283,18 +250,31 @@
           }
         }
       });
-      this.input.atwho({
+      $input.atwho({
         at: '~',
         alias: 'labels',
         searchKey: 'search',
-        displayTpl: this.Labels.template,
+        data: this.defaultLoadingData,
+        displayTpl: function(value) {
+          return this.isLoading(value) ? this.Loading.template : this.Labels.template;
+        }.bind(this),
         insertTpl: '${atwho-at}${title}',
         startWithSpace: false,
         callbacks: {
           matcher: this.DefaultOptions.matcher,
           sorter: this.DefaultOptions.sorter,
           beforeInsert: this.DefaultOptions.beforeInsert,
+          filter: this.DefaultOptions.filter,
           beforeSave: function(merges) {
+            if (gl.GfmAutoComplete.isLoading(merges)) return merges;
+            var sanitizeLabelTitle;
+            sanitizeLabelTitle = function(title) {
+              if (/[\w\?&]+\s+[\w\?&]+/g.test(title)) {
+                return "\"" + (sanitize(title)) + "\"";
+              } else {
+                return sanitize(title);
+              }
+            };
             return $.map(merges, function(m) {
               return {
                 title: sanitize(m.title),
@@ -306,12 +286,14 @@
         }
       });
       // We don't instantiate the slash commands autocomplete for note and issue/MR edit forms
-      this.input.filter('[data-supports-slash-commands="true"]').atwho({
+      $input.filter('[data-supports-slash-commands="true"]').atwho({
         at: '/',
         alias: 'commands',
         searchKey: 'search',
         skipSpecialCharacterTest: true,
+        data: this.defaultLoadingData,
         displayTpl: function(value) {
+          if (this.isLoading(value)) return this.Loading.template;
           var tpl = '<li>/${name}';
           if (value.aliases.length > 0) {
             tpl += ' <small>(or /<%- aliases.join(", /") %>)</small>';
@@ -324,7 +306,7 @@
           }
           tpl += '</li>';
           return _.template(tpl)(value);
-        },
+        }.bind(this),
         insertTpl: function(value) {
           var tpl = "/${name} ";
           var reference_prefix = null;
@@ -342,6 +324,7 @@
           filter: this.DefaultOptions.filter,
           beforeInsert: this.DefaultOptions.beforeInsert,
           beforeSave: function(commands) {
+            if (gl.GfmAutoComplete.isLoading(commands)) return commands;
             return $.map(commands, function(c) {
               var search = c.name;
               if (c.aliases.length > 0) {
@@ -369,32 +352,40 @@
       });
       return;
     },
-    destroyAtWho: function() {
+    fetchData: function($input, at) {
-      return this.input.atwho('destroy');
+      if (this.isLoadingData[at]) return;
+      this.isLoadingData[at] = true;
+      if (this.cachedData[at]) {
+        this.loadData($input, at, this.cachedData[at]);
+      } else {
+        $.getJSON(this.dataSources[this.atTypeMap[at]], (data) => {
+          this.loadData($input, at, data);
+        }).fail(() => { this.isLoadingData[at] = false; });
+      }
     },
-    fetchData: function(dataSource) {
+    loadData: function($input, at, data) {
-      return $.getJSON(dataSource);
+      this.isLoadingData[at] = false;
-    },
+      this.cachedData[at] = data;
-    loadData: function(data) {
+      $input.atwho('load', at, data);
-      this.cachedData = data;
-      this.dataLoaded = true;
-      // load members
-      this.input.atwho('load', '@', data.members);
-      // load issues
-      this.input.atwho('load', 'issues', data.issues);
-      // load milestones
-      this.input.atwho('load', 'milestones', data.milestones);
-      // load merge requests
-      this.input.atwho('load', 'mergerequests', data.mergerequests);
-      // load emojis
-      this.input.atwho('load', ':', data.emojis);
-      // load labels
-      this.input.atwho('load', '~', data.labels);
-      // load commands
-      this.input.atwho('load', '/', data.commands);
       // This trigger at.js again
       // otherwise we would be stuck with loading until the user types
-      return $(':focus').trigger('keyup');
+      return $input.trigger('keyup');
+    },
+    isLoading(data) {
+      if (!data) return false;
+      if (Array.isArray(data)) data = data[0];
+      return data === this.defaultLoadingData[0] || data.name === this.defaultLoadingData[0];
+    },
+    togglePreventSelection(isPrevented = !!this.setting.tabSelectsMatch) {
+      this.setting.tabSelectsMatch = !isPrevented;
+      this.setting.spaceSelectsMatch = !isPrevented;
+      const eventListenerAction = `${isPrevented ? 'add' : 'remove'}EventListener`;
+      this.$inputor[0][eventListenerAction]('keydown', gl.GfmAutoComplete.preventSpaceTabEnter);
+    },
+    preventSpaceTabEnter(e) {
+      const key = e.which || e.keyCode;
+      const preventables = [9, 13, 32];
+      if (preventables.indexOf(key) > -1) e.preventDefault();
     }
   };
 

app/assets/javascripts/gl_form.js

@@ -30,7 +30,7 @@
         this.form.addClass('gfm-form');
         // remove notify commit author checkbox for non-commit notes
         gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button'));
-        GitLab.GfmAutoComplete.setup(this.form.find('.js-gfm-input'));
+        gl.GfmAutoComplete.setup(this.form.find('.js-gfm-input'));
         new DropzoneInput(this.form);
         autosize(this.textarea);
         // form and textarea event listeners

app/assets/javascripts/issuable_form.js

@@ -19,7 +19,7 @@
       this.renderWipExplanation = bind(this.renderWipExplanation, this);
       this.resetAutosave = bind(this.resetAutosave, this);
       this.handleSubmit = bind(this.handleSubmit, this);
-      GitLab.GfmAutoComplete.setup();
+      gl.GfmAutoComplete.setup();
       new UsersSelect();
       new ZenMode();
       this.titleField = this.form.find("input[name*='[title]']");

app/controllers/projects/autocomplete_sources_controller.rb

@@ -0,0 +1,48 @@
+class Projects::AutocompleteSourcesController < Projects::ApplicationController
+  before_action :load_autocomplete_service, except: [:emojis, :members]
+
+  def emojis
+    render json: Gitlab::AwardEmoji.urls
+  end
+
+  def members
+    render json: ::Projects::ParticipantsService.new(@project, current_user).execute(noteable)
+  end
+
+  def issues
+    render json: @autocomplete_service.issues
+  end
+
+  def merge_requests
+    render json: @autocomplete_service.merge_requests
+  end
+
+  def labels
+    render json: @autocomplete_service.labels
+  end
+
+  def milestones
+    render json: @autocomplete_service.milestones
+  end
+
+  def commands
+    render json: @autocomplete_service.commands(noteable, params[:type])
+  end
+
+  private
+
+  def load_autocomplete_service
+    @autocomplete_service = ::Projects::AutocompleteService.new(@project, current_user)
+  end
+
+  def noteable
+    case params[:type]
+    when 'Issue'
+      IssuesFinder.new(current_user, project_id: @project.id).execute.find_by(iid: params[:type_id])
+    when 'MergeRequest'
+      MergeRequestsFinder.new(current_user, project_id: @project.id).execute.find_by(iid: params[:type_id])
+    when 'Commit'
+      @project.commit(params[:type_id])
+    end
+  end
+end

app/controllers/projects_controller.rb

@@ -127,39 +127,6 @@ class ProjectsController < Projects::ApplicationController
     redirect_to edit_project_path(@project), alert: ex.message
   end
 
-  def autocomplete_sources
-    noteable =
-      case params[:type]
-      when 'Issue'
-        IssuesFinder.new(current_user, project_id: @project.id).
-          execute.find_by(iid: params[:type_id])
-      when 'MergeRequest'
-        MergeRequestsFinder.new(current_user, project_id: @project.id).
-          execute.find_by(iid: params[:type_id])
-      when 'Commit'
-        @project.commit(params[:type_id])
-      else
-        nil
-      end
-
-    autocomplete = ::Projects::AutocompleteService.new(@project, current_user)
-    participants = ::Projects::ParticipantsService.new(@project, current_user).execute(noteable)
-
-    @suggestions = {
-      emojis: Gitlab::AwardEmoji.urls,
-      issues: autocomplete.issues,
-      milestones: autocomplete.milestones,
-      mergerequests: autocomplete.merge_requests,
-      labels: autocomplete.labels,
-      members: participants,
-      commands: autocomplete.commands(noteable, params[:type])
-    }
-
-    respond_to do |format|
-      format.json { render json: @suggestions }
-    end
-  end
-
   def new_issue_address
     return render_404 unless Gitlab::IncomingEmail.supports_issue_creation?
 

app/views/layouts/_init_auto_complete.html.haml

@@ -3,6 +3,14 @@
 
 - if project
   :javascript
-    GitLab.GfmAutoComplete.dataSource = "#{autocomplete_sources_namespace_project_path(project.namespace, project, type: noteable_type, type_id: params[:id])}"
+    gl.GfmAutoComplete.dataSources = {
-    GitLab.GfmAutoComplete.cachedData = undefined;
+      emojis: "#{emojis_namespace_project_autocomplete_sources_path(project.namespace, project)}",
-    GitLab.GfmAutoComplete.setup();
+      members: "#{members_namespace_project_autocomplete_sources_path(project.namespace, project, type: noteable_type, type_id: params[:id])}",
+      issues: "#{issues_namespace_project_autocomplete_sources_path(project.namespace, project)}",
+      mergeRequests: "#{merge_requests_namespace_project_autocomplete_sources_path(project.namespace, project)}",
+      labels: "#{labels_namespace_project_autocomplete_sources_path(project.namespace, project)}",
+      milestones: "#{milestones_namespace_project_autocomplete_sources_path(project.namespace, project)}",
+      commands: "#{commands_namespace_project_autocomplete_sources_path(project.namespace, project, type: noteable_type, type_id: params[:id])}"
+    };
+
+    gl.GfmAutoComplete.setup();

changelogs/unreleased/18435-autocomplete-is-not-performant.yml

@@ -0,0 +1,4 @@
+---
+title: Made comment autocomplete more performant and removed some loading bugs
+merge_request: 6856
+author: 

config/routes/project.rb

@@ -11,6 +11,18 @@ constraints(ProjectUrlConstrainer.new) do
           module: :projects,
           as: :project) do
 
+      resources :autocomplete_sources, only: [] do
+        collection do
+          get 'emojis'
+          get 'members'
+          get 'issues'
+          get 'merge_requests'
+          get 'labels'
+          get 'milestones'
+          get 'commands'
+        end
+      end
+
       #
       # Templates
       #
@@ -316,7 +328,6 @@ constraints(ProjectUrlConstrainer.new) do
         post :remove_export
         post :generate_new_export
         get :download_export
-        get :autocomplete_sources
         get :activity
         get :refs
         put :new_issue_address

spec/features/participants_autocomplete_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 feature 'Member autocomplete', feature: true do
+  include WaitForAjax
+
   let(:project) { create(:project, :public) }
   let(:user) { create(:user) }
   let(:participant) { create(:user) }
@@ -79,11 +81,10 @@ feature 'Member autocomplete', feature: true do
   end
 
   def open_member_suggestions
-    sleep 1
     page.within('.new-note') do
-      sleep 1
+      find('#note_note').send_keys('@')
-      find('#note_note').native.send_keys('@')
     end
+    wait_for_ajax
   end
 
   def visit_issue(project, issue)

spec/features/projects/gfm_autocomplete_load_spec.rb

@@ -10,12 +10,12 @@ describe 'GFM autocomplete loading', feature: true, js: true do
   end
 
   it 'does not load on project#show' do
-    expect(evaluate_script('GitLab.GfmAutoComplete.dataSource')).to eq('')
+    expect(evaluate_script('gl.GfmAutoComplete.dataSources')).to eq({})
   end
 
   it 'loads on new issue page' do
     visit new_namespace_project_issue_path(project.namespace, project)
 
-    expect(evaluate_script('GitLab.GfmAutoComplete.dataSource')).not_to eq('')
+    expect(evaluate_script('gl.GfmAutoComplete.dataSources')).not_to eq({})
   end
 end

spec/routing/project_routing_spec.rb

@@ -80,10 +80,6 @@ describe 'project routing' do
       expect(get('/gitlab/gitlabhq/edit')).to route_to('projects#edit', namespace_id: 'gitlab', id: 'gitlabhq')
     end
 
-    it 'to #autocomplete_sources' do
-      expect(get('/gitlab/gitlabhq/autocomplete_sources')).to route_to('projects#autocomplete_sources', namespace_id: 'gitlab', id: 'gitlabhq')
-    end
-
     describe 'to #show' do
       context 'regular name' do
         it { expect(get('/gitlab/gitlabhq')).to route_to('projects#show', namespace_id: 'gitlab', id: 'gitlabhq') }
@@ -117,6 +113,21 @@ describe 'project routing' do
     end
   end
 
+  # emojis_namespace_project_autocomplete_sources_path         GET /:project_id/autocomplete_sources/emojis(.:format)         projects/autocomplete_sources#emojis
+  # members_namespace_project_autocomplete_sources_path        GET /:project_id/autocomplete_sources/members(.:format)        projects/autocomplete_sources#members
+  # issues_namespace_project_autocomplete_sources_path         GET /:project_id/autocomplete_sources/issues(.:format)         projects/autocomplete_sources#issues
+  # merge_requests_namespace_project_autocomplete_sources_path GET /:project_id/autocomplete_sources/merge_requests(.:format) projects/autocomplete_sources#merge_requests
+  # labels_namespace_project_autocomplete_sources_path         GET /:project_id/autocomplete_sources/labels(.:format)         projects/autocomplete_sources#labels
+  # milestones_namespace_project_autocomplete_sources_path     GET /:project_id/autocomplete_sources/milestones(.:format)     projects/autocomplete_sources#milestones
+  # commands_namespace_project_autocomplete_sources_path       GET /:project_id/autocomplete_sources/commands(.:format)       projects/autocomplete_sources#commands
+  describe Projects::AutocompleteSourcesController, 'routing' do
+    [:emojis, :members, :issues, :merge_requests, :labels, :milestones, :commands].each do |action|
+      it "to ##{action}" do
+        expect(get("/gitlab/gitlabhq/autocomplete_sources/#{action}")).to route_to("projects/autocomplete_sources##{action}", namespace_id: 'gitlab', project_id: 'gitlabhq')
+      end
+    end
+  end
+
   #  pages_project_wikis GET    /:project_id/wikis/pages(.:format)       projects/wikis#pages
   # history_project_wiki GET    /:project_id/wikis/:id/history(.:format) projects/wikis#history
   #        project_wikis POST   /:project_id/wikis(.:format)             projects/wikis#create