more review comments

Signed-off-by: Brian Fletcher <brian@roadie.io>

plugins/catalog-react/api-report.md

@@ -255,14 +255,13 @@ export class EntityOwnerFilter implements EntityFilter {
 export const EntityOwnerPicker: () => JSX.Element | null;
 
 // @public
-export const EntityPeekAheadPopover: ({
-  entityRef,
-  children,
-}: EntityPeekAheadPopoverProps) => JSX.Element;
+export const EntityPeekAheadPopover: (
+  props: EntityPeekAheadPopoverProps,
+) => JSX.Element;
 
 // @public
 export type EntityPeekAheadPopoverProps = PropsWithChildren<{
-  entityRef: CompoundEntityRef;
+  entityRef: string;
 }>;
 
 // @public (undocumented)

plugins/catalog-react/src/components/EntityPeekAheadPopover/EntityPeekAheadPopover.stories.tsx

@@ -60,16 +60,27 @@ const mockCatalogApi = {
         },
       };
     }
+    if (
+      entityRef.namespace === 'default' &&
+      entityRef.name === 'slow.catalog.item' &&
+      entityRef.kind === 'component'
+    ) {
+      await new Promise(resolve => setTimeout(resolve, 3000));
+      return {
+        kind: 'Component',
+        metadata: {
+          name: 'slow.catalog.item',
+          namespace: 'default',
+          description: 'Details about the slow.catalog.item service',
+        },
+      };
+    }
     return undefined;
   },
 };
 
 const defaultArgs = {
-  entityRef: {
-    namespace: 'default',
-    name: 'playback',
-    kind: 'component',
-  },
+  entityRef: 'component:default/playback',
 };
 
 export default {
@@ -106,9 +117,23 @@ export const User = (args: EntityPeekAheadPopoverProps) => (
   </EntityPeekAheadPopover>
 );
 User.args = {
-  entityRef: {
-    kind: 'user',
-    namespace: 'default',
-    name: 'fname.lname',
-  },
+  entityRef: 'user:default/fname.lname',
+};
+
+export const NotFound = (args: EntityPeekAheadPopoverProps) => (
+  <EntityPeekAheadPopover {...args}>
+    <Button>Hover over me to see details about this not found entity</Button>
+  </EntityPeekAheadPopover>
+);
+NotFound.args = {
+  entityRef: 'user:default/doesnt.exist',
+};
+
+export const SlowCatalogItem = (args: EntityPeekAheadPopoverProps) => (
+  <EntityPeekAheadPopover {...args}>
+    <Button>Hover over me to see details about this slow entity</Button>
+  </EntityPeekAheadPopover>
+);
+SlowCatalogItem.args = {
+  entityRef: 'component:default/slow.catalog.item',
 };

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

@@ -14,22 +14,25 @@
  * limitations under the License.
  */
 
-import { fireEvent, render, screen } from '@testing-library/react';
+import { fireEvent, screen } from '@testing-library/react';
 import React from 'react';
 import { EntityPeekAheadPopover } from './EntityPeekAheadPopover';
 import { ApiProvider } from '@backstage/core-app-api';
-import { TestApiRegistry } from '@backstage/test-utils';
+import { TestApiRegistry, renderInTestApp } from '@backstage/test-utils';
 import { catalogApiRef } from '../../api';
 import { CompoundEntityRef, Entity } from '@backstage/catalog-model';
 import { CatalogApi } from '@backstage/catalog-client';
+import { Button } from '@material-ui/core';
+import { entityRouteRef } from '../../routes';
 
 const catalogApi: Partial<CatalogApi> = {
   getEntityByRef: async (
     entityRef: CompoundEntityRef,
   ): Promise<Entity | undefined> => {
     if (
-      entityRef ===
-      { name: 'service1', namespace: 'default', kind: 'component ' }
+      entityRef.name === 'service1' &&
+      entityRef.namespace === 'default' &&
+      entityRef.kind === 'component'
     ) {
       return {
         apiVersion: '',
@@ -51,22 +54,31 @@ const apis = TestApiRegistry.from([catalogApiRef, catalogApi]);
 
 describe('<EntityPeekAheadPopover/>', () => {
   it('renders all owners', async () => {
-    render(
+    renderInTestApp(
       <ApiProvider apis={apis}>
-        <EntityPeekAheadPopover
-          entityRef={{
-            name: 'service1',
-            namespace: 'default',
-            kind: 'component',
-          }}
-        >
-          <div data-testid="popover">asdf</div>
+        <EntityPeekAheadPopover entityRef="component:default/service1">
+          <Button data-testid="popover1">s1</Button>
+        </EntityPeekAheadPopover>
+        <EntityPeekAheadPopover entityRef="component:default/service2">
+          <Button data-testid="popover2">s2</Button>
         </EntityPeekAheadPopover>
       </ApiProvider>,
+      {
+        mountedRoutes: {
+          '/catalog/:namespace/:kind/:name': entityRouteRef,
+        },
+      },
     );
-    expect(screen.getByText('asdf')).toBeInTheDocument();
+    expect(screen.getByText('s1')).toBeInTheDocument();
     expect(screen.queryByText('service1')).toBeNull();
-    fireEvent.mouseOver(screen.getByTestId('popover'));
-    expect(screen.getByText('service1')).toBeInTheDocument();
+    fireEvent.mouseOver(screen.getByTestId('popover1'));
+    expect(await screen.findByText('service1')).toBeInTheDocument();
+
+    expect(screen.getByText('s2')).toBeInTheDocument();
+    expect(screen.queryByText('service2')).toBeNull();
+    fireEvent.mouseOver(screen.getByTestId('popover2'));
+    expect(
+      await screen.findByText(/service2 was not found/),
+    ).toBeInTheDocument();
   });
 });

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

@@ -34,17 +34,17 @@ import {
   Tooltip,
   Typography,
 } from '@material-ui/core';
-import { Alert, Skeleton } from '@material-ui/lab';
+import { Alert } from '@material-ui/lab';
 import EmailIcon from '@material-ui/icons/Email';
 import InfoIcon from '@material-ui/icons/Info';
 import { useApiHolder, useRouteRef } from '@backstage/core-plugin-api';
 import {
-  CompoundEntityRef,
   isUserEntity,
   isGroupEntity,
   UserEntity,
   GroupEntity,
   Entity,
+  parseEntityRef,
 } from '@backstage/catalog-model';
 import { Link, Progress } from '@backstage/core-components';
 
@@ -54,11 +54,14 @@ import { Link, Progress } from '@backstage/core-components';
  * @public
  */
 export type EntityPeekAheadPopoverProps = PropsWithChildren<{
-  entityRef: CompoundEntityRef;
+  entityRef: string;
 }>;
 
 const useStyles = makeStyles(() => {
   return {
+    trigger: {
+      display: 'inline-block',
+    },
     popoverPaper: {
       width: '30em',
     },
@@ -130,33 +133,53 @@ const EntityCardActions = ({ entity }: { entity: Entity }) => {
   );
 };
 
+const EntityNotFoundCard = ({
+  entityRef,
+  error,
+}: {
+  entityRef: string;
+  error?: Error;
+}) => {
+  return (
+    <Card>
+      <CardContent>
+        <Alert severity="warning">
+          {entityRef} was not found {error?.message}
+        </Alert>
+      </CardContent>
+    </Card>
+  );
+};
+
 /**
  * Shows an entity popover on hover of a component.
  *
  * @public
  */
-export const EntityPeekAheadPopover = ({
-  entityRef,
-  children,
-}: EntityPeekAheadPopoverProps) => {
+export const EntityPeekAheadPopover = (props: EntityPeekAheadPopoverProps) => {
+  const { entityRef, children } = props;
+
   const classes = useStyles();
   const apiHolder = useApiHolder();
   const popupState = usePopupState({
     variant: 'popover',
     popupId: 'entity-peek-ahead',
   });
+  const compoundEntityRef = parseEntityRef(entityRef);
 
   const [{ loading, error, value: entity }, load] = useAsyncFn(async () => {
     const catalogApi = apiHolder.get(catalogApiRef);
     if (catalogApi) {
-      const retrievedEntity = await catalogApi.getEntityByRef(entityRef);
+      const retrievedEntity = await catalogApi.getEntityByRef(
+        compoundEntityRef,
+      );
       if (!retrievedEntity) {
-        throw new Error(`${entityRef.name} was not found`);
+        throw new Error(`${compoundEntityRef.name} was not found`);
       }
       return retrievedEntity;
     }
     return undefined;
-  }, [apiHolder, entityRef]);
+  }, [apiHolder, compoundEntityRef]);
 
   useEffect(() => {
     if (popupState.isOpen && !entity && !error && !loading) {
@@ -166,9 +189,9 @@ export const EntityPeekAheadPopover = ({
 
   return (
     <>
-      <div data-testid="trigger" {...bindHover(popupState)}>
+      <span data-testid="trigger" {...bindHover(popupState)}>
         {children}
-      </div>
+      </span>
       <HoverPopover
         PaperProps={{
           className: classes.popoverPaper,
@@ -183,16 +206,20 @@ export const EntityPeekAheadPopover = ({
           horizontal: 'center',
         }}
       >
-        <Card>
+        <>
           {loading && <Progress />}
-          <CardContent>
-            <Typography color="textSecondary">{entityRef.namespace}</Typography>
-            <Typography variant="h5" component="div">
-              {entityRef.name}
-            </Typography>
-            {error && <Alert severity="warning">{error.message}</Alert>}
-            {entity ? (
-              <>
+          {!entity && !loading && (
+            <EntityNotFoundCard entityRef={entityRef} error={error} />
+          )}
+          {entity && (
+            <Card>
+              <CardContent>
+                <Typography color="textSecondary">
+                  {compoundEntityRef.namespace}
+                </Typography>
+                <Typography variant="h5" component="div">
+                  {compoundEntityRef.name}
+                </Typography>
                 <Typography color="textSecondary">{entity.kind}</Typography>
                 <Typography className={classes.descriptionTypography} paragraph>
                   {entity.metadata.description}
@@ -202,35 +229,28 @@ export const EntityPeekAheadPopover = ({
                   {(entity.metadata.tags || [])
                     .slice(0, maxTagChips)
                     .map(tag => {
-                      return <Chip size="small" label={tag} />;
+                      return <Chip key={tag} size="small" label={tag} />;
                     })}
                   {entity.metadata.tags?.length &&
                     entity.metadata.tags?.length > maxTagChips && (
                       <Tooltip title="Drill into the entity to see all of the tags.">
-                        <Chip size="small" label="..." />
+                        <Chip key="other-tags" size="small" label="..." />
                       </Tooltip>
                     )}
                 </Box>
-              </>
-            ) : (
-              <>
-                <Skeleton width="30%" variant="text" />
-                <Skeleton variant="text" />
-                <Skeleton width="50%" variant="text" />
-                <Skeleton width="20%" variant="text" />
-              </>
-            )}
-          </CardContent>
-          <CardActions>
-            {entity && (
-              <>
-                {isUserEntity(entity) && <UserCardActions entity={entity} />}
-                {isGroupEntity(entity) && <GroupCardActions entity={entity} />}
-                <EntityCardActions entity={entity} />
-              </>
-            )}
-          </CardActions>
-        </Card>
+              </CardContent>
+              <CardActions>
+                <>
+                  {isUserEntity(entity) && <UserCardActions entity={entity} />}
+                  {isGroupEntity(entity) && (
+                    <GroupCardActions entity={entity} />
+                  )}
+                  <EntityCardActions entity={entity} />
+                </>
+              </CardActions>
+            </Card>
+          )}
+        </>
       </HoverPopover>
     </>
   );

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

@@ -77,7 +77,7 @@ export const EntityRefLink = forwardRef<any, EntityRefLinkProps>(
     );
 
     return (
-      <EntityPeekAheadPopover entityRef={{ kind, namespace, name }}>
+      <EntityPeekAheadPopover entityRef={`${kind}:${namespace}/${name}`}>
         <Link {...linkProps} ref={ref} to={entityRoute(routeParams)}>
           {children}
           {!children && (title ?? formattedEntityRefTitle)}