Reviewing Changes

Reviewing Changes - Android Additions

This skill provides Android-specific workflow additions that complement the base bitwarden-code-reviewer agent standards.

Instructions

IMPORTANT: Work systematically through each step before providing feedback. Each checklist file includes structured thinking guidance for its review passes.

Step 1: Retrieve Additional Details

Retrieve any additional information linked to the pull request using available tools (JIRA MCP, GitHub API).

If pull request title and message do not provide enough context, request additional details from the reviewer:

• Link a JIRA ticket
• Associate a GitHub issue
• Link to another pull request
• Add more detail to the PR title or body

Android metadata checks — flag as ❓ if any of these are missing:

• PR includes *Screen.kt or Composable changes but has no screenshots
• PR adds new ViewModel or Repository but has no test plan or test file changes

Step 2: Detect Change Type with Android Refinements

Use the base change type detection from the agent, with Android-specific refinements:

Android-specific patterns:

• Feature Addition: New ViewModel, new Repository, new @Composable functions, new *Screen.kt files
• UI Refinement: Changes only in *Screen.kt, *Composable.kt, ui/ package files
• Infrastructure: Changes to .github/workflows/, gradle/, build.gradle.kts, libs.versions.toml
• Dependency Update: Changes only to libs.versions.toml or build.gradle.kts with version bumps

Step 3: Load Appropriate Checklist

Based on detected type, read the relevant checklist file:

• Dependency Update → checklists/dependency-update.md (expedited review)
• Bug Fix → checklists/bug-fix.md (focused review)
• Feature Addition → checklists/feature-addition.md (comprehensive review)
• UI Refinement → checklists/ui-refinement.md (design-focused review)
• Refactoring → checklists/refactoring.md (pattern-focused review)
• Infrastructure → checklists/infrastructure.md (tooling-focused review)

The checklist provides:

• Multi-pass review strategy
• Type-specific focus areas
• What to check and what to skip
• Structured thinking guidance

Step 4: Execute Review Following Checklist

Follow the checklist's multi-pass strategy, thinking through each pass systematically.

Step 5: Consult Android Reference Materials As Needed

Load reference files only when needed for specific questions:

• Re-reviews → invoke reviewing-incremental-changes agent skill; scope to changed lines only, do not flag new issues in unchanged code
• Issue prioritization → reference/priority-framework.md (Critical vs Suggested vs Optional)
• Phrasing feedback → reference/review-psychology.md (questions vs commands, I-statements)
• Architecture questions → reference/architectural-patterns.md (MVVM, Hilt DI, module org, error handling)
• Security questions (quick reference) → reference/security-patterns.md (common patterns and anti-patterns)
• Security questions (comprehensive) → docs/ARCHITECTURE.md#security (full zero-knowledge architecture)
• Testing questions → reference/testing-patterns.md (unit tests, mocking, null safety)
• UI questions → reference/ui-patterns.md (Compose patterns, theming)
• Style questions (project-specific) → reference/style-patterns.md (Kotlin rules enforced in review)
• Style questions (general) → docs/STYLE_AND_BEST_PRACTICES.md

Core Principles

• Priority order: Security → Correctness → Breaking Changes → Performance → Maintainability
• Appropriate depth: Match review rigor to change complexity and risk
• Specific references: Always use file:line_number format for precise location
• Actionable feedback: Say what to do and why, not just what's wrong
• Efficient reviews: Use multi-pass strategy, skip what's not relevant
• Android patterns: Validate MVVM, Hilt DI, Compose conventions, Kotlin idioms

Bug Fix Review Checklist

Multi-Pass Strategy

First Pass: Understand the Bug

1. Understand root cause:

• What was the broken behavior?
• What caused it?
• How does this fix address the root cause?

2. Assess scope:

• How many files changed?
• Is this a targeted fix or broader refactoring?
• Does this affect multiple features?

3. Check for side effects:

• Could this break other features?
• Are there edge cases not considered?

Second Pass: Verify the Fix

4. Code changes:

• Does the fix make sense?
• Is it the simplest solution?
• Any unnecessary changes included?

5. Testing:

• Is there a regression test?
• Does test verify the bug is fixed?
• Are edge cases covered?

6. Related code:

• Same pattern in other places that might have same bug?
• Should other similar code be fixed too?

What to CHECK

✅ Root Cause Analysis

• Does the fix address the root cause or just symptoms?
• Is the explanation in PR/commit clear?

✅ Regression Testing

• Is there a new test that would fail without this fix?
• Does test cover the reported bug scenario?
• Are related edge cases tested?

✅ Side Effects

• Could this break existing functionality?
• Are there similar code paths that need checking?
• Does this change behavior in unexpected ways?

✅ Fix Scope

• Is the fix appropriately scoped (not too broad, not too narrow)?
• Are all instances of the bug fixed?
• Any related bugs discovered during investigation?

What to SKIP

❌ Full Architecture Review - Unless fix reveals architectural problems ❌ Comprehensive Testing Review - Focus on regression tests, not entire test suite ❌ Major Refactoring Suggestions - Unless directly related to preventing similar bugs

Red Flags

🚩 No test for the bug - How will we prevent regression? 🚩 Fix doesn't match root cause - Is this fixing symptoms? 🚩 Broad changes beyond the bug - Should this be split into separate PRs? 🚩 Similar patterns elsewhere - Should those be fixed too?

Key Questions to Ask

Use reference/review-psychology.md for phrasing:

• "Can we add a test that would fail without this fix?"
• "I see this pattern in [other file] - does it have the same issue?"
• "Is this fixing the root cause or masking the symptom?"
• "Could this change affect [related feature]?"

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Example Review

**Overall Assessment:** APPROVE

See inline comments for suggested improvements.

Inline comment examples:

**data/auth/BiometricRepository.kt:120** - SUGGESTED: Extract null handling

<details>
<summary>Details</summary>

Root cause analysis: BiometricPrompt result was nullable but code assumed non-null, causing crash on cancellation (PM-12345).

Consider extracting null handling pattern:

