Conversation
WalkthroughThis pull request refactors multiple components. In the Changes
Sequence Diagram(s)sequenceDiagram
participant View as BitcoinView
participant VM as BitcoinViewModel
participant TaskBlock as Task
View->>VM: onReceive event triggers update()
VM->>TaskBlock: Initiate Task { fetch balances, prices, payments, status }
TaskBlock-->>VM: Complete asynchronous operations
VM-->>View: Update UI state
Possibly related PRs
Poem
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 (
|
|
code and ui looking good to me |
danielnordh
force-pushed
the
ux/walletscreen
branch
from
dbcf29a to
0b061a5
Compare
danielnordh
changed the title
![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
🧹 Nitpick comments (9)
LDKNodeMonday/View/Home/Payments/PaymentsListView.swift (1)
59-66: Remove commented-out code.Instead of keeping the commented-out header section, it should be removed entirely to maintain code cleanliness.
- /* - header: { - Text("Activity") - .font(.subheadline) - .fontWeight(.semibold) - } - */LDKNodeMonday/View/Settings/SettingsView.swift (1)
175-182: Consider adding visual feedback before dismissal.While dismissing after restart improves the flow, consider adding a brief success message or loading indicator to:
- Indicate restart progress
- Confirm successful completion
- Provide better user feedback
Task { await viewModel.walletClient.restart( newNetwork: viewModel.walletClient.network, newServer: viewModel.walletClient.server, appMode: viewModel.walletClient.appMode == .mock ? .live : .mock ) + // Show brief success message + try? await Task.sleep(nanoseconds: 1_000_000_000) // 1 second delay dismiss() }LDKNodeMonday/View/Home/BitcoinView.swift (3)
42-62: Consider more concise network status text.While the network status integration looks good, the text could be more concise.
Consider this more concise version:
- Text( - viewModel.walletClient.appMode == .mock - ? "Mock data /" : "" - ) - Text(viewModel.walletClient.network.description.capitalized) + if viewModel.walletClient.appMode == .mock { + Text("Mock") + } + Text(viewModel.walletClient.network.description.capitalized)
65-78: Optimize event handling code.The event handling code has some inefficiencies:
- Unused parameters in closure
- Redundant event handlers
Apply these improvements:
- .onChange( - of: eventService.lastMessage, - { oldValue, newValue in - showToast = eventService.lastMessage != nil - } - ) + .onChange( + of: eventService.lastMessage, + { _, _ in + showToast = eventService.lastMessage != nil + } + )Also, consider consolidating the
onReceivehandlers to reduce redundancy.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
231-293: Improve TransactionButtons component efficiency.The component is well-structured but has some areas for improvement:
- Empty button action in Send button
- Redundant NavigationLink for Scan QR
Consider these improvements:
- NavigationLink(value: NavigationDestination.address) { - Button { - // Optional button action if needed - } label: { - Text("Send") - }.buttonStyle( + NavigationLink(value: NavigationDestination.address) { + Text("Send") + .buttonStyle( BitcoinFilled( width: 120, tintColor: .accent, isCapsule: true ) - ).allowsHitTesting(false) // Required to enable NavigationLink to work + ) }.disabled(viewModel.unifiedBalance == 0)LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (4)
56-63: Consider adding task cancellation check.The
getStatusmethod should check for task cancellation to allow proper cleanup.func getStatus() async { + guard !Task.isCancelled else { return } let status = lightningClient.status() let sCopy = status // To avoid issues with non-sendable object await MainActor.run { self.status = sCopy self.isStatusFinished = true } }
65-74: Consider adding task cancellation check.The
getBalancesmethod should check for task cancellation to allow proper cleanup.func getBalances() async { + guard !Task.isCancelled else { return } let balances = await lightningClient.balances() let bdCopy = balances // To avoid issues with non-sendable object await MainActor.run { self.balances = bdCopy self.unifiedBalance = balances.totalOnchainBalanceSats + balances.totalLightningBalanceSats self.isBalancesFinished = true } }
76-97: Consider adding task cancellation check.The
getPricesmethod should check for task cancellation to allow proper cleanup.func getPrices() async { + guard !Task.isCancelled else { return } do { let price = try await priceClient.fetchPrice() await MainActor.run { self.price = price.usd self.time = price.time self.isPriceFinished = true } } catch let error as NodeError { let errorString = handleNodeError(error) await MainActor.run { self.bitcoinViewError = .init(title: errorString.title, detail: errorString.detail) } } catch { await MainActor.run { self.bitcoinViewError = .init( title: "Unexpected error", detail: error.localizedDescription ) } } }
99-105: Consider adding task cancellation check.The
getPaymentsmethod should check for task cancellation to allow proper cleanup.func getPayments() async { + guard !Task.isCancelled else { return } let payments = lightningClient.listPayments() let pCopy = payments await MainActor.run { self.payments = pCopy } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
LDKNodeMonday/View Model/Home/BitcoinViewModel.swift(1 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(3 hunks)LDKNodeMonday/View/Home/Payments/PaymentsListView.swift(2 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
LDKNodeMonday/View/Settings/SettingsView.swift (1)
Learnt from: danielnordh
PR: reez/Monday#158
File: LDKNodeMonday/View/Settings/SettingsView.swift:97-108
Timestamp: 2025-01-30T11:38:58.572Z
Learning: User prefers to handle UI/UX improvements like error handling and loading states in separate PRs to keep changes focused and manageable.
🪛 SwiftLint (0.57.0)
LDKNodeMonday/View/Home/BitcoinView.swift
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (5)
LDKNodeMonday/View/Home/Payments/PaymentsListView.swift (2)
41-46: LGTM! Empty state UI improvements look great.The updated empty state message is more engaging and action-oriented, with proper styling and spacing.
69-69: LGTM! Proper horizontal padding added.The addition of horizontal padding improves the visual appearance by maintaining proper content margins.
LDKNodeMonday/View/Settings/SettingsView.swift (1)
22-22: Verify navigation behavior with VStack.The replacement of NavigationView with VStack simplifies the view hierarchy but may affect standard iOS navigation patterns. Please ensure that:
- Navigation between views works as expected
- Back button behavior is preserved
- Navigation title remains visible
LDKNodeMonday/View/Home/BitcoinView.swift (2)
27-41: LGTM! Improved layout structure and accessibility.The switch to VStack with proper padding and the integration of PaymentsListView creates a more logical vertical flow. The dynamic type size limit helps maintain layout integrity.
334-345: LGTM! Well-structured preview code.The preview implementation follows best practices with appropriate use of mock data and debug conditional compilation.
| tintColor: .accent, | ||
| isCapsule: true | ||
| ) | ||
| ).allowsHitTesting(false) // Required to enable NavigationLink to work |
There was a problem hiding this comment.
interesting, never ran into this modifier but makes sense from taking a look
There was a problem hiding this comment.
I'm hoping to address this when we do the send screen(s).
We need to use NavigationLink instead of a plain Button to use the NavigationAddress.
But I think we can simplify inside the Send views so this is not necessary.
reez
left a comment
reez
left a comment
There was a problem hiding this comment.
ACK
Squash and merge whenever you're ready!
2 participants
This PR updates:
Summary by CodeRabbit
New Features
Refactor