Conversation
volunteer status change
|
@greptile |
Greptile SummaryThis PR adds a complete in-app and email notification system to Neuron — covering 7 event types, a registry-driven architecture, a job-based delivery pipeline, a notification inbox UI, and per-user preference management. The overall design is solid and well-structured, but there are two bugs in the job-delivery layer that need fixing before merging. Key issues found:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/server/jobs/definitions/process-notification.job.ts | Two P1 bugs: excludeUserIds is absent from the payload type and not forwarded to processNotification, causing volunteer-exclusion logic in coverage notifications to be silently dropped at job-execution time. |
| src/server/services/notificationService.ts | Idempotency guard in processNotification is broken — it queries the base key but individual rows are inserted with per-user keyed variants, so the guard never matches on retry and duplicate emails are re-queued. Also contains a leftover debug console.log. |
| src/server/jobs/definitions/check-shift-notifications.job.ts | Correctly handles cancellation guard, coverage-swap roster, and attendance check. Minor: toLocaleDateString/toLocaleTimeString are locale-sensitive; consider pinning locale and timezone. |
| src/server/services/notificationEventService.ts | Well-structured event façade; correctly fans out to different audience/context combinations per event type. excludeUserIds is set correctly here — the bug is in the downstream job handler. |
| src/server/services/preferenceService.ts | Clean preference resolution; defaults are merged with overrides correctly, role filtering is applied, and bulk recipient lookup is efficient. |
| src/server/notifications/registry.ts | Registry-driven architecture is well-designed. Each notification type declares titles, bodies, email renderer, channel defaults, and role applicability in one place. |
| src/server/db/schema/notification.ts | Schema looks correct — appropriate indexes, per-user cascade deletes, and a unique constraint on (user_id, type, channel) for preferences. |
| src/server/api/routers/notification-router.ts | All CRUD, bulk-action, and preference endpoints are correctly scoped to the authenticated user's ID; no cross-user access issues. |
| src/components/notifications/notification-inbox.tsx | Inbox component is well-structured with time-bucket grouping, filter modes, and correct cache invalidation after mutations. |
Last reviewed commit: "fix: consolidate mig..."
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| audience: Audience; | ||
| context: Record<string, unknown>; | ||
| actorId?: string; | ||
| idempotencyKey?: string; | ||
| }; | ||
|
|
There was a problem hiding this comment.
excludeUserIds silently dropped
ProcessNotificationPayload does not include an excludeUserIds field, and the handler does not pass it to processNotification. Even though notify() serialises the field into the job payload (via payload as any), it is simply ignored here.
This causes concrete misbehaviour today:
notifyCoverageRequestedpassesexcludeUserIds: [requestingVolunteerUserId]so the requesting volunteer won't see the admin-level "coverage requested" notification. That exclusion is lost, so the requesting volunteer receives the notification anyway.notifyCoverageFilledpassesexcludeUserIds: [coveredByVolunteerUserId, requestingVolunteerUserId]for the same reason. Both are excluded from the admin copy, but on job execution neither exclusion is applied.
Fix: add excludeUserIds to the payload type and forward it in the handler:
| audience: Audience; | |
| context: Record<string, unknown>; | |
| actorId?: string; | |
| idempotencyKey?: string; | |
| }; | |
| export type ProcessNotificationPayload = { | |
| type: string; | |
| audience: Audience | Audience[]; | |
| context: Record<string, unknown>; | |
| actorId?: string; | |
| idempotencyKey?: string; | |
| excludeUserIds?: string[]; | |
| }; |
| if (idempotencyKey) { | ||
| const [existing] = await this.db | ||
| .select({ id: notification.id }) | ||
| .from(notification) | ||
| .where(eq(notification.idempotencyKey, idempotencyKey)) | ||
| .limit(1); | ||
|
|
||
| if (existing) return; | ||
| } |
There was a problem hiding this comment.
The guard queries for a row whose idempotency_key equals the base key (e.g. shift-cancelled-abc), but the actual insertions on line 352 store per-user keys of the form ${idempotencyKey}:${r.userId}. The base key is never written to the database, so this check always returns no row and the guard is a no-op.
On a job retry the onConflictDoNothing protects individual notification rows, but the email-dispatch loop runs again in full, resulting in duplicate emails sent to every email recipient.
The simplest fix is to query for any row whose key starts with the base:
const [existing] = await this.db
.select({ id: notification.id })
.from(notification)
.where(sql`${notification.idempotencyKey} like ${idempotencyKey + ':%'}`)
.limit(1);Or alternatively, record the base idempotency key on the first write by inserting a sentinel row, or store emailSent state persistently alongside each row so the email loop is safe to re-run.
| private async resolveAudience( | ||
| audience: Audience, | ||
| ): Promise<ResolvedRecipient[]> { | ||
| console.log("processing notif"); | ||
| switch (audience.kind) { | ||
| case "user": { |
There was a problem hiding this comment.
| private async resolveAudience( | |
| audience: Audience, | |
| ): Promise<ResolvedRecipient[]> { | |
| console.log("processing notif"); | |
| switch (audience.kind) { | |
| case "user": { | |
| private async resolveAudience( | |
| audience: Audience, | |
| ): Promise<ResolvedRecipient[]> { | |
| switch (audience.kind) { |
| }), | ||
| volunteerUserIds: effectiveVolunteers.map((v) => v.userId), | ||
| }); | ||
| } else { | ||
| // no-checkin: find volunteers who have no attendance record | ||
| const attendanceRecords = await db | ||
| .select({ userId: shiftAttendance.userId }) |
There was a problem hiding this comment.
toLocaleDateString() and toLocaleTimeString() depend on the Node.js process locale and timezone. On a server whose locale is not en-CA, dates might render as 3/21/2026 instead of March 21, 2026, or times might omit AM/PM. This string is stored in the notification body and sent in emails, where users expect a consistent human-readable format.
Consider using a locale-stable formatter, e.g.:
shiftDate: shiftRow.startAt.toLocaleDateString("en-CA", {
year: "numeric", month: "long", day: "numeric", timeZone: "America/Vancouver"
}),
shiftTime: shiftRow.startAt.toLocaleTimeString("en-CA", {
hour: "numeric", minute: "2-digit", timeZone: "America/Vancouver"
}),| handler: async (payload, { cradle }) => { | ||
| await cradle.notificationService.processNotification({ | ||
| type: payload.type, | ||
| audience: payload.audience, | ||
| context: payload.context, | ||
| actorId: payload.actorId, | ||
| idempotencyKey: payload.idempotencyKey, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
excludeUserIds must also be forwarded to processNotification
The handler reads five fields from the payload but omits excludeUserIds. Once it is added to ProcessNotificationPayload (see the companion comment on the type above), it must also be passed through to processNotification here, otherwise the exclusion set in notifyCoverageRequested and notifyCoverageFilled will still be silently ignored at runtime.
1 participant
Summary
Adds a full notification system covering the entire lifecycle — from event triggers through delivery (in-app + email) to user-facing inbox and preference management.
What it does
7 notification types are supported, each with in-app and email delivery:
Notification inbox
A popover dropdown in the header with:
Per-user notification preferences
Users can toggle individual notification types for emails (always send in-app). Defaults are defined in the notification registry and vary by role. The existing notification settings page has been overhauled to use these preferences.
How it works
Registry-driven architecture — each notification type is declared once in a central registry with its title/body templates, email renderer, default channels, and applicable roles. Adding a new notification type is a single registry entry + an email template.
Event service layer — domain services (shift, coverage, user) call
NotificationEventServicemethods likenotifyShiftCancelled(...)which resolve the audience (by role, shift assignment, class membership, etc.), check per-user preferences, and fan out to the notification + email jobs.Job-based delivery — notifications are processed through the existing job queue:
process-notification— creates the in-app DB record and conditionally enqueues an email jobsend-email— renders and sends the emailcheck-shift-notifications— scheduled at shift creation time for reminders (1hr before) and missed check-in detection (at shift end)Preference resolution — the
PreferenceServicemerges registry-defined defaults with user overrides, filtering to only notification types relevant to the user's role.Database changes
Two new tables via Drizzle migrations:
notification— stores in-app notifications with read/archived state, idempotency keys, and source referencesnotification_preference— per-user channel overrides with a unique constraint on (user, type, channel)Testing
Integration tests for the preference service covering default resolution, override behavior, role filtering, and the clear-preference flow.