Revert content-blocker target redistribution and restore extension display names#209
Revert content-blocker target redistribution and restore extension display names#209
Conversation
Summary of ChangesHello @0xCUB3, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request reverts a recent change that introduced a complex filter redistribution mechanism across multiple content-blocker targets and assigned generic, numbered display names to extensions. The primary goal is to restore the prior, simpler behavior where filters are mapped to extensions by category, ensuring that extension names are descriptive within the Xcode project. This change also simplifies the filter processing and reloading logic, aiming for better maintainability and compatibility with existing protobuf storage and extension mapping. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively reverts the previous content-blocker target redistribution logic and restores descriptive display names for extensions. The changes simplify the filter application process by mapping filters to extensions based on their category, which is a welcome improvement for maintainability. The updates to the Xcode project and Info.plist files correctly restore the human-readable names.
I've identified a couple of areas for improvement. Firstly, there appears to be a misconfiguration in the Xcode project file for the 'wBlock Privacy' macOS target, which I've flagged as a high-severity issue. Secondly, the new logic for handling rule-limit overflows in AppFilterManager could be made more memory-efficient and less duplicative by reusing an existing helper function. My review includes a suggestion for this refactoring. Addressing these points will improve the correctness and robustness of the implementation.
| GENERATE_INFOPLIST_FILE = YES; | ||
| INFOPLIST_FILE = "wBlock Ads/Info.plist"; | ||
| INFOPLIST_KEY_CFBundleDisplayName = "wBlock 2"; | ||
| INFOPLIST_KEY_CFBundleDisplayName = "wBlock Privacy"; |
There was a problem hiding this comment.
While you're correctly updating the display name to be descriptive, it seems the wBlock Privacy target (for macOS) is incorrectly configured to use the Info.plist from the wBlock Ads target (see line 2831: INFOPLIST_FILE = "wBlock Ads/Info.plist";).
This is likely a copy-paste error and could lead to incorrect bundle information for the Privacy extension on macOS. Each target should have its own Info.plist file. Please verify and correct the INFOPLIST_FILE path for this target.
| GENERATE_INFOPLIST_FILE = YES; | ||
| INFOPLIST_FILE = "wBlock Ads/Info.plist"; | ||
| INFOPLIST_KEY_CFBundleDisplayName = "wBlock 2"; | ||
| INFOPLIST_KEY_CFBundleDisplayName = "wBlock Privacy"; |
There was a problem hiding this comment.
Similar to the Debug configuration, the wBlock Privacy target's Release configuration also seems to be pointing to the wrong Info.plist file (wBlock Ads/Info.plist on line 2863). This should be corrected to use a plist specific to the Privacy target to ensure correct bundle information in release builds.
| // Re-process this target with the reset filters - on background thread | ||
| let disabledSites = self.dataManager.disabledSites | ||
| let resetResult = await Task.detached { | ||
| let containerURL = await MainActor.run { self.loader.getSharedContainerURL() } | ||
| var resetRulesChunks: [String] = [] | ||
|
|
||
| // Auto-reset categories assigned to this target and warn the user | ||
| let affectedCategories = categoriesForTarget.filter { $0 != .all } | ||
| for category in affectedCategories { | ||
| await resetCategoryToRecommended(category) | ||
| // Get the updated filter list after reset | ||
| let updatedSelectedFilters = await MainActor.run { | ||
| self.filterLists.filter { $0.isSelected } | ||
| } | ||
|
|
||
| await MainActor.run { | ||
| self.showTargetWarning(targetName: targetDisplayName(for: targetInfo), categories: affectedCategories) | ||
| self.showingAutoDisabledAlert = true | ||
| resetRulesChunks.reserveCapacity(updatedSelectedFilters.count) | ||
|
|
||
| for filter in updatedSelectedFilters { | ||
| if filter.category == targetInfo.primaryCategory | ||
| || (targetInfo.secondaryCategory != nil | ||
| && filter.category == targetInfo.secondaryCategory!) | ||
| { | ||
| guard let containerURL = containerURL else { continue } | ||
| let fileURL = containerURL.appendingPathComponent( | ||
| self.loader.filename(for: filter) | ||
| ) | ||
| if FileManager.default.fileExists(atPath: fileURL.path) { | ||
| do { | ||
| resetRulesChunks.append(try String(contentsOf: fileURL, encoding: .utf8)) | ||
| } catch { | ||
| await ConcurrentLogManager.shared.error( | ||
| .filterApply, "Error reading filter after reset", | ||
| metadata: ["filter": filter.name, "error": "\(error)"]) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| await ConcurrentLogManager.shared.info( | ||
| .filterApply, "Auto-reset categories due to rule limit exceeded", | ||
| metadata: ["target": targetDisplayName(for: targetInfo)]) | ||
|
|
||
| didRedistribute = true | ||
| break | ||
| let resetRulesString = resetRulesChunks.joined(separator: "\n") | ||
| return ContentBlockerService.convertFilter( | ||
| rules: resetRulesString.isEmpty ? "[]" : resetRulesString, | ||
| groupIdentifier: GroupIdentifier.shared.value, | ||
| targetRulesFilename: targetInfo.rulesFilename, | ||
| disabledSites: disabledSites | ||
| ) | ||
| }.value |
There was a problem hiding this comment.
The logic to re-process filters after a rule limit is exceeded duplicates functionality from convertFiltersMemoryEfficient but in a less memory-safe way. It reads all filter files into an in-memory string (resetRulesString), which defeats the purpose of the memory-efficient streaming approach used elsewhere.
This could be refactored to reuse convertFiltersMemoryEfficient for better maintainability and to avoid potential memory issues with large filter lists. You can get the updated list of filters for the current target and pass them to convertFiltersMemoryEfficient.
// Re-process this target with the reset filters.
let disabledSites = self.dataManager.disabledSites
let updatedFiltersForTarget = await MainActor.run {
self.filterLists.filter {
guard $0.isSelected else { return false }
let filterTargetInfo = ContentBlockerTargetManager.shared.targetInfo(
forCategory: $0.category,
platform: self.currentPlatform
)
return filterTargetInfo?.bundleIdentifier == targetInfo.bundleIdentifier
}
}
let resetResult = await Task.detached {
return await self.convertFiltersMemoryEfficient(
filters: updatedFiltersForTarget,
targetInfo: targetInfo,
disabledSites: disabledSites
)
}.value![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee942a4150
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await MainActor.run { | ||
| self.ruleCountsByExtension[targetInfo.bundleIdentifier] = resetRuleCount | ||
| self.extensionsApproachingLimit.remove(targetInfo.bundleIdentifier) | ||
| } | ||
|
|
There was a problem hiding this comment.
After the auto-reset path, the new resetRuleCount is accepted and the “approaching limit” flag is cleared without verifying it actually fell below the 150k limit. If the recommended lists (plus disabled-site rules) still exceed the limit, the app will proceed as if the rules are valid and may attempt reloads that Safari will reject or silently drop, leaving users with a broken blocker but no error. Consider re-validating resetRuleCount against ruleLimit (and re-setting extensionsApproachingLimit accordingly) or aborting with a user-visible error when the reset is still over limit.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
distributeFiltersToTargetsandtargetDisplayName), instead assigning filters usingContentBlockerTargetManager.targetInfo(forCategory:platform:).AppFilterManagerto process filters per target category, updateruleCountsByExtension, and handle rule-limit conditions by resetting affected categories to recommended lists and reprocessing.targetnames tocategory/bundleId, and adjusted retry/reload helpers to usetargetInfo.primaryCategory.rawValue.INFOPLIST_KEY_CFBundleDisplayName) and removed the numberedCFBundleDisplayName/CFBundleNameentries introduced earlier in several extensionInfo.plistfiles.Testing
Codex Task