[PULSE-55] In-app notification center, settings tab, and notifications page #28
Reference in New Issue
Block a user
No description provided.
Delete Branch "staging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Work Item
PULSE-55
Summary
Changes
Components:
components/notifications/NotificationCenter.tsx: Bell dropdown with recent notifications, unread badge, "View all" and "Manage settings" links, empty state, 90s polling.Pages:
app/notifications/page.tsx: Full list of notifications, pagination (load more), mark read / mark all read.Settings:
components/settings/OrganizationSettings.tsx: Notifications tab with toggles for Billing and Uptime; hidden for members.API:
lib/api/notifications.ts:listNotifications,markNotificationRead,markAllNotificationsRead.lib/api/notification-settings.ts:getNotificationSettings,updateNotificationSettings.Layout:
app/layout-content.tsx: Notification center in header.Test Plan
Greptile Summary
This PR adds a complete in-app notification system: a bell icon dropdown in the header (
NotificationCenter), a full/notificationspage with pagination, and a Notifications settings tab in organization settings for owners/admins. It also introduces two new API client modules for notifications and notification settings, bumps the app version to 0.6.0-alpha, and updates the UI package./notificationswith offset-based pagination ("Load more"), mark-read on click, and mark-all-readhandleMarkAllReadin the notifications page discards the actual API error and constructs a new genericError, losing status-code-specific error messagesBellIconSVG component andformatTimeAgo/getTypeIconhelper functions are duplicated acrossNotificationCenter.tsxandnotifications/page.tsx/OrganizationSettings.tsx— consider extracting to shared modulesConfidence Score: 4/5
app/notifications/page.tsxhas a bug in error handling for mark-all-read.components/settings/OrganizationSettings.tsxhas a minor stale closure concern in the toggle handler.Important Files Changed
Sequence Diagram
Last reviewed commit:
4a489459 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +1,231 @@'use client'Actual API error is discarded
The catch block creates a
new Error('Failed to mark all as read')instead of passing the caught error togetAuthErrorMessage. SincegetAuthErrorMessagechecks for.statuson the error to return the appropriate message, this new plainErrorwill always yield'Something went wrong, please try again.'regardless of the actual failure reason (e.g. 401 session expired vs. 500 server error).Prompt To Fix With AI
Stale closure in optimistic rollback
The
.catchrollback on line 1024 capturesnotificationSettingsfrom the closure at the time of the click. If a user rapidly toggles two different categories (theisSavingNotificationSettingsflag disables buttons, but there's still a brief race window betweensetIsSavingNotificationSettings(true)and the React re-render that disables the buttons), the rollback could restore incorrect state.Consider capturing the previous state explicitly before the optimistic update:
Prompt To Fix With AI
Issue: The .catch rollback used notificationSettings from the closure, which can be stale if the user toggles quickly before React re-renders, so the rollback could restore the wrong state.
Fix: Store the previous state before the optimistic update: const prev = { ...notificationSettings } and roll back with setNotificationSettings(prev) in the .catch handler.
Why: The closure captures whatever notificationSettings was at click time. On rapid toggles, the catch handler can see an older value, so the rollback must use the exact state from when the update was started.
@@ -0,0 +1,231 @@'use client'Issue: The catch block was creating new Error('Failed to mark all as read') and passing it to getAuthErrorMessage, so the original API error was lost and users always saw a generic message even for auth-specific failures (e.g. 401).
Fix: Use the caught error: catch (err) and getAuthErrorMessage(err as Error) || 'Failed to mark all as read' so auth-related errors surface correctly and other errors still show a fallback message.
Why: getAuthErrorMessage relies on the real error (e.g. .status) to choose the message. Without the actual error, auth failures like expired sessions cannot be distinguished from generic server errors.
Greptile Summary
This PR adds a complete in-app notification system: a bell icon dropdown in the header (
NotificationCenter), a full/notificationspage with pagination, and a Notifications settings tab in organization settings for owners/admins to toggle billing and uptime notification categories./notifications) lists all notifications with load-more pagination, mark-read on click, and mark-all-read with toast feedback.lib/api/notifications.ts,lib/api/notification-settings.ts) follow existingapiRequestpatterns.0.6.0-alpha,@ciphera-net/uiupdated to^0.0.50,@radix-ui/react-iconsadded explicitly, TypeScript pinned to exact5.9.3.Issues found:
loadNotificationSettings, causing a wasted 403 API call and brief error toast for members who navigate to?tab=notificationsdirectly.Confidence Score: 4/5
components/settings/OrganizationSettings.tsx— the member redirect race condition on the notifications tab.Important Files Changed
Sequence Diagram
Last reviewed commit:
56b99df9 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +43,4 @@if (diffHours < 24) return `${diffHours}h ago`if (diffDays < 7) return `${diffDays}d ago`return d.toLocaleDateString()}Duplicated
formatTimeAgoandgetTypeIconutilitiesformatTimeAgoandgetTypeIconare identical copies between this file andapp/notifications/page.tsx. Consider extracting them into a shared utility (e.g.,lib/utils/notifications.ts) to avoid maintaining the same logic in two places.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
API call fires before member redirect
When a member navigates to
/org-settings?tab=notifications, two effects run: the one on line 283 firesloadNotificationSettings()(which will fail with 403 for members), and the redirect on line 290 switches the tab to'general'. Because React batches state updates but fires effects in order, the API call executes before the redirect, resulting in a brief loading spinner and a toast error ("Session expired" or similar) before the tab switches away.Consider guarding the load effect with a role check to avoid the wasted call:
Prompt To Fix With AI
@@ -0,0 +43,4 @@if (diffHours < 24) return `${diffHours}h ago`if (diffDays < 7) return `${diffDays}d ago`return d.toLocaleDateString()}Issue: formatTimeAgo and getTypeIcon were duplicated in NotificationCenter.tsx and app/notifications/page.tsx, so both need updates when the behavior changes.
Fix: Moved both helpers to lib/utils/notifications.tsx and switched both consumers to import from the shared utility.
Why: Centralizes logic in one place so future changes are consistent across the notification center and notifications page.
Issue: For members on /org-settings?tab=notifications, the load effect calls loadNotificationSettings() before the redirect effect runs, causing a 403, loading spinner, and error toast before the tab switches.
Fix: Guard the load effect with user?.role !== 'member' so it only runs for owners/admins. The redirect effect still runs when user?.role === 'member' and switches the tab.
Why: Avoids the unnecessary 403 request and error toast, and reduces wasted API calls for members.