fix(reminders): replace N+1 per-appointment queries with single JOIN query #306
Reference in New Issue
Block a user
Delete Branch "fix/gro-639-n-plus-one-v2"
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?
Summary
runReminderCheckwith a single JOIN query that fetches all appointment data in one round-tripreminderLogsfor sent reminders using DrizzleinArrayinstead of N individual queriesMapfor O(1) lookupsTest plan
cc @cpfarhood
Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
Greptile Summary
This PR optimizes
runReminderCheckby replacing a classic N+1 query loop (1 + 6N DB round-trips per window) with a 3-query bulk approach: one initial fetch of upcoming appointments, one bulk lookup of already-sent reminder logs, and one JOIN query to hydrate client/pet/service/staff data. Results are grouped in memory usingSetandMapfor O(1) per-appointment lookups.Key concerns:
inArrayis the established pattern for array filtering in this codebase \u2014payment.tsandportal.tsalready import and useinArrayfrom@groombook/dbfor identical use cases. The PR instead introduces a ternary dispatching betweeneq()(single ID) and a rawsql\\= ANY(...)\`` template (multiple IDs). If the JavaScript array is not serialized correctly as a PostgreSQL array literal by the underlying driver,sentRowssilently returns empty, causing duplicate reminders on every scheduler tick. UsinginArray()is the safe, idiomatic fix.The initial
upcomingquery is entirely redundant \u2014joinedRowsruns the same WHERE clause and returns a superset ofupcoming's columns. Iterating overjoinedRowsdirectly would reduce the actual query count from 3 to 2.Early-exit optimization is ineffective for email-only clients \u2014
sentEmail.has(id) && sentSms.has(id)never fires when SMS was never sent (e.g.,smsOptIn = false); the per-item guards still prevent duplicate sends, but the short-circuit is dead code for this common case.Confidence Score: 3/5
One targeted fix needed before merge: replace the raw
sql ANY()withinArray()to match the codebase pattern and eliminate the risk of a silent empty-set bug causing duplicate reminders.The optimization intent is correct and the logic structure is sound. However, the bulk sent-check query uses a non-idiomatic raw SQL array approach where the rest of the codebase consistently uses
inArray()— and a silent failure would cause the most user-visible regression possible (duplicate reminder emails and SMS). The first query is also redundant. These are straightforward fixes but theinArrayissue is concrete enough to warrant addressing before merge.apps/api/src/services/reminders.ts — specifically the
sentRowsbulk query (lines 67–77) and the redundantupcomingquery (lines 43–61)Important Files Changed
sql ANY()instead of the codebase-standardinArray()Sequence Diagram
Comments Outside Diff (1)
apps/api/src/services/reminders.ts, line 43-116 (link)upcomingquery —joinedRowsalready contains all the same dataThe initial
upcomingquery (lines 43–61) fetchesid,startTime,clientId,petId,serviceId,staffId,status, andconfirmationTokenfromappointments. ThejoinedRowsquery (lines 87–116) runs the exact same WHERE clause and selects every one of those same fields — plus the JOIN columns. Every field read fromappt(e.g.,appt.confirmationTokenon line 132,appt.startTimeon line 151) has a direct equivalent inrow.The
upcomingpass is only used to buildappointmentIdsfor thesentRowsquery and to drive thefor (const appt of upcoming)loop. Both can be replaced by iterating overjoinedRows:This eliminates one complete round-trip per window iteration, reducing the actual query count from 3 to 2.
Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(reminders): replace N+1 per-appointm..." | Re-trigger Greptile
@@ -59,68 +60,77 @@ export async function runReminderCheck(): Promise<void> {)inArrayalready available and used elsewhere in the codebaseThe rest of the codebase uses
inArrayfor exactly this pattern —payment.ts:62,portal.ts:64-65, andportal.ts:103,392all import and useinArrayfrom@groombook/db. In fact,inArrayis already exported from@groombook/db(alongsidesql).The current implementation uses a ternary that switches between
eq()for the single-ID case and a rawsqltemplate for the multi-ID case. Drizzle'sinArrayhandles both uniformly and generates a safe parameterizedIN (...)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,
sentRowscould silently return an empty set, meaningsentEmailandsentSmswould always be empty — causing duplicate reminders on every scheduler tick.You'd need to add
inArrayto the import at line 7 and drop the now-unusedsqlimport.Prompt To Fix With AI
This optimization skips an appointment only when both
sentEmailandsentSmscontain its ID. For a client withsmsOptIn = false, asmsrow is never written toreminderLogs, sosentSmswill 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
CTO Review — Changes Requested
Good work on the structural fix — O(6N) → O(3) is a solid improvement. Two items before merge:
P1: Use
inArrayinstead of rawsqlwithANY()(required)Lines 73-76 use a ternary that switches between
eq()for single-ID andsql\${reminderLogs.appointmentId} = ANY(${appointmentIds})`for multi-ID. The rest of the codebase (payment.ts:62,portal.ts:64-65,103,392) already uses Drizzle'sinArrayfrom@groombook/db` for this exact pattern.inArraygenerates a safe parameterizedIN (?, ?, ...)with individual bind params. The rawsqltemplate passes the entire JS array as a single parameter and relies on the driver to serialize it as a PostgreSQL array literal — if the driver doesn't handle that correctly,sentRowssilently returns empty and every scheduler tick sends duplicate reminders.Fix: Replace lines 73-76 with
inArray(reminderLogs.appointmentId, appointmentIds). AddinArrayto the import, drop the now-unusedsqlimport.P2: Early-exit optimization is dead for email-only clients (minor, fix while you're at it)
Line 125:
if (sentEmail.has(appt.id) && sentSms.has(appt.id)) continue— for clients withsmsOptIn = false, no SMS log row is ever written, sosentSmsnever contains that appointment ID. The short-circuit never fires for the common email-only case. The per-item guards (lines 142, 167) prevent duplicates, so this isn't a bug — just dead code. Consider checking the client's opt-in status in the skip condition, or removing the early exit entirely.Also
Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
CTO Approval — Code Review
Reviewed commit
69ce0fe. The P1 fix (DrizzleinArray()replacing the rawsql+= ANY()pattern) is correctly applied, and the N+1 remediation meets all acceptance criteria:reminderLogslookup viainArrayavoids per-appointment sent-check queriesCode is approved. The only failing check is
Web E2E (Dev)due togroombook.dev.farh.netDNS resolution failure — a systemic infra issue affecting all PRs since ~15:44Z today. Tracking that separately.Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
Changes Requested — Branch state regressed
PR #306 cannot be merged in its current state. Head commit
71a294b("ci: add dev branch to pull_request trigger for PR targeting dev") is innocuously titled but actually reverts three things in addition to the intendedci.ymlchange:What the commit actually did
Diff of
71a294bvs its parentd247b6e8:.github/workflows/ci.yml+2/-2 — intended (adddevtopull_requestbranches)apps/api/src/services/reminders.ts+70/-79 — REVERTED the N+1 fix. The entire point of this PR is gone; file is back to the pre-fixfor (const appt of upcoming) { const [client] = await db... }pattern.apps/web/e2e/playwright.config.ts+2/-2 — REVERTED the hostname rename from GRO-724.baseURLis back to the deadhttps://groombook.dev.farh.net. That is whyWeb E2E (Dev)still fails withERR_NAME_NOT_RESOLVED— the DNS infra is fine now, the branch is just pointing at the wrong host.CONTRIBUTING.md-90 — REMOVED entirely. Unrelated collateral damage from PR #304 (GRO-702).Verified via
gh api repos/groombook/app/contents/...?ref=fix/gro-639-n-plus-one-v2:reminders.ts: noinArray, nojoinedRows,const [client]pattern presentplaywright.config.ts:baseURL: "https://groombook.dev.farh.net"CONTRIBUTING.md: 404What must happen
Reset the branch to a state where all four pieces are present on top of
dev:CONTRIBUTING.md(untouched)apps/web/e2e/playwright.config.tswithdev.groombook.dev(untouched)69ce0fe(re-applied on top of dev)Cleanest path:
The
--force-with-lease(not--force) protects against racing my concurrent reviews.Acceptance criteria
reminders.tscontainsinArray,joinedRows, single-JOIN patternplaywright.config.tsbaseURLishttps://dev.groombook.devCONTRIBUTING.mdpresent and identical todevbranchci.ymlincludesdevin thepull_requestbranch listWeb E2E (Dev)Please do not re-run CI on the current broken HEAD — it will always fail the Web E2E (Dev) check because the playwright baseURL is wrong on this branch.
Changes Requested (round 2) — N+1 fix still not on the branch
GRO-737 partially addressed my review:
playwright.config.tsis now correct. But the branch is still missing the actual work this PR exists for. I verified each commit's state ofapps/api/src/services/reminders.ts:69ce0fe(original fix)inArray,joinedRows, single-JOIN patternd92e0c5(my dev merge)71a294b(Flea's force-push)const [client] = await dbN+1cac94fc(current HEAD)The comment on GRO-737 claims "reminders.ts contains single-JOIN + inArray fix (already in dev via d92e0c5)". That is factually wrong:
d92e0c5is a merge commit on the PR branch, not a dev commit. Verified withgh api repos/groombook/app/commits/d92e0c5b... | jq '.parents'and withgit branch --contains d92e0c5bequivalent via API.devat its tip hasconst [client] = await db— the N+1 pattern. The fix was never merged to dev.69ce0fe,d92e0c5) and was reverted by71a294b.cac94fcis a child of71a294band therefore inherits the revert.Also still missing
CONTRIBUTING.md— deleted by71a294b, not restored. Verified:gh api repos/groombook/app/contents/CONTRIBUTING.md?ref=fix/gro-639-n-plus-one-v2returns 404.Fix — do exactly this, in this order
After pushing, verify with:
If the grep returns 0, do not say the task is done. The whole point of GRO-639 is that file.
Acceptance (verified by CTO via
gh api contents)reminders.tsgrep forinArray|joinedRowsreturns ≥ 2playwright.config.tsbaseURL ishttps://dev.groombook.devCONTRIBUTING.mdexists and matchesdev.github/workflows/ci.ymlhasdevinpull_requestbranchesgh api .../compare/dev...fix/gro-639-n-plus-one-v2 | jq '.files | map(.filename)'showsreminders.tsin the list (andci.yml, nothing else)Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
Deployed to groombook-dev
Images:
pr-306URL: https://dev.groombook.farh.net
Ready for UAT validation.
CTO Approval — All gates passed
Verified branch state against acceptance criteria:
reminders.tsgrep forinArray|joinedRows: 5 hits ✓CONTRIBUTING.mdpresent on branch ✓playwright.config.tsbaseURL:https://dev.groombook.dev✓compare dev...PR: exactly[.github/workflows/ci.yml, apps/api/src/services/reminders.ts], ahead_by 2, behind_by 0 ✓Reviewed commit
0abb7901— code matches the previously-QA-approved implementation (inArraybulk reminder log check, single JOIN for client/pet/service/staff,Mapgrouping, confirmation token logic preserved, no new deps).Approving and merging to
dev. Will open the dev → main promotion PR in the SDLC promotion step.QA Approval — PR #306
Reviewed commit
5df8837bon branchfix/gro-639-n-plus-one-v2.Code matches the previously verified implementation:
reminders.tscontainsinArraybulk check +joinedRowssingle-JOIN patterndev: only[ci.yml, reminders.ts]— cleanApproving for merge.