Skip to content

[PM-39807] fix: Gate fill-assist rules sync on user settings#7123

Merged
aj-rosado merged 1 commit into
mainfrom
PM-39807/fix-fill-assist-unintended-sync-call
Jun 30, 2026
Merged

[PM-39807] fix: Gate fill-assist rules sync on user settings#7123
aj-rosado merged 1 commit into
mainfrom
PM-39807/fix-fill-assist-unintended-sync-call

Conversation

@aj-rosado

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

FillAssistManagerImpl.syncIfNecessary() was fetching fill-assist targeting rules from the server as soon as the server config became available and the FillAssistTargetingRules feature flag was enabled — regardless of whether the user had opted into fill assist in their settings.

This PR gates the sync behind settingsRepository.isFillAssistEnabled, so rules are only fetched once the user explicitly enables fill assist. The check is short-circuited alongside the existing feature-flag guard in syncIfNecessary().

Changes:

  • Added var isFillAssistEnabled: Boolean to SettingsRepository / SettingsRepositoryImpl, backed by a new getFillAssistEnabled / storeFillAssistEnabled pair in SettingsDiskSource / SettingsDiskSourceImpl
  • Added settingsRepository: SettingsRepository to FillAssistManagerImpl and FillAssistModule
  • Updated syncIfNecessary() to return early when isFillAssistEnabled is false
  • Added test coverage for the new early-return path

@aj-rosado aj-rosado added the ai-review Request a Claude code review label Jun 30, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jun 30, 2026
@aj-rosado
aj-rosado marked this pull request as ready for review June 30, 2026 17:54
@aj-rosado
aj-rosado requested review from a team and david-livefront as code owners June 30, 2026 17:54
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR gates FillAssistManagerImpl.syncIfNecessary() behind a new per-user isFillAssistEnabled setting so fill-assist rules are only fetched after the user opts in. The new setting is plumbed through SettingsDiskSourceSettingsRepository and wired into the DI module, following the existing isInlineAutofillEnabled pattern closely, including logout cleanup. Test coverage for the new early-return path and the fake disk source are updated appropriately. One open question about whether the setting's writer is deferred to a follow-up PR.

Code Review Details
  • ❓ : isFillAssistEnabled has no production writer; the gate is always false, so rules are never fetched. Likely intentional staging behind the unreleased flag — confirming.
    • app/src/main/kotlin/com/x8bit/bitwarden/data/autofill/manager/FillAssistManagerImpl.kt:78

Comment on lines +78 to +82
if (!featureFlagManager.getFeatureFlag(FlagKey.FillAssistTargetingRules) ||
!settingsRepository.isFillAssistEnabled
) {
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUESTION: Is the writer for isFillAssistEnabled intentionally deferred to a follow-up PR?

Details

This change gates syncIfNecessary() on settingsRepository.isFillAssistEnabled, but nothing in production code ever sets that property to true. Across the codebase it is only: defined here, defaulted to false (getter returns false when unset), and cleared to null on logout. The parallel isInlineAutofillEnabled setting, by contrast, has a writer in AutoFillViewModel (AutoFillViewModel.kt:248).

As written, the gate will always be false, so fill-assist rules will never be fetched even when the FillAssistTargetingRules flag is on. If the settings toggle that flips this on lands in a later PR, this is fine; if not, the feature is effectively disabled. Confirming intent here.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.78%. Comparing base (d468197) to head (34f4466).

Files with missing lines Patch % Lines
...data/platform/repository/SettingsRepositoryImpl.kt 12.50% 7 Missing ⚠️
...platform/datasource/disk/SettingsDiskSourceImpl.kt 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7123      +/-   ##
==========================================
+ Coverage   86.39%   86.78%   +0.39%     
==========================================
  Files         884      909      +25     
  Lines       64383    65356     +973     
  Branches     9337     9353      +16     
==========================================
+ Hits        55622    56719    +1097     
+ Misses       5538     5412     -126     
- Partials     3223     3225       +2     
Flag Coverage Δ
app-data 17.23% <50.00%> (-0.34%) ⬇️
app-ui-auth-tools 18.69% <0.00%> (-0.15%) ⬇️
app-ui-platform 16.87% <0.00%> (+0.30%) ⬆️
app-ui-vault 28.23% <0.00%> (+0.53%) ⬆️
authenticator 6.16% <0.00%> (-0.01%) ⬇️
lib-core-network-bridge 4.04% <0.00%> (-0.01%) ⬇️
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 Harness.
📢 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.

@aj-rosado
aj-rosado added this pull request to the merge queue Jun 30, 2026
@aj-rosado

Copy link
Copy Markdown
Contributor Author

thanks @david-livefront

Merged via the queue into main with commit 7124ccb Jun 30, 2026
35 of 40 checks passed
@aj-rosado
aj-rosado deleted the PM-39807/fix-fill-assist-unintended-sync-call branch June 30, 2026 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants