[BEEEP] Add error handling for ElectronStore errors

[trmartin4]

🎟️ Tracking

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

📔 Objective

Prevent application crashes when there are values in the ElectronStore that cannot be deserialized.

Before this change, data that could not be deserialized caused the application to crash with a stack trace shown below.

With this change in place, when an invalid data.json is detected on ElectronStore initialization, the service will:

1. Back the corrupt file up to data.json.corrupt.${Date.now()}
2. Create a new data.json.

Some considerations

🤔 Is backing this up to a corrupt backup the right option here? Should we back up this data and risk duplicating data that the user wishes to keep only in data.json versus proliferating in different files that need to be cleaned up?

📸 Screenshots

Example of error:

With handling on initialization to move to data.corrupt.json:

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – ad76d043-5780-49a7-9cd3-db6c579f6ecd

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CVE-2025-13224	Npm-electron-37.7.0	details Recommended version: 39.2.3 Description: Type Confusion in V8 in Google Chrome prior to 142.0.7444.175 allowed a remote attacker to potentially exploit heap corruption via a crafted HTML p... Attack Vector: NETWORK Attack Complexity: LOW ID: 93OLZp51TDC%2FzAeJIpQMSvzhKizDoA5%2FC7EN8MhC6P4%3D Vulnerable Package

[claude]

Claude finished @trmartin4's task —— View job

---

Review Update

I've reviewed the recent changes since my last review. The commit "Added log on delete error" addressed my previous feedback about logging cleanup failures.

Summary

The implementation looks solid overall. The error handling is comprehensive and the test coverage is excellent at 97.82%.

Findings

Finding 1: PR title contains "[BEEEP]" placeholder

💭 The PR title includes "[BEEEP]" which appears to be a placeholder. Consider updating to a more descriptive title like "[Platform] Add error handling for ElectronStore errors" or similar.

Finding 2: Missing test coverage line is acceptable

The codecov report shows 1 missing line in apps/desktop/src/main.ts:137. This is the constructor call to ElectronStorageService with the new logService parameter. This is acceptable as it's difficult to test the main.ts constructor in isolation, and the service itself has comprehensive test coverage.

Positive Observations

• Excellent test coverage: 568 lines of comprehensive tests covering all error scenarios
• Good error recovery: The implementation gracefully handles backup failures, falling back to deletion
• Proper logging: All error paths are now logged appropriately with debug-level logging for cleanup errors
• Timestamp-based backups: Prevents overwrites of previous corrupt backups

Previous Review Items

My previous review comments have been addressed:

• ✅ Debug logging added for cleanup errors (line 116 in electron-storage.service.ts:116)
• The PR description discrepancy I mentioned (backup filename format) is minor and doesn't affect functionality
• The test suggestion I made was acknowledged by the PR author

[codecov]

Codecov Report

❌ Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 41.65%. Comparing base (a8d6ad4) to head (ea89074).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
apps/desktop/src/main.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17724      +/-   ##
==========================================
+ Coverage   41.60%   41.65%   +0.04%     
==========================================
  Files        3553     3553              
  Lines      102390   102423      +33     
  Branches    15366    15368       +2     
==========================================
+ Hits        42602    42662      +60     
+ Misses      57995    57966      -29     
- Partials     1793     1795       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dani-garcia]

Looks good to me overall.

I think backing up the corrupt file is a good idea that would allow us to debug the corruption issues better, though I'm also a bit concerned about potentially duplicating the data into a file that we don't keep track of.

That means any operation that would generally delete the user data consistently (like logging off) now may leave behind a corrupted copy of the data. Should we consider cleaning them up in the background once they're old?