Merge pull request #2295 from hypothesis/disable-separate-replies

Disable `_separate_replies` for annotation loading in sidebar (behind feature flag)

src/sidebar/search-client.js

 import { TinyEmitter } from 'tiny-emitter';
 
+/**
+ * @typedef {import('../types/api').Annotation} Annotation
+ */
+
 /**
  * Client for the Hypothesis search API.
  *
@@ -8,21 +12,28 @@ import { TinyEmitter } from 'tiny-emitter';
 export default class SearchClient extends TinyEmitter {
   /**
    * @param {Object} searchFn - Function for querying the search API
-   * @param {Object} opts - Search options
+   * @param {Object} options
+   *   @param {number} [options.chunkSize] - page size/number of annotations
+   *   per batch
+   *   @param {boolean} [options.separateReplies] - When `true`, request that
+   *   top-level annotations and replies be returned separately.
+   *   NOTE: This has issues with annotations that have large numbers of
+   *   replies.
+   *   @param {boolean} [options.incremental] - Emit `results` events incrementally
+   *   as batches of annotations are available
    */
-  constructor(searchFn, opts) {
+  constructor(
+    searchFn,
+    { chunkSize = 200, separateReplies = true, incremental = true } = {}
+  ) {
     super();
-    opts = opts || {};
-
-    const DEFAULT_CHUNK_SIZE = 200;
     this._searchFn = searchFn;
-    this._chunkSize = opts.chunkSize || DEFAULT_CHUNK_SIZE;
-    if (typeof opts.incremental !== 'undefined') {
-      this._incremental = opts.incremental;
-    } else {
-      this._incremental = true;
-    }
+    this._chunkSize = chunkSize;
+    this._separateReplies = separateReplies;
+    this._incremental = incremental;
+
     this._canceled = false;
+    /** @type {Annotation[]} */
     this._results = [];
   }
 
@@ -33,7 +44,7 @@ export default class SearchClient extends TinyEmitter {
         offset: offset,
         sort: 'created',
         order: 'asc',
-        _separate_replies: true,
+        _separate_replies: this._separateReplies,
       },
       query
     );

src/sidebar/services/load-annotations.js

 /**
  * A service for fetching annotations, filtered by document URIs and group.
  */
+
 import SearchClient from '../search-client';
 
 import { isReply } from '../util/annotation-metadata';
@@ -8,6 +9,7 @@ import { isReply } from '../util/annotation-metadata';
 // @ngInject
 export default function loadAnnotationsService(
   api,
+  features,
   store,
   streamer,
   streamFilter
@@ -39,6 +41,7 @@ export default function loadAnnotationsService(
   function searchAndLoad(uris, groupId) {
     searchClient = new SearchClient(api.search, {
       incremental: true,
+      separateReplies: !features.flagEnabled('client_do_not_separate_replies'),
     });
     searchClient.on('results', results => {
       if (results.length) {

src/sidebar/services/test/load-annotations-test.js

@@ -12,6 +12,7 @@ class FakeSearchClient extends EventEmitter {
     searchClients.push(this);
     this.cancel = sinon.stub();
     this.incremental = !!opts.incremental;
+    this.separateReplies = !!opts.separateReplies;
 
     this.get = sinon.spy(query => {
       assert.ok(query.uri);
@@ -30,6 +31,7 @@ class FakeSearchClient extends EventEmitter {
 
 describe('loadAnnotationsService', () => {
   let fakeApi;
+  let fakeFeatures;
   let fakeStore;
   let fakeStreamer;
   let fakeStreamFilter;
@@ -49,6 +51,10 @@ describe('loadAnnotationsService', () => {
       },
     };
 
+    fakeFeatures = {
+      flagEnabled: sinon.stub().returns(false),
+    };
+
     fakeStore = {
       addAnnotations: sinon.stub(),
       annotationFetchFinished: sinon.stub(),
@@ -94,6 +100,7 @@ describe('loadAnnotationsService', () => {
     );
     return loadAnnotationsService(
       fakeApi,
+      fakeFeatures,
       fakeStore,
       fakeStreamer,
       fakeStreamFilter
@@ -217,6 +224,26 @@ describe('loadAnnotationsService', () => {
       assert.ok(searchClients[0].incremental);
     });
 
+    it('loads annotations without separating replies if feature flag is enabled', () => {
+      fakeFeatures.flagEnabled
+        .withArgs('client_do_not_separate_replies')
+        .returns(true);
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.isFalse(searchClients[0].separateReplies);
+    });
+
+    it('loads annotations with separated replies if feature flag is not enabled', () => {
+      fakeFeatures.flagEnabled
+        .withArgs('client_do_not_separate_replies')
+        .returns(false);
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.isTrue(searchClients[0].separateReplies);
+    });
+
     it("cancels previously search client if it's still running", () => {
       const svc = createService();