Feat/1261 timepicker #3612
Feat/1261 timepicker #3612
Conversation
|
/publish |
PR release:
|
adamhaeger
added
kind/product-feature
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds a new TimePicker component with segmented input, dropdown selection, validation, configuration, and summaries. Integrates into layout system, introduces extensive utilities, hooks, styles, and tests. Adds localization keys (EN/NB/NN) for time labels and validation messages. Minor logging added to DateComponent. Includes expression test for displayValue. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
♻️ Duplicate comments (1)
src/app-components/TimePicker/TimePicker.tsx (1)
259-270: Fix the useless assignment to nextIndex.The initial value of
nextIndexis immediately overwritten and never used.Apply this diff to fix the issue:
- const handleSegmentNavigate = (direction: 'left' | 'right', currentIndex: number) => { - let nextIndex = currentIndex; - - if (direction === 'right') { - nextIndex = (currentIndex + 1) % segments.length; - } else { - nextIndex = (currentIndex - 1 + segments.length) % segments.length; - } + const handleSegmentNavigate = (direction: 'left' | 'right', currentIndex: number) => { + const nextIndex = direction === 'right' + ? (currentIndex + 1) % segments.length + : (currentIndex - 1 + segments.length) % segments.length;
🧹 Nitpick comments (44)
src/language/texts/en.ts (1)
43-43: Minor copy tweak for consistency with date picker messageTo match the existing “date_picker.invalid_date_message” phrasing and tone, consider adding “the”.
Apply this diff:
- 'time_picker.invalid_time_message': 'Invalid time format. Use format {0}.', + 'time_picker.invalid_time_message': 'Invalid time format. Use the format {0}.',src/language/texts/nn.ts (1)
45-47: Nynorsk wording – align “tillaten/tillat” with existing date messagesThe date-picker keys in this file use “…dato tillat”, while the new time-picker keys use “…tillaten tid”. For intra-file consistency, consider using the same adjective form as the date messages.
Proposed diff (please have a native reviewer confirm):
- 'time_picker.min_time_exceeded': 'Tida du har vald er før tidlegaste tillaten tid ({0}).', - 'time_picker.max_time_exceeded': 'Tida du har vald er etter seinaste tillaten tid ({0}).', + 'time_picker.min_time_exceeded': 'Tida du har vald er før tidlegaste tid tillat ({0}).', + 'time_picker.max_time_exceeded': 'Tida du har vald er etter seinaste tid tillat ({0}).',If the project prefers “tillaten” here for grammatical reasons, feel free to keep as-is; the main point is to be consistent within the file.
src/app-components/TimePicker/debug.test.tsx (2)
3-3: Prefer renderWithProviders and user-event; avoid brittle selectors
- Tests should use renderWithProviders to align with our testing setup and future-proof context needs.
- Replace fireEvent.keyPress with userEvent.type for more realistic typing.
- Avoid querySelector with exact aria-label strings; instead, prefer Testing Library queries by role/accessible name.
Example refactor:
-import { fireEvent, render } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { screen } from '@testing-library/react'; +import { renderWithProviders } from 'src/test/renderWithProviders'; @@ - const { container } = render( + renderWithProviders( <TimePicker id='test-timepicker' value='' onChange={onChange} aria-label='Select time' />, ); @@ - const hoursInput = container.querySelector('input[aria-label="Select time hours"]') as HTMLInputElement; + const hoursInput = screen.getByRole('textbox', { name: /hours/i }); @@ - fireEvent.keyPress(hoursInput, { key: '2', charCode: 50 }); + await userEvent.type(hoursInput, '2'); @@ - fireEvent.keyPress(hoursInput, { key: '2', charCode: 50 }); + await userEvent.type(hoursInput, '2'); @@ - expect(hoursInput.value).toBe('22'); + expect(hoursInput).toHaveValue('22');Note: If TimePicker relies on timers internally, keep the fake timers; user-event can still work with them.
Also applies to: 19-26, 28-29, 42-44, 53-55, 61-61
7-16: Consider skipping or converting this debug-spec into an actionable unit testThis file reads like an investigation aid (naming and extensive logging). If it’s not intended as a stable test, either mark it as skipped or transform it into an assertion-driven spec that validates expected behavior across formats/locales.
Option A (skip):
-describe('Debug typing behavior', () => { +describe.skip('Debug typing behavior', () => {Option B: Keep and harden (apply the refactor above and add concrete assertions on onChange calls).
I can help convert this into a deterministic typing-behavior spec exercising edge cases (e.g., 0-prefixed hours, overflow to minutes, AM/PM boundaries).
Also applies to: 17-63
src/layout/TimePicker/TimePickerComponent.test.tsx (1)
68-84: Seconds visibility test is fine, but consider asserting accessible names tooAsserting count is good; adding accessible-name checks makes it stricter without being brittle.
Example:
- const inputs = screen.getAllByRole('textbox'); - expect(inputs).toHaveLength(3); // Hours, minutes, and seconds + const inputs = screen.getAllByRole('textbox'); + expect(inputs).toHaveLength(3); // Hours, minutes, and seconds + expect(inputs[2]).toHaveAccessibleName(/seconds\b/i);src/layout/TimePicker/useTimePickerValidation.ts (3)
123-129: Add bindingKey to component validations for consistencyAttach the binding key so validations map cleanly back to the component binding in summary/UX.
Apply this diff:
validations.push({ message: { key: 'time_picker.invalid_time_message', params: [format] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', }); @@ validations.push({ message: { key: 'time_picker.min_time_exceeded', params: [minTime] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', }); @@ validations.push({ message: { key: 'time_picker.max_time_exceeded', params: [maxTime] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', });Also applies to: 136-141, 148-153
9-67: Avoid duplicating time parsing logic; consider a strict validator in shared utilsThis file reimplements a strict parser while timeConstraintUtils.parseTimeString is permissive by design. To avoid drift:
- Introduce a “strict” parser (e.g., validateTimeString or parseTimeStringStrict) in src/app-components/TimePicker/timeConstraintUtils.ts and reuse it here, or
- Centralize the regex into a shared util to keep TimePicker, Summary, and validation in sync.
I can draft a minimal strict parse helper in timeConstraintUtils and update the hook accordingly if you want.
71-105: Duplicate ISO-to-display conversion; centralize for reuseextractTimeFromValue duplicates the display logic that TimePicker.useDisplayData already has. Consider moving the conversion to a shared utility (e.g., timeFormatUtils) and consume it both here and in the component/summary to avoid divergence.
Would you like me to propose a small helper in timeFormatUtils and replace both sites?
src/app-components/TimePicker/timeConstraintUtils.test.ts (2)
8-25: Remove duplicated type declarations; import from the module to fix lint warningLocal interfaces duplicate exported types and trigger an unused var warning for TimeConstraints. Import the types instead.
Apply this diff:
import { getNextValidValue, getSegmentConstraints, isTimeInRange, parseTimeString, } from 'src/app-components/TimePicker/timeConstraintUtils'; -interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -} - -interface TimeConstraints { - minTime?: string; - maxTime?: string; -} - -interface SegmentConstraints { - min: number; - max: number; - validValues: number[]; -} +import type { TimeValue, SegmentConstraints } from 'src/app-components/TimePicker/timeConstraintUtils';
116-120: Strengthen edge-case coverage: seconds and immutability
- isTimeInRange: add seconds-sensitive assertions.
- getSegmentConstraints: add 'seconds' segment boundary test.
- getNextValidValue: ensure validValues is not mutated (reverse() in impl currently mutates).
Apply this diff to append tests:
@@ describe('isTimeInRange', () => { @@ it('should return true when no constraints provided', () => { const result = isTimeInRange(sampleTime, {}, 'HH:mm'); expect(result).toBe(true); }); + it('should respect seconds when format includes seconds (before min seconds)', () => { + const t: TimeValue = { hours: 14, minutes: 30, seconds: 1, period: 'PM' }; + const constraints = { minTime: '14:30:02', maxTime: '14:31:00' }; + expect(isTimeInRange(t, constraints, 'HH:mm:ss')).toBe(false); + }); + it('should respect seconds when at exact boundary', () => { + const t: TimeValue = { hours: 14, minutes: 30, seconds: 2, period: 'PM' }; + const constraints = { minTime: '14:30:02', maxTime: '14:31:00' }; + expect(isTimeInRange(t, constraints, 'HH:mm:ss')).toBe(true); + }); }); @@ describe('getSegmentConstraints', () => { @@ expect(result.validValues).toEqual(Array.from({ length: 60 }, (_, i) => i)); }); + it('should constrain seconds when at min boundary', () => { + const currentTime: TimeValue = { hours: 14, minutes: 30, seconds: 0, period: 'PM' }; + const constraints = { minTime: '14:30:15' }; + const result = getSegmentConstraints('seconds', currentTime, constraints, 'HH:mm:ss'); + expect(result.min).toBe(15); + expect(result.max).toBe(59); + expect(result.validValues[0]).toBe(15); + }); }); @@ describe('getNextValidValue', () => { @@ it('should skip invalid values and find next valid one', () => { const constraints: SegmentConstraints = { min: 5, max: 20, validValues: [5, 8, 12, 15, 20], }; const result = getNextValidValue(5, 'up', constraints); expect(result).toBe(8); }); + it('should not mutate constraints.validValues (order preserved after call)', () => { + const constraints: SegmentConstraints = { + min: 0, + max: 10, + validValues: [0, 3, 7, 10], + }; + const copy = [...constraints.validValues]; + getNextValidValue(5, 'down', constraints); + expect(constraints.validValues).toEqual(copy); + }); });If this last test fails, we should adjust getNextValidValue to avoid mutating arrays (use a shallow copy before reverse()).
Also applies to: 151-166, 209-218
src/app-components/TimePicker/segmentTyping.test.ts (1)
224-243: Add 12-hour advancement and minute-buffer edge assertionsExpand coverage to ensure 12h auto-advance and minute single-digit completion behave as intended.
Apply this diff to append tests:
@@ describe('shouldAdvanceSegment', () => { @@ it('should not advance from seconds segment', () => { expect(shouldAdvanceSegment('seconds', '59', false)).toBe(false); }); + it('should advance on single-digit 2..9 in 12h mode', () => { + expect(shouldAdvanceSegment('hours', '2', true)).toBe(true); + expect(shouldAdvanceSegment('hours', '1', true)).toBe(false); + }); }); + + describe('processSegmentBuffer - minutes single digit completion', () => { + it('should mark minutes single-digit >5 as complete', () => { + expect(processSegmentBuffer('7', 'minutes', false)).toEqual({ + displayValue: '07', + actualValue: 7, + isComplete: true, + }); + }); + });src/app-components/TimePicker/typingBehavior.test.tsx (1)
233-252: Avoid stale node after rerender: re-query DOMAfter rerender, reuse of the pre-rerender input reference is brittle. Re-query to ensure you hold the current element.
Apply this diff:
- // Type another "2" - should result in "22", not "02" - fireEvent.keyPress(hoursInput, { key: '2', charCode: 50 }); - expect(hoursInput.value).toBe('22'); + // Re-query after rerender to avoid stale reference + const hoursInputAfter = container.querySelector('input[aria-label="Select time hours"]') as HTMLInputElement; + fireEvent.keyPress(hoursInputAfter, { key: '2', charCode: 50 }); + expect(hoursInputAfter.value).toBe('22');src/app-components/TimePicker/keyboardNavigation.test.ts (3)
15-21: Remove unused local type to satisfy lint and avoid drift.
SegmentNavigationResultis defined but never used (see static analysis warning). Drop it to keep the test lean and quiet.- interface SegmentNavigationResult { - shouldNavigate: boolean; - direction?: 'left' | 'right'; - shouldIncrement?: boolean; - shouldDecrement?: boolean; - preventDefault: boolean; - }
1-6: Prefer importing shared types and strongly typing the mock event.
- Reuse the exported
SegmentTypefrom the implementation to prevent type drift.- Define a local
KeyEventfrom the function signature instead of casting viaunknown.import { getNextSegmentIndex, handleSegmentKeyDown, handleValueDecrement, handleValueIncrement, + type SegmentType, } from 'src/app-components/TimePicker/keyboardNavigation'; -interface MockKeyboardEvent { - key: string; - preventDefault: () => void; -} - -type SegmentType = 'hours' | 'minutes' | 'seconds' | 'period'; +type KeyEvent = Parameters<typeof handleSegmentKeyDown>[0];Follow-up: Replace casts like
as unknown as MockKeyboardEventwithas KeyEventat their call sites.Also applies to: 8-14
34-40: Optional: assert preventDefault for all arrow cases.You already assert it for ArrowUp. Mirroring that on Down/Left/Right will guard regressions in keyboard behavior.
Also applies to: 42-58
src/app-components/TimePicker/timeFormatUtils.test.ts (1)
8-13: ImportTimeValuefrom source to keep types centralized.Prevents duplicate definitions and reduces the risk of divergence if the shape changes.
-import interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -} +import type { TimeValue } from 'src/app-components/TimePicker/timeConstraintUtils';src/app-components/TimePicker/TimeSegment.test.tsx (3)
84-99: Use keys instead ofclear()to enter a single-digit hour; blur to ensure commit.Makes the test align with the component’s event model and removes flakiness from timeout-based commits.
- await userEvent.clear(input); - await userEvent.type(input, '8'); - - expect(onValueChange).toHaveBeenCalledWith(8); + await userEvent.click(input); + await userEvent.keyboard('{Backspace}'); + await userEvent.type(input, '8'); + await userEvent.tab(); + expect(onValueChange).toHaveBeenCalledWith(8);
100-114: Same rationale for two-digit entry.Use Backspace and blur to commit deterministically.
- await userEvent.clear(input); - await userEvent.type(input, '11'); - - expect(onValueChange).toHaveBeenCalledWith(11); + await userEvent.click(input); + await userEvent.keyboard('{Backspace}'); + await userEvent.type(input, '11'); + await userEvent.tab(); + expect(onValueChange).toHaveBeenCalledWith(11);
220-252: Optional: make the period toggle test self-contained withrerenderto avoid multiple textboxes.Using
screen.getAllByRole('textbox')[1]is a bit brittle as other tests evolve. Arerenderkeeps the test scoped to a single input.- jest.clearAllMocks(); - - // Simulate component with PM value for ArrowDown test - render( - <TimeSegment - {...defaultProps} - type='period' - value='PM' - onValueChange={onValueChange} - />, - ); - const pmInput = screen.getAllByRole('textbox')[1]; // Get the second input (PM one) + jest.clearAllMocks(); + const { rerender } = render( + <TimeSegment + {...defaultProps} + type='period' + value='PM' + onValueChange={onValueChange} + />, + ); + const pmInput = screen.getByRole('textbox');src/layout/TimePicker/config.ts (1)
41-43: Clarify seconds/12-hour expectations inminTime/maxTimedescriptions.Given
formatcan include seconds or AM/PM, the current “HH:mm” wording may confuse users. Either enforce HH:mm in validation or broaden the description to note accepted forms.- .setDescription('Sets the earliest allowed time in HH:mm format.') + .setDescription('Sets the earliest allowed time. Use HH:mm (or HH:mm:ss when seconds are enabled).') ... - .setDescription('Sets the latest allowed time in HH:mm format.') + .setDescription('Sets the latest allowed time. Use HH:mm (or HH:mm:ss when seconds are enabled).')Also applies to: 51-53
src/layout/TimePicker/TimePickerComponent.tsx (1)
29-51: Consider timezone intent when persisting ISO timestamps.The
timeStampbranch storesnowwith local time components as an ISO string (UTC). When round-tripping, you convert back using localDate, which is consistent, but the stored value will vary by client timezone. If the backend expects “local wall time with date” rather than an absolute instant, consider storing a local-date-time string (e.g.,YYYY-MM-DDTHH:mm[:ss]withoutZ) or also persisting timezone context.Would you like a small helper to parse/format “local date-time” strings without timezone conversion?
src/app-components/TimePicker/TimePicker.tsx (2)
69-79: Consider using a more robust mobile detection library.While the current implementation works, user agent string detection can be unreliable. Consider using a dedicated library like
react-device-detectfor more accurate device detection.Would you like me to provide an implementation using a more robust device detection library?
145-191: Consider extracting the scroll centering logic to a utility function.The scroll centering logic is repeated three times (hours, minutes, seconds). This could be extracted to reduce duplication.
Apply this diff to extract the repeated logic:
+ const scrollToSelectedOption = (containerRef: React.RefObject<HTMLDivElement | null>, selector: string) => { + if (containerRef.current) { + const selectedOption = containerRef.current.querySelector(selector); + if (selectedOption) { + const container = containerRef.current; + const elementTop = (selectedOption as HTMLElement).offsetTop; + const elementHeight = (selectedOption as HTMLElement).offsetHeight; + const containerHeight = container.offsetHeight; + + // Center the selected item in the container + container.scrollTop = elementTop - containerHeight / 2 + elementHeight / 2; + } + } + }; useEffect(() => { if (showDropdown) { // Small delay to ensure DOM is rendered setTimeout(() => { - // Scroll hours into view - if (hoursListRef.current) { - const selectedHour = hoursListRef.current.querySelector(`.${styles.dropdownOptionSelected}`); - if (selectedHour) { - const container = hoursListRef.current; - const elementTop = (selectedHour as HTMLElement).offsetTop; - const elementHeight = (selectedHour as HTMLElement).offsetHeight; - const containerHeight = container.offsetHeight; - - // Center the selected item in the container - container.scrollTop = elementTop - containerHeight / 2 + elementHeight / 2; - } - } - - // Scroll minutes into view - if (minutesListRef.current) { - const selectedMinute = minutesListRef.current.querySelector(`.${styles.dropdownOptionSelected}`); - if (selectedMinute) { - const container = minutesListRef.current; - const elementTop = (selectedMinute as HTMLElement).offsetTop; - const elementHeight = (selectedMinute as HTMLElement).offsetHeight; - const containerHeight = container.offsetHeight; - - container.scrollTop = elementTop - containerHeight / 2 + elementHeight / 2; - } - } - - // Scroll seconds into view - if (secondsListRef.current) { - const selectedSecond = secondsListRef.current.querySelector(`.${styles.dropdownOptionSelected}`); - if (selectedSecond) { - const container = secondsListRef.current; - const elementTop = (selectedSecond as HTMLElement).offsetTop; - const elementHeight = (selectedSecond as HTMLElement).offsetHeight; - const containerHeight = container.offsetHeight; - - container.scrollTop = elementTop - containerHeight / 2 + elementHeight / 2; - } - } + scrollToSelectedOption(hoursListRef, `.${styles.dropdownOptionSelected}`); + scrollToSelectedOption(minutesListRef, `.${styles.dropdownOptionSelected}`); + scrollToSelectedOption(secondsListRef, `.${styles.dropdownOptionSelected}`); }, 0); } }, [showDropdown]);src/app-components/TimePicker/timeFormatUtils.ts (1)
51-75: Consider removing the unused _format parameter.The
_formatparameter inparseSegmentInputis prefixed with underscore but never used. If it's intended for future use, consider adding a comment. Otherwise, remove it.Apply this diff if the parameter is not needed:
export const parseSegmentInput = ( input: string, segmentType: SegmentType, - _format: TimeFormat, ): number | string | null => {If it's intended for future use, add a comment:
export const parseSegmentInput = ( input: string, segmentType: SegmentType, + // eslint-disable-next-line @typescript-eslint/no-unused-vars _format: TimeFormat, ): number | string | null => {src/app-components/TimePicker/dropdownKeyboardNavigation.test.tsx (1)
21-32: Consider adding error handling for async operations.The
openDropdownhelper should handle potential errors when the dropdown fails to open.Apply this diff to add error handling:
const openDropdown = async () => { const triggerButton = screen.getByRole('button', { name: /open time picker/i }); fireEvent.click(triggerButton); - await waitFor(() => { - const dropdown = screen.getByRole('dialog'); - expect(dropdown).toBeInTheDocument(); - expect(dropdown).toHaveAttribute('aria-hidden', 'false'); - }); + await waitFor(() => { + const dropdown = screen.getByRole('dialog'); + expect(dropdown).toBeInTheDocument(); + expect(dropdown).toHaveAttribute('aria-hidden', 'false'); + }, { timeout: 3000 }).catch(() => { + throw new Error('Failed to open dropdown within timeout'); + }); return screen.getByRole('dialog'); };src/app-components/TimePicker/dropdownBehavior.ts (3)
62-76: Page jump calculation can divide by zero; and “60 minutes worth of options” is minutes-specific
- If
stepMinutesis 0/invalid,Math.floor(60 / stepMinutes)breaks.- This utility is only correct for a minutes list; ensure it isn’t used for hours/period lists.
Apply this diff to harden the function:
- const itemsToJump = Math.max(1, Math.floor(60 / stepMinutes)); + const safeStep = Number.isFinite(stepMinutes) && stepMinutes > 0 ? stepMinutes : 1; + const itemsToJump = Math.max(1, Math.floor(60 / safeStep));And add a JSDoc note to limit usage to the minutes column or rename to
getMinutePageJumpIndex.Confirm this function is only invoked for the minutes column.
125-132: Scroll position not clamped to end; can overshoot containerCentering is fine, but we should cap to the max scrollable position.
Apply this diff (requires
totalOptionsto compute max; if not available, at least clamp to 0):-export const calculateScrollPosition = (index: number, containerHeight: number, itemHeight: number): number => { +export const calculateScrollPosition = ( + index: number, + containerHeight: number, + itemHeight: number, + totalOptions?: number, +): number => { // Calculate position to center the item const itemTop = index * itemHeight; const scrollTo = itemTop - containerHeight / 2 + itemHeight / 2; - // Don't scroll negative - return Math.max(0, scrollTo); + // Clamp within scrollable range + const min = 0; + const max = + totalOptions && totalOptions > 0 ? Math.max(0, totalOptions * itemHeight - containerHeight) : Number.POSITIVE_INFINITY; + return Math.min(Math.max(min, scrollTo), max); };
91-105: Case-insensitive match for period stringsIf callers pass 'am'/'pm',
findNearestOptionIndexwill miss and default to index 0. Support case-insensitive string matching.Apply this diff:
- const exactIndex = options.findIndex((opt) => opt.value === value); + const exactIndex = options.findIndex((opt) => + typeof opt.value === 'string' && typeof value === 'string' + ? opt.value.toLowerCase() === value.toLowerCase() + : opt.value === value, + );src/app-components/TimePicker/TimePicker.module.css (4)
7-7: Use design token instead of hardcoded white backgroundHardcoded
whitecan break theming (e.g., dark mode). Prefer a design token background.Apply this diff:
- background: white; + background: var(--ds-color-neutral-background-default);
118-126: Avoid !important for selected option styling
!importantreduces maintainability and can interfere with focus/hover states. Increase specificity or use stateful attributes (e.g.,[aria-selected="true"]) instead.Apply this diff:
-.dropdownOptionSelected { - background-color: var(--ds-color-accent-base-active) !important; - color: white; - font-weight: 500; -} - -.dropdownOptionSelected:hover { - background-color: var(--ds-color-accent-base-active) !important; -} +.dropdownOptionSelected, +.dropdownOption[aria-selected='true'], +.dropdownOptionSelected:hover, +.dropdownOption[aria-selected='true']:hover { + background-color: var(--ds-color-accent-base-active); + color: white; + font-weight: 500; +}
128-132: Ensure keyboard focus visible even if JS class is missingRelying only on a class for focus can miss native keyboard focus. Add a
:focus-visiblefallback.Apply this diff:
.dropdownOptionFocused { outline: 2px solid var(--ds-color-accent-border-strong); outline-offset: -2px; background-color: var(--ds-color-accent-surface-hover); } + +.dropdownOption:focus-visible { + outline: 2px solid var(--ds-color-accent-border-strong); + outline-offset: -2px; +}
56-61: Revisit dropdown min/max widthMin-width is commented out; depending on content length, columns can wrap unpredictably. Consider an explicit min width or responsive rule.
Example:
/* prevents column wrapping for 3–4 columns */ .timePickerDropdown { min-width: 24rem; /* adjust to DS spacing */ }src/layout/TimePicker/index.tsx (2)
47-76: Display formatting duplicates logic from TimePickerComponent; extract a shared utilityFormatting 12/24h with optional seconds is implemented here and in
TimePickerComponent(see relevant snippet). Extract to a shared formatter to keep behavior consistent.I can factor this into
timeFormatUtils.formatTimeDisplay({ date, format })and replace both call sites.
110-125: Simplify errors extraction and remove unused local
_componentis never used, and destructuringconst [errors] = [validation[0] ?? []];is unnecessarily indirect.Apply this diff:
- const _component = useLayoutLookups().getComponent(baseComponentId, 'TimePicker'); + // component lookup not needed here; keeping lookups for binding validation only @@ - const [errors] = [validation[0] ?? []]; - - return errors; + return validation[0] ?? [];src/app-components/TimePicker/TimeSegment.tsx (3)
172-218: onKeyPress is deprecated; prefer onBeforeInput or unify onKeyDownRelying on
onKeyPresswill become brittle across browsers/React versions. Migrate toonBeforeInputfor character input or handle characters inonKeyDown.High-level approach:
- Replace
onKeyPresswithonBeforeInput={(e) => { const char = e.data; ... }}.- Keep non-character logic in
onKeyDown.
20-36: Propsmin/maxare unusedThey are declared but not used in the component. Either remove them or enforce them during commit/increment/decrement.
Apply this diff if not needed:
- min: number; - max: number;
295-297: Redundant conditional for maxLengthBoth branches are
2.Apply this diff:
- maxLength={type === 'period' ? 2 : 2} + maxLength={2}src/app-components/TimePicker/segmentTyping.ts (7)
208-214: Robustness: guardparseIntresult before comparison inshouldAdvanceSegmentIf
bufferis ever non-numeric in this path,digitbecomesNaNand comparisons return falsey subtly. Add an explicit guard for clarity and safety.Apply this diff:
if (buffer.length === 1) { - const digit = parseInt(buffer, 10); + const digit = parseInt(buffer, 10); + if (Number.isNaN(digit)) { + return false; + } if (is12Hour) { return digit >= 2; // 2-9 get coerced and advance } else { return digit >= 3; // 3-9 get coerced and advance }
234-234: TimeFormat detection should be case-insensitiveUsing
format.includes('a')missesAif upstream tokenization ever uses uppercase. Prefer a case-insensitive test.Apply this diff:
- const is12Hour = format.includes('a'); + const is12Hour = /a/i.test(format);
45-54: Confirm 12-hour semantics for00→ current behavior sets01, not12In 12h mode with first digit 0 and second 0, you coerce to
'01'. Many time pickers treat00as12(12 AM/PM). If your UX spec expects12, adjust the coercion.If aligning to
12is desired, apply:- // 01-09 valid, but 00 becomes 01 - finalValue = digitNum === 0 ? '01' : `0${digit}`; + // 01-09 valid; treat 00 as 12 in 12-hour clocks + finalValue = digitNum === 0 ? '12' : `0${digit}`;
72-95: Minute/second typing never requests auto-advance — confirm UX
processMinuteInputalways returnsshouldAdvance: false. That’s consistent with your comment (“Chrome behavior”) andshouldAdvanceSegment()returningfalsefor minutes/seconds. If design ever changes to auto-advance after two digits, you can flip the return to true whencurrentBuffer.length === 1.
120-146: Minor: underscore_is12Houris unusedThe underscore suggests intentional, but if not planned for future use, consider removing the parameter to avoid confusion.
1-279: Consolidate segment constraints to avoid duplicationRanges are currently codified in multiple places (
processHourInput,processMinuteInput,coerceToValidRange, andshouldAdvanceSegment). Centralizing constraints (e.g., via a small config map { hours12: [1,12], hours24: [0,23], mins: [0,59] }) reduces drift and eases future changes.If you want, I can propose a small constants module and refactor these helpers to reference it.
159-167: Ensure empty hours respect 12h vs 24h formatTo make
commitSegmentValuedefault hours correctly in 12-hour mode, add an optionalis12Hourflag (defaulting tofalse) and thread it through each call site where you know the display format:• In
src/app-components/TimePicker/segmentTyping.ts
– Change the signature at line 159:-export const commitSegmentValue = ( - value: number | string | null, - segmentType: SegmentType, -): number | string => { +export const commitSegmentValue = ( + value: number | string | null, + segmentType: SegmentType, + is12Hour: boolean = false, +): number | string => {– Update the “empty hours” return to:
if (value === null) { if (segmentType === 'minutes' || segmentType === 'seconds') { return 0; // Fill empty minutes/seconds with 00 } - return 0; // Default for hours too + return is12Hour ? 12 : 0; // Default hours: 12 for 12h, 0 for 24h }• In
src/app-components/TimePicker/TimeSegment.tsx
Locate each invocation ofcommitSegmentValue(buffer.actualValue, type)and change to pass the 12h flag based on yourformatprop (e.g.format.includes('a')):// Inside commitBuffer (≈line 98) - const committedValue = commitSegmentValue(buffer.actualValue, type); + const committedValue = commitSegmentValue(buffer.actualValue, type, format.includes('a')); // In immediate-commit path (≈line 198) - const committedValue = commitSegmentValue(buffer.actualValue, type); + const committedValue = commitSegmentValue(buffer.actualValue, type, format.includes('a'));• Tests in
segmentTyping.test.tsremain unchanged (the new default is backwards-compatible for minutes/seconds and hours in 24h mode), but consider adding a case to assert that in 12h mode,commitSegmentValue(null, 'hours', true)returns12.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
src/app-components/TimePicker/TimePicker.module.css(1 hunks)src/app-components/TimePicker/TimePicker.tsx(1 hunks)src/app-components/TimePicker/TimeSegment.test.tsx(1 hunks)src/app-components/TimePicker/TimeSegment.tsx(1 hunks)src/app-components/TimePicker/debug.test.tsx(1 hunks)src/app-components/TimePicker/dropdownBehavior.test.ts(1 hunks)src/app-components/TimePicker/dropdownBehavior.ts(1 hunks)src/app-components/TimePicker/dropdownKeyboardNavigation.test.tsx(1 hunks)src/app-components/TimePicker/keyboardNavigation.test.ts(1 hunks)src/app-components/TimePicker/keyboardNavigation.ts(1 hunks)src/app-components/TimePicker/segmentTyping.test.ts(1 hunks)src/app-components/TimePicker/segmentTyping.ts(1 hunks)src/app-components/TimePicker/timeConstraintUtils.test.ts(1 hunks)src/app-components/TimePicker/timeConstraintUtils.ts(1 hunks)src/app-components/TimePicker/timeFormatUtils.test.ts(1 hunks)src/app-components/TimePicker/timeFormatUtils.ts(1 hunks)src/app-components/TimePicker/typingBehavior.test.tsx(1 hunks)src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/Date/DateComponent.tsx(2 hunks)src/layout/TimePicker/TimePickerComponent.test.tsx(1 hunks)src/layout/TimePicker/TimePickerComponent.tsx(1 hunks)src/layout/TimePicker/TimePickerSummary.tsx(1 hunks)src/layout/TimePicker/config.ts(1 hunks)src/layout/TimePicker/index.tsx(1 hunks)src/layout/TimePicker/useTimePickerValidation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/layout/TimePicker/TimePickerSummary.tsxsrc/layout/TimePicker/TimePickerComponent.test.tsxsrc/app-components/TimePicker/timeConstraintUtils.test.tssrc/app-components/TimePicker/debug.test.tsxsrc/language/texts/nb.tssrc/language/texts/en.tssrc/app-components/TimePicker/TimeSegment.test.tsxsrc/app-components/TimePicker/dropdownBehavior.tssrc/app-components/TimePicker/segmentTyping.test.tssrc/layout/Date/DateComponent.tsxsrc/app-components/TimePicker/dropdownKeyboardNavigation.test.tsxsrc/language/texts/nn.tssrc/app-components/TimePicker/timeConstraintUtils.tssrc/app-components/TimePicker/TimeSegment.tsxsrc/app-components/TimePicker/typingBehavior.test.tsxsrc/app-components/TimePicker/timeFormatUtils.tssrc/app-components/TimePicker/dropdownBehavior.test.tssrc/app-components/TimePicker/timeFormatUtils.test.tssrc/app-components/TimePicker/TimePicker.tsxsrc/app-components/TimePicker/segmentTyping.tssrc/layout/TimePicker/useTimePickerValidation.tssrc/app-components/TimePicker/keyboardNavigation.test.tssrc/app-components/TimePicker/keyboardNavigation.tssrc/layout/TimePicker/TimePickerComponent.tsxsrc/layout/TimePicker/config.tssrc/layout/TimePicker/index.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/layout/TimePicker/TimePickerComponent.test.tsxsrc/app-components/TimePicker/timeConstraintUtils.test.tssrc/app-components/TimePicker/debug.test.tsxsrc/app-components/TimePicker/TimeSegment.test.tsxsrc/app-components/TimePicker/segmentTyping.test.tssrc/app-components/TimePicker/dropdownKeyboardNavigation.test.tsxsrc/app-components/TimePicker/typingBehavior.test.tsxsrc/app-components/TimePicker/dropdownBehavior.test.tssrc/app-components/TimePicker/timeFormatUtils.test.tssrc/app-components/TimePicker/keyboardNavigation.test.ts
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/app-components/TimePicker/TimePicker.module.css
src/layout/*/{config.ts,Component.tsx,index.tsx,config.generated.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Layout components must follow the standardized structure:
config.ts,Component.tsx,index.tsx, and include generated types inconfig.generated.ts
Files:
src/layout/TimePicker/config.tssrc/layout/TimePicker/index.tsx
🧠 Learnings (2)
📚 Learning: 2025-08-22T13:53:28.201Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.201Z
Learning: Applies to **/*.test.{ts,tsx} : In tests, use `renderWithProviders` from `src/test/renderWithProviders.tsx` to supply required form layout context
Applied to files:
src/layout/TimePicker/TimePickerComponent.test.tsx
📚 Learning: 2025-08-22T13:53:28.201Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.201Z
Learning: Applies to src/layout/*/{config.ts,Component.tsx,index.tsx,config.generated.ts} : Layout components must follow the standardized structure: `config.ts`, `Component.tsx`, `index.tsx`, and include generated types in `config.generated.ts`
Applied to files:
src/layout/TimePicker/config.ts
🧬 Code graph analysis (19)
src/layout/TimePicker/TimePickerSummary.tsx (9)
src/layout/Summary2/SummaryComponent2/types.ts (1)
Summary2Props(1-3)src/layout/Summary2/summaryStoreContext.tsx (2)
useSummaryOverrides(43-69)useSummaryProp(29-37)src/layout/TimePicker/index.tsx (1)
useDisplayData(37-80)src/features/validation/selectors/unifiedValidationsForNode.ts (1)
useUnifiedValidationsForNode(15-27)src/features/validation/utils.ts (1)
validationsOfSeverity(39-44)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/layout/Summary2/SummaryComponent2/ComponentSummary.tsx (1)
SummaryFlex(123-151)src/layout/Summary2/CommonSummaryComponents/SingleValueSummary.tsx (1)
SingleValueSummary(22-82)src/features/language/Lang.tsx (1)
Lang(15-23)
src/layout/TimePicker/TimePickerComponent.test.tsx (3)
src/test/renderWithProviders.tsx (1)
renderGenericComponentTest(683-733)src/layout/TimePicker/TimePickerComponent.tsx (1)
TimePickerComponent(12-117)src/__mocks__/getLayoutSetsMock.ts (1)
defaultDataTypeMock(3-3)
src/app-components/TimePicker/timeConstraintUtils.test.ts (1)
src/app-components/TimePicker/timeConstraintUtils.ts (7)
TimeValue(3-8)TimeConstraints(10-13)SegmentConstraints(15-19)parseTimeString(21-56)isTimeInRange(58-80)getSegmentConstraints(82-189)getNextValidValue(191-217)
src/app-components/TimePicker/debug.test.tsx (1)
src/layout/TimePicker/index.tsx (1)
TimePicker(30-134)
src/app-components/TimePicker/TimeSegment.test.tsx (1)
src/app-components/TimePicker/TimeSegment.tsx (2)
TimeSegmentProps(20-36)TimeSegment(38-300)
src/app-components/TimePicker/dropdownBehavior.ts (1)
src/app-components/TimePicker/keyboardNavigation.ts (1)
SegmentType(4-4)
src/app-components/TimePicker/segmentTyping.test.ts (1)
src/app-components/TimePicker/segmentTyping.ts (9)
processHourInput(18-67)processMinuteInput(72-95)processPeriodInput(100-109)processSegmentBuffer(120-146)isNavigationKey(114-115)clearSegment(151-154)commitSegmentValue(159-167)coerceToValidRange(172-198)shouldAdvanceSegment(203-219)
src/app-components/TimePicker/TimeSegment.tsx (3)
src/app-components/TimePicker/keyboardNavigation.ts (4)
SegmentType(4-4)handleSegmentKeyDown(14-58)handleValueIncrement(74-110)handleValueDecrement(112-148)src/app-components/TimePicker/timeFormatUtils.ts (1)
formatSegmentValue(28-49)src/app-components/TimePicker/segmentTyping.ts (4)
processSegmentBuffer(120-146)commitSegmentValue(159-167)clearSegment(151-154)handleSegmentCharacterInput(224-278)
src/app-components/TimePicker/typingBehavior.test.tsx (1)
src/layout/TimePicker/index.tsx (1)
TimePicker(30-134)
src/app-components/TimePicker/timeFormatUtils.ts (2)
src/app-components/TimePicker/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/keyboardNavigation.ts (1)
SegmentType(4-4)
src/app-components/TimePicker/dropdownBehavior.test.ts (1)
src/app-components/TimePicker/dropdownBehavior.ts (9)
roundToStep(11-11)getInitialHighlightIndex(16-46)getNextIndex(51-57)getPageJumpIndex(62-76)getHomeIndex(81-81)getEndIndex(86-86)findNearestOptionIndex(91-120)calculateScrollPosition(125-132)shouldScrollToOption(137-152)
src/app-components/TimePicker/timeFormatUtils.test.ts (2)
src/app-components/TimePicker/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/timeFormatUtils.ts (4)
formatTimeValue(5-26)formatSegmentValue(28-49)parseSegmentInput(51-75)isValidSegmentInput(77-117)
src/app-components/TimePicker/segmentTyping.ts (1)
src/app-components/TimePicker/keyboardNavigation.ts (1)
SegmentType(4-4)
src/layout/TimePicker/useTimePickerValidation.ts (5)
src/app-components/TimePicker/timeConstraintUtils.ts (1)
parseTimeString(21-56)src/features/validation/index.ts (1)
ComponentValidation(151-153)src/utils/layout/hooks.ts (1)
useDataModelBindingsFor(102-112)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/features/formData/FormDataWrite.tsx (1)
FD(683-1096)
src/app-components/TimePicker/keyboardNavigation.test.ts (1)
src/app-components/TimePicker/keyboardNavigation.ts (6)
SegmentType(4-4)SegmentNavigationResult(6-12)handleSegmentKeyDown(14-58)getNextSegmentIndex(60-72)handleValueIncrement(74-110)handleValueDecrement(112-148)
src/app-components/TimePicker/keyboardNavigation.ts (1)
src/app-components/TimePicker/timeConstraintUtils.ts (1)
SegmentConstraints(15-19)
src/layout/TimePicker/TimePickerComponent.tsx (6)
src/layout/index.ts (1)
PropsFromGenericComponent(28-32)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/features/formData/useDataModelBindings.ts (1)
useDataModelBindings(42-57)src/utils/layout/useLabel.tsx (1)
useLabel(13-72)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/app-components/Flex/Flex.tsx (1)
Flex(25-84)
src/layout/TimePicker/config.ts (1)
src/codegen/CG.ts (1)
CG(25-57)
src/layout/TimePicker/index.tsx (12)
src/layout/index.ts (4)
ValidateComponent(68-70)ValidationFilter(86-88)PropsFromGenericComponent(28-32)ValidationFilterFunction(80-84)src/layout/TimePicker/TimePickerComponent.tsx (1)
TimePickerComponent(12-117)src/utils/layout/useNodeItem.ts (1)
useNodeFormDataWhenType(97-103)src/utils/layout/hooks.ts (1)
useExternalItem(16-22)src/layout/LayoutComponent.tsx (2)
SummaryRendererProps(167-172)ExprResolver(41-53)src/layout/Summary/SummaryItemSimple.tsx (1)
SummaryItemSimple(14-35)src/layout/Summary2/SummaryComponent2/types.ts (1)
Summary2Props(1-3)src/layout/TimePicker/useTimePickerValidation.ts (1)
useTimePickerValidation(107-157)src/layout/layout.ts (1)
IDataModelBindings(61-64)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels(382-423)src/features/form/layout/LayoutsContext.tsx (1)
useLayoutLookups(113-113)src/utils/layout/generator/validation/hooks.ts (1)
validateDataModelBindingsAny(10-56)
🪛 GitHub Actions: Tests
src/layout/TimePicker/TimePickerComponent.test.tsx
[error] 47-47: Element does not have aria-label='Hours'. Actual aria-label='schmable hours'. Tests expecting specific aria-labels are failing.
[error] 65-65: Unable to find a button with name matching '/AM|PM/i'. The component's accessibility labels do not match test expectations.
src/app-components/TimePicker/TimeSegment.test.tsx
[error] 130-130: Expected element to have value, but got undefined. Possibly the input did not receive the expected value.
[error] 149-149: Expected onChange to have been called with 'PM'. It was not called.
[error] 101-101: Cannot find a single element with role 'button' and name '13'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 149-149: Expected onValueChange to have been called with 'PM'. It was not called.
[error] 130-130: Expected input to have value, but got undefined. Possibly the input did not receive the expected value.
[error] 149-149: Expected onValueChange to have been called with 'PM'. It was not called.
[error] 101-101: Cannot find a single element with role 'button' and name '13'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 149-149: Expected onValueChange to have been called with 'PM'. It was not called.
src/app-components/TimePicker/dropdownKeyboardNavigation.test.tsx
[error] 42-42: Cannot find a single element with the role 'button' and name '14'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 75-75: Cannot find a single element with the role 'button' and name '14'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 101-101: Cannot find a single element with the role 'button' and name '15'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 126-126: Cannot find a single element with the role 'button' and name '14'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 171-171: Cannot find a single element with the role 'button' and name '16'. Multiple matches found. Use getAllByRole instead of getByRole.
[error] 42-42: Cannot select element with getByRole('button', { name: '14' }) because multiple matches found. Use getAllByRole and select the correct element.
[error] 75-75: Cannot select element with getByRole('button', { name: '14' }) because multiple matches found. Use getAllByRole and select the correct element.
[error] 101-101: Cannot select element with getByRole('button', { name: '15' }) because multiple matches found. Use getAllByRole and select the correct element.
[error] 126-126: Cannot select element with getByRole('button', { name: '14' }) because multiple matches found. Use getAllByRole and select the correct element.
🪛 GitHub Check: Type-checks, eslint, unit tests and SonarCloud
src/app-components/TimePicker/timeConstraintUtils.test.ts
[warning] 15-15:
'TimeConstraints' is defined but never used. Allowed unused vars must match /^_/u
src/app-components/TimePicker/debug.test.tsx
[warning] 55-55:
Unexpected console statement. Only these console methods are allowed: warn, error
[warning] 44-44:
Unexpected console statement. Only these console methods are allowed: warn, error
[warning] 37-37:
Unexpected console statement. Only these console methods are allowed: warn, error
[warning] 30-30:
Unexpected console statement. Only these console methods are allowed: warn, error
src/layout/Date/DateComponent.tsx
[warning] 26-26:
Unexpected console statement. Only these console methods are allowed: warn, error
[warning] 43-43:
Unexpected console statement. Only these console methods are allowed: warn, error
src/app-components/TimePicker/keyboardNavigation.test.ts
[warning] 15-15:
'SegmentNavigationResult' is defined but never used. Allowed unused vars must match /^_/u
🪛 GitHub Check: CodeQL
src/app-components/TimePicker/TimePicker.tsx
[warning] 260-260: Useless assignment to local variable
The initial value of nextIndex is unused, since it is always overwritten.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install
| const selectedHour = screen.getByRole('button', { name: '14' }); | ||
| expect(selectedHour).toHaveClass('dropdownOptionSelected'); | ||
|
|
There was a problem hiding this comment.
Fix duplicate button matches in tests.
Multiple tests are failing because there are duplicate buttons with the same label (e.g., "14") in the dropdown. This occurs because the hour "14" appears as both a button and possibly in other columns.
Apply this diff to fix the duplicate matches by using more specific queries:
- const selectedHour = screen.getByRole('button', { name: '14' });
+ const hoursColumn = screen.getByText('Timer').parentElement;
+ const selectedHour = within(hoursColumn).getByRole('button', { name: '14' });You'll need to import within from '@testing-library/react':
-import { fireEvent, render, screen, waitFor } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor, within } from '@testing-library/react';Apply similar fixes to lines 75, 101, 126, and 228-236 where the same issue occurs.
Also applies to: 75-77
| import { defaultDataTypeMock } from 'src/__mocks__/getLayoutSetsMock'; | ||
| import { TimePickerComponent } from 'src/layout/TimePicker/TimePickerComponent'; | ||
| import { renderGenericComponentTest } from 'src/test/renderWithProviders'; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Upstream component has a placeholder aria-label that should be fixed at the source
Related to the failing test above: the wrapper currently sets aria-label='schmable' on TimePickerControl (see src/layout/TimePicker/TimePickerComponent.tsx). Prefer either:
- Link label via aria-labelledby to the ’s id, and let segment inputs expose stable, localized names (“Hours/Minutes/Seconds”), or
- Use a descriptive aria-label derived from textResourceBindings (e.g., “Select time”) without altering segment names.
Proposed change in TimePickerComponent.tsx:
- aria-label='schmable'
+ // Rely on external <Label> and segment-level accessible names
+ aria-label={undefined as unknown as never}Or set aria-labelledby to the ’s id if supported by TimePickerControl. This will reduce brittleness across locales and match a11y best practices.
🏁 Script executed:
#!/bin/bash
# Confirm how TimePickerControl computes segment accessible names
rg -n -C3 -e "TimePickerControl" -e "aria-label" src/app-components/TimePicker --type tsxLength of output: 115
I can’t locate any TSX matches because ripgrep isn’t recognizing .tsx by default. Let’s broaden the search to all TSX files:
🏁 Script executed:
#!/bin/bash
# Locate TimePickerControl usages in TSX files
rg -n -C3 "TimePickerControl" -g '*.tsx' src
# Locate any aria-label props in TSX files
rg -n -C3 "aria-label" -g '*.tsx' srcLength of output: 74440
Fix TimePickerControl accessibility: support aria-labelledby and remove placeholder aria-label
The upstream TimePicker control currently only accepts a required aria-label and prefixes it onto each segment ("${ariaLabel} ${segmentType}"), and explicitly forbids aria-labelledby (see src/app-components/TimePicker/TimePicker.tsx lines 26–29). This makes it impossible to expose stable segment names (“Hours”, “Minutes”) and breaks localization/tests.
To resolve, we must refactor both the control and its wrapper:
• In src/app-components/TimePicker/TimePicker.tsx
– Change the props definition to a union that allows either
{ 'aria-label': string; 'aria-labelledby'?: never }
or
{ 'aria-label'?: never; 'aria-labelledby': string }.
– In the rendered JSX, apply the root-level aria-labelledby when provided, and render each segment input with aria-label={segmentType} only (remove the ${ariaLabel} prefix).
• In src/layout/TimePicker/TimePickerComponent.tsx
– Remove the hard-coded aria-label='schmable'.
– Pass the generated label ID from the <Label> component via aria-labelledby={labelId}.
• Update TimePickerComponent.test.tsx to assert that each segment’s aria-label is exactly "Hours" and "Minutes", and that the control itself is labeled by the external <Label>.
These changes align with a11y best practices, eliminate brittle placeholder text, and restore the intended test behavior.
🤖 Prompt for AI Agents
In src/layout/TimePicker/TimePickerComponent.test.tsx around lines 5 to 8, the
test must be updated to match the refactor: stop expecting a prefixed
placeholder aria-label and instead assert that each segment input has an exact
aria-label of "Hours" and "Minutes", and assert the root control is labeled via
aria-labelledby referencing the generated Label id; update the test setup to
obtain the Label's id (labelId) and check the TimePicker element has
aria-labelledby={labelId} while each segment's aria-label is exactly the segment
name.
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (1)
src/layout/TimePicker/TimePickerComponent.tsx (1)
110-122: Resolved: placeholder aria-label removedThe earlier placeholder aria-label has been removed; the control is correctly named via the associated Label.
🧹 Nitpick comments (52)
src/app-components/TimePicker/components/TimePicker.module.css (8)
1-9: Prefer design tokens over hard-coded values for consistency and themingReplace literal values with design-system tokens to align with existing
*.module.csspatterns and ensure dark-mode/theming support..calendarInputWrapper { display: flex; align-items: center; - border-radius: 4px; + border-radius: var(--ds-border-radius-md); border: var(--ds-border-width-default, 1px) solid var(--ds-color-neutral-border-strong); gap: var(--ds-size-1); - background: white; - padding: 2px; + background: var(--ds-color-neutral-background-default); + padding: var(--ds-size-1); }
11-17: Hover ring looks right, but consider token dedicated to focus/interactive rings
box-shadowring on hover uses accent-border-strong; if the DS has a specific interactive/hover ring token, prefer that for consistency across inputs. Otherwise, keep as-is.
19-39: Focus outline offset is negative; consider positive offset to avoid clipping and improve accessibilityNegative
outline-offsetcan clip the focus ring in some environments..segmentContainer input:focus-visible { - outline: 2px solid var(--ds-color-accent-border-strong); - outline-offset: -1px; + outline: 2px solid var(--ds-color-accent-border-strong); + outline-offset: 2px; border-radius: 2px; }
56-61: Remove commented min-width or document why it’s needed
/*min-width: 320px;*/suggests a previous constraint. Either delete or add a code comment explaining responsive rationale.
101-126: Avoid !important; increase specificity insteadSignals potential specificity issues. Prefer a more specific selector to ensure selected state wins over hover, and duplicate the rule for
:hoverif necessary.-.dropdownOptionSelected { - background-color: var(--ds-color-accent-base-active) !important; - color: white; - font-weight: 500; -} - -.dropdownOptionSelected:hover { - background-color: var(--ds-color-accent-base-active) !important; -} +.dropdownOptionSelected, +.dropdownOptionSelected:hover { + background-color: var(--ds-color-accent-base-active); + color: var(--ds-color-neutral-text-on-inverted); + font-weight: 500; +}
118-138: Outline color uses a text token; prefer a border/focus token if available
var(--ds-color-neutral-text-on-inverted)for the outline is semantically a text color. If the DS has a*-border-strongor*-focustoken for outlines on inverted surfaces, prefer that.
90-99: Firefox scrollbar support and reduced-motionYou’ve styled WebKit scrollbars; add
scrollbar-colorfor Firefox and respect reduced motion on hover transitions..dropdownList { max-height: 160px; overflow-y: auto; overflow-x: hidden; border: 1px solid var(--ds-color-neutral-border-subtle); border-radius: var(--ds-border-radius-md); padding: 2px 0; box-sizing: border-box; width: 100%; + /* Firefox scrollbar colors: thumb track */ + scrollbar-color: var(--ds-color-neutral-border-default) var(--ds-color-neutral-background-subtle); } .dropdownOption { width: 100%; padding: 6px 10px; border: none; background: transparent; font-size: 0.875rem; font-family: inherit; text-align: center; cursor: pointer; color: var(--ds-color-neutral-text-default); - transition: background-color 0.15s ease; + transition: background-color 0.15s ease; } + +@media (prefers-reduced-motion: reduce) { + .dropdownOption { + transition: none; + } +}Also applies to: 150-167
1-18: Class name reads “calendar” in a TimePicker stylesheet
calendarInputWrappermay confuse future readers. If safe, rename to something time-specific (e.g.,timeInputWrapper) and update usage.src/language/texts/nb.ts (1)
45-51: Minor Bokmål grammar tweak for clarity“tidligst tillatte tid” → “tidligste tillatte tid” reads more natural; similarly ensure parallel phrasing for max. If product prefers “tidspunkt”, consider that for extra clarity.
- 'time_picker.invalid_time_message': 'Ugyldig tidsformat. Bruk formatet {0}.', - 'time_picker.min_time_exceeded': 'Tiden du har valgt er før tidligst tillatte tid ({0}).', - 'time_picker.max_time_exceeded': 'Tiden du har valgt er etter seneste tillatte tid ({0}).', + 'time_picker.invalid_time_message': 'Ugyldig tidsformat. Bruk formatet {0}.', + 'time_picker.min_time_exceeded': 'Tiden du har valgt er før tidligste tillatte tid ({0}).', + 'time_picker.max_time_exceeded': 'Tiden du har valgt er etter seneste tillatte tid ({0}).',Note: The keys mix
time_picker.*(validation) andtimepicker.*(labels). If intentional to mirror existingdate_picker.*, fine. If not, we should unify naming in all locales in a follow-up to avoid future confusion.src/app-components/TimePicker/README.md (2)
11-17: Tiny grammar/wording polish (optional)
- “Auto-coercion” → “Auto‑padding/coercion” for clarity.
- “moves to next segment” → “moves to the next segment”
- “Type ':', '.', ',' or space” → “Type ':', '.', ',', or space”
Also applies to: 20-24
136-142: Accessibility: consider explicitly documenting ARIA relationshipsIf the dropdown is aria-controlled by the trigger, add a note recommending
aria-controlsandaria-expandedon the trigger, androle="listbox"/option"with proper keyboard support in the dropdown. This keeps implementers aligned with WAI-ARIA practices.src/app-components/TimePicker/tests/TimeSegment.test.tsx (2)
83-99: Timer-backed behavior: make intent explicit or use fake timersSome segment commits depend on timeouts. While these tests avoid the 1s buffer by advancing/committing via navigation/blur, it’s worth documenting that to prevent accidental flakiness if tests change. Alternatively, wrap with fake timers where applicable.
Also applies to: 100-115, 134-153
241-255: Avoid brittle getAllByRole indexingSelecting the second textbox by index can break if test order or render tree changes. Prefer scoping via
within(container)or addingaria-labelspecific to the period input.-const pmInput = screen.getAllByRole('textbox')[1]; +const pmInput = screen.getByRole('textbox', { name: /period/i });You may need to pass a specific
aria-labelwhen rendering that instance.src/app-components/TimePicker/tests/dropdownBehavior.test.ts (1)
111-139: Page jump logic: clarify minute-step assumptions in a code comment
getPageJumpIndexassumes pages = 60 / step. Consider documenting this behavior in the implementation to help future maintainers (e.g., that “PageUp/Down” jumps one hour worth of minute options).src/app-components/TimePicker/tests/keyboardNavigation.test.ts (3)
13-14: Avoid duplicating exported types; import SegmentType from the source instead.Redefining SegmentType here risks drift if the production type changes. Import the type from the utils module to keep tests aligned with the implementation.
+import type { SegmentType } from 'src/app-components/TimePicker/utils/keyboardNavigation'; - -type SegmentType = 'hours' | 'minutes' | 'seconds' | 'period';
18-19: Remove double type assertions and centralize a mock key event helper.The pattern
as unknown as MockKeyboardEventis brittle. Build a tiny factory that returns the exact shape the handler requires; it keeps tests tight and type-safe.+const mkKeyEvt = (key: string): MockKeyboardEvent => ({ key, preventDefault: jest.fn() }); - -const mockEvent = { key: 'ArrowUp', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; +const mockEvent = mkKeyEvt('ArrowUp'); -const mockEvent = { key: 'ArrowDown', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; +const mockEvent = mkKeyEvt('ArrowDown'); -const mockEvent = { key: 'ArrowRight', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; +const mockEvent = mkKeyEvt('ArrowRight'); -const mockEvent = { key: 'ArrowLeft', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; +const mockEvent = mkKeyEvt('ArrowLeft'); -const mockEvent = { key: 'Enter', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; +const mockEvent = mkKeyEvt('Enter');Also applies to: 27-28, 35-36, 44-45, 53-54
199-218: Add a decrement-with-gaps constraint test for symmetry.You test increment skipping invalid values; add the mirror for decrement to prevent regressions.
it('should skip invalid values when decrementing', () => { const constraints = { min: 8, max: 12, validValues: [8, 10, 12] }; // Missing 9, 11 const result = handleValueDecrement(12, 'hours', 'HH:mm', constraints); expect(result).toBe(10); });src/app-components/TimePicker/tests/segmentTyping.test.ts (2)
121-153: Add buffer parsing tests for invalid input and seconds segment.Consider adding:
- processSegmentBuffer with non-numeric buffer for hours/minutes to assert it returns placeholder and isComplete=false.
- A seconds-specific case to ensure symmetry with minutes.
it('should handle non-numeric buffer', () => { expect(processSegmentBuffer('x', 'hours', false)).toEqual({ displayValue: '--', actualValue: null, isComplete: false, }); }); it('should handle seconds buffer like minutes', () => { expect(processSegmentBuffer('7', 'seconds', false)).toEqual({ displayValue: '07', actualValue: 7, isComplete: true, }); });
182-196: Consider a 12-hour commit + coerce integration assertion.commitSegmentValue(null, 'hours') returns 0. In 12-hour flows, coerceToValidRange should then correct 0 → 1. A quick assertion helps document that contract.
expect(coerceToValidRange(commitSegmentValue(null, 'hours') as number, 'hours', true)).toBe(1);src/app-components/TimePicker/tests/typingBehavior.test.tsx (5)
30-31: Prefer accessible queries over container.querySelector.Use role/label-based queries to match user-facing semantics and reduce brittleness.
-const hoursInput = container.querySelector('input[aria-label="Hours"]') as HTMLInputElement; +const hoursInput = screen.getByRole('textbox', { name: /hours/i }) as HTMLInputElement; -const minutesInput = container.querySelector('input[aria-label="Minutes"]') as HTMLInputElement; +const minutesInput = screen.getByRole('textbox', { name: /minutes/i }) as HTMLInputElement;Also applies to: 67-69, 99-103, 130-133, 168-174, 206-212, 233-239, 267-271, 303-310
37-43: Replace keyPress with userEvent.type (keypress is deprecated and flaky in React 18).keyPress is deprecated and can diverge across environments. userEvent.type simulates real typing (keydown/press/input/keyup) and reduces flakiness.
- fireEvent.keyPress(hoursInput, { key: '2', charCode: 50 }); + await userEvent.type(hoursInput, '2'); - fireEvent.keyPress(minutesInput, { key: '5', charCode: 53 }); + await userEvent.type(minutesInput, '5');Apply similarly for all digit inputs in this file.
Also applies to: 71-77, 103-109, 134-141, 172-179, 209-214, 249-252, 272-279, 314-319
13-16: Tighten fake timer cleanup.Clear pending timers before returning to real timers to avoid cross-test leakage.
afterEach(() => { jest.runOnlyPendingTimers(); + jest.clearAllTimers(); jest.useRealTimers(); });
49-54: Wrap timer advances in act for React 18.While waitFor often covers act boundaries, explicitly wrapping timer advances in act prevents subtle scheduler warnings.
import { act } from 'react-dom/test-utils'; // ... await act(async () => { jest.advanceTimersByTime(1100); });Also applies to: 79-86, 111-117, 147-153, 181-191, 214-220, 283-288
50-51: Avoid magic numbers; extract debounce to a constant used in both component and tests.If the component uses a debounce (e.g., 1000–1100ms), expose it or export a test-only constant to keep tests robust to tuning changes.
Also applies to: 80-81, 112-113, 148-149, 186-187
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (3)
8-24: Don’t re-declare interfaces already exported by the module under test.Import the types to prevent duplication/drift and ensure the tests fail if the public API changes incompatibly.
-import { - getNextValidValue, - getSegmentConstraints, - isTimeInRange, - parseTimeString, -} from 'src/app-components/TimePicker/utils/timeConstraintUtils'; +import { + getNextValidValue, + getSegmentConstraints, + isTimeInRange, + parseTimeString, + type TimeValue, + type TimeConstraints, + type SegmentConstraints, +} from 'src/app-components/TimePicker/utils/timeConstraintUtils'; - -interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -} - -interface TimeConstraints { - minTime?: string; - maxTime?: string; -} - -interface SegmentConstraints { - min: number; - max: number; - validValues: number[]; -}
151-166: Add a complementary minutes-at-max-hour constraint case.You covered minTime on minutes; add the mirror for maxTime to assert upper bound clipping.
it('should constrain minutes when on maxTime hour', () => { const currentTime: TimeValue = { hours: 16, minutes: 0, seconds: 0, period: 'PM' }; const constraints = { maxTime: '16:15' }; const result = getSegmentConstraints('minutes', currentTime, constraints, 'HH:mm'); expect(result.validValues).toEqual(Array.from({ length: 16 }, (_, i) => i)); // 0..15 });
89-120: Add equality-to-maxTime check.You asserted equals minTime; also assert equals maxTime to validate inclusive upper bound semantics.
it('should return true when time equals maxTime', () => { const sampleTime: TimeValue = { hours: 17, minutes: 0, seconds: 0, period: 'PM' }; const constraints = { minTime: '09:00', maxTime: '17:00' }; expect(isTimeInRange(sampleTime, constraints, 'HH:mm')).toBe(true); });src/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsx (4)
15-19: Restore scrollIntoView after tests to avoid cross-suite side effects.Mocking Element.prototype persists across tests. Restore it in afterEach.
beforeEach(() => { jest.clearAllMocks(); // Mock scrollIntoView Element.prototype.scrollIntoView = jest.fn(); }); + +afterEach(() => { + // Restore to a no-op function to prevent leakage across other suites + // eslint-disable-next-line @typescript-eslint/no-empty-function + Element.prototype.scrollIntoView = function () {}; +});
56-59: Avoid brittle text selector for localized headers.Using getByText('Timer') ties the test to a specific locale string. Prefer selecting the hour option directly (as you already do) or add stable testids/roles to the column containers.
-// Selected hour should be visually highlighted -const hoursColumn = screen.getByText('Timer').parentElement; -const selectedHour = within(hoursColumn!).getByRole('button', { name: '14' }); +// Selected hour should be visually highlighted +const selectedHour = screen.getByRole('button', { name: '14' }); expect(selectedHour).toHaveClass('dropdownOptionSelected');
410-431: scrollIntoView assertion: assert on the prototype mock to avoid element-bound ambiguity.When mocking the prototype, asserting on Element.prototype.scrollIntoView avoids confusion about which instance was called.
- expect(hour15.scrollIntoView).toHaveBeenCalledWith({ + expect(Element.prototype.scrollIntoView).toHaveBeenCalledWith({ behavior: 'smooth', block: 'nearest', });
557-565: Space key name should be ' ' (space), not 'Space'.DOM KeyboardEvent.key for space is a single space character. Using 'Space' is non-standard and may cause confusion.
-const ignoredKeys = ['Tab', 'Space', 'a', '1', 'Backspace']; +const ignoredKeys = ['Tab', ' ', 'a', '1', 'Backspace'];src/app-components/TimePicker/tests/timeFormatUtils.test.ts (3)
8-13: Prefer importing the shared TimeValue type to avoid driftDuplicate local interface risks divergence from the source of truth. Import the existing TimeValue instead.
Apply within this file:
-interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -}And add this import near the top:
import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
200-207: Align hour padding semantics across component and utilsThese tests assert that 12‑hour strings are not zero‑padded (e.g., '9:05 AM'). In TimePickerComponent, displayValue currently pads the 12‑hour hour segment to two digits. Please align the component to match these semantics or adjust tests if the intended UX is to pad. I’ve proposed a component-side fix below.
112-120: Add whitespace robustness tests for period parsingparseSegmentInput currently won’t accept inputs like ' am ' due to missing trim before comparison. Consider adding tests for whitespace-surrounded period inputs and address with the util fix I proposed in timeFormatUtils.ts.
Also applies to: 127-135
src/layout/TimePicker/TimePickerComponent.tsx (2)
68-85: ISO detection heuristic is brittleUsing value.includes('T') to detect ISO-like values can misclassify non-ISO strings that contain 'T'. Prefer checking a stricter ISO pattern or attempting Date.parse with a guarded try before formatting.
117-119: Confirm both disabled and readOnly props are neededIf TimePickerControl treats readOnly and disabled differently, carry both; otherwise, prefer a single prop to avoid divergent states.
src/app-components/TimePicker/utils/timeFormatUtils.ts (2)
60-66: Trim period input before case normalizationWithout trimming, values like ' am ' won’t parse.
- const upperInput = input.toUpperCase(); + const upperInput = input.trim().toUpperCase();
28-33: Optional: normalize period casing on outputFor consistency, return uppercase for the 'period' segment even if a lowercased string slips in.
- if (segmentType === 'period') { - return value.toString(); - } + if (segmentType === 'period') { + return value.toString().toUpperCase(); + }src/app-components/TimePicker/utils/dropdownBehavior.ts (2)
34-48: Initial highlight may be wrong in 12h mode if options are 1–12 but getHours() returns 0–23getInitialHighlightIndex uses systemTime.getHours() directly. Ensure the options' value domain matches (e.g., 0–23 for 24h lists) or map to 12h before lookup when used with 12h hours lists.
I can add a format argument and normalize hours accordingly if needed.
112-125: Safer nearest numeric lookup with heterogeneous optionsIf options include strings (e.g., 'AM', 'PM'), Number('AM') is NaN and the current logic happens to fall back to index 0. Prefer filtering numeric options for distance calculations to avoid NaN comparisons.
- // Find nearest numeric value - let nearestIndex = 0; - let nearestDiff = Math.abs(Number(options[0].value) - value); - - for (let i = 1; i < options.length; i++) { - const diff = Math.abs(Number(options[i].value) - value); - if (diff < nearestDiff) { - nearestDiff = diff; - nearestIndex = i; - } - } + // Find nearest numeric value + const numeric = options + .map((opt, i) => ({ i, v: typeof opt.value === 'number' ? opt.value : Number.NaN })) + .filter(({ v }) => Number.isFinite(v)); + if (numeric.length === 0) return 0; + let nearestIndex = numeric[0].i; + let nearestDiff = Math.abs(numeric[0].v - value); + for (let k = 1; k < numeric.length; k++) { + const diff = Math.abs(numeric[k].v - value); + if (diff < nearestDiff) { + nearestDiff = diff; + nearestIndex = numeric[k].i; + } + } return nearestIndex;src/app-components/TimePicker/utils/timeConstraintUtils.ts (2)
58-80: Time range computation is correct; consider small refactor for readabilityMinor: compute timeInSeconds directly without the intermediate minutes var.
21-56: Optional: centralize 12h⇄24h conversionparseTimeString embeds the conversion; extracting helpers would reduce duplication across modules and test fixtures.
src/app-components/TimePicker/components/TimeSegment.tsx (3)
272-279: PreferonBeforeInput/onKeyDownoveronKeyPress(deprecated).
onKeyPressis deprecated in modern React/DOM. Consider moving character handling toonBeforeInput(for text insertion) and keeping navigation/increment ononKeyDown. This improves IME and paste handling.Minimal change in render:
- onKeyPress={handleKeyPress} + onBeforeInput={handleBeforeInput} onKeyDown={handleKeyDown}And add a handler near other callbacks:
const handleBeforeInput = (e: React.FormEvent<HTMLInputElement> & { nativeEvent: InputEvent }) => { const ev = e.nativeEvent; if (ev.inputType === 'insertText' && typeof ev.data === 'string' && ev.data.length === 1) { e.preventDefault(); const char = ev.data; // reuse logic from handleKeyPress isTypingRef.current = true; const result = handleSegmentCharacterInput(char, type, segmentBuffer, format); if (result.shouldNavigate) { commitBuffer(true); onNavigate('right'); return; } setSegmentBuffer(result.newBuffer); bufferRef.current = result.newBuffer; const buffer = processSegmentBuffer(result.newBuffer, type, format.includes('a')); setLocalValue(buffer.displayValue); if (result.shouldAdvance) { if (buffer.actualValue !== null) { const committedValue = commitSegmentValue(buffer.actualValue, type); onValueChange(committedValue); } setSegmentBuffer(''); bufferRef.current = ''; isTypingRef.current = false; if (bufferTimeout) clearTimeout(bufferTimeout), setBufferTimeout(null); if (typingEndTimeout) clearTimeout(typingEndTimeout), setTypingEndTimeout(null); onNavigate('right'); } else { resetBufferTimeout(); } } };
295-297: Redundant ternary formaxLength.
type === 'period' ? 2 : 2is always 2.Apply this diff:
- maxLength={type === 'period' ? 2 : 2} + maxLength={2}
136-170: Arrow increment/decrement ignore constraints at the segment level.
handleValueIncrement/Decrementare called without constraints, producing intermediate invalid values the parent later corrects. This causes extra renders and a janky UX around boundaries.If you pass the per-segment
validValuestohandleValueIncrement/Decrement, arrows will skip disabled values immediately. That requires adding aconstraints?: SegmentConstraintsprop orvalidValues?: number[]toTimeSegmentPropsand wiring it fromTimePicker. Want me to draft that end-to-end change?src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
6-12:preventDefaultin result is redundant.You already call
event.preventDefault()insidehandleSegmentKeyDown. Returning apreventDefaultflag invites misuse and adds noise.Apply this diff:
export interface SegmentNavigationResult { shouldNavigate: boolean; direction?: 'left' | 'right'; shouldIncrement?: boolean; shouldDecrement?: boolean; - preventDefault: boolean; }And drop the
preventDefaultfields in return objects.src/app-components/TimePicker/components/TimePicker.tsx (5)
281-292: Useless initial assignment tonextIndex(CodeQL).
nextIndexis immediately overwritten. Simplify.Apply this diff:
- const handleSegmentNavigate = (direction: 'left' | 'right', currentIndex: number) => { - let nextIndex = currentIndex; - - if (direction === 'right') { - nextIndex = (currentIndex + 1) % segments.length; - } else { - nextIndex = (currentIndex - 1 + segments.length) % segments.length; - } + const handleSegmentNavigate = (direction: 'left' | 'right', currentIndex: number) => { + const nextIndex = + direction === 'right' + ? (currentIndex + 1) % segments.length + : (currentIndex - 1 + segments.length) % segments.length;
697-704: Hardcoded Norwegian/English labels in dropdown; uselabelsprop for i18n consistency.Column headers use fixed strings ("Timer", "Minutter", "Sekunder", "AM/PM") while segments accept
labels. This mixes locales and blocks translation.Apply this diff to reuse
segmentLabels:- <div className={styles.dropdownLabel}>Timer</div> + <div className={styles.dropdownLabel}>{segmentLabels.hours}</div> ... - <div className={styles.dropdownLabel}>Minutter</div> + <div className={styles.dropdownLabel}>{segmentLabels.minutes}</div> ... - <div className={styles.dropdownLabel}>Sekunder</div> + <div className={styles.dropdownLabel}>{segmentLabels.seconds}</div> ... - <div className={styles.dropdownLabel}>AM/PM</div> + <div className={styles.dropdownLabel}>{segmentLabels.period}</div>Also applies to: 742-747, 781-785, 821-825
669-696: Dialog a11y: add an accessible name.Popover content has
role='dialog'but noaria-labelledbyoraria-label. Provide an accessible name. If you don’t have a visible heading, addaria-labelon the dialog.Example:
- <Popover + <Popover ref={dropdownRef} className={styles.timePickerDropdown} aria-modal - aria-hidden={!showDropdown} + aria-hidden={!showDropdown} role='dialog' open={showDropdown} + aria-label='Time picker'Or reference a visible heading element by id via
aria-labelledby.Also applies to: 681-694
15-36:aria-labelledby?: neverunnecessarily forbids a common a11y pattern.Disallowing
aria-labelledbyprevents integrating with external labels. Consider allowing it alongsidearia-label.If this was intentional (design system constraint), ignore. Otherwise, remove
neverand let standard ARIA patterns work.
409-445: AM/PM enablement not constrained; period may be selectable even when all hours in that period are invalid.Cases with tight
minTime/maxTimecan make an entire period invalid (e.g., only morning times allowed). AM/PM buttons are always enabled.We can disable the AM/PM option if switching would yield no valid hour/minute/second combination. I can implement a helper that probes
getSegmentConstraintsfor the target period and returns disabled whenhours.validValuesis empty (and minutes/seconds compatible). Want me to draft it?src/app-components/TimePicker/utils/segmentTyping.ts (1)
114-116:TabinisNavigationKeyis ineffective here and could be misleading.
isNavigationKeyincludes'Tab', but you only call it fromonKeyPress(or plannedonBeforeInput).Tabis processed on keydown and will not be handled here.Either remove
'Tab'fromisNavigationKeyor handleTabin the keydown handler (committing buffer before default tabbing).Also applies to: 244-249
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (23)
src/app-components/TimePicker/README.md(1 hunks)src/app-components/TimePicker/components/TimePicker.module.css(1 hunks)src/app-components/TimePicker/components/TimePicker.tsx(1 hunks)src/app-components/TimePicker/components/TimeSegment.tsx(1 hunks)src/app-components/TimePicker/tests/TimeSegment.test.tsx(1 hunks)src/app-components/TimePicker/tests/dropdownBehavior.test.ts(1 hunks)src/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsx(1 hunks)src/app-components/TimePicker/tests/keyboardNavigation.test.ts(1 hunks)src/app-components/TimePicker/tests/segmentTyping.test.ts(1 hunks)src/app-components/TimePicker/tests/timeConstraintUtils.test.ts(1 hunks)src/app-components/TimePicker/tests/timeFormatUtils.test.ts(1 hunks)src/app-components/TimePicker/tests/typingBehavior.test.tsx(1 hunks)src/app-components/TimePicker/utils/dropdownBehavior.ts(1 hunks)src/app-components/TimePicker/utils/keyboardNavigation.ts(1 hunks)src/app-components/TimePicker/utils/segmentTyping.ts(1 hunks)src/app-components/TimePicker/utils/timeConstraintUtils.ts(1 hunks)src/app-components/TimePicker/utils/timeFormatUtils.ts(1 hunks)src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)src/layout/TimePicker/TimePickerComponent.test.tsx(1 hunks)src/layout/TimePicker/TimePickerComponent.tsx(1 hunks)src/layout/TimePicker/useTimePickerValidation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/language/texts/en.ts
- src/layout/TimePicker/TimePickerComponent.test.tsx
- src/language/texts/nn.ts
- src/layout/TimePicker/useTimePickerValidation.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/tests/timeFormatUtils.test.tssrc/app-components/TimePicker/tests/timeConstraintUtils.test.tssrc/app-components/TimePicker/utils/timeConstraintUtils.tssrc/app-components/TimePicker/tests/segmentTyping.test.tssrc/app-components/TimePicker/utils/dropdownBehavior.tssrc/app-components/TimePicker/tests/dropdownBehavior.test.tssrc/app-components/TimePicker/utils/keyboardNavigation.tssrc/app-components/TimePicker/utils/segmentTyping.tssrc/app-components/TimePicker/tests/TimeSegment.test.tsxsrc/app-components/TimePicker/tests/typingBehavior.test.tsxsrc/app-components/TimePicker/tests/keyboardNavigation.test.tssrc/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsxsrc/app-components/TimePicker/utils/timeFormatUtils.tssrc/app-components/TimePicker/components/TimeSegment.tsxsrc/language/texts/nb.tssrc/layout/TimePicker/TimePickerComponent.tsxsrc/app-components/TimePicker/components/TimePicker.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/app-components/TimePicker/tests/timeFormatUtils.test.tssrc/app-components/TimePicker/tests/timeConstraintUtils.test.tssrc/app-components/TimePicker/tests/segmentTyping.test.tssrc/app-components/TimePicker/tests/dropdownBehavior.test.tssrc/app-components/TimePicker/tests/TimeSegment.test.tsxsrc/app-components/TimePicker/tests/typingBehavior.test.tsxsrc/app-components/TimePicker/tests/keyboardNavigation.test.tssrc/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/app-components/TimePicker/components/TimePicker.module.css
🧠 Learnings (1)
📚 Learning: 2025-08-22T13:53:28.201Z
Learnt from: CR
PR: Altinn/app-frontend-react#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-22T13:53:28.201Z
Learning: Applies to **/*.module.css : Use CSS Modules for component styling and follow existing patterns in `*.module.css` files
Applied to files:
src/app-components/TimePicker/components/TimePicker.module.css
🧬 Code graph analysis (16)
src/app-components/TimePicker/tests/timeFormatUtils.test.ts (2)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/utils/timeFormatUtils.ts (4)
formatTimeValue(5-26)formatSegmentValue(28-49)parseSegmentInput(51-75)isValidSegmentInput(77-117)
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (1)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (7)
TimeValue(3-8)TimeConstraints(10-13)SegmentConstraints(15-19)parseTimeString(21-56)isTimeInRange(58-80)getSegmentConstraints(82-189)getNextValidValue(191-217)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)
src/app-components/TimePicker/tests/segmentTyping.test.ts (1)
src/app-components/TimePicker/utils/segmentTyping.ts (9)
processHourInput(18-67)processMinuteInput(72-95)processPeriodInput(100-109)processSegmentBuffer(120-153)isNavigationKey(114-115)clearSegment(158-161)commitSegmentValue(166-174)coerceToValidRange(179-205)shouldAdvanceSegment(210-226)
src/app-components/TimePicker/utils/dropdownBehavior.ts (1)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)
src/app-components/TimePicker/tests/dropdownBehavior.test.ts (1)
src/app-components/TimePicker/utils/dropdownBehavior.ts (9)
roundToStep(11-16)getInitialHighlightIndex(21-51)getNextIndex(56-62)getPageJumpIndex(67-81)getHomeIndex(86-86)getEndIndex(91-91)findNearestOptionIndex(96-125)calculateScrollPosition(130-137)shouldScrollToOption(142-157)
src/app-components/TimePicker/utils/keyboardNavigation.ts (2)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
SegmentConstraints(15-19)
src/app-components/TimePicker/utils/segmentTyping.ts (2)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)
src/app-components/TimePicker/tests/TimeSegment.test.tsx (1)
src/app-components/TimePicker/components/TimeSegment.tsx (2)
TimeSegmentProps(20-36)TimeSegment(38-300)
src/app-components/TimePicker/tests/typingBehavior.test.tsx (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimePicker(87-854)
src/app-components/TimePicker/tests/keyboardNavigation.test.ts (1)
src/app-components/TimePicker/utils/keyboardNavigation.ts (5)
SegmentType(4-4)handleSegmentKeyDown(14-58)getNextSegmentIndex(60-72)handleValueIncrement(74-110)handleValueDecrement(112-148)
src/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsx (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimePicker(87-854)
src/app-components/TimePicker/utils/timeFormatUtils.ts (3)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)
src/app-components/TimePicker/components/TimeSegment.tsx (4)
src/app-components/TimePicker/utils/keyboardNavigation.ts (4)
SegmentType(4-4)handleSegmentKeyDown(14-58)handleValueIncrement(74-110)handleValueDecrement(112-148)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatSegmentValue(28-49)src/app-components/TimePicker/utils/segmentTyping.ts (4)
processSegmentBuffer(120-153)commitSegmentValue(166-174)clearSegment(158-161)handleSegmentCharacterInput(231-285)
src/layout/TimePicker/TimePickerComponent.tsx (7)
src/layout/index.ts (1)
PropsFromGenericComponent(28-32)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/features/formData/useDataModelBindings.ts (1)
useDataModelBindings(42-57)src/features/language/useLanguage.ts (1)
useLanguage(90-93)src/utils/layout/useLabel.tsx (1)
useLabel(13-72)src/layout/ComponentStructureWrapper.tsx (1)
ComponentStructureWrapper(20-48)src/app-components/Flex/Flex.tsx (1)
Flex(25-84)
src/app-components/TimePicker/components/TimePicker.tsx (4)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (4)
parseTimeString(21-56)TimeValue(3-8)TimeConstraints(10-13)getSegmentConstraints(82-189)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatTimeValue(5-26)src/app-components/TimePicker/components/TimeSegment.tsx (1)
TimeSegment(38-300)
🪛 GitHub Check: CodeQL
src/app-components/TimePicker/components/TimePicker.tsx
[warning] 282-282: Useless assignment to local variable
The initial value of nextIndex is unused, since it is always overwritten.
🪛 LanguageTool
src/app-components/TimePicker/README.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...- Auto-advance: Automatically moves to next segment when current segment is co...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...**: Automatically moves to next segment when current segment is complete ### Keyboa...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ... Type ":", ".", "," or space to advance to next segment ### Format Support - **2...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...24-hour format*: "HH:mm" or "HH:mm:ss" - 12-hour format: "HH:mm a" or "HH:mm:ss...
(QB_NEW_EN)
[grammar] ~28-~28: There might be a mistake here.
Context: ...: "HH:mm a" or "HH:mm:ss a" (with AM/PM) - Flexible display: Configurable time fo...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...t for hours, minutes, seconds, or period - Implements Chrome-like typing behavior w...
(QB_NEW_EN)
[grammar] ~84-~84: There might be a mistake here.
Context: ...e typing behavior with buffer management - Handles keyboard navigation and value co...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...ercion logic for different segment types - Buffer Management: Handles multi-chara...
(QB_NEW_EN)
[grammar] ~92-~92: There might be a mistake here.
Context: ...dles multi-character input with timeouts - Validation: Ensures values stay within...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...*: Arrow key navigation between segments - Value Manipulation: Increment/decremen...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...n**: Increment/decrement with arrow keys - Key Handling: Special key processing (...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...for second digit, 3-9 auto-coerces to 0X - 12-hour mode: First digit 0-1 waits fo...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...for second digit, 2-9 auto-coerces to 0X - Second digit: Validates against first ...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...for second digit, 6-9 auto-coerces to 0X - Second digit: Always accepts 0-9 - **O...
(QB_NEW_EN)
[grammar] ~118-~118: There might be a mistake here.
Context: ...X - Second digit: Always accepts 0-9 - Overflow handling: Values > 59 are cor...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...Input (AM/PM) - A/a key: Sets to AM - P/p key: Sets to PM - **Case insensiti...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...**: Sets to AM - P/p key: Sets to PM - Case insensitive: Accepts both upper a...
(QB_NEW_EN)
[grammar] ~138-~138: There might be a mistake here.
Context: ...bility - ARIA Labels: Each segment has descriptive aria-label - **Keyboard Nav...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...*: Various input patterns and edge cases - Navigation: Keyboard navigation betwee...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...**: Keyboard navigation between segments - Buffer Management: Race condition prev...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ...ondition prevention and timeout handling - Format Support: Different time formats...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...*: Different time formats and validation - Accessibility: Screen reader compatibi...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Install
🔇 Additional comments (16)
src/app-components/TimePicker/tests/TimeSegment.test.tsx (2)
1-19: Guideline check: renderWithProviders only if neededThese are unit tests for a leaf component with no app context. If the component later starts reading from context, switch to
renderWithProvidersper repo guidelines. For now,renderis fine.
3-5: The scripts above will print the full test file (first 200 lines) and locate anyuserEvent.usage to confirm where the v14 API refactor is needed. Once you provide the output, I can verify and draft the updated review comment.src/app-components/TimePicker/tests/dropdownBehavior.test.ts (1)
1-12: LGTM: Clean, deterministic coverage of dropdown utilitiesGood breadth across rounding, navigation, paging, home/end, nearest, and scroll math. Test data is straightforward and readable.
src/app-components/TimePicker/tests/keyboardNavigation.test.ts (1)
15-62: Overall: solid coverage and intent tests for navigation.Coverage across arrows, wrapping, 12/24h, and constraint-aware increments/decrements looks good and maps cleanly to the utility semantics.
Also applies to: 64-98, 100-150, 152-197
src/app-components/TimePicker/tests/segmentTyping.test.ts (2)
73-100: Nice, readable minute typing behavior coverage.The cases mirror browser behavior and the utility logic closely.
13-71: Overall: strong segmentation logic tests.The hour typing edge handling for 12/24h, navigation key detection, and range coercion scenarios are well covered.
Also applies to: 102-171, 173-181, 198-243
src/app-components/TimePicker/tests/typingBehavior.test.tsx (1)
7-12: Overall: excellent behavioral coverage for the “no initial value” bug and buffer semantics.Scenarios around overwrites, parent updates, and blur/focus are thorough.
Also applies to: 18-28, 56-86, 119-154, 156-191, 194-221, 223-253, 255-320
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (1)
26-87: Solid baseline coverage for parsing and core constraints.Parsing 12/24h, seconds, inclusive range checks, and hour/minute constraints are well-exercised.
Also applies to: 122-166, 168-218
src/app-components/TimePicker/tests/dropdownKeyboardNavigation.test.tsx (1)
45-79: End-to-end dropdown keyboard coverage is comprehensive.Opening/closing semantics, focus restoration, wrapping, constraints, and 12h specifics are well validated.
Also applies to: 81-215, 217-351, 353-475, 477-531, 533-569
src/app-components/TimePicker/tests/timeFormatUtils.test.ts (1)
1-207: Good coverage and edge casesOverall, the suite exercises formatting, segment formatting, parsing, validation, and boundaries well. Nice job covering noon/midnight and partial inputs.
Also applies to: 209-217
src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
5-26: Utilities look solid and match testsClear separation of formatting, segment formatting, parsing, and validation with sensible handling of partial inputs and ranges.
Also applies to: 28-49, 51-75, 77-117
src/app-components/TimePicker/utils/dropdownBehavior.ts (1)
1-16: Navigation and scrolling helpers look goodRound/next/home/end/scrolling logic is straightforward and clear.
Also applies to: 21-51, 53-63, 83-92, 130-137, 142-157
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
82-131: Constraints logic is clear and consistent across segmentsGood normalization of 12h/24h bounds and contextual minute/second limits at boundary hours/minutes.
Also applies to: 133-189, 191-217
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
74-110: LGTM: wrapped increment semantics are correct and align with 12h/24h rules.Default wrapping for hours/minutes/seconds and AM/PM toggling look good.
src/app-components/TimePicker/utils/segmentTyping.ts (2)
18-66: Hour input coercion mirrors native behavior—nice.Smart handling of first/second digit across 12h/24h is solid and matches Chrome-like expectations.
176-205: Ignore removal suggestion: functions are used in testsI confirmed that both
coerceToValidRangeandshouldAdvanceSegmentare explicitly imported and exercised insrc/app-components/TimePicker/tests/segmentTyping.test.ts. They are not unused—removing them would break the existing tests. If you still want to centralize clamping and auto-advance behavior intoTimeSegment, feel free to invoke these helpers there, but they must remain defined for the current test suite.Likely an incorrect or invalid review comment.
| // Mobile: Use native time input | ||
| if (isMobile) { | ||
| const mobileValue = `${String(timeValue.hours).padStart(2, '0')}:${String(timeValue.minutes).padStart(2, '0')}`; | ||
|
|
||
| return ( | ||
| <div className={styles.calendarInputWrapper}> | ||
| <input | ||
| type='time' | ||
| id={id} | ||
| value={mobileValue} | ||
| onChange={(e) => onChange(e.target.value)} | ||
| disabled={disabled} | ||
| readOnly={readOnly} | ||
| required={required} | ||
| autoComplete={autoComplete} | ||
| aria-label={ariaLabel} | ||
| aria-describedby={ariaDescribedBy} | ||
| aria-invalid={ariaInvalid} | ||
| className={styles.nativeInput} | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Mobile path returns raw <input type="time"> value, breaking format contract and dropping seconds.
On mobile you call onChange(e.target.value) directly. This returns a 24h HH:mm (or HH:mm:ss if step set) string regardless of format (e.g., it omits AM/PM when format is 'hh:mm a'). It also discards seconds when format includes seconds.
Apply this diff to normalize the native value back to the configured format and support seconds:
if (isMobile) {
const mobileValue = `${String(timeValue.hours).padStart(2, '0')}:${String(timeValue.minutes).padStart(2, '0')}`;
return (
<div className={styles.calendarInputWrapper}>
<input
type='time'
id={id}
value={mobileValue}
- onChange={(e) => onChange(e.target.value)}
+ onChange={(e) => {
+ const parts = e.target.value.split(':');
+ const hours = parseInt(parts[0] || '0', 10) || 0;
+ const minutes = parseInt(parts[1] || '0', 10) || 0;
+ const seconds = includesSeconds ? parseInt(parts[2] || '0', 10) || 0 : 0;
+ const period: TimeValue['period'] = is12Hour ? (hours >= 12 ? 'PM' : 'AM') : 'AM';
+ const newTime: TimeValue = { hours, minutes, seconds, period };
+ onChange(formatTimeValue(newTime, format));
+ }}
disabled={disabled}
readOnly={readOnly}
required={required}
autoComplete={autoComplete}
aria-label={ariaLabel}
aria-describedby={ariaDescribedBy}
aria-invalid={ariaInvalid}
className={styles.nativeInput}
+ {...(includesSeconds ? { step: 1 } : {})}
/>
</div>
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Mobile: Use native time input | |
| if (isMobile) { | |
| const mobileValue = `${String(timeValue.hours).padStart(2, '0')}:${String(timeValue.minutes).padStart(2, '0')}`; | |
| return ( | |
| <div className={styles.calendarInputWrapper}> | |
| <input | |
| type='time' | |
| id={id} | |
| value={mobileValue} | |
| onChange={(e) => onChange(e.target.value)} | |
| disabled={disabled} | |
| readOnly={readOnly} | |
| required={required} | |
| autoComplete={autoComplete} | |
| aria-label={ariaLabel} | |
| aria-describedby={ariaDescribedBy} | |
| aria-invalid={ariaInvalid} | |
| className={styles.nativeInput} | |
| /> | |
| </div> | |
| ); | |
| } | |
| // Mobile: Use native time input | |
| if (isMobile) { | |
| const mobileValue = `${String(timeValue.hours).padStart(2, '0')}:${String(timeValue.minutes).padStart(2, '0')}`; | |
| return ( | |
| <div className={styles.calendarInputWrapper}> | |
| <input | |
| type='time' | |
| id={id} | |
| value={mobileValue} | |
| onChange={(e) => { | |
| const parts = e.target.value.split(':'); | |
| const hours = parseInt(parts[0] || '0', 10) || 0; | |
| const minutes = parseInt(parts[1] || '0', 10) || 0; | |
| const seconds = includesSeconds ? parseInt(parts[2] || '0', 10) || 0 : 0; | |
| const period: TimeValue['period'] = is12Hour ? (hours >= 12 ? 'PM' : 'AM') : 'AM'; | |
| const newTime: TimeValue = { hours, minutes, seconds, period }; | |
| onChange(formatTimeValue(newTime, format)); | |
| }} | |
| disabled={disabled} | |
| readOnly={readOnly} | |
| required={required} | |
| autoComplete={autoComplete} | |
| aria-label={ariaLabel} | |
| aria-describedby={ariaDescribedBy} | |
| aria-invalid={ariaInvalid} | |
| className={styles.nativeInput} | |
| {...(includesSeconds ? { step: 1 } : {})} | |
| /> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimePicker.tsx around lines 557 to
579, the mobile branch currently calls onChange(e.target.value) and thus returns
a raw "HH:mm" (or "HH:mm:ss") native value which breaks the component's external
format contract and drops AM/PM or seconds; instead parse e.target.value into
hours, minutes and optional seconds (handle empty string), convert to the
internal timeValue representation, then reformat that time according to the
configured format prop (including converting to 12-hour with AM/PM when format
is 'hh:mm a' and preserving seconds when format includes them) and call onChange
with that formatted string; ensure disabled/readOnly/required behavior unchanged
and respect step/seconds support when parsing and formatting.
| const handleFocus = (e: React.FocusEvent<HTMLInputElement>) => { | ||
| // Don't clear buffer if we're already focused and typing | ||
| const wasAlreadyFocused = inputRef.current === document.activeElement; | ||
|
|
||
| if (!wasAlreadyFocused) { | ||
| // Clear buffer and select all text only on fresh focus | ||
| setSegmentBuffer(''); | ||
| bufferRef.current = ''; | ||
| isTypingRef.current = false; // End typing state on fresh focus | ||
| if (bufferTimeout) { | ||
| clearTimeout(bufferTimeout); | ||
| setBufferTimeout(null); | ||
| } | ||
| if (typingEndTimeout) { | ||
| clearTimeout(typingEndTimeout); | ||
| setTypingEndTimeout(null); | ||
| } | ||
| e.target.select(); | ||
| } | ||
|
|
||
| onFocus?.(); | ||
| }; |
There was a problem hiding this comment.
Focus detection is incorrect; fresh-focus branch never runs.
wasAlreadyFocused = inputRef.current === document.activeElement is always true inside onFocus, so you never enter the “fresh focus” branch. Use e.relatedTarget to detect focus transitions instead.
Apply this diff:
- const handleFocus = (e: React.FocusEvent<HTMLInputElement>) => {
- // Don't clear buffer if we're already focused and typing
- const wasAlreadyFocused = inputRef.current === document.activeElement;
-
- if (!wasAlreadyFocused) {
+ const handleFocus = (e: React.FocusEvent<HTMLInputElement>) => {
+ // Clear on fresh focus (coming from another element), not when refocusing same element
+ const cameFromSelf = e.relatedTarget === e.currentTarget;
+ if (!cameFromSelf) {
// Clear buffer and select all text only on fresh focus
setSegmentBuffer('');
bufferRef.current = '';
isTypingRef.current = false; // End typing state on fresh focus
if (bufferTimeout) {
clearTimeout(bufferTimeout);
setBufferTimeout(null);
}
if (typingEndTimeout) {
clearTimeout(typingEndTimeout);
setTypingEndTimeout(null);
}
e.target.select();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleFocus = (e: React.FocusEvent<HTMLInputElement>) => { | |
| // Don't clear buffer if we're already focused and typing | |
| const wasAlreadyFocused = inputRef.current === document.activeElement; | |
| if (!wasAlreadyFocused) { | |
| // Clear buffer and select all text only on fresh focus | |
| setSegmentBuffer(''); | |
| bufferRef.current = ''; | |
| isTypingRef.current = false; // End typing state on fresh focus | |
| if (bufferTimeout) { | |
| clearTimeout(bufferTimeout); | |
| setBufferTimeout(null); | |
| } | |
| if (typingEndTimeout) { | |
| clearTimeout(typingEndTimeout); | |
| setTypingEndTimeout(null); | |
| } | |
| e.target.select(); | |
| } | |
| onFocus?.(); | |
| }; | |
| const handleFocus = (e: React.FocusEvent<HTMLInputElement>) => { | |
| // Clear on fresh focus (coming from another element), not when refocusing same element | |
| const cameFromSelf = e.relatedTarget === e.currentTarget; | |
| if (!cameFromSelf) { | |
| // Clear buffer and select all text only on fresh focus | |
| setSegmentBuffer(''); | |
| bufferRef.current = ''; | |
| isTypingRef.current = false; // End typing state on fresh focus | |
| if (bufferTimeout) { | |
| clearTimeout(bufferTimeout); | |
| setBufferTimeout(null); | |
| } | |
| if (typingEndTimeout) { | |
| clearTimeout(typingEndTimeout); | |
| setTypingEndTimeout(null); | |
| } | |
| e.target.select(); | |
| } | |
| onFocus?.(); | |
| }; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimeSegment.tsx around lines
220-241, the focus check using document.activeElement always evaluates true
inside the onFocus handler so the “fresh focus” branch never runs; replace that
check with one based on e.relatedTarget to detect focus transitions. Concretely,
compute wasAlreadyFocused from e.relatedTarget (e.g. const wasAlreadyFocused =
e.relatedTarget === inputRef.current || (e.relatedTarget instanceof Node &&
inputRef.current?.contains(e.relatedTarget as Node))); keep the existing
buffer/timeouts clearing and e.target.select() only when wasAlreadyFocused is
false, and ensure any necessary TypeScript narrowing/cast for relatedTarget is
applied, then call onFocus?.() as before.
| export const getPageJumpIndex = ( | ||
| currentIndex: number, | ||
| direction: 'up' | 'down', | ||
| totalOptions: number, | ||
| stepMinutes: number, | ||
| ): number => { | ||
| // Calculate how many items represent 60 minutes | ||
| const itemsToJump = Math.max(1, Math.floor(60 / stepMinutes)); | ||
|
|
||
| if (direction === 'up') { | ||
| return Math.max(0, currentIndex - itemsToJump); | ||
| } else { | ||
| return Math.min(totalOptions - 1, currentIndex + itemsToJump); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Guard against invalid stepMinutes to avoid NaN/Infinity jumps
When stepMinutes ≤ 0 or not finite, Math.floor(60 / stepMinutes) yields NaN/Infinity, leading to NaN indices. Add a defensive default.
export const getPageJumpIndex = (
currentIndex: number,
direction: 'up' | 'down',
totalOptions: number,
stepMinutes: number,
): number => {
- // Calculate how many items represent 60 minutes
- const itemsToJump = Math.max(1, Math.floor(60 / stepMinutes));
+ // Calculate how many items represent 60 minutes
+ const denom = Number.isFinite(stepMinutes) && stepMinutes > 0 ? stepMinutes : 1;
+ const itemsToJump = Math.max(1, Math.floor(60 / denom));
if (direction === 'up') {
return Math.max(0, currentIndex - itemsToJump);
} else {
return Math.min(totalOptions - 1, currentIndex + itemsToJump);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getPageJumpIndex = ( | |
| currentIndex: number, | |
| direction: 'up' | 'down', | |
| totalOptions: number, | |
| stepMinutes: number, | |
| ): number => { | |
| // Calculate how many items represent 60 minutes | |
| const itemsToJump = Math.max(1, Math.floor(60 / stepMinutes)); | |
| if (direction === 'up') { | |
| return Math.max(0, currentIndex - itemsToJump); | |
| } else { | |
| return Math.min(totalOptions - 1, currentIndex + itemsToJump); | |
| } | |
| }; | |
| export const getPageJumpIndex = ( | |
| currentIndex: number, | |
| direction: 'up' | 'down', | |
| totalOptions: number, | |
| stepMinutes: number, | |
| ): number => { | |
| // Calculate how many items represent 60 minutes | |
| const denom = Number.isFinite(stepMinutes) && stepMinutes > 0 ? stepMinutes : 1; | |
| const itemsToJump = Math.max(1, Math.floor(60 / denom)); | |
| if (direction === 'up') { | |
| return Math.max(0, currentIndex - itemsToJump); | |
| } else { | |
| return Math.min(totalOptions - 1, currentIndex + itemsToJump); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/utils/dropdownBehavior.ts around lines 67 to
81, the calculation itemsToJump = Math.max(1, Math.floor(60 / stepMinutes)) can
produce NaN or Infinity when stepMinutes is ≤ 0 or not finite; guard against
invalid stepMinutes by validating it first (e.g., if !isFinite(stepMinutes) ||
stepMinutes <= 0 then treat stepMinutes as 1 or directly set itemsToJump = 1),
then compute itemsToJump using the validated value and keep the existing
Math.max(1, Math.floor(...)) clamp; this prevents NaN/Infinity and ensures
returned indices remain numeric and within bounds.
| const prevValid = validValues.reverse().find((v) => v < currentValue); | ||
| return prevValid ?? null; | ||
| } |
There was a problem hiding this comment.
Bug: reverse() mutates validValues, corrupting subsequent navigation
Using Array.prototype.reverse() changes the original validValues array, which will break future calls. Iterate from the end instead.
- } else {
- const prevValid = validValues.reverse().find((v) => v < currentValue);
- return prevValid ?? null;
- }
+ } else {
+ for (let i = validValues.length - 1; i >= 0; i--) {
+ if (validValues[i] < currentValue) {
+ return validValues[i];
+ }
+ }
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const prevValid = validValues.reverse().find((v) => v < currentValue); | |
| return prevValid ?? null; | |
| } | |
| } else { | |
| for (let i = validValues.length - 1; i >= 0; i--) { | |
| if (validValues[i] < currentValue) { | |
| return validValues[i]; | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/utils/timeConstraintUtils.ts around lines 205
to 207, using validValues.reverse() mutates the original array and corrupts
future navigation; replace that with a non-mutating approach (either iterate
from the end with an index-based loop to find the first value < currentValue, or
operate on a shallow copy like validValues.slice().reverse() before find) and
keep the same return behavior (return the found value or null).
| // Use different padding logic for 12-hour vs 24-hour format | ||
| const hoursStr = is12Hour ? displayHours.toString() : displayHours.toString().padStart(2, '0'); | ||
| const minutesStr = time.minutes.toString().padStart(2, '0'); | ||
| const secondsStr = includesSeconds ? `:${time.seconds.toString().padStart(2, '0')}` : ''; | ||
| const periodStr = is12Hour ? ` ${time.period}` : ''; | ||
|
|
||
| return `${hoursStr}:${minutesStr}${secondsStr}${periodStr}`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Derive AM/PM from hours to avoid stale/incorrect period
formatTimeValue uses time.period as-is, which can be wrong when callers provide a 24h TimeValue (e.g., from parseTimeString for 'HH:mm', which sets period to 'AM' by default). Deriving period from time.hours removes this coupling.
- const periodStr = is12Hour ? ` ${time.period}` : '';
+ const computedPeriod = time.hours >= 12 ? 'PM' : 'AM';
+ const periodStr = is12Hour ? ` ${computedPeriod}` : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use different padding logic for 12-hour vs 24-hour format | |
| const hoursStr = is12Hour ? displayHours.toString() : displayHours.toString().padStart(2, '0'); | |
| const minutesStr = time.minutes.toString().padStart(2, '0'); | |
| const secondsStr = includesSeconds ? `:${time.seconds.toString().padStart(2, '0')}` : ''; | |
| const periodStr = is12Hour ? ` ${time.period}` : ''; | |
| return `${hoursStr}:${minutesStr}${secondsStr}${periodStr}`; | |
| }; | |
| // Use different padding logic for 12-hour vs 24-hour format | |
| const hoursStr = is12Hour | |
| ? displayHours.toString() | |
| : displayHours.toString().padStart(2, '0'); | |
| const minutesStr = time.minutes.toString().padStart(2, '0'); | |
| const secondsStr = includesSeconds | |
| ? `:${time.seconds.toString().padStart(2, '0')}` | |
| : ''; | |
| const computedPeriod = time.hours >= 12 ? 'PM' : 'AM'; | |
| const periodStr = is12Hour ? ` ${computedPeriod}` : ''; | |
| return `${hoursStr}:${minutesStr}${secondsStr}${periodStr}`; | |
| }; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/utils/timeFormatUtils.ts around lines 19 to 26,
formatTimeValue currently uses time.period directly which can be stale for
24-hour inputs; change it to derive the AM/PM period from time.hours (e.g.,
period = time.hours >= 12 ? 'PM' : 'AM') and use that only when is12Hour is
true, ensuring you compute displayHours for 12-hour rendering (convert 0/12 to
12, use hours % 12 for others) so the returned string reflects the actual hours
value rather than the provided time.period.
| const period = timeString.match(/(AM|PM)/i)?.[0]; | ||
|
|
||
| let adjustedHours = hours; | ||
| if (period === 'PM' && hours !== 12) { | ||
| adjustedHours += 12; | ||
| } | ||
| if (period === 'AM' && hours === 12) { | ||
| adjustedHours = 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
AM/PM parsing is case-sensitive; 'pm' will not convert to 24h correctly
period is taken directly from the regex match, so 'pm' won’t match 'PM' in the conversion checks. Uppercase the match before comparing to avoid wrong hours around noon/midnight.
- const period = timeString.match(/(AM|PM)/i)?.[0];
+ const period = timeString.match(/(AM|PM)/i)?.[0]?.toUpperCase();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const period = timeString.match(/(AM|PM)/i)?.[0]; | |
| let adjustedHours = hours; | |
| if (period === 'PM' && hours !== 12) { | |
| adjustedHours += 12; | |
| } | |
| if (period === 'AM' && hours === 12) { | |
| adjustedHours = 0; | |
| } | |
| const period = timeString.match(/(AM|PM)/i)?.[0]?.toUpperCase(); | |
| let adjustedHours = hours; | |
| if (period === 'PM' && hours !== 12) { | |
| adjustedHours += 12; | |
| } | |
| if (period === 'AM' && hours === 12) { | |
| adjustedHours = 0; | |
| } |
🤖 Prompt for AI Agents
In src/layout/TimePicker/TimePickerComponent.tsx around lines 46 to 55, the
AM/PM value from the regex match can be lowercase so comparisons against
'PM'/'AM' fail; normalize the match before comparing (e.g. convert the matched
period to uppercase or use case-insensitive checks) and handle the possible
undefined match safely, then use the normalized value in both the PM and AM
conversion branches so noon/midnight hour adjustments work correctly.
| if (format.includes('a')) { | ||
| const period = hours >= 12 ? 'PM' : 'AM'; | ||
| const displayHours = hours === 0 ? 12 : hours > 12 ? hours - 12 : hours; | ||
| const timeStr = `${displayHours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}`; | ||
| const secondsStr = format.includes('ss') ? `:${seconds.toString().padStart(2, '0')}` : ''; | ||
| return `${timeStr}${secondsStr} ${period}`; | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
12‑hour display pads hours to 2 digits; utils/tests expect no padding
formatTimeValue intentionally doesn’t pad 12‑hour hours (tests assert '9:05 AM', not '09:05 AM'). Align component display for consistency.
- const displayHours = hours === 0 ? 12 : hours > 12 ? hours - 12 : hours;
- const timeStr = `${displayHours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}`;
+ const displayHours = hours === 0 ? 12 : hours > 12 ? hours - 12 : hours;
+ const hoursStr = displayHours.toString(); // no zero-pad in 12h mode
+ const timeStr = `${hoursStr}:${minutes.toString().padStart(2, '0')}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (format.includes('a')) { | |
| const period = hours >= 12 ? 'PM' : 'AM'; | |
| const displayHours = hours === 0 ? 12 : hours > 12 ? hours - 12 : hours; | |
| const timeStr = `${displayHours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}`; | |
| const secondsStr = format.includes('ss') ? `:${seconds.toString().padStart(2, '0')}` : ''; | |
| return `${timeStr}${secondsStr} ${period}`; | |
| } else { | |
| if (format.includes('a')) { | |
| const period = hours >= 12 ? 'PM' : 'AM'; | |
| const displayHours = hours === 0 ? 12 : hours > 12 ? hours - 12 : hours; | |
| const hoursStr = displayHours.toString(); // no zero-pad in 12h mode | |
| const timeStr = `${hoursStr}:${minutes.toString().padStart(2, '0')}`; | |
| const secondsStr = format.includes('ss') ? `:${seconds.toString().padStart(2, '0')}` : ''; | |
| return `${timeStr}${secondsStr} ${period}`; | |
| } else { |
🤖 Prompt for AI Agents
In src/layout/TimePicker/TimePickerComponent.tsx around lines 74 to 80, the
12‑hour branch pads the hour to 2 digits (producing "09:05 AM") but tests and
formatTimeValue expect no leading zero for 12‑hour hours; update the formatting
so displayHours is converted to string without padStart (leave minutes and
seconds padding intact), i.e. use displayHours.toString() instead of
displayHours.toString().padStart(2, '0') when building timeStr so output becomes
"9:05 AM".
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/app-components/TimePicker/tests/dropdownBehavior.test.ts (8)
1-7: Imports look good; consider preemptively importing remaining utils when you add tests for them.If you expand coverage to
getPageJumpIndex,getHomeIndex,getEndIndex, orshouldScrollToOption, add them here to keep imports centralized.
10-23: Solid coverage for roundToStep; add non-finite step edge cases.You already cover zero/negative steps. Add NaN/Infinity cases to lock in the guard on non-finite steps.
Apply this diff within the same
describe('roundToStep', ...)block:it('should handle gracefully with invalid step', () => { expect(roundToStep(7, 0)).toBe(7); // Invalid step, return value expect(roundToStep(7, -1)).toBe(7); // Invalid step, return value }); + + it('should return original value for non-finite steps (NaN/Infinity)', () => { + expect(roundToStep(7, Number.NaN)).toBe(7); + expect(roundToStep(7, Number.POSITIVE_INFINITY)).toBe(7); + });
25-53: Add tests for system-time fallback and step rounding in getInitialHighlightIndex.The branch that uses
systemTime+segmentType+stepisn’t exercised. Add hours/minutes fallback tests (including minute rounding), and verify thatperiodreturns 0 when using system time.Apply this diff inside the same
describe('getInitialHighlightIndex', ...)block:it('should return 0 when no match found', () => { expect(getInitialHighlightIndex(99, hourOptions)).toBe(0); }); + + it('should use system time (hours) when no current value', () => { + const systemTime = new Date(2020, 0, 1, 13, 17); // 13:17 + // Note: step is required by the util even for hours; use 1 + expect(getInitialHighlightIndex(null, hourOptions, 'hours', 1, systemTime)).toBe(13); + }); + + it('should use rounded system minutes when no current value', () => { + const systemTime = new Date(2020, 0, 1, 13, 17); // 17 -> rounds to 15 with step 5 + // minuteOptions uses 5-minute steps; 15 is at index 3 + expect(getInitialHighlightIndex(null, minuteOptions, 'minutes', 5, systemTime)).toBe(3); + }); + + it('should return 0 for period segment when using system time', () => { + const periodOptions = [ + { value: 'AM', label: 'AM' }, + { value: 'PM', label: 'PM' }, + ]; + expect(getInitialHighlightIndex(null, periodOptions, 'period', 1, new Date())).toBe(0); + }); + + it('should fall back to 0 if rounded system minute is not in options', () => { + const systemTime = new Date(2020, 0, 1, 13, 17); // 17 -> rounds to 21 with step 7, not present + expect(getInitialHighlightIndex(null, minuteOptions, 'minutes', 7, systemTime)).toBe(0); + });
55-62: Good bounds checks; add a single-option edge case test.One-item lists are a common boundary; asserting both directions stay at 0 will make the intent explicit.
Apply this diff within the same
describe('getNextIndex', ...)block:it('should move up and down correctly', () => { expect(getNextIndex(5, 'up', 10)).toBe(4); expect(getNextIndex(5, 'down', 10)).toBe(6); expect(getNextIndex(0, 'up', 10)).toBe(0); // Can't go below 0 expect(getNextIndex(9, 'down', 10)).toBe(9); // Can't go above max }); + + it('should clamp within bounds for a single option', () => { + expect(getNextIndex(0, 'up', 1)).toBe(0); + expect(getNextIndex(0, 'down', 1)).toBe(0); + });
64-90: Nearest-index tests are solid; consider empty/string-miss and tie behavior.
- Empty options should return 0.
- For string values with no match, util returns 0.
- Tie distance should prefer the lower index (current implementation uses strict
<).Apply this diff inside the same
describe('findNearestOptionIndex', ...)block:it('should handle string values', () => { const periodOptions = [ { value: 'AM', label: 'AM' }, { value: 'PM', label: 'PM' }, ]; expect(findNearestOptionIndex('PM', periodOptions)).toBe(1); }); + + it('should return 0 for empty options', () => { + expect(findNearestOptionIndex(10, [])).toBe(0); + }); + + it('should return 0 for non-matching string values', () => { + const periodOptions = [ + { value: 'AM', label: 'AM' }, + { value: 'PM', label: 'PM' }, + ]; + expect(findNearestOptionIndex('XX', periodOptions)).toBe(0); + }); + + it('should prefer the lower index on equal distance (tie)', () => { + const evenOptions = [ + { value: 0, label: '00' }, // index 0, diff 5 + { value: 10, label: '10' }, // index 1, diff 5 + ]; + expect(findNearestOptionIndex(5, evenOptions)).toBe(0); + });
92-102: Scroll centering cases look good; add equal container/item height edge case.This validates the centering arithmetic when
containerHeight === itemHeight.Apply this diff within the same
describe('calculateScrollPosition', ...)block:it('should not scroll negative', () => { expect(calculateScrollPosition(1, 400, 40)).toBe(0); }); + + it('should center correctly when container height equals item height', () => { + expect(calculateScrollPosition(1, 40, 40)).toBe(40); + });
9-103: Overall: clear, focused util tests; component provider not needed here.This file tests pure utility functions—no need for
renderWithProvidersper our test guideline, so you’re aligned. Nice coverage of typical and boundary cases; the added edge cases above will round it out.
1-103: Add unit tests for all exported dropdownBehavior helpersOur verification shows that several helpers in
src/app-components/TimePicker/utils/dropdownBehavior.tsare exported but lack test coverage:
getPageJumpIndex(export at line 67)getHomeIndex(export at line 86)getEndIndex(export at line 91)shouldScrollToOption(export at line 142)Please add corresponding unit tests—either in
dropdownBehavior.test.tsor in a dedicated test file—to ensure these functions behave as intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/app-components/TimePicker/tests/dropdownBehavior.test.ts(1 hunks)src/app-components/TimePicker/tests/keyboardNavigation.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app-components/TimePicker/tests/keyboardNavigation.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/tests/dropdownBehavior.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/app-components/TimePicker/tests/dropdownBehavior.test.ts
🧬 Code graph analysis (1)
src/app-components/TimePicker/tests/dropdownBehavior.test.ts (1)
src/app-components/TimePicker/utils/dropdownBehavior.ts (5)
roundToStep(11-16)getInitialHighlightIndex(21-51)getNextIndex(56-62)findNearestOptionIndex(96-125)calculateScrollPosition(130-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app-components/TimePicker/components/TimePicker.tsx (2)
250-256: Remove redundant initialization ofnextIndex.Initial value is always overwritten on the following branch; simplify.
- let nextIndex = currentIndex; - - if (direction === 'right') { - nextIndex = (currentIndex + 1) % segments.length; - } else { - nextIndex = (currentIndex - 1 + segments.length) % segments.length; - } + const nextIndex = + direction === 'right' + ? (currentIndex + 1) % segments.length + : (currentIndex - 1 + segments.length) % segments.length;
383-396: Hour disabling logic compares 24h candidates to a 12h valid set; options get wrongly disabled.
getSegmentConstraints('hours', ...)returns 1–12 in 12‑hour formats, but the code derives a 24hactualHourbeforeincludes(...). Compare using the option’s native 12h value. Also, reuseisOptionDisabledin the render to avoid duplicated logic.Apply these diffs:
case 0: { - // Hours - const hourValue = typeof optionValue === 'number' ? optionValue : parseInt(optionValue.toString(), 10); - let actualHour = hourValue; - if (is12Hour) { - if (timeValue.period === 'AM' && hourValue === 12) { - actualHour = 0; - } else if (timeValue.period === 'PM' && hourValue !== 12) { - actualHour = hourValue + 12; - } - } - return !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes(actualHour); + // Hours — compare in the segment's own domain (1–12 for 12h, 0–23 for 24h) + const hourValue = typeof optionValue === 'number' ? optionValue : parseInt(optionValue.toString(), 10); + const hoursValid = getSegmentConstraints('hours', timeValue, constraints, format).validValues; + return !hoursValid.includes(hourValue); }- const isDisabled = - constraints.minTime || constraints.maxTime - ? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes( - is12Hour - ? option.value === 12 - ? timeValue.period === 'AM' - ? 0 - : 12 - : timeValue.period === 'PM' && option.value !== 12 - ? option.value + 12 - : option.value - : option.value, - ) - : false; + const isDisabled = isOptionDisabled(0, option.value);Follow‑up: For ranges spanning AM→PM (e.g., 11:30 AM–1:15 PM), the current constraints builder returns a non-wrapping 12h window that can be empty or misleading. Consider a helper that evaluates a 24h candidate against
minTime/maxTimeinclusively to capture edge minutes/seconds across period boundaries. I can draft this if you want it in this PR.Also applies to: 650-664
🧹 Nitpick comments (6)
src/app-components/TimePicker/components/TimePicker.tsx (6)
8-8: Deduplicate time parsing: import the sharedparseTimeStringutil instead of reimplementing locally.Prevents logic drift and keeps all parsing rules in one place.
Apply these diffs:
- import { getSegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; + import { getSegmentConstraints, parseTimeString } from 'src/app-components/TimePicker/utils/timeConstraintUtils';-const parseTimeString = (timeStr: string, format: TimeFormat): TimeValue => { - const defaultValue: TimeValue = { hours: 0, minutes: 0, seconds: 0, period: 'AM' }; - - if (!timeStr) { - return defaultValue; - } - - const is12Hour = format.includes('a'); - const includesSeconds = format.includes('ss'); - - const parts = timeStr.replace(/\s*(AM|PM)/i, '').split(':'); - const periodMatch = timeStr.match(/(AM|PM)/i); - - const hours = parseInt(parts[0] || '0', 10); - const minutes = parseInt(parts[1] || '0', 10); - const seconds = includesSeconds ? parseInt(parts[2] || '0', 10) : 0; - const period = periodMatch ? (periodMatch[1].toUpperCase() as 'AM' | 'PM') : 'AM'; - - let actualHours = isNaN(hours) ? 0 : hours; - - if (is12Hour && !isNaN(hours)) { - // Parse 12-hour format properly - if (period === 'AM' && actualHours === 12) { - actualHours = 0; - } else if (period === 'PM' && actualHours !== 12) { - actualHours += 12; - } - } - - return { - hours: actualHours, - minutes: isNaN(minutes) ? 0 : minutes, - seconds: isNaN(seconds) ? 0 : seconds, - period: is12Hour ? period : 'AM', - }; -}; +// Use shared parseTimeString from utilsAlso applies to: 32-68
565-573: Period change can yield out‑of‑range minutes/seconds; clamp to nearest valid per constraints.Switching AM/PM adjusts hours but doesn’t revalidate minutes/seconds, so the resulting time can fall outside the allowed window.
Apply this diff to clamp minutes/seconds after changing the period:
const handleDropdownPeriodChange = (period: 'AM' | 'PM') => { let newHours = timeValue.hours; if (period === 'PM' && timeValue.hours < 12) { newHours += 12; } else if (period === 'AM' && timeValue.hours >= 12) { newHours -= 12; } - updateTime({ period, hours: newHours }); + const base = { ...timeValue, hours: newHours }; + const minuteValid = getSegmentConstraints('minutes', base, constraints, format).validValues; + const nextMinutes = minuteValid.length && minuteValid.includes(base.minutes) ? base.minutes : (minuteValid[0] ?? 0); + const secondValid = getSegmentConstraints('seconds', { ...base, minutes: nextMinutes }, constraints, format).validValues; + const nextSeconds = secondValid.length && secondValid.includes(base.seconds) ? base.seconds : (secondValid[0] ?? 0); + updateTime({ period, hours: newHours, minutes: nextMinutes, seconds: nextSeconds }); };
24-30: Expose/localize trigger button label via props.Avoid hardcoded English and let integrators localize it.
export interface TimePickerProps { id: string; value: string; onChange: (time: string) => void; format?: TimeFormat; minTime?: string; maxTime?: string; disabled?: boolean; readOnly?: boolean; labels?: { hours?: string; minutes?: string; seconds?: string; amPm?: string; + openPicker?: string; }; }- aria-label='Open time picker' + aria-label={labels.openPicker || 'Open time picker'}Also applies to: 622-623
645-646: Use the same labels for dropdown column headers to avoid mixed locales.Currently “Timer/Minutter/Sekunder” are hardcoded (Norwegian) while segment labels default to English. Reuse
segmentLabels.- <div className={styles.dropdownLabel}>Timer</div> + <div className={styles.dropdownLabel}>{segmentLabels.hours}</div>- <div className={styles.dropdownLabel}>Minutter</div> + <div className={styles.dropdownLabel}>{segmentLabels.minutes}</div>- <div className={styles.dropdownLabel}>Sekunder</div> + <div className={styles.dropdownLabel}>{segmentLabels.seconds}</div>- <div className={styles.dropdownLabel}>AM/PM</div> + <div className={styles.dropdownLabel}>{segmentLabels.period}</div>Also applies to: 691-692, 730-731, 770-771
535-541: Micro-optimization: memoize options arrays to avoid reallocation on every render.Low impact but easy win when the component re-renders frequently.
Example:
const hourOptions = useMemo( () => (is12Hour ? Array.from({ length: 12 }, (_, i) => ({ value: i + 1, label: (i + 1).toString().padStart(2, '0') })) : Array.from({ length: 24 }, (_, i) => ({ value: i, label: i.toString().padStart(2, '0') }))), [is12Hour], ); const minuteOptions = useMemo(() => Array.from({ length: 60 }, (_, i) => ({ value: i, label: i.toString().padStart(2, '0') })), []); const secondOptions = useMemo(() => Array.from({ length: 60 }, (_, i) => ({ value: i, label: i.toString().padStart(2, '0') })), []);
642-799: Consider disabling AM/PM options when the entire period is invalid under constraints.Currently AM/PM is never disabled, so users can select a period that immediately forces hour/minute corrections. For better UX, compute if any hour within that period is valid and disable the period if none is.
I can help implement an
isPeriodDisabled(period)using the constraints util to probe for any valid hour/minute/second in that period and wire it into the AM/PM buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/app-components/TimePicker/components/TimePicker.module.css(1 hunks)src/app-components/TimePicker/components/TimePicker.tsx(1 hunks)src/layout/TimePicker/TimePickerComponent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/layout/TimePicker/TimePickerComponent.tsx
- src/app-components/TimePicker/components/TimePicker.module.css
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/components/TimePicker.tsx
🧬 Code graph analysis (1)
src/app-components/TimePicker/components/TimePicker.tsx (4)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (4)
parseTimeString(21-56)TimeValue(3-8)TimeConstraints(10-13)getSegmentConstraints(82-189)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatTimeValue(5-26)src/app-components/TimePicker/components/TimeSegment.tsx (1)
TimeSegment(38-300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app-components/TimePicker/README.md (1)
33-33: Fix import path and 12-hour format token in the usage example
- The import path likely points to a non-existent module; the component file appears to live under
components/TimePicker.tsxor is re-exported elsewhere.- The 12-hour example uses the 24-hour token (
HH) together witha(AM/PM). For 12-hour formats, usehh.Run this script to verify the actual export location before applying the change:
#!/bin/bash # Locate TimePicker source and any re-exports fd -t f -e ts -e tsx TimePicker rg -nP --type ts --type tsx -C2 'export\s+\{?\s*TimePicker\s*\}?'Apply this diff to the README:
-import { TimePicker } from 'src/app-components/TimePicker/TimePicker'; +import { TimePicker } from 'src/app-components/TimePicker/components/TimePicker'; @@ - format="HH:mm:ss a" + format="hh:mm:ss a"Also applies to: 44-51
🧹 Nitpick comments (9)
src/app-components/TimePicker/README.md (9)
25-30: Document supported format tokens where “Format Support” is introducedAdding an explicit token reference reduces confusion and prevents misuse (e.g.,
HHvshh).## Format Support @@ -- **24-hour format**: "HH:mm" or "HH:mm:ss" -- **12-hour format**: "hh:mm a" or "hh:mm:ss a" (with AM/PM) -- **Flexible display**: Configurable time format with optional seconds +- **24-hour format**: "HH:mm" or "HH:mm:ss" +- **12-hour format**: "hh:mm a" or "hh:mm:ss a" (with AM/PM) +- **Flexible display**: Configurable time format with optional seconds + +### Token reference +- `HH` — hours 00–23 (24-hour) +- `hh` — hours 01–12 (12-hour) +- `mm` — minutes 00–59 +- `ss` — seconds 00–59 +- `a` — period marker (AM/PM) + +Note: When using `a`, pair it with `hh` (not `HH`).
57-65: Clarify prop semantics for value/onChange/formatMake it explicit that
onChangeemits a string in the activeformat, and whatvalueshould look like for each mode.-`onChange: (value: string) => void` - Callback when time value changes +`onChange: (value: string) => void` - Called with the time string formatted according to `format` @@ -`value?: string` - Current time value in the specified format -`format?: TimeFormat` - Time format string (default: "HH:mm") +`value?: string` - Current time value as a string matching `format` (e.g., "14:30" for "HH:mm", "02:30 PM" for "hh:mm a") +`format?: TimeFormat` - Time format string controlling display and emitted value (default: "HH:mm")
20-24: Punctuation/clarity: add serial comma in separators listSmall readability tweak.
-- **Separators**: Type ":", ".", "," or space to advance to next segment +- **Separators**: Type ":", ".", ",", or space to advance to the next segment
16-16: Grammar: “the next” improves flow-- **Auto-advance**: Automatically moves to next segment +- **Auto-advance**: Automatically moves to the next segment
120-125: Hyphenation and consistency in Period InputMinor readability fixes.
-- **Case insensitive**: Accepts both upper and lower case +- **Case-insensitive**: Accepts both upper and lower case
137-141: ARIA phrasing: add article and code formatting-- **ARIA Labels**: Each segment has descriptive aria-label +- **ARIA Labels**: Each segment has a descriptive `aria-label`
110-113: Terminology: replace “0X” placeholder with “0x”Lowercase “x” is the conventional placeholder for “some digit”.
-- **24-hour mode**: First digit 0-2 waits for second digit, 3-9 auto-coerces to 0X -... -- **12-hour mode**: First digit 0-1 waits for second digit, 2-9 auto-coerces to 0X +-- **24-hour mode**: First digit 0-2 waits for second digit, 3-9 auto-coerces to 0x +... +-- **12-hour mode**: First digit 0-1 waits for second digit, 2-9 auto-coerces to 0x @@ -- **First digit**: 0-5 waits for second digit, 6-9 auto-coerces to 0X +- **First digit**: 0-5 waits for second digit, 6-9 auto-coerces to 0xAlso applies to: 116-119
131-134: Avoid “race condition” wording in buffer section“Race condition” is overloaded; “stale closure” is more precise for React event timing.
-3. **Race Condition Prevention**: Uses refs to avoid stale closure issues +3. **Stale Closure Prevention**: Uses refs to avoid stale-closure issues
30-51: Optional: add a quick controlled vs. uncontrolled note under UsageThis helps integrators understand how
valueandonChangeinterplay.## Usage @@ ```tsx @@ </>
+Note: The component is controlled. When
valueis provided, the displayed time mirrors that value; user edits are surfaced viaonChange(value), and it’s the parent’s responsibility to updatevalue. Ifvalueis omitted, the component manages its internal state but still callsonChangewith the formatted string on edits.</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **💡 Knowledge Base configuration:** - MCP integration is disabled by default for public repositories - Jira integration is disabled by default for public repositories - Linear integration is disabled by default for public repositories You can enable these sources in your CodeRabbit configuration. <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ecceadb82cdf67807e482ca634db4d814f2fcaa6 and 16526575eb4fd512b41e83399240579db42a8e0b. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/app-components/TimePicker/README.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>src/app-components/TimePicker/README.md</summary> [grammar] ~16-~16: There might be a mistake here. Context: ...- **Auto-advance**: Automatically moves to next segment when current segment is co... (QB_NEW_EN) --- [grammar] ~16-~16: There might be a mistake here. Context: ...**: Automatically moves to next segment when current segment is complete ### Keyboa... (QB_NEW_EN) --- [grammar] ~23-~23: There might be a mistake here. Context: ... Type ":", ".", "," or space to advance to next segment ### Format Support - **2... (QB_NEW_EN) --- [grammar] ~27-~27: There might be a mistake here. Context: ...*24-hour format**: "HH:mm" or "HH:mm:ss" - **12-hour format**: "hh:mm a" or "hh:mm:ss... (QB_NEW_EN) --- [grammar] ~28-~28: There might be a mistake here. Context: ...: "hh:mm a" or "hh:mm:ss a" (with AM/PM) - **Flexible display**: Configurable time fo... (QB_NEW_EN) --- [grammar] ~29-~29: There might be a mistake here. Context: ...urable time format with optional seconds ## Usage ```tsx import { TimePicker } from... (QB_NEW_EN) --- [grammar] ~82-~82: There might be a mistake here. Context: ...t for hours, minutes, seconds, or period - Implements Chrome-like typing behavior w... (QB_NEW_EN) --- [grammar] ~83-~83: There might be a mistake here. Context: ...e typing behavior with buffer management - Handles keyboard navigation and value co... (QB_NEW_EN) --- [grammar] ~90-~90: There might be a mistake here. Context: ...ercion logic for different segment types - **Buffer Management**: Handles multi-chara... (QB_NEW_EN) --- [grammar] ~91-~91: There might be a mistake here. Context: ...dles multi-character input with timeouts - **Validation**: Ensures values stay within... (QB_NEW_EN) --- [grammar] ~96-~96: There might be a mistake here. Context: ...*: Arrow key navigation between segments - **Value Manipulation**: Increment/decremen... (QB_NEW_EN) --- [grammar] ~97-~97: There might be a mistake here. Context: ...n**: Increment/decrement with arrow keys - **Key Handling**: Special key processing (... (QB_NEW_EN) --- [grammar] ~110-~110: There might be a mistake here. Context: ...for second digit, 3-9 auto-coerces to 0X - **12-hour mode**: First digit 0-1 waits fo... (QB_NEW_EN) --- [grammar] ~111-~111: There might be a mistake here. Context: ...for second digit, 2-9 auto-coerces to 0X - **Second digit**: Validates against first ... (QB_NEW_EN) --- [grammar] ~116-~116: There might be a mistake here. Context: ...ond Input - **First digit**: 0-5 waits for second digit, 6-9 auto-coerces to 0X - ... (QB_NEW_EN) --- [grammar] ~116-~116: There might be a mistake here. Context: ...for second digit, 6-9 auto-coerces to 0X - **Second digit**: Always accepts 0-9 - **O... (QB_NEW_EN) --- [grammar] ~122-~122: There might be a mistake here. Context: ...Input (AM/PM) - **A/a key**: Sets to AM - **P/p key**: Sets to PM - **Case insensiti... (QB_NEW_EN) --- [grammar] ~123-~123: There might be a mistake here. Context: ...**: Sets to AM - **P/p key**: Sets to PM - **Case insensitive**: Accepts both upper a... (QB_NEW_EN) --- [grammar] ~137-~137: There might be a mistake here. Context: ...bility - **ARIA Labels**: Each segment has descriptive aria-label - **Keyboard Nav... (QB_NEW_EN) --- [grammar] ~146-~146: There might be a mistake here. Context: ...*: Various input patterns and edge cases - **Navigation**: Keyboard navigation betwee... (QB_NEW_EN) --- [grammar] ~147-~147: There might be a mistake here. Context: ...**: Keyboard navigation between segments - **Buffer Management**: Race condition prev... (QB_NEW_EN) --- [grammar] ~148-~148: There might be a mistake here. Context: ...ondition prevention and timeout handling - **Format Support**: Different time formats... (QB_NEW_EN) --- [grammar] ~149-~149: There might be a mistake here. Context: ...*: Different time formats and validation - **Accessibility**: Screen reader compatibi... (QB_NEW_EN) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)</summary> * GitHub Check: Type-checks, eslint, unit tests and SonarCloud * GitHub Check: Analyze (javascript) * GitHub Check: Install </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
/publish |
PR release:
|
adamhaeger
force-pushed
the
feat/1261-timepicker
branch
from
1652657 to
32d8585
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/app-components/TimePicker/components/TimeSegment.tsx (1)
111-120: Focus detection is incorrect; fresh-focus branch never runs.
event.currentTarget !== document.activeElementis always false inonFocus. Usee.relatedTargetto detect transitions.- const handleFocusEvent = (event: React.FocusEvent<HTMLInputElement>) => { - const wasFreshFocus = event.currentTarget !== document.activeElement; - - if (wasFreshFocus) { + const handleFocusEvent = (event: React.FocusEvent<HTMLInputElement>) => { + // Fresh focus when coming from another element (not re-focusing self) + const cameFromSelf = event.relatedTarget === event.currentTarget; + if (!cameFromSelf) { typingBuffer.resetToIdleState(); event.target.select(); } onFocus?.(); };src/app-components/TimePicker/hooks/useSegmentInputHandlers.ts (1)
107-114: Type mismatch:currentValueisn’t nullable but code checks fornull.
currentValueisnumber | string, yet you checkcurrentValue === null. Remove the null check or widen the type. Simplest:- const fillEmptyMinutesOrSecondsWithZero = useCallback(() => { - const valueIsEmpty = - currentValue === null || currentValue === '' || (typeof currentValue === 'number' && isNaN(currentValue)); + const fillEmptyMinutesOrSecondsWithZero = useCallback(() => { + const valueIsEmpty = + currentValue === '' || (typeof currentValue === 'number' && isNaN(currentValue));
🧹 Nitpick comments (8)
src/app-components/TimePicker/hooks/useSegmentDisplay.ts (2)
19-21: Avoid visual flicker on external value changes.When
externalValue/segmentType/timeFormatchange, updating after paint can cause a brief mismatch. UsinguseLayoutEffecthere prevents flicker in controlled inputs.- useEffect(() => { + useLayoutEffect(() => { syncWithExternalValue(); }, [syncWithExternalValue]);
7-17: Consider narrowing types per segment to improve safety.
externalValue: number | stringallows any segment to receive any type. If feasible, expose a discriminated signature or overloads so that:
- numeric for hours/minutes/seconds
- 'AM' | 'PM' for period
This prevents accidental cross-type propagation at call sites.
src/app-components/TimePicker/hooks/useTypingBuffer.ts (2)
32-41: Reduce unnecessary re-creations by stabilizing timer deps.With the updated
useTimeoutreturning a memoized object, you can depend only on its stable methods to avoid accidental re-renders. Not mandatory, but clearer:- const clearAllTimers = useCallback(() => { - commitTimer.clear(); - typingEndTimer.clear(); - }, [commitTimer, typingEndTimer]); + const clearAllTimers = useCallback(() => { + commitTimer.clear(); + typingEndTimer.clear(); + }, [commitTimer.clear, typingEndTimer.clear]); - const restartTimers = useCallback(() => { + const restartTimers = useCallback(() => { clearAllTimers(); commitTimer.start(); typingEndTimer.start(); - }, [clearAllTimers, commitTimer, typingEndTimer]); + }, [clearAllTimers, commitTimer.start, typingEndTimer.start]);
57-67: Commit ordering guarantees are fine; add a tiny guard.If
commitImmediatelyAndEndTypingruns with an empty buffer,onCommitis skipped (by your guard) but timers still get cleared. Good. Optional: early-return to avoid a state write when already idle:const commitImmediatelyAndEndTyping = useCallback(() => { - commitBufferWithoutEndingTyping(); - setIsTyping(false); - clearAllTimers(); + commitBufferWithoutEndingTyping(); + if (isTyping) { + setIsTyping(false); + } + clearAllTimers(); - }, [commitBufferWithoutEndingTyping, clearAllTimers]); + }, [commitBufferWithoutEndingTyping, clearAllTimers, isTyping]);src/app-components/TimePicker/components/TimeSegment.tsx (3)
67-75: Effect re-runs unnecessarily because it depends on the entire buffer object.Depending on
typingBuffer(object) causes re-runs every render. Narrow totypingBuffer.isTyping:- React.useEffect(syncExternalChangesWhenNotTyping, [value, type, format, typingBuffer, syncWithExternalValue]); + React.useEffect(() => { + if (!typingBuffer.isTyping) { + syncWithExternalValue(); + typingBuffer.resetToIdleState(); + } + }, [value, type, format, typingBuffer.isTyping, typingBuffer.resetToIdleState, syncWithExternalValue]);
11-28: Props define min/max but they’re unused.
min/maxare declared yet not enforced for typing or arrow increments/decrements. Either use them (e.g., pass constraints into input handlers) or drop them to avoid confusion.Follow-up path:
- Extend
useSegmentInputHandlersto accept optional constraints and thread into keyboard utilities.- Clamp committed values in
commitBufferValueandhandleValueIncrement/Decrement.I can draft the plumbing if you prefer.
146-153:maxLengthis redundant (always 2); consider deriving from segment type.You already branch on
type === 'period', but both cases yield2. Either remove or keep solely forperiodif you foresee longer tokens. Minor.src/app-components/TimePicker/hooks/useSegmentInputHandlers.ts (1)
52-66: Commit on empty buffer never fires; consider defaulting minutes/seconds to 0.
commitBufferValueno-ops whenprocessed.actualValue === null. For parity with blur behavior, you may wish to commit 0 for minutes/seconds when committing an empty buffer (optional, but improves consistency).- } else { + } else { const processed = processSegmentBuffer(bufferValue, segmentType, timeFormat.includes('a')); - if (processed.actualValue !== null) { - const committedValue = commitSegmentValue(processed.actualValue, segmentType); - onValueChange(committedValue); - } + const actual = processed.actualValue ?? (segmentType === 'minutes' || segmentType === 'seconds' ? 0 : null); + if (actual !== null) { + onValueChange(commitSegmentValue(actual, segmentType)); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/app-components/TimePicker/components/TimeSegment.tsx(1 hunks)src/app-components/TimePicker/hooks/useSegmentDisplay.ts(1 hunks)src/app-components/TimePicker/hooks/useSegmentInputHandlers.ts(1 hunks)src/app-components/TimePicker/hooks/useTimeout.ts(1 hunks)src/app-components/TimePicker/hooks/useTypingBuffer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/hooks/useTypingBuffer.tssrc/app-components/TimePicker/hooks/useTimeout.tssrc/app-components/TimePicker/hooks/useSegmentDisplay.tssrc/app-components/TimePicker/hooks/useSegmentInputHandlers.tssrc/app-components/TimePicker/components/TimeSegment.tsx
🧬 Code graph analysis (4)
src/app-components/TimePicker/hooks/useTypingBuffer.ts (1)
src/app-components/TimePicker/hooks/useTimeout.ts (1)
useTimeout(3-19)
src/app-components/TimePicker/hooks/useSegmentDisplay.ts (3)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatSegmentValue(28-49)
src/app-components/TimePicker/hooks/useSegmentInputHandlers.ts (3)
src/app-components/TimePicker/utils/keyboardNavigation.ts (4)
SegmentType(4-4)handleValueIncrement(74-110)handleValueDecrement(112-148)handleSegmentKeyDown(14-58)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/utils/segmentTyping.ts (4)
handleSegmentCharacterInput(231-285)processSegmentBuffer(120-153)commitSegmentValue(166-174)clearSegment(158-161)
src/app-components/TimePicker/components/TimeSegment.tsx (5)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(13-13)src/app-components/TimePicker/hooks/useSegmentDisplay.ts (1)
useSegmentDisplay(7-28)src/app-components/TimePicker/hooks/useSegmentInputHandlers.ts (1)
useSegmentInputHandlers(27-123)src/app-components/TimePicker/hooks/useTypingBuffer.ts (1)
useTypingBuffer(11-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
- GitHub Check: Analyze (javascript)
| return ( | ||
| <Textfield | ||
| ref={ref} | ||
| type='text' | ||
| value={displayValue} | ||
| onChange={() => {}} | ||
| onKeyPress={handleCharacterTyping} | ||
| onKeyDown={handleSpecialKeys} | ||
| onFocus={handleFocusEvent} | ||
| onBlur={handleBlurEvent} | ||
| placeholder={placeholder} | ||
| disabled={disabled} | ||
| readOnly={readOnly} | ||
| required={required} | ||
| aria-label={ariaLabel} | ||
| className={className} | ||
| autoFocus={autoFocus} | ||
| data-size='sm' | ||
| style={{ | ||
| width: type === 'period' ? '4rem' : '3rem', | ||
| textAlign: 'center', | ||
| padding: '0.25rem', | ||
| }} | ||
| autoComplete='off' | ||
| inputMode={type === 'period' ? 'text' : 'numeric'} | ||
| maxLength={type === 'period' ? 2 : 2} | ||
| /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
onKeyPress is deprecated; handle character input via onBeforeInput or onKeyDown.
React has deprecated onKeyPress. On some platforms it won’t fire reliably. Migrate to onBeforeInput (preferred for text) or fold into onKeyDown with a printable-key check.
- onKeyPress={handleCharacterTyping}
- onKeyDown={handleSpecialKeys}
+ onBeforeInput={(e) => {
+ if (typeof e.data === 'string' && e.data.length === 1) {
+ // Mirror previous onKeyPress flow
+ // @ts-expect-error: data is string in beforeinput
+ handleCharacterTyping(e as unknown as React.KeyboardEvent<HTMLInputElement>);
+ }
+ }}
+ onKeyDown={handleSpecialKeys}If sticking to onKeyDown, gate printable chars with /^.$/u and !event.ctrlKey && !event.metaKey && !event.altKey.
Run to find any remaining onKeyPress handlers:
🏁 Script executed:
#!/bin/bash
rg -n --type=ts --type=tsx -C1 '\bonKeyPress\s*='Length of output: 79
🏁 Script executed:
#!/bin/bash
# Search for any remaining onKeyPress handlers in TypeScript and TSX files without relying on ripgrep type filters
rg -n -C1 '\bonKeyPress\s*=' -g '*.ts' -g '*.tsx'Length of output: 1529
Migrate all deprecated onKeyPress handlers to supported events
We’ve found several remaining uses of the deprecated onKeyPress prop. React no longer guarantees this event across all platforms, so we should replace every occurrence with either:
onBeforeInputfor pure text inputs (captures actual character data ine.data), oronKeyDownwith a printable-character guard (e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey).
Affected locations:
-
src/app-components/TimePicker/components/TimeSegment.tsx
Line 134:onKeyPress={handleCharacterTyping}→ migrate toonBeforeInputas shown below. -
src/components/molecules/AltinnCollapsibleAttachments.tsx
Line 38:onKeyPress={handleOpenClose}→ fold intoonKeyDownwith an “Enter”/printable-key check. -
src/components/altinnParty.tsx
Lines 108 & 142:onKeyPress={onKeyPress.bind(null, …)}→ merge intoonKeyDownand guard against non-printable keys. -
src/components/AltinnCollapsible.tsx
Line 35:onKeyPress={onKeyPress}→ likewise replace withonKeyDown.
Suggested pattern for text inputs (e.g. TimeSegment.tsx):
- onKeyPress={handleCharacterTyping}
+ onBeforeInput={(e) => {
+ if (typeof e.data === 'string' && e.data.length === 1) {
+ // Mirror previous onKeyPress flow
+ // @ts-expect-error: casting BeforeInputEvent to KeyboardEvent for handler signature
+ handleCharacterTyping(e as unknown as React.KeyboardEvent<HTMLInputElement>);
+ }
+ }}Suggested pattern for actionable components (buttons/list-items):
- onKeyPress={handleAction}
+ onKeyDown={(e) => {
+ // Trigger on “Enter” or single printable character
+ const isPrintable = e.key.length === 1 && !e.ctrlKey && !e.metaKey && !e.altKey;
+ if (e.key === 'Enter' || isPrintable) {
+ handleAction(e);
+ }
+ }}• Please update each of the above files to remove onKeyPress and apply one of these patterns.
• Run a final grep to confirm no onKeyPress= remains:
rg -n '\bonKeyPress\s*=' -g '*.ts' -g '*.tsx'🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimeSegment.tsx around lines
128-154, replace the deprecated onKeyPress={handleCharacterTyping} with
onBeforeInput and wire it to the same logic (use e.data for the input character)
— update handleCharacterTyping's handler signature if needed to accept a
BeforeInputEvent and read printable characters from e.data, keep existing
onKeyDown for special keys, and remove the onKeyPress prop entirely; after
updating, run the provided rg command to confirm no onKeyPress usages remain.
| import { useCallback, useRef } from 'react'; | ||
|
|
||
| export function useTimeout(callback: () => void, delayMs: number) { | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const clear = useCallback(() => { | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| timeoutRef.current = null; | ||
| } | ||
| }, []); | ||
|
|
||
| const start = useCallback(() => { | ||
| clear(); | ||
| timeoutRef.current = setTimeout(callback, delayMs); | ||
| }, [callback, clear, delayMs]); | ||
|
|
||
| return { start, clear }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent timer leaks and stale-callback bugs; also stabilize returned API.
- Current hook doesn’t clear pending timeouts on unmount.
- The scheduled callback can become stale if
callbackchanges afterstart()is called. - Returning a new
{ start, clear }object every render forces downstream deps to churn.
Apply this diff to address all three:
-import { useCallback, useRef } from 'react';
+import { useCallback, useEffect, useMemo, useRef } from 'react';
export function useTimeout(callback: () => void, delayMs: number) {
const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const callbackRef = useRef(callback);
+ useEffect(() => {
+ callbackRef.current = callback;
+ }, [callback]);
const clear = useCallback(() => {
- if (timeoutRef.current) {
+ if (timeoutRef.current !== null) {
clearTimeout(timeoutRef.current);
timeoutRef.current = null;
}
}, []);
const start = useCallback(() => {
clear();
- timeoutRef.current = setTimeout(callback, delayMs);
- }, [callback, clear, delayMs]);
+ timeoutRef.current = setTimeout(() => callbackRef.current(), delayMs);
+ }, [clear, delayMs]);
- return { start, clear };
+ useEffect(() => () => clear(), [clear]);
+
+ return useMemo(() => ({ start, clear }), [start, clear]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { useCallback, useRef } from 'react'; | |
| export function useTimeout(callback: () => void, delayMs: number) { | |
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const clear = useCallback(() => { | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| timeoutRef.current = null; | |
| } | |
| }, []); | |
| const start = useCallback(() => { | |
| clear(); | |
| timeoutRef.current = setTimeout(callback, delayMs); | |
| }, [callback, clear, delayMs]); | |
| return { start, clear }; | |
| } | |
| import { useCallback, useEffect, useMemo, useRef } from 'react'; | |
| export function useTimeout(callback: () => void, delayMs: number) { | |
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const callbackRef = useRef(callback); | |
| useEffect(() => { | |
| callbackRef.current = callback; | |
| }, [callback]); | |
| const clear = useCallback(() => { | |
| if (timeoutRef.current !== null) { | |
| clearTimeout(timeoutRef.current); | |
| timeoutRef.current = null; | |
| } | |
| }, []); | |
| const start = useCallback(() => { | |
| clear(); | |
| timeoutRef.current = setTimeout(() => callbackRef.current(), delayMs); | |
| }, [clear, delayMs]); | |
| // Clear any pending timeout on unmount | |
| useEffect(() => () => clear(), [clear]); | |
| // Return a stable API object | |
| return useMemo(() => ({ start, clear }), [start, clear]); | |
| } |
| return false; | ||
| }; | ||
|
|
||
| const parseTimeString = (timeStr: string, format: TimeFormat): TimeValue => { |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
The best way to fix the unused function issue is to remove the entire definition of parseTimeString, from its declaration to its closing brace. This means deleting lines 119 through 154 in src/app-components/TimePicker/utils/timeFormatUtils.ts. No further changes are necessary, as the function is not referenced, exported, or required elsewhere in the visible code.
| @@ -116,39 +116,4 @@ | ||
| return false; | ||
| }; | ||
|
|
||
| const parseTimeString = (timeStr: string, format: TimeFormat): TimeValue => { | ||
| const defaultValue: TimeValue = { hours: 0, minutes: 0, seconds: 0, period: 'AM' }; | ||
|
|
||
| if (!timeStr) { | ||
| return defaultValue; | ||
| } | ||
|
|
||
| const is12Hour = format.includes('a'); | ||
| const includesSeconds = format.includes('ss'); | ||
|
|
||
| const parts = timeStr.replace(/\s*(AM|PM)/i, '').split(':'); | ||
| const periodMatch = timeStr.match(/(AM|PM)/i); | ||
|
|
||
| const hours = parseInt(parts[0] || '0', 10); | ||
| const minutes = parseInt(parts[1] || '0', 10); | ||
| const seconds = includesSeconds ? parseInt(parts[2] || '0', 10) : 0; | ||
| const period = periodMatch ? (periodMatch[1].toUpperCase() as 'AM' | 'PM') : 'AM'; | ||
|
|
||
| let actualHours = isNaN(hours) ? 0 : hours; | ||
|
|
||
| if (is12Hour && !isNaN(hours)) { | ||
| // Parse 12-hour format properly | ||
| if (period === 'AM' && actualHours === 12) { | ||
| actualHours = 0; | ||
| } else if (period === 'PM' && actualHours !== 12) { | ||
| actualHours += 12; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| hours: actualHours, | ||
| minutes: isNaN(minutes) ? 0 : minutes, | ||
| seconds: isNaN(seconds) ? 0 : seconds, | ||
| period: is12Hour ? period : 'AM', | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/app-components/TimePicker/components/TimePicker.tsx (4)
212-223: Remove redundant assignment to nextIndex.Initial assignment is overwritten. Inline the computation.
- let nextIndex = currentIndex; - - if (direction === 'right') { - nextIndex = (currentIndex + 1) % segments.length; - } else { - nextIndex = (currentIndex - 1 + segments.length) % segments.length; - } + const nextIndex = + direction === 'right' + ? (currentIndex + 1) % segments.length + : (currentIndex - 1 + segments.length) % segments.length;
613-627: Fix hours disabled logic in dropdown to use native hours domain (same issue).Use the option’s own value for includes() in 12h mode.
- const isDisabled = - constraints.minTime || constraints.maxTime - ? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes( - is12Hour - ? option.value === 12 - ? timeValue.period === 'AM' - ? 0 - : 12 - : timeValue.period === 'PM' && option.value !== 12 - ? option.value + 12 - : option.value - : option.value, - ) - : false; + const hoursValid = getSegmentConstraints('hours', timeValue, constraints, format).validValues; + const isDisabled = constraints.minTime || constraints.maxTime ? !hoursValid.includes(option.value) : false;
346-359: Hours disabling check compares 24h candidate against 12h validValues (domain mismatch).Check against the segment’s native domain (1–12 in 12h, 0–23 in 24h).
case 0: { - // Hours - const hourValue = typeof optionValue === 'number' ? optionValue : parseInt(optionValue.toString(), 10); - let actualHour = hourValue; - if (is12Hour) { - if (timeValue.period === 'AM' && hourValue === 12) { - actualHour = 0; - } else if (timeValue.period === 'PM' && hourValue !== 12) { - actualHour = hourValue + 12; - } - } - return !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes(actualHour); + // Hours (use native domain of the hours segment) + const hoursValid = getSegmentConstraints('hours', timeValue, constraints, format).validValues; + const candidate = typeof optionValue === 'number' ? optionValue : parseInt(optionValue.toString(), 10); + return !hoursValid.includes(candidate); }
155-209: 12‑hour edits write 1–12 into internal hours; breaks 24h invariant and can misapply constraints. Also unsafe reduce() on empty valid list.Store hours internally as 0–23 and guard empty lists.
const handleSegmentValueChange = (segmentType: SegmentType, newValue: number | string) => { if (segmentType === 'period') { @@ updateTime({ period, hours: newHours }); } else { // Apply constraints for numeric segments const segmentConstraints = getSegmentConstraints(segmentType, timeValue, constraints, format); - let validValue = newValue as number; + let validValue = newValue as number; @@ - // Check if value is within constraints - if (segmentConstraints.validValues.includes(validValue)) { - updateTime({ [segmentType]: validValue }); - } else { - // Find nearest valid value - const nearestValid = segmentConstraints.validValues.reduce((prev, curr) => - Math.abs(curr - validValue) < Math.abs(prev - validValue) ? curr : prev, - ); - updateTime({ [segmentType]: nearestValid }); - } + // Normalize and update within the segment's native domain; store hours internally as 24h + const isHours = segmentType === 'hours'; + const validList = segmentConstraints.validValues; + const to24h = (h: number) => { + if (!is12Hour) return h; + return timeValue.period === 'AM' ? (h === 12 ? 0 : h) : (h === 12 ? 12 : h + 12); + }; + if (validList.length === 0) { + // No permissible values for this segment under current constraints + return; + } + if (validList.includes(validValue)) { + const nextVal = isHours ? to24h(validValue) : validValue; + updateTime({ [segmentType]: nextVal }); + } else { + const nearest = validList.reduce((prev, curr) => + Math.abs(curr - validValue) < Math.abs(prev - validValue) ? curr : prev, + ); + const nextVal = isHours ? to24h(nearest) : nearest; + updateTime({ [segmentType]: nextVal }); + } } };
🧹 Nitpick comments (10)
src/features/expressions/shared-tests/functions/displayValue/type-TimePicker.json (1)
21-22: Binding matches data model; consider adding edge-case fixturesBinding
Skjema.Tidis present and consistent with the value. Consider adding complementary fixtures to harden the expression behavior:
- Empty value → expects ""
- Out-of-range input (e.g., "24:00" or "03:60") → define expected normalization or empty
- Alternate token set if supported (e.g., "HH:mm:ss" or 12h "hh:mm a")
Also applies to: 29-33
src/app-components/TimePicker/components/TimePicker.tsx (9)
146-153: Avoid stale state in updateTime; use functional setState.Prevents lost updates when multiple segment changes occur quickly and guarantees onChange gets the latest value.
- const updateTime = useCallback( - (updates: Partial<TimeValue>) => { - const newTime = { ...timeValue, ...updates }; - setTimeValue(newTime); - onChange(formatTimeValue(newTime, format)); - }, - [timeValue, onChange, format], - ); + const updateTime = useCallback( + (updates: Partial<TimeValue>) => { + setTimeValue((prev) => { + const next = { ...prev, ...updates }; + onChange(formatTimeValue(next, format)); + return next; + }); + }, + [onChange, format], + );
592-599: Be explicit with aria-modal value.Small a11y nit: set boolean explicitly for clarity.
- <Popover + <Popover ref={dropdownRef} className={styles.timePickerDropdown} - aria-modal + aria-modal={true} aria-hidden={!showDropdown} role='dialog'
606-616: Use provided labels for column header to keep localization consistent.Avoid hardcoded “Timer”.
- <div className={styles.dropdownLabel}>Timer</div> + <div className={styles.dropdownLabel}>{segmentLabels.hours}</div>
653-659: Use provided labels for column header (Minutes).- <div className={styles.dropdownLabel}>Minutter</div> + <div className={styles.dropdownLabel}>{segmentLabels.minutes}</div>
691-698: Use provided labels for column header (Seconds).- <div className={styles.dropdownLabel}>Sekunder</div> + <div className={styles.dropdownLabel}>{segmentLabels.seconds}</div>
731-739: Use provided labels for column header (AM/PM).- <div className={styles.dropdownLabel}>AM/PM</div> + <div className={styles.dropdownLabel}>{segmentLabels.period}</div>
46-47: Remove unused focusedSegment state and its setters.It’s written but never read.
- const [_focusedSegment, setFocusedSegment] = useState<number | null>(null); + // Unused focused segment state removed- segmentRefs.current[nextIndex]?.focus(); - setFocusedSegment(nextIndex); + segmentRefs.current[nextIndex]?.focus();- onFocus={() => setFocusedSegment(index)} - onBlur={() => setFocusedSegment(null)} + onFocus={undefined} + onBlur={undefined}Also applies to: 221-223, 566-567
505-518: Deduplicate 12h↔24h hour conversion helpers.You convert hours in multiple places; consider a small local helper (or reuse a shared util) to reduce duplication and mistakes.
I can draft a tiny to24h/from24h helper if you want.
Also applies to: 528-536
340-376: Sanity tests for constraint edges and AM/PM transitions recommended.Add unit tests for:
- 12 AM/PM conversion and formatting
- Min/max hour boundaries across AM/PM
- Minutes/seconds clamping when hour hits boundary
I can scaffold tests targeting getSegmentConstraints + interactions through dropdown and segments.
Also applies to: 379-409
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/app-components/TimePicker/components/TimePicker.tsx(1 hunks)src/app-components/TimePicker/utils/timeFormatUtils.ts(1 hunks)src/features/expressions/shared-tests/functions/displayValue/type-TimePicker.json(1 hunks)src/layout/TimePicker/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app-components/TimePicker/utils/timeFormatUtils.ts
- src/layout/TimePicker/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/components/TimePicker.tsx
🧬 Code graph analysis (1)
src/app-components/TimePicker/components/TimePicker.tsx (5)
src/layout/TimePicker/index.tsx (1)
TimePicker(30-136)src/app-components/TimePicker/utils/timeConstraintUtils.ts (4)
TimeValue(3-8)parseTimeString(21-56)TimeConstraints(10-13)getSegmentConstraints(82-189)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatTimeValue(5-26)src/app-components/TimePicker/components/TimeSegment.tsx (1)
TimeSegment(30-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (3)
src/features/expressions/shared-tests/functions/displayValue/type-TimePicker.json (3)
1-12: Solid fixture: validates displayValue for TimePicker with a clear expectationThe structure and expectation are correct and readable. Nice coverage of the 24h format happy-path.
14-15: Schema pointer alignment verifiedAll 101 expression fixtures in
src/features/expressions/shared-testsreference the same$schemaURL
//altinncdn.no/schemas/json/layout/layout.schema.v1.json. Consistency confirmed—no changes required.
13-24: Ignore suggestion to renameformattotimeFormatThe TimePicker schema (in
config.def.generated) defines the JSON prop as"format", and the component’s code then destructuresformatinto an internaltimeFormatvariable (src/layout/TimePicker/index.tsx:40). Therefore, JSON test fixtures and layout definitions should continue using"format", and no change to"timeFormat"is required.Likely an incorrect or invalid review comment.
|
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (1)
168-218: Add immutability test for validValues to prevent regressions.Covers the reverse()-mutation pitfall.
describe('getNextValidValue', () => { @@ it('should skip invalid values and find next valid one', () => { const constraints: SegmentConstraints = { min: 5, max: 20, validValues: [5, 8, 12, 15, 20], }; const result = getNextValidValue(5, 'up', constraints); expect(result).toBe(8); }); + + it('should not mutate constraints.validValues when navigating down', () => { + const constraints: SegmentConstraints = { + min: 0, + max: 10, + validValues: [0, 2, 4, 6, 8, 10], + }; + const original = [...constraints.validValues]; + const result = getNextValidValue(6, 'down', constraints); + expect(result).toBe(4); + expect(constraints.validValues).toStrictEqual(original); + }); });src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
204-213: Critical: reverse() mutates validValues and corrupts subsequent calls.Use a non-mutating backward scan.
if (currentIndex === -1) { // Current value is not in valid values, find nearest if (direction === 'up') { const nextValid = validValues.find((v) => v > currentValue); return nextValid ?? null; } else { - const prevValid = validValues.reverse().find((v) => v < currentValue); - return prevValid ?? null; + for (let i = validValues.length - 1; i >= 0; i--) { + if (validValues[i] < currentValue) { + return validValues[i]; + } + } + return null; } }src/app-components/TimePicker/components/TimePicker.tsx (1)
514-527: Hours constraint validation logic breaks at AM/PM boundaries.The complex 12-hour to 24-hour conversion logic for constraint validation creates incorrect hour disabling. This is a duplicate of a previously identified issue where 12-hour UI options (1-12) are being converted to 24-hour values before checking against
getSegmentConstraints('hours', ...)which returnsvalidValuesin the 1-12 domain for 12-hour formats.
🧹 Nitpick comments (27)
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.ts (5)
29-33: Guard invalid input and precompute safe helpers.Avoid modulo by 0 and normalize option counts before the switch.
if (!current.isActive) { return current; } + // Normalize inputs to safe values + const safeMaxColumns = Math.max(1, Math.floor(maxColumns || 1)); + const getOptionCount = (col: number) => Math.max(1, Math.floor(optionCounts[col] ?? 1)); + switch (action.type) {
35-47: Use normalized option count (don’t rely on|| 1).Prevents surprises if a column reports 0 or non-integer options.
- const currentColumnOptions = optionCounts[current.column] || 1; + const currentColumnOptions = getOptionCount(current.column);- const currentColumnOptions = optionCounts[current.column] || 1; + const currentColumnOptions = getOptionCount(current.column);
50-57: Clamp option to [0, maxIdx] when switching right; use safe column count.Ensures non-negative option after column switch and avoids
% 0.- const newColumn = (current.column + 1) % maxColumns; + const newColumn = (current.column + 1) % safeMaxColumns; return { column: newColumn, - option: Math.min(current.option, (optionCounts[newColumn] || 1) - 1), + option: Math.min(Math.max(0, current.option), getOptionCount(newColumn) - 1), isActive: true, };
59-66: Mirror the same safety on left navigation.Keep behavior consistent with ARROW_RIGHT change.
- const newColumn = (current.column - 1 + maxColumns) % maxColumns; + const newColumn = (current.column - 1 + safeMaxColumns) % safeMaxColumns; return { column: newColumn, - option: Math.min(current.option, (optionCounts[newColumn] || 1) - 1), + option: Math.min(Math.max(0, current.option), getOptionCount(newColumn) - 1), isActive: true, };
23-28: Nit: RenamemaxColumnstocolumnCountfor clarity.It’s the actual column count, not a “maximum”.
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts (1)
8-12: Remove unusedinitialState(CodeQL warning).Not referenced; drop it to keep tests clean.
- const initialState: DropdownFocusState = { - column: 0, - option: 0, - isActive: true, - };src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.test.ts (3)
1-6: Import SegmentConstraints from the source of truth (utils), unless explicitly re-exported.Prevents type drift between modules.
-import { - handleSegmentValueChange, - SegmentConstraints, -} from 'src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange'; -import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; +import { handleSegmentValueChange } from 'src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange'; +import type { TimeValue, SegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
184-194: Fix mismatched test names (“higher/lower side”).Titles don’t match expectations; tweak for clarity.
- it('should find nearest valid value on the higher side', () => { + it('should find nearest valid value on the lower side', () => { const result = handleSegmentValueChange( 'minutes', 37, // Closer to 30 than 45 mockTimeValue, mockConstraints, true, ); expect(result.updatedTimeValue).toEqual({ minutes: 30 }); }); - it('should find nearest valid value on the lower side', () => { + it('should find nearest valid value on the higher side', () => { const result = handleSegmentValueChange( 'minutes', 38, // Closer to 45 than 30 mockTimeValue, mockConstraints, true, ); expect(result.updatedTimeValue).toEqual({ minutes: 45 }); });Also applies to: 196-206
171-207: Clarify tie-breaking when equidistant to two valid values.If 37.5 were possible, which side wins? Consider adding a spec and test to lock desired behavior.
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (1)
8-24: Don’t shadow library types in tests; import them.Local interfaces diverge from the module (e.g., period optionality). Import types to keep tests aligned.
-import { - getNextValidValue, - getSegmentConstraints, - isTimeInRange, - parseTimeString, -} from 'src/app-components/TimePicker/utils/timeConstraintUtils'; - -interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -} - -interface TimeConstraints { - minTime?: string; - maxTime?: string; -} - -interface SegmentConstraints { - min: number; - max: number; - validValues: number[]; -} +import { + getNextValidValue, + getSegmentConstraints, + isTimeInRange, + parseTimeString, +} from 'src/app-components/TimePicker/utils/timeConstraintUtils'; +import type { TimeValue, TimeConstraints, SegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils';src/app-components/TimePicker/utils/timeConstraintUtils.ts (2)
63-85: Optional: Support cross‑midnight ranges in isTimeInRange.Handle minTime > maxTime (e.g., 22:00–02:00).
if (constraints.maxTime) { const maxTime = parseTimeString(constraints.maxTime, format); maxInSeconds = maxTime.hours * 3600 + maxTime.minutes * 60 + maxTime.seconds; } - return timeInSeconds >= minInSeconds && timeInSeconds <= maxInSeconds; + if (constraints.minTime && constraints.maxTime && minInSeconds > maxInSeconds) { + // Window wraps past midnight + return timeInSeconds >= minInSeconds || timeInSeconds <= maxInSeconds; + } + return timeInSeconds >= minInSeconds && timeInSeconds <= maxInSeconds;
1-2: Decouple utils from component layer for TimeFormat.Importing TimeFormat from a component introduces upward dependency. Consider moving TimeFormat to a shared utils/types module and importing from there.
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts (3)
126-129: Remove the unsafe cast and validate period at runtime (avoid unnecessaryas).Casting the string to
'AM'|'PM'bypasses runtime validation and violates our TS guideline. Narrow via a guard and throw on invalid input.Apply:
- if (segmentType === 'period' && typeof newValue === 'string') { - return handlePeriodChange(newValue as 'AM' | 'PM', currentTimeValue); - } + if (segmentType === 'period') { + if (newValue === 'AM' || newValue === 'PM') { + return handlePeriodChange(newValue, currentTimeValue); + } + throw new Error(`Invalid period value: ${String(newValue)}`); + }Optionally, add overloads for stricter typing at call sites:
export function handleSegmentValueChange( segmentType: 'period', newValue: 'AM' | 'PM', currentTimeValue: TimeValue, segmentConstraints: SegmentConstraints, is12Hour: boolean, ): SegmentChangeResult; export function handleSegmentValueChange( segmentType: NumericSegmentType, newValue: number, currentTimeValue: TimeValue, segmentConstraints: SegmentConstraints, is12Hour: boolean, ): SegmentChangeResult;
131-134: Validate numeric input shape (integers only).If decimals slip through (e.g., via wheel input), wrapping will keep fractions. Gate to integers.
Apply:
- if (segmentType !== 'period' && typeof newValue === 'number') { - return handleNumericSegmentChange(segmentType, newValue, segmentConstraints, is12Hour); + if (segmentType !== 'period' && typeof newValue === 'number') { + const intValue = Number.isInteger(newValue) ? newValue : Math.trunc(newValue); + return handleNumericSegmentChange(segmentType, intValue, segmentConstraints, is12Hour); }
3-6: Leverage min/max consistently or remove them.
min/maxare currently unused except in the suggested clamp path. Either consistently enforce [min,max] for all numeric segments or drop them from the type to avoid dead fields.Would you like me to wire min/max into wrapping (post-wrap clamp) for all numeric segments?
Also applies to: 102-114
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.ts (3)
40-45: Remove unnecessary type assertions in Math.max/Math.min
valuesis inferred asnumber[]; theas number[]casts are redundant.- expect(Math.max(...(values as number[]))).toBe(12); - expect(Math.min(...(values as number[]))).toBe(1); + expect(Math.max(...values)).toBe(12); + expect(Math.min(...values)).toBe(1);
49-85: Add non-divisor step case (e.g., 7) to catch off‑by‑one bugsWhen step doesn’t divide 60, we should still include the largest multiple below 60 (e.g., 56 for step=7). Add a test to lock this down.
it('should generate correct number of options for step=15', () => { const options = generateMinuteOptions(15); @@ expect(options[10].label).toBe('10'); }); + + it('should include the largest multiple below 60 when step does not divide 60 (step=7)', () => { + const options = generateMinuteOptions(7); + expect(options.at(-1)).toEqual({ value: 56, label: '56' }); + });
87-112: Mirror non-divisor and boundary tests for seconds; add validation tests if we guard inputsAdd equivalent tests for seconds (step=7 and step=60). If we add input validation (recommended in the impl comment), also assert throws for invalid steps.
it('should behave identically to generateMinuteOptions', () => { const minuteOptions = generateMinuteOptions(10); const secondOptions = generateSecondOptions(10); expect(secondOptions).toEqual(minuteOptions); }); + + it('should include the largest multiple below 60 when step does not divide 60 (step=7)', () => { + const options = generateSecondOptions(7); + expect(options.at(-1)).toEqual({ value: 56, label: '56' }); + }); + + it('should handle step=60 by returning only 00', () => { + expect(generateMinuteOptions(60)).toEqual([{ value: 0, label: '00' }]); + expect(generateSecondOptions(60)).toEqual([{ value: 0, label: '00' }]); + }); + + // Enable these once input validation is added in the implementation: + // expect(() => generateMinuteOptions(0)).toThrow(RangeError); + // expect(() => generateSecondOptions(-5)).toThrow(RangeError); + // expect(() => generateMinuteOptions(2.5)).toThrow(RangeError); + // expect(() => generateSecondOptions(61)).toThrow(RangeError);src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts (2)
27-29: Doc tweak: reflect supported/common step values and thrown errorDocs mention common values but omit 10, 60 and don’t state the error on invalid input. Update JSDoc for clarity.
/** * Generates minute options for the timepicker dropdown - * @param step - Step increment for minutes (default: 1, common values: 1, 5, 15, 30) + * @param step - Positive integer step (1–60). Common values: 1, 5, 10, 15, 30, 60. Throws RangeError on invalid input. * @returns Array of minute options with value and label */ @@ /** * Generates second options for the timepicker dropdown - * @param step - Step increment for seconds (default: 1, common values: 1, 5, 15, 30) + * @param step - Positive integer step (1–60). Common values: 1, 5, 10, 15, 30, 60. Throws RangeError on invalid input. * @returns Array of second options with value and label */Also applies to: 44-46
25-40: Optional: de-duplicate minute/second generatorsA small private helper reduces duplication and keeps the boundary logic in one place.
+const generateUnitOptions = (step: number): TimeOption[] => { + if (!Number.isFinite(step) || !Number.isInteger(step) || step <= 0 || step > 60) { + throw new RangeError('step must be an integer between 1 and 60'); + } + const count = Math.floor(59 / step) + 1; + return Array.from({ length: count }, (_, i) => { + const value = i * step; + return { value, label: value.toString().padStart(2, '0') }; + }); +}; @@ -export const generateMinuteOptions = (step: number = 1): TimeOption[] => { - const count = Math.floor(59 / step) + 1; - - return Array.from({ length: count }, (_, i) => { - const value = i * step; - return { - value, - label: value.toString().padStart(2, '0'), - }; - }); -}; +export const generateMinuteOptions = (step: number = 1): TimeOption[] => generateUnitOptions(step); @@ -export const generateSecondOptions = (step: number = 1): TimeOption[] => { - const count = Math.floor(59 / step) + 1; - - return Array.from({ length: count }, (_, i) => { - const value = i * step; - return { - value, - label: value.toString().padStart(2, '0'), - }; - }); -}; +export const generateSecondOptions = (step: number = 1): TimeOption[] => generateUnitOptions(step);Also applies to: 42-57
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.ts (4)
7-21: Add input validation to enforce 0–23 hour rangeOut-of-range inputs (e.g., 24, -1, NaN) silently misformat; e.g., 24 → 12 in 12h mode. Guard early and throw.
Apply:
export const formatDisplayHour = (hour: number, is12Hour: boolean): number => { + if (!Number.isInteger(hour) || hour < 0 || hour > 23) { + throw new RangeError(`hour must be an integer in [0, 23], received: ${hour}`); + } if (!is12Hour) { return hour; }
1-6: Document failure modeIf adding the range guard, document it explicitly.
/** * Formats an hour value for display based on the time format * @param hour - The hour value (0-23) * @param is12Hour - Whether to use 12-hour format display - * @returns The formatted hour value for display + * @returns The formatted hour value for display + * @throws RangeError if hour is not an integer in [0, 23] */
7-7: Optional: provide overloads for clearer return contractHelps callers with intellisense when toggling 12h vs 24h.
-export const formatDisplayHour = (hour: number, is12Hour: boolean): number => { +export function formatDisplayHour(hour: number, is12Hour: false): number; +export function formatDisplayHour(hour: number, is12Hour: true): number; +export function formatDisplayHour(hour: number, is12Hour: boolean): number {-}; +}
17-19: Minor readability tweakUse >= 13 to make the boundary explicit; behavior unchanged.
- if (hour > 12) { + if (hour >= 13) {src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.ts (1)
53-68: Add full 0–23 mapping verificationProperty-like check ensures no gaps and guards future regressions.
describe('comprehensive 12-hour conversion table', () => { @@ it.each(conversions)('should convert hour $input to $expected in 12-hour format', ({ input, expected }) => { expect(formatDisplayHour(input, true)).toBe(expected); }); + + it('should correctly map every hour in [0,23] for both modes', () => { + const expected12h = [12,1,2,3,4,5,6,7,8,9,10,11,12,1,2,3,4,5,6,7,8,9,10,11]; + for (let h = 0; h < 24; h++) { + expect(formatDisplayHour(h, false)).toBe(h); + expect(formatDisplayHour(h, true)).toBe(expected12h[h]); + } + }); });src/app-components/TimePicker/components/TimePicker.tsx (1)
652-652: Hardcoded English label should use localized label.The AM/PM dropdown label is hardcoded as "AM/PM" but should use the localized label for consistency with the component's design.
- <div className={styles.dropdownLabel}>AM/PM</div> + <div className={styles.dropdownLabel}>{segmentLabels.period}</div>src/app-components/TimePicker/tests/TimePicker.responsive.test.tsx (1)
51-52: Consider using a more reliable element selection approach.Using
.closest('.calendarInputWrapper')with a CSS class name makes the test brittle if the CSS class changes. Consider usingdata-testidor role-based selection.- const wrapper = screen.getByRole('textbox', { name: /hours/i }).closest('.calendarInputWrapper'); + const wrapper = screen.getByRole('group'); // Uses the role='group' from TimePicker
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/app-components/TimePicker/components/TimePicker.module.css(1 hunks)src/app-components/TimePicker/components/TimePicker.tsx(1 hunks)src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts(1 hunks)src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.ts(1 hunks)src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.ts(1 hunks)src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.ts(1 hunks)src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.ts(1 hunks)src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts(1 hunks)src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.test.ts(1 hunks)src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts(1 hunks)src/app-components/TimePicker/tests/TimePicker.focus.test.tsx(1 hunks)src/app-components/TimePicker/tests/TimePicker.responsive.test.tsx(1 hunks)src/app-components/TimePicker/tests/timeConstraintUtils.test.ts(1 hunks)src/app-components/TimePicker/utils/timeConstraintUtils.ts(1 hunks)src/language/texts/en.ts(1 hunks)src/language/texts/nb.ts(1 hunks)src/language/texts/nn.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/language/texts/nb.ts
- src/language/texts/nn.ts
- src/language/texts/en.ts
- src/app-components/TimePicker/components/TimePicker.module.css
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.tssrc/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.tssrc/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.tssrc/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.tssrc/app-components/TimePicker/tests/TimePicker.responsive.test.tsxsrc/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.tssrc/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.tssrc/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.tssrc/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.test.tssrc/app-components/TimePicker/utils/timeConstraintUtils.tssrc/app-components/TimePicker/tests/TimePicker.focus.test.tsxsrc/app-components/TimePicker/tests/timeConstraintUtils.test.tssrc/app-components/TimePicker/components/TimePicker.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.tssrc/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.tssrc/app-components/TimePicker/tests/TimePicker.responsive.test.tsxsrc/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.tssrc/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.test.tssrc/app-components/TimePicker/tests/TimePicker.focus.test.tsxsrc/app-components/TimePicker/tests/timeConstraintUtils.test.ts
🧬 Code graph analysis (10)
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.ts (1)
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.ts (1)
formatDisplayHour(7-22)
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.ts (1)
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts (3)
generateHourOptions(11-23)generateMinuteOptions(30-40)generateSecondOptions(47-57)
src/app-components/TimePicker/tests/TimePicker.responsive.test.tsx (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimePicker(44-691)
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts (1)
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.ts (3)
DropdownFocusState(1-5)NavigationAction(7-13)calculateNextFocusState(23-80)
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts (1)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (2)
SegmentConstraints(15-19)TimeValue(3-8)
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.test.ts (2)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (2)
TimeValue(3-8)SegmentConstraints(15-19)src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts (2)
SegmentConstraints(3-7)handleSegmentValueChange(119-138)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimeFormat(25-25)
src/app-components/TimePicker/tests/TimePicker.focus.test.tsx (1)
src/app-components/TimePicker/components/TimePicker.tsx (1)
TimePicker(44-691)
src/app-components/TimePicker/tests/timeConstraintUtils.test.ts (1)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (7)
TimeValue(3-8)TimeConstraints(10-13)SegmentConstraints(15-19)parseTimeString(21-61)isTimeInRange(63-85)getSegmentConstraints(87-194)getNextValidValue(196-222)
src/app-components/TimePicker/components/TimePicker.tsx (8)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (4)
parseTimeString(21-61)TimeConstraints(10-13)TimeValue(3-8)getSegmentConstraints(87-194)src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.ts (3)
DropdownFocusState(1-5)NavigationAction(7-13)calculateNextFocusState(23-80)src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts (2)
SegmentType(11-11)handleSegmentValueChange(119-138)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatTimeValue(5-26)src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.ts (1)
formatDisplayHour(7-22)src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts (3)
generateHourOptions(11-23)generateMinuteOptions(30-40)generateSecondOptions(47-57)src/app-components/TimePicker/components/TimeSegment.tsx (1)
TimeSegment(30-157)
🪛 GitHub Check: CodeQL
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts
[notice] 8-8: Unused variable, import, function or class
Unused variable initialState.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript)
- GitHub Check: Install
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
🔇 Additional comments (16)
src/app-components/TimePicker/functions/calculateNextFocusState/calculateNextFocusState.test.ts (1)
1-228: Solid coverage of navigation behaviors.Tests are focused and independent of rendering; no need for
renderWithProvidershere.src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.test.ts (1)
28-46: Solid coverage for 12/24-hour logicThe tests assert ranges, padding, and absence of 00 in 12h correctly. Nice.
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts (1)
11-23: Hour options look correctCorrect ranges and zero-padding for both 12h and 24h formats.
src/app-components/TimePicker/functions/formatDisplayHour/formatDisplayHour.test.ts (2)
4-31: Solid coverage of core scenariosCovers 24h passthrough and 12h edge cases well.
1-1: Path alias 'src/*' is correctly configured for teststsconfig.json defines "paths": { "src/": ["src/"] } and jest.config.js maps '^src/(.*)$' → '/src/$1'; the import will resolve in tests.
src/app-components/TimePicker/components/TimePicker.tsx (3)
370-383: Hours conversion logic handles 12-hour format correctly.The dropdown hours change handler properly converts between 12-hour UI values and 24-hour internal representation, correctly handling midnight (12 AM → 0) and noon (12 PM → 12) edge cases.
393-401: Period change logic correctly updates hours.The AM/PM change handler properly converts hours when switching periods, handling the transitions between AM and PM correctly.
460-497: Well-structured Popover with proper accessibility and focus management.The Popover implementation includes appropriate ARIA attributes (
aria-modal,role="dialog",aria-label), proper focus management with initial focus setup, and focus restoration on close.src/app-components/TimePicker/tests/TimePicker.focus.test.tsx (4)
15-26: Good practice: Comprehensive getComputedStyle mock for JSDOM.The mock includes all necessary properties to prevent JSDOM failures with Popover positioning, which is essential for stable testing.
82-100: Pragmatic approach to JSDOM limitations with focus restoration.The test acknowledges JSDOM's limitations with Popover behavior and provides a fallback assertion strategy. This is a practical solution for testing focus restoration logic that may not work perfectly in the test environment.
139-152: Comprehensive focus navigation test for complex format.The test validates that all four segments (hours, minutes, seconds, AM/PM) are properly focusable and that arrow key navigation works correctly across all segments in the most complex format.
156-178: Good coverage of focus cycling behavior.The test validates that focus properly wraps around between segments, testing both forward and backward navigation which is essential for keyboard accessibility.
src/app-components/TimePicker/tests/TimePicker.responsive.test.tsx (4)
15-28: Enhanced getComputedStyle mock with additional properties.The mock includes width and height properties compared to the focus test, providing more comprehensive coverage for layout-dependent functionality.
42-57: Effective responsive testing at minimum breakpoint.The test properly simulates the smallest supported width and verifies that the component remains functional with the expected number of inputs.
113-129: Good test coverage of custom labels.The test validates that custom labels are properly applied to input elements, ensuring the localization system works correctly.
205-239: Comprehensive disabled and readonly state testing.The tests properly validate accessibility requirements for disabled and readonly states, ensuring screen readers can properly announce these states to users.
| const [showDropdown, setShowDropdown] = useState(false); | ||
| const [_focusedSegment, setFocusedSegment] = useState<number | null>(null); |
There was a problem hiding this comment.
Unused state variable flagged by security scanner.
The _focusedSegment state variable is prefixed with underscore to indicate it's unused, but GitHub's security scanner has flagged this as a "useless assignment". Consider removing this state entirely if it's truly unused.
- const [_focusedSegment, setFocusedSegment] = useState<number | null>(null);And remove all calls to setFocusedSegment:
- setFocusedSegment(nextIndex);
- onFocus={() => setFocusedSegment(index)}
- onBlur={() => setFocusedSegment(null)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [showDropdown, setShowDropdown] = useState(false); | |
| const [_focusedSegment, setFocusedSegment] = useState<number | null>(null); | |
| const [showDropdown, setShowDropdown] = useState(false); |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimePicker.tsx around lines 57-58,
remove the unused state declaration const [_focusedSegment, setFocusedSegment] =
useState<number | null>(null) and delete every call to setFocusedSegment and any
reads of _focusedSegment throughout the file; ensure no remaining references to
that identifier exist and remove any now-unused imports if they become unused
after this change.
| role='group' | ||
| aria-label='Hours selection' | ||
| > | ||
| <div className={styles.dropdownLabel}>Timer</div> |
There was a problem hiding this comment.
Hardcoded Norwegian label breaks localization.
The dropdown label "Timer" is hardcoded in Norwegian, breaking the component's localization system and making it unusable for non-Norwegian users.
Use the segmentLabels.hours property that respects the labels prop:
- <div className={styles.dropdownLabel}>Timer</div>
+ <div className={styles.dropdownLabel}>{segmentLabels.hours}</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={styles.dropdownLabel}>Timer</div> | |
| <div className={styles.dropdownLabel}>{segmentLabels.hours}</div> |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimePicker.tsx around line 506, the
dropdown label is hardcoded as "Timer" which breaks localization; replace the
literal string with the localized value (use the component's labels prop /
segmentLabels.hours) so the label reads from labels.segmentLabels.hours (falling
back to the default label if labels or segmentLabels is missing) to respect
localization.
| role='group' | ||
| aria-label='Minutes selection' | ||
| > | ||
| <div className={styles.dropdownLabel}>Minutter</div> |
There was a problem hiding this comment.
Hardcoded Norwegian label breaks localization.
The dropdown label "Minutter" is hardcoded in Norwegian, breaking the component's localization system.
- <div className={styles.dropdownLabel}>Minutter</div>
+ <div className={styles.dropdownLabel}>{segmentLabels.minutes}</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={styles.dropdownLabel}>Minutter</div> | |
| <div className={styles.dropdownLabel}>{segmentLabels.minutes}</div> |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimePicker.tsx around line 559, the
dropdown label "Minutter" is hardcoded in Norwegian which breaks localization;
replace the literal with the component's localization mechanism (e.g., use the
existing i18n/translation hook or the component's label props) so the label is
resolved from translations or passed-in props (e.g., t('timePicker.minutes') or
props.labels.minutes), and ensure fallback to an English default if the
translation key or prop is missing.
| role='group' | ||
| aria-label='Seconds selection' | ||
| > | ||
| <div className={styles.dropdownLabel}>Sekunder</div> |
There was a problem hiding this comment.
Hardcoded Norwegian label breaks localization.
The dropdown label "Sekunder" is hardcoded in Norwegian, breaking the component's localization system.
- <div className={styles.dropdownLabel}>Sekunder</div>
+ <div className={styles.dropdownLabel}>{segmentLabels.seconds}</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={styles.dropdownLabel}>Sekunder</div> | |
| <div className={styles.dropdownLabel}>{segmentLabels.seconds}</div> |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/components/TimePicker.tsx around line 605, the
dropdown label currently uses the hardcoded Norwegian string "Sekunder"; replace
it with the component's localization mechanism (e.g., use the existing
translation function or locale prop such as t('timePicker.seconds') or
props.locale.labels.seconds) instead of a literal string, add the corresponding
key/value to the translations/locales files for all supported languages, and
ensure a sensible fallback (like 'Seconds') if the translation is missing.
| export const generateMinuteOptions = (step: number = 1): TimeOption[] => { | ||
| const count = Math.floor(60 / step); | ||
|
|
||
| return Array.from({ length: count }, (_, i) => { | ||
| const value = i * step; | ||
| return { | ||
| value, | ||
| label: value.toString().padStart(2, '0'), | ||
| }; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix off-by-one and add step validation in minute options
- Off-by-one: for non-divisor steps (e.g., 7), the current
Math.floor(60 / step)misses the last valid value (56). UseMath.floor(59 / step) + 1. - Add guards to prevent runtime errors for
step <= 0, non-integers,> 60, or non-finite values.
export const generateMinuteOptions = (step: number = 1): TimeOption[] => {
- const count = Math.floor(60 / step);
+ if (!Number.isFinite(step) || !Number.isInteger(step) || step <= 0 || step > 60) {
+ throw new RangeError('step must be an integer between 1 and 60');
+ }
+ const count = Math.floor(59 / step) + 1;
return Array.from({ length: count }, (_, i) => {
const value = i * step;
return {
value,
label: value.toString().padStart(2, '0'),
};
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const generateMinuteOptions = (step: number = 1): TimeOption[] => { | |
| const count = Math.floor(60 / step); | |
| return Array.from({ length: count }, (_, i) => { | |
| const value = i * step; | |
| return { | |
| value, | |
| label: value.toString().padStart(2, '0'), | |
| }; | |
| }); | |
| }; | |
| export const generateMinuteOptions = (step: number = 1): TimeOption[] => { | |
| if (!Number.isFinite(step) || !Number.isInteger(step) || step <= 0 || step > 60) { | |
| throw new RangeError('step must be an integer between 1 and 60'); | |
| } | |
| const count = Math.floor(59 / step) + 1; | |
| return Array.from({ length: count }, (_, i) => { | |
| const value = i * step; | |
| return { | |
| value, | |
| label: value.toString().padStart(2, '0'), | |
| }; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts
around lines 30 to 40, the minute generation has an off-by-one for non-divisor
steps and lacks input validation; change the count calculation to use
Math.floor(59 / step) + 1 so values like 56 for step=7 are included, and add
upfront guards that validate step is a finite integer > 0 and <= 60 (throw a
clear TypeError or RangeError for invalid inputs) before computing the array;
keep the existing mapping logic (value and zero-padded label) unchanged.
| export const generateSecondOptions = (step: number = 1): TimeOption[] => { | ||
| const count = Math.floor(60 / step); | ||
|
|
||
| return Array.from({ length: count }, (_, i) => { | ||
| const value = i * step; | ||
| return { | ||
| value, | ||
| label: value.toString().padStart(2, '0'), | ||
| }; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same off-by-one fix and validation in second options
Mirror the minute options fix for consistency and correctness.
export const generateSecondOptions = (step: number = 1): TimeOption[] => {
- const count = Math.floor(60 / step);
+ if (!Number.isFinite(step) || !Number.isInteger(step) || step <= 0 || step > 60) {
+ throw new RangeError('step must be an integer between 1 and 60');
+ }
+ const count = Math.floor(59 / step) + 1;
return Array.from({ length: count }, (_, i) => {
const value = i * step;
return {
value,
label: value.toString().padStart(2, '0'),
};
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const generateSecondOptions = (step: number = 1): TimeOption[] => { | |
| const count = Math.floor(60 / step); | |
| return Array.from({ length: count }, (_, i) => { | |
| const value = i * step; | |
| return { | |
| value, | |
| label: value.toString().padStart(2, '0'), | |
| }; | |
| }); | |
| }; | |
| export const generateSecondOptions = (step: number = 1): TimeOption[] => { | |
| if (!Number.isFinite(step) || !Number.isInteger(step) || step <= 0 || step > 60) { | |
| throw new RangeError('step must be an integer between 1 and 60'); | |
| } | |
| const count = Math.floor(59 / step) + 1; | |
| return Array.from({ length: count }, (_, i) => { | |
| const value = i * step; | |
| return { | |
| value, | |
| label: value.toString().padStart(2, '0'), | |
| }; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/functions/generateTimeOptions/generateTimeOptions.ts
around lines 47 to 57, the seconds generator needs the same off-by-one and step
validation as minutes: validate that step is a positive integer (>0) and
default/throw if not, compute the number of entries so values include up to 59
(use Math.ceil(60 / step) or loop while value <= 59) and ensure you never emit a
value >= 60 by capping/checking each generated value; keep labels zero-padded
with padStart(2,'0').
| export interface SegmentConstraints { | ||
| min: number; | ||
| max: number; | ||
| validValues: number[]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deduplicate SegmentConstraints; import from the shared utils.
This local interface duplicates the one in utils/timeConstraintUtils and risks drift. Import it instead to keep a single source of truth.
Apply:
import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
+import type { SegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
-export interface SegmentConstraints {
- min: number;
- max: number;
- validValues: number[];
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SegmentConstraints { | |
| min: number; | |
| max: number; | |
| validValues: number[]; | |
| } | |
| import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; | |
| import type { SegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; |
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts
around lines 3 to 7, the local SegmentConstraints interface duplicates the one
in utils/timeConstraintUtils; remove the local declaration and import the shared
SegmentConstraints from utils/timeConstraintUtils instead, update the file's
imports accordingly, and ensure any type references use the imported name so
there is a single source of truth.
| const wrapHours = (value: number, is12Hour: boolean): number => { | ||
| if (is12Hour) { | ||
| if (value > 12) { | ||
| return 1; | ||
| } | ||
| if (value < 1) { | ||
| return 12; | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| if (value > 23) { | ||
| return 0; | ||
| } | ||
| if (value < 0) { | ||
| return 23; | ||
| } | ||
| return value; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make wrapping truly cyclic (handle jumps >1) using modulo.
Current wrap returns 1/12 or 0/59 for any out-of-range value, which is wrong for multi-step changes (e.g., 26h ⇒ 2, 125s ⇒ 5). Use modulo.
Apply:
-const wrapHours = (value: number, is12Hour: boolean): number => {
- if (is12Hour) {
- if (value > 12) {
- return 1;
- }
- if (value < 1) {
- return 12;
- }
- return value;
- }
-
- if (value > 23) {
- return 0;
- }
- if (value < 0) {
- return 23;
- }
- return value;
-};
+const mod = (n: number, m: number) => ((n % m) + m) % m;
+const wrapHours = (value: number, is12Hour: boolean): number =>
+ is12Hour ? mod(value - 1, 12) + 1 : mod(value, 24);
-const wrapMinutesSeconds = (value: number): number => {
- if (value > 59) {
- return 0;
- }
- if (value < 0) {
- return 59;
- }
- return value;
-};
+const wrapMinutesSeconds = (value: number): number => mod(value, 60);Also applies to: 62-70
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts
around lines 39 to 57 (and similarly for lines 62 to 70), the
wrapHours/wrapSeconds logic only clamps single-step out-of-range values instead
of performing cyclic wrapping for arbitrary jumps; replace the current
conditional returns with modulo arithmetic using the appropriate base (12-hour
-> 12 with 1..12 mapping, 24-hour -> 24 with 0..23, seconds/minutes -> 60 with
0..59) and normalize negatives by adding the base before modulo so values like
26h -> 2 and 125s -> 5 wrap correctly.
| const findNearestValidValue = (targetValue: number, validValues: number[]): number => | ||
| validValues.reduce((prev, curr) => (Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev)); | ||
|
|
There was a problem hiding this comment.
Guard against empty validValues to avoid reduce() crash; also use min/max.
reduce() on an empty array throws. If no discrete valid values are provided, clamp to [min,max].
Apply:
-const findNearestValidValue = (targetValue: number, validValues: number[]): number =>
- validValues.reduce((prev, curr) => (Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev));
+const findNearestValidValue = (targetValue: number, validValues: number[]): number => {
+ // Caller must ensure length > 0
+ return validValues.reduce((prev, curr) =>
+ Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev
+ );
+};And in the caller:
const wrappedValue = wrapNumericValue(value, segmentType, is12Hour);
- // Return wrapped value if it's within constraints
- if (segmentConstraints.validValues.includes(wrappedValue)) {
+ const { validValues, min, max } = segmentConstraints;
+ if (!validValues || validValues.length === 0) {
+ const clamped = Math.min(max, Math.max(min, wrappedValue));
+ return { updatedTimeValue: { [segmentType]: clamped } };
+ }
+ // Return wrapped value if it's within constraints
+ if (validValues.includes(wrappedValue)) {
return {
- updatedTimeValue: { [segmentType]: wrappedValue },
+ updatedTimeValue: { [segmentType]: wrappedValue },
};
}
- // Find and return nearest valid value
- const nearestValid = findNearestValidValue(wrappedValue, segmentConstraints.validValues);
+ // Find and return nearest valid value
+ const nearestValid = findNearestValidValue(wrappedValue, validValues);
return {
updatedTimeValue: { [segmentType]: nearestValid },
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const findNearestValidValue = (targetValue: number, validValues: number[]): number => | |
| validValues.reduce((prev, curr) => (Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev)); | |
| const findNearestValidValue = (targetValue: number, validValues: number[]): number => { | |
| // Caller must ensure length > 0 | |
| return validValues.reduce((prev, curr) => | |
| Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev | |
| ); | |
| }; | |
| const wrappedValue = wrapNumericValue(value, segmentType, is12Hour); | |
| const { validValues, min, max } = segmentConstraints; | |
| if (!validValues || validValues.length === 0) { | |
| const clamped = Math.min(max, Math.max(min, wrappedValue)); | |
| return { updatedTimeValue: { [segmentType]: clamped } }; | |
| } | |
| // Return wrapped value if it's within constraints | |
| if (validValues.includes(wrappedValue)) { | |
| return { | |
| updatedTimeValue: { [segmentType]: wrappedValue }, | |
| }; | |
| } | |
| // Find and return nearest valid value | |
| const nearestValid = findNearestValidValue(wrappedValue, validValues); | |
| return { | |
| updatedTimeValue: { [segmentType]: nearestValid }, | |
| }; |
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/functions/handleSegmentValueChange/handleSegmentValueChange.ts
around lines 88 to 90, the helper findNearestValidValue uses reduce() which will
throw on an empty validValues array; change it to first check if validValues is
empty and if so return Math.min(Math.max(targetValue, min), max) (clamping to
provided min/max), otherwise perform the nearest-value reduce; also ensure the
caller passes or has access to min and max and falls back to clamping when no
discrete validValues are supplied.
| } from 'src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState'; | ||
|
|
||
| describe('calculateNextFocusState', () => { | ||
| const initialState: DropdownFocusState = { |
Check notice
Code scanning / CodeQL
Unused variable, import, function or class Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the unused variable problem, we should remove the declaration and initialization of initialState entirely from the test file. Specifically, delete line 8:
8: const initialState: DropdownFocusState = {
9: column: 0,
10: option: 0,
11: isActive: true,
12: };Since lines 8-12 are only the declaration of an unused variable, removing these lines will resolve the error without impacting any code functionality. There are no dependencies or required imports for this variable outside these lines, so no further changes are needed.
| @@ -5,11 +5,6 @@ | ||
| } from 'src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState'; | ||
|
|
||
| describe('calculateNextFocusState', () => { | ||
| const initialState: DropdownFocusState = { | ||
| column: 0, | ||
| option: 0, | ||
| isActive: true, | ||
| }; | ||
|
|
||
| const maxColumns = 3; // hours, minutes, seconds | ||
| const optionCounts = [24, 60, 60]; // 24 hours, 60 minutes, 60 seconds |
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (4)
src/app-components/TimePicker/TimePicker.tsx (4)
506-506: Hardcoded Norwegian labels should be internationalizedThe dropdown labels "Timer", "Minutter", and "Sekunder" are hardcoded in Norwegian. These should use the localization system for proper internationalization support.
559-559: Hardcoded Norwegian label "Minutter" should be internationalizedThis label should use the localization system.
605-605: Hardcoded Norwegian label "Sekunder" should be internationalizedThis label should use the localization system.
149-156: Unused variable assignment flagged by static analysisThe initial value of
nextIndexis immediately overwritten in both branches of the conditional. Consider declaring it only when needed.const handleSegmentNavigate = (direction: 'left' | 'right', currentIndex: number) => { - let nextIndex: number; + const nextIndex = direction === 'right' + ? (currentIndex + 1) % segments.length + : (currentIndex - 1 + segments.length) % segments.length; - if (direction === 'right') { - nextIndex = (currentIndex + 1) % segments.length; - } else { - nextIndex = (currentIndex - 1 + segments.length) % segments.length; - } - segmentRefs.current[nextIndex]?.focus(); setFocusedSegment(nextIndex); };
🧹 Nitpick comments (27)
src/layout/TimePicker/useTimePickerValidation.ts (3)
9-12: Rename to clarify “strict” parsing and avoid confusion with util helperThis local parser performs validation (strict). Consider renaming it to
parseTimeStringStrictand updating call sites to reduce ambiguity withutils/timeConstraintUtils.parseTimeString(non-strict). Also aligns the file with future centralization.-const parseTimeString = ( +const parseTimeStringStrict = ( @@ - const parsedTime = parseTimeString(timeString, format); + const parsedTime = parseTimeStringStrict(timeString, format); @@ - const minParsed = parseTimeString(minTime, format); + const minParsed = parseTimeStringStrict(minTime, format); @@ - const maxParsed = parseTimeString(maxTime, format); + const maxParsed = parseTimeStringStrict(maxTime, format);Also applies to: 124-125, 135-137, 147-149
126-131: Add bindingKey for better mapping to the failing bindingInclude
bindingKey: 'simpleBinding'in emitted validations so the framework can attribute messages precisely.if (!parsedTime) { validations.push({ message: { key: 'time_picker.invalid_time_message', params: [format] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', }); @@ validations.push({ message: { key: 'time_picker.min_time_exceeded', params: [minTime] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', }); @@ validations.push({ message: { key: 'time_picker.max_time_exceeded', params: [maxTime] }, severity: 'error', source: FrontendValidationSource.Component, category: ValidationMask.Component, + bindingKey: 'simpleBinding', });Also applies to: 138-144, 150-155
135-157: Optional: guard against misconfigured bounds (min > max)If both
minTimeandmaxTimeare set and min > max, emit a dev warning or a validation to avoid confusing UX.I can draft a small pre-check block (with a console.warn) before the individual min/max checks if you’d like.
src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.ts (1)
7-21: Defensively normalize out-of-range hoursNormalize
hourinto [0,23] to avoid unexpected results if upstream validation ever slips.export const formatDisplayHour = (hour: number, is12Hour: boolean): number => { - if (!is12Hour) { - return hour; - } + // Normalize hour to [0, 23] + const h = ((hour % 24) + 24) % 24; + if (!is12Hour) { + return h; + } // Convert 24-hour to 12-hour format - if (hour === 0) { + if (h === 0) { return 12; // Midnight (00:xx) -> 12:xx AM } - if (hour > 12) { - return hour - 12; // PM hours (13-23) -> 1-11 PM + if (h > 12) { + return h - 12; // PM hours (13-23) -> 1-11 PM } - return hour; // AM hours (1-12) stay the same + return h; // AM hours (1-12) stay the same };src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.ts (1)
50-66: Clamp option to non-negative when changing columnsIf
current.optionis-1, preserve-wrap yields-1. Clamp lower bound to0.case 'ARROW_RIGHT': { const newColumn = (current.column + 1) % maxColumns; return { column: newColumn, - option: Math.min(current.option, (optionCounts[newColumn] || 1) - 1), + option: Math.max(0, Math.min(current.option, (optionCounts[newColumn] || 1) - 1)), isActive: true, }; } @@ case 'ARROW_LEFT': { const newColumn = (current.column - 1 + maxColumns) % maxColumns; return { column: newColumn, - option: Math.min(current.option, (optionCounts[newColumn] || 1) - 1), + option: Math.max(0, Math.min(current.option, (optionCounts[newColumn] || 1) - 1)), isActive: true, }; }src/app-components/TimePicker/utils/keyboardNavigation.test.ts (2)
13-13: Reuse exported SegmentType instead of redefiningImport
SegmentTypefrom the module to avoid drift.-import type SegmentType = 'hours' | 'minutes' | 'seconds' | 'period'; +import type { SegmentType } from 'src/app-components/TimePicker/utils/keyboardNavigation';
17-61: Remove double-casts; create a typed event helperAvoid
as unknown as. A small factory keeps types clean and readable.- it('should handle Arrow Up key', () => { - const mockEvent = { key: 'ArrowUp', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; + const makeEvent = (key: string): MockKeyboardEvent => ({ key, preventDefault: jest.fn() }); + it('should handle Arrow Up key', () => { + const mockEvent = makeEvent('ArrowUp'); const result = handleSegmentKeyDown(mockEvent); @@ - it('should handle Arrow Down key', () => { - const mockEvent = { key: 'ArrowDown', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; + it('should handle Arrow Down key', () => { + const mockEvent = makeEvent('ArrowDown'); @@ - it('should handle Arrow Right key', () => { - const mockEvent = { key: 'ArrowRight', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; + it('should handle Arrow Right key', () => { + const mockEvent = makeEvent('ArrowRight'); @@ - it('should handle Arrow Left key', () => { - const mockEvent = { key: 'ArrowLeft', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; + it('should handle Arrow Left key', () => { + const mockEvent = makeEvent('ArrowLeft'); @@ - it('should not handle other keys', () => { - const mockEvent = { key: 'Enter', preventDefault: jest.fn() } as unknown as MockKeyboardEvent; + it('should not handle other keys', () => { + const mockEvent = makeEvent('Enter');src/app-components/TimePicker/TimeSegment/TimeSegment.test.tsx (1)
241-255: Prefer rerender over multiple render calls in the same testUsing two
rendercalls in one test leaves multiple inputs in the DOM. Rely onrerenderfor clarity and isolation.- jest.clearAllMocks(); - - // Simulate component with PM value for ArrowDown test - render( - <TimeSegment - {...defaultProps} - type='period' - value='PM' - onValueChange={onValueChange} - />, - ); - const pmInput = screen.getAllByRole('textbox')[1]; // Get the second input (PM one) - - await userEvent.click(pmInput); + jest.clearAllMocks(); + const { rerender } = render( + <TimeSegment + {...defaultProps} + type='period' + value='AM' + onValueChange={onValueChange} + />, + ); + rerender( + <TimeSegment + {...defaultProps} + type='period' + value='PM' + onValueChange={onValueChange} + />, + ); + const input = screen.getByRole('textbox'); + await userEvent.click(input); await userEvent.keyboard('{ArrowDown}'); expect(onValueChange).toHaveBeenCalledWith('AM');src/app-components/TimePicker/TimePicker.module.css (1)
144-148: Clarify the commented focus outline stylesThe commented-out focus outline styles for selected+focused options need clarification. Are these intentionally disabled or placeholder for future implementation?
Consider either:
- Removing the commented code if not needed
- Adding a comment explaining why it's disabled
- Implementing the styles if they're needed for better accessibility
.dropdownOptionFocused.dropdownOptionSelected { - /* When option is both focused and selected, prioritize selection styling but add focus outline */ - /*outline: 2px solid var(--ds-color-neutral-text-on-inverted);*/ - /*outline-offset: -2px;*/ + /* Selected state styling takes precedence - focus is indicated by position */ }src/app-components/TimePicker/utils/timeConstraintUtils.test.ts (1)
8-24: Duplicate interface definitions - consider importing from sourceThe test file redefines interfaces that already exist in the source file. This creates maintenance overhead and potential inconsistencies.
Consider importing the interfaces directly:
-interface TimeValue { - hours: number; - minutes: number; - seconds: number; - period: 'AM' | 'PM'; -} - -interface TimeConstraints { - minTime?: string; - maxTime?: string; -} - -interface SegmentConstraints { - min: number; - max: number; - validValues: number[]; -} +import type { + TimeValue, + TimeConstraints, + SegmentConstraints +} from 'src/app-components/TimePicker/utils/timeConstraintUtils';Also note the same
periodtype inconsistency mentioned in the previous file (should be optional).src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts (1)
88-89: Consider performance optimization for findNearestValidValueFor large arrays, this O(n) approach could be optimized. Since
validValuesare typically sorted for time segments, a binary search approach would be more efficient.-const findNearestValidValue = (targetValue: number, validValues: number[]): number => - validValues.reduce((prev, curr) => (Math.abs(curr - targetValue) < Math.abs(prev - targetValue) ? curr : prev)); +const findNearestValidValue = (targetValue: number, validValues: number[]): number => { + if (validValues.length === 0) return targetValue; + if (validValues.length === 1) return validValues[0]; + + // Binary search for efficiency with sorted arrays + let left = 0; + let right = validValues.length - 1; + + while (left < right - 1) { + const mid = Math.floor((left + right) / 2); + if (validValues[mid] === targetValue) return targetValue; + if (validValues[mid] < targetValue) { + left = mid; + } else { + right = mid; + } + } + + // Return the closer value + const leftDiff = Math.abs(validValues[left] - targetValue); + const rightDiff = Math.abs(validValues[right] - targetValue); + return leftDiff <= rightDiff ? validValues[left] : validValues[right]; +};src/app-components/TimePicker/TimePicker.tsx (2)
93-98: Consider using localization keys for default labelsThe default labels should also come from the localization system rather than being hardcoded in English.
Since you're accepting labels as props, the defaults should ideally come from your localization system. Consider fetching these from your translation files:
// Segment labels and placeholders const segmentLabels = { - hours: labels.hours || 'Hours', - minutes: labels.minutes || 'Minutes', - seconds: labels.seconds || 'Seconds', - period: labels.amPm || 'AM/PM', + hours: labels.hours || t('time_picker.hours'), + minutes: labels.minutes || t('time_picker.minutes'), + seconds: labels.seconds || t('time_picker.seconds'), + period: labels.amPm || t('time_picker.am_pm'), };
652-652: Consider localizing the AM/PM labelThe "AM/PM" label in the dropdown should also be localized.
- <div className={styles.dropdownLabel}>AM/PM</div> + <div className={styles.dropdownLabel}>{labels.amPm || 'AM/PM'}</div>src/app-components/TimePicker/TimeSegment/TimeSegment.tsx (2)
153-153: Remove redundant maxLength calculation.The
maxLengthprop has the same value (2) for both conditions, making the ternary operator unnecessary.Apply this diff to simplify:
- maxLength={type === 'period' ? 2 : 2} + maxLength={2}
13-14: Remove unused props from interface.The
minandmaxprops are defined in the interface but never used in the component implementation.If these props are intended for future validation, consider adding a TODO comment. Otherwise, remove them:
export interface TimeSegmentProps { value: number | string; - min: number; - max: number; type: SegmentType;src/app-components/TimePicker/utils/segmentTyping.ts (3)
137-143: Unreachable code path for numeric segments.For numeric segments (hours, minutes, seconds), the buffer will always contain digits since non-numeric input is filtered out in
handleSegmentCharacterInput. TheisNaNcheck on Line 137 will never be true for these segments.Consider removing the unreachable code or adding a comment explaining when this might occur:
const numValue = parseInt(buffer, 10); if (Number.isNaN(numValue)) { + // This should not occur for numeric segments as input is validated return { displayValue: '--', actualValue: null, isComplete: false, }; }
120-120: Unused parameter_is12Hour.The
_is12Hourparameter inprocessSegmentBufferis prefixed with underscore but could be useful for proper validation of hour values.Consider using this parameter to validate hour ranges:
-export const processSegmentBuffer = (buffer: string, segmentType: SegmentType, _is12Hour: boolean): SegmentBuffer => { +export const processSegmentBuffer = (buffer: string, segmentType: SegmentType, is12Hour: boolean): SegmentBuffer => { if (buffer === '') { return { displayValue: '--', actualValue: null, isComplete: false, }; } if (segmentType === 'period') { return { displayValue: buffer, actualValue: buffer, isComplete: buffer === 'AM' || buffer === 'PM', }; } const numValue = parseInt(buffer, 10); if (Number.isNaN(numValue)) { return { displayValue: '--', actualValue: null, isComplete: false, }; } + + // Validate hour range + if (segmentType === 'hours') { + const maxHour = is12Hour ? 12 : 23; + const minHour = is12Hour ? 1 : 0; + if (numValue < minHour || numValue > maxHour) { + // Handle invalid hour value + } + }
80-81: Inconsistent advancement behavior for minute coercion.When minutes 6-9 are coerced to 0X (e.g., '6' becomes '06'), the function returns
shouldAdvance: false, but the value is already complete. This differs from the hour input behavior where coercion triggers advancement.Consider making the behavior consistent:
} else { - // Coerce 6-9 to 0X (complete, but don't advance - Chrome behavior) - return { value: `0${digit}`, shouldAdvance: false }; + // Coerce 6-9 to 0X and advance since value is complete + return { value: `0${digit}`, shouldAdvance: true }; }src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.ts (4)
1-6: Import SegmentConstraints from the source of truth to avoid type divergencePrefer importing SegmentConstraints from timeConstraintUtils (or re-export from there) to keep typings consistent across utils and tests.
-import { - handleSegmentValueChange, - SegmentConstraints, -} from 'src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange'; +import { handleSegmentValueChange } from 'src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange'; +import type { SegmentConstraints } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
21-63: Optional: add boundary tests for 12↔24 edge hours (12 AM/PM)Consider adding cases for 12 AM (should map to 0) and 12 PM (should stay 12) to lock down boundary behavior.
171-207: Tighten test names and add a tie-breaker case
- The names “higher side”/“lower side” don’t match the actual direction picked. Rename for clarity.
- Add a tie-break case (e.g., 22.5 between 15 and 30) to assert deterministic behavior (lower/upper preference).
- it('should find nearest valid value on the higher side', () => { + it('should pick nearest valid value (37 -> 30)', () => { ... - it('should find nearest valid value on the lower side', () => { + it('should pick nearest valid value (38 -> 45)', () => {
209-233: Make error assertions resilient to message changesAssert using regex/partial match rather than full string to reduce brittleness.
- }).toThrow('Invalid combination: segmentType period with value type number'); + }).toThrow(/Invalid combination: segmentType period\b/i); ... - }).toThrow('Invalid combination: segmentType hours with value type string'); + }).toThrow(/Invalid combination: segmentType hours\b/i);src/app-components/TimePicker/TimePicker.responsive.test.tsx (5)
15-28: Restore getComputedStyle after tests to avoid cross-test pollutionCapture original and restore in afterAll.
beforeAll(() => { + // Save original + const original = window.getComputedStyle; // Mock getComputedStyle to avoid JSDOM errors with Popover Object.defineProperty(window, 'getComputedStyle', { value: () => ({ getPropertyValue: () => '', position: 'absolute', top: '0px', left: '0px', width: '300px', height: '200px', }), writable: true, }); + // Stash for restore + (window as any).__origGetComputedStyle = original; }); + + afterAll(() => { + if ((window as any).__origGetComputedStyle) { + Object.defineProperty(window, 'getComputedStyle', { + value: (window as any).__origGetComputedStyle, + writable: true, + }); + } + });
42-57: Avoid brittle className selectors; target the root by idCSS modules and refactors can break class-based queries. Use the known id.
- const wrapper = screen.getByRole('textbox', { name: /hours/i }).closest('.calendarInputWrapper'); + const wrapper = document.getElementById('test-timepicker'); expect(wrapper).toBeInTheDocument();
42-57: Dispatch resize after changing innerWidthSome components react to resize events; emit it for fidelity.
Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: 205, }); + window.dispatchEvent(new Event('resize'));Repeat for the 348px and “long format at small widths” cases.
Also applies to: 59-76, 77-100
162-168: Clarify test titleThis test checks the trigger button accessibility, not the dialog.
- it('should have accessible dropdown dialog', () => { + it('should have an accessible dropdown trigger', () => {
169-179: Await dialog open and assert aria-expanded statePortaled content can render async; also verify the trigger’s expanded state.
const clockButton = screen.getByRole('button', { name: /open time picker/i }); - await user.click(clockButton); - - const dropdown = screen.getByRole('dialog'); + await user.click(clockButton); + expect(clockButton).toHaveAttribute('aria-expanded', 'true'); + + const dropdown = await screen.findByRole('dialog'); expect(dropdown).toHaveAttribute('aria-modal', 'true'); expect(dropdown).toHaveAttribute('role', 'dialog');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
src/app-components/TimePicker/TimePicker.focus.test.tsx(1 hunks)src/app-components/TimePicker/TimePicker.module.css(1 hunks)src/app-components/TimePicker/TimePicker.responsive.test.tsx(1 hunks)src/app-components/TimePicker/TimePicker.tsx(1 hunks)src/app-components/TimePicker/TimeSegment/TimeSegment.test.tsx(1 hunks)src/app-components/TimePicker/TimeSegment/TimeSegment.tsx(1 hunks)src/app-components/TimePicker/TimeSegment/hooks/useSegmentDisplay.ts(1 hunks)src/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.ts(1 hunks)src/app-components/TimePicker/TimeSegment/hooks/useTimeout.ts(1 hunks)src/app-components/TimePicker/TimeSegment/hooks/useTypingBuffer.ts(1 hunks)src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.test.ts(1 hunks)src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.ts(1 hunks)src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.test.ts(1 hunks)src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.ts(1 hunks)src/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.test.ts(1 hunks)src/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.ts(1 hunks)src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.ts(1 hunks)src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts(1 hunks)src/app-components/TimePicker/utils/keyboardNavigation.test.ts(1 hunks)src/app-components/TimePicker/utils/keyboardNavigation.ts(1 hunks)src/app-components/TimePicker/utils/segmentTyping.ts(1 hunks)src/app-components/TimePicker/utils/timeConstraintUtils.test.ts(1 hunks)src/app-components/TimePicker/utils/timeConstraintUtils.ts(1 hunks)src/app-components/TimePicker/utils/timeFormatUtils.test.ts(1 hunks)src/app-components/TimePicker/utils/timeFormatUtils.ts(1 hunks)src/layout/TimePicker/TimePickerComponent.tsx(1 hunks)src/layout/TimePicker/useTimePickerValidation.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/layout/TimePicker/TimePickerComponent.tsx
- src/app-components/TimePicker/utils/timeFormatUtils.ts
- src/app-components/TimePicker/utils/keyboardNavigation.ts
- src/app-components/TimePicker/utils/timeConstraintUtils.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganyand unnecessary type casts (as Type) in TypeScript; prefer precise typings and refactor existing casts/anys
For TanStack Query, use objects to manage query keys and functions, and centralize shared options viaqueryOptions
Files:
src/app-components/TimePicker/utils/timeConstraintUtils.test.tssrc/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.tssrc/app-components/TimePicker/TimePicker.focus.test.tsxsrc/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.test.tssrc/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.tssrc/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.test.tssrc/app-components/TimePicker/utils/keyboardNavigation.test.tssrc/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.tssrc/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.tssrc/app-components/TimePicker/utils/timeFormatUtils.test.tssrc/app-components/TimePicker/TimeSegment/hooks/useSegmentDisplay.tssrc/app-components/TimePicker/TimePicker.tsxsrc/app-components/TimePicker/TimePicker.responsive.test.tsxsrc/app-components/TimePicker/TimeSegment/TimeSegment.test.tsxsrc/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.tssrc/app-components/TimePicker/TimeSegment/TimeSegment.tsxsrc/app-components/TimePicker/TimeSegment/hooks/useTimeout.tssrc/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.tssrc/app-components/TimePicker/TimeSegment/hooks/useTypingBuffer.tssrc/layout/TimePicker/useTimePickerValidation.tssrc/app-components/TimePicker/utils/segmentTyping.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
In tests, use
renderWithProvidersfromsrc/test/renderWithProviders.tsxto supply required form layout context
Files:
src/app-components/TimePicker/utils/timeConstraintUtils.test.tssrc/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.tssrc/app-components/TimePicker/TimePicker.focus.test.tsxsrc/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.test.tssrc/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.test.tssrc/app-components/TimePicker/utils/keyboardNavigation.test.tssrc/app-components/TimePicker/utils/timeFormatUtils.test.tssrc/app-components/TimePicker/TimePicker.responsive.test.tsxsrc/app-components/TimePicker/TimeSegment/TimeSegment.test.tsx
**/*.module.css
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and follow existing patterns in
*.module.cssfiles
Files:
src/app-components/TimePicker/TimePicker.module.css
🧬 Code graph analysis (17)
src/app-components/TimePicker/utils/timeConstraintUtils.test.ts (1)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (7)
TimeValue(3-8)TimeConstraints(10-13)SegmentConstraints(15-19)parseTimeString(21-61)isTimeInRange(63-85)getSegmentConstraints(87-194)getNextValidValue(196-222)
src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.ts (2)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (2)
TimeValue(3-8)SegmentConstraints(15-19)src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts (2)
SegmentConstraints(3-7)handleSegmentValueChange(119-138)
src/app-components/TimePicker/TimePicker.focus.test.tsx (1)
src/app-components/TimePicker/TimePicker.tsx (1)
TimePicker(44-691)
src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.test.ts (1)
src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.ts (1)
formatDisplayHour(7-22)
src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.test.ts (1)
src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.ts (3)
DropdownFocusState(1-5)NavigationAction(7-13)calculateNextFocusState(23-80)
src/app-components/TimePicker/utils/keyboardNavigation.test.ts (1)
src/app-components/TimePicker/utils/keyboardNavigation.ts (5)
SegmentType(4-4)handleSegmentKeyDown(14-58)getNextSegmentIndex(60-72)handleValueIncrement(74-110)handleValueDecrement(112-148)
src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts (2)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)
src/app-components/TimePicker/utils/timeFormatUtils.test.ts (2)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
TimeValue(3-8)src/app-components/TimePicker/utils/timeFormatUtils.ts (4)
formatTimeValue(5-26)formatSegmentValue(28-49)parseSegmentInput(51-75)isValidSegmentInput(77-117)
src/app-components/TimePicker/TimeSegment/hooks/useSegmentDisplay.ts (3)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/TimePicker.tsx (1)
TimeFormat(25-25)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatSegmentValue(28-49)
src/app-components/TimePicker/TimePicker.tsx (8)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (4)
parseTimeString(21-61)TimeConstraints(10-13)TimeValue(3-8)getSegmentConstraints(87-194)src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.ts (3)
DropdownFocusState(1-5)NavigationAction(7-13)calculateNextFocusState(23-80)src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts (2)
SegmentType(11-11)handleSegmentValueChange(119-138)src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/utils/timeFormatUtils.ts (1)
formatTimeValue(5-26)src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.ts (1)
formatDisplayHour(7-22)src/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.ts (3)
generateHourOptions(11-23)generateMinuteOptions(30-40)generateSecondOptions(47-57)src/app-components/TimePicker/TimeSegment/TimeSegment.tsx (1)
TimeSegment(30-157)
src/app-components/TimePicker/TimePicker.responsive.test.tsx (1)
src/app-components/TimePicker/TimePicker.tsx (1)
TimePicker(44-691)
src/app-components/TimePicker/TimeSegment/TimeSegment.test.tsx (1)
src/app-components/TimePicker/TimeSegment/TimeSegment.tsx (2)
TimeSegmentProps(11-28)TimeSegment(30-157)
src/app-components/TimePicker/TimeSegment/TimeSegment.tsx (5)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/TimePicker.tsx (1)
TimeFormat(25-25)src/app-components/TimePicker/TimeSegment/hooks/useSegmentDisplay.ts (1)
useSegmentDisplay(7-28)src/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.ts (1)
useSegmentInputHandlers(27-123)src/app-components/TimePicker/TimeSegment/hooks/useTypingBuffer.ts (1)
useTypingBuffer(11-88)
src/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.ts (3)
src/app-components/TimePicker/utils/keyboardNavigation.ts (4)
SegmentType(4-4)handleValueIncrement(74-110)handleValueDecrement(112-148)handleSegmentKeyDown(14-58)src/app-components/TimePicker/TimePicker.tsx (1)
TimeFormat(25-25)src/app-components/TimePicker/utils/segmentTyping.ts (4)
handleSegmentCharacterInput(179-233)processSegmentBuffer(120-153)commitSegmentValue(166-174)clearSegment(158-161)
src/app-components/TimePicker/TimeSegment/hooks/useTypingBuffer.ts (1)
src/app-components/TimePicker/TimeSegment/hooks/useTimeout.ts (1)
useTimeout(3-19)
src/layout/TimePicker/useTimePickerValidation.ts (6)
src/app-components/TimePicker/utils/timeConstraintUtils.ts (1)
parseTimeString(21-61)src/app-components/TimePicker/TimePicker.tsx (1)
TimeFormat(25-25)src/features/validation/index.ts (1)
ComponentValidation(151-153)src/utils/layout/hooks.ts (1)
useDataModelBindingsFor(102-112)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/features/formData/FormDataWrite.tsx (1)
FD(684-1097)
src/app-components/TimePicker/utils/segmentTyping.ts (2)
src/app-components/TimePicker/utils/keyboardNavigation.ts (1)
SegmentType(4-4)src/app-components/TimePicker/TimePicker.tsx (1)
TimeFormat(25-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Type-checks, eslint, unit tests and SonarCloud
- GitHub Check: Install
🔇 Additional comments (18)
src/layout/TimePicker/useTimePickerValidation.ts (1)
79-105: Confirm intended timezone handling for ISO timestampsparseISO(...) + date.getHours()/getMinutes()/getSeconds() returns the timestamp converted to the runtime/local timezone; the codebase stores/works with ISO timestamps elsewhere (e.g. Datepicker uses formatISO(..., representation: 'complete') and other components construct new Date(timestamp), so this will render times in the viewer's local TZ. Confirm desired behaviour: display in viewer local TZ (keep current getHours()/getMinutes()), display in UTC (use getUTC*), or preserve the original offset/time as written in the ISO string (parse the time substring from the ISO instead).
File: src/layout/TimePicker/useTimePickerValidation.ts (lines ~79–105).src/app-components/TimePicker/utils/formatDisplayHour/formatDisplayHour.test.ts (1)
1-69: LGTMGood coverage of 12/24-hour boundaries and representative values.
src/app-components/TimePicker/TimeSegment/hooks/useSegmentDisplay.ts (1)
7-28: LGTMState initialization and sync logic are clean and dependency-safe.
src/app-components/TimePicker/utils/generateTimeOptions/generateTimeOptions.ts (1)
1-57: LGTM! Clean and well-documented time option generatorsThe implementation is straightforward and correct:
- Proper 12-hour (1-12) vs 24-hour (0-23) hour generation
- Consistent zero-padding for display values
- Flexible step parameter for minutes/seconds with appropriate defaults
- Clear JSDoc documentation
The utility functions provide exactly what the TimePicker dropdown needs for populating time options.
src/app-components/TimePicker/utils/calculateNextFocusState/calculateNextFocusState.test.ts (1)
1-228: Excellent comprehensive test suite for focus navigationThis test suite thoroughly validates the
calculateNextFocusStateutility with comprehensive coverage:
- Inactive state handling: Properly returns unchanged state when not active
- Arrow navigation: Complete coverage of up/down/left/right with proper wrapping
- Cross-column navigation: Correctly handles option index adjustment when moving to columns with fewer options
- Escape/Enter actions: Proper deactivation and state reset
- Edge cases: Empty option counts and single-column navigation
- Boundary testing: Wrapping behavior at min/max values
The test structure is clean, uses descriptive test names, and covers all the navigation scenarios needed for the TimePicker dropdown keyboard navigation.
src/app-components/TimePicker/TimePicker.focus.test.tsx (4)
71-100: Good pragmatic handling of JSDOM limitationsThe test appropriately handles JSDOM limitations with Popover behavior by providing a fallback assertion strategy. The comment clearly explains the reasoning and the test still validates that the focus restoration logic exists.
32-54: Effective focus tracking validationThe test properly validates that keyboard navigation within the dropdown maintains focus within the dialog container, which is crucial for accessibility.
103-153: Comprehensive AM/PM and 12-hour format testingGood coverage of:
- AM/PM button accessibility and interaction in the dropdown
- Full 12-hour format with seconds (4 segments)
- Focus navigation through all segments including the period segment
155-178: Good focus cycling validationThe test correctly validates the expected wrapping behavior for focus navigation between segments using arrow keys, ensuring proper keyboard accessibility.
src/app-components/TimePicker/TimePicker.module.css (2)
179-236: Excellent responsive design implementationThe responsive breakpoints are well-designed:
- 348px breakpoint: Smart flex wrapping and column layout adjustments
- 205px breakpoint: Proper stacking for very small screens with appropriate size constraints
- Dropdown adaptations: Proper width constraints and layout changes for each breakpoint
The responsive behavior should work well across different device sizes.
1-178: Well-structured CSS with good accessibility supportThe styling provides:
- Proper focus-visible outlines for accessibility
- Consistent design system variable usage
- Good hover and interaction states
- Proper scrollbar styling for dropdown lists
- Clean typography and spacing
src/app-components/TimePicker/utils/timeFormatUtils.test.ts (1)
15-217: Excellent comprehensive test coverage for formatting utilitiesThe test suite thoroughly validates all the time formatting utilities with excellent coverage of:
- formatTimeValue: All format variations (24h, 12h, with/without seconds)
- formatSegmentValue: Individual segment formatting with proper zero-padding
- parseSegmentInput: Input parsing with edge cases and validation
- isValidSegmentInput: Comprehensive validation logic for all segment types
- Edge cases: Boundary values, midnight/noon handling, format variations
The tests are well-organized and cover both happy path and edge case scenarios.
src/app-components/TimePicker/utils/timeConstraintUtils.test.ts (1)
26-219: Comprehensive test coverage for constraint utilitiesExcellent test suite with thorough coverage of:
- parseTimeString: All format variations including edge cases like 12 AM/PM
- isTimeInRange: Complete range validation scenarios
- getSegmentConstraints: Complex constraint logic for hours, minutes, and seconds
- getNextValidValue: Navigation logic with boundary conditions
The tests properly validate the constraint and navigation logic that powers the TimePicker's time selection behavior.
src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.test.ts (4)
72-90: Good coverage for 12-hour wrapping logicCases >12→1, <1→12, and pass-through look correct.
92-117: Good coverage for 24-hour wrapping logicWrap above 23 to 0, below 0 to 23, and pass-through verified.
119-143: Minutes wrapping tests are clear and sufficientCovers wrap and valid-range pass-through.
145-169: Seconds wrapping tests look solidMirrors minutes coverage; all good.
src/app-components/TimePicker/TimePicker.responsive.test.tsx (1)
205-239: Readonly behavior assertions look correctInputs have readonly and trigger is disabled when readOnly.
| import { render, screen } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| import { TimePicker } from 'src/app-components/TimePicker/TimePicker'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use renderWithProviders per repo testing guideline
These component tests should use renderWithProviders to supply required layout/context.
-import { render, screen } from '@testing-library/react';
+import { screen } from '@testing-library/react';
+import { renderWithProviders } from 'src/test/renderWithProviders';
...
- render(<TimePicker {...defaultProps} />);
+ renderWithProviders(<TimePicker {...defaultProps} />);Apply the same replacement for all render(...) calls in this file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { render, screen } from '@testing-library/react'; | |
| import userEvent from '@testing-library/user-event'; | |
| import { TimePicker } from 'src/app-components/TimePicker/TimePicker'; | |
| import { screen } from '@testing-library/react'; | |
| import { renderWithProviders } from 'src/test/renderWithProviders'; | |
| import userEvent from '@testing-library/user-event'; | |
| import { TimePicker } from 'src/app-components/TimePicker/TimePicker'; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimePicker.responsive.test.tsx around lines 3
to 6, the tests import and use render from @testing-library/react but per repo
guidelines you must use the project's renderWithProviders test helper; replace
the import of render with the repo's renderWithProviders (import path used
across the repo, e.g. the test utils file) and update every call to render(...)
in this file to renderWithProviders(...), ensuring any wrapper options
previously passed to render are forwarded to renderWithProviders.
| return includesSeconds ? secondOptions : is12Hour ? [{ value: 'AM' }, { value: 'PM' }] : []; | ||
| case 3: | ||
| return is12Hour && includesSeconds ? [{ value: 'AM' }, { value: 'PM' }] : []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex ternary expression reduces readability
The nested ternary operators make this code hard to follow and prone to errors.
- return includesSeconds ? secondOptions : is12Hour ? [{ value: 'AM' }, { value: 'PM' }] : [];
+ if (includesSeconds) {
+ return secondOptions;
+ }
+ return is12Hour ? [{ value: 'AM' }, { value: 'PM' }] : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return includesSeconds ? secondOptions : is12Hour ? [{ value: 'AM' }, { value: 'PM' }] : []; | |
| case 3: | |
| return is12Hour && includesSeconds ? [{ value: 'AM' }, { value: 'PM' }] : []; | |
| if (includesSeconds) { | |
| return secondOptions; | |
| } | |
| return is12Hour ? [{ value: 'AM' }, { value: 'PM' }] : []; | |
| case 3: | |
| return is12Hour && includesSeconds ? [{ value: 'AM' }, { value: 'PM' }] : []; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimePicker.tsx around lines 236 to 238, the
nested ternary expression that returns secondOptions, AM/PM array, or [] is hard
to read and error-prone; replace the nested ternary with an explicit conditional
block (if/else or switch) that clearly handles each case: if includesSeconds
return secondOptions; else if is12Hour return [{ value: 'AM' }, { value: 'PM'
}]; otherwise return []; keep identical return values and preserve short-circuit
logic for the case where both is12Hour and includesSeconds need to return AM/PM
when appropriate.
| constraints.minTime || constraints.maxTime | ||
| ? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes( | ||
| is12Hour | ||
| ? option.value === 12 | ||
| ? timeValue.period === 'AM' | ||
| ? 0 | ||
| : 12 | ||
| : timeValue.period === 'PM' && option.value !== 12 | ||
| ? option.value + 12 | ||
| : option.value | ||
| : option.value, | ||
| ) | ||
| : false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex nested ternary logic for hour validation
This complex nested ternary expression for validating hours is difficult to understand and maintain. Consider extracting it into a helper function.
+ const getActualHourValue = (optionValue: number, is12Hour: boolean, period?: 'AM' | 'PM') => {
+ if (!is12Hour) return optionValue;
+
+ if (optionValue === 12) {
+ return period === 'AM' ? 0 : 12;
+ }
+ return period === 'PM' && optionValue !== 12 ? optionValue + 12 : optionValue;
+ };
+
const isDisabled =
constraints.minTime || constraints.maxTime
? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes(
- is12Hour
- ? option.value === 12
- ? timeValue.period === 'AM'
- ? 0
- : 12
- : timeValue.period === 'PM' && option.value !== 12
- ? option.value + 12
- : option.value
- : option.value,
+ getActualHourValue(option.value, is12Hour, timeValue.period),
)
: false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constraints.minTime || constraints.maxTime | |
| ? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes( | |
| is12Hour | |
| ? option.value === 12 | |
| ? timeValue.period === 'AM' | |
| ? 0 | |
| : 12 | |
| : timeValue.period === 'PM' && option.value !== 12 | |
| ? option.value + 12 | |
| : option.value | |
| : option.value, | |
| ) | |
| : false; | |
| const getActualHourValue = (optionValue: number, is12Hour: boolean, period?: 'AM' | 'PM') => { | |
| if (!is12Hour) return optionValue; | |
| if (optionValue === 12) { | |
| return period === 'AM' ? 0 : 12; | |
| } | |
| return period === 'PM' && optionValue !== 12 ? optionValue + 12 : optionValue; | |
| }; | |
| const isDisabled = | |
| constraints.minTime || constraints.maxTime | |
| ? !getSegmentConstraints('hours', timeValue, constraints, format).validValues.includes( | |
| getActualHourValue(option.value, is12Hour, timeValue.period), | |
| ) | |
| : false; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimePicker.tsx around lines 515 to 527, the
nested ternary that computes the hour value for validation is hard to read and
should be extracted into a small helper; create a function (e.g.,
normalizeHour(optionValue: number, is12Hour: boolean, period: 'AM'|'PM')) that
returns the 0-23 hour number according to 12/24h rules and the current period,
replace the inline nested ternary with a variable assigned by that helper, then
call getSegmentConstraints('hours', timeValue, constraints,
format).validValues.includes(normalizedHour) and use that boolean in the
existing conditional logic so the main expression is short and readable.
| const handleDeleteOrBackspace = useCallback(() => { | ||
| const clearedSegment = clearSegment(); | ||
| onUpdateDisplay(clearedSegment.displayValue); | ||
| }, [onUpdateDisplay]); |
There was a problem hiding this comment.
Clear segment value is not committed
The handleDeleteOrBackspace function only updates the display but doesn't clear the actual value through onValueChange. This could lead to a mismatch between what's displayed and the underlying value.
const handleDeleteOrBackspace = useCallback(() => {
const clearedSegment = clearSegment();
onUpdateDisplay(clearedSegment.displayValue);
+ // For non-period segments, explicitly clear the value
+ if (segmentType !== 'period') {
+ onValueChange(0);
+ }
- }, [onUpdateDisplay]);
+ }, [onUpdateDisplay, onValueChange, segmentType]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDeleteOrBackspace = useCallback(() => { | |
| const clearedSegment = clearSegment(); | |
| onUpdateDisplay(clearedSegment.displayValue); | |
| }, [onUpdateDisplay]); | |
| const handleDeleteOrBackspace = useCallback(() => { | |
| const clearedSegment = clearSegment(); | |
| onUpdateDisplay(clearedSegment.displayValue); | |
| // For non-period segments, explicitly clear the value | |
| if (segmentType !== 'period') { | |
| onValueChange(0); | |
| } | |
| }, [onUpdateDisplay, onValueChange, segmentType]); |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.ts
around lines 78 to 81, handleDeleteOrBackspace only calls onUpdateDisplay with
the cleared segment display but does not commit the cleared value; update the
handler to call onValueChange with the cleared segment actual value (e.g.,
clearedSegment.value) in addition to
onUpdateDisplay(clearedSegment.displayValue), and update the useCallback
dependency array to include onValueChange and clearSegment so the handler always
uses the latest functions/values.
| const valueIsEmpty = | ||
| currentValue === null || currentValue === '' || (typeof currentValue === 'number' && isNaN(currentValue)); | ||
|
|
||
| if (valueIsEmpty && (segmentType === 'minutes' || segmentType === 'seconds')) { | ||
| onValueChange(0); | ||
| } | ||
| }, [currentValue, segmentType, onValueChange]); |
There was a problem hiding this comment.
Fix the function name inconsistency
The function is named fillEmptyMinutesOrSecondsWithZero but the return object exports it as fillEmptyMinutesWithZero. This mismatch could cause confusion.
return {
processCharacterInput,
commitBufferValue,
handleArrowKeyNavigation,
handleDeleteOrBackspace,
- fillEmptyMinutesOrSecondsWithZero,
+ fillEmptyMinutesOrSecondsWithZero,
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimeSegment/hooks/useSegmentInputHandlers.ts
around lines 108 to 114, the internal function is named
fillEmptyMinutesOrSecondsWithZero but the returned/exported property is
fillEmptyMinutesWithZero, causing a name mismatch; fix this by making the names
consistent — either rename the function to fillEmptyMinutesWithZero or update
the returned property to fillEmptyMinutesOrSecondsWithZero and ensure any
callers/imports are updated accordingly so the exported identifier matches the
actual function name.
| ref={ref} | ||
| type='text' | ||
| value={displayValue} | ||
| onChange={() => {}} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use controlled component pattern correctly.
The empty onChange handler violates React's controlled component pattern. This could trigger React warnings about controlled components.
Apply this diff to properly handle the controlled input:
- onChange={() => {}}
+ onChange={() => {
+ // Controlled externally via keyboard events
+ }}Or better yet, make it explicitly read-only since all input is handled via keyboard events:
- onChange={() => {}}
+ readOnly={true}Then remove the readOnly prop from being passed separately since it's always true for this implementation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onChange={() => {}} | |
| readOnly={true} |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimeSegment/TimeSegment.tsx around line 133,
the component currently uses an empty onChange={() => {}} which violates React's
controlled component pattern; remove the no-op onChange and instead mark the
input explicitly readOnly (since keyboard handling is used) and stop passing a
separate readOnly prop into the input; update the component to omit onChange
entirely and set the input to readOnly by default so the control is truly
read-only and no React controlled/uncontrolled warnings occur.
| type='text' | ||
| value={displayValue} | ||
| onChange={() => {}} | ||
| onKeyPress={handleCharacterTyping} |
There was a problem hiding this comment.
onKeyPress is deprecated - use onKeyDown instead.
The onKeyPress event is deprecated in modern browsers and will be removed in future versions. All keyboard handling should use onKeyDown or onKeyUp.
Apply this diff to use the modern event handler:
- onKeyPress={handleCharacterTyping}
- onKeyDown={handleSpecialKeys}
+ onKeyDown={(event) => {
+ handleSpecialKeys(event);
+ if (!event.defaultPrevented) {
+ handleCharacterTyping(event);
+ }
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onKeyPress={handleCharacterTyping} | |
| onKeyDown={(event) => { | |
| handleSpecialKeys(event); | |
| if (!event.defaultPrevented) { | |
| handleCharacterTyping(event); | |
| } | |
| }} |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/TimeSegment/TimeSegment.tsx around line 134,
replace the deprecated onKeyPress usage with onKeyDown: change the JSX prop from
onKeyPress={handleCharacterTyping} to onKeyDown={handleCharacterTyping} and
ensure the handler accepts a KeyboardEvent (React.KeyboardEvent) and handles key
values appropriately (if relying on 'char' behavior, use event.key and
event.preventDefault()/stopPropagation() as needed); update any type annotations
or tests referencing onKeyPress accordingly.
| export interface SegmentConstraints { | ||
| min: number; | ||
| max: number; | ||
| validValues: number[]; | ||
| } | ||
|
|
||
| export type NumericSegmentType = 'hours' | 'minutes' | 'seconds'; | ||
| export type PeriodSegmentType = 'period'; | ||
| export type SegmentType = NumericSegmentType | PeriodSegmentType; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate type definitions
The SegmentType is already defined in src/app-components/TimePicker/utils/keyboardNavigation.ts. Having duplicate type definitions could lead to maintenance issues.
+import type { SegmentType } from 'src/app-components/TimePicker/utils/keyboardNavigation';
import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils';
export interface SegmentConstraints {
min: number;
max: number;
validValues: number[];
}
-export type NumericSegmentType = 'hours' | 'minutes' | 'seconds';
-export type PeriodSegmentType = 'period';
-export type SegmentType = NumericSegmentType | PeriodSegmentType;
+export type NumericSegmentType = Extract<SegmentType, 'hours' | 'minutes' | 'seconds'>;
+export type PeriodSegmentType = Extract<SegmentType, 'period'>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SegmentConstraints { | |
| min: number; | |
| max: number; | |
| validValues: number[]; | |
| } | |
| export type NumericSegmentType = 'hours' | 'minutes' | 'seconds'; | |
| export type PeriodSegmentType = 'period'; | |
| export type SegmentType = NumericSegmentType | PeriodSegmentType; | |
| import type { SegmentType } from 'src/app-components/TimePicker/utils/keyboardNavigation'; | |
| import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; | |
| export interface SegmentConstraints { | |
| min: number; | |
| max: number; | |
| validValues: number[]; | |
| } | |
| export type NumericSegmentType = Extract<SegmentType, 'hours' | 'minutes' | 'seconds'>; | |
| export type PeriodSegmentType = Extract<SegmentType, 'period'>; |
🤖 Prompt for AI Agents
In
src/app-components/TimePicker/utils/handleSegmentValueChange/handleSegmentValueChange.ts
around lines 3 to 11, remove the duplicate type declarations
(NumericSegmentType, PeriodSegmentType, and SegmentType) and instead import the
existing SegmentType from
src/app-components/TimePicker/utils/keyboardNavigation.ts; keep the
SegmentConstraints interface in this file, update any exports/imports
accordingly to use the centralized SegmentType definition, and ensure the import
uses a correct relative path and is a type-only import if your build supports
it.
| // Handle period segment | ||
| if (segmentType === 'period') { | ||
| const newPeriod = processPeriodInput(char, 'AM'); |
There was a problem hiding this comment.
Period input logic always returns 'AM' regardless of input.
The processPeriodInput function is called with a hardcoded 'AM' value as the current period, which means it cannot properly toggle between AM/PM or preserve the current state when an invalid key is pressed.
Apply this diff to properly handle period input:
// Handle period segment
if (segmentType === 'period') {
- const newPeriod = processPeriodInput(char, 'AM');
+ // Current period should come from currentBuffer or default to current value
+ const currentPeriod = (currentBuffer === 'AM' || currentBuffer === 'PM') ? currentBuffer as 'AM' | 'PM' : 'AM';
+ const newPeriod = processPeriodInput(char, currentPeriod);
return {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle period segment | |
| if (segmentType === 'period') { | |
| const newPeriod = processPeriodInput(char, 'AM'); | |
| // Handle period segment | |
| if (segmentType === 'period') { | |
| // Current period should come from currentBuffer or default to current value | |
| const currentPeriod = (currentBuffer === 'AM' || currentBuffer === 'PM') ? currentBuffer as 'AM' | 'PM' : 'AM'; | |
| const newPeriod = processPeriodInput(char, currentPeriod); | |
| return { |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/utils/segmentTyping.ts around lines 200 to 202,
the period handling currently calls processPeriodInput with a hardcoded 'AM',
preventing toggling or preserving the current period; change the call to pass
the current period (e.g., segment.value || existingPeriod || 'AM') instead of
the hardcoded string, then use the returned value to update the segment only
when valid (preserve the original on invalid input); also ensure the input is
normalized (case-insensitive) before passing to processPeriodInput.
| interface TimeValue { | ||
| hours: number; | ||
| minutes: number; | ||
| seconds: number; | ||
| period: 'AM' | 'PM'; | ||
| } |
There was a problem hiding this comment.
Fix interface inconsistency with source code
The local TimeValue interface defines period as required (period: 'AM' | 'PM'), but the actual interface from timeConstraintUtils.ts defines it as optional (period?: 'AM' | 'PM'). This mismatch could cause type issues and test failures.
interface TimeValue {
hours: number;
minutes: number;
seconds: number;
- period: 'AM' | 'PM';
+ period?: 'AM' | 'PM';
}Alternatively, import the interface directly from the source file:
-interface TimeValue {
- hours: number;
- minutes: number;
- seconds: number;
- period: 'AM' | 'PM';
-}
+import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface TimeValue { | |
| hours: number; | |
| minutes: number; | |
| seconds: number; | |
| period: 'AM' | 'PM'; | |
| } | |
| interface TimeValue { | |
| hours: number; | |
| minutes: number; | |
| seconds: number; | |
| period?: 'AM' | 'PM'; | |
| } |
| interface TimeValue { | |
| hours: number; | |
| minutes: number; | |
| seconds: number; | |
| period: 'AM' | 'PM'; | |
| } | |
| import type { TimeValue } from 'src/app-components/TimePicker/utils/timeConstraintUtils'; |
🤖 Prompt for AI Agents
In src/app-components/TimePicker/utils/timeFormatUtils.test.ts around lines 8 to
13, the local TimeValue interface declares period as required but the real
interface in timeConstraintUtils.ts marks period optional; update the test to
match the source by making period optional (period?: 'AM' | 'PM') or, better,
remove the local interface and import the TimeValue type directly from the
source file so the test always stays consistent with the implementation.
This review is still in draft, just changed status to try out code rabbit :)
Description
Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit