diff --git a/.changeset/cold-donuts-train.md b/.changeset/cold-donuts-train.md new file mode 100644 index 0000000000..fe84653601 --- /dev/null +++ b/.changeset/cold-donuts-train.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-react': patch +--- + +Fix a potential race condition in EntityListProvider when selecting filters diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx index de85fa1042..ef3c83315a 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.test.tsx @@ -14,7 +14,7 @@ * limitations under the License. */ -import { catalogApiMock } from '@backstage/plugin-catalog-react/testUtils'; +import type { GetEntitiesResponse } from '@backstage/catalog-client'; import { Entity } from '@backstage/catalog-model'; import { alertApiRef, @@ -23,7 +23,10 @@ import { identityApiRef, storageApiRef, } from '@backstage/core-plugin-api'; +import { translationApiRef } from '@backstage/core-plugin-api/alpha'; +import { catalogApiMock } from '@backstage/plugin-catalog-react/testUtils'; import { mockApis, TestApiProvider } from '@backstage/test-utils'; +import { useMountEffect } from '@react-hookz/web'; import { act, renderHook, waitFor } from '@testing-library/react'; import qs from 'qs'; import { PropsWithChildren } from 'react'; @@ -37,10 +40,9 @@ import { EntityTypeFilter, EntityUserFilter, } from '../filters'; -import { EntityListProvider, useEntityList } from './useEntityListProvider'; -import { useMountEffect } from '@react-hookz/web'; -import { translationApiRef } from '@backstage/core-plugin-api/alpha'; +import { createDeferred } from '@backstage/types'; import { EntityListPagination } from '../types'; +import { EntityListProvider, useEntityList } from './useEntityListProvider'; const entities: Entity[] = [ { @@ -341,6 +343,62 @@ describe('', () => { filter: { kind: 'group' }, }); }); + + it('uses the last applied filter even if an earlier request finishes later', async () => { + const { result } = renderHook(() => useEntityList(), { + wrapper: createWrapper({ pagination }), + }); + + const firstResult = createDeferred(); + const secondResult = createDeferred(); + + await waitFor(() => { + expect(result.current.backendEntities.length).toBeGreaterThan(0); + }); + expect(result.current.totalItems).toBe(2); + expect(result.current.backendEntities.length).toBe(2); + expect(mockCatalogApi.getEntities).toHaveBeenCalledTimes(1); + + mockCatalogApi.getEntities!.mockReturnValueOnce(firstResult); + + await act(async () => { + result.current.updateFilters({ + kind: new EntityKindFilter('api', 'API'), + }); + }); + + await waitFor(() => { + expect(mockCatalogApi.getEntities).toHaveBeenNthCalledWith(2, { + filter: { kind: 'api' }, + }); + }); + + mockCatalogApi.getEntities!.mockReturnValueOnce(secondResult); + + await act(async () => { + result.current.updateFilters({ + kind: new EntityKindFilter('system', 'System'), + }); + }); + + await waitFor(() => { + expect(mockCatalogApi.getEntities).toHaveBeenNthCalledWith(3, { + filter: { kind: 'system' }, + }); + }); + + await act(async () => { + secondResult.resolve({ + items: [], + }); + firstResult.resolve({ + items: entities, + }); + }); + + expect(result.current.filters.kind!.value).toBe('system'); + expect(result.current.backendEntities.length).toBe(0); + }); }); describe('', () => { diff --git a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx index 56af7e9a73..f424dafb56 100644 --- a/plugins/catalog-react/src/hooks/useEntityListProvider.tsx +++ b/plugins/catalog-react/src/hooks/useEntityListProvider.tsx @@ -14,7 +14,9 @@ * limitations under the License. */ +import { QueryEntitiesResponse } from '@backstage/catalog-client'; import { Entity } from '@backstage/catalog-model'; +import { useApi } from '@backstage/core-plugin-api'; import { compact, isEqual } from 'lodash'; import qs from 'qs'; import { @@ -22,6 +24,7 @@ import { PropsWithChildren, useCallback, useContext, + useEffect, useMemo, useState, } from 'react'; @@ -50,8 +53,6 @@ import { reduceCatalogFilters, reduceEntityFilters, } from '../utils/filters'; -import { useApi } from '@backstage/core-plugin-api'; -import { QueryEntitiesResponse } from '@backstage/catalog-client'; /** @public */ export type DefaultEntityFilters = { @@ -239,7 +240,7 @@ export const EntityListProvider = ( // 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( + const [{ value: resolvedValue, loading, error }, refresh] = useAsyncFn( async () => { const kindValue = requestedFilters.kind?.value?.toLocaleLowerCase('en-US'); @@ -249,19 +250,6 @@ export const EntityListProvider = ( : requestedFilters; const compacted = compact(Object.values(adjustedFilters)); - const queryParams = Object.keys(requestedFilters).reduce( - (params, key) => { - const filter = requestedFilters[key as keyof EntityFilters] as - | EntityFilter - | undefined; - if (filter?.toQueryValue) { - params[key] = filter.toQueryValue(); - } - return params; - }, - {} as Record, - ); - if (paginationMode !== 'none') { if (cursor) { if (cursor !== outputState.appliedCursor) { @@ -270,14 +258,14 @@ export const EntityListProvider = ( cursor, limit, }); - setOutputState({ + return { appliedFilters: requestedFilters, appliedCursor: cursor, backendEntities: response.items, entities: response.items.filter(entityFilter), pageInfo: response.pageInfo, totalItems: response.totalItems, - }); + }; } } else { const entityFilter = reduceEntityFilters(compacted); @@ -296,7 +284,7 @@ export const EntityListProvider = ( limit, offset, }); - setOutputState({ + return { appliedFilters: requestedFilters, backendEntities: response.items, entities: response.items.filter(entityFilter), @@ -304,7 +292,7 @@ export const EntityListProvider = ( totalItems: response.totalItems, limit, offset, - }); + }; } } } else { @@ -324,43 +312,22 @@ export const EntityListProvider = ( filter: backendFilter, }); const entities = response.items.filter(entityFilter); - setOutputState({ + return { appliedFilters: requestedFilters, backendEntities: response.items, entities, totalItems: entities.length, - }); - } else { - const entities = outputState.backendEntities.filter(entityFilter); - setOutputState({ - appliedFilters: requestedFilters, - backendEntities: outputState.backendEntities, - entities, - totalItems: entities.length, - }); + }; } + const entities = outputState.backendEntities.filter(entityFilter); + return { + appliedFilters: requestedFilters, + backendEntities: outputState.backendEntities, + entities, + totalItems: entities.length, + }; } - - if (isMounted()) { - const oldParams = qs.parse(location.search, { - ignoreQueryPrefix: true, - }); - const newParams = qs.stringify( - { - ...oldParams, - filters: queryParams, - ...(paginationMode === 'none' ? {} : { cursor, limit, offset }), - }, - { addQueryPrefix: true, arrayFormat: 'repeat' }, - ); - const newUrl = `${window.location.pathname}${newParams}`; - // We use direct history manipulation since useSearchParams and - // useNavigate in react-router-dom cause unnecessary extra rerenders. - // Also make sure to replace the state rather than pushing, since we - // don't want there to be back/forward slots for every single filter - // change. - window.history?.replaceState(null, document.title, newUrl); - } + return undefined; }, [ catalogApi, @@ -379,6 +346,55 @@ export const EntityListProvider = ( // filters will be calling this in rapid succession. useDebounce(refresh, 10, [requestedFilters, cursor, limit, offset]); + useEffect(() => { + if (resolvedValue === undefined) { + return; + } + setOutputState(resolvedValue); + if (isMounted()) { + const queryParams = Object.keys(requestedFilters).reduce( + (params, key) => { + const filter = requestedFilters[key as keyof EntityFilters] as + | EntityFilter + | undefined; + if (filter?.toQueryValue) { + params[key] = filter.toQueryValue(); + } + return params; + }, + {} as Record, + ); + + const oldParams = qs.parse(location.search, { + ignoreQueryPrefix: true, + }); + const newParams = qs.stringify( + { + ...oldParams, + filters: queryParams, + ...(paginationMode === 'none' ? {} : { cursor, limit, offset }), + }, + { addQueryPrefix: true, arrayFormat: 'repeat' }, + ); + const newUrl = `${window.location.pathname}${newParams}`; + // We use direct history manipulation since useSearchParams and + // useNavigate in react-router-dom cause unnecessary extra rerenders. + // Also make sure to replace the state rather than pushing, since we + // don't want there to be back/forward slots for every single filter + // change. + window.history?.replaceState(null, document.title, newUrl); + } + }, [ + cursor, + isMounted, + limit, + location.search, + offset, + requestedFilters, + resolvedValue, + paginationMode, + ]); + const updateFilters = useCallback( ( update: