Fix Feed Filter and Wallet Screen Performance Issues

[akash2017sky]

PR Summary:

Bug 1: Feed Page 7/30/60 Days Filter Not Working

Problem: The time filter buttons (7 days, 30 days, 60 days) on the Feed page were not filtering transactions - all data was displayed regardless of which filter was selected.

Root Cause: The transaction.time field returns an ISO date string format ('2025-12-08T09:02:07.478617+00:00') from the API, but the filter logic was expecting a Unix timestamp number.

Fix: Added type checking and parsing logic in FeedList.tsx to handle both number (Unix timestamp) and string (ISO date) formats:
if (typeof txTimeRaw === 'number') { txTimeSeconds = txTimeRaw; } else if (typeof txTimeRaw === 'string') { txTimeSeconds = Math.floor(new Date(txTimeRaw).getTime() / 1000); }

Bug 2: Wallet Screen Freezing/Performance Issues

Problem: The Wallet screen was freezing the browser and not loading properly.

Root Causes:

1. users state was included in the useEffect dependency array while also being set inside the same effect, causing unnecessary re-triggers
2. Missing guard for empty MSAL accounts array causing premature fetch attempts
3. Redundant API calls to getUserWallets and getAllUsersFromAPI for each user

Fixes:

1. Removed users from the dependency array in WalletTransactionLog.tsx
2. Added guard: accounts && accounts.length > 0 before fetching
3. Added caching layer in lnbitsServiceLocal.ts:

• 60-second cache for getUserWallets and getAllUsersFromAPI responses
• Request deduplication to prevent concurrent duplicate API calls

Closes #137
Closes #135
Closes #40

[claude]

Pull Request Review - Fix Feed Filter and Wallet Screen Performance Issues

Summary

This PR addresses two critical bugs: feed page time filtering and wallet screen performance issues. The fixes are generally sound and address the root causes effectively. However, there are several areas that need attention before merging.

---

🐛 Bug Fixes

✅ Bug 1: Feed Filter (FeedList.tsx:268-287)

Good:

• Correctly identifies the issue: API returns ISO date strings, but code expected Unix timestamps
• Implements type checking to handle both number and string formats
• Adds clear comments explaining the logic

Issues:

1. Potential timezone issues: new Date(txTimeRaw).getTime() assumes the ISO string is properly formatted. Consider adding validation:

   const date = new Date(txTimeRaw);
   if (isNaN(date.getTime())) {
     console.warn(`Invalid date format for transaction: ${txTimeRaw}`);
     return false; // or handle appropriately
   }
   txTimeSeconds = Math.floor(date.getTime() / 1000);

2. Inconsistent fallback: When txTimeRaw is neither number nor string, txTimeSeconds = 0 is set, which means transactions with invalid timestamps will always pass the filter (since 0 < timestamp). Consider filtering them out instead:

   } else {
     console.warn(`Unknown time format: ${typeof txTimeRaw}`);
     return false; // Exclude invalid transactions
   }

⚠️ Bug 2: Wallet Performance (WalletTransactionLog.tsx:219)

Good:

• Removed users from dependency array (line 224) - correct fix for infinite loop
• Added guard for empty accounts array (line 219)

Critical Issue:
The guard condition has a logic error. The current code:

if (currentWallet !== activeWallet && accounts && accounts.length > 0)

This means if accounts is empty, the fetch will NEVER happen, even after login when accounts become available. This could leave the component in a broken state.

Recommended fix:

// Early return if no accounts
if (!accounts || accounts.length === 0) {
  setLoading(false);
  return;
}

// Only fetch if wallet changed
if (currentWallet !== activeWallet) {
  setAllTransactions([]);
  setDisplayedTransactions([]);
  fetchTransactions();
}

---

🔒 Security Concerns

1. WalletAllowanceComponent.tsx:78: Error message exposes internal implementation details:

   console.error('Allowance wallet inkey not found');

   Consider using a more generic error message or removing sensitive terminology from client-side logs.

2. Admin key exposure: All three components use process.env.REACT_APP_LNBITS_ADMINKEY directly. While this is environment-based, ensure this key is properly secured and not exposed in production builds.

---

🎯 Code Quality

Good Practices:

• ✅ Clear comments explaining the bug fixes
• ✅ Type checking for backward compatibility
• ✅ Defensive programming with null checks

Issues:

1. WalletTransactionLog.tsx:259-265: Duplicate loading/error checks

   if (loading) {
     return <div>Loading...</div>;
   }
   if (error) {
     return <div>{error}</div>;
   }
   // ... same checks above at lines 251-257

   Impact: Dead code that will never execute
   Fix: Remove the duplicate (lines 259-265)

2. FeedList.tsx:352-368: Inconsistent date parsing logic
   The component has date parsing in two places (lines 274-283 for filtering, and lines 352-368 for display). Consider extracting this into a reusable function:

   const parseTransactionTime = (timestamp: number | string): Date => {
     if (typeof timestamp === 'number') {
       return new Date(timestamp * 1000);
     }
     const date = new Date(timestamp);
     if (isNaN(date.getTime())) {
       throw new Error(`Invalid timestamp: ${timestamp}`);
     }
     return date;
   };

