feat: add in-app notification system (#98)#99
Conversation
- Notification types, Firestore service with real-time listeners - NotificationProvider context with subscribe/mark-read/preferences - NotificationBell dropdown with unread badge in dashboard top bar - NotificationCenter page at /dashboard/notifications with filters, search, pagination - NotificationPreferences page at /dashboard/notification-preferences - Notification trigger utilities for project, payment, account, and admin events - Routes and sidebar navigation integration Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughAdds a complete in-app notification system to the dashboard. New Firestore-backed service functions handle real-time subscription, pagination, read mutations, and preference persistence. A React context provider exposes notifications and actions to the UI. Three new UI components are added: ChangesIn-App Notification System
Sequence Diagram(s)sequenceDiagram
participant User
participant NotificationBell
participant NotificationProvider
participant notificationService
participant Firestore
User->>NotificationBell: Click bell icon
NotificationBell->>NotificationProvider: reads useNotifications() context
NotificationProvider-->>NotificationBell: notifications[], unreadCount
User->>NotificationBell: Click notification item
NotificationBell->>NotificationProvider: markAsRead(notificationId)
NotificationProvider->>notificationService: markAsRead(id)
notificationService->>Firestore: updateDoc(isRead: true)
User->>NotificationBell: Click "Mark all read"
NotificationBell->>NotificationProvider: markAllAsRead()
NotificationProvider->>notificationService: markAllAsRead(userId)
notificationService->>Firestore: batch.update all isRead=false docs
Note over notificationService,Firestore: Real-time subscription
Firestore-->>notificationService: onSnapshot update
notificationService-->>NotificationProvider: Notification[]
NotificationProvider-->>NotificationBell: updated unreadCount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Visit the preview URL for this PR (updated for commit 7713e02): https://servio-0--pr99-devin-1782064598-not-utrqi3a8.web.app (expires Sun, 28 Jun 2026 18:05:27 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 15915abb5951eb298a844eda460b24f444d93a69 |
|
add a welcome message notification for all new users when they signup |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/dashboard/notifications/NotificationContextObject.ts (1)
15-23: ⚡ Quick winFail fast when
NotificationProvideris missing.The default no-op context value hides wiring bugs; consumers outside the provider will “work” but silently discard actions. Prefer a nullable context and throw in the hook.
♻️ Proposed refactor
- export const NotificationContext = createContext<NotificationContextValue>({ - notifications: [], - unreadCount: 0, - loading: true, - preferences: DEFAULT_PREFERENCES, - markAsRead: async () => {}, - markAllAsRead: async () => {}, - updatePreferences: async () => {}, -}); +export const NotificationContext = + createContext<NotificationContextValue | null>(null);// src/dashboard/notifications/useNotifications.ts export function useNotifications() { - return useContext(NotificationContext); + const context = useContext(NotificationContext); + if (!context) { + throw new Error("useNotifications must be used within NotificationProvider"); + } + return context; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/dashboard/notifications/NotificationContextObject.ts` around lines 15 - 23, The NotificationContext default value currently provides no-op implementations that silently ignore actions when the context is used outside of NotificationProvider, making bugs hard to detect. Change the NotificationContext default value to null instead of providing default implementations. Then, in the custom hook that consumes this context (typically a useNotification or similar hook), add a null check that throws a descriptive error message if the hook is called when NotificationProvider is not present in the component tree, ensuring developers get immediate feedback when the context is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/dashboard/notifications/NotificationCenter.tsx`:
- Around line 93-133: The Card component in NotificationCenter currently relies
on an onClick handler which is not keyboard-accessible. Implement the stretched
link pattern by: setting position: relative on the Card, converting the
notification.title element into an interactive element (link or button), adding
a CSS ::after pseudo-element with position: absolute; inset: 0 to make the
entire card surface clickable, and positioning the secondary interactive
elements (the icon from typeIcon, the Badge with categoryBadgeClass, and the
timestamp) with position: relative and appropriate z-index values so they remain
interactive and layered above the stretched link. Remove the onClick handler
from the Card element.
In `@src/dashboard/notifications/NotificationContext.tsx`:
- Around line 27-31: In the NotificationContext.tsx file, the conditional block
checking if currentUser is falsy is resetting notifications and loading state
but not resetting preferences, which leaves stale user data exposed. Add a call
to setPreferences with DEFAULT_PREFERENCES in the same conditional block where
currentUser is null, alongside the existing setNotifications and setLoading
calls, to ensure preferences are cleared when the user logs out or switches.
In `@src/dashboard/notifications/NotificationPreferences.tsx`:
- Around line 58-63: The localPrefs state is initialized once from the
preferences prop but never syncs when preferences changes asynchronously from
Firestore, causing stale data. Add a useEffect hook that watches the preferences
dependency and calls setLocalPrefs with the updated preferences value whenever
the preferences prop changes, ensuring the local state stays in sync with the
remote data loaded from Firestore.
- Around line 65-73: The handleSave function currently sets setSaved(true)
immediately after awaiting updatePreferences, but since updatePreferences in
NotificationContext.tsx catches and logs errors internally without throwing, the
success state displays even when the update fails. Modify the updatePreferences
function to return a success/failure indicator (such as a boolean or result
object) instead of silently catching errors, then update the handleSave function
to only call setSaved(true) when updatePreferences actually returns a successful
result. Alternatively, rethrow the error in updatePreferences so handleSave can
catch it in an additional catch block to handle failures appropriately.
In `@src/dashboard/notifications/notificationService.ts`:
- Around line 108-117: The `markAsRead()` function lacks user validation,
allowing any user to update any notification by ID, and `markAllAsRead()`
accepts userId as a client parameter without verifying it matches the
authenticated user. Modify the `markAsRead()` signature to accept a userId
parameter alongside notificationId, then add validation logic before the
updateDoc call to verify that the notification's userId matches the provided
userId parameter. For `markAllAsRead()`, ensure the userId parameter is
validated against the authenticated user's ID before executing the query.
Additionally, define explicit Firestore security rules for the
NOTIFICATIONS_COLLECTION that enforce request.auth.uid ==
request.resource.data.userId for all read and write operations to prevent
unauthorized access at the database level.
- Around line 122-127: The markAllAsRead function processes all unread
notifications in a single writeBatch, which fails when the count exceeds
Firestore's 500 operation limit. Modify the function to chunk the snapshot.docs
array into groups of 500 or fewer, creating a new writeBatch for each chunk and
committing each batch separately. This ensures that regardless of the number of
unread notifications, they will all be marked as read without exceeding
Firestore's batch operation constraints.
In `@src/dashboard/notifications/notificationTriggers.ts`:
- Around line 13-15: The trigger function is creating notifications directly
without checking the user's notification preferences, making preference settings
ineffective. Before calling createNotification within the trigger function,
fetch the user's notification preferences based on the user ID and notification
category/channel from the params object, then only proceed with the
createNotification call if the user's preferences allow notifications for that
specific category and channel combination. This will ensure that disabled
notification preferences are properly respected during notification generation.
- Around line 13-18: The trigger function is catching errors from
createNotification and logging them without re-throwing, which prevents callers
from detecting failures or implementing retry logic. Remove the try-catch block
around the createNotification call in the trigger function to allow errors to
propagate up to callers, or alternatively keep the catch block but re-throw the
error after logging so that the failure is not silently swallowed.
---
Nitpick comments:
In `@src/dashboard/notifications/NotificationContextObject.ts`:
- Around line 15-23: The NotificationContext default value currently provides
no-op implementations that silently ignore actions when the context is used
outside of NotificationProvider, making bugs hard to detect. Change the
NotificationContext default value to null instead of providing default
implementations. Then, in the custom hook that consumes this context (typically
a useNotification or similar hook), add a null check that throws a descriptive
error message if the hook is called when NotificationProvider is not present in
the component tree, ensuring developers get immediate feedback when the context
is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 102aa66f-1ba5-4896-ba16-4f83997d24dd
📒 Files selected for processing (12)
src/app/App.tsxsrc/dashboard/components/DashboardLayout.tsxsrc/dashboard/notifications/NotificationBell.tsxsrc/dashboard/notifications/NotificationCenter.tsxsrc/dashboard/notifications/NotificationContext.tsxsrc/dashboard/notifications/NotificationContextObject.tssrc/dashboard/notifications/NotificationPreferences.tsxsrc/dashboard/notifications/index.tssrc/dashboard/notifications/notificationService.tssrc/dashboard/notifications/notificationTriggers.tssrc/dashboard/notifications/types.tssrc/dashboard/notifications/useNotifications.ts
| <Card | ||
| className={`border-gray-200 dark:border-slate-800 bg-white dark:bg-slate-900 cursor-pointer hover:shadow-md transition-shadow ${ | ||
| !notification.isRead ? "ring-1 ring-indigo-200 dark:ring-indigo-800" : "" | ||
| }`} | ||
| onClick={handleClick} | ||
| > | ||
| <CardContent className="flex items-start gap-4 py-4"> | ||
| <div className="mt-0.5 shrink-0">{typeIcon(notification.type)}</div> | ||
| <div className="flex-1 min-w-0"> | ||
| <div className="flex items-center gap-2 flex-wrap"> | ||
| <h3 | ||
| className={`text-sm ${ | ||
| !notification.isRead | ||
| ? "font-semibold text-gray-900 dark:text-gray-100" | ||
| : "font-medium text-gray-700 dark:text-gray-300" | ||
| }`} | ||
| > | ||
| {notification.title} | ||
| </h3> | ||
| <Badge className={categoryBadgeClass(notification.category)}> | ||
| {categoryLabel(notification.category)} | ||
| </Badge> | ||
| {!notification.isRead && ( | ||
| <span className="h-2 w-2 rounded-full bg-indigo-500 shrink-0" /> | ||
| )} | ||
| </div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400 mt-1"> | ||
| {notification.message} | ||
| </p> | ||
| <p className="text-xs text-gray-400 dark:text-gray-500 mt-1.5"> | ||
| {formatDistanceToNow(notification.createdAt, { | ||
| addSuffix: true, | ||
| })} | ||
| </p> | ||
| </div> | ||
| {notification.actionUrl && ( | ||
| <ExternalLink className="h-4 w-4 text-gray-400 shrink-0 mt-1" /> | ||
| )} | ||
| </CardContent> | ||
| </Card> | ||
| </motion.div> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
keyboard accessibility for React Card components button wrapper
💡 Result:
To make a React Card component accessible while allowing the entire surface area to be clickable, you should avoid wrapping the entire card in a single anchor or button tag [1][2][3]. Doing so forces screen readers to concatenate all the content inside the card, resulting in a confusing and non-descriptive accessible name [1][2]. The recommended industry approach is to use a CSS "stretched link" technique [1][3]. Recommended Implementation Strategy: 1. Use a standard interactive element (an tag if the card navigates, or a if it triggers an action) for the card's primary action [4][1]. 2. Place this link or button inside the card, often associated with the card's title [1][2]. 3. Apply a CSS::after pseudo-element to the primary link/button that covers the entire card area [1][2]. - Set the pseudo-element to position: absolute, inset: 0 (or top, right, bottom, left: 0) [1]. - Ensure the card container itself is set to position: relative to serve as the reference for the absolute positioning [1]. 4. This ensures the entire card is clickable for mouse users while screen readers receive a concise, meaningful label derived from the actual link or button text [1][2]. Handling Nested Interactive Elements: If your card contains secondary buttons or links (e.g., "Share", "Delete"), you must ensure they remain independently interactive [1][2]. - Position these secondary elements above the stretched link/button layer using CSS [1]. - Apply position: relative and z-index (e.g., z-index: 1) to the secondary elements so they sit visually and functionally "on top" of the stretched link area [1]. Best Practices: - Semantic Grouping: If displaying multiple cards, wrap them in an unordered list (
- ) with each card as a list item (
- ) to provide screen reader users with context about the list's structure and total count [4][3]. - Focus Indicators: Ensure the primary link/button has a clearly visible CSS:focus-visible indicator [4][3]. - ARIA Attributes: If the card's purpose is not clear from the main link text, use aria-label on the interactive element to provide a descriptive name [1][5]. Avoid adding excessive ARIA landmarks unless necessary [6]. - Avoid "Div" Buttons: Do not build cards by adding click handlers to generic or elements [4]. Even with role="button" and tabindex="0", you must manually manage keyboard event listeners (Enter/Space) and focus styles, which is prone to errors [4][7]. Using standard native elements is inherently more accessible [4]. Top Results: [4], [1], [2], [3]
Citations:
- 1: https://a11y-examples.com/examples/cards/
- 2: https://design-system.w3.org/components/cards.html
- 3: https://govtnz.github.io/web-a11y-guidance/wct/cards/
- 4: https://livefront.com/writing/accessibility-dos-and-donts-for-interactive-cards/
- 5: https://www.patternfly.org/components/card/accessibility/
- 6: https://www.w3.org/WAI/ARIA/apg/patterns/disclosure/examples/disclosure-card/
- 7: https://wai-aria-practices.netlify.app/aria-practices/
Make notification rows keyboard-accessible using a stretched link pattern, not a button wrapper.
The row is currently click-only (Card with onClick), preventing keyboard access. However, wrapping the entire Card in a button creates poor semantics—screen readers will concatenate all content as the button label, resulting in a confusing accessible name.
Use the industry-standard stretched link pattern instead:
- Place an interactive element (link or button) for the primary action inside the card, typically on the title
- Use a CSS
::afterpseudo-element on that element to cover the entire card surface withposition: absolute; inset: 0 - Set
position: relativeon the card container - Position any secondary interactive elements (if present) above the stretched link using
position: relativeand appropriatez-index
This preserves semantic structure while making the entire surface clickable for mouse users and keyboard-navigable for assistive technology users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/NotificationCenter.tsx` around lines 93 - 133,
The Card component in NotificationCenter currently relies on an onClick handler
which is not keyboard-accessible. Implement the stretched link pattern by:
setting position: relative on the Card, converting the notification.title
element into an interactive element (link or button), adding a CSS ::after
pseudo-element with position: absolute; inset: 0 to make the entire card surface
clickable, and positioning the secondary interactive elements (the icon from
typeIcon, the Badge with categoryBadgeClass, and the timestamp) with position:
relative and appropriate z-index values so they remain interactive and layered
above the stretched link. Remove the onClick handler from the Card element.
| if (!currentUser) { | ||
| setNotifications([]); | ||
| setLoading(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat src/dashboard/notifications/NotificationContext.tsxRepository: hrx01-dev/Servio
Length of output: 2656
Reset preferences on logout/user switch to prevent stale cross-user state.
When currentUser becomes null, preferences retains the previous user's value and remains exposed via the NotificationContext until the next user logs in. This causes a brief window where one user's notification preferences are visible to another session.
Reset to DEFAULT_PREFERENCES when currentUser is null (and before subscribing to new user data) to eliminate this cross-session data leakage.
🐛 Proposed fix
useEffect(() => {
if (!currentUser) {
setNotifications([]);
+ setPreferences(DEFAULT_PREFERENCES);
setLoading(false);
return;
}
setLoading(true);
+ setPreferences(DEFAULT_PREFERENCES);
const unsubNotifications = subscribeToNotifications(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!currentUser) { | |
| setNotifications([]); | |
| setLoading(false); | |
| return; | |
| } | |
| if (!currentUser) { | |
| setNotifications([]); | |
| setPreferences(DEFAULT_PREFERENCES); | |
| setLoading(false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/NotificationContext.tsx` around lines 27 - 31, In
the NotificationContext.tsx file, the conditional block checking if currentUser
is falsy is resetting notifications and loading state but not resetting
preferences, which leaves stale user data exposed. Add a call to setPreferences
with DEFAULT_PREFERENCES in the same conditional block where currentUser is
null, alongside the existing setNotifications and setLoading calls, to ensure
preferences are cleared when the user logs out or switches.
| const [localPrefs, setLocalPrefs] = useState<PrefsType>(preferences); | ||
| const [saving, setSaving] = useState(false); | ||
| const [saved, setSaved] = useState(false); | ||
|
|
||
| const hasChanges = | ||
| JSON.stringify(localPrefs) !== JSON.stringify(preferences); |
There was a problem hiding this comment.
Sync localPrefs when remote preferences change.
localPrefs on Line 58 is initialized once, but preferences is loaded asynchronously from Firestore. This can leave stale toggle state and mark the form as changed without user edits.
Proposed fix
-import { useState } from "react";
+import { useEffect, useState } from "react";
...
const [localPrefs, setLocalPrefs] = useState<PrefsType>(preferences);
+ const [isDirty, setIsDirty] = useState(false);
const [saving, setSaving] = useState(false);
const [saved, setSaved] = useState(false);
+ useEffect(() => {
+ if (!isDirty && !saving) {
+ setLocalPrefs(preferences);
+ }
+ }, [preferences, isDirty, saving]);
+
const hasChanges =
JSON.stringify(localPrefs) !== JSON.stringify(preferences);
...
const updateCategory = (
key: keyof PrefsType["categories"],
value: boolean,
) => {
+ setIsDirty(true);
setLocalPrefs((prev) => ({
...prev,
categories: { ...prev.categories, [key]: value },
}));
};
...
const updateChannel = (
key: keyof PrefsType["channels"],
value: boolean,
) => {
+ setIsDirty(true);
setLocalPrefs((prev) => ({
...prev,
channels: { ...prev.channels, [key]: value },
}));
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [localPrefs, setLocalPrefs] = useState<PrefsType>(preferences); | |
| const [saving, setSaving] = useState(false); | |
| const [saved, setSaved] = useState(false); | |
| const hasChanges = | |
| JSON.stringify(localPrefs) !== JSON.stringify(preferences); | |
| const [localPrefs, setLocalPrefs] = useState<PrefsType>(preferences); | |
| const [isDirty, setIsDirty] = useState(false); | |
| const [saving, setSaving] = useState(false); | |
| const [saved, setSaved] = useState(false); | |
| useEffect(() => { | |
| if (!isDirty && !saving) { | |
| setLocalPrefs(preferences); | |
| } | |
| }, [preferences, isDirty, saving]); | |
| const hasChanges = | |
| JSON.stringify(localPrefs) !== JSON.stringify(preferences); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/NotificationPreferences.tsx` around lines 58 -
63, The localPrefs state is initialized once from the preferences prop but never
syncs when preferences changes asynchronously from Firestore, causing stale
data. Add a useEffect hook that watches the preferences dependency and calls
setLocalPrefs with the updated preferences value whenever the preferences prop
changes, ensuring the local state stays in sync with the remote data loaded from
Firestore.
| const handleSave = async () => { | ||
| setSaving(true); | ||
| try { | ||
| await updatePreferences(localPrefs); | ||
| setSaved(true); | ||
| setTimeout(() => setSaved(false), 2000); | ||
| } finally { | ||
| setSaving(false); | ||
| } |
There was a problem hiding this comment.
Don’t show “Saved!” unless persistence actually succeeds.
Lines 68-70 assume success after await updatePreferences(localPrefs), but updatePreferences in src/dashboard/notifications/NotificationContext.tsx currently catches/logs errors and resolves. This can show a false success state.
Proposed fix
// src/dashboard/notifications/NotificationContext.tsx
const updatePreferences = useCallback(
async (prefs: NotificationPreferences) => {
if (!currentUser) return;
try {
await updatePreferencesService(currentUser.uid, prefs);
} catch (error) {
console.error("Failed to update preferences:", error);
+ throw error;
}
},
[currentUser],
);// src/dashboard/notifications/NotificationPreferences.tsx
const handleSave = async () => {
setSaving(true);
try {
await updatePreferences(localPrefs);
setSaved(true);
setTimeout(() => setSaved(false), 2000);
+ setIsDirty(false);
+ } catch {
+ setSaved(false);
} finally {
setSaving(false);
}
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/NotificationPreferences.tsx` around lines 65 -
73, The handleSave function currently sets setSaved(true) immediately after
awaiting updatePreferences, but since updatePreferences in
NotificationContext.tsx catches and logs errors internally without throwing, the
success state displays even when the update fails. Modify the updatePreferences
function to return a success/failure indicator (such as a boolean or result
object) instead of silently catching errors, then update the handleSave function
to only call setSaved(true) when updatePreferences actually returns a successful
result. Alternatively, rethrow the error in updatePreferences so handleSave can
catch it in an additional catch block to handle failures appropriately.
| export async function markAsRead(notificationId: string): Promise<void> { | ||
| const ref = doc(db, NOTIFICATIONS_COLLECTION, notificationId); | ||
| await updateDoc(ref, { isRead: true }); | ||
| } | ||
|
|
||
| export async function markAllAsRead(userId: string): Promise<void> { | ||
| const q = query( | ||
| collection(db, NOTIFICATIONS_COLLECTION), | ||
| where("userId", "==", userId), | ||
| where("isRead", "==", false), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating Firestore rules/config files..."
fd -i 'firestore.rules|firebase.json|firestore.indexes.json'
RULE_FILE="$(fd -i 'firestore.rules' | head -n1 || true)"
if [ -z "${RULE_FILE}" ]; then
echo "No firestore.rules file found in repository root scan."
exit 0
fi
echo "Inspecting notification-related rule blocks in ${RULE_FILE}..."
rg -n -C4 'notifications|notificationPreferences|allow\s+(read|write|create|update|delete)' "${RULE_FILE}"Repository: hrx01-dev/Servio
Length of output: 3560
🏁 Script executed:
# Get the full firestore.rules file to check if notifications collection has rules
cat firestore.rulesRepository: hrx01-dev/Servio
Length of output: 6289
🏁 Script executed:
# Read the full notificationService.ts file to understand the implementation
cat src/dashboard/notifications/notificationService.tsRepository: hrx01-dev/Servio
Length of output: 4224
Add per-user ownership verification to notification mutation functions and define explicit Firestore rules.
markAsRead() lacks user validation entirely—any user can update any notification by ID. markAllAsRead() accepts userId as a client parameter without verifying it matches the authenticated user. Define explicit rules for the notifications and notificationPreferences collections that enforce request.auth.uid == userId for all read/write operations, and modify markAsRead(notificationId, userId) to accept and validate the userId parameter server-side (via Firestore rules).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/notificationService.ts` around lines 108 - 117,
The `markAsRead()` function lacks user validation, allowing any user to update
any notification by ID, and `markAllAsRead()` accepts userId as a client
parameter without verifying it matches the authenticated user. Modify the
`markAsRead()` signature to accept a userId parameter alongside notificationId,
then add validation logic before the updateDoc call to verify that the
notification's userId matches the provided userId parameter. For
`markAllAsRead()`, ensure the userId parameter is validated against the
authenticated user's ID before executing the query. Additionally, define
explicit Firestore security rules for the NOTIFICATIONS_COLLECTION that enforce
request.auth.uid == request.resource.data.userId for all read and write
operations to prevent unauthorized access at the database level.
| const batch = writeBatch(db); | ||
| snapshot.docs.forEach((docSnap) => { | ||
| batch.update(docSnap.ref, { isRead: true }); | ||
| }); | ||
| await batch.commit(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to see the current state
cat -n src/dashboard/notifications/notificationService.ts | head -150Repository: hrx01-dev/Servio
Length of output: 4994
🌐 Web query:
Firestore writeBatch operation limit maximum writes per batch
💡 Result:
The maximum number of write operations allowed in a single Firestore WriteBatch is 500 [1]. If you attempt to exceed this limit by adding more than 500 operations to a single batch, the operation will fail. It is recommended that for operations requiring a larger number of documents, you should either split the work into smaller batches or use alternative methods such as a bulk writer or parallelized individual writes [2]. Note that this 500-operation limit also applies to transactions [2][3].
Citations:
- 1: https://github.com/firebase/firebase-js-sdk/blob/master/packages/firestore/src/api/write_batch.ts
- 2: https://firebase.google.com/docs/firestore/enterprise/transactions
- 3: Adding documentation for WriteBatch size limit firebase/firebase-js-sdk#1715
Add batch chunking to prevent failure when unread notification count exceeds 500.
The markAllAsRead function queries all unread notifications with no limit and attempts to update them in a single batch. Firestore's writeBatch operation limit is 500; if a user has more than 500 unread notifications, commit() will fail and nothing will be marked read.
Proposed fix
export async function markAllAsRead(userId: string): Promise<void> {
const q = query(
collection(db, NOTIFICATIONS_COLLECTION),
where("userId", "==", userId),
where("isRead", "==", false),
);
const snapshot = await getDocs(q);
if (snapshot.empty) return;
+ const BATCH_SIZE = 500;
- const batch = writeBatch(db);
- snapshot.docs.forEach((docSnap) => {
+ for (let i = 0; i < snapshot.docs.length; i += BATCH_SIZE) {
+ const batch = writeBatch(db);
+ snapshot.docs.slice(i, i + BATCH_SIZE).forEach((docSnap) => {
batch.update(docSnap.ref, { isRead: true });
+ });
+ await batch.commit();
}
- await batch.commit();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const batch = writeBatch(db); | |
| snapshot.docs.forEach((docSnap) => { | |
| batch.update(docSnap.ref, { isRead: true }); | |
| }); | |
| await batch.commit(); | |
| } | |
| const BATCH_SIZE = 500; | |
| for (let i = 0; i < snapshot.docs.length; i += BATCH_SIZE) { | |
| const batch = writeBatch(db); | |
| snapshot.docs.slice(i, i + BATCH_SIZE).forEach((docSnap) => { | |
| batch.update(docSnap.ref, { isRead: true }); | |
| }); | |
| await batch.commit(); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/notificationService.ts` around lines 122 - 127,
The markAllAsRead function processes all unread notifications in a single
writeBatch, which fails when the count exceeds Firestore's 500 operation limit.
Modify the function to chunk the snapshot.docs array into groups of 500 or
fewer, creating a new writeBatch for each chunk and committing each batch
separately. This ensures that regardless of the number of unread notifications,
they will all be marked as read without exceeding Firestore's batch operation
constraints.
| async function trigger(params: TriggerParams) { | ||
| try { | ||
| await createNotification(params); |
There was a problem hiding this comment.
Notification preferences are not applied before creating notifications.
All trigger helpers write notifications directly, so disabled categories/channels in preferences do not affect delivery. That makes preferences effectively non-functional for generation behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/notificationTriggers.ts` around lines 13 - 15,
The trigger function is creating notifications directly without checking the
user's notification preferences, making preference settings ineffective. Before
calling createNotification within the trigger function, fetch the user's
notification preferences based on the user ID and notification category/channel
from the params object, then only proceed with the createNotification call if
the user's preferences allow notifications for that specific category and
channel combination. This will ensure that disabled notification preferences are
properly respected during notification generation.
| async function trigger(params: TriggerParams) { | ||
| try { | ||
| await createNotification(params); | ||
| } catch (error) { | ||
| console.error("Failed to create notification:", error); | ||
| } |
There was a problem hiding this comment.
Do not swallow notification creation failures.
trigger logs and resolves on error, so callers cannot detect failure or retry. This silently drops notifications on write errors.
🐛 Proposed fix
async function trigger(params: TriggerParams) {
try {
await createNotification(params);
} catch (error) {
console.error("Failed to create notification:", error);
+ throw error;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function trigger(params: TriggerParams) { | |
| try { | |
| await createNotification(params); | |
| } catch (error) { | |
| console.error("Failed to create notification:", error); | |
| } | |
| async function trigger(params: TriggerParams) { | |
| try { | |
| await createNotification(params); | |
| } catch (error) { | |
| console.error("Failed to create notification:", error); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/dashboard/notifications/notificationTriggers.ts` around lines 13 - 18,
The trigger function is catching errors from createNotification and logging them
without re-throwing, which prevents callers from detecting failures or
implementing retry logic. Remove the try-catch block around the
createNotification call in the trigger function to allow errors to propagate up
to callers, or alternatively keep the catch block but re-throw the error after
logging so that the failure is not silently swallowed.
|
Done — welcome notification is now triggered on both email and Google signup flows. The change is in the new PR #100 (this PR's branch got blocked by repo rulesets on subsequent pushes, so I opened a fresh PR with all changes including this one). In notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there'); |
1 participant
Summary
Implements the full in-app notification system requested in #98. The system uses Firebase Firestore for persistence and real-time updates.
Architecture
Key decisions
onSnapshot:NotificationProvidersubscribes to Firestore withonSnapshot— notifications update instantly without refresh.NotificationContextObject.tsholds thecreateContextcall;NotificationContext.tsxholds theProvidercomponent;useNotifications.tshas the hook — avoids thereact-refresh/only-export-componentslint error.DashboardLayoutheader (now always visible, not mobile-only) includes<NotificationBell />with animated unread badge and dropdown panel.NotificationCenterdoes client-side filtering/search/pagination over the real-time snapshot;fetchNotificationsPageis available for server-side cursor pagination if needed.notificationTriggers.tsexports typed helpers (notifyProjectCreated,notifyPaymentReceived,notifyWelcome, etc.) ready to be called from existing event handlers.notificationPreferences/{userId}in Firestore.Routes added
/dashboard/notifications→NotificationCenter/dashboard/notification-preferences→NotificationPreferencesLink to Devin session: https://app.devin.ai/sessions/0f66026af44c4180b0c1a5adefe5832b
Requested by: @hrx01-dev
Summary by CodeRabbit
Release Notes