Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds client-side error propagation for date/time fields and strengthens validation/normalization of phone numbers and addresses by introducing server-side address validation and applying it in trip and booking creation/update flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FrontendForm as Frontend Form
participant ServerAPI as Server API (bookings/trip)
participant FormRouter as form.validateAddress
participant GoogleAPI as Google Address Validation
participant DB
Client->>FrontendForm: submit booking (phone, pickup, destination, start/end)
FrontendForm->>ServerAPI: call create mutation (trimmed inputs)
ServerAPI->>ServerAPI: validate phone (Zod + parsePhoneNumber)
ServerAPI->>FormRouter: validateAddress({ regionCode, address: [..] })
FormRouter->>GoogleAPI: request address validation
GoogleAPI-->>FormRouter: response (verdict, missingComponentTypes)
FormRouter-->>ServerAPI: validated/formatted address OR null
alt address invalid
ServerAPI-->>FrontendForm: TRPCError BAD_REQUEST (Invalid Address)
else address valid
ServerAPI->>DB: persist normalized phone and addresses
DB-->>ServerAPI: success
ServerAPI-->>FrontendForm: success response
end
FrontendForm-->>Client: show success or error feedback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/api/routers/form.ts`:
- Around line 142-154: The code reads data.result.address.missingComponentTypes
without guarding for undefined which can throw; update the logic around
addrIsPerfect/addrIsGoodEnough to first normalize missingFromAddr (e.g., const
missingFromAddr = Array.isArray(data.result.address.missingComponentTypes) ?
data.result.address.missingComponentTypes : []) so subsequent length checks and
index accesses are safe, then use that normalized missingFromAddr in the
existing addrIsGoodEnough computation and the final return that references
data.result.address.formattedAddress.
In `@src/server/api/routers/trip.ts`:
- Around line 29-38: The parser call
parsePhoneNumberWithError(input.phoneNumber) can throw before your guard and you
also replace the value with a national display string; wrap the
parsePhoneNumberWithError call in a try-catch around the block that validates
phoneNumberSchema and call phoneNumber.isValid(), and on error throw the same
TRPCError; on success assign input.phoneNumber = phoneNumber.number (the E.164
canonical form) instead of "+1 " + phoneNumber.formatNational() so it matches
phoneNumberSchema and downstream validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b76c2762-f8c5-45d9-834d-3369a9822517
📒 Files selected for processing (4)
src/app/_components/agencycomponents/agency-form.tsxsrc/app/_components/agencycomponents/agency-interactive-area.tsxsrc/server/api/routers/form.tssrc/server/api/routers/trip.ts
jason-duong4509
force-pushed
the
SANC-104-Fix-Create-Bookings
branch
from
cfea8b5 to
9fe6816
Compare
jason-duong4509
force-pushed
the
SANC-104-Fix-Create-Bookings
branch
from
9fe6816 to
3cd28ce
Compare
Did the following to improve the create bookings feature - Added front-end phone number validation - Fixed start/end time field boxes not turning red on invalid input - Added back-end validation for creating/updating bookings - Updated validate address endpoint to allow some leniency in validation
jason-duong4509
force-pushed
the
SANC-104-Fix-Create-Bookings
branch
from
3cd28ce to
9758633
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/api/routers/bookings.ts (1)
629-665: Consider extracting address validation into a helper to reduce duplication.The
serverCalleris created twice (lines 631 and 650), and both pickup and destination validation blocks follow an identical pattern. Extracting a helper would reduce repetition and simplify future maintenance.Suggested refactor
// Helper function (can be placed above the router or inline) async function validateAndNormalizeAddress( serverCaller: ReturnType<typeof appRouter.createCaller>, address: string, fieldName: string, ): Promise<string> { const result = await serverCaller.form.validateAddress({ regionCode: "ca", address: [address], }); if (result === null) { throw new TRPCError({ code: "BAD_REQUEST", message: `Invalid ${fieldName}`, }); } return result; } // Usage in the update mutation: const serverCaller = appRouter.createCaller(ctx); if (updatesToApply.pickupAddress) { updatesToApply.pickupAddress = await validateAndNormalizeAddress( serverCaller, updatesToApply.pickupAddress, "Pickup Address", ); } if (updatesToApply.destinationAddress) { updatesToApply.destinationAddress = await validateAndNormalizeAddress( serverCaller, updatesToApply.destinationAddress, "Destination Address", ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/bookings.ts` around lines 629 - 665, Duplicate address validation logic: create a single helper (e.g., validateAndNormalizeAddress) that accepts the serverCaller returned by appRouter.createCaller(ctx), the raw address string, and a fieldName; have it call serverCaller.form.validateAddress({ regionCode: "ca", address: [address] }), throw the same TRPCError when result is null, and return the normalized result. Instantiate serverCaller once via appRouter.createCaller(ctx) and replace both inline validation blocks for updatesToApply.pickupAddress and updatesToApply.destinationAddress by awaiting validateAndNormalizeAddress(...) and assigning the returned normalized address back to the respective updatesToApply field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 608-627: The current phone-number update flow can swallow
non-Error throwables and trims only the value passed to phoneNumberSchema but
not to parsePhoneNumberWithError; to fix, pass the trimmed string (e.g., const
raw = updatesToApply.phoneNumber.trim()) to both phoneNumberSchema.safeParse and
parsePhoneNumberWithError, use the parsed phoneNumber result only if
phoneNumber.isValid(), and make the catch unconditionally rethrow a TRPCError
for any thrown value (not just when e instanceof Error)—map ParseError to
"Invalid phone number" otherwise include the thrown value/message—then set
updatesToApply.phoneNumber = phoneNumber.number only after successful
validation.
---
Nitpick comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 629-665: Duplicate address validation logic: create a single
helper (e.g., validateAndNormalizeAddress) that accepts the serverCaller
returned by appRouter.createCaller(ctx), the raw address string, and a
fieldName; have it call serverCaller.form.validateAddress({ regionCode: "ca",
address: [address] }), throw the same TRPCError when result is null, and return
the normalized result. Instantiate serverCaller once via
appRouter.createCaller(ctx) and replace both inline validation blocks for
updatesToApply.pickupAddress and updatesToApply.destinationAddress by awaiting
validateAndNormalizeAddress(...) and assigning the returned normalized address
back to the respective updatesToApply field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89730b17-4370-42c0-b941-8379faebccb5
📒 Files selected for processing (5)
src/app/_components/agencycomponents/agency-form.tsxsrc/app/_components/agencycomponents/agency-interactive-area.tsxsrc/server/api/routers/bookings.tssrc/server/api/routers/form.tssrc/server/api/routers/trip.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/_components/agencycomponents/agency-interactive-area.tsx
- src/app/_components/agencycomponents/agency-form.tsx
- src/server/api/routers/trip.ts
jason-duong4509
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
promatty,
tanvimahal,
themaxboucher and
wesleylui
as code owners
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/server/api/routers/bookings.ts (1)
608-627:⚠️ Potential issue | 🟠 MajorTrim once and rethrow every phone-validation failure.
Lines 611-612 validate two different values, and Line 619 only converts
Errorinstances intoTRPCError. If anything else is thrown, the mutation continues withupdatesToApply.phoneNumberuntouched.🛠️ Proposed fix
if (updatesToApply.phoneNumber) { - //Wants to change phone number + const trimmedPhone = updatesToApply.phoneNumber.trim(); try { - const res = phoneNumberSchema.safeParse(updatesToApply.phoneNumber.trim()); //Check if phone num is valid - const phoneNumber = parsePhoneNumberWithError(updatesToApply.phoneNumber); + const res = phoneNumberSchema.safeParse(trimmedPhone); + const phoneNumber = parsePhoneNumberWithError(trimmedPhone); if (!res.success || !phoneNumber.isValid()) { - // Failed regex check or API check throw new Error(res.error?.issues[0]?.message ?? "Invalid phone number"); } updatesToApply.phoneNumber = phoneNumber.number; } catch (e) { - if (e instanceof Error) { - // Failed regex check or API check - throw new TRPCError({ - code: "BAD_REQUEST", - message: e instanceof ParseError ? "Invalid phone number" : e.message, - }); - } + throw new TRPCError({ + code: "BAD_REQUEST", + message: + e instanceof ParseError + ? "Invalid phone number" + : e instanceof Error + ? e.message + : "Invalid phone number", + }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/bookings.ts` around lines 608 - 627, The phone validation uses different inputs and fails to rethrow non-Error throwables; first trim the input once into a local const (e.g., const raw = updatesToApply.phoneNumber.trim()), use that same raw for phoneNumberSchema.safeParse(raw) and parsePhoneNumberWithError(raw), and after success assign updatesToApply.phoneNumber = phoneNumber.number; in the catch block always throw a TRPCError (not only for Error instances) and map ParseError to a "Invalid phone number" message while preserving other error messages for debugging (referencing phoneNumberSchema, parsePhoneNumberWithError, ParseError, TRPCError, and updatesToApply.phoneNumber).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 608-665: The create mutation still copies input.phoneNumber,
input.pickupAddress, and input.destinationAddress directly into bookingData, so
replicate the same validation/normalization logic used in the update path: reuse
phoneNumberSchema + parsePhoneNumberWithError (as used in the update block) to
validate/normalize input.phoneNumber before assigning to
bookingData.phoneNumber, and call
appRouter.createCaller(ctx).form.validateAddress for input.pickupAddress and
input.destinationAddress (same params and null checks as the update flow) to
replace bookingData.pickupAddress and bookingData.destinationAddress with the
normalized results; surface TRPCError BAD_REQUEST messages on failures just like
the update logic.
- Around line 608-665: The current falsy guards (if
(updatesToApply.phoneNumber), if (updatesToApply.pickupAddress), if
(updatesToApply.destinationAddress)) let empty strings bypass validation and be
written to the DB; change these checks to explicit undefined checks (e.g.,
updatesToApply.phoneNumber !== undefined) and add empty-string handling: for
phoneNumber, either reject empty string with a BAD_REQUEST TRPCError or
normalize it to null before validation (update logic around phoneNumberSchema,
parsePhoneNumberWithError, and the TRPCError thrown); for pickupAddress and
destinationAddress, explicitly throw a BAD_REQUEST TRPCError with "Invalid
Pickup Address"/"Invalid Destination Address" when the value === "" before
calling serverCaller.form.validateAddress (serverCaller created via
appRouter.createCaller and using form.validateAddress).
In `@src/server/api/routers/form.ts`:
- Around line 111-112: Update the Zod input schema for validateAddress (the
protectedProcedure with .input(...)) to reject empty arrays and whitespace-only
strings: change address to z.array(z.string().min(1).refine(s => s.trim().length
> 0, { message: "address entries cannot be blank" })).nonempty({ message:
"address must contain at least one entry" }) so validation fails locally before
calling the Google Maps API.
---
Duplicate comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 608-627: The phone validation uses different inputs and fails to
rethrow non-Error throwables; first trim the input once into a local const
(e.g., const raw = updatesToApply.phoneNumber.trim()), use that same raw for
phoneNumberSchema.safeParse(raw) and parsePhoneNumberWithError(raw), and after
success assign updatesToApply.phoneNumber = phoneNumber.number; in the catch
block always throw a TRPCError (not only for Error instances) and map ParseError
to a "Invalid phone number" message while preserving other error messages for
debugging (referencing phoneNumberSchema, parsePhoneNumberWithError, ParseError,
TRPCError, and updatesToApply.phoneNumber).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f56ebb38-742c-4733-bdca-470de0dd5f5e
📒 Files selected for processing (5)
src/app/_components/agencycomponents/agency-form.tsxsrc/app/_components/agencycomponents/agency-interactive-area.tsxsrc/server/api/routers/bookings.tssrc/server/api/routers/form.tssrc/server/api/routers/trip.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/_components/agencycomponents/agency-form.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/_components/agencycomponents/agency-interactive-area.tsx
- src/server/api/routers/trip.ts
Contributor
|
merge conflicts @jason-duong4509. also please take a look at the coderabbit comments |
…ookings # Conflicts: # src/server/api/routers/trip.ts
- Carried over input validation to from bookings.update endpoint to bookings.create endpoint - Prevented empty strings from being added to the DB - Rejected input early in form.validateAddress to avoid unnecessary API costs - Added handling for rare errors when phone number inputs are invalid
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Made a few fixes to the create bookings feature to address various bugs associated with it
1. Phone number validation
Phone number validation was added to the form to prevent malformed input from existing in the DB
1.mp4
A regex check was added to the front-end and the back-end uses an additional library to confirm that the phone number is valid
2.mp4
2. Better UI response for invalid start/end time
The start and end time fields silently reject the user's input when invalid. This has been changed to show a red outline and an error message, like all other fields in the modal
3.mp4
3. Back-end address validation
There's no back-end validation for addresses, allowing one to put anything into the field if they modify the front-end
4.mp4
The back-end now validates address inputs and can fix inputs if needed
5.mp4
Summary by CodeRabbit
New Features
Improvements