Cleanups before a major release#1903
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR adds an options-object constructor for ChangesClient API and request-shape updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 several refactorings and breaking changes, including updating the WebServiceClient constructor to accept an options object with a custom fetcher, removing the external validator dependency in favor of custom and built-in validation helpers, and ensuring Transaction serialization does not mutate input objects. Feedback on these changes highlights a potential TypeError in the WebServiceClient constructor if null is passed as options, the need to prevent purely numeric top-level domains in the custom isFQDN validation, and a missing object check in camelcaseKeys that could lead to errors when processing primitive values.
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/request/transaction.ts (1)
103-111: 🩺 Stability & Availability | 🟠 Major
toString()still mutatesorder.referrerUriin place. Insrc/request/transaction.ts:103-107,sanitizeKeys()only makes a shallow copy, sosanitized.orderis still the caller’sOrderinstance. SincereferrerUriis aURL, converting it to a string overwrites that shared object; rebuildorderbefore serializing 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/request/transaction.ts` around lines 103 - 111, The Transaction.toString() path still mutates the shared Order object because sanitizeKeys() only shallow-copies and the referrerUri conversion is applied in place. Update the toString() logic in Transaction to rebuild sanitized.order as a new object before serializing, converting referrerUri to a string on that copy instead of assigning back onto the original Order instance. Keep the change localized to Transaction.sanitizeKeys()/toString() so JSON.stringify(snakecaseKeys(...)) operates on fully detached data.
🤖 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 4-6: `ArgumentError` in `src/errors.ts` does not expose the
standard library error fields expected by consumers. Update the `ArgumentError`
class so it includes `code`, `error`, and optional `url` properties alongside
`name` and `message`, or relocate this class out of `src/errors.ts` if it should
not follow that contract. Use the existing `ArgumentError` constructor as the
place to initialize and expose these fields consistently.
In `@src/request/transaction.spec.ts`:
- Around line 239-253: The non-mutation coverage in the Transaction
serialization spec only checks Billing, but toString() also rebuilds Shipping
and CreditCard and rewrites Order.referrerUri via sanitizeKeys(). Extend the
existing test in transaction.spec.ts using the Transaction, Billing, Device,
Shipping, CreditCard, and Order symbols so it asserts those caller-owned objects
still retain their original fields and do not gain renamed keys after
serialization.
In `@src/webServiceClient.spec.ts`:
- Around line 34-49: The test helper clientWith currently injects a fetch stub
and bypasses HTTP-layer mocking, which conflicts with the required pattern for
this spec suite. Update the request/response tests in webServiceClient.spec.ts
to use nock for mocking the Client HTTP interactions, and remove or avoid the
fetcher injection path in clientWith so the tests exercise the real HTTP layer
while still asserting captured requests where needed.
In `@src/webServiceClient.ts`:
- Around line 81-92: The options handling in webServiceClient should not
dereference null in the `typeof options === 'object'` branch. Update the
`Client` constructor logic to guard `options != null` before accessing
`options.fetcher`, `options.host`, and `options.timeout`, or explicitly reject
null with a stable library error instead of allowing a raw TypeError. Use the
`Client` constructor and the options-object branch as the fix point.
---
Outside diff comments:
In `@src/request/transaction.ts`:
- Around line 103-111: The Transaction.toString() path still mutates the shared
Order object because sanitizeKeys() only shallow-copies and the referrerUri
conversion is applied in place. Update the toString() logic in Transaction to
rebuild sanitized.order as a new object before serializing, converting
referrerUri to a string on that copy instead of assigning back onto the original
Order instance. Keep the change localized to
Transaction.sanitizeKeys()/toString() so JSON.stringify(snakecaseKeys(...))
operates on fully detached data.
🪄 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: a3990048-c00c-4c43-b0a9-a3637c796235
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
CHANGELOG.mdREADME.mdpackage.jsonsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/request/email.spec.tssrc/request/email.tssrc/request/order.tssrc/request/transaction.spec.tssrc/request/transaction.tssrc/response/models/factors.tssrc/response/models/insights.tssrc/response/models/score.tssrc/types.tssrc/utils.spec.tssrc/utils.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
Member
Author
|
Note This is Claude, commenting on Greg's behalf. Thanks for the reviews. Summary of how each finding was handled (the branch was rewritten via autosquash, so the fixes are folded into the relevant commits): Fixed
Intentional (kept)
Won't fix
204 tests pass; lint, build, and prettier are clean. |
Member
Author
|
Note This is Claude, commenting on Greg's behalf. Follow-up from a fresh review pass. The changes below are folded into the relevant commits (branch rewritten via autosquash). Addressed
Not changed (with reasons)
206 tests pass; lint, build, and prettier are clean. |
Replace the plain `string` type on `code` with `WebServiceErrorCode`, an open
`ClientErrorCode | (string & {})` union, mirroring geoip2-node. This offers
autocompletion for the five client-generated codes while still accepting any
code the web service returns. The `ClientErrorCode` and `WebServiceErrorCode`
types are exported from the package.
Client-generated errors are now built through a `clientError()` helper typed
with the closed `ClientErrorCode`, so a typo at a throw site is a compile
error. The server-returned `data.code` path stays open.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give ArgumentError an optional `cause` option and forward it to the Error constructor, matching WebServiceError and the geoip2-node error classes. This is additive; no current caller passes a cause. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 38-39: The changelog entry for the serialization mutation fix is
missing `Order`, so update the `CHANGELOG.md` note to reflect that serializing a
`Transaction` no longer mutates `Billing`, `Shipping`, `CreditCard`, or `Order`.
Keep the entry user-facing and release-agnostic, and make sure the mutation-fix
summary in the changelog matches the full scope described by the PR.
In `@CLAUDE.md`:
- Around line 304-325: The markdown in this section needs blank lines around
each fenced code block to satisfy MD031. Update the documentation near the
camelcaseKeys() example and the constructor validation example so the opening
and closing fences in the affected section are surrounded by empty lines,
keeping the surrounding prose and the suggested diff structure intact.
In `@src/request/email.ts`:
- Around line 8-24: The isFQDN helper in email request validation currently
assumes a string and can throw a raw TypeError when callers pass non-string
values such as a numeric domain. Update isFQDN() to first verify the input is a
string and return false for any non-string or otherwise invalid runtime value so
the Email constructor continues to surface the expected ArgumentError path. Keep
the fix localized to isFQDN() and its callers in src/request/email.ts, without
changing the existing FQDN validation rules for valid strings.
In `@src/request/transaction.ts`:
- Around line 197-216: The `customInputs` record validation in `transaction.ts`
is too permissive and should only allow plain JSON-safe values. Update the
validation logic in the `customInputs` branch to reject non-plain objects such
as `Date` instances and any non-finite numbers like `NaN` or `Infinity`, while
still allowing booleans, strings, and finite numbers. Use the existing
`ArgumentError` checks in the `props.customInputs` loop to enforce this before
serialization.
In `@src/webServiceClient.ts`:
- Around line 68-98: The WebServiceClient constructor currently only handles the
third argument and silently ignores a legacy fourth host value, which can change
request routing for existing callers. Update the constructor in WebServiceClient
to either continue supporting the positional timeout/host signature or
explicitly reject a fourth argument with a stable error instead of falling back
to the default host. Use the constructor’s options handling branch and the
existing accountID, licenseKey, timeout, and host assignment logic to locate the
fix.
🪄 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: 23fd8bd2-b51f-45d3-b07f-67d11b00edb1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
CHANGELOG.mdCLAUDE.mdREADME.mdpackage.jsonsrc/errors.spec.tssrc/errors.tssrc/index.tssrc/request/email.spec.tssrc/request/email.tssrc/request/order.spec.tssrc/request/order.tssrc/request/transaction.spec.tssrc/request/transaction.tssrc/response/models/factors.tssrc/response/models/insights.tssrc/response/models/score.tssrc/types.tssrc/utils.spec.tssrc/utils.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
Change the WebServiceClient constructor from
`(accountID, licenseKey, timeout?, host?)` to
`(accountID, licenseKey, options?: Options | number)`, matching geoip2-node.
`Options` carries `timeout`, `host`, and a new `fetcher` (a custom `fetch`
implementation, useful for proxies/dispatchers and testing). A bare number is
still accepted as the timeout for backward compatibility, so existing
`new Client(id, key, 3000)` callers are unaffected; only the rare caller that
passed `host` as a fourth positional argument must switch to `{ host }`.
Requests now go through `this.fetcher` rather than the global `fetch`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Transaction.toString() builds a shallow copy via Object.assign({}, this), so
the nested billing/shipping/creditCard objects were still the caller's
instances — and the address2 -> address_2 and was3DSecureSuccessful ->
was_3d_secure_successful renames mutated them in place (a caller's Billing
would lose address2 and gain address_2 after serialization). Rebuild each
renamed object as a fresh object instead, leaving the caller's instances
untouched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
TransactionProps.customInputs was typed CustomInput[] but the stored field is a Record, and the value was always run through Object.assign to flatten it. Additively widen the input type to `CustomInput[] | Record<string, boolean | number | string>` and handle both: an array is flattened as before, a record is used directly. Existing CustomInput[] callers are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Match geoip2-node's utility name for cross-library consistency. This is an internal helper (not exported from the package), so the rename has no effect on consumers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Note This is Claude, commenting on Greg's behalf. Another follow-up pass (review.md + the new bot threads). All folded into the relevant commits via autosquash. Addressed
The validation tweaks stay within the "match the old dependency, don't get stricter for valid inputs" guidance — no 212 tests pass; lint, build, and prettier are clean. |
Replace the three `validator` uses with built-ins: * email.ts: reimplement the `isEmail`/`isFQDN` input guards with small regex helpers (covered by the existing email.spec.ts cases, including IDN domains and trailing-dot rejections). * order.ts: the `referrerUri` `isURL` check was effectively validating a value that is already a `URL`; validate with the native `URL` constructor instead, preserving the ArgumentError on invalid input. * transaction.spec.ts: replace the test-only `isJSON` with a local `JSON.parse` helper. Drops `validator` and `@types/validator` from the dependencies. 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. A dedicated test asserts the request shape (method, path, POST body, auth header) directly; error/status cases return the corresponding `Response`, and the timeout case rejects when the request signal aborts — exercising the real timeout signal deterministically. This removes the `nock` dev dependency (which was on a beta release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
horgh
requested changes
Jun 29, 2026
horgh
left a comment
horgh
left a comment
Contributor
There was a problem hiding this comment.
Looks good! A few nits from Claude. At least one seems pre-existing but seems like it might be worth addressing:
- e4d6959 — the project CLAUDE.md (Web Service Client Pattern section) still shows the old new WebServiceClient(accountID, licenseKey, timeout, host) signature, which now throws. Out of scope for this commit's files but made stale by it. Also: the legacyHost?: never guard only fires on !== undefined (intended; TS callers are blocked at compile time anyway).
- 4d4dc3b — record keys are snake_cased on the wire (fizzBuzz → fizz_buzz). Not a regression (the array path did this too), but the ergonomic record form makes it easier to trip on; worth a JSDoc note since custom-input keys are portal-configured.
- 6f4a439 — clientWith()'s host? option is dead (no test passes it); the options.host branch isn't exercised here.
- 3b3e9f8 (CHANGELOG) — (a) the breaking constructor entry doesn't mention that a 4th positional arg now throws (reads as if it degrades silently); (b) the newly-exported WebServiceClientOptions type isn't listed, though the other new exported types (ClientErrorCode, WebServiceErrorCode) are. Otherwise complete and accurate — all 7 user-facing changes covered, breaking changes flagged first, 9.0.0 (unreleased) with no premature date.
Member
Author
|
Note This is Claude, replying on Greg's behalf. Thanks @horgh — all four nits addressed (in new commits on top, split by type of change rather than squashed):
Those three are documentation-only, so they're together in
The two cross-applicable items were also applied to geoip2-node#1726, likewise as separate commits: the dead 212 tests pass; lint, build, and prettier are clean. |
* Update the stale `WebServiceClient` constructor example in CLAUDE.md to the options-object form (the positional `timeout, host` form now throws). * Document on `Transaction.customInputs` that input keys are snake_cased on the wire (e.g. `fizzBuzz` -> `fizz_buzz`), to match portal-configured keys. * CHANGELOG: note that a fourth positional constructor argument now throws, and list the newly-exported `WebServiceClientOptions` type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/request/transaction.ts (1)
209-219: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject non-plain
customInputsrecords at runtime.Line 210 still treats any non-array value as a record.
Object.entries('ab')passes here and serializes as indexed keys, whileObject.entries(new Date())passes and serializes as{}. Both should throwArgumentErrorinstead of sending a malformedcustom_inputspayload.🛠️ Suggested fix
} else { + const proto = Object.getPrototypeOf(props.customInputs); + if ( + typeof props.customInputs !== 'object' || + props.customInputs === null || + (proto !== Object.prototype && proto !== null) + ) { + throw new ArgumentError( + '`customInputs` needs to be a plain object mapping input keys to boolean, finite number, or string values' + ); + } + for (const [key, value] of Object.entries(props.customInputs)) { const isValid = typeof value === 'boolean' || typeof value === 'string' || (typeof value === 'number' && Number.isFinite(value));As per coding guidelines,
src/request/**/*.ts: "Request components must validate constructor inputs and throwArgumentErrorfor invalid data".🤖 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/request/transaction.ts` around lines 209 - 219, The customInputs validation in transaction request handling currently accepts any non-array object-like value, so non-plain records such as strings or class instances can slip through and serialize incorrectly. Update the validation in the transaction/request path (the block using Object.entries(props.customInputs)) to first verify that customInputs is a plain object record before iterating its entries, and throw ArgumentError for anything else. Keep the existing per-entry primitive checks, but make the constructor/request input validation in src/request/transaction.ts reject invalid customInputs at runtime.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 `@src/request/order.ts`:
- Around line 77-83: The host validation in request/order.ts is incorrectly
rejecting valid IP-literal HTTP(S) referrers because the
parsed.hostname.includes('.') check treats IPv6 literals as invalid. Update the
referrer validation logic in the branch that uses parsed.protocol and
parsed.hostname so it still blocks non-http(s) schemes and single-label hosts,
but explicitly allows IP literals (especially IPv6 bracketed hosts) instead of
relying only on the presence of a dot in the hostname.
In `@src/request/transaction.spec.ts`:
- Around line 130-142: The new plain-record customInputs case in Transaction is
only asserting constructor state, but it also changes serialization. Update the
Transaction spec to verify Transaction.toString() for this record-based input
path and assert the emitted custom_inputs payload matches the expected
serialized form, using the existing Transaction and Device test setup.
In `@src/utils.spec.ts`:
- Around line 62-68: The current camelcaseKeys passthrough test only covers
primitives, but the new !isObject branch in camelcaseKeys should also be locked
down for non-plain objects. Extend the existing "returns non-object input
unchanged" test in src/utils.spec.ts to assert that Date, Error, and RegExp
values are returned unchanged, so the behavior in camelcaseKeys stays covered if
the object-handling logic changes later.
In `@src/utils.ts`:
- Line 34: Add a short JSDoc/TypeDoc block for the exported camelcaseKeys helper
to document its public contract. Make sure the comment explains that primitives
and non-plain objects are returned unchanged, while arrays and plain objects are
recursively converted to camelCase, so consumers can rely on the behavior
without reading the implementation.
In `@src/webServiceClient.spec.ts`:
- Around line 39-50: Add test coverage in WebServiceClient.spec around the new
Client({ host }) behavior by using the existing clientWith(fetcher, options)
helper to assert the request URL is rewritten correctly and the parsed response
still matches expectations. Update the relevant Client/WebServiceClient test
cases that currently cover fetcher, null, and legacy-host rejection so one
assertion exercises the host-based path and verifies both the final request
shape and returned data, without mocking the HTTP layer.
In `@src/webServiceClient.ts`:
- Around line 93-104: The option-handling branch in webServiceClient.ts
currently assigns values from the options object without validating their types,
so null values for host, timeout, or fetcher can be stored and fail later.
Update the constructor/options parsing in WebServiceClient to explicitly reject
invalid member types before assignment by checking options.host,
options.timeout, and options.fetcher, and raise ArgumentError immediately when
any of them are null or otherwise not the expected type, while keeping the
existing object guard intact.
---
Duplicate comments:
In `@src/request/transaction.ts`:
- Around line 209-219: The customInputs validation in transaction request
handling currently accepts any non-array object-like value, so non-plain records
such as strings or class instances can slip through and serialize incorrectly.
Update the validation in the transaction/request path (the block using
Object.entries(props.customInputs)) to first verify that customInputs is a plain
object record before iterating its entries, and throw ArgumentError for anything
else. Keep the existing per-entry primitive checks, but make the
constructor/request input validation in src/request/transaction.ts reject
invalid customInputs at runtime.
🪄 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: 1c22a028-eaa7-44b9-8277-ec952502ccc6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
CHANGELOG.mdCLAUDE.mdREADME.mdlychee.tomlpackage.jsonsrc/index.tssrc/request/email.spec.tssrc/request/email.tssrc/request/order.spec.tssrc/request/order.tssrc/request/transaction.spec.tssrc/request/transaction.tssrc/response/models/factors.tssrc/response/models/insights.tssrc/response/models/score.tssrc/utils.spec.tssrc/utils.tssrc/webServiceClient.spec.tssrc/webServiceClient.ts
The single-label-host guard rejected valid IPv6-literal referrers like `https://[2001:db8::1]/` (an IPv6 literal has no dot), which was stricter than the former `validator.isURL`. Exempt IP literals from the dot requirement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The record `customInputs` form changes serialization, so assert that `Transaction.toString()` emits the expected `custom_inputs` payload, not just the constructor state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the inaccurate "deeply clones an object" JSDoc with the actual contract: arrays and plain objects are deep key-converted, while primitives and non-plain objects (Date, Error, RegExp, etc.) pass through unchanged. Add tests locking in the Date/Error/RegExp passthrough. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
For JS callers, `{ host: null }`, `{ timeout: null }`, and `{ fetcher: null }`
previously slipped past the `!== undefined` checks and failed later (a bad URL,
a non-callable fetcher, or a NaN timeout signal). Validate each up front and
throw `ArgumentError`. Also add a test asserting the `host` option changes the
request URL.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The IP-literal referrer tests use RFC 5737 / RFC 3849 documentation addresses (192.0.2.1, [2001:db8::1]) that never resolve; exclude them so the link check doesn't fail or hang on them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Note This is Claude, replying on Greg's behalf. Thanks — all six addressed, in new commits (a human has reviewed, so no history rewrite), grouped by area:
The cross-applicable items ( 218 tests pass; lint, build, prettier, and the link check are clean. |
horgh
approved these changes
Jun 29, 2026
horgh
left a comment
horgh
left a comment
Contributor
There was a problem hiding this comment.
LGTM, I won't merge in case my comment on GeoIP2-node leads to a change here somehow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the pre-major-release cleanups tracked in STF-809. This is the
minfraud-api-nodefollow-up to the geoip2-node PR (maxmind/GeoIP2-node#1726),bringing the two libraries in line and doing a few minfraud-specific cleanups.
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)
Mirroring geoip2-node:
ClientErrorCodeunion forWebServiceError.code.codeis nowWebServiceErrorCode(ClientErrorCode | (string & {})) instead ofstring, giving autocompletion for the client-generated codes while stillaccepting any server code. Client errors are built through a
clientError()helper typed with the closed
ClientErrorCode, so a typo at a throw site isa compile error. New types are exported.
causeonArgumentError— accepts and forwards an optionalcause,matching
WebServiceError.fetcher. The client constructortakes
(accountID, licenseKey, options?: Options | number). A bare number isstill accepted as the timeout, so only callers passing
hostpositionallychange (to
{ host }).fetcherallows a customfetch(proxy/dispatcher,testing).
minfraud-specific:
Transactionserialization mutating caller objects.toString()shallow-copied the transaction, so the
address2→address_2/was3DSecureSuccessfulrenames mutated the caller'sBilling/Shipping/CreditCardinstances. Each renamed object is now rebuilt fresh.customInputsmay be a plain record, not just aCustomInput[](additive).
Dependencies & tests:
camelizeResponse→camelcaseKeysto match geoip2-node (internal).validatorproduction dependency — reimplement theisEmail/isFQDNinput guards (covered by the existingemail.spec.ts,including IDN domains), validate
referrerUriwith the nativeURLconstructor, and replace the test-only
isJSON.nockwith the injectedfetcherin the web-service tests anddrop the
nockdev dependency.Testing
npm run lint,npm run build, andprettierare clean.validatorandnockare gone frompackage.json.🤖 Generated with Claude Code
Summary by CodeRabbit
customInputscan be provided as an array or as a plain key/value object.cause.