Reduce notification polling#436
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR removes SSE realtime notification support and related hooks/routes, scopes frontend notification queries to the open UI, converts backend delivery to batched processing that treats IN_APP deliveries as immediate successes, and updates types, constants, and tests accordingly. ChangesNotification System Architecture Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/notifications/batchingService.ts (2)
91-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hide scheduling failures behind a
voidfire-and-forget call.
findMany()runs after this method returns, and.catch(console.error)drops the failure on the floor. If the user lookup fails, callers still see success even though no maintenance notices were queued. Make this methodasync, await the query, and let the error propagate so the caller can handle or surface it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/notifications/batchingService.ts` around lines 91 - 119, The scheduleMaintenanceNotification method currently fires prisma.user.findMany() without awaiting and swallows errors with .catch(console.error); change the signature to async scheduleMaintenanceNotification(...): Promise<void>, await the prisma.user.findMany(...) call, iterate users and call this.scheduleNotification as before, and remove the .catch so any thrown error propagates to the caller (or rethrow the caught error) so failures in prisma.user.findMany or scheduling are not hidden.
95-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch maintenance recipients to the channels you actually send.
This lookup only filters
inAppEnabled, but every queued notification is sent asDeliveryChannel.BOTH. That will email users who only opted into in-app delivery, and it will miss users who enabled email but disabled in-app. Build the recipient list from both channel preferences and derive the channel per user before scheduling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/notifications/batchingService.ts` around lines 95 - 113, The current query filters only notificationPreferences.some.inAppEnabled and then always schedules with DeliveryChannel.BOTH, which mismatches user prefs; update the query that builds the recipients (the block that selects notificationPreferences for NotificationType.MAINTENANCE_NOTICE) to fetch both inApp/email preference fields, compute per-user deliveryChannel (EMAIL, IN_APP, or BOTH) from their notificationPreferences, and call this.scheduleNotification with the derived deliveryChannel instead of always DeliveryChannel.BOTH; ensure you reference NotificationType.MAINTENANCE_NOTICE and the scheduleNotification call when making the change.src/server/notifications/service.ts (1)
167-181:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix email delivery for
immediate: truewithDeliveryChannel.BOTH(or enforce it’s unsupported)
deliverNotification()only sends email whendata.deliveryChannel === DeliveryChannel.EMAIL, while the batching path treatsDeliveryChannel.BOTHas including email. There are currently no repo call sites using{ immediate: true }withDeliveryChannel.BOTH, but the logic would silently drop email if that combination becomes possible.Suggested alignment
if ( this.config.enableEmailDelivery && this.emailService && - data.deliveryChannel === DeliveryChannel.EMAIL + (data.deliveryChannel === DeliveryChannel.EMAIL || + data.deliveryChannel === DeliveryChannel.BOTH) ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/notifications/service.ts` around lines 167 - 181, deliverNotification() currently only sends email when data.deliveryChannel === DeliveryChannel.EMAIL which drops email for immediate deliveries when deliveryChannel === DeliveryChannel.BOTH; update the logic in the deliverNotification method of the notifications service to treat DeliveryChannel.BOTH the same as EMAIL for immediate:true (i.e., include BOTH in the condition that triggers sending email), or alternatively add an explicit guard that throws/returns an unsupported error when immediate:true is passed with DeliveryChannel.BOTH so the behavior is explicit; reference DeliveryChannel, deliverNotification (or the method in the notifications service where the shown diff lives) when making the change.
🧹 Nitpick comments (1)
src/server/notifications/service.ts (1)
99-101: 💤 Low valueDuplicate log statement.
notificationBatchingService.scheduleNotificationalready logs the same "Notification scheduled for batch processing" message (seebatchingService.ts:66). Remove to avoid log noise.Suggested fix
const batchId = notificationBatchingService.scheduleNotification(data, scheduledFor) - - console.log(`Notification scheduled for batch processing: ${batchId}`) return batchId🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/notifications/service.ts` around lines 99 - 101, Remove the duplicate console.log in service.ts: after calling notificationBatchingService.scheduleNotification(data, scheduledFor) drop the line that logs "Notification scheduled for batch processing: ${batchId}" because scheduleNotification already emits the same message; keep the call and the returned batchId if needed, but avoid emitting the redundant log to reduce noise (refer to notificationBatchingService.scheduleNotification for the existing logging).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/notifications/batchingService.ts`:
- Around line 91-119: The scheduleMaintenanceNotification method currently fires
prisma.user.findMany() without awaiting and swallows errors with
.catch(console.error); change the signature to async
scheduleMaintenanceNotification(...): Promise<void>, await the
prisma.user.findMany(...) call, iterate users and call this.scheduleNotification
as before, and remove the .catch so any thrown error propagates to the caller
(or rethrow the caught error) so failures in prisma.user.findMany or scheduling
are not hidden.
- Around line 95-113: The current query filters only
notificationPreferences.some.inAppEnabled and then always schedules with
DeliveryChannel.BOTH, which mismatches user prefs; update the query that builds
the recipients (the block that selects notificationPreferences for
NotificationType.MAINTENANCE_NOTICE) to fetch both inApp/email preference
fields, compute per-user deliveryChannel (EMAIL, IN_APP, or BOTH) from their
notificationPreferences, and call this.scheduleNotification with the derived
deliveryChannel instead of always DeliveryChannel.BOTH; ensure you reference
NotificationType.MAINTENANCE_NOTICE and the scheduleNotification call when
making the change.
In `@src/server/notifications/service.ts`:
- Around line 167-181: deliverNotification() currently only sends email when
data.deliveryChannel === DeliveryChannel.EMAIL which drops email for immediate
deliveries when deliveryChannel === DeliveryChannel.BOTH; update the logic in
the deliverNotification method of the notifications service to treat
DeliveryChannel.BOTH the same as EMAIL for immediate:true (i.e., include BOTH in
the condition that triggers sending email), or alternatively add an explicit
guard that throws/returns an unsupported error when immediate:true is passed
with DeliveryChannel.BOTH so the behavior is explicit; reference
DeliveryChannel, deliverNotification (or the method in the notifications service
where the shown diff lives) when making the change.
---
Nitpick comments:
In `@src/server/notifications/service.ts`:
- Around line 99-101: Remove the duplicate console.log in service.ts: after
calling notificationBatchingService.scheduleNotification(data, scheduledFor)
drop the line that logs "Notification scheduled for batch processing:
${batchId}" because scheduleNotification already emits the same message; keep
the call and the returned batchId if needed, but avoid emitting the redundant
log to reduce noise (refer to notificationBatchingService.scheduleNotification
for the existing logging).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 527f31d3-6922-4626-be90-042bae7ab0b6
📒 Files selected for processing (9)
src/app/api/notifications/stream/route.tssrc/components/notifications/NotificationCenter.tsxsrc/data/constants.tssrc/hooks/useRealtimeNotifications.tssrc/server/notifications/batchingService.tssrc/server/notifications/realtimeService.tssrc/server/notifications/service.test.tssrc/server/notifications/service.tssrc/server/notifications/types.ts
💤 Files with no reviewable changes (6)
- src/app/api/notifications/stream/route.ts
- src/hooks/useRealtimeNotifications.ts
- src/data/constants.ts
- src/server/notifications/service.test.ts
- src/server/notifications/types.ts
- src/server/notifications/realtimeService.ts
Producdevity
commented
Jun 5, 2026
| } | ||
|
|
||
| // Schedule weekly digest notifications | ||
| scheduleWeeklyDigest(userId: string): void { |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/notifications/batchingService.ts (1)
144-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse enum values instead of string literals for
deliveryChannelcomparison.Lines 137-140 correctly use
DeliveryChannel.IN_APPandDeliveryChannel.BOTH, but this block uses string literals'EMAIL'and'BOTH'. This is inconsistent and violates the coding guidelines.Proposed fix
if ( - (data.deliveryChannel === 'EMAIL' || data.deliveryChannel === 'BOTH') && + (data.deliveryChannel === DeliveryChannel.EMAIL || data.deliveryChannel === DeliveryChannel.BOTH) && this.emailService ) {As per coding guidelines: "Import enum values from
@orm; do not use string literals for enum values in application code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/notifications/batchingService.ts` around lines 144 - 149, Replace the string-literal checks for deliveryChannel with the DeliveryChannel enum: update the condition in the block that calls this.deliverEmail(data) to compare data.deliveryChannel against DeliveryChannel.EMAIL and DeliveryChannel.BOTH (imported from `@orm` if not already), ensuring consistency with other checks that use DeliveryChannel.IN_APP and DeliveryChannel.BOTH; keep the call to this.deliverEmail(data) and push to deliveryPromises unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/server/notifications/batchingService.ts`:
- Around line 144-149: Replace the string-literal checks for deliveryChannel
with the DeliveryChannel enum: update the condition in the block that calls
this.deliverEmail(data) to compare data.deliveryChannel against
DeliveryChannel.EMAIL and DeliveryChannel.BOTH (imported from `@orm` if not
already), ensuring consistency with other checks that use DeliveryChannel.IN_APP
and DeliveryChannel.BOTH; keep the call to this.deliverEmail(data) and push to
deliveryPromises unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5248335-7c59-40dc-b1af-4269ecd4df4f
📒 Files selected for processing (3)
src/server/notifications/batchingService.tssrc/server/notifications/service.test.tssrc/server/notifications/service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/notifications/service.ts
15 tasks
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
Reduces notification server load by removing the unused realtime/SSE notification path and making the navbar notification center less chatty.
The bell now keeps only the unread-count query active for signed-in users, with a 10-minute refresh interval while closed and no background list polling. The notification list query only runs while the dropdown or mobile sheet is open, then refreshes once per minute while visible. Opening the menu also refreshes the unread count so the badge stays reasonably current when a user interacts with it.
Notification writes no longer do extra unread-count lookups for realtime delivery that had no active client integration. Unused weekly and maintenance batching wrappers were removed as dead code, and immediate
BOTHdelivery now sends email when email delivery is enabled.No linked issue.
Type of change
How Has This Been Tested?
Ran:
./node_modules/.bin/eslint src/data/constants.ts src/components/notifications/NotificationCenter.tsx src/server/notifications/service.ts src/server/notifications/batchingService.ts src/server/notifications/types.ts src/server/notifications/service.test.ts src/types/static-images.d.ts ./node_modules/.bin/vitest run src/server/notifications/service.test.ts ./node_modules/.bin/tsc --noEmit --pretty false git diff --checkScreenshots (if applicable)
N/A
Checklist
Notes for reviewers
The removed realtime server path used an in-memory connection map and was not connected to the notification UI. Keeping polling, but reducing it to a single cheap badge refresh plus on-demand list loading, keeps the behavior predictable without adding always-open client connections.