Skip to content

refactor(web): extract DatePickerWithRange component from assignment form#324

Open
fvcci wants to merge 1 commit intofeat_web_replace_datetime_inputs_with_calendar_component_for_assignment_datesfrom
refactor_web_extract_DatePickerWithRange_component_from_assignment_form
Open

refactor(web): extract DatePickerWithRange component from assignment form#324
fvcci wants to merge 1 commit intofeat_web_replace_datetime_inputs_with_calendar_component_for_assignment_datesfrom
refactor_web_extract_DatePickerWithRange_component_from_assignment_form

Conversation

@fvcci
Copy link
Copy Markdown
Contributor

@fvcci fvcci commented Mar 25, 2026

TL;DR

Extracted date picker functionality into a reusable component with comprehensive test coverage.

What changed?

  • Created a new DatePickerWithRange component that encapsulates calendar selection and time input functionality
  • Moved date/time utility functions (formatTime, defaultDateRange, updateDateRange, updateDateRangeTime) to the new component file
  • Updated the new assignment page to use the extracted component instead of inline calendar and time input elements
  • Modified defaultDateRange to set the end date 7 days after the start date instead of the same day
  • Simplified time input labels from "Availability Start Time / Release Time" to "Start Time" and "Availability End Time / Due Time" to "End Time"
  • Added comprehensive unit tests covering all utility functions with 258 test cases

How to test?

  1. Navigate to the teacher assignment creation page (/teacher/classroom/[id]/assignment/new)
  2. Verify the date picker displays a calendar with range selection
  3. Test that time inputs update correctly when changed
  4. Confirm that selecting new dates preserves the previously set times
  5. Run the test suite to verify all utility functions work as expected

Why make this change?

This refactoring improves code reusability by extracting the date picker logic into a standalone component that can be used across different parts of the application. The comprehensive test coverage ensures the date/time manipulation functions work correctly and prevents regressions. The simplified labels make the interface cleaner and more user-friendly.

Copy link
Copy Markdown
Contributor Author

fvcci commented Mar 25, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR extracts date/time picker logic from the new-assignment page into a standalone DatePickerWithRange component, improves defaultDateRange to span 7 days, and adds thorough unit tests for all utility functions — a solid refactor overall.

Issues found:

  • Missing issue link: The PR description does not include resolves #issue-id, which is required by the project template.
  • Component + integration in one PR: The new component is created and integrated into page.tsx in the same PR. Per project convention, components should be developed in isolation and integrated in a separate PR.
  • Logic bug in updateDateRange: dateRange.from?.getMinutes() and dateRange.to?.getMinutes() can resolve to undefined when those date fields are absent. Passing undefined explicitly to setHours (rather than omitting the argument) converts it to NaN and produces an Invalid Date. The to case is a realistic path: selecting a start date sets to = undefined, so a subsequent range selection would corrupt the new to date.
  • Unused label prop: Declared in the props interface but never used inside the component.
  • Relative import in test file: ./date-picker-with-range should be ~/components/date-picker-with-range to match the project's absolute-import convention.
  • Misleading test description: "should return empty DateRange when newDateRange is undefined" describes the old behaviour; the implementation (and assertions) now correctly return the original dateRange.

Confidence Score: 3/5

  • Not yet safe to merge — the setHours(undefined) bug can produce Invalid Dates on a realistic user interaction, and two process rules (issue link, component isolation) are violated.
  • The refactor is well-structured and the test coverage is commendable, but the getMinutes()undefined → Invalid Date bug is a concrete logic defect that can be triggered in normal usage (select a start date, then complete the range). Combined with the missing issue link and the policy of not integrating a component into a page in the same PR, a targeted fix pass is needed before merging.
  • apps/nextjs/src/components/date-picker-with-range.tsx — fix the setHours minutes fallback and remove the unused label prop.

Important Files Changed

