Skip to content

[PM-43254] Decouple settings from the repository API - #1455

Open
dani-garcia wants to merge 5 commits into
mainfrom
ps/decouple-settings-from-repo
Open

[PM-43254] Decouple settings from the repository API#1455
dani-garcia wants to merge 5 commits into
mainfrom
ps/decouple-settings-from-repo

Conversation

@dani-garcia

@dani-garcia dani-garcia commented Sep 4, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

Update Setting to be a first class citizen rather than rely on Repository. Instead, the Setting type now builds upon a trait, which is currently implemented by directly wrapping the DB like Repository does. This will open up the possibility of having client-managed settings in the future, but it's not part of this PR.

Also added a blanket Persist trait to avoid duplicating the Serialize+Deserialize+Send+Sync+'static bounds everywhere.

🚨 Breaking Changes

Settings no longer share a single `Repository<SettingItem>`. Each `Setting<T>`
resolves to its own backend, bound to one key at construction, so a client can
own an individual setting without taking over storage for the rest. The
SDK-managed backend sits beside `DBRepository` and is reached through
`SystemDatabase::get_setting`, mirroring `get_repository`.

`PersistentValue` names the bound shared by `RepositoryItem` and the setting
APIs.
@dani-garcia dani-garcia added the ai-review Request a Claude code review label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the refactor that moves Setting<T> off Repository<SettingItem> onto a dedicated SettingTrait backend, plus the blanket Persist bound and the new MemorySetting test helper. The on-disk contract is preserved: DBSetting reads and writes the bare serialized value in the Setting table keyed by the setting name — the same path DBRepository<SettingItem> took — and the new registry test asserts that explicitly, with SettingItem::data() still in get_sdk_managed_migrations. StateRegistry::setting now goes straight to the SDK-managed database instead of the client-managed-first get::<SettingItem>(); that is behavior-preserving today, since SettingItem is absent from create_client_managed_repositories! and no crate registers a client-managed repository for it (test_accounts.rs and tests/register.rs register only LocalUserDataKeyState). Consumers (StateClient::setting, FlagsClient, PasswordManagerTokenHandler, send pending-deletions, sync last_sync, CLI invalidate_session_key) keep the same Setting<T> surface, SettingsError::RepositoryDatabase stays inside #[error(transparent)] wrapping so error text is unchanged, and both client breaking-change checks are green.

Code Review Details

No findings at or above the reporting threshold.

The follow-up commits since the previous review were checked individually: the PersistentValuePersist rename is mechanical, and PhantomData<fn() -> T>PhantomData<T> on DBSetting is safe because Persist already requires Send + Sync + 'static, so the auto-trait and dropck implications are the same as DBRepository.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: ps/decouple-settings-from-repo (61923b6)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ✅ No breaking changes detected Compilation passed with new SDK version - View Details
android ✅ No breaking changes detected Compilation passed with new SDK version - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.53086% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.17%. Comparing base (e56c281) to head (1f65d86).

Files with missing lines Patch % Lines
crates/bitwarden-test/src/setting.rs 81.81% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1455   +/-   ##
=======================================
  Coverage   86.17%   86.17%           
=======================================
  Files         534      535    +1     
  Lines       79747    79790   +43     
=======================================
+ Hits        68719    68758   +39     
- Misses      11028    11032    +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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dani-garcia
dani-garcia marked this pull request as ready for review September 9, 2026 14:38
@dani-garcia
dani-garcia requested review from a team as code owners September 9, 2026 14:38
@dani-garcia dani-garcia changed the title refactor(state): Decouple settings from the repository API [PM-43254] Decouple settings from the repository API Sep 9, 2026
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant