Add Built-in Adblocker #42
Conversation
WalkthroughA built-in content blocker feature was integrated into the application. This involved updating dependencies, adding a new settings option for content blocking, introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant ContentBlocker
participant ElectronSession
participant Profiles
User->>Settings: Change "Content Blocker" setting
Settings->>ContentBlocker: Emit settings change event
ContentBlocker->>ContentBlocker: updateConfig()
ContentBlocker->>Profiles: Get all loaded profiles
loop For each profile session
ContentBlocker->>ElectronSession: Enable/disable blocker as per setting
end
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🧹 Nitpick comments (3)
src/main/modules/basic-settings.ts (1)
28-54: Consider adding documentation about the blocking levels.While the options are clear, users might benefit from more detailed information about what each blocking level entails, especially the difference between "Block Ads & Trackers" and "Block All".
Consider adding tooltip documentation or a help link that explains each blocking level in more detail, particularly what additional elements are blocked in the "Block All" option beyond ads and trackers.
src/main/modules/content-blocker.ts (2)
71-83: Unnecessary null check for blockerThe check
if (!blocker) return;on line 73 is unnecessary sincecreateBlockerInstancealways returns a Promise that resolves to anElectronBlocker. According to the method signature and implementation, it can't return null or undefined.private async enableBlockerForSession(blockerType: BlockerInstanceType, session: Session): Promise<void> { const blocker = await this.createBlockerInstance(blockerType); - if (!blocker) return; // check if session is already blocked if (this.blockedSessions.includes(session)) return;
55-66: Consider optimizing blocker type changesWhen changing blocker types, the current implementation disables blocking on all sessions before creating a new blocker instance. This could lead to a brief period where content isn't blocked.
Consider creating the new blocker instance first, then switching sessions from the old blocker to the new one to minimize the time window where content isn't blocked:
private async createBlockerInstance(type: BlockerInstanceType): Promise<ElectronBlocker> { if (this.blockerInstancePromise && this.blockerInstanceType === type) { return this.blockerInstancePromise; } + const oldBlockerPromise = this.blockerInstancePromise; + const oldBlockedSessions = [...this.blockedSessions]; + this.blockerInstancePromise = undefined; + this.blockerInstanceType = undefined; + this.blockedSessions = []; + + debugPrint("CONTENT_BLOCKER", "Creating blocker instance:", type); + // Create the new blocker instance + switch (type) { + case "all": + this.blockerInstancePromise = ElectronBlocker.fromPrebuiltFull(); + break; + case "adsAndTrackers": + this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsAndTracking(); + break; + case "adsOnly": + this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsOnly(); + break; + } + + // Set up the event listener for the new blocker + this.blockerInstancePromise.then((blocker) => { + blocker.on("request-blocked", (request) => { + debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url); + }); + }).catch(error => { + debugPrint("CONTENT_BLOCKER", "Error creating blocker instance:", error); + }); + + this.blockerInstanceType = type; + + // If there was an old blocker, transition sessions to the new one + if (oldBlockerPromise) { + try { + const [oldBlocker, newBlocker] = await Promise.all([ + oldBlockerPromise, + this.blockerInstancePromise + ]); + + // Enable blocking on the new blocker for all sessions + for (const session of oldBlockedSessions) { + newBlocker.enableBlockingInSession(session); + this.blockedSessions.push(session); + } + + // Disable the old blocker after enabling the new one + for (const session of oldBlockedSessions) { + oldBlocker.disableBlockingInSession(session); + } + } catch (error) { + debugPrint("CONTENT_BLOCKER", "Error transitioning between blockers:", error); + + // If transition fails, disable old blocker and return the new one + if (oldBlockerPromise) { + oldBlockerPromise.then(oldBlocker => { + for (const session of oldBlockedSessions) { + oldBlocker.disableBlockingInSession(session); + } + }).catch(error => { + debugPrint("CONTENT_BLOCKER", "Error disabling old blocker:", error); + }); + } + } + } + + return this.blockerInstancePromise as Promise<ElectronBlocker>; - if (this.blockerInstancePromise) { - await this.disableBlocker(); - } - - debugPrint("CONTENT_BLOCKER", "Creating blocker instance:", type); - switch (type) { - case "all": - this.blockerInstancePromise = ElectronBlocker.fromPrebuiltFull(); - break; - case "adsAndTrackers": - this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsAndTracking(); - break; - case "adsOnly": - this.blockerInstancePromise = ElectronBlocker.fromPrebuiltAdsOnly(); - break; - } - - this.blockerInstancePromise.then((blocker) => { - blocker.on("request-blocked", (request) => { - debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url); - }); - }); - - this.blockerInstanceType = type; - return this.blockerInstancePromise as Promise<ElectronBlocker>; }Note: This is a more complex implementation and might be overkill depending on your use case, but it ensures minimal blocking interruption during blocker type changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
package.json(1 hunks)src/main/index.ts(1 hunks)src/main/modules/basic-settings.ts(2 hunks)src/main/modules/content-blocker.ts(1 hunks)src/main/modules/output.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/modules/content-blocker.ts (3)
src/main/modules/output.ts (1)
debugPrint(23-29)src/main/index.ts (1)
browser(14-14)src/main/saving/settings.ts (3)
getSettingValueById(54-56)settingsEmitter(12-12)onSettingsCached(51-51)
🔇 Additional comments (9)
src/main/index.ts (1)
11-11: Good integration of the new content blocker module.The import follows the established pattern for module imports in this file. The side-effect import is appropriate for initializing the content blocker when the application starts.
src/main/modules/output.ts (1)
17-18: Good addition of debug area for the content blocker.You've properly added a new debug area for the content blocker and fixed the missing comma in the previous line. This follows the established pattern in the codebase.
src/main/modules/basic-settings.ts (3)
28-54: Well-structured setting with appropriate default and options.The content blocker setting is well-defined with clear naming and a comprehensive set of options that provide different levels of blocking. Setting the default to "disabled" is a sensible choice to avoid unexpected behavior for users.
158-158: Good placement in General Settings card.The content blocker setting is appropriately added to the General Settings card between autoUpdate and the internal default browser setting.
28-54:✅ Verification successful
Verify missing module implementation.
I don't see the implementation file
src/main/modules/content-blocker.tsin this PR. Make sure this file is included and properly implements the logic for each of the blocking levels defined in the settings.
🏁 Script executed:
#!/bin/bash # Check if the content-blocker.ts file exists if [ -f src/main/modules/content-blocker.ts ]; then echo "Content blocker module exists" # Show the implementation to verify it handles all the blocking modes cat src/main/modules/content-blocker.ts else echo "Content blocker module not found at src/main/modules/content-blocker.ts" fiLength of output: 4343
Content Blocker Module Verified
The
src/main/modules/content-blocker.tsfile exists and correctly implements all configured blocking levels (all,adsAndTrackers,adsOnly) with a fallback to disable blocking. No further action needed.src/main/modules/content-blocker.ts (4)
1-6: Imports are well-organizedThe imports are clean and properly structured, with appropriate dependencies from both internal modules and external packages.
7-8: Well-defined content blocker typesGood use of TypeScript type to define the three supported blocker modes, making the code more maintainable and type-safe.
12-16: Class properties are well-designedThe private properties provide good encapsulation of the blocker state. Using
undefinedfor the initial state of the blocker instance and type is clear and explicit.
1-138: Overall excellent implementation of content blockingThe ContentBlocker class is well-designed with clear separation of concerns, good encapsulation, and thorough documentation. The singleton pattern is appropriate for this functionality, and the integration with the application's settings and browser profiles is well-implemented.
The code effectively leverages the Ghostery adblocker library to provide three different blocking modes and dynamically updates the blocking configuration based on user settings and profile changes.
Summary by CodeRabbit
New Features
Chores