3. WalletAllowanceComponent.tsx:65: Magic number and incorrect variable name

   const sevenDaysAgo = Date.now() / 1000 - 30 * 24 * 60 * 60;

   Variable named sevenDaysAgo but calculates 30 days. Either rename or extract as constant:

   const DAYS_TO_FETCH = 30;
   const thirtyDaysAgo = Date.now() / 1000 - DAYS_TO_FETCH * 24 * 60 * 60;

---

⚡ Performance Considerations

1. Caching mentioned in PR description: The PR description mentions adding a 60-second cache layer in lnbitsServiceLocal.ts, but these changes are not included in the PR. Either:

   • Include the caching changes in this PR, or
   • Remove the caching mention from the description and create a separate PR

2. WalletTransactionLog.tsx:72-85: Fetches wallets for ALL users in a loop

   for (const u of allUsers) {
     const wallets = await getUserWallets(adminKey, u.id);

   This is still a performance bottleneck. Consider:

   • Batch API calls if the API supports it
   • Implement the caching layer mentioned in the PR description
   • Use Promise.all() for parallel requests:

   const walletPromises = allUsers.map(u => getUserWallets(adminKey, u.id));
   const allWallets = await Promise.all(walletPromises);

---

🧪 Test Coverage

Missing: No tests included for the bug fixes. Consider adding:

1. FeedList.test.tsx:

   describe('FeedList timestamp filtering', () => {
     it('should filter transactions with Unix timestamp', () => { /* ... */ });
     it('should filter transactions with ISO date string', () => { /* ... */ });
     it('should handle invalid timestamps gracefully', () => { /* ... */ });
   });

2. WalletTransactionLog.test.tsx:

   describe('WalletTransactionLog', () => {
     it('should not fetch when accounts array is empty', () => { /* ... */ });
     it('should fetch when wallet changes', () => { /* ... */ });
     it('should not create infinite loops', () => { /* ... */ });
   });

---

📝 Recommendations

Must Fix (Before Merge):

1. ❗ Fix the accounts guard logic in WalletTransactionLog.tsx:219
2. ❗ Remove duplicate loading/error checks in WalletTransactionLog.tsx:259-265
3. ❗ Fix variable naming: sevenDaysAgo → thirtyDaysAgo or use constant

Should Fix:

4. ⚠️ Add invalid date validation in FeedList timestamp filtering
5. ⚠️ Extract date parsing logic into reusable function
6. ⚠️ Clarify PR description regarding caching implementation

Nice to Have:

7. 💡 Add unit tests for the bug fixes
8. 💡 Implement parallel API calls in WalletTransactionLog
9. 💡 Use more generic error messages for security

---

✅ Conclusion

The PR successfully addresses the reported bugs with good understanding of root causes. However, the WalletTransactionLog accounts guard logic is incorrect and could prevent the component from loading after authentication. Please address the "Must Fix" items before merging.

The duplicate code and naming inconsistencies should also be cleaned up to maintain code quality standards.

[claude]

Pull Request Review: Fix Feed Filter and Wallet Screen Performance Issues

Overview

This PR addresses two critical bugs: non-functional time filters on the Feed page and performance issues causing the Wallet screen to freeze. The fixes are well-targeted and include proper caching mechanisms.

---

✅ Strengths

1. Root Cause Analysis

• Excellent problem identification with clear documentation in the PR description
• The type mismatch issue (Unix timestamp vs ISO string) was correctly diagnosed
• Performance bottlenecks were properly identified (infinite loops, redundant API calls)

2. Caching Implementation (lnbitsServiceLocal.ts)

• Smart addition of 60-second cache with request deduplication
• Prevents race conditions with pending request tracking
• Cache invalidation logic is sound with timestamp-based expiry

3. Type Safety (FeedList.tsx)

• Good handling of both number and string formats for transaction time
• Proper validation with isNaN() check before using parsed dates
• Informative console warnings for debugging

---

🔍 Issues & Concerns

Critical Issues

1. Incorrect Variable Name in Comment (WalletAllowanceComponent.tsx:65)

Location: tabs/src/components/WalletAllowanceComponent.tsx:65

const sevenDaysAgo = Date.now() / 1000 - 30 * 24 * 60 * 60;

Issue: The variable is named sevenDaysAgo but calculates 30 days ago (not 7). This is misleading.

Recommendation:

const thirtyDaysAgo = Date.now() / 1000 - 30 * 24 * 60 * 60;

---

2. Module-Level State in Service File (lnbitsServiceLocal.ts)

Location: tabs/src/services/lnbitsServiceLocal.ts:18-39

Issue: The cache and pending request maps are stored at module level:

const apiCache: {
  allUsers?: CacheEntry<any[]>;
  userWallets: Map<string, CacheEntry<Wallet[]>>;
} = {
  userWallets: new Map(),
};
let pendingUsersRequest: Promise<any[]> | null = null;
const pendingWalletRequests: Map<string, Promise<Wallet[] | null>> = new Map();

Concerns:

• No cache invalidation mechanism (only time-based expiry)
• No way to clear cache when user logs out or switches accounts
• Memory could grow unbounded if many users are accessed
• No max cache size limits

Recommendations:

1. Add a cache clearing function:

export const clearCache = () => {
  apiCache.allUsers = undefined;
  apiCache.userWallets.clear();
  pendingUsersRequest = null;
  pendingWalletRequests.clear();
};

2. Consider implementing LRU cache with max size limit
3. Call clearCache() on logout/account switch

---

3. Error Handling in Cache Layer

Location: tabs/src/services/lnbitsServiceLocal.ts:237-310

Issue: When API calls fail, the error is thrown but:

• Failed requests are removed from pendingWalletRequests (good)
• BUT no negative caching - if an API call fails, retrying immediately will make the same failing request again
• Multiple components calling simultaneously after a failure will trigger multiple identical failing requests

Recommendation: Consider adding temporary negative cache for failed requests (e.g., 5-second backoff).

---

Medium Priority Issues

4. Type Safety: any[] for Users

Location: tabs/src/services/lnbitsServiceLocal.ts:1079

const getAllUsersFromAPI = async (): Promise<any[]> => {

Issue: Using any[] loses type safety. Should use UserAccount[] from global types.

Recommendation:

const getAllUsersFromAPI = async (): Promise<UserAccount[]> => {

---

5. Silent Failure in WalletAllowanceComponent

Location: tabs/src/components/WalletAllowanceComponent.tsx:78-79

} else {
  console.error('Wallet configuration not found');
  setSpentSats(0);
}

Issue: Logs error but doesn't inform the user. The UI will show 0 spent sats without indicating an error occurred.

Recommendation: Consider setting an error state and displaying it to the user, or at least setting spentSats to null to distinguish between "no spending" and "error loading".

---

6. Transaction Type Union Issue

Location: tabs/src/types/global.d.ts:56

The type definition correctly shows:

time: number | string; // Unix timestamp (number) or ISO date string

Observation: This dual format suggests inconsistency in the API. While the fix handles it correctly, the root cause should ideally be addressed on the backend to return consistent formats.

Recommendation: File a follow-up issue to normalize the API response format.

---

7. Magic Number for Cache Duration

Location: tabs/src/services/lnbitsServiceLocal.ts:19

const CACHE_DURATION_MS = 60000; // 1 minute cache

Issue: 60 seconds might be too long for wallet balance data (could show stale balance).

Recommendation: Consider different TTLs for different data types:

• User list: 5 minutes (changes rarely)
• Wallet data: 10-30 seconds (changes frequently)

---

Minor Issues

8. Console Logs in Production Code

Locations: Multiple files

Issue: Multiple console.log, console.warn, and console.error statements added:

• tabs/src/components/FeedList.tsx:282, 287
• tabs/src/services/lnbitsServiceLocal.ts:244, 249, 1081, 1090-1092
• tabs/src/components/WalletAllowanceComponent.tsx:78

Recommendation:

• Use a proper logging utility (like the existing logger imported at line 6 of lnbitsServiceLocal.ts)
• Ensure logs can be disabled in production
• Remove debug logs or wrap in if (process.env.NODE_ENV === 'development')

---

9. Inconsistent Guard Check Pattern

Location: tabs/src/components/WalletTransactionLog.tsx:217-220

if (!accounts || accounts.length === 0) {
  setLoading(false);
  return;
}

Observation: This guard is good, but it sets loading to false before returning. If accounts becomes available later, the component won't automatically retry. Consider whether this is the intended behavior.

---

🧪 Test Coverage

Major Concern: No tests added for the new functionality.

Missing test coverage:

1. ✗ FeedList timestamp filtering logic (both number and string formats)
2. ✗ Invalid date handling in FeedList
3. ✗ Cache hit/miss scenarios in lnbitsServiceLocal.ts
4. ✗ Request deduplication logic
5. ✗ Cache expiration behavior
6. ✗ WalletTransactionLog early return guard

Recommendation: Add unit tests for:

// Example test cases needed:
describe('FeedList timestamp filtering', () => {
  it('should filter by Unix timestamp (number)', () => {});
  it('should filter by ISO date string', () => {});
  it('should exclude invalid date strings', () => {});
  it('should handle null/undefined timestamps', () => {});
});

describe('getUserWallets caching', () => {
  it('should return cached data within TTL', () => {});
  it('should refetch after cache expiry', () => {});
  it('should deduplicate concurrent requests', () => {});
});

---

🔒 Security Considerations

✅ No Critical Security Issues Found

Positive observations:

• No XSS vulnerabilities introduced
• No SQL injection risks (using API client)
• Token handling unchanged (existing sessionStorage approach maintained)
• Input validation added for date parsing

Minor Note: The caching layer stores wallet data in memory, which is acceptable for this use case but be aware it persists for the session lifetime.

---

⚡ Performance Improvements

Excellent Performance Fixes

1. Cache Implementation: 60-second cache will significantly reduce API load
2. Request Deduplication: Prevents redundant concurrent API calls
3. useEffect Dependency Fix: Removes infinite loop trigger in WalletTransactionLog
4. Early Return Guard: Prevents unnecessary work when accounts aren't loaded

Estimated Impact:

• Wallet screen load time: ~70-90% improvement (based on eliminating redundant requests)
• API server load: ~60-80% reduction (with caching)
• Feed filtering: Now functional (was broken, so 100% improvement)

---

📋 Recommendations Summary

Must Fix Before Merge:

1. ✅ Rename sevenDaysAgo → thirtyDaysAgo in WalletAllowanceComponent.tsx:65
2. ⚠️ Add cache clearing function for logout/account switching
3. ⚠️ Add unit tests for critical paths (filtering, caching)

Should Fix:

4. Change any[] → UserAccount[] in getAllUsersFromAPI
5. Use proper logging utility instead of console methods
6. Consider different cache TTLs for different data types

Consider for Follow-up:

7. Implement LRU cache with size limits
8. Add negative caching for failed requests
9. File backend issue to normalize API date format
10. Add user-facing error messages for failed wallet loads

---

✨ Final Assessment

Overall: Good fix with solid approach ✅

Strengths:

• Correctly identifies and fixes root causes
• Smart caching implementation
• Good type handling for dual-format timestamps

Weaknesses:

• Lacks test coverage
• Missing cache management utilities
• Some minor code quality issues

Recommendation: Approve with minor changes

• Fix the sevenDaysAgo variable name
• Add cache clearing mechanism
• Add tests in follow-up PR if not immediately

Great work debugging these tricky issues! The caching implementation is well-thought-out and will significantly improve performance. 🚀

---

Review generated with attention to code quality, security, performance, and maintainability.

[claude]

Pull Request Review - PR #138

Summary

This PR addresses two important bugs: Feed page time filter not working and wallet screen performance issues. The changes include bug fixes for timestamp handling and performance optimizations through caching and request deduplication.

---

✅ Strengths

1. Good Problem Analysis

• Clear identification of root causes (ISO string vs Unix timestamp mismatch, useEffect dependency issues)
• Well-documented PR description with detailed explanations

2. Performance Improvements

• Caching layer: 5-minute cache for users, 30-second cache for wallets
• Request deduplication: Prevents concurrent duplicate API calls
• Cache size limits: MAX_WALLET_CACHE_SIZE prevents unbounded memory growth

3. Robust Timestamp Handling

• parseTransactionTime() helper handles both Unix timestamps and ISO strings
• Good error handling with console warnings for invalid timestamps

---

🔍 Code Quality Issues

Critical Issues

1. Memory Leak Risk in lnbitsServiceLocal.ts ⚠️

Location: lnbitsServiceLocal.ts:37-42, 55-60

The cache and pending request maps are module-level variables that persist for the application lifetime:

const apiCache: {
  allUsers?: CacheEntry<User[]>;
  userWallets: Map<string, CacheEntry<Wallet[]>>;
} = {
  userWallets: new Map(),
};

Issues:

• No automatic cleanup mechanism
• clearApiCache() is exported but never called in the codebase
• Cache can grow stale with outdated user/wallet data
• In a multi-tenant scenario, switching between accounts won't clear old data

Recommendation:

• Call clearApiCache() on logout/account switch events
• Consider implementing TTL-based automatic cleanup
• Add cache invalidation on relevant mutations (wallet creation, user updates)

2. Inconsistent Cache Duration ⚠️

Location: lnbitsServiceLocal.ts:20-21

const CACHE_DURATION_USERS_MS = 300000; // 5 minutes for user list (changes rarely)
const CACHE_DURATION_WALLETS_MS = 30000; // 30 seconds for wallet data (changes frequently)

Issues:

• User data can become stale for 5 minutes (new users won't appear)
• Wallet balance updates happen frequently but only refresh every 30 seconds
• No consideration for real-time balance updates after transactions

Recommendation:

• Reduce user cache to 1-2 minutes or implement event-based invalidation
• Consider invalidating wallet cache after successful transaction mutations
• Add manual refresh capability for users

3. Duplicate Loading/Error State 🐛

Location: WalletTransactionLog.tsx:252-258

if (loading) {
  return <div>Loading...</div>;
}

if (error) {
  return <div>{error}</div>;
}

This code is unreachable dead code - it appears AFTER identical checks were removed (lines 264-271 in the diff). This suggests incomplete refactoring.

Recommendation: Remove the duplicate checks entirely.

---

Performance Concerns

1. Nested Loops in Feed Fetching 🐌

Location: FeedList.tsx:86-118

for (const user of fetchedUsers) {
  const userWallets = await getUserWallets(adminKey, user.id);
  // ...
  for (const wallet of filteredWallets) {
    const payments = await getWalletTransactionsSince(...);
  }
}

Issues:

• Sequential API calls (not parallelized)
• If you have 50 users with 2 wallets each = 100+ sequential API calls
• Blocks UI during data loading
• No error recovery if one wallet fails

Recommendation:

// Parallelize user wallet fetches
const walletPromises = fetchedUsers.map(user => 
  getUserWallets(adminKey, user.id).catch(err => {
    console.error(`Failed to fetch wallets for user ${user.id}:`, err);
    return [];
  })
);
const allWalletsResults = await Promise.all(walletPromises);

// Parallelize payment fetches
const paymentPromises = filteredWallets.map(wallet =>
  getWalletTransactionsSince(wallet.inkey, timestamp, null)
    .catch(err => {
      console.error(`Failed for wallet ${wallet.id}:`, err);
      return [];
    })
);
const paymentsResults = await Promise.all(paymentPromises);

2. Same Issue in WalletTransactionLog 🐌

Location: WalletTransactionLog.tsx:68-108

Identical nested loop pattern with sequential API calls.

---

Security Concerns

1. Sensitive Data in sessionStorage 🔒

Location: lnbitsServiceLocal.ts:141-142

sessionStorage.setItem(TOKEN_KEY, accessToken);

Issues:

• Access tokens stored in sessionStorage are vulnerable to XSS attacks
• No encryption or additional protection
• Token expiry is client-side only (can be manipulated)

Recommendation:

• Consider using httpOnly cookies for token storage (if backend supports)
• Implement token rotation
• Add CSRF protection if not already present
• Consider using secure, httpOnly cookies instead

2. No Error Sanitization ⚠️

Location: Multiple files (e.g., FeedList.tsx:222-225)

const errorMessage = error instanceof Error
  ? `Failed to load activity feed: ${error.message}`
  : 'Unable to load activity feed. Please refresh and try again.';

Issues:

• Error messages from API might contain sensitive information
• Stack traces could leak implementation details

Recommendation:

• Sanitize error messages before displaying to users
• Log detailed errors to monitoring service, show generic messages to users

---

Code Quality & Maintainability

1. Magic Numbers 📏

Location: Multiple locations

const DAYS_TO_FETCH = 30;
const thirtyDaysAgo = Date.now() / 1000 - DAYS_TO_FETCH * 24 * 60 * 60;

Recommendation: Extract to constants

const SECONDS_PER_DAY = 86400; // 24 * 60 * 60
const DAYS_TO_FETCH = 30;
const MILLISECONDS_TO_SECONDS = 1000;

2. Inconsistent Naming 🏷️

Location: WalletAllowanceComponent.tsx:65-66

const DAYS_TO_FETCH = 30;
const thirtyDaysAgo = ...

Variable says "thirty" but constant says "30" - should match. Also, the comment in the original code said "7 days ago" but fetched 30 days.

Recommendation: Use consistent naming:

const TRANSACTION_HISTORY_DAYS = 30;
const transactionHistoryStart = Date.now() / 1000 - TRANSACTION_HISTORY_DAYS * 86400;

3. Type Safety Issues 🔒

Location: lnbitsServiceLocal.ts:1115, 1167

return apiCache.allUsers.data as RawApiUser[];
// ...
return pendingUsersRequest as Promise<RawApiUser[]>;

Issues:

• Type assertions bypass TypeScript's type checking
• Mismatch between cache type (User[]) and return type (RawApiUser[])

Recommendation:

// Create separate cache entries with proper types
interface ApiCache {
  allUsers?: CacheEntry<RawApiUser[]>;  // Not User[]
  userWallets: Map<string, CacheEntry<Wallet[]>>;
}

4. Console Logs in Production 🐛

Location: Multiple files

console.log('Total users retrieved:', responseData?.data?.length || 0);
console.log("DATA", data);
console.log("DATA2", filteredPayments);

Recommendation:

• Use the logger utility consistently (already imported)
• Remove or gate debug logs behind environment flag
• Use proper log levels (debug, info, warn, error)

---

🧪 Testing Concerns

Missing Test Coverage

1. Caching logic: No tests for cache hit/miss scenarios, TTL expiration
2. Request deduplication: No tests for concurrent request handling
3. Timestamp parsing: No tests for edge cases (invalid formats, null values, future dates)
4. Error handling: No tests for API failures, network errors

Recommendation: Add unit tests for:

describe('parseTransactionTime', () => {
  it('handles Unix timestamps', () => { ... });
  it('handles ISO strings', () => { ... });
  it('handles invalid input', () => { ... });
  it('returns null for malformed data', () => { ... });
});

describe('API caching', () => {
  it('returns cached data within TTL', () => { ... });
  it('fetches fresh data after TTL expires', () => { ... });
  it('deduplicates concurrent requests', () => { ... });
});

---

🎯 Best Practices Recommendations

1. Extract Timestamp Logic to Utility

Create src/utils/timeUtils.ts:

export const SECONDS_PER_DAY = 86400;
export const MS_TO_SECONDS = 1000;

export function parseTimestamp(timestamp: number | string): Date | null {
  // ... parseTransactionTime logic
}

export function getDaysAgo(days: number): number {
  return Math.floor(Date.now() / MS_TO_SECONDS) - (days * SECONDS_PER_DAY);
}

2. Add Cache Management

Create src/utils/cacheManager.ts:

export class CacheManager {
  clear() { clearApiCache(); }
  
  invalidateUser(userId: string) { ... }
  
  invalidateWallet(walletId: string) { ... }
}

// Hook into logout, account switch, etc.

3. Error Boundary

Wrap components in error boundaries to handle runtime errors gracefully:

<ErrorBoundary fallback={<ErrorFallback />}>
  <FeedList />
</ErrorBoundary>

4. Loading States

Replace simple "Loading..." text with proper loading skeletons for better UX.

---

📋 Summary of Recommendations

Must Fix Before Merge

1. ❌ Remove dead code in WalletTransactionLog.tsx:252-258
2. ❌ Implement cache cleanup on logout/account switch
3. ❌ Parallelize API calls in FeedList.tsx and WalletTransactionLog.tsx

Should Fix

4. ⚠️ Fix type safety issues with cache typing
5. ⚠️ Remove console.logs or replace with logger
6. ⚠️ Reduce user cache TTL to prevent stale data
7. ⚠️ Add manual cache invalidation after mutations

Nice to Have

8. 💡 Extract timestamp utilities to shared module
9. 💡 Add unit tests for caching and timestamp parsing
10. 💡 Implement proper error boundaries
11. 💡 Add loading skeletons

---

🎉 Conclusion

This PR makes important fixes to real bugs and adds valuable performance optimizations. The caching strategy is well-thought-out, and the timestamp handling is more robust. However, there are some critical issues around cache management, performance (sequential API calls), and code quality that should be addressed.

Recommendation: Request changes for must-fix items, then approve once addressed.

Great work on identifying and documenting the root causes! 🚀

[claude]

Pull Request Review: Fix Feed Filter and Wallet Screen Performance Issues

Summary

This PR addresses two critical bugs: (1) non-functioning time filters on the Feed page, and (2) performance issues causing the Wallet screen to freeze. The fixes include proper timestamp parsing, parallelized API calls, and a caching layer to reduce redundant requests.

---

✅ Strengths

1. Well-Documented Problem Analysis

The PR description clearly identifies root causes for both bugs with specific technical details, making the fixes easy to understand and verify.

2. Performance Optimizations

• Parallelization: Excellent use of Promise.all() to fetch wallet and payment data concurrently instead of sequentially (FeedList.tsx:83-92, WalletTransactionLog.tsx:74-109)
• Caching Layer: Smart implementation with appropriate TTLs (60s for users, 15s for wallets) and request deduplication
• Error Handling: Graceful degradation - individual failures don't break the entire feed (lines 87-90, 112-115 in FeedList.tsx)

3. Type Safety Improvements

The parseTransactionTime() helper function properly handles both Unix timestamps and ISO date strings with proper validation (FeedList.tsx:27-40)

---

🔍 Issues & Concerns

CRITICAL: Security - Potential Memory Leak

Location: lnbitsServiceLocal.ts:276-367 (getUserWallets function)

Issue: The pendingWalletRequests Map is populated but entries are only removed in the finally block. If a request throws an error or is never awaited, the promise remains in the Map indefinitely.

Current Code:

pendingWalletRequests.set(userId, requestPromise);
return requestPromise;

Risk: In high-traffic scenarios or with network errors, this could lead to memory leaks.

Recommendation: Consider adding a timeout mechanism or using WeakMap if appropriate.

---

HIGH: Race Condition in useEffect Dependency

Location: WalletTransactionLog.tsx:215

Issue: The useEffect removed users from dependencies (which is good), but it's missing accounts from the new dependency array. The code accesses accounts[0] at line 46, but if accounts changes, the effect won't re-run.

Current Code:

}, [activeWallet, accounts, currentWallet]);

Problem: The guard if (!accounts || accounts.length === 0) was added at line 215, but this creates a timing issue where the effect runs, hits the guard, and never re-runs when accounts become available.

Recommendation: Consider using a separate effect to watch accounts or ensure the effect re-triggers when accounts populate.

---

MEDIUM: Inconsistent Error Handling

Location: FeedList.tsx:87-90 vs lnbitsServiceLocal.ts:350

Issue 1: Silent error suppression in FeedList could hide systematic issues:

} catch (err) {
  // Log error but continue - don't fail entire feed for one user
  return { userId: user.id, wallets: [] };
}

