Implement complete Sonora audio NFT marketplace functionality

[evanmcfarland]

No description provided.

[github-actions]

Pull Request Review: Sonora Audio NFT Marketplace

This is a comprehensive implementation of a complete audio NFT marketplace with 3,461 additions across 42 files. The work demonstrates solid engineering with good separation of concerns and feature organization. Here's my detailed feedback:

---

✅ Strengths

Code Architecture

• Excellent separation of concerns: Features are well-organized with clear boundaries between components, hooks, API layers, and state management
• Consistent patterns: The implementation follows established patterns from permasearch and pinax features
• Type safety: Good use of TypeScript interfaces and types throughout
• State management: Well-structured Redux slices with clear responsibilities (archive, studio, market)

Feature Completeness

• Complete marketplace workflow: Archive → Studio → Market
• Full ICRC-2 approval flow for secure NFT purchases
• Proper error handling and loading states throughout
• Audio recording with MediaRecorder API integration
• Arweave integration for permanent storage

---

⚠️ Issues & Concerns

🔴 Critical Issues

1. Pagination Logic Flaw (fetchMarketAudioNFTs.ts:193-197)

const audioRatio = audioNFTs.length / filteredListings.length || 0;
const estimatedTotalAudio = Number(listingsResponse.total_count) * audioRatio;
const estimatedTotalPages = Math.ceil(estimatedTotalAudio / pageSize);

Problem: This estimation is unreliable. If page 1 has 8 audio NFTs out of 8 listings (100% ratio), but page 2 has 0 audio NFTs, the estimation breaks down. This will cause:

• Incorrect "Load More" button visibility
• Potential infinite loops or missing data
• User confusion about available content

Recommendation: Either:

• Fetch all listings first, filter for audio, then paginate client-side (better UX)
• Or use server-side filtering if the backend supports content-type filtering
• Or implement cursor-based pagination instead of offset-based

2. Price Calculation Precision Issues (useBuyAudio.ts:40-43)

const priceNum = parseFloat(price); // price is a string like "1.234 ICP"
const amountFormatApprove = BigInt(
    Number((priceNum + 0.0001) * 10 ** 8).toFixed(0)
);

Problem:

• Parsing "1.234 ICP" with parseFloat will fail, resulting in NaN
• Floating point arithmetic before BigInt conversion can lose precision
• The 0.0001 fee is hardcoded and may not match actual ledger fees

Recommendation:

// Extract number from price string properly
const priceNum = parseFloat(price.replace(/[^\d.]/g, ''));
// Use precise BigInt arithmetic
const priceE8s = BigInt(Math.floor(priceNum * 100_000_000));
const feeE8s = BigInt(10000); // Use exact fee value
const amountFormatApprove = priceE8s + feeE8s;

3. Memory Leak Risk (AudioCard.tsx:27-28, 91)

const audioUrl = item.id.startsWith('blob:') ? item.id : `https://arweave.net/${item.id}`;
// ...
audioRef.current.src = '';

Problem: Blob URLs created with URL.createObjectURL() are not revoked when the component unmounts or audio changes. This causes memory leaks over time.

Recommendation:

useEffect(() => {
    return () => {
        if (audioRef.current?.src?.startsWith('blob:')) {
            URL.revokeObjectURL(audioRef.current.src);
        }
    };
}, []);

🟡 Significant Issues

4. No Input Validation on User Input (BuyButton.tsx, SellButton.tsx, EditButton.tsx)

The dialog forms accept price inputs without validation:

• No minimum price check
• No maximum price check
• No format validation (could enter negative numbers, letters, etc.)
• No check for reasonable decimal places

Recommendation: Add Zod schema validation or input constraints.

5. Race Condition in Audio Playback (AudioCard.tsx:114-119)

document.querySelectorAll('audio').forEach(otherAudio => {
    if (otherAudio !== audio) {
        otherAudio.pause();
        otherAudio.src = '';
    }
});

Problem: If multiple users click play rapidly on different cards, there's a race condition. The global selector approach is brittle.

Recommendation: Use a global audio player service/context to manage playback state centrally.

6. Inconsistent Error Handling

• Some functions throw errors, others return error objects
• Error messages are inconsistent (some technical, some user-friendly)
• No error boundaries to catch component errors

Recommendation: Standardize error handling with a consistent approach across the codebase.

7. Missing Abort Controller Cleanup (Multiple API files)

