fix: email notifications silently skipped when other channels exist#300
Merged
fix: email notifications silently skipped when other channels exist#300
Conversation
When a monitor had notification_channels rows (e.g. Slack) but no email row, the backwards-compatibility fallback (emailEnabled boolean) was bypassed, causing email to be silently dropped with zero delivery log entries. Root cause: monitor creation never seeded a default email channel row. Once any channel was added, the dispatcher only iterated explicit rows. Changes: - Seed a default email channel row on monitor creation (routes.ts) - Log a failed delivery entry when email row is missing but emailEnabled is true, so the gap shows in the Delivery Log (notification.ts) - Add console.warn for missing-email-row drops (observability) - Add backfill script for existing monitors (scripts/backfill-email-channels.ts) - Add 4 regression tests covering the missing-row scenario https://claude.ai/code/session_01DobxmLZCR4Za9bKRMsPDqk
The initial fix only seeded the email channel row in the main route. The API v1 and Chrome extension routes also create monitors and must seed the same default row to prevent silent email loss when a second channel is later added. https://claude.ai/code/session_01DobxmLZCR4Za9bKRMsPDqk
…ustness - Extract seedDefaultEmailChannel() shared helper to eliminate duplication across 3 monitor creation routes - Add console.warn when email channel is deleted while emailEnabled=true - Improve digest observability log to include batch change count - Backfill script: batch inserts (100/tx) to avoid lock contention, add fallback for Drizzle result shape changes - Add notification mock to extension test to prevent import chain failure https://claude.ai/code/session_01DobxmLZCR4Za9bKRMsPDqk
📝 WalkthroughWalkthroughThis PR introduces email channel initialization and observability for monitors. It adds a backfill script and new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Routes as Route Handler
participant Storage
participant Notification as Notification Service
participant DeliveryLog as Delivery Log
rect rgba(100, 150, 200, 0.5)
Note over Client,DeliveryLog: Monitor Creation with Email Channel Seeding
Client->>Routes: POST /monitors
Routes->>Storage: createMonitor()
Storage-->>Routes: monitor
Routes->>Notification: seedDefaultEmailChannel(monitor.id)
Notification->>Storage: upsertMonitorChannel(email)
Storage-->>Notification: success/fail
Routes-->>Client: 201 Created
end
rect rgba(200, 150, 100, 0.5)
Note over Client,DeliveryLog: Notification Delivery with Missing Channel Detection
Client->>Notification: processChangeNotification(monitor, changes)
Notification->>Storage: getMonitorChannels(monitor.id)
Storage-->>Notification: channels (email row missing)
alt Email channel missing but emailEnabled=true
Notification->>Notification: Log warning
Notification->>DeliveryLog: addDeliveryLog(status=failed, error=email row missing)
else Email channel exists
Notification->>Notification: Send email
end
Notification-->>Client: Complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/backfill-email-channels.ts`:
- Around line 33-41: The current backfill selects monitorsNeedingEmail and
inserts an email channel with enabled=true regardless of the monitor's legacy
preference; change the logic so only monitors with monitors.emailEnabled = true
are backfilled or, alternatively, when creating rows for notification_channels
set the channel row's enabled value to the monitor's monitors.emailEnabled
value. Update the SELECT used to build monitorsNeedingEmail to JOIN the monitors
table and filter WHERE monitors.emailEnabled = true, or modify the INSERT path
that consumes monitorsNeedingEmail to query the monitor's emailEnabled and use
that boolean for the notification_channels.enabled column so existing opt-outs
are preserved.
In `@server/services/notification.ts`:
- Around line 214-228: The observability log block for missing email rows is
never reached because hasActiveChannels() currently treats monitors with
emailEnabled=true but no "email" channel (and only disabled non-email channels)
as inactive; update the notification flow so that the missing-email check is
evaluated before or as part of the active-channel gate: either move the existing
missing-row check (the hasEmailRow check and storage.addDeliveryLog call) to run
before hasActiveChannels() is consulted in processChangeNotification(),
processQueuedNotifications(), and processDigestCron(), or modify
hasActiveChannels() to return true when monitor.emailEnabled && !hasEmailRow so
the email-missing path is treated as active for observability. Ensure references
to hasEmailRow, monitor.emailEnabled, hasActiveChannels(), and the three
functions are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 743e2868-156b-43fe-8175-1a2c89b0f549
📒 Files selected for processing (7)
scripts/backfill-email-channels.tsserver/routes.tsserver/routes/extension.test.tsserver/routes/extension.tsserver/routes/v1.tsserver/services/notification.test.tsserver/services/notification.ts
Comment on lines
+33
to
+41
| // Find monitors that have at least one channel row but no email row. | ||
| const monitorsNeedingEmail = await db.execute(sql` | ||
| SELECT DISTINCT nc.monitor_id | ||
| FROM notification_channels nc | ||
| WHERE NOT EXISTS ( | ||
| SELECT 1 FROM notification_channels nc2 | ||
| WHERE nc2.monitor_id = nc.monitor_id AND nc2.channel = 'email' | ||
| ) | ||
| `); |
There was a problem hiding this comment.
Don't re-enable email for monitors that are opted out.
This migration ignores monitors.emailEnabled and inserts every missing "email" row as enabled=true. Once an explicit email channel exists, delivery no longer falls back to the legacy boolean, so monitors that currently have emailEnabled=false but use Slack/webhook will start sending email after this backfill.
Limit the backfill to monitors with email enabled, or insert the email row with enabled mirroring the existing monitor preference so current behavior is preserved.
Also applies to: 68-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/backfill-email-channels.ts` around lines 33 - 41, The current
backfill selects monitorsNeedingEmail and inserts an email channel with
enabled=true regardless of the monitor's legacy preference; change the logic so
only monitors with monitors.emailEnabled = true are backfilled or,
alternatively, when creating rows for notification_channels set the channel
row's enabled value to the monitor's monitors.emailEnabled value. Update the
SELECT used to build monitorsNeedingEmail to JOIN the monitors table and filter
WHERE monitors.emailEnabled = true, or modify the INSERT path that consumes
monitorsNeedingEmail to query the monitor's emailEnabled and use that boolean
for the notification_channels.enabled column so existing opt-outs are preserved.
Comment on lines
+214
to
+228
| // Observability: if email is expected (emailEnabled=true) but no email channel row exists, | ||
| // log a delivery entry so the gap is visible rather than silently skipped. | ||
| const hasEmailRow = channels.some((c) => c.channel === "email"); | ||
| if (!hasEmailRow && monitor.emailEnabled) { | ||
| console.warn(`[notification] Email channel row missing for monitor ${monitor.id} — email skipped. Add an email channel row to enable delivery.`); | ||
| try { | ||
| await storage.addDeliveryLog({ | ||
| monitorId: monitor.id, | ||
| changeId: change.id, | ||
| channel: "email", | ||
| status: "failed", | ||
| response: { error: "No email channel configured — email channel row missing from notification_channels" }, | ||
| }); | ||
| } catch { /* delivery log table may not exist yet */ } | ||
| } |
There was a problem hiding this comment.
This still silently skips email when the only existing rows are disabled non-email channels.
These new warning/logging blocks are unreachable if a legacy monitor has emailEnabled=true, no "email" row, and only disabled Slack/webhook rows. processChangeNotification(), processQueuedNotifications(), and processDigestCron() all return early via hasActiveChannels(), which currently treats that state as fully inactive, so the silent-drop bug still survives there.
Treat emailEnabled && !hasEmailRow as an active path for observability, or move the missing-row check ahead of the hasActiveChannels() gate.
🔧 Minimal fix
async function hasActiveChannels(monitor: Monitor): Promise<boolean> {
let channels: NotificationChannel[];
try {
channels = await storage.getMonitorChannels(monitor.id);
} catch {
channels = [];
}
if (channels.length === 0) {
return monitor.emailEnabled;
}
- return channels.some((c) => c.enabled);
+ const hasEnabledChannel = channels.some((c) => c.enabled);
+ const missingLegacyEmailRow =
+ monitor.emailEnabled && !channels.some((c) => c.channel === "email");
+ return hasEnabledChannel || missingLegacyEmailRow;
}Also applies to: 352-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/notification.ts` around lines 214 - 228, The observability
log block for missing email rows is never reached because hasActiveChannels()
currently treats monitors with emailEnabled=true but no "email" channel (and
only disabled non-email channels) as inactive; update the notification flow so
that the missing-email check is evaluated before or as part of the
active-channel gate: either move the existing missing-row check (the hasEmailRow
check and storage.addDeliveryLog call) to run before hasActiveChannels() is
consulted in processChangeNotification(), processQueuedNotifications(), and
processDigestCron(), or modify hasActiveChannels() to return true when
monitor.emailEnabled && !hasEmailRow so the email-missing path is treated as
active for observability. Ensure references to hasEmailRow,
monitor.emailEnabled, hasActiveChannels(), and the three functions are updated
accordingly.
…x hasActiveChannels gate 1. Backfill script now JOINs monitors table and uses emailEnabled value for the channel row's enabled column, preserving user opt-outs. 2. hasActiveChannels() now returns true when emailEnabled=true but no email channel row exists, so the observability log is reachable even when all other channel rows are disabled. https://claude.ai/code/session_01DobxmLZCR4Za9bKRMsPDqk
github-actions
bot
deleted the
claude/fix-monitor-email-notifications-URZlf
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Email notifications were silently dropped for monitors that had a Slack or webhook channel configured but no explicit email channel row in
notification_channels. The backwards-compatibility fallback (monitors.emailEnabledboolean) only fires when there are zero channel rows — once any channel row exists, the dispatcher iterates only explicit rows. Since monitor creation never seeded a default email channel row, adding Slack/webhook later caused email to disappear with no delivery log entry.This PR fixes all three monitor creation paths, adds observability logging for the gap state, provides a backfill script for existing data, and includes regression tests.
Changes
Core fix — notification service (
server/services/notification.ts)emailEnabled=true, writes afaileddelivery log entry andconsole.warninstead of silently droppingseedDefaultEmailChannel()shared helper used by all creation routesMonitor creation — all three routes
server/routes.ts— seeds default email channel row aftercreateMonitor()server/routes/v1.ts— same seeding for API v1 routeserver/routes/extension.ts— same seeding for Chrome extension routeChannel deletion observability (
server/routes.ts)console.warnwhen email channel is deleted whileemailEnabled=trueBackfill script (
scripts/backfill-email-channels.ts)ON CONFLICT DO NOTHING) to insert email rows for monitors that have other channels but no email rowTests (
server/services/notification.test.ts)faileddelivery log entry with descriptive reasonemailEnabled=false— asserts no false-positive log entrysuccessdeliveryfaileddelivery log entry for missing email rowHow to test
npm run check && npm run test— all 1900 tests passnpm run buildsucceedsnotification_channelshas an email rownpx tsx scripts/backfill-email-channels.ts— run once, safe to re-runhttps://claude.ai/code/session_01DobxmLZCR4Za9bKRMsPDqk
Summary by CodeRabbit
New Features
Chores