NCTO-155-make-notifications-system by Notgoyome · Pull Request #17 · Naucto/Backend

Notgoyome · 2026-02-28T03:18:51Z

Jira ticket

https://naucto.atlassian.net/jira/software/projects/NCTO/boards/2?selectedIssue=NCTO-155

What does your MR do ?

Implementing notifications system via websocket and storing notifications in backend

How to test it

swagger

Screenshot

Notes

Copilot

Pull request overview

Implements a backend notifications system backed by Prisma (persisted notifications) and a WebSocket channel to push notifications to connected clients, wired into the NestJS application.

Changes:

Added Notification Prisma model + migration and linked it to User.
Added NotificationsService + module + controller endpoints to create/test and mark notifications as read.
Added a dedicated WebSocket server (/socket/notifications) that authenticates via JWT and emits init + realtime notification messages.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
src/notifications/notifications.types.ts	Adds shared types for notification payloads and creation input.
src/notifications/notifications.socket.ts	Implements notification WebSocket server, auth, connection tracking, and emit helper.
src/notifications/notifications.service.ts	Adds Prisma-backed CRUD-like operations (list, create+prune, mark read).
src/notifications/notifications.module.ts	Registers controller/service and exports the service.
src/notifications/notifications.controller.ts	Adds REST endpoints for test notification + mark-as-read operations.
src/notifications/dto/notification-test.dto.ts	Adds DTO validation + Swagger metadata for test notification creation.
src/main.ts	Wires notification WebSocket setup into server bootstrap.
src/collab/signaling/signal.ts	Scopes WebRTC upgrade handling to avoid clashing with other WS endpoints.
src/app.module.ts	Registers the new NotificationsModule in the app.
prisma/models/user.prisma	Adds User ↔ Notification relation.
prisma/models/notification.prisma	Defines Notification model schema and indexes.
prisma/migrations/20260222071209_add_notifications/migration.sql	Creates Notification table, indexes, and FK.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/notifications/notifications.controller.ts

Copilot · 2026-02-28T03:23:40Z

src/notifications/notifications.socket.ts

+let notificationWss: WebSocketServer | undefined;
+const userSockets = new Map<number, Set<WebSocket>>();
+


This WebSocket implementation stores active user connections in an in-memory Map. In a multi-instance/auto-scaling deployment, notifications will only reach clients connected to the same node that emitted them, leading to inconsistent delivery. If horizontal scaling is expected, consider a shared pub/sub (e.g., Redis) and/or a dedicated gateway layer.

src/notifications/notifications.service.ts

src/collab/signaling/signal.ts

Copilot · 2026-02-28T03:23:41Z

src/notifications/notifications.service.ts

+  async createNotification(input: CreateNotificationInput): Promise<NotificationPayload> {
+    const created = await this.prisma.$transaction(async (tx) => {
+      const notification = await tx.notification.create({
+        data: {
+          userId: input.userId,
+          title: input.title,
+          message: input.message,
+          type: input.type,
+        },
+      });
+
+      const extraNotifications = await tx.notification.findMany({
+        where: { userId: input.userId },
+        orderBy: [
+          { createdAt: "desc" },
+          { id: "desc" },
+        ],
+        skip: MAX_NOTIFICATIONS_PER_USER,
+        select: { id: true },
+      });
+
+      if (extraNotifications.length > 0) {
+        await tx.notification.deleteMany({
+          where: {
+            id: { in: extraNotifications.map((entry) => entry.id) },
+          },
+        });
+      }
+
+      return notification;
+    });
+
+    const payload = this.toPayload(created);
+    emitNotificationToUser(input.userId, payload);
+    return payload;
+  }


NotificationsService introduces non-trivial behavior (transactional create + pruning, markAsRead ownership checks) but there are no unit tests validating these scenarios. Given the repo already uses Jest for services/controllers, please add tests covering creation/pruning limits and markAsRead/markAllAsRead behavior (including invalid ids and not-found cases).

tests dont work for now

They do btw, even if they break every now and then because of changes, you can create test and then run npm run test to see the tests.

@Notgoyome I fixed them all in #14, just have to make sure that I didn't break stuff with the front and I'll do the merge once this is integrated

Copilot · 2026-02-28T03:23:41Z

src/notifications/notifications.socket.ts

