Skip to content

Commit 4db8243

Browse files
wmontwewmontwe
committed
docs: add code review guide
1 parent 6c4bea5 commit 4db8243

File tree

2 files changed

+112
-0
lines changed

2 files changed

+112
-0
lines changed

‎docs/SUMMARY.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ generator, in this case, **mdbook**. It defines the structure and navigation of
77

88
- [Contributing](CONTRIBUTING.md)
99
- [Code Quality Guide](contributing/code-quality-guide.md)
10+
- [Code Review Guide](contributing/code-review-guide.md)
1011
- [Git Commit Guide](contributing/git-commit-guide.md)
1112
- [Testing Guide](contributing/testing-guide.md)
1213
- [Java to Kotlin Conversion Guide](contributing/java-to-kotlin-conversion-guide.md)

‎docs/contributing/code-review-guide.md‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
# 👁️ Code Review Guide
2+
3+
This guide outlines best practices for creating and reviewing pull requests (PRs) in the Thunderbird for Android
4+
project. It is intended to help both authors and reviewers ensure high-quality contributions.
5+
6+
### ✅ Quick PR checklist (for authors)
7+
8+
Paste this into your PR to self-check:
9+
10+
- [ ] Focused scope (< ~800 LOC); clear description and rationale
11+
- [ ] UI changes: screenshots/videos; accessibility (TalkBack, contrast, touch targets)
12+
- [ ] Tests added/updated (AAA; assertK; deterministic; edge cases); CI green
13+
- [ ] Architecture: business logic outside UI; module API/impl respected; DI via constructor/Koin
14+
- [ ] Performance: no main-thread blocking; Compose recompositions reasonable; hot paths allocation-lean
15+
- [ ] Security/privacy: inputs validated; no PII in logs; TLS; secure storage; permission changes documented
16+
- [ ] i18n: No new localizable strings unless justified; translations policy followed
17+
- [ ] Release train: feature flags set; uplift label + risk/impact (if applicable)
18+
- [ ] Docs/CHANGELOG updated; issues linked; PR title/commits clear
19+
20+
## 🧑‍💻 For Code Authors (self‑review checklist)
21+
22+
1. **Scope and clarity**
23+
- Keep the PR focused on a single concern; split large or mixed changes.
24+
- Keep PRs small (aim for <~ 800 lines of code (LOC))
25+
- Provide a clear description: problem, approach, rationale, alternatives considered.
26+
- For UI changes: include screenshots/videos for UI changes and note any UX impacts.
27+
- Use Draft PRs for early feedback
28+
2. **Tests**
29+
- Include tests matching the change type (unit/integration/UI).
30+
- Use AAA pattern and use assertK. Prefer fakes over mocks.
31+
- Name tests descriptively; cover edge cases and error conditions.
32+
3. **Architecture & module boundaries**
33+
- Follow modular rules: API vs implementation separation; no leaking implementation across module boundaries.
34+
- Only depend on `:feature:foo:api` externally; `:feature:foo:impl` is internal.
35+
- Respect MVI/Compose patterns in the UI layer; keep business logic out of UI implementation.
36+
- Prefer constructor injection with Koin; keep constructors simple and dependencies explicit.
37+
4. **Code quality & style**
38+
- Keep functions small, clear naming, avoid duplication.
39+
- Add KDoc for public API.
40+
- Run Spotless, Detekt and Lint locally.
41+
5. **Performance & threading**
42+
- Use coroutines with appropriate dispatchers; avoid blocking the main thread.
43+
- Watch allocations in hot paths, avoid unnecessary recompositions in Compose.
44+
- For critical changes, check baseline profiling or startup metrics.
45+
6. **Security & privacy**
46+
- Validate inputs, avoid logging Personally Identifiable Information (PII).
47+
- Use TLS and safe storage APIs.
48+
- Review permission use and document your rationale to them in the PR description.
49+
7. **Accessibility**
50+
- Provide `contentDescription` and TalkBack support.
51+
- Ensure sufficient contrast, touch targets and dynamic text sizing (up to 200%).
52+
8. **i18n**
53+
- Follow strings policy: don’t modify translations here; avoid late string changes; see [translations](../translations.md).
54+
- No string concatenation with localized text; use placeholders.
55+
9. **Feature flags & release train awareness**
56+
- Gate incomplete features behind flags aligned with branch rules [Release - Feature Flags](../ci/RELEASE.md#feature-flags).
57+
- For uplifts: add label and risk/impact notes [Release - Branch uplifts](../ci/RELEASE.md#branch-uplifts).
58+
10. **Documentation & metadata**
59+
- Update relevant docs, CHANGELOG entries and add context as needed.
60+
- Link relevant issues using GitHub keywords so they auto-close on merge (`Fixes #123`, `Resolves #456`).
61+
11. **CI status**
62+
- Ensure CI is green: fix reported issues or request re-run if failures are unrelated.
63+
64+
## 👀 For Code Reviewers (what to look for)
65+
66+
1. **Correctness & requirements**
67+
- Does the change solve the stated problem? Any edge cases missed? Are invariants upheld?
68+
2. **Architecture & boundaries**
69+
- Adheres to module API/impl separation and project architecture (UI: Compose/MVI, Domain, Data).
70+
- No cross‑module leaks; dependencies flow in the right direction.
71+
3. **Readability & maintainability**
72+
- Code is easy to follow; good names; small functions; comments where necessary; public APIs documented.
73+
4. **Test quality**
74+
- Adequate tests exist and are meaningful.
75+
- Negative/error paths covered.
76+
- Tests are deterministic and prefer fakes.
77+
5. **Performance**
78+
- No obvious inefficiencies; avoids allocations on hot paths.
79+
- Background work is appropriate.
80+
- Compose recomposition reasonable.
81+
6. **Security, privacy, and permissions**
82+
- No new vulnerabilities; safe defaults; least privilege permissions.
83+
- Secrets not committed; logs avoid personal identifiable information.
84+
- Permission rationale provided if applicable.
85+
7. **Accessibility & i18n**
86+
- Accessible UI; strings externalized; no hard‑coded locales.
87+
- Respects translations policy, only english source files.
88+
8. **Consistency & style**
89+
- Matches existing patterns.
90+
- Formatting and static analysis clean (Spotless/Detekt/Lint).
91+
9. **Release train considerations**
92+
- Feature flags set correctly for target branch
93+
- Consider if an uplift is necessary.
94+
10. **CI status**
95+
- CI is green -> good to merge.
96+
- If failures are unrelated or flaky, do a re-run OR leave a note.
97+
- Don’t merge with failing checks!
98+
99+
## 🤝 Review etiquette
100+
101+
- Be kind, specific, and actionable.
102+
- Prefer questions over directives. Explain trade‑offs.
103+
- Use severity tags if appropriate to weight your comments:
104+
- Nit: trivial style/readability; non-blocking.
105+
- Suggestion: improves design/maintainability; author’s call.
106+
- Blocking: must be addressed for correctness, safety, or architecture.
107+
- Avoid scope creep and request follow‑ups for non‑critical issues.
108+
- Acknowledge good practices and improvements.
109+
- When disagreeing, provide reasoning and seek consensus.
110+
- Use GitHub suggestions for trivial fixes where possible.
111+

0 commit comments

Comments
 (0)