Filename Overview
apps/nextjs/src/components/date-picker-with-range.tsx New reusable component extracting calendar + time inputs; contains an unused label prop and a logic bug where getMinutes() can return undefined and corrupt a date via setHours.
apps/nextjs/src/components/date-picker-with-range.test.tsx Comprehensive tests for all utility functions; uses a relative import path instead of the required ~/ alias, and one test description ("should return empty DateRange") is misleading.
apps/nextjs/src/app/teacher/classroom/[id]/assignment/new/page.tsx Assignment page correctly adopts the new DatePickerWithRange component and removes the leftover console.log; also removes the now-unused Calendar import cleanly.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/nextjs/src/components/date-picker-with-range.tsx
Line: 18

Comment:
**Unused `label` prop in type definition**

The `label?: string` prop is declared in the props interface but is never read or rendered inside the component. This dead code should be removed to keep the API clean.

```suggestion
  required?: boolean;
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/nextjs/src/components/date-picker-with-range.tsx
Line: 93-96

Comment:
**`undefined` passed to `setHours` minutes parameter makes date invalid**

`dateRange.from?.getMinutes()` evaluates to `undefined` when `dateRange.from` is `undefined`. Passing `undefined` explicitly (not omitting the argument) causes JavaScript to convert it to `NaN`, producing an Invalid Date. The same issue exists on lines 103–104 for `dateRange.to?.getMinutes()`, which is a more realistic code path: when a user clicks a start date first (which sets `from` but leaves `to = undefined`) and then clicks an end date, `newDateRange.to` is defined but `dateRange.to` is still `undefined`, so `newTo.setHours(0, undefined, 59, 999)` will corrupt the date.

Fix by using `?? 0` as a fallback, consistent with how `getHours()` is already handled:

```suggestion
    newFrom.setHours(
      dateRange.from?.getHours() ?? 0,
      dateRange.from?.getMinutes() ?? 0,
    );
```

And on lines 103–104:
```
    newTo.setHours(
      dateRange.to?.getHours() ?? 0,
      dateRange.to?.getMinutes() ?? 0,
      59,
      999,
    );
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/nextjs/src/components/date-picker-with-range.test.tsx
Line: 8

Comment:
**Relative import instead of absolute path**

The project convention requires absolute imports starting with `~/`. This relative import should use the configured alias.

```suggestion
} from "~/components/date-picker-with-range";
```

**Rule Used:** Always use absolute paths (starting with '~/' or s... ([source](https://app.greptile.com/review/custom-context?memory=b2963068-1d68-42ee-accd-2fab5b14abcd))

**Learnt From**
[deltahacks/landing-12#5](https://github.com/deltahacks/landing-12/pull/5)

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/nextjs/src/components/date-picker-with-range.test.tsx
Line: 131-133

Comment:
**Misleading test description**

The description "should return empty DateRange when newDateRange is undefined" is inaccurate — the implementation (and the assertions below) actually return the *original* `dateRange`, not an empty one. The description should be corrected to reflect the real behaviour.

```suggestion
    it("should return the original dateRange when newDateRange is undefined", () => {
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "refactor(web): extract DatePickerWithRan..." | Re-trigger Greptile

@fvcci fvcci force-pushed the feat_web_replace_datetime_inputs_with_calendar_component_for_assignment_dates branch from 17e7fec to f53098e Compare March 25, 2026 06:24
@fvcci fvcci force-pushed the refactor_web_extract_DatePickerWithRange_component_from_assignment_form branch 2 times, most recently from e68763d to 799d2df Compare March 25, 2026 06:26
@fvcci fvcci force-pushed the feat_web_replace_datetime_inputs_with_calendar_component_for_assignment_dates branch from f53098e to 58944e3 Compare March 25, 2026 06:26
@fvcci fvcci force-pushed the refactor_web_extract_DatePickerWithRange_component_from_assignment_form branch from 799d2df to ae75e40 Compare March 25, 2026 20:20
@fvcci fvcci force-pushed the feat_web_replace_datetime_inputs_with_calendar_component_for_assignment_dates branch from 58944e3 to 00327a6 Compare March 25, 2026 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant