Time Picker Block #847

[alaleung]

Description

Time Picker Block
#847

[github-actions]

@CodiumAI-Agent /describe

[CodiumAI-Agent]

Title

Time Picker Block #847

---

User description

Description

Time Picker Block
#847

---

PR Type

Tests

---

Description

• Add tests for TimePickerBlock rendering

• Verify initial and value-based displays

• Simulate user interactions in time picker

• Assert selected time updates correctly

---

Changes walkthrough 📝

	Relevant files
Tests	TimePickerBlock.spec.tsx New TimePickerBlock spec tests                                                      libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx Created spec file for TimePickerBlock Added render test with mocked provider Added value rendering and interaction test Assert hours, minutes, AM/PM selection +130/-0

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[github-actions]

@CodiumAI-Agent /review

[github-actions]

@CodiumAI-Agent /improve

[CodiumAI-Agent]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 847 - Partially compliant Compliant requirements: Test default rendering of TimePickerBlock with no initial value Test rendering of TimePickerBlock with a preset time value Test interactive selection of hour, minute, and AM/PM in the picker dialog Test presence of dialog role and OK button Non-compliant requirements: Test clearable functionality (clear button behavior) Test placeholder text behavior Requires further human verification: Verify that the test assertions match the actual UI markup and localization settings

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Value Assertion Method Using inputElement.querySelector("[value='...']") to check the input’s value is unreliable; prefer inputElement.value or testing-library queries like toHaveValue. expect(inputElement.querySelector("[value='']")).toBeNull(); expect(screen.getByText("Select Time")).toBeInTheDocument(); }); it("renders time value correctly", async () => { const { container } = render(<TimePickerBlock id="time-picker2" />, { blocks: blocks, }); const element = container.querySelector("[data-block='time-picker2']"); const inputElement = container.querySelector("input"); const buttonElement = container.querySelector("button"); expect(element).toBeInTheDocument(); expect(inputElement).toBeInTheDocument(); expect(inputElement.querySelector("[value='09:25 am']")).toBeNull(); Incorrect Matcher The tests use .equals on expectations, which is not a Vitest matcher. Use .toEqual, .toBe, or another built-in matcher. expect(textElements.length).equals(2); } else { expect(screen.getByText(hourValue)).toBeInTheDocument(); } const minuteValue = (i - 1) * 5; if (i > 3) { expect(screen.getByText(minuteValue)).toBeInTheDocument(); } } expect(screen.getByText("00")).toBeInTheDocument(); expect(screen.getByText("AM")).toBeInTheDocument(); expect(screen.getByText("PM")).toBeInTheDocument(); expect(screen.getByText("OK")).toBeInTheDocument(); let selectedElements = timePickerElement.querySelectorAll( "[aria-selected='true']", ); expect(selectedElements[0].textContent).equal("09"); expect(selectedElements[1].textContent).equal("25"); expect(selectedElements[2].textContent).equal("AM"); Brittle Loop Logic The hour/minute selection loop makes hardcoded assumptions about duplicate entries and visibility that may break if the UI structure or localization changes. for (let i = 1; i < 13; i++) { const hourValue = i < 10 ? `0${i}` : `${i}`; if (i === 5 || i === 10) { const textElements = screen.getAllByText(hourValue); expect(textElements).not.toBeNull(); expect(textElements.length).equals(2); } else { expect(screen.getByText(hourValue)).toBeInTheDocument(); } const minuteValue = (i - 1) * 5; if (i > 3) { expect(screen.getByText(minuteValue)).toBeInTheDocument(); } }

[CodiumAI-Agent]

PR Code Suggestions ✨

Latest suggestions up to e977980

Category	Suggestion	Impact
General	Assert input shows correct time To verify the rendered time value, assert the input’s value property directly with the proper matcher instead of querying a child element. This ensures correct checking of the displayed time. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [90] -expect(inputElement.querySelector("[value='09:25 am']")).toBeNull(); +expect(inputElement).toHaveValue('09:25 am'); Suggestion importance[1-10]: 8 __ Why: This fix corrects the test by asserting the actual displayed time with toHaveValue rather than expecting a null selector, improving test accuracy.	Medium
	Use toHaveValue for empty input Instead of querying a child element for the input’s value, use the dedicated Jest DOM matcher to assert the input’s value directly. This makes the intent clearer and avoids misusing querySelector on an input element. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [75] -expect(inputElement.querySelector("[value='']")).toBeNull(); +expect(inputElement).toHaveValue(''); Suggestion importance[1-10]: 6 __ Why: Using toHaveValue directly makes the intent clearer and correctly asserts the input’s value instead of misusing querySelector.	Low
Possible issue	Replace .equals with .toBe The Vitest/Jest expect API does not include .equals; replace it with .toBe for strict equality assertions. This fixes matcher errors and clarifies the expectation. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [100] -expect(textElements.length).equals(2); +expect(textElements.length).toBe(2); Suggestion importance[1-10]: 7 __ Why: Changing .equals to .toBe uses a valid Jest/Vitest matcher and prevents test failures due to unsupported methods.	Medium

---

Previous suggestions

Suggestions up to commit 675b5a6

Category	Suggestion	Impact
Possible issue	Fix array length assertion The .equals matcher is not valid in Vitest/Jest. Use .toHaveLength(2) to accurately assert the number of elements in the array. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [100] -expect(textElements.length).equals(2); +expect(textElements).toHaveLength(2); Suggestion importance[1-10]: 8 __ Why: Replacing the invalid .equals matcher with toHaveLength(2) uses the correct Vitest/Jest API and fixes a potential test failure.	Medium
	Correct string equality assertion Replace the invalid .equal matcher with .toBe("09") for a correct strict equality comparison of the text content. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [116] -expect(selectedElements[0].textContent).equal("09"); +expect(selectedElements[0].textContent).toBe("09"); Suggestion importance[1-10]: 8 __ Why: The .equal matcher isn’t valid in Vitest; using toBe("09") ensures a proper strict equality check on the text content.	Medium
General	Use proper value assertion Use jest-dom's toHaveValue matcher to assert the input's value instead of querying for a child element. This makes the test clearer and correctly checks the input's value. libs/renderer/src/testing/block-defaults/TimePickerBlock.spec.tsx [75] -expect(inputElement.querySelector("[value='']")).toBeNull(); +expect(inputElement).toHaveValue(''); Suggestion importance[1-10]: 6 __ Why: Using toHaveValue('') with inputElement makes the test clearer and directly checks the input’s value instead of querying a child element.	Low

[memisrose]

issue rendering the component

[alaleung]

issue rendering the component

fixed

[memisrose]