Conversation
jprusik
added
the
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR closes several gaps in the Fill Targeting Rules implementation across the browser autofill pipeline: it stops Code Review DetailsNo findings. The unresolved threads from earlier rounds of review have been substantively addressed by later commits: the The iframe-routing match-count heuristic is known to be imperfect — an outer-hop iframe match counts toward |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20970 +/- ##
==========================================
+ Coverage 49.11% 49.16% +0.05%
==========================================
Files 4052 4052
Lines 126859 126955 +96
Branches 19348 19379 +31
==========================================
+ Hits 62304 62415 +111
+ Misses 59969 59949 -20
- Partials 4586 4591 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
audreyality
left a comment
audreyality
left a comment
There was a problem hiding this comment.
♻️ I'm concerned about how verbose this code is. I've pointed out a few places where this impedes readability and in one case it appears to be related to a bug.
| if ( | ||
| !elementIsFillableFormField(formFieldElement) || | ||
| autofillFieldData.inlineMenuFillType === CipherType.Card | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| autofillFieldData.targeted && | ||
| autofillFieldData.form && | ||
| this.targetedSubmitButtonsByFormOpid.has(autofillFieldData.form) | ||
| ) { | ||
| this.setupSubmitListenerOnTargetedField(formFieldElement, autofillFieldData.form); | ||
| return; | ||
| } | ||
|
|
||
| if (autofillFieldData.form) { | ||
| await this.setupSubmitListenerOnFieldWithForms(formFieldElement); | ||
| return; | ||
| } | ||
|
|
||
| await this.setupSubmitListenerOnFormlessField(formFieldElement); | ||
| return; |
There was a problem hiding this comment.
🎨 Consider using if/else if/else instead of early returns. This makes it more clear that this function is performing dispatch.
if (
!elementIsFillableFormField(formFieldElement) ||
autofillFieldData.inlineMenuFillType === CipherType.Card
) {
return;
} else if (
autofillFieldData.targeted &&
autofillFieldData.form &&
this.targetedSubmitButtonsByFormOpid.has(autofillFieldData.form)
) {
this.setupSubmitListenerOnTargetedField(formFieldElement, autofillFieldData.form);
} else if (autofillFieldData.form) {
await this.setupSubmitListenerOnFieldWithForms(formFieldElement);
} else {
await this.setupSubmitListenerOnFormlessField(formFieldElement);
}🤔 It also reveals a possible bug. The autofillFieldData.form branch should probably be:
await this.setupSubmitListenerOnFieldWithForms(autofillFieldData.form);There was a problem hiding this comment.
The autofillFieldData.form branch should probably be:
fwiw, my understanding of setupSubmitListenerOnFieldWithForms: this is meant to take a field and any form associated with it that isn't already in formElements is added to it and given submit event listeners
| for (const selector of container) { | ||
| if (typeof selector !== "string") { | ||
| continue; | ||
| } | ||
| if (this.domQueryService.findIframeCrossing(selector)) { | ||
| continue; | ||
| } | ||
| const match = this.domQueryService.queryDeepSelector(selector); | ||
| if (match instanceof HTMLElement) { | ||
| return match; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
🎨 Consider using an array method. Control flow is harder to follow than comparison logic. If you'd like to preserve the return match breakpoint, split the find and return onto different lines.
| for (const selector of container) { | |
| if (typeof selector !== "string") { | |
| continue; | |
| } | |
| if (this.domQueryService.findIframeCrossing(selector)) { | |
| continue; | |
| } | |
| const match = this.domQueryService.queryDeepSelector(selector); | |
| if (match instanceof HTMLElement) { | |
| return match; | |
| } | |
| } | |
| return null; | |
| const found = container.find( | |
| (s) => | |
| typeof selector s === "string" && | |
| !this.domQueryService.findIframeCrossing(selector) && | |
| this.domQueryService.queryDeepSelector(selector) instanceof HTMLElement | |
| ); | |
| return found ?? null; |
jprusik
force-pushed
the
pm-34491
branch
2 times, most recently
from
e161959 to
5c8e0b3
Compare
…d field inline menu typing
…ssword inline menu
… if a new password has not been filled
|
|
Apologies for the rebase - I had to pull in some important related changes that have been merged to |
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and the feature flag configuration used by ✅ Fortunately, these BIT tests have passed! 🎉 |
Changes in this PR impact the Autofill experience of the browser clientBIT has tested the core experience with these changes and all feature flags disabled. ✅ Fortunately, these BIT tests have passed! 🎉 |
| for (const selector of selectorAlternatives ?? []) { | ||
| if (matchCount >= maxMatches) { | ||
| break; | ||
| } | ||
|
|
||
| if (typeof selector !== "string") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎨 The number of continue branches in this loop suggest that pre-filtering selectorAlternatives would be worthwhile.
| form.htmlAction = isFormElement | ||
| ? (this.getFormActionAttribute(element as ElementWithOpId<HTMLFormElement>) ?? "") | ||
| : ""; | ||
| form.htmlMethod = isFormElement |
There was a problem hiding this comment.
⛏️ The type assertion is incorrect. getFormActionAttribute() never uses opid, however, so you can fix this by narrowing the argument's type to HtmlFormElement.
|
Remaining work has been carried over to #21364 - linking this pull request to preserve existing feedback. |
2 participants
🎟️ Tracking
PM-34491
📔 Objective
Fixed:
newPasswordselector arrays match up to two elements (instead of one); instead of stopping on the first new password field, fill will handle up to two new password fields if specified in the targeting rules.Deferred:
newPasswordtargeted fields no longer fill with the cipher's current password during a normal Login fill.(split to [PM-38992] Fix Targeting Rules autofilling new password inputs with current password values #21238)
newPasswordtargeted fields now map toInlineMenuFillTypes.PasswordGeneration(notCipherType.Login), so the inline menu offers the generator instead of the cipher list.(split to [PM-38992] Fix Targeting Rules autofilling new password inputs with current password values #21238)
generateTargetedFillScripthonorsPasswordGenerationby routingcipher.login.passwordinto targetednewPasswordfields, otherwise the menu would appear but fail to fill.(split to [PM-39000] Fix targeted new password field not filling upon click on generate password inline menu #21239)
Filling a generated password no longer prunes targetednewPasswordfields through heuristic filter before fill(split to [PM-39009] Do not do heuristic filtering on targeted fields #21245)
the inline menu save prompt no longer fires on "new password" fields just because the current-password field was filled on an update form.(split to [PM-39008] Do not show save new password inline menu over generate password menu if a new password has not been filled #21244)
autofill logic now leverages provided selectors for form "submit" actions (other actions and auto submit support not implemented)(split to PM-38476)