Conversation
WalkthroughThis pull request introduces significant changes to the application's state management system, replacing the previous boolean-based onboarding approach with a more versatile Changes
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: 3
🧹 Nitpick comments (5)
LDKNodeMonday/View/Home/ErrorView.swift (1)
14-18: Enhance error presentation for better UX.The current error presentation is very basic. Consider adding:
- Error icon or symbol
- Different styling for different error types
- Action button for retry/refresh
- Error code display if available
Here's a suggested implementation:
var body: some View { Spacer() - Text(error != nil ? error!.localizedDescription : "Unknown error") + VStack(spacing: 16) { + Image(systemName: "exclamationmark.triangle.fill") + .foregroundColor(.red) + .font(.system(size: 50)) + + Text(error?.localizedDescription ?? "Unknown error") + .multilineTextAlignment(.center) + .foregroundColor(.secondary) + + if error != nil { + Button(action: { + // TODO: Add retry action + }) { + Text("Retry") + .fontWeight(.semibold) + } + .buttonStyle(.bordered) + } + } + .padding() Spacer() }LDKNodeMonday.xcodeproj/project.pbxproj (1)
Line range hint
628-685: Clean up duplicate LDKNode package dependencies.There are multiple entries for the LDKNode package in the build phases. This could lead to confusion or build issues.
Consider consolidating the LDKNode package references to use a single source:
- Keep the one from the
ldk-noderepository- Remove the duplicate product dependencies
LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
36-36: Replace unused closure parameters with underscoresThe closure parameters
oldValueandnewValuein the.onChangemodifier are not used inside the closure. To eliminate warnings from static analysis tools and adhere to Swift best practices, replace them with underscores_.Apply this diff to replace the unused parameters:
- .onChange(of: appState) { oldValue, newValue in + .onChange(of: appState) { _, _ in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
60-60: Implement parameter passing tostart()functionThe TODO comment suggests modifying
.start()to take parameters frombackupInfo(e.g., seed, network, URL, LSP). Passing these parameters explicitly would enhance flexibility and make the initialization ofLightningNodeServicemore robust.Would you like assistance in updating the
start()function to accept parameters frombackupInfoand modifying the call accordingly?🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 60-60: TODOs should be resolved (.start could take parameters f...)
(todo)
LDKNodeMonday/View Model/Home/OnboardingViewModel.swift (1)
12-12: Consider using@PublishedforappStateinstead of@BindinginOnboardingViewModelUsing
@Bindingin aViewModelcan lead to tight coupling between theViewand theViewModel. It's generally recommended to manage the state internally within theViewModelusing@Publishedproperties. TheViewcan then observe these properties using@ObservedObject, promoting better separation of concerns and testability.Proposed refactor:
In
OnboardingViewModel.swift, changeappStateto an@Publishedproperty:class OnboardingViewModel: ObservableObject { @Published var appState: AppState // ... init() { self.appState = .onboarding // Set the initial state // Existing initialization code... } // ... }In
OnboardingView.swift, observe theappStatefrom theViewModel:struct OnboardingView: View { @ObservedObject var viewModel: OnboardingViewModel // ... var body: some View { // Use `viewModel.appState` as needed } }Also applies to: 80-81
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
LDKNodeMonday.xcodeproj/project.pbxproj(5 hunks)LDKNodeMonday/App/LDKNodeMondayApp.swift(2 hunks)LDKNodeMonday/View Model/Home/OnboardingViewModel.swift(3 hunks)LDKNodeMonday/View Model/Home/StartViewModel.swift(0 hunks)LDKNodeMonday/View/Home/ErrorView.swift(1 hunks)LDKNodeMonday/View/Home/LoadingView.swift(1 hunks)LDKNodeMonday/View/Home/OnboardingView.swift(1 hunks)LDKNodeMonday/View/Home/StartView.swift(0 hunks)
💤 Files with no reviewable changes (2)
- LDKNodeMonday/View/Home/StartView.swift
- LDKNodeMonday/View Model/Home/StartViewModel.swift
✅ Files skipped from review due to trivial changes (1)
- LDKNodeMonday/View/Home/LoadingView.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
LDKNodeMonday/App/LDKNodeMondayApp.swift
[Warning] 51-51: TODOs should be resolved (Show error on relevant screen,...)
(todo)
[Warning] 60-60: TODOs should be resolved (.start could take parameters f...)
(todo)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (4)
LDKNodeMonday/View/Home/ErrorView.swift (1)
21-29: LGTM! Preview provides good test data.The preview implementation uses appropriate test data with a realistic error message.
LDKNodeMonday.xcodeproj/project.pbxproj (1)
10-11: LGTM! File references are properly configured.The new files
LoadingView.swiftandErrorView.swiftare correctly added to the project structure and build phases.Also applies to: 96-97, 307-308, 628-628, 685-685
LDKNodeMonday/View Model/Home/OnboardingViewModel.swift (1)
112-112: State transition to.walletis correctly implementedThe assignment of
self.appState = .walletupon successful seed saving correctly transitions the application state, aligning with the new state management approach.LDKNodeMonday/View/Home/OnboardingView.swift (1)
105-105: Initialization ofOnboardingViewModelin Preview is appropriateThe update to initialize the
OnboardingViewModelwithappState: .constant(.onboarding)in the preview ensures that the onboarding state is correctly represented during development.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I had a node running on my device, then went to Settings -> Full Reset, and it didn't seem to work going back to onboarding (it kind of stayed on settings, yet stopped the node running yet didn't go back to Onboarding but just stayed in the settings and still would show the main wallet view). and then when I deleted or killed the app and then ran it I would get this screen:
Did you see anything similar when doing Delete Wallet or Full Reset or anything? Could totally be something on my end. I think something in This PR is looking good overall otherwise. |
Good point, forgot that those had the isOnboarding code. Will fix. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
LDKNodeMonday/View/Settings/SettingsView.swift (1)
Line range hint
1-176: Consider adding state restoration logic.Based on the PR comments about the app not returning to onboarding after a reset, it seems the SettingsView's delete functionality might need to properly handle state restoration. Consider updating the delete action to explicitly set the appState to .onboarding:
Button("Yes", role: .destructive) { viewModel.delete() + viewModel.appState = .onboarding dismiss() }
🧹 Nitpick comments (6)
LDKNodeMonday/View/Home/BitcoinView.swift (1)
349-352: Enhance preview coverage with multiple AppState cases.The preview only shows the view in
.onboardingstate. Consider adding multiple previews to demonstrate how the view behaves in different states (e.g.,.wallet,.loading,.error).Example implementation:
#if DEBUG struct BitcoinView_Previews: PreviewProvider { static var previews: some View { Group { BitcoinView( viewModel: .init(appState: .constant(.onboarding), priceClient: .mock), sendNavigationPath: .constant(.init()) ) .previewDisplayName("Onboarding State") BitcoinView( viewModel: .init(appState: .constant(.wallet), priceClient: .mock), sendNavigationPath: .constant(.init()) ) .previewDisplayName("Wallet State") BitcoinView( viewModel: .init(appState: .constant(.loading), priceClient: .mock), sendNavigationPath: .constant(.init()) ) .previewDisplayName("Loading State") BitcoinView( viewModel: .init(appState: .constant(.error), priceClient: .mock), sendNavigationPath: .constant(.init()) ) .previewDisplayName("Error State") } } } #endifLDKNodeMonday/View Model/Profile/SettingsViewModel.swift (2)
13-13: Consider avoiding@Bindingin ViewModel; use@Publishedand delegate pattern insteadUsing
@Bindingin aViewModelis unconventional since bindings are intended for use withinViews to create a two-way connection between a property and aView. In aViewModel, it's more appropriate to use@Publishedproperties and communicate changes back to theViewusing delegate methods or closure callbacks. This approach adheres to the MVVM pattern and enhances the separation of concerns.
64-72: Reduce duplicated error handling code with a helper methodThe error handling logic in your
delete()method'scatchblocks is repetitive. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this logic into a helper method.Here's an example of how you could refactor the error handling:
private func handleNodeIDError(_ error: Error) { let errorString: (title: String, detail: String) if let nodeError = error as? NodeError { errorString = handleNodeError(nodeError) } else { errorString = (title: "Error", detail: error.localizedDescription) } DispatchQueue.main.async { self.nodeIDError = .init(title: errorString.title, detail: errorString.detail) } }Then, in your
delete()method, you can replace the catch blocks with:catch { handleNodeIDError(error) }LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (1)
12-12: Consider documenting the purpose of appState binding.The
appStatebinding is added but not used in any of the view model's methods. If it's intended for future use, consider adding a documentation comment explaining its purpose.LDKNodeMonday/App/LDKNodeMondayApp.swift (2)
36-38: Replace unused closure parameters with underscores.The
onChangemodifier has unused parameters that can be replaced with underscores.-.onChange(of: appState) { oldValue, newValue in +.onChange(of: appState) { _, _ in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
58-76: Consider handling node startup errors differently.When
LightningNodeService.start()fails, it might be better to allow retrying rather than showing a generic error view. Consider adding a.retryNodeStartstate or implementing a retry mechanism.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 60-60: TODOs should be resolved (.start could take parameters f...)
(todo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
LDKNodeMonday/App/LDKNodeMondayApp.swift(2 hunks)LDKNodeMonday/View Model/Home/BitcoinViewModel.swift(2 hunks)LDKNodeMonday/View Model/Profile/SettingsViewModel.swift(3 hunks)LDKNodeMonday/View/Home/BitcoinView.swift(2 hunks)LDKNodeMonday/View/Settings/SettingsView.swift(2 hunks)
🧰 Additional context used
📓 Learnings (1)
LDKNodeMonday/App/LDKNodeMondayApp.swift (1)
Learnt from: danielnordh
PR: reez/Monday#150
File: LDKNodeMonday/App/LDKNodeMondayApp.swift:49-56
Timestamp: 2025-01-16T19:56:11.062Z
Learning: In the LDKNodeMonday app, KeyClient.live.getBackupInfo() returns nil when no backup info is found, rather than throwing an error. The nil case is used to determine if the app should proceed to the onboarding state.
🪛 SwiftLint (0.57.0)
LDKNodeMonday/App/LDKNodeMondayApp.swift
[Warning] 51-51: TODOs should be resolved (Show error on relevant screen,...)
(todo)
[Warning] 60-60: TODOs should be resolved (.start could take parameters f...)
(todo)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (8)
LDKNodeMonday/View/Home/BitcoinView.swift (1)
242-242: Verify BitcoinViewModel's appState property initialization.The
SettingsViewinitialization now requires anappStatebinding, but theBitcoinViewModel'sappStateproperty is not visible in the provided code. This could lead to runtime issues.Let's verify the BitcoinViewModel implementation:
✅ Verification successful
The BitcoinViewModel implementation is correct and safe.
The
appStateproperty is properly implemented in BitcoinViewModel as a@Bindingproperty and correctly initialized, matching the usage pattern in BitcoinView. The code is safe from runtime issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BitcoinViewModel's appState property definition ast-grep --pattern 'class BitcoinViewModel { $$$ @Published var appState: AppState $$$ }' # Search for BitcoinViewModel's init method to verify appState initialization ast-grep --pattern 'init(appState: Binding<AppState>, $$$) { $$$ self.appState = appState $$$ }'Length of output: 202
Script:
#!/bin/bash # Find BitcoinViewModel file fd BitcoinViewModel # Search for appState property and initialization using ripgrep rg "class BitcoinViewModel" -A 10 rg "@Published.*appState" -A 2 rg "init.*appState.*\{" -A 5Length of output: 2861
LDKNodeMonday/View/Settings/SettingsView.swift (2)
128-128: LGTM: Improved alert message formatting.The addition of a newline character improves the readability of the alert message.
175-175: Verify AppState handling in SettingsViewModel.While the preview has been updated to use the new AppState parameter, based on the PR comments about onboarding issues, the actual SettingsViewModel might still contain legacy
.isOnboardingcode that needs to be updated.Let's verify the SettingsViewModel implementation:
✅ Verification successful
SettingsViewModel implementation is correctly using AppState
The ViewModel has been properly updated to use the new AppState pattern and contains no legacy
.isOnboardingcode. The usage of.onboardingstate in thedelete()method is intentional and correct, matching the preview implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining isOnboarding references in SettingsViewModel rg "isOnboarding" --type swift # Search for AppState usage in SettingsViewModel ast-grep --pattern 'class SettingsViewModel { $$$ var appState: $_ { $$$ } $$$ }'Length of output: 126
Script:
#!/bin/bash # First locate the ViewModel file fd -e swift "SettingsViewModel" # Then examine its contents echo "=== File contents ===" for f in $(fd -e swift "SettingsViewModel"); do echo "File: $f" echo "---" cat "$f" echo "---" doneLength of output: 661
Script:
#!/bin/bash # Read the file contents with proper path handling echo "=== SettingsViewModel Contents ===" cat "LDKNodeMonday/View Model/Profile/SettingsViewModel.swift" # Search for specific patterns in this file echo -e "\n=== AppState Usage ===" rg "AppState" "LDKNodeMonday/View Model/Profile/SettingsViewModel.swift" || true echo -e "\n=== isOnboarding References ===" rg "isOnboarding" "LDKNodeMonday/View Model/Profile/SettingsViewModel.swift" || trueLength of output: 4274
LDKNodeMonday/View Model/Profile/SettingsViewModel.swift (1)
22-23: Ensure proper initialization of@BindingpropertyWhen initializing an
@Bindingproperty in a class, assigning toself._appState = appStateis correct. However, as mentioned earlier, consider whether using@Bindingin theViewModelis the optimal approach for state management.LDKNodeMonday/View Model/Home/BitcoinViewModel.swift (1)
37-39: LGTM! Initializer correctly handles the new appState binding.The initializer properly initializes the appState binding using the underscore prefix convention.
LDKNodeMonday/App/LDKNodeMondayApp.swift (3)
15-16: LGTM! Clear state initialization.Good choice initializing
appStateto.loadingby default.
22-33: LGTM! Clean state-based view switching.The switch statement provides a clear separation of views based on application state.
87-92: LGTM! Clear and concise AppState enum.The enum cases clearly represent all possible application states.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
LDKNodeMonday/App/LDKNodeMondayApp.swift (3)
22-33: Consider handling nil error case in ErrorView.The error view is initialized with a potentially nil error. Consider providing a default error message when
self.appErroris nil.- ErrorView(error: self.appError) + ErrorView(error: self.appError ?? NSError(domain: "LDKNodeMonday", code: -1, userInfo: [NSLocalizedDescriptionKey: "An unknown error occurred"]))
36-41: Replace unused closure parameters with underscores.The closure parameters are unused and can be replaced with underscores as per Swift best practices.
-.onChange(of: appState) { oldValue, newValue in +.onChange(of: appState) { _, _ in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
51-64: Consider specific error handling for node startup.The error handling is generic and could benefit from more specific error cases to provide better user feedback.
do { try await LightningNodeService.shared.start() LightningNodeService.shared.listenForEvents() await MainActor.run { self.appState = .wallet } -} catch let error { +} catch let error as LightningNodeError { debugPrint(error) await MainActor.run { self.appError = error self.appState = .error } +} catch { + debugPrint("Unexpected error during node startup: \(error)") + await MainActor.run { + self.appError = NSError( + domain: "LDKNodeMonday", + code: -1, + userInfo: [NSLocalizedDescriptionKey: "Failed to start Lightning node: \(error.localizedDescription)"] + ) + self.appState = .error + } }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 52-52: TODOs should be resolved (.start could take parameters f...)
(todo)
LDKNodeMonday/View Model/Profile/SettingsViewModel.swift (1)
52-72: LGTM! Well-structured delete implementation with proper error handling.The changes demonstrate several improvements:
- Safe node stopping with status check
- Logical operation ordering
- Proper use of injected
keyClient- Clean state transition to onboarding
- Comprehensive error handling
Consider extracting the error handling logic into a reusable method since similar patterns appear in other methods (
getNetwork,getEsploraUrl):private func handleError(_ error: Error, completion: @escaping (MondayError) -> Void) { if let nodeError = error as? NodeError { let errorString = handleNodeError(nodeError) DispatchQueue.main.async { completion(.init(title: errorString.title, detail: errorString.detail)) } } else { DispatchQueue.main.async { completion(.init(title: "Error", detail: error.localizedDescription)) } } }Usage:
} catch let error { handleError(error) { error in self.nodeIDError = error } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
LDKNodeMonday/App/LDKNodeMondayApp.swift(2 hunks)LDKNodeMonday/View Model/Profile/SettingsViewModel.swift(3 hunks)
🧰 Additional context used
📓 Learnings (1)
LDKNodeMonday/App/LDKNodeMondayApp.swift (1)
Learnt from: danielnordh
PR: reez/Monday#150
File: LDKNodeMonday/App/LDKNodeMondayApp.swift:49-56
Timestamp: 2025-01-16T19:56:11.062Z
Learning: In the LDKNodeMonday app, KeyClient.live.getBackupInfo() returns nil when no backup info is found, rather than throwing an error. The nil case is used to determine if the app should proceed to the onboarding state.
🪛 SwiftLint (0.57.0)
LDKNodeMonday/App/LDKNodeMondayApp.swift
[Warning] 52-52: TODOs should be resolved (.start could take parameters f...)
(todo)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 36-36: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (5)
LDKNodeMonday/App/LDKNodeMondayApp.swift (3)
12-16: LGTM! Clean state management setup.The state management implementation using @State properties follows SwiftUI best practices.
52-52: Address TODO: Enhance start method with backup parameters.The TODO suggests that the start method could be enhanced to use backup parameters. This would improve flexibility and configuration options.
Would you like me to help implement the parameter passing from backupInfo to the start method?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 52-52: TODOs should be resolved (.start could take parameters f...)
(todo)
79-84: LGTM! Well-structured state enum.The AppState enum is clear, concise, and appropriately scoped with the public modifier.
LDKNodeMonday/View Model/Profile/SettingsViewModel.swift (2)
13-13: LGTM! Clean transition to AppState binding.The changes correctly implement the new state management approach using
@Binding var appState: AppStateinstead of@AppStorage("isOnboarding").Also applies to: 22-24
52-61: Verify reset functionality addresses reported issues.Based on the PR comments about reset issues, let's verify that this implementation properly handles device reset scenarios.
Run the following script to check for any remaining
isOnboardingreferences that might affect the reset behavior:✅ Verification successful
Reset implementation is correct and comprehensive
The implementation properly handles the reset scenario by:
- Cleaning up all necessary data (LightningNodeService, keyClient)
- Correctly transitioning to onboarding state
- No conflicting persistent state that could prevent proper reset
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining isOnboarding references that might affect reset behavior echo "Searching for remaining isOnboarding references..." rg --type swift "isOnboarding" echo "Searching for UserDefaults access that might affect reset..." ast-grep --pattern 'UserDefaults.standard'Length of output: 315
Script:
#!/bin/bash # Search for app state related code and state transitions echo "Searching for AppState enum and related code..." rg --type swift "enum.*AppState" rg --type swift "appState.*=" echo "Searching for state persistence..." rg --type swift "UserDefaults|@AppStorage" echo "Searching for other potential reset handlers..." rg --type swift "func.*reset|reset\("Length of output: 1534
Fixed this by:
Should be good to go now. |
|
ACK, great job!! |
2 participants
This PR refactors the startup and onboarding flows by using AppState enums instead of .isOnboarding from user defaults.
It removes the unused StartView and viewModel, and adds a LoadingView and ErrorView
Broken out from #145
Summary by CodeRabbit
New Features
ErrorViewandLoadingViewto improve user experience.Removed Features
StartViewandStartViewModel.Improvements