Tweak the EntityListProvider to do single-cycle updates.

The old code had some "waterfalls" of things triggering other things. For example, the following could happen in succession based on a single filter changing:

- the filter contents changed
- the client side filtered entities changed
- a backend load was triggered
- the backend responded, which made both the client side filtered entities and the backend entities change

The proposed new code does all of the actual work in one single unit, applying all updates in one go.

Sadly this doesn't entirely address the slowness of the entities table. But it makes updates more consistent and less flickery, and less frequent.

Signed-off-by: Fredrik Adelöw <freben@gmail.com>

.changeset/slimy-kids-attack.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-catalog-react': patch
+---
+
+Tweak the `EntityListProvider` to do single-cycle updates

plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx

@@ -157,6 +157,7 @@ describe('<EntityListProvider/>', () => {
         user: new UserListFilter('owned', mockUser, () => true),
       }),
     );
+    await waitFor(() => result.current.entities.length !== 2);
     expect(mockCatalogApi.getEntities).toHaveBeenCalledTimes(1);
     expect(result.current.entities.length).toBe(1);
   });

plugins/catalog-react/src/hooks/useEntityListProvider.tsx

@@ -14,18 +14,17 @@
  * limitations under the License.
  */
 
+import { Entity } from '@backstage/catalog-model';
+import { useApi } from '@backstage/core';
+import { compact, isEqual } from 'lodash';
 import React, {
   createContext,
   PropsWithChildren,
   useCallback,
   useContext,
-  useEffect,
   useState,
 } from 'react';
 import { useAsyncFn, useDebounce } from 'react-use';
