feat: Build Notifications Center & Activity Feed #99
Conversation
- Created notification model with user, title, description, category, read status, action URL, and metadata - Implemented notification controller with CRUD operations (get, mark as read, mark all as read, delete, unread count) - Added notification routes with proper authentication and rate limiting - Integrated notification routes into server.js - Created dedicated Notifications page with filtering by status and category - Added Notifications route to App.jsx with ProtectedRoute - Updated Navbar to link notification popover and mobile menu to Notifications page - Implemented read/unread visual distinction with blue indicators - Added mark as read, mark all as read, and delete functionality - Included empty state, loading state, and error handling - Designed responsive layout for desktop, tablet, and mobile - Followed MeetOnMemory design language with consistent styling
|
@mrunmayeekokitkar is attempting to deploy a commit to the Shiv Raj Singh's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a Notifications Center feature: a Mongoose notification model, Express controller with five handlers (get, mark-as-read, mark-all-read, delete, unread-count), a rate-limited router mounted at /api/notifications, a new React Notifications page, and updated App.jsx/Navbar.jsx routing/navigation to that page. ChangesNotifications Center
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Navbar
participant NotificationsPage
participant NotificationAPI
participant MongoDB
User->>Navbar: Click notification bell
Navbar->>NotificationsPage: Navigate to /notifications
NotificationsPage->>NotificationAPI: GET /api/notifications
NotificationAPI->>MongoDB: find(user, category, isRead filters)
MongoDB-->>NotificationAPI: notifications + unreadCount
NotificationAPI-->>NotificationsPage: response
NotificationsPage-->>User: Render list with filters
User->>NotificationsPage: Mark as read / Delete
NotificationsPage->>NotificationAPI: PATCH/DELETE /api/notifications/:id
NotificationAPI->>MongoDB: update/delete notification
MongoDB-->>NotificationAPI: result
NotificationAPI-->>NotificationsPage: success response
NotificationsPage-->>User: Update UI state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Remove unused markAllAsRead function from Navbar.jsx - Add input validation to notificationController category filter (whitelist allowed categories) - Rate limiting already present in all notification routes (apiLimiter/writeLimiter)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Refactor notificationController to use Mongoose query builder instead of user-controlled filter object - Move userAuth middleware to router level in notificationRoutes for clearer rate limiting application
|
@mrunmayeekokitkar Thank you for the quick follow-up and for addressing the feedback! 🚀 I appreciate the effort you put into improving the implementation. The query refactor in Requested Changes
Once these items are addressed and all CodeQL checks pass, I'll be happy to review the updated implementation again. Thanks again for your continued contribution to MeetOnMemory! 🎉 |
imuniqueshiv
added
enhancement
![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)
client/src/components/Navbar.jsx (1)
49-80: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDesktop popover still displays hardcoded mock notifications.
notificationsandunreadCountare now read-only state (no setters captured) with hardcoded mock data. The desktop popover (lines 365-394) still renders these fake notifications, while the actual Notifications page fetches real data from the API. This creates an inconsistent UX — users see fabricated notifications in the popover that don't reflect reality.Consider either fetching real unread count/notifications for the popover badge, or simplifying the popover to just a "View All" link without the mock list.
Also applies to: 365-394
🤖 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 `@client/src/components/Navbar.jsx` around lines 49 - 80, The Navbar desktop popover is still rendering hardcoded mock data through the unreadCount and notifications state, which makes it inconsistent with the real Notifications page. Update Navbar’s popover logic to stop using the fake notifications array and unread badge value, and instead either wire it to the same API-backed unread notifications data used by the Notifications page or remove the mock list entirely and keep only a “View All” action in the popover. Use the existing Navbar popover rendering block and the notifications/unreadCount state declarations as the main symbols to update.
🧹 Nitpick comments (5)
server/controllers/notificationController.js (2)
56-56: 🚀 Performance & Scalability | 🔵 TrivialConsider adding pagination for the notifications list.
limit(100)returns the most recent 100 notifications but provides no way to page through older ones. Users with high notification volume will lose access to historical items. Consider addingskip/pagequery parameters or cursor-based pagination.🤖 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 `@server/controllers/notificationController.js` at line 56, The notifications query in notificationController is capped with limit(100) but has no way to access older results. Update the notifications list endpoint to support pagination, using either page/skip query parameters or cursor-based pagination alongside the existing sort on createdAt. Keep the change centered around the notifications query so callers can request historical notifications instead of only the latest 100.
36-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the repetitive category filter chain.
Six
if/elsebranches each do the samequery.where("category").equals(...)with a hardcoded string. This is verbose and error-prone when adding new categories. Since the values are already constrained by the model's enum, a single check against a whitelist suffices.♻️ Proposed simplification
- if (category === "meetings") { - query = query.where("category").equals("meetings"); - } else if (category === "ai_processing") { - query = query.where("category").equals("ai_processing"); - } else if (category === "organizations") { - query = query.where("category").equals("organizations"); - } else if (category === "policies") { - query = query.where("category").equals("policies"); - } else if (category === "reports") { - query = query.where("category").equals("reports"); - } else if (category === "system") { - query = query.where("category").equals("system"); - } + const validCategories = [ + "meetings", + "ai_processing", + "organizations", + "policies", + "reports", + "system", + ]; + if (validCategories.includes(category)) { + query = query.where("category").equals(category); + }🤖 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 `@server/controllers/notificationController.js` around lines 36 - 48, The category filtering in notificationController’s getNotifications logic is repetitive and should be simplified. Replace the long if/else chain around query.where("category").equals(...) with a single whitelist-based check for the allowed enum values, then apply the filter once when the category is valid. Keep the logic localized near the existing category handling in notificationController so future category additions only require updating the allowed list.server/models/notificationModel.js (1)
6-36: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a compound index for the dominant query patterns.
The controller's most frequent queries filter by both
userandisRead(e.g.,countDocuments({ user, isRead: false })) and sort bycreatedAt: -1. Separate single-field indexes onuserandisReadforce MongoDB to scan or intersect indexes. A compound index would cover these patterns more efficiently.♻️ Proposed compound index
user: { type: mongoose.Schema.Types.ObjectId, ref: "user", required: true, - index: true, }, title: { type: String, required: true, }, description: { type: String, required: true, }, category: { type: String, enum: [ "meetings", "ai_processing", "organizations", "policies", "reports", "system", ], default: "system", }, isRead: { type: Boolean, default: false, - index: true, }, actionUrl: { type: String, default: "", }, actionLabel: { type: String, default: "", }, metadata: { type: mongoose.Schema.Types.Mixed, default: {}, }, }, - { timestamps: true }, + { timestamps: true }, ); + +notificationSchema.index({ user: 1, isRead: 1, createdAt: -1 });🤖 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 `@server/models/notificationModel.js` around lines 6 - 36, Add a compound index to the notification schema in notificationModel so the common queries on user plus isRead and sorting by createdAt are optimized. Update the schema definition near the user, isRead, and createdAt fields to include a compound index that matches the controller’s countDocuments and latest-notification lookup patterns, rather than relying only on the existing single-field indexes.client/src/components/Navbar.jsx (1)
40-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead
mobileNotifOpenstate.After the refactor,
mobileNotifOpenis never set totrue— the mobile notifications button (line 620) now navigates to/notificationsand closes the drawer. The state is still referenced in the resize handler (line 122) and for button styling (lines 626-633), but it will always befalse, making the conditional styling dead code.♻️ Proposed refactor
Remove the state declaration and all references:
- const [mobileNotifOpen, setMobileNotifOpen] = useState(false);const onResize = () => { if (window.innerWidth >= 768) { setMobileOpen(false); - setMobileNotifOpen(false); } };className={`w-full flex items-center justify-between px-4 py-3 text-sm font-semibold rounded-xl transition-all cursor-pointer ${ - mobileNotifOpen - ? "bg-blue-50/50 text-blue-600" - : "text-gray-700 hover:bg-gray-50 hover:text-gray-900" + "text-gray-700 hover:bg-gray-50 hover:text-gray-900" }`}<Bell - className={`w-4 h-4 ${mobileNotifOpen ? "text-blue-600" : "text-gray-400"}`} + className="w-4 h-4 text-gray-400" />Also applies to: 122-122, 626-633
🤖 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 `@client/src/components/Navbar.jsx` at line 40, The mobile notifications open state in Navbar is now dead code because it is never set to true after the refactor. Remove the mobileNotifOpen state declaration from Navbar and clean up all remaining references in the resize handler and the mobile notifications button styling, keeping the navigation-to-/notifications behavior and drawer close logic intact.client/src/pages/Notifications.jsx (1)
178-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary
filteredNotificationsalias.
filteredNotificationsis justnotifications— the variable name implies client-side filtering, but filtering is done server-side via query params. Usenotificationsdirectly to avoid confusion.♻️ Proposed refactor
- const filteredNotifications = notifications; - return (Then replace
filteredNotificationsreferences on lines 277 and 293 withnotifications.🤖 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 `@client/src/pages/Notifications.jsx` at line 178, The `filteredNotifications` alias in `Notifications` is unnecessary because it just mirrors `notifications` and suggests client-side filtering that does not exist. Remove the `filteredNotifications` variable and update the affected references in the same component to use `notifications` directly, especially where the list is rendered and any related checks are performed.
🤖 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.
Inline comments:
In `@client/src/pages/Notifications.jsx`:
- Around line 146-167: The handleDelete flow in Notifications.jsx is updating
unread count as a side effect inside the setNotifications updater, which should
stay pure. Move the unread-count calculation out of the setNotifications
callback in handleDelete, using the existing id, unreadCount, and deleted
notification state to compute the new count before or after the state update,
then call setUnreadCount separately and keep setNotifications limited to
returning the filtered notifications array.
---
Outside diff comments:
In `@client/src/components/Navbar.jsx`:
- Around line 49-80: The Navbar desktop popover is still rendering hardcoded
mock data through the unreadCount and notifications state, which makes it
inconsistent with the real Notifications page. Update Navbar’s popover logic to
stop using the fake notifications array and unread badge value, and instead
either wire it to the same API-backed unread notifications data used by the
Notifications page or remove the mock list entirely and keep only a “View All”
action in the popover. Use the existing Navbar popover rendering block and the
notifications/unreadCount state declarations as the main symbols to update.
---
Nitpick comments:
In `@client/src/components/Navbar.jsx`:
- Line 40: The mobile notifications open state in Navbar is now dead code
because it is never set to true after the refactor. Remove the mobileNotifOpen
state declaration from Navbar and clean up all remaining references in the
resize handler and the mobile notifications button styling, keeping the
navigation-to-/notifications behavior and drawer close logic intact.
In `@client/src/pages/Notifications.jsx`:
- Line 178: The `filteredNotifications` alias in `Notifications` is unnecessary
because it just mirrors `notifications` and suggests client-side filtering that
does not exist. Remove the `filteredNotifications` variable and update the
affected references in the same component to use `notifications` directly,
especially where the list is rendered and any related checks are performed.
In `@server/controllers/notificationController.js`:
- Line 56: The notifications query in notificationController is capped with
limit(100) but has no way to access older results. Update the notifications list
endpoint to support pagination, using either page/skip query parameters or
cursor-based pagination alongside the existing sort on createdAt. Keep the
change centered around the notifications query so callers can request historical
notifications instead of only the latest 100.
- Around line 36-48: The category filtering in notificationController’s
getNotifications logic is repetitive and should be simplified. Replace the long
if/else chain around query.where("category").equals(...) with a single
whitelist-based check for the allowed enum values, then apply the filter once
when the category is valid. Keep the logic localized near the existing category
handling in notificationController so future category additions only require
updating the allowed list.
In `@server/models/notificationModel.js`:
- Around line 6-36: Add a compound index to the notification schema in
notificationModel so the common queries on user plus isRead and sorting by
createdAt are optimized. Update the schema definition near the user, isRead, and
createdAt fields to include a compound index that matches the controller’s
countDocuments and latest-notification lookup patterns, rather than relying only
on the existing single-field indexes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dce7484-03b3-4d8e-b2f2-9f7af253cffe
📒 Files selected for processing (7)
client/src/App.jsxclient/src/components/Navbar.jsxclient/src/pages/Notifications.jsxserver/controllers/notificationController.jsserver/models/notificationModel.jsserver/routes/notificationRoutes.jsserver/server.js
- Apply apiLimiter at router level in notificationRoutes for CodeQL detection - Update Navbar to fetch unread count and recent notifications from backend - Remove hardcoded mock notification data from Navbar - Add formatTimeAgo helper function for consistent timestamp display
|
🎉 Thank you for your contribution! Your pull request has been successfully merged. We appreciate your effort in improving MeetOnMemory, @mrunmayeekokitkar! |
|
@mrunmayeekokitkar Thank you for your excellent contribution and for your patience throughout the review process! 🚀 This is a substantial feature addition that significantly improves the MeetOnMemory experience by introducing a complete Notifications Center with backend APIs, responsive frontend integration, filtering, read/unread management, and a much cleaner notification workflow. I also appreciate your responsiveness in addressing the review feedback, including:
Regarding the remaining GitHub Advanced Security warning: after reviewing the implementation, the notification routes are already protected by both authentication and rate limiting middleware. The remaining alert appears to be a CodeQL false positive related to its analysis of the middleware chain rather than an actual security vulnerability, so it has been dismissed accordingly. Overall, this is a well-implemented feature that integrates nicely with the existing architecture while keeping the codebase maintainable and consistent. Thank you again for your contribution to MeetOnMemory! Looking forward to seeing more of your work in future issues. 🎉 |
3 participants
Pull Request
Description
Implements a dedicated Notifications Center & Activity Feed for MeetOnMemory, providing authenticated users with a centralized place to view, manage, and filter notifications. This update adds a complete backend notification system, a responsive frontend notifications page, and integrates notification navigation into the existing application while following the MeetOnMemory design language.
Type of Change
Related Issue
Closes #78
Changes
Backend
Created a complete notification management system.
Notification Model
Created
notificationModel.jswith support for:Supported notification categories include:
Notification Controller
Created
notificationController.jsimplementing:Notification Routes
Created
notificationRoutes.jswith:Server Integration
Updated
server.jsto register notification routes at:Frontend
Notifications Page
Created a dedicated
Notifications.jsxpage featuring:Filters
Added filtering by:
Status
Category
Supports filtering by notification category through a dropdown.
Read / Unread State
Implemented clear visual distinction using blue indicators for unread notifications.
Notification Actions
Added support for:
Navigation
Updated:
App.jsx/notificationsroute.Navbar.jsxUser Experience
Added:
Implementation Summary
Backend Files Created
notificationModel.jsnotificationController.jsnotificationRoutes.jsBackend Updates
Updated:
server.jsFrontend Files Created
Notifications.jsxFrontend Updates
Updated:
App.jsxNavbar.jsxMinimal Refactor
This implementation was intentionally kept focused, modifying only 7 files:
New Files (4)
notificationModel.jsnotificationController.jsnotificationRoutes.jsNotifications.jsxModified Files (3)
server.jsApp.jsxNavbar.jsxThis minimizes impact on existing functionality while delivering the complete Notifications Center.
Features Implemented
Testing
Verification
Screenshots
N/A
Checklist
Summary by CodeRabbit
New Features
Bug Fixes