Setup root error boundary to gracefully handle all unhandled errors

[nitishdhar]

• Adds a root level error boundary component
• Catches all unhandled client side or server side errors and renders this error component with a button to attempt retry
• Updates README to include the clean script I had added in a previous commit
• Adds playwright tests to validate the error boundary behavior for both server and client side errors
• Apply standard UX to 404 and Error pages
• Retains detailed error message pop up during local development
• Closes Setup root error boundary to gracefully handle all unhandled client & server side errors #103

[aslink87]

It's a small PR but i'm having a tough time wrapping my head around it. What makes it challenging is it's centered around the playground route which uses fetch calls and page structures differing from all the other routes. Perhaps we should really convert our faq route into an example form page so these kind of feature PRs can be built upon an example that easily correlates to the majority of our routes. Or update this playground route to serve in that capacity.

Going off of what you've added here what files would have to be added to each route, let's say the /commodities route? That route can present errors at the page.tsx level if it fails to fetch cached content from the API. It can also present errors at the page.client.tsx level if it fails to submit fetch calls upon form submissions. My first thought is we'd want to catch both of those errors separately as an error at the page.tsx level should prevent the page.client.tsx from rendering and explain why to the user. Errors as the page.client.tsx level should tell the user why the form submission failed.

I'm interested in diving more into what you've implemented here so that I, and other devs, can get on the same wavelength.

[aslink87]

Could you explain the necessity of this please at the page.tsx level?

At this level I don't foresee us fetching anything that we don't want cached such as a list of commodity names that don't really change.

From what I understand NextJS will automatically switch these pages to dynamically rendered if uncached data is detected. It seems to go against the premise of caching if we were to force these routes to be dynamic.

[nitishdhar]

I had removed it in one of the commits in this PR based on the same understanding that it should not make these fetch calls during build but for some reason it was still making these calls during the github actions build. Have added this for now to fix that issue until I look into what's causing this to make these calls. However, it is needed in the other server-error scenario because I am not doing anything there for nextJs to detect that it should be done dynamically.

[nitishdhar]

I also found this useful implementation to simulate github actions when needed, more details on the purpose and intent - #112. Will implement this to help with debugging these issues.

[aslink87]

on the same understanding that it should not make these fetch calls during build

The calls that populate cached data at the page.tsx level should happen at build (atm we only do this to fetch an updated list of possible commodities). The data is required by the children. But you're probably talking specifically about the playground route, where you're right...though it's an exception to use 'no-cache' in our app.

From what I'm understanding here, there's a build error w/o this dynamic flag that occurs in the playground route because the server fetches are set to no-cache.

[nitishdhar]

Updated this to use cache config.

[nitishdhar]

It still fails - https://github.com/ed-pilots-network/frontend/actions/runs/6192011962/job/16811315676. I will debug this once I setup act (#112) for our package so I can do this locally. For now, have reverted back to forcing dynamic rendering.

[nitishdhar]

It's a small PR but i'm having a tough time wrapping my head around it. What makes it challenging is it's centered around the playground route which uses fetch calls and page structures differing from all the other routes. Perhaps we should really convert our faq route into an example form page so these kind of feature PRs can be built upon an example that easily correlates to the majority of our routes. Or update this playground route to serve in that capacity.

Not sure what you mean by playground using a different routing. I can update it to match other routes but I am have an issue created to implement swr and I am expecting to propagate the new fetch pattern across the application with that change, including playground. If you can clarify the route difference, I can bring playground to match that.

Going off of what you've added here what files would have to be added to each route, let's say the /commodities route? That route can present errors at the page.tsx level if it fails to fetch cached content from the API. It can also present errors at the page.client.tsx level if it fails to submit fetch calls upon form submissions. My first thought is we'd want to catch both of those errors separately as an error at the page.tsx level should prevent the page.client.tsx from rendering and explain why to the user. Errors as the page.client.tsx level should tell the user why the form submission failed.

I'm interested in diving more into what you've implemented here so that I, and other devs, can get on the same wavelength.

Ahh, may be I should have been explicit about the intent, this change is just specific to be the catch-all root level error handler, it is working as expected i.e., if the error isn't handled at page level, this will catch all those. I don't have an example in this change that shows how we will do this on specific pages. I have now created an issue #111 to show case that for page level handling both between server and client side.

About form submission failure type issues, e.g., if you see playground has a commodity fetch call to local API which is done from the client, ideally these errors should be handled and will show a specific localized error message instead of this catch-all pattern. This error boundary pattern is for unexpected errors that usually end up with 'blank screen' rendering in the UI.

[aslink87]

Not sure what you mean by playground using a different routing. I can update it to match other routes but I am have an issue created to implement swr and I am expecting to propagate the new fetch pattern across the application with that change, including playground. If you can clarify the route difference, I can bring playground to match that.

Most routes involve a page.tsx with a server fetch for cached data, and a client.tsx containing fetches from a child form. If either playground or faq (perhaps renamed to template) was also configured this way, then when we make future updates that effect each route it'll be easier to comprehend how the update integrates into each route. In this PR, we went out of our way to setup a dynamic server flag b/c the route on which the PR is built is unique in how it handles server calls compared to all other routes in our app. So the example contains code that shouldn't be duplicated elsewhere.

If you intend to use the playground as a 'scratchpad' so to speak, we should just update the faq route to serve as a template route with some fetches and a proper outline that can be copied when new routes are created. I'd be happy to do this for the team. We can have different PRs to do this, and present route-level error boundaries if we want to do so.

With your response I have a better grasp on what the change is doing at the root level. Thanks. Since the PR is covering down on root level errors, perhaps it's necessary to add a global-error.tsx to cover errors at the layout level. root error.js doesn't cover app/layout.js

[aslink87]

Returning to this, we can merge this in once the maxWidth comment is addressed, and a global-error file is added.

The dynamic build flag caused by the no-cache fetch call is a one-off for the app at the moment. Our other routes will have cached server calls. So it's perfectly fine to handle it like you did. It's odd that next requires a const dynamic = 'force-dynamic' option as well as the no-cache flag. The docs suggest that either should suffice on their own.

Thanks again for setting this up.

[nitishdhar]

With your response I have a better grasp on what the change is doing at the root level. Thanks. Since the PR is covering down on root level errors, perhaps it's necessary to add a global-error.tsx to cover errors at the layout level. root error.js doesn't cover app/layout.js

I had considered global-error but it also specifies that this isn't usually needed because those files aren't as dynamic as page / content files, they don't depend on dynamic data / API calls which can cause these unhandled errors. Decided to not apply this to, I can add it if you feel strong about it, I am fine either way.

[nitishdhar]

Returning to this, we can merge this in once the maxWidth comment is addressed, and a global-error file is added.

The dynamic build flag caused by the no-cache fetch call is a one-off for the app at the moment. Our other routes will have cached server calls. So it's perfectly fine to handle it like you did. It's odd that next requires a const dynamic = 'force-dynamic' option as well as the no-cache flag. The docs suggest that either should suffice on their own.

Thanks again for setting this up.

I am switching the playground server code to set cache as other fetches do. I will have to keep the ones that trigger errors intentionally for testing purposes.

[aslink87]

lgtm