Implemention for "Send some zaps" pop-up

[akash2017sky]

Summary

This PR implements a "Send Zaps" feature, allowing users to send zaps directly from their Allowance wallet to team members, along with several UI improvements and bug fixes.

Key Features

• Send Zaps Popup: New popup component (SendZapsPopup.tsx) that allows users to:
• Select a team member from a dropdown (with GUID filtering to show only users with valid names/emails)
• Enter custom amount or use preset amounts (5,000 / 10,000 / 25,000)
• Add a memo/description for the payment
• Send anonymously (hides sender name in feed)
• View available balance before sending
• Anonymous Sending: Users can opt to send zaps anonymously - the sender shows as "Anonymous" in the activity feed while the memo is preserved
• UI/Layout Improvements:
• Repositioned "Send some zaps" button next to the battery indicator in the Allowance section
• Improved dropdown to filter out users with invalid names (GUIDs) and display email as fallback
• Updated button and layout styling for better visual alignment

Screenshot

Files Changed

• SendZapsPopup.tsx - New component for sending zaps
• SendZapsPopup.module.css - Styling for the popup
• WalletAllowanceComponent.tsx - Added button and layout changes
• WalletAllowanceComponent.css - Updated styles
• FeedList.tsx - Anonymous sender detection

Closes #126

[claude]

Pull Request Review - Send Zaps Feature

Thank you for this contribution! I've reviewed the implementation of the Send Zaps feature. Below is my detailed feedback organized by category.

Strengths

1. Well-structured component architecture - Clean separation between the popup component and parent component
2. Good UX considerations - Preset amounts, anonymous sending, user filtering
3. Proper error handling - Validation checks before sending transactions
4. Loading states - Good feedback during async operations

Potential Bugs and Issues

Critical Issues

1. Missing Type Imports (SendZapsPopup.tsx:27) - User and Wallet types are used but not imported. This will cause TypeScript compilation errors. Add: import type { User, Wallet } from '../types/global';

2. Race Condition in Balance Update (SendZapsPopup.tsx:199-200) - Local balance state is updated optimistically but wont reflect actual wallet state. If user closes and reopens popup, balance will be stale.

3. selectedValue State Unused (SendZapsPopup.tsx:33, 338-348) - The Value dropdown selection is captured but never used in the payment. Either remove it or include it in the memo/transaction metadata.

Medium Issues

4. Error State Management (SendZapsPopup.tsx:426-443) - When Try Again is clicked, it only clears the error but doesnt reset form state.

5. Inconsistent User Filtering Logic (SendZapsPopup.tsx:272-278, 281-284) - GUID detection regex appears twice. Extract to a helper function.

6. Wallet Fallback Logic (SendZapsPopup.tsx:92-97) - Falls back to first available wallet if private wallet not found. Could accidentally send to wrong wallet type.

Security Concerns

Critical Security Issues

1. Admin Key Exposure (SendZapsPopup.tsx:11) - Admin key is hardcoded and exposed in client-side code. SECURITY RISK: Anyone can inspect the bundle and extract this key. FIX: Move all admin key operations to server-side API endpoints. The getUsers() and getUserWallets() calls with adminKey should be backend operations.

2. No Transaction Amount Limits - Only checks if amount > 0 and amount <= balance. No maximum transaction limit to prevent accidental large transfers.

3. No Rate Limiting - Users could spam transactions. Add client-side debouncing and server-side rate limiting.

4. Anonymous Detection Logic (FeedList.tsx:358-360) - Uses string prefix [Anonymous] in memo which can be spoofed. Use transaction metadata/extra field for anonymous flag instead.

Performance Considerations

1. Expensive Filtering on Each Render (SendZapsPopup.tsx:271-290) - GUID regex and filtering runs on every render. Memoize filtered users with useMemo().

2. Large CSS File (SendZapsPopup.module.css: 594 lines) - Contains legacy/unused styles that should be removed.

Test Coverage

Missing Tests - No test file found for SendZapsPopup component. Add unit tests for user filtering, amount validation, balance checking, and integration tests for the payment flow.

Code Quality and Best Practices

Improvements Needed

1. Console Logging - Multiple console.log statements in production code (lines 65, 67, 69, 84, 90)

2. Magic Numbers - PRESET_AMOUNTS and balance_msat / 1000 conversion factor should be extracted as named constants

3. Accessibility - No ARIA labels on form elements, modal doesnt trap focus

4. Layout Hardcoded Values (WalletAllowanceComponent.tsx:119) - gap: 160px may break on different screen sizes

Summary and Recommendations

Must Fix Before Merge:

1. ⚠️ CRITICAL: Remove admin key from client-side code - migrate to backend API
2. Add missing TypeScript imports
3. Fix balance update race condition
4. Remove or utilize the unused Value dropdown

Should Fix:

1. Add comprehensive test coverage
2. Remove console.log statements
3. Add accessibility attributes
4. Extract duplicate GUID detection logic
5. Add maximum transaction limits

Nice to Have:

1. Memoize expensive filtering operations
2. Add rate limiting UI feedback
3. Improve error handling UX
4. Clean up unused CSS

Overall Assessment: The feature implementation is solid with good UX considerations, but has critical security issues (admin key exposure) that must be addressed before production deployment. Great work on the feature! The UI looks polished and the user experience is well thought out. Please address the security concerns as highest priority.

[claude]

Pull Request Review: Send Zaps Feature

This PR implements a Send Zaps feature. Overall well-structured with good UX, but has critical security issues.

Critical Issues (Must Fix)

1. SECURITY: Admin Key Exposed in Client (SendZapsPopup.tsx:11)

• Admin key is in client-side code and will be visible in bundled JavaScript
• Risk: HIGH - Allows unauthorized wallet access and transactions
• Fix: Move wallet operations to backend API with proper auth

2. Race Condition in Balance Updates (SendZapsPopup.tsx:199-200)

• Balance updated optimistically without server refresh
• Multiple transactions will show incorrect balance
• Fix: Fetch fresh balance from server after payment

3. No Transaction ID Stored (SendZapsPopup.tsx:196)

• Payment hash not stored or displayed
• Users cannot track transactions
• Fix: Store and display payment_hash

High Priority Issues

4. Missing Error Handling (SendZapsPopup.tsx:171-209)

• No handling for partial failures or network timeouts
• Fix: Add specific error cases and timeout handling

5. Unsafe Wallet Fallback (SendZapsPopup.tsx:93-96)

• Falls back to first wallet if no private wallet found
• Could send to allowance wallet instead
• Fix: Only allow sending to explicit private wallets

6. Unused State Variable (SendZapsPopup.tsx:33)

• Value dropdown collected but never used
• Fix: Use in memo or remove

Medium Priority Issues

7. Console.log in Production (multiple lines)

• Use logger utility or development checks

8. Performance: Multiple Wallet Fetches (SendZapsPopup.tsx:86-108)

• Fetches all user wallets on mount
• Fix: Lazy load when user selected

9. Anonymous Feature Incomplete

• Only hides display name, not avatar or wallet ID

10. Magic String

• Extract [Anonymous] to constant

Minor Issues

11. Type Safety (line 51) - Type assertion bypasses checks
12. Large CSS File - 594 lines with unused legacy styles
13. Accessibility - No ARIA labels, keyboard trap, or focus management
14. Memo Field - Not required, users can send without description

Testing

No tests included. Add:

• Unit tests for validation
• Integration tests for send flow
• E2E tests

Verdict

Well-implemented feature but CRITICAL SECURITY ISSUES must be fixed before merge.

Recommendation: REQUEST CHANGES

Great work on the UI and UX! Once security is addressed, this will be excellent.

[claude]

PR Review - Send Zaps Feature

Overview

This PR implements a comprehensive "Send Zaps" feature allowing users to send cryptocurrency payments to team members. Overall, the implementation is well-structured with good UI/UX considerations. Below are my findings across different categories.

---

✅ Strengths

