[Draft] Update 3x pixel-firing logic to more closely match broken site prompt #5862

laghee · 2025-04-04T13:47:12Z

Task/Issue URL: https://app.asana.com/0/72649045549333/1209712075249435/f

Description

We decided in https://app.asana.com/0/1205142657210376/1209698956036733 that we would prefer the separate 3x pixel to fire for distinct 3-refresh groupings in the chosen time period as opposed to 3 refreshes that fall into a sliding window of time to more closely match the breakage prompt trigger.

Steps to test this PR

NB: Logic is currently patched to delay 7 seconds between accepted prompts & stop prompting with 3+ dismissals within the past 30 min.

Feature 1

Go to a site
Refresh 3 times
Verify that RELOAD-2X and RELOAD-3X pixels fire
Dismiss prompt
Refresh 3 times, pause for at least 20 seconds, then refresh 3 times again
Verify that RELOAD-2X and RELOAD-3X fire twice

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

CrisBarreiro · 2025-04-10T15:57:04Z

@laghee, If I understand correctly, the logic to reset refresh count is not affected (reset once 3x or more refreshes), however, if the user refreshes 4x, we'd only send the 2x once. Not sure if that's expected / what iOS does in that case

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

CrisBarreiro · 2025-04-10T15:46:09Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

+                    }
+                    pixel.fire(AppPixelName.RELOAD_THREE_TIMES_WITHIN_20_SECONDS)
+                    Timber.d("KateTest--> should show bc 3 refreshes")
+                    site?.let { brokenSitePrompt.shouldShowBrokenSitePrompt(it.url) } ?: false


I guess null site here is an edge case here, so probably fine if pixel and prompt fall out of sync

app/src/main/java/com/duckduckgo/app/global/db/AppDatabase.kt

CrisBarreiro · 2025-04-10T15:50:05Z

...n-site/broken-site-impl/src/main/java/com/duckduckgo/brokensite/impl/RealBrokenSitePrompt.kt

        }

        val dismissStreakResetDays = brokenSiteReportRepository.getDismissStreakResetDays().toLong()
        val dismissalCount = brokenSiteReportRepository.getDismissalCountBetween(
-            currentTimestamp.minusDays(dismissStreakResetDays),
+            currentTimestamp.minusMinutes(dismissStreakResetDays),


Careful to revert this later

CrisBarreiro · 2025-04-10T15:50:17Z

...n-site/broken-site-impl/src/main/java/com/duckduckgo/brokensite/impl/RealBrokenSitePrompt.kt


        return dismissalCount < brokenSiteReportRepository.getMaxDismissStreak()
    }

    override suspend fun ctaShown() {
        val currentTimestamp = currentTimeProvider.localDateTimeNow()
-        val newNextShownDate = currentTimestamp.plusDays(brokenSiteReportRepository.getCoolDownDays())
+        val newNextShownDate = currentTimestamp.plusSeconds(brokenSiteReportRepository.getCoolDownDays())


CrisBarreiro · 2025-04-10T16:00:32Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -3946,6 +3952,31 @@ class BrowserTabViewModel @Inject constructor(
        newTabPixels.get().fireNewTabDisplayed()
    }

+    private suspend fun shouldPromptForBrokenSiteReport(): Boolean {


From the method name, it's not obvious what it does

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

…refactor

marcosholgado · 2025-04-15T08:28:57Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -468,6 +469,7 @@ class BrowserTabViewModel @Inject constructor(
    private val defaultBrowserPromptsExperiment: DefaultBrowserPromptsExperiment,
    private val swipingTabsFeature: SwipingTabsFeatureProvider,
    private val visualDesignExperimentDataStore: VisualDesignExperimentDataStore,
+    private val blockListPixelsPlugin: BlockListPixelsPlugin,


Don't think this is needed anymore

marcosholgado · 2025-04-15T08:36:22Z

app/src/main/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSender.kt

        pixel.fire(CustomTabPixelNames.CUSTOM_TABS_MENU_REFRESH)
    }

-    private fun sendTimeBasedPixels() {
+    override fun sendBreakageRefreshPixels(patternsDetected: Set<Int>) {


I would call this method something like onRefreshPatternDetected, the fact that internally we send a pixel or do something else is irrelevant to the caller so I wouldn't be that specific.

marcosholgado · 2025-04-15T08:47:16Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

@@ -3946,6 +3950,16 @@ class BrowserTabViewModel @Inject constructor(
        newTabPixels.get().fireNewTabDisplayed()
    }

+    private suspend fun shouldShowPromptForBrokenSiteReport(): Boolean {


This method is called right before refreshCta and the value it sends is used in the CtaViewModel so why not doing it there instead?

That's a good point. I originally moved it to avoid losing the context for the user refreshes and resetting the count too often, but now that the logic's been altered, that's probably no longer necessary.

marcosholgado · 2025-04-15T08:48:41Z

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt

            val cta = withContext(dispatchers.io()) {
                ctaViewModel.refreshCta(
                    dispatchers.io(),
                    isBrowserShowing && !isErrorShowing,
                    siteLiveData.value,
+                    showBrokenSitePrompt,


Overall I'm struggling with this. I don't fully get why we have pull this out of the CtaViewModel to pass a boolean back to it when within the CtaViewModel we still have access to the BrokenSitePrompt. It seems an unnecessary journey for a value that we can directly get when needed in the CtaViewModel

marcosholgado · 2025-04-15T08:57:00Z

app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt


-        blockListPixelsPlugin.get2XRefresh()!!.getPixelDefinitions().forEach {
+        blockListPixelsPlugin.get2XRefresh()?.getPixelDefinitions()?.forEach {


Go back to !! I want this test to fail if get2Refresh returns null and this would bypass it.

Whoops, unintentional

marcosholgado · 2025-04-15T08:57:09Z

app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt


-        blockListPixelsPlugin.get3XRefresh()!!.getPixelDefinitions().forEach {
+        blockListPixelsPlugin.get3XRefresh()?.getPixelDefinitions()?.forEach {


Same as above

marcosholgado · 2025-04-15T08:57:56Z

app/src/test/java/com/duckduckgo/app/browser/refreshpixels/RefreshPixelSenderTest.kt

@@ -143,181 +115,28 @@ class RefreshPixelSenderTest {
    }

    @Test
-    fun whenSendMenuRefreshPixelsThenTimeBasedPixelsFired() = runTest {


Overall a lot of tests have been deleted, are the others covering for the new logic, pixels, etc?

marcosholgado · 2025-04-15T08:59:17Z

...n-site-impl/src/main/java/com/duckduckgo/brokensite/impl/BrokenSiteRefreshesInMemoryStore.kt

+
+    companion object {
+        private const val TWICE_REFRESH = 2
+        private const val TWICE_REFRESH_WINDOW = 12L


Is this 12 seconds? minutes? Would change the const to TWICE_REFRESH_WINDOW_IN_SECS or whatever the right thing is. Same for thrice.

marcosholgado · 2025-04-15T09:02:13Z

.../broken-site-impl/src/main/java/com/duckduckgo/brokensite/impl/BrokenSiteReportRepository.kt

@@ -59,15 +59,15 @@ interface BrokenSiteReportRepository {

    fun resetRefreshCount()
    fun addRefresh(url: Uri, localDateTime: LocalDateTime)
-    fun getAndUpdateUserRefreshesBetween(t1: LocalDateTime, t2: LocalDateTime): Int
+    fun checkForRefreshPatterns(currentDateTime: LocalDateTime): Set<Int>


Would probably rename to getRefreshPatterns rather than check. Check doesn't tell me what is the thing that is checking so a bit confusing. Also, this returns the refresh patterns but they are not associated to any URL so how do you know it's relevant for the current site?

Make 3x pixel fire logic match prompt trigger not sliding window

b325812

laghee requested a review from CrisBarreiro April 4, 2025 13:47

marcosholgado reviewed Apr 4, 2025

View reviewed changes

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt Outdated Show resolved Hide resolved

laghee added 3 commits April 10, 2025 13:24

Try new approach to grouping pixels with prompt check

4afe244

Remove refreshDAO

2fe332c

Remove refresh count fetch from tests

d2eef06

CrisBarreiro reviewed Apr 10, 2025

View reviewed changes

app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt Outdated Show resolved Hide resolved

Rework count logic to ensure 2x pixel triggering + revert pixel fire …

7db243b

…refactor

marcosholgado reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] Update 3x pixel-firing logic to more closely match broken site prompt #5862

[Draft] Update 3x pixel-firing logic to more closely match broken site prompt #5862

laghee commented Apr 4, 2025 •

edited

Loading

CrisBarreiro commented Apr 10, 2025

CrisBarreiro Apr 10, 2025

CrisBarreiro Apr 10, 2025

CrisBarreiro Apr 10, 2025

CrisBarreiro Apr 10, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025

laghee Apr 17, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025

laghee Apr 17, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025

marcosholgado Apr 15, 2025


		blockListPixelsPlugin.get2XRefresh()!!.getPixelDefinitions().forEach {
		blockListPixelsPlugin.get2XRefresh()?.getPixelDefinitions()?.forEach {


		blockListPixelsPlugin.get3XRefresh()!!.getPixelDefinitions().forEach {
		blockListPixelsPlugin.get3XRefresh()?.getPixelDefinitions()?.forEach {

[Draft] Update 3x pixel-firing logic to more closely match broken site prompt #5862

Are you sure you want to change the base?

[Draft] Update 3x pixel-firing logic to more closely match broken site prompt #5862

Conversation

laghee commented Apr 4, 2025 • edited Loading

Description

Steps to test this PR

CrisBarreiro commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laghee commented Apr 4, 2025 •

edited

Loading