Issue 2: The caching layer catches errors but doesn't differentiate between network failures (which should retry) vs. authentication errors (which shouldn't be cached).

Recommendation:

• Add logging to track how often these errors occur
• Consider differentiating between transient and permanent errors
• Monitor if certain users consistently fail (could indicate data corruption)

---

MEDIUM: Magic Numbers & Constants

Location: Multiple files

Issues:

• MS_PER_SECOND = 1000 (WalletAllowanceComponent.tsx:14) - This is used for division by 1000 at line 79, which appears to be converting millisats to sats, not milliseconds. The constant name is misleading.
• MAX_WALLET_CACHE_SIZE = 100 - No justification for this limit. What happens in organizations with >100 users?

Recommendation:

const MILLISATS_PER_SAT = 1000;
const MAX_WALLET_CACHE_SIZE = 100; // LRU eviction prevents unbounded growth

---

MEDIUM: Potential Cache Invalidation Issue

Location: lnbitsServiceLocal.ts:61-78

Issue: clearApiCache() is called on logout (useTeamsAuth.ts:70), but there's no invalidation when:

• A new transaction is made
• A wallet is created/deleted
• User permissions change

Recommendation: Consider invalidating cache after write operations or implementing cache versioning.

---

LOW: Code Quality Issues

1. Unused Variable (WalletAllowanceComponent.tsx:68):

   const encodedExtra = {};  // Not used

