Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdd grouped drop-reaction support: new NotificationDisplayItem types and grouping util, grouped UI components (avatar stack, emoji preview, follow-all), propagate a mark-as-read handler through hooks/controllers, and switch notification lists to render grouped or single items accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant API
participant DB
User->>Client: Open notifications UI
Client->>API: GET /notifications (paginated)
API->>DB: fetch notifications
DB-->>API: TypedNotification[]
API-->>Client: return raw notifications
Note over Client: useNotificationsQuery -> rawItems
Client->>Client: groupReactionNotifications(rawItems)
Client-->>Client: NotificationDisplayItem[] (grouped + single)
loop render items
Client->>Client: isGroupedReactionsItem?
alt grouped
Client->>Client: Render NotificationDropReactedGroup (OverlappingAvatars, ReactionEmojiPreview, NotificationsFollowAllBtn)
else single
Client->>Client: Render NotificationItem
end
end
User->>Client: Click "Follow All" (group)
Client->>API: POST /identities/{handle}/subscriptions (concurrent)
API->>DB: create subscriptions
DB-->>API: success
API-->>Client: success
Client->>Client: onIdentityFollowChange -> update UI
User->>Client: Click grouped drop
Client->>API: POST /notifications/{id}/read
API->>DB: mark read
DB-->>API: success
API-->>Client: success
Client->>Client: navigate to drop
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@components/brain/notifications/drop-reacted/NotificationDropReactedGroup.tsx`:
- Around line 67-151: The Follow All button currently receives reactors which is
truncated by MAX_OVERLAP_AVATARS so it only follows the displayed avatars;
instead compute a full unique reactor list from latestPerUser (e.g.,
fullReactors = latestPerUser.map(n => n.related_identity) and dedupe by
id/handle/primary_address) and pass that fullReactors to
NotificationsFollowAllBtn while keeping the existing reactors slice for
OverlappingAvatars and emoji display; update references to reactors vs
fullReactors in the JSX so OverlappingAvatars and emoji rendering remain
unchanged but NotificationsFollowAllBtn receives the complete set.
- Around line 60-79: The read-guard hasMarkedRef never resets, so when the
grouped notifications (notifications / ids) change the component may not mark
new items as read; add a useEffect that watches the notification id set (e.g.,
ids or notifications) and resets hasMarkedRef.current = false when it changes.
Locate hasMarkedRef and handleDropContentClick in NotificationDropReactedGroup
and add the effect to clear the guard whenever ids/notifications update so new
clicks will call onMarkAsRead for the new items.
In `@components/brain/notifications/utils/groupReactionNotifications.ts`:
- Around line 21-56: The code currently anchors grouped reactions by array index
(using Math.max(existing.anchorIndex, index) in the items.forEach block), which
breaks when items are not strictly ordered by created_at; update the grouping to
anchor by the latest created_at instead: track the anchor by comparing
timestamps (created_at) when merging into byDropId (or store anchorCreatedAt
alongside notifications) and when building indexToGroup compute the anchorIndex
from the notification whose created_at equals the group's latest (or find its
index in items) so indexToGroup.set uses the position of the latest-created
notification; adjust references in this file: byDropId, anchorIndex,
items.forEach loop, and indexToGroup/GroupedReactionsItem to use
created_at-based anchoring.
🧹 Nitpick comments (3)
components/brain/notifications/NotificationItems.tsx (1)
4-76: Simplify onMarkAsRead pass-through.The wrapper allocates a new function per render, but the signatures already match. Passing the handler directly is simpler.
♻️ Suggested simplification
- onMarkAsRead={ - onMarkGroupAsRead - ? (ids) => onMarkGroupAsRead(ids) - : undefined - } + onMarkAsRead={onMarkGroupAsRead}components/brain/notifications/hooks/useNotificationsController.ts (2)
165-189: ConsiderPromise.allSettledto handle partial failures gracefully.Using
Promise.allmeans if any single notification fails to mark as read, the entire operation fails and the user sees an error toast—even though some notifications may have been successfully marked. Additionally, for grouped notifications with many IDs, firing all requests in parallel could hit rate limits.Also consider handling the edge case of an empty
idsarray to avoid unnecessary API invalidation.♻️ Proposed fix using Promise.allSettled
const { mutateAsync: markNotificationIdsAsRead } = useMutation({ mutationFn: async (ids: number[]) => { + if (ids.length === 0) return; - await Promise.all( + const results = await Promise.allSettled( ids.map((id) => commonApiPostWithoutBodyAndResponse({ endpoint: `notifications/${id}/read`, }) ) ); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length > 0) { + throw new Error(`Failed to mark ${failures.length} notification(s) as read`); + } },
420-421: Nit: Consider simplifying the wrapper.The wrapper function
(ids: number[]) => markNotificationIdsAsRead(ids)could be simplified to justmarkNotificationIdsAsRead, unless the intent is to restrict the consumer from passing mutation options.♻️ Simplified return
- markNotificationIdsAsRead: (ids: number[]) => - markNotificationIdsAsRead(ids), + markNotificationIdsAsRead,
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/brain/notifications/NotificationsFollowBtn.tsx (1)
49-54:⚠️ Potential issue | 🔴 CriticalMissing import and inconsistent usage of
DEFAULT_SUBSCRIPTION_BODY.
ApiIdentitySubscriptionTargetActionis used here but not imported. Additionally,unFollowMutation(line 77) already usesDEFAULT_SUBSCRIPTION_BODYwhich contains the exact same logic. Use the shared constant for consistency and to fix the missing import.🔧 Proposed fix
body: { - actions: Object.values(ApiIdentitySubscriptionTargetAction).filter( - (i) => i !== ApiIdentitySubscriptionTargetAction.DropVoted - ), - }, + ...DEFAULT_SUBSCRIPTION_BODY, + },Or more simply:
- body: { - actions: Object.values(ApiIdentitySubscriptionTargetAction).filter( - (i) => i !== ApiIdentitySubscriptionTargetAction.DropVoted - ), - }, + body: DEFAULT_SUBSCRIPTION_BODY,
🧹 Nitpick comments (1)
components/brain/notifications/NotificationsFollowAllBtn.tsx (1)
49-67: Consider handling partial failures in batch follow.
Promise.allrejects on the first failure, so some profiles may be followed while others fail. The error toast won't indicate which profiles were successfully followed. For a more resilient UX, considerPromise.allSettledto process all requests and report partial success/failure.♻️ Optional improvement using Promise.allSettled
try { - await Promise.all( + const results = await Promise.allSettled( toFollow.map((profile) => commonApiPost< ApiIdentitySubscriptionActions, ApiIdentitySubscriptionActions >({ endpoint: `identities/${profile.handle}/subscriptions`, body: DEFAULT_SUBSCRIPTION_BODY, }) ) ); + const failures = results.filter((r) => r.status === "rejected"); + if (failures.length > 0 && failures.length < toFollow.length) { + setToast({ + message: `Followed ${toFollow.length - failures.length} of ${toFollow.length} profiles`, + type: "warning", + }); + } else if (failures.length === toFollow.length) { + throw new Error("Failed to follow all profiles"); + } onIdentityFollowChange(); - } catch (error) {
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@components/brain/notifications/NotificationsFollowAllBtn.tsx`:
- Around line 49-69: Replace Promise.all with Promise.allSettled for the
toFollow.map(...) calls (which use commonApiPost and DEFAULT_SUBSCRIPTION_BODY)
and then inspect the returned results: if any result.status === "fulfilled" call
onIdentityFollowChange(), and if any are "rejected" aggregate their error
messages and pass a combined message to setToast({ message: ..., type: "error"
}); ensure you still call setMutating(false) in finally/after handling so the
mutating state is cleared regardless of partial failures.
🧹 Nitpick comments (1)
components/waves/list/WaveItemDropped.tsx (1)
8-33: Slice contributors before mapping to avoid extra work.You’re mapping the full list even though the avatar component will slice to 5. Pre-slicing reduces allocations and work on large lists.
♻️ Suggested refactor
export default function WaveItemDropped({ wave }: { readonly wave: ApiWave }) { const contributors = wave.contributors_overview ?? []; + const maxAvatars = 5; + const visibleContributors = contributors.slice(0, maxAvatars); return ( <div className="tw-flex tw-min-w-0 tw-items-center tw-gap-x-2"> <div className="tw-hidden tw-flex-shrink @[320px]/wave:tw-flex"> <OverlappingAvatars - items={contributors.map((c, index) => { + items={visibleContributors.map((c, index) => { const href = c.contributor_identity ? `/${c.contributor_identity}` : undefined; return { key: `${c.contributor_identity ?? "anon"}-${c.contributor_pfp ?? "no-pfp"}-${index}`, pfpUrl: c.contributor_pfp ?? null, ...(href !== undefined && { href }), ariaLabel: c.contributor_identity ? `View @${c.contributor_identity}` : "View contributor profile", fallback: c.contributor_identity?.slice(0, 2).toUpperCase() ?? "?", }; })} - maxCount={5} + maxCount={maxAvatars} size="sm" overlapClass="-tw-space-x-1" onItemClick={(e) => e.stopPropagation()} /> </div>
|
2 participants
Summary by CodeRabbit