Skip to content

feature/email-reminders - #1256

Merged
arpandhakal merged 44 commits into
mainfrom
feature/email-reminders
Jun 1, 2026
Merged

feature/email-reminders #1256
arpandhakal merged 44 commits into
mainfrom
feature/email-reminders

Conversation

@arpandhakal

Copy link
Copy Markdown
Collaborator

Changes

  • All changes regarding the Tasks Improved Email Notifications - Reminder Emails goes here.

arpandhakal and others added 9 commits May 18, 2026 16:34
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
@vercel

vercel Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
tasks-app Ready Ready Preview, Comment May 28, 2026 2:30pm

Request Review

@greptile-apps

greptile-apps Bot commented May 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces a task email reminder system: a new TaskReminderSents table to deduplicate sent reminders, a raw-SQL eligibility query that identifies tasks needing reminders today, and a getReminderEmailDetails helper that generates email copy for all six reminder types.

  • Schema & migration (taskReminderSent.prisma, migration.sql): adds the TaskReminderSents table with a (taskId, recipientId, reminderType) unique constraint as the idempotency primitive, and a CASCADE DELETE FK on taskId.
  • Eligibility query (eligibility.ts): raw SQL that joins Tasks against itself to suppress same-assignee subtask duplicates and date-windows eligible tasks; the regex guard for malformed VARCHAR(10) dueDate values is structurally unsafe — it sits in a separate AND condition from the ::date cast, leaving the cast unprotected against planner reordering.
  • Email copy (notification.helpers.ts): getReminderEmailDetails covers all six TaskReminderType variants with workspace-branded subjects and workspace-label-driven headers; the NO_DUE_DATE bodies still use "assigned to you" for company recipients, contradicting the correct company-specific header.

Confidence Score: 3/5

The 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

Filename Overview
src/jobs/notifications/eligibility.ts Introduces getEligibleReminders with a raw SQL query; the regex guard for malformed VARCHAR dueDate is in a separate AND condition from the cast, so PostgreSQL's planner may attempt the cast before the guard fires.
src/app/api/notification/notification.helpers.ts Adds getReminderEmailDetails; NO_DUE_DATE body text uses "assigned to you" for both individual and company recipients, inconsistent with the company-specific header.
prisma/migrations/20260515091539_add_task_reminder_sents_table/migration.sql Creates TaskReminderSents table with a unique constraint on (taskId, recipientId, reminderType) and a CASCADE DELETE FK on taskId; schema looks correct.
prisma/schema/taskReminderSent.prisma New TaskReminderSent model with correct unique composite key and CASCADE delete from Task; looks correct.
prisma/schema/task.prisma Adds the taskReminderSents relation to the Task model; minimal, correct change.
src/jobs/notifications/eligibility.test.ts Good coverage of happy path, empty results, call count, and error propagation.
src/app/api/notification/notification.helpers.test.ts Solid snapshot + behavioral tests covering all reminder types, custom labels, missing brandName, and ctaParams.
src/app/api/notification/snapshots/notification.helpers.test.ts.snap Auto-generated snapshot file; reflects the current (inconsistent) body text for company recipients.

Sequence Diagram

sequenceDiagram
    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
Loading

Reviews (1): Last reviewed commit: "Merge pull request #1236 from assemblyco..." | Re-trigger Greptile

Comment thread src/jobs/notifications/eligibility.ts Outdated
Comment on lines +77 to +83
-- 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)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Suggested change
-- 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)
)

Comment on lines +240 to +248
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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>
@vercel

vercel Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Deployment failed with the following error:

Deploying Serverless Functions to multiple regions is restricted to the Pro and Enterprise plans.

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>
arpandhakal and others added 7 commits May 25, 2026 16:05
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
arpandhakal and others added 2 commits May 25, 2026 18:23
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>
arpandhakal and others added 8 commits May 26, 2026 16:16
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>
OUT-3730 | Trigger.dev job: send-task-reminders scheduled task
arpandhakal and others added 2 commits May 27, 2026 16:38
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
arpandhakal and others added 10 commits May 27, 2026 18:25
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
@arpandhakal
arpandhakal merged commit d246440 into main Jun 1, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants