fix: explicitly set easylist url #248
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (3)
maxun-core/src/interpret.ts (3)
Line range hint
105-109: Enhance error handling in ad-blocker initialization.The error handling should include the error message for better debugging.
PlaywrightBlocker.fromLists(fetch, ['https://easylist.to/easylist/easylist.txt']).then(blocker => { this.blocker = blocker; }).catch(err => { - this.log(`Failed to initialize ad-blocker:`, Level.ERROR); + this.log(`Failed to initialize ad-blocker: ${err.message}`, Level.ERROR); })
Line range hint
146-164: Review the selector collection logic.
- The early return after finding the first set of selectors might miss important selectors from earlier actions.
- The commented-out old implementation should be removed.
Consider this implementation to collect all unique selectors:
private getSelectors(workflow: Workflow): string[] { const selectorsSet = new Set<string>(); if (workflow.length === 0) { return []; } for (let index = workflow.length - 1; index >= 0; index--) { const currentSelectors = workflow[index]?.where?.selectors; if (currentSelectors && currentSelectors.length > 0) { - currentSelectors.forEach((selector) => selectorsSet.add(selector)); - return Array.from(selectorsSet); + currentSelectors.forEach((selector) => selectorsSet.add(selector)); } } - return []; + return Array.from(selectorsSet); }
Line range hint
117-145: Remove commented code.The old implementation of
getSelectorsshould be removed as it's no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
maxun-core/src/interpret.ts(1 hunks)server/src/browser-management/classes/RemoteBrowser.ts(1 hunks)
| const blocker = await PlaywrightBlocker.fromLists(fetch, ['https://easylist.to/easylist/easylist.txt']); | ||
| await blocker.enableBlockingInPage(this.currentPage); | ||
| this.client = await this.currentPage.context().newCDPSession(this.currentPage); | ||
| await blocker.disableBlockingInPage(this.currentPage); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the ad-blocker initialization.
- The EasyList URL should be configurable rather than hardcoded to allow for easy updates and maintenance.
- The blocker is enabled and immediately disabled in sequence, which seems unnecessary.
Consider these improvements:
- const blocker = await PlaywrightBlocker.fromLists(fetch, ['https://easylist.to/easylist/easylist.txt']);
+ const EASYLIST_URL = process.env.EASYLIST_URL || 'https://easylist.to/easylist/easylist.txt';
+ try {
+ const blocker = await PlaywrightBlocker.fromLists(fetch, [EASYLIST_URL]);
+ await blocker.enableBlockingInPage(this.currentPage);
+ } catch (error) {
+ logger.log('error', `Failed to initialize ad-blocker: ${error.message}`);
+ }Committable suggestion skipped: line range outside the PR's diff.
ref: #207, #126