Fix/authentication

[evanmcfarland]

No description provided.

[claude]

Pull Request Review - Fix/Authentication (#135)

Overview

This PR adds a new Sonora audio app and comprehensive authentication settings UI for multiple blockchain providers. Changes: 47 files, 3,942 additions, 20 deletions.

---

Code Quality & Best Practices

Strengths

• Good separation of concerns with organized feature structure
• Consistent naming conventions and TypeScript usage
• Proper Redux Toolkit patterns with slices and action creators

Critical Issue: Code Duplication

The four auth settings components (Ethereum/Solana/Arweave/Oisy) share 95% identical code (500+ lines each). This violates DRY principles and increases maintenance burden.

---

Bugs & Issues Found

Critical Bugs

1. Memory Leak in AudioPlayer (AudioPlayer.tsx:9-10)

• Problem: selected.id.includes('.') is too broad, will match Arweave IDs with periods
• Fix: Use selected.id.startsWith('http') instead

2. Memory Leak in UploadPage (UploadPage.tsx:64)

• Problem: Object URLs not revoked on unmount
• Fix: Add useEffect cleanup to revoke URLs
• Same issue in RecordPage.tsx:90

3. Microphone Stream Not Released (RecordPage.tsx:68-70)

• Problem: Stream stays active if user navigates away while recording
• Fix: Add cleanup effect to stop all tracks on unmount

4. Alert Usage (UploadPage.tsx:77, RecordPage.tsx:102)

• Problem: Using alert() blocks UI thread, poor UX
• Fix: Replace with toast notifications

Medium Priority

5. Audio Player Not Persistent (SonoraLayout.tsx:12-14)

• Problem: clearSelected() on navigation defeats persistent player from SONORA.md
• Fix: Remove or only clear on specific routes

6. Generic Error Messages (RecordPage.tsx:55)

• Problem: Doesnt show actual error details
• Fix: Log actual error and show meaningful message

---

Security Concerns

1. Hardcoded Default Salts (All Auth Settings)

• Default salt values are predictable: ethereum-mulauth-salt
• Risk: Compromises security if users dont change defaults
• Fix: Generate random salts or add production validation

2. Insecure Default URI

• Uses http://localhost:8080 by default
• Fix: Use environment-aware defaults, enforce HTTPS

3. No Input Sanitization

• Auth settings accept user input without validation
• Risk: Potential injection attacks
• Fix: Add URL validation and sanitization

4. Webhook Secret Logging (webhook.rs:264-269)

• Prints partial secrets to logs
• Fix: Remove or guard with debug assertions

---

Performance Issues

1. Unnecessary re-renders - clearSelected() on every navigation
2. Large bundle size - 2000+ lines of duplicated auth code (~15-20KB)
3. URL recreation - AudioPlayer recalculates URL every render (needs useMemo)
4. No lazy loading - Audio files load immediately

---

Test Coverage: MISSING

No tests included for new features. Recommendation: Add minimum 70% test coverage before merge.

---

Summary

BLOCKING (Must Fix)

• Fix memory leaks (UploadPage, RecordPage, AudioPlayer)
• Fix microphone stream cleanup
• Remove hardcoded salts or add warnings
• Replace alert() with proper UI
• Add test coverage for critical paths

HIGH PRIORITY

• Refactor duplicated auth settings components
• Fix AudioPlayer URL logic
• Remove webhook secret logging
• Fix persistent audio player behavior

---

Recommendation

DO NOT MERGE until blocking issues fixed. The architecture is solid, but memory leaks and security issues need addressing first.

Great work overall - just needs polish for production!

---

Review by Claude Code