fix: use SameValueZero equality for special numeric values - #1573
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdded Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
commit: |
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 `@library/src/schemas/intersect/utils/_merge/_merge.test.ts`:
- Around line 43-49: Strengthen the invalid-date test around _merge by
explicitly asserting that the merged result value is a Date before validating
its invalid timestamp with Number.isNaN. Keep the existing issue expectation and
timestamp assertion unchanged.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c47477ef-e073-4b0a-abe3-984fe7af8d25
📒 Files selected for processing (16)
library/src/actions/notValue/notValue.test.tslibrary/src/actions/notValue/notValue.tslibrary/src/actions/notValues/notValues.test.tslibrary/src/actions/notValues/notValues.tslibrary/src/actions/value/value.test.tslibrary/src/actions/value/value.tslibrary/src/actions/values/values.test.tslibrary/src/actions/values/values.tslibrary/src/schemas/intersect/utils/_merge/_merge.test.tslibrary/src/schemas/intersect/utils/_merge/_merge.tslibrary/src/schemas/literal/literal.test.tslibrary/src/schemas/literal/literal.tslibrary/src/utils/_sameValueZero/_sameValueZero.test.tslibrary/src/utils/_sameValueZero/_sameValueZero.tslibrary/src/utils/_sameValueZero/index.tslibrary/src/utils/index.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes edge-case equality behavior by introducing a shared SameValueZero comparator and using it across literal/value-based validations and intersect merging, so that NaN (and invalid Date timestamps during merge) can match itself while keeping -0/+0 interchangeable.
Changes:
- Added internal
_sameValueZeroutility (SameValueZero semantics:NaNequalsNaN,-0equals+0) with tests and exported it from the utils barrel. - Updated
literal,value,notValue,values, andnotValuesto use_sameValueZerobefore (or instead of) existing comparison logic. - Updated
intersect’s_mergeto use_sameValueZerofor both direct equality andDatetimestamp equality (including invalid dates), with regression tests.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| library/src/utils/index.ts | Exports the new internal _sameValueZero utility. |
| library/src/utils/_sameValueZero/index.ts | Adds barrel export for the _sameValueZero implementation. |
| library/src/utils/_sameValueZero/_sameValueZero.ts | Introduces SameValueZero comparison helper used across multiple APIs. |
| library/src/utils/_sameValueZero/_sameValueZero.test.ts | Adds unit coverage for NaN and signed-zero behavior. |
| library/src/schemas/literal/literal.ts | Uses _sameValueZero so literal(NaN) works while keeping -0/+0 interchangeable. |
| library/src/schemas/literal/literal.test.ts | Adds regression tests for NaN and signed-zero literals. |
| library/src/schemas/intersect/utils/_merge/_merge.ts | Uses _sameValueZero for merge equality, including invalid Date timestamps. |
| library/src/schemas/intersect/utils/_merge/_merge.test.ts | Adds regression tests for NaN merges, signed zeros, and invalid dates. |
| library/src/actions/values/values.ts | Adds _sameValueZero short-circuit to correctly match NaN in values. |
| library/src/actions/values/values.test.ts | Adds regression test ensuring values([NaN]) accepts NaN. |
| library/src/actions/value/value.ts | Adds _sameValueZero short-circuit to correctly match NaN in value. |
| library/src/actions/value/value.test.ts | Adds regression test ensuring value(NaN) accepts NaN. |
| library/src/actions/notValues/notValues.ts | Adds _sameValueZero short-circuit so notValues([NaN]) correctly rejects NaN. |
| library/src/actions/notValues/notValues.test.ts | Adds regression test ensuring NaN is rejected for notValues([NaN]). |
| library/src/actions/notValue/notValue.ts | Adds _sameValueZero short-circuit so notValue(NaN) correctly rejects NaN. |
| library/src/actions/notValue/notValue.test.ts | Adds regression test ensuring NaN is rejected for notValue(NaN). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
All reported issues were addressed across 16 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
I consider it a bug fix rather than an API-breaking change. With strict equality,
Done |
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Fix SameValueZero equality for special numeric values
Summary
This change makes value equality consistent across
literal,intersect,value,notValue,values, andnotValues.Problem
JavaScript strict equality and ordering comparisons do not consider
NaNequalto itself. As a result, several APIs rejected a value that was identical to its
configured requirement:
literal(NaN)rejectedNaN.value(NaN)andvalues([NaN])rejectedNaN.notValue(NaN)andnotValues([NaN])acceptedNaN.intersectreported a conflict when two branches producedNaNor invalidDatevalues for the same entry.Changes
_sameValueZeroutility.literaland_merge.value,notValue,values, andnotValues.-0and+0.Datevalues in the valueactions.
Tests
Adds regression coverage for:
NaNmatching itself in every affected API.-0and+0remaining interchangeable.Dateobjects.Verification
pnpm -C library test— 520 test files and 4514 tests passed.pnpm -C library lint— passed without errors.pnpm -C library format.check— passed.Acknowledgements
This consolidated fix builds on the work and investigation in:
Closes #1477
Closes #1517
Closes #1529
Closes #1535
Thank you all for identifying the edge cases, proposing fixes, and adding
regression coverage that informed this unified change.
Summary by CodeRabbit
Bug Fixes
NaNand positive or negative zero.Tests
NaN, signed zero, invalid dates, and value-matching behavior across validation and comparison scenarios.