Conversation
📝 WalkthroughWalkthroughThis pull request adds support for a new "DropBoosted" notification type across the notification system. Changes include adding the new cause to the API schema and enum, creating corresponding TypeScript type definitions, wiring it through notification components (NotificationItem, NotificationItems, NotificationDropReacted), updating filter options, and refactoring NotificationsCauseFilter to use direct DOM manipulation for highlight positioning with ResizeObserver support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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
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: 1
🤖 Fix all issues with AI agents
In `@openapi.yaml`:
- Around line 7425-7455: The ApiMintMetrics schema uses invalid OpenAPI types
(type: number with format: int64) for integer fields; update each property in
ApiMintMetrics (card, mint_time, subscriptions, mints) to use type: integer and
keep format: int64 (or remove format if you prefer plain integers), ensuring
ApiMintMetricsPage continues to reference ApiMintMetrics unchanged; this will
make the schema OpenAPI-compliant and compatible with strict validators and
codegen.
🧹 Nitpick comments (3)
types/feed.types.ts (1)
77-85: AvoidRecord<string, any>foradditional_context; preferunknown(or a typed shape).If the payload is truly opaque,
Record<string, unknown>is safer; otherwise define explicit fields (even if optional) so UI handling can stay type-safe.Proposed fix
export type INotificationDropBoosted = { readonly id: number; readonly cause: ApiNotificationCause.DropBoosted; readonly created_at: number; readonly read_at: number | null; readonly related_identity: ApiProfileMin; readonly related_drops: Array<ApiDrop>; - readonly additional_context: Record<string, any>; + readonly additional_context: Record<string, unknown>; };components/brain/notifications/drop-reacted/NotificationDropReacted.tsx (1)
54-61: Type narrowing relies on correct conditional guards.The cause-based checks using
ApiNotificationCauseenum values are cleaner than string-based checks. TypeScript should narrow thenotificationtype within each branch, allowing safe access toadditional_context.votewhenisVotedis true.However, since
isVoted,isReacted, andisBoostedare separate boolean variables rather than inline checks, TypeScript may not narrow the union type automatically. Consider using inline checks or type guards for stronger type safety:💡 Optional: Use inline checks for better type narrowing
- const isVoted = notification.cause === ApiNotificationCause.DropVoted; - const isReacted = notification.cause === ApiNotificationCause.DropReacted; - const isBoosted = notification.cause === ApiNotificationCause.DropBoosted; - let actionElement: React.ReactNode = null; - if (isVoted) { - const voteValue = notification.additional_context.vote; + if (notification.cause === ApiNotificationCause.DropVoted) { + const voteValue = notification.additional_context.vote; // ... rest of voted branch - } else if (isBoosted) { + } else if (notification.cause === ApiNotificationCause.DropBoosted) { // ... boosted branch - } else if (isReacted) { - const rawId = notification.additional_context.reaction.replaceAll(":", ""); + } else if (notification.cause === ApiNotificationCause.DropReacted) { + const rawId = notification.additional_context.reaction.replaceAll(":", ""); // ... rest of reacted branchcomponents/brain/notifications/NotificationsCauseFilter.tsx (1)
148-150: Avoid non-null assertion on ref callback.The
el!non-null assertion is unsafe because React passesnullto ref callbacks when elements unmount. While this may work in practice because the array isn't accessed after unmount, it's cleaner to guard against null:💡 Suggested fix
- buttonRef={(el) => (buttonRefs.current[index] = el!)} + buttonRef={(el) => { + if (el) buttonRefs.current[index] = el; + }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
generated/models/ApiNotificationCause.tsis excluded by!**/generated/**
📒 Files selected for processing (7)
components/brain/notifications/NotificationItem.tsxcomponents/brain/notifications/NotificationItems.tsxcomponents/brain/notifications/NotificationsCauseFilter.tsxcomponents/brain/notifications/drop-reacted/NotificationDropReacted.tsxcomponents/brain/notifications/index.tsxopenapi.yamltypes/feed.types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
types/feed.types.ts (3)
generated/models/ApiProfileMin.ts (1)
ApiProfileMin(17-150)generated/models/ApiDrop.ts (1)
ApiDrop(29-244)generated/models/ApiNotificationsResponse.ts (1)
ApiNotificationsResponse(17-45)
components/brain/notifications/drop-reacted/NotificationDropReacted.tsx (3)
types/feed.types.ts (3)
INotificationDropVoted(53-63)INotificationDropReacted(65-75)INotificationDropBoosted(77-85)helpers/Helpers.ts (1)
numberWithCommas(114-131)components/brain/notifications/subcomponents/NotificationTimestamp.tsx (1)
NotificationTimestamp(7-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
openapi.yaml (1)
7701-7713:ApiNotificationCause+DROP_BOOSTEDlooks consistent with existing enum style.types/feed.types.ts (2)
149-159: Nice:TypedNotificationnow coversINotificationDropBoosted.This keeps downstream switch rendering strongly typed.
161-166:TypedNotificationsResponsewrapper is a clean way to re-typenotifications.components/brain/notifications/NotificationItem.tsx (1)
53-56: DropBoosted routing viaNotificationDropReactedis correct with full prop-type coverage.The
NotificationDropReactedcomponent importsINotificationDropBoosted, includes it in theNotificationUniontype, and explicitly handles the boosted case (line 56:const isBoosted = notification.cause === ApiNotificationCause.DropBoosted;). All DropBoosted enum usage is consistent across the codebase.components/brain/notifications/NotificationItems.tsx (1)
1-6: LGTM!The import reordering places type imports before runtime imports, which is a good practice for code organization. No functional changes to the component logic.
components/brain/notifications/index.tsx (2)
16-27: LGTM!The priority mapping correctly integrates
DropBoostedwith a sensible priority value (5) betweenDropVoted(4) andDropReacted(6). All enum values are accounted for with consecutive priorities.
74-89: LGTM!The structural and styling updates improve the layout consistency. The
WebkitOverflowScrolling: "touch"style is a good addition for smoother scrolling on iOS devices.components/brain/notifications/drop-reacted/NotificationDropReacted.tsx (2)
79-87: LGTM!The boosted notification UI follows the established pattern of other notification types, with a descriptive label and timestamp. The "🔥 boosted 🔥" visual indicator is clear and consistent with the notification styling.
30-33: LGTM!The
NotificationUniontype correctly includesINotificationDropBoostedalongside the existing voted and reacted types, ensuring type safety for the component's props.components/brain/notifications/NotificationsCauseFilter.tsx (3)
25-29: LGTM!Adding
DropBoostedto the Reactions filter alongsideDropVotedandDropReactedis semantically appropriate, as boosting is a form of positive reaction to content.
80-100: LGTM with a minor observation.The
useLayoutEffectwithResizeObserveris well-implemented for handling layout shifts during resize, font loading, etc. The cleanup properly disconnects the observer.Note: The effect has
activeFilterin its dependency array but also referencesupdateHighlightPositionwhich closes overbuttonRefsandhighlightRef. Since these are stable refs, this is fine, but ifupdateHighlightPositionwere to change, it would need to be added to dependencies.
59-78: Direct DOM manipulation is appropriate here.The switch from React state to direct DOM manipulation for the highlight animation is a valid optimization—it avoids re-renders during animation transitions. The edge-case adjustments for first/last buttons (lines 66-72) handle the border spacing correctly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.