-import { useApi } from '@backstage/core';
-import { Entity } from '@backstage/catalog-model';
-import { reduceCatalogFilters, reduceEntityFilters } from '../utils';
 import { catalogApiRef } from '../api';
 import {
   EntityFilter,
@@ -34,7 +33,7 @@ import {
   EntityTypeFilter,
   UserListFilter,
 } from '../types';
-import { compact, isEqual } from 'lodash';
+import { reduceCatalogFilters, reduceEntityFilters } from '../utils';
 
 export type DefaultEntityFilters = {
   kind?: EntityKindFilter;
@@ -80,51 +79,57 @@ export const EntityListContext = createContext<
   EntityListContextProps<any> | undefined
 >(undefined);
 
+type OutputState<EntityFilters extends DefaultEntityFilters> = {
+  appliedFilters: EntityFilters;
+  entities: Entity[];
+  backendEntities: Entity[];
+};
+
 export const EntityListProvider = <EntityFilters extends DefaultEntityFilters>({
   children,
 }: PropsWithChildren<{}>) => {
   const catalogApi = useApi(catalogApiRef);
+  const [requestedFilters, setRequestedFilters] = useState<EntityFilters>(
+    {} as EntityFilters,
+  );
+  const [outputState, setOutputState] = useState<OutputState<EntityFilters>>({
+    appliedFilters: {} as EntityFilters,
+    entities: [],
+    backendEntities: [],
+  });
 
-  const [filters, setFilters] = useState<EntityFilters>({} as EntityFilters);
-  const [entities, setEntities] = useState<Entity[]>([]);
-  const [backendEntities, setBackendEntities] = useState<Entity[]>([]);
-
-  // Store resolved catalog-backend filters and deep compare on filter updates, to avoid refetching
-  // when only frontend filters change
-  const [backendFilters, setBackendFilters] = useState<
-    Record<string, string | string[]>
-  >(reduceCatalogFilters(compact(Object.values(filters))));
-
-  useEffect(() => {
-    const newBackendFilters = reduceCatalogFilters(
-      compact(Object.values(filters)),
-    );
-    if (!isEqual(newBackendFilters, backendFilters)) {
-      setBackendFilters(newBackendFilters);
-    }
-  }, [backendFilters, filters]);
-
+  // The main async filter worker. Note that while it has a lot of dependencies
+  // in terms of its implementation, the triggering only happens (debounced)
+  // based on the requested filters changing.
   const [{ loading, error }, refresh] = useAsyncFn(async () => {
-    // TODO(timbonicus): should limit fields here, but would need filter fields + table columns
-    const items = await catalogApi
-      .getEntities({
-        filter: backendFilters,
-      })
-      .then(response => response.items);
-    setBackendEntities(items);
-  }, [backendFilters, catalogApi]);
-
-  // Slight debounce on the catalog-backend call, to prevent eager refresh on multiple programmatic
-  // filter changes.
-  useDebounce(refresh, 10, [backendFilters]);
-
-  // Apply frontend filters
-  useEffect(() => {
-    const resolvedEntities = (backendEntities ?? []).filter(
-      reduceEntityFilters(compact(Object.values(filters))),
+    const compacted = compact(Object.values(requestedFilters));
+    const entityFilter = reduceEntityFilters(compacted);
+    const backendFilter = reduceCatalogFilters(compacted);
+    const previousBackendFilter = reduceCatalogFilters(
+      compact(Object.values(outputState.appliedFilters)),
     );
-    setEntities(resolvedEntities);
-  }, [backendEntities, filters]);
+
+    if (!isEqual(previousBackendFilter, backendFilter)) {
+      // TODO(timbonicus): should limit fields here, but would need filter
+      // fields + table columns
+      const response = await catalogApi.getEntities({ filter: backendFilter });
+      setOutputState({
+        appliedFilters: requestedFilters,
+        backendEntities: response.items,
+        entities: response.items.filter(entityFilter),
+      });
+    } else {
+      setOutputState({
+        appliedFilters: requestedFilters,
+        backendEntities: outputState.backendEntities,
+        entities: outputState.backendEntities.filter(entityFilter),
+      });
+    }
+  }, [catalogApi, requestedFilters, outputState]);
+
+  // Slight debounce on the refresh, since (especially on page load) several
+  // filters will be calling this in rapid succession.
+  useDebounce(refresh, 10, [requestedFilters]);
 
   const updateFilters = useCallback(
     (
@@ -132,11 +137,11 @@ export const EntityListProvider = <EntityFilters extends DefaultEntityFilters>({
         | Partial<EntityFilter>
         | ((prevFilters: EntityFilters) => Partial<EntityFilters>),
     ) => {
-      if (typeof update === 'function') {
-        setFilters(prevFilters => ({ ...prevFilters, ...update(prevFilters) }));
-      } else {
-        setFilters(prevFilters => ({ ...prevFilters, ...update }));
-      }
+      setRequestedFilters(prevFilters => {
+        const newFilters =
+          typeof update === 'function' ? update(prevFilters) : update;
+        return { ...prevFilters, ...newFilters };
+      });
     },
     [],
   );
@@ -144,9 +149,9 @@ export const EntityListProvider = <EntityFilters extends DefaultEntityFilters>({
   return (
     <EntityListContext.Provider
       value={{
-        filters,
-        entities,
-        backendEntities,
+        filters: outputState.appliedFilters,
+        entities: outputState.entities,
+        backendEntities: outputState.backendEntities,
         updateFilters,
         loading,
         error,

plugins/catalog/src/components/CatalogPage/CatalogPage.test.tsx

@@ -34,7 +34,7 @@ import {
   renderWithEffects,
   wrapInTestApp,
 } from '@backstage/test-utils';
-import { fireEvent, waitFor } from '@testing-library/react';
+import { fireEvent, screen } from '@testing-library/react';
 import React from 'react';
 import { createComponentRouteRef } from '../../routes';
 import { CatalogPage } from './CatalogPage';
@@ -132,35 +132,38 @@ describe('CatalogPage', () => {
   // related to some theme issues in mui-table
   // https://github.com/mbrn/material-table/issues/1293
   it('should render', async () => {
-    const { getByText, getByTestId } = await renderWrapped(<CatalogPage />);
-    expect(getByText(/Owned \(1\)/)).toBeInTheDocument();
+    const { findByText, getByTestId } = await renderWrapped(<CatalogPage />);
+    await expect(findByText(/Owned \(1\)/)).resolves.toBeInTheDocument();
     fireEvent.click(getByTestId('user-picker-all'));
-    expect(getByText(/All \(2\)/)).toBeInTheDocument();
+    await expect(findByText(/All \(2\)/)).resolves.toBeInTheDocument();
   });
+
   it('should set initial filter correctly', async () => {
-    const { getByText } = await renderWrapped(
+    const { findByText } = await renderWrapped(
       <CatalogPage initiallySelectedFilter="all" />,
     );
-    expect(getByText(/All \(2\)/)).toBeInTheDocument();
+    await expect(findByText(/All \(2\)/)).resolves.toBeInTheDocument();
   });
-  // this test is for fixing the bug after favoriting an entity, the matching entities defaulting
-  // to "owned" filter and not based on the selected filter
-  it('should render the correct entities filtered on the selectedfilter', async () => {
-    const { getByText, findAllByTitle, getByTestId } = await renderWrapped(
-      <CatalogPage />,
-    );
-    expect(getByText(/Owned \(1\)/)).toBeInTheDocument();
-    expect(getByText(/Starred/)).toBeInTheDocument();
-    fireEvent.click(getByTestId('user-picker-starred'));
-    expect(getByText(/Starred \(0\)/)).toBeInTheDocument();
-    fireEvent.click(getByTestId('user-picker-all'));
-    expect(getByText(/All \(2\)/)).toBeInTheDocument();
 
-    const starredIcons = await findAllByTitle('Add to favorites');
+  // this test is for fixing the bug after favoriting an entity, the matching
+  // entities defaulting to "owned" filter and not based on the selected filter
+  it('should render the correct entities filtered on the selected filter', async () => {
+    await renderWrapped(<CatalogPage />);
+    await expect(screen.findByText(/Owned \(1\)/)).resolves.toBeInTheDocument();
+    fireEvent.click(screen.getByTestId('user-picker-starred'));
+    await expect(
+      screen.findByText(/Starred \(0\)/),
+    ).resolves.toBeInTheDocument();
+    fireEvent.click(screen.getByTestId('user-picker-all'));
+    await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument();
+
+    const starredIcons = await screen.findAllByTitle('Add to favorites');
     fireEvent.click(starredIcons[0]);
-    expect(getByText(/All \(2\)/)).toBeInTheDocument();
+    await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument();
 
-    fireEvent.click(getByTestId('user-picker-starred'));
-    waitFor(() => expect(getByText(/Starred \(1\)/)).toBeInTheDocument());
+    fireEvent.click(screen.getByTestId('user-picker-starred'));
+    await expect(
+      screen.findByText(/Starred \(1\)/),
+    ).resolves.toBeInTheDocument();
   });
 });