Remove Sentry error capture integration - #645
Conversation
Fully removes the optional server-side Sentry integration since it is no longer used. The capture calls were fire-and-forget no-ops without a SENTRY_DSN, so removing them preserves runtime behavior. - Delete src/lib/observability/error-capture.ts and its test. - Drop the Sentry init block and the onRequestError hook from src/instrumentation.ts (the boot guards are unchanged). - Remove captureServerException/captureServerEvent call sites in the answer routes and rag.ts, along with the now-dead clientAborted/signal guards that only gated those calls. - Remove the SENTRY_DSN env schema entry and the .env.example block. - Remove the @sentry/node dependency and regenerate the lockfile. - Strip the Sentry-specific cases from tests/instrumentation.test.ts, keeping the production boot-guard coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHJmAtjSweeQLdnhHLicJK
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
CI triageCI failed on this PR. Automated classification of the 9 failed job(s):
Heuristic only — a main-side or flake label is a starting point, not a verdict. |
The previous commit regenerated the lockfile with `npm install --package-lock-only`, which re-resolved the whole tree against a drifted local node_modules and churned unrelated @emnapi transitive versions. That left package-lock.json out of sync with package.json, so every CI job failed at `npm ci` (EUSAGE: "Missing @emnapi/... from lock file"). Restore the lockfile from the branch base and remove only the @sentry entries (the root dependency plus the six @sentry/* tree nodes), leaving every other package — including the nested @emnapi/wasi-threads@1.2.2 entries — byte-identical to the passing base. Verified with `npm ci --dry-run` (exit 0, no sync error). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHJmAtjSweeQLdnhHLicJK
📝 WalkthroughWalkthroughThe change removes Sentry server and client integrations, including dependencies, environment fields, initialization, error capture, fallback telemetry, and client boundary reporting. Answer routes retain structured error responses and logging. Monitoring tests use only the public DSN. ChangesSentry observability removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@copilot resolve the merge conflicts on this branch. |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/answer/stream/route.ts`:
- Around line 95-97: Update logStreamError to restore the prior filtering:
ignore client-abort errors and caught responses with non-500 payload statuses,
while continuing to log only qualifying stream failures through logger.error.
Keep the Sentry capture removed and preserve the existing safeErrorLogDetails
handling for errors that pass the filters.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7285f1a2-1334-42bc-8fe2-7ca37fc4f8c5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.env.examplepackage.jsonsrc/app/api/answer/route.tssrc/app/api/answer/stream/route.tssrc/app/api/monitoring/route.tssrc/instrumentation-client.tssrc/instrumentation.tssrc/lib/env.tssrc/lib/observability/error-capture.tssrc/lib/rag.tstests/error-capture.test.tstests/instrumentation.test.tstests/sentry-client.test.ts
💤 Files with no reviewable changes (6)
- src/lib/observability/error-capture.ts
- tests/error-capture.test.ts
- src/lib/rag.ts
- package.json
- src/app/api/answer/route.ts
- .env.example
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
main merged #642 (gated client-side Sentry via a same-origin tunnel) while this branch removed server-side Sentry. Per the decision to remove Sentry entirely, this extends the removal to the client layer merged from main: - Delete src/lib/observability/sentry-client.ts, sentry-scrub.ts, the /api/monitoring tunnel route, and tests/sentry-client.test.ts. - Restore src/instrumentation-client.ts to its pre-#642 form: the Zod `jitless` CSP fix stays (it is unrelated to Sentry); the Sentry.init block and __SENTRY_ENABLED__ gate are gone. - Drop the __SENTRY_ENABLED__ DefinePlugin from next.config.ts (the Node-24 hashFunction webpack fix is kept). - Remove NEXT_PUBLIC_SENTRY_DSN / NEXT_PUBLIC_SENTRY_ENVIRONMENT from the env schema and the client capture calls in global-error.tsx and route-error-boundary.tsx (console.error logging stays). - Remove @sentry/browser and strip all @sentry entries from the lockfile. - Regenerate docs/site-map.md (the /api/monitoring route is gone). Also restore the abort / sub-500 filter in the stream route's logStreamError so client aborts are no longer logged as failures — the original comment's documented intent, now that logger.error is the only sink (addresses a review finding). Verified: typecheck + lint clean; 2283 vitest tests pass; npm ci --dry-run in sync; sitemap:check passes. (knip and the jsdom DOM tests are not installed in this environment; CI covers them.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XHJmAtjSweeQLdnhHLicJK
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/answer/stream/route.ts (2)
306-308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the original exception when replacing Sentry capture.
This branch wraps the error with a new
PublicApiErrorcontaining onlyerror.name, then passes that wrapper tojsonError. The original message and stack are therefore unavailable to the supplied logging path, leaving production logs without the root cause after Sentry removal. Preserve the original error as a cause or log its sanitized details before wrapping it for the public response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/answer/stream/route.ts` around lines 306 - 308, Update the Error branch in the answer-processing handler to preserve the original exception’s message and stack for internal logging while still returning the sanitized PublicApiError response. Pass the original error as the wrapper’s cause or explicitly log sanitized error details before calling jsonError, without exposing sensitive details publicly.
303-308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep client-abort gating before
jsonError.Abort errors raised during parsing, authentication, or rate-limit work now reach
jsonError, whose logging path records them. The stream-local abort guard does not cover this outer catch, so routine client cancellations can still pollute server-failure logs. Restore an earlyrequest.signal.abortedpath or makejsonErrorabort-aware.As per coding guidelines, add a targeted regression test for this abort path and run the narrowest relevant validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/answer/stream/route.ts` around lines 303 - 308, Ensure the outer error handling in the answer stream route checks request.signal.aborted before either jsonError call, returning the existing client-abort response without logging it as a server failure. Preserve the current PublicApiError and generic Error responses for non-aborted requests, and add a focused regression test covering an abort during parsing, authentication, or rate-limit processing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/app/api/answer/stream/route.ts`:
- Around line 306-308: Update the Error branch in the answer-processing handler
to preserve the original exception’s message and stack for internal logging
while still returning the sanitized PublicApiError response. Pass the original
error as the wrapper’s cause or explicitly log sanitized error details before
calling jsonError, without exposing sensitive details publicly.
- Around line 303-308: Ensure the outer error handling in the answer stream
route checks request.signal.aborted before either jsonError call, returning the
existing client-abort response without logging it as a server failure. Preserve
the current PublicApiError and generic Error responses for non-aborted requests,
and add a focused regression test covering an abort during parsing,
authentication, or rate-limit processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a480ca4d-8fbd-4389-ac02-fc7d63544e8e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
docs/site-map.mdnext.config.tspackage.jsonsrc/app/api/answer/stream/route.tssrc/app/api/monitoring/route.tssrc/app/global-error.tsxsrc/components/route-error-boundary.tsxsrc/instrumentation-client.tssrc/lib/env.tssrc/lib/observability/sentry-client.tssrc/lib/observability/sentry-scrub.tstests/sentry-client.test.ts
💤 Files with no reviewable changes (10)
- src/lib/observability/sentry-client.ts
- src/app/global-error.tsx
- src/components/route-error-boundary.tsx
- docs/site-map.md
- src/lib/observability/sentry-scrub.ts
- package.json
- tests/sentry-client.test.ts
- src/lib/env.ts
- src/app/api/monitoring/route.ts
- src/instrumentation-client.ts
#645 removed Sentry from package.json but left 13 unused transitive packages in package-lock.json. Regenerate/cleanup so npm install matches the current dependency graph. Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Summary
src/lib/observability/error-capture.ts(the@sentry/nodewrapper) and its test.Sentry.init(...)block and theonRequestErrorhook fromsrc/instrumentation.ts; the production boot guards (requireServerEnv/requireOpenAIEnv/requireQueryHashSecret, demo-mode and no-auth refusals) are unchanged.captureServerException/captureServerEventcall sites insrc/app/api/answer/route.ts,src/app/api/answer/stream/route.ts, andsrc/lib/rag.ts, along with the now-deadclientAborted/signalguards that existed only to gate those calls.SENTRY_DSNentry from the env schema (src/lib/env.ts) and the corresponding block in.env.example.@sentry/nodedependency frompackage.jsonand regeneratespackage-lock.json(453 lines of@sentry/*tree removed).tests/instrumentation.test.ts, keeping full production boot-guard coverage.Behavior is preserved: the capture calls were fire-and-forget and fully inert without a
SENTRY_DSN, so removing them changes no runtime behavior.logStreamErrorstill logs vialogger.error; only the Sentry-reporting branch was removed.Verification
npm run verify:pr-local— not run in full; ran the individual offline gates below instead.npm run typecheck— no errors in any touched file (the 20 reported errors are pre-existing missing-dep issues in untouched DOM/UI test files).npm run lint— clean at--max-warnings 0.vitest run:tests/instrumentation.test.ts(8/8),answer-slo+answer-telemetry+clinical-search(50/50), and all 25rag-*suites (291/291, includingrag-answer-fallbackwhich exercises the generation-fallback path where a capture call was removed).sentry/error-capture/captureServer*references in code, config, or docs.Not run (out of scope for this change):
verify:ui(no UI/routing/styling change),eval:retrieval:qualityandeval:rag(no retrieval, ranking, selection, scoring, synthesis-prompt, or answer-contract change — only an observability call was removed from the fallback branch), andcheck:production-readiness(it is a static env validator that never referencedSENTRY_DSN, so this change is transparent to it; in this demo-mode container it would only emit expected missing-secret noise).Clinical Governance Preflight
This change touches the answer-generation route files, but only to remove observability call sites; no clinical, retrieval, ranking, source-rendering, document-access, or privacy behavior changes.
Clinical KB Database(sjrfecxgysukkwxsowpy) — untouched.Notes
SENTRY_DSNmeans any deployment that set it will now ignore that variable; no other config changes are required.captureServer*helpers — those call sites are now gone and would need to be re-introduced.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
Documentation
/api/monitoringentry from the sitemap..env.exampleto drop the server-side Sentry DSN placeholder.