Skip to content

Ncto 155 make notifications system#46

Open
Notgoyome wants to merge 21 commits intomainfrom
NCTO-155-make-notifications-system
Open

Ncto 155 make notifications system#46
Notgoyome wants to merge 21 commits intomainfrom
NCTO-155-make-notifications-system

Conversation

@Notgoyome
Copy link
Copy Markdown
Collaborator

@Notgoyome Notgoyome commented Feb 28, 2026

Jira ticket

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

What does your MR do ?

adding notification system

  • Use of websocket to send notifications if the user is connected.
  • on page load, get notifications

How to test it

go to swagger, there will be am api call notification test which can be used to notify itself (need backend branch)

Screenshot

Notes

I also added a docker-compose.dev.yml, let you me know if its good (this file is used to get access to hot reload)

@Notgoyome Notgoyome force-pushed the NCTO-155-make-notifications-system branch from 4085a40 to a4c93d2 Compare February 28, 2026 02:33
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a frontend notification system integrated into the navbar, backed by a new notifications API client and a WebSocket connection to receive/update notifications in real time.

Changes:

  • Introduces notification UI components (badge + menu) and NotificationItem typing.
  • Adds OpenAPI-generated notifications service/models and exports them from the API index.
  • Adds a dev Docker Compose setup for hot reload and updates README accordingly.

Reviewed changes

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

Show a summary per file
File Description
src/shared/navbar/notifications/types.ts Adds the NotificationItem type used across the notification UI.
src/shared/navbar/notifications/NotificationMenu.tsx Implements the menu container that renders notification items (or empty state).
src/shared/navbar/notifications/NotificationListItem.tsx Adds UI for a single notification entry and a “Mark as read” action.
src/shared/navbar/notifications/NotificationBox.tsx Connects to a notifications WebSocket, maintains notification state, renders badge/menu, and calls mark-as-read API.
src/shared/navbar/NavBar.tsx Inserts NotificationBox into the navbar.
src/shared/authOverlay/AuthOverlay.tsx Stops storing a refresh token on login/register (aligns with API DTO change).
src/assets/inbox.svg Adds an inbox icon asset.
src/api/services/NotificationsService.ts Adds generated API client for notifications endpoints.
src/api/models/NotificationTestDto.ts Adds generated DTO model for test notifications.
src/api/models/AuthResponseDto.ts Removes refresh_token from the auth response model.
src/api/index.ts Exports notifications types/services (and an additional model export).
src/api/core/OpenAPI.ts Changes default API base URL fallback to localhost.
package.json Removes eslint-config-airbnb-typescript dev dependency.
docker-compose.dev.yml Adds a dev-only compose setup using Bun + volume mounts for hot reload.
bun.lock Updates lockfile to reflect dependency changes.
README.md Documents the new dev docker compose command.

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

@@ -0,0 +1,15 @@
services:
frontend:
image: oven/bun:latest
Copy link

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using oven/bun:latest makes the dev environment non-reproducible (the image can change under you and break hot reload or dependency installs). Pin to a specific Bun tag/version (and optionally document the required Bun version) to keep dev setups stable.

Suggested change
image: oven/bun:latest
image: oven/bun:1.1.34

Copilot uses AI. Check for mistakes.
@Notgoyome Notgoyome self-assigned this Feb 28, 2026
@Notgoyome Notgoyome force-pushed the NCTO-155-make-notifications-system branch from f4a6b97 to df060f3 Compare February 28, 2026 03:36
@Notgoyome Notgoyome force-pushed the NCTO-155-make-notifications-system branch from 6786b18 to 6f7ade7 Compare February 28, 2026 04:06
@Notgoyome Notgoyome changed the title [DRAFT] Ncto 155 make notifications system Ncto 155 make notifications system Feb 28, 2026

export const OpenAPI: OpenAPIConfig = {
BASE: import.meta.env.VITE_BACKEND_URL || 'none',
BASE: (import.meta.env.VITE_BACKEND_URL || "none "),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

b9328ea i did something and forgot to revert completely mb

const [showMenu, setShowMenu] = useState(false);
const [anchorEl, setAnchorEl] = useState<HTMLElement | undefined>(undefined);
const socketRef = useRef<WebSocket | null>(null);
const backendUrl = (import.meta.env.VITE_BACKEND_URL || "http://localhost:3000").trim();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for you to get that info from OpenAPI.BASE?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try {
socket.send(JSON.stringify({ type: "auth", token }));
} catch {
// eslint-disable-next-line no-console
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that you remove this and let it just warn when linting -- maybe we could make a simple API to do logging so that we can use internally console.* or something else

But I don't really get it why it was decided to ban the use of console altogether though

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,15 @@
services:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ialso have a docker-compose.dev.yml file on my frontend branch, this will conflict -- I'll see if I either keep your version or mine

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

- "3001:3001"
environment:
- CHOKIDAR_USEPOLLING=true
command: sh -lc "bun install --frozen-lockfile && bun run dev --host 0.0.0.0 --port 3001"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this, please declare a rule in the package.json file instead of writing commands to do the work so that we can also run it outside of Docker

Copy link
Copy Markdown
Collaborator Author

@Notgoyome Notgoyome Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change depending of your choice 👍

),
);
NotificationsService.notificationsControllerMarkAsRead(notificationId).catch(() => {
// eslint-disable-next-line no-console
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto as previous comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator

@Louis-rollet Louis-rollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems all good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants