feat(notifications): implement /me/notifications with global unread sync#401
feat(notifications): implement /me/notifications with global unread sync#401jaydendancer12 wants to merge 1 commit intoboundlessfi:mainfrom
Conversation
|
@jaydendancer12 is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe pull request migrates the notifications system to a centralized provider-based architecture. The legacy notifications page at Changes
Sequence DiagramsequenceDiagram
participant User
participant NotificationProvider
participant AuthClient
participant useNotifications Hook
participant WebSocket
participant NotificationPolling
participant UI
User->>NotificationProvider: App loads
NotificationProvider->>AuthClient: useSession()
AuthClient-->>NotificationProvider: userId
NotificationProvider->>useNotifications Hook: initialize(userId, limit=50, autoFetch=true)
NotificationProvider->>NotificationPolling: start polling(interval=30s)
NotificationProvider-->>UI: expose via useNotificationCenter context
loop Every 30 seconds
NotificationPolling->>useNotifications Hook: refetch()
useNotifications Hook->>WebSocket: fetch notifications
WebSocket-->>useNotifications Hook: notifications list
useNotifications Hook->>UI: update context
end
User->>UI: Opens /me/notifications
UI->>UI: consume useNotificationCenter
UI->>UI: group notifications (New/Earlier/Archived)
UI-->>User: render grouped list
User->>UI: Click notification
UI->>useNotifications Hook: markNotificationAsRead(id)
useNotifications Hook->>WebSocket: mark read API call
useNotifications Hook->>UI: update unread count
UI-->>User: show notification details in sheet
User->>UI: Click "Mark all as read"
UI->>useNotifications Hook: markAllAsRead()
useNotifications Hook->>WebSocket: bulk mark read API call
useNotifications Hook->>UI: reset unread count
UI-->>User: update display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
hooks/useNotifications.ts (2)
51-73:⚠️ Potential issue | 🟠 Major
unreadCountis never recalibrated from fetched data; badge drifts after polling.
fetchNotificationsupdatesnotificationsandtotalbut notunreadCount. After a 30 s poll that brings back notifications marked read on another device/session, the notifications list will correctly show no unread indicator whileunreadCount(and therefore the sidebar badge) retains the stale count. Add a recalibration step from the fetched payload:🔧 Proposed fix
setNotifications(sorted); setTotal(response.total || 0); + // Recalibrate unread count from the latest server snapshot. + // Prefer an explicit field from the response if the API exposes it; + // fall back to counting from the returned notifications array. + if (typeof response.unreadCount === 'number') { + setUnreadCount(response.unreadCount); + } else { + setUnreadCount(sorted.filter(n => !n.read).length); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useNotifications.ts` around lines 51 - 73, fetchNotifications currently updates notifications and total but never updates the unreadCount, causing the badge to drift; after you setNotifications(sorted) (inside fetchNotifications in the useNotifications hook) compute the fresh unread count from response.notifications (e.g., count items with read === false / !item.read) and call setUnreadCount with that value so the badge is recalibrated to the fetched payload; ensure this recalculation runs every time fetchNotifications successfully receives response.notifications.
163-207:⚠️ Potential issue | 🟠 Major
markAsRead/markNotificationAsReadare socket-only; marks are silently lost when disconnected.Both functions optimistically update local state and then emit
mark-readover the socket. There is no REST API fallback. WhenisConnectedisfalse(socket not yet connected, network interruption, orautoConnect=false), the optimistic UI update is applied locally but the change is never persisted on the server. The next poll will revert the notification to unread.The API module (
lib/api/notifications) exposes REST endpoints for marking notifications. Call those as the primary path (or as a fallback), and reserve the socket emit for the real-time push to other sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useNotifications.ts` around lines 163 - 207, markAsRead and markNotificationAsRead currently only emit socket events so optimistic updates are lost when disconnected; call the REST endpoint in lib/api/notifications to persist changes and use socket.emit('mark-read') only to broadcast if connected. Update both functions (markAsRead, markNotificationAsRead) to: perform the optimistic setNotifications/setUnreadCount update, then call the API (e.g., notificationsApi.markRead or similar) to persist the id(s); if the API call succeeds and socket && isConnected then emit the socket event; if the API call fails, revert the optimistic state (or handle retry/error) and surface the error. Ensure you reference and use the existing setNotifications and setUnreadCount logic and keep socket.emit as a best-effort broadcast only.components/notifications/NotificationBell.tsx (1)
14-19:⚠️ Potential issue | 🟡 Minor
limitprop is declared but never used
limit?: numberis part of the public interface but is never destructured from props, never referenced in the component body, and has no effect. Either wire it up to the underlying data fetch or remove it to avoid a misleading API surface.🛠️ Proposed fix
interface NotificationBellProps { className?: string; - limit?: number; } -export const NotificationBell = ({ className }: NotificationBellProps) => { +export const NotificationBell = ({ className, limit }: NotificationBellProps) => {Pick one: remove the prop, or destructure and pass it to the underlying notification call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/notifications/NotificationBell.tsx` around lines 14 - 19, NotificationBellProps declares limit?: number but the NotificationBell component never reads or uses it; either remove limit from the public interface and the component signature, or destructure it from the props in the NotificationBell function and thread it into the notification retrieval call (e.g., pass limit into whatever hook or function you use to fetch notifications such as useNotifications/fetchNotifications or the underlying notifications API call used in the component). Update NotificationBellProps and the NotificationBell parameter list consistently so the prop is not misleading.
🧹 Nitpick comments (8)
app/(landing)/notifications/page.tsx (2)
1-4: UsepermanentRedirectfor a URL that has permanently moved.
redirect()issues a 307 (temporary) redirect in Next.js 15 page components. Since/notificationsis being replaced by/me/notificationsindefinitely, a 308 permanent redirect is semantically correct and ensures crawlers/browsers update cached URLs.🔧 Suggested change
-import { redirect } from 'next/navigation'; +import { permanentRedirect } from 'next/navigation'; export default function LegacyNotificationsPage() { - redirect('/me/notifications'); + permanentRedirect('/me/notifications'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(landing)/notifications/page.tsx around lines 1 - 4, The page currently calls redirect('/me/notifications') which issues a 307 temporary redirect; update LegacyNotificationsPage to call permanentRedirect('/me/notifications') instead (and update the import from next/navigation if needed) so the route uses a 308 permanent redirect; locate the LegacyNotificationsPage function in this file and replace redirect with permanentRedirect to reflect the URL move.
3-4: Function declaration; preferconstarrow for consistency with guidelines.🔧 Suggested change
-export default function LegacyNotificationsPage() { - redirect('/me/notifications'); -} +const LegacyNotificationsPage = () => { + redirect('/me/notifications'); +}; + +export default LegacyNotificationsPage;As per coding guidelines: "Prefer const arrow functions with explicit type annotations over function declarations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`(landing)/notifications/page.tsx around lines 3 - 4, Change the function declaration LegacyNotificationsPage to a const arrow component with an explicit type annotation (e.g., const LegacyNotificationsPage: React.FC or NextPage) and keep the redirect('/me/notifications') call inside its body; export the const as default. Ensure you update the declaration from "export default function LegacyNotificationsPage() { ... }" to a named const arrow (const LegacyNotificationsPage: React.FC = () => { redirect('/me/notifications'); }) and then export default LegacyNotificationsPage.hooks/useNotifications.ts (1)
29-31: Function declaration; preferconstarrow function.🔧 Suggested change
-export function useNotifications( - input?: string | UseNotificationsOptions -): UseNotificationsReturn { +export const useNotifications = ( + input?: string | UseNotificationsOptions +): UseNotificationsReturn => {As per coding guidelines: "Prefer const arrow functions with explicit type annotations over function declarations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useNotifications.ts` around lines 29 - 31, Change the function declaration "export function useNotifications(input?: string | UseNotificationsOptions): UseNotificationsReturn" to a const arrow function using an explicit type annotation, e.g. export const useNotifications: (input?: string | UseNotificationsOptions) => UseNotificationsReturn = (input) => { ... }; ensure the exported symbol remains named useNotifications and all existing internal logic and references to UseNotificationsOptions and UseNotificationsReturn are preserved.components/providers/notification-provider.tsx (2)
28-28:useMemo(() => notificationsHook, [notificationsHook])provides no memoization.
useNotificationsreturns a new plain object literal on every render, sonotificationsHookalways has a new reference, causinguseMemoto recompute unconditionally. Every render ofNotificationProviderpropagates a new context value and re-renders alluseNotificationCenterconsumers — including the sidebar badge,NotificationBell, and the full notifications page.The correct fix is to extract the individual stable values as memo dependencies, or to memoize the returned object inside
useNotifications:🔧 Suggested change (provider side)
- const value = useMemo(() => notificationsHook, [notificationsHook]); + const { + notifications, + unreadCount, + isConnected, + loading, + error, + total, + currentPage, + setCurrentPage, + markAsRead, + markAllAsRead, + markNotificationAsRead, + fetchNotifications, + refetch, + } = notificationsHook; + + const value = useMemo( + () => ({ + notifications, + unreadCount, + isConnected, + loading, + error, + total, + currentPage, + setCurrentPage, + markAsRead, + markAllAsRead, + markNotificationAsRead, + fetchNotifications, + refetch, + }), + // Stable function refs (useCallback/setState) don't change; include data values only. + // eslint-disable-next-line react-hooks/exhaustive-deps + [notifications, unreadCount, isConnected, loading, error, total, currentPage] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/providers/notification-provider.tsx` at line 28, The context value is being recreated every render because useMemo(() => notificationsHook, [notificationsHook]) simply forwards a new object reference from useNotifications; update NotificationProvider to pass a stable value by either (A) destructuring stable primitives/functions out of notificationsHook (e.g., items, unreadCount, fetchNotifications, markRead) and memoizing an object built from those specific values in the useMemo dependencies, or (B) move the memoization into useNotifications so it returns a stable object (memoize the return of useNotifications) — change references to notificationsHook and useMemo accordingly so consumers like useNotificationCenter and NotificationBell receive a stable context value.
13-13: Function declarations; preferconstarrow functions.🔧 Suggested change
-export function NotificationProvider({ children }: { children: ReactNode }) { +export const NotificationProvider = ({ children }: { children: ReactNode }) => { -export function useNotificationCenter(): UseNotificationsReturn { +export const useNotificationCenter = (): UseNotificationsReturn => {As per coding guidelines: "Prefer const arrow functions with explicit type annotations over function declarations."
Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/providers/notification-provider.tsx` at line 13, Replace function declarations with const arrow functions and explicit types: change export function NotificationProvider({ children }: { children: ReactNode }) { ... } to an exported const arrow with an explicit React type (e.g., export const NotificationProvider: React.FC<{ children: ReactNode }> = ({ children }) => { ... }), and do the same for the other function declaration in this file (the second function noted around line 37). Ensure exports, parameters and returned JSX remain unchanged, and adjust any internal references if needed.app/me/notifications/page.tsx (1)
30-30: Function declaration; preferconstarrow function.🔧 Suggested change
-export default function MeNotificationsPage() { +const MeNotificationsPage = () => { ... -} +}; + +export default MeNotificationsPage;As per coding guidelines: "Prefer const arrow functions with explicit type annotations over function declarations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/me/notifications/page.tsx` at line 30, Replace the function declaration MeNotificationsPage with a const arrow function using an explicit React component type; change "export default function MeNotificationsPage() { ... }" to "const MeNotificationsPage: React.FC = () => { ... }" (or another explicit component type you use across the codebase) and export it as default, ensuring props/type annotations match existing usage and any top-level async/metadata exports are preserved.components/notifications/NotificationDropdown.tsx (1)
165-177:onMarkAsReadprop name no longer reflects its handler's behavior.After removing the auto-mark-as-read logic, the handler passed to
onMarkAsReadonNotificationItemonly performs navigation and callsonClose. Consider renaming the prop toonClickoronSelectto match the actual semantics, especially sinceNotificationItemnow also exposesdisableNavigationto decouple navigation from click handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/notifications/NotificationDropdown.tsx` around lines 165 - 177, Rename the misleading prop usage: replace the onMarkAsRead prop passed to NotificationItem with a semantically correct name (e.g., onClick or onSelect) wherever NotificationDropdown renders NotificationItem; update the handler reference (currently handleNotificationClick) to match the new prop name and ensure NotificationItem's disableNavigation and onClose behavior remain unchanged. Locate the NotificationItem JSX in NotificationDropdown (the block mapping groupNotifications) and change the prop key and any corresponding prop type/usage to the new name so it accurately reflects that the handler performs navigation and closes the menu rather than marking as read.components/app-sidebar.tsx (1)
105-109:String(unreadCount)conversion is redundant —badgenow acceptsnumberSince
NavMain.badgewas widened tostring | numberandhasBadgeusesNumber(item.badge) > 0, passing the raw number is equivalent and removes a needless round-trip through string conversion.♻️ Proposed simplification
const accountItems = navigationData.account.map(item => item.title === 'Notifications' - ? { ...item, badge: unreadCount > 0 ? String(unreadCount) : undefined } + ? { ...item, badge: unreadCount > 0 ? unreadCount : undefined } : item );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/app-sidebar.tsx` around lines 105 - 109, The current mapping in accountItems unnecessarily converts unreadCount to a string; update the conditional to assign the raw number instead (use unreadCount rather than String(unreadCount)) so badge receives a number directly; change the expression in the accountItems mapping that matches item.title === 'Notifications' to set badge: unreadCount > 0 ? unreadCount : undefined (keeping navigationData.account, accountItems, item.title 'Notifications', badge and unreadCount as the referenced symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/me/notifications/page.tsx`:
- Around line 31-39: The subtitle is using notifications.length (loaded page
size) instead of the server-side total; update the UI to use the total value
returned by useNotificationCenter (the total variable) when rendering counts
(e.g., the subtitle and the block around lines 97-104) and keep
notifications.length only for "loaded" indicators if you want to show loaded vs
total; adjust any text to read "{unreadCount} unread of {total}" (or
"{unreadCount} unread of {notifications.length} loaded of {total}") and ensure
you reference useNotificationCenter's total and unreadCount variables rather
than notifications.length for the overall total display.
- Line 83: The onClick handler for the retry Button calls refetch() which
returns a Promise<void> and currently leaves rejection unhandled; update the
Button's onClick to handle errors from refetch (e.g., call refetch().catch(err
=> { /* log or show error */ }) or make the handler async and use try/catch) so
any rejection is caught and reported (ensure you reference the Button's onClick
that invokes refetch()).
In `@components/nav-main.tsx`:
- Around line 46-49: The current guard for hasBadge (using Number(item.badge) >
0) silences non-numeric string badges like "new" by coercing to NaN; update the
logic in components/nav-main.tsx around hasBadge and item.badge: either tighten
the prop/type for item.badge to only number/numeric-string at the type level, or
handle both numeric and label badges at runtime by checking typeof item.badge
=== 'number' && item.badge > 0 for numeric badges, separately accepting
non-empty string badges (e.g., typeof item.badge === 'string' &&
item.badge.trim() !== '') and for numeric-strings use a safe parse (Number or
parseInt) plus isFinite/isNaN check to render counts; adjust any consumers/types
accordingly (functions/props referencing item.badge and hasBadge).
In `@components/notifications/NotificationBell.tsx`:
- Around line 33-35: The async handler handleMarkAllAsRead currently calls
markAllAsRead() without awaiting or handling rejections; update
handleMarkAllAsRead to await markAllAsRead() and handle errors (try/catch) or at
least return the promise so rejections are propagated; reference the
handleMarkAllAsRead function and the markAllAsRead call in NotificationBell.tsx
and either await within a try/catch to log/show the error or propagate the
promise to the caller.
In `@components/notifications/NotificationItem.tsx`:
- Around line 151-160: In NotificationItem, remove the hard-coded aria-label on
the button (aria-label={notification.title}) so the browser computes the
accessible name from the visible content (title, message, timestamp, amount)
instead of overriding it; locate the JSX return in the NotificationItem
component where the button with onClick={handleClick} is rendered and delete the
aria-label prop (or replace it with an aria-labelledby approach referencing IDs
inside the content if you need a custom accessible name).
- Line 136: The tailwind-style shadow value using rgba(var(--primary),0.5) in
NotificationItem (the className string containing
'shadow-[0_0_8px_rgba(var(--primary),0.5)]') is invalid; update the shadow to
use a valid color source—either switch to the modern rgb(from_var(--primary) r g
b / 0.5) syntax or introduce a separate CSS variable (e.g., --primary-rgb with
"R G B") and use rgba(var(--primary-rgb),0.5) in the shadow utility—so replace
the current shadow-[...] token in the className on NotificationItem with one of
these valid options.
In `@components/notifications/NotificationList.tsx`:
- Around line 137-149: NotificationList is forcing NotificationItem to never
navigate by hardcoding disableNavigation={true}; remove or set disableNavigation
based on a prop/state so NotificationItem can navigate (e.g., pass
disableNavigation={disableNavigation} or omit it), and ensure the existing
onMarkAsRead/onNotificationClick logic still runs (NotificationItem,
onMarkAsRead, onNotificationClick) so clicks both mark read and then navigate to
the target when navigation is enabled.
- Around line 125-153: The exit animations don't run because AnimatePresence's
direct child is the wrapper <div className='space-y-2'>; move that wrapper
outside (or replace it so) AnimatePresence's direct children are the motion.div
items so each motion.div (the elements rendering NotificationItem) can receive
exit/initial/animate props; update the JSX in NotificationList (where
AnimatePresence, motion.div and NotificationItem are used) so AnimatePresence
wraps the mapped motion.divs directly while preserving the spacing/layout (e.g.,
apply the 'space-y-2' class to a parent container or to the motion.div elements
themselves).
In `@hooks/use-notification-polling.ts`:
- Around line 22-24: The polling currently swallows all refetch errors via
.catch(() => {}) which loses diagnostics; update the setInterval callback (where
intervalRef.current is assigned) to catch and log the error (e.g., console.error
or console.warn) including context (like "notification polling refetch failed")
and the error object so failures from refetch() are visible, and keep the
existing behavior of not throwing from the interval handler.
---
Outside diff comments:
In `@components/notifications/NotificationBell.tsx`:
- Around line 14-19: NotificationBellProps declares limit?: number but the
NotificationBell component never reads or uses it; either remove limit from the
public interface and the component signature, or destructure it from the props
in the NotificationBell function and thread it into the notification retrieval
call (e.g., pass limit into whatever hook or function you use to fetch
notifications such as useNotifications/fetchNotifications or the underlying
notifications API call used in the component). Update NotificationBellProps and
the NotificationBell parameter list consistently so the prop is not misleading.
In `@hooks/useNotifications.ts`:
- Around line 51-73: fetchNotifications currently updates notifications and
total but never updates the unreadCount, causing the badge to drift; after you
setNotifications(sorted) (inside fetchNotifications in the useNotifications
hook) compute the fresh unread count from response.notifications (e.g., count
items with read === false / !item.read) and call setUnreadCount with that value
so the badge is recalibrated to the fetched payload; ensure this recalculation
runs every time fetchNotifications successfully receives response.notifications.
- Around line 163-207: markAsRead and markNotificationAsRead currently only emit
socket events so optimistic updates are lost when disconnected; call the REST
endpoint in lib/api/notifications to persist changes and use
socket.emit('mark-read') only to broadcast if connected. Update both functions
(markAsRead, markNotificationAsRead) to: perform the optimistic
setNotifications/setUnreadCount update, then call the API (e.g.,
notificationsApi.markRead or similar) to persist the id(s); if the API call
succeeds and socket && isConnected then emit the socket event; if the API call
fails, revert the optimistic state (or handle retry/error) and surface the
error. Ensure you reference and use the existing setNotifications and
setUnreadCount logic and keep socket.emit as a best-effort broadcast only.
---
Nitpick comments:
In `@app/`(landing)/notifications/page.tsx:
- Around line 1-4: The page currently calls redirect('/me/notifications') which
issues a 307 temporary redirect; update LegacyNotificationsPage to call
permanentRedirect('/me/notifications') instead (and update the import from
next/navigation if needed) so the route uses a 308 permanent redirect; locate
the LegacyNotificationsPage function in this file and replace redirect with
permanentRedirect to reflect the URL move.
- Around line 3-4: Change the function declaration LegacyNotificationsPage to a
const arrow component with an explicit type annotation (e.g., const
LegacyNotificationsPage: React.FC or NextPage) and keep the
redirect('/me/notifications') call inside its body; export the const as default.
Ensure you update the declaration from "export default function
LegacyNotificationsPage() { ... }" to a named const arrow (const
LegacyNotificationsPage: React.FC = () => { redirect('/me/notifications'); })
and then export default LegacyNotificationsPage.
In `@app/me/notifications/page.tsx`:
- Line 30: Replace the function declaration MeNotificationsPage with a const
arrow function using an explicit React component type; change "export default
function MeNotificationsPage() { ... }" to "const MeNotificationsPage: React.FC
= () => { ... }" (or another explicit component type you use across the
codebase) and export it as default, ensuring props/type annotations match
existing usage and any top-level async/metadata exports are preserved.
In `@components/app-sidebar.tsx`:
- Around line 105-109: The current mapping in accountItems unnecessarily
converts unreadCount to a string; update the conditional to assign the raw
number instead (use unreadCount rather than String(unreadCount)) so badge
receives a number directly; change the expression in the accountItems mapping
that matches item.title === 'Notifications' to set badge: unreadCount > 0 ?
unreadCount : undefined (keeping navigationData.account, accountItems,
item.title 'Notifications', badge and unreadCount as the referenced symbols).
In `@components/notifications/NotificationDropdown.tsx`:
- Around line 165-177: Rename the misleading prop usage: replace the
onMarkAsRead prop passed to NotificationItem with a semantically correct name
(e.g., onClick or onSelect) wherever NotificationDropdown renders
NotificationItem; update the handler reference (currently
handleNotificationClick) to match the new prop name and ensure
NotificationItem's disableNavigation and onClose behavior remain unchanged.
Locate the NotificationItem JSX in NotificationDropdown (the block mapping
groupNotifications) and change the prop key and any corresponding prop
type/usage to the new name so it accurately reflects that the handler performs
navigation and closes the menu rather than marking as read.
In `@components/providers/notification-provider.tsx`:
- Line 28: The context value is being recreated every render because useMemo(()
=> notificationsHook, [notificationsHook]) simply forwards a new object
reference from useNotifications; update NotificationProvider to pass a stable
value by either (A) destructuring stable primitives/functions out of
notificationsHook (e.g., items, unreadCount, fetchNotifications, markRead) and
memoizing an object built from those specific values in the useMemo
dependencies, or (B) move the memoization into useNotifications so it returns a
stable object (memoize the return of useNotifications) — change references to
notificationsHook and useMemo accordingly so consumers like
useNotificationCenter and NotificationBell receive a stable context value.
- Line 13: Replace function declarations with const arrow functions and explicit
types: change export function NotificationProvider({ children }: { children:
ReactNode }) { ... } to an exported const arrow with an explicit React type
(e.g., export const NotificationProvider: React.FC<{ children: ReactNode }> = ({
children }) => { ... }), and do the same for the other function declaration in
this file (the second function noted around line 37). Ensure exports, parameters
and returned JSX remain unchanged, and adjust any internal references if needed.
In `@hooks/useNotifications.ts`:
- Around line 29-31: Change the function declaration "export function
useNotifications(input?: string | UseNotificationsOptions):
UseNotificationsReturn" to a const arrow function using an explicit type
annotation, e.g. export const useNotifications: (input?: string |
UseNotificationsOptions) => UseNotificationsReturn = (input) => { ... }; ensure
the exported symbol remains named useNotifications and all existing internal
logic and references to UseNotificationsOptions and UseNotificationsReturn are
preserved.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
app/(landing)/notifications/page.tsxapp/me/notifications/page.tsxapp/providers.tsxcomponents/app-sidebar.tsxcomponents/nav-main.tsxcomponents/notifications/NotificationBell.tsxcomponents/notifications/NotificationDropdown.tsxcomponents/notifications/NotificationItem.tsxcomponents/notifications/NotificationList.tsxcomponents/providers/notification-provider.tsxhooks/use-notification-polling.tshooks/useNotifications.ts
| const { | ||
| notifications, | ||
| unreadCount, | ||
| loading, | ||
| error, | ||
| markAllAsRead, | ||
| markNotificationAsRead, | ||
| refetch, | ||
| } = useNotificationCenter(); |
There was a problem hiding this comment.
total from the context is unused; subtitle shows a potentially misleading loaded-only count.
useNotificationCenter returns total (server-side total), but only notifications.length (at most 50, the limit set in the provider) is shown in the subtitle. When a user has more than 50 notifications the display will read "3 unread of 50 loaded" instead of the real total.
🔧 Suggested change
const {
notifications,
unreadCount,
loading,
error,
+ total,
markAllAsRead,
markNotificationAsRead,
refetch,
} = useNotificationCenter();- {unreadCount} unread of {notifications.length} loaded
+ {unreadCount} unread · {total} totalAlso applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/me/notifications/page.tsx` around lines 31 - 39, The subtitle is using
notifications.length (loaded page size) instead of the server-side total; update
the UI to use the total value returned by useNotificationCenter (the total
variable) when rendering counts (e.g., the subtitle and the block around lines
97-104) and keep notifications.length only for "loaded" indicators if you want
to show loaded vs total; adjust any text to read "{unreadCount} unread of
{total}" (or "{unreadCount} unread of {notifications.length} loaded of {total}")
and ensure you reference useNotificationCenter's total and unreadCount variables
rather than notifications.length for the overall total display.
| Error loading notifications | ||
| </p> | ||
| <p className='mt-2 text-sm text-zinc-400'>{error.message}</p> | ||
| <Button onClick={() => refetch()} className='mt-4' variant='outline'> |
There was a problem hiding this comment.
refetch() in the error retry button has an unhandled rejection.
refetch() returns a Promise<void>. An unhandled rejection here will surface as an uncaught promise error in the console. Wrap it or use .catch():
🔧 Suggested change
- <Button onClick={() => refetch()} className='mt-4' variant='outline'>
+ <Button
+ onClick={() => refetch().catch(() => toast.error('Retry failed'))}
+ className='mt-4'
+ variant='outline'
+ >📝 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.
| <Button onClick={() => refetch()} className='mt-4' variant='outline'> | |
| <Button | |
| onClick={() => refetch().catch(() => toast.error('Retry failed'))} | |
| className='mt-4' | |
| variant='outline' | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/me/notifications/page.tsx` at line 83, The onClick handler for the retry
Button calls refetch() which returns a Promise<void> and currently leaves
rejection unhandled; update the Button's onClick to handle errors from refetch
(e.g., call refetch().catch(err => { /* log or show error */ }) or make the
handler async and use try/catch) so any rejection is caught and reported (ensure
you reference the Button's onClick that invokes refetch()).
| const hasBadge = | ||
| item.badge !== undefined && | ||
| item.badge !== null && | ||
| Number(item.badge) > 0; |
There was a problem hiding this comment.
Number(item.badge) > 0 silently suppresses non-numeric string badges
The type was widened to string | number, but the guard casts everything through Number(). Any non-numeric string (e.g. 'new', 'beta') evaluates to NaN, and NaN > 0 is false, so the badge is never rendered without any error or warning.
The current static badges ('3', '2') are numeric strings so they're unaffected. If the intent is strictly numeric-only badges going forward this is fine, but the type should ideally reflect that constraint — or the check should handle string-typed labels separately:
💡 Option A — restrict the type
- badge?: string | number;
+ badge?: number;💡 Option B — allow label-type strings too
- const hasBadge =
- item.badge !== undefined &&
- item.badge !== null &&
- Number(item.badge) > 0;
+ const hasBadge =
+ item.badge !== undefined &&
+ item.badge !== null &&
+ (typeof item.badge === 'string'
+ ? item.badge.trim().length > 0
+ : item.badge > 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const hasBadge = | |
| item.badge !== undefined && | |
| item.badge !== null && | |
| Number(item.badge) > 0; | |
| const hasBadge = | |
| item.badge !== undefined && | |
| item.badge !== null && | |
| (typeof item.badge === 'string' | |
| ? item.badge.trim().length > 0 | |
| : item.badge > 0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/nav-main.tsx` around lines 46 - 49, The current guard for hasBadge
(using Number(item.badge) > 0) silences non-numeric string badges like "new" by
coercing to NaN; update the logic in components/nav-main.tsx around hasBadge and
item.badge: either tighten the prop/type for item.badge to only
number/numeric-string at the type level, or handle both numeric and label badges
at runtime by checking typeof item.badge === 'number' && item.badge > 0 for
numeric badges, separately accepting non-empty string badges (e.g., typeof
item.badge === 'string' && item.badge.trim() !== '') and for numeric-strings use
a safe parse (Number or parseInt) plus isFinite/isNaN check to render counts;
adjust any consumers/types accordingly (functions/props referencing item.badge
and hasBadge).
| const handleMarkAllAsRead = async () => { | ||
| markAllAsRead(); | ||
| }; |
There was a problem hiding this comment.
markAllAsRead() rejection is unhandled
handleMarkAllAsRead is async but never awaits markAllAsRead(). If the call rejects, the rejection escapes uncaught — the outer async wrapper does not capture it — producing an unhandled Promise rejection.
🛠️ Proposed fix
- const handleMarkAllAsRead = async () => {
- markAllAsRead();
- };
+ const handleMarkAllAsRead = () => {
+ markAllAsRead().catch(() => {});
+ };Or, if you want to surface failures to the user, await and handle the error explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/notifications/NotificationBell.tsx` around lines 33 - 35, The
async handler handleMarkAllAsRead currently calls markAllAsRead() without
awaiting or handling rejections; update handleMarkAllAsRead to await
markAllAsRead() and handle errors (try/catch) or at least return the promise so
rejections are propagated; reference the handleMarkAllAsRead function and the
markAllAsRead call in NotificationBell.tsx and either await within a try/catch
to log/show the error or propagate the promise to the caller.
| animate={{ opacity: 1, scale: 1 }} | ||
| exit={{ opacity: 0, scale: 0.2 }} | ||
| transition={{ duration: 0.2, ease: 'easeOut' }} | ||
| className='bg-primary absolute top-11 right-2 h-1.5 w-1.5 rounded-full shadow-[0_0_8px_rgba(var(--primary),0.5)]' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for --primary CSS variable definition across the project
rg --type css --type scss --type tsx --type ts '\-\-primary' -B2 -A2 | head -100Repository: boundlessfi/boundless
Length of output: 93
🏁 Script executed:
# Also check for tailwind.config files and theme configurations
fd -e "css" -e "scss" -e "js" -e "ts" | grep -E "(global|theme|config)" | head -20Repository: boundlessfi/boundless
Length of output: 134
🏁 Script executed:
# Look specifically in common theme files
find . -type f \( -name "globals.css" -o -name "globals.scss" -o -name "theme.css" -o -name "theme.ts" -o -name "tailwind.config.*" \) -exec cat {} \;Repository: boundlessfi/boundless
Length of output: 15264
🏁 Script executed:
# Check the NotificationItem.tsx file around line 136
cat -n components/notifications/NotificationItem.tsx | sed -n '130,145p'Repository: boundlessfi/boundless
Length of output: 747
🏁 Script executed:
# Also search for all uses of rgba(var(--primary) in the codebase
rg "rgba.*var.*--primary" -A1 -B1Repository: boundlessfi/boundless
Length of output: 922
The rgba(var(--primary), 0.5) syntax is invalid CSS and the shadow will not render.
rgba() expects numeric RGB channel values, but --primary contains either a hex color (#a7f950) or an OKLCH function (oklch(0.922 0 0)). Neither can be directly substituted into rgba(). Use shadow-[0_0_8px_rgb(from_var(--primary)_r_g_b_/_0.5)] (modern CSS) or define a separate --primary-rgb CSS variable with raw channel values (e.g., 167 249 80) for use with rgba(var(--primary-rgb), 0.5).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/notifications/NotificationItem.tsx` at line 136, The
tailwind-style shadow value using rgba(var(--primary),0.5) in NotificationItem
(the className string containing 'shadow-[0_0_8px_rgba(var(--primary),0.5)]') is
invalid; update the shadow to use a valid color source—either switch to the
modern rgb(from_var(--primary) r g b / 0.5) syntax or introduce a separate CSS
variable (e.g., --primary-rgb with "R G B") and use rgba(var(--primary-rgb),0.5)
in the shadow utility—so replace the current shadow-[...] token in the className
on NotificationItem with one of these valid options.
| return ( | ||
| <button | ||
| type='button' | ||
| onClick={handleClick} | ||
| className='block w-full text-left' | ||
| aria-label={notification.title} | ||
| > | ||
| {content} | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
aria-label on the button overrides the full accessible content.
Setting aria-label={notification.title} causes screen readers to announce only the title, discarding message text, timestamp, and amount. Since the button already contains visible, descriptive text, the aria-label should be removed so the computed accessible name includes all visible content.
🔧 Suggested change
<button
type='button'
onClick={handleClick}
className='block w-full text-left'
- aria-label={notification.title}
>📝 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.
| return ( | |
| <button | |
| type='button' | |
| onClick={handleClick} | |
| className='block w-full text-left' | |
| aria-label={notification.title} | |
| > | |
| {content} | |
| </button> | |
| ); | |
| return ( | |
| <button | |
| type='button' | |
| onClick={handleClick} | |
| className='block w-full text-left' | |
| > | |
| {content} | |
| </button> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/notifications/NotificationItem.tsx` around lines 151 - 160, In
NotificationItem, remove the hard-coded aria-label on the button
(aria-label={notification.title}) so the browser computes the accessible name
from the visible content (title, message, timestamp, amount) instead of
overriding it; locate the JSX return in the NotificationItem component where the
button with onClick={handleClick} is rendered and delete the aria-label prop (or
replace it with an aria-labelledby approach referencing IDs inside the content
if you need a custom accessible name).
| ) : ( | ||
| <AnimatePresence initial={false}> | ||
| <div className='space-y-2'> | ||
| {sectionItems.map(notification => ( | ||
| <motion.div | ||
| key={notification.id} | ||
| layout | ||
| initial={{ opacity: 0, y: 6 }} | ||
| animate={{ opacity: 1, y: 0 }} | ||
| exit={{ opacity: 0, y: -4 }} | ||
| transition={{ duration: 0.18 }} | ||
| > | ||
| <NotificationItem | ||
| notification={notification} | ||
| onMarkAsRead={() => { | ||
| if (onMarkAsRead) { | ||
| onMarkAsRead(notification.id); | ||
| } | ||
| if (onNotificationClick) { | ||
| onNotificationClick(notification); | ||
| } | ||
| }} | ||
| showUnreadIndicator={true} | ||
| disableNavigation={true} | ||
| /> | ||
| </motion.div> | ||
| ))} | ||
| </div> | ||
| </AnimatePresence> |
There was a problem hiding this comment.
AnimatePresence exit animations are silently broken by the intermediate <div>
AnimatePresence only triggers exit animations on its direct children. The current structure places <div className='space-y-2'> between AnimatePresence and the motion.div items, so the exit={{ opacity: 0, y: -4 }} prop on each item will never fire — notifications vanish instantly when moved between sections.
🛠️ Proposed fix — move the wrapper div outside AnimatePresence
- <AnimatePresence initial={false}>
- <div className='space-y-2'>
- {sectionItems.map(notification => (
- <motion.div
- key={notification.id}
- layout
- initial={{ opacity: 0, y: 6 }}
- animate={{ opacity: 1, y: 0 }}
- exit={{ opacity: 0, y: -4 }}
- transition={{ duration: 0.18 }}
- >
- <NotificationItem ... />
- </motion.div>
- ))}
- </div>
- </AnimatePresence>
+ <div className='space-y-2'>
+ <AnimatePresence initial={false}>
+ {sectionItems.map(notification => (
+ <motion.div
+ key={notification.id}
+ layout
+ initial={{ opacity: 0, y: 6 }}
+ animate={{ opacity: 1, y: 0 }}
+ exit={{ opacity: 0, y: -4 }}
+ transition={{ duration: 0.18 }}
+ >
+ <NotificationItem ... />
+ </motion.div>
+ ))}
+ </AnimatePresence>
+ </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.
| ) : ( | |
| <AnimatePresence initial={false}> | |
| <div className='space-y-2'> | |
| {sectionItems.map(notification => ( | |
| <motion.div | |
| key={notification.id} | |
| layout | |
| initial={{ opacity: 0, y: 6 }} | |
| animate={{ opacity: 1, y: 0 }} | |
| exit={{ opacity: 0, y: -4 }} | |
| transition={{ duration: 0.18 }} | |
| > | |
| <NotificationItem | |
| notification={notification} | |
| onMarkAsRead={() => { | |
| if (onMarkAsRead) { | |
| onMarkAsRead(notification.id); | |
| } | |
| if (onNotificationClick) { | |
| onNotificationClick(notification); | |
| } | |
| }} | |
| showUnreadIndicator={true} | |
| disableNavigation={true} | |
| /> | |
| </motion.div> | |
| ))} | |
| </div> | |
| </AnimatePresence> | |
| ) : ( | |
| <div className='space-y-2'> | |
| <AnimatePresence initial={false}> | |
| {sectionItems.map(notification => ( | |
| <motion.div | |
| key={notification.id} | |
| layout | |
| initial={{ opacity: 0, y: 6 }} | |
| animate={{ opacity: 1, y: 0 }} | |
| exit={{ opacity: 0, y: -4 }} | |
| transition={{ duration: 0.18 }} | |
| > | |
| <NotificationItem | |
| notification={notification} | |
| onMarkAsRead={() => { | |
| if (onMarkAsRead) { | |
| onMarkAsRead(notification.id); | |
| } | |
| if (onNotificationClick) { | |
| onNotificationClick(notification); | |
| } | |
| }} | |
| showUnreadIndicator={true} | |
| disableNavigation={true} | |
| /> | |
| </motion.div> | |
| ))} | |
| </AnimatePresence> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/notifications/NotificationList.tsx` around lines 125 - 153, The
exit animations don't run because AnimatePresence's direct child is the wrapper
<div className='space-y-2'>; move that wrapper outside (or replace it so)
AnimatePresence's direct children are the motion.div items so each motion.div
(the elements rendering NotificationItem) can receive exit/initial/animate
props; update the JSX in NotificationList (where AnimatePresence, motion.div and
NotificationItem are used) so AnimatePresence wraps the mapped motion.divs
directly while preserving the spacing/layout (e.g., apply the 'space-y-2' class
to a parent container or to the motion.div elements themselves).
| <NotificationItem | ||
| notification={notification} | ||
| onMarkAsRead={() => { | ||
| if (onMarkAsRead) { | ||
| onMarkAsRead(notification.id); | ||
| } | ||
| if (onNotificationClick) { | ||
| onNotificationClick(notification); | ||
| } | ||
| }} | ||
| showUnreadIndicator={true} | ||
| disableNavigation={true} | ||
| /> |
There was a problem hiding this comment.
disableNavigation={true} hardcoded — users cannot click through to notification targets
With disableNavigation always true, every NotificationItem on the full /me/notifications page renders as a <button> (see NotificationItem.tsx Lines 148-155: isClickable && !disableNavigation guard). Clicking only marks the item as read; it never navigates to the linked resource (/hackathons/…, /projects/…, etc.).
If keeping users on the notifications page is the intent, consider at least navigating after marking as read. If navigation is simply being deferred, track this as a known gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/notifications/NotificationList.tsx` around lines 137 - 149,
NotificationList is forcing NotificationItem to never navigate by hardcoding
disableNavigation={true}; remove or set disableNavigation based on a prop/state
so NotificationItem can navigate (e.g., pass
disableNavigation={disableNavigation} or omit it), and ensure the existing
onMarkAsRead/onNotificationClick logic still runs (NotificationItem,
onMarkAsRead, onNotificationClick) so clicks both mark read and then navigate to
the target when navigation is enabled.
| intervalRef.current = setInterval(() => { | ||
| refetch().catch(() => { | ||
| // Silently fail polling errors to avoid disrupting UX | ||
| }); | ||
| refetch().catch(() => {}); | ||
| }, interval); |
There was a problem hiding this comment.
Silent polling errors leave no diagnostic signal.
.catch(() => {}) fully swallows errors from refetch. When polling consistently fails (network issues, auth expiry, server errors), there is no log entry, metric, or user feedback. At minimum, log the error at console.warn/console.error level.
🔧 Suggested change
- refetch().catch(() => {});
+ refetch().catch((err) => {
+ console.warn('[useNotificationPolling] refetch failed:', err);
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/use-notification-polling.ts` around lines 22 - 24, The polling
currently swallows all refetch errors via .catch(() => {}) which loses
diagnostics; update the setInterval callback (where intervalRef.current is
assigned) to catch and log the error (e.g., console.error or console.warn)
including context (like "notification polling refetch failed") and the error
object so failures from refetch() are visible, and keep the existing behavior of
not throwing from the interval handler.
1 participant
CI context:
npm run lintpasses locallynpm run buildpasses locallySecurity audit failure appears to come from existing transitive vulnerabilities in upstream wallet/crypto dependency chains (
bn.jsvia@trezor/*,@hot-wallet/sdk,near-api-jspath), not from this feature change.Vercel preview is blocked by project authorization (
Authorization required to deploy) for this branch.Requesting maintainer guidance on:
@0xdevcollins
Summary by CodeRabbit
Release Notes
New Features
Improvements