\```kotlin
private fun handleBiometricResult(result: BiometricPrompt.AuthenticationResult?): AuthResult {
    return result?.let { AuthResult.Success(it) } ?: AuthResult.Cancelled
}
\```

This pattern could be reused if we add other biometric auth points.
</details>

**app/auth/BiometricViewModel.kt:89** - SUGGESTED: Add regression test

<details>
<summary>Details</summary>

Add test for cancellation scenario to prevent regression:

\```kotlin
@Test
fun `when biometric cancelled then returns cancelled state`() = runTest {
    coEvery { repository.authenticate() } returns Result.failure(CancelledException())
    viewModel.onBiometricAuth()
    assertEquals(AuthState.Cancelled, viewModel.state.value)
}
\```

This prevents regression of the bug just fixed.
</details>

Dependency Update Review Checklist

Multi-Pass Strategy

First Pass: Identify and Assess

1. Identify the change:

• Which library? Old version → New version?
• Major (X.0.0), Minor (0.X.0), or Patch (0.0.X) version change?
• Single dependency or multiple?

2. Check compilation safety:

• Any imports in codebase that might break?
• Any deprecated APIs we're currently using?
• Check if this is a breaking change version

Second Pass: Deep Analysis

3. Review release notes (if available):

• Breaking changes mentioned?
• Security fixes included?
• New features we should know about?
• Deprecations that affect our usage?

4. Verify consistency:

• If updating androidx library, are related libraries updated consistently?
• BOM (Bill of Materials) consistency if applicable?
• Test dependencies updated alongside main dependencies?

What to CHECK

✅ Compilation Safety

• Look for API deprecations in our codebase
• Check if import statements still valid
• Major version bumps require extra scrutiny
• Beta/alpha versions need stability assessment

✅ Security Implications (if applicable)

• Security-related libraries (crypto, auth, networking)?
• Check for CVEs addressed in release notes
• Review security advisories for this library

✅ Testing Implications

• Does this affect test utilities?
• Are there breaking changes in test APIs?
• Do existing tests still cover the same scenarios?

✅ Changelog Review

• Read release notes for breaking changes
• Note any behavioral changes
• Check migration guides if major version

What to SKIP

❌ Full Architecture Review - No code changed, patterns unchanged ❌ Code Style Review - No code to review ❌ New Test Requirements - Unless API changed significantly ❌ Security Deep-Dive - Unless crypto/auth/networking library ❌ Performance Analysis - Unless release notes mention performance changes

Red Flags (Escalate to Full Review)

🚩 Major version bump (e.g., 1.x → 2.0) - Read checklists/feature-addition.md 🚩 Security/crypto library - Read reference/architectural-patterns.md and docs/ARCHITECTURE.md#security 🚩 Breaking changes in release notes - Read relevant code sections carefully 🚩 Multiple dependency updates at once - Check for interaction risks 🚩 Beta/Alpha versions - Assess stability concerns and rollback plan

If any red flags present, escalate to more comprehensive review using appropriate checklist.

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Example Reviews

Example 1: Simple Patch Version (No Critical Issues)

**Overall Assessment:** APPROVE

See inline comments for all issue details.

Inline comment example:

**libs.versions.toml:45** - SUGGESTED: Beta version in production

<details>
<summary>Details</summary>

androidx.credentials updated from 1.5.0 to 1.6.0-beta03

Monitor for stability issues - beta releases may have unexpected behavior in production.

Changelog: Adds support for additional credential types, internal bug fixes.
</details>

Example 2: Major Version with Breaking Changes (With Critical Issues)

**Overall Assessment:** REQUEST CHANGES

**Critical Issues:**
- Breaking API changes in Retrofit 3.0.0 (network/api/BitwardenApiService.kt)
- Breaking API changes in Retrofit 3.0.0 (network/api/VaultApiService.kt)

See inline comments for migration details.

Inline comment example:

**network/api/BitwardenApiService.kt:15** - CRITICAL: Breaking API changes

<details>
<summary>Details and fix</summary>

Retrofit 3.0.0 removes `Call<T>` return type. Migration required:

\```kotlin
// Before
fun getUser(): Call<UserResponse>

// After
suspend fun getUser(): Response<UserResponse>
\```

Update all API service interfaces to use suspend functions, update call sites to use coroutines instead of enqueue/execute, and update tests accordingly.

Consider creating a separate PR for this migration due to scope.

Reference: https://github.com/square/retrofit/blob/master/CHANGELOG.md#version-300
</details>

Feature Addition Review Checklist

Multi-Pass Strategy

First Pass: High-Level Assessment

1. Understand the feature:

• Read PR description - what problem does this solve?
• Identify user-facing changes vs internal changes
• Note any security implications (auth, encryption, data handling)

2. Scan file structure:

• Which modules affected? (app, data, network, ui, core?)
• Are files organized correctly per module structure?
• Any new public APIs introduced?

3. Initial risk assessment:

• Does this touch sensitive data or security-critical paths?
• Does this affect existing features or only add new ones?
• Are there obvious compilation or null safety issues?

Second Pass: Architecture Deep-Dive

4. MVVM + UDF Pattern Compliance:

• ViewModels properly structured?
• State management using StateFlow?
• Business logic in correct layer?

5. Dependency Injection:

• Hilt DI used correctly?
• Dependencies injected, not manually instantiated?
• Proper scoping applied?

6. Module Organization:

• Code placed in correct modules?
• No circular dependencies introduced?
• Proper separation of concerns?

7. Error Handling:

• Using Result types, not exception-based handling?
• Errors propagated correctly through layers?

Third Pass: Details and Quality

8. Testing:

• Unit tests for ViewModels and repositories?
• Test coverage for edge cases and error scenarios?
• Tests verify behavior, not implementation?

9. Code Quality:

• Null safety handled properly?
• Public APIs have KDoc documentation?
• Naming follows project conventions?

10. Security:

• Sensitive data encrypted properly?
• Authentication/authorization handled correctly?
• Zero-knowledge architecture preserved?

Architecture Review

Read reference/architectural-patterns.md for full patterns and code examples.

Check these four areas:

• MVVM/UDF: ViewModel exposes StateFlow (not MutableStateFlow), business logic in Repository, UI is stateless
• Hilt DI: @HiltViewModel + @Inject constructor, inject interfaces not implementations, no manual instantiation
• Module placement: UI in :ui/:app, data in :data, network in :network, no circular dependencies
• Error handling: Result<T> / runCatching throughout — no thrown exceptions from data layer

Security Review

Reference: docs/ARCHITECTURE.md#security

Critical Security Checks:

• Sensitive data encrypted: Passwords, keys, tokens use Android Keystore or EncryptedSharedPreferences
• No plaintext secrets: No passwords/keys in logs, memory dumps, or SharedPreferences
• Input validation: All user-provided data validated and sanitized
• Authentication tokens: Securely stored and transmitted
• Zero-knowledge architecture: Encryption happens client-side, server never sees plaintext

Red Flags:

// ❌ CRITICAL - Plaintext storage
sharedPreferences.edit {
    putString("pin", userPin)  // Must use EncryptedSharedPreferences
}

// ❌ CRITICAL - Logging sensitive data
Log.d("Auth", "Password: $password")  // Never log sensitive data

// ❌ CRITICAL - Weak encryption
val cipher = Cipher.getInstance("DES")  // Use AES-256-GCM

// ✅ GOOD - Keystore encryption
val encryptedData = keystoreManager.encrypt(sensitiveData)
secureStorage.store(encryptedData)

If security concerns found, classify as CRITICAL using `reference/priority-framework.md`

Testing Review

Reference: reference/testing-patterns.md

Required Test Coverage:

• ViewModels: Unit tests for state transitions, actions, error scenarios
• Repositories: Unit tests for data transformations, error handling
• Business logic: Unit tests for complex algorithms, calculations
• Edge cases: Null inputs, empty states, network failures, concurrent operations

Test Quality:

// ✅ GOOD - Tests behavior
@Test
fun `when login succeeds then state updates to success`() = runTest {
    val viewModel = LoginViewModel(mockRepository)

    coEvery { mockRepository.login(any(), any()) } returns Result.success(User())

    viewModel.onLoginClicked("user", "pass")

    viewModel.state.test {
        assertEquals(LoginState.Success, awaitItem())
    }
}

// ❌ BAD - Tests implementation
@Test
fun `repository is called with correct parameters`() {
    // This is testing internal implementation, not behavior
}

Testing Frameworks:

• JUnit 5 for test structure
• MockK for mocking
• Turbine for Flow testing
• Kotlinx-coroutines-test for coroutine testing

Code Quality

Null Safety

• No !! (non-null assertion) without clear safety guarantee
• Platform types (from Java) handled with explicit nullability
• Nullable types have proper null checks or use safe operators (?., ?:)

// ❌ BAD - Unsafe assertion
val result = apiService.getData()!!  // Could crash

// ✅ GOOD - Safe handling
val result = apiService.getData() ?: return State.Error("No data")

// ❌ BAD - Platform type unchecked
val intent: Intent = getIntent()  // Could be null from Java
intent.getStringExtra("key")  // Potential NPE

// ✅ GOOD - Explicit nullability
val intent: Intent? = getIntent()
intent?.getStringExtra("key")

Documentation

• Public APIs: Have KDoc comments explaining purpose, parameters, return values
• Complex algorithms: Explained in comments
• Non-obvious behavior: Documented with rationale

// ✅ GOOD - Documented public API
/**
 * Encrypts the given data using AES-256-GCM with a key from Android Keystore.
 *
 * @param plaintext The data to encrypt
 * @return Result containing encrypted data or encryption error
 */
suspend fun encrypt(plaintext: ByteArray): Result<EncryptedData>

Style Compliance

Reference: docs/STYLE_AND_BEST_PRACTICES.md

Only flag style issues if:

• Not caught by linters (Detekt, ktlint)
• Have architectural implications
• Significantly impact readability

Skip minor formatting (spaces, line breaks, etc.) - linters handle this.

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Providing Feedback

Use reference/review-psychology.md for phrasing guidance.

Key principles:

• Ask questions for design decisions: "Can we use the existing BitwardenTextField component here?"
• Be prescriptive for clear violations: "Change MutableStateFlow to StateFlow (MVVM pattern requirement)"
• Explain rationale: "This exposes mutable state, violating unidirectional data flow"
• Use I-statements: "It's hard for me to understand this logic without comments"
• Avoid condescension: Don't use "just", "simply", "obviously"

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Infrastructure Review Checklist

Multi-Pass Strategy

First Pass: Understand the Change

1. Identify the goal:

• What problem does this solve?
• Is this optimization, fix, or new capability?
• What's the expected impact?

2. Assess risk:

• Does this affect production builds?
• Could this break CI/CD pipelines?
• Impact on developer workflow?

3. Performance implications:

• Will builds be faster or slower?
• CI time impact?
• Resource usage changes?

Second Pass: Verify Implementation

4. Configuration correctness:

• Syntax valid?
• References correct?
• Secrets/credentials handled securely?

5. Impact analysis:

• What workflows/builds are affected?
• Rollback plan if this breaks?
• Documentation for team?

6. Testing strategy:

• How can this be tested before merge?
• Canary/gradual rollout possible?
• Monitoring for issues post-merge?

What to CHECK

✅ Configuration Correctness

• YAML/Groovy syntax valid
• File references correct
• Version numbers/tags valid
• Conditional logic sound

✅ Security

• No hardcoded secrets or credentials
• GitHub secrets used properly
• Permissions appropriately scoped
• No sensitive data in logs

✅ Performance Impact

• Build time implications understood
• CI queue time impact assessed
• Resource usage reasonable

✅ Rollback Plan

• Can this be reverted easily?
• Dependencies on other changes?
• Gradual rollout possible?

✅ Documentation

• Changes documented for team?
• README or CONTRIBUTING updated?
• Breaking changes clearly noted?

What to SKIP

❌ Bikeshedding Configuration - Unless clear performance/maintenance benefit ❌ Over-Optimization - Unless current system has proven problems ❌ Suggesting Major Rewrites - Unless current approach is fundamentally broken

Red Flags

🚩 Hardcoded secrets - Use GitHub secrets or secure storage 🚩 No rollback plan - Critical infrastructure should be revertible 🚩 Untested changes - CI changes should be validated 🚩 Breaking changes without notice - Team needs advance warning 🚩 Performance regression - Builds shouldn't get significantly slower

Key Questions to Ask

Use reference/review-psychology.md for phrasing:

• "What's the rollback plan if this breaks CI?"
• "Can we test this on a feature branch before main?"
• "Will this impact build times? By how much?"
• "Should this be documented in CONTRIBUTING.md?"

Common Infrastructure Patterns

GitHub Actions

# ✅ GOOD - Secure, clear, tested
name: Build and Test
on:
  pull_request:
    branches: [main]

jobs:
  build:
    runs-on: ubuntu-latest
    timeout-minutes: 30  # Prevent runaway builds
    steps:
      - uses: actions/checkout@v4
      - name: Run tests
        env:
          API_KEY: ${{ secrets.API_KEY }}  # Secure secret usage
        run: ./gradlew test

# ❌ BAD - Insecure, unclear
name: Build
on: push  # Too broad, runs on all branches
jobs:
  build:
    runs-on: ubuntu-latest
    # No timeout - could run forever
    steps:
      - run: |
          export API_KEY="hardcoded_key_here"  # Hardcoded secret!
          ./gradlew test

Gradle Configuration

// ✅ GOOD - Clear, maintainable
dependencies {
    implementation(libs.androidx.core.ktx)  // Version catalog
    implementation(libs.hilt.android)

    testImplementation(libs.junit5)
    testImplementation(libs.mockk)
}

// ❌ BAD - Hardcoded versions
dependencies {
    implementation("androidx.core:core-ktx:1.12.0")  // Hardcoded version
    implementation("com.google.dagger:hilt-android:2.48")
}

Build Optimization

// ✅ GOOD - Parallel, cached
tasks.register("checkAll") {
    dependsOn("detekt", "ktlintCheck", "testStandardDebug")
    group = "verification"
    description = "Run all checks in parallel"

    // Enable caching for faster builds
    outputs.upToDateWhen { false }
}

// ❌ BAD - Sequential, no caching
tasks.register("checkAll") {
    doLast {
        exec { commandLine("./gradlew", "detekt") }
        exec { commandLine("./gradlew", "ktlintCheck") }  // Sequential
        exec { commandLine("./gradlew", "test") }
    }
}

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Example Review

## Summary
Optimizes CI build by parallelizing test execution and caching dependencies

Impact: Estimated 40% reduction in CI time (12 min → 7 min per build)

## Critical Issues
None

## Suggested Improvements

**.github/workflows/build.yml:23** - Add timeout for safety

jobs: build: runs-on: ubuntu-latest timeout-minutes: 30 # Prevent builds from hanging steps:

...

This prevents runaway builds if something goes wrong.

**.github/workflows/build.yml:45** - Consider matrix strategy for module tests
Can we run module tests in parallel using a matrix strategy?

strategy: matrix: module: [app, data, network, ui] jobs: test: runs-on: ubuntu-latest steps:

• run: ./gradlew :${{ matrix.module }}:test

This could further reduce CI time.

**build.gradle.kts:12** - Document caching strategy
Can we add a comment explaining the caching configuration?
Future maintainers will appreciate understanding why these specific cache keys are used.

## Rollback Plan
If CI breaks:
- Revert commit: `git revert [commit-hash]`
- Previous workflow available at: `.github/workflows/build.yml@main^`
- Monitor CI times at: https://github.com/[org]/[repo]/actions

Refactoring Review Checklist

Multi-Pass Strategy

First Pass: Understand the Refactoring

1. Understand the goal:

• What pattern is being improved?
• Why is this refactoring needed?
• What's the scope of changes?

2. Assess completeness:

• Are all instances refactored or just some?
• Are there related areas that should also change?
• Is the migration complete or partial?

3. Risk assessment:

• Does this change behavior?
• How many files affected?
• Are tests updated to reflect changes?

Second Pass: Verify Consistency

4. Pattern consistency:

• Is the new pattern applied consistently throughout?
• Are there missed instances of the old pattern?
• Does this match established project patterns?

5. Migration completeness:

• Old pattern fully removed or deprecated?
• All usages updated?
• Documentation updated?

6. Test coverage:

• Do tests still pass?
• Are tests refactored to match?
• Does behavior remain unchanged?

What to CHECK

✅ Pattern Consistency

• New pattern applied consistently across all touched code
• Follows established project patterns (MVVM, DI, error handling)
• No mix of old and new patterns

✅ Migration Completeness

• All instances of old pattern updated?
• Deprecated methods removed or marked @Deprecated?
• Related code also updated (tests, docs)?

✅ Behavior Preservation

• Refactoring doesn't change behavior
• Tests still pass
• Edge cases still handled

✅ Deprecation Strategy (if applicable)

• Old APIs marked @Deprecated with migration guidance
• Replacement clearly documented
• Timeline for removal specified

What to SKIP

❌ Suggesting Additional Refactorings - Unless directly related to current changes ❌ Scope Creep - Don't request refactoring of untouched code ❌ Perfection - Better code is better than perfect code

Red Flags

🚩 Incomplete migration - Mix of old and new patterns 🚩 Behavior changes - Refactoring shouldn't change behavior 🚩 Broken tests - Tests should be updated to match refactoring 🚩 Undocumented pattern - New pattern should be clear to team

Key Questions to Ask

Use reference/review-psychology.md for phrasing:

• "I see the old pattern still used in [file:line] - should that be updated too?"
• "Can we add @Deprecated to the old method with migration guidance?"
• "How do we ensure this behavior remains the same?"
• "Should this pattern be documented in ARCHITECTURE.md?"

Common Refactoring Patterns

Extract Interface/Repository

// ✅ GOOD - Complete migration
interface FeatureRepository {
    suspend fun getData(): Result<Data>
}

class FeatureRepositoryImpl @Inject constructor(
    private val apiService: FeatureApiService
) : FeatureRepository {
    override suspend fun getData(): Result<Data> = runCatching {
        apiService.fetchData()
    }
}

// All usages updated to inject interface
class FeatureViewModel @Inject constructor(
    private val repository: FeatureRepository  // Interface
) : ViewModel()

// ❌ BAD - Incomplete migration
// Some files still inject FeatureRepositoryImpl directly

Modernize Error Handling

// ✅ GOOD - Complete migration
// Old exception-based removed
suspend fun fetchData(): Result<Data> = runCatching {
    apiService.getData()
}

// All call sites updated
repository.fetchData().fold(
    onSuccess = { /* handle */ },
    onFailure = { /* handle */ }
)

// ❌ BAD - Mixed patterns
// Some functions use Result, others still throw exceptions

Extract Reusable Component

// ✅ GOOD - Complete extraction
// Component moved to :ui module
@Composable
fun BitwardenButton(
    text: String,
    onClick: () -> Unit,
    modifier: Modifier = Modifier
)

// All usages updated to use new component
// Old inline button implementations removed

// ❌ BAD - Incomplete extraction
// Some screens use new component, others still have inline implementation

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Example Reviews

Example 1: Refactoring with Incomplete Migration

Context: Refactoring authentication to Repository pattern, but one ViewModel still uses old pattern

Summary Comment:

**Overall Assessment:** REQUEST CHANGES

**Critical Issues:**
- Incomplete migration (app/vault/VaultViewModel.kt:89)

See inline comments for details.

Inline Comment 1 (on app/vault/VaultViewModel.kt:89):

**IMPORTANT**: Incomplete migration

<details>
<summary>Details and fix</summary>

This ViewModel still injects AuthManager directly. Should it use AuthRepository like the other 11 ViewModels?

\```kotlin
// Current (old pattern)
class VaultViewModel @Inject constructor(
    private val authManager: AuthManager
)

// Should be (new pattern)
class VaultViewModel @Inject constructor(
    private val authRepository: AuthRepository
)
\```

This is the only ViewModel still using the old pattern.
</details>

Inline Comment 2 (on data/auth/AuthManager.kt:1):

**SUGGESTED**: Add deprecation notice

<details>
<summary>Details</summary>

Can we add @Deprecated to AuthManager to guide future development?

\```kotlin
@Deprecated(
    message = "Use AuthRepository interface instead",
    replaceWith = ReplaceWith("AuthRepository"),
    level = DeprecationLevel.WARNING
)
class AuthManager @Inject constructor(...)
\```

This helps prevent new code from using the old pattern.
</details>

---

Example 2: Clean Refactoring (No Issues)

Context: Refactoring with complete migration, all patterns followed correctly, tests passing

Review Comment:

**Overall Assessment:** APPROVE

Clean refactoring moving ExitManager to :ui module. Follows established patterns, eliminates duplication, tests updated correctly.

Token count: ~30 tokens (vs ~800 for verbose format)

Why this works:

• 3 lines total
• Clear approval decision
• Briefly notes what was done
• No elaborate sections, checkmarks, or excessive praise
• Author gets immediate green light to merge

What NOT to do for clean refactorings:

❌ DO NOT create these sections:

## Summary
This PR successfully refactors ExitManager into shared code...

## Key Strengths
- ✅ Follows established module organization patterns
- ✅ Removes code duplication between apps
- ✅ Improves test coverage
- ✅ Maintains consistent behavior
[...20 more checkmarks...]

## Code Quality & Architecture
**Architectural Compliance:** ✅
- Correctly places manager in :ui module
- Follows established pattern for UI-layer managers
[...detailed analysis...]

## Changes
- ✅ Moved ExitManager interface from app → ui module
- ✅ Moved ExitManagerImpl from app → ui module
[...listing every file...]

This is excessive. For clean PRs: 2-3 lines maximum.

UI Refinement Review Checklist

Multi-Pass Strategy

First Pass: Visual Changes

1. Understand the changes:

• What visual/UX problem is being solved?
• Are there designs or screenshots to reference?
• Is this a bug fix or enhancement?

2. Component usage:

• Using existing components from :ui module?
• Any new custom components created?
• Could existing components be reused?

Second Pass: Implementation Review

3. Compose best practices:

• Composables properly structured?
• State hoisted correctly?
• Preview composables included?

4. Accessibility:

• Content descriptions for images/icons?
• Semantic properties for screen readers?
• Touch targets meet minimum size (48dp)?

5. Design consistency:

• Using theme colors, spacing, typography?
• Consistent with other screens?
• Responsive to different screen sizes?

What to CHECK

✅ Compose Best Practices

• Composables are stateless where possible
• State hoisting follows patterns
• Side effects (LaunchedEffect, DisposableEffect) used correctly
• Preview composables provided for development

✅ Component Reuse

• Using existing BitwardenButton, BitwardenTextField, etc.?
• Could custom UI be replaced with existing components?
• New reusable components placed in :ui module?

✅ Accessibility

• contentDescription for icons and images
• semantics for custom interactions
• Sufficient contrast ratios
• Touch targets ≥ 48dp minimum

✅ Design Consistency

• Using BitwardenTheme colors (not hardcoded)
• Using BitwardenTheme spacing (16.dp, 8.dp, etc.)
• Using BitwardenTheme typography styles
• Consistent with existing screen patterns

✅ Responsive Design

• Handles different screen sizes?
• Scrollable content where appropriate?
• Landscape orientation considered?

What to SKIP

❌ Deep Architecture Review - Unless ViewModel changes are substantial ❌ Business Logic Review - Focus is on presentation, not logic ❌ Security Review - Unless UI exposes sensitive data improperly

Red Flags

🚩 Duplicating existing components - Should reuse from :ui module 🚩 Hardcoded colors/dimensions - Should use theme 🚩 Missing accessibility properties - Critical for screen readers 🚩 State management in UI - Should be hoisted to ViewModel

Key Questions to Ask

Use reference/review-psychology.md for phrasing:

• "Can we use BitwardenButton here instead of this custom button?"
• "Should this color come from BitwardenTheme instead of being hardcoded?"
• "How will this look on a small screen?"
• "Is there a contentDescription for this icon?"

Common Patterns

Composable Structure

// ✅ GOOD - Stateless, hoisted state
@Composable
fun FeatureScreen(
    state: FeatureState,
    onActionClick: () -> Unit,
    modifier: Modifier = Modifier
) {
    // UI rendering only
}

// ❌ BAD - Business state in composable
@Composable
fun FeatureScreen() {
    var userData by remember { mutableStateOf<User?>(null) }  // Business state should be in ViewModel
    var isLoading by remember { mutableStateOf(false) }  // App state should be in ViewModel
    // ...
}

// ✅ OK - UI-local state in composable
@Composable
fun LoginForm(onSubmit: (String, String) -> Unit) {
    var username by remember { mutableStateOf("") }  // UI-local input state is fine
    var password by remember { mutableStateOf("") }
    // Hoist only as high as needed
}

Theme Usage

// ✅ GOOD - Using theme
Text(
    text = "Title",
    style = BitwardenTheme.typography.titleLarge,
    color = BitwardenTheme.colorScheme.primary
)

// Design system uses 4.dp increments (4, 8, 12, 16, 24, 32, etc.)
Spacer(modifier = Modifier.height(16.dp))

// ❌ BAD - Hardcoded
Text(
    text = "Title",
    style = TextStyle(fontSize = 24.sp, fontWeight = FontWeight.Bold),  // Should use theme
    color = Color(0xFF0000FF)  // Should use theme color
)

Spacer(modifier = Modifier.height(17.dp))  // Non-standard spacing

Accessibility

// ✅ GOOD - Interactive element with description
Icon(
    painter = painterResource(R.drawable.ic_password),
    contentDescription = "Password visibility toggle",
    modifier = Modifier.clickable { onToggle() }
)

// ✅ GOOD - Decorative icon with explicit null
Icon(
    painter = painterResource(R.drawable.ic_check),
    contentDescription = null,  // Decorative icon next to descriptive text
    tint = BitwardenTheme.colorScheme.success
)

// ❌ BAD - Interactive element missing description
Icon(
    painter = painterResource(R.drawable.ic_delete),
    contentDescription = null,  // Interactive elements need descriptions
    modifier = Modifier.clickable { onDelete() }
)

Prioritizing Findings

Use reference/priority-framework.md to classify findings as Critical/Important/Suggested/Optional.

Output Format

See examples/review-outputs.md for the required output format and inline comment structure.

Example Review

## Summary
Updates login screen layout for improved visual hierarchy and touch targets

## Critical Issues
None

## Suggested Improvements

**app/auth/LoginScreen.kt:67** - Can we use BitwardenTextField?
This custom text field looks very similar to `ui/components/BitwardenTextField.kt:89`.
Would using the existing component maintain consistency?

**app/auth/LoginScreen.kt:123** - Add contentDescription

Icon( painter = painterResource(R.drawable.ic_visibility), contentDescription = "Show password", // Add for accessibility modifier = Modifier.clickable { onToggleVisibility() } )

**app/auth/LoginScreen.kt:145** - Use design system spacing

// Current Spacer(modifier = Modifier.height(17.dp))

// Design system uses 4.dp increments (4, 8, 12, 16, 24, 32, etc.) Spacer(modifier = Modifier.height(16.dp))

Review Output Examples

Well-structured code reviews demonstrating appropriate depth, tone, and formatting for different change types.

Table of Contents

Format Reference:

• Quick Format Reference
• Inline Comment Format
• Summary Comment Format

Examples:

• Example 1: Clean PR (No Issues)
• Example 2: Dependency Update with Breaking Changes
• Example 3: Feature Addition with Critical Issues

Anti-Patterns:

• ❌ Anti-Patterns to Avoid
• Problem: Verbose Summary with Multiple Sections
• Problem: Praise-Only Inline Comments
• Problem: Missing `<details>` Tags

Summary:

• Summary

---

Quick Format Reference

Inline Comment Format (REQUIRED)

MUST use `<details>` tags. Only severity + description visible; all other content collapsed.

[emoji] **[SEVERITY]**: [One-line issue description]

<details>
<summary>Details and fix</summary>

[Code example or specific fix]

[Rationale explaining why]

Reference: [docs link if applicable]
</details>

Severity Levels:

• ❌ CRITICAL - Blocking, must fix (security, crashes, architecture violations)
• ⚠️ IMPORTANT - Should fix (missing tests, quality issues)
• ♻️ DEBT - Technical debt (duplication, convention violations, future rework needed)
• 🎨 SUGGESTED - Nice to have (refactoring, improvements)
• ❓ QUESTION - Seeking clarification (requirements, design decisions)

Summary Comment Format

Uses the agent's posting-review-summary skill format. Surface ❌ CRITICAL issues at the top level for immediate visibility, wrap the full findings list in <details> for scannability.

**Overall Assessment:** APPROVE / REQUEST CHANGES

[1-2 neutral sentences describing what was reviewed]

**Critical Issues** (if any):
- ❌ [One-line summary with file:line]

<details>
<summary>All findings</summary>

- ❌ **CRITICAL**: [description] (`file:line`)
- ⚠️ **IMPORTANT**: [description] (`file:line`)
- ♻️ **DEBT**: [description] (`file:line`)
- 🎨 **SUGGESTED**: [description] (`file:line`)
- ❓ **QUESTION**: [description] (`file:line`)
</details>

For clean PRs with no findings, omit both sections entirely — verdict + 1-2 sentences is sufficient.

GitHub pitfall: Never use # followed by a number in comment text (e.g., #42, #PR123). GitHub autolinks these to issues/PRs. Use Finding 1: or item 42 instead.

---

Example 1: Clean PR (No Issues)

Context: Moving shared code to common module, complete migration, all patterns followed

Review Comment:

**Overall Assessment:** APPROVE

Clean refactoring that moves ExitManager to :ui module, eliminating duplication between apps.

Why this works:

• Immediate approval visible (2-3 lines)
• One sentence acknowledging the work
• No unnecessary sections or elaborate praise
• Author gets quick feedback and can proceed

---

Example 2: Dependency Update with Breaking Changes

Context: Major version update requiring code migration

Summary Comment:

**Overall Assessment:** REQUEST CHANGES

**Critical Issues:**
- API migration required for Retrofit 3.0 breaking changes (network/api/BitwardenApiService.kt:34)

See inline comments for migration details.

Inline Comment 1 (on network/api/BitwardenApiService.kt:34):

❌ **CRITICAL**: API migration required for Retrofit 3.0

<details>
<summary>Details and fix</summary>

Retrofit 3.0 removes the `Call<T>` return type. All 12 API methods in this file need migration:

// Current (deprecated in Retrofit 3.0) @GET("api/accounts/profile") fun getProfile(): Call<ProfileResponse>

// Must migrate to @GET("api/accounts/profile") suspend fun getProfile(): Response<ProfileResponse>

Breaking API change affects:
- 12 methods in BitwardenApiService
- 8 methods in VaultApiService
- All call sites using enqueue/execute
- Test utilities

Consider creating separate PR for this migration given the scope.

Reference: [Retrofit 3.0 migration guide](https://square.github.io/retrofit/changelogs/changelog-3.x/)
</details>

Key Features:

• Minimal summary (2-3 lines)
• Full details in collapsed inline comment
• Specific file:line references
• Code examples in <details>
• Migration guidance and scope assessment

---

Example 3: Feature Addition with Critical Issues

Context: Implements PIN unlock for vault access

Summary Comment:

**Overall Assessment:** REQUEST CHANGES

**Critical Issues:**
- Exposes mutable state violating MVVM (UnlockViewModel.kt:78)
- PIN stored without encryption - SECURITY ISSUE (UnlockRepository.kt:145)

See inline comments for all issues and suggestions.

Inline Comment 1 (on app/vault/unlock/UnlockViewModel.kt:78):

❌ **CRITICAL**: Exposes mutable state

<details>
<summary>Details and fix</summary>

Change `MutableStateFlow<State>` to `StateFlow<State>`:

// Current (problematic) val unlockState: MutableStateFlow<UnlockState>

// Should be private val _unlockState = MutableStateFlow<UnlockState>() val unlockState: StateFlow<UnlockState> = _unlockState.asStateFlow()

Exposing MutableStateFlow allows external mutation, violating MVVM unidirectional data flow.

Reference: docs/ARCHITECTURE.md#mvvm-pattern
</details>

Inline Comment 2 (on data/vault/UnlockRepository.kt:145):

❌ **CRITICAL**: PIN stored without encryption - SECURITY ISSUE

<details>
<summary>Details and fix</summary>

Storing PIN in plaintext SharedPreferences exposes it to backup systems and rooted devices.

// Current (CRITICAL SECURITY ISSUE) sharedPreferences.edit { putString(KEY_PIN, pin) }

// Must use Android Keystore encryption suspend fun storePin(pin: String): Result<Unit> = runCatching { val encrypted = keystoreManager.encrypt(pin.toByteArray()) encryptedPrefs.putBytes(KEY_PIN, encrypted) }

Use Android Keystore encryption or EncryptedSharedPreferences per security architecture.

Reference: docs/ARCHITECTURE.md#security
</details>

Inline Comment 3 (on app/vault/unlock/UnlockViewModel.kt:92):

⚠️ **IMPORTANT**: Missing error handling test

<details>
<summary>Details and fix</summary>

Add test to prevent regression if error handling changes:

@Test fun when incorrect PIN entered then returns error state() = runTest { val viewModel = UnlockViewModel(mockRepository) coEvery { mockRepository.validatePin("1234") } returns Result.failure(InvalidPinException())

viewModel.onPinEntered("1234")

assertEquals(UnlockState.Error("Invalid PIN"), viewModel.state.value) }

Ensures error flow remains robust across refactorings.
</details>

Inline Comment 4 (on app/vault/unlock/UnlockViewModel.kt:105):

🎨 **SUGGESTED**: Consider rate limiting for PIN attempts

<details>
<summary>Details and fix</summary>

Currently allows unlimited attempts, which could enable brute force attacks.

private var attemptCount = 0 private var lockoutUntil: Instant? = null

fun onPinEntered(pin: String) { if (isLockedOut()) { _state.value = UnlockState.LockedOut(lockoutUntil!!) return } // ... validate PIN ... if (invalid) { attemptCount++ if (attemptCount >= MAX_ATTEMPTS) { lockoutUntil = clock.millis() + 15.minutes } } }

Would add security layer against brute force. Consider discussing threat model with security team.
</details>

Inline Comment 5 (on app/vault/unlock/UnlockScreen.kt:134):

❓ **QUESTION**: Can we use BitwardenTextField?

<details>
<summary>Details</summary>

This custom PIN input field looks similar to `ui/components/BitwardenTextField.kt:67`.

Would using the existing component maintain consistency and reduce custom UI code?
</details>

Key Features:

• Minimal summary (3-4 lines) with critical issues only
• Each issue gets separate inline comment with <details> tag
• Multiple severity levels demonstrated (CRITICAL, IMPORTANT, SUGGESTED, QUESTION)
• Mix of prescriptive fixes and collaborative questions
• Code examples collapsed in <details>
• No "Good Practices" or "Action Items" sections

---

❌ Anti-Patterns to Avoid

Problem: Verbose Summary with Multiple Sections

What NOT to do:

### Review Complete ✅

## Summary
[Lengthy description of what the PR does]

### Strengths 👍
1. **Excellent documentation** - KDoc comments are comprehensive
2. **Proper fail-closed design** - Security defaults to rejection
3. **Defense in depth** - Multiple validation layers
[7 total items with elaboration]

### Critical Issues ⚠️
- Missing test coverage for security-critical code (with full details)
- [More issues with full explanations]

### Recommendations 🎨
- [Multiple recommendations]

### Test Coverage Status 📊
- [Analysis]

### Architecture Compliance ✅
- [Analysis]

## Recommendation
**Conditional approval** with follow-up...

Why this is wrong:

• 800+ tokens for a summary comment
• Multiple sections (Strengths, Recommendations, Test Coverage, Architecture)
• Elaborates on positive aspects ("Excellent documentation...")
• Duplicates critical issues (summary has details + inline comments have same details)
• Creates visual clutter in PR conversation

Correct approach:

**Overall Assessment:** REQUEST CHANGES

**Critical Issues:**
- Missing test coverage for security-critical code (PasswordManagerSignatureVerifierImpl.kt:47)

See inline comments for details.

Key differences:

• 3-5 lines vs 800+ tokens
• Verdict + critical issues only
• All details belong in inline comments
• No positive commentary sections
• Scales with PR complexity, not analysis thoroughness

Problem: Praise-Only Inline Comments

What NOT to do:

Creating inline comment on AuthenticatorBridgeManagerImpl.kt:73:

👍 **Excellent integration of signature verification**

The signature verification is properly integrated into the connection flow:
- Checked during initialization (line 73)
- Checked before binding (line 134)
- Ensures only validated apps can connect

This is exactly the right approach for fail-safe security.

Why this is wrong:

• Entire comment is positive feedback with no actionable issue
• Takes up space in PR conversation
• Distracts from actual issues
• Violates "focus on actionable feedback" principle

Correct approach:

• Do not create this comment at all
• Reserve inline comments exclusively for issues requiring attention

Problem: Missing <details> Tags

What NOT to do:

❌ **CRITICAL**: Missing test coverage for security-critical code

The `@OmitFromCoverage` annotation excludes this entire class from test coverage.

**Problems:**
1. No validation that certificate hashes match actual Bitwarden certificates
2. No verification of fail-closed behavior on edge cases
3. No tests for multiple signer rejection logic
4. Certificate hash typos would go undetected until production

**Recommendation:**
Replace `@OmitFromCoverage` with proper unit tests.

Example test structure:
[long code block]

Security-critical code should have the highest test coverage, not be omitted.

Why this is wrong:

• All content visible immediately (code examples, problems list, rationale)
• Creates visual clutter in PR conversation
• Makes it hard to scan multiple issues quickly

Correct approach:

❌ **CRITICAL**: Missing test coverage for security-critical code

<details>
<summary>Details and fix</summary>

The `@OmitFromCoverage` annotation excludes this entire class from test coverage.

**Problems:**
1. No validation that certificate hashes match actual Bitwarden certificates
2. No verification of fail-closed behavior on edge cases
3. No tests for multiple signer rejection logic
4. Certificate hash typos would go undetected until production

**Recommendation:**
Replace `@OmitFromCoverage` with proper unit tests.

Example test structure:
[code block]

Security-critical code should have the highest test coverage, not be omitted.
</details>

Key difference: Only severity + one-line description visible. All details collapsed.

---

Summary

Always use:

• Minimal summary (verdict + critical issues)
• Separate inline comments with <details> tags
• Hybrid emoji + text severity prefixes
• Focus exclusively on actionable feedback

Never use:

• Multiple summary sections (Strengths, Recommendations, etc.)
• Praise-only inline comments
• Duplication between summary and inline comments
• Verbose analysis in summary (belongs in inline comments)

Architectural Patterns Quick Reference

Quick reference for Bitwarden Android architectural patterns during code reviews. For comprehensive details, read docs/ARCHITECTURE.md and docs/STYLE_AND_BEST_PRACTICES.md.

Table of Contents

Core Patterns:

• MVVM + UDF Pattern
• ViewModel Structure
• UI Layer (Compose)
• Hilt Dependency Injection
• ViewModels
• Repositories and Managers
• Clock/Time Handling
• Module Organization
• Error Handling
• Use Result Types, Not Exceptions
• Quick Checklist

---

MVVM + UDF Pattern

ViewModel Structure

✅ GOOD - Proper state encapsulation:

@HiltViewModel
class FeatureViewModel @Inject constructor(
    private val repository: FeatureRepository
) : ViewModel() {
    // Private mutable state
    private val _state = MutableStateFlow<FeatureState>(FeatureState.Initial)

    // Public immutable state
    val state: StateFlow<FeatureState> = _state.asStateFlow()

    // Actions as functions, state updated via internal action
    fun onActionClicked() {
        viewModelScope.launch {
            val result = repository.performAction()
            sendAction(FeatureAction.Internal.ActionComplete(result))
        }
    }

    // The ViewModel has a handler that processes the internal action
    private fun handleInternalAction(action: FeatureAction.Internal) {
        when (action) {
            is FeatureAction.Internal.ActionComplete -> {
                // The action handler evaluates the result and updates state
                action.result.fold(
                    onSuccess = { _state.value = State.Success },
                    onFailure = { _state.value = State.Error(it) }
                )
            }
        }
    }
}

❌ BAD - Common violations:

class FeatureViewModel : ViewModel() {
    // ❌ Exposes mutable state
    val state: MutableStateFlow<FeatureState>

    // ❌ Business logic in ViewModel
    fun onSubmit() {
        val encrypted = encryptionManager.encrypt(data)  // Should be in Repository
        _state.value = FeatureState.Success
    }

    // ❌ Direct Android framework dependency
    fun onCreate(context: Context) {  // ViewModels shouldn't depend on Context
        // ...
    }
}

Key Rules:

• Expose StateFlow<T>, never MutableStateFlow<T>
• Delegate business logic to Repository/Manager
• No direct Android framework dependencies (except ViewModel, SavedStateHandle)
• Use viewModelScope for coroutines

Reference: docs/ARCHITECTURE.md#mvvm-pattern

---

UI Layer (Compose)

✅ GOOD - Stateless, observes only:

@Composable
fun FeatureScreen(
    state: FeatureState,
    onActionClick: () -> Unit,
    modifier: Modifier = Modifier
) {
    Column(modifier = modifier) {
        when (state) {
            is FeatureState.Loading -> LoadingIndicator()
            is FeatureState.Success -> SuccessContent(state.data)
            is FeatureState.Error -> ErrorMessage(state.error)
        }

        BitwardenButton(
            text = "Action",
            onClick = onActionClick  // Sends event to ViewModel
        )
    }
}

❌ BAD - Stateful, modifies state:

@Composable
fun FeatureScreen(viewModel: FeatureViewModel) {
    var localState by remember { mutableStateOf(...) }  // ❌ State in UI

    Button(onClick = {
        viewModel._state.value = FeatureState.Loading  // ❌ Directly modifying ViewModel state
    })
}

Key Rules:

• Compose screens observe state, never modify
• User actions passed as events/callbacks to ViewModel
• No business logic in UI layer
• Use existing components from :ui module

---

Hilt Dependency Injection

ViewModels

✅ GOOD - Interface injection:

@HiltViewModel
class FeatureViewModel @Inject constructor(
    private val repository: FeatureRepository,  // Interface, not implementation
    private val authManager: AuthManager,
    savedStateHandle: SavedStateHandle
) : ViewModel()

❌ BAD - Common violations:

// ❌ No @HiltViewModel annotation
class FeatureViewModel @Inject constructor(...)

// ❌ Injecting implementation instead of interface
class FeatureViewModel @Inject constructor(
    private val repository: FeatureRepositoryImpl  // Should inject interface
)

// ❌ Manual instantiation
class FeatureViewModel : ViewModel() {
    private val repository = FeatureRepositoryImpl()  // Should use @Inject
}

Key Rules:

• Annotate with @HiltViewModel
• Use @Inject constructor
• Inject interfaces, not implementations
• Use SavedStateHandle for process death survival

Reference: docs/ARCHITECTURE.md#dependency-injection

---

Repositories and Managers

✅ GOOD - Implementation with @Inject:

interface FeatureRepository {
    suspend fun fetchData(): Result<Data>
}

class FeatureRepositoryImpl @Inject constructor(
    private val apiService: FeatureApiService,
    private val database: FeatureDao
) : FeatureRepository {
    override suspend fun fetchData(): Result<Data> = runCatching {
        apiService.getData()
    }
}

Module provides interface:

@Module
@InstallIn(SingletonComponent::class)
abstract class DataModule {
    @Binds
    @Singleton
    abstract fun bindFeatureRepository(
        impl: FeatureRepositoryImpl
    ): FeatureRepository
}

Key Rules:

• Define interface for abstraction
• Implementation uses @Inject constructor
• Module binds implementation to interface
• Appropriate scoping (@Singleton, @ViewModelScoped)

---

Clock/Time Handling

Time-dependent code must use injected Clock rather than direct Instant.now() or DateTime.now() calls. This follows the same DI principle as other dependencies.

✅ GOOD - Injected Clock:

// ViewModel with Clock injection
class MyViewModel @Inject constructor(
    private val clock: Clock,
) {
    fun save() {
        val timestamp = clock.instant()
    }
}

// Extension function with Clock parameter
fun State.getTimestamp(clock: Clock): Instant =
    existingTime ?: clock.instant()

❌ BAD - Static/direct calls:

// Hidden dependency, non-testable
val timestamp = Instant.now()
val dateTime = DateTime.now()

Key Rules:

• Inject Clock via Hilt constructor (like other dependencies)
• Pass Clock as parameter to extension functions
• Clock is provided via CoreModule as singleton
• Enables deterministic testing with Clock.fixed(...)

Reference: docs/STYLE_AND_BEST_PRACTICES.md#best-practices--time-and-clock-handling

---

Module Organization

android/
├── core/           # Shared utilities (cryptography, analytics, logging)
├── data/           # Repositories, database, domain models
├── network/        # API clients, network utilities
├── ui/             # Reusable Compose components, theme
├── app/            # Application, feature screens, ViewModels
└── authenticator/  # Authenticator app (separate from password manager)

Correct Placement:

• UI screens and ViewModels → :app
• Reusable Compose components → :ui
• Data models and Repositories → :data
• API services → :network
• Cryptography, logging → :core

Check for:

• No circular dependencies
• Correct module placement
• Proper visibility (internal vs public)

Reference: docs/ARCHITECTURE.md#module-structure

---

Error Handling

Use Result Types, Not Exceptions

✅ GOOD - Result-based:

// Repository
suspend fun fetchData(): Result<Data> = runCatching {
    apiService.getData()
}

// ViewModel
fun onFetch() {
    viewModelScope.launch {
        val result = repository.fetchData()
        sendAction(FeatureAction.Internal.FetchComplete(result))
    }
}

❌ BAD - Exception-based in business logic:

// ❌ Don't throw in business logic
suspend fun fetchData(): Data {
    try {
        return apiService.getData()
    } catch (e: Exception) {
        throw FeatureException(e)  // Don't throw in repositories
    }
}

// ❌ Try-catch in ViewModel
fun onFetch() {
    viewModelScope.launch {
        try {
            val data = repository.fetchData()
            sendAction(FeatureAction.Internal.FetchComplete(data))
        } catch (e: Exception) {
            sendAction(FeatureAction.Internal.FetchComplete(e))
        }
    }
}

Key Rules:

• Use Result<T> return types in repositories
• Use runCatching { } to wrap API calls
• Handle results with .fold() in ViewModels
• Don't throw exceptions in business logic

Reference: docs/ARCHITECTURE.md#error-handling

---

Quick Checklist

Architecture

• [ ] ViewModels expose StateFlow, not MutableStateFlow?
• [ ] Business logic in Repository, not ViewModel?
• [ ] Using Hilt DI (@HiltViewModel, @Inject constructor)?
• [ ] Injecting interfaces, not implementations?
• [ ] Time-dependent code uses injected Clock (not Instant.now())?
• [ ] Correct module placement?

Error Handling

• [ ] Using Result types, not exceptions in business logic?
• [ ] Errors handled with .fold() in ViewModels?

---

For comprehensive details, always refer to:

• docs/ARCHITECTURE.md - Full architecture patterns
• docs/STYLE_AND_BEST_PRACTICES.md - Complete style guide

Finding Priority Framework

Use this framework to classify findings during code review. Clear prioritization helps authors triage and address issues effectively.

Table of Contents

Severity Categories:

• ❌ CRITICAL (Blocker - Must Fix Before Merge)
• ⚠️ IMPORTANT (Should Fix)
• ♻️ DEBT (Technical Debt)
• 🎨 SUGGESTED (Nice to Have)
• ❓ QUESTION (Seeking Clarification)
• Optional (Acknowledge But Don't Require)

Guidelines:

• Classification Guidelines
• When Something is Between Categories
• Context Matters
• Examples by Change Type
• Special Cases
• Summary

---

❌ CRITICAL (Blocker - Must Fix Before Merge)

These issues must be addressed before the PR can be merged. They pose immediate risks to security, stability, or architecture integrity.

Security

• Data leaks or plaintext sensitive data (passwords, keys, tokens)
• Weak encryption or insecure key storage
• Missing authentication or authorization checks
• Input injection vulnerabilities (SQL, XSS, command injection)
• Sensitive data in logs or error messages

Example:

**data/vault/VaultRepository.kt:145** - CRITICAL: PIN stored without encryption
PIN must be encrypted using Android Keystore, not stored in plaintext SharedPreferences.
Reference: docs/ARCHITECTURE.md#security

Stability

• Compilation errors or warnings
• Null pointer exceptions in production paths
• Resource leaks (file handles, network connections, memory)
• Crashes or unhandled exceptions in critical paths
• Thread safety violations

Example:

**app/auth/BiometricRepository.kt:120** - CRITICAL: Missing null safety check
biometricPrompt result can be null. Add explicit null check to prevent crash.

Architecture

• Mutable state exposure in ViewModels (violates MVVM)
• Exception-based error handling in business logic (should use Result)
• Circular dependencies between modules
• Violation of zero-knowledge principles
• Direct dependency instantiation (should use DI)

Example:

**app/login/LoginViewModel.kt:45** - CRITICAL: Exposes mutable state
Change MutableStateFlow to StateFlow in public API to prevent external state mutation.
This violates MVVM encapsulation pattern.

---

⚠️ IMPORTANT (Should Fix)

These issues should be addressed but don't block merge if there's a compelling reason. They improve code quality, maintainability, or robustness.

Testing

• Missing tests for critical paths (authentication, encryption, data sync)
• Missing tests for new public APIs
• Tests that don't verify actual behavior (test implementation, not behavior)
• Missing test coverage for error scenarios

Example:

**data/auth/BiometricRepository.kt** - IMPORTANT: Missing test for cancellation
Add test for user cancellation scenario to prevent regression.

Architecture

• Inconsistent patterns within PR (mixing error handling approaches)
• Poor separation of concerns
• Tight coupling between components
• Not following established project patterns

Example:

**app/vault/VaultViewModel.kt:89** - IMPORTANT: Business logic in ViewModel
Encryption logic should be in Repository, not ViewModel.
Reference: docs/ARCHITECTURE.md#mvvm-pattern

Documentation

• Undocumented public APIs (missing KDoc)
• Missing documentation for complex algorithms
• Unclear naming or confusing interfaces

Example:

**core/crypto/EncryptionManager.kt:34** - IMPORTANT: Missing KDoc
Public encryption method should document parameters, return value, and exceptions.

Performance

• Inefficient algorithms in hot paths (with evidence from profiling)
• Blocking main thread with I/O operations
• Memory inefficient data structures (with evidence)

Example:

**app/vault/VaultListViewModel.kt:78** - IMPORTANT: N+1 query pattern
Fetching items one-by-one in loop. Consider batch fetch to reduce database queries.

---

♻️ DEBT (Technical Debt)

Code that duplicates existing patterns, violates established conventions, or will require rework within 6 months. Introduces technical debt that should be tracked for future cleanup.

Duplication

• Copy-pasted code blocks across files
• Repeated validation or business logic
• Multiple implementations of same pattern
• Data transformation duplicated in multiple places

Example:

**app/vault/VaultListViewModel.kt:156** - DEBT: Duplicates encryption logic
Same encryption pattern exists in VaultRepository.kt:234 and SyncManager.kt:89.
Extract to shared EncryptionUtil to reduce maintenance burden.

Convention Violations

• Inconsistent error handling approaches within same module
• Mixing architectural patterns (MVVM + MVC)
• Not following established DI patterns
• Deviating from project code style significantly

Example:

**data/auth/AuthRepository.kt:78** - DEBT: Exception-based error handling
Project standard is Result<T> for error handling. This uses try-catch with throws.
Creates inconsistency and makes testing harder.
Reference: docs/ARCHITECTURE.md#error-handling

Future Rework Required

• Hardcoded values that should be configurable
• Temporary workarounds without TODO/FIXME
• Code that will need changes when planned features arrive
• Tight coupling that prevents future extensibility

Example:

**app/settings/SettingsViewModel.kt:45** - DEBT: Hardcoded feature flags
Feature flags should come from remote config for A/B testing.
Will require rework when experimentation framework launches.

---

🎨 SUGGESTED (Nice to Have)

Improvements with measurable value only. A finding qualifies as SUGGESTED if it provides: security gain, cyclomatic complexity reduction, bug class prevention, or elimination of an O(n²) pattern. Subjective style preferences, vague simplifications, and naming nitpicks do not qualify — leave those out entirely or raise in conversation.

Code Quality

• Extractable duplicated logic that reduces measurable complexity or improves testability
• Patterns that would prevent a recurring bug class in this module
• Architecture improvements that eliminate tight coupling with measurable impact

Example:

**app/vault/VaultScreen.kt:145** - SUGGESTED: Consider extracting helper function
This 20-line block appears in 3 places. Consider extracting to reduce duplication.

Testing

• Additional test coverage for edge cases (beyond critical paths)
• More comprehensive integration tests
• Performance tests for non-critical paths

Example:

**app/login/LoginViewModelTest.kt** - SUGGESTED: Add test for concurrent login attempts
Not critical, but would increase confidence in edge case handling.

Refactoring

• Extracting reusable patterns
• Modernizing old patterns (if touching related code)
• Improving testability

Example:

**data/vault/VaultRepository.kt:200** - SUGGESTED: Consider extracting validation logic
Could be extracted to separate validator class for reusability and testing.

---

❓ QUESTION (Seeking Clarification)

Questions about requirements, unclear intent, or potential conflicts that require human knowledge to answer. Open inquiries that cannot be resolved through code inspection alone.

Requirements Clarification

• Ambiguous acceptance criteria
• Multiple valid implementation approaches
• Unclear business rules or edge case handling
• Conflicting requirements between specs and implementation

Example:

**app/vault/ItemListViewModel.kt:67** - QUESTION: Expected sort behavior for equal timestamps?
When items have identical timestamps, should secondary sort be by:
- Name (alphabetical)
- Creation order
- Item type priority

Spec doesn't specify tie-breaking logic.

Design Decisions

• Architecture choices that could go multiple ways
• Trade-offs between approaches without clear winner
• Feature flag strategy or rollout approach
• API design with multiple valid patterns

Example:

**data/sync/SyncManager.kt:134** - QUESTION: Should sync failures retry automatically?
Current implementation fails immediately. Options:
- Exponential backoff (3 retries)
- User-triggered retry only
- Background retry on network restore

What's the expected UX?

System Integration

• Unclear contracts with external systems
• Potential conflicts with other features/modules
• Assumptions about third-party API behavior
• Cross-team coordination needs

Example:

**app/auth/BiometricPrompt.kt:89** - QUESTION: Compatibility with pending device credentials PR?
PR #1234 is refactoring device credentials. Should this:
- Merge first and adapt later
- Wait for #1234 to land
- Coordinate with that author

Timing unclear.

Testing Strategy

• Uncertainty about test scope or approach
• Questions about mocking external dependencies
• Edge cases that need product input
• Performance testing requirements

Example:

**data/vault/EncryptionTest.kt:45** - QUESTION: Should we test against real Keystore?
Currently using mocked Keystore. Real Keystore testing would:
+ Catch hardware-specific issues
- Slow down CI significantly
- Require API 23+ emulators

What's the priority?

---

Optional (Acknowledge But Don't Require)

Note good practices to reinforce positive patterns. Keep these brief - list only, no elaboration.

Good Practices

Format: Simple bullet list, no explanation

## Good Practices
- Proper Hilt DI usage throughout
- Comprehensive unit test coverage
- Clear separation of concerns
- Well-documented public APIs

Don't do this (too verbose):

## Good Practices
- Proper Hilt DI usage throughout: Great job using @Inject constructor and injecting interfaces! This follows our established patterns perfectly and makes the code very testable. Really excellent work here! 👍

---

Classification Guidelines

When Something is Between Categories

If unsure between Critical and Important:

• Ask: "Could this cause production incidents, data loss, or security breaches?"
• If yes → Critical
• If no → Important

If unsure between Important and Debt:

• Ask: "Is this a bug/defect or just duplication/inconsistency?"
• If bug/defect → Important
• If duplication/inconsistency → Debt

If unsure between Important and Suggested:

• Ask: "Would I block merge over this?"
• If yes → Important
• If no → Suggested

If unsure between Debt and Suggested:

• Ask: "Will this require rework within 6 months?"
• If yes → Debt
• If no → Suggested

If unsure between Suggested and Question:

• Ask: "Am I requesting a change or asking for clarification?"
• If requesting change → Suggested
• If seeking clarification → Question

If unsure between Suggested and Optional:

• Ask: "Is this actionable feedback or just acknowledgment?"
• If actionable → Suggested
• If acknowledgment → Optional

Context Matters

Same issue, different contexts:

// Critical for production code
Missing null safety check in auth flow → CRITICAL

// Suggested for internal test utility
Missing null safety check in test helper → SUGGESTED

Same pattern, different risk levels:

// Critical for new feature
Missing tests for new auth method → CRITICAL

// Important for bug fix
Missing regression test → IMPORTANT

// Suggested for refactoring
Missing tests for refactored helper → SUGGESTED

---

Examples by Change Type

Dependency Update

• Critical: Known CVEs in old version not addressed
• Important: Breaking changes that need migration
• Suggested: Beta/alpha version stability concerns

Bug Fix

• Critical: Fix doesn't address root cause
• Important: Missing regression test
• Suggested: Similar bugs in related code

Feature Addition

• Critical: Security vulnerabilities, architecture violations
• Important: Missing tests for critical paths
• Suggested: Additional test coverage, minor refactoring

UI Refinement

• Critical: Missing accessibility for key actions
• Important: Not using theme (hardcoded colors)
• Suggested: Minor spacing/alignment improvements

Refactoring

• Critical: Changes behavior (should be behavior-preserving)
• Important: Incomplete migration (mix of old/new patterns)
• Suggested: Additional instances that could be refactored

Infrastructure

• Critical: Hardcoded secrets, no rollback plan
• Important: Performance regression in build times
• Suggested: Further optimization opportunities

---

Special Cases

Technical Debt

• Acknowledge existing tech debt but don't require fixing in unrelated PR
• Exception: If change makes tech debt worse, it's Important to address

Scope Creep

• Don't request changes outside PR scope
• Can note as "Future consideration" but not required for this PR

Linter-Catchable Issues

• Don't flag issues that automated tools handle
• Exception: If linter is misconfigured and missing real issues

Personal Preferences

• Don't flag unless grounded in project standards or architectural principles
• Use "I-statements" if suggesting alternative approaches

---

Summary

Critical: Block merge, must fix (security, stability, architecture) Important: Should fix before merge (testing, quality, performance) Debt: Technical debt introduced, track for future cleanup Suggested: Nice to have, consider effort vs benefit Question: Seeking clarification on requirements or design Optional: Acknowledge good practices, keep brief

Review Psychology: Constructive Feedback Phrasing

Effective code review feedback is clear, actionable, and constructive. This guide provides phrasing patterns for inline comments.

Table of Contents

Guidelines:

• Phrasing Templates
• Critical Issues (Prescriptive)
• Suggested Improvements (Exploratory)
• Questions (Collaborative)
• Test Suggestions
• When to Be Prescriptive vs Ask Questions
• Special Cases

---

Phrasing Templates

Critical Issues (Prescriptive)

Pattern: State problem + Provide solution + Explain why

**[file:line]** - CRITICAL: [Issue description]

[Specific fix with code example if applicable]

[Rationale explaining why this is critical]

Reference: [docs link if applicable]

Example:

**data/vault/VaultRepository.kt:145** - CRITICAL: PIN stored without encryption

PIN must be encrypted using Android Keystore, not stored in plaintext SharedPreferences.
Plaintext storage exposes the PIN to backup systems and rooted devices.

Reference: docs/ARCHITECTURE.md#security

---

Suggested Improvements (Exploratory)

Pattern: Observe + Suggest + Explain benefit

**[file:line]** - Consider [alternative approach]

[Current observation]
Can we [specific suggestion]?

[Benefit or rationale]

Example:

**app/login/LoginScreen.kt:89** - Consider using existing BitwardenButton

This custom button implementation looks similar to `ui/components/BitwardenButton.kt:45`.
Can we use the existing component to maintain consistency across the app?

---

Questions (Collaborative)

Pattern: Ask + Provide context (optional)

**[file:line]** - [Question about intent or approach]?

[Optional context or observation]

Example:

**data/sync/SyncManager.kt:234** - How does this handle concurrent sync attempts?

It looks like multiple coroutines could call `startSync()` simultaneously.
Is there a mechanism to prevent race conditions, or is that handled elsewhere?

---

Test Suggestions

Pattern: Observe gap + Suggest specific test + Provide skeleton

**[file:line]** - Consider adding test for [scenario]

[Rationale]

@Test fun test description() = runTest { // Test skeleton }

Example:

**data/auth/BiometricRepository.kt** - Consider adding test for cancellation scenario

This would prevent regression of the bug you just fixed:

@Test fun when biometric cancelled then returns cancelled state() = runTest { coEvery { biometricPrompt.authenticate() } returns null

val result = repository.authenticate()

assertEquals(AuthResult.Cancelled, result) }

---

When to Be Prescriptive vs Ask Questions

Be Prescriptive (Tell them what to do):

• Security issues
• Architecture pattern violations
• Null safety problems
• Compilation errors
• Documented project standards

Ask Questions (Seek explanation):

• Design decisions with multiple valid approaches
• Performance trade-offs without data
• Unclear intent or reasoning
• Scope decisions (this PR vs future work)
• Patterns not documented in project guidelines

---

Special Cases

Nitpicks - For truly minor suggestions, use "Nit:" prefix:

**Nit**: Extra blank line at line 145

Uncertainty - If unsure, acknowledge it:

I'm not certain, but this might be called frequently.
Has this been profiled?

Positive Feedback - Brief list only, no elaboration:

## Good Practices
- Proper Hilt DI usage throughout
- Comprehensive unit test coverage
- Clear separation of concerns

Security Patterns Quick Reference

Quick reference for Bitwarden Android security patterns during code reviews. For comprehensive details, read docs/ARCHITECTURE.md#security.

Encryption and Key Storage

✅ GOOD - Android Keystore:

// Sensitive data encrypted with Keystore
class SecureStorage @Inject constructor(
    private val keystoreManager: KeystoreManager
) {
    suspend fun storePin(pin: String): Result<Unit> = runCatching {
        val encrypted = keystoreManager.encrypt(pin.toByteArray())
        securePreferences.putBytes(KEY_PIN, encrypted)
    }
}

// Or use EncryptedSharedPreferences
val encryptedPrefs = EncryptedSharedPreferences.create(
    context,
    "secure_prefs",
    masterKey,
    EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
    EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
)

❌ BAD - Plaintext or weak encryption:

// ❌ CRITICAL - Plaintext storage
sharedPreferences.edit {
    putString("pin", userPin)  // Never store sensitive data in plaintext
}

// ❌ CRITICAL - Weak encryption
val cipher = Cipher.getInstance("DES")  // Use AES-256-GCM

// ❌ CRITICAL - Hardcoded keys
val key = "my_secret_key_123"  // Use Android Keystore

Key Rules:

• Use Android Keystore for encryption keys
• Use EncryptedSharedPreferences for simple key-value storage
• Use AES-256-GCM for encryption
• Never store sensitive data in plaintext
• Never hardcode encryption keys

Reference: docs/ARCHITECTURE.md#security

---

Logging Sensitive Data

✅ GOOD - No sensitive data:

Log.d(TAG, "Authentication attempt for user")
Log.d(TAG, "Vault sync completed with ${items.size} items")

❌ BAD - Logs sensitive data:

// ❌ CRITICAL
Log.d(TAG, "Password: $password")
Log.d(TAG, "Auth token: $token")
Log.d(TAG, "PIN: $pin")
Log.d(TAG, "Encryption key: ${key.encoded}")

Key Rules:

• Never log passwords, PINs, tokens, keys
• Never log encryption keys or sensitive data
• Be careful with error messages (don't include sensitive context)

---

Quick Checklist

Security

• [ ] Sensitive data encrypted with Keystore?
• [ ] No plaintext passwords/keys?
• [ ] No sensitive data in logs?
• [ ] Using AES-256-GCM for encryption?
• [ ] No hardcoded encryption keys?

---

For comprehensive security details, always refer to:

• docs/ARCHITECTURE.md#security - Complete security architecture and zero-knowledge principles

Style Patterns Quick Reference

Project-specific Kotlin style rules to catch during code review. These supplement (not replace) docs/STYLE_AND_BEST_PRACTICES.md.

when branches with wrapped right-hand side require curly braces

When a when branch's expression is too long to fit on the same line as -> and is wrapped to its own line, the body must be wrapped in { }. A bare -> followed by an indented expression on the next line should be flagged.

Flag this:

when (type) {
    VaultItemCipherType.LOGIN -> VaultAddEditState.ViewState.Content.ItemType.Login()
    VaultItemCipherType.BANK_ACCOUNT ->
        VaultAddEditState.ViewState.Content.ItemType.BankAccount()
}

Accept this:

when (type) {
    VaultItemCipherType.LOGIN -> VaultAddEditState.ViewState.Content.ItemType.Login()
    VaultItemCipherType.BANK_ACCOUNT -> {
        VaultAddEditState.ViewState.Content.ItemType.BankAccount()
    }
}

Single-line branches (body fits alongside ->) do not require braces.

Suggested classification: SUGGESTED (style consistency, not correctness).

Testing Patterns Quick Reference

Quick reference for Bitwarden Android testing patterns during code reviews. For comprehensive details, read docs/ARCHITECTURE.md and docs/STYLE_AND_BEST_PRACTICES.md.

ViewModel Tests

✅ GOOD - Tests behavior:

@Test
fun `when login succeeds then state updates to success`() = runTest {
    // Arrange
    val viewModel = LoginViewModel(mockRepository)
    coEvery { mockRepository.login(any(), any()) } returns Result.success(User())

    // Act
    viewModel.onLoginClicked("user@example.com", "password")

    // Assert
    viewModel.state.test {
        assertEquals(LoginState.Loading, awaitItem())
        assertEquals(LoginState.Success, awaitItem())
    }
}

❌ BAD - Tests implementation:

@Test
fun `repository is called with correct parameters`() {
    // ❌ This tests implementation details, not behavior
    viewModel.onLoginClicked("user", "pass")
    coVerify { mockRepository.login("user", "pass") }
}

Key Rules:

• Test behavior, not implementation
• Use runTest for coroutine tests
• Use Turbine for Flow testing
• Use MockK for mocking

---

Repository Tests

✅ GOOD - Tests data transformations:

@Test
fun `fetchItems maps API response to domain model`() = runTest {
    // Arrange
    val apiResponse = listOf(ApiItem(id = "1", name = "Test"))
    coEvery { apiService.getItems() } returns apiResponse

    // Act
    val result = repository.fetchItems()

    // Assert
    assertTrue(result.isSuccess)
    assertEquals(
        listOf(DomainItem(id = "1", name = "Test")),
        result.getOrThrow()
    )
}

Key Rules:

• Test data transformations
• Test error handling (network failures, API errors)
• Test caching behavior if applicable
• Mock API services and databases

Reference: Project uses JUnit 5, MockK, Turbine, kotlinx-coroutines-test

---

Null Safety

✅ GOOD - Safe handling:

// Safe call with elvis operator
val result = apiService.getData() ?: return State.Error("No data")

// Let with safe call
intent?.getStringExtra("key")?.let { value ->
    processValue(value)
}

// Require with message
val data = requireNotNull(response.data) { "Response data must not be null" }

❌ BAD - Unsafe assertions:

// ❌ Unsafe - can crash
val result = apiService.getData()!!

// ❌ Platform type unchecked
val intent: Intent = getIntent()  // Could be null from Java
val value = intent.getStringExtra("key")  // Potential NPE

Key Rules:

• Avoid !! unless safety is guaranteed (rare)
• Handle platform types with explicit nullability
• Use safe calls (?.), elvis operator (?:), or explicit checks
• Use requireNotNull with descriptive message if crash is acceptable

---

Quick Checklist

Testing

• [ ] ViewModels have unit tests?
• [ ] Tests verify behavior, not implementation?
• [ ] Edge cases covered?
• [ ] Error scenarios tested?

Code Quality

• [ ] Null safety handled properly (no !! without guarantee)?
• [ ] Public APIs have KDoc?
• [ ] Following naming conventions?

---

For comprehensive details, always refer to:

• docs/ARCHITECTURE.md - Full architecture patterns
• docs/STYLE_AND_BEST_PRACTICES.md - Complete style guide

Compose UI Patterns Quick Reference

Quick reference for Bitwarden Android Compose UI patterns during code reviews. For comprehensive details, read docs/ARCHITECTURE.md and docs/STYLE_AND_BEST_PRACTICES.md.

Component Reuse

✅ GOOD - Uses existing components:

BitwardenButton(
    text = "Submit",
    onClick = onSubmit
)

BitwardenTextField(
    value = text,
    onValueChange = onTextChange,
    label = "Email"
)

❌ BAD - Duplicates existing components:

// ❌ Recreating BitwardenButton
Button(
    onClick = onSubmit,
    colors = ButtonDefaults.buttonColors(
        containerColor = BitwardenTheme.colorScheme.primary
    )
) {
    Text("Submit")
}

Key Rules:

• Check :ui module for existing components before creating custom ones
• Use BitwardenButton, BitwardenTextField, etc. for consistency
• Place new reusable components in :ui module

---

Theme Usage

✅ GOOD - Uses theme:

Text(
    text = "Title",
    style = BitwardenTheme.typography.titleLarge,
    color = BitwardenTheme.colorScheme.primary
)

Spacer(modifier = Modifier.height(16.dp))  // Standard spacing

❌ BAD - Hardcoded values:

Text(
    text = "Title",
    style = TextStyle(fontSize = 24.sp, fontWeight = FontWeight.Bold),  // Use theme
    color = Color(0xFF0066FF)  // Use theme color
)

Spacer(modifier = Modifier.height(17.dp))  // Non-standard spacing

Key Rules:

• Use BitwardenTheme.colorScheme for colors
• Use BitwardenTheme.typography for text styles
• Use standard spacing (4.dp, 8.dp, 16.dp, 24.dp)

---

Quick Checklist

UI Patterns

• [ ] Using existing Bitwarden components from :ui module?
• [ ] Using BitwardenTheme for colors and typography?
• [ ] Using standard spacing values (4, 8, 16, 24 dp)?
• [ ] No hardcoded colors or text styles?
• [ ] UI is stateless (observes state, doesn't modify)?

---

For comprehensive details, always refer to:

• docs/ARCHITECTURE.md - Full architecture patterns
• docs/STYLE_AND_BEST_PRACTICES.md - Complete style guide