2. Duplicate Loading State Check (WalletTransactionLog.tsx:262-267):

   if (loading) {
     return <div>Loading...</div>;
   }
   // ... same check repeated ...

3. Console Statements: Several console.warn and console.error calls should use the logger utility for consistency

---

🧪 Test Coverage Concerns

CRITICAL MISSING: No tests were added for:

• parseTransactionTime() helper - this should have unit tests for edge cases (null, undefined, invalid strings, Unix timestamps, ISO strings)
• Cache hit/miss logic
• Cache TTL expiration
• Request deduplication

Existing Test Files: The repo has test infrastructure (lnbitsServiceLocal.test.ts exists) but no tests were added/updated for these changes.

Recommendation: Add at minimum:

describe('parseTransactionTime', () => {
  it('should parse Unix timestamp', () => {
    expect(parseTransactionTime(1609459200)).toEqual(new Date(2021, 0, 1));
  });
  
  it('should parse ISO string', () => {
    expect(parseTransactionTime('2021-01-01T00:00:00Z')).toEqual(new Date(2021, 0, 1));
  });
  
  it('should return null for invalid input', () => {
    expect(parseTransactionTime('invalid')).toBeNull();
  });
});

---

🛡️ Security Review

✅ PASS: No obvious injection vulnerabilities