While signal is passed to fetch calls, the abort controllers aren't always properly cleaned up when components unmount.

---

🔵 Performance Concerns

1. Multiple Sequential API Calls (fetchUserAudioNFTs.ts, fetchStudioAudioNFTs.ts)

Each page fetch makes:

1. Multiple canister calls (ICRC7, Emporium)
2. Arweave GraphQL query
3. Data processing and filtering

Impact: Slow page loads, especially with poor network conditions.

Recommendation:

• Implement parallel fetching where possible
• Add caching layer (React Query is already used, leverage it better)
• Consider virtualization for long lists

2. No Audio Preloading Strategy

Audio files are loaded on-demand when play is clicked. For better UX:

• Preload metadata with preload="metadata" (already done ✓)
• Consider preloading the first few items in viewport

3. Large GraphQL Queries

Fetching up to 100 transactions at once (fetchMarketAudioNFTs.ts:122) could be slow and wasteful if only 8 are displayed.

---

🔒 Security Concerns

1. CORS and Content Security (AudioCard.tsx:183)

crossOrigin="anonymous"

This allows loading audio from any source. While necessary for Arweave, it could be a vector for attack if IDs are user-controlled.

Recommendation: Validate Arweave transaction IDs before constructing URLs.

2. Principal Display Truncation (AudioCard.tsx:161-168)

Showing truncated principals could lead to spoofing attacks where similar-looking principals are confused.

Recommendation: Consider showing a visual hash/avatar or providing a copy-to-clipboard feature for full verification.

3. No Rate Limiting

Multiple rapid "Load More" clicks could spam the backend.

Recommendation: Implement debouncing or disable the button during loading.

---

📊 Test Coverage

Major Gap: No tests for any Sonora features despite significant business logic.

High Priority Test Needs:

1. useBuyAudio - Price calculation, approval flow
2. fetchMarketAudioNFTs - Pagination logic
3. Audio recording workflow
4. Error handling paths
5. State management (Redux slices)

Recommendation: Add unit tests for critical business logic, especially financial transactions.

---

🎨 Code Quality Observations

Good Practices ✅

• Consistent naming conventions
• Clear component structure
• Good use of TypeScript
• Proper cleanup in useEffects
• Loading and error states implemented

Areas for Improvement

1. Magic Numbers: 100_000_000, 10000, 30 blocks - use named constants
2. Console.logs: Many debug logs left in production code (fetchMarketAudioNFTs.ts:75, 89, 97, etc.)
3. Commented Code: Some TODO comments should be addressed or removed
4. Duplicate Code: Audio content types list repeated in multiple files (create shared constant)
5. Complex Functions: Some functions exceed 100 lines (break into smaller utilities)

---

📝 Additional Recommendations

Documentation

• Add JSDoc comments for complex functions
• Document the ICRC-2 approval flow (it's not intuitive)
• Add README or developer guide for Sonora feature

NFT Manager DID Changes

type AudioNFT = record {
    arweave_id : text;
    timestamp : nat64;
    owner : principal;
};

The DID file changes suggest new functionality, but no implementation is provided. Is this for future use?

Refactoring Opportunities

1. Extract useUploadAndMint hook could be more generic and reusable
2. Audio filtering logic (audioFilters.ts) could use functional composition
3. Consider creating a shared useAudioPlayer hook to reduce duplication

---

🎯 Action Items Summary

Before Merge (Blockers):

1. ✅ Fix price parsing in useBuyAudio.ts
2. ✅ Fix pagination estimation in fetchMarketAudioNFTs.ts
3. ✅ Fix memory leak with blob URLs
4. ✅ Add input validation for price fields

High Priority (Post-Merge):

1. Add test coverage for financial logic
2. Implement proper error boundaries
3. Remove debug console.logs
4. Standardize error handling

Medium Priority:

1. Performance optimization for API calls
2. Extract magic numbers to constants
3. Add JSDoc documentation
4. Implement rate limiting

---

🎉 Conclusion

This is solid work with a well-architected feature implementation. The code demonstrates good understanding of the existing codebase patterns and implements a complete user journey. However, the critical issues around price calculation and pagination need to be addressed before merging to avoid financial errors and poor UX.

The feature is nearly production-ready with the recommended fixes.

Estimated merge readiness: 85% - Fix the 4 critical/significant issues and this is good to go!

Great work on this comprehensive implementation! 🚀