UI: Tappable balance#163
Conversation
WalkthroughThis pull request updates the initialization of the Changes
Sequence Diagram(s)sequenceDiagram
participant BV as BitcoinView
participant BVM as BitcoinViewModel
participant LNS as LightningNodeService
BV->>BVM: getBalances()
BVM->>LNS: listBalances() async call
LNS-->>BVM: Return BalanceDetails
BVM->>BV: Update balances & unifiedBalance
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (1)🔇 Additional comments (8)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
LDKNodeMonday/App/WalletClient.swift (1)
53-53:⚠️ Potential issueCritical mismatch in
start()ignoringappMode.
Even ifappMode = .mock, line 53 still callsKeyClient.live.getBackupInfo(). Use the in-scopekeyClientinstead:- backupInfo = try? KeyClient.live.getBackupInfo() + backupInfo = try? keyClient.getBackupInfo()
🧹 Nitpick comments (7)
LDKNodeMonday/View/Home/BitcoinView.swift (3)
40-40: Consider debouncing multiple update calls.The view calls
viewModel.update()in multiple places:refreshable,onAppear,onReceive, and various sheet dismissals. While consolidating to a singleupdate()method is good, consider implementing debouncing to prevent rapid successive updates.// Add to BitcoinViewModel private var updateTask: Task<Void, Never>? func debouncedUpdate() { updateTask?.cancel() updateTask = Task { try? await Task.sleep(nanoseconds: 500_000_000) // 500ms if !Task.isCancelled { await update() } } }Also applies to: 107-107, 123-123, 130-130, 177-177, 191-191, 218-218
230-308: Add documentation for the BalanceHeader component.The component is well-structured, but would benefit from documentation explaining:
- The purpose of each computed property
- The display type cycle flow
- The animation behavior
Add documentation like this:
+ /// Displays the wallet balance with configurable display types. + /// Supports cycling through different balance representations on tap. struct BalanceHeader: View { @Binding var displayBalanceType: DisplayBalanceType @ObservedObject var viewModel: BitcoinViewModel + /// The primary balance value based on the current display type. + /// - Returns: Formatted string representation of the balance. var balanceValue: String { // ... }
315-347: Add error handling for UserDefaults persistence.The UserDefaults implementation could be more robust:
- Consider using a dedicated UserDefaults key enum
- Add error handling for invalid raw values
- Document the display type cycle order
private enum UserDefaultsKey { static let displayBalanceType = "displayBalanceType" } extension DisplayBalanceType { /// Cycles through display types in the following order: /// fiatSats -> fiatBtc -> btcFiat -> totalSats -> onchainSats -> lightningSats -> fiatSats mutating func next() { // ... } static let userDefaults: DisplayBalanceType = { guard let savedValue = UserDefaults.standard.string(forKey: UserDefaultsKey.displayBalanceType), let savedType = DisplayBalanceType(rawValue: savedValue) else { return .fiatBtc } return savedType }() }LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (2)
42-47: Consider parallelizing the async calls.
All three async functions are invoked in sequence. You can potentially speed this up by usingasync letor aTaskGroupto run them concurrently, if the logic permits handling partial failures independently.
79-87: Consistent error propagation.
This mirrors the approach used throughout the file for settingbitcoinViewErroron the main actor. Consider applying a similar do/catch ingetBalanceDetails()for consistency.LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (2)
176-179: Consider adding error handling to balanceDetails().While consistent with other methods in the class, consider adding error handling to make the method more robust. This is especially important as it's a new addition that could set a better pattern.
-func balanceDetails() async -> BalanceDetails { - let balanceDetails = ldkNode.listBalances() - return balanceDetails +func balanceDetails() async throws -> BalanceDetails { + do { + let balanceDetails = ldkNode.listBalances() + return balanceDetails + } catch { + throw error + } +}
539-556: Consider expanding mock BalanceDetails scenarios.While the current mock implementation is good, consider adding more scenarios to test edge cases:
- High balance values to test formatting
- Non-empty lightningBalances array
- Non-empty pendingBalancesFromChannelClosures
static let mock = BalanceDetails( totalOnchainBalanceSats: 150000, spendableOnchainBalanceSats: 100000, totalAnchorChannelsReserveSats: 0, totalLightningBalanceSats: 50000, - lightningBalances: [], - pendingBalancesFromChannelClosures: [] + lightningBalances: [ + LightningBalance(channelId: "ch1", balanceMsat: 25000000), + LightningBalance(channelId: "ch2", balanceMsat: 25000000) + ], + pendingBalancesFromChannelClosures: [ + PendingSweepBalance(txid: "tx1", confirmationHeight: 100, amountSats: 1000) + ] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
LDKNodeMonday/App/LDKNodeMondayApp.swift(1 hunks)LDKNodeMonday/App/WalletClient.swift(2 hunks)LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift(4 hunks)LDKNodeMonday/View Model/Home/BitcoinViewModel.swift(2 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(7 hunks)LDKNodeMonday/View/Home/NetworkSettingsView.swift(1 hunks)LDKNodeMonday/View/Home/OnboardingView.swift(1 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- LDKNodeMonday/View/Home/NetworkSettingsView.swift
- LDKNodeMonday/View/Settings/SettingsView.swift
🧰 Additional context used
🧠 Learnings (1)
LDKNodeMonday/App/LDKNodeMondayApp.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/App/LDKNodeMondayApp.swift:47-52
Timestamp: 2025-01-29T19:03:13.205Z
Learning: Error handling for wallet operations (start, create, restart, delete) is managed within the WalletClient class, which updates both appError and appState properties when errors occur. Additional error handling in LDKNodeMondayApp is not needed.
🔇 Additional comments (11)
LDKNodeMonday/View/Home/BitcoinView.swift (2)
21-21: LGTM! Accessibility and state management improvements.Good improvements:
- Added state management for balance display types
- Set maximum dynamic type size for better accessibility
Also applies to: 104-104
205-205: LGTM! Consistent use of balance details.Good improvements:
- Consistent usage of
balanceDetailsfor spendable balance- Clean preview implementation with mock mode
Also applies to: 213-213, 353-353
LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (4)
17-19: Good consolidation of balance properties.
Storing all related data inbalanceDetailsand having a conciseunifiedBalanceflag is a clean approach.
28-28: No immediate issues with the USD conversion.
Casting toDoubleand calling.valueInUSD(price:)is straightforward.
51-53: Handling non-sendable objects is correct.
CopyingstatusintosCopybefore updating on the main actor helps avoid concurrency issues.
72-77: UI state updates on the main actor look correct.
Ensuring thatself.priceandself.isPriceFinishedare assigned on the main thread is appropriate for SwiftUI.LDKNodeMonday/App/LDKNodeMondayApp.swift (1)
15-15: Switching toappModealigns with the new WalletClient design.
This is consistent with the retrieved learning that additional error handling here is not needed.LDKNodeMonday/App/WalletClient.swift (2)
22-31: Initialization withappModeis clear and flexible.
Distinguishing between.liveand.mockhelps maintain a cleaner design.
123-126: NewAppModeenum neatly centralizes environment selection.
Explicitly specifying.liveor.mockmakes the code more testable.LDKNodeMonday/View/Home/OnboardingView.swift (1)
141-141: LGTM: AppMode initialization aligns with the new configuration pattern.The change from
KeyClient.mocktoAppMode.mockis consistent with the broader refactoring effort to standardize client configuration.LDKNodeMonday/Service/Lightning Service/LightningNodeService.swift (1)
446-502: LGTM: Well-structured mock implementation.The mock implementation provides realistic test data and maintains consistency with the live implementation. The mock values are well-chosen for testing different scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
LDKNodeMonday/View/Home/BitcoinView.swift (1)
230-309: Consider adding documentation for the computed properties.The
BalanceHeadercomponent is well-structured, but adding documentation for the computed properties would improve maintainability.Add documentation comments for the computed properties:
+ /// Returns the primary balance value based on the current display type var balanceValue: String { // ... existing code ... } + /// Returns the secondary value to be displayed below the primary balance var secondaryValue: String { // ... existing code ... } + /// Returns the unit label based on the current display type var unitValue: String { // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
LDKNodeMonday/View/Home/BitcoinView.swift(7 hunks)
🔇 Additional comments (4)
LDKNodeMonday/View/Home/BitcoinView.swift (4)
21-21: LGTM! Good accessibility considerations.The addition of
displayBalanceTypestate and dynamic type size limit improves both functionality and accessibility.Also applies to: 104-104
40-40: Improved efficiency by consolidating balance updates.The consolidation of multiple balance retrieval calls into a single
update()method reduces redundant API calls and improves performance.Also applies to: 107-108, 123-124, 130-131, 177-178, 191-192, 218-219
316-348: LGTM! Well-structured enum with robust default handling.The
DisplayBalanceTypeimplementation provides a clean way to manage different balance display states with proper persistence.
354-354: LGTM! Preview updated to use new AppMode.The preview configuration correctly reflects the new WalletClient initialization pattern.
|
ACK just needs that one function renamed to |
2 participants
This PR updates the way balances are shown.
A tappable view switches between the following states with primary and secondary values:
Simulator.Screen.Recording.-.iPhone.16.-.2025-02-11.at.13.13.31.mp4
Other changes:
Summary by CodeRabbit
New Features
Refactor