Throw WebServiceError instances and preserve error causes#1725
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 57 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughIntroduces ChangesWebServiceError class and client integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a breaking change where errors thrown by WebServiceClient are now instances of a new WebServiceError class extending the built-in Error, rather than plain objects. This preserves existing properties (code, error, status, url) while adding support for standard Error properties like message and cause (for preserving underlying errors). The documentation, types, and tests have been updated accordingly. A review comment suggests hardcoding this.name = 'WebServiceError' instead of using this.constructor.name to prevent issues when the code is bundled and minified.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| options?: { cause?: unknown } | ||
| ) { | ||
| super(properties.error, options); | ||
| this.name = this.constructor.name; |
There was a problem hiding this comment.
Using this.constructor.name can be problematic if the library is bundled and minified (e.g., for serverless deployments like AWS Lambda or Cloudflare Workers), as minifiers will rename the class, resulting in err.name being a single letter (like e) instead of 'WebServiceError'. Hardcoding the string 'WebServiceError' ensures that err.name remains reliable regardless of minification.
| this.name = this.constructor.name; | |
| this.name = 'WebServiceError'; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/webServiceClient.ts (1)
144-166: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the original fetch rejection as
cause.Line 148 wraps non-
Errorrejections, and Lines 156/165 then store that wrapper ascause, discarding the original caught value despitecause?: unknownsupporting it.Proposed fix
} catch (err) { const error = err instanceof Error || err instanceof DOMException ? err : new Error(String(err)); if (error.name === 'TimeoutError') { throw new WebServiceError( @@ - { cause: error } + { cause: err } ); } throw new WebServiceError( @@ - { cause: error } + { cause: err } ); }🤖 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/webServiceClient.ts` around lines 144 - 166, Preserve the original fetch rejection value when building WebServiceError in webServiceClient.ts: the catch block in the fetch wrapper currently normalizes err into a new Error and passes that wrapper as the cause, which loses the original rejection for non-Error values. Update the error handling around the catch path so the thrown WebServiceError uses the original caught value as its cause while still deriving a safe message for FETCH_ERROR and TimeoutError handling, keeping the existing WebServiceError constructor usage in place.
🤖 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/errors.spec.ts`:
- Around line 41-53: Add an explicit negative test in the WebServiceError suite
to cover the optional cause contract when no cause is passed. In
src/errors.spec.ts, extend the existing WebServiceError tests by creating an
instance without the second options argument (or with an empty options object),
then assert that err.cause is undefined and that any serialized form does not
include a cause field. Keep the new assertion alongside the existing “preserves
the underlying cause” case so both presence and absence are covered.
In `@src/webServiceClient.spec.ts`:
- Around line 16-33: `expectError` only checks the core WebServiceError fields,
so `cause` behavior is never asserted; update this helper to accept an explicit
expected `cause` mode/value and verify it on the `WebServiceError` instance,
then adjust all call sites in `src/webServiceClient.spec.ts` to cover both cases
where `cause` should be present and where it should be absent. Use the existing
`expectError` helper and `WebServiceError` symbol to keep the assertions
centralized and ensure undefined handling is tested.
In `@src/webServiceClient.ts`:
- Around line 210-229: The error parsing in `src/webServiceClient.ts` only
checks truthiness before creating `WebServiceError`, so invalid payloads can
still slip through. In the `response.json()` handling inside the error निर्माण
path, update the validation in the `data` block to verify `code` and `error` are
actually strings before constructing `new WebServiceError`. If either field is
missing or not a string, return the `invalidResponseBody` case instead; keep the
existing `catch` and final `WebServiceError` construction flow unchanged.
---
Outside diff comments:
In `@src/webServiceClient.ts`:
- Around line 144-166: Preserve the original fetch rejection value when building
WebServiceError in webServiceClient.ts: the catch block in the fetch wrapper
currently normalizes err into a new Error and passes that wrapper as the cause,
which loses the original rejection for non-Error values. Update the error
handling around the catch path so the thrown WebServiceError uses the original
caught value as its cause while still deriving a safe message for FETCH_ERROR
and TimeoutError handling, keeping the existing WebServiceError constructor
usage in place.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 745cab5f-84e0-4e54-8b7d-7b3bc4ad9379
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/types.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
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/errors.ts`:
- Around line 50-53: The WebServiceError prototype name assignment makes name
enumerable, unlike built-in Error behavior. Update the WebServiceError prototype
setup in src/errors.ts to use Object.defineProperty on WebServiceError.prototype
so name stays non-enumerable while still preserving the literal class name for
minification safety. Keep the change scoped to the WebServiceError prototype
initialization.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b80df35-546b-432f-949f-23dfa563c353
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/types.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
| // Set `name` on the prototype rather than in the constructor. Using a string | ||
| // literal keeps the name correct under minification, and keeping it off the | ||
| // instance means it stays out of `JSON.stringify` output. | ||
| WebServiceError.prototype.name = 'WebServiceError'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
class AssignedNameError extends Error {}
AssignedNameError.prototype.name = 'AssignedNameError';
class DefinedNameError extends Error {}
Object.defineProperty(DefinedNameError.prototype, 'name', {
value: 'DefinedNameError',
writable: true,
configurable: true,
});
console.log('Error.prototype.name:', Object.getOwnPropertyDescriptor(Error.prototype, 'name'));
console.log('AssignedNameError.prototype.name:', Object.getOwnPropertyDescriptor(AssignedNameError.prototype, 'name'));
console.log('DefinedNameError.prototype.name:', Object.getOwnPropertyDescriptor(DefinedNameError.prototype, 'name'));
NODERepository: maxmind/GeoIP2-node
Length of output: 514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file section with line numbers.
sed -n '1,140p' src/errors.ts | cat -nRepository: maxmind/GeoIP2-node
Length of output: 3515
Make WebServiceError.prototype.name non-enumerable
Direct assignment makes name enumerable on the prototype, so for...in on instances will surface it unlike built-in Error. Use Object.defineProperty here instead.
🤖 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/errors.ts` around lines 50 - 53, The WebServiceError prototype name
assignment makes name enumerable, unlike built-in Error behavior. Update the
WebServiceError prototype setup in src/errors.ts to use Object.defineProperty on
WebServiceError.prototype so name stays non-enumerable while still preserving
the literal class name for minification safety. Keep the change scoped to the
WebServiceError prototype initialization.
The web service client rejected with plain objects, which meant errors were not Error instances and the underlying cause (for example, the network error behind a FETCH_ERROR) was discarded. Introduce a WebServiceError class that extends Error and carries the existing code/error/status/url properties as own enumerable properties, plus the standard cause. Capture the cause at every throw site. Export WebServiceError and the WebServiceClientError type. STF-803 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A FETCH_ERROR previously surfaced only "TypeError - fetch failed", with the real reason (DNS failure, refused connection, TLS error, etc.) hidden in the cause. Consumers that log only `code`/`error` never saw it. Append the underlying cause's message to the error string so the reason is visible without inspecting `cause`, which is still attached. STF-803 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
The
WebServiceClientrejected with plain objects ({ code, error, status?, url }), which meant:Errorinstances (err instanceof Errorwasfalse, no stack trace).causewas discarded. A Nodefetchfailure throwsTypeError: fetch failedwhose real reason (DNS failure,ECONNREFUSED, TLS error, etc.) lives inerror.cause, but the client surfaced onlyFETCH_ERROR: TypeError - fetch failed.This introduces a
WebServiceErrorclass that extendsErrorand carriescode/error/status/urlas own enumerable properties (so existing field access andJSON.stringifyoutput are preserved), plus the standardcause. The cause is now captured at every throw site (fetch failure, timeout, response-body parse, bad-server-response parse).WebServiceErrorand theWebServiceClientErrortype are now exported from the package.Backward compatibility
err.code,err.error,err.status,err.url, and existingJSON.stringify(err)keys.err instanceof Error,err.message(===err.error),err.cause, a stack trace.Errorinstance rather than a plain object.Testing
src/errors.spec.tscovering theWebServiceErrorclass.instanceof WebServiceError, the preserved fields, and the propagatedcause.npm run lint, fulljestsuite (101 tests), and prettier all pass.This is the
geoip2-nodecompanion to the same change inminfraud-api-node(maxmind/minfraud-api-node#1902).Part of STF-803.
Summary by CodeRabbit
Breaking Changes
WebServiceError(anError) instead of plain objects.code,error/message, optionalstatus, optionalurl) and preserves the underlying error viacause.New Features
WebServiceErroris now exported for direct use.WebServiceClientErroris also exported, with an optionalcause.Documentation
Tests
WebServiceErrorproperties andcausehandling.