chore(sentry): capture console errors only, stop reporting local builds and two expected 4xx - #3268
Conversation
peanut-ui sends ~31k Sentry error events a month and every accepted event is
billed regardless of level, so the cheapest wins are the events nobody reads.
captureConsoleIntegration listened on `warn`, which bills a console.warn like
an exception. Warn-level output here is handled conditions, not defects: the
Radix DialogTitle notice (2,158 in 90 days), a missing icon name (1,420), the
tokenPrice fallback (3,320). A message captured without an Error also arrives
with a minified title ("d", "Module.d", "iE"), so ~3k a month were unreadable.
Every build also reported into the production project. Ad-hoc PR previews
(2,649 error + 1,374 warning in 30 days) and local builds were billed and
mixed into production issues that nobody triages. Only production, staging and
native now init at all.
Server tracing ran at 100% while the client ran at 10%. Match them.
Each of these is an outcome the UI handles and shows the user, so an error in Sentry says only that the product worked. - /invites/validate 409: the code resolves to a campaign only, which validateInviteCode already reads as a success. - /bridge/exchange-rate 429: the upstream quota doing its job. Every mounted rate hook retries, so one fault arrived as many events. - /perks/pending 401: a stale session. The skip rule already covered it, but the call site's own console.error re-created the event under a minified title — that is the "Expected stale-session 401" known-noise row. 5xx on all three still reports, pinned by tests here and on /manteca/qr-payment/init.
Sentry bills a captureMessage like a crash. None of these is a fault: - The native transport canary already wrote the same verdict to PostHog as native_transport_canary_failed; the Sentry message was a paid duplicate. The per-probe error text that used to ride only on the Sentry event now rides on the PostHog one, so nothing is lost and the daily sampling it needed is gone. - Passkey debug info (1,558 in 90 days at level info) is device capability state. In PostHog it is queryable against the passkey funnel. The captureException for a real failure stays. - The OneSignal subscription snapshot reported its own read failure (~410 a month) as a stackless message. It now rides the same PostHog event as the snapshot itself, under snapshot_error.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: peanutprotocol/peanut-ui/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code-analysis diffPainscore total: 8459.51 → 8461.25 (+1.74) 🆕 New findings (11)
✅ Resolved (11)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
Scope correction after checking the current volumes. The native transport canary and the OneSignal subscription snapshot are restored untouched: both already write to PostHog, and their remaining Sentry volume (14 and ~407 a month) comes from the retired canary in old native binaries, which no code change can stop — a Sentry inbound filter will. That leaves the passkey debug capture, which is live (SignTestTransaction calls it on a failed test transaction) but reported device capability state as a Sentry message at level info, 1,558 in 90 days. It is console-only now; the captureException for a real failure is unchanged.
Three comments justified a console.info by 'captureConsoleIntegration listens on warn'. That is no longer true, and a comment stating a false fact is worse than no comment. The decisions they explain are unchanged.
|
/chip review |
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Solid telemetry-hygiene change: env gating, console-error-only capture, skip rules and comment corrections are all correct and pinned by tests both ways. One major on the /bridge/exchange-rate 429 skip: it silences the escalation channel for the standing open FX-stampede P2, and the rule's stated justification ("peanut-api already reports the upstream cause") does not hold for that defect — the 429 comes from peanut-api's own global limiter, which nothing reports server-side.
Findings
-
MAJOR · src/utils/sentry.utils.ts:36 · 429 skip rule deletes the escalation signal for the open FX-stampede P2, on a justification that is wrong for the stampede case
The standing open P2 (same-key FX request stampede, PEANUT-UI-T1M/T1R/T1N/T1P/T1Q, ops/monitor/sentry-triage.md) is detected and escalated purely by the volume of client-captured 429s on /bridge/exchange-rate — the runbook decision ignores the family "at 5 events/60m", i.e. volume is the escalation channel. This skip rule removes it entirely, and also from PostHog (findSkipRule runs in reportNonOkResponse before captureMessage, and posthogErrorMirror mirrors only Sentry events). The comment's justification — "peanut-api already reports the upstream cause" — is accurate for Bridge 5xx but not for the stampede: its 429 is generated by peanut-api's own global @fastify/rate-limit (app.ts, 300/min/IP keyed on CF-Connecting-IP), which produces a plain 429 reply with no onExceeded hook and no error-path Sentry capture, so nothing server-side reports it. Only the Rewards-endpoint spill (429s on endpoints not in the skip table) keeps any signal, and that is indirect and was not the filed fingerprint. Concrete failure: the stampede recurs in production after this ships, the T1M family groups never form again, the "ignore until 5 events/60m" decision can never re-open, and the open P2 loses its only escalation path until someone notices Rewards 429s by chance. Fix: drop this one entry from the skip table until the keyed single-flight fix for no-cache.ts lands (the other two skips have no such twin), or before merge arm an equivalent signal — pin the runbook to escalate on the Rewards-spill 429 groups / an API-side limiter counter and record the watermark in sentry-triage.md and known-noise.md (the docs sweep is already listed as a follow-up; it must cover this decision, not just the invites/perks rows). -
MAJOR · src/utils/sentry.utils.ts:36 · [claude-opus] New /bridge/exchange-rate 429 skip silences the one alert for a wrong withdrawal minimum; its stated premise is false
The added rule{ pattern: /\/bridge\/exchange-rate/, statuses: [429] }is justified by two claims in its comment, both of which the code contradicts.
(1) "the UI keeps the last rate and retries". It does not. useGetExchangeRate (src/hooks/useGetExchangeRate.tsx:41-53) catches the failure inside queryFn and returns '1'. Because the function resolves rather than throws, react-query treats it as a success — retry: 3 never fires — and '1' is cached for the 5-minute staleTime.
(2) "peanut-api already reports the upstream cause". Not for a 429: the route catches Bridge failures and returns 500 (peanut-api-ts src/routes/bridge/exchange-rates.ts:72-74). A 429 on this path comes from Peanut's own global @fastify/rate-limit (peanut-api-ts src/app.ts:136), which reports no upstream cause at all.
Consequence, on a money screen: bankWithdrawMinUsd computes Math.ceil(localMin / rate) (src/features/withdraw/amount-validation.ts:63) from that rate, and it feeds both the displayed amount-step minimum (src/features/withdraw/useWithdrawRootFlow.ts:165) and the submit-side re-check (src/features/withdraw/useBridgeOfframpFlow.ts:118). With rate = '1' the Mexican minimum becomes ceil(50/1) = $50 instead of ~$3, and GB becomes $3 instead of ~$4 (under-enforced, so Bridge rejects). The isMinReady guard does not catch it — parseFloat('1') > 0 passes, so submission is not gated. This differs from the existing /tokens/price 404 entry the author cites as precedent, whose comment correctly notes it is "a degraded display, never a wrong number"; here it is a wrong number.
Fix: either drop 429 from this rule, or make the fallback honest first — have getExchangeRate/useGetExchangeRate surface the failure (throw so retry applies, or return null so bankWithdrawMinNeedsRate callers keep the submit disabled) and correct the comment to say the 429 originates from our own limiter, not upstream.
Checked clean
- head SHA verified (git rev-parse HEAD) and diff read in full against base d035b65
- Env gating: isSentryReportingEnvironment logic traced for production/staging (dev-branch preview)/preview/native/local in sentry-env.ts, sentry-init.ts, sentry.server.config.ts, sentry.edge.config.ts; native client init in instrumentation-client.ts is capacitor-only so 'native' stays reporting; client test exclusion of CAPACITOR_BUILD/PERF_BARE unchanged
- Console capture narrowed to ['error'] on web/server/edge; native was already ['error'] (instrumentation-client.ts untouched); perks.ts console.error→console.info now safe on both web and native capture sets
- Skip-rule tests pinned both directions (409/429/401 skipped; 500s still reported, including /manteca/qr-payment/init 500) in sentry.utils.test.ts
- /invites/validate 409 skip verified against the known-noise row (PEANUT-UI-T5N, benign-PROVEN 2026-09-11) which requested exactly this rule, and validateInviteCode's typedCampaignOnly handling
- /perks/pending 401 skip matches the pre-existing skip-table row; the call-site leak fix follows the reportNonOkResponse precedent
- passkeyDebug: captureMessage removal leaves the catch-path captureException and the SignTestTransaction caller intact
- tracesSampleRate: server 1→0.1 matching the client (sentry-init.ts:61); edge deliberately left at 1 — consistent with the described trade-off
- Sibling peanut-api-ts read: /bridge/exchange-rate handler and global rate limiter (app.ts rateLimit register) — no onExceeded hook, no error-path capture of limiter 429s, confirming finding 1
- CI at head: analyze/refs/review/bot-approval green; Deploy-Preview and capture_after suites still in progress at review time
- Two duplicated sentry.utils.ts files, near-identical server/edge configs and the console.error(non-Error) pattern are pre-existing and explicitly flagged by the author as follow-ups — not counted as findings
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 1 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
The usual first reviewer was out of plan, so this review was done by openrouter/z-ai/glm-5.3.
Exact head: c8473e140971 · Context: repo, mono, sibling-repo · Took 3m
Two reviewers landed on the same rule independently, and the code agrees with them. useGetExchangeRate swallows the failure and returns a rate of '1' (retry:3 never fires, because resolving is not throwing), bankWithdrawMinUsd divides by it, and a Mexican bank withdrawal then shows a $50 minimum instead of about $3 with nothing gating submission. The 429 is the only alert for that wrong number, and for the open FX-stampede P2 whose runbook entry escalates on exactly this volume. The rule's justification was also wrong: the 429 comes from our own global rate limiter in peanut-api, which reports nothing server-side. Replaced with a comment saying why it must not be skipped and a test that fails if anyone adds the rule back. The invites/validate 409 and perks/pending 401 skips stand — neither has a money screen behind it.
|
/chip review |
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Telemetry-cost hygiene change, reviewed at head f96453b. Both prior findings (the /bridge/exchange-rate 429 skip) are fixed in this head commit: the rule is removed, a comment at the same spot explains why the 429 must keep reporting, and it.each([429, 500]) pins reporting so re-adding the rule fails the suite. The remaining suppressions match the API contract: the only 409 peanut-api returns on /invites/validate is the campaign-only outcome, and both client consumers already treat it as success. No new findings.
Checked clean
- Prior findings P1/P2: verified at head that the /bridge/exchange-rate 429 skip rule is gone from SKIP_REPORTING (src/utils/sentry.utils.ts), the replacement comment documents the useGetExchangeRate '1'-rate fallback and the FX-stampede escalation, and src/utils/tests/sentry.utils.test.ts pins both 429 and 500 as still reported
- New /invites/validate 409 skip: checked the sibling peanut-api-ts checkout — the route's only 409 is the campaign-only response (src/routes/invite.ts:445); both client call sites (src/app/actions/invites.ts:22, src/services/invites.ts:41) read it as success via isTypedCampaignOnlyInviteResponse, so no reachable failure is suppressed today. Note: unlike the qr-payment/init 409 rule, this one is not errorCodes-scoped (the campaign-only body carries no
errorfield, so the mechanism cannot scope it) — a future, different 409 on this route would be silently suppressed - Environment gating: isSentryReportingEnvironment() composes with the existing inferSentryEnvironment() tagging logic; staging (dev-branch preview) still reports; client/server/edge all gate consistently; the capacitor-preview-reported-as-native gap is acknowledged in the PR body and out of scope
- Console capture narrowed to ['error'] on client, server and edge; instrumentation-client.ts was already ['error'] — passkeyDebug keeps its captureException for real failures and perks.ts 5xx still reports via fetchWithSentry's captureMessage
- tracesSampleRate 1 -> 0.1 on server only; client was already 0.1 — no behavior beyond sampling cost
- CI at this head: no failing checks (refs, bot-approval, analyze, review green; deploy/build suites in progress). Tests not run locally: the detached worktree has no node_modules; the PR's local run claim (8,358 passed) and the new pinned tests rest on CI
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
The usual first reviewer was out of plan, so this review was done by openrouter/z-ai/glm-5.3.
Exact head: f96453b34c22 · Context: repo, peanut-api-ts sibling checkout · Took 3m
🖼 Visual diff — 3 screens moved5 of 144 shots changed · 139 identical · baseline
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
abalinda
changed the title
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Re-reviewed at the same head SHA after the P1/P2 fix round. Both prior findings (the /bridge/exchange-rate 429 skip) are fixed in head commit f96453b: the rule is removed, a comment at the same spot in SKIP_REPORTING explains why the 429 must keep reporting, and it.each([429, 500]) pins reporting so re-adding the rule fails the suite. The remaining suppressions match the API contract — the only 409 peanut-api returns on /invites/validate is the campaign-only outcome, which both client consumers already treat as success. No new findings.
Checked clean
- Prior findings P1/P2: verified at head that the /bridge/exchange-rate 429 skip rule is gone from SKIP_REPORTING (src/utils/sentry.utils.ts), the replacement comment documents the useGetExchangeRate '1'-rate fallback and the FX-stampede escalation, and src/utils/tests/sentry.utils.test.ts pins both 429 and 500 as still reported
- New /invites/validate 409 skip: checked the sibling peanut-api-ts checkout — the route's only 409 is the campaign-only response (src/routes/invite.ts:445); both client call sites (src/app/actions/invites.ts:22, src/services/invites.ts:41) read it as success via isTypedCampaignOnlyInviteResponse, so no reachable failure is suppressed today. Unlike the qr-payment/init 409 rule this one is not errorCodes-scoped (the campaign-only body carries no
errorfield, so the mechanism cannot scope it) — a future, different 409 on this route would be silently suppressed - Environment gating: isSentryReportingEnvironment() composes with the existing inferSentryEnvironment() tagging logic; staging (dev-branch preview) still reports; client/server/edge all gate consistently; the capacitor-preview-reported-as-native gap is acknowledged in the PR body and out of scope
- Console capture narrowed to ['error'] on client, server and edge; instrumentation-client.ts was already ['error'] — passkeyDebug keeps its captureException for real failures and perks.ts 5xx still reports via fetchWithSentry's captureMessage
- tracesSampleRate 1 -> 0.1 on server only; client was already 0.1 — no behavior beyond sampling cost
- CI at this head: no failing checks. Tests not run locally: the detached worktree has no node_modules; the PR's local run claim and the new pinned tests rest on CI
Security review by moonshotai/kimi-k3: 0 finding(s), marked with the model name. It reads the diff only and answers only security, privacy and money, so treat its findings as advice.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
The usual first reviewer was out of plan, so this review was done by openrouter/z-ai/glm-5.3.
Exact head: f96453b34c22 · Context: repo, peanut-api-ts sibling checkout · Took 1m
…sage Two reversions, both to avoid losing a diagnostic somebody depends on. Preview was going to stop initialising the SDK. It must not: the OTA liveness proof in ops/native-ota-envless-bundle-rca.md reads preview and canary events out of Sentry, so a dark preview takes that check with it. Only `development` is disabled now — which still beats the NODE_ENV guard it replaces, because a `next build` on a laptop has NODE_ENV=production and reported as production. passkeyDebug goes back to what `dev` has, captureMessage included. Support reads the collected navigator/credentials data on registration and test-transaction failures, and ~300 events a month is a fair price for it. Error sampling and the console-error-only setting are unchanged and apply everywhere; tracesSampleRate is a flat 0.1 on client and server.
abalinda
changed the title
|
/chip review |
|
English · Español · Español (Argentina) · Português (Brasil) After merge: f3154a9 → 12bd900. Capture complete in all locales. |
|
/chip review Head |
Summary
peanut-ui sends about 31,000 Sentry events a month (last 30 days: production 10,561 error + 4,482 warning + 284 info; native 3,347 error + 3,428 warning + 2,958 info + 114 fatal; preview 2,649 error + 1,374 warning; development 336; CSP reports 1,165). Sentry bills every accepted event whatever its level, and archiving an issue does not stop the bill. This PR removes the events nobody reads.
1. Console capture listens on
erroronly —src/utils/sentry-init.ts(web client),sentry.server.config.ts,sentry.edge.config.ts. Aconsole.warncost the same as an exception, and warn-level output here is handled conditions, not defects: the Radix "DialogContentrequires aDialogTitle" notice (2,158 in 90 days), "Icon "upload" not found" (1,420), "error fetching tokenPrice, falling back to tokenDenomination" (3,320). A message captured with no Error object also arrives under a minified title ("d", "s", "Module.d", "iE"), so roughly 3,000 a month were unreadable.attachStacktraceandposthogErrorMirror()are unchanged. The native client (instrumentation-client.ts) was already['error'].2. Local builds stop reporting. New
isSentryReportingEnvironment()insrc/utils/sentry-env.tsgates client, server and edge on the inferred environment rather than onNODE_ENV. That is stricter than the guard it replaces: anext buildon a laptop runs withNODE_ENV=productionand noVERCEL_ENV, so it reported as production and was billed there — 336 events in 30 days, now zero.previewdeliberately keeps reporting. The OTA liveness proof inops/native-ota-envless-bundle-rca.mdreads preview and canary events out of Sentry, so a dark preview would take that check with it. The preview families this PR was expected to remove — the "OneSignal configuration missing" warnings (1,116 in 30 days), thereading 'waiting'TypeError (2,080), "[sumsub] failed to load websdk script" (388) — therefore stay. Change 1 still removes the warn-level share of them.3. Server
tracesSampleRate1 → 0.1, matching the client.4. Two expected non-2xx responses stop reporting (
findSkipRule,src/utils/sentry.utils.ts):/invites/validate409 — the code resolves to a campaign only, whichvalidateInviteCodealready reads as a success (typedCampaignOnly)./perks/pending401 — a stale session. The skip rule already covered the fetch layer; the leak wassrc/services/perks.tscallingconsole.errorwith a non-Error at the call site, re-creating under a minified title the eventfetchWithSentryhad just suppressed. That line isconsole.infonow, following the precedent already written intoreportNonOkResponse.5xx on both still reports, pinned by tests.
/bridge/exchange-rate429 was in this list and has been taken back out — see Design notes. It reports, and a test now fails if anyone adds the rule back.Task
None — Sentry cost hygiene, no tracking task exists.
Risks
findSkipRuleruns inreportNonOkResponsebeforeSentry.captureMessage, andposthogErrorMirror()mirrors Sentry events into PostHog — so a skipped response disappears from both, not just from Sentry.ops/monitor/known-noise.md:226is a PostHog-sourced/invites/validatebucket that assumes otherwise. True for the two remaining skips; both are benign.console.warnnow reaches the browser console and nothing else. Nothing in the named noise sources is in that class.next buildno longer reports. It used to file intoproduction, which is the reason to stop — but if you were relying on a local production build to surface an error in Sentry, it will not any more. Preview, staging, production and native all still report.Design notes / accepted trade-offs
/bridge/exchange-rate429 was dropped from the skip table after review. Two reviewers landed on it independently and the code agreed with them.useGetExchangeRate(src/hooks/useGetExchangeRate.tsx:41-53) swallows the failure and returns a rate of'1'— it resolves, so react-query counts it a success andretry: 3never fires, and'1'is cached for the 5-minutestaleTime.bankWithdrawMinUsdthen computesMath.ceil(50/1), so a Mexican bank withdrawal shows a $50 minimum instead of about $3, andparseFloat('1') > 0passesisMinReady, so submission is not gated either. That 429 is the only alert for a wrong number on a money screen — and for the open FX-stampede P2 (ops/monitor/sentry-triage.md:522) whose runbook entry escalates on exactly this volume. The rule's stated justification was also wrong: the 429 comes from peanut-api's own global@fastify/rate-limit, which reports nothing server-side. It is replaced by a comment at the same spot saying why it must not be skipped, plusit.each([429, 500])('still reports …')so re-adding it fails the suite. The honest follow-up is the keyed single-flight fix insrc/utils/no-cache.ts, not a Sentry rule./invites/validate409 rule is noterrorCodes-scoped, unlike theqr-payment/init409 rule beside it. It cannot be: the campaign-only response body carries noerrorfield for the mechanism to match on. Today the route returns 409 for that outcome only (peanut-api-ts src/routes/invite.ts:445), so nothing reachable is suppressed — but a future, different 409 on this route would go quiet. Scoping it needs an error code on the backend response first./invites/validate400 and/perks/pending401 were already in the skip table (added 2026-07-08 and 2026-02-05), so no duplicate rule was added for them.claimPerk's neighbouringconsole.erroris left alone on purpose: a failed claim is money-adjacent and worth reporting.console.infowith "captureConsoleIntegration listens on['error','warn']", which this PR makes false. Corrected in place; the decisions they explain are unchanged.beforeSendHandlerredaction,isPaymentNetworkExplorerPathand the CSP report route are untouched.Smells: adds none. Reveals three, all left for follow-ups — flagged, not changed:
sentry.utils.tsfiles — one at the repo root, one insrc/utils/— and the second imports from the first (../../sentry.utils). Same name, related work, no way to guess which is which.sentry.server.config.tsandsentry.edge.config.tsare now byte-identical apart fromtracesSampleRate. Shared options would remove the copy.console.error(<non-Error>, …)is a repo-wide pattern, and it is what produces the minified Sentry titles ("d", "Module.d", "iE").src/utils/to-error.tsalready exists for it; a lint rule is the systemic fix. This PR only touches the one instance it was sent to fix.Docs follow-ups (separate, NOT in this PR)
A mono docs sweep found runbook drift this creates. None of it belongs in a code PR:
ops/monitor/known-noise.md— rows:47(invites 409),:59(perks 401),:65and:133(buckets that exist only becausewarnwas captured),:81(canary baseline).ops/monitor/sentry-triage.md— the passkey-debug decisions at:279, :542, :797recommend exactly this removal; the preview/development families at:140, :156, :1007are structurally gone.skills/monitor/SKILL.md,ops/monitor/log.md,ops/post-release-monitoring.md— the sweep diffs unresolved counts against a baseline this PR moves; needs a watermark so the next sweep reads "telemetry moved", not "telemetry improved".engineering/testing/README.md:203-221,engineering/native/crash-analytics.md:3-15— note that a local build no longer initializes Sentry, and thatconsole.warnis no longer captured.Legal: no impact.
content/legal/privacy/en.mdwas read end to end. Nothing new leaves our systems — this PR only sends less to Sentry, adds no processor, no data category, no retention or deletion change. Sentry stays named at:77and stays in use.QA
npm test— 680 suites, 8,359 passed (5 skipped).npm run typecheck,pnpm prettier --check .,npm run buildgreen locally.src/utils/__tests__/sentry.utils.test.tspins the rules both ways:/invites/validate409 and/perks/pending401 are skipped;/invites/validate500,/manteca/qr-payment/init500 and/bridge/exchange-rate429 and 500 still report.src/utils/__tests__/sentry-init.test.tsandsrc/features/payment-network-explorer/__tests__/sentryServerEdge.test.tsassert on client, server and edge that apreviewbuild does callSentry.initand a local build (noVERCEL_ENV) does not.Screenshots: N/A (no visible change — telemetry configuration only).
🤖 Generated with Claude Code