From b563a261bb97df003d64fe382346ef7ac409c291 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 30 Jul 2025 19:33:31 +0200 Subject: [PATCH 1/3] add createSentryHandleError --- .../app/entry.server.tsx | 10 +- .../app/entry.server.tsx | 10 +- .../app/entry.server.tsx | 10 +- .../src/server/createSentryHandleError.ts | 34 +++ packages/react-router/src/server/index.ts | 1 + .../server/createSentryHandleError.test.ts | 200 ++++++++++++++++++ 6 files changed, 238 insertions(+), 27 deletions(-) create mode 100644 packages/react-router/src/server/createSentryHandleError.ts create mode 100644 packages/react-router/test/server/createSentryHandleError.test.ts diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/app/entry.server.tsx index 97260755da21..738cd1515a4d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/app/entry.server.tsx @@ -15,12 +15,4 @@ const handleRequest = Sentry.createSentryHandleRequest({ export default handleRequest; -export const handleError: HandleErrorFunction = (error, { request }) => { - // React Router may abort some interrupted requests, don't log those - if (!request.signal.aborted) { - Sentry.captureException(error); - - // make sure to still log the error so you can see it - console.error(error); - } -}; +export const handleError: HandleErrorFunction = Sentry.createSentryHandleError({ logErrors: true }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/app/entry.server.tsx index 97260755da21..738cd1515a4d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/app/entry.server.tsx @@ -15,12 +15,4 @@ const handleRequest = Sentry.createSentryHandleRequest({ export default handleRequest; -export const handleError: HandleErrorFunction = (error, { request }) => { - // React Router may abort some interrupted requests, don't log those - if (!request.signal.aborted) { - Sentry.captureException(error); - - // make sure to still log the error so you can see it - console.error(error); - } -}; +export const handleError: HandleErrorFunction = Sentry.createSentryHandleError({ logErrors: true }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework/app/entry.server.tsx b/dev-packages/e2e-tests/test-applications/react-router-7-framework/app/entry.server.tsx index 97260755da21..738cd1515a4d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework/app/entry.server.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework/app/entry.server.tsx @@ -15,12 +15,4 @@ const handleRequest = Sentry.createSentryHandleRequest({ export default handleRequest; -export const handleError: HandleErrorFunction = (error, { request }) => { - // React Router may abort some interrupted requests, don't log those - if (!request.signal.aborted) { - Sentry.captureException(error); - - // make sure to still log the error so you can see it - console.error(error); - } -}; +export const handleError: HandleErrorFunction = Sentry.createSentryHandleError({ logErrors: true }); diff --git a/packages/react-router/src/server/createSentryHandleError.ts b/packages/react-router/src/server/createSentryHandleError.ts new file mode 100644 index 000000000000..3fbd2b7e4d03 --- /dev/null +++ b/packages/react-router/src/server/createSentryHandleError.ts @@ -0,0 +1,34 @@ +import { captureException, flushIfServerless } from '@sentry/core'; +import type { ActionFunctionArgs, HandleErrorFunction, LoaderFunctionArgs } from 'react-router'; + +export type SentryHandleErrorOptions = { + logErrors?: boolean; +}; + +/** + * A complete Sentry-instrumented handleError implementation that handles error reporting + * + * @returns A Sentry-instrumented handleError function + */ +export function createSentryHandleError({ logErrors = false }: SentryHandleErrorOptions): HandleErrorFunction { + const handleError = async function handleError( + error: unknown, + args: LoaderFunctionArgs | ActionFunctionArgs, + ): Promise { + // React Router may abort some interrupted requests, don't report those + if (!args.request.signal.aborted) { + captureException(error); + if (logErrors) { + // eslint-disable-next-line no-console + console.error(error); + } + try { + await flushIfServerless(); + } catch { + // Ignore flush errors to ensure error handling completes gracefully + } + } + }; + + return handleError; +} diff --git a/packages/react-router/src/server/index.ts b/packages/react-router/src/server/index.ts index b42b769a78e6..91520059d3fb 100644 --- a/packages/react-router/src/server/index.ts +++ b/packages/react-router/src/server/index.ts @@ -6,3 +6,4 @@ export { wrapSentryHandleRequest, sentryHandleRequest, getMetaTagTransformer } f export { createSentryHandleRequest, type SentryHandleRequestOptions } from './createSentryHandleRequest'; export { wrapServerAction } from './wrapServerAction'; export { wrapServerLoader } from './wrapServerLoader'; +export { createSentryHandleError, type SentryHandleErrorOptions } from './createSentryHandleError'; diff --git a/packages/react-router/test/server/createSentryHandleError.test.ts b/packages/react-router/test/server/createSentryHandleError.test.ts new file mode 100644 index 000000000000..eeffd12ff2c2 --- /dev/null +++ b/packages/react-router/test/server/createSentryHandleError.test.ts @@ -0,0 +1,200 @@ +import * as core from '@sentry/core'; +import type { ActionFunctionArgs, LoaderFunctionArgs } from 'react-router'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createSentryHandleError } from '../../src/server/createSentryHandleError'; + +vi.mock('@sentry/core', () => ({ + captureException: vi.fn(), + flushIfServerless: vi.fn().mockResolvedValue(undefined), +})); + +describe('createSentryHandleError', () => { + const mockCaptureException = vi.mocked(core.captureException); + const mockFlushIfServerless = vi.mocked(core.flushIfServerless); + const mockConsoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + + const mockError = new Error('Test error'); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + mockConsoleError.mockClear(); + }); + + // Helper function to create mock args with proper Request structure + const createMockArgs = (aborted: boolean): LoaderFunctionArgs => { + const controller = new AbortController(); + if (aborted) { + controller.abort(); + } + + const request = new Request('http://test.com', { + signal: controller.signal, + }); + + return { request } as LoaderFunctionArgs; + }; + + describe('with default options', () => { + it('should create a handle error function with logErrors disabled by default', async () => { + const handleError = createSentryHandleError({}); + + expect(typeof handleError).toBe('function'); + }); + + it('should capture exception and flush when request is not aborted', async () => { + const handleError = createSentryHandleError({}); + const mockArgs = createMockArgs(false); + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + expect(mockConsoleError).not.toHaveBeenCalled(); + }); + + it('should not capture exception when request is aborted', async () => { + const handleError = createSentryHandleError({}); + const mockArgs = createMockArgs(true); + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockFlushIfServerless).not.toHaveBeenCalled(); + expect(mockConsoleError).not.toHaveBeenCalled(); + }); + }); + + describe('with logErrors enabled', () => { + it('should log errors to console when logErrors is true', async () => { + const handleError = createSentryHandleError({ logErrors: true }); + const mockArgs = createMockArgs(false); + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + expect(mockConsoleError).toHaveBeenCalledWith(mockError); + }); + + it('should not log errors to console when request is aborted even with logErrors enabled', async () => { + const handleError = createSentryHandleError({ logErrors: true }); + const mockArgs = createMockArgs(true); + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).not.toHaveBeenCalled(); + expect(mockFlushIfServerless).not.toHaveBeenCalled(); + expect(mockConsoleError).not.toHaveBeenCalled(); + }); + }); + + describe('with logErrors disabled explicitly', () => { + it('should not log errors to console when logErrors is false', async () => { + const handleError = createSentryHandleError({ logErrors: false }); + const mockArgs = createMockArgs(false); + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + expect(mockConsoleError).not.toHaveBeenCalled(); + }); + }); + + describe('with different error types', () => { + it('should handle string errors', async () => { + const handleError = createSentryHandleError({}); + const stringError = 'String error message'; + const mockArgs = createMockArgs(false); + + await handleError(stringError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(stringError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + }); + + it('should handle null/undefined errors', async () => { + const handleError = createSentryHandleError({}); + const mockArgs = createMockArgs(false); + + await handleError(null, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(null); + expect(mockFlushIfServerless).toHaveBeenCalled(); + }); + + it('should handle custom error objects', async () => { + const handleError = createSentryHandleError({}); + const customError = { message: 'Custom error', code: 500 }; + const mockArgs = createMockArgs(false); + + await handleError(customError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(customError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + }); + }); + + describe('with ActionFunctionArgs', () => { + it('should work with ActionFunctionArgs instead of LoaderFunctionArgs', async () => { + const handleError = createSentryHandleError({ logErrors: true }); + const mockArgs = createMockArgs(false) as ActionFunctionArgs; + + await handleError(mockError, mockArgs); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + expect(mockConsoleError).toHaveBeenCalledWith(mockError); + }); + }); + + describe('flushIfServerless behavior', () => { + it('should wait for flushIfServerless to complete', async () => { + const handleError = createSentryHandleError({}); + + // Create a promise that resolves after 10ms + let resolveFlush: () => void; + const flushPromise = new Promise(resolve => { + resolveFlush = resolve; + }); + + // Mock flushIfServerless to return our controlled promise + mockFlushIfServerless.mockReturnValueOnce(flushPromise); + + const mockArgs = createMockArgs(false); + + const startTime = Date.now(); + + // Start the handleError call + const handleErrorPromise = handleError(mockError, mockArgs); + + // Resolve the flush after 10ms + setTimeout(() => resolveFlush(), 10); + + await handleErrorPromise; + const endTime = Date.now(); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + expect(endTime - startTime).toBeGreaterThanOrEqual(10); + }); + + it('should handle flushIfServerless rejection gracefully', async () => { + const handleError = createSentryHandleError({}); + + // Make flushIfServerless reject + mockFlushIfServerless.mockRejectedValueOnce(new Error('Flush failed')); + + const mockArgs = createMockArgs(false); + + // This should not throw + await expect(handleError(mockError, mockArgs)).resolves.toBeUndefined(); + + expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockFlushIfServerless).toHaveBeenCalled(); + }); + }); +}); From fee1d189a635f3840b8af9e8a299dfa5a8011923 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 31 Jul 2025 10:22:58 +0200 Subject: [PATCH 2/3] add mechanism --- .../src/server/createSentryHandleError.ts | 7 +++++- .../server/createSentryHandleError.test.ts | 23 +++++++++++-------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/packages/react-router/src/server/createSentryHandleError.ts b/packages/react-router/src/server/createSentryHandleError.ts index 3fbd2b7e4d03..65114d035655 100644 --- a/packages/react-router/src/server/createSentryHandleError.ts +++ b/packages/react-router/src/server/createSentryHandleError.ts @@ -17,7 +17,12 @@ export function createSentryHandleError({ logErrors = false }: SentryHandleError ): Promise { // React Router may abort some interrupted requests, don't report those if (!args.request.signal.aborted) { - captureException(error); + captureException(error, { + mechanism: { + type: 'react-router', + handled: false, + }, + }); if (logErrors) { // eslint-disable-next-line no-console console.error(error); diff --git a/packages/react-router/test/server/createSentryHandleError.test.ts b/packages/react-router/test/server/createSentryHandleError.test.ts index eeffd12ff2c2..41dcee5311d2 100644 --- a/packages/react-router/test/server/createSentryHandleError.test.ts +++ b/packages/react-router/test/server/createSentryHandleError.test.ts @@ -8,6 +8,11 @@ vi.mock('@sentry/core', () => ({ flushIfServerless: vi.fn().mockResolvedValue(undefined), })); +const mechanism = { + handled: false, + type: 'react-router', +}; + describe('createSentryHandleError', () => { const mockCaptureException = vi.mocked(core.captureException); const mockFlushIfServerless = vi.mocked(core.flushIfServerless); @@ -50,7 +55,7 @@ describe('createSentryHandleError', () => { await handleError(mockError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); expect(mockConsoleError).not.toHaveBeenCalled(); }); @@ -74,7 +79,7 @@ describe('createSentryHandleError', () => { await handleError(mockError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); expect(mockConsoleError).toHaveBeenCalledWith(mockError); }); @@ -98,7 +103,7 @@ describe('createSentryHandleError', () => { await handleError(mockError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); expect(mockConsoleError).not.toHaveBeenCalled(); }); @@ -112,7 +117,7 @@ describe('createSentryHandleError', () => { await handleError(stringError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(stringError); + expect(mockCaptureException).toHaveBeenCalledWith(stringError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); }); @@ -122,7 +127,7 @@ describe('createSentryHandleError', () => { await handleError(null, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(null); + expect(mockCaptureException).toHaveBeenCalledWith(null, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); }); @@ -133,7 +138,7 @@ describe('createSentryHandleError', () => { await handleError(customError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(customError); + expect(mockCaptureException).toHaveBeenCalledWith(customError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); }); }); @@ -145,7 +150,7 @@ describe('createSentryHandleError', () => { await handleError(mockError, mockArgs); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); expect(mockConsoleError).toHaveBeenCalledWith(mockError); }); @@ -177,7 +182,7 @@ describe('createSentryHandleError', () => { await handleErrorPromise; const endTime = Date.now(); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); expect(endTime - startTime).toBeGreaterThanOrEqual(10); }); @@ -193,7 +198,7 @@ describe('createSentryHandleError', () => { // This should not throw await expect(handleError(mockError, mockArgs)).resolves.toBeUndefined(); - expect(mockCaptureException).toHaveBeenCalledWith(mockError); + expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism }); expect(mockFlushIfServerless).toHaveBeenCalled(); }); }); From a00226af3f28228f716582dc540de645d9d21800 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 31 Jul 2025 10:46:17 +0200 Subject: [PATCH 3/3] fix tests hopefully --- .../tests/errors/errors.server.test.ts | 6 ++++-- .../tests/errors/errors.server.test.ts | 6 ++++-- .../tests/errors/errors.server.test.ts | 6 ++++-- .../test/server/createSentryHandleError.test.ts | 11 ++--------- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/errors/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/errors/errors.server.test.ts index d702f8cee597..2759bfecb67e 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/errors/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/errors/errors.server.test.ts @@ -20,7 +20,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], @@ -67,7 +68,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/errors/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/errors/errors.server.test.ts index d702f8cee597..2759bfecb67e 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/errors/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/errors/errors.server.test.ts @@ -20,7 +20,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], @@ -67,7 +68,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/errors/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/errors/errors.server.test.ts index d702f8cee597..2759bfecb67e 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/errors/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/errors/errors.server.test.ts @@ -20,7 +20,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], @@ -67,7 +68,8 @@ test.describe('server-side errors', () => { type: 'Error', value: errorMessage, mechanism: { - handled: true, + handled: false, + type: 'react-router', }, }, ], diff --git a/packages/react-router/test/server/createSentryHandleError.test.ts b/packages/react-router/test/server/createSentryHandleError.test.ts index 41dcee5311d2..0cad5d2a04ba 100644 --- a/packages/react-router/test/server/createSentryHandleError.test.ts +++ b/packages/react-router/test/server/createSentryHandleError.test.ts @@ -28,16 +28,15 @@ describe('createSentryHandleError', () => { mockConsoleError.mockClear(); }); - // Helper function to create mock args with proper Request structure const createMockArgs = (aborted: boolean): LoaderFunctionArgs => { const controller = new AbortController(); if (aborted) { controller.abort(); } - const request = new Request('http://test.com', { + const request = { signal: controller.signal, - }); + } as Request; return { request } as LoaderFunctionArgs; }; @@ -160,23 +159,19 @@ describe('createSentryHandleError', () => { it('should wait for flushIfServerless to complete', async () => { const handleError = createSentryHandleError({}); - // Create a promise that resolves after 10ms let resolveFlush: () => void; const flushPromise = new Promise(resolve => { resolveFlush = resolve; }); - // Mock flushIfServerless to return our controlled promise mockFlushIfServerless.mockReturnValueOnce(flushPromise); const mockArgs = createMockArgs(false); const startTime = Date.now(); - // Start the handleError call const handleErrorPromise = handleError(mockError, mockArgs); - // Resolve the flush after 10ms setTimeout(() => resolveFlush(), 10); await handleErrorPromise; @@ -190,12 +185,10 @@ describe('createSentryHandleError', () => { it('should handle flushIfServerless rejection gracefully', async () => { const handleError = createSentryHandleError({}); - // Make flushIfServerless reject mockFlushIfServerless.mockRejectedValueOnce(new Error('Flush failed')); const mockArgs = createMockArgs(false); - // This should not throw await expect(handleError(mockError, mockArgs)).resolves.toBeUndefined(); expect(mockCaptureException).toHaveBeenCalledWith(mockError, { mechanism });