From 5ad3a52c5fae05dcd6f349804b5f1a93b415b33b Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Wed, 30 Sep 2026 11:14:06 +0200 Subject: [PATCH 1/2] feat(remix): Complete Remix 3 server error handling Covers the error paths the router middleware alone does not see, and stops reporting things that are not faults. `createRequestListener` in `@remix-run/node-fetch-server` is patched to install an `onError` that chains to the app's own. This is the only hook that covers an app whose fetch handler is not a router. It needs no abort guard: the listener checks `isRequestAbortError` itself and returns before calling `onError`. The middleware captures too, so an event carries the route and request already on the scope. That path does need the abort guard, because `raceRequestAbort` rejects with `signal.reason` when the client disconnects. Without it every user navigating away mid request creates an issue. Removing the guard makes the new e2e test fail. `shouldHandleError` follows `@sentry/hono`: skip 3xx and 4xx errors carrying a numeric `status`, capture the rest. Co-Authored-By: Claude Fable 5.1 --- .../remix-v3/app/actions/controller.tsx | 15 +++ .../test-applications/remix-v3/app/routes.ts | 3 + .../test-applications/remix-v3/server.ts | 11 ++- .../remix-v3/tests/server-errors.test.ts | 45 +++++++++ packages/remix/src/v3/index.server.ts | 2 + packages/remix/src/v3/server/errorFilter.ts | 60 ++++++++++++ packages/remix/src/v3/server/instrument.ts | 69 ++++++++++++-- packages/remix/src/v3/server/integration.ts | 16 +++- packages/remix/src/v3/server/middleware.ts | 12 ++- packages/remix/src/v3/types.ts | 5 + packages/remix/test/v3/errorFilter.test.ts | 91 +++++++++++++++++++ packages/remix/test/v3/instrument.test.ts | 52 ++++++++++- .../src/orchestrion/config/remix-v3.ts | 11 +++ 13 files changed, 372 insertions(+), 20 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/remix-v3/tests/server-errors.test.ts create mode 100644 packages/remix/src/v3/server/errorFilter.ts create mode 100644 packages/remix/test/v3/errorFilter.test.ts diff --git a/dev-packages/e2e-tests/test-applications/remix-v3/app/actions/controller.tsx b/dev-packages/e2e-tests/test-applications/remix-v3/app/actions/controller.tsx index 8b6dd2cf9214..9d52a907bcbe 100644 --- a/dev-packages/e2e-tests/test-applications/remix-v3/app/actions/controller.tsx +++ b/dev-packages/e2e-tests/test-applications/remix-v3/app/actions/controller.tsx @@ -46,6 +46,8 @@ function UserPage(handle: Handle<{ id?: string }>) { ); } +let slowStarted = false; + export default createController(routes, { actions: { async assets(context) { @@ -60,5 +62,18 @@ export default createController(routes, { teapot() { return new Response("I'm a teapot", { status: 418 }); }, + boom() { + throw new Error('Route handler failed'); + }, + // Long enough for a test to disconnect mid request. `/slow-started` tells the test when the handler + // is running, so the disconnect lands inside it rather than before it. + async slow() { + slowStarted = true; + await new Promise(resolve => setTimeout(resolve, 3000)); + return new Response('slow'); + }, + slowStarted() { + return new Response(slowStarted ? '1' : '0'); + }, }, }); diff --git a/dev-packages/e2e-tests/test-applications/remix-v3/app/routes.ts b/dev-packages/e2e-tests/test-applications/remix-v3/app/routes.ts index 77b32281374b..37c442cc6c82 100644 --- a/dev-packages/e2e-tests/test-applications/remix-v3/app/routes.ts +++ b/dev-packages/e2e-tests/test-applications/remix-v3/app/routes.ts @@ -5,4 +5,7 @@ export const routes = route({ home: '/', user: get('/users/:id'), teapot: get('/teapot'), + boom: get('/boom'), + slow: get('/slow'), + slowStarted: get('/slow-started'), }); diff --git a/dev-packages/e2e-tests/test-applications/remix-v3/server.ts b/dev-packages/e2e-tests/test-applications/remix-v3/server.ts index 56408af2bbe9..c9ac4f00d087 100644 --- a/dev-packages/e2e-tests/test-applications/remix-v3/server.ts +++ b/dev-packages/e2e-tests/test-applications/remix-v3/server.ts @@ -13,12 +13,13 @@ Sentry.init({ const port = process.env.PORT ? Number.parseInt(process.env.PORT, 10) : 3060; const server = http.createServer( - createRequestListener(async request => { - try { - return await router.fetch(request); - } catch { - return new Response('Internal Server Error', { status: 500 }); + createRequestListener(request => { + // Handled outside the router on purpose: the only hook that can see this is the listener's + // `onError`, which is the case the router middleware cannot cover. + if (new URL(request.url).pathname === '/plain-throw') { + throw new Error('Plain handler failed'); } + return router.fetch(request); }), ); diff --git a/dev-packages/e2e-tests/test-applications/remix-v3/tests/server-errors.test.ts b/dev-packages/e2e-tests/test-applications/remix-v3/tests/server-errors.test.ts new file mode 100644 index 000000000000..01e9c2c5810f --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/remix-v3/tests/server-errors.test.ts @@ -0,0 +1,45 @@ +import { expect, test } from '@playwright/test'; +import { waitForError } from '@sentry-internal/test-utils'; + +const APP_NAME = 'remix-v3'; + +test('reports an error thrown in a route handler, with its route', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed'); + + await fetch(`${baseURL}/boom`); + + const event = await errorPromise; + expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.middleware' }); + expect(event.transaction).toBe('GET /boom'); +}); + +test('reports an error from a fetch handler that is not the router', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Plain handler failed'); + + const response = await fetch(`${baseURL}/plain-throw`); + + expect(response.status).toBe(500); + const event = await errorPromise; + expect(event.exception?.values?.[0]?.mechanism).toEqual({ handled: false, type: 'auto.http.remix_v3.on_error' }); +}); + +test('does not report a request the client aborted', async ({ baseURL }) => { + const seen: string[] = []; + void waitForError(APP_NAME, event => { + seen.push(event.exception?.values?.[0]?.value ?? ''); + return false; + }); + + const controller = new AbortController(); + const slow = fetch(`${baseURL}/slow`, { signal: controller.signal }).catch(() => undefined); + await expect.poll(async () => (await fetch(`${baseURL}/slow-started`)).text()).toBe('1'); + controller.abort(); + await slow; + + // A later real error proves the pipeline is live, so an abort error would have arrived before it. + const sentinel = waitForError(APP_NAME, event => event.exception?.values?.[0]?.value === 'Route handler failed'); + await fetch(`${baseURL}/boom`); + await sentinel; + + expect(seen.filter(value => value !== 'Route handler failed')).toEqual([]); +}); diff --git a/packages/remix/src/v3/index.server.ts b/packages/remix/src/v3/index.server.ts index 4765039207d4..422e5b31e7f6 100644 --- a/packages/remix/src/v3/index.server.ts +++ b/packages/remix/src/v3/index.server.ts @@ -5,3 +5,5 @@ export { remixV3Integration } from './server/integration'; export { sentryRemixMiddleware } from './server/middleware'; export { instrumentRemixV3 } from './server/instrument'; export { instrumentAssetServer } from './assetServer'; +export { defaultShouldHandleError } from './server/errorFilter'; +export type { ShouldHandleError } from './server/errorFilter'; diff --git a/packages/remix/src/v3/server/errorFilter.ts b/packages/remix/src/v3/server/errorFilter.ts new file mode 100644 index 000000000000..9e98a2874a27 --- /dev/null +++ b/packages/remix/src/v3/server/errorFilter.ts @@ -0,0 +1,60 @@ +import { captureException } from '@sentry/core'; + +/** Decides whether a thrown value should become a Sentry issue. */ +export type ShouldHandleError = (error: unknown) => boolean; + +/** + * Skip 3xx and 4xx errors, capture everything else. A numeric `status` in that range is an expected + * outcome, not a fault, and the request still produces a span. + */ +export function defaultShouldHandleError(error: unknown): boolean { + if (typeof error !== 'object' || error === null) { + return true; + } + + const status = (error as { status?: unknown }).status; + + return !(typeof status === 'number' && status >= 300 && status < 500); +} + +// Configured at `Sentry.init()`, but read by subscribers installed earlier by the `--import` entry, so +// it lives in module scope rather than being captured before it exists. +let shouldHandleError: ShouldHandleError = defaultShouldHandleError; + +/** @internal Set by the integration during `Sentry.init()`. */ +export function setShouldHandleError(filter: ShouldHandleError | undefined): void { + shouldHandleError = filter ?? defaultShouldHandleError; +} + +/** + * Report an error unless it is an aborted request or filtered out. + * + * A router failure can arrive twice, from the middleware and again from `onError`. `captureException` + * marks the object `__sentry_captured__` and drops the second report; only a thrown primitive cannot + * be marked and may be reported twice. + */ +export function captureRequestError(error: unknown, request: Request | undefined, mechanism: string): boolean { + // The app's filter can throw. Letting that out would replace the request's own error and skip the + // app's `onError`, so reporting is best effort. + try { + if (request && isRequestAbort(error, request)) { + return false; + } + if (!shouldHandleError(error)) { + return false; + } + captureException(error, { mechanism: { handled: false, type: mechanism } }); + return true; + } catch { + return false; + } +} + +/** + * Whether a rejection is the client giving up rather than the app failing. The router rejects with + * `signal.reason` when the connection drops; without this check every user navigating away mid + * request creates an issue. + */ +export function isRequestAbort(error: unknown, request: Request): boolean { + return request.signal.aborted && error === request.signal.reason; +} diff --git a/packages/remix/src/v3/server/instrument.ts b/packages/remix/src/v3/server/instrument.ts index 8fc254055588..75e5a848a683 100644 --- a/packages/remix/src/v3/server/instrument.ts +++ b/packages/remix/src/v3/server/instrument.ts @@ -2,7 +2,8 @@ import * as diagnosticsChannel from 'node:diagnostics_channel'; import { createMultiMatcher } from 'remix/route-pattern/match'; import { remixV3Channels } from '@sentry/server-utils/orchestrion/config'; -import type { MatcherLike, RouterOptionsLike } from '../types'; +import type { MatcherLike, RequestListenerOptionsLike, RouterOptionsLike } from '../types'; +import { captureRequestError } from './errorFilter'; import { sentryRemixMiddleware } from './middleware'; const NOOP = (): void => {}; @@ -34,6 +35,11 @@ export function instrumentRemixV3(): void { } subscribed = true; + subscribeToCreateRouter(); + subscribeToCreateRequestListener(); +} + +function subscribeToCreateRouter(): void { diagnosticsChannel.tracingChannel(remixV3Channels.REMIX_V3_CREATE_ROUTER).subscribe({ start(data) { // Node rethrows anything this handler throws as an uncaught exception, which would kill an app @@ -52,32 +58,75 @@ export function instrumentRemixV3(): void { }); } +/** + * Report errors from any fetch handler, router or not. The middleware only sees failures inside a + * router; this is the only hook that covers a plain handler. No abort guard is needed, because the + * listener drops aborted requests before calling `onError`. + */ +function subscribeToCreateRequestListener(): void { + diagnosticsChannel + .tracingChannel(remixV3Channels.REMIX_V3_CREATE_REQUEST_LISTENER) + .subscribe({ + start(data) { + try { + injectOnError(ensureOptions(data.arguments, 1)); + } catch { + // Ignored on purpose. + } + }, + end: NOOP, + asyncStart: NOOP, + asyncEnd: NOOP, + error: NOOP, + }); +} + +function injectOnError(raw: Record | undefined): void { + if (!raw || markInjected(raw)) { + return; + } + + const options = raw as RequestListenerOptionsLike; + const appOnError = options.onError; + + options.onError = error => { + captureRequestError(error, undefined, 'auto.http.remix_v3.on_error'); + + // Chained so the app keeps its own response. + return appOnError?.(error); + }; +} + /** * The options object, created when the caller omitted it. `undefined` when the caller passed something * that is not an options object, which must not be overwritten. */ -function ensureOptions(args: unknown[]): Record | undefined { - const existing = args[0]; +function ensureOptions(args: unknown[], index = 0): Record | undefined { + const existing = args[index]; if (existing === undefined || existing === null) { const created: Record = {}; - args[0] = created; + args[index] = created; return created; } return typeof existing === 'object' ? (existing as Record) : undefined; } -function injectRouterMiddleware(raw: Record | undefined): void { - if (!raw) { - return; - } - +/** Whether this object was already injected into, marking it when it was not. */ +function markInjected(raw: Record): boolean { const marker = raw as { [INJECTED]?: boolean }; if (marker[INJECTED]) { - return; + return true; } marker[INJECTED] = true; + return false; +} + +function injectRouterMiddleware(raw: Record | undefined): void { + if (!raw || markInjected(raw)) { + return; + } const options = raw as RouterOptionsLike; diff --git a/packages/remix/src/v3/server/integration.ts b/packages/remix/src/v3/server/integration.ts index 84af41b67156..3ceabf6acbd4 100644 --- a/packages/remix/src/v3/server/integration.ts +++ b/packages/remix/src/v3/server/integration.ts @@ -1,13 +1,24 @@ import { defineIntegration, type IntegrationFn } from '@sentry/core'; +import { setShouldHandleError, type ShouldHandleError } from './errorFilter'; import { instrumentRemixV3 } from './instrument'; const INTEGRATION_NAME = 'RemixV3' as const; -const _remixV3Integration = (() => { +interface RemixV3IntegrationOptions { + /** + * Decides which thrown values become issues. Return `false` to drop one. The default skips a numeric + * `status` from 300 to 499. Aborted requests never reach this. + */ + shouldHandleError?: ShouldHandleError; +} + +const _remixV3Integration = ((options: RemixV3IntegrationOptions = {}) => { return { name: INTEGRATION_NAME, setupOnce() { + setShouldHandleError(options.shouldHandleError); + // Usually a no-op: `createRouter()` runs while the app's modules are imported, before any // `init()`, so `--import @sentry/remix/v3/node` has already subscribed. This covers setups that // register the module hook from `init()` instead. @@ -17,6 +28,7 @@ const _remixV3Integration = (() => { }) satisfies IntegrationFn; /** - * Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route. + * Names the `http.server` spans `@sentry/node` opens after the matched Remix 3 route, and reports the + * errors the spans alone would not show. */ export const remixV3Integration = defineIntegration(_remixV3Integration); diff --git a/packages/remix/src/v3/server/middleware.ts b/packages/remix/src/v3/server/middleware.ts index 0409d48f992e..541adc696214 100644 --- a/packages/remix/src/v3/server/middleware.ts +++ b/packages/remix/src/v3/server/middleware.ts @@ -11,6 +11,7 @@ import { } from '@sentry/core'; import type { MatcherLike, MiddlewareLike, NextFunctionLike, RequestContextLike } from '../types'; +import { captureRequestError } from './errorFilter'; import { resolveRoutePattern } from './route'; /** @@ -28,7 +29,16 @@ export function sentryRemixMiddleware(matcher: MatcherLike): MiddlewareLike { // Applied before `next()` so anything captured while the handler runs already carries the route. applyRoute(isolationScope, matcher, context); - const response = await next(); + let response; + try { + response = await next(); + } catch (error) { + // Captured here as well as in `onError`, so the event carries the route this middleware just put + // on the scope. + captureRequestError(error, context.request, 'auto.http.remix_v3.middleware'); + throw error; + } + setResponseStatus(response); return response; diff --git a/packages/remix/src/v3/types.ts b/packages/remix/src/v3/types.ts index 87dff35d797c..a5bcbe96e017 100644 --- a/packages/remix/src/v3/types.ts +++ b/packages/remix/src/v3/types.ts @@ -37,3 +37,8 @@ export interface RouterOptionsLike { middleware?: MiddlewareLike[]; matcher?: MatcherLike; } + +/** `createRequestListener`'s options, of which only the error hook is touched. */ +export interface RequestListenerOptionsLike { + onError?: (error: unknown) => void | Response | Promise; +} diff --git a/packages/remix/test/v3/errorFilter.test.ts b/packages/remix/test/v3/errorFilter.test.ts new file mode 100644 index 000000000000..f307f8bdd4e4 --- /dev/null +++ b/packages/remix/test/v3/errorFilter.test.ts @@ -0,0 +1,91 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const captureException = vi.fn(); +vi.mock('@sentry/core', () => ({ captureException: (...args: unknown[]) => captureException(...args) })); + +const { captureRequestError, defaultShouldHandleError, isRequestAbort, setShouldHandleError } = + await import('../../src/v3/server/errorFilter'); + +/** A request whose connection has dropped, rejecting with `signal.reason` as the router does. */ +function abortedRequest(reason: unknown): Request { + const controller = new AbortController(); + const request = new Request('http://x/', { signal: controller.signal }); + controller.abort(reason); + return request; +} + +describe('defaultShouldHandleError', () => { + it.each([300, 302, 404, 499])('skips status %i', status => { + expect(defaultShouldHandleError({ status })).toBe(false); + }); + + it.each([200, 500, 503])('captures status %i', status => { + expect(defaultShouldHandleError({ status })).toBe(true); + }); + + it.each([ + ['a plain error', new Error('boom')], + ['a thrown string', 'boom'], + ['null', null], + ['a non-numeric status', { status: '404' }], + ])('captures %s', (_what, error) => { + expect(defaultShouldHandleError(error)).toBe(true); + }); +}); + +describe('isRequestAbort', () => { + it('matches the reason the request was aborted with', () => { + const reason = new Error('aborted'); + expect(isRequestAbort(reason, abortedRequest(reason))).toBe(true); + }); + + it('does not match a different error on an aborted request', () => { + expect(isRequestAbort(new Error('boom'), abortedRequest(new Error('aborted')))).toBe(false); + }); + + it('does not match on a live request', () => { + expect(isRequestAbort(new Error('boom'), new Request('http://x/'))).toBe(false); + }); +}); + +describe('captureRequestError', () => { + beforeEach(() => { + captureException.mockClear(); + setShouldHandleError(undefined); + }); + + it('captures with the mechanism it was given', () => { + const error = new Error('boom'); + + expect(captureRequestError(error, new Request('http://x/'), 'auto.test')).toBe(true); + expect(captureException).toHaveBeenCalledWith(error, { mechanism: { handled: false, type: 'auto.test' } }); + }); + + it('drops an aborted request', () => { + const reason = new Error('aborted'); + + expect(captureRequestError(reason, abortedRequest(reason), 'auto.test')).toBe(false); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('drops what the filter rejects', () => { + setShouldHandleError(() => false); + + expect(captureRequestError(new Error('boom'), new Request('http://x/'), 'auto.test')).toBe(false); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('swallows a throwing filter so the request keeps its own error', () => { + setShouldHandleError(() => { + throw new Error('filter broke'); + }); + + expect(() => captureRequestError(new Error('boom'), new Request('http://x/'), 'auto.test')).not.toThrow(); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('still captures without a request, as the onError hook has none', () => { + expect(captureRequestError(new Error('boom'), undefined, 'auto.test')).toBe(true); + expect(captureException).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/remix/test/v3/instrument.test.ts b/packages/remix/test/v3/instrument.test.ts index 7eddc88c84d3..e74e7b68f224 100644 --- a/packages/remix/test/v3/instrument.test.ts +++ b/packages/remix/test/v3/instrument.test.ts @@ -1,16 +1,30 @@ import { channel } from 'node:diagnostics_channel'; +import type * as SentryCore from '@sentry/core'; import { remixV3Channels } from '@sentry/server-utils/orchestrion/config'; -import { beforeAll, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; -import { instrumentRemixV3 } from '../../src/v3/server/instrument'; +const captureException = vi.fn(); + +// Only `captureException` is replaced; the middleware needs the rest for real. +vi.mock('@sentry/core', async importOriginal => ({ + ...(await importOriginal()), + captureException: (...args: unknown[]) => captureException(...args), +})); + +const { instrumentRemixV3 } = await import('../../src/v3/server/instrument'); const startChannel = channel(`tracing:${remixV3Channels.REMIX_V3_CREATE_ROUTER}:start`); +const listenerStartChannel = channel(`tracing:${remixV3Channels.REMIX_V3_CREATE_REQUEST_LISTENER}:start`); /** What orchestrion's transform publishes: the call's arguments, collected into a real array. */ function publishCreateRouter(args: unknown[]): void { startChannel.publish({ arguments: args }); } +function publishCreateRequestListener(args: unknown[]): void { + listenerStartChannel.publish({ arguments: args }); +} + describe('instrumentRemixV3', () => { beforeAll(() => { // Called twice on purpose: the `--import` entry and `setupOnce()` both call it, and a second set @@ -68,3 +82,37 @@ describe('instrumentRemixV3', () => { expect(options.middleware).toEqual([]); }); }); + +describe('the createRequestListener error hook', () => { + beforeAll(() => { + instrumentRemixV3(); + }); + + beforeEach(() => { + captureException.mockClear(); + }); + + it("captures and then calls the app's own onError", async () => { + const appResponse = new Response('handled', { status: 500 }); + const appOnError = vi.fn(() => appResponse); + const options: Record = { onError: appOnError }; + + publishCreateRequestListener([() => new Response(), options]); + const error = new Error('boom'); + const returned = await (options.onError as (e: unknown) => Promise)(error); + + expect(captureException).toHaveBeenCalledWith(error, { + mechanism: { handled: false, type: 'auto.http.remix_v3.on_error' }, + }); + expect(appOnError).toHaveBeenCalledWith(error); + expect(returned).toBe(appResponse); + }); + + it('installs a hook when the app passed no options at all', () => { + const args: unknown[] = [() => new Response()]; + + publishCreateRequestListener(args); + + expect(args[1]).toEqual(expect.objectContaining({ onError: expect.any(Function) })); + }); +}); diff --git a/packages/server-utils/src/orchestrion/config/remix-v3.ts b/packages/server-utils/src/orchestrion/config/remix-v3.ts index 831232dcd276..51ed8a14adef 100644 --- a/packages/server-utils/src/orchestrion/config/remix-v3.ts +++ b/packages/server-utils/src/orchestrion/config/remix-v3.ts @@ -26,9 +26,20 @@ export const remixV3Config: InstrumentationConfig[] = [ module: { name: '@remix-run/assets', versionRange: '>=0.6.0 <1', filePath: 'dist/lib/asset-server.js' }, functionQuery: { functionName: 'createAssetServer', kind: 'Sync' }, }, + // The only error hook that covers an app whose fetch handler is not a router. + { + channelName: 'createRequestListener', + module: { + name: '@remix-run/node-fetch-server', + versionRange: '>=0.14.0 <1', + filePath: 'dist/lib/request-listener.js', + }, + functionQuery: { functionName: 'createRequestListener', kind: 'Sync' }, + }, ]; export const remixV3Channels = { REMIX_V3_CREATE_ROUTER: 'orchestrion:@remix-run/fetch-router:createRouter', REMIX_V3_CREATE_ASSET_SERVER: 'orchestrion:@remix-run/assets:createAssetServer', + REMIX_V3_CREATE_REQUEST_LISTENER: 'orchestrion:@remix-run/node-fetch-server:createRequestListener', } as const; From ad5bcdbdc8901cf10a24b79f5dc81c3730f1ba5d Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Thu, 1 Oct 2026 16:08:27 +0200 Subject: [PATCH 2/2] fix(remix): Keep the request listener's default error logging Setting `onError` replaces the listener's default handler, which logs the error to the console. When the app passed no handler of its own, the hook now logs the same way after capturing. Co-Authored-By: Claude Fable 5.1 --- packages/remix/src/v3/server/instrument.ts | 11 +++++++++-- packages/remix/test/v3/instrument.test.ts | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/packages/remix/src/v3/server/instrument.ts b/packages/remix/src/v3/server/instrument.ts index 75e5a848a683..d16fff4746af 100644 --- a/packages/remix/src/v3/server/instrument.ts +++ b/packages/remix/src/v3/server/instrument.ts @@ -92,8 +92,15 @@ function injectOnError(raw: Record | undefined): void { options.onError = error => { captureRequestError(error, undefined, 'auto.http.remix_v3.on_error'); - // Chained so the app keeps its own response. - return appOnError?.(error); + if (appOnError) { + // Chained so the app keeps its own response. + return appOnError(error); + } + + // Setting `onError` replaced the listener's default handler, which logs the error. + // oxlint-disable-next-line no-console + console.error(error); + return undefined; }; } diff --git a/packages/remix/test/v3/instrument.test.ts b/packages/remix/test/v3/instrument.test.ts index e74e7b68f224..1fa4e14d0513 100644 --- a/packages/remix/test/v3/instrument.test.ts +++ b/packages/remix/test/v3/instrument.test.ts @@ -108,6 +108,23 @@ describe('the createRequestListener error hook', () => { expect(returned).toBe(appResponse); }); + it('keeps the default console logging when the app passed no onError', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined); + const args: unknown[] = [() => new Response()]; + + publishCreateRequestListener(args); + const error = new Error('boom'); + const returned = await (args[1] as { onError: (e: unknown) => unknown }).onError(error); + + expect(captureException).toHaveBeenCalledWith(error, { + mechanism: { handled: false, type: 'auto.http.remix_v3.on_error' }, + }); + // What the listener's own default handler does, which the hook replaced. + expect(consoleError).toHaveBeenCalledWith(error); + expect(returned).toBeUndefined(); + consoleError.mockRestore(); + }); + it('installs a hook when the app passed no options at all', () => { const args: unknown[] = [() => new Response()];