feat(observability): gated client-side Sentry via same-origin tunnel#642
Conversation
Adds browser error capture as the client counterpart to main's server-side @sentry/node capture. Uses @sentry/browser (not @sentry/nextjs) to avoid overlapping the existing server SDK. Fully inert until NEXT_PUBLIC_SENTRY_DSN is set — the SDK is tree-shaken out of the client bundle (zero bytes shipped). - src/instrumentation-client.ts: gated Sentry.init behind a build-time __SENTRY_ENABLED__ literal (DefinePlugin in next.config.ts) so webpack DCEs the whole block when no public DSN is set; typeof-guarded for Turbopack dev. - src/lib/observability/sentry-scrub.ts: mirrors the server privacy boundary — drops event.request (URL/query/headers), breadcrumbs, and user; keeps only the error. beforeBreadcrumb also returns null. - src/lib/observability/sentry-client.ts: lazy holder so error boundaries call captureClientException() without statically importing the SDK. - src/app/api/monitoring/route.ts: same-origin tunnel (keeps CSP connect-src 'self' intact). Hardened: DSN-match validation (no open relay), 1 MB envelope cap (Content-Length preflight + streamed byte count → 413), and a 5s upstream timeout. - Wires captureClientException into the route + global error boundaries. - env.ts: NEXT_PUBLIC_SENTRY_DSN/ENVIRONMENT (blank normalized to undefined). Verified: build exit 0 with @sentry/browser tree-shaken out of client chunks when DSN unset; typecheck / lint / format clean; new tests cover scrubbing, the inert no-op, and tunnel 404/403/413/forward behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds gated browser Sentry initialization, a same-origin ChangesSentry observability
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Browser as Browser Sentry SDK
participant MonitoringRoute as /api/monitoring
participant SentryIngest as Sentry ingest endpoint
Browser->>MonitoringRoute: POST event envelope
MonitoringRoute->>MonitoringRoute: validate DSN and payload
MonitoringRoute->>SentryIngest: forward validated envelope
SentryIngest-->>MonitoringRoute: return response
MonitoringRoute-->>Browser: return response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/sentry-client.test.ts (1)
96-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage of the happy path; consider adding negative-path coverage for the remaining validation branches.
This test correctly asserts the forwarded URL, method, and
AbortSignaltimeout wiring. The route (per the related snippet) also has several other branches — invalid/missing envelope header, non-stringdsn, malformed DSN, and upstream fetch failure (502) — that aren't exercised anywhere in this file. Given this route is explicitly a security boundary (no open relay) and a reliability boundary (upstream timeout), it would be worth adding a few more targeted cases (missing newline → 400, unparsable header → 400, non-stringdsn→ 400,fetchrejecting/timing out → 502).🤖 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 `@tests/sentry-client.test.ts` around lines 96 - 115, Add targeted negative-path tests alongside the existing Sentry forwarding test, covering missing envelope newline, unparsable envelope headers, non-string dsn values, and upstream fetch rejection or timeout. Assert the route returns 400 for validation failures and 502 when fetch fails, using the existing loadRoute, POST, envelope, and fetchMock test patterns.next.config.ts (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueP2 summary (per coding guideline: "P2 context belongs in one summary"). Consolidated minor notes across this cohort, none individually rising to the 3-inline-finding cap:
next.config.tsLine 54 vs.src/lib/env.tsLines 32-34:__SENTRY_ENABLED__isBoolean(process.env.NEXT_PUBLIC_SENTRY_DSN)(any non-empty string, including whitespace-only, is truthy), whileenvSchemanormalizes a whitespace-only DSN toundefinedvia.trim() === "". A whitespace-onlyNEXT_PUBLIC_SENTRY_DSNwould ship the Sentry chunk (__SENTRY_ENABLED__ = true) yet fail DSN parsing atSentry.inittime ininstrumentation-client.ts(which also reads the raw, untrimmedprocess.env.NEXT_PUBLIC_SENTRY_DSN). Low real-world likelihood, but worth aligning the truthy check with the same trim logic for consistency.next.config.tsLines 52-56: theDefinePluginis pushed unconditionally for every webpack compiler pass (client, server, edge), even though__SENTRY_ENABLED__is only consumed byinstrumentation-client.ts(browser-only). Harmless today, but scoping it behind theisServer/nextRuntimeoption (already available on the secondwebpack()argument) would avoid defining an unused global on non-client bundles.src/app/api/monitoring/route.tsLines 32-37 (readCappedText's!readerfallback branch): comparestext.length(UTF-16 code units) against the byte-orientedmaxBytesthreshold. For multi-byte UTF-8 payloads this undercounts actual byte size, technically allowing the 1 MB cap to be bypassed on this path. In practicerequest.bodyis virtually always aReadableStreamfor a POST with a body under Node's fetch implementation, so this branch is effectively unreachable — flagging for defensive-coding completeness only.src/lib/env.tsLines 32-35:z.string().url()is deprecated in Zod 4 in favor of the top-levelz.url()(still functional, not removed) — consistent with the pre-existingSENTRY_DSNentry, so not a regression, just worth aligning on the new API when this file is next touched.- Per the repo guideline for
**/*.{js,jsx,ts,tsx}environment changes ("run the smallest relevant domain check plusnpm run check:production-readiness"), recommend runningnpm run check:production-readinessbefore merge given the newNEXT_PUBLIC_SENTRY_DSN/NEXT_PUBLIC_SENTRY_ENVIRONMENTenv additions.🤖 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 `@next.config.ts` around lines 52 - 56, Align the __SENTRY_ENABLED__ value in the DefinePlugin configuration with envSchema by trimming NEXT_PUBLIC_SENTRY_DSN before checking it, and define the plugin only for the browser compiler using the available webpack options. In readCappedText, enforce maxBytes using UTF-8 byte length in the !reader fallback. When updating envSchema, prefer the Zod 4 top-level URL validator consistently, then run check:production-readiness.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.
Inline comments:
In `@src/app/api/monitoring/route.ts`:
- Around line 55-58: Update POST in the monitoring route to invoke the existing
shared rate-limit helper before processing or forwarding the monitoring
envelope, using the request/client identity expected by other public API routes.
Return the helper’s rate-limit response when the limit is exceeded, while
preserving the configuredDsn check and normal relay behavior.
In `@src/instrumentation-client.ts`:
- Around line 29-49: Add a rejection handler to the async initialization IIFE
guarded by __SENTRY_ENABLED__, catching failures from either dynamic import or
Sentry setup and recording a diagnostic error without allowing an unhandled
rejection. Keep the existing successful initialization and registerSentryClient
flow unchanged.
In `@tests/sentry-client.test.ts`:
- Around line 48-74: Update the SENTRY_DSN test setup and cleanup around
ORIGINAL_DSN and afterEach to also capture, clear, and restore
process.env.NEXT_PUBLIC_SENTRY_DSN. Ensure the “returns 404 when no DSN is
configured” test removes both DSN environment variables before loading the
route, while preserving the existing restoration behavior.
---
Nitpick comments:
In `@next.config.ts`:
- Around line 52-56: Align the __SENTRY_ENABLED__ value in the DefinePlugin
configuration with envSchema by trimming NEXT_PUBLIC_SENTRY_DSN before checking
it, and define the plugin only for the browser compiler using the available
webpack options. In readCappedText, enforce maxBytes using UTF-8 byte length in
the !reader fallback. When updating envSchema, prefer the Zod 4 top-level URL
validator consistently, then run check:production-readiness.
In `@tests/sentry-client.test.ts`:
- Around line 96-115: Add targeted negative-path tests alongside the existing
Sentry forwarding test, covering missing envelope newline, unparsable envelope
headers, non-string dsn values, and upstream fetch rejection or timeout. Assert
the route returns 400 for validation failures and 502 when fetch fails, using
the existing loadRoute, POST, envelope, and fetchMock test patterns.
🪄 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: d08cacdb-0197-4b02-8039-ea4f25cd1499
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
docs/site-map.mdnext.config.tspackage.jsonsrc/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
Resolves the CodeRabbit review threads on the client-Sentry PR: - Rate-limit the /api/monitoring tunnel: lightweight per-IP in-memory window (120/min → 429). It's an unauthenticated public POST, so a valid-envelope spammer could otherwise burn the project's Sentry ingest quota. Kept per- instance/best-effort (not the durable answer/upload limiter) since this is a telemetry relay, not a paid or state-changing path. - Guard the client Sentry init IIFE with .catch() so a failed dynamic import of @sentry/browser can never surface as an unhandled rejection. - Make the tunnel tests clear NEXT_PUBLIC_SENTRY_DSN too (the route reads SENTRY_DSN || NEXT_PUBLIC_SENTRY_DSN), so an ambient public DSN can't cause a false pass/fail; restore both in afterEach. Verified: typecheck / lint / format clean; tunnel + scrub + inert tests pass (7/7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
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
Summary
Adds browser error capture as the client counterpart to main's server-side
@sentry/nodecapture (src/lib/observability/error-capture.ts+src/instrumentation.ts). Uses@sentry/browser(not@sentry/nextjs) to avoid overlapping the existing server SDK. Fully inert untilNEXT_PUBLIC_SENTRY_DSNis set — the SDK is tree-shaken out of the client bundle (zero bytes shipped, CSP untouched).src/instrumentation-client.ts— gatedSentry.initbehind a build-time__SENTRY_ENABLED__literal (injected by DefinePlugin innext.config.ts) so webpack dead-code-eliminates the whole block when no public DSN is set;typeof-guarded so Turbopack dev (which skips the webpack plugin) doesn't throw. Events route through the same-origin tunnel.src/lib/observability/sentry-scrub.ts— mirrors the server privacy boundary: dropsevent.request(URL/query/headers),breadcrumbs, anduser, keeping only the error.beforeBreadcrumbreturns null.src/lib/observability/sentry-client.ts— lazy holder so error boundaries callcaptureClientException()without statically importing the SDK.src/app/api/monitoring/route.ts— same-origin tunnel (keeps CSPconnect-src 'self'). Hardened per prior review: DSN-match validation (no open relay), 1 MB envelope cap (Content-Lengthpreflight + streamed byte count → 413), and a 5s upstream timeout.captureClientExceptioninto the route + global error boundaries; addsNEXT_PUBLIC_SENTRY_DSN/ENVIRONMENTtoenv.ts(blank normalized toundefined).Verification
test— 248 files / 2308 tests pass, 0 failures (incl. new scrub / inert-no-op / tunnel 404·403·413·forward tests)typecheck,lint,format:check— cleanbuild— exit 0;@sentry/browserconfirmed absent from client chunks when DSN unset (__SENTRY_ENABLED__folded away);check:bundle-budgetwarn-only pass; client-bundle secret scan passsitemap:update— regenerated for the new/api/monitoringrouteClinical Governance Preflight
Touches privacy + adds an API route (error tracking):
sjrfecxgysukkwxsowpy)Notes
NEXT_PUBLIC_SENTRY_DSN(+ optionalSENTRY_DSNfor the server side, already wired). No runtime/behavior change and zero client-bundle cost until then.@sentry/browser, not@sentry/nextjs), so noSENTRY_AUTH_TOKENis involved.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Privacy & Security
Documentation
Bug Fixes