Skip to content

[PM-36839] Merge API errors#1050

Open
dani-garcia wants to merge 2 commits into
mainfrom
ps/PM-36839-merge-api-errors
Open

[PM-36839] Merge API errors#1050
dani-garcia wants to merge 2 commits into
mainfrom
ps/PM-36839-merge-api-errors

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

Remove the API error from bitwarden-core and reexport the one from bitwarden-api-base instead, which is now identical. We can also finally remove the unused generic type parameter in Error.

This change will save every user of the API bindings from having to do a manual .map_err() or impl<T> From<bitwarden_api_api::apis::Error<T>> every time they interact with an API error.

🚨 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 unifies bitwarden_core::ApiError with bitwarden_api_base::Error by re-exporting the latter, removes the unused T generic parameter (and its _Phantom uninhabited placeholder), and drops the boilerplate impl<T> From<bitwarden_api_api::apis::Error<T>> and .map_err(ApiError::from)? patterns at ~25 call sites. The change is a clean deduplication: since bitwarden_api_api::apis::Error is pub use bitwarden_api_base::*, the types are now literally identical, so downstream #[from] ApiError impls (via thiserror) propagate via ? directly. UniFFI consumers remain unaffected (uniffi(flat_error) is preserved) and the existing pattern-matching tests on ApiError::Response(ResponseContent { ... }) continue to compile because variant structure is unchanged.

Code Review Details

No findings.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 8, 2026

🔍 SDK Breaking Change Detection

SDK Version: ps/PM-36839-merge-api-errors (3dbb193)

⚠️ 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 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.17%. Comparing base (d2118d2) to head (19de958).

Files with missing lines Patch % Lines
...ates/bitwarden-core/src/auth/login/auth_request.rs 0.00% 2 Missing ⚠️
crates/bitwarden-core/src/auth/login/prelogin.rs 0.00% 1 Missing ⚠️
crates/bitwarden-core/src/auth/login/two_factor.rs 0.00% 1 Missing ⚠️
...es/bitwarden-core/src/platform/get_user_api_key.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
+ Coverage   84.12%   84.17%   +0.05%     
==========================================
  Files         446      445       -1     
  Lines       58817    58725      -92     
==========================================
- Hits        49478    49434      -44     
+ Misses       9339     9291      -48     

☔ 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 force-pushed the ps/PM-36839-merge-api-errors branch from 3dbb193 to f1b4c56 Compare May 8, 2026 18:25
dani-garcia added a commit that referenced this pull request Jun 2, 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

<!-- Does this PR introduce any breaking changes? If so, please describe
the impact and migration path for clients.

If you're unsure, the automated TypeScript compatibility check will run
when you open/update this PR and provide feedback.

For breaking changes:
1. Describe what changed in the client interface
2. Explain why the change was necessary
3. Provide migration steps for client developers
4. Link to any paired client PRs if needed

Otherwise, you can remove this section. -->
Base automatically changed from ps/PM-36839-prepare-api-base-error to main June 2, 2026 17:02
@dani-garcia dani-garcia force-pushed the ps/PM-36839-merge-api-errors branch from f1b4c56 to 60ceabd Compare June 2, 2026 19:18
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 3, 2026

@dani-garcia dani-garcia marked this pull request as ready for review June 3, 2026 14:30
@dani-garcia dani-garcia requested review from a team as code owners June 3, 2026 14:30
Copy link
Copy Markdown
Contributor

@harr1424 harr1424 left a comment

Choose a reason for hiding this comment

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

Tools owned changes to Sends look good!

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.

2 participants