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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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
|
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. |
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
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
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>
`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>
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>
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>
|
Note This is Claude, commenting on Greg's behalf. Addressed the two CodeRabbit findings (folded into the relevant commits):
The same two changes were applied to minfraud-api-node for consistency. 126 tests pass; lint and build are clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils.ts (1)
197-220: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd TypeDoc for the widened
camelcaseKeyscontract.This export now accepts and returns
unknown, but the public API still does not document the new passthrough behavior for primitives,null, andundefined.Suggested change
+/** + * Converts object keys from snake_case to camelCase. + * + * Non-object inputs, including primitives, `null`, and `undefined`, are + * returned unchanged. + * + * Availability: utility helper; not tied to a specific endpoint or database. + */ export function camelcaseKeys(input: unknown): unknown {As per coding guidelines, "Public API properties and methods must include TypeDoc JSDoc-style comments documenting the field, including availability (which endpoints/databases provide it)".
🤖 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.ts` around lines 197 - 220, The public contract of camelcaseKeys has been widened to accept and return unknown, but its TypeDoc still does not describe the passthrough behavior for primitives, null, and undefined. Add or update the JSDoc/TypeDoc on camelcaseKeys to document that non-object inputs are returned unchanged, and note the array/object recursion behavior so the exported API matches the implementation.Source: Coding guidelines
🤖 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 17-30: The changelog section is using the released version heading
instead of the pre-release heading. Update the section header in CHANGELOG.md
from the current 7.0.0 label to 7.0.0 (unreleased), keeping the existing entries
under that unreleased section until the actual release.
---
Outside diff comments:
In `@src/utils.ts`:
- Around line 197-220: The public contract of camelcaseKeys has been widened to
accept and return unknown, but its TypeDoc still does not describe the
passthrough behavior for primitives, null, and undefined. Add or update the
JSDoc/TypeDoc on camelcaseKeys to document that non-object inputs are returned
unchanged, and note the array/object recursion behavior so the exported API
matches the implementation.
🪄 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: 40e73627-29dc-4982-8344-6e2434214ebc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mdpackage.jsonsrc/errors.tssrc/index.tssrc/records.tssrc/types.tssrc/utils.spec.tssrc/utils.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
💤 Files with no reviewable changes (1)
- package.json
Use the `7.0.0 (unreleased)` heading until release time, per the changelog convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note This is Claude, commenting on Greg's behalf. Marked the CHANGELOG section as |
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 for custom request handling.open/openBuffer) to improve localized lookups.ClientErrorCodeandWebServiceErrorCodefor more type-safe error-code handling.cause.cause.enterpriselookup return type/behavior.