Feat/notification module#77
Merged
JosueBrenes merged 9 commits intoACTA-Team:developfrom Mar 11, 2026
Merged
Conversation
Contributor
|
@FabianSanchezD is attempting to deploy a commit to the ACTA Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements a complete notifications module for the dApp, adding a notification bell in the header with an unread badge and preview popover, a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Header as Header<br/>(NotificationBell)
participant Hook as useNotifications
participant Query as React Query
participant API as ACTA API
participant Toast as Sonner Toast
User->>Header: Opens dApp
Header->>Hook: Call useNotifications(limit: 8)
Hook->>Query: Execute two queries in parallel
Query->>API: GET /notifications?wallet_address=...
Query->>API: GET /notifications/unread-count?wallet_address=...
API-->>Query: Return notifications array
API-->>Query: Return unread count
Query-->>Hook: Return data + loading state
Hook-->>Header: Render badge with unread count
Header->>User: Show bell icon with badge
User->>Header: Click notification bell
Header->>Header: Open popover
Header->>User: Display preview list (compact NotificationItem)
Note over Header,API: Real-time polling (every 20s on dashboard)
Hook->>Hook: useNotificationsRealtime polls
Hook->>API: GET /notifications?unread_only=true&limit=20
API-->>Hook: Return new notifications
Hook->>Hook: Deduplicate with seenIdsRef
Hook->>Toast: Show new notification toast
Toast->>User: Display success/info/warning toast
sequenceDiagram
participant User
participant List as NotificationList Page
participant Hook as useNotifications +<br/>useNotificationsActions
participant Query as React Query
participant API as ACTA API
User->>List: Navigate to /dashboard/notifications
List->>Hook: Call useNotifications(limit: 20, offset: 0)
Hook->>Query: Fetch notifications list
Query->>API: GET /notifications?wallet_address=...&limit=20
API-->>Query: Return paginated notifications
Query-->>Hook: Return data
Hook-->>List: Render NotificationItem components
User->>List: Click "All" or "Unread" filter
List->>List: Update unreadOnly state
List->>Hook: Refetch with unreadOnly flag
Hook->>API: GET /notifications?unread_only=true
API-->>Hook: Return filtered notifications
Hook-->>List: Re-render filtered list
User->>List: Click "Mark as read" on item
List->>Hook: Call markAsRead(notificationId)
Hook->>API: PATCH /notifications/:id/read?wallet_address=...
API-->>Hook: Return success
Hook->>Query: Invalidate notifications + unread count caches
Query-->>List: Auto-refetch and update UI
List->>User: Remove item or update read state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
And ready for review! Sorry for the wait. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/components/modules/notifications/hooks/useNotificationsRealtime.ts (1)
23-31: Share the list-path builder withuseNotifications.This helper has already drifted from
src/components/modules/notifications/hooks/useNotifications.tsin how it encodesunread_onlyandlimit. Pulling both hooks onto one shared builder will make future API-param changes much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modules/notifications/hooks/useNotificationsRealtime.ts` around lines 23 - 31, The buildListPath in useNotificationsRealtime.ts has drifted from the one in useNotifications.ts (different encoding for unread_only and limit); extract a single shared builder (e.g., buildNotificationsListPath) into a common module and update both useNotifications and useNotificationsRealtime to import and use it. Make the shared function signature match the canonical implementation in useNotifications.ts (types for walletAddress, network, unreadOnly boolean, limit number) and ensure it encodes unread_only and limit exactly the same way as the canonical code (use URLSearchParams or explicit serialisation used in useNotifications.ts). Replace local buildListPath references in both hooks with the shared builder and export it for reuse.src/components/modules/notifications/ui/NotificationPreview.tsx (1)
10-15: Redundant slice operation.The hook is called with
limit: PREVIEW_LIMIT(8), sonotificationswill already contain at most 8 items. The.slice(0, PREVIEW_LIMIT)on line 15 is redundant.♻️ Proposed fix
const { notifications, loading } = useNotifications({ limit: PREVIEW_LIMIT, unreadOnly: false, }); - const previewList = notifications.slice(0, PREVIEW_LIMIT); + // notifications is already limited to PREVIEW_LIMIT by the hookThen replace
previewListwithnotificationsthroughout the component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modules/notifications/ui/NotificationPreview.tsx` around lines 10 - 15, The code redundantly slices notifications even though useNotifications was already called with limit: PREVIEW_LIMIT; remove the unnecessary slice by deleting previewList and replace any usage of previewList with notifications (references: useNotifications, PREVIEW_LIMIT, previewList, notifications) so the component consumes notifications directly.src/components/modules/notifications/hooks/useNotifications.ts (2)
42-44: Inconsistent URL encoding between path builders.
buildUnreadCountPathusesencodeURIComponentforwalletAddressbut not fornetwork, whilebuildListPathusesURLSearchParamswhich automatically encodes values. For consistency and safety, consider usingURLSearchParamshere as well.♻️ Proposed fix
function buildUnreadCountPath(walletAddress: string, network: string): string { - return `/notifications/unread-count?wallet_address=${encodeURIComponent(walletAddress)}&network=${network}`; + const params = new URLSearchParams({ + wallet_address: walletAddress, + network, + }); + return `/notifications/unread-count?${params.toString()}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modules/notifications/hooks/useNotifications.ts` around lines 42 - 44, buildUnreadCountPath currently encodes walletAddress with encodeURIComponent but leaves network unencoded, which is inconsistent with buildListPath; change buildUnreadCountPath to construct its query using URLSearchParams (e.g., new URLSearchParams({ wallet_address: walletAddress, network })) so both wallet_address and network are properly percent-encoded and consistent with buildListPath's behavior.
61-63: Redundant enabled check inside queryFn.The condition
!walletAddress || !apiKey?.trim()is already enforced by theenabledoption (line 72). The query function won't execute whenenabledis false, making this guard redundant.♻️ Proposed fix
queryFn: async () => { - if (!walletAddress || !apiKey?.trim()) return []; const path = buildListPath(walletAddress, network, options);Note: Similar redundant check exists in
unreadCountQueryat lines 78-79.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modules/notifications/hooks/useNotifications.ts` around lines 61 - 63, The query functions contain redundant guards against missing walletAddress/apiKey inside the async queryFn and unreadCountQuery even though the same checks are already enforced via the React Query "enabled" option; remove the `if (!walletAddress || !apiKey?.trim()) return [];` (and the analogous check in unreadCountQuery) so the queryFn implementations only build the path (using buildListPath) and perform fetching, relying on the enabled flag to prevent execution when inputs are absent.src/components/modules/notifications/ui/NotificationBell.tsx (1)
21-35: Consider making the aria-label dynamic to convey unread count to screen readers.The current static
aria-label="Notifications"doesn't inform screen reader users of the unread count. While the badge is correctly markedaria-hidden, this means the count is only conveyed visually.♿ Proposed accessibility improvement
<button type="button" - aria-label="Notifications" + aria-label={ + unreadCount > 0 + ? `Notifications, ${unreadCount > 99 ? '99+' : unreadCount} unread` + : 'Notifications' + } className="relative flex items-center justify-center rounded-lg p-2 text-zinc-400 hover:bg-[`#edeed1`]/10 hover:text-[`#edeed1`] transition-colors" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modules/notifications/ui/NotificationBell.tsx` around lines 21 - 35, Update the NotificationBell button's static aria-label to a dynamic string that includes the unreadCount so screen readers receive the unread count (use the unreadCount variable in NotificationBell to produce labels like "No new notifications", "1 unread notification", or "5 unread notifications"); keep the visual badge element aria-hidden so it remains ignored by assistive tech and ensure the dynamic label updates whenever unreadCount changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/modules/notifications/hooks/useNotificationsRealtime.ts`:
- Around line 38-39: The refs seenIdsRef and hasInitializedRef persist across
wallet/network switches, causing old notifications to be treated as new; update
the useNotificationsRealtime hook to reset these on identity changes by adding
an effect that watches the wallet/network identifiers (e.g., walletAddress or
networkId) and sets seenIdsRef.current = new Set() and hasInitializedRef.current
= false when they change; also reset the same realtime state referenced in the
later block (the variables around lines 64-76) so any per-session caches are
cleared on identity switch.
In `@src/components/modules/notifications/ui/formatRelativeTime.ts`:
- Around line 1-19: The formatRelativeTime function currently lets future
timestamps produce negative diffs and relies on try/catch; explicitly validate
the parsed date by checking if const ts = date.getTime() is NaN and return an
empty string (or suitable fallback) if so, then compute diffMs using a clamped
value (e.g., diffMs = Math.max(0, now.getTime() - ts)) so future times are
treated as "Just now" instead of negative values; update references in
formatRelativeTime to use the validated ts and clamped diff calculation.
In `@src/components/modules/notifications/ui/NotificationItem.tsx`:
- Around line 60-62: The code currently prefers static type-level copy over
backend-provided fields, causing notification.title and notification.message to
be overwritten by NOTIFICATION_TYPE_LABEL entries; update the logic in
NotificationItem (refs: NOTIFICATION_TYPE_LABEL, notification, typeLabel,
typeMessage) and similarly in useNotificationToast (the equivalent title/message
selection) to prefer notification.title and notification.message when present,
falling back to NOTIFICATION_TYPE_LABEL[type]?.title or .message only if the
corresponding notification field is missing or empty; keep the same
nullish/coalescing approach but swap operands so API-provided values take
precedence.
In `@src/components/modules/notifications/ui/NotificationList.tsx`:
- Around line 14-18: The notifications list currently hardcodes offset: 0 in the
useNotifications call so pagination/load-more is broken; change to maintain an
offset state (e.g. const [offset, setOffset] = useState(0)), pass that offset
into useNotifications({ unreadOnly, limit: PAGE_SIZE, offset }), render a "Load
more" button that calls setOffset(offset + PAGE_SIZE) when clicked, and ensure
the button is disabled/hidden when loading or when the returned notifications
length is less than PAGE_SIZE (no more pages) or on error; update any local
variables that expect a single-page result to handle appending new pages instead
of replacing them if the hook returns incremental data.
- Around line 21-27: The click handler currently navigates to
'/dashboard/notifications' when vcId is undefined, causing a no-op; update
handleItemClick in NotificationList.tsx to provide user feedback instead of
silent navigation: if vcId is falsy, show a toast/inline feedback (using your
app's toast/notification utility) and return early; otherwise call
router.push(`/credential/${vcId}`). Also update the clickable UI (the element
that calls handleItemClick) to be non-interactive/aria-disabled when
notification.vc_id is undefined so it doesn't appear clickable.
In `@src/components/modules/notifications/ui/NotificationPreview.tsx`:
- Around line 17-41: NotificationPreview currently only checks loading and
previewList but ignores the error from useNotifications; update the component to
read the error (and any refetch/retry function) returned by useNotifications and
render an error state instead of the loading/empty UI when an API error occurs.
Specifically, inside NotificationPreview replace the conditional that checks
loading/previewList to first check for error and render a concise error message
(and an optional retry control that calls the hook's refetch/retry if provided),
ensuring NotificationItem, previewList, and loading behavior remain unchanged
when there is no error.
---
Nitpick comments:
In `@src/components/modules/notifications/hooks/useNotifications.ts`:
- Around line 42-44: buildUnreadCountPath currently encodes walletAddress with
encodeURIComponent but leaves network unencoded, which is inconsistent with
buildListPath; change buildUnreadCountPath to construct its query using
URLSearchParams (e.g., new URLSearchParams({ wallet_address: walletAddress,
network })) so both wallet_address and network are properly percent-encoded and
consistent with buildListPath's behavior.
- Around line 61-63: The query functions contain redundant guards against
missing walletAddress/apiKey inside the async queryFn and unreadCountQuery even
though the same checks are already enforced via the React Query "enabled"
option; remove the `if (!walletAddress || !apiKey?.trim()) return [];` (and the
analogous check in unreadCountQuery) so the queryFn implementations only build
the path (using buildListPath) and perform fetching, relying on the enabled flag
to prevent execution when inputs are absent.
In `@src/components/modules/notifications/hooks/useNotificationsRealtime.ts`:
- Around line 23-31: The buildListPath in useNotificationsRealtime.ts has
drifted from the one in useNotifications.ts (different encoding for unread_only
and limit); extract a single shared builder (e.g., buildNotificationsListPath)
into a common module and update both useNotifications and
useNotificationsRealtime to import and use it. Make the shared function
signature match the canonical implementation in useNotifications.ts (types for
walletAddress, network, unreadOnly boolean, limit number) and ensure it encodes
unread_only and limit exactly the same way as the canonical code (use
URLSearchParams or explicit serialisation used in useNotifications.ts). Replace
local buildListPath references in both hooks with the shared builder and export
it for reuse.
In `@src/components/modules/notifications/ui/NotificationBell.tsx`:
- Around line 21-35: Update the NotificationBell button's static aria-label to a
dynamic string that includes the unreadCount so screen readers receive the
unread count (use the unreadCount variable in NotificationBell to produce labels
like "No new notifications", "1 unread notification", or "5 unread
notifications"); keep the visual badge element aria-hidden so it remains ignored
by assistive tech and ensure the dynamic label updates whenever unreadCount
changes.
In `@src/components/modules/notifications/ui/NotificationPreview.tsx`:
- Around line 10-15: The code redundantly slices notifications even though
useNotifications was already called with limit: PREVIEW_LIMIT; remove the
unnecessary slice by deleting previewList and replace any usage of previewList
with notifications (references: useNotifications, PREVIEW_LIMIT, previewList,
notifications) so the component consumes notifications directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e668ff6-0a3d-4bbd-abbb-892d560dd2cb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
package.jsonsrc/app/dashboard/notifications/page.tsxsrc/components/modules/notifications/hooks/useNotificationToast.tssrc/components/modules/notifications/hooks/useNotifications.tssrc/components/modules/notifications/hooks/useNotificationsActions.tssrc/components/modules/notifications/hooks/useNotificationsRealtime.tssrc/components/modules/notifications/types.tssrc/components/modules/notifications/ui/NotificationBell.tsxsrc/components/modules/notifications/ui/NotificationItem.tsxsrc/components/modules/notifications/ui/NotificationList.tsxsrc/components/modules/notifications/ui/NotificationPreview.tsxsrc/components/modules/notifications/ui/NotificationsRealtimeEffect.tsxsrc/components/modules/notifications/ui/formatRelativeTime.tssrc/components/ui/mobile-bottom-nav.tsxsrc/components/ui/popover.tsxsrc/layouts/client/Client.tsxsrc/layouts/header/Header.tsxsrc/layouts/sidebar/Sidebar.tsx
Comment on lines
+38
to
+39
| const seenIdsRef = useRef<Set<string>>(new Set()); | ||
| const hasInitializedRef = useRef(false); |
There was a problem hiding this comment.
Reset the realtime seen-state when wallet or network changes.
seenIdsRef and hasInitializedRef survive identity switches, so the first unread batch for a new wallet/network is treated as newly arrived and every existing notification can toast at once.
🛠️ Proposed fix
const { showNotificationToast } = useNotificationToast();
const seenIdsRef = useRef<Set<string>>(new Set());
const hasInitializedRef = useRef(false);
+
+ useEffect(() => {
+ seenIdsRef.current = new Set();
+ hasInitializedRef.current = false;
+ }, [walletAddress, network]);
const enabled =
!!(walletAddress && apiKey?.trim()) &&Also applies to: 64-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/hooks/useNotificationsRealtime.ts`
around lines 38 - 39, The refs seenIdsRef and hasInitializedRef persist across
wallet/network switches, causing old notifications to be treated as new; update
the useNotificationsRealtime hook to reset these on identity changes by adding
an effect that watches the wallet/network identifiers (e.g., walletAddress or
networkId) and sets seenIdsRef.current = new Set() and hasInitializedRef.current
= false when they change; also reset the same realtime state referenced in the
later block (the variables around lines 64-76) so any per-session caches are
cleared on identity switch.
Comment on lines
+1
to
+19
| export function formatRelativeTime(isoString: string): string { | ||
| try { | ||
| const date = new Date(isoString); | ||
| const now = new Date(); | ||
| const diffMs = now.getTime() - date.getTime(); | ||
| const diffSec = Math.floor(diffMs / 1000); | ||
| const diffMin = Math.floor(diffSec / 60); | ||
| const diffHour = Math.floor(diffMin / 60); | ||
| const diffDay = Math.floor(diffHour / 24); | ||
|
|
||
| if (diffSec < 60) return 'Just now'; | ||
| if (diffMin < 60) return `${diffMin} min ago`; | ||
| if (diffHour < 24) return `${diffHour}h ago`; | ||
| if (diffDay < 7) return `${diffDay}d ago`; | ||
| return date.toLocaleDateString(); | ||
| } catch { | ||
| return ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
Clamp future times and validate the parsed date explicitly.
Future timestamps currently render as "Just now" because the delta goes negative before the threshold checks. It would also be safer to validate date.getTime() directly instead of depending on the try/catch.
🛠️ Proposed fix
export function formatRelativeTime(isoString: string): string {
- try {
- const date = new Date(isoString);
- const now = new Date();
- const diffMs = now.getTime() - date.getTime();
- const diffSec = Math.floor(diffMs / 1000);
- const diffMin = Math.floor(diffSec / 60);
- const diffHour = Math.floor(diffMin / 60);
- const diffDay = Math.floor(diffHour / 24);
-
- if (diffSec < 60) return 'Just now';
- if (diffMin < 60) return `${diffMin} min ago`;
- if (diffHour < 24) return `${diffHour}h ago`;
- if (diffDay < 7) return `${diffDay}d ago`;
- return date.toLocaleDateString();
- } catch {
- return '';
- }
+ const date = new Date(isoString);
+ const timestamp = date.getTime();
+ if (Number.isNaN(timestamp)) return '';
+
+ const diffMs = Math.max(0, Date.now() - timestamp);
+ const diffSec = Math.floor(diffMs / 1000);
+ const diffMin = Math.floor(diffSec / 60);
+ const diffHour = Math.floor(diffMin / 60);
+ const diffDay = Math.floor(diffHour / 24);
+
+ if (diffSec < 60) return 'Just now';
+ if (diffMin < 60) return `${diffMin} min ago`;
+ if (diffHour < 24) return `${diffHour}h ago`;
+ if (diffDay < 7) return `${diffDay}d ago`;
+ return date.toLocaleDateString();
}📝 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 formatRelativeTime(isoString: string): string { | |
| try { | |
| const date = new Date(isoString); | |
| const now = new Date(); | |
| const diffMs = now.getTime() - date.getTime(); | |
| const diffSec = Math.floor(diffMs / 1000); | |
| const diffMin = Math.floor(diffSec / 60); | |
| const diffHour = Math.floor(diffMin / 60); | |
| const diffDay = Math.floor(diffHour / 24); | |
| if (diffSec < 60) return 'Just now'; | |
| if (diffMin < 60) return `${diffMin} min ago`; | |
| if (diffHour < 24) return `${diffHour}h ago`; | |
| if (diffDay < 7) return `${diffDay}d ago`; | |
| return date.toLocaleDateString(); | |
| } catch { | |
| return ''; | |
| } | |
| } | |
| export function formatRelativeTime(isoString: string): string { | |
| const date = new Date(isoString); | |
| const timestamp = date.getTime(); | |
| if (Number.isNaN(timestamp)) return ''; | |
| const diffMs = Math.max(0, Date.now() - timestamp); | |
| const diffSec = Math.floor(diffMs / 1000); | |
| const diffMin = Math.floor(diffSec / 60); | |
| const diffHour = Math.floor(diffMin / 60); | |
| const diffDay = Math.floor(diffHour / 24); | |
| if (diffSec < 60) return 'Just now'; | |
| if (diffMin < 60) return `${diffMin} min ago`; | |
| if (diffHour < 24) return `${diffHour}h ago`; | |
| if (diffDay < 7) return `${diffDay}d ago`; | |
| return date.toLocaleDateString(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/ui/formatRelativeTime.ts` around lines 1
- 19, The formatRelativeTime function currently lets future timestamps produce
negative diffs and relies on try/catch; explicitly validate the parsed date by
checking if const ts = date.getTime() is NaN and return an empty string (or
suitable fallback) if so, then compute diffMs using a clamped value (e.g.,
diffMs = Math.max(0, now.getTime() - ts)) so future times are treated as "Just
now" instead of negative values; update references in formatRelativeTime to use
the validated ts and clamped diff calculation.
Comment on lines
+60
to
+62
| const copy = NOTIFICATION_TYPE_LABEL[notification.type]; | ||
| const typeLabel = copy?.title ?? 'Notification'; | ||
| const typeMessage = copy?.message ?? notification.message; |
There was a problem hiding this comment.
Prefer the API-provided title/message over the static fallback copy.
These lines currently replace notification.title and notification.message with generic type-level text, so all notifications of the same type render the same content. That drops backend-specific context in the list, and the same precedence issue also exists in src/components/modules/notifications/hooks/useNotificationToast.ts Lines 25-27.
🛠️ Proposed fix
const copy = NOTIFICATION_TYPE_LABEL[notification.type];
- const typeLabel = copy?.title ?? 'Notification';
- const typeMessage = copy?.message ?? notification.message;
+ const typeLabel = notification.title || copy?.title || 'Notification';
+ const typeMessage = notification.message || copy?.message || '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/ui/NotificationItem.tsx` around lines 60
- 62, The code currently prefers static type-level copy over backend-provided
fields, causing notification.title and notification.message to be overwritten by
NOTIFICATION_TYPE_LABEL entries; update the logic in NotificationItem (refs:
NOTIFICATION_TYPE_LABEL, notification, typeLabel, typeMessage) and similarly in
useNotificationToast (the equivalent title/message selection) to prefer
notification.title and notification.message when present, falling back to
NOTIFICATION_TYPE_LABEL[type]?.title or .message only if the corresponding
notification field is missing or empty; keep the same nullish/coalescing
approach but swap operands so API-provided values take precedence.
Comment on lines
+14
to
+18
| const { notifications, loading, error } = useNotifications({ | ||
| unreadOnly, | ||
| limit: PAGE_SIZE, | ||
| offset: 0, | ||
| }); |
There was a problem hiding this comment.
Pagination is not implemented.
The PR objectives mention "pagination/load-more behavior" but offset is hardcoded to 0. Users cannot load more notifications beyond the first 20 items.
Would you like me to help implement a "Load more" button that increments the offset, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/ui/NotificationList.tsx` around lines 14
- 18, The notifications list currently hardcodes offset: 0 in the
useNotifications call so pagination/load-more is broken; change to maintain an
offset state (e.g. const [offset, setOffset] = useState(0)), pass that offset
into useNotifications({ unreadOnly, limit: PAGE_SIZE, offset }), render a "Load
more" button that calls setOffset(offset + PAGE_SIZE) when clicked, and ensure
the button is disabled/hidden when loading or when the returned notifications
length is less than PAGE_SIZE (no more pages) or on error; update any local
variables that expect a single-page result to handle appending new pages instead
of replacing them if the hook returns incremental data.
Comment on lines
+21
to
+27
| const handleItemClick = (vcId: string | undefined) => { | ||
| if (vcId) { | ||
| router.push(`/credential/${vcId}`); | ||
| } else { | ||
| router.push('/dashboard/notifications'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
No-op navigation when vcId is undefined.
When a notification lacks vc_id metadata, clicking it navigates to /dashboard/notifications — the current page. This creates a confusing UX where the click appears to do nothing. Consider either disabling the click handler for such notifications or showing a toast/feedback instead.
♻️ Proposed fix
const handleItemClick = (vcId: string | undefined) => {
if (vcId) {
router.push(`/credential/${vcId}`);
- } else {
- router.push('/dashboard/notifications');
}
+ // No navigation when vcId is absent
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleItemClick = (vcId: string | undefined) => { | |
| if (vcId) { | |
| router.push(`/credential/${vcId}`); | |
| } else { | |
| router.push('/dashboard/notifications'); | |
| } | |
| }; | |
| const handleItemClick = (vcId: string | undefined) => { | |
| if (vcId) { | |
| router.push(`/credential/${vcId}`); | |
| } | |
| // No navigation when vcId is absent | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/ui/NotificationList.tsx` around lines 21
- 27, The click handler currently navigates to '/dashboard/notifications' when
vcId is undefined, causing a no-op; update handleItemClick in
NotificationList.tsx to provide user feedback instead of silent navigation: if
vcId is falsy, show a toast/inline feedback (using your app's toast/notification
utility) and return early; otherwise call router.push(`/credential/${vcId}`).
Also update the clickable UI (the element that calls handleItemClick) to be
non-interactive/aria-disabled when notification.vc_id is undefined so it doesn't
appear clickable.
Comment on lines
+17
to
+41
| return ( | ||
| <div className="flex flex-col"> | ||
| <div className="max-h-[320px] overflow-y-auto"> | ||
| {loading ? ( | ||
| <div className="p-4 text-center text-sm text-zinc-500">Loading…</div> | ||
| ) : previewList.length === 0 ? ( | ||
| <div className="p-4 text-center text-sm text-zinc-500">No notifications</div> | ||
| ) : ( | ||
| <div className="divide-y divide-zinc-800"> | ||
| {previewList.map((notification) => ( | ||
| <NotificationItem key={notification.id} notification={notification} compact /> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| {previewList.length > 0 && ( | ||
| <Link | ||
| href="/dashboard/notifications" | ||
| className="border-t border-zinc-800 px-4 py-3 text-center text-sm text-[#edeed1] hover:bg-zinc-800/50" | ||
| > | ||
| View all | ||
| </Link> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Missing error state handling.
Unlike NotificationList, this component doesn't handle the error state from useNotifications. If the API request fails, users see the loading state indefinitely or an empty preview.
🛡️ Proposed fix
export function NotificationPreview() {
- const { notifications, loading } = useNotifications({
+ const { notifications, loading, error } = useNotifications({
limit: PREVIEW_LIMIT,
unreadOnly: false,
});
- const previewList = notifications.slice(0, PREVIEW_LIMIT);
-
return (
<div className="flex flex-col">
<div className="max-h-[320px] overflow-y-auto">
- {loading ? (
+ {error ? (
+ <div className="p-4 text-center text-sm text-red-400">Failed to load</div>
+ ) : loading ? (
<div className="p-4 text-center text-sm text-zinc-500">Loading…</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modules/notifications/ui/NotificationPreview.tsx` around lines
17 - 41, NotificationPreview currently only checks loading and previewList but
ignores the error from useNotifications; update the component to read the error
(and any refetch/retry function) returned by useNotifications and render an
error state instead of the loading/empty UI when an API error occurs.
Specifically, inside NotificationPreview replace the conditional that checks
loading/previewList to first check for error and render a concise error message
(and an optional retry control that calls the hook's refetch/retry if provided),
ensuring NotificationItem, previewList, and loading behavior remain unchanged
when there is no error.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Thanks @FabianSanchezD |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
🚀 ACTA Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Implements the full notifications module for the ACTA dApp, consuming the existing backend Notifications API.
I wasn't able to use ActaFetchJson without changing the API.
📸 Evidence (A Loom/Cap video is required as evidence, we WON'T merge if there's no proof)
https://cap.so/s/1ycrjznq6az9qme
⏰ Time spent breakdown
3.5 hours
🌌 Comments
Thank you for contributing to ACTA! We hope you can continue contributing to this project.