Skip to content

Conversation

@ben-kaufman
Copy link
Contributor

@ben-kaufman ben-kaufman commented Nov 28, 2025

Fix issue where after first receive wallet UI did not load properly (as in the image)

IMG_9428

@claude
Copy link

claude bot commented Nov 28, 2025

PR Review: Fix Wallet UI Not Loading After Payment Received

Summary

This PR fixes a UI bug where the wallet view would not properly display after receiving the first payment. The fix addresses a race condition in the state management logic.


✅ Code Quality & Best Practices

Strengths:

  1. Proper use of withAnimation - The change from DispatchQueue.main.async to withAnimation(.spring(response: 0.3)) is more idiomatic SwiftUI and matches the animation modifier already applied on line 74
  2. Better state synchronization - Using the newValue parameter in onChange is more explicit and reliable than reading from wallet.totalBalanceSats directly
  3. Condition refinement - Adding the && app.showHomeViewEmptyState check prevents unnecessary state updates when the empty state is already hidden

Minor Suggestion:

  • Consider adding a code comment explaining why the condition on line 25 needs both checks: if !app.showHomeViewEmptyState || wallet.totalBalanceSats > 0. This prevents future refactoring from breaking the fix.
// Show wallet UI if: empty state is hidden OR balance exists
// The OR condition handles edge case where balance arrives before empty state flag updates
if !app.showHomeViewEmptyState || wallet.totalBalanceSats > 0 {

🐛 Bug Analysis

Root Cause:
The issue appears to be a timing race condition:

  1. User receives first payment → wallet.totalBalanceSats updates to > 0
  2. The UI condition if !app.showHomeViewEmptyState evaluates while showHomeViewEmptyState is still true
  3. Wallet UI doesn't render until state updates complete

The Fix:

  • Line 25: Adding || wallet.totalBalanceSats > 0 ensures the wallet UI renders immediately when balance exists, regardless of the empty state flag
  • Lines 89-94: The refined onChange handler now only updates the flag when needed, preventing redundant state changes

This is a solid fix! 👍


⚡ Performance Considerations

Positive:

  • Replacing DispatchQueue.main.async with withAnimation removes unnecessary async dispatch overhead
  • Adding && app.showHomeViewEmptyState guard prevents unnecessary state mutations when the flag is already false

Potential Issue:

  • Line 74 and 88 both have .animation(.spring(response: 0.3), value: app.showHomeViewEmptyState) - this is duplicate. According to SwiftUI best practices, you should only need one .animation() modifier per value. Consider removing line 88 (the second one).

🔒 Security Concerns

No security issues identified. This is purely UI state management.


🧪 Test Coverage

Recommendation:
This bug appears to be a timing/race condition issue that would be difficult to catch in unit tests. Consider:

  1. Manual testing checklist:

    • Fresh wallet (balance = 0) shows empty state
    • First payment received → wallet UI appears correctly
    • Subsequent payments don't cause UI glitches
    • Closing and dismissing empty state manually still works
    • Pull-to-refresh works after receiving payment
  2. Edge case to test:

    • What happens if totalBalanceSats changes from > 0 back to 0? (e.g., user spends all funds)
    • The current logic doesn't reset showHomeViewEmptyState back to true. Is this intended behavior?

📝 Additional Notes

Consistency Check:
The empty state overlay condition (line 76) checks:

if wallet.totalBalanceSats == 0 && app.showHomeViewEmptyState

This is now consistent with the main content condition (line 25) using OR logic. The state transitions look correct:

  • Empty state shows when: balance == 0 AND flag == true
  • Wallet UI shows when: flag == false OR balance > 0

Architecture Compliance:
✅ Follows SwiftUI patterns from CLAUDE.md
✅ Uses @EnvironmentObject ViewModels (legacy pattern, acknowledged in docs)
✅ Proper use of .onChange modifier for reactive updates
✅ Animation consistency with spring curves


🎯 Final Verdict

Approval: ✅ LGTM with minor suggestions

This is a well-crafted fix that addresses the root cause properly. The changes are minimal, focused, and improve code quality. The only suggestions are:

  1. Remove duplicate .animation() modifier (line 88)
  2. Add explanatory comment for the OR condition (line 25)
  3. Ensure manual testing covers the edge cases mentioned above

Great work! 🚀

Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utAck

@ben-kaufman ben-kaufman merged commit 6092396 into master Nov 28, 2025
10 checks passed
@ben-kaufman ben-kaufman deleted the fix/wallet-load-first-receive branch November 28, 2025 21:42
BitcoinErrorLog pushed a commit to BitcoinErrorLog/bitkit-ios that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants