Conversation
Codecov Results 📊Generated by Codecov Action |
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
81b555e to
161862d
Compare
size-limit report 📦
|
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
2 times, most recently
from
1317257 to
2a86160
Compare
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
99ea611 to
9ded4c9
Compare
logaretm
changed the title
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/in/1153895?s=60&v=4){kind=small}
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
53bf486 to
5f43f63
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }, | ||
|
|
||
| "//": "This is built separately in tsconfig.setup-types.json", | ||
| "exclude": ["src/setup.ts"] |
There was a problem hiding this comment.
Copy-pasted tsconfig excludes non-existent setup files
Low Severity
The tsconfig.types.json excludes src/setup.ts and references a tsconfig.setup-types.json — neither of which exist in this package. This was copied from the NestJS package (which actually has both files). While functionally harmless, it's misleading scaffolding that suggests missing build infrastructure.
Reviewed by Cursor Bugbot for commit ff579f3. Configure here.
There was a problem hiding this comment.
Pull request overview
Adds a new @sentry/nitro package skeleton to the monorepo and wires it into the repo’s release/testing/triage infrastructure as the base for a stacked series of Nitro SDK PRs.
Changes:
- Introduces the new
packages/nitropackage with initial SDK/config scaffolding, build config, and a placeholder test. - Registers the package across monorepo plumbing (workspace list, root README, issue templates/labeling, release config).
- Updates lockfile for the newly introduced dependency graph.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Lockfile updates due to dependency graph changes. |
| packages/nitro/package.json | New package manifest, exports, build/test scripts. |
| packages/nitro/rollup.npm.config.mjs | Rollup config for publishing builds (ESM-only). |
| packages/nitro/tsconfig.json | Package TS config (module/moduleResolution overrides). |
| packages/nitro/tsconfig.types.json | Declaration-only build config for emitted types. |
| packages/nitro/tsconfig.test.json | Test TS config for package tests. |
| packages/nitro/src/index.ts | Public entrypoint re-exporting config + @sentry/node + Nitro init. |
| packages/nitro/src/sdk.ts | Nitro SDK initialization wrapper around @sentry/node. |
| packages/nitro/src/config.ts | withSentryConfig / module setup helper for Nitro config. |
| packages/nitro/src/module.ts | Nitro module factory placeholder. |
| packages/nitro/src/common/debug-build.ts | Debug-build flag constant (pattern consistent with other packages). |
| packages/nitro/src/utils/resolver.ts | Path resolver helper utility. |
| packages/nitro/src/utils/plugin.ts | Helper to add Nitro plugins. |
| packages/nitro/test/index.test.ts | Placeholder Vitest test. |
| packages/nitro/test/tsconfig.json | Test tsconfig shim extending package test config. |
| packages/nitro/README.md | Initial package documentation. |
| packages/nitro/LICENSE | Package license file. |
| packages/nitro/.eslintrc.js | Package-local ESLint config extending repo base. |
| package.json | Adds packages/nitro to workspace list. |
| dev-packages/e2e-tests/verdaccio-config/config.yaml | Allows publishing @sentry/nitro in Verdaccio E2E setup. |
| README.md | Adds @sentry/nitro to root package list. |
| .github/workflows/issue-package-label.yml | Adds Nitro to package-to-label mapping. |
| .github/ISSUE_TEMPLATE/bug.yml | Adds @sentry/nitro to bug template SDK dropdown. |
| .craft.yml | Adds Nitro package to Craft release targets/metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Compatibility | ||
|
|
||
| The minimum supported version of Nitro is `3.0.0-alpha.1`. |
There was a problem hiding this comment.
The README states the minimum supported Nitro version is 3.0.0-alpha.1, but peerDependencies requires nitro >=3.0.260311-beta. Please align the README with the actual supported range (or adjust the peer dependency) to avoid confusing users.
| The minimum supported version of Nitro is `3.0.0-alpha.1`. | |
| The minimum supported version of Nitro is `3.0.260311-beta`. |
| ...options, | ||
| }; | ||
|
|
||
| applySdkMetadata(opts, 'nitro'); |
There was a problem hiding this comment.
For wrapper SDKs around @sentry/node, we typically include both the wrapper and node package names in applySdkMetadata so events show the full package chain (e.g. Remix uses ['remix', 'node']). Consider using applySdkMetadata(opts, 'nitro', ['nitro', 'node']) for consistency.
| applySdkMetadata(opts, 'nitro'); | |
| applySdkMetadata(opts, 'nitro', ['nitro', 'node']); |
There was a problem hiding this comment.
mmm, we need a better way to do this but that's a fine suggestion for now.
|
Thank you for all comments @s1gr1d very thorough review! all addressed 🙏 |
| /* eslint-disable import/export */ | ||
| export * from './config'; | ||
| export * from '@sentry/node'; | ||
| export { init } from './sdk'; |
There was a problem hiding this comment.
Missing getDefaultIntegrations export from package index
Medium Severity
The Nitro-specific getDefaultIntegrations from ./sdk is not re-exported in index.ts. Only init is explicitly exported, so the getDefaultIntegrations that consumers receive via export * from '@sentry/node' is the node version, not the Nitro one. Every other SDK in the monorepo (NestJS, Elysia, Bun, Angular, Browser, AWS Serverless) exports both init and getDefaultIntegrations from their ./sdk module. This inconsistency will silently cause the wrong function to be used when Nitro-specific integrations are added.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5f3baf0. Configure here.
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
5f3baf0 to
1dad524
Compare
logaretm
changed the base branch from
develop
to
awad/tracing-channel-otel-vendoring
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
1dad524 to
120ecba
Compare
logaretm
force-pushed
the
awad/tracing-channel-otel-vendoring
branch
from
f3b037f to
1b87a8f
Compare
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
120ecba to
014114f
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Implements HTTP server instrumentation for both `h3` and `srvx` by listening to their tracing channel events. - `h3` TC PR: h3js/h3#1251 - `srvx` TC PR: h3js/srvx#141 Closes #18123 --- **This PR is part of a stack:** - #20358 - #19224 - #19225 👈 - #19304 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| label: 'nestjs-microservices (latest)' | ||
| - test-application: 'nitro-3' | ||
| build-command: 'test:build-canary' | ||
| label: 'nitro-3 (canary)' |
There was a problem hiding this comment.
Missing test:build-canary script breaks canary CI
High Severity
The canary workflow references test:build-canary as the build-command for the nitro-3 test application, but the package.json for nitro-3 only defines test:build — there is no test:build-canary script. The workflow step executes yarn ${{ matrix.build-command }}, which will fail. Other test applications like nuxt-3 and nuxt-4 correctly define test:build-canary scripts that install canary/nightly versions of their framework dependencies.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4cd6415. Configure here.
| await request.get('/api/non-existent-route'); | ||
|
|
||
| expect(errorReceived).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Negative test never fails due to race condition
Medium Severity
The "Does not send 404 errors" test voids the waitForError promise and immediately asserts errorReceived is false after the request completes. Since there's no waiting period, even if the SDK incorrectly sent a 404 error, it likely wouldn't arrive at the proxy server before the assertion runs. This test effectively provides no confidence that 404 errors are filtered — it will always pass regardless of SDK behavior.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 4cd6415. Configure here.
- Update LICENSE copyright year to 2026 - Expand peerDependencies range to cover nitro prereleases - Bump nitro devDependency to latest beta - Add beta notice to README - Improve README wording (section title, Vite capitalization, --import mention) - Point .craft.yml docs URL to package README until docs page exists - Include 'node' in applySdkMetadata packages list for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The repo uses Oxlint, not ESLint. Remove the legacy .eslintrc.js file. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements HTTP server instrumentation for both `h3` and `srvx` by listening to their tracing channel events. - `h3` TC PR: h3js/h3#1251 - `srvx` TC PR: h3js/srvx#141 Closes #18123 --- **This PR is part of a stack:** - #20358 - #19224 - #19225 👈 - #19304 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
4cd6415 to
47d85ab
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 6 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 47d85ab. Configure here.
| "clean": "npx rimraf node_modules pnpm-lock.yaml .output", | ||
| "test": "playwright test", | ||
| "test:build": "pnpm install && pnpm build", | ||
| "test:assert": "pnpm test" |
There was a problem hiding this comment.
Missing test:build-canary script breaks canary CI
Medium Severity
The canary workflow specifies build-command: 'test:build-canary' for nitro-3, but the package.json only defines test:build — there is no test:build-canary script. Every other canary test application (e.g. nuxt-3, nextjs-14) explicitly defines a test:build-canary script that installs canary/nightly versions of its framework. The canary CI job will fail when it runs yarn test:build-canary and the script is not found.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 47d85ab. Configure here.
| captureException(data.error, { mechanism: { type: 'auto.http.nitro.onTraceError', handled: false } }); | ||
| data._sentrySpan?.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| data._sentrySpan?.end(); | ||
| } |
There was a problem hiding this comment.
onTraceError duplicates error capture without HTTP filtering
Medium Severity
onTraceError calls captureException for all errors without filtering, while captureErrorHook deliberately skips 3xx/4xx HTTPErrors. If both tracing channel error handlers and Nitro's error hook fire for the same error, this results in duplicate error events in Sentry. Additionally, onTraceError may capture HTTP errors that captureErrorHook intentionally suppresses, undermining the filtering logic.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 47d85ab. Configure here.
Implements HTTP server instrumentation for both `h3` and `srvx` by listening to their tracing channel events. - `h3` TC PR: h3js/h3#1251 - `srvx` TC PR: h3js/srvx#141 Closes #18123 --- **This PR is part of a stack:** - #20358 - #19224 - #19225 👈 - #19304 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
logaretm
force-pushed
the
awad/create-nitro-sdk-metadata
branch
from
47d85ab to
fa580dd
Compare
This PR just isolates the mundane changes needed for a new SDK to keep the next stacked PRs clean, it adds the Nitro SDK to the monorepo.
This PR is a base of a stack, the stacked PRs will be merged into it. I thought this will be easier to review.
This PR is part of a stack: