Add CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication) #30
Conversation
commit: |
|
Warning Rate limit exceeded@robertherber has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 9 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. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces modifications to three extension files: Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
Line range hint
33-69: Synchronize user defaults before executing actionsIn the
executeActionsForEventmethod, you're accessinguserDefaultswithout synchronizing preferences, which may lead to inconsistent data.Add
CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication)before accessinguserDefaults:func executeActionsForEvent(activityName: String, callbackName: String, eventName: String? = nil) { let key = eventName != nil ? "actions_for_\(activityName)_\(callbackName)_\(eventName!)" : "actions_for_\(activityName)_\(callbackName)" + CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication) let placeholders = [ "activityName": activityName, "callbackName": callbackName, "eventName": eventName ] if let actions = userDefaults?.array(forKey: key) { // Existing code... } }This ensures that you have the most recent preferences before executing actions.
🧹 Nitpick comments (2)
targets/ShieldAction/ShieldActionExtension.swift (1)
94-96: Consistent synchronization before accessing user defaultsIn the
handlemethods forApplicationToken,WebDomainToken, andActivityCategoryToken, you are callinghandleActionwithout ensuring that preferences are synchronized. AlthoughhandleActionsynchronizes preferences, it's good practice to synchronize before making significant calls.Consider synchronizing preferences at the beginning of each
handlemethod to ensure consistency:override func handle( action: ShieldAction, for application: ApplicationToken, completionHandler: @escaping (ShieldActionResponse) -> Void ) { logger.log("handle application") + CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication) handleAction( action: action, completionHandler: completionHandler, applicationToken: application, webdomainToken: nil ) }Apply similar changes to the other
handlemethods.Also applies to: 108-110, 122-124
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift (1)
50-51: Redundant synchronizationYou're synchronizing preferences in
intervalDidEndafter callingpersistToUserDefaults, which already saves touserDefaults. This may be unnecessary.Consider reviewing if the synchronization is needed here or if it can be optimized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
targets/ActivityMonitorExtension/DeviceActivityMonitorExtension.swift(1 hunks)targets/ShieldAction/ShieldActionExtension.swift(4 hunks)targets/ShieldConfiguration/ShieldConfigurationExtension.swift(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: swift-test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (1)
84-84:⚠️ Potential issueAvoid force unwrapping to prevent potential crashes
Force unwrapping with
as! Intcan lead to a runtime crash ifconfig["backgroundBlurStyle"]is not anInt.- ? UIBlurEffect.Style.init(rawValue: config["backgroundBlurStyle"] as! Int) : nil, + ? (config["backgroundBlurStyle"] as? Int).flatMap(UIBlurEffect.Style.init) : nil,
🧹 Nitpick comments (1)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (1)
55-98: Consider extracting configuration parsing logic.The
getShieldConfigurationfunction handles multiple responsibilities: preference synchronization, configuration retrieval, parsing, and shield creation. Consider extracting the configuration parsing logic into a separate function for better maintainability and testability.Example structure:
private func parseShieldConfiguration(_ config: [String: Any], placeholders: [String: String?]) -> ShieldConfiguration { // Move parsing logic here } func getShieldConfiguration(placeholders: [String: String?]) -> ShieldConfiguration { CFPreferencesAppSynchronize(kCFPreferencesCurrentApplication) guard let config = userDefaults?.dictionary(forKey: "shieldConfiguration") else { return ShieldConfiguration() } return parseShieldConfiguration(config, placeholders: placeholders) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: swift-test
🔇 Additional comments (3)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (3)
60-60: LGTM: Proper preference synchronization added.Good addition of
CFPreferencesAppSynchronizeto ensure configuration is up-to-date before access.
99-100: LGTM: Proper fallback handling added.Good addition of a default shield configuration when no configuration is found in user defaults.
121-121: LGTM: Consistent implementation across configuration methods.The changes to use the simplified
getShieldConfigurationfunction are applied consistently across all configuration methods while maintaining proper placeholder handling for each context.Also applies to: 140-140, 157-157, 176-176
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (2)
62-81: Enhance error handling and logging for configuration retrieval.While the configuration retrieval works, consider adding more detailed logging to help diagnose issues when configurations are missing or invalid.
if let config = userDefaults?.dictionary(forKey: "shieldConfiguration") { + logger.log("Shield configuration found with \(config.count) keys") let backgroundColor = getColor(color: config["backgroundColor"] as? [String: Double]) + if backgroundColor == nil { + logger.log("Background color configuration is missing or invalid") + } // ... rest of the configuration parsing } else { + logger.log("No shield configuration found in userDefaults") }
82-98: Consider validating required configuration fields.While the code handles optional fields well, consider adding validation for any fields that should be required for a valid shield configuration. This would help catch configuration issues early.
let shield = ShieldConfiguration( backgroundBlurStyle: config["backgroundBlurStyle"] != nil ? (config["backgroundBlurStyle"] as? Int).flatMap(UIBlurEffect.Style.init) : nil, backgroundColor: backgroundColor, icon: resolveIcon(dict: config), title: buildLabel(text: title, with: titleColor, placeholders: placeholders), subtitle: buildLabel(text: subtitle, with: subtitleColor, placeholders: placeholders), primaryButtonLabel: buildLabel( text: primaryButtonLabel, with: primaryButtonLabelColor, placeholders: placeholders), primaryButtonBackgroundColor: primaryButtonBackgroundColor, secondaryButtonLabel: buildLabel( text: secondaryButtonLabel, with: secondaryButtonLabelColor, placeholders: placeholders) ) +// Validate required fields +guard shield.title != nil else { + logger.log("Warning: Shield configuration created without a title") + // Consider if this should throw an error instead + return ShieldConfiguration() +} logger.log("shield initialized")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: swift-test
🔇 Additional comments (3)
targets/ShieldConfiguration/ShieldConfigurationExtension.swift (3)
55-61: LGTM! Good improvements to preference synchronization and function signature.The changes improve the code by:
- Simplifying the function signature
- Adding preference synchronization before accessing userDefaults
121-121: LGTM! Consistent implementation across configuration methods.The changes maintain consistency in how shield configurations are retrieved across different contexts (applications, web domains, and categories).
Also applies to: 140-140, 157-157, 176-176
Line range hint
55-176: Consider adding unit tests for the refactored configuration logic.The changes look good, but it would be valuable to add unit tests to verify:
- Preference synchronization behavior
- Default configuration fallback
- Configuration parsing with various input scenarios
- Placeholder substitution in different contexts
Would you like me to help create a test suite for these scenarios?
Needs testing - but hopefully ensures that shield config/actions are available when triggered.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor