ui: receive button context menu #156
Conversation
WalkthroughThis pull request involves significant restructuring of the payment receiving functionality in the LDKNodeMonday project. The changes include removing the Changes
Sequence DiagramsequenceDiagram
participant User
participant BitcoinView
participant BIP21View
participant AmountEntryView
User->>BitcoinView: Open Bitcoin View
User->>BitcoinView: Select BIP21 Option
BitcoinView->>BIP21View: Present BIP21 Sheet
BIP21View->>AmountEntryView: Open Amount Entry
User->>AmountEntryView: Enter Amount
AmountEntryView-->>BIP21View: Confirm Amount
BIP21View-->>BitcoinView: Dismiss Sheet
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 (
|
![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 (2)
LDKNodeMonday/View/Home/Receive/BIP21View.swift (2)
Line range hint
25-41: Enhance error handling in QR generation.The
generateUnifiedQR()function should handle errors more explicitly to prevent the loading state from getting stuck if an error occurs.Consider this implementation:
private func generateUnifiedQR() async { isLoadingQR = true + defer { isLoadingQR = false } let amountSat = (UInt64(viewModel.amountSat) ?? 0) - await viewModel.receivePayment( - amountSat: amountSat, - message: "Monday Wallet", - expirySecs: UInt32(3600) - ) - isLoadingQR = false + do { + try await viewModel.receivePayment( + amountSat: amountSat, + message: "Monday Wallet", + expirySecs: UInt32(3600) + ) + } catch { + viewModel.receiveViewError = ReceiveViewError( + title: "QR Generation Failed", + detail: error.localizedDescription + ) + } }Also applies to: 156-165
Line range hint
177-223: Add validation and documentation for QR parsing.The
parseUnifiedQRfunction lacks input validation and documentation about the expected format.Consider adding:
- Documentation explaining the expected format
- URL encoding/decoding handling
- Component format validation
+/// Parses a Unified QR code string in the format: +/// BITCOIN:<address>?lightning=<bolt11>&lno=<bolt12> +/// +/// - Parameter unifiedQR: The unified QR code string to parse +/// - Returns: A UnifiedQRComponents object containing the parsed components, or nil if parsing fails func parseUnifiedQR(_ unifiedQR: String) -> UnifiedQRComponents? { + guard !unifiedQR.isEmpty else { return nil } + // Split the string by '?' let components = unifiedQR.components(separatedBy: "?") guard components.count > 1 else { return nil } // Extract onchain (everything before the first '?') and remove the "BITCOIN:" prefix var onchain = components[0] if onchain.lowercased().hasPrefix("bitcoin:") { onchain = String(onchain.dropFirst(8)) // Remove "BITCOIN:" } + guard onchain.count > 0 else { return nil } // Join the rest of the components back together let remainingString = components.dropFirst().joined(separator: "?") // Split the remaining string by '&' let params = remainingString.components(separatedBy: "&") var bolt11: String? var bolt12: String? for param in params { + let decodedParam = param.removingPercentEncoding ?? param if param.starts(with: "lightning=") { - bolt11 = String(param.dropFirst("lightning=".count)) + bolt11 = String(decodedParam.dropFirst("lightning=".count)) } else if param.starts(with: "lno=") { - bolt12 = String(param.dropFirst("lno=".count)) + bolt12 = String(decodedParam.dropFirst("lno=".count)) } } - guard let bolt11 = bolt11, let bolt12 = bolt12 else { return nil } + guard let bolt11 = bolt11, bolt11.count > 0, + let bolt12 = bolt12, bolt12.count > 0 else { return nil }
🧹 Nitpick comments (6)
LDKNodeMonday/View/Home/BitcoinView.swift (3)
14-21: Consider consolidating sheet dismissal refresh logic.Multiple sheets in this view perform identical refresh operations on dismissal. Consider extracting this common logic into a reusable function to improve maintainability and reduce code duplication.
+ private func refreshBalancesAndStatus() async { + await viewModel.getTotalOnchainBalanceSats() + await viewModel.getTotalLightningBalanceSats() + await viewModel.getPrices() + await viewModel.getSpendableOnchainBalanceSats() + await viewModel.getStatus() + }
166-178: Add accessibility support to the receive menu.The menu and its buttons lack accessibility labels, which could impact users relying on VoiceOver. Consider adding appropriate accessibility modifiers.
Menu { Button { isBIP21SheetPresented = true } label: { Label("BIP21", systemImage: "bitcoinsign") - } + } + .accessibilityLabel("Generate BIP21 Bitcoin Address") Button { isJITSheetPresented = true } label: { Label("JIT", systemImage: "bolt.fill") - } + } + .accessibilityLabel("Generate Just-In-Time Lightning Invoice") } label: { Image(systemName: "qrcode") .font(.title) .foregroundColor(.primary) -} +} +.accessibilityLabel("Receive Bitcoin Menu")
183-212: Optimize sheet presentations and reduce code duplication.
- The dismissal handlers contain identical refresh logic that could be consolidated using the suggested
refreshBalancesAndStatus()function.- Consider supporting multiple presentation detents for better adaptability across different screen sizes.
.sheet( isPresented: $isBIP21SheetPresented, onDismiss: { Task { - await viewModel.getTotalOnchainBalanceSats() - await viewModel.getTotalLightningBalanceSats() - await viewModel.getPrices() - await viewModel.getSpendableOnchainBalanceSats() - await viewModel.getStatus() + await refreshBalancesAndStatus() } } ) { BIP21View(viewModel: .init(lightningClient: viewModel.lightningClient)) - .presentationDetents([.large]) + .presentationDetents([.medium, .large]) }Apply similar changes to the JIT sheet presentation.
LDKNodeMonday/View/Home/Receive/BIP21View.swift (3)
Line range hint
13-21: Consider consolidating copy status state management.The current implementation uses separate state variables for each invoice type's copy status. This could be simplified using an enum-based approach or a single dictionary to track copy states.
Example implementation:
- @State private var onchainIsCopied = false - @State private var onchainShowCheckmark = false - @State private var bolt11IsCopied = false - @State private var bolt11ShowCheckmark = false - @State private var bolt12IsCopied = false - @State private var bolt12ShowCheckmark = false - @State private var unifiedIsCopied = false - @State private var unifiedShowCheckmark = false + enum InvoiceType: String { + case onchain, bolt11, bolt12, unified + } + @State private var copiedStates: [InvoiceType: Bool] = [:] + @State private var checkmarkStates: [InvoiceType: Bool] = [:]
286-324: Improve InvoiceRowView accessibility and user experience.The InvoiceRowView needs accessibility improvements and better handling of long text values.
Consider these enhancements:
struct InvoiceRowView: View { let title: String let value: String let isCopied: Bool let showCheckmark: Bool let networkColor: Color let onCopy: () -> Void + private let copyFeedbackDuration: TimeInterval = 2.0 var body: some View { HStack(alignment: .center) { VStack(alignment: .leading, spacing: 5.0) { Text(title) .bold() Text(value) .truncationMode(.middle) .lineLimit(1) .foregroundColor(.secondary) .redacted(reason: value.isEmpty ? .placeholder : []) + .onTapGesture { + // Show full text in a modal/sheet + } } .font(.caption2) + .accessibilityElement(children: .combine) + .accessibilityLabel("\(title): \(value)") Spacer() Button(action: onCopy) { HStack { withAnimation { Image(systemName: showCheckmark ? "checkmark" : "doc.on.doc") .font(.title3) .minimumScaleFactor(0.5) } } .bold() .foregroundColor(networkColor) } + .accessibilityLabel(showCheckmark ? "Copied" : "Copy \(title)") .font(.caption2) } .padding(.horizontal) } } + +#if DEBUG +struct InvoiceRowView_Previews: PreviewProvider { + static var previews: some View { + InvoiceRowView( + title: "Test Invoice", + value: "bc1qxy2kgdygjrsqtzq2n0yrf2493p83kkfjhx0wlh", + isCopied: false, + showCheckmark: false, + networkColor: .blue + ) {} + } +} +#endif
Line range hint
326-330: Enhance preview implementations.The current preview implementation could be more comprehensive.
Consider adding:
#if DEBUG - #Preview { - BIP21View(viewModel: .init(lightningClient: .mock)) + struct ContentView_Previews: PreviewProvider { + static var previews: some View { + Group { + BIP21View(viewModel: .init(lightningClient: .mock)) + .previewDisplayName("Light Mode") + + BIP21View(viewModel: .init(lightningClient: .mock)) + .preferredColorScheme(.dark) + .previewDisplayName("Dark Mode") + + BIP21View(viewModel: .init(lightningClient: .mock)) + .previewDevice("iPhone SE (3rd generation)") + .previewDisplayName("iPhone SE") + } + } } #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
LDKNodeMonday.xcodeproj/project.pbxproj(0 hunks)LDKNodeMonday/Model/ReceiveOption.swift(0 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(2 hunks)LDKNodeMonday/View/Home/Receive/BIP21View.swift(1 hunks)LDKNodeMonday/View/Home/Receive/ReceiveView.swift(0 hunks)
💤 Files with no reviewable changes (3)
- LDKNodeMonday/Model/ReceiveOption.swift
- LDKNodeMonday/View/Home/Receive/ReceiveView.swift
- LDKNodeMonday.xcodeproj/project.pbxproj
|
A couple of thoughts that could simplify this PR and improve the experience. 1. Context menu should be optional 2. Use ReceiveView as entrypoint and drive the view with existing enum @State public var selectedOption: ReceiveOption = .bip21And then setting the desired option when presenting from BitcoinView: ReceiveView(lightningClient: viewModel.lightningClient, selectedOption: .bolt11JIT)3. Simplify the sheet presentation @State private var isReceiveSheetPresented = falsewith: @State private var showReceiveViewWithOption: ReceiveOption?we don't need two separate bools and can show the sheet with: showReceiveViewWithOption = .bip21 // or .bolt11JIT
//...
.sheet(
item: $showReceiveViewWithOption,
onDismiss: {
...
}
) { receiveOption in
ReceiveView(lightningClient: viewModel.lightningClient, selectedOption: receiveOption)
}I might have missed some other requirements that make bigger changes necessary, in case which you can ignore these suggestions. EDIT: Since I had tested the above things just to make sure they worked, I realised I might as add the context menu. If you are interested in how this alternative implementation looks/works I pushed a draft PR #159 |
Agree. Let's use #159 since you've already got it ready! Closing this |
Description
Receive button uses context menu
ReceiveViewandReceiveOptionI understand Receive is a bit of a WIP, and ideal end state for a regular user would be to just have one Receive that handles everything (never has to see/understand JIT vs BIP21 etc)... but for the moment this actually is nice for me personally improving my user experience.
@danielnordh I'm if we dont merge, but it's ready to be merged.
Notes to the reviewers
Before
After
Changelog notice
Checklists
All Submissions:
.swift-formatfileNew Features:
Bugfixes:
Summary by CodeRabbit
Removed Features
ReceiveViewandReceiveOptioncomponents from the applicationNew Features
BIP21Viewwith enhanced invoice and amount entry capabilitiesChanges
BitcoinViewwith new sheet presentation logic for Bitcoin transactions