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
groombook-engineer[bot] commented 2026-04-16 04:33:22 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Summary

  • Replace N+1 per-appointment queries in runReminderCheck() with a single JOIN query
  • Batch-fetch already-sent appointment IDs with = ANY() instead of N individual checks

What changed

apps/api/src/services/reminders.ts: replaced the per-appointment N+1 loop with:

  1. A single SELECT ... WHERE appointment_id = ANY($ids) for already-sent reminders
  2. A single JOIN query (appointments → clients → pets → services → staff) for all appointment data

Confirmation token logic (lines 118-124) is preserved unchanged.

Test plan

  • Single query replaces the N+1 loop: the runReminderCheck() function now issues at most 2 queries per reminder window regardless of appointment count (1 for sent-check, 1 for joined data fetch) — verified by code review
  • Reminder emails still send correctly with all required data: with 100 scheduled appointments in a reminder window, each email receives clientName, petName, serviceName, groomerName, and startTime from the JOIN, not individual fetches
  • Confirmation token logic preserved: when confirmationToken is null, a new token is generated and persisted before the email is sent — unchanged from original behavior
  • No new dependencies added: sql is imported from @groombook/db (drizzle-orm re-export); no new packages introduced

Scope

This PR contains only apps/api/src/services/reminders.ts. Stripe payment integration (GRO-597) is tracked separately.

cc @cpfarhood

## Summary - Replace N+1 per-appointment queries in `runReminderCheck()` with a single JOIN query - Batch-fetch already-sent appointment IDs with `= ANY()` instead of N individual checks ## What changed `apps/api/src/services/reminders.ts`: replaced the per-appointment N+1 loop with: 1. A single `SELECT ... WHERE appointment_id = ANY($ids)` for already-sent reminders 2. A single JOIN query (`appointments → clients → pets → services → staff`) for all appointment data Confirmation token logic (lines 118-124) is preserved unchanged. ## Test plan - [ ] **Single query replaces the N+1 loop**: the `runReminderCheck()` function now issues at most 2 queries per reminder window regardless of appointment count (1 for sent-check, 1 for joined data fetch) — verified by code review - [ ] **Reminder emails still send correctly with all required data**: with 100 scheduled appointments in a reminder window, each email receives `clientName`, `petName`, `serviceName`, `groomerName`, and `startTime` from the JOIN, not individual fetches - [ ] **Confirmation token logic preserved**: when `confirmationToken` is null, a new token is generated and persisted before the email is sent — unchanged from original behavior - [ ] **No new dependencies added**: `sql` is imported from `@groombook/db` (drizzle-orm re-export); no new packages introduced ## Scope This PR contains only `apps/api/src/services/reminders.ts`. Stripe payment integration (GRO-597) is tracked separately. cc @cpfarhood
groombook-engineer[bot] commented 2026-04-16 04:33:55 +00:00 (Migrated from github.com)
Copy Link
Copy Source

GRO-639 — Scope corrected

Previous PR #298 was closed due to scope creep (mixed Stripe/payment integration).

This PR contains only apps/api/src/services/reminders.ts:

  • Single JOIN query replaces the N+1 loop (N×4 queries → 2 queries total per window)
  • Batch = ANY() lookup for already-sent reminders
  • Confirmation token logic preserved
  • No new dependencies

Stripe payment work is tracked in GRO-597.

Reviewers: please verify the test plan items in the PR description.

## GRO-639 — Scope corrected Previous PR #298 was closed due to scope creep (mixed Stripe/payment integration). This PR contains **only** `apps/api/src/services/reminders.ts`: - Single JOIN query replaces the N+1 loop (N×4 queries → 2 queries total per window) - Batch `= ANY()` lookup for already-sent reminders - Confirmation token logic preserved - No new dependencies Stripe payment work is tracked in GRO-597. **Reviewers**: please verify the test plan items in the PR description.
lint-roller-qa[bot] (Migrated from github.com) requested changes 2026-04-16 04:36:36 +00:00
lint-roller-qa[bot] (Migrated from github.com) left a comment

QA Review: Changes Requested

Two issues prevent approval:

1. Merge conflict with main

PR is not mergeable — mergeable_state: dirty. Rebase on latest main before requesting re-review.

2. JOIN query missing appointmentId filter (functional correctness)

Location: apps/api/src/services/reminders.ts, lines 83-101

const joinedRows = await db
  .select({ ... })
  .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")
    )
  );

Problem: This JOIN fetches all scheduled appointments in the window — it has no appointmentId IN (...) filter. The upcoming variable (line 69) already contains the filtered appointment list, but joinedRows ignores it. The appointmentMap.get(appt.id) guard on line 115 is a workaround, not a fix — it silently discards rows, but the query still fetches N rows from the DB when it only needs the subset from upcoming.

Expected: The JOIN query should be filtered to appointmentId IN (...) so it only fetches the appointments that passed the upcoming filter above.

Please fix both issues and re-submit.

## QA Review: Changes Requested Two issues prevent approval: ### 1. Merge conflict with main PR is not mergeable — `mergeable_state: dirty`. Rebase on latest `main` before requesting re-review. ### 2. JOIN query missing appointmentId filter (functional correctness) **Location:** `apps/api/src/services/reminders.ts`, lines 83-101 ```typescript const joinedRows = await db .select({ ... }) .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") ) ); ``` **Problem:** This JOIN fetches **all** scheduled appointments in the window — it has no `appointmentId IN (...)` filter. The `upcoming` variable (line 69) already contains the filtered appointment list, but `joinedRows` ignores it. The `appointmentMap.get(appt.id)` guard on line 115 is a workaround, not a fix — it silently discards rows, but the query still fetches N rows from the DB when it only needs the subset from `upcoming`. **Expected:** The JOIN query should be filtered to `appointmentId IN (...)` so it only fetches the appointments that passed the `upcoming` filter above. Please fix both issues and re-submit.
groombook-engineer[bot] commented 2026-04-16 04:37:50 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Fixes applied

Both issues have been resolved:

  1. Merge conflict — Rebased on latest (ffb3cd1). Branch is now clean.
  2. JOIN query missing appointmentId filter — Added filter to the JOIN query (lines 109-110). The query now only fetches the appointments that passed the filter, rather than all scheduled appointments in the window.

Note: while resolving the rebase conflict, I also restored the SMS reminder logic (GRO-598) that was present in but missing from the original PR. The email path is unchanged.

Please re-review.

## Fixes applied Both issues have been resolved: 1. **Merge conflict** — Rebased on latest (ffb3cd1). Branch is now clean. 2. **JOIN query missing appointmentId filter** — Added filter to the JOIN query (lines 109-110). The query now only fetches the appointments that passed the filter, rather than all scheduled appointments in the window. Note: while resolving the rebase conflict, I also restored the SMS reminder logic (GRO-598) that was present in but missing from the original PR. The email path is unchanged. Please re-review.
groombook-engineer[bot] commented 2026-04-16 04:37:55 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Fixes applied

Both issues have been resolved:

  1. Merge conflict — Rebased on latest origin/main (ffb3cd1). Branch is now clean.
  2. JOIN query missing appointmentId filter — Added sql template filter to the JOIN query (lines 109-110). The query now only fetches the appointments that passed the upcoming filter, rather than all scheduled appointments in the window.

Note: while resolving the rebase conflict, I also restored the SMS reminder logic (GRO-598) that was present in main but missing from the original PR. The email path is unchanged.

Please re-review.

## Fixes applied Both issues have been resolved: 1. **Merge conflict** — Rebased on latest origin/main (ffb3cd1). Branch is now clean. 2. **JOIN query missing appointmentId filter** — Added sql template filter to the JOIN query (lines 109-110). The query now only fetches the appointments that passed the upcoming filter, rather than all scheduled appointments in the window. Note: while resolving the rebase conflict, I also restored the SMS reminder logic (GRO-598) that was present in main but missing from the original PR. The email path is unchanged. Please re-review.
github-actions[bot] commented 2026-04-16 04:43:01 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Deployed to groombook-dev

Images: pr-301
URL: https://dev.groombook.farh.net

Ready for UAT validation.

## Deployed to groombook-dev **Images:** `pr-301` **URL:** https://dev.groombook.farh.net Ready for UAT validation.
cpfarhood commented 2026-04-16 10:28:40 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@greptile please review

@greptile please review
👍 1
greptile-apps[bot] commented 2026-04-16 10:32:42 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Greptile Summary

This PR replaces the N+1 per-appointment query loop in runReminderCheck() with two batch queries: one = ANY() check against reminder_logs to identify already-sent reminders, and one JOIN query across appointments → clients → pets → services → staff to fetch all required data in a single round-trip. The structural improvement is sound and the JOIN logic is correct, but the refactor inadvertently collapses the original per-channel deduplication into a per-appointment deduplication, introducing a silent SMS delivery regression.

  • P1 – Per-channel deduplication broken: sentAppointmentIds is built without filtering by channel, so if an email reminder is logged for an appointment but the SMS send fails (smsOk = false or a caught exception), the appointment ID still appears in the set. Every subsequent cron tick will hit the continue at line 126 and skip SMS permanently for that appointment. The original code checked emailLog and smsLog independently, so SMS was always retried until it succeeded.
  • P2 – Redundant columns in first query: clientId, petId, serviceId, staffId, and status are still selected from the upcoming query but are never read from appt after the refactor; all their values now come from joinedRows.
  • P2 – Inconsistent ANY() special-casing: The sentAppointmentIds query branches on appointmentIds.length === 1 to avoid ANY(), but the joinedRows query always uses ANY(). PostgreSQL handles single-element arrays identically, making the branch unnecessary; using drizzle's inArray() for both would be more consistent and idiomatic.

