Skip to content

Commit b5ae514

Browse files
authored
fix(nextjs): Return correct lastEventId for SSR pages (#19240)
When using `captureUnderscoreErrorException` on an `_error` page, the events were mostly dropped because it already existed from a Sentry-wrapped data fetcher (like `getServerProps`). This resulted in not sending the error to Sentry but still generating a new event ID which was used as `lastEventId` (and thus was wrong). Closes #19217 Also, check out this specific comment within the issue as it gives more context: #19217 (comment)
1 parent 4763ff1 commit b5ae514

File tree

8 files changed

+153
-2
lines changed

8 files changed

+153
-2
lines changed
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import type { NextPageContext } from 'next';
2+
import * as Sentry from '@sentry/nextjs';
3+
4+
interface ErrorProps {
5+
statusCode?: number;
6+
eventId?: string;
7+
lastEventId?: string;
8+
}
9+
10+
function ErrorPage({ statusCode, eventId, lastEventId }: ErrorProps) {
11+
return (
12+
<div>
13+
<h1>Error Page</h1>
14+
<p data-testid="status-code">Status Code: {statusCode}</p>
15+
<p data-testid="event-id">Event ID from return: {eventId || 'No event ID'}</p>
16+
<p data-testid="last-event-id">Event ID from lastEventId(): {lastEventId || 'No event ID'}</p>
17+
</div>
18+
);
19+
}
20+
21+
ErrorPage.getInitialProps = async (context: NextPageContext) => {
22+
const { res, err } = context;
23+
24+
const statusCode = res?.statusCode || err?.statusCode || 404;
25+
26+
// Capture the error using captureUnderscoreErrorException
27+
// This should return the already-captured event ID from the data fetcher
28+
const eventId = await Sentry.captureUnderscoreErrorException(context);
29+
30+
// Also get the last event ID from lastEventId()
31+
const lastEventId = Sentry.lastEventId();
32+
33+
return { statusCode, eventId, lastEventId };
34+
};
35+
36+
export default ErrorPage;
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
export default function TestErrorPage() {
2+
return <div>This page should never render</div>;
3+
}
4+
5+
export function getServerSideProps() {
6+
throw new Error('Test error to trigger _error.tsx page');
7+
}
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { expect, test } from '@playwright/test';
2+
import { waitForError } from '@sentry-internal/test-utils';
3+
import { isDevMode } from './isDevMode';
4+
import { isNext13 } from './nextjsVersion';
5+
6+
test('lastEventId() should return the event ID after captureUnderscoreErrorException', async ({ page }) => {
7+
test.skip(isDevMode, 'should be skipped for non-dev mode');
8+
test.skip(isNext13, 'should be skipped for Next.js 13');
9+
10+
const errorEventPromise = waitForError('nextjs-pages-dir', errorEvent => {
11+
return errorEvent?.exception?.values?.[0]?.value === 'Test error to trigger _error.tsx page';
12+
});
13+
14+
await page.goto('/underscore-error/test-error-page');
15+
const errorEvent = await errorEventPromise;
16+
17+
// Since the error is already captured by withErrorInstrumentation in getServerSideProps,
18+
// the mechanism should be 'auto.function.nextjs.wrapped', not 'auto.function.nextjs.underscore_error'
19+
expect(errorEvent.exception?.values?.[0]?.mechanism?.type).toBe('auto.function.nextjs.wrapped');
20+
// The function name might be e.g. 'getServerSideProps$1'
21+
expect(errorEvent.exception?.values?.[0]?.mechanism?.data?.function).toContain('getServerSideProps');
22+
expect(errorEvent.exception?.values?.[0]?.mechanism?.handled).toBe(false);
23+
24+
const eventIdFromReturn = await page.locator('[data-testid="event-id"]').textContent();
25+
const returnedEventId = eventIdFromReturn?.replace('Event ID from return: ', '');
26+
27+
const lastEventIdFromFunction = await page.locator('[data-testid="last-event-id"]').textContent();
28+
const lastEventId = lastEventIdFromFunction?.replace('Event ID from lastEventId(): ', '');
29+
30+
expect(returnedEventId).toBeDefined();
31+
expect(returnedEventId).not.toBe('No event ID');
32+
expect(lastEventId).toBeDefined();
33+
expect(lastEventId).not.toBe('No event ID');
34+
35+
expect(lastEventId).toBe(returnedEventId);
36+
expect(errorEvent.event_id).toBe(returnedEventId);
37+
expect(errorEvent.event_id).toBe(lastEventId);
38+
});
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
const packageJson = require('../package.json');
2+
const nextjsVersion = packageJson.dependencies.next;
3+
const nextjsMajor = Number(nextjsVersion.split('.')[0]);
4+
5+
export const isNext13 = !isNaN(nextjsMajor) && nextjsMajor === 13;

packages/core/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ export {
221221
addExceptionMechanism,
222222
addExceptionTypeValue,
223223
checkOrSetAlreadyCaught,
224+
isAlreadyCaptured,
224225
getEventDescription,
225226
parseSemver,
226227
uuid4,

packages/core/src/utils/misc.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,13 @@ export function checkOrSetAlreadyCaught(exception: unknown): boolean {
227227
return false;
228228
}
229229

230-
function isAlreadyCaptured(exception: unknown): boolean | void {
230+
/**
231+
* Checks whether we've already captured the given exception (note: not an identical exception - the very object).
232+
* It is considered already captured if it has the `__sentry_captured__` property set to `true`.
233+
*
234+
* @internal Only considered for internal usage
235+
*/
236+
export function isAlreadyCaptured(exception: unknown): boolean | void {
231237
try {
232238
return (exception as { __sentry_captured__?: boolean }).__sentry_captured__;
233239
} catch {} // eslint-disable-line no-empty

packages/nextjs/src/common/pages-router-instrumentation/_error.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import { captureException, httpRequestToRequestData, withScope } from '@sentry/core';
1+
import {
2+
captureException,
3+
getIsolationScope,
4+
httpRequestToRequestData,
5+
isAlreadyCaptured,
6+
withScope,
7+
} from '@sentry/core';
28
import type { NextPageContext } from 'next';
39
import { flushSafelyWithTimeout, waitUntil } from '../utils/responseEnd';
410

@@ -38,6 +44,13 @@ export async function captureUnderscoreErrorException(contextOrProps: ContextOrP
3844
return;
3945
}
4046

47+
// If the error was already captured (e.g., by wrapped functions in data fetchers),
48+
// return the existing event ID instead of capturing it again (needed for lastEventId() to work)
49+
if (err && isAlreadyCaptured(err)) {
50+
waitUntil(flushSafelyWithTimeout());
51+
return getIsolationScope().lastEventId();
52+
}
53+
4154
const eventId = withScope(scope => {
4255
if (req) {
4356
const normalizedRequest = httpRequestToRequestData(req);

packages/nextjs/test/common/pages-router-instrumentation/captureUnderscoreErrorException.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
11
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
22
import { captureUnderscoreErrorException } from '../../../src/common/pages-router-instrumentation/_error';
33

4+
let storedLastEventId: string | undefined = undefined;
5+
46
const mockCaptureException = vi.fn(() => 'test-event-id');
57
const mockWithScope = vi.fn((callback: (scope: any) => any) => {
68
const mockScope = {
79
setSDKProcessingMetadata: vi.fn(),
810
};
911
return callback(mockScope);
1012
});
13+
const mockGetIsolationScope = vi.fn(() => ({
14+
setLastEventId: (id: string | undefined) => {
15+
storedLastEventId = id;
16+
},
17+
lastEventId: () => storedLastEventId,
18+
}));
1119

1220
vi.mock('@sentry/core', async () => {
1321
const actual = await vi.importActual('@sentry/core');
@@ -16,6 +24,8 @@ vi.mock('@sentry/core', async () => {
1624
captureException: (...args: unknown[]) => mockCaptureException(...args),
1725
withScope: (callback: (scope: any) => any) => mockWithScope(callback),
1826
httpRequestToRequestData: vi.fn(() => ({ url: 'http://test.com' })),
27+
lastEventId: () => mockGetIsolationScope().lastEventId(),
28+
getIsolationScope: () => mockGetIsolationScope(),
1929
};
2030
});
2131

@@ -27,6 +37,7 @@ vi.mock('../../../src/common/utils/responseEnd', () => ({
2737
describe('captureUnderscoreErrorException', () => {
2838
beforeEach(() => {
2939
vi.clearAllMocks();
40+
storedLastEventId = undefined;
3041
});
3142

3243
afterEach(() => {
@@ -114,4 +125,38 @@ describe('captureUnderscoreErrorException', () => {
114125
expect(result).toBeUndefined();
115126
expect(mockCaptureException).not.toHaveBeenCalled();
116127
});
128+
129+
it('should return existing event ID for already captured errors without re-capturing', async () => {
130+
// Set up an existing event ID in the isolation scope
131+
storedLastEventId = 'existing-event-id';
132+
133+
// Create an error that has already been captured (marked with __sentry_captured__)
134+
const error = new Error('Already captured error');
135+
(error as any).__sentry_captured__ = true;
136+
137+
const eventId = await captureUnderscoreErrorException({
138+
err: error,
139+
pathname: '/test',
140+
res: { statusCode: 500 } as any,
141+
});
142+
143+
// Should return the existing event ID
144+
expect(eventId).toBe('existing-event-id');
145+
// Should NOT call captureException again
146+
expect(mockCaptureException).not.toHaveBeenCalled();
147+
});
148+
149+
it('should capture string errors even if they were marked as captured', async () => {
150+
// String errors can't have __sentry_captured__ property, so they should always be captured
151+
const errorString = 'String error';
152+
153+
const eventId = await captureUnderscoreErrorException({
154+
err: errorString,
155+
pathname: '/test',
156+
res: { statusCode: 500 } as any,
157+
});
158+
159+
expect(eventId).toBe('test-event-id');
160+
expect(mockCaptureException).toHaveBeenCalled();
161+
});
117162
});

0 commit comments

Comments
 (0)