• User inputs are not directly concatenated into queries
• API keys properly use environment variables

⚠️ WARNING: Token Storage

Location: lnbitsServiceLocal.ts:82-99

The token is stored in sessionStorage, which is appropriate, but there's no token refresh logic. If a user keeps a tab open for >24 hours, they'll get authentication errors.

Recommendation: Implement token refresh or clear UX messaging when token expires.

---

📊 Performance Analysis

Before vs After (estimated based on code analysis):

Metric	Before	After	Improvement
Feed Load (10 users)	~2-3s (sequential)	~500ms (parallel)	4-6x faster
Wallet Load	Infinite loop (freeze)	~1s	Fixed
Redundant API Calls	100+ on wallet screen	~10 (cached)	90% reduction

Potential Issues:

• Cache invalidation could cause stale data (balance shows old amount)
• 15s TTL for wallets might be too aggressive if users are actively transacting

---

🎯 Recommendations

Must Fix Before Merge:

1. ✅ Add proper cleanup for pendingWalletRequests Map
2. ✅ Fix the accounts dependency array issue in WalletTransactionLog
3. ✅ Add unit tests for parseTransactionTime()

Should Fix:

4. Rename MS_PER_SECOND to MILLISATS_PER_SAT in WalletAllowanceComponent
5. Add cache invalidation after write operations
6. Remove duplicate loading state checks
7. Add error logging/monitoring for silent failures

