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

fix(reminders): replace N+1 per-appointment queries with single JOIN query #306

Merged
groombook-engineer[bot] merged 2 commits from fix/gro-639-n-plus-one-v2 into dev 2026-04-17 10:45:17 +00:00
Conversation 14 Commits 2 Files Changed 2
+80 -70
2 changed files with 80 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/workflows/ci.yml
+2 -2
View File
@@ -2,9 +2,9 @@ name: CI
on:
push:
branches: [main]
branches: [main, dev]
pull_request:
branches: [main]
branches: [main, dev]
workflow_dispatch:
inputs:
ref:
apps/api/src/services/reminders.ts
+78 -68
View File
@@ -5,6 +5,7 @@ import {
eq,
getDb,
gte,
inArray,
lt,
appointments,
clients,
@@ -59,68 +60,77 @@ export async function runReminderCheck(): Promise<void> {
)
greptile-apps[bot] commented 2026-04-16 15:50:30 +00:00 (Migrated from github.com)
Review
Copy Link
Copy Source

P1 inArray already available and used elsewhere in the codebase

The rest of the codebase uses inArray for exactly this pattern — payment.ts:62, portal.ts:64-65, and portal.ts:103,392 all import and use inArray from @groombook/db. In fact, inArray is already exported from @groombook/db (alongside sql).

The current implementation uses a ternary that switches between eq() for the single-ID case and a raw sql template for the multi-ID case. Drizzle's inArray handles both uniformly and generates a safe parameterized IN (...) clause, avoiding any ambiguity about how the driver serializes a JavaScript array into a PostgreSQL = ANY($1) literal.

If the array parameter isn't serialized correctly by the driver, sentRows could silently return an empty set, meaning sentEmail and sentSms would always be empty — causing duplicate reminders on every scheduler tick.

          inArray(reminderLogs.appointmentId, appointmentIds)

You'd need to add inArray to the import at line 7 and drop the now-unused sql import.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/api/src/services/reminders.ts
Line: 73-76

Comment:
**`inArray` already available and used elsewhere in the codebase**

The rest of the codebase uses `inArray` for exactly this pattern — `payment.ts:62`, `portal.ts:64-65`, and `portal.ts:103,392` all import and use `inArray` from `@groombook/db`. In fact, `inArray` is already exported from `@groombook/db` (alongside `sql`).

The current implementation uses a ternary that switches between `eq()` for the single-ID case and a raw `sql` template for the multi-ID case. Drizzle's `inArray` handles both uniformly and generates a safe parameterized `IN (...)` clause, avoiding any ambiguity about how the driver serializes a JavaScript array into a PostgreSQL `= ANY($1)` literal.

If the array parameter isn't serialized correctly by the driver, `sentRows` could silently return an empty set, meaning `sentEmail` and `sentSms` would always be empty — causing duplicate reminders on every scheduler tick.

```suggestion
          inArray(reminderLogs.appointmentId, appointmentIds)
```

You'd need to add `inArray` to the import at line 7 and drop the now-unused `sql` import.

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> **`inArray` already available and used elsewhere in the codebase** The rest of the codebase uses `inArray` for exactly this pattern — `payment.ts:62`, `portal.ts:64-65`, and `portal.ts:103,392` all import and use `inArray` from `@groombook/db`. In fact, `inArray` is already exported from `@groombook/db` (alongside `sql`). The current implementation uses a ternary that switches between `eq()` for the single-ID case and a raw `sql` template for the multi-ID case. Drizzle's `inArray` handles both uniformly and generates a safe parameterized `IN (...)` clause, avoiding any ambiguity about how the driver serializes a JavaScript array into a PostgreSQL `= ANY($1)` literal. If the array parameter isn't serialized correctly by the driver, `sentRows` could silently return an empty set, meaning `sentEmail` and `sentSms` would always be empty — causing duplicate reminders on every scheduler tick. ```suggestion inArray(reminderLogs.appointmentId, appointmentIds) ``` You'd need to add `inArray` to the import at line 7 and drop the now-unused `sql` import. <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: 73-76 Comment: **`inArray` already available and used elsewhere in the codebase** The rest of the codebase uses `inArray` for exactly this pattern — `payment.ts:62`, `portal.ts:64-65`, and `portal.ts:103,392` all import and use `inArray` from `@groombook/db`. In fact, `inArray` is already exported from `@groombook/db` (alongside `sql`). The current implementation uses a ternary that switches between `eq()` for the single-ID case and a raw `sql` template for the multi-ID case. Drizzle's `inArray` handles both uniformly and generates a safe parameterized `IN (...)` clause, avoiding any ambiguity about how the driver serializes a JavaScript array into a PostgreSQL `= ANY($1)` literal. If the array parameter isn't serialized correctly by the driver, `sentRows` could silently return an empty set, meaning `sentEmail` and `sentSms` would always be empty — causing duplicate reminders on every scheduler tick. ```suggestion inArray(reminderLogs.appointmentId, appointmentIds) ``` You'd need to add `inArray` to the import at line 7 and drop the now-unused `sql` import. How can I resolve this? If you propose a fix, please make it concise. ````` </details>
greptile-apps[bot] commented 2026-04-16 15:50:34 +00:00 (Migrated from github.com)
Review
Copy Link
Copy Source

P2 Early-exit never fires for email-only clients

This optimization skips an appointment only when both sentEmail and sentSms contain its ID. For a client with smsOptIn = false, a sms row is never written to reminderLogs, so sentSms will never contain that appointment ID — meaning the early exit never triggers for the common email-only case.

No duplicate sends occur (the per-item guards on lines 142 and 167 handle that), but the short-circuit is dead code for email-only clients.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/api/src/services/reminders.ts
Line: 125

Comment:
**Early-exit never fires for email-only clients**

This optimization skips an appointment only when **both** `sentEmail` and `sentSms` contain its ID. For a client with `smsOptIn = false`, a `sms` row is never written to `reminderLogs`, so `sentSms` will never contain that appointment ID — meaning the early exit never triggers for the common email-only case.

No duplicate sends occur (the per-item guards on lines 142 and 167 handle that), but the short-circuit is dead code for email-only clients.

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> **Early-exit never fires for email-only clients** This optimization skips an appointment only when **both** `sentEmail` and `sentSms` contain its ID. For a client with `smsOptIn = false`, a `sms` row is never written to `reminderLogs`, so `sentSms` will never contain that appointment ID — meaning the early exit never triggers for the common email-only case. No duplicate sends occur (the per-item guards on lines 142 and 167 handle that), but the short-circuit is dead code for email-only clients. <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: 125 Comment: **Early-exit never fires for email-only clients** This optimization skips an appointment only when **both** `sentEmail` and `sentSms` contain its ID. For a client with `smsOptIn = false`, a `sms` row is never written to `reminderLogs`, so `sentSms` will never contain that appointment ID — meaning the early exit never triggers for the common email-only case. No duplicate sends occur (the per-item guards on lines 142 and 167 handle that), but the short-circuit is dead code for email-only clients. How can I resolve this? If you propose a fix, please make it concise. ````` </details>
);
const appointmentIds: string[] = upcoming.map((a) => a.id as string);
if (appointmentIds.length === 0) continue;
// Bulk check: which appointments already have email and SMS reminders sent?
const sentRows = 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]!)
: inArray(reminderLogs.appointmentId, appointmentIds)
)
);
const sentEmail = new Set(
sentRows.filter((r) => r.channel === "email").map((r) => r.appointmentId)
);
const sentSms = new Set(
sentRows.filter((r) => r.channel === "sms").map((r) => r.appointmentId)
);
// Bulk JOIN: fetch all client/pet/service/staff data in one 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,
clientSmsOptIn: clients.smsOptIn,
clientPhone: clients.phone,
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(
gte(appointments.startTime, windowStart),
lt(appointments.startTime, windowEnd),
eq(appointments.status, "scheduled")
)
);
const appointmentMap = new Map<string, typeof joinedRows[number]>();
for (const row of joinedRows) {
appointmentMap.set(row.appointmentId, row);
}
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);
const joined = appointmentMap.get(appt.id as string);
if (!joined) 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 { clientName, clientEmail, clientEmailOptOut, clientSmsOptIn, clientPhone, petName, serviceName, staffName } = joined;
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 (!clientEmail || clientEmailOptOut) continue;
if (!petName || !serviceName) continue;
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 emailSent = sentEmail.has(appt.id as string);
const smsSent = sentSms.has(appt.id as string);
let confirmationToken = appt.confirmationToken;
if (!confirmationToken) {
@@ -131,15 +141,15 @@ export async function runReminderCheck(): Promise<void> {
.where(eq(appointments.id, appt.id));
}
if (!emailLog) {
if (!emailSent) {
const sent = await sendEmail(
buildReminderEmail(
client.email,
clientEmail,
{
clientName: client.name,
petName: pet.name,
serviceName: service.name,
groomerName,
clientName,
petName,
serviceName,
groomerName: staffName,
startTime: appt.startTime,
},
window.hours,
@@ -155,20 +165,20 @@ export async function runReminderCheck(): Promise<void> {
}
}
if (!smsLog && client.smsOptIn && client.phone) {
if (!smsSent && clientSmsOptIn && clientPhone) {
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}${staffName ? ` with ${staffName}` : ""}`,
`Confirm: ${confirmUrl}`,
`Cancel: ${cancelUrl}`,
TCPA_OPT_OUT,
].join(". ");
try {
const smsOk = await smsSend(client.phone, smsBody);
const smsOk = await smsSend(clientPhone, smsBody);
if (smsOk) {
await db
.insert(reminderLogs)