diff --git a/.changeset/beige-garlics-doubt.md b/.changeset/beige-garlics-doubt.md new file mode 100644 index 0000000000..436abce45e --- /dev/null +++ b/.changeset/beige-garlics-doubt.md @@ -0,0 +1,5 @@ +--- +'@backstage/test-utils': patch +--- + +Fix a bug in `MockStorageApi` where it unhelpfully returned new empty buckets every single time diff --git a/.changeset/slimy-kids-attack.md b/.changeset/slimy-kids-attack.md new file mode 100644 index 0000000000..ddb7735571 --- /dev/null +++ b/.changeset/slimy-kids-attack.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Tweak the `EntityListProvider` to do single-cycle updates diff --git a/packages/test-utils/src/testUtils/apis/StorageApi/MockStorageApi.ts b/packages/test-utils/src/testUtils/apis/StorageApi/MockStorageApi.ts index 006b44a49c..5cb4bdfff6 100644 --- a/packages/test-utils/src/testUtils/apis/StorageApi/MockStorageApi.ts +++ b/packages/test-utils/src/testUtils/apis/StorageApi/MockStorageApi.ts @@ -23,6 +23,8 @@ import ObservableImpl from 'zen-observable'; export type MockStorageBucket = { [key: string]: any }; +const bucketStorageApis = new Map(); + export class MockStorageApi implements StorageApi { private readonly namespace: string; private readonly data: MockStorageBucket; @@ -37,7 +39,13 @@ export class MockStorageApi implements StorageApi { } forBucket(name: string): StorageApi { - return new MockStorageApi(`${this.namespace}/${name}`, this.data); + if (!bucketStorageApis.has(name)) { + bucketStorageApis.set( + name, + new MockStorageApi(`${this.namespace}/${name}`, this.data), + ); + } + return bucketStorageApis.get(name)!; } get(key: string): T | undefined { diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx index fcb599888e..e2dfbd91da 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx @@ -157,6 +157,7 @@ describe('', () => { 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); }); diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx index 159c898b74..a410c243a1 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx +++ b/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 | undefined >(undefined); +type OutputState = { + appliedFilters: EntityFilters; + entities: Entity[]; + backendEntities: Entity[]; +}; + export const EntityListProvider = ({ children, }: PropsWithChildren<{}>) => { const catalogApi = useApi(catalogApiRef); + const [requestedFilters, setRequestedFilters] = useState( + {} as EntityFilters, + ); + const [outputState, setOutputState] = useState>({ + appliedFilters: {} as EntityFilters, + entities: [], + backendEntities: [], + }); - const [filters, setFilters] = useState({} as EntityFilters); - const [entities, setEntities] = useState([]); - const [backendEntities, setBackendEntities] = useState([]); - - // Store resolved catalog-backend filters and deep compare on filter updates, to avoid refetching - // when only frontend filters change - const [backendFilters, setBackendFilters] = useState< - Record - >(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 = ({ | Partial | ((prevFilters: EntityFilters) => Partial), ) => { - 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 = ({ return ( { // 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(); - expect(getByText(/Owned \(1\)/)).toBeInTheDocument(); + const { findByText, getByTestId } = await renderWrapped(); + 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( , ); - 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( - , - ); - 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(); + 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(); }); });