Feature/notification badge popup

[DarinHajou]

In this PR we're introducing a new /notifications/count endpoint to return the number of uncleared notifications for the authenticated user. We also updated the main /notifications GET route to pull pi_uid from req.currentUser for improved security. Cleared/uncleared filter logic was added to notification.service.ts, and the new endpoint is wired to support frontend notification badge behavior.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
map-of-pi-backend-react	Ready	Preview	Comment	Sep 4, 2025 2:40pm

[amt-fredrick-amoako]

Hello @DarinHajou, I don't now much about the notification apis and the requirements yet, so as a suggestion, I feel like having a separate endpoint to get the count of unread notifications to display as a batch icon could be a lot of queries to the server at a certain point and one way we can look at this would be to build a response object that will include all the necessary details about a notification for a user including the number of unread ones, for example a response could have a list of notification objects where each has a status we can count at the client side, or if doing that would be too much work, then the server could perform the counting and include it as a property in the response. Like I said, I am not familiar with the notifications aspect yet so it could be that I could be missing certain points in the requirements.

[swoocn]

Hello @DarinHajou, I don't now much about the notification apis and the requirements yet, so as a suggestion, I feel like having a separate endpoint to get the count of unread notifications to display as a batch icon could be a lot of queries to the server at a certain point and one way we can look at this would be to build a response object that will include all the necessary details about a notification for a user including the number of unread ones, for example a response could have a list of notification objects where each has a status we can count at the client side, or if doing that would be too much work, then the server could perform the counting and include it as a property in the response. Like I said, I am not familiar with the notifications aspect yet so it could be that I could be missing certain points in the requirements.

I agree with @amt-fredrick-amoako. Could we consider leveraging the existing getNotifications endpoint to return both the list and the count in a single response? This would allow the client to make just one call to fetch both the data and the badge count, instead of two separate queries/endpoints for the same use case (list + badge). It also reduces duplication of filter logic and helps keep the API surface area smaller. On the FE, we could then persist the response from the getNotifications call and only trigger background refreshes from the same endpoint (with skip/limit) for pagination.

[swoocn]

Please see latest comments @DarinHajou.

[DarinHajou]

Hi @amt-fredrick-amoako, @swoocn. I agree, I guess it makes sense to merge the enpoints and get the fetching list + count together. Thanks for pointing that out. I prolly should've consolidated it before pushing. Do you want me to go ahead and make the changes?

[swoocn]

Hi @amt-fredrick-amoako, @swoocn. I agree, I guess it makes sense to merge the enpoints and get the fetching list + count together. Thanks for pointing that out. I prolly should've consolidated it before pushing. Do you want me to go ahead and make the changes?

Yes, that would be great if you could update this PR accordingly. Thanks @DarinHajou!

[DarinHajou]

Hi,

As I mentioned in stand-up, I noticed the notifications setup could be improved a bit further.

Right now we're creating notifications through the frontend (POST /notifications), which could let users manipulate the payload. Maybe it's better to move that logic to the backend instead.

Have a look @amt-fredrick-amoako, @omotoshoayomikun, @swoocn, when you can and let me know what you think.

[swoocn]

Hi,

As I mentioned in stand-up, I noticed the notifications setup could be improved a bit further.

Right now we're creating notifications through the frontend (POST /notifications), which could let users manipulate the payload. Maybe it's better to move that logic to the backend instead.

Have a look @amt-fredrick-amoako, @omotoshoayomikun, @swoocn, when you can and let me know what you think.

Hi @DarinHajou - I'm not sure I fully understand what you mean by creating notifications through the frontend. Could you please clarify? As of now, we haven't implemented notification creation yet. There's a WIP task for this assigned to @amt-fredrick-amoako, as per @ https://sharing.clickup.com/9015444408/t/h/86c4nfx23/8DZGCP6CJT2O8KN.

[DarinHajou]

Hi,
As I mentioned in stand-up, I noticed the notifications setup could be improved a bit further.
Right now we're creating notifications through the frontend (POST /notifications), which could let users manipulate the payload. Maybe it's better to move that logic to the backend instead.
Have a look @amt-fredrick-amoako, @omotoshoayomikun, @swoocn, when you can and let me know what you think.

Hi @DarinHajou - I'm not sure I fully understand what you mean by creating notifications through the frontend. Could you please clarify? As of now, we haven't implemented notification creation yet. There's a WIP task for this assigned to @amt-fredrick-amoako, as per @ https://sharing.clickup.com/9015444408/t/h/86c4nfx23/8DZGCP6CJT2O8KN.

OK, thanks @swoocn. After looking closer, I see the intention was never for the FE to trigger notifications.

What I meant is that we currently have a POST /notifications endpoint, which technically allows a client to create notifications by posting a payload. Even if that’s not how we plan to use it, it still leaves a potential risk since notifications aren’t user-triggered actions, they're by-products of backend events. That’s what I meant by my comment.