+
+  server.on("upgrade", (request, socket, head) => {
+    const requestUrl = new URL(request.url ?? "/", "http://localhost");
+    if (!requestUrl.pathname.startsWith(NOTIFICATION_SOCKET_PATH)) {


This upgrade handler returns early when the path doesn’t match, but it does not close/destroy the underlying TCP socket. If an Upgrade request comes in for an unexpected path (or if no other upgrade handler handles it), the connection can hang and leak resources. Consider routing upgrades centrally and calling socket.destroy() (or writing a 400/404 response) when no handler matches.

Suggested change

if (!requestUrl.pathname.startsWith(NOTIFICATION_SOCKET_PATH)) {

if (!requestUrl.pathname.startsWith(NOTIFICATION_SOCKET_PATH)) {

socket.destroy();

src/notifications/notifications.socket.ts

src/notifications/dto/notification-test.dto.ts

src/collab/signaling/signal.ts

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/notifications/notifications.socket.ts

src/main.ts

src/notifications/dto/notification-test.dto.ts

src/notifications/notifications.controller.ts

src/notifications/notifications.service.ts

src/notifications/dto/notification-test.dto.ts

…ns controller

…tication logic

…per user

Priax · 2026-03-01T12:08:47Z

prisma/models/notification.prisma

+  userId    Int
+  title     String
+  message   String
+  type      String


Wouldn't it be better to have an enum / something different for the notification type ? If we keep it as a string, it might create to many very similar notification types while we could easily control what types we want for notifications

I agree, this needs to be enum -- otherwise clients could send unknown notification types that just overload the system -- notification types should be checked when received

Priax · 2026-03-01T12:10:27Z

src/notifications/notifications.service.ts

+    createdAt: Date;
+  }): NotificationPayload {
+    return {
+      id: String(notification.id),


Kinda pedantic but notification.id.toString() would be better in the context that toString() is used everywhere else, but it's not that important

I think that doing String(...) over toString is clearer imho

That's also fair but then the change needs to be done in other places to keep it logical

Priax · 2026-03-01T12:17:19Z

src/notifications/notifications.socket.ts

+  }
+};
+
+const authenticateToken = (token: string, jwtSecret: string): number | null => {


This is kinda the case for this whole file, why are you recreating the whole JWT token verification ?
Use the JwtAuthGuards in the auth directory. This is a redo of an already existing middleware and could be a problem

Priax · 2026-03-01T12:22:31Z

src/notifications/notifications.service.ts

+export class NotificationsService {
+  constructor(private readonly prisma: PrismaService) {}
+
+  async getUserNotifications(userId: number): Promise<NotificationPayload[]> {


Idk for what use the Notifications are going to be used, but wouldn't it be better, instead of using a notification cap, to implement pagination, as to be able to get older notifications and implementing the limit in the getUserNotifications method ? If it's not useful it's ok to do it like that

Since there is a MAX_NOTIFICATIONS_PER_USER, this is ok-ish if the notifications aren't that big in size

alexis-belmonte · 2026-03-01T13:15:51Z

Needed by #14

alexis-belmonte · 2026-03-01T13:13:14Z

src/main.ts

+  const notificationsService = app.get(NotificationsService);
+  const jwtSecret = configService.getOrThrow<string>("JWT_SECRET");
+
+  setupWebSocketServer(server);


Should be refactored to something clearer like setupCollaborationSocket

alexis-belmonte · 2026-03-01T13:16:40Z

src/notifications/notifications.socket.ts

+  }
+};
+
+const authenticateToken = (token: string, jwtSecret: string): number | null => {


alexis-belmonte · 2026-03-01T13:18:26Z

src/notifications/notifications.service.ts

+  async createNotification(input: CreateNotificationInput): Promise<NotificationPayload> {
+    const created = await this.prisma.$transaction(async (tx) => {
+      const notification = await tx.notification.create({
+        data: {
+          userId: input.userId,
+          title: input.title,
+          message: input.message,
+          type: input.type,
+        },
+      });
+
+      const extraNotifications = await tx.notification.findMany({
+        where: { userId: input.userId },
+        orderBy: [
+          { createdAt: "desc" },
+          { id: "desc" },
+        ],
+        skip: MAX_NOTIFICATIONS_PER_USER,
+        select: { id: true },
+      });
+
+      if (extraNotifications.length > 0) {
+        await tx.notification.deleteMany({
+          where: {
+            id: { in: extraNotifications.map((entry) => entry.id) },
+          },
+        });
+      }
+
+      return notification;
+    });
+
+    const payload = this.toPayload(created);
+    emitNotificationToUser(input.userId, payload);
+    return payload;
+  }


@Notgoyome I fixed them all in #14, just have to make sure that I didn't break stuff with the front and I'll do the merge once this is integrated

alexis-belmonte · 2026-03-01T13:19:20Z

prisma/models/notification.prisma

+  userId    Int
+  title     String
+  message   String
+  type      String


I agree, this needs to be enum -- otherwise clients could send unknown notification types that just overload the system -- notification types should be checked when received

alexis-belmonte · 2026-03-01T13:22:50Z

src/notifications/notifications.service.ts

+export class NotificationsService {
+  constructor(private readonly prisma: PrismaService) {}
+
+  async getUserNotifications(userId: number): Promise<NotificationPayload[]> {


Since there is a MAX_NOTIFICATIONS_PER_USER, this is ok-ish if the notifications aren't that big in size

alexis-belmonte · 2026-03-01T13:29:41Z

src/notifications/notifications.controller.ts

+export class NotificationsController {
+  constructor(private readonly notificationsService: NotificationsService) {}
+
+  // TODO: Remove this endpoint after testing, or restrict it to admin users


alexis-belmonte · 2026-03-01T13:50:23Z

src/notifications/notifications.service.ts

+    createdAt: Date;
+  }): NotificationPayload {
+    return {
+      id: String(notification.id),


I think that doing String(...) over toString is clearer imho

…ocket for clarity

Notgoyome requested a review from Copilot February 28, 2026 03:18

Notgoyome self-assigned this Feb 28, 2026

Copilot started reviewing on behalf of Notgoyome February 28, 2026 03:19 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

Notgoyome requested a review from Copilot March 1, 2026 02:42

Naucto deleted a comment from Copilot AI Mar 1, 2026

Copilot started reviewing on behalf of Notgoyome March 1, 2026 02:43 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

Notgoyome force-pushed the NCTO-155-make-notifications-system branch from a6cf0cd to 4f4d5de Compare March 1, 2026 03:00

Naucto deleted a comment from Copilot AI Mar 1, 2026

Notgoyome force-pushed the NCTO-155-make-notifications-system branch from 8bc7715 to fe1db69 Compare March 1, 2026 05:35

Notgoyome added 8 commits March 1, 2026 14:40

[BACKEND] [FEATURE] Implement notifications system

b8b0f8d

[BACKEND] [FIX] Remove unused markAllAsRead endpoint from notificatio…

213357a

…ns controller

[BACKEND] [REFactor] Enhance WebSocket connection handling and authen…

02501dd

…tication logic

[FIX] Improve error handling in markAsRea

5f96e3d

[BACKEND] [FIX] Handle WebSocket upgrade

d7e7aa3

[BACKEND] [FIX] Enhance raw data handling in WebSocket authentication

722d570

[MAIN][FIX]: lint error

7eb415e

[BACKEND] [FIX] ws upgrade handler

d72ca5c

Notgoyome force-pushed the NCTO-155-make-notifications-system branch from fe1db69 to d72ca5c Compare March 1, 2026 05:41

[BACKEND] [FIX] Ensure notification retrieval respects maximum limit …

225b7a7

…per user

Notgoyome requested review from Priax and alexis-belmonte March 1, 2026 10:56

Priax requested changes Mar 1, 2026

View reviewed changes

alexis-belmonte requested changes Mar 1, 2026

View reviewed changes

Notgoyome added 3 commits March 6, 2026 19:29

[BACKEND] [REFACT] Rename setupWebSocketServer to setupCollaborationS…

353658c

…ocket for clarity

[BACKEND][FIX] Convert notification ID to string in payload

f4ba8da

[BACKEND][REFACTO] Change notification type from string to enum

7724251

alexis-belmonte mentioned this pull request Mar 7, 2026

NCTO-23 client server system for multiplayer #14

Merged

		let notificationWss: WebSocketServer \| undefined;
		const userSockets = new Map<number, Set<WebSocket>>();

	if (!requestUrl.pathname.startsWith(NOTIFICATION_SOCKET_PATH)) {
	if (!requestUrl.pathname.startsWith(NOTIFICATION_SOCKET_PATH)) {
	socket.destroy();

Conversation

Notgoyome commented Feb 28, 2026

Jira ticket

What does your MR do ?

How to test it

Screenshot

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexis-belmonte commented Mar 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!