Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Telnyx SMS integration: new env vars and dependency, server env schema updates, a new SMS utility to send booking-created/updated messages, and hooks in booking create/update flows to notify driver phone numbers asynchronously. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as API Client
participant Server as Server
participant DB as Database
participant Telnyx as Telnyx API
participant Drivers as Drivers
Client->>Server: POST create/update booking
Server->>DB: Insert / Update booking record
DB-->>Server: Booking result (id, details)
Server->>DB: Query users where role = "driver"
DB-->>Server: Driver phone numbers
Server->>Server: Build SMS body(s)
Server->>Telnyx: Send SMS messages (concurrent)
rect rgba(100,150,200,0.5)
Telnyx->>Drivers: Deliver SMS
end
Telnyx-->>Server: Responses / errors
Server-->>Client: Return API response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Contributor
Author
|
Currently, SMS notifications only get sent to |
themaxboucher
requested review from
JustinTan-1,
Lujarios,
Yemyam,
burtonjong,
jason-duong4509,
promatty,
tanvimahal and
wesleylui
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/lib/sms.ts (1)
45-46: Make SMS date/time formatting timezone-explicit.Current output depends on server-local timezone, which causes drift across environments. The two SMS functions (
sendBookingCreatedSmsandsendBookingUpdatedSms) at lines 45–46 and 55–56 format booking times without timezone specification.Implement explicit timezone handling by extending dayjs with the
utcandtimezoneplugins, then use.tz('America/Edmonton')to ensure consistent formatting:Suggested fix
import dayjs from "dayjs"; +import timezone from "dayjs/plugin/timezone"; +import utc from "dayjs/plugin/utc"; import Telnyx from "telnyx"; import { env } from "@/env"; +dayjs.extend(utc); +dayjs.extend(timezone); +const BOOKING_TIMEZONE = "America/Edmonton"; + +function formatBookingStart(startTime: string) { + return dayjs(startTime).tz(BOOKING_TIMEZONE).format("MMM D, YYYY [at] h:mm A"); +} + ... - `Date: ${dayjs(details.startTime).format("MMM D, YYYY")} at ${dayjs(details.startTime).format("h:mm A")}\n` + + `Date: ${formatBookingStart(details.startTime)}\n` + ... - `Date: ${dayjs(details.startTime).format("MMM D, YYYY")} at ${dayjs(details.startTime).format("h:mm A")}\n` + + `Date: ${formatBookingStart(details.startTime)}\n` +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sms.ts` around lines 45 - 46, The SMS date/time formatting currently uses dayjs without timezone, causing environment-dependent output; update the module by importing and extending dayjs with the utc and timezone plugins (dayjs.extend(utc); dayjs.extend(timezone)) and change the formatting calls in sendBookingCreatedSms and sendBookingUpdatedSms to apply .tz('America/Edmonton') to details.startTime before calling .format(...) so the SMS always shows the booking time in the explicit America/Edmonton timezone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 28-32: Reorder the environment variable entries to satisfy
dotenv-linter ordering rules and add a trailing blank line at EOF: ensure the
keys GOOGLE_MAPS_API_KEY, TELNYX_API_KEY, and TELNYX_NUMBER appear in the
correct alphabetical order relative to the rest of the .env.example entries and
append a single blank line at the end of the file so the linter stops
complaining.
In `@src/lib/sms.ts`:
- Line 29: The error log is exposing full recipient phone numbers via the
console.error call that interpolates the "to" variable; replace that with a
redacted/masked version (e.g., implement and call a small helper like
maskPhoneNumber(to) that preserves only the last 2–4 digits and replaces the
rest with asterisks or Xs while keeping any country/formatting minimal) and use
the masked value in the console.error message for the "Failed to send SMS to
..." log; ensure the helper is used wherever the raw "to" variable would be
logged to avoid PII leakage.
- Around line 11-12: The Telnyx client is created without explicit timeout/retry
bounds causing parallel SMS sends (see Promise.all usage around SMS send code)
to stall; update the Telnyx instantiation (new Telnyx({...})) to include
sensible timeout and maxRetries options (e.g., timeout: 10000, maxRetries: 1)
when constructing conn, so globalForTelnyx.telnyx uses the bounded client in
non-production as well; keep the existing apiKey parameter and assignment to
globalForTelnyx.telnyx (and preserve the environment guard).
In `@src/server/api/routers/bookings.ts`:
- Around line 648-656: The update mutation is awaiting external SMS delivery
(await sendBookingUpdatedSms(...)) which adds latency and a post-commit failure
point; change this to a fire-and-forget call and do the same in the create flow
(sendBookingCreatedSms in trip.ts). Concretely, remove the await and invoke the
function without blocking (e.g., void sendBookingUpdatedSms(...) or
Promise.resolve().then(() => sendBookingUpdatedSms(...))), and ensure any
internal errors from sendBookingUpdatedSms and sendBookingCreatedSms are
handled/logged inside those functions (or by attaching a .catch(...) on the
non-awaited promise) so they don't surface to the mutation.
In `@src/server/api/routers/trip.ts`:
- Around line 47-55: The mutation currently awaits external SMS calls which
blocks the response; change the calls to sendBookingCreatedSms(...) and
sendBookingUpdatedSms(...) to fire-and-forget: invoke them without awaiting and
attach a .catch(...) that logs errors (using the existing logger) so any Telnyx
failures are recorded but do not affect the mutation response; ensure you pass
the same payload (title, startTime, pickupAddress, destinationAddress,
driverPhones, etc.) to the non-awaited call and do not alter the database write
order or error handling around the persistence logic.
---
Nitpick comments:
In `@src/lib/sms.ts`:
- Around line 45-46: The SMS date/time formatting currently uses dayjs without
timezone, causing environment-dependent output; update the module by importing
and extending dayjs with the utc and timezone plugins (dayjs.extend(utc);
dayjs.extend(timezone)) and change the formatting calls in sendBookingCreatedSms
and sendBookingUpdatedSms to apply .tz('America/Edmonton') to details.startTime
before calling .format(...) so the SMS always shows the booking time in the
explicit America/Edmonton timezone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8abe73f-e1b9-4e02-894e-bb900f131b68
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.env.examplepackage.jsonsrc/env.jssrc/lib/sms.tssrc/server/api/routers/bookings.tssrc/server/api/routers/trip.ts
promatty
reviewed
Mar 22, 2026
| @@ -633,6 +634,28 @@ export const bookingsRouter = createTRPCRouter({ | |||
| .returning(); | |||
| }); | |||
|
|
|||
Contributor
There was a problem hiding this comment.
maybe we should only notify on specific updates, like status (ie. if a ride is cancelled), start/end times changing, driverid change, or others you think are important. we just dont care to notify if there's a useless title change, etc
Contributor
Author
There was a problem hiding this comment.
I updated it so that SMS notifications only get sent out if the start time, pickup address, or destination address get updated. Let me know if this works or we want something different.
We could also just show the fields that have been updated in the text message. Something like:
Updates have been made on booking #[booking id]:
Start time: 12:00 PM -> 1:15 PM
Pickup address: location 1 -> location 2
Contributor
Author
There was a problem hiding this comment.
Also now that there is the driver_id field in the bookings table. Would it make sense to only notify drivers about their own bookings? How do drivers get assigned to bookings in the first place?
Contributor
There was a problem hiding this comment.
yeah i like showing the fields that changed, if it isnt too complicated to do. and lets keep notifying all drivers; there are only ever 1 or 2 drivers doing the trips
This was referenced Mar 22, 2026
Contributor
Author
|
Here is what the new SMS notifications look like: 
|
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/bookings.ts`:
- Around line 649-675: The driver phone lookup (ctx.db.select(...) mapping to
driverPhones) is still awaited after the booking update commits, so a select
failure can surface as a mutation error; move the driver lookup ahead of the
commit path: perform the ctx.db.select(...).map(...).filter(...) and build
driverPhones before applying the update (so use the same driverPhones variable
when calling sendBookingUpdatedSms), then keep the post-commit call to
sendBookingUpdatedSms(updated.id, beforeSnapshot, afterSnapshot, driverPhones)
wrapped in a non-throwing fire-and-forget with a catch; reference
sendBookingUpdatedSms, ctx.db.select, driverPhones, updated and existing to
locate and relocate the logic.
- Around line 641-669: The code compares the pre-transaction record `existing`
to `updated`, which can be stale and lead to incorrect SMS diffs; instead,
perform the change-detection inside the same transaction or re-fetch the current
DB row after applying updates and compare that fresh record to the incoming
updated payload. Specifically, move the start/end/pickup/destination change
checks (the `startChanged`, `endChanged`, `pickupChanged`, `destinationChanged`
logic and creation of `beforeSnapshot`/`afterSnapshot`) into the transaction
that updates the booking or replace `existing` with a fresh `ctx.db` read of the
booking row after the update; ensure you still derive `afterSnapshot` from the
authoritative post-update values and build `driverPhones` from `ctx.db` within
the same transactional context.
🪄 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: d8fa1b20-8c6d-4edc-93b3-cd657a23250b
📒 Files selected for processing (3)
src/lib/sms.tssrc/server/api/routers/bookings.tssrc/server/api/routers/trip.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/api/routers/trip.ts
- src/lib/sms.ts
Comment on lines
+641
to
+669
| const startChanged = | ||
| new Date(existing.startTime).getTime() !== new Date(updated.startTime).getTime(); | ||
| const endChanged = | ||
| new Date(existing.endTime).getTime() !== new Date(updated.endTime).getTime(); | ||
| const pickupChanged = existing.pickupAddress.trim() !== updated.pickupAddress.trim(); | ||
| const destinationChanged = | ||
| existing.destinationAddress.trim() !== updated.destinationAddress.trim(); | ||
|
|
||
| if (startChanged || endChanged || pickupChanged || destinationChanged) { | ||
| const drivers = await ctx.db | ||
| .select({ phoneNumber: user.phoneNumber }) | ||
| .from(user) | ||
| .where(eq(user.role, "driver")); | ||
| const driverPhones = drivers | ||
| .map((d) => d.phoneNumber) | ||
| .filter((n): n is string => Boolean(n?.trim())); | ||
|
|
||
| const beforeSnapshot = { | ||
| startTime: existing.startTime, | ||
| endTime: existing.endTime, | ||
| pickupAddress: existing.pickupAddress, | ||
| destinationAddress: existing.destinationAddress, | ||
| }; | ||
| const afterSnapshot = { | ||
| startTime: updated.startTime, | ||
| endTime: updated.endTime, | ||
| pickupAddress: updated.pickupAddress, | ||
| destinationAddress: updated.destinationAddress, | ||
| }; |
There was a problem hiding this comment.
Don't diff against a stale pre-transaction snapshot.
Lines 641-647 compare existing from before the transaction with updated from after it. A concurrent edit in between can make this SMS report fields that this mutation never touched.
Low-friction mitigation
- const startChanged =
- new Date(existing.startTime).getTime() !== new Date(updated.startTime).getTime();
- const endChanged =
- new Date(existing.endTime).getTime() !== new Date(updated.endTime).getTime();
- const pickupChanged = existing.pickupAddress.trim() !== updated.pickupAddress.trim();
- const destinationChanged =
- existing.destinationAddress.trim() !== updated.destinationAddress.trim();
+ const afterSnapshot = {
+ startTime: updatesToApply.startTime ?? existing.startTime,
+ endTime: updatesToApply.endTime ?? existing.endTime,
+ pickupAddress: updatesToApply.pickupAddress ?? existing.pickupAddress,
+ destinationAddress: updatesToApply.destinationAddress ?? existing.destinationAddress,
+ };
+ const startChanged =
+ new Date(existing.startTime).getTime() !== new Date(afterSnapshot.startTime).getTime();
+ const endChanged =
+ new Date(existing.endTime).getTime() !== new Date(afterSnapshot.endTime).getTime();
+ const pickupChanged =
+ existing.pickupAddress.trim() !== afterSnapshot.pickupAddress.trim();
+ const destinationChanged =
+ existing.destinationAddress.trim() !== afterSnapshot.destinationAddress.trim();
if (startChanged || endChanged || pickupChanged || destinationChanged) {
const beforeSnapshot = {
startTime: existing.startTime,
endTime: existing.endTime,
pickupAddress: existing.pickupAddress,
destinationAddress: existing.destinationAddress,
};
- const afterSnapshot = {
- startTime: updated.startTime,
- endTime: updated.endTime,
- pickupAddress: updated.pickupAddress,
- destinationAddress: updated.destinationAddress,
- };
void sendBookingUpdatedSms(updated.id, beforeSnapshot, afterSnapshot, driverPhones).catch(
(err) => {
console.error("Failed to send booking updated SMS:", err);
},🤖 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 641 - 669, The code compares
the pre-transaction record `existing` to `updated`, which can be stale and lead
to incorrect SMS diffs; instead, perform the change-detection inside the same
transaction or re-fetch the current DB row after applying updates and compare
that fresh record to the incoming updated payload. Specifically, move the
start/end/pickup/destination change checks (the `startChanged`, `endChanged`,
`pickupChanged`, `destinationChanged` logic and creation of
`beforeSnapshot`/`afterSnapshot`) into the transaction that updates the booking
or replace `existing` with a fresh `ctx.db` read of the booking row after the
update; ensure you still derive `afterSnapshot` from the authoritative
post-update values and build `driverPhones` from `ctx.db` within the same
transactional context.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/server/api/routers/bookings.ts (1)
651-671:⚠️ Potential issue | 🟡 MinorUse request-derived snapshots to avoid false SMS diffs.
Line 651 currently diffs a stale pre-transaction row (
existing) against post-update data (updated). Concurrent edits can leak unrelated changes into the SMS payload.Suggested low-friction fix
- const startChanged = - new Date(existing.startTime).getTime() !== new Date(updated.startTime).getTime(); - const endChanged = - new Date(existing.endTime).getTime() !== new Date(updated.endTime).getTime(); - const pickupChanged = existing.pickupAddress.trim() !== updated.pickupAddress.trim(); - const destinationChanged = - existing.destinationAddress.trim() !== updated.destinationAddress.trim(); + const beforeSnapshot = { + startTime: existing.startTime, + endTime: existing.endTime, + pickupAddress: existing.pickupAddress, + destinationAddress: existing.destinationAddress, + }; + const afterSnapshot = { + startTime: updatesToApply.startTime ?? existing.startTime, + endTime: updatesToApply.endTime ?? existing.endTime, + pickupAddress: updatesToApply.pickupAddress ?? existing.pickupAddress, + destinationAddress: updatesToApply.destinationAddress ?? existing.destinationAddress, + }; + const startChanged = + new Date(beforeSnapshot.startTime).getTime() !== new Date(afterSnapshot.startTime).getTime(); + const endChanged = + new Date(beforeSnapshot.endTime).getTime() !== new Date(afterSnapshot.endTime).getTime(); + const pickupChanged = + beforeSnapshot.pickupAddress.trim() !== afterSnapshot.pickupAddress.trim(); + const destinationChanged = + beforeSnapshot.destinationAddress.trim() !== afterSnapshot.destinationAddress.trim(); if (startChanged || endChanged || pickupChanged || destinationChanged) { - const beforeSnapshot = { - startTime: existing.startTime, - endTime: existing.endTime, - pickupAddress: existing.pickupAddress, - destinationAddress: existing.destinationAddress, - }; - const afterSnapshot = { - startTime: updated.startTime, - endTime: updated.endTime, - pickupAddress: updated.pickupAddress, - destinationAddress: updated.destinationAddress, - };🤖 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 651 - 671, The current diff compares the stale DB row `existing` to the post-update `updated` row which can include concurrent changes; instead build the "after" snapshot from the request-derived values (the incoming update payload) and compute change flags against `existing` using those request values. Concretely, replace usage of `updated` when computing `startChanged`, `endChanged`, `pickupChanged`, `destinationChanged` and when constructing `afterSnapshot` with the values from the request body (ensuring you parse/compare dates the same way and trim addresses like `pickupAddress.trim()`/`destinationAddress.trim()`), so the SMS diff only reflects what the requester intended. Ensure the code references the same unique symbols (`existing`, `afterSnapshot`, `beforeSnapshot`, `startChanged`, `endChanged`, `pickupChanged`, `destinationChanged`) when making the change.
🧹 Nitpick comments (1)
src/server/api/routers/bookings.ts (1)
680-685: Deduplicate recipient phone numbers before sending.If two driver records share the same number, this sends duplicate SMS notifications. Consider normalizing + deduping first.
Refactor suggestion
const driverPhones = drivers .map((d) => d.phoneNumber) .filter((n): n is string => Boolean(n?.trim())); + const uniqueDriverPhones = [...new Set(driverPhones.map((n) => n.trim()))]; - await sendBookingUpdatedSms(updated.id, beforeSnapshot, afterSnapshot, driverPhones); + await sendBookingUpdatedSms( + updated.id, + beforeSnapshot, + afterSnapshot, + uniqueDriverPhones, + );🤖 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 680 - 685, The code collects phone numbers into driverPhones and calls sendBookingUpdatedSms, but it can send duplicates when multiple drivers share the same number; normalize (trim, remove non-digit characters or convert to a consistent format) and deduplicate the driverPhones array before calling sendBookingUpdatedSms (e.g., map drivers → phoneNumber, normalize each string, filter out empty values, then use a Set to remove duplicates) so sendBookingUpdatedSms receives only unique, normalized recipient numbers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 651-671: The current diff compares the stale DB row `existing` to
the post-update `updated` row which can include concurrent changes; instead
build the "after" snapshot from the request-derived values (the incoming update
payload) and compute change flags against `existing` using those request values.
Concretely, replace usage of `updated` when computing `startChanged`,
`endChanged`, `pickupChanged`, `destinationChanged` and when constructing
`afterSnapshot` with the values from the request body (ensuring you
parse/compare dates the same way and trim addresses like
`pickupAddress.trim()`/`destinationAddress.trim()`), so the SMS diff only
reflects what the requester intended. Ensure the code references the same unique
symbols (`existing`, `afterSnapshot`, `beforeSnapshot`, `startChanged`,
`endChanged`, `pickupChanged`, `destinationChanged`) when making the change.
---
Nitpick comments:
In `@src/server/api/routers/bookings.ts`:
- Around line 680-685: The code collects phone numbers into driverPhones and
calls sendBookingUpdatedSms, but it can send duplicates when multiple drivers
share the same number; normalize (trim, remove non-digit characters or convert
to a consistent format) and deduplicate the driverPhones array before calling
sendBookingUpdatedSms (e.g., map drivers → phoneNumber, normalize each string,
filter out empty values, then use a Set to remove duplicates) so
sendBookingUpdatedSms receives only unique, normalized recipient numbers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce522a34-2f53-4de5-9fd9-c61706f1a6ed
📒 Files selected for processing (1)
src/server/api/routers/bookings.ts
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.
https://codethechangeyyc.atlassian.net/browse/SANC-91
Added SMS notifications using Telnyx whenever a booking is created or updated.
Example:
Summary by CodeRabbit
New Features
Chores