Skip to content

Conversation

@KatherineInCode
Copy link
Contributor

🎟️ Tracking

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

📔 Objective

This segregates out some of the StateService interface into a LanguageStateService, providing a common definition and finer granularity for working with the apps' language selections.

I elected to make this change separate from bringing the whole Select Language UX in to make for smaller PRs in general.

⏰ 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
Copy link
Contributor

github-actions bot commented Nov 17, 2025

Logo
Checkmarx One – Scan Summary & Detailsc68087a4-f0fc-4598-bde6-74ba1e64bcd4

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.94%. Comparing base (98ee2b6) to head (bad742b).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...ared/Core/Platform/Services/ServiceContainer.swift 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2148      +/-   ##
==========================================
- Coverage   85.39%   83.94%   -1.46%     
==========================================
  Files        1721     1978     +257     
  Lines      145463   160942   +15479     
==========================================
+ Hits       124224   135102   +10878     
- Misses      21239    25840    +4601     

☔ 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.

@KatherineInCode KatherineInCode marked this pull request as ready for review November 17, 2025 20:58
@claude
Copy link

claude bot commented Nov 17, 2025

Claude finished @KatherineInCode's task —— View job


Code Review Complete

Summary

This PR successfully extracts language state management from StateService into a dedicated LanguageStateService protocol in BitwardenKit. The implementation follows established architectural patterns, properly uses dependency injection, and includes comprehensive test updates. The change provides better granularity and interface segregation.

Key Changes:

  • Created new LanguageStateService protocol in BitwardenKit with single responsibility for language state (BitwardenKit/Core/Platform/Services/LanguageStateService.swift:1)
  • DefaultStateService now conforms to LanguageStateService in both apps
  • Updated SelectLanguageProcessor in both apps to depend on LanguageStateService instead of full StateService
  • Properly integrated into ServiceContainer with dependency injection
  • Updated all tests to use MockLanguageStateService

Findings

Finding 1: Missing DocC documentation on protocol property

Details

The LanguageStateService protocol in BitwardenKit/Core/Platform/Services/LanguageStateService.swift:1-5 follows the .claude/CLAUDE.md guideline that protocol properties should have DocC documentation (implementations inherit from protocol). The protocol-level documentation is present and properly formatted. ✅

No issue found - documentation is correct as-is.

Finding 2: Review comment addressed

Details

The review comment from @matt-livefront about moving HasLanguageStateService to the centralized Services.swift file has been properly addressed. The protocol is now in BitwardenKit/Core/Platform/Services/Services.swift:38-43, consistent with other Has<Service> protocols. ✅


Architecture & Design Review

Follows established patterns: The implementation correctly follows the dependency injection pattern described in Docs/Architecture.md
Interface segregation: Properly segregates language state concerns from the broader StateService
Protocol composition: Correctly uses Has<Service> pattern for dependency injection
Target boundaries: Changes appropriately span BitwardenKit, BitwardenShared, and AuthenticatorShared
Actor isolation: DefaultStateService implementations correctly use nonisolated for the appLanguage property since it accesses thread-safe stores


Code Quality

Documentation: Protocol and implementations properly documented following DocC standards
Naming: Clear, descriptive names following Swift conventions
Dependencies: Properly uses ServiceContainer for DI
Test coverage: All processor tests updated to use new service type
Mock generation: Protocol correctly annotated with // sourcery: AutoMockable


Test Coverage Analysis

Updated Tests:

  • SelectLanguageProcessorTests (both apps): ✅ Updated to use MockLanguageStateService
  • ServiceContainer+Mocks (both apps): ✅ Added languageStateService parameter with default mock

Test Pattern Compliance:

  • ✅ Tests properly use mocks for isolation
  • ✅ Tests verify state changes and service interactions
  • ✅ setUp/tearDown properly manage test lifecycle

Codecov Report: 93.75% patch coverage with 1 missing line in AuthenticatorShared ServiceContainer (likely a non-critical initialization path).


Security Considerations

✅ No security concerns identified. Language selection is a benign user preference that doesn't involve sensitive data or authentication flows.


Good Practices Observed

  • Clean interface segregation following SOLID principles
  • Consistent implementation across both Password Manager and Authenticator apps
  • Proper use of protocol composition for dependency management
  • nonisolated correctly applied for actor-isolated property access

Action Items

None. This PR is ready to merge. All requirements have been met:

  • ✅ Architecture guidelines followed
  • ✅ Code style compliant
  • ✅ Tests updated and passing (linting passed, other CI checks in progress)
  • ✅ Documentation complete
  • ✅ Review comments addressed

@KatherineInCode KatherineInCode merged commit b5e7033 into main Nov 18, 2025
18 checks passed
@KatherineInCode KatherineInCode deleted the pm-26060/language-state-service branch November 18, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants