Skip to content

Create ad blocking module with basic support for RC flags#8495

Open
CrisBarreiro wants to merge 4 commits into
developfrom
feature/cris/ad-blocking/ad-blocking-module
Open

Create ad blocking module with basic support for RC flags#8495
CrisBarreiro wants to merge 4 commits into
developfrom
feature/cris/ad-blocking/ad-blocking-module

Conversation

@CrisBarreiro
Copy link
Copy Markdown
Collaborator

@CrisBarreiro CrisBarreiro commented May 8, 2026

Task/Issue URL: https://app.asana.com/1/137249556945/project/72649045549333/task/1214489465023870?focus=true

Description

Add module and RC support for ad blocking

Steps to test this PR

n/a

UI changes

n/a


Note

Low Risk
Low risk: this PR only scaffolds a new module and defines remote feature flags/data models, with no runtime wiring or behavior changes shown beyond adding new buildable sources.

Overview
Adds a new ad-blocking-impl Android library module wired for DI (anvil/ksp) and depends on feature-toggles-api/browser-api.

Introduces AdBlockingExtensionFeature as a @ContributesRemoteFeature with two RC-backed toggles (self and enabledByDefault, both defaulting to false) plus simple settings models (AdBlockingExtensionSettings, ScriptletEntry).

Adds a minimal ad-blocking/readme.md describing the feature and linking to design docs.

Reviewed by Cursor Bugbot for commit f0a99e3. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Collaborator Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/ad-blocking/ad-blocking-module branch from 013db12 to a4b572c Compare May 11, 2026 10:22
@CrisBarreiro CrisBarreiro marked this pull request as ready for review May 11, 2026 10:37
@CrisBarreiro CrisBarreiro requested a review from cmonfortep May 11, 2026 10:37
Copy link
Copy Markdown
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

NIT: if api is empty, we don't need to create it. We can just have -impl for now.

Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f0a99e3. Configure here.

fun self(): Toggle

@Toggle.DefaultValue(DefaultFeatureValue.FALSE)
fun enabledByDefault(): Toggle
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toggle named enabledByDefault defaults to FALSE

Medium Severity

The enabledByDefault() toggle is annotated with DefaultFeatureValue.FALSE, which contradicts its name. The codebase's own test fixture in FeatureTogglesTest establishes the convention that enabledByDefault() maps to DefaultFeatureValue.TRUE and disableByDefault() maps to DefaultFeatureValue.FALSE. If the intent is for this sub-feature to be off until RC enables it, the name is misleading and will confuse future readers; if the intent matches the name, the default value is wrong.


Please tell me if this was useful or not with a 👍 or 👎.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f0a99e3. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants