packages/database/src/collections/schemas/notifications.ts (1)

1-57: Consider specifying required fields.

The schema is well-structured with good documentation. A few observations:

1. Missing required array: RxDB treats all properties as optional by default. Consider adding a required array for essential fields like id, subscriberId, and createdAt to ensure data integrity.

2. Unbounded title and body: These string fields have no maxLength. If Novu could potentially send very long content, consider adding reasonable limits to prevent storage bloat.

💡 Suggested improvement

 export const notificationsLiteral = {
 	title: 'Notification schema',
 	version: 0,
 	description: 'In-app notifications from Novu',
 	type: 'object',
 	primaryKey: 'id',
+	required: ['id', 'subscriberId', 'createdAt'],
 	properties: {
 		// ... existing properties
 		title: {
 			description: 'Notification title',
 			type: 'string',
+			maxLength: 500,
 		},
 		body: {
 			description: 'Notification body/message',
 			type: 'string',
+			maxLength: 5000,
 		},

packages/components/src/badge/index.tsx (1)

89-92: Dot mode ignores the variant prop.

The dot indicator is hardcoded to use bg-destructive, while the count badge respects the variant prop. If this is intentional (dots should always be red), consider documenting this behavior. Otherwise, consider applying the variant:

♻️ Optional: Apply variant to dot mode

 	// Dot mode - just show a small indicator
 	if (dot) {
-		return <View className={cn('bg-destructive h-2.5 w-2.5 rounded-full', className)} {...props} />;
+		return <View className={cn(badgeVariants({ variant: variant ?? 'destructive', size: 'sm' }), 'h-2.5 w-2.5 p-0', className)} {...props} />;
 	}

packages/core/src/services/novu/subscriber.ts (1)

32-43: Potential for malformed subscriber ID if nested properties are missing.

If store.id, store.localID, or wpCredentials.uuid are undefined, the subscriber ID will contain the literal string "undefined". While the NovuProvider checks for the existence of parent objects, it doesn't validate these nested properties.

Consider adding validation or fallbacks:

♻️ Proposed validation

 export function generateSubscriberId(
 	site: SiteDocument,
 	store: StoreDocument,
 	wpCredentials: WPCredentialsDocument
 ): string {
 	// Extract domain from site URL (remove protocol)
 	const domain = extractDomain(site.url || '');
 	const storeId = store.id || store.localID;
 	const userUuid = wpCredentials.uuid;
 
+	if (!domain || !storeId || !userUuid) {
+		throw new Error('Missing required fields for subscriber ID generation');
+	}
+
 	return `${domain}:${storeId}:${userUuid}`;
 }

packages/core/src/services/novu/client.ts (1)

50-81: The _metadata parameter is unused.

The _metadata parameter is passed but never used in client initialization. Either remove it or implement metadata registration with Novu (if the SDK supports it).

packages/core/src/contexts/novu/index.tsx (1)

14-18: Duplicate NOVU_CONFIG definition.

This configuration is also defined in packages/core/src/services/novu/client.ts. Consider extracting to a shared constants file to avoid inconsistencies if URLs need to change.

♻️ Extract to shared config

Create a shared config file:

// packages/core/src/services/novu/config.ts
export const NOVU_CONFIG = {
	applicationIdentifier: '64qzhASJJNnb',
	backendUrl: 'https://api.notifications.wcpos.com',
	socketUrl: 'wss://ws.notifications.wcpos.com',
};

Then import from both files.

packages/core/src/hooks/use-novu-notifications.ts (3)

149-156: Sequential notification sync could be slow for large lists.

Processing notifications one-by-one with await inside a loop is O(n) sequential operations. Consider using Promise.all for parallel processing or RxDB's bulk operations if available.

♻️ Parallel sync option

 	const syncToRxDB = React.useCallback(
 		async (novuNotifications: NovuNotification[]) => {
-			for (const notification of novuNotifications) {
-				await syncNotificationToRxDB(notification);
-			}
+			await Promise.all(
+				novuNotifications.map((notification) => syncNotificationToRxDB(notification))
+			);
 		},
 		[syncNotificationToRxDB]
 	);

---

209-228: No rollback if server update fails after local update.

The optimistic update pattern updates RxDB first for immediate UI feedback, but if novuMarkAsRead fails, the local state becomes inconsistent with the server. Consider reverting the local change on failure:

♻️ Add rollback on failure

 	const markAsRead = React.useCallback(
 		async (notificationId: string) => {
 			if (!notificationsCollection) return;
 
 			try {
 				// Update RxDB first for immediate UI feedback
 				const doc = await notificationsCollection.findOne(notificationId).exec();
+				const previousStatus = doc?.status;
+				const previousSeen = doc?.seen;
 				if (doc) {
 					await doc.patch({ status: 'read', seen: true });
 				}
 
 				// Then update Novu
-				await novuMarkAsRead(notificationId);
+				const success = await novuMarkAsRead(notificationId);
+				if (!success && doc && previousStatus !== undefined) {
+					// Rollback on failure
+					await doc.patch({ status: previousStatus, seen: previousSeen ?? false });
+					log.warn('Novu: Rolled back local state after server failure');
+				}
 			} catch (error) {
 				log.error('Novu: Failed to mark notification as read', { context: { error } });
 			}
 		},
 		[notificationsCollection]
 	);

---

188-199: Missing error handling for initial fetch.

The .then() chain doesn't have a .catch() handler. While fetchNotifications logs errors internally and returns an empty array, any error in syncToRxDB would be unhandled.

♻️ Add catch handler

 		// Initial fetch of notifications
 		setIsLoading(true);
 		fetchNotifications()
 			.then((notifications) => {
 				log.info('Novu: Initial notifications loaded', {
 					context: { count: notifications.length },
 				});
 				return syncToRxDB(notifications);
 			})
+			.catch((error) => {
+				log.error('Novu: Failed during initial sync', { context: { error } });
+			})
 			.finally(() => {
 				setIsLoading(false);
 			});

packages/core/src/services/novu/client.ts (2)

97-102: Previous review feedback not addressed: proper client cleanup is needed.

Simply nulling the novuClient reference does not properly close the WebSocket connection. As noted in a previous review, the @novu/js SDK may require an explicit cleanup method (e.g., unmountComponent() or similar) to disconnect the socket and prevent memory leaks.

`@novu/js` SDK v3 disconnect cleanup method

packages/core/src/services/novu/client.ts (1)

97-103: Use the SDK’s explicit unmount/cleanup API (don’t rely on socket.disconnect alone).
If the Novu SDK exposes unmountComponent() (or equivalent), it should be called to fully release UI/socket resources before nulling the client.

🔧 Proposed fix (verify SDK API first)

-		novuClient.socket.disconnect();
+		novuClient.unmountComponent?.();
+		novuClient.socket?.disconnect?.();

Does the `@novu/js` SDK provide an unmount/cleanup method (e.g., unmountComponent) that should be called to fully disconnect the UI/WebSocket?

Also applies to: 352-357

packages/core/src/screens/main/components/header/notification-bell.tsx (1)

16-23: Use the hook’s exported function name (likely markAllAsRead) to avoid runtime errors.

This component calls markAllAsSeen, but earlier review found only markAllAsRead exported from the hook. If that mismatch still exists, opening the popover will throw. Please verify the hook’s return shape and align the call (and consider guarding on unreadCount).

🛠️ Proposed fix

-export function NotificationBell() {
-	const { unreadCount, markAllAsSeen } = useNovuNotifications();
+export function NotificationBell() {
+	const { unreadCount, markAllAsRead } = useNovuNotifications();
 	const [isOpen, setIsOpen] = React.useState(false);

 	const handleOpenChange = (open: boolean) => {
 		setIsOpen(open);
-		if (open) {
-			markAllAsSeen();
+		if (open && unreadCount > 0) {
+			markAllAsRead();
 		}
 	};

Run this to confirm the hook’s exported field name:

#!/bin/bash
# Verify which mark-all function is returned by the hook
rg -nP "markAllAs(Read|Seen)" packages/core/src/hooks/use-novu-notifications.ts
rg -nP "return\s*{[^}]*markAllAs(Read|Seen)" packages/core/src/hooks/use-novu-notifications.ts

packages/core/src/hooks/use-novu-notifications.ts (1)

233-260: Prevent createdAt from becoming NaN before upsert.
notification.createdAt can be missing/malformed, and new Date(...).getTime() can yield NaN, which breaks RxDB sorting/indexing. (Duplicate of prior review note.)

🛠️ Suggested defensive conversion

-			try {
-				await notificationsCollection.upsert({
+			try {
+				const createdAtMs = notification.createdAt
+					? new Date(notification.createdAt).getTime()
+					: 0;
+				await notificationsCollection.upsert({
 					id: String(notificationId),
 					subscriberId,
 					title: notification.subject || '',
 					body: notification.body || '',
 					status: notification.isRead ? 'read' : 'unread',
 					seen: notification.isSeen ?? false,
-					createdAt: new Date(notification.createdAt).getTime(),
+					createdAt: Number.isFinite(createdAtMs) ? createdAtMs : 0,
 					// v3 uses `data` instead of `payload`
 					payload: (notification.data as NotificationPayload) || {},
 					channel: notification.channelType || 'in_app',
 				});

packages/core/src/hooks/use-novu-notifications.ts (1)

337-405: Consider reconciling RxDB state when Novu mutations fail.
Local optimistic updates can diverge from server state if novuMarkAsRead/Seen fails. Consider rollback or a follow-up refresh to realign local state.

packages/core/src/hooks/use-novu-notifications.ts (3)

157-180: Guard against invalid level values before calling log[level].

While behavior.level is typed as 'info' | 'success' | 'warn' | 'error', runtime data could contain unexpected values. The previous review recommended validating the level before using it as a dynamic property access:

 if (behavior.showToast) {
-    const level = behavior.level || 'info';
+    const rawLevel = behavior.level || 'info';
+    const level = ['info', 'success', 'warn', 'error'].includes(rawLevel) ? rawLevel : 'info';
     log[level](title, {

This prevents TypeError: log[level] is not a function if an unexpected value slips through.

---

217-229: Guard against invalid createdAt to prevent NaN in RxDB.

The notification.createdAt field from Novu is optional and could be missing or malformed. new Date(undefined).getTime() returns NaN, which breaks RxDB sorting on the indexed createdAt field.

Add defensive conversion:

+const createdAtMs = notification.createdAt
+    ? new Date(notification.createdAt).getTime()
+    : Date.now();
+const validCreatedAt = Number.isFinite(createdAtMs) ? createdAtMs : Date.now();
+
 await notificationsCollection.upsert({
     id: String(notificationId),
     subscriberId,
     title: notification.subject || '',
     body: notification.body || '',
     status: notification.isRead ? 'read' : 'unread',
     seen: notification.isSeen ?? false,
-    createdAt: new Date(notification.createdAt).getTime(),
+    createdAt: validCreatedAt,
     workflowId,
     channel: notification.channelType || 'in_app',
 });

---

330-341: Handle initial fetch failures to avoid unhandled promise rejections.

The promise chain for fetchNotifications() lacks a .catch() handler. If the fetch fails, the rejection could surface as an unhandled promise rejection.

 fetchNotifications()
     .then((notifications) => {
         log.info('Novu: Initial notifications loaded', {
             context: { count: notifications.length },
         });
         return syncToRxDB(notifications);
     })
+    .catch((error) => {
+        log.error('Novu: Failed to load initial notifications', {
+            context: { error: error instanceof Error ? error.message : String(error) },
+        });
+    })
     .finally(() => {
         setIsLoading(false);
     });

packages/core/src/services/novu/notification-behaviors.ts (1)

60-118: Consider stricter typing for workflow behavior keys.

The WORKFLOW_BEHAVIORS uses Record<string, NotificationBehavior> which allows any string key. Consider using a stricter type to ensure only known workflow IDs are used:

const WORKFLOW_BEHAVIORS: Partial<Record<typeof WORKFLOW_IDS[keyof typeof WORKFLOW_IDS], NotificationBehavior>> = {

This would catch typos at compile time. However, the current approach works fine given the controlled access via WORKFLOW_IDS constants.

packages/core/src/hooks/use-novu-notifications.ts (1)

271-274: isConnected set to true before WebSocket is actually ready.

setIsConnected(true) is called immediately after getNovuClient(), but the WebSocket connection is established asynchronously. Consider setting isConnected to true only after waitForNovuReady() resolves successfully:

 getNovuClient(subscriberId, subscriberMetadata);
-setIsConnected(true);
+// isConnected will be set after WebSocket is ready

Then in the waitForNovuReady callback:

waitForNovuReady(5000).then((connected) => {
    setIsConnected(connected);
    // ...
});

This provides more accurate connection state to consumers.