Throw WebServiceError instances and preserve error causes#1902
Conversation
|
Warning Review limit reached
More reviews will be available in 26 minutes and 47 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)
📝 WalkthroughWalkthroughAdds ChangesWebServiceError Error Class
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 updates the web service client to throw errors as WebServiceError instances (which extend the built-in Error class) instead of plain objects, preserving the original error properties and adding support for the standard cause property to hold underlying errors. The WebServiceError and ArgumentError classes, along with the WebServiceClientError type, are now exported. The documentation and test suites have been updated accordingly. One piece of feedback suggests explicitly setting this.name to 'WebServiceError' instead of using this.constructor.name to avoid issues when the code is minified or bundled.
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 to set this.name is prone to breaking when the code is minified or bundled (e.g., using Webpack, Terser, or esbuild), as the class name WebServiceError may be renamed to a short, single-character name (like e or t). This can break error-handling logic that relies on checking error.name === 'WebServiceError'.\n\nIt is safer and more robust to explicitly assign the string literal 'WebServiceError' to this.name.
| 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: 2
🤖 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 `@README.md`:
- Around line 163-167: The per-code `cause` note in the error code list is
incomplete and can mislead readers into thinking it only applies to
`FETCH_ERROR`. Update the list near the documented error codes so
`NETWORK_TIMEOUT` and `INVALID_RESPONSE_BODY` are also annotated with the
preserved `cause`, or remove the single-code inline comment and rely on the
broader note already given earlier in the README.
In `@src/errors.ts`:
- Around line 49-53: The WebServiceError constructor currently assigns
`this.name` directly, which makes `name` an own enumerable property and changes
`JSON.stringify` output. Update `WebServiceError` to keep `name` non-enumerable
when setting it, and apply the same fix consistently in `ArgumentError` if it
uses the same pattern, so serialized errors remain limited to `code`, `error`,
`status`, and `url`.
🪄 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: 3b493e53-15e4-4cfa-b6fd-bd8937343228
📒 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: 2
🤖 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 49-52: The WebServiceClientError constructor is always defining an
own status property via this.status = properties.status, which makes status
present even when it should be omitted at runtime for FETCH_ERROR and
NETWORK_TIMEOUT. Update the WebServiceClientError initialization logic in
src/errors.ts so status is only assigned when a real value exists, keeping the
observable shape aligned with the optional contract while preserving the
existing handling of code, error, and url.
- Around line 4-9: Expose the structured error contract on ArgumentError in
src/errors.ts before it is re-exported through src/index.ts: update the
ArgumentError class so instances include the expected code and error fields, and
add an optional url field when applicable, alongside the existing name/message
setup. Make the constructor initialize these public properties consistently so
consumers can reliably inspect the error shape after importing ArgumentError.
🪄 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: 8d291b52-4e7d-4263-b500-89e59079f423
📒 Files selected for processing (8)
CHANGELOG.mdREADME.mdsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/types.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
| export class ArgumentError extends Error { | ||
| constructor(message: string) { | ||
| super(message); | ||
| this.name = this.constructor.name; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Expose structured fields on ArgumentError before exporting it publicly.
src/index.ts now re-exports this class, but it still only exposes name and message. That leaves library consumers without the code / error contract this file is supposed to provide for errors.
Proposed fix
export class ArgumentError extends Error {
+ public readonly code: string;
+ public readonly error: string;
+
constructor(message: string) {
super(message);
this.name = this.constructor.name;
+ this.code = 'INVALID_ARGUMENT';
+ this.error = message;
}
}As per coding guidelines, src/errors.ts: Ensure error objects include code, error, and optional url properties for proper error handling by library consumers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class ArgumentError extends Error { | |
| constructor(message: string) { | |
| super(message); | |
| this.name = this.constructor.name; | |
| } | |
| } | |
| export class ArgumentError extends Error { | |
| public readonly code: string; | |
| public readonly error: string; | |
| constructor(message: string) { | |
| super(message); | |
| this.name = this.constructor.name; | |
| this.code = 'INVALID_ARGUMENT'; | |
| this.error = message; | |
| } | |
| } |
🤖 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 4 - 9, Expose the structured error contract on
ArgumentError in src/errors.ts before it is re-exported through src/index.ts:
update the ArgumentError class so instances include the expected code and error
fields, and add an optional url field when applicable, alongside the existing
name/message setup. Make the constructor initialize these public properties
consistently so consumers can reliably inspect the error shape after importing
ArgumentError.
Source: Coding guidelines
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, ArgumentError, 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 web service client rejected 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).WebServiceError,ArgumentError, and 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 (183 tests), and prettier all pass.A parallel PR applies the same change to
@maxmind/geoip2-node.Part of STF-803.
Summary by CodeRabbit
WebServiceError(extendsError) using consistent fields:code,error(mapped tomessage), optionalstatus, optionalurl, and optional underlyingcause.WebServiceError,ArgumentError, and theWebServiceClientErrortype for clearer consumer handling.cause, including for invalid or unexpected server payloads.