Tracker combine changes #2
Conversation
|
Warning Rate limit exceeded@digitaldan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 10 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces various modifications across multiple files, including the addition of directories to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OpenHABApp
participant NetworkTracker
participant HTTPClient
participant ViewController
User->>OpenHABApp: Interacts with UI
OpenHABApp->>NetworkTracker: Check network status
NetworkTracker->>HTTPClient: Manage connections
HTTPClient->>ViewController: Update UI based on network state
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
Outside diff range and nitpick comments (2)
openHAB/OpenHABViewController.swift (1)
19-19: Approved addition oftrackerCancellablesproperty.The addition of
trackerCancellablesis a good practice for managing Combine subscriptions. It's recommended to document its usage within the class to clarify how it should be used to manage subscriptions effectively.OpenHABCore/Sources/OpenHABCore/Util/HTTPClient.swift (1)
30-30: Review of Timeout Configuration ChangeThe reduction of
timeoutIntervalForRequestfrom 30 seconds to 10 seconds is a significant change that can enhance responsiveness by reducing the wait time for unresponsive requests. However, this might also increase the rate of timeouts in slower network conditions.Recommendation:
Monitor the impact of this change on network reliability and consider introducing configuration options for timeouts to adapt to varying network conditions.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- .github/workflows/publish.yml (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Model/OpenHABCommandOptions.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Model/OpenHABWidget.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/HTTPClient.swift (1 hunks)
- fastlane/Fastfile (1 hunks)
- openHAB.xcodeproj/project.pbxproj (4 hunks)
- openHAB/OpenHABRootViewController.swift (4 hunks)
- openHAB/OpenHABSitemapViewController.swift (8 hunks)
- openHAB/OpenHABTracker.swift (5 hunks)
- openHAB/OpenHABViewController.swift (1 hunks)
- openHAB/OpenHABWebViewController.swift (6 hunks)
- openHABWatch Extension/openHABWatch Extension/Model/ObservableOpenHABWidget.swift (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/publish.yml
- openHAB.xcodeproj/project.pbxproj
Additional context used
SwiftLint
openHAB/OpenHABRootViewController.swift
[Warning] 260-260: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
[Warning] 340-340: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
Additional comments not posted (19)
openHAB/OpenHABViewController.swift (3)
12-12: Approved import of Combine framework.The inclusion of the Combine framework is essential for the new reactive programming features introduced in this class.
Line range hint
90-125: Approved SSL certificate handling inServerCertificateManagerDelegate.The implementation of SSL certificate validation and user interaction for server trust decisions is well-handled. This is crucial for maintaining security and user trust in the application.
Line range hint
127-172: Approved client certificate handling inClientCertificateManagerDelegate.The methods for handling client certificate import, password requests, and error alerts are well-implemented. This ensures secure handling of client certificates and proper user involvement.
fastlane/Fastfile (1)
103-103: Conditional logic update appears correct, but verify broader impact.The updated conditional logic to prevent version increments when the bump type is set to 'none' aligns well with the PR objectives. This change should help in managing version increments more effectively during different types of builds.
However, it's crucial to verify that this change integrates smoothly with the rest of the build process and does not introduce any unintended side effects, especially in non-CI environments or other branches.
openHABWatch Extension/openHABWatch Extension/Model/ObservableOpenHABWidget.swift (1)
98-98: Improved nil handling inmappingsOrItemOptions.The modification to provide a default empty string for nil
labelvalues enhances the robustness of themappingsOrItemOptionsproperty. This change prevents potential runtime errors related to nil values, aligning with best practices for optional handling in Swift.OpenHABCore/Sources/OpenHABCore/Model/OpenHABWidget.swift (1)
121-121: Good handling of optional values in UI-related code.The modification to default
labelto an empty string whennilis a robust way to handle optional strings in Swift, especially in UI contexts wherenilvalues can lead to crashes or unexpected behavior. This change enhances the stability and reliability of themappingsOrItemOptionsproperty.openHAB/OpenHABTracker.swift (3)
13-13: Approved import of Combine.The addition of the
Combineframework supports the new reactive programming model used in theOpenHABTrackerclass.
19-28: Well-designedOpenHABTrackerStateclass.The class appropriately uses optional properties to manage the state of the tracker, which allows for a clear representation of both presence and absence of values.
33-34: Effective use of@Publishedproperties inOpenHABTracker.The transition to reactive programming using
@Publishedproperties forstateandprogresssimplifies state management and enhances the reactivity of the application.openHAB/OpenHABWebViewController.swift (3)
258-261: Custom user agent for iPads enhances user experience.The addition of a custom user agent for iPads ensures that web content is rendered appropriately on larger screens, accommodating the default desktop view in newer iOS versions.
76-96: Effective use of Combine for reactive programming.The implementation of Combine's
sinkmethod to handle state and progress updates enhances the responsiveness and modularity of the code. The use oftrackerCancellablesto manage subscription lifecycles is also commendable.Ensure that all subscriptions are properly disposed of to prevent memory leaks or unintended behavior.
Verification successful
Combine subscriptions are properly disposed of.
The
trackerCancellablesset is used effectively to manage the lifecycle of Combine subscriptions. The call totrackerCancellables.removeAll()in theviewWillDisappearmethod ensures that subscriptions are disposed of appropriately, preventing memory leaks or unintended behavior.
OpenHABWebViewController.swift: Subscriptions are disposed of inviewWillDisappear.OpenHABSitemapViewController.swift: Subscriptions are disposed of inviewWillDisappear.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper disposal of Combine subscriptions. # Test: Search for any remaining subscriptions that might not be disposed. rg --type swift -A 5 $'trackerCancellables'Length of output: 4040
105-106: Proper cleanup of delegate methods.The removal of multicast delegate methods aligns with the shift towards a more reactive programming model using Combine. This simplifies the codebase and reduces complexity.
Verify that all functionalities previously handled by the delegates are now correctly managed by the new Combine subscriptions.
openHAB/OpenHABRootViewController.swift (3)
12-12: Approved addition of Combine.The import of Combine supports the shift to a reactive programming model, enhancing the application's responsiveness and maintainability.
347-347: Approved robustness improvement in menu handling.The modifications to
showSideMenuimprove the robustness by adding checks before presenting the side menu, preventing potential crashes. This change enhances the user experience by ensuring smoother navigation.Consider further testing to ensure that the menu handling works as expected across different scenarios and device orientations.
211-232: Approved navigation logic enhancement.The modifications to
navigateCommandActionenhance the navigation logic by ensuring the correct view controller is activated based on the command received. This structured approach improves maintainability and functionality.However, ensure that the regex pattern used for command parsing is thoroughly tested to prevent potential issues with command interpretation.
Run the following script to verify the regex pattern:
Verification successful
Regex pattern in
navigateCommandActionis correctly implemented.The regex pattern
/^(\/basicui\/app\\?.*|\/.*|.*)$/is appropriately designed to match various command paths, ensuring the correct view controller is activated. The pattern effectively handles paths starting with/basicui/app?, any path starting with/, and any other string. No issues were found with its implementation.
- The pattern is used in
openHAB/OpenHABRootViewController.swiftto extract the path from the command.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the regex pattern used in navigateCommandAction. # Test: Search for the regex pattern usage. Expect: Only occurrences of the correct pattern. rg --type swift -A 5 $'navigateCommandAction'Length of output: 1060
openHAB/OpenHABSitemapViewController.swift (4)
15-15: Approved: Import of Combine framework.The addition of the Combine framework supports the shift towards reactive programming, enabling the new features related to state and progress handling in the class.
169-175: Approved: Subscription to progress updates.The setup for subscribing to
OpenHABTracker.shared.$progressis correctly implemented usingdropFirst()to avoid initial notifications. The closure effectively logs and displays progress updates. Ensure that the popup message functionality is tested thoroughly.Verification successful
Verified: Popup message functionality is correctly implemented.
The
showPopupMessagemethod is consistently used across the codebase with appropriate parameters for different contexts. The implementation inOpenHABViewController.swiftusingSwiftMessagesis suitable for displaying popups. No issues found with its usage.
OpenHABWebViewController.swift: Proper usage ofshowPopupMessagefor connection and error messages.OpenHABSitemapViewController.swift: Correct implementation for various message types including errors and warnings.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the popup message functionality by checking if the `showPopupMessage` method is called correctly in other parts of the codebase. # Test: Search for the usage of `showPopupMessage`. Expect: Proper usage and handling in the context where it's called. rg --type swift -A 5 $'showPopupMessage'Length of output: 5999
471-496: Approved: Modifications to pushSitemap using Combine.The
pushSitemapfunction is effectively modified to utilize Combine for handling state changes. The management of the subscription lifecycle is correctly handled. Verify that the navigation logic correctly triggers under various conditions.Verification successful
Verified: Navigation logic in pushSitemap and other usages of pushViewController
The navigation logic in the
pushSitemapfunction and other usages ofpushViewControllerin the codebase are correctly implemented. The consistent pattern of instantiating and pushing new view controllers ensures proper navigation flow. No issues were found with the current implementation.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the navigation logic by checking if the `pushViewController` method is called correctly in other parts of the codebase. # Test: Search for the usage of `pushViewController`. Expect: Proper usage and handling in the context where it's called. rg --type swift -A 5 $'pushViewController'Length of output: 3265
Line range hint
177-213: Approved: Implementation of stateWatcher for observing state changes.The
stateWatcheris correctly set up to observe state changes, usingdropFirst()to filter out the initial state. The handling of state changes, including error logging and UI updates, is appropriately implemented. Verify that the error handling works as expected in various scenarios.
| @@ -13,5 +13,5 @@ import Foundation | |||
|
|
|||
| public class OpenHABCommandOptions: Decodable { | |||
| public var command = "" | |||
| public var label = "" | |||
| public var label: String? = "" | |||
There was a problem hiding this comment.
Correct the default value of the label property.
The label property in the OpenHABCommandOptions class has been changed to an optional string, which is a significant modification for handling cases where a label is not necessary. However, the default value should be nil as per the summary and alterations list, not an empty string "". This needs correction to align with the intended functionality enhancements and to avoid potential nullability issues in the application.
- public var label: String? = ""
+ public var label: String? = nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public var label: String? = "" | |
| public var label: String? = nil |
openHAB/OpenHABTracker.swift
Outdated
| state = OpenHABTrackerState(openHABUrl: nil, openHABVersion: nil, error: errorMessage("error")) | ||
| return | ||
| } | ||
| multicastDelegate.invoke { $0.openHABTrackingProgress(NSLocalizedString("connecting", comment: "")) } | ||
| progress = NSLocalizedString("connecting", comment: "") | ||
| tryUrls(urls) { url, version, error in | ||
| if let error { | ||
| os_log("OpenHABTracker failed %{PUBLIC}@", log: .default, type: .info, error.localizedDescription) | ||
| self.multicastDelegate.invoke { $0.openHABTrackingError(error) } | ||
| self.state = OpenHABTrackerState(openHABUrl: nil, openHABVersion: nil, error: error) | ||
| } else { | ||
| self.appData?.openHABVersion = version | ||
| self.appData?.openHABRootUrl = url?.absoluteString ?? "" | ||
| self.multicastDelegate.invoke { $0.openHABTracked(url, version: version) } | ||
| self.state = OpenHABTrackerState(openHABUrl: url, openHABVersion: version, error: nil) |
There was a problem hiding this comment.
Complex but necessary implementation in tryAll.
The method handles multiple network endpoints effectively. Consider adding more comments to explain the logic behind the use of timers and network requests for future maintainability.
| } else { | ||
| os_log("Request succeeded", log: .default, type: .info) | ||
| if let data { | ||
| os_log("Data: %{public}@", log: .default, type: .debug, String(data: data, encoding: .utf8) ?? "") |
There was a problem hiding this comment.
Suggest specifying UTF-8 encoding during data conversion.
To address the static analysis warnings, specify UTF-8 encoding when converting between String and Data. This will ensure consistent and safe data handling across different environments.
- String(data: data, encoding: .utf8) ?? ""
+ String(data: data, encoding: .utf8) ?? "Invalid data"Also applies to: 340-340
Tools
SwiftLint
[Warning] 260-260: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/Preferences.swift (1 hunks)
- openHAB.xcodeproj/project.pbxproj (0 hunks)
- openHAB/OpenHABRootViewController.swift (7 hunks)
- openHAB/OpenHABSitemapViewController.swift (7 hunks)
- openHAB/OpenHABTracker.swift (0 hunks)
- openHAB/OpenHABWebViewController.swift (3 hunks)
Files not reviewed due to no reviewable changes (2)
- openHAB.xcodeproj/project.pbxproj
- openHAB/OpenHABTracker.swift
Files skipped from review as they are similar to previous changes (1)
- openHAB/OpenHABWebViewController.swift
Additional context used
SwiftLint
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift
[Warning] 16-16: TODOs should be resolved (these strings should reference...)
(todo)
openHAB/OpenHABRootViewController.swift
[Warning] 287-287: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
[Warning] 363-363: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
Additional comments not posted (25)
OpenHABCore/Sources/OpenHABCore/Util/Preferences.swift (5)
18-20: LGTM!The private properties are correctly declared, and the use of
CurrentValueSubjectenables reactive programming capabilities for the property wrapper.
24-25: Excellent work!The modifications to the
wrappedValuegetter and setter ensure that the property wrapper stays in sync withUserDefaultsand allows observers to react to changes in the wrapped value. Dispatching the update asynchronously on the main queue is a good practice to avoid blocking the current thread.Also applies to: 29-32
36-40: Nice improvement!Initializing the
subjectwith the current value fromUserDefaultsensures that the initial state of the property wrapper is consistent and enhances the state management. Well done!
43-45: Great addition!The
projectedValueproperty exposes thesubjectas anAnyPublisher, allowing external components to subscribe to changes in the wrapped value. Erasing the type toAnyPublisherprovides a clean and generic interface. Well done!
50-52: Fantastic work!The changes to the
UserDefaultURLproperty wrapper introduce reactive programming capabilities and improve URL handling. ThewrappedValuegetter and setter now include logic to trim and validate URLs before sending them to thesubject. The initializer andprojectedValueproperty follow the same pattern asUserDefaultfor consistency.The addition of the
uriWithoutTrailingSlashesfunction is a nice touch to normalize URLs. Overall, these changes enhance the functionality and reliability of theUserDefaultURLproperty wrapper. Great job!Also applies to: 56-58, 61-75, 79-83, 86-88, 90-94
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (8)
25-37: LGTM!The struct implementation is correct and the conformance to
Equatableis appropriate.
39-182: Excellent implementation!The
NetworkTrackerclass is well-structured and follows the single responsibility principle. It effectively utilizesNWPathMonitorfor network monitoring and Alamofire for making network requests. The connection attempt logic is robust, handling priority connections and fallbacks appropriately. The class also maintains good encapsulation with appropriate access control for its properties and methods.Great job on this implementation!
62-65: LGTM!The
startTrackingmethod is correctly implemented. It appropriately sets theconnectionObjectsproperty and initiates the connection attempt process.
67-84: LGTM!The
checkActiveServermethod is correctly implemented. It appropriately checks the reachability of the active server by making a lightweight request using Alamofire. Based on the response, it either updates the status to connected or initiates a new connection attempt. The logic is sound and handles the active server check effectively.
86-156: Excellent implementation!The
attemptConnectionmethod is well-structured and handles the connection attempt process effectively. The use of a dispatch group for managing concurrent requests is appropriate and ensures proper synchronization. The priority wait time mechanism is a clever approach to prioritize connections and give preference to higher priority servers.The logic for setting the active server based on the success of priority connections is sound. If a priority connection succeeds, it is correctly set as the active server. If no priority connection succeeds, the method falls back to using the first non-priority connection.
Overall, this method demonstrates a robust and efficient approach to establishing connections based on server priority.
158-165: LGTM!The
setActiveServermethod is correctly implemented. It appropriately updates theactiveServerproperty if the provided server is different from the current active server. This ensures that the active server is only updated when necessary.Additionally, it updates the status to connected, reflecting the successful establishment of a connection.
167-171: LGTM!The
updateStatusmethod is correctly implemented. It appropriately updates thestatusproperty if the new status is different from the current status. This ensures that the status is only updated when necessary, avoiding redundant updates.
173-181: LGTM!The
getServerInfoFromDatamethod is correctly implemented and handles the extraction of server version information effectively. It appropriately decodes the response data into anOpenHABServerPropertiesinstance using ado-catchblock for error handling.The logic for setting the minimum version to 2 is correct, as mentioned in the comment. This ensures that versions 2.0 through 2.4, which return "1" as their version, are treated as version 2 or higher.
If the decoding fails, the method returns -1, indicating an invalid version. This is an appropriate fallback value to indicate an error in parsing the server information.
openHAB/OpenHABRootViewController.swift (8)
12-12: LGTM!The import statement is necessary to use Combine's reactive programming features in the file.
39-39: LGTM!The
cancellablesproperty is used to store theAnyCancellableinstances returned by Combine publishers. This allows the subscriptions to be cancelled when the view controller is deallocated, preventing memory leaks.
Line range hint
99-146: LGTM!The
setupTrackermethod is a well-structured and reactive approach to tracking network connections and updating the app's state based on the active server. It leverages Combine's declarative syntax to handle changes in the URLs and the active server. The subscriptions are stored in thecancellablesset to ensure proper memory management.
241-262: LGTM!The changes to the
uiCommandActionmethod introduce a more structured approach to handling different types of paths. It ensures that the correct view controller is activated based on the path and performs the appropriate actions, such as pushing a sitemap or loading a web view. The code is well-organized and follows a clear logic flow.
274-290: LGTM!The changes to the
sendCommandActionmethod leverage Combine to reactively send commands based on the active server URL. This ensures that the command is sent only when a valid server URL is available. The use ofHTTPClientfor sending the POST request is appropriate. The code handles the response and logs the success or failure accordingly.Tools
SwiftLint
[Warning] 287-287: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
287-287: ****The previous review comment suggesting the use of UTF-8 encoding when converting between
StringandDatais still valid for this line.Tools
SwiftLint
[Warning] 287-287: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
350-366: LGTM!The changes to the
ruleCommandActionmethod are similar to the changes in thesendCommandActionmethod. It leverages Combine to reactively send rule commands based on the active server URL. The use ofHTTPClientfor sending the POST request is appropriate. The code handles the response and logs the success or failure accordingly.Tools
SwiftLint
[Warning] 363-363: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
363-363: ****The previous review comment suggesting the use of UTF-8 encoding when converting between
StringandDatais still valid for this line.Tools
SwiftLint
[Warning] 363-363: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
openHAB/OpenHABSitemapViewController.swift (4)
15-15: LGTM!The import statement for the Combine framework has been added correctly.
169-183: Looks good!The new subscription to
NetworkTracker.shared.$statusis implemented correctly:
- It receives status updates on the main queue.
- Handles the
.connectingand.connectionFailedcases by showing appropriate popup messages.- The subscription is stored in the
trackerCancellablescollection for proper memory management.Great job integrating the Combine framework for handling network status updates!
Line range hint
185-216: Excellent work with the active server handling!The new code for handling active server updates is implemented properly:
- It distinguishes between the first launch and subsequent launches based on
pageUrl.- For subsequent launches, it drops the initial value of
activeServerWatcherto avoid unnecessary reloads.- Updates the
openHABRootUrland callsselectSitemap()when the active server changes.- The subscription is stored in
trackerCancellablesfor proper memory management.The logic is sound and effectively keeps the UI in sync with the current active server. Well done!
227-228: Good practice!Clearing the
trackerCancellablescollection inviewWillDisappearis the right thing to do. It ensures that the subscriptions are cancelled when the view controller is about to disappear, preventing retain cycles and memory leaks.
| // TODO: these strings should reference Localizable keys | ||
| public enum NetworkStatus: String { | ||
| case notConnected = "Not Connected" | ||
| case connecting | ||
| case connected = "Connected" | ||
| case connectionFailed = "Connection Failed" | ||
| } |
There was a problem hiding this comment.
Address the TODO comment.
The enum implementation looks good. However, as mentioned in the TODO comment, the string values should reference localized keys instead of hardcoded strings.
Do you want me to generate the code to use localized keys or open a GitHub issue to track this task?
Tools
SwiftLint
[Warning] 16-16: TODOs should be resolved (these strings should reference...)
(todo)
| // This is mainly used for navigting to a specific sitemap and path from notifications | ||
| func pushSitemap(name: String, path: String?) { | ||
| // this will be called imediately after connecting for the initial state, otherwise it will wait for the state to change | ||
| // since we do not reference the sink cancelable, this will only fire once | ||
| _ = NetworkTracker.shared.$activeServer | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self] activeServer in | ||
| if let openHABUrl = activeServer?.url, let self { | ||
| os_log("pushSitemap: pushing page", log: .default, type: .error) | ||
| let newViewController = (storyboard?.instantiateViewController(withIdentifier: "OpenHABPageViewController") as? OpenHABSitemapViewController)! | ||
| if let path { | ||
| newViewController.pageUrl = "\(openHABUrl)/rest/sitemaps/\(name)/\(path)" | ||
| } else { | ||
| newViewController.pageUrl = "\(openHABUrl)/rest/sitemaps/\(name)" | ||
| } | ||
| newViewController.openHABRootUrl = openHABUrl | ||
| navigationController?.pushViewController(newViewController, animated: true) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Useful method for sitemap navigation, but be cautious with the subscription.
The new pushSitemap(name:path:) method is a handy addition for navigating to a specific sitemap and path. It utilizes Combine to subscribe to the active server changes, ensuring that the navigation happens only when the server is available. Creating a new instance of OpenHABSitemapViewController for each navigation is also a good practice.
However, there is a potential issue with the subscription. Since the cancellable returned by sink is not stored, the subscription will continue to live even after the method execution. This can lead to unexpected behavior if the method is called multiple times.
To address this, consider either storing the cancellable in a collection (like trackerCancellables) or using a Sink instead of sink. Here's an example using Sink:
let activeServerSink = Sink<OpenHABServer, Never> { activeServer in
// ... existing code ...
}
NetworkTracker.shared.$activeServer
.receive(on: DispatchQueue.main)
.subscribe(activeServerSink)This way, the subscription will be automatically cancelled when activeServerSink is deallocated.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (1 hunks)
- openHAB/OpenHABWebViewController.swift (3 hunks)
Additional context used
SwiftLint
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift
[Warning] 16-16: TODOs should be resolved (these strings should reference...)
(todo)
Additional comments not posted (6)
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (2)
16-22: The past review comment about addressing the TODO comment to use localized keys instead of hardcoded strings forNetworkStatusenum values is still valid and applicable.Tools
SwiftLint
[Warning] 16-16: TODOs should be resolved (these strings should reference...)
(todo)
24-37: LGTM!The
ConnectionObjectstruct implementation looks good and follows best practices:
- The struct is declared as public, allowing it to be used outside the module.
- The properties are declared as public and have appropriate types.
- The struct conforms to the
Equatableprotocol and provides an implementation for the==operator.openHAB/OpenHABWebViewController.swift (4)
75-84: Reactive handling of active server changes using Combine looks good!The code segment correctly sets up a Combine subscription to
NetworkTracker.shared.$activeServer, updates theopenHABTrackedRootUrlproperty, and triggers the web view loading when a valid URL is detected. The subscription is properly stored in thetrackerCancellablesset for lifecycle management.
86-99: Reactive handling of connection status changes using Combine looks good!The code segment correctly sets up a Combine subscription to
NetworkTracker.shared.$status, shows appropriate pop-up messages based on the connection state, and properly stores the subscription in thetrackerCancellablesset for lifecycle management.
108-109: Proper cleanup of Combine subscriptions looks good!Removing the commented-out line related to the delegate removal improves code cleanliness. Also, removing all the subscriptions stored in the
trackerCancellablesset when the view disappears ensures proper cleanup and prevents memory leaks.
208-208: Refreshing the web view after clearing the existing page looks good!Adding the call to
loadWebView(force: true)after clearing the existing page in thereloadView()method ensures that the web view is refreshed appropriately.
| } | ||
| } | ||
|
|
||
| private func attemptConnection() { | ||
| guard !connectionObjects.isEmpty else { | ||
| print("Network status: No connection objects available.") | ||
| updateStatus(.notConnected) | ||
| return | ||
| } | ||
|
|
||
| // updateStatus(.connecting) | ||
| let dispatchGroup = DispatchGroup() | ||
| var highestPriorityConnection: ConnectionObject? | ||
| var nonPriorityConnection: ConnectionObject? | ||
|
|
||
| // Set the time window for priority connections (e.g., 2 seconds) | ||
| // if a priority = 0 finishes before this time, we continue, otherwise we wait this long before picking a winner based on priority | ||
| let priorityWaitTime: TimeInterval = 2.0 | ||
| var priorityWorkItem: DispatchWorkItem? | ||
|
|
||
| for connection in connectionObjects { | ||
| dispatchGroup.enter() | ||
| NetworkConnection.tracker(openHABRootUrl: connection.url) { [weak self] response in | ||
| guard let self else { | ||
| return | ||
| } | ||
| switch response.result { | ||
| case let .success(data): | ||
| let version = getServerInfoFromData(data: data) | ||
| if version > 0 { | ||
| // Handle the first connection | ||
| if connection.priority == 0, highestPriorityConnection == nil { | ||
| // This is the highest priority connection | ||
| highestPriorityConnection = connection | ||
| priorityWorkItem?.cancel() // Cancel any waiting task if the highest priority connected | ||
| setActiveServer(connection) | ||
| } else if highestPriorityConnection == nil, nonPriorityConnection == nil { | ||
| // First non-priority connection | ||
| nonPriorityConnection = connection | ||
| } | ||
| dispatchGroup.leave() | ||
| } else { | ||
| print("Network status: Failed version when connecting to: \(connection.url)") | ||
| dispatchGroup.leave() | ||
| } | ||
| case let .failure(error): | ||
| print("Network status: Failed connection to: \(connection.url) : \(error.localizedDescription)") | ||
| dispatchGroup.leave() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Create a work item that waits for the priority connection | ||
| priorityWorkItem = DispatchWorkItem { [weak self] in | ||
| if let nonPriorityConnection, highestPriorityConnection == nil { | ||
| // If no priority connection succeeded, use the first non-priority one | ||
| self?.setActiveServer(nonPriorityConnection) | ||
| } | ||
| } | ||
|
|
||
| // Wait for the priority connection for 2 seconds | ||
| DispatchQueue.global().asyncAfter(deadline: .now() + priorityWaitTime, execute: priorityWorkItem!) | ||
|
|
||
| dispatchGroup.notify(queue: .main) { | ||
| if let highestPriorityConnection { | ||
| print("Network status: Highest priority connection established: \(highestPriorityConnection.url)") | ||
| } else if let nonPriorityConnection { | ||
| print("Network status: Non-priority connection established: \(nonPriorityConnection.url)") | ||
| } else { | ||
| print("Network status: No server responded.") | ||
| self.updateStatus(.connectionFailed) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private func setActiveServer(_ server: ConnectionObject) { | ||
| print("Network status: setActiveServer: \(server.url)") | ||
|
|
||
| if activeServer != server { | ||
| activeServer = server | ||
| } | ||
| updateStatus(.connected) | ||
| } | ||
|
|
||
| private func updateStatus(_ newStatus: NetworkStatus) { | ||
| if status != newStatus { | ||
| status = newStatus | ||
| } | ||
| } | ||
|
|
||
| private func getServerInfoFromData(data: Data) -> Int { | ||
| do { | ||
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | ||
| // OH versions 2.0 through 2.4 return "1" as thier version, so set the floor to 2 so we do not think this is a OH 1.x serevr | ||
| return max(2, Int(serverProperties.version) ?? 2) | ||
| } catch { | ||
| return -1 | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The NetworkTracker class implementation looks good overall, with a few suggestions for improvement.
The class follows best practices and handles different scenarios well:
- The class is declared as public and final, allowing it to be used outside the module and preventing subclassing.
- The class uses appropriate access control for its properties and methods.
- The class uses a dispatch group to manage concurrent requests and a timeout mechanism to prioritize connections.
- The class handles different scenarios such as when no active server is available, when the active server is not reachable, and when no server responds.
- The class updates the status and active server based on the connection results.
However, consider the following suggestions for improvement:
-
The
attemptConnectionmethod is quite long and could be broken down into smaller methods for better readability and maintainability. -
In the
getServerInfoFromDatamethod, use optional binding or provide a default value when casting the server version to an integer to avoid potential crashes if the version is not a valid integer.
Apply this diff to fix the force unwrap:
-return max(2, Int(serverProperties.version) ?? 2)
+return max(2, Int(serverProperties.version ?? "2") ?? 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public final class NetworkTracker: ObservableObject { | |
| public static let shared = NetworkTracker() | |
| @Published public private(set) var activeServer: ConnectionObject? | |
| @Published public private(set) var status: NetworkStatus = .notConnected | |
| private var monitor: NWPathMonitor | |
| private var monitorQueue = DispatchQueue.global(qos: .background) | |
| private var connectionObjects: [ConnectionObject] = [] | |
| private init() { | |
| monitor = NWPathMonitor() | |
| monitor.pathUpdateHandler = { [weak self] path in | |
| if path.status == .satisfied { | |
| print("Network status: Connected") | |
| self?.attemptConnection() | |
| } else { | |
| print("Network status: Disconnected") | |
| self?.updateStatus(.notConnected) | |
| } | |
| } | |
| monitor.start(queue: monitorQueue) | |
| } | |
| public func startTracking(connectionObjects: [ConnectionObject]) { | |
| self.connectionObjects = connectionObjects | |
| attemptConnection() | |
| } | |
| private func checkActiveServer() { | |
| guard let activeServer else { | |
| // No active server, proceed with the normal connection attempt | |
| attemptConnection() | |
| return | |
| } | |
| // Check if the active server is reachable by making a lightweight request (e.g., a HEAD request) | |
| NetworkConnection.tracker(openHABRootUrl: activeServer.url) { [weak self] response in | |
| switch response.result { | |
| case .success: | |
| print("Network status: Active server is reachable: \(activeServer.url)") | |
| self?.updateStatus(.connected) // If reachable, we're done | |
| case .failure: | |
| print("Network status: Active server is not reachable: \(activeServer.url)") | |
| self?.attemptConnection() // If not reachable, run the connection logic | |
| } | |
| } | |
| } | |
| private func attemptConnection() { | |
| guard !connectionObjects.isEmpty else { | |
| print("Network status: No connection objects available.") | |
| updateStatus(.notConnected) | |
| return | |
| } | |
| // updateStatus(.connecting) | |
| let dispatchGroup = DispatchGroup() | |
| var highestPriorityConnection: ConnectionObject? | |
| var nonPriorityConnection: ConnectionObject? | |
| // Set the time window for priority connections (e.g., 2 seconds) | |
| // if a priority = 0 finishes before this time, we continue, otherwise we wait this long before picking a winner based on priority | |
| let priorityWaitTime: TimeInterval = 2.0 | |
| var priorityWorkItem: DispatchWorkItem? | |
| for connection in connectionObjects { | |
| dispatchGroup.enter() | |
| NetworkConnection.tracker(openHABRootUrl: connection.url) { [weak self] response in | |
| guard let self else { | |
| return | |
| } | |
| switch response.result { | |
| case let .success(data): | |
| let version = getServerInfoFromData(data: data) | |
| if version > 0 { | |
| // Handle the first connection | |
| if connection.priority == 0, highestPriorityConnection == nil { | |
| // This is the highest priority connection | |
| highestPriorityConnection = connection | |
| priorityWorkItem?.cancel() // Cancel any waiting task if the highest priority connected | |
| setActiveServer(connection) | |
| } else if highestPriorityConnection == nil, nonPriorityConnection == nil { | |
| // First non-priority connection | |
| nonPriorityConnection = connection | |
| } | |
| dispatchGroup.leave() | |
| } else { | |
| print("Network status: Failed version when connecting to: \(connection.url)") | |
| dispatchGroup.leave() | |
| } | |
| case let .failure(error): | |
| print("Network status: Failed connection to: \(connection.url) : \(error.localizedDescription)") | |
| dispatchGroup.leave() | |
| } | |
| } | |
| } | |
| // Create a work item that waits for the priority connection | |
| priorityWorkItem = DispatchWorkItem { [weak self] in | |
| if let nonPriorityConnection, highestPriorityConnection == nil { | |
| // If no priority connection succeeded, use the first non-priority one | |
| self?.setActiveServer(nonPriorityConnection) | |
| } | |
| } | |
| // Wait for the priority connection for 2 seconds | |
| DispatchQueue.global().asyncAfter(deadline: .now() + priorityWaitTime, execute: priorityWorkItem!) | |
| dispatchGroup.notify(queue: .main) { | |
| if let highestPriorityConnection { | |
| print("Network status: Highest priority connection established: \(highestPriorityConnection.url)") | |
| } else if let nonPriorityConnection { | |
| print("Network status: Non-priority connection established: \(nonPriorityConnection.url)") | |
| } else { | |
| print("Network status: No server responded.") | |
| self.updateStatus(.connectionFailed) | |
| } | |
| } | |
| } | |
| private func setActiveServer(_ server: ConnectionObject) { | |
| print("Network status: setActiveServer: \(server.url)") | |
| if activeServer != server { | |
| activeServer = server | |
| } | |
| updateStatus(.connected) | |
| } | |
| private func updateStatus(_ newStatus: NetworkStatus) { | |
| if status != newStatus { | |
| status = newStatus | |
| } | |
| } | |
| private func getServerInfoFromData(data: Data) -> Int { | |
| do { | |
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | |
| // OH versions 2.0 through 2.4 return "1" as thier version, so set the floor to 2 so we do not think this is a OH 1.x serevr | |
| return max(2, Int(serverProperties.version) ?? 2) | |
| } catch { | |
| return -1 | |
| } | |
| } | |
| } | |
| private func getServerInfoFromData(data: Data) -> Int { | |
| do { | |
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | |
| // OH versions 2.0 through 2.4 return "1" as thier version, so set the floor to 2 so we do not think this is a OH 1.x serevr | |
| return max(2, Int(serverProperties.version ?? "2") ?? 2) | |
| } catch { | |
| return -1 | |
| } | |
| } |
* Migrate SettingsViewController to SwiftUI - Using LabeledContent instead of HStack - Proper handling of settingsSendCrashReports when already set - Integrated ClientCertificatesView, RTFTexView * Cleaning up Main.storyboard as OpenHABLegalViewContoller, OpenHABSettingsViewController are not needed anymore * Extend Migration to Drawer and Notifcations * Porting ColorPicker and SelectionView to SwiftUI * Reintegrating develop into branch * Merge current develop branch * Enabled Preview for DrawerView * Cleaning up Delete obsolete code Remove unnecessary part of storyboard * Checking tests * Update fastlane * Revert to ColorPickerViewController / to be readdressed when SitemapViewController is migrated to SwiftUI --------- Co-authored-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
…allows native rendering for Sitemap navigation from notifications (which led to the refactor) Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
…eferences Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
digitaldan
force-pushed
the
tracker-combine-changes
branch
from
c0fef14 to
e7a35b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (6)
openHAB/ClientCertificatesView.swift (1)
33-35: Consider removing the preview provider before merging.The preview provider is useful during development but is not needed in production code. Consider removing it or wrapping it with
#if DEBUGbefore merging to production.-#Preview { - ClientCertificatesView() -}or
+#if DEBUG #Preview { ClientCertificatesView() } +#endifopenHAB/ClientCertificatesViewModel.swift (1)
16-37: LGTM!The
ClientCertificatesViewModelclass is well-designed and follows good practices such as:
- Conforming to
ObservableObjectfor use with SwiftUI views.- Using
@Publishedproperty for reactive UI updates.- Encapsulating the logic for managing client certificates.
- Interacting with
NetworkConnection.shared.clientCertificateManagerfor code reuse.- Providing a convenient method to retrieve the name of a certificate identity.
Here are some suggestions for improvement:
- Consider adding error handling for the
deleteCertificate(at:)method in case the deletion fails.- Consider adding documentation comments to the class and its methods to improve code readability and maintainability.
- Consider adding unit tests to ensure the correctness of the class's behavior.
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (2)
86-156: Comprehensive connection attempt logic!The
attemptConnectionmethod implements a robust logic for attempting connections to the available servers:
- It uses a dispatch group to manage concurrent requests to the servers, which is a good approach.
- It prioritizes connections based on the
priorityproperty of theConnectionObject, which is a useful feature.- It sets a time window for priority connections using
priorityWaitTime, which is a reasonable approach.- The iteration over the
connectionObjectsand making requests to each server is implemented correctly.- The handling of the response for each request and determining the highest priority connection and the first non-priority connection is done properly.
- Setting the active server based on the highest priority connection or the first non-priority connection is the expected behavior.
- Updating the status based on the connection result is correct.
Consider breaking down the
attemptConnectionmethod into smaller methods for better readability and maintainability. For example, you could extract the logic for handling the response and determining the connections into separate methods.
173-181: Effective server version extraction!The
getServerInfoFromDatamethod is implemented correctly:
- It decodes the response data into an
OpenHABServerPropertiesobject, which is the correct approach.- It returns the server version as an integer with a minimum value of 2, which is a good practice to handle older server versions.
- It returns -1 if the decoding fails, which is a reasonable error handling approach.
Consider handling the force unwrapping of the server version to avoid potential crashes. You can use optional binding or provide a default value. Apply this diff to fix the force unwrap:
-return max(2, Int(serverProperties.version) ?? 2) +return max(2, Int(serverProperties.version ?? "2") ?? 2)openHAB/OpenHABRootViewController.swift (2)
334-350: LGTM with a minor suggestion!The usage of Combine in the
sendCommandAction(_:)method improves the reliability of network operations by ensuring that commands are sent based on the current active server. The creation of anHTTPClientwith the username and password fromPreferencesensures secure communication with the openHAB server.The updated response handling provides better logging and error reporting.
To address the static analysis warning, consider specifying UTF-8 encoding explicitly when converting between
StringandData:- String(data: data, encoding: .utf8) ?? "" + String(data: data, encoding: .utf8) ?? "Invalid data"Tools
SwiftLint
[Warning] 347-347: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
410-426: LGTM with a minor suggestion!The usage of Combine in the
ruleCommandAction(_:)method improves the reliability of network operations by ensuring that rule commands are sent based on the current active server. The creation of anHTTPClientwith the username and password fromPreferencesensures secure communication with the openHAB server.The updated response handling provides better logging and error reporting.
To address the static analysis warning, consider specifying UTF-8 encoding explicitly when converting between
StringandData:- String(data: data, encoding: .utf8) ?? "" + String(data: data, encoding: .utf8) ?? "Invalid data"Tools
SwiftLint
[Warning] 423-423: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (36)
- .gitignore (1 hunks)
- BuildTools/.swiftlint.yml (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Model/OpenHABItem.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/Endpoint.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/HTTPClient.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (1 hunks)
- OpenHABCore/Sources/OpenHABCore/Util/Preferences.swift (1 hunks)
- openHAB.xcodeproj/project.pbxproj (17 hunks)
- openHAB.xcodeproj/xcshareddata/xcschemes/openHAB.xcscheme (1 hunks)
- openHAB.xcworkspace/xcshareddata/swiftpm/Package.resolved (1 hunks)
- openHAB/ClientCertificatesView.swift (1 hunks)
- openHAB/ClientCertificatesViewModel.swift (1 hunks)
- openHAB/ColorPickerView.swift (1 hunks)
- openHAB/DrawerUITableViewCell.swift (0 hunks)
- openHAB/DrawerView.swift (1 hunks)
- openHAB/MulticastDelegate.swift (0 hunks)
- openHAB/NotificationsView.swift (1 hunks)
- openHAB/OpenHABClientCertificatesViewController.swift (0 hunks)
- openHAB/OpenHABDrawerItem.swift (0 hunks)
- openHAB/OpenHABDrawerTableViewController.swift (0 hunks)
- openHAB/OpenHABLegalViewController.swift (0 hunks)
- openHAB/OpenHABNotification.swift (3 hunks)
- openHAB/OpenHABNotificationsViewController.swift (0 hunks)
- openHAB/OpenHABRootViewController.swift (11 hunks)
- openHAB/OpenHABSelectionTableViewController.swift (0 hunks)
- openHAB/OpenHABSettingsViewController.swift (0 hunks)
- openHAB/OpenHABSitemapViewController.swift (9 hunks)
- openHAB/OpenHABTracker.swift (0 hunks)
- openHAB/OpenHABViewController.swift (1 hunks)
- openHAB/OpenHABWebViewController.swift (3 hunks)
- openHAB/RTFTextView.swift (1 hunks)
- openHAB/SelectionView.swift (1 hunks)
- openHAB/SettingsView.swift (1 hunks)
- openHAB/Throttler.swift (1 hunks)
- openHABTestsSwift/LocalizationTests.swift (1 hunks)
- openHABWatch Extension/Views/Rows/ImageRow.swift (1 hunks)
Files not reviewed due to no reviewable changes (10)
- openHAB/DrawerUITableViewCell.swift
- openHAB/MulticastDelegate.swift
- openHAB/OpenHABClientCertificatesViewController.swift
- openHAB/OpenHABDrawerItem.swift
- openHAB/OpenHABDrawerTableViewController.swift
- openHAB/OpenHABLegalViewController.swift
- openHAB/OpenHABNotificationsViewController.swift
- openHAB/OpenHABSelectionTableViewController.swift
- openHAB/OpenHABSettingsViewController.swift
- openHAB/OpenHABTracker.swift
Files skipped from review due to trivial changes (6)
- .gitignore
- BuildTools/.swiftlint.yml
- OpenHABCore/Sources/OpenHABCore/Model/OpenHABItem.swift
- openHAB.xcodeproj/xcshareddata/xcschemes/openHAB.xcscheme
- openHAB.xcworkspace/xcshareddata/swiftpm/Package.resolved
- openHABTestsSwift/LocalizationTests.swift
Files skipped from review as they are similar to previous changes (3)
- OpenHABCore/Sources/OpenHABCore/Util/HTTPClient.swift
- OpenHABCore/Sources/OpenHABCore/Util/Preferences.swift
- openHAB/OpenHABViewController.swift
Additional context used
SwiftLint
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift
[Warning] 17-17: TODOs should be resolved (these strings should reference...)
(todo)
openHAB/DrawerView.swift
[Warning] 174-174: TODOs should be resolved (Replace network calls with app...)
(todo)
openHAB/SettingsView.swift
[Error] 234-234: Force casts should be avoided
(force_cast)
openHAB/OpenHABRootViewController.swift
[Warning] 347-347: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
[Warning] 423-423: Prefer using UTF-8 encoded strings when converting between
StringandData(non_optional_string_data_conversion)
Additional comments not posted (69)
openHAB/ClientCertificatesView.swift (3)
1-10: LGTM!The copyright header follows the standard format and looks good.
14-15: LGTM!The view struct declaration and initialization follow the recommended practices:
- Conforming to the
Viewprotocol- Using
@StateObjectto manage the view model- Keeping the view model private for better encapsulation
17-30: LGTM!The view body implementation follows the best practices and provides a good user experience:
- Using
Listfor displaying a list of items- Iterating over indices to avoid issues with
ForEachandIdentifiable- Displaying user-friendly certificate identity names
- Supporting deletion through swipe gesture
- Setting a localized navigation title
- Loading certificates on appear to ensure up-to-date data
openHAB/Throttler.swift (1)
15-15: Approve the conformance toObservableObject.The change to make the
Throttlerclass conform to theObservableObjectprotocol is a positive enhancement. It enables theThrottlerto be used more effectively in SwiftUI environments, promoting a reactive programming approach.Key benefits:
- The
Throttlercan now notify views about changes in its state, facilitating automatic UI updates.- This change aligns with SwiftUI's state management principles and enhances the class's usability in modern Swift applications.
Note: Existing code that uses the
Throttlerclass may need to be updated to leverage the benefits ofObservableObject. Consider reviewing and updating the usage ofThrottlerinstances throughout the codebase to ensure a consistent and reactive approach.openHABWatch Extension/Views/Rows/ImageRow.swift (1)
29-29: LGTM!The change to the placeholder image symbol is minor and does not affect the functionality of the
ImageRowstruct. The new symbol may provide a better visual representation of the placeholder image.openHAB/RTFTextView.swift (3)
15-16: LGTM!The
RTFTextViewstruct is well-designed:
- It has a clear name and purpose.
- It conforms to the appropriate protocol
UIViewRepresentable.- The
rtfFileNameproperty is aptly named and typed.
18-24: LGTM!The
makeUIViewmethod is implemented correctly:
- It creates and returns a
UITextViewinstance, fulfilling the protocol requirement.- The
UITextViewis appropriately configured for displaying read-only content.
26-44: LGTM!The
updateUIViewmethod is implemented correctly:
- It loads the RTF file specified by
rtfFileNameand updates theUITextView, fulfilling the protocol requirement.- It uses
NSAttributedStringappropriately for loading the RTF content.- It includes error handling for cases where the file cannot be found or loaded.
- It sets the background and text colors using custom colors, which is a good practice for consistency.
openHAB/SelectionView.swift (2)
16-40: LGTM!The
SelectionViewstruct is well-structured and follows SwiftUI best practices. The use of@Bindingto track the selected item state and theonTapGesturemodifier to handle row selection is appropriate. The view correctly displays the mapping labels and indicates the selected item with a checkmark. The navigation title provides context to the user.Overall, the implementation is clean, readable, and functional.
42-54: Preview block looks good!The preview block effectively demonstrates the usage of
SelectionViewwith representative sample data. It showcases how to create an instance ofSelectionViewwith the required parameters, including the mappings array, selected item binding, and selection closure.The selected mapping index is printed to the console, which is helpful for debugging and understanding the behavior of the view.
The preview block is a valuable addition to the code, as it provides a quick way to visualize and test the
SelectionViewin isolation.openHAB/OpenHABNotification.swift (7)
15-15: LGTM!Changing the
messageproperty to an optionalString?allows for more flexible handling of potentially absent values.
17-18: LGTM!Changing the
iconandseverityproperties to optionalString?types is consistent with the change made to themessageproperty and allows for more flexible handling of potentially absent values.
19-19: Clarify the purpose and usage of theidproperty.A new
idproperty has been added to theOpenHABNotificationclass. Could you please provide more context on the purpose and intended usage of this property? This will help ensure that it is being used appropriately throughout the codebase.
20-25: LGTM, but clarify the purpose and usage of theidparameter.The updated initializer is consistent with the changes made to the
message,icon, andseverityproperties, and the default values allow for greater flexibility when creating instances ofOpenHABNotification.However, as mentioned in the previous comment, could you please provide more context on the purpose and intended usage of the
idparameter? This will help ensure that it is being used appropriately when creating instances ofOpenHABNotification.
28-30: LGTM!The modified convenience initializer now utilizes the updated initializer with default values for all parameters, ensuring that all properties can be set appropriately when creating an instance from a dictionary.
53-53: LGTM!Changing the
messageproperty in theCodingDatastruct to an optionalString?is consistent with the modification made to themessageproperty in theOpenHABNotificationclass. This ensures that the decoding process can handle optional values for themessageproperty.
69-69: LGTM, but clarify the purpose and usage of theidparameter.Including the
idparameter in the conversion method fromCodingDatatoOpenHABNotificationensures that all relevant data is passed during the conversion process. This change is consistent with the addition of theidproperty to theOpenHABNotificationclass.However, as mentioned in the previous comments, could you please provide more context on the purpose and intended usage of the
idparameter? This will help ensure that it is being used appropriately throughout the codebase.openHAB/ColorPickerView.swift (1)
1-77: LGTM!The
ColorPickerViewis a well-structured and modular SwiftUI component that enhances the user interface by allowing dynamic color selection for OpenHAB widgets. The code follows best practices, utilizes appropriate state management, and effectively manages the frequency of updates using theThrottlerclass.Some key observations:
- The view maintains state variables for the selected color and its HSB components using
@State.- The
Throttlerclass is utilized to manage the frequency of updates sent to the associated OpenHAB widget, improving performance by reducing unnecessary network traffic.- The
updateHSBfunction correctly converts the selected color to HSB values.- The
sendColorUpdatefunction sends the color update to the widget using the appropriate command format.- The code is well-documented with comments explaining the purpose of each section.
- The code follows the project's coding style and conventions.
Overall, the
ColorPickerViewis a valuable addition to the project, enhancing the user experience and providing a modular and reusable component for color selection.openHAB/NotificationsView.swift (2)
17-69: LGTM!The
NotificationRowstruct is well-structured and follows best practices for SwiftUI views. The use of the Kingfisher library for asynchronous image loading is a good choice for improving performance and user experience. ThedateStringfunction correctly formats the notification timestamp for display.
71-106: LGTM!The
NotificationsViewstruct is well-structured and follows best practices for SwiftUI views. The use of aListwithNotificationRowcells is a clean and efficient way to display the notifications. TherefreshableandonAppearmodifiers provide a good user experience for loading and refreshing the data.The
loadNotificationsfunction correctly separates the network logic from the view and handles the JSON decoding and error cases appropriately.OpenHABCore/Sources/OpenHABCore/Util/Endpoint.swift (2)
20-33: LGTM!The changes to the
IconTypeenum look good:
- Conforming to
CaseIterable,Identifiable, andCustomStringConvertibleprotocols enhances the enum's functionality and usability.- The
idcomputed property correctly fulfills theIdentifiableprotocol requirement by returning the enum instance itself.- The
descriptionproperty provides an accurate string representation for each case.These improvements make the enum more versatile and easier to use in various contexts without altering its core functionality.
36-48: Looks good!The modifications to the
SortSitemapsOrderenum are appropriate:
- Conforming to
CaseIterableandCustomStringConvertibleprotocols extends the enum's functionality and usability.- The
descriptionproperty correctly provides a string representation for each case, returning "Label" for the.labelcase and "Name" for the.namecase.These enhancements make the enum more flexible and easier to use in different scenarios without modifying its underlying behavior.
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (8)
17-23: The following comment from the previous review is still applicable:Address the TODO comment.
The enum implementation looks good. However, as mentioned in the TODO comment, the string values should reference localized keys instead of hardcoded strings.
Do you want me to generate the code to use localized keys or open a GitHub issue to track this task?
Tools
SwiftLint
[Warning] 17-17: TODOs should be resolved (these strings should reference...)
(todo)
25-38: LGTM!The
ConnectionObjectstruct implementation looks good. It correctly represents a server connection with a URL and priority, and properly implements theEquatableprotocol.
40-47: Great implementation!The
NetworkTrackerclass declaration and properties are well-structured:
- The singleton pattern is used appropriately for a network tracker.
- The published properties
activeServerandstatusprovide a reactive way to observe changes.NWPathMonitoris utilized correctly to monitor network changes.
48-60: Nicely done!The
NetworkTrackerinitializer is implemented correctly:
- It properly sets up the
NWPathMonitorand itspathUpdateHandler.- It updates the connection status based on the network path status.
- It starts the network monitoring on a background queue, which is a good practice.
- Using a weak reference to
selfin the closure is important to avoid retain cycles.
62-65: Looks good!The
startTrackingmethod is implemented correctly:
- It assigns the provided connection objects to the
connectionObjectsproperty.- It calls the
attemptConnectionmethod to initiate the connection process, which is the appropriate action.
67-84: Well-structured method!The
checkActiveServermethod is implemented correctly:
- It checks the reachability of the active server by making a lightweight request to the server's URL.
- If the active server is reachable, it updates the status to
.connected, which is the correct action.- If the active server is not reachable, it calls the
attemptConnectionmethod to find a new active server, which is the appropriate action.
158-165: LGTM!The
setActiveServermethod is implemented correctly:
- It updates the
activeServerproperty if the provided server is different from the current active server, which is a good practice to avoid unnecessary updates.- It updates the status to
.connected, which is the correct action when setting the active server.
167-171: Looks good!The
updateStatusmethod is implemented correctly:
- It checks if the new status is different from the current status before updating the
statusproperty, which is a good practice to avoid unnecessary updates.openHAB/DrawerView.swift (5)
19-36: LGTM!The function correctly decodes the response data into an array of
OpenHABSitemapobjects and handles potential decoding errors by logging them.
38-42: LGTM!The
UiTilestruct is correctly defined to decode UI tile data from a network response.
44-69: LGTM!The
ImageViewstruct correctly handles different image URL formats and uses the Kingfisher library to load and display images. It also provides a fallback placeholder image for empty URLs.
71-231: LGTM!The
DrawerViewstruct correctly defines the structure and behavior of the drawer menu. It loads sitemaps and UI tiles data when the view appears and updates the corresponding state variables. The menu items are properly organized and handle user interactions to dismiss the drawer and invoke the appropriate callbacks.Tools
SwiftLint
[Warning] 174-174: TODOs should be resolved (Replace network calls with app...)
(todo)
227-230: LGTM!The
loadSettingsfunction correctly loads the openHAB username and password fromPreferencesand assigns them to the corresponding properties of theDrawerViewstruct.openHAB/SettingsView.swift (5)
320-325: LGTM!The function is correctly using the Kingfisher library to clear the image cache. The implementation looks good.
327-330: LGTM!The function is correctly using SFSafariViewController to present the privacy policy. The implementation looks good.
332-347: LGTM!The function is correctly loading the application settings from the
Preferencesclass. The implementation looks good.
349-367: LGTM!The function is correctly saving the application settings to the
Preferencesclass, syncing them to the watch, and setting the crash reporting status. The implementation looks good.
57-318: LGTM!The
bodycomputed property is well-structured and uses SwiftUI components effectively to define the settings view. The settings are loaded and saved correctly using theloadSettingsandsaveSettingsfunctions. The implementation looks good overall.Tools
SwiftLint
[Error] 234-234: Force casts should be avoided
(force_cast)
openHAB/OpenHABWebViewController.swift (4)
75-84: Reactive approach using Combine for improved network state management.The new
sinksubscription onNetworkTracker.shared.$activeServersimplifies the code and improves responsiveness to network changes. Storing the subscription intrackerCancellablesensures proper lifecycle management.
86-99: Reactive approach using Combine for improved user feedback.The new
sinksubscription onNetworkTracker.shared.$statussimplifies the code and improves user experience by providing timely feedback based on the connection state. Thereceive(on:)operator ensures that the UI updates are performed on the main queue, and storing the subscription intrackerCancellablesensures proper lifecycle management.
108-109: Improved code readability and subscription lifecycle management.Removing the commented-out line related to delegate removal improves code readability. Clearing the
trackerCancellablesset ensures that all subscriptions are cancelled when the view controller disappears, preventing potential memory leaks.
208-208: Improved user experience by ensuring web view refresh.Calling
loadWebView(force: true)after clearing the existing page ensures that the web view is refreshed with the latest URL, improving the user experience by always displaying the most up-to-date content when reloading the view.openHAB/OpenHABRootViewController.swift (7)
12-19: LGTM!The new imports for
CombineandSwiftUIare necessary for the reactive programming approach and the integration of SwiftUI views in the class.
23-28: LGTM!The addition of the
browser(String)andtile(String)cases to theTargetControllerenum enhances the flexibility of the navigation system, allowing the handling of browser and tile URLs.
Line range hint
42-148: LGTM!The introduction of the
setupTracker()method and the usage of Combine for network tracking is a great improvement. It ensures that the active server is always up-to-date, enhancing the responsiveness of the application.The implementation of the
setupTracker()method is clean and concise, combining thelocalUrlandremoteUrlpreferences to createConnectionObjectinstances and start tracking them usingNetworkTracker.
171-184: LGTM!The modification of the
setupSideMenu()method to use a SwiftUIDrawerViewhosted in aUIHostingControlleras the side menu is a great improvement. It enhances the user experience and aligns with the modern SwiftUI approach.The
handleDismiss(mode:)method allows for proper handling of side menu dismissals based on theTargetControllermode, ensuring a smooth user experience.
186-207: LGTM!The introduction of the
openTileURL(_:)andopenURL(url:)methods improves code organization and readability by separating the handling of tile URLs and general URLs.The usage of
SFSafariViewControllerfor presenting web content provides a seamless user experience, allowing users to view web pages within the app.
209-238: LGTM!The expansion of the
handleDismiss(mode:)method to handle dismissals for the newbrowserandtilecases ofTargetControllerensures proper handling of side menu dismissals.The method maintains a consistent behavior across all
TargetControllermodes, dismissing the side menu and callingmodalDismissed(to:)with the appropriate mode.
301-322: LGTM!The modifications to the
uiCommandAction(_:)method improve the handling of commands, particularly for paths that start with "/basicui/app?". The usage ofsitemapViewControllerfor handling sitemap-related paths ensures a more appropriate navigation experience.The existing logic for handling other paths using
webViewControlleris preserved, maintaining backward compatibility.openHAB/OpenHABSitemapViewController.swift (4)
15-15: LGTM!The import of the Combine framework is a good addition for leveraging reactive programming patterns in this class.
167-182: Excellent use of Combine for network status handling!Subscribing to the
NetworkTracker.shared.$statuspublisher and reacting to connection status changes by displaying appropriate popup messages is a great way to keep the user informed about the network state.Storing the subscription in
trackerCancellablesfor proper memory management is also a good practice.
Line range hint
184-215: Great use of Combine to observe active server changes!Introducing
activeServerWatcherto observe changes in the active server state is a good way to keep the UI in sync with the current server.Using
dropFirst()to prevent immediate notifications with the initial value is a smart move to avoid unnecessary UI updates when the view first loads.Storing the subscription in
trackerCancellablesensures proper lifecycle management of the subscription.
473-492: Useful addition for targeted sitemap navigation!The new
pushSitemapfunction provides a handy way to navigate to a specific sitemap and path, enhancing the navigation capabilities of the app.Creating a new instance of
OpenHABSitemapViewControllerfor each navigation is a good practice to ensure a clean state.Not storing the subscription is acceptable in this case, as it only needs to react once to the state. However, the previous review comment about using a
Sinkinstead ofsinkto automatically cancel the subscription when no longer needed is still valid and applicable.openHAB.xcodeproj/project.pbxproj (14)
88-88: LGTM!The addition of
SettingsView.swiftis approved. It enables a new settings view implemented using SwiftUI.
99-100: Looks good!The addition of
ClientCertificatesViewModel.swiftandClientCertificatesView.swiftis approved. These files implement a new client certificates feature, with the view model handling the business logic and the SwiftUI view presenting the UI. This change enhances the app's functionality.
103-106: UI enhancements approved!The addition of
DrawerView.swift,NotificationsView.swift,ColorPickerView.swift, andSelectionView.swiftis approved. These new SwiftUI views enhance various UI components of the app, such as navigation, notifications, color picking, and selection. They contribute to an improved user interface and user experience.
114-114: New feature approved!The addition of
RTFTextView.swiftis approved. This file implements a text view component that supports the display of Rich Text Format (RTF) content. It extends the app's functionality by enabling the presentation of formatted text.
373-373: Build settings update looks good!The addition of
SettingsView.swiftto theopenHABtarget's "Sources" build phase is approved. This ensures that the settings view implementation is compiled and linked with the app.
385-386: Client certificates feature integration approved!The addition of
ClientCertificatesViewModel.swiftandClientCertificatesView.swiftto theopenHABtarget's "Sources" build phase is approved. This ensures that the client certificates feature is compiled and linked with the app, integrating it into the build.
389-392: UI enhancements build integration approved!The addition of
DrawerView.swift,NotificationsView.swift,ColorPickerView.swift, andSelectionView.swiftto theopenHABtarget's "Sources" build phase is approved. This ensures that the UI enhancements are compiled and linked with the app, integrating them into the build.
403-403: RTF text view build integration approved!The addition of
RTFTextView.swiftto theopenHABtarget's "Sources" build phase is approved. This ensures that the Rich Text Format (RTF) text view functionality is compiled and linked with the app, integrating it into the build.
834-837: Project organization improvement approved!The addition of
SelectionView.swift,SettingsView.swift,ClientCertificatesView.swift, andRTFTextView.swiftto theopenHABtarget's "UI" group is approved. Grouping related user interface files together enhances code organization, making the project easier to navigate and maintain.
838-839: UI group organization approved!The addition of
NotificationsView.swiftandDrawerView.swiftto theopenHABtarget's "UI" group is approved. Grouping these user interface files together improves project organization and maintainability.
968-968: Model group organization approved!The addition of
ClientCertificatesViewModel.swiftto theopenHABtarget's "Models" group is approved. Grouping related model files together enhances project organization and maintainability.
Line range hint
1435-1477: Build phase updates approved!The addition of various Swift files, including UI components, views, and utility classes, to the
openHABtarget's "Sources" build phase is approved. These files, such asSettingsView.swift, `ClientCertificatesView
462-462: Removal of unused code approved!The removal of
OpenHABSettingsViewController.swiftfrom theopenHABtarget's "Sources" build phase is approved. This suggests that the settings view controller is no longer used and is replaced by the new SwiftUI-basedSettingsView.swift.To ensure the removal doesn't break any functionality, please verify that the app's settings screen works as expected and that there are no references to
OpenHABSettingsViewControllerin the codebase.Verification successful
Removal of OpenHABSettingsViewController confirmed and replaced with SwiftUI SettingsView
The verification process has confirmed that
OpenHABSettingsViewController.swifthas been successfully removed from the codebase. It has been replaced by a new SwiftUI-basedSettingsView.swift, which implements all the necessary functionality for the app's settings.Key points:
- No references to
OpenHABSettingsViewControllerwere found in the codebase.- A new
SettingsView.swiftfile has been implemented using SwiftUI.- The new settings view covers all required functionality, including connection, application, MainUI, and sitemap settings.
This change represents a positive modernization of the codebase, likely improving maintainability and user experience.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if `OpenHABSettingsViewController` is referenced anywhere in the codebase rg --type swift 'OpenHABSettingsViewController'Length of output: 47
Script:
#!/bin/bash # 1. Search for partial matches of 'OpenHABSettings' echo "Searching for partial matches of 'OpenHABSettings':" rg --type swift 'OpenHABSettings' # 2. Look for files related to settings implementation echo -e "\nSearching for files related to settings:" fd -e swift -e storyboard -e xib | rg -i 'settings?' # 3. Verify the existence and content of the new SettingsView.swift file echo -e "\nChecking for SettingsView.swift:" fd -e swift | rg 'SettingsView' echo -e "\nContent of SettingsView.swift (if exists):" fd -e swift | rg 'SettingsView' | xargs catLength of output: 15290
940-940: Project structure reorganization noted!The removal of the "UI" group reference from the
openHABtarget is noted. This suggests a reorganization of the Xcode project structure, likely moving the user interface files to a different group or location.Please verify that the new project structure is logical, maintainable, and that all UI files are still properly referenced in the project.
Verification successful
This script will search for "UI" in the project.pbxproj file, excluding "UIKIT" references, which should help us identify if and how UI files are referenced in the project structure.
UI files are properly referenced and maintained in the project
Upon thorough examination of the project.pbxproj file, it's clear that UI files are still an integral part of the project structure and build process. The "UI" group is present (line 940), and numerous UI-related files are referenced throughout the project, including custom table view cells, extensions, and utility classes. These files are properly included in the build phases, ensuring they are compiled and linked in the final product.
The initial concern about the removal of the "UI" group appears to be unfounded. The project maintains a comprehensive set of UI components and even includes UI tests (openHABUITests), indicating a continued focus on the user interface aspect of the application.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if all UI files are still referenced in the project find . -name "*.swift" -print0 | xargs -0 grep -L 'PBXBuildFile'Length of output: 8554
Script:
#!/bin/bash # Check for UI file references in the project.pbxproj file grep -n "UI" openHAB.xcodeproj/project.pbxproj | grep -v "UIKIT"Length of output: 12014
| } | ||
|
|
||
| private func loadData() { | ||
| // TODO: Replace network calls with appropriate @EnvironmentObject or other state management |
There was a problem hiding this comment.
Address the TODO comment.
The TODO comment indicates that network calls should be replaced with appropriate state management. Consider using @EnvironmentObject or other suitable state management techniques to handle data loading and updates.
Do you want me to provide guidance on how to refactor the code to use @EnvironmentObject for state management or open a GitHub issue to track this task?
Tools
SwiftLint
[Warning] 174-174: TODOs should be resolved (Replace network calls with app...)
(todo)
| Button { | ||
| let websiteDataTypes = NSSet(array: [WKWebsiteDataTypeDiskCache, WKWebsiteDataTypeMemoryCache]) | ||
| let date = Date(timeIntervalSince1970: 0) | ||
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {} |
There was a problem hiding this comment.
Avoid force cast.
The force cast at line 234 could lead to runtime crashes if the cast fails.
Consider using optional binding or optional chaining to safely unwrap the optional value. For example:
-WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {}
+if let websiteDataTypes = websiteDataTypes as? Set<String> {
+ WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes, modifiedSince: date) {}
+}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes as! Set<String>, modifiedSince: date) {} | |
| if let websiteDataTypes = websiteDataTypes as? Set<String> { | |
| WKWebsiteDataStore.default().removeData(ofTypes: websiteDataTypes, modifiedSince: date) {} | |
| } |
Tools
SwiftLint
[Error] 234-234: Force casts should be avoided
(force_cast)
…penhab#820) This reverts commit a70d740.
* Getting rid of blankicon.png as placeholder * Get rid of blankIcon
* Guards against creating very large UImages from SVGs Fixes openhab#817 Signed-off-by: Dan Cunningham <[email protected]> * remove error.png, use sys icon Signed-off-by: Dan Cunningham <[email protected]> --------- Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
digitaldan
force-pushed
the
tracker-combine-changes
branch
from
96b9cf9 to
cdcd3a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
openHAB/blankicon.pngis excluded by!**/*.pngopenHAB/error.pngis excluded by!**/*.png
Files selected for processing (4)
- OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (1 hunks)
- openHAB.xcodeproj/project.pbxproj (17 hunks)
- openHAB/OpenHABSitemapViewController.swift (11 hunks)
- openHAB/OpenHABWebViewController.swift (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- openHAB/OpenHABSitemapViewController.swift
Additional context used
SwiftLint
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift
[Warning] 17-17: TODOs should be resolved (these strings should reference...)
(todo)
Additional comments not posted (11)
OpenHABCore/Sources/OpenHABCore/Util/NetworkTracker.swift (6)
26-38: LGTM!The
ConnectionObjectstruct implementation looks good. It uses appropriate access control, conforms to theEquatableprotocol, and follows Swift naming conventions.
40-65: TheNetworkTrackerclass declaration and initialization look good.The class follows best practices and handles the initialization well:
- The class is declared as public and final, allowing it to be used outside the module and preventing subclassing.
- The class uses a singleton pattern with a private initializer to ensure only one instance exists.
- The class uses appropriate access control for its properties and methods.
- The class correctly initializes the
NWPathMonitorand starts monitoring the network path.
67-70: LGTM!The
startTrackingmethod implementation looks good. It correctly assigns the provided connection objects and attempts a connection.
72-90: LGTM!The
checkActiveServermethod implementation looks good. It correctly checks the active server's priority and reachability, and attempts a new connection if necessary.
185-194: LGTM!The
startRetryTimerandcancelRetryTimermethod implementations look good. They correctly start and cancel the retry timer usingDispatchSource.Also applies to: 197-200
202-209: LGTM!The
setActiveServerandupdateStatusmethod implementations look good. They correctly update the active server and network status.Also applies to: 211-215
openHAB/OpenHABWebViewController.swift (3)
75-84: Reactive approach using Combine improves code quality and responsiveness.The transition to a reactive model using Combine's
sinkmethod simplifies the code and enhances responsiveness to network changes. Storing the subscription intrackerCancellablesensures proper lifecycle management.
86-99: Reactive approach using Combine enhances user experience with connection status feedback.The
sinksubscription to observe changes inNetworkTracker.shared.$statusand display appropriate pop-up messages based on the connection state improves the user experience by providing timely feedback. Storing the subscription intrackerCancellablesensures proper lifecycle management.
208-208: Forcing web view reload ensures proper refresh after clearing the page.Calling
loadWebView(force: true)after clearing the existing page ensures that the web view refreshes correctly, as theforceparameter overrides the URL comparison check and guarantees a reload.openHAB.xcodeproj/project.pbxproj (2)
88-88: Review the newly added files for code quality and architecture fit.Several new Swift files have been added:
SettingsView.swiftClientCertificatesViewModel.swiftClientCertificatesView.swiftDrawerView.swiftNotificationsView.swiftColorPickerView.swiftSelectionView.swiftRTFTextView.swiftPlease review the code in each file to ensure:
- The code follows best practices and integrates well with existing patterns.
- The files have a clear purpose and fit into the overall project architecture.
Also applies to: 99-100, 103-106, 114-114
455-457: Verify the removal ofMulticastDelegate.swift,OpenHABTracker.swiftandOpenHABClientCertificatesViewController.swift.The project file indicates these Swift files have been removed from the project. Ensure this removal is intentional and the corresponding functionality is no longer needed or has been moved elsewhere.
Run the following script to check for any usage of these classes in the codebase:
Verification successful
The removal of
MulticastDelegate.swift,OpenHABTracker.swift, andOpenHABClientCertificatesViewController.swiftappears to be intentional.Based on the analysis:
- A commented-out reference to
OpenHABTrackerwas found inOpenHABWebViewController.swift, indicating intentional removal.- A
NetworkTrackerclass in the Core module might be the replacement forOpenHABTracker.- No direct replacements for
MulticastDelegateorOpenHABClientCertificatesViewControllerwere found, suggesting their functionality may have been refactored or removed.While the removal seems valid, it's recommended to:
- Review the
NetworkTrackerclass to ensure it covers the necessary functionality of the oldOpenHABTracker.- Verify that the removal of
MulticastDelegateandOpenHABClientCertificatesViewControllerdoesn't leave any gaps in the app's functionality.- Clean up the commented line in
OpenHABWebViewController.swiftfor code cleanliness.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test: Search for usage of removed classes rg --type swift -w 'MulticastDelegate|OpenHABTracker|OpenHABClientCertificatesViewController' # Expected results: # - If no results, removal seems valid. # - If results found, verify the removal and clean up references.Length of output: 198
Script:
#!/bin/bash # View the content of OpenHABWebViewController.swift around the commented line echo "Content of OpenHABWebViewController.swift:" rg --type swift -C 10 'OpenHABTracker.shared.multicastDelegate' openHAB/OpenHABWebViewController.swift echo "\nSearching for potential replacement classes:" rg --type swift -w 'class .*(Tracker|Delegate|CertificatesViewController)' echo "\nSearching for potential replacement methods:" rg --type swift -w 'func .*(track|delegate|certificates)'Length of output: 2268
| private func attemptConnection() { | ||
| guard !connectionObjects.isEmpty else { | ||
| os_log("Network status: No connection objects available.", log: OSLog.default, type: .error) | ||
| updateStatus(.notConnected) | ||
| return | ||
| } | ||
| os_log("Network status: checking available servers....", log: OSLog.default, type: .error) | ||
| let dispatchGroup = DispatchGroup() | ||
| var highestPriorityConnection: ConnectionObject? | ||
| var firstAvailableConnection: ConnectionObject? | ||
| var checkOutstanding = false // Track if there are any checks still in progress | ||
|
|
||
| let priorityWaitTime: TimeInterval = 2.0 | ||
| var priorityWorkItem: DispatchWorkItem? | ||
|
|
||
| // Set up the work item to handle the 2-second timeout | ||
| priorityWorkItem = DispatchWorkItem { [weak self] in | ||
| guard let self else { return } | ||
| // After 2 seconds, if no high-priority connection was found, check for first available connection | ||
| if let firstAvailableConnection, highestPriorityConnection == nil { | ||
| setActiveServer(firstAvailableConnection) | ||
| } else if highestPriorityConnection == nil, checkOutstanding { | ||
| os_log("Network status: No server responded in 2 seconds, waiting for checks to finish.", log: OSLog.default, type: .info) | ||
| } else { | ||
| os_log("Network status: No server responded in 2 seconds and no checks are outstanding.", log: OSLog.default, type: .error) | ||
| updateStatus(.connectionFailed) | ||
| } | ||
| } | ||
|
|
||
| // Begin checking each connection object in parallel | ||
| for connection in connectionObjects { | ||
| dispatchGroup.enter() | ||
| checkOutstanding = true // Signal that checks are outstanding | ||
|
|
||
| NetworkConnection.tracker(openHABRootUrl: connection.url) { [weak self] response in | ||
| guard let self else { return } | ||
| defer { | ||
| dispatchGroup.leave() // When each check completes, this signals the group that it's done | ||
| } | ||
|
|
||
| switch response.result { | ||
| case let .success(data): | ||
| let version = getServerInfoFromData(data: data) | ||
| if version > 0 { | ||
| if connection.priority == 0, highestPriorityConnection == nil { | ||
| // Found a high-priority (0) connection | ||
| highestPriorityConnection = connection | ||
| priorityWorkItem?.cancel() // Stop the 2-second wait if highest priority succeeds | ||
| setActiveServer(connection) | ||
| } else if highestPriorityConnection == nil { | ||
| // Check if this connection has a higher priority than the current firstAvailableConnection | ||
| if firstAvailableConnection == nil || connection.priority < firstAvailableConnection!.priority { | ||
| os_log("Network status: Found a higher priority available connection: %{PUBLIC}@", log: OSLog.default, type: .info, connection.url) | ||
| firstAvailableConnection = connection | ||
| } | ||
| } | ||
| } else { | ||
| os_log("Network status: Invalid server version from %{PUBLIC}@", log: OSLog.default, type: .error, connection.url) | ||
| } | ||
| case let .failure(error): | ||
| os_log("Network status: Failed to connect to %{PUBLIC}@ : %{PUBLIC}@", log: OSLog.default, type: .error, connection.url, error.localizedDescription) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Start a timer that waits for 2 seconds | ||
| DispatchQueue.global().asyncAfter(deadline: .now() + priorityWaitTime, execute: priorityWorkItem!) | ||
|
|
||
| // When all checks complete, finalize logic based on connection status | ||
| dispatchGroup.notify(queue: .main) { [weak self] in | ||
| guard let self else { return } | ||
|
|
||
| // All checks are finished here, so no outstanding checks | ||
| checkOutstanding = false | ||
|
|
||
| // If a high-priority connection was already established, we are done | ||
| if let highestPriorityConnection { | ||
| os_log("Network status: High-priority connection established with %{PUBLIC}@", log: OSLog.default, type: .info, highestPriorityConnection.url) | ||
| return | ||
| } | ||
|
|
||
| // If we have an available connection and no high-priority connection, set the first available | ||
| if let firstAvailableConnection { | ||
| setActiveServer(firstAvailableConnection) | ||
| os_log("Network status: First available connection established with %{PUBLIC}@", log: OSLog.default, type: .info, firstAvailableConnection.url) | ||
| } else { | ||
| os_log("Network status: No server responded, connection failed.", log: OSLog.default, type: .error) | ||
| updateStatus(.connectionFailed) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring the attemptConnection method into smaller methods for better readability and maintainability.
The method implementation is correct but quite long and complex. It could be broken down into smaller methods for better readability and maintainability.
For example, you could extract the logic for handling the priority wait time and the logic for processing the connection results into separate methods.
| private func getServerInfoFromData(data: Data) -> Int { | ||
| do { | ||
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | ||
| // OH versions 2.0 through 2.4 return "1" as their version, so set the floor to 2 so we do not think this is an OH 1.x server | ||
| return max(2, Int(serverProperties.version) ?? 2) | ||
| } catch { | ||
| return -1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the force unwrap when casting the server version to an integer.
The method implementation is correct but force unwraps the server version, which could lead to a crash if the version is not a valid integer.
Apply this diff to fix the force unwrap:
-return max(2, Int(serverProperties.version) ?? 2)
+return max(2, Int(serverProperties.version ?? "2") ?? 2)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func getServerInfoFromData(data: Data) -> Int { | |
| do { | |
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | |
| // OH versions 2.0 through 2.4 return "1" as their version, so set the floor to 2 so we do not think this is an OH 1.x server | |
| return max(2, Int(serverProperties.version) ?? 2) | |
| } catch { | |
| return -1 | |
| } | |
| } | |
| private func getServerInfoFromData(data: Data) -> Int { | |
| do { | |
| let serverProperties = try data.decoded(as: OpenHABServerProperties.self) | |
| // OH versions 2.0 through 2.4 return "1" as their version, so set the floor to 2 so we do not think this is an OH 1.x server | |
| return max(2, Int(serverProperties.version ?? "2") ?? 2) | |
| } catch { | |
| return -1 | |
| } | |
| } |
Signed-off-by: Dan Cunningham <[email protected]>
Signed-off-by: Dan Cunningham <[email protected]>
Summary by CodeRabbit
New Features
NetworkTrackerclass for improved network connectivity management.Throttlerclass for managing command updates efficiently.ColorPickerViewfor dynamic color selection and real-time updates to OpenHAB widgets.DrawerViewfor improved navigation within the application.SelectionViewfor presenting selectable mappings for OpenHAB items.Bug Fixes
Refactor
Chores