feat(notifications): add notification type filtering to email notifications#2692
feat(notifications): add notification type filtering to email notifications#2692manuelgr0 wants to merge 5 commits intoseerr-team:developfrom
Conversation
…ering to email notifications Adds system-level notification type filtering to email notifications, matching the functionality already available in all other notification agents (Telegram, Discord, Slack, Webhook, Gotify, Ntfy, Pushbullet, Pushover).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEmail notifications now respect system-level notification type settings. The backend email agent checks configured types before sending when Changes
Sequence DiagramsequenceDiagram
actor Admin
participant UI as "Email Settings UI"
participant Form as "Formik Form"
participant API as "Settings API"
participant EmailAgent as "Email Agent"
participant Store as "Config Store"
Admin->>UI: Open email settings
UI->>Form: Render values (including types)
Admin->>Form: Select types & enable email
Form->>Form: Validate (types required if enabled)
Admin->>UI: Save
Form->>API: POST `/api/v1/settings/notifications/email` with types
API->>Store: Persist settings
Note over EmailAgent,Store: Later, on notification trigger
EmailAgent->>Store: getSettings()
EmailAgent->>EmailAgent: If payload.notifySystem == true
EmailAgent->>EmailAgent: hasNotificationType(settings.types, type)?
alt type enabled
EmailAgent->>EmailAgent: Proceed to send email
else type disabled
EmailAgent->>EmailAgent: Return early (skip sending)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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 `@server/lib/notifications/agents/email.ts`:
- Around line 209-213: The condition in email notification gating uses
hasNotificationType with its arguments reversed, breaking the (types, value)
contract and disabling the "types === 0" allow-all path; update the call in the
block that checks payload.notifySystem to call
hasNotificationType(settings.types ?? 0, type) instead of
hasNotificationType(type, settings.types ?? 0) so existing installs with
settings.types unset/0 will allow system notifications as intended; keep the
rest of the condition (payload.notifySystem and return true) unchanged.
In `@src/components/Settings/Notifications/NotificationsEmail.tsx`:
- Around line 131-135: The form treats a types value of 0 (legacy "send all") as
falsy and marks existing configs invalid; update the NotificationsEmail form
initialValues to preserve the legacy state by using data.types when present or 0
otherwise (set types to data.types ?? 0) and change the validation/checks that
currently use "!values.types" (in the submit/validation logic around the
handlers referenced) to only treat undefined/null as missing (e.g., use
values.types == null or values.types === undefined) so that a numeric 0 bitmask
is accepted; reference NotificationTypeSelector and its hasNotificationType
helper when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cf3ac61-dd6f-4b0b-87f2-ce1b5faf6668
📒 Files selected for processing (2)
server/lib/notifications/agents/email.tssrc/components/Settings/Notifications/NotificationsEmail.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds system-level notification type filtering support to the Email notification agent and exposes notification type selection in the Email notification settings UI, aligning Email with other notification agents.
Changes:
- Add
NotificationTypeSelectorto the Email notification settings and persisttypeson save/test. - Add server-side email agent filtering so system notifications only send when the notification type is enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/components/Settings/Notifications/NotificationsEmail.tsx | Adds notification type selection UI for Email and includes types in save/test requests. |
| server/lib/notifications/agents/email.ts | Adds system-level notification type filtering to the Email agent. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip sending if this is a system notification and the type is not enabled | ||
| if ( | ||
| payload.notifySystem && | ||
| !hasNotificationType(settings.types ?? 0, type) |
There was a problem hiding this comment.
hasNotificationType is being called with arguments reversed (hasNotificationType(settings.types ?? 0, type)). This breaks the helper’s intended behavior of always allowing TEST_NOTIFICATION: once settings.types is non-zero (i.e., after the admin selects types in the UI), test emails will be filtered out because the function will end up checking (Notification.TEST_NOTIFICATION & settings.types).
Suggested fix: call it consistently with other agents (hasNotificationType(type, settings.types ?? 0)) and, to preserve backward compatibility for existing installs where email.types is unset, only apply filtering when settings.types is explicitly defined (e.g., skip the filter when settings.types == null).
| // Skip sending if this is a system notification and the type is not enabled | |
| if ( | |
| payload.notifySystem && | |
| !hasNotificationType(settings.types ?? 0, type) | |
| // Skip sending if this is a system notification and the type is not enabled. | |
| // If settings.types is unset, preserve legacy behavior and do not filter. | |
| if ( | |
| payload.notifySystem && | |
| settings.types != null && | |
| !hasNotificationType(type, settings.types) |
Description
This PR adds system-level notification type filtering to email notifications, bringing email into alignment with all other notification agents (Telegram, Discord, Slack, Webhook, Gotify, Ntfy, Pushbullet, Pushover).
AI was used to help generate these changes. The changes were reviewed, modified and tested by me after it.
How Has This Been Tested?
Locally run and tested.
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
New Features
Improvements