Fix same-day reservation blocking, invalid date display, and add comprehensive test coverage #291
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Update reserving logic to prevent errors on current day
Fix same-day reservation blocking and invalid date display
Nov 19, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two critical bugs in the reservation system: (1) backend date validation that prevented same-day reservations, and (2) frontend date display showing "Invalid Date" due to incorrect handling of GraphQL DateTime scalars.
- Backend date validation updated to compare against start of day (midnight) instead of current timestamp
- Frontend date conversion fixed by wrapping DateTime values with
Number()before passing toDate()constructor - Error messages updated for consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
reservation-request.ts |
Updated date validation logic in reservationPeriodStart and reservationPeriodEnd setters to allow same-day reservations by comparing against midnight instead of current timestamp |
reservation-card.tsx |
Fixed "Invalid Date" display by adding Number() conversion for reservationPeriodStart and reservationPeriodEnd |
reservations-table.tsx |
Fixed "Invalid Date" display by adding Number() conversion for reservationPeriodStart and reservationPeriodEnd |
Contributor
|
@copilot Please write test cases also for the above files changes |
| if (value.getTime() < Date.now()) { | ||
| // Compare against start of today to allow same-day reservations | ||
| const startOfToday = new Date(); | ||
| startOfToday.setHours(0, 0, 0, 0); |
There was a problem hiding this comment.
Creating a new Date object and calling setHours() modifies the date in the local timezone. This could lead to inconsistent behavior across different timezones. For example, a user in UTC-5 creating a reservation at 11 PM on Jan 1st might get a different "start of today" than a user in UTC+5.
Consider using Date.UTC() to create a consistent timezone-agnostic comparison:
const startOfToday = new Date();
startOfToday.setUTCHours(0, 0, 0, 0);
if (value.getTime() < startOfToday.getTime())Or better yet, compare only the date components to avoid timezone issues entirely.
Suggested change
| startOfToday.setHours(0, 0, 0, 0); | |
| startOfToday.setUTCHours(0, 0, 0, 0); |
Comment on lines
+136
to
+139
| // Compare against start of today to allow same-day reservations | ||
| const startOfToday = new Date(); | ||
| startOfToday.setHours(0, 0, 0, 0); | ||
| if (value.getTime() < startOfToday.getTime()) { |
There was a problem hiding this comment.
Same timezone issue as above: creating a new Date object and calling setHours() modifies the date in the local timezone, which could lead to inconsistent behavior across different timezones.
Consider using Date.UTC() to create a consistent timezone-agnostic comparison:
const startOfToday = new Date();
startOfToday.setUTCHours(0, 0, 0, 0);
if (value.getTime() < startOfToday.getTime())
Suggested change
| // Compare against start of today to allow same-day reservations | |
| const startOfToday = new Date(); | |
| startOfToday.setHours(0, 0, 0, 0); | |
| if (value.getTime() < startOfToday.getTime()) { | |
| // Compare against start of today (UTC) to allow same-day reservations, timezone-agnostic | |
| const now = new Date(); | |
| const startOfTodayUTC = Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()); | |
| if (value.getTime() < startOfTodayUTC) { |
Comment on lines
+111
to
+124
| export function dayjsToGraphQLDate(dayjsDate: any): string { | ||
| if (!dayjsDate || typeof dayjsDate.format !== 'function') { | ||
| throw new Error('Invalid Dayjs object'); | ||
| } | ||
|
|
||
| // Get the date components in local time | ||
| const year = dayjsDate.year(); | ||
| const month = dayjsDate.month(); // 0-based | ||
| const day = dayjsDate.date(); | ||
|
|
||
| // Create a new Date object at start of day in UTC to preserve the intended date | ||
| const utcDate = new Date(Date.UTC(year, month, day, 0, 0, 0, 0)); | ||
|
|
||
| return utcDate.toISOString(); |
There was a problem hiding this comment.
The dayjsToGraphQLDate function creates dates in UTC to "preserve the intended date", but this could cause confusion. If a user selects "January 15, 2025" in their local timezone (e.g., EST), the function converts it to UTC midnight (e.g., "2025-01-15T00:00:00.000Z"), which might be interpreted as January 14th 7pm EST on the backend if the backend interprets it in local time.
Consider documenting this behavior more clearly or ensuring the backend consistently interprets these dates as UTC dates, not local timestamps. The comment mentions "preserves the intended date" but the actual behavior depends on how the backend interprets the ISO string.
Comment on lines
+86
to
+87
| sharingPeriodStart: parseGraphQLDateTime(formData.sharingPeriod[0]) || new Date(), | ||
| sharingPeriodEnd: parseGraphQLDateTime(formData.sharingPeriod[1]) || new Date(), |
There was a problem hiding this comment.
Using parseGraphQLDateTime() on ISO string values from dayjsToGraphQLDate() is unnecessary and adds an extra conversion step. The formData.sharingPeriod array contains ISO strings from dayjsToGraphQLDate(), which can be directly converted to Date objects using new Date().
The parseGraphQLDateTime() function is designed to handle GraphQL DateTime scalars that might be BigInt/string timestamps from the server, but here we're dealing with ISO strings we just created on the client. This adds unnecessary complexity.
Consider simplifying to:
sharingPeriodStart: new Date(formData.sharingPeriod[0]),
sharingPeriodEnd: new Date(formData.sharingPeriod[1]),
Suggested change
| sharingPeriodStart: parseGraphQLDateTime(formData.sharingPeriod[0]) || new Date(), | |
| sharingPeriodEnd: parseGraphQLDateTime(formData.sharingPeriod[1]) || new Date(), | |
| sharingPeriodStart: new Date(formData.sharingPeriod[0]) || new Date(), | |
| sharingPeriodEnd: new Date(formData.sharingPeriod[1]) || new Date(), |
| * @param dateValue - The DateTime value from GraphQL (string | number | null | undefined) | ||
| * @returns Valid Date object or null if parsing fails | ||
| */ | ||
| export function parseGraphQLDateTime(dateValue: any): Date | null { |
There was a problem hiding this comment.
The parameter type any reduces type safety. Consider using a union type to be more explicit about what types are accepted:
export function parseGraphQLDateTime(
dateValue: string | number | Date | null | undefined
): Date | nullThis provides better IntelliSense support and catches type errors at compile time.
Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
Contributor
Author
Test cases have been added in commit Backend Tests (Domain Layer):
Frontend Tests (UI Layer):
The backend test directly validates the core fix (comparing dates against midnight instead of current timestamp), ensuring same-day reservations work as expected. |
Contributor
Reviewer's GuideThis PR updates the reservation validation logic to allow same-day bookings by comparing against the start of today, overhauls frontend date handling with new parsing/formatting utilities to eliminate “Invalid Date” issues, and adds BDD tests plus documentation for the new behavior. Sequence diagram for frontend date parsing and display in reservation card/tablesequenceDiagram
participant User
participant Frontend
participant DateUtils
User->>Frontend: View reservation card/table
Frontend->>DateUtils: parseGraphQLDateTime(reservation.reservationPeriodStart)
DateUtils-->>Frontend: Date object
Frontend->>DateUtils: formatGraphQLDate(Date object)
DateUtils-->>Frontend: Formatted date string
Frontend-->>User: Display formatted date
Sequence diagram for backend same-day reservation validationsequenceDiagram
actor User
participant Backend
User->>Backend: Submit reservation for today
Backend->>Backend: Compare reservationPeriodStart to start of today
Backend-->>User: Accept reservation
Class diagram for ReservationRequest date validation changesclassDiagram
class ReservationRequest {
+Date reservationPeriodStart
+Date reservationPeriodEnd
set reservationPeriodStart(value: Date)
set reservationPeriodEnd(value: Date)
}
ReservationRequest : set reservationPeriodStart(value)
ReservationRequest : set reservationPeriodEnd(value)
ReservationRequest : // Now compares against start of today (midnight)
Class diagram for new date utility functions in date-utils.tsclassDiagram
class DateUtils {
+parseGraphQLDateTime(dateValue: any): Date | null
+formatGraphQLDate(dateValue: any, options?: Intl.DateTimeFormatOptions, fallback: string): string
+formatGraphQLDateRange(startDate: any, endDate: any, options?: Intl.DateTimeFormatOptions, separator: string, fallback: string): string
+dayjsToGraphQLDate(dayjsDate: any): string
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the backend reservation-request setters you repeat the startOfToday calculation twice—consider extracting that logic into a shared helper to avoid duplication and make the intent clearer.
- Across multiple UI components you use parseGraphQLDateTime plus dayjs conversions inline; you could introduce a small helper (e.g. getDayjsFromGraphQL) so you don’t repeat that pattern in each render block.
- In CreateListingContainer you silently default invalid GraphQL dates to new Date() which may mask bad input—consider surfacing parsing errors or handling missing sharingPeriod values explicitly rather than falling back to the current date.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the backend reservation-request setters you repeat the startOfToday calculation twice—consider extracting that logic into a shared helper to avoid duplication and make the intent clearer.
- Across multiple UI components you use parseGraphQLDateTime plus dayjs conversions inline; you could introduce a small helper (e.g. getDayjsFromGraphQL) so you don’t repeat that pattern in each render block.
- In CreateListingContainer you silently default invalid GraphQL dates to new Date() which may mask bad input—consider surfacing parsing errors or handling missing sharingPeriod values explicitly rather than falling back to the current date.
## Individual Comments
### Comment 1
<location> `apps/ui-sharethrift/src/utils/date-utils.ts:17` </location>
<code_context>
+ * @param dateValue - The DateTime value from GraphQL (string | number | null | undefined)
+ * @returns Valid Date object or null if parsing fails
+ */
+export function parseGraphQLDateTime(dateValue: any): Date | null {
+ if (!dateValue) {
+ return null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the date parsing logic and splitting formatting functions into a separate module for better maintainability and tree-shaking.
```suggestion
// 1) Simplify parseGraphQLDateTime to a single new Date() call,
// automatically handling ISO‐strings, numbers and numeric‐strings:
export function parseGraphQLDateTime(
dateValue: string | number | Date | null | undefined
): Date | null {
if (dateValue == null) return null
// numeric‐only strings → Number; else leave as‐is (Date handles ISO, Date objects, numbers)
const input =
typeof dateValue === 'string' && /^\d+$/.test(dateValue)
? Number(dateValue)
: dateValue
const date = new Date(input)
return isNaN(date.getTime()) ? null : date
}
// 2) Split out the formatters into their own module (only import when you need them):
// src/utils/graphql-date-formatters.ts
export function formatGraphQLDate(
dateValue: any,
options?: Intl.DateTimeFormatOptions,
fallback = 'Invalid Date'
): string {
const date = parseGraphQLDateTime(dateValue)
if (!date) return fallback
try {
return date.toLocaleDateString('en-US', options)
} catch {
return fallback
}
}
export function formatGraphQLDateRange(
startDate: any,
endDate: any,
options?: Intl.DateTimeFormatOptions,
separator = ' - ',
fallback = 'Invalid Date Range'
): string {
const start = parseGraphQLDateTime(startDate)
const end = parseGraphQLDateTime(endDate)
if (!start || !end) return fallback
try {
return (
start.toLocaleDateString('en-US', options) +
separator +
end.toLocaleDateString('en-US', options)
)
} catch {
return fallback
}
}
export function dayjsToGraphQLDate(dayjsDate: any): string {
if (!dayjsDate?.format) throw new Error('Invalid Dayjs object')
const [year, month, day] = [
dayjsDate.year(),
dayjsDate.month(),
dayjsDate.date(),
]
return new Date(Date.UTC(year, month, day)).toISOString()
}
```
- Shrinks `parseGraphQLDateTime` from ~20 lines to 8 by leveraging `new Date()` for most cases and a simple numeric‐string check.
- Moves formatting helpers into a separate file (`graphql-date-formatters.ts`), so your build only includes what you actually import.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| * @param dateValue - The DateTime value from GraphQL (string | number | null | undefined) | ||
| * @returns Valid Date object or null if parsing fails | ||
| */ | ||
| export function parseGraphQLDateTime(dateValue: any): Date | null { |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the date parsing logic and splitting formatting functions into a separate module for better maintainability and tree-shaking.
Suggested change
| export function parseGraphQLDateTime(dateValue: any): Date | null { | |
| // 1) Simplify parseGraphQLDateTime to a single new Date() call, | |
| // automatically handling ISO‐strings, numbers and numeric‐strings: | |
| export function parseGraphQLDateTime( | |
| dateValue: string | number | Date | null | undefined | |
| ): Date | null { | |
| if (dateValue == null) return null | |
| // numeric‐only strings → Number; else leave as‐is (Date handles ISO, Date objects, numbers) | |
| const input = | |
| typeof dateValue === 'string' && /^\d+$/.test(dateValue) | |
| ? Number(dateValue) | |
| : dateValue | |
| const date = new Date(input) | |
| return isNaN(date.getTime()) ? null : date | |
| } | |
| // 2) Split out the formatters into their own module (only import when you need them): | |
| // src/utils/graphql-date-formatters.ts | |
| export function formatGraphQLDate( | |
| dateValue: any, | |
| options?: Intl.DateTimeFormatOptions, | |
| fallback = 'Invalid Date' | |
| ): string { | |
| const date = parseGraphQLDateTime(dateValue) | |
| if (!date) return fallback | |
| try { | |
| return date.toLocaleDateString('en-US', options) | |
| } catch { | |
| return fallback | |
| } | |
| } | |
| export function formatGraphQLDateRange( | |
| startDate: any, | |
| endDate: any, | |
| options?: Intl.DateTimeFormatOptions, | |
| separator = ' - ', | |
| fallback = 'Invalid Date Range' | |
| ): string { | |
| const start = parseGraphQLDateTime(startDate) | |
| const end = parseGraphQLDateTime(endDate) | |
| if (!start || !end) return fallback | |
| try { | |
| return ( | |
| start.toLocaleDateString('en-US', options) + | |
| separator + | |
| end.toLocaleDateString('en-US', options) | |
| ) | |
| } catch { | |
| return fallback | |
| } | |
| } | |
| export function dayjsToGraphQLDate(dayjsDate: any): string { | |
| if (!dayjsDate?.format) throw new Error('Invalid Dayjs object') | |
| const [year, month, day] = [ | |
| dayjsDate.year(), | |
| dayjsDate.month(), | |
| dayjsDate.date(), | |
| ] | |
| return new Date(Date.UTC(year, month, day)).toISOString() | |
| } |
- Shrinks
parseGraphQLDateTimefrom ~20 lines to 8 by leveragingnew Date()for most cases and a simple numeric‐string check. - Moves formatting helpers into a separate file (
graphql-date-formatters.ts), so your build only includes what you actually import.
Contributor
|
@copilot I am facing this issue, can you resolve this: |
Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
Contributor
Author
Resolved the SonarQube coverage issue in commit Added comprehensive test coverage:
Coverage achieved:
The quality gate failure was due to the new |
Copilot
AI
changed the title
Fix same-day reservation blocking and invalid date display
Fix same-day reservation blocking, invalid date display, and add comprehensive test coverage
Nov 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Users couldn't create reservations for the current day, and reservation dates displayed as "Invalid Date" after booking. Additionally, the PR now includes comprehensive test coverage to resolve SonarQube quality gate failures.
Backend validation
Domain validation compared reservation dates against
Date.now()(current timestamp), rejecting same-day reservations. Changed to compare against start of day:Applied to both
reservationPeriodStartandreservationPeriodEndsetters inreservation-request.ts.Frontend date parsing
GraphQL returns timestamps as BigInt/string but frontend passed them directly to
Date()constructor. Added explicitNumber()conversion in:reservation-card.tsx- card view displayreservations-table.tsx- table view displayTest coverage
Added comprehensive test cases to validate the same-day reservation fix and achieve 100% coverage on new utility functions:
Backend Tests
reservation-request.test.tsreservation-request.featureTEST_DOCUMENTATION.mdwith detailed test rationale and future recommendationsThe test validates that reservations can be created for the current day (e.g., noon today) without errors, confirming the fix works as expected.
Frontend Tests
date-utils.test.tswith 34 comprehensive test casesdate-utils.ts(125 lines)package.jsonparseGraphQLDateTime- 8 tests covering null handling, Date validation, ISO/timestamp parsingformatGraphQLDate- 7 tests covering formatting, fallback, error handlingformatGraphQLDateRange- 9 tests covering range formatting, separators, edge casesdayjsToGraphQLDate- 7 tests covering validation, UTC conversion, timezone handlingThis resolves the SonarQube quality gate failure (coverage requirement: ≥ 23.0%, achieved: 100% on new utility module).
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by Sourcery
Fix backend and frontend date handling to allow same-day reservations and prevent invalid date displays, accompanied by expanded test coverage and documentation.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: