feature/email-reminders - #1256
Conversation
Adds a minimal ledger to enforce reminder idempotency at the DB level. Unique constraint on (taskId, recipientId, reminderType) is the dedupe primitive so retries and manual re-triggers cannot double-send. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Returns subject/header/title/body/ctaParams for each of the six TaskReminderType variants. Header branches on whether the recipient is a company (uses workspace groupTerm) or an individual. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds getEligibleReminders() that returns one row per (task, assignee, reminderType) eligible for a task reminder today, across the six exact-day windows defined in the Reminder Emails PRD. The EligibilityRow carries the companyId derived per assigneeType so the future sender can stamp ClientNotifications.companyId and Copilot's recipientCompanyId without a follow-up task lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without this, a same-assignee subtask under a soft-deleted (or archived / completed) parent is silently dropped: the parent itself is filtered out by the main WHERE, but the LEFT JOIN still returns its assigneeId, which fails IS DISTINCT FROM and drops the subtask. Filtering parent lifecycle in the JOIN makes parent.assigneeId come back NULL for dead parents, so subtasks correctly emit their own reminder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-add-taskremindersent-table-taskremindertype-enum OUT-3734 | Schema: add TaskReminderSent table + TaskReminderType enum
OUT-3735 | Reminder copy helper: getReminderEmailDetails
OUT-3736 | Eligibility SQL: single-day reminder query
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR introduces a task email reminder system: a new
Confidence Score: 3/5The eligibility query's dueDate cast guard is structurally incorrect — a single malformed row in the Tasks table could cause the entire daily cron run to throw a PostgreSQL cast error, silently skipping all reminders for that day. The developer explicitly documented the risk of malformed VARCHAR dueDate values poisoning the query, but the chosen mitigation (a separate AND condition) does not guarantee evaluation order in PostgreSQL. If any task row contains a dueDate that passes the regex format check but fails the ::date cast (e.g. '2023-13-99'), or if the planner evaluates the cast condition before the regex, the entire getEligibleReminders call fails and no reminders go out that day. This is the critical path of the new feature and would be completely silent in normal monitoring. src/jobs/notifications/eligibility.ts — the WHERE clause combining the regex guard and the ::date cast needs the most attention before merge. Important Files Changed
Sequence DiagramsequenceDiagram
participant Cron as Cron Job
participant Eligibility as getEligibleReminders
participant DB as PostgreSQL (Tasks / TaskReminderSents)
participant Helper as getReminderEmailDetails
participant Email as Email Service
Cron->>Eligibility: getEligibleReminders(db)
Eligibility->>DB: $queryRaw — SELECT eligible (task, assignee, reminderType) rows
DB-->>Eligibility: EligibilityRow[]
Eligibility-->>Cron: EligibilityRow[]
loop For each EligibilityRow
Cron->>Helper: getReminderEmailDetails(workspace, task, isCompanyRecipient)
Helper-->>Cron: email content keyed by TaskReminderType
Cron->>Email: Send reminder email (subject, header, body, ctaParams)
Cron->>DB: INSERT INTO TaskReminderSents (taskId, recipientId, reminderType) ON CONFLICT DO NOTHING
end
Reviews (1): Last reviewed commit: "Merge pull request #1236 from assemblyco..." | Re-trigger Greptile |
| -- Guard against malformed VARCHAR(10) dueDate values: only cast when the string | ||
| -- looks like ISO YYYY-MM-DD. Without this, a single bad row poisons the whole query. | ||
| AND (t."dueDate" IS NULL OR t."dueDate" ~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}$') | ||
| AND ( | ||
| (t."dueDate" IS NULL AND t."assignedAt"::date IN (CURRENT_DATE - 3, CURRENT_DATE - 7)) | ||
| OR t."dueDate"::date IN (CURRENT_DATE - 7, CURRENT_DATE - 3, CURRENT_DATE, CURRENT_DATE + 3) | ||
| ) |
There was a problem hiding this comment.
The regex guard and the
::date cast are in two separate AND conditions. PostgreSQL's query planner is free to reorder AND predicates, so it can attempt t."dueDate"::date on a malformed value before the regex check has filtered it out, throwing a cast error that poisons the entire query for that day. The developer's comment confirms this risk is known; the fix is to use a CASE WHEN expression, which PostgreSQL guarantees evaluates sequentially and short-circuits.
| -- Guard against malformed VARCHAR(10) dueDate values: only cast when the string | |
| -- looks like ISO YYYY-MM-DD. Without this, a single bad row poisons the whole query. | |
| AND (t."dueDate" IS NULL OR t."dueDate" ~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}$') | |
| AND ( | |
| (t."dueDate" IS NULL AND t."assignedAt"::date IN (CURRENT_DATE - 3, CURRENT_DATE - 7)) | |
| OR t."dueDate"::date IN (CURRENT_DATE - 7, CURRENT_DATE - 3, CURRENT_DATE, CURRENT_DATE + 3) | |
| ) | |
| -- Guard against malformed VARCHAR(10) dueDate values: use CASE WHEN to guarantee | |
| -- the regex is evaluated before the ::date cast (AND predicate order is not guaranteed). | |
| AND ( | |
| (t."dueDate" IS NULL AND t."assignedAt"::date IN (CURRENT_DATE - 3, CURRENT_DATE - 7)) | |
| OR (CASE WHEN t."dueDate" ~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}$' | |
| THEN t."dueDate"::date IN (CURRENT_DATE - 7, CURRENT_DATE - 3, CURRENT_DATE, CURRENT_DATE + 3) | |
| ELSE FALSE END) | |
| ) |
| body: `This is a friendly reminder that you have a task ‘${task.title}’ assigned to you that's still pending completion.\n\nIf you've already completed this task, please mark it as done in the portal.`, | ||
| ctaParams, | ||
| }, | ||
| [TaskReminderType.NO_DUE_DATE_7D]: { | ||
| subject: `${portalPrefix} [Reminder] Task still pending`, | ||
| header, | ||
| title, | ||
| body: `This is a friendly reminder that you have a task ‘${task.title}’ that was assigned to you a week ago and is still pending.\n\nIf you've already completed this task, please mark it as done in the portal.`, | ||
| ctaParams, |
There was a problem hiding this comment.
NO_DUE_DATE bodies say "assigned to you" for company recipients
The NO_DUE_DATE_3D and NO_DUE_DATE_7D bodies hard-code the phrase "assigned to you" regardless of isCompanyRecipient. When a company is the assignee and isCompanyRecipient=true, the header correctly reads "A task was assigned to your company" but the body still says "assigned to you", sending an inconsistent message to recipients of company tasks. Adding a derived phrase — e.g. const assignedTo = isCompanyRecipient ? 'your ' + labels.groupTerm : 'you' — and using it in both NO_DUE_DATE bodies would align them with the already-correct header logic.
…rder Postgres does not guarantee AND-predicate evaluation order, so the separate regex guard could be reordered after the ::date cast and the cron would crash on any malformed dueDate. CASE WHEN evaluates sequentially and short-circuits, which is the documented-safe pattern for this. Behavior is unchanged for valid rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Deployment failed with the following error: Learn More: https://vercel.link/multiple-function-regions |
Plumbs the reminder email payload through copilot.createNotification with deliveryTargets.email only. Deliberately does not write to ClientNotification (that table tracks in-product read-state, which reminders don't create) and does not write to TaskReminderSent (the caller owns the ledger insert as the dedupe primitive on success). Throws on Copilot failure so callers can skip the ledger and let the next cron run retry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Daily 00:00 UTC cron that walks getEligibleReminders, fans out company- assigned rows to current members via getCompanyClients, and dispatches email-only notifications via sendReminderEmail. Idempotency lives in the ledger insert: a single batched INSERT ... ON CONFLICT (taskId, recipientId, reminderType) DO NOTHING RETURNING ... runs *before* any Copilot call, so retried cron runs and in-flight duplicates can never double-send. Only rows that come back from RETURNING are net-new and proceed to the send phase. On Copilot failure we DELETE the ledger row so the next cron run retries; a failing DELETE is logged distinctly so on-call can clean up the stuck row. Per-workspace CopilotAPI is minted from any task.createdById + workspaceId via encodePayload — same shape as cmd/backfill-missed-emails. Workspace bottleneck = 5 matches WORKSPACE_CONCURRENCY in auto-archive. allSettled keeps a failing workspace from aborting the sweep. IU rows are filtered in the cron rather than in the SQL to keep OUT-3736's contract untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d apiKey Drops the IU-token mint and uses the workspace-scoped apiKey pattern that the SDK patch already supports when COPILOT_ENV is set on the Trigger.dev runtime — same env that auto-archive's dispatch-task-archived-webhook relies on. Two wins: - No "pick a random task's createdById to forge a token" fallback, which was structurally awkward (the IU we mint as had no semantic meaning). - One fewer crypto call per workspace per cron run. senderId for the email itself still comes from task.createdById in sendReminderEmail — that's unchanged, since the IU who created the task is the legitimate sender identity for the reminder. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prisma 5.14+ exposes createManyAndReturn, which compiles to exactly the INSERT ... ON CONFLICT DO NOTHING RETURNING shape the cron needs but does it as a typed Prisma call. Drops: - The Prisma.sql / Prisma.join template assembly. - Manual ::uuid and ::"TaskReminderType" casts (Prisma handles via the model's @db.Uuid / enum typing). - The hand-written gen_random_uuid() in VALUES — the model already sets id via @default(dbgenerated("gen_random_uuid()")), so Postgres fills it in automatically when Prisma omits it from the INSERT. - The LedgerInsertedRow shim type (now inferred from the Prisma model). Net 15 lines shorter, no behavior change. skipDuplicates: true compiles to ON CONFLICT DO NOTHING against the existing (taskId, recipientId, reminderType) unique constraint, and createManyAndReturn only returns the rows that actually got inserted — identical semantics to the previous raw query. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Strip restating-the-code and ticket-reference comments. Keep three short load-bearing notes: the workspace-scoped apiKey shape, the ledger-before-send ordering, and why we DELETE on Copilot failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds t.title and t.createdById to the eligibility SELECT and drops the per-workspace task.findMany. processWorkspace now operates on a single consistent snapshot from the eligibility query — no more two-step read that could pick up divergent state between the query and the send. Same behavior, fewer DB calls, tighter consistency window. The remaining race (task reassigned between eligibility query and Copilot send) is the unavoidable one and was never closable without distributed transactions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the type-field annotation, the function docstring, and shorten the three inline SQL comments to one line each. Keeps the genuinely load-bearing notes (subtask carve-out, IS DISTINCT FROM rationale, the CASE WHEN evaluation-order guarantee). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OUT-3737 | Send flow: email-only delivery via NotificationService
Copilot's email service prepends `<workspace.brandName> portal:` to every
notification subject server-side. Our reminder copy helper was also
prepending it, producing doubled subjects like:
"Assembly + Outside portal: Assembly + Outside portal: [Overdue] ..."
The existing `getEmailDetails` (for non-reminder emails) emits bare
subjects for this reason — reminders should match that convention.
Side effect: closes the open PRD-verbatim question on DUE_DATE_OVERDUE_7D.
The PRD's inconsistent inclusion of `{Company} portal:` was a description
of the rendered subject, not what the code should emit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors auto-archive's dispatchTaskArchivedWebhook pattern. The cron used
to call copilot.createNotification sequentially within each workspace; a
company task with 50 members forced 50 serial round-trips inside the
scheduled task's wall-clock budget. Now the cron:
1. Resolves recipients (still includes copilot.getCompanyClients fan-out).
2. Inserts the ledger with ON CONFLICT DO NOTHING.
3. batchTriggers one dispatch-reminder-email per net-new ledger row.
Each dispatch-reminder-email is its own Trigger.dev task with:
* queue.concurrencyLimit = 5 (global parallelism across all workspaces).
* retry.maxAttempts = 3 with exponential backoff (transient 5xx no longer
costs a day of reminders).
* onFailure hook that DELETEs the ledger row after retries exhaust, so
the next cron run retries. Compensating in onFailure (not inline catch)
avoids dropping the ledger on transient failures a retry would recover.
Cron's per-workspace totals shift from {sent, failed, skipped} to
{enqueued, skipped} — per-send success/failure is now tracked in the
dispatcher's Trigger.dev logs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lure Trigger.dev caps batchTrigger at 500 items per call. A workspace with a single company task fanning out to 1700+ members blew past that and threw BatchTriggerError, leaving the ledger rows orphaned — the unique constraint then blocked any future cron from re-sending those reminders. Two fixes: 1. Chunk triggers into 500-item batches so any workspace fits. 2. On per-chunk batchTrigger failure, deleteMany the chunk's ledger rows so the next cron run can retry. Same compensation contract as the per-row dispatcher's onFailure hook, just scoped to the chunk. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's three rename suggestions, plus the cascaded references: * allRows → eligibleTasks * rows (filtered) → tasks * byWorkspace → tasksByWorkspace * workspaceRows param → workspaceTasks * processWorkspace's `rows` param → `tasks` * LedgerPlanEntry.row field → .task (so entry.row.X reads as entry.task.X) * Loop variable in resolveRecipients renamed for symmetry Variable referring to inserted ledger rows (`for (const row of inserted)`) intentionally kept as `row` — that's a SQL row, not a task. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Before: a single getCompanyClients throw (after Copilot's own withRetry exhausts) would propagate out of the plan loop, leak through processWorkspace, and the outer try/catch would mark the entire workspace as failed — dropping every other eligible task in that workspace for the day, including client-assigned tasks that don't even need fan-out. After: per-task try/catch around resolveRecipients. The failing task is logged and skipped; siblings continue. No added retry — Copilot's internal retry is the only retry layer; this is just blast-radius containment. Resolves greptile P1 on PR #1258. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Extract serializeError to src/utils/serializeError.ts; drop the duplicated local copy in send-task-reminders.ts and dispatch-reminder-email.ts. * Simplify resolveRecipients — drop the dead `return []` branch since IUs are filtered upstream; the function now reads as "client by default, fan out only for company". * In dispatchReminderEmailOnFailure, replace the `p` alias with a typed destructure of the payload. The SDK's AnyOnFailureHookFunction types the payload as `unknown`, so we still cast once at destructure time, but downstream code reads the meaningful field names directly. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The chunked batchTrigger loop had a nested try/catch and manual index
arithmetic inline. Extract the dispatch-or-compensate logic into a small
closure so the outer loop reads as just "chunk and accumulate":
for (let i = 0; i < triggers.length; i += BATCH_TRIGGER_CHUNK_SIZE) {
enqueued += await dispatchChunk(triggers.slice(i, i + BATCH_TRIGGER_CHUNK_SIZE))
}
Per-chunk compensation semantics are unchanged. `chunkOffset` dropped from
the failure log — workspaceId + chunkSize + log ordering are enough for
post-mortem, and the index didn't add diagnostic value worth the noise.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…feature/email-reminders
OUT-3730 | Trigger.dev job: send-task-reminders scheduled task
…feature/email-reminders
Wire Sentry into the Trigger.dev runtime (it runs in a separate process from the Next.js server, so sentry.server.config.ts never loads there) via a one-time init in src/jobs/sentry.ts, reusing the installed @sentry/nextjs. - send-task-reminders: capture eligibility-query failures (rethrow so the run still fails) and emit one structured run-summary log. - dispatch-reminder-email: capture terminal send failures in onFailure with taskId / recipientId / reminderType / workspaceId tags. ON CONFLICT skips are never captured. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OUT-3738 | Observability: structured logging + Sentry for reminder cron
Add a testcontainers-backed integration harness (jest.integration.config.ts + test/integration/*) that boots an ephemeral Postgres, applies the real migration history, and is hard-guarded to never truncate a non-local DB. Kept separate from the default `jest` unit run. - eligibility.integration.test.ts: exercises the real SQL — all six windows hit on their exact day, boundary misses, deleted/archived/completed exclusions, company single-row, and subtask carve-outs. - reminder-idempotency.integration.test.ts: one send → one ledger row; re-run → zero new (unique constraint); forced Copilot failure → ledger cleared + Sentry event. Fix surfaced by the real DB: the global softDelete Prisma extension rewrites .delete()/.deleteMany() into deletedAt updates for every model, but TaskReminderSents has no deletedAt — so ledger compensation silently failed and the unique constraint would block all future re-sends. Both compensation paths now hard-delete via $executeRaw; affected unit tests updated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
testcontainers pulls in undici@7.x, which requires node >=20.18.1; the workflow's hardcoded 20.18.0 failed `yarn install` with an engine incompatibility. .nvmrc already pins 20.19.1, so point setup-node at it to match local dev and stop the drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When an IU marks a shared task (isShared + associations) as done, email the client users it's shared with (viewers): a single client, or every client in a shared company. Mirrors the existing email-only "shared with you" pattern (disableInProduct), so viewers get an email and no in-product badge. - Add CompletedToSharedCU / CompletedToSharedCompany notification actions - getNotificationParties: sender is the completing IU; recipients resolved from associations - Email + (unused) in-product copy in notification.helpers - Case 5b in sendTaskUpdateNotifications routes shared-task completion to the new dispatchers Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors Case 5's assigneeId guard so the IU-only invariant for shared tasks is explicit. Without it, an edge-case shared task with no assignee would reach getNotificationParties and throw a ZodError on senderId parsing that the create() try/catch silently swallows. Per Greptile review feedback on PR #1265. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the intermediate function reference; branch the ternary directly to the awaited call. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-notifications OUT-3038 | Send CU task completed email notifications if they are viewers of the task
OUT-3731 | Tests: eligibility SQL unit tests + idempotency integration test
Changes