fix(DatePicker): reset time after clearing value#2636
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDatePickerRoot zeroes time components when initializing a selected date into an empty modelValue via a new resetTime helper. Tests were added and the test helper typing widened to assert emitted onUpdate:modelValue values have hour/minute/second/millisecond reset; the zoned test verifies timeZone is preserved. ChangesDatePicker time reset on initial selection
Sequence Diagram(s)sequenceDiagram
participant DatePickerCalendar
participant DatePickerRoot
participant Consumer
DatePickerCalendar->>DatePickerRoot: select(date)
DatePickerRoot->>DatePickerRoot: resetTime(date) -> normalizedDate
DatePickerRoot->>Consumer: emit onUpdate:modelValue(normalizedDate)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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
🧹 Nitpick comments (1)
packages/core/src/DatePicker/DatePicker.test.ts (1)
255-284: ⚖️ Poor tradeoffConsider expanding test coverage for time reset behavior.
The test correctly validates the core fix (hour and minute reset to 0), but could be more comprehensive:
- The
resetTimefunction also resetssecond, but the test doesn't verify this field.- If
millisecondshould be reset (pending verification), that should also be tested.- The test only covers
CalendarDateTime; consider adding coverage forZonedDateTimeas well.💡 Example expansion
const selectedValue = emittedValues.at(-1) expect(selectedValue).toBeInstanceOf(CalendarDateTime) expect((selectedValue as CalendarDateTime).hour).toBe(0) expect((selectedValue as CalendarDateTime).minute).toBe(0) + expect((selectedValue as CalendarDateTime).second).toBe(0)Consider a similar test case with
ZonedDateTimeandgranularity: 'second'to ensure time reset works across all time-enabled DateValue types.🤖 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 `@packages/core/src/DatePicker/DatePicker.test.ts` around lines 255 - 284, Test only asserts hour and minute reset for CalendarDateTime; expand assertions and add a complementary test for ZonedDateTime. In the existing test (uses CalendarDateTime, emittedValues, granularity: 'minute'), also assert that second (and millisecond if intended) are reset to 0 after selection; then add a new test case using ZonedDateTime with granularity: 'second' to verify resetTime behavior resets hour, minute, second (and millisecond if applicable) for ZonedDateTime as well.
🤖 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 `@packages/core/src/DatePicker/DatePickerRoot.vue`:
- Around line 143-151: The resetTime function currently clears hour, minute, and
second but omits milliseconds; update the resetTime(DateValue) helper so that
when 'hour' exists it also sets millisecond to 0 (i.e., include millisecond: 0
in the object passed to date.set) to fully normalize CalendarDateTime and
ZonedDateTime instances from `@internationalized/date`; keep the check on 'hour'
and return date unchanged otherwise, and update any type assumptions if
necessary to satisfy the DateValue.set signature.
---
Nitpick comments:
In `@packages/core/src/DatePicker/DatePicker.test.ts`:
- Around line 255-284: Test only asserts hour and minute reset for
CalendarDateTime; expand assertions and add a complementary test for
ZonedDateTime. In the existing test (uses CalendarDateTime, emittedValues,
granularity: 'minute'), also assert that second (and millisecond if intended)
are reset to 0 after selection; then add a new test case using ZonedDateTime
with granularity: 'second' to verify resetTime behavior resets hour, minute,
second (and millisecond if applicable) for ZonedDateTime as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62eaa00d-e74a-49d0-909b-02951142b47e
📒 Files selected for processing (2)
packages/core/src/DatePicker/DatePicker.test.tspackages/core/src/DatePicker/DatePickerRoot.vue
…tePicker/DatePickerRoot.vue Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Nice catch on the root cause. Two things to address before merging: 1. Regression risk in 2. Redundant |
|
Addressed, thanks! Moved the time reset to the stale placeholder path when |
|
Actionable comments posted: 0 |
🔗 Linked issue
Closes #2599
❓ Type of change
📚 Description
DatePickerRootcould preserve a previously selected time after its model value was cleared. When a user selected a new date through the calendar with time granularity enabled, the calendar emitted a value based on the placeholder that still contained the old time.This resets the time fields to
00:00:00when selecting a date while the current model value is empty, and adds a regression test covering the clear-then-select flow.📸 Screenshots (if appropriate)
N/A
📝 Checklist
Summary by CodeRabbit