Cleanups before a major release#1726
Conversation
Give AddressNotFoundError, BadMethodCallError, InvalidDbBufferError, and ValueError an optional `cause` option and forward it to the Error constructor, matching the WebServiceError behavior. Reader.openBuffer previously threw `new InvalidDbBufferError(e)`, passing the underlying Error as the message and discarding its stack and cause. It now passes the error message and preserves the original error as `cause`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The method constructs a models.Enterprise but was annotated as the wider models.City. Enterprise extends City and adds no fields today, so the two are structurally identical and the change is not yet observable, but the annotation now reflects the actual return type and will expose any Enterprise-specific fields added in the future. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 14 minutes and 25 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 (2)
📝 WalkthroughWalkthroughThis PR updates error-code typing and cause propagation, threads locale preferences through reader/model creation, wraps ChangesLocale, fetcher, and error handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 support for a locales option in WebServiceClient and Reader to resolve localized place names via a new name accessor. It also adds a custom fetcher option, supports request cancellation via AbortSignal, and updates error classes to preserve the underlying cause. Feedback on the changes highlights a potential bug in selectName where prototype properties (e.g., toString) could be incorrectly resolved as localized names if passed as a locale; it is recommended to ensure the resolved value is a string and to add a corresponding test case.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
41-57: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTighten the abort-signal docs to match the API.
This section currently says every endpoint accepts a
{ signal }second argument and that request failures reject withWebServiceError. In the documented change set, onlycity(),country(), andinsights()takesignal, and an aborted request surfaces the original abort error instead of being wrapped. Please narrow the wording so callers are not told to expect options or error shapes that other methods do not support.🤖 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 `@README.md` around lines 41 - 57, The README usage text is too broad for the current API: update the endpoint docs around the client methods so only city(), country(), and insights() are described as accepting an optional { signal } AbortSignal option, and avoid implying every endpoint supports it. Also adjust the failure description so aborted requests are documented as surfacing the original abort error rather than always rejecting with WebServiceError, keeping the wording aligned with the actual client behavior.
🤖 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 `@CHANGELOG.md`:
- Around line 28-32: The changelog entry for localized records only mentions the
new name accessor and omits the user-visible change that these records now
return as class instances instead of plain objects. Update the existing
CHANGELOG.md release note to explicitly call out the record-instance behavior
change alongside the name accessor, using the same localized-records wording so
consumers know about prototype, instanceof, and direct-construction impact
before the major release.
In `@src/models/City.ts`:
- Around line 51-56: The City subdivision mapping still assigns undefined when
no subdivisions are present, which conflicts with the documented behavior.
Update the City model’s subdivisions initialization so the branch in the
constructor returns an empty array instead of undefined, or adjust the
type/contract consistently; use the existing camel.subdivisions mapping and
records.SubdivisionsRecord path to locate the fix.
In `@src/models/Country.ts`:
- Around line 42-46: The public Country constructor now takes a new locales
parameter, but the TypeDoc only documents response, so update the JSDoc on
Country and its constructor to include `@param` locales. Make sure the
documentation explains that locales controls localized name resolution and
defaults to ['en'], and keep the comment aligned with the existing public API
docs for the Country model.
In `@src/models/locales.spec.ts`:
- Around line 7-34: The current locale tests only cover the default English
fallback and miss the new locale-forwarding behavior in the model name
accessors. Update the locales spec around models.Insights, models.Enterprise,
models.City, and models.Country to assert a caller-supplied locale order
resolves the expected translated name, and add a no-match case where the name
accessor returns undefined when none of the provided locales exist. Keep the
assertions focused on the record name resolution path so the behavior is covered
for both presence and absence of localized data.
In `@src/names.ts`:
- Around line 56-61: The NameRecord constructor is keeping the caller’s locales
array by reference, so later mutations can change record.name behavior after
construction. Update the locales definition inside the constructor in
Names/NameRecord to snapshot the input array at creation time by storing a
copied array instead of the original reference, while keeping the property
non-enumerable.
In `@src/reader.ts`:
- Around line 21-27: Support passing locales as the second argument to
Reader.open(): the current signature treats a string array as mmdb.OpenOpts, so
add an overload and runtime type check in Reader.open to distinguish
mmdb.OpenOpts from locales and forward the correct values to mmdb.open. Update
the Reader.open method signature/documentation in src/reader.ts with TypeDoc
JSDoc for the public API, and keep the existing default locales behavior while
allowing Reader.open(file, ['de', 'en']) to work without an undefined
placeholder.
In `@src/records.ts`:
- Around line 1-4: `Names` is only referenced in type positions in records.ts,
but `NamedRecord` and `Names` are currently imported together as a value import
from `./names.js`. Update the import at the top of the module so `NamedRecord`
remains a normal import while `Names` is brought in with a type-only import, and
keep the existing re-export of `Names` intact so the runtime bundle stays
minimal.
In `@src/webServiceClient.ts`:
- Around line 196-206: The abort-handling path in this fetch catch block is
normalizing every non-Error rejection before rethrowing, which changes
caller-supplied abort reasons. Update the logic around this `this.fetcher`
try/catch so that when `signal?.aborted` is true, the original rejection value
(`err`) is thrown back exactly as received, without wrapping it in `Error`. Keep
the existing `WebServiceError` wrapping behavior for non-abort failures, and use
the `signal`/`error` handling in `src/webServiceClient.ts` to locate the change.
---
Outside diff comments:
In `@README.md`:
- Around line 41-57: The README usage text is too broad for the current API:
update the endpoint docs around the client methods so only city(), country(),
and insights() are described as accepting an optional { signal } AbortSignal
option, and avoid implying every endpoint supports it. Also adjust the failure
description so aborted requests are documented as surfacing the original abort
error rather than always rejecting with WebServiceError, keeping the wording
aligned with the actual client behavior.
🪄 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: a50d8ac6-9f55-4a57-b023-6d4a9b1376a4
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mdsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/models/City.tssrc/models/Country.tssrc/models/Enterprise.tssrc/models/Insights.tssrc/models/locales.spec.tssrc/names.spec.tssrc/names.tssrc/reader.spec.tssrc/reader.tssrc/readerModel.spec.tssrc/readerModel.tssrc/records.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/webServiceClient.spec.ts`:
- Line 63: Remove the duplicate calls declaration in the webServiceClient spec
so the test file compiles. In the same scope where calls is currently declared
twice, keep only one const calls array and update any nearby test setup or
assertions in src/webServiceClient.spec.ts to reuse that single variable; use
the existing calls identifier in the affected test block(s) to locate and clean
up the redeclaration.
🪄 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: e999d4de-4e0d-4f45-9e6a-8be0f387ab31
📒 Files selected for processing (5)
CHANGELOG.mdREADME.mdsrc/records.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
Replace the plain `string` type on `code` with `WebServiceErrorCode`, an
open `ClientErrorCode | (string & {})` union. This offers autocompletion
for the six client-generated codes while still accepting any code the web
service may return. The `ClientErrorCode` and `WebServiceErrorCode` types
are exported from the package.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `Names` interface comment referred to "the locales list passed to the `WebServiceClient` constructor", but no such argument exists in this library; the `names` object always contains every available locale. Reword the comment to describe the actual behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an optional `fetcher` to the WebServiceClient options so a custom `fetch` implementation can be supplied (useful for proxies, custom dispatchers, and testing); it defaults to the global `fetch`. Also record the cause-preservation and code-union changes from earlier in this branch in the CHANGELOG. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The WebServiceClient tests now drive requests through an injected `fetcher` that returns canned `Response`s and records the requests made, instead of mocking at the HTTP layer with nock. Request shape (method, path, auth header) is asserted directly, error/status cases return the corresponding `Response`, and the timeout case rejects when the request signal aborts — exercising the real timeout signal deterministically rather than via a wall-clock delay. This removes the `nock` dev dependency (which was on a beta release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note This is Claude, commenting on Greg's behalf. Re: the CodeRabbit and Gemini reviews above — these reviewed commit As a result, nearly all of the inline comments reference code that no longer exists in the diff ( The two suggestions that did apply to the current code have been addressed (folded into the relevant commits):
123 tests pass; lint, build, and prettier are clean. |
`typeof null === 'object'`, so `new Client(id, key, null)` from a JS caller entered the options-object branch and threw a TypeError on `options.fetcher`. Treat null like undefined. The buggy branch predates this PR, so this is a standalone fix rather than a fixup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/types.ts`:
- Around line 32-33: Add TypeDoc to the WebServiceClientError.code property in
the WebServiceClientError interface so the public API field is documented per
the src/**/*.ts guidelines. Update the interface member comment near
WebServiceClientError to state that code is always present on web-service client
errors, and clarify whether the value is client-generated or returned by the
service, including any availability/source notes needed for consumers.
🪄 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: c90fe3cd-0299-48af-bf75-97f23c3aad67
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdpackage.jsonsrc/errors.tssrc/index.tssrc/records.tssrc/types.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
💤 Files with no reviewable changes (1)
- package.json
| export interface WebServiceClientError { | ||
| code: string; | ||
| code: WebServiceErrorCode; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add TypeDoc for WebServiceClientError.code.
Line 33 changes a public API field, but this interface member still has no TypeDoc block describing the field and its availability/source. Please document that it is always present on web-service client errors and whether the value is client-generated or service-returned.
As per coding guidelines, src/**/*.ts: Public API properties and methods must include TypeDoc JSDoc-style comments documenting the field, including availability (which endpoints/databases provide it).
Proposed fix
export interface WebServiceClientError {
+ /**
+ * The error code returned by the web service or generated by this client.
+ * Available on all web service client errors; client-side failures use a
+ * {`@link` ClientErrorCode}, while server responses may return any string code.
+ */
code: WebServiceErrorCode;
error: string;📝 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 interface WebServiceClientError { | |
| code: string; | |
| code: WebServiceErrorCode; | |
| export interface WebServiceClientError { | |
| /** | |
| * The error code returned by the web service or generated by this client. | |
| * Available on all web service client errors; client-side failures use a | |
| * {`@link` ClientErrorCode}, while server responses may return any string code. | |
| */ | |
| code: WebServiceErrorCode; | |
| error: string; |
🤖 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/types.ts` around lines 32 - 33, Add TypeDoc to the
WebServiceClientError.code property in the WebServiceClientError interface so
the public API field is documented per the src/**/*.ts guidelines. Update the
interface member comment near WebServiceClientError to state that code is always
present on web-service client errors, and clarify whether the value is
client-generated or returned by the service, including any availability/source
notes needed for consumers.
Source: Coding guidelines
Guard against primitive input (e.g. from a JS caller) the way the minfraud-api-node port does: a non-object now passes through unchanged rather than being turned into an index-keyed object by Object.entries. Keeps the two libraries' shared utility consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/utils.spec.ts`:
- Around line 122-126: The passthrough test for camelcaseKeys is missing
coverage for the new null and undefined branch. Update the existing “returns
non-object input unchanged” spec in camelcaseKeys to also assert that null and
undefined are returned unchanged, keeping the checks alongside the current
string, number, and boolean cases.
🪄 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: c73e6b9f-a10c-4a5d-b243-494e8dc79b32
📒 Files selected for processing (2)
src/utils.spec.tssrc/utils.ts
| it('returns non-object input unchanged', () => { | ||
| expect(camelcaseKeys('foo')).toBe('foo'); | ||
| expect(camelcaseKeys(42)).toBe(42); | ||
| expect(camelcaseKeys(true)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add null and undefined coverage for the new passthrough branch.
src/utils.ts now explicitly preserves null and undefined, but this spec only exercises string/number/boolean. Please assert those two cases as well so the new behavior is fully covered.
Suggested test additions
it('returns non-object input unchanged', () => {
expect(camelcaseKeys('foo')).toBe('foo');
expect(camelcaseKeys(42)).toBe(42);
expect(camelcaseKeys(true)).toBe(true);
+ expect(camelcaseKeys(null)).toBeNull();
+ expect(camelcaseKeys(undefined)).toBeUndefined();
});📝 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.
| it('returns non-object input unchanged', () => { | |
| expect(camelcaseKeys('foo')).toBe('foo'); | |
| expect(camelcaseKeys(42)).toBe(42); | |
| expect(camelcaseKeys(true)).toBe(true); | |
| }); | |
| it('returns non-object input unchanged', () => { | |
| expect(camelcaseKeys('foo')).toBe('foo'); | |
| expect(camelcaseKeys(42)).toBe(42); | |
| expect(camelcaseKeys(true)).toBe(true); | |
| expect(camelcaseKeys(null)).toBeNull(); | |
| expect(camelcaseKeys(undefined)).toBeUndefined(); | |
| }); |
🤖 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/utils.spec.ts` around lines 122 - 126, The passthrough test for
camelcaseKeys is missing coverage for the new null and undefined branch. Update
the existing “returns non-object input unchanged” spec in camelcaseKeys to also
assert that null and undefined are returned unchanged, keeping the checks
alongside the current string, number, and boolean cases.
Rename the constructor's `Options` interface to `WebServiceClientOptions` and export it so consumers can name the options object. Keeps the public surface consistent with minfraud-api-node. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Part of the pre-major-release cleanups tracked in STF-809. These are the
geoip2-nodechanges;minfraud-api-nodewill follow in a separate PR.The guiding principle is gentle breaking changes: most users need to change
nothing, or only make trivial (lint/type-level) adjustments.
Changes (one commit each)
AddressNotFoundError,BadMethodCallError,InvalidDbBufferError, andValueErrornow accept anoptional
causeand forward it toError.Reader.openBuffer()previouslythrew
new InvalidDbBufferError(e)— passing the underlying error as themessage and discarding its stack/cause — and now preserves it as
cause.enterprise()asEnterpriserather than the widerCity(italready constructs an
Enterprise).ClientErrorCodeunion forWebServiceError.code. Typed asWebServiceErrorCode(ClientErrorCode | (string & {})) for autocompletionof the six client-generated codes, still accepting any server code. New
types are exported.
Namesdoc comment that referred to alocalesconstructor argument this library does not have.
fetcher. A customfetchcan be supplied via the optionsobject (useful for proxies/dispatchers and testing); defaults to the global
fetch.nockwith the injectedfetcherin the web service tests anddrop the
nockdev dependency (it was on a beta release). Tests now driverequests through a recording fetcher, assert the request shape (method,
path, auth header) directly, and simulate the timeout via the request
signal — deterministic, no wall-clock delay.
Testing
npm run lint,npm run build, andprettierare clean.🤖 Generated with Claude Code
Summary by CodeRabbit
fetcheroption to the web service client to support custom request handling.open/openBuffer) to improve localized lookups.ClientErrorCodeandWebServiceErrorCodefor more type-safe error code handling.cause.cause.enterpriselookup return type/behavior.