Feat omniblock#4533
Open
Dev-NotAqua wants to merge 1 commit into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new ChangesOmniblock Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/plugins/omniblock/style.css (3)
32-33: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBroad wildcard selectors may cause false positives on non-YouTube sites.
[class*="sponsor" i]and[class*="ad-badge" i]will match any element containing those substrings on any domain. Consider scoping to known YouTube domains or using more specific selectors to reduce collateral hiding on sites wheresponsororad-badgeclasses have unrelated meanings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/omniblock/style.css` around lines 32 - 33, The broad wildcard selectors in the omniblock CSS are causing unintended matches outside YouTube. Tighten the selectors in the style rule that uses [class*="sponsor" i] and [class*="ad-badge" i] by scoping them to known YouTube domains or replacing them with more specific YouTube-only selectors so unrelated elements on other sites are not hidden.
51-57: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSkip button visibility rule may not override hidden parent containers.
.ytp-ad-skip-button-containercannot become visible if it's nested within any of the hidden parent selectors (e.g.,.ytp-ad-module,.ytp-ad-player-overlay). Thedisplay: block !importanton a child does not overridedisplay: none !importanton an ancestor. Ensure the JavaScriptsetupAdSkipperhandles programmatic clicking when the natural UI is hidden by ancestor rules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/omniblock/style.css` around lines 51 - 57, The `.ytp-ad-skip-button-container` CSS rule in the omniblock styles cannot override hidden ancestor containers such as `.ytp-ad-module` or `.ytp-ad-player-overlay`, so the skip button may still be unreachable. Update `setupAdSkipper` to detect when the natural skip UI is hidden by parent rules and trigger programmatic clicking of the skip action instead of relying only on visibility styling. Keep the existing selector logic in the ad-skipping path, but make the JS fallback handle cases where the container is nested inside hidden parents.
1-49: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant hiding properties are defensive but consider
contain: strictfor performance.The 9-property fallback stack is standard for ad blockers. For additional performance on pages with many matched elements, consider adding
contain: strict !important;to isolate the hidden subtrees from layout/style recalculation. This is optional and may be overkill for video player UIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugins/omniblock/style.css` around lines 1 - 49, The ad-hiding rule in the omniblock stylesheet already uses a broad fallback stack, but it can be made more performance-friendly by isolating matched nodes from layout/style work. Update the existing selector block in the omniblock CSS to add a containment rule on the same hidden elements, keeping the current hiding properties intact while applying the extra isolation only in this shared rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/omniblock/blocker.ts`:
- Around line 99-115: Serialize initialization in blocker.ts so only one
ElectronBlocker.fromLists() call can be in flight at a time. The current
backend.start / onConfigChange path can enter the `blocker` setup block
concurrently before `blocker` is assigned, creating multiple enabled instances.
Add an initialization guard or shared promise around the `blocker`
creation/enabling logic in the `blocker` setup flow so subsequent callers reuse
the same pending instance instead of starting a new one.
In `@src/plugins/omniblock/index.ts`:
- Around line 228-231: The renderer startup path in start, along with
updateCosmeticState and setupAdSkipper, only checks sub-feature flags and can
still run when the top-level plugin is disabled. Add the enabled gate to the
feature checks in these lifecycle paths so page mutation, cosmetic hiding, and
the ad-skip loop only run when the plugin is fully enabled. Use the existing
symbols start, updateCosmeticState, setupAdSkipper, and the related renderer
feature gate logic to locate and update the conditions.
- Around line 355-373: The per-video play() patch in patchVideo is not gated by
the current plugin state and the original method is never restored, so
already-patched video elements can keep the ad-skipping behavior after
disable/stop. Update patchVideo so the override checks the current config
(enabled and adSkipper) before muting/seeking, keep a per-element reference to
the original play implementation on the PatchedVideoElement, and make stop()
iterate patched videos to restore their original play method and clear the
patched flag.
- Around line 24-39: The global pruning logic in omniblock is too broad and
removes generic fields from every parsed payload, which can corrupt non-ad data.
Update the JSON.parse/Response patch handling in index.ts so the adKeys-based
deletion only runs for known ad/YouTube ad response URLs or other clearly
identified ad payloads, and keep the generic keys like trackingParams, banner,
clientMessages, and engagementPanel out of unrestricted global filtering. Use
the existing adKeys set and the patching hooks to apply schema-aware or
URL-scoped pruning instead of blanket deletion.
- Line 171: The omniblock plugin’s JSON pruning flag is treated as live, but the
preload injection only happens once and cannot remove or add the global proxies
after startup. Update the lifecycle handling in the omniblock config path so
`restartNeeded`/reload behavior matches `config.enabled` and `jsonPruning`, or
make the injected script in the preload path (`jsonPruning` handling and the
corresponding injection logic) fully config-aware; ensure toggling either option
is reflected by requiring a restart/reload rather than implying hot switching.
- Around line 267-272: The ad-dismissal logic in
`src/plugins/omniblock/index.ts` is treating advertiser CTAs as skip controls
and the visibility gate in the button-click path is too permissive. Update the
selector used in the skip-button loop so it only targets actual skip/close
controls and remove `.ytp-ad-visit-advertiser-button` from that set, then
tighten the click guard in the related visibility/click check around the button
handling so it doesn’t fall back to `typeof button.click === 'function'` for
every `HTMLElement`. Make the filtering in the same skip-button handling flow
rely on real visibility/role-based conditions before invoking `button.click()`.
- Around line 143-146: The blocked XHR handling in XMLHttpRequest.prototype.open
is incorrect because calling abort() there leaves requests in UNSENT and can
make a later send() fail with InvalidStateError. Update the Omniblock XHR patch
so open() only marks blocked requests (using a flag on the instance) and then
patch XMLHttpRequest.prototype.send to detect that flag and short-circuit
blocked requests harmlessly; use the existing open() wrapper and
originalXhrOpen/original send flow as the reference points.
---
Nitpick comments:
In `@src/plugins/omniblock/style.css`:
- Around line 32-33: The broad wildcard selectors in the omniblock CSS are
causing unintended matches outside YouTube. Tighten the selectors in the style
rule that uses [class*="sponsor" i] and [class*="ad-badge" i] by scoping them to
known YouTube domains or replacing them with more specific YouTube-only
selectors so unrelated elements on other sites are not hidden.
- Around line 51-57: The `.ytp-ad-skip-button-container` CSS rule in the
omniblock styles cannot override hidden ancestor containers such as
`.ytp-ad-module` or `.ytp-ad-player-overlay`, so the skip button may still be
unreachable. Update `setupAdSkipper` to detect when the natural skip UI is
hidden by parent rules and trigger programmatic clicking of the skip action
instead of relying only on visibility styling. Keep the existing selector logic
in the ad-skipping path, but make the JS fallback handle cases where the
container is nested inside hidden parents.
- Around line 1-49: The ad-hiding rule in the omniblock stylesheet already uses
a broad fallback stack, but it can be made more performance-friendly by
isolating matched nodes from layout/style work. Update the existing selector
block in the omniblock CSS to add a containment rule on the same hidden
elements, keeping the current hiding properties intact while applying the extra
isolation only in this shared rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e33875-2260-4c89-80a2-c812b3cc3026
📒 Files selected for processing (4)
src/i18n/resources/en.jsonsrc/plugins/omniblock/blocker.tssrc/plugins/omniblock/index.tssrc/plugins/omniblock/style.css
Comment on lines
+24
to
+39
| const adKeys = new Set([ | ||
| 'playerAds', 'adPlacements', 'adSlots', 'adBreakHeartbeatParams', | ||
| 'trackingParams', 'adEngine', 'adBreakParams', 'ad', 'ad_slots', | ||
| 'ad_placements', 'loggingContext', 'vmapRes', 'adBreak', 'adBreaks', | ||
| 'adReason', 'adSurvey', 'adThumbnails', 'adFormat', 'adType', 'adPreroll', | ||
| 'adMidroll', 'adPostroll', 'adState', 'adServer', 'adTags', 'adUrl', | ||
| 'adUrls', 'adCreative', 'adCreatives', 'adConfig', 'adConfigs', 'adContext', | ||
| 'adParams', 'adParameters', 'adPlayback', 'adRenderer', 'adRenderers', | ||
| 'adSlot', 'adTarget', 'adTargets', 'adTemplate', 'adTemplates', 'adTracking', | ||
| 'adTrackings', 'adView', 'adViews', 'adVolume', 'adVolumes', 'adWaterfall', | ||
| 'adWaterfalls', 'adWrapper', 'adWrappers', 'adZone', 'adZones', 'clientMessages', | ||
| 'engagementPanel', 'engagementPanels', 'mealbar', 'promoted', 'sparkles', | ||
| 'banner', 'banners', 'promotedSparkles', 'webRenderer', 'actionCompanion', | ||
| 'displayAd', 'mastheadAd', 'inFeedAd', 'statementBanner', 'mealbarPromo', | ||
| 'adBreakHeartbeat', 'adServerConfig', 'adTrackingParams', 'adPrerollParams', | ||
| 'adMidrollParams', 'adPostrollParams', 'adStateParams', 'adServerParams' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope pruning before deleting generic JSON fields.
The global JSON.parse/Response patches delete broad keys like trackingParams, banner, clientMessages, and engagementPanel from every parsed payload, not just ad responses. That can corrupt unrelated page/application data. Restrict broad pruning to known ad/YouTube response URLs or use schema-aware rules.
Also applies to: 48-55, 64-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/omniblock/index.ts` around lines 24 - 39, The global pruning
logic in omniblock is too broad and removes generic fields from every parsed
payload, which can corrupt non-ad data. Update the JSON.parse/Response patch
handling in index.ts so the adKeys-based deletion only runs for known ad/YouTube
ad response URLs or other clearly identified ad payloads, and keep the generic
keys like trackingParams, banner, clientMessages, and engagementPanel out of
unrestricted global filtering. Use the existing adKeys set and the patching
hooks to apply schema-aware or URL-scoped pruning instead of blanket deletion.
Comment on lines
+228
to
+231
| async start({ getConfig }) { | ||
| this.config = await getConfig(); | ||
| this.updateCosmeticState(); | ||
| this.setupAdSkipper(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include enabled in renderer feature gates.
Renderer startup, cosmetic hiding, and the ad-skip loop only check sub-feature flags. Since those default to true, the renderer can still mutate the page if this lifecycle runs while the top-level plugin is disabled.
🐛 Minimal guard
- if (this.config?.domRemoval) {
+ if (this.config?.enabled && this.config?.domRemoval) {
@@
- if (!this.config?.adSkipper) return;
+ if (!this.config?.enabled || !this.config?.adSkipper) return;Also applies to: 243-248, 286-287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/plugins/omniblock/index.ts` around lines 228 - 231, The renderer startup
path in start, along with updateCosmeticState and setupAdSkipper, only checks
sub-feature flags and can still run when the top-level plugin is disabled. Add
the enabled gate to the feature checks in these lifecycle paths so page
mutation, cosmetic hiding, and the ad-skip loop only run when the plugin is
fully enabled. Use the existing symbols start, updateCosmeticState,
setupAdSkipper, and the related renderer feature gate logic to locate and update
the conditions.
Dev-NotAqua
force-pushed
the
feat-omniblock
branch
from
2c8d548 to
1d9aac3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since the existing Do Not Track and SponsorBlock plugins weren’t working properly, I put together a new adblock plugin for Pear.
It blocks ad and tracker requests using Ghostery with a few custom blocklists, strips ad-related data from responses, removes ad elements from the UI before they load, and handles video ads by muting, speeding up, and skipping them automatically.
I tested everything and it’s working well without interruptions.
It passes build, lint, and type checks.
Summary by CodeRabbit