Merge pull request #8937 from backstage/fix-initial-filter

catalog-react: improve handling of groups with 0 items in UserListPicker

.changeset/itchy-bulldogs-dance.md

@@ -0,0 +1,10 @@
+---
+'@backstage/plugin-catalog-react': patch
+---
+
+Fix bug: previously the filter would be set to "all" on page load, even if the
+`initiallySelectedFilter` on the `DefaultCatalogPage` was set to something else,
+or a different query parameter was supplied. Now, the prop and query parameters
+control the filter as expected. Additionally, after this change any filters
+which match 0 items will be disabled, and the filter will be reverted to 'all'
+if they're set on page load.

plugins/catalog-react/src/components/UserListPicker/UserListPicker.test.tsx

@@ -69,11 +69,13 @@ const apis = TestApiRegistry.from(
   [storageApiRef, MockStorageApi.create()],
 );
 
-const mockIsOwnedEntity = (entity: Entity) =>
-  entity.metadata.name === 'component-1';
+const mockIsOwnedEntity = jest.fn(
+  (entity: Entity) => entity.metadata.name === 'component-1',
+);
 
-const mockIsStarredEntity = (entity: Entity) =>
-  entity.metadata.name === 'component-3';
+const mockIsStarredEntity = jest.fn(
+  (entity: Entity) => entity.metadata.name === 'component-3',
+);
 
 jest.mock('../../hooks', () => {
   const actual = jest.requireActual('../../hooks');
@@ -250,4 +252,86 @@ describe('<UserListPicker />', () => {
       ),
     });
   });
+
+  describe.each`
+    type         | filterFn
+    ${'owned'}   | ${mockIsOwnedEntity}
+    ${'starred'} | ${mockIsStarredEntity}
+  `('filter resetting for $type entities', ({ type, filterFn }) => {
+    let updateFilters: jest.Mock;
+
+    const picker = ({ loading }: { loading: boolean }) => (
+      <ApiProvider apis={apis}>
+        <MockEntityListContextProvider
+          value={{ backendEntities, updateFilters, loading }}
+        >
+          <UserListPicker initialFilter={type} />
+        </MockEntityListContextProvider>
+      </ApiProvider>
+    );
+
+    beforeEach(() => {
+      updateFilters = jest.fn();
+    });
+
+    describe(`when there are no ${type} entities match the filter`, () => {
+      beforeEach(() => {
+        filterFn.mockReturnValue(false);
+      });
+
+      it('does not reset the filter while entities are loading', () => {
+        render(picker({ loading: true }));
+
+        expect(updateFilters).not.toHaveBeenCalledWith({
+          user: new UserListFilter(
+            'all',
+            mockIsOwnedEntity,
+            mockIsStarredEntity,
+          ),
+        });
+      });
+
+      it('resets the filter to "all" when entities are loaded', () => {
+        render(picker({ loading: false }));
+
+        expect(updateFilters).toHaveBeenLastCalledWith({
+          user: new UserListFilter(
+            'all',
+            mockIsOwnedEntity,
+            mockIsStarredEntity,
+          ),
+        });
+      });
+    });
+
+    describe(`when there are some ${type} entities present`, () => {
+      beforeEach(() => {
+        filterFn.mockReturnValue(true);
+      });
+
+      it('does not reset the filter while entities are loading', () => {
+        render(picker({ loading: true }));
+
+        expect(updateFilters).not.toHaveBeenCalledWith({
+          user: new UserListFilter(
+            'all',
+            mockIsOwnedEntity,
+            mockIsStarredEntity,
+          ),
+        });
+      });
+
+      it('does not reset the filter when entities are loaded', () => {
+        render(picker({ loading: false }));
+
+        expect(updateFilters).toHaveBeenLastCalledWith({
+          user: new UserListFilter(
+            type,
+            mockIsOwnedEntity,
+            mockIsStarredEntity,
+          ),
+        });
+      });
+    });
+  });
 });

plugins/catalog-react/src/components/UserListPicker/UserListPicker.tsx

