perf(notifications): deduplicate user queries across notification agents#2877
Open
dougrathbone wants to merge 2 commits intoseerr-team:developfrom
Open
Conversation
Each notification agent independently called userRepository.find() to load all users, resulting in up to 6 concurrent full-table scans per notification event. Move the query into NotificationManager.sendNotification() so it runs once and passes the result to agents via payload.adminUsers. Also replace requestRepository.find() with count() in the webpush agent, which was loading all pending MediaRequest entities with eager relations just to read .length for a badge count.
|
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 (1)
📝 WalkthroughWalkthroughNotificationPayload gained an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant NotificationManager
participant Database as UserRepo
participant Agents
Caller->>NotificationManager: sendNotification(type, payload)
NotificationManager->>NotificationManager: void dispatchNotification(type, payload)
alt notifyAdmin or MEDIA_APPROVED/DECLINED
NotificationManager->>UserRepo: find() (async)
UserRepo-->>NotificationManager: users[] / error
NotificationManager->>NotificationManager: payload.adminUsers = users[] or []
end
NotificationManager->>Agents: invoke each active agent with (type, payload)
Agents-->>NotificationManager: send results/errors (handled per-agent)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/lib/notifications/index.ts (1)
101-123:⚠️ Potential issue | 🟠 MajorHandle admin-user preload failures inside the manager.
sendNotification()now awaitsuserRepository.find()before any agent runs, but the existing fire-and-forget call sites inserver/entity/MediaRequest.tsand the subscribers still do not await this method. A transient failure here now rejects the promise, skips every downstream notification path, and can surface as an unhandled rejection even forMEDIA_APPROVED/MEDIA_DECLINED, where the lookup is only needed for the WebPush badge update. Please catch/log this preload locally or keep a synchronous wrapper around an internal async dispatch so user/system notifications still fan out when the admin lookup fails.Suggested change
- public async sendNotification( + public sendNotification( type: Notification, payload: NotificationPayload - ): Promise<void> { + ): void { + void this.dispatchNotification(type, payload); + } + + private async dispatchNotification( + type: Notification, + payload: NotificationPayload + ): Promise<void> { logger.info(`Sending notification(s) for ${Notification[type]}`, { label: 'Notifications', subject: payload.subject, }); if ( payload.notifyAdmin || type === Notification.MEDIA_APPROVED || type === Notification.MEDIA_DECLINED ) { - const userRepository = getRepository(User); - payload.adminUsers = await userRepository.find(); + try { + payload.adminUsers = await getRepository(User).find(); + } catch (e) { + logger.error('Failed to preload admin users for notification', { + label: 'Notifications', + type: Notification[type], + subject: payload.subject, + errorMessage: e instanceof Error ? e.message : String(e), + }); + payload.adminUsers = []; + } } this.activeAgents.forEach((agent) => { if (agent.shouldSend()) { - agent.send(type, payload); + void agent.send(type, payload); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/lib/notifications/index.ts` around lines 101 - 123, sendNotification currently awaits userRepository.find() and will reject the whole notification flow on transient DB failures; wrap the admin preload in a try/catch so failures are handled locally: when type is MEDIA_APPROVED or MEDIA_DECLINED or payload.notifyAdmin, attempt to load admin users via userRepository.find() inside a try block, on success assign payload.adminUsers, on error log the error (use logger with label 'Notifications' and context) and set payload.adminUsers to an empty array (or leave unchanged) so execution continues; then proceed to iterate this.activeAgents.forEach(...) calling agent.shouldSend() and agent.send(...) so other notification paths still run even if the admin lookup failed. Include references to sendNotification, userRepository.find(), Notification.MEDIA_APPROVED / MEDIA_DECLINED, this.activeAgents, agent.shouldSend and agent.send in the change.
🤖 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/webpush.ts`:
- Around line 171-173: The code eagerly executes
getRepository(MediaRequest).count() into pendingRequestsCount for every Web Push
notification; make this count lazy so it only runs for notification types that
need it (the MEDIA_PENDING flow and the approval/decline badge-update payload).
Refactor so that pendingRequestsCount is obtained on-demand (e.g., call
getRepository(MediaRequest).count() or a small helper only inside the
MEDIA_PENDING branch and the approval/decline badge-update branches), leaving
other notification paths untouched; update references at the existing
pendingRequestsCount usage sites (the MEDIA_PENDING handling and the
approval/decline badge payload construction) to call the lazy helper or local
count instead of the eagerly-evaluated variable.
---
Outside diff comments:
In `@server/lib/notifications/index.ts`:
- Around line 101-123: sendNotification currently awaits userRepository.find()
and will reject the whole notification flow on transient DB failures; wrap the
admin preload in a try/catch so failures are handled locally: when type is
MEDIA_APPROVED or MEDIA_DECLINED or payload.notifyAdmin, attempt to load admin
users via userRepository.find() inside a try block, on success assign
payload.adminUsers, on error log the error (use logger with label
'Notifications' and context) and set payload.adminUsers to an empty array (or
leave unchanged) so execution continues; then proceed to iterate
this.activeAgents.forEach(...) calling agent.shouldSend() and agent.send(...) so
other notification paths still run even if the admin lookup failed. Include
references to sendNotification, userRepository.find(),
Notification.MEDIA_APPROVED / MEDIA_DECLINED, this.activeAgents,
agent.shouldSend and agent.send in the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb67f42c-e31e-43be-8ff1-bb1181cfbeab
📒 Files selected for processing (8)
server/lib/notifications/agents/agent.tsserver/lib/notifications/agents/discord.tsserver/lib/notifications/agents/email.tsserver/lib/notifications/agents/pushbullet.tsserver/lib/notifications/agents/pushover.tsserver/lib/notifications/agents/telegram.tsserver/lib/notifications/agents/webpush.tsserver/lib/notifications/index.ts
Wrap the admin user preload in try/catch so a transient DB failure does not kill the entire notification flow. Keep sendNotification() synchronous and delegate to a private async dispatchNotification() to avoid unhandled promise rejections from fire-and-forget callers. Only run the pending request count query for notification types that actually use it (MEDIA_PENDING, MEDIA_APPROVED, MEDIA_DECLINED) instead of on every webpush notification.
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
1 participant
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.
Description
While investigating the subscriber notification chain in #2745 and #2743, I found a separate performance optimisation along the way: the notification dispatch layer itself was triggering redundant database queries across all active agents.
When a notification fires,
NotificationManager.sendNotification()dispatches to all active agents in parallel. Six of those agents (Discord, Email, Telegram, Webpush, Pushover, Pushbullet) each independently calluserRepository.find()with no filter, loading every user row plus the eager-loadedUserSettingsrelation. A single notification event triggers up to 6 concurrent full-table scans of the user table.This PR moves the user query into the dispatch layer so it runs once, passing the result to agents via
payload.adminUsers. Each agent replaces its own query with the pre-loaded list and continues to apply its agent-specific filter as before.Additionally, the Webpush agent was loading all pending
MediaRequestentities (with eager-loaded Media, Seasons, Users, and SeasonRequests) just to read.lengthfor a badge count. Replaced with a lightweightcount()query.Changes:
adminUsers?: User[]toNotificationPayloadsendNotification()whennotifyAdminis true or type isMEDIA_APPROVED/MEDIA_DECLINED(the Webpush agent needs users for badge-update pushes on those types even whennotifyAdminis false)userRepository.find()withpayload.adminUsersin all 6 agentsrequestRepository.find()withrequestRepository.count()in the Webpush agentgetRepositoryandUserimports from 5 agentsAI Disclosure: I used Claude Code to help validate my approach.
How Has This Been Tested?
pnpm buildpasses with no type errorspnpm test- 33/33)Checklist:
pnpm buildpnpm i18n:extract(no new UI strings added)Summary by CodeRabbit