mirror of
https://github.com/bitwarden/android.git
synced 2026-05-03 13:48:37 -05:00
Enhance code review skill documentation with TOCs and missing severity categories (#6186)
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Patrick Honkonen
GitHub
@@ -1,8 +1,28 @@
|
||||
# Issue Priority Framework
|
||||
# Finding Priority Framework
|
||||
|
||||
Use this framework to classify findings during code review. Clear prioritization helps authors triage and address issues effectively.
|
||||
|
||||
## Critical (Blocker - Must Fix Before Merge)
|
||||
## Table of Contents
|
||||
|
||||
**Severity Categories:**
|
||||
- [❌ CRITICAL (Blocker - Must Fix Before Merge)](#critical-blocker---must-fix-before-merge)
|
||||
- [⚠️ IMPORTANT (Should Fix)](#important-should-fix)
|
||||
- [♻️ DEBT (Technical Debt)](#debt-technical-debt)
|
||||
- [🎨 SUGGESTED (Nice to Have)](#suggested-nice-to-have)
|
||||
- [💭 QUESTION (Seeking Clarification)](#question-seeking-clarification)
|
||||
- [Optional (Acknowledge But Don't Require)](#optional-acknowledge-but-dont-require)
|
||||
|
||||
**Guidelines:**
|
||||
- [Classification Guidelines](#classification-guidelines)
|
||||
- [When Something is Between Categories](#when-something-is-between-categories)
|
||||
- [Context Matters](#context-matters)
|
||||
- [Examples by Change Type](#examples-by-change-type)
|
||||
- [Special Cases](#special-cases)
|
||||
- [Summary](#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.
|
||||
|
||||
@@ -49,7 +69,7 @@ This violates MVVM encapsulation pattern.
|
||||
|
||||
---
|
||||
|
||||
## Important (Should Fix)
|
||||
## ⚠️ **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.
|
||||
|
||||
@@ -102,7 +122,53 @@ Fetching items one-by-one in loop. Consider batch fetch to reduce database queri
|
||||
|
||||
---
|
||||
|
||||
## Suggested (Nice to Have)
|
||||
## ♻️ **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)
|
||||
|
||||
These are improvement opportunities but not required. Consider the effort vs. benefit before requesting changes.
|
||||
|
||||
@@ -142,6 +208,80 @@ 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.
|
||||
@@ -175,11 +315,26 @@ Note good practices to reinforce positive patterns. Keep these **brief** - list
|
||||
- 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
|
||||
@@ -270,5 +425,7 @@ Missing tests for refactored helper → SUGGESTED
|
||||
|
||||
**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
|
||||
|
||||
Reference in New Issue
Block a user