Skip to content

Add ad-blocking browser menu logic#9154

Open
CrisBarreiro wants to merge 3 commits into
feature/cris/ad-blocking-menu-choice-stubfrom
feature/cris/ad-blocking-menu-logic
Open

Add ad-blocking browser menu logic#9154
CrisBarreiro wants to merge 3 commits into
feature/cris/ad-blocking-menu-choice-stubfrom
feature/cris/ad-blocking-menu-logic

Conversation

@CrisBarreiro

@CrisBarreiro CrisBarreiro commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Task/Issue URL:
Tech Design URL (if applicable):

Description

Add logic to browser menu entry

  • If ad blocking enabled, clicking the item will show a bottom sheet, allowing users to change settings
    • Always On -> Unchanged
    • Disable until relaunch -> Disable but don't persist, next app start (after being killed) will re-enable the feature
    • Always off -> Equivalent to disabling in settings. A line below the toggle item in settings is added
  • If ad blocking disabled, clicking the item will enable ad blocking, no bottom sheet shown

Steps to test this PR

UX improvements RC flag disabled - no menu entry

  • Fresh install
  • Open a YouTube video
  • Check no entry for YouTube ad blocking is shown

adBlockingExtension RC flag disabled, UX improvements RC flag enabled - no menu entry

  • Disable adBlockingExtension and enable adBlockingUXImprovementsflags
  • Open a YouTube video
  • Check no entry for YouTube ad blocking is shown

UX improvements RC flag enabled - menu entry on YT pages - test enable

  • Enable adBlockingExtension and adBlockingUXImprovements RC flag
  • Open a YouTube video
  • Check "Enable YouTube Ad Blocking" is shown
  • Click it
  • Check no new bottom sheet appears
  • Check current page reloads
  • Check no logs for "Ad blocking disabled"
  • Open the menu again and check it now says "Disable YouTube Ad Blocking"
  • Open Ad Blocking settings
  • Check the toggle is enabled

UX improvements RC flag enabled - menu entry on YT pages - test disable until relaunch

  • From the previous scenario (RC flag enabled, ad blocking enabled), open a YouTube video
  • Click "Disable YouTube Ad Blocking"
  • Check a new bottom sheet appears with "Always On" selected
  • Click "Disable Until Relaunch"
  • Check current page reloads
  • Check logs for "Ad blocking disabled"
  • Open the menu again and check "Disable until Relaunch" is selected
  • Open Ad Blocking settings
  • Check the toggle is disabled and "Disabled until relaunch" is displayed
  • Toggle the feature on and check that second line disappears
  • Toggle the feature off and check that second line still doesn't show
  • Toggle the feature on again
  • Select "Disable Until Relaunch" again from the browser menu
  • Close the app
  • Open it again and check ad blocking is now enabled

UX improvements RC flag enabled - menu entry on YT pages - test disable

  • From the previous test scenario (RC flags enabled, ad blocking enabled), open a YouTube video
  • Check logs for "Ad blocking disabled"
  • Check Enable YouTube Ad Blocking is shown
  • Click it
  • Check current page reloads
  • Check no logs for "Ad blocking disabled"
  • Open the menu again and check "Disable until Relaunch" is selected
  • Open Ad Blocking settings
  • Check the toggle is on

UX improvements RC flag enabled - no menu entry on non-YT pages

  • From the previous test scenario (RC flags enabled, ad blocking enabled), open wikipedia.org
  • Open browser menu
  • Check no entry is shown for ad blocking

UI changes

Before After
!(Upload before screenshot) (Upload after screenshot)

Note

Medium Risk
Changes when ad blocking injects and how user vs session overrides interact with persisted settings; mistakes could leave blocking on/off unexpectedly, though scope is limited to the ad-blocking module with solid test coverage.

Overview
Adds YouTube browser menu behavior: when ad blocking is off, a tap turns it on without a sheet; when on, a bottom sheet offers Always On, Disable Until Relaunch, and Always Off, with checkmarks for the active option.

Disable until relaunch is a new in-memory AdBlockingSessionStore override that turns off injection for the current process without changing persisted settings; it clears on app kill or when the user picks Always On/Off or toggles in settings. AdBlockingState now distinguishes permanent disable vs until relaunch, and AdBlockingStatusChecker / injection gating honor that session flag ahead of stored prefs.

Settings v2 swaps to a two-line toggle with “Disabled until relaunch” while the session override is active, and AdBlockingMenuController centralizes applying menu choices to the repository and session store.

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

CrisBarreiro commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@CrisBarreiro CrisBarreiro changed the base branch from feature/cris/ad-blocking-menu-choice-stub to graphite-base/9154 July 10, 2026 09:04
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/ad-blocking-menu-logic branch from ef14862 to 8889335 Compare July 10, 2026 09:07
@CrisBarreiro CrisBarreiro changed the base branch from graphite-base/9154 to feature/cris/ad-blocking-menu-choice-stub July 10, 2026 09:07

@cmonfortep cmonfortep left a comment

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.

LGTM

I've additionally tested the menu on duck.ai and custom tab.

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/ad-blocking-menu-choice-stub branch from 6357404 to 08f2bda Compare July 10, 2026 18:34
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/ad-blocking-menu-logic branch from e0694b3 to 264b921 Compare July 10, 2026 18:34

@cursor cursor Bot left a comment

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.

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 264b921. Configure here.

when (menuState) {
AdBlockingMenuState.Disabled -> menuController.enable()
else -> showMenuBottomSheet()
}

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.

Stale menu state misroutes clicks

Medium Severity

menuState defaults to Hidden and the click handler treats anything that is not Disabled as “show the bottom sheet.” Until the first observe emission updates menuState, a tap on “Enable YouTube Ad Blocking” can open the disable sheet instead of calling menuController.enable(), and a briefly visible row can accept taps when the item should stay hidden.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 264b921. Configure here.

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

Labels

None yet

2 participants