diff --git a/.changeset/little-boats-think.md b/.changeset/little-boats-think.md new file mode 100644 index 0000000000..d389bfbbb1 --- /dev/null +++ b/.changeset/little-boats-think.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-catalog-react': minor +--- + +The `EntityOwnerPicker` component has undergone improvements to enhance its performance. + +The component now loads entities asynchronously, resulting in improved performance and responsiveness. Instead of loading all entities upfront, they are now loaded in batches as the user scrolls. +The previous implementation inferred users and groups displayed by the `EntityOwnerPicker` component based on the entities available in the `EntityListContext`. The updated version no longer relies on the `EntityListContext` for inference, allowing for better decoupling and improved performance. diff --git a/plugins/catalog-react/package.json b/plugins/catalog-react/package.json index 569e9d5d8a..2628d2dee9 100644 --- a/plugins/catalog-react/package.json +++ b/plugins/catalog-react/package.json @@ -60,6 +60,7 @@ "@material-ui/core": "^4.12.2", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.61", + "@react-hookz/web": "^23.0.0", "@types/react": "^16.13.1 || ^17.0.0", "classnames": "^2.2.6", "jwt-decode": "^3.1.0", diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx index e4dc322c7c..e5497d022a 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.test.tsx @@ -14,8 +14,8 @@ * limitations under the License. */ -import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; -import { fireEvent, screen } from '@testing-library/react'; +import { Entity } from '@backstage/catalog-model'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; import React from 'react'; import { MockEntityListContextProvider } from '../../testUtils/providers'; import { EntityOwnerFilter } from '../../filters'; @@ -28,8 +28,9 @@ import { } from '@backstage/test-utils'; import { catalogApiRef, CatalogApi } from '../..'; import { errorApiRef } from '@backstage/core-plugin-api'; +import { QueryEntitiesCursorRequest } from '@backstage/catalog-client'; -const ownerEntities: Entity[] = [ +const ownerEntitiesBatch1: Entity[] = [ { apiVersion: '1', kind: 'Group', @@ -68,64 +69,56 @@ const ownerEntities: Entity[] = [ }, ]; -const sampleEntities: Entity[] = [ +const ownerEntitiesBatch2: Entity[] = [ { apiVersion: '1', - kind: 'Component', + kind: 'Group', metadata: { - name: 'component-1', + name: 'some-owner-batch-2', }, - relations: [ - { - type: 'ownedBy', - targetRef: 'group:default/some-owner', - }, - { - type: 'ownedBy', - targetRef: 'group:default/some-owner-2', - }, - ], }, { apiVersion: '1', - kind: 'Component', + kind: 'Group', metadata: { - name: 'component-2', + name: 'some-owner-2-batch-2', }, - relations: [ - { - type: 'ownedBy', - targetRef: 'group:default/another-owner', + spec: { + profile: { + displayName: 'Some Owner Batch 2', }, - { - type: 'ownedBy', - targetRef: 'group:test-namespace/another-owner-2', - }, - ], + }, }, { apiVersion: '1', - kind: 'Component', + kind: 'Group', metadata: { - name: 'component-3', + name: 'another-owner-batch-2', + title: 'Another Owner Batch 2', + }, + }, + { + apiVersion: '1', + kind: 'Group', + metadata: { + namespace: 'test-namespace', + name: 'another-owner-2-batch-2', + title: 'Another Owner in Another Namespace Batch 2', }, - relations: [ - { - type: 'ownedBy', - targetRef: 'group:default/some-owner', - }, - ], }, ]; -const getEntitiesByRefs = jest.fn(async ({ entityRefs }) => ({ - items: entityRefs.map((e: string) => - ownerEntities.find(f => stringifyEntityRef(f) === e), - ), -})); +const mockedQueryEntities: jest.MockedFn = + jest.fn(); + +const mockedGetEntitiesByRef: jest.MockedFn = + jest.fn(); + const mockCatalogApi: Partial = { - getEntitiesByRefs, + queryEntities: mockedQueryEntities, + getEntitiesByRefs: mockedGetEntitiesByRef, }; + const mockErrorApi = new MockErrorApi(); describe('', () => { @@ -134,12 +127,33 @@ describe('', () => { [errorApiRef, mockErrorApi], ); - it('renders all owners', async () => { + beforeEach(() => { + jest.resetAllMocks(); + + mockedQueryEntities.mockImplementation(async request => { + const totalItems = + ownerEntitiesBatch1.length + ownerEntitiesBatch2.length; + if ((request as QueryEntitiesCursorRequest).cursor) { + return { + items: ownerEntitiesBatch2, + pageInfo: {}, + totalItems, + }; + } + return { + items: ownerEntitiesBatch1, + pageInfo: { + nextCursor: 'nextCursor', + }, + totalItems, + }; + }); + }); + + it('renders all users and groups', async () => { await renderWithEffects( - + , @@ -147,36 +161,37 @@ describe('', () => { expect(screen.getByText('Owner')).toBeInTheDocument(); fireEvent.click(screen.getByTestId('owner-picker-expand')); + + await waitFor(() => + expect(screen.getByText('Another Owner')).toBeInTheDocument(), + ); + [ - 'Another Owner', 'some-owner', 'Some Owner 2', 'Another Owner in Another Namespace', ].forEach(owner => { expect(screen.getByText(owner)).toBeInTheDocument(); }); - }); - it('renders unique owners in alphabetical order', async () => { - await renderWithEffects( - - - - - , + expect(mockedQueryEntities).toHaveBeenCalledTimes(1); + expect(mockedGetEntitiesByRef).not.toHaveBeenCalled(); + + fireEvent.scroll(screen.getByTestId('owner-picker-listbox')); + + await waitFor(() => + expect(screen.getByText('some-owner-batch-2')).toBeInTheDocument(), ); - expect(screen.getByText('Owner')).toBeInTheDocument(); - fireEvent.click(screen.getByTestId('owner-picker-expand')); + [ + 'some-owner-batch-2', + 'Some Owner Batch 2', + 'Another Owner in Another Namespace Batch 2', + ].forEach(owner => { + expect(screen.getByText(owner)).toBeInTheDocument(); + }); - expect(screen.getAllByRole('option').map(o => o.textContent)).toEqual([ - 'Another Owner', - 'Another Owner in Another Namespace', - 'some-owner', - 'Some Owner 2', - ]); + expect(mockedQueryEntities).toHaveBeenCalledTimes(2); }); it('respects the query parameter filter value', async () => { @@ -186,8 +201,6 @@ describe('', () => { ', () => { , ); + expect(mockedGetEntitiesByRef).toHaveBeenCalledWith({ + entityRefs: ['another-owner'], + }); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['group:default/another-owner']), }); }); + it('should display the selected owners as humanized entities', async () => { + const updateFilters = jest.fn(); + const queryParameters = { owners: ['another-owner'] }; + + mockedGetEntitiesByRef.mockResolvedValue({ + items: [ + { + metadata: { + name: 'another-owner', + title: 'Beautiful display name', + namespace: 'default', + }, + apiVersion: '1', + kind: 'group', + }, + ], + }); + await renderWithEffects( + + + + + , + ); + + await waitFor(() => + expect( + screen.getByRole('button', { + name: 'Beautiful display name', + }), + ).toBeInTheDocument(), + ); + + expect(mockedGetEntitiesByRef).toHaveBeenCalledWith({ + entityRefs: ['another-owner'], + }); + + fireEvent.click(screen.getByTestId('owner-picker-expand')); + await waitFor(() => screen.getByText('Some Owner 2')); + fireEvent.click(screen.getByText('Some Owner 2')); + + expect(mockedGetEntitiesByRef).toHaveBeenCalledTimes(1); + + await waitFor(() => + expect( + screen.getByRole('button', { + name: 'Some Owner 2', + }), + ).toBeInTheDocument(), + ); + }); + it('adds owners to filters', async () => { const updateFilters = jest.fn(); await renderWithEffects( @@ -217,11 +288,14 @@ describe('', () => { , ); + expect(mockedGetEntitiesByRef).not.toHaveBeenCalled(); expect(updateFilters).toHaveBeenLastCalledWith({ owners: undefined, }); fireEvent.click(screen.getByTestId('owner-picker-expand')); + await waitFor(() => screen.getByText('some-owner')); + fireEvent.click(screen.getByText('some-owner')); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['group:default/some-owner']), @@ -234,8 +308,6 @@ describe('', () => { ', () => { , ); + expect(mockedGetEntitiesByRef).toHaveBeenCalledWith({ + entityRefs: ['group:default/some-owner'], + }); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['group:default/some-owner']), }); fireEvent.click(screen.getByTestId('owner-picker-expand')); - expect(screen.getByLabelText('some-owner')).toBeChecked(); + await waitFor(() => + expect(screen.getByLabelText('some-owner')).toBeChecked(), + ); fireEvent.click(screen.getByLabelText('some-owner')); expect(updateFilters).toHaveBeenLastCalledWith({ @@ -265,13 +342,15 @@ describe('', () => { value={{ updateFilters, queryParameters: { owners: ['team-a'] }, - backendEntities: sampleEntities, }} > , ); + expect(mockedGetEntitiesByRef).toHaveBeenCalledWith({ + entityRefs: ['team-a'], + }); expect(updateFilters).toHaveBeenLastCalledWith({ owners: new EntityOwnerFilter(['group:default/team-a']), }); @@ -281,7 +360,6 @@ describe('', () => { value={{ updateFilters, queryParameters: { owners: ['team-b'] }, - backendEntities: sampleEntities, }} > @@ -292,23 +370,4 @@ describe('', () => { owners: new EntityOwnerFilter(['group:default/team-b']), }); }); - it('removes owners from filters if there are none available', async () => { - const updateFilters = jest.fn(); - await renderWithEffects( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - owners: undefined, - }); - }); }); diff --git a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx index 8c4f2bfea0..8b223c50b1 100644 --- a/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx +++ b/plugins/catalog-react/src/components/EntityOwnerPicker/EntityOwnerPicker.tsx @@ -14,32 +14,30 @@ * limitations under the License. */ -import { - Entity, - parseEntityRef, - RELATION_OWNED_BY, - stringifyEntityRef, -} from '@backstage/catalog-model'; +import { Entity, stringifyEntityRef } from '@backstage/catalog-model'; import { Box, Checkbox, FormControlLabel, - makeStyles, TextField, Typography, + makeStyles, } from '@material-ui/core'; import CheckBoxIcon from '@material-ui/icons/CheckBox'; import CheckBoxOutlineBlankIcon from '@material-ui/icons/CheckBoxOutlineBlank'; import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; import { Autocomplete } from '@material-ui/lab'; -import React, { useEffect, useMemo, useState } from 'react'; +import React, { useEffect, useMemo, useRef, useState } from 'react'; import { useEntityList } from '../../hooks/useEntityListProvider'; import { EntityOwnerFilter } from '../../filters'; -import { getEntityRelations } from '../../utils'; -import useAsync from 'react-use/lib/useAsync'; -import { errorApiRef, useApi } from '@backstage/core-plugin-api'; +import { useApi } from '@backstage/core-plugin-api'; import { catalogApiRef } from '../../api'; -import { humanizeEntity, humanizeEntityRef } from '../EntityRefLink/humanize'; +import useAsync from 'react-use/lib/useAsync'; +import useAsyncFn from 'react-use/lib/useAsyncFn'; +import { useDebouncedEffect } from '@react-hookz/web'; +import PersonIcon from '@material-ui/icons/Person'; +import GroupIcon from '@material-ui/icons/Group'; +import { humanizeEntity } from '../EntityRefLink/humanize'; /** @public */ export type CatalogReactEntityOwnerPickerClassKey = 'input'; @@ -61,12 +59,51 @@ export const EntityOwnerPicker = () => { const classes = useStyles(); const { updateFilters, - backendEntities, filters, queryParameters: { owners: ownersParameter }, } = useEntityList(); + const catalogApi = useApi(catalogApiRef); - const errorApi = useApi(errorApiRef); + const [text, setText] = useState(''); + + const [{ value, loading }, handleFetch] = useAsyncFn( + async (request: { text: string } | { cursor: string; prev: Entity[] }) => { + const initialRequest = request as { text: string }; + const cursorRequest = request as { cursor: string; prev: Entity[] }; + const limit = 20; + + if (cursorRequest.cursor) { + const response = await catalogApi.queryEntities({ + cursor: cursorRequest.cursor, + limit, + }); + return { + ...response, + items: [...cursorRequest.prev, ...response.items], + }; + } + + return catalogApi.queryEntities({ + fullTextFilter: { + term: initialRequest.text || '', + fields: [ + 'metadata.name', + 'kind', + 'spec.profile.displayname', + 'metadata.title', + ], + }, + filter: { kind: ['User', 'Group'] }, + orderFields: [{ field: 'metadata.name', order: 'asc' }], + limit, + }); + }, + [text], + ); + + useDebouncedEffect(() => handleFetch({ text }), [text], 250); + + const availableOwners = value?.items || []; const queryParamOwners = useMemo( () => [ownersParameter].flat().filter(Boolean) as string[], @@ -74,71 +111,10 @@ export const EntityOwnerPicker = () => { ); const [selectedOwners, setSelectedOwners] = useState( - queryParamOwners.length - ? new EntityOwnerFilter(queryParamOwners).values - : filters.owners?.values ?? [], + queryParamOwners.length ? queryParamOwners : filters.owners?.values ?? [], ); - const { - loading, - error, - value: ownerEntities, - } = useAsync(async () => { - const ownerEntityRefs = [ - ...new Set( - backendEntities - .flatMap((e: Entity) => - getEntityRelations(e, RELATION_OWNED_BY).map(o => - stringifyEntityRef(o), - ), - ) - .filter(Boolean) as string[], - ), - ]; - const { items: ownerEntitiesOrNull } = await catalogApi.getEntitiesByRefs({ - entityRefs: ownerEntityRefs, - fields: [ - 'kind', - 'metadata.name', - 'metadata.title', - 'metadata.namespace', - 'spec.profile.displayName', - ], - }); - const owners = ownerEntitiesOrNull.map((entity, index) => { - if (entity) { - return { - label: humanizeEntity(entity, { defaultKind: 'Group' }), - entityRef: stringifyEntityRef(entity), - }; - } - return { - label: humanizeEntityRef(parseEntityRef(ownerEntityRefs[index]), { - defaultKind: 'group', - }), - entityRef: ownerEntityRefs[index], - }; - }); - - return owners.sort((a, b) => - a.label.localeCompare(b.label, 'en-US', { - ignorePunctuation: true, - caseFirst: 'upper', - }), - ); - }, [backendEntities]); - - useEffect(() => { - if (error) { - errorApi.post( - { - ...error, - message: `EntityOwnerPicker failed to initialize: ${error.message}`, - }, - {}, - ); - } - }, [error, errorApi]); + const { getEntity, setEntity } = useSelectedOwners(selectedOwners); // Set selected owners on query parameter updates; this happens at initial page load and from // external updates to the page location. @@ -150,17 +126,20 @@ export const EntityOwnerPicker = () => { }, [queryParamOwners]); useEffect(() => { - if (!loading && ownerEntities) { - updateFilters({ - owners: - selectedOwners.length && ownerEntities.length - ? new EntityOwnerFilter(selectedOwners) - : undefined, - }); - } - }, [selectedOwners, updateFilters, ownerEntities, loading]); + updateFilters({ + owners: selectedOwners.length + ? new EntityOwnerFilter(selectedOwners) + : undefined, + }); + }, [selectedOwners, updateFilters]); - if (!loading && !ownerEntities?.length) return null; + if ( + ['user', 'group'].includes( + filters.kind?.value.toLocaleLowerCase('en-US') || '', + ) + ) { + return null; + } return ( @@ -170,40 +149,130 @@ export const EntityOwnerPicker = () => { multiple disableCloseOnSelect loading={loading} - options={ownerEntities || []} - value={ - ownerEntities?.filter(e => - selectedOwners.some((f: string) => f === e.entityRef), - ) ?? [] - } - onChange={(_: object, value: { entityRef: string }[]) => - setSelectedOwners(value.map(e => e.entityRef)) - } - getOptionLabel={option => option.label} - renderOption={(option, { selected }) => ( - - } - onClick={event => event.preventDefault()} - label={option.label} - /> - )} + options={availableOwners} + value={selectedOwners as unknown as Entity[]} + getOptionSelected={(o, v) => { + if (typeof v === 'string') { + return stringifyEntityRef(o) === v; + } + return o === v; + }} + getOptionLabel={o => { + const entity = typeof o === 'string' ? getEntity(o) || o : o; + + return typeof entity === 'string' + ? entity + : humanizeEntity(entity, entity.metadata.name); + }} + onChange={(_: object, owners) => { + setText(''); + setSelectedOwners( + owners.map(e => { + const entityRef = + typeof e === 'string' ? e : stringifyEntityRef(e); + + if (typeof e !== 'string') { + setEntity(e); + } + + return entityRef; + }), + ); + }} + filterOptions={x => x} + renderOption={(entity, { selected }) => { + const isGroup = entity.kind === 'Group'; + + return ( + + } + onClick={event => event.preventDefault()} + label={ + + {isGroup ? ( + + ) : ( + + )} +   + {humanizeEntity(entity, entity.metadata.name)} + + } + /> + ); + }} size="small" popupIcon={} renderInput={params => ( { + setText(e.currentTarget.value); + }} variant="outlined" /> )} + ListboxProps={{ + onScroll: (e: React.MouseEvent) => { + const element = e.currentTarget; + const hasReachedEnd = + Math.abs( + element.scrollHeight - + element.clientHeight - + element.scrollTop, + ) < 1; + + if (hasReachedEnd && value?.pageInfo.nextCursor) { + handleFetch({ + cursor: value.pageInfo.nextCursor, + prev: value.items, + }); + } + }, + 'data-testid': 'owner-picker-listbox', + }} /> ); }; + +/** + * Hook used for storing the full entity of the specified owners + * in order to display users and group using the information contained on each entity. + * When a component is rendered for the first time, it loads the content of the entities + * specified by `initialSelectedOwnersRefs` and export the `getEntity` and `setEntity` + * utilities, used to retrieve and modify the owners. + */ +function useSelectedOwners(initialSelectedOwnersRefs: string[]) { + const allEntities = useRef>({}); + const catalogApi = useApi(catalogApiRef); + + useAsync(async () => { + if (initialSelectedOwnersRefs.length === 0) { + return; + } + const initialSelectedEntities = await catalogApi.getEntitiesByRefs({ + entityRefs: initialSelectedOwnersRefs, + }); + initialSelectedEntities.items.forEach(e => { + if (e) { + allEntities.current[stringifyEntityRef(e)] = e; + } + }); + }, []); + + return { + getEntity: (entityRef: string) => allEntities.current[entityRef], + setEntity: (entity: Entity) => { + allEntities.current[stringifyEntityRef(entity)] = entity; + }, + }; +} diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts index e7d1649a29..1182a7778d 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.test.ts @@ -215,9 +215,12 @@ describe('humanizeEntityRef', () => { describe('humanizeEntity', () => { it('gives a readable name when one is provided at metadata.title', () => { expect( - humanizeEntity({ - metadata: { name: 'my-entity', title: 'My Title' }, - } as Entity), + humanizeEntity( + { + metadata: { name: 'my-entity', title: 'My Title' }, + } as Entity, + 'default', + ), ).toBe('My Title'); }); @@ -257,13 +260,16 @@ describe('humanizeEntity', () => { ])( 'gives a readable name for kind %s when one is provided at spec.profile.displayName', (_, entity: Entity, expected) => { - expect(humanizeEntity(entity)).toBe(expected); + expect(humanizeEntity(entity, 'default')).toBe(expected); }, ); it('should pass through to humanizeEntityRef when nothing matches', () => { expect( - humanizeEntity({ kind: 'Group', metadata: { name: 'test' } } as Entity), - ).toBe('group:test'); + humanizeEntity( + { kind: 'Group', metadata: { name: 'test' } } as Entity, + 'default', + ), + ).toBe('default'); }); }); diff --git a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts index 7f1501280a..478f0dce9d 100644 --- a/plugins/catalog-react/src/components/EntityRefLink/humanize.ts +++ b/plugins/catalog-react/src/components/EntityRefLink/humanize.ts @@ -25,7 +25,8 @@ import get from 'lodash/get'; * @param defaultNamespace - if set to false then namespace is never omitted, * if set to string which matches namespace of entity then omitted * - * @public */ + * @public + **/ export function humanizeEntityRef( entityRef: Entity | CompoundEntityRef, opts?: { @@ -73,27 +74,19 @@ export function humanizeEntityRef( * If an entity is either User or Group, this will be its `spec.profile.displayName`. * Otherwise, this is `metadata.title`. * - * If neither of those are found or populated, fallback to `humanizeEntityRef`. + * If neither of those are found or populated, fallback to `defaultName`. * * @param entity - Entity to convert. - * @param opts - If entity readable name is not available, opts will be used to specify humanizeEntityRef options. - * @returns Readable name, defaults to unique identifier. + * @param defaultName - If entity readable name is not available, `defaultName` will be returned. + * @returns Readable name, defaults to `defaultName`. * - * @public */ -export function humanizeEntity( - entity: Entity, - opts?: { - defaultKind?: string; - defaultNamespace?: string | false; - }, -) { +export function humanizeEntity(entity: Entity, defaultName: string) { for (const path of ['spec.profile.displayName', 'metadata.title']) { const value = get(entity, path); if (value && typeof value === 'string') { return value; } } - - return humanizeEntityRef(entity, opts); + return defaultName; } diff --git a/yarn.lock b/yarn.lock index fce0be422f..632d538a16 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5697,6 +5697,7 @@ __metadata: "@material-ui/core": ^4.12.2 "@material-ui/icons": ^4.9.1 "@material-ui/lab": 4.0.0-alpha.61 + "@react-hookz/web": ^23.0.0 "@testing-library/dom": ^8.0.0 "@testing-library/jest-dom": ^5.10.1 "@testing-library/react": ^12.1.3 @@ -13738,6 +13739,22 @@ __metadata: languageName: node linkType: hard +"@react-hookz/web@npm:^23.0.0": + version: 23.0.0 + resolution: "@react-hookz/web@npm:23.0.0" + dependencies: + "@react-hookz/deep-equal": ^1.0.4 + peerDependencies: + js-cookie: ^3.0.1 + react: ^16.8 || ^17 || ^18 + react-dom: ^16.8 || ^17 || ^18 + peerDependenciesMeta: + js-cookie: + optional: true + checksum: 230bff62291bcafa65e0b9adcc3f0769fd63fabc226c77149e7797d1ea67362e9ad581ceff3cdfe2949698798d2deefb223c05bdfed1e465b5d92298d48cf3e5 + languageName: node + linkType: hard + "@remix-run/router@npm:1.3.2": version: 1.3.2 resolution: "@remix-run/router@npm:1.3.2"