Conversation
WalkthroughAdded a changelog entry for 22.4.1 describing a fix for expanding very large double values. Updated README CDN script and package.json version from 22.4.0 to 22.4.1. In src/client.ts added BigInt 64-bit bounds (MAX_INT64, MIN_INT64), adjusted the reviver: integer BigInt values within JS safe integer range remain numbers; values within 64-bit bounds are returned as BigInt; values outside 64-bit bounds fall back to toNumber(). Updated SDK version header to 22.4.1. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client.ts (1)
30-33: Out-of-INT64-range fallback silently loses precision.Line 33 (
return value.toNumber()) is reached only whenbiis outside the[MIN_INT64, MAX_INT64]range. JavaScript's standardNumbertype cannot accurately represent integers larger thanNumber.MAX_SAFE_INTEGER; for such values,BigIntor string handling is needed. Since every value that reaches this branch exceedsMAX_INT64(> 2⁶³ > 2⁵³),toNumber()will silently corrupt it.The
biBigInt is already computed on line 26 and would preserve precision at no extra cost. The inconsistency — returning a preciseBigIntfor in-range INT64 values but a lossyNumberfor out-of-range ones — is easy to close:♻️ Proposed fix — return the already-computed BigInt instead of falling back to toNumber()
if (bi >= MIN_INT64 && bi <= MAX_INT64) { return bi; } - return value.toNumber(); + return bi; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.ts` around lines 30 - 33, The fallback branch silently loses precision by returning value.toNumber() for values outside [MIN_INT64, MAX_INT64]; change that branch to return the already-computed BigInt (bi) instead, and update any related type annotations or callers if necessary so the function consistently returns BigInt for big integers (referencing variables bi, MIN_INT64, MAX_INT64 and the current value.toNumber() fallback).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client.ts`:
- Around line 30-33: The fallback branch silently loses precision by returning
value.toNumber() for values outside [MIN_INT64, MAX_INT64]; change that branch
to return the already-computed BigInt (bi) instead, and update any related type
annotations or callers if necessary so the function consistently returns BigInt
for big integers (referencing variables bi, MIN_INT64, MAX_INT64 and the current
value.toNumber() fallback).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/client.ts (1)
24-33: Optional: skipBigInt(str)construction for out-of-range values via a string-length fast-path.The fix is logically correct, but
BigInt(str)at line 26 is unconditionally called even whenstris a hundreds-of-digits expansion of a large double (the exact scenario this PR addresses). The transient BigInt is allocated just to fail the bounds check and be discarded.Using
toFixed()prevents exponential notation being returned, no matter how large or small the value, so the resulting string for1.7976931348623157e+308will be a ~309-character integer literal. SinceMAX_INT64has 19 digits andMIN_INT64has 20 characters (including-), anystr.length > 20is definitively outside INT64 range — no BigInt needed.♻️ Proposed early-exit optimization
if (value.isInteger()) { const str = value.toFixed(); + // Fast-path: INT64 max is 19 digits, so anything longer is out-of-range + if (str.length > 20) { + return value.toNumber(); + } const bi = BigInt(str); if (bi >= MIN_SAFE && bi <= MAX_SAFE) { return Number(str); } if (bi >= MIN_INT64 && bi <= MAX_INT64) { return bi; } return value.toNumber();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client.ts` around lines 24 - 33, Add a string-length fast-path before constructing BigInt in the integer-handling branch: after const str = value.toFixed(), check the string length against INT64 digit limits (negative literals include the '-' so use 20 chars for negatives, 19 for positives); if the length is already greater than those limits, skip BigInt and directly return value.toNumber() (this avoids creating a huge transient BigInt). Keep the existing MIN_SAFE/MAX_SAFE and MIN_INT64/MAX_INT64 checks and only call BigInt(str) when the string length is within the INT64-character bound so the subsequent BigInt conversion is cheap and meaningful.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/client.ts`:
- Around line 24-33: Add a string-length fast-path before constructing BigInt in
the integer-handling branch: after const str = value.toFixed(), check the string
length against INT64 digit limits (negative literals include the '-' so use 20
chars for negatives, 19 for positives); if the length is already greater than
those limits, skip BigInt and directly return value.toNumber() (this avoids
creating a huge transient BigInt). Keep the existing MIN_SAFE/MAX_SAFE and
MIN_INT64/MAX_INT64 checks and only call BigInt(str) when the string length is
within the INT64-character bound so the subsequent BigInt conversion is cheap
and meaningful.
This PR contains updates to the Web SDK for version 22.4.1.