refactor: Enhance ContentBlocker with improved error handling, memory management, and session management #149
Conversation
… management, and session management - Introduced BlockerConfig interface for better configuration handling. - Replaced array with Set for blocked sessions to optimize performance. - Added error handling in blocker instance creation and session management methods. - Implemented cleanup method to handle resource deallocation on app shutdown. - Updated initialization process to ensure proper event listener binding.
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant ContentBlocker
participant Session
participant ElectronBlocker
App->>ContentBlocker: initialize()
ContentBlocker->>ContentBlocker: Check isInitialized
ContentBlocker->>ContentBlocker: Set up event listeners
ContentBlocker->>ContentBlocker: Mark as initialized
App->>ContentBlocker: updateConfig()
ContentBlocker->>ContentBlocker: Debounce update
ContentBlocker->>ContentBlocker: getBlockerConfig()
alt enabled
ContentBlocker->>ElectronBlocker: createBlockerInstance()
loop for each Session
ContentBlocker->>Session: enableBlockerForSession()
end
else not enabled
ContentBlocker->>Session: disableBlocker()
end
App->>ContentBlocker: cleanup() (on beforeExit)
ContentBlocker->>Session: disableBlocker()
ContentBlocker->>ContentBlocker: Clear timers, reset state
Possibly related PRs
Poem
✨ 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 (
|
Build artifacts for all platforms are ready! 🚀Download the artifacts from: (execution 16326139839 / attempt 1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/modules/content-blocker.ts (1)
205-224: Consider more granular initialization state trackingThe
isInitializedflag is set to true even if some initialization steps fail. Consider only setting it after all steps succeed, or implementing more granular state tracking:public async initialize(): Promise<void> { if (this.isInitialized) return; try { // Initial configuration await this.updateConfig(); // Listen for setting changes settingsEmitter.on("settings-changed", this.handleSettingsChanged); // Listen for profile changes browser?.on("profile-loaded", this.handleProfileLoaded); browser?.on("profile-unloaded", this.handleProfileRemoved); - this.isInitialized = true; debugPrint("CONTENT_BLOCKER", "Content blocker initialized successfully"); + this.isInitialized = true; } catch (error) { debugPrint("CONTENT_BLOCKER", "Failed to initialize content blocker:", error); + // Consider cleanup of partially initialized state + await this.cleanup(); + throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/modules/content-blocker.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/modules/content-blocker.ts (4)
src/main/modules/output.ts (1)
debugPrint(28-34)src/main/browser/utility/web-requests.ts (1)
createBetterSession(780-818)src/main/saving/settings.ts (2)
getSettingValueById(54-56)settingsEmitter(12-12)src/main/index.ts (1)
browser(15-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (macos-latest)
- GitHub Check: format
- GitHub Check: main-checks
🔇 Additional comments (8)
src/main/modules/content-blocker.ts (8)
12-15: Clean interface design for configurationThe
BlockerConfiginterface provides clear type safety and structure for managing blocker configurations.
24-26: Excellent performance optimization with SetUsing
Set<Session>instead of an array provides O(1) lookups and automatic uniqueness guarantees, which is perfect for session tracking.
84-88: Robust concurrent session handlingExcellent use of
Promise.allSettledto ensure all sessions are processed even if some operations fail. The conversion from Set to Array is clean and efficient.
107-115: Efficient session management with duplicate preventionGood optimization to check if the session is already blocked before attempting to enable blocking again. The error handling ensures the method is resilient to failures.
125-139: Well-implemented session removal methodThe method properly handles individual session removal with appropriate guards and error handling. Good separation of concerns.
144-155: Clean configuration encapsulationGood separation of configuration logic into a dedicated method. The default case appropriately returns a disabled state.
237-244: Add null check for profile lookupThe profile lookup might return null/undefined. Add a check before accessing the session:
private handleProfileRemoved = (profileId: string) => { // Find the session for this profile and remove it if (!browser) return; const profile = browser.getLoadedProfile(profileId); - if (profile) { + if (profile?.session) { this.removeSession(profile.session); } };Likely an incorrect or invalid review comment.
257-259: Ensure cleanup completes before process exitThe
beforeExitevent might not wait for the async cleanup to complete. Consider using a synchronous wrapper or theexitevent:// Handle app shutdown -process.on("beforeExit", async () => { - await contentBlocker.cleanup(); +process.on("beforeExit", (code) => { + contentBlocker.cleanup().then(() => { + process.exit(code); + }).catch((error) => { + console.error("Failed to cleanup content blocker:", error); + process.exit(code); + }); });Alternatively, consider making the cleanup synchronous or using a shutdown manager that properly handles async cleanup.
Likely an incorrect or invalid review comment.
| const blocker = await promise; | ||
| blocker.on("request-blocked", (request) => { | ||
| debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url); | ||
| }); |
There was a problem hiding this comment.
Potential memory leak: Event listeners not cleaned up
The event listener is attached to the blocker instance but never removed. If createBlockerInstance is called multiple times (e.g., when switching blocker types), old blocker instances may not be garbage collected due to lingering event listeners.
Consider storing a reference to remove the listener when creating a new instance:
private async createBlockerInstance(type: BlockerInstanceType): Promise<ElectronBlocker> {
if (this.blockerInstancePromise && this.blockerInstanceType === type) {
return this.blockerInstancePromise;
}
if (this.blockerInstancePromise) {
+ // Clean up previous blocker instance
+ const previousBlocker = await this.blockerInstancePromise;
+ previousBlocker.removeAllListeners();
await this.disableBlocker();
}📝 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.
| const blocker = await promise; | |
| blocker.on("request-blocked", (request) => { | |
| debugPrint("CONTENT_BLOCKER", "Request blocked:", request.url); | |
| }); | |
| private async createBlockerInstance(type: BlockerInstanceType): Promise<ElectronBlocker> { | |
| if (this.blockerInstancePromise && this.blockerInstanceType === type) { | |
| return this.blockerInstancePromise; | |
| } | |
| if (this.blockerInstancePromise) { | |
| + // Clean up previous blocker instance | |
| + const previousBlocker = await this.blockerInstancePromise; | |
| + previousBlocker.removeAllListeners(); | |
| await this.disableBlocker(); | |
| } | |
| // ... rest of the method unchanged | |
| } |
🤖 Prompt for AI Agents
In src/main/modules/content-blocker.ts around lines 60 to 63, the event listener
added to the blocker instance is never removed, which can cause a memory leak if
createBlockerInstance is called multiple times. To fix this, store a reference
to the event listener function and ensure you remove it using blocker.off or
equivalent before replacing or discarding the blocker instance. This cleanup
will allow old blocker instances to be garbage collected properly.
| if (!browser) return; | ||
|
|
||
| // Debounce rapid configuration changes | ||
| if (this.updateTimeout) { | ||
| clearTimeout(this.updateTimeout); | ||
| } | ||
|
|
||
| this.updateTimeout = setTimeout(async () => { | ||
| try { | ||
| const config = this.getBlockerConfig(); | ||
| const profiles = browser?.getLoadedProfiles() ?? []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving browser null check inside timeout callback
The browser null check at line 161 might become stale by the time the debounced callback executes. Consider moving it inside the timeout callback:
public async updateConfig(): Promise<void> {
- if (!browser) return;
-
// Debounce rapid configuration changes
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
}
this.updateTimeout = setTimeout(async () => {
try {
+ if (!browser) return;
const config = this.getBlockerConfig();
const profiles = browser?.getLoadedProfiles() ?? [];📝 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.
| if (!browser) return; | |
| // Debounce rapid configuration changes | |
| if (this.updateTimeout) { | |
| clearTimeout(this.updateTimeout); | |
| } | |
| this.updateTimeout = setTimeout(async () => { | |
| try { | |
| const config = this.getBlockerConfig(); | |
| const profiles = browser?.getLoadedProfiles() ?? []; | |
| public async updateConfig(): Promise<void> { | |
| // Debounce rapid configuration changes | |
| if (this.updateTimeout) { | |
| clearTimeout(this.updateTimeout); | |
| } | |
| this.updateTimeout = setTimeout(async () => { | |
| try { | |
| if (!browser) return; | |
| const config = this.getBlockerConfig(); | |
| const profiles = browser?.getLoadedProfiles() ?? []; | |
| // …rest of logic… | |
| } catch (e) { | |
| // …error handling… | |
| } | |
| }, 100); | |
| } |
🤖 Prompt for AI Agents
In src/main/modules/content-blocker.ts around lines 161 to 171, the null check
for the browser variable is done before setting the debounce timeout, which may
become outdated when the callback runs. Move the browser null check inside the
setTimeout callback function to ensure the browser is still valid when the
debounced code executes.
| public async cleanup(): Promise<void> { | ||
| if (this.updateTimeout) { | ||
| clearTimeout(this.updateTimeout); | ||
| this.updateTimeout = undefined; | ||
| } | ||
|
|
||
| await this.disableBlocker(); | ||
| this.isInitialized = false; | ||
| } |
There was a problem hiding this comment.
Missing event listener cleanup
The cleanup method should also remove the event listeners that were added during initialization to prevent memory leaks and unexpected behavior:
public async cleanup(): Promise<void> {
if (this.updateTimeout) {
clearTimeout(this.updateTimeout);
this.updateTimeout = undefined;
}
+ // Remove event listeners
+ settingsEmitter.off("settings-changed", this.handleSettingsChanged);
+ browser?.off("profile-loaded", this.handleProfileLoaded);
+ browser?.off("profile-unloaded", this.handleProfileRemoved);
+
await this.disableBlocker();
this.isInitialized = false;
}📝 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 async cleanup(): Promise<void> { | |
| if (this.updateTimeout) { | |
| clearTimeout(this.updateTimeout); | |
| this.updateTimeout = undefined; | |
| } | |
| await this.disableBlocker(); | |
| this.isInitialized = false; | |
| } | |
| public async cleanup(): Promise<void> { | |
| if (this.updateTimeout) { | |
| clearTimeout(this.updateTimeout); | |
| this.updateTimeout = undefined; | |
| } | |
| // Remove event listeners | |
| settingsEmitter.off("settings-changed", this.handleSettingsChanged); | |
| browser?.off("profile-loaded", this.handleProfileLoaded); | |
| browser?.off("profile-unloaded", this.handleProfileRemoved); | |
| await this.disableBlocker(); | |
| this.isInitialized = false; | |
| } |
🤖 Prompt for AI Agents
In src/main/modules/content-blocker.ts around lines 192 to 200, the cleanup
method currently clears the updateTimeout and disables the blocker but does not
remove event listeners added during initialization. To fix this, identify all
event listeners registered in the initialization phase and explicitly remove
them in the cleanup method to prevent memory leaks and unexpected behavior.
Summary by CodeRabbit