Nice to Have:

8. Add metrics/telemetry for cache hit rates
9. Consider implementing progressive cache warming on login
10. Add JSDoc comments for the new caching functions

---

✅ Final Verdict

Recommendation: Approve with required changes

This PR solves real user-facing bugs and includes significant performance improvements. The parallelization and caching strategy is sound. However, the memory leak risk and race condition must be addressed before merging.

Estimated Risk:

• Without fixes: HIGH (memory leaks in production)
• With fixes: LOW-MEDIUM (cache invalidation edge cases)

Great work on the performance optimizations and error handling patterns! 🚀

[EdiWeeks]

Tested, ready to merge @BenGWeeks

[claude]

Pull Request Review: Fix Feed Filter and Wallet Screen Performance Issues

Summary

This PR addresses two important bugs: the feed page filter not working and wallet screen performance issues. The fixes include proper timestamp parsing, caching optimizations, and parallel API request handling. Overall, the implementation is solid with good performance improvements, though there are some areas for improvement.

---

Positive Highlights ✅

Performance Improvements

1. Excellent parallelization in FeedList.tsx:94-103 - Converting sequential wallet fetches to Promise.all() will significantly reduce loading times
2. Smart caching implementation in lnbitsServiceLocal.ts:18-78 with:
   • Appropriate TTLs (60s for users, 15s for wallets)
   • Request deduplication to prevent concurrent duplicate calls
   • Cache size limits to prevent memory bloat
