feat: add in-app notification system with welcome message (#98)#100
Merged
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>
Calls notifyWelcome() after both email and Google sign-up flows, creating a welcome notification in the user's notification center. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
Author
🤖 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:
|
📝 WalkthroughWalkthroughIntroduces a complete in-app notification system: domain types, a Firestore service layer, event trigger helpers, a React context/provider with real-time subscriptions, a ChangesIn-App Notification System
Sequence Diagram(s)sequenceDiagram
participant User
participant SignUp
participant notificationTriggers
participant notificationService
participant Firestore
participant NotificationBell
rect rgba(70, 130, 180, 0.5)
Note over User,Firestore: Sign-up flow
User->>SignUp: submit credentials / Google sign-in
SignUp->>notificationTriggers: notifyWelcome(userId, name)
notificationTriggers->>notificationService: createNotification(payload)
notificationService->>Firestore: addDoc to notifications collection
end
rect rgba(60, 179, 113, 0.5)
Note over NotificationBell,Firestore: Real-time notification delivery
NotificationBell->>notificationService: subscribeToNotifications(userId, cb)
notificationService->>Firestore: onSnapshot(query)
Firestore-->>notificationService: new notification snapshot
notificationService-->>NotificationBell: cb(Notification[]) → renders badge + dropdown item
end
rect rgba(255, 165, 0, 0.5)
Note over User,Firestore: Mark all as read
User->>NotificationBell: click "Mark all read"
NotificationBell->>notificationService: markAllAsRead(userId)
notificationService->>Firestore: writeBatch isRead=true
Firestore-->>notificationService: commit ack
notificationService-->>NotificationBell: updated snapshot clears badge
end
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 af49c79): https://servio-0--pr100-devin-1782065796-not-sm8znyqg.web.app (expires Sun, 28 Jun 2026 18:18:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 15915abb5951eb298a844eda460b24f444d93a69 |
![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
🤖 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-99: The Card component in NotificationCenter.tsx with the
onClick handler is only accessible via pointer clicks, blocking keyboard users.
Make the Card keyboard-activatable by adding a tabIndex attribute to make it
focusable, adding an onKeyDown event handler to detect Enter or Space key
presses and trigger the same handleClick logic, and optionally adding a role
attribute to indicate the component is interactive. This ensures both pointer
and keyboard users can open notifications and perform actions.
In `@src/dashboard/notifications/NotificationPreferences.tsx`:
- Around line 58-63: The `localPrefs` state is initialized only once with the
`preferences` prop, but when `preferences` changes asynchronously (such as when
preferences are loaded from the backend), the `localPrefs` state is not updated,
causing the UI to display stale defaults and false unsaved changes. Add a
useEffect hook that watches `preferences` as a dependency and updates
`localPrefs` whenever the `preferences` prop changes, ensuring the local state
stays synchronized with the async-loaded preferences.
- Around line 65-73: The handleSave function in NotificationPreferences.tsx
calls setSaved(true) after awaiting updatePreferences, but since the
updatePreferences function in NotificationContext.tsx logs and swallows errors
without rethrowing, the success state is set even when persistence fails. Modify
the updatePreferences function in NotificationContext.tsx to rethrow the error
after logging it, then update the try-catch block in handleSave to properly
handle the caught error so that setSaved(true) only executes when the update
actually succeeds.
- Around line 43-52: The Switch component in the notification preference item
lacks explicit association with its Label, making it inaccessible to screen
readers. Add a unique id attribute to the Label component and then link the
Switch component to that id using the aria-labelledby attribute to establish the
proper accessible relationship between the label and the switch toggle.
In `@src/dashboard/notifications/notificationService.ts`:
- Around line 154-161: The updatePreferences function contains an unnecessary
dynamic import of setDoc from firebase/firestore inside the function body. Since
setDoc is already imported at the top of the file from the same
firebase/firestore package, remove the dynamic import statement and simply use
the already-imported setDoc directly in the setDoc call within the function.
This will eliminate the redundant import and keep the code consistent with the
rest of the codebase.
- Around line 113-127: The markAllAsRead function performs a query on the
notifications collection with two where clauses filtering by userId and isRead
fields, which requires a composite index in Firestore. Add a composite index
configuration to firestore.indexes.json for the notifications collection with
fields userId (ascending) and isRead (ascending) to ensure the query on the
NOTIFICATIONS_COLLECTION will execute without runtime errors.
In `@src/Firebase/SignUp.tsx`:
- Around line 264-267: The current implementation calls notifyWelcome for all
users who sign in via the GoogleAuthProvider, including returning users. To fix
this, use getAdditionalUserInfo from Firebase to check if the user is new before
sending the welcome notification. Destructure the result of signInWithPopup to
include the additional user info, then check the isNewUser property from
getAdditionalUserInfo before calling notifyWelcome. Only invoke notifyWelcome
when isNewUser is true to prevent duplicate notifications for returning users.
🪄 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: ad755d63-0d50-44f8-b2fe-5ac86c8df873
📒 Files selected for processing (13)
src/Firebase/SignUp.tsxsrc/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
Comment on lines
+93
to
+99
| <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"> |
There was a problem hiding this comment.
Make rows keyboard-activatable, not pointer-only.
Line [93]-Line [99] wires interaction through onClick only. This can block keyboard users from opening actionable notifications or marking them read.
Suggested fix
<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" : ""
}`}
+ role="button"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ handleClick();
+ }
+ }}
onClick={handleClick}
>📝 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.
Suggested change
| <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"> | |
| <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" : "" | |
| }`} | |
| role="button" | |
| tabIndex={0} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| handleClick(); | |
| } | |
| }} | |
| onClick={handleClick} | |
| > | |
| <CardContent className="flex items-start gap-4 py-4"> |
🤖 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 - 99, The
Card component in NotificationCenter.tsx with the onClick handler is only
accessible via pointer clicks, blocking keyboard users. Make the Card
keyboard-activatable by adding a tabIndex attribute to make it focusable, adding
an onKeyDown event handler to detect Enter or Space key presses and trigger the
same handleClick logic, and optionally adding a role attribute to indicate the
component is interactive. This ensures both pointer and keyboard users can open
notifications and perform actions.
Comment on lines
+43
to
+52
| <Label className="text-sm font-medium text-gray-900 dark:text-gray-100"> | ||
| {label} | ||
| </Label> | ||
| <p className="text-xs text-gray-500 dark:text-gray-400"> | ||
| {description} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| <Switch checked={checked} onCheckedChange={onCheckedChange} /> | ||
| </div> |
There was a problem hiding this comment.
Associate each switch with its visible label.
The Switch controls currently have no explicit accessible name linkage (id/htmlFor), which can make preference toggles ambiguous for screen readers.
Suggested fix
-import { useState } from "react";
+import { useId, useState } from "react";
@@
function ToggleRow({
@@
}: ToggleRowProps) {
+ const switchId = useId();
return (
@@
- <Label className="text-sm font-medium text-gray-900 dark:text-gray-100">
+ <Label htmlFor={switchId} className="text-sm font-medium text-gray-900 dark:text-gray-100">
{label}
</Label>
@@
- <Switch checked={checked} onCheckedChange={onCheckedChange} />
+ <Switch id={switchId} checked={checked} onCheckedChange={onCheckedChange} />
</div>
);
}📝 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.
Suggested change
| <Label className="text-sm font-medium text-gray-900 dark:text-gray-100"> | |
| {label} | |
| </Label> | |
| <p className="text-xs text-gray-500 dark:text-gray-400"> | |
| {description} | |
| </p> | |
| </div> | |
| </div> | |
| <Switch checked={checked} onCheckedChange={onCheckedChange} /> | |
| </div> | |
| import { useId, useState } from "react"; | |
| function ToggleRow({ | |
| label, | |
| description, | |
| checked, | |
| onCheckedChange, | |
| }: ToggleRowProps) { | |
| const switchId = useId(); | |
| return ( | |
| <div className="flex items-center justify-between py-3"> | |
| <div className="flex flex-col gap-1"> | |
| <Label htmlFor={switchId} className="text-sm font-medium text-gray-900 dark:text-gray-100"> | |
| {label} | |
| </Label> | |
| <p className="text-xs text-gray-500 dark:text-gray-400"> | |
| {description} | |
| </p> | |
| </div> | |
| </div> | |
| <Switch id={switchId} checked={checked} onCheckedChange={onCheckedChange} /> | |
| </div> | |
| ); | |
| } |
🤖 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 43 -
52, The Switch component in the notification preference item lacks explicit
association with its Label, making it inaccessible to screen readers. Add a
unique id attribute to the Label component and then link the Switch component to
that id using the aria-labelledby attribute to establish the proper accessible
relationship between the label and the switch toggle.
Comment on lines
+58
to
+63
| 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 draft preferences when context preferences change.
Line [58] snapshots preferences only once. Because preferences are loaded asynchronously, the UI can display stale defaults and false “unsaved changes”.
Suggested fix
-import { useState } from "react";
+import { useEffect, useState } from "react";
@@
const [localPrefs, setLocalPrefs] = useState<PrefsType>(preferences);
@@
+ useEffect(() => {
+ setLocalPrefs(preferences);
+ }, [preferences]);📝 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.
Suggested change
| 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 [saving, setSaving] = useState(false); | |
| const [saved, setSaved] = useState(false); | |
| useEffect(() => { | |
| setLocalPrefs(preferences); | |
| }, [preferences]); | |
| 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 only once with the `preferences` prop,
but when `preferences` changes asynchronously (such as when preferences are
loaded from the backend), the `localPrefs` state is not updated, causing the UI
to display stale defaults and false unsaved changes. Add a useEffect hook that
watches `preferences` as a dependency and updates `localPrefs` whenever the
`preferences` prop changes, ensuring the local state stays synchronized with the
async-loaded preferences.
Comment on lines
+65
to
+73
| 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 report save success when persistence fails.
Line [69] sets success after await updatePreferences(localPrefs), but the provider implementation logs and swallows errors, so this path can show “Saved!” even on write failure.
Suggested fix
const handleSave = async () => {
setSaving(true);
+ setSaved(false);
try {
await updatePreferences(localPrefs);
setSaved(true);
setTimeout(() => setSaved(false), 2000);
+ } catch {
+ setSaved(false);
} finally {
setSaving(false);
}
};Also rethrow in src/dashboard/notifications/NotificationContext.tsx inside updatePreferences after logging so callers can react to failures.
🤖 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 in NotificationPreferences.tsx calls setSaved(true)
after awaiting updatePreferences, but since the updatePreferences function in
NotificationContext.tsx logs and swallows errors without rethrowing, the success
state is set even when persistence fails. Modify the updatePreferences function
in NotificationContext.tsx to rethrow the error after logging it, then update
the try-catch block in handleSave to properly handle the caught error so that
setSaved(true) only executes when the update actually succeeds.
Comment on lines
+113
to
+127
| 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 = 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:
#!/bin/bash
# Description: Check for Firestore composite index configuration
# Look for firestore.indexes.json or firebase.json with index definitions
fd -e json -e js 'firestore.indexes|firebase' --exec cat {} \; | grep -A 10 -B 2 'notifications'Repository: hrx01-dev/Servio
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for Firebase configuration files more broadly
echo "=== Searching for Firebase/Firestore config files ==="
find . -type f \( -name "firestore.indexes.json" -o -name "firebase.json" -o -name ".firebaserc" \) 2>/dev/null | head -20
echo ""
echo "=== Checking for any JSON files in root/config directories ==="
ls -la . 2>/dev/null | grep -E '\.(json|config)'
ls -la ./config 2>/dev/null || echo "No config directory found"
ls -la ./firebase 2>/dev/null || echo "No firebase directory found"
echo ""
echo "=== Looking for index definitions in the codebase ==="
rg -i "composite.*index|index.*notification|isRead.*userId" --type json --type js --type ts | head -20
echo ""
echo "=== Checking the project structure ==="
git ls-files | grep -E '(firebase|firestore)' | head -20Repository: hrx01-dev/Servio
Length of output: 1170
🏁 Script executed:
#!/bin/bash
# Check if Firestore is actually being used and how indexes are managed
echo "=== Checking Firestore initialization and setup ==="
rg -i "initializeApp|getFirestore|firebaseConfig" --type ts --type js -A 3 | head -40
echo ""
echo "=== Looking for Firestore rules file ==="
find . -name "firestore.rules" 2>/dev/null
echo ""
echo "=== Checking package.json for Firebase version ==="
cat package.json | grep -A 5 -B 5 firebaseRepository: hrx01-dev/Servio
Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Check the actual content of firestore.indexes.json
echo "=== Content of firestore.indexes.json ==="
cat firestore.indexes.json
echo ""
echo "=== Content of firebase.json ==="
cat firebase.jsonRepository: hrx01-dev/Servio
Length of output: 1165
🏁 Script executed:
#!/bin/bash
# Verify the exact query in the notificationService.ts file
echo "=== Viewing the exact code in notificationService.ts lines 113-127 ==="
sed -n '113,127p' src/dashboard/notifications/notificationService.tsRepository: hrx01-dev/Servio
Length of output: 559
Add composite index for the markAllAsRead query.
The query uses two where clauses on different fields (userId and isRead), which requires a composite index in Firestore. The index is currently missing from firestore.indexes.json. Without it, this query will fail at runtime.
Add the following index to firestore.indexes.json:
Composite index configuration
{
"collectionGroup": "notifications",
"queryScope": "COLLECTION",
"fields": [
{ "fieldPath": "userId", "order": "ASCENDING" },
{ "fieldPath": "isRead", "order": "ASCENDING" }
]
}🤖 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 113 - 127,
The markAllAsRead function performs a query on the notifications collection with
two where clauses filtering by userId and isRead fields, which requires a
composite index in Firestore. Add a composite index configuration to
firestore.indexes.json for the notifications collection with fields userId
(ascending) and isRead (ascending) to ensure the query on the
NOTIFICATIONS_COLLECTION will execute without runtime errors.
Comment on lines
+154
to
+161
| export async function updatePreferences( | ||
| userId: string, | ||
| prefs: NotificationPreferences, | ||
| ): Promise<void> { | ||
| const { setDoc } = await import("firebase/firestore"); | ||
| const ref = doc(db, PREFERENCES_COLLECTION, userId); | ||
| await setDoc(ref, prefs, { merge: true }); | ||
| } |
There was a problem hiding this comment.
Remove unnecessary dynamic import.
setDoc is available from the same firebase/firestore package already imported at the top of the file. The dynamic import is inconsistent with the rest of the codebase and adds unnecessary overhead.
♻️ Proposed fix
import {
collection,
query,
where,
orderBy,
onSnapshot,
doc,
updateDoc,
addDoc,
writeBatch,
getDocs,
Timestamp,
limit,
startAfter,
+ setDoc,
type QueryDocumentSnapshot,
type DocumentData,
} from "firebase/firestore"; export async function updatePreferences(
userId: string,
prefs: NotificationPreferences,
): Promise<void> {
- const { setDoc } = await import("firebase/firestore");
const ref = doc(db, PREFERENCES_COLLECTION, userId);
await setDoc(ref, prefs, { merge: true });
}🤖 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 154 - 161,
The updatePreferences function contains an unnecessary dynamic import of setDoc
from firebase/firestore inside the function body. Since setDoc is already
imported at the top of the file from the same firebase/firestore package, remove
the dynamic import statement and simply use the already-imported setDoc directly
in the setDoc call within the function. This will eliminate the redundant import
and keep the code consistent with the rest of the codebase.
Comment on lines
264
to
267
| const provider = new GoogleAuthProvider(); | ||
| await signInWithPopup(auth, provider); | ||
| const { user } = await signInWithPopup(auth, provider); | ||
| notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there'); | ||
| navigate('/dashboard'); |
There was a problem hiding this comment.
Duplicate welcome notifications for returning Google users.
signInWithPopup succeeds for both new and existing users. A returning user who uses the signup page will receive another welcome notification. Use getAdditionalUserInfo to check if the user is new before triggering the notification.
🐛 Proposed fix to only notify new users
+import { getAdditionalUserInfo } from 'firebase/auth'; const handleGoogleSignIn = async () => {
setError('');
try {
const provider = new GoogleAuthProvider();
- const { user } = await signInWithPopup(auth, provider);
- notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there');
+ const result = await signInWithPopup(auth, provider);
+ const { user } = result;
+ if (getAdditionalUserInfo(result)?.isNewUser) {
+ notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there');
+ }
navigate('/dashboard');
} catch (err: unknown) {📝 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.
Suggested change
| const provider = new GoogleAuthProvider(); | |
| await signInWithPopup(auth, provider); | |
| const { user } = await signInWithPopup(auth, provider); | |
| notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there'); | |
| navigate('/dashboard'); | |
| const provider = new GoogleAuthProvider(); | |
| const result = await signInWithPopup(auth, provider); | |
| const { user } = result; | |
| if (getAdditionalUserInfo(result)?.isNewUser) { | |
| notifyWelcome(user.uid, user.displayName ?? user.email ?? 'there'); | |
| } | |
| navigate('/dashboard'); |
🤖 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/Firebase/SignUp.tsx` around lines 264 - 267, The current implementation
calls notifyWelcome for all users who sign in via the GoogleAuthProvider,
including returning users. To fix this, use getAdditionalUserInfo from Firebase
to check if the user is new before sending the welcome notification. Destructure
the result of signInWithPopup to include the additional user info, then check
the isNewUser property from getAdditionalUserInfo before calling notifyWelcome.
Only invoke notifyWelcome when isNewUser is true to prevent duplicate
notifications for returning users.
hrx01-dev
approved these changes
Jun 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the full in-app notification system requested in #98, plus a welcome notification on signup (requested in PR #99 review).
Supersedes #99 (blocked by branch protection rules on subsequent pushes).
Architecture
Key decisions
onSnapshot:NotificationProvidersubscribes to Firestore — notifications update instantly without refresh.createContext,Provider, anduseNotificationsinto three files to satisfyreact-refresh/only-export-components.DashboardLayoutheader includes<NotificationBell />with animated unread badge and dropdown.NotificationCenterfilters/searches/paginates over the real-time snapshot;fetchNotificationsPageis available for server-side cursor pagination.notificationTriggers.tsexports typed helpers (notifyProjectCreated,notifyPaymentReceived,notifyWelcome, etc.).SignUp.tsxnow callnotifyWelcome(user.uid, name)after successful registration.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