[PM-26809] Improve Claude and add Review changes Skill

.claude/CLAUDE.md

@@ -1,12 +1,86 @@
-# Bitwarden iOS Password Manager & Authenticator Apps
+# Bitwarden iOS Password Manager & Authenticator Apps Claude Guidelines
+
+Core directives for maintaining code quality and consistency in the Bitwarden iOS project.
+
+## Core Directives
+
+**You MUST follow these directives at all times.**
+
+1. **Adhere to Architecture**: All code modifications MUST follow patterns in `./../Docs/Architecture.md`
+2. **Follow Code Style**: ALWAYS follow https://contributing.bitwarden.com/contributing/code-style/swift
+3. **Follow Testing Guidelines**: Analyzing or implementing tests MUST follow guidelines in `./../Docs/Testing.md`.
+4. **Best Practices**: Follow Swift / SwiftUI general best practices (value over reference types, guard clauses, extensions, protocol oriented programming)
+5. **Document Everything**: Everything in the code requires DocC documentation except for protocol properties/functions implementations as the docs for them will be in the protocol.
+6. **Dependency Management**: Use `ServiceContainer` as established in the project
+7. **Use Established Patterns**: Leverage existing components before creating new ones
+8. **File References**: Use file:line_number format when referencing code
+
+## Security Requirements
+
+**Every change must consider:**
+- Zero-knowledge architecture preservation
+- Proper encryption key handling (iOS Keychain)
+- Input validation and sanitization
+- Secure data storage patterns
+- Threat model implications
+
+## Workflow Practices
+
+### Before Implementation
+
+1. Read relevant architecture documentation
+2. Search for existing patterns to follow
+3. Identify affected targets and dependencies
+4. Consider security implications
+
+### During Implementation
+
+1. Follow existing code style in surrounding files
+2. Write tests alongside implementation
+3. Add DocC to everything except protocol implementations
+4. Validate against architecture guidelines
+
+### After Implementation
+
+1. Ensure all tests pass
+2. Verify compilation succeeds
+3. Review security considerations
+4. Update relevant documentation
+
+## Anti-Patterns
+
+**Avoid these:**
+- Creating new patterns when established ones exist
+- Exception-based error handling in business logic
+- Direct dependency access (use DI)
+- Undocumented public APIs
+- Tight coupling between targets
+
+## Communication & Decision-Making
+
+Always clarify ambiguous requirements before implementing. Use specific questions:
+- "Should this use [Approach A] or [Approach B]?"
+- "This affects [X]. Proceed or review first?"
+- "Expected behavior for [specific requirement]?"
+
+Defer high-impact decisions to the user:
+- Architecture/module changes, public API modifications
+- Security mechanisms, database migrations
+- Third-party library additions
 
 ## References
 
--   [iOS Architecture](./../Docs/Architecture.md)
+### Critical resources:
+- `./../Docs/Architecture.md` - Architecture patterns and principles
+- `./../Docs/Testing.md` - Testing guidelines
+- https://contributing.bitwarden.com/contributing/code-style/swift - Code style guidelines
+
+**Do not duplicate information from these files - reference them instead.**
+
+### Additional resources:
 -   [Architectural Decision Records (ADRs)](https://contributing.bitwarden.com/architecture/adr/)
 -   [Contributing Guidelines](https://contributing.bitwarden.com/contributing/)
 -   [Accessibility](https://contributing.bitwarden.com/contributing/accessibility/)
 -   [Setup Guide](https://contributing.bitwarden.com/getting-started/mobile/ios/)
--   [Code Style](https://contributing.bitwarden.com/contributing/code-style/swift)
 -   [Security Whitepaper](https://bitwarden.com/help/bitwarden-security-white-paper/)
 -   [Security Definitions](https://contributing.bitwarden.com/architecture/security/definitions)

.claude/prompts/review-code.md

@@ -1,9 +1,4 @@
-Please review this pull request with a focus on:
-
-- Code quality and best practices
-- Potential bugs or issues
-- Security implications
-- Performance considerations
+Use the `reviewing-changes` skill to review this pull request.
 
 Note: The PR branch is already checked out in the current working directory.
 

.claude/skills/reviewing-changes/SKILL.md

@@ -0,0 +1,109 @@
+---
+name: reviewing-changes
+description: Performs comprehensive code reviews for Bitwarden iOS projects, verifying architecture compliance, style guidelines, compilation safety, test coverage, and security requirements. Use when reviewing pull requests, checking commits, analyzing code changes, verifying Bitwarden coding standards, evaluating unidirectional data flow pattern, checking services container dependency injection usage, reviewing security implementations, or assessing test coverage. Automatically invoked by CI pipeline or manually for interactive code reviews.
+---
+
+# Reviewing Changes
+
+## Instructions
+
+Follow this process to review code changes for Bitwarden iOS:
+
+### Step 1: Understand Context
+
+Start with high-level assessment of the change's purpose and approach. Read PR/commit descriptions and understand what problem is being solved.
+
+### Step 2: Verify Compliance
+
+Systematically check each area against Bitwarden standards documented in `CLAUDE.md`:
+
+1. **Architecture**: Follow patterns in `Docs/Architecture.md`
+   - Unidirectional data flow Coordinators + Processors (using SwiftUI)
+   - Dependency Injection using `ServiceContainer`
+   - Repository pattern and proper data flow
+
+2. **Style**: Adhere to [Code Style](https://contributing.bitwarden.com/contributing/code-style/swift)
+   - Naming conventions, code organization, formatting
+   - Swift/SwiftUI guidelines
+
+3. **Compilation**: Analyze for potential build issues
+   - Import statements and dependencies
+   - Type safety and null safety
+   - API compatibility and deprecation warnings
+   - Resource/SDK references and requirements
+
+4. **Testing**: Verify appropriate test coverage
+   - Unit tests for business logic and utility functions
+   - Snapshot/View inspector tests for user-facing features when applicable
+   - Test coverage for edge cases and error scenarios
+
+5. **Security**: Given Bitwarden's security-focused nature
+   - Proper handling of sensitive data
+   - Secure storage practices (Keychain)
+   - Authentication and authorization patterns
+   - Data encryption and decryption flows
+   - Zero-knowledge architecture preservation
+
+### Step 3: Document Findings
+
+Identify specific violations with `file:line_number` references. Be precise about locations.
+
+### Step 4: Provide Recommendations
+
+Give actionable recommendations for improvements. Explain why changes are needed and suggest specific solutions.
+
+### Step 5: Flag Critical Issues
+
+Highlight issues that must be addressed before merge. Distinguish between blockers and suggestions.
+
+### Step 6: Acknowledge Quality
+
+Note well-implemented patterns (briefly, without elaboration). Keep positive feedback concise.
+
+## Review Anti-Patterns (DO NOT)
+
+- Be nitpicky about linter-catchable style issues
+- Review without understanding context - ask for clarification first
+- Focus only on new code - check surrounding context for issues
+- Request changes outside the scope of this changeset
+
+## Examples
+
+### Good Review Format
+
+```markdown
+## Summary
+This PR adds biometric authentication to the login flow, implementing unidirectional data flow pattern with proper state management.
+
+## Critical Issues
+- `BitwardenShared/UI/Auth/Login/LoginView.swift:25` - No `scrollView` added, user can't scroll through the view.
+- `BitwardenShared/Core/Auth/Services/AuthService.swift:120` - You must not use `try!`, change it to `try` in a `do...catch` block or throwing function.
+
+## Suggested Improvements
+- Consider extracting biometric prompt logic to separate struct
+- Add missing tests for biometric failure scenarios
+- `BitwardenShared/UI/Auth/Login/LoginView.swift:43` - Consider using existing `BitwardenTextField` component
+
+## Good Practices
+- Proper comments documentation
+- Comprehensive unit test coverage
+- Clear separation of concerns
+
+## Action Items
+1. Add scroll view in `LoginView`
+2. Change `try!` to `try` in `AuthService`
+3. Consider adding tests for error flows
+```
+
+### What to Focus On
+
+**DO focus on:**
+- Architecture violations (incorrect patterns)
+- Security issues (data handling, encryption)
+- Missing tests for critical paths
+- Compilation risks (type safety, null safety)
+
+**DON'T focus on:**
+- Minor formatting (handled by linters)
+- Personal preferences without architectural basis
+- Issues outside the changeset scope

.gitignore

@@ -83,3 +83,6 @@ Authenticator/Application/Support/Settings.bundle/Acknowledgements.latest_result
 
 # Backup files
 *.bak
+
+# AI
+.claude/settings.local.json

Docs/Architecture.md

@@ -1,6 +1,11 @@
 # Architecture
 
 - [Overview](#overview)
+  - [Password Manager App Targets](#password-manager-app-targets)
+  - [Authenticator App Targets](#authenticator-app-targets)
+  - [Shared Frameworks](#shared-frameworks)
+  - [Test Helpers](#test-helpers)
+  - [Architecture Structure](#architecture-structure)
 - [Core Layer](#core-layer)
   - [Models](#models)
   - [Data Stores](#data-stores)
@@ -16,36 +21,58 @@
   - [View](#view)
   - [Actions and Effects](#actions-and-effects)
   - [Example](#example)
-- [Tests](#tests)
-  - [Overview](#overview-1)
-  - [Strategies](#strategies)
+- [Test Architecture](#test-architecture)
+  - [Testing Philosophy](#testing-philosophy)
+  - [Test Layer Alignment](#test-layer-alignment)
+  - [Testing Strategies by Component Type](#testing-strategies-by-component-type)
+  - [Test Organization](#test-organization)
+  - [Dependency Mocking](#dependency-mocking)
+  - [Test Isolation](#test-isolation)
 
 ## Overview
 
-The Bitwarden app is composed of the following targets:
+The iOS repository contains two main apps: Bitwarden Password Manager and Bitwarden Authenticator.
 
-- `Bitwarden`: The main iOS app. 
-- `BitwardenActionExtension`: An [Action extension](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/Action.html) that can be accessed via the system share sheet "Autofill with Bitwarden" option. 
+### Password Manager App Targets
+
+- `Bitwarden`: The main iOS Password Manager app.
+- `BitwardenActionExtension`: An [Action extension](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/Action.html) that can be accessed via the system share sheet "Autofill with Bitwarden" option.
 - `BitwardenAutoFillExtension`: An AutoFill Credential Provider extension which allows Bitwarden to offer up credentials for [Password AutoFill](https://developer.apple.com/documentation/security/password_autofill/).
-- `BitwardenShareExtension`: A [Share extension](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/Share.html) that allows creating text or file sends via the system share sheet. 
+- `BitwardenShareExtension`: A [Share extension](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/Share.html) that allows creating text or file sends via the system share sheet.
 - `BitwardenWatchApp`: The companion watchOS app.
+- `BitwardenShared`: The main Password Manager framework containing the Core and UI layers shared between the app and extensions.
+
+### Authenticator App Targets
 
-Additionally, the following top-level folders provide shared functionality between the targets:
+- `Authenticator`: The main iOS Authenticator app.
+- `AuthenticatorShared`: The main Authenticator framework containing the Core and UI layers.
 
-- `BitwardenShared`: A framework that is shared between the app and extensions.
+### Shared Frameworks
+
+- `BitwardenKit`: A shared framework providing common functionality across apps.
+- `BitwardenResources`: A framework containing shared resources (assets, localizations, etc.).
+- `AuthenticatorBridgeKit`: A framework for communication between the Password Manager and Authenticator apps.
 - `BitwardenWatchShared`: Models and encoding/decoding logic for communicating between the iOS and watchOS apps.
-- `GlobalTestHelpers`: Shared functionality between the app's test targets. 
-- `Networking`: A local Swift package that implements the app's networking layer on top of `URLSession`. 
+- `Networking`: A local Swift package that implements the app's networking layer on top of `URLSession`.
+
+### Test Helpers
 
-Most of the app's functionality is implemented in the `BitwardenShared` target. The files within this target are split up between two top-level folders, `Core` and `UI`. Each of these folders is then subdivided into the following folders:
+- `GlobalTestHelpers`: Shared functionality between the app's test targets.
+- `BitwardenKitMocks`: Mock implementations for BitwardenKit components.
+- `AuthenticatorBridgeKitMocks`: Mock implementations for AuthenticatorBridgeKit components.
+- `TestHelpers`: Additional test utilities and helpers. 
+
+### Architecture Structure
+
+Most of the app's functionality is implemented in the `BitwardenShared` and `AuthenticatorShared` frameworks. The files within these frameworks are mainly split up between two top-level folders, `Core` and `UI`. Each of these folders is then subdivided into the following folders:
 
 - `Auth`
 - `Autofill`
 - `Platform`
 - `Tools`
 - `Vault`
 
-These folders align with the [CODEOWNERS](../.github/CODEOWNERS) file for the project; no additional direct subfolders of `Core` or `UI` should be added. While this top-level structure is deliberately inflexible, the folder structure within the subfolders are not specifically prescribed.
+These folders align with the [CODEOWNERS](../.github/CODEOWNERS) file for the project. One **MUST** not add additional direct subfolders to `Core` or `UI`. While this top-level structure is deliberately inflexible, the folder structure within the subfolders are not specifically prescribed.
 
 The responsibilities of the core layer are to manage the storage and retrieval of data from low-level sources (such as from the network, persistence, or Bitwarden SDK) and to expose them in a more ready-to-consume manner by the UI layer via "repository" and "service" classes. The UI layer is then responsible for any final processing of this data for display in the UI as well as receiving events from the UI and updating the tracked state accordingly.
 
@@ -55,7 +82,7 @@ The core layer is where all the UI-independent data is stored and retrieved. It
 
 ### Models
 
-The lowest level of the core layer are the data model objects. These are the raw sources of data that include data retrieved or sent via network requests, data persisted with [CoreData](https://developer.apple.com/documentation/coredata/), and data that is used to interact with the [Bitwarden SDK](https://github.com/bitwarden/sdk).
+The lowest level of the core layer are the data model objects. These are the raw sources of data that include data retrieved or sent via network requests, data persisted with [CoreData](https://developer.apple.com/documentation/coredata/), and data that is used to interact with the [Bitwarden SDK](https://github.com/bitwarden/sdk-internal).
 
 The models are roughly organized based on their use and type:
 
@@ -310,14 +337,67 @@ struct ExampleView: View {
 
 </details>
 
-## Tests
+## Test Architecture
+
+### Testing Philosophy
+
+The test architecture mirrors the layered structure of the application, ensuring that each layer can be tested in isolation through dependency injection and protocol-based abstractions.
+
+### Test Layer Alignment
+
+#### Core Layer Testing
+
+Tests for the core layer focus on data integrity, business logic, and repository/service behavior:
+
+- **Repositories**: Test data synthesis from multiple sources, error handling, and asynchronous operations
+- **Services**: Test discrete responsibilities and interactions with lower-level stores
+- **Data Stores**: Test persistence operations (CoreData, Keychain, UserDefaults)
+- **Models**: Test data transformations, encoding/decoding, and domain logic
+
+Core layer tests should use mocked dependencies to isolate the system under test from external services.
+
+#### UI Layer Testing
+
+Tests for the UI layer validate the unidirectional data flow and state management:
+
+- **Processors**: Test state mutations in response to actions, effect handling, and coordinator navigation requests
+- **Coordinators**: Test route handling and child coordinator creation using mocked modules
+- **Views**: Test UI rendering based on state and user interaction handling
+- **State**: Test state equality and state transformations
+
+UI layer tests leverage the `Store` abstraction to verify the connection between processors and views.
+
+### Testing Strategies by Component Type
+
+The architecture employs three complementary testing strategies:
+
+1. **Logic Testing**: Unit tests validate business logic, state management, and data transformations using protocol mocks
+2. **Interaction Testing**: View inspector tests verify user interactions send correct actions/effects through the store
+3. **Visual Testing**: Snapshot tests capture visual regressions across different display modes and accessibility settings
+
+### Test Organization
+
+Test files are co-located with the code they test, maintaining the same folder structure as the main codebase. This organization:
+
+- Makes it easy to find tests for a given type
+- Ensures tests evolve alongside the code
+- Reinforces the architectural boundaries (Auth, Autofill, Platform, Tools, Vault)
+
+### Dependency Mocking
+
+The architecture's use of protocol composition in the `Services` typealias enables comprehensive mocking:
+
+- All services and repositories are defined as protocols
+- Mock implementations are generated using Sourcery's `AutoMockable` annotation
+- Coordinators can be tested with mocked modules to verify navigation logic
+- Processors can be tested with mocked services to verify state management
 
-### Overview
+### Test Isolation
 
-Every type containing logic should be tested. Test files should be named `<TypeToTest>Tests.swift`. A test file should exist in the same folder as the type being tested. For example, [AppProcessorTests](../BitwardenShared/UI/Platform/Application/AppProcessorTests.swift) is in the same folder as [AppProcessor](../BitwardenShared/UI/Platform/Application/AppProcessor.swift). This makes it convenient to switch between these files or open them side-by-side.
+Each architectural layer can be tested independently:
 
-### Strategies
+- **Core layer** tests mock network responses, SDK interactions, and persistence layers
+- **UI layer** tests mock repositories and services from the core layer
+- **Integration points** between layers are tested by verifying protocol conformance
 
-- **Unit**: Unit tests compose the majority of tests in the suite. These are written using [XCTest](https://developer.apple.com/documentation/xctest) assertions and should be used to test all logic portions within a type.
-- **View**: In a SwiftUI view test, [ViewInspector](https://github.com/nalexn/ViewInspector) is used to test any user interactions within the view. This is commonly used to assert that tapping a button sends an action or effect to the processor, but it can also be used to test other view interactions.
-- **Snapshot**: In addition to using [ViewInspector](https://github.com/nalexn/ViewInspector) to interact with a view under test, [SnapshotTesting](https://github.com/pointfreeco/swift-snapshot-testing) is used to take snapshots of the view to test for visual changes from one test run to another. The resulting snapshot images are stored in the repository and are compared against on future test runs. Any visual differences on future test runs will result in a failing test. Snapshot tests are usually recorded in light mode, dark mode, and with a large dynamic type size. ⚠️ These tests are done using the simulator device and iOS version specified in `.test-simulator-device-name` and `.test-simulator-ios-version` files, otherwise tests may fail because of subtle differences between iOS versions.
+This isolation enables fast, reliable tests that pinpoint failures to specific architectural components.