Repository navigation
Conversation
fb12663 to
de8fd3d
Compare
s1gr1d
changed the title
size-limit report 📦
|
|
bugbot run |
![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.
Reviewed by Cursor Bugbot for commit de8fd3d. Configure here.
| await page.goto('/'); | ||
| await pageloadSpanPromise; | ||
| // Wait for the server to send the spans that the tunnel request would wrongly produce | ||
| await page.waitForTimeout(3000); |
There was a problem hiding this comment.
Tunnel test sleeps to assert absence
Low Severity
The new handler test asserts that tunnel requests create no server spans by sleeping for three seconds after pageload. Absence checks that wait on a timeout stay racy under load and can pass while a late span still arrives.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit de8fd3d. Configure here.
| const ingestSpans = allSpans.filter( | ||
| span => | ||
| getSpanOp(span) === 'http.client' && | ||
| !!getStringAttribute(span, 'url.full')?.startsWith('http://localhost:3031/'), |
There was a problem hiding this comment.
Tunnel tests hardcode span attributes
Low Severity
New span assertions compare http.request.method and url.full (and http.target / next.route in the unit tests) as raw strings. Those names already have constants in @sentry/conventions.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit de8fd3d. Configure here.
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Tiny issue and a small FYI, LGTM and looks nice!
| } | ||
|
|
||
| // Like the SDK transports, so that the forward creates no spans, breadcrumbs or trace headers. | ||
| return suppressTracing(() => handleTunnelRequest({ request, allowedDsns })); |
There was a problem hiding this comment.
FYI: This will lose the actual client IP, so relay will think the server is the user. Another issue is it parses the whole payload only to re-serialize it again.
This is pr-existing in the current tunnel utils, I'm handling both concern in my project stack atm so let's keep it. Just an FYI.
Check #25199 for reference.
| projectDir: string = process.cwd(), | ||
| ): string | undefined { | ||
| // Like Next.js, ignore `src/app` if there is a root `app` directory. | ||
| const appDir = ['app', 'src/app'].find(dir => getStats(path.join(projectDir, dir))?.isDirectory()); |
There was a problem hiding this comment.
If the root has pages/ and the handler is in src/app/sentry-tunnel/route.ts, this picks src/app and enables handler mode.
Next.js ignores src/ when app or pages exists in the root, so /sentry-tunnel 404s and all events are lost. Checking for a root pages too fixes it:
| const appDir = ['app', 'src/app'].find(dir => getStats(path.join(projectDir, dir))?.isDirectory()); | |
| const hasRootDir = ['app', 'pages'].some(dir => getStats(path.join(projectDir, dir))?.isDirectory()); | |
| const appDir = hasRootDir ? 'app' : 'src/app'; |
This branch has not been deployed
2 participants
Today,
tunnelRouterewrites tunnel requests to Sentry as a proxy. This PR adds an opt-in handler mode: ifapp/sentry-tunnel/route.ts(orsrc/app/sentry-tunnel/route.ts) exists, the SDK rewrites the tunnel route internally to it, and the handler sends only the event payload to Sentry. Without the file, nothing changes.tunnelRouteoption, a warning is shown.beforeFiles, so a public file or page at the tunnel path cannot shadow it.createSentryTunnelRoute({ allowedDsns })which covers other DSNs.Tested with unit tests, two new
nextjs-16-tunnelvariants (Turbopack/Node.js, webpack/edge) that also check the forwarded headers.Also tested with a manual Vercel deploy on Next.js 14-16.
Fixes JS-3927