[PM-37255] feat: Add fill-assist targeting rules data layer by aj-rosado · Pull Request #6991 · bitwarden/android

aj-rosado · 2026-05-29T12:48:21Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-37255

📔 Objective

Adds the data layer for caching and managing fill-assist targeting rules, including CSS selector parsing and the self-managing sync manager.

Changes:

FillAssistRules — domain model with HostRule and SelectorClause(tag, id, name, type, role); CSS selectors are parsed at sync time, not at autofill time
FillAssistDiskSource / FillAssistDiskSourceImpl — per-server SharedPreferences storage (keyed by fillAssistRulesUrl); includes CURRENT_CACHE_VERSION migration to invalidate cached data when parsing logic changes
FillAssistManager / FillAssistManagerImpl — self-managing singletonthat syncs on serverConfigStateFlow changes; uses a 6-hour timestamp throttle before checking the manifest CID; CSS forms parsed from all pathnames, pooled and merged by category (one HostRule per form type per host); shadow DOM selectors (>>>) use the last segment for matching
FillAssistModule — Hilt DI bindings

codecov · 2026-05-29T12:52:55Z

Codecov Report

❌ Patch coverage is 76.42857% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.34%. Comparing base (f57a7d0) to head (61db0cd).

Files with missing lines	Patch %	Lines
...den/data/autofill/manager/FillAssistManagerImpl.kt	72.50%	9 Missing and 24 partials ⚠️

Additional details and impacted files

@@                          Coverage Diff                           @@
##           PM-37255/fill-assist-network-layer    #6991      +/-   ##
======================================================================
- Coverage                               86.47%   86.34%   -0.14%     
======================================================================
  Files                                     873      878       +5     
  Lines                                   63670    63892     +222     
  Branches                                 9234     9272      +38     
======================================================================
+ Hits                                    55059    55166     +107     
- Misses                                   5436     5527      +91     
- Partials                                 3175     3199      +24

