fix(sentry): stop double-counting fetch failures, add missing DrawerTitle#2448
Open
innolope-dev wants to merge 1 commit into
Open
fix(sentry): stop double-counting fetch failures, add missing DrawerTitle#2448innolope-dev wants to merge 1 commit into
innolope-dev wants to merge 1 commit into
Conversation
…itle captureConsoleIntegration promotes every console.error/warn into a Sentry event, and fetchWithSentry both captures the underlying failure AND re-throws a ServiceUnavailableError wrapper that gets captured again by global handlers. One API timeout could produce three Sentry events. - sentry.utils: ignore ServiceUnavailableError (always a wrapper whose cause was already captured at the fetch site) — PEANUT-UI-QDJ - fetchWithSentry: console.info instead of console.error for the raw fetch rejection; the explicit captures below carry the full context - useTokenPrice: fallback log at console.info — the fallback is graceful and the failure is already reported — PEANUT-UI-MH5 - BadgeStatusDrawer: badge name becomes the DrawerTitle, fixing the Radix 'DialogContent requires a DialogTitle' a11y warning — PEANUT-UI-MJS
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe changes update shared Sentry filtering and console logging behavior, adjust token price error handling, and replace the badge drawer’s plain heading with ChangesSentry observability
Badge drawer semantics
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: dependency version conflict. Check your lock file or package.json. Comment |
Contributor
Code-analysis diffPainscore total: 6208.86 → 6209.3 (+0.44) 🆕 New findings (3)
✅ Resolved (3)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@sentry.utils.ts`:
- Around line 41-46: Restrict the alreadyReported rule in sentry.utils.ts so
shouldIgnoreError ignores only the fetchWithSentry wrapper’s exact
ServiceUnavailableError exception type, not events whose message, value, type,
or culprit merely contains that text. Update the rule using the existing
error-pattern configuration mechanism and add regression coverage for both the
wrapper match and unrelated containing messages if tests are available.
🪄 Autofix (Beta)
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
Run ID: 35a31120-3ad6-4871-8247-b7cc312339c4
📒 Files selected for processing (4)
sentry.utils.tssrc/components/Badges/BadgeStatusDrawer.tsxsrc/hooks/useTokenPrice.tssrc/utils/sentry.utils.ts
Comment on lines
+41
to
+46
| // fetchWithSentry wrapper errors: the underlying timeout/network/HTTP | ||
| // failure is already captured at the fetch site with full context, so the | ||
| // re-thrown ServiceUnavailableError bubbling to global handlers (or being | ||
| // console.error'd by a consumer) would only double-count it (PEANUT-UI-QDJ). | ||
| alreadyReported: ['ServiceUnavailableError'], | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the ServiceUnavailableError ignore rule.
shouldIgnoreError applies patterns with substring matching across the message, exception value, type, and culprit. This suppresses unrelated events that merely mention ServiceUnavailableError, despite the comment describing an exact wrapper match. Match the exception type explicitly (or add regression tests covering both the wrapper and unrelated messages containing this text).
🤖 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 `@sentry.utils.ts` around lines 41 - 46, Restrict the alreadyReported rule in
sentry.utils.ts so shouldIgnoreError ignores only the fetchWithSentry wrapper’s
exact ServiceUnavailableError exception type, not events whose message, value,
type, or culprit merely contains that text. Update the rule using the existing
error-pattern configuration mechanism and add regression coverage for both the
wrapper match and unrelated containing messages if tests are available.
Collaborator
Author
|
Reconciliation note — this PR overlapped with two others opened later the same day; they've been rescoped so nothing conflicts:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Three of our top-volume Sentry issues are amplification, not new signal.
captureConsoleIntegration({levels: ['error','warn']})turns everyconsole.error/console.warninto a Sentry event, andfetchWithSentryboth captures the underlying failure at the call site and re-throws aServiceUnavailableErrorwrapper that global handlers capture again. A single API timeout could produce up to three events:captureException(kept — this is the real signal, e.g. PEANUT-UI-QHG)console.errorabout the same failure — PEANUT-UI-MH5, 2.5k eventsSeparately, PEANUT-UI-MJS (1.7k events, 854 users) is Radix's
DialogContent requires a DialogTitlea11y warning:BadgeStatusDraweris the only drawer without aDrawerTitle.Fix
ServiceUnavailableErrorto the ignore list — it is always a wrapper created byfetchWithSentrywhose cause was already captured with full request context at the fetch site.console.info(noterror) — the explicitcaptureExceptioncalls right below carry the full context; the error-level log only produced a duplicate event via captureConsole.console.info— the fallback is graceful and the underlying failure is already reported. ThecaptureExceptionstays for genuinely new (non-fetch) errors; beforeSend now drops the wrapper case.DrawerTitle(real a11y fix, not a suppression). Note:DrawerTitleaddsleading-none tracking-tightfrom the shared component — minor typographic delta on this one-line title.Deliberately NOT suppressed
/users/me,/rain/cards,/tokens/price) — 10s timeouts hitting 1.7k users is genuine API-latency signal.Testing
pnpm typecheckclean, related jest tests green, prettier applied.Summary by CodeRabbit
Bug Fixes
UI Improvements