Fix loading indicator for uiSettingsClient (#18694)

* [ui/chrome/loadingIndicator] expose VanillaJS hooks for gloabl loading indicator

* [ui/uiSettingsClient] increment/decrement loading count while request in progress

* [ui/loadingIndicator] fix HeaderPage.isGlobalLoadingIndicatorHidden()

* [ui/loadingIndicator] remove rxjs, fix tests

* [ui/loadingIndicator] improve docs

* [ui/loadingIndicator] remove needless unmounts

* [ui/loadingCount] correct typo

* [functionalTests/headerPage] awaitGlobalLoadingIndicatorHidden

* [ui/loadingCount] send subscribers the loading count on subscription

* [ui/loadingIndicator] remove error throwing, cleanup render

(cherry picked from commit d35316642d)

src/ui/public/chrome/__tests__/kbn_loading_indicator.js

@@ -1,35 +0,0 @@
-import ngMock from 'ng_mock';
-import expect from 'expect.js';
-import $ from 'jquery';
-
-import '../directives/loading_indicator/loading_indicator';
-
-describe('kbnLoadingIndicator', function () {
-  let compile;
-
-  beforeEach(() => {
-    ngMock.module('kibana');
-    ngMock.inject(function ($compile, $rootScope) {
-      compile = function (hasActiveConnections) {
-        $rootScope.chrome = {
-          httpActive: hasActiveConnections ? [1] : []
-        };
-        const $el = $('<kbn-loading-indicator></kbn-loading-indicator>');
-        $compile($el)($rootScope);
-        $rootScope.$apply();
-        return $el;
-      };
-    });
-
-  });
-
-  it(`doesn't have ng-hide class when there are connections`, function () {
-    const $el = compile(true);
-    expect($el.hasClass('ng-hide')).to.be(false);
-  });
-
-  it('has ng-hide class when there are no connections', function () {
-    const $el = compile(false);
-    expect($el.hasClass('ng-hide')).to.be(true);
-  });
-});

src/ui/public/chrome/api/angular.js

@@ -39,6 +39,7 @@ export function initAngularApi(chrome, internals) {
 
         $locationProvider.hashPrefix('');
       })
