Skip to content

Conversation

@jonkafton
Copy link
Contributor

@jonkafton jonkafton commented Oct 10, 2024

What are the relevant tickets?

Fixes: https://github.com/mitodl/hq/issues/5734

Description (What does it do?)

Renders the not found page and returns a 404 status for requests for resources that are not found during server render.

  • Provides a utility function to wrap API calls that may produce 404s so we can navigate to the not found page.
  • Also adds a global catch all error page so we are not displaying the default Next.js mostly blank screen:
image

How can this be tested?

Request a page with a drawer open for a non existing resource. The not found page html should be returned with 404 status, e.g. http://open.odl.local:8062/?resource=noexist

Note that the global error page is not active in dev mode. To test we need to throw and error in any server component and then yarn build; yarn start;. The error message should be displayed (in place of "Something went wrong" in the image above).

Additional Context

Next.js provides a file convention for catching errors from root layout (and generateMetadata()), however they are stripped to just { message, digest }, so we are not able to detect 404s to show the not found page. Instead we need to handle errors in place, in this case by try..catching around our API calls and calling next/navigations notFound() to render the ./app/not-found.tsx page.

@jonkafton jonkafton force-pushed the jk/5734-server-error-boundary branch from 458f192 to efc100e Compare October 10, 2024 20:03

export const PageWrapper = styled.div({
height: "calc(100vh - 80px)",
height: "100vh",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the gap at the bottom of the page:

image

return (
<Container maxWidth="sm">
<MuiCard sx={{ marginTop: "1rem" }}>
<MuiCard sx={{ marginTop: "4rem" }}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were colliding:

image

After:
image

@gumaerc gumaerc self-assigned this Oct 11, 2024

export const PageWrapper = styled.div({
height: "calc(100vh - 80px)",
height: "100vh",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gumaerc you're well versed on this it seems! Let's make sure this change doesn't interfere with your more considered PR.

Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried setting NODE_ENV=production to test throwing an error and seeing the global error page, but I kept getting this error on every page with NODE_ENV set to anything but development:

image

I don't think that error has anything to do with the changes here, but nonetheless I wasn't able to test the error handling component of this. It does seem to be set up correctly in global-error.tsx, but I did see this in the documentation (https://nextjs.org/docs/app/building-your-application/routing/error-handling#handling-errors-in-root-layouts):

Even if a global-error.js is defined, it is still recommended to define a root error.js whose fallback component will be rendered within the root layout, which includes globally shared UI and branding.

I'm not worried about this, since it seems your strategy of creating ErrorPageTemplate includes the header and it looks okay in your screenshots, but I thought it worth mentioning.

@jonkafton jonkafton merged commit 610a7a3 into nextjs Oct 15, 2024
12 checks passed
@odlbot odlbot mentioned this pull request Oct 22, 2024
74 tasks
@rhysyngsun rhysyngsun deleted the jk/5734-server-error-boundary branch February 7, 2025 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants