Repository navigation
Conversation
size-limit report 📦Compared: d6a317b (PR test merge using develop at b68afeb) Against: ca4fc32 (develop) · base run #62950
|
3dfbedc to
e9cafef
Compare
e9cafef to
aa89862
Compare
aa89862 to
3b1e94f
Compare
3b1e94f to
f8eea69
Compare
f8eea69 to
bb2078f
Compare
bb2078f to
9c5e4d6
Compare
9c5e4d6 to
62ef54d
Compare
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
62ef54d to
40a29bc
Compare
|
bugbot run |
40a29bc to
fcb4724
Compare
fcb4724 to
acf7baf
Compare
3c79c4e to
cd1c651
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ba30e6a. Configure here.
|
One or more gzipped bundles increased by more than 500 bytes. If this increase is intentional, add the Accept Bundlesize Increase label to this PR to rerun and accept the check. |
Optional E2E testsAll optional checks were successful. 2 successful. See the run. |
The Sentry middleware runs first and answers tunnel requests. The integration hands Astro's `base` and `trailingSlash: 'always'` to both SDKs, so `tunnel: true` follows Astro's routing without repeating them.
…orks on a cold server
This reverts commit 6647a68. The middleware module ran the inline default init a second time, and running the init before a cold non-page request is a gap develop has for every server request, not one the tunnel adds.
The server's tunnel base path only reached `@sentry/astro`'s own server init. On Workers the client comes from `@sentry/cloudflare`, so the browser posted under `base` and got a 404. The server side of the tunnel options module now goes through core's managed tunnel defaults, and the Worker entry imports it first.
…the tunnel variant, and drop tests that repeat core's
…request from a prerendered page finds a client
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Two minor concerns to address, approving to unblock. Mostly LGTM.
| addMiddleware({ | ||
| order: 'pre', | ||
| entrypoint: '@sentry/astro/middleware', | ||
| entrypoint: serverInitSnippet ? SENTRY_MIDDLEWARE_MODULE : '@sentry/astro/middleware', |
There was a problem hiding this comment.
Before this PR, Sentry.init on the server ran only when an SSR page rendered. Now it also runs on the first request that reaches the Sentry middleware. The tunnel needs this, but it also changes behavior for everything else the middleware handles. Server-island requests, API endpoints and other non-page routes hit after a prerendered page are now instrumented from the first request. The comment block above if (!getClient()) describes the old behavior, including a TODO with repro steps ("Run the astro-5 tracing.serverIslands.test only"). After this PR that comment is mostly wrong: the bail now only covers apps with no server init at all. That is mostly a fix, and a good one, but it is a visible behavior change hidden inside a tunnel PR.
The PR also swaps the public middleware entry for a virtual one (virtual:@sentry/astro/middleware) whenever the integration injects the init. Users who disable the middleware with autoInstrumentation.requestHandler: false and add @sentry/astro/middleware themselves with sequence() don't get either the cold-start init or the tunnel, unless an SSR page rendered first.
Suggestion: update or remove the stale comment and TODO in packages/astro/src/server/middleware.ts. Note the init change in the PR body and changelog. For manual middleware users, either export the virtual entry's behavior from a public path (for example @sentry/astro/middleware importing the init when the integration provides one), or warn at build time when tunnel is set and the middleware is off. The integration can't see the runtime tunnel option, so the docs may be the only practical fix.
| */ | ||
| export function applyAstroTunnelOptions(options: Pick<Options, 'tunnel'>): void { | ||
| const astroTunnelOptions = (GLOBAL_OBJ as { [TUNNEL_OPTIONS_GLOBAL]?: BrowserTunnelOptions })[TUNNEL_OPTIONS_GLOBAL]; | ||
| options.tunnel = applyTunnelDefaults(options.tunnel, astroTunnelOptions); |
There was a problem hiding this comment.
The browser now rewrites a user's explicit relative tunnel string. This is the same issue as in #25415. Commit 36fcbad ("Serve a relative tunnel path under the base path too") passes relative strings through applyTunnelDefaults. With base: '/docs', tunnel: '/tunnel' in the browser now posts to /docs/tunnel (plus a trailing slash under trailingSlash: 'always'). Before this PR, it posted to /tunnel. A user whose tunnel lives outside Astro (a reverse proxy, CDN rule or separate service) silently breaks. /docs/tunnel still works, because core strips a duplicated base path.
There is a stronger case here than in SvelteKit for adding the trailing slash. Astro enforces trailingSlash before middleware, so a managed path without it can't work. But that argument covers only paths Astro serves.
Suggestion: apply the defaults only for tunnel: true and object forms, and leave explicit strings as written. Or keep the behavior and list it in the changelog as a change for apps with base and a relative tunnel. Whichever way you choose, make the same decision across the stack (SvelteKit, Astro, and any other PR that copied e7224db / 36fcbad).
This PR adds the tunnel wiring to the astro SDK. We already had a global middleware active, so it works by adding logic to check if the incoming request is a tunnel request and if it is, then it will be handled.
This also respects the decision made earlier to follow the base path configured by the user, which comes from Astro's
baseconfig.The
astro-7andastro-6-cf-workerstunnel variants cover node and Cloudflare.For reviewers: if you need an overview of what this is, refer to #25199.
closes #25463