+      .run(internals.capture$httpLoadingCount)
       .run(($location, $rootScope, Private, config) => {
         chrome.getFirstPathSegment = () => {
           return $location.path().split('/')[1];

src/ui/public/chrome/api/loading_count.js

@@ -0,0 +1,73 @@
+import { isSystemApiRequest } from '../../system_api';
+
+export function initLoadingCountApi(chrome, internals) {
+  const counts = { angular: 0, manual: 0 };
+  const handlers = new Set();
+
+  function getCount() {
+    return counts.angular + counts.manual;
+  }
+
+  // update counts and call handlers with sum if there is a change
+  function update(name, count) {
+    if (counts[name] === count) {
+      return;
+    }
+
+    counts[name] = count;
+    for (const handler of handlers) {
+      handler(getCount());
+    }
+  }
+
+  /**
+   * Injected into angular module by ui/chrome angular integration
+   * and adds a root-level watcher that will capture the count of
+   * active $http requests on each digest loop
+   * @param  {Angular.Scope} $rootScope
+   * @param  {HttpService} $http
+   * @return {undefined}
+   */
+  internals.capture$httpLoadingCount = function ($rootScope, $http) {
+    $rootScope.$watch(() => {
+      const reqs = $http.pendingRequests || [];
+      update('angular', reqs.filter(req => !isSystemApiRequest(req)).length);
+    });
+  };
+
+  chrome.loadingCount = new class ChromeLoadingCountApi {
+    /**
+     * Call to add a subscriber to for the loading count that
+     * will be called every time the loading count changes.
+     *
+     * @type {Observable<number>}
+     * @return {Function} unsubscribe
+     */
+    subscribe(handler) {
+      handlers.add(handler);
+
+      // send the current count to the handler
+      handler(getCount());
+
+      return () => {
+        handlers.delete(handler);
+      };
+    }
+
+    /**
+     * Increment the loading count by one
+     * @return {undefined}
+     */
+    increment() {
+      update('manual', counts.manual + 1);
+    }
+
+    /**
+     * Decrement the loading count by one
+     * @return {undefined}
+     */
+    decrement() {
+      update('manual', counts.manual - 1);
+    }
+  };
+}

src/ui/public/chrome/chrome.js

@@ -23,6 +23,7 @@ import themeApi from './api/theme';
 import translationsApi from './api/translations';
 import { initChromeXsrfApi } from './api/xsrf';
 import { initUiSettingsApi } from './api/ui_settings';
+import { initLoadingCountApi } from './api/loading_count';
 
 export const chrome = {};
 const internals = _.defaults(
@@ -44,6 +45,7 @@ initUiSettingsApi(chrome);
 appsApi(chrome, internals);
 initChromeXsrfApi(chrome, internals);
 initChromeNavApi(chrome, internals);
+initLoadingCountApi(chrome, internals);
 initAngularApi(chrome, internals);
 controlsApi(chrome, internals);
 templateApi(chrome, internals);

src/ui/public/chrome/directives/__snapshots__/loading_indicator.test.js.snap

@@ -0,0 +1,23 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`kbnLoadingIndicator is hidden by default 1`] = `
+<div
+  className="loadingIndicator hidden"
+  data-test-subj="globalLoadingIndicator-hidden"
+>
+  <div
+    className="loadingIndicator__bar"
+  />
+</div>
+`;
+
+exports[`kbnLoadingIndicator is visible when loadingCount is > 0 1`] = `
+<div
+  className="loadingIndicator"
+  data-test-subj="globalLoadingIndicator"
+>
+  <div
+    className="loadingIndicator__bar"
+  />
+</div>
+`;

src/ui/public/chrome/directives/index.js

@@ -3,7 +3,7 @@ import './global_nav';
 
 import { kbnChromeProvider } from './kbn_chrome';
 import { kbnAppendChromeNavControls } from './append_nav_controls';
-import './loading_indicator/loading_indicator';
+import './loading_indicator';
 
 export function directivesProvider(chrome, internals) {
   kbnChromeProvider(chrome, internals);

src/ui/public/chrome/directives/kbn_chrome.js

@@ -1,11 +1,9 @@
 import React from 'react';
 import ReactDOM from 'react-dom';
 import $ from 'jquery';
-import { remove } from 'lodash';
 
 import './kbn_chrome.less';
 import { uiModules } from '../../modules';
-import { isSystemApiRequest } from '../../system_api';
 import {
   getUnhashableStatesProvider,
   unhashUrl,
@@ -68,13 +66,6 @@ export function kbnChromeProvider(chrome, internals) {
           $rootScope.$on('$routeUpdate', onRouteChange);
           updateSubUrls(); // initialize sub urls
 
-          const allPendingHttpRequests = () => $http.pendingRequests;
-          const removeSystemApiRequests = (pendingHttpRequests = []) => remove(pendingHttpRequests, isSystemApiRequest);
-          $scope.$watchCollection(allPendingHttpRequests, removeSystemApiRequests);
-
-          // and some local values
-          chrome.httpActive = $http.pendingRequests;
-
           // Notifications
           $scope.notifList = notify._notifs;
 

src/ui/public/chrome/directives/loading_indicator.js

@@ -0,0 +1,48 @@
+import 'ngreact';
+import React from 'react';
+import classNames from 'classnames';
+
+import { uiModules } from 'ui/modules';
+import chrome from 'ui/chrome';
+
+import './loading_indicator.less';
+
+export class LoadingIndicator extends React.Component {
+  state = {
+    visible: false,
+  }
+
+  componentDidMount() {
+    this._unsub = chrome.loadingCount.subscribe((count) => {
+      this.setState({
+        visible: count > 0
+      });
+    });
+  }
+
+  componentWillUnmount() {
+    this._unsub();
+    this._unsub = null;
+  }
+
+  render() {
+    const className = classNames(
+      'loadingIndicator',
+      this.state.visible ? null : 'hidden'
+    );
+
+    const testSubj = this.state.visible
+      ? 'globalLoadingIndicator'
+      : 'globalLoadingIndicator-hidden';
+
+    return (
+      <div className={className} data-test-subj={testSubj}>
+        <div className="loadingIndicator__bar" />
+      </div>
+    );
+  }
+}
+
+uiModules
+  .get('app/kibana', ['react'])
+  .directive('kbnLoadingIndicator', reactDirective => reactDirective(LoadingIndicator));

src/ui/public/chrome/directives/loading_indicator.less

@@ -18,7 +18,7 @@
   overflow: hidden; // 2
   height: @loadingIndicatorHeight;
 
-  &.ng-hide {
+  &.hidden {
     visibility: hidden;
     opacity: 0;
     transition-delay: 0.25s;

src/ui/public/chrome/directives/loading_indicator.test.js

@@ -0,0 +1,38 @@
+import React from 'react';
+import { shallow } from 'enzyme';
+import chrome from 'ui/chrome';
+
+import { LoadingIndicator } from './loading_indicator';
+
+jest.mock('ui/chrome', () => {
+  return {
+    loadingCount: {
+      subscribe: jest.fn(() => {
+        return () => {};
+      })
+    }
+  };
+});
+
+beforeEach(() => {
+  chrome.loadingCount.subscribe.mockClear();
+});
+
+describe('kbnLoadingIndicator', function () {
+  it('is hidden by default', function () {
+    const wrapper = shallow(<LoadingIndicator />);
+    expect(wrapper.prop('data-test-subj')).toBe('globalLoadingIndicator-hidden');
+    expect(wrapper).toMatchSnapshot();
+  });
+
+  it('is visible when loadingCount is > 0', () => {
+    chrome.loadingCount.subscribe.mockImplementation((fn) => {
+      fn(1);
+      return () => {};
+    });
+
+    const wrapper = shallow(<LoadingIndicator />);
+    expect(wrapper.prop('data-test-subj')).toBe('globalLoadingIndicator');
+    expect(wrapper).toMatchSnapshot();
+  });
+});

src/ui/public/chrome/directives/loading_indicator/index.js

@@ -1 +0,0 @@
-import './loading_indicator';

src/ui/public/chrome/directives/loading_indicator/loading_indicator.html

@@ -1,7 +0,0 @@
-<div
-  class="loadingIndicator"
-  ng-show="chrome.httpActive.length"
-  data-test-subj="globalLoadingIndicator"
->
-  <div class="loadingIndicator__bar"></div>
-</div>

src/ui/public/chrome/directives/loading_indicator/loading_indicator.js

@@ -1,13 +0,0 @@
-import { uiModules } from '../../../modules';
-import template from './loading_indicator.html';
-import './loading_indicator.less';
-
-uiModules
-  .get('ui/kibana')
-  .directive('kbnLoadingIndicator', function () {
-    return {
-      restrict: 'E',
-      replace: true,
-      template,
-    };
-  });

src/ui/ui_settings/public/send_request.js

@@ -1,20 +1,25 @@
 import chrome from 'ui/chrome';
 
 export async function sendRequest({ method, path, body }) {
-  const response = await fetch(chrome.addBasePath(path), {
-    method,
-    body: JSON.stringify(body),
-    headers: {
-      accept: 'application/json',
-      'content-type': 'application/json',
-      'kbn-xsrf': 'kibana',
-    },
-    credentials: 'same-origin'
-  });
+  chrome.loadingCount.increment();
+  try {
+    const response = await fetch(chrome.addBasePath(path), {
+      method,
+      body: JSON.stringify(body),
+      headers: {
+        accept: 'application/json',
+        'content-type': 'application/json',
+        'kbn-xsrf': 'kibana',
+      },
+      credentials: 'same-origin'
+    });
 
-  if (response.status >= 300) {
-    throw new Error(`Request failed with status code: ${response.status}`);
+    if (response.status >= 300) {
+      throw new Error(`Request failed with status code: ${response.status}`);
+    }
+
+    return await response.json();
+  } finally {
+    chrome.loadingCount.decrement();
   }
-
-  return await response.json();
 }

test/functional/apps/dashboard/_dashboard_save.js

@@ -56,7 +56,7 @@ export default function ({ getService, getPageObjects }) {
     it('Does not warn when you save an existing dashboard with the title it already has, and that title is a duplicate',
       async function () {
         await PageObjects.dashboard.selectDashboard(dashboardName);
-        await PageObjects.header.isGlobalLoadingIndicatorHidden();
+        await PageObjects.header.awaitGlobalLoadingIndicatorHidden();
         await PageObjects.dashboard.clickEdit();
         await PageObjects.dashboard.saveDashboard(dashboardName);
 

test/functional/apps/visualize/_point_series_options.js

@@ -75,7 +75,7 @@ export default function ({ getService, getPageObjects }) {
           return PageObjects.visualize.clickGo();
         })
         .then(function () {
-          return PageObjects.header.isGlobalLoadingIndicatorHidden();
+          return PageObjects.header.awaitGlobalLoadingIndicatorHidden();
         });
     });
 

test/functional/page_objects/header_page.js

@@ -20,27 +20,27 @@ export function HeaderPageProvider({ getService, getPageObjects }) {
       log.debug('click Discover tab');
       await this.clickSelector('a[href*=\'discover\']');
       await PageObjects.common.waitForTopNavToBeVisible();
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async clickVisualize() {
       log.debug('click Visualize tab');
       await this.clickSelector('a[href*=\'visualize\']');
       await PageObjects.common.waitForTopNavToBeVisible();
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async clickDashboard() {
       log.debug('click Dashboard tab');
       await this.clickSelector('a[href*=\'dashboard\']');
       await PageObjects.common.waitForTopNavToBeVisible();
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async clickManagement() {
       log.debug('click Management tab');
       await this.clickSelector('a[href*=\'management\']');
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async clickSettings() {
@@ -158,7 +158,7 @@ export function HeaderPageProvider({ getService, getPageObjects }) {
       log.debug('--Setting To Time : ' + toTime);
       await this.setToTime(toTime);
       await this.clickGoButton();
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async setQuickTime(quickTime) {
@@ -225,7 +225,7 @@ export function HeaderPageProvider({ getService, getPageObjects }) {
           throw exception;
         }
       }
-      await this.isGlobalLoadingIndicatorHidden();
+      await this.awaitGlobalLoadingIndicatorHidden();
     }
 
     async isGlobalLoadingIndicatorVisible() {
@@ -233,9 +233,9 @@ export function HeaderPageProvider({ getService, getPageObjects }) {
       return await testSubjects.exists('globalLoadingIndicator');
     }
 
-    async isGlobalLoadingIndicatorHidden() {
-      log.debug('isGlobalLoadingIndicatorHidden');
-      return await find.byCssSelector('[data-test-subj="globalLoadingIndicator"].ng-hide', defaultFindTimeout * 10);
+    async awaitGlobalLoadingIndicatorHidden() {
+      log.debug('awaitGlobalLoadingIndicatorHidden');
+      await testSubjects.find('globalLoadingIndicator-hidden', defaultFindTimeout * 10);
     }
 
     async getPrettyDuration() {

x-pack/test/functional/page_objects/security_page.js

@@ -346,7 +346,7 @@ export function SecurityPageProvider({ getService, getPageObjects }) {
       log.debug('Delete user ' + username);
       return remote.findDisplayedByLinkText(username).click()
         .then(() => {
-          return PageObjects.header.isGlobalLoadingIndicatorHidden();
+          return PageObjects.header.awaitGlobalLoadingIndicatorHidden();
         })
         .then(() => {
           log.debug('Find delete button and click');