Reset the progressive search cache from the received page, not the patched results state

packages/core-data/src/hooks/ProgressiveSearch.js

@@ -1,13 +1,17 @@
 // @flow
 
-import { dequal } from 'dequal/lite';
 import {
   useCallback,
   useEffect,
   useRef,
   useState
 } from 'react';
 import type { SearchResult } from '../types/typesense/SearchResult';
+import {
+  CACHE_ACTION_RESET,
+  CACHE_ACTION_SKIP,
+  getCacheAction
+} from '../utils/ProgressiveSearch';
 import TypesenseUtils from '../utils/Typesense';
 
 type OnCompleteCallback = (results: Array<SearchResult>) => void;
@@ -16,7 +20,7 @@ const useProgressiveSearch = (infiniteHits, transformResults = null) => {
   const [cachedHits, setCachedHits] = useState(TypesenseUtils.createCachedHits([]));
   const [searching, setSearching] = useState(false);
 
-  const lastSearchState = useRef<any>();
+  const lastResults = useRef<any>();
   const callbacks = useRef<Array<OnCompleteCallback>>([]);
 
   /**
@@ -37,27 +41,6 @@ const useProgressiveSearch = (infiniteHits, transformResults = null) => {
     callbacks.current = callbacks.current.filter((c) => c !== callback);
   }, []);
 
-  /**
-   * Returns true if the state has changed.
-   *
-   * @param a
-   * @param b
-   * @param ignorePageNo
-   *
-   * @returns {boolean}
-   */
-  const hasStateChanged = (a: any, b: any, ignorePageNo?: boolean) => {
-    if (ignorePageNo && a) {
-      delete a.page;
-    }
-
-    if (ignorePageNo && b) {
-      delete b.page;
-    }
-
-    return !dequal(a, b);
-  };
-
   /**
    * Returns the transformed hits if the callback is provided. Otherwise, the untransformed hits are returned.
    *
@@ -75,40 +58,46 @@ const useProgressiveSearch = (infiniteHits, transformResults = null) => {
     return value;
   };
 
+  /**
+   * Adds the hits from each newly received page to the cache. A first page starts the cache over; later pages
+   * are appended. The decision is made from the results object and its page number rather than from
+   * `results._state`, because InstantSearch patches that state onto the previous results while a new search is
+   * in flight (see `getCacheAction`).
+   */
   useEffect(() => {
-    const { isFirstPage, results } = infiniteHits;
-    const hits = getHits(results);
+    const { results } = infiniteHits;
+    const action = getCacheAction(results, lastResults.current);
 
-    if (isFirstPage) {
-      setSearching(true);
+    if (action === CACHE_ACTION_SKIP) {
+      return;
     }
 
-    // Add to cache and load next page
-    if (isFirstPage && hasStateChanged(results._state, lastSearchState.current, true)) {
+    lastResults.current = results;
+
+    const hits = getHits(results);
+
+    if (action === CACHE_ACTION_RESET) {
+      setSearching(true);
       setCachedHits(() => TypesenseUtils.createCachedHits(hits));
     } else {
       setCachedHits(({ merge }) => merge(hits));
     }
   }, [infiniteHits.results]);
 
+  /**
+   * Loads the next page after each page is cached. Once the last page is in the cache, notifies the observers
+   * with the complete set of hits.
+   */
   useEffect(() => {
-    const { isLastPage, results } = infiniteHits;
-    const hits = getHits(results);
-
-    if (!isLastPage && infiniteHits.showMore) {
-      setTimeout(() => infiniteHits.showMore(), 25);
-    } else if (hasStateChanged(results._state, lastSearchState.current)) {
-      callbacks.current.forEach((callback) => {
-        const merged = cachedHits.merge(hits);
-        callback(merged.hits);
-      });
-    }
+    const { isLastPage, showMore } = infiniteHits;
 
-    if (isLastPage) {
-      setSearching(false);
+    if (!isLastPage && showMore) {
+      setTimeout(() => showMore(), 25);
+      return;
     }
 
-    lastSearchState.current = results._state;
+    callbacks.current.forEach((callback) => callback(cachedHits.hits));
+    setSearching(false);
   }, [cachedHits]);
 
   return {

packages/core-data/src/utils/ProgressiveSearch.js

@@ -0,0 +1,27 @@
+// @flow
+
+export const CACHE_ACTION_MERGE = 'merge';
+export const CACHE_ACTION_RESET = 'reset';
+export const CACHE_ACTION_SKIP = 'skip';
+
+/**
+ * Decides what a progressive search should do with a results object it has just been rendered with.
+ *
+ * InstantSearch re-renders widgets with the previous results while a new search is in flight, and it patches
+ * `results._state` on that previous object to the current state so the UI looks up to date (its "optimistic UI").
+ * The state on a results object therefore does not identify the search that produced its hits. What does is the
+ * results object itself, which InstantSearch replaces only when a response arrives, and the page number of that
+ * response, which comes from the response rather than from the patched state.
+ *
+ * @param results          The results the hook was rendered with.
+ * @param previousResults  The results object the hook processed last.
+ *
+ * @returns {string} `reset` when a first page arrives, `merge` when a later page arrives, `skip` otherwise.
+ */
+export const getCacheAction = (results: any, previousResults: any): string => {
+  if (!results || results === previousResults || results.__isArtificial) {
+    return CACHE_ACTION_SKIP;
+  }
+
+  return (results.page || 0) === 0 ? CACHE_ACTION_RESET : CACHE_ACTION_MERGE;
+};

packages/core-data/src/utils/ProgressiveSearch.spec.js

@@ -0,0 +1,41 @@
+import { getCacheAction } from './ProgressiveSearch';
+
+const results = (page, extra = {}) => ({ page, hits: [], ...extra });
+
+describe('getCacheAction', () => {
+  it('skips when there are no results yet', () => {
+    expect(getCacheAction(undefined, undefined)).toEqual('skip');
+    expect(getCacheAction(null, undefined)).toEqual('skip');
+  });
+
+  it('skips a re-render of the results object already processed', () => {
+    const first = results(0);
+    expect(getCacheAction(first, first)).toEqual('skip');
+  });
+
+  it('skips the artificial results InstantSearch renders before a search completes', () => {
+    expect(getCacheAction(results(0, { __isArtificial: true }), undefined)).toEqual('skip');
+  });
+
+  it('resets the cache when a first page arrives', () => {
+    expect(getCacheAction(results(0), undefined)).toEqual('reset');
+    expect(getCacheAction(results(undefined), undefined)).toEqual('reset');
+  });
+
+  it('merges when a later page arrives', () => {
+    expect(getCacheAction(results(1), results(0))).toEqual('merge');
+    expect(getCacheAction(results(2), results(1))).toEqual('merge');
+  });
+
+  it('resets again when a new search starts, even if the state was patched to look unchanged', () => {
+    // InstantSearch patches `_state` on the previous results to the current state
+    // (optimistic UI), so two different results objects can carry an identical `_state`.
+    // Only the page number of the received results says whether a new search began.
+    const state = { disjunctiveFacetsRefinements: { facet: ['value'] }, page: 0 };
+    const stale = results(0, { _state: state, hits: [{ uuid: 'a' }, { uuid: 'b' }] });
+    const fresh = results(0, { _state: state, hits: [{ uuid: 'a' }] });
+
+    expect(getCacheAction(stale, undefined)).toEqual('reset');
+    expect(getCacheAction(fresh, stale)).toEqual('reset');
+  });
+});