[Dashboard] Add notifications system with unread tracking #7302
+915
−475
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
## Walkthrough
A new notification system was introduced, replacing the previous implementation. This includes a new API module, React components for notification display, and a React hook for managing notification state. The old notification button components and related logic were removed from team and account headers, streamlining notification handling to use the new system based on account ID.
## Changes
| File(s) / Path(s) | Change Summary |
|--------------------------------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------|
| apps/dashboard/src/@/api/notifications.ts | Added new API module for notifications and product updates, with typed functions and error handling. |
| apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx | Added new `NotificationsButton` React component for displaying notification bell and popover. |
| apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx | Added new `NotificationEntry` component for rendering individual notifications. |
| apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx | Added new `NotificationList` component with tabbed display for inbox, archive, and product updates. |
| apps/dashboard/src/@/components/blocks/notifications/state/manager.ts | Added new `useNotifications` hook for managing notification state, fetching, mutations, and local storage. |
| apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx | Removed notification fetching/marking imports and props. |
| apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx | Removed notification-related props from stories. |
| apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx | Replaced old notification button with new component; removed related props and types. |
| apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx | Replaced old notification button with new component; removed related props and types. |
| apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx | **Deleted**: Storybook stories for old notification button. |
| apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx | **Deleted**: Old notification button UI and types. |
| apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts | **Deleted**: Old notification fetching and marking logic using IndexedDB. |
| apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx | Removed notification-related stub props from stories. |
| apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx | Replaced old notification button with new component; removed related props and types. |
| apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx | Removed notification fetching/marking imports and props. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant NotificationsButton
participant useNotifications (Hook)
participant Notifications API
participant Product Updates API
participant IndexedDB
User->>NotificationsButton: Clicks bell icon
NotificationsButton->>useNotifications: Fetch unread, archived, updates (by accountId)
useNotifications->>Notifications API: getUnreadNotifications / getArchivedNotifications / getUnreadNotificationsCount
useNotifications->>Product Updates API: fetchProductUpdates
useNotifications->>IndexedDB: get/set read product update IDs
Notifications API-->>useNotifications: Return notifications data
Product Updates API-->>useNotifications: Return updates data
IndexedDB-->>useNotifications: Return read status
useNotifications-->>NotificationsButton: Provide notification state
NotificationsButton->>User: Display popover with notification list
User->>NotificationsButton: Mark as read
NotificationsButton->>useNotifications: markNotificationAsRead
useNotifications->>Notifications API: markNotificationAsRead
Notifications API-->>useNotifications: Confirm update
useNotifications-->>NotificationsButton: Update notification stateSuggested reviewers
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
jnsdls
added
the
DO NOT MERGE
Graphite App
![cursor-com[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7302 +/- ##
=======================================
Coverage 55.57% 55.57%
=======================================
Files 909 909
Lines 58673 58673
Branches 4158 4158
=======================================
Hits 32607 32607
Misses 25959 25959
Partials 107 107
🚀 New features to boost your workflow:
|
![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 (8)
apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx (1)
22-23: Improve the implementation comment and consider refactoring.The comment "kinda jank but it works" suggests the unread count logic could be improved. Consider either:
- Improving the implementation to be more robust
- Adding a more descriptive comment explaining why this approach is necessary
- {/* kinda jank but it works: always take the last page and check the unread count of IT */} + {/* Display notification indicator when there are unread notifications */}apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx (1)
45-53: Consider adding accessibility improvements to the mark as read button.The icon-only button lacks an accessible label for screen readers.
<Button variant="ghost" size="icon" onClick={() => onMarkAsRead(notification.id)} className="text-muted-foreground hover:text-foreground" + aria-label="Mark notification as read" > <Trash2Icon className="h-4 w-4" /> </Button>apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
59-60: Consider making the max height configurable.The hardcoded max height of 600px might not be ideal for all screen sizes. Consider making this configurable or responsive.
- <div className="max-h-[600px] w-full overflow-y-auto"> + <div className="max-h-[min(600px,80vh)] w-full overflow-y-auto">apps/dashboard/src/@/components/blocks/notifications/state/manager.ts (3)
78-79: Improve type safety for pageParam.The type assertion pattern used for
pageParamcould be simplified and made more type-safe.- const cursor = (pageParam ?? undefined) as string | undefined; + const cursor = pageParam as string | undefined;Since
pageParamis already typed throughinitialPageParam, the nullish coalescing and type assertion are redundant.Also applies to: 93-94
135-135: Extract magic numbers into named constants.The stale time values should be extracted into named constants for better maintainability.
At the top of the file, add:
+const STALE_TIME = { + UNREAD_COUNT: 60_000, // 1 minute + PRODUCT_UPDATES: 60_000 * 15, // 15 minutes +} as const;Then update the usages:
- staleTime: 60_000, // 1min + staleTime: STALE_TIME.UNREAD_COUNT,- staleTime: 60_000 * 15, // 15min + staleTime: STALE_TIME.PRODUCT_UPDATES,
127-129: Consider using a more robust error handling approach.Using
console.errorin production code might not be ideal. Consider using a proper error tracking service or at least a more centralized error handling approach.- cleanupProductUpdates(res.data).catch((err) => { - console.error("Failed to cleanup product updates", err); - }); + cleanupProductUpdates(res.data).catch((err) => { + // Log to error tracking service if available + if (process.env.NODE_ENV === "development") { + console.error("Failed to cleanup product updates", err); + } + });apps/dashboard/src/@/api/notifications.ts (2)
128-144: Clarify the dual behavior of markNotificationAsRead function.The function's behavior changes significantly based on whether
notificationIdis provided, but this isn't clear from the function signature alone.Consider splitting into two separate functions for clarity:
-export async function markNotificationAsRead(notificationId?: string) { +export async function markNotificationAsRead(notificationId: string) { + const authToken = await getAuthToken(); + if (!authToken) { + throw new Error("No auth token found"); + } + const url = new URL( + "/v1/dashboard-notifications/mark-as-read", + NEXT_PUBLIC_THIRDWEB_API_HOST, + ); + const response = await fetch(url, { + method: "PUT", + headers: { + Authorization: `Bearer ${authToken}`, + "Content-Type": "application/json", + }, + body: JSON.stringify({ notificationId }), + }); + // ... rest of function +} + +export async function markAllNotificationsAsRead() { const authToken = await getAuthToken(); if (!authToken) { throw new Error("No auth token found"); } const url = new URL( "/v1/dashboard-notifications/mark-as-read", NEXT_PUBLIC_THIRDWEB_API_HOST, ); const response = await fetch(url, { method: "PUT", headers: { Authorization: `Bearer ${authToken}`, "Content-Type": "application/json", }, - // if notificationId is provided, mark it as read, otherwise mark all as read - body: JSON.stringify(notificationId ? { notificationId } : {}), + body: JSON.stringify({}), }); - if (!response.ok) { - const body = await response.text(); - return { - status: "error", - reason: "unknown", - body, - } as const; - } - return { - status: "success", - } as const; + // ... rest of function }
41-47: Enhance error handling with more specific error types.All functions return generic "unknown" error reasons. Consider providing more specific error categorization based on HTTP status codes.
- return { - status: "error", - reason: "unknown", - body, - } as const; + let reason: string; + switch (response.status) { + case 401: + reason = "unauthorized"; + break; + case 403: + reason = "forbidden"; + break; + case 404: + reason = "not_found"; + break; + case 429: + reason = "rate_limited"; + break; + case 500: + reason = "server_error"; + break; + default: + reason = "unknown"; + } + return { + status: "error", + reason, + body, + statusCode: response.status, + } as const;Also applies to: 77-83, 109-115, 146-152, 179-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/src/@/api/notifications.ts(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/state/manager.ts(1 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx (1)
apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx (1)
NotificationsButton(14-36)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (3)
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts (1)
useNotifications(52-269)apps/dashboard/src/@/components/ui/tabs.tsx (1)
TabButtons(85-159)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx (1)
NotificationEntry(13-57)
apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx (1)
apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx (1)
NotificationsButton(14-36)
apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx (2)
apps/dashboard/src/@/api/notifications.ts (1)
Notification(7-16)apps/dashboard/src/@/components/ui/button.tsx (1)
Button(85-85)
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts (1)
apps/dashboard/src/@/api/notifications.ts (8)
Notification(7-16)NotificationsApiResponse(18-21)getUnreadNotifications(23-56)getArchivedNotifications(58-92)getUnreadNotificationsCount(94-126)fetchProductUpdates(170-191)markNotificationAsRead(128-157)ProductUpdateItem(163-168)
apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx (1)
apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx (1)
NotificationsButton(14-36)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (9)
apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx (1)
13-13: LGTM! Notification props properly removed.The removal of notification-related imports and props from the team header aligns with the centralized notification system refactor.
apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx (1)
14-36: Well-designed component with simplified props interface.The new
NotificationsButtoncomponent successfully centralizes notification state management through theuseNotificationshook and simplifies the component interface to just require anaccountId.apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx (2)
5-5: Correct import update for new notification system.The import change from the old notification component to the new
NotificationsButtonaligns with the refactor.
20-20: Simplified component usage improves maintainability.The new
NotificationsButtononly requires theaccountIdprop, eliminating the need for async callback functions and simplifying the component interface.apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx (2)
8-8: Correct import for new notification system.Import of the new
NotificationsButtoncomponent is consistent with the refactor pattern.
119-119:✅ Verification successful
Verify notification coverage across UI variants.
The
NotificationsButtonis added to the mobile UI. Ensure that the desktop UI also has notification access (likely through theSecondaryNavcomponent).
🏁 Script executed:
#!/bin/bash # Verify that both mobile and desktop UIs have notification functionality echo "Checking desktop UI notification coverage..." rg -A 10 -B 5 "SecondaryNav" apps/dashboard/src/app/\(app\)/account/components/AccountHeaderUI.tsx echo "Checking SecondaryNav includes notifications..." rg -A 5 -B 5 "NotificationsButton" apps/dashboard/src/app/\(app\)/components/Header/SecondaryNav/SecondaryNav.tsxLength of output: 2496
Notification coverage verified across UI variants
– In AccountHeaderUI.tsx (mobile view):
<NotificationsButton accountId={props.account.id} />is present.
– In SecondaryNav.tsx (desktop view):<NotificationsButton accountId={props.account.id} />is already rendered.No further changes needed.
apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx (1)
9-9: Clean implementation of the new notifications system.The replacement of the old notification button with the new
NotificationsButtoncomponent simplifies the props interface by requiring only theaccountId. The import path correctly uses the alias pattern.Also applies to: 198-198
apps/dashboard/src/@/api/notifications.ts (2)
1-6: LGTM! Server-side setup is correctly configured.The file properly uses server-only directives and imports the necessary dependencies for server-side API operations.
7-22: Well-defined type structures.The type definitions are comprehensive and properly structured for the notification system. The optional
nextCursorinNotificationsApiResponsecorrectly supports pagination.
apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts
Outdated
Show resolved
Hide resolved
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
59b6357 to
fa3c433
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
fa3c433 to
0ac15b4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (3)
90-124: Missing pagination UI for loading more notifications.The component receives pagination props (
hasMoreUnread,loadMoreUnread,isFetchingMoreUnread) but doesn't render any UI elements to allow users to load more notifications.
126-159: Missing pagination UI for loading more archived notifications.Similar to InboxTab, this component receives pagination props (
hasMoreArchived,loadMoreArchived,isFetchingMoreArchived) but doesn't provide any UI for users to load more content.
183-193: Hardcoded accountId and teamId values still present.The hardcoded
"1"values foraccountIdandteamIdremain in the notification object transformation, which was previously flagged as problematic.
🧹 Nitpick comments (2)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (2)
11-17: Consider dynamic tab switching for better UX.The active tab is only determined during component initialization. If unread counts change dynamically (e.g., new notifications arrive), users won't be automatically redirected to the relevant tab to see them.
Consider implementing logic to automatically switch to the "inbox" tab when new unread notifications arrive, or at minimum provide visual cues when other tabs have new content.
104-121: Consider showing loading states for better UX.All tab components receive loading state props (
isLoadingUnread,isLoadingArchived,isLoadingUpdates) but don't render any loading indicators. Users might see empty states or stale data while new content is being fetched.Consider adding skeleton loaders or spinner components when
isLoading*props aretrue.Also applies to: 138-156, 169-197
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/src/@/api/notifications.ts(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/state/manager.ts(1 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx
- apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx
- apps/dashboard/src/@/components/blocks/notifications/state/manager.ts
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx
- apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx
- apps/dashboard/src/@/api/notifications.ts
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Unit Tests
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
0ac15b4 to
f3ffc0a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
197-198:⚠️ Potential issueRemove hardcoded accountId and teamId values.
The hardcoded "1" values are still present and should be addressed as previously mentioned.
notification={{ id: update.id, description: update.title, createdAt: update.published_at, - accountId: "1", - teamId: "1", + accountId: "", // These fields are not used for product updates + teamId: null, // we "cheat" here and use the published_at as the readAt if it "is read" readAt: update.isRead ? update.published_at : null, ctaText: "Read more", ctaUrl: update.url, }}
🧹 Nitpick comments (1)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
227-227: Remove console.log from production code.Debug logging should be removed before deployment.
- console.log("scrolled into view, loading more");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/src/@/api/notifications.ts(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/state/manager.ts(1 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx
- apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx
- apps/dashboard/src/@/components/blocks/notifications/state/manager.ts
- apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx
- apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx
- apps/dashboard/src/@/api/notifications.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (2)
9-17: LGTM! Smart tab selection logic.The component intelligently defaults to the tab with unread content, providing a good user experience by prioritizing relevant notifications.
90-129: Pagination implementation looks good.The AutoLoadMore component addresses the previous concern about missing pagination UI, providing a smooth infinite scroll experience.
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx
Outdated
Show resolved
Hide resolved
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
f3ffc0a to
ca8ffa3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
215-216: Address the unresolved hardcoded values.The hardcoded
"1"values foraccountIdandteamIdwere flagged in previous reviews but remain unaddressed. These should use actual values or be omitted if not needed.Apply this fix to remove the hardcoded values:
- accountId: "1", - teamId: "1", + accountId: "", // Not used for product updates + teamId: null,
🧹 Nitpick comments (1)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
248-248: Remove console.log statement.Debug logging should be removed from production code.
- console.log("scrolled into view, loading more");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/src/@/api/notifications.ts(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/state/manager.ts(1 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx
- apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx
- apps/dashboard/src/@/components/blocks/notifications/state/manager.ts
- apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx
- apps/dashboard/src/@/api/notifications.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Size
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Lint Packages
- GitHub Check: Build Packages
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (2)
14-22: Well-designed tab initialization logic.The tab selection logic properly prioritizes unread content (inbox → updates → archive), providing a good user experience by showing the most relevant content first.
241-266: AutoLoadMore implementation looks robust.The IntersectionObserver implementation now includes proper guards and cleanup:
- Checks
isLoadingandhasMorebefore triggering- Uses
unobserve()to prevent duplicate fires- Includes proper cleanup in the effect return
- Correctly scoped to the scroll container
This addresses the performance concerns from previous reviews.
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
ca8ffa3 to
afe40cc
Compare
jnsdls
force-pushed
the
_Dashboard_Add_notifications_system_with_unread_tracking
branch
from
afe40cc to
d1e0c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts (3)
129-131: Consider extracting side effects from query function.The cleanup operation inside the query function is an anti-pattern as query functions should be pure. Consider moving this to a separate effect or mutation.
- // as a side effect, clean up the read notifications - cleanupProductUpdates(res.data).catch((err) => { - console.error("Failed to cleanup product updates", err); - });Move the cleanup to a separate effect:
+// Add this effect after the query definition +useEffect(() => { + if (updatesQuery.isSuccess && updatesQuery.data) { + const currentUpdates = updatesQuery.data.map(d => ({ ...d, isRead: false })); + cleanupProductUpdates(currentUpdates).catch((err) => { + console.error("Failed to cleanup product updates", err); + }); + } +}, [updatesQuery.isSuccess, updatesQuery.data]);
277-277: Track the temporary IndexedDB implementation.The comment indicates this is a temporary solution that will be replaced by a proper API. Consider creating a tracking issue to ensure this technical debt is addressed.
Would you like me to generate a GitHub issue template to track the replacement of the IndexedDB implementation with a proper API?
88-88: Consider making refetch intervals configurable.The hardcoded 1-minute refetch intervals might not be optimal for all use cases and could impact performance or user experience.
+interface UseNotificationsOptions { + refetchInterval?: number; +} + -export function useNotifications(accountId: string) { +export function useNotifications( + accountId: string, + options: UseNotificationsOptions = {} +) { + const { refetchInterval = 60_000 } = options;Then use
refetchIntervalin the query configurations instead of the hardcoded values.Also applies to: 104-104, 116-116
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (1)
14-14: Consider more specific prop typing for better maintainability.Using the entire return type of
useNotificationsmakes the component tightly coupled and brittle to changes in the hook interface.+interface NotificationListProps { + unreadNotifications: Notification[]; + archivedNotifications: Notification[]; + updates: (ProductUpdateItem & { isRead: boolean })[]; + totalUnreadCount: number; + unreadUpdatesCount: number; + unreadNotificationsCount: number; + isLoadingUnread: boolean; + isLoadingArchived: boolean; + isLoadingUpdates: boolean; + hasMoreUnread: boolean; + hasMoreArchived: boolean; + isFetchingMoreUnread: boolean; + isFetchingMoreArchived: boolean; + loadMoreUnread: () => void; + loadMoreArchived: () => void; + markAsRead: (id: string) => void; + markAllAsRead: () => void; + markUpdateAsRead: (id: string) => void; +} + -export function NotificationList(props: ReturnType<typeof useNotifications>) { +export function NotificationList(props: NotificationListProps) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/src/@/api/notifications.ts(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx(1 hunks)apps/dashboard/src/@/components/blocks/notifications/state/manager.ts(1 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx(0 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx(2 hunks)apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx(1 hunks)
💤 Files with no reviewable changes (6)
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeader.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.stories.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/NotificationButton.tsx
- apps/dashboard/src/app/(app)/team/components/NotificationButton/fetch-notifications.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/dashboard/src/app/(app)/team/components/TeamHeader/team-header-logged-in.client.tsx
- apps/dashboard/src/app/(app)/team/components/TeamHeader/TeamHeaderUI.tsx
- apps/dashboard/src/app/(app)/account/components/AccountHeaderUI.tsx
- apps/dashboard/src/app/(app)/components/Header/SecondaryNav/SecondaryNav.tsx
- apps/dashboard/src/@/components/blocks/notifications/notification-button.tsx
- apps/dashboard/src/@/components/blocks/notifications/notification-entry.tsx
- apps/dashboard/src/@/api/notifications.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/dashboard/src/@/components/blocks/notifications/state/manager.ts (1)
apps/dashboard/src/@/api/notifications.ts (8)
Notification(7-16)NotificationsApiResponse(18-21)getUnreadNotifications(23-56)getArchivedNotifications(58-92)getUnreadNotificationsCount(94-126)fetchProductUpdates(170-191)markNotificationAsRead(128-157)ProductUpdateItem(163-168)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Size
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx (2)
245-245: Good implementation of intersection observer optimization.The
observer.unobserve(entry.target)call prevents duplicate trigger events, which is a solid optimization for the infinite scrolling behavior.
16-22: Well-designed initial tab selection logic.The priority-based tab selection (Inbox → Updates → Archive) based on unread counts provides a good user experience by showing the most relevant content first.
| * ``` | ||
| */ | ||
|
|
||
| export function useNotifications(accountId: string) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for accountId parameter.
The hook doesn't validate the accountId parameter, which could lead to unexpected behavior if called with invalid values.
-export function useNotifications(accountId: string) {
+export function useNotifications(accountId: string) {
+ if (!accountId || typeof accountId !== 'string') {
+ throw new Error('useNotifications requires a valid accountId');
+ }📝 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
| export function useNotifications(accountId: string) { | |
| export function useNotifications(accountId: string) { | |
| if (!accountId || typeof accountId !== 'string') { | |
| throw new Error('useNotifications requires a valid accountId'); | |
| } | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In apps/dashboard/src/@/components/blocks/notifications/state/manager.ts at line
52, the useNotifications hook lacks validation for the accountId parameter. Add
input validation to check if accountId is a non-empty string and valid before
proceeding. If the validation fails, handle it appropriately by either returning
early, throwing an error, or providing a fallback to prevent unexpected
behavior.
Comment on lines
+211
to
+217
| notification={{ | ||
| id: update.id, | ||
| description: update.title, | ||
| createdAt: update.published_at, | ||
| ctaText: "Read more", | ||
| ctaUrl: update.url, | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety for product update transformation.
The notification object creation for product updates lacks some required fields and could benefit from better type safety.
notification={{
id: update.id,
description: update.title,
createdAt: update.published_at,
+ accountId: "", // Not applicable for product updates
+ teamId: null,
+ readAt: update.isRead ? update.published_at : null,
ctaText: "Read more",
ctaUrl: update.url,
- }}
+ } satisfies Partial<Notification>}This ensures the object matches the expected Notification interface while being explicit about which fields are not applicable.
📝 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
| notification={{ | |
| id: update.id, | |
| description: update.title, | |
| createdAt: update.published_at, | |
| ctaText: "Read more", | |
| ctaUrl: update.url, | |
| }} | |
| notification={{ | |
| id: update.id, | |
| description: update.title, | |
| createdAt: update.published_at, | |
| accountId: "", // Not applicable for product updates | |
| teamId: null, | |
| readAt: update.isRead ? update.published_at : null, | |
| ctaText: "Read more", | |
| ctaUrl: update.url, | |
| } satisfies Partial<Notification>} |
🤖 Prompt for AI Agents
In apps/dashboard/src/@/components/blocks/notifications/notification-list.tsx
around lines 211 to 217, the notification object for product updates is missing
some required fields and lacks explicit typing. Update the object to include all
required fields from the Notification interface, explicitly setting any
non-applicable fields to null or appropriate defaults. This will improve type
safety and ensure the object fully conforms to the expected interface.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 by CodeRabbit
New Features
Refactor
Chores
PR-Codex overview
This PR focuses on removing the
NotificationButtoncomponent and its associated files, while integrating a newNotificationsButtoncomponent throughout the application for handling notifications.Detailed summary
NotificationButton.tsx,fetch-notifications.ts, andNotificationButton.stories.tsx.NotificationsButtoncomponent in various files.TeamHeaderandAccountHeadercomponents to useNotificationsButton.getInboxNotificationsandmarkNotificationAsReadfunctions from multiple components.SecondaryNav,TeamHeader, andAccountHeadercomponents.