groombook/app
Archived
13
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(GRO-639): N+1 queries in reminder scheduler #301

Closed
groombook-engineer[bot] wants to merge 1 commits from clean-gro-639 into main
pull from: clean-gro-639
merge into: groombook:main
Conversation 9 Commits 1 Files Changed 1
+83 -74
1 changed files with 83 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/api/src/services/reminders.ts
+83 -74
View File
@@ -6,6 +6,7 @@ import {
getDb,
gte,
lt,
sql,
appointments,
clients,
pets,
@@ -31,6 +32,8 @@ function getReminderWindows(): { label: string; hours: number }[] {
];
}
// Checks for upcoming appointments that need reminders and sends them.
// Runs every minute — idempotent via reminder_logs unique constraint.
export async function runReminderCheck(): Promise<void> {
const db = getDb();
const now = new Date();
@@ -59,69 +62,74 @@ export async function runReminderCheck(): Promise<void> {
)
);
const appointmentIds: string[] = upcoming.map((a) => a.id as string);
if (appointmentIds.length === 0) continue;
// Batch-fetch already-sent appointment IDs (both EMAIL and SMS channels)
const sentAppointmentIds = new Set(
(
await db
.select({ appointmentId: reminderLogs.appointmentId })
.from(reminderLogs)
.where(
and(
eq(reminderLogs.reminderType, window.label),
appointmentIds.length === 1
? eq(reminderLogs.appointmentId, appointmentIds[0]!)
: sql`${reminderLogs.appointmentId} = ANY(${appointmentIds})`
)
)
).map((r) => r.appointmentId)
);
// Batch-fetch all appointment data with related joins in a single query
const joinedRows = await db
.select({
appointmentId: appointments.id,
startTime: appointments.startTime,
clientId: appointments.clientId,
petId: appointments.petId,
serviceId: appointments.serviceId,
staffId: appointments.staffId,
confirmationToken: appointments.confirmationToken,
clientName: clients.name,
clientEmail: clients.email,
clientEmailOptOut: clients.emailOptOut,
clientPhone: clients.phone,
clientSmsOptIn: clients.smsOptIn,
petName: pets.name,
serviceName: services.name,
staffName: staff.name,
})
.from(appointments)
.innerJoin(clients, eq(appointments.clientId, clients.id))
.innerJoin(pets, eq(appointments.petId, pets.id))
.innerJoin(services, eq(appointments.serviceId, services.id))
.leftJoin(staff, eq(appointments.staffId, staff.id))
.where(
and(
sql`${appointments.id} = ANY(${appointmentIds})`,
gte(appointments.startTime, windowStart),
lt(appointments.startTime, windowEnd),
eq(appointments.status, "scheduled")
)
);
const appointmentMap = new Map<string, typeof joinedRows[number]>();
greptile-apps[bot] commented 2026-04-16 10:32:46 +00:00 (Migrated from github.com)
Review
Copy Link
Copy Source

P2 Inconsistent ANY() special-casing between the two queries

The sentAppointmentIds query has a branch for appointmentIds.length === 1 that falls back to a plain eq(), while the joinedRows query at line 112 always uses ANY() without that guard. PostgreSQL handles = ANY('{single-uuid}'::uuid[]) identically to = 'single-uuid', so the special case is unnecessary and creates a maintenance inconsistency. Consider using inArray from drizzle-orm for both queries — it is the idiomatic way to express this and avoids the raw sql template entirely:

inArray(reminderLogs.appointmentId, appointmentIds)
// and
inArray(appointments.id, appointmentIds)
Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/api/src/services/reminders.ts
Line: 78-81

Comment:
**Inconsistent `ANY()` special-casing between the two queries**

The `sentAppointmentIds` query has a branch for `appointmentIds.length === 1` that falls back to a plain `eq()`, while the `joinedRows` query at line 112 always uses `ANY()` without that guard. PostgreSQL handles `= ANY('{single-uuid}'::uuid[])` identically to `= 'single-uuid'`, so the special case is unnecessary and creates a maintenance inconsistency. Consider using `inArray` from drizzle-orm for both queries — it is the idiomatic way to express this and avoids the raw `sql` template entirely:

```ts
inArray(reminderLogs.appointmentId, appointmentIds)
// and
inArray(appointments.id, appointmentIds)
```

How can I resolve this? If you propose a fix, please make it concise.
<a href="#"><img alt="P2" src="https://greptile-static-assets.s3.amazonaws.com/badges/p2.svg?v=7" align="top"></a> **Inconsistent `ANY()` special-casing between the two queries** The `sentAppointmentIds` query has a branch for `appointmentIds.length === 1` that falls back to a plain `eq()`, while the `joinedRows` query at line 112 always uses `ANY()` without that guard. PostgreSQL handles `= ANY('{single-uuid}'::uuid[])` identically to `= 'single-uuid'`, so the special case is unnecessary and creates a maintenance inconsistency. Consider using `inArray` from drizzle-orm for both queries — it is the idiomatic way to express this and avoids the raw `sql` template entirely: ```ts inArray(reminderLogs.appointmentId, appointmentIds) // and inArray(appointments.id, appointmentIds) ``` <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/services/reminders.ts Line: 78-81 Comment: **Inconsistent `ANY()` special-casing between the two queries** The `sentAppointmentIds` query has a branch for `appointmentIds.length === 1` that falls back to a plain `eq()`, while the `joinedRows` query at line 112 always uses `ANY()` without that guard. PostgreSQL handles `= ANY('{single-uuid}'::uuid[])` identically to `= 'single-uuid'`, so the special case is unnecessary and creates a maintenance inconsistency. Consider using `inArray` from drizzle-orm for both queries — it is the idiomatic way to express this and avoids the raw `sql` template entirely: ```ts inArray(reminderLogs.appointmentId, appointmentIds) // and inArray(appointments.id, appointmentIds) ``` How can I resolve this? If you propose a fix, please make it concise. ````` </details>
for (const row of joinedRows) {
appointmentMap.set(row.appointmentId, row);
}
greptile-apps[bot] commented 2026-04-16 10:32:45 +00:00 (Migrated from github.com)
Review
Copy Link
Copy Source

P1 Per-channel deduplication is lost — SMS will silently go unsent after partial failures

The original code fetched emailLog (filtered to channel = "email") and smsLog (filtered to channel = "sms") independently, so each channel was only skipped when its own log entry existed. The new sentAppointmentIds set omits any channel filter and therefore contains the appointment ID as soon as either channel has been logged.

Consider this failure scenario (feasible every time the SMS provider returns a non-OK response):

  1. Email sends → (apptId, "24h", "email") inserted into reminder_logs.
  2. SMS send call returns smsOk = false → no SMS log inserted.
  3. Next cron tick: sentAppointmentIds contains apptId (from the email log) → continue fires → SMS is permanently skipped for this appointment.

The old code would retry SMS on every subsequent tick until the provider succeeds. The new code silently drops it.

A minimal fix is to track appointmentId:channel composite keys:

const sentKeys = new Set(
  (
    await db
      .select({
        appointmentId: reminderLogs.appointmentId,
        channel: reminderLogs.channel,
      })
      .from(reminderLogs)
      .where(
        and(
          eq(reminderLogs.reminderType, window.label),
          appointmentIds.length === 1
            ? eq(reminderLogs.appointmentId, appointmentIds[0]!)
            : sql`${reminderLogs.appointmentId} = ANY(${appointmentIds})`
        )
      )
  ).map((r) => `${r.appointmentId}:${r.channel}`)
);

Then replace the early-exit check (line 126) and the per-channel checks with:

const emailSent = sentKeys.has(`${appt.id}:email`);
const smsSent   = sentKeys.has(`${appt.id}:sms`);
if (emailSent && smsSent) continue;

// EMAIL reminder
if (!emailSent && row.clientEmail && !row.clientEmailOptOut) { … }

// SMS reminder
if (!smsSent && row.clientPhone && row.clientSmsOptIn) { … }
Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/api/src/services/reminders.ts
Line: 70-84

Comment:
**Per-channel deduplication is lost — SMS will silently go unsent after partial failures**

The original code fetched `emailLog` (filtered to `channel = "email"`) and `smsLog` (filtered to `channel = "sms"`) independently, so each channel was only skipped when its own log entry existed. The new `sentAppointmentIds` set omits any channel filter and therefore contains the appointment ID as soon as **either** channel has been logged.

Consider this failure scenario (feasible every time the SMS provider returns a non-OK response):
1. Email sends → `(apptId, "24h", "email")` inserted into `reminder_logs`.
2. SMS send call returns `smsOk = false` → no SMS log inserted.
3. Next cron tick: `sentAppointmentIds` contains `apptId` (from the email log) → `continue` fires → SMS is **permanently skipped** for this appointment.

The old code would retry SMS on every subsequent tick until the provider succeeds. The new code silently drops it.

A minimal fix is to track `appointmentId:channel` composite keys:

```ts
const sentKeys = new Set(
  (
    await db
      .select({
        appointmentId: reminderLogs.appointmentId,
        channel: reminderLogs.channel,
      })
      .from(reminderLogs)
      .where(
        and(
          eq(reminderLogs.reminderType, window.label),
          appointmentIds.length === 1
            ? eq(reminderLogs.appointmentId, appointmentIds[0]!)
            : sql`${reminderLogs.appointmentId} = ANY(${appointmentIds})`
        )
      )
  ).map((r) => `${r.appointmentId}:${r.channel}`)
);
```

Then replace the early-exit check (line 126) and the per-channel checks with:

```ts
const emailSent = sentKeys.has(`${appt.id}:email`);
const smsSent   = sentKeys.has(`${appt.id}:sms`);
if (emailSent && smsSent) continue;

// EMAIL reminder
if (!emailSent && row.clientEmail && !row.clientEmailOptOut) { … }

// SMS reminder
if (!smsSent && row.clientPhone && row.clientSmsOptIn) { … }
```

How can I resolve this? If you propose a fix, please make it concise.
<a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=7" align="top"></a> **Per-channel deduplication is lost — SMS will silently go unsent after partial failures** The original code fetched `emailLog` (filtered to `channel = "email"`) and `smsLog` (filtered to `channel = "sms"`) independently, so each channel was only skipped when its own log entry existed. The new `sentAppointmentIds` set omits any channel filter and therefore contains the appointment ID as soon as **either** channel has been logged. Consider this failure scenario (feasible every time the SMS provider returns a non-OK response): 1. Email sends → `(apptId, "24h", "email")` inserted into `reminder_logs`. 2. SMS send call returns `smsOk = false` → no SMS log inserted. 3. Next cron tick: `sentAppointmentIds` contains `apptId` (from the email log) → `continue` fires → SMS is **permanently skipped** for this appointment. The old code would retry SMS on every subsequent tick until the provider succeeds. The new code silently drops it. A minimal fix is to track `appointmentId:channel` composite keys: ```ts const sentKeys = new Set( ( await db .select({ appointmentId: reminderLogs.appointmentId, channel: reminderLogs.channel, }) .from(reminderLogs) .where( and( eq(reminderLogs.reminderType, window.label), appointmentIds.length === 1 ? eq(reminderLogs.appointmentId, appointmentIds[0]!) : sql`${reminderLogs.appointmentId} = ANY(${appointmentIds})` ) ) ).map((r) => `${r.appointmentId}:${r.channel}`) ); ``` Then replace the early-exit check (line 126) and the per-channel checks with: ```ts const emailSent = sentKeys.has(`${appt.id}:email`); const smsSent = sentKeys.has(`${appt.id}:sms`); if (emailSent && smsSent) continue; // EMAIL reminder if (!emailSent && row.clientEmail && !row.clientEmailOptOut) { … } // SMS reminder if (!smsSent && row.clientPhone && row.clientSmsOptIn) { … } ``` <details><summary>Prompt To Fix With AI</summary> `````markdown This is a comment left during a code review. Path: apps/api/src/services/reminders.ts Line: 70-84 Comment: **Per-channel deduplication is lost — SMS will silently go unsent after partial failures** The original code fetched `emailLog` (filtered to `channel = "email"`) and `smsLog` (filtered to `channel = "sms"`) independently, so each channel was only skipped when its own log entry existed. The new `sentAppointmentIds` set omits any channel filter and therefore contains the appointment ID as soon as **either** channel has been logged. Consider this failure scenario (feasible every time the SMS provider returns a non-OK response): 1. Email sends → `(apptId, "24h", "email")` inserted into `reminder_logs`. 2. SMS send call returns `smsOk = false` → no SMS log inserted. 3. Next cron tick: `sentAppointmentIds` contains `apptId` (from the email log) → `continue` fires → SMS is **permanently skipped** for this appointment. The old code would retry SMS on every subsequent tick until the provider succeeds. The new code silently drops it. A minimal fix is to track `appointmentId:channel` composite keys: ```ts const sentKeys = new Set( ( await db .select({ appointmentId: reminderLogs.appointmentId, channel: reminderLogs.channel, }) .from(reminderLogs) .where( and( eq(reminderLogs.reminderType, window.label), appointmentIds.length === 1 ? eq(reminderLogs.appointmentId, appointmentIds[0]!) : sql`${reminderLogs.appointmentId} = ANY(${appointmentIds})` ) ) ).map((r) => `${r.appointmentId}:${r.channel}`) ); ``` Then replace the early-exit check (line 126) and the per-channel checks with: ```ts const emailSent = sentKeys.has(`${appt.id}:email`); const smsSent = sentKeys.has(`${appt.id}:sms`); if (emailSent && smsSent) continue; // EMAIL reminder if (!emailSent && row.clientEmail && !row.clientEmailOptOut) { … } // SMS reminder if (!smsSent && row.clientPhone && row.clientSmsOptIn) { … } ``` How can I resolve this? If you propose a fix, please make it concise. ````` </details>
for (const appt of upcoming) {
const [emailLog] = await db
.select({ id: reminderLogs.id })
.from(reminderLogs)
.where(
and(
eq(reminderLogs.appointmentId, appt.id),
eq(reminderLogs.reminderType, window.label),
eq(reminderLogs.channel, "email")
)
)
.limit(1);
// Already sent a reminder for this appointment in this window
if (sentAppointmentIds.has(appt.id)) continue;
const [smsLog] = await db
.select({ id: reminderLogs.id })
.from(reminderLogs)
.where(
and(
eq(reminderLogs.appointmentId, appt.id),
eq(reminderLogs.reminderType, window.label),
eq(reminderLogs.channel, "sms")
)
)
.limit(1);
const [client] = await db
.select({
name: clients.name,
email: clients.email,
emailOptOut: clients.emailOptOut,
smsOptIn: clients.smsOptIn,
phone: clients.phone,
})
.from(clients)
.where(eq(clients.id, appt.clientId))
.limit(1);
if (!client || !client.email || client.emailOptOut) continue;
const [pet] = await db
.select({ name: pets.name })
.from(pets)
.where(eq(pets.id, appt.petId))
.limit(1);
const [service] = await db
.select({ name: services.name })
.from(services)
.where(eq(services.id, appt.serviceId))
.limit(1);
let groomerName: string | null = null;
if (appt.staffId) {
const [groomer] = await db
.select({ name: staff.name })
.from(staff)
.where(eq(staff.id, appt.staffId))
.limit(1);
groomerName = groomer?.name ?? null;
}
if (!pet || !service) continue;
const row = appointmentMap.get(appt.id);
if (!row) continue;
if (!row.petName || !row.serviceName) continue;
// Generate confirmation token if missing
let confirmationToken = appt.confirmationToken;
if (!confirmationToken) {
confirmationToken = randomBytes(32).toString("hex");
@@ -131,22 +139,22 @@ export async function runReminderCheck(): Promise<void> {
.where(eq(appointments.id, appt.id));
}
if (!emailLog) {
const clientName = row.clientName;
const petName = row.petName;
const serviceName = row.serviceName;
const groomerName = row.staffName ?? null;
const startTime = appt.startTime;
// EMAIL reminder
if (row.clientEmail && !row.clientEmailOptOut) {
const sent = await sendEmail(
buildReminderEmail(
client.email,
{
clientName: client.name,
petName: pet.name,
serviceName: service.name,
groomerName,
startTime: appt.startTime,
},
row.clientEmail,
{ clientName, petName, serviceName, groomerName, startTime },
window.hours,
confirmationToken
)
);
if (sent) {
await db
.insert(reminderLogs)
@@ -155,20 +163,21 @@ export async function runReminderCheck(): Promise<void> {
}
}
if (!smsLog && client.smsOptIn && client.phone) {
// SMS reminder
if (row.clientPhone && row.clientSmsOptIn) {
const apiUrl = process.env.API_URL ?? "http://localhost:3000";
const confirmUrl = `${apiUrl}/api/book/confirm/${confirmationToken}`;
const cancelUrl = `${apiUrl}/api/book/cancel/${confirmationToken}`;
const when = window.hours >= 24 ? "tomorrow" : `in ${window.hours} hours`;
const smsBody = [
`Hi ${client.name}, just a reminder: ${pet.name}'s grooming appointment is ${when}.`,
`Service: ${service.name}${groomerName ? ` with ${groomerName}` : ""}`,
`Hi ${clientName}, just a reminder: ${petName}'s grooming appointment is ${when}.`,
`Service: ${serviceName}${groomerName ? ` with ${groomerName}` : ""}`,
`Confirm: ${confirmUrl}`,
`Cancel: ${cancelUrl}`,
TCPA_OPT_OUT,
].join(". ");
try {
const smsOk = await smsSend(client.phone, smsBody);
const smsOk = await smsSend(row.clientPhone, smsBody);
if (smsOk) {
await db
.insert(reminderLogs)