[PM-33561] debt: Wire CipherManager and cipher ViewModel error handlers by SaintPatrck · Pull Request #6651 · bitwarden/android

SaintPatrck · 2026-03-12T20:14:05Z

🎟️ Tracking

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

📔 Objective

Wire userFriendlyMessage through CipherManager fold(onFailure) blocks and update all cipher-related ViewModel error handlers to prefer errorMessage over generic defaults.

This is part of the PM-33394 series to surface CookieRedirectException user-friendly messages through result types to ViewModels. The foundation (userFriendlyMessage extension and errorMessage on result types) was merged in #6642.

Changes:

Wire userFriendlyMessage in CipherManager for delete, restore, share, archive, unarchive, and attachment operations
Update VaultItemViewModel, VaultAddEditViewModel, AttachmentsViewModel, VaultMoveToOrganizationViewModel, SearchViewModel, VaultItemListingViewModel, and VaultViewModel error handlers
Add CipherManager tests for CookieRedirectException error message propagation
Add ViewModel tests for non-null errorMessage display

codecov · 2026-03-12T20:18:05Z

Codecov Report

❌ Patch coverage is 65.90909% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.77%. Comparing base (d9f8c3d) to head (ff0a5e5).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...warden/ui/vault/feature/item/VaultItemViewModel.kt	50.00%	0 Missing and 4 partials ⚠️
.../ui/vault/feature/addedit/VaultAddEditViewModel.kt	50.00%	0 Missing and 3 partials ⚠️
...rden/ui/platform/feature/search/SearchViewModel.kt	50.00%	0 Missing and 2 partials ⚠️
...t/feature/itemlisting/VaultItemListingViewModel.kt	50.00%	0 Missing and 2 partials ⚠️
...bitwarden/ui/vault/feature/vault/VaultViewModel.kt	50.00%	0 Missing and 2 partials ⚠️
.../vault/feature/attachments/AttachmentsViewModel.kt	75.00%	0 Missing and 1 partial ⚠️
...toorganization/VaultMoveToOrganizationViewModel.kt	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6651      +/-   ##
==========================================
- Coverage   85.80%   85.77%   -0.03%     
==========================================
  Files         848      805      -43     
  Lines       58851    57257    -1594     
  Branches     8351     8329      -22     
==========================================
- Hits        50497    49114    -1383     
+ Misses       5455     5244     -211     
  Partials     2899     2899