1. Good UX Design: The popup has a polished UI with preset amounts, anonymous sending, and clear visual feedback
2. Proper State Management: Loading states, error handling, and success states are well managed
3. Input Validation: Amount and user selection validation before sending
4. Balance Checking: Prevents overspending by validating against available balance
5. Anonymous Feature: Well-implemented with memo prefixing approach

---

🔴 Critical Issues

1. Missing Type Definitions (SendZapsPopup.tsx:27)

const [users, setUsers] = useState<User[]>([]);

The User interface is imported from global.d.ts, but the extended User type with privateWallet property used in this component doesn't match the global definition. The code extends users with privateWallet at line 105-108, but this creates a type mismatch.

Fix: Create a local extended type:

type UserWithWallet = User & { privateWallet: Wallet | null };
const [users, setUsers] = useState<UserWithWallet[]>([]);

2. Security: Admin Key Exposure (SendZapsPopup.tsx:11, WalletAllowanceComponent.tsx:11)

const adminKey = process.env.REACT_APP_LNBITS_ADMINKEY as string;

The admin key is stored in client-side code and exposed in the browser. This is a critical security vulnerability. Admin keys should never be in frontend code.

Fix: Move wallet operations to a backend API that securely stores and uses the admin key.

3. Race Condition in Balance Updates (SendZapsPopup.tsx:210-211)

const updatedBalance = currentUserWallets.balance - zapAmount;
setCurrentUserWallets(prev => ({ ...prev, balance: updatedBalance }));

The optimistic update here could cause issues if:

• Multiple sends happen quickly
• The transaction fails after setState
• The popup is reopened before API refresh

Fix: Either remove optimistic updates or implement proper transaction queuing and rollback.

---

⚠️ Major Issues

4. Production Console Logs (SendZapsPopup.tsx:54,60,66,70,75,80,90,96)

Multiple console.log statements throughout the code will clutter production logs:

console.log('Fetching users from API...');
console.log('All users:', allUsers.length);
// ... 8+ more instances

Fix: Use a proper logging utility (like the logger from utils/logger used elsewhere) or remove debug logs.

5. Inefficient API Calls (SendZapsPopup.tsx:92-114)

The component fetches wallet data for every user in parallel on mount:

const usersWithWallets = await Promise.all(
  otherUsers.map(async (user) => {
    const wallets = await getUserWallets(adminKey, user.id);
    // ...
  })
);

Impact: With 50 users, this makes 50+ API calls on popup open.

Fix:

• Only fetch wallet data for the selected recipient
• Or implement backend filtering to return users with their target wallets in one call
• Consider pagination for large teams

6. Missing Error Boundaries (SendZapsPopup.tsx)

If the component crashes (e.g., during wallet fetching), it could break the entire UI with no graceful fallback.

Fix: Wrap the component in an Error Boundary or add try-catch blocks around render logic.

7. Wallet Selection Logic Fragility (SendZapsPopup.tsx:99-102)

let targetWallet = wallets?.find(w => w.name.toLowerCase().includes('private'));
if (!targetWallet && wallets && wallets.length > 0) {
  targetWallet = wallets[0]; // Falls back to any wallet
}

Issue: Falls back to "first wallet" which could be incorrect (e.g., allowance instead of private).

Fix: Define explicit wallet naming conventions or add wallet type fields to prevent wrong wallet selection.

---

⚙️ Code Quality Issues

8. Memo String Manipulation (FeedList.tsx:358-360, SendZapsPopup.tsx:179-185)

The anonymous detection relies on string prefix matching:

{zap.transaction.memo?.startsWith('[Anonymous]') ? 'Anonymous' : ...}

Issues:

• Fragile if memo format changes
• Could be circumvented by users manually adding [Anonymous] to memos
• Magic strings scattered across files

Fix: Use a structured approach:

// In transaction extra field:
{ anonymous: true, value: 'teamwork', originalMemo: '...' }

9. Inconsistent Null Checking (SendZapsPopup.tsx:375-376)

title={zap.transaction.memo?.replace('[Anonymous] ', '')}
{zap.transaction.memo?.replace('[Anonymous] ', '')}

Uses optional chaining but .replace() could still fail if memo is undefined (though unlikely with optional chaining on the object).

Fix: Add explicit null check or default:

{(zap.transaction.memo || '').replace('[Anonymous] ', '')}

10. Magic Numbers (SendZapsPopup.tsx:17)

const PRESET_AMOUNTS = [5000, 10000, 25000];

Issue: Hardcoded amounts aren't configurable per organization.

Fix: Move to config file or make it an admin setting.

11. Incomplete eslint-disable Comment (SendZapsPopup.tsx:126-127)

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [accounts]); // cache and setCache are from context and are stable, intentionally excluded

Issue: The comment says "intentionally excluded" but this could mask real dependency issues. If cache/setCache aren't stable, this creates stale closure bugs.

Fix: Verify that useCache returns stable references, or properly memoize the effect logic.

---

🎯 Performance Considerations

12. No Debouncing on Amount Input (SendZapsPopup.tsx:319)

onChange={(e) => setAmount(e.target.value)}

Issue: Every keystroke triggers a re-render. Not critical but could be optimized.

Fix: Add debouncing for amount validation if validation becomes more complex.

13. Large CSS File (SendZapsPopup.module.css - 619 lines)

The CSS file is quite large for a single component. Consider:

• Extracting shared styles (buttons, inputs) to a common stylesheet
• Using CSS variables for theming
• Removing unused legacy styles (lines 540-619)

---

🔒 Security Concerns

14. No Transaction Amount Limits

There's no maximum transaction limit, allowing users to drain entire allowances in one send.

Fix: Add configurable max transaction amounts and rate limiting.

15. Missing CSRF Protection

The payInvoice and createInvoice calls don't show CSRF token usage (would need to check the service implementation).

Verify: Ensure the backend API has CSRF protection for state-changing operations.

---

🧪 Test Coverage

16. No Tests Visible

The PR doesn't include tests for:

• Send zap functionality
• Anonymous sending
• Balance validation
• Error scenarios (insufficient balance, failed payment)
• User filtering logic

Recommendation: Add unit tests for SendZapsPopup component and integration tests for the zap flow.

---

📝 Minor Issues

17. Typo in class name (WalletAllowanceComponent.css:157)

.nextAllwanceContainer

Should be: .nextAllowanceContainer

18. Unused CSS properties (WalletAllowanceComponent.css)

text-align: center,;  /* Line 137 - extra comma */

19. Inconsistent spacing (WalletAllowanceComponent.tsx:119)

style={{ display: 'flex', alignItems: 'center', gap: '160px' }}

The 160px gap seems arbitrary. Consider using a CSS class or CSS variable.

20. Missing accessibility attributes

• No aria-label on form inputs
• No role attributes for the popup overlay
• No focus trap for keyboard navigation

---

🎨 Suggestions for Future Enhancement

1. Transaction Confirmation: Add a confirmation dialog before sending large amounts
2. Transaction History: Link to full transaction details from the success message
3. Multiple Recipients: Allow sending to multiple users at once
4. Templates: Save frequent send configurations (amount + memo + value)
5. Wallet Refresh: Add a manual refresh button to update balance without reopening popup

---

Summary

Recommendation: ⚠️ Request changes before merge

Must Fix Before Merge:

