Fix #155 Added notification system#168
Conversation
📝 WalkthroughWalkthroughImplements a comprehensive notification system spanning backend persistence and frontend UI integration. Adds notification models, service layer, and CRUD endpoints. Integrates notification creation into comment workflows, badge awards, team joins, team debates, and rating updates. Frontend includes notification polling, read-status management, deletion, and user profile popover. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend<br/>(Header.tsx)
participant API as Backend API<br/>(Controller)
participant Svc as Services<br/>(Notification)
participant DB as MongoDB<br/>(notifications)
Note over Frontend,DB: Notification Creation Flow<br/>(e.g., Comment Posted)
User->>API: POST /comments
API->>Svc: services.CreateNotification(...)
Svc->>DB: Insert notification record
DB-->>Svc: Success
Svc-->>API: Returns error or nil
API-->>User: 200 OK (comment posted)
Note over Frontend,DB: Notification Consumption Flow
Frontend->>Frontend: useEffect interval (60s)
Frontend->>API: GET /notifications
API->>DB: Query notifications for user
DB-->>API: Return sorted notifications
API-->>Frontend: JSON array
Frontend->>Frontend: Update state, compute unreadCount
User->>Frontend: Click notification
Frontend->>API: PUT /notifications/{id}/read
API->>DB: Update isRead = true
DB-->>API: Success
API-->>Frontend: Success message
Frontend->>Frontend: Mark item visually, update badge
alt User deletes notification
User->>Frontend: Click delete button
Frontend->>API: DELETE /notifications/{id}
API->>DB: Delete record
DB-->>API: Success
API-->>Frontend: Success message
Frontend->>Frontend: Remove from list, update unreadCount
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 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
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: 5
🧹 Nitpick comments (5)
backend/controllers/comment_controller.go (1)
100-143: Consider logging notification creation failures.The goroutine correctly uses
context.Background()for the async notification task. However, errors fromservices.CreateNotificationare silently discarded, which could make debugging notification delivery issues difficult.🔎 Proposed improvement
go func() { if req.ParentID != nil && *req.ParentID != "" { // Notify parent comment author var parent models.Comment if err := db.MongoDatabase.Collection("comments").FindOne(context.Background(), bson.M{"_id": comment.ParentID}).Decode(&parent); err == nil { if parent.UserID != userID { - services.CreateNotification( + if err := services.CreateNotification( parent.UserID, models.NotificationTypeComment, "New Reply", user.DisplayName+" replied to your comment", "/debate/"+req.TranscriptID, - ) + ); err != nil { + log.Printf("Failed to create reply notification: %v", err) + } } } } else { // ... similar handling for transcript owner notification } }()Also, note the different link patterns:
/debate/for replies (line 112) vs/view-debate/for new comments (line 138). Please verify this inconsistency is intentional.backend/services/notification_service.go (1)
13-28: Consider adding a timeout for the database operation.Using
context.Background()without a timeout means the insert could hang indefinitely if MongoDB is unresponsive. Since this function is often called asynchronously, a timeout would prevent goroutine leaks.🔎 Proposed improvement
func CreateNotification(userID primitive.ObjectID, notifType models.NotificationType, title, message, link string) error { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + notification := models.Notification{ ID: primitive.NewObjectID(), UserID: userID, Type: notifType, Title: title, Message: message, Link: link, IsRead: false, CreatedAt: time.Now(), } collection := db.GetCollection("notifications") - _, err := collection.InsertOne(context.Background(), notification) + _, err := collection.InsertOne(ctx, notification) return err }backend/controllers/notification_controller.go (1)
35-55: Consider adding pagination for notification fetching.Fetching all notifications without pagination could become problematic for active users who accumulate many notifications over time, leading to large response payloads and slower queries.
🔎 Example pagination approach
func GetNotificationsHandler(c *gin.Context) { // ... auth code ... + // Parse pagination params + limit := 50 // default + if l := c.Query("limit"); l != "" { + if parsed, err := strconv.Atoi(l); err == nil && parsed > 0 && parsed <= 100 { + limit = parsed + } + } + collection := db.GetCollection("notifications") - opts := options.Find().SetSort(bson.D{{Key: "createdAt", Value: -1}}) + opts := options.Find().SetSort(bson.D{{Key: "createdAt", Value: -1}}).SetLimit(int64(limit)) cursor, err := collection.Find(context.Background(), bson.M{"userId": userID}, opts)frontend/src/services/notificationService.ts (1)
30-70: Mutation operations don't validate response status.The
markNotificationAsRead,markAllNotificationsAsRead, anddeleteNotificationfunctions don't checkresponse.ok, so server errors (4xx/5xx) are silently ignored. While this may be acceptable for a fire-and-forget UX, it could mask issues.🔎 Proposed improvement
export const markNotificationAsRead = async (id: string): Promise<void> => { const token = localStorage.getItem('token'); if (!token) return; try { - await fetch(`${API_URL}/notifications/${id}/read`, { + const response = await fetch(`${API_URL}/notifications/${id}/read`, { method: 'PUT', headers: { Authorization: `Bearer ${token}` } }); + if (!response.ok) throw new Error('Failed to mark notification as read'); } catch (error) { console.error('Error marking notification as read:', error); } };frontend/src/components/Header.tsx (1)
46-51: Missing dependency in useEffect may cause stale closure.The
fetchNotificationsfunction is called inside the effect but not listed in the dependency array. While it currently works becausefetchNotificationsonly capturesuser, this could cause subtle bugs if the function changes. Consider usinguseCallbackor moving the fetch logic inline.🔎 Proposed fix using useCallback
+import { useCallback } from "react"; -const fetchNotifications = async () => { +const fetchNotifications = useCallback(async () => { if (user) { const data = await getNotifications(); setNotifications(data); setUnreadCount(data.filter(n => !n.isRead).length); } -}; +}, [user]); useEffect(() => { fetchNotifications(); const interval = setInterval(fetchNotifications, 60000); return () => clearInterval(interval); -}, [user]); +}, [fetchNotifications]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/cmd/server/main.gobackend/controllers/comment_controller.gobackend/controllers/gamification_controller.gobackend/controllers/notification_controller.gobackend/controllers/team_controller.gobackend/controllers/team_debate_controller.gobackend/mainbackend/models/notification.gobackend/routes/notification.gobackend/services/notification_service.gobackend/services/rating_service.gofrontend/src/components/Header.tsxfrontend/src/services/notificationService.ts
🧰 Additional context used
🧬 Code graph analysis (10)
backend/services/notification_service.go (3)
backend/models/notification.go (2)
NotificationType(9-9)Notification(19-28)frontend/src/services/notificationService.ts (1)
Notification(3-12)backend/db/db.go (1)
GetCollection(23-25)
backend/routes/notification.go (1)
backend/controllers/notification_controller.go (4)
GetNotificationsHandler(19-55)MarkNotificationAsReadHandler(58-97)MarkAllNotificationsAsReadHandler(100-127)DeleteNotificationHandler(130-168)
backend/controllers/comment_controller.go (4)
backend/models/comment.go (2)
Comment(11-23)DebatePost(48-64)backend/db/db.go (1)
MongoDatabase(18-18)backend/services/notification_service.go (1)
CreateNotification(14-29)backend/models/notification.go (1)
NotificationTypeComment(12-12)
backend/controllers/notification_controller.go (4)
backend/utils/auth.go (1)
ValidateTokenAndFetchEmail(133-146)backend/utils/user.go (1)
GetUserIDFromEmail(15-28)backend/db/db.go (1)
GetCollection(23-25)backend/models/notification.go (1)
Notification(19-28)
frontend/src/services/notificationService.ts (1)
backend/models/notification.go (1)
Notification(19-28)
backend/controllers/team_debate_controller.go (2)
backend/services/notification_service.go (1)
CreateNotification(14-29)backend/models/notification.go (1)
NotificationTypeTournament(14-14)
backend/services/rating_service.go (2)
backend/services/notification_service.go (1)
CreateNotification(14-29)backend/models/notification.go (1)
NotificationTypeLeaderboard(15-15)
backend/cmd/server/main.go (1)
backend/routes/notification.go (4)
GetNotificationsRouteHandler(9-11)MarkNotificationAsReadRouteHandler(13-15)MarkAllNotificationsAsReadRouteHandler(17-19)DeleteNotificationRouteHandler(21-23)
frontend/src/components/Header.tsx (3)
frontend/src/state/userAtom.ts (1)
userAtom(4-4)frontend/src/context/authContext.tsx (1)
AuthContext(35-37)frontend/src/services/notificationService.ts (5)
Notification(3-12)getNotifications(14-28)markNotificationAsRead(30-42)deleteNotification(58-70)markAllNotificationsAsRead(44-56)
backend/controllers/team_controller.go (2)
backend/services/notification_service.go (1)
CreateNotification(14-29)backend/models/notification.go (1)
NotificationTypeSystem(16-16)
🔇 Additional comments (6)
backend/cmd/server/main.go (1)
157-161: LGTM!The notification routes are properly registered under the authenticated group with appropriate HTTP methods for each operation.
backend/routes/notification.go (1)
1-23: LGTM!The route handler wrappers follow a clean delegation pattern to the controllers. This maintains separation between routing and business logic.
backend/models/notification.go (1)
1-28: LGTM!The notification model is well-structured with appropriate BSON/JSON tags. The
NotificationTypeconstants align correctly with the frontend interface definition.backend/controllers/notification_controller.go (1)
57-168: LGTM!The handlers correctly enforce ownership by filtering on
userIdin addition to_id, preventing unauthorized access to other users' notifications. Error handling is consistent across all operations.frontend/src/services/notificationService.ts (1)
1-28: LGTM!The
Notificationinterface aligns with the backend model, andgetNotificationsproperly handles errors with fallback to an empty array.frontend/src/components/Header.tsx (1)
127-238: Well-implemented notification and profile popovers.The notification UI provides good UX with read/unread indicators, delete on hover, and mark-all-read functionality. The user profile popover addresses the issue requirement for profile icon behavior with name, email, and logout option.
| // Create notification for the badge | ||
| go func() { | ||
| services.CreateNotification( | ||
| targetUserID, | ||
| models.NotificationTypeBadge, | ||
| "New Badge Earned!", | ||
| fmt.Sprintf("You have earned the %s badge!", req.BadgeName), | ||
| "/profile", | ||
| ) | ||
| }() |
There was a problem hiding this comment.
Log errors from CreateNotification to avoid silent failures.
Similar to other notification calls in this PR, the goroutine ignores errors from CreateNotification, which means badge award notifications may fail silently.
🔎 Proposed fix to add error logging
// Create notification for the badge
go func() {
- services.CreateNotification(
+ err := services.CreateNotification(
targetUserID,
models.NotificationTypeBadge,
"New Badge Earned!",
fmt.Sprintf("You have earned the %s badge!", req.BadgeName),
"/profile",
)
+ if err != nil {
+ log.Printf("Failed to create badge notification for user %s: %v", targetUserID.Hex(), err)
+ }
}()🤖 Prompt for AI Agents
In backend/controllers/gamification_controller.go around lines 149-158, the
goroutine calling CreateNotification ignores its returned error, causing silent
failures; modify the goroutine to capture the error returned by
services.CreateNotification, check if err != nil and log the error (using the
project's logger or log.Printf/log.Errorf) with context (e.g.,
"CreateNotification failed for user <targetUserID>: %v") so notification
creation failures are visible.
| // Notify team captain | ||
| go func() { | ||
| services.CreateNotification( | ||
| team.CaptainID, | ||
| models.NotificationTypeSystem, | ||
| "New Team Member", | ||
| user.DisplayName+" has joined your team "+team.Name, | ||
| "/team/"+teamID, | ||
| ) | ||
| }() |
There was a problem hiding this comment.
Log errors from CreateNotification to avoid silent failures.
The goroutine ignores any error returned by CreateNotification. If notification creation fails (e.g., database issue), the captain won't receive the notification and there will be no trace of the failure.
🔎 Proposed fix to add error logging
// Notify team captain
go func() {
- services.CreateNotification(
+ err := services.CreateNotification(
team.CaptainID,
models.NotificationTypeSystem,
"New Team Member",
user.DisplayName+" has joined your team "+team.Name,
"/team/"+teamID,
)
+ if err != nil {
+ log.Printf("Failed to create notification for team captain %s: %v", team.CaptainID.Hex(), err)
+ }
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Notify team captain | |
| go func() { | |
| services.CreateNotification( | |
| team.CaptainID, | |
| models.NotificationTypeSystem, | |
| "New Team Member", | |
| user.DisplayName+" has joined your team "+team.Name, | |
| "/team/"+teamID, | |
| ) | |
| }() | |
| // Notify team captain | |
| go func() { | |
| err := services.CreateNotification( | |
| team.CaptainID, | |
| models.NotificationTypeSystem, | |
| "New Team Member", | |
| user.DisplayName+" has joined your team "+team.Name, | |
| "/team/"+teamID, | |
| ) | |
| if err != nil { | |
| log.Printf("Failed to create notification for team captain %s: %v", team.CaptainID.Hex(), err) | |
| } | |
| }() |
| // Notify all members of both teams | ||
| go func() { | ||
| allMembers := append(team1.Members, team2.Members...) | ||
| for _, member := range allMembers { | ||
| services.CreateNotification( | ||
| member.UserID, | ||
| models.NotificationTypeTournament, | ||
| "Team Debate Started", | ||
| "Your team debate on '"+req.Topic+"' has started!", | ||
| "/team-debate/"+debate.ID.Hex(), | ||
| ) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Log errors from CreateNotification to track individual notification failures.
The loop notifies all team members but ignores errors from CreateNotification. If notification creation fails for specific members, there will be no record of which users didn't receive the notification.
🔎 Proposed fix to add error logging
// Notify all members of both teams
go func() {
allMembers := append(team1.Members, team2.Members...)
for _, member := range allMembers {
- services.CreateNotification(
+ err := services.CreateNotification(
member.UserID,
models.NotificationTypeTournament,
"Team Debate Started",
"Your team debate on '"+req.Topic+"' has started!",
"/team-debate/"+debate.ID.Hex(),
)
+ if err != nil {
+ log.Printf("Failed to create tournament notification for user %s: %v", member.UserID.Hex(), err)
+ }
}
}()🤖 Prompt for AI Agents
In backend/controllers/team_debate_controller.go around lines 98 to 110, the
goroutine that calls services.CreateNotification ignores returned errors; update
the loop to capture the error from CreateNotification and log it (including
member.UserID, debate.ID.Hex(), req.Topic and the error) so individual
notification failures are traceable — perform the error check inside the
goroutine and use the controller's logger or a package logger (e.g. log.Printf
or c.logger.Errorf) to record a clear message when CreateNotification returns an
error.
| // Notify users about rating updates | ||
| go func() { | ||
| if userPlayer.Rating > preUserRating { | ||
| CreateNotification( | ||
| userID, | ||
| models.NotificationTypeLeaderboard, | ||
| "Rating Increased", | ||
| fmt.Sprintf("Your rating increased by %.1f points!", userPlayer.Rating-preUserRating), | ||
| "/leaderboard", | ||
| ) | ||
| } | ||
|
|
||
| if opponentPlayer.Rating > preOpponentRating { | ||
| CreateNotification( | ||
| opponentID, | ||
| models.NotificationTypeLeaderboard, | ||
| "Rating Increased", | ||
| fmt.Sprintf("Your rating increased by %.1f points!", opponentPlayer.Rating-preOpponentRating), | ||
| "/leaderboard", | ||
| ) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Log errors from CreateNotification to avoid silent failures.
Both notification calls in the goroutine ignore errors from CreateNotification. Rating increase notifications may fail silently without any logging.
🔎 Proposed fix to add error logging
// Notify users about rating updates
go func() {
if userPlayer.Rating > preUserRating {
- CreateNotification(
+ err := CreateNotification(
userID,
models.NotificationTypeLeaderboard,
"Rating Increased",
fmt.Sprintf("Your rating increased by %.1f points!", userPlayer.Rating-preUserRating),
"/leaderboard",
)
+ if err != nil {
+ log.Printf("Failed to create rating notification for user %s: %v", userID.Hex(), err)
+ }
}
if opponentPlayer.Rating > preOpponentRating {
- CreateNotification(
+ err := CreateNotification(
opponentID,
models.NotificationTypeLeaderboard,
"Rating Increased",
fmt.Sprintf("Your rating increased by %.1f points!", opponentPlayer.Rating-preOpponentRating),
"/leaderboard",
)
+ if err != nil {
+ log.Printf("Failed to create rating notification for opponent %s: %v", opponentID.Hex(), err)
+ }
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Notify users about rating updates | |
| go func() { | |
| if userPlayer.Rating > preUserRating { | |
| CreateNotification( | |
| userID, | |
| models.NotificationTypeLeaderboard, | |
| "Rating Increased", | |
| fmt.Sprintf("Your rating increased by %.1f points!", userPlayer.Rating-preUserRating), | |
| "/leaderboard", | |
| ) | |
| } | |
| if opponentPlayer.Rating > preOpponentRating { | |
| CreateNotification( | |
| opponentID, | |
| models.NotificationTypeLeaderboard, | |
| "Rating Increased", | |
| fmt.Sprintf("Your rating increased by %.1f points!", opponentPlayer.Rating-preOpponentRating), | |
| "/leaderboard", | |
| ) | |
| } | |
| }() | |
| // Notify users about rating updates | |
| go func() { | |
| if userPlayer.Rating > preUserRating { | |
| err := CreateNotification( | |
| userID, | |
| models.NotificationTypeLeaderboard, | |
| "Rating Increased", | |
| fmt.Sprintf("Your rating increased by %.1f points!", userPlayer.Rating-preUserRating), | |
| "/leaderboard", | |
| ) | |
| if err != nil { | |
| log.Printf("Failed to create rating notification for user %s: %v", userID.Hex(), err) | |
| } | |
| } | |
| if opponentPlayer.Rating > preOpponentRating { | |
| err := CreateNotification( | |
| opponentID, | |
| models.NotificationTypeLeaderboard, | |
| "Rating Increased", | |
| fmt.Sprintf("Your rating increased by %.1f points!", opponentPlayer.Rating-preOpponentRating), | |
| "/leaderboard", | |
| ) | |
| if err != nil { | |
| log.Printf("Failed to create rating notification for opponent %s: %v", opponentID.Hex(), err) | |
| } | |
| } | |
| }() |
🤖 Prompt for AI Agents
In backend/services/rating_service.go around lines 118-139, the goroutine sends
two CreateNotification calls but ignores their returned errors; update each call
to capture the error return and log any failure (include context such as
userID/opponentID, notification type/title and the error) so notification
failures are not silent; use the package logger used elsewhere in this file (or
log.Printf if none) and keep the calls inside the goroutine so they don't block
the main flow.
| const handleDeleteNotification = async (e: React.MouseEvent, id: string) => { | ||
| e.stopPropagation(); | ||
| await deleteNotification(id); | ||
| setNotifications(prev => prev.filter(n => n.id !== id)); | ||
| // Update unread count if the deleted notification was unread | ||
| const notification = notifications.find(n => n.id === id); | ||
| if (notification && !notification.isRead) { | ||
| setUnreadCount(prev => Math.max(0, prev - 1)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Potential stale state when checking unread status after deletion.
Line 73 accesses notifications after the async deleteNotification call, but the closure captures the state at the time of the click. If notifications changed during the async operation, this could lead to incorrect unreadCount updates.
🔎 Proposed fix
const handleDeleteNotification = async (e: React.MouseEvent, id: string) => {
e.stopPropagation();
+ // Find notification before async operation to avoid stale state
+ const notification = notifications.find(n => n.id === id);
+ const wasUnread = notification && !notification.isRead;
+
await deleteNotification(id);
setNotifications(prev => prev.filter(n => n.id !== id));
- // Update unread count if the deleted notification was unread
- const notification = notifications.find(n => n.id === id);
- if (notification && !notification.isRead) {
+ if (wasUnread) {
setUnreadCount(prev => Math.max(0, prev - 1));
}
};
|
@DeveloperAmrit I see so many PRs |
|
@bhavik-mangla Here is the long video demonstrating it. I have tested them locally with two different accounts parallelly Screen.Recording.2026-01-08.at.7.56.46.PM.mov |
|
good work @DeveloperAmrit |
2 participants
Fixes #155 second part. Added notification system that notifies for
Comments: Updated comment_controller.go to notify users when someone replies to their comment or comments on their transcript.
Badges: Updated gamification_controller.go to notify users when they earn a new badge.
Tournaments: Updated team_debate_controller.go to notify team members when a team debate starts.
Leaderboard/Rating: Updated rating_service.go to notify users when their rating increases after a debate.
Team Join: Updated team_controller.go to notify the captain when a new member joins the team.
Video
Screen.Recording.2026-01-08.at.7.56.46.PM.mov
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.