@@ -130,7 +130,7 @@ export const UserListPicker = ({
   const classes = useStyles();
   const configApi = useApi(configApiRef);
   const orgName = configApi.getOptionalString('organization.name') ?? 'Company';
-  const { filters, updateFilters, backendEntities, queryParameters } =
+  const { filters, updateFilters, backendEntities, queryParameters, loading } =
     useEntityListProvider();
 
   // Remove group items that aren't in availableFilters and exclude
@@ -161,19 +161,46 @@ export const UserListPicker = ({
     [isOwnedEntity, isStarredEntity],
   );
 
+  const [selectedUserFilter, setSelectedUserFilter] = useState(
+    [queryParameters.user].flat()[0] ?? initialFilter,
+  );
+
   // To show proper counts for each section, apply all other frontend filters _except_ the user
   // filter that's controlled by this picker.
-  const [entitiesWithoutUserFilter, setEntitiesWithoutUserFilter] =
-    useState(backendEntities);
-  const totalOwnedUserEntities = entitiesWithoutUserFilter.filter(entity =>
-    ownedFilter.filterEntity(entity),
-  ).length;
-  const [selectedUserFilter, setSelectedUserFilter] = useState(
-    totalOwnedUserEntities > 0
-      ? [queryParameters.user].flat()[0] ?? initialFilter
-      : 'all',
+  const entitiesWithoutUserFilter = useMemo(
+    () =>
+      backendEntities.filter(
+        reduceEntityFilters(
+          compact(Object.values({ ...filters, user: undefined })),
+        ),
+      ),
+    [filters, backendEntities],
   );
 
+  const filterCounts = useMemo<Record<string, number>>(
+    () => ({
+      all: entitiesWithoutUserFilter.length,
+      starred: entitiesWithoutUserFilter.filter(entity =>
+        starredFilter.filterEntity(entity),
+      ).length,
+      owned: entitiesWithoutUserFilter.filter(entity =>
+        ownedFilter.filterEntity(entity),
+      ).length,
+    }),
+    [entitiesWithoutUserFilter, starredFilter, ownedFilter],
+  );
+
+  useEffect(() => {
+    if (
+      !loading &&
+      !!selectedUserFilter &&
+      selectedUserFilter !== 'all' &&
+      filterCounts[selectedUserFilter] === 0
+    ) {
+      setSelectedUserFilter('all');
+    }
+  }, [loading, filterCounts, selectedUserFilter, setSelectedUserFilter]);
+
   useEffect(() => {
     updateFilters({
       user: selectedUserFilter
@@ -186,26 +213,6 @@ export const UserListPicker = ({
     });
   }, [selectedUserFilter, isOwnedEntity, isStarredEntity, updateFilters]);
 
-  useEffect(() => {
-    const filterFn = reduceEntityFilters(
-      compact(Object.values({ ...filters, user: undefined })),
-    );
-    setEntitiesWithoutUserFilter(backendEntities.filter(filterFn));
-  }, [filters, backendEntities]);
-
-  function getFilterCount(id: UserListFilterKind) {
-    switch (id) {
-      case 'owned':
-        return totalOwnedUserEntities;
-      case 'starred':
-        return entitiesWithoutUserFilter.filter(entity =>
-          starredFilter.filterEntity(entity),
-        ).length;
-      default:
-        return entitiesWithoutUserFilter.length;
-    }
-  }
-
   return (
     <Card className={classes.root}>
       {filterGroups.map(group => (
@@ -223,6 +230,8 @@ export const UserListPicker = ({
                   onClick={() => setSelectedUserFilter(item.id)}
                   selected={item.id === filters.user?.value}
                   className={classes.menuItem}
+                  disabled={filterCounts[item.id] === 0}
+                  data-testid={`user-picker-${item.id}`}
                 >
                   {item.icon && (
                     <ListItemIcon className={classes.listIcon}>
@@ -230,15 +239,10 @@ export const UserListPicker = ({
                     </ListItemIcon>
                   )}
                   <ListItemText>
-                    <Typography
-                      variant="body1"
-                      data-testid={`user-picker-${item.id}`}
-                    >
-                      {item.label}
-                    </Typography>
+                    <Typography variant="body1">{item.label}</Typography>
                   </ListItemText>
                   <ListItemSecondaryAction>
-                    {getFilterCount(item.id) ?? '-'}
+                    {filterCounts[item.id] ?? '-'}
                   </ListItemSecondaryAction>
                 </MenuItem>
               ))}

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

@@ -263,10 +263,12 @@ describe('DefaultCatalogPage', () => {
     const { getByTestId } = await renderWrapped(<DefaultCatalogPage />);
     fireEvent.click(getByTestId('user-picker-owned'));
     await expect(screen.findByText(/Owned \(1\)/)).resolves.toBeInTheDocument();
-    fireEvent.click(screen.getByTestId('user-picker-starred'));
-    await expect(
-      screen.findByText(/Starred \(0\)/),
-    ).resolves.toBeInTheDocument();
+    // The "Starred" menu option should initially be disabled, since there
+    // aren't any starred entities.
+    await expect(screen.getByTestId('user-picker-starred')).toHaveAttribute(
+      'aria-disabled',
+      'true',
+    );
     fireEvent.click(screen.getByTestId('user-picker-all'));
     await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument();
 
@@ -274,6 +276,12 @@ describe('DefaultCatalogPage', () => {
     fireEvent.click(starredIcons[0]);
     await expect(screen.findByText(/All \(2\)/)).resolves.toBeInTheDocument();
 
+    // Now that we've starred an entity, the "Starred" menu option should be
+    // enabled.
+    await expect(screen.getByTestId('user-picker-starred')).not.toHaveAttribute(
+      'aria-disabled',
+      'true',
+    );
     fireEvent.click(screen.getByTestId('user-picker-starred'));
     await expect(
       screen.findByText(/Starred \(1\)/),