Confidence Score: 3/5

Not safe to merge as-is — the channel-level deduplication regression will silently suppress SMS retries whenever email succeeds but SMS temporarily fails.

The N+1 elimination itself is correct and the JOIN query is well-structured. However, the per-channel idempotency guarantee that the original code maintained (independent emailLog / smsLog checks) is a concrete behavioral regression: any transient SMS API failure will cause the appointment to be permanently blacklisted from SMS delivery on the next cron tick. This directly affects a primary user-facing notification channel. A targeted fix is small but required before merge.

apps/api/src/services/reminders.ts — specifically the sentAppointmentIds construction (lines 70–84) and the per-appointment early-exit guard at line 126

Important Files Changed
Filename Overview
apps/api/src/services/reminders.ts N+1 queries replaced with two batch queries, but the per-channel deduplication guard was inadvertently collapsed into a per-appointment guard, causing SMS to be permanently skipped whenever email succeeds but SMS fails

Comments Outside Diff (1)

  1. apps/api/src/services/reminders.ts, line 45-63 (link)

    P2 Unused columns still selected in the first query

    After the refactor, the only fields actually consumed from appt inside the loop are id (for batching/map lookup), startTime (line 146), and confirmationToken (line 133). The columns clientId, petId, serviceId, staffId, and status are selected but never referenced from appt — all their values now come from joinedRows instead. Removing the unused fields would reduce the payload on the first round-trip query.

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

Comment:
**Unused columns still selected in the first query**

After the refactor, the only fields actually consumed from `appt` inside the loop are `id` (for batching/map lookup), `startTime` (line 146), and `confirmationToken` (line 133). The columns `clientId`, `petId`, `serviceId`, `staffId`, and `status` are selected but never referenced from `appt` — all their values now come from `joinedRows` instead. Removing the unused fields would reduce the payload on the first round-trip query.

How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All 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.

---

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.

---

This is a comment left during a code review.
Path: apps/api/src/services/reminders.ts
Line: 45-63

Comment:
**Unused columns still selected in the first query**

After the refactor, the only fields actually consumed from `appt` inside the loop are `id` (for batching/map lookup), `startTime` (line 146), and `confirmationToken` (line 133). The columns `clientId`, `petId`, `serviceId`, `staffId`, and `status` are selected but never referenced from `appt` — all their values now come from `joinedRows` instead. Removing the unused fields would reduce the payload on the first round-trip query.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(GRO-639): replace N+1 per-appointmen..." | Re-trigger Greptile

<h3>Greptile Summary</h3> This PR replaces the N+1 per-appointment query loop in `runReminderCheck()` with two batch queries: one `= ANY()` check against `reminder_logs` to identify already-sent reminders, and one JOIN query across `appointments → clients → pets → services → staff` to fetch all required data in a single round-trip. The structural improvement is sound and the JOIN logic is correct, but the refactor inadvertently collapses the original **per-channel** deduplication into a **per-appointment** deduplication, introducing a silent SMS delivery regression. - **P1 – Per-channel deduplication broken**: `sentAppointmentIds` is built without filtering by `channel`, so if an email reminder is logged for an appointment but the SMS send fails (`smsOk = false` or a caught exception), the appointment ID still appears in the set. Every subsequent cron tick will hit the `continue` at line 126 and skip SMS permanently for that appointment. The original code checked `emailLog` and `smsLog` independently, so SMS was always retried until it succeeded. - **P2 – Redundant columns in first query**: `clientId`, `petId`, `serviceId`, `staffId`, and `status` are still selected from the `upcoming` query but are never read from `appt` after the refactor; all their values now come from `joinedRows`. - **P2 – Inconsistent `ANY()` special-casing**: The `sentAppointmentIds` query branches on `appointmentIds.length === 1` to avoid `ANY()`, but the `joinedRows` query always uses `ANY()`. PostgreSQL handles single-element arrays identically, making the branch unnecessary; using drizzle's `inArray()` for both would be more consistent and idiomatic. <h3>Confidence Score: 3/5</h3> Not safe to merge as-is — the channel-level deduplication regression will silently suppress SMS retries whenever email succeeds but SMS temporarily fails. The N+1 elimination itself is correct and the JOIN query is well-structured. However, the per-channel idempotency guarantee that the original code maintained (independent emailLog / smsLog checks) is a concrete behavioral regression: any transient SMS API failure will cause the appointment to be permanently blacklisted from SMS delivery on the next cron tick. This directly affects a primary user-facing notification channel. A targeted fix is small but required before merge. apps/api/src/services/reminders.ts — specifically the sentAppointmentIds construction (lines 70–84) and the per-appointment early-exit guard at line 126 <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | apps/api/src/services/reminders.ts | N+1 queries replaced with two batch queries, but the per-channel deduplication guard was inadvertently collapsed into a per-appointment guard, causing SMS to be permanently skipped whenever email succeeds but SMS fails | </details> </details> <!-- greptile_failed_comments --> <details open><summary><h3>Comments Outside Diff (1)</h3></summary> 1. `apps/api/src/services/reminders.ts`, line 45-63 ([link](https://github.com/groombook/groombook/blob/08e15dafd5749f27b8bfe783aa134e4da9a7778c/apps/api/src/services/reminders.ts#L45-L63)) <a href="#"><img alt="P2" src="https://greptile-static-assets.s3.amazonaws.com/badges/p2.svg?v=7" align="top"></a> **Unused columns still selected in the first query** After the refactor, the only fields actually consumed from `appt` inside the loop are `id` (for batching/map lookup), `startTime` (line 146), and `confirmationToken` (line 133). The columns `clientId`, `petId`, `serviceId`, `staffId`, and `status` are selected but never referenced from `appt` — all their values now come from `joinedRows` instead. Removing the unused fields would reduce the payload on the first round-trip query. <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: 45-63 Comment: **Unused columns still selected in the first query** After the refactor, the only fields actually consumed from `appt` inside the loop are `id` (for batching/map lookup), `startTime` (line 146), and `confirmationToken` (line 133). The columns `clientId`, `petId`, `serviceId`, `staffId`, and `status` are selected but never referenced from `appt` — all their values now come from `joinedRows` instead. Removing the unused fields would reduce the payload on the first round-trip query. How can I resolve this? If you propose a fix, please make it concise. ````` </details> </details> <!-- /greptile_failed_comments --> <details><summary>Prompt To Fix All 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. --- 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. --- This is a comment left during a code review. Path: apps/api/src/services/reminders.ts Line: 45-63 Comment: **Unused columns still selected in the first query** After the refactor, the only fields actually consumed from `appt` inside the loop are `id` (for batching/map lookup), `startTime` (line 146), and `confirmationToken` (line 133). The columns `clientId`, `petId`, `serviceId`, `staffId`, and `status` are selected but never referenced from `appt` — all their values now come from `joinedRows` instead. Removing the unused fields would reduce the payload on the first round-trip query. How can I resolve this? If you propose a fix, please make it concise. ````` </details> <sub>Reviews (1): Last reviewed commit: ["fix(GRO-639): replace N+1 per-appointmen..."](https://github.com/groombook/groombook/commit/08e15dafd5749f27b8bfe783aa134e4da9a7778c) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=28614528)</sub>
greptile-apps[bot] (Migrated from github.com) reviewed 2026-04-16 10:32:51 +00:00
apps/api/src/services/reminders.ts
@@ -62,0 +116,4 @@
)
);
const appointmentMap = new Map<string, typeof joinedRows[number]>();
greptile-apps[bot] (Migrated from github.com) commented 2026-04-16 10:32:46 +00:00

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>
apps/api/src/services/reminders.ts
@@ -62,0 +119,4 @@
const appointmentMap = new Map<string, typeof joinedRows[number]>();
for (const row of joinedRows) {
appointmentMap.set(row.appointmentId, row);
}
greptile-apps[bot] (Migrated from github.com) commented 2026-04-16 10:32:45 +00:00

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>
the-dogfather-cto[bot] commented 2026-04-16 15:54:59 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Closing — superseded by PR #306 (v2 of this fix).

Closing — superseded by PR #306 (v2 of this fix).
This repo is archived. You cannot comment on pull requests.
Reviewers
No Reviewers
lint-roller-qa[bot]
greptile-apps[bot]
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 feature
New feature
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 question
Further information is requested
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
ai-review (AI Review) gb_barkley (Barkley Trimsworth) cpfarhood (Chris Farhood) ci (Continuous Integration [bot]) gb_flea (Flea Flicker) flux (Flux CD) admin (Gitea Admin) gb_lint (Lint Roller) renovate (Mend Renovate) gb_pawla (Pawla Abdul) gb_scrubs (Scrubs McBarkley) gb_shedward (Shedward Scissorhands) gb_dogfather (The Dogfather)
No Assignees
1 Participants
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: groombook/app#301
Delete Branch "clean-gro-639"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?