[PM-41293] feat: Add identity field-key mapping to fill-assist - #7342
[PM-41293] feat: Add identity field-key mapping to fill-assist#7342aj-rosado wants to merge 7 commits into
Conversation
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the identity field-key mapping added to the fill-assist path in Code Review DetailsNo new findings. Prior open finding on the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7342 +/- ##
==========================================
- Coverage 86.16% 85.72% -0.45%
==========================================
Files 928 946 +18
Lines 67315 67852 +537
Branches 10156 10199 +43
==========================================
+ Hits 58002 58166 +164
- Misses 5735 6106 +371
- Partials 3578 3580 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| val fillAssistViews = assistStructure.buildFillAssistViews( | ||
| hostRules = hostRules, | ||
| urlBarWebsite = urlBarWebsite, | ||
| isIdentityAutofillEnabled = isIdentityAutofillEnabled, |
There was a problem hiding this comment.
Unfillable, unlike the heuristic path.
Details and fix
toEffectiveViews returns fillAssistViews unfiltered, and buildFillAssistViews is given all of the host's rules (including identity-category ones), so an identity-keyed node is now classified even when coversCurrentPartition matched on a login/card rule. parseInternal then does:
val effectiveFocusedView = effectiveViews.firstFocusedOrNull() ?: return AutofillRequest.Unfillable
val partition = when (effectiveFocusedView) {
...
is AutofillView.Identity -> return AutofillRequest.UnfillableTraceable path (flag on, fill-assist on, host has account-login rules plus identity field keys):
- User focuses a "Full name" field. Heuristics classify it
Identity(orUnused), so it is not inautofillViews;focusedViewfalls back toautofillViews.firstOrNull()— an unfocusedLogin.Username. coversCurrentPartitionis true (Login rule present), so fill-assist runs.fillAssistViewsnow containsIdentity.PersonNameFull(isFocused = true)alongsideLogin.Username/Login.Password.firstFocusedOrNull()returns the Identity view →Unfillable.
Before this change the identity key mapped to null, the node was omitted, and firstFocusedOrNull() fell back to the first Login view → a fillable Login partition. So turning the flag on removes login suggestions on these hosts.
This is the exact case the heuristic path guards against at line 141-144 (it !is AutofillView.Unused && it !is AutofillView.Identity), covered by parse should choose AutofillPartition Login when an Identity view is focused but a Login view is fillable elsewhere. The fill-assist path has no equivalent.
Mirroring that filter until Phase D would keep the two paths aligned, e.g. in parseInternal:
val effectiveFocusedView = effectiveViews
.filterNot { it is AutofillView.Identity }
.firstFocusedOrNull()
?: return AutofillRequest.UnfillableA parser-level test with a focused identity-keyed fill-assist node would lock this in.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 18s —— View job
Fix verified ✅
- Read the new commit since my review (
e75acee"Filter out autofill identity") - Check the focused-Identity →
Unfillablepath is closed - Check test coverage locks the behavior in
The fix is correct and complete. AutofillParserImpl.kt:186-189 now filters Identity out before resolving the effective focused view:
val effectiveFocusedView = effectiveViews
.filterNot { it is AutofillView.Identity }
.firstFocusedOrNull()
?: return AutofillRequest.UnfillableTracing the path I reported: with the flag on, the focused identity-keyed node is now skipped, firstFocusedOrNull()'s ?: firstOrNull() fallback (AutofillParserImpl.kt:337-338) lands on the fill-assist Login.Username, and the request stays a fillable Login partition. The two paths are aligned — this mirrors the heuristic filter at AutofillParserImpl.kt:141-144.
No regressions I can see from the filter itself:
- Fill-assist off:
autofillViewsalready excludesIdentity, sofilterNotis a no-op. - Only identity views present: filtered list is empty →
Unfillable, same as before.
The new test parse should choose AutofillPartition Login when fill-assist resolves the focused view to Identity but a Login view is fillable elsewhere (AutofillParserTests.kt:1789+) exercises exactly the scenario — focused Unused node reclassified to Identity.PersonNameFull by a rule that also carries a username key — and asserts the Login partition rather than Unfillable. It would fail without the filterNot. Fixtures all check out against the existing test setup.
One nit (non-blocking)
AutofillParserImpl.kt:205-208 — the is AutofillView.Identity -> return AutofillRequest.Unfillable branch is now unreachable, since Identity can no longer reach effectiveFocusedView. The branch still has to exist for when exhaustiveness, but the comment now reads as if it's a live path. Consider matching the tone of the adjacent Unused branch:
is AutofillView.Identity -> {
// Unreachable: Identity views are filtered out of the focused-view lookup above.
// Identity partition construction lands in Phase D.
return AutofillRequest.Unfillable
}./gradlew :app:testStandardDebugUnitTest in this job — the Gradle command was blocked by the tool permission gate — so my verification is by code reading only. CI test results are authoritative.
• PM-41293/identity-autofill-fill-assist-field-mapping
| val view = candidateViews | ||
| .firstOrNull { (_, view) -> view is AutofillView.Login.Username } | ||
| ?.second | ||
| ?: candidateViews.firstOrNull()?.second | ||
| ?: return@let null |
There was a problem hiding this comment.
Details and fix
candidateViews holds one entry per matched field key, in hostRules.flatMap { it.fields.entries } order. The fallback is candidateViews.firstOrNull(), which has no notion of Identity being the weakest classification.
Traceable path (flag on):
- A node matches both an identity key and a non-
username/phonelogin or card key — e.g.personNameFullandemail— because two of the host's clauses select it (a loose clause such asinput[type=text]is enough). - No
Login.Usernamecandidate exists, socandidateViews.firstOrNull()wins. - If the identity key is declared first,
viewisIdentity.PersonNameFull. parseInternalbuilds the partition witheffectiveViews.filterIsInstance<AutofillView.Login>(), so that node is silently absent — the email field gets no suggestion.
With the flag off, toIdentityViewForFieldKey returns null, the identity key is dropped from candidateViews, and the same node classifies as Login.Email. So flipping the flag removes a login/card field from the partition, the same class of flag-on regression as the focused-Identity case fixed in AutofillParserImpl.
Login.Username is already preferred for a reason; extending that ordering to deprioritize Identity keeps Login/Card authoritative while Identity partitions are unbuilt:
val view = candidateViews
.firstOrNull { (_, view) -> view is AutofillView.Login.Username }
?.second
?: candidateViews.firstOrNull { (_, view) -> view !is AutofillView.Identity }?.second
?: candidateViews.firstOrNull()?.second
?: return@let nullA test with a node matched under both an identity key (listed first) and email would lock this in.
There was a problem hiding this comment.
Claude finished @aj-rosado's task in 1m 6s —— View job
Fix verified ✅
- Read commit
d1c1bb4"Prioritize login/card classifications over identity" - Verify the Identity-outranks-Login/Card path is closed
- Check test coverage locks it in
The tie-break in FillAssistViewNodeExtensions.kt:84-90 now demotes Identity below every other classification:
val view = candidateViews
.firstOrNull { (_, view) -> view is AutofillView.Login.Username }
?.second
?: candidateViews.firstOrNull { (_, view) -> view !is AutofillView.Identity }
?.second
?: candidateViews.firstOrNull()?.second
?: return@let nullTracing the path I reported — node matched under personNameFull (declared first) and email: no Login.Username candidate, so the new middle clause picks Login.Email instead of Identity.PersonNameFull. isLoginIdentifierView is then true, matchedKeys contains email, so Identity.Email is appended — the node lands in the Login partition and keeps its identity candidate. That's the behavior the new test asserts.
No regressions I can find:
- Flag off —
toIdentityViewForFieldKeyis skipped, socandidateViewsholds no Identity entries and the newfilterNot-style clause is a no-op. - Only identity keys match — both preferential clauses miss,
candidateViews.firstOrNull()still returns the Identity view, so plain identity-only nodes classify exactly as before. - Relative order among non-Identity candidates —
firstOrNullon the filtered view preserves declaration order, so existing Login/Card tie-breaks are untouched. - Card keys — a node matched under an identity key plus
cardNumbernow resolves toCard.Number, which is the intended direction.
FillAssistViewNodeExtensionsTest.kt:458+ (buildFillAssistViews should prefer Login Email over Identity PersonNameFull when identity key is listed first and matches the same node) uses linkedMapOf so the identity key genuinely comes first, and createHtmlInfo() matches any clause so both keys hit the same node. It would fail on the pre-fix code (candidateViews.firstOrNull() → Identity.PersonNameFull). Fixture helpers all line up with the existing suite.
One nit (non-blocking, Phase D)
An identity classification can now be silently dropped when the same node also matches a login/card key that isn't in the dual-classify set. E.g. a node matched under phoneFull + email resolves to Login.Email + Identity.Email, and the Identity.PhoneFull candidate disappears — the dual-classify block only checks the "email" and "phone" keys (FillAssistViewNodeExtensions.kt:100-105), not "phoneFull". No user-visible effect today since identity partitions aren't built, but worth revisiting when Phase D starts consuming these views.
./gradlew :app:testStandardDebugUnitTest in this job — the Gradle command was blocked by the tool permission gate — so this verification is by code reading only. CI results are authoritative.
• PM-41293/identity-autofill-fill-assist-field-mapping
1 participant
🎟️ Tracking
PM-41293
📔 Objective
Adds identity field-key mapping to the fill-assist targeting path, extending
FillAssistViewNodeExtensionsto resolve the 16 identityAutofillViewsubtypes (name, address, phone, company, SSN, passport, license) from host-rule field keys, alongside the existing Login/Card mapping.All identity classification produced by fill-assist — the plain identity field-key mapping and the email/phone dual-classification (offering an email- or phone-matched field as both a Login and an Identity candidate) — is gated behind
FlagKey.IdentityAutofill, mirroring the heuristic detection path's gating. This keeps pre-existing behavior unchanged whenever the flag is off, even if a host's fill-assist rules declare identity field keys. The dual-classification also now considers the full set of matched field keys for a node rather than only the one that wins the Login.Username/Login.Email tie-break, so a field matched under both"email"and"phone"is classified under both Identity types.Also retires
"autocomplete"from the heuristic attribute-hint list inHtmlInfoExtensions.kt— the platform doesn't actually return this attribute to the app, so it was dead weight in the hint list. Unrelated to fill-assist, but intentionally included in this PR.📸 Screenshots
N/A — no UI changes.