fix: Datetimepicker typecheck #244
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing error handling for date selection across multiple components. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx (1)
275-275: Type assertion is acceptable but needs documentation.The
anytype assertion is a valid workaround for accessing thedateproperty from Recharts' click event. However, to maintain code clarity:
- Add a comment explaining why this type assertion is necessary
- Consider creating proper type definitions for the event object
- const start = new Date((e as any).date); + // TODO: Create proper type definitions for Recharts' click event + // Using 'any' assertion as Recharts doesn't provide correct typings for the click event + const start = new Date((e as any).date);apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx (1)
Line range hint
151-173: Add validation for time range consistencyThe current implementation doesn't validate that startTime is before endTime. Consider adding validation to ensure valid release windows.
You could:
- Add validation in the form schema:
const schema = z.object({ releaseWindows: z.array( z.object({ // ... existing fields ... startTime: z.date(), endTime: z.date(), }).refine( (data) => data.startTime < data.endTime, "End time must be after start time" ), ), // ... rest of the schema });
- Or handle it in the onChange handlers to provide immediate feedback.
apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/DateConditionRender.tsx (2)
85-85: LGTM! Consider additional error handling.The null check is good, but consider adding error handling for invalid date strings in the
valueprop.Consider wrapping the date conversion in a try-catch:
<DateTimePicker - value={toZonedDateTime(new Date(value))} + value={(() => { + try { + return toZonedDateTime(new Date(value)); + } catch (e) { + console.error('Invalid date string:', value); + return toZonedDateTime(new Date()); + } + })()} onChange={(value) => value != null && setDate(value)} aria-label={type} variant="filter" />
Line range hint
14-41: Consider improving the robustness of toZonedDateTime utility.The utility function could benefit from additional error handling and validation:
- Input validation for the Date object
- Fallback for unavailable timezone
- Simpler offset calculation
Consider this improved implementation:
const toZonedDateTime = (date: Date): ZonedDateTime => { + if (!(date instanceof Date) || isNaN(date.getTime())) { + throw new Error('Invalid Date object'); + } - const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone; + const timeZone = Intl.DateTimeFormat().resolvedOptions().timeZone || 'UTC'; - const offset = -date.getTimezoneOffset() * ms("1m"); + const offset = -date.getTimezoneOffset() * 60000; // Direct calculation in milliseconds // ... rest of the function };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/_components/environment-policy-drawer/RolloutAndTiming.tsx(2 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/filter/DateConditionRender.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/systems/JobHistoryChart.tsx(1 hunks)packages/ui/src/date-time-picker/date-time-picker.tsx(1 hunks)
🔇 Additional comments (1)
packages/ui/src/date-time-picker/date-time-picker.tsx (1)
73-73: LGTM! Type guard improves runtime safety.
The added null check is a good defensive programming practice that prevents potential runtime issues.
Let's verify if this null check is necessary by checking React Aria's TimeField documentation and implementation:
✅ Verification successful
The null check is necessary and correct
The null check is justified because:
- The TimeField component uses React Aria's
useTimeFieldhook which works with theTimeValuetype from react-aria - The DateTimePicker's state management is handled by
useDatePickerStatefrom react-stately, wheretimeValueandsetTimeValueare part of the state - The type guard ensures type safety between these two different state management systems
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TimeField implementation details and null value handling
rg -A 5 "TimeField.*onChange.*null" --type ts
Length of output: 47
Script:
#!/bin/bash
# Let's check the TimeField usage and implementation in the codebase
ast-grep --pattern 'TimeField'
# Also check the imports to understand which TimeField we're using
rg "import.*TimeField" --type ts
Length of output: 839
Script:
#!/bin/bash
# Let's examine the time-field.tsx implementation to understand the value handling
cat packages/ui/src/date-time-picker/time-field.tsx
Length of output: 1314
Script:
#!/bin/bash
# Let's check the DateTimePicker implementation to understand the full context
cat packages/ui/src/date-time-picker/date-time-picker.tsx
Length of output: 2704
| startTime: | ||
| t != null | ||
| ? t.toDate( | ||
| Intl.DateTimeFormat().resolvedOptions() | ||
| .timeZone, | ||
| ) | ||
| : new Date(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the default fallback for null dates
While the null check is good, defaulting to the current date when t is null might mask errors or create unexpected behavior. Consider either:
- Preserving the existing date value instead of defaulting to current date
- Implementing proper form validation to handle null dates
- startTime:
- t != null
- ? t.toDate(
- Intl.DateTimeFormat().resolvedOptions()
- .timeZone,
- )
- : new Date(),
+ startTime: t?.toDate(
+ Intl.DateTimeFormat().resolvedOptions()
+ .timeZone,
+ ) ?? value.startTime,📝 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.
| startTime: | |
| t != null | |
| ? t.toDate( | |
| Intl.DateTimeFormat().resolvedOptions() | |
| .timeZone, | |
| ) | |
| : new Date(), | |
| startTime: t?.toDate( | |
| Intl.DateTimeFormat().resolvedOptions() | |
| .timeZone, | |
| ) ?? value.startTime, |
| endTime: | ||
| t != null | ||
| ? t.toDate( | ||
| Intl.DateTimeFormat().resolvedOptions() | ||
| .timeZone, | ||
| ) | ||
| : new Date(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent handling for endTime
Apply the same improvement as suggested for startTime to maintain consistency.
- endTime:
- t != null
- ? t.toDate(
- Intl.DateTimeFormat().resolvedOptions()
- .timeZone,
- )
- : new Date(),
+ endTime: t?.toDate(
+ Intl.DateTimeFormat().resolvedOptions()
+ .timeZone,
+ ) ?? value.endTime,📝 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.
| endTime: | |
| t != null | |
| ? t.toDate( | |
| Intl.DateTimeFormat().resolvedOptions() | |
| .timeZone, | |
| ) | |
| : new Date(), | |
| endTime: t?.toDate( | |
| Intl.DateTimeFormat().resolvedOptions() | |
| .timeZone, | |
| ) ?? value.endTime, |
Summary by CodeRabbit
New Features
Bug Fixes
RolloutAndTiming,DateConditionRender, andDateTimePickercomponents to prevent null values from causing issues.Chores