• ✋ Security: Remove admin key from frontend (Issue React Webapp added #2)
• ⚠️ Fix type definitions for extended User type (Issue Changed code to use MS Teams Bot Framework with LNBits #1)
• 🐛 Remove/replace production console.logs (Issue [ENHANCEMENT 91563] Send zap command should check (and show) users balance #4)
• 🔧 Fix wallet selection logic to prevent wrong wallet usage (Issue Feature 90545: SSO Authentication #7)

Should Fix:

• Optimize API calls for user wallet fetching (Issue Zap feed component is not showing from/to #5)
• Add transaction amount limits (Issue User Story 90317: As a user, I want to see the available rewards (updated after comments)react app rewards carousel component #14)
• Fix race condition in balance updates (Issue React web app & bot updates #3)

Nice to Have:

• Add tests
• Improve accessibility
• Fix CSS typos and inconsistencies

---

Great work on the UX and feature completeness! The core functionality is solid, but the security and performance issues need addressing before this goes to production.

[claude]

Pull Request Review - Send Zaps Feature

Summary

This PR implements a new Send Zaps feature allowing users to send zaps from their Allowance wallet to team members. The implementation includes a well-designed popup UI, anonymous sending capability, and integration with the existing LNbits service.

Critical Issues

1. Security: Admin Key Exposure (SendZapsPopup.tsx:11)

Using the admin key in frontend code is a CRITICAL security vulnerability. Anyone can inspect the JavaScript bundle and extract the admin key, gaining full administrative access to the LNbits instance.

Recommendation: Create a backend API endpoint that handles zap transactions server-side. The admin key should only exist on the server.

2. Environment File Committed (.env.development)

The PR includes changes to .env.development with actual credentials including admin key, password, tenant ID and client ID. This file should NEVER be committed.

Immediate action required:

• Rotate all exposed credentials immediately
• Add .env.development to .gitignore
• Remove from git history
• Use .env.example with placeholder values instead

3. Settings File Committed (.claude/settings.local.json)

This local configuration file should not be in version control. Add to .gitignore.

4. Wiki Submodule Added (Zapl.ie.wiki)

Unclear why this submodule addition is in this PR. If unrelated, should be removed.

Bugs and Issues

Silent Error Handling (SendZapsPopup.tsx:127-129)

Swallowing errors makes debugging difficult. At minimum, log errors to console.

GUID Regex Pattern (SendZapsPopup.tsx:288, 295)

Standard GUIDs include hyphens. The regex only matches unhyphenated versions.

Race Condition in Balance Updates (SendZapsPopup.tsx:213-214)

Optimistic update does not guarantee accuracy. Consider refreshing balance from API after successful payment.

Magic Numbers (WalletAllowanceComponent.tsx:65)

Comment says seven days but calculates 30 days. Extract to named constant.

Testing

No test files added for the new functionality. Recommend adding tests for form validation, anonymous sending, balance validation, and user filtering.

Strengths

• Well-structured React component with clear separation of concerns
• Good UX with loading states, success/error handling, and validation
• Responsive design with mobile support
• Proper TypeScript usage
• On-demand wallet fetching for performance
• Nice features: anonymous sending, value categorization, preset amounts

Overall Assessment

This is a well-implemented feature with good UX and code structure, but has CRITICAL security vulnerabilities that must be addressed before merging. The admin key exposure and committed credentials are serious issues needing immediate attention.

Recommendation: REQUEST CHANGES - do not merge until security issues are resolved.

---

Review generated with assistance from Claude Code

[akash2017sky]

@BenGWeeks , this is ready to review/approve.
Fixed all the critical comments code review, except for the following, for which we already raised the issue

1. Unit testing - [ENHANCEMENT] Add unit test for overall project #116
2. Admin key and other parameters defined in the env variable [ENHANCEMENT] Critical Security Issue: Sensitive Credentials Exposed in Client-Side Bundle #129
3. Race condition will be covered under the performance issue [ENHANCEMENT] Performance Optimization: Reduce API Calls and Improve Load Times #130

[BenGWeeks]

Can we be consistent with terminology please and use "Sats" not "coins" (that term may have been configurable in the backend so check how it's implemented elsewhere). Please review @EdiWeeks

[akash2017sky]

Actually, we have a settings page where you can define the reward's name.

[EdiWeeks]

Tested this, findings logged:
Failed to display transaction history (#142)
Display sats by default (#141)
@akash2017sky @BenGWeeks

[akash2017sky]

Hi @EdiWeeks
This is already raised and fixed in a separate PR #135
PR - #138

[EdiWeeks]

Tested, no critical issues. Bug mentioned are fixed in further PRs. @BenGWeeks please merge.