Flag	Coverage Δ
app-data	`17.36% <76.42%> (+0.13%)`	⬆️
app-ui-auth-tools	`18.98% <0.00%> (-0.05%)`	⬇️
app-ui-platform	`16.45% <0.00%> (-0.04%)`	⬇️
app-ui-vault	`27.75% <0.00%> (-0.07%)`	⬇️
authenticator	`6.19% <0.00%> (-0.03%)`	⬇️
lib-core-network-bridge	`4.09% <0.00%> (-0.02%)`	⬇️
lib-data-ui	`1.14% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…assist-data-layer

…emoved unnecessary deserialization tests

…assist-data-layer

…ow updates Updating code to schema with required values

david-livefront · 2026-06-10T15:55:26Z

+        serverConfigRepository.serverConfigStateFlow
+            .onEach { config ->
+                environmentDiskSource.fillAssistRulesUrl =
+                    config?.serverData?.environment?.fillAssistRulesUrl


Why store the same data in two places?

We cannot use ServerConfigRepository on the BaseUrlInterceptor as it would cause a circular dependency. Stored on environmentDiskSource to avoid it.

I think that despite storing same data in two places, it is cleaner and more consistent with our code than the other approach of passing the url

david-livefront · 2026-06-10T15:57:11Z

+        )
+    }.also { result ->
+        result.onFailure { Timber.w(it, "Fill-assist sync failed") }
+    }


The also is redundant, you can just do this:

... fillAssistDiskSource.storeLastFetchTimestamp( serverUrl = serverUrl, timestamp = clock.millis(), ) } .onFailure { Timber.w(it, "Fill-assist sync failed") }

david-livefront · 2026-06-10T15:59:40Z

+        val parsedFields = form.fields
+            .mapValues { (_, elem) -> parseCompositeSelectorArray(elem) }
+            .filterValues { it.isNotEmpty() }
+            .takeIf { it.isNotEmpty() } ?: return@forEach


The ?: return@forEach should be on it's own line

Also, should the return be a continue?

Had to confirm it but it acts like a continue on this scenario, continue here returns an error
'break' and 'continue' are only allowed inside loops.

https://kotlinlang.org/docs/returns.html#return-to-labels

david-livefront · 2026-06-10T16:05:15Z

+private fun parseCompositeSelectorArray(element: JsonElement): List<SelectorClause> {
+    if (element !is JsonArray) return emptyList()
+    val result = mutableListOf<SelectorClause>()
+    for (item in element) {


Can we make this more functional?

private fun parseCompositeSelectorArray(element: JsonElement): List<SelectorClause> { if (element !is JsonArray) return emptyList() return element.flatMap { item -> when (item) { is JsonPrimitive -> listOfNotNull(parseSingleSelector(item.content)) is JsonArray -> { item .filterIsInstance<JsonPrimitive>() .mapNotNull { parseSingleSelector(it.content) } } else -> emptyList() } } }

david-livefront · 2026-06-10T16:08:56Z

+    forms: List<FillAssistFormsJson.FormJson>,
+): Map<String, MutableMap<String, MutableList<SelectorClause>>> {
+    val result = mutableMapOf<String, MutableMap<String, MutableList<SelectorClause>>>()
+    forms.forEach { form ->


This one looks a bit more complicated but could this also be made more functional... Using groupBy might do the trick

david-livefront · 2026-06-10T16:12:58Z

+            "id" -> id = attrValue
+            "name" -> name = attrValue
+            "type" -> type = attrValue
+            "role" -> role = attrValue


Can we make these constants?

david-livefront · 2026-06-10T16:17:15Z

+    val allForms = buildList {
+        addAll(hostEntry.forms.orEmpty())
+        hostEntry.pathnames?.values?.filterNotNull()?.forEach { addAll(it.forms) }
+    }.distinct()


What do you think of this?

val allForms = buildList { addAll(hostEntry.forms.orEmpty()) addAll(hostEntry.pathnames?.values?.filterNotNull()?.flatMap { it.forms }.orEmpty()) } .distinct()

david-livefront · 2026-06-10T16:18:16Z

+
+    ATTRIBUTE_REGEX.findAll(effective).forEach { match ->
+        val attrName = match.groupValues[1]
+        val attrValue = match.groupValues[2]


Can we add some additional comments here to explain what is actually happening.

This all seems rather odd (Autofill usually is that way lol)

We are still processing the data returned from the server forms. Basically this PR is processing the server data into something that can be useful to use when Autofilling.
Added a comment with an example that should make it easier to understand.

github-actions · 2026-06-11T14:33:28Z

Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the data layer for fill-assist targeting rules: per-server disk source with cache-version migration, a self-managing sync manager driven by serverConfigStateFlow, and a CSS selector parser producing storage-ready FillAssistRules. Implementation follows established Bitwarden patterns (BaseDiskSource, Hilt DI, Result-returning network calls, Clock injection) and is well-tested across migration, scoping, sync gating, and parser edge cases. The previous reviewer (david-livefront) provided substantive feedback that the author has already addressed; remaining unresolved threads contain either author rationale (e.g., the fillAssistRulesUrl write rationale for avoiding circular DI) or are marked outdated.

Code Review Details

No new findings beyond what has already been raised in the existing review threads. The code is consistent with the codebase, the per-server scoping is correctly implemented (the removeWithPrefix("fillAssistRules_") prefix avoids colliding with fillAssistRulesUrl), the migration runs once on singleton init, and the CSS parser has good unit-test coverage for shadow DOM, compound selectors, and pure-class rejection.

david-livefront · 2026-06-11T15:07:25Z

+
+        val forms = fillAssistService
+            .getForms(filename = versionEntry.filename)
+            .getOrThrow()


Can we not throw here.

val forms = fillAssistService .getForms(filename = versionEntry.filename) .getOrNull() ?: return@runCatching

david-livefront · 2026-06-11T15:07:55Z

+    }
+
+    private suspend fun sync(serverUrl: String) = runCatching {
+        val manifest = fillAssistService.getManifest().getOrThrow()


Same, let's handle all these error cases more gracefully, we shouldn't be throwing everything

david-livefront · 2026-06-11T15:08:32Z

+            val parsedFields = form.fields
+                .mapValues { (_, elem) -> parseCompositeSelectorArray(elem) }
+                .filterValues { it.isNotEmpty() }
+                .takeIf { it.isNotEmpty() } ?: return@mapNotNull null


Formatting:

.takeIf { it.isNotEmpty() } ?: return@mapNotNull null

…assist-data-layer

aj-rosado added 3 commits May 29, 2026 12:03

Add fill assist rules network data

c646372

reverted unwanted changes on AuthRepositoryTest

f57a7d0

Add fill assist data layer

8b6ce8b

aj-rosado added the ai-review Request a Claude code review label May 29, 2026

github-actions Bot added app:password-manager Bitwarden Password Manager app context and removed ai-review Request a Claude code review labels May 29, 2026

aj-rosado changed the title ~~Add fill assist data layer~~ [PM-37255] feat: Add fill-assist targeting rules data layer May 29, 2026

chained code on FillAssistManager

00b2479

github-actions Bot added the t:feature Change Type - Feature Development label May 29, 2026

aj-rosado added 8 commits May 29, 2026 14:07

Merge branch 'PM-37255/fill-assist-network-layer' into PM-37255/fill-…

61db0cd

…assist-data-layer

Removed nulls and sets on non nullable fields by schema definition. R…

f66485f

…emoved unnecessary deserialization tests

following autofill assist forms schema

0b39ad2

removed unnecessary null set

b25404e

Added FillAssist to BaseUrlInterceptors

31a9cf7

Merge branch 'PM-37255/fill-assist-network-layer' into PM-37255/fill-…

bbdc2f1

…assist-data-layer

updating environmentDiskSource.fillAssistUrl when serverConfigStateFl…

ad2aada

…ow updates Updating code to schema with required values

Merge branch 'main' into PM-37255/fill-assist-network-layer

d6f0a47

david-livefront reviewed Jun 10, 2026

View reviewed changes

Improved code readability

6c0bfa5

aj-rosado requested a review from david-livefront June 11, 2026 14:32

aj-rosado marked this pull request as ready for review June 11, 2026 14:32

aj-rosado requested a review from a team as a code owner June 11, 2026 14:32

david-livefront reviewed Jun 11, 2026

View reviewed changes

Addressing pr comments

8b05d40

aj-rosado requested a review from david-livefront June 11, 2026 15:55

Merge branch 'PM-37255/fill-assist-network-layer' into PM-37255/fill-…

1db11d1

…assist-data-layer

Base automatically changed from PM-37255/fill-assist-network-layer to main June 15, 2026 11:04

Conversation

aj-rosado commented May 29, 2026

🎟️ Tracking

📔 Objective

Uh oh!

codecov Bot commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitwarden Claude Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 29, 2026 •

edited

Loading

github-actions Bot commented Jun 11, 2026 •

edited

Loading