Replace `FRAME_CONNECTED` event in groups service

src/sidebar/services/groups.js

@@ -3,6 +3,7 @@ import serviceConfig from '../service-config';
 import { isReply } from '../util/annotation-metadata';
 import { combineGroups } from '../util/groups';
 import { awaitStateChange } from '../util/state';
+import { watch } from '../util/watch';
 
 const DEFAULT_ORG_ID = '__default__';
 
@@ -31,14 +32,19 @@ export default function groups(
   const svc = serviceConfig(settings);
   const authority = svc ? svc.authority : null;
 
-  function getDocumentUriForGroupSearch() {
-    function mainUri() {
-      const mainFrame = store.mainFrame();
-      if (!mainFrame) {
-        return null;
-      }
-      return mainFrame.uri;
+  /**
+   * Return the main document URI that is used to fetch groups associated with
+   * the site that the user is on.
+   */
+  function mainUri() {
+    const mainFrame = store.mainFrame();
+    if (!mainFrame) {
+      return null;
     }
+    return mainFrame.uri;
+  }
+
+  function getDocumentUriForGroupSearch() {
     return awaitStateChange(store, mainUri);
   }
 
@@ -167,11 +173,7 @@ export default function groups(
     });
   }
 
-  // The document URI passed to the most recent `GET /api/groups` call in order
-  // to include groups associated with this page. This is retained to determine
-  // whether we need to re-fetch groups if the URLs of frames connected to the
-  // sidebar app changes.
-  let documentUri = null;
+  let didSubscribeToUriChanges = false;
 
   /*
    * Fetch an individual group.
@@ -193,15 +195,27 @@ export default function groups(
    * The groups that are fetched depend on the current user, the URI of
    * the current document, and the direct-linked group and/or annotation.
    *
+   * On startup, `load()` must be called to trigger the initial groups fetch.
+   * Subsequently groups are automatically reloaded if the logged-in user or
+   * main document URI changes.
+   *
    * @return {Promise<Group[]>}
    */
   async function load() {
     // Step 1: Get the URI of the active document, so we can fetch groups
     // associated with that document.
+    let documentUri;
     if (isSidebar) {
       documentUri = await getDocumentUriForGroupSearch();
     }
 
+    if (!didSubscribeToUriChanges) {
+      didSubscribeToUriChanges = true;
+      watch(store.subscribe, mainUri, () => {
+        load();
+      });
+    }
+
     // Step 2: Concurrently fetch the groups the user is a member of,
     // the groups associated with the current document and the annotation
     // and/or group that was direct-linked (if any).
@@ -362,16 +376,6 @@ export default function groups(
     return load();
   });
 
-  // refetch the list of groups when document url changes
-  $rootScope.$on(events.FRAME_CONNECTED, () => {
-    return getDocumentUriForGroupSearch().then(uri => {
-      if (documentUri !== uri) {
-        documentUri = uri;
-        load();
-      }
-    });
-  });
-
   return {
     all: all,
     get: get,

src/sidebar/services/test/groups-test.js

 import events from '../../events';
 import fakeReduxStore from '../../test/fake-redux-store';
 import groups, { $imports } from '../groups';
+import { waitFor } from '../../../test-util/wait';
 
 /**
  * Generate a truth table containing every possible combination of a set of
@@ -108,7 +109,7 @@ describe('groups', function () {
       },
 
       $on: function (event, callback) {
-        if (event === events.USER_CHANGED || event === events.FRAME_CONNECTED) {
+        if (event === events.USER_CHANGED) {
           this.eventCallbacks[event] = callback;
         }
       },
@@ -809,7 +810,7 @@ describe('groups', function () {
     });
   });
 
-  describe('calls load on various events', function () {
+  describe('automatic re-fetching', function () {
     it('refetches groups when the logged-in user changes', () => {
       service();
 
@@ -819,43 +820,46 @@ describe('groups', function () {
     });
 
     context('when a new frame connects', () => {
-      it('should refetch groups if main frame URL has changed', () => {
+      it('should refetch groups if main frame URL has changed', async () => {
         const svc = service();
 
         fakeStore.setState({
           frames: [{ uri: 'https://domain.com/page-a' }],
         });
-        return svc
-          .load()
-          .then(() => {
-            // Simulate main frame URL change, eg. due to client-side navigation in
-            // a single page application.
-            fakeApi.groups.list.resetHistory();
-            fakeStore.setState({
-              frames: [{ uri: 'https://domain.com/page-b' }],
-            });
-
-            return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
-          })
-          .then(() => {
-            assert.calledOnce(fakeApi.groups.list);
-          });
+        await svc.load();
+
+        // Simulate main frame URL change, eg. due to client-side navigation in
+        // a single page application.
+        fakeApi.groups.list.resetHistory();
+        fakeStore.setState({
+          frames: [{ uri: 'https://domain.com/page-b' }],
+        });
+
+        await waitFor(() => fakeApi.groups.list.callCount > 0);
+        assert.calledOnce(fakeApi.groups.list);
       });
 
-      it('should not refetch groups if main frame URL has not changed', () => {
+      it('should not refetch groups if main frame URL has not changed', async () => {
         const svc = service();
 
         fakeStore.setState({
           frames: [{ uri: 'https://domain.com/page-a' }],
         });
-        return svc
-          .load()
-          .then(() => {
-            return fakeRootScope.eventCallbacks[events.FRAME_CONNECTED]();
-          })
-          .then(() => {
-            assert.calledOnce(fakeApi.groups.list);
-          });
+
+        await svc.load();
+        assert.calledOnce(fakeApi.groups.list);
+
+        // A new frame connects, but the main frame URI remains the same.
+        fakeApi.groups.list.resetHistory();
+        fakeStore.setState({
+          frames: [
+            { uri: 'https://domain.com/page-a' },
+            { uri: 'https://domain.com/iframe-b' },
+          ],
+        });
+
+        await new Promise(resolve => setTimeout(resolve, 1));
+        assert.notCalled(fakeApi.groups.list);
       });
     });
   });