Issue 59: Family Notification System#94
Conversation
📝 WalkthroughWalkthroughThis PR implements a complete notification system for the gift registry. It adds type definitions, Firestore persistence, server functions for notification CRUD operations, integrates notification publishing into the gift claim flow, provides React Query hooks for client-side data fetching, and refactors the home route and notification card component to display real notifications instead of hardcoded placeholders. ChangesGift Notification System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 3
🧹 Nitpick comments (1)
common/src/types/notification.ts (1)
12-17: ⚡ Quick winUse
z.uuid()for Notification ID fields (id,familyId,childId,giftId)
notificationService.server.tsgeneratesidwithuuidv7(), andfamilyForm.ts/donor.tspopulatefamilyId,childId, andgiftIdfrom UUIDv7-basedfamilies/children/giftsdocuments—so the schema should enforce UUID format for consistency and early rejection.♻️ Proposed refactor to add UUID validation
export const NotificationSchema = z.object({ - id: z.string(), - familyId: z.string(), - childId: z.string(), + id: z.uuid(), + familyId: z.uuid(), + childId: z.uuid(), type: NotificationTypeSchema, message: z.string(), - giftId: z.string(), + giftId: z.uuid(), createdAt: z.string().datetime(), read: z.boolean(), });🤖 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 `@common/src/types/notification.ts` around lines 12 - 17, Update the notification schema so ID fields validate as UUIDs: replace the current z.string() validators for id, familyId, childId, and giftId with UUID validators (use z.string().uuid(), or z.uuid() if your Zod version exposes that helper) while leaving the type field as NotificationTypeSchema and message as z.string(); this enforces UUID format for the id, familyId, childId, and giftId properties.
🤖 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 `@app/src/routes/family/`$token/home.tsx:
- Around line 48-50: handleDismiss currently only updates local state
(setDismissedIds) so dismissed notifications reappear after refresh; update it
to call the mark-as-read routine from useMarkNotificationAsRead (e.g., invoke
markNotificationAsRead or similar returned by the hook) passing the notification
id, await or handle the promise and only then add the id to setDismissedIds (or
perform an optimistic update but rollback on error); also handle errors from the
hook (log/show toast) so UI and Firestore stay consistent.
In `@app/src/server/functions/notifications.ts`:
- Around line 56-65: clearAllNotifications currently builds a single WriteBatch
for all notifications which can exceed Firestore per-request limits; change the
logic to only query and update unread notifications (filter where read == false)
and process them in small chunks (e.g., 200 updates per batch) by creating a new
db._instance.batch() for each chunk and committing each batch sequentially (or
with controlled concurrency) until all chunks are committed; update the code
that iterates notificationsSnap.forEach to collect only unread docs, split into
chunks, call batch.update for each doc in the chunk, and commit each chunk
separately.
---
Nitpick comments:
In `@common/src/types/notification.ts`:
- Around line 12-17: Update the notification schema so ID fields validate as
UUIDs: replace the current z.string() validators for id, familyId, childId, and
giftId with UUID validators (use z.string().uuid(), or z.uuid() if your Zod
version exposes that helper) while leaving the type field as
NotificationTypeSchema and message as z.string(); this enforces UUID format for
the id, familyId, childId, and giftId properties.
🪄 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
Run ID: 80304ba1-e812-44d9-ab55-d0d7bc717f39
📒 Files selected for processing (11)
app/src/components/family/NotificationCard.tsxapp/src/data/collections.tsapp/src/hooks/mutations/useNotificationMutations.tsapp/src/hooks/queries/useFamilyNotifications.tsapp/src/lib/firebase.server.tsapp/src/routes/family/$token/home.tsxapp/src/server/functions/donor.tsapp/src/server/functions/notifications.tsapp/src/server/services/notificationService.server.tscommon/src/types/index.tscommon/src/types/notification.ts
| const handleDismiss = (id: string) => { | ||
| setVisibleIds((prev) => prev.filter((i) => i !== id)); | ||
| setDismissedIds((prev) => [...prev, id]); | ||
| }; |
There was a problem hiding this comment.
Persist dismisses by marking notifications as read.
Dismiss currently hides items only in local state, so they return on refresh because read is never updated server-side. Wire dismiss to useMarkNotificationAsRead so UI state and Firestore stay consistent.
🔧 Proposed fix
-import { useClearAllNotifications } from "`@/hooks/mutations/useNotificationMutations`";
+import {
+ useClearAllNotifications,
+ useMarkNotificationAsRead,
+} from "`@/hooks/mutations/useNotificationMutations`";
const { data: notificationsData } = useFamilyNotifications(family?.id);
const clearAllMutation = useClearAllNotifications();
+ const markReadMutation = useMarkNotificationAsRead();
- const handleDismiss = (id: string) => {
- setDismissedIds((prev) => [...prev, id]);
- };
+ const handleDismiss = async (id: string) => {
+ setDismissedIds((prev) => [...prev, id]); // optimistic
+ try {
+ await markReadMutation.mutateAsync(id);
+ } catch {
+ setDismissedIds((prev) => prev.filter((dismissedId) => dismissedId !== id));
+ }
+ };
- onDismiss={() => handleDismiss(n.id)}
+ onDismiss={() => void handleDismiss(n.id)}🤖 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 `@app/src/routes/family/`$token/home.tsx around lines 48 - 50, handleDismiss
currently only updates local state (setDismissedIds) so dismissed notifications
reappear after refresh; update it to call the mark-as-read routine from
useMarkNotificationAsRead (e.g., invoke markNotificationAsRead or similar
returned by the hook) passing the notification id, await or handle the promise
and only then add the id to setDismissedIds (or perform an optimistic update but
rollback on error); also handle errors from the hook (log/show toast) so UI and
Firestore stay consistent.
| export const getFamilyNotifications = createServerFn({ method: "POST" }) | ||
| .inputValidator(getFamilyNotificationsSchema) | ||
| .handler(async ({ data }) => { | ||
| const db = getServerDB(); | ||
| const { familyId } = data; | ||
|
|
||
| const notificationsSnap = await db.notifications | ||
| .where("familyId", "==", familyId) | ||
| .orderBy("createdAt", "desc") | ||
| .get(); | ||
|
|
||
| const notifications: Array<Notification> = []; | ||
| notificationsSnap.forEach((doc) => { | ||
| notifications.push(doc.data() as Notification); | ||
| }); | ||
|
|
||
| return { notifications }; | ||
| }); | ||
|
|
||
| export const markNotificationAsRead = createServerFn({ method: "POST" }) | ||
| .inputValidator( | ||
| z.object({ | ||
| notificationId: z.string().min(1), | ||
| }), | ||
| ) | ||
| .handler(async ({ data }) => { | ||
| const db = getServerDB(); | ||
| const { notificationId } = data; | ||
|
|
||
| await db.notifications.doc(notificationId).update({ | ||
| read: true, | ||
| }); | ||
|
|
||
| return { success: true }; | ||
| }); | ||
|
|
||
| export const clearAllNotifications = createServerFn({ method: "POST" }) | ||
| .inputValidator( | ||
| z.object({ | ||
| familyId: z.string().min(1), | ||
| }), | ||
| ) | ||
| .handler(async ({ data }) => { | ||
| const db = getServerDB(); | ||
| const { familyId } = data; | ||
|
|
||
| const notificationsSnap = await db.notifications | ||
| .where("familyId", "==", familyId) | ||
| .get(); | ||
|
|
||
| const batch = db._instance.batch(); | ||
| notificationsSnap.forEach((doc) => { | ||
| batch.update(doc.ref, { read: true }); | ||
| }); | ||
|
|
||
| await batch.commit(); | ||
|
|
||
| return { success: true }; |
There was a problem hiding this comment.
Add family-scoped authorization checks to these server functions.
These handlers trust caller-supplied familyId / notificationId and never verify that the caller actually owns the target family or notification. As written, a client can read another family's notifications or mark them read/clear them by guessing ids. Require the family/token auth middleware here, and for markNotificationAsRead load the notification first and verify its familyId before updating it.
| const notificationsSnap = await db.notifications | ||
| .where("familyId", "==", familyId) | ||
| .get(); | ||
|
|
||
| const batch = db._instance.batch(); | ||
| notificationsSnap.forEach((doc) => { | ||
| batch.update(doc.ref, { read: true }); | ||
| }); | ||
|
|
||
| await batch.commit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the maximum number of operations allowed in a single Cloud Firestore batched write, according to the official Firebase documentation?
💡 Result:
According to official Firebase documentation, there is no longer a fixed maximum of 500 operations for a single Cloud Firestore batched write/commit; Firestore “no longer limits the number of writes that can be passed to a Commit operation” (the previous limit was 500). [1] So, the answer is: no maximum number (the former 500 cap was removed), with other limits still applying (for example, maximum request size and transaction time limits are still relevant). [1]
Citations:
🌐 Web query:
Firestore WriteBatch commit maximum number of writes removed 500 any remaining limits request size transaction time
💡 Result:
In Firestore, a WriteBatch (batched writes) has these relevant limits/constraints: 1) “Maximum number of writes” (the old 500 limit) The currently supported limits do not show “max 500 writes per WriteBatch commit” in the official Firestore quotas/limits tables. Instead, Firestore documents other hard limits for requests and transactions, and explicitly notes that large batched writes may exceed other constraints such as request size. [1][2] 2) Maximum API request size (applies to batch/transaction requests) Maximum API request size is 10 MiB. If your batch is too large, the commit can fail because the underlying request would exceed this size. [1][3] 3) Transaction time limits (applies to transactions; batched writes aren’t “transactions”) For transactions, the time limit is 270 seconds, with a 60-second idle expiration time. [1][3] (Write batches are atomic units of multiple writes, but they’re not the same as transactions; the transaction time limit is still the relevant “time limit” number Firestore publishes for transactions.) [2][1] 4) “Remaining limits” that can affect batched writes Firesture’s “Usage and limits” also lists limits for counts of document access calls per request, and it includes batched writes in that limit category. Exceeding those can result in permission denied errors. [1] What you should do in practice - If you’re trying to write a lot of documents: split into smaller batches when you’re likely to exceed 10 MiB request size or hit per-request document-access call limits. [1][2] - If you need to both read and write atomically with read constraints, use a transaction (and design around the 270s transaction time limit and 60s idle expiration). [1][2] Answering your “removed 500 / any remaining limits” concern - The “500 writes” figure is no longer presented as a current hard limit in the official quotas tables I found; the authoritative published limits include 10 MiB maximum API request size and other per-request constraints that are more general determinants of whether a batch commit will succeed. [1][3] If you share which client SDK/language you use (Node, Python, Java, Admin SDK) and roughly how large each write is (number of operations and average payload size), I can suggest a batching strategy that stays under the 10 MiB request-size limit and the relevant per-request call limits.
Citations:
- 1: https://firebase.google.com/docs/firestore/quotas
- 2: https://firebase.google.com/docs/firestore/manage-data/transactions
- 3: https://docs.cloud.google.com/firestore/quotas
Avoid oversized Firestore batches in clearAllNotifications
The current code creates one WriteBatch for all matching notifications; Firestore no longer enforces the old fixed “500 writes per commit” limit, but batch commits can still fail due to other hard constraints like the 10 MiB maximum API request size (and other per-request limits). Split updates into smaller batches (and ideally only mark unread notifications) to keep commits safely within limits. https://firebase.google.com/docs/firestore/quotas https://firebase.google.com/docs/firestore/manage-data/transactions
🤖 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 `@app/src/server/functions/notifications.ts` around lines 56 - 65,
clearAllNotifications currently builds a single WriteBatch for all notifications
which can exceed Firestore per-request limits; change the logic to only query
and update unread notifications (filter where read == false) and process them in
small chunks (e.g., 200 updates per batch) by creating a new
db._instance.batch() for each chunk and committing each batch sequentially (or
with controlled concurrency) until all chunks are committed; update the code
that iterates notificationsSnap.forEach to collect only unread docs, split into
chunks, call batch.update for each doc in the chunk, and commit each chunk
separately.
1 participant
Pull Request
Description
Updated notification display area on the family home page: gift-related events such as claims and delivery confirmations are now published to a notifications collection in Firestore.
Type of Change
Related Issues (put task name here from notion)
Issue #59
Screenshots (If it is a front end feature screenshot is required)
Additional Notes
Summary by CodeRabbit
New Features
Refactor