Do not capture ErrorResponses in ErrorBoundary

Author: onurtemizkan

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/utils/instrumentServer.ts

@@ -8,6 +8,7 @@ import {
   dynamicSamplingContextToSentryBaggageHeader,
   fill,
   isNodeEnv,
+  isPrimitive,
   loadModule,
   logger,
   objectify,
@@ -80,15 +81,16 @@ async function extractResponseError(response: Response): Promise<unknown> {
  * export const handleError = Sentry.wrapRemixHandleError
  */
 export function wrapRemixHandleError(err: unknown, { request }: DataFunctionArgs): void {
-  // We are skipping thrown responses here as they are either handled:
-  // - Remix v1: by captureRemixServerException at loader / action
-  // - Remix v2: by ErrorBoundary
+  // 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);
 }
 
@@ -162,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,
@@ -172,28 +174,42 @@ function makeWrappedDocumentRequestFunction(remixVersion: number) {
       context: EntryContext,
       loadContext?: Record<string, unknown>,
     ): Promise<Response> {
+      let res: Response;
       const activeTransaction = getActiveTransaction();
 
-      const span = activeTransaction?.startChild({
-        op: 'function.remix.document_request',
-        origin: 'auto.function.remix',
-        description: activeTransaction.name,
-        tags: {
-          method: request.method,
-          url: request.url,
-        },
-      });
-
-      const res = await origDocumentRequestFunction.call(
-        this,
-        request,
-        responseStatusCode,
-        responseHeaders,
-        context,
-        loadContext,
-      );
+      try {
+        const span = activeTransaction?.startChild({
+          op: 'function.remix.document_request',
+          origin: 'auto.function.remix',
+          description: activeTransaction.name,
+          tags: {
+            method: request.method,
+            url: request.url,
+          },
+        });
+
+        res = await origDocumentRequestFunction.call(
+          this,
+          request,
+          responseStatusCode,
+          responseHeaders,
+          context,
+          loadContext,
+        );
+
+        span?.finish();
+      } catch (err) {
+        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);
+        }
 
-      span?.finish();
+        throw err;
+      }
 
       return res;
     };
@@ -231,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);
       }
 

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/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/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);
+});

packages/remix/test/integration/test/server/action.test.ts

@@ -140,7 +140,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
     });
   });
 
-  it('handles a thrown 500 response', async () => {
+  it('handles an error-throwing redirection target', async () => {
     const env = await RemixTestEnv.init(adapter);
     const url = `${env.url}/action-json-response/-2`;
 
@@ -254,7 +254,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
             stacktrace: expect.any(Object),
             mechanism: {
               data: {
-                function: useV2 ? 'ErrorResponse' : 'action',
+                function: 'action',
               },
               handled: false,
               type: 'instrument',
@@ -303,13 +303,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
         values: [
           {
             type: 'Error',
-            value: useV2
-              ? 'Object captured as exception with keys: data, internal, status, statusText'
-              : 'Object captured as exception with keys: data',
+            value: 'Object captured as exception with keys: data',
             stacktrace: expect.any(Object),
             mechanism: {
               data: {
-                function: useV2 ? 'ErrorResponse' : 'action',
+                function: 'action',
               },
               handled: false,
               type: 'instrument',
@@ -362,7 +360,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
             stacktrace: expect.any(Object),
             mechanism: {
               data: {
-                function: useV2 ? 'ErrorResponse' : 'action',
+                function: 'action',
               },
               handled: false,
               type: 'instrument',
@@ -411,13 +409,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
         values: [
           {
             type: 'Error',
-            value: useV2
-              ? 'Object captured as exception with keys: data, internal, status, statusText'
-              : 'Object captured as exception with keys: [object has no keys]',
+            value: 'Object captured as exception with keys: [object has no keys]',
             stacktrace: expect.any(Object),
             mechanism: {
               data: {
-                function: useV2 ? 'ErrorResponse' : 'action',
+                function: 'action',
               },
               handled: false,
               type: 'instrument',
@@ -428,7 +424,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada
     });
   });
 
-  it('handles thrown string from an action', async () => {
+  it('handles thrown string (primitive) from an action', async () => {
     const env = await RemixTestEnv.init(adapter);
     const url = `${env.url}/server-side-unexpected-errors/-1`;
 

packages/remix/test/integration/test/server/loader.test.ts

@@ -78,7 +78,7 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
     });
   });
 
-  it('handles a thrown 500 response', async () => {
+  it('handles an error-throwing redirection target', async () => {
     const env = await RemixTestEnv.init(adapter);
     const url = `${env.url}/loader-json-response/-1`;
 

packages/remix/test/integration/test/server/ssr.test.ts

@@ -0,0 +1,46 @@
+import { RemixTestEnv, assertSentryEvent, assertSentryTransaction } from './utils/helpers';
+
+const useV2 = process.env.REMIX_VERSION === '2';
+
+describe('Server Side Rendering', () => {
+  it('correctly reports a server side rendering error', async () => {
+    const env = await RemixTestEnv.init('builtin');
+    const url = `${env.url}/ssr-error`;
+    const envelopes = await env.getMultipleEnvelopeRequest({ url, count: 2, envelopeType: ['transaction', 'event'] });
+    const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction');
+    const [event] = envelopes.filter(envelope => envelope[1].type === 'event');
+
+    assertSentryTransaction(transaction[2], {
+      contexts: {
+        trace: {
+          status: 'internal_error',
+          tags: {
+            'http.status_code': '500',
+          },
+          data: {
+            'http.response.status_code': 500,
+          },
+        },
+      },
+    });
+
+    assertSentryEvent(event[2], {
+      exception: {
+        values: [
+          {
+            type: 'Error',
+            value: 'Sentry SSR Test Error',
+            stacktrace: expect.any(Object),
+            mechanism: {
+              data: {
+                function: useV2 ? 'remix.server.handleError' : 'documentRequest',
+              },
+              handled: false,
+              type: 'instrument',
+            },
+          },
+        ],
+      },
+    });
+  });
+});