Merge pull request #1656 from thunderstore-io/eb2-error-logging

Replace root error boundary with RouteErrorBoundary

Author: anttimaki

apps/cyberstorm-remix/app/commonComponents/ErrorBoundary/RouteErrorBoundary.tsx

@@ -1,5 +1,6 @@
-import type { JSX } from "react";
+import { useEffect, type JSX } from "react";
 import { isRouteErrorResponse, useRouteError } from "react-router";
+import { captureRemixErrorBoundaryError } from "@sentry/remix";
 
 import { ApiError } from "@thunderstore/thunderstore-api";
 
@@ -15,6 +16,17 @@ type StatusCode = number | "???";
 export function RouteErrorBoundary() {
   const error = useRouteError();
 
+  // Seeing double console logs caused by React's strictMode (that
+  // *should* happen only in dev mode) makes me want to ensure any
+  // rerenders don't get logged in Sentry twice.
+  useEffect(() => {
+    if (error && import.meta.env.PROD) {
+      captureRemixErrorBoundaryError(error);
+    } else if (error) {
+      console.error("Error boundary caught error", error);
+    }
+  }, [error]);
+
   let statusCode: StatusCode = "???";
 
   if (isRouteErrorResponse(error)) {

apps/cyberstorm-remix/app/root.tsx

@@ -10,11 +10,9 @@ import {
   ScrollRestoration,
   type ShouldRevalidateFunctionArgs,
   type UIMatch,
-  isRouteErrorResponse,
   useLoaderData,
   useLocation,
   useMatches,
-  useRouteError,
 } from "react-router";
 // import { LinksFunction } from "@remix-run/react/dist/routeModules";
 import { Provider as RadixTooltip } from "@radix-ui/react-tooltip";
@@ -30,7 +28,7 @@ import {
 import { DapperTs } from "@thunderstore/dapper-ts";
 import { type CurrentUser } from "@thunderstore/dapper/types";
 
-import { captureRemixErrorBoundaryError, withSentry } from "@sentry/remix";
+import { withSentry } from "@sentry/remix";
 import { memo, type ReactNode, Suspense, useEffect, useRef } from "react";
 import { useHydrated } from "remix-utils/use-hydrated";
 import Toast from "@thunderstore/cyberstorm/src/newComponents/Toast";
@@ -600,42 +598,7 @@ function App() {
 }
 
 export default withSentry(App);
-
-// REMIX TODO: We don't have any data available in the root ErrorBoundary, so we might want to change the designs
-export function ErrorBoundary() {
-  // REMIX TODO: We need to call the loader separately somehow to get the CurrentUser
-  // const loaderOutput = useLoaderData<RootLoadersType>();
-  // const parsedLoaderOutput: {
-  //   envStuff: { ENV: { PUBLIC_API_URL: string } };
-  //   sessionId: string | null;
-  //   currentUser: CurrentUser;
-  // } = JSON.parse(JSON.stringify(loaderOutput));
-  const error = useRouteError();
-  if (import.meta.env.PROD) {
-    captureRemixErrorBoundaryError(error);
-  } else if (import.meta.env.DEV) {
-    console.log(error);
-  }
-  const isResponseError = isRouteErrorResponse(error);
-  return (
-    <div className="error">
-      <div
-        className="error__glitch"
-        data-text={isResponseError ? error.status : 500}
-      >
-        <span>{isResponseError ? error.status : 500}</span>
-      </div>
-      <div className="error__description">
-        {isResponseError ? error.data : "Internal server error"}
-      </div>
-      {!isResponseError && (
-        <div className="error__flavor">
-          Beep boop. Server something error happens.
-        </div>
-      )}
-    </div>
-  );
-}
+export { RouteErrorBoundary as ErrorBoundary } from "app/commonComponents/ErrorBoundary";
 
 // Temporary solution for implementing ads
 // REMIX TODO: Move to dynamic html

apps/cyberstorm-remix/docs/error-boundary.md

@@ -14,9 +14,7 @@ export { RouteErrorBoundary as ErrorBoundary } from "app/commonComponents/ErrorB
 
 ### TODO
 
-- Consider replacing the error boundary in `root.tsx` with this one
 - Use `RouteErrorBoundary` in route components
-- Investigate if [error reporting](https://reactrouter.com/how-to/error-reporting) (via Sentry) should be done separately or implement as a part of the error boundary
 - Add "retry buttons" (e.g. reloading the page on 500 series of errors)
 - As `RouteErrorBoundary` is all-or-nothing solution when it comes to rendering, consider if there's a need for more granular non-route component level error boundaries. We might not need or want to use them, as error boundaries are intended more for render time errors, and any errors caused by user interaction should be handled by the component logic itself
   - If it turns out we don't want other error boundaries, consider renaming `RouteErrorBoundary` to just `ErrorBoundary`