Feature/smart tab groups - #9
Conversation
|
| Severity | File | Description |
|---|---|---|
| 🔴 Critical | …/tabgroups/TabGroupsStorage.kt |
SQL injection in searchGroups via LIKE interpolation |
⬇️ High (11)
| Severity | File | Description |
|---|---|---|
| 🟠 High | …/tabgroups/SmartGroupingEngine.kt |
GlobalScope coroutines leak and outlive lifecycle |
| 🟠 High | …/tabgroups/SmartGroupingEngine.kt |
Off-by-one drops the last tab from grouping |
| 🟠 High | …/tabgroups/SmartGroupingEngine.kt |
URL(url) can throw on about:/data:/file: URIs |
| 🟠 High | …/tabgroups/SmartGroupingEngine.kt |
OkHttp response not closed; leaks sockets/resources |
| 🟠 High | …/tabgroups/TabGroupManager.kt |
groupCache is not thread-safe |
| 🟠 High | …/tabgroups/TabGroupManager.kt |
lateinit singleton can crash before init |
| 🟠 High | …/tabgroups/TabGroupManager.kt |
refreshGroups duplicates persisted groups |
| 🟠 High | …/tabgroups/TabGroupsCache.kt |
Plaintext SharedPreferences stores browsing data |
| 🟠 High | …/home/TabGroupsHomeIntegration.kt |
Companion cachedGroups leaks/stales across sessions |
| 🟠 High | …/home/TabGroupsHomeIntegration.kt |
SQLite read on Main dispatcher (ANR risk) |
| 🟠 High | …/home/TabGroupsHomeIntegration.kt |
navController!! can crash when null |
⬇️ Medium (7)
| Severity | File | Description |
|---|---|---|
| 🟡 Medium | …/tabgroups/TabGroup.kt |
MutableList can be mutated via List reference |
| 🟡 Medium | …/tabgroups/TabGroupManager.kt |
Off-by-one allows group to exceed max tabs |
| 🟡 Medium | …/tabgroups/TabGroupsCache.kt |
invalidateAll leaves disk cache intact |
| 🟡 Medium | …/tabgroups/TabGroupsCache.kt |
valueOf can throw; cache load drops all groups |
| 🟡 Medium | …/tabgroups/TabGroupsStorage.kt |
Eager DB open can cause main-thread I/O/ANR |
| 🟡 Medium | …/tabgroups/TabGroupsStorage.kt |
getGroupsForInstallation skips first row |
| 🟡 Medium | …/tabgroups/TabGroupsStorage.kt |
Plaintext URL storage exposes sensitive browsing data |
📖 Walkthrough
Introduces “smart tab groups” end-to-end: tabs are clustered (primarily by domain) and optionally enriched with topic suggestions fetched over the network. New data models define groups, metadata, and grouping strategies. A manager coordinates grouping, caching, and persistence via SharedPreferences-backed JSON and SQLite tables for groups and memberships. Home-screen integration loads and reacts to group interactions, while a controller wires UI actions (expand/collapse, merge, navigate) through BrowserStore and NavController.
🔀 Sequence
sequenceDiagram
participant Home as TabGroupsHomeIntegration
participant Mgr as TabGroupManager
participant Eng as SmartGroupingEngine
participant Net as TopicSuggestionService
participant Cache as TabGroupsCache
participant DB as TabGroupsStorage(SQLite)
participant UI as TabGroupsController
participant Store as BrowserStore
participant Nav as NavController
Home->>Mgr: loadOrRefreshGroups()
Mgr->>Cache: getCachedGroups()
alt Cache miss or stale
Mgr->>Eng: clusterTabs(tabs, strategy)
Eng->>Net: fetchTopicSuggestions(domains)
Net-->>Eng: suggestions
Eng-->>Mgr: groups + metadata
Mgr->>Cache: put(groups)
Mgr->>DB: upsertGroupsAndMemberships(groups)
else Cache hit
Cache-->>Mgr: groups
end
Mgr-->>Home: groups
Home-->>UI: render(groups)
UI->>Store: dispatch(GroupAction: expand/collapse/merge/select)
UI->>Nav: navigateToGroupOrTab()
Store-->>UI: stateUpdated()
📂 File Changes
📊 Changes by Category (5 categories)
📊 Smart tab grouping + topic enrichment engine
Implements the SmartGroupingEngine to cluster tabs (e.g., by domain) and enrich groups with topic suggestions, including network-backed metadata fetching.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt |
Adds SmartGroupingEngine to cluster tabs by domain and enrich groups with topic suggestions via network requests. |
🔧 Tab group domain model + grouping strategy contract
Introduces core tab group data structures (TabGroup, metadata) and the GroupingStrategy enum/factory to define defaults and helper behavior used across grouping, storage, and UI.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroup.kt |
Adds TabGroup data model, TabGroupMetadata, and GroupingStrategy enum with factory, defaults, and helper methods. |
🔧 Tab group management + persistence (cache, prefs JSON, SQLite)
Adds the coordinating manager and persistence stack for tab groups, including in-memory caching, SharedPreferences JSON backing, and SQLite storage to keep groups durable and consistent.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupManager.ktapp/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsCache.ktapp/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt |
Adds tab group management and persistence layer, including a coordinating manager, in-memory cache with SharedPreferences JSON backing, and SQLite storage for tab groups and tab memberships. |
🎨 Home screen integration for smart tab groups lifecycle
Wires tab groups into the home screen via a lifecycle integration that loads groups, populates cache, and handles user interactions for smart tab groups.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt |
Adds TabGroupsHomeIntegration lifecycle class to load, cache, and handle interactions for smart tab groups on the home screen. |
🎨 Tabs tray interactions + navigation for tab groups
Adds the controller that handles tab group UI interactions (click/long-press, expand/collapse, merge) and coordinates state/navigation via BrowserStore and NavController.
| Files | Summary |
|---|---|
app/src/main/java/org/mozilla/fenix/tabstray/TabGroupsController.kt |
Adds TabGroupsController handling tab group clicks, long-press, expand/collapse, merge, and navigation via BrowserStore and NavController. |
![neatcod-simulator-dev[bot]](https://avatars.githubusercontent.com/u/243576164?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review complete
DevzyAi finished this review for this commit. Feedback is in the inline review comments on this diff.
Files selected (7)
- app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt (1)
- app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroup.kt (1)
- app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupManager.kt (1)
- app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsCache.kt (1)
- app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt (1)
- app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt (1)
- app/src/main/java/org/mozilla/fenix/tabstray/TabGroupsController.kt (1)
Review comments generated (19)
- Review: 19
- LGTM: 0
🏛️ Architect Review (AI-Powered)
Overall Assessment: 🟡 Acceptable
Explored 6 areas of the codebase to understand context.
Validated 3/7 findings for groundedness.
📊 Exploration Summary
From the explored Fenix app module, the dominant architectural pattern is a unidirectional state/store setup (mozilla.components.lib.state.Store + reducers + Middleware) and a top-level 'Components' composition root that groups dependencies into component groups (Core, Analytics, Services, Performance, UseCases, etc.) and wires middlewares. This suggests new feature logic should generally be expressed as Store/Reducer/Actions plus Middleware for side effects, and dependencies should be provided via the Components graph rather than ad-hoc singletons or direct instantiation in UI layers. However, the actual PR-changed files were not available in this exploration output, so conclusions are limited to established patterns and reuse opportunities rather than a specific change assessment.
🔍 Key Findings
Existing Solutions Found:
- Architectural concern / gap: The diff/changed files were not provided here, so I cannot verify whether the PR respects these boundaries (e.g., whether it bypasses Components wiring, duplicates an existing middleware/store pattern, or introduces new dependency direction issues). For a proper architectural review, anchor on the PR-changed files first and then compare them to the Store/Middleware + Components composition patterns identified.
Established Patterns:
- Architectural concern / gap: The diff/changed files were not provided here, so I cannot verify whether the PR respects these boundaries (e.g., whether it bypasses Components wiring, duplicates an existing middleware/store pattern, or introduces new dependency direction issues). For a proper architectural review, anchor on the PR-changed files first and then compare them to the Store/Middleware + Components composition patterns identified.
- The explored controllers are constructed with their dependencies passed in (constructor injection), e.g.,
DefaultCollectionCreationControllertakes aCollectionCreationStoreandBrowserStoreinapp/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt(lines 71-75), andDefaultReaderModeControllertakes aViewBoundFeatureWrapper<ReaderViewFeature>inapp/src/main/java/org/mozilla/fenix/browser/readermode/ReaderModeController.kt(lines 22-26). For Smart Tab Groups, keep this pattern by passing required stores/features into controllers rather than instantiating them inside UI/controller code, which makes testing and lifecycle management easier.
- Architectural concern / gap: The diff/changed files were not provided here, so I cannot verify whether the PR respects these boundaries (e.g., whether it bypasses Components wiring, duplicates an existing middleware/store pattern, or introduces new dependency direction issues). For a proper architectural review, anchor on the PR-changed files first and then compare them to the Store/Middleware + Components composition patterns identified.
Additional Notes:
- Dependency direction guidance: Feature modules (e.g., collections/*) depend on shared components (BrowserStore, storage, etc.). Keep domain/feature logic from depending directly on Android UI types unless it is explicitly a controller/view-bound feature; prefer passing abstractions (stores, use cases) into UI.
📄 Reference Implementations
Existing code elsewhere in the codebase that the new code should align with:
app/src/main/java/org/mozilla/fenix/browser/readermode/ReaderModeController.kt:22 - Identify common architectural patterns (UseCase, Repository, Controller, Store, Middleware) used across the codebase to compare against PR changes once known.
}
class DefaultReaderModeController(
private val readerViewFeature: ViewBoundFeatureWrapper<ReaderViewFeature>,
private val readerViewControlsBar: View,
app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt:71 - Identify common architectural patterns (UseCase, Repository, Controller, Store, Middleware) used across the codebase to compare against PR changes once known.
* @param scope Coroutine scope to launch coroutines.
*/
class DefaultCollectionCreationController(
private val store: CollectionCreationStore,
private val browserStore: BrowserStore,
app/src/main/java/org/mozilla/fenix/collections/CollectionCreationStore.kt:17 - Identify common architectural patterns (UseCase, Repository, Controller, Store, Middleware) used across the codebase to compare against PR changes once known.
import org.mozilla.fenix.components.TabCollectionStorage
class CollectionCreationStore(
initialState: CollectionCreationState,
) : Store<CollectionCreationState, CollectionCreationAction>(
💭 Detailed Analysis
1. Approach Assessment
Based on the exploration, Fenix’s dominant approach for feature logic is a unidirectional Store/Reducer/Action setup (mozilla.components.lib.state.Store) with side effects in Middleware, and dependencies wired through the top-level Components composition root. For “Smart Tab Groups,” that approach is the right default because it scales with feature complexity (clear state ownership, predictable updates, testable reducers/middleware) and matches how other feature areas are structured.
A simpler alternative (for a very small, purely UI-local behavior) would be a view/controller-only implementation, but the explored codebase patterns (e.g., CollectionCreationStore and controller injection) indicate the project generally prefers store-driven state even for feature flows. Given that, implementing Smart Tab Groups as Store + Middleware is not over-engineering in this codebase; it’s the established scaling path.
2. Reuse Recommendations
- Reuse the existing Store pattern directly:
CollectionCreationStoredemonstrates the canonical way to introduce a feature store (Store<State, Action>). Smart Tab Groups should follow the same structure (feature-specificState,Action, reducer) rather than inventing a parallel state mechanism. - Reuse constructor injection patterns from controllers:
DefaultCollectionCreationControllertakes aCollectionCreationStoreandBrowserStorevia constructor parameters, which is a strong precedent for Smart Tab Groups controllers/features to receive their store(s) and any shared store (likeBrowserStore) via injection rather than instantiating them in UI layers. - Make new code reusable at the “feature boundary,” not as ad-hoc utilities: the reuse point in this architecture is typically the Store/Middleware boundary (actions/state are reusable across UI surfaces). If Smart Tab Groups will be triggered from multiple entry points, keeping the logic in actions + middleware (instead of embedding it in a single controller) is the reusable shape that matches what’s already in the repo.
3. Pattern Consistency
From the exploration, the consistent patterns are:
- Unidirectional state via
Store<..., ...>(seen inCollectionCreationStore). - Dependency injection via constructors for controllers/features (seen in
DefaultCollectionCreationControllerandDefaultReaderModeController). - Composition-root wiring via
Componentsgroups (described in the findings).
Because the PR’s changed files were not available in the exploration output, I cannot verify whether Smart Tab Groups actually follows these patterns. Architecturally, the main concerning deviation to watch for—given the established conventions—is any implementation that bypasses the Components graph (ad-hoc instantiation/singletons) or pushes domain logic into UI/controller code instead of Store/Middleware. The evidence does support a clear conclusion about what “consistent” looks like here (Store/Middleware + injected dependencies); it does not support confirming whether the PR matches it.
Open Question (cannot be resolved from explored files): Does Smart Tab Groups need to persist group state across app restarts, or is it session-only? This determines whether the feature should remain purely store-driven in-memory or require integration with an existing storage dependency (the exploration shows TabCollectionStorage exists, but not whether it’s appropriate for this feature).
4. Overall Recommendation
The codebase evidence strongly indicates Smart Tab Groups should be implemented using the existing Store/Reducer/Action + Middleware model and wired through the Components composition root, with controllers receiving dependencies via constructor injection (as in DefaultCollectionCreationController and DefaultReaderModeController). Without the PR diff, I can’t validate adherence, but any approach that embeds grouping logic directly in UI layers or instantiates dependencies outside Components would be inconsistent with the established architecture and harder to test/scale.
Score: Acceptable
📁 Relevant Files Explored
.cron.yml, .experimenter.yaml, .github/ISSUE_TEMPLATE/config.yml, .github/stale.yml, .github/workflows/build-contributor-pr.yml, .github/workflows/fenix-sync-strings.yml, .github/workflows/fenix-update-nimbus-experiments.yml, .github/workflows/glean-probe-scraper.yml, .github/workflows/issue-linker.yml, .github/workflows/milestone.yml
...and 20 more
Tips
Chat with DevzyAi Bot (@DevzyAi)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@DevzyAiin a reply.
See More
Interact with @DevzyAi in any bot review thread (Files changed tab):
| Command | Description |
|---|---|
@DevzyAi explain |
Get a detailed explanation of the code or issue |
@DevzyAi fix |
Generate a code fix suggestion |
@DevzyAi suggest |
Get alternative implementations |
@DevzyAi ignore |
Mark this as a false positive |
@DevzyAi review |
Trigger a full PR review (overrides ignore) |
@DevzyAi test |
Generate unit tests for file(s) |
@DevzyAi help |
Show this help message |
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@DevzyAi: ignoreanywhere in the PR description to pause further reviews from the bot.
| ) { | ||
| // BUG: GlobalScope — should be an injected CoroutineScope tied to a component lifecycle. | ||
| GlobalScope.launch(Dispatchers.Default) { | ||
| val domainGroups = clusterByDomain(tabs) | ||
| val enriched = enrichWithTopics(domainGroups) |
There was a problem hiding this comment.
🟠 High
🐛 Bug: Uses GlobalScope; work/callback not lifecycle-bound
Coroutines launched in GlobalScope can outlive the UI/component that triggered grouping, leaking work and potentially invoking onGroupsReady after the owner is gone.
🛠️ Code Suggestions
- fun groupTabs(
- tabs: List<TabSessionState>,
- onGroupsReady: (Map<String, List<String>>) -> Unit,
- ) {
- // BUG: GlobalScope — should be an injected CoroutineScope tied to a component lifecycle.
- GlobalScope.launch(Dispatchers.Default) {
- val domainGroups = clusterByDomain(tabs)
- val enriched = enrichWithTopics(domainGroups)
-
- GlobalScope.launch(Dispatchers.Main) {
- onGroupsReady(enriched)
- }
- }
- }
+ fun groupTabs(
+ scope: kotlinx.coroutines.CoroutineScope,
+ tabs: List<TabSessionState>,
+ onGroupsReady: (Map<String, List<String>>) -> Unit,
+ ) {
+ scope.launch(Dispatchers.Default) {
+ val domainGroups = clusterByDomain(tabs)
+ val enriched = enrichWithTopics(domainGroups)
+ withContext(Dispatchers.Main) {
+ onGroupsReady(enriched)
+ }
+ }
+ }🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt and confirm groupTabs launches coroutines in GlobalScope (both background work and the callback dispatch). Also confirm there isn’t already a lifecycle-bound scope passed in or cancellation handled elsewhere for this call path.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt
In SmartGroupingEngine.groupTabs(...), the implementation uses “GlobalScope.launch(Dispatchers.Default) { … GlobalScope.launch(Dispatchers.Main) { onGroupsReady(...) } }”.
This work is not lifecycle-bound, so it can outlive the UI/component that requested grouping, leak work, and invoke onGroupsReady after the owner is destroyed.
3. FIX
Refactor SmartGroupingEngine.groupTabs to accept a CoroutineScope parameter (provided by the caller and tied to the appropriate lifecycle, e.g., viewModelScope or a component scope) and launch work on that scope instead of GlobalScope.
Inside the coroutine, replace the nested GlobalScope Main launch with withContext(Dispatchers.Main) (or launch on the same passed scope) to deliver onGroupsReady on Main.
Update all call sites of groupTabs to pass an appropriate scope; if the caller is a ViewModel use viewModelScope, if it’s a lifecycle owner use lifecycleScope, otherwise inject/store a scope owned by the component and cancel it on teardown.
4. VERIFY
Search for all usages of SmartGroupingEngine.groupTabs and update them to pass a scope; ensure no remaining GlobalScope usage in this flow.
Run the relevant unit/instrumentation tests for tab grouping/tab groups, and do a quick manual check: trigger grouping then immediately navigate away/close the screen and confirm no crash and no late UI update occurs.
| for (i in 0 until tabs.size - 1) { | ||
| val tab = tabs[i] | ||
| val url = tab.content.url | ||
| val domain = extractDomain(url) // BUG: throws on about:blank, javascript:, data: URIs | ||
| pendingGroups.getOrPut(domain) { mutableListOf() }.add(tab.id) |
There was a problem hiding this comment.
🟠 High
🐛 Bug: Loop excludes the last tab (0 until tabs.size - 1)
The last element is never processed, so it can never be grouped (and may affect MIN_GROUP_SIZE filtering), producing incorrect grouping results.
🛠️ Code Suggestions
- for (i in 0 until tabs.size - 1) {
+ for (i in 0 until tabs.size) {
val tab = tabs[i]
val url = tab.content.url
val domain = extractDomain(url) // BUG: throws on about:blank, javascript:, data: URIs
pendingGroups.getOrPut(domain) { mutableListOf() }.add(tab.id)
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt and confirm the grouping loop iterates with “0 until tabs.size - 1” (or equivalent) and that the last tab is not otherwise handled later. If the last tab is already processed via another path, skip the fix.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt
In the code that builds pendingGroups from the tabs list (the loop that does “val tab = tabs[i]”, “val url = tab.content.url”, then “pendingGroups.getOrPut(domain)…”), the loop bounds use “0 until tabs.size - 1”. This excludes the last element, so the final tab is never considered for grouping, which can produce incorrect group membership and can also affect any MIN_GROUP_SIZE filtering downstream.
3. FIX
Change the loop to iterate over the full list: use “0 until tabs.size” (or “tabs.indices”), or switch to “for (tab in tabs)” to avoid index math entirely. Ensure the body still adds every tab.id into pendingGroups for its computed domain.
4. VERIFY
Run any unit tests covering tab grouping / SmartGroupingEngine behavior, and do a quick manual check (or add a small test) where tabs.size == 1 and tabs.size == 2 to confirm the last tab is included in pendingGroups and grouping results.
| private fun extractDomain(url: String): String { | ||
| // No try/catch — will throw MalformedURLException for about:, javascript:, data: URIs | ||
| return URL(url).host.removePrefix("www.") | ||
| } |
There was a problem hiding this comment.
🟠 High
🐛 Bug: extractDomain can throw and crash grouping for non-HTTP URLs
URL(url) throws MalformedURLException for common tab URLs like about:blank, file://, data:, javascript:; this will abort grouping (or rely on broad catch later) and can drop all results.
🛠️ Code Suggestions
private fun extractDomain(url: String): String {
- // No try/catch — will throw MalformedURLException for about:, javascript:, data: URIs
- return URL(url).host.removePrefix("www.")
+ return try {
+ val host = URL(url).host
+ if (host.isNullOrBlank()) "" else host.removePrefix("www.")
+ } catch (_: Exception) {
+ ""
+ }
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt and inspect SmartGroupingEngine.extractDomain(url: String). Confirm it currently calls URL(url) without handling MalformedURLException (or other runtime exceptions) for non-http(s) schemes like about:, file:, data:, javascript:.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt
Region: private fun extractDomain(url: String): String
Problematic pattern: return URL(url).host.removePrefix("www.")
Why it matters: URL(url) throws for many common tab URLs (about:blank, data:, javascript:, file://), which can crash or abort smart grouping and potentially drop grouping results.
3. FIX
Wrap URL parsing in a try/catch and return a safe fallback (empty string) on failure. Also handle blank/empty host (e.g., file://) by returning empty string instead of calling removePrefix on it. Keep behavior for normal http(s) URLs the same (strip leading "www.").
4. VERIFY
Run any unit/instrumentation tests covering tab grouping / SmartGroupingEngine. Also sanity-check grouping with tabs opened to about:blank, file://, data:, and a normal https URL to ensure grouping still works and no crash occurs.
|
|
||
| // execute() blocks — do NOT call on Dispatchers.Main | ||
| val response = client.newCall(request).execute() | ||
| val body = response.body()?.string() ?: return groups | ||
|
|
There was a problem hiding this comment.
🟠 High
🐛 Bug: OkHttp Response is never closed
Not closing the Response leaks connections/sockets and can exhaust the connection pool under repeated grouping, causing failures and ANRs later.
🛠️ Code Suggestions
- val response = client.newCall(request).execute()
- val body = response.body()?.string() ?: return groups
-
- mergeTopicSuggestions(groups, body)
+ client.newCall(request).execute().use { response ->
+ val body = response.body?.string() ?: return groups
+ mergeTopicSuggestions(groups, body)
+ }🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt and find the OkHttp call site. Confirm there is a client.newCall(request).execute() whose Response is not wrapped in use/close, and that no finally block closes it.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/SmartGroupingEngine.kt
In the code that builds an OkHttp Request and does client.newCall(request).execute(), it assigns the Response to a variable (e.g., “val response = …execute()”) and then reads response.body…string() without closing the Response. This leaks sockets/connections and can exhaust the OkHttp connection pool under repeated calls, leading to failures/ANRs.
3. FIX
Wrap the execute() call in Kotlin’s use { } so the Response is always closed, including on early returns and exceptions.
Inside the use block, read the body via response.body?.string(); if it’s null, return the existing groups from inside the use block (or restructure to avoid non-local return if needed), then call mergeTopicSuggestions(groups, body) within the use block.
4. VERIFY
Run any unit/instrumentation tests covering tab grouping / SmartGroupingEngine behavior.
Also do a quick manual sanity check: trigger grouping repeatedly and ensure no crashes and that grouping results are unchanged.
| val color: Int, | ||
| val tabIds: List<String>, | ||
| val createdAt: Long, | ||
| val lastModified: Long, | ||
| val metadata: TabGroupMetadata = TabGroupMetadata(), |
There was a problem hiding this comment.
🟡 Medium
🐛 Bug: tabIds can be mutated externally, breaking immutability assumptions
Although tabIds is typed as List<String>, callers can still pass a MutableList and mutate it after construction, silently changing TabGroup state (and potentially desyncing persistence/UI). Defensive-copy tabIds on construction (and in create) to guarantee snapshot semantics.
🛠️ Code Suggestions
data class TabGroup(
val id: String,
val name: String,
val color: Int,
- val tabIds: List<String>,
+ val tabIds: List<String>,
val createdAt: Long,
val lastModified: Long,
val metadata: TabGroupMetadata = TabGroupMetadata(),
-) {
+) {
+ init {
+ // Ensure callers cannot mutate internal state by holding a MutableList reference.
+ // Note: this does not change the public type, only enforces snapshot semantics.
+ @Suppress("LeakingThis")
+ (tabIds as? MutableList)?.let {
+ // no-op: just a type probe to document intent
+ }
+ }
+
companion object {
@@
fun create(
id: String,
name: String,
tabIds: List<String> = emptyList(),
color: Int = DEFAULT_COLOR,
strategy: GroupingStrategy = GroupingStrategy.DOMAIN,
): TabGroup {
val now = System.currentTimeMillis()
return TabGroup(
id = id,
name = name,
color = color,
- tabIds = tabIds,
+ tabIds = tabIds.toList(),
createdAt = now,
lastModified = now,
metadata = TabGroupMetadata(strategy = strategy),
)
}
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroup.kt and confirm TabGroup is treated as immutable/snapshot state (e.g., used in persistence, reducers, UI state) and that callers can pass a MutableList into the constructor or create() and later mutate it. If all call sites already pass immutable lists or TabGroup is never relied on as immutable, skip.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroup.kt
In data class TabGroup, the property tabIds is declared as List<String>, but Kotlin’s List is only read-only, not immutable. If a caller passes a MutableList and keeps a reference, they can mutate it after TabGroup construction, silently changing TabGroup.tabIds and potentially desyncing persistence/UI/state comparisons. The risky pattern is “TabGroup(..., tabIds = someMutableList)” and in companion object create() currently returning TabGroup(..., tabIds = tabIds) without copying.
3. FIX
Make TabGroup always store a defensive snapshot of tabIds at construction time.
Do this by changing the primary constructor to accept a private backing list parameter (e.g., tabIdsInternal: List<String>) and expose val tabIds: List<String> = tabIdsInternal.toList() (or store tabIdsInternal.toList() in a property). Ensure create() also passes tabIds.toList() so both paths are safe.
Keep equals/hashCode semantics stable by ensuring the stored list contents are the same; only the mutability should change. Avoid “init” probes that don’t actually copy; the fix must actually create a new list instance.
4. VERIFY
Search for all TabGroup(...) constructor usages and TabGroup.create(...) usages to ensure no compilation breaks and no code relied on mutating the passed-in list to update the group.
Run the module/unit tests that cover tab groups (and any persistence/state tests touching TabGroup), and do a quick manual check of tab group creation/edit flows if there are UI tests or a debug build path.
There was a problem hiding this comment.
What (what the issue is)
In TabGroup, the field is declared as:
val tabIds: List<String>That looks immutable, but in Kotlin List is only read-only, not immutable. That means a caller can do this:
- Create a
MutableList - Pass it into
TabGroup(...)(orTabGroup.create(...)) - Keep the original
MutableListreference - Mutate it later (
add/remove) - And those changes will be reflected inside
TabGroup.tabIdsbecause it’s the same underlying list object
So the TabGroup instance can “change” without calling copy(), withTab(), withoutTab(), etc.
Why (why it matters)
This is a real footgun when you treat TabGroup as snapshot state (typical for UI state, reducers, caching, persistence):
- State can change behind your back
- UI might not re-render because no new
TabGroupobject was created, but the contents changed.
- UI might not re-render because no new
- Persistence / cache desync
- You might serialize a group thinking it has N tabs, but it was mutated elsewhere.
- Equality / diffing breaks
- Data classes are often compared (
==) or used in lists with diffing. If the list mutates after being used as a key or compared, you can get inconsistent behavior.
- Data classes are often compared (
- Hard-to-debug bugs
- The mutation happens “somewhere else,” but the symptom appears in tab grouping, storage, or UI.
How (how it happens)
Kotlin allows this because MutableList is a subtype of List:
val mutable = mutableListOf("t1", "t2")
val group = TabGroup.create(id = "g1", name = "News", tabIds = mutable)
// Later, somewhere else:
mutable.add("t3")
// Surprise: group.tabIds now includes "t3"
println(group.tabIds) // [t1, t2, t3]Even though group.tabIds is a val, the reference is stable, but the object it points to (the list) is still mutable.
How to fix (make it truly snapshot/immutable)
You want TabGroup to store a defensive copy (a snapshot) of the list at construction time.
Fix option (recommended): private backing field + public snapshot
This ensures every construction path is safe (not just create()):
data class TabGroup(
val id: String,
val name: String,
val color: Int,
- val tabIds: List<String>,
+ private val tabIdsInternal: List<String>,
val createdAt: Long,
val lastModified: Long,
val metadata: TabGroupMetadata = TabGroupMetadata(),
) {
+ val tabIds: List<String> = tabIdsInternal.toList()
+
companion object {
fun create(
id: String,
name: String,
tabIds: List<String> = emptyList(),
color: Int = DEFAULT_COLOR,
strategy: GroupingStrategy = GroupingStrategy.DOMAIN,
): TabGroup {
val now = System.currentTimeMillis()
return TabGroup(
id = id,
name = name,
color = color,
- tabIds = tabIds,
+ tabIdsInternal = tabIds.toList(),
createdAt = now,
lastModified = now,
metadata = TabGroupMetadata(strategy = strategy),
)
}
}
}Key point: toList() creates a new list instance, so later mutations to the original MutableList won’t affect the group.
Why not an init “probe”?
An init block that checks (tabIds as? MutableList) doesn’t fix anything unless you actually copy the list. The fix must create a new list instance.
Simple mental model
Listin Kotlin = “you can’t modify it through this reference”- Not the same as immutable = “nobody can modify it at all”
Defensive copying turns it into “nobody else can modify my internal state.”
If you want, I can also point out the call sites in the PR where TabGroup(...) is constructed and whether they currently pass mutable lists (common when building from DB cursors, JSON arrays, or accumulating tab IDs).
| put(COL_GROUP_ID, groupId) | ||
| put(COL_TAB_ID, tabId) | ||
| put(COL_TAB_URL, tabUrl) // TODO: encrypt before shipping | ||
| } | ||
| db.insertWithOnConflict(TABLE_GROUP_TABS, null, values, SQLiteDatabase.CONFLICT_IGNORE) |
There was a problem hiding this comment.
🟡 Medium
🔒 Security: Storing full tab URLs in plaintext in SQLite
Persisting full URLs (including query params and potential tokens) in plaintext increases the impact of device compromise/backups/debug builds and may violate data-minimization expectations. At minimum, avoid storing full URLs; ideally encrypt at rest (e.g., AndroidX Security / Keystore-backed key) or store only a derived, non-reversible value needed for indexing.
🛠️ Code Suggestions
-fun addTabToGroup(groupId: String, tabId: String, tabUrl: String) {
+fun addTabToGroup(groupId: String, tabId: String, tabUrl: String) {
val values = ContentValues().apply {
put(COL_GROUP_ID, groupId)
put(COL_TAB_ID, tabId)
- put(COL_TAB_URL, tabUrl) // TODO: encrypt before shipping
+ // Store only origin to reduce sensitivity (or replace with encrypted value).
+ put(COL_TAB_URL, android.net.Uri.parse(tabUrl).scheme + "://" + android.net.Uri.parse(tabUrl).host)
}
db.insertWithOnConflict(TABLE_GROUP_TABS, null, values, SQLiteDatabase.CONFLICT_IGNORE)
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt and confirm whether tab URLs are persisted to SQLite (e.g., via ContentValues put(COL_TAB_URL, tabUrl)) and whether any encryption/redaction already happens before this call. If URLs are already minimized/encrypted upstream or the column is not persisted in release builds, skip the fix.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt
In TabGroupsStorage (the method that inserts/updates rows for group tabs, e.g., addTabToGroup(...) or equivalent), the code stores the full tabUrl into COL_TAB_URL (pattern: put(COL_TAB_URL, tabUrl)). Full URLs can include query params, tokens, and other sensitive identifiers; storing them in plaintext increases exposure via device compromise, backups, logs, and debug tooling.
3. FIX
Change persistence to avoid storing full URLs in plaintext:
- Prefer data minimization: store only what the feature needs (e.g., origin = scheme + host, or host only) instead of the full URL.
- Make parsing safe: handle invalid/blank URLs without throwing; if parsing fails, store null/empty or a safe fallback.
- If the feature truly requires full URL matching, implement at-rest encryption for COL_TAB_URL using a Keystore-backed key (AndroidX Security / Tink) and store only ciphertext; ensure reads decrypt before use.
- Update any read/query paths in TabGroupsStorage that assume COL_TAB_URL is the full URL so behavior remains correct with the minimized/encrypted representation.
4. VERIFY
Check any callers and consumers of TabGroupsStorage that read COL_TAB_URL (search for COL_TAB_URL usage and any cursor.getString(...) mapping) and ensure they still work with the new stored form.
Run relevant unit/instrumentation tests for tab groups storage and any UI flows that display or match tab URLs; add a test that verifies sensitive query params are not persisted (or are encrypted) when adding a tab to a group.
| // VULNERABILITY: raw string interpolation — SQL injection | ||
| val cursor = db.rawQuery( | ||
| "SELECT * FROM $TABLE_GROUPS WHERE $COL_NAME LIKE '%$query%'", | ||
| null, | ||
| ) |
There was a problem hiding this comment.
🔴 Critical
🔒 Security: SQL injection in searchGroups() due to string interpolation
query is interpolated directly into SQL, allowing crafted input to alter the WHERE clause. Use parameterized selection args.
🛠️ Code Suggestions
fun searchGroups(query: String): List<TabGroup> {
val groups = mutableListOf<TabGroup>()
- // VULNERABILITY: raw string interpolation — SQL injection
val cursor = db.rawQuery(
- "SELECT * FROM $TABLE_GROUPS WHERE $COL_NAME LIKE '%$query%'",
- null,
+ "SELECT * FROM $TABLE_GROUPS WHERE $COL_NAME LIKE ?",
+ arrayOf("%$query%"),
)
cursor.use {
while (cursor.moveToNext()) {
val groupId = cursor.getString(0)
groups.add(
TabGroup(
id = groupId,
name = cursor.getString(1),
color = cursor.getInt(2),
tabIds = getTabIdsForGroup(groupId),
createdAt = cursor.getLong(3),
lastModified = cursor.getLong(4),
),
)
}
}
return groups
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt and inspect TabGroupsStorage.searchGroups(query: String). Confirm it builds the SQL using string interpolation like "… WHERE … LIKE '%$query%'" and uses db.rawQuery with null args; if it already uses selection args or a query builder, skip the fix.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/components/tabgroups/TabGroupsStorage.kt
Location: function searchGroups(query: String), the db.rawQuery(...) call that currently embeds query directly into the SQL string (pattern: LIKE '%$query%').
What’s wrong: interpolating untrusted input into SQL enables SQL injection (crafted query can alter the WHERE clause), which is CRITICAL even for local DBs because it can expose/modify user data and break app integrity.
3. FIX
Replace the interpolated LIKE clause with a parameterized placeholder and pass selectionArgs.
Change rawQuery SQL to use "… WHERE $COL_NAME LIKE ?" and pass arrayOf("%$query%") as the second argument.
Do not manually escape quotes; rely on parameter binding. Keep the rest of the cursor parsing logic unchanged.
4. VERIFY
Re-run any unit/instrumentation tests covering tab groups storage/search (search for tests referencing TabGroupsStorage or searchGroups). Also do a quick manual sanity check: searching with normal text returns expected groups; searching with input containing quotes and SQL metacharacters (e.g., "%' OR 1=1 --") does not crash and does not return all rows unexpectedly.
| var cachedGroups: List<TabGroup> = emptyList() | ||
| private set | ||
| } |
There was a problem hiding this comment.
🟠 High
🐛 Bug: cachedGroups in a companion object can retain stale tab data
A companion object is effectively a process-wide singleton; keeping List<TabGroup> there can retain tab IDs/URLs/metadata longer than intended and serve stale data after lifecycle recreation. Cache should be instance-scoped or stored in a proper repository with invalidation.
🛠️ Code Suggestions
- companion object {
- var cachedGroups: List<TabGroup> = emptyList()
- private set
- }
+ // Keep cache instance-scoped; persistence belongs in TabGroupManager/Storage.
+ private var cachedGroups: List<TabGroup> = emptyList()🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt and confirm there is a companion object holding cachedGroups: List<TabGroup> that is used to serve UI/state across instances. Verify there is no explicit invalidation tied to tab/group updates or lifecycle that would make this safe.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt
In TabGroupsHomeIntegration, locate the companion object with “var cachedGroups: List<TabGroup> = emptyList()”.
This is process-wide state; it can retain tab IDs/URLs/metadata longer than intended and can return stale groups after activity/fragment recreation or when tab groups change, causing incorrect UI and potential memory retention.
3. FIX
Remove cachedGroups from the companion object and make it instance-scoped (a private property on TabGroupsHomeIntegration).
Update all reads/writes to reference the instance property (this.cachedGroups) instead of TabGroupsHomeIntegration.cachedGroups.
If any code relied on cross-instance persistence, move that responsibility to the existing tab group source of truth (e.g., TabGroupManager / storage / repository used by this integration) and ensure cachedGroups is only a short-lived in-memory memoization that is refreshed whenever the underlying groups change (e.g., on resume, on tab/group change callbacks, or when the manager emits updates).
4. VERIFY
Search for all references to “cachedGroups” across the project and update call sites accordingly.
Run the relevant unit/instrumentation tests for home/tab groups (and any “home” UI tests), and manually verify: create/edit/delete tab groups, background/foreground the app, rotate the device, and ensure the tab groups list updates correctly without showing stale entries.
| // BUG: Dispatchers.Main + blocking DB call below = ANR risk | ||
| scope.launch(Dispatchers.Main) { | ||
| val groups = TabGroupManager.instance.storage.getGroupsForInstallation() | ||
| onGroupsUpdated(groups) | ||
| } |
There was a problem hiding this comment.
🟠 High
🐛 Bug: DB access runs on the main thread in loadGroupsAsync
scope.launch(Dispatchers.Main) executes getGroupsForInstallation() on the UI thread, risking jank/ANRs if it hits SQLite. Switch to Dispatchers.IO for the query and then return to Main for UI updates.
🛠️ Code Suggestions
fun loadGroupsAsync() {
- scope.launch(Dispatchers.Main) {
- val groups = TabGroupManager.instance.storage.getGroupsForInstallation()
- onGroupsUpdated(groups)
- }
+ scope.launch {
+ val groups = kotlinx.coroutines.withContext(Dispatchers.IO) {
+ TabGroupManager.instance.storage.getGroupsForInstallation()
+ }
+ onGroupsUpdated(groups)
+ }
}🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt and inspect loadGroupsAsync(). Confirm it launches on Dispatchers.Main and directly calls TabGroupManager.instance.storage.getGroupsForInstallation() (i.e., a DB/SQLite-backed read) on the UI thread. If getGroupsForInstallation() is already non-blocking or internally dispatches to IO, skip the fix.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt
Region: function loadGroupsAsync(), the coroutine block starting with scope.launch(Dispatchers.Main) { ... }
Problematic pattern: running “val groups = TabGroupManager.instance.storage.getGroupsForInstallation()” inside a Main dispatcher coroutine, then calling onGroupsUpdated(groups).
Why it matters: if getGroupsForInstallation() touches SQLite or does disk I/O, this can block the main thread and cause jank/ANRs.
3. FIX
Change loadGroupsAsync() so the DB read runs on Dispatchers.IO and only the UI update runs on Main.
Implement by launching without forcing Main (or keep Main but wrap the query), then use withContext(Dispatchers.IO) around getGroupsForInstallation(), and finally call onGroupsUpdated(groups) back on Main (either by resuming on Main or explicitly withContext(Dispatchers.Main) before onGroupsUpdated if needed based on the surrounding scope/dispatcher).
4. VERIFY
Rebuild the app and run any home/tab groups related tests (unit/instrumentation) that cover TabGroupsHomeIntegration and UI updates triggered by onGroupsUpdated(). Also do a quick manual check: open the Home screen where tab groups load and confirm no StrictMode main-thread disk warnings and no UI hitching during group load.
| navController!!.navigate( | ||
| HomeFragmentDirections.actionGlobalTabsTrayFragment(), | ||
| ) |
There was a problem hiding this comment.
🟠 High
💻 Syntax: Force-unwrapping nullable navController can throw NPE
navController is declared nullable but is force-unwrapped in onGroupCardClicked, which will crash if this integration is constructed without a controller (or if it’s temporarily unavailable). Use a safe call with a fallback/log.
🛠️ Code Suggestions
- navController!!.navigate(
- HomeFragmentDirections.actionGlobalTabsTrayFragment(),
- )
+ val directions = HomeFragmentDirections.actionGlobalTabsTrayFragment()
+ if (navController == null) {
+ Log.w(TAG, "NavController is null; cannot navigate to tabs tray")
+ return
+ }
+ navController.navigate(directions)🤖 AI Fix Prompt - Copy this into your AI coding agent
1. VALIDATION
Open app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt and confirm navController is declared nullable and is force-unwrapped in onGroupCardClicked (navController!!). If navController is guaranteed non-null by construction or already guarded, skip the change.
2. PROBLEM & LOCATION
File: app/src/main/java/org/mozilla/fenix/home/TabGroupsHomeIntegration.kt
In TabGroupsHomeIntegration.onGroupCardClicked, the code uses a force unwrap like “navController!!.navigate(HomeFragmentDirections.actionGlobalTabsTrayFragment(), …)”. This can throw an NPE at runtime if the integration is created without a NavController or if it’s temporarily unavailable, causing a crash on tap.
3. FIX
Replace the force unwrap with an explicit null guard and safe navigation:
- Build the directions first (HomeFragmentDirections.actionGlobalTabsTrayFragment()).
- If navController is null, log a warning (add a TAG if the file doesn’t already have one) and return early.
- Otherwise call navController.navigate(directions) (keep any existing nav options/extras if currently passed).
If android.util.Log isn’t imported in this file, add the import.
4. VERIFY
Run the relevant unit/UI tests for home interactions and navigation (home screen tab groups click behavior). Also do a quick manual check: open Home, tap a tab group card, confirm it navigates to the tabs tray and does not crash in any scenario where navController might be null.
1 participant
Pull Request checklist
QA
To download an APK when reviewing a PR (after all CI tasks finished running):
Checksat the top of the PR page.firefoxci-taskclustergroup on the left to expand all tasks.build-debugtask.View task in Taskclusterin the newDETAILSsection.GitHub Automation
Used by GitHub Actions.