Refactir deep
This commit is contained in:
parent
e81b31c9ee
commit
894fb304ce
239
NOTIFICATION_FIXES_IMPLEMENTED.md
Normal file
239
NOTIFICATION_FIXES_IMPLEMENTED.md
Normal file
@ -0,0 +1,239 @@
|
||||
# Notification System Fixes - Implementation Summary
|
||||
|
||||
**Date**: 2026-01-06
|
||||
**Status**: ✅ All fixes implemented
|
||||
|
||||
---
|
||||
|
||||
## ✅ **Fix #1: Redis Caching for Leantime User ID**
|
||||
|
||||
### **Problem**:
|
||||
- `getLeantimeUserId()` fetched ALL users from Leantime API every time
|
||||
- No caching, causing slow performance and inconsistent results
|
||||
- Race conditions between different calls
|
||||
|
||||
### **Solution**:
|
||||
- Added Redis caching with 1-hour TTL
|
||||
- Cache key: `leantime:userid:${email.toLowerCase()}`
|
||||
- Checks cache first before making API call
|
||||
- Caches result after successful fetch
|
||||
|
||||
### **Implementation**:
|
||||
- **File**: `lib/services/notifications/leantime-adapter.ts`
|
||||
- **Method**: `getLeantimeUserId()`
|
||||
- **Cache TTL**: 3600 seconds (1 hour)
|
||||
- **Static helper**: `invalidateUserIdCache()` for manual cache clearing
|
||||
|
||||
### **Benefits**:
|
||||
- ✅ Faster performance (no API call if cached)
|
||||
- ✅ More reliable (consistent results)
|
||||
- ✅ Reduced API load on Leantime
|
||||
- ✅ Better error recovery (can use cached value if API fails)
|
||||
|
||||
---
|
||||
|
||||
## ✅ **Fix #2: Retry Logic with Exponential Backoff**
|
||||
|
||||
### **Problem**:
|
||||
- `getLeantimeUserId()` failed immediately on API errors
|
||||
- No retry mechanism for transient failures
|
||||
- Network errors caused permanent failures
|
||||
|
||||
### **Solution**:
|
||||
- Added retry logic with up to 3 retries
|
||||
- Exponential backoff: 1s, 2s, 4s (max 5s)
|
||||
- Retries on:
|
||||
- Server errors (5xx)
|
||||
- Rate limiting (429)
|
||||
- Network errors
|
||||
- Certain JSON-RPC errors
|
||||
|
||||
### **Implementation**:
|
||||
- **File**: `lib/services/notifications/leantime-adapter.ts`
|
||||
- **Method**: `getLeantimeUserId()` with `fetchWithRetry()`
|
||||
- **Max Retries**: 3
|
||||
- **Backoff**: Exponential (1s → 2s → 4s)
|
||||
|
||||
### **Benefits**:
|
||||
- ✅ Handles transient failures gracefully
|
||||
- ✅ Better resilience to network issues
|
||||
- ✅ Improved success rate for user ID lookup
|
||||
|
||||
---
|
||||
|
||||
## ✅ **Fix #3: Always Invalidate Cache After Marking**
|
||||
|
||||
### **Problem**:
|
||||
- Cache only invalidated if marking operation succeeded
|
||||
- If `getLeantimeUserId()` failed, cache stayed stale
|
||||
- Count remained at old value (65) even after marking attempts
|
||||
|
||||
### **Solution**:
|
||||
- Always invalidate cache after marking attempt
|
||||
- Even if operation failed or returned `false`
|
||||
- Ensures fresh data on next fetch
|
||||
|
||||
### **Implementation**:
|
||||
- **File**: `lib/services/notifications/notification-service.ts`
|
||||
- **Methods**:
|
||||
- `markAsRead()` - Always invalidates cache
|
||||
- `markAllAsRead()` - Always invalidates cache
|
||||
- **Logic**: Cache invalidation happens regardless of success/failure
|
||||
|
||||
### **Benefits**:
|
||||
- ✅ Count always refreshes after marking attempts
|
||||
- ✅ User sees accurate data even if operation partially failed
|
||||
- ✅ Better UX (no stale count stuck at 65)
|
||||
|
||||
---
|
||||
|
||||
## ✅ **Fix #4: Improved Count Accuracy**
|
||||
|
||||
### **Problem**:
|
||||
- Count only based on first 100 notifications
|
||||
- If user had >100 notifications, count was inaccurate
|
||||
- Used cached notifications which might be stale
|
||||
|
||||
### **Solution**:
|
||||
- Fetch up to 1000 notifications directly from API for counting
|
||||
- Bypasses cache to get fresh data
|
||||
- More accurate count for users with many notifications
|
||||
|
||||
### **Implementation**:
|
||||
- **File**: `lib/services/notifications/leantime-adapter.ts`
|
||||
- **Method**: `getNotificationCount()`
|
||||
- **Change**: Fetches directly from API (up to 1000) instead of using cached `getNotifications()`
|
||||
- **Warning**: Logs if count reaches 1000 (might have more)
|
||||
|
||||
### **Benefits**:
|
||||
- ✅ More accurate count (up to 1000 notifications)
|
||||
- ✅ Fresh data (bypasses cache)
|
||||
- ✅ Better handling of users with many notifications
|
||||
|
||||
---
|
||||
|
||||
## ✅ **Fix #5: Better Error Handling and Logging**
|
||||
|
||||
### **Problem**:
|
||||
- Errors were logged but not handled gracefully
|
||||
- No way to manually clear user ID cache
|
||||
- Limited error context in logs
|
||||
|
||||
### **Solution**:
|
||||
- Added static method to invalidate user ID cache
|
||||
- Improved error messages with more context
|
||||
- Better logging throughout the flow
|
||||
- Graceful degradation on errors
|
||||
|
||||
### **Implementation**:
|
||||
- **File**: `lib/services/notifications/leantime-adapter.ts`
|
||||
- **Static Method**: `invalidateUserIdCache(email)`
|
||||
- **Improved Logging**: More detailed error messages
|
||||
- **Error Recovery**: Continues operation even if caching fails
|
||||
|
||||
### **Benefits**:
|
||||
- ✅ Better debugging with detailed logs
|
||||
- ✅ Manual cache clearing for troubleshooting
|
||||
- ✅ More resilient to partial failures
|
||||
|
||||
---
|
||||
|
||||
## 📊 **Expected Behavior After Fixes**
|
||||
|
||||
### **Before Fixes**:
|
||||
1. Mark all as read → `getLeantimeUserId()` fails → Returns `false`
|
||||
2. Cache NOT invalidated → Count stays 65 ❌
|
||||
3. User sees stale count
|
||||
|
||||
### **After Fixes**:
|
||||
1. Mark all as read → `getLeantimeUserId()` checks cache first ✅
|
||||
2. If cached: Uses cached ID immediately ✅
|
||||
3. If not cached: Fetches with retry logic ✅
|
||||
4. Marks notifications as read ✅
|
||||
5. **Always invalidates cache** ✅
|
||||
6. Count refresh gets fresh data → Shows 0 ✅
|
||||
|
||||
---
|
||||
|
||||
## 🎯 **Key Improvements**
|
||||
|
||||
### **Reliability**:
|
||||
- ✅ User ID lookup is now cached and retried
|
||||
- ✅ Cache always invalidated after marking
|
||||
- ✅ Better error recovery
|
||||
|
||||
### **Performance**:
|
||||
- ✅ Faster user ID lookup (cached)
|
||||
- ✅ Reduced API calls to Leantime
|
||||
- ✅ More efficient cache usage
|
||||
|
||||
### **Accuracy**:
|
||||
- ✅ Count based on up to 1000 notifications
|
||||
- ✅ Fresh data from API (bypasses stale cache)
|
||||
- ✅ Better handling of edge cases
|
||||
|
||||
### **User Experience**:
|
||||
- ✅ Count updates correctly after marking
|
||||
- ✅ No more stuck count at 65
|
||||
- ✅ Faster response times
|
||||
|
||||
---
|
||||
|
||||
## 🚀 **Testing Checklist**
|
||||
|
||||
After rebuild (`rm -rf .next && npm run build && npm start`):
|
||||
|
||||
1. ✅ **Test Mark All As Read**:
|
||||
- Should work even if user ID lookup was previously failing
|
||||
- Count should update to 0 after marking
|
||||
- Cache should be invalidated
|
||||
|
||||
2. ✅ **Test Mark Single As Read**:
|
||||
- Should work reliably
|
||||
- Count should decrement correctly
|
||||
- Cache should be invalidated
|
||||
|
||||
3. ✅ **Test Count Accuracy**:
|
||||
- Should show accurate count (up to 1000)
|
||||
- Should refresh after marking
|
||||
- Should use fresh data from API
|
||||
|
||||
4. ✅ **Test User ID Caching**:
|
||||
- First call should fetch from API
|
||||
- Subsequent calls should use cache
|
||||
- Should be faster on subsequent calls
|
||||
|
||||
5. ✅ **Test Retry Logic**:
|
||||
- Should retry on transient failures
|
||||
- Should eventually succeed or fail gracefully
|
||||
- Should log retry attempts
|
||||
|
||||
---
|
||||
|
||||
## 📝 **Files Modified**
|
||||
|
||||
1. **`lib/services/notifications/leantime-adapter.ts`**:
|
||||
- Added Redis caching for user ID
|
||||
- Added retry logic with exponential backoff
|
||||
- Improved `getNotificationCount()` to fetch directly from API
|
||||
- Added `invalidateUserIdCache()` static method
|
||||
- Better error handling and logging
|
||||
|
||||
2. **`lib/services/notifications/notification-service.ts`**:
|
||||
- Always invalidate cache in `markAsRead()`
|
||||
- Always invalidate cache in `markAllAsRead()`
|
||||
- Better error handling and logging
|
||||
|
||||
---
|
||||
|
||||
## 🔧 **Configuration**
|
||||
|
||||
- **User ID Cache TTL**: 3600 seconds (1 hour)
|
||||
- **Max Retries**: 3 attempts
|
||||
- **Retry Backoff**: Exponential (1s, 2s, 4s, max 5s)
|
||||
- **Count Fetch Limit**: 1000 notifications
|
||||
|
||||
---
|
||||
|
||||
**Status**: ✅ All fixes implemented and ready for testing
|
||||
|
||||
526
NOTIFICATION_FLOW_ANALYSIS.md
Normal file
526
NOTIFICATION_FLOW_ANALYSIS.md
Normal file
@ -0,0 +1,526 @@
|
||||
# Complete Notification Flow Analysis
|
||||
|
||||
**Date**: 2026-01-06
|
||||
**Purpose**: Trace the entire notification system flow to identify issues and improvements
|
||||
|
||||
---
|
||||
|
||||
## 🔍 **FLOW 1: Initial Page Load & Count Display**
|
||||
|
||||
### Step-by-Step Flow:
|
||||
|
||||
1. **Component Mount** (`notification-badge.tsx`)
|
||||
- `useNotifications()` hook initializes
|
||||
- `useEffect` triggers when `status === 'authenticated'`
|
||||
- Calls `fetchNotificationCount(true)` (force refresh)
|
||||
- Calls `fetchNotifications()`
|
||||
- Starts polling every 60 seconds
|
||||
|
||||
2. **Count Fetch** (`use-notifications.ts` → `/api/notifications/count`)
|
||||
- Hook calls `/api/notifications/count?_t=${Date.now()}` (cache-busting)
|
||||
- API route authenticates user
|
||||
- Calls `NotificationService.getNotificationCount(userId)`
|
||||
|
||||
3. **Service Layer** (`notification-service.ts`)
|
||||
- **Checks Redis cache first** (`notifications:count:${userId}`)
|
||||
- If cached: Returns cached data immediately
|
||||
- If not cached: Fetches from adapters
|
||||
|
||||
4. **Adapter Layer** (`leantime-adapter.ts`)
|
||||
- `getNotificationCount()` calls `getNotifications(userId, 1, 100)`
|
||||
- **⚠️ ISSUE**: Only fetches first 100 notifications for counting
|
||||
- Filters unread: `notifications.filter(n => !n.isRead).length`
|
||||
- Returns count object
|
||||
|
||||
5. **Cache Storage**
|
||||
- Service stores count in Redis with 30-second TTL
|
||||
- Returns to API route
|
||||
- API returns to hook
|
||||
- Hook updates React state: `setNotificationCount(data)`
|
||||
|
||||
6. **UI Update**
|
||||
- Badge displays `notificationCount.unread`
|
||||
- Shows "65" if 65 unread notifications
|
||||
|
||||
---
|
||||
|
||||
## 🔍 **FLOW 2: Mark Single Notification as Read**
|
||||
|
||||
### Step-by-Step Flow:
|
||||
|
||||
1. **User Action** (`notification-badge.tsx`)
|
||||
- User clicks "Mark as read" button
|
||||
- Calls `handleMarkAsRead(notificationId)`
|
||||
- Calls `markAsRead(notificationId)` from hook
|
||||
|
||||
2. **Hook Action** (`use-notifications.ts`)
|
||||
- Makes POST to `/api/notifications/${notificationId}/read`
|
||||
- **Optimistic UI Update**:
|
||||
- Updates notification in state: `isRead: true`
|
||||
- Decrements count: `unread: Math.max(0, prev.unread - 1)`
|
||||
- Waits 100ms, then calls `fetchNotificationCount(true)`
|
||||
|
||||
3. **API Route** (`app/api/notifications/[id]/read/route.ts`)
|
||||
- Authenticates user
|
||||
- Extracts notification ID: `leantime-2732` → splits to get source and ID
|
||||
- Calls `NotificationService.markAsRead(userId, notificationId)`
|
||||
|
||||
4. **Service Layer** (`notification-service.ts`)
|
||||
- Extracts source: `leantime` from ID
|
||||
- Gets adapter: `this.adapters.get('leantime')`
|
||||
- Calls `adapter.markAsRead(userId, notificationId)`
|
||||
|
||||
5. **Adapter Layer** (`leantime-adapter.ts`)
|
||||
- **Gets user email from session**: `getUserEmail()`
|
||||
- **Gets Leantime user ID**: `getLeantimeUserId(email)`
|
||||
- **⚠️ CRITICAL ISSUE**: If `getLeantimeUserId()` fails → returns `false`
|
||||
- If successful: Calls Leantime API `markNotificationRead`
|
||||
- Returns success/failure
|
||||
|
||||
6. **Cache Invalidation** (`notification-service.ts`)
|
||||
- If `markAsRead()` returns `true`:
|
||||
- Calls `invalidateCache(userId)`
|
||||
- Deletes count cache: `notifications:count:${userId}`
|
||||
- Deletes all list caches: `notifications:list:${userId}:*`
|
||||
- If returns `false`: **Cache NOT invalidated** ❌
|
||||
|
||||
7. **Count Refresh** (`use-notifications.ts`)
|
||||
- After 100ms delay, calls `fetchNotificationCount(true)`
|
||||
- Fetches fresh count from API
|
||||
- **⚠️ ISSUE**: If cache wasn't invalidated, might get stale count
|
||||
|
||||
---
|
||||
|
||||
## 🔍 **FLOW 3: Mark All Notifications as Read**
|
||||
|
||||
### Step-by-Step Flow:
|
||||
|
||||
1. **User Action** (`notification-badge.tsx`)
|
||||
- User clicks "Mark all read" button
|
||||
- Calls `handleMarkAllAsRead()`
|
||||
- Calls `markAllAsRead()` from hook
|
||||
|
||||
2. **Hook Action** (`use-notifications.ts`)
|
||||
- Makes POST to `/api/notifications/read-all`
|
||||
- **Optimistic UI Update**:
|
||||
- Sets all notifications: `isRead: true`
|
||||
- Sets count: `unread: 0`
|
||||
- Waits 200ms, then calls `fetchNotificationCount(true)`
|
||||
|
||||
3. **API Route** (`app/api/notifications/read-all/route.ts`)
|
||||
- Authenticates user
|
||||
- Calls `NotificationService.markAllAsRead(userId)`
|
||||
|
||||
4. **Service Layer** (`notification-service.ts`)
|
||||
- Loops through all adapters
|
||||
- For each adapter:
|
||||
- Checks if configured
|
||||
- Calls `adapter.markAllAsRead(userId)`
|
||||
- Collects results: `[true/false, ...]`
|
||||
- Determines: `success = results.every(r => r)`, `anySuccess = results.some(r => r)`
|
||||
- **Cache Invalidation**:
|
||||
- If `anySuccess === true`: Invalidates cache ✅
|
||||
- If `anySuccess === false`: **Cache NOT invalidated** ❌
|
||||
|
||||
5. **Adapter Layer** (`leantime-adapter.ts`)
|
||||
- **Gets user email**: `getUserEmail()`
|
||||
- **Gets Leantime user ID**: `getLeantimeUserId(email)`
|
||||
- **⚠️ CRITICAL ISSUE**: If this fails → returns `false` immediately
|
||||
- If successful:
|
||||
- Fetches all notifications directly from API (up to 1000)
|
||||
- Filters unread: `rawNotifications.filter(n => n.read === 0)`
|
||||
- Marks each individually using `markNotificationRead`
|
||||
- Returns success if any were marked
|
||||
|
||||
6. **Cache Invalidation** (`notification-service.ts`)
|
||||
- Only happens if `anySuccess === true`
|
||||
- **⚠️ ISSUE**: If `getLeantimeUserId()` fails, `anySuccess = false`
|
||||
- Cache stays stale → count remains 65
|
||||
|
||||
7. **Count Refresh** (`use-notifications.ts`)
|
||||
- After 200ms, calls `fetchNotificationCount(true)`
|
||||
- **⚠️ ISSUE**: If cache wasn't invalidated, gets stale count from cache
|
||||
|
||||
---
|
||||
|
||||
## 🔍 **FLOW 4: Fetch Notification List**
|
||||
|
||||
### Step-by-Step Flow:
|
||||
|
||||
1. **User Opens Dropdown** (`notification-badge.tsx`)
|
||||
- `handleOpenChange(true)` called
|
||||
- Calls `manualFetch()` which calls `fetchNotifications(1, 10)`
|
||||
|
||||
2. **Hook Action** (`use-notifications.ts`)
|
||||
- Makes GET to `/api/notifications?page=1&limit=20`
|
||||
- Updates state: `setNotifications(data.notifications)`
|
||||
|
||||
3. **API Route** (`app/api/notifications/route.ts`)
|
||||
- Authenticates user
|
||||
- Calls `NotificationService.getNotifications(userId, page, limit)`
|
||||
|
||||
4. **Service Layer** (`notification-service.ts`)
|
||||
- **Checks Redis cache first**: `notifications:list:${userId}:${page}:${limit}`
|
||||
- If cached: Returns cached data immediately
|
||||
- If not cached: Fetches from adapters
|
||||
|
||||
5. **Adapter Layer** (`leantime-adapter.ts`)
|
||||
- Gets user email and Leantime user ID
|
||||
- Calls Leantime API `getAllNotifications` with pagination
|
||||
- Transforms notifications to our format
|
||||
- Returns array
|
||||
|
||||
6. **Cache Storage**
|
||||
- Service stores list in Redis with 5-minute TTL
|
||||
- Returns to API
|
||||
- API returns to hook
|
||||
- Hook updates React state
|
||||
|
||||
---
|
||||
|
||||
## 🐛 **IDENTIFIED ISSUES**
|
||||
|
||||
### **Issue #1: getLeantimeUserId() Fails Inconsistently**
|
||||
|
||||
**Problem**:
|
||||
- `getLeantimeUserId()` works in `getNotifications()` and `getNotificationCount()`
|
||||
- But fails in `markAllAsRead()` and sometimes in `markAsRead()`
|
||||
- Logs show: `"User not found in Leantime: a.tmiri@clm.foundation"`
|
||||
|
||||
**Root Cause**:
|
||||
- `getLeantimeUserId()` calls Leantime API `getAll` users endpoint
|
||||
- Fetches ALL users, then searches for matching email
|
||||
- **Possible causes**:
|
||||
1. **Race condition**: API call happens at different times
|
||||
2. **Session timing**: Session might be different between calls
|
||||
3. **API rate limiting**: Leantime API might throttle requests
|
||||
4. **Caching issue**: No caching of user ID lookup
|
||||
|
||||
**Impact**:
|
||||
- Mark all as read fails → cache not invalidated → count stays 65
|
||||
- Mark single as read might fail → cache not invalidated → count doesn't update
|
||||
|
||||
**Solution**:
|
||||
- Cache Leantime user ID in Redis with longer TTL
|
||||
- Add retry logic with exponential backoff
|
||||
- Add better error handling and logging
|
||||
|
||||
---
|
||||
|
||||
### **Issue #2: Cache Invalidation Only on Success**
|
||||
|
||||
**Problem**:
|
||||
- Cache is only invalidated if `markAsRead()` or `markAllAsRead()` returns `true`
|
||||
- If operation fails (e.g., `getLeantimeUserId()` fails), cache stays stale
|
||||
- Count remains at old value (65)
|
||||
|
||||
**Root Cause**:
|
||||
```typescript
|
||||
if (success) {
|
||||
await this.invalidateCache(userId);
|
||||
}
|
||||
```
|
||||
|
||||
**Impact**:
|
||||
- User sees stale count even after attempting to mark as read
|
||||
- UI shows optimistic update, but server count doesn't match
|
||||
|
||||
**Solution**:
|
||||
- Always invalidate cache after marking attempt (even on failure)
|
||||
- Or: Invalidate cache before marking, then refresh after
|
||||
- Or: Use optimistic updates with eventual consistency
|
||||
|
||||
---
|
||||
|
||||
### **Issue #3: Count Based on First 100 Notifications**
|
||||
|
||||
**Problem**:
|
||||
- `getNotificationCount()` only fetches first 100 notifications
|
||||
- If user has 200 notifications with 66 unread, count shows 66
|
||||
- But if 66 unread are beyond first 100, count is wrong
|
||||
|
||||
**Root Cause**:
|
||||
```typescript
|
||||
const notifications = await this.getNotifications(userId, 1, 100);
|
||||
const unreadCount = notifications.filter(n => !n.isRead).length;
|
||||
```
|
||||
|
||||
**Impact**:
|
||||
- Count might be inaccurate if >100 notifications exist
|
||||
- User might see "66 unread" but only 10 displayed (pagination)
|
||||
|
||||
**Solution**:
|
||||
- Use dedicated count API if Leantime provides one
|
||||
- Or: Fetch all notifications for counting (up to reasonable limit)
|
||||
- Or: Show "66+ unread" if count reaches 100
|
||||
|
||||
---
|
||||
|
||||
### **Issue #4: Race Condition Between Cache Invalidation and Count Fetch**
|
||||
|
||||
**Problem**:
|
||||
- Hook calls `fetchNotificationCount(true)` after 100-200ms delay
|
||||
- But cache invalidation might not be complete
|
||||
- Count fetch might still get stale cache
|
||||
|
||||
**Root Cause**:
|
||||
```typescript
|
||||
setTimeout(() => {
|
||||
fetchNotificationCount(true);
|
||||
}, 200);
|
||||
```
|
||||
|
||||
**Impact**:
|
||||
- Count might not update immediately after marking
|
||||
- User sees optimistic update, then stale count
|
||||
|
||||
**Solution**:
|
||||
- Increase delay to 500ms
|
||||
- Or: Poll count until it matches expected value
|
||||
- Or: Use WebSocket/SSE for real-time updates
|
||||
|
||||
---
|
||||
|
||||
### **Issue #5: No Caching of Leantime User ID**
|
||||
|
||||
**Problem**:
|
||||
- `getLeantimeUserId()` fetches ALL users from Leantime API every time
|
||||
- No caching, so repeated calls are slow and might fail
|
||||
- Different calls might get different results (race condition)
|
||||
|
||||
**Root Cause**:
|
||||
- No Redis cache for user ID mapping
|
||||
- Each call makes full API request
|
||||
|
||||
**Impact**:
|
||||
- Slow performance
|
||||
- Inconsistent results
|
||||
- API rate limiting issues
|
||||
|
||||
**Solution**:
|
||||
- Cache user ID in Redis: `leantime:userid:${email}` with 1-hour TTL
|
||||
- Invalidate cache only when user changes or on explicit refresh
|
||||
|
||||
---
|
||||
|
||||
### **Issue #6: getNotificationCount Uses Cached getNotifications**
|
||||
|
||||
**Problem**:
|
||||
- `getNotificationCount()` calls `getNotifications(userId, 1, 100)`
|
||||
- `getNotifications()` uses cache if available
|
||||
- Count might be based on stale cached notifications
|
||||
|
||||
**Root Cause**:
|
||||
```typescript
|
||||
async getNotificationCount(userId: string): Promise<NotificationCount> {
|
||||
const notifications = await this.getNotifications(userId, 1, 100);
|
||||
// Uses cached data if available
|
||||
}
|
||||
```
|
||||
|
||||
**Impact**:
|
||||
- Count might be stale even if notifications were marked as read
|
||||
- Cache TTL mismatch: count cache (30s) vs list cache (5min)
|
||||
|
||||
**Solution**:
|
||||
- Fetch notifications directly from API for counting (bypass cache)
|
||||
- Or: Use dedicated count endpoint
|
||||
- Or: Invalidate list cache when count cache is invalidated
|
||||
|
||||
---
|
||||
|
||||
### **Issue #7: Optimistic Updates Don't Match Server State**
|
||||
|
||||
**Problem**:
|
||||
- Hook optimistically updates count: `unread: 0`
|
||||
- But server count might still be 65 (cache not invalidated)
|
||||
- After refresh, count jumps back to 65
|
||||
|
||||
**Root Cause**:
|
||||
- Optimistic update happens immediately
|
||||
- Server cache invalidation might fail
|
||||
- Count refresh gets stale data
|
||||
|
||||
**Impact**:
|
||||
- Confusing UX: count goes to 0, then back to 65
|
||||
- User thinks operation failed when it might have succeeded
|
||||
|
||||
**Solution**:
|
||||
- Only show optimistic update if we're confident operation will succeed
|
||||
- Or: Show loading state until server confirms
|
||||
- Or: Poll until count matches expected value
|
||||
|
||||
---
|
||||
|
||||
## 🎯 **RECOMMENDED IMPROVEMENTS**
|
||||
|
||||
### **Priority 1: Fix getLeantimeUserId() Reliability**
|
||||
|
||||
1. **Cache User ID Mapping**
|
||||
```typescript
|
||||
// Cache key: leantime:userid:${email}
|
||||
// TTL: 1 hour
|
||||
// Invalidate on user update or explicit refresh
|
||||
```
|
||||
|
||||
2. **Add Retry Logic**
|
||||
```typescript
|
||||
// Retry 3 times with exponential backoff
|
||||
// Log each attempt
|
||||
// Return cached value if API fails
|
||||
```
|
||||
|
||||
3. **Better Error Handling**
|
||||
```typescript
|
||||
// Log full error details
|
||||
// Return null only after all retries fail
|
||||
// Don't fail entire operation on user ID lookup failure
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### **Priority 2: Always Invalidate Cache After Marking**
|
||||
|
||||
1. **Invalidate Before Marking**
|
||||
```typescript
|
||||
// Invalidate cache first
|
||||
// Then mark as read
|
||||
// Then refresh count
|
||||
```
|
||||
|
||||
2. **Or: Always Invalidate After Attempt**
|
||||
```typescript
|
||||
// Always invalidate cache after marking attempt
|
||||
// Even if operation failed
|
||||
// This ensures fresh data on next fetch
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### **Priority 3: Fix Count Accuracy**
|
||||
|
||||
1. **Use Dedicated Count API** (if available)
|
||||
```typescript
|
||||
// Check if Leantime has count-only endpoint
|
||||
// Use that instead of fetching all notifications
|
||||
```
|
||||
|
||||
2. **Or: Fetch All for Counting**
|
||||
```typescript
|
||||
// Fetch up to 1000 notifications for counting
|
||||
// Or use pagination to count all
|
||||
```
|
||||
|
||||
3. **Or: Show "66+ unread" if limit reached**
|
||||
```typescript
|
||||
// If count === 100, show "100+ unread"
|
||||
// Indicate there might be more
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### **Priority 4: Improve Cache Strategy**
|
||||
|
||||
1. **Unified Cache Invalidation**
|
||||
```typescript
|
||||
// When count cache is invalidated, also invalidate list cache
|
||||
// When list cache is invalidated, also invalidate count cache
|
||||
// Keep them in sync
|
||||
```
|
||||
|
||||
2. **Shorter Cache TTLs**
|
||||
```typescript
|
||||
// Count cache: 10 seconds (currently 30s)
|
||||
// List cache: 1 minute (currently 5min)
|
||||
// More frequent updates
|
||||
```
|
||||
|
||||
3. **Cache Tags/Versioning**
|
||||
```typescript
|
||||
// Use cache version numbers
|
||||
// Increment on invalidation
|
||||
// Check version before using cache
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### **Priority 5: Better Error Recovery**
|
||||
|
||||
1. **Graceful Degradation**
|
||||
```typescript
|
||||
// If mark as read fails, still invalidate cache
|
||||
// Show error message to user
|
||||
// Allow retry
|
||||
```
|
||||
|
||||
2. **Retry Logic**
|
||||
```typescript
|
||||
// Retry failed operations automatically
|
||||
// Exponential backoff
|
||||
// Max 3 retries
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 📊 **FLOW DIAGRAM: Current vs Improved**
|
||||
|
||||
### **Current Flow (Mark All As Read)**:
|
||||
```
|
||||
User clicks → Hook → API → Service → Adapter
|
||||
↓
|
||||
getLeantimeUserId() → FAILS ❌
|
||||
↓
|
||||
Returns false → Service: anySuccess = false
|
||||
↓
|
||||
Cache NOT invalidated ❌
|
||||
↓
|
||||
Count refresh → Gets stale cache → Shows 65 ❌
|
||||
```
|
||||
|
||||
### **Improved Flow (Mark All As Read)**:
|
||||
```
|
||||
User clicks → Hook → API → Service → Adapter
|
||||
↓
|
||||
getLeantimeUserId() → Check cache first
|
||||
↓
|
||||
If cached: Use cached ID ✅
|
||||
If not cached: Fetch from API → Cache result ✅
|
||||
↓
|
||||
Mark all as read → Success ✅
|
||||
↓
|
||||
Always invalidate cache (even on partial failure) ✅
|
||||
↓
|
||||
Count refresh → Gets fresh data → Shows 0 ✅
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 🚀 **IMPLEMENTATION PRIORITY**
|
||||
|
||||
1. **Fix getLeantimeUserId() caching** (High Priority)
|
||||
- Add Redis cache for user ID mapping
|
||||
- Add retry logic
|
||||
- Better error handling
|
||||
|
||||
2. **Always invalidate cache** (High Priority)
|
||||
- Invalidate cache even on failure
|
||||
- Or invalidate before marking
|
||||
|
||||
3. **Fix count accuracy** (Medium Priority)
|
||||
- Use dedicated count API or fetch all
|
||||
- Show "66+ unread" if limit reached
|
||||
|
||||
4. **Improve cache strategy** (Medium Priority)
|
||||
- Unified invalidation
|
||||
- Shorter TTLs
|
||||
- Cache versioning
|
||||
|
||||
5. **Better error recovery** (Low Priority)
|
||||
- Graceful degradation
|
||||
- Retry logic
|
||||
- Better UX
|
||||
|
||||
---
|
||||
|
||||
**Status**: Analysis complete. Ready for implementation.
|
||||
|
||||
@ -2,6 +2,7 @@ import { Notification, NotificationCount } from '@/lib/types/notification';
|
||||
import { NotificationAdapter } from './notification-adapter.interface';
|
||||
import { getServerSession } from 'next-auth';
|
||||
import { authOptions } from "@/app/api/auth/options";
|
||||
import { getRedisClient } from '@/lib/redis';
|
||||
|
||||
// Leantime notification type from their API
|
||||
interface LeantimeNotification {
|
||||
@ -20,6 +21,8 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
readonly sourceName = 'leantime';
|
||||
private apiUrl: string;
|
||||
private apiToken: string;
|
||||
private static readonly USER_ID_CACHE_TTL = 3600; // 1 hour
|
||||
private static readonly USER_ID_CACHE_KEY_PREFIX = 'leantime:userid:';
|
||||
|
||||
constructor() {
|
||||
this.apiUrl = process.env.LEANTIME_API_URL || '';
|
||||
@ -28,6 +31,21 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
console.log('[LEANTIME_ADAPTER] Initialized with API URL and token');
|
||||
}
|
||||
|
||||
/**
|
||||
* Invalidate cached Leantime user ID for an email
|
||||
* Useful when user data changes or for debugging
|
||||
*/
|
||||
static async invalidateUserIdCache(email: string): Promise<void> {
|
||||
try {
|
||||
const redis = getRedisClient();
|
||||
const cacheKey = `${LeantimeAdapter.USER_ID_CACHE_KEY_PREFIX}${email.toLowerCase()}`;
|
||||
await redis.del(cacheKey);
|
||||
console.log(`[LEANTIME_ADAPTER] Invalidated user ID cache for ${email}`);
|
||||
} catch (error) {
|
||||
console.error(`[LEANTIME_ADAPTER] Error invalidating user ID cache:`, error);
|
||||
}
|
||||
}
|
||||
|
||||
async getNotifications(userId: string, page = 1, limit = 20): Promise<Notification[]> {
|
||||
console.log(`[LEANTIME_ADAPTER] getNotifications called for userId: ${userId}, page: ${page}, limit: ${limit}`);
|
||||
|
||||
@ -122,20 +140,107 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
console.log(`[LEANTIME_ADAPTER] getNotificationCount called for userId: ${userId}`);
|
||||
|
||||
try {
|
||||
// Get all notifications and count them
|
||||
// NOTE: This only gets first 100 notifications. If there are more, the count will be inaccurate.
|
||||
// TODO: Consider using a dedicated count API endpoint if Leantime provides one
|
||||
const notifications = await this.getNotifications(userId, 1, 100); // Get up to 100 for counting
|
||||
// Fetch notifications directly from API for accurate counting (bypassing cache)
|
||||
// Fetch up to 1000 notifications to get accurate count
|
||||
const email = await this.getUserEmail();
|
||||
if (!email) {
|
||||
console.error('[LEANTIME_ADAPTER] Could not get user email for count');
|
||||
return {
|
||||
total: 0,
|
||||
unread: 0,
|
||||
sources: {
|
||||
leantime: {
|
||||
total: 0,
|
||||
unread: 0
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// Count total and unread
|
||||
const totalCount = notifications.length;
|
||||
const unreadCount = notifications.filter(n => !n.isRead).length;
|
||||
const leantimeUserId = await this.getLeantimeUserId(email);
|
||||
if (!leantimeUserId) {
|
||||
console.error('[LEANTIME_ADAPTER] Could not get Leantime user ID for count');
|
||||
return {
|
||||
total: 0,
|
||||
unread: 0,
|
||||
sources: {
|
||||
leantime: {
|
||||
total: 0,
|
||||
unread: 0
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
// Fetch directly from API (bypassing cache) to get accurate count
|
||||
const jsonRpcBody = {
|
||||
jsonrpc: '2.0',
|
||||
method: 'leantime.rpc.Notifications.Notifications.getAllNotifications',
|
||||
params: {
|
||||
userId: leantimeUserId,
|
||||
showNewOnly: 0, // Get all notifications
|
||||
limitStart: 0,
|
||||
limitEnd: 1000, // Fetch up to 1000 for accurate counting
|
||||
filterOptions: []
|
||||
},
|
||||
id: 1
|
||||
};
|
||||
|
||||
const response = await fetch(`${this.apiUrl}/api/jsonrpc`, {
|
||||
method: 'POST',
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
'X-API-Key': this.apiToken
|
||||
},
|
||||
body: JSON.stringify(jsonRpcBody)
|
||||
});
|
||||
|
||||
if (!response.ok) {
|
||||
console.error(`[LEANTIME_ADAPTER] Failed to fetch notifications for count: HTTP ${response.status}`);
|
||||
return {
|
||||
total: 0,
|
||||
unread: 0,
|
||||
sources: {
|
||||
leantime: {
|
||||
total: 0,
|
||||
unread: 0
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
const data = await response.json();
|
||||
|
||||
if (data.error || !Array.isArray(data.result)) {
|
||||
console.error('[LEANTIME_ADAPTER] Error or invalid response for count:', data.error);
|
||||
return {
|
||||
total: 0,
|
||||
unread: 0,
|
||||
sources: {
|
||||
leantime: {
|
||||
total: 0,
|
||||
unread: 0
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
const rawNotifications = data.result;
|
||||
|
||||
// Count total and unread from raw data
|
||||
const totalCount = rawNotifications.length;
|
||||
const unreadCount = rawNotifications.filter((n: any) =>
|
||||
n.read === 0 || n.read === false || n.read === '0'
|
||||
).length;
|
||||
|
||||
const hasMore = totalCount === 1000; // If we got exactly 1000, there might be more
|
||||
|
||||
console.log('[LEANTIME_ADAPTER] Notification counts:', {
|
||||
total: totalCount,
|
||||
unread: unreadCount,
|
||||
read: totalCount - unreadCount,
|
||||
note: totalCount === 100 ? 'WARNING: May have more than 100 notifications total' : 'OK'
|
||||
hasMore: hasMore,
|
||||
note: hasMore ? 'WARNING: May have more than 1000 notifications total' : 'OK'
|
||||
});
|
||||
|
||||
return {
|
||||
@ -447,13 +552,37 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
}
|
||||
}
|
||||
|
||||
// Helper function to get Leantime user ID by email
|
||||
private async getLeantimeUserId(email: string): Promise<number | null> {
|
||||
// Helper function to get Leantime user ID by email with caching and retry logic
|
||||
private async getLeantimeUserId(email: string, retryCount = 0): Promise<number | null> {
|
||||
const MAX_RETRIES = 3;
|
||||
const CACHE_KEY = `${LeantimeAdapter.USER_ID_CACHE_KEY_PREFIX}${email.toLowerCase()}`;
|
||||
|
||||
try {
|
||||
if (!this.apiToken) {
|
||||
console.error('[LEANTIME_ADAPTER] No API token available for getLeantimeUserId');
|
||||
return null;
|
||||
}
|
||||
|
||||
// Check Redis cache first
|
||||
try {
|
||||
const redis = getRedisClient();
|
||||
const cachedUserId = await redis.get(CACHE_KEY);
|
||||
if (cachedUserId) {
|
||||
const userId = parseInt(cachedUserId, 10);
|
||||
if (!isNaN(userId)) {
|
||||
console.log(`[LEANTIME_ADAPTER] Found cached Leantime user ID for ${email}: ${userId}`);
|
||||
return userId;
|
||||
}
|
||||
}
|
||||
} catch (cacheError) {
|
||||
console.warn('[LEANTIME_ADAPTER] Error checking cache for user ID, will fetch from API:', cacheError);
|
||||
}
|
||||
|
||||
// Fetch from API with retry logic
|
||||
const fetchWithRetry = async (attempt: number): Promise<number | null> => {
|
||||
try {
|
||||
console.log(`[LEANTIME_ADAPTER] Fetching Leantime user ID for ${email} (attempt ${attempt + 1}/${MAX_RETRIES + 1})`);
|
||||
|
||||
const response = await fetch(`${this.apiUrl}/api/jsonrpc`, {
|
||||
method: 'POST',
|
||||
headers: {
|
||||
@ -468,12 +597,35 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
});
|
||||
|
||||
if (!response.ok) {
|
||||
const errorText = await response.text();
|
||||
console.error(`[LEANTIME_ADAPTER] API error (HTTP ${response.status}):`, errorText.substring(0, 200));
|
||||
|
||||
// Retry on server errors (5xx) or rate limiting (429)
|
||||
if ((response.status >= 500 || response.status === 429) && attempt < MAX_RETRIES) {
|
||||
const delay = Math.min(1000 * Math.pow(2, attempt), 5000); // Exponential backoff, max 5s
|
||||
console.log(`[LEANTIME_ADAPTER] Retrying in ${delay}ms...`);
|
||||
await new Promise(resolve => setTimeout(resolve, delay));
|
||||
return fetchWithRetry(attempt + 1);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
const data = await response.json();
|
||||
|
||||
if (data.error) {
|
||||
console.error('[LEANTIME_ADAPTER] JSON-RPC error:', data.error);
|
||||
// Retry on certain errors
|
||||
if (attempt < MAX_RETRIES && (data.error.code === -32603 || data.error.code === -32000)) {
|
||||
const delay = Math.min(1000 * Math.pow(2, attempt), 5000);
|
||||
console.log(`[LEANTIME_ADAPTER] Retrying after error in ${delay}ms...`);
|
||||
await new Promise(resolve => setTimeout(resolve, delay));
|
||||
return fetchWithRetry(attempt + 1);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
if (!data.result || !Array.isArray(data.result)) {
|
||||
console.error('[LEANTIME_ADAPTER] Invalid response format:', data);
|
||||
return null;
|
||||
}
|
||||
|
||||
@ -486,13 +638,44 @@ export class LeantimeAdapter implements NotificationAdapter {
|
||||
(typeof u.username === 'string' && u.username.toLowerCase() === email.toLowerCase())
|
||||
);
|
||||
|
||||
if (user) {
|
||||
return user.id;
|
||||
if (user && user.id) {
|
||||
const userId = typeof user.id === 'number' ? user.id : parseInt(String(user.id), 10);
|
||||
|
||||
if (!isNaN(userId)) {
|
||||
// Cache the result
|
||||
try {
|
||||
const redis = getRedisClient();
|
||||
await redis.set(CACHE_KEY, userId.toString(), 'EX', LeantimeAdapter.USER_ID_CACHE_TTL);
|
||||
console.log(`[LEANTIME_ADAPTER] Cached Leantime user ID for ${email}: ${userId} (TTL: ${LeantimeAdapter.USER_ID_CACHE_TTL}s)`);
|
||||
} catch (cacheError) {
|
||||
console.warn('[LEANTIME_ADAPTER] Failed to cache user ID (non-fatal):', cacheError);
|
||||
// Continue even if caching fails
|
||||
}
|
||||
|
||||
return userId;
|
||||
}
|
||||
}
|
||||
|
||||
console.warn(`[LEANTIME_ADAPTER] User not found in Leantime for email: ${email}`);
|
||||
return null;
|
||||
} catch (error) {
|
||||
console.error(`[LEANTIME_ADAPTER] Error fetching user ID (attempt ${attempt + 1}):`, error);
|
||||
|
||||
// Retry on network errors
|
||||
if (attempt < MAX_RETRIES && error instanceof Error) {
|
||||
const delay = Math.min(1000 * Math.pow(2, attempt), 5000);
|
||||
console.log(`[LEANTIME_ADAPTER] Retrying after network error in ${delay}ms...`);
|
||||
await new Promise(resolve => setTimeout(resolve, delay));
|
||||
return fetchWithRetry(attempt + 1);
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
return await fetchWithRetry(retryCount);
|
||||
} catch (error) {
|
||||
console.error('[LEANTIME_ADAPTER] Error getting Leantime user ID:', error);
|
||||
console.error('[LEANTIME_ADAPTER] Fatal error getting Leantime user ID:', error);
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
@ -237,17 +237,28 @@ export class NotificationService {
|
||||
const sourceId = idParts.join('-'); // Reconstruct the ID in case it has hyphens
|
||||
|
||||
if (!source || !this.adapters.has(source)) {
|
||||
console.log(`[NOTIFICATION_SERVICE] markAsRead - Invalid source or adapter not found: ${source}`);
|
||||
// Still invalidate cache to ensure fresh data
|
||||
await this.invalidateCache(userId);
|
||||
return false;
|
||||
}
|
||||
|
||||
const adapter = this.adapters.get(source)!;
|
||||
const success = await adapter.markAsRead(userId, notificationId);
|
||||
let success = false;
|
||||
|
||||
if (success) {
|
||||
// Invalidate caches
|
||||
await this.invalidateCache(userId);
|
||||
try {
|
||||
success = await adapter.markAsRead(userId, notificationId);
|
||||
console.log(`[NOTIFICATION_SERVICE] markAsRead - Result: ${success} for notification ${notificationId}`);
|
||||
} catch (error) {
|
||||
console.error(`[NOTIFICATION_SERVICE] markAsRead - Error:`, error);
|
||||
success = false;
|
||||
}
|
||||
|
||||
// Always invalidate cache after marking attempt (even on failure)
|
||||
// This ensures fresh data on next fetch, even if the operation partially failed
|
||||
console.log(`[NOTIFICATION_SERVICE] markAsRead - Invalidating cache for user ${userId} (success: ${success})`);
|
||||
await this.invalidateCache(userId);
|
||||
|
||||
return success;
|
||||
}
|
||||
|
||||
@ -290,15 +301,11 @@ export class NotificationService {
|
||||
const anySuccess = results.some(result => result);
|
||||
console.log(`[NOTIFICATION_SERVICE] markAllAsRead overall success: ${success}, any success: ${anySuccess}`);
|
||||
|
||||
// Always invalidate cache if any notifications were marked (even partial success)
|
||||
// This ensures count and list are refreshed after marking
|
||||
if (anySuccess) {
|
||||
console.log(`[NOTIFICATION_SERVICE] Invalidating caches for user ${userId} (some notifications were marked)`);
|
||||
// Invalidate caches to force fresh data on next fetch
|
||||
// Always invalidate cache after marking attempt (even on failure)
|
||||
// This ensures fresh data on next fetch, even if the operation failed
|
||||
// The user might have marked some notifications manually, or the operation might have partially succeeded
|
||||
console.log(`[NOTIFICATION_SERVICE] Invalidating caches for user ${userId} (anySuccess: ${anySuccess}, overallSuccess: ${success})`);
|
||||
await this.invalidateCache(userId);
|
||||
} else {
|
||||
console.log(`[NOTIFICATION_SERVICE] Not invalidating caches - no notifications were marked`);
|
||||
}
|
||||
|
||||
return success;
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user