feat(time-field): new TEDI-ready component #25#601
Open
airikej wants to merge 20 commits into
Open
Conversation
21 tasks
|
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 TimeField and TimePicker UI with wheel/grid pickers, time parsing/snapping/scrolling helpers, styles, tests, stories, label translations, and barrel exports; components support controlled/uncontrolled modes, native-picker fallback, Floating UI popovers/dropdowns, keyboard navigation, and minute-step snapping. Changes
Sequence Diagram(s)sequenceDiagram
participant User as rgba(52,152,219,0.5) User
participant TimeField as rgba(46,204,113,0.5) TimeField
participant FloatingUI as rgba(155,89,182,0.5) FloatingUI
participant TimePicker as rgba(241,196,15,0.5) TimePicker
participant TimeWheel as rgba(230,126,34,0.5) TimeWheel
User->>TimeField: Click icon or input
TimeField->>FloatingUI: request open popover/dropdown
FloatingUI->>TimePicker: render picker (wheel/grid)
TimePicker->>TimeWheel: provide hours/minutes & selection
TimeWheel->>TimeWheel: render wheels and handle scroll/click
User->>TimeWheel: scroll or click item
TimeWheel->>TimePicker: onChange(hour, minute)
TimePicker->>TimeField: onChange("HH:mm")
TimeField->>User: update input value / close popover
sequenceDiagram
participant User as rgba(52,152,219,0.5) User
participant TimeField as rgba(46,204,113,0.5) TimeField
participant FloatingUI as rgba(155,89,182,0.5) FloatingUI
participant TimePicker as rgba(241,196,15,0.5) TimePicker
participant TimeGrid as rgba(230,126,34,0.5) TimeGrid
User->>TimeField: Click icon/input (availableTimes present)
TimeField->>FloatingUI: open dropdown/popover
FloatingUI->>TimePicker: render with grid variant
TimePicker->>TimeGrid: pass availableTimes + variant
TimeGrid->>TimeGrid: render buttons or radios
User->>TimeGrid: click time option
TimeGrid->>TimePicker: onSelect(time)
TimePicker->>TimeField: onChange("HH:mm")
TimeField->>User: update input value / close popover
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
src/tedi/components/form/time-field/time-field-helpers.ts (3)
32-34: PreferNumber.isNaNover globalisNaN.
isNaNcoerces its argument, so inputs likeisNaN(undefined)returntruewhileisNaN(' ')returnsfalse.Number(target)here should always produce a number, but usingNumber.isNaNis the safer and more idiomatic check.♻️ Proposed change
- const t = Number(target); - - if (isNaN(t)) return mins[0]; + const t = Number(target); + + if (Number.isNaN(t)) return mins[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field-helpers.ts` around lines 32 - 34, Replace the global isNaN check with Number.isNaN to avoid coercion issues: after converting target to a number (const t = Number(target)) update the conditional in the time-field helper to use Number.isNaN(t) when returning mins[0]; locate this logic in the helper (the const t = Number(target) / if (isNaN(t)) return mins[0] block) and swap to Number.isNaN to perform a safe numeric NaN check.
1-1: Excluding helpers from coverage undermines the 80% coverage requirement.This module contains non-trivial branching (
findClosestMinutetie-break,normalizeTimelength/colon branches, validation regex). Adding/* istanbul ignore file */hides that logic from the coverage report, even though bothtime-picker.spec.tsxandtime-wheel.spec.tsxmock these helpers rather than exercising them. Consider dropping the pragma and adding direct unit tests for the pure helpers — they're cheap to test and will keep thesrc/tedi/components/**80% threshold meaningful.As per coding guidelines: "Coverage threshold of 80% must be maintained for branches, functions, lines, and statements in
src/tedi/components/**/*.{js,ts,tsx}files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field-helpers.ts` at line 1, Remove the top-level coverage exclusion and add unit tests that exercise the helper functions directly: delete the `/* istanbul ignore file */` pragma in the module and create tests that cover `findClosestMinute` (including tie-break behavior), `normalizeTime` (all length/colon branches), and the validation regex/path (e.g., `isTimeValid` or equivalent) to hit branching and edge cases so the helpers contribute to the 80% threshold; ensure tests assert expected outputs for typical, boundary, and invalid inputs to fully cover branches and logic.
110-141:normalizeTimereturn type mixes''andnullfor "not a time".Empty input returns the empty string, while every other un-normalizable input returns
null. Callers will have to handle three states (string,'',null) instead of two. Consider normalizing to a single sentinel (e.g. always returnnullon non-time input and let the caller coerce to''), or document the contract explicitly in the JSDoc.Also note that 1–2 digit inputs (e.g.
"9","12") fall through tonull— if that's intentional for a typing-in-progress UX it's worth a comment; otherwise"9"→"09:00"might be a friendlier normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field-helpers.ts` around lines 110 - 141, normalizeTime currently returns '' for empty input but null for other non-times, forcing callers to handle three states; change normalizeTime to use a single sentinel (prefer null) for any non-time input by returning null instead of '' on empty input and update any callers/tests accordingly; additionally decide and implement desired behavior for 1–2 digit inputs (either document as intentionally ignored or normalize e.g. "9" -> "09:00" and "12" -> "12:00") and annotate the function (or add a brief inline comment) referencing normalizeTime and isValidTime so future readers know the chosen contract.src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx (1)
83-83: Avoid using ternary for side effects.Using a conditional expression purely to assign is harder to read and trips several lint rules (
no-unused-expressions). Prefer a plainif/else.♻️ Proposed change
- isHour ? (isProgrammaticScrollHour.current = true) : (isProgrammaticScrollMinute.current = true); + if (isHour) { + isProgrammaticScrollHour.current = true; + } else { + isProgrammaticScrollMinute.current = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx` at line 83, Replace the ternary used for side effects with a clear if/else: instead of using "isHour ? (isProgrammaticScrollHour.current = true) : (isProgrammaticScrollMinute.current = true)", use an if (isHour) { isProgrammaticScrollHour.current = true } else { isProgrammaticScrollMinute.current = true } so the intent is explicit and avoids using a conditional expression for assignments to isProgrammaticScrollHour.current and isProgrammaticScrollMinute.current.src/tedi/components/form/time-field/time-field.spec.tsx (2)
162-167:renders dropdown variantdoesn't actually assert it's in a dropdown.Because the
Dropdown.Contentmock unconditionally renders its children,getByText('09:00')would pass whether the dropdown is open or closed — this test gives no real signal beyond "availableTimesreach the DOM". Consider asserting against the mocked trigger/content test ids or, better, dropping the mock and using the realDropdownto validate open/close behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field.spec.tsx` around lines 162 - 167, The test "renders dropdown variant when availableTimes + dropdown" for TimeField is only asserting DOM presence via getByText and not that the times are inside the dropdown (Dropdown.Content mock always renders children); update the test to either assert against the mocked trigger/content test ids (e.g., check for the mocked Dropdown trigger id and content id instead of plain getByText) or remove the Dropdown mock and render the real Dropdown to assert open/close behavior by simulating user events (click trigger) and then asserting the times appear only after opening when availableTimesVariant="dropdown" on the TimeField component.
52-76: Prefer semantic queries in the TextField mock.The mocked textfield emits a real
<input>— usingscreen.getByRole('textbox')(and giving the trigger<button>anaria-labelso you can query it by role/name) would align the spec with the coding-guideline preference for semantic queries, and would also survive future DOM restructurings.As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field.spec.tsx` around lines 52 - 76, The mock for the TextField (the jest.mock('../textfield/textfield') block) uses data-testid attributes and a plain <input>, which prevents tests from using semantic queries; update the mock to render an accessible text input (e.g., type="text" so it maps to role="textbox") and give the trigger button an aria-label (e.g., aria-label="icon") instead of data-testid, and ensure the mock still calls props.onChange and props.onIconClick so tests can switch to screen.getByRole('textbox') and screen.getByRole('button', { name: /icon/i }) in the spec.src/tedi/components/form/time-picker/time-picker.stories.tsx (1)
30-68: Inconsistent story styles — mix ofStoryObj+Templateand inlineStoryFn.
Default,WithInitialValue, andPredefinedSlotsareStoryObjs, whileControlledExampleis typed asStoryFn<TimePickerProps>. Also,PredefinedSlots.renderandControlledExampledon't go throughTemplate, so arg-controls changes in Storybook won't re-sync local state. Consider routing all stories through a singleTemplate(or a shared controlled wrapper) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/time-picker.stories.tsx` around lines 30 - 68, The stories mix StoryObj and StoryFn and bypass the shared Template which prevents Storybook arg-controls from syncing local state; make all stories use the same Template (or a small shared controlled wrapper) instead of individual render functions: change ControlledExample, PredefinedSlots, Default and WithInitialValue to be StoryObj-style entries that pass props into Template (e.g., args: { value, availableTimes, stepMinutes }) or have Template return a controlled TimePicker wrapper that reads args and calls onChange to update args-driven state; ensure the exported story types are consistent (use StoryObj<TimePickerProps> for all) and reference the existing Template, TimePicker, ControlledExample, PredefinedSlots and TimePickerProps symbols when applying the change.src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx (1)
182-194: Second cleanup test duplicates the first."cleans up retry + scroll timers on unmount (full cleanup branch)" only advances timers by 50ms without triggering any scroll/retry first, so neither
scrollTimeoutHournorretryTimeoutHourare ever set. The assertion (clearScrollTimeoutcalled) is satisfied purely by the unmountuseEffectcleanup invokingclearScrollTimeout(undefined)— same as the previous test. To meaningfully cover the retry branch, dispatch a scroll event (soscrollTimeoutHour.currentis populated) beforeunmount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx` around lines 182 - 194, The test "cleans up retry + scroll timers on unmount (full cleanup branch)" currently never sets scroll or retry timers; update the test for the TimeWheel component to dispatch a scroll event (e.g., fireEvent.scroll or element.dispatchEvent(new Event('scroll'))) against the wheel/container so that scrollTimeoutHour (and consequently retryTimeoutHour) are populated, then advance timers as needed (jest.advanceTimersByTime) before calling unmount and asserting clearScrollTimeout was called; reference TimeWheel, scrollTimeoutHour, retryTimeoutHour, clearScrollTimeout, and unmount to locate and modify the test.src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx (1)
44-109: Prefer semantic queries over text-only lookups for buttons.
screen.getByText('10:00')resolves against any element containing the text — because theButtonmock renders a<button>,getByRole('button', { name: '10:00' })would more clearly express the assertion and catch accidental regressions (e.g. labels leaking intodivs).♻️ Example
- await user.click(screen.getByText('10:00')); + await user.click(screen.getByRole('button', { name: '10:00' }));As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx` around lines 44 - 109, The tests in time-grid.spec.tsx use screen.getByText('...') for buttons which is non-semantic; update those assertions and user interactions to use role-based queries: replace screen.getByText('09:00'|'10:00'|'11:00') and any user.click(screen.getByText(...)) with screen.getByRole('button', { name: '09:00'|'10:00'|'11:00' }) (and use getAllByRole('button') where appropriate), change the selection assertion that reads selectedButton.className by first locating the button via getByRole('button', { name: '10:00' }), and leave radio-related queries (getAllByRole('radio'), toBeChecked) as-is; ensure references to the TimeGrid render calls and user.click usages are updated accordingly.src/tedi/components/form/time-picker/time-picker.spec.tsx (1)
1-48: Prefer semantic queries and proper typing overdata-testid/any.The suite consistently relies on
data-testid/getByTestIdand usesanytyping for mocked component props (with a file-leveleslint-disable). Since the realTimeWheelandTimeGridalready expose accessible roles (listbox,button), prefergetByRole/getByLabelText, and type the mocks with the actual prop interfaces (or a narrowPick<>).As per coding guidelines: "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)" and "Use proper TypeScript generics instead of@ts-ignoreoranytypes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/time-picker.spec.tsx` around lines 1 - 48, Update the test mocks and queries to use semantic queries and proper typings: replace data-testid usages in this spec with role/label queries (e.g., use getByRole('listbox')/getByRole('button') or getByLabelText for inputs instead of getByTestId) and remove the file-level eslint-disable/@ts-expect-any by typing the mocked components with the real prop interfaces (or a narrow Pick like Pick<TimeWheelProps,'hours'|'minutes'|'selectedHour'|'selectedMinute'|'onChange'> and Pick<TimeGridProps,'times'|'value'|'onSelect'>) when defining the jest.mock implementations for TimeWheel and TimeGrid; also update the mocked time-wheel markup to render accessible roles (e.g., role="listbox" for hour/minute lists and role="button" for actions) so the tests can use getByRole/getByLabelText against TimePicker behaviour instead of data-testid.src/tedi/components/form/time-field/time-field.tsx (1)
178-182:setReferenceeffect runs only once; prefer a callback ref.
refsfromuseFloatingis stable, so this effect is effectively mount-only. IftextFieldRef.current.innerisn't populated on first commit, or if the inner element is remounted (e.g. conditional rendering insideTextField), the floating reference will go stale and the popover position will be wrong. A callback ref is both simpler and correct across remounts.♻️ Suggested refactor
- const textFieldRef = React.useRef<TextFieldForwardRef | null>(null); - - useEffect(() => { - if (textFieldRef.current?.inner) { - refs.setReference(textFieldRef.current.inner); - } - }, [refs]); + const textFieldRef = React.useRef<TextFieldForwardRef | null>(null); + const setTextFieldRef = React.useCallback( + (node: TextFieldForwardRef | null) => { + textFieldRef.current = node; + refs.setReference(node?.inner ?? null); + }, + [refs] + );and use
ref={setTextFieldRef}on theTextField.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-field/time-field.tsx` around lines 178 - 182, Replace the mount-only useEffect that calls refs.setReference with a callback ref that sets the local textFieldRef and updates the floating refs on every mount/unmount; implement a function (e.g. setTextFieldRef) that takes the DOM node, assigns it to textFieldRef.current.inner (or clears it when node is null) and calls refs.setReference(node || null), then pass that callback into the TextField via ref={setTextFieldRef} so the floating reference stays correct across remounts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tedi/components/form/time-field/time-field.stories.tsx`:
- Around line 124-130: Two TimeField instances in the OnClickType story share
the same DOM id ("calendar-button-trigger"), causing duplicate IDs and broken
label associations; update the TimeField props so each has a unique id (e.g.,
change the first TimeField id used with timePickerTrigger="button" to
"time-button-trigger" and the second with timePickerTrigger="input" to
"time-input-trigger") to restore correct label-for behavior.
In `@src/tedi/components/form/time-field/time-field.tsx`:
- Line 108: The exported React component TimeField is missing a displayName; set
TimeField.displayName = 'TimeField' after the component declaration to follow
TEDI conventions and improve React DevTools output, e.g., add a statement
assigning the string 'TimeField' to the TimeField.displayName property right
after the TimeField function definition.
- Around line 133-134: The component currently auto-selects availableTimes?.[0]
when neither value nor defaultValue is provided; change the internal state
initialization in the useState call for internalValue to remove that fallback
(use value ?? defaultValue ?? ''), so the placeholder (e.g., "Choose time") is
shown for uncontrolled mode; keep the isControlled boolean and setInternalValue
usage intact, and if you prefer automatic selection instead, implement a
useEffect that, when uncontrolled and availableTimes?.[0] exists, calls
setInternalValue(availableTimes[0]) and invokes onChange(availableTimes[0]) so
parent consumers stay in sync.
In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx`:
- Line 35: The TimeGrid component is exported as an anonymous arrow function and
lacks a displayName; change the export to a named function (e.g., function
TimeGrid(props) { ... }) and then set TimeGrid.displayName = 'TimeGrid'; apply
the same pattern for the TimePicker and TimeField components (convert to named
functions if necessary and assign TimePicker.displayName = 'TimePicker' and
TimeField.displayName = 'TimeField') so all exported TEDI components include the
required displayName property.
- Around line 46-64: The TimeGrid component renders ChoiceGroup with hardcoded
id "time-picker-group" and name "time-grid", causing duplicate DOM IDs and
shared radio groups; update TimeGrid to derive a stable, instance-scoped
identifier (e.g., call React's useId() or accept a required id/name prop) and
pass that unique value into ChoiceGroup's id and name props (reference TimeGrid
and ChoiceGroup, and the onChange/value props remain unchanged) so each TimeGrid
instance gets its own id/name and radio groups don't collide.
In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx`:
- Around line 263-266: The focus/scroll logic in TimeWheel uses
document.getElementById(`${type}-${nextIndex}`) which produces non-unique IDs
across multiple TimeWheel instances and the list items are missing
role="option"; change the id generation to use a component-scoped unique prefix
(e.g., useId() or a prop-derived namespace) when creating item ids and update
the lookup in the focusNext/focusPrev logic to use that namespaced id instead of
`${type}-${nextIndex}` (refer to the TimeWheel component and the getElementById
usage), and add role="option" and aria-selected attributes to each rendered item
(replace the visual-only --selected state with aria-selected for the selected
item) so listbox semantics are correct and keyboard navigation targets only
items within the same TimeWheel instance.
- Around line 218-266: handleColumnKeyDown currently only moves DOM focus and
never updates selection — and Enter/Space uses the stale currentIndex. Update
the handler so arrow/Home/End/PageUp/PageDown both call
onSelect(list[nextIndex]) after computing nextIndex (before focusing/scrolling)
so keyboard navigation commits the new value, and change the Enter/Space branch
to call onSelect(list[nextIndex >= 0 ? nextIndex : currentIndex]) (or track a
focusedIndex state and use it) instead of onSelect(list[currentIndex]);
reference function handleColumnKeyDown and variables list, selected,
currentIndex, nextIndex, and onSelect.
- Around line 112-138: The useLayoutEffect currently only runs on mount
(dependency array is []) but reads hours, minutes, selectedHour and
selectedMinute, causing desync; update the dependency array to include hours,
minutes, selectedHour, selectedMinute so the effect re-runs when they change and
re-initializes the wheel position via forceScrollTo(hourRef, hourIndex, true)
and forceScrollTo(minuteRef, minuteIndex, false); ensure the cleanup still
clears retryTimeoutHour.current and retryTimeoutMinute.current and keep
resetting isProgrammaticScrollHour/currentMinute and
lastHourIndex/lastMinuteIndex as before to avoid stale refs and satisfy
react-hooks/exhaustive-deps.
- Around line 284-311: aria-activedescendant in TimeWheel points to
`hour-${selectedHour}` / `minute-${selectedMinute}` while items use
`id={`hour-${idx}`}` / `minute-${idx}` causing mismatch when minutes are stepped
(e.g. stepMinutes=5) and ID collisions across instances; fix by deriving the
active item id from the rendered index rather than the value (use the index in
the `minutes`/`hours` arrays), i.e. make `aria-activedescendant` reference
`minute-${activeMinuteIndex}` / `hour-${activeHourIndex}` (compute active index
by finding the index of `selectedMinute`/`selectedHour` in `minutes`/`hours`),
and namespace IDs with React's `useId()` (or a generated prefix) so IDs like
`minute-{prefix}-{idx}` / `hour-{prefix}-{idx}` are unique per TimeWheel
instance and update `id={`minute-${idx}`}` / `id={`hour-${idx}`}` and any
handlers (`handleHourClick`, `handleMinuteScroll`, `onKeyDown` via
`handleColumnKeyDown`) accordingly.
In `@src/tedi/components/form/time-picker/time-picker.spec.tsx`:
- Around line 107-117: The two tests in time-picker.spec.tsx ("applies className
to TimeGrid" and "passes gridVariant to TimeGrid") only assert TimeGrid renders
and don't verify prop forwarding; update the TimeGrid mock used in these tests
so it exposes received props (e.g. render a simple element with data-classname
and data-variant attributes) and then assert that <TimePicker ...
className="custom-class"> results in the mocked TimeGrid receiving that
className and that <TimePicker ... gridVariant="radio"> results in the mocked
TimeGrid receiving variant/gridVariant; alternatively remove these ineffective
stubs—locate the TimePicker component and its TimeGrid mock in the spec and
change the mock to surface props for assertions.
In `@src/tedi/components/form/time-picker/time-picker.tsx`:
- Around line 38-48: The prop availableTimesView declared on TimePickerProps is
never used in the TimePicker component; either remove it from the public props
or wire it into the component render logic — update the TimePicker component to
destructure availableTimesView (alongside gridVariant and availableTimes) and
use it to choose between rendering the grid vs dropdown (the same branching
currently driven implicitly by gridVariant/availableTimes), or remove
availableTimesView from TimePickerProps and related JSDoc so the API matches
behavior; ensure you update any references to availableTimesVariant/TimeField if
needed and keep gridVariant semantics for the grid variant selection.
- Line 56: The exported React component TimePicker lacks a displayName; after
the TimePicker declaration (the exported const TimePicker:
React.FC<TimePickerProps> = ...) add a static displayName assignment (e.g.
TimePicker.displayName = 'TimePicker') so the component has an explicit
displayName for debugging and tooling.
---
Nitpick comments:
In `@src/tedi/components/form/time-field/time-field-helpers.ts`:
- Around line 32-34: Replace the global isNaN check with Number.isNaN to avoid
coercion issues: after converting target to a number (const t = Number(target))
update the conditional in the time-field helper to use Number.isNaN(t) when
returning mins[0]; locate this logic in the helper (the const t = Number(target)
/ if (isNaN(t)) return mins[0] block) and swap to Number.isNaN to perform a safe
numeric NaN check.
- Line 1: Remove the top-level coverage exclusion and add unit tests that
exercise the helper functions directly: delete the `/* istanbul ignore file */`
pragma in the module and create tests that cover `findClosestMinute` (including
tie-break behavior), `normalizeTime` (all length/colon branches), and the
validation regex/path (e.g., `isTimeValid` or equivalent) to hit branching and
edge cases so the helpers contribute to the 80% threshold; ensure tests assert
expected outputs for typical, boundary, and invalid inputs to fully cover
branches and logic.
- Around line 110-141: normalizeTime currently returns '' for empty input but
null for other non-times, forcing callers to handle three states; change
normalizeTime to use a single sentinel (prefer null) for any non-time input by
returning null instead of '' on empty input and update any callers/tests
accordingly; additionally decide and implement desired behavior for 1–2 digit
inputs (either document as intentionally ignored or normalize e.g. "9" ->
"09:00" and "12" -> "12:00") and annotate the function (or add a brief inline
comment) referencing normalizeTime and isValidTime so future readers know the
chosen contract.
In `@src/tedi/components/form/time-field/time-field.spec.tsx`:
- Around line 162-167: The test "renders dropdown variant when availableTimes +
dropdown" for TimeField is only asserting DOM presence via getByText and not
that the times are inside the dropdown (Dropdown.Content mock always renders
children); update the test to either assert against the mocked trigger/content
test ids (e.g., check for the mocked Dropdown trigger id and content id instead
of plain getByText) or remove the Dropdown mock and render the real Dropdown to
assert open/close behavior by simulating user events (click trigger) and then
asserting the times appear only after opening when
availableTimesVariant="dropdown" on the TimeField component.
- Around line 52-76: The mock for the TextField (the
jest.mock('../textfield/textfield') block) uses data-testid attributes and a
plain <input>, which prevents tests from using semantic queries; update the mock
to render an accessible text input (e.g., type="text" so it maps to
role="textbox") and give the trigger button an aria-label (e.g.,
aria-label="icon") instead of data-testid, and ensure the mock still calls
props.onChange and props.onIconClick so tests can switch to
screen.getByRole('textbox') and screen.getByRole('button', { name: /icon/i }) in
the spec.
In `@src/tedi/components/form/time-field/time-field.tsx`:
- Around line 178-182: Replace the mount-only useEffect that calls
refs.setReference with a callback ref that sets the local textFieldRef and
updates the floating refs on every mount/unmount; implement a function (e.g.
setTextFieldRef) that takes the DOM node, assigns it to
textFieldRef.current.inner (or clears it when node is null) and calls
refs.setReference(node || null), then pass that callback into the TextField via
ref={setTextFieldRef} so the floating reference stays correct across remounts.
In
`@src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx`:
- Around line 44-109: The tests in time-grid.spec.tsx use
screen.getByText('...') for buttons which is non-semantic; update those
assertions and user interactions to use role-based queries: replace
screen.getByText('09:00'|'10:00'|'11:00') and any
user.click(screen.getByText(...)) with screen.getByRole('button', { name:
'09:00'|'10:00'|'11:00' }) (and use getAllByRole('button') where appropriate),
change the selection assertion that reads selectedButton.className by first
locating the button via getByRole('button', { name: '10:00' }), and leave
radio-related queries (getAllByRole('radio'), toBeChecked) as-is; ensure
references to the TimeGrid render calls and user.click usages are updated
accordingly.
In
`@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx`:
- Around line 182-194: The test "cleans up retry + scroll timers on unmount
(full cleanup branch)" currently never sets scroll or retry timers; update the
test for the TimeWheel component to dispatch a scroll event (e.g.,
fireEvent.scroll or element.dispatchEvent(new Event('scroll'))) against the
wheel/container so that scrollTimeoutHour (and consequently retryTimeoutHour)
are populated, then advance timers as needed (jest.advanceTimersByTime) before
calling unmount and asserting clearScrollTimeout was called; reference
TimeWheel, scrollTimeoutHour, retryTimeoutHour, clearScrollTimeout, and unmount
to locate and modify the test.
In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx`:
- Line 83: Replace the ternary used for side effects with a clear if/else:
instead of using "isHour ? (isProgrammaticScrollHour.current = true) :
(isProgrammaticScrollMinute.current = true)", use an if (isHour) {
isProgrammaticScrollHour.current = true } else {
isProgrammaticScrollMinute.current = true } so the intent is explicit and avoids
using a conditional expression for assignments to
isProgrammaticScrollHour.current and isProgrammaticScrollMinute.current.
In `@src/tedi/components/form/time-picker/time-picker.spec.tsx`:
- Around line 1-48: Update the test mocks and queries to use semantic queries
and proper typings: replace data-testid usages in this spec with role/label
queries (e.g., use getByRole('listbox')/getByRole('button') or getByLabelText
for inputs instead of getByTestId) and remove the file-level
eslint-disable/@ts-expect-any by typing the mocked components with the real prop
interfaces (or a narrow Pick like
Pick<TimeWheelProps,'hours'|'minutes'|'selectedHour'|'selectedMinute'|'onChange'>
and Pick<TimeGridProps,'times'|'value'|'onSelect'>) when defining the jest.mock
implementations for TimeWheel and TimeGrid; also update the mocked time-wheel
markup to render accessible roles (e.g., role="listbox" for hour/minute lists
and role="button" for actions) so the tests can use getByRole/getByLabelText
against TimePicker behaviour instead of data-testid.
In `@src/tedi/components/form/time-picker/time-picker.stories.tsx`:
- Around line 30-68: The stories mix StoryObj and StoryFn and bypass the shared
Template which prevents Storybook arg-controls from syncing local state; make
all stories use the same Template (or a small shared controlled wrapper) instead
of individual render functions: change ControlledExample, PredefinedSlots,
Default and WithInitialValue to be StoryObj-style entries that pass props into
Template (e.g., args: { value, availableTimes, stepMinutes }) or have Template
return a controlled TimePicker wrapper that reads args and calls onChange to
update args-driven state; ensure the exported story types are consistent (use
StoryObj<TimePickerProps> for all) and reference the existing Template,
TimePicker, ControlledExample, PredefinedSlots and TimePickerProps symbols when
applying the change.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d831031-b176-4b06-8899-e84c8c0b3cf0
📒 Files selected for processing (14)
src/tedi/components/form/time-field/time-field-helpers.tssrc/tedi/components/form/time-field/time-field.module.scsssrc/tedi/components/form/time-field/time-field.spec.tsxsrc/tedi/components/form/time-field/time-field.stories.tsxsrc/tedi/components/form/time-field/time-field.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsxsrc/tedi/components/form/time-picker/time-picker.module.scsssrc/tedi/components/form/time-picker/time-picker.spec.tsxsrc/tedi/components/form/time-picker/time-picker.stories.tsxsrc/tedi/components/form/time-picker/time-picker.tsxsrc/tedi/index.ts
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/tedi/components/form/time-picker/time-picker.tsx (1)
86-88: Inner parameter names shadow outerhour/minute.The callback params
(hour, minute)shadow the same names destructured at line 63. Harmless today but easy to misread when the outer values eventually need referencing here. Rename to e.g.(h, m)or(nextHour, nextMinute).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/time-picker.tsx` around lines 86 - 88, The inline arrow callback for the TimePicker's onChange currently uses parameter names (hour, minute) that shadow the outer destructured variables named hour and minute; rename the callback parameters (e.g., to h and m or nextHour and nextMinute) in the onChange handler in time-picker.tsx so they no longer shadow the outer variables and then use those new names when constructing the string passed to the onChange prop.src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx (1)
30-32: JSDoc opened with/*instead of/**.This block won't be picked up as TSDoc for
className(other props above correctly use/**). Trivial fix:✏️ Proposed fix
- /* + /** * Additional CSS class name for custom styling */ className?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx` around lines 30 - 32, Update the comment above the className prop in time-grid.tsx to use proper TSDoc by changing the opening from /* to /** so the block is recognized as JSDoc/TSDoc; locate the comment attached to the className prop in the TimeGrid component (reference symbol: className) and replace the block comment opener so the documentation is picked up by tooling.src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx (1)
219-267:handleColumnKeyDownis re-created on every render.Because the handler is inline-curried and attached to
onKeyDownon lines 288/314, a fresh function is allocated per render. Not a correctness bug, but since this file already usesuseRefheavily to avoid churn, wrapping this inuseCallback(or hoisting the curry) would be consistent and remove a subtle re-render cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx` around lines 219 - 267, The inline-curried handler handleColumnKeyDown is recreated each render; wrap it in useCallback to stabilize its identity: replace the current const handleColumnKeyDown = (type, list, selected, onSelect) => (event) => { ... } with a useCallback version, e.g. const handleColumnKeyDown = useCallback((type: 'hour'|'minute', list: string[], selected: string, onSelect: (v: string)=>void) => (event: React.KeyboardEvent<HTMLDivElement>) => { ... }, []), so the returned curried factory is stable while still receiving list/selected/onSelect as arguments when attached to onKeyDown. Ensure you import useCallback from React.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx`:
- Line 51: The ChoiceGroup in time-grid.tsx currently uses a hardcoded English
label "Pick time"; update the TimeGrid component to accept a label (or
ariaLabel) prop and pass that prop into the ChoiceGroup's label (or aria-label)
instead of the literal string, or route the label through the app's i18n
function if present (e.g., use t('time.pick') where your i18n helper is used
elsewhere). Modify the TimeGrid component signature to include label?: string
(or ariaLabel?: string), default to a passed-in translated value if needed, and
replace the hardcoded "Pick time" in the ChoiceGroup call so screen readers
receive a localized, configurable label.
- Around line 56-61: The per-item IDs generated inside TimeGrid (the items array
built via times.map producing id: `time-${time}`) still collide across multiple
TimeGrid instances; update the item id generation to include the component's
namespaced id from useId() (the same group-level id/name) so each radio item's
id becomes unique (e.g., prefix or interpolate the groupId/namespace into the
item id), and ensure any references that use those item ids (labels/aria) use
the new namespaced id as well.
In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx`:
- Around line 74-111: The retry chain in forceScrollTo can act on stale
selectedHour/selectedMinute because timeouts/rAFs continue after props change;
add per-column generation refs (e.g., hourGenerationRef, minuteGenerationRef)
that are incremented inside the useLayoutEffect that triggers scrolling, capture
the current generation in forceScrollTo/tryScroll and bail early from the
rAF/timeout callbacks if the captured generation !== current generation; also
ensure existing retryTimeoutHour.current/retryTimeoutMinute.current are cleared
when bumping the generation so in-flight retries do not reschedule with old
targetIndex and isProgrammaticScrollHour/current flags remain consistent.
- Around line 48-55: Add a displayName to the exported TimeWheel component to
comply with the project policy: after the TimeWheel declaration, set
TimeWheel.displayName = 'TimeWheel'; (mirror how TimePicker sets its
displayName) so the exported React component has an explicit displayName
property.
- Around line 264-266: handleColumnKeyDown is trying to focus/scroll an element
using document.getElementById(`${type}-${nextIndex}`) but the rendered items use
ids namespaced with uid (id={`${uid}-hour-${idx}` / `${uid}-minute-${idx}`), so
the lookup always returns null; update the lookup to
document.getElementById(`${uid}-${type}-${nextIndex}`) (ensuring uid is in
scope) so el?.focus() and el?.scrollIntoView(...) target the correct element and
keyboard navigation works.
In `@src/tedi/components/form/time-picker/time-picker.tsx`:
- Around line 38-44: The JSDoc for the gridVariant prop incorrectly references a
non-existent availableTimesView prop; update the comment to state that the grid
variant applies when availableTimes is provided (not when availableTimesView is
set), and clarify that gridVariant ('buttons' | 'radio') controls the internal
layout of that grid. Modify the doc block above gridVariant to mention
availableTimes and that the default is 'buttons', leaving the type and name
(gridVariant) unchanged.
- Around line 52-92: TimePicker currently works only as a controlled component
(reads value and calls onChange) so add uncontrolled-mode support by introducing
internal state (e.g., const [internalValue, setInternalValue] =
useState<string>(defaultValue ?? '12:00')) and a derived currentValue = value
!== undefined ? value : internalValue; also implement a handleChange function
that calls onChange?.(newValue) and when value === undefined updates
setInternalValue(newValue); then pass currentValue and handleChange into
TimeGrid (value, onSelect) and TimeWheel (selectedHour/selectedMinute derived
from currentValue, onChange) so the component supports both controlled and
uncontrolled modes while preserving existing props (value, onChange,
defaultValue).
- Around line 63-66: The code currently uses parseTime(value || '12:00') and
silently falls back on malformed inputs; change it to first validate value with
isValidTime(value) (and handle empty string consistently with TimeField's empty
state) before calling parseTime, and if invalid then either (a) in dev mode call
console.warn with the invalid value or (b) call onChange to sync the controlled
prop to the normalized/fallback time; then compute selectedHour using a safe
fallback derived from the hours array (e.g., hours[0] instead of hardcoded '00')
and selectedMinute via findClosestMinute(minute, minutes) as before. Ensure you
reference parseTime, isValidTime, findClosestMinute, selectedHour/selectedMinute
and onChange when making the changes.
---
Nitpick comments:
In `@src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx`:
- Around line 30-32: Update the comment above the className prop in
time-grid.tsx to use proper TSDoc by changing the opening from /* to /** so the
block is recognized as JSDoc/TSDoc; locate the comment attached to the className
prop in the TimeGrid component (reference symbol: className) and replace the
block comment opener so the documentation is picked up by tooling.
In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsx`:
- Around line 219-267: The inline-curried handler handleColumnKeyDown is
recreated each render; wrap it in useCallback to stabilize its identity: replace
the current const handleColumnKeyDown = (type, list, selected, onSelect) =>
(event) => { ... } with a useCallback version, e.g. const handleColumnKeyDown =
useCallback((type: 'hour'|'minute', list: string[], selected: string, onSelect:
(v: string)=>void) => (event: React.KeyboardEvent<HTMLDivElement>) => { ... },
[]), so the returned curried factory is stable while still receiving
list/selected/onSelect as arguments when attached to onKeyDown. Ensure you
import useCallback from React.
In `@src/tedi/components/form/time-picker/time-picker.tsx`:
- Around line 86-88: The inline arrow callback for the TimePicker's onChange
currently uses parameter names (hour, minute) that shadow the outer destructured
variables named hour and minute; rename the callback parameters (e.g., to h and
m or nextHour and nextMinute) in the onChange handler in time-picker.tsx so they
no longer shadow the outer variables and then use those new names when
constructing the string passed to the onChange prop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 304eee97-305b-4b93-be80-e901716e7f96
📒 Files selected for processing (5)
src/tedi/components/form/time-field/time-field.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsxsrc/tedi/components/form/time-picker/time-picker.spec.tsxsrc/tedi/components/form/time-picker/time-picker.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tedi/components/form/time-picker/time-picker.spec.tsx
- src/tedi/components/form/time-field/time-field.tsx
Contributor
Author
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx (1)
278-281: Use RTL role queries for wheel options.These tests already rely on ARIA roles, so prefer
within(col).getAllByRole('option')overquerySelectorAll('[role="option"]'). As per coding guidelines, "Use semantic queries in tests (getByRole,getByLabelText) instead of non-semantic queries (getByTestId)".🧪 Example refactor
-import { act, render, screen } from '@testing-library/react'; +import { act, render, screen, within } from '@testing-library/react'; ... - const items = col.querySelectorAll('[role="option"]'); + const items = within(col).getAllByRole('option');Also applies to: 303-305, 325-328, 346-348, 367-369, 699-701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx` around lines 278 - 281, Replace direct DOM querying of wheel options with RTL role queries: instead of using col.querySelectorAll('[role="option"]') create items via within(col).getAllByRole('option') (importing within from `@testing-library/react`) and keep the subsequent spies (focusSpy and scrollSpy) on items[1] as before; apply the same change to the other occurrences that build items and spies in this spec (the blocks that create col, items, focusSpy, scrollSpy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx`:
- Around line 16-17: The test mock implementation of snapToNearestItem uses
Math.floor which diverges from production; update the mock snapToNearestItem in
time-wheel.spec.tsx to use Math.round like the production helper (see
time-field-helpers.ts) so tests exercise the same boundary behavior (use
Math.max(0, Math.min(length - 1, Math.round(scrollTop / 40))) matching the
production snapping algorithm).
- Around line 10-12: The mock for time-field-helpers should preserve real timer
cleanup: replace the current no-op mock for clearScrollTimeout with a spy-able
function that forwards the timer id to the real clearTimeout (e.g., set
clearScrollTimeout = jest.fn((id) => clearTimeout(id))) so unmount/rerender
tests actually clear scheduled scroll-correction timers; keep scrollToIndex as
jest.fn() and export the same named symbols (clearScrollTimeout, scrollToIndex)
so existing assertions still work.
---
Nitpick comments:
In
`@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx`:
- Around line 278-281: Replace direct DOM querying of wheel options with RTL
role queries: instead of using col.querySelectorAll('[role="option"]') create
items via within(col).getAllByRole('option') (importing within from
`@testing-library/react`) and keep the subsequent spies (focusSpy and scrollSpy)
on items[1] as before; apply the same change to the other occurrences that build
items and spies in this spec (the blocks that create col, items, focusSpy,
scrollSpy).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3f004b48-6ee3-4fce-ae1c-f7573bda2acd
📒 Files selected for processing (6)
src/tedi/components/form/time-picker/components/time-grid/time-grid.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsxsrc/tedi/components/form/time-picker/time-picker.spec.tsxsrc/tedi/components/form/time-picker/time-picker.tsxsrc/tedi/providers/label-provider/labels-map.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tedi/components/form/time-picker/time-picker.spec.tsx
- src/tedi/components/form/time-picker/components/time-grid/time-grid.tsx
✅ Actions performedComments resolved. Approval is disabled; enable |
ly-tempel-bitweb
requested changes
Apr 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx (2)
16-17:⚠️ Potential issue | 🟡 Minor
snapToNearestItemmock still diverges from production (Math.floorvsMath.round).The production helper in
src/tedi/components/form/time-field/time-field-helpers.tsrounds (Math.round(scrollTop / ITEM_HEIGHT)), while this mock floors. Any test that pokesscrollTopto a half-item boundary (e.g.scrollTop=60withITEM_HEIGHT=40) exercises different snapping behavior than production, making the suite's scroll assertions unreliable as regression protection. This was raised in an earlier review and hasn't been picked up.🧪 Proposed mock fix
snapToNearestItem: (scrollTop: number, length: number) => - Math.max(0, Math.min(length - 1, Math.floor(scrollTop / 40))), + Math.max(0, Math.min(length - 1, Math.round(scrollTop / 40))),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx` around lines 16 - 17, The mock implementation of snapToNearestItem in the test diverges from production behavior (it uses Math.floor) — update the mock in time-wheel.spec.tsx so snapToNearestItem(scrollTop, length) matches the production helper in time-field-helpers.ts by using Math.round(scrollTop / ITEM_HEIGHT) (or Math.round(scrollTop / 40) if ITEM_HEIGHT is inlined) and clamping with Math.max/Math.min to the [0, length-1] range so tests exercise the same snapping logic as the production Snap behavior.
10-12:⚠️ Potential issue | 🟡 MinorMock
clearScrollTimeoutstill doesn't actually clear timers.A previous review suggested forwarding to the real
clearTimeoutso that pending scroll/correction timers are actually released during unmount/rerender tests (otherwise thecleans up timers on unmounttests only assert the spy was called, not that scheduled work is cancelled). The mock is still a plain no-op.🧪 Proposed mock fix
jest.mock('../../../time-field/time-field-helpers', () => ({ - clearScrollTimeout: jest.fn(), + clearScrollTimeout: jest.fn((timeout?: ReturnType<typeof setTimeout>) => { + if (timeout) clearTimeout(timeout); + }), scrollToIndex: jest.fn(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx` around lines 10 - 12, The mock for clearScrollTimeout should actually clear scheduled timers instead of being a no-op: update the jest.mock for time-field-helpers so clearScrollTimeout forwards to the real clearTimeout (e.g., implement clearScrollTimeout: (id: ReturnType<typeof setTimeout>) => global.clearTimeout(id) or use jest.requireActual to call clearTimeout), keep scrollToIndex as a jest.fn(), and ensure the mocked module exports these implementations so tests that unmount/rerender truly cancel pending timers invoked by the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tedi/components/form/time-field/time-field.tsx`:
- Around line 154-158: The interactions array currently includes the custom
click interaction whenever showPicker && timePickerTrigger === 'input' &&
!readOnly, which routes input clicks through the custom Floating UI state even
when useNativePicker is true; change the condition to also require
!useNativePicker so click is only added when using the custom picker (e.g.,
update the useInteractions call that defines interactions to use [...(showPicker
&& timePickerTrigger === 'input' && !readOnly && !useNativePicker ? [click] :
[]), dismiss, role] ), and ensure the input is not forced readOnly when
useNativePicker is true so the browser native picker can open (check where
readOnly is set around render/Input props).
- Around line 233-260: The dropdown branch currently renders whenever
availableTimes exists and ignores showPicker, readOnly, useNativePicker,
empty-list checks and always wraps the whole field in Dropdown.Trigger which
breaks timePickerTrigger semantics; before returning the Dropdown, add guards so
it only renders when showPicker is true, readOnly is false, useNativePicker is
false, and availableTimes.length > 0, and adjust the trigger wiring so the input
is not the trigger when timePickerTrigger === "button" (keep the input outside
Dropdown.Trigger and only the icon/button inside Dropdown.Trigger) while
preserving defaultActiveIndex logic (selectedIndex/currentValue), updateTime
handler, textFieldRef/textFieldProps usage, and Dropdown.Item mapping.
In `@src/tedi/components/form/time-picker/time-picker.spec.tsx`:
- Around line 106-110: The test title and assertion are inconsistent: the spec
for TimePicker uses a mocked parseTime that returns { hour: '99', minute: '10' }
and the component ends up rendering '12:00', not '00:00'; update the test so it
accurately documents behavior — either rename the it(...) description to
indicate it falls back to '12' (e.g. "falls back to '12' when hour is not in
generated hours") or change the expectation to expect('12:00') for the element
with testId 'selected'; reference the TimePicker component and the parseTime
mock in the spec when making this change.
In `@src/tedi/components/overlays/dropdown/dropdown.tsx`:
- Around line 171-175: The logic that sets focusItemOnOpen uses a strict
comparison (defaultActiveIndex !== null) which treats undefined as a provided
value and incorrectly forces pre-focus; change the check in the dropdown
component so focusItemOnOpen is determined with a nullish comparison
(defaultActiveIndex != null ? true : 'auto') and keep selectedIndex as
defaultActiveIndex ?? null; update the component (symbols: defaultActiveIndex,
selectedIndex, focusItemOnOpen) accordingly and add a regression test that opens
the dropdown via click with no defaultActiveIndex provided and asserts
aria-activedescendant is not present until keyboard navigation occurs.
---
Duplicate comments:
In
`@src/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsx`:
- Around line 16-17: The mock implementation of snapToNearestItem in the test
diverges from production behavior (it uses Math.floor) — update the mock in
time-wheel.spec.tsx so snapToNearestItem(scrollTop, length) matches the
production helper in time-field-helpers.ts by using Math.round(scrollTop /
ITEM_HEIGHT) (or Math.round(scrollTop / 40) if ITEM_HEIGHT is inlined) and
clamping with Math.max/Math.min to the [0, length-1] range so tests exercise the
same snapping logic as the production Snap behavior.
- Around line 10-12: The mock for clearScrollTimeout should actually clear
scheduled timers instead of being a no-op: update the jest.mock for
time-field-helpers so clearScrollTimeout forwards to the real clearTimeout
(e.g., implement clearScrollTimeout: (id: ReturnType<typeof setTimeout>) =>
global.clearTimeout(id) or use jest.requireActual to call clearTimeout), keep
scrollToIndex as a jest.fn(), and ensure the mocked module exports these
implementations so tests that unmount/rerender truly cancel pending timers
invoked by the code under test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dfe3db71-674a-47ff-909f-445c795669cb
📒 Files selected for processing (12)
src/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.module.scsssrc/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.spec.tsxsrc/tedi/components/form/choice-group/components/choice-group-item/choice-group-item.tsxsrc/tedi/components/form/time-field/time-field.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsxsrc/tedi/components/form/time-picker/components/time-grid/time-grid.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.spec.tsxsrc/tedi/components/form/time-picker/components/time-wheel/time-wheel.tsxsrc/tedi/components/form/time-picker/time-picker.spec.tsxsrc/tedi/components/form/time-picker/time-picker.tsxsrc/tedi/components/overlays/dropdown/dropdown.spec.tsxsrc/tedi/components/overlays/dropdown/dropdown.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tedi/components/form/time-picker/components/time-grid/time-grid.spec.tsx
- src/tedi/components/form/time-picker/time-picker.tsx
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
mart-sessman
approved these changes
May 20, 2026
| return ( | ||
| <VerticalSpacing> | ||
| <Row> | ||
| <Col lg={3} md={6}> |
Contributor
There was a problem hiding this comment.
mobiilis 1 col read sest muidu läheb visuaalselt väga katki see story
There was a problem hiding this comment.
Ma ei tea, kas Märt mõtles sama asja, aga ma muud ei näe, et oleks katki. 🤔
Contributor
Author
There was a problem hiding this comment.
panin mobiilis hetkel 2 columniliseks, kuna see time ei saa pikemaks minna kui xx:xx siis sellest esialgu piisas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style
Tests
Stories
Chores