SANC-43-Connect-agency-booking-form-to-trip-creation-API #34
Conversation
wesleylui
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
jason-duong4509,
promatty,
tanvimahal and
themaxboucher
as code owners
📝 WalkthroughWalkthroughAdds client-side booking creation via a mutation with success/error handlers, expanded form validation (including time range and phone), local-ISO date handling in the form, two validation helpers, a stricter DatePicker NaN check, and a server schema rename contactInfo→phoneNumber. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Form as Agency Form Component
participant Validation as Validation Helpers
participant GooglePlaces as Google Places Autocomplete
participant Mutation as createBookingMutation
participant API as Backend API
participant Toast as Toast Notification
User->>Form: Submit booking
Form->>Validation: Validate fields (title, name, phone, times, addresses)
alt Validation fails
Validation-->>Form: errors
Form->>Toast: show validation error
else Validation passes
Form->>GooglePlaces: ensure destination address valid
alt Address invalid
GooglePlaces-->>Form: invalid
Form->>Toast: set address field error
else Address valid
Form->>Form: convert start/end to local ISO
Form->>Mutation: submit payload
Mutation->>API: create booking request
alt API success
API-->>Mutation: success
Mutation->>Toast: show success
Mutation->>Form: reset form & close modal
else API error
API-->>Mutation: error
Mutation->>Toast: show error
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (2)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)
⏰ 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: ci
🔇 Additional comments (3)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
12-12: LGTM! TRPC integration properly imported.The import of the TRPC API client follows the standard pattern and is correctly used for the mutation hook below.
30-40: Well-structured mutation with comprehensive state management.The mutation setup correctly handles both success and error cases with appropriate notifications and cleanup. The defensive error handling with the fallback message is a good practice.
196-196: Excellent use of mutation pending state.Connecting the modal's loading state directly to
createBookingMutation.isPendingensures the UI accurately reflects the mutation status without additional state management overhead.
burtonjong
left a comment
burtonjong
left a comment
There was a problem hiding this comment.
@wesleylui I think as part of this ticket I wouldn't mind cleaning up some of the validation on the frontend for this.
Things like:
- residentName shouldn't be crazy long (or any of the fields for that matter, maybe except the purpose of the trip)
- endTime shouldn't be less that startTime
If you think there's some validation function that is reusable, you can also put that in validation.ts or whatever the shared validation file is called (has the email validation there as well)
wesleylui
force-pushed
the
SANC-43-Connect-agency-booking-form-to-trip-creation-API
branch
from
0c03daf to
be4a77f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
142-152: Add date validation before ISO string conversion.Lines 149-150 directly convert
form.values.startTimeandform.values.endTimeto ISO strings without validation. If these values are invalid date strings,new Date().toISOString()will throw aRangeError: Invalid time value. The current form validation (lines 72-73) only checks for non-empty values, not date validity. This synchronous error occurs before the mutation call and won't be caught by the mutation'sonErrorhandler, resulting in an uncaught exception.🔎 Recommended fix to validate dates before conversion
if (!validationAddressGood) { form.setFieldError("destinationAddress", "Please select a valid address from the dropdown"); return; } + const startDate = new Date(form.values.startTime); + const endDate = new Date(form.values.endTime); + + if (isNaN(startDate.getTime())) { + form.setFieldError("startTime", "Invalid date format"); + return; + } + + if (isNaN(endDate.getTime())) { + form.setFieldError("endTime", "Invalid date format"); + return; + } + createBookingMutation.mutate({ title: form.values.title, residentName: form.values.residentName, contactInfo: form.values.contactInfo, additionalInfo: form.values.additionalInfo, pickupAddress: form.values.pickupAddress, destinationAddress: inputElement.current?.value || "", - startTime: new Date(form.values.startTime).toISOString(), - endTime: new Date(form.values.endTime).toISOString(), + startTime: startDate.toISOString(), + endTime: endDate.toISOString(), purpose: form.values.purpose, });
🧹 Nitpick comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
31-41: Consider invalidating queries on successful booking creation.The mutation's
onSuccesshandler properly resets the form and shows a notification, but it doesn't invalidate any TRPC queries. If the calendar or table views fetch bookings via TRPC queries, they won't automatically reflect the newly created booking until a manual refresh or navigation.🔎 Suggested enhancement to invalidate queries
+ const utils = api.useUtils(); + const createBookingMutation = api.trip.create.useMutation({ onSuccess: () => { notify.success("Booking successfully created"); form.reset(); setShowBookingModal(false); setValidationAddressGood(false); + void utils.trip.invalidate(); }, onError: (error) => { notify.error(error.message || "Failed to create a booking"); }, });Note: Adjust the invalidation target based on which specific query endpoint is used to fetch the trip list (e.g.,
utils.trip.getAll.invalidate()if that's the specific endpoint).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (2)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)
🔇 Additional comments (3)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
12-12: LGTM!The TRPC API client import is correct and necessary for the mutation implementation.
137-140: LGTM!The Google Maps address validation check properly prevents submission of unvalidated addresses and provides clear user feedback.
197-197: LGTM!Using
createBookingMutation.isPendingfor the modal loading state is the correct approach. This properly reflects the mutation status and prevents duplicate submissions during the API call.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
152-162: CRITICAL: Add date validation before ISO string conversion (duplicate concern).The direct conversion to ISO strings on lines 159-160 will throw a
RangeErrorifform.values.startTimeorform.values.endTimecontain invalid date strings. While the form validation checks if the fields are non-empty and if the time range is valid, it doesn't validate the actual date format forstartTime. If an Invalid Date object is created, calling.toISOString()will throw a synchronous error that won't be caught by the mutation'sonErrorhandler, potentially crashing the component.This is the same concern raised in the previous review but has not been addressed yet.
🔎 Proposed fix to add date validation
if (!validationAddressGood) { form.setFieldError("destinationAddress", "Please select a valid address from the dropdown"); return; } + + const startDate = new Date(form.values.startTime); + const endDate = new Date(form.values.endTime); + + if (isNaN(startDate.getTime())) { + form.setFieldError("startTime", "Invalid date format"); + return; + } + + if (isNaN(endDate.getTime())) { + form.setFieldError("endTime", "Invalid date format"); + return; + } createBookingMutation.mutate({ title: form.values.title, residentName: form.values.residentName, contactInfo: form.values.contactInfo, additionalInfo: form.values.additionalInfo, pickupAddress: form.values.pickupAddress, destinationAddress: inputElement.current?.value || "", - startTime: new Date(form.values.startTime).toISOString(), - endTime: new Date(form.values.endTime).toISOString(), + startTime: startDate.toISOString(), + endTime: endDate.toISOString(), purpose: form.values.purpose, });
🧹 Nitpick comments (2)
src/types/validation.ts (1)
20-37: LGTM with minor documentation note.The validation logic is correct and handles trimming appropriately. However, the JSDoc comment on line 15 mentions "default: 1" for
minLength, but the function signature doesn't include a default value. Consider either adding the default to the signature or removing it from the documentation for consistency.src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
69-88: Consider adding date format validation.The form validation correctly uses the new validation helpers and properly chains the endTime validation (required check + time range). However, the
startTimevalidation on line 78 only checks if the value is non-empty, not whether it's a valid date string. While the Mantine DateTimePicker component likely enforces valid dates, adding defensive validation would prevent potential runtime errors.🔎 Proposed enhancement for startTime validation
- startTime: (value) => (value.trim().length > 0 ? null : "Date and time is required"), + startTime: (value) => { + if (value.trim().length === 0) return "Date and time is required"; + const date = new Date(value); + if (isNaN(date.getTime())) return "Invalid date format"; + return null; + },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/_components/agencycomponents/agency-interactive-area.tsxsrc/types/validation.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)src/types/validation.ts (2)
validateStringLength(20-37)validateTimeRange(45-59)
⏰ 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: ci
🔇 Additional comments (3)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
12-14: LGTM!The imports are properly structured and typed according to the TRPC and validation module setup.
32-42: LGTM!The mutation setup follows best practices with proper success and error handling. The cleanup in
onSuccesscorrectly resets all relevant state including the form, modal visibility, and address validation.
207-207: LGTM!Using
createBookingMutation.isPendingfor the loading state is the correct approach and follows TanStack Query best practices. This ensures the loading state is automatically synchronized with the mutation status.
|
Added reusable validation utilities to
These validators are used in the agency booking form but can be reused in other forms for validating general strings and dates |
promatty
left a comment
promatty
left a comment
There was a problem hiding this comment.
also creation doesnt work rn since the datepicker doesnt work. this is likely an issue with iso strings not converting to a Date properly, but may also be how we no longer use this transportDateTime field
Screen.Recording.2025-12-28.at.7.09.03.PM.mov
| }, | ||
| startTime: (value) => (value.trim().length > 0 ? null : "Date and time is required"), | ||
| endTime: (value) => (value.trim().length > 0 ? null : "Date and time is required"), | ||
| purpose: (value) => (value.trim().length > 0 ? null : "Purpose is required"), | ||
| pickupAddress: (value) => (value.trim().length > 0 ? null : "Pickup address is required"), | ||
| destinationAddress: (value) => | ||
| value.trim().length > 0 ? null : "Destination address is required", | ||
| endTime: (value, values) => { | ||
| // First check if required | ||
| if (value.trim().length === 0) return "Date and time is required"; | ||
| // Then validate time range | ||
| return validateTimeRange(values.startTime, value); | ||
| }, |
There was a problem hiding this comment.
the agency-form only displays a single datepicker for date and time of transport. have it show a datepicker for both the start and end times instead of a transportDateTime
| contactInfo: form.values.contactInfo, | ||
| additionalInfo: form.values.additionalInfo, | ||
| pickupAddress: form.values.pickupAddress, | ||
| destinationAddress: inputElement.current?.value || "", |
There was a problem hiding this comment.
why do we use the ref value instead of form value?
There was a problem hiding this comment.
I believe we have to use the ref value for destinationAddress because of the Google autocomplete. I believe it'll autocomplete the value directly in the HTML element so we will get the full address from there and not the original input on the form.
| notify.success("Booking successfully created"); | ||
| form.reset(); | ||
| setShowBookingModal(false); | ||
| setValidationAddressGood(false); |
There was a problem hiding this comment.
after creating a new booking, the table and calendar wont refresh. perhaps we should invalidate the bookings query to cause a refresh
| setValidationAddressGood(false); | |
| setValidationAddressGood(false); | |
| void utils.bookings.invalidate(); |
wesleylui
force-pushed
the
SANC-43-Connect-agency-booking-form-to-trip-creation-API
branch
from
ee91731 to
fa49840
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/_components/agencycomponents/agency-interactive-area.tsx (2)
171-181: Add date validation before ISO string conversion.Lines 178-179 directly convert date strings to ISO format without validation. If
form.values.startTimeorform.values.endTimecontain invalid date strings,new Date().toISOString()will throw aTypeError. The current form validation only checks for non-empty values (line 87), not date validity. This synchronous error won't be caught by the mutation'sonErrorhandler.🔎 Proposed fix to validate dates before conversion
if (!validationAddressGood) { form.setFieldError( "destinationAddress", "Please select a valid address from the dropdown" ); return; } + const startDate = new Date(form.values.startTime); + const endDate = new Date(form.values.endTime); + + if (isNaN(startDate.getTime())) { + form.setFieldError("startTime", "Invalid date format"); + return; + } + + if (isNaN(endDate.getTime())) { + form.setFieldError("endTime", "Invalid date format"); + return; + } createBookingMutation.mutate({ title: form.values.title, residentName: form.values.residentName, contactInfo: form.values.contactInfo, additionalInfo: form.values.additionalInfo, pickupAddress: form.values.pickupAddress, destinationAddress: inputElement.current?.value || "", - startTime: new Date(form.values.startTime).toISOString(), - endTime: new Date(form.values.endTime).toISOString(), + startTime: startDate.toISOString(), + endTime: endDate.toISOString(), purpose: form.values.purpose, });This issue was previously flagged in an earlier review but remains unaddressed.
41-51: Add query invalidation to refresh the bookings list after creation.After a successful booking creation, the table and calendar views won't automatically display the new booking. Users must manually refresh to see the update.
🔎 Recommended fix to invalidate the bookings query
Add the
useUtils()hook and invalidate the bookings query in theonSuccesshandler:+ const utils = api.useUtils(); const createBookingMutation = api.trip.create.useMutation({ onSuccess: () => { notify.success("Booking successfully created"); form.reset(); setShowBookingModal(false); setValidationAddressGood(false); + void utils.bookings.invalidate(); }, onError: (error) => { notify.error(error.message || "Failed to create a booking"); }, });Based on past review comment from promatty on line 46.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/_components/agencycomponents/agency-interactive-area.tsxsrc/types/validation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types/validation.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)src/types/validation.ts (2)
validateStringLength(20-37)validateTimeRange(45-59)
🪛 GitHub Actions: CI
src/app/_components/agencycomponents/agency-interactive-area.tsx
[error] 24-24: File content differs from formatting output.
🔇 Additional comments (5)
src/app/_components/agencycomponents/agency-interactive-area.tsx (5)
14-14: LGTM!The validation utility imports are correctly added and will be used for form validation throughout the component.
78-97: LGTM!The form validation is comprehensive and correctly implements:
- String length validation for all required text fields with appropriate max lengths
- Optional field handling for
additionalInfo- Time range validation ensuring
endTimeis afterstartTime- Clear, user-friendly field names in error messages
104-118: LGTM!The enhanced null checks prevent potential runtime errors when setting up Google Places Autocomplete. The code now properly validates that the API is loaded, the places library is available, and the input element reference exists before initialization.
177-177: Explanation: Using ref value for Google Places integration.The code uses
inputElement.current?.valueinstead ofform.values.destinationAddressbecause Google Places Autocomplete directly updates the DOM input element's value when a user selects an address from the dropdown. This happens outside of Mantine's form state management, so the ref provides access to the actual selected value. This is a standard integration pattern for Google Places with React forms.In response to promatty's question on line 177.
240-240: LGTM!Using
createBookingMutation.isPendingfor the modal loading state is the correct pattern. This ensures the loading indicator accurately reflects the mutation's pending state during form submission.
burtonjong
left a comment
burtonjong
left a comment
There was a problem hiding this comment.
You'll have to take another look at https://github.com/Code-the-Change-YYC/salvationarmy/pull/40/changes#diff-09231f59b104026943ace40134be0e25e8f2821eb96983bf8b5e02721e2834dd
once it's merged.
You might have to merge that into your branch and fix some things.
You have to be careful with the start/end times, as there are many things that can go wrong with timestamps
Just for example:
- Drizzle declares them in booking schema as below, but what does that really mean?
startTime: timestamp("start_time", {
mode: "string",
withTimezone: true,
}).notNull(),
- Supabase stores timestamps as
timestamptz Date.prototype.toISOString()always converts the date to UTC in the standard ISO 8601 format, so that could cause problems- basically when the agency books the booking, we have to make sure when it's reflected on the schedule that it is in LOCAL TIME, not UTC
|
Here's what I mean: { it is sent like this. I don't even think this can be parsed into a valid format. You might have to change how this date is stored! |
|
You should be able to format it the way you want to change it to an isostring with the valueFormat prop: |
…, add success notify.
wesleylui
force-pushed
the
SANC-43-Connect-agency-booking-form-to-trip-creation-API
branch
from
442a47f to
92bfb7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
1-17: Fix import sorting to resolve pipeline failure.The pipeline reports that imports are not properly sorted. Run your formatter to auto-fix this issue.
#!/bin/bash # Description: Run Biome to fix import sorting npx @biomejs/biome check --apply src/app/_components/agencycomponents/agency-interactive-area.tsx
🤖 Fix all issues with AI agents
In @src/app/_components/agencycomponents/agency-interactive-area.tsx:
- Around line 166-177: The redundant date parsing/validation block that creates
startDate/endDate and calls form.setFieldError for "startTime" and "endTime"
should be removed because the same validation is already performed in the form's
validate function (the logic using Number.isNaN(date.getTime()) around
form.validate()). Delete the lines that construct startDate and endDate and the
subsequent if checks that call form.setFieldError("startTime", ...) and
form.setFieldError("endTime", ...), leaving any code that relies on validated
values but relying on the existing validate() path instead.
🧹 Nitpick comments (1)
src/types/validation.ts (1)
20-37: Consider more flexible error messaging for varying minLength values.The function always returns
"${fieldName} is required"whenlength < minLength, which works for the current usage whereminLength=1throughout the codebase. However, since this is an exported utility, future callers might useminLength > 1and expect an error message like"${fieldName} must be at least ${minLength} characters"instead of"is required".♻️ Proposed enhancement for more descriptive error messages
if (trimmedValue.length < minLength) { - return `${fieldName} is required`; + return minLength === 1 + ? `${fieldName} is required` + : `${fieldName} must be at least ${minLength} characters`; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/_components/agencycomponents/agency-interactive-area.tsxsrc/app/_components/common/datepicker/DatePicker.tsxsrc/app/debug/bookings/page.tsxsrc/types/validation.ts
💤 Files with no reviewable changes (1)
- src/app/debug/bookings/page.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
src/trpc/react.tsx (1)
api(25-25)src/lib/notifications.ts (1)
notify(13-58)src/types/validation.ts (2)
validateStringLength(20-37)validateTimeRange(45-68)
🪛 GitHub Actions: CI
src/app/_components/agencycomponents/agency-interactive-area.tsx
[warning] 1-1: Imports/exports are not sorted. Organize imports.
🔇 Additional comments (5)
src/app/_components/common/datepicker/DatePicker.tsx (1)
45-45: LGTM: Stricter NaN validation.Using
Number.isNaN()instead ofisNaN()is correct here since it avoids type coercion and provides stricter validation for numeric NaN values.src/app/_components/agencycomponents/agency-interactive-area.tsx (4)
33-50: LGTM: Well-structured mutation with proper cleanup.The mutation setup correctly handles:
- Success notification and form reset
- Cache invalidation to refresh the bookings list
- Error handling with user-friendly notifications
- Validation state cleanup
76-102: LGTM: Comprehensive form validation.The form validation properly handles:
- Required fields with appropriate length constraints
- Optional fields (additionalInfo) with conditional validation
- Date format validation before time range comparison
- Reusable validation helpers for consistency
186-187: Datetime format appears correct; verify if PR comment concern is resolved.The code uses
toISOString()which produces ISO 8601 format (e.g.,"2025-01-08T12:43:00.000Z"). However, a PR comment from @burtonjong mentioned the payload contains datetime values like"2026-01-08 12:43:00"(non-ISO format).If the concern from that comment is still valid, there may be a mismatch elsewhere in the data flow (e.g., server-side formatting or database storage). Otherwise, this implementation is correct.
248-248: LGTM: Proper loading state integration.Using
createBookingMutation.isPendingcorrectly ties the modal's loading indicator to the mutation state, providing user feedback during API calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/app/_components/agencycomponents/agency-form.tsx:
- Around line 28-40: Update the toLocalISOString helper to accept Date | string
| null (instead of Date | null) and normalize string inputs: if value is a
string, parse it into a Date (e.g., new Date(value)) or return "" for
invalid/nullable values; then format the resulting Date as before. Make sure to
update the function signature (toLocalISOString) and its internal checks so
calls from the DateTimePicker onChange handlers (where value can be Date |
string | null) no longer cause TypeScript errors.
🧹 Nitpick comments (1)
src/app/_components/agencycomponents/agency-interactive-area.tsx (1)
79-79: Minor: Inconsistent field label in validation.The validation uses the label
"Contact information"but the field is namedphoneNumber. Consider using"Phone number"for consistency with the field name and UI label.📝 Suggested change
- phoneNumber: (value) => validateStringLength(value, 1, 150, "Contact information"), + phoneNumber: (value) => validateStringLength(value, 1, 150, "Phone number"),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/_components/agencycomponents/agency-form.tsxsrc/app/_components/agencycomponents/agency-interactive-area.tsxsrc/server/api/routers/trip.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/server/api/routers/trip.ts (2)
src/app/debug/bookings/page.tsx (1)
v(32-32)src/app/_components/agencypage/table-view.tsx (2)
booking(60-68)transformBookingsToScheduleInfo(59-69)
src/app/_components/agencycomponents/agency-interactive-area.tsx (3)
src/trpc/react.tsx (1)
api(25-25)src/types/validation.ts (2)
validateStringLength(20-37)validateTimeRange(45-68)src/app/debug/bookings/page.tsx (1)
v(30-30)
src/app/_components/agencycomponents/agency-form.tsx (1)
src/app/_components/common/datepicker/DatePicker.tsx (3)
DatePicker(31-83)mantineValue(51-63)DatePickerProps(7-14)
🪛 GitHub Actions: CI
src/app/_components/agencycomponents/agency-form.tsx
[error] 105-105: TS2345: Argument of type 'string | null' is not assignable to parameter of type 'Date | null'. (while running 'yarn tsc --noEmit')
🪛 GitHub Check: ci
src/app/_components/agencycomponents/agency-form.tsx
[failure] 105-105:
Argument of type 'string | null' is not assignable to parameter of type 'Date | null'.
[failure] 131-131:
Argument of type 'string | null' is not assignable to parameter of type 'Date | null'.
🔇 Additional comments (8)
src/server/api/routers/trip.ts (2)
12-12: LGTM! Good field separation.Renaming
contactInfotophoneNumberprovides better semantic clarity and aligns with the database schema changes.
27-28: LGTM! Cleaner data structure.Storing
phoneNumberas a separate field rather than concatenating it intopassengerInfoimproves data organization and makes it easier to query and display phone numbers independently.src/app/_components/agencycomponents/agency-form.tsx (2)
102-112: Good datetime handling approach.The
valueFormatprop and local ISO string conversion avoid UTC offset issues. The logic to invalidateendTimewhenstartTimechanges is correct. This will work properly once the type issue intoLocalISOStringis resolved.
127-141: LGTM! Consistent datetime handling.The end time picker correctly enforces that it's disabled until a start time is selected and uses
minDateto prevent selecting an end time before the start time. Implementation is consistent with the start time handling.src/app/_components/agencycomponents/agency-interactive-area.tsx (4)
33-50: LGTM! Well-structured mutation with proper cleanup.The mutation setup follows best practices:
- Cache invalidation ensures the booking list refreshes after creation
- Form reset and modal closure provide good UX
- Error handling with user-friendly notifications
- All cleanup actions (form reset, validation flag, modal state) are properly sequenced
85-98: LGTM! Comprehensive datetime validation.The validation properly checks:
- Required fields
- Valid date format using
Number.isNaN(date.getTime())- End time is after start time via
validateTimeRangeThe use of
Number.isNaN(not the globalisNaN) is correct for date validation.
179-189: Verify destinationAddress source is reliable.The mutation uses
inputElement.current?.value || ""fordestinationAddress(line 185). SincevalidationAddressGoodis checked earlier (line 161), the ref should have a value. However, the fallback to empty string could silently pass an invalid address to the API.Consider making the fallback more explicit:
🔍 Suggested defensive check
+ const destinationAddress = inputElement.current?.value; + if (!destinationAddress) { + form.setFieldError("destinationAddress", "Destination address is required"); + return; + } + createBookingMutation.mutate({ title: form.values.title, residentName: form.values.residentName, phoneNumber: form.values.phoneNumber, additionalInfo: form.values.additionalInfo, pickupAddress: form.values.pickupAddress, - destinationAddress: inputElement.current?.value || "", + destinationAddress: destinationAddress, startTime: form.values.startTime, endTime: form.values.endTime, purpose: form.values.purpose, });
248-248: LGTM! Proper mutation loading state.Using
createBookingMutation.isPendingdirectly from the mutation is cleaner than managing local loading state and automatically reflects the mutation's status.
|
Changes:
Lmk if this makes sense, I am a bit confused on the Date stuff |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/_components/agencycomponents/agency-form.tsx (1)
28-50: Consider validating Date objects passed directly.The function validates strings after parsing but uses Date objects directly without checking validity. If an invalid Date is passed (e.g., from external sources), it would produce
"NaN-NaN-NaNTNaN:NaN:NaN".Since DateTimePicker returns valid Date or null, this is low risk in current usage, but adding a guard improves robustness.
🔧 Suggested improvement
} else { date = value; + if (Number.isNaN(date.getTime())) return ""; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/_components/agencycomponents/agency-form.tsx
🔇 Additional comments (3)
src/app/_components/agencycomponents/agency-form.tsx (3)
137-143: minDate may not prevent selecting an earlier time on the same day.The
minDateprop restricts the calendar date selection but typically doesn't prevent picking a time earlier thanstartTimeon that same day. Ensure form-level validation (e.g.,validateTimeRangementioned in related files) catches this case before submission.
119-121: Good use of string comparison for ISO datetime validation.The lexicographic comparison of ISO-formatted strings (
endTime <= localISOString) works correctly because the format maintains chronological order. The reset logic properly handles the edge case where a user changes startTime to be after the existing endTime.
113-113: This concern is not applicable to modern browser environments.Modern ECMAScript (ES2016+) consistently interprets ISO date-time strings without timezone (e.g.,
"YYYY-MM-DDTHH:mm:ss") as local time. The code intentionally preserves local time semantics throughout—thetoLocalISOString()helper function explicitly constructs the ISO string using local date components and includes a comment confirming this design. The parsing behavior on line 113 is correct and predictable across current browsers.Likely an incorrect or invalid review comment.
The agency booking form now uses the trip creation API through a TanStack Query Mutation, this also takes care of loading/error states, and success notifications. Form values are mapped to the API format with proper validation, including Google Maps address verification and datetime conversion. The form automatically resets after successful submission.
Key change:
api.trip.createendpointSummary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.