fix(multipleOf): use float-safe modulo to prevent false positives with decimal requirements#1422
fix(multipleOf): use float-safe modulo to prevent false positives with decimal requirements#1422Vadaski wants to merge 5 commits intoopen-circle:mainfrom
Conversation
…h decimal requirements JavaScript % operator produces floating-point precision errors with decimal requirements, e.g. 3 % 0.01 is ~2.22e-16 instead of 0, causing multipleOf(0.01) to incorrectly reject valid inputs like 3 or 1.5. Fix by scaling both operands to integers before the modulo check: - Compute max decimal places of value and requirement via _getDecimalPlaces() - Multiply both by 10^decimalPlaces and round to nearest integer - Compare integer remainder to 0 The bigint path now uses explicit typeof narrowing and compares to 0n, removing the need for the @ts-expect-error suppression. Fixes: open-circle#1375 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@Vadaski is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
the
size:M
|
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:
📝 WalkthroughWalkthroughAdds decimal-aware divisibility to multipleOf by computing decimal places and scaling numbers for integer modulus checks; preserves bigint behavior with special zero handling. Introduces a decimal-place utility and expands tests to cover decimals, scientific notation, infinities, NaN, and bigint zero cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
There was a problem hiding this comment.
Pull request overview
Fixes multipleOf() validation for decimal requirements by avoiding floating-point modulo precision issues in JavaScript.
Changes:
- Add
_getDecimalPlaces()and switch number-path validation to integer-scaled modulo with rounding. - Keep bigint behavior while removing the prior
@ts-expect-errorvia explicit type narrowing. - Add tests for valid decimal multiples and for Infinity/NaN with a decimal requirement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
library/src/actions/multipleOf/multipleOf.ts |
Implements float-safer multiple checking by scaling to integers and adds _getDecimalPlaces() helper. |
library/src/actions/multipleOf/multipleOf.test.ts |
Adds test coverage for decimal multipleOf success cases and non-finite number failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if ( | ||
| Math.round(dataset.value * multiplier) % | ||
| Math.round(this.requirement * multiplier) !== | ||
| 0 | ||
| ) { |
There was a problem hiding this comment.
multiplier = 10 ** decimalPlaces can become Infinity when decimalPlaces > 308 (e.g. requirement 1e-309 => 309). In that case value * multiplier becomes NaN for 0 (0 * Infinity) and the remainder check always fails, so valid multiples like 0 or 1e-309 would incorrectly produce an issue. Consider guarding for non-finite multiplier / scaled operands and falling back to a safer check (e.g. direct % or value / requirement closeness-to-integer) when scaling isn’t possible.
| if ( | |
| Math.round(dataset.value * multiplier) % | |
| Math.round(this.requirement * multiplier) !== | |
| 0 | |
| ) { | |
| let isMultiple = true; | |
| // Preserve existing behavior: if requirement is 0, always treat as invalid. | |
| if (this.requirement === 0) { | |
| isMultiple = false; | |
| } else { | |
| const scaledValue = dataset.value * multiplier; | |
| const scaledRequirement = this.requirement * multiplier; | |
| if ( | |
| Number.isFinite(multiplier) && | |
| Number.isFinite(scaledValue) && | |
| Number.isFinite(scaledRequirement) | |
| ) { | |
| const roundedValue = Math.round(scaledValue); | |
| const roundedRequirement = Math.round(scaledRequirement); | |
| // If roundedRequirement becomes 0 due to rounding, fall back to division check. | |
| if (roundedRequirement !== 0) { | |
| isMultiple = roundedValue % roundedRequirement === 0; | |
| } else { | |
| const quotient = dataset.value / this.requirement; | |
| if (Number.isFinite(quotient)) { | |
| const nearest = Math.round(quotient); | |
| const EPSILON = 1e-12; | |
| isMultiple = Math.abs(quotient - nearest) <= EPSILON; | |
| } else { | |
| isMultiple = false; | |
| } | |
| } | |
| } else { | |
| // Fallback when scaling is not possible (e.g., multiplier is Infinity). | |
| const quotient = dataset.value / this.requirement; | |
| if (Number.isFinite(quotient)) { | |
| const nearest = Math.round(quotient); | |
| const EPSILON = 1e-12; | |
| isMultiple = Math.abs(quotient - nearest) <= EPSILON; | |
| } else { | |
| isMultiple = false; | |
| } | |
| } | |
| } | |
| if (!isMultiple) { |
| typeof this.requirement === 'bigint' && | ||
| dataset.value % this.requirement !== 0n | ||
| ) { | ||
| _addIssue(this, 'multiple', dataset, config); |
There was a problem hiding this comment.
With the new type narrowing, mismatched runtime types (e.g. dataset.value is a number but requirement is a bigint, or vice versa) will now skip validation entirely and return the dataset without an issue. Previously, the unguarded % would throw on mixed number/bigint, preventing a silent pass. It would be safer to explicitly handle the mixed-type case (e.g. add an issue or throw a clear error) so incorrect schema/action combinations don’t become false negatives.
| _addIssue(this, 'multiple', dataset, config); | |
| _addIssue(this, 'multiple', dataset, config); | |
| } else { | |
| // Mismatched or unsupported typed values: treat as a validation failure | |
| _addIssue(this, 'multiple', dataset, config); |
| test('for valid decimal numbers', () => { | ||
| expectNoActionIssue(multipleOf(0.01), [3, 0.1, 0.03, 1.5]); | ||
| expectNoActionIssue(multipleOf(0.1), [0.3]); | ||
| expectNoActionIssue(multipleOf(0.25), [0.75]); | ||
| }); |
There was a problem hiding this comment.
The new tests cover valid decimal multiples and non-finite inputs, but there’s no coverage for invalid decimal multiples (e.g. multipleOf(0.01) rejecting values like 0.005 / 0.015 / 1.234). Adding at least one failing case would help ensure the integer-scaling modulo logic doesn’t accidentally accept non-multiples due to rounding.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/actions/multipleOf/multipleOf.test.ts (1)
68-72: Good test coverage for decimal precision fix.The test cases directly verify the fix for the floating-point precision issue (e.g.,
0.3 % 0.1would incorrectly fail with naive modulo).Consider adding negative decimal values for completeness (e.g.,
-0.1withmultipleOf(0.01)), thoughMath.abs()in_getDecimalPlacesshould handle them correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/actions/multipleOf/multipleOf.test.ts` around lines 68 - 72, Add negative decimal cases to the existing decimal-precision test to cover negative values: update the test 'for valid decimal numbers' to include negative numbers (e.g., call expectNoActionIssue(multipleOf(0.01), [..., -0.1]) and similar for other multiples) so that multipleOf and the helper _getDecimalPlaces are exercised with negative inputs; ensure you add negative counterparts for the existing positive test vectors (for example -0.1 with multipleOf(0.01), -0.3 for multipleOf(0.1), -0.75 for multipleOf(0.25)) using the same expectNoActionIssue helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@library/src/actions/multipleOf/multipleOf.test.ts`:
- Around line 68-72: Add negative decimal cases to the existing
decimal-precision test to cover negative values: update the test 'for valid
decimal numbers' to include negative numbers (e.g., call
expectNoActionIssue(multipleOf(0.01), [..., -0.1]) and similar for other
multiples) so that multipleOf and the helper _getDecimalPlaces are exercised
with negative inputs; ensure you add negative counterparts for the existing
positive test vectors (for example -0.1 with multipleOf(0.01), -0.3 for
multipleOf(0.1), -0.75 for multipleOf(0.25)) using the same expectNoActionIssue
helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f518431-d2e0-4b39-8a9f-8ce1f5b0ceff
📒 Files selected for processing (2)
library/src/actions/multipleOf/multipleOf.test.tslibrary/src/actions/multipleOf/multipleOf.ts
Extend decimal-precision test cases with negative counterparts to exercise the Math.abs() path in _getDecimalPlaces() more thoroughly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thank you for creating a PR. Here some feedback:
|
|
Current Alternative A — Optimized string (drop-in replacement): const _getDecimalPlaces = (value: number): number => {
const s = String(Math.abs(value));
const dotIndex = s.indexOf('.');
const eIndex = s.indexOf('e');
if (dotIndex === -1 && eIndex === -1) return 0;
const decimals = dotIndex === -1 ? 0 : (eIndex === -1 ? s.length : eIndex) - dotIndex - 1;
if (eIndex === -1) return decimals;
return Math.max(0, decimals - Number(s.slice(eIndex + 1)));
};Alternative B — Epsilon-based (eliminates const remainder = Math.abs(value % requirement);
const tolerance = Number.EPSILON * Math.max(Math.abs(value), Math.abs(requirement));
if (remainder > tolerance && Math.abs(remainder - Math.abs(requirement)) > tolerance) {
_addIssue(this, 'multiple', dataset, config);
}The double check is needed because Trade-off summary:
This was created by Claude Code 🤖. Let me know if it is wrong or complicates the code. |
…laces, add tests Address 3 mandatory points from PR open-circle#1422 maintainer review: 1. Extract _getDecimalPlaces to utils/_getDecimalPlaces.ts following isbn/utils/ pattern; use function keyword and indexOf/slice to reduce heap allocations. 2. Precompute _getDecimalPlaces(requirement) once in the factory closure instead of recalculating on every validation call. 3. Add missing test cases: invalid decimal multiples, decimal values with integer requirement, and zero requirement (division by zero always invalid). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
size:L
|
Good catches, thank you. All three points addressed in the latest commit:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/actions/multipleOf/multipleOf.test.ts (1)
68-72: Add one regression case for scientific-notation inputs.Given
_getDecimalPlaceshas a dedicated exponent path (Line 12-31 inlibrary/src/actions/multipleOf/utils/_getDecimalPlaces.ts), add at least one passing and one failinge-notation case (e.g.,multipleOf(1e-7)with2e-7and3e-8) to lock that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/actions/multipleOf/multipleOf.test.ts` around lines 68 - 72, Add regression tests exercising the exponent branch of _getDecimalPlaces by adding scientific-notation cases to multipleOf.test.ts: call expectNoActionIssue(multipleOf(1e-7), [2e-7]) as a passing case and expectActionIssue or the inverse helper with [3e-8] as a failing case (or similar pair like multipleOf(1e-3) with 2e-3 pass and 3e-4 fail); use the existing helpers expectNoActionIssue/expectActionIssue and the multipleOf factory so the e-notation path in _getDecimalPlaces is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@library/src/actions/multipleOf/multipleOf.test.ts`:
- Around line 68-72: Add regression tests exercising the exponent branch of
_getDecimalPlaces by adding scientific-notation cases to multipleOf.test.ts:
call expectNoActionIssue(multipleOf(1e-7), [2e-7]) as a passing case and
expectActionIssue or the inverse helper with [3e-8] as a failing case (or
similar pair like multipleOf(1e-3) with 2e-3 pass and 3e-4 fail); use the
existing helpers expectNoActionIssue/expectActionIssue and the multipleOf
factory so the e-notation path in _getDecimalPlaces is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feacd407-302b-4e99-b38c-bbc845fae9db
📒 Files selected for processing (4)
library/src/actions/multipleOf/multipleOf.test.tslibrary/src/actions/multipleOf/multipleOf.tslibrary/src/actions/multipleOf/utils/_getDecimalPlaces.tslibrary/src/actions/multipleOf/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- library/src/actions/multipleOf/multipleOf.ts
commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const decimalPlaces = Math.max( | ||
| _getDecimalPlaces(dataset.value), | ||
| requirementDecimalPlaces | ||
| ); | ||
| const multiplier = 10 ** decimalPlaces; | ||
|
|
||
| if ( | ||
| Math.round(dataset.value * multiplier) % | ||
| Math.round(this.requirement * multiplier) !== | ||
| 0 | ||
| ) { |
| typeof this.requirement === 'bigint' && | ||
| dataset.value % this.requirement !== 0n | ||
| ) { | ||
| _addIssue(this, 'multiple', dataset, config); |
| expectNoActionIssue(multipleOf(0.1), [0.3, -0.3]); | ||
| expectNoActionIssue(multipleOf(0.25), [0.75, -0.75]); | ||
| }); | ||
|
|
- Prevent RangeError when requirement is 0n by treating it as always invalid - Short-circuit value === 0 for finite nonzero requirements to avoid Infinity multiplier overflow in decimal scaling path - Add scientific-notation test coverage for _getDecimalPlaces exponent branch - Add zero bigint requirement regression test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Latest push addresses two edge cases I missed:
Also added scientific-notation test cases ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/actions/multipleOf/multipleOf.ts`:
- Around line 166-179: The scaled-modulo can overflow because multiplier = 10 **
decimalPlaces may be Infinity; update the multiple check in the multipleOf logic
to guard for that: compute decimalPlaces via _getDecimalPlaces(...) and
multiplier as before, then if Number.isFinite(multiplier) perform the existing
Math.round(dataset.value * multiplier) % Math.round(this.requirement *
multiplier) check, otherwise fall back to a native modulo check (e.g.,
dataset.value % this.requirement === 0 or an appropriate tolerance-based
comparison) so the function (references: dataset.value, this.requirement,
_getDecimalPlaces, decimalPlaces, multiplier) returns correct results for
subnormal/small values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdc5d356-ce6a-4d3c-8710-68e05e76cdc8
📒 Files selected for processing (2)
library/src/actions/multipleOf/multipleOf.test.tslibrary/src/actions/multipleOf/multipleOf.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- library/src/actions/multipleOf/multipleOf.test.ts
When decimalPlaces > 308 (e.g. requirement = 5e-324), 10 ** decimalPlaces overflows to Infinity, causing Math.round(value * Infinity) to produce NaN and incorrectly reject valid multiples. Fall back to native % operator when the multiplier is not finite. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/actions/multipleOf/multipleOf.ts (1)
166-186: Consider explicit guard forrequirement === 0with numbers for consistency.The bigint path explicitly guards against
0nrequirement (lines 191-194), but the number path relies on implicitNaNbehavior when0 % 0is computed. While functionally correct (NaN fails the check), explicit handling would improve clarity and consistency:if (dataset.value === 0 && this.requirement !== 0 && Number.isFinite(this.requirement)) { return dataset; } + + if (this.requirement === 0) { + _addIssue(this, 'multiple', dataset, config); + return dataset; + } const decimalPlaces = Math.max(This also avoids unnecessary
_getDecimalPlacescomputation when the requirement is zero.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/actions/multipleOf/multipleOf.ts` around lines 166 - 186, Add an explicit guard for the numeric-path when this.requirement === 0 (mirroring the bigint branch) before calling _getDecimalPlaces: if this.requirement === 0 then return dataset when dataset.value === 0, otherwise call _addIssue(this, 'multiple', dataset, config); place this check before computing decimalPlaces/multiplier to avoid unnecessary work and to make behavior consistent with the bigint 0n handling (symbols: this.requirement, dataset.value, _getDecimalPlaces, multiplier, _addIssue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@library/src/actions/multipleOf/multipleOf.ts`:
- Around line 166-186: Add an explicit guard for the numeric-path when
this.requirement === 0 (mirroring the bigint branch) before calling
_getDecimalPlaces: if this.requirement === 0 then return dataset when
dataset.value === 0, otherwise call _addIssue(this, 'multiple', dataset,
config); place this check before computing decimalPlaces/multiplier to avoid
unnecessary work and to make behavior consistent with the bigint 0n handling
(symbols: this.requirement, dataset.value, _getDecimalPlaces, multiplier,
_addIssue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d37fd08-4048-4f03-b918-ca062e81a061
📒 Files selected for processing (1)
library/src/actions/multipleOf/multipleOf.ts
Problem
v.multipleOf()incorrectly rejects valid inputs when the requirement is a decimal number, because JavaScript's%operator introduces floating-point precision errors.The root cause:
3 % 0.01in JavaScript is~2.22e-16instead of0.Reported in #1375.
Fix
Scale both operands to integers before taking the modulo:
_getDecimalPlaces()10^decimalPlacesusingMath.round()to neutralize any remaining float error at the scale boundary0The
bigintpath is unchanged in behavior; the explicittypeofnarrowing now allows!== 0nwithout the previous@ts-expect-error.Tests added
Known limitation
For pathological inputs with very high decimal precision combined with very large values (e.g.
multipleOf(1e-17)onNumber.MAX_SAFE_INTEGER), the scaled integer can exceedNumber.MAX_SAFE_INTEGERand lose precision. This matches the inherent limitation of IEEE 754 arithmetic. Practical uses ofmultipleOfwith reasonable decimal requirements (like0.01,0.1,0.25) are not affected.Summary by CodeRabbit
Bug Fixes
Tests