Skip to content

[PM-38443] feat: Add fill-assist per-account storage and constants#2811

Merged
andrebispo5 merged 21 commits into
mainfrom
pm-38443/fill-assist-storage
Jun 29, 2026
Merged

[PM-38443] feat: Add fill-assist per-account storage and constants#2811
andrebispo5 merged 21 commits into
mainfrom
pm-38443/fill-assist-storage

Conversation

@andrebispo5

@andrebispo5 andrebispo5 commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Adds per-account fillAssistCachedData and fillAssistLastFetchTimestamp storage keys to AppSettingsStore (following the existing serverConfig pattern), fill-assist constants (fillAssistFormsVersion = "v1", fillAssistUpdateInterval = 6h, fillAssistExpectedSchemaMajor = "1"), and clears fill-assist data on account logout.

Depends on #2810 (PR 1/3). This is PR 2/3 for PM-38443. PR 3/3 adds the repository and sync integration.

@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:feature labels Jun 19, 2026
@andrebispo5
andrebispo5 marked this pull request as ready for review June 19, 2026 17:27
@andrebispo5
andrebispo5 requested review from a team and matt-livefront as code owners June 19, 2026 17:27
Copilot AI review requested due to automatic review settings June 19, 2026 17:27
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of per-account fillAssistCachedData and fillAssistLastFetchTimestamp storage to AppSettingsStore, the matching mock implementations, the logout-clearing in StateService, and the new fill-assist constants. The changes mirror the existing serverConfig storage pattern precisely (Codable-backed store/fetch helpers, alphabetically-ordered logout clears, mock dictionary semantics). No security, correctness, or breaking-change concerns were found.

Code Review Details

No findings. The implementation correctly follows the established serverConfig storage pattern: the generic store helper removes the UserDefaults key on nil, the new logout clears are placed in alphabetical order alongside the other per-account keys, and the mock setters match the existing setServerConfig semantics. FillAssistCachedData is Codable, satisfying the storage helper constraints.

Copilot AI left a comment

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.

Pull request overview

Adds new per-account persistence hooks and shared constants to support Fill-Assist caching and fetch cadence, and ensures Fill-Assist persisted data is cleared on logout. This extends the existing AppSettingsStore per-account storage pattern (e.g., serverConfig) in preparation for the repository/sync work in the next PR.

Changes:

  • Added Fill-Assist constants (fillAssistFormsVersion, fillAssistUpdateInterval, fillAssistExpectedSchemaMajor) to shared Constants.
  • Added per-account fillAssistCachedData and fillAssistLastFetchTimestamp getters/setters to AppSettingsStore (plus mock + default implementations).
  • Cleared Fill-Assist persisted values during account logout in StateService.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
BitwardenShared/Core/Platform/Utilities/Constants.swift Introduces shared Fill-Assist constants for versioning and update interval.
BitwardenShared/Core/Platform/Services/Stores/TestHelpers/MockAppSettingsStore.swift Adds mock storage + accessors for new per-account Fill-Assist values.
BitwardenShared/Core/Platform/Services/Stores/AppSettingsStore.swift Extends the settings store protocol and default implementation with per-account Fill-Assist persistence keys.
BitwardenShared/Core/Platform/Services/StateService.swift Clears Fill-Assist persisted data on account logout.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.20%. Comparing base (61cc961) to head (e76798b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2811      +/-   ##
==========================================
+ Coverage   81.19%   81.20%   +0.01%     
==========================================
  Files        1026     1026              
  Lines       65799    65817      +18     
==========================================
+ Hits        53425    53447      +22     
+ Misses      12374    12370       -4     

☔ 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.

Comment on lines +159 to +168
// MARK: Fill-Assist

/// The forms map major version key requested from the manifest (e.g. `"v1"`).
static let fillAssistFormsVersion = "v1"

/// The minimum interval between fill-assist manifest checks (6 hours, matching the browser implementation).
static let fillAssistUpdateInterval: TimeInterval = 6 * 60 * 60

/// The expected major component of the forms schema version (e.g. `"1"` for `"1.0.0"`).
static let fillAssistExpectedSchemaMajor = "1"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🎨 If you agree I'd like to start adding nested enums here in constants for things that are related like this so we avoid the prefix repetition. We're already doing that with UTType. E.g.

extension Constants {

    /// Fill-Assist constants used by the app.
    ///
    enum FillAssist {
         /// The forms map major version key requested from the manifest (e.g. `"v1"`).
         static let formsVersion = "v1"
          /// ....
    }

So then we can use them by Constants.FillAssist.formsVersion.
I feel like it's better organized this way.

Swift synthesizes CompactDecodable<Int> in the memberwise init rather
than [String: Int]?, so direct literal assignment fails to compile.
fedemkr
fedemkr previously approved these changes Jun 29, 2026
Base automatically changed from pm-38443/fill-assist-api-and-models to main June 29, 2026 14:57
@andrebispo5
andrebispo5 dismissed stale reviews from fedemkr and morganzellers-bw June 29, 2026 14:57

The base branch was changed.

@andrebispo5
andrebispo5 merged commit eaa024d into main Jun 29, 2026
20 of 21 checks passed
@andrebispo5
andrebispo5 deleted the pm-38443/fill-assist-storage branch June 29, 2026 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:password-manager Bitwarden Password Manager app context t:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants