fix: issue w/ timepicker (#26696)

Co-authored-by: lyzno1 <yuanyouhuilyz@gmail.com> Co-authored-by: lyzno1 <92089059+lyzno1@users.noreply.github.com>
2025-10-10 13:17:33 +08:00
parent 54db4c176a
commit cf1778e696
5 changed files with 388 additions and 41 deletions
--- a/web/app/components/base/date-and-time-picker/time-picker/index.spec.tsx
+++ b/web/app/components/base/date-and-time-picker/time-picker/index.spec.tsx
@@ -0,0 +1,95 @@
+import React from 'react'
+import { fireEvent, render, screen } from '@testing-library/react'
+import TimePicker from './index'
+import dayjs from '../utils/dayjs'
+import { isDayjsObject } from '../utils/dayjs'
+
+jest.mock('react-i18next', () => ({
+  useTranslation: () => ({
+    t: (key: string) => {
+      if (key === 'time.defaultPlaceholder') return 'Pick a time...'
+      if (key === 'time.operation.now') return 'Now'
+      if (key === 'time.operation.ok') return 'OK'
+      if (key === 'common.operation.clear') return 'Clear'
+      return key
+    },
+  }),
+}))
+
+jest.mock('@/app/components/base/portal-to-follow-elem', () => ({
+  PortalToFollowElem: ({ children }: { children: React.ReactNode }) => <div>{children}</div>,
+  PortalToFollowElemTrigger: ({ children, onClick }: { children: React.ReactNode, onClick: (e: React.MouseEvent) => void }) => (
+    <div onClick={onClick}>{children}</div>
+  ),
+  PortalToFollowElemContent: ({ children }: { children: React.ReactNode }) => (
+    <div data-testid="timepicker-content">{children}</div>
+  ),
+}))
+
+jest.mock('./options', () => () => <div data-testid="time-options" />)
+jest.mock('./header', () => () => <div data-testid="time-header" />)
+
+describe('TimePicker', () => {
+  const baseProps = {
+    onChange: jest.fn(),
+    onClear: jest.fn(),
+  }
+
+  beforeEach(() => {
+    jest.clearAllMocks()
+  })
+
+  test('renders formatted value for string input (Issue #26692 regression)', () => {
+    render(
+      <TimePicker
+        {...baseProps}
+        value="18:45"
+        timezone="UTC"
+      />,
+    )
+
+    expect(screen.getByDisplayValue('06:45 PM')).toBeInTheDocument()
+  })
+
+  test('confirms cleared value when confirming without selection', () => {
+    render(
+      <TimePicker
+        {...baseProps}
+        value={dayjs('2024-01-01T03:30:00Z')}
+        timezone="UTC"
+      />,
+    )
+
+    const input = screen.getByRole('textbox')
+    fireEvent.click(input)
+
+    const clearButton = screen.getByRole('button', { name: /clear/i })
+    fireEvent.click(clearButton)
+
+    const confirmButton = screen.getByRole('button', { name: 'OK' })
+    fireEvent.click(confirmButton)
+
+    expect(baseProps.onChange).toHaveBeenCalledTimes(1)
+    expect(baseProps.onChange).toHaveBeenCalledWith(undefined)
+    expect(baseProps.onClear).not.toHaveBeenCalled()
+  })
+
+  test('selecting current time emits timezone-aware value', () => {
+    const onChange = jest.fn()
+    render(
+      <TimePicker
+        {...baseProps}
+        onChange={onChange}
+        timezone="America/New_York"
+      />,
+    )
+
+    const nowButton = screen.getByRole('button', { name: 'Now' })
+    fireEvent.click(nowButton)
+
+    expect(onChange).toHaveBeenCalledTimes(1)
+    const emitted = onChange.mock.calls[0][0]
+    expect(isDayjsObject(emitted)).toBe(true)
+    expect(emitted?.utcOffset()).toBe(dayjs().tz('America/New_York').utcOffset())
+  })
+})
--- a/web/app/components/base/date-and-time-picker/time-picker/index.tsx
+++ b/web/app/components/base/date-and-time-picker/time-picker/index.tsx
@@ -1,6 +1,13 @@
 import React, { useCallback, useEffect, useRef, useState } from 'react'
-import type { Period, TimePickerProps } from '../types'
-import dayjs, { cloneTime, getDateWithTimezone, getHourIn12Hour } from '../utils/dayjs'
+import type { Dayjs } from 'dayjs'
+import { Period } from '../types'
+import type { TimePickerProps } from '../types'
+import dayjs, {
+  getDateWithTimezone,
+  getHourIn12Hour,
+  isDayjsObject,
+  toDayjs,
+} from '../utils/dayjs'
 import {
  PortalToFollowElem,
  PortalToFollowElemContent,
@@ -13,6 +20,11 @@ import { useTranslation } from 'react-i18next'
 import { RiCloseCircleFill, RiTimeLine } from '@remixicon/react'
 import cn from '@/utils/classnames'

+const to24Hour = (hour12: string, period: Period) => {
+  const normalized = Number.parseInt(hour12, 10) % 12
+  return period === Period.PM ? normalized + 12 : normalized
+}
+
 const TimePicker = ({
  value,
  timezone,
@@ -28,7 +40,11 @@ const TimePicker = ({
  const [isOpen, setIsOpen] = useState(false)
  const containerRef = useRef<HTMLDivElement>(null)
  const isInitial = useRef(true)
-  const [selectedTime, setSelectedTime] = useState(() => value ? getDateWithTimezone({ timezone, date: value }) : undefined)
+
+  // Initialize selectedTime
+  const [selectedTime, setSelectedTime] = useState(() => {
+    return toDayjs(value, { timezone })
+  })

  useEffect(() => {
    const handleClickOutside = (event: MouseEvent) => {
@@ -39,20 +55,47 @@ const TimePicker = ({
    return () => document.removeEventListener('mousedown', handleClickOutside)
  }, [])

+  // Track previous values to avoid unnecessary updates
+  const prevValueRef = useRef(value)
+  const prevTimezoneRef = useRef(timezone)
+
  useEffect(() => {
    if (isInitial.current) {
      isInitial.current = false
+      // Save initial values on first render
+      prevValueRef.current = value
+      prevTimezoneRef.current = timezone
      return
    }
-    if (value) {
-      const newValue = getDateWithTimezone({ date: value, timezone })
-      setSelectedTime(newValue)
-      onChange(newValue)
+
+    // Only update when timezone changes but value doesn't
+    const valueChanged = prevValueRef.current !== value
+    const timezoneChanged = prevTimezoneRef.current !== timezone
+
+    // Update reference values
+    prevValueRef.current = value
+    prevTimezoneRef.current = timezone
+
+    // Skip if neither timezone changed nor value changed
+    if (!timezoneChanged && !valueChanged) return
+
+    if (value !== undefined && value !== null) {
+      const dayjsValue = toDayjs(value, { timezone })
+      if (!dayjsValue) return
+
+      setSelectedTime(dayjsValue)
+
+      if (timezoneChanged && !valueChanged)
+        onChange(dayjsValue)
+      return
    }
-    else {
-      setSelectedTime(prev => prev ? getDateWithTimezone({ date: prev, timezone }) : undefined)
-    }
-  }, [timezone])
+
+    setSelectedTime((prev) => {
+      if (!isDayjsObject(prev))
+        return undefined
+      return timezone ? getDateWithTimezone({ date: prev, timezone }) : prev
+    })
+  }, [timezone, value, onChange])

  const handleClickTrigger = (e: React.MouseEvent) => {
    e.stopPropagation()
@@ -61,8 +104,16 @@ const TimePicker = ({
      return
    }
    setIsOpen(true)
-    if (value)
-      setSelectedTime(value)
+
+    if (value) {
+      const dayjsValue = toDayjs(value, { timezone })
+      const needsUpdate = dayjsValue && (
+        !selectedTime
+        || !isDayjsObject(selectedTime)
+        || !dayjsValue.isSame(selectedTime, 'minute')
+      )
+      if (needsUpdate) setSelectedTime(dayjsValue)
+    }
  }

  const handleClear = (e: React.MouseEvent) => {
@@ -73,42 +124,68 @@ const TimePicker = ({
  }

  const handleTimeSelect = (hour: string, minute: string, period: Period) => {
-    const newTime = cloneTime(dayjs(), dayjs(`1/1/2000 ${hour}:${minute} ${period}`))
+    const periodAdjustedHour = to24Hour(hour, period)
+    const nextMinute = Number.parseInt(minute, 10)
    setSelectedTime((prev) => {
-      return prev ? cloneTime(prev, newTime) : newTime
+      const reference = isDayjsObject(prev)
+        ? prev
+        : (timezone ? getDateWithTimezone({ timezone }) : dayjs()).startOf('minute')
+      return reference
+        .set('hour', periodAdjustedHour)
+        .set('minute', nextMinute)
+        .set('second', 0)
+        .set('millisecond', 0)
    })
  }

+  const getSafeTimeObject = useCallback(() => {
+    if (isDayjsObject(selectedTime))
+      return selectedTime
+    return (timezone ? getDateWithTimezone({ timezone }) : dayjs()).startOf('day')
+  }, [selectedTime, timezone])
+
  const handleSelectHour = useCallback((hour: string) => {
-    const time = selectedTime || dayjs().startOf('day')
+    const time = getSafeTimeObject()
    handleTimeSelect(hour, time.minute().toString().padStart(2, '0'), time.format('A') as Period)
-  }, [selectedTime])
+  }, [getSafeTimeObject])

  const handleSelectMinute = useCallback((minute: string) => {
-    const time = selectedTime || dayjs().startOf('day')
+    const time = getSafeTimeObject()
    handleTimeSelect(getHourIn12Hour(time).toString().padStart(2, '0'), minute, time.format('A') as Period)
-  }, [selectedTime])
+  }, [getSafeTimeObject])

  const handleSelectPeriod = useCallback((period: Period) => {
-    const time = selectedTime || dayjs().startOf('day')
+    const time = getSafeTimeObject()
    handleTimeSelect(getHourIn12Hour(time).toString().padStart(2, '0'), time.minute().toString().padStart(2, '0'), period)
-  }, [selectedTime])
+  }, [getSafeTimeObject])

  const handleSelectCurrentTime = useCallback(() => {
    const newDate = getDateWithTimezone({ timezone })
    setSelectedTime(newDate)
    onChange(newDate)
    setIsOpen(false)
-  }, [onChange, timezone])
+  }, [timezone, onChange])

  const handleConfirm = useCallback(() => {
-    onChange(selectedTime)
+    const valueToEmit = isDayjsObject(selectedTime) ? selectedTime : undefined
+    onChange(valueToEmit)
    setIsOpen(false)
-  }, [onChange, selectedTime])
+  }, [selectedTime, onChange])

  const timeFormat = 'hh:mm A'
-  const displayValue = value?.format(timeFormat) || ''
-  const placeholderDate = isOpen && selectedTime ? selectedTime.format(timeFormat) : (placeholder || t('time.defaultPlaceholder'))
+
+  const formatTimeValue = useCallback((timeValue: string | Dayjs | undefined): string => {
+    if (!timeValue) return ''
+
+    const dayjsValue = toDayjs(timeValue, { timezone })
+    return dayjsValue?.format(timeFormat) || ''
+  }, [timezone])
+
+  const displayValue = formatTimeValue(value)
+
+  const placeholderDate = isOpen && isDayjsObject(selectedTime)
+    ? selectedTime.format(timeFormat)
+    : (placeholder || t('time.defaultPlaceholder'))

  const inputElem = (
    <input
@@ -146,6 +223,8 @@ const TimePicker = ({
                'hidden h-4 w-4 shrink-0 text-text-quaternary',
                (displayValue || (isOpen && selectedTime)) && 'hover:text-text-secondary group-hover:inline-block',
              )}
+              role='button'
+              aria-label={t('common.operation.clear')}
              onClick={handleClear}
            />
          </div>