Fix tab not being inserted when opening in new tab

CrisBarreiro · CrisBarreiro · commit 87559540def5 · 2025-06-04T13:53:35.000+02:00
diff --git a/app/src/main/java/com/duckduckgo/app/tabs/TabFeatureFlags.kt b/app/src/main/java/com/duckduckgo/app/tabs/TabFeatureFlags.kt
@@ -32,4 +32,7 @@ interface TabManagerFeatureFlags {
 
     @Toggle.DefaultValue(DefaultFeatureValue.FALSE)
     fun multiSelection(): Toggle
+
+    @Toggle.DefaultValue(DefaultFeatureValue.TRUE)
+    fun tabInsertionFixes(): Toggle
 }
diff --git a/app/src/main/java/com/duckduckgo/app/tabs/db/TabsDao.kt b/app/src/main/java/com/duckduckgo/app/tabs/db/TabsDao.kt
@@ -26,6 +26,7 @@ import dagger.SingleInstanceIn
 import java.time.LocalDateTime
 import kotlin.math.max
 import kotlinx.coroutines.flow.Flow
+import logcat.logcat
 
 @Dao
 @SingleInstanceIn(AppScope::class)
@@ -160,10 +161,24 @@ abstract class TabsDao {
     abstract fun incrementPositionStartingAt(position: Int)
 
     @Transaction
-    open fun addAndSelectTab(tab: TabEntity) {
-        deleteBlankTabs()
-        insertTab(tab)
-        insertTabSelection(TabSelectionEntity(tabId = tab.tabId))
+    open fun addAndSelectTab(tab: TabEntity, updateIfBlankParent: Boolean = false) {
+        try {
+            val parent = tab.sourceTabId?.let { tab(it) }
+            val newTab = if (updateIfBlankParent && parent != null && parent.url == null) {
+                /*
+                 * If the parent tab is blank, we need to update the parent tab with the previous
+                 * one. Otherwise, foreign key constrains won't be met
+                 */
+                tab.copy(sourceTabId = parent.sourceTabId, position = parent.position)
+            } else {
+                tab
+            }
+            deleteBlankTabs()
+            insertTab(newTab)
+            insertTabSelection(TabSelectionEntity(tabId = newTab.tabId))
+        } catch (e: Exception) {
+            logcat { "Error adding and selecting tab: $e" }
+        }
     }
 
     @Transaction
diff --git a/app/src/main/java/com/duckduckgo/app/tabs/model/TabDataRepository.kt b/app/src/main/java/com/duckduckgo/app/tabs/model/TabDataRepository.kt
@@ -93,12 +93,21 @@ class TabDataRepository @Inject constructor(
 
     private var purgeDeletableTabsJob = ConflatedJob()
 
+    private var tabInsertionFixesFlag: Boolean? = null
+
     override suspend fun add(
         url: String?,
         skipHome: Boolean,
     ): String = withContext(dispatchers.io()) {
         val tabId = generateTabId()
-        add(tabId, buildSiteData(url, tabId), skipHome = skipHome, isDefaultTab = false)
+        val flag = getAndCacheTabInsertionFixesFlag()
+        val siteData = if (flag) {
+            buildSiteData(url, tabId)
+        } else {
+            buildSiteDataSync(url, tabId)
+        }
+        add(tabId, siteData, skipHome = skipHome, isDefaultTab = false, updateIfBlankParent = flag)
+
         return@withContext tabId
     }
 
@@ -108,34 +117,52 @@ class TabDataRepository @Inject constructor(
         sourceTabId: String,
     ): String = withContext(dispatchers.io()) {
         val tabId = generateTabId()
-
+        val flag = getAndCacheTabInsertionFixesFlag()
+        val siteData = if (flag) {
+            buildSiteData(url, tabId)
+        } else {
+            buildSiteDataSync(url, tabId)
+        }
         add(
             tabId = tabId,
-            data = buildSiteData(url, tabId),
+            data = siteData,
             skipHome = skipHome,
             isDefaultTab = false,
             sourceTabId = sourceTabId,
+            updateIfBlankParent = flag,
         )
 
         return@withContext tabId
     }
 
     override suspend fun addDefaultTab(): String = withContext(dispatchers.io()) {
         val tabId = generateTabId()
-
+        val flag = getAndCacheTabInsertionFixesFlag()
+        val siteData = if (flag) {
+            buildSiteData(url = null, tabId = tabId)
+        } else {
+            buildSiteDataSync(url = null, tabId)
+        }
         add(
             tabId = tabId,
-            data = buildSiteData(url = null, tabId = tabId),
+            data = siteData,
             skipHome = false,
             isDefaultTab = true,
+            updateIfBlankParent = flag,
         )
 
         return@withContext tabId
     }
 
+    private suspend fun getAndCacheTabInsertionFixesFlag(): Boolean {
+        return tabInsertionFixesFlag ?: withContext(dispatchers.io()) {
+            tabManagerFeatureFlags.tabInsertionFixes().isEnabled()
+        }.also { tabInsertionFixesFlag = it }
+    }
+
     private fun generateTabId() = UUID.randomUUID().toString()
 
-    private fun buildSiteData(url: String?, tabId: String): MutableLiveData<Site> {
+    private fun buildSiteDataSync(url: String?, tabId: String): MutableLiveData<Site> {
         val data = MutableLiveData<Site>()
         url?.let {
             val siteMonitor = siteFactory.buildSite(url = it, tabId = tabId)
@@ -144,12 +171,23 @@ class TabDataRepository @Inject constructor(
         return data
     }
 
+    private suspend fun buildSiteData(url: String?, tabId: String): MutableLiveData<Site> {
+        val data = MutableLiveData<Site>()
+        url ?: return data
+        val siteMonitor = siteFactory.buildSite(url = url, tabId = tabId)
+        withContext(dispatchers.main()) {
+            data.value = siteMonitor
+        }
+        return data
+    }
+
     private fun add(
         tabId: String,
         data: MutableLiveData<Site>,
         skipHome: Boolean,
         isDefaultTab: Boolean,
         sourceTabId: String? = null,
+        updateIfBlankParent: Boolean = false,
     ) {
         siteData[tabId] = data
         databaseExecutor().scheduleDirect {
@@ -178,6 +216,7 @@ class TabDataRepository @Inject constructor(
                     position = position,
                     sourceTabId = sourceTabId,
                 ),
+                updateIfBlankParent = updateIfBlankParent,
             )
         }
     }
diff --git a/app/src/test/java/com/duckduckgo/tabs/db/TabsDaoTest.kt b/app/src/test/java/com/duckduckgo/tabs/db/TabsDaoTest.kt
@@ -465,4 +465,36 @@ class TabsDaoTest {
         assertNotNull(testee.tab(tab3.tabId))
         assertEquals(tab3, testee.selectedTab())
     }
+
+    @Test
+    fun whenAddAndSelectWithBlankParentAndUpdateIfBlankParentTrueThenUpdateTabBeforeInserting() {
+        val zero = TabEntity(
+            "TAB_ID0",
+            position = 0,
+            sourceTabId = null,
+            url = "http://example.com",
+        )
+        val first = TabEntity(
+            "TAB_ID1",
+            position = 1,
+            sourceTabId = "TAB_ID0",
+            url = null,
+        )
+        val second = TabEntity(
+            "TAB_ID2",
+            position = 2,
+            sourceTabId = "TAB_ID1",
+            url = "http://example.com",
+        )
+
+        testee.insertTab(zero)
+        testee.insertTab(first)
+        testee.addAndSelectTab(second, updateIfBlankParent = true)
+
+        assertFalse(testee.tabs().contains(first))
+        val storedTab = testee.tab("TAB_ID2")
+        assertEquals("TAB_ID0", storedTab?.sourceTabId)
+        assertEquals(1, storedTab?.position)
+        assertEquals(storedTab, testee.selectedTab())
+    }
 }
diff --git a/app/src/test/java/com/duckduckgo/tabs/model/TabDataRepositoryTest.kt b/app/src/test/java/com/duckduckgo/tabs/model/TabDataRepositoryTest.kt
@@ -123,10 +123,28 @@ class TabDataRepositoryTest {
         testee.add("http://www.example.com")
 
         val captor = argumentCaptor<TabEntity>()
-        verify(mockDao).addAndSelectTab(captor.capture())
+        verify(mockDao).addAndSelectTab(captor.capture(), any())
         assertTrue(captor.firstValue.viewed)
     }
 
+    @Test
+    fun whenTabAddAndTabInsertionFixesOnThenAddAndSelectIsCalledWithUpdateIfBlankParent() = runTest {
+        val testee = tabDataRepository()
+        tabManagerFeatureFlags.tabInsertionFixes().setRawStoredState(State(enable = true))
+        testee.add("http://www.example.com")
+
+        verify(mockDao).addAndSelectTab(any(), eq(true))
+    }
+
+    @Test
+    fun whenTabAddAndTabInsertionFixesOffThenAddAndSelectIsCalledWithUpdateIfBlankParentFalse() = runTest {
+        val testee = tabDataRepository()
+        tabManagerFeatureFlags.tabInsertionFixes().setRawStoredState(State(enable = false))
+        testee.add("http://www.example.com")
+
+        verify(mockDao).addAndSelectTab(any(), eq(false))
+    }
+
     @Test
     fun whenTabUpdatedAfterOpenInBackgroundThenViewedIsTrue() = runTest {
         val testee = tabDataRepository()
@@ -170,7 +188,7 @@ class TabDataRepositoryTest {
     fun whenAddCalledThenTabAddedAndSelectedAndBlankSiteDataCreated() = runTest {
         val testee = tabDataRepository()
         val createdId = testee.add()
-        verify(mockDao).addAndSelectTab(any())
+        verify(mockDao).addAndSelectTab(any(), any())
         assertNotNull(testee.retrieveSiteData(createdId))
     }
 
@@ -179,7 +197,7 @@ class TabDataRepositoryTest {
         val testee = tabDataRepository()
         val url = "http://example.com"
         val createdId = testee.add(url)
-        verify(mockDao).addAndSelectTab(any())
+        verify(mockDao).addAndSelectTab(any(), any())
         assertNotNull(testee.retrieveSiteData(createdId))
         assertEquals(url, testee.retrieveSiteData(createdId).value!!.url)
     }
@@ -229,7 +247,7 @@ class TabDataRepositoryTest {
         testee.add("http://www.example.com")
 
         val captor = argumentCaptor<TabEntity>()
-        verify(mockDao).addAndSelectTab(captor.capture())
+        verify(mockDao).addAndSelectTab(captor.capture(), any())
         assertTrue(captor.firstValue.position == 0)
     }
 
@@ -245,7 +263,7 @@ class TabDataRepositoryTest {
         testee.add("http://www.example.com")
 
         val captor = argumentCaptor<TabEntity>()
-        verify(mockDao).addAndSelectTab(captor.capture())
+        verify(mockDao).addAndSelectTab(captor.capture(), any())
         assertTrue(captor.firstValue.position == 1)
     }
 

Original file line number	Diff line number	Diff line change
`@@ -32,4 +32,7 @@ interface TabManagerFeatureFlags {`
`32`	`32`
`33`	`33`	`@Toggle.DefaultValue(DefaultFeatureValue.FALSE)`
`34`	`34`	`fun multiSelection(): Toggle`
	`35`	`+`
	`36`	`+ @Toggle.DefaultValue(DefaultFeatureValue.TRUE)`
	`37`	`+ fun tabInsertionFixes(): Toggle`
`35`	`38`	`}`