fix: notifications pagination + dropdown updates by Anshpujara5 · Pull Request #86 · tarinagarwal/Edulume

Anshpujara5 · 2026-01-05T14:53:46Z

Description

Adds pagination to the notifications system so notifications load in batches instead of all at once.

Backend pagination parameters (page, limit) are verified and used correctly, and the frontend notification dropdown now supports incremental loading via a Load More button. This improves performance and provides a smoother user experience for users with large notification histories.

Type of Change

Bug fix
New feature

Related Issues

Closes #13

Testing

Tested locally
Client build (npm run build) passes successfully
Client lint reports existing repository-wide lint issues unrelated to this PR
Server has no lint script; notifications endpoint tested manually

Screenshots

Checklist

Code follows existing project style guidelines
Self-reviewed the code
Added comments for complex logic where needed
Documentation update not required for this change

Anshpujara5 · 2026-01-08T16:18:28Z

Hi @tarinagarwal 👋

Just a gentle follow-up regarding this PR.
Whenever you get a chance, could you please review it and let me know if any changes are required from my side?

I’m happy to iterate or make updates as needed.
Thanks for your time and guidance 😊

tarinagarwal · 2026-01-08T17:38:10Z

@Anshpujara5 Hey! ill review this pr asap, sorry for the delay

tarin-lgtm

Changes Requested 🐈

This PR adds pagination to the notifications system, improving performance by loading notifications in batches and enhancing the UI with a Load More button. Review found improvements in backend pagination and UI, but raised concerns about input validation, potential race conditions, inefficient client-side data handling, lack of test coverage, and missing documentation.

There are a few things I'd like to see addressed before we merge this:

Before merging

Add input validation and sanitization for notification IDs and pagination parameters to improve security.
Refactor NotificationDropdown component to improve readability and maintainability, including adding JSDoc comments and reducing complexity.
Optimize client-side notification handling by using efficient data structures for duplicate detection and read status updates, and add test coverage for new features.

Findings breakdown (35 total)

3 high / 12 medium / 15 low / 5 info

Confidence: 90%

🔗 View Full Review Report — detailed findings, severity breakdown, and agent analysis

_{Reviewed by Looks Good To Meow — AI-powered code review}

tarin-lgtm · 2026-04-07T09:23:20Z


 const NotificationDropdown: React.FC = () => {
-  const [notifications, setNotifications] = useState<Notification[]>([]);
+  const [notifications, setNotifications] = useState<AppNotification[]>([]);


⚠️ High — The NotificationDropdown component now implements pagination for notifications with page size 20 and supports loading more notifications on demand. This is a positive change preventing unbounded notification fetches.

No action needed; pagination is correctly implemented for notifications.

_performance

tarin-lgtm · 2026-04-07T09:23:20Z

-        where: { userId: req.user.id },
-      }),
-    ]);
+    const [notifications, total,unreadCount] = await Promise.all([


⚠️ High — The notifications API endpoint now supports pagination with page and limit parameters, fetching notifications with skip and take, and returns pagination metadata. This prevents unbounded queries and large data transfers.

No action needed; pagination is properly implemented in the notifications API.

_performance

tarin-lgtm · 2026-04-07T09:23:20Z


 const NotificationDropdown: React.FC = () => {
-  const [notifications, setNotifications] = useState<Notification[]>([]);
+  const [notifications, setNotifications] = useState<AppNotification[]>([]);


⚠️ High — The exported React functional component 'NotificationDropdown' lacks a JSDoc comment describing its purpose, parameters, and return value.

Add a JSDoc comment above the 'NotificationDropdown' component explaining its functionality, props (if any), and what it returns.

_{documentation}

tarin-lgtm · 2026-04-07T09:23:20Z


 const NotificationDropdown: React.FC = () => {
-  const [notifications, setNotifications] = useState<Notification[]>([]);
+  const [notifications, setNotifications] = useState<AppNotification[]>([]);


🔍 Medium — The 'notifications' state is updated inside the socket 'new_notification' event handler using a functional update, but the 'unreadCount' state is updated separately inside the same handler. This can cause race conditions or inconsistent state if multiple notifications arrive rapidly.

Combine the updates to 'notifications' and 'unreadCount' into a single state update or use a reducer to manage both states atomically.

_bugs

tarin-lgtm · 2026-04-07T09:23:20Z

-        where: { userId: req.user.id },
-      }),
-    ]);
+    const [notifications, total,unreadCount] = await Promise.all([


🔍 Medium — Notification read status update endpoint checks that the notification belongs to the authenticated user before allowing update, which is good. However, the notification ID is taken directly from the URL parameter without validation or sanitization. While Prisma ORM likely handles parameterization, ensure that the ID is validated to prevent injection or malformed input.

Validate and sanitize the notification ID parameter before using it in database queries. Use proper type checks and reject invalid IDs early.

_security

tarin-lgtm · 2026-04-07T09:23:21Z

        {unreadCount > 0 && (
-          <span className="absolute -top-1 -right-1 bg-red-500 text-white text-xs rounded-full h-5 w-5 flex items-center justify-center animate-pulse">
-            {unreadCount > 9 ? "9+" : unreadCount}
+          <span className="absolute -top-1 -right-1 z-[9999] bg-red-500 text-white text-[10px] font-semibold rounded-full h-5 min-w-5 px-1 flex items-center justify-center">


💡 Suggestion — The unread count badge shows the exact number even if it exceeds 9, whereas previously it showed '9+'. This may cause UI inconsistency or overflow.

Consider limiting the displayed count to '9+' or a max value to maintain consistent badge size and appearance.

_{best-practices}

tarin-lgtm · 2026-04-07T09:23:21Z

              notifications.map((notification) => (
                <div
-                  key={notification.id}
+                  key={getNotifId(notification)}


💡 Suggestion — Notification list items use getNotifId(notification) as key, which may cause issues if IDs are not unique or stable.

Ensure that notification IDs are unique and stable; otherwise, consider a more stable key or include additional identifiers.

_{best-practices}

tarin-lgtm · 2026-04-07T09:23:21Z

-                          </p>
+                        </span>
+                        {!notification.is_read && (
+                          <button


💡 Suggestion — The 'Mark as read' button inside each notification item stops propagation on click but does not prevent default, which may cause unexpected behavior in some browsers.

Add e.preventDefault() along with e.stopPropagation() to fully prevent event bubbling and default actions.

_{best-practices}

tarin-lgtm · 2026-04-07T09:23:21Z

+            {page < totalPages && (
+              <div className="p-3 text-center">
+                <button
+                  onClick={() => fetchNotifications(page + 1)}


💡 Suggestion — The 'Load More' button for pagination does not have aria-label or accessible name, which may affect accessibility.

Add aria-label or descriptive text for screen readers to improve accessibility.

_{best-practices}

tarin-lgtm · 2026-04-07T09:23:21Z

-    socket.on("new_notification", (notification: Notification) => {
-      setNotifications((prev) => [notification, ...prev]);
-      setUnreadCount((prev) => prev + 1);
+    const handler = (notification: AppNotification) => {


💡 Suggestion — The variable 'n' in the getNotifId function and in the some() callback is typed as 'any', which reduces type safety and clarity.

Define a proper type for the notification parameter in getNotifId and the some() callback to improve type safety and readability.

_readability

Anshpujara5 added 5 commits December 27, 2025 16:40

chore: ignore python venv

c8d0223

feat: add skeleton loaders to main pages

c04de90

feat: add skeleton loaders to ebooks and discussion detail

366b81a

fix: notifications pagination and dropdown updates

928806d

fix: notifications pagination and dropdown updates

67a90d1

tarin-lgtm Bot suggested changes Apr 7, 2026

View reviewed changes

Conversation

Anshpujara5 commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Related Issues

Testing

Screenshots

Checklist

Uh oh!

Anshpujara5 commented Jan 8, 2026

Uh oh!

tarinagarwal commented Jan 8, 2026

Uh oh!

tarin-lgtm Bot left a comment

Choose a reason for hiding this comment

Changes Requested 🐈

Before merging

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

tarin-lgtm Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Anshpujara5 commented Jan 5, 2026 •

edited

Loading