feat(reporter): add pluggable error reporter with noop default#242
Conversation
|
@Abdulrasaq1515 is attempting to deploy a commit to the ezedikeevan's projects Team on Vercel. A member of the Team first needs to authorize it. |
ezedike-evan
left a comment
There was a problem hiding this comment.
The implementation in lib/reporter.ts is clean and well-structured — the noop default, the configureReporter/reportError/resetReporter API, and the Sentry-compatible doc example are all good patterns. Tests are solid too.
Two things need fixing before this can merge:
1. No linked issue.
Closes #pluggable-error-reporter is a text string, not a GitHub issue number. Every PR in this project must close a tracked issue. If no issue exists yet for this feature, open one first, then reference it here with Closes #<number>. The PR template's "Linked issue" section is also left blank and the checklist is unchecked — please fill those in.
2. Auto-reporting in the SepError constructor is too aggressive.
// lib/stellar/errors.ts
reportError(this, { code, httpStatus }) // fires on every SepError constructionparseSepErrorBody is called whenever any anchor returns a non-2xx response — 404s, 400s, rate limits, validation errors. Wiring reportError unconditionally into the constructor means a Sentry-configured reporter would receive an alert for every routine anchor error. In production, a single user session hitting a throttled anchor could generate dozens of Sentry events.
The fix is straightforward: remove the auto-call from the constructor and instead call reportError at the specific call sites where the error is genuinely unexpected (e.g., a 5xx from the anchor, a timeout, a network failure). Let the caller decide what is worth reporting:
// In the caller that handles a real server error:
try {
await submitChallenge(...)
} catch (err) {
if (err instanceof SepError && err.httpStatus >= 500) {
reportError(err, { anchorDomain })
}
throw err
}The reporter module itself is good — just fix the issue link and the wiring point, and this is ready to merge.
4fe5a1e to
537f52c
Compare
- Add lib/reporter.ts with reportError, configureReporter, resetReporter - SepError constructor left clean; callers decide what is worth reporting - reportError called at call sites for genuine failures (5xx) only - Tests cover noop default, mock reporter, Sentry-compatible pattern, reset behavior, and call-site 5xx filtering pattern Closes ezedike-evan#184
|
@Abdulrasaq1515 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Closes #184
Summary
Linked issue
Closes #184
Changes
Testing notes
Automated
npm run typecheck· ⏳ not run / ✅ green / ❌ failingnpm run lint· ⏳ not run / ✅ green / ❌ failingnpm run test· ⏳ not run / ✅ green / ❌ failingnpm run build· ⏳ not run / ✅ green / ❌ failingNew / modified tests
Manual verification (if applicable)
Screenshots / recordings
Checklist
Correctness
npm run typecheckpassesnpm run lintpasses with zero new warnings (we run--max-warnings 0in CI)npm run testpasses; new behaviour has a testnpm run buildpassesData integrity
issue.md #005)isMock,// MOCK,// TODO: replace with real data, or commented-out real codestellar.tomlis publicly resolvable athttps://{domain}/.well-known/stellar.tomland containsTRANSFER_SERVER_SEP0024AbortControllertimeout is intact on anchor fetchescompleted | refunded | error) still stop the SWR loopSecurity & non-custody (see
docs/NON_CUSTODY.mdonce it lands).env.localis unchanged; new env vars are added to.env.exampleDocs
CHANGELOG.mdentry under[Unreleased]docs/*.mdupdated in the same PRdocs/ARCHITECTURE.mdupdated (file map, diagram, or invariants as applicable)docs/showcase/images/when relevant.env.example+ README env table updatedRelease hygiene
[ ]indocs/ROADMAP.mdis updatedBreaking changes
None.
For reviewers