3. Proper cleanup - Added clearApiCache() call in logout handler (useTeamsAuth.ts:70)

Code Quality

1. Good defensive programming - Graceful error handling with fallback to empty arrays instead of failing entire operations
2. Clear documentation - Well-commented cache configuration and wallet type identifiers
3. Type safety - Proper TypeScript types for cache entries and raw API data

---

Issues & Recommendations 🔍

Critical Issues

1. FeedList.tsx:317-329 - Filter Logic Applied AFTER Sorting

The timestamp filter is applied after sorting all transactions. This is inefficient because:

const sortedZaps = [...zaps].sort(...); // Sorts ALL zaps
const filteredZaps = timestamp && timestamp > 0
  ? sortedZaps.filter(zap => { ... }) // Then filters
  : sortedZaps;

Recommendation: Apply filter before sorting to reduce computational overhead:

const filteredZaps = timestamp && timestamp > 0
  ? zaps.filter(zap => { ... }) // Filter first
  : zaps;

const sortedZaps = [...filteredZaps].sort(...); // Sort only filtered results

2. WalletTransactionLog.tsx:65-109 - Unnecessary Duplication

The code fetches wallets for ALL users twice in the same function:

• Once at lines 65-84 (building walletToUserMap)
• Again at lines 86-110 (fetching payments)

This negates the benefit of caching. Recommendation: Reuse the first fetch results instead of making duplicate calls.

3. WalletAllowanceComponent.tsx:75 - Incorrect Math

const spent = transaction
  .filter(t => t.amount < 0)
  .reduce((total, t) => total + Math.abs(t.amount), 0) / MS_PER_SECOND;

This divides the total by 1000 (MS_PER_SECOND), but the comment and variable name suggest this should convert millisats to sats. The original code divided by 1000 correctly. This change appears to be a bug.

Expected: / 1000 (millisats → sats)
Current: / MS_PER_SECOND (which is 1000, but semantically wrong)

Moderate Issues

4. FeedList.tsx:23-37 - Missing Null Handling in parseTransactionTime

const parseTransactionTime = (timestamp: number | string): Date | null => {
  if (typeof timestamp === 'number') {
    return new Date(timestamp * 1000);
  }
  // ...
  return null;
};

For number inputs, there's no validation if the timestamp is valid (e.g., negative numbers, NaN). Recommendation: Add validation:

if (typeof timestamp === 'number') {
  if (!isFinite(timestamp) || timestamp < 0) {
    console.warn(`Invalid numeric timestamp: ${timestamp}`);
    return null;
  }
  return new Date(timestamp * 1000);
}

5. lnbitsServiceLocal.ts:276-360 - Cache Eviction Strategy

The wallet cache uses FIFO eviction (removes first entry when max size reached):

if (apiCache.userWallets.size >= MAX_WALLET_CACHE_SIZE) {
  const firstKey = apiCache.userWallets.keys().next().value;
  if (firstKey) {
    apiCache.userWallets.delete(firstKey);
  }
}

This may remove frequently-accessed entries. Recommendation: Consider LRU (Least Recently Used) eviction or at least remove the oldest by timestamp.

6. Missing Error Context in Catch Blocks

Multiple locations (e.g., FeedList.tsx:99, WalletTransactionLog.tsx:97) have empty catch blocks with only comments. While logging is present elsewhere, these silent failures could hide issues during debugging.

Recommendation: At minimum, log to console:

catch (err) {
  console.error(`Error fetching wallets for user ${user.id}:`, err);
  return { userId: user.id, wallets: [] };
}

Minor Issues

7. Inconsistent Variable Naming

• MS_PER_SECOND (milliseconds per second) is semantically incorrect - should be MILLISATS_PER_SAT or similar
• transactionHistoryStart is clearer than paymentsSinceTimestamp but both are used

8. Code Duplication in WalletTransactionLog.tsx:257-265

Duplicate loading/error checks can be consolidated.

---

Security Considerations 🔒

Good Practices:

✅ Using sessionStorage instead of localStorage for tokens (more secure)
✅ Token expiration handling (24-hour TTL)
✅ No hardcoded credentials or API keys

Potential Concerns:

⚠️ No rate limiting on API calls - the parallel requests could overwhelm the backend if many users are fetched
⚠️ No input sanitization on transaction memos before display (potential XSS if memos contain scripts)

Recommendation: Add rate limiting and sanitize user-generated content (memos) before rendering.

---

Test Coverage 📋

Current State:

• Existing tests in lnbitsServiceLocal.test.ts don't cover new caching logic
• No tests for parseTransactionTime() helper function
• No tests for parallel fetching behavior

Recommendations:

1. Add tests for cache hit/miss scenarios
2. Test cache expiration and eviction
3. Test parseTransactionTime() with edge cases (invalid dates, negative timestamps, etc.)
4. Test request deduplication logic
5. Add integration tests for the timestamp filter fix

Example test case:

describe('parseTransactionTime', () => {
  it('should handle Unix timestamp in seconds', () => {
    const result = parseTransactionTime(1700000000);
    expect(result).toBeInstanceOf(Date);
  });
  
  it('should handle ISO date strings', () => {
    const result = parseTransactionTime('2025-12-08T09:02:07.478617+00:00');
    expect(result).toBeInstanceOf(Date);
  });
  
  it('should return null for invalid input', () => {
    expect(parseTransactionTime('invalid')).toBeNull();
    expect(parseTransactionTime(NaN)).toBeNull();
  });
});

---

Performance Impact Analysis 📊

Improvements:

• Feed loading: ~70-80% faster due to parallel wallet fetches (N sequential → 1 parallel batch)
• Wallet screen: ~90% reduction in redundant API calls with 60s user cache + 15s wallet cache
• Request deduplication: Prevents duplicate concurrent calls

Potential Regressions:

• Filter-after-sort could be slower with large datasets (recommend fixing as noted above)
• Double wallet fetch in WalletTransactionLog.tsx reduces cache effectiveness

---

Overall Assessment

Quality: Good ⭐⭐⭐⭐
Performance: Excellent ⭐⭐⭐⭐⭐
Security: Adequate ⭐⭐⭐
Test Coverage: Needs Improvement ⭐⭐

Recommendation: ✅ Approve with changes requested

The PR successfully fixes both reported bugs and introduces significant performance improvements. However, please address:

1. Critical: Fix the incorrect division in WalletAllowanceComponent.tsx:75
2. Critical: Remove duplicate wallet fetching in WalletTransactionLog.tsx
3. High Priority: Move filter before sort in FeedList.tsx
4. Medium Priority: Add tests for new caching logic and helper functions

Once these issues are addressed, this will be an excellent contribution to the codebase.

---

Great work on the performance optimizations! The caching strategy and parallelization improvements show strong engineering judgment. 👍

[akash2017sky]

CRITICAL: Security - Potential Memory Leak, Race Condition, Inconsistent Error Handling - It's a common issue for whole app and will need a separate fixing as a medium priority.
New issue created #139 and #140

For performance, we already have an issue created #130

Unused Variable (WalletAllowanceComponent.tsx:68): - Already removed the unused variable in the previous commit

@EdiWeeks can you test this again? this branch had a few merge conflicts.

[EdiWeeks]

Wallet transaction history is temperamental after sending zap, I created issue for it to be fixed in here: knowall-ai/zaplie-webapp#3
@akash2017sky @BenGWeeks