Fix/dirty dates #132
Fix/dirty dates #132
Conversation
lead to wrong display of dirty states
WalkthroughThe pull request introduces enhancements to the test coverage for the form management logic and utility functions in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report for Dev Tools React Coverage
File CoverageNo changed files found. |
Coverage Report for Form Core Coverage
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Coverage Report for Form React Coverage
File CoverageNo changed files found. |
Coverage Report for Validation Adapter Zod Coverage
File CoverageNo changed files found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/form-core/src/utils/access.utils.spec.ts (2)
50-53: Consider enhancing the date test coverage.While the test correctly verifies object identity, consider adding assertions to validate the actual date value and timestamp to ensure complete correctness.
it('should return dates', () => { const date = new Date() expect(getValueAtPath({ date }, 'date')).toBe(date) + expect(getValueAtPath({ date }, 'date').getTime()).toBe(date.getTime()) })
153-160: Consider making date value preservation more explicit.The test effectively verifies object copying and instance separation. However, it could be more explicit about preserving the actual date value.
it('should copy dates', () => { const date = new Date() const obj = { date } const copy = deepCopy(obj) expect(copy).toStrictEqual(obj) expect(copy).not.toBe(obj) expect(copy.date).toStrictEqual(obj.date) + expect(copy.date.getTime()).toBe(obj.date.getTime()) + expect(copy.date).not.toBe(obj.date) // Explicitly verify date was copied })packages/form-core/src/FieldGroupLogic.spec.ts (1)
208-232: Consider adding test cases for date edge cases.The current test effectively covers the basic date modification scenario. Consider enhancing coverage with additional test cases for:
- Setting date to null/undefined
- Modifying individual date components
- Multiple date field changes in sequence
Example test case structure:
it('should handle null/undefined date values correctly', () => { const form = new FormLogic({ defaultValues: { date: new Date(2024, 1, 1) } }) // Test setting to null // Test setting to undefined // Verify dirty states }) it('should track dirty state when modifying date components', () => { const form = new FormLogic({ defaultValues: { date: new Date(2024, 1, 1) } }) // Test modifying year // Test modifying month // Test modifying day // Verify dirty states for each change })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/form-core/src/FieldGroupLogic.spec.ts(1 hunks)packages/form-core/src/utils/access.utils.spec.ts(2 hunks)packages/form-core/src/utils/access.utils.ts(1 hunks)
🔇 Additional comments (3)
packages/form-core/src/utils/access.utils.ts (1)
158-160: LGTM! Correct implementation of Date object deep copying.
The implementation properly creates a new Date instance using getTime(), which ensures value equality while breaking reference equality. This fixes the dirty state detection for dates.
Let's verify the usage of dates in forms across the codebase:
✅ Verification successful
Date object deep copying implementation is correctly used across the codebase
The verification confirms that the Date handling in deepCopy is appropriate for the codebase's usage patterns:
- Test files properly verify Date object state management in forms
- Forms handle Date fields with appropriate transformations (string-to-Date and vice-versa)
- Date objects are used in form validation (max date constraints)
- The implementation correctly supports both direct Date field usage and Date objects within nested form structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for Date object usage in form-related code
# Expected: Find form field definitions or state management code using Date objects
# Look for Date type definitions in form contexts
ast-grep --pattern 'type $_ = {
$$$
$_: Date
$$$
}'
# Look for Date instantiation in form-related code
rg -l 'new Date' | xargs rg -l 'form|Form'
Length of output: 517
Script:
#!/bin/bash
# Let's examine the actual usage of dates in form contexts to verify the implementation
# Check test files for Date handling scenarios
rg -A 5 -B 5 "new Date" "packages/form-core/src/FormLogic.spec.ts" "packages/form-core/src/FieldGroupLogic.spec.ts"
# Check form components for Date field usage patterns
rg -A 5 -B 5 "new Date" "examples/react/async-form-signals/src/components/application/UserForm.tsx" "examples/react/simple-form-signals/src/App.tsx"
# Look for any date-related validation or transformation logic
ast-grep --pattern 'function $_($$_) {
$$$
new Date
$$$
}'
Length of output: 5718
packages/form-core/src/utils/access.utils.spec.ts (1)
153-160: Verify the deepCopy implementation for dates.
The test changes look good and align with the PR objective. Let's verify the corresponding implementation changes.
✅ Verification successful
Implementation correctly handles date objects
The implementation properly creates a new Date instance using getTime(), which ensures deep copying of date objects as verified by the test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of date handling in deepCopy function
# Test: Search for date handling in the implementation
rg -A 5 "instanceof Date" packages/form-core/src/utils/access.utils.ts
# Test: Look for the deepCopy function implementation
ast-grep --pattern $'function deepCopy($_) {
$$$
}'
Length of output: 363
packages/form-core/src/FieldGroupLogic.spec.ts (1)
208-232: LGTM! Test case effectively verifies date handling in dirty state.
The test case thoroughly validates the form's dirty state behavior when modifying date fields:
- Properly initializes with different dates for comparison
- Verifies initial clean state across form, group, and fields
- Confirms dirty state updates when date value changes
Pull Request Template
TODO
Summary by CodeRabbit
Dateobjects in thedeepCopyfunction to ensure accurate copying without reference issues.