Skip to content

Commit 09514e4

Browse files
GerardPaligotGerardPaligot
committed
Add chatIdOrNull to DuckChat api and drop chatID string duplication
The "chatID" query parameter name was duplicated across three call sites extracting the chatID from a Duck.ai URL, each implementing the lookup differently and only two of them gating on isDuckChatUrl. Lift the operation onto the public DuckChat interface as chatIdOrNull(uri) so the literal lives in one place and the safety guard is enforced for every consumer — closing a latent foot-gun in DuckChatContextualViewModel.hasChatId that previously trusted any URL carrying a chatID query param. DuckChatConstants.CHAT_ID_PARAM is removed; the raw "chatID" string now only lives in RealDuckChat's private const. URL building stays on the impl-only DuckChatInternal since every builder is impl-internal.
1 parent 3166499 commit 09514e4

17 files changed

Lines changed: 65 additions & 69 deletions

File tree

‎app/src/main/java/com/duckduckgo/app/browser/nativeinput/NativeInputManager.kt‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -722,8 +722,7 @@ class RealNativeInputManager @Inject constructor(
722722
internal fun isExistingDuckAiChat(rawUrl: String?): Boolean {
723723
if (rawUrl.isNullOrBlank()) return false
724724
val uri = runCatching { rawUrl.toUri() }.getOrNull() ?: return false
725-
if (!duckChat.isDuckChatUrl(uri)) return false
726-
return !uri.getQueryParameter("chatID").isNullOrBlank()
725+
return duckChat.chatIdOrNull(uri) != null
727726
}
728727

729728
companion object {

‎app/src/main/java/com/duckduckgo/app/fire/DuckAiTabsCleanupPlugin.kt‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.duckduckgo.app.fire
1818

19-
import android.net.Uri
2019
import androidx.core.net.toUri
2120
import com.duckduckgo.app.tabs.model.TabRepository
2221
import com.duckduckgo.dataclearing.api.plugin.ClearableData
@@ -61,19 +60,14 @@ class DuckAiTabsCleanupPlugin @Inject constructor(
6160
*/
6261
private suspend fun closeTabsMatching(chatUrls: Set<String>) {
6362
if (chatUrls.isEmpty()) return
64-
val targetChatIds = chatUrls.mapNotNullTo(mutableSetOf()) { it.toUri().chatIdOrNull() }
63+
val targetChatIds = chatUrls.mapNotNullTo(mutableSetOf()) { duckChat.chatIdOrNull(it.toUri()) }
6564
if (targetChatIds.isEmpty()) return
6665
val ids = tabRepository.getTabs()
67-
.filter { tab -> tab.url?.toUri()?.chatIdOrNull() in targetChatIds }
66+
.filter { tab -> tab.url?.toUri()?.let(duckChat::chatIdOrNull) in targetChatIds }
6867
.map { it.tabId }
6968
if (ids.isNotEmpty()) {
7069
logcat { "Closing ${ids.size} Duck.ai tab(s) matching the cleared subset" }
7170
tabRepository.deleteTabs(ids)
7271
}
7372
}
74-
75-
private fun Uri.chatIdOrNull(): String? {
76-
if (!duckChat.isDuckChatUrl(this)) return null
77-
return getQueryParameter("chatID")?.takeIf { it.isNotBlank() }
78-
}
7973
}

‎app/src/test/java/com/duckduckgo/app/browser/nativeinput/RealNativeInputManagerTest.kt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,39 +70,39 @@ class RealNativeInputManagerTest {
7070

7171
@Test
7272
fun whenUrlIsNotDuckAiThenIsExistingDuckAiChatFalse() {
73-
whenever(duckChat.isDuckChatUrl(any())).thenReturn(false)
73+
whenever(duckChat.chatIdOrNull(any())).thenReturn(null)
7474

7575
assertFalse(testee.isExistingDuckAiChat("https://example.com/?chatID=abcd"))
7676
}
7777

7878
@Test
7979
fun whenDuckAiUrlWithoutChatIdThenIsExistingDuckAiChatFalse() {
80-
whenever(duckChat.isDuckChatUrl(any())).thenReturn(true)
80+
whenever(duckChat.chatIdOrNull(any())).thenReturn(null)
8181

8282
assertFalse(testee.isExistingDuckAiChat("https://duck.ai/"))
8383
}
8484

8585
@Test
8686
fun whenDuckAiUrlWithBlankChatIdThenIsExistingDuckAiChatFalse() {
87-
whenever(duckChat.isDuckChatUrl(any())).thenReturn(true)
87+
whenever(duckChat.chatIdOrNull(any())).thenReturn(null)
8888

8989
assertFalse(testee.isExistingDuckAiChat("https://duck.ai/?chatID="))
9090
}
9191

9292
@Test
9393
fun whenDuckAiUrlWithChatIdThenIsExistingDuckAiChatTrue() {
94-
whenever(duckChat.isDuckChatUrl(any())).thenReturn(true)
94+
whenever(duckChat.chatIdOrNull(any())).thenReturn("abc-123")
9595

9696
assertTrue(testee.isExistingDuckAiChat("https://duck.ai/?chatID=abc-123"))
9797
}
9898

9999
@Test
100100
fun whenIsDuckChatUrlChecksParsedUriThenCheckedWithSameUri() {
101-
whenever(duckChat.isDuckChatUrl(any())).thenReturn(true)
101+
whenever(duckChat.chatIdOrNull(any())).thenReturn("xyz")
102102
val raw = "https://duck.ai/chat?chatID=xyz"
103103

104104
testee.isExistingDuckAiChat(raw)
105105

106-
verify(duckChat).isDuckChatUrl(Uri.parse(raw))
106+
verify(duckChat).chatIdOrNull(Uri.parse(raw))
107107
}
108108
}

‎app/src/test/java/com/duckduckgo/app/fire/DuckAiTabsCleanupPluginTest.kt‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,9 @@ class DuckAiTabsCleanupPluginTest {
9191
val unrelatedDuckAiTab = TabEntity(tabId = "tab2", url = "https://duck.ai?chatID=zzz")
9292
val browserTab = TabEntity(tabId = "tab3", url = "https://example.com")
9393
whenever(mockTabRepository.getTabs()).thenReturn(listOf(matchingTab, unrelatedDuckAiTab, browserTab))
94-
whenever(mockDuckChat.isDuckChatUrl(eq("https://duck.ai?chatID=abc".toUri()))).thenReturn(true)
95-
whenever(mockDuckChat.isDuckChatUrl(eq("https://duck.ai?chatID=zzz".toUri()))).thenReturn(true)
96-
whenever(mockDuckChat.isDuckChatUrl(eq("https://example.com".toUri()))).thenReturn(false)
94+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=abc".toUri()))).thenReturn("abc")
95+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=zzz".toUri()))).thenReturn("zzz")
96+
whenever(mockDuckChat.chatIdOrNull(eq("https://example.com".toUri()))).thenReturn(null)
9797

9898
testee.onClearData(setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai?chatID=abc"))))
9999

@@ -106,9 +106,9 @@ class DuckAiTabsCleanupPluginTest {
106106
val t2 = TabEntity(tabId = "tab2", url = "https://duck.ai?chatID=b")
107107
val t3 = TabEntity(tabId = "tab3", url = "https://example.com")
108108
whenever(mockTabRepository.getTabs()).thenReturn(listOf(t1, t2, t3))
109-
whenever(mockDuckChat.isDuckChatUrl(eq("https://duck.ai?chatID=a".toUri()))).thenReturn(true)
110-
whenever(mockDuckChat.isDuckChatUrl(eq("https://duck.ai?chatID=b".toUri()))).thenReturn(true)
111-
whenever(mockDuckChat.isDuckChatUrl(eq("https://example.com".toUri()))).thenReturn(false)
109+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=a".toUri()))).thenReturn("a")
110+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=b".toUri()))).thenReturn("b")
111+
whenever(mockDuckChat.chatIdOrNull(eq("https://example.com".toUri()))).thenReturn(null)
112112

113113
testee.onClearData(
114114
setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai?chatID=a", "https://duck.ai?chatID=b"))),
@@ -121,7 +121,8 @@ class DuckAiTabsCleanupPluginTest {
121121
fun `DuckChats Selected with no matching tab does not call deleteTabs`() = runTest {
122122
val unrelated = TabEntity(tabId = "tab1", url = "https://duck.ai?chatID=other")
123123
whenever(mockTabRepository.getTabs()).thenReturn(listOf(unrelated))
124-
whenever(mockDuckChat.isDuckChatUrl(any())).thenReturn(true)
124+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=other".toUri()))).thenReturn("other")
125+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai?chatID=abc".toUri()))).thenReturn("abc")
125126

126127
testee.onClearData(setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai?chatID=abc"))))
127128

@@ -134,7 +135,7 @@ class DuckAiTabsCleanupPluginTest {
134135
val t2 = TabEntity(tabId = "tab2", url = "https://duck.ai?chatID=abc")
135136
val t3 = TabEntity(tabId = "tab3", url = "https://duck.ai?chatID=abc")
136137
whenever(mockTabRepository.getTabs()).thenReturn(listOf(t1, t2, t3))
137-
whenever(mockDuckChat.isDuckChatUrl(any())).thenReturn(true)
138+
whenever(mockDuckChat.chatIdOrNull(any())).thenReturn("abc")
138139

139140
testee.onClearData(setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai?chatID=abc"))))
140141

@@ -150,7 +151,7 @@ class DuckAiTabsCleanupPluginTest {
150151
val withExtraParams = TabEntity(tabId = "tab3", url = "https://duck.ai?chatID=abc&model=gpt&session=xyz")
151152
val withFragment = TabEntity(tabId = "tab4", url = "https://duck.ai?chatID=abc#message-5")
152153
whenever(mockTabRepository.getTabs()).thenReturn(listOf(original, withPath, withExtraParams, withFragment))
153-
whenever(mockDuckChat.isDuckChatUrl(any())).thenReturn(true)
154+
whenever(mockDuckChat.chatIdOrNull(any())).thenReturn("abc")
154155

155156
testee.onClearData(setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai?chatID=abc"))))
156157

@@ -161,7 +162,7 @@ class DuckAiTabsCleanupPluginTest {
161162
fun `Selected url with no chatID query param is a no-op`() = runTest {
162163
val anyTab = TabEntity(tabId = "tab1", url = "https://duck.ai?chatID=abc")
163164
whenever(mockTabRepository.getTabs()).thenReturn(listOf(anyTab))
164-
whenever(mockDuckChat.isDuckChatUrl(any())).thenReturn(true)
165+
whenever(mockDuckChat.chatIdOrNull(eq("https://duck.ai".toUri()))).thenReturn(null)
165166

166167
testee.onClearData(setOf(ClearableData.DuckChats.Selected(setOf("https://duck.ai"))))
167168

‎duckchat/duckchat-api/src/main/java/com/duckduckgo/duckchat/api/DuckChat.kt‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,12 @@ interface DuckChat {
6262
*/
6363
fun isDuckChatUrl(uri: Uri): Boolean
6464

65+
/**
66+
* Returns the chatID query parameter from [uri] when it is a Duck.ai chat URL with a
67+
* non-blank chatID; null otherwise (including for non-Duck.ai URLs).
68+
*/
69+
fun chatIdOrNull(uri: Uri): String?
70+
6571
/**
6672
* Returns `true` if Duck Chat was ever opened before.
6773
*/

‎duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/DuckChatConstants.kt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package com.duckduckgo.duckchat.impl
1818

1919
object DuckChatConstants {
2020
const val JS_MESSAGING_FEATURE_NAME = "aiChat"
21-
const val CHAT_ID_PARAM = "chatID"
2221
const val DUCK_AI_FEATURE_PAGE = "duckai"
2322

2423
object JsResponseKeys {

‎duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/RealDuckChat.kt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -836,6 +836,11 @@ class RealDuckChat @Inject constructor(
836836
return appendParameters(mapOf(CHAT_ID_QUERY_NAME to chatId), getDuckChatLink())
837837
}
838838

839+
override fun chatIdOrNull(uri: Uri): String? {
840+
if (!isDuckChatUrl(uri)) return null
841+
return uri.getQueryParameter(CHAT_ID_QUERY_NAME)?.takeIf { it.isNotBlank() }
842+
}
843+
839844
override suspend fun setDefaultTogglePosition(position: DefaultTogglePosition) {
840845
duckChatFeatureRepository.setDefaultTogglePosition(position.name)
841846
}

‎duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/clearing/DuckChatDataClearingPlugin.kt‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import com.duckduckgo.dataclearing.api.plugin.ClearableData
2222
import com.duckduckgo.dataclearing.api.plugin.DataClearingPlugin
2323
import com.duckduckgo.di.scopes.AppScope
2424
import com.duckduckgo.duckchat.api.DuckChat
25-
import com.duckduckgo.duckchat.impl.DuckChatConstants.CHAT_ID_PARAM
2625
import com.duckduckgo.duckchat.impl.repository.DuckChatFeatureRepository
2726
import com.duckduckgo.duckchat.impl.sync.DuckChatSyncRepository
2827
import com.duckduckgo.sync.api.engine.SyncEngine
@@ -74,7 +73,7 @@ class DuckChatDataClearingPlugin @Inject constructor(
7473
if (chatUrls.isEmpty()) return
7574
var anyDeleted = false
7675
chatUrls.forEach { chatUrl ->
77-
val chatId = extractChatId(chatUrl) ?: return@forEach
76+
val chatId = duckChat.chatIdOrNull(chatUrl.toUri()) ?: return@forEach
7877
if (duckChatDeleter.deleteChat(chatId)) {
7978
duckChatSyncRepository.recordSingleChatDeletion(chatId)
8079
anyDeleted = true
@@ -83,10 +82,4 @@ class DuckChatDataClearingPlugin @Inject constructor(
8382
// One sync trigger per user-visible delete action, not N.
8483
if (anyDeleted) syncEngine.triggerSync(SyncEngine.SyncTrigger.DATA_CHANGE)
8584
}
86-
87-
private fun extractChatId(url: String): String? {
88-
val uri = url.toUri()
89-
if (!duckChat.isDuckChatUrl(uri)) return null
90-
return uri.getQueryParameter(CHAT_ID_PARAM)?.takeIf { it.isNotBlank() }
91-
}
9285
}

‎duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/contextual/DuckChatContextualViewModel.kt‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import com.duckduckgo.anvil.annotations.ContributesViewModel
2424
import com.duckduckgo.common.utils.DispatcherProvider
2525
import com.duckduckgo.di.scopes.FragmentScope
2626
import com.duckduckgo.duckchat.api.DuckChat
27-
import com.duckduckgo.duckchat.impl.DuckChatConstants.CHAT_ID_PARAM
2827
import com.duckduckgo.duckchat.impl.DuckChatInternal
2928
import com.duckduckgo.duckchat.impl.feature.DuckChatFeature
3029
import com.duckduckgo.duckchat.impl.helper.DuckChatJSHelper
@@ -650,9 +649,8 @@ class DuckChatContextualViewModel @Inject constructor(
650649
}
651650

652651
private fun hasChatId(url: String?): Boolean {
653-
return url?.toUri()?.getQueryParameter(CHAT_ID_PARAM)
654-
.orEmpty()
655-
.isNotBlank()
652+
val uri = url?.toUri() ?: return false
653+
return duckChat.chatIdOrNull(uri) != null
656654
}
657655

658656
private suspend fun shouldReuseStoredChatUrl(tabId: String): Boolean {

‎duckchat/duckchat-impl/src/main/java/com/duckduckgo/duckchat/impl/inputscreen/ui/viewmodel/InputScreenViewModel.kt‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ import com.duckduckgo.common.utils.DispatcherProvider
4444
import com.duckduckgo.common.utils.SingleLiveEvent
4545
import com.duckduckgo.common.utils.extensions.toBinaryString
4646
import com.duckduckgo.duckchat.api.DuckAiFeatureState
47-
import com.duckduckgo.duckchat.impl.DuckChatConstants.CHAT_ID_PARAM
4847
import com.duckduckgo.duckchat.impl.DuckChatInternal
4948
import com.duckduckgo.duckchat.impl.feature.DuckAiChatHistoryFeature
5049
import com.duckduckgo.duckchat.impl.feature.DuckChatFeature
@@ -863,12 +862,7 @@ class InputScreenViewModel @AssistedInject constructor(
863862
saveLastUsedTogglePosition()
864863
duckChatJSHelper.clearTabContextPromptEvent()
865864
viewModelScope.launch {
866-
val url = duckChat.getDuckChatUrl("", false)
867-
.toUri()
868-
.buildUpon()
869-
.appendQueryParameter(CHAT_ID_PARAM, chatId)
870-
.build()
871-
.toString()
865+
val url = duckChat.buildChatUrl(chatId)
872866
command.value = Command.SubmitSearch(url)
873867

874868
if (pinned) {

0 commit comments

Comments
 (0)