Flag	Coverage Δ
app-data	`17.60% <22.72%> (+0.39%)`	⬆️
app-ui-auth-tools	`20.82% <0.00%> (-0.22%)`	⬇️
app-ui-platform	`15.10% <4.54%> (-0.42%)`	⬇️
app-ui-vault	`25.85% <38.63%> (-0.69%)`	⬇️
authenticator	`6.64% <0.00%> (-0.03%)`	⬇️
lib-core-network-bridge	`4.27% <0.00%> (-0.03%)`	⬇️
lib-data-ui	`0.95% <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 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.

github-actions · 2026-03-12T20:20:41Z

Checkmarx One – Scan Summary & Details – 7989cd0e-d601-4d1d-af36-658a66226cd0

New Issues (1)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		Use_of_Hardcoded_Password	/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt: 487	details The application uses the hard-coded password "mockId-1" for authentication purposes, either using it to verify users' identities, or to access an... Attack Vector

...src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/attachments/AttachmentsViewModelTest.kt

+    @Test
+    fun `DeleteClick with deleteCipherAttachment error with errorMessage should display that message`() =
+        runTest {
+            val cipherId = "mockId-1"


claude · 2026-03-12T20:21:32Z

Claude finished @SaintPatrck's task in 4m 38s —— View job

I'll analyze this and get back to you.

claude · 2026-03-12T20:32:49Z

⚠️ Inline finding -- VaultViewModel.kt (not in diff)

IMPORTANT (Correctness): VaultViewModel archive/unarchive error handlers at lines 1021 and 1042 were not updated with the result.errorMessage?.asText() ?: fallback pattern that was applied to all other ViewModels in this PR.

// VaultViewModel.kt:1019-1022 (current)
dialog = VaultState.DialogState.Error(
    title = BitwardenString.an_error_has_occurred.asText(),
    message = BitwardenString.unable_to_archive_selected_item.asText(),  // <-- missing errorMessage check
    error = result.error,
)

// Suggested fix (same pattern as SearchViewModel, VaultItemListingViewModel, etc.):
dialog = VaultState.DialogState.Error(
    title = BitwardenString.an_error_has_occurred.asText(),
    message = result.errorMessage?.asText()
        ?: BitwardenString.unable_to_archive_selected_item.asText(),
    error = result.error,
)

Same issue applies to handleUnarchiveCipherReceive at line 1042. Without this change, archive/unarchive errors from the main vault screen will always show generic fallback text even when CipherManagerImpl now provides a userFriendlyMessage (e.g., from CookieRedirectException).

david-livefront · 2026-03-12T20:35:30Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt

    ): ArchiveCipherResult {
-        val userId = activeUserId ?: return ArchiveCipherResult.Error(NoActiveUserException())
+        val userId = activeUserId
+            ?: return ArchiveCipherResult.Error(error = NoActiveUserException())


Why did this change?

Leftover from adding errorMessage because I originally added it as the first param so the param name had to be added. Just forgot to revert these two spots.

They're minor changes and we usually declare param names anyways. Unless you feel strongly about leaving it as it was, I think this actually keeps things more consistent with the rest of the code.

david-livefront · 2026-03-12T20:35:36Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt

    ): UnarchiveCipherResult {
-        val userId = activeUserId ?: return UnarchiveCipherResult.Error(NoActiveUserException())
+        val userId = activeUserId
+            ?: return UnarchiveCipherResult.Error(error = NoActiveUserException())


#6651 (comment)

Wire userFriendlyMessage through CipherManager fold(onFailure) blocks for delete, restore, share, archive, unarchive, and attachment operations. Update all cipher-related ViewModel error handlers to prefer errorMessage over generic defaults.

claude · 2026-03-13T13:08:59Z

Code Review Summary (Updated)

PR: PM-33394 debt: Wire CipherManager and cipher ViewModel error handlers

Overview

This PR wires userFriendlyMessage through all CipherManagerImpl fold(onFailure) blocks and updates all seven cipher-related ViewModel error handlers to prefer errorMessage over generic defaults. This is a well-scoped continuation of the PM-33394 series building on the foundation in #6642.

Previous Review Correction

The prior automated review incorrectly reported that VaultViewModel.kt was missing from the changeset. Upon re-examination, VaultViewModel.kt is present in the diff and correctly updated at both handleArchiveCipherReceive (line 1021) and handleUnarchiveCipherReceive (line 1043) with the result.errorMessage?.asText() ?: fallback pattern. That finding was a false positive.

Findings

No issues found. The PR is clean.

What Looks Good

CipherManagerImpl consistently wires userFriendlyMessage across all 11 fold(onFailure) blocks (createCipher x2, archive, unarchive, hardDelete, softDelete, deleteCipherAttachment, restore, updateCipher, shareCipher, updateCipherCollections).
NoActiveUserException early-return paths correctly omit errorMessage (defaults to null), which is appropriate since that exception type is not in the userFriendlyMessage allow-list.
All seven ViewModels (SearchViewModel, VaultAddEditViewModel, AttachmentsViewModel, VaultItemViewModel, VaultItemListingViewModel, VaultMoveToOrganizationViewModel, VaultViewModel) are consistently updated with the result.errorMessage?.asText() ?: fallback pattern.
Test coverage is thorough: representative CipherManager tests verify CookieRedirectException propagation, and every modified ViewModel has corresponding tests for the non-null errorMessage display path.
The changes are backward-compatible since all errorMessage parameters have default values of null.

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt

claude · 2026-03-13T15:30:03Z

Code Review Summary (Re-review)

PR: PM-33561 debt: Wire CipherManager and cipher ViewModel error handlers

Reviewed commits: All 3 commits through ff0a5e5 (including 2 new commits since last review)

Changes Since Last Review

Two follow-up commits refined the approach by moving userFriendlyMessage derivation from CipherManagerImpl call sites into the result type constructors as default values:

Result type defaults (b7d8237): All 8 cipher result types now default errorMessage to error.userFriendlyMessage instead of null.
CipherManagerImpl cleanup: Removed explicit userFriendlyMessage wiring from all 11 fold(onFailure) blocks. Each block simply constructs Error(error = it) and the default value handles the rest.
Parameter reordering: CreateCipherResult.Error and UpdateCipherResult.Error swapped parameter order from (errorMessage, error) to (error, errorMessage). All callers use named parameters, so this is safe.
Fido2CredentialStoreImpl: Updated toCreateCipherResult() to use named parameters for the reordered constructor.

Findings

No issues found. The refactoring is clean and well-executed.

Verification Notes

The userFriendlyMessage extension (ThrowableExtensions.kt) only allowlists CookieRedirectException, returning null for all other exception types. No risk of leaking internal error details to the UI.
All callers of CreateCipherResult.Error and UpdateCipherResult.Error (13+ call sites across production and test code) use named parameters, making the parameter reorder safe.
The Invalid response paths in createCipher/updateCipher explicitly pass errorMessage, which overrides the default -- correct behavior.
NoActiveUserException early-return paths omit errorMessage, defaulting to null since that exception type is not allowlisted -- correct behavior.

github-actions bot added the app:password-manager Bitwarden Password Manager app context label Mar 12, 2026

SaintPatrck changed the title ~~[PM-33394] tech: Wire CipherManager and cipher ViewModel error handlers~~ [PM-33394] debt: Wire CipherManager and cipher ViewModel error handlers Mar 12, 2026

SaintPatrck added t:tech-debt Change Type - Tech debt ai-review Request a Claude code review labels Mar 12, 2026

github-advanced-security bot found potential problems Mar 12, 2026

View reviewed changes

SaintPatrck force-pushed the fix/PM-33394-cipher-error-handlers branch from a63268e to f25b532 Compare March 12, 2026 20:22

david-livefront reviewed Mar 12, 2026

View reviewed changes

SaintPatrck force-pushed the fix/PM-33394-cipher-error-handlers branch from f25b532 to 8cd25ef Compare March 13, 2026 13:03

Default errorMessage to error.userFriendlyMessage in result types

b7d8237

SaintPatrck marked this pull request as ready for review March 13, 2026 14:32

SaintPatrck requested a review from a team as a code owner March 13, 2026 14:32

SaintPatrck requested a review from david-livefront March 13, 2026 14:50

SaintPatrck changed the title ~~[PM-33394] debt: Wire CipherManager and cipher ViewModel error handlers~~ [PM-33561] debt: Wire CipherManager and cipher ViewModel error handlers Mar 13, 2026

david-livefront reviewed Mar 13, 2026

View reviewed changes

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt Outdated Show resolved Hide resolved

david-livefront reviewed Mar 13, 2026

View reviewed changes

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/UpdateCipherResult.kt Outdated Show resolved Hide resolved

Remove default value from error parameter in result types

ff0a5e5

SaintPatrck enabled auto-merge March 13, 2026 15:39

SaintPatrck requested a review from david-livefront March 13, 2026 15:48

david-livefront approved these changes Mar 13, 2026

View reviewed changes

SaintPatrck added this pull request to the merge queue Mar 13, 2026

Merged via the queue into main with commit 85c3a1d Mar 13, 2026
32 checks passed

SaintPatrck deleted the fix/PM-33394-cipher-error-handlers branch March 13, 2026 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-33561] debt: Wire CipherManager and cipher ViewModel error handlers#6651

[PM-33561] debt: Wire CipherManager and cipher ViewModel error handlers#6651
SaintPatrck merged 3 commits intomainfrom
fix/PM-33394-cipher-error-handlers

SaintPatrck commented Mar 12, 2026 •

edited

Loading

Uh oh!

codecov bot commented Mar 12, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Mar 12, 2026 •

edited

Loading

Uh oh!

Check warning

claude bot commented Mar 12, 2026 •

edited

Loading

Uh oh!

claude bot commented Mar 12, 2026

Uh oh!

david-livefront Mar 12, 2026

Uh oh!

SaintPatrck Mar 13, 2026

Uh oh!

david-livefront Mar 12, 2026

Uh oh!

SaintPatrck Mar 13, 2026

Uh oh!

claude bot commented Mar 13, 2026 •

edited by atlassian bot

Loading

Uh oh!

Uh oh!

Uh oh!

claude bot commented Mar 13, 2026 •

edited by atlassian bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

SaintPatrck commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

codecov bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Check warning

claude bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Mar 12, 2026

Uh oh!

david-livefront Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

SaintPatrck Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

david-livefront Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

SaintPatrck Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 13, 2026 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary (Updated)

Overview

Previous Review Correction

Findings

What Looks Good

Uh oh!

Uh oh!

Uh oh!

claude bot commented Mar 13, 2026 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary (Re-review)

Changes Since Last Review

Findings

Verification Notes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SaintPatrck commented Mar 12, 2026 •

edited

Loading

codecov bot commented Mar 12, 2026 •

edited

Loading

github-actions bot commented Mar 12, 2026 •

edited

Loading

claude bot commented Mar 12, 2026 •

edited

Loading

claude bot commented Mar 13, 2026 •

edited by atlassian bot

Loading

claude bot commented Mar 13, 2026 •

edited by atlassian bot

Loading