fix(nextjs): Return correct lastEventId for SSR pages #19240
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (err && checkOrSetAlreadyCaught(err)) { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| return getIsolationScope().lastEventId(); | ||
| } |
There was a problem hiding this comment.
Fresh errors not sent when calling captureException
High Severity
Calling checkOrSetAlreadyCaught before captureException marks fresh errors as already captured, preventing them from being sent to Sentry. When captureException is subsequently called at line 62, it detects the error as already captured (due to the check at line 49) and skips processing it, generating a new event ID without actually sending the event. This breaks scenarios where errors occur directly in _error.tsx pages without being pre-captured by data fetchers.
| await page.goto('/underscore-error/test-error-page'); | ||
| const errorEvent = await errorEventPromise; | ||
|
|
||
| console.log('errorEvent.exception?.values?.[0]?.mechanism', errorEvent.exception?.values?.[0]?.mechanism); |
There was a problem hiding this comment.
Console.log statement left in test code
Low Severity
A console.log statement logs the error event mechanism, which appears to be debug code that was accidentally left in the test. The logged values are asserted in the subsequent expectations (lines 18-21), making the console output redundant.
| // return the existing event ID instead of capturing it again (needed for lastEventId() to work) | ||
| if (err && checkOrSetAlreadyCaught(err)) { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| return getIsolationScope().lastEventId(); |
There was a problem hiding this comment.
I feel like this has a high chance of not being the event id we are looking for... we really should think about storing lastEventId with more info of what the actual event was that this belongs to.
I don't really think there's much we can do here, just wanted to mention this tho and this is probably better than straight up return an event id of an event that gets deduped.
There was a problem hiding this comment.
I think this can work but doesn't feel bullet proof to me.
Could we perhaps associate error objects with their event IDs? Like add a __sentry_evt_id__ to each error instance so that we can refer to that purely from the error object regardless of where we catch it?
|
|
||
| // If the error was already captured (e.g., by wrapped functions in data fetchers), | ||
| // return the existing event ID instead of capturing it again (needed for lastEventId() to work) | ||
| if (err && checkOrSetAlreadyCaught(err)) { |
There was a problem hiding this comment.
This modifies the error object already before sending it, which we do not want bc this prevents it from capturing it?
There was a problem hiding this comment.
probably worth exposing isAlreadyCaptured from core since we want this to be read only.
4 participants
When using
captureUnderscoreErrorExceptionon an_errorpage, the events were mostly dropped because it already existed from a Sentry-wrapped data fetcher (likegetServerProps). This resulted in not sending the error to Sentry but still generating a new event ID which was used aslastEventId(and thus was wrong).Closes #19217
Also, check out this specific comment within the issue as it gives more context: #19217 (comment)