feat: 404 page for entity not found in catalog #2623
Conversation
|
I think you should remove both the header and the ContentHeader in this case. |
|
@stefanalund I can do that. |
|
@BA1RY go ahead 👍 |
|
@stefanalund I've made the changes you've suggested. Let me know if there are any corrections.
|
| }, REDIRECT_DELAY); | ||
| } | ||
| // Commenting to check EntityNotFound page | ||
| // if (error || (!loading && !entity)) { |
There was a problem hiding this comment.
@shmidt-i Can this code block be removed now that we are not redirecting to /catalog when entity isn't found?
There was a problem hiding this comment.
Yes, please remove them and then we can merge!
| }, REDIRECT_DELAY); | ||
| } | ||
| // Commenting to check EntityNotFound page | ||
| // if (error || (!loading && !entity)) { |
There was a problem hiding this comment.
Yes, please remove them and then we can merge!
| @@ -20,7 +20,7 @@ import { catalogApiRef } from '../api/types'; | |||
| import { useAsync } from 'react-use'; | |||
| import { Entity } from '@backstage/catalog-model'; | |||
|
|
|||
| const REDIRECT_DELAY = 2000; | |||
| // const REDIRECT_DELAY = 2000; | |||
| }, | ||
| })); | ||
|
|
||
| export const EntityNotFound: FC<{}> = () => { |
There was a problem hiding this comment.
|
@stefanalund I've made the changes you've asked for. |
| @@ -47,7 +48,7 @@ const DefaultEntityPage = () => ( | |||
| const EntityPageSwitch = ({ EntityPage }: { EntityPage: ComponentType }) => { | |||
| const { entity } = useEntity(); | |||
| // Loading and error states | |||
| if (!entity) return <EntityPageLayout />; | |||
| if (!entity) return <EntityNotFound />; | |||
There was a problem hiding this comment.
Oh no, that's probably not gonna work nicely. Now there is going to be a flickering when entity is being loaded, so the EntityNotFound will be first to show, then immediately after we get an entity it will be replaced with an EntityPageLayout.
You need to decouple loading from the error state, if loading then just return <EntityPageLayout/>.
https://github.com/spotify/backstage/blob/0d33fdba1e278da69c1b81bae4d4d433ac19377b/plugins/catalog/src/hooks/useEntity.ts#L70
You can also make useEntity hook to return if the entity is loading/error. (keep in mind that there is also case where the entity hasn't been fetched yet, hence loading: false, entity: undefined, error: undefined
There was a problem hiding this comment.
@shmidt-i I've made the changes. Please check it
There was a problem hiding this comment.
LGTM, now some type fixes and can merge ! 😃
| @@ -58,6 +58,8 @@ export const useEntityFromUrl = (): EntityLoadingStatus => { | |||
| * Always going to return an entity, or throw an error if not a descendant of a EntityProvider. | |||
| */ | |||
| export const useEntity = () => { | |||
| const { entity } = useContext<{ entity: Entity }>(EntityContext as any); | |||
| return { entity }; | |||
| const { entity, loading, error } = useContext<EntityLoadingStatus>( | |||
There was a problem hiding this comment.
@shmidt-i I noticed that since entity is optional in EntityLoadingStatus it is giving error. If we pass { entity: Entity; loading: boolean; error: Error; } instead of EntityLoadingStatus to useContext, the error is resolved. Should I make that change?
There was a problem hiding this comment.
Yes, let's start with that 👍
There was a problem hiding this comment.
I mean you literally just covered the entity is not there case :)
There was a problem hiding this comment.
Thanks. I've made the changes. Please check it
Hey, I just made a Pull Request!
Addresses #2266
Task: Creation of 404 page for Entity not found
Solution: Created EntityNotFound component for catalog which displays the 404 page when entity is not found.
To test the page:
Visit url http://localhost:3000/catalog/Component/xyz (where xyz component doesn't exist).
Screenshot:
✔️ Checklist
yarn test