feat: notification system #275
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a notification banner system with a configuration module, a new React component to display notifications, custom hooks for tracking active notifications and managing conditions, a persisted store for dismissed notification IDs, and integration into the Header component. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Header
participant Banner as NotificationBanner
participant Active as useActiveNotifications
participant Conditions as useNotificationConditions
participant Store as useNotificationStore
participant Config as NOTIFICATIONS config
Header->>Active: render, call hook
Active->>Config: read notifications list
Active->>Conditions: get condition results (Map)
Conditions-->>Active: Map<conditionId, ConditionResult>
Active->>Store: check dismissedIds
Store-->>Active: set of dismissed IDs
Note over Active: Filter by:<br/>expiry, dismissal,<br/>conditions, loading
Active-->>Header: activeNotifications[]
Header->>Banner: pass currentNotification
Banner->>Banner: render banner with message,<br/>icon, action, close button
User->>Banner: click dismiss/close
Banner->>Store: dismiss(notificationId)
Store->>Store: add to dismissedIds
Note over Store: persist to localStorage
Store-->>Banner: confirmed
Banner->>Active: hook updates
Active-->>Banner: next notification or hide
Banner->>Banner: re-render or hide
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (3)
src/stores/useNotificationStore.ts (1)
36-58: Consider adding version for future migrations.The persist middleware supports a
versionfield andmigratefunction for handling breaking changes to the store schema. SincesetAllis already documented for migration use, consider adding version tracking now:{ name: 'monarch_store_notifications', + version: 1, + migrate: (persistedState: any, version: number) => { + if (version === 0) { + // Example: migrate old format to new format + return persistedState; + } + return persistedState; + }, },This makes future schema changes easier to handle.
src/hooks/useNotificationConditions.ts (1)
24-40: Add dependencies to useMemo when implementing conditions.The empty dependency array means the map is created once and never updates. When you add conditions that use hooks (like
useConnection,useSomeDataHook, etc.), include those dependencies in the array to keep the conditions in sync with their data.src/config/notifications.ts (1)
11-28: Consider enforcing conditionId for personalized notifications.The type allows
category: 'personalized'without aconditionId, which doesn't match the stated intent (line 27). A discriminated union would prevent this misconfiguration.🔎 Proposed type refinement
-export type NotificationConfig = { +type BaseNotificationConfig = { /** Unique identifier for persistence */ id: string; /** Message to display in the banner */ message: string; /** Optional custom icon (ReactNode) */ icon?: ReactNode; /** Notification type for styling */ type: NotificationType; /** Optional action button */ action?: NotificationAction; /** Optional expiration date - notification auto-hides after this */ expiresAt?: Date; - /** Category: global (all users) or personalized (condition-based) */ - category: 'global' | 'personalized'; - /** For personalized notifications, maps to a condition in useNotificationConditions */ - conditionId?: string; }; + +export type NotificationConfig = + | (BaseNotificationConfig & { category: 'global' }) + | (BaseNotificationConfig & { category: 'personalized'; conditionId: string });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/layout/header/Header.tsxsrc/components/layout/notification-banner.tsxsrc/config/notifications.tssrc/hooks/useActiveNotifications.tssrc/hooks/useNotificationConditions.tssrc/stores/useNotificationStore.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T10:06:39.848Z
Learnt from: antoncoding
Repo: antoncoding/monarch PR: 231
File: src/hooks/useDeployMorphoMarketV1Adapter.ts:3-3
Timestamp: 2025-12-09T10:06:39.848Z
Learning: In Wagmi v3, useConnection is the correct hook to obtain the connected wallet address, chainId, and connection status (isConnected). This replaces the useAccount hook from Wagmi v2. In your code under src/hooks, use: const { address, chainId, isConnected } = useConnection() from 'wagmi' to access the connected wallet information.
Applied to files:
src/hooks/useNotificationConditions.tssrc/hooks/useActiveNotifications.ts
🧬 Code graph analysis (3)
src/components/layout/notification-banner.tsx (2)
src/hooks/useActiveNotifications.ts (1)
useActiveNotifications(37-96)src/stores/useNotificationStore.ts (1)
useNotificationStore(36-58)
src/hooks/useActiveNotifications.ts (3)
src/config/notifications.ts (2)
NotificationConfig(11-28)NOTIFICATIONS(37-50)src/stores/useNotificationStore.ts (1)
useNotificationStore(36-58)src/hooks/useNotificationConditions.ts (1)
useNotificationConditions(24-40)
src/components/layout/header/Header.tsx (1)
src/components/layout/notification-banner.tsx (2)
useNotificationBannerVisible(89-92)NotificationBanner(8-83)
🔇 Additional comments (14)
src/components/layout/header/Header.tsx (3)
5-5: LGTM!Import is clean and both exports are used.
15-15: LGTM!The dynamic spacer logic correctly doubles the height when the banner is visible.
Also applies to: 37-38
43-43: LGTM!The banner is correctly positioned within the fixed header, and the spacer prevents content overlap.
Also applies to: 53-54
src/hooks/useNotificationConditions.ts (1)
3-7: LGTM!Type definition is clear and matches the documented use case.
src/components/layout/notification-banner.tsx (4)
8-15: LGTM!Early return logic correctly prevents rendering during loading or when no notifications are active.
17-19: LGTM!Action handling correctly supports both Link navigation and button onClick, with proper dismissal in both cases.
Also applies to: 47-68
23-82: Verify close button doesn't overlap content on narrow viewports.The close button is positioned absolutely at
right-4, while the content container usespx-4padding. On very narrow screens with long messages or action labels, the content might extend under the close button.Consider adding right padding to the content container to reserve space for the close button:
- <div className="container mx-auto flex items-center justify-center gap-4 px-4 sm:px-6 md:px-8"> + <div className="container mx-auto flex items-center justify-center gap-4 px-4 pr-12 sm:px-6 sm:pr-14 md:px-8 md:pr-16">This ensures content never overlaps with the close button.
85-92: LGTM!The visibility hook correctly mirrors the banner's rendering condition.
src/stores/useNotificationStore.ts (2)
4-18: LGTM!Type definitions are clear and well-documented.
41-50: LGTM!Dismiss logic correctly prevents duplicates and uses immutable updates.
src/hooks/useActiveNotifications.ts (3)
6-17: LGTM!Return type is well-defined and documented.
37-84: Verify dependency array stability.The filtering logic is correct, but the dependency array
[isDismissed, conditions]might cause unnecessary re-renders:
isDismissedis a function selector from Zustand. It may have stable reference, but verify this doesn't create new instances on every store update.conditionsMap fromuseNotificationConditionscurrently has an empty deps array, so it's stable. When conditions are added with reactive dependencies, verify the Map reference changes only when needed.Test that adding/dismissing notifications doesn't cause excessive re-computations. If it does, consider:
- Using
useShallowfrom Zustand for the selector- Memoizing the conditions Map more carefully when reactive values are added
86-95: LGTM!Return value computation correctly derives all fields from the filtered notifications.
src/config/notifications.ts (1)
37-50: LGTM.Empty array with clear documentation is appropriate for initial setup. The commented example demonstrates proper usage.
| export type NotificationAction = { | ||
| label: string; | ||
| href?: string; | ||
| onClick?: () => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make NotificationAction type-safe.
The type allows actions with neither href nor onClick (button does nothing) or both (ambiguous behavior). Use a discriminated union to enforce exactly one action type.
🔎 Proposed type-safe structure
-export type NotificationAction = {
- label: string;
- href?: string;
- onClick?: () => void;
-};
+export type NotificationAction =
+ | { label: string; href: string }
+ | { label: string; onClick: () => void };🤖 Prompt for AI Agents
In src/config/notifications.ts around lines 5 to 9, the NotificationAction type
currently allows actions with neither or both href and onClick; replace it with
a discriminated union that enforces exactly one action shape (e.g., { type:
'link'; label: string; href: string } | { type: 'button'; label: string;
onClick: () => void }) and update any consumers to switch on the discriminant
(or check the type field) when rendering/handling actions; ensure label remains
required and adjust imports/usage sites to handle the new union so the compiler
enforces a single valid action per NotificationAction.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.