feat: Implement Notification Page + Utility Notification Formatter Function#126
Conversation
…icated page - Add notification type definitions and interfaces - Implement generateNotificationMessage() utility function for formatting - Create API endpoints for CRUD operations (GET, POST, mark as read) - Build dedicated /notifications page with unread indicators - Add NotificationBell component with unread count badge in header - Implement auto-refresh polling every 30 seconds - Add notification service layer with helper functions - Support 12+ notification types (follow, contest, quiz, achievement, etc.) - Include smooth animations and modern UI design - Add comprehensive documentation and integration examples Closes #<123>
Pratyush2240
requested review from
Jagath-P and
aviralsaxena16
as code owners
|
@Pratyush2240 is attempting to deploy a commit to the aviralsaxena16's projects Team on Vercel. A member of the Team first needs to authorize it. |
🎉 Thanks for Your Contribution to CanonForces!
|
WalkthroughAdds a full notification subsystem: typed models, message/link utilities, service APIs, Next.js API routes, Firestore rules/index guidance, React UI components (bell, item), pages for viewing/testing notifications, integration examples, and supporting docs and setup guides. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as NotificationBell / NotificationsPage
participant API as /api/v1/notifications
participant DB as Firestore
participant Auth as Firebase Auth
Note over User,DB: Producer flow (create notification)
User->>UI: perform action (follow/solve/quiz)
UI->>API: POST /api/v1/notifications (userId, type, metadata)
API->>DB: create notification doc (userId,type,message,read:false,createdAt)
DB-->>API: doc created (id)
API-->>UI: 201 Created
Note over UI,DB: Consumer flow (fetch & badge)
UI->>Auth: subscribe auth state
Auth-->>UI: userId
UI->>API: GET /api/v1/notifications?userId=XXX (polled or on load)
API->>DB: query notifications by userId ordered by createdAt
DB-->>API: list of notifications
API-->>UI: notifications payload
UI->>UI: render list and compute unread badge
Note over User,API: Mark as read
User->>UI: click notification or "Mark all as read"
UI->>API: POST /api/v1/notifications/read/{id} or /read-all (userId in body)
API->>DB: update notification(s) set read:true
DB-->>API: update confirmed
API-->>UI: 200 OK
UI->>UI: update local state, refresh badge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (15)
src/common/components/NotificationBell/NotificationBell.module.css (1)
37-66: Consider respectingprefers-reduced-motionfor the pulsing badgeThe infinite
pulseanimation on.badgeis fine visually but can be wrapped in aprefers-reduced-motionmedia query to be more accessible.Example:
.badge { position: absolute; top: 0; right: 0; background: #ef4444; color: white; font-size: 0.65rem; font-weight: 600; padding: 0.15rem 0.35rem; border-radius: 10px; min-width: 18px; height: 18px; display: flex; align-items: center; justify-content: center; border: 2px solid white; - animation: pulse 2s ease-in-out infinite; } +@media (prefers-reduced-motion: no-preference) { + .badge { + animation: pulse 2s ease-in-out infinite; + } +}NOTIFICATION_ARCHITECTURE.md (1)
5-226: Add languages to fenced code blocks for better tooling supportSeveral code fences (diagrams, flows, file tree) don’t specify a language, which triggers MD040 and reduces readability in some editors.
For example:
-``` +```text ┌─────────────────────────────────────────────────────────────────────┐ │ USER ACTIONS │ ...and for the file tree / command examples, use appropriate languages like
text,bash, ortypescript.src/common/components/Notifications/NotificationItem.tsx (2)
23-34: Consider memoizing time-ago calculation for performance.The
getTimeAgofunction recalculates on every render. For notification lists with many items, this could impact performance.If performance becomes a concern, consider using
useMemo:+const timeAgo = React.useMemo(() => { const now = Date.now(); const diff = now - notification.createdAt; const minutes = Math.floor(diff / 60000); const hours = Math.floor(diff / 3600000); const days = Math.floor(diff / 86400000); if (minutes < 1) return "Just now"; if (minutes < 60) return `${minutes}m ago`; if (hours < 24) return `${hours}h ago`; return `${days}d ago`; +}, [notification.createdAt]);Then use
{timeAgo}instead of{getTimeAgo(notification.createdAt)}on Line 74.
36-60: Consider movinggetNotificationIconoutside the component.This function doesn't depend on props or state, so it's recreated on every render unnecessarily.
Move it outside the component:
+const getNotificationIcon = (type: string) => { + switch (type) { + case "follow": + case "unfollow": + return "👤"; + // ... rest of cases + default: + return "🔔"; + } +}; + export default function NotificationItem({ notification, onMarkAsRead, }: NotificationItemProps) { - const getNotificationIcon = (type: string) => { - switch (type) { - // ... - } - };src/common/components/NotificationBell/NotificationBell.tsx (1)
37-51: Consider server-side filtering for unread count.The current implementation fetches all notifications (up to 50) and filters client-side. This sends unnecessary data over the network.
Add a query parameter to the API to return only unread count:
Option 1: Add count-only endpoint:
// New API route: /api/v1/notifications/unread-count const response = await fetch(`/api/v1/notifications/unread-count?userId=${userId}`); const data = await response.json(); setUnreadCount(data.count);Option 2: Add query parameter to existing endpoint:
const response = await fetch(`/api/v1/notifications?userId=${userId}&unreadOnly=true&countOnly=true`);This would reduce bandwidth, especially for users with many notifications.
src/pages/api/v1/notifications.ts (1)
72-80: Use FirestoreserverTimestamp()for consistency.Line 79 uses
Date.now(), which relies on the client/server system clock and can be inconsistent or manipulated.Use Firestore's
serverTimestamp()for reliable, server-side timestamps:+import { serverTimestamp } from "firebase/firestore"; const notificationData = { userId, type, message, metadata: metadata || {}, link, read: false, - createdAt: Date.now(), + createdAt: serverTimestamp(), };Note: If you need to return the timestamp immediately, you can use
Date.now()in the response while storingserverTimestamp():+const now = Date.now(); const notificationData = { userId, type, message, metadata: metadata || {}, link, read: false, - createdAt: Date.now(), + createdAt: serverTimestamp(), }; const docRef = await addDoc(collection(db, "notifications"), notificationData); return res.status(201).json({ success: true, - notification: { id: docRef.id, ...notificationData }, + notification: { id: docRef.id, ...notificationData, createdAt: now }, });src/utils/notifications.ts (1)
21-64: Consider stricter typing with NotificationType.The
typeparameter isstring, but could beNotificationTypefor better type safety. However, the function also needs to handle unknown types (line 61-62 default case).You could create a type guard and use a discriminated union for better type safety:
export function generateNotificationMessage(notification: NotificationPayload): string { const { type, metadata = {} } = notification; // Type guard for known types const knownTypes: NotificationType[] = ["follow", "contest_reminder", /* ... */]; if (!knownTypes.includes(type as NotificationType)) { return metadata.message || "You have a new notification"; } // Now TypeScript knows type is NotificationType switch (type as NotificationType) { case "follow": return `${metadata.username || "Someone"} started following you`; // ... rest of cases } }Alternatively, keep the current approach for flexibility if notification types can be extended dynamically.
src/pages/test-notifications.tsx (1)
32-51: Handle notification creation result more gracefully.The loading state is only shown while the request is in flight. Consider showing immediate feedback when the create button is clicked.
The current implementation is acceptable, but you could improve UX by disabling all buttons while any request is in flight:
<button onClick={() => handleCreateNotification(test.type, test.metadata)} disabled={loading} className="..." > {loading ? "Creating..." : "Create Notification"} </button>This already disables all buttons when
loadingis true, which is good.src/pages/notifications.tsx (3)
37-56: Avoid duplicating notifications fetch logic already provided by the service
fetchNotificationsis manually calling/api/v1/notificationsand shaping the response, whilesrc/services/notifications.tsalready exposesgetNotifications(userId)with similar behavior. To keep the API contract and error‑handling in one place, consider delegating to that helper here and only managingloading/errorstate in this component.For example:
import { getNotifications } from "../services/notifications"; const fetchNotifications = async () => { try { setLoading(true); setError(null); const list = await getNotifications(userId!); setNotifications(list); } catch (err) { console.error("Error fetching notifications:", err); setError("Failed to load notifications"); } finally { setLoading(false); } };
58-96: Reuse mark‑as‑read helpers from the notifications service for consistency
handleMarkAsReadandhandleMarkAllAsReadre‑implement the same API calls thatmarkNotificationAsReadandmarkAllNotificationsAsReadalready wrap insrc/services/notifications.ts. Centralizing these calls there reduces duplication and keeps paths/HTTP details in one place.Within this component you could keep the optimistic UI update but delegate the network work:
- const handleMarkAsRead = async (notificationId: string) => { - try { - const response = await fetch(`/api/v1/notifications/read/${notificationId}`, { - method: "POST", - }); - - if (response.ok) { - setNotifications((prev) => - prev.map((notif) => - notif.id === notificationId ? { ...notif, read: true } : notif - ) - ); - } - } catch (err) { - console.error("Error marking notification as read:", err); - } - }; + const handleMarkAsRead = async (notificationId: string) => { + const ok = await markNotificationAsRead(notificationId); + if (ok) { + setNotifications((prev) => + prev.map((notif) => + notif.id === notificationId ? { ...notif, read: true } : notif + ) + ); + } + }; @@ - const handleMarkAllAsRead = async () => { - if (!userId) return; - - try { - const response = await fetch("/api/v1/notifications/read-all", { - method: "POST", - headers: { - "Content-Type": "application/json", - }, - body: JSON.stringify({ userId }), - }); - - if (response.ok) { - setNotifications((prev) => - prev.map((notif) => ({ ...notif, read: true })) - ); - } - } catch (err) { - console.error("Error marking all notifications as read:", err); - } - }; + const handleMarkAllAsRead = async () => { + if (!userId) return; + + const ok = await markAllNotificationsAsRead(userId); + if (ok) { + setNotifications((prev) => + prev.map((notif) => ({ ...notif, read: true })) + ); + } + };And add the corresponding import at the top:
import { markNotificationAsRead, markAllNotificationsAsRead, } from "../services/notifications";
134-169: Optional: add ARIA roles/live regions for loading and error statesThe loading/error/empty states are visually clear; for screen‑reader users you can make them more accessible by adding roles/ARIA hints, e.g.:
role="status"andaria-live="polite"on the loading container.role="alert"on the error container.This is purely a UX/A11y enhancement; behavior is otherwise solid.
src/examples/notification-integration-examples.ts (2)
41-52: Align example helper stubs with real Firebase service shapes to avoid confusionThe commented
friend_solvedexample andcheckAndNotifyAchievementsuse a localgetUserByUserIdhelper that returns a singleuser, whereas the realgetUserByUserIdinsrc/services/firebase.tsreturns an array ofUserProfileobjects. That mismatch (and reusing the same function name) can be confusing when someone wires these examples into production code.Consider either:
- Importing and demonstrating use of the real helpers (e.g.,
const [user] = await getUserByUserId(userId);), or- Renaming these local stubs (e.g.,
exampleGetUserByUserId) and clarifying in comments that real implementations should come fromsrc/services/firebase.ts.Same applies for
getAllUsers/getAllActiveUsers: pointing readers to the actual service implementations will help keep examples and production behavior in sync.Also applies to: 105-129, 177-200
81-98: Be mindful of fan‑out and rate limits in reminder/POTD examples
sendContestRemindersandnotifyNewPOTDcurrently loop over all contests × all users or all active users, sending one notification per combination. That’s fine for illustrative code, but in production you’ll likely want batching, filtering (e.g., per‑user preferences), and some throttling/backoff to avoid hitting Firestore/API write limits.When you implement these for real, consider:
- Chunking users and using batched writes where possible.
- Respecting user notification settings.
- Adding logging/metrics around how many notifications are sent per run.
Also applies to: 161-171
src/services/notifications.ts (2)
12-40: Tightentype/metadatatyping usingNotificationPayload(orNotificationType)
createNotificationcurrently acceptstype: stringandmetadata?: Record<string, any>, even though you already haveNotificationPayload(and likely aNotificationTypeunion) defined. You can get better compile‑time safety by tying these parameters to your existing payload type:-export async function createNotification( - userId: string, - type: string, - metadata?: Record<string, any> -): Promise<Notification | null> { +export async function createNotification( + userId: string, + type: NotificationPayload["type"], + metadata?: NotificationPayload["metadata"] +): Promise<Notification | null> {(Adjust the property names if
NotificationPayloadis shaped differently.) This prevents callers from passing unsupported notification types or malformed metadata.
68-101: Optional: log non‑OK HTTP responses in mark‑as‑read helpers
markNotificationAsReadandmarkAllNotificationsAsReadreturnresponse.okbut only log when an exception is thrown (e.g., network failure). If the server responds with a 4xx/5xx, you silently getfalsewith no context.You could add lightweight logging for non‑OK responses, e.g.:
export async function markNotificationAsRead(notificationId: string): Promise<boolean> { try { const response = await fetch(`/api/v1/notifications/read/${notificationId}`, { method: "POST", }); - - return response.ok; + if (!response.ok) { + console.error("Failed to mark notification as read", { + status: response.status, + }); + return false; + } + return true; } catch (error) { console.error("Error marking notification as read:", error); return false; } }and similarly for
markAllNotificationsAsRead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
IMPLEMENTATION_SUMMARY.md(1 hunks)NOTIFICATION_ARCHITECTURE.md(1 hunks)NOTIFICATION_SYSTEM.md(1 hunks)SETUP_CHECKLIST.md(1 hunks)firestore-notifications-rules.txt(1 hunks)src/common/components/Header/Header.tsx(1 hunks)src/common/components/NotificationBell/NotificationBell.module.css(1 hunks)src/common/components/NotificationBell/NotificationBell.tsx(1 hunks)src/common/components/Notifications/NotificationItem.tsx(1 hunks)src/common/components/Notifications/Notifications.module.css(1 hunks)src/constants/routes.tsx(1 hunks)src/examples/notification-integration-examples.ts(1 hunks)src/pages/api/v1/notifications.ts(1 hunks)src/pages/api/v1/notifications/read-all.ts(1 hunks)src/pages/api/v1/notifications/read/[id].ts(1 hunks)src/pages/notifications.tsx(1 hunks)src/pages/test-notifications.tsx(1 hunks)src/services/notifications.ts(1 hunks)src/types/notification.ts(1 hunks)src/utils/notifications.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/common/components/Notifications/NotificationItem.tsx (1)
src/types/notification.ts (1)
Notification(8-17)
src/common/components/NotificationBell/NotificationBell.tsx (1)
src/lib/firebase.ts (1)
auth(21-21)
src/utils/notifications.ts (1)
src/types/notification.ts (1)
NotificationPayload(3-6)
src/pages/test-notifications.tsx (2)
src/lib/firebase.ts (1)
auth(21-21)src/services/notifications.ts (1)
createNotification(12-40)
src/pages/api/v1/notifications/read/[id].ts (3)
src/pages/api/v1/notifications.ts (1)
handler(27-95)src/pages/api/v1/notifications/read-all.ts (1)
handler(17-61)src/lib/firebase.ts (1)
db(21-21)
src/pages/api/v1/notifications/read-all.ts (3)
src/pages/api/v1/notifications.ts (1)
handler(27-95)src/pages/api/v1/notifications/read/[id].ts (1)
handler(11-36)src/lib/firebase.ts (1)
db(21-21)
src/pages/api/v1/notifications.ts (5)
src/pages/api/v1/notifications/read-all.ts (1)
handler(17-61)src/pages/api/v1/notifications/read/[id].ts (1)
handler(11-36)src/lib/firebase.ts (1)
db(21-21)src/types/notification.ts (1)
Notification(8-17)src/utils/notifications.ts (2)
generateNotificationMessage(21-64)getNotificationLink(72-101)
src/pages/notifications.tsx (3)
src/types/notification.ts (1)
Notification(8-17)src/lib/firebase.ts (1)
auth(21-21)src/common/components/Notifications/NotificationItem.tsx (1)
NotificationItem(13-86)
src/services/notifications.ts (1)
src/types/notification.ts (1)
Notification(8-17)
src/examples/notification-integration-examples.ts (2)
src/services/notifications.ts (6)
notifyUserFollowed(105-114)notifyProblemSolved(129-138)notifyQuizCompleted(140-153)notifyAchievement(155-162)notifyContestReminder(116-127)createNotification(12-40)src/services/firebase.ts (2)
getAllUsers(129-136)getUserByUserId(67-78)
src/common/components/Header/Header.tsx (1)
src/common/components/NotificationBell/NotificationBell.tsx (1)
NotificationBell(9-80)
🪛 LanguageTool
firestore-notifications-rules.txt
[style] ~43-~43: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... update their own notifications * - Only the 'read' field can be modified * ...
(ADVERB_REPETITION_PREMIUM)
NOTIFICATION_ARCHITECTURE.md
[style] ~248-~248: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1309 characters long)
Context: ...ard_rank | 📊 | "You've reached rank #42!" | --- Last Updated: December 9, ...
(EN_EXCESSIVE_EXCLAMATION)
SETUP_CHECKLIST.md
[grammar] ~64-~64: Ensure spelling is correct
Context: ...unning at http://localhost:3000 2.
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~276-~276: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...ts. --- Last Updated: December 9, 2025 Status: ✅ Implementation Complete...
(MISSING_COMMA_AFTER_YEAR)
🪛 markdownlint-cli2 (0.18.1)
NOTIFICATION_ARCHITECTURE.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
131-131: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
SETUP_CHECKLIST.md (1)
1-276: Checklist is clear and actionableThe setup checklist aligns well with the implemented notification system (rules, indexes, testing, integration points) and should make onboarding straightforward. Aside from the
allow createtightening already noted in the rules file, this doc looks good.src/constants/routes.tsx (1)
10-13: Notifications route constant is consistent and minimal
NOTIFICATIONS = "/notifications"fits the existing route naming convention and matches the new notifications page.src/common/components/Header/Header.tsx (1)
4-29: Header integration ofNotificationBelllooks cleanThe bell is added in a dedicated right-side actions column alongside Signup/Login, and the layout remains simple and readable. Given
NotificationBellrendersnullfor unauthenticated users, this should degrade gracefully.src/common/components/Notifications/Notifications.module.css (1)
3-108: Notification list styling covers key states and looks robustThe CSS cleanly handles unread state, hover feedback, icons, message/time text, and empty/loading/error views. It should support the NotificationItem and notifications page well.
src/pages/api/v1/notifications/read/[id].ts (1)
11-35: Clarify auth model and enforce ownership inread/:idAPIThis handler marks a notification as read based solely on the
idfrom the URL:const notificationRef = doc(db, "notifications", id); await updateDoc(notificationRef, { read: true });Without tying the request to the authenticated user and checking that the document's
userIdmatches, this endpoint can be abused ifdbhas elevated privileges (e.g., Admin/service credentials): any caller who can guess/obtain an ID can mark someone else's notifications as read.Consider:
- If
dbis an Admin/privileged instance:
- Extract the current user from your auth/session layer (e.g., NextAuth, Firebase ID token in headers).
- Read the doc and verify
doc.data().userId === currentUserIdbeforeupdateDoc, returning 403 otherwise.- If
dbis a client SDK relying on Firestore rules:
- Confirm that this API route actually runs with the caller's Firebase Auth context; otherwise, security rules will see
request.auth == nulland deny access.- Alternatively, move all Firestore access for this route to the Admin SDK and enforce ownership in the handler.
Also consider returning 404 (or a specific error) when the document doesn't exist rather than a generic 500, once you add a preceding
getDoccheck.firestore-notifications-rules.txt (1)
11-27: Tightenallow createrule to deny client-side creation in productionThe current rule
allow create: if request.auth != null;allows any authenticated user to write arbitrary notifications without validating ownership. This creates a security gap if clients communicate directly with Firestore.For production, deny client-side creation and route all writes through a trusted server (Admin SDK or Cloud Functions):
- allow create: if request.auth != null; + allow create: if false;If development environments require client writes, maintain a clearly-marked separate ruleset for local testing only.
src/pages/api/v1/notifications/read-all.ts (1)
32-37: Verify Firestore composite index exists.This query uses a compound filter (
userId== X ANDread== false) that requires a composite index in Firestore.Confirm that the Firestore composite index for the
notificationscollection has been created with fieldsuserIdandread. The index should be defined in your Firestore configuration or will be auto-created on first query (which may cause initial failures).Check your Firestore console under Indexes, or add to
firestore.indexes.json:{ "indexes": [ { "collectionGroup": "notifications", "queryScope": "COLLECTION", "fields": [ { "fieldPath": "userId", "order": "ASCENDING" }, { "fieldPath": "read", "order": "ASCENDING" } ] } ] }NOTIFICATION_SYSTEM.md (1)
1-313: Comprehensive documentation for the notification system.The documentation clearly covers all aspects of the notification system including types, utilities, API endpoints, integration examples, and troubleshooting. The examples are consistent with the implementation.
src/pages/api/v1/notifications.ts (1)
39-45: Verify Firestore composite index for query.This query combines
whereandorderBywhich requires a composite index in Firestore.Ensure a composite index exists for the
notificationscollection with fieldsuserId(==) andcreatedAt(desc). Add tofirestore.indexes.json:{ "indexes": [ { "collectionGroup": "notifications", "queryScope": "COLLECTION", "fields": [ { "fieldPath": "userId", "order": "ASCENDING" }, { "fieldPath": "createdAt", "order": "DESCENDING" } ] } ] }src/utils/notifications.ts (1)
21-101: Functions look good with comprehensive type coverage.Both
generateNotificationMessageandgetNotificationLinkhandle all notification types with sensible fallbacks for missing metadata. The default cases ensure the functions never return undefined.src/pages/notifications.tsx (1)
12-29: Auth gating and state wiring look goodUsing
onAuthStateChangedwith cleanup plus redirecting unauthenticated users to/loginis correct and idiomatic for a protected page; no issues here.src/examples/notification-integration-examples.ts (1)
18-53: Example handlers are clear and map well to the notification helpersThe follow, problem‑submission, and quiz‑completion examples read cleanly, and the conditions around verdict/score are sensible. They show good patterns for calling
notifyUserFollowed,notifyProblemSolved, andnotifyQuizCompletedwithout leaking API details.src/services/notifications.ts (1)
103-162: Helper wrappers for specific notification types look clean and DRYThe
notifyUserFollowed,notifyContestReminder,notifyProblemSolved,notifyQuizCompleted, andnotifyAchievementwrappers are straightforward and keep type‑specific details close to their creation sites. No issues here.IMPLEMENTATION_SUMMARY.md (1)
1-202: Documentation is thorough and matches the implemented surface areaThe summary does a good job enumerating types, utilities, services, endpoints, UI components, and example usage. It should give new contributors a clear mental model of how the notification system is wired end‑to‑end.
| ## 📊 Database Schema | ||
|
|
||
| Notifications are stored in Firestore under the `notifications` collection: | ||
|
|
||
| ```typescript | ||
| { | ||
| id: string; // Auto-generated | ||
| userId: string; // Recipient | ||
| type: string; // Notification type | ||
| message: string; // Formatted message | ||
| metadata: object; // Additional data | ||
| link: string; // Navigation link | ||
| read: boolean; // Read status | ||
| createdAt: number; // Timestamp | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Clarify optional metadata/link fields in schema to match the Notification type
In the Firestore schema snippet, metadata and link are shown as required:
metadata: object; // Additional data
link: string; // Navigation linkBut src/types/notification.ts defines them as optional (metadata?, link?). To avoid confusion for integrators, consider updating the snippet to mirror the type:
- metadata: object; // Additional data
- link: string; // Navigation link
+ metadata?: object; // Additional data (optional)
+ link?: string; // Navigation link (optional)This keeps docs and runtime types aligned.
🤖 Prompt for AI Agents
In IMPLEMENTATION_SUMMARY.md around lines 203 to 218, the Firestore schema shows
metadata and link as required fields but src/types/notification.ts defines them
as optional; update the schema snippet so metadata and link are marked optional
(or annotated as "optional"/nullable) to match the Notification type, ensuring
the documentation mirrors the runtime types and avoids confusion for
integrators.
| if (req.method === "GET") { | ||
| try { | ||
| const { userId } = req.query; | ||
|
|
||
| if (!userId || typeof userId !== "string") { | ||
| return res.status(400).json({ error: "userId is required" }); | ||
| } |
There was a problem hiding this comment.
Add authentication to protect user data.
The GET endpoint allows fetching notifications for any userId without authentication, exposing potentially private notification data.
Apply authentication similar to the read-all endpoint:
if (req.method === "GET") {
try {
+ // Verify authentication
+ const session = await getServerSession(req, res, authOptions);
+ if (!session?.user) {
+ return res.status(401).json({ error: "Unauthorized" });
+ }
+
const { userId } = req.query;
if (!userId || typeof userId !== "string") {
return res.status(400).json({ error: "userId is required" });
}
+
+ // Ensure user can only fetch their own notifications
+ if (userId !== session.user.id) {
+ return res.status(403).json({ error: "Forbidden" });
+ }🤖 Prompt for AI Agents
In src/pages/api/v1/notifications.ts around lines 31 to 37, the GET branch
currently accepts any userId query param without auth; update it to require and
validate an authenticated session like the read-all endpoint: fetch the
session/user from the request (e.g., via your auth helper), return 401 if
missing, then ensure the session user id matches the requested userId (or
session has admin scope) and return 403 if it doesn’t; only after those checks
proceed to read notifications and return 400/401/403 as appropriate.
| if (req.method === "POST") { | ||
| try { | ||
| const { userId, type, metadata } = req.body; | ||
|
|
||
| if (!userId || !type) { | ||
| return res.status(400).json({ error: "userId and type are required" }); | ||
| } |
There was a problem hiding this comment.
Add authentication to POST endpoint.
Any client can create notifications for any user without authentication.
Add authentication check:
if (req.method === "POST") {
try {
+ // Verify authentication - system/admin only
+ const session = await getServerSession(req, res, authOptions);
+ if (!session?.user?.isAdmin) {
+ return res.status(401).json({ error: "Unauthorized" });
+ }
+
const { userId, type, metadata } = req.body;
if (!userId || !type) {
return res.status(400).json({ error: "userId and type are required" });
}Note: The POST endpoint might need system/admin-level auth, or should be called from server-side code only, not directly from client.
Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| const { userId } = req.body; | ||
|
|
||
| if (!userId) { | ||
| return res.status(400).json({ error: "userId is required" }); | ||
| } |
There was a problem hiding this comment.
Add authentication and type validation.
The endpoint accepts userId from the request body without authentication, allowing any client to mark any user's notifications as read. Additionally, userId isn't validated as a string.
Apply this diff:
+ // Verify authentication
+ const session = await getServerSession(req, res, authOptions);
+ if (!session?.user) {
+ return res.status(401).json({ error: "Unauthorized" });
+ }
+
const { userId } = req.body;
- if (!userId) {
+ if (!userId || typeof userId !== "string") {
return res.status(400).json({ error: "userId is required" });
}
+
+ // Ensure user can only modify their own notifications
+ if (userId !== session.user.id) {
+ return res.status(403).json({ error: "Forbidden" });
+ }Note: Adjust the authentication check to match your project's auth pattern.
Committable suggestion skipped: line range outside the PR's diff.
…icated page - Add notification type definitions and interfaces - Implement generateNotificationMessage() utility function for formatting - Create API endpoints for CRUD operations (GET, POST, mark as read) - Build dedicated /notifications page with unread indicators - Add NotificationBell component with unread count badge in header - Implement auto-refresh polling every 30 seconds - Add notification service layer with helper functions - Support 12+ notification types (follow, contest, quiz, achievement, etc.) - Include smooth animations and modern UI design - Add comprehensive documentation and integration examples - Fix useEffect dependency warning with useCallback - Add security comments for authentication model Closes #<ISSUE_NUMBER>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/common/components/NotificationBell/NotificationBell.tsx (2)
34-34: Replaceanywith proper Notification type.Using
anydefeats TypeScript's type safety. Define or import aNotificationinterface with at least areadproperty.Example interface (add near the top of the file or import from a types file):
interface Notification { id: string; read: boolean; // ... other properties }Then update the filter:
- const unread = data.notifications.filter((n: any) => !n.read).length; + const unread = data.notifications.filter((n: Notification) => !n.read).length;
29-36: Consider handling non-OK responses explicitly.When
response.okis false, the error is silently ignored. Consider logging the error status or showing a user-facing indicator when fetching fails.if (response.ok && data.notifications) { const unread = data.notifications.filter((n: any) => !n.read).length; setUnreadCount(unread); + } else { + console.error("Failed to fetch notifications:", response.status, data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CODERABBIT_FIXES.md(1 hunks)src/common/components/NotificationBell/NotificationBell.tsx(1 hunks)src/pages/api/v1/notifications.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CODERABBIT_FIXES.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/api/v1/notifications.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/components/NotificationBell/NotificationBell.tsx (1)
src/lib/firebase.ts (1)
auth(21-21)
🔇 Additional comments (5)
src/common/components/NotificationBell/NotificationBell.tsx (5)
3-11: LGTM!The imports and component setup are clean and appropriate for the notification bell functionality.
13-24: LGTM!The authentication state listener is properly implemented with correct cleanup. Resetting the unread count on sign-out is a good UX decision.
26-40: Good fix for the dependency issue from the previous review.The
useCallbackwrapper with theuserIddependency correctly addresses the stale closure concern raised in the previous review.
42-51: LGTM!The polling effect correctly includes
fetchUnreadCountin its dependency array, ensuring the interval always uses the latest function reference. The 30-second polling interval is reasonable for notification updates.
30-30: Verify that the backend validates userId against the authenticated session.Passing
userIdas a query parameter is acceptable only if the API endpoint validates that the requesting user's authentication token matches the requesteduserId. Otherwise, any authenticated user could query another user's notifications. This requires examination of the backend implementation for the/api/v1/notificationsendpoint to confirm proper authorization checks are in place.
- Add missing notification types to NotificationType union (unfollow, contest_started, contest_ended, new_potd, friend_solved, leaderboard_rank) - Add aria-label to notification bell for screen reader accessibility - Add userId validation to mark-as-read endpoint - Add production environment check to test-notifications page - Fix useEffect dependency warning with useCallback in NotificationBell - Add security comments explaining Firestore rules authentication model - Update notifications page to include userId in mark-as-read requests Addresses all CodeRabbit suggestions for type safety, accessibility, and security
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/api/v1/notifications/read/[id].ts (1)
19-29: Clarify the role ofuserIdin this handler (validate vs. actually use it).You currently require and validate
userIdfrom the body but never use it in the update (security is delegated entirely to Firestore rules). That makes the API contract a bit confusing and forces clients to send a field that has no server-side effect.Consider either:
- Removing
userIdfrom the request body and its validation here (and updating docs/clients to match), if Firestore auth rules are the sole source of truth, or- Using
userIdfor a second-layer check (e.g., fetch the doc and assertdoc.userId === userIdbefore updating) if you want explicit server-side enforcement and clearer errors.Right now the extra parameter is just noise and can lead to misunderstandings about what is actually being authorized.
Also applies to: 34-37
src/types/notification.ts (1)
3-17: UseNotificationTypefor thetypefields to get real type safety.You’ve defined a good
NotificationTypeunion, butNotificationPayload.typeandNotification.typeare still plainstring, so callers don’t actually benefit from the union.Consider tightening the interfaces like this:
-export interface NotificationPayload { - type: string; - metadata?: Record<string, any>; -} +export interface NotificationPayload { + type: NotificationType; + metadata?: Record<string, any>; +} -export interface Notification { +export interface Notification { id: string; userId: string; - type: string; + type: NotificationType; message: string; read: boolean; createdAt: number; metadata?: Record<string, any>; link?: string; }This will surface mistakes at compile time if a caller passes an unsupported notification
type, keeping usages consistent with the rest of the notification system.Also applies to: 19-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/common/components/NotificationBell/NotificationBell.tsx(1 hunks)src/pages/api/v1/notifications/read/[id].ts(1 hunks)src/pages/notifications.tsx(1 hunks)src/pages/test-notifications.tsx(1 hunks)src/types/notification.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/common/components/NotificationBell/NotificationBell.tsx
- src/pages/notifications.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/pages/test-notifications.tsx (2)
src/lib/firebase.ts (1)
auth(21-21)src/services/notifications.ts (1)
createNotification(12-40)
src/pages/api/v1/notifications/read/[id].ts (1)
src/lib/firebase.ts (1)
db(21-21)
🔇 Additional comments (1)
src/pages/test-notifications.tsx (1)
21-37: Dev-only test page behavior looks solid; production guard now implemented.The
NODE_ENV === "production"early redirect inuseEffectcombined with the auth check makes this page effectively dev-only while still protecting it behind login in non-prod. The notification creation flow (handleCreateNotification+testNotificationspresets) is straightforward and keeps state transitions (loading,result) clear.No functional issues from this snippet; this is appropriate for an internal testing surface.
Also applies to: 39-58, 60-111
aviralsaxena16
left a comment
aviralsaxena16
left a comment
There was a problem hiding this comment.
@Pratyush2240 I have reviewed your PR and its great, am merging it , Thanks for your contribution 😇
|
@aviralsaxena16 Thank you for the review and merge. Glad to contribute, and looking forward to working on more improvements. |
2 participants
name: "📦 Pull Request"
about: Submit changes for review
title: "PR: Implement Notification Page + Utility Notification Formatter Function"
labels: "enhancement, feature"
assignees: ""
📌 Related Issue
Closes #123
✨ Changes Introduced
/notificationspage/test-notificationsfor manual testing/notificationsroute🤔 Why This Change?
🧪 Testing
npm test) - No new test failuresnpm run test:integration) - N/A/test-notifications→ Click "Follow Notification" button/notificationspage ✅/notifications→ Click on unread notification/notificationswith multiple unread → Click "Mark all as read"📝 Documentation Updates
✅ Checklist
feat/notification-system)👥 Reviewers
🚀 Deployment Notes
Post-Merge Setup Required:
Firestore Security Rules (Required):
Firestore Indexes (Auto-created):
userId + createdAtanduserId + read + createdAtIntegration (Optional):
Testing:
/test-notificationsto create test notificationsDetailed instructions in SETUP_CHECKLIST.md
💬 Community & Support
Need help or want to discuss this PR?
Join our Discord community 👉 CanonForces Discord
💡 Additional Notes (If any)
Files Created (18 new files):
Core System:
API Endpoints:
UI Components:
Pages:
Documentation:
Modified Files:
Supported Notification Types:
follow/unfollow- User follow notificationsproblem_solved- Problem completionquiz_completed- Quiz completioncontest_reminder/contest_started/contest_ended- Contest updatesachievement- Achievement unlockednew_potd- New Problem of the Dayfriend_solved- Friend activityleaderboard_rank- Rank updatessystem- General system notificationsQuick Integration Example: