Skip to content

[PM-36839] Prepare API base Error#1049

Merged
dani-garcia merged 13 commits into
mainfrom
ps/PM-36839-prepare-api-base-error
Jun 2, 2026
Merged

[PM-36839] Prepare API base Error#1049
dani-garcia merged 13 commits into
mainfrom
ps/PM-36839-prepare-api-base-error

Conversation

@dani-garcia
Copy link
Copy Markdown
Member

@dani-garcia dani-garcia commented May 8, 2026

🎟️ Tracking

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

📔 Objective

Prepare the Error type in bitwarden-api-base so it can be merged with the one in bitwarden-core. This requires:

  • UniFFI support
  • ResponseContent variant as it's own type, rather than inline in the enum, and without a generic type. Also renamed the variant to not have the Error suffix

Once this change is done, we can proceed with merging both error types, which is done on a separate PR as it involves basically every team: #1050

🚨 Breaking Changes

@dani-garcia dani-garcia added the ai-review-vnext Request a Claude code review using the vNext workflow label May 8, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR prepares bitwarden-api-base's Error and ResponseContent types to be merged with bitwarden-core::ApiError (tracked in #1050). It adds an optional uniffi feature (with a custom StatusCode type converter and a hand-rolled serde module), pulls ResponseContent out of the enum into a top-level non-generic struct (renaming contentmessage and dropping entity), and renames the ResponseError variant to Response. The _Phantom(PhantomData<T>, Infallible) variant is a clean way to keep the generic T alive so downstream impl<T> From<Error<T>> for FooError impls continue to compile without unconstrained-type-parameter errors, while remaining unreachable at runtime.

Code Review Details

No new findings. Prior reviewer concerns (StatusCode type erasure, the http-serde dependency, and the breaking impact on bitwarden-user-crypto-management) were addressed in earlier rounds. All downstream callers of the renamed variant/fields (bitwarden-core, bitwarden-auth, bitwarden-user-crypto-management) have been updated, and the new status_code_serializer is covered by three unit tests (serialize, round-trip, invalid value rejection).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🔍 SDK Breaking Change Detection

SDK Version: ps/PM-36839-prepare-api-base-error (708c16b)

⚠️ 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
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.10%. Comparing base (7bca9fc) to head (0d1055f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-core/src/error.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1049      +/-   ##
==========================================
- Coverage   84.10%   84.10%   -0.01%     
==========================================
  Files         446      446              
  Lines       58768    58769       +1     
==========================================
  Hits        49428    49428              
- Misses       9340     9341       +1     

☔ View full report in Codecov by Sentry.
📢 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 May 8, 2026 18:25
@dani-garcia dani-garcia requested review from a team as code owners May 8, 2026 18:25
Copy link
Copy Markdown
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

Changes are looking great, just one question regarding removing type information.

Comment thread crates/bitwarden-api-base/src/error.rs Outdated
Comment thread crates/bitwarden-api-base/Cargo.toml Outdated
@dani-garcia dani-garcia requested a review from djsmith85 May 14, 2026 16:16
djsmith85
djsmith85 previously approved these changes May 15, 2026
Copy link
Copy Markdown
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

Changes are looking great !

Comment thread crates/bitwarden-api-base/src/error.rs
@dani-garcia dani-garcia requested a review from a team as a code owner May 26, 2026 14:59
@dani-garcia dani-garcia requested a review from mzieniukbw May 26, 2026 14:59
mzieniukbw
mzieniukbw previously approved these changes May 26, 2026
# Conflicts:
#	crates/bitwarden-api-base/src/lib.rs
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 2, 2026

@dani-garcia dani-garcia merged commit df211a2 into main Jun 2, 2026
81 of 83 checks passed
@dani-garcia dani-garcia deleted the ps/PM-36839-prepare-api-base-error branch June 2, 2026 17:02
bw-ghapp Bot added a commit to bitwarden/sdk-swift that referenced this pull request Jun 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants