ref(remix): Rework Error Handling (#9725)

Author: onurtemizkan

packages/e2e-tests/test-applications/create-remix-app-v2/app/entry.server.tsx

@@ -24,9 +24,7 @@ Sentry.init({
   tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
 });
 
-export function handleError(error: unknown, { request }: DataFunctionArgs): void {
-  Sentry.captureRemixServerException(error, 'remix.server', request);
-}
+export const handleError = Sentry.wrapRemixHandleError;
 
 export default function handleRequest(
   request: Request,

packages/e2e-tests/test-applications/create-remix-app/app/entry.server.tsx

@@ -21,9 +21,7 @@ Sentry.init({
   tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
 });
 
-export function handleError(error: unknown, { request }: DataFunctionArgs): void {
-  Sentry.captureRemixServerException(error, 'remix.server', request);
-}
+export const handleError = Sentry.wrapRemixHandleError;
 
 export default function handleRequest(
   request: Request,

packages/remix/src/client/errors.tsx

@@ -1,8 +1,7 @@
 import { captureException } from '@sentry/core';
-import { isNodeEnv, isString } from '@sentry/utils';
+import { isNodeEnv } from '@sentry/utils';
 
-import { isRouteErrorResponse } from '../utils/vendor/response';
-import type { ErrorResponse } from '../utils/vendor/types';
+import { isResponse } from '../utils/vendor/response';
 
 /**
  * Captures an error that is thrown inside a Remix ErrorBoundary.
@@ -11,57 +10,26 @@ import type { ErrorResponse } from '../utils/vendor/types';
  * @returns void
  */
 export function captureRemixErrorBoundaryError(error: unknown): string | undefined {
-  let eventId: string | undefined;
-  const isClientSideRuntimeError = !isNodeEnv() && error instanceof Error;
-  // We only capture `ErrorResponse`s that are 5xx errors.
-  const isRemixErrorResponse = isRouteErrorResponse(error) && error.status >= 500;
-  // Server-side errors apart from `ErrorResponse`s also appear here without their stacktraces.
-  // So, we only capture:
-  //    1. `ErrorResponse`s
-  //    2. Client-side runtime errors here,
-  //    And other server-side errors captured in `handleError` function where stacktraces are available.
-  if (isRemixErrorResponse || isClientSideRuntimeError) {
-    const eventData = isRemixErrorResponse
-      ? {
-          function: 'ErrorResponse',
-          ...getErrorData(error),
-        }
-      : {
-          function: 'ReactError',
-        };
-
-    const actualError = isRemixErrorResponse ? getExceptionToCapture(error) : error;
-
-    eventId = captureException(actualError, {
-      mechanism: {
-        type: 'instrument',
-        handled: false,
-        data: eventData,
-      },
-    });
-  }
-
-  return eventId;
-}
-
-function getErrorData(error: ErrorResponse): object {
-  if (isString(error.data)) {
-    return {
-      error: error.data,
-    };
+  // Server-side errors also appear here without their stacktraces.
+  // So, we only capture client-side runtime errors here.
+  // ErrorResponses that are 5xx errors captured at loader / action level by `captureRemixRouteError` function,
+  // And other server-side errors captured in `handleError` function where stacktraces are available.
+  //
+  // We don't want to capture:
+  // - Response Errors / Objects [They are originated and handled on the server-side]
+  // - SSR Errors [They are originated and handled on the server-side]
+  // - Anything without a stacktrace [Remix trims the stacktrace of the errors that are thrown on the server-side]
+  if (isResponse(error) || isNodeEnv() || !(error instanceof Error)) {
+    return;
   }
 
-  return error.data;
-}
-
-function getExceptionToCapture(error: ErrorResponse): string | ErrorResponse {
-  if (isString(error.data)) {
-    return error.data;
-  }
-
-  if (error.statusText) {
-    return error.statusText;
-  }
-
-  return error;
+  return captureException(error, {
+    mechanism: {
+      type: 'instrument',
+      handled: false,
+      data: {
+        function: 'ReactError',
+      },
+    },
+  });
 }

packages/remix/src/index.server.ts

@@ -59,7 +59,7 @@ export {
 // Keeping the `*` exports for backwards compatibility and types
 export * from '@sentry/node';
 
-export { captureRemixServerException } from './utils/instrumentServer';
+export { captureRemixServerException, wrapRemixHandleError } from './utils/instrumentServer';
 export { ErrorBoundary, withErrorBoundary } from '@sentry/react';
 export { remixRouterInstrumentation, withSentry } from './client/performance';
 export { captureRemixErrorBoundaryError } from './client/errors';

packages/remix/src/utils/instrumentServer.ts

@@ -8,8 +8,10 @@ import {
   dynamicSamplingContextToSentryBaggageHeader,
   fill,
   isNodeEnv,
+  isPrimitive,
   loadModule,
   logger,
+  objectify,
   tracingContextFromHeaders,
 } from '@sentry/utils';
 
@@ -70,6 +72,28 @@ async function extractResponseError(response: Response): Promise<unknown> {
   return responseData;
 }
 
+/**
+ * Sentry utility to be used in place of `handleError` function of Remix v2
+ * Remix Docs: https://remix.run/docs/en/main/file-conventions/entry.server#handleerror
+ *
+ * Should be used in `entry.server` like:
+ *
+ * export const handleError = Sentry.wrapRemixHandleError
+ */
+export function wrapRemixHandleError(err: unknown, { request }: DataFunctionArgs): void {
+  // We are skipping thrown responses here as they are handled by
+  // `captureRemixServerException` at loader / action level
+  // We don't want to capture them twice.
+  // This function if only for capturing unhandled server-side exceptions.
+  // https://remix.run/docs/en/main/file-conventions/entry.server#thrown-responses
+  // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders
+  if (isResponse(err) || isRouteErrorResponse(err)) {
+    return;
+  }
+
+  void captureRemixServerException(err, 'remix.server.handleError', request);
+}
+
 /**
  * Captures an exception happened in the Remix server.
  *
@@ -107,7 +131,9 @@ export async function captureRemixServerException(err: unknown, name: string, re
     DEBUG_BUILD && logger.warn('Failed to normalize Remix request');
   }
 
-  captureException(isResponse(err) ? await extractResponseError(err) : err, scope => {
+  const objectifiedErr = objectify(err);
+
+  captureException(isResponse(objectifiedErr) ? await extractResponseError(objectifiedErr) : objectifiedErr, scope => {
     const activeTransactionName = getActiveTransaction()?.name;
 
     scope.setSDKProcessingMetadata({
@@ -138,7 +164,7 @@ export async function captureRemixServerException(err: unknown, name: string, re
   });
 }
 
-function makeWrappedDocumentRequestFunction(remixVersion: number) {
+function makeWrappedDocumentRequestFunction(remixVersion?: number) {
   return function (origDocumentRequestFunction: HandleDocumentRequestFunction): HandleDocumentRequestFunction {
     return async function (
       this: unknown,
@@ -149,7 +175,6 @@ function makeWrappedDocumentRequestFunction(remixVersion: number) {
       loadContext?: Record<string, unknown>,
     ): Promise<Response> {
       let res: Response;
-
       const activeTransaction = getActiveTransaction();
 
       try {
@@ -174,7 +199,12 @@ function makeWrappedDocumentRequestFunction(remixVersion: number) {
 
         span?.finish();
       } catch (err) {
-        if (!FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2) {
+        const isRemixV1 = !FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2;
+
+        // This exists to capture the server-side rendering errors on Remix v1
+        // On Remix v2, we capture SSR errors at `handleError`
+        // We also skip primitives here, as we can't dedupe them, and also we don't expect any primitive SSR errors.
+        if (isRemixV1 && !isPrimitive(err)) {
           await captureRemixServerException(err, 'documentRequest', request);
         }
 
@@ -217,7 +247,12 @@ function makeWrappedDataFunction(
       currentScope.setSpan(activeTransaction);
       span?.finish();
     } catch (err) {
-      if (!FUTURE_FLAGS?.v2_errorBoundary && remixVersion !== 2) {
+      const isRemixV2 = FUTURE_FLAGS?.v2_errorBoundary || remixVersion === 2;
+
+      // On Remix v2, we capture all unexpected errors (except the `Route Error Response`s / Thrown Responses) in `handleError` function.
+      // This is both for consistency and also avoid duplicates such as primitives like `string` or `number` being captured twice.
+      // Remix v1 does not have a `handleError` function, so we capture all errors here.
+      if (isRemixV2 ? isResponse(err) : true) {
         await captureRemixServerException(err, name, args.request);
       }
 
@@ -240,7 +275,10 @@ const makeWrappedLoader =
     return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion);
   };
 
-function getTraceAndBaggage(): { sentryTrace?: string; sentryBaggage?: string } {
+function getTraceAndBaggage(): {
+  sentryTrace?: string;
+  sentryBaggage?: string;
+} {
   const transaction = getActiveTransaction();
   const currentScope = getCurrentHub().getScope();
 
@@ -287,7 +325,11 @@ function makeWrappedRootLoader(remixVersion: number) {
           if (typeof data === 'object') {
             return json(
               { ...data, ...traceAndBaggage, remixVersion },
-              { headers: res.headers, statusText: res.statusText, status: res.status },
+              {
+                headers: res.headers,
+                statusText: res.statusText,
+                status: res.status,
+              },
             );
           } else {
             DEBUG_BUILD && logger.warn('Skipping injection of trace and baggage as the response body is not an object');
@@ -498,7 +540,9 @@ function makeWrappedCreateRequestHandler(
  * which Remix Adapters (https://remix.run/docs/en/v1/api/remix) use underneath.
  */
 export function instrumentServer(): void {
-  const pkg = loadModule<{ createRequestHandler: CreateRequestHandlerFunction }>('@remix-run/server-runtime');
+  const pkg = loadModule<{
+    createRequestHandler: CreateRequestHandlerFunction;
+  }>('@remix-run/server-runtime');
 
   if (!pkg) {
     DEBUG_BUILD && logger.warn('Remix SDK was unable to require `@remix-run/server-runtime` package.');

packages/remix/src/utils/vendor/types.ts

@@ -108,7 +108,12 @@ export type DeferredData = {
 };
 
 export interface MetaFunction {
-  (args: { data: AppData; parentsData: RouteData; params: Params; location: Location }): HtmlMetaDescriptor;
+  (args: {
+    data: AppData;
+    parentsData: RouteData;
+    params: Params;
+    location: Location;
+  }): HtmlMetaDescriptor;
 }
 
 export interface HtmlMetaDescriptor {
@@ -143,7 +148,11 @@ export interface LoaderFunction {
 }
 
 export interface HeadersFunction {
-  (args: { loaderHeaders: Headers; parentHeaders: Headers; actionHeaders: Headers }): Headers | HeadersInit;
+  (args: {
+    loaderHeaders: Headers;
+    parentHeaders: Headers;
+    actionHeaders: Headers;
+  }): Headers | HeadersInit;
 }
 
 export interface ServerRouteModule extends EntryRouteModule {

packages/remix/test/integration/app_v1/routes/server-side-unexpected-errors/$id.tsx

@@ -0,0 +1,2 @@
+export * from '../../../common/routes/server-side-unexpected-errors.$id';
+export { default } from '../../../common/routes/server-side-unexpected-errors.$id';

packages/remix/test/integration/app_v1/routes/ssr-error.tsx

@@ -0,0 +1,2 @@
+export * from '../../common/routes/ssr-error';
+export { default } from '../../common/routes/ssr-error';

packages/remix/test/integration/app_v2/entry.server.tsx

@@ -11,9 +11,7 @@ Sentry.init({
   autoSessionTracking: false,
 });
 
-export function handleError(error: unknown, { request }: DataFunctionArgs): void {
-  Sentry.captureRemixServerException(error, 'remix.server', request);
-}
+export const handleError = Sentry.wrapRemixHandleError;
 
 export default function handleRequest(
   request: Request,

packages/remix/test/integration/app_v2/routes/server-side-unexpected-errors.$id.tsx

@@ -0,0 +1,2 @@
+export * from '../../common/routes/server-side-unexpected-errors.$id';
+export { default } from '../../common/routes/server-side-unexpected-errors.$id';

packages/remix/test/integration/app_v2/routes/ssr-error.tsx

@@ -0,0 +1,2 @@
+export * from '../../common/routes/ssr-error';
+export { default } from '../../common/routes/ssr-error';

packages/remix/test/integration/common/routes/action-json-response.$id.tsx

@@ -2,7 +2,7 @@ import { ActionFunction, LoaderFunction, json, redirect } from '@remix-run/node'
 import { useActionData } from '@remix-run/react';
 
 export const loader: LoaderFunction = async ({ params: { id } }) => {
-  if (id === '-1') {
+  if (id === '-100') {
     throw new Error('Unexpected Server Error');
   }
 
@@ -16,7 +16,7 @@ export const action: ActionFunction = async ({ params: { id } }) => {
 
   if (id === '-2') {
     // Note: This GET request triggers the `Loader` of the URL, not the `Action`.
-    throw redirect('/action-json-response/-1');
+    throw redirect('/action-json-response/-100');
   }
 
   if (id === '-3') {

packages/remix/test/integration/common/routes/server-side-unexpected-errors.$id.tsx

@@ -0,0 +1,27 @@
+import { ActionFunction, LoaderFunction, json, redirect } from '@remix-run/node';
+import { useActionData } from '@remix-run/react';
+
+export const action: ActionFunction = async ({ params: { id } }) => {
+  // Throw string
+  if (id === '-1') {
+    throw 'Thrown String Error';
+  }
+
+  // Throw object
+  if (id === '-2') {
+    throw {
+      message: 'Thrown Object Error',
+      statusCode: 500,
+    };
+  }
+};
+
+export default function ActionJSONResponse() {
+  const data = useActionData();
+
+  return (
+    <div>
+      <h1>{data && data.test ? data.test : 'Not Found'}</h1>
+    </div>
+  );
+}

packages/remix/test/integration/common/routes/ssr-error.tsx

@@ -0,0 +1,7 @@
+export default function SSRError() {
+  const data = ['err'].map(err => {
+    throw new Error('Sentry SSR Test Error');
+  });
+
+  return <div>{data}</div>;
+}

packages/remix/test/integration/test/client/ssr-error.test.ts

@@ -0,0 +1,8 @@
+import { expect, test } from '@playwright/test';
+import { countEnvelopes } from './utils/helpers';
+
+test('should not report an SSR error on client side.', async ({ page }) => {
+  const count = await countEnvelopes(page, { url: '/ssr-error', envelopeType: 'event' });
+
+  expect(count).toBe(0);
+});