From efc100e27743a3145d5b02cac8e09044b256b624 Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Thu, 10 Oct 2024 21:44:53 +0200 Subject: [PATCH 1/2] Handle 404 errors from API during server render --- .../app-pages/ErrorPage/ErrorPageTemplate.tsx | 2 +- .../app-pages/ErrorPage/GlobalErrorPage.tsx | 18 +++++++++ .../src/app/c/[channelType]/[name]/page.tsx | 14 ++++--- frontends/main/src/app/global-error.tsx | 18 +++++++++ frontends/main/src/app/styled.tsx | 2 +- .../main/src/common/handleNotFound.test.ts | 39 +++++++++++++++++++ frontends/main/src/common/handleNotFound.ts | 24 ++++++++++++ frontends/main/src/common/metadata.ts | 22 +++++------ 8 files changed, 118 insertions(+), 21 deletions(-) create mode 100644 frontends/main/src/app-pages/ErrorPage/GlobalErrorPage.tsx create mode 100644 frontends/main/src/app/global-error.tsx create mode 100644 frontends/main/src/common/handleNotFound.test.ts create mode 100644 frontends/main/src/common/handleNotFound.ts diff --git a/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx b/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx index c27c8eee31..54a567d86e 100644 --- a/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx +++ b/frontends/main/src/app-pages/ErrorPage/ErrorPageTemplate.tsx @@ -16,7 +16,7 @@ type ErrorPageTemplateProps = { const ErrorPageTemplate: React.FC = ({ children }) => { return ( - + {/* TODO {`${title} | ${APP_SETTINGS.SITE_NAME}`} diff --git a/frontends/main/src/app-pages/ErrorPage/GlobalErrorPage.tsx b/frontends/main/src/app-pages/ErrorPage/GlobalErrorPage.tsx new file mode 100644 index 0000000000..19c8f77f76 --- /dev/null +++ b/frontends/main/src/app-pages/ErrorPage/GlobalErrorPage.tsx @@ -0,0 +1,18 @@ +"use client" + +import React from "react" +import ErrorPageTemplate from "./ErrorPageTemplate" +import { Typography } from "ol-components" + +const GlobalErrorPage = ({ error }: { error: Pick }) => { + return ( + + + Unexpected Error + + {error.message || ""} + + ) +} + +export default GlobalErrorPage diff --git a/frontends/main/src/app/c/[channelType]/[name]/page.tsx b/frontends/main/src/app/c/[channelType]/[name]/page.tsx index c67a167ab6..b157a3c8ad 100644 --- a/frontends/main/src/app/c/[channelType]/[name]/page.tsx +++ b/frontends/main/src/app/c/[channelType]/[name]/page.tsx @@ -3,6 +3,7 @@ import ChannelPage from "@/app-pages/ChannelPage/ChannelPage" import { channelsApi } from "api/clients" import { ChannelTypeEnum } from "api/v0" import { getMetadataAsync } from "@/common/metadata" +import handleNotFound from "@/common/handleNotFound" type RouteParams = { channelType: ChannelTypeEnum @@ -18,14 +19,15 @@ export async function generateMetadata({ }) { const { channelType, name } = params - const channelDetails = await channelsApi - .channelsTypeRetrieve({ channel_type: channelType, name: name }) - .then((res) => res.data) + const { data } = await handleNotFound( + channelsApi.channelsTypeRetrieve({ channel_type: channelType, name: name }), + ) + return getMetadataAsync({ searchParams, - title: `${channelDetails.title}`, - description: channelDetails.public_description, - image: channelDetails.configuration.logo, + title: `${data.title}`, + description: data.public_description, + image: data.configuration.logo, }) } diff --git a/frontends/main/src/app/global-error.tsx b/frontends/main/src/app/global-error.tsx new file mode 100644 index 0000000000..828260dd5a --- /dev/null +++ b/frontends/main/src/app/global-error.tsx @@ -0,0 +1,18 @@ +"use client" + +/* This is the catch-all error page for catching everythign including root components (layout). + * It is only enabled in production so that in development we see the Next.js error overlay. + * It is passed an error object as an argument, though this has been stripped of everything except + * the message and a digest for server logs correlation; to prevent leaking anything to the client + * + * https://nextjs.org/docs/app/building-your-application/routing/error-handling#handling-errors-in-root-layouts + */ + +import React from "react" +import GlobalErrorPage from "@/app-pages/ErrorPage/GlobalErrorPage" + +const GlobalError = ({ error }: { error: Error }) => { + return +} + +export default GlobalError diff --git a/frontends/main/src/app/styled.tsx b/frontends/main/src/app/styled.tsx index ba2078ad2a..93390ddd48 100644 --- a/frontends/main/src/app/styled.tsx +++ b/frontends/main/src/app/styled.tsx @@ -9,7 +9,7 @@ import { styled } from "ol-components" */ export const PageWrapper = styled.div({ - height: "calc(100vh - 80px)", + height: "100vh", display: "flex", flexDirection: "column", }) diff --git a/frontends/main/src/common/handleNotFound.test.ts b/frontends/main/src/common/handleNotFound.test.ts new file mode 100644 index 0000000000..7978b9e78b --- /dev/null +++ b/frontends/main/src/common/handleNotFound.test.ts @@ -0,0 +1,39 @@ +import type { AxiosError } from "axios" +import handleNotFound from "./handleNotFound" +import { nextNavigationMocks } from "ol-test-utilities/mocks/nextNavigation" + +describe("Handle not found wrapper utility", () => { + test("Should call notFound() for errors with status 404", async () => { + const error: Partial = { + status: 404, + message: "Not Found", + } + + const promise = Promise.reject(error) + + await handleNotFound(promise) + + expect(nextNavigationMocks.notFound).toHaveBeenCalled() + }) + + test("Should not call notFound() for success and return result", async () => { + const resolvedValue = { data: "success" } + const promise = Promise.resolve(resolvedValue) + + const result = await handleNotFound(promise) + + expect(result).toEqual(resolvedValue) + expect(nextNavigationMocks.notFound).not.toHaveBeenCalled() + }) + + test("Should rethrow non 404 errors", async () => { + const error = new Error("Something went wrong") + + const promise = Promise.reject(error) + + await expect(handleNotFound(promise)).rejects.toThrow( + "Something went wrong", + ) + expect(nextNavigationMocks.notFound).not.toHaveBeenCalled() + }) +}) diff --git a/frontends/main/src/common/handleNotFound.ts b/frontends/main/src/common/handleNotFound.ts new file mode 100644 index 0000000000..f34a77bece --- /dev/null +++ b/frontends/main/src/common/handleNotFound.ts @@ -0,0 +1,24 @@ +import type { AxiosError } from "axios" +import { notFound } from "next/navigation" + +/* This is intended to wrap API calls that fetch resources during server render, + * such as to gather metadata for the learning resource drawer. + * + * The ./app/global-error.tsx boundary for root layout errors is only supplied the + * error message so we cannot determine that it is a 404 to show the NotFoundPage. + * Instead we must handle at each point of use so need a utility function. Below we + * use next/navigation's notFound() to render ./app/not-found.tsx + */ + +const handleNotFound = async (promise: Promise): Promise => { + try { + return await promise + } catch (error) { + if ((error as AxiosError).status === 404) { + return notFound() + } + throw error + } +} + +export default handleNotFound diff --git a/frontends/main/src/common/metadata.ts b/frontends/main/src/common/metadata.ts index c16aff390b..c112873175 100644 --- a/frontends/main/src/common/metadata.ts +++ b/frontends/main/src/common/metadata.ts @@ -1,6 +1,7 @@ import { RESOURCE_DRAWER_QUERY_PARAM } from "@/common/urls" import { learningResourcesApi } from "api/clients" import type { Metadata } from "next" +import handleNotFound from "./handleNotFound" const DEFAULT_OG_IMAGE = "/images/learn-og-image.jpg" @@ -29,21 +30,16 @@ export const getMetadataAsync = async ({ // The learning resource drawer is open const learningResourceId = searchParams?.[RESOURCE_DRAWER_QUERY_PARAM] if (learningResourceId) { - try { - const { data } = await learningResourcesApi.learningResourcesRetrieve({ + const { data } = await handleNotFound( + learningResourcesApi.learningResourcesRetrieve({ id: Number(learningResourceId), - }) + }), + ) - title = data?.title - description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? "" - image = data?.image?.url || image - imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || "" - } catch (error) { - console.warn("Failed to fetch learning resource", { - learningResourceId, - error, - }) - } + title = data?.title + description = data?.description?.replace(/<\/[^>]+(>|$)/g, "") ?? "" + image = data?.image?.url || image + imageAlt = image === data?.image?.url ? imageAlt : data?.image?.alt || "" } return standardizeMetadata({ From 9031c1a1ed9e637ef6b7c9bcb59f20cccb235754 Mon Sep 17 00:00:00 2001 From: Jon Kafton <939376+jonkafton@users.noreply.github.com> Date: Thu, 10 Oct 2024 22:15:11 +0200 Subject: [PATCH 2/2] Spelling --- frontends/main/src/app/global-error.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontends/main/src/app/global-error.tsx b/frontends/main/src/app/global-error.tsx index 828260dd5a..43d2266c5c 100644 --- a/frontends/main/src/app/global-error.tsx +++ b/frontends/main/src/app/global-error.tsx @@ -1,6 +1,7 @@ "use client" -/* This is the catch-all error page for catching everythign including root components (layout). +/* This is the catch-all error page that receives errors from server rendered root layout + * components and metadata. * It is only enabled in production so that in development we see the Next.js error overlay. * It is passed an error object as an argument, though this has been stripped of everything except * the message and a digest for server logs correlation; to prevent leaking anything to the client