Skip to content

Conversation

@pwltr
Copy link
Contributor

@pwltr pwltr commented Nov 28, 2025

Description

  • add activity banner for transfer to spending/savings
  • remove transfer icon from WalletBalanceView (removed in design)

Notes

Just the initial UI component for the basic transfer flows to keep PR small. Other banner types still to be added. Suggestions and widgets were only hidden for development and screen capture.

Screenshot / Video

Simulator.Screen.Recording.-.iPhone.17.-.2025-11-28.at.12.51.30.mov

@claude
Copy link

claude bot commented Nov 28, 2025

PR Review: feat(activity): add transfer banner

Summary

This PR adds an animated activity banner component to display transfer status (to spending/savings) and removes the transfer icon from WalletBalanceView. Overall, this is a clean implementation with good visual design.


✅ Strengths

  1. Clean Component Design: The ActivityBanner component is well-structured and follows SwiftUI best practices
  2. Smooth Animations: Good use of .task modifier with withAnimation for the pulsing effect
  3. Proper State Management: Correctly uses @Published properties in WalletViewModel for reactive UI updates
  4. Code Cleanup: Removing the transfer icon from balance view simplifies the component
  5. Small, Focused PR: Good practice keeping the PR focused on a specific feature

🔍 Issues & Suggestions

1. Missing Localization ⚠️

Location: ActivityBanner.swift:29

Text(tTodo("TRANSFER IN PROGRESS"))

Issue: Using tTodo() for user-facing text that will be in production. This is a placeholder for untranslated strings.

Recommendation:

  • Add proper translation key to localization files before merging
  • Run node scripts/validate-translations.js to ensure all languages are updated
  • Consider a more descriptive key like "activity.transfer.in_progress" instead of all caps

2. Potential Logic Issue in Banner Type Selection ⚠️

Location: ActivityLatest.swift:13-18

private var bannerType: ActivityBannerType {
    if wallet.balanceInTransferToSpending > 0 {
        return .spending
    } else {
        return .savings
    }
}

Issue: If both balanceInTransferToSpending and balanceInTransferToSavings are > 0 simultaneously, only the spending banner shows. The logic assumes mutual exclusivity but doesn't enforce it.

Recommendation:

  • Document whether simultaneous transfers are possible
  • If they are possible, consider showing both banners or prioritizing more clearly:
private var bannerType: ActivityBannerType {
    // If both transfers are active, prioritize spending (or consider showing both)
    if wallet.balanceInTransferToSpending > 0 {
        return .spending
    }
    return .savings // Safe since shouldShowBanner already checks > 0
}

3. Animation Performance Consideration 💭

Location: ActivityBanner.swift:73-80

.task {
    withAnimation(.easeInOut(duration: 1.2).repeatForever(autoreverses: true)) {
        innerShadowOpacity = 0.64
        dropShadowOpacity = 0.0
        radialGradientOpacity = 0.0
        borderOpacity = 1.0
    }
}

Consideration: The .task modifier starts animations when the view appears, which is correct. However:

  • Animating 4 separate opacity values with shadows can be GPU-intensive
  • The repeatForever animation continues even when app is backgrounded

Recommendation: Consider adding cancellation or pausing animations when not visible (though SwiftUI may optimize this already).


4. Missing Accessibility 🌐

Location: ActivityBanner.swift

Issue: No accessibility labels or traits for VoiceOver users.

Recommendation:

.accessibilityElement(children: .combine)
.accessibilityLabel(Text(tTodo("TRANSFER IN PROGRESS")))
.accessibilityAddTraits(.updatesFrequently) // Due to animation

5. Magic Numbers in Styling 📐

Location: ActivityBanner.swift:42,49,62

Issue: Opacity values and radius values are hardcoded without explanation:

  • innerShadowOpacity: Double = 0.320.64
  • dropShadowOpacity: Double = 1.00.0
  • radialGradientOpacity: Double = 0.40.0
  • radius: 40, startRadius: 0, endRadius: 160

Recommendation: Consider adding comments or constants:

private enum AnimationConstants {
    static let innerShadowStart = 0.32
    static let innerShadowEnd = 0.64
    static let animationDuration = 1.2
    // ... etc
}

6. Architecture Compliance

Location: Overall structure

Observation: The implementation follows modern SwiftUI patterns correctly:

  • Uses @EnvironmentObject for dependency injection
  • Properly uses .animation() with value-based triggering
  • No ViewModels in the new component (good!)

However, the PR description mentions this is only for "basic transfer flows" and "other banner types still to be added."

Recommendation: Consider whether ActivityBannerType enum should be more extensible or if you'll need a different approach for other banner types.


🧪 Testing Recommendations

  1. Animation Testing: Verify the pulsing animation doesn't cause battery drain or performance issues
  2. State Transitions: Test rapid toggling of transfer states
  3. Simultaneous Transfers: Confirm behavior when both transfer balances > 0
  4. Dark Mode: Verify colors work well in both light and dark modes
  5. Accessibility: Test with VoiceOver enabled

🔒 Security & Performance

  • ✅ No security concerns
  • ✅ No direct Lightning/Bitcoin operations in UI (follows service layer pattern)
  • ⚠️ Animation performance should be monitored in production

📝 Code Quality Score: 7.5/10

Blockers:

  • Missing localization needs to be addressed

Nice-to-haves:

  • Accessibility improvements
  • Better handling of simultaneous transfers
  • Code comments for magic numbers

Conclusion

This is a solid implementation of the transfer banner UI. The main blocker is the missing localization. Once that's addressed and the minor suggestions are considered, this should be good to merge.

Great job keeping the PR focused and following the SwiftUI patterns outlined in CLAUDE.md! 🚀

@jvsena42 jvsena42 merged commit 35b854f into master Dec 1, 2025
9 checks passed
@jvsena42 jvsena42 deleted the feat/transfer-banner branch December 1